fix(platform): keep in-flight runs alive across control-plane restarts

[JuanMarchetto]

What changes

An API or worker restart no longer kills in-flight runs (#35).

Runner. fetchJson used to retry only on HTTP 429. It now separates two failure classes. Codes that fire before the request could reach a route handler (connection refused, name resolution loss when a containerized API restarts, no route to host, connect timeout) prove nothing was committed, so every caller replays them: this is the control-plane-restart case. Codes where the request was already on the wire (reset, broken pipe, header and body timeouts) and transient 5xx do not prove that, so replaying them is at-least-once delivery and only an endpoint whose handler absorbs a duplicate opts in. Retries use exponential backoff with jitter under a bounded budget: 3 minutes for ordinary calls, 5 minutes for the terminal result. The runner spends the whole budget and always grants one retry, so a single stalled attempt cannot eat the budget by itself. On 5xx it honors Retry-After but keeps the backoff as a floor, so a proxy answering Retry-After: 0 cannot make every runner hammer it at once. The 429 path keeps its old semantics and stays exempt from the budget.

Whether an ambiguous loss may be replayed is a property of the handler on the far end, not of the caller, so it is declared once per endpoint in ENDPOINT_RETRY_POLICIES and api() looks it up from the path. api() takes no policy argument, so a call site cannot invent one, and an endpoint nobody has classified gets the conservative default rather than whatever its caller felt like. The two calls that do not go through api() read their entry from the same table by name. bundle, steer, transcript, session-state and result replay; hello, push-token and events do not. Each entry carries the handler behaviour that justifies it.

The session-state restore used to call fetch directly and got no outage tolerance at all. It now shares the same transport through a byte-reading caller, so it is classified like everything else.

Related runner changes:

• Event batches keep buffering through an outage. The existing single-flight batcher already applies backpressure; retry turns delivery failure into delivery delay, so order holds and no line is dropped.
• When delivery fails for good, the drain now resumes its source stream. Before, readline left the stream paused, a child writing into a full pipe blocked forever, and the failure aborted the process as an unhandled rejection before the result could post. The command timeout timer is cleared on that path too.
• Control messages are handled before they are acknowledged, and the acknowledgment names the ids whose durable action landed. A message whose response died on the wire, or whose handling threw, is served again.
• An interrupt is never retired. A steer that keeps failing is retired after CONTROL_MESSAGE_MAX_ATTEMPTS (3) with one steer_undeliverable event, because leaving it unacked would make it the oldest pending row of every batch forever. An operator's stop does not get that treatment: it keeps being retried for the life of the run. Nothing in the batch holds the line in front of anything else.
• The steering poll survives failed iterations, emits a steer_poll_degraded event once the transport returns, and exits only on a terminal run. When a served batch produces no new acknowledgment, the poll backs off (1 s doubling to 15 s, equal jitter) so a message that can never be acknowledged cannot spin the loop against the control plane.
• A replayed result post that gets 409 run_terminal counts as already recorded. The discarded outcome is written to the container log, the one channel still open at that point, from an allowlist of scalar coordinates: attempted status, changed, branch, head sha, and push error. Each field is bounded at 512 characters and the finished line passes through redactSecrets(), so the generated pull request title and body never reach a log store that sits outside the run event redaction boundary.

Control plane. Reconcile used to declare sandbox_lost from a single driver.status() probe. That verdict is irreversible: it revokes the run's keys, and every later runner call gets a 409. Now the first exited/lost observation only stamps lossObservedAt in the sandbox state, and the run fails only when the loss persists past SANDBOX_LOSS_GRACE_MS (90 s; with the 2-minute cron that means the next tick). A later probe that sees the sandbox alive clears the stamp.

Every write in that path compare-and-sets against the stamp the tick read, including the failure itself: the tick's snapshot is taken before a network probe per live run, so a concurrent tick that saw the sandbox alive and cleared the stamp must win. failRun takes an optional guard folded into its existing atomic claim and reports whether it claimed the row; updateGithubRunProgress is gated on that, which also stops a lost claim from rewriting a live run's progress comment to failed. reconcileSandboxes takes an injectable driver resolver for tests, mirroring DispatchRunDeps.

GET /internal/runs/:runId/steer no longer marks a message delivered because it was fetched. It marks exactly the ids the next poll acknowledges, scoped to the run and its org, and refuses an acknowledgment whole unless every id matches what newId("evt") produces, up to STEER_ACK_MAX (32). A runner launched before this change keeps its image for the life of its run, so a transitional branch answers a poll carrying only the old afterId cursor with the previous mark-on-select behaviour: without it, such a runner would be served the same row on every iteration and re-apply the same steer in a loop with no delay. An acknowledgment always takes precedence, and the cursor is validated the same way. That branch can be deleted once no sandbox launched before this change can still be polling.

Why

Three architect runs died as sandbox_lost in one day of dogfooding (#35). A tsx watch restart of the API made the runner's next call fail, the runner treated that as fatal and exited, and reconcile recorded the loss. A production API deploy does the same. The sandbox container is an independent process and the API is stateless per request, so the runs were recoverable the whole time. Two things made the loss real: the runner gave up, and reconcile judged from one probe.

Verification

• pnpm verify passes end to end: lint, typecheck, clean build, DB-backed suites with skips forbidden, guards, audit.
• The runner tests drive the real code against real node:http servers, no fake timers. Covered: a server killed and re-listened on the same port mid-call, 503-503-200 recovery, a 200 whose body is cut mid-flight, Retry-After honored when large and floored at zero, budget bounds plus the one guaranteed retry, 429 exempt from the budget, no retry on 4xx, transient classification of nested undici causes, and the per-endpoint policy split: an ambiguous mid-flight failure is replayed for a caller that opts in and rejected after exactly one attempt for one that does not. The policy table is not merely snapshotted: the suite derives the endpoints the runner actually calls from the source and fails if one is unclassified, or if a control-plane request bypasses the shared transport.
• The control channel is covered on both sides: an acknowledgment marks exactly the ids it names and nothing that was never served, a malformed or oversized acknowledgment mutates nothing, another run's and another org's messages are untouched, a lost response redelivers, an interrupt survives a control plane that dies mid-response and is applied exactly once, an interrupt is never retired while a steer is, and a stalled poll backs off.
• Two tiers cover reconcile grace: the pure predicate in orchestrator-checks.test.ts, and sandbox.test.ts against real Postgres, including a run whose stamp is cleared while its probe is in flight, which must not be failed and must keep its keys.
• The tests bite: with each source change reverted and the tests kept, the corresponding tests fail.
• The Docker sandbox E2E (FACILITY_E2E_DOCKER=1 pnpm test:e2e-sandbox) passes against a facility-runner:dev image built from this branch.

Known limits, on purpose:

• /events does not replay an ambiguous loss, because appending is unguarded and the run's receipt counts event rows and lists check events before it is sealed with a chained digest, so a duplicate is a wrong receipt rather than a cosmetic artifact. A control plane that dies mid-POST therefore still fails the run, and behind a proxy a restart surfaces as 502 or 503 rather than a refused connection, so that path behaves as it does on main today. A client batch key with server-side dedupe would let it opt in. I am happy to file or take that follow-up.

• /push-token is conservative for the same reason: every call mints a fresh contents:write installation token with no idempotency guard, so a lost response now fails the delivery rather than leaving a second live token that nothing revokes.

• /hello is a one-shot credential claim, so a restart landing between its commit and its response still fails the run at bootstrap. Making /hello replay-safe is a server-side follow-up.

• An outage longer than the result budget still loses the run's outcome, breadcrumbed to the container log as coordinates only.

• Control-message delivery is at-least-once across a lost response: a steer can be applied twice. Within a live runner the in-memory cursor keeps it exactly once.

• The grace window delays surfacing a dead sandbox by up to one extra tick, which also means its keys stay live that much longer. That is the cost of not failing runs from one observation.

• A failed session-state restore is still swallowed, so a resume degrades to a cold start rather than failing the run.

• pnpm verify passes locally

• Behaviour verified beyond the test suite (say how): revert-the-fix mutation checks, plus the Docker sandbox E2E on an image built from this branch. Both described above.

• Documentation updated, or no user-facing change: no user-facing surface changed; constants carry their rationale at their definitions.

🤖 Generated with Claude Code

[adrian-lorenzo]

I'm requesting changes for the stale reconcile decision and the replay-safety gaps. Details inline.

[adrian-lorenzo]

I think this can still fail a recovered sandbox. This decision uses the lossObservedAt value from the earlier query, but failRun() only checks that the run is non-terminal. Another reconcile job can see the sandbox running and clear the stamp while this job is waiting on driver.status(), then this job still fails the run and revokes its keys. Can we make the failure conditional on the stored stamp still matching the value read here? The clear path below needs the same check.

[JuanMarchetto]

Fixed. Every write in that path now compare-and-sets against the lossObservedAt
value the tick read, the failure included. failRun takes an optional guard that
is ANDed into its existing atomic claim and returns whether it claimed the row,
so a concurrent tick that saw the sandbox alive and cleared the stamp wins and
the live run survives. The clear path carries the same predicate, so a stale
snapshot can no longer delete a stamp written after it was read, and the stamp
write reuses it rather than duplicating the SQL.

One related change worth declaring rather than leaving to be found:
updateGithubRunProgress(..., "failed", ...) is now gated on the claim landing.
It ran unconditionally before, so a lost claim would rewrite a live run's
progress comment to Failed.

The new coverage uses the driver seam to make this deterministic instead of
timing-dependent: the injected status() mutates the row mid-probe, so the test
reproduces the exact interleaving. A run whose stamp is cleared while its probe
is in flight must stay running with its keys intact, and a stamp written after
the tick read the run must survive the stale clear.

[adrian-lorenzo]

fetchJson is also used by /events, /hello, /push-token, and the upload endpoints. Retrying all of them after a lost response isn't safe. In particular, duplicate check events change the signed receipt, and replaying /push-token can mint another contents-write token. I think retries need to be enabled per call, after each endpoint has replay-safe semantics.

[JuanMarchetto]

Narrowed per call, and moved off the call sites entirely.

The transient classification is split in two. Codes that fire before the request
could reach a route handler (connection refused, name resolution loss, no route
to host, connect timeout) prove nothing was committed, so they stay retryable for
every caller: that is the control-plane restart this branch exists for. Codes
where the request was already on the wire (reset, broken pipe, header and body
timeouts) and transient 5xx do not prove that, so replaying them is at-least-once
and only an endpoint whose handler absorbs a duplicate opts in.

Since that is a property of the far end rather than of the caller, it is declared
once per endpoint in ENDPOINT_RETRY_POLICIES and api() looks it up from the
path. api() takes no policy argument, so a call site cannot invent one, and an
unclassified endpoint gets the conservative default rather than whatever its
caller felt like. hello, push-token and events do not replay; bundle,
steer, transcript, session-state and result do, each entry carrying the
handler behaviour that justifies it. The 429 path is unchanged and stays exempt
from the budget.

Two things surfaced while doing this. The session-state restore was calling
fetch directly and getting no outage tolerance at all; it now shares the same
transport through a byte-reading caller, so it is classified like everything
else. And the policy table is not merely snapshotted in a test: the suite derives
the endpoints the runner actually calls from the source and fails if one is
unclassified, or if a control-plane request bypasses the shared transport, so the
booleans cannot quietly drift.

One consequence I want to state rather than bury. With events conservative, a
control plane that dies mid-POST still fails the run, and behind a proxy a
restart surfaces as 502 or 503 rather than a refused connection, so that path
behaves as it does on main today. This branch improves the refused-connection
case and leaves that one where it was. Closing it needs /events to be
replay-safe, which is the client batch key with server-side dedupe I mentioned in
the description: an optional request header plus a dedupe read inside the
advisory-locked transaction appendRunEvents already holds, so no migration and
no breaking contract change. I am happy to add it here or as a separate PR.
Whichever you prefer.

[adrian-lorenzo]

There's still a lost-interrupt window here. /steer sets deliveredAt before returning the message. If that update commits and the response drops, the retry sees no undelivered messages and the runner never handles the interrupt. The new test starts after the message has reached handleControlMessage, so it doesn't cover this case. Can delivery remain pending until the runner acknowledges it?

[JuanMarchetto]

Fixed, and delivery does stay pending until the runner acknowledges. The
acknowledgment names the ids the runner has actually handled and rides on its
next poll, so no new endpoint or column was needed. GET /steer no longer writes
deliveredAt from the select, which is what burned the message.

Two things I would flag from doing it. Acknowledging by cursor range instead of
by explicit id is not safe here: ids are per-process uuidv7, so with more than one
API task the id order can disagree with commit order, and a range would retire a
message that was never served. And the ack has to be validated, not just parsed,
because an out-of-range value would otherwise mark every pending message for a
run delivered in one request. It now takes only ids this route could have issued,
bounded in count, scoped to the run and its org.

On the runner side the cursor advances only after the durable action landed, so a
message whose response died on the wire, or whose handling threw, is served
again. An interrupt is never retired: a steer that keeps failing is dropped after
three attempts with one diagnostic, because leaving it unacked makes it the
oldest pending row of every batch forever, but an operator's stop does not get
that treatment. Nothing holds the line in front of anything else. Because a
message can now stay pending indefinitely, the poll backs off when a served batch
produces no new acknowledgment, so an unappliable message cannot spin the loop
against the control plane.

One deployment detail: a run keeps the runner image its sandbox launched with, so
during the deploy that ships this route every run already in flight still speaks
the old protocol. A poll carrying only the old cursor therefore still gets the
previous mark-on-select behaviour; without that, such a runner would be handed
the same row on every iteration and re-apply one steer in a loop with no delay.
An acknowledgment always takes precedence, and the branch is marked deletable
once no sandbox launched before this change can still be polling.

Your point about the test was right: the old one started after the message
reached handleControlMessage. The new coverage drives a control plane that dies
mid-response on the same message and asserts the interrupt is applied exactly
once.

[adrian-lorenzo]

Please don't log the whole git object here. It can contain the generated PR title and body, and this write bypasses redactSecrets(). The attempted status, branch, and head SHA should be enough to diagnose the conflict.

[JuanMarchetto]

Fixed. The line is built from an allowlist of scalar coordinates (attempted
status, changed, branch, head sha, push error), each bounded, and the finished
string passes through redactSecrets(), so the generated title and body are
structurally excluded rather than filtered.

I kept push_error because it is already redacted at its assignment and, with
the run terminal, /events is refused too, so this line is the last place a push
failure can be recorded. It is agent-influenced text, which is why it is bounded
and inside the redaction pass rather than trusted. Say the word if you would
rather it were cut to the status, branch and sha you named. The stable
result_discarded_run_terminal prefix and attempted_status= field are
preserved so existing log greps still match.

[adrian-lorenzo]

Thanks for the thorough follow-up!

The earlier review points are addressed, but one restart window remains around /result. finishRun commits the terminal run status before completing required work such as security synchronization, result and audit events, PR update recording, and conversation finalization. If the API restarts in that window, the retry is rejected as run_terminal and the runner treats it as success, leaving that work silently incomplete.

Please make result finalization resumable and idempotent, or avoid replaying ambiguous result requests. Add an integration test that interrupts processing after the terminal claim, retries the result, and verifies finalization completes exactly once.

The branch also needs updating against main, preserving the Builder plan-gate and workspace provenance changes. Once those points are covered, this should be ready to approve.

[JuanMarchetto]

Addressed in 74f4904 (fix(api): resume an interrupted result finalization on the runner's replay), on top of a rebase onto current main.

Result finalization is now resumable and idempotent. finishRun's claim also records that finalization is pending (sandbox.finishedAt + a finalizingAt lease), and finalizedAt is written only once every step after the claim has landed. While it is pending, /result — and only that route — admits the runner's replay; finishRun resumes from the committed row (status, error, receipt, delivery and proposal rows are read back, the body only supplies what the claim never persisted). Each step is guarded by the durable trace its own effect leaves — destroyedAt, revokedAt, an event of its type, an audit row for its action, the conversation reply carrying the run's id — rather than a ledger written beside the effect, so a crash between effect and record cannot repeat it. The lease keeps two attempts from running the steps at once (a rolling restart can leave the first attempt running on the old process while the runner replays to the new one): a replay inside the lease gets 503 finalization_in_progress + Retry-After, which the runner already retries on this endpoint; once finalized, a replay is refused run_terminal as before.

Test: services/api/test/result-finalization.test.ts interrupts finishRun after the terminal claim, after key revocation, after the result event and after the audit row; replays the result through the route; and checks every effect exists exactly once (result event, run.finished audit, conversation reply, key revocation, finalizedAt) and that a further replay is 409 run_terminal with nothing changed. It also pins the lease takeover to a single winner under two concurrent replays, the admission to /result alone and to the run's own token, and the refusal of a result for a run that failRun/cancel made terminal.

Rebase: onto main at 5fb228d, keeping the Builder plan-gate and workspace provenance changes. The new /internal/runs/:runId/workspace endpoint is classified replaySafe in the runner's policy table (93e72e1) — its handler records the SHA once under an is-null guard and answers an exact replay with the recorded value — since the derivation test refuses an unclassified endpoint.

Verification: API suite 619 passed; the 3 watchtower timeouts fail identically on clean main in my environment and are unrelated. Runner suite 229 passed. Typecheck and biome clean.

[adrian-lorenzo]

Thanks for following up!

The fixed 60-second finalization lease can expire while the original attempt is still running. Finalization may synchronously process up to 20 security findings, each requiring several GitHub calls, and nothing renews finalizingAt. A replay can therefore take over while the first attempt still owns the work.

Several effects use check-then-write guards, so concurrent attempts can both observe the effect as missing and then create duplicate result events, audit rows, or tracked GitHub issues. Please keep ownership fenced or renewed for the whole finalization, and add an integration test where the first attempt remains active past the lease while a replay arrives.

Once that concurrency case is covered, this should be ready.

[JuanMarchetto]

Addressed in 2bfca61 (fix(api): keep the finalization lease renewed and fenced for the whole run).

Ownership is now renewed and fenced for the whole finalization. The terminal claim and the takeover each mint a finalizingToken beside finalizingAt, and every renewal is a compare-and-set on that token — at every step boundary, on a heartbeat timer (lease/3 = 20s) inside long steps, and before each finding syncSecurityFindings publishes. Two consequences:

• A live attempt never looks expired, however long the security sync takes: the runner's replay keeps getting the 503 + Retry-After it already honors, instead of taking the lease over and running the check-then-write guards beside the first attempt.
• An attempt that genuinely stalled past the lease (the only way finalizingAt can now go stale) finds the winner's token at its next renewal and aborts with that same 503 before its next effect — including per-finding inside the sync, so a stalled attempt waking mid-report cannot race the winner's ensureTrackedIssue into a duplicate issue. The mid-sync status downgrade and the finalizedAt marker carry the token fence too, so a superseded attempt can neither rewrite the winner's row nor report completion over work the winner is still doing; losing the lease inside the sync is rethrown past the security_issue_sync_failed downgrade, since it is not a sync failure.

Rows claimed by a pre-token process (a rolling restart's old task) renew against finalizingToken is null, so they keep the old behaviour until a takeover mints a token.

Tests: result-finalization.test.ts now holds a first attempt open past the lease both ways, on the real route. With its heartbeat running (finalizationLeaseRenewMs shrunk to 25ms), the test backdates the lease past expiry under the working attempt, waits until the heartbeat has visibly re-freshened it in the row, and pins the replay to 503 finalization_in_progress with nothing past the paused step executed — then releases the attempt and checks every effect exactly once and the further replay 409 run_terminal. With renewal stalled (cadence stretched past the test), the replay takes the expired lease over and completes; the woken attempt then fails the compare-and-set at its next boundary and rejects without repeating an effect, leaving state identical to the winner's. A unit test in tracked-issues.test.ts pins syncSecurityFindings to re-proving ownership before each finding: two qualifying findings, ownership lost after the first, exactly one issue created.

One residual window to be explicit about: a process that stalls longer than the lease and wakes inside a single external call (one ensureTrackedIssue, one progress-comment update) still completes that call before the next fence can stop it. That's inherent to non-transactional external effects; the fences bound the damage to at most the one call in flight, and the DB-side effects stay single-writer via the token.

Verification: finalization suite 12/12 and tracked-issues 4/4 pass in isolation; typecheck and biome clean. The full parallel suite shows environment flakes in my run (disjoint failure sets across runs), which I confirmed pre-existing by re-running it on clean 74f4904: 12 failures there, including this same finalization suite's 'audit' case, so it's the shared test database under parallel load rather than this change.

Rebase: the branch is rebased onto current main at 8889753 (Node 24 LTS toolchain, project-interface polish, the OAuth/MCP scope fixes, and the KB chain-change guard), conflict-free. On the new base: API typecheck and biome clean, finalization suite 12/12 and tracked-issues 4/4, runner suite 229/229 with typecheck clean.