fix(sandbox): harden session lifecycle and retire the one-shot lane - #3247
Merged
Conversation
…weep The session registry is a per-replica cache, but every route handler answered 404 from the cache alone and the only backend→registry path (adoptExisting) ran once at boot. A session owned by a peer replica, or missed by a boot-time `docker ps`/apiserver blip, was unreachable for the life of the process: the platform read the 404 as a phantom session and recreated one that was alive elsewhere, and nothing ever reaped the orphan. - Every handler now goes through ensureRegistered: on a miss it lists the backend, adopts a RUNNING match (stopped objects stay a genuine 404 — that is the resume signal) and routes to it. - adoptExisting runs on every sweep tick (not while draining), logs a skipped adoption instead of swallowing it, adopts running objects only, and heals the build cache for newly adopted orgs only. - Both listSessions THROW on a failed list instead of returning [] (K8s under withRetry), so a blip is logged and retried rather than read as "no sessions". Findings: sandbox-svc-session-1, sandbox-svc-session-5.
…ce delete K8s createSession caught ANY Secret/Pod create error and ran the failed- create cleanup — on a 409 that deleted the RUNNING peer Pod + Secret that caused the conflict (and, on the fresh path, a PVC a concurrent ensure may own). A create that lost the deterministic-name race must never destroy the winner; the Docker backend already refuses this. Now a Secret 409 throws without any cleanup and a Pod 409 removes only the Secret this call created (leaving it would 409 every future create). destroySession swallowed the workspace-deletion failure on both backends (Docker rm of the host dir, K8s PVC delete), so DELETE answered destroyed:true while the user's data and its storage survived with no retry and no sweeper. Both now throw (the PVC delete under withRetry); the route already maps a throw to 502 destroyed:false and the platform retries. The C4 test used a 409 as its failure stimulus and pinned the wrong behaviour by accident; it now uses a definitive 400. Findings: sandbox-svc-session-2, sandbox-svc-session-6.
…s too The K8s Pod builder applied DinD (runAsUser 0, seccomp Unconfined, privileged on runc, TALE_DIND=1, the docker-storage mount) to EVERY profile, while the Docker argv builder gates it on the agent profile: a `default` (run_code / crawler render) Pod ran untrusted content as root — or never became ready, because the entrypoint's DinD branch drops to uid 10001 on a 65534-group workspace. And only the Docker builder emitted TALE_BROWSER_CDP, so with SANDBOX_BROWSER_VIEW on (the default) no Pod ever started the headed Chromium: the live-browser pane, browser restart/reset and the in-sandbox Playwright MCP CDP attach were silent no-ops on Kubernetes. Both gates now live in one backend-neutral module (session/session-profile.ts) used by both builders. Findings: sandbox-svc-session-3, sandbox-svc-session-4.
handleCreate compared only the registry against maxSessions and maxSessionsPerOrg, but the registry is populated only after backend.createSession resolves — up to createHealthTimeoutMs later through a slow image pull. A burst of distinct ids during that window all passed the caps and oversubscribed the host by the number of creates in flight (each agent session up to 2 cpu / 4 GB). `creating` is now a Map<sessionId, organizationId>; both caps add its size (per org for the org cap) to the registry count. A failed create still releases its share in the finally. Finding: sandbox-svc-session-7.
wire.ts documented the field as runnerd's activity clock, but toInfo always sent createdAtMs — the registry has no activity clock (the real one is runnerd's health.lastActivityAtMs, consulted only by the sweep) — and nothing on the platform read it. A wire field that is both wrong and unread misleads the first consumer who trusts its name, so it goes: from the spawner wire type, the platform mirror type, its test fixture and the integration-check mocks. Finding: sandbox-svc-session-9.
validate-session.ts re-declared RUNNERD_ENV_MAX_VALUE_BYTES as its own MAX_ENV_VALUE literal, and nothing kept the two hand-mirrored protocol copies (the spawner's canonical runnerd-protocol.ts and the daemon's protocol.ts, both excluded from knip on purpose) in sync — a cap changed on one side drifted silently. The validator now imports the constant, the canonical copy gains the RUNNERD_CONSUMER_BUFFER_MAX_BYTES only the mirror carried, and runnerd-protocol.test.ts imports both copies and fails on any constant that differs or exists on one side only. Removed the public surface only unit tests exercised: ExecManager.has() and extendDeadline() (the "platform keepalive" the docblock promised was never built — attach() is the only deadline refresh, and the tests now poll status()), resolveTier() (the accessors cover the table), and ControlRoutes.drainStartedAt (the linger anchor is observable through takeLingerReap, which the tests now use with an injected clock). Findings: sandbox-svc-session-8, sandbox-svc-runtime-8, sandbox-svc-runtime-9, sandbox-svc-runtime-10.
…mage entrypoint.sh still carried ~160 lines implementing the per-call `<language> <packages.json> <options.json> <entry>` protocol (install python/node deps, PHASE markers, run the entry) that no producer emits: every session launcher passes `daemon`, the K8s pod adds `egress-sidecar`, and the /v1/execute route is gone. The dispatch was not fail-closed either — any other argv fell through into that install/run lane. The tail now exits 65 with a clear message; the script header, docker-entrypoint.sh, the Dockerfile header and the README describe the two real dispatches. jq stays (the hook scripts use it); only its comment changes. The DinD doc's "one-shot never gets docker" wording becomes the real rule: the default profile never does. Finding: sandbox-svc-runtime-1.
SANDBOX_DEFAULT_TIMEOUT_MS, SANDBOX_OUTPUT_FILE_MAX_BYTES and SANDBOX_OUTPUT_TOTAL_MAX_BYTES were parsed (boot-fatal on a bad value) and bound nothing; SANDBOX_CACHE and SANDBOX_SPAWNER_IMAGE were parsed and documented — SPAWNER_IMAGE as "required" for stage/harvest containers that no longer exist, CACHE=pvc as mounting per-org cache PVCs no code creates. All five fields leave SpawnerConfig, loadConfig and the test fixtures. docs/kubernetes.md described the retired Pod-per-exec model end to end; it now documents the session model, the env the backends actually read (SANDBOX_K8S_CACHE_STORAGECLASS as the workspace-PVC storage class), and a Role that matches the code — including the persistentvolumeclaims `delete` the destroy path needs (a cluster wired from the old snippet leaked one PVC per destroyed session) and the networkpolicies verbs init uses. sessions.md drops its "per-org cache PVCs" reference and the claim that /v1/execute still exists. Findings: sandbox-svc-runtime-2, sandbox-svc-runtime-3, sandbox-svc-runtime-4.
stageFiles fetched every URL item with no AbortSignal or deadline of its own: the only bound was undici's 300 s defaults, and the body timeout is per-chunk idle, so a trickling blob server was unbounded. The spawner aborts its side at 30 s — ten times sooner — and that abort never reaches the daemon's outbound fetch, so the handler and its accumulated buffers (up to 100 MiB) stayed alive long after the platform had been told "timeout", and every later item in the batch waited behind the stall. Each URL fetch now runs under a 25 s AbortController covering headers and body; a stalled item is reported as skipped with reason 'timeout' and the batch moves on. Finding: sandbox-svc-runtime-5.
The spawner accepted 8 MiB request bodies (SANDBOX_MAX_REQUEST_BODY_BYTES, "headroom for growth") and forwarded the same stage payload to runnerd, whose private readBody capped at 4 MiB and lumped the overflow in with malformed JSON as 400 bad_request. Any client using the advertised headroom failed staging with a misleading error. RUNNERD_MAX_REQUEST_BODY_BYTES (8 MiB) is now a protocol constant on both sides; runnerd's routes read bodies through one helper that answers 413 payload_too_large for an oversize body and 400 only for malformed JSON; and the spawner's cap defaults to, and is clamped at, the daemon's (a higher operator value warns and clamps), so a body the spawner accepts can never be refused downstream. Finding: sandbox-svc-runtime-7.
runDocker armed its kill timer only when a caller passed timeoutMs, and the /health probe (`docker version`), the periodic sweep's listings, the cache-volume setup on the session create path and the buildkitd bring-up passed none. Under the exact failure the healthcheck exists to detect — a wedged dockerd — the spawner spawned one hung child per 10 s healthcheck plus one per sweep tick, and session create blocked in ensureCacheVolume with no bound; the probe cached only after resolving, so concurrent healthchecks each forked their own probe, and the 5-min sweep had no overlap guard. runDocker now defaults to a 60 s budget (Infinity opts out); the probe is 5 s, the listings 15 s, the image-pulling runs 120 s; concurrent /health hits share one in-flight probe (health-probe.ts); and a sweep tick that overlaps a running sweep skips with a warning. Finding: sandbox-svc-runtime-6.
runnerd-protocol.test.ts now imports the canonical copy whole, so knip sees every export used and reported its ignore entry as redundant. The daemon mirror stays excluded (its spawner-only members would otherwise read as dead). Companion to sandbox-svc-session-8.
Commit 3a254ab gave runDocker a 60 s default and ensureImage's boot-time `docker pull` of the runtime image passed none, so the pull inherited it. The image is multi-GB (LibreOffice + Playwright/Chromium + Node + Python); on any host without a `tale deploy` pre-pull the CLI was SIGKILLed at 60 s on all three attempts — which cancels the daemon-side pull — and the first session create then died on its own `docker run -d` bound. Sessions could not start until someone pulled by hand. Each pull attempt now carries IMAGE_PULL_TIMEOUT_MS (30 min: the slow-link ceiling, still a real bound so a wedged daemon cannot pin boot forever); `image inspect` keeps the default. The default's docblock names the pull as the one routine opt-out. spawn-util.test.ts pins the pull budget an order of magnitude above the default and the inspect-miss → single-pull path against a recording fake docker. Review of PR #3247, blocking item 1.
1a5f022 made ensureRegistered re-resolve a registry miss against the backend and adopt a running match — with no drain check, while the sweep tick in server.ts (same PR) deliberately skips adoptExisting during a drain because a lingering spawner must never adopt the sessions its replacement is creating. The route path re-opened that hole: an exec/GET/ status for a replacement-created session landing on the lingering replica (shared network alias / VIP during the roll) was adopted, inflated the /v1/drain-status sessionIds so the deploy kept lingering, and at max-linger stopAllSessions() stopped a live session another replica owns. SessionRoutes takes an isDraining hook (default: never); server.ts wires it to controlRoutes.isDraining. A miss while draining answers 404 without listing the backend at all — the replacement serves it. Regression test: a listed ready session + draining → 404 / nothing adopted / no list; the same instance not draining → 200. Review of PR #3247, blocking item 2.
Two review notes on the registry-miss path and create ordering: - ensureRegistered ran one full backend list (`docker ps -a` / pod list) per concurrent miss; the platform probes sessionIsAlive + exec-status per turn, so a stopped session cost one list per probe. Concurrent misses now share the one list in flight (health-probe.ts shape, without the TTL — a resolve must see the current backend state). - handleCreate checked the host/org quotas before the in-flight duplicate check, so a retried create of an id still in `creating` answered 429 session_quota instead of 409 when the host was at cap. The duplicate check now precedes the quotas. Tests: three probes across two ids share one list and a later miss lists again; a retry of an in-flight id at maxSessions=1 is 409 duplicate. Review of PR #3247, non-blocking notes 3 and 4.
The round-1 in-flight dedup (6f39075) let a late miss join the `docker ps` / pod list already in flight. That list is a snapshot taken when it STARTED — before the late caller's session may have existed. In the multi-replica setup session-1 exists for (create on replica A, the platform's next exec-status/attach on replica B while B is listing for an unrelated miss) the joiner did not find the fresh session, answered 404, and the platform mapped it to {kind:'gone'} and finalized the turn on a phantom, although the session was alive on A. Keep the dedup but coalesce every miss that arrives during an in-flight list into ONE follow-up list that starts once the current one settles (success or failure). Every caller is answered by a snapshot taken after its own miss; at most two lists are ever in flight. Tests: three synchronous probes → two lists (one + one shared follow-up), not three and not one; regression: a session pushed into the backend while a list is in flight is 200 from the probe that arrived during that list (404 before this change). Repair review of PR #3247, blocking-1 (round 2).
Deleting `{ timeoutMs: IMAGE_PULL_TIMEOUT_MS }` from ensureImage kept the
suite green: the round-1 test pinned the constant and the argv path
through a fake docker binary, not the opts the pull was called with.
ensureImage takes an injectable runner (`run`, defaults to runDocker);
the test records every call and asserts `image inspect` carries no
timeout (the default) and `pull` carries IMAGE_PULL_TIMEOUT_MS. The
tmp-dir fake binary is gone with it. Mutation-checked: dropping the
budget fails the test.
Repair review of PR #3247, nonblocking-1 (round 2).
Refusing an oversize body by throwing inside `for await (const chunk of req)` runs the iterator's return(), which destroys the IncomingMessage: under Bun 1.3.12 (the pinned CI runtime) the 413 the route writes next is lost and the client reads a 200 — the PR's own test failed there — and on Node it aborts the socket. The body now comes off the `data` event; a declared oversize Content-Length is refused before the first byte, a running total past the cap pauses the stream, and the route's 413 goes out with `Connection: close` so the unread remainder is dropped with the socket. Verified 413 on Node 22, Bun 1.3.12 and Bun 1.4.0, with and without Content-Length (a chunked oversize case is now pinned).
This was referenced Sep 6, 2026
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.
Summary
Hardening of the sandbox spawner (
services/sandbox) and the runtime image / runnerd (services/sandbox-runtime) from the backend deep-review campaign, theme sandbox-service. Sixteen atomic commits; every finding in the theme is fixed, none skipped. The last four commits address the adversarial review (two blocking items + two cheap notes) — see Review repair below.listSessionsthrows on a failed list instead of laundering it into "no sessions".destroyed:truewhile the user's data lives on.defaultPod is never root/privileged; agent Pods now actually getTALE_BROWSER_CDP).dockercall carries a budget (the boot-time image pull its own 30 min one); the health probe dedups concurrent callers; the periodic sweep has an overlap guard.Findings fixed
ensureRegistered); stopped objects stay a genuine 404; a draining spawner never adopts on a miss (same rule as the sweep tick); concurrent misses share one backend list.session/session-profile.ts.TALE_BROWSER_CDP=1for agent sessions when the browser view is on.adoptExistingruns on each sweep tick (not while draining), logs skipped adoptions, adopts running objects only; bothlistSessionsthrow on a failed list.rmof the workspace and the K8s PVC delete (underwithRetry) throw; the route maps that to 502destroyed:false.creatingis a Map<id, org>; both caps add creates in flight; a retry of an id still being created is judged a 409 duplicate before the quotas.validate-session.tsusesRUNNERD_ENV_MAX_VALUE_BYTES;runnerd-protocol.test.tspins both protocol copies equal; the canonical copy gainsRUNNERD_CONSUMER_BUFFER_MAX_BYTES.SessionInfo.lastActivityAtMsremoved from the wire type, the platform mirror and the itest mocks.defaultTimeoutMs,outputFileMaxBytes,outputTotalMaxBytesand their envs removed.SANDBOX_CACHE/SANDBOX_SPAWNER_IMAGEremoved; kubernetes.md documents the session model andSANDBOX_K8S_CACHE_STORAGECLASS.delete, session-Secret verbs, networkpolicies verbs; sessions.md points at it.stageFilesruns each URL fetch under a 25 s AbortController (headers + body); a stall isskipped: timeout.runDockerdefault 60 s budget; health 5 s, listings 15 s, image-pulling runs 120 s, the boot-timedocker pullof the runtime imageIMAGE_PULL_TIMEOUT_MS= 30 min per attempt;health-probe.tsshares in-flight probes;makeSweepTickoverlap guard.RUNNERD_MAX_REQUEST_BODY_BYTES(8 MiB) on both sides; runnerd answers 413payload_too_large; the spawner's cap is clamped to it (warns).ExecManager.has()/extendDeadline()removed; tests pollstatus().resolveTier()removed.ControlRoutes.drainStartedAtremoved; tests observe the anchor throughtakeLingerReapwith an injected clock.Skipped
None.
Review repair (commits 13–16)
570a273e6):ensureImage'sdocker pullnow carriesIMAGE_PULL_TIMEOUT_MS(30 min per attempt — the multi-GB runtime image on a slow link; still finite so a wedged daemon cannot pin boot forever);image inspectkeeps the default; the default's docblock names the pull as the one routine opt-out.spawn-util.test.tspins the pull budget ≥ 10× the default and the inspect-miss → single-pull path against a recording fake docker.d8a99ee6d):SessionRoutes(cfg, backend, isDraining = () => false);server.tspasses() => controlRoutes.isDraining.ensureRegisteredanswers 404 on a miss while draining without listing the backend. Regression test: listed ready session + draining → 404 /sessionCount()0 /sessionIds()[] / 0 lists; the same instance not draining → 200. A mutation check (guard removed) made the test fail.6f39075fc): concurrent registry misses share the onelistSessionsin flight (health-probe.tsshape, no TTL);handleCreatejudges the in-flight duplicate (409) before the quotas (429). Tests for both.b6c179eb0):services/sandbox-runtime/package.jsondescription no longer names the one-shot lane.creationTimestampolder thancreateHealthTimeoutMswith no Pod), which is its own change with its own test; the window is a crash window and the previous behaviour (destroy the peer) was worse. Follow-up.makeSweepTickexported for its test) — left as is.Tests & gates observed
services/sandbox:bunx tsc --noEmitexit 0 ·bunx oxlint --type-awareexit 0 ·bun test:290 pass / 0 fail — Ran 290 tests across 21 files(after the repair; 285 before)services/sandbox-runtime/daemon:bunx tsc --noEmitexit 0 ·bunx oxlint --type-awareexit 0 ·bun test:90 pass / 0 fail — Ran 90 tests across 11 filesservices/platform:bunx tsc --noEmitexit 0 ·bunx oxlint --type-awareexit 0 ·bunx vitest --run --project server:Test Files 519 passed (519) · Tests 6101 passed (6101)bun run check(re-run after the repair): exit 0,Tasks: 40 successful, 40 total(platformTest Files 527 passed (527) · Tests 73340 passed (73340); test:uiTest Files 456 passed (456) · Tests 3512 passed (3512)).bun run knip:check(re-run after the repair): exit 0 — one pre-existing hint (cron-parserinignoreDependencies).run-itest.sh(re-run after the repair):[itest] 475/476 checks passed across 134/134 lanes— noRUN TRUNCATED; the single failing check is the pre-existingwebdav re-homelane (webdav theme, red on main).git merge-tree --write-tree origin/main HEAD: CLEAN (main = 3997f6a).Notes for the reviewer
docker ps/listNamespacedPod, 10 s / apiTimeout bounded) on a miss — never while draining, and concurrent misses share one list. A platform probing a stopped session pays one list per turn before the resume create — cheap, and the only way to get the fullBackendSessionrecord.destroySessionon retry: after a thrown workspace rm the container is already gone;removeContainer/clearPinMarker/rm --forceare idempotent, and the route restores the registry entry so the platform's retry hits the same path.SANDBOX_MAX_REQUEST_BODY_BYTESabove 8 MiB keeps booting.runDockerdefault 60 s: callers that passed no timeout before now get 60 s; the twodocker run -dbring-ups (buildkitd/mirror) and the busybox chmod get 120 s because they may pull; the boot-time runtime-image pull gets 30 min per attempt.Infinitystill opts out.