fix(core): remove only a container the handle created

packages/core/src/sandbox/docker/container.ts

@@ -115,6 +115,18 @@ async function adopt(name: string, status: string): Promise<string | undefined>
   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<string | undefined>
  * 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<string> {
+async function acquire(name: string, options: ContainerOptions): Promise<Acquisition> {
   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<string>
       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<string>
   /** Host address an exposed container port is reachable at, as `host:port`. */
   readonly hostAddress: (port: number) => Promise<string>
-  /** 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<void>
   /**
    * 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<string> | 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<Acquisition> | undefined
 
-  const ready = (): Promise<string> => (acquisition ??= acquire(name, options).catch(
+  const acquireOnce = (): Promise<Acquisition> => (acquisition ??= acquire(name, options).catch(
     (cause: unknown) => {
       acquisition = undefined
       throw cause
     },
   ))
 
+  const ready = async (): Promise<string> => (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.

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 <name>`, 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<string[]> {
+  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))
+  })
+})