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..4c14422 --- /dev/null +++ b/packages/core/test/sandbox/docker/container-ownership.test.ts @@ -0,0 +1,134 @@ +/** + * 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' +import { containerName } from '../../../src/sandbox/docker/container' + +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 +` + +/** 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" +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 ')) + +// 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()') + + // 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([REMOVAL]) + }) + + 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([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)) + }) +})