From c6ee9efc894275646dcdfdc7a6b008e768c1eff8 Mon Sep 17 00:00:00 2001 From: Aron <263346377+aron-cf@users.noreply.github.com> Date: Wed, 1 Jul 2026 10:57:04 +0100 Subject: [PATCH 01/24] test: reproduce RPC idle disconnect during pending call --- .../sandbox/tests/rpc-sandbox-client.test.ts | 62 ++++++++++++++++++- 1 file changed, 59 insertions(+), 3 deletions(-) diff --git a/packages/sandbox/tests/rpc-sandbox-client.test.ts b/packages/sandbox/tests/rpc-sandbox-client.test.ts index dd9599986..de3f97e2c 100644 --- a/packages/sandbox/tests/rpc-sandbox-client.test.ts +++ b/packages/sandbox/tests/rpc-sandbox-client.test.ts @@ -13,6 +13,11 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; let stats = { imports: 1, exports: 1 }; let connected = true; const disconnects: number[] = []; +let commandExecuteImpl: (...args: unknown[]) => unknown = () => ({ + exitCode: 0, + stdout: '', + stderr: '' +}); /** * onClose callbacks installed by `ContainerControlClient` on the active * mock connection. The client's `getConnection()` wires this so the WS @@ -52,9 +57,14 @@ vi.mock('../src/container-control/connection', () => ({ disconnects.push(Date.now()); } rpc() { - // Stub sub-clients so wrapStub() has something to Proxy. Tests in - // this file don't actually invoke any RPC method. - return new Proxy({}, { get: () => ({}) }); + return new Proxy( + { + commands: { + execute: (...args: unknown[]) => commandExecuteImpl(...args) + } + }, + { get: (target, prop) => Reflect.get(target, prop) ?? {} } + ); } async connect() {} } @@ -72,6 +82,7 @@ describe('ContainerControlClient busy/idle tracking', () => { connected = true; disconnects.length = 0; onCloseHandlers.length = 0; + commandExecuteImpl = () => ({ exitCode: 0, stdout: '', stderr: '' }); }); afterEach(() => { @@ -224,6 +235,51 @@ describe('ContainerControlClient busy/idle tracking', () => { expect(disconnects).toHaveLength(1); expect(onSessionIdle).toHaveBeenCalledTimes(1); }); + + it('does not idle-disconnect while an RPC method promise is pending even when stats look idle', async () => { + let resolveExecute!: (value: unknown) => void; + commandExecuteImpl = vi.fn( + () => + new Promise((resolve) => { + resolveExecute = resolve; + }) + ); + + const onActivity = vi.fn(); + const onSessionBusy = vi.fn(); + const onSessionIdle = vi.fn(); + + const client = new ContainerControlClient({ + stub: { fetch: vi.fn() }, + onActivity, + onSessionBusy, + onSessionIdle, + busyPollIntervalMs: 1_000, + idleDisconnectMs: 1_000 + }); + + const pending = client.commands.execute('sleep 10', 'default'); + + // Reproduce the race from sandbox-sdk#794: capnweb stats can report the + // bootstrap baseline while the method promise is still pending. The old + // implementation treated that as idle, armed the 1s disconnect timer, and + // disposed the main stub while the operation was still in flight. + stats = { imports: 1, exports: 1 }; + + vi.advanceTimersByTime(5_000); + expect(disconnects).toHaveLength(0); + expect(onSessionBusy).toHaveBeenCalledTimes(1); + expect(onSessionIdle).not.toHaveBeenCalled(); + + resolveExecute({ exitCode: 0, stdout: '', stderr: '' }); + await pending; + await Promise.resolve(); + + expect(onSessionIdle).toHaveBeenCalledTimes(1); + + vi.advanceTimersByTime(1_000); + expect(disconnects).toHaveLength(1); + }); }); describe('translateRPCError', () => { From 5fc1224cd73012d087b9438ff0ad931174e4bd86 Mon Sep 17 00:00:00 2001 From: Aron <263346377+aron-cf@users.noreply.github.com> Date: Wed, 1 Jul 2026 10:57:05 +0100 Subject: [PATCH 02/24] fix: keep RPC connection busy while calls are pending --- .changeset/rpc-method-tracking.md | 5 + .../sandbox/src/container-control/client.ts | 130 ++++++++++++------ 2 files changed, 96 insertions(+), 39 deletions(-) create mode 100644 .changeset/rpc-method-tracking.md diff --git a/.changeset/rpc-method-tracking.md b/.changeset/rpc-method-tracking.md new file mode 100644 index 000000000..4aa35b635 --- /dev/null +++ b/.changeset/rpc-method-tracking.md @@ -0,0 +1,5 @@ +--- +'@cloudflare/sandbox': patch +--- + +Keep RPC sessions alive while method calls are pending to avoid intermittent `RPC session was shut down by disposing the main stub` failures during concurrent sandbox startup. diff --git a/packages/sandbox/src/container-control/client.ts b/packages/sandbox/src/container-control/client.ts index 0a107b41c..5c37b4ffb 100644 --- a/packages/sandbox/src/container-control/client.ts +++ b/packages/sandbox/src/container-control/client.ts @@ -373,20 +373,22 @@ function buildInterruptedOperationResponse( * from the JSON wire format into typed SandboxError instances and signals * activity at call start. * - * `onCallStarted` fires synchronously when an RPC method is invoked. The - * ContainerControlClient uses this to renew the DO's activity timeout - * immediately, so even a call that completes entirely between two - * busy-poll ticks still pushes the sleepAfter deadline forward. + * `onCallStarted` fires synchronously when an RPC method is invoked, and + * `onCallSettled` fires when the returned promise settles. The + * ContainerControlClient uses these hooks to keep the session marked busy + * even if capnweb stats briefly report the bootstrap baseline while a call is + * still pending. * - * Note: there is no `onCallSettled` hook. A method whose returned promise - * resolves with a `ReadableStream` is *not* finished when the promise - * settles — capnweb keeps the export alive until the stream ends. The - * busy/idle poll on `getStats()` is the source of truth for that. + * A method whose returned promise resolves with a `ReadableStream` is *not* + * finished when the promise settles — capnweb keeps the export alive until + * the stream ends. The busy/idle poll on `getStats()` remains the source of + * truth for stream lifetimes after the initial RPC promise settles. */ function wrapStub( stub: T, domain: string, - onCallStarted: () => void + onCallStarted: () => void, + onCallSettled: () => void ): T { return new Proxy(stub, { get(target, prop, receiver) { @@ -410,12 +412,14 @@ function wrapStub( result != null && typeof (result as { then?: unknown }).then === 'function' ) { - return (result as Promise).catch((err: unknown) => - translateRPCError(err, { operation }) - ); + return (result as Promise) + .catch((err: unknown) => translateRPCError(err, { operation })) + .finally(onCallSettled); } + onCallSettled(); return result; } catch (err) { + onCallSettled(); translateRPCError(err, { operation }); } }; @@ -480,6 +484,8 @@ export class ContainerControlClient { private conn: ContainerControlConnection | null = null; private idleTimer: ReturnType | null = null; private busyPollTimer: ReturnType | null = null; + /** Number of RPC method promises that have started but not settled. */ + private activeCalls = 0; /** Tracks whether we currently believe the session is busy. */ private busy = false; @@ -531,15 +537,58 @@ export class ContainerControlClient { // Activity & busy/idle tracking // ------------------------------------------------------------------------- + private markBusy(): void { + if (!this.busy) { + this.busy = true; + this.onSessionBusy?.(); + } + this.clearIdleTimer(); + } + + private isSessionBusy(conn: ContainerControlConnection): boolean { + const { imports, exports } = conn.getStats(); + return ( + this.activeCalls > 0 || + imports > IDLE_IMPORT_THRESHOLD || + exports > IDLE_EXPORT_THRESHOLD + ); + } + + private maybeTransitionIdle(): void { + const conn = this.conn; + if (!conn || !conn.isConnected()) return; + + if (this.isSessionBusy(conn)) { + this.markBusy(); + return; + } + + if (this.busy) { + this.busy = false; + this.onSessionIdle?.(); + this.scheduleIdleDisconnect(); + } else if (!this.idleTimer) { + this.scheduleIdleDisconnect(); + } + } + /** * Called synchronously at the start of each RPC method invocation. * Renews the DO activity timeout so the sleepAfter alarm is pushed - * forward before the container processes the call. + * forward before the container processes the call, and pins the RPC + * WebSocket as busy until the method's promise settles. */ - private renewActivity = (): void => { + private recordCallStarted = (): void => { + this.activeCalls++; + this.markBusy(); this.onActivity?.(); }; + private recordCallSettled = (): void => { + this.activeCalls = Math.max(0, this.activeCalls - 1); + this.maybeTransitionIdle(); + }; + /** * Sample `getStats()` and update busy/idle state. While busy, renews the * activity timeout each tick so an in-flight stream keeps pushing the @@ -567,19 +616,11 @@ export class ContainerControlClient { return; } - const { imports, exports } = conn.getStats(); - const isBusy = - imports > IDLE_IMPORT_THRESHOLD || exports > IDLE_EXPORT_THRESHOLD; - - if (isBusy) { - if (!this.busy) { - this.busy = true; - this.onSessionBusy?.(); - } + if (this.isSessionBusy(conn)) { + this.markBusy(); // Renew on every busy tick — this is what keeps a long-lived stream // alive past sleepAfter. this.onActivity?.(); - this.clearIdleTimer(); } else if (this.busy) { this.busy = false; this.onSessionIdle?.(); @@ -615,11 +656,7 @@ export class ContainerControlClient { if (!conn || !conn.isConnected()) return; // Re-check before disconnecting — a new call may have started. - const { imports, exports } = conn.getStats(); - if ( - imports <= IDLE_IMPORT_THRESHOLD && - exports <= IDLE_EXPORT_THRESHOLD - ) { + if (!this.isSessionBusy(conn)) { this.logger.debug('Disconnecting idle RPC connection'); this.destroyConnection(); } @@ -636,6 +673,7 @@ export class ContainerControlClient { private destroyConnection(): void { this.stopBusyPoll(); this.clearIdleTimer(); + this.activeCalls = 0; // If we tear down while still believing the session is busy, fire the // idle transition so the DO's inflight counter doesn't leak. if (this.busy) { @@ -661,66 +699,80 @@ export class ContainerControlClient { return wrapStub( this.getConnection().rpc().commands, 'commands', - this.renewActivity + this.recordCallStarted, + this.recordCallSettled ); } get files(): SandboxFilesAPI { return wrapStub( this.getConnection().rpc().files, 'files', - this.renewActivity + this.recordCallStarted, + this.recordCallSettled ) as unknown as SandboxFilesAPI; } get processes(): SandboxProcessesAPI { return wrapStub( this.getConnection().rpc().processes, 'processes', - this.renewActivity + this.recordCallStarted, + this.recordCallSettled ); } get ports(): SandboxPortsAPI { return wrapStub( this.getConnection().rpc().ports, 'ports', - this.renewActivity + this.recordCallStarted, + this.recordCallSettled ); } get git(): SandboxGitAPI { - return wrapStub(this.getConnection().rpc().git, 'git', this.renewActivity); + return wrapStub( + this.getConnection().rpc().git, + 'git', + this.recordCallStarted, + this.recordCallSettled + ); } get utils(): SandboxUtilsAPI { return wrapStub( this.getConnection().rpc().utils, 'utils', - this.renewActivity + this.recordCallStarted, + this.recordCallSettled ); } get backup(): SandboxBackupAPI { return wrapStub( this.getConnection().rpc().backup, 'backup', - this.renewActivity + this.recordCallStarted, + this.recordCallSettled ); } get watch(): SandboxWatchAPI { return wrapStub( this.getConnection().rpc().watch, 'watch', - this.renewActivity + this.recordCallStarted, + this.recordCallSettled ); } get tunnels(): SandboxTunnelsAPI { return wrapStub( this.getConnection().rpc().tunnels, 'tunnels', - this.renewActivity + this.recordCallStarted, + this.recordCallSettled ); } get interpreter(): SandboxInterpreterAPI { return wrapStub( this.getConnection().rpc().interpreter, 'interpreter', - this.renewActivity + this.recordCallStarted, + this.recordCallSettled ); } From f95c2d7b1c56135263f8e6899f4661549f3ec208 Mon Sep 17 00:00:00 2001 From: Aron <263346377+aron-cf@users.noreply.github.com> Date: Wed, 1 Jul 2026 12:20:28 +0100 Subject: [PATCH 03/24] Surface container allocation failures as ContainerUnavailableError The Containers platform throws "no container instance" / "max instances exceeded" during startup, which capnweb masked as a generic utils.createSession interruption. Detect these platform errors, convert them to typed ContainerUnavailableError, and retry them within the existing startup budget. --- .changeset/surface-container-unavailable.md | 5 + .../sandbox/src/container-control/client.ts | 104 +++++++-- .../src/container-control/connection.ts | 117 ++++++++++- packages/sandbox/src/response-retry.ts | 43 +++- .../tests/container-connection.test.ts | 197 ++++++++++++++++++ packages/sandbox/tests/response-retry.test.ts | 86 ++++++++ .../sandbox/tests/rpc-sandbox-client.test.ts | 86 ++++++++ packages/shared/src/errors/contexts.ts | 20 +- 8 files changed, 639 insertions(+), 19 deletions(-) create mode 100644 .changeset/surface-container-unavailable.md diff --git a/.changeset/surface-container-unavailable.md b/.changeset/surface-container-unavailable.md new file mode 100644 index 000000000..93e5acbb3 --- /dev/null +++ b/.changeset/surface-container-unavailable.md @@ -0,0 +1,5 @@ +--- +'@cloudflare/sandbox': patch +--- + +Surface container allocation failures as `ContainerUnavailableError` instead of masking them as generic `utils.createSession` interruptions. When the Containers platform cannot admit a container during startup ("There is no container instance that can be provided to this Durable Object" or "Maximum number of running container instances exceeded"), callers now receive the real, retryable cause with structured context. diff --git a/packages/sandbox/src/container-control/client.ts b/packages/sandbox/src/container-control/client.ts index 5c37b4ffb..fa69972a3 100644 --- a/packages/sandbox/src/container-control/client.ts +++ b/packages/sandbox/src/container-control/client.ts @@ -167,6 +167,14 @@ interface RPCErrorPayload { export interface RPCTranslationContext { /** Public operation name, e.g. `commands.execute` or `files.writeFile`. */ operation?: string; + /** + * Error captured by `ContainerControlConnection` during connection startup + * (e.g. a platform container-allocation failure). When the RPC call rejects + * with a generic capnweb disposal / connection-failure error caused by that + * same startup abort, this captured error is the real, actionable cause and + * is preferred over the masking transport error. + */ + connectionError?: unknown; } export function translateRPCError( @@ -230,6 +238,12 @@ export function translateRPCError( // errors so consumers can branch on public SDK contracts instead of // substring-matching transport internals. const transportResponse = buildTransportErrorResponse(error); + // If this call rejected because a connection-startup failure aborted the + // transport, prefer that captured error — it carries the real, actionable + // cause (e.g. CONTAINER_UNAVAILABLE) instead of the generic disposal / + // connection-failure message capnweb raises on the queued call. + const captured = maybePreferConnectionError(transportResponse, context); + if (captured) throw captured; const interruptedResponse = buildInterruptedOperationResponse( transportResponse, context @@ -330,6 +344,34 @@ function buildTransportErrorResponse( }; } +/** + * When a queued RPC call rejects with a transport error that a connection + * abort could have caused (session disposed, connection failed, upgrade + * failed, or peer closed), and the connection captured a real startup error, + * return that captured error. `SandboxError` instances (e.g. the typed + * ContainerUnavailableError) are surfaced as-is; anything else is returned + * untouched so the caller falls back to normal transport classification. + */ +function maybePreferConnectionError( + transportResponse: ErrorResponse, + context: RPCTranslationContext +): SandboxError | null { + if (!context.connectionError) return null; + const { kind } = transportResponse.context; + if ( + kind !== 'session_disposed' && + kind !== 'connection_failed' && + kind !== 'upgrade_failed' && + kind !== 'peer_closed' + ) { + return null; + } + if (context.connectionError instanceof SandboxError) { + return context.connectionError; + } + return null; +} + function buildInterruptedOperationResponse( transportResponse: ErrorResponse, context: RPCTranslationContext @@ -388,7 +430,8 @@ function wrapStub( stub: T, domain: string, onCallStarted: () => void, - onCallSettled: () => void + onCallSettled: () => void, + getConnectionError: () => unknown ): T { return new Proxy(stub, { get(target, prop, receiver) { @@ -413,14 +456,22 @@ function wrapStub( typeof (result as { then?: unknown }).then === 'function' ) { return (result as Promise) - .catch((err: unknown) => translateRPCError(err, { operation })) + .catch((err: unknown) => + translateRPCError(err, { + operation, + connectionError: getConnectionError() + }) + ) .finally(onCallSettled); } onCallSettled(); return result; } catch (err) { onCallSettled(); - translateRPCError(err, { operation }); + translateRPCError(err, { + operation, + connectionError: getConnectionError() + }); } }; } @@ -482,6 +533,13 @@ export class ContainerControlClient { private readonly onSessionIdle: (() => void) | undefined; private conn: ContainerControlConnection | null = null; + /** + * Real cause captured by the connection during startup failure (e.g. a + * platform container-allocation error). Preferred over the generic capnweb + * disposal error when translating queued RPC rejections. Cleared each time a + * fresh connection is created. + */ + private lastConnectionError: unknown = null; private idleTimer: ReturnType | null = null; private busyPollTimer: ReturnType | null = null; /** Number of RPC method promises that have started but not settled. */ @@ -505,6 +563,12 @@ export class ContainerControlClient { // every in-flight RPC rejects with a peer-closed error. onClose: () => { if (this.conn) this.destroyConnection(); + }, + // Capture the real startup failure before capnweb masks it on the + // queued RPC rejections. `translateRPCError` prefers this over the + // generic disposal / connection-failure transport error. + onConnectionError: (error: unknown) => { + this.lastConnectionError = error; } }; this.idleDisconnectMs = @@ -527,6 +591,7 @@ export class ContainerControlClient { */ private getConnection(): ContainerControlConnection { if (!this.conn) { + this.lastConnectionError = null; this.conn = new ContainerControlConnection(this.connOptions); this.startBusyPoll(); } @@ -589,6 +654,9 @@ export class ContainerControlClient { this.maybeTransitionIdle(); }; + /** Return the last connection-startup error captured, if any. */ + private getLastConnectionError = (): unknown => this.lastConnectionError; + /** * Sample `getStats()` and update busy/idle state. While busy, renews the * activity timeout each tick so an in-flight stream keeps pushing the @@ -700,7 +768,8 @@ export class ContainerControlClient { this.getConnection().rpc().commands, 'commands', this.recordCallStarted, - this.recordCallSettled + this.recordCallSettled, + this.getLastConnectionError ); } get files(): SandboxFilesAPI { @@ -708,7 +777,8 @@ export class ContainerControlClient { this.getConnection().rpc().files, 'files', this.recordCallStarted, - this.recordCallSettled + this.recordCallSettled, + this.getLastConnectionError ) as unknown as SandboxFilesAPI; } get processes(): SandboxProcessesAPI { @@ -716,7 +786,8 @@ export class ContainerControlClient { this.getConnection().rpc().processes, 'processes', this.recordCallStarted, - this.recordCallSettled + this.recordCallSettled, + this.getLastConnectionError ); } get ports(): SandboxPortsAPI { @@ -724,7 +795,8 @@ export class ContainerControlClient { this.getConnection().rpc().ports, 'ports', this.recordCallStarted, - this.recordCallSettled + this.recordCallSettled, + this.getLastConnectionError ); } get git(): SandboxGitAPI { @@ -732,7 +804,8 @@ export class ContainerControlClient { this.getConnection().rpc().git, 'git', this.recordCallStarted, - this.recordCallSettled + this.recordCallSettled, + this.getLastConnectionError ); } get utils(): SandboxUtilsAPI { @@ -740,7 +813,8 @@ export class ContainerControlClient { this.getConnection().rpc().utils, 'utils', this.recordCallStarted, - this.recordCallSettled + this.recordCallSettled, + this.getLastConnectionError ); } get backup(): SandboxBackupAPI { @@ -748,7 +822,8 @@ export class ContainerControlClient { this.getConnection().rpc().backup, 'backup', this.recordCallStarted, - this.recordCallSettled + this.recordCallSettled, + this.getLastConnectionError ); } get watch(): SandboxWatchAPI { @@ -756,7 +831,8 @@ export class ContainerControlClient { this.getConnection().rpc().watch, 'watch', this.recordCallStarted, - this.recordCallSettled + this.recordCallSettled, + this.getLastConnectionError ); } get tunnels(): SandboxTunnelsAPI { @@ -764,7 +840,8 @@ export class ContainerControlClient { this.getConnection().rpc().tunnels, 'tunnels', this.recordCallStarted, - this.recordCallSettled + this.recordCallSettled, + this.getLastConnectionError ); } get interpreter(): SandboxInterpreterAPI { @@ -772,7 +849,8 @@ export class ContainerControlClient { this.getConnection().rpc().interpreter, 'interpreter', this.recordCallStarted, - this.recordCallSettled + this.recordCallSettled, + this.getLastConnectionError ); } diff --git a/packages/sandbox/src/container-control/connection.ts b/packages/sandbox/src/container-control/connection.ts index 6d11e0da0..64cdae779 100644 --- a/packages/sandbox/src/container-control/connection.ts +++ b/packages/sandbox/src/container-control/connection.ts @@ -86,6 +86,73 @@ async function tryParseContainerUnavailable( } } +/** + * Platform messages emitted by the Containers runtime when it cannot admit a + * container for a Durable Object during startup. They surface as plain Errors + * thrown from the container-binding fetch, before the capnweb session is + * established. Each maps to a categorical `ContainerUnavailableContext.reason`. + */ +const PLATFORM_UNAVAILABLE_SIGNATURES: ReadonlyArray<{ + substring: string; + reason: + | 'no_container_instance_available' + | 'max_container_instances_exceeded'; +}> = [ + { + substring: + 'There is no container instance that can be provided to this Durable Object', + reason: 'no_container_instance_available' + }, + { + substring: 'Maximum number of running container instances exceeded', + reason: 'max_container_instances_exceeded' + } +]; + +/** + * True when a thrown connection-startup error matches a known platform + * container-admission failure. These are transient: the platform asks the + * caller to try again later, so they are safe to retry within the budget. + */ +function isPlatformUnavailableError(error: unknown): boolean { + return ( + error instanceof Error && + PLATFORM_UNAVAILABLE_SIGNATURES.some((sig) => + error.message.includes(sig.substring) + ) + ); +} + +/** + * Convert a raw connection-startup error into a typed ContainerUnavailableError + * when it matches a known platform container-admission failure. Returns null + * for anything else so the caller preserves the original error. + */ +function tryConvertPlatformUnavailable(error: unknown): Error | null { + if (!(error instanceof Error)) return null; + const match = PLATFORM_UNAVAILABLE_SIGNATURES.find((sig) => + error.message.includes(sig.substring) + ); + if (!match) return null; + + const context = { + reason: match.reason, + retryable: true as const, + originalMessage: error.message + }; + return createErrorFromResponse( + { + code: ErrorCode.CONTAINER_UNAVAILABLE, + message: error.message, + context, + httpStatus: getHttpStatus(ErrorCode.CONTAINER_UNAVAILABLE), + suggestion: getSuggestion(ErrorCode.CONTAINER_UNAVAILABLE, context), + timestamp: new Date().toISOString() + }, + { cause: error } + ); +} + // --------------------------------------------------------------------------- // Connection manager // --------------------------------------------------------------------------- @@ -126,6 +193,16 @@ export interface ContainerControlConnectionOptions { * Not fired for `disconnect()`. */ onClose?: () => void; + /** + * Invoked with the connection-startup error just before the deferred + * transport is aborted. Lets the owner capture the *real* failure cause + * (e.g. a platform container-allocation error) before capnweb replaces it + * with a generic "RPC session was shut down" message on the queued calls + * that reject as a result of the abort. + * + * Fired at most once per connection attempt. Not fired for `disconnect()`. + */ + onConnectionError?: (error: unknown) => void; } /** @@ -147,6 +224,7 @@ export class ContainerControlConnection { private readonly logger: Logger; private retryTimeoutMs: number; private readonly onClose: (() => void) | undefined; + private readonly onConnectionError: ((error: unknown) => void) | undefined; constructor(options: ContainerControlConnectionOptions) { this.containerStub = options.stub; @@ -154,6 +232,7 @@ export class ContainerControlConnection { this.logger = options.logger ?? createNoOpLogger(); this.retryTimeoutMs = options.retryTimeoutMs ?? DEFAULT_RETRY_TIMEOUT_MS; this.onClose = options.onClose; + this.onConnectionError = options.onConnectionError; this.transport = new DeferredTransport(); this.session = new RpcSession( @@ -259,6 +338,22 @@ export class ContainerControlConnection { } } + /** + * Run the owner-provided `onConnectionError` callback exactly once per + * failed connection attempt, swallowing any listener errors. + */ + private fireConnectionError(error: unknown): void { + if (!this.onConnectionError) return; + try { + this.onConnectionError(error); + } catch (err) { + this.logger.warn( + 'ContainerControlConnection onConnectionError handler threw', + { error: err instanceof Error ? err.message : String(err) } + ); + } + } + /** * WebSocket `close` listener. Defined as a bound arrow field so the * same reference can be passed to both `addEventListener` and @@ -338,7 +433,15 @@ export class ContainerControlConnection { }); } catch (error) { this.connected = false; - this.transport.abort(error); + // Convert the platform container-allocation failure into a typed + // ContainerUnavailableError so the real cause survives. capnweb will + // otherwise mask it: `transport.abort()` below rejects the queued RPC + // calls with a generic "RPC session was shut down" message. + const connectionError = tryConvertPlatformUnavailable(error) ?? error; + // Hand the owner the real cause before aborting the transport, so it can + // prefer this over the masking disposal error on queued RPC rejections. + this.fireConnectionError(connectionError); + this.transport.abort(connectionError); // Signal the client to discard this connection. The transport is now // permanently aborted; any in-flight or future stub calls would fail // immediately. Firing onClose here lets the client null out its @@ -346,9 +449,11 @@ export class ContainerControlConnection { this.fireOnClose(); this.logger.error( 'ContainerControlConnection failed', - error instanceof Error ? error : new Error(String(error)) + connectionError instanceof Error + ? connectionError + : new Error(String(connectionError)) ); - throw error; + throw connectionError; } } @@ -364,7 +469,11 @@ export class ContainerControlConnection { logger: this.logger, retryLogMessage: 'ContainerControlConnection upgrade returned retryable status, retrying', - shouldRetry: isRetryableWebSocketUpgradeResponse + shouldRetry: isRetryableWebSocketUpgradeResponse, + // The Containers platform signals transient container-admission failure + // by *throwing* (e.g. "There is no container instance...") rather than + // returning a retryable status. Retry those within the same budget. + shouldRetryError: isPlatformUnavailableError }); } diff --git a/packages/sandbox/src/response-retry.ts b/packages/sandbox/src/response-retry.ts index 157d43ebf..ec4ec86ed 100644 --- a/packages/sandbox/src/response-retry.ts +++ b/packages/sandbox/src/response-retry.ts @@ -16,6 +16,15 @@ export interface ResponseRetryOptions { logger: Logger; retryLogMessage: string; shouldRetry(response: Response): boolean; + /** + * Decide whether a *thrown* error from `fetchResponse` should be retried. + * Some backends signal transient unavailability by throwing rather than + * returning a retryable status (e.g. the Containers platform throwing + * "There is no container instance..."). When this returns true the error is + * retried within the same budget as retryable responses; otherwise it is + * rethrown immediately. Omitted means thrown errors are never retried. + */ + shouldRetryError?: (error: unknown) => boolean; getRetryLogContext?: (response: Response) => Partial; onRetryExhausted?: (params: { attempts: number; @@ -37,7 +46,39 @@ export async function fetchWithResponseRetry( let attempt = 0; while (true) { - const response = await fetchResponse(); + let response: Response; + try { + response = await fetchResponse(); + } catch (error) { + // A thrown error is only retryable when the caller opts in via + // `shouldRetryError`. Everything else propagates unchanged. + if (!options.shouldRetryError?.(error)) { + throw error; + } + + const elapsed = Date.now() - startTime; + const remaining = options.retryTimeoutMs - elapsed; + if (remaining <= options.minTimeForRetryMs) { + // Budget exhausted — surface the real cause rather than swallowing it. + throw error; + } + + const delay = Math.min( + DEFAULT_INITIAL_RETRY_DELAY_MS * 2 ** attempt, + DEFAULT_MAX_RETRY_DELAY_MS + ); + + options.logger.info(options.retryLogMessage, { + attempt: attempt + 1, + delayMs: delay, + remainingSec: Math.floor(remaining / 1000), + error: error instanceof Error ? error.message : String(error) + }); + + await new Promise((resolve) => setTimeout(resolve, delay)); + attempt++; + continue; + } if (!options.shouldRetry(response)) { return response; diff --git a/packages/sandbox/tests/container-connection.test.ts b/packages/sandbox/tests/container-connection.test.ts index 096d5b15b..782717012 100644 --- a/packages/sandbox/tests/container-connection.test.ts +++ b/packages/sandbox/tests/container-connection.test.ts @@ -387,6 +387,116 @@ describe('ContainerControlConnection', () => { } }); + it('retries a thrown platform "no container instance" error until success', async () => { + vi.useFakeTimers(); + try { + const platformMessage = + 'There is no container instance that can be provided to this Durable Object, try again later'; + const fetchMock = vi + .fn<(req: Request) => Promise>() + .mockRejectedValueOnce(new Error(platformMessage)) + .mockRejectedValueOnce(new Error(platformMessage)) + .mockResolvedValueOnce(makeUpgradeResponse()); + + const conn = new ContainerControlConnection({ + stub: { fetch: fetchMock }, + retryTimeoutMs: 60_000 + }); + + const connectPromise = conn.connect(); + await vi.advanceTimersByTimeAsync(0); + expect(fetchMock).toHaveBeenCalledTimes(1); + + await vi.advanceTimersByTimeAsync(3_000); + expect(fetchMock).toHaveBeenCalledTimes(2); + + await vi.advanceTimersByTimeAsync(6_000); + await connectPromise; + + expect(fetchMock).toHaveBeenCalledTimes(3); + expect(conn.isConnected()).toBe(true); + } finally { + vi.useRealTimers(); + } + }); + + it('retries a thrown "max instances exceeded" error until success', async () => { + vi.useFakeTimers(); + try { + const platformMessage = + 'Maximum number of running container instances exceeded. Try again later, or try configuring a higher value for max_instances'; + const fetchMock = vi + .fn<(req: Request) => Promise>() + .mockRejectedValueOnce(new Error(platformMessage)) + .mockResolvedValueOnce(makeUpgradeResponse()); + + const conn = new ContainerControlConnection({ + stub: { fetch: fetchMock }, + retryTimeoutMs: 60_000 + }); + + const connectPromise = conn.connect(); + await vi.advanceTimersByTimeAsync(0); + expect(fetchMock).toHaveBeenCalledTimes(1); + + await vi.advanceTimersByTimeAsync(3_000); + await connectPromise; + + expect(fetchMock).toHaveBeenCalledTimes(2); + expect(conn.isConnected()).toBe(true); + } finally { + vi.useRealTimers(); + } + }); + + it('surfaces ContainerUnavailableError when the platform error retry budget is exhausted', async () => { + vi.useFakeTimers(); + try { + const platformMessage = + 'There is no container instance that can be provided to this Durable Object, try again later'; + const fetchMock = vi + .fn<(req: Request) => Promise>() + .mockRejectedValue(new Error(platformMessage)); + + const conn = new ContainerControlConnection({ + stub: { fetch: fetchMock }, + retryTimeoutMs: 20_000 + }); + + const connectPromise = conn.connect(); + const assertion = expect(connectPromise).rejects.toMatchObject({ + code: 'CONTAINER_UNAVAILABLE', + context: { + reason: 'no_container_instance_available', + retryable: true, + originalMessage: platformMessage + } + }); + + await vi.advanceTimersByTimeAsync(60_000); + await assertion; + + expect(conn.isConnected()).toBe(false); + expect(fetchMock).toHaveBeenCalledTimes(3); + } finally { + vi.useRealTimers(); + } + }); + + it('does not retry a thrown non-platform error', async () => { + const fetchMock = vi + .fn<(req: Request) => Promise>() + .mockRejectedValue(new Error('some other failure')); + + const conn = new ContainerControlConnection({ + stub: { fetch: fetchMock }, + retryTimeoutMs: 120_000 + }); + + await expect(conn.connect()).rejects.toThrow('some other failure'); + expect(fetchMock).toHaveBeenCalledTimes(1); + }); + it('does not retry terminal upgrade failures', async () => { const fetchMock = vi .fn<(req: Request) => Promise>() @@ -573,6 +683,93 @@ describe('ContainerControlConnection', () => { }); }); + it('converts the platform "no container instance" error into a ContainerUnavailableError', async () => { + const platformMessage = + 'There is no container instance that can be provided to this Durable Object, try again later'; + const conn = new ContainerControlConnection({ + stub: { + fetch: vi.fn().mockRejectedValue(new Error(platformMessage)) + }, + retryTimeoutMs: 0 + }); + + const error = await conn.connect().catch((e: unknown) => e); + expect((error as { code?: string }).code).toBe('CONTAINER_UNAVAILABLE'); + expect((error as Error).constructor.name).toBe( + 'ContainerUnavailableError' + ); + expect(error).toMatchObject({ + context: { + reason: 'no_container_instance_available', + retryable: true, + originalMessage: platformMessage + } + }); + }); + + it('converts the platform "max instances exceeded" error into a ContainerUnavailableError', async () => { + const platformMessage = + 'Maximum number of running container instances exceeded. Try again later, or try configuring a higher value for max_instances'; + const conn = new ContainerControlConnection({ + stub: { + fetch: vi.fn().mockRejectedValue(new Error(platformMessage)) + }, + retryTimeoutMs: 0 + }); + + const error = await conn.connect().catch((e: unknown) => e); + expect((error as { code?: string }).code).toBe('CONTAINER_UNAVAILABLE'); + expect((error as Error).constructor.name).toBe( + 'ContainerUnavailableError' + ); + expect(error).toMatchObject({ + context: { + reason: 'max_container_instances_exceeded', + retryable: true, + originalMessage: platformMessage + } + }); + }); + + it('fires onConnectionError with the converted platform error before aborting the transport', async () => { + const platformMessage = + 'There is no container instance that can be provided to this Durable Object, try again later'; + const onConnectionError = vi.fn(); + const conn = new ContainerControlConnection({ + stub: { + fetch: vi.fn().mockRejectedValue(new Error(platformMessage)) + }, + retryTimeoutMs: 0, + onConnectionError + }); + + await conn.connect().catch(() => {}); + expect(onConnectionError).toHaveBeenCalledOnce(); + const captured = onConnectionError.mock.calls[0][0]; + expect((captured as { code?: string }).code).toBe( + 'CONTAINER_UNAVAILABLE' + ); + expect(captured).toMatchObject({ + context: { reason: 'no_container_instance_available' } + }); + }); + + it('fires onConnectionError for a generic upgrade failure', async () => { + const onConnectionError = vi.fn(); + const conn = new ContainerControlConnection({ + stub: { + fetch: vi + .fn() + .mockResolvedValue(new Response('Not Found', { status: 404 })) + }, + retryTimeoutMs: 0, + onConnectionError + }); + + await conn.connect().catch(() => {}); + expect(onConnectionError).toHaveBeenCalledOnce(); + }); + it('fires onClose after a failed connect so the client can discard the poisoned connection', async () => { const onClose = vi.fn(); const conn = new ContainerControlConnection({ diff --git a/packages/sandbox/tests/response-retry.test.ts b/packages/sandbox/tests/response-retry.test.ts index b3e8c77f0..6c7aef937 100644 --- a/packages/sandbox/tests/response-retry.test.ts +++ b/packages/sandbox/tests/response-retry.test.ts @@ -102,5 +102,91 @@ describe('response retry helpers', () => { response: response503 }); }); + + it('retries thrown errors matched by shouldRetryError until success', async () => { + vi.useFakeTimers(); + + try { + const fetchResponse = vi + .fn<() => Promise>() + .mockRejectedValueOnce(new Error('no container instance')) + .mockResolvedValueOnce(responseWithStatus(200)); + + const retrying = fetchWithResponseRetry(fetchResponse, { + retryTimeoutMs: 20_000, + minTimeForRetryMs: 15_000, + logger: createNoOpLogger(), + retryLogMessage: 'retrying test response', + shouldRetry: () => false, + shouldRetryError: (err) => + err instanceof Error && err.message === 'no container instance' + }); + + await vi.advanceTimersByTimeAsync(0); + expect(fetchResponse).toHaveBeenCalledTimes(1); + + await vi.advanceTimersByTimeAsync(3_000); + const response = await retrying; + + expect(fetchResponse).toHaveBeenCalledTimes(2); + expect(response.status).toBe(200); + } finally { + vi.useRealTimers(); + } + }); + + it('rethrows thrown errors that shouldRetryError does not match', async () => { + const fetchResponse = vi + .fn<() => Promise>() + .mockRejectedValue(new Error('fatal')); + + await expect( + fetchWithResponseRetry(fetchResponse, { + retryTimeoutMs: 20_000, + minTimeForRetryMs: 15_000, + logger: createNoOpLogger(), + retryLogMessage: 'retrying test response', + shouldRetry: () => false, + shouldRetryError: (err) => + err instanceof Error && err.message === 'no container instance' + }) + ).rejects.toThrow('fatal'); + expect(fetchResponse).toHaveBeenCalledTimes(1); + }); + + it('rethrows the last error when the retry budget is exhausted', async () => { + const fetchResponse = vi + .fn<() => Promise>() + .mockRejectedValue(new Error('no container instance')); + + await expect( + fetchWithResponseRetry(fetchResponse, { + retryTimeoutMs: 0, + minTimeForRetryMs: 15_000, + logger: createNoOpLogger(), + retryLogMessage: 'retrying test response', + shouldRetry: () => false, + shouldRetryError: () => true + }) + ).rejects.toThrow('no container instance'); + expect(fetchResponse).toHaveBeenCalledTimes(1); + }); + + it('rethrows a thrown error without retrying when no shouldRetryError is provided', async () => { + const fetchResponse = vi + .fn<() => Promise>() + .mockRejectedValue(new Error('boom')); + + await expect( + fetchWithResponseRetry(fetchResponse, { + retryTimeoutMs: 20_000, + minTimeForRetryMs: 15_000, + logger: createNoOpLogger(), + retryLogMessage: 'retrying test response', + shouldRetry: () => false + }) + ).rejects.toThrow('boom'); + expect(fetchResponse).toHaveBeenCalledTimes(1); + }); }); }); diff --git a/packages/sandbox/tests/rpc-sandbox-client.test.ts b/packages/sandbox/tests/rpc-sandbox-client.test.ts index de3f97e2c..a806390e6 100644 --- a/packages/sandbox/tests/rpc-sandbox-client.test.ts +++ b/packages/sandbox/tests/rpc-sandbox-client.test.ts @@ -699,6 +699,92 @@ describe('translateRPCError', () => { expect((thrown as Error).cause).toBe(original); }); + // ------------------------------------------------------------------------- + // Captured connection error preference + // ------------------------------------------------------------------------- + + it('prefers a captured connection error over a masking session_disposed transport error', async () => { + const translateRPCError = await loadFn(); + const { ContainerUnavailableError } = await loadErr(); + // The typed error the connection layer captures for the platform + // "no container instance" failure. + const platformMessage = + 'There is no container instance that can be provided to this Durable Object, try again later'; + const connectionError = new ContainerUnavailableError({ + code: 'CONTAINER_UNAVAILABLE', + message: platformMessage, + context: { + reason: 'no_container_instance_available', + retryable: true, + originalMessage: platformMessage + }, + httpStatus: 503, + timestamp: new Date().toISOString() + }); + + let thrown: unknown; + try { + translateRPCError( + new Error('RPC session was shut down by disposing the main stub'), + { operation: 'utils.createSession', connectionError } + ); + } catch (e) { + thrown = e; + } + expect(thrown).toBeInstanceOf(ContainerUnavailableError); + const err = thrown as InstanceType; + expect(err.code).toBe('CONTAINER_UNAVAILABLE'); + expect(err.reason).toBe('no_container_instance_available'); + expect(err.context.retryable).toBe(true); + expect(err.context.originalMessage).toBe(platformMessage); + }); + + it('ignores a captured connection error for container-side structured errors', async () => { + const translateRPCError = await loadFn(); + const { FileNotFoundError, ContainerUnavailableError } = await loadErr(); + const connectionError = new ContainerUnavailableError({ + code: 'CONTAINER_UNAVAILABLE', + message: 'no container', + context: { + reason: 'no_container_instance_available', + retryable: true, + originalMessage: 'no container' + }, + httpStatus: 503, + timestamp: new Date().toISOString() + }); + const payload = JSON.stringify({ + code: 'FILE_NOT_FOUND', + message: 'no such file', + context: { path: '/missing' } + }); + let thrown: unknown; + try { + translateRPCError(new Error(payload), { + operation: 'files.readFile', + connectionError + }); + } catch (e) { + thrown = e; + } + expect(thrown).toBeInstanceOf(FileNotFoundError); + }); + + it('falls back to the transport error when no connection error was captured', async () => { + const translateRPCError = await loadFn(); + const { OperationInterruptedError } = await loadErr(); + let thrown: unknown; + try { + translateRPCError( + new Error('RPC session was shut down by disposing the main stub'), + { operation: 'utils.createSession' } + ); + } catch (e) { + thrown = e; + } + expect(thrown).toBeInstanceOf(OperationInterruptedError); + }); + it('does not set `cause` on errors translated from JSON-encoded structured payloads', async () => { // The container-side errors flow through createErrorFromResponse without // an `options` argument, so `cause` stays unset (matching pre-fix diff --git a/packages/shared/src/errors/contexts.ts b/packages/shared/src/errors/contexts.ts index 2a8c8653a..fed049a72 100644 --- a/packages/shared/src/errors/contexts.ts +++ b/packages/shared/src/errors/contexts.ts @@ -250,12 +250,24 @@ export interface ContainerUnavailableContext { /** * Categorical reason distinguishing startup unavailability from runtime * replacement and exhausted upgrade retries. + * + * `no_container_instance_available` is raised when the Containers platform + * cannot allocate an instance for the Durable Object during connection + * startup ("There is no container instance that can be provided to this + * Durable Object, try again later"). + * + * `max_container_instances_exceeded` is raised when the account has reached + * its configured concurrent-instance ceiling ("Maximum number of running + * container instances exceeded. Try again later, or try configuring a + * higher value for max_instances"). */ reason: | 'container_starting' | 'container_unhealthy' | 'container_replaced' - | 'rpc_upgrade_failed'; + | 'rpc_upgrade_failed' + | 'no_container_instance_available' + | 'max_container_instances_exceeded'; /** * Always true — this error represents a transient unavailability, not a * permanent failure. Callers should retry the same operation. @@ -263,6 +275,12 @@ export interface ContainerUnavailableContext { retryable: true; /** Suggested delay in milliseconds before the next retry attempt. */ retryAfterMs?: number; + /** + * Original platform/transport error message, preserved verbatim when the + * unavailability was detected from a lower-level error (e.g. the platform + * container-allocation failure) rather than a structured response body. + */ + originalMessage?: string; } /** From e6c77d36e15c723af0bdd5aa78e44f6b4c946fc2 Mon Sep 17 00:00:00 2001 From: Aron <263346377+aron-cf@users.noreply.github.com> Date: Wed, 1 Jul 2026 12:22:45 +0100 Subject: [PATCH 04/24] Export ContainerUnavailableReason for typed error branching Extract the container-unavailable reason union into a named, exported type so callers can distinguish no_container_instance_available from max_container_instances_exceeded without hardcoding string literals. --- .changeset/surface-container-unavailable.md | 2 +- packages/sandbox/src/errors/index.ts | 1 + packages/sandbox/src/index.ts | 1 + packages/shared/src/errors/contexts.ts | 48 +++++++++++++-------- packages/shared/src/errors/index.ts | 1 + 5 files changed, 34 insertions(+), 19 deletions(-) diff --git a/.changeset/surface-container-unavailable.md b/.changeset/surface-container-unavailable.md index 93e5acbb3..949ca72e4 100644 --- a/.changeset/surface-container-unavailable.md +++ b/.changeset/surface-container-unavailable.md @@ -2,4 +2,4 @@ '@cloudflare/sandbox': patch --- -Surface container allocation failures as `ContainerUnavailableError` instead of masking them as generic `utils.createSession` interruptions. When the Containers platform cannot admit a container during startup ("There is no container instance that can be provided to this Durable Object" or "Maximum number of running container instances exceeded"), callers now receive the real, retryable cause with structured context. +Surface container allocation failures as `ContainerUnavailableError` instead of masking them as generic `utils.createSession` interruptions. When the Containers platform cannot admit a container during startup ("There is no container instance that can be provided to this Durable Object" or "Maximum number of running container instances exceeded"), callers now receive the real, retryable cause with structured context. Callers can distinguish the two via `error.reason` (`'no_container_instance_available'` vs `'max_container_instances_exceeded'`), typed by the new exported `ContainerUnavailableReason`. diff --git a/packages/sandbox/src/errors/index.ts b/packages/sandbox/src/errors/index.ts index 553a0e3a3..94994cbba 100644 --- a/packages/sandbox/src/errors/index.ts +++ b/packages/sandbox/src/errors/index.ts @@ -48,6 +48,7 @@ export type { CommandErrorContext, CommandNotFoundContext, ContainerUnavailableContext, + ContainerUnavailableReason, ContextNotFoundContext, ErrorCodeType, ErrorResponse, diff --git a/packages/sandbox/src/index.ts b/packages/sandbox/src/index.ts index 23810af97..cae580fd0 100644 --- a/packages/sandbox/src/index.ts +++ b/packages/sandbox/src/index.ts @@ -114,6 +114,7 @@ export type { } from './clients/interpreter-client.js'; export type { ContainerUnavailableContext, + ContainerUnavailableReason, OperationInterruptedContext, OperationInterruptedReason, RPCTransportContext, diff --git a/packages/shared/src/errors/contexts.ts b/packages/shared/src/errors/contexts.ts index fed049a72..17df66e4f 100644 --- a/packages/shared/src/errors/contexts.ts +++ b/packages/shared/src/errors/contexts.ts @@ -240,6 +240,34 @@ export interface InternalErrorContext { [key: string]: unknown; // Allow extension } +/** + * Reason the sandbox container could not accept the incoming RPC connection. + * Callers may branch on this to distinguish container-startup unavailability + * from account-level capacity limits. `retryable` is always true regardless + * of reason. + */ +export type ContainerUnavailableReason = + /** The container is still booting. */ + | 'container_starting' + /** The container is temporarily unhealthy. */ + | 'container_unhealthy' + /** The container was replaced while the connection attempt was in progress. */ + | 'container_replaced' + /** The WebSocket upgrade retry budget was exhausted. */ + | 'rpc_upgrade_failed' + /** + * The Containers platform could not allocate an instance for the Durable + * Object during connection startup ("There is no container instance that + * can be provided to this Durable Object, try again later"). + */ + | 'no_container_instance_available' + /** + * The account reached its configured concurrent-instance ceiling + * ("Maximum number of running container instances exceeded. Try again + * later, or try configuring a higher value for max_instances"). + */ + | 'max_container_instances_exceeded'; + /** * Container availability error context. Surfaced when the sandbox container * cannot accept the incoming RPC connection. The container may be starting @@ -249,25 +277,9 @@ export interface InternalErrorContext { export interface ContainerUnavailableContext { /** * Categorical reason distinguishing startup unavailability from runtime - * replacement and exhausted upgrade retries. - * - * `no_container_instance_available` is raised when the Containers platform - * cannot allocate an instance for the Durable Object during connection - * startup ("There is no container instance that can be provided to this - * Durable Object, try again later"). - * - * `max_container_instances_exceeded` is raised when the account has reached - * its configured concurrent-instance ceiling ("Maximum number of running - * container instances exceeded. Try again later, or try configuring a - * higher value for max_instances"). + * replacement, exhausted upgrade retries, and account capacity limits. */ - reason: - | 'container_starting' - | 'container_unhealthy' - | 'container_replaced' - | 'rpc_upgrade_failed' - | 'no_container_instance_available' - | 'max_container_instances_exceeded'; + reason: ContainerUnavailableReason; /** * Always true — this error represents a transient unavailability, not a * permanent failure. Callers should retry the same operation. diff --git a/packages/shared/src/errors/index.ts b/packages/shared/src/errors/index.ts index 4c39c8dcc..2051002dc 100644 --- a/packages/shared/src/errors/index.ts +++ b/packages/shared/src/errors/index.ts @@ -42,6 +42,7 @@ export type { CommandErrorContext, CommandNotFoundContext, ContainerUnavailableContext, + ContainerUnavailableReason, ContextNotFoundContext, FileExistsContext, FileNotFoundContext, From c4dbba524416e0b20963ec59298697831f4c87b6 Mon Sep 17 00:00:00 2001 From: Aron <263346377+aron-cf@users.noreply.github.com> Date: Wed, 1 Jul 2026 13:50:05 +0100 Subject: [PATCH 05/24] Harden container-unavailable detection and error preference Make platform detection case-insensitive and realm-safe so lower-case container-library messages are retried, classify plain-text 503 no-instance bodies, and prefer captured connection errors by structure (any CONTAINER_UNAVAILABLE-coded value) rather than only same-realm SandboxError instances. Add a pool-based reproduction driving a real Sandbox DO over RPC transport. --- .changeset/surface-container-unavailable.md | 2 +- .../sandbox/src/container-control/client.ts | 61 ++++- .../src/container-control/connection.ts | 159 ++++++++----- .../tests/container-connection.test.ts | 97 ++++++++ .../tests/rpc-container-unavailable.test.ts | 167 ++++++++++++++ .../tests/rpc-no-container-instance.test.ts | 208 ++++++++++++++++++ .../sandbox/tests/rpc-sandbox-client.test.ts | 62 ++++++ 7 files changed, 700 insertions(+), 56 deletions(-) create mode 100644 packages/sandbox/tests/rpc-container-unavailable.test.ts create mode 100644 packages/sandbox/tests/rpc-no-container-instance.test.ts diff --git a/.changeset/surface-container-unavailable.md b/.changeset/surface-container-unavailable.md index 949ca72e4..d9b07bc74 100644 --- a/.changeset/surface-container-unavailable.md +++ b/.changeset/surface-container-unavailable.md @@ -2,4 +2,4 @@ '@cloudflare/sandbox': patch --- -Surface container allocation failures as `ContainerUnavailableError` instead of masking them as generic `utils.createSession` interruptions. When the Containers platform cannot admit a container during startup ("There is no container instance that can be provided to this Durable Object" or "Maximum number of running container instances exceeded"), callers now receive the real, retryable cause with structured context. Callers can distinguish the two via `error.reason` (`'no_container_instance_available'` vs `'max_container_instances_exceeded'`), typed by the new exported `ContainerUnavailableReason`. +Surface container allocation failures as `ContainerUnavailableError` instead of masking them as generic `utils.createSession` interruptions. When the Containers platform cannot admit a container during startup ("There is no container instance that can be provided to this Durable Object", the plain-text 503 "There is no Container instance available at this time", or "Maximum number of running container instances exceeded"), callers now receive the real, retryable cause with structured context. Detection is case-insensitive and realm-safe, and captured connection errors are preferred by structure (any `CONTAINER_UNAVAILABLE`-coded value) rather than only same-realm `SandboxError` instances, so the failure is no longer masked as an interrupted operation. Callers can distinguish the causes via `error.reason` (`'no_container_instance_available'` vs `'max_container_instances_exceeded'`), typed by the new exported `ContainerUnavailableReason`. diff --git a/packages/sandbox/src/container-control/client.ts b/packages/sandbox/src/container-control/client.ts index fa69972a3..f32e36de9 100644 --- a/packages/sandbox/src/container-control/client.ts +++ b/packages/sandbox/src/container-control/client.ts @@ -348,15 +348,25 @@ function buildTransportErrorResponse( * When a queued RPC call rejects with a transport error that a connection * abort could have caused (session disposed, connection failed, upgrade * failed, or peer closed), and the connection captured a real startup error, - * return that captured error. `SandboxError` instances (e.g. the typed - * ContainerUnavailableError) are surfaced as-is; anything else is returned - * untouched so the caller falls back to normal transport classification. + * return that captured error in preference to the masking transport error. + * + * The captured error is accepted whether it is: + * - a same-realm `SandboxError` (surfaced as-is), or + * - any structured, error-like value carrying a recognized `code` — e.g. a + * raw or cross-realm error decorated with `code: 'CONTAINER_UNAVAILABLE'` + * and optional `context`/`details`/`message`. Such values are rehydrated + * via `createErrorFromResponse` so the caller still receives a proper + * typed SandboxError instead of a masked OperationInterruptedError. + * + * Anything without a recognized code is ignored so the caller falls back to + * normal transport classification. */ function maybePreferConnectionError( transportResponse: ErrorResponse, context: RPCTranslationContext ): SandboxError | null { - if (!context.connectionError) return null; + const captured = context.connectionError; + if (!captured) return null; const { kind } = transportResponse.context; if ( kind !== 'session_disposed' && @@ -366,9 +376,48 @@ function maybePreferConnectionError( ) { return null; } - if (context.connectionError instanceof SandboxError) { - return context.connectionError; + + // Same-realm typed error: surface as-is. + if (captured instanceof SandboxError) { + return captured; } + + // Structured, error-like value (raw/cross-realm) carrying a recognized + // code. Rehydrate into a typed SandboxError. Reads `context` or `details` + // for the structured payload, matching the two wire formats handled by + // `translateRPCError`. + const shape = captured as { + code?: unknown; + message?: unknown; + context?: unknown; + details?: unknown; + } | null; + if ( + shape && + typeof shape.code === 'string' && + Object.hasOwn(ErrorCode, shape.code) + ) { + const code = shape.code as ErrorCode; + const structured = + shape.context && typeof shape.context === 'object' + ? (shape.context as Record) + : shape.details && typeof shape.details === 'object' + ? (shape.details as Record) + : {}; + const message = typeof shape.message === 'string' ? shape.message : code; + return createErrorFromResponse( + { + code, + message, + context: structured, + httpStatus: getHttpStatus(code), + suggestion: getSuggestion(code, structured as Record), + timestamp: new Date().toISOString() + }, + { cause: captured } + ) as SandboxError; + } + return null; } diff --git a/packages/sandbox/src/container-control/connection.ts b/packages/sandbox/src/container-control/connection.ts index 64cdae779..1d7ad749a 100644 --- a/packages/sandbox/src/container-control/connection.ts +++ b/packages/sandbox/src/container-control/connection.ts @@ -52,35 +52,52 @@ async function tryParseContainerUnavailable( response: Response ): Promise { const contentType = response.headers.get('content-type') ?? ''; - if (!contentType.includes('application/json')) return null; + // JSON path: structured CONTAINER_UNAVAILABLE body emitted by the SDK's own + // container-side handlers. + if (contentType.includes('application/json')) { + try { + const body = (await response.clone().json()) as StructuredErrorBody; + if (body.code !== ErrorCode.CONTAINER_UNAVAILABLE) return null; + + const reason = + body.context?.reason === 'container_starting' || + body.context?.reason === 'container_unhealthy' || + body.context?.reason === 'container_replaced' || + body.context?.reason === 'rpc_upgrade_failed' + ? body.context.reason + : 'container_replaced'; + const context = { + reason, + retryable: true as const, + ...(typeof body.context?.retryAfterMs === 'number' && { + retryAfterMs: body.context.retryAfterMs + }) + }; + + return createErrorFromResponse({ + code: ErrorCode.CONTAINER_UNAVAILABLE, + message: body.message ?? 'Container is unavailable', + context, + httpStatus: getHttpStatus(ErrorCode.CONTAINER_UNAVAILABLE), + suggestion: getSuggestion(ErrorCode.CONTAINER_UNAVAILABLE, context), + timestamp: new Date().toISOString() + }); + } catch { + return null; + } + } + + // Plain-text path: the @cloudflare/containers base class returns a plain + // text 503 ("There is no Container instance available at this time...") + // when it cannot admit a container. Classify the body so an exhausted retry + // budget still surfaces a typed ContainerUnavailableError rather than a + // generic rpc_upgrade_failed. try { - const body = (await response.clone().json()) as StructuredErrorBody; - if (body.code !== ErrorCode.CONTAINER_UNAVAILABLE) return null; - - const reason = - body.context?.reason === 'container_starting' || - body.context?.reason === 'container_unhealthy' || - body.context?.reason === 'container_replaced' || - body.context?.reason === 'rpc_upgrade_failed' - ? body.context.reason - : 'container_replaced'; - const context = { - reason, - retryable: true as const, - ...(typeof body.context?.retryAfterMs === 'number' && { - retryAfterMs: body.context.retryAfterMs - }) - }; - - return createErrorFromResponse({ - code: ErrorCode.CONTAINER_UNAVAILABLE, - message: body.message ?? 'Container is unavailable', - context, - httpStatus: getHttpStatus(ErrorCode.CONTAINER_UNAVAILABLE), - suggestion: getSuggestion(ErrorCode.CONTAINER_UNAVAILABLE, context), - timestamp: new Date().toISOString() - }); + const text = await response.clone().text(); + const match = matchPlatformUnavailable(text); + if (!match) return null; + return buildContainerUnavailableError(match.reason, text); } catch { return null; } @@ -93,66 +110,110 @@ async function tryParseContainerUnavailable( * established. Each maps to a categorical `ContainerUnavailableContext.reason`. */ const PLATFORM_UNAVAILABLE_SIGNATURES: ReadonlyArray<{ + /** Lowercase substring matched case-insensitively against the error text. */ substring: string; reason: | 'no_container_instance_available' | 'max_container_instances_exceeded'; }> = [ { + // Platform error thrown from the container binding during startup. substring: - 'There is no container instance that can be provided to this Durable Object', + 'there is no container instance that can be provided to this durable object', reason: 'no_container_instance_available' }, { - substring: 'Maximum number of running container instances exceeded', + // Plain-text 503 body returned by @cloudflare/containers' containerFetch + // when no instance can be admitted (see container.ts). + substring: 'there is no container instance available at this time', + reason: 'no_container_instance_available' + }, + { + substring: 'maximum number of running container instances exceeded', reason: 'max_container_instances_exceeded' } ]; +/** + * Extract a matchable message string from any thrown value. + * + * Deliberately avoids `instanceof Error`: the platform's container-admission + * errors are raised by the workerd container binding, which may live in a + * different realm than this SDK bundle, so `instanceof Error` can be false + * even for a genuine Error (the same cross-realm trap documented in + * container-control/client.ts). Mirrors the base `@cloudflare/containers` + * `isErrorOfType` helper: coerce to string, then match case-insensitively. + */ +function errorText(error: unknown): string { + const message = (error as { message?: unknown } | null | undefined)?.message; + return (typeof message === 'string' ? message : String(error)).toLowerCase(); +} + +/** + * Find the platform container-admission signature matching a thrown value, + * or null. Case-insensitive and realm-safe (does not use `instanceof`). + */ +function matchPlatformUnavailable( + error: unknown +): (typeof PLATFORM_UNAVAILABLE_SIGNATURES)[number] | null { + const text = errorText(error); + return ( + PLATFORM_UNAVAILABLE_SIGNATURES.find((sig) => + text.includes(sig.substring) + ) ?? null + ); +} + /** * True when a thrown connection-startup error matches a known platform * container-admission failure. These are transient: the platform asks the * caller to try again later, so they are safe to retry within the budget. */ function isPlatformUnavailableError(error: unknown): boolean { - return ( - error instanceof Error && - PLATFORM_UNAVAILABLE_SIGNATURES.some((sig) => - error.message.includes(sig.substring) - ) - ); + return matchPlatformUnavailable(error) !== null; } /** - * Convert a raw connection-startup error into a typed ContainerUnavailableError - * when it matches a known platform container-admission failure. Returns null - * for anything else so the caller preserves the original error. + * Build a typed ContainerUnavailableError for a matched platform + * container-admission failure, preserving the original message verbatim. */ -function tryConvertPlatformUnavailable(error: unknown): Error | null { - if (!(error instanceof Error)) return null; - const match = PLATFORM_UNAVAILABLE_SIGNATURES.find((sig) => - error.message.includes(sig.substring) - ); - if (!match) return null; - +function buildContainerUnavailableError( + reason: (typeof PLATFORM_UNAVAILABLE_SIGNATURES)[number]['reason'], + originalMessage: string, + cause?: unknown +): Error { const context = { - reason: match.reason, + reason, retryable: true as const, - originalMessage: error.message + originalMessage }; return createErrorFromResponse( { code: ErrorCode.CONTAINER_UNAVAILABLE, - message: error.message, + message: originalMessage, context, httpStatus: getHttpStatus(ErrorCode.CONTAINER_UNAVAILABLE), suggestion: getSuggestion(ErrorCode.CONTAINER_UNAVAILABLE, context), timestamp: new Date().toISOString() }, - { cause: error } + cause !== undefined ? { cause } : undefined ); } +/** + * Convert a raw connection-startup error into a typed ContainerUnavailableError + * when it matches a known platform container-admission failure. Returns null + * for anything else so the caller preserves the original error. + */ +function tryConvertPlatformUnavailable(error: unknown): Error | null { + const match = matchPlatformUnavailable(error); + if (!match) return null; + + const originalMessage = + error instanceof Error ? error.message : String(error); + return buildContainerUnavailableError(match.reason, originalMessage, error); +} + // --------------------------------------------------------------------------- // Connection manager // --------------------------------------------------------------------------- diff --git a/packages/sandbox/tests/container-connection.test.ts b/packages/sandbox/tests/container-connection.test.ts index 782717012..1131fb8ca 100644 --- a/packages/sandbox/tests/container-connection.test.ts +++ b/packages/sandbox/tests/container-connection.test.ts @@ -387,6 +387,68 @@ describe('ContainerControlConnection', () => { } }); + it('retries a thrown lower-case no-instance error (containers-library form) until success', async () => { + vi.useFakeTimers(); + try { + // The @cloudflare/containers package uses a lower-case message; the + // matcher must be case-insensitive or this failure skips the retry + // loop entirely (the ~2s failures seen in the repro). + const lowerMessage = + 'there is no container instance that can be provided to this durable object'; + const fetchMock = vi + .fn<(req: Request) => Promise>() + .mockRejectedValueOnce(new Error(lowerMessage)) + .mockResolvedValueOnce(makeUpgradeResponse()); + + const conn = new ContainerControlConnection({ + stub: { fetch: fetchMock }, + retryTimeoutMs: 60_000 + }); + + const connectPromise = conn.connect(); + await vi.advanceTimersByTimeAsync(0); + expect(fetchMock).toHaveBeenCalledTimes(1); + + await vi.advanceTimersByTimeAsync(3_000); + await connectPromise; + + expect(fetchMock).toHaveBeenCalledTimes(2); + expect(conn.isConnected()).toBe(true); + } finally { + vi.useRealTimers(); + } + }); + + it('exhausts the retry budget on a lower-case no-instance error and surfaces ContainerUnavailableError', async () => { + vi.useFakeTimers(); + try { + const lowerMessage = + 'there is no container instance that can be provided to this durable object'; + const fetchMock = vi + .fn<(req: Request) => Promise>() + .mockRejectedValue(new Error(lowerMessage)); + + const conn = new ContainerControlConnection({ + stub: { fetch: fetchMock }, + retryTimeoutMs: 20_000 + }); + + const connectPromise = conn.connect(); + const assertion = expect(connectPromise).rejects.toMatchObject({ + code: 'CONTAINER_UNAVAILABLE', + context: { + reason: 'no_container_instance_available', + retryable: true + } + }); + await vi.advanceTimersByTimeAsync(60_000); + await assertion; + expect(conn.isConnected()).toBe(false); + } finally { + vi.useRealTimers(); + } + }); + it('retries a thrown platform "no container instance" error until success', async () => { vi.useFakeTimers(); try { @@ -627,6 +689,41 @@ describe('ContainerControlConnection', () => { }); describe('structured error classification', () => { + it('classifies a plain-text 503 no-instance body as ContainerUnavailableError', async () => { + // The @cloudflare/containers base class returns a plain-text 503 (not + // JSON) when it cannot admit an instance. After the retry budget is + // exhausted this must still surface as a typed ContainerUnavailableError. + const body = + 'There is no Container instance available at this time.\nThis is likely because you have reached your max concurrent instance count.'; + const conn = new ContainerControlConnection({ + stub: { + fetch: vi.fn().mockResolvedValue( + new Response(body, { + status: 503, + headers: { 'content-type': 'text/plain' } + }) + ) + }, + retryTimeoutMs: 0 + }); + + const error = await conn.connect().catch((e: unknown) => e); + expect((error as { code?: string }).code).toBe('CONTAINER_UNAVAILABLE'); + expect((error as Error).constructor.name).toBe( + 'ContainerUnavailableError' + ); + expect(error).toMatchObject({ + context: { + reason: 'no_container_instance_available', + retryable: true + } + }); + expect( + (error as { context?: { originalMessage?: string } }).context + ?.originalMessage + ).toContain('no Container instance available'); + }); + it('throws ContainerUnavailableError when upgrade response contains structured CONTAINER_UNAVAILABLE body', async () => { const body = JSON.stringify({ code: 'CONTAINER_UNAVAILABLE', diff --git a/packages/sandbox/tests/rpc-container-unavailable.test.ts b/packages/sandbox/tests/rpc-container-unavailable.test.ts new file mode 100644 index 000000000..78b40120c --- /dev/null +++ b/packages/sandbox/tests/rpc-container-unavailable.test.ts @@ -0,0 +1,167 @@ +import { describe, expect, it, vi } from 'vitest'; +import { ContainerControlClient } from '../src/container-control/client'; +import { ContainerUnavailableError } from '../src/errors'; + +/** + * End-to-end RPC-path coverage for platform container-admission failures. + * + * Unlike rpc-sandbox-client.test.ts (which mocks ContainerControlConnection) + * and container-connection.test.ts (which drives the connection directly), + * these tests wire up the *real* stack: + * + * ContainerControlClient + * → real ContainerControlConnection (real capnweb RpcSession + DeferredTransport) + * → stub.fetch() that reproduces the platform failure + * + * The stub throws the same message the Containers runtime raises when it can't + * admit a container ("There is no container instance..."). The test asserts the + * caller of a queued RPC method (`utils.createSession`) receives a typed + * ContainerUnavailableError — not a masked OPERATION_INTERRUPTED / generic + * capnweb disposal error. + * + * These run in workerd via vitest-pool-workers, so the RpcSession, transport, + * and error propagation are exercised for real. + */ +describe('RPC path: platform container-admission failures', () => { + const PLATFORM_MESSAGE = + 'There is no container instance that can be provided to this Durable Object, try again later'; + + function makeClient(fetchImpl: (req: Request) => Promise) { + return new ContainerControlClient({ + stub: { fetch: fetchImpl }, + port: 3000, + // Disable the upgrade retry budget so the test fails fast on the first + // attempt instead of waiting out exponential backoff. + retryTimeoutMs: 0 + }); + } + + it('surfaces ContainerUnavailableError to a queued RPC caller when the platform throws a real Error', async () => { + const client = makeClient(() => + Promise.reject(new Error(PLATFORM_MESSAGE)) + ); + + let thrown: unknown; + try { + await client.utils.createSession({ id: 'test', cwd: '/' }); + } catch (e) { + thrown = e; + } finally { + client.disconnect(); + } + + expect(thrown).toBeInstanceOf(ContainerUnavailableError); + const err = thrown as ContainerUnavailableError; + expect(err.code).toBe('CONTAINER_UNAVAILABLE'); + expect(err.reason).toBe('no_container_instance_available'); + expect(err.context.retryable).toBe(true); + expect(err.context.originalMessage).toContain( + 'no container instance that can be provided' + ); + }); + + it('detects the failure even when the thrown value is a cross-realm-style plain object (no instanceof Error)', async () => { + // Simulate an error raised in another realm: `instanceof Error` is false + // but it still carries a `message`. This is the exact case the previous + // `instanceof Error` gate silently dropped. + const crossRealm = { name: 'Error', message: PLATFORM_MESSAGE }; + const client = makeClient(() => Promise.reject(crossRealm)); + + let thrown: unknown; + try { + await client.utils.createSession({ id: 'test', cwd: '/' }); + } catch (e) { + thrown = e; + } finally { + client.disconnect(); + } + + expect(thrown).toBeInstanceOf(ContainerUnavailableError); + expect((thrown as ContainerUnavailableError).reason).toBe( + 'no_container_instance_available' + ); + }); + + it('detects the failure regardless of message casing', async () => { + const upper = new Error(PLATFORM_MESSAGE.toUpperCase()); + const client = makeClient(() => Promise.reject(upper)); + + let thrown: unknown; + try { + await client.utils.createSession({ id: 'test', cwd: '/' }); + } catch (e) { + thrown = e; + } finally { + client.disconnect(); + } + + expect(thrown).toBeInstanceOf(ContainerUnavailableError); + expect((thrown as ContainerUnavailableError).reason).toBe( + 'no_container_instance_available' + ); + }); + + it('classifies the "max instances exceeded" platform message', async () => { + const message = + 'Maximum number of running container instances exceeded. Try again later, or try configuring a higher value for max_instances'; + const client = makeClient(() => Promise.reject(new Error(message))); + + let thrown: unknown; + try { + await client.utils.createSession({ id: 'test', cwd: '/' }); + } catch (e) { + thrown = e; + } finally { + client.disconnect(); + } + + expect(thrown).toBeInstanceOf(ContainerUnavailableError); + expect((thrown as ContainerUnavailableError).reason).toBe( + 'max_container_instances_exceeded' + ); + }); + + it('retries the thrown platform error within the budget, then succeeds', async () => { + vi.useFakeTimers(); + try { + const ws = { + addEventListener: () => {}, + removeEventListener: () => {}, + send: () => {}, + close: () => {}, + accept: () => {} + } as unknown as WebSocket; + const upgrade = { + status: 101, + statusText: 'Switching Protocols', + webSocket: ws + } as unknown as Response; + + const fetchMock = vi + .fn<(req: Request) => Promise>() + .mockRejectedValueOnce(new Error(PLATFORM_MESSAGE)) + .mockResolvedValueOnce(upgrade); + + const client = new ContainerControlClient({ + stub: { fetch: fetchMock }, + port: 3000, + retryTimeoutMs: 60_000 + }); + + // Kick the connection; don't await the RPC (it would hang waiting for a + // real capnweb response over our fake WS). We only assert the upgrade + // was retried past the first thrown platform error. + void client.connect().catch(() => {}); + + await vi.advanceTimersByTimeAsync(0); + expect(fetchMock).toHaveBeenCalledTimes(1); + + await vi.advanceTimersByTimeAsync(3_000); + expect(fetchMock).toHaveBeenCalledTimes(2); + + client.disconnect(); + } finally { + vi.useRealTimers(); + } + }); +}); diff --git a/packages/sandbox/tests/rpc-no-container-instance.test.ts b/packages/sandbox/tests/rpc-no-container-instance.test.ts new file mode 100644 index 000000000..6fb20c4d9 --- /dev/null +++ b/packages/sandbox/tests/rpc-no-container-instance.test.ts @@ -0,0 +1,208 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +/** + * Reproduces issue #794 end-to-end through a real Sandbox DO on the RPC + * transport. + * + * In production the Containers platform raises + * + * "There is no container instance that can be provided to this Durable + * Object, try again later" + * + * from the container binding when it cannot admit a container. On the RPC + * transport the SDK reaches the container by issuing a WebSocket upgrade + * `fetch()` against the DO itself, which flows into the base + * `@cloudflare/containers` `Container.fetch()`. That is exactly where the + * platform error is thrown, so we reproduce it by throwing the real message + * from the mocked base-class `fetch()` on the WebSocket-upgrade path. + * + * Everything above that point is the real SDK stack: `sandbox.mkdir()` → + * `ensureDefaultSession()` → `this.client.utils.createSession()` → real + * `ContainerControlClient` → real `ContainerControlConnection` (real capnweb + * `RpcSession` + `DeferredTransport`) → real `translateRPCError`. + * + * Before the fix this surfaced as a generic + * `OPERATION_INTERRUPTED` ("utils.createSession was interrupted…"). The test + * asserts the caller now receives a typed `ContainerUnavailableError`. + */ + +const NO_INSTANCE_MESSAGE = + 'There is no container instance that can be provided to this Durable Object, try again later'; + +// Throwing switch so individual tests can pick which platform message the +// base container raises on the WebSocket-upgrade path. +let upgradeError: Error = new Error(NO_INSTANCE_MESSAGE); + +vi.mock('@cloudflare/containers', () => { + const mockSwitchPort = vi.fn((request: Request, port: number) => { + const url = new URL(request.url); + url.pathname = `/proxy/${port}${url.pathname}`; + return new Request(url, request); + }); + + const MockContainer = class Container { + ctx: any; + env: any; + sleepAfter: string | number = '10m'; + constructor(ctx: any, env: any) { + this.ctx = ctx; + this.env = env; + } + async fetch(request: Request): Promise { + const upgradeHeader = request.headers.get('Upgrade'); + if (upgradeHeader?.toLowerCase() === 'websocket') { + // The container binding cannot admit an instance: throw the platform + // error exactly as workerd does. This is the RPC upgrade path. + throw upgradeError; + } + return new Response('Mock Container fetch'); + } + async containerFetch(): Promise { + return new Response('Mock Container HTTP fetch'); + } + async startAndWaitForPorts(): Promise {} + async destroy(): Promise {} + async stop(): Promise {} + async getState() { + return { status: 'healthy' }; + } + renewActivityTimeout() {} + }; + + const MockContainerProxy = class ContainerProxy { + ctx: any; + env: any; + constructor(ctx: any, env: any) { + this.ctx = ctx; + this.env = env; + } + async fetch(): Promise { + return new Response('Mock ContainerProxy fetch'); + } + }; + + return { + Container: MockContainer, + ContainerProxy: MockContainerProxy, + getContainer: vi.fn(), + switchPort: mockSwitchPort + }; +}); + +vi.mock('../src/interpreter', () => ({ + CodeInterpreter: class { + constructor(_getInterpreter: unknown) {} + } +})); + +import { ContainerUnavailableError } from '../src/errors'; +import { Sandbox } from '../src/sandbox'; + +function makeCtx() { + const storageState = new Map(); + const storage = { + get: vi.fn(async (key: string) => storageState.get(key) ?? null), + put: vi.fn(async (key: string, value: unknown) => { + storageState.set(key, value); + }), + delete: vi.fn(async (key: string) => { + storageState.delete(key); + }), + list: vi.fn().mockResolvedValue(new Map()), + transaction: vi.fn(async (cb: (s: unknown) => unknown) => cb(storage)) + }; + return { + storage, + blockConcurrencyWhile: vi + .fn() + .mockImplementation((cb: () => Promise): Promise => cb()), + waitUntil: vi.fn(), + container: { running: false, start: vi.fn() }, + id: { + toString: () => 'test-sandbox-id', + equals: vi.fn(), + name: 'test-sandbox' + } + }; +} + +async function makeRpcSandbox(): Promise { + const ctx = makeCtx(); + // SANDBOX_TRANSPORT=rpc forces the ContainerControlClient (RPC transport). + const env = { SANDBOX_TRANSPORT: 'rpc' } as Record; + const sandbox = new Sandbox( + ctx as unknown as ConstructorParameters[0], + env + ); + await vi.waitFor(() => { + expect(ctx.blockConcurrencyWhile).toHaveBeenCalled(); + }); + await Promise.all( + (ctx.blockConcurrencyWhile as any).mock.results.map( + (r: { value: unknown }) => r.value + ) + ); + // Fail fast: skip the upgrade retry budget so the platform error surfaces + // on the first attempt instead of after ~2 minutes of backoff. + sandbox.client.setRetryTimeoutMs(0); + return sandbox; +} + +describe('RPC transport: no container instance (issue #794)', () => { + beforeEach(() => { + upgradeError = new Error(NO_INSTANCE_MESSAGE); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + it('surfaces ContainerUnavailableError from sandbox.mkdir() (not OPERATION_INTERRUPTED)', async () => { + const sandbox = await makeRpcSandbox(); + + let thrown: unknown; + try { + await sandbox.mkdir('/workspace', { recursive: true }); + } catch (e) { + thrown = e; + } + + expect(thrown).toBeInstanceOf(ContainerUnavailableError); + const err = thrown as ContainerUnavailableError; + expect(err.code).toBe('CONTAINER_UNAVAILABLE'); + expect(err.reason).toBe('no_container_instance_available'); + expect(err.context.retryable).toBe(true); + expect(err.context.originalMessage).toContain( + 'no container instance that can be provided' + ); + // Regression guard: the old behaviour masked this as an interrupted op. + expect((err as Error).message).not.toContain('was interrupted'); + }); + + it('surfaces ContainerUnavailableError from sandbox.exec() as well', async () => { + const sandbox = await makeRpcSandbox(); + + const thrown = await sandbox.exec('echo hi').catch((e: unknown) => e); + + expect(thrown).toBeInstanceOf(ContainerUnavailableError); + expect((thrown as ContainerUnavailableError).reason).toBe( + 'no_container_instance_available' + ); + }); + + it('classifies the "max instances exceeded" platform message', async () => { + upgradeError = new Error( + 'Maximum number of running container instances exceeded. Try again later, or try configuring a higher value for max_instances' + ); + const sandbox = await makeRpcSandbox(); + + const thrown = await sandbox + .mkdir('/workspace', { recursive: true }) + .catch((e: unknown) => e); + + expect(thrown).toBeInstanceOf(ContainerUnavailableError); + expect((thrown as ContainerUnavailableError).reason).toBe( + 'max_container_instances_exceeded' + ); + }); +}); diff --git a/packages/sandbox/tests/rpc-sandbox-client.test.ts b/packages/sandbox/tests/rpc-sandbox-client.test.ts index a806390e6..495ca58ea 100644 --- a/packages/sandbox/tests/rpc-sandbox-client.test.ts +++ b/packages/sandbox/tests/rpc-sandbox-client.test.ts @@ -785,6 +785,68 @@ describe('translateRPCError', () => { expect(thrown).toBeInstanceOf(OperationInterruptedError); }); + it('prefers a structured (non-instanceof) CONTAINER_UNAVAILABLE connection error over the disposal error', async () => { + // Reproduces the masked failure: a queued createSession rejects with + // capnweb's disposal message, and the connection captured a *structured* + // but cross-realm error-like object (not an instanceof SandboxError). It + // must still surface as ContainerUnavailableError, not + // OperationInterruptedError. + const translateRPCError = await loadFn(); + const { ContainerUnavailableError } = await loadErr(); + const connectionError = { + name: 'Error', + code: 'CONTAINER_UNAVAILABLE', + message: + 'There is no container instance that can be provided to this Durable Object, try again later', + context: { + reason: 'no_container_instance_available', + retryable: true, + originalMessage: + 'There is no container instance that can be provided to this Durable Object, try again later' + } + }; + let thrown: unknown; + try { + translateRPCError( + new Error('RPC session was shut down by disposing the main stub'), + { operation: 'utils.createSession', connectionError } + ); + } catch (e) { + thrown = e; + } + expect(thrown).toBeInstanceOf(ContainerUnavailableError); + const err = thrown as InstanceType; + expect(err.code).toBe('CONTAINER_UNAVAILABLE'); + expect(err.reason).toBe('no_container_instance_available'); + expect(err.context.retryable).toBe(true); + }); + + it('rehydrates a structured connection error carried in `details`', async () => { + const translateRPCError = await loadFn(); + const { ContainerUnavailableError } = await loadErr(); + const connectionError = { + code: 'CONTAINER_UNAVAILABLE', + message: 'no instance', + details: { + reason: 'max_container_instances_exceeded', + retryable: true + } + }; + let thrown: unknown; + try { + translateRPCError(new Error('WebSocket connection failed.'), { + operation: 'utils.createSession', + connectionError + }); + } catch (e) { + thrown = e; + } + expect(thrown).toBeInstanceOf(ContainerUnavailableError); + expect( + (thrown as InstanceType).reason + ).toBe('max_container_instances_exceeded'); + }); + it('does not set `cause` on errors translated from JSON-encoded structured payloads', async () => { // The container-side errors flow through createErrorFromResponse without // an `options` argument, so `cause` stays unset (matching pre-fix From 7df77dac9c3e3434b7b987f3e3a3787aa1f5e881 Mon Sep 17 00:00:00 2001 From: Aron <263346377+aron-cf@users.noreply.github.com> Date: Wed, 1 Jul 2026 14:15:14 +0100 Subject: [PATCH 06/24] Emit structured CONTAINER_UNAVAILABLE from containerFetch startup The no-instance startup branch returned a generic INTERNAL_ERROR 503 with a rewritten message, so the RPC control connection could not classify it and callers saw rpc_upgrade_failed after the retry budget drained. Emit a CONTAINER_UNAVAILABLE 503 preserving the platform message and classify max-instances capacity failures the same way, so callers receive a typed ContainerUnavailableError with an actionable reason. --- .changeset/surface-container-unavailable.md | 4 +- .../src/container-control/connection.ts | 7 ++- packages/sandbox/src/sandbox.ts | 63 +++++++++++++++---- .../tests/container-connection.test.ts | 37 +++++++++++ .../tests/rpc-no-container-instance.test.ts | 29 +++++---- .../tests/sandbox-error-handling.test.ts | 51 +++++++++------ 6 files changed, 148 insertions(+), 43 deletions(-) diff --git a/.changeset/surface-container-unavailable.md b/.changeset/surface-container-unavailable.md index d9b07bc74..b771f0562 100644 --- a/.changeset/surface-container-unavailable.md +++ b/.changeset/surface-container-unavailable.md @@ -2,4 +2,6 @@ '@cloudflare/sandbox': patch --- -Surface container allocation failures as `ContainerUnavailableError` instead of masking them as generic `utils.createSession` interruptions. When the Containers platform cannot admit a container during startup ("There is no container instance that can be provided to this Durable Object", the plain-text 503 "There is no Container instance available at this time", or "Maximum number of running container instances exceeded"), callers now receive the real, retryable cause with structured context. Detection is case-insensitive and realm-safe, and captured connection errors are preferred by structure (any `CONTAINER_UNAVAILABLE`-coded value) rather than only same-realm `SandboxError` instances, so the failure is no longer masked as an interrupted operation. Callers can distinguish the causes via `error.reason` (`'no_container_instance_available'` vs `'max_container_instances_exceeded'`), typed by the new exported `ContainerUnavailableReason`. +Surface container allocation failures as `ContainerUnavailableError` instead of masking them as generic `utils.createSession` interruptions. When the Containers platform cannot admit a container during startup ("There is no container instance that can be provided to this Durable Object" or "Maximum number of running container instances exceeded"), the SDK now retries within the startup budget and, if it remains unavailable, surfaces a typed `ContainerUnavailableError` carrying the real platform message and an actionable `reason`. + +`Sandbox.containerFetch` now emits a structured `CONTAINER_UNAVAILABLE` 503 (preserving the original platform message) for these admission failures instead of a generic `INTERNAL_ERROR`, so the RPC control connection classifies them correctly. Detection is case-insensitive and realm-safe, and captured connection errors are preferred by structure (any `CONTAINER_UNAVAILABLE`-coded value) rather than only same-realm `SandboxError` instances. Callers can distinguish the causes via `error.reason` (`'no_container_instance_available'` vs `'max_container_instances_exceeded'`), typed by the new exported `ContainerUnavailableReason`. diff --git a/packages/sandbox/src/container-control/connection.ts b/packages/sandbox/src/container-control/connection.ts index 1d7ad749a..7ab0ad76f 100644 --- a/packages/sandbox/src/container-control/connection.ts +++ b/packages/sandbox/src/container-control/connection.ts @@ -64,7 +64,9 @@ async function tryParseContainerUnavailable( body.context?.reason === 'container_starting' || body.context?.reason === 'container_unhealthy' || body.context?.reason === 'container_replaced' || - body.context?.reason === 'rpc_upgrade_failed' + body.context?.reason === 'rpc_upgrade_failed' || + body.context?.reason === 'no_container_instance_available' || + body.context?.reason === 'max_container_instances_exceeded' ? body.context.reason : 'container_replaced'; const context = { @@ -72,6 +74,9 @@ async function tryParseContainerUnavailable( retryable: true as const, ...(typeof body.context?.retryAfterMs === 'number' && { retryAfterMs: body.context.retryAfterMs + }), + ...(typeof body.context?.originalMessage === 'string' && { + originalMessage: body.context.originalMessage }) }; diff --git a/packages/sandbox/src/sandbox.ts b/packages/sandbox/src/sandbox.ts index d1505f365..b5f27bf93 100644 --- a/packages/sandbox/src/sandbox.ts +++ b/packages/sandbox/src/sandbox.ts @@ -3052,17 +3052,28 @@ export class Sandbox extends Container implements ISandbox { } }); } catch (e) { - // 1. Provisioning: Container VM not yet available - if (this.isNoInstanceError(e)) { + // 1. Provisioning: the Containers platform cannot admit an instance + // yet ("There is no container instance that can be provided..."). + // Emit a structured CONTAINER_UNAVAILABLE body and preserve the + // original platform message so the caller — including the RPC + // control connection's upgrade-response classifier — surfaces a + // typed ContainerUnavailableError with an actionable reason rather + // than a generic INTERNAL_ERROR / rpc_upgrade_failed. + const admissionReason = this.classifyContainerAdmissionError(e); + if (admissionReason) { + const originalMessage = e instanceof Error ? e.message : String(e); + const context = { + reason: admissionReason, + retryable: true as const, + originalMessage + }; const errorBody: ErrorResponse = { - code: ErrorCode.INTERNAL_ERROR, - message: - 'Container is currently provisioning. This can take several minutes on first deployment.', - context: { phase: 'provisioning' }, + code: ErrorCode.CONTAINER_UNAVAILABLE, + message: originalMessage, + context, httpStatus: 503, timestamp: new Date().toISOString(), - suggestion: - 'This is expected during first deployment. The SDK will retry automatically.' + suggestion: getSuggestion(ErrorCode.CONTAINER_UNAVAILABLE, context) }; return new Response(JSON.stringify(errorBody), { status: 503, @@ -3182,10 +3193,38 @@ export class Sandbox extends Container implements ISandbox { * This indicates the container VM is still being provisioned. */ private isNoInstanceError(error: unknown): boolean { - return ( - error instanceof Error && - error.message.toLowerCase().includes('no container instance') - ); + return this.classifyContainerAdmissionError(error) !== null; + } + + /** + * Classify a container-startup error as a platform admission/capacity + * failure, returning the categorical `ContainerUnavailableContext.reason` + * or null if it is not a recognized admission failure. + * + * Realm-safe and case-insensitive: the platform raises these from the + * container binding, which may live in a different realm, so we coerce to + * a string rather than gating on `instanceof Error`. + */ + private classifyContainerAdmissionError( + error: unknown + ): + | 'no_container_instance_available' + | 'max_container_instances_exceeded' + | null { + const message = + (error as { message?: unknown } | null | undefined)?.message ?? error; + const text = ( + typeof message === 'string' ? message : String(message) + ).toLowerCase(); + if (text.includes('no container instance')) { + return 'no_container_instance_available'; + } + if ( + text.includes('maximum number of running container instances exceeded') + ) { + return 'max_container_instances_exceeded'; + } + return null; } /** diff --git a/packages/sandbox/tests/container-connection.test.ts b/packages/sandbox/tests/container-connection.test.ts index 1131fb8ca..c80bb7d50 100644 --- a/packages/sandbox/tests/container-connection.test.ts +++ b/packages/sandbox/tests/container-connection.test.ts @@ -724,6 +724,43 @@ describe('ContainerControlConnection', () => { ).toContain('no Container instance available'); }); + it('preserves reason and originalMessage from a structured no-instance JSON body', async () => { + // Mirrors what Sandbox.containerFetch now emits for the platform + // no-instance failure: a JSON 503 with the real reason + message. + const platformMessage = + 'There is no container instance that can be provided to this Durable Object, try again later'; + const body = JSON.stringify({ + code: 'CONTAINER_UNAVAILABLE', + message: platformMessage, + context: { + reason: 'no_container_instance_available', + retryable: true, + originalMessage: platformMessage + } + }); + const conn = new ContainerControlConnection({ + stub: { + fetch: vi.fn().mockResolvedValue( + new Response(body, { + status: 503, + headers: { 'content-type': 'application/json' } + }) + ) + }, + retryTimeoutMs: 0 + }); + + const error = await conn.connect().catch((e: unknown) => e); + expect((error as { code?: string }).code).toBe('CONTAINER_UNAVAILABLE'); + expect(error).toMatchObject({ + context: { + reason: 'no_container_instance_available', + retryable: true, + originalMessage: platformMessage + } + }); + }); + it('throws ContainerUnavailableError when upgrade response contains structured CONTAINER_UNAVAILABLE body', async () => { const body = JSON.stringify({ code: 'CONTAINER_UNAVAILABLE', diff --git a/packages/sandbox/tests/rpc-no-container-instance.test.ts b/packages/sandbox/tests/rpc-no-container-instance.test.ts index 6fb20c4d9..c51fab078 100644 --- a/packages/sandbox/tests/rpc-no-container-instance.test.ts +++ b/packages/sandbox/tests/rpc-no-container-instance.test.ts @@ -48,23 +48,30 @@ vi.mock('@cloudflare/containers', () => { this.ctx = ctx; this.env = env; } + // Base-class fetch: mirrors @cloudflare/containers, which forwards all + // requests (including the RPC WebSocket upgrade) to containerFetch. + // Dynamic dispatch routes this to Sandbox.containerFetch (the override), + // so the platform no-instance error flows through the SDK's real + // classification path rather than being thrown straight to the caller. async fetch(request: Request): Promise { - const upgradeHeader = request.headers.get('Upgrade'); - if (upgradeHeader?.toLowerCase() === 'websocket') { - // The container binding cannot admit an instance: throw the platform - // error exactly as workerd does. This is the RPC upgrade path. - throw upgradeError; - } - return new Response('Mock Container fetch'); + return ( + this as unknown as { + containerFetch: (req: Request, port?: number) => Promise; + } + ).containerFetch(request, 3000); } - async containerFetch(): Promise { - return new Response('Mock Container HTTP fetch'); + // Base-class startAndWaitForPorts: the platform cannot admit an instance. + // Throwing here is exactly what workerd's container binding does; the + // Sandbox.containerFetch override catches it via isNoInstanceError and + // converts it into a structured CONTAINER_UNAVAILABLE 503 response. + async startAndWaitForPorts(): Promise { + throw upgradeError; } - async startAndWaitForPorts(): Promise {} async destroy(): Promise {} async stop(): Promise {} async getState() { - return { status: 'healthy' }; + // Non-healthy so containerFetch runs its startup path. + return { status: 'stopped' }; } renewActivityTimeout() {} }; diff --git a/packages/sandbox/tests/sandbox-error-handling.test.ts b/packages/sandbox/tests/sandbox-error-handling.test.ts index ac84bee30..690b06e45 100644 --- a/packages/sandbox/tests/sandbox-error-handling.test.ts +++ b/packages/sandbox/tests/sandbox-error-handling.test.ts @@ -250,18 +250,27 @@ describe('Sandbox.containerFetch() error classification', () => { ); }); - describe('no instance error → 503 with provisioning message', () => { - it('returns 503 with provisioning message for "no container instance"', async () => { - const response = await triggerContainerFetchWithError( - 'there is no container instance that can be provided to this durable object' - ); + describe('no instance error → 503 CONTAINER_UNAVAILABLE', () => { + it('returns a structured CONTAINER_UNAVAILABLE 503 for "no container instance"', async () => { + const platformMessage = + 'there is no container instance that can be provided to this durable object'; + const response = await triggerContainerFetchWithError(platformMessage); expect(response.status).toBe(503); expect(response.headers.get('Retry-After')).toBe('10'); - expect( - ((await response.json()) as { context: { phase: string } }).context - .phase - ).toBe('provisioning'); + const body = (await response.json()) as { + code: string; + message: string; + context: { + reason: string; + retryable: boolean; + originalMessage: string; + }; + }; + expect(body.code).toBe('CONTAINER_UNAVAILABLE'); + expect(body.context.reason).toBe('no_container_instance_available'); + expect(body.context.retryable).toBe(true); + expect(body.context.originalMessage).toBe(platformMessage); }); it('returns 503 for case-insensitive "No Container Instance" match', async () => { @@ -345,17 +354,23 @@ describe('Sandbox.containerFetch() error classification', () => { }); }); describe('unrecognized errors → 503 (safe to retry)', () => { - it('returns 503 for max instances exceeded (recoverable capacity limit)', async () => { - // Confirmed via platform source: TOOMANYDURABLEOBJECTS resets the retry timer - // and adds 10s backoff, expecting the condition to clear as load drops. - // Lands in unrecognized tier (Retry-After: 5) since the message doesn't - // match a known transient pattern, but still gets 503 for safe retry. - const response = await triggerContainerFetchWithError( - 'maximum number of running container instances exceeded. Try again later, or try configuring a higher value for max_instances' - ); + it('returns a structured CONTAINER_UNAVAILABLE 503 for max instances exceeded', async () => { + // Platform capacity limit (TOOMANYDURABLEOBJECTS). Classified as a + // container-admission failure so callers receive a typed + // ContainerUnavailableError with an actionable reason. + const platformMessage = + 'maximum number of running container instances exceeded. Try again later, or try configuring a higher value for max_instances'; + const response = await triggerContainerFetchWithError(platformMessage); expect(response.status).toBe(503); - expect(response.headers.get('Retry-After')).toBe('5'); + expect(response.headers.get('Retry-After')).toBe('10'); + const body = (await response.json()) as { + code: string; + context: { reason: string; retryable: boolean }; + }; + expect(body.code).toBe('CONTAINER_UNAVAILABLE'); + expect(body.context.reason).toBe('max_container_instances_exceeded'); + expect(body.context.retryable).toBe(true); }); it('returns 503 for "container already exists" errors (workerd)', async () => { From 77f1f94f0c592b19913459d226b21a819d0f55b8 Mon Sep 17 00:00:00 2001 From: Aron <263346377+aron-cf@users.noreply.github.com> Date: Wed, 1 Jul 2026 14:35:58 +0100 Subject: [PATCH 07/24] Start container explicitly before RPC WebSocket upgrade Drive startAndWaitForPorts from the control connection's own retry loop instead of triggering start as a side effect of the upgrade fetch. The platform's no-instance / max-instances error now throws where we can classify it directly into a typed ContainerUnavailableError, giving one retry authority and removing the dependency on round-tripping the failure through a 503 upgrade-response body. --- .changeset/surface-container-unavailable.md | 2 +- .../sandbox/src/container-control/client.ts | 4 ++ .../src/container-control/connection.ts | 29 ++++++++++ packages/sandbox/src/sandbox.ts | 20 +++++++ .../tests/container-connection.test.ts | 58 +++++++++++++++++++ 5 files changed, 112 insertions(+), 1 deletion(-) diff --git a/.changeset/surface-container-unavailable.md b/.changeset/surface-container-unavailable.md index b771f0562..8281f505f 100644 --- a/.changeset/surface-container-unavailable.md +++ b/.changeset/surface-container-unavailable.md @@ -4,4 +4,4 @@ Surface container allocation failures as `ContainerUnavailableError` instead of masking them as generic `utils.createSession` interruptions. When the Containers platform cannot admit a container during startup ("There is no container instance that can be provided to this Durable Object" or "Maximum number of running container instances exceeded"), the SDK now retries within the startup budget and, if it remains unavailable, surfaces a typed `ContainerUnavailableError` carrying the real platform message and an actionable `reason`. -`Sandbox.containerFetch` now emits a structured `CONTAINER_UNAVAILABLE` 503 (preserving the original platform message) for these admission failures instead of a generic `INTERNAL_ERROR`, so the RPC control connection classifies them correctly. Detection is case-insensitive and realm-safe, and captured connection errors are preferred by structure (any `CONTAINER_UNAVAILABLE`-coded value) rather than only same-realm `SandboxError` instances. Callers can distinguish the causes via `error.reason` (`'no_container_instance_available'` vs `'max_container_instances_exceeded'`), typed by the new exported `ContainerUnavailableReason`. +On the RPC transport the control connection now starts the container explicitly (via `startAndWaitForPorts`) before opening the WebSocket, so admission failures throw inside the connection's own retry loop and are classified directly — rather than being triggered as a side effect of the upgrade fetch and round-tripped through a 503 response body. `Sandbox.containerFetch` also emits a structured `CONTAINER_UNAVAILABLE` 503 (preserving the original platform message) for these failures on the HTTP path. Detection is case-insensitive and realm-safe, and captured connection errors are preferred by structure (any `CONTAINER_UNAVAILABLE`-coded value) rather than only same-realm `SandboxError` instances. Callers can distinguish the causes via `error.reason` (`'no_container_instance_available'` vs `'max_container_instances_exceeded'`), typed by the new exported `ContainerUnavailableReason`. diff --git a/packages/sandbox/src/container-control/client.ts b/packages/sandbox/src/container-control/client.ts index f32e36de9..258981984 100644 --- a/packages/sandbox/src/container-control/client.ts +++ b/packages/sandbox/src/container-control/client.ts @@ -603,6 +603,10 @@ export class ContainerControlClient { localMain: options.localMain, logger: options.logger, retryTimeoutMs: options.retryTimeoutMs, + // Explicit container-start hook: when provided, the connection starts + // the container in its own retry loop before the WebSocket upgrade so + // capacity failures throw where we can classify them directly. + startContainer: options.startContainer, // Event-driven failure recovery: when the live WebSocket closes // or errors, tear the connection down inside the same turn of // the event loop so the next RPC call builds a fresh one. The diff --git a/packages/sandbox/src/container-control/connection.ts b/packages/sandbox/src/container-control/connection.ts index 7ab0ad76f..f12279c46 100644 --- a/packages/sandbox/src/container-control/connection.ts +++ b/packages/sandbox/src/container-control/connection.ts @@ -236,6 +236,22 @@ export interface ContainerControlConnectionOptions { stub: ContainerFetchStub; port?: number; logger?: Logger; + /** + * Optional hook to explicitly start the container (and wait for its ports) + * before issuing the WebSocket upgrade. + * + * When provided, the platform's container-admission failure ("no container + * instance" / "max instances exceeded") is thrown here — synchronously in + * the connection's own retry loop — so it is retried within the upgrade + * budget and, when exhausted, classified and surfaced directly as a typed + * `ContainerUnavailableError` via `onConnectionError`. This avoids relying + * on the failure being round-tripped through a 503 upgrade-response body. + * + * When omitted, the upgrade fetch itself triggers container start (the + * legacy behavior, still exercised by unit tests that pass a bare `fetch` + * stub). + */ + startContainer?: (signal: AbortSignal) => Promise; /** * Total retry budget (ms) for retryable upgrade responses while the * container is unavailable. Defaults to 120 000 (2 minutes), matching the @@ -291,6 +307,9 @@ export class ContainerControlConnection { private retryTimeoutMs: number; private readonly onClose: (() => void) | undefined; private readonly onConnectionError: ((error: unknown) => void) | undefined; + private readonly startContainer: + | ((signal: AbortSignal) => Promise) + | undefined; constructor(options: ContainerControlConnectionOptions) { this.containerStub = options.stub; @@ -299,6 +318,7 @@ export class ContainerControlConnection { this.retryTimeoutMs = options.retryTimeoutMs ?? DEFAULT_RETRY_TIMEOUT_MS; this.onClose = options.onClose; this.onConnectionError = options.onConnectionError; + this.startContainer = options.startContainer; this.transport = new DeferredTransport(); this.session = new RpcSession( @@ -556,6 +576,15 @@ export class ContainerControlConnection { ); try { + // Explicitly start the container first (when a hook is provided). The + // platform's no-instance / capacity error is thrown here, in our own + // retry loop, rather than surfacing as a 503 upgrade-response body. + // `shouldRetryError` retries it within the budget; once exhausted the + // throw is classified by `doConnect`'s catch into a typed + // ContainerUnavailableError. + if (this.startContainer) { + await this.startContainer(controller.signal); + } const url = `http://localhost:${this.port}/rpc`; const request = new Request(url, { headers: { diff --git a/packages/sandbox/src/sandbox.ts b/packages/sandbox/src/sandbox.ts index b5f27bf93..859e5b6f2 100644 --- a/packages/sandbox/src/sandbox.ts +++ b/packages/sandbox/src/sandbox.ts @@ -1142,6 +1142,26 @@ export class Sandbox extends Container implements ISandbox { port: 3000, logger: this.logger, retryTimeoutMs: this.computeRetryTimeoutMs(), + // Explicitly start the container before the RPC WebSocket upgrade. + // Running start() here — rather than as a side effect of the upgrade + // fetch through containerFetch() — makes the platform's + // container-admission failure ("no container instance" / "max + // instances exceeded") throw inside the connection's own retry loop, + // where it is classified and surfaced as a typed + // ContainerUnavailableError instead of being round-tripped through a + // 503 upgrade-response body. Each attempt uses a fresh per-attempt + // abort signal from the connection; the container timeouts come from + // the DO's current configuration. + startContainer: (signal: AbortSignal) => + this.startAndWaitForPorts({ + ports: 3000, + cancellationOptions: { + instanceGetTimeoutMS: this.containerTimeouts.instanceGetTimeoutMS, + portReadyTimeoutMS: this.containerTimeouts.portReadyTimeoutMS, + waitInterval: this.containerTimeouts.waitIntervalMS, + abort: signal + } + }), // localMain exposes the DO-side control callback (tunnel-exit // notifications, etc.) to the container side of the session. localMain: this.controlCallback, diff --git a/packages/sandbox/tests/container-connection.test.ts b/packages/sandbox/tests/container-connection.test.ts index c80bb7d50..8b4095f9f 100644 --- a/packages/sandbox/tests/container-connection.test.ts +++ b/packages/sandbox/tests/container-connection.test.ts @@ -318,6 +318,64 @@ describe('ContainerControlConnection', () => { }); } + it('surfaces ContainerUnavailableError when explicit startContainer throws a platform error', async () => { + const platformMessage = + 'There is no container instance that can be provided to this Durable Object, try again later'; + const fetchMock = vi.fn(); + const conn = new ContainerControlConnection({ + stub: { fetch: fetchMock }, + startContainer: () => Promise.reject(new Error(platformMessage)), + retryTimeoutMs: 0 + }); + + const error = await conn.connect().catch((e: unknown) => e); + expect((error as { code?: string }).code).toBe('CONTAINER_UNAVAILABLE'); + expect(error).toMatchObject({ + context: { + reason: 'no_container_instance_available', + retryable: true, + originalMessage: platformMessage + } + }); + // The upgrade fetch is never attempted when start fails. + expect(fetchMock).not.toHaveBeenCalled(); + }); + + it('retries a thrown startContainer platform error, then upgrades on success', async () => { + vi.useFakeTimers(); + try { + const platformMessage = + 'there is no container instance that can be provided to this durable object'; + const startContainer = vi + .fn<(signal: AbortSignal) => Promise>() + .mockRejectedValueOnce(new Error(platformMessage)) + .mockResolvedValueOnce(undefined); + const fetchMock = vi + .fn<(req: Request) => Promise>() + .mockResolvedValue(makeUpgradeResponse()); + + const conn = new ContainerControlConnection({ + stub: { fetch: fetchMock }, + startContainer, + retryTimeoutMs: 60_000 + }); + + const connectPromise = conn.connect(); + await vi.advanceTimersByTimeAsync(0); + expect(startContainer).toHaveBeenCalledTimes(1); + expect(fetchMock).not.toHaveBeenCalled(); + + await vi.advanceTimersByTimeAsync(3_000); + await connectPromise; + + expect(startContainer).toHaveBeenCalledTimes(2); + expect(fetchMock).toHaveBeenCalledTimes(1); + expect(conn.isConnected()).toBe(true); + } finally { + vi.useRealTimers(); + } + }); + it('retries retryable upgrade responses until success', async () => { vi.useFakeTimers(); try { From 96e5df36634f0b51a912aa88d4d4e0e04518a1b0 Mon Sep 17 00:00:00 2001 From: Aron <263346377+aron-cf@users.noreply.github.com> Date: Wed, 1 Jul 2026 15:09:01 +0100 Subject: [PATCH 08/24] Only report OPERATION_INTERRUPTED for established sessions Gate the interrupted-operation mapping on whether the capnweb session ever connected to a running container. When the container never started, a queued RPC that rejects with a generic capnweb disposal error now surfaces the real transport/connection error directly instead of being masked as a runtime interruption. Also scope the per-attempt connect timeout to the WebSocket upgrade only, letting explicit container start run under its own budget so the classifiable no-instance error can surface. --- .changeset/surface-container-unavailable.md | 2 +- .../sandbox/src/container-control/client.ts | 83 +++++++++++++++---- .../src/container-control/connection.ts | 71 ++++++++++++---- packages/sandbox/src/sandbox.ts | 18 ++-- .../tests/container-connection.test.ts | 2 +- .../sandbox/tests/rpc-client-retry.test.ts | 7 ++ .../sandbox/tests/rpc-sandbox-client.test.ts | 68 +++++++++++++++ 7 files changed, 210 insertions(+), 41 deletions(-) diff --git a/.changeset/surface-container-unavailable.md b/.changeset/surface-container-unavailable.md index 8281f505f..44e5a1101 100644 --- a/.changeset/surface-container-unavailable.md +++ b/.changeset/surface-container-unavailable.md @@ -4,4 +4,4 @@ Surface container allocation failures as `ContainerUnavailableError` instead of masking them as generic `utils.createSession` interruptions. When the Containers platform cannot admit a container during startup ("There is no container instance that can be provided to this Durable Object" or "Maximum number of running container instances exceeded"), the SDK now retries within the startup budget and, if it remains unavailable, surfaces a typed `ContainerUnavailableError` carrying the real platform message and an actionable `reason`. -On the RPC transport the control connection now starts the container explicitly (via `startAndWaitForPorts`) before opening the WebSocket, so admission failures throw inside the connection's own retry loop and are classified directly — rather than being triggered as a side effect of the upgrade fetch and round-tripped through a 503 response body. `Sandbox.containerFetch` also emits a structured `CONTAINER_UNAVAILABLE` 503 (preserving the original platform message) for these failures on the HTTP path. Detection is case-insensitive and realm-safe, and captured connection errors are preferred by structure (any `CONTAINER_UNAVAILABLE`-coded value) rather than only same-realm `SandboxError` instances. Callers can distinguish the causes via `error.reason` (`'no_container_instance_available'` vs `'max_container_instances_exceeded'`), typed by the new exported `ContainerUnavailableReason`. +On the RPC transport the control connection now starts the container explicitly (via `startAndWaitForPorts`) before opening the WebSocket, so admission failures throw inside the connection's own retry loop and are classified directly — rather than being triggered as a side effect of the upgrade fetch and round-tripped through a 503 response body. A queued RPC call is now only reported as `OPERATION_INTERRUPTED` when the session actually established a connection to a running container and was then interrupted; if the container never started, the underlying transport/connection error is surfaced directly instead of being masked as an interruption. `Sandbox.containerFetch` also emits a structured `CONTAINER_UNAVAILABLE` 503 (preserving the original platform message) for these failures on the HTTP path. Detection is case-insensitive and realm-safe, and captured connection errors are preferred by structure (any `CONTAINER_UNAVAILABLE`-coded value) rather than only same-realm `SandboxError` instances. Callers can distinguish the causes via `error.reason` (`'no_container_instance_available'` vs `'max_container_instances_exceeded'`), typed by the new exported `ContainerUnavailableReason`. diff --git a/packages/sandbox/src/container-control/client.ts b/packages/sandbox/src/container-control/client.ts index 258981984..5f25ed5f6 100644 --- a/packages/sandbox/src/container-control/client.ts +++ b/packages/sandbox/src/container-control/client.ts @@ -175,6 +175,17 @@ export interface RPCTranslationContext { * is preferred over the masking transport error. */ connectionError?: unknown; + /** + * Whether the capnweb session ever established a live connection to a + * running container during the current connection's lifetime. + * + * `OPERATION_INTERRUPTED` means "the operation was admitted to a running + * runtime, then interrupted." If the session never established (the + * container never started — e.g. no instance available), that framing is + * wrong: the operation was never admitted. In that case we surface the + * thrown transport error directly instead of masking it as an interruption. + */ + sessionEstablished?: boolean; } export function translateRPCError( @@ -244,10 +255,17 @@ export function translateRPCError( // connection-failure message capnweb raises on the queued call. const captured = maybePreferConnectionError(transportResponse, context); if (captured) throw captured; - const interruptedResponse = buildInterruptedOperationResponse( - transportResponse, - context - ); + // Only map to OPERATION_INTERRUPTED when the session actually established + // a live connection to a running container and was then interrupted. If + // it never connected (container never started), surface the thrown + // transport error directly — the operation was never admitted, so + // "interrupted" would be misleading. `sessionEstablished` defaults to + // undefined for callers that don't track it (e.g. direct unit tests), + // preserving the prior behavior in that case. + const interruptedResponse = + context.sessionEstablished === false + ? null + : buildInterruptedOperationResponse(transportResponse, context); throw createErrorFromResponse( (interruptedResponse ?? transportResponse) as unknown as ErrorResponse, { cause: error } @@ -480,7 +498,8 @@ function wrapStub( domain: string, onCallStarted: () => void, onCallSettled: () => void, - getConnectionError: () => unknown + getConnectionError: () => unknown, + getSessionEstablished: () => boolean ): T { return new Proxy(stub, { get(target, prop, receiver) { @@ -508,7 +527,8 @@ function wrapStub( .catch((err: unknown) => translateRPCError(err, { operation, - connectionError: getConnectionError() + connectionError: getConnectionError(), + sessionEstablished: getSessionEstablished() }) ) .finally(onCallSettled); @@ -519,7 +539,8 @@ function wrapStub( onCallSettled(); translateRPCError(err, { operation, - connectionError: getConnectionError() + connectionError: getConnectionError(), + sessionEstablished: getSessionEstablished() }); } }; @@ -589,6 +610,14 @@ export class ContainerControlClient { * fresh connection is created. */ private lastConnectionError: unknown = null; + /** + * Whether the current connection ever established a live session to a + * running container. Set true by the connection's `onConnected` callback, + * reset when a fresh connection is created. Lets `translateRPCError` + * distinguish a true interruption (established, then dropped) from a + * never-connected failure (container never started). + */ + private sessionEstablished = false; private idleTimer: ReturnType | null = null; private busyPollTimer: ReturnType | null = null; /** Number of RPC method promises that have started but not settled. */ @@ -607,6 +636,12 @@ export class ContainerControlClient { // the container in its own retry loop before the WebSocket upgrade so // capacity failures throw where we can classify them directly. startContainer: options.startContainer, + // Record that the session actually connected to a running container, so + // a later teardown is classified as a true OPERATION_INTERRUPTED rather + // than surfacing as a never-connected failure. + onConnected: () => { + this.sessionEstablished = true; + }, // Event-driven failure recovery: when the live WebSocket closes // or errors, tear the connection down inside the same turn of // the event loop so the next RPC call builds a fresh one. The @@ -645,6 +680,7 @@ export class ContainerControlClient { private getConnection(): ContainerControlConnection { if (!this.conn) { this.lastConnectionError = null; + this.sessionEstablished = false; this.conn = new ContainerControlConnection(this.connOptions); this.startBusyPoll(); } @@ -710,6 +746,9 @@ export class ContainerControlClient { /** Return the last connection-startup error captured, if any. */ private getLastConnectionError = (): unknown => this.lastConnectionError; + /** Whether the current connection ever established a live session. */ + private getSessionEstablished = (): boolean => this.sessionEstablished; + /** * Sample `getStats()` and update busy/idle state. While busy, renews the * activity timeout each tick so an in-flight stream keeps pushing the @@ -822,7 +861,8 @@ export class ContainerControlClient { 'commands', this.recordCallStarted, this.recordCallSettled, - this.getLastConnectionError + this.getLastConnectionError, + this.getSessionEstablished ); } get files(): SandboxFilesAPI { @@ -831,7 +871,8 @@ export class ContainerControlClient { 'files', this.recordCallStarted, this.recordCallSettled, - this.getLastConnectionError + this.getLastConnectionError, + this.getSessionEstablished ) as unknown as SandboxFilesAPI; } get processes(): SandboxProcessesAPI { @@ -840,7 +881,8 @@ export class ContainerControlClient { 'processes', this.recordCallStarted, this.recordCallSettled, - this.getLastConnectionError + this.getLastConnectionError, + this.getSessionEstablished ); } get ports(): SandboxPortsAPI { @@ -849,7 +891,8 @@ export class ContainerControlClient { 'ports', this.recordCallStarted, this.recordCallSettled, - this.getLastConnectionError + this.getLastConnectionError, + this.getSessionEstablished ); } get git(): SandboxGitAPI { @@ -858,7 +901,8 @@ export class ContainerControlClient { 'git', this.recordCallStarted, this.recordCallSettled, - this.getLastConnectionError + this.getLastConnectionError, + this.getSessionEstablished ); } get utils(): SandboxUtilsAPI { @@ -867,7 +911,8 @@ export class ContainerControlClient { 'utils', this.recordCallStarted, this.recordCallSettled, - this.getLastConnectionError + this.getLastConnectionError, + this.getSessionEstablished ); } get backup(): SandboxBackupAPI { @@ -876,7 +921,8 @@ export class ContainerControlClient { 'backup', this.recordCallStarted, this.recordCallSettled, - this.getLastConnectionError + this.getLastConnectionError, + this.getSessionEstablished ); } get watch(): SandboxWatchAPI { @@ -885,7 +931,8 @@ export class ContainerControlClient { 'watch', this.recordCallStarted, this.recordCallSettled, - this.getLastConnectionError + this.getLastConnectionError, + this.getSessionEstablished ); } get tunnels(): SandboxTunnelsAPI { @@ -894,7 +941,8 @@ export class ContainerControlClient { 'tunnels', this.recordCallStarted, this.recordCallSettled, - this.getLastConnectionError + this.getLastConnectionError, + this.getSessionEstablished ); } get interpreter(): SandboxInterpreterAPI { @@ -903,7 +951,8 @@ export class ContainerControlClient { 'interpreter', this.recordCallStarted, this.recordCallSettled, - this.getLastConnectionError + this.getLastConnectionError, + this.getSessionEstablished ); } diff --git a/packages/sandbox/src/container-control/connection.ts b/packages/sandbox/src/container-control/connection.ts index f12279c46..5f6784166 100644 --- a/packages/sandbox/src/container-control/connection.ts +++ b/packages/sandbox/src/container-control/connection.ts @@ -250,8 +250,13 @@ export interface ContainerControlConnectionOptions { * When omitted, the upgrade fetch itself triggers container start (the * legacy behavior, still exercised by unit tests that pass a bare `fetch` * stub). + * + * Runs under the container's own instance-get / port-ready budget — it is + * intentionally NOT passed the per-attempt connect-timeout signal, so the + * base class can run its budget to exhaustion and throw the classifiable + * no-instance error rather than a generic abort error. */ - startContainer?: (signal: AbortSignal) => Promise; + startContainer?: () => Promise; /** * Total retry budget (ms) for retryable upgrade responses while the * container is unavailable. Defaults to 120 000 (2 minutes), matching the @@ -285,6 +290,13 @@ export interface ContainerControlConnectionOptions { * Fired at most once per connection attempt. Not fired for `disconnect()`. */ onConnectionError?: (error: unknown) => void; + /** + * Invoked once when the WebSocket upgrade succeeds and the capnweb session + * is live. Lets the owner record that the session actually established a + * connection to a running container, so a later teardown can be classified + * as a true interruption rather than a never-connected failure. + */ + onConnected?: () => void; } /** @@ -307,9 +319,8 @@ export class ContainerControlConnection { private retryTimeoutMs: number; private readonly onClose: (() => void) | undefined; private readonly onConnectionError: ((error: unknown) => void) | undefined; - private readonly startContainer: - | ((signal: AbortSignal) => Promise) - | undefined; + private readonly onConnected: (() => void) | undefined; + private readonly startContainer: (() => Promise) | undefined; constructor(options: ContainerControlConnectionOptions) { this.containerStub = options.stub; @@ -318,6 +329,7 @@ export class ContainerControlConnection { this.retryTimeoutMs = options.retryTimeoutMs ?? DEFAULT_RETRY_TIMEOUT_MS; this.onClose = options.onClose; this.onConnectionError = options.onConnectionError; + this.onConnected = options.onConnected; this.startContainer = options.startContainer; this.transport = new DeferredTransport(); @@ -513,6 +525,19 @@ export class ContainerControlConnection { this.ws = ws; this.transport.activate(ws); this.connected = true; + // Record that a live session was established so a later teardown is + // classified as a true interruption rather than a never-connected + // failure. Swallow listener errors like the other fire* helpers. + try { + this.onConnected?.(); + } catch (err) { + this.logger.warn( + 'ContainerControlConnection onConnected handler threw', + { + error: err instanceof Error ? err.message : String(err) + } + ); + } this.logger.debug('ContainerControlConnection established', { port: this.port @@ -564,11 +589,34 @@ export class ContainerControlConnection { } /** - * Single WebSocket-upgrade fetch attempt. Owns its own AbortController so - * each retry gets a fresh per-attempt connect timeout independent of the - * total retry budget. + * Single WebSocket-upgrade fetch attempt. + * + * Container start and the upgrade fetch have deliberately separate timeout + * scopes: + * + * - `startContainer()` runs under the base `Container` class's own + * instance-get / port-ready budget. It must NOT be cut short by the + * per-attempt connect timeout: the base class only emits the + * classifiable `NO_CONTAINER_INSTANCE_ERROR` after its internal budget + * is exhausted, and it checks its abort signal *before* that throw + * (see @cloudflare/containers `doStartContainer`). Aborting it early + * yields a generic "Aborted waiting for container to start" error that + * we can't classify — which is what masked the real cause as + * OPERATION_INTERRUPTED. Letting it run its own budget means the + * platform capacity failure throws here, in our retry loop, where + * `shouldRetryError` retries it and `doConnect`'s catch converts it to + * a typed ContainerUnavailableError. + * + * - The WebSocket upgrade fetch gets its own fresh AbortController so + * each retry has an independent connect timeout for the *upgrade only*. */ private async fetchUpgradeAttempt(): Promise { + // Explicitly start the container first (when a hook is provided), under + // its own budget — no per-attempt abort signal. + if (this.startContainer) { + await this.startContainer(); + } + const controller = new AbortController(); const timeout = setTimeout( () => controller.abort(), @@ -576,15 +624,6 @@ export class ContainerControlConnection { ); try { - // Explicitly start the container first (when a hook is provided). The - // platform's no-instance / capacity error is thrown here, in our own - // retry loop, rather than surfacing as a 503 upgrade-response body. - // `shouldRetryError` retries it within the budget; once exhausted the - // throw is classified by `doConnect`'s catch into a typed - // ContainerUnavailableError. - if (this.startContainer) { - await this.startContainer(controller.signal); - } const url = `http://localhost:${this.port}/rpc`; const request = new Request(url, { headers: { diff --git a/packages/sandbox/src/sandbox.ts b/packages/sandbox/src/sandbox.ts index 859e5b6f2..9bb04c6d2 100644 --- a/packages/sandbox/src/sandbox.ts +++ b/packages/sandbox/src/sandbox.ts @@ -1149,17 +1149,23 @@ export class Sandbox extends Container implements ISandbox { // instances exceeded") throw inside the connection's own retry loop, // where it is classified and surfaced as a typed // ContainerUnavailableError instead of being round-tripped through a - // 503 upgrade-response body. Each attempt uses a fresh per-attempt - // abort signal from the connection; the container timeouts come from - // the DO's current configuration. - startContainer: (signal: AbortSignal) => + // 503 upgrade-response body. + // + // No abort signal is passed: the base Container class only emits the + // classifiable NO_CONTAINER_INSTANCE_ERROR after its own instance-get + // budget is exhausted, and it checks the abort signal *before* that + // throw. Passing the connection's per-attempt connect-timeout signal + // here would abort start early with a generic "Aborted waiting for + // container to start" error that can't be classified — masking the + // real capacity cause as OPERATION_INTERRUPTED. Letting start run + // under its own timeouts lets the real error surface. + startContainer: () => this.startAndWaitForPorts({ ports: 3000, cancellationOptions: { instanceGetTimeoutMS: this.containerTimeouts.instanceGetTimeoutMS, portReadyTimeoutMS: this.containerTimeouts.portReadyTimeoutMS, - waitInterval: this.containerTimeouts.waitIntervalMS, - abort: signal + waitInterval: this.containerTimeouts.waitIntervalMS } }), // localMain exposes the DO-side control callback (tunnel-exit diff --git a/packages/sandbox/tests/container-connection.test.ts b/packages/sandbox/tests/container-connection.test.ts index 8b4095f9f..babd880d9 100644 --- a/packages/sandbox/tests/container-connection.test.ts +++ b/packages/sandbox/tests/container-connection.test.ts @@ -347,7 +347,7 @@ describe('ContainerControlConnection', () => { const platformMessage = 'there is no container instance that can be provided to this durable object'; const startContainer = vi - .fn<(signal: AbortSignal) => Promise>() + .fn<() => Promise>() .mockRejectedValueOnce(new Error(platformMessage)) .mockResolvedValueOnce(undefined); const fetchMock = vi diff --git a/packages/sandbox/tests/rpc-client-retry.test.ts b/packages/sandbox/tests/rpc-client-retry.test.ts index 2ba0ddd86..24b7e657c 100644 --- a/packages/sandbox/tests/rpc-client-retry.test.ts +++ b/packages/sandbox/tests/rpc-client-retry.test.ts @@ -11,6 +11,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; interface CapturedOptions { retryTimeoutMs?: number; + onConnected?: () => void; } interface CapturedRPCMain { @@ -39,8 +40,14 @@ const captured: { vi.mock('../src/container-control/connection', () => ({ ContainerControlConnection: class { + private onConnected: (() => void) | undefined; constructor(options: CapturedOptions) { captured.options.push(options); + this.onConnected = options.onConnected; + // Reflect an established session so disposed-mid-call errors classify as + // OPERATION_INTERRUPTED (a true interruption), matching real behavior + // once the WebSocket upgrade has succeeded. + this.onConnected?.(); } setRetryTimeoutMs(ms: number) { captured.setRetryTimeoutCalls.push(ms); diff --git a/packages/sandbox/tests/rpc-sandbox-client.test.ts b/packages/sandbox/tests/rpc-sandbox-client.test.ts index 495ca58ea..fa66ef06a 100644 --- a/packages/sandbox/tests/rpc-sandbox-client.test.ts +++ b/packages/sandbox/tests/rpc-sandbox-client.test.ts @@ -785,6 +785,74 @@ describe('translateRPCError', () => { expect(thrown).toBeInstanceOf(OperationInterruptedError); }); + it('maps to OPERATION_INTERRUPTED only when the session was established', async () => { + const translateRPCError = await loadFn(); + const { OperationInterruptedError } = await loadErr(); + let thrown: unknown; + try { + translateRPCError( + new Error('RPC session was shut down by disposing the main stub'), + { operation: 'utils.createSession', sessionEstablished: true } + ); + } catch (e) { + thrown = e; + } + expect(thrown).toBeInstanceOf(OperationInterruptedError); + }); + + it('surfaces the transport error (not OPERATION_INTERRUPTED) when the session never established', async () => { + // The container never started, so the session never connected. A queued + // call rejecting with capnweb's disposal message must NOT be reported as + // an interruption — the operation was never admitted. Surface the raw + // transport error instead. + const translateRPCError = await loadFn(); + const { RPCTransportError, OperationInterruptedError } = await loadErr(); + let thrown: unknown; + try { + translateRPCError( + new Error('RPC session was shut down by disposing the main stub'), + { operation: 'utils.createSession', sessionEstablished: false } + ); + } catch (e) { + thrown = e; + } + expect(thrown).not.toBeInstanceOf(OperationInterruptedError); + expect(thrown).toBeInstanceOf(RPCTransportError); + expect((thrown as InstanceType).kind).toBe( + 'session_disposed' + ); + }); + + it('still prefers a captured connection error even when the session never established', async () => { + const translateRPCError = await loadFn(); + const { ContainerUnavailableError } = await loadErr(); + const connectionError = new ContainerUnavailableError({ + code: 'CONTAINER_UNAVAILABLE', + message: 'no instance', + context: { + reason: 'no_container_instance_available', + retryable: true, + originalMessage: 'no instance' + }, + httpStatus: 503, + timestamp: new Date().toISOString() + }); + let thrown: unknown; + try { + translateRPCError( + new Error('RPC session was shut down by disposing the main stub'), + { + operation: 'utils.createSession', + connectionError, + sessionEstablished: false + } + ); + } catch (e) { + thrown = e; + } + expect(thrown).toBeInstanceOf(ContainerUnavailableError); + }); + it('prefers a structured (non-instanceof) CONTAINER_UNAVAILABLE connection error over the disposal error', async () => { // Reproduces the masked failure: a queued createSession rejects with // capnweb's disposal message, and the connection captured a *structured* From e33124fbb39196015d707754aa7f9ce50c1b34db Mon Sep 17 00:00:00 2001 From: Aron <263346377+aron-cf@users.noreply.github.com> Date: Wed, 1 Jul 2026 16:06:13 +0100 Subject: [PATCH 09/24] Start container under a short budget with typed errors on the RPC path Run the RPC transport's explicit container start under a small instance-get budget so each attempt fails fast under capacity pressure and hands back to the control connection's retry loop, instead of one attempt blocking ~30s and letting the DO be evicted mid-wait. Port readiness keeps the full timeout so a booting app is not cut short. The start hook now always throws a typed SandboxError: a retryable ContainerUnavailableError for capacity failures, or an INTERNAL_ERROR carrying the real cause otherwise, so the caller never sees a raw capnweb transport string. --- .changeset/surface-container-unavailable.md | 2 +- packages/sandbox/src/sandbox.ts | 115 ++++++++++++++++-- .../tests/rpc-no-container-instance.test.ts | 25 +++- 3 files changed, 128 insertions(+), 14 deletions(-) diff --git a/.changeset/surface-container-unavailable.md b/.changeset/surface-container-unavailable.md index 44e5a1101..887083bf3 100644 --- a/.changeset/surface-container-unavailable.md +++ b/.changeset/surface-container-unavailable.md @@ -4,4 +4,4 @@ Surface container allocation failures as `ContainerUnavailableError` instead of masking them as generic `utils.createSession` interruptions. When the Containers platform cannot admit a container during startup ("There is no container instance that can be provided to this Durable Object" or "Maximum number of running container instances exceeded"), the SDK now retries within the startup budget and, if it remains unavailable, surfaces a typed `ContainerUnavailableError` carrying the real platform message and an actionable `reason`. -On the RPC transport the control connection now starts the container explicitly (via `startAndWaitForPorts`) before opening the WebSocket, so admission failures throw inside the connection's own retry loop and are classified directly — rather than being triggered as a side effect of the upgrade fetch and round-tripped through a 503 response body. A queued RPC call is now only reported as `OPERATION_INTERRUPTED` when the session actually established a connection to a running container and was then interrupted; if the container never started, the underlying transport/connection error is surfaced directly instead of being masked as an interruption. `Sandbox.containerFetch` also emits a structured `CONTAINER_UNAVAILABLE` 503 (preserving the original platform message) for these failures on the HTTP path. Detection is case-insensitive and realm-safe, and captured connection errors are preferred by structure (any `CONTAINER_UNAVAILABLE`-coded value) rather than only same-realm `SandboxError` instances. Callers can distinguish the causes via `error.reason` (`'no_container_instance_available'` vs `'max_container_instances_exceeded'`), typed by the new exported `ContainerUnavailableReason`. +On the RPC transport the control connection now starts the container explicitly before opening the WebSocket, so admission failures throw inside the connection's own retry loop and are classified directly — rather than being triggered as a side effect of the upgrade fetch and round-tripped through a 503 response body. That explicit start runs under a short instance-get budget so each attempt fails fast under capacity pressure and hands back to the connection's retry loop (with backoff) instead of one attempt blocking ~30s and letting the Durable Object be evicted mid-wait; port readiness still uses the full timeout so a genuinely-booting app isn't cut short. The start hook always throws a typed error — a retryable `ContainerUnavailableError` for capacity failures, or an `INTERNAL_ERROR`-coded error carrying the real cause otherwise — so the caller never sees a raw capnweb transport string. A queued RPC call is now only reported as `OPERATION_INTERRUPTED` when the session actually established a connection to a running container and was then interrupted; if the container never started, the underlying transport/connection error is surfaced directly instead of being masked as an interruption. `Sandbox.containerFetch` also emits a structured `CONTAINER_UNAVAILABLE` 503 (preserving the original platform message) for these failures on the HTTP path. Detection is case-insensitive and realm-safe, and captured connection errors are preferred by structure (any `CONTAINER_UNAVAILABLE`-coded value) rather than only same-realm `SandboxError` instances. Callers can distinguish the causes via `error.reason` (`'no_container_instance_available'` vs `'max_container_instances_exceeded'`), typed by the new exported `ContainerUnavailableReason`. diff --git a/packages/sandbox/src/sandbox.ts b/packages/sandbox/src/sandbox.ts index 9bb04c6d2..f8290b473 100644 --- a/packages/sandbox/src/sandbox.ts +++ b/packages/sandbox/src/sandbox.ts @@ -92,6 +92,7 @@ import { SandboxError, SessionAlreadyExistsError } from './errors'; +import { createErrorFromResponse } from './errors/adapter'; import { collectFile, streamFile } from './file-stream'; import { CodeInterpreter } from './interpreter'; import { LocalMountSyncManager } from './local-mount-sync'; @@ -391,6 +392,24 @@ const R2_DEFAULT_S3FS_OPTION_ENTRIES = Object.entries( const S3FS_DISABLE_EXPECT_HEADER_CONFIG = ' Expect:\n'; const BACKUP_DEFAULT_TTL_SECONDS = 259200; + +/** + * Instance-get budget (ms) for the RPC transport's explicit container-start + * hook, independent of the user-facing `containerTimeouts.instanceGetTimeoutMS` + * (which still governs the HTTP path's `containerFetch`). + * + * Deliberately small: it bounds how long a single start attempt polls for a + * container instance before the base class throws the classifiable + * `NO_CONTAINER_INSTANCE_ERROR`. The RPC control connection's own retry loop + * (`fetchWithResponseRetry`, ~2 min budget with exponential backoff) owns the + * cross-attempt retries, so a short inner budget means each attempt fails fast + * under capacity pressure and hands back to the outer loop — rather than one + * attempt burning ~30s and letting the DO be evicted mid-wait. Once an + * instance exists, `portReadyTimeoutMS` still governs app boot on the next + * attempt (the base fast-path returns immediately when the container is + * already running). + */ +const RPC_START_INSTANCE_GET_TIMEOUT_MS = 3_000; const BACKUP_MAX_NAME_LENGTH = 256; const BACKUP_CONTAINER_DIR = '/var/backups'; const BACKUP_STORAGE_PREFIX = 'backups'; @@ -1156,18 +1175,10 @@ export class Sandbox extends Container implements ISandbox { // budget is exhausted, and it checks the abort signal *before* that // throw. Passing the connection's per-attempt connect-timeout signal // here would abort start early with a generic "Aborted waiting for - // container to start" error that can't be classified — masking the - // real capacity cause as OPERATION_INTERRUPTED. Letting start run - // under its own timeouts lets the real error surface. - startContainer: () => - this.startAndWaitForPorts({ - ports: 3000, - cancellationOptions: { - instanceGetTimeoutMS: this.containerTimeouts.instanceGetTimeoutMS, - portReadyTimeoutMS: this.containerTimeouts.portReadyTimeoutMS, - waitInterval: this.containerTimeouts.waitIntervalMS - } - }), + // container to start" error that can't be classified. Instead we run + // start under a short instance-get budget and always throw a typed + // SandboxError — see startContainerForRpc. + startContainer: () => this.startContainerForRpc(), // localMain exposes the DO-side control callback (tunnel-exit // notifications, etc.) to the container side of the session. localMain: this.controlCallback, @@ -3253,6 +3264,86 @@ export class Sandbox extends Container implements ISandbox { return null; } + /** + * Explicit container-start hook for the RPC transport. + * + * Runs `startAndWaitForPorts` under a short instance-get budget + * ({@link RPC_START_INSTANCE_GET_TIMEOUT_MS}) so a single attempt fails fast + * under capacity pressure — the base class throws the classifiable + * `NO_CONTAINER_INSTANCE_ERROR` once that budget is exhausted, and the RPC + * control connection's own retry loop (with backoff) owns cross-attempt + * retries. Port readiness still uses the full `portReadyTimeoutMS` so a + * genuinely-booting app isn't cut short once an instance exists. + * + * Always throws a typed `SandboxError` on failure so the control connection + * never surfaces a raw capnweb/transport string to the caller: + * - container-admission/capacity failures → retryable + * `ContainerUnavailableError` (preserving the platform message so the + * connection's `shouldRetryError` still recognizes and retries it); + * - anything else (platform-transient DO reset, network loss, unexpected + * startup failure) → `INTERNAL_ERROR`-coded SandboxError carrying the + * original message. + */ + private async startContainerForRpc(): Promise { + try { + await this.startAndWaitForPorts({ + ports: 3000, + cancellationOptions: { + instanceGetTimeoutMS: RPC_START_INSTANCE_GET_TIMEOUT_MS, + portReadyTimeoutMS: this.containerTimeouts.portReadyTimeoutMS, + waitInterval: this.containerTimeouts.waitIntervalMS + } + }); + } catch (error) { + throw this.toContainerStartError(error); + } + } + + /** + * Convert a container-start failure into a typed SandboxError. Preserves an + * existing SandboxError as-is; maps platform admission/capacity failures to + * a retryable ContainerUnavailableError; and wraps everything else as an + * INTERNAL_ERROR carrying the original message (never a raw transport + * string). + */ + private toContainerStartError(error: unknown): Error { + if (error instanceof SandboxError) return error; + + const originalMessage = + error instanceof Error ? error.message : String(error); + const admissionReason = this.classifyContainerAdmissionError(error); + if (admissionReason) { + const context = { + reason: admissionReason, + retryable: true as const, + originalMessage + }; + return createErrorFromResponse( + { + code: ErrorCode.CONTAINER_UNAVAILABLE, + message: originalMessage, + context, + httpStatus: getHttpStatus(ErrorCode.CONTAINER_UNAVAILABLE), + suggestion: getSuggestion(ErrorCode.CONTAINER_UNAVAILABLE, context), + timestamp: new Date().toISOString() + }, + { cause: error } + ); + } + + const context = { phase: 'startup' as const, error: originalMessage }; + return createErrorFromResponse( + { + code: ErrorCode.INTERNAL_ERROR, + message: `Container failed to start: ${originalMessage}`, + context, + httpStatus: getHttpStatus(ErrorCode.INTERNAL_ERROR), + timestamp: new Date().toISOString() + }, + { cause: error } + ); + } + /** * Helper: Check if error is a transient startup error that should trigger retry * diff --git a/packages/sandbox/tests/rpc-no-container-instance.test.ts b/packages/sandbox/tests/rpc-no-container-instance.test.ts index c51fab078..248d95e40 100644 --- a/packages/sandbox/tests/rpc-no-container-instance.test.ts +++ b/packages/sandbox/tests/rpc-no-container-instance.test.ts @@ -102,7 +102,7 @@ vi.mock('../src/interpreter', () => ({ } })); -import { ContainerUnavailableError } from '../src/errors'; +import { ContainerUnavailableError, SandboxError } from '../src/errors'; import { Sandbox } from '../src/sandbox'; function makeCtx() { @@ -212,4 +212,27 @@ describe('RPC transport: no container instance (issue #794)', () => { 'max_container_instances_exceeded' ); }); + + it('wraps a non-admission start failure as a typed SandboxError (no raw transport string)', async () => { + // A startup failure that is NOT a capacity/admission error (e.g. a + // platform DO reset) must still surface as a typed SandboxError carrying + // the real cause — never the raw capnweb "RPC session was shut down by + // disposing the main stub" string. + upgradeError = new Error( + 'Durable Object reset because its code was updated.' + ); + const sandbox = await makeRpcSandbox(); + + const thrown = await sandbox + .mkdir('/workspace', { recursive: true }) + .catch((e: unknown) => e); + + expect(thrown).toBeInstanceOf(SandboxError); + expect(thrown).not.toBeInstanceOf(ContainerUnavailableError); + const err = thrown as SandboxError; + expect(err.code).toBe('INTERNAL_ERROR'); + expect((err as Error).message).toContain('code was updated'); + expect((err as Error).message).not.toContain('disposing the main stub'); + expect((err as Error).message).not.toContain('was interrupted'); + }); }); From 336e14ffffdf8131a0ab2339f71ecc3b96a2b723 Mon Sep 17 00:00:00 2001 From: Aron <263346377+aron-cf@users.noreply.github.com> Date: Wed, 1 Jul 2026 16:12:16 +0100 Subject: [PATCH 10/24] Uppercase RPC in identifiers, prune redundant conversion, trim changeset Rename startContainerForRpc -> startContainerForRPC and the ContainerRpc interface -> ContainerRPC per the uppercase-acronym rule. Short-circuit tryConvertPlatformUnavailable for values that are already SandboxError so a typed hook error isn't rebuilt. Shorten the container-unavailable changeset to one line. --- .changeset/surface-container-unavailable.md | 4 +--- packages/sandbox/src/bridge/warm-pool.ts | 10 +++++----- packages/sandbox/src/container-control/connection.ts | 5 +++++ packages/sandbox/src/sandbox.ts | 6 +++--- 4 files changed, 14 insertions(+), 11 deletions(-) diff --git a/.changeset/surface-container-unavailable.md b/.changeset/surface-container-unavailable.md index 887083bf3..0bc4d45bf 100644 --- a/.changeset/surface-container-unavailable.md +++ b/.changeset/surface-container-unavailable.md @@ -2,6 +2,4 @@ '@cloudflare/sandbox': patch --- -Surface container allocation failures as `ContainerUnavailableError` instead of masking them as generic `utils.createSession` interruptions. When the Containers platform cannot admit a container during startup ("There is no container instance that can be provided to this Durable Object" or "Maximum number of running container instances exceeded"), the SDK now retries within the startup budget and, if it remains unavailable, surfaces a typed `ContainerUnavailableError` carrying the real platform message and an actionable `reason`. - -On the RPC transport the control connection now starts the container explicitly before opening the WebSocket, so admission failures throw inside the connection's own retry loop and are classified directly — rather than being triggered as a side effect of the upgrade fetch and round-tripped through a 503 response body. That explicit start runs under a short instance-get budget so each attempt fails fast under capacity pressure and hands back to the connection's retry loop (with backoff) instead of one attempt blocking ~30s and letting the Durable Object be evicted mid-wait; port readiness still uses the full timeout so a genuinely-booting app isn't cut short. The start hook always throws a typed error — a retryable `ContainerUnavailableError` for capacity failures, or an `INTERNAL_ERROR`-coded error carrying the real cause otherwise — so the caller never sees a raw capnweb transport string. A queued RPC call is now only reported as `OPERATION_INTERRUPTED` when the session actually established a connection to a running container and was then interrupted; if the container never started, the underlying transport/connection error is surfaced directly instead of being masked as an interruption. `Sandbox.containerFetch` also emits a structured `CONTAINER_UNAVAILABLE` 503 (preserving the original platform message) for these failures on the HTTP path. Detection is case-insensitive and realm-safe, and captured connection errors are preferred by structure (any `CONTAINER_UNAVAILABLE`-coded value) rather than only same-realm `SandboxError` instances. Callers can distinguish the causes via `error.reason` (`'no_container_instance_available'` vs `'max_container_instances_exceeded'`), typed by the new exported `ContainerUnavailableReason`. +Surface container capacity failures as retryable `ContainerUnavailableError` (with a `reason`) instead of masking them as `utils.createSession` interruptions or raw transport errors. diff --git a/packages/sandbox/src/bridge/warm-pool.ts b/packages/sandbox/src/bridge/warm-pool.ts index 0b6c42a04..123d4f1f4 100644 --- a/packages/sandbox/src/bridge/warm-pool.ts +++ b/packages/sandbox/src/bridge/warm-pool.ts @@ -42,7 +42,7 @@ export interface PoolStats { // Container RPC shapes (inherited by Sandbox from Container) // --------------------------------------------------------------------------- -interface ContainerRpc { +interface ContainerRPC { startAndWaitForPorts(): Promise; stop(signal?: string): Promise; renewActivityTimeout(): void; @@ -205,7 +205,7 @@ export class WarmPool extends DurableObject { for (const containerUUID of [...this.warmContainers]) { try { const stub = this.getSandboxStub(containerUUID); - await (stub as unknown as ContainerRpc).stop(); + await (stub as unknown as ContainerRPC).stop(); this.warmContainers.delete(containerUUID); } catch (error) { console.error({ @@ -296,7 +296,7 @@ export class WarmPool extends DurableObject { try { const stub = this.getSandboxStub(containerUUID); - await (stub as unknown as ContainerRpc).startAndWaitForPorts(); + await (stub as unknown as ContainerRPC).startAndWaitForPorts(); console.info({ message: 'Container started', component: 'warm-pool', @@ -364,7 +364,7 @@ export class WarmPool extends DurableObject { for (const containerUUID of this.warmContainers) { try { const stub = this.getSandboxStub(containerUUID); - (stub as unknown as ContainerRpc).renewActivityTimeout(); + (stub as unknown as ContainerRPC).renewActivityTimeout(); } catch (error) { console.error({ message: 'Failed to renew activity timeout', @@ -455,7 +455,7 @@ export class WarmPool extends DurableObject { for (const uuid of toStop) { try { const stub = this.getSandboxStub(uuid); - await (stub as unknown as ContainerRpc).stop(); + await (stub as unknown as ContainerRPC).stop(); stopped.push(uuid); } catch (error) { console.error({ diff --git a/packages/sandbox/src/container-control/connection.ts b/packages/sandbox/src/container-control/connection.ts index 5f6784166..8ca9516c0 100644 --- a/packages/sandbox/src/container-control/connection.ts +++ b/packages/sandbox/src/container-control/connection.ts @@ -23,6 +23,7 @@ import { createNoOpLogger } from '@repo/shared'; import { ErrorCode, getHttpStatus, getSuggestion } from '@repo/shared/errors'; import { RpcSession, type RpcStub, type RpcTransport } from 'capnweb'; import { createErrorFromResponse } from '../errors/adapter'; +import { SandboxError } from '../errors/classes'; import { fetchWithResponseRetry, isRetryableWebSocketUpgradeResponse @@ -211,6 +212,10 @@ function buildContainerUnavailableError( * for anything else so the caller preserves the original error. */ function tryConvertPlatformUnavailable(error: unknown): Error | null { + // Already a typed SDK error (e.g. thrown by the DO's startContainer hook): + // preserve it rather than rebuilding an equivalent instance. + if (error instanceof SandboxError) return error; + const match = matchPlatformUnavailable(error); if (!match) return null; diff --git a/packages/sandbox/src/sandbox.ts b/packages/sandbox/src/sandbox.ts index f8290b473..ce5ff4146 100644 --- a/packages/sandbox/src/sandbox.ts +++ b/packages/sandbox/src/sandbox.ts @@ -1177,8 +1177,8 @@ export class Sandbox extends Container implements ISandbox { // here would abort start early with a generic "Aborted waiting for // container to start" error that can't be classified. Instead we run // start under a short instance-get budget and always throw a typed - // SandboxError — see startContainerForRpc. - startContainer: () => this.startContainerForRpc(), + // SandboxError — see startContainerForRPC. + startContainer: () => this.startContainerForRPC(), // localMain exposes the DO-side control callback (tunnel-exit // notifications, etc.) to the container side of the session. localMain: this.controlCallback, @@ -3284,7 +3284,7 @@ export class Sandbox extends Container implements ISandbox { * startup failure) → `INTERNAL_ERROR`-coded SandboxError carrying the * original message. */ - private async startContainerForRpc(): Promise { + private async startContainerForRPC(): Promise { try { await this.startAndWaitForPorts({ ports: 3000, From d786e71028a4e89dc2714588ea70de4fdeaf02ef Mon Sep 17 00:00:00 2001 From: Aron <263346377+aron-cf@users.noreply.github.com> Date: Wed, 1 Jul 2026 16:20:51 +0100 Subject: [PATCH 11/24] Surface never-connected teardown as retryable ContainerUnavailableError When the session never established a live connection and a queued RPC rejects with a teardown-family transport error (disposed / connection failed / peer closed), synthesize a clean retryable ContainerUnavailableError instead of leaking the raw capnweb "RPC session was shut down by disposing the main stub" string. This covers DO eviction mid-startup under capacity pressure, where no structured connection cause was captured. Non-teardown transport errors still surface as RPCTransportError. --- .../sandbox/src/container-control/client.ts | 80 ++++++++++++++++--- .../sandbox/tests/rpc-sandbox-client.test.ts | 38 +++++++-- 2 files changed, 102 insertions(+), 16 deletions(-) diff --git a/packages/sandbox/src/container-control/client.ts b/packages/sandbox/src/container-control/client.ts index 5f25ed5f6..4e36b9b99 100644 --- a/packages/sandbox/src/container-control/client.ts +++ b/packages/sandbox/src/container-control/client.ts @@ -85,6 +85,7 @@ import type { } from '@repo/shared'; import { createNoOpLogger } from '@repo/shared'; import { + type ContainerUnavailableContext, ErrorCode, type ErrorResponse, getHttpStatus, @@ -255,17 +256,27 @@ export function translateRPCError( // connection-failure message capnweb raises on the queued call. const captured = maybePreferConnectionError(transportResponse, context); if (captured) throw captured; - // Only map to OPERATION_INTERRUPTED when the session actually established - // a live connection to a running container and was then interrupted. If - // it never connected (container never started), surface the thrown - // transport error directly — the operation was never admitted, so - // "interrupted" would be misleading. `sessionEstablished` defaults to - // undefined for callers that don't track it (e.g. direct unit tests), - // preserving the prior behavior in that case. - const interruptedResponse = - context.sessionEstablished === false - ? null - : buildInterruptedOperationResponse(transportResponse, context); + // If the session never established a live connection, don't map to + // OPERATION_INTERRUPTED (nothing was admitted, so "interrupted" is + // misleading) and don't leak the raw capnweb disposal string. A + // teardown-family error here means the container was never reachable — + // surface a clean, retryable CONTAINER_UNAVAILABLE. `sessionEstablished` + // defaults to undefined for callers that don't track it (e.g. direct unit + // tests), preserving the prior behavior in that case. + if (context.sessionEstablished === false) { + const neverConnected = buildNeverConnectedUnavailableResponse( + transportResponse, + context + ); + throw createErrorFromResponse( + (neverConnected ?? transportResponse) as unknown as ErrorResponse, + { cause: error } + ); + } + const interruptedResponse = buildInterruptedOperationResponse( + transportResponse, + context + ); throw createErrorFromResponse( (interruptedResponse ?? transportResponse) as unknown as ErrorResponse, { cause: error } @@ -439,6 +450,53 @@ function maybePreferConnectionError( return null; } +/** + * When the session never established a live connection and a queued RPC call + * rejects with a teardown-family transport error (disposed / connection + * failed / peer closed), surface a clean, retryable `ContainerUnavailableError` + * instead of the raw capnweb string (e.g. "RPC session was shut down by + * disposing the main stub"). + * + * Reaching this point means: the container never became reachable (so no + * OPERATION_INTERRUPTED — nothing was admitted) AND no structured connection + * error was captured (so `maybePreferConnectionError` didn't fire). That's the + * Durable Object being torn down/evicted mid-startup under capacity pressure + * before `doConnect` recorded a cause — which is, from the caller's view, the + * container being unavailable. Retryable, with the raw transport message + * preserved as `originalMessage` for diagnostics. + */ +function buildNeverConnectedUnavailableResponse( + transportResponse: ErrorResponse, + context: RPCTranslationContext +): ErrorResponse | null { + if (context.sessionEstablished !== false) return null; + const { kind } = transportResponse.context; + if ( + kind !== 'session_disposed' && + kind !== 'connection_failed' && + kind !== 'peer_closed' + ) { + return null; + } + const ctx: ContainerUnavailableContext = { + reason: 'container_replaced', + retryable: true, + originalMessage: transportResponse.context.originalMessage + }; + return { + code: ErrorCode.CONTAINER_UNAVAILABLE, + message: + 'The sandbox container could not be reached before the connection was torn down; it may be starting or temporarily unavailable.', + context: ctx, + httpStatus: getHttpStatus(ErrorCode.CONTAINER_UNAVAILABLE), + suggestion: getSuggestion( + ErrorCode.CONTAINER_UNAVAILABLE, + ctx as unknown as Record + ), + timestamp: new Date().toISOString() + }; +} + function buildInterruptedOperationResponse( transportResponse: ErrorResponse, context: RPCTranslationContext diff --git a/packages/sandbox/tests/rpc-sandbox-client.test.ts b/packages/sandbox/tests/rpc-sandbox-client.test.ts index fa66ef06a..f1eb015ae 100644 --- a/packages/sandbox/tests/rpc-sandbox-client.test.ts +++ b/packages/sandbox/tests/rpc-sandbox-client.test.ts @@ -800,13 +800,15 @@ describe('translateRPCError', () => { expect(thrown).toBeInstanceOf(OperationInterruptedError); }); - it('surfaces the transport error (not OPERATION_INTERRUPTED) when the session never established', async () => { + it('surfaces a retryable ContainerUnavailableError (not a raw transport string) when the session never established', async () => { // The container never started, so the session never connected. A queued // call rejecting with capnweb's disposal message must NOT be reported as - // an interruption — the operation was never admitted. Surface the raw - // transport error instead. + // an interruption (nothing was admitted) and must NOT leak the raw + // capnweb string. Surface a clean, retryable CONTAINER_UNAVAILABLE that + // preserves the transport message for diagnostics. const translateRPCError = await loadFn(); - const { RPCTransportError, OperationInterruptedError } = await loadErr(); + const { ContainerUnavailableError, OperationInterruptedError } = + await loadErr(); let thrown: unknown; try { translateRPCError( @@ -817,9 +819,35 @@ describe('translateRPCError', () => { thrown = e; } expect(thrown).not.toBeInstanceOf(OperationInterruptedError); + expect(thrown).toBeInstanceOf(ContainerUnavailableError); + const err = thrown as InstanceType; + expect(err.code).toBe('CONTAINER_UNAVAILABLE'); + expect(err.context.retryable).toBe(true); + expect(err.context.originalMessage).toBe( + 'RPC session was shut down by disposing the main stub' + ); + // The raw capnweb string must not be the user-facing message. + expect((err as Error).message).not.toContain('disposing the main stub'); + }); + + it('does not synthesize CONTAINER_UNAVAILABLE for a never-established non-teardown transport error', async () => { + // An invalid-frame / protocol error is not a teardown-family kind, so it + // should still surface as the transport error rather than being + // reclassified as container-unavailable. + const translateRPCError = await loadFn(); + const { RPCTransportError } = await loadErr(); + let thrown: unknown; + try { + translateRPCError( + new TypeError('Received non-string message from WebSocket.'), + { operation: 'utils.createSession', sessionEstablished: false } + ); + } catch (e) { + thrown = e; + } expect(thrown).toBeInstanceOf(RPCTransportError); expect((thrown as InstanceType).kind).toBe( - 'session_disposed' + 'invalid_frame' ); }); From 0b5a930a246bf31e2e58e73ebe42d7683794b3b1 Mon Sep 17 00:00:00 2001 From: Aron <263346377+aron-cf@users.noreply.github.com> Date: Wed, 1 Jul 2026 17:19:42 +0100 Subject: [PATCH 12/24] Make destroy() a no-op when the container was never admitted Destroying a sandbox whose container never started (e.g. no instance available under capacity pressure) threw the platform no-instance error from the base container.destroy(), producing a confusing second failure on the cleanup path. Treat that specific error as an idempotent no-op success. Also give the never-connected teardown fallback an accurate reason (container_unreachable) and drop the misleading "may be starting" wording. --- .changeset/surface-container-unavailable.md | 2 +- .../sandbox/src/container-control/client.ts | 4 ++-- packages/sandbox/src/sandbox.ts | 15 +++++++++++- .../sandbox/tests/rpc-sandbox-client.test.ts | 5 +++- .../tests/sandbox-destroy-lifetime.test.ts | 24 +++++++++++++++++++ packages/shared/src/errors/contexts.ts | 10 +++++++- 6 files changed, 54 insertions(+), 6 deletions(-) diff --git a/.changeset/surface-container-unavailable.md b/.changeset/surface-container-unavailable.md index 0bc4d45bf..a650de4ab 100644 --- a/.changeset/surface-container-unavailable.md +++ b/.changeset/surface-container-unavailable.md @@ -2,4 +2,4 @@ '@cloudflare/sandbox': patch --- -Surface container capacity failures as retryable `ContainerUnavailableError` (with a `reason`) instead of masking them as `utils.createSession` interruptions or raw transport errors. +Surface container capacity failures as retryable `ContainerUnavailableError` (with a `reason`) instead of masking them as `utils.createSession` interruptions or raw transport errors, and make `destroy()` an idempotent no-op when the container was never admitted. diff --git a/packages/sandbox/src/container-control/client.ts b/packages/sandbox/src/container-control/client.ts index 4e36b9b99..0e6407c98 100644 --- a/packages/sandbox/src/container-control/client.ts +++ b/packages/sandbox/src/container-control/client.ts @@ -479,14 +479,14 @@ function buildNeverConnectedUnavailableResponse( return null; } const ctx: ContainerUnavailableContext = { - reason: 'container_replaced', + reason: 'container_unreachable', retryable: true, originalMessage: transportResponse.context.originalMessage }; return { code: ErrorCode.CONTAINER_UNAVAILABLE, message: - 'The sandbox container could not be reached before the connection was torn down; it may be starting or temporarily unavailable.', + 'The sandbox container was unavailable: the connection was torn down before it became reachable. Retry the operation.', context: ctx, httpStatus: getHttpStatus(ErrorCode.CONTAINER_UNAVAILABLE), suggestion: getSuggestion( diff --git a/packages/sandbox/src/sandbox.ts b/packages/sandbox/src/sandbox.ts index ce5ff4146..534d1a3fa 100644 --- a/packages/sandbox/src/sandbox.ts +++ b/packages/sandbox/src/sandbox.ts @@ -2827,7 +2827,20 @@ export class Sandbox extends Container implements ISandbox { this.client.disconnect(); outcome = 'success'; - await super.destroy(); + try { + await super.destroy(); + } catch (error) { + // `super.destroy()` is `this.container.destroy()`, which throws the + // platform "no container instance" error when no instance was ever + // admitted (e.g. destroying a sandbox whose container never started + // under capacity pressure). There is nothing to tear down in that + // case, so treat it as an idempotent no-op success rather than + // surfacing a second error on the cleanup path. + if (!this.isNoInstanceError(error)) throw error; + this.logger.debug( + 'super.destroy() reported no container instance; treating as no-op' + ); + } } catch (error) { caughtError = error instanceof Error ? error : new Error(String(error)); throw error; diff --git a/packages/sandbox/tests/rpc-sandbox-client.test.ts b/packages/sandbox/tests/rpc-sandbox-client.test.ts index f1eb015ae..d8ddc79dc 100644 --- a/packages/sandbox/tests/rpc-sandbox-client.test.ts +++ b/packages/sandbox/tests/rpc-sandbox-client.test.ts @@ -822,12 +822,15 @@ describe('translateRPCError', () => { expect(thrown).toBeInstanceOf(ContainerUnavailableError); const err = thrown as InstanceType; expect(err.code).toBe('CONTAINER_UNAVAILABLE'); + expect(err.reason).toBe('container_unreachable'); expect(err.context.retryable).toBe(true); expect(err.context.originalMessage).toBe( 'RPC session was shut down by disposing the main stub' ); - // The raw capnweb string must not be the user-facing message. + // The message states unavailability plainly — no raw capnweb string, and + // no misleading "may be starting" hedge (the container was never reached). expect((err as Error).message).not.toContain('disposing the main stub'); + expect((err as Error).message).not.toContain('starting'); }); it('does not synthesize CONTAINER_UNAVAILABLE for a never-established non-teardown transport error', async () => { diff --git a/packages/sandbox/tests/sandbox-destroy-lifetime.test.ts b/packages/sandbox/tests/sandbox-destroy-lifetime.test.ts index c27870cc1..b03ef4f11 100644 --- a/packages/sandbox/tests/sandbox-destroy-lifetime.test.ts +++ b/packages/sandbox/tests/sandbox-destroy-lifetime.test.ts @@ -161,4 +161,28 @@ describe('Sandbox destroy() sandbox lifetime', () => { const lifetimePut = putCalls.find((c) => c.key === 'sandbox:lifetime'); expect(lifetimePut).toBeUndefined(); }); + + it('resolves (idempotent no-op) when the container was never admitted', async () => { + // Destroying a sandbox whose container never started (e.g. no instance + // available under capacity pressure) must not throw: there is nothing to + // tear down. The base container.destroy() throws the platform no-instance + // error; Sandbox.destroy() should treat it as success. + vi.spyOn(Container.prototype, 'destroy').mockRejectedValue( + new Error( + 'There is no container instance that can be provided to this Durable Object, try again later' + ) + ); + + await expect(sandbox.destroy()).resolves.toBeUndefined(); + }); + + it('still rejects when the container destroy fails for a non-no-instance reason', async () => { + vi.spyOn(Container.prototype, 'destroy').mockRejectedValue( + new Error('some other teardown failure') + ); + + await expect(sandbox.destroy()).rejects.toThrow( + 'some other teardown failure' + ); + }); }); diff --git a/packages/shared/src/errors/contexts.ts b/packages/shared/src/errors/contexts.ts index 17df66e4f..f803b607c 100644 --- a/packages/shared/src/errors/contexts.ts +++ b/packages/shared/src/errors/contexts.ts @@ -266,7 +266,15 @@ export type ContainerUnavailableReason = * ("Maximum number of running container instances exceeded. Try again * later, or try configuring a higher value for max_instances"). */ - | 'max_container_instances_exceeded'; + | 'max_container_instances_exceeded' + /** + * The RPC connection was torn down before the container ever became + * reachable, and the underlying platform cause was not captured (e.g. the + * Durable Object was evicted mid-startup under capacity pressure). The + * container was never admitted — this is unavailability, not a cold start + * in progress. + */ + | 'container_unreachable'; /** * Container availability error context. Surfaced when the sandbox container From 24a591b8a6a3a821f9aca0d8f816fcbe7c41718c Mon Sep 17 00:00:00 2001 From: Aron <263346377+aron-cf@users.noreply.github.com> Date: Wed, 1 Jul 2026 17:47:12 +0100 Subject: [PATCH 13/24] Guard connect teardown, stamp causes, and trace the RPC path Prevent a lifecycle disconnect (e.g. the alarm firing onStop) from tearing down a connection while it is still attempting to connect: disconnect() now defers teardown until the in-flight attempt settles and stamps the cause up front. Stamp the first container-admission failure onto the transport as soon as startContainer throws, and thread a typed cause through disconnect()/destroyConnection() so queued RPC calls reject with the real reason (sandbox stopping / lifetime change / transport switch) instead of the generic capnweb disposal string. Add spans for the RPC call (rpc.call), the connect/disconnect lifecycle (rpc.connect / rpc.disconnect), and each upgrade attempt (rpc.connect.attempt), stamping error and error.stack attributes per the Cloudflare trace-UI convention. --- .../sandbox/src/container-control/client.ts | 45 +++++++-- .../src/container-control/connection.ts | 97 ++++++++++++++----- .../sandbox/src/container-control/tracing.ts | 87 +++++++++++++++++ packages/sandbox/src/sandbox.ts | 67 ++++++++++++- .../tests/container-connection.test.ts | 86 ++++++++++++++++ .../sandbox/tests/rpc-sandbox-client.test.ts | 4 + 6 files changed, 350 insertions(+), 36 deletions(-) create mode 100644 packages/sandbox/src/container-control/tracing.ts diff --git a/packages/sandbox/src/container-control/client.ts b/packages/sandbox/src/container-control/client.ts index 0e6407c98..b88cea428 100644 --- a/packages/sandbox/src/container-control/client.ts +++ b/packages/sandbox/src/container-control/client.ts @@ -101,6 +101,7 @@ import { ContainerControlConnection, type ContainerControlConnectionOptions } from './connection'; +import { withSpan } from './tracing'; // --------------------------------------------------------------------------- // Constants @@ -581,15 +582,17 @@ function wrapStub( result != null && typeof (result as { then?: unknown }).then === 'function' ) { - return (result as Promise) - .catch((err: unknown) => + // Span the RPC call so each method invocation (and its failure, + // with error/error.stack attributes) is visible in traces. + return withSpan('rpc.call', { operation }, () => + (result as Promise).catch((err: unknown) => translateRPCError(err, { operation, connectionError: getConnectionError(), sessionEstablished: getSessionEstablished() }) ) - .finally(onCallSettled); + ).finally(onCallSettled); } onCallSettled(); return result; @@ -888,7 +891,7 @@ export class ContainerControlClient { } } - private destroyConnection(): void { + private destroyConnection(cause?: unknown): void { this.stopBusyPoll(); this.clearIdleTimer(); this.activeCalls = 0; @@ -899,7 +902,10 @@ export class ContainerControlClient { this.onSessionIdle?.(); } if (this.conn) { - this.conn.disconnect(); + // Pass the cause so queued RPC calls reject with the real reason + // (stamped into lastConnectionError via onConnectionError) instead of + // the generic capnweb disposal message. + this.conn.disconnect(cause); this.conn = null; } } @@ -1036,8 +1042,33 @@ export class ContainerControlClient { await this.getConnection().connect(); } - disconnect(): void { - this.destroyConnection(); + /** + * Tear down the active connection. + * + * When a connection attempt is still in progress, the teardown is deferred + * until that attempt settles — so a lifecycle disconnect (e.g. the DO's + * alarm firing `onStop`) cannot rip the transport out from under an + * in-flight connect and reject queued calls with a generic disposal error. + * The provided `cause` is stamped immediately so that if the attempt fails, + * queued calls surface it; if the attempt succeeds, the (now-established) + * connection is then torn down cleanly with the cause. + * + * `cause` should be a typed `SandboxError` describing why the connection is + * being torn down (sandbox stopping, lifetime change, transport switch). + */ + disconnect(cause?: unknown): void { + const conn = this.conn; + if (conn?.isConnecting()) { + // Stamp the cause now so an in-flight-attempt failure surfaces it. + if (cause !== undefined) this.lastConnectionError = cause; + // Defer the actual teardown until the attempt settles. Guard with an + // identity check so we don't destroy a newer connection. + void conn.whenSettled().then(() => { + if (this.conn === conn) this.destroyConnection(cause); + }); + return; + } + this.destroyConnection(cause); } } diff --git a/packages/sandbox/src/container-control/connection.ts b/packages/sandbox/src/container-control/connection.ts index 8ca9516c0..fec26e94b 100644 --- a/packages/sandbox/src/container-control/connection.ts +++ b/packages/sandbox/src/container-control/connection.ts @@ -28,6 +28,7 @@ import { fetchWithResponseRetry, isRetryableWebSocketUpgradeResponse } from '../response-retry'; +import { traceEvent, withSpan } from './tracing'; // --------------------------------------------------------------------------- // Helpers @@ -372,6 +373,26 @@ export class ContainerControlConnection { return this.connected; } + /** + * True while a connection attempt is in progress (upgrade not yet + * established and not yet failed). Owners use this to avoid tearing the + * connection down mid-attempt. + */ + isConnecting(): boolean { + return this.connectPromise !== null; + } + + /** + * Resolve once any in-flight connection attempt settles (success or + * failure). Resolves immediately when no attempt is in progress. Never + * rejects — callers only care that the attempt is done. + */ + async whenSettled(): Promise { + const pending = this.connectPromise; + if (!pending) return; + await pending.catch(() => {}); + } + async connect(): Promise { if (this.connected) return; @@ -387,7 +408,22 @@ export class ContainerControlConnection { } } - disconnect(): void { + /** + * Tear down the connection. When `cause` is provided (e.g. a lifecycle + * teardown from the owning DO), it is handed to `onConnectionError` first + * so any RPC calls queued on the transport reject with that real cause + * rather than the generic capnweb "disposing the main stub" message. + */ + disconnect(cause?: unknown): void { + if (cause !== undefined) { + this.fireConnectionError(cause); + traceEvent('rpc.disconnect', { + port: this.port, + reason: cause instanceof Error ? cause.message : String(cause) + }); + } else { + traceEvent('rpc.disconnect', { port: this.port }); + } try { (this.stub as unknown as Disposable)[Symbol.dispose]?.(); } catch { @@ -544,6 +580,7 @@ export class ContainerControlConnection { ); } + traceEvent('rpc.connect', { port: this.port, outcome: 'established' }); this.logger.debug('ContainerControlConnection established', { port: this.port }); @@ -616,31 +653,43 @@ export class ContainerControlConnection { * each retry has an independent connect timeout for the *upgrade only*. */ private async fetchUpgradeAttempt(): Promise { - // Explicitly start the container first (when a hook is provided), under - // its own budget — no per-attempt abort signal. - if (this.startContainer) { - await this.startContainer(); - } + return withSpan('rpc.connect.attempt', { port: this.port }, async () => { + // Explicitly start the container first (when a hook is provided), + // under its own budget — no per-attempt abort signal. + if (this.startContainer) { + try { + await this.startContainer(); + } catch (error) { + // Stamp the real cause on the owner as soon as the first + // container-admission failure surfaces — before any retry/abort + // can mask it — so a teardown mid-retry still surfaces this. + this.fireConnectionError( + tryConvertPlatformUnavailable(error) ?? error + ); + throw error; + } + } - const controller = new AbortController(); - const timeout = setTimeout( - () => controller.abort(), - DEFAULT_CONNECT_TIMEOUT_MS - ); + const controller = new AbortController(); + const timeout = setTimeout( + () => controller.abort(), + DEFAULT_CONNECT_TIMEOUT_MS + ); - try { - const url = `http://localhost:${this.port}/rpc`; - const request = new Request(url, { - headers: { - Upgrade: 'websocket', - Connection: 'Upgrade' - }, - signal: controller.signal - }); - return await this.containerStub.fetch(request); - } finally { - clearTimeout(timeout); - } + try { + const url = `http://localhost:${this.port}/rpc`; + const request = new Request(url, { + headers: { + Upgrade: 'websocket', + Connection: 'Upgrade' + }, + signal: controller.signal + }); + return await this.containerStub.fetch(request); + } finally { + clearTimeout(timeout); + } + }); } } diff --git a/packages/sandbox/src/container-control/tracing.ts b/packages/sandbox/src/container-control/tracing.ts new file mode 100644 index 000000000..83b9d2d20 --- /dev/null +++ b/packages/sandbox/src/container-control/tracing.ts @@ -0,0 +1,87 @@ +/** + * Thin tracing helper for the RPC control path. + * + * Wraps `cloudflare:workers` `tracing.enterSpan` so the connection/client code + * can emit spans for RPC calls, the connect/disconnect lifecycle, and + * individual upgrade attempts — without hard-failing in environments where the + * tracing API is unavailable. + * + * Error convention: the Cloudflare trace UI surfaces `error` and `error.stack` + * span attributes specially (this differs from the OpenTelemetry standard, + * which uses exception events). We therefore stamp both attributes on the span + * when the wrapped work throws, so failures are visible in the GUI. + */ + +import { tracing } from 'cloudflare:workers'; + +/** Minimal shape of the span object passed to `enterSpan` callbacks. */ +export interface TraceSpan { + setAttribute(key: string, value?: boolean | number | string): void; +} + +type SpanAttributes = Record; + +/** + * Stamp the Cloudflare-convention error attributes onto a span. Safe to call + * with any thrown value. + */ +function setErrorAttributes(span: TraceSpan, error: unknown): void { + if (error instanceof Error) { + span.setAttribute('error', error.message); + if (typeof error.stack === 'string') { + span.setAttribute('error.stack', error.stack); + } + // A code (e.g. SandboxError.code) is high-signal for filtering. + const code = (error as { code?: unknown }).code; + if (typeof code === 'string') span.setAttribute('error.code', code); + return; + } + span.setAttribute('error', String(error)); +} + +/** + * Run `fn` inside a span named `name`, applying `attributes` up front and + * stamping `error`/`error.stack` if it throws. Re-throws the original error. + * + * Falls back to running `fn` directly if the tracing API is unavailable, so a + * missing tracer never changes behavior. + */ +export async function withSpan( + name: string, + attributes: SpanAttributes, + fn: (span: TraceSpan) => Promise +): Promise { + const enter = tracing?.enterSpan?.bind(tracing); + if (typeof enter !== 'function') { + return fn({ setAttribute: () => {} }); + } + return enter(name, async (span: TraceSpan) => { + for (const [key, value] of Object.entries(attributes)) { + if (value !== undefined) span.setAttribute(key, value); + } + try { + return await fn(span); + } catch (error) { + setErrorAttributes(span, error); + throw error; + } + }); +} + +/** + * Emit a zero-duration marker span for a lifecycle event (e.g. disconnect). + * Best-effort: never throws, never changes behavior. + */ +export function traceEvent(name: string, attributes: SpanAttributes): void { + const enter = tracing?.enterSpan?.bind(tracing); + if (typeof enter !== 'function') return; + try { + enter(name, (span: TraceSpan) => { + for (const [key, value] of Object.entries(attributes)) { + if (value !== undefined) span.setAttribute(key, value); + } + }); + } catch { + // Tracing must never break the control path. + } +} diff --git a/packages/sandbox/src/sandbox.ts b/packages/sandbox/src/sandbox.ts index 534d1a3fa..8af235c72 100644 --- a/packages/sandbox/src/sandbox.ts +++ b/packages/sandbox/src/sandbox.ts @@ -1399,7 +1399,12 @@ export class Sandbox extends Container implements ISandbox { this.tunnelsHandler = null; this.tunnelExitHandler = null; this.destroyAllTunnels = null; - previousClient.disconnect(); + previousClient.disconnect( + this.buildDisconnectCause( + 'runtime_replaced', + 'The sandbox transport was switched while the operation was pending.' + ) + ); } if (storedTransport) { this.hasStoredTransport = true; @@ -1604,7 +1609,12 @@ export class Sandbox extends Container implements ISandbox { this.tunnelsHandler = null; this.tunnelExitHandler = null; this.destroyAllTunnels = null; - previousClient.disconnect(); + previousClient.disconnect( + this.buildDisconnectCause( + 'runtime_replaced', + 'The sandbox transport was switched while the operation was pending.' + ) + ); this.renewActivityTimeout(); this.logger.debug('Transport updated', { transport }); } @@ -2823,8 +2833,16 @@ export class Sandbox extends Container implements ISandbox { await this.ctx.storage.delete('tunnels'); await this.ctx.storage.delete('tunnels:meta'); - // Disconnect transport after all cleanup commands have completed - this.client.disconnect(); + // Disconnect transport after all cleanup commands have completed. + // Stamp a lifetime-changed cause so any RPC still queued on the + // transport rejects with an actionable (non-retryable) error rather + // than a generic capnweb disposal string. + this.client.disconnect( + this.buildDisconnectCause( + 'sandbox_lifetime_changed', + 'The sandbox was destroyed while the operation was pending.' + ) + ); outcome = 'success'; try { @@ -3026,7 +3044,14 @@ export class Sandbox extends Container implements ISandbox { } // Disconnect the active client so open sockets do not hold the DO alive. - this.client.disconnect(); + // Stamp a runtime-replaced cause so a pending RPC surfaces a retryable + // interruption instead of a generic disposal string. + this.client.disconnect( + this.buildDisconnectCause( + 'runtime_replaced', + 'The sandbox container stopped while the operation was pending.' + ) + ); // Stop local sync managers before clearing the map. let hadR2EgressMount = false; @@ -3357,6 +3382,38 @@ export class Sandbox extends Container implements ISandbox { ); } + /** + * Build a typed lifecycle cause to stamp on the RPC transport when the DO + * tears the connection down for its own reasons (container stopped, sandbox + * destroyed, transport switched). Handed to `client.disconnect(cause)` so + * any RPC calls queued on the transport reject with this actionable reason + * instead of the generic capnweb "disposing the main stub" message. + */ + private buildDisconnectCause( + reason: 'runtime_replaced' | 'sandbox_lifetime_changed', + detail: string + ): OperationInterruptedError { + const retryable = reason === 'runtime_replaced'; + const context = { + reason, + operation: 'rpc.connect', + phase: 'connection', + admitted: 'unknown' as const, + retryable + }; + return new OperationInterruptedError({ + code: ErrorCode.OPERATION_INTERRUPTED, + message: detail, + context, + httpStatus: getHttpStatus(ErrorCode.OPERATION_INTERRUPTED), + suggestion: getSuggestion( + ErrorCode.OPERATION_INTERRUPTED, + context as unknown as Record + ), + timestamp: new Date().toISOString() + }); + } + /** * Helper: Check if error is a transient startup error that should trigger retry * diff --git a/packages/sandbox/tests/container-connection.test.ts b/packages/sandbox/tests/container-connection.test.ts index babd880d9..9f214fb7b 100644 --- a/packages/sandbox/tests/container-connection.test.ts +++ b/packages/sandbox/tests/container-connection.test.ts @@ -45,6 +45,55 @@ describe('ContainerControlConnection', () => { conn.disconnect(); expect(conn.isConnected()).toBe(false); }); + + it('fires onConnectionError with the provided cause before disposing', () => { + const onConnectionError = vi.fn(); + const conn = new ContainerControlConnection({ + stub: { fetch: vi.fn() }, + onConnectionError + }); + const cause = new Error('sandbox stopped'); + conn.disconnect(cause); + expect(onConnectionError).toHaveBeenCalledWith(cause); + }); + + it('does not fire onConnectionError when disconnected without a cause', () => { + const onConnectionError = vi.fn(); + const conn = new ContainerControlConnection({ + stub: { fetch: vi.fn() }, + onConnectionError + }); + conn.disconnect(); + expect(onConnectionError).not.toHaveBeenCalled(); + }); + }); + + describe('connection-attempt state', () => { + it('reports isConnecting() while a connect is in flight and clears after it settles', async () => { + let resolveFetch: (r: Response) => void = () => {}; + const fetchGate = new Promise((res) => { + resolveFetch = res; + }); + const conn = new ContainerControlConnection({ + stub: { fetch: vi.fn().mockReturnValue(fetchGate) }, + retryTimeoutMs: 0 + }); + + expect(conn.isConnecting()).toBe(false); + const connectPromise = conn.connect().catch(() => {}); + expect(conn.isConnecting()).toBe(true); + + // Fail the upgrade so the attempt settles. + resolveFetch(new Response('nope', { status: 404 })); + await conn.whenSettled(); + await connectPromise; + expect(conn.isConnecting()).toBe(false); + }); + + it('whenSettled() resolves immediately when no attempt is in flight', async () => { + const conn = new ContainerControlConnection({ stub: { fetch: vi.fn() } }); + await expect(conn.whenSettled()).resolves.toBeUndefined(); + }); }); describe('connect', () => { @@ -341,6 +390,43 @@ describe('ContainerControlConnection', () => { expect(fetchMock).not.toHaveBeenCalled(); }); + it('stamps onConnectionError as soon as startContainer first throws (before the retry budget is exhausted)', async () => { + vi.useFakeTimers(); + try { + const platformMessage = + 'There is no container instance that can be provided to this Durable Object, try again later'; + const onConnectionError = vi.fn(); + const startContainer = vi + .fn<() => Promise>() + .mockRejectedValue(new Error(platformMessage)); + const conn = new ContainerControlConnection({ + stub: { fetch: vi.fn() }, + startContainer, + onConnectionError, + retryTimeoutMs: 60_000 + }); + + const connectPromise = conn.connect().catch(() => {}); + // Let the first attempt run and throw. + await vi.advanceTimersByTimeAsync(0); + expect(startContainer).toHaveBeenCalledTimes(1); + // The cause is stamped immediately, well before the retry budget or + // any teardown — as a typed ContainerUnavailableError. + expect(onConnectionError).toHaveBeenCalled(); + const stamped = onConnectionError.mock.calls[0][0]; + expect((stamped as { code?: string }).code).toBe( + 'CONTAINER_UNAVAILABLE' + ); + + // Clean up the pending retry loop. + conn.disconnect(); + await vi.advanceTimersByTimeAsync(60_000); + await connectPromise; + } finally { + vi.useRealTimers(); + } + }); + it('retries a thrown startContainer platform error, then upgrades on success', async () => { vi.useFakeTimers(); try { diff --git a/packages/sandbox/tests/rpc-sandbox-client.test.ts b/packages/sandbox/tests/rpc-sandbox-client.test.ts index d8ddc79dc..6516cdff1 100644 --- a/packages/sandbox/tests/rpc-sandbox-client.test.ts +++ b/packages/sandbox/tests/rpc-sandbox-client.test.ts @@ -49,6 +49,10 @@ vi.mock('../src/container-control/connection', () => ({ isConnected() { return connected; } + isConnecting() { + return false; + } + async whenSettled() {} getStats() { return stats; } From 914f051f2ebc07682da1c6272200c47b06ea6920 Mon Sep 17 00:00:00 2001 From: Aron <263346377+aron-cf@users.noreply.github.com> Date: Wed, 1 Jul 2026 17:54:59 +0100 Subject: [PATCH 14/24] Namespace RPC spans under sandbox.rpc.* with sandbox identifiers Rename the RPC trace spans to sandbox.rpc.call / sandbox.rpc.connect / sandbox.rpc.connect.attempt / sandbox.rpc.disconnect, and stamp sandbox.id (the DO ctx.id), sandbox.name (the user-provided name), and sandbox.rpc.port on every span so RPC traces are attributable to a specific sandbox. --- .../sandbox/src/container-control/client.ts | 68 ++++++++---- .../src/container-control/connection.ts | 102 +++++++++++------- packages/sandbox/src/sandbox.ts | 4 + 3 files changed, 118 insertions(+), 56 deletions(-) diff --git a/packages/sandbox/src/container-control/client.ts b/packages/sandbox/src/container-control/client.ts index b88cea428..171a2d219 100644 --- a/packages/sandbox/src/container-control/client.ts +++ b/packages/sandbox/src/container-control/client.ts @@ -558,7 +558,8 @@ function wrapStub( onCallStarted: () => void, onCallSettled: () => void, getConnectionError: () => unknown, - getSessionEstablished: () => boolean + getSessionEstablished: () => boolean, + getSpanAttrs: () => Record ): T { return new Proxy(stub, { get(target, prop, receiver) { @@ -584,14 +585,17 @@ function wrapStub( ) { // Span the RPC call so each method invocation (and its failure, // with error/error.stack attributes) is visible in traces. - return withSpan('rpc.call', { operation }, () => - (result as Promise).catch((err: unknown) => - translateRPCError(err, { - operation, - connectionError: getConnectionError(), - sessionEstablished: getSessionEstablished() - }) - ) + return withSpan( + 'sandbox.rpc.call', + { ...getSpanAttrs(), operation }, + () => + (result as Promise).catch((err: unknown) => + translateRPCError(err, { + operation, + connectionError: getConnectionError(), + sessionEstablished: getSessionEstablished() + }) + ) ).finally(onCallSettled); } onCallSettled(); @@ -693,6 +697,8 @@ export class ContainerControlClient { localMain: options.localMain, logger: options.logger, retryTimeoutMs: options.retryTimeoutMs, + sandboxId: options.sandboxId, + sandboxName: options.sandboxName, // Explicit container-start hook: when provided, the connection starts // the container in its own retry loop before the WebSocket upgrade so // capacity failures throw where we can classify them directly. @@ -810,6 +816,20 @@ export class ContainerControlClient { /** Whether the current connection ever established a live session. */ private getSessionEstablished = (): boolean => this.sessionEstablished; + /** + * Base span attributes for RPC-call spans: sandbox identifiers plus the + * container port. Mirrors the connection's `spanAttrs()` so all + * `sandbox.rpc.*` spans share a consistent shape. + */ + private getSpanAttrs = (): Record => ({ + 'sandbox.id': this.connOptions.sandboxId, + 'sandbox.name': this.connOptions.sandboxName, + 'sandbox.rpc.port': + this.connOptions.port !== undefined + ? String(this.connOptions.port) + : undefined + }); + /** * Sample `getStats()` and update busy/idle state. While busy, renews the * activity timeout each tick so an in-flight stream keeps pushing the @@ -926,7 +946,8 @@ export class ContainerControlClient { this.recordCallStarted, this.recordCallSettled, this.getLastConnectionError, - this.getSessionEstablished + this.getSessionEstablished, + this.getSpanAttrs ); } get files(): SandboxFilesAPI { @@ -936,7 +957,8 @@ export class ContainerControlClient { this.recordCallStarted, this.recordCallSettled, this.getLastConnectionError, - this.getSessionEstablished + this.getSessionEstablished, + this.getSpanAttrs ) as unknown as SandboxFilesAPI; } get processes(): SandboxProcessesAPI { @@ -946,7 +968,8 @@ export class ContainerControlClient { this.recordCallStarted, this.recordCallSettled, this.getLastConnectionError, - this.getSessionEstablished + this.getSessionEstablished, + this.getSpanAttrs ); } get ports(): SandboxPortsAPI { @@ -956,7 +979,8 @@ export class ContainerControlClient { this.recordCallStarted, this.recordCallSettled, this.getLastConnectionError, - this.getSessionEstablished + this.getSessionEstablished, + this.getSpanAttrs ); } get git(): SandboxGitAPI { @@ -966,7 +990,8 @@ export class ContainerControlClient { this.recordCallStarted, this.recordCallSettled, this.getLastConnectionError, - this.getSessionEstablished + this.getSessionEstablished, + this.getSpanAttrs ); } get utils(): SandboxUtilsAPI { @@ -976,7 +1001,8 @@ export class ContainerControlClient { this.recordCallStarted, this.recordCallSettled, this.getLastConnectionError, - this.getSessionEstablished + this.getSessionEstablished, + this.getSpanAttrs ); } get backup(): SandboxBackupAPI { @@ -986,7 +1012,8 @@ export class ContainerControlClient { this.recordCallStarted, this.recordCallSettled, this.getLastConnectionError, - this.getSessionEstablished + this.getSessionEstablished, + this.getSpanAttrs ); } get watch(): SandboxWatchAPI { @@ -996,7 +1023,8 @@ export class ContainerControlClient { this.recordCallStarted, this.recordCallSettled, this.getLastConnectionError, - this.getSessionEstablished + this.getSessionEstablished, + this.getSpanAttrs ); } get tunnels(): SandboxTunnelsAPI { @@ -1006,7 +1034,8 @@ export class ContainerControlClient { this.recordCallStarted, this.recordCallSettled, this.getLastConnectionError, - this.getSessionEstablished + this.getSessionEstablished, + this.getSpanAttrs ); } get interpreter(): SandboxInterpreterAPI { @@ -1016,7 +1045,8 @@ export class ContainerControlClient { this.recordCallStarted, this.recordCallSettled, this.getLastConnectionError, - this.getSessionEstablished + this.getSessionEstablished, + this.getSpanAttrs ); } diff --git a/packages/sandbox/src/container-control/connection.ts b/packages/sandbox/src/container-control/connection.ts index fec26e94b..3b7c98358 100644 --- a/packages/sandbox/src/container-control/connection.ts +++ b/packages/sandbox/src/container-control/connection.ts @@ -242,6 +242,10 @@ export interface ContainerControlConnectionOptions { stub: ContainerFetchStub; port?: number; logger?: Logger; + /** Durable Object id (`ctx.id`), stamped on spans as `sandbox.id`. */ + sandboxId?: string; + /** User-provided sandbox name, stamped on spans as `sandbox.name`. */ + sandboxName?: string; /** * Optional hook to explicitly start the container (and wait for its ports) * before issuing the WebSocket upgrade. @@ -327,6 +331,8 @@ export class ContainerControlConnection { private readonly onConnectionError: ((error: unknown) => void) | undefined; private readonly onConnected: (() => void) | undefined; private readonly startContainer: (() => Promise) | undefined; + private readonly sandboxId: string | undefined; + private readonly sandboxName: string | undefined; constructor(options: ContainerControlConnectionOptions) { this.containerStub = options.stub; @@ -337,6 +343,8 @@ export class ContainerControlConnection { this.onConnectionError = options.onConnectionError; this.onConnected = options.onConnected; this.startContainer = options.startContainer; + this.sandboxId = options.sandboxId; + this.sandboxName = options.sandboxName; this.transport = new DeferredTransport(); this.session = new RpcSession( @@ -417,12 +425,12 @@ export class ContainerControlConnection { disconnect(cause?: unknown): void { if (cause !== undefined) { this.fireConnectionError(cause); - traceEvent('rpc.disconnect', { - port: this.port, + traceEvent('sandbox.rpc.disconnect', { + ...this.spanAttrs(), reason: cause instanceof Error ? cause.message : String(cause) }); } else { - traceEvent('rpc.disconnect', { port: this.port }); + traceEvent('sandbox.rpc.disconnect', this.spanAttrs()); } try { (this.stub as unknown as Disposable)[Symbol.dispose]?.(); @@ -461,6 +469,19 @@ export class ContainerControlConnection { // Internal // ----------------------------------------------------------------------- + /** + * Base span attributes for this connection: the container port plus the + * sandbox identifiers (`sandbox.id` = DO id, `sandbox.name` = user name) + * when available. + */ + private spanAttrs(): Record { + return { + 'sandbox.rpc.port': this.port, + 'sandbox.id': this.sandboxId, + 'sandbox.name': this.sandboxName + }; + } + /** * Run the owner-provided `onClose` callback exactly once per call, * swallowing any errors so a buggy listener can't keep the connection @@ -580,7 +601,10 @@ export class ContainerControlConnection { ); } - traceEvent('rpc.connect', { port: this.port, outcome: 'established' }); + traceEvent('sandbox.rpc.connect', { + ...this.spanAttrs(), + outcome: 'established' + }); this.logger.debug('ContainerControlConnection established', { port: this.port }); @@ -653,43 +677,47 @@ export class ContainerControlConnection { * each retry has an independent connect timeout for the *upgrade only*. */ private async fetchUpgradeAttempt(): Promise { - return withSpan('rpc.connect.attempt', { port: this.port }, async () => { - // Explicitly start the container first (when a hook is provided), - // under its own budget — no per-attempt abort signal. - if (this.startContainer) { - try { - await this.startContainer(); - } catch (error) { - // Stamp the real cause on the owner as soon as the first - // container-admission failure surfaces — before any retry/abort - // can mask it — so a teardown mid-retry still surfaces this. - this.fireConnectionError( - tryConvertPlatformUnavailable(error) ?? error - ); - throw error; + return withSpan( + 'sandbox.rpc.connect.attempt', + this.spanAttrs(), + async () => { + // Explicitly start the container first (when a hook is provided), + // under its own budget — no per-attempt abort signal. + if (this.startContainer) { + try { + await this.startContainer(); + } catch (error) { + // Stamp the real cause on the owner as soon as the first + // container-admission failure surfaces — before any retry/abort + // can mask it — so a teardown mid-retry still surfaces this. + this.fireConnectionError( + tryConvertPlatformUnavailable(error) ?? error + ); + throw error; + } } - } - const controller = new AbortController(); - const timeout = setTimeout( - () => controller.abort(), - DEFAULT_CONNECT_TIMEOUT_MS - ); + const controller = new AbortController(); + const timeout = setTimeout( + () => controller.abort(), + DEFAULT_CONNECT_TIMEOUT_MS + ); - try { - const url = `http://localhost:${this.port}/rpc`; - const request = new Request(url, { - headers: { - Upgrade: 'websocket', - Connection: 'Upgrade' - }, - signal: controller.signal - }); - return await this.containerStub.fetch(request); - } finally { - clearTimeout(timeout); + try { + const url = `http://localhost:${this.port}/rpc`; + const request = new Request(url, { + headers: { + Upgrade: 'websocket', + Connection: 'Upgrade' + }, + signal: controller.signal + }); + return await this.containerStub.fetch(request); + } finally { + clearTimeout(timeout); + } } - }); + ); } } diff --git a/packages/sandbox/src/sandbox.ts b/packages/sandbox/src/sandbox.ts index 8af235c72..edcbc61c6 100644 --- a/packages/sandbox/src/sandbox.ts +++ b/packages/sandbox/src/sandbox.ts @@ -1161,6 +1161,10 @@ export class Sandbox extends Container implements ISandbox { port: 3000, logger: this.logger, retryTimeoutMs: this.computeRetryTimeoutMs(), + // Sandbox identifiers stamped on RPC trace spans (sandbox.id = DO id, + // sandbox.name = user-provided name). + sandboxId: this.ctx.id.toString(), + sandboxName: this.sandboxName ?? undefined, // Explicitly start the container before the RPC WebSocket upgrade. // Running start() here — rather than as a side effect of the upgrade // fetch through containerFetch() — makes the platform's From b06bd08b844d8aed1379c0f0086c6dcd27c50197 Mon Sep 17 00:00:00 2001 From: Aron <263346377+aron-cf@users.noreply.github.com> Date: Wed, 1 Jul 2026 17:58:34 +0100 Subject: [PATCH 15/24] Include operation in the sandbox.rpc.call span name --- packages/sandbox/src/container-control/client.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/sandbox/src/container-control/client.ts b/packages/sandbox/src/container-control/client.ts index 171a2d219..1f2ce13b2 100644 --- a/packages/sandbox/src/container-control/client.ts +++ b/packages/sandbox/src/container-control/client.ts @@ -586,7 +586,7 @@ function wrapStub( // Span the RPC call so each method invocation (and its failure, // with error/error.stack attributes) is visible in traces. return withSpan( - 'sandbox.rpc.call', + `sandbox.rpc.call ${operation}`, { ...getSpanAttrs(), operation }, () => (result as Promise).catch((err: unknown) => From 3dd2ca3e77a30ad69a29359d11712e74547c7d5a Mon Sep 17 00:00:00 2001 From: Aron <263346377+aron-cf@users.noreply.github.com> Date: Wed, 1 Jul 2026 18:02:21 +0100 Subject: [PATCH 16/24] Resolve sandbox trace identifiers lazily via a single callback Replace the static sandboxId/sandboxName options with a getSandboxInfo callback queried at span time, so a sandbox name set (or changed) after the RPC client is built is reflected in sandbox.id / sandbox.name span attributes. --- .../sandbox/src/container-control/client.ts | 22 ++++++------ .../src/container-control/connection.ts | 24 +++++++------ packages/sandbox/src/sandbox.ts | 9 +++-- .../tests/container-connection.test.ts | 34 +++++++++++++++++++ 4 files changed, 66 insertions(+), 23 deletions(-) diff --git a/packages/sandbox/src/container-control/client.ts b/packages/sandbox/src/container-control/client.ts index 1f2ce13b2..b769c420c 100644 --- a/packages/sandbox/src/container-control/client.ts +++ b/packages/sandbox/src/container-control/client.ts @@ -697,8 +697,7 @@ export class ContainerControlClient { localMain: options.localMain, logger: options.logger, retryTimeoutMs: options.retryTimeoutMs, - sandboxId: options.sandboxId, - sandboxName: options.sandboxName, + getSandboxInfo: options.getSandboxInfo, // Explicit container-start hook: when provided, the connection starts // the container in its own retry loop before the WebSocket upgrade so // capacity failures throw where we can classify them directly. @@ -821,14 +820,17 @@ export class ContainerControlClient { * container port. Mirrors the connection's `spanAttrs()` so all * `sandbox.rpc.*` spans share a consistent shape. */ - private getSpanAttrs = (): Record => ({ - 'sandbox.id': this.connOptions.sandboxId, - 'sandbox.name': this.connOptions.sandboxName, - 'sandbox.rpc.port': - this.connOptions.port !== undefined - ? String(this.connOptions.port) - : undefined - }); + private getSpanAttrs = (): Record => { + const info = this.connOptions.getSandboxInfo?.(); + return { + 'sandbox.id': info?.id, + 'sandbox.name': info?.name, + 'sandbox.rpc.port': + this.connOptions.port !== undefined + ? String(this.connOptions.port) + : undefined + }; + }; /** * Sample `getStats()` and update busy/idle state. While busy, renews the diff --git a/packages/sandbox/src/container-control/connection.ts b/packages/sandbox/src/container-control/connection.ts index 3b7c98358..d789ca1db 100644 --- a/packages/sandbox/src/container-control/connection.ts +++ b/packages/sandbox/src/container-control/connection.ts @@ -242,10 +242,13 @@ export interface ContainerControlConnectionOptions { stub: ContainerFetchStub; port?: number; logger?: Logger; - /** Durable Object id (`ctx.id`), stamped on spans as `sandbox.id`. */ - sandboxId?: string; - /** User-provided sandbox name, stamped on spans as `sandbox.name`. */ - sandboxName?: string; + /** + * Lazily resolve sandbox identifiers for trace spans. Queried at span time + * (not construction) so a name that is set after the client is built — or + * changes across the connection's lifetime — is reflected. Stamped on spans + * as `sandbox.id` and `sandbox.name`. + */ + getSandboxInfo?: () => { id?: string; name?: string }; /** * Optional hook to explicitly start the container (and wait for its ports) * before issuing the WebSocket upgrade. @@ -331,8 +334,9 @@ export class ContainerControlConnection { private readonly onConnectionError: ((error: unknown) => void) | undefined; private readonly onConnected: (() => void) | undefined; private readonly startContainer: (() => Promise) | undefined; - private readonly sandboxId: string | undefined; - private readonly sandboxName: string | undefined; + private readonly getSandboxInfo: + | (() => { id?: string; name?: string }) + | undefined; constructor(options: ContainerControlConnectionOptions) { this.containerStub = options.stub; @@ -343,8 +347,7 @@ export class ContainerControlConnection { this.onConnectionError = options.onConnectionError; this.onConnected = options.onConnected; this.startContainer = options.startContainer; - this.sandboxId = options.sandboxId; - this.sandboxName = options.sandboxName; + this.getSandboxInfo = options.getSandboxInfo; this.transport = new DeferredTransport(); this.session = new RpcSession( @@ -475,10 +478,11 @@ export class ContainerControlConnection { * when available. */ private spanAttrs(): Record { + const info = this.getSandboxInfo?.(); return { 'sandbox.rpc.port': this.port, - 'sandbox.id': this.sandboxId, - 'sandbox.name': this.sandboxName + 'sandbox.id': info?.id, + 'sandbox.name': info?.name }; } diff --git a/packages/sandbox/src/sandbox.ts b/packages/sandbox/src/sandbox.ts index edcbc61c6..21668387b 100644 --- a/packages/sandbox/src/sandbox.ts +++ b/packages/sandbox/src/sandbox.ts @@ -1162,9 +1162,12 @@ export class Sandbox extends Container implements ISandbox { logger: this.logger, retryTimeoutMs: this.computeRetryTimeoutMs(), // Sandbox identifiers stamped on RPC trace spans (sandbox.id = DO id, - // sandbox.name = user-provided name). - sandboxId: this.ctx.id.toString(), - sandboxName: this.sandboxName ?? undefined, + // sandbox.name = user-provided name). Resolved lazily so a name set + // after the client is built is still reflected. + getSandboxInfo: () => ({ + id: this.ctx.id.toString(), + name: this.sandboxName ?? undefined + }), // Explicitly start the container before the RPC WebSocket upgrade. // Running start() here — rather than as a side effect of the upgrade // fetch through containerFetch() — makes the platform's diff --git a/packages/sandbox/tests/container-connection.test.ts b/packages/sandbox/tests/container-connection.test.ts index 9f214fb7b..7f5571372 100644 --- a/packages/sandbox/tests/container-connection.test.ts +++ b/packages/sandbox/tests/container-connection.test.ts @@ -96,6 +96,40 @@ describe('ContainerControlConnection', () => { }); }); + describe('sandbox trace identifiers', () => { + it('resolves getSandboxInfo lazily at span time, not at construction', () => { + const getSandboxInfo = vi.fn(() => ({ id: 'do-1', name: 'ws-1' })); + const conn = new ContainerControlConnection({ + stub: { fetch: vi.fn() }, + getSandboxInfo + }); + // Not queried on construction. + expect(getSandboxInfo).not.toHaveBeenCalled(); + + // A span-emitting path (disconnect) queries it. + conn.disconnect(); + expect(getSandboxInfo).toHaveBeenCalled(); + }); + + it('reflects a name that changes after the connection is built', () => { + let name: string | undefined; + const getSandboxInfo = vi.fn(() => ({ id: 'do-2', name })); + const conn = new ContainerControlConnection({ + stub: { fetch: vi.fn() }, + getSandboxInfo + }); + + // Name assigned after construction. + name = 'late-name'; + conn.disconnect(); + + // The lazy callback saw the updated name (proves it isn't captured at + // construction time). + const last = getSandboxInfo.mock.results.at(-1)?.value; + expect(last).toEqual({ id: 'do-2', name: 'late-name' }); + }); + }); + describe('connect', () => { it('should fail when WebSocket upgrade is rejected', async () => { const conn = new ContainerControlConnection({ From 3d0e938ef3e1020999fd50fed29daea58eed10aa Mon Sep 17 00:00:00 2001 From: Aron <263346377+aron-cf@users.noreply.github.com> Date: Wed, 1 Jul 2026 18:21:53 +0100 Subject: [PATCH 17/24] Rank teardown causes as weak so the real failure wins A lifecycle teardown (onStop firing runtime_replaced) was overwriting the authoritative container-admission cause the connect attempt had already stamped, so a queued RPC surfaced "container stopped" while the connect.attempt span showed the real "no container instance". Track whether lastConnectionError came from an actual connect-attempt failure (authoritative) vs a teardown (weak); weak causes only stamp when nothing authoritative was captured. The connection no longer fires onConnectionError on disconnect; the client stamps the teardown cause weakly before disposing the stub. --- .../sandbox/src/container-control/client.ts | 37 +++++++++-- .../src/container-control/connection.ts | 11 ++-- .../tests/container-connection.test.ts | 18 ++--- .../sandbox/tests/rpc-sandbox-client.test.ts | 65 ++++++++++++++++++- 4 files changed, 107 insertions(+), 24 deletions(-) diff --git a/packages/sandbox/src/container-control/client.ts b/packages/sandbox/src/container-control/client.ts index b769c420c..60668ee09 100644 --- a/packages/sandbox/src/container-control/client.ts +++ b/packages/sandbox/src/container-control/client.ts @@ -675,6 +675,15 @@ export class ContainerControlClient { * fresh connection is created. */ private lastConnectionError: unknown = null; + /** + * Whether `lastConnectionError` was captured from an actual connection + * attempt failure (authoritative root cause) rather than a lifecycle + * teardown reason (weak). A weak teardown cause — e.g. `onStop` firing + * `runtime_replaced` — is usually a downstream *consequence* of the real + * failure, so it must not overwrite an authoritative cause already + * captured from the connect attempt. + */ + private connectionErrorIsAuthoritative = false; /** * Whether the current connection ever established a live session to a * running container. Set true by the connection's `onConnected` callback, @@ -722,7 +731,9 @@ export class ContainerControlClient { // queued RPC rejections. `translateRPCError` prefers this over the // generic disposal / connection-failure transport error. onConnectionError: (error: unknown) => { + // Authoritative: captured from an actual connection-attempt failure. this.lastConnectionError = error; + this.connectionErrorIsAuthoritative = true; } }; this.idleDisconnectMs = @@ -746,6 +757,7 @@ export class ContainerControlClient { private getConnection(): ContainerControlConnection { if (!this.conn) { this.lastConnectionError = null; + this.connectionErrorIsAuthoritative = false; this.sessionEstablished = false; this.conn = new ContainerControlConnection(this.connOptions); this.startBusyPoll(); @@ -753,6 +765,17 @@ export class ContainerControlClient { return this.conn; } + /** + * Stamp a *weak* connection cause (a lifecycle teardown reason). Only takes + * effect if no authoritative connect-attempt failure has been captured, so + * a teardown consequence never masks the real root cause. + */ + private stampWeakConnectionCause(cause: unknown): void { + if (cause === undefined) return; + if (this.connectionErrorIsAuthoritative) return; + this.lastConnectionError = cause; + } + // ------------------------------------------------------------------------- // Activity & busy/idle tracking // ------------------------------------------------------------------------- @@ -924,9 +947,12 @@ export class ContainerControlClient { this.onSessionIdle?.(); } if (this.conn) { - // Pass the cause so queued RPC calls reject with the real reason - // (stamped into lastConnectionError via onConnectionError) instead of - // the generic capnweb disposal message. + // Weakly stamp the teardown cause so queued RPC calls reject with a + // real reason instead of the generic capnweb disposal message — but + // never overwrite an authoritative connect-attempt failure (the + // teardown is usually a downstream consequence of it). Stamp before + // disconnect() disposes the stub and rejects the queued calls. + this.stampWeakConnectionCause(cause); this.conn.disconnect(cause); this.conn = null; } @@ -1091,8 +1117,9 @@ export class ContainerControlClient { disconnect(cause?: unknown): void { const conn = this.conn; if (conn?.isConnecting()) { - // Stamp the cause now so an in-flight-attempt failure surfaces it. - if (cause !== undefined) this.lastConnectionError = cause; + // Weakly stamp the cause so an in-flight-attempt failure (authoritative) + // still wins over this teardown reason. + this.stampWeakConnectionCause(cause); // Defer the actual teardown until the attempt settles. Guard with an // identity check so we don't destroy a newer connection. void conn.whenSettled().then(() => { diff --git a/packages/sandbox/src/container-control/connection.ts b/packages/sandbox/src/container-control/connection.ts index d789ca1db..8f17cd22d 100644 --- a/packages/sandbox/src/container-control/connection.ts +++ b/packages/sandbox/src/container-control/connection.ts @@ -420,14 +420,15 @@ export class ContainerControlConnection { } /** - * Tear down the connection. When `cause` is provided (e.g. a lifecycle - * teardown from the owning DO), it is handed to `onConnectionError` first - * so any RPC calls queued on the transport reject with that real cause - * rather than the generic capnweb "disposing the main stub" message. + * Tear down the connection. `cause` (a lifecycle teardown reason from the + * owning DO) is used only for the disconnect trace event here; the owner + * is responsible for stamping it as a *weak* connection cause (one that + * never overwrites an authoritative failure already captured from a + * connection attempt), since a teardown is usually a downstream + * consequence of that earlier failure rather than the root cause. */ disconnect(cause?: unknown): void { if (cause !== undefined) { - this.fireConnectionError(cause); traceEvent('sandbox.rpc.disconnect', { ...this.spanAttrs(), reason: cause instanceof Error ? cause.message : String(cause) diff --git a/packages/sandbox/tests/container-connection.test.ts b/packages/sandbox/tests/container-connection.test.ts index 7f5571372..cb7744698 100644 --- a/packages/sandbox/tests/container-connection.test.ts +++ b/packages/sandbox/tests/container-connection.test.ts @@ -46,24 +46,16 @@ describe('ContainerControlConnection', () => { expect(conn.isConnected()).toBe(false); }); - it('fires onConnectionError with the provided cause before disposing', () => { + it('does not fire onConnectionError on disconnect (owner stamps the cause weakly)', () => { + // The teardown cause is used only for the disconnect trace event; the + // owning client is responsible for stamping it as a weak cause so it + // never overwrites an authoritative connect-attempt failure. const onConnectionError = vi.fn(); const conn = new ContainerControlConnection({ stub: { fetch: vi.fn() }, onConnectionError }); - const cause = new Error('sandbox stopped'); - conn.disconnect(cause); - expect(onConnectionError).toHaveBeenCalledWith(cause); - }); - - it('does not fire onConnectionError when disconnected without a cause', () => { - const onConnectionError = vi.fn(); - const conn = new ContainerControlConnection({ - stub: { fetch: vi.fn() }, - onConnectionError - }); - conn.disconnect(); + conn.disconnect(new Error('sandbox stopped')); expect(onConnectionError).not.toHaveBeenCalled(); }); }); diff --git a/packages/sandbox/tests/rpc-sandbox-client.test.ts b/packages/sandbox/tests/rpc-sandbox-client.test.ts index 6516cdff1..d0db1625c 100644 --- a/packages/sandbox/tests/rpc-sandbox-client.test.ts +++ b/packages/sandbox/tests/rpc-sandbox-client.test.ts @@ -26,6 +26,8 @@ let commandExecuteImpl: (...args: unknown[]) => unknown = () => ({ * runtime dispatching that event. */ const onCloseHandlers: Array<() => void> = []; +/** `onConnectionError` callbacks installed on each mock connection. */ +const onConnectionErrorHandlers: Array<(error: unknown) => void> = []; /** * Simulate the runtime firing a `close` / `error` event on the live @@ -43,8 +45,15 @@ function triggerPeerClose(): boolean { vi.mock('../src/container-control/connection', () => ({ ContainerControlConnection: class { - constructor(options: { onClose?: () => void } = {}) { + constructor( + options: { + onClose?: () => void; + onConnectionError?: (error: unknown) => void; + } = {} + ) { if (options.onClose) onCloseHandlers.push(options.onClose); + if (options.onConnectionError) + onConnectionErrorHandlers.push(options.onConnectionError); } isConnected() { return connected; @@ -86,6 +95,7 @@ describe('ContainerControlClient busy/idle tracking', () => { connected = true; disconnects.length = 0; onCloseHandlers.length = 0; + onConnectionErrorHandlers.length = 0; commandExecuteImpl = () => ({ exitCode: 0, stdout: '', stderr: '' }); }); @@ -284,6 +294,59 @@ describe('ContainerControlClient busy/idle tracking', () => { vi.advanceTimersByTime(1_000); expect(disconnects).toHaveLength(1); }); + + it('surfaces the authoritative connect-attempt cause on a queued call even after a weak teardown', async () => { + vi.useRealTimers(); + const { ContainerUnavailableError } = await import('../src/errors'); + const client = new ContainerControlClient({ stub: { fetch: vi.fn() } }); + + // Materialize the connection (wires onConnectionError) and capture the + // wrapped stub that an in-flight call would hold. + const commands = client.commands; + const fireConnectionError = onConnectionErrorHandlers.at(-1); + expect(fireConnectionError).toBeDefined(); + + // 1) Connect attempt stamps the authoritative capacity cause. + const capacityCause = new ContainerUnavailableError({ + code: 'CONTAINER_UNAVAILABLE', + message: 'no container instance', + context: { + reason: 'no_container_instance_available', + retryable: true, + originalMessage: 'no container instance' + }, + httpStatus: 503, + timestamp: new Date().toISOString() + }); + fireConnectionError?.(capacityCause); + + // 2) A lifecycle teardown fires a weak cause. Use the mock's onClose so + // the client tears down without recreating the connection under us. + connected = false; + // Simulate the weak teardown stamp path directly via the public API. + // (destroyConnection weak-stamps; authoritative must win.) + const onClose = onCloseHandlers.at(-1); + onClose?.(); + + // 3) The queued call rejects with the generic disposal error; it must + // surface the authoritative capacity cause via the retained stub. + commandExecuteImpl = () => { + throw new Error('RPC session was shut down by disposing the main stub'); + }; + let thrown: unknown; + try { + // The mock throws synchronously; the real capnweb stub rejects. Handle + // both so we assert on the surfaced cause either way. + thrown = await commands.execute('echo', 'default'); + } catch (e) { + thrown = e; + } + + expect(thrown).toBeInstanceOf(ContainerUnavailableError); + expect( + (thrown as InstanceType).reason + ).toBe('no_container_instance_available'); + }); }); describe('translateRPCError', () => { From cc3547a522e8ceff68d05b01b7e58f05a4247ec7 Mon Sep 17 00:00:00 2001 From: Aron <263346377+aron-cf@users.noreply.github.com> Date: Wed, 1 Jul 2026 18:30:21 +0100 Subject: [PATCH 18/24] Wrap the whole connect retry loop in one span Previously only the first upgrade attempt's span attached to the trace: the retry loop's setTimeout backoff resumes in a detached async context that no longer carries the request trace context, so later attempts were invisible and the connect flow looked far shorter than the RPC call awaiting it. Wrap the entire doConnect retry loop in a single sandbox.rpc.connect span so its duration covers every attempt, with per-attempt spans nested inside. --- .../sandbox/src/container-control/connection.ts | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/packages/sandbox/src/container-control/connection.ts b/packages/sandbox/src/container-control/connection.ts index 8f17cd22d..3b4cd5c72 100644 --- a/packages/sandbox/src/container-control/connection.ts +++ b/packages/sandbox/src/container-control/connection.ts @@ -545,6 +545,19 @@ export class ContainerControlConnection { }; private async doConnect(): Promise { + // Wrap the entire connect + retry loop in a single span so its duration + // spans every attempt (and matches the RPC call that is waiting on it). + // Per-attempt spans (sandbox.rpc.connect.attempt) nest inside this one. + // Without this, only the first attempt's span attaches to the trace: the + // retry loop's setTimeout backoff resumes in a detached async context + // that no longer carries the request's trace context, so later attempts + // would be invisible. + return withSpan('sandbox.rpc.connect', this.spanAttrs(), () => + this.doConnectInner() + ); + } + + private async doConnectInner(): Promise { try { const response = await this.fetchUpgradeWithRetry(); @@ -606,10 +619,6 @@ export class ContainerControlConnection { ); } - traceEvent('sandbox.rpc.connect', { - ...this.spanAttrs(), - outcome: 'established' - }); this.logger.debug('ContainerControlConnection established', { port: this.port }); From e4bc0c890d1122e6cd30f6c91734b729fdcc55d5 Mon Sep 17 00:00:00 2001 From: Aron <263346377+aron-cf@users.noreply.github.com> Date: Wed, 1 Jul 2026 20:51:51 +0100 Subject: [PATCH 19/24] Stamp wrapped cause chain on RPC error spans --- .changeset/surface-container-unavailable.md | 2 +- .../sandbox/src/container-control/tracing.ts | 86 +++++++++++++++--- packages/sandbox/tests/tracing.test.ts | 90 +++++++++++++++++++ 3 files changed, 167 insertions(+), 11 deletions(-) create mode 100644 packages/sandbox/tests/tracing.test.ts diff --git a/.changeset/surface-container-unavailable.md b/.changeset/surface-container-unavailable.md index a650de4ab..60f9403a8 100644 --- a/.changeset/surface-container-unavailable.md +++ b/.changeset/surface-container-unavailable.md @@ -2,4 +2,4 @@ '@cloudflare/sandbox': patch --- -Surface container capacity failures as retryable `ContainerUnavailableError` (with a `reason`) instead of masking them as `utils.createSession` interruptions or raw transport errors, and make `destroy()` an idempotent no-op when the container was never admitted. +Surface container capacity failures as retryable `ContainerUnavailableError` (with a `reason`) instead of masking them as `utils.createSession` interruptions or raw transport errors, make `destroy()` an idempotent no-op when the container was never admitted, and record the wrapped error `.cause` chain on RPC trace spans so the true startup failure (e.g. container not listening, network loss) is visible instead of the generic "no container instance" wrapper. diff --git a/packages/sandbox/src/container-control/tracing.ts b/packages/sandbox/src/container-control/tracing.ts index 83b9d2d20..875adad29 100644 --- a/packages/sandbox/src/container-control/tracing.ts +++ b/packages/sandbox/src/container-control/tracing.ts @@ -10,6 +10,13 @@ * span attributes specially (this differs from the OpenTelemetry standard, * which uses exception events). We therefore stamp both attributes on the span * when the wrapped work throws, so failures are visible in the GUI. + * + * Cause chain: the base `@cloudflare/containers` class wraps the true failure + * inside `new Error(NO_CONTAINER_INSTANCE_ERROR, { cause })`, so the real + * reason (e.g. "the container is not listening", "Network connection lost", a + * non-zero exit code) is only visible on `.cause`. Every distinct failure mode + * therefore collapses to the same generic top-level message in traces. We walk + * the cause chain and stamp it so a trace names the actual root cause. */ import { tracing } from 'cloudflare:workers'; @@ -21,22 +28,81 @@ export interface TraceSpan { type SpanAttributes = Record; +/** Bound on cause-chain traversal — guards against cycles and runaway depth. */ +const MAX_CAUSE_DEPTH = 8; + +/** Read a string `code` property off an arbitrary value, if present. */ +function stringCode(value: unknown): string | undefined { + const code = (value as { code?: unknown } | null | undefined)?.code; + return typeof code === 'string' ? code : undefined; +} + +/** Render any thrown value as a short human-readable message. */ +function messageOf(value: unknown): string { + return value instanceof Error ? value.message : String(value); +} + +/** + * Compute the Cloudflare-convention error span attributes for any thrown value, + * including the wrapped `.cause` chain. Pure and side-effect-free so it can be + * unit-tested without a live tracer. + * + * Emits: + * - `error` — top-level message (or stringified non-Error) + * - `error.stack` — top-level stack, when available + * - `error.code` — top-level string `code`, when available + * - `error.cause` — immediate cause message, when a cause exists + * - `error.cause.code` — immediate cause string `code`, when available + * - `error.cause_chain`— all nested cause messages joined by " <- " + */ +export function computeErrorAttributes(error: unknown): SpanAttributes { + const attrs: SpanAttributes = {}; + + if (error instanceof Error) { + attrs.error = error.message; + if (typeof error.stack === 'string') attrs['error.stack'] = error.stack; + const code = stringCode(error); + if (code !== undefined) attrs['error.code'] = code; + } else { + attrs.error = String(error); + return attrs; + } + + // Walk the cause chain, guarding against cycles and runaway depth. + const chain: string[] = []; + const seen = new Set([error]); + let current: unknown = (error as { cause?: unknown }).cause; + let depth = 0; + while (current !== undefined && current !== null && depth < MAX_CAUSE_DEPTH) { + if (seen.has(current)) break; + seen.add(current); + + if (depth === 0) { + attrs['error.cause'] = messageOf(current); + const causeCode = stringCode(current); + if (causeCode !== undefined) attrs['error.cause.code'] = causeCode; + } + chain.push(messageOf(current)); + + current = + current instanceof Error + ? (current as { cause?: unknown }).cause + : undefined; + depth++; + } + + if (chain.length > 0) attrs['error.cause_chain'] = chain.join(' <- '); + return attrs; +} + /** * Stamp the Cloudflare-convention error attributes onto a span. Safe to call * with any thrown value. */ function setErrorAttributes(span: TraceSpan, error: unknown): void { - if (error instanceof Error) { - span.setAttribute('error', error.message); - if (typeof error.stack === 'string') { - span.setAttribute('error.stack', error.stack); - } - // A code (e.g. SandboxError.code) is high-signal for filtering. - const code = (error as { code?: unknown }).code; - if (typeof code === 'string') span.setAttribute('error.code', code); - return; + for (const [key, value] of Object.entries(computeErrorAttributes(error))) { + if (value !== undefined) span.setAttribute(key, value); } - span.setAttribute('error', String(error)); } /** diff --git a/packages/sandbox/tests/tracing.test.ts b/packages/sandbox/tests/tracing.test.ts new file mode 100644 index 000000000..c03bac942 --- /dev/null +++ b/packages/sandbox/tests/tracing.test.ts @@ -0,0 +1,90 @@ +import { describe, expect, it } from 'vitest'; +import { computeErrorAttributes } from '../src/container-control/tracing'; + +/** + * Tests for computeErrorAttributes — the pure error → span-attribute mapping + * used by the RPC tracing helper. + * + * The key behavior under test is cause-chain capture: the base + * `@cloudflare/containers` class wraps the true failure inside + * `new Error(NO_CONTAINER_INSTANCE_ERROR, { cause })`, so the real reason + * (e.g. "the container is not listening", "Network connection lost") is only + * visible on `.cause`. Without stamping the chain, every capacity/startup + * failure looks identical in traces. + */ +describe('computeErrorAttributes', () => { + it('stamps message and stack for a plain Error', () => { + const err = new Error('boom'); + const attrs = computeErrorAttributes(err); + expect(attrs.error).toBe('boom'); + expect(typeof attrs['error.stack']).toBe('string'); + }); + + it('stamps a string code when present', () => { + const err = Object.assign(new Error('boom'), { + code: 'CONTAINER_UNAVAILABLE' + }); + const attrs = computeErrorAttributes(err); + expect(attrs['error.code']).toBe('CONTAINER_UNAVAILABLE'); + }); + + it('stringifies a non-Error thrown value', () => { + const attrs = computeErrorAttributes('just a string'); + expect(attrs.error).toBe('just a string'); + }); + + it('stamps the immediate cause message', () => { + const cause = new Error('the container is not listening'); + const err = new Error( + 'there is no container instance that can be provided to this durable object', + { cause } + ); + const attrs = computeErrorAttributes(err); + expect(attrs.error).toBe( + 'there is no container instance that can be provided to this durable object' + ); + expect(attrs['error.cause']).toBe('the container is not listening'); + }); + + it('stamps the cause code when the cause carries one', () => { + const cause = Object.assign(new Error('not listening'), { + code: 'NOT_LISTENING' + }); + const err = new Error('wrapper', { cause }); + const attrs = computeErrorAttributes(err); + expect(attrs['error.cause.code']).toBe('NOT_LISTENING'); + }); + + it('joins a multi-level cause chain into error.cause_chain', () => { + const root = new Error('Network connection lost'); + const mid = new Error('the container is not listening', { cause: root }); + const top = new Error('there is no container instance', { cause: mid }); + const attrs = computeErrorAttributes(top); + const chain = attrs['error.cause_chain']; + expect(typeof chain).toBe('string'); + expect(chain).toContain('the container is not listening'); + expect(chain).toContain('Network connection lost'); + }); + + it('handles a non-Error cause value', () => { + const err = new Error('wrapper', { cause: 'raw string cause' }); + const attrs = computeErrorAttributes(err); + expect(attrs['error.cause']).toBe('raw string cause'); + }); + + it('does not emit cause attributes when there is no cause', () => { + const attrs = computeErrorAttributes(new Error('lonely')); + expect(attrs['error.cause']).toBeUndefined(); + expect(attrs['error.cause_chain']).toBeUndefined(); + }); + + it('terminates on a cyclic cause chain without hanging', () => { + const a = new Error('a'); + const b = new Error('b', { cause: a }); + (a as { cause?: unknown }).cause = b; // cycle + const attrs = computeErrorAttributes(b); + // Should complete and include what it walked before bailing. + expect(attrs['error.cause']).toBe('a'); + expect(typeof attrs['error.cause_chain']).toBe('string'); + }); +}); From 85f04a9c89a2d99b2531191be5dc0f80081f8e33 Mon Sep 17 00:00:00 2001 From: Aron <263346377+aron-cf@users.noreply.github.com> Date: Thu, 2 Jul 2026 11:43:32 +0100 Subject: [PATCH 20/24] Clear stale connect-attempt cause on session establish --- .../sandbox/src/container-control/client.ts | 6 ++ .../sandbox/tests/rpc-sandbox-client.test.ts | 85 ++++++++++++++++++- 2 files changed, 87 insertions(+), 4 deletions(-) diff --git a/packages/sandbox/src/container-control/client.ts b/packages/sandbox/src/container-control/client.ts index 60668ee09..f483bd04e 100644 --- a/packages/sandbox/src/container-control/client.ts +++ b/packages/sandbox/src/container-control/client.ts @@ -716,6 +716,12 @@ export class ContainerControlClient { // than surfacing as a never-connected failure. onConnected: () => { this.sessionEstablished = true; + // A prior attempt may have stamped an authoritative connect-attempt + // failure (e.g. a capacity error) before a retry succeeded. That cause + // is now stale: the session is live. Clear it so a later lifecycle + // teardown surfaces its own reason instead of this masked stale error. + this.lastConnectionError = null; + this.connectionErrorIsAuthoritative = false; }, // Event-driven failure recovery: when the live WebSocket closes // or errors, tear the connection down inside the same turn of diff --git a/packages/sandbox/tests/rpc-sandbox-client.test.ts b/packages/sandbox/tests/rpc-sandbox-client.test.ts index d0db1625c..73362e620 100644 --- a/packages/sandbox/tests/rpc-sandbox-client.test.ts +++ b/packages/sandbox/tests/rpc-sandbox-client.test.ts @@ -28,6 +28,8 @@ let commandExecuteImpl: (...args: unknown[]) => unknown = () => ({ const onCloseHandlers: Array<() => void> = []; /** `onConnectionError` callbacks installed on each mock connection. */ const onConnectionErrorHandlers: Array<(error: unknown) => void> = []; +/** `onConnected` callbacks installed on each mock connection. */ +const onConnectedHandlers: Array<() => void> = []; /** * Simulate the runtime firing a `close` / `error` event on the live @@ -49,11 +51,13 @@ vi.mock('../src/container-control/connection', () => ({ options: { onClose?: () => void; onConnectionError?: (error: unknown) => void; + onConnected?: () => void; } = {} ) { if (options.onClose) onCloseHandlers.push(options.onClose); if (options.onConnectionError) onConnectionErrorHandlers.push(options.onConnectionError); + if (options.onConnected) onConnectedHandlers.push(options.onConnected); } isConnected() { return connected; @@ -96,6 +100,7 @@ describe('ContainerControlClient busy/idle tracking', () => { disconnects.length = 0; onCloseHandlers.length = 0; onConnectionErrorHandlers.length = 0; + onConnectedHandlers.length = 0; commandExecuteImpl = () => ({ exitCode: 0, stdout: '', stderr: '' }); }); @@ -274,10 +279,10 @@ describe('ContainerControlClient busy/idle tracking', () => { const pending = client.commands.execute('sleep 10', 'default'); - // Reproduce the race from sandbox-sdk#794: capnweb stats can report the - // bootstrap baseline while the method promise is still pending. The old - // implementation treated that as idle, armed the 1s disconnect timer, and - // disposed the main stub while the operation was still in flight. + // capnweb stats can report the bootstrap baseline while the method promise + // is still pending. A pending in-flight call must keep the session busy so + // the idle-disconnect timer stays disarmed and the main stub is not + // disposed mid-operation. stats = { imports: 1, exports: 1 }; vi.advanceTimersByTime(5_000); @@ -347,6 +352,78 @@ describe('ContainerControlClient busy/idle tracking', () => { (thrown as InstanceType).reason ).toBe('no_container_instance_available'); }); + + it('clears a retried connect-attempt cause once the session establishes, so a later teardown surfaces its own cause', async () => { + vi.useRealTimers(); + const { ContainerUnavailableError, OperationInterruptedError } = + await import('../src/errors'); + const client = new ContainerControlClient({ stub: { fetch: vi.fn() } }); + + // Materialize the connection (wires onConnectionError / onConnected). + const commands = client.commands; + const fireConnectionError = onConnectionErrorHandlers.at(-1); + const fireConnected = onConnectedHandlers.at(-1); + expect(fireConnectionError).toBeDefined(); + expect(fireConnected).toBeDefined(); + + // 1) An early connect attempt fails with a capacity cause (authoritative). + fireConnectionError?.( + new ContainerUnavailableError({ + code: 'CONTAINER_UNAVAILABLE', + message: 'no container instance', + context: { + reason: 'no_container_instance_available', + retryable: true, + originalMessage: 'no container instance' + }, + httpStatus: 503, + timestamp: new Date().toISOString() + }) + ); + + // 2) A subsequent retry succeeds: the session establishes. This must clear + // the now-stale capacity cause so it can't leak into a later teardown. + fireConnected?.(); + + // 3) A lifecycle teardown later stamps a non-retryable interruption cause + // (as the DO does when the sandbox lifetime changes). With the stale + // capacity cause cleared, this is no longer blocked by an authoritative + // error, so it becomes the weak cause on the queued calls. + const lifetimeCause = new OperationInterruptedError({ + code: 'OPERATION_INTERRUPTED', + message: 'sandbox lifetime changed', + context: { + reason: 'sandbox_lifetime_changed', + operation: 'rpc.connect', + phase: 'connection', + admitted: 'unknown', + retryable: false + }, + httpStatus: 500, + timestamp: new Date().toISOString() + }); + client.disconnect(lifetimeCause); + + // 4) The queued call rejects with the generic disposal error. Because the + // stale capacity cause was cleared on connect, the non-retryable + // lifetime cause surfaces — not a misleading retryable + // "no container instance". + commandExecuteImpl = () => { + throw new Error('RPC session was shut down by disposing the main stub'); + }; + let thrown: unknown; + try { + thrown = await commands.execute('echo', 'default'); + } catch (e) { + thrown = e; + } + + expect(thrown).not.toBeInstanceOf(ContainerUnavailableError); + expect(thrown).toBeInstanceOf(OperationInterruptedError); + expect( + (thrown as InstanceType).reason + ).toBe('sandbox_lifetime_changed'); + }); }); describe('translateRPCError', () => { From 07cd0f9e8e7e29f63668ca6b994beacd601eff85 Mon Sep 17 00:00:00 2001 From: Aron <263346377+aron-cf@users.noreply.github.com> Date: Thu, 2 Jul 2026 12:16:24 +0100 Subject: [PATCH 21/24] Consolidate platform admission-error matching into one helper --- .../src/container-control/connection.ts | 82 +++---------------- packages/sandbox/src/platform-errors.ts | 70 ++++++++++++++++ packages/sandbox/src/sandbox.ts | 42 ++-------- .../sandbox/tests/platform-errors.test.ts | 58 ++++++++++++- .../tests/sandbox-error-handling.test.ts | 2 +- 5 files changed, 148 insertions(+), 106 deletions(-) diff --git a/packages/sandbox/src/container-control/connection.ts b/packages/sandbox/src/container-control/connection.ts index 3b4cd5c72..c02f3f82d 100644 --- a/packages/sandbox/src/container-control/connection.ts +++ b/packages/sandbox/src/container-control/connection.ts @@ -24,6 +24,10 @@ import { ErrorCode, getHttpStatus, getSuggestion } from '@repo/shared/errors'; import { RpcSession, type RpcStub, type RpcTransport } from 'capnweb'; import { createErrorFromResponse } from '../errors/adapter'; import { SandboxError } from '../errors/classes'; +import { + matchContainerUnavailable, + type PlatformUnavailableReason +} from '../platform-errors'; import { fetchWithResponseRetry, isRetryableWebSocketUpgradeResponse @@ -102,82 +106,22 @@ async function tryParseContainerUnavailable( // generic rpc_upgrade_failed. try { const text = await response.clone().text(); - const match = matchPlatformUnavailable(text); - if (!match) return null; - return buildContainerUnavailableError(match.reason, text); + const reason = matchContainerUnavailable(text); + if (!reason) return null; + return buildContainerUnavailableError(reason, text); } catch { return null; } } -/** - * Platform messages emitted by the Containers runtime when it cannot admit a - * container for a Durable Object during startup. They surface as plain Errors - * thrown from the container-binding fetch, before the capnweb session is - * established. Each maps to a categorical `ContainerUnavailableContext.reason`. - */ -const PLATFORM_UNAVAILABLE_SIGNATURES: ReadonlyArray<{ - /** Lowercase substring matched case-insensitively against the error text. */ - substring: string; - reason: - | 'no_container_instance_available' - | 'max_container_instances_exceeded'; -}> = [ - { - // Platform error thrown from the container binding during startup. - substring: - 'there is no container instance that can be provided to this durable object', - reason: 'no_container_instance_available' - }, - { - // Plain-text 503 body returned by @cloudflare/containers' containerFetch - // when no instance can be admitted (see container.ts). - substring: 'there is no container instance available at this time', - reason: 'no_container_instance_available' - }, - { - substring: 'maximum number of running container instances exceeded', - reason: 'max_container_instances_exceeded' - } -]; - -/** - * Extract a matchable message string from any thrown value. - * - * Deliberately avoids `instanceof Error`: the platform's container-admission - * errors are raised by the workerd container binding, which may live in a - * different realm than this SDK bundle, so `instanceof Error` can be false - * even for a genuine Error (the same cross-realm trap documented in - * container-control/client.ts). Mirrors the base `@cloudflare/containers` - * `isErrorOfType` helper: coerce to string, then match case-insensitively. - */ -function errorText(error: unknown): string { - const message = (error as { message?: unknown } | null | undefined)?.message; - return (typeof message === 'string' ? message : String(error)).toLowerCase(); -} - -/** - * Find the platform container-admission signature matching a thrown value, - * or null. Case-insensitive and realm-safe (does not use `instanceof`). - */ -function matchPlatformUnavailable( - error: unknown -): (typeof PLATFORM_UNAVAILABLE_SIGNATURES)[number] | null { - const text = errorText(error); - return ( - PLATFORM_UNAVAILABLE_SIGNATURES.find((sig) => - text.includes(sig.substring) - ) ?? null - ); -} - /** * True when a thrown connection-startup error matches a known platform * container-admission failure. These are transient: the platform asks the * caller to try again later, so they are safe to retry within the budget. + * Delegates to the shared matcher in platform-errors.ts. */ function isPlatformUnavailableError(error: unknown): boolean { - return matchPlatformUnavailable(error) !== null; + return matchContainerUnavailable(error) !== null; } /** @@ -185,7 +129,7 @@ function isPlatformUnavailableError(error: unknown): boolean { * container-admission failure, preserving the original message verbatim. */ function buildContainerUnavailableError( - reason: (typeof PLATFORM_UNAVAILABLE_SIGNATURES)[number]['reason'], + reason: PlatformUnavailableReason, originalMessage: string, cause?: unknown ): Error { @@ -217,12 +161,12 @@ function tryConvertPlatformUnavailable(error: unknown): Error | null { // preserve it rather than rebuilding an equivalent instance. if (error instanceof SandboxError) return error; - const match = matchPlatformUnavailable(error); - if (!match) return null; + const reason = matchContainerUnavailable(error); + if (!reason) return null; const originalMessage = error instanceof Error ? error.message : String(error); - return buildContainerUnavailableError(match.reason, originalMessage, error); + return buildContainerUnavailableError(reason, originalMessage, error); } // --------------------------------------------------------------------------- diff --git a/packages/sandbox/src/platform-errors.ts b/packages/sandbox/src/platform-errors.ts index eb7e0abf9..9787aaa2f 100644 --- a/packages/sandbox/src/platform-errors.ts +++ b/packages/sandbox/src/platform-errors.ts @@ -12,6 +12,76 @@ function errorMessageOf(error: unknown): string { : ''; } +/** + * Extract a matchable message from any thrown value, reading a `.message` + * property even when the value is not an `Error` instance. The Containers + * runtime raises admission failures from the container binding, which may + * live in a different realm — so `instanceof Error` can be false for a + * genuine error. Mirrors the base `@cloudflare/containers` `isErrorOfType` + * helper: coerce to a string, then match case-insensitively. + */ +function realmSafeMessageOf(error: unknown): string { + const message = (error as { message?: unknown } | null | undefined)?.message; + return typeof message === 'string' ? message : String(error); +} + +/** + * Categorical reason the Containers platform could not admit an instance for + * a Durable Object during startup. A subset of `ContainerUnavailableReason` + * covering the two failures the platform signals by *throwing* (rather than + * returning a structured body). + */ +export type PlatformUnavailableReason = + | 'no_container_instance_available' + | 'max_container_instances_exceeded'; + +/** + * Platform messages emitted by the Containers runtime when it cannot admit a + * container for a Durable Object during startup. Matched case-insensitively as + * lowercase substrings. Single source of truth for both the RPC connection + * path (connection.ts) and the HTTP `containerFetch` path (sandbox.ts). + */ +const CONTAINER_UNAVAILABLE_SIGNATURES: ReadonlyArray<{ + substring: string; + reason: PlatformUnavailableReason; +}> = [ + { + // Platform error thrown from the container binding during startup. + substring: + 'there is no container instance that can be provided to this durable object', + reason: 'no_container_instance_available' + }, + { + // Plain-text 503 body returned by @cloudflare/containers' containerFetch + // when no instance can be admitted (see container.ts). + substring: 'there is no container instance available at this time', + reason: 'no_container_instance_available' + }, + { + substring: 'maximum number of running container instances exceeded', + reason: 'max_container_instances_exceeded' + } +]; + +/** + * Classify a container-startup error as a platform admission/capacity failure, + * returning the categorical reason or null. Realm-safe (does not gate on + * `instanceof Error`), case-insensitive, and walks the `.cause` chain so a + * wrapped admission failure is still recognized. + */ +export function matchContainerUnavailable( + error: unknown +): PlatformUnavailableReason | null { + for (const candidate of selfAndCauses(error)) { + const text = realmSafeMessageOf(candidate).toLowerCase(); + const match = CONTAINER_UNAVAILABLE_SIGNATURES.find((sig) => + text.includes(sig.substring) + ); + if (match) return match.reason; + } + return null; +} + function* selfAndCauses(error: unknown): Generator { let current = error; for (let depth = 0; depth < 8 && current != null; depth += 1) { diff --git a/packages/sandbox/src/sandbox.ts b/packages/sandbox/src/sandbox.ts index 21668387b..b3ad8af40 100644 --- a/packages/sandbox/src/sandbox.ts +++ b/packages/sandbox/src/sandbox.ts @@ -96,7 +96,10 @@ import { createErrorFromResponse } from './errors/adapter'; import { collectFile, streamFile } from './file-stream'; import { CodeInterpreter } from './interpreter'; import { LocalMountSyncManager } from './local-mount-sync'; -import { isPlatformTransientError } from './platform-errors'; +import { + isPlatformTransientError, + matchContainerUnavailable +} from './platform-errors'; import { forwardPreviewRequest, type PreviewTCPPort @@ -3141,7 +3144,7 @@ export class Sandbox extends Container implements ISandbox { // control connection's upgrade-response classifier — surfaces a // typed ContainerUnavailableError with an actionable reason rather // than a generic INTERNAL_ERROR / rpc_upgrade_failed. - const admissionReason = this.classifyContainerAdmissionError(e); + const admissionReason = matchContainerUnavailable(e); if (admissionReason) { const originalMessage = e instanceof Error ? e.message : String(e); const context = { @@ -3275,38 +3278,7 @@ export class Sandbox extends Container implements ISandbox { * This indicates the container VM is still being provisioned. */ private isNoInstanceError(error: unknown): boolean { - return this.classifyContainerAdmissionError(error) !== null; - } - - /** - * Classify a container-startup error as a platform admission/capacity - * failure, returning the categorical `ContainerUnavailableContext.reason` - * or null if it is not a recognized admission failure. - * - * Realm-safe and case-insensitive: the platform raises these from the - * container binding, which may live in a different realm, so we coerce to - * a string rather than gating on `instanceof Error`. - */ - private classifyContainerAdmissionError( - error: unknown - ): - | 'no_container_instance_available' - | 'max_container_instances_exceeded' - | null { - const message = - (error as { message?: unknown } | null | undefined)?.message ?? error; - const text = ( - typeof message === 'string' ? message : String(message) - ).toLowerCase(); - if (text.includes('no container instance')) { - return 'no_container_instance_available'; - } - if ( - text.includes('maximum number of running container instances exceeded') - ) { - return 'max_container_instances_exceeded'; - } - return null; + return matchContainerUnavailable(error) !== null; } /** @@ -3356,7 +3328,7 @@ export class Sandbox extends Container implements ISandbox { const originalMessage = error instanceof Error ? error.message : String(error); - const admissionReason = this.classifyContainerAdmissionError(error); + const admissionReason = matchContainerUnavailable(error); if (admissionReason) { const context = { reason: admissionReason, diff --git a/packages/sandbox/tests/platform-errors.test.ts b/packages/sandbox/tests/platform-errors.test.ts index ac537ab49..a5becb972 100644 --- a/packages/sandbox/tests/platform-errors.test.ts +++ b/packages/sandbox/tests/platform-errors.test.ts @@ -1,7 +1,8 @@ import { describe, expect, it } from 'vitest'; import { isDurableObjectCodeUpdateReset, - isPlatformTransientError + isPlatformTransientError, + matchContainerUnavailable } from '../src/platform-errors'; describe('platform error classifiers', () => { @@ -62,3 +63,58 @@ describe('platform error classifiers', () => { ).toBe(false); }); }); + +describe('matchContainerUnavailable', () => { + it('classifies the platform no-instance error thrown during startup', () => { + expect( + matchContainerUnavailable( + new Error( + 'there is no container instance that can be provided to this durable object' + ) + ) + ).toBe('no_container_instance_available'); + }); + + it('classifies the plain-text 503 no-instance body', () => { + expect( + matchContainerUnavailable( + 'There is no Container instance available at this time. Try again later.' + ) + ).toBe('no_container_instance_available'); + }); + + it('classifies the max-instances capacity error', () => { + expect( + matchContainerUnavailable( + new Error( + 'Maximum number of running container instances exceeded. Try again later.' + ) + ) + ).toBe('max_container_instances_exceeded'); + }); + + it('matches case-insensitively and without instanceof (realm-safe)', () => { + // A cross-realm error whose message is a plain property, not an Error. + const crossRealm = { + message: + 'THERE IS NO CONTAINER INSTANCE THAT CAN BE PROVIDED TO THIS DURABLE OBJECT' + }; + expect(matchContainerUnavailable(crossRealm)).toBe( + 'no_container_instance_available' + ); + }); + + it('walks the cause chain to find a wrapped admission failure', () => { + const cause = new Error( + 'there is no container instance that can be provided to this durable object' + ); + expect(matchContainerUnavailable(new Error('wrapper', { cause }))).toBe( + 'no_container_instance_available' + ); + }); + + it('returns null for unrelated errors', () => { + expect(matchContainerUnavailable(new Error('boom'))).toBeNull(); + expect(matchContainerUnavailable(undefined)).toBeNull(); + }); +}); diff --git a/packages/sandbox/tests/sandbox-error-handling.test.ts b/packages/sandbox/tests/sandbox-error-handling.test.ts index 690b06e45..66c4b020a 100644 --- a/packages/sandbox/tests/sandbox-error-handling.test.ts +++ b/packages/sandbox/tests/sandbox-error-handling.test.ts @@ -424,7 +424,7 @@ describe('Sandbox.containerFetch() error classification', () => { it('503 responses include Retry-After: 10 for provisioning errors', async () => { const response = await triggerContainerFetchWithError( - 'no container instance available' + 'There is no Container instance available at this time. Try again later.' ); expect(response.status).toBe(503); From f5531481a9b6f69de7f5107f2e7dda9e090723bb Mon Sep 17 00:00:00 2001 From: Aron <263346377+aron-cf@users.noreply.github.com> Date: Mon, 6 Jul 2026 12:37:51 +0100 Subject: [PATCH 22/24] Capture container exit code and stop reason on onStop disconnect cause --- packages/sandbox/src/sandbox.ts | 28 +++++++-- .../tests/sandbox-destroy-lifetime.test.ts | 57 +++++++++++++++++++ packages/shared/src/errors/contexts.ts | 15 +++++ 3 files changed, 94 insertions(+), 6 deletions(-) diff --git a/packages/sandbox/src/sandbox.ts b/packages/sandbox/src/sandbox.ts index b3ad8af40..75e4d437f 100644 --- a/packages/sandbox/src/sandbox.ts +++ b/packages/sandbox/src/sandbox.ts @@ -3,6 +3,7 @@ import { Container, getContainer, type OutboundHandlerContext, + type StopParams, switchPort } from '@cloudflare/containers'; import type { @@ -3028,8 +3029,11 @@ export class Sandbox extends Container implements ISandbox { ); } - override async onStop() { - this.logger.debug('Sandbox stopped'); + override async onStop(params?: StopParams) { + this.logger.debug('Sandbox stopped', { + exitCode: params?.exitCode, + reason: params?.reason + }); // Invalidate default-session state before the first await. Bumping // containerGeneration signals any in-flight initializeDefaultSession @@ -3055,11 +3059,14 @@ export class Sandbox extends Container implements ISandbox { // Disconnect the active client so open sockets do not hold the DO alive. // Stamp a runtime-replaced cause so a pending RPC surfaces a retryable - // interruption instead of a generic disposal string. + // interruption instead of a generic disposal string. Thread the platform's + // exit code / stop reason through so a trace can distinguish an OOM kill + // (137) or signalled eviction (143 / 'runtime_signal') from a clean exit. this.client.disconnect( this.buildDisconnectCause( 'runtime_replaced', - 'The sandbox container stopped while the operation was pending.' + 'The sandbox container stopped while the operation was pending.', + params ) ); @@ -3370,7 +3377,8 @@ export class Sandbox extends Container implements ISandbox { */ private buildDisconnectCause( reason: 'runtime_replaced' | 'sandbox_lifetime_changed', - detail: string + detail: string, + stopParams?: StopParams ): OperationInterruptedError { const retryable = reason === 'runtime_replaced'; const context = { @@ -3378,7 +3386,15 @@ export class Sandbox extends Container implements ISandbox { operation: 'rpc.connect', phase: 'connection', admitted: 'unknown' as const, - retryable + retryable, + // Preserve the platform's container-stop details when present so a trace + // names the actual cause of a mid-operation stop (OOM / signal / exit). + ...(stopParams?.exitCode !== undefined && { + containerExitCode: stopParams.exitCode + }), + ...(stopParams?.reason !== undefined && { + stopReason: stopParams.reason + }) }; return new OperationInterruptedError({ code: ErrorCode.OPERATION_INTERRUPTED, diff --git a/packages/sandbox/tests/sandbox-destroy-lifetime.test.ts b/packages/sandbox/tests/sandbox-destroy-lifetime.test.ts index b03ef4f11..0ccce059e 100644 --- a/packages/sandbox/tests/sandbox-destroy-lifetime.test.ts +++ b/packages/sandbox/tests/sandbox-destroy-lifetime.test.ts @@ -162,6 +162,63 @@ describe('Sandbox destroy() sandbox lifetime', () => { expect(lifetimePut).toBeUndefined(); }); + it('stamps the container exit code and stop reason on the disconnect cause', async () => { + const { OperationInterruptedError } = await import('../src/errors'); + const disconnectSpy = vi + .spyOn( + ( + sandbox as unknown as { + client: { disconnect: (c?: unknown) => void }; + } + ).client, + 'disconnect' + ) + .mockImplementation(() => {}); + + // The base container class calls onStop({ exitCode, reason }). A non-zero + // exit code (e.g. 137 = OOM kill) with reason 'runtime_signal' is exactly + // what we need visible to diagnose why a container died mid-operation. + await ( + sandbox as unknown as { + onStop: (p: { exitCode: number; reason: string }) => Promise; + } + ).onStop({ exitCode: 137, reason: 'runtime_signal' }); + + expect(disconnectSpy).toHaveBeenCalledTimes(1); + const cause = disconnectSpy.mock.calls[0][0]; + expect(cause).toBeInstanceOf(OperationInterruptedError); + const context = (cause as InstanceType) + .context; + expect(context.reason).toBe('runtime_replaced'); + expect(context.containerExitCode).toBe(137); + expect(context.stopReason).toBe('runtime_signal'); + }); + + it('tolerates onStop() invoked with no params (exit code/reason omitted)', async () => { + const { OperationInterruptedError } = await import('../src/errors'); + const disconnectSpy = vi + .spyOn( + ( + sandbox as unknown as { + client: { disconnect: (c?: unknown) => void }; + } + ).client, + 'disconnect' + ) + .mockImplementation(() => {}); + + await (sandbox as unknown as { onStop: () => Promise }).onStop(); + + expect(disconnectSpy).toHaveBeenCalledTimes(1); + const cause = disconnectSpy.mock.calls[0][0]; + expect(cause).toBeInstanceOf(OperationInterruptedError); + const context = (cause as InstanceType) + .context; + expect(context.reason).toBe('runtime_replaced'); + expect(context.containerExitCode).toBeUndefined(); + expect(context.stopReason).toBeUndefined(); + }); + it('resolves (idempotent no-op) when the container was never admitted', async () => { // Destroying a sandbox whose container never started (e.g. no instance // available under capacity pressure) must not throw: there is nothing to diff --git a/packages/shared/src/errors/contexts.ts b/packages/shared/src/errors/contexts.ts index f803b607c..804ae3b8e 100644 --- a/packages/shared/src/errors/contexts.ts +++ b/packages/shared/src/errors/contexts.ts @@ -386,4 +386,19 @@ export interface OperationInterruptedContext { recoveryAttempts?: number; /** Maximum number of internal recovery attempts allowed. */ maxRecoveryAttempts?: number; + /** + * Container process exit code reported by the platform when the runtime + * stopped mid-operation (`reason === 'runtime_replaced'`). Preserved from + * the base container `onStop` params so a trace distinguishes an OOM kill + * (137) or signal (143) from a clean exit (0). Absent when the interruption + * was not driven by a container stop. + */ + containerExitCode?: number; + /** + * Container stop reason reported by the platform (`'exit'` for a process + * exit, `'runtime_signal'` for a signalled shutdown/eviction). Preserved + * from the base container `onStop` params. Absent when not driven by a + * container stop. + */ + stopReason?: string; } From ad9f3437c42f42a50073a0badb1ca234e0a5f6fc Mon Sep 17 00:00:00 2001 From: Aron <263346377+aron-cf@users.noreply.github.com> Date: Mon, 6 Jul 2026 13:04:59 +0100 Subject: [PATCH 23/24] Align RPC instance-get budget with platform 8s default --- packages/sandbox/src/sandbox.ts | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/packages/sandbox/src/sandbox.ts b/packages/sandbox/src/sandbox.ts index 75e4d437f..5b87fbfd8 100644 --- a/packages/sandbox/src/sandbox.ts +++ b/packages/sandbox/src/sandbox.ts @@ -402,18 +402,17 @@ const BACKUP_DEFAULT_TTL_SECONDS = 259200; * hook, independent of the user-facing `containerTimeouts.instanceGetTimeoutMS` * (which still governs the HTTP path's `containerFetch`). * - * Deliberately small: it bounds how long a single start attempt polls for a - * container instance before the base class throws the classifiable - * `NO_CONTAINER_INSTANCE_ERROR`. The RPC control connection's own retry loop - * (`fetchWithResponseRetry`, ~2 min budget with exponential backoff) owns the - * cross-attempt retries, so a short inner budget means each attempt fails fast - * under capacity pressure and hands back to the outer loop — rather than one - * attempt burning ~30s and letting the DO be evicted mid-wait. Once an - * instance exists, `portReadyTimeoutMS` still governs app boot on the next - * attempt (the base fast-path returns immediately when the container is - * already running). + * Shorter than the SDK's user-facing 30s default, but long enough to match + * the Containers platform's own 8s instance-get default. This keeps one RPC + * start attempt from burning the full 30s under capacity pressure while still + * letting ordinary 4-8s cold provisions succeed on the first attempt instead + * of always paying the first backoff delay. The RPC control connection's own + * retry loop (`fetchWithResponseRetry`, ~2 min budget with exponential + * backoff) still owns cross-attempt retries. Once an instance exists, + * `portReadyTimeoutMS` still governs app boot on the next attempt (the base + * fast-path returns immediately when the container is already running). */ -const RPC_START_INSTANCE_GET_TIMEOUT_MS = 3_000; +const RPC_START_INSTANCE_GET_TIMEOUT_MS = 8_000; const BACKUP_MAX_NAME_LENGTH = 256; const BACKUP_CONTAINER_DIR = '/var/backups'; const BACKUP_STORAGE_PREFIX = 'backups'; From 66773f6b8b334d96d0744b8d62d25283c1f46c3b Mon Sep 17 00:00:00 2001 From: Aron <263346377+aron-cf@users.noreply.github.com> Date: Mon, 6 Jul 2026 13:05:07 +0100 Subject: [PATCH 24/24] Harden RPC client callbacks and deferred-disconnect cleanup --- .../sandbox/src/container-control/client.ts | 80 +++++++++++++++---- .../src/container-control/connection.ts | 18 +++-- .../sandbox/tests/rpc-sandbox-client.test.ts | 70 +++++++++++++++- 3 files changed, 145 insertions(+), 23 deletions(-) diff --git a/packages/sandbox/src/container-control/client.ts b/packages/sandbox/src/container-control/client.ts index f483bd04e..7e7339512 100644 --- a/packages/sandbox/src/container-control/client.ts +++ b/packages/sandbox/src/container-control/client.ts @@ -465,6 +465,12 @@ function maybePreferConnectionError( * before `doConnect` recorded a cause — which is, from the caller's view, the * container being unavailable. Retryable, with the raw transport message * preserved as `originalMessage` for diagnostics. + * + * `upgrade_failed` is intentionally excluded here. Unlike disposal/close + * during startup, an HTTP upgrade failure with no captured structured cause can + * be a permanent configuration or routing error (for example, a 404 on `/rpc`), + * so the generic transport classification is more accurate than retryable + * container unavailability. */ function buildNeverConnectedUnavailableResponse( transportResponse: ErrorResponse, @@ -786,10 +792,37 @@ export class ContainerControlClient { // Activity & busy/idle tracking // ------------------------------------------------------------------------- + private fireUserCallback( + name: 'onActivity' | 'onSessionBusy' | 'onSessionIdle', + callback: (() => void) | undefined + ): void { + if (!callback) return; + try { + callback(); + } catch (error) { + this.logger.warn('ContainerControlClient callback threw', { + callback: name, + error: error instanceof Error ? error.message : String(error) + }); + } + } + + private fireActivity(): void { + this.fireUserCallback('onActivity', this.onActivity); + } + + private fireSessionBusy(): void { + this.fireUserCallback('onSessionBusy', this.onSessionBusy); + } + + private fireSessionIdle(): void { + this.fireUserCallback('onSessionIdle', this.onSessionIdle); + } + private markBusy(): void { if (!this.busy) { this.busy = true; - this.onSessionBusy?.(); + this.fireSessionBusy(); } this.clearIdleTimer(); } @@ -805,8 +838,19 @@ export class ContainerControlClient { private maybeTransitionIdle(): void { const conn = this.conn; - if (!conn || !conn.isConnected()) return; + if (!conn) return; + if (!conn.isConnected()) { + // Calls can settle while their sends are still queued behind the + // deferred WebSocket upgrade. Keep `busy` true until either the session + // connects and the poller observes the baseline stats, or the attempt + // fails and `destroyConnection()` releases the busy state. + return; + } + + this.transitionIdleIfReady(conn); + } + private transitionIdleIfReady(conn: ContainerControlConnection): void { if (this.isSessionBusy(conn)) { this.markBusy(); return; @@ -814,7 +858,7 @@ export class ContainerControlClient { if (this.busy) { this.busy = false; - this.onSessionIdle?.(); + this.fireSessionIdle(); this.scheduleIdleDisconnect(); } else if (!this.idleTimer) { this.scheduleIdleDisconnect(); @@ -830,7 +874,7 @@ export class ContainerControlClient { private recordCallStarted = (): void => { this.activeCalls++; this.markBusy(); - this.onActivity?.(); + this.fireActivity(); }; private recordCallSettled = (): void => { @@ -892,17 +936,11 @@ export class ContainerControlClient { this.markBusy(); // Renew on every busy tick — this is what keeps a long-lived stream // alive past sleepAfter. - this.onActivity?.(); - } else if (this.busy) { - this.busy = false; - this.onSessionIdle?.(); - this.scheduleIdleDisconnect(); - } else { - // Already idle, no state change. Still ensure the disconnect timer - // is armed (covers the case where we connected but never observed - // any activity). - if (!this.idleTimer) this.scheduleIdleDisconnect(); + this.fireActivity(); + return; } + + this.transitionIdleIfReady(conn); }; private startBusyPoll(): void { @@ -950,7 +988,7 @@ export class ContainerControlClient { // idle transition so the DO's inflight counter doesn't leak. if (this.busy) { this.busy = false; - this.onSessionIdle?.(); + this.fireSessionIdle(); } if (this.conn) { // Weakly stamp the teardown cause so queued RPC calls reject with a @@ -1117,12 +1155,24 @@ export class ContainerControlClient { * queued calls surface it; if the attempt succeeds, the (now-established) * connection is then torn down cleanly with the cause. * + * During this deferral window `this.conn` still points at the connecting + * transport so queued calls can keep their original connection context. + * Container lifecycle code treats `disconnect()` as terminal and does not + * issue more RPC calls after requesting it. + * * `cause` should be a typed `SandboxError` describing why the connection is * being torn down (sandbox stopping, lifetime change, transport switch). */ disconnect(cause?: unknown): void { const conn = this.conn; if (conn?.isConnecting()) { + this.stopBusyPoll(); + this.clearIdleTimer(); + this.activeCalls = 0; + if (this.busy) { + this.busy = false; + this.fireSessionIdle(); + } // Weakly stamp the cause so an in-flight-attempt failure (authoritative) // still wins over this teardown reason. this.stampWeakConnectionCause(cause); diff --git a/packages/sandbox/src/container-control/connection.ts b/packages/sandbox/src/container-control/connection.ts index c02f3f82d..118172b72 100644 --- a/packages/sandbox/src/container-control/connection.ts +++ b/packages/sandbox/src/container-control/connection.ts @@ -244,7 +244,10 @@ export interface ContainerControlConnectionOptions { * with a generic "RPC session was shut down" message on the queued calls * that reject as a result of the abort. * - * Fired at most once per connection attempt. Not fired for `disconnect()`. + * Fired for connection-attempt failures before queued RPC calls are rejected. + * A single logical connect can emit multiple errors as the retry loop observes + * transient failures; the owner should treat the latest value as the current + * best startup cause. Not fired for `disconnect()`. */ onConnectionError?: (error: unknown) => void; /** @@ -448,8 +451,8 @@ export class ContainerControlConnection { } /** - * Run the owner-provided `onConnectionError` callback exactly once per - * failed connection attempt, swallowing any listener errors. + * Run the owner-provided `onConnectionError` callback, swallowing any + * listener errors. */ private fireConnectionError(error: unknown): void { if (!this.onConnectionError) return; @@ -645,9 +648,12 @@ export class ContainerControlConnection { try { await this.startContainer(); } catch (error) { - // Stamp the real cause on the owner as soon as the first - // container-admission failure surfaces — before any retry/abort - // can mask it — so a teardown mid-retry still surfaces this. + // Stamp the converted structured cause on the owner as soon as a + // container-admission failure surfaces — before any retry/abort can + // mask it — so queued calls can surface it. Re-throw the original + // platform error because fetchWithResponseRetry's retry predicate + // matches the platform message, and doConnectInner converts the + // final exhausted error before exposing it to callers. this.fireConnectionError( tryConvertPlatformUnavailable(error) ?? error ); diff --git a/packages/sandbox/tests/rpc-sandbox-client.test.ts b/packages/sandbox/tests/rpc-sandbox-client.test.ts index 73362e620..8bbaa7187 100644 --- a/packages/sandbox/tests/rpc-sandbox-client.test.ts +++ b/packages/sandbox/tests/rpc-sandbox-client.test.ts @@ -12,6 +12,8 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; let stats = { imports: 1, exports: 1 }; let connected = true; +let connecting = false; +let settleConnectionAttempt: (() => void) | undefined; const disconnects: number[] = []; let commandExecuteImpl: (...args: unknown[]) => unknown = () => ({ exitCode: 0, @@ -63,9 +65,17 @@ vi.mock('../src/container-control/connection', () => ({ return connected; } isConnecting() { - return false; + return connecting; + } + async whenSettled() { + if (!connecting) return; + await new Promise((resolve) => { + settleConnectionAttempt = () => { + connecting = false; + resolve(); + }; + }); } - async whenSettled() {} getStats() { return stats; } @@ -97,6 +107,8 @@ describe('ContainerControlClient busy/idle tracking', () => { vi.useFakeTimers(); stats = { imports: 1, exports: 1 }; connected = true; + connecting = false; + settleConnectionAttempt = undefined; disconnects.length = 0; onCloseHandlers.length = 0; onConnectionErrorHandlers.length = 0; @@ -181,6 +193,60 @@ describe('ContainerControlClient busy/idle tracking', () => { expect(onSessionIdle).toHaveBeenCalledTimes(1); }); + it('releases inflight and stops timers when disconnect is deferred mid-connect', async () => { + connecting = true; + connected = false; + commandExecuteImpl = vi.fn( + () => + new Promise(() => { + // Keep the RPC promise pending behind the deferred transport. + }) + ); + const onSessionBusy = vi.fn(); + const onSessionIdle = vi.fn(); + + const client = new ContainerControlClient({ + stub: { fetch: vi.fn() }, + onSessionBusy, + onSessionIdle, + busyPollIntervalMs: 1_000, + idleDisconnectMs: 60_000 + }); + + void client.commands.execute('sleep 10', 'default'); + expect(onSessionBusy).toHaveBeenCalledTimes(1); + expect(vi.getTimerCount()).toBe(1); + + client.disconnect(new Error('sandbox stopping')); + + expect(onSessionIdle).toHaveBeenCalledTimes(1); + expect(vi.getTimerCount()).toBe(0); + expect(disconnects).toHaveLength(0); + + settleConnectionAttempt?.(); + await Promise.resolve(); + await Promise.resolve(); + + expect(disconnects).toHaveLength(1); + expect(onSessionIdle).toHaveBeenCalledTimes(1); + }); + + it('does not let user idle callback errors replace RPC results', async () => { + commandExecuteImpl = vi.fn(() => Promise.resolve({ ok: true })); + const client = new ContainerControlClient({ + stub: { fetch: vi.fn() }, + onSessionIdle: () => { + throw new Error('idle callback failed'); + }, + busyPollIntervalMs: 1_000, + idleDisconnectMs: 60_000 + }); + + await expect( + client.commands.execute('echo ok', 'default') + ).resolves.toEqual({ ok: true }); + }); + it('releases inflight when the WebSocket drops while busy', () => { const onSessionBusy = vi.fn(); const onSessionIdle = vi.fn();