fix(sandbox): rebuild a checkout the container threw away - #126
fix(sandbox): rebuild a checkout the container threw away#126nplusonedev wants to merge 1 commit into
Conversation
An offload-test run failed with the exec step retrying exactly as designed and
recovering nothing:
attempt 1 (2m54s) HTTP error! status: 500
attempt 2 (1.1s) HTTP error! status: 500
attempt 3 (4.7s) working directory '<dir>' was missing at exec time
attempt 4 (0.6s) working directory '<dir>' was missing at exec time
Attempts 3 and 4 reached a fresh container. That is the whole bug: the platform
replaced the container, the new one booted with an empty disk, and the step that
would have cloned into it had already completed, so Workflows never re-ran it.
Every later attempt was doomed before it started.
Cloudflare is explicit that this is normal: "All disk is ephemeral. When a
Container instance goes to sleep, the next time it is started, it will have a
fresh disk as defined by its container image." There is also no minimum runtime,
an out-of-memory instance is restarted, and a host restart can take one at any
time. We were treating the container filesystem as state that survives a
checkpoint, and it never was.
The fix mirrors what the substrate already does with `ensure()` before every
exec. `workspace()` now returns a `spec` — plain data, so it rides the
checkpoint — and `execInWorkspace` re-clones from it inside the step that finds
the tree gone, then runs the command once more. One rebuild, not a loop: a
second miss means the container is being replaced faster than it can be
populated, and the step's own retry budget already re-arms the recovery with
backoff between attempts.
The rebuild reaches the replacement, not a corpse. `workspaceMissing` can only
be raised after a shell RAN on the new container — `isWorkingDirFailure` needs a
non-zero exit with the shell's own directory-change error on stderr — so the
sandbox client in hand has just round-tripped to it, and the clone goes through
that same client.
`ExecFailed` gains `workspaceMissing` so the decision is a typed field rather
than a match against the message, and that classification now runs BEFORE the
timeout regex: the marked throw embeds `cwd`, so a repo path containing
"timeout" would otherwise be classified `ExecTimeout` and silently disable the
rebuild. `RETRY_ON` gains `CheckoutFailed`, because a clone failing mid-recovery
happens in exactly the weather that triggers recovery, and without it that
transient would be rethrown as non-retryable.
Docs corrected, because they said the opposite. ADR-0001 claimed the container
filesystem is "shared state across durable steps" kept alive by `sleepAfter`;
it now states the platform gives no such guarantee, and carries a new rule 3.
That rule is scoped deliberately: a re-clone restores the tree the SPEC
describes, which is right for a suite or a lint and WRONG for a step reading a
tree an earlier step mutated — re-cloning would hand self-heal-pr's verify a
clean checkout and pass on unmodified code. Those need captured bytes, which is
REWRITE.md's open FileRef chokepoint, and the rule says so rather than inviting
a wrong green. The dispatcher's `sleepAfter` comment no longer claims to buy
durability; runs/README.md and packages/core/README.md follow.
Only offload-test moves onto the primitive here. check, worker-deploy,
playwright-demo, pr-review and oxlint have the same exposure and are
mechanical follow-ups; self-heal-pr, refresh-fixtures and cdp-acceptance read
mutated or detached state and need the FileRef work instead.
![flaredispatch-fractalboxdev[bot]](https://avatars.githubusercontent.com/in/4273665?s=60&v=4){kind=small}
|
Ran SupersededThis branched from #125. Six commits landed on Why the conflict can't be resolvedFour conflicted files, and the conflicts are design-level rather than textual. Taking this PR's side would:
Neither side wrote What this PR got right, and what it's owedThe diagnosis was correct and it found three defects that are still live on
The Recommend closing this in favour of #136 rather than rebasing it. Happy to open that PR if you want it. |
|
Correction to my comment above, after both findings were verified against Finding 2 is unconditional — I hedged it wrongly. I wrote that it is "masked when the The asymmetry makes it sharper: the initial Finding 1, two details worth having. |
|
Superseded — closing in favour of #139. While this sat open, #127, #128, #130 and #134 landed a different recovery for the same failure: Merging this as-is would also regress: it sets Three things here were genuinely missing from
The Thanks — the diagnosis in this PR body is what made the defect findable. |
2 participants
A consumer's
offload-testruns have been red for days. The exec step retriedexactly as designed and recovered nothing:
Read from the Workflows instance record, not from logs.
Attempts 3 and 4 reached a fresh container. That is the bug. The platform
replaced the container, the new one booted with an empty disk, and the
checkoutstep had already completed — so Workflows never re-ran it. Every attempt after
the replacement was doomed before it started, and no retry budget could change
that.
This is documented platform behaviour, not a platform bug
From the Containers FAQ and
Lifecycle of a Container:
The same pages state there is no guaranteed minimum runtime, that an
out-of-memory instance is restarted, and that host restarts terminate instances
at unpredictable times.
We were treating the container filesystem as state that survives a checkpoint.
It never was.
The fix
workspace()now returns aspec— plain JSON, so it rides the checkpoint —and the new
execInWorkspaceprimitive re-clones from it inside the step thatfinds the tree gone, then runs the command once more.
This mirrors what the substrate already does:
ensure()before every exec,rebuilding from the recipe when the restore misses. The dispatcher path had only
the reclassification backstop and no recovery.
One rebuild, not a loop. A second miss means the container is being replaced
faster than it can be populated, and the step's own retry budget already re-arms
the recovery with exponential backoff between attempts.
The rebuild reaches the replacement, not a corpse
This was the load-bearing question, and it is settled by the mechanism rather
than by hope.
workspaceMissingcan only be raised after a shell ran on thenew container —
isWorkingDirFailurerequires a non-zero exit carrying theshell's own directory-change error on stderr. So at the moment the rebuild is
decided, the sandbox client in hand has just round-tripped to the replacement,
and the clone goes through that same client. The production trace confirms it:
attempts 3 and 4 got answers back from a fresh container through this handle.
Typed signal, classified first
ExecFailedgains an optionalworkspaceMissing, so the decision is a fieldrather than a match against the message the runtime just wrote.
That classification now runs before the timeout regex. The marked throw
embeds
cwd, so a repo path containing "timeout" (/workspace/request-timeout)would otherwise classify as
ExecTimeout, skip the rebuild, and — sinceExecTimeoutis not inRETRY_ON— kill the step non-retryably. Two tests pinthis; both fail if the ordering is reverted.
RETRY_ONalso gainsCheckoutFailed: a clone failing mid-recovery happens inexactly the weather that triggers recovery, and
rethrowForRetryPolicywouldotherwise rethrow that transient as
NonRetryableError.Docs, which said the opposite
specs/adr/0001-cloudflare-workflows-scope.mdclaimed the container filesystemis "shared state across durable steps" kept alive by
sleepAfter. It nowrecords the platform facts, cites this run as the evidence, and carries a new
rule 3.
Rule 3 is scoped deliberately. A re-clone restores the tree the spec
describes, which is correct for a suite, a lint or a build, and wrong for a step
that reads a tree an earlier step mutated. Applied blindly to
self-heal-pr'sverify step it would hand back a clean checkout and pass on unmodified code —
turning an infra failure into a wrong green, which is worse than the red it
replaces. Those steps need captured bytes restored, which is the
FileRefchokepoint already open in
REWRITE.md, and the rule says so.Also updated: the dispatcher's
sleepAftercomment no longer claims to buydurability (it narrows the idle window and does nothing when the container dies
mid-exec),
runs/README.md,packages/core/README.md, and theREWRITE.mdgaprow that already called this out as "the container FS is mistaken for durable
state" — now marked partly landed, since this re-clones rather than restoring
captured bytes.
Scope
Only
offload-testmoves onto the primitive: it is the failing path and the onewith 35-minute execs.
Same exposure, mechanical follow-ups:
check,worker-deploy,playwright-demo,pr-review,oxlint.Same exposure, but
execInWorkspaceis the wrong fix and would produce awrong green:
self-heal-pr,refresh-fixtures(mutated trees),cdp-acceptance(detached model). These need the
FileRefwork.Not exposed:
matrix-fanoutandplaywright-e2eclone and exec inside one step;release-notesbuilds its workspace outside a step, so replay re-runs it.Verification
pnpm typecheck0 errors,pnpm lintclean,pnpm test174 files / 2242passed, 1 skipped.
Mutation-checked rather than assumed: disabling the
workspaceMissingguardfails 3 of the 7 new primitive tests, and reverting the classification ordering
fails both new runtime tests. They pin behaviour, not the fake.
Honest boundary: this is proven against fakes plus the production trace, not
end to end against a real container replacement. Container boot is not
exercisable under Miniflare, so the live-path claim rests on the shared-client
mechanism described above and on the observed run.