diff --git a/packages/workspace/src/backends/container/cloudflare-container.test.ts b/packages/workspace/src/backends/container/cloudflare-container.test.ts index d7a8c2ad..3fee7a0c 100644 --- a/packages/workspace/src/backends/container/cloudflare-container.test.ts +++ b/packages/workspace/src/backends/container/cloudflare-container.test.ts @@ -16,7 +16,16 @@ 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; + // Pre-set a prior exit reason so connect()'s pre-flight + // exitInfo() check observes it. + priorExit?: { exitedAt: number; reason: string } | null; } interface FakeHost { @@ -25,18 +34,41 @@ interface FakeHost { startEnv?: Record; interceptedHost?: string; interceptedWorkspace?: WorkspaceRef; + running: boolean; + exit: { exitedAt: number; reason: string } | null; + simulateExit(reason: string): void; } 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, + 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) { + return healthSequence.shift() ?? defaultHealthy; + } + return defaultHealthy; + } state.host = { async start(env) { 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] }); @@ -48,7 +80,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 +94,23 @@ 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; + state.exit = null; + }, + async status() { + calls.push({ name: "status", args: [] }); + return { running: state.running, exit: state.exit }; + }, + async exitInfo() { + calls.push({ name: "exitInfo", args: [] }); + return state.exit; + }, + } satisfies IWorkspaceContainerAPI; return state; } @@ -75,9 +123,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 +233,113 @@ describe("CloudflareContainerBackend", () => { const res = await backend.handleFetch(new Request("http://workspace.internal/ws")); 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. + // 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 40afcbcf..fc5df7df 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 @@ -100,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 @@ -111,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"; @@ -141,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, }; } @@ -151,11 +182,20 @@ export class CloudflareContainerBackend implements WorkspaceBackend { const holder = await this.#options.container(); const host = await holder.getWorkspaceContainer(); - await host.start({ + // 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", ...this.#options.containerEnv, - }); + }; + await host.start(env); await host.interceptOutboundHttp(this.#options.egressHost, this.#options.workspace); // Arm the upgrade promise before posting /connect — wsd @@ -163,7 +203,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, priorExit); await this.#postConnect(host, deadline); const ws = await this.#waitForUpgrade(deadline); @@ -289,24 +329,116 @@ 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, + priorExit: { exitedAt: number; reason: string } | null, + ): Promise { + const maxAttempts = this.#options.restartAttempts + 1; + // Split the remaining time across attempts so a failing + // 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(250, 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, + priorExit, + }), + 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 { - 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( + this.#options.healthProbeTimeoutMs, + Math.max(50, deadline - Date.now()), + ), }); - void res.body?.cancel(); 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; + 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}${priorExit} ` + + `lastError=${describeError(info.lastError)}` ); } @@ -325,13 +457,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}`, + ); } } @@ -349,7 +486,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..fffe09f9 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 { + type ContainerExitInfo, + containerExitInfo, + destroyContainerExpectingExit, + installContainerMonitor, +} 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. @@ -35,8 +44,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 +62,29 @@ 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. `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 @@ -73,8 +105,50 @@ export class WorkspaceContainerAPI extends RpcTarget implements IWorkspaceContai } async start(env: Record) { - if (this.#container.running) return; + // 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. + } + this.#container.start({ enableInternet: true, env }); + } else if (!this.#container.running) { + this.#container.start({ enableInternet: true, env }); + } + installContainerMonitor(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. + // 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 }); + installContainerMonitor(this.#ctx, this.#container); + } + + async status() { + return { running: this.#container.running, exit: containerExitInfo(this.#ctx) }; + } + + async exitInfo(): Promise { + return containerExitInfo(this.#ctx); } async interceptOutboundHttp(host: string, ref: WorkspaceRef) { @@ -91,6 +165,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..ee8f6968 --- /dev/null +++ b/packages/workspace/src/backends/container/container-lifecycle.test.ts @@ -0,0 +1,420 @@ +import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; + +import { + containerExitInfo, + destroyContainerExpectingExit, + formatExitReason, + getContainerLifecycle, + installContainerMonitor, + resetContainerLifecycleForTests, +} from "./container-lifecycle.js"; + +// Minimal Container stand-in. The lifecycle module only touches +// .destroy(), .start(), .running, and .monitor(). A controllable +// 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; + current: MonitorControls; + generations: MonitorControls[]; +} { + let starts = 0; + let destroys = 0; + let monitorCalls = 0; + let running = false; + 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. + promise.catch(() => {}); + const controls: MonitorControls = { resolve, reject, promise }; + generations.push(controls); + return controls; + } + let current = 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. + current = armPromise(); + }, + async destroy() { + destroys++; + running = false; + // 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 current.promise; + }, + } as unknown as NonNullable; + + return { + container, + get starts() { + return starts; + }, + get destroys() { + return destroys; + }, + get monitorCalls() { + return monitorCalls; + }, + get current() { + return current; + }, + generations, + } as ReturnType; +} + +// Lifecycle state is keyed by ctx — a tiny opaque object suffices. +function makeContext(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("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 = makeContext(fake.container); + resetContainerLifecycleForTests(ctx); + fake.container.start(); + installContainerMonitor(ctx, fake.container); + + expect(containerExitInfo(ctx)).toBeNull(); + fake.current.resolve(); + // 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 = makeContext(fake.container); + resetContainerLifecycleForTests(ctx); + fake.container.start(); + installContainerMonitor(ctx, fake.container); + + fake.current.reject(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 = makeContext(fake.container); + resetContainerLifecycleForTests(ctx); + fake.container.start(); + installContainerMonitor(ctx, fake.container); + + fake.current.reject(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 () => { + // 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(); + const ctx = makeContext(fake.container); + resetContainerLifecycleForTests(ctx); + fake.container.start(); + installContainerMonitor(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, + reason: "container destroyed", + }); + }); + + 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 = makeContext(fake.container); + resetContainerLifecycleForTests(ctx); + + fake.container.start(); + const firstGeneration = fake.current; + installContainerMonitor(ctx, fake.container); + + // Second generation — a new monitor promise is armed in the + // fake's start(); installContainerMonitor bumps the lifecycle's + // generation counter and attaches a fresh handler against the + // new promise. + fake.container.start(); + installContainerMonitor(ctx, fake.container); + 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("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 = makeContext(fake.container); + resetContainerLifecycleForTests(ctx); + fake.container.start(); + 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(); + installContainerMonitor(ctx, fake.container); + fake.current.reject(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("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() + // 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 = makeContext(fake.container); + resetContainerLifecycleForTests(ctx); + fake.container.start(); + installContainerMonitor(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/); + + // Fresh generation, fresh monitor. + fake.container.start(); + installContainerMonitor(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 }); + }); +}); + +describe("getContainerLifecycle", () => { + test("returns null exit info before any monitor has fired", () => { + const fake = makeContainer(); + const ctx = makeContext(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 = makeContext(a.container); + const ctxB = makeContext(b.container); + resetContainerLifecycleForTests(ctxA); + resetContainerLifecycleForTests(ctxB); + a.container.start(); + installContainerMonitor(ctxA, a.container); + b.container.start(); + installContainerMonitor(ctxB, b.container); + + a.current.reject(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..f73896af --- /dev/null +++ b/packages/workspace/src/backends/container/container-lifecycle.ts @@ -0,0 +1,181 @@ +// 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. + // 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; + // Monotonically incremented every time a new monitor is armed. + // The installContainerMonitor 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; + // 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(); + +export function getContainerLifecycle(ctx: DurableObjectState): ContainerLifecycleState { + let state = LIFECYCLE.get(ctx); + if (state === undefined) { + state = { + exit: null, + currentGeneration: 0, + expectedExitGeneration: null, + currentMonitorSettled: null, + }; + 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. +// 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. +// +// 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 installContainerMonitor(ctx: DurableObjectState, container: ContainerHandle): void { + const state = getContainerLifecycle(ctx); + state.currentGeneration += 1; + const generation = state.currentGeneration; + // Clear any prior exit info — a fresh generation has started. + state.exit = null; + + 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), + ); +} + +// 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: ContainerHandle, +): 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 +// path relies on WeakMap GC. +export function resetContainerLifecycleForTests(ctx: DurableObjectState): void { + LIFECYCLE.delete(ctx); +} + +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 }; + // 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, + }); + } +} 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..327acf9c --- /dev/null +++ b/packages/workspace/src/backends/container/health-probe.test.ts @@ -0,0 +1,71 @@ +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 { + const host: HealthProbeHost = { + fetchPort: vi.fn(handler), + }; + return host 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}`); + } +} 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/transport-failure.test.ts b/packages/workspace/src/transport-failure.test.ts new file mode 100644 index 00000000..aa175a5b --- /dev/null +++ b/packages/workspace/src/transport-failure.test.ts @@ -0,0 +1,75 @@ +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); + // 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", () => { + 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("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); + 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..5dbe8cba --- /dev/null +++ b/packages/workspace/src/transport-failure.ts @@ -0,0 +1,97 @@ +// 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. 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 stub after it has been disposed/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, + // 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 { + 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) { + // .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; + } + 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..48b6e45e 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,303 @@ 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); + }); + + 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); + }); + + 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 3ee2aaf1..ea693146 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 @@ -500,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, { @@ -516,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 @@ -527,9 +565,20 @@ export class Workspace { this.#defaultBackendId ?? "", (id) => this.#shellFor(id), (id) => this.#resolveBackendId(id) ?? "", + (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. 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; + this.#invalidateHandle(id, handle); + } } // Selector wrapper that satisfies the WorkspaceShell surface but @@ -544,30 +593,98 @@ 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, 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, handle: BackendHandle, 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); + // 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; - return (shell.exec as unknown as (c: string, o: typeof rest) => unknown)(command, rest); + let execHandle: unknown; + try { + execHandle = await (shell.exec as unknown as (c: string, o: typeof rest) => Promise)( + command, + rest, + ); + } catch (error) { + this.#onError(id, dispatchHandle, error); + throw error; + } + 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; - return (shell.get as unknown as (e: string, o: typeof rest) => unknown)(id, rest); + let execHandle: unknown; + try { + execHandle = await (shell.get as unknown as (e: string, o: typeof rest) => Promise)( + id, + rest, + ); + } catch (error) { + this.#onError(backendId, dispatchHandle, error); + throw error; + } + return this.#wrapHandle(backendId, dispatchHandle, execHandle); + } + + // 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. + // + // 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, 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(execHandle) as () => Promise; + Object.defineProperty(execHandle, "result", { + value: async () => { + try { + return await originalResult(); + } catch (error) { + onError(id, dispatchHandle, error); + throw error; + } + }, + enumerable: false, + writable: false, + configurable: true, + }); + return execHandle; } }