feat(sandbox): an async, cancellable docker boundary (#479, #394 AC-3)#499
Merged
Conversation
docker.ts only; the consumers do not compile yet. Pushed as recoverable WIP. spawnSync is uninterruptible by construction, so #394 could set a deadline but nothing could interrupt a call in flight - the thread sits inside the syscall with no point at which an AbortSignal could be observed. Now async spawn with an AbortSignal per CALL (not per runner: one runner serves several calls, and cancellation has to reach whichever is in flight). Three kills all arrive as close(null, "SIGKILL"), so the runner records WHICH killer fired before killing rather than inferring after. Abort is typed apart from timeout because they mean different things: a timeout is a wedged operation, an abort is the operator asking to stop, and logging one as the other would put "timed out" in an audit trail for a deliberate Ctrl-C. maxBuffer moved out of the spawn options bag into the runner, because async spawn has no such option - a caller still passing it inside `extra` would have silently lost its bound. That also fixes a latent mis-classification: spawnSync killed the child on a maxBuffer breach, which surfaced as status===null with a signal, the same shape the deadline check read, so an output overflow reported itself as a TIMEOUT with exit 124. An already-aborted signal now refuses to spawn at all, so a cancelled multi-step command cannot still create the next container. Refs #479 Claude-Session: https://claude.ai/code/session_01Y8UPz5RZwgnda3NbAVfd6L
exec.ts carries AC-2: an ABORTED exec now runs the same bounded teardown a timed-out one does. The container is left running whether the deadline fired or the operator pressed Ctrl-C, and an orphaned box is the same leak either way - what differs is only what the report calls it, which is why docker.ts types the two apart rather than collapsing both into timedOut. The teardown stop deliberately carries NO signal. It is the cleanup for a call that was just cancelled, so handing it the same aborted signal would cancel the cleanup too and guarantee the orphan it exists to prevent. Also fixes a bound that the async conversion would otherwise have dropped silently: exec.ts passed maxBuffer inside the SPAWN options bag, which async spawn ignores. It is now the runner's own option, so the 256MB backstop still exists and capBytes stays the authoritative limit. Consumers still mid-conversion; the tree does not typecheck yet. Refs #479 Claude-Session: https://claude.ai/code/session_01Y8UPz5RZwgnda3NbAVfd6L
Both teardown paths deliberately carry NO signal. They run BECAUSE the operation failed or was cancelled, so handing them the aborted signal would cancel the cleanup and orphan exactly the containers and volumes they exist to reclaim. Still converging: destroySandbox and pruneSandboxes need the async boundary next, then registry.ts, cli.ts, and the test doubles. Refs #479
Both are now async, and every dockerRun call inside destroy.ts is awaited. Also carries the same NUL-byte source fix as #498, so the file is greppable on this branch too - it will merge cleanly with that PR since the change is identical. Remaining: registry.ts, cli.ts, claude-inside-cli.ts, and the test doubles. The error count RISES as each layer converts (220 now), because making a function async lights up every caller; it falls to zero only when the boundary reaches the CLI entry, which is already async and already awaited. Refs #479
registry.ts was the last large module: 10 lookups, all of which either call dockerRun or delegate to one that does, now async. labelValue too - the per-object inspect behind listSandboxes. Production type errors: 50 -> 20. Remaining are cli.ts's dispatch, claude-inside, claude-inside-cli, and two stragglers in run/destroy; then the test doubles. Refs #479
The async boundary reached the CLI entry. Handlers is promise-returning across the board, runCli awaits each dispatch arm, and claude-inside-cli's entry now awaits its own promise rather than calling process.exit ON it - exiting on a pending promise would have reported 0 while the container start was still in flight. Production: 0 type errors. Test doubles remain (218): they return plain objects where DockerRun now returns a promise, and deadline.test.ts injects a spawnSync shape where an async child is expected. Refs #479
The injected fakes return a promise now, because DockerRun does. Two shapes needed converting: those annotated as DockerRun, and the inline ones passed straight as an argument, which carry no annotation to key off and were matched on their RunResult SHAPE instead. Every it/test callback became async. Harmless where nothing is awaited, and vitest treats the two identically - safer than deciding per-callback which bodies gained an await and getting one wrong silently. Test type errors 218 -> 44. Remaining: deadline.test.ts, whose spawn seam still injects a spawnSync-shaped return where an async child (an EventEmitter with stdout/stderr streams and a kill that emits close) is now expected. Refs #479
The spawn seam injected a spawnSync-shaped RETURN; the runner now consumes a child process. The double is an EventEmitter with stdout/stderr streams and a kill that emits close(null, signal). That last detail is deliberate and load-bearing. A real SIGKILL surfaces as close(null, signal), IDENTICALLY whether the deadline fired, the caller aborted, or the output cap tripped - which is exactly why the runner records which killer fired rather than inferring it. A double that helpfully labelled its close event would let a broken runner pass, so this one reproduces the ambiguity faithfully. The deadline assertion changed shape with it: it used to read the SPAWN OPTION, which async spawn does not have. It is now asserted through observable behaviour - a child that never settles must still resolve, as a typed timeout, at the bound. Test type errors 218 -> 36. Refs #479
Two patterns the blanket await pass got wrong or left behind. `expect(() => await X).toThrow()` is invalid - an await inside a sync arrow - and the async equivalent asserts on the REJECTION, not on a synchronous throw. Handler calls through the defaultHandlers object are promises now. Test type errors 218 -> 29; production still 0. Refs #479
… tests Last shapes converted: vi.fn() handler mocks (they return promises now), the remaining sync docker fakes, and two await-precedence bugs the blanket pass introduced - `await findSandbox(id)!.containers` awaits the MEMBER, not the call, so it needed `(await findSandbox(id))!.containers`. That one would have typechecked as `any` in a looser config and failed at runtime. Two throw assertions became rejection assertions, which is a real semantic change rather than a syntactic one: `expect(() => await X).toThrow()` cannot work, and the async equivalent asserts the promise REJECTS. For the usage-error case the intent was "returns a code rather than throwing", so it now asserts the promise RESOLVES to a number. Typecheck is clean; the suite has not been run yet. Refs #479
1. `await expect(await createSandbox(...)).rejects.toThrow()` awaits the promise
BEFORE expect can wrap it, so the rejection escaped as a test error instead of
being asserted on. Six tests failed this way - my own blanket await pass put
the await inside expect(). The .rejects form needs the promise itself.
2. A synchronous spawn failure rejected instead of returning a result. spawnSync
reported a failed launch through r.error and the old runner mapped it to
{code: 1}; async spawn normally EMITS an error event, but an invalid argument,
a bad cwd, or a blocked exec can still throw straight out of spawn(). A throw
inside a Promise executor rejects, and nothing in this driver catches - so an
unusable docker would have surfaced as an unhandled rejection rather than the
{code: 1} every caller is written against. Found by reading the diff, not by a
failing test, which is why it is worth naming: no fixture spawns a real
missing binary.
Refs #479
Eight tests covering the acceptance criteria that had none: AC-1 - an in-flight call aborts rather than running to its deadline; the child is KILLED rather than the promise merely resolving around it (resolving while leaving the process alive would look like cancellation and leak a docker client); and an ALREADY-aborted signal refuses to spawn at all, so a cancelled multi-step command cannot still create the next container. AC-2 - an aborted exec stops the container it targeted, exactly as a timed-out one does, and an ordinary non-zero exec still does NOT (escalation destroys a working box; doing it on exit 1 would tear one down every time a command failed). AC-4 - abort and timeout are typed apart. Both arrive as close(null, "SIGKILL"), so this is only true because the runner records which killer fired. Also declares ExecResult.aborted, which the new tests caught: exec.ts SET the flag but never declared it, so no caller could have read it. The pre-spawn guard is proven non-vacuous by mutation: disabling it fails exactly the already-aborted test and nothing else. deadline.test.ts 16/16. Full suite was 291/291 before these were added. Refs #479
The last piece of the cancellation contract. Node's default SIGINT handling ends the CLI immediately, which leaves the box the operator was exec-ing into still running with no one left to reclaim it - the same orphan a timeout left before #394. The entry now aborts instead, so the runner kills its child, returns a typed aborted result, and the command module runs the SAME bounded teardown a timeout gets. A SECOND Ctrl-C exits immediately. The first already asked nicely, and a CLI that refuses to exit is how one earns a kill -9. The signal reaches the command modules through a HANDLERS FACTORY rather than a new runCli parameter, so the Handlers contract is unchanged and every test that injects its own handlers is untouched. Threading it through runCli would have changed that contract for every caller to serve exactly one of them. Installed at the entry, not in the runner: the signal is a property of this process, while docker.ts stays a primitive that observes a signal handed to it. That is what keeps the cancellation path testable without a daemon. Also threads the signal through createSandbox's own docker calls. It was declared on CreateOptions and unused - a dormant seam, which is the thing this issue exists to avoid shipping. Refs #479
destroy.ts conflicted as BINARY rather than textually: the merge base still carried the literal NUL bytes #498 removed, so git refused a line-level merge and left this branch's copy in place. That copy is the correct one - it holds both the NUL fix (applied here too, identically) and the async conversion. Verified rather than assumed: 0 NUL bytes, escapes present, destroySandbox and prune async, and the teardown/lifecycle/deadline suites green at 53/53.
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.
What
The ca-sandbox docker chokepoint moves from
spawnSyncto an asyncspawnthat accepts anAbortSignal, the CLI installs a SIGINT handler that cancels rather than exits, and an aborted call runs the same bounded teardown a timeout does.Closes #479 and, with it, #394's last acceptance criterion.
Why it could not ship with #394
spawnSyncis uninterruptible by construction. A deadline can be set on it — that is what #394 did — but nothing can interrupt the call once in flight, because the calling thread sits inside the syscall and there is no point at which a signal could be observed. Shipping a deadline that looks cancellable and is not would have been worse than the honest gap, so #394 stated it.The user-visible consequence: Ctrl-C killed the CLI and left the container the operator was exec-ing into still running, with nothing left to reclaim it — the same orphan the deadline exists to prevent.
Design
Abort is typed apart from timeout, not folded into it. Both arrive as
close(null, "SIGKILL")and are indistinguishable after the fact, so the runner records which killer fired before killing. Reporting one as the other would put "timed out" in an audit trail for a deliberate Ctrl-C and send a reader hunting a wedged daemon that does not exist.An already-aborted signal refuses to spawn at all. Checking only after the spawn would still create the next container in a cancelled multi-step command, and teardown would then have something to reclaim the caller never knew existed.
Teardown paths deliberately carry no signal. They run because the call was cancelled; handing them the aborted signal would cancel the cleanup and guarantee the orphan they exist to prevent.
The signal reaches the modules through a handlers factory, not a new
runCliparameter — so theHandlerscontract is unchanged and every test injecting its own handlers is untouched.A second Ctrl-C exits immediately. The first already asked nicely; a CLI that refuses to exit is how one earns a
kill -9.Three defects found on the way
exec.tspassed its 256 MB output cap as a spawn option — asyncspawnhas nomaxBuffer, so it would have been dropped without a word. Moved into the runner.spawnSynckilled the child on amaxBufferbreach, producing the samestatus === null+ signal shape the deadline check read — so an output overflow reported itself as a timeout (exit 124, "timed out"). Now its own typed outcome.spawnthrow rejected instead of returning a result.spawnSyncreported a failed launch viar.error→{code: 1}; a throw inside a Promise executor rejects, and nothing here catches. Found by reading the diff — no fixture spawns a missing binary.Plus
ExecResult.abortedwas set but never declared, so no caller could have read it, andCreateOptions.signalwas declared but unthreaded — a dormant seam, which is the thing this issue exists to avoid shipping.Verification
sandbox.js,claude-inside.jswere stale after the runner changed)One test-authoring note worth recording:
await expect(await createSandbox(...)).rejectsawaits the promise beforeexpectcan wrap it, so the rejection escapes as a test error. Six tests failed that way on the first full run — my own blanket await pass put theawaitinsideexpect().Versions
No bump. ca-sandbox 0.1.5 carries no tag, so the payload gate accepts the change on the current version and the entry joins the open section.
Closes #479
Closes #394
https://claude.ai/code/session_01Y8UPz5RZwgnda3NbAVfd6L