From e07585d203faeebb57a523526e0664913e90d1c5 Mon Sep 17 00:00:00 2001 From: nplusonedev <313439419+nplusonedev@users.noreply.github.com> Date: Sat, 15 Aug 2026 19:48:21 +0000 Subject: [PATCH 1/7] fix(runs): let the workspace rebuild survive its own two failure modes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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. --- packages/runtime-cf/src/sandbox-cf.test.ts | 22 +++++++++++++++ packages/runtime-cf/src/sandbox-cf.ts | 28 ++++++++++++++++++-- runs/check.test.ts | 6 ++++- runs/check.ts | 11 +++++--- runs/offload-test.test.ts | 24 ++++++++++++++--- runs/offload-test.ts | 20 +++++++++++--- runs/oxlint.test.ts | 6 ++++- runs/oxlint.ts | 11 +++++--- specs/adr/0001-cloudflare-workflows-scope.md | 25 ++++++++++++++--- 9 files changed, 132 insertions(+), 21 deletions(-) diff --git a/packages/runtime-cf/src/sandbox-cf.test.ts b/packages/runtime-cf/src/sandbox-cf.test.ts index b60165b..1fd2c1b 100644 --- a/packages/runtime-cf/src/sandbox-cf.test.ts +++ b/packages/runtime-cf/src/sandbox-cf.test.ts @@ -429,6 +429,28 @@ describe("makeSandboxCloudflareLive — exec result folding (D)", () => { }), ); + it.effect("a checkout path containing 'timeout' still fails as ExecFailed, not ExecTimeout", () => + Effect.gen(function* () { + // The throw embeds `cwd`, and the `catch` also matches /timeout/i to spot + // an SDK timeout. Classify by message first and this lands as ExecTimeout + // — which RETRY_ON excludes on purpose, so the step dies unretried and the + // rebuild never runs. + currentBox = makeFakeBox({ proc: null }); + currentBox.exec = vi.fn(async () => ({ + exitCode: 1, + duration: 0, + stdout: "", + stderr: "Failed to change directory to '/workspace/request-timeout'", + })); + const exit = yield* Effect.flatMap(SandboxTag, (s) => + s.exec({ command: "cargo test", cwd: "/workspace/request-timeout", env: {} }), + ).pipe(Effect.provide(execLayer()), Effect.exit); + const err = failureOf<{ _tag: string; stderrTail?: string }>(exit); + expect(err?._tag).toBe("ExecFailed"); + expect(err?.stderrTail).toContain("was missing at exec time"); + }), + ); + it("isWorkingDirFailure — fires only on a cwd-set, non-zero, no-stdout, cd-error result", () => { const cd = (over: Record = {}) => ({ exitCode: 1, diff --git a/packages/runtime-cf/src/sandbox-cf.ts b/packages/runtime-cf/src/sandbox-cf.ts index 2666d89..bc67df4 100644 --- a/packages/runtime-cf/src/sandbox-cf.ts +++ b/packages/runtime-cf/src/sandbox-cf.ts @@ -185,6 +185,16 @@ interface RawExecResult { * keeps the container warm across the inter-step gap so this rarely fires; this * is the honesty backstop for the residual eviction/replay cases.) */ +/** + * Marks the throw raised for a vanished working directory, so the `catch` below + * classifies it without re-reading the message it just wrote. A symbol, so it + * cannot collide with a property an SDK error carries. + */ +const WORKSPACE_MISSING: unique symbol = Symbol("workspaceMissing"); + +const isWorkspaceMissingThrow = (cause: unknown): boolean => + typeof cause === "object" && cause !== null && WORKSPACE_MISSING in cause; + export const isWorkingDirFailure = ( r: { readonly exitCode: number; readonly stdout: string; readonly stderr: string }, cwd: string | undefined, @@ -618,8 +628,11 @@ export const makeSandboxCloudflareLive = ( // as a lint/test verdict (see `isWorkingDirFailure`). The throw is // classified by the `catch` below. if (isWorkingDirFailure(result, cwd)) { - throw new Error( - `working directory '${cwd}' was missing at exec time — the checkout did not survive to this step (container recycled). stderr: ${stderr.slice(0, 200)}`, + throw Object.assign( + new Error( + `working directory '${cwd}' was missing at exec time — the checkout did not survive to this step (container recycled). stderr: ${stderr.slice(0, 200)}`, + ), + { [WORKSPACE_MISSING]: true }, ); } // Only a bounded TAIL is inlined in the step's return value, so the @@ -641,6 +654,17 @@ export const makeSandboxCloudflareLive = ( // generic launch failure. Prefer any stdout/stderr the throw carried // (some SDK errors attach them); else the Error message — this is // what Workflows persists via ExecFailed.message (#88). + // Ahead of the message regex. The marked throw embeds `cwd` and 200 + // chars of stderr, so a checkout under a path like + // `/workspace/request-timeout` matches that regex and lands as + // `ExecTimeout` — which `RETRY_ON` deliberately excludes, so the step + // dies unretried and `ensureWorkspace` never gets its second chance. + if (isWorkspaceMissingThrow(cause)) { + return new ExecFailed({ + exitCode: -1, + stderrTail: diagnosticTail(cause, redactValues), + }); + } const message = cause instanceof Error ? cause.message : String(cause); if (/timed?\s*out|timeout/i.test(message)) { return new ExecTimeout({ diff --git a/runs/check.test.ts b/runs/check.test.ts index feb65cb..8b0ef1a 100644 --- a/runs/check.test.ts +++ b/runs/check.test.ts @@ -121,7 +121,11 @@ describe("check", () => { const execStep = handles.executions.steps.find((s) => s.name === "exec"); expect(execStep?.metadata?.["stepOpts.timeoutSec"]).toBe(1800 + 120); expect(execStep?.metadata?.["stepOpts.retries"]).toBe(3); - expect(execStep?.metadata?.["stepOpts.retryOn"]).toEqual(["ExecFailed", "StepFailed"]); + expect(execStep?.metadata?.["stepOpts.retryOn"]).toEqual([ + "ExecFailed", + "StepFailed", + "CheckoutFailed", + ]); }).pipe(Effect.provide(layer)); }, ); diff --git a/runs/check.ts b/runs/check.ts index fd5fa29..484c0cf 100644 --- a/runs/check.ts +++ b/runs/check.ts @@ -164,15 +164,20 @@ const STEP_TIMEOUT_HEADROOM_SEC = 120; /** * Platform-failure retries — see the `exec` step. A verdict is never retried: - * a command that runs and exits non-zero is a normal `ExecResult`, so neither - * class here can reach one. + * a command that runs and exits non-zero is a normal `ExecResult`, so no class + * here can reach one. * * `StepFailed` is included because a platform kill leaves no Effect `Cause` to * read a tag from, and the runner falls back to that name — so listing only * `ExecFailed` left the purely-platform failure as the one thing not retried. + * + * `CheckoutFailed` is included because `ensureWorkspace` re-clones inside this + * step, in the same weather that destroyed the checkout. A transient clone + * failure there would end the step non-retryably, one call short of the + * recovery it was invoked to perform. */ const PLATFORM_RETRIES = 3; -const RETRY_ON = ["ExecFailed", "StepFailed"] as const; +const RETRY_ON = ["ExecFailed", "StepFailed", "CheckoutFailed"] as const; /** CONFIG_KV key — strictly per-repo (see header: no global fallback). */ const commandKey = (repo: string): string => `check.command:${repo}`; diff --git a/runs/offload-test.test.ts b/runs/offload-test.test.ts index 6101e57..ca003dc 100644 --- a/runs/offload-test.test.ts +++ b/runs/offload-test.test.ts @@ -126,7 +126,11 @@ describe("offload-test", () => { yield* offloadTest.run(baseInput); const execStep = handles.executions.steps.find((st) => st.name === "exec"); expect(execStep?.metadata?.["stepOpts.retries"]).toBe(3); - expect(execStep?.metadata?.["stepOpts.retryOn"]).toEqual(["ExecFailed", "StepFailed"]); + expect(execStep?.metadata?.["stepOpts.retryOn"]).toEqual([ + "ExecFailed", + "StepFailed", + "CheckoutFailed", + ]); }).pipe(Effect.provide(layer)); }); @@ -232,7 +236,11 @@ describe("offload-test", () => { // raised by the engine, so `retryOn` cannot gate it: a wedged exec is // replayed for the whole budget. expect(execStep?.metadata?.["stepOpts.retries"]).toBe(3); - expect(execStep?.metadata?.["stepOpts.retryOn"]).toEqual(["ExecFailed", "StepFailed"]); + expect(execStep?.metadata?.["stepOpts.retryOn"]).toEqual([ + "ExecFailed", + "StepFailed", + "CheckoutFailed", + ]); }).pipe(Effect.provide(layer)); }, ); @@ -751,7 +759,11 @@ describe("offload-test staged mode", () => { const execWorkspace = handles.executions.steps.find((s) => s.name === "exec-workspace"); expect(execWorkspace?.metadata?.["stepOpts.timeoutSec"]).toBe(900 + 120); expect(execWorkspace?.metadata?.["stepOpts.retries"]).toBe(3); - expect(execWorkspace?.metadata?.["stepOpts.retryOn"]).toEqual(["ExecFailed", "StepFailed"]); + expect(execWorkspace?.metadata?.["stepOpts.retryOn"]).toEqual([ + "ExecFailed", + "StepFailed", + "CheckoutFailed", + ]); // The suspicious labelled-key-missing fallback is recorded on the // stage's step metadata — `workspace` resolved its own key, so only // `features` is flagged. @@ -1155,7 +1167,11 @@ describe("offload-test isolated stages", () => { // The retry contract is unchanged — it is the UNIT that changed. const execA = handles.executions.steps.find((s) => s.name === "exec-a"); expect(execA?.metadata?.["stepOpts.retries"]).toBe(3); - expect(execA?.metadata?.["stepOpts.retryOn"]).toEqual(["ExecFailed", "StepFailed"]); + expect(execA?.metadata?.["stepOpts.retryOn"]).toEqual([ + "ExecFailed", + "StepFailed", + "CheckoutFailed", + ]); expect(result.exitCode).toBe(0); expect(result.durationMs).toBe(300); diff --git a/runs/offload-test.ts b/runs/offload-test.ts index afdb1ae..b303fbe 100644 --- a/runs/offload-test.ts +++ b/runs/offload-test.ts @@ -258,9 +258,9 @@ const PLATFORM_RETRIES = 3; * the one that was missing, and its absence made the whole retry policy inert * on the failure it was written for. * - * A step body here can fail in exactly two typed ways — `ExecFailed` and - * `ExecTimeout` — because a command that RUNS and exits non-zero comes back as a - * normal `ExecResult`. When the platform kills the step outright, no Effect + * A step body here fails in three typed ways — `ExecFailed`, `ExecTimeout` and + * `CheckoutFailed` — because a command that RUNS and exits non-zero comes back as + * a normal `ExecResult`. When the platform kills the step outright, no Effect * `Cause` survives the Workflow boundary, `errorTagOf` falls back to * `"StepFailed"`, and a `retryOn` listing only `ExecFailed` classified that as * non-retryable — so the one failure mode that is purely the platform's was the @@ -271,11 +271,23 @@ const PLATFORM_RETRIES = 3; * consumer's heaviest stage peaks at 2.2 GiB of 11.9 GiB with 8.4 GB of disk * free, so this is not resource pressure being papered over. * + * `CheckoutFailed` is the third because `ensureWorkspace` now re-clones INSIDE + * this step. The clone runs in exactly the weather that made the rebuild + * necessary — a container the platform just replaced — so its own transient + * failure would otherwise end the step non-retryably, one call short of the + * recovery it was invoked to perform. + * * `ExecTimeout` stays OUT, deliberately. Its tag survives the boundary intact * whenever there is a Cause to read, so it lands here as itself rather than as * `StepFailed` — and a command that outran its ceiling will outrun it again. + * + * `ContainerLaunchFailed` and `ContainerBusy` also reach here from + * `ensureWorkspace`'s `sandbox.acquire`, and stay OUT for now: `acquire` already + * waits to the layer's ceiling before raising `ContainerBusy`, so retrying it + * here stacks a second wait on top of admission control rather than recovering + * anything. That coupling deserves its own change, not a line in this one. */ -const RETRY_ON = ["ExecFailed", "StepFailed"] as const; +const RETRY_ON = ["ExecFailed", "StepFailed", "CheckoutFailed"] as const; const stepTimeoutFor = (execTimeoutSec: number): number => execTimeoutSec + STEP_TIMEOUT_HEADROOM_SEC; diff --git a/runs/oxlint.test.ts b/runs/oxlint.test.ts index 56750ea..166aee7 100644 --- a/runs/oxlint.test.ts +++ b/runs/oxlint.test.ts @@ -289,7 +289,11 @@ describe("oxlint source determinism", () => { // oxlint exiting non-zero is a normal ExecResult decided by the run body, // so `retryOn: ExecFailed` can only ever cover the container. expect(execStep?.metadata?.["stepOpts.retries"]).toBe(3); - expect(execStep?.metadata?.["stepOpts.retryOn"]).toEqual(["ExecFailed", "StepFailed"]); + expect(execStep?.metadata?.["stepOpts.retryOn"]).toEqual([ + "ExecFailed", + "StepFailed", + "CheckoutFailed", + ]); }).pipe(Effect.provide(layer)); }); }); diff --git a/runs/oxlint.ts b/runs/oxlint.ts index aa95c86..9524144 100644 --- a/runs/oxlint.ts +++ b/runs/oxlint.ts @@ -97,15 +97,20 @@ const TIMEOUT_SEC_DEFAULT = 300; /** * Platform-failure retries — see the `exec` step. A verdict is never retried: - * a command that runs and exits non-zero is a normal `ExecResult`, so neither - * class here can reach one. + * a command that runs and exits non-zero is a normal `ExecResult`, so no class + * here can reach one. * * `StepFailed` is included because a platform kill leaves no Effect `Cause` to * read a tag from, and the runner falls back to that name — so listing only * `ExecFailed` left the purely-platform failure as the one thing not retried. + * + * `CheckoutFailed` is included because `ensureWorkspace` re-clones inside this + * step, in the same weather that destroyed the checkout. A transient clone + * failure there would end the step non-retryably, one call short of the + * recovery it was invoked to perform. */ const PLATFORM_RETRIES = 3; -const RETRY_ON = ["ExecFailed", "StepFailed"] as const; +const RETRY_ON = ["ExecFailed", "StepFailed", "CheckoutFailed"] as const; export const oxlint = defineRun({ name: "oxlint", diff --git a/specs/adr/0001-cloudflare-workflows-scope.md b/specs/adr/0001-cloudflare-workflows-scope.md index f29125b..7946121 100644 --- a/specs/adr/0001-cloudflare-workflows-scope.md +++ b/specs/adr/0001-cloudflare-workflows-scope.md @@ -60,12 +60,13 @@ Three repo facts the decision has to account for: - **Replay re-executes anything not inside a completed step.** `self-heal-pr.ts:186-202` uses `runDetached` + `waitForExit` rather than one long `exec` precisely so a Worker eviction mid-agent does not re-spawn the agent and double-spend the model budget. The - container's `sleepAfter = "10m"` (`apps/dispatcher/src/sandbox.ts:60`) exists for the - same reason: the container filesystem is shared state *across* durable steps. + container's `sleepAfter = "10m"` (`apps/dispatcher/src/sandbox.ts:60`) narrows the idle + window in which a container is reclaimed between steps. It does not make the container + filesystem durable, and nothing does — see rule 5. ## Decision -Four rules govern how runs use Workflows. +Five rules govern how runs use Workflows. **1. One dispatch, one instance, and the instance id is the semantic idempotency key.** Every entry point (webhook, Action, schedule, child spawn) names its instance from the @@ -90,6 +91,24 @@ humans also act on — a GitHub issue, a PR, a deployment — the authoritative that entity's own status field (labels, PR state, deployment status), and each webhook event starts a short instance that reads state, acts, and writes state back. +**5. Container disk is not durable state, so a step re-establishes what it needs.** A +checkout does not survive between durable steps by right. Cloudflare states the +mechanism plainly: "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" +([Containers FAQ](https://developers.cloudflare.com/containers/faq/)). The same pages +give no guaranteed minimum runtime, restart an out-of-memory instance, and terminate +instances on host restarts. A `checkout` step that already completed is never re-run by +replay, so every attempt after a container swap hits an empty disk. `ensureWorkspace` +(`packages/core/src/primitives/workspace.ts`) is the repair, and it belongs INSIDE the +retryable step — a rebuild the retry does not re-run is not a rebuild. + +Scoped deliberately. 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 that reads a tree an earlier step +mutated. Applied to `self-heal-pr`'s verify step it would hand back a clean checkout and +pass on unmodified code, turning an infra failure into a wrong green — worse than the red +it replaces. Those steps need captured bytes restored, which is the `FileRef` chokepoint +still open in `REWRITE.md`. + Corollary to rules 3 and 4: wall-clock bounds must be written explicitly (`timeoutSec` on exec, `Effect.timeoutFail` around waits, as `waitForPort` already does at `sandbox-cf.ts:565-577`). Declaring `maxDurationSec` is documentation, not enforcement, From 5b7a8c40f5a31ba350211ad22307e6088ed4fd2f Mon Sep 17 00:00:00 2001 From: nplusonedev <313439419+nplusonedev@users.noreply.github.com> Date: Sat, 15 Aug 2026 19:56:04 +0000 Subject: [PATCH 2/7] review: answer the domain lenses on the retry and classification change MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- packages/runtime-cf/src/sandbox-cf.ts | 42 ++++++++------------ runs/README.md | 10 +++++ runs/check.ts | 10 ++++- runs/offload-test.ts | 20 +++++++--- runs/oxlint.ts | 10 ++++- specs/adr/0001-cloudflare-workflows-scope.md | 13 ++++++ specs/adr/README.md | 2 +- 7 files changed, 73 insertions(+), 34 deletions(-) diff --git a/packages/runtime-cf/src/sandbox-cf.ts b/packages/runtime-cf/src/sandbox-cf.ts index bc67df4..e367c63 100644 --- a/packages/runtime-cf/src/sandbox-cf.ts +++ b/packages/runtime-cf/src/sandbox-cf.ts @@ -185,16 +185,6 @@ interface RawExecResult { * keeps the container warm across the inter-step gap so this rarely fires; this * is the honesty backstop for the residual eviction/replay cases.) */ -/** - * Marks the throw raised for a vanished working directory, so the `catch` below - * classifies it without re-reading the message it just wrote. A symbol, so it - * cannot collide with a property an SDK error carries. - */ -const WORKSPACE_MISSING: unique symbol = Symbol("workspaceMissing"); - -const isWorkspaceMissingThrow = (cause: unknown): boolean => - typeof cause === "object" && cause !== null && WORKSPACE_MISSING in cause; - export const isWorkingDirFailure = ( r: { readonly exitCode: number; readonly stdout: string; readonly stderr: string }, cwd: string | undefined, @@ -627,13 +617,22 @@ export const makeSandboxCloudflareLive = ( // fold a phantom non-zero result a `failOnNonZeroExit` run would render // as a lint/test verdict (see `isWorkingDirFailure`). The throw is // classified by the `catch` below. + // Classified HERE rather than in the `catch`, which reads the message + // and would see the `cwd` this one embeds: a checkout under a path + // like `/workspace/request-timeout` matches its timeout regex and + // lands as `ExecTimeout`, a class `RETRY_ON` excludes on purpose — so + // the step would die unretried and `ensureWorkspace` never get its + // second chance. if (isWorkingDirFailure(result, cwd)) { - throw Object.assign( - new Error( - `working directory '${cwd}' was missing at exec time — the checkout did not survive to this step (container recycled). stderr: ${stderr.slice(0, 200)}`, + throw new ExecFailed({ + exitCode: -1, + stderrTail: diagnosticTail( + new Error( + `working directory '${cwd}' was missing at exec time — the checkout did not survive to this step (container recycled). stderr: ${stderr.slice(0, 200)}`, + ), + redactValues, ), - { [WORKSPACE_MISSING]: true }, - ); + }); } // Only a bounded TAIL is inlined in the step's return value, so the // Workflow checkpoint stays small (see `inlineTail`). When a viewer @@ -654,17 +653,8 @@ export const makeSandboxCloudflareLive = ( // generic launch failure. Prefer any stdout/stderr the throw carried // (some SDK errors attach them); else the Error message — this is // what Workflows persists via ExecFailed.message (#88). - // Ahead of the message regex. The marked throw embeds `cwd` and 200 - // chars of stderr, so a checkout under a path like - // `/workspace/request-timeout` matches that regex and lands as - // `ExecTimeout` — which `RETRY_ON` deliberately excludes, so the step - // dies unretried and `ensureWorkspace` never gets its second chance. - if (isWorkspaceMissingThrow(cause)) { - return new ExecFailed({ - exitCode: -1, - stderrTail: diagnosticTail(cause, redactValues), - }); - } + // Already classified at the throw site, message-independent. + if (cause instanceof ExecFailed) return cause; const message = cause instanceof Error ? cause.message : String(cause); if (/timed?\s*out|timeout/i.test(message)) { return new ExecTimeout({ diff --git a/runs/README.md b/runs/README.md index a1fbe95..092cbc1 100644 --- a/runs/README.md +++ b/runs/README.md @@ -276,6 +276,12 @@ command in the same missing directory three times and reported a failure about a missing directory rather than anything about the code. The retry could never have worked: the thing it needed was the thing that was gone. +That classification is made where the failure is raised, not by reading the +message afterwards. The message embeds the missing `cwd`, and the runtime also +matches `/timeout/i` on it to spot an SDK timeout — so a checkout under a path +like `/workspace/request-timeout` used to land as `ExecTimeout`, which `retryOn` +excludes on purpose, and the step died without ever retrying. + So **every** PR run and every path within it — `offload-test` staged and single-exec, `check`, and `oxlint` — calls the `ensureWorkspace` primitive inside its retryable step: it @@ -283,6 +289,10 @@ probes `test -d /.git` and re-clones when the probe fails. On the happy pat that is one extra exec of about a second; on a recycled container it is a clone and an install, which is what the step was going to need anyway. +Because that re-clone runs inside the step, `retryOn` covers `CheckoutFailed` +alongside `ExecFailed` and `StepFailed`: a clone that flakes mid-recovery would +otherwise end the step one call short of the repair it was there to perform. + Isolated stages need no probe: they acquire a workspace inside the retryable step already, so a retry rebuilds it by construction. diff --git a/runs/check.ts b/runs/check.ts index 484c0cf..b4f79b8 100644 --- a/runs/check.ts +++ b/runs/check.ts @@ -174,7 +174,15 @@ const STEP_TIMEOUT_HEADROOM_SEC = 120; * `CheckoutFailed` is included because `ensureWorkspace` re-clones inside this * step, in the same weather that destroyed the checkout. A transient clone * failure there would end the step non-retryably, one call short of the - * recovery it was invoked to perform. + * recovery it was invoked to perform. It is a catch-all that also wraps + * deterministic failures, and what keeps those off this step is the sequence + * rather than the tag — see the longer note in `offload-test.ts`. + * + * `ContainerLaunchFailed` and `ContainerBusy` reach here too, from + * `ensureWorkspace`'s `sandbox.acquire`, and stay OUT: `acquire` already waits + * to the layer's ceiling before raising `ContainerBusy`, so retrying it here + * stacks a second wait on top of admission control rather than recovering + * anything. */ const PLATFORM_RETRIES = 3; const RETRY_ON = ["ExecFailed", "StepFailed", "CheckoutFailed"] as const; diff --git a/runs/offload-test.ts b/runs/offload-test.ts index b303fbe..1b441ab 100644 --- a/runs/offload-test.ts +++ b/runs/offload-test.ts @@ -251,16 +251,15 @@ const STEP_TIMEOUT_HEADROOM_SEC = 120; const PLATFORM_RETRIES = 3; /** - * The classes a stage step retries — both of which are the PLATFORM, never a + * The classes a stage step retries — all of which are the PLATFORM, never a * verdict. * * `ExecFailed` is the obvious one: the command could not run. `StepFailed` is * the one that was missing, and its absence made the whole retry policy inert * on the failure it was written for. * - * A step body here fails in three typed ways — `ExecFailed`, `ExecTimeout` and - * `CheckoutFailed` — because a command that RUNS and exits non-zero comes back as - * a normal `ExecResult`. When the platform kills the step outright, no Effect + * A command that RUNS and exits non-zero comes back as a normal `ExecResult`, so + * no tag listed here can carry one. When the platform kills the step outright, no Effect * `Cause` survives the Workflow boundary, `errorTagOf` falls back to * `"StepFailed"`, and a `retryOn` listing only `ExecFailed` classified that as * non-retryable — so the one failure mode that is purely the platform's was the @@ -277,6 +276,16 @@ const PLATFORM_RETRIES = 3; * failure would otherwise end the step non-retryably, one call short of the * recovery it was invoked to perform. * + * `CheckoutFailed` is a catch-all over the whole clone body, and several of the + * failures it wraps are deterministic: a bad sha, a repo that is gone, the + * facade's pinned-recipe mismatch (`sandbox-facade.ts:277-287`). Retrying those + * would be four attempts at something that cannot succeed. What keeps that off + * this step is the sequence, not the tag: the only clone reachable from here is + * `ensureWorkspace` re-cloning the SAME repo and sha that the `checkout` step + * already cloned successfully earlier in this run. A deterministic clone failure + * would have ended the run at `checkout`, before any of this. What survives that + * filter is transient, plus a credential that expired mid-run. + * * `ExecTimeout` stays OUT, deliberately. Its tag survives the boundary intact * whenever there is a Cause to read, so it lands here as itself rather than as * `StepFailed` — and a command that outran its ceiling will outrun it again. @@ -856,7 +865,8 @@ export const offloadTest = defineRun({ // `printf '%s\n' ''`, so free vendor text could close the // quote. A `[A-Za-z0-9_-]` id and a signed integer cannot. const rendered = Option.match(Cause.failureOption(exit.cause), { - onSome: (f) => `${String((f as { cause?: unknown }).cause ?? "")} ${String((f as { message?: unknown }).message ?? "")}`, + onSome: (f) => + `${String((f as { cause?: unknown }).cause ?? "")} ${String((f as { message?: unknown }).message ?? "")}`, onNone: () => "", }); const reference = /reference\s*=\s*([A-Za-z0-9_-]{1,64})/.exec(rendered)?.[1]; diff --git a/runs/oxlint.ts b/runs/oxlint.ts index 9524144..bebd839 100644 --- a/runs/oxlint.ts +++ b/runs/oxlint.ts @@ -107,7 +107,15 @@ const TIMEOUT_SEC_DEFAULT = 300; * `CheckoutFailed` is included because `ensureWorkspace` re-clones inside this * step, in the same weather that destroyed the checkout. A transient clone * failure there would end the step non-retryably, one call short of the - * recovery it was invoked to perform. + * recovery it was invoked to perform. It is a catch-all that also wraps + * deterministic failures, and what keeps those off this step is the sequence + * rather than the tag — see the longer note in `offload-test.ts`. + * + * `ContainerLaunchFailed` and `ContainerBusy` reach here too, from + * `ensureWorkspace`'s `sandbox.acquire`, and stay OUT: `acquire` already waits + * to the layer's ceiling before raising `ContainerBusy`, so retrying it here + * stacks a second wait on top of admission control rather than recovering + * anything. */ const PLATFORM_RETRIES = 3; const RETRY_ON = ["ExecFailed", "StepFailed", "CheckoutFailed"] as const; diff --git a/specs/adr/0001-cloudflare-workflows-scope.md b/specs/adr/0001-cloudflare-workflows-scope.md index 7946121..f6338c3 100644 --- a/specs/adr/0001-cloudflare-workflows-scope.md +++ b/specs/adr/0001-cloudflare-workflows-scope.md @@ -168,6 +168,14 @@ anyway. Any new hibernating run cites this ADR and names its decider and timeout. - Step count and step-result size become design constraints runs are expected to respect, not incidental limits discovered in production. +- Rule 5 costs one `test -d` probe per retryable step on the happy path, and a clone plus + install when the probe misses. `CheckoutFailed` joins the retried set in those steps, + because the repair's own clone must survive the weather that made it necessary. +- Rule 5 is not yet repo-wide. `check`, `oxlint` and `offload-test` call `ensureWorkspace`; + `cdp-acceptance`, `matrix-fanout`, `playwright-e2e`, `playwright-demo`, `pr-review`, + `refresh-fixtures`, `spec-drift-pr`, `worker-deploy`, `org-spec-audit` and + `release-notes` still call `workspace()` bare. Each is either mechanically portable or + in the mutated-tree class the rule excludes, and that call is per run. ## Revisit triggers @@ -182,3 +190,8 @@ anyway. to keep entity state. - `maxDurationSec` becomes enforced, or Workflows introduces an instance-level wall-clock bound, changing what rule 2's corollary has to do by hand. +- The `FileRef` capture-and-restore chokepoint lands, making the mutated-tree case + serviceable. Rule 5's exclusion exists only because a re-clone is the wrong repair + there, and restoring captured bytes would let those steps recover too. +- Containers gain durable or reattachable disk, which would retire rule 5 rather than + extend it. diff --git a/specs/adr/README.md b/specs/adr/README.md index 73255bb..4526854 100644 --- a/specs/adr/README.md +++ b/specs/adr/README.md @@ -11,6 +11,6 @@ interfaces or assume they exist. | ADR | Status | Decision | | --- | --- | --- | -| [0001](./0001-cloudflare-workflows-scope.md) | proposed | How runs use Cloudflare Workflows: one instance per dispatch, hibernation reserved for bounded human decisions, entity lifecycles keep state in the system of record | +| [0001](./0001-cloudflare-workflows-scope.md) | proposed | How runs use Cloudflare Workflows: one instance per dispatch, hibernation reserved for bounded human decisions, entity lifecycles keep state in the system of record, container disk is never durable state | | [0002](./0002-memory-capability.md) | proposed | Org context is an optional, total `memory` capability with MCP-speaking adapters; runs consume, never produce | | [0003](./0003-no-context-relay.md) | proposed | Context backends ingest GitHub directly; FlareDispatch builds no relay, export, or pull endpoint for them | From 0821e6ed7ff12ed8455c3b5518cf78abe70a06af Mon Sep 17 00:00:00 2001 From: nplusonedev <313439419+nplusonedev@users.noreply.github.com> Date: Sat, 15 Aug 2026 20:15:21 +0000 Subject: [PATCH 3/7] review: correct two false claims the reviewer caught, and cut the prose MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- packages/runtime-cf/src/sandbox-cf.test.ts | 4 ---- packages/runtime-cf/src/sandbox-cf.ts | 11 +++------ .../runtime-cf/src/step-runner-cf.test.ts | 23 +++++++++++++++++-- runs/check.ts | 10 +------- runs/offload-test.ts | 20 ++++------------ runs/oxlint.ts | 10 +------- specs/adr/0001-cloudflare-workflows-scope.md | 15 +++++++----- 7 files changed, 40 insertions(+), 53 deletions(-) diff --git a/packages/runtime-cf/src/sandbox-cf.test.ts b/packages/runtime-cf/src/sandbox-cf.test.ts index 1fd2c1b..bb9b506 100644 --- a/packages/runtime-cf/src/sandbox-cf.test.ts +++ b/packages/runtime-cf/src/sandbox-cf.test.ts @@ -431,10 +431,6 @@ describe("makeSandboxCloudflareLive — exec result folding (D)", () => { it.effect("a checkout path containing 'timeout' still fails as ExecFailed, not ExecTimeout", () => Effect.gen(function* () { - // The throw embeds `cwd`, and the `catch` also matches /timeout/i to spot - // an SDK timeout. Classify by message first and this lands as ExecTimeout - // — which RETRY_ON excludes on purpose, so the step dies unretried and the - // rebuild never runs. currentBox = makeFakeBox({ proc: null }); currentBox.exec = vi.fn(async () => ({ exitCode: 1, diff --git a/packages/runtime-cf/src/sandbox-cf.ts b/packages/runtime-cf/src/sandbox-cf.ts index e367c63..46004a7 100644 --- a/packages/runtime-cf/src/sandbox-cf.ts +++ b/packages/runtime-cf/src/sandbox-cf.ts @@ -615,14 +615,9 @@ export const makeSandboxCloudflareLive = ( // never ran — the checkout did not survive to this exec (container // recycled between durable steps). Raise a real ExecFailed rather than // fold a phantom non-zero result a `failOnNonZeroExit` run would render - // as a lint/test verdict (see `isWorkingDirFailure`). The throw is - // classified by the `catch` below. - // Classified HERE rather than in the `catch`, which reads the message - // and would see the `cwd` this one embeds: a checkout under a path - // like `/workspace/request-timeout` matches its timeout regex and - // lands as `ExecTimeout`, a class `RETRY_ON` excludes on purpose — so - // the step would die unretried and `ensureWorkspace` never get its - // second chance. + // as a lint/test verdict (see `isWorkingDirFailure`). Built HERE, not + // in the `catch`, which reads the message and would see the `cwd` this + // one embeds — pinned by "a checkout path containing 'timeout'". if (isWorkingDirFailure(result, cwd)) { throw new ExecFailed({ exitCode: -1, diff --git a/packages/runtime-cf/src/step-runner-cf.test.ts b/packages/runtime-cf/src/step-runner-cf.test.ts index e671afa..a7a3567 100644 --- a/packages/runtime-cf/src/step-runner-cf.test.ts +++ b/packages/runtime-cf/src/step-runner-cf.test.ts @@ -5,10 +5,15 @@ import { Cause, Effect } from "effect"; import { describe, expect, it } from "vitest"; -import { ExecFailed, ExecTimeout, runEffect } from "@fractalboxdev/flare-dispatch-core"; +import { + CheckoutFailed, + ExecFailed, + ExecTimeout, + runEffect, +} from "@fractalboxdev/flare-dispatch-core"; import { buildStepConfig, rethrowForRetryPolicy } from "./step-runner-cf"; -const thrownFor = async (failure: ExecFailed | ExecTimeout): Promise => { +const thrownFor = async (failure: ExecFailed | ExecTimeout | CheckoutFailed): Promise => { try { await runEffect(Effect.fail(failure)); throw new Error("expected a throw"); @@ -67,6 +72,20 @@ describe("rethrowForRetryPolicy", () => { expect(Cause.isCause(out.cause)).toBe(true); }); + it("CheckoutFailed passes through under the runs' policy, so the rebuild's own clone retries", async () => { + const thrown = await thrownFor(new CheckoutFailed({ repo: "o/r", sha: "abc", cause: "flake" })); + expect(rethrowForRetryPolicy(thrown, ["ExecFailed", "StepFailed", "CheckoutFailed"])).toBe( + thrown, + ); + }); + + it("CheckoutFailed is non-retryable under a policy that omits it", async () => { + const thrown = await thrownFor(new CheckoutFailed({ repo: "o/r", sha: "abc", cause: "flake" })); + const out = rethrowForRetryPolicy(thrown, ["ExecFailed", "StepFailed"]) as Error; + expect(out).not.toBe(thrown); + expect(out.name).toBe("NonRetryableError"); + }); + it("without retryOn every failure stays retryable — the platform default", async () => { const thrown = await thrownFor(new ExecTimeout({ timeoutSec: 600, command: "pnpm test" })); expect(rethrowForRetryPolicy(thrown, undefined)).toBe(thrown); diff --git a/runs/check.ts b/runs/check.ts index b4f79b8..484c0cf 100644 --- a/runs/check.ts +++ b/runs/check.ts @@ -174,15 +174,7 @@ const STEP_TIMEOUT_HEADROOM_SEC = 120; * `CheckoutFailed` is included because `ensureWorkspace` re-clones inside this * step, in the same weather that destroyed the checkout. A transient clone * failure there would end the step non-retryably, one call short of the - * recovery it was invoked to perform. It is a catch-all that also wraps - * deterministic failures, and what keeps those off this step is the sequence - * rather than the tag — see the longer note in `offload-test.ts`. - * - * `ContainerLaunchFailed` and `ContainerBusy` reach here too, from - * `ensureWorkspace`'s `sandbox.acquire`, and stay OUT: `acquire` already waits - * to the layer's ceiling before raising `ContainerBusy`, so retrying it here - * stacks a second wait on top of admission control rather than recovering - * anything. + * recovery it was invoked to perform. */ const PLATFORM_RETRIES = 3; const RETRY_ON = ["ExecFailed", "StepFailed", "CheckoutFailed"] as const; diff --git a/runs/offload-test.ts b/runs/offload-test.ts index 1b441ab..570e379 100644 --- a/runs/offload-test.ts +++ b/runs/offload-test.ts @@ -276,25 +276,15 @@ const PLATFORM_RETRIES = 3; * failure would otherwise end the step non-retryably, one call short of the * recovery it was invoked to perform. * - * `CheckoutFailed` is a catch-all over the whole clone body, and several of the - * failures it wraps are deterministic: a bad sha, a repo that is gone, the - * facade's pinned-recipe mismatch (`sandbox-facade.ts:277-287`). Retrying those - * would be four attempts at something that cannot succeed. What keeps that off - * this step is the sequence, not the tag: the only clone reachable from here is - * `ensureWorkspace` re-cloning the SAME repo and sha that the `checkout` step - * already cloned successfully earlier in this run. A deterministic clone failure - * would have ended the run at `checkout`, before any of this. What survives that - * filter is transient, plus a credential that expired mid-run. + * `CheckoutFailed` is a catch-all over the whole clone body, so deterministic + * failures ride it too — a bad sha, a repo that is gone. On the isolated path a + * stage's clone is the run's FIRST, with no earlier `checkout` to have caught + * those, so this knowingly spends the retry budget on some failures that cannot + * succeed. The cost is bounded by the backoff; the alternative loses the repair. * * `ExecTimeout` stays OUT, deliberately. Its tag survives the boundary intact * whenever there is a Cause to read, so it lands here as itself rather than as * `StepFailed` — and a command that outran its ceiling will outrun it again. - * - * `ContainerLaunchFailed` and `ContainerBusy` also reach here from - * `ensureWorkspace`'s `sandbox.acquire`, and stay OUT for now: `acquire` already - * waits to the layer's ceiling before raising `ContainerBusy`, so retrying it - * here stacks a second wait on top of admission control rather than recovering - * anything. That coupling deserves its own change, not a line in this one. */ const RETRY_ON = ["ExecFailed", "StepFailed", "CheckoutFailed"] as const; diff --git a/runs/oxlint.ts b/runs/oxlint.ts index bebd839..9524144 100644 --- a/runs/oxlint.ts +++ b/runs/oxlint.ts @@ -107,15 +107,7 @@ const TIMEOUT_SEC_DEFAULT = 300; * `CheckoutFailed` is included because `ensureWorkspace` re-clones inside this * step, in the same weather that destroyed the checkout. A transient clone * failure there would end the step non-retryably, one call short of the - * recovery it was invoked to perform. It is a catch-all that also wraps - * deterministic failures, and what keeps those off this step is the sequence - * rather than the tag — see the longer note in `offload-test.ts`. - * - * `ContainerLaunchFailed` and `ContainerBusy` reach here too, from - * `ensureWorkspace`'s `sandbox.acquire`, and stay OUT: `acquire` already waits - * to the layer's ceiling before raising `ContainerBusy`, so retrying it here - * stacks a second wait on top of admission control rather than recovering - * anything. + * recovery it was invoked to perform. */ const PLATFORM_RETRIES = 3; const RETRY_ON = ["ExecFailed", "StepFailed", "CheckoutFailed"] as const; diff --git a/specs/adr/0001-cloudflare-workflows-scope.md b/specs/adr/0001-cloudflare-workflows-scope.md index f6338c3..4b0a3a6 100644 --- a/specs/adr/0001-cloudflare-workflows-scope.md +++ b/specs/adr/0001-cloudflare-workflows-scope.md @@ -98,9 +98,11 @@ next time it is started, it will have a fresh disk as defined by its container i ([Containers FAQ](https://developers.cloudflare.com/containers/faq/)). The same pages give no guaranteed minimum runtime, restart an out-of-memory instance, and terminate instances on host restarts. A `checkout` step that already completed is never re-run by -replay, so every attempt after a container swap hits an empty disk. `ensureWorkspace` -(`packages/core/src/primitives/workspace.ts`) is the repair, and it belongs INSIDE the -retryable step — a rebuild the retry does not re-run is not a rebuild. +replay, so on the container backend every attempt after a container swap hits an empty +disk. `ensureWorkspace` (`packages/core/src/primitives/workspace.ts`) is the repair, and +it belongs INSIDE the retryable step — a rebuild the retry does not re-run is not a +rebuild. On the substrate backend (`SUBSTRATE_BACKEND=on`) the facade restores the tree +inside the exec fence, so the probe is redundant there rather than wrong. Scoped deliberately. 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 that reads a tree an earlier step @@ -173,9 +175,10 @@ anyway. because the repair's own clone must survive the weather that made it necessary. - Rule 5 is not yet repo-wide. `check`, `oxlint` and `offload-test` call `ensureWorkspace`; `cdp-acceptance`, `matrix-fanout`, `playwright-e2e`, `playwright-demo`, `pr-review`, - `refresh-fixtures`, `spec-drift-pr`, `worker-deploy`, `org-spec-audit` and - `release-notes` still call `workspace()` bare. Each is either mechanically portable or - in the mutated-tree class the rule excludes, and that call is per run. + `refresh-fixtures`, `spec-drift-pr`, `worker-deploy`, `org-spec-audit`, `vitest-shard` + and `release-notes` still call `workspace()` bare, as does `self-heal-pr`, which is in + the mutated-tree class the rule excludes. Each remaining one is either mechanically + portable or excluded, and that call is per run. ## Revisit triggers From 9531f14661b2f3b237d10d3ed57606a47b990857 Mon Sep 17 00:00:00 2001 From: nplusonedev <313439419+nplusonedev@users.noreply.github.com> Date: Sat, 15 Aug 2026 20:25:59 +0000 Subject: [PATCH 4/7] review: stop the retry docblock arguing with itself 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. --- runs/offload-test.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/runs/offload-test.ts b/runs/offload-test.ts index 570e379..66cdb22 100644 --- a/runs/offload-test.ts +++ b/runs/offload-test.ts @@ -251,8 +251,7 @@ const STEP_TIMEOUT_HEADROOM_SEC = 120; const PLATFORM_RETRIES = 3; /** - * The classes a stage step retries — all of which are the PLATFORM, never a - * verdict. + * The classes a stage step retries — never a verdict. * * `ExecFailed` is the obvious one: the command could not run. `StepFailed` is * the one that was missing, and its absence made the whole retry policy inert @@ -280,7 +279,8 @@ const PLATFORM_RETRIES = 3; * failures ride it too — a bad sha, a repo that is gone. On the isolated path a * stage's clone is the run's FIRST, with no earlier `checkout` to have caught * those, so this knowingly spends the retry budget on some failures that cannot - * succeed. The cost is bounded by the backoff; the alternative loses the repair. + * succeed — four clone attempts plus backoff, times the stages running at once. + * The alternative loses the repair on every path, so it is the worse trade. * * `ExecTimeout` stays OUT, deliberately. Its tag survives the boundary intact * whenever there is a Cause to read, so it lands here as itself rather than as From e3ddaee3464d7f3ce0da282081b0c85f404455b9 Mon Sep 17 00:00:00 2001 From: nplusonedev <313439419+nplusonedev@users.noreply.github.com> Date: Sat, 15 Aug 2026 20:38:39 +0000 Subject: [PATCH 5/7] review: keep the scrub notice visible, and stop the docs contradicting each other MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- apps/dispatcher/src/sandbox.ts | 4 +++- packages/core/src/primitives/workspace.ts | 4 +++- packages/runtime-cf/src/sandbox-cf.ts | 14 ++++++++++---- runs/offload-test.ts | 17 ++++++++++------- specs/adr/0001-cloudflare-workflows-scope.md | 8 ++++++-- 5 files changed, 32 insertions(+), 15 deletions(-) diff --git a/apps/dispatcher/src/sandbox.ts b/apps/dispatcher/src/sandbox.ts index e6bc6fa..93786e4 100644 --- a/apps/dispatcher/src/sandbox.ts +++ b/apps/dispatcher/src/sandbox.ts @@ -52,7 +52,9 @@ import type { Env } from "./env"; * `offload-test`) then mis-rendered as a red lint/test verdict. Because * `destroy()` is the real teardown, a longer idle window costs extra ONLY on the * rare paths that skip finalize, so 10m (the SDK default, and the `LEASE_TTL_MS` - * run-scale) buys durability across normal inter-step gaps at negligible cost. + * run-scale) narrows the idle window at negligible cost. It buys no durability: + * container disk is ephemeral on every path (ADR-0001 rule 5), so a run that + * needs its checkout between steps re-establishes it with `ensureWorkspace`. * `sandbox-cf.ts` `isWorkingDirFailure` is the honesty backstop for the residual * (eviction / replay beyond this window): it re-classifies a lost-workspace exec * as a retryable `ExecFailed`, never a phantom finding. diff --git a/packages/core/src/primitives/workspace.ts b/packages/core/src/primitives/workspace.ts index 726c7c6..ed236aa 100644 --- a/packages/core/src/primitives/workspace.ts +++ b/packages/core/src/primitives/workspace.ts @@ -53,7 +53,9 @@ export const workspace = (opts: { * precondition the failure destroyed is not a retry. * * Call this INSIDE the retryable step, not before it. That placement is the - * whole point: the rebuild has to be part of what a retry re-runs. + * whole point: the rebuild has to be part of what a retry re-runs. That step's + * `retryOn` must list `CheckoutFailed` too, or a clone that flakes mid-rebuild + * ends the step one call short of the repair. * * Costs one `test -d` on the happy path. On a recycled container it is a clone * and an install — which is what the step was about to need anyway. diff --git a/packages/runtime-cf/src/sandbox-cf.ts b/packages/runtime-cf/src/sandbox-cf.ts index 46004a7..31414fc 100644 --- a/packages/runtime-cf/src/sandbox-cf.ts +++ b/packages/runtime-cf/src/sandbox-cf.ts @@ -565,6 +565,13 @@ export const makeSandboxCloudflareLive = ( ); const err = redactCloneFailure(cause, token); if (scrubFailure !== undefined) { + // Logged as well as appended: `CheckoutFailed` is retryable in + // the runs that call `ensureWorkspace`, so an attempt that fails + // here and a later one that succeeds would carry this notice out + // of the run's final result and into per-attempt history alone. + console.warn( + `sandbox.clone: credential scrub failed for ${targetDir} — a token may remain in .git/config until the next attempt clears it`, + ); err.message = `${err.message}\n[post-failure credential scrub ALSO failed: ${redact( scrubFailure, [token], @@ -615,9 +622,9 @@ export const makeSandboxCloudflareLive = ( // never ran — the checkout did not survive to this exec (container // recycled between durable steps). Raise a real ExecFailed rather than // fold a phantom non-zero result a `failOnNonZeroExit` run would render - // as a lint/test verdict (see `isWorkingDirFailure`). Built HERE, not - // in the `catch`, which reads the message and would see the `cwd` this - // one embeds — pinned by "a checkout path containing 'timeout'". + // as a lint/test verdict (see `isWorkingDirFailure`). Built here + // rather than in the `catch`, whose timeout regex would match the + // `cwd` this message embeds. if (isWorkingDirFailure(result, cwd)) { throw new ExecFailed({ exitCode: -1, @@ -648,7 +655,6 @@ export const makeSandboxCloudflareLive = ( // generic launch failure. Prefer any stdout/stderr the throw carried // (some SDK errors attach them); else the Error message — this is // what Workflows persists via ExecFailed.message (#88). - // Already classified at the throw site, message-independent. if (cause instanceof ExecFailed) return cause; const message = cause instanceof Error ? cause.message : String(cause); if (/timed?\s*out|timeout/i.test(message)) { diff --git a/runs/offload-test.ts b/runs/offload-test.ts index 66cdb22..6180b55 100644 --- a/runs/offload-test.ts +++ b/runs/offload-test.ts @@ -257,8 +257,9 @@ const PLATFORM_RETRIES = 3; * the one that was missing, and its absence made the whole retry policy inert * on the failure it was written for. * - * A command that RUNS and exits non-zero comes back as a normal `ExecResult`, so - * no tag listed here can carry one. When the platform kills the step outright, no Effect + * A command that RUNS and exits non-zero comes back as a normal `ExecResult`, + * so no tag listed here can carry one. When the platform kills the step + * outright, no Effect * `Cause` survives the Workflow boundary, `errorTagOf` falls back to * `"StepFailed"`, and a `retryOn` listing only `ExecFailed` classified that as * non-retryable — so the one failure mode that is purely the platform's was the @@ -276,11 +277,13 @@ const PLATFORM_RETRIES = 3; * recovery it was invoked to perform. * * `CheckoutFailed` is a catch-all over the whole clone body, so deterministic - * failures ride it too — a bad sha, a repo that is gone. On the isolated path a - * stage's clone is the run's FIRST, with no earlier `checkout` to have caught - * those, so this knowingly spends the retry budget on some failures that cannot - * succeed — four clone attempts plus backoff, times the stages running at once. - * The alternative loses the repair on every path, so it is the worse trade. + * failures ride it too — a bad sha, a repo that is gone, and on the substrate + * backend the recipe-pin mismatch. On the isolated path a stage's clone is the + * run's FIRST, with no earlier `checkout` to have caught those, so this + * knowingly spends the retry budget on some failures that cannot succeed: four + * clone attempts plus backoff, times the stages running at once. Each attempt + * also mints its own installation token. The alternative loses the repair on + * every path, so it is the worse trade. * * `ExecTimeout` stays OUT, deliberately. Its tag survives the boundary intact * whenever there is a Cause to read, so it lands here as itself rather than as diff --git a/specs/adr/0001-cloudflare-workflows-scope.md b/specs/adr/0001-cloudflare-workflows-scope.md index 4b0a3a6..b0238e4 100644 --- a/specs/adr/0001-cloudflare-workflows-scope.md +++ b/specs/adr/0001-cloudflare-workflows-scope.md @@ -101,8 +101,12 @@ instances on host restarts. A `checkout` step that already completed is never re replay, so on the container backend every attempt after a container swap hits an empty disk. `ensureWorkspace` (`packages/core/src/primitives/workspace.ts`) is the repair, and it belongs INSIDE the retryable step — a rebuild the retry does not re-run is not a -rebuild. On the substrate backend (`SUBSTRATE_BACKEND=on`) the facade restores the tree -inside the exec fence, so the probe is redundant there rather than wrong. +rebuild. On the substrate backend (`SUBSTRATE_BACKEND=on`, `wrangler.jsonc` is `"off"` +today) `execUnderGrant` rebuilds inside the fence (`apps/substrate/src/facade.ts:210-212`), +so the probe is redundant there rather than wrong — but not free, and not a no-op: that +deploy still pays one fenced exec per retryable step, gains a retry on pool-admission +refusals, and pays a retried recipe-pin mismatch, because +`packages/runtime-cf/src/sandbox-facade.ts:274-287` folds both into `CheckoutFailed`. Scoped deliberately. 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 that reads a tree an earlier step From 357164272fadd4208e19cb4c4096b8db0a0f2b5e Mon Sep 17 00:00:00 2001 From: nplusonedev <313439419+nplusonedev@users.noreply.github.com> Date: Sat, 15 Aug 2026 20:47:26 +0000 Subject: [PATCH 6/7] review: scope the token-mint note to the backend that mints one 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. --- runs/offload-test.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/runs/offload-test.ts b/runs/offload-test.ts index 6180b55..2d8d446 100644 --- a/runs/offload-test.ts +++ b/runs/offload-test.ts @@ -281,9 +281,9 @@ const PLATFORM_RETRIES = 3; * backend the recipe-pin mismatch. On the isolated path a stage's clone is the * run's FIRST, with no earlier `checkout` to have caught those, so this * knowingly spends the retry budget on some failures that cannot succeed: four - * clone attempts plus backoff, times the stages running at once. Each attempt - * also mints its own installation token. The alternative loses the repair on - * every path, so it is the worse trade. + * clone attempts plus backoff, times the stages running at once, each minting + * its own installation token on the container backend. The alternative loses + * the repair on every path, so it is the worse trade. * * `ExecTimeout` stays OUT, deliberately. Its tag survives the boundary intact * whenever there is a Cause to read, so it lands here as itself rather than as From 7f01295e90306d876fabeb7d969e7154de164623 Mon Sep 17 00:00:00 2001 From: nplusonedev <313439419+nplusonedev@users.noreply.github.com> Date: Sat, 15 Aug 2026 21:21:38 +0000 Subject: [PATCH 7/7] =?UTF-8?q?review:=20sleepAfter=20is=20a=20grace=20per?= =?UTF-8?q?iod,=20not=20a=20narrowing=20=E2=80=94=20say=20so?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- apps/dispatcher/src/sandbox.ts | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/apps/dispatcher/src/sandbox.ts b/apps/dispatcher/src/sandbox.ts index 93786e4..f40cb5f 100644 --- a/apps/dispatcher/src/sandbox.ts +++ b/apps/dispatcher/src/sandbox.ts @@ -52,9 +52,11 @@ import type { Env } from "./env"; * `offload-test`) then mis-rendered as a red lint/test verdict. Because * `destroy()` is the real teardown, a longer idle window costs extra ONLY on the * rare paths that skip finalize, so 10m (the SDK default, and the `LEASE_TTL_MS` - * run-scale) narrows the idle window at negligible cost. It buys no durability: - * container disk is ephemeral on every path (ADR-0001 rule 5), so a run that - * needs its checkout between steps re-establishes it with `ensureWorkspace`. + * run-scale) is close to free. It is a grace period, not a durability + * mechanism: it EXTENDS how long an idle container stays awake, and it + * guarantees nothing — container disk is ephemeral on every path (ADR-0001 + * rule 5), so a run that needs its checkout between steps re-establishes it + * with `ensureWorkspace`. * `sandbox-cf.ts` `isWorkingDirFailure` is the honesty backstop for the residual * (eviction / replay beyond this window): it re-classifies a lost-workspace exec * as a retryable `ExecFailed`, never a phantom finding.