fix(core): remove only a container the handle created - #25
Merged
Conversation
`ContainerHandle.remove()` ran `docker rm --force --volumes <name>`, addressed by name and blind to ownership. A container name is shared across processes by design, so carrying the name was never evidence the handle was entitled to a destructive call on it: a handle that adopted another process's live container — or one that never acquired anything — would force-remove it anyway. A failed `onCreate` in one process could take down a live session in another. `acquire()` already branches on create and adopt, so it now reports which path it took, and the handle keeps that alongside its memoised acquisition. Ownership follows the existing latch resets — a failed acquisition is not memoised, and `remove()` clears the latch so the handle may create again — rather than being set once. `remove()` stays idempotent, and an adopted or untouched container resolves without reaching the daemon. Adoption itself is deliberately unchanged: `harness/provider.ts` documents cross-process adoption as the feature that makes a sandbox id resumable. What `acquire()` adopts is a separate decision, owned by #16.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
amondnet
marked this pull request as ready for review
August 28, 2026 19:04
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 2 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…f pinning it `remove()` settles an acquisition still in flight before deciding ownership, and no test reached that path: every case awaited `ready()` first, and a synchronous fake daemon leaves no window to observe. A `docker` that sleeps on `run` opens one, so abandoning a create mid-flight is now pinned to remove what it produced. The expected `rm` argv is derived from `containerName` rather than spelling out its current FNV-1a digest, so a change to the hash reads as a change to the hash instead of as an ownership regression.
This was referenced Aug 29, 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
ContainerHandle.remove()issueddocker rm --force --volumes <name>addressed by name and blind to ownership. Container names are shared across processes by design, so the name alone was never authorisation for a destructive call:onCreatekills process A's live session;ready()at all would still issue therm.The fix is ownership tracking inside
packages/core/src/sandbox/docker/container.tsonly:acquire()already branches on create vs. adopt, so it now returns that distinction ({ name, created }) instead of leaving callers to re-derive it.remove()clears the latch so the handle may create again afterwards.remove()removes only a container this handle created. An adopted one, or a handle that never acquired anything, does nothing and resolves.remove()stays idempotent; a create still in flight is settled first, so an abandoned create is not leaked.Adoption itself is unchanged.
harness/provider.tsdocuments cross-process adoption as the feature that makes a sandbox id resumable, so whatacquire()adopts — and whethercreateSessionshould refuse a taken name — stays with #16. This PR fixes only the destructive half of #18.New tests in
packages/core/test/sandbox/docker/container-ownership.test.tsuse the fake-docker-binary technique already established byprovider.test.ts(tmpdir script +PLEASE_DOCKER_PATH+ a child-process driver, becauseDOCKER_BINis read once at import). They cover: adopted → norm; created →rm --force --volumes <name>; never-acquired → no docker call at all; and idempotency. Removing the guard fails 3 of the 4.Related issue
Closes #18
Checklist
bun run test)bun run lint,bun run type-check)BREAKING CHANGE:note is includedSummary by cubic
Fixes
ContainerHandle.remove()so it only removes a container this handle created, instead of force-removing any container with the same name. Previously, adopting another process's container (or never acquiring one) still randocker rm --force --volumes, which could kill a live session in another process. Now adopted or never-acquired containers are left untouched, andremove()remains idempotent, even when a create is still in flight. Closes #18.Written for commit 06a283c. Summary will update on new commits.