fix(runs): a lost checkout under a path named "timeout" was never retried - #139
Open
nplusonedev wants to merge 7 commits into
Open
fix(runs): a lost checkout under a path named "timeout" was never retried#139nplusonedev wants to merge 7 commits into
nplusonedev wants to merge 7 commits into
Conversation
`ensureWorkspace` re-clones a checkout the container threw away, from inside the retryable step. Two things could stop it running, both left over from before that recovery existed. Classification order. The missing-working-directory throw embeds `cwd` and 200 chars of stderr in its message, and the `catch` below matched `/timed?\s*out|timeout/i` against that message first. So a checkout under a path like `/workspace/request-timeout` classified as `ExecTimeout` — which `RETRY_ON` excludes on purpose, since a command that outran its ceiling will outrun it again. The step then died unretried and the rebuild never ran. The throw now carries a symbol marker and the `catch` reads it ahead of the regex. Mutation-checked: drop the marker branch and the new test reports `expected 'ExecTimeout' to be 'ExecFailed'`. `CheckoutFailed` in `RETRY_ON`. The rebuild's own clone runs in exactly the weather that made the rebuild necessary. `sandbox.gitClone` fails `CheckoutFailed`, which was in none of the three lists, so a transient clone failure ended the step non-retryably one call short of the recovery it was invoked to perform. Added to `offload-test`, `check` and `oxlint`, alongside the `StepFailed` they already carry. `ContainerLaunchFailed` and `ContainerBusy` reach the same place from `ensureWorkspace`'s `acquire` and are deliberately left out: `acquire` already waits to the layer's ceiling before raising `ContainerBusy`, so retrying here stacks a second wait on top of admission control instead of recovering anything. That coupling wants its own change. ADR-0001 said the opposite of the platform. It claimed the container filesystem is "shared state across durable steps" kept alive by `sleepAfter`. It now records that all container disk is ephemeral, and carries a fifth rule with the qualifier that matters: a re-clone restores the tree the spec describes, so it is right for a suite or a build and wrong for a step reading a tree an earlier step mutated. Applied to `self-heal-pr`'s verify it would pass on unmodified code — an infra red turned into a wrong green. Those need `FileRef`. pnpm lint, typecheck clean; test 173 files / 2245 passed, 1 skipped.
Classification moved to the throw site. `isWorkingDirFailure` now raises `ExecFailed` directly instead of a symbol-marked `Error` the `catch` re-reads, so the `catch` needs only `if (cause instanceof ExecFailed) return cause`. That drops the hand-rolled discriminant, drops the duplicated `ExecFailed` construction, and stops the new declarations from sitting between the long `isWorkingDirFailure` doc block and the function it documents. Mutation still holds: delete the early return and the test reports `expected 'ExecTimeout' to be 'ExecFailed'`. Why retrying `CheckoutFailed` is safe, written down. It is a catch-all over the whole clone body and also wraps deterministic failures — a bad sha, a repo that is gone, the facade's pinned-recipe mismatch at `sandbox-facade.ts:277-287`. What keeps those off these steps is the sequence, not the tag: the only clone reachable from an exec step is `ensureWorkspace` re-cloning the same repo and sha the `checkout` step already cloned once, so a deterministic failure ends the run earlier. `check` and `oxlint` get the same note plus the `ContainerLaunchFailed` / `ContainerBusy` omission that only `offload-test` carried. Docs the change had left behind: the `RETRY_ON` header still said "both of which"; `runs/README.md` still described the retried set as `ExecFailed` alone and implied message-based classification; the ADR index row still summarised four rules; and rule 5 landed with no Consequences and no revisit trigger, so the probe cost, the widened retry set, the ten runs still on bare `workspace()`, and the `FileRef` dependency are all recorded now. lint, typecheck clean; test 173 files / 2245 passed, 1 skipped.
The sequence argument was wrong. It said the only clone reachable from an exec
step is a re-clone of a repo and sha `checkout` already cloned, so deterministic
clone failures could never reach the retry. That holds on the shared path and
not on the isolated one: `offload-test.ts:653` skips the `checkout` step
entirely when stages are isolated, and `:794-796` then performs the stage's
FIRST clone inside the retryable step. So a bad sha or a gone repo does get
retried there. The comment now says that plainly and owns the cost instead of
arguing it away.
"acquire already waits to the layer's ceiling" was wrong on both backends. That
came from the port's doc comment, not from either implementation. The container
backend's `acquire` is `Effect.succeed({ id: sandboxId })`
(`sandbox-cf.ts:464`) and cannot raise `ContainerBusy` at all; the facade's
refusal path is fail-fast and says so (`apps/substrate/src/facade.ts:163-165`).
The paragraph is deleted rather than repaired — the conclusion may stand, but
not for the reason given, and a wrong reason in a comment is worse than none.
Comment volume, per AGENTS.md. Dropped the test's four-line restatement of its
own name, the duplicated `CheckoutFailed` paragraphs in `check` and `oxlint`,
and six lines in `sandbox-cf.ts` that the one-line `catch` note and the named
test already carry. Also deleted the stale "The throw is classified by the
`catch` below", which the change had just made false.
`retryOn` had no behavioural test — the six assertions read a literal array out
of step metadata, and the inline fake never retries. Two cases now drive
`rethrowForRetryPolicy` itself: `CheckoutFailed` passes through under the runs'
policy, and comes back `NonRetryableError` under one that omits it.
ADR: added `vitest-shard` and `self-heal-pr` to the bare-`workspace()` list, and
scoped rule 5's empty-disk claim to the container backend, since the substrate
restores the tree inside the exec fence.
lint, typecheck clean; test 173 files / 2247 passed, 1 skipped.
Its opening line called every retried class the platform, twenty lines above an admission that a bad sha and a gone repo ride `CheckoutFailed` into the same list. A bad sha is neither. Dropped that half; "never a verdict" is the half that is true and the half that matters. The cost line said "bounded by the backoff", which understates it: the bound is four clone attempts plus backoff, multiplied by the stages running at once on the isolated path.
…g each other
A failed credential scrub now logs as well as appending to the error. Three
review lenses landed on the same gap independently: that notice reached the
operator only because `CheckoutFailed` ended the run, and this branch makes it
retryable, so an attempt that fails here followed by one that succeeds carries
the notice out of the final result and into per-attempt history alone. The
residue itself was never the issue — `rm -rf ${targetDir}` runs ahead of every
clone, so a later attempt clears it and a green run always ends scrubbed. The
disclosure path was the issue.
`apps/dispatcher/src/sandbox.ts` still said `sleepAfter` "buys durability across
normal inter-step gaps", which is the exact framing ADR-0001 rule 5 retires —
and the ADR cites that file, so following the citation landed a reader on the
retired claim.
`ensureWorkspace`'s own docblock said to call it inside the retryable step and
never mentioned that the step's `retryOn` has to list `CheckoutFailed`. The next
run to adopt the primitive reads the primitive, not the three run files that
already know, and would reproduce the bug this branch fixes.
Rule 5's substrate paragraph now names what that deploy gains and pays rather
than implying a no-op: one fenced exec per retryable step, a retry on pool
admission refusals, and a retried recipe-pin mismatch, since
`sandbox-facade.ts:274-287` folds both into `CheckoutFailed`. The `RETRY_ON`
note picks up the pin mismatch and the per-attempt token mint.
Also dropped two comments the policy forbids — one restating the line under it,
one narrating where the code used to live — and rewrapped a ragged paragraph.
lint, typecheck clean; test 173 files / 2247 passed, 1 skipped.
The sentence sat after one about the substrate backend, where `gitClone` sends no clone and no token is minted at all. Only the container path calls `resolveCloneToken` per attempt.
![flaredispatch-fractalboxdev[bot]](https://avatars.githubusercontent.com/in/4273665?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI code review — 💬 Comment
Risk tier: full · 0 critical · 1 warnings · 1 suggestions
Reviewers: security
1. 💡 Suggestion — Avoid direct console.warn in runtime code
📍 packages/runtime-cf/src/sandbox-cf.ts:568-570
The credential-scrub failure path writes directly to the global console, unlike the surrounding structured error handling. This can produce noisy, uncorrelated production logs and is difficult for callers/tests to control. Route the warning through the runtime's existing logger or injectable diagnostics mechanism, preserving the redaction guarantees.
2. ⚠️ Warning — Clarify the effect of sleepAfter
📍 apps/dispatcher/src/sandbox.ts:57-62
The comment says 'sleepAfter = "10m"' “narrows the idle window,” but 'sleepAfter' is the idle grace period before the container sleeps; a 10-minute value defines or extends that period rather than narrowing it. Rephrase this as reducing the maximum time a container remains active, without implying that the setting makes reclamation less likely or the disk durable.
The rewritten comment said a LONGER idle window "narrows the idle window", which contradicts itself inside one sentence. The PR-review bot read it right: sleepAfter is the idle grace period before the container sleeps, and 10m extends it. The sentence now says what the setting does — extends how long an idle container stays awake — and keeps the claim that matters: it guarantees nothing, because container disk is ephemeral on every path.
Contributor
Author
|
Validated both findings; one fixed, one refuted with evidence. Finding 2 (sleepAfter wording) — correct, fixed in Finding 1 ( |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three things PR #126 had that
maindoes not. #126 itself is superseded — see below.Why this exists
#126 proposed an
execInWorkspaceprimitive to rebuild a checkout a replaced container threw away. While it sat open, #127, #128, #130 and #134 landed a different recovery:ensureWorkspace, atest -d <dir>/.gitprobe that re-clones on a miss, called inside the retryable step byoffload-test(both paths),checkandoxlint.That recovery is wider than #126's — #126 only ever touched
offload-test— and the primitive is redundant against it. But three things in #126 are still missing frommain, and one of them is a live defect.1. A lost checkout under a path containing "timeout" was never retried
isWorkingDirFailureraised a bareErrorwhose message embeds the missingcwd. The enclosingcatchthen tested/timed?\s*out|timeout/iagainst that same message to spot an SDK timeout. So a checkout at, say,/workspace/request-timeoutclassified asExecTimeout— a classRETRY_ONexcludes deliberately, because a command that outran its ceiling will outrun it again. The step died unretried andensureWorkspacenever got its second chance.The failure is now built at the throw site as
ExecFailed, and thecatchopens withif (cause instanceof ExecFailed) return cause;.Mutation-checked rather than asserted: remove that early return and the new test reports
expected 'ExecTimeout' to be 'ExecFailed'. The test drivesmakeSandboxCloudflareLive, not a fake.2.
CheckoutFailedjoinsRETRY_ONensureWorkspacere-clones inside the retryable step, andsandbox.gitClonefailsCheckoutFailed, which was in none of the three lists. A clone that flaked mid-recovery ended the step one call short of the repair it was invoked to perform.The cost, stated plainly.
CheckoutFailedis a catch-all over the whole clone body, so deterministic failures ride it too — a bad sha, a repo that is gone, and on the substrate backend the recipe-pin mismatch atsandbox-facade.ts:274-287. On the isolated-stages path there is no earliercheckoutstep (offload-test.ts:653), so the stage's clone is the run's first and those deterministic failures do reach the retry.For the failures named above that is four fast attempts plus 5s/10s/20s of backoff — under a minute — times the stages running at once, each minting its own installation token on the container backend. A clone that hangs rather than fails is bounded by
CLONE_TIMEOUT_SEC = 600per attempt, so roughly 40 minutes worst case. That last figure is read from the code and assumes Cloudflare applies the per-step timeout per attempt; it is not a measured bound.ContainerLaunchFailedandContainerBusystay out.ExecTimeoutstays out, unchanged.3. ADR-0001 rule 5
The ADR claimed the container filesystem is "shared state across durable steps" kept alive by
sleepAfter. It is not, andapps/dispatcher/src/sandbox.tssaid the same thing in the file the ADR cites — so following the citation landed a reader on the retired claim. Both are corrected, with Cloudflare's own wording on ephemeral disk.Rule 5 carries the caveat that matters: a re-clone restores the tree the spec describes, which is right for a suite, a lint or a build and wrong for a step reading a tree an earlier step mutated. Applied to
self-heal-pr's verify it would hand back a clean checkout and pass on unmodified code — an infra red turned into a wrong green. Those needFileRef.Deployment matrix
SUBSTRATE_BACKEND=off, deployed)=on)isWorkingDirFailurefold and never forwardscwdtest -dper retryable stepexecUnderGrantrebuilds inside the fence anywaywrangler.jsonc:90is"off", so no deployed configuration exercises the substrate column today.Also in here
A failed credential scrub now logs as well as appending to the error. Because
CheckoutFailedbecame retryable, an attempt that fails there followed by one that succeeds would have carried that notice out of the run's final result into per-attempt history alone. The residue itself was never the risk —rm -rf ${targetDir}runs ahead of every clone, so a green run always ends scrubbed. The disclosure path was.ensureWorkspace's docblock now states that the step'sretryOnmust listCheckoutFailed. The next run to adopt the primitive reads the primitive, not the three run files that already know.Known follow-ups, deliberately not in this PR
git.cloneis the real fix.+120swhileensureWorkspacecan run a clone plus install inside the same step. Pre-existing, predates this branch; two independent lenses flagged it.RETRY_ONand its rationale are duplicated across three runs, edited in lockstep twice now.workspace()list true; a test asserting theworkspace()vsensureWorkspace()split would.proposedwhile rule 5 governs merged behaviour. A governance call, not one to make inside this PR.Verification
pnpm lint0,pnpm typecheck0,pnpm test173 files / 2247 passed, 1 skipped. Test count is +2 overmain: the six existingretryOnassertions only ever read a literal array out of step metadata, and the inline fake never retries, so two cases now driverethrowForRetryPolicyitself —CheckoutFailedpasses through under the runs' policy and returnsNonRetryableErrorunder one that omits it.Closes #126.