From ed6f1bf7b3a6aa81c0adcab1132b2ca5ebf8f6e2 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Sat, 10 Oct 2026 15:39:41 +0000 Subject: [PATCH 1/5] =?UTF-8?q?=F0=9F=A4=96=20tests:=20list=20crash=20left?= =?UTF-8?q?overs=20of=20the=20bug-bash=20sandbox=20and=20add=20a=20recover?= =?UTF-8?q?=20target?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR C2 of the #5714 sandbox plan (#5882, plan item 11), stacked on PR C. - Every launch lists the job containers of its checkout (by the checkout label) after `docker info`, with the owner state dead, alive or cannot tell. It removes nothing. - `make bug-bash-sandbox-recover` (`launch.ts --recover`) prints the Docker endpoint, then removes only containers whose name matches this checkout's job pattern and whose owner is dead: same boot and PID namespace, and a PID that is gone or has another start time. Removal is by ID, after the same name and label check as cleanup. Everything else is listed and left. Exit 3 when a removal cannot be proved. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$46.10`_ --- Makefile | 7 +- tests/bugbash/sandbox/launch.ts | 80 +++++++++++++++-- tests/bugbash/sandbox/runner.test.ts | 129 +++++++++++++++++++++++++-- tests/bugbash/sandbox/runner.ts | 38 +++++++- 4 files changed, 239 insertions(+), 15 deletions(-) diff --git a/Makefile b/Makefile index 5461b7e2d41..8537ac476f6 100644 --- a/Makefile +++ b/Makefile @@ -91,7 +91,7 @@ include fmt.mk .PHONY: all build dev start clean help .PHONY: build-renderer version build-icons build-static build-docker-runtime verify-docker-runtime-artifacts .PHONY: lint lint-fix typecheck static-check static-check-full -.PHONY: test-bugbash-repros test-bugbash-known-failures bugbash-sandbox-key bugbash-sandbox-image bugbash-sandbox-publish +.PHONY: test-bugbash-repros test-bugbash-known-failures bugbash-sandbox-key bugbash-sandbox-image bugbash-sandbox-publish bug-bash-sandbox-recover .PHONY: test test-unit test-unit-ci test-integration test-watch test-coverage test-e2e test-e2e-perf perf-tape-replay smoke-test .PHONY: dist dist-mac dist-win dist-linux install-mac-arm64 ensure-mac-sharp-runtime-deps check-appimage-icons check-mac-attach-file-runtime .PHONY: vscode-ext vscode-ext-install @@ -235,6 +235,11 @@ bugbash-sandbox-image: ## Build the bug-bash sandbox image locally (linux/amd64) bugbash-sandbox-publish: ## Workflow only: build and push the bug-bash sandbox image, print its image.json record @./tests/bugbash/sandbox/build.sh --push +# Every sandbox launch lists the job containers that crashed launches of this checkout left +# (#5882). This removes the ones whose launcher is gone, by ID, and lists the rest. +bug-bash-sandbox-recover: ## Remove bug-bash sandbox containers of this checkout whose launcher is gone + @cd tests/bugbash && bun sandbox/launch.ts --recover + # Bug-bash repro tests (tests/bugbash/repros/*.e2e.ts): exact UI steps, no model calls, against a # seeded `xum server` (tests/bugbash/startApp.ts). e2e needs Node.js 22.22.3+ or 24.8+ (E2E_NODE) # and its own Chromium: `node_modules/.bin/e2e-web install chromium`. diff --git a/tests/bugbash/sandbox/launch.ts b/tests/bugbash/sandbox/launch.ts index c16f500506a..7d211c76a7d 100644 --- a/tests/bugbash/sandbox/launch.ts +++ b/tests/bugbash/sandbox/launch.ts @@ -2,6 +2,7 @@ * Runs one exact-step bug-bash e2e job in a disposable container: the bug-bash sandbox (#5714). * Usage, from tests/bugbash, with BUGBASH_AI=mock: * bun sandbox/launch.ts -- run --config e2e.config.ts --output .e2e/ [e2e args...] + * bun sandbox/launch.ts --recover (make bug-bash-sandbox-recover; see recover()) * * The container runs the e2e CLI, Chromium and the seeded app with the pinned image (runner.ts). * It gets no network, no capabilities, a read-only root, copies of the git-listed inputs, and @@ -29,6 +30,19 @@ const DEADLINE_MS = 30 * 60_000; const log = (message: string) => console.error(`sandbox ${message}`); const sha = (text: string) => crypto.createHash("sha256").update(text).digest("hex"); const bootId = () => fs.readFileSync("/proc/sys/kernel/random/boot_id", "utf8").trim(); +const pidNamespace = () => fs.readlinkSync("/proc/self/ns/pid").replace(/\D/g, ""); +/** The checkout label: the first 12 hex of the sha256 of its real path. Job names hold 6. */ +const checkoutId = (root: string) => sha(root).slice(0, 12); +/** Field 22 of /proc//stat (start time in clock ticks). Undefined when the PID is gone. */ +function startTime(pid: string): string | undefined { + try { + const stat = fs.readFileSync(`/proc/${pid}/stat`, "utf8"); + return stat.slice(stat.lastIndexOf(")") + 2).split(" ")[19]; + } catch (error) { + if ((error as NodeJS.ErrnoException).code === "ENOENT") return undefined; + throw error; + } +} /** * Only the mock app AI runs until the provider proxy (#5714): no provider key enters the @@ -48,12 +62,60 @@ function bind(src: string, dst: string): string[] { return ["--mount", `type=bind,src=${src},dst=${dst},readonly`]; } -/** boot id : PID namespace : PID : start time of this launcher. A later sweep reads it. */ +/** boot id : PID namespace : PID : start time of this launcher. ownerState() reads it. */ function ownerLabel(): string { - const pidns = fs.readlinkSync("/proc/self/ns/pid").replace(/\D/g, ""); - const stat = fs.readFileSync(`/proc/${process.pid}/stat`, "utf8"); - const start = stat.slice(stat.lastIndexOf(")") + 2).split(" ")[19]; - return `${bootId()}:${pidns}:${process.pid}:${start}`; + return `${bootId()}:${pidNamespace()}:${process.pid}:${startTime(String(process.pid))}`; +} + +/** + * Whether the launcher named by an owner label still runs. "dead" needs this boot and this PID + * namespace, and a PID that is gone or now has another start time (a reused PID). Anything that + * cannot be checked from here (another boot, another namespace, a malformed label, an + * unreadable /proc entry) is "cannot tell", and recover() leaves it. + */ +export function ownerState(owner: string): "dead" | "alive" | "cannot tell" { + const parts = owner.split(":"); + if (parts.length !== 4 || !/^\d+$/.test(parts[2]) || !/^\d+$/.test(parts[3])) + return "cannot tell"; + const [boot, pidns, pid, start] = parts; + if (boot !== bootId() || pidns !== pidNamespace()) return "cannot tell"; + try { + return startTime(pid) === start ? "alive" : "dead"; + } catch { + return "cannot tell"; + } +} + +/** + * `make bug-bash-sandbox-recover` (#5882): removes the job containers of this checkout whose + * launcher is dead. It needs the job name pattern and a dead owner (ownerState), and it removes + * by ID after the same name and label check as cleanup. It lists every other container and + * leaves it. The owner label names a launcher, not the daemon, so this runs only on purpose, + * and it prints the Docker endpoint first. Exit 3 when a removal cannot be proved. + */ +export async function recover(o: LaunchOptions & { log?: (line: string) => void }) { + const say = o.log ?? log; + const session = new Session(o.stop, { root: o.root, log: say }); + const checkout = checkoutId(o.root); + const jobName = new RegExp(`^xbb-${checkout.slice(0, 6)}-[0-9a-f]{6}$`); + let unproved = 0; + try { + say(`recover: docker endpoint ${await session.connect()}, checkout ${checkout}`); + for (const { id, job } of await session.listJobs(checkout)) { + const owner = ownerState(job.owner); + const named = jobName.test(job.name); + if (owner !== "dead" || !named) { + say(`recover: left ${job.name}: owner ${owner}${named ? "" : ", not a job name"}`); + continue; + } + const state = await session.removeJob(id, job); + say(`recover: ${job.name} ${state}`); + if (state !== "removed") unproved += 1; + } + } finally { + await session.cleanup(); + } + return unproved > 0 ? 3 : 0; } export interface LaunchOptions { @@ -86,13 +148,16 @@ export async function launch(args: string[], o: LaunchOptions): Promise ); mounts(); // before preflight, and again just before `docker run` const session = new Session(o.stop, { root: o.root }); - const checkout = sha(o.root).slice(0, 12); + const checkout = checkoutId(o.root); const name = `xbb-${checkout.slice(0, 6)}-${crypto.randomBytes(3).toString("hex")}`; const jobDir = path.join(os.tmpdir(), "xum-bugbash-sandbox", checkout, name); let made = false; const run = async (): Promise => { const image = await session.ensureImage(); + // Leftovers of crashed launches (#5882) are only listed here; recover removes dead ones. + for (const { job } of await session.listJobs(checkout)) + log(`leftover: ${job.name} owner ${ownerState(job.owner)} (make bug-bash-sandbox-recover)`); fs.mkdirSync(path.dirname(jobDir), { recursive: true, mode: 0o700 }); fs.mkdirSync(jobDir, { mode: 0o700 }); // EEXIST: not this job's folder, so cleanup keeps it made = true; @@ -164,7 +229,8 @@ if (import.meta.main) { const given = process.argv.slice(2); const args = given[0] === "--" ? given.slice(1) : given; const options = { root: ROOT, cwd: process.cwd(), env: process.env, stop: controller.signal }; - launch(args, options).then( + const recovering = args.length === 1 && args[0] === "--recover"; + (recovering ? recover(options) : launch(args, options)).then( (code) => process.exit(code), (error: unknown) => { if (error instanceof Stopped) { diff --git a/tests/bugbash/sandbox/runner.test.ts b/tests/bugbash/sandbox/runner.test.ts index 2cf6ded2ea0..471f27c9a8f 100644 --- a/tests/bugbash/sandbox/runner.test.ts +++ b/tests/bugbash/sandbox/runner.test.ts @@ -1,9 +1,10 @@ import { afterEach, beforeEach, expect, spyOn, test } from "bun:test"; import { spawnSync } from "node:child_process"; +import * as crypto from "node:crypto"; import * as fs from "node:fs"; import * as os from "node:os"; import * as path from "node:path"; -import { launch } from "./launch"; +import { launch, ownerState, recover } from "./launch"; import { groupState, readImageLock, Refusal, Session, Stopped } from "./runner"; // Each test runs the real build.sh in a throwaway git repo and a fake `docker` on PATH. The @@ -37,7 +38,7 @@ case "$1" in label=xum.bugbash.owner=*) o=\${a#label=xum.bugbash.owner=} ;; label=xum.bugbash.checkout=*) c=\${a#label=xum.bugbash.checkout=} ;; esac; done - awk -v n="$n" -v o="$o" -v c="$c" '$2==n && $3==o && $4==c {print $1}' "$bin/containers" ;; + awk -v n="$n" -v o="$o" -v c="$c" '(n=="" || $2==n) && (o=="" || $3==o) && (c=="" || $4==c) {print $1}' "$bin/containers" ;; # Like \`docker run --rm --cidfile\`: the create registers the container and writes its ID, # and the container ends on stdin EOF (the lifeline). Modes that keep the registry line model # a container the daemon has not removed yet, so only cleanup removes it. @@ -60,7 +61,7 @@ case "$1" in for id in "$@"; do :; done awk -v id="$id" '$1==id {print "/" $2 "|" $3 "|" $4; f=1} END {exit !f}' "$bin/containers" || { echo "Error response from daemon: No such container: $id" >&2; exit 1; } ;; - rm) shift 2; [ "\${RM-}" = slow ] && sleep 1; for id in "$@"; do unregister "$id"; done ;; + rm) shift 2; [ "\${RM-}" = slow ] && sleep 1; [ "\${RM-}" = noop ] && exit 0; for id in "$@"; do unregister "$id"; done ;; *) exit 9 ;; esac `; @@ -522,14 +523,15 @@ const HOST_ENV = { BUGBASH_AI: "mock", ANTHROPIC_API_KEY: "sk-secret", BUGBASH_A function launchIn( over: Record = {}, stop = new AbortController(), - env: Record = HOST_ENV + env: Record = HOST_ENV, + leftovers: string[] = [] ) { const real = fs.realpathSync(root); fs.mkdirSync(path.join(real, "tests/bugbash"), { recursive: true }); fs.writeFileSync(path.join(real, "tests/bugbash/e2e.config.ts"), "export default {}"); for (const dir of ["dist", "node_modules"]) fs.mkdirSync(path.join(real, dir), { recursive: true }); - fs.writeFileSync(path.join(bin, "containers"), FOREIGN.join("\n") + "\n"); + fs.writeFileSync(path.join(bin, "containers"), [...FOREIGN, ...leftovers].join("\n") + "\n"); fake({ IMAGES: "sha256:abc", SWAP: path.join(real, "node_modules"), ...over }); const cwd = path.join(real, "tests/bugbash"); return launch(ARGS, { root: real, cwd, env, stop: stop.signal }); @@ -607,6 +609,123 @@ test("C8: a stop during cleanup does not cut `docker rm`; the stop decides the e expectNothingLeft(); }, 15_000); +// Crash leftovers (#5882, plan PR C2). Owner labels are boot ID : PID namespace : PID : start. +const procStart = (pid: number | string) => { + const stat = fs.readFileSync(`/proc/${pid}/stat`, "utf8"); + return stat.slice(stat.lastIndexOf(")") + 2).split(" ")[19]; +}; +const self = () => ({ + boot: fs.readFileSync("/proc/sys/kernel/random/boot_id", "utf8").trim(), + pidns: fs.readlinkSync("/proc/self/ns/pid").replace(/\D/g, ""), +}); +/** A PID above the kernel's limit, so no process has it. */ +const NO_PID = Number(fs.readFileSync("/proc/sys/kernel/pid_max", "utf8")) + 1; +function leftovers() { + const checkout = crypto + .createHash("sha256") + .update(fs.realpathSync(root)) + .digest("hex") + .slice(0, 12); + const { boot, pidns } = self(); + const id = (n: number) => n.toString(16).padStart(64, "0"); + const name = (hex: string) => `xbb-${checkout.slice(0, 6)}-${hex}`; + const live = `${boot}:${pidns}:${process.pid}:${procStart(process.pid)}`; + const lines = { + dead: `${id(1)} ${name("aaaaa1")} ${boot}:${pidns}:${NO_PID}:1 ${checkout}`, + restarted: `${id(2)} ${name("aaaaa2")} ${boot}:${pidns}:${process.pid}:1 ${checkout}`, + live: `${id(3)} ${name("aaaaa3")} ${live} ${checkout}`, + otherPidns: `${id(4)} ${name("aaaaa4")} ${boot}:1:${NO_PID}:1 ${checkout}`, + otherBoot: `${id(5)} ${name("aaaaa5")} other-boot:${pidns}:${NO_PID}:1 ${checkout}`, + otherName: `${id(6)} xbb-other ${boot}:${pidns}:${NO_PID}:1 ${checkout}`, + otherCheckout: `${id(7)} ${name("aaaaa7")} ${boot}:${pidns}:${NO_PID}:1 ${"e".repeat(12)}`, + }; + return { checkout, lines }; +} + +test("ownerState: dead, alive, or cannot tell", () => { + const { boot, pidns } = self(); + expect(ownerState(`${boot}:${pidns}:${process.pid}:${procStart(process.pid)}`)).toBe("alive"); + expect(ownerState(`${boot}:${pidns}:${NO_PID}:1`)).toBe("dead"); + expect(ownerState(`${boot}:${pidns}:${process.pid}:1`)).toBe("dead"); // the PID was reused + for (const owner of [ + `x:${pidns}:${NO_PID}:1`, + `${boot}:1:${NO_PID}:1`, + "garbage", + `${boot}:${pidns}:-1:1`, + ]) + expect(ownerState(owner)).toBe("cannot tell"); +}); + +test("C2: a launch lists the leftovers of its checkout and removes nothing", async () => { + const { lines } = leftovers(); + const all = Object.values(lines); + const logged: string[] = []; + const error = spyOn(console, "error").mockImplementation( + (line: string) => void logged.push(line) + ); + try { + expect(await launchIn({}, new AbortController(), HOST_ENV, all)).toBe(7); + } finally { + error.mockRestore(); + } + const listed = logged.filter((line) => line.includes("leftover: ")); + expect( + listed.map((line) => line.replace(/^sandbox leftover: (\S+) owner (.+?) \(.*$/, "$1 $2")) + ).toEqual( + [ + lines.dead, + lines.restarted, + lines.live, + lines.otherPidns, + lines.otherBoot, + lines.otherName, + ].map( + (line, i) => + `${line.split(" ")[1]} ${["dead", "dead", "alive", "cannot tell", "cannot tell", "dead"][i]}` + ) + ); + expect(registry().trim().split("\n")).toEqual([...FOREIGN, ...all]); +}); + +test("C2: recover removes only dead owners of this checkout with a job name, by ID", async () => { + const { lines } = leftovers(); + fs.writeFileSync(path.join(bin, "containers"), Object.values(lines).join("\n") + "\n"); + fake(); + const real = fs.realpathSync(root); + const logged: string[] = []; + const o = { root: real, cwd: real, env: {}, stop: new AbortController().signal }; + expect(await recover({ ...o, log: (line) => logged.push(line) })).toBe(0); + const left = [ + lines.live, + lines.otherPidns, + lines.otherBoot, + lines.otherName, + lines.otherCheckout, + ]; + expect(registry().trim().split("\n")).toEqual(left); + expect(logged[0]).toStartWith("recover: docker endpoint unix:///var/run/docker.sock"); + // Removal is by ID only, after an inspect, and nothing is built or pulled. + const ids = [1, 2].map((n) => n.toString(16).padStart(64, "0")); + expect([...(calls().match(/^rm -f \w+/gm) ?? [])]).toEqual(ids.map((id) => `rm -f ${id}`)); + expect(calls()).not.toMatch(/^(images|pull|image |run )/m); + expect(fs.readdirSync(tmp)).toEqual([]); +}); + +test("C2: recover reports a removal it cannot prove (exit 3)", async () => { + const { lines } = leftovers(); + fs.writeFileSync(path.join(bin, "containers"), lines.dead + "\n"); + fake({ RM: "noop" }); + const real = fs.realpathSync(root); + const o = { + root: real, + cwd: real, + env: {}, + stop: new AbortController().signal, + log: () => undefined, + }; + expect(await recover(o)).toBe(3); +}); + test("a signal during the synchronous staging starts no container", async () => { const stop = new AbortController(); // containerEnv reads BUGBASH_EFFORT right after staging, still synchronously. A real signal diff --git a/tests/bugbash/sandbox/runner.ts b/tests/bugbash/sandbox/runner.ts index e4ac90f078d..d38c4d61082 100644 --- a/tests/bugbash/sandbox/runner.ts +++ b/tests/bugbash/sandbox/runner.ts @@ -67,6 +67,8 @@ export interface Job { } /** "none": the session owned no job, so no container can exist. */ export type CleanupState = "none" | "removed" | `unknown: ${string}`; +/** `docker container inspect` output: `/name|owner label|checkout label`. */ +const INSPECT = `{{.Name}}|{{index .Config.Labels "xum.bugbash.owner"}}|{{index .Config.Labels "xum.bugbash.checkout"}}`; /** The container name goes into a `name=^/…$` filter, which is a regex: no dots, no specials. */ const CONTAINER_NAME = /^[a-z0-9][a-z0-9-]{0,62}$/; @@ -157,6 +159,39 @@ export class Session { this.#owned = Object.freeze({ name: job.name, owner: job.owner, checkout: job.checkout }); } + /** Connects to the local daemon as ensureImage() does, and returns the endpoint it uses. */ + async connect(): Promise { + await this.#connect(); + return this.#client!.DOCKER_HOST; + } + + /** + * Every job container of one checkout, of any owner, by its checkout label (#5882). It + * removes nothing: a launch logs the list, and only recover() removes from it. + */ + async listJobs(checkout: string): Promise<{ id: string; job: Job }[]> { + if (!/^[0-9a-f]{12}$/.test(checkout)) throw new Refusal(`bad checkout ID ${checkout}`); + const filter = ["--filter", `label=xum.bugbash.checkout=${checkout}`]; + const ps = await this.#must(["ps", "-aq", "--no-trunc", ...filter], 15_000); + const found: { id: string; job: Job }[] = []; + for (const id of ps.stdout.split("\n").filter((line) => line !== "")) { + if (!/^[0-9a-f]{64}$/.test(id)) throw new Refusal(`docker ps: unexpected ID ${id}`); + const args = ["container", "inspect", "--format", INSPECT, id]; + const look = await this.#job("docker", args, this.#client!, 15_000); + if (!look.ok) continue; // removed since the list: nothing to report + // The label filter picked the checkout; removeJob() checks all three again before a removal. + const [name, owner] = look.stdout.replace(/^\//, "").split("|"); + found.push({ id, job: { name, owner, checkout } }); + } + return found; + } + + /** Removes one listed container by ID, with the same name and label check as cleanup(). */ + removeJob(id: string, job: Job): Promise { + if (this.#owned != null) throw new Error("removeJob() is for a session without its own job"); + return this.#removeById(id, job, "the leftover list"); + } + /** Runs `fn` first on a stop, before the lifeline closes (B1: the provider proxy's close). */ onStop(fn: () => unknown): void { this.#hooks.push(fn); @@ -250,11 +285,10 @@ export class Session { /** Removes the container `id` only when its name and both labels are this job's. */ async #removeById(id: string, job: Job, source: string): Promise { - const format = `{{.Name}}|{{index .Config.Labels "xum.bugbash.owner"}}|{{index .Config.Labels "xum.bugbash.checkout"}}`; const inspect = () => this.#spawn( "docker", - ["container", "inspect", "--format", format, id], + ["container", "inspect", "--format", INSPECT, id], this.#client!, 15_000 ); From 1b2d592429189a280c2d5a6b799b20d96685f4d5 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Sat, 10 Oct 2026 16:20:45 +0000 Subject: [PATCH 2/5] =?UTF-8?q?=F0=9F=A4=96=20tests:=20harden=20sandbox=20?= =?UTF-8?q?recovery=20after=20Codex=20review=20round=201?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - A launch lists the leftovers right after the Docker preflight, before it prepares the image, so a launch whose pull fails still shows them. - listJobs() ignores a container only when inspect says "No such container"; any other inspect failure refuses, so recover cannot exit 0 past an unseen container. - recover() checks the stop before each removal: a removal that started finishes, and no new one starts. - ownerState() counts a zombie launcher (state Z or X) as dead. - recover() returns 3 when the session cleanup state is unknown. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$52.06`_ --- tests/bugbash/sandbox/launch.ts | 36 ++++++++--- tests/bugbash/sandbox/runner.test.ts | 91 +++++++++++++++++++++++++++- tests/bugbash/sandbox/runner.ts | 4 +- 3 files changed, 119 insertions(+), 12 deletions(-) diff --git a/tests/bugbash/sandbox/launch.ts b/tests/bugbash/sandbox/launch.ts index 7d211c76a7d..247683f07a2 100644 --- a/tests/bugbash/sandbox/launch.ts +++ b/tests/bugbash/sandbox/launch.ts @@ -33,11 +33,14 @@ const bootId = () => fs.readFileSync("/proc/sys/kernel/random/boot_id", "utf8"). const pidNamespace = () => fs.readlinkSync("/proc/self/ns/pid").replace(/\D/g, ""); /** The checkout label: the first 12 hex of the sha256 of its real path. Job names hold 6. */ const checkoutId = (root: string) => sha(root).slice(0, 12); -/** Field 22 of /proc//stat (start time in clock ticks). Undefined when the PID is gone. */ -function startTime(pid: string): string | undefined { +/** + * The fields of /proc//stat after the command name: [0] the state, [19] the start time in + * clock ticks. Undefined when the PID is gone. + */ +function procStat(pid: string): string[] | undefined { try { const stat = fs.readFileSync(`/proc/${pid}/stat`, "utf8"); - return stat.slice(stat.lastIndexOf(")") + 2).split(" ")[19]; + return stat.slice(stat.lastIndexOf(")") + 2).split(" "); } catch (error) { if ((error as NodeJS.ErrnoException).code === "ENOENT") return undefined; throw error; @@ -64,12 +67,13 @@ function bind(src: string, dst: string): string[] { /** boot id : PID namespace : PID : start time of this launcher. ownerState() reads it. */ function ownerLabel(): string { - return `${bootId()}:${pidNamespace()}:${process.pid}:${startTime(String(process.pid))}`; + return `${bootId()}:${pidNamespace()}:${process.pid}:${procStat(String(process.pid))?.[19]}`; } /** * Whether the launcher named by an owner label still runs. "dead" needs this boot and this PID - * namespace, and a PID that is gone or now has another start time (a reused PID). Anything that + * namespace, and a PID that is gone, a zombie (it exited and waits for its parent), or now has + * another start time (a reused PID). Anything that * cannot be checked from here (another boot, another namespace, a malformed label, an * unreadable /proc entry) is "cannot tell", and recover() leaves it. */ @@ -80,7 +84,8 @@ export function ownerState(owner: string): "dead" | "alive" | "cannot tell" { const [boot, pidns, pid, start] = parts; if (boot !== bootId() || pidns !== pidNamespace()) return "cannot tell"; try { - return startTime(pid) === start ? "alive" : "dead"; + const stat = procStat(pid); + return stat != null && !/^[ZX]$/.test(stat[0]) && stat[19] === start ? "alive" : "dead"; } catch { return "cannot tell"; } @@ -99,6 +104,7 @@ export async function recover(o: LaunchOptions & { log?: (line: string) => void const checkout = checkoutId(o.root); const jobName = new RegExp(`^xbb-${checkout.slice(0, 6)}-[0-9a-f]{6}$`); let unproved = 0; + let failure: { error: unknown } | undefined; try { say(`recover: docker endpoint ${await session.connect()}, checkout ${checkout}`); for (const { id, job } of await session.listJobs(checkout)) { @@ -108,13 +114,21 @@ export async function recover(o: LaunchOptions & { log?: (line: string) => void say(`recover: left ${job.name}: owner ${owner}${named ? "" : ", not a job name"}`); continue; } + // A removal that started finishes (cleanup commands ignore a stop); no new one starts. + if (o.stop.aborted) throw new Stopped(String(o.stop.reason)); const state = await session.removeJob(id, job); say(`recover: ${job.name} ${state}`); if (state !== "removed") unproved += 1; } - } finally { - await session.cleanup(); + } catch (error) { + failure = { error }; } + const state = await session.cleanup(); + if (state.startsWith("unknown")) { + say(`recover: cleanup ${state}`); + return 3; + } + if (failure) throw failure.error; return unproved > 0 ? 3 : 0; } @@ -154,10 +168,12 @@ export async function launch(args: string[], o: LaunchOptions): Promise let made = false; const run = async (): Promise => { - const image = await session.ensureImage(); - // Leftovers of crashed launches (#5882) are only listed here; recover removes dead ones. + // Leftovers of crashed launches (#5882) are only listed here, first, so a launch that fails + // later (a long or failed pull) still shows them. recover removes the dead ones. + await session.connect(); for (const { job } of await session.listJobs(checkout)) log(`leftover: ${job.name} owner ${ownerState(job.owner)} (make bug-bash-sandbox-recover)`); + const image = await session.ensureImage(); fs.mkdirSync(path.dirname(jobDir), { recursive: true, mode: 0o700 }); fs.mkdirSync(jobDir, { mode: 0o700 }); // EEXIST: not this job's folder, so cleanup keeps it made = true; diff --git a/tests/bugbash/sandbox/runner.test.ts b/tests/bugbash/sandbox/runner.test.ts index 471f27c9a8f..935c49c10a7 100644 --- a/tests/bugbash/sandbox/runner.test.ts +++ b/tests/bugbash/sandbox/runner.test.ts @@ -1,5 +1,5 @@ import { afterEach, beforeEach, expect, spyOn, test } from "bun:test"; -import { spawnSync } from "node:child_process"; +import { spawn, spawnSync } from "node:child_process"; import * as crypto from "node:crypto"; import * as fs from "node:fs"; import * as os from "node:os"; @@ -27,6 +27,7 @@ case "$1" in if [ "$PULL" = orphan ]; then (trap '' TERM; exec sleep 60) >/dev/null 2>&1 & echo $! > "$bin/grandchild.pid"; sleep 30; fi if [ "$PULL" = group ]; then sleep 60 >/dev/null 2>&1 & echo $! > "$bin/grandchild.pid"; trap '' TERM; wait $!; fi if [ "$PULL" = swap ]; then rm -r "$SWAP"; ln -s / "$SWAP"; fi + if [ "$PULL" = fail ]; then echo "pull failed" >&2; exit 1; fi # Exits at once and leaves a member in its group (cleanup item 7). if [ "$PULL" = leave ]; then (trap '' TERM; exec sleep 60) >/dev/null 2>&1 & echo $! > "$bin/grandchild.pid"; fi ;; image) echo "{\\"org.xum.bugbash.inputs\\":\\"$LABEL\\"}" ;; @@ -726,6 +727,94 @@ test("C2: recover reports a removal it cannot prove (exit 3)", async () => { expect(await recover(o)).toBe(3); }); +test("C2: a zombie launcher counts as dead", async () => { + // `sleep 0` exits at once, and its parent then execs `sleep 30`, which never reaps it. + const parent = spawn("sh", ["-c", "sleep 0 & echo $!; exec sleep 30"], { + stdio: ["ignore", "pipe", "ignore"], + }); + try { + const pid = Number( + await new Promise((resolve) => + parent.stdout.once("data", (d: Buffer) => resolve(d.toString())) + ) + ); + while (!/\) Z /.test(stat(pid))) await Bun.sleep(10); + const { boot, pidns } = self(); + expect(ownerState(`${boot}:${pidns}:${pid}:${procStart(pid)}`)).toBe("dead"); + } finally { + parent.kill("SIGKILL"); + } +}); + +test("C2: a launch lists the leftovers even when the image pull fails", async () => { + const { lines } = leftovers(); + const logged: string[] = []; + const error = spyOn(console, "error").mockImplementation( + (line: string) => void logged.push(line) + ); + try { + const result = await launchIn({ IMAGES: "", PULL: "fail" }, new AbortController(), HOST_ENV, [ + lines.dead, + ]).catch((e: unknown) => e); + expect(result).toBeInstanceOf(Refusal); + } finally { + error.mockRestore(); + } + expect( + logged.some((line) => line.includes(`leftover: ${lines.dead.split(" ")[1]} owner dead`)) + ).toBe(true); +}); + +test("C2: recover refuses when a listed container cannot be inspected", async () => { + const { lines } = leftovers(); + fs.writeFileSync(path.join(bin, "containers"), lines.dead + "\n"); + fake({ INSPECT_RC: "1" }); + const real = fs.realpathSync(root); + const o = { + root: real, + cwd: real, + env: {}, + stop: new AbortController().signal, + log: () => undefined, + }; + expect(await failure(recover(o))).toThrow(/docker container inspect: daemon down/); + expect(registry()).toBe(lines.dead + "\n"); +}); + +test("C2: a stop during recover starts no further removal", async () => { + const { lines } = leftovers(); + const second = lines.dead.replace(/^\w+/, "f".repeat(64)).replace("aaaaa1", "aaaaaf"); + fs.writeFileSync(path.join(bin, "containers"), [lines.dead, second].join("\n") + "\n"); + fake(); + const real = fs.realpathSync(root); + const stop = new AbortController(); + const log = (line: string) => line.endsWith(" removed") && stop.abort("SIGTERM"); + const o = { root: real, cwd: real, env: {}, stop: stop.signal, log }; + expect(await recover(o).catch((e: unknown) => e)).toEqual(new Stopped("SIGTERM")); + expect(registry()).toBe(second + "\n"); // the first removal finished, the second never started +}); + +test("C2: recover reports an unknown cleanup state (exit 3)", async () => { + fs.writeFileSync(path.join(bin, "containers"), ""); + fake(); + const real = fs.realpathSync(root); + const cleanup = spyOn(Session.prototype, "cleanup").mockResolvedValue( + "unknown: process groups 1 still run" + ); + try { + const o = { + root: real, + cwd: real, + env: {}, + stop: new AbortController().signal, + log: () => undefined, + }; + expect(await recover(o)).toBe(3); + } finally { + cleanup.mockRestore(); + } +}); + test("a signal during the synchronous staging starts no container", async () => { const stop = new AbortController(); // containerEnv reads BUGBASH_EFFORT right after staging, still synchronously. A real signal diff --git a/tests/bugbash/sandbox/runner.ts b/tests/bugbash/sandbox/runner.ts index d38c4d61082..52bc3da8d84 100644 --- a/tests/bugbash/sandbox/runner.ts +++ b/tests/bugbash/sandbox/runner.ts @@ -178,7 +178,9 @@ export class Session { if (!/^[0-9a-f]{64}$/.test(id)) throw new Refusal(`docker ps: unexpected ID ${id}`); const args = ["container", "inspect", "--format", INSPECT, id]; const look = await this.#job("docker", args, this.#client!, 15_000); - if (!look.ok) continue; // removed since the list: nothing to report + // Removed since the list: nothing to report. Any other failure hides a container: refuse. + if (!look.ok && look.error.includes("No such container")) continue; + if (!look.ok) throw new Refusal(`docker container inspect: ${look.error}`); // The label filter picked the checkout; removeJob() checks all three again before a removal. const [name, owner] = look.stdout.replace(/^\//, "").split("|"); found.push({ id, job: { name, owner, checkout } }); From 57e1f7b20a0540f6059a50e00cbc17bb1415f935 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Sat, 10 Oct 2026 16:22:42 +0000 Subject: [PATCH 3/5] =?UTF-8?q?=F0=9F=A4=96=20tests:=20use=20String#includ?= =?UTF-8?q?es=20in=20the=20zombie=20owner=20test=20(lint)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$52.06`_ --- tests/bugbash/sandbox/runner.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/bugbash/sandbox/runner.test.ts b/tests/bugbash/sandbox/runner.test.ts index 935c49c10a7..34652998560 100644 --- a/tests/bugbash/sandbox/runner.test.ts +++ b/tests/bugbash/sandbox/runner.test.ts @@ -738,7 +738,7 @@ test("C2: a zombie launcher counts as dead", async () => { parent.stdout.once("data", (d: Buffer) => resolve(d.toString())) ) ); - while (!/\) Z /.test(stat(pid))) await Bun.sleep(10); + while (!stat(pid).includes(") Z ")) await Bun.sleep(10); const { boot, pidns } = self(); expect(ownerState(`${boot}:${pidns}:${pid}:${procStart(pid)}`)).toBe("dead"); } finally { From 21c652b1be1286b90881cbedc76c335a92720e48 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Sat, 10 Oct 2026 16:43:34 +0000 Subject: [PATCH 4/5] =?UTF-8?q?=F0=9F=A4=96=20tests:=20make=20the=20zombie?= =?UTF-8?q?=20owner=20fixture=20deterministic?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex review round 2: a child that exits at once can be reaped by the shell before the shell execs `sleep 30`, and the test then waited forever. The child now exits 300 ms later, after the exec, and the wait is bounded with a clear assertion. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$52.06`_ --- tests/bugbash/sandbox/runner.test.ts | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tests/bugbash/sandbox/runner.test.ts b/tests/bugbash/sandbox/runner.test.ts index 34652998560..8f1c192d018 100644 --- a/tests/bugbash/sandbox/runner.test.ts +++ b/tests/bugbash/sandbox/runner.test.ts @@ -728,8 +728,9 @@ test("C2: recover reports a removal it cannot prove (exit 3)", async () => { }); test("C2: a zombie launcher counts as dead", async () => { - // `sleep 0` exits at once, and its parent then execs `sleep 30`, which never reaps it. - const parent = spawn("sh", ["-c", "sleep 0 & echo $!; exec sleep 30"], { + // The child exits only after its parent shell became `sleep 30`, which never reaps it. (A + // child that exits at once can be reaped by the shell before the exec.) + const parent = spawn("sh", ["-c", "(sleep 0.3) & echo $!; exec sleep 30"], { stdio: ["ignore", "pipe", "ignore"], }); try { @@ -738,7 +739,8 @@ test("C2: a zombie launcher counts as dead", async () => { parent.stdout.once("data", (d: Buffer) => resolve(d.toString())) ) ); - while (!stat(pid).includes(") Z ")) await Bun.sleep(10); + for (let i = 0; i < 500 && !stat(pid).includes(") Z "); i++) await Bun.sleep(10); + expect(stat(pid)).toContain(") Z "); // a bounded wait: no hang if the zombie never shows const { boot, pidns } = self(); expect(ownerState(`${boot}:${pidns}:${pid}:${procStart(pid)}`)).toBe("dead"); } finally { From 2bd7b633a9a0c772deb8ecc156ae0282a287dd5a Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Sat, 10 Oct 2026 16:52:46 +0000 Subject: [PATCH 5/5] =?UTF-8?q?=F0=9F=A4=96=20test(bugbash):=20zombie=20fi?= =?UTF-8?q?xture=20handshake;=20bound=20each=20sandbox=20stop=20hook?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - runner.test.ts: the zombie-launcher test now releases the child through a FIFO only after the parent shell became `sleep 30`, instead of a fixed `sleep 0.3` that a descheduled shell could lose (Codex r3). - runner.ts: #endJob waits at most 10 s for each onStop hook, then logs and closes the lifeline anyway, so a hung hook (B1's proxy close) cannot block cleanup. Test: a never-settling hook still lets stdin close and cleanup finish. --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-opus-5-5` • Thinking: `high`_ --- tests/bugbash/sandbox/runner.test.ts | 45 +++++++++++++++++++++++++--- tests/bugbash/sandbox/runner.ts | 23 +++++++++++++- 2 files changed, 63 insertions(+), 5 deletions(-) diff --git a/tests/bugbash/sandbox/runner.test.ts b/tests/bugbash/sandbox/runner.test.ts index 8f1c192d018..06aa4d16316 100644 --- a/tests/bugbash/sandbox/runner.test.ts +++ b/tests/bugbash/sandbox/runner.test.ts @@ -518,6 +518,25 @@ test("C11: the stop hooks finish before the lifeline closes", async () => { expect(await s.cleanup()).toBe("removed"); }); +test("C11: a stop hook that never settles still lets the lifeline close and cleanup finish", async () => { + fake({ IMAGES: "sha256:abc", RUN: "hang" }); + const stop = new AbortController(); + const logged: string[] = []; + const s = new Session(stop.signal, { root, hookMs: 100, log: (line) => logged.push(line) }); + sessions.push(s); + await s.ensureImage(); + s.own(JOB); + const job = s.runJob(["--rm", "img"], drain, 60_000); + job.catch(() => undefined); + while (!calls().includes("run ")) await Bun.sleep(20); + s.onStop(() => new Promise(() => undefined)); + stop.abort("SIGTERM"); + await job.catch(() => undefined); + expect(calls()).toContain("stdin closed"); + expect(logged.some((line) => line.includes("did not settle in 100 ms"))).toBe(true); + expect(await s.cleanup()).toBe("removed"); +}); + // launch.ts, with the same fake docker: a checkout with a repro config and both mount sources. const ARGS = ["run", "--config", "e2e.config.ts", "--output", ".e2e/r"]; const HOST_ENV = { BUGBASH_AI: "mock", ANTHROPIC_API_KEY: "sk-secret", BUGBASH_APP_LOG: "/x.log" }; @@ -728,9 +747,12 @@ test("C2: recover reports a removal it cannot prove (exit 3)", async () => { }); test("C2: a zombie launcher counts as dead", async () => { - // The child exits only after its parent shell became `sleep 30`, which never reaps it. (A - // child that exits at once can be reaped by the shell before the exec.) - const parent = spawn("sh", ["-c", "(sleep 0.3) & echo $!; exec sleep 30"], { + // A handshake, not a delay: the child blocks on a FIFO, and the test releases it only after the + // parent shell has become `sleep 30` (which never reaps it). A timed child can exit while the + // shell still runs and be reaped before the exec on a loaded host. + const fifo = path.join(root, "release"); + expect(spawnSync("mkfifo", [fifo]).status).toBe(0); + const parent = spawn("sh", ["-c", '(read _ < "$1") & echo $!; exec sleep 30', "sh", fifo], { stdio: ["ignore", "pipe", "ignore"], }); try { @@ -739,8 +761,23 @@ test("C2: a zombie launcher counts as dead", async () => { parent.stdout.once("data", (d: Buffer) => resolve(d.toString())) ) ); + const comm = () => /\(([^)]*)\)/.exec(stat(parent.pid!))?.[1]; + for (let i = 0; i < 500 && comm() !== "sleep"; i++) await Bun.sleep(10); + expect(comm()).toBe("sleep"); // bounded waits: no hang if a step never happens + // O_NONBLOCK: the open fails (ENXIO) instead of blocking while the child has not opened it. + let fd = -1; + for (let i = 0; i < 500 && fd < 0; i++) { + try { + fd = fs.openSync(fifo, fs.constants.O_WRONLY | fs.constants.O_NONBLOCK); + } catch { + await Bun.sleep(10); + } + } + expect(fd).toBeGreaterThanOrEqual(0); + fs.writeSync(fd, "go\n"); + fs.closeSync(fd); for (let i = 0; i < 500 && !stat(pid).includes(") Z "); i++) await Bun.sleep(10); - expect(stat(pid)).toContain(") Z "); // a bounded wait: no hang if the zombie never shows + expect(stat(pid)).toContain(") Z "); const { boot, pidns } = self(); expect(ownerState(`${boot}:${pidns}:${pid}:${procStart(pid)}`)).toBe("dead"); } finally { diff --git a/tests/bugbash/sandbox/runner.ts b/tests/bugbash/sandbox/runner.ts index 52bc3da8d84..5fa279ad4f9 100644 --- a/tests/bugbash/sandbox/runner.ts +++ b/tests/bugbash/sandbox/runner.ts @@ -20,6 +20,8 @@ export const IMAGE = "ghcr.io/coder/xum-bugbash-sandbox"; const KILL_AFTER_MS = 5_000; /** How long the `docker run` client gets to exit after its lifeline (stdin) closed. */ const GRACE_MS = 60_000; +/** How long one stop hook gets before the lifeline closes anyway: a hung hook must not block it. */ +const HOOK_MS = 10_000; /** A short option, or a flag that names, labels or tracks the container: runJob() sets those. */ const CALLER_FLAG = /^(-[^-]|--(name|label|label-file|cidfile)(=|$))/; @@ -91,6 +93,7 @@ export class Session { #cleanup: Promise | null = null; #owned: Job | null = null; readonly #graceMs: number; + readonly #hookMs: number; readonly #log: (line: string) => void; #hooks: (() => unknown)[] = []; /** The `docker run` client, once spawned. A stop never signals it: it closes its stdin. */ @@ -105,6 +108,7 @@ export class Session { options: { root?: string; graceMs?: number; + hookMs?: number; log?: (line: string) => void; /** Injection point: runner.test.ts records the group signals; production uses killGroup. */ signalGroup?: (group: number, signal: NodeJS.Signals) => void; @@ -113,6 +117,7 @@ export class Session { this.#signalGroup = options.signalGroup ?? killGroup; this.#root = options.root ?? ROOT; this.#graceMs = options.graceMs ?? GRACE_MS; + this.#hookMs = options.hookMs ?? HOOK_MS; this.#log = options.log ?? ((line) => console.error(`sandbox ${line}`)); if (stop.aborted) this.#stop(String(stop.reason)); else stop.addEventListener("abort", () => this.#stop(String(stop.reason)), { once: true }); @@ -322,7 +327,7 @@ export class Session { /** The stop hooks first, then the lifeline; SIGKILL only for a client past the grace period. */ async #endJob() { - await Promise.allSettled(this.#hooks.splice(0).map(async (fn) => fn())); + await Promise.allSettled(this.#hooks.splice(0).map((fn) => this.#bounded(fn))); const client = this.#runClient; if (client == null || client.exitCode != null || client.signalCode != null) return; client.stdin?.end(); @@ -335,6 +340,22 @@ export class Session { client.once("close", () => clearTimeout(timer)); } + /** Runs one stop hook, but waits at most #hookMs for it. */ + async #bounded(fn: () => unknown) { + let timer: ReturnType | undefined; + const late = new Promise((resolve) => { + timer = setTimeout(() => { + this.#log(`a stop hook did not settle in ${this.#hookMs} ms: the lifeline closes anyway`); + resolve(); + }, this.#hookMs); + }); + try { + await Promise.race([(async () => fn())(), late]); + } finally { + clearTimeout(timer); + } + } + /** * Signals a tracked group only while it is still ours: it has a live member and its ID was not * reused (groupState). Other agents share this host, so an emptied group is never signalled.