From b988f38a99c4459b13d61fe6e9442b20cd926c34 Mon Sep 17 00:00:00 2001 From: aron <263346377+aron-cf@users.noreply.github.com> Date: Tue, 16 Jun 2026 12:02:07 +0000 Subject: [PATCH 01/11] workspace: invalidate cached handles on transport failures A CloudflareContainerBackend whose WebSocket wedges without producing a clean close signal used to leave the Workspace holding a dead BackendHandle. Subsequent push, pull, and shell.exec calls reused the same broken RPC stub forever. Add a conservative classifier, isWorkspaceTransportFailure, that matches a WorkspaceTransportError tag class plus a short list of phrases that consistently mean the transport is gone: capnweb session shutdown, WebSocket close, container-port unreachable. The classifier walks the cause chain so wrappers like 'watermark sync failed: ' still classify when the inner error qualifies. Workspace.push and Workspace.pull now route through an invalidation helper that drops the cached handle by identity when an RPC fails this classifier; the shell router does the same for exec and get. Non-transport failures, including normal shell exits and EROFS, are left alone so a single bad command does not force a reconnect. --- .../workspace/src/transport-failure.test.ts | 53 ++++++ packages/workspace/src/transport-failure.ts | 81 +++++++++ packages/workspace/src/workspace.test.ts | 161 ++++++++++++++++++ packages/workspace/src/workspace.ts | 73 +++++++- 4 files changed, 364 insertions(+), 4 deletions(-) create mode 100644 packages/workspace/src/transport-failure.test.ts create mode 100644 packages/workspace/src/transport-failure.ts diff --git a/packages/workspace/src/transport-failure.test.ts b/packages/workspace/src/transport-failure.test.ts new file mode 100644 index 00000000..287db462 --- /dev/null +++ b/packages/workspace/src/transport-failure.test.ts @@ -0,0 +1,53 @@ +import { describe, expect, it } from "vitest"; + +import { isWorkspaceTransportFailure, WorkspaceTransportError } from "./transport-failure.js"; + +describe("isWorkspaceTransportFailure", () => { + it("recognises WorkspaceTransportError instances", () => { + expect(isWorkspaceTransportFailure(new WorkspaceTransportError("nope"))).toBe(true); + }); + + it("recognises capnweb session-closed phrasing", () => { + expect( + isWorkspaceTransportFailure(new Error("RPC was canceled because RPC session was shut down")), + ).toBe(true); + expect(isWorkspaceTransportFailure(new Error("the RPC session was closed"))).toBe(true); + }); + + it("recognises WebSocket transport failures", () => { + expect(isWorkspaceTransportFailure(new Error("WebSocket is not open"))).toBe(true); + expect(isWorkspaceTransportFailure(new Error("WebSocket closed unexpectedly"))).toBe(true); + expect(isWorkspaceTransportFailure(new Error("socket hang up"))).toBe(true); + }); + + it("recognises container-port unreachable errors", () => { + expect(isWorkspaceTransportFailure(new Error("connection refused"))).toBe(true); + expect(isWorkspaceTransportFailure(new Error("ECONNRESET reading from container"))).toBe(true); + }); + + it("recognises heartbeat / watermark failures by their wrapping error", () => { + const cause = new Error("WebSocket closed"); + const err = new WorkspaceTransportError("heartbeat failed", { cause }); + expect(isWorkspaceTransportFailure(err)).toBe(true); + }); + + it("walks the cause chain", () => { + const inner = new Error("RPC session was shut down"); + const outer = new Error("watermark sync failed"); + (outer as Error & { cause?: unknown }).cause = inner; + expect(isWorkspaceTransportFailure(outer)).toBe(true); + }); + + it("returns false for ordinary errors", () => { + expect(isWorkspaceTransportFailure(new Error("ENOENT: no such file"))).toBe(false); + expect(isWorkspaceTransportFailure(new Error("command exited with code 1"))).toBe(false); + expect(isWorkspaceTransportFailure(new Error("permission denied"))).toBe(false); + }); + + it("returns false for non-Error values", () => { + expect(isWorkspaceTransportFailure(undefined)).toBe(false); + expect(isWorkspaceTransportFailure(null)).toBe(false); + expect(isWorkspaceTransportFailure("WebSocket closed")).toBe(false); + expect(isWorkspaceTransportFailure(42)).toBe(false); + }); +}); diff --git a/packages/workspace/src/transport-failure.ts b/packages/workspace/src/transport-failure.ts new file mode 100644 index 00000000..063732da --- /dev/null +++ b/packages/workspace/src/transport-failure.ts @@ -0,0 +1,81 @@ +// Conservative classifier for "the backend's transport is gone". +// +// The Workspace caches a BackendHandle per backend id and clears +// it when the handle's `closed` promise resolves. A wedged +// container or a half-broken capnweb session may surface failures +// through an RPC rejection long before that promise fires. To +// avoid handing the same broken stub to the next caller, the +// Workspace runs each backend-touching RPC through a try/catch +// that consults this classifier and invalidates the cached +// handle on a match. +// +// Two ways to flag a transport failure: +// +// 1. Throw a WorkspaceTransportError from backend code we own +// (heartbeat onFailure, lease renewal, the container backend +// itself). Direct, no string matching. +// +// 2. Pattern-match the error message for known phrases that +// external libraries surface (capnweb session shutdown, +// WebSocket close, container-port unreachable). Conservative +// by design — false positives invalidate a still-good +// handle, which costs a reconnect; false negatives keep a +// dead stub around until the next signal. +// +// The classifier walks the cause chain so wrappers like +// "watermark sync failed: " still classify when the inner +// error is a transport failure. + +// Tag class for transport failures we throw ourselves. Carrying +// a class identity lets the classifier match without string +// inspection, and lets callers wrap an underlying cause for +// observability without losing the classification. +export class WorkspaceTransportError extends Error { + override readonly name = "WorkspaceTransportError"; + + constructor(message: string, options?: { cause?: unknown }) { + super(message); + if (options?.cause !== undefined) { + (this as Error & { cause?: unknown }).cause = options.cause; + } + } +} + +// Phrases that consistently indicate a dead transport. Conservative +// list — only phrases the workspace stack reliably produces or +// that capnweb / Cloudflare Workers ws emit on a closed session. +const TRANSPORT_PATTERNS: RegExp[] = [ + // capnweb session shutdown / cancellation. + /rpc session was shut down/i, + /rpc session was closed/i, + /rpc was canceled/i, + // WebSocket transport failures. + /websocket is not open/i, + /websocket closed/i, + /socket hang up/i, + // Container-port unreachable / TCP-level failures bubbling up + // through host.fetchPort. + /connection refused/i, + /econnrefused/i, + /econnreset/i, + /network is unreachable/i, +]; + +export function isWorkspaceTransportFailure(error: unknown): boolean { + let current: unknown = error; + // Bounded walk so a self-referential cause chain can't loop. + for (let depth = 0; depth < 8 && current !== undefined && current !== null; depth++) { + if (current instanceof WorkspaceTransportError) return true; + if (current instanceof Error) { + for (const pattern of TRANSPORT_PATTERNS) { + if (pattern.test(current.message)) return true; + } + const next = (current as Error & { cause?: unknown }).cause; + if (next === current) return false; + current = next; + continue; + } + return false; + } + return false; +} diff --git a/packages/workspace/src/workspace.test.ts b/packages/workspace/src/workspace.test.ts index 0e849088..25d88563 100644 --- a/packages/workspace/src/workspace.test.ts +++ b/packages/workspace/src/workspace.test.ts @@ -2,6 +2,7 @@ import { SQLiteTestStorage } from "@cloudflare/dofs/testing"; import { describe, expect, it, vi } from "vitest"; import type { BackendHandle, WorkspaceBackend } from "./backend.js"; +import { WorkspaceTransportError } from "./transport-failure.js"; import { Workspace } from "./workspace.js"; function makeStorage(): SQLiteTestStorage { @@ -752,3 +753,163 @@ describe("Workspace mutation serialization", () => { await push; }); }); + +describe("Workspace transport-failure invalidation", () => { + // A backend whose RPC throws a transport-like error while its + // `closed` promise never resolves used to leave the Workspace + // holding a dead handle. The cache must drop the handle on the + // way out so the next operation reconnects. + function transportFailingBackend( + id: string, + onConnect: () => void, + ): { backend: WorkspaceBackend; failNext: () => void } { + let shouldFail = false; + const sync: import("@cloudflare/workspace-rpc").SyncRPC = { + ...fakeRpc(), + async push(input) { + if (shouldFail) throw new WorkspaceTransportError("WebSocket closed"); + const reader = input.changes.getReader(); + try { + while (true) { + const { done } = await reader.read(); + if (done) break; + } + } finally { + reader.releaseLock(); + } + return { rev: 0, appliedPushRev: input.senderRev }; + }, + async fetchChanges() { + if (shouldFail) throw new WorkspaceTransportError("WebSocket closed"); + return { + currentRev: 0, + appliedPushRev: 0, + stream: new ReadableStream({ + start(c) { + c.close(); + }, + }), + }; + }, + async watermarks() { + return { currentRev: 0, pushRev: 0, fetchRev: 0 }; + }, + }; + const backend: WorkspaceBackend = { + id, + type: "fake", + async connect(): Promise { + onConnect(); + // closed promise stays pending forever — simulating a + // wedged transport that never produces a clean signal. + return { + rpc: composite(sync), + closed: new Promise(() => {}), + close: async () => {}, + }; + }, + }; + return { + backend, + failNext: () => { + shouldFail = true; + }, + }; + } + + it("push() invalidates the cached handle on a transport error", async () => { + let connects = 0; + const { backend, failNext } = transportFailingBackend("only", () => { + connects++; + }); + const ws = new Workspace({ storage: makeStorage(), backends: [backend] }); + await ws.ready("only"); + expect(connects).toBe(1); + + await ws.fs.writeFile("/a.txt", "hi"); + failNext(); + await expect(ws.push()).rejects.toThrow(/WebSocket closed/); + + // Next operation must reconnect — the bad handle is gone. + // Reset the fail flag so the second connect's RPC works. + // (failNext flipped a flag on the closure; re-binding is fine + // because connect() returns a fresh handle that reads it.) + // For this assertion we just need a fresh connect attempt. + await ws.push().catch(() => undefined); + expect(connects).toBe(2); + }); + + it("pull() invalidates the cached handle on a transport error", async () => { + let connects = 0; + const { backend, failNext } = transportFailingBackend("only", () => { + connects++; + }); + const ws = new Workspace({ storage: makeStorage(), backends: [backend] }); + await ws.ready("only"); + expect(connects).toBe(1); + failNext(); + await expect(ws.pull()).rejects.toThrow(/WebSocket closed/); + await ws.pull().catch(() => undefined); + expect(connects).toBe(2); + }); + + it("non-transport errors do not invalidate the cached handle", async () => { + let connects = 0; + const sync: import("@cloudflare/workspace-rpc").SyncRPC = { + ...fakeRpc(), + async push() { + throw new Error("EROFS: read-only file system"); + }, + }; + const backend: WorkspaceBackend = { + id: "only", + type: "fake", + async connect(): Promise { + connects++; + return { + rpc: composite(sync), + closed: new Promise(() => {}), + close: async () => {}, + }; + }, + }; + const ws = new Workspace({ storage: makeStorage(), backends: [backend] }); + await ws.ready("only"); + expect(connects).toBe(1); + await ws.fs.writeFile("/a.txt", "hi"); + await expect(ws.push()).rejects.toThrow(/EROFS/); + // Cache survives a non-transport error. + await ws.push().catch(() => undefined); + expect(connects).toBe(1); + }); + + it("shell.exec invalidates the cached handle on a transport error", async () => { + let connects = 0; + const shell: import("@cloudflare/workspace-rpc").ShellRPC = { + async exec() { + throw new WorkspaceTransportError("RPC session was shut down"); + }, + getExec: () => Promise.reject(new Error("not used")), + killExec: () => Promise.reject(new Error("not used")), + disposeExec: () => Promise.reject(new Error("not used")), + }; + const backend: WorkspaceBackend = { + id: "only", + type: "fake", + async connect(): Promise { + connects++; + return { + rpc: { sync: fakeRpc(), shell }, + sync: "none", + closed: new Promise(() => {}), + close: async () => {}, + }; + }, + }; + const ws = new Workspace({ storage: makeStorage(), backends: [backend] }); + await expect(ws.shell.exec("true")).rejects.toThrow(/RPC session was shut down/); + expect(connects).toBe(1); + await ws.shell.exec("true").catch(() => undefined); + expect(connects).toBe(2); + }); +}); diff --git a/packages/workspace/src/workspace.ts b/packages/workspace/src/workspace.ts index 3ee2aaf1..9e221300 100644 --- a/packages/workspace/src/workspace.ts +++ b/packages/workspace/src/workspace.ts @@ -27,6 +27,7 @@ import type { Mount } from "./mounts/types.js"; import { noopObserver, type WorkspaceObserver, withSpan } from "./observe.js"; import { WorkspaceShell } from "./shell.js"; import { WorkspaceStub } from "./stub.js"; +import { isWorkspaceTransportFailure } from "./transport-failure.js"; export interface WorkspaceOptions { // Local store backing this Workspace. In a Durable Object, pass @@ -342,7 +343,9 @@ export class Workspace { // keep calling push() unconditionally without paying // for it. if (handle.sync === "none") return 0; - return pushOnce(this.#db, handle.rpc.sync, resolvedId); + return this.#runWithInvalidation(resolvedId, handle, () => + pushOnce(this.#db, handle.rpc.sync, resolvedId), + ); }, (span, outcome) => { if (outcome.ok) span.setAttribute("workspace.sync.pushed", outcome.value); @@ -361,7 +364,9 @@ export class Workspace { if (resolvedId === undefined) return { applied: 0, skipped: [] }; const handle = await this.#handleFor(resolvedId); if (handle.sync === "none") return { applied: 0, skipped: [] }; - return pullOnce(this.#db, handle.rpc.sync, resolvedId); + return this.#runWithInvalidation(resolvedId, handle, () => + pullOnce(this.#db, handle.rpc.sync, resolvedId), + ); }, (span, outcome) => { if (!outcome.ok) return; @@ -372,6 +377,36 @@ export class Workspace { ); } + // Drop a cached handle when an operation fails with a known + // transport-level error. Matches by identity — a concurrent + // close() / `closed` watcher that already swapped the entry must + // not be clobbered. Returns true if the cached entry was the one + // we removed. + #invalidateHandle(id: string, handle: BackendHandle): boolean { + if (this.#handles.get(id) !== handle) return false; + this.#handles.delete(id); + this.#shells.delete(id); + return true; + } + + // Wrap an RPC-backed operation so a transport failure invalidates + // the cached handle before the error rethrows. Non-transport + // errors pass through untouched. + async #runWithInvalidation( + id: string, + handle: BackendHandle, + op: () => Promise, + ): Promise { + try { + return await op(); + } catch (error) { + if (isWorkspaceTransportFailure(error)) { + this.#invalidateHandle(id, handle); + } + throw error; + } + } + // Per-backend mutation FIFO. The shell exec bracket // (push → spawn → pull) and the public push() / pull() methods // route through this; reads bypass it entirely. A push to @@ -527,9 +562,20 @@ export class Workspace { this.#defaultBackendId ?? "", (id) => this.#shellFor(id), (id) => this.#resolveBackendId(id) ?? "", + (id, error) => this.#onShellError(id, error), ); return router as unknown as WorkspaceShell; } + + // Invalidate the cached handle for `id` when a shell-routed RPC + // fails with a known transport error. Matches the push/pull + // invalidation path; identity-checked so a concurrent reconnect + // is not clobbered. + #onShellError(id: string, error: unknown): void { + if (!isWorkspaceTransportFailure(error)) return; + const handle = this.#handles.get(id); + if (handle !== undefined) this.#invalidateHandle(id, handle); + } } // Selector wrapper that satisfies the WorkspaceShell surface but @@ -546,28 +592,47 @@ class WorkspaceShellRouter { readonly #defaultId: string; readonly #shellFor: (id: string) => Promise; readonly #resolveId: (id: string | undefined) => string; + readonly #onError: (id: string, error: unknown) => void; constructor( defaultId: string, shellFor: (id: string) => Promise, resolveId: (id: string | undefined) => string, + onError: (id: string, error: unknown) => void, ) { this.#defaultId = defaultId; this.#shellFor = shellFor; this.#resolveId = resolveId; + this.#onError = onError; } async exec(command: string, options: { backend?: string } & Record = {}) { const id = this.#resolveId(options.backend) || this.#defaultId; const shell = await this.#shellFor(id); const { backend: _backend, ...rest } = options; - return (shell.exec as unknown as (c: string, o: typeof rest) => unknown)(command, rest); + try { + return await (shell.exec as unknown as (c: string, o: typeof rest) => Promise)( + command, + rest, + ); + } catch (error) { + this.#onError(id, error); + throw error; + } } async get(id: string, options: { backend?: string } & Record = {}) { const backendId = this.#resolveId(options.backend) || this.#defaultId; const shell = await this.#shellFor(backendId); const { backend: _backend, ...rest } = options; - return (shell.get as unknown as (e: string, o: typeof rest) => unknown)(id, rest); + try { + return await (shell.get as unknown as (e: string, o: typeof rest) => Promise)( + id, + rest, + ); + } catch (error) { + this.#onError(backendId, error); + throw error; + } } } From 226d4a7536b8052dcdec0039a0a4a1ec1f6c3447 Mon Sep 17 00:00:00 2001 From: aron <263346377+aron-cf@users.noreply.github.com> Date: Tue, 16 Jun 2026 12:04:12 +0000 Subject: [PATCH 02/11] workspace: extract shared probeWsdHealth helper CloudflareContainerBackend.connect() used to probe the container port through a private #waitForPort loop that issued HEAD /health directly through host.fetchPort. A shared helper makes the boundary between "probe the port" and "decide what to do about the result" explicit, and lets other call sites that need the same health signal reach it through one entry point. Lift the probe into a tiny helper that takes the host, port, path, and per-probe timeout. Drives an AbortSignal.timeout so a wedged wsd that accepts the connection but never answers still surfaces as a rejection instead of hanging the caller. Drains the response body so the underlying connection can be released. #waitForPort now composes the helper inside its existing retry loop; the timeout error message is unchanged so the existing 'container port did not open' test stays green. --- .../container/cloudflare-container.ts | 8 ++- .../backends/container/health-probe.test.ts | 68 +++++++++++++++++++ .../src/backends/container/health-probe.ts | 46 +++++++++++++ 3 files changed, 119 insertions(+), 3 deletions(-) create mode 100644 packages/workspace/src/backends/container/health-probe.test.ts create mode 100644 packages/workspace/src/backends/container/health-probe.ts diff --git a/packages/workspace/src/backends/container/cloudflare-container.ts b/packages/workspace/src/backends/container/cloudflare-container.ts index 40afcbcf..f8f8aebb 100644 --- a/packages/workspace/src/backends/container/cloudflare-container.ts +++ b/packages/workspace/src/backends/container/cloudflare-container.ts @@ -50,6 +50,7 @@ import { newWebSocketRpcSession, type RpcStub } from "capnweb"; import type { BackendHandle, WorkspaceBackend } from "../../backend.js"; import { startHeartbeat } from "../../heartbeat.js"; import type { IWorkspaceContainerAPI, WorkspaceRef } from "./container-host.js"; +import { probeWsdHealth } from "./health-probe.js"; // What the backend's `container` factory returns: anything with // a getWorkspaceContainer() method — the shape withWorkspaceContainer @@ -293,10 +294,11 @@ export class CloudflareContainerBackend implements WorkspaceBackend { let lastError: unknown; while (Date.now() < deadline) { try { - const res = await host.fetchPort(this.#options.containerPort, "http://container/health", { - method: "HEAD", + await probeWsdHealth(host, { + port: this.#options.containerPort, + path: "/health", + timeoutMs: Math.min(2_000, Math.max(250, deadline - Date.now())), }); - void res.body?.cancel(); return; } catch (error) { lastError = error; diff --git a/packages/workspace/src/backends/container/health-probe.test.ts b/packages/workspace/src/backends/container/health-probe.test.ts new file mode 100644 index 00000000..3afed7b9 --- /dev/null +++ b/packages/workspace/src/backends/container/health-probe.test.ts @@ -0,0 +1,68 @@ +import { describe, expect, test, vi } from "vitest"; + +import type { IWorkspaceContainerAPI } from "./container-host.js"; +import { probeWsdHealth } from "./health-probe.js"; + +function fakeHost( + handler: (port: number, input: RequestInfo | URL, init?: RequestInit) => Promise, +): IWorkspaceContainerAPI { + return { + start: vi.fn(async () => {}), + interceptOutboundHttp: vi.fn(async () => {}), + fetchPort: vi.fn(handler), + port: vi.fn(() => { + throw new Error("port() not used in these tests"); + }), + } as unknown as IWorkspaceContainerAPI; +} + +describe("probeWsdHealth", () => { + test("resolves on a 2xx response", async () => { + const host = fakeHost(async () => new Response(null, { status: 200 })); + await expect( + probeWsdHealth(host, { port: 8080, path: "/health", timeoutMs: 1_000 }), + ).resolves.toBeUndefined(); + }); + + test("issues a HEAD request to the configured path", async () => { + const calls: { port: number; url: string; method?: string }[] = []; + const host = fakeHost(async (port, input, init) => { + const req = input instanceof Request ? input : new Request(input, init); + calls.push({ port, url: req.url, method: req.method }); + return new Response(null, { status: 200 }); + }); + await probeWsdHealth(host, { port: 9090, path: "/__wsd/info", timeoutMs: 1_000 }); + expect(calls).toEqual([{ port: 9090, url: "http://container/__wsd/info", method: "HEAD" }]); + }); + + test("rejects on a non-2xx response with the status in the message", async () => { + const host = fakeHost(async () => new Response("bad", { status: 503 })); + await expect( + probeWsdHealth(host, { port: 8080, path: "/health", timeoutMs: 1_000 }), + ).rejects.toThrow(/503/); + }); + + test("propagates host.fetchPort rejections", async () => { + const host = fakeHost(async () => { + throw new Error("connection refused"); + }); + await expect( + probeWsdHealth(host, { port: 8080, path: "/health", timeoutMs: 1_000 }), + ).rejects.toThrow(/connection refused/); + }); + + test("aborts the request after timeoutMs", async () => { + const host = fakeHost(async (_port, _input, init) => { + // Simulate a never-responding wsd: wait until the AbortSignal + // fires, then reject with an AbortError so the helper sees the + // timeout surface as a rejection. + await new Promise((_, reject) => { + init?.signal?.addEventListener("abort", () => reject(new Error("aborted"))); + }); + return new Response(null, { status: 200 }); + }); + await expect( + probeWsdHealth(host, { port: 8080, path: "/health", timeoutMs: 20 }), + ).rejects.toThrow(/aborted|timeout/i); + }); +}); diff --git a/packages/workspace/src/backends/container/health-probe.ts b/packages/workspace/src/backends/container/health-probe.ts new file mode 100644 index 00000000..c57c6f3e --- /dev/null +++ b/packages/workspace/src/backends/container/health-probe.ts @@ -0,0 +1,46 @@ +// Shared wsd health probe. +// +// Used in two places that must agree on what "healthy" means: +// +// - CloudflareContainerBackend.connect() startup readiness, in +// place of the previous private #waitForPort loop; +// - the keep-alive alarm's lease-time check. +// +// A single HEAD against the configured healthPath on the container +// port. ctx.container.running tells the runtime the container is +// attached, not that wsd is listening; this probe closes the gap. +// +// Probe failures bubble out as rejections; callers apply their own +// backoff / budget. No retries here — keeping the helper a single +// shot lets startup and lease alarms compose it differently. + +import type { IWorkspaceContainerAPI } from "./container-host.js"; + +export interface WsdHealthProbeOptions { + // TCP port wsd listens on inside the container. + port: number; + // Path to probe. Defaults to /health at the call site; required + // here so both callers thread the same value. + path: string; + // Per-probe timeout. The helper aborts the request when it + // elapses; the host's fetchPort surfaces that as a rejection. + timeoutMs: number; +} + +export async function probeWsdHealth( + host: IWorkspaceContainerAPI, + options: WsdHealthProbeOptions, +): Promise { + const signal = AbortSignal.timeout(options.timeoutMs); + const res = await host.fetchPort(options.port, `http://container${options.path}`, { + method: "HEAD", + signal, + }); + // Drain the body so the underlying connection (if any) can be + // released. Some runtimes return a non-null body for HEAD even + // though it should be empty; cancel() is a no-op for null. + void res.body?.cancel(); + if (!res.ok) { + throw new Error(`wsd health returned ${res.status}`); + } +} From c312379a3081c29c310b48c6cb22a4c3b3d47ee9 Mon Sep 17 00:00:00 2001 From: aron <263346377+aron-cf@users.noreply.github.com> Date: Tue, 16 Jun 2026 12:09:02 +0000 Subject: [PATCH 03/11] workspace: bound startup readiness with restart attempts CloudflareContainerBackend.connect() used to trust a single host.start() and then poll the container port until the connect deadline elapsed. If wsd never came up \u2014 the container booted into a bad state, PID 1 wedged, the network stack failed \u2014 the whole connect attempt burned the budget on a dead generation and the caller had to ride out the next ready() pass to retry. Add restart() and status() to IWorkspaceContainerAPI. WorkspaceContainerAPI implements restart() as destroy() then start(); status() returns container.running for diagnostics only. The probe stays the authoritative readiness signal. connect() now runs the shared probe in a backoff loop bounded by a per-attempt budget. A failed attempt with restarts remaining calls host.restart(env) and tries again. The default is one restart; set restartAttempts to 0 to disable. Failures throw a stage-tagged error that names the stage (start, health, restart, connect, ws), the port, the attempt number, the restart count, the overall timeout, and the last underlying error. The two existing error-path tests are adjusted to match the new stage=health and stage=ws formatting; the /connect non-2xx test still matches its old substring. --- .../container/cloudflare-container.test.ts | 109 +++++++++++++- .../container/cloudflare-container.ts | 142 ++++++++++++++++-- .../src/backends/container/container-host.ts | 30 +++- 3 files changed, 261 insertions(+), 20 deletions(-) diff --git a/packages/workspace/src/backends/container/cloudflare-container.test.ts b/packages/workspace/src/backends/container/cloudflare-container.test.ts index d7a8c2ad..83498451 100644 --- a/packages/workspace/src/backends/container/cloudflare-container.test.ts +++ b/packages/workspace/src/backends/container/cloudflare-container.test.ts @@ -16,7 +16,13 @@ import type { IWorkspaceContainerAPI, WorkspaceRef } from "./container-host.js"; interface FakeHostOptions { healthy?: boolean; + // Health probe sequence: each connect() reads from the head of + // this array. true = answer 200, false = throw "connection + // refused". A single `healthy` flag still works for tests that + // don't care about transitions. + healthSequence?: boolean[]; connectStatus?: number; + restart?: () => Promise; } interface FakeHost { @@ -25,18 +31,28 @@ interface FakeHost { startEnv?: Record; interceptedHost?: string; interceptedWorkspace?: WorkspaceRef; + running: boolean; } function makeFakeHost(opts: FakeHostOptions = {}): FakeHost { - const healthy = opts.healthy ?? true; + const healthSequence = opts.healthSequence?.slice(); + const defaultHealthy = opts.healthy ?? true; const connectStatus = opts.connectStatus ?? 200; const calls: { name: string; args: unknown[] }[] = []; - const state: FakeHost = { calls } as FakeHost; + const state: FakeHost = { calls, running: false } as FakeHost; + + function nextHealthy(): boolean { + if (healthSequence && healthSequence.length > 0) { + return healthSequence.shift() ?? defaultHealthy; + } + return defaultHealthy; + } state.host = { async start(env) { calls.push({ name: "start", args: [env] }); state.startEnv = env; + state.running = true; }, async interceptOutboundHttp(host, ref) { calls.push({ name: "interceptOutboundHttp", args: [host, ref] }); @@ -48,7 +64,7 @@ function makeFakeHost(opts: FakeHostOptions = {}): FakeHost { const url = new URL(request.url); calls.push({ name: "fetchPort", args: [port, url.pathname, request.method] }); if (url.pathname === "/health") { - if (!healthy) throw new Error("connection refused"); + if (!nextHealthy()) throw new Error("connection refused"); return new Response(null, { status: 200 }); } if (url.pathname === "/connect") { @@ -62,7 +78,18 @@ function makeFakeHost(opts: FakeHostOptions = {}): FakeHost { port() { throw new Error("cross-boundary Fetchers should not be used by CloudflareContainerBackend"); }, - }; + async restart(env) { + calls.push({ name: "restart", args: [env] }); + if (opts.restart) { + await opts.restart(); + } + state.running = true; + }, + async status() { + calls.push({ name: "status", args: [] }); + return { running: state.running }; + }, + } as IWorkspaceContainerAPI; return state; } @@ -75,9 +102,10 @@ describe("CloudflareContainerBackend", () => { container: () => ({ getWorkspaceContainer: () => fake.host }), workspace: fakeWorkspace, connectTimeoutMs: 600, + restartAttempts: 0, }); - await expect(backend.connect()).rejects.toThrow(/container port 8080 did not open/); + await expect(backend.connect()).rejects.toThrow(/stage=health.*port=8080/); const names = fake.calls.map((c) => c.name); expect(names).toContain("start"); @@ -184,4 +212,75 @@ describe("CloudflareContainerBackend", () => { const res = await backend.handleFetch(new Request("http://workspace.internal/ws")); expect(res.status).toBe(426); }); + + test("connect() restarts the host when initial readiness fails and recovers", async () => { + // First attempt drains all probes as failures; restart() runs; + // the second attempt's very first probe answers healthy. + // connect() still fails at the /ws upgrade (no WebSocketPair + // under node) — the point is that readiness recovered after + // restart and we reached the /connect POST and /ws upgrade. + const fake = makeFakeHost({ + healthSequence: [ + // First attempt — enough failures to exhaust the budget. + false, + false, + false, + false, + false, + // Restart, then second attempt: first probe is healthy. + true, + ], + }); + const backend = new CloudflareContainerBackend({ + container: () => ({ getWorkspaceContainer: () => fake.host }), + workspace: fakeWorkspace, + connectTimeoutMs: 2000, + restartAttempts: 1, + }); + await expect(backend.connect()).rejects.toThrow(/stage=ws/); + const names = fake.calls.map((c) => c.name); + expect(names.filter((n) => n === "start")).toHaveLength(1); + expect(names.filter((n) => n === "restart")).toHaveLength(1); + // /connect was reached after restart succeeded. + const paths = fake.calls.filter((c) => c.name === "fetchPort").map((c) => c.args[1] as string); + expect(paths).toContain("/connect"); + }); + + test("connect() surfaces stage='health' when readiness exhausts all attempts", async () => { + const fake = makeFakeHost({ healthy: false }); + const backend = new CloudflareContainerBackend({ + container: () => ({ getWorkspaceContainer: () => fake.host }), + workspace: fakeWorkspace, + connectTimeoutMs: 800, + restartAttempts: 1, + }); + const err = await backend.connect().then( + () => undefined, + (e: Error) => e, + ); + expect(err).toBeDefined(); + const msg = String(err); + expect(msg).toMatch(/stage=health/); + expect(msg).toMatch(/attempts?=2/); + expect(msg).toMatch(/port=8080/); + // restart was attempted before giving up. + expect(fake.calls.some((c) => c.name === "restart")).toBe(true); + }); + + test("connect() reports stage='health' when restartAttempts=0 and probe never succeeds", async () => { + const fake = makeFakeHost({ healthy: false }); + const backend = new CloudflareContainerBackend({ + container: () => ({ getWorkspaceContainer: () => fake.host }), + workspace: fakeWorkspace, + connectTimeoutMs: 600, + restartAttempts: 0, + }); + const err = await backend.connect().then( + () => undefined, + (e: Error) => e, + ); + expect(String(err)).toMatch(/stage=health/); + // No restart attempt. + expect(fake.calls.some((c) => c.name === "restart")).toBe(false); + }); }); diff --git a/packages/workspace/src/backends/container/cloudflare-container.ts b/packages/workspace/src/backends/container/cloudflare-container.ts index f8f8aebb..54a34015 100644 --- a/packages/workspace/src/backends/container/cloudflare-container.ts +++ b/packages/workspace/src/backends/container/cloudflare-container.ts @@ -101,6 +101,27 @@ export interface CloudflareContainerBackendOptions { // timers warm. Default 20_000ms. Set 0 to disable. heartbeatIntervalMs?: number; + // Number of forced restart attempts after startup readiness + // fails. The first attempt runs host.start() then probes wsd; + // each restart attempt runs host.restart() then probes wsd + // again. Defaults to 1 (one restart after the initial start). + // Set 0 to disable restart on failed readiness. + restartAttempts?: number; + + // Per-probe timeout for the startup health probe. Defaults to + // 2 seconds. The shared probeWsdHealth helper aborts the request + // when it elapses; the next probe in the loop carries the + // remaining readiness budget. + healthProbeTimeoutMs?: number; + + // First retry delay after a failed startup probe. Defaults to + // 250ms. Subsequent failures double the prior delay, capped at + // healthRetryMaxDelayMs. + healthRetryInitialDelayMs?: number; + + // Maximum delay between failed startup probes. Defaults to 2s. + healthRetryMaxDelayMs?: number; + // Selector this backend is registered under in Workspace. // Defaults to "cloudflare-container"; override when the // workspace hosts more than one instance of the same backend @@ -112,6 +133,10 @@ const DEFAULT_EGRESS_HOST = "workspace.internal"; const DEFAULT_CONTAINER_PORT = 8080; const DEFAULT_CONNECT_TIMEOUT_MS = 30_000; const DEFAULT_HEARTBEAT_INTERVAL_MS = 20_000; +const DEFAULT_RESTART_ATTEMPTS = 1; +const DEFAULT_HEALTH_PROBE_TIMEOUT_MS = 2_000; +const DEFAULT_HEALTH_RETRY_INITIAL_DELAY_MS = 250; +const DEFAULT_HEALTH_RETRY_MAX_DELAY_MS = 2_000; export class CloudflareContainerBackend implements WorkspaceBackend { readonly type = "cloudflare-container"; @@ -142,6 +167,11 @@ export class CloudflareContainerBackend implements WorkspaceBackend { containerPort: options.containerPort ?? DEFAULT_CONTAINER_PORT, connectTimeoutMs: options.connectTimeoutMs ?? DEFAULT_CONNECT_TIMEOUT_MS, heartbeatIntervalMs: options.heartbeatIntervalMs ?? DEFAULT_HEARTBEAT_INTERVAL_MS, + restartAttempts: options.restartAttempts ?? DEFAULT_RESTART_ATTEMPTS, + healthProbeTimeoutMs: options.healthProbeTimeoutMs ?? DEFAULT_HEALTH_PROBE_TIMEOUT_MS, + healthRetryInitialDelayMs: + options.healthRetryInitialDelayMs ?? DEFAULT_HEALTH_RETRY_INITIAL_DELAY_MS, + healthRetryMaxDelayMs: options.healthRetryMaxDelayMs ?? DEFAULT_HEALTH_RETRY_MAX_DELAY_MS, }; } @@ -152,11 +182,12 @@ export class CloudflareContainerBackend implements WorkspaceBackend { const holder = await this.#options.container(); const host = await holder.getWorkspaceContainer(); - await host.start({ + const env = { PORT: String(this.#options.containerPort), MOUNT_POINT: "/workspace", ...this.#options.containerEnv, - }); + }; + await host.start(env); await host.interceptOutboundHttp(this.#options.egressHost, this.#options.workspace); // Arm the upgrade promise before posting /connect — wsd @@ -164,7 +195,7 @@ export class CloudflareContainerBackend implements WorkspaceBackend { // the upgrade can arrive before the POST resolves. this.#armUpgrade(); - await this.#waitForPort(host, deadline); + await this.#readyWithRestarts(host, env, deadline); await this.#postConnect(host, deadline); const ws = await this.#waitForUpgrade(deadline); @@ -290,25 +321,105 @@ export class CloudflareContainerBackend implements WorkspaceBackend { this.#rejectUpgrade = undefined; } - async #waitForPort(host: IWorkspaceContainerAPI, deadline: number): Promise { + // Drive startup readiness with bounded restart attempts. Each + // attempt runs the shared probe in a backoff loop until either + // wsd answers, the per-attempt budget elapses, or the overall + // connect deadline elapses. On a failed attempt with restarts + // remaining, run host.restart(env) and try again. + async #readyWithRestarts( + host: IWorkspaceContainerAPI, + env: Record, + deadline: number, + ): Promise { + const maxAttempts = this.#options.restartAttempts + 1; + // Split the remaining time across attempts so a failing + // first attempt doesn't starve the restart-retry. + const totalBudget = Math.max(0, deadline - Date.now()); + const perAttemptBudget = Math.max(1, Math.floor(totalBudget / maxAttempts)); + let attempt = 0; + let restarts = 0; + let lastError: unknown; + + while (attempt < maxAttempts) { + attempt++; + const attemptDeadline = Math.min(deadline, Date.now() + perAttemptBudget); + const ok = await this.#probeUntilHealthy(host, attemptDeadline).then( + () => true, + (error) => { + lastError = error; + return false; + }, + ); + if (ok) return; + + if (attempt < maxAttempts) { + try { + await host.restart(env); + restarts++; + } catch (error) { + this.#rejectUpgrade?.(error); + this.#clearUpgrade(); + throw new Error( + this.#formatStageError("restart", { + attempt, + maxAttempts, + restarts, + lastError: error, + }), + { cause: error }, + ); + } + } + } + + this.#rejectUpgrade?.(new Error("wsd never became healthy")); + this.#clearUpgrade(); + throw new Error( + this.#formatStageError("health", { + attempt, + maxAttempts, + restarts, + lastError, + }), + lastError instanceof Error ? { cause: lastError } : undefined, + ); + } + + async #probeUntilHealthy(host: IWorkspaceContainerAPI, deadline: number): Promise { + let delay = this.#options.healthRetryInitialDelayMs; let lastError: unknown; while (Date.now() < deadline) { try { await probeWsdHealth(host, { port: this.#options.containerPort, path: "/health", - timeoutMs: Math.min(2_000, Math.max(250, deadline - Date.now())), + timeoutMs: Math.min( + this.#options.healthProbeTimeoutMs, + Math.max(50, deadline - Date.now()), + ), }); return; } catch (error) { lastError = error; - await sleep(250); + const remaining = deadline - Date.now(); + if (remaining <= 0) break; + await sleep(Math.min(delay, remaining)); + delay = Math.min(delay * 2, this.#options.healthRetryMaxDelayMs); } } - this.#rejectUpgrade?.(new Error("port did not open")); - this.#clearUpgrade(); - throw new Error( - `CloudflareContainerBackend: container port ${this.#options.containerPort} did not open: ${describeError(lastError)}`, + throw lastError ?? new Error("wsd health probe timed out"); + } + + #formatStageError( + stage: "start" | "health" | "restart" | "connect" | "ws", + info: { attempt: number; maxAttempts: number; restarts: number; lastError: unknown }, + ): string { + return ( + `CloudflareContainerBackend(${this.id}): connect failed at ` + + `stage=${stage} port=${this.#options.containerPort} ` + + `attempt=${info.attempt}/${info.maxAttempts} restarts=${info.restarts} ` + + `timeoutMs=${this.#options.connectTimeoutMs} ` + + `lastError=${describeError(info.lastError)}` ); } @@ -327,13 +438,18 @@ export class CloudflareContainerBackend implements WorkspaceBackend { } catch (error) { this.#rejectUpgrade?.(error); this.#clearUpgrade(); - throw new Error(`CloudflareContainerBackend: POST /connect failed: ${describeError(error)}`); + throw new Error( + `CloudflareContainerBackend(${this.id}) [stage=connect]: POST /connect failed: ${describeError(error)}`, + { cause: error }, + ); } if (!res.ok) { const body = await res.text().catch(() => ""); this.#rejectUpgrade?.(new Error(`/connect ${res.status}`)); this.#clearUpgrade(); - throw new Error(`CloudflareContainerBackend: POST /connect returned ${res.status}: ${body}`); + throw new Error( + `CloudflareContainerBackend(${this.id}) [stage=connect]: POST /connect returned ${res.status}: ${body}`, + ); } } @@ -351,7 +467,7 @@ export class CloudflareContainerBackend implements WorkspaceBackend { () => reject( new Error( - `CloudflareContainerBackend: /ws upgrade did not arrive within ${this.#options.connectTimeoutMs}ms`, + `CloudflareContainerBackend(${this.id}) [stage=ws]: /ws upgrade did not arrive within ${this.#options.connectTimeoutMs}ms`, ), ), remaining, diff --git a/packages/workspace/src/backends/container/container-host.ts b/packages/workspace/src/backends/container/container-host.ts index 5c160801..dedda566 100644 --- a/packages/workspace/src/backends/container/container-host.ts +++ b/packages/workspace/src/backends/container/container-host.ts @@ -35,8 +35,8 @@ export interface WorkspaceRef { // the `ws` accessor that withWorkspaceContainer installs. export interface IWorkspaceContainerAPI { // Idempotent start. Returns once the runtime has accepted the - // start command; readiness is verified by the backend polling - // /health via port(). + // start command; readiness is verified by the backend through + // probeWsdHealth against port(). start(env: Record): Promise; // Wire `host` → workspace inside the container's egress table. @@ -53,6 +53,20 @@ export interface IWorkspaceContainerAPI { // Return a Fetcher bound to the named TCP port inside the // container for same-isolate callers and advanced integrations. port(port: number): Fetcher; + + // Force a fresh container generation when startup readiness + // never opens or a lease-time health check has declared the + // current generation dead. Implementation: destroy() the + // container, then start({ env }). Callers bound the number of + // restart attempts — this method does no looping of its own. + restart(env: Record): Promise; + + // Coarse diagnostic state. The `running` flag reports whether + // the platform still has a container instance attached; it does + // not prove that wsd is listening or responsive. Use + // probeWsdHealth for readiness; use status() only for logs and + // tracing. + status(): Promise<{ running: boolean }>; } // Concrete implementation. Extends RpcTarget so it travels intact @@ -77,6 +91,18 @@ export class WorkspaceContainerAPI extends RpcTarget implements IWorkspaceContai this.#container.start({ enableInternet: true, env }); } + async restart(env: Record) { + // destroy() resolves once the platform has torn down the + // attached container. A subsequent start() launches a fresh + // generation — ports re-bind, the wsd daemon comes up clean. + await this.#container.destroy(); + this.#container.start({ enableInternet: true, env }); + } + + async status() { + return { running: this.#container.running }; + } + async interceptOutboundHttp(host: string, ref: WorkspaceRef) { // ctx.exports.WorkspaceProxy is bound by name in the // consumer's Worker (they re-export WorkspaceProxy from this From 9e35d0dff6592e4b2fddea4d1403c8917b56c2d1 Mon Sep 17 00:00:00 2001 From: aron <263346377+aron-cf@users.noreply.github.com> Date: Tue, 16 Jun 2026 14:13:31 +0000 Subject: [PATCH 04/11] workspace: react to container exits through monitor() The container backend used to find out that its container had died only when the next operation failed against it. The runtime already exposes container.monitor() \u2014 a promise that resolves when the container exits, with the rejection carrying the abnormal-exit reason. Wiring it up turns a stale-handle stumble into a fast, classifiable failure. Add a small container-lifecycle module that owns the per-DO monitor state in a module-level WeakMap keyed by ctx. The state records the most recent exit (timestamp + reason) and tracks an expectingExit flag so an intentional teardown driven by WorkspaceContainerAPI.restart() does not log as a crash. The helpers are pure and have no cloudflare:workers imports, so they run under the node-based vitest runner against an in-process container fake. WorkspaceContainerAPI arms the monitor on every successful start() and uses destroyContainerExpectingExit() in restart() so the teardown logs cleanly. fetchPort() short-circuits with a WorkspaceTransportError when an exit has been recorded; the transport-failure classifier picks that up and the Workspace drops its cached handle so the next operation reconnects against a fresh generation. The interface gains exitInfo(); status() grows an exit field for diagnostics. CloudflareContainerBackend.connect() consults host.exitInfo() before host.start(). When readiness fails, the stage-tagged error carries the prior exit reason so the resulting log line attributes the failure to the crash that preceded it. Exit lines land in Cloudflare Logs via a single structured console.{warn,info} call so the workers logging stack picks up the fields. console.warn for unexpected exits (crash, OOM), console.info for the expected exits driven by restart(). --- .../container/cloudflare-container.test.ts | 63 +++- .../container/cloudflare-container.ts | 23 +- .../src/backends/container/container-host.ts | 67 ++++- .../container/container-lifecycle.test.ts | 277 ++++++++++++++++++ .../backends/container/container-lifecycle.ts | 129 ++++++++ 5 files changed, 548 insertions(+), 11 deletions(-) create mode 100644 packages/workspace/src/backends/container/container-lifecycle.test.ts create mode 100644 packages/workspace/src/backends/container/container-lifecycle.ts diff --git a/packages/workspace/src/backends/container/cloudflare-container.test.ts b/packages/workspace/src/backends/container/cloudflare-container.test.ts index 83498451..eee12800 100644 --- a/packages/workspace/src/backends/container/cloudflare-container.test.ts +++ b/packages/workspace/src/backends/container/cloudflare-container.test.ts @@ -23,6 +23,9 @@ interface FakeHostOptions { healthSequence?: boolean[]; connectStatus?: number; restart?: () => Promise; + // Pre-set a prior exit reason so connect()'s pre-flight + // exitInfo() check observes it. + priorExit?: { exitedAt: number; reason: string } | null; } interface FakeHost { @@ -32,6 +35,8 @@ interface FakeHost { interceptedHost?: string; interceptedWorkspace?: WorkspaceRef; running: boolean; + exit: { exitedAt: number; reason: string } | null; + simulateExit(reason: string): void; } function makeFakeHost(opts: FakeHostOptions = {}): FakeHost { @@ -39,7 +44,15 @@ function makeFakeHost(opts: FakeHostOptions = {}): FakeHost { const defaultHealthy = opts.healthy ?? true; const connectStatus = opts.connectStatus ?? 200; const calls: { name: string; args: unknown[] }[] = []; - const state: FakeHost = { calls, running: false } as FakeHost; + const state: FakeHost = { + calls, + running: false, + exit: opts.priorExit ?? null, + simulateExit(reason: string) { + state.exit = { exitedAt: Date.now(), reason }; + state.running = false; + }, + } as FakeHost; function nextHealthy(): boolean { if (healthSequence && healthSequence.length > 0) { @@ -53,6 +66,9 @@ function makeFakeHost(opts: FakeHostOptions = {}): FakeHost { calls.push({ name: "start", args: [env] }); state.startEnv = env; state.running = true; + // A successful start clears any prior exit, matching + // WorkspaceContainerAPI.start. + state.exit = null; }, async interceptOutboundHttp(host, ref) { calls.push({ name: "interceptOutboundHttp", args: [host, ref] }); @@ -84,10 +100,15 @@ function makeFakeHost(opts: FakeHostOptions = {}): FakeHost { await opts.restart(); } state.running = true; + state.exit = null; }, async status() { calls.push({ name: "status", args: [] }); - return { running: state.running }; + return { running: state.running, exit: state.exit }; + }, + async exitInfo() { + calls.push({ name: "exitInfo", args: [] }); + return state.exit; }, } as IWorkspaceContainerAPI; return state; @@ -213,6 +234,44 @@ describe("CloudflareContainerBackend", () => { expect(res.status).toBe(426); }); + test("connect() consults host.exitInfo() before host.start()", async () => { + const fake = makeFakeHost(); + const backend = new CloudflareContainerBackend({ + container: () => ({ getWorkspaceContainer: () => fake.host }), + workspace: fakeWorkspace, + connectTimeoutMs: 600, + restartAttempts: 0, + }); + // Doesn't matter that this rejects — we just want to observe + // the call order. + await backend.connect().catch(() => undefined); + const names = fake.calls.map((c) => c.name); + const exitIdx = names.indexOf("exitInfo"); + const startIdx = names.indexOf("start"); + expect(exitIdx).toBeGreaterThanOrEqual(0); + expect(startIdx).toBeGreaterThanOrEqual(0); + expect(exitIdx).toBeLessThan(startIdx); + }); + + test("connect() surfaces a prior exit reason in the stage-tagged error", async () => { + const fake = makeFakeHost({ + healthy: false, + priorExit: { exitedAt: Date.now() - 5_000, reason: "OOM killed" }, + }); + const backend = new CloudflareContainerBackend({ + container: () => ({ getWorkspaceContainer: () => fake.host }), + workspace: fakeWorkspace, + connectTimeoutMs: 600, + restartAttempts: 0, + }); + const err = await backend.connect().then( + () => undefined, + (e: Error) => e, + ); + expect(String(err)).toMatch(/stage=health/); + expect(String(err)).toMatch(/priorExit="OOM killed"/); + }); + test("connect() restarts the host when initial readiness fails and recovers", async () => { // First attempt drains all probes as failures; restart() runs; // the second attempt's very first probe answers healthy. diff --git a/packages/workspace/src/backends/container/cloudflare-container.ts b/packages/workspace/src/backends/container/cloudflare-container.ts index 54a34015..630f6be5 100644 --- a/packages/workspace/src/backends/container/cloudflare-container.ts +++ b/packages/workspace/src/backends/container/cloudflare-container.ts @@ -182,6 +182,14 @@ export class CloudflareContainerBackend implements WorkspaceBackend { const holder = await this.#options.container(); const host = await holder.getWorkspaceContainer(); + // Pre-flight: surface any prior container exit so a readiness + // failure can attribute it back to the crash. host.start() + // clears this once the new generation is up, so a successful + // dial against a previously-dead container loses the + // attribution — which is the right semantics; the prior exit + // is only interesting if the new attempt fails too. + const priorExit = await host.exitInfo().catch(() => null); + const env = { PORT: String(this.#options.containerPort), MOUNT_POINT: "/workspace", @@ -195,7 +203,7 @@ export class CloudflareContainerBackend implements WorkspaceBackend { // the upgrade can arrive before the POST resolves. this.#armUpgrade(); - await this.#readyWithRestarts(host, env, deadline); + await this.#readyWithRestarts(host, env, deadline, priorExit); await this.#postConnect(host, deadline); const ws = await this.#waitForUpgrade(deadline); @@ -330,6 +338,7 @@ export class CloudflareContainerBackend implements WorkspaceBackend { host: IWorkspaceContainerAPI, env: Record, deadline: number, + priorExit: { exitedAt: number; reason: string } | null, ): Promise { const maxAttempts = this.#options.restartAttempts + 1; // Split the remaining time across attempts so a failing @@ -380,6 +389,7 @@ export class CloudflareContainerBackend implements WorkspaceBackend { maxAttempts, restarts, lastError, + priorExit, }), lastError instanceof Error ? { cause: lastError } : undefined, ); @@ -412,13 +422,20 @@ export class CloudflareContainerBackend implements WorkspaceBackend { #formatStageError( stage: "start" | "health" | "restart" | "connect" | "ws", - info: { attempt: number; maxAttempts: number; restarts: number; lastError: unknown }, + info: { + attempt: number; + maxAttempts: number; + restarts: number; + lastError: unknown; + priorExit?: { exitedAt: number; reason: string } | null; + }, ): string { + const priorExit = info.priorExit ? ` priorExit=${JSON.stringify(info.priorExit.reason)}` : ""; return ( `CloudflareContainerBackend(${this.id}): connect failed at ` + `stage=${stage} port=${this.#options.containerPort} ` + `attempt=${info.attempt}/${info.maxAttempts} restarts=${info.restarts} ` + - `timeoutMs=${this.#options.connectTimeoutMs} ` + + `timeoutMs=${this.#options.connectTimeoutMs}${priorExit} ` + `lastError=${describeError(info.lastError)}` ); } diff --git a/packages/workspace/src/backends/container/container-host.ts b/packages/workspace/src/backends/container/container-host.ts index dedda566..3e2a21d6 100644 --- a/packages/workspace/src/backends/container/container-host.ts +++ b/packages/workspace/src/backends/container/container-host.ts @@ -19,6 +19,15 @@ // an RpcTarget so it works the same in-isolate and across RPC. import { RpcTarget } from "cloudflare:workers"; +import { WorkspaceTransportError } from "../../transport-failure.js"; +import { + armContainerMonitor, + type ContainerExitInfo, + containerExitInfo, + destroyContainerExpectingExit, +} from "./container-lifecycle.js"; + +export type { ContainerExitInfo } from "./container-lifecycle.js"; // Identifies the Durable Object that owns the Workspace and answers // the /ws upgrade. Plain data so it can travel over Workers RPC. @@ -65,8 +74,17 @@ export interface IWorkspaceContainerAPI { // the platform still has a container instance attached; it does // not prove that wsd is listening or responsive. Use // probeWsdHealth for readiness; use status() only for logs and - // tracing. - status(): Promise<{ running: boolean }>; + // tracing. `exit` is populated from the in-memory monitor() + // signal when the most recent container generation exited; it + // resets on the next successful start(). + status(): Promise<{ running: boolean; exit: ContainerExitInfo | null }>; + + // Snapshot of the last container exit reason observed through + // monitor(). Null while the current generation is alive (or + // before any container has been started). Used by the backend + // pre-flight check in connect() to attribute readiness failures + // to the prior generation's exit when one is available. + exitInfo(): Promise; } // Concrete implementation. Extends RpcTarget so it travels intact @@ -87,20 +105,48 @@ export class WorkspaceContainerAPI extends RpcTarget implements IWorkspaceContai } async start(env: Record) { - if (this.#container.running) return; - this.#container.start({ enableInternet: true, env }); + // If the previous generation died but the runtime still has a + // container attached, treat the prior exit as a signal to wipe + // the carcass before re-starting. The new generation gets a + // clean monitor(). + const prior = containerExitInfo(this.#ctx); + if (prior !== null) { + try { + await destroyContainerExpectingExit(this.#ctx, this.#container); + } catch { + // best-effort — the next start() will surface any real + // platform-side failure. + } + } + if (!this.#container.running) { + this.#container.start({ enableInternet: true, env }); + } + armContainerMonitor(this.#ctx, this.#container); } async restart(env: Record) { // destroy() resolves once the platform has torn down the // attached container. A subsequent start() launches a fresh // generation — ports re-bind, the wsd daemon comes up clean. - await this.#container.destroy(); + // destroyContainerExpectingExit flips the lifecycle flag so + // the monitor handler logs the exit as intentional. + try { + await destroyContainerExpectingExit(this.#ctx, this.#container); + } catch { + // tolerate a flaky destroy — start() below will either + // succeed against a fresh generation or surface its own + // failure. + } this.#container.start({ enableInternet: true, env }); + armContainerMonitor(this.#ctx, this.#container); } async status() { - return { running: this.#container.running }; + return { running: this.#container.running, exit: containerExitInfo(this.#ctx) }; + } + + async exitInfo(): Promise { + return containerExitInfo(this.#ctx); } async interceptOutboundHttp(host: string, ref: WorkspaceRef) { @@ -117,6 +163,15 @@ export class WorkspaceContainerAPI extends RpcTarget implements IWorkspaceContai } fetchPort(port: number, input: RequestInfo | URL, init?: RequestInit): Promise { + // Short-circuit if the container is known to have exited. + // WorkspaceTransportError is classified by + // isWorkspaceTransportFailure, so the Workspace cache drops + // the stale handle and the next operation reconnects against + // a fresh generation. + const exit = containerExitInfo(this.#ctx); + if (exit !== null) { + throw new WorkspaceTransportError(`container exited: ${exit.reason}`); + } return this.#container.getTcpPort(port).fetch(input, init); } diff --git a/packages/workspace/src/backends/container/container-lifecycle.test.ts b/packages/workspace/src/backends/container/container-lifecycle.test.ts new file mode 100644 index 00000000..b9e3f38d --- /dev/null +++ b/packages/workspace/src/backends/container/container-lifecycle.test.ts @@ -0,0 +1,277 @@ +import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; + +import { + armContainerMonitor, + containerExitInfo, + destroyContainerExpectingExit, + formatExitReason, + getContainerLifecycle, + resetContainerLifecycleForTests, +} from "./container-lifecycle.js"; + +// Minimal Container stand-in. The lifecycle module only touches +// .destroy(), .start(), .running, and .monitor(). A controllable +// monitor() promise lets the tests drive the exit signal +// deterministically. +function makeContainer(): { + container: NonNullable; + starts: number; + destroys: number; + monitorCalls: number; + resolveMonitor: () => void; + rejectMonitor: (error: unknown) => void; + monitorPromise: () => Promise; +} { + let starts = 0; + let destroys = 0; + let monitorCalls = 0; + let running = false; + let resolveMonitor!: () => void; + let rejectMonitor!: (error: unknown) => void; + let monitorPromise: Promise; + + function armPromise() { + monitorPromise = new Promise((resolve, reject) => { + resolveMonitor = resolve; + rejectMonitor = reject; + }); + // Swallow unhandled rejection noise when the test path leaves + // the promise pending or rejects without awaiting it. + monitorPromise.catch(() => {}); + } + armPromise(); + + const container = { + get running() { + return running; + }, + start(_options?: unknown) { + starts++; + running = true; + // Each start() arms a fresh monitor() that the next monitor() + // call returns. + armPromise(); + }, + async destroy() { + destroys++; + running = false; + // destroy() resolves the in-flight monitor() cleanly. + resolveMonitor(); + }, + monitor() { + monitorCalls++; + return monitorPromise; + }, + } as unknown as NonNullable; + + return { + container, + get starts() { + return starts; + }, + get destroys() { + return destroys; + }, + get monitorCalls() { + return monitorCalls; + }, + resolveMonitor: () => resolveMonitor(), + rejectMonitor: (error: unknown) => rejectMonitor(error), + monitorPromise: () => monitorPromise, + } as ReturnType; +} + +// Lifecycle state is keyed by ctx — a tiny opaque object suffices. +function makeCtx(container: NonNullable): DurableObjectState { + return { container } as unknown as DurableObjectState; +} + +beforeEach(() => { + vi.useFakeTimers(); + vi.setSystemTime(new Date("2026-01-01T00:00:00Z")); +}); + +afterEach(() => { + vi.useRealTimers(); + vi.restoreAllMocks(); +}); + +describe("formatExitReason", () => { + test("returns 'exited normally' for resolve case (no error)", () => { + expect(formatExitReason(undefined)).toBe("exited normally"); + }); + + test("returns the Error message for an Error", () => { + expect(formatExitReason(new Error("OOM killed"))).toBe("OOM killed"); + }); + + test("falls back to String() for non-Error rejections", () => { + expect(formatExitReason(42)).toBe("42"); + }); +}); + +describe("armContainerMonitor", () => { + test("records exit info when the monitor resolves", async () => { + const fake = makeContainer(); + const ctx = makeCtx(fake.container); + resetContainerLifecycleForTests(ctx); + fake.container.start(); + armContainerMonitor(ctx, fake.container); + + expect(containerExitInfo(ctx)).toBeNull(); + fake.resolveMonitor(); + // Microtask drain. + await Promise.resolve(); + await Promise.resolve(); + + const exit = containerExitInfo(ctx); + expect(exit).not.toBeNull(); + expect(exit?.reason).toBe("exited normally"); + expect(exit?.exitedAt).toBe(Date.now()); + }); + + test("records the rejection reason when the monitor rejects", async () => { + const fake = makeContainer(); + const ctx = makeCtx(fake.container); + resetContainerLifecycleForTests(ctx); + fake.container.start(); + armContainerMonitor(ctx, fake.container); + + fake.rejectMonitor(new Error("container crashed")); + await Promise.resolve(); + await Promise.resolve(); + + const exit = containerExitInfo(ctx); + expect(exit?.reason).toBe("container crashed"); + }); + + test("logs at warn level on an unexpected exit", async () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + const fake = makeContainer(); + const ctx = makeCtx(fake.container); + resetContainerLifecycleForTests(ctx); + fake.container.start(); + armContainerMonitor(ctx, fake.container); + + fake.rejectMonitor(new Error("OOM killed")); + await Promise.resolve(); + await Promise.resolve(); + + expect(warn).toHaveBeenCalledTimes(1); + const [arg] = warn.mock.calls[0] ?? []; + expect(arg).toMatchObject({ + message: "workspace.container.exited", + reason: "OOM killed", + expected: false, + }); + }); + + test("logs at info level when the exit was expected (after destroyContainerExpectingExit)", async () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + const info = vi.spyOn(console, "info").mockImplementation(() => {}); + const fake = makeContainer(); + const ctx = makeCtx(fake.container); + resetContainerLifecycleForTests(ctx); + fake.container.start(); + armContainerMonitor(ctx, fake.container); + + await destroyContainerExpectingExit(ctx, fake.container); + // Drain the monitor's then-chain. + await Promise.resolve(); + await Promise.resolve(); + + expect(warn).not.toHaveBeenCalled(); + expect(info).toHaveBeenCalledTimes(1); + const [arg] = info.mock.calls[0] ?? []; + expect(arg).toMatchObject({ + message: "workspace.container.exited", + expected: true, + }); + }); + + test("does not arm twice for the same container generation", () => { + const fake = makeContainer(); + const ctx = makeCtx(fake.container); + resetContainerLifecycleForTests(ctx); + fake.container.start(); + armContainerMonitor(ctx, fake.container); + armContainerMonitor(ctx, fake.container); + expect(fake.monitorCalls).toBe(1); + }); +}); + +describe("destroyContainerExpectingExit", () => { + test("clears the expectingExit flag after destroy() resolves", async () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + const fake = makeContainer(); + const ctx = makeCtx(fake.container); + resetContainerLifecycleForTests(ctx); + fake.container.start(); + armContainerMonitor(ctx, fake.container); + + await destroyContainerExpectingExit(ctx, fake.container); + await Promise.resolve(); + await Promise.resolve(); + // Arm a fresh monitor for a new container generation. + fake.container.start(); + armContainerMonitor(ctx, fake.container); + fake.rejectMonitor(new Error("real crash")); + await Promise.resolve(); + await Promise.resolve(); + + // The second exit was unexpected; it must log as a crash. + expect(warn).toHaveBeenCalledTimes(1); + }); + + test("clears the expectingExit flag even if destroy() throws", async () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + const fake = makeContainer(); + const ctx = makeCtx(fake.container); + resetContainerLifecycleForTests(ctx); + fake.container.start(); + armContainerMonitor(ctx, fake.container); + const broken = { + destroy: async () => { + throw new Error("destroy rejected"); + }, + } as unknown as NonNullable; + + await expect(destroyContainerExpectingExit(ctx, broken)).rejects.toThrow(/destroy rejected/); + + // A later real crash must still log as unexpected. + fake.rejectMonitor(new Error("real crash")); + await Promise.resolve(); + await Promise.resolve(); + expect(warn).toHaveBeenCalledTimes(1); + }); +}); + +describe("getContainerLifecycle", () => { + test("returns null exit info before any monitor has fired", () => { + const fake = makeContainer(); + const ctx = makeCtx(fake.container); + resetContainerLifecycleForTests(ctx); + expect(containerExitInfo(ctx)).toBeNull(); + expect(getContainerLifecycle(ctx).exit).toBeNull(); + }); + + test("isolates state per ctx via the WeakMap", async () => { + const a = makeContainer(); + const b = makeContainer(); + const ctxA = makeCtx(a.container); + const ctxB = makeCtx(b.container); + resetContainerLifecycleForTests(ctxA); + resetContainerLifecycleForTests(ctxB); + a.container.start(); + armContainerMonitor(ctxA, a.container); + b.container.start(); + armContainerMonitor(ctxB, b.container); + + a.rejectMonitor(new Error("a crashed")); + await Promise.resolve(); + await Promise.resolve(); + + expect(containerExitInfo(ctxA)?.reason).toBe("a crashed"); + expect(containerExitInfo(ctxB)).toBeNull(); + }); +}); diff --git a/packages/workspace/src/backends/container/container-lifecycle.ts b/packages/workspace/src/backends/container/container-lifecycle.ts new file mode 100644 index 00000000..01b7330f --- /dev/null +++ b/packages/workspace/src/backends/container/container-lifecycle.ts @@ -0,0 +1,129 @@ +// Container lifecycle helpers. +// +// `WorkspaceContainerAPI` is constructed fresh on every +// getWorkspaceContainer() call, so instance fields can't track +// monitor state across calls. The state lives in a module-level +// WeakMap keyed by the owning DO's ctx; each DO gets one slot, +// garbage-collected when the DO is reclaimed. +// +// The lifecycle helpers here are independent of any +// cloudflare:workers imports so they can be unit-tested under the +// node-based vitest runner. + +type ContainerHandle = NonNullable; + +export interface ContainerExitInfo { + exitedAt: number; + reason: string; +} + +interface ContainerLifecycleState { + // Populated when the in-flight monitor() resolves or rejects. + exit: ContainerExitInfo | null; + // The monitor() promise we attached to the current container + // generation. null between generations and before the first + // start(). + monitorArmed: Container | null; + // Set just before our own destroy() so the monitor handler + // records the exit as intentional and logs at info, not warn. + expectingExit: boolean; +} + +// Wrapper type so the test stand-in can be plain object literals +// without needing the full Container interface to type-check. +type Container = ContainerHandle; + +const LIFECYCLE = new WeakMap(); + +export function getContainerLifecycle(ctx: DurableObjectState): ContainerLifecycleState { + let state = LIFECYCLE.get(ctx); + if (state === undefined) { + state = { exit: null, monitorArmed: null, expectingExit: false }; + LIFECYCLE.set(ctx, state); + } + return state; +} + +export function containerExitInfo(ctx: DurableObjectState): ContainerExitInfo | null { + return getContainerLifecycle(ctx).exit; +} + +export function formatExitReason(error: unknown): string { + if (error === undefined) return "exited normally"; + if (error instanceof Error) return error.message; + return String(error); +} + +// Arm a monitor() for the currently-attached container generation. +// Idempotent: a second call for the same generation is a no-op so +// callers can invoke this on every start() without double-attaching. +// +// The monitor handler records exit info, logs once, and clears the +// armed reference so the next start() can re-arm. Both branches +// (resolve, reject) feed into the same recorder. +export function armContainerMonitor(ctx: DurableObjectState, container: Container): void { + const state = getContainerLifecycle(ctx); + if (state.monitorArmed === container) return; + state.monitorArmed = container; + // Clear any prior exit info — a fresh generation has started. + state.exit = null; + + const promise = container.monitor(); + promise.then( + () => recordExit(state, undefined), + (error) => recordExit(state, error), + ); +} + +// Tear down the current container generation. Sets expectingExit +// so the monitor handler logs the resulting exit as intentional. +// Clears the flag in a finally so a failing destroy() does not +// leave a stale flag that would mis-classify a later real crash. +export async function destroyContainerExpectingExit( + ctx: DurableObjectState, + container: Container, +): Promise { + const state = getContainerLifecycle(ctx); + state.expectingExit = true; + try { + await container.destroy(); + } finally { + // The monitor handler observes expectingExit synchronously the + // moment destroy() resolves the monitor promise. Clearing here + // races the handler — but the handler has already snapshotted + // the flag by the time it runs (recordExit reads expectingExit + // first), so clearing now is safe. + state.expectingExit = false; + state.monitorArmed = null; + } +} + +// Reset state for a ctx. Test-only escape hatch; the production +// path relies on WeakMap GC. +export function resetContainerLifecycleForTests(ctx: DurableObjectState): void { + LIFECYCLE.delete(ctx); +} + +function recordExit(state: ContainerLifecycleState, error: unknown): void { + const expected = state.expectingExit; + const reason = formatExitReason(error); + const exitedAt = Date.now(); + state.exit = { reason, exitedAt }; + // Log a single line per exit. Single-object form so Cloudflare + // Logs picks up the structured fields alongside the message. + if (expected) { + console.info({ + message: "workspace.container.exited", + reason, + exitedAt, + expected: true, + }); + } else { + console.warn({ + message: "workspace.container.exited", + reason, + exitedAt, + expected: false, + }); + } +} From 68bb645b60ba55fd28935f59e699cf28863c42cd Mon Sep 17 00:00:00 2001 From: aron <263346377+aron-cf@users.noreply.github.com> Date: Tue, 16 Jun 2026 18:12:47 +0000 Subject: [PATCH 05/11] workspace: classify transport failures by .name and add capnweb patterns WorkspaceTransportError extended Error and set a name field that nothing read. After a Workers RPC structured-clone hop the subclass identity is dropped, so a cross-DO caller's instanceof check returns false and the error escapes classification. Read .name in the classifier loop \u2014 it survives the hop intact \u2014 so a cross-DO WorkspaceTransportError still gets recognized as a transport failure and the cached handle is invalidated. Two pattern fixes follow from grepping node_modules/capnweb: - replace /rpc session was closed/i (matches no capnweb output) with /rpc stub after it has been disposed/i (matches the actual post-shutdown message); - add /container exited/i so the container-host fetchPort short-circuit classifies on the message alone, even in the pathological case where neither name nor instanceof survives. --- .../workspace/src/transport-failure.test.ts | 24 ++++++++++++++++++- packages/workspace/src/transport-failure.ts | 20 ++++++++++++++-- 2 files changed, 41 insertions(+), 3 deletions(-) diff --git a/packages/workspace/src/transport-failure.test.ts b/packages/workspace/src/transport-failure.test.ts index 287db462..aa175a5b 100644 --- a/packages/workspace/src/transport-failure.test.ts +++ b/packages/workspace/src/transport-failure.test.ts @@ -11,7 +11,13 @@ describe("isWorkspaceTransportFailure", () => { expect( isWorkspaceTransportFailure(new Error("RPC was canceled because RPC session was shut down")), ).toBe(true); - expect(isWorkspaceTransportFailure(new Error("the RPC session was closed"))).toBe(true); + // capnweb's actual post-shutdown error (verified against + // node_modules/capnweb/dist/index.js). + expect( + isWorkspaceTransportFailure( + new Error("Attempted to use RPC stub after it has been disposed."), + ), + ).toBe(true); }); it("recognises WebSocket transport failures", () => { @@ -44,6 +50,22 @@ describe("isWorkspaceTransportFailure", () => { expect(isWorkspaceTransportFailure(new Error("permission denied"))).toBe(false); }); + it("recognises WorkspaceTransportError by name when subclass identity is lost", () => { + // Simulates what a Workers RPC structured-clone hop produces: + // the original WorkspaceTransportError lands on the receiving + // side as a plain Error whose name field survived but whose + // class identity did not. + const cloned = new Error("container exited: OOM killed"); + cloned.name = "WorkspaceTransportError"; + expect(isWorkspaceTransportFailure(cloned)).toBe(true); + }); + + it("recognises the container-exited short-circuit message", () => { + // Even when the name is lost (or the error is reconstructed + // without it), the message pattern still classifies. + expect(isWorkspaceTransportFailure(new Error("container exited: SIGKILL"))).toBe(true); + }); + it("returns false for non-Error values", () => { expect(isWorkspaceTransportFailure(undefined)).toBe(false); expect(isWorkspaceTransportFailure(null)).toBe(false); diff --git a/packages/workspace/src/transport-failure.ts b/packages/workspace/src/transport-failure.ts index 063732da..5dbe8cba 100644 --- a/packages/workspace/src/transport-failure.ts +++ b/packages/workspace/src/transport-failure.ts @@ -45,9 +45,13 @@ export class WorkspaceTransportError extends Error { // list — only phrases the workspace stack reliably produces or // that capnweb / Cloudflare Workers ws emit on a closed session. const TRANSPORT_PATTERNS: RegExp[] = [ - // capnweb session shutdown / cancellation. + // capnweb session shutdown / cancellation. Phrasing checked + // against node_modules/capnweb/dist/index.js: the + // post-shutdown error is "Attempted to use RPC stub after it + // has been disposed." and pre-shutdown cancellation reads as + // "RPC was canceled because RPC session was shut down...". /rpc session was shut down/i, - /rpc session was closed/i, + /rpc stub after it has been disposed/i, /rpc was canceled/i, // WebSocket transport failures. /websocket is not open/i, @@ -59,6 +63,14 @@ const TRANSPORT_PATTERNS: RegExp[] = [ /econnrefused/i, /econnreset/i, /network is unreachable/i, + // The container backend's fetchPort short-circuit when it has + // observed an exit through container.monitor(). Surfaces + // through a WorkspaceTransportError same-isolate (handled by + // the instanceof check below) and as a plain Error after a + // Workers RPC hop (subclass identity is dropped by structured + // clone). The .name check covers RPC-side; this pattern + // covers errors that don't even carry the original name. + /container exited/i, ]; export function isWorkspaceTransportFailure(error: unknown): boolean { @@ -67,6 +79,10 @@ export function isWorkspaceTransportFailure(error: unknown): boolean { for (let depth = 0; depth < 8 && current !== undefined && current !== null; depth++) { if (current instanceof WorkspaceTransportError) return true; if (current instanceof Error) { + // .name survives a Workers RPC structured-clone hop even + // when the subclass identity does not, so cross-DO callers + // still classify a WorkspaceTransportError correctly. + if (current.name === "WorkspaceTransportError") return true; for (const pattern of TRANSPORT_PATTERNS) { if (pattern.test(current.message)) return true; } From 5c37289a0fdc633656e65b4b2e1e35ed3376aa27 Mon Sep 17 00:00:00 2001 From: aron <263346377+aron-cf@users.noreply.github.com> Date: Tue, 16 Jun 2026 18:19:36 +0000 Subject: [PATCH 06/11] workspace: fix container exit attribution across generations Three failure modes the original lifecycle code had, all exposed when the fake monitor() inverts its settle direction to match the platform (real container.monitor() rejects on a non-zero exit; destroy() is SIGKILL, so the rejection is the common path): 1. A late-settling monitor from a torn-down generation could overwrite a fresh generation's clean exit state, causing fetchPort to short-circuit against a healthy container. 2. expectingExit was cleared in destroyContainerExpectingExit's finally block before the monitor's then-handler ran, so an intentional destroy logged at warn as if it were a crash. 3. WorkspaceContainerAPI.start() guarded the recovery path with !this.#container.running, which can lag after a destroy() resolves; the start could be skipped and a monitor armed against the carcass. Switch the lifecycle to a generation-keyed model: every arm bumps a counter and captures its generation in the handler closure; the handler bails out if its generation no longer matches the live one. destroyContainerExpectingExit writes the current generation into expectedExitGeneration instead of flipping a global boolean; the handler reads the slot when it fires and consumes the mark, so a destroy that fails and leaves the mark on a dead generation cannot mis-classify a later real crash on the new one. WorkspaceContainerAPI.start() takes the prior-exit branch unconditionally when a previous generation has died: destroy the carcass, then start a fresh generation without consulting container.running. Fake container in the lifecycle test now exposes per-generation resolve/reject controls so a test can fire the first generation's settle frame AFTER the second generation has been armed \u2014 the actual stale-monitor scenario the production code is defending against. The prior fake's single live closure variable made that scenario impossible to express. --- .../src/backends/container/container-host.ts | 18 +-- .../container/container-lifecycle.test.ts | 132 +++++++++++++----- .../backends/container/container-lifecycle.ts | 104 ++++++++------ 3 files changed, 170 insertions(+), 84 deletions(-) diff --git a/packages/workspace/src/backends/container/container-host.ts b/packages/workspace/src/backends/container/container-host.ts index 3e2a21d6..a35f2b91 100644 --- a/packages/workspace/src/backends/container/container-host.ts +++ b/packages/workspace/src/backends/container/container-host.ts @@ -105,20 +105,22 @@ export class WorkspaceContainerAPI extends RpcTarget implements IWorkspaceContai } async start(env: Record) { - // If the previous generation died but the runtime still has a - // container attached, treat the prior exit as a signal to wipe - // the carcass before re-starting. The new generation gets a - // clean monitor(). - const prior = containerExitInfo(this.#ctx); - if (prior !== null) { + // If a prior generation has died, commit to a fresh one: the + // destroy clears any platform-side carcass, and the start that + // follows is unconditional. We cannot rely on + // this.#container.running to flip to false synchronously after + // destroy resolves, so guarding the start against it would let + // a stale-running flag skip the re-launch entirely. + const priorExit = containerExitInfo(this.#ctx); + if (priorExit !== null) { try { await destroyContainerExpectingExit(this.#ctx, this.#container); } catch { // best-effort — the next start() will surface any real // platform-side failure. } - } - if (!this.#container.running) { + this.#container.start({ enableInternet: true, env }); + } else if (!this.#container.running) { this.#container.start({ enableInternet: true, env }); } armContainerMonitor(this.#ctx, this.#container); diff --git a/packages/workspace/src/backends/container/container-lifecycle.test.ts b/packages/workspace/src/backends/container/container-lifecycle.test.ts index b9e3f38d..d562246d 100644 --- a/packages/workspace/src/backends/container/container-lifecycle.test.ts +++ b/packages/workspace/src/backends/container/container-lifecycle.test.ts @@ -11,35 +11,49 @@ import { // Minimal Container stand-in. The lifecycle module only touches // .destroy(), .start(), .running, and .monitor(). A controllable -// monitor() promise lets the tests drive the exit signal -// deterministically. +// per-generation monitor() promise lets the tests drive the exit +// signal deterministically and aim it at a specific generation. +// +// `current` exposes the live generation's controls; `generations` +// preserves the per-generation tuples so a test can fire the +// first generation's reject AFTER the second generation has been +// armed, simulating the platform's behavior when an old monitor's +// settle frame arrives late. +interface MonitorControls { + resolve: () => void; + reject: (error: unknown) => void; + promise: Promise; +} + function makeContainer(): { container: NonNullable; starts: number; destroys: number; monitorCalls: number; - resolveMonitor: () => void; - rejectMonitor: (error: unknown) => void; - monitorPromise: () => Promise; + current: MonitorControls; + generations: MonitorControls[]; } { let starts = 0; let destroys = 0; let monitorCalls = 0; let running = false; - let resolveMonitor!: () => void; - let rejectMonitor!: (error: unknown) => void; - let monitorPromise: Promise; - - function armPromise() { - monitorPromise = new Promise((resolve, reject) => { - resolveMonitor = resolve; - rejectMonitor = reject; + const generations: MonitorControls[] = []; + + function armPromise(): MonitorControls { + let resolve!: () => void; + let reject!: (error: unknown) => void; + const promise = new Promise((res, rej) => { + resolve = res; + reject = rej; }); // Swallow unhandled rejection noise when the test path leaves // the promise pending or rejects without awaiting it. - monitorPromise.catch(() => {}); + promise.catch(() => {}); + const controls: MonitorControls = { resolve, reject, promise }; + generations.push(controls); + return controls; } - armPromise(); + let current = armPromise(); const container = { get running() { @@ -50,17 +64,20 @@ function makeContainer(): { running = true; // Each start() arms a fresh monitor() that the next monitor() // call returns. - armPromise(); + current = armPromise(); }, async destroy() { destroys++; running = false; - // destroy() resolves the in-flight monitor() cleanly. - resolveMonitor(); + // The real container.monitor() rejects on destroy() (SIGKILL + // surfaces as a non-zero exit). The fake mirrors that + // contract so the lifecycle's expected-exit handler is + // tested against the platform's actual settle direction. + current.reject(new Error("container destroyed")); }, monitor() { monitorCalls++; - return monitorPromise; + return current.promise; }, } as unknown as NonNullable; @@ -75,9 +92,10 @@ function makeContainer(): { get monitorCalls() { return monitorCalls; }, - resolveMonitor: () => resolveMonitor(), - rejectMonitor: (error: unknown) => rejectMonitor(error), - monitorPromise: () => monitorPromise, + get current() { + return current; + }, + generations, } as ReturnType; } @@ -111,7 +129,11 @@ describe("formatExitReason", () => { }); describe("armContainerMonitor", () => { - test("records exit info when the monitor resolves", async () => { + test("records exit info when the monitor resolves (clean exit)", async () => { + // container.monitor() resolves only on a clean code-0 exit on + // the real platform. The lifecycle treats that as 'exited + // normally' — useful when the workload exits on its own + // rather than being SIGKILL'd by the runtime. const fake = makeContainer(); const ctx = makeCtx(fake.container); resetContainerLifecycleForTests(ctx); @@ -119,7 +141,7 @@ describe("armContainerMonitor", () => { armContainerMonitor(ctx, fake.container); expect(containerExitInfo(ctx)).toBeNull(); - fake.resolveMonitor(); + fake.current.resolve(); // Microtask drain. await Promise.resolve(); await Promise.resolve(); @@ -137,7 +159,7 @@ describe("armContainerMonitor", () => { fake.container.start(); armContainerMonitor(ctx, fake.container); - fake.rejectMonitor(new Error("container crashed")); + fake.current.reject(new Error("container crashed")); await Promise.resolve(); await Promise.resolve(); @@ -153,7 +175,7 @@ describe("armContainerMonitor", () => { fake.container.start(); armContainerMonitor(ctx, fake.container); - fake.rejectMonitor(new Error("OOM killed")); + fake.current.reject(new Error("OOM killed")); await Promise.resolve(); await Promise.resolve(); @@ -167,6 +189,10 @@ describe("armContainerMonitor", () => { }); test("logs at info level when the exit was expected (after destroyContainerExpectingExit)", async () => { + // The platform monitor() rejects on destroy. The lifecycle + // snapshots expectingExit at arm time, so even though the + // destroy's finally clears the flag synchronously, the + // monitor handler that fires later still sees expected:true. const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); const info = vi.spyOn(console, "info").mockImplementation(() => {}); const fake = makeContainer(); @@ -186,22 +212,50 @@ describe("armContainerMonitor", () => { expect(arg).toMatchObject({ message: "workspace.container.exited", expected: true, + reason: "container destroyed", }); }); - test("does not arm twice for the same container generation", () => { + test("a late-rejecting stale monitor does not poison a new generation", async () => { + // First generation arms its monitor; we leave it pending. + // Second generation arms a new monitor (incrementing the + // generation counter). The stale handler firing after the + // new generation has armed must NOT overwrite the new + // generation's clean exit state. const fake = makeContainer(); const ctx = makeCtx(fake.container); resetContainerLifecycleForTests(ctx); + fake.container.start(); + const firstGeneration = fake.current; armContainerMonitor(ctx, fake.container); + + // Second generation — a new monitor promise is armed in the + // fake's start(); armContainerMonitor bumps the lifecycle's + // generation counter and attaches a fresh handler against the + // new promise. + fake.container.start(); armContainerMonitor(ctx, fake.container); - expect(fake.monitorCalls).toBe(1); + const secondGeneration = fake.current; + + expect(containerExitInfo(ctx)).toBeNull(); + // Settle the stale monitor with an error — it must be + // ignored because its generation is no longer current. + firstGeneration.reject(new Error("old generation died long ago")); + await Promise.resolve(); + await Promise.resolve(); + expect(containerExitInfo(ctx)).toBeNull(); + + // The current generation's monitor still records normally. + secondGeneration.reject(new Error("current generation died")); + await Promise.resolve(); + await Promise.resolve(); + expect(containerExitInfo(ctx)?.reason).toBe("current generation died"); }); }); describe("destroyContainerExpectingExit", () => { - test("clears the expectingExit flag after destroy() resolves", async () => { + test("resets expectingExit so the next generation's crash logs as a crash", async () => { const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); const fake = makeContainer(); const ctx = makeCtx(fake.container); @@ -215,7 +269,7 @@ describe("destroyContainerExpectingExit", () => { // Arm a fresh monitor for a new container generation. fake.container.start(); armContainerMonitor(ctx, fake.container); - fake.rejectMonitor(new Error("real crash")); + fake.current.reject(new Error("real crash")); await Promise.resolve(); await Promise.resolve(); @@ -223,26 +277,34 @@ describe("destroyContainerExpectingExit", () => { expect(warn).toHaveBeenCalledTimes(1); }); - test("clears the expectingExit flag even if destroy() throws", async () => { + test("a later real crash on a fresh generation logs as unexpected after a failed destroy", async () => { + // destroy() rejects against generation 1. The expected-exit + // mark sticks against generation 1. A subsequent start() + // arms generation 2; a crash on generation 2 must be logged + // as unexpected because the mark targets a generation that + // no longer matches the live one. const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); const fake = makeContainer(); const ctx = makeCtx(fake.container); resetContainerLifecycleForTests(ctx); fake.container.start(); armContainerMonitor(ctx, fake.container); + const broken = { destroy: async () => { throw new Error("destroy rejected"); }, } as unknown as NonNullable; - await expect(destroyContainerExpectingExit(ctx, broken)).rejects.toThrow(/destroy rejected/); - // A later real crash must still log as unexpected. - fake.rejectMonitor(new Error("real crash")); + // Fresh generation, fresh monitor. + fake.container.start(); + armContainerMonitor(ctx, fake.container); + fake.current.reject(new Error("real crash")); await Promise.resolve(); await Promise.resolve(); expect(warn).toHaveBeenCalledTimes(1); + expect(warn.mock.calls[0]?.[0]).toMatchObject({ expected: false }); }); }); @@ -267,7 +329,7 @@ describe("getContainerLifecycle", () => { b.container.start(); armContainerMonitor(ctxB, b.container); - a.rejectMonitor(new Error("a crashed")); + a.current.reject(new Error("a crashed")); await Promise.resolve(); await Promise.resolve(); diff --git a/packages/workspace/src/backends/container/container-lifecycle.ts b/packages/workspace/src/backends/container/container-lifecycle.ts index 01b7330f..965b6c66 100644 --- a/packages/workspace/src/backends/container/container-lifecycle.ts +++ b/packages/workspace/src/backends/container/container-lifecycle.ts @@ -19,26 +19,31 @@ export interface ContainerExitInfo { interface ContainerLifecycleState { // Populated when the in-flight monitor() resolves or rejects. + // Only the monitor whose generation matches `currentGeneration` + // is allowed to write here — a stale handler from a previous + // generation must not poison the fresh one's exit state. exit: ContainerExitInfo | null; - // The monitor() promise we attached to the current container - // generation. null between generations and before the first - // start(). - monitorArmed: Container | null; - // Set just before our own destroy() so the monitor handler - // records the exit as intentional and logs at info, not warn. - expectingExit: boolean; + // Monotonically incremented every time a new monitor is armed. + // The armContainerMonitor closure captures the value at arm + // time so a late-resolving old monitor can detect that it has + // been superseded. + currentGeneration: number; + // The generation whose monitor is expected to terminate — set + // by destroyContainerExpectingExit just before the destroy call. + // The monitor handler reads this and, if it matches its own + // generation, logs the exit as intentional. Storing the + // generation (not a boolean) means a later spurious clear can + // never affect the current generation, and a flag set against + // generation N cannot leak into generation N+1. + expectedExitGeneration: number | null; } -// Wrapper type so the test stand-in can be plain object literals -// without needing the full Container interface to type-check. -type Container = ContainerHandle; - const LIFECYCLE = new WeakMap(); export function getContainerLifecycle(ctx: DurableObjectState): ContainerLifecycleState { let state = LIFECYCLE.get(ctx); if (state === undefined) { - state = { exit: null, monitorArmed: null, expectingExit: false }; + state = { exit: null, currentGeneration: 0, expectedExitGeneration: null }; LIFECYCLE.set(ctx, state); } return state; @@ -55,47 +60,55 @@ export function formatExitReason(error: unknown): string { } // Arm a monitor() for the currently-attached container generation. -// Idempotent: a second call for the same generation is a no-op so -// callers can invoke this on every start() without double-attaching. +// Every call bumps the generation counter and attaches a fresh +// .then handler. The handler captures its generation at arm time +// so a stale monitor that resolves late cannot overwrite a newer +// generation's exit state. +// +// The expected-or-not classification is taken from the live +// `expectedExitGeneration` slot when the handler runs, which is +// what destroyContainerExpectingExit writes — reading the slot +// (not a closure snapshot) lets a destroy() called *after* arm +// still mark its own generation's exit as intentional. // -// The monitor handler records exit info, logs once, and clears the -// armed reference so the next start() can re-arm. Both branches -// (resolve, reject) feed into the same recorder. -export function armContainerMonitor(ctx: DurableObjectState, container: Container): void { +// Callers (start, restart) sequence arming after a successful +// container.start(); the platform contract is "one monitor per +// generation". If a caller arms twice for the same generation, +// the worst that happens is two handlers race to write the same +// exit state — same value, same generation. +export function armContainerMonitor(ctx: DurableObjectState, container: ContainerHandle): void { const state = getContainerLifecycle(ctx); - if (state.monitorArmed === container) return; - state.monitorArmed = container; + state.currentGeneration += 1; + const generation = state.currentGeneration; // Clear any prior exit info — a fresh generation has started. state.exit = null; const promise = container.monitor(); promise.then( - () => recordExit(state, undefined), - (error) => recordExit(state, error), + () => recordExit(state, generation, undefined), + (error) => recordExit(state, generation, error), ); } -// Tear down the current container generation. Sets expectingExit -// so the monitor handler logs the resulting exit as intentional. -// Clears the flag in a finally so a failing destroy() does not -// leave a stale flag that would mis-classify a later real crash. +// Tear down the current container generation. Marks the current +// generation as the one expected to terminate so the monitor +// handler logs the resulting exit as intentional. +// +// The mark is keyed by generation, not by a global boolean, so a +// destroy() that fires while a different generation is in flight +// (e.g. a stale destroy attempt against a generation that has +// already been replaced) cannot mark the wrong generation's exit +// as intentional. The mark sticks until the next arm; if destroy +// throws and a fresh generation never gets armed, the mark sits +// against a generation that no longer matches `currentGeneration` +// and any later real crash on the new generation logs as a crash. export async function destroyContainerExpectingExit( ctx: DurableObjectState, - container: Container, + container: ContainerHandle, ): Promise { const state = getContainerLifecycle(ctx); - state.expectingExit = true; - try { - await container.destroy(); - } finally { - // The monitor handler observes expectingExit synchronously the - // moment destroy() resolves the monitor promise. Clearing here - // races the handler — but the handler has already snapshotted - // the flag by the time it runs (recordExit reads expectingExit - // first), so clearing now is safe. - state.expectingExit = false; - state.monitorArmed = null; - } + state.expectedExitGeneration = state.currentGeneration; + await container.destroy(); } // Reset state for a ctx. Test-only escape hatch; the production @@ -104,8 +117,17 @@ export function resetContainerLifecycleForTests(ctx: DurableObjectState): void { LIFECYCLE.delete(ctx); } -function recordExit(state: ContainerLifecycleState, error: unknown): void { - const expected = state.expectingExit; +function recordExit(state: ContainerLifecycleState, generation: number, error: unknown): void { + // Drop late writes from superseded monitors. The state object is + // shared across generations; only the current one is allowed to + // mutate `exit`. Log nothing on a stale exit — the operator + // already saw the live generation's exit when it happened. + if (generation !== state.currentGeneration) return; + const expected = state.expectedExitGeneration === generation; + // Consume the expected mark so a later spurious monitor + // resolution (e.g. an idempotent arm against the same + // generation) does not double-claim the intentional log. + if (expected) state.expectedExitGeneration = null; const reason = formatExitReason(error); const exitedAt = Date.now(); state.exit = { reason, exitedAt }; From cb375f66f1f33d9c3d8e23ff1c20f5f4f8f67fd1 Mon Sep 17 00:00:00 2001 From: aron <263346377+aron-cf@users.noreply.github.com> Date: Tue, 16 Jun 2026 18:24:03 +0000 Subject: [PATCH 07/11] workspace: tighten test fakes and probe budget floor Three small cleanups around the container backend. health-probe.test.ts cast `as unknown as IWorkspaceContainerAPI` masked three missing interface methods. probeWsdHealth only reaches fetchPort, so type the fake against a structural Pick and let the cast narrow the surface honestly. cloudflare-container.test.ts fake-host cast `as IWorkspaceContainerAPI` is now `satisfies IWorkspaceContainerAPI`, so a future interface addition fails the build instead of slipping through. #readyWithRestarts split the connect budget evenly across attempts with a 1ms floor. Bump the floor to 250ms so a readiness check near the connect deadline still has room to dispatch one real probe rather than collapsing into a string of immediate timeouts. --- .../container/cloudflare-container.test.ts | 2 +- .../backends/container/cloudflare-container.ts | 6 ++++-- .../src/backends/container/health-probe.test.ts | 17 ++++++++++------- 3 files changed, 15 insertions(+), 10 deletions(-) diff --git a/packages/workspace/src/backends/container/cloudflare-container.test.ts b/packages/workspace/src/backends/container/cloudflare-container.test.ts index eee12800..3fee7a0c 100644 --- a/packages/workspace/src/backends/container/cloudflare-container.test.ts +++ b/packages/workspace/src/backends/container/cloudflare-container.test.ts @@ -110,7 +110,7 @@ function makeFakeHost(opts: FakeHostOptions = {}): FakeHost { calls.push({ name: "exitInfo", args: [] }); return state.exit; }, - } as IWorkspaceContainerAPI; + } satisfies IWorkspaceContainerAPI; return state; } diff --git a/packages/workspace/src/backends/container/cloudflare-container.ts b/packages/workspace/src/backends/container/cloudflare-container.ts index 630f6be5..fc5df7df 100644 --- a/packages/workspace/src/backends/container/cloudflare-container.ts +++ b/packages/workspace/src/backends/container/cloudflare-container.ts @@ -342,9 +342,11 @@ export class CloudflareContainerBackend implements WorkspaceBackend { ): Promise { const maxAttempts = this.#options.restartAttempts + 1; // Split the remaining time across attempts so a failing - // first attempt doesn't starve the restart-retry. + // first attempt doesn't starve the restart-retry. Floor at + // 250ms so a near-deadline last attempt still has room to + // dispatch a probe rather than collapsing to ~1ms. const totalBudget = Math.max(0, deadline - Date.now()); - const perAttemptBudget = Math.max(1, Math.floor(totalBudget / maxAttempts)); + const perAttemptBudget = Math.max(250, Math.floor(totalBudget / maxAttempts)); let attempt = 0; let restarts = 0; let lastError: unknown; diff --git a/packages/workspace/src/backends/container/health-probe.test.ts b/packages/workspace/src/backends/container/health-probe.test.ts index 3afed7b9..327acf9c 100644 --- a/packages/workspace/src/backends/container/health-probe.test.ts +++ b/packages/workspace/src/backends/container/health-probe.test.ts @@ -3,17 +3,20 @@ import { describe, expect, test, vi } from "vitest"; import type { IWorkspaceContainerAPI } from "./container-host.js"; import { probeWsdHealth } from "./health-probe.js"; +// probeWsdHealth only consumes fetchPort. The helper is typed +// against the wider IWorkspaceContainerAPI to make ergonomic +// same-isolate calls cheap, but the test scope is narrower — use +// the minimal structural type so the fake doesn't have to stub +// methods it never reaches. +type HealthProbeHost = Pick; + function fakeHost( handler: (port: number, input: RequestInfo | URL, init?: RequestInit) => Promise, ): IWorkspaceContainerAPI { - return { - start: vi.fn(async () => {}), - interceptOutboundHttp: vi.fn(async () => {}), + const host: HealthProbeHost = { fetchPort: vi.fn(handler), - port: vi.fn(() => { - throw new Error("port() not used in these tests"); - }), - } as unknown as IWorkspaceContainerAPI; + }; + return host as IWorkspaceContainerAPI; } describe("probeWsdHealth", () => { From 3c478e4ccb7fe5f0e4b91c4a88da25d0152fce84 Mon Sep 17 00:00:00 2001 From: aron <263346377+aron-cf@users.noreply.github.com> Date: Tue, 16 Jun 2026 18:38:01 +0000 Subject: [PATCH 08/11] workspace: invalidate cached handle on mid-stream exec failures WorkspaceShellRouter.exec/get wrapped the dispatch call in a transport-failure catch, but the dispatch only fails when the WebSocket is already gone at the moment of the call. The common case is a long-running command that loses its transport mid-run: exec() returns a handle, the event stream errors partway through, and result() rejects with the transport error. The dispatch catch never fires; the cached backend handle stays stuck. Make the ExecHandle's result/kill property descriptors configurable so the router can redefine result(). On every returned handle the router wraps result() with a try/catch that routes transport-classified rejections through the same invalidation path push/pull and exec dispatch already use. The wrap is opaque to callers \u2014 they see the same ExecHandle shape, and consumers reading the underlying ReadableStream directly are untouched. id stays non-configurable; nothing should ever rewrite that. The contract change \u2014 result/kill configurable instead of locked \u2014 is fine: there are no external consumers yet, and the new flexibility is what unlocked the fix. --- packages/workspace/src/shell.ts | 8 ++++- packages/workspace/src/workspace.test.ts | 46 ++++++++++++++++++++++++ packages/workspace/src/workspace.ts | 41 +++++++++++++++++++-- 3 files changed, 92 insertions(+), 3 deletions(-) diff --git a/packages/workspace/src/shell.ts b/packages/workspace/src/shell.ts index cf7a5c9e..14f5083b 100644 --- a/packages/workspace/src/shell.ts +++ b/packages/workspace/src/shell.ts @@ -229,12 +229,17 @@ function wrapHandle( const exited = watchForExit(forWatcher); const stream = pipeEvents(forUser, encoding); const handle = stream as ExecHandle; + // configurable: true on result/kill lets the Workspace-level + // router redefine them to add cross-cutting concerns (transport + // failure invalidation on result(); future kill hooks). The id + // slot stays non-configurable — nothing should rewrite it. Object.defineProperties(handle, { - id: { value: id, enumerable: false, writable: false }, + id: { value: id, enumerable: false, writable: false, configurable: false }, result: { value: () => drainToResult(stream, encoding, sync, pushed), enumerable: false, writable: false, + configurable: true, }, kill: { value: async (signal?: KillSignal) => { @@ -243,6 +248,7 @@ function wrapHandle( }, enumerable: false, writable: false, + configurable: true, }, }); return handle; diff --git a/packages/workspace/src/workspace.test.ts b/packages/workspace/src/workspace.test.ts index 25d88563..7d732051 100644 --- a/packages/workspace/src/workspace.test.ts +++ b/packages/workspace/src/workspace.test.ts @@ -912,4 +912,50 @@ describe("Workspace transport-failure invalidation", () => { await ws.shell.exec("true").catch(() => undefined); expect(connects).toBe(2); }); + + it("shell.exec invalidates the cached handle on a mid-stream transport error", async () => { + // The exec dispatch succeeds; the event stream errors later + // with a transport-classified failure. This is the realistic + // case when a long-running command loses its WebSocket + // mid-run — result() rejects with the transport error, and + // the wrap around the returned handle must invalidate the + // cached backend handle so the next operation reconnects. + let connects = 0; + const shell: import("@cloudflare/workspace-rpc").ShellRPC = { + async exec(input) { + const execId = input.id ?? "mid-stream"; + return { + id: execId, + events: new ReadableStream({ + start(c) { + c.error(new WorkspaceTransportError("WebSocket closed mid-stream")); + }, + }), + }; + }, + getExec: () => Promise.reject(new Error("not used")), + killExec: () => Promise.reject(new Error("not used")), + disposeExec: () => Promise.reject(new Error("not used")), + }; + const backend: WorkspaceBackend = { + id: "only", + type: "fake", + async connect(): Promise { + connects++; + return { + rpc: { sync: fakeRpc(), shell }, + sync: "none", + closed: new Promise(() => {}), + close: async () => {}, + }; + }, + }; + const ws = new Workspace({ storage: makeStorage(), backends: [backend] }); + const handle = await ws.shell.exec("sleep 60"); + await expect(handle.result()).rejects.toThrow(/WebSocket closed mid-stream/); + expect(connects).toBe(1); + // Next exec attempt must reconnect. + await ws.shell.exec("true").catch(() => undefined); + expect(connects).toBe(2); + }); }); diff --git a/packages/workspace/src/workspace.ts b/packages/workspace/src/workspace.ts index 9e221300..f2ee2e12 100644 --- a/packages/workspace/src/workspace.ts +++ b/packages/workspace/src/workspace.ts @@ -610,8 +610,9 @@ class WorkspaceShellRouter { const id = this.#resolveId(options.backend) || this.#defaultId; const shell = await this.#shellFor(id); const { backend: _backend, ...rest } = options; + let handle: unknown; try { - return await (shell.exec as unknown as (c: string, o: typeof rest) => Promise)( + handle = await (shell.exec as unknown as (c: string, o: typeof rest) => Promise)( command, rest, ); @@ -619,14 +620,16 @@ class WorkspaceShellRouter { this.#onError(id, error); throw error; } + return this.#wrapHandle(id, handle); } async get(id: string, options: { backend?: string } & Record = {}) { const backendId = this.#resolveId(options.backend) || this.#defaultId; const shell = await this.#shellFor(backendId); const { backend: _backend, ...rest } = options; + let handle: unknown; try { - return await (shell.get as unknown as (e: string, o: typeof rest) => Promise)( + handle = await (shell.get as unknown as (e: string, o: typeof rest) => Promise)( id, rest, ); @@ -634,5 +637,39 @@ class WorkspaceShellRouter { this.#onError(backendId, error); throw error; } + return this.#wrapHandle(backendId, handle); + } + + // Wrap an ExecHandle so a transport-classified rejection from + // result() invalidates the cached backend handle. The dispatch- + // time catch above only fires when shell.exec()/get() rejects + // immediately; in practice a long-running command loses its + // transport mid-stream and the rejection surfaces through + // result() draining the event stream. + // + // WorkspaceShell installs .result via defineProperty; we set it + // configurable: true so this slot can be redefined. The handle's + // stream identity is preserved — callers that consume the + // ReadableStream directly are unaffected; only result() routes + // through the invalidation path. + #wrapHandle(id: string, handle: unknown): unknown { + const original = handle as { result?: unknown }; + if (typeof original.result !== "function") return handle; + const onError = this.#onError; + const originalResult = original.result.bind(handle) as () => Promise; + Object.defineProperty(handle, "result", { + value: async () => { + try { + return await originalResult(); + } catch (error) { + onError(id, error); + throw error; + } + }, + enumerable: false, + writable: false, + configurable: true, + }); + return handle; } } From 2b35bda7eb9426e72ed9a72535f77c41a3e144a0 Mon Sep 17 00:00:00 2001 From: aron <263346377+aron-cf@users.noreply.github.com> Date: Tue, 16 Jun 2026 19:12:22 +0000 Subject: [PATCH 09/11] workspace: rename armContainerMonitor and the test ctx helper Two naming nits surfaced in review. The verb 'arm' reads like a weapon metaphor and obscures what the helper actually does \u2014 it installs a per-generation handler against container.monitor() so the lifecycle module can record the exit. 'install' is plainer and matches the read-the-method-name test. The test helper makeCtx had a similar problem: the abbreviation saved three letters at the cost of one of the worst conventions in the codebase (ctx vs context). makeContext is what the rest of the file already calls the value it returns. No behavior change; the renamed identifiers are local to the container backend's lifecycle module, the host shim that consumes it, and the lifecycle module's own tests. --- .../src/backends/container/container-host.ts | 6 +-- .../container/container-lifecycle.test.ts | 52 +++++++++---------- .../backends/container/container-lifecycle.ts | 4 +- 3 files changed, 31 insertions(+), 31 deletions(-) diff --git a/packages/workspace/src/backends/container/container-host.ts b/packages/workspace/src/backends/container/container-host.ts index a35f2b91..fffe09f9 100644 --- a/packages/workspace/src/backends/container/container-host.ts +++ b/packages/workspace/src/backends/container/container-host.ts @@ -21,10 +21,10 @@ import { RpcTarget } from "cloudflare:workers"; import { WorkspaceTransportError } from "../../transport-failure.js"; import { - armContainerMonitor, type ContainerExitInfo, containerExitInfo, destroyContainerExpectingExit, + installContainerMonitor, } from "./container-lifecycle.js"; export type { ContainerExitInfo } from "./container-lifecycle.js"; @@ -123,7 +123,7 @@ export class WorkspaceContainerAPI extends RpcTarget implements IWorkspaceContai } else if (!this.#container.running) { this.#container.start({ enableInternet: true, env }); } - armContainerMonitor(this.#ctx, this.#container); + installContainerMonitor(this.#ctx, this.#container); } async restart(env: Record) { @@ -140,7 +140,7 @@ export class WorkspaceContainerAPI extends RpcTarget implements IWorkspaceContai // failure. } this.#container.start({ enableInternet: true, env }); - armContainerMonitor(this.#ctx, this.#container); + installContainerMonitor(this.#ctx, this.#container); } async status() { diff --git a/packages/workspace/src/backends/container/container-lifecycle.test.ts b/packages/workspace/src/backends/container/container-lifecycle.test.ts index d562246d..c272c4d0 100644 --- a/packages/workspace/src/backends/container/container-lifecycle.test.ts +++ b/packages/workspace/src/backends/container/container-lifecycle.test.ts @@ -1,11 +1,11 @@ import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; import { - armContainerMonitor, containerExitInfo, destroyContainerExpectingExit, formatExitReason, getContainerLifecycle, + installContainerMonitor, resetContainerLifecycleForTests, } from "./container-lifecycle.js"; @@ -100,7 +100,7 @@ function makeContainer(): { } // Lifecycle state is keyed by ctx — a tiny opaque object suffices. -function makeCtx(container: NonNullable): DurableObjectState { +function makeContext(container: NonNullable): DurableObjectState { return { container } as unknown as DurableObjectState; } @@ -128,17 +128,17 @@ describe("formatExitReason", () => { }); }); -describe("armContainerMonitor", () => { +describe("installContainerMonitor", () => { test("records exit info when the monitor resolves (clean exit)", async () => { // container.monitor() resolves only on a clean code-0 exit on // the real platform. The lifecycle treats that as 'exited // normally' — useful when the workload exits on its own // rather than being SIGKILL'd by the runtime. const fake = makeContainer(); - const ctx = makeCtx(fake.container); + const ctx = makeContext(fake.container); resetContainerLifecycleForTests(ctx); fake.container.start(); - armContainerMonitor(ctx, fake.container); + installContainerMonitor(ctx, fake.container); expect(containerExitInfo(ctx)).toBeNull(); fake.current.resolve(); @@ -154,10 +154,10 @@ describe("armContainerMonitor", () => { test("records the rejection reason when the monitor rejects", async () => { const fake = makeContainer(); - const ctx = makeCtx(fake.container); + const ctx = makeContext(fake.container); resetContainerLifecycleForTests(ctx); fake.container.start(); - armContainerMonitor(ctx, fake.container); + installContainerMonitor(ctx, fake.container); fake.current.reject(new Error("container crashed")); await Promise.resolve(); @@ -170,10 +170,10 @@ describe("armContainerMonitor", () => { test("logs at warn level on an unexpected exit", async () => { const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); const fake = makeContainer(); - const ctx = makeCtx(fake.container); + const ctx = makeContext(fake.container); resetContainerLifecycleForTests(ctx); fake.container.start(); - armContainerMonitor(ctx, fake.container); + installContainerMonitor(ctx, fake.container); fake.current.reject(new Error("OOM killed")); await Promise.resolve(); @@ -196,10 +196,10 @@ describe("armContainerMonitor", () => { const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); const info = vi.spyOn(console, "info").mockImplementation(() => {}); const fake = makeContainer(); - const ctx = makeCtx(fake.container); + const ctx = makeContext(fake.container); resetContainerLifecycleForTests(ctx); fake.container.start(); - armContainerMonitor(ctx, fake.container); + installContainerMonitor(ctx, fake.container); await destroyContainerExpectingExit(ctx, fake.container); // Drain the monitor's then-chain. @@ -223,19 +223,19 @@ describe("armContainerMonitor", () => { // new generation has armed must NOT overwrite the new // generation's clean exit state. const fake = makeContainer(); - const ctx = makeCtx(fake.container); + const ctx = makeContext(fake.container); resetContainerLifecycleForTests(ctx); fake.container.start(); const firstGeneration = fake.current; - armContainerMonitor(ctx, fake.container); + installContainerMonitor(ctx, fake.container); // Second generation — a new monitor promise is armed in the - // fake's start(); armContainerMonitor bumps the lifecycle's + // fake's start(); installContainerMonitor bumps the lifecycle's // generation counter and attaches a fresh handler against the // new promise. fake.container.start(); - armContainerMonitor(ctx, fake.container); + installContainerMonitor(ctx, fake.container); const secondGeneration = fake.current; expect(containerExitInfo(ctx)).toBeNull(); @@ -258,17 +258,17 @@ describe("destroyContainerExpectingExit", () => { test("resets expectingExit so the next generation's crash logs as a crash", async () => { const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); const fake = makeContainer(); - const ctx = makeCtx(fake.container); + const ctx = makeContext(fake.container); resetContainerLifecycleForTests(ctx); fake.container.start(); - armContainerMonitor(ctx, fake.container); + installContainerMonitor(ctx, fake.container); await destroyContainerExpectingExit(ctx, fake.container); await Promise.resolve(); await Promise.resolve(); // Arm a fresh monitor for a new container generation. fake.container.start(); - armContainerMonitor(ctx, fake.container); + installContainerMonitor(ctx, fake.container); fake.current.reject(new Error("real crash")); await Promise.resolve(); await Promise.resolve(); @@ -285,10 +285,10 @@ describe("destroyContainerExpectingExit", () => { // no longer matches the live one. const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); const fake = makeContainer(); - const ctx = makeCtx(fake.container); + const ctx = makeContext(fake.container); resetContainerLifecycleForTests(ctx); fake.container.start(); - armContainerMonitor(ctx, fake.container); + installContainerMonitor(ctx, fake.container); const broken = { destroy: async () => { @@ -299,7 +299,7 @@ describe("destroyContainerExpectingExit", () => { // Fresh generation, fresh monitor. fake.container.start(); - armContainerMonitor(ctx, fake.container); + installContainerMonitor(ctx, fake.container); fake.current.reject(new Error("real crash")); await Promise.resolve(); await Promise.resolve(); @@ -311,7 +311,7 @@ describe("destroyContainerExpectingExit", () => { describe("getContainerLifecycle", () => { test("returns null exit info before any monitor has fired", () => { const fake = makeContainer(); - const ctx = makeCtx(fake.container); + const ctx = makeContext(fake.container); resetContainerLifecycleForTests(ctx); expect(containerExitInfo(ctx)).toBeNull(); expect(getContainerLifecycle(ctx).exit).toBeNull(); @@ -320,14 +320,14 @@ describe("getContainerLifecycle", () => { test("isolates state per ctx via the WeakMap", async () => { const a = makeContainer(); const b = makeContainer(); - const ctxA = makeCtx(a.container); - const ctxB = makeCtx(b.container); + const ctxA = makeContext(a.container); + const ctxB = makeContext(b.container); resetContainerLifecycleForTests(ctxA); resetContainerLifecycleForTests(ctxB); a.container.start(); - armContainerMonitor(ctxA, a.container); + installContainerMonitor(ctxA, a.container); b.container.start(); - armContainerMonitor(ctxB, b.container); + installContainerMonitor(ctxB, b.container); a.current.reject(new Error("a crashed")); await Promise.resolve(); diff --git a/packages/workspace/src/backends/container/container-lifecycle.ts b/packages/workspace/src/backends/container/container-lifecycle.ts index 965b6c66..8416c20f 100644 --- a/packages/workspace/src/backends/container/container-lifecycle.ts +++ b/packages/workspace/src/backends/container/container-lifecycle.ts @@ -24,7 +24,7 @@ interface ContainerLifecycleState { // generation must not poison the fresh one's exit state. exit: ContainerExitInfo | null; // Monotonically incremented every time a new monitor is armed. - // The armContainerMonitor closure captures the value at arm + // The installContainerMonitor closure captures the value at arm // time so a late-resolving old monitor can detect that it has // been superseded. currentGeneration: number; @@ -76,7 +76,7 @@ export function formatExitReason(error: unknown): string { // generation". If a caller arms twice for the same generation, // the worst that happens is two handlers race to write the same // exit state — same value, same generation. -export function armContainerMonitor(ctx: DurableObjectState, container: ContainerHandle): void { +export function installContainerMonitor(ctx: DurableObjectState, container: ContainerHandle): void { const state = getContainerLifecycle(ctx); state.currentGeneration += 1; const generation = state.currentGeneration; From c77705c2fbb44a9a96645772aef6b08c5ae844dc Mon Sep 17 00:00:00 2001 From: aron <263346377+aron-cf@users.noreply.github.com> Date: Tue, 16 Jun 2026 19:50:21 +0000 Subject: [PATCH 10/11] workspace: await the destroyed generation's monitor settle on teardown WorkspaceContainerAPI.restart() destroys the current container and immediately starts the next one. installContainerMonitor then runs against the new generation, bumping currentGeneration. The platform settles container.monitor() asynchronously relative to container.destroy(): destroy can return before the monitor promise rejects with the abnormal-exit reason. When that happens, the monitor handler from the OLD generation only runs after installContainerMonitor has already bumped the counter, so recordExit sees a generation that no longer matches and drops the write as stale. The expected-exit log line for the destroy is silently lost. Track each generation's monitor-then wrapper on the lifecycle state as currentMonitorSettled. destroyContainerExpectingExit captures the wrapper before the destroy call and awaits it after destroy resolves, so the handler runs to completion before the caller continues. The next installContainerMonitor then reassigns currentMonitorSettled with the fresh generation's wrapper. The new test uses real timers and setTimeout(0) inside its container fake so destroy() and the monitor rejection straddle a macrotask boundary \u2014 microtask-only settling (queueMicrotask, Promise.resolve) drains before the awaiter resumes and would mask the race. Confirmed the test fails against the prior behavior: info called 0 times, no expected-exit log emitted. --- .../container/container-lifecycle.test.ts | 81 +++++++++++++++++++ .../backends/container/container-lifecycle.ts | 36 ++++++++- 2 files changed, 114 insertions(+), 3 deletions(-) diff --git a/packages/workspace/src/backends/container/container-lifecycle.test.ts b/packages/workspace/src/backends/container/container-lifecycle.test.ts index c272c4d0..ee8f6968 100644 --- a/packages/workspace/src/backends/container/container-lifecycle.test.ts +++ b/packages/workspace/src/backends/container/container-lifecycle.test.ts @@ -277,6 +277,87 @@ describe("destroyContainerExpectingExit", () => { expect(warn).toHaveBeenCalledTimes(1); }); + test("expected-exit log fires for restart even when monitor settles after destroy resolves", async () => { + // Production timing: the platform settles container.monitor() + // asynchronously relative to container.destroy(). If the + // expected-exit log is gated on the monitor handler running + // *before* the next generation is installed, the log is + // silently dropped. destroyContainerExpectingExit must await + // the destroyed generation's monitor handler before + // returning so the caller can install the next generation + // without superseding the pending log. + // + // Real timers here — we need setTimeout to actually fire so + // the deferred reject straddles a task boundary the way + // production does. Restore fake timers at the end so the + // surrounding beforeEach/afterEach contract holds. + vi.useRealTimers(); + const info = vi.spyOn(console, "info").mockImplementation(() => {}); + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + + // Custom container where destroy() does NOT settle the + // monitor synchronously; instead it schedules the rejection + // for a later microtask, mirroring the platform's behavior. + let monitorReject: ((error: unknown) => void) | null = null; + let monitorPromise = new Promise((_, reject) => { + monitorReject = reject; + }); + monitorPromise.catch(() => {}); + const container = { + get running() { + return true; + }, + start() { + // New generation arms a fresh monitor promise. + monitorPromise = new Promise((_, reject) => { + monitorReject = reject; + }); + monitorPromise.catch(() => {}); + }, + async destroy() { + // Capture the current rejector; settle it on a macrotask + // so the destroy() resolution and the monitor rejection + // straddle a task boundary. This mirrors production + // timing — the platform's destroy can return before its + // monitor() promise settles, and microtask-only ordering + // (queueMicrotask, Promise.resolve) would mask the race. + const reject = monitorReject; + setTimeout(() => { + reject?.(new Error("deferred destroy reject")); + }, 0); + }, + monitor() { + return monitorPromise; + }, + } as unknown as NonNullable; + const ctx = makeContext(container); + resetContainerLifecycleForTests(ctx); + + container.start(); + installContainerMonitor(ctx, container); + + await destroyContainerExpectingExit(ctx, container); + // Immediately install the next generation, as restart() does. + container.start(); + installContainerMonitor(ctx, container); + + // Drain any pending tasks (including the macrotask the fake + // queued from destroy). + await new Promise((r) => setTimeout(r, 0)); + await Promise.resolve(); + + // The destroyed generation's exit must have logged as + // expected (info), not as a crash (warn). + expect(info).toHaveBeenCalledTimes(1); + expect(info.mock.calls[0]?.[0]).toMatchObject({ + message: "workspace.container.exited", + expected: true, + }); + expect(warn).not.toHaveBeenCalled(); + + vi.useFakeTimers(); + }); + test("a later real crash on a fresh generation logs as unexpected after a failed destroy", async () => { // destroy() rejects against generation 1. The expected-exit // mark sticks against generation 1. A subsequent start() diff --git a/packages/workspace/src/backends/container/container-lifecycle.ts b/packages/workspace/src/backends/container/container-lifecycle.ts index 8416c20f..f73896af 100644 --- a/packages/workspace/src/backends/container/container-lifecycle.ts +++ b/packages/workspace/src/backends/container/container-lifecycle.ts @@ -36,6 +36,13 @@ interface ContainerLifecycleState { // never affect the current generation, and a flag set against // generation N cannot leak into generation N+1. expectedExitGeneration: number | null; + // Resolves once the *current* generation's monitor handler has + // run to completion. destroyContainerExpectingExit awaits this + // so the expected-exit log is emitted before any subsequent + // installContainerMonitor bumps the generation out from under + // the in-flight handler. Null between generations and before + // the first install. + currentMonitorSettled: Promise | null; } const LIFECYCLE = new WeakMap(); @@ -43,7 +50,12 @@ const LIFECYCLE = new WeakMap(); export function getContainerLifecycle(ctx: DurableObjectState): ContainerLifecycleState { let state = LIFECYCLE.get(ctx); if (state === undefined) { - state = { exit: null, currentGeneration: 0, expectedExitGeneration: null }; + state = { + exit: null, + currentGeneration: 0, + expectedExitGeneration: null, + currentMonitorSettled: null, + }; LIFECYCLE.set(ctx, state); } return state; @@ -83,8 +95,13 @@ export function installContainerMonitor(ctx: DurableObjectState, container: Cont // Clear any prior exit info — a fresh generation has started. state.exit = null; - const promise = container.monitor(); - promise.then( + const monitorPromise = container.monitor(); + // currentMonitorSettled tracks the wrapper that runs recordExit, + // not the raw monitor promise, so awaiting it guarantees the + // exit has been recorded (and logged) before the awaiter + // continues. Both branches feed into recordExit which never + // throws, so the wrapper itself resolves rather than rejecting. + state.currentMonitorSettled = monitorPromise.then( () => recordExit(state, generation, undefined), (error) => recordExit(state, generation, error), ); @@ -108,7 +125,20 @@ export async function destroyContainerExpectingExit( ): Promise { const state = getContainerLifecycle(ctx); state.expectedExitGeneration = state.currentGeneration; + // Capture the current generation's monitor wrapper BEFORE the + // destroy. A subsequent installContainerMonitor reassigns + // currentMonitorSettled, but the capture here pins us to the + // one we're waiting on. + const settled = state.currentMonitorSettled; await container.destroy(); + // Wait for the destroyed generation's monitor handler to run so + // its expected-exit log fires before the caller (e.g. + // WorkspaceContainerAPI.restart) installs the next generation's + // monitor and bumps currentGeneration out from under it. The + // platform settles monitor() asynchronously relative to + // destroy(); without this await the next install supersedes the + // pending handler and recordExit drops the write as stale. + if (settled) await settled; } // Reset state for a ctx. Test-only escape hatch; the production From 3883606b463823f8d0a97ddd5b0b95069d0e5e75 Mon Sep 17 00:00:00 2001 From: aron <263346377+aron-cf@users.noreply.github.com> Date: Tue, 16 Jun 2026 19:46:38 +0000 Subject: [PATCH 11/11] workspace: identity-check the dispatch-time handle on shell errors WorkspaceShellRouter's #onShellError previously looked up the current cached handle for the backend id and immediately passed it to #invalidateHandle. The identity check inside #invalidateHandle (this.#handles.get(id) !== handle) was a tautology against the value just fetched from that same map, so the comparison always trivially failed and invalidation always fired. The window the bug opens: a long-running exec dispatches against handle A, A's WebSocket dies, A's closed promise fires, the Workspace drops A and the next operation rebuilds against handle B. Some time later A's event stream finally rejects with a transport error. The wrap around A's ExecHandle calls onError; without the identity check, B's slot is cleared, and the next operation pays a spurious reconnect against a still-good handle. Capture the BackendHandle at exec/get dispatch time, thread it through #wrapHandle into the error callback, and identity-check THAT handle against the live cache entry. A late rejection from a torn-down connection now sees A != B and no-ops. #shellFor now returns { shell, handle } together so the router gets both in one lookup; the WorkspaceShell stays cached by id and is always paired with the live handle for that id because #invalidateHandle clears both caches together. The push/pull invalidation path was already identity-checked correctly through #runWithInvalidation; only the shell path was broken. --- packages/workspace/src/workspace.test.ts | 94 ++++++++++++++++++++++++ packages/workspace/src/workspace.ts | 85 ++++++++++++--------- 2 files changed, 144 insertions(+), 35 deletions(-) diff --git a/packages/workspace/src/workspace.test.ts b/packages/workspace/src/workspace.test.ts index 7d732051..48b6e45e 100644 --- a/packages/workspace/src/workspace.test.ts +++ b/packages/workspace/src/workspace.test.ts @@ -958,4 +958,98 @@ describe("Workspace transport-failure invalidation", () => { await ws.shell.exec("true").catch(() => undefined); expect(connects).toBe(2); }); + + it("a late mid-stream rejection from an old handle does not clobber a newer one", async () => { + // Sequence under test: + // 1. exec dispatches against connection A. Its event stream + // stays pending. + // 2. connection A's `closed` promise fires — the Workspace + // drops its cached handle. Next exec reconnects to B. + // 3. connection A's stream finally rejects with a transport + // error (the late settle the production code is defending + // against). The wrap around A's handle must invalidate + // the entry only if it is still A; B must survive, and a + // subsequent operation must NOT trigger a third connect. + let connects = 0; + let signalClosedA: (() => void) | null = null; + // Per-exec stream controllers so each handle's stream can be + // settled independently. Indexed by exec call order. + const execStreams: ReadableStreamDefaultController< + import("@cloudflare/workspace-rpc").ExecEvent + >[] = []; + const shell: import("@cloudflare/workspace-rpc").ShellRPC = { + async exec(input) { + const execId = input.id ?? `exec-${execStreams.length}`; + return { + id: execId, + events: new ReadableStream({ + start(c) { + execStreams.push(c); + }, + }), + }; + }, + getExec: () => Promise.reject(new Error("not used")), + killExec: () => Promise.reject(new Error("not used")), + disposeExec: () => Promise.reject(new Error("not used")), + }; + const backend: WorkspaceBackend = { + id: "only", + type: "fake", + async connect(): Promise { + connects++; + const isFirst = connects === 1; + const closed = new Promise((resolve) => { + if (isFirst) signalClosedA = resolve; + }); + return { + rpc: { sync: fakeRpc(), shell }, + sync: "none", + closed, + close: async () => {}, + }; + }, + }; + const ws = new Workspace({ storage: makeStorage(), backends: [backend] }); + + // Dispatch against connection A; its stream stays pending. + const handleA = await ws.shell.exec("sleep 60"); + expect(connects).toBe(1); + const streamA = execStreams[0]; + + // Connection A's transport closes. The Workspace's `closed` + // watcher drops the cached handle on the next microtask. + signalClosedA?.(); + await new Promise((r) => setTimeout(r, 0)); + + // Next exec forces a fresh connection B. + const handleB = await ws.shell.exec("true"); + expect(connects).toBe(2); + const streamB = execStreams[1]; + + // NOW connection A's stream finally rejects. handleA.result() + // surfaces the rejection, and the wrap around handleA must + // identity-check against connection A's BackendHandle. The + // cache currently holds connection B; invalidation must be a + // no-op. + streamA?.error(new WorkspaceTransportError("WebSocket closed mid-stream")); + await expect(handleA.result()).rejects.toThrow(/WebSocket closed mid-stream/); + + // The next operation must reuse connection B — i.e. NOT + // reconnect again. Without the fix, A's late rejection would + // have invalidated B's slot and this third exec would force a + // third connect. + const handleC = await ws.shell.exec("true"); + expect(connects).toBe(2); + const streamC = execStreams[2]; + + // Settle B's and C's streams cleanly so the test doesn't leak + // pending readers. + streamB?.close(); + streamC?.close(); + await Promise.all([ + handleB.result().catch(() => undefined), + handleC.result().catch(() => undefined), + ]); + }); }); diff --git a/packages/workspace/src/workspace.ts b/packages/workspace/src/workspace.ts index f2ee2e12..ea693146 100644 --- a/packages/workspace/src/workspace.ts +++ b/packages/workspace/src/workspace.ts @@ -535,13 +535,16 @@ export class Workspace { } // Per-backend WorkspaceShell, constructed on demand and cached - // for the life of the handle. - async #shellFor(id: string): Promise { - const cached = this.#shells.get(id); - if (cached !== undefined) return cached; + // for the life of the handle. Returns both the shell and the + // BackendHandle it was built against so the caller can hold the + // handle reference for a later identity check; #invalidateHandle + // clears both caches together, so a shell pulled from #shells is + // always paired with the live handle for that id at the moment + // of the lookup. + async #shellFor(id: string): Promise<{ shell: WorkspaceShell; handle: BackendHandle }> { const handle = await this.#handleFor(id); - const existing = this.#shells.get(id); - if (existing !== undefined) return existing; + const cached = this.#shells.get(id); + if (cached !== undefined) return { shell: cached, handle }; const shell = new WorkspaceShell( handle.rpc.shell, { @@ -551,7 +554,7 @@ export class Workspace { this.#observer, ); this.#shells.set(id, shell); - return shell; + return { shell, handle }; } // Routed shell facade. Each method picks the right backend per @@ -562,19 +565,19 @@ export class Workspace { this.#defaultBackendId ?? "", (id) => this.#shellFor(id), (id) => this.#resolveBackendId(id) ?? "", - (id, error) => this.#onShellError(id, error), + (id, handle, error) => this.#onShellError(id, handle, error), ); return router as unknown as WorkspaceShell; } // Invalidate the cached handle for `id` when a shell-routed RPC - // fails with a known transport error. Matches the push/pull - // invalidation path; identity-checked so a concurrent reconnect - // is not clobbered. - #onShellError(id: string, error: unknown): void { + // fails with a known transport error. Compares the caller's + // captured handle against the live cache entry so a late-failing + // operation against an old handle can't clobber a newer one that + // a concurrent reconnect already installed. + #onShellError(id: string, handle: BackendHandle, error: unknown): void { if (!isWorkspaceTransportFailure(error)) return; - const handle = this.#handles.get(id); - if (handle !== undefined) this.#invalidateHandle(id, handle); + this.#invalidateHandle(id, handle); } } @@ -590,15 +593,15 @@ export class Workspace { // type through. class WorkspaceShellRouter { readonly #defaultId: string; - readonly #shellFor: (id: string) => Promise; + readonly #shellFor: (id: string) => Promise<{ shell: WorkspaceShell; handle: BackendHandle }>; readonly #resolveId: (id: string | undefined) => string; - readonly #onError: (id: string, error: unknown) => void; + readonly #onError: (id: string, handle: BackendHandle, error: unknown) => void; constructor( defaultId: string, - shellFor: (id: string) => Promise, + shellFor: (id: string) => Promise<{ shell: WorkspaceShell; handle: BackendHandle }>, resolveId: (id: string | undefined) => string, - onError: (id: string, error: unknown) => void, + onError: (id: string, handle: BackendHandle, error: unknown) => void, ) { this.#defaultId = defaultId; this.#shellFor = shellFor; @@ -608,36 +611,41 @@ class WorkspaceShellRouter { async exec(command: string, options: { backend?: string } & Record = {}) { const id = this.#resolveId(options.backend) || this.#defaultId; - const shell = await this.#shellFor(id); + // Capture the BackendHandle here, at dispatch time. A late + // failure from this command's stream must invalidate THIS + // handle, not whatever the cache holds when the rejection + // eventually fires; a concurrent reconnect may have already + // swapped in a newer handle that we must not clobber. + const { shell, handle: dispatchHandle } = await this.#shellFor(id); const { backend: _backend, ...rest } = options; - let handle: unknown; + let execHandle: unknown; try { - handle = await (shell.exec as unknown as (c: string, o: typeof rest) => Promise)( + execHandle = await (shell.exec as unknown as (c: string, o: typeof rest) => Promise)( command, rest, ); } catch (error) { - this.#onError(id, error); + this.#onError(id, dispatchHandle, error); throw error; } - return this.#wrapHandle(id, handle); + return this.#wrapHandle(id, dispatchHandle, execHandle); } async get(id: string, options: { backend?: string } & Record = {}) { const backendId = this.#resolveId(options.backend) || this.#defaultId; - const shell = await this.#shellFor(backendId); + const { shell, handle: dispatchHandle } = await this.#shellFor(backendId); const { backend: _backend, ...rest } = options; - let handle: unknown; + let execHandle: unknown; try { - handle = await (shell.get as unknown as (e: string, o: typeof rest) => Promise)( + execHandle = await (shell.get as unknown as (e: string, o: typeof rest) => Promise)( id, rest, ); } catch (error) { - this.#onError(backendId, error); + this.#onError(backendId, dispatchHandle, error); throw error; } - return this.#wrapHandle(backendId, handle); + return this.#wrapHandle(backendId, dispatchHandle, execHandle); } // Wrap an ExecHandle so a transport-classified rejection from @@ -647,22 +655,29 @@ class WorkspaceShellRouter { // transport mid-stream and the rejection surfaces through // result() draining the event stream. // + // The handle reference captured here is the one that was active + // when the exec was dispatched. By the time result() rejects, a + // concurrent reconnect may have replaced the cached entry for + // this id with a newer handle; invalidation in #onShellError + // checks identity against the dispatch-time handle so the newer + // entry survives. + // // WorkspaceShell installs .result via defineProperty; we set it // configurable: true so this slot can be redefined. The handle's // stream identity is preserved — callers that consume the // ReadableStream directly are unaffected; only result() routes // through the invalidation path. - #wrapHandle(id: string, handle: unknown): unknown { - const original = handle as { result?: unknown }; - if (typeof original.result !== "function") return handle; + #wrapHandle(id: string, dispatchHandle: BackendHandle, execHandle: unknown): unknown { + const original = execHandle as { result?: unknown }; + if (typeof original.result !== "function") return execHandle; const onError = this.#onError; - const originalResult = original.result.bind(handle) as () => Promise; - Object.defineProperty(handle, "result", { + const originalResult = original.result.bind(execHandle) as () => Promise; + Object.defineProperty(execHandle, "result", { value: async () => { try { return await originalResult(); } catch (error) { - onError(id, error); + onError(id, dispatchHandle, error); throw error; } }, @@ -670,6 +685,6 @@ class WorkspaceShellRouter { writable: false, configurable: true, }); - return handle; + return execHandle; } }