From b2d0c74d6e1516441742e79033f8dbddb28e0298 Mon Sep 17 00:00:00 2001 From: Minsu Lee Date: Fri, 28 Aug 2026 23:19:48 +0900 Subject: [PATCH 1/2] fix(core): remove only a container the handle created MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ContainerHandle.remove()` ran `docker rm --force --volumes `, 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. --- packages/core/src/sandbox/docker/container.ts | 46 ++++++-- .../docker/container-ownership.test.ts | 106 ++++++++++++++++++ 2 files changed, 145 insertions(+), 7 deletions(-) create mode 100644 packages/core/test/sandbox/docker/container-ownership.test.ts diff --git a/packages/core/src/sandbox/docker/container.ts b/packages/core/src/sandbox/docker/container.ts index 31924af..773bf7f 100644 --- a/packages/core/src/sandbox/docker/container.ts +++ b/packages/core/src/sandbox/docker/container.ts @@ -115,6 +115,18 @@ async function adopt(name: string, status: string): Promise return name } +/** + * How the container behind a handle came to exist. + * + * `created` is what tells a later teardown whether it is allowed to destroy this container. + * `acquire` already branches on the two cases, so it reports which one it took rather than + * leaving the caller to re-derive an answer only this function ever knew. + */ +interface Acquisition { + name: string + created: boolean +} + /** * Create the container, or adopt one that already carries this name. * @@ -128,12 +140,12 @@ async function adopt(name: string, status: string): Promise * cancelled command) leaves nothing to adopt, and the next `ready()` must be free to create * from scratch rather than keep starting a name the daemon has never heard of. */ -async function acquire(name: string, options: ContainerOptions): Promise { +async function acquire(name: string, options: ContainerOptions): Promise { const existing = await containerStatus(name) if (existing !== undefined) { const adopted = await adopt(name, existing) if (adopted !== undefined) { - return adopted + return { name: adopted, created: false } } } try { @@ -146,12 +158,12 @@ async function acquire(name: string, options: ContainerOptions): Promise const raced = await containerStatus(name) const adopted = raced === undefined ? undefined : await adopt(name, raced) if (adopted !== undefined) { - return adopted + return { name: adopted, created: false } } } throw cause } - return name + return { name, created: true } } export interface ContainerHandle { @@ -160,7 +172,15 @@ export interface ContainerHandle { readonly ready: () => Promise /** Host address an exposed container port is reachable at, as `host:port`. */ readonly hostAddress: (port: number) => Promise - /** Remove the container and everything on it. Idempotent. */ + /** + * Remove the container and everything on it, if this handle is the one that created it. + * Idempotent. + * + * A handle that adopted an existing container, or that never acquired one at all, removes + * nothing and resolves. `docker rm` is addressed by name, and a name is shared across + * processes by design — so carrying the name is not evidence that this handle is entitled + * to a destructive call on it. Only having created the container is. + */ readonly remove: () => Promise /** * The container name if it is already running, without creating or starting anything. @@ -177,15 +197,20 @@ export function createContainerHandle( options: ContainerOptions & { prefix?: string }, ): ContainerHandle { const name = containerName(sandboxId, options.prefix) - let acquisition: Promise | undefined + // Ownership lives on the latch rather than beside it: `remove` clears the latch so the + // handle may create again afterwards, and a claim to a container that no longer exists + // would otherwise outlive the acquisition that earned it. + let acquisition: Promise | undefined - const ready = (): Promise => (acquisition ??= acquire(name, options).catch( + const acquireOnce = (): Promise => (acquisition ??= acquire(name, options).catch( (cause: unknown) => { acquisition = undefined throw cause }, )) + const ready = async (): Promise => (await acquireOnce()).name + return { name, ready, @@ -202,7 +227,14 @@ export function createContainerHandle( return `127.0.0.1:${line.slice(separator + 1)}` }, remove: async () => { + // Settle an acquisition still in flight before deciding: a create this handle started + // and then abandoned is exactly the container it is responsible for removing. A + // failed one owns nothing, which is also what an untouched handle reports. + const owned = await acquisition?.then(result => result.created, () => false) ?? false acquisition = undefined + if (!owned) { + return + } const result = await runDocker(['rm', '--force', '--volumes', name]) // A container that was never there is the state `remove` promises, so that one result // is success. Anything else leaked a container, and a silent resolve would hide it. diff --git a/packages/core/test/sandbox/docker/container-ownership.test.ts b/packages/core/test/sandbox/docker/container-ownership.test.ts new file mode 100644 index 0000000..3ea3459 --- /dev/null +++ b/packages/core/test/sandbox/docker/container-ownership.test.ts @@ -0,0 +1,106 @@ +/** + * Who is allowed to remove a container, without a daemon. + * + * `remove()` issues `docker rm --force --volumes `, and a name is shared across + * processes by design — adoption is what makes a sandbox id resumable. So the question these + * tests ask is not whether the removal works but whether it is issued at all, and that is + * only visible in the argv the daemon was called with. + * + * The recording runs in a child process, and that is forced rather than chosen, for the same + * reason `provider.test.ts` gives: `DOCKER_BIN` is read from `PLEASE_DOCKER_PATH` once, when + * `docker/cli.ts` is first imported, and `bun test` shares one module registry across every + * file — so a value set from inside a test arrives too late. + */ +import { chmod, mkdtemp, readFile, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import process from 'node:process' +import { describe, expect, it } from 'bun:test' + +const CONTAINER_MODULE = join(import.meta.dir, '..', '..', '..', 'src', 'sandbox', 'docker', 'container.ts') + +/** A `docker` that records its argv and reports no container of this name. */ +const FAKE_ABSENT = `#!/bin/sh +printf '%s\\n' "$*" >> "$PLEASE_ARGV_LOG" +case "$1" in container) exit 1 ;; esac +exit 0 +` + +/** A `docker` that records its argv and reports the name already taken by a running container. */ +const FAKE_RUNNING = `#!/bin/sh +printf '%s\\n' "$*" >> "$PLEASE_ARGV_LOG" +case "$1" in container) printf 'running\\n' ; exit 0 ;; esac +exit 0 +` + +/** Drive a handle in a child process pointed at `fake`, and return the argv it produced. */ +async function record(fake: string, body: string): Promise { + const dir = await mkdtemp(join(tmpdir(), 'please-docker-own-')) + const binary = join(dir, 'docker') + const log = join(dir, 'argv.log') + const driver = join(dir, 'driver.ts') + await writeFile(binary, fake) + await chmod(binary, 0o755) + await writeFile(driver, ` +import { createContainerHandle } from ${JSON.stringify(CONTAINER_MODULE)} + +const handle = createContainerHandle('owned', { + image: 'debian:probe', + workDir: '/srv', + ports: [], + prefix: 'suite', +}) +${body} +`) + + const child = Bun.spawn([process.execPath, 'run', driver], { + env: { ...process.env, PLEASE_DOCKER_PATH: binary, PLEASE_ARGV_LOG: log }, + stdout: 'pipe', + stderr: 'pipe', + }) + const stderr = await new Response(child.stderr).text() + if (await child.exited !== 0) { + throw new Error(`driver failed: ${stderr}`) + } + // A missing log is a real observation, not a broken fixture: the fake only creates it when + // it is invoked, so "no file" is how "no docker call at all" reaches the assertions. + const recorded = await readFile(log, 'utf-8').catch(() => '') + return recorded.split('\n').filter(line => line.length > 0) +} + +const removals = (argv: string[]): string[] => argv.filter(line => line.startsWith('rm ')) + +describe('container removal ownership', () => { + it('does not remove a running container this handle only adopted', async () => { + const argv = await record(FAKE_RUNNING, 'await handle.ready()\nawait handle.remove()') + + // The adopted container belongs to whoever created it — very possibly a live session in + // another process, which a name-addressed `rm --force` would kill outright. + expect(removals(argv)).toEqual([]) + expect(argv.some(line => line.startsWith('container inspect'))).toBe(true) + }) + + it('removes a container this handle created', async () => { + const argv = await record(FAKE_ABSENT, 'await handle.ready()\nawait handle.remove()') + + expect(argv.some(line => line.startsWith('run '))).toBe(true) + expect(removals(argv)).toEqual(['rm --force --volumes suite-owned-865b016c']) + }) + + it('issues nothing for a handle that never acquired a container', async () => { + const argv = await record(FAKE_ABSENT, 'await handle.remove()') + + expect(argv).toEqual([]) + }) + + it('stays idempotent after a create', async () => { + const argv = await record( + FAKE_ABSENT, + 'await handle.ready()\nawait handle.remove()\nawait handle.remove()', + ) + + // The second call has nothing left to own — `remove` clears the latch — so it resolves + // without reaching the daemon again, which is what makes repeating it safe. + expect(removals(argv)).toEqual(['rm --force --volumes suite-owned-865b016c']) + }) +}) From 06a283caae6fb82c885b40ff07d33727a104d298 Mon Sep 17 00:00:00 2001 From: Minsu Lee Date: Sat, 29 Aug 2026 04:12:02 +0900 Subject: [PATCH 2/2] test(core): cover the in-flight create, and derive the name instead of 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. --- .../docker/container-ownership.test.ts | 32 +++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-) diff --git a/packages/core/test/sandbox/docker/container-ownership.test.ts b/packages/core/test/sandbox/docker/container-ownership.test.ts index 3ea3459..4c14422 100644 --- a/packages/core/test/sandbox/docker/container-ownership.test.ts +++ b/packages/core/test/sandbox/docker/container-ownership.test.ts @@ -16,6 +16,7 @@ import { tmpdir } from 'node:os' import { join } from 'node:path' import process from 'node:process' import { describe, expect, it } from 'bun:test' +import { containerName } from '../../../src/sandbox/docker/container' const CONTAINER_MODULE = join(import.meta.dir, '..', '..', '..', 'src', 'sandbox', 'docker', 'container.ts') @@ -26,6 +27,16 @@ case "$1" in container) exit 1 ;; esac exit 0 ` +/** As {@link FAKE_ABSENT}, but slow to create, so an acquisition can be observed in flight. */ +const FAKE_SLOW = `#!/bin/sh +printf '%s\\n' "$*" >> "$PLEASE_ARGV_LOG" +case "$1" in + container) exit 1 ;; + run) sleep 0.5 ;; +esac +exit 0 +` + /** A `docker` that records its argv and reports the name already taken by a running container. */ const FAKE_RUNNING = `#!/bin/sh printf '%s\\n' "$*" >> "$PLEASE_ARGV_LOG" @@ -70,6 +81,10 @@ ${body} const removals = (argv: string[]): string[] => argv.filter(line => line.startsWith('rm ')) +// Derived rather than written out: the digest is `containerName`'s business, and pinning its +// current output here would make a change to the hash look like an ownership regression. +const REMOVAL = `rm --force --volumes ${containerName('owned', 'suite')}` + describe('container removal ownership', () => { it('does not remove a running container this handle only adopted', async () => { const argv = await record(FAKE_RUNNING, 'await handle.ready()\nawait handle.remove()') @@ -84,7 +99,7 @@ describe('container removal ownership', () => { const argv = await record(FAKE_ABSENT, 'await handle.ready()\nawait handle.remove()') expect(argv.some(line => line.startsWith('run '))).toBe(true) - expect(removals(argv)).toEqual(['rm --force --volumes suite-owned-865b016c']) + expect(removals(argv)).toEqual([REMOVAL]) }) it('issues nothing for a handle that never acquired a container', async () => { @@ -101,6 +116,19 @@ describe('container removal ownership', () => { // The second call has nothing left to own — `remove` clears the latch — so it resolves // without reaching the daemon again, which is what makes repeating it safe. - expect(removals(argv)).toEqual(['rm --force --volumes suite-owned-865b016c']) + expect(removals(argv)).toEqual([REMOVAL]) + }) + + it('waits for a create still in flight and removes what it produced', async () => { + const argv = await record( + FAKE_SLOW, + 'const pending = handle.ready()\nawait handle.remove()\nawait pending', + ) + + // The case the latch exists for: a create this handle started and then abandoned is + // exactly the container it is responsible for removing, and deciding ownership before the + // acquisition settles would read `undefined` and walk away from a container it made. + expect(removals(argv)).toEqual([REMOVAL]) + expect(argv.findIndex(line => line.startsWith('run '))).toBeLessThan(argv.indexOf(REMOVAL)) }) })