fix(platform): keep in-flight runs alive across control-plane restarts - #162
fix(platform): keep in-flight runs alive across control-plane restarts#162JuanMarchetto wants to merge 3 commits into
Conversation
A control-plane restart (dev watch, deploy) failed any in-flight run: the runner retried only HTTP 429, so the first connection error or 5xx from a restarting API killed the process, the container exited, and reconcile recorded sandbox_lost. fetchJson now retries network-level failures — connection refused/reset, DNS deregistration of a containerized API's name, stalled-proxy timeouts — and 500/502/503/504, with jittered exponential backoff capped to a bounded outage budget that is spent in full, plus one guaranteed retry so a single stalled attempt cannot consume the budget and turn a slow failure into a fatal one. Retry-After is honored on 5xx but floored at the backoff so a recovering proxy answering Retry-After: 0 cannot make every runner stampede it. Body reads live inside the same classification, so a process killed between headers and body is the same outage as a refused connection. The existing rate-limit semantics are untouched and exempt from the budget. Around it: event batches keep buffering through an outage via the existing single-flight backpressure; a drain that fails for good resumes its source stream so a child blocked on a full pipe can still exit (and the armed command timeout is now cleared on that path); control messages act before they ack — the server marks them delivered on fetch, so an interrupt must land even when the ack transport is down; the steering poll survives failed iterations, reports the degraded window once the transport returns, and only ends on a terminal run; and a replayed terminal result answered with 409 run_terminal is treated as already recorded, leaving a container- log breadcrumb when the recorded verdict diverges.
…ng runs Reconcile declared sandbox_lost from a single driver.status() probe. The verdict is irreversible — it revokes the run's keys and 409s every later runner call — so one probe racing an API restart, an in-flight terminal result, or a provider misreport permanently killed a recoverable run. The first exited/lost observation now only stamps lossObservedAt in the sandbox state: an atomic jsonb_set guarded to live statuses and compare- and-set against the value the tick read, so an overlapping tick holding a stale snapshot cannot move the window later while a corrupt stamp — which could never confirm — can still be replaced. The run is failed only when the loss persists past SANDBOX_LOSS_GRACE_MS, and the stamp is cleared when the sandbox is seen alive again. A returning worker cannot fail a run it never observed lost. reconcileSandboxes accepts an injectable driver resolver for tests, mirroring DispatchRunDeps.
adrian-lorenzo
left a comment
adrian-lorenzo
left a comment
There was a problem hiding this comment.
I'm requesting changes for the stale reconcile decision and the replay-safety gaps. Details inline.
| if (status === "exited" || status === "lost") { | ||
| await failRun(db, run.orgId, run.id, "sandbox_lost", "sandbox_lost"); | ||
| await updateGithubRunProgress(db, run.id, "failed", { config }).catch(() => undefined); | ||
| if (sandboxLossConfirmed(sandbox, new Date())) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| const sleep = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)); | ||
| let transientAttempt = 0; | ||
| let rateLimitAttempt = 0; | ||
| // Retrying makes these requests at-least-once: a request whose response was |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| }); | ||
| try { | ||
| const query = afterId ? `?afterId=${encodeURIComponent(afterId)}` : ""; | ||
| const messages = await api<Array<{ id: string; body: string; kind?: string }>>( |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| // The control plane already holds a different terminal verdict and now | ||
| // rejects both /result and /events for this run, so the container log is | ||
| // the only place left to record what this attempt would have reported. | ||
| process.stderr.write( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Review follow-up on four counts, each of which could turn a recovered state into a destroyed one. fetchJson retried every endpoint alike. Codes that fire before the request could reach a handler prove nothing was committed, so they stay retryable for everyone: that is the control-plane restart this branch exists for. Codes where the request was already on the wire, and transient 5xx, do not prove that, so replaying them is at-least-once and now only an endpoint whose handler absorbs a duplicate opts in. That is a property of the far end rather than of the caller, so it is declared once per endpoint and api() looks it up from the path; api() takes no policy argument, so a call site cannot invent one, and an unclassified endpoint fails rather than duplicates. push-token stops replaying: every call mints a contents:write installation token with no idempotency guard, so a lost response left a second live token that nothing revokes. events stops replaying: appending is unguarded and the receipt counts event rows before it is sealed with a chained digest, so a duplicate is a wrong receipt. The session-state restore joins the same transport instead of calling fetch directly with no tolerance at all. The steer route marked a message delivered because it was fetched, so a response lost on the wire burned it and an operator's stop vanished. Delivery is now marked from the ids the next poll acknowledges, scoped to the run and its org and refused whole unless every id is one this route could have issued. Acknowledging by cursor range would not do: 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. The runner acknowledges only after the durable action landed, never retires an interrupt, and backs off when a served batch produces no acknowledgment so an unappliable message cannot spin the poll. A runner launched before this change keeps its image, so a poll carrying only the old cursor still gets the previous mark-on-select behaviour; without it such a runner would re-apply one steer in a loop. The loss verdict read its stamp before a network probe per live run and then failed the run on that snapshot, while failRun only checked that the run was non-terminal. A concurrent tick that saw the sandbox alive and cleared the stamp therefore lost. Every write in that path now compare-and-sets against the stamp the tick read, the failure included, and failRun reports whether it claimed the row so a lost claim can no longer rewrite a live run's GitHub progress comment to failed. The discarded-result diagnostic serialized the whole delivery object into container stderr, which sits outside the run-event redaction boundary. It now prints an allowlist of scalar coordinates, each bounded, with the finished line passed through redactSecrets, so the generated pull request title and body cannot reach an operator log store.
2 participants
What changes
An API or worker restart no longer kills in-flight runs (#35).
Runner.
fetchJsonused 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 honorsRetry-Afterbut keeps the backoff as a floor, so a proxy answeringRetry-After: 0cannot 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_POLICIESandapi()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 throughapi()read their entry from the same table by name.bundle,steer,transcript,session-stateandresultreplay;hello,push-tokenandeventsdo not. Each entry carries the handler behaviour that justifies it.The session-state restore used to call
fetchdirectly 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:
CONTROL_MESSAGE_MAX_ATTEMPTS(3) with onesteer_undeliverableevent, 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.steer_poll_degradedevent 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.409 run_terminalcounts 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 throughredactSecrets(), 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_lostfrom a singledriver.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 stampslossObservedAtin the sandbox state, and the run fails only when the loss persists pastSANDBOX_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.
failRuntakes an optional guard folded into its existing atomic claim and reports whether it claimed the row;updateGithubRunProgressis gated on that, which also stops a lost claim from rewriting a live run's progress comment to failed.reconcileSandboxestakes an injectable driver resolver for tests, mirroringDispatchRunDeps.GET /internal/runs/:runId/steerno 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 whatnewId("evt")produces, up toSTEER_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 oldafterIdcursor 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_lostin one day of dogfooding (#35). Atsx watchrestart 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 verifypasses end to end: lint, typecheck, clean build, DB-backed suites with skips forbidden, guards, audit.node:httpservers, 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-Afterhonored 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.orchestrator-checks.test.ts, andsandbox.test.tsagainst 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.FACILITY_E2E_DOCKER=1 pnpm test:e2e-sandbox) passes against afacility-runner:devimage built from this branch.Known limits, on purpose:
/eventsdoes 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 onmaintoday. A client batch key with server-side dedupe would let it opt in. I am happy to file or take that follow-up./push-tokenis 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./hellois a one-shot credential claim, so a restart landing between its commit and its response still fails the run at bootstrap. Making/helloreplay-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 verifypasses locallyBehaviour 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