fix(sandbox): bound every docker invocation and stop an orphaned exec

plugins/ca-sandbox/CHANGELOG.md

@@ -7,6 +7,8 @@ All notable changes to the **ca-sandbox** plugin are recorded here. Format follo
 ## [0.1.5] — 2026-07-24 — Teardown failures are surfaced and fail
 
 ### Fixed
+- **Every docker invocation is bounded (#394).** The single docker chokepoint called `spawnSync("docker", ...)` with no timeout, and `execInSandbox` widened only `maxBuffer` before waiting synchronously — so an in-container `sleep infinity`, a wedged daemon, or a stalled build had no deadline at all. The host process blocked until something external killed it, and untrusted repository code could deterministically wedge `sandbox exec`. Every invocation now carries a finite, per-operation deadline (generous where work is genuinely long — build, pull, exec — and short for a plain client call), and a deadline kill returns a **typed** `timedOut` result rather than a non-zero indistinguishable from an ordinary docker failure.
+- **A timed-out `exec` no longer orphans the in-container process (#394).** The deadline kills the docker *client*; the process it started inside the container keeps running, holding the box open. A timed-out exec now reaches back in and stops the container it targeted, with `--time 0` — the graceful window is what the deadline already spent, and a longer grace can itself exceed the runner's deadline and be killed before it lands. Escalation happens **only** on a timeout: doing it on an ordinary non-zero exit would destroy a working box every time a command returned 1. A failed escalation is reported but never masks the original timeout, since a wedged daemon is exactly when the cleanup cannot succeed.
 - **A failed teardown can no longer report success (#393).** `destroySandbox` and `prune` wrote `if (r.code === 0) removed.push(x)` and discarded every non-zero docker result; the result types carried no failure field at all, and the CLI returned 0 for both verbs unconditionally. A daemon outage, a permission error, or an in-use object therefore left **untrusted sandbox containers and source volumes running while automation read exit 0** — the advertised teardown guarantee was false on exactly the path where cleanup matters. Both verbs now retain every failed docker operation in a bounded `failures` list (with `failureCount` staying exact past the bound), and `sandbox destroy` / `sandbox prune` exit non-zero whenever anything failed or anything remains.
 
 ### Changed

plugins/ca-sandbox/tools/deadline.test.ts

@@ -0,0 +1,178 @@
+/**
+ * deadline.test.ts — #394. Every Docker invocation is bounded.
+ *
+ * The single Docker chokepoint called `spawnSync("docker", args, ...)` with NO
+ * timeout, and `execInSandbox` widened only `maxBuffer` before synchronously
+ * waiting. So an in-container `sleep infinity`, a wedged daemon, or a stalled
+ * build had no deadline at all: the host process blocked until something
+ * external killed it, and untrusted repository code could deterministically
+ * wedge `sandbox exec`. ca-sandbox is the driver that clones UNTRUSTED
+ * repositories into containers — a hang it cannot escape is a control failure,
+ * not an inconvenience.
+ *
+ * Two halves are tested here, and they are genuinely different:
+ *
+ *   ARGV/CONTRACT (no daemon needed) — every operation has a finite, declared
+ *   deadline; a timed-out run returns a TYPED result rather than a bare
+ *   non-zero; and a `docker exec` timeout escalates, because killing the docker
+ *   CLI does NOT stop the process it started inside the container.
+ *
+ *   REAL DAEMON (docker-gated, per #406's discipline) — a genuinely hung
+ *   in-container command terminates within its deadline and leaves nothing
+ *   running behind it. That is the assertion the argv tests cannot make.
+ */
+import { describe, it, expect } from "vitest";
+import { spawnSync } from "node:child_process";
+import {
+  DEFAULT_DOCKER_TIMEOUT_MS,
+  DOCKER_OPERATION_TIMEOUTS_MS,
+  defaultDockerRun,
+  makeDockerRun,
+  type RunResult,
+} from "./docker.ts";
+import { execInSandbox } from "./exec.ts";
+import { dockerGate } from "./docker-gate.ts";
+
+describe("#394 — every docker invocation carries a finite deadline", () => {
+  it("declares a default timeout, and it is finite and plausible", () => {
+    expect(Number.isFinite(DEFAULT_DOCKER_TIMEOUT_MS)).toBe(true);
+    expect(DEFAULT_DOCKER_TIMEOUT_MS).toBeGreaterThan(0);
+    // A deadline nobody can hit is not a deadline. Anything past a few minutes
+    // for a plain client call means the bound has drifted into decoration.
+    expect(DEFAULT_DOCKER_TIMEOUT_MS).toBeLessThanOrEqual(5 * 60_000);
+  });
+
+  it("gives every declared operation a finite deadline, longest for builds", () => {
+    const entries = Object.entries(DOCKER_OPERATION_TIMEOUTS_MS);
+    expect(entries.length).toBeGreaterThan(0);
+    for (const [operation, ms] of entries) {
+      expect(Number.isFinite(ms), `${operation} has no finite deadline`).toBe(true);
+      expect(ms, `${operation} deadline must be positive`).toBeGreaterThan(0);
+    }
+    // A pull or a build legitimately takes minutes; an inspect does not. If
+    // these ever equalise, the per-operation map has stopped meaning anything.
+    expect(DOCKER_OPERATION_TIMEOUTS_MS.build).toBeGreaterThan(DOCKER_OPERATION_TIMEOUTS_MS.inspect);
+  });
+
+  it("passes the deadline through to the spawn, not merely stores it", () => {
+    // An argv whose subcommand is NOT in the per-operation map, so the value
+    // handed to spawnSync is unambiguously the DEFAULT rather than a coincidence.
+    // The failure this catches: a timeout constant that exists, is exported,
+    // is asserted by a test, and is never handed to spawnSync.
+    const seen: Array<Record<string, unknown>> = [];
+    const run = makeDockerRun({}, { spawn: ((_cmd: string, _args: string[], options: unknown) => {
+      seen.push(options as Record<string, unknown>);
+      return { status: 0, stdout: "", stderr: "" };
+    }) as unknown as typeof spawnSync });
+    run(["totally-not-a-docker-subcommand"]);
+    expect(seen).toHaveLength(1);
+    expect(seen[0]!.timeout).toBe(DEFAULT_DOCKER_TIMEOUT_MS);
+    expect(seen[0]!.killSignal).toBeDefined();
+  });
+
+  it("reports a timeout as a TYPED result, not an indistinguishable non-zero", () => {
+    // spawnSync signals a deadline kill through `error` / `signal`, and a plain
+    // `{code: 1}` is what a normal docker failure looks like. A caller that
+    // cannot tell them apart cannot escalate, which is the whole point.
+    const timedOut = makeDockerRun({}, { spawn: (() => ({
+      status: null,
+      signal: "SIGTERM",
+      stdout: "",
+      stderr: "",
+      error: Object.assign(new Error("spawnSync docker ETIMEDOUT"), { code: "ETIMEDOUT" }),
+    })) as unknown as typeof spawnSync });
+    const result: RunResult = timedOut(["totally-not-a-docker-subcommand"]);
+    expect(result.timedOut).toBe(true);
+    expect(result.code).not.toBe(0);
+    expect(result.stderr).toMatch(/timed out/i);
+    expect(result.stderr).toMatch(new RegExp(String(DEFAULT_DOCKER_TIMEOUT_MS)));
+  });
+
+  it("does not mark an ordinary docker failure as a timeout", () => {
+    const failed = makeDockerRun({}, { spawn: (() => ({
+      status: 1, signal: null, stdout: "", stderr: "no such container", error: undefined,
+    })) as unknown as typeof spawnSync });
+    const result = failed(["inspect", "missing"]);
+    expect(result.timedOut).toBeFalsy();
+    expect(result.code).toBe(1);
+  });
+
+  it("escalates a timed-out exec: the container process is stopped, not orphaned", () => {
+    // THE POINT OF THE ESCALATION. spawnSync's timeout kills the docker CLIENT.
+    // The process it started inside the container keeps running, holding the
+    // box open and whatever it was doing. So a timed-out exec must reach back
+    // in and stop the container it targeted.
+    const calls: string[][] = [];
+    const dockerRun = (args: string[]): RunResult => {
+      calls.push(args);
+      if (args[0] === "exec") {
+        return { code: 124, stdout: "", stderr: "docker exec timed out", timedOut: true };
+      }
+      return { code: 0, stdout: "", stderr: "" };
+    };
+    const result = execInSandbox("box-1", ["sh", "-c", "sleep infinity"], { dockerRun });
+
+    expect(result.timedOut).toBe(true);
+    expect(result.exitCode).not.toBe(0);
+    const escalation = calls.slice(1);
+    expect(escalation.length, "a timed-out exec performed no cleanup").toBeGreaterThan(0);
+    expect(escalation.some((argv) => argv[0] === "stop" && argv.includes("box-1"))).toBe(true);
+  });
+
+  it("does not escalate when the exec merely fails", () => {
+    // Escalation stops a container. Doing that on an ordinary non-zero exit
+    // would destroy a working box every time a command returned 1.
+    const calls: string[][] = [];
+    const dockerRun = (args: string[]): RunResult => {
+      calls.push(args);
+      return { code: 7, stdout: "", stderr: "" };
+    };
+    const result = execInSandbox("box-2", ["sh", "-c", "exit 7"], { dockerRun });
+    expect(result.exitCode).toBe(7);
+    expect(result.timedOut).toBeFalsy();
+    expect(calls).toHaveLength(1);
+  });
+
+  it("still returns a result when the escalation itself fails", () => {
+    // A wedged daemon is exactly the case where the escalation cannot succeed.
+    // Reporting the original timeout matters more than reporting the cleanup.
+    const dockerRun = (args: string[]): RunResult => (args[0] === "exec"
+      ? { code: 124, stdout: "", stderr: "timed out", timedOut: true }
+      : { code: 1, stdout: "", stderr: "Cannot connect to the Docker daemon" });
+    const result = execInSandbox("box-3", ["sh", "-c", "sleep infinity"], { dockerRun });
+    expect(result.timedOut).toBe(true);
+    expect(result.stderr).toMatch(/timed out/i);
+  });
+});
+
+const realDocker = dockerGate("deadline", { linux: true });
+
+realDocker("#394 — a real hung command terminates within its deadline", () => {
+  it("a sleeping exec is killed at the deadline and leaves nothing running", () => {
+    const name = `ca-sbx-deadline-${Date.now().toString(36)}`;
+    const created = defaultDockerRun([
+      "run", "-d", "--rm", "--name", name, "alpine:latest", "sleep", "600",
+    ]);
+    expect(created.code, created.stderr).toBe(0);
+    const id = created.stdout.trim();
+    try {
+      const started = Date.now();
+      const result = execInSandbox(id, ["sh", "-c", "sleep 600"], {
+        dockerRun: makeDockerRun({ maxBuffer: 8 * 1024 * 1024 }, { timeoutMs: 4_000 }),
+      });
+      const elapsed = Date.now() - started;
+
+      expect(result.timedOut).toBe(true);
+      // Generous upper bound: this asserts the deadline EXISTS, not that the
+      // runner is fast. Without it the call never returns at all.
+      expect(elapsed).toBeLessThan(60_000);
+
+      // AC: no targeted process is left running in the container. The
+      // escalation stops the box, so the container itself is gone (--rm).
+      const alive = defaultDockerRun(["ps", "-q", "--filter", `name=${name}`]);
+      expect(alive.stdout.trim()).toBe("");
+    } finally {
+      defaultDockerRun(["rm", "-f", id || name]);
+    }
+  }, 120_000);
+});

plugins/ca-sandbox/tools/docker.ts

@@ -24,8 +24,77 @@ import { spawnSync, type SpawnSyncOptionsWithStringEncoding } from "node:child_p
 // re-spreading process.env itself.
 export const DOCKER_ENV: NodeJS.ProcessEnv = { ...process.env, MSYS_NO_PATHCONV: "1" };
 
-/** The one result shape every docker invocation in this plugin returns. */
-export type RunResult = { code: number; stdout: string; stderr: string };
+/**
+ * The one result shape every docker invocation in this plugin returns.
+ *
+ * `timedOut` (#394) is what makes a deadline actionable. spawnSync reports a
+ * deadline kill through `error`/`signal`, and a plain `{code: 1}` is what an
+ * ordinary docker failure looks like — a caller that cannot tell them apart
+ * cannot escalate, and escalation is the whole point for `docker exec`.
+ */
+export type RunResult = { code: number; stdout: string; stderr: string; timedOut?: boolean };
+
+/**
+ * #394 — every docker invocation is bounded.
+ *
+ * The chokepoint below used to call `spawnSync("docker", ...)` with NO timeout.
+ * An in-container `sleep infinity`, a wedged daemon, or a stalled build
+ * therefore blocked the host process until something external killed it, and
+ * untrusted repository code could deterministically wedge `sandbox exec`.
+ * ca-sandbox is the driver that clones UNTRUSTED repositories into containers;
+ * a hang it cannot escape is a control failure, not an inconvenience.
+ *
+ * The default is the bound for a plain client call — inspect, ps, rm, a label
+ * query. Anything that legitimately takes minutes declares its own below.
+ */
+export const DEFAULT_DOCKER_TIMEOUT_MS = 120_000;
+
+/**
+ * Per-operation deadlines, keyed on the docker subcommand (`args[0]`).
+ *
+ * Deliberately generous where the work is genuinely long: a cold `build` or
+ * `pull` over a slow network, and an `exec` that runs the user's own build
+ * inside the box, are all legitimately slow. The bound exists so that "slow"
+ * cannot become "never", not to second-guess how long real work takes. An
+ * operation absent from this map takes DEFAULT_DOCKER_TIMEOUT_MS.
+ */
+export const DOCKER_OPERATION_TIMEOUTS_MS: Readonly<Record<string, number>> = Object.freeze({
+  inspect: 30_000,
+  ps: 30_000,
+  images: 30_000,
+  version: 30_000,
+  info: 30_000,
+  volume: 60_000,
+  stop: 60_000,
+  kill: 60_000,
+  rm: 60_000,
+  rmi: 120_000,
+  network: 60_000,
+  cp: 600_000,
+  run: 900_000,
+  create: 300_000,
+  start: 300_000,
+  exec: 1_800_000,
+  build: 1_800_000,
+  buildx: 1_800_000,
+  pull: 1_800_000,
+});
+
+/** The deadline for `args`, chosen from its docker subcommand. */
+export function timeoutForArgs(args: readonly string[]): number {
+  return DOCKER_OPERATION_TIMEOUTS_MS[args[0] ?? ""] ?? DEFAULT_DOCKER_TIMEOUT_MS;
+}
+
+/** Conventional shell exit code for "killed by a timeout" (GNU `timeout`). */
+export const DOCKER_TIMEOUT_EXIT_CODE = 124;
+
+/** Options for the bounded runner; `spawn` is a test seam, never production. */
+export type DockerRunOptions = {
+  /** Override the per-operation deadline (tests, and callers with context). */
+  timeoutMs?: number;
+  /** Injectable spawn, so the deadline itself is assertable without a daemon. */
+  spawn?: typeof spawnSync;
+};
 
 /** An injectable docker runner — the seam every command module's tests use. */
 export type DockerRun = (args: string[]) => RunResult;
@@ -39,8 +108,33 @@ export type DockerRun = (args: string[]) => RunResult;
 function runDocker(
   args: string[],
   extra: Partial<SpawnSyncOptionsWithStringEncoding> = {},
+  options: DockerRunOptions = {},
 ): RunResult {
-  const r = spawnSync("docker", args, { encoding: "utf8", env: DOCKER_ENV, ...extra });
+  const timeout = options.timeoutMs ?? timeoutForArgs(args);
+  const spawn = options.spawn ?? spawnSync;
+  // `timeout` + `killSignal` are the whole of the bound: spawnSync sends the
+  // signal at the deadline and returns. `extra` is spread FIRST so a caller can
+  // still widen maxBuffer, but cannot silently remove the deadline.
+  const r = spawn("docker", args, {
+    encoding: "utf8",
+    env: DOCKER_ENV,
+    ...extra,
+    timeout,
+    killSignal: "SIGKILL",
+  });
+  // A deadline kill surfaces as ETIMEDOUT (or, on some platforms, only as a
+  // null status with a signal). Both are treated as the same typed outcome.
+  const timedOut = (r.error as NodeJS.ErrnoException | undefined)?.code === "ETIMEDOUT"
+    || (r.status === null && r.signal !== null && r.signal !== undefined);
+  if (timedOut) {
+    return {
+      code: DOCKER_TIMEOUT_EXIT_CODE,
+      stdout: r.stdout ?? "",
+      stderr: `${r.stderr ?? ""}ca-sandbox: \`docker ${args[0] ?? ""}\` timed out after ${timeout}ms `
+        + "and was killed (issue #394).",
+      timedOut: true,
+    };
+  }
   return {
     code: r.status ?? 1,
     stdout: r.stdout ?? "",
@@ -65,6 +159,9 @@ export function defaultDockerRun(args: string[]): RunResult {
  * exec.ts's own `capBytes` is meant to be the authoritative, deterministic
  * bound instead).
  */
-export function makeDockerRun(extra: Partial<SpawnSyncOptionsWithStringEncoding>): DockerRun {
-  return (args) => runDocker(args, extra);
+export function makeDockerRun(
+  extra: Partial<SpawnSyncOptionsWithStringEncoding>,
+  options: DockerRunOptions = {},
+): DockerRun {
+  return (args) => runDocker(args, extra, options);
 }

plugins/ca-sandbox/tools/exec.ts

@@ -56,6 +56,12 @@ export type ExecResult = {
   stderr: string;
   durationMs: number;
   truncated: boolean;
+  /**
+   * #394: the command hit its deadline and was killed, rather than exiting.
+   * Typed separately from `exitCode` because an ordinary non-zero exit and a
+   * hang are different events with different remedies.
+   */
+  timedOut?: boolean;
 };
 
 export type ExecOptions = {
@@ -126,9 +132,36 @@ export function execInSandbox(id: string, argv: string[], opts: ExecOptions = {}
   const r = dockerRun(args);
   const durationMs = Date.now() - start;
 
+  // #394 ESCALATION. spawnSync's deadline kills the docker CLIENT; the process
+  // it started INSIDE the container keeps running, holding the box open and
+  // whatever it was doing. Untrusted repository code can therefore wedge a
+  // sandbox permanently even after the exec "returns". So a timed-out exec
+  // reaches back in and stops the container it targeted.
+  //
+  // Only on a timeout. Escalation stops a container, and doing that on an
+  // ordinary non-zero exit would destroy a working box every time a command
+  // returned 1. Failure of the escalation itself is reported but never masks
+  // the original timeout - a wedged daemon is exactly the case where the
+  // cleanup cannot succeed, and the timeout is the more important fact.
+  let escalation = "";
+  if (r.timedOut) {
+    // `--time 0` on purpose: a graceful window is what the deadline already
+    // spent. Asking for another 5 seconds of grace here also means the
+    // escalation can itself exceed the runner's own timeout and be killed
+    // before it lands - measured, with a 4s runner deadline a `stop --time 5`
+    // never completes, and the container survives the "cleanup".
+    const stopped = dockerRun(["stop", "--time", "0", id]);
+    escalation = stopped.code === 0
+      ? `
+ca-sandbox: stopped container ${id} after the exec deadline.`
+      : `
+ca-sandbox: could not stop container ${id} after the exec deadline `
+        + `(${stopped.stderr.trim() || `exit ${stopped.code}`}); it may still be running.`;
+  }
+
   // Cap each stream INDEPENDENTLY (a huge stdout must not steal stderr's budget).
   const out = capBytes(r.stdout, maxBytes);
-  const err = capBytes(r.stderr, maxBytes);
+  const err = capBytes(r.stderr + escalation, maxBytes);
 
   return {
     id,
@@ -137,5 +170,6 @@ export function execInSandbox(id: string, argv: string[], opts: ExecOptions = {}
     stderr: err.value,
     durationMs,
     truncated: out.truncated || err.truncated,
+    ...(r.timedOut ? { timedOut: true } : {}),
   };
 }