Skip to content

fix(server): never wedge a thread in stopping when sandbox teardown fails - #50

Merged
awtprod merged 3 commits into
mainfrom
impl/sandbox-stop-wedge
Aug 22, 2026
Merged

awtprod merged 3 commits into
mainfrom
impl/sandbox-stop-wedge

Conversation

@awtprod

@awtprod awtprod commented Aug 21, 2026

Copy link
Copy Markdown
Owner

A thread whose sandbox teardown hits any snag gets stuck in stopping forever — every new turn is rejected and the thread is permanently unusable. Three release-blocking lifecycle bugs, all found on this branch and each sufficient to strand a user's thread on its own:

  1. stop() destroyed its own workspace container. The sibling-cleanup phase listed the thread's containers by ID but excluded the workspace container by comparing those IDs against its name — never equal — so every orderly stop force-removed its own workspace container as a "sibling". The later cleanup rm by name then failed, stop() threw, and the stop never completed. Fixed by listing {{.ID}}\t{{.Names}} and excluding by name (the egress sidecar, also removed by name later, is excluded the same way). The per-candidate label verification is unchanged.

  2. The lifecycle reactor early-returned without completing the stop. The decider accepts sandbox.stop from every non-terminal lifecycle — including unprovisioned and failed, which never recorded a runtime — and moves the thread to stopping unconditionally, but the reactor returned early for those threads without dispatching sandbox.stop.complete. Now it always completes: nothing to tear down still dispatches stop.complete, and a teardown failure flows to the existing sandbox.operation.fail path (stage teardown, carrying the runtime's real error), which the decider resolves from stopping to failed — re-provisionable, not wedged. A decider test pins that transition. SandboxSettleCleanupReactor's narrow guard is deliberately left as-is; its comment now records that the trap it worked around is gone.

  3. Stale execution-target cache served dead containers. ProviderCommandReactor's per-thread target cache was never invalidated when a sandbox stopped, expired, or failed, so a cache hit could hand the provider a destroyed container's name. Cache hits are now validated under the provision lock against the current sandbox projection (lifecycle ready, same sandboxId/runtimeRef); anything else evicts and re-provisions. The maps are one tiny entry per sandbox thread, documented as such — staleness, not growth, was the defect.

Every fix ships with regression tests that fail on the reverted code (verified by reverting each fix momentarily): the bug-1 tests stub ps with a non-empty listing — every prior stop test stubbed it empty, which is why this survived — the bug-2 tests assert a completion or an operation.fail is dispatched in both no-container and teardown-failure paths, and the bug-3 test provisions, stops through the real command path, and asserts the next turn re-provisions rather than serving the dead generation's name.

Stacked on #47 (fix/sandbox-graceful-fallback); retarget to main after that merges.

Verification: vp check exits 0, vpr typecheck exits 0, full server suite CI=true pnpm -C apps/server test green (3263 passed, 19 skipped, 329 files).

Claude Fable 5 via Claude Code

@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:L labels Aug 21, 2026
@github-actions

Copy link
Copy Markdown

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

ℹ️ No successful main baseline artifact is available yet. This run establishes the initial measurement.

Provider Metric Main baseline This PR Impact PR ceiling
Codex Total thread wire — 13.6 KiB — 15.1 KiB ✅
Codex Thread snapshot wire — 6.9 KiB — 7.3 KiB ✅
Codex Live turn WebSocket wire — 6.7 KiB — 7.8 KiB ✅
Codex Live turn WebSocket decoded — 54.2 KiB — 66.4 KiB ✅
Codex Live turn messages — 17 — 21 ✅
Claude Total thread wire — 13.7 KiB — 15.1 KiB ✅
Claude Thread snapshot wire — 7.0 KiB — 7.3 KiB ✅
Claude Live turn WebSocket wire — 6.7 KiB — 7.8 KiB ✅
Claude Live turn WebSocket decoded — 55.0 KiB — 66.4 KiB ✅
Claude Live turn messages — 17 — 21 ✅

Baseline: unavailable · PR result: 0888a72 · Source CI: success

Scenario and decoded snapshot size

10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

  • Codex decoded thread snapshot: 107.8 KiB
  • Claude decoded thread snapshot: 108.6 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

@awtprod
awtprod changed the base branch from fix/sandbox-graceful-fallback to main August 22, 2026 02:04
awtprod and others added 3 commits August 22, 2026 02:07
The sibling-removal phase of ContainerSandboxBackend.stop() listed the
thread's managed containers with `ps --format {{.ID}}` and excluded the
workspace container by comparing each listed ID against the workspace
NAME (t3-thread-<hex>). An ID never equals a name, and the listing
filters on exactly the labels the workspace container carries, so every
stop force-removed its own workspace container as a "sibling". The
`rm --force <name>` in #cleanup then failed, stop() collected the
failure and threw, SandboxLifecycleReactor.stop never dispatched
sandbox.stop.complete, and the decider left the thread in `stopping` --
where every thread.turn.start is rejected. A permanently unusable
thread on every orderly stop with a non-empty sibling listing.

List `{{.ID}}\t{{.Names}}` instead (same tab-column parsing as
reconcile's label listing; `--format` prints the plain name on both
docker and podman) and skip entries whose name is one #cleanup removes
by name -- the workspace container and, when configured, the egress
proxy sidecar. The per-candidate label-inspect verification before each
removal is unchanged.

The regression tests stub `ps` with a NON-EMPTY listing -- every prior
stop test stubbed it empty, which is exactly why this was missed -- and
answer whichever format the implementation asks for, so reverting the
fix fails them (verified: 2 failed on the reverted code, 27/27 pass
with it).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The decider accepts sandbox.stop from every non-terminal lifecycle --
including `unprovisioned`, `provisioning`, and `failed`, which never
recorded a runtime -- and moves the thread to `stopping`
unconditionally. The lifecycle reactor's stop handler then returned
early when the sandbox had no docker/podman runtime, without ever
dispatching sandbox.stop.complete: the thread stayed in `stopping`
forever, where every thread.turn.start is rejected. This is the second
independent cause of the stopping wedge, and the trap
SandboxSettleCleanupReactor's narrow ready/paused guard was written
around.

Now the reactor always completes: when the sandbox projection records
nothing to tear down, it skips the teardown and still dispatches
sandbox.stop.complete so the decider can settle the lifecycle. When
teardown IS attempted and runtimes.stop throws, the failure propagates
to the worker's existing catchCause, which dispatches
sandbox.operation.fail (stage `teardown`) carrying the runtime's real
error message -- and the decider already resolves that from `stopping`
to `failed`, a re-provisionable state, so no decider change was needed;
a decider test now pins that transition. The settle-cleanup guard is
deliberately left narrow in this PR; its comment now records that the
underlying trap is gone.

Reactor tests cover both paths: a `stopping` sandbox with no runtime
dispatches exactly one stop.complete (verified failing on the reverted
code), and a teardown failure dispatches operation.fail with the
runtime's own message rather than propagating and wedging.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ProviderCommandReactor's per-thread provisionedTargets cache was only
evicted from a snapshot the caller read before any lock wait, so an
entry naming a container that a stop, an expiry, or a failure had
already destroyed could still be served on a cache hit -- pointing the
provider at a container name that no longer resolves.

Validate at point of use instead: under the per-thread provision lock,
a cache hit is served only if the CURRENT thread.sandbox projection is
`ready` and still names the cached sandboxId/runtimeRef; anything else
evicts the entry and re-provisions. Validating under the lock against a
fresh projection read (rather than the caller's snapshot) also stops a
stale pre-lock snapshot from throwing away a target another fiber just
provisioned.

Neither map needs bounding for memory -- one tiny entry per sandbox
thread this server generation provisioned -- which is now documented at
the declarations; staleness, not growth, was the defect.

The regression test provisions, stops the sandbox through the real
command path, and asserts the next turn re-provisions and hands the
provider the new generation's container name, not the destroyed one
(generation-suffixed names in the harness make the two
distinguishable). Verified failing against an unvalidated bare cache
hit: 3 failed, including this test; 53/53 pass with the fix.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@awtprod
awtprod force-pushed the impl/sandbox-stop-wedge branch from 0888a72 to 567eb73 Compare August 22, 2026 02:08
@awtprod
awtprod merged commit ceb12db into main Aug 22, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant