From 978f4cc7a6b1890110bda212fba5ccaea6e19c7c Mon Sep 17 00:00:00 2001 From: nplusonedev <313439419+nplusonedev@users.noreply.github.com> Date: Fri, 14 Aug 2026 21:18:20 +0000 Subject: [PATCH] fix(sandbox): rebuild a checkout the container threw away MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 '' was missing at exec time attempt 4 (0.6s) working directory '' 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. --- REWRITE.md | 2 +- apps/dispatcher/src/sandbox.ts | 17 ++- packages/core/README.md | 1 + packages/core/src/errors.ts | 2 + packages/core/src/fakes/sandbox-fake.ts | 25 +++++ .../src/primitives/exec-in-workspace.test.ts | 103 ++++++++++++++++++ .../core/src/primitives/exec-in-workspace.ts | 43 ++++++++ packages/core/src/primitives/index.ts | 3 +- packages/core/src/primitives/workspace.ts | 39 +++++-- packages/runtime-cf/src/sandbox-cf.test.ts | 38 +++++++ packages/runtime-cf/src/sandbox-cf.ts | 27 ++++- runs/README.md | 3 + runs/offload-test.test.ts | 6 +- runs/offload-test.ts | 20 ++-- specs/adr/0001-cloudflare-workflows-scope.md | 52 +++++++-- 15 files changed, 344 insertions(+), 37 deletions(-) create mode 100644 packages/core/src/primitives/exec-in-workspace.test.ts create mode 100644 packages/core/src/primitives/exec-in-workspace.ts diff --git a/REWRITE.md b/REWRITE.md index 0a9d09a..95646c7 100644 --- a/REWRITE.md +++ b/REWRITE.md @@ -231,7 +231,7 @@ An adversarial audit of the repo's 8 issues + 216 PRs (~90 of them bug-fix/incid | ---------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------------------------------- | -------------- | | **One `Model` port over `@effect/ai`** | Two parallel model stacks (Worker `modelGateway` + container `demo-agent/model.ts`) each hand-roll transport routing, structured-output emulation, and retry; provider shape-divergence leaks past transport into every decode site | One `Model` Tag over `@effect/ai` provider Layers, consumed identically by Worker + container; native `completeStructured`; **retire the raw `env.AI.run` binding as a transport**; normalization + `cf-aig-authorization` inside the adapter Layer; Bedrock = a dedicated SigV4 `InvokeModel` adapter | PR5 (+PR1 `completeStructured`, PR3 governor) | 22 | | **Auth as decode-or-fail-loud contracts** | No Principal/Credential capability — every trust-boundary crossing (HMAC, installation token, CF Access) is hand-rolled inline and fails _green_ | Map each crossing onto an existing stage (no fourth pole): Trigger decode owns inbound (HMAC verify, `installation_id`≤0→`None`, `head.sha` a decoded field never ambient `GITHUB_SHA`); Sink typed against the granted permission + single-source manifest parity; a secret-parity+sync Layer (`Config`+`Redacted`+sha256[:8] both sides); one `AccessSession` Tag (outbound, cookie-by-url) + a separate inbound viewer-gate | PR1/PR2/PR3/PR4 | 20 | -| **Byte-boundary capability (`sandbox.capture`/`FileRef`)** | No capability owns "move a container path's bytes across the sandbox boundary"; each transfer re-picks an SDK read method and re-decodes framing, and the container FS is mistaken for durable state | One `sandbox.capture(path): Effect` chokepoint (`stat` size gate → always `readFileStream` → decode-once → `putStream`); `FileRef` schema handle rides checkpoints, never inline bytes; reads are _bounded_ (never `.arrayBuffer()` the whole archive); container FS is step-scoped non-durable — externalize to a `FileRef` and re-hydrate in the consuming step (keep #206's `isWorkingDirFailure` backstop) | PR4 (+PR1 `FileRef`, PR2 workspace) | 15 | +| **Byte-boundary capability (`sandbox.capture`/`FileRef`)** | No capability owns "move a container path's bytes across the sandbox boundary"; each transfer re-picks an SDK read method and re-decodes framing, and the container FS is mistaken for durable state | One `sandbox.capture(path): Effect` chokepoint (`stat` size gate → always `readFileStream` → decode-once → `putStream`); `FileRef` schema handle rides checkpoints, never inline bytes; reads are _bounded_ (never `.arrayBuffer()` the whole archive); container FS is step-scoped non-durable — externalize to a `FileRef` and re-hydrate in the consuming step (keep #206's `isWorkingDirFailure` backstop) — **partly landed**: `workspace()` now returns a rebuildable `spec` and `execInWorkspace` re-hydrates in the consuming step (ADR-0001 rule 3). The `FileRef` capture chokepoint is still open; the landed form re-clones rather than restoring captured bytes | PR4 (+PR1 `FileRef`, PR2 workspace) | 15 | | **Four-way `RunOutcome` verdict + `RunError` partition** | The run→dispatcher boundary reduces a rich result to a pass/fail bit, so infra faults render as code findings and causes get swallowed | `verdict = Passed \| Finding(summaryMd) \| InfraFault(cause) \| Skipped(reason)`, one `Match.exhaustive` → GitHub conclusion; partition `RunError` into infra vs finding supertypes at definition; every `TaggedError` carries `cause` (`Cause.pretty`); a branded absolute `ArtifactRef` is the Sink's only link type | PR1 | 15 | | **Execution env as a declared typed capability** | The sandbox env is an imperative property of one hand-maintained Dockerfile + a coarse `sandboxImage` string; every drift surfaces only at `Deploy` on main | Replace the enum with a Schema `RequiredCapabilities` set the run declares; one `imagePlan` → build args + checkout closure (computed from the pnpm dep graph, #159 gone); a deploy-time preflight that **runtime-probes** each declared capability; a PR-CI `BundleManifest` assertion (one file, shebang, `--help` exit 0); content-digest image keying; DO migrations = a reviewed delta vs a persisted tier ledger; `concurrency: cancel-in-progress` (not `Effect.timeout`) fixes the #77 build-queue hang | PR4 (net-new) | 14 | | **`SandboxPool` admission capability** | The container pool is a finite shared resource nothing models; capacity is scattered across global/per-container/per-acquisition altitudes | One `SandboxPool` Tag is the sole scoped path to a container: D1 counting semaphore (sized to `max_instances`) wrapping the per-key lease, bounded transient-aware retry, one wall-clock deadline from `limits.maxDurationSec`; the dispatcher is a capacity gate, not only a router | PR1 Tag, PR3 consumes | 11 | diff --git a/apps/dispatcher/src/sandbox.ts b/apps/dispatcher/src/sandbox.ts index e6bc6fa..92d6590 100644 --- a/apps/dispatcher/src/sandbox.ts +++ b/apps/dispatcher/src/sandbox.ts @@ -33,14 +33,23 @@ import type { Env } from "./env"; * paid a 10-minute idle tail after its last command. Across a CI-shaped * workload (hundreds of short runs a day) that tail was ~45% of total spend. * - * `isActivityExpired()` never fires while a request is in-flight (a long quiet - * `exec` keeps the container awake regardless), and the window restarts when - * the last request completes — so this only trims the *idle* tail. The primary + * `isActivityExpired()` never fires while a request is in-flight — it renews + * and returns false while `inflightRequests > 0` (`@cloudflare/containers` + * `dist/lib/container.js`), and `containerFetch` increments that before + * proxying, so a long quiet `exec` keeps the container awake regardless. The + * window restarts when the last request completes, so this only trims the + * *idle* tail. The primary * teardown is the explicit `destroy()` at the workflow's finalize boundary * (workflow.ts), which fires on success/failure/defect/interrupt; this idle * window is only the backstop for paths that die before reaching it (Worker * eviction, deploy mid-run). * + * What this window does NOT do is make the filesystem durable. Cloudflare gives + * a container no durable disk, no minimum runtime, and restarts one that runs + * out of memory, so the tree can vanish while the container is BUSY — which no + * idle setting reaches. `execInWorkspace` is what recovers that; this window + * only narrows the idle case below. See ADR-0001 rule 3. + * * Why 10m, not the 2m a cost pass once set: a run's container filesystem is the * SHARED state across its durable steps — `step("checkout")` clones into it, a * later `step("exec")` runs in it. A checkpointed step's RESULT is memoized, but @@ -52,7 +61,7 @@ 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) covers normal inter-step gaps at negligible cost. * `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/README.md b/packages/core/README.md index bed0889..afbbe8f 100644 --- a/packages/core/README.md +++ b/packages/core/README.md @@ -44,6 +44,7 @@ The split keeps the layer boundary visible at the top of every recipe file. The | Primitive | Does | Built from | Used by | | --------------------------------------------------- | -------------------------------------------------------------- | -------------------------- | -------------------------- | | [`workspace`](src/primitives/workspace.ts) | Acquire a container + clone a repo (+ optional cached install) | `sandbox`, `installCached` | every recipe | +| [`execInWorkspace`](src/primitives/exec-in-workspace.ts) | Run a command in a workspace, rebuilding the checkout if the container lost it | `sandbox`, `workspace` | any recipe that execs against a clone | | [`installCached`](src/primitives/install-cached.ts) | R2-backed dependency install, keyed on the lockfile hash | `cache`, `sandbox` | `workspace`, browser-tests | | [`sharded`](src/primitives/sharded.ts) | Count-and-index parallel fan-out | `Effect.forEach` | test-matrix, browser-tests | | [`bootApp`](src/primitives/boot-app.ts) | Start a detached process and wait for its port | `sandbox` | cdp-acceptance | diff --git a/packages/core/src/errors.ts b/packages/core/src/errors.ts index c866699..04e5dc0 100644 --- a/packages/core/src/errors.ts +++ b/packages/core/src/errors.ts @@ -18,6 +18,8 @@ export class CheckoutFailed extends Schema.TaggedError()("Checko export class ExecFailed extends Schema.TaggedError()("ExecFailed", { exitCode: Schema.Number, stderrTail: Schema.String, + /** The command never ran: its working directory was gone. `execInWorkspace` repairs this. */ + workspaceMissing: Schema.optional(Schema.Boolean), }) { // Read by `runEffect` (step.ts) and folded into the own-property message of // the Error thrown at the Workflow boundary — a prototype getter does not diff --git a/packages/core/src/fakes/sandbox-fake.ts b/packages/core/src/fakes/sandbox-fake.ts index e772a15..1b9a026 100644 --- a/packages/core/src/fakes/sandbox-fake.ts +++ b/packages/core/src/fakes/sandbox-fake.ts @@ -102,6 +102,15 @@ export const makeSandboxFake = ( * exercised. Each matched key is counted down independently across calls. */ launchFailures: Record = {}, + /** + * Transient lost-workspace failures, keyed by command-substring → number of + * leading `exec` attempts to reject with `ExecFailed.workspaceMissing`. + * Models the container being replaced after the checkout step completed: + * Cloudflare container disk does not survive a restart, so the tree the run + * checkpointed is simply gone. Counted down per key, like `launchFailures`, + * so a caller's rebuild-and-retry can be exercised. + */ + workspaceLosses: Record = {}, ): { layer: Layer.Layer; state: SandboxFakeState } => { const state: SandboxFakeState = { acquired: [], @@ -114,6 +123,7 @@ export const makeSandboxFake = ( const detachedCommands = new Map(); // Mutable per-key countdown of remaining transient launch failures. const remainingLaunchFailures = new Map(Object.entries(launchFailures)); + const remainingWorkspaceLosses = new Map(Object.entries(workspaceLosses)); const resolve = (command: string): CannedExec | undefined => { const key = Object.keys(program).find((k) => command.includes(k)); @@ -136,6 +146,21 @@ export const makeSandboxFake = ( exec: (opts: ExecOpts) => { const command = normalizeCommand(opts.command); + const lostKey = Object.keys(workspaceLosses).find((k) => command.includes(k)); + if (lostKey !== undefined) { + const remaining = remainingWorkspaceLosses.get(lostKey) ?? 0; + if (remaining > 0) { + remainingWorkspaceLosses.set(lostKey, remaining - 1); + state.execs.push({ command, cwd: opts.cwd, env: opts.env, timeoutSec: opts.timeoutSec }); + return Effect.fail( + new ExecFailed({ + exitCode: -1, + stderrTail: `working directory '${opts.cwd ?? ""}' was missing at exec time`, + workspaceMissing: true, + }), + ); + } + } const entry: SandboxFakeState["execs"][number] = { command, cwd: opts.cwd, diff --git a/packages/core/src/primitives/exec-in-workspace.test.ts b/packages/core/src/primitives/exec-in-workspace.test.ts new file mode 100644 index 0000000..562cc2d --- /dev/null +++ b/packages/core/src/primitives/exec-in-workspace.test.ts @@ -0,0 +1,103 @@ +import { it } from "@effect/vitest"; +import { Effect, Exit, Layer } from "effect"; +import { describe, expect } from "vitest"; +import { ExecFailed } from "../errors"; +import { makeSandboxFake } from "../fakes/sandbox-fake"; +import { CacheFake } from "../fakes/misc-fakes"; +import { IOFake } from "../fakes/io-fake"; +import { execInWorkspace } from "./exec-in-workspace"; +import { workspace, type Workspace } from "./workspace"; + +const SPEC = { repo: "owner/repo", sha: "abc123", install: false } as const; + +const build = (workspaceLosses: Record) => + makeSandboxFake({}, {}, {}, workspaceLosses); + + +describe("execInWorkspace", () => { + it.effect("re-clones and runs again when the container lost the workspace", () => { + const { layer, state } = build({ "cargo test": 1 }); + return Effect.gen(function* () { + const ws = yield* workspace({ ...SPEC }); + expect(state.clones).toHaveLength(1); + + const result = yield* execInWorkspace(ws, { command: "cargo test" }); + + expect(result.exitCode).toBe(0); + // The rebuild is the point: a second clone, into the SAME container. + expect(state.clones).toHaveLength(2); + expect(state.clones[1]).toEqual({ repo: SPEC.repo, sha: SPEC.sha }); + expect(state.acquired).toHaveLength(1); + }).pipe(Effect.provide(Layer.mergeAll(layer, CacheFake, IOFake))); + }); + + it.effect("gives up after one rebuild rather than looping on a dying container", () => { + const { layer, state } = build({ "cargo test": 5 }); + return Effect.gen(function* () { + const ws = yield* workspace({ ...SPEC }); + const exit = yield* Effect.exit(execInWorkspace(ws, { command: "cargo test" })); + + expect(Exit.isFailure(exit)).toBe(true); + expect(state.clones).toHaveLength(2); + }).pipe(Effect.provide(Layer.mergeAll(layer, CacheFake, IOFake))); + }); + + it.effect("does not re-clone for an ordinary command failure", () => { + const { layer, state } = makeSandboxFake({ + "cargo test": { fail: "ExecFailed", exitCode: 101, stderrTail: "tests failed" }, + }); + return Effect.gen(function* () { + const ws = yield* workspace({ ...SPEC }); + const exit = yield* Effect.exit(execInWorkspace(ws, { command: "cargo test" })); + + expect(Exit.isFailure(exit)).toBe(true); + expect(state.clones).toHaveLength(1); + }).pipe(Effect.provide(Layer.mergeAll(layer, CacheFake, IOFake))); + }); + + it.effect("a red suite is a result, never a rebuild", () => { + const { layer, state } = makeSandboxFake({ "cargo test": { exitCode: 1 } }); + return Effect.gen(function* () { + const ws = yield* workspace({ ...SPEC }); + const result = yield* execInWorkspace(ws, { command: "cargo test" }); + + expect(result.exitCode).toBe(1); + expect(state.clones).toHaveLength(1); + }).pipe(Effect.provide(Layer.mergeAll(layer, CacheFake, IOFake))); + }); + + it.effect("re-runs the install when the workspace spec asked for one", () => { + const { layer, state } = build({ "cargo test": 1 }); + return Effect.gen(function* () { + const ws = yield* workspace({ ...SPEC, install: true }); + const clonesAfterCheckout = state.clones.length; + + yield* execInWorkspace(ws, { command: "cargo test" }); + + expect(state.clones).toHaveLength(clonesAfterCheckout + 1); + }).pipe(Effect.provide(Layer.mergeAll(layer, CacheFake, IOFake))); + }); + + it("carries workspaceMissing on the error the runtime raises", () => { + const err = new ExecFailed({ exitCode: -1, stderrTail: "gone", workspaceMissing: true }); + expect(err.workspaceMissing).toBe(true); + expect(new ExecFailed({ exitCode: 1, stderrTail: "x" }).workspaceMissing).toBeUndefined(); + }); + + it.effect("threads the command options through to the sandbox", () => { + const { layer, state } = build({}); + return Effect.gen(function* () { + const ws: Workspace = yield* workspace({ ...SPEC }); + yield* execInWorkspace(ws, { + command: "cargo test", + env: { CI: "1" }, + timeoutSec: 900, + }); + + const exec = state.execs.find((e) => e.command === "cargo test"); + expect(exec?.cwd).toBe(ws.dir); + expect(exec?.env).toEqual({ CI: "1" }); + expect(exec?.timeoutSec).toBe(900); + }).pipe(Effect.provide(Layer.mergeAll(layer, CacheFake, IOFake))); + }); +}); diff --git a/packages/core/src/primitives/exec-in-workspace.ts b/packages/core/src/primitives/exec-in-workspace.ts new file mode 100644 index 0000000..e4677b1 --- /dev/null +++ b/packages/core/src/primitives/exec-in-workspace.ts @@ -0,0 +1,43 @@ +// Primitive: execInWorkspace — run a command against a workspace, rebuilding +// the checkout first if the container lost it. +// +// Container disk does not survive a restart, so a memoized checkout can point +// at a directory that is gone. specs/adr/0001-cloudflare-workflows-scope.md +// rule 3. + +import { Effect } from "effect"; +import type { ExecFailed, ExecTimeout } from "../errors"; +import { sandbox, type ExecResult } from "../services/sandbox"; +import { hydrateWorkspace, type Workspace } from "./workspace"; + +export type ExecInWorkspaceOpts = { + readonly command: string | readonly string[]; + readonly env?: Record; + readonly timeoutSec?: number; + readonly redactValues?: readonly string[]; +}; + +export const execInWorkspace = ( + ws: Workspace, + opts: ExecInWorkspaceOpts, +): Effect.Effect< + ExecResult, + ExecFailed | ExecTimeout | Effect.Effect.Error>, + Effect.Effect.Context> +> => + Effect.gen(function* () { + const run = (dir: string) => + sandbox.exec({ ...opts, cwd: dir, container: ws.container }); + + const first = yield* Effect.either(run(ws.dir)); + if (first._tag === "Right") return first.right; + if (first.left._tag !== "ExecFailed" || first.left.workspaceMissing !== true) { + return yield* Effect.fail(first.left); + } + + yield* Effect.logWarning( + `workspace ${ws.dir} was gone at exec time — rebuilding the checkout and running again`, + ); + const dir = yield* hydrateWorkspace(ws.container, ws.spec); + return yield* run(dir); + }); diff --git a/packages/core/src/primitives/index.ts b/packages/core/src/primitives/index.ts index 57eddba..f0ae1d4 100644 --- a/packages/core/src/primitives/index.ts +++ b/packages/core/src/primitives/index.ts @@ -9,7 +9,8 @@ // // See specs/03-dsl.md § Primitives and ./README.md. -export { workspace, type Workspace } from "./workspace"; +export { workspace, hydrateWorkspace, type Workspace, type WorkspaceSpec } from "./workspace"; +export { execInWorkspace, type ExecInWorkspaceOpts } from "./exec-in-workspace"; export { installCached } from "./install-cached"; export { sharded, type Shard } from "./sharded"; export { fanOut, type FanOutShard } from "./fan-out"; diff --git a/packages/core/src/primitives/workspace.ts b/packages/core/src/primitives/workspace.ts index 2798f4d..2d36007 100644 --- a/packages/core/src/primitives/workspace.ts +++ b/packages/core/src/primitives/workspace.ts @@ -5,6 +5,9 @@ // the container handle and the checkout directory together, so the rest of // the run threads one value instead of two. // +// `spec` rides the checkpoint so `execInWorkspace` can redo the clone when the +// container lost it. specs/adr/0001-cloudflare-workflows-scope.md rule 3. +// // Rides on the `sandbox` capability and the `installCached` primitive. // Layer: 03-dsl § Primitives. @@ -12,11 +15,34 @@ import { Effect } from "effect"; import { sandbox, type Container } from "../services/sandbox"; import { installCached } from "./install-cached"; +/** Everything needed to rebuild the checkout. JSON, so it survives a checkpoint. */ +export type WorkspaceSpec = { + readonly repo: string; + readonly sha: string; + readonly image?: string; + readonly install?: boolean; +}; + export type Workspace = { container: Container; dir: string; + spec: WorkspaceSpec; }; +/** Clone into an acquired container, optionally installing dependencies. */ +export const hydrateWorkspace = (container: Container, spec: WorkspaceSpec) => + Effect.gen(function* () { + const dir = yield* sandbox.git.clone({ + repo: spec.repo, + sha: spec.sha, + container, + }); + if (spec.install) { + yield* installCached({ container, dir }); + } + return dir; + }); + export const workspace = (opts: { repo: string; sha: string; @@ -25,13 +51,12 @@ export const workspace = (opts: { }) => Effect.gen(function* () { const container = yield* sandbox.acquire({ image: opts.image }); - const dir = yield* sandbox.git.clone({ + const spec: WorkspaceSpec = { repo: opts.repo, sha: opts.sha, - container, - }); - if (opts.install) { - yield* installCached({ container, dir }); - } - return { container, dir }; + image: opts.image, + install: opts.install, + }; + const dir = yield* hydrateWorkspace(container, spec); + return { container, dir, spec }; }); diff --git a/packages/runtime-cf/src/sandbox-cf.test.ts b/packages/runtime-cf/src/sandbox-cf.test.ts index b60165b..8506e18 100644 --- a/packages/runtime-cf/src/sandbox-cf.test.ts +++ b/packages/runtime-cf/src/sandbox-cf.test.ts @@ -429,6 +429,44 @@ describe("makeSandboxCloudflareLive — exec result folding (D)", () => { }), ); + it.effect("a missing working dir sets workspaceMissing so the caller can rebuild", () => + Effect.gen(function* () { + currentBox = makeFakeBox({ proc: null }); + currentBox.exec = vi.fn(async () => ({ + exitCode: 1, + duration: 0, + stdout: "", + stderr: "Failed to change directory to '/workspace/repo'", + })); + const exit = yield* Effect.flatMap(SandboxTag, (s) => + s.exec({ command: "cargo test", cwd: "/workspace/repo", env: {} }), + ).pipe(Effect.provide(execLayer()), Effect.exit); + const err = failureOf<{ _tag: string; workspaceMissing?: boolean }>(exit); + expect(err?._tag).toBe("ExecFailed"); + expect(err?.workspaceMissing).toBe(true); + }), + ); + + it.effect("a repo path containing 'timeout' is still classified as a missing workspace", () => + Effect.gen(function* () { + // The marked throw embeds `cwd`, so classifying by message first would + // call this an ExecTimeout and silently disable the rebuild. + 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; workspaceMissing?: boolean }>(exit); + expect(err?._tag).toBe("ExecFailed"); + expect(err?.workspaceMissing).toBe(true); + }), + ); + 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..f59ccde 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 + * can set `ExecFailed.workspaceMissing` without re-parsing the message it just + * wrote. A symbol, so it cannot collide with an SDK error's own properties. + */ +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,16 @@ 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). + // Before the message regex: the marked throw embeds `cwd` and 200 + // chars of stderr, so a repo path containing "timeout" would classify + // as ExecTimeout and silently disable the rebuild. + if (isWorkspaceMissingThrow(cause)) { + return new ExecFailed({ + exitCode: -1, + stderrTail: diagnosticTail(cause, redactValues), + workspaceMissing: true, + }); + } 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 7e997ea..f16e9bc 100644 --- a/runs/README.md +++ b/runs/README.md @@ -263,3 +263,6 @@ Staged mode is webhook-only: a dispatch that passes `command` skips the config read and stays single-exec. Stages run sequentially inside one workflow instance posting one check-run — they are ordered dependents of one checkout, not the parallel independent gates `matrix-fanout` + labelled `check` serve. +The checkout each stage depends on is rebuilt if the container lost it between +stages: stages exec through `execInWorkspace`, because container disk does not +survive a restart (specs/adr/0001-cloudflare-workflows-scope.md rule 3). diff --git a/runs/offload-test.test.ts b/runs/offload-test.test.ts index 00f3f21..3cc7614 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", "CheckoutFailed"]); }).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", "CheckoutFailed"]); }).pipe(Effect.provide(layer)); }, ); @@ -717,7 +717,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", "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. diff --git a/runs/offload-test.ts b/runs/offload-test.ts index 21a214f..6cec94f 100644 --- a/runs/offload-test.ts +++ b/runs/offload-test.ts @@ -126,7 +126,11 @@ import { StepFailed, step, } from "@fractalboxdev/flare-dispatch-core"; -import { loadSecrets, workspace } from "@fractalboxdev/flare-dispatch-core/primitives"; +import { + execInWorkspace, + loadSecrets, + workspace, +} from "@fractalboxdev/flare-dispatch-core/primitives"; /** Input contract — specs/02-runs.md § 1. */ const OffloadTestInput = Schema.Struct({ @@ -215,7 +219,7 @@ const STEP_TIMEOUT_HEADROOM_SEC = 120; const PLATFORM_RETRIES = 3; -const RETRY_ON = ["ExecFailed"] as const; +const RETRY_ON = ["ExecFailed", "CheckoutFailed"] as const; const stepTimeoutFor = (execTimeoutSec: number): number => execTimeoutSec + STEP_TIMEOUT_HEADROOM_SEC; @@ -528,7 +532,7 @@ export const offloadTest = defineRun({ // checkout — acquire a container (honouring the `image` override), clone // the repo at the requested SHA, and optionally run the R2-cached // dependency install. One primitive, same opening move as cdp-acceptance. - const { container, dir } = yield* step("checkout", () => + const ws = yield* step("checkout", () => workspace({ repo: input.repo, sha: input.sha, @@ -630,9 +634,7 @@ export const offloadTest = defineRun({ step( `exec-${stage.label}`, () => - sandbox.exec({ - cwd: dir, - container, + execInWorkspace(ws, { command: stage.command, env: { ...secretEnv, ...input.env }, timeoutSec: stageTimeoutSec, @@ -695,7 +697,7 @@ export const offloadTest = defineRun({ () => sandbox .exec({ - container, + container: ws.container, command: `printf '%s\\n' '${markerLine}'`, timeoutSec: 30, }) @@ -823,9 +825,7 @@ export const offloadTest = defineRun({ const result = yield* step( "exec", () => - sandbox.exec({ - cwd: dir, - container, + execInWorkspace(ws, { command: soleCommand, // Per-dispatch `env` wins over a same-named config-store secret — // the more specific source overrides the global one. diff --git a/specs/adr/0001-cloudflare-workflows-scope.md b/specs/adr/0001-cloudflare-workflows-scope.md index f29125b..ff9c614 100644 --- a/specs/adr/0001-cloudflare-workflows-scope.md +++ b/specs/adr/0001-cloudflare-workflows-scope.md @@ -59,13 +59,27 @@ Three repo facts the decision has to account for: we name instances, not of anything inside the run. - **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. + eviction mid-agent does not re-spawn the agent and double-spend the model budget. +- **The container filesystem is not durable, and no setting makes it durable.** + Cloudflare states it 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." The platform also guarantees no minimum runtime, restarts an instance + that exhausts its memory, and can terminate one when its host restarts. A checkout is + therefore an ephemeral SIDE EFFECT of a step whose RESULT is memoized: replay or a + container replacement leaves a valid checkpoint pointing at a directory that no longer + exists, and the step that would restore it has already "succeeded" so it never re-runs. + The container's `sleepAfter = "10m"` (`apps/dispatcher/src/sandbox.ts:60`) narrows the + window by keeping an idle container warm between steps; it does not close it, and it + does nothing at all when the container dies mid-exec. + + This was observed, not deduced. An `exec` step failed with `HTTP error! status: 500` + after 34m51s; its replay reached a fresh, empty container and failed with + `working directory '' was missing at exec time`, as did every attempt after it. + Rule 3 below is the response. ## 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 @@ -79,18 +93,38 @@ existing PR/comment before creating it) or structured so replay observes rather repeats (`runDetached` then `waitForExit`, never one indivisible long `exec`). Step results are capped at 1 MiB: large outputs go to R2 and the step returns a key. -**3. Hibernation is reserved for bounded human decisions with a named decider and a +**3. A command whose inputs are fully determined by the workspace spec runs through +`execInWorkspace`, never a bare `sandbox.exec`.** The workspace has to be rebuildable +from data that rides the checkpoint, because the directory itself does not survive one. +`workspace()` returns a `spec` for exactly this, and `execInWorkspace` rebuilds from it +inside the step that finds the tree gone, then runs the command once more. A step retry +alone cannot fix this — it re-runs one callback, and the callback is not the one that +clones. + +The qualifier is load-bearing. A re-clone restores the tree the SPEC describes, which is +the right tree only for a command that reads what was cloned — a test suite, a lint, a +build. A step that reads a tree an earlier step MUTATED must not use it: re-cloning hands +`self-heal-pr`'s verify step a clean checkout, so it passes on unmodified code, and hands +a writeback step nothing to stage. That converts an infra failure into a wrong green, +which is worse than the red it replaces. Those steps need captured bytes restored, not a +fresh clone — the `FileRef` capture chokepoint in REWRITE.md — and until that lands they +carry the exposure knowingly. + +`isWorkingDirFailure` (`packages/runtime-cf/src/sandbox-cf.ts`) stays as the backstop for +both cases: a lost workspace never renders as a phantom lint or test verdict. + +**4. Hibernation is reserved for bounded human decisions with a named decider and a declared timeout.** `step.waitForEvent` and long sleeps are legitimate when a specific person owes a specific answer inside a stated window — release approval is the shape. They are not a mechanism for waiting on the world in general. -**4. Long-lived entity lifecycles keep their state in the system of record, and get a +**5. Long-lived entity lifecycles keep their state in the system of record, and get a fresh instance per event.** When the thing being tracked is an external entity that humans also act on — a GitHub issue, a PR, a deployment — the authoritative state is 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. -Corollary to rules 3 and 4: wall-clock bounds must be written explicitly (`timeoutSec` +Corollary to rules 4 and 5: 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, until that changes. @@ -112,7 +146,7 @@ the code the durable design was supposed to remove. **Re-entry restarts the work anyway.** Issue retriage re-runs the whole reproduce→diagnose→verify→fix pipeline when a comment adds new information. Inside a hibernating instance that is either a loop back to the top — an explicit state machine -again — or killing the instance and starting another, which is rule 4 with extra +again — or killing the instance and starting another, which is rule 5 with extra bookkeeping. **External state must be authoritative because humans edit it.** Maintainers read and @@ -145,7 +179,7 @@ anyway. - Observability loses "one instance = one entity" — the dashboard groups executions by entity instead, and an operator tracing an issue reads its thread plus D1 rows, not one instance timeline. -- `release-notes` stays exactly as it is, and is the reference implementation for rule 3. +- `release-notes` stays exactly as it is, and is the reference implementation for rule 4. 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.