diff --git a/runs/README.md b/runs/README.md index 99810e9..a1fbe95 100644 --- a/runs/README.md +++ b/runs/README.md @@ -276,8 +276,9 @@ 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. -So **every** PR run — `offload-test`'s shared-container stages, `check`, and -`oxlint` — calls the `ensureWorkspace` primitive inside its retryable step: it +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 probes `test -d /.git` and re-clones when the probe fails. On the happy path 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. diff --git a/runs/check.test.ts b/runs/check.test.ts index d5fe9e5..feb65cb 100644 --- a/runs/check.test.ts +++ b/runs/check.test.ts @@ -121,7 +121,7 @@ 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"]); + expect(execStep?.metadata?.["stepOpts.retryOn"]).toEqual(["ExecFailed", "StepFailed"]); }).pipe(Effect.provide(layer)); }, ); diff --git a/runs/check.ts b/runs/check.ts index 2b0a0a8..fd5fa29 100644 --- a/runs/check.ts +++ b/runs/check.ts @@ -162,9 +162,17 @@ const DEFAULT_TIMEOUT_SEC = 600; */ const STEP_TIMEOUT_HEADROOM_SEC = 120; -/** Platform-failure retries — see the `exec` step. A verdict is never retried. */ +/** + * 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. + * + * `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. + */ const PLATFORM_RETRIES = 3; -const RETRY_ON = ["ExecFailed"] as const; +const RETRY_ON = ["ExecFailed", "StepFailed"] 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 3e3b306..6101e57 100644 --- a/runs/offload-test.test.ts +++ b/runs/offload-test.test.ts @@ -126,7 +126,7 @@ 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"]); + expect(execStep?.metadata?.["stepOpts.retryOn"]).toEqual(["ExecFailed", "StepFailed"]); }).pipe(Effect.provide(layer)); }); @@ -232,7 +232,7 @@ 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"]); + expect(execStep?.metadata?.["stepOpts.retryOn"]).toEqual(["ExecFailed", "StepFailed"]); }).pipe(Effect.provide(layer)); }, ); @@ -658,6 +658,32 @@ describe("offload-test", () => { // earlier stage's log; a stage step that dies gets a one-line marker uploaded // under its log name. ABSENT key → the 1.1.0 behaviour, byte-identical — the // unstaged tests above are the pin for that. +describe("offload-test single-exec", () => { + it.effect("a recycled container is re-cloned rather than retried into", () => { + const { layer, handles } = makeCFRuntimeTest({ + sandboxProgram: { + // The probe answers non-zero: the container was recycled between + // `checkout` and `exec`, which is one boundary every run has. + "test -d /workspace/name/.git": { exitCode: 1 }, + "pnpm test": { exitCode: 0 }, + }, + }); + + return Effect.gen(function* () { + const result = yield* offloadTest.run(baseInput); + + // TWO clones: the run's own checkout, then the rebuild inside the exec + // step. #128 left this path uncovered on the grounds that its "exposure + // is one step"; the exposure is one BOUNDARY, and every run has one. + expect(handles.sandbox.clones).toEqual([ + { repo: "owner/name", sha: "abc123" }, + { repo: "owner/name", sha: "abc123" }, + ]); + expect(result.exitCode).toBe(0); + }).pipe(Effect.provide(layer)); + }); +}); + describe("offload-test staged mode", () => { // Webhook-shaped input — staged mode only exists on the path that omits // `command` (the resolve step is where the stages key is read). @@ -725,7 +751,7 @@ 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"]); + expect(execWorkspace?.metadata?.["stepOpts.retryOn"]).toEqual(["ExecFailed", "StepFailed"]); // The suspicious labelled-key-missing fallback is recorded on the // stage's step metadata — `workspace` resolved its own key, so only // `features` is flagged. @@ -1129,7 +1155,7 @@ 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"]); + expect(execA?.metadata?.["stepOpts.retryOn"]).toEqual(["ExecFailed", "StepFailed"]); expect(result.exitCode).toBe(0); expect(result.durationMs).toBe(300); diff --git a/runs/offload-test.ts b/runs/offload-test.ts index 4b65956..afdb1ae 100644 --- a/runs/offload-test.ts +++ b/runs/offload-test.ts @@ -250,7 +250,32 @@ const STEP_TIMEOUT_HEADROOM_SEC = 120; const PLATFORM_RETRIES = 3; -const RETRY_ON = ["ExecFailed"] as const; +/** + * The classes a stage step retries — both 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 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 + * `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 + * one the platform was never asked to retry. + * + * Observed: a 70-second TypeScript stage died as `StepFailed` after ~80s with + * `check` and `oxlint` green beside it, and no retry was attempted. The same + * 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. + * + * `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. + */ +const RETRY_ON = ["ExecFailed", "StepFailed"] as const; const stepTimeoutFor = (execTimeoutSec: number): number => execTimeoutSec + STEP_TIMEOUT_HEADROOM_SEC; @@ -1138,15 +1163,37 @@ export const offloadTest = defineRun({ const result = yield* step( "exec", () => - sandbox.exec({ - cwd: soleWorkspace.dir, - container: soleWorkspace.container, - command: soleCommand, - // Per-dispatch `env` wins over a same-named config-store secret — - // the more specific source overrides the global one. - env: { ...secretEnv, ...input.env }, - timeoutSec, - }), + // The checkout is re-established INSIDE the retryable step, for the + // same reason the staged path does it: a container recycled between + // `checkout` and here takes the checkout with it, and the retry would + // otherwise re-run the command in a directory that is gone. + // + // #128 left this path alone, reasoning that a single-exec run's + // "exposure is one step rather than five". That was wrong twice over. + // A container can be recycled between ANY two durable steps, and + // `checkout` is a step earlier than this one by construction — so the + // exposure is one BOUNDARY, which every run has. This repo's own gate + // then died exactly that way: `working directory + // '/workspace/' was missing at exec time`. + ensureWorkspace({ + current: soleWorkspace, + repo: input.repo, + sha: input.sha, + ...(input.image !== undefined ? { image: input.image } : {}), + install, + }).pipe( + Effect.flatMap((ws) => + sandbox.exec({ + cwd: ws.dir, + container: ws.container, + command: soleCommand, + // Per-dispatch `env` wins over a same-named config-store secret + // — the more specific source overrides the global one. + env: { ...secretEnv, ...input.env }, + timeoutSec, + }), + ), + ), { timeoutSec: stepTimeoutFor(timeoutSec), retries: PLATFORM_RETRIES, retryOn: RETRY_ON }, ); diff --git a/runs/oxlint.test.ts b/runs/oxlint.test.ts index 60d6a7c..56750ea 100644 --- a/runs/oxlint.test.ts +++ b/runs/oxlint.test.ts @@ -289,7 +289,7 @@ 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"]); + expect(execStep?.metadata?.["stepOpts.retryOn"]).toEqual(["ExecFailed", "StepFailed"]); }).pipe(Effect.provide(layer)); }); }); diff --git a/runs/oxlint.ts b/runs/oxlint.ts index 13ce53e..aa95c86 100644 --- a/runs/oxlint.ts +++ b/runs/oxlint.ts @@ -95,9 +95,17 @@ const OxlintOutput = Schema.Struct({ /** Default `exec` timeout — lint is fast, so a tighter ceiling than tests. */ const TIMEOUT_SEC_DEFAULT = 300; -/** Platform-failure retries — see the `exec` step. A verdict is never retried. */ +/** + * 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. + * + * `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. + */ const PLATFORM_RETRIES = 3; -const RETRY_ON = ["ExecFailed"] as const; +const RETRY_ON = ["ExecFailed", "StepFailed"] as const; export const oxlint = defineRun({ name: "oxlint",