feat(runtime-cf): route exec by the container handle, so a run can hold more than one - #135
Conversation
…ld more than one `exec` ran against a client resolved once at Layer build, so the `container` handle every signature in `SandboxService` already carried selected nothing. `acquire` returned the execution's id whatever it was asked for. The reasoning in the comment was circular — one container per execution, therefore one client is enough, therefore a run cannot have two containers. What it cost: `offload-test.stageConcurrency > 1` (#127) was unusable. Turned on for five stages, all five got the same container and raced to wipe each other's checkout — `git clone` clears its target directory first — and died `CheckoutFailed` in under five seconds. `boxFor(container)` resolves per call; `getSandbox` is lazy, so that costs nothing. `acquire({ key })` derives a second id through the SAME normalisation as the execution id, over `<executionId>:<key>`, inheriting the whole preview-URL budget: DNS-safe, <= 40 chars, digested rather than tail-truncated when it does not fit. That last part is load-bearing — preview-sandbox-id.ts documents what tail-truncation cost when a fan-out collided onto one container. `destroy({ container })` is new, because a keyed container is not covered by the dispatcher's end-of-run teardown: that destroys the execution's own id and cannot know what a run named. `offload-test` reaps each stage's container as the stage finishes; the substrate facade REFUSES a key rather than ignoring one, since quietly returning the shared sandbox is the defect this exists to fix. **Both test doubles contradicted the live layer on exactly this property**, which is why a suite of 2200 tests could not catch it. The fake minted a fresh id on every `acquire`, so isolated stages looked isolated; the live layer's `getSandbox` mock ignored its id argument, so routing was unobservable. Both now model it, and two tests pin it: unkeyed acquires are the same container, and `exec` reaches the handle it was given.
There was a problem hiding this comment.
AI code review — 💬 Comment
Risk tier: full · 0 critical · 1 warnings · 0 suggestions
Reviewers: security
1. ⚠️ Warning — Keyed stage containers can leak on failed workspace setup
📍 runs/offload-test.ts:802-805
The keyed container is acquired at the start of the stage, but 'reapStageContainer(stage.label)' is only called on the explicit 'dead', 'unreported', 'red', and 'ok' return paths later in the function. If 'workspace()'/'gitClone'/installation or another earlier stage operation fails, 'runStage' can exit without reaching any cleanup call, leaving the keyed container idle until 'sleepAfter' and incurring the full idle charge. Put acquisition and stage execution in a scoped/finally-style cleanup path so every acquired keyed container is destroyed on failure as well as success.
…four of them pr-review caught the shape. The reap was called from each of the four outcome returns, which is correct exactly as long as nobody adds a fifth — and the cost of forgetting is a container idling out `sleepAfter` on the bill. It also missed interruption outright: a peer stage failing cancels its siblings mid-flight, and those fibers reach no return at all. `Effect.ensuring` cannot be forgotten. It runs after the outcome rather than before, which is safe because everything an outcome carries is already in R2 by then — the container is genuinely finished with. The new test drives the path enumeration missed: a stage that dies before its command ever runs still gives its container back. It uses TWO stages, because concurrency is clamped to the stage count and a single stage is never isolated however high the knob goes.
Closes #131.
Problem & Insight
execran against a client resolved once at Layer build:So the
containerhandle that every signature inSandboxServicealready carries selected nothing, andacquirereturned the execution's id whatever it was asked for. The comment justified it as "V0 = one container per execution" — reasoning that is circular: one container per execution, therefore one client suffices, therefore a run cannot have two containers.What it cost.
offload-test.stageConcurrency > 1(#127) was unusable. Turned on for five stages, all five got the same container and raced to wipe each other's checkout —git cloneclears its target directory before cloning — and diedCheckoutFailedin under five seconds. The knob shipped, was tried, and was reverted the same hour.Why 2,200 tests did not catch it. Both doubles contradicted the live layer on precisely this property:
sandbox-fakefake-container-<n>on everyacquiresandbox-cf.test'sgetSandboxmockgetSandbox: () => currentBox— ignored itsidA fake that is more capable than the thing it stands for does not merely fail to catch a bug; it certifies its absence.
Take
boxFor(container)resolves per call.getSandboxis lazy — the container is provisioned on first use — so per-call resolution costs nothing and makes the handle mean what the contract always said. The cache and artifact layers have always routed bycontainer.id; the sandbox layer now agrees with them.acquire({ key })derives a second id through the same normalisation as the execution id, over<executionId>:<key>. That inherits the whole preview-URL budget: DNS-safe, ≤ 40 chars, and digested rather than tail-truncated when it does not fit — load-bearing, sincepreview-sandbox-id.tsdocuments what tail-truncation cost when a fan-out collided onto one container.destroy({ container })is new. A keyed container is not covered by the dispatcher's end-of-run teardown, which destroys the execution's own id and cannot know what a run named.offload-testreaps each stage's container as that stage finishes; re-deriving the id viaacquireis free and replay-safe, so no handle has to survive a step boundary.Both doubles now model the rule, and two tests pin it: unkeyed acquires return the same container, and
execreaches the handle it was given.Key actions
stageConcurrencystays unset on consumers until someone turns it on deliberately; this PR only makes it possible