workspace: improve container lifecycle - #9
Merged
Merged
Conversation
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: <inner>' 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.
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.
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.
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().
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.
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.
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<IWorkspaceContainerAPI, "fetchPort"> 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.
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.
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.
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.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The container backend used to assume that once
connect()had succeeded the WebSocket session towsdwould either work or close cleanly. Neither half holds in practice. A wedged capnweb session, awsdhealth probe failing, or a container that died without sending a clean close frame all leave theWorkspaceholding a cachedBackendHandlewhose underlying RPC stub is dead. Every subsequentpush,pull, orshell.execreuses that stub. The existinghandle.closedwatcher only fires on a clean close, so the failure modes that matter in production never trigger a reconnect.This change makes the cache react to transport failures wherever they surface and gives the container backend first-class signals for pre-flight readiness and live container exits. A workspace operation that hits a dead transport now produces a single classified error, drops the stale cached handle, and the next operation reconnects against a fresh container generation.
sequenceDiagram autonumber participant W as Workspace participant B as ContainerBackend participant H as WorkspaceContainerAPI participant C as Container rect rgba(200, 50, 50, 0.08) Note over W,C: Before — single connect path, no recovery W->>B: connect() B->>H: start(env) B->>H: waitForPort (250ms loop) B->>H: POST /connect B-->>W: BackendHandle (cached) Note over W,C: container crashes mid-session C--xB: WebSocket frames stop W->>B: push() / shell.exec() B-->>W: rejects with dead RPC stub W->>B: next push() / shell.exec() B-->>W: same dead RPC stub, again end rect rgba(50, 150, 50, 0.08) Note over W,C: After — classified failures + monitor() + bounded restart W->>B: connect() B->>H: exitInfo() B->>H: start(env) H->>C: container.monitor() Note over H,C: armed per generation B->>H: probeWsdHealth (bounded restart loop) alt readiness fails B->>H: restart(env) H->>C: destroy() + start() B->>H: probeWsdHealth end B->>H: POST /connect B-->>W: BackendHandle (cached) Note over W,C: container crashes mid-session C-->>H: monitor() rejects → exitInfo recorded W->>B: push() / shell.exec() B->>H: fetchPort → WorkspaceTransportError("container exited: ...") B-->>W: classified failure W->>W: drop cached handle + shell W->>B: next push() / shell.exec() B->>H: fresh start(), fresh monitor, fresh session endWorkspace cache invalidation is driven by an
isWorkspaceTransportFailureclassifier that walks the cause chain and matches against three signals: aWorkspaceTransportErrortag class for failures the workspace stack raises itself, the.namefield on a cloned error (which survives a Workers RPC structured-clone hop even when the subclass identity does not), and a small list of patterns for capnweb session shutdown, WebSocket close, container-port unreachable, and the container-exited short-circuit. The classifier is applied at three boundaries:Workspace.push(),Workspace.pull(), and theWorkspaceShellrouter'sexec()/get()paths. The router also rewraps the returnedExecHandle.result()so that a long-running command that loses its transport mid-stream — the realistic failure mode — drops the cached handle whenresult()rejects, not when the originalexec()call returns.Startup readiness is no longer a private
setTimeoutloop hittinghost.fetchPortdirectly. The sharedprobeWsdHealthhelper takes the host, port, path, and per-probe timeout, andAbortSignal.timeoutaborts a probe that the runtime accepts but never answers. Withinconnect()it runs in a backoff loop bounded by a per-attempt budget; a failed attempt callshost.restart(env)while restart attempts remain. Failures throw a stage-tagged error that names the stage, port, attempt number, restart count, overall timeout, and the last underlying error. Whenhost.exitInfo()reports a prior container exit, the error carries the prior reason as an attribute too.Container exits become a first-class signal through
container.monitor().WorkspaceContainerAPIarms the monitor on every successfulstart(), tags each armed monitor with a generation counter, and snapshots the expected-exit state in a per-generation slot so a destroy that fires while a different generation is in flight cannot mis-classify the wrong generation's exit. The monitor handler bails out if its generation is no longer the current one, so a late-resolving monitor from a torn-down generation cannot overwrite a fresh generation's clean state. Exits log through a single structuredconsole.warn(unexpected) orconsole.info(expected) call so Cloudflare Logs picks up the fields.fetchPortshort-circuits once an exit has been recorded, throwing aWorkspaceTransportError("container exited: <reason>")that the classifier picks up.Run the workspace tests from the repository root:
The new behavior is exercised by 443 vitest cases across thirty-one files. The lifecycle module is unit-tested with an in-process container fake that mirrors the real
monitor()contract (rejects ondestroy()because the platform usesSIGKILL; resolves only on a clean code-zero exit). The fake exposes per-generation resolve and reject controls so a test can fire the first generation's settle frame after a second generation has been armed — the scenario the production code is defending against. The successful WebSocket round trip still cannot run under the Node test runner becauseWebSocketPairis a workerd global, and the example container app atexamples/containerremains the smallest end-to-end check for that path.The container backend section of
packages/workspace/README.mdis unchanged; the new behaviors fit inside the existing public surface. One small contract relaxation onExecHandle: theresultandkillproperty descriptors are nowconfigurable: trueso the Workspace router can redefineresult()to wire its invalidation path. The handle'sidslot stays non-configurable.Two follow-ups remain. A workerd-backed test under
vitest.config.worker-backend.tswould close the unit-test gap on the successfulconnectpath —WebSocketPairis a workerd global and the Node runner cannot reach it. Andcontainer.setInactivityTimeoutis not wired up, so a brief Durable Object hibernation between operations still risks reclaiming the container; with the recovery path in place the cost is a freshstart()rather than a stale-handle crash, but a small inactivity timeout would close that window cheaply.