Fix for race condition in RPC connection#799
Open
aron-cf wants to merge 24 commits into
Open
Conversation
aron-cf
requested review from
ghostwriternr,
scuffi and
whoiskatrin
as code owners
July 1, 2026 10:02
🦋 Changeset detectedLatest commit: 66773f6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
commit: |
Contributor
🐳 Docker Images Published
Usage: FROM cloudflare/sandbox:0.0.0-pr-799-66773f6bVersion: 📦 Standalone BinaryFor arbitrary Dockerfiles: COPY --from=cloudflare/sandbox:0.0.0-pr-799-66773f6b /container-server/sandbox /sandbox
ENTRYPOINT ["/sandbox"]Download via GitHub CLI: gh run download 28790266763 -n sandbox-binaryExtract from Docker: docker run --rm cloudflare/sandbox:0.0.0-pr-799-66773f6b cat /container-server/sandbox > sandbox && chmod +x sandbox |
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
andreykrupskii
pushed a commit
to andreykrupskii/cloudflare-sandbox-sdk-issues
that referenced
this pull request
Jul 7, 2026
Follow-up to aron-cf's f553148 on cloudflare/sandbox-sdk#799: - Bump preview SDK to the f553148 build (pkg.pr.new @799) which stamps the container exit code / stop reason onto onStop(). - Forward the SDK's full error `context` (not cherry-picked fields): the create_workspace span JSON-stamps it and promotes each primitive to a filterable `ctx.*` attribute; the HTTP error response returns it and the client prints `context={...}` on FAIL lines. - Set SANDBOX_LOG_LEVEL=debug so the DO emits the "Sandbox stopped" { exitCode, reason } log — the exit code does not survive the DO->Worker RPC boundary, so the DO-side debug log is the only place it is observable (via wrangler tail / Workers Logs). - Truncate non-JSON error bodies in the client so a transient 404 page during deploy rollout no longer dumps HTML into the output. Kept the JWT auth layer and the local Dockerfile. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0184EGvwYGjSB25BZsaXu7fc
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #794
Concurrent sandbox startup (e.g. creating many workspaces from a snapshot at once) intermittently failed, and the errors callers saw were misleading. This PR fixes two related problems on the RPC transport: an idle-disconnect race, and the way container capacity/startup failures are surfaced.
1. Idle-disconnect race disposing the main stub
The RPC client tracks busy/idle by polling capnweb session stats. During concurrent startup those stats can briefly look like the idle baseline (
imports=1, exports=1) while a method promise is still in flight, so the client could arm the idle-disconnect timer and dispose the capnweb main stub before the pending call resolved — surfacing as:Fix: pending RPC method calls are now part of the client's busy/idle state via an
activeCallscounter. A wrapped method increments it before invoking, marks the session busy, and only decrements on settle. The session transitions to idle (and arms disconnect) only whenactiveCalls === 0and capnweb stats are at the idle threshold — preserving the stats-based behavior for long-lived stream exports.2. Capacity/startup failures were masked
When the Containers platform could not admit a container ("There is no container instance…", "Maximum number of running container instances exceeded"), the failure reached callers as a generic
utils.createSessioninterruption — or, later, as a rawRPC session was shut down…string — with no actionable signal.Fixes:
startAndWaitForPorts) before opening the WebSocket, under a short instance-get budget. Each attempt fails fast under capacity pressure and hands back to the connection's own retry loop (exponential backoff within the startup budget) instead of one attempt blocking ~30s and letting the DO be evicted mid-wait. Port readiness still uses the full timeout so a genuinely-booting app isn't cut short.SandboxError: a retryableContainerUnavailableErrorfor capacity/admission failures (carrying the real platform message and areason—no_container_instance_availablevsmax_container_instances_exceeded, via the new exportedContainerUnavailableReason), or anINTERNAL_ERROR-coded error carrying the real cause otherwise. Detection is case-insensitive and realm-safe.OPERATION_INTERRUPTEDwhen the session actually established a connection to a running container and was then interrupted. If the container never started, the underlying typed error is surfaced instead of a masked interruption.Sandbox.containerFetchemits a structuredCONTAINER_UNAVAILABLE503 (preserving the original platform message) for these failures.Caveat
This does not change the platform reality that under
max_instancespressure some concurrent starts can't get an instance. Those now surface as clean, retryableContainerUnavailableErrors across the retry budget rather than misleading interruptions or raw transport strings.