From 27cbc063638230ce7dfa1376d46f7101e2176578 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Artur=20M=C4=99dryga=C5=82?= Date: Mon, 17 Aug 2026 15:53:06 +0200 Subject: [PATCH 1/2] fix(runner): release a container without needing one (DEV-2556) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sentry DEMOS-1 caught two `DELETE /api/session/:id` calls, 328 ms apart, throwing "Maximum number of running container instances exceeded" out of `sandbox.destroy()` and into a 500. The ticket's premise that each failed DELETE leaks a session is backwards: an instance that cannot be allocated is not occupying one of the five slots, so a refused teardown is a symptom of a full pool and never a contributor to one. Nothing leaked — and nobody read the 500 either, since `deleteSession()` is a fire-and-forget `keepalive` fetch from `pagehide` that discards the response. What was actually wrong is an asymmetry: that route held the only `destroy()` in index.ts that was not best-effort, while both of its siblings already swallow. So a platform hiccup on the one path whose job is release became a thrown request. - Recognised platform refusals ("maximum number of running container instances exceeded", "container service is unreachable", "container is not running") become a log line and a 204. Anything unrecognised still throws to the outer catch and keeps today's status and today's Sentry event, so a real teardown regression stays visible. The predicate is a message match against strings workerd raises, so a Cloudflare rewording degrades to today's behaviour — noisy, never silent. - The tombstone gains a second state. It already existed before the sandbox RPC, but written by that same request, so the obvious "skip when tombstoned" rule would skip a first DELETE's own teardown and would never retry one whose destroy had just failed. `"1"` (unchanged on the wire, so a rolling deploy and legacy markers keep working) now means "teardown attempted"; `"destroyed"` is written only after `destroy()` resolves, and only that value lets a repeat DELETE answer from KV instead of booting a container in order to destroy it. `closedWhileCreating()` deliberately never skips — its premise is that the create built a second container after the DELETE — but it does record the confirmation, which is what makes the client's follow-up DELETE on the create/delete race free. - The create half: a capacity failure on `POST /api/session` handed the visitor the platform's own words ("try configuring a higher value for max_instances") inside a 500. It is now a 503 `{ error: "at_capacity" }` with a sentence written for the person reading it, passed through unwrapped by `sessionStartMessage` like the budget refusals — and phrased to miss the App.tsx connectivity heuristic that would otherwise tell a production visitor to install Docker (DEMOS-9). Deliberately not here: `max_instances` stays at 5 (a spend decision, and two capacity events in 90 days is not yet evidence for it); no absolute session lifetime for a visible-but-abandoned tab; no retry policy for a refused anonymous visitor; and no timed retry of a failed destroy, which would re-create the dependency on the resource being released — the tombstone plus `sleepAfter = 5m` is already the backstop. Both decisions live in a new import-free `session-lifecycle.ts` (erasable syntax only) because index.ts is a Worker entrypoint that cannot be loaded under `--experimental-strip-types`; `pipeline/session-lifecycle.test.mjs` is therefore the only seam either is testable through. It pins the regression the naive fix would have introduced — an attempted marker must not skip the RPC — and the degrade direction of both classifiers. Co-Authored-By: Claude Opus 5 --- runner/packages/runtime/src/container.ts | 7 + runner/pipeline/session-lifecycle.test.mjs | 149 +++++++++++++++++ .../pipeline/session-start-failure.test.mjs | 17 ++ runner/workers/api/src/index.ts | 113 ++++++++++++- runner/workers/api/src/session-lifecycle.ts | 154 ++++++++++++++++++ 5 files changed, 431 insertions(+), 9 deletions(-) create mode 100644 runner/pipeline/session-lifecycle.test.mjs create mode 100644 runner/workers/api/src/session-lifecycle.ts diff --git a/runner/packages/runtime/src/container.ts b/runner/packages/runtime/src/container.ts index 96dbbbc3..16613775 100644 --- a/runner/packages/runtime/src/container.ts +++ b/runner/packages/runtime/src/container.ts @@ -205,6 +205,13 @@ function sessionStartMessage( // A guardrail refusal already reads as a sentence aimed at the user; wrapping it in // "session start failed (503): …" would bury it (and trip the heuristic above). if (failure.code?.startsWith("budget_")) return failure.message; + // Same reasoning, different refusal: `at_capacity` (DEV-2556) is the Worker + // saying every container slot is taken, in a sentence written for the person + // reading it. Wrapping it in "session start failed (503): …" would both bury + // it and hand it to the App.tsx heuristic. Before the envelope-less 503 tier + // 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; // Nothing answered in time, and no envelope means the silence came from above our // Worker. Nothing is wrong with the demo or with the visitor's connection, so say so // — "Restart preview" is the error card's own button (packages/editor-shell/src/ diff --git a/runner/pipeline/session-lifecycle.test.mjs b/runner/pipeline/session-lifecycle.test.mjs new file mode 100644 index 00000000..d71f47dd --- /dev/null +++ b/runner/pipeline/session-lifecycle.test.mjs @@ -0,0 +1,149 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import { + AT_CAPACITY_CODE, + atCapacityMessage, + destroyConfirmed, + isAtCapacityFailure, + isExpectedTeardownFailure, + TOMBSTONE_ATTEMPTED, + TOMBSTONE_DESTROYED, + TOMBSTONE_TTL_SECONDS, +} from "../workers/api/src/session-lifecycle.ts"; + +// DEV-2556. Sentry DEMOS-1 caught two `DELETE /api/session/:id` calls, 328 ms +// apart, failing with "Maximum number of running container instances exceeded". +// A teardown cannot be a cause of a full pool — an instance that cannot be +// allocated is not occupying a slot — so those 500s bought a Sentry event and +// nothing else: `deleteSession()` in packages/runtime/src/container.ts is a +// fire-and-forget `keepalive` fetch that discards the response entirely. +// +// Both decisions the fix rests on live in `session-lifecycle.ts` because +// `index.ts` is a Worker entrypoint and cannot be imported under +// `--experimental-strip-types` (the constraint `sentry-gating.test.mjs` +// documents). This file is therefore the only place either is pinned. + +/** The DEMOS-1 event message, verbatim. */ +const CAPACITY = + "Maximum number of running container instances exceeded. Try again later, or try configuring a higher value for max_instances"; +/** The 2026-08-07 event that Sentry grouped alongside it (same culprit). */ +const UNREACHABLE = "The container service is unreachable, try again later"; +const NOT_RUNNING = "The container is not running, consider calling start()"; + +// ---- the tombstone state machine ------------------------------------------ + +test("a marker that was never written means the sandbox RPC still has to run", () => { + assert.equal(destroyConfirmed(null), false); + assert.equal(destroyConfirmed(undefined), false); +}); + +test("an ATTEMPTED marker does NOT skip the destroy", () => { + // THE REGRESSION THIS FILE EXISTS FOR. The obvious form of this fix — "skip + // the sandbox RPC whenever the session is tombstoned" — is wrong twice over. + // The DELETE handler writes the marker itself, immediately before its own + // destroy, so a *first* DELETE would skip the very teardown it exists to do; + // and a DELETE whose destroy just failed would never be retried by the next + // one, turning a transient platform failure into a guaranteed leak until + // `sleepAfter`. Only a destroy we watched resolve may skip. + assert.equal(destroyConfirmed(TOMBSTONE_ATTEMPTED), false); + assert.equal(destroyConfirmed("1"), false, "the legacy KV value is an attempt, not a confirmation"); +}); + +test("only a confirmed destroy skips the sandbox RPC", () => { + assert.equal(destroyConfirmed(TOMBSTONE_DESTROYED), true); +}); + +test("an unrecognised marker falls back to doing the work", () => { + // A value from a future release, or a partial write. Every mistake here must + // cost an extra RPC, never a stranded container. + assert.equal(destroyConfirmed(""), false); + assert.equal(destroyConfirmed("destroyed "), false); + assert.equal(destroyConfirmed("DESTROYED"), false); + assert.equal(destroyConfirmed("2"), false); +}); + +test("the attempted marker keeps the byte value already in KV", () => { + // A rolling deploy has both versions live, and markers written by the old one + // survive for their whole TTL. `isTombstoned` in index.ts treats any non-null + // value as tombstoned, so the resurrection gate keeps working either way — + // but changing this literal would still be a needless flag day. + assert.equal(TOMBSTONE_ATTEMPTED, "1"); + assert.notEqual(TOMBSTONE_DESTROYED, TOMBSTONE_ATTEMPTED); +}); + +test("the marker outlives the container it guards", () => { + // `sleepAfter` is 5m (index.ts). A marker that expired first would let a + // straggler request resurrect a container under a dead id. + assert.ok(TOMBSTONE_TTL_SECONDS >= 300, `${TOMBSTONE_TTL_SECONDS}s must outlast sleepAfter=5m`); +}); + +// ---- the teardown failure classifier -------------------------------------- + +test("the three platform messages are expected teardown failures", () => { + assert.equal(isExpectedTeardownFailure(new Error(CAPACITY)), true); + assert.equal(isExpectedTeardownFailure(new Error(UNREACHABLE)), true); + assert.equal(isExpectedTeardownFailure(new Error(NOT_RUNNING)), true); +}); + +test("it sees through a cause chain", () => { + const wrapped = new Error("destroy failed", { cause: new Error(CAPACITY) }); + assert.equal(isExpectedTeardownFailure(wrapped), true); + assert.equal(isExpectedTeardownFailure(new Error("outer", { cause: wrapped })), true); +}); + +test("a self-referencing cause chain terminates", () => { + const loop = new Error("boom"); + loop.cause = loop; + assert.equal(isExpectedTeardownFailure(loop), false); +}); + +test("anything we have not diagnosed keeps today's 500 and today's report", () => { + assert.equal(isExpectedTeardownFailure(new TypeError("x is not a function")), false); + assert.equal(isExpectedTeardownFailure(new Error("Network connection lost.")), false); + assert.equal(isExpectedTeardownFailure("Maximum number of running container instances exceeded"), false, + "a non-Error throw is not a platform message we recognise"); + assert.equal(isExpectedTeardownFailure(null), false); + assert.equal(isExpectedTeardownFailure(undefined), false); +}); + +test("a reworded capacity message degrades to today's behaviour, not to silence", () => { + // The documented degrade direction, same as `isPortNotListening`: these + // strings are raised by the platform, not by any package here, so a match is + // the only signal available. If Cloudflare rewords one, the predicate stops + // matching and we go back to reporting a 500 — noisy, never silent. + assert.equal(isExpectedTeardownFailure(new Error("Too many container instances are running")), false); +}); + +// ---- the create-side capacity classifier ---------------------------------- + +test("only the capacity message means 'at capacity' on create", () => { + // Narrower than the teardown predicate on purpose. Telling a visitor the + // service is full when the platform actually said "the container is not + // running" would be a lie, and an unreachable service already has an honest + // tier client-side (`sessionStartMessage`, DEV-2553). + assert.equal(isAtCapacityFailure(new Error(CAPACITY)), true); + assert.equal(isAtCapacityFailure(new Error(UNREACHABLE)), false); + assert.equal(isAtCapacityFailure(new Error(NOT_RUNNING)), false); + assert.equal(isAtCapacityFailure(new Error("boom")), false); +}); + +test("the at-capacity sentence never trips the App.tsx connectivity heuristic", () => { + // Same cross-package contract `session-start-failure.test.mjs` pins: + // `describeRuntimeError` in apps/authoring/src/App.tsx REPLACES a container + // message matching this alternation with "install Docker, run the API + // worker". A visitor who found the pool full is not a developer with no + // worker running. + assert.doesNotMatch(atCapacityMessage, /session start failed/i); + assert.doesNotMatch(atCapacityMessage, /fetch/i); + assert.doesNotMatch(atCapacityMessage, /failed to fetch|networkerror|load failed/i); + // And it must not leak the platform's own words: "configuring a higher value + // for max_instances" is an instruction to us, not to a visitor. + assert.doesNotMatch(atCapacityMessage, /max_instances|container instances/i); + assert.ok(atCapacityMessage.length < 200, "a sentence, not a log excerpt"); +}); + +test("the envelope code is stable", () => { + // `sessionStartMessage` in packages/runtime/src/container.ts matches this + // exact code to pass the sentence through unwrapped. + assert.equal(AT_CAPACITY_CODE, "at_capacity"); +}); diff --git a/runner/pipeline/session-start-failure.test.mjs b/runner/pipeline/session-start-failure.test.mjs index d654d6ac..c999440d 100644 --- a/runner/pipeline/session-start-failure.test.mjs +++ b/runner/pipeline/session-start-failure.test.mjs @@ -254,6 +254,23 @@ test("a budget refusal still reaches the user as the server phrased it", async ( assert.equal(err.message, sentence); }); +test("an at-capacity refusal reaches the user as the server phrased it", async () => { + // DEV-2556. When every container slot is taken the Worker now answers 503 + // `{ error: "at_capacity", message }` instead of letting the platform's own + // words ("…try configuring a higher value for max_instances") leave as a 500. + // That sentence is written for a visitor, so it must arrive unwrapped — the + // generic tier would prefix "session start failed (503):" and hand the whole + // thing to the App.tsx heuristic, which answers with the local-dev Docker + // hint. `pipeline/session-lifecycle.test.mjs` pins the sentence itself; this + // pins that the runtime does not wrap it. + const sentence = "All live-preview sandboxes are busy right now. Try again in a minute."; + const err = await sessionStartError(503, JSON.stringify({ error: "at_capacity", message: sentence })); + + assert.equal(err.code, "at_capacity"); + assert.equal(err.message, sentence); + assert.doesNotMatch(err.message, /unavailable/i, "the envelope-less tier must not swallow an envelope"); +}); + test("an ordinary envelope error is unchanged", async () => { const err = await sessionStartError(500, JSON.stringify({ error: "boom", message: "boom" })); diff --git a/runner/workers/api/src/index.ts b/runner/workers/api/src/index.ts index 7dd0de90..b7344d65 100644 --- a/runner/workers/api/src/index.ts +++ b/runner/workers/api/src/index.ts @@ -26,6 +26,16 @@ import { isMcpValidationError, validateMcpFiles } from "./mcp-create.js"; import { demoListQuery, parseDemoScope } from "./demos-list.js"; import { errorPageResponse, wantsHtmlError } from "./error-page.js"; import { classifyPreviewBootFailure, isPortNotListening } from "./preview-boot.js"; +import { + AT_CAPACITY_CODE, + atCapacityMessage, + destroyConfirmed, + isAtCapacityFailure, + isExpectedTeardownFailure, + TOMBSTONE_ATTEMPTED, + TOMBSTONE_DESTROYED, + TOMBSTONE_TTL_SECONDS, +} from "./session-lifecycle.js"; import { ImportError, MAX_PAYLOAD_CHARS, importFromUrl, validatePayloadFiles } from "./import-url.js"; import { createDemo, getDemo, getDemoSource, invalidateDemo, serveDemoAsset, shortId, updateDemo, type DemoRow } from "./share.js"; import { @@ -332,7 +342,28 @@ const liveSbx = (env: Env, id: string): SandboxLike => getSandboxShallow(env.San * A KV read failure counts as "no tombstone" — refusing healthy sessions on * a KV hiccup is worse than falling back to the sleepAfter backstop. */ const isTombstoned = async (env: Env, sessionId: string): Promise => - (await env.CACHE.get(`session-tombstone:${sessionId}`).catch(() => null)) !== null; + (await readTombstone(env, sessionId)) !== null; + +const tombstoneKey = (sessionId: string) => `session-tombstone:${sessionId}`; + +/** The marker's value, not just its presence — `TOMBSTONE_ATTEMPTED` (a teardown + * started, outcome unknown) vs `TOMBSTONE_DESTROYED` (we watched `destroy()` + * resolve). Only the second lets `DELETE /api/session/:id` answer without a + * sandbox RPC; see `destroyConfirmed`. A KV read failure reads as no marker, + * for the reason `isTombstoned` documents. */ +const readTombstone = (env: Env, sessionId: string): Promise => + env.CACHE.get(tombstoneKey(sessionId)).catch(() => null); + +/** Write (or upgrade) a marker. Always best-effort: every caller is on a + * teardown path whose primary action is the destroy, and a KV hiccup there + * must not become the 500 this whole change exists to remove. Without the + * marker the mid-create race and the duplicate-DELETE skip both fall back to + * the `sleepAfter` backstop, which is where they were before DEV-2556. */ +async function putTombstone(env: Env, sessionId: string, marker: string): Promise { + try { + await env.CACHE.put(tombstoneKey(sessionId), marker, { expirationTtl: TOMBSTONE_TTL_SECONDS }); + } catch { /* defense-in-depth only */ } +} function cors(resp: Response): Response { const h = new Headers(resp.headers); @@ -459,11 +490,13 @@ async function sessionSubrouteGuard(env: Env, sessionId: string): Promise => { if (!(await isTombstoned(env, sessionId))) return null; - try { await sandbox.destroy(); } catch { /* best effort */ } + // Never skipped on a `destroyed` marker, unlike the DELETE handler: a + // confirmed destroy here refers to the generation the DELETE tore + // down, and the whole reason this check exists is that the create + // kept running afterwards and booted a NEW container under the same + // id. Skipping would leak exactly the container this is here to + // reclaim. The marker below is written after the destroy resolves, so + // it describes the container that actually just went away — which is + // what makes the client's follow-up DELETE (container.ts sends one + // when a create finishes after dispose) free instead of a boot. + try { + await sandbox.destroy(); + await putTombstone(env, sessionId, TOMBSTONE_DESTROYED); + } catch { /* best effort */ } return json({ error: "session was closed while it was being created" }, 410); }; @@ -701,6 +746,22 @@ export default Sentry.withSentry(sentryOptions, { // run the same tombstone check before surfacing the error. const closed = await closedWhileCreating(); if (closed) return closed; + // The pool is full (DEV-2556). Today this leaves as a 500 carrying + // the platform's own words — "…try configuring a higher value for + // max_instances" — straight to a visitor, via the raw-body tier of + // `sessionStartMessage`. It is a refusal, not a fault: a 503 with an + // envelope, phrased for the person reading it, alongside the budget + // guardrail's denials. Kept as a Sentry event on the client (the + // `tier2-session-start`/503 fingerprint) — with the outer catch no + // longer firing, that plus this log line is the only capacity signal + // we have, and raising `max_instances` is a spend decision that needs + // to be made on evidence. + if (isAtCapacityFailure(err)) { + console.warn( + `[session] refused ${body.framework} session ${sessionId}: container pool at capacity`, + ); + return json({ error: AT_CAPACITY_CODE, message: atCapacityMessage }, 503); + } throw err; } } @@ -783,6 +844,24 @@ export default Sentry.withSentry(sentryOptions, { // DELETE /api/session/:id -> destroy container if (request.method === "DELETE" && parts[0] === "api" && parts[1] === "session" && parts.length === 3) { const sessionId = parts[2]!; + // A session we have already watched go away answers from KV. Every + // sandbox RPC boots a container if one isn't running, so a second + // DELETE that re-entered the sandbox would be asking for a slot in + // order to destroy something that is not there — the create/delete + // race (packages/runtime/src/container.ts) sends exactly that. + if (destroyConfirmed(await readTombstone(env, sessionId))) { + // Still metered: `closedWhileCreating()` writes the confirmation + // without ever metering final, so on that arm this request is the + // only one that can close the awake window. A no-op once the meter + // key is gone (`meterSessionUnsafe` returns early on a missing + // meter), which is what every other writer of this marker leaves + // behind — the cost of keeping it is one KV read. + await meterSession(env, sessionId, { final: true }); + // Keep the marker alive for as long as DELETEs keep arriving: it is + // also what the resurrection gate above reads. + await putTombstone(env, sessionId, TOMBSTONE_DESTROYED); + return cors(new Response(null, { status: 204 })); + } // Tombstone BEFORE destroying: if a create for this id is still in // flight (tab closed mid-POST), destroy() alone hits a half-built // session and the create keeps going — the POST handler re-checks @@ -790,14 +869,30 @@ export default Sentry.withSentry(sentryOptions, { // stale markers from accumulating. Best-effort: a KV hiccup must not // block the primary destroy below (without the marker the mid-create // race falls back to the sleepAfter backstop). - try { - await env.CACHE.put(`session-tombstone:${sessionId}`, "1", { expirationTtl: 600 }); - } catch { /* tombstone is defense-in-depth only */ } + await putTombstone(env, sessionId, TOMBSTONE_ATTEMPTED); // Close the awake window before the container goes away: this is the // one teardown path that knows the session is over for good. await meterSession(env, sessionId, { final: true }); const sandbox = liveSbx(env, sessionId); - await sandbox.destroy(); + // Releasing a container must not need one (DEV-2556, Sentry DEMOS-1). + // When the pool is full the platform refuses `destroy()` itself, and + // this route — the only unguarded destroy in the file — turned that + // into a thrown 500 nobody could act on: the caller is a fire-and- + // forget `keepalive` fetch from `pagehide` that discards the response. + // A refusal also means no slot is being held by whatever we failed to + // reach, so there is nothing left to reclaim here. Recognised platform + // messages become a log line and a 204; anything else still throws to + // the outer catch and keeps today's status and today's Sentry event. + try { + await sandbox.destroy(); + await putTombstone(env, sessionId, TOMBSTONE_DESTROYED); + } catch (err) { + if (!isExpectedTeardownFailure(err)) throw err; + console.warn( + `[session] teardown for ${sessionId} declined by the platform:`, + err instanceof Error ? err.message : String(err), + ); + } return cors(new Response(null, { status: 204 })); } diff --git a/runner/workers/api/src/session-lifecycle.ts b/runner/workers/api/src/session-lifecycle.ts new file mode 100644 index 00000000..0a38b9bb --- /dev/null +++ b/runner/workers/api/src/session-lifecycle.ts @@ -0,0 +1,154 @@ +// The two decisions the Tier-2 session lifecycle makes about a container the +// platform will not give us (DEV-2556): what a *teardown* does when the pool +// refuses it, and what a visitor is told when a *create* is refused. +// +// Sentry DEMOS-1 caught the first half. Two `DELETE /api/session/:id` calls, +// 328 ms apart, threw "Maximum number of running container instances exceeded" +// out of `sandbox.destroy()`, through the outer catch in `index.ts`, into a 500 +// and a Sentry event. The causal story is the inverse of what it looks like: an +// instance that cannot be allocated is not occupying one of the five slots, so a +// failed teardown is a *symptom* of a full pool and never a contributor to one. +// Nothing leaked. And nobody read the 500 either — `deleteSession()` in +// packages/runtime/src/container.ts is `void fetch(…, { keepalive: true }) +// .catch(() => {})`, fired from `pagehide`. The release path was the only +// `destroy()` in `index.ts` that was not already best-effort; its two siblings +// (`closedWhileCreating`, the closed-tier teardown) both swallow. +// +// Deliberately free of Cloudflare imports and of runtime sibling imports, and +// written in erasable syntax only (no enum, no parameter properties), so +// `pipeline/` can import this `.ts` directly under `--experimental-strip-types` +// — the same constraint `preview-boot.ts` and `monitor-inject.ts` document. +// `index.ts` is a Worker entrypoint and cannot be imported that way at all, +// which is exactly why both decisions are lifted out of it: this module is the +// only seam either one can be tested through. + +// ---- the tombstone state machine ------------------------------------------ +// +// `session-tombstone:` in KV is written before a teardown and read by the +// resurrection gate in `index.ts` (any RPC on a destroyed session auto-boots a +// fresh container under the dead id, so every session-scoped route checks it +// first). It now carries two states instead of one, which is what makes a +// duplicate DELETE free rather than a container boot in order to destroy one. + +/** A teardown was started for this session. What the gate has always written, + * kept byte-identical so a rolling deploy and any legacy marker still in KV + * keep working — `isTombstoned` treats any non-null value as tombstoned. */ +export const TOMBSTONE_ATTEMPTED = "1"; + +/** `destroy()` resolved. The ONLY value that lets a later DELETE answer from KV + * without touching the sandbox. */ +export const TOMBSTONE_DESTROYED = "destroyed"; + +/** Ten minutes, comfortably past `sleepAfter = "5m"` in `index.ts`: a marker + * must outlive the container it guards, or a straggler request resurrects one + * under a dead id. Also the ceiling on how long a DELETE stays idempotent. */ +export const TOMBSTONE_TTL_SECONDS = 600; + +/** + * Whether we have watched this session's container actually go away. + * + * True for exactly one literal. `null` (no marker), `"1"` (a teardown was + * attempted, outcome unknown) and anything unrecognised all mean "still needs + * the RPC" — the direction a mistake has to fail in. Answering `true` for an + * attempt would turn a transient destroy failure into a guaranteed leak, since + * the retry a failed teardown depends on is the *next* DELETE. + * + * True is a statement about a container *generation*, not about an id, so it + * only licenses a skip where nothing can have booted a fresh container under + * the same id since. That holds for a repeated `DELETE /api/session/:id` (the + * resurrection gate refuses every other route on a tombstoned session). It does + * NOT hold in `closedWhileCreating`, whose entire premise is that a create kept + * running past the DELETE and built a second container — that path must always + * destroy, and only writes the confirmation afterwards. + */ +export function destroyConfirmed(marker: string | null | undefined): boolean { + return marker === TOMBSTONE_DESTROYED; +} + +// ---- platform failure classification -------------------------------------- + +/** Bound on the `.cause` walk. A self-referencing cause is not hypothetical. */ +const MAX_CAUSE_DEPTH = 5; + +/** Test a message pattern against an error and its causes. + * + * Non-`Error` throws answer false everywhere: these strings are raised by + * workerd and by the containers SDK, both of which throw real Errors, so a + * bare string carrying the same words is somebody else's — and unrecognised is + * the safe answer for every caller here. */ +function messageMatches(err: unknown, pattern: RegExp): boolean { + const seen = new Set(); + let current: unknown = err; + for (let depth = 0; depth < MAX_CAUSE_DEPTH; depth += 1) { + if (!(current instanceof Error) || seen.has(current)) return false; + seen.add(current); + if (pattern.test(current.message)) return true; + current = current.cause; + } + return false; +} + +/** The pool is full. The DEMOS-1 message, and the only one of the three that + * supports telling a visitor "we are at capacity". */ +const AT_CAPACITY_PATTERN = /maximum number of running container instances exceeded/i; + +/** The other two ways the platform says "there is no container here to talk to + * right now" — both seen on this project, the first in the same Sentry group. */ +const SERVICE_UNREACHABLE_PATTERN = /container service is unreachable/i; +const NOT_RUNNING_PATTERN = /container is not running/i; + +/** + * Whether a failed `destroy()` is the platform declining rather than a teardown + * regression. + * + * All three messages mean the same thing for a release: there is nothing here + * to destroy right now, and there is no slot being held by whatever we failed + * to reach. The caller answers 204 and logs; anything that does NOT match is + * rethrown and keeps today's status and today's Sentry event. + * + * DEGRADE DIRECTION, documented like `isPortNotListening`: these strings come + * from the platform, not from any package in this repo, so a message match is + * the only signal available. If Cloudflare rewords one, this predicate stops + * matching and the case falls back to report-and-500 — noisy, never silent. + */ +export function isExpectedTeardownFailure(err: unknown): boolean { + return ( + messageMatches(err, AT_CAPACITY_PATTERN) || + messageMatches(err, SERVICE_UNREACHABLE_PATTERN) || + messageMatches(err, NOT_RUNNING_PATTERN) + ); +} + +/** + * Whether a failed *create* means the instance pool is full. + * + * Narrower than the teardown predicate on purpose: only the capacity message + * supports the sentence below. "The container is not running" and "the service + * is unreachable" are different faults, and the client already has an honest + * tier for an unavailable service (`sessionStartMessage`, DEV-2553). + */ +export function isAtCapacityFailure(err: unknown): boolean { + return messageMatches(err, AT_CAPACITY_PATTERN); +} + +/** Machine-readable reason on the 503 envelope. `sessionStartMessage` in + * packages/runtime/src/container.ts matches this exact code to pass the + * sentence below through to the user unwrapped. */ +export const AT_CAPACITY_CODE = "at_capacity"; + +/** + * What a visitor sees when every live-preview slot is taken. + * + * Today they get the platform's own words instead — "…try configuring a higher + * value for max_instances", an instruction to us, wrapped in a 500. Two things + * constrain this sentence: + * - It must contain none of /failed to fetch|networkerror|load failed|session + * start failed|fetch/i. `describeRuntimeError` in apps/authoring/src/App.tsx + * REPLACES any container message matching that alternation with "install + * Docker and run the local API worker", which is the wrong answer for a + * visitor on demos.handsontable.com (DEMOS-9, DEV-2538/DEV-2553). + * - It must not leak `max_instances` or any other knob only we can turn. + * `pipeline/session-lifecycle.test.mjs` pins both. + */ +export const atCapacityMessage = + "All live-preview sandboxes are busy right now. Nothing is wrong with the code — try again in a minute."; From 7fca293f95b264fd84ded019bd9b4d9496f51e95 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Artur=20M=C4=99dryga=C5=82?= Date: Mon, 17 Aug 2026 16:04:58 +0200 Subject: [PATCH 2/2] fix(runner): keep a swallowed teardown legible in Sentry (DEV-2556) Review follow-up on the DEV-2556 teardown fix. The swallow is right -- a 204 is the correct answer to a fire-and-forget `keepalive` fetch from `pagehide` that discards the response -- but it is only defensible if the failure moves somewhere durable, and it did not. The change replaced the thrown 500 (and its Sentry event) with a `console.warn`. `observability.head_sampling_rate` is 0.1 in workers/api/wrangler.jsonc, so nine of ten of those lines are never retained. At two capacity events in 90 days the expected number of surviving log lines is a fraction of one: the signal the swallow was traded for would not have existed. A declined teardown now also files a `warning`-level Sentry event under its own `tier2-teardown-declined` fingerprint. Fingerprinted for the reason the preview-boot capture is: the outer catch reports bare and this project groups on the culprit `Object.fetch(index)`, so an unfingerprinted event lands straight back in the DEMOS-1 grab-bag. `beforeSend` (rehomeBudgetAlert) only re-homes `context: budget-alert` and drops nothing, so a warning arrives. This matters most for `container service is unreachable`, the weakest member of `isExpectedTeardownFailure`: unlike the other two it does NOT imply no slot is held, so a swallowed one can leave a container billing until sleepAfter. Same defect on the create side, client-side. `reportRuntimeError` in apps/authoring/src/App.tsx fingerprints on `["tier2-session-start", status]`, so the new `at_capacity` 503 -- named in the Worker comment as the surviving capacity evidence -- shared one issue with an envelope-less platform 503, same status and an unrelated cause. The server's machine-readable code now joins the fingerprint when it sent one. Appended rather than substituted, so uncoded failures keep the fingerprint they have today and no live issue regroups. Verified: pnpm build, pnpm typecheck (4/4 projects), pnpm test (584/584), pnpm --filter @handsontable/demo-authoring build, and playwright on an own --strictPort port 4291 via E2E_BASE_URL (166 expected, 0 unexpected, 0 flaky). Co-Authored-By: Claude Opus 5 --- runner/apps/authoring/src/App.tsx | 21 ++++++++- runner/workers/api/src/index.ts | 49 ++++++++++++++++++--- runner/workers/api/src/session-lifecycle.ts | 19 ++++++-- 3 files changed, 76 insertions(+), 13 deletions(-) diff --git a/runner/apps/authoring/src/App.tsx b/runner/apps/authoring/src/App.tsx index 2d1e3f62..4c889d9d 100644 --- a/runner/apps/authoring/src/App.tsx +++ b/runner/apps/authoring/src/App.tsx @@ -200,9 +200,26 @@ function reportRuntimeError(e: unknown, engine: string): void { if (isBudgetRefusal(e)) return; if (e instanceof SessionStartError) { if (e.status === 410) return; + // The server's machine-readable reason joins the fingerprint when it sent + // one (DEV-2556). Status alone is too coarse now that the Worker refuses at + // capacity with its own 503: `at_capacity` — every container slot taken, + // which is a spend decision — would otherwise land in the same issue as an + // envelope-less platform 503 ("The sandbox service is unavailable right + // now"), which is a gateway fault and has nothing to do with our pool. That + // grouping is the only capacity evidence the create side has left, since the + // refusal is now a returned 503 rather than a throw the outer catch reports. + // + // Appended rather than substituted so uncoded failures keep the exact + // fingerprint they have today: no live issue regroups, and only the coded + // refusals split off. `budget_*` codes never reach here (suppressed above). + const code = typeof e.code === "string" && e.code.length > 0 ? e.code : null; Sentry.captureException(e, { - tags: { context: "tier2-session-start", session_status: String(e.status) }, - fingerprint: ["tier2-session-start", String(e.status)], + tags: { + context: "tier2-session-start", + session_status: String(e.status), + ...(code ? { session_refusal: code } : {}), + }, + fingerprint: ["tier2-session-start", String(e.status), ...(code ? [code] : [])], }); return; } diff --git a/runner/workers/api/src/index.ts b/runner/workers/api/src/index.ts index b7344d65..486f36dd 100644 --- a/runner/workers/api/src/index.ts +++ b/runner/workers/api/src/index.ts @@ -751,11 +751,20 @@ export default Sentry.withSentry(sentryOptions, { // max_instances" — straight to a visitor, via the raw-body tier of // `sessionStartMessage`. It is a refusal, not a fault: a 503 with an // envelope, phrased for the person reading it, alongside the budget - // guardrail's denials. Kept as a Sentry event on the client (the - // `tier2-session-start`/503 fingerprint) — with the outer catch no - // longer firing, that plus this log line is the only capacity signal - // we have, and raising `max_instances` is a spend decision that needs - // to be made on evidence. + // guardrail's denials. + // + // The outer catch no longer fires here, so this refusal has to stay + // legible some other way — raising `max_instances` is a spend decision + // and it needs evidence. That evidence is the CLIENT's Sentry event: + // `reportRuntimeError` in apps/authoring/src/App.tsx keeps reporting a + // `SessionStartError` (only `isBudgetRefusal` is suppressed, and this + // code is not a `budget_` one), fingerprinted + // `["tier2-session-start", "503", "at_capacity"]` — the trailing code + // is what keeps it out of the same issue as an envelope-less platform + // 503, which carries the same status and a completely different cause. + // The log line below is a convenience, NOT the signal: + // `observability.head_sampling_rate` is 0.1, so nine of ten are never + // retained. if (isAtCapacityFailure(err)) { console.warn( `[session] refused ${body.framework} session ${sessionId}: container pool at capacity`, @@ -881,8 +890,8 @@ export default Sentry.withSentry(sentryOptions, { // forget `keepalive` fetch from `pagehide` that discards the response. // A refusal also means no slot is being held by whatever we failed to // reach, so there is nothing left to reclaim here. Recognised platform - // messages become a log line and a 204; anything else still throws to - // the outer catch and keeps today's status and today's Sentry event. + // messages become a 204; anything else still throws to the outer catch + // and keeps today's status and today's Sentry event. try { await sandbox.destroy(); await putTombstone(env, sessionId, TOMBSTONE_DESTROYED); @@ -892,6 +901,32 @@ export default Sentry.withSentry(sentryOptions, { `[session] teardown for ${sessionId} declined by the platform:`, err instanceof Error ? err.message : String(err), ); + // The log line alone is NOT the signal, and assuming it was is how + // this swallow would have gone dark: `observability.head_sampling_rate` + // is 0.1 in wrangler.jsonc, so nine of ten of these console.warn lines + // are never retained. Capacity events are rare (two in 90 days), which + // makes the expected number of surviving log lines a fraction of one. + // + // So the event still goes to Sentry — just as a `warning` that no + // longer fails the request, instead of the 500 it used to ride in on. + // Fingerprinted for the reason the preview-boot capture above is: the + // outer catch reports bare, and this project groups on the culprit + // `Object.fetch(index)`, so without a fingerprint this would land back + // in the same grab-bag as DEMOS-1 and be unreadable as a capacity + // signal. `beforeSend` (rehomeBudgetAlert) only re-homes + // `context: "budget-alert"` and drops nothing, so a warning arrives. + // + // This matters most for `container service is unreachable`, the + // weakest member of `isExpectedTeardownFailure`: unlike the other two + // it does NOT imply no slot is held, so a swallowed one can leave a + // container billing until sleepAfter. 204 is still the right answer to + // a caller that discards the response — but only because the failure + // is legible somewhere, and this is that somewhere. + Sentry.captureException(err, { + level: "warning", + fingerprint: ["tier2-teardown-declined"], + tags: { context: "tier2-teardown" }, + }); } return cors(new Response(null, { status: 204 })); } diff --git a/runner/workers/api/src/session-lifecycle.ts b/runner/workers/api/src/session-lifecycle.ts index 0a38b9bb..541b9973 100644 --- a/runner/workers/api/src/session-lifecycle.ts +++ b/runner/workers/api/src/session-lifecycle.ts @@ -101,10 +101,21 @@ const NOT_RUNNING_PATTERN = /container is not running/i; * Whether a failed `destroy()` is the platform declining rather than a teardown * regression. * - * All three messages mean the same thing for a release: there is nothing here - * to destroy right now, and there is no slot being held by whatever we failed - * to reach. The caller answers 204 and logs; anything that does NOT match is - * rethrown and keeps today's status and today's Sentry event. + * The caller answers 204 and files a `warning`-level Sentry event under its own + * fingerprint; anything that does NOT match is rethrown and keeps today's status + * and today's (bare, grab-bag) Sentry event. The 204 is for the caller's + * benefit — a fire-and-forget `keepalive` fetch that discards the response — and + * is only defensible because the failure stays legible in Sentry. A `console` + * line is not that: `observability.head_sampling_rate` is 0.1 in wrangler.jsonc. + * + * The three messages are NOT equally strong evidence, and the weakest one is why + * the report above is not optional. "Not running" and "maximum instances + * exceeded" both imply there is no slot being held by whatever we failed to + * reach — nothing to reclaim. "Service unreachable" does not: a container may + * well be running and billing, and the teardown simply could not get to it. The + * `sleepAfter` backstop bounds that, and it is exactly today's outcome (a 500 + * nobody retries either), so this is not a regression — but it is the case a + * reviewer should expect to see in the `tier2-teardown-declined` issue. * * DEGRADE DIRECTION, documented like `isPortNotListening`: these strings come * from the platform, not from any package in this repo, so a message match is