Skip to content

fix(sdk): renew agent leases while CLI steps execute - #249

Closed
kjgbot wants to merge 1 commit into
mainfrom
lane/agent-lease-renewal-0909
Closed

kjgbot wants to merge 1 commit into
mainfrom
lane/agent-lease-renewal-0909

Conversation

@kjgbot

@kjgbot kjgbot commented Sep 9, 2026 •

Copy link
Copy Markdown
Contributor

Agent CLI steps that exceed the kernel's 30-second lease currently lose their attempt before reporting completion. Renew each attempt through the existing step.heartbeat protocol every 10 seconds while its CLI is running.

Stop renewal on CLI settlement, execution failure, or worker close. Drain an outstanding renewal before completing; preserve CLI output with worker_error on renewal failure and surface both renewal and completion errors when both fail. No kernel or timeout changes.

The real 34-second subprocess regression, simulated ten-minute case, lifecycle coverage, and literal command/output records are in the verification report. The report includes the original reproduction and the failure/pass pair after removing and restoring the fix byte-for-byte. It states the selected test scope and the prebuilt kernel used.

Closing stops renewals immediately, so a CLI still draining after close can report lease expiry. This preserves the requested shutdown contract. Ready for human review; do not merge automatically.


Note

Medium Risk
Changes core agent step execution and journal completion semantics for long steps and shutdown; mistakes could cause lease conflicts or incorrect completion reasons, though scope is isolated to the SDK worker.

Overview
AgentWorker now keeps the kernel attempt lease alive for long-running agent CLIs by calling existing step.heartbeat on a 10s cadence (inside the 30s lease), via new startWorkerHeartbeat.

Renewal is non-overlapping (next tick only after the prior heartbeat ack), stops when the CLI finishes or throws, and close() stops all heartbeats immediately—even if a CLI is still draining, which can surface lease conflicts. stop() waits for an in-flight heartbeat before stepComplete so completion cannot race a late renewal. Failed renewals force worker_error completion while still journaling CLI output; renewal, completion, and CLI errors collapse to a single error or AggregateError.

Adds deterministic Vitest coverage (34s / 10min simulated steps, concurrency, close races) plus an opt-in live 34s subprocess test when RELAYFLOWD_BIN is set, and an evidence doc with reproduction and mutation checks. SDK-only; no kernel or protocol changes.

Reviewed by Cursor Bugbot for commit 33297fa. Bugbot is set up for automated code reviews on this repo. Configure here.

@coderabbitai

coderabbitai Bot commented Sep 9, 2026

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: d2b86c12-4edb-437a-876e-5bae16444b9f


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown

Review swarm: maintainability

No fresh transcript was produced for run b9375c4a-44ba-48c2-9390-edb97e236d0f (MISSING).

@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown

Review swarm: history

No fresh transcript was produced for run b9375c4a-44ba-48c2-9390-edb97e236d0f (MISSING).

@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown

Review swarm: structure

No fresh transcript was produced for run b9375c4a-44ba-48c2-9390-edb97e236d0f (MISSING).

@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown

Review swarm: FAILED

  • maintainability: MISSING
  • history: MISSING
  • structure: MISSING

Cloud run: b9375c4a-44ba-48c2-9390-edb97e236d0f

@kjgbot

kjgbot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

maintainability lens — PASS

Maintainability review — PR #249 (agent lease renewal)

Blockers

None. The change is small and behaves correctly against the fixed-clock and live tests. Mutation evidence in docs/agent-lease-renewal-0909-evidence.md (mutation-red output, restored, retested) is comprehensive and reproducible.

Concerns

Shared errors array leaks the helper's contract — packages/sdk/src/worker-heartbeat.ts:9,27 returns errors as a public array that both the helper (line 27) and the caller push into (worker.ts:107,116). There is no ownership signal: is it a channel, a log, a queue? A future caller who imports startWorkerHeartbeat and never reads .errors will silently lose lease-loss information. The comment at line 25 states the caller's obligation but nothing enforces it. Consider returning stop() that resolves to the collected renewal errors, and letting the caller own aggregation.

worker_error collapses two distinct failure modes — worker.ts:124 returns worker_error when either the CLI exits non-zero OR heartbeat renewal fails. A stranger reading the journal cannot distinguish "the agent's CLI reported failure" from "the kernel lost my lease and this attempt is undead." The closed CompletionReason vocabulary (RFC-0001 decision #13) forces the merge, but the completion payload should carry the distinction — currently the only trace is the emitted worker error event, which is not journaled.

Heartbeat interval is not derived from the dispatched deadline — worker-heartbeat.ts:6 hardcodes 10s "comfortably inside the kernel's 30-second lease." StepDispatchEvent.lease_deadline_ms (protocol.ts:221) and StepHeartbeatResult.lease_deadline_ms (protocol.ts:230) both exist. If the kernel raises the lease later, a stranger will not know to touch this file — the two constants drift silently. Either derive the interval from the deadline or add an assertion that fails loudly on mismatch.

Notes

  • worker-heartbeat.ts:12 — let timer: ReturnType<typeof setTimeout>; is used unassigned in stop() before the first schedule. clearTimeout(undefined) is a no-op today; fragile if control flow moves. Initialize to undefined explicitly or type it as such.
  • The return type ReturnType<typeof startWorkerHeartbeat> at worker.ts:36 is opaque — introduce an exported WorkerHeartbeat interface so consumers can name the contract.
  • worker.ts:137 — the amendment "(unless renewal failed)" was patched into an existing multi-line comment; the sentence now reads awkwardly. Rework the paragraph.
  • worker-heartbeat.test.ts:157 — the "keeps another dispatch renewing" test asserts the sequence ['agent', 'second', 'second'] but doesn't independently verify that dispatch A's timer was cleared; a bug that renewed A once more on a delayed tick could still produce this trace under different timing.
  • Live test opt-in via RELAYFLOWD_BIN (worker-heartbeat-live.test.ts:11) means CI without the binary never crosses a real 30s lease boundary. AGENTS.md §5 is fine with local proof, but flag this so the gate is not misread as "CI proves it."
  • The signature startWorkerHeartbeat(client, dispatch) takes a full JournalClient while using only stepHeartbeat; narrower typing would document the coupling.

REVIEW_PASSED

@kjgbot

kjgbot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

history lens — PASS

Blockers: none. PR #249 passes the HISTORY lens at 33297fa310fe8aed503d90877a8defe12d6d2ac6.

  • Recorded mistakes: The change follows the diagnosis recorded in ops/DRIVE-LOG.md:6383–6416: renew leases in the SDK through the existing protocol instead of extending the kernel timeout. packages/sdk/src/worker-heartbeat.ts:4–29 implements that approach with serialized requests and stops after failure. I found no deliberately removed behavior reintroduced. The worker retains memory injection, wake context, model selection, and JSON-output promotion (packages/sdk/src/worker.ts:102–105,126–139).

  • Settled RFC decisions: No new contradiction found. Heartbeats use JournalClient.stepHeartbeat (packages/sdk/src/worker-heartbeat.ts:20–22), preserving decision regressions: red/green flows for the 2026-08-27 platform bugs (dormant until gates 2+6) #5’s protocol boundary. The diff introduces no provider logic into the kernel and edits no acceptance gate. Renewal failure prevents a success completion, and renewal/completion errors remain observable (packages/sdk/src/worker.ts:112–124), consistent with fail-closed handling.

  • Commit truthfulness: The sole commit subject, “fix(sdk): renew agent leases while CLI steps execute,” accurately describes the diff; its body is empty. The accompanying evidence document explicitly distinguishes the real subprocess from Claude or the full drive workflow, and simulated ten-minute coverage from wall-clock execution (docs/agent-lease-renewal-0909-evidence.md:11–18). It includes removal/restoration commands and captured failure/pass output, while disclaiming full-suite and RFC-gate success (:71–296).

Concern, non-blocking: close() stops renewal while awaiting active dispatches (packages/sdk/src/worker.ts:63–81). A sufficiently long drain can therefore lose its lease. This limitation is explicitly disclosed; the prior worker never renewed leases, so this does not remove previously working shutdown renewal behavior.

Notes: I read the requested history and operational guidance. This is a source/history review; I did not independently rerun the reported tests. Broader workload acceptance remains outside this verdict.

REVIEW_PASSED

@kjgbot

kjgbot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

structure lens — MISSING

@kjgbot

kjgbot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

🎯 review-swarm: FAILED (M:pass H:pass S:missing)

Lens transcripts posted as sibling comments above.

@kjgbot

kjgbot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

Closing as superseded — the objective is already solved on main, by a more
complete implementation.

What happened

This branch was committed at 2026-09-09 15:14:28 +0200. #247 landed
packages/sdk/src/worker-lease.ts on main at 16:37:13 +0200
, 83 minutes
later, and wired it up at packages/sdk/src/worker.ts:94. So the fix arrived
from another direction while this PR sat unmerged.

That is also what made this PR DIRTY: the conflict in worker.ts is not
textual drift, it is two different solutions to the same problem meeting in the
same function.

Why main's version is the one to keep

withWorkerLease does everything this PR's startWorkerHeartbeat does and
several things it does not:

  • renews on a timer at remaining / 3 rather than a fixed interval, so it
    adapts to whatever deadline the server issues;
  • aborts the running CLI through an AbortSignal when renewal fails, instead of
    letting a subprocess keep running under a lease we no longer hold;
  • rejects a renewal response that arrives after local expiry — "a response
    handled after local expiry cannot revive ownership, even if its future
    deadline was issued before this event loop stalled";
  • drains any in-flight renewal before step.complete, so a renewal racing
    completion cannot report a spurious lease error;
  • re-checks the wall clock before permitting completion, because timer callbacks
    can be delayed behind a resolved subprocess promise.

Merging this PR on top would not add coverage; it would fight that
implementation inside the same function.

The diagnosis still stands

The underlying finding this branch came from was correct and is worth keeping in
the record: the agent lease was 30s (LEASE_DURATION_MS in
kernel/relayflowd-core/src/machine.rs:18), a CLI step routinely ran ~34s,
renew_lease already existed in kernel/relayflowd/src/server.rs:317-372, and
AgentWorker never called it. I verified the repair end-to-end at the time —
merged this branch into a local worktree, rebuilt the SDK, ran the drive loop,
and the lease-expiry line was gone with the agent exiting 0.

That verification is exactly why I am comfortable closing rather than rebasing:
the behaviour it proved is the behaviour withWorkerLease now provides.

Not deleting the branch

Leaving lane/agent-lease-renewal-0909 in place. The evidence doc
(docs/agent-lease-renewal-0909-evidence.md) and the two heartbeat test files
may still be worth harvesting into the worker-lease tests, since they were
written against the live failure rather than the abstraction.

@kjgbot kjgbot closed this Sep 9, 2026
kjgbot pushed a commit that referenced this pull request Sep 9, 2026
The DIRTY state was a semantic conflict: main gained withWorkerLease 83 minutes
after #249's commit and wires it at worker.ts:94. Main's version renews at
remaining/3, aborts via AbortSignal, rejects post-expiry renewals and drains
before complete -- strictly more than the heartbeat. Closed with evidence,
branch kept for its tests.

Also caught a vacuous conflict probe (wrong merge-tree form + `|| echo 0`).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR
kjgbot pushed a commit that referenced this pull request Sep 9, 2026
…se-renewal

Checking target-not-liveness immediately surfaced lease-renewal-0909 alive on a
CLOSED #249. Re-verified 0 tracked changes / 0 unpushed / 0 writes in 60m before
stopping it; kept the worktree and branch for its harvestable tests.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant