fix(sandbox): free volumes and work dirs when a sandbox is destroyed

[dviejokfs]

Problem

Destroying a sandbox removed its container but left storage behind. On a host that creates and destroys sandboxes — the normal usage pattern — this accumulates until the disk fills, and none of it is reachable through any API.

Two leaks, both on the standalone sandbox path:

1. The Docker home volume, on every delete. create named the volume temps-sandbox-home-{config.run_id} while destroy rebuilt the name by stripping the prefix off the container name. Those agree for agent runs (temps-sandbox-<run_id>) — which is exactly why this stayed invisible. But standalone sandboxes override the container suffix with their opaque public_id label, so destroy asked Docker to remove temps-sandbox-home-<hex>, a volume that had never existed. The 404 was swallowed as tracing::warn!("...may not exist") and the real volume stayed on disk permanently.

That single remove_volume call was the only one in the entire backend.

2. The host work dir, on every delete. create_sandbox makes data_root/<public_id> and bind-mounts it to /workspace. Nothing ever removed it — there was no remove_dir_all anywhere in temps-sandbox. Every destroyed sandbox stranded its full working tree: node_modules, build output, cloned repos.

Fix

Volume naming. Both sides now go through one DockerSandboxProvider::sandbox_names, which returns the container name and the home volume name together; destroy re-derives the volume from the container name via the same home_volume_name. They cannot drift again.

This also closes a latent cross-tenant collision: standalone sandbox row 5 and agent run 5 previously shared temps-sandbox-home-5, meaning one sandbox could read another user's shell history, ~/.claude credentials, and project files. 16-hex labels can never equal a decimal run_id, so the collision is now structurally impossible.

Work dir. Removed on destroy and on both create-failure paths. The seed-source failure arm matters most: the directory can already hold a full clone, and the row is marked destroyed, so no later destroy_sandbox could ever reach it. Guarded by a sbx_<16 hex> shape check via work_dir_to_remove, because data_root.join() silently resolves .. and absolute paths and this is a recursive delete.

Volume label. Home volumes are stamped sh.temps.sandbox.home at create. Nothing in the server reads it — it exists so an operator can reclaim volumes stranded by older builds:

docker volume prune --filter label=sh.temps.sandbox.home

prune only touches volumes no container references, and a label can't collide with something the operator created themselves.

What this PR deliberately does not do

An earlier revision added a background sweep to reclaim already-leaked storage. Three review rounds found three different ways for it to delete the wrong thing:

1. It deleted a live sandbox's home volume during the image pull that create opens by force-removing the old container first — minutes wide on a cold pull.
2. The fix for that still deleted any volume belonging to a second temps instance sharing the same Docker daemon, since a foreign instance's volumes can never appear in this instance's claim set. That's the normal local dev setup: several worktree slots, each with its own database.
3. The work-dir sweep had the same shape. sandboxes rows are ON DELETE CASCADE from users, so deleting a user hard-deletes the rows while the containers keep running — and the next sweep would recursively delete live workspaces.

The pattern is the design, not the details. A sweep has to infer which storage is garbage; an explicit destroy is told. Each predicate we added — dangling, labelled, name-shaped, unclaimed — narrowed the wrong answers without making them impossible, and the failure mode is silent and irreversible.

Reclaiming the existing backlog is now an operator action with the prune command above. A safer automatic sweep can be designed separately, with instance identity and a provider cross-check as requirements rather than afterthoughts.

Evidence

Run against a live Docker daemon, not mocks.

The e2e fails on the old naming. Reverting just the create-side line:

thread 'sandbox::docker::tests::destroy_frees_the_home_volume_of_a_standalone_sandbox' panicked:
home volume should exist while the sandbox does: DockerResponseServerError { status_code: 404,
message: "get temps-sandbox-home-purge-test-c0ffee01: no such volume" }
test result: FAILED. 0 passed; 1 failed

That 404 is the bug — create had made temps-sandbox-home-99993 instead. Restored:

test sandbox::docker::tests::destroy_frees_the_home_volume_of_a_standalone_sandbox ... ok
test sandbox::docker::tests::sandbox_names_agree_for_standalone_and_agent_run ... ok

The leak was real on the dev host. Eight temps-sandbox-home-* volumes had accumulated, five of them referenced by no container at all.

Suites:

$ cargo test --lib -p temps-agents -p temps-sandbox
test result: ok. 258 passed; 0 failed; 1 ignored
test result: ok. 109 passed; 0 failed; 0 ignored

$ cargo clippy -p temps-agents -p temps-sandbox --all-targets -- -D warnings
Finished `dev` profile

Tests added

• destroy_frees_the_home_volume_of_a_standalone_sandbox — Docker e2e. Asserts on the volume, not the container, since the container was always removed correctly. Also asserts the numeric-named volume was never created, so it can't pass by accident.
• sandbox_names_agree_for_standalone_and_agent_run — unit, no Docker. The regression guard that works on CI runners without a daemon; the first revision's unit tests all stayed green on the buggy code, which a review caught.
• home_volume_names_carry_the_documented_prefix — unit.
• work_dir_to_remove_targets_the_directory_create_allocated / ..._refuses_ids_that_escape_the_data_root — unit; the traversal guard in front of the recursive delete.

Docker-gated tests skip gracefully with no #[ignore], per project policy.

Upgrade note

Sandboxes created before this change keep their old temps-sandbox-home-<row.id> volume while their container exists, and it becomes reclaimable once they're destroyed. The cross-tenant collision described above is closed for new sandboxes but not remediated for existing ones — recreating them is the operator action. Worth a release note.

No API surface changed, so no @temps-sdk/cli parity is needed.

---

Post-review round 3 (commit 076abddc)

A third review pass — security audit + test-coverage audit — found one security issue and a test that passed for the wrong reason.

Security: generated names could re-enter the pre-fix namespace

The naming fix wasn't retroactive in a way that mattered. On an upgraded host, a pre-fix standalone sandbox's volume is stranded (destroy computes the new name and misses) but stays on disk as temps-sandbox-home-<sandboxes.id>. Agent runs still name theirs temps-sandbox-home-<agent_runs.id>, and Docker attaches an existing volume by name — so the next agent run whose id matched would silently mount a previous standalone sandbox's /home/temps: another user's Claude credentials, shell history and project state, read-write.

Same root cause as the original collision: two independent id sequences sharing one namespace. Every name this build generates now carries a v2- scheme marker, so it can't land on a pre-fix name and stranded legacy volumes are inert. Cost is one fresh home for a container recreated across the upgrade — agent-run homes are ephemeral, and standalone containers aren't recreated in place (stop/start/restart reuse the container).

Other fixes

• My reclaim command was wrong. Legacy volumes were created implicitly by the bind and carry no label, and create_volume doesn't retro-apply one — so the label-filtered prune matched none of the volumes I advertised it for. Documented the prefix + dangling=true command for those.
• The provider-create failure arm deleted the work dir without tearing the container down. create can fail after start_container (ownership normalisation propagates its error) and containers are restart-unless-stopped, so it could leave a live container holding the caller's env vars, unreachable once the row is destroyed, with its /workspace deleted underneath it. Now destroys first, matching the seeding arm.
• Moved the labelled create_volume to just before create_container, so a caller looping failed creates with a bogus image can't mint a volume per attempt.
• Dropped a stale comment promising the (reverted) reaper would clean up.

Tests

The coverage audit's finding: nothing covered the call sites, only the pure helpers. Deleting the remove_work_dir calls left every test green. Each new test below was verified by neutering the production code and watching it fail.

• storage_cleanup_tests (no Docker, MockDatabase + fake provider): destroy removes the work dir; still removes it when the provider destroy fails; leaves agent-run dirs alone; neither create-failure arm strands one. Uses a real nested work dir, so a recursive→shallow delete regression also fails. Verified: neutering the 3 call sites turns 4 of 5 red; hoisting cleanup above the agent-run early return turns the 5th red.
• create_binds_mount_the_volume_destroy_will_remove (no Docker): pins the bind create actually hands Docker — where the original leak lived. Re-inlining the run_id name previously passed everywhere except an e2e that skips without a daemon.
• sandbox_service_and_provider_agree_on_volume_naming: pins the cross-crate half — the service picks the container label, the provider turns it into a volume name, nothing typed connects them.
• generated_names_never_reenter_the_pre_fix_namespace: the guard for the security fix above.
• The e2e now asserts the volume carries the label and that the container actually mounts it. Without the mount assertion, pre-creating the volume made "the volume exists" pass on its own merits — dropping the bind entirely went undetected. Verified: dropping the bind now fails with the real mount list. It also gained the temps serve skip guard its siblings have, pre-cleans both volume names, and captures observations before teardown so a failure can't leak a container onto the host.

On that last point — the guard fired for real during this work: a container leaked by an earlier deliberately-failed run was still running and correctly caused a skip.

$ cargo test --lib -p temps-agents -p temps-sandbox
test result: ok. 260 passed; 0 failed; 1 ignored
test result: ok. 115 passed; 0 failed; 0 ignored

$ cargo clippy -p temps-agents -p temps-sandbox --all-targets -- -D warnings
(clean)

Deliberately not added

A full Postgres + Docker integration test was considered. sandbox_service_and_provider_agree_on_volume_naming covers the same cross-crate identity agreement deterministically, in microseconds, with no daemon or container — and the existing e2e already proves the Docker half. The integration test's unique remaining value was small relative to the CI weight and flakiness it would add.

[github-actions]

📓 Changelog preview

This is what your commits will add to the generated CHANGELOG.md at release time (via git-cliff). Do not edit CHANGELOG.md by hand — it is generated from your Conventional Commit messages.

## [Unreleased]

### Documentation

- **sandbox:** Record the ordering the work-dir reap depends on

### Fixed

- **sandbox:** Free volumes and work dirs when a sandbox is destroyed
- **sandbox:** Only reap volumes no live sandbox claims
- **sandbox:** Keep generated volume names out of the pre-fix namespace

### Revert

- **sandbox:** Drop the background orphan-volume reaper

### Testing

- **sandbox:** Adapt the storage-cleanup harness to the cookie_crypto dependency