fix(runner): stop paying two container-start retry budgets, degrade properly (DEV-2857)

runner/packages/runtime/src/container.ts

@@ -262,6 +262,15 @@ function sessionStartMessage(
   // below on purpose — this one HAS an envelope, so it would otherwise fall
   // through to the generic wrapper at the bottom.
   if (failure.code === "at_capacity") return failure.message;
+  // DEV-2857 / Sentry DEMOS-1Z & DEMOS-20. Same reasoning as `at_capacity`
+  // immediately above: a 503 WITH an envelope and this code is the Worker's
+  // own degrade branch for a container that never finished booting, phrased
+  // for the person reading it. Without this tier the envelope-less-503
+  // fallthrough at the bottom of this function would wrap it as
+  // "session start failed (503): …", which trips the App.tsx heuristic and
+  // tells the visitor to install Docker — the exact trap this function exists
+  // to avoid for every other refusal.
+  if (failure.code === "container_starting") return failure.message;
   // The interception tier (Sentry DEMOS-9, UNREACHED_STATUS above). Placed before
   // TIMEOUT_STATUSES because 504 is a member of that set and this more specific gate
   // must win. `edge.headersReadable` is required, not just `!edge.ray`: without it a

runner/pipeline/fixtures/sentry-cloudflare-stub.mjs

@@ -0,0 +1,48 @@
+// Stands in for `@sentry/cloudflare` when the router runs under plain Node (see
+// worker-hooks.mjs). The real client is inert in every other route spec
+// already — `makeEnv()`'s `ERROR_REPORTING_DSN: ""` and the absence of
+// `SENTRY_ENVIRONMENT` make `apiSentryDsn()` return `undefined`, so
+// `sentryOptions()` never wires up a live DSN — but nothing has ever observed
+// what index.ts PASSES to `Sentry.captureException`, because there was no
+// stub to record it. This one does, additively: every symbol below is a
+// passthrough or a no-op recorder, so a spec that asserts nothing about
+// Sentry (mcp-routes, token-routes, demo-routes-version, snapshot-build) stays
+// exactly as green as it was before this file existed.
+//
+// Confirmed against `grep -rn "Sentry\." workers/api/src` (DEV-2857 planning):
+// the full symbol surface used across that tree is `withSentry`,
+// `instrumentDurableObjectWithSentry`, `captureException`, and
+// `captureMessage`. Nothing else is exported here on purpose — an unstubbed
+// symbol should fail loudly (ReferenceError/TypeError on import) rather than
+// silently resolve to `undefined`.
+
+/** Every `captureException`/`captureMessage` call this process has recorded,
+ *  in order. `node --test` runs each spec file in its own process, so this
+ *  never crosses files — no reset hook needed between tests in the same file
+ *  either, since each test constructs its own fake sandbox and asserts on the
+ *  tail of this array or filters by a fingerprint/tag it just triggered. */
+export const captures = [];
+
+/** `withSentry(options, handlers) => handlers`. The real function wraps
+ *  `fetch` to install request-scoped Sentry context; nothing under test reads
+ *  that context, so the handlers object passes through unwrapped. */
+export function withSentry(_options, handlers) {
+  return handlers;
+}
+
+/** `instrumentDurableObjectWithSentry(options, cls) => cls`. Same reasoning:
+ *  index.ts exports `Sandbox`/`BuilderSandbox`/`BuildJob` through this at
+ *  module scope, but no route spec drives a Durable Object directly (the
+ *  sandbox stub throws before one would be reached), so the class passes
+ *  through unwrapped. */
+export function instrumentDurableObjectWithSentry(_options, cls) {
+  return cls;
+}
+
+export function captureException(error, context) {
+  captures.push({ kind: "exception", error, context });
+}
+
+export function captureMessage(message, context) {
+  captures.push({ kind: "message", message, context });
+}

runner/pipeline/fixtures/worker-hooks.mjs

@@ -15,13 +15,25 @@
 // - `@cloudflare/sandbox` imports the `cloudflare:` URL scheme at load time,
 //   which only exists inside workerd. The routes under test never reach a
 //   sandbox, so a structural stub stands in for the package.
+//
+// - `@sentry/cloudflare` likewise expects a Workers runtime. Nothing under
+//   test needs live reporting (see sentry-cloudflare-stub.mjs for why), but a
+//   spec that wants to assert on a `Sentry.captureException` call needs
+//   somewhere to observe it — a plain "does not crash" stub would leave that
+//   untestable. Additive only: every symbol used in workers/api/src passes
+//   through or no-ops, so specs that assert nothing about Sentry are
+//   unaffected.
 
 const SANDBOX_STUB = new URL("./cloudflare-sandbox-stub.mjs", import.meta.url).href;
+const SENTRY_STUB = new URL("./sentry-cloudflare-stub.mjs", import.meta.url).href;
 
 export async function resolve(specifier, context, nextResolve) {
   if (specifier === "@cloudflare/sandbox") {
     return { url: SANDBOX_STUB, shortCircuit: true };
   }
+  if (specifier === "@sentry/cloudflare") {
+    return { url: SENTRY_STUB, shortCircuit: true };
+  }
   if (
     specifier.startsWith(".")
     && specifier.endsWith(".js")

runner/pipeline/session-create-container-starting.test.mjs

@@ -0,0 +1,191 @@
+// POST /api/session route test for DEV-2857 (Sentry DEMOS-1Z / DEMOS-20).
+//
+// The premise the original ticket got wrong: `@cloudflare/sandbox@0.12.3`
+// ALREADY retries a 503 "Container is starting" — `sandbox.mkdir`/
+// `.writeFile` route through `BaseTransport.fetch` -> `fetchWithResponseRetry`
+// with `shouldRetry: r => r.status === 503`, budget ~150s, ~7 attempts. The
+// `SandboxError` reaching `writeFiles` in workers/api/src/index.ts is the
+// EXHAUSTED END of that loop, not a first attempt — so this fix adds zero
+// retries anywhere.
+//
+// The real bug was `catch { /* dir may exist */ }` after `mkdir` swallowing
+// that transient AFTER a full ~140s SDK budget had already burned, so the
+// first `writeFile` below opened a FRESH one — Sentry DEMOS-20 measured the
+// sum: `sessionElapsedMs: 283943`, 4m44s for one `POST /api/session`. T2 below
+// is the double-budget proof: with the fix, `mkdir` is called once and
+// `writeFile`/`startProcess` are never reached, because the create handler's
+// catch degrades to a 503 instead of the loop reopening a second attempt.
+//
+// This is the first route test for `POST /api/session`. It reaches the sandbox
+// with no new plumbing: `makeEnv()` already supplies `Sandbox: {}` and fake
+// KV/D1/R2, the budget gate allows on a throwing ledger read, and
+// `FRAMEWORK_DEV`/`BUILD_CONFIG` carry the Tier-2 "react-js" framework (NOT
+// bare "react", which is Tier-1 only and absent from FRAMEWORK_DEV — verified
+// against workers/api/src/frameworks.generated.ts during planning).
+//
+// Run: node --experimental-strip-types --test pipeline/session-create-container-starting.test.mjs
+
+import test from "node:test";
+import assert from "node:assert/strict";
+import { register } from "node:module";
+
+import { ctx, makeEnv } from "./fixtures/worker-harness.mjs";
+import { setSandboxFactory } from "./fixtures/cloudflare-sandbox-stub.mjs";
+
+register("./fixtures/worker-hooks.mjs", import.meta.url);
+
+const { default: worker } = await import("../workers/api/src/index.ts");
+const { captures } = await import("./fixtures/sentry-cloudflare-stub.mjs");
+const {
+  CONTAINER_STARTING_CODE,
+  containerStartingMessage,
+} = await import("../workers/api/src/session-lifecycle.ts");
+
+/** The SDK's own 503 body, verbatim — see session-lifecycle.ts. */
+const CONTAINER_STARTING = "Container is starting. Please retry in a moment.";
+
+// A file map with one nested directory, so `writeFiles` actually calls
+// `sandbox.mkdir` at least once (a flat file map never does: its "dir" is
+// CONTAINER_ROOT itself, which writeFiles skips).
+const FILES = {
+  "package.json": JSON.stringify({ name: "demo" }),
+  "src/App.jsx": "export default function App() { return null; }",
+};
+
+const sessionRequest = (body) =>
+  new Request("https://demos.handsontable.com/api/session", {
+    method: "POST",
+    headers: { "Content-Type": "application/json" },
+    body: JSON.stringify(body),
+  });
+
+/**
+ * A fake sandbox recording call counts, with `mkdir` scripted to throw.
+ *
+ * `dirReallyExists` models the physical consequence of the two failure
+ * families the narrowed catch has to tell apart: an EEXIST means the
+ * directory genuinely is there already (a prior run, a shared parent dir), so
+ * a write into it still succeeds — `writeFile` below only fails when the
+ * directory it targets was never actually created AND the sandbox was
+ * scripted with an error. This is what makes T4 (EACCES) end in a raw 500
+ * without this file's fake claiming any first-hand knowledge of the SDK's own
+ * retry mechanics: a permission error really does leave the directory
+ * missing, and the subsequent `writeFile` genuinely fails against it, exactly
+ * as it would against the real sandbox.
+ */
+function fakeSandbox({ mkdirError, dirReallyExists = false } = {}) {
+  const calls = { mkdir: 0, writeFile: 0, startProcess: 0, exposePort: 0 };
+  const createdDirs = new Set();
+  return {
+    calls,
+    async mkdir(dir) {
+      calls.mkdir += 1;
+      if (mkdirError) {
+        if (dirReallyExists) createdDirs.add(dir);
+        throw mkdirError;
+      }
+      createdDirs.add(dir);
+    },
+    async writeFile(path) {
+      calls.writeFile += 1;
+      const dir = path.slice(0, path.lastIndexOf("/"));
+      if (mkdirError && dir && dir !== "/app" && !createdDirs.has(dir)) throw mkdirError;
+    },
+    deleteFile: async () => {},
+    exec: async () => ({ success: true, stdout: "", stderr: "" }),
+    async startProcess() {
+      calls.startProcess += 1;
+    },
+    async exposePort() {
+      calls.exposePort += 1;
+      return { url: "https://preview.test/session" };
+    },
+    destroy: async () => {},
+  };
+}
+
+test("T1: a container that never finished starting degrades to a pinned 503, not a raw 500", async () => {
+  const { env } = makeEnv();
+  const sandbox = fakeSandbox({ mkdirError: new Error(CONTAINER_STARTING) });
+  setSandboxFactory(() => sandbox);
+
+  const res = await worker.fetch(sessionRequest({ framework: "react-js", files: FILES }), env, ctx);
+  const body = await res.json();
+
+  assert.equal(res.status, 503, "reverting EDIT 3 falls back to a raw 500 with the platform's own words");
+  assert.equal(body.error, CONTAINER_STARTING_CODE);
+  assert.equal(body.message, containerStartingMessage);
+});
+
+test("T2: the mkdir catch does not reopen a second SDK retry budget", async () => {
+  const { env } = makeEnv();
+  const sandbox = fakeSandbox({ mkdirError: new Error(CONTAINER_STARTING) });
+  setSandboxFactory(() => sandbox);
+
+  await worker.fetch(sessionRequest({ framework: "react-js", files: FILES }), env, ctx);
+
+  // THE double-budget proof, and the substitute for a "retry count" assertion
+  // — the retry itself is the SDK's, not ours (see the file header). Reverting
+  // EDIT 2 alone (the mkdir catch's rethrow) makes writeFiles swallow the
+  // transient and call writeFile below, which is a second SDK RPC and a second
+  // ~140s budget — this assertion catches that regression even though EDIT 3
+  // alone would still turn the eventual throw into a 503.
+  assert.equal(sandbox.calls.mkdir, 1, "mkdir must not be retried by us");
+  assert.equal(sandbox.calls.writeFile, 0, "writeFile must never run after mkdir's transient");
+  assert.equal(sandbox.calls.startProcess, 0, "startProcess must never run after mkdir's transient");
+});
+
+test("T3 (regression guard, passes before and after): EEXIST is still swallowed by the narrowed catch", async () => {
+  const { env } = makeEnv();
+  const sandbox = fakeSandbox({
+    mkdirError: new Error("EEXIST: file already exists, mkdir '/app/src'"),
+    dirReallyExists: true,
+  });
+  setSandboxFactory(() => sandbox);
+
+  const res = await worker.fetch(sessionRequest({ framework: "react-js", files: FILES }), env, ctx);
+  const body = await res.json();
+
+  assert.equal(res.status, 200, "the original swallow's own case must still work");
+  assert.ok(body.sessionId, "expected a sessionId in a successful create");
+  assert.ok(body.previewUrl, "expected a previewUrl in a successful create");
+  assert.equal(sandbox.calls.writeFile, Object.keys(FILES).length, "the create must have proceeded");
+});
+
+test("T4 (negative guard, passes before and after): a genuine EACCES is not degraded", async () => {
+  const { env } = makeEnv();
+  const sandbox = fakeSandbox({ mkdirError: new Error("EACCES: permission denied, mkdir '/app/src'") });
+  setSandboxFactory(() => sandbox);
+
+  const before = captures.length;
+  const res = await worker.fetch(sessionRequest({ framework: "react-js", files: FILES }), env, ctx);
+  const body = await res.json();
+
+  assert.equal(res.status, 500, "a genuine failure must not be degraded to a 503");
+  assert.match(body.error, /EACCES/);
+
+  const captured = captures.slice(before);
+  assert.equal(captured.length, 1, "the outer catch-all still reports it");
+  assert.equal(captured[0].kind, "exception");
+  assert.equal(
+    captured[0].context?.level,
+    undefined,
+    "no level override — this must file at Sentry's default (error), unlike the container-starting degrade",
+  );
+});
+
+test("T5: the container-starting capture is a warning under its own fingerprint", async () => {
+  const { env } = makeEnv();
+  const sandbox = fakeSandbox({ mkdirError: new Error(CONTAINER_STARTING) });
+  setSandboxFactory(() => sandbox);
+
+  const before = captures.length;
+  await worker.fetch(sessionRequest({ framework: "react-js", files: FILES }), env, ctx);
+  const captured = captures.slice(before);
+
+  assert.equal(captured.length, 1, "exactly one Sentry event for the degrade");
+  assert.equal(captured[0].kind, "exception");
+  assert.equal(captured[0].context?.level, "warning");
+  assert.deepEqual(captured[0].context?.fingerprint, ["tier2-session-container-starting"]);
+  assert.deepEqual(captured[0].context?.tags, { context: "tier2-session-start" });
+});

runner/pipeline/session-lifecycle.test.mjs

@@ -3,8 +3,11 @@ import assert from "node:assert/strict";
 import {
   AT_CAPACITY_CODE,
   atCapacityMessage,
+  CONTAINER_STARTING_CODE,
+  containerStartingMessage,
   destroyConfirmed,
   isAtCapacityFailure,
+  isContainerStartingFailure,
   isExpectedTeardownFailure,
   TOMBSTONE_ATTEMPTED,
   TOMBSTONE_DESTROYED,
@@ -169,3 +172,87 @@ test("the envelope code is stable", () => {
   // exact code to pass the sentence through unwrapped.
   assert.equal(AT_CAPACITY_CODE, "at_capacity");
 });
+
+// ---- the container-starting classifier (DEV-2857) -------------------------
+//
+// The withdrawn premise: `@cloudflare/sandbox@0.12.3` already retries a 503
+// "Container is starting" for ~150s/~7 attempts (BaseTransport.fetch ->
+// fetchWithResponseRetry, shouldRetry: r => r.status === 503) BEFORE this
+// message ever reaches workers/api/src/index.ts. So `isContainerStartingFailure`
+// recognises the EXHAUSTED end of that loop; it does not gate a retry of ours
+// — there is no retry of ours, by design (see the ticket's "Attempts added: 0").
+
+/** The SDK's own 503 body, verbatim — built in the DO's `containerFetch` catch,
+ *  never by any package in this repo. */
+const CONTAINER_STARTING = "Container is starting. Please retry in a moment.";
+
+test("the exact SDK string is recognised", () => {
+  assert.equal(isContainerStartingFailure(new Error(CONTAINER_STARTING)), true);
+});
+
+test("it sees through a cause chain", () => {
+  const wrapped = new Error("mkdir failed", { cause: new Error(CONTAINER_STARTING) });
+  assert.equal(isContainerStartingFailure(wrapped), true);
+  assert.equal(isContainerStartingFailure(new Error("outer", { cause: wrapped })), true);
+});
+
+test("a non-Error throw carrying the same words is not recognised", () => {
+  // Same reasoning `messageMatches` documents at the top of this file: workerd
+  // and the containers SDK both throw real Errors, so a bare string is
+  // somebody else's, and unrecognised is the safe direction for every caller.
+  assert.equal(isContainerStartingFailure(CONTAINER_STARTING), false);
+});
+
+test("the not-running teardown fault is a different fault and must not match", () => {
+  // THE narrowness this predicate exists to prove. "The container is not
+  // running" is NOT_RUNNING_PATTERN's teardown-only wording (a container that
+  // WAS running and stopped) — a different fault from one that never finished
+  // starting, and conflating them would let a slow-boot visitor land on the
+  // teardown path's assumptions.
+  assert.equal(
+    isContainerStartingFailure(new Error("The container is not running, consider calling start()")),
+    false,
+  );
+});
+
+test("isAtCapacityFailure stays false for the container-starting string", () => {
+  // The other half of the same narrowness requirement: a visitor whose sandbox
+  // is merely slow to boot must never be told "we are at capacity".
+  assert.equal(isAtCapacityFailure(new Error(CONTAINER_STARTING)), false);
+});
+
+test("isExpectedTeardownFailure is unaffected by the container-starting classifier", () => {
+  // This predicate is create-only (DEV-2857); the teardown classifier keeps
+  // recognising exactly the four messages it always has.
+  assert.equal(isExpectedTeardownFailure(new Error(CONTAINER_STARTING)), false);
+  assert.equal(isExpectedTeardownFailure(new Error(CAPACITY)), true);
+  assert.equal(isExpectedTeardownFailure(new Error(UNREACHABLE)), true);
+  assert.equal(isExpectedTeardownFailure(new Error(NOT_RUNNING)), true);
+  assert.equal(isExpectedTeardownFailure(new Error(NO_INSTANCE)), true);
+});
+
+test("a reworded platform string degrades to today's behaviour, not to silence", () => {
+  // The documented degrade direction, same as `isAtCapacityFailure` and
+  // `isExpectedTeardownFailure` above: if Cloudflare rewords the 503 body, this
+  // predicate stops matching and the create falls back to today's
+  // report-and-500 — noisy, never silent.
+  assert.equal(isContainerStartingFailure(new Error("The container is booting up, please wait")), false);
+});
+
+test("the container-starting envelope code is stable", () => {
+  // `sessionStartMessage` in packages/runtime/src/container.ts matches this
+  // exact code to pass the sentence below through unwrapped.
+  assert.equal(CONTAINER_STARTING_CODE, "container_starting");
+});
+
+test("the container-starting sentence never trips the App.tsx connectivity heuristic", () => {
+  // Same two constraints `atCapacityMessage` is pinned on above.
+  assert.doesNotMatch(containerStartingMessage, /session start failed/i);
+  assert.doesNotMatch(containerStartingMessage, /fetch/i);
+  assert.doesNotMatch(containerStartingMessage, /failed to fetch|networkerror|load failed/i);
+  // And it must not leak an internal knob — same shape as the max_instances
+  // guard on atCapacityMessage, even though this sentence has no analogous
+  // platform-config term to accidentally repeat.
+  assert.doesNotMatch(containerStartingMessage, /max_instances|container instances/i);
+  assert.ok(containerStartingMessage.length < 200, "a sentence, not a log excerpt");
+});