From 2c0ddec1114fe88b8a6b70dfa17bb2bf246c26c4 Mon Sep 17 00:00:00 2001 From: Tim Nunamaker Date: Fri, 6 Nov 2026 16:27:13 -0600 Subject: [PATCH 01/15] test(scratch): reap groups a killed-owner test leaves so a failure ends red The two kill-on-purpose lifecycle tests started a command whose group nothing stopped when an assertion failed. The command inherited the wrapper's stdout and stderr pipes, so the test process never exited and the step hung until the job was cancelled instead of failing. On failure, stop every command group recorded under the test's scratch parent and close the test's end of the wrapper pipes. Assisted-by: AI Signed-off-by: Tim Nunamaker --- scripts/test-scratch/run-command.test.ts | 32 ++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/scripts/test-scratch/run-command.test.ts b/scripts/test-scratch/run-command.test.ts index 48f96c5bc..1fba4120f 100644 --- a/scripts/test-scratch/run-command.test.ts +++ b/scripts/test-scratch/run-command.test.ts @@ -1043,6 +1043,36 @@ function killQuietly(pid: number | undefined): void { } } +// Failure-path cleanup for a test whose owner or sidecar was killed on purpose: +// nothing else stops the process groups it left behind, and a leftover command +// that inherited the wrapper's stdout/stderr keeps those pipes open, so the test +// process never exits and the whole step hangs instead of ending red. This stops +// every group still recorded under a scratch parent and drops our pipe ends. +async function reapOwnedGroups(runnerTemp: string, wrapper?: ReturnType): Promise { + const parent = join(runnerTemp, "pdpp-test-scratch"); + const roots = (await readdir(parent).catch(() => [] as string[])).map((entry) => join(parent, entry)); + const recorded = await Promise.all( + roots.flatMap((root) => [ + readFile(join(root, ".pdpp-test-scratch.json"), "utf8") + .then((text) => String((JSON.parse(text) as { pgid?: unknown }).pgid)) + .catch(() => ""), + readFile(join(root, ".sandbox-child-group"), "utf8").catch(() => ""), + ]) + ); + for (const text of recorded) { + const pgid = Number(text.trim()); + if (Number.isSafeInteger(pgid) && pgid > 1) { + try { + process.kill(-pgid, "SIGKILL"); + } catch { + // The group already exited. + } + } + } + wrapper?.stdout?.destroy(); + wrapper?.stderr?.destroy(); +} + test("SIGINT waits for the sidecar to publish its group before the launcher cleans up", async () => { const runnerTemp = await temporaryParent(); const wrapper = spawn( @@ -1173,6 +1203,7 @@ test("a sidecar killed right after spawning the command does not orphan the comm } } finally { killQuietly(commandPid); + await reapOwnedGroups(runnerTemp); await rm(runnerTemp, { force: true, recursive: true }); await rm(work, { force: true, recursive: true }); } @@ -1224,6 +1255,7 @@ test("owner SIGKILL then a command that leaves a background child does not let r wrapper.kill("SIGKILL"); } killQuietly(backgroundPid); + await reapOwnedGroups(runnerTemp, wrapper); await rm(runnerTemp, { force: true, recursive: true }); await rm(work, { force: true, recursive: true }); } From 388ff4d99753cc6486a8c1d8e27689581e3358e0 Mon Sep 17 00:00:00 2001 From: Tim Nunamaker Date: Fri, 6 Nov 2026 16:29:06 -0600 Subject: [PATCH 02/15] test(scratch): keep helper files where the sandbox can see them On images that ship bubblewrap (Ubuntu 26.04 runners) the sandbox mounts a private /tmp and binds back only the scratch parent's top directory and the working directory. Two lifecycle tests put the files their command writes in a second, sibling temp directory, so the command could not create them and the tests waited out their 20 s read timeout. On ubuntu-24.04, bubblewrap is absent, the sandbox adds no private /tmp, and the same tests passed. Create that directory inside the test's own runner temp directory, which the sandbox binds back. Assisted-by: AI Signed-off-by: Tim Nunamaker --- scripts/test-scratch/run-command.test.ts | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/scripts/test-scratch/run-command.test.ts b/scripts/test-scratch/run-command.test.ts index 1fba4120f..ab9765995 100644 --- a/scripts/test-scratch/run-command.test.ts +++ b/scripts/test-scratch/run-command.test.ts @@ -1175,7 +1175,10 @@ test("a sandbox that exits before publishing a command group leaves no scratch a test("a sidecar killed right after spawning the command does not orphan the command under a deleted scratch", async () => { const runnerTemp = await temporaryParent(); - const work = await temporaryParent(); + // The command runs under a private /tmp where bwrap binds back only the scratch + // parent's top directory; a sibling temp directory would be invisible to it. + const work = join(runnerTemp, "work"); + await mkdir(work); const recordFile = join(work, "command.record"); let commandPid: number | undefined; try { @@ -1205,13 +1208,15 @@ test("a sidecar killed right after spawning the command does not orphan the comm killQuietly(commandPid); await reapOwnedGroups(runnerTemp); await rm(runnerTemp, { force: true, recursive: true }); - await rm(work, { force: true, recursive: true }); } }); test("owner SIGKILL then a command that leaves a background child does not let recovery delete a live group's scratch", async () => { const runnerTemp = await temporaryParent(); - const work = await temporaryParent(); + // The command runs under a private /tmp where bwrap binds back only the scratch + // parent's top directory; a sibling temp directory would be invisible to it. + const work = join(runnerTemp, "work"); + await mkdir(work); const pidFile = join(work, "background.pid"); const releaseFile = join(work, "release"); let backgroundPid: number | undefined; @@ -1257,7 +1262,6 @@ test("owner SIGKILL then a command that leaves a background child does not let r killQuietly(backgroundPid); await reapOwnedGroups(runnerTemp, wrapper); await rm(runnerTemp, { force: true, recursive: true }); - await rm(work, { force: true, recursive: true }); } }); From c1710428836c2b1783655ab3278df98192e41e4f Mon Sep 17 00:00:00 2001 From: Tim Nunamaker Date: Fri, 6 Nov 2026 16:29:06 -0600 Subject: [PATCH 03/15] ci(scratch): bound the lifecycle oracle step The oracle file normally finishes in under a minute. A leaked process once kept it open until the 10-minute job limit was cancelled by hand. Bound the step so a hang fails the step on its own. Assisted-by: AI Signed-off-by: Tim Nunamaker --- .github/workflows/test-scratch-lifecycle.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/test-scratch-lifecycle.yml b/.github/workflows/test-scratch-lifecycle.yml index f229a76b1..267e2ee40 100644 --- a/.github/workflows/test-scratch-lifecycle.yml +++ b/.github/workflows/test-scratch-lifecycle.yml @@ -77,6 +77,8 @@ jobs: run: pnpm exec tsc --project scripts/test-scratch/tsconfig.json --pretty false - name: Lifecycle owner oracle + # The whole file runs in well under a minute; a hung process must fail the step, not the job. + timeout-minutes: 5 run: pnpm exec tsx --test scripts/test-scratch/run-command.test.ts - name: Canonical-entrypoint ratchet From f891198777438ca71b9fc489ea470c932c0ba5ca Mon Sep 17 00:00:00 2001 From: Tim Nunamaker Date: Fri, 6 Nov 2026 16:57:44 -0600 Subject: [PATCH 04/15] test(scratch): route every lifecycle-oracle launch through one fixture The two mistakes that hung the oracle on a bubblewrap runner could recur in any test of this file: naming a helper path the sandbox's private /tmp hides, and leaving a command's process group running after a failed assertion. LifecycleFixture now owns the runner temp directory, rejects arguments and variables that name a host temp path outside it, and in the test's after hook kills the wrappers it started, closes their pipes and kills every process group recorded under its scratch root. The per-test reaper and the manual rm calls go away; a ratchet test fails if a test spawns the wrapper directly. Assisted-by: AI Signed-off-by: Tim Nunamaker --- .../fixtures/lifecycle-fixture.ts | 195 ++++ scripts/test-scratch/run-command.test.ts | 877 ++++++++---------- 2 files changed, 575 insertions(+), 497 deletions(-) create mode 100644 scripts/test-scratch/fixtures/lifecycle-fixture.ts diff --git a/scripts/test-scratch/fixtures/lifecycle-fixture.ts b/scripts/test-scratch/fixtures/lifecycle-fixture.ts new file mode 100644 index 000000000..e542d9226 --- /dev/null +++ b/scripts/test-scratch/fixtures/lifecycle-fixture.ts @@ -0,0 +1,195 @@ +// Copyright The PDP-Connect Contributors +// SPDX-License-Identifier: Apache-2.0 + +// The one place the lifecycle oracle creates temporary directories for, launches, +// and cleans up after a command that runs through the scratch wrapper. +// +// Two mistakes broke this suite on a runner image that ships bubblewrap, and this +// fixture makes both of them impossible rather than something each test remembers: +// +// 1. Path visibility. `scripts/test-sandbox/test-sandbox` gives the command a +// private /tmp and binds back only the top directory of its scratch root. A +// helper file in any other directory under /tmp is invisible to the command. +// The fixture hands out paths only inside `runnerTemp` (the scratch root's top +// directory) and `spawn` rejects any argument or environment value that names +// another path under a host temporary directory. +// 2. Leaked groups. A test that kills an owner on purpose leaves the command's +// process group running if an assertion then fails. That group still holds the +// stdout/stderr pipes of the wrapper, so the test process never exits and the +// CI step hangs instead of ending red. The fixture closes every pipe it opened, +// kills every wrapper it started, and kills every process group recorded under +// its scratch root when the test ends, whatever the outcome. + +import { type ChildProcess, spawn as nodeSpawn, type SpawnOptions } from "node:child_process"; +import { mkdir, mkdtemp, readdir, readFile, rm } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { isAbsolute, join, relative, resolve } from "node:path"; +import type { TestContext } from "node:test"; + +export interface LifecycleFixture { + /** Point this process's `RUNNER_TEMP` at `runnerTemp` until the test ends, for in-process `runScratchCommand`. */ + adoptAsRunnerTemp: () => void; + /** Create `name` inside `runnerTemp` and return its path. */ + dir: (name: string) => Promise; + /** `process.env` with `RUNNER_TEMP` pointing at `runnerTemp`, plus `overrides`. */ + env: (overrides?: NodeJS.ProcessEnv) => NodeJS.ProcessEnv; + /** A path inside `runnerTemp`; throws for anything that would resolve outside it. */ + path: (...segments: string[]) => string; + /** The only directory tree a command launched through this fixture may name. */ + readonly runnerTemp: string; + /** Where the wrapper allocates its per-run scratch roots. */ + readonly scratchParent: string; + /** + * Start a process that runs the wrapper, or a stand-in for it. The process is + * guarded (see `assertCommandSeesOnlyBoundPaths`) and killed with its pipes + * closed when the test ends. + */ + spawn: (file: string, args: readonly string[], spawnOptions?: SpawnOptions) => ChildProcess; +} + +// Host directories whose contents a bwrap-private /tmp hides. A value that names +// one of these but is not inside `runnerTemp` is the mistake the guard rejects. +function hostTemporaryRoots(): string[] { + return [...new Set([tmpdir(), "/tmp", "/var/tmp"].map((root) => resolve(root)))]; +} + +function isInside(root: string, candidate: string): boolean { + const rel = relative(root, candidate); + return rel === "" || !(rel.startsWith("..") || isAbsolute(rel)); +} + +/** Absolute-path tokens in one argument or value: a PATH-style list or `--flag=/path` yields each path. */ +const PATH_TOKEN_SEPARATOR = /[:=]/; + +function pathTokens(value: string): string[] { + return value.split(PATH_TOKEN_SEPARATOR).filter((token) => token.startsWith("/")); +} + +/** + * Throw if any argument or non-inherited environment value names a path under a + * host temporary directory that is outside `runnerTemp`. The sandbox would hide + * that path from the command, so a test using it fails by timing out. + */ +export function assertCommandSeesOnlyBoundPaths( + runnerTemp: string, + args: readonly string[], + env: NodeJS.ProcessEnv +): void { + const inherited = process.env; + const values = [ + ...args, + ...Object.entries(env) + .filter(([key, value]) => value !== undefined && value !== inherited[key]) + .map(([, value]) => value as string), + ]; + const roots = hostTemporaryRoots(); + for (const value of values) { + for (const token of pathTokens(value)) { + const path = resolve(token); + if (roots.some((root) => isInside(root, path)) && !isInside(runnerTemp, path)) { + throw new Error( + `${token} is outside the bound runner temp ${runnerTemp}; the sandbox's private /tmp hides it from the command. ` + + "Create it with fixture.dir() or fixture.path()." + ); + } + } + } +} + +async function recordedGroups(scratchParent: string): Promise { + const roots = (await readdir(scratchParent).catch(() => [] as string[])).map((entry) => join(scratchParent, entry)); + const texts = await Promise.all( + roots.flatMap((root) => [ + readFile(join(root, ".pdpp-test-scratch.json"), "utf8") + .then((text) => String((JSON.parse(text) as { pgid?: unknown }).pgid)) + .catch(() => ""), + readFile(join(root, ".sandbox-child-group"), "utf8").catch(() => ""), + ]) + ); + return texts + .map((text) => Number(text.trim())) + .filter((pgid) => Number.isSafeInteger(pgid) && pgid > 1 && pgid !== process.pid && pgid !== process.ppid); +} + +function killQuietly(pid: number): void { + try { + process.kill(pid, "SIGKILL"); + } catch { + // The process already exited. + } +} + +/** Create a fixture for one test; everything it handed out is cleaned up in the test's `after` hook. */ +export async function lifecycleFixture( + t: TestContext, + options: { /** Parent of `runnerTemp`; defaults to the host temporary directory. */ under?: string } = {} +): Promise { + const runnerTemp = await mkdtemp(join(options.under ?? tmpdir(), "pdpp-test-scratch-test-")); + const scratchParent = join(runnerTemp, "pdpp-test-scratch"); + const children: ChildProcess[] = []; + const restoreEnv: Array<() => void> = []; + + const path = (...segments: string[]): string => { + const resolved = resolve(runnerTemp, ...segments); + if (!isInside(runnerTemp, resolved)) { + throw new Error(`${resolved} is outside the bound runner temp ${runnerTemp}`); + } + return resolved; + }; + const env = (overrides: NodeJS.ProcessEnv = {}): NodeJS.ProcessEnv => ({ + ...process.env, + ...overrides, + RUNNER_TEMP: runnerTemp, + }); + + t.after(async () => { + for (const restore of restoreEnv.reverse()) { + restore(); + } + for (const proc of children) { + proc.stdin?.destroy(); + proc.stdout?.destroy(); + proc.stderr?.destroy(); + if (proc.pid !== undefined && proc.exitCode === null && proc.signalCode === null) { + killQuietly(proc.pid); + } + } + for (const pgid of await recordedGroups(scratchParent)) { + killQuietly(-pgid); + } + await rm(runnerTemp, { force: true, recursive: true }); + }); + + return { + runnerTemp, + scratchParent, + path, + env, + async dir(name) { + const target = path(name); + await mkdir(target, { recursive: true }); + return target; + }, + spawn(file, args, spawnOptions = {}) { + const launchEnv = spawnOptions.env ?? env(); + if (launchEnv.RUNNER_TEMP !== runnerTemp) { + throw new Error(`RUNNER_TEMP must be the fixture's runner temp ${runnerTemp}; got ${launchEnv.RUNNER_TEMP}`); + } + assertCommandSeesOnlyBoundPaths(runnerTemp, args, launchEnv); + const proc = nodeSpawn(file, args, { ...spawnOptions, env: launchEnv }); + children.push(proc); + return proc; + }, + adoptAsRunnerTemp() { + const previous = process.env.RUNNER_TEMP; + process.env.RUNNER_TEMP = runnerTemp; + restoreEnv.push(() => { + if (previous === undefined) { + delete process.env.RUNNER_TEMP; + } else { + process.env.RUNNER_TEMP = previous; + } + }); + }, + }; +} diff --git a/scripts/test-scratch/run-command.test.ts b/scripts/test-scratch/run-command.test.ts index ab9765995..7655c8dd2 100644 --- a/scripts/test-scratch/run-command.test.ts +++ b/scripts/test-scratch/run-command.test.ts @@ -21,7 +21,12 @@ import { } from "node:fs/promises"; import { platform, tmpdir } from "node:os"; import { join, posix, win32 } from "node:path"; -import test from "node:test"; +import test, { type TestContext } from "node:test"; +import { + assertCommandSeesOnlyBoundPaths, + type LifecycleFixture, + lifecycleFixture, +} from "./fixtures/lifecycle-fixture.ts"; import { allocateScratchOwnership, cleanupScratchOwnership, @@ -57,20 +62,31 @@ interface ChildResult { signal: NodeJS.Signals | null; } -function startWrapper(args: string[], env: NodeJS.ProcessEnv = process.env) { - return spawn(process.execPath, ["--import", "tsx", wrapperPath, ...command(args)], { +// Every launch of the wrapper goes through a LifecycleFixture: it keeps the paths a +// sandboxed command touches inside the bound runner temp and reaps what the test leaves. +function startWrapper(fx: LifecycleFixture, args: string[], env: NodeJS.ProcessEnv = fx.env()) { + return fx.spawn(process.execPath, ["--import", "tsx", wrapperPath, ...command(args)], { env, stdio: ["ignore", "pipe", "ignore"], }); } -function startRawWrapper(argv: string[], env: NodeJS.ProcessEnv = process.env) { - return spawn(process.execPath, ["--import", "tsx", wrapperPath, "--", ...argv], { +function startRawWrapper(fx: LifecycleFixture, argv: string[], env: NodeJS.ProcessEnv = fx.env()) { + return fx.spawn(process.execPath, ["--import", "tsx", wrapperPath, "--", ...argv], { env, stdio: ["ignore", "pipe", "pipe"], }); } +// The launch must allocate its own owner, not join one this process inherited. +const NO_INHERITED_OWNER = { + PDPP_TEST_SCRATCH_ROOT: undefined, + PDPP_TEST_SCRATCH_SCHEMA: undefined, + PDPP_TEST_SCRATCH_MARKER: undefined, + PDPP_TEST_SCRATCH_NONCE: undefined, + PDPP_TEST_SCRATCH_OWNER_PID: undefined, +}; + // Each sandbox these tests spawn must start from its own defaults, whatever // TEST_SANDBOX_* knobs the caller's shell or an outer run has set. const FRESH_SANDBOX_CONTEXT = { @@ -85,43 +101,33 @@ const FRESH_SANDBOX_CONTEXT = { const sandboxBashMajor = Number(spawnSync("bash", ["-c", "echo $BASH_VERSINFO"], { encoding: "utf8" }).stdout.trim()); const sandboxContract = { skip: sandboxBashMajor >= 5 ? false : "test-sandbox needs bash 5 to enforce its contract" }; -test("owner routes children through the sandbox unless explicitly disabled", sandboxContract, async () => { - const runnerTemp = await temporaryParent(); +test("owner routes children through the sandbox unless explicitly disabled", sandboxContract, async (t) => { + const fx = await lifecycleFixture(t); const probe = [ process.execPath, "-e", "console.log(JSON.stringify({active:process.env.TEST_SANDBOX_ACTIVE||null,tmp:process.env.TMPDIR,mode:require('node:fs').statSync(process.env.TMPDIR).mode%0o1000}))", ]; - try { - for (const disabled of [false, true]) { - const env = { - ...process.env, - PDPP_TEST_SCRATCH_ROOT: undefined, - PDPP_TEST_SCRATCH_SCHEMA: undefined, - PDPP_TEST_SCRATCH_MARKER: undefined, - PDPP_TEST_SCRATCH_NONCE: undefined, - PDPP_TEST_SCRATCH_OWNER_PID: undefined, - RUNNER_TEMP: runnerTemp, - ...FRESH_SANDBOX_CONTEXT, - TEST_SANDBOX_DISABLE: disabled ? "1" : "0", - }; - const wrapper = startRawWrapper(probe, env); - let stdout = ""; - wrapper.stdout?.on("data", (chunk: Buffer) => { - stdout += chunk.toString(); - }); - // biome-ignore lint/performance/noAwaitInLoops: exercise enabled and disabled launches sequentially. - assert.deepEqual(await closed(wrapper), { code: 0, signal: null }); - const result = JSON.parse(stdout.trim()) as { active: string | null; tmp: string; mode: number }; - assert.equal(result.active, disabled ? null : "1"); - if (!disabled) { - assert.equal(result.mode, 0o700); - } - assert.ok(result.tmp.includes("pdpp-test-scratch")); - await assert.rejects(access(result.tmp)); + for (const disabled of [false, true]) { + const env = fx.env({ + ...NO_INHERITED_OWNER, + ...FRESH_SANDBOX_CONTEXT, + TEST_SANDBOX_DISABLE: disabled ? "1" : "0", + }); + const wrapper = startRawWrapper(fx, probe, env); + let stdout = ""; + wrapper.stdout?.on("data", (chunk: Buffer) => { + stdout += chunk.toString(); + }); + // biome-ignore lint/performance/noAwaitInLoops: exercise enabled and disabled launches sequentially. + assert.deepEqual(await closed(wrapper), { code: 0, signal: null }); + const result = JSON.parse(stdout.trim()) as { active: string | null; tmp: string; mode: number }; + assert.equal(result.active, disabled ? null : "1"); + if (!disabled) { + assert.equal(result.mode, 0o700); } - } finally { - await rm(runnerTemp, { force: true, recursive: true }); + assert.ok(result.tmp.includes("pdpp-test-scratch")); + await assert.rejects(access(result.tmp)); } }); @@ -154,48 +160,34 @@ test("sandbox creates a missing --scratch root 0700 once and never re-creates it } }); -test("sandbox preserves literal shell variable syntax in child argv", async () => { - const runnerTemp = await temporaryParent(); - try { - // biome-ignore lint/suspicious/noTemplateCurlyInString: exercise literal shell syntax across the launcher boundary. - const wrapper = startRawWrapper(["bash", "-c", 'X=ok; printf %s "${X}"'], { - ...process.env, - PDPP_TEST_SCRATCH_ROOT: undefined, - PDPP_TEST_SCRATCH_SCHEMA: undefined, - PDPP_TEST_SCRATCH_MARKER: undefined, - PDPP_TEST_SCRATCH_NONCE: undefined, - PDPP_TEST_SCRATCH_OWNER_PID: undefined, - RUNNER_TEMP: runnerTemp, - ...FRESH_SANDBOX_CONTEXT, - TEST_SANDBOX_DISABLE: "0", - }); - let output = ""; - wrapper.stdout?.on("data", (chunk: Buffer) => { - output += chunk.toString(); - }); - assert.deepEqual(await closed(wrapper), { code: 0, signal: null }); - assert.equal(output, "ok"); - } finally { - await rm(runnerTemp, { force: true, recursive: true }); - } +test("sandbox preserves literal shell variable syntax in child argv", async (t) => { + const fx = await lifecycleFixture(t); + // biome-ignore lint/suspicious/noTemplateCurlyInString: exercise literal shell syntax across the launcher boundary. + const wrapper = startRawWrapper(fx, ["bash", "-c", 'X=ok; printf %s "${X}"'], { + ...fx.env(), + ...NO_INHERITED_OWNER, + ...FRESH_SANDBOX_CONTEXT, + TEST_SANDBOX_DISABLE: "0", + }); + let output = ""; + wrapper.stdout?.on("data", (chunk: Buffer) => { + output += chunk.toString(); + }); + assert.deepEqual(await closed(wrapper), { code: 0, signal: null }); + assert.equal(output, "ok"); }); -test("normal sandbox exit stops a background command child before scratch cleanup", async () => { - const runnerTemp = await temporaryParent(); +test("normal sandbox exit stops a background command child before scratch cleanup", async (t) => { + const fx = await lifecycleFixture(t); const wrapper = startRawWrapper( + fx, ["bash", "-c", 'sleep 300 /dev/null 2>&1 & printf "%s\\n%s\\n" "$!" "$TMPDIR"'], - { - ...process.env, - PDPP_TEST_SCRATCH_ROOT: undefined, - PDPP_TEST_SCRATCH_SCHEMA: undefined, - PDPP_TEST_SCRATCH_MARKER: undefined, - PDPP_TEST_SCRATCH_NONCE: undefined, - PDPP_TEST_SCRATCH_OWNER_PID: undefined, - RUNNER_TEMP: runnerTemp, + fx.env({ + ...NO_INHERITED_OWNER, TEST_RECEIPT_ACTIVE: "1", TEST_SANDBOX_ACTIVE: "1", TEST_SANDBOX_DISABLE: "0", - } + }) ); let output = ""; wrapper.stdout?.on("data", (chunk: Buffer) => { @@ -212,56 +204,37 @@ test("normal sandbox exit stops a background command child before scratch cleanu await eventuallyAbsentProcess(backgroundPid); await assert.rejects(access(root)); } finally { - if (backgroundPid) { - try { - process.kill(backgroundPid, "SIGKILL"); - } catch { - // The launcher may already have stopped the child. - } - } - await rm(runnerTemp, { force: true, recursive: true }); + killQuietly(backgroundPid); } }); test("sandbox sees its owned scratch root below /tmp", { skip: process.platform !== "linux", -}, async () => { - const runnerTemp = await mkdtemp(join("/tmp", "pdpp-test-sandbox-tmp-")); - try { - const wrapper = startRawWrapper( - [ - process.execPath, - "-e", - 'const fs=require("node:fs");const root=process.env.TMPDIR;fs.writeFileSync(root+"/visible","ok");console.log(JSON.stringify({root,active:process.env.TEST_SANDBOX_ACTIVE}))', - ], - { - ...process.env, - PDPP_TEST_SCRATCH_ROOT: undefined, - PDPP_TEST_SCRATCH_SCHEMA: undefined, - PDPP_TEST_SCRATCH_MARKER: undefined, - PDPP_TEST_SCRATCH_NONCE: undefined, - PDPP_TEST_SCRATCH_OWNER_PID: undefined, - RUNNER_TEMP: runnerTemp, - ...FRESH_SANDBOX_CONTEXT, - TEST_SANDBOX_DISABLE: "0", - } - ); - let output = ""; - wrapper.stdout?.on("data", (chunk: Buffer) => { - output += chunk.toString(); - }); - assert.deepEqual(await closed(wrapper), { code: 0, signal: null }); - const result = JSON.parse(output.trim()) as { root: string; active: string }; - assert.equal(result.active, "1"); - assert.ok(result.root.startsWith(`${runnerTemp}/pdpp-test-scratch/run-`)); - await assert.rejects(access(result.root)); - } finally { - await rm(runnerTemp, { force: true, recursive: true }); - } +}, async (t) => { + // The case under test is a scratch root directly below /tmp, whatever TMPDIR is. + const fx = await lifecycleFixture(t, { under: "/tmp" }); + const wrapper = startRawWrapper( + fx, + [ + process.execPath, + "-e", + 'const fs=require("node:fs");const root=process.env.TMPDIR;fs.writeFileSync(root+"/visible","ok");console.log(JSON.stringify({root,active:process.env.TEST_SANDBOX_ACTIVE}))', + ], + fx.env({ ...NO_INHERITED_OWNER, ...FRESH_SANDBOX_CONTEXT, TEST_SANDBOX_DISABLE: "0" }) + ); + let output = ""; + wrapper.stdout?.on("data", (chunk: Buffer) => { + output += chunk.toString(); + }); + assert.deepEqual(await closed(wrapper), { code: 0, signal: null }); + const result = JSON.parse(output.trim()) as { root: string; active: string }; + assert.equal(result.active, "1"); + assert.ok(result.root.startsWith(`${fx.runnerTemp}/pdpp-test-scratch/run-`)); + await assert.rejects(access(result.root)); }); -function startLaunchingWrapper(env: NodeJS.ProcessEnv = process.env) { - return spawn(process.execPath, ["--import", "tsx", lifecycleWrapperPath], { +function startLaunchingWrapper(fx: LifecycleFixture, env: NodeJS.ProcessEnv = fx.env()) { + return fx.spawn(process.execPath, ["--import", "tsx", lifecycleWrapperPath], { env, stdio: ["ignore", "pipe", "ignore"], }); @@ -514,8 +487,8 @@ test("cleanup refuses a live recorded process group before quarantine", async () } }); -test("participant creates no nested owner", async () => { - const parent = await temporaryParent(); +test("participant creates no nested owner", async (t) => { + const { runnerTemp: parent } = await lifecycleFixture(t); const ownership = await allocateScratchOwnership({ parent }); const keys = Object.keys(ownership.env) as Array; const before = new Map(keys.map((key) => [key, process.env[key]])); @@ -532,90 +505,60 @@ test("participant creates no nested owner", async () => { } } await cleanupScratchOwnership(ownership); - await rm(parent, { force: true, recursive: true }); } }); -test("participant validation fails closed when any inherited temporary path is altered", async () => { - const parent = await temporaryParent(); - const outside = await temporaryParent(); - const ownership = await allocateScratchOwnership({ parent }); +test("participant validation fails closed when any inherited temporary path is altered", async (t) => { + const fx = await lifecycleFixture(t); + const outside = await fx.dir("outside"); + const ownership = await allocateScratchOwnership({ parent: await fx.dir("parent") }); try { const altered = { ...ownership.env, TMPDIR: outside }; await assert.rejects(inheritedScratchOwnership(altered), { name: "ScratchOwnershipError", reason: "invalid-inherited-ownership", }); - const nested = startRawWrapper([process.execPath, "--import", "tsx", child, "--exit=0"], altered); + const nested = startRawWrapper(fx, [process.execPath, "--import", "tsx", child, "--exit=0"], fx.env(altered)); assert.deepEqual(await closed(nested), { code: 1, signal: null }); await assert.rejects(access(join(outside, "child.txt"))); await access(ownership.allocation.root); } finally { await cleanupScratchOwnership(ownership); - await rm(parent, { force: true, recursive: true }); - await rm(outside, { force: true, recursive: true }); } }); -test("launch failure removes its allocated root and returns the infrastructure exit", async () => { - const runnerTemp = await temporaryParent(); - const parent = join(runnerTemp, "pdpp-test-scratch"); - try { - const wrapper = startRawWrapper(["/definitely-not-a-pdpp-command"], { ...process.env, RUNNER_TEMP: runnerTemp }); - assert.deepEqual(await closed(wrapper), { code: 74, signal: null }); - assert.deepEqual(await readdir(parent), []); - } finally { - await rm(runnerTemp, { force: true, recursive: true }); - } +test("launch failure removes its allocated root and returns the infrastructure exit", async (t) => { + const fx = await lifecycleFixture(t); + const wrapper = startRawWrapper(fx, ["/definitely-not-a-pdpp-command"]); + assert.deepEqual(await closed(wrapper), { code: 74, signal: null }); + assert.deepEqual(await readdir(fx.scratchParent), []); }); -test("an unproven group shutdown retains scratch and makes a successful command infrastructure-failed", async () => { - const runnerTemp = await temporaryParent(); - const parent = join(runnerTemp, "pdpp-test-scratch"); - const previousRunnerTemp = process.env.RUNNER_TEMP; - process.env.RUNNER_TEMP = runnerTemp; - try { - const result = await runScratchCommand(command(["--exit=0"]), { stopGroup: async () => false }); - assert.deepEqual(result, { code: 74, signal: null }); - const roots = (await readdir(parent)).filter((entry) => entry.startsWith("run-")); - assert.equal(roots.length, 1); - } finally { - if (previousRunnerTemp === undefined) { - delete process.env.RUNNER_TEMP; - } else { - process.env.RUNNER_TEMP = previousRunnerTemp; - } - await rm(runnerTemp, { force: true, recursive: true }); - } +test("an unproven group shutdown retains scratch and makes a successful command infrastructure-failed", async (t) => { + const fx = await lifecycleFixture(t); + fx.adoptAsRunnerTemp(); + const result = await runScratchCommand(command(["--exit=0"]), { stopGroup: async () => false }); + assert.deepEqual(result, { code: 74, signal: null }); + const roots = (await readdir(fx.scratchParent)).filter((entry) => entry.startsWith("run-")); + assert.equal(roots.length, 1); }); -test("a rejected group proof retains scratch but preserves the child result", async () => { - const runnerTemp = await temporaryParent(); - const parent = join(runnerTemp, "pdpp-test-scratch"); - const previousRunnerTemp = process.env.RUNNER_TEMP; - process.env.RUNNER_TEMP = runnerTemp; - try { - for (const exitCode of [0, 42]) { - // biome-ignore lint/performance/noAwaitInLoops: each case must retain and reap its own live group before the next case. - const result = await runScratchCommand(["--", "sh", "-c", `sleep 30 & exit ${exitCode}`], { - stopGroup: (pgid) => { - process.kill(-pgid, "SIGKILL"); - throw new Error("simulate rejected group proof"); - }, - }); - assert.deepEqual(result, { code: exitCode === 0 ? 74 : exitCode, signal: null }); - const roots = (await readdir(parent)).filter((entry) => entry.startsWith("run-")); - assert.equal(roots.length, 1); - const root = join(parent, roots[0] as string); - await rm(root, { force: true, recursive: true }); - } - } finally { - if (previousRunnerTemp === undefined) { - delete process.env.RUNNER_TEMP; - } else { - process.env.RUNNER_TEMP = previousRunnerTemp; - } - await rm(runnerTemp, { force: true, recursive: true }); +test("a rejected group proof retains scratch but preserves the child result", async (t) => { + const fx = await lifecycleFixture(t); + fx.adoptAsRunnerTemp(); + for (const exitCode of [0, 42]) { + // biome-ignore lint/performance/noAwaitInLoops: each case must retain and reap its own live group before the next case. + const result = await runScratchCommand(["--", "sh", "-c", `sleep 30 & exit ${exitCode}`], { + stopGroup: (pgid) => { + process.kill(-pgid, "SIGKILL"); + throw new Error("simulate rejected group proof"); + }, + }); + assert.deepEqual(result, { code: exitCode === 0 ? 74 : exitCode, signal: null }); + const roots = (await readdir(fx.scratchParent)).filter((entry) => entry.startsWith("run-")); + assert.equal(roots.length, 1); + const root = join(fx.scratchParent, roots[0] as string); + await rm(root, { force: true, recursive: true }); } }); @@ -927,28 +870,18 @@ test("cleanup rejects a forged nonce before constructing a quarantine target", a } }); -test("CLI child signal remains a signal to its parent", async () => { - const result = await new Promise<{ code: number | null; signal: NodeJS.Signals | null }>((resolve, reject) => { - const proc = spawn( - process.execPath, - ["--import", "tsx", new URL("./run-command.ts", import.meta.url).pathname, ...command(["--self-signal=SIGTERM"])], - { - stdio: "ignore", - } - ); - proc.once("error", reject); - proc.once("close", (code, signal) => resolve({ code, signal })); +test("CLI child signal remains a signal to its parent", async (t) => { + const fx = await lifecycleFixture(t); + const proc = fx.spawn(process.execPath, ["--import", "tsx", wrapperPath, ...command(["--self-signal=SIGTERM"])], { + stdio: "ignore", }); - assert.deepEqual(result, { code: null, signal: "SIGTERM" }); + assert.deepEqual(await closed(proc), { code: null, signal: "SIGTERM" }); }); -test("SIGTERM reaches the owned group and cleanup completes before wrapper termination", async () => { +test("SIGTERM reaches the owned group and cleanup completes before wrapper termination", async (t) => { + const fx = await lifecycleFixture(t); let output = ""; - const wrapper = spawn( - process.execPath, - ["--import", "tsx", new URL("./run-command.ts", import.meta.url).pathname, ...command(["--print-root", "--wait"])], - { stdio: ["ignore", "pipe", "ignore"] } - ); + const wrapper = startWrapper(fx, ["--print-root", "--wait"]); wrapper.stdout?.on("data", (chunk: Buffer) => { output += chunk.toString(); }); @@ -973,18 +906,10 @@ test("SIGTERM reaches the owned group and cleanup completes before wrapper termi await assert.rejects(access(root)); }); -test("SIGINT is forwarded unchanged to the owned group before cleanup", async () => { +test("SIGINT is forwarded unchanged to the owned group before cleanup", async (t) => { + const fx = await lifecycleFixture(t); let output = ""; - const wrapper = spawn( - process.execPath, - [ - "--import", - "tsx", - new URL("./run-command.ts", import.meta.url).pathname, - ...command(["--print-root", "--record-signals", "--wait"]), - ], - { stdio: ["ignore", "pipe", "ignore"] } - ); + const wrapper = startWrapper(fx, ["--print-root", "--record-signals", "--wait"]); wrapper.stdout?.on("data", (chunk: Buffer) => { output += chunk.toString(); }); @@ -1013,19 +938,14 @@ test("SIGINT is forwarded unchanged to the owned group before cleanup", async () }); /** Environment for a sandbox with its cgroup layer absent, as under test-receipt or without a systemd user manager. */ -function noCgroupEnv(runnerTemp: string): NodeJS.ProcessEnv { - return { - ...process.env, - PDPP_TEST_SCRATCH_ROOT: undefined, - PDPP_TEST_SCRATCH_SCHEMA: undefined, - PDPP_TEST_SCRATCH_MARKER: undefined, - PDPP_TEST_SCRATCH_NONCE: undefined, - PDPP_TEST_SCRATCH_OWNER_PID: undefined, - RUNNER_TEMP: runnerTemp, +function noCgroupEnv(fx: LifecycleFixture, overrides: NodeJS.ProcessEnv = {}): NodeJS.ProcessEnv { + return fx.env({ + ...NO_INHERITED_OWNER, TEST_RECEIPT_ACTIVE: "1", ...FRESH_SANDBOX_CONTEXT, TEST_SANDBOX_DISABLE: "0", - }; + ...overrides, + }); } function processAlive(pid: number): boolean { @@ -1043,39 +963,9 @@ function killQuietly(pid: number | undefined): void { } } -// Failure-path cleanup for a test whose owner or sidecar was killed on purpose: -// nothing else stops the process groups it left behind, and a leftover command -// that inherited the wrapper's stdout/stderr keeps those pipes open, so the test -// process never exits and the whole step hangs instead of ending red. This stops -// every group still recorded under a scratch parent and drops our pipe ends. -async function reapOwnedGroups(runnerTemp: string, wrapper?: ReturnType): Promise { - const parent = join(runnerTemp, "pdpp-test-scratch"); - const roots = (await readdir(parent).catch(() => [] as string[])).map((entry) => join(parent, entry)); - const recorded = await Promise.all( - roots.flatMap((root) => [ - readFile(join(root, ".pdpp-test-scratch.json"), "utf8") - .then((text) => String((JSON.parse(text) as { pgid?: unknown }).pgid)) - .catch(() => ""), - readFile(join(root, ".sandbox-child-group"), "utf8").catch(() => ""), - ]) - ); - for (const text of recorded) { - const pgid = Number(text.trim()); - if (Number.isSafeInteger(pgid) && pgid > 1) { - try { - process.kill(-pgid, "SIGKILL"); - } catch { - // The group already exited. - } - } - } - wrapper?.stdout?.destroy(); - wrapper?.stderr?.destroy(); -} - -test("SIGINT waits for the sidecar to publish its group before the launcher cleans up", async () => { - const runnerTemp = await temporaryParent(); - const wrapper = spawn( +test("SIGINT waits for the sidecar to publish its group before the launcher cleans up", async (t) => { + const fx = await lifecycleFixture(t); + const wrapper = fx.spawn( process.execPath, [ "--import", @@ -1090,7 +980,7 @@ test("SIGINT waits for the sidecar to publish its group before the launcher clea "--record-signals", "--wait", ], - { env: noCgroupEnv(runnerTemp), stdio: ["ignore", "ignore", "ignore"] } + { env: noCgroupEnv(fx), stdio: ["ignore", "ignore", "ignore"] } ); const result = closed(wrapper); let settled = false; @@ -1100,8 +990,7 @@ test("SIGINT waits for the sidecar to publish its group before the launcher clea let sidecarPgid: number | undefined; let groupPath: string | undefined; try { - const parent = join(runnerTemp, "pdpp-test-scratch"); - const root = await eventuallyRunRoot(parent); + const root = await eventuallyRunRoot(fx.scratchParent); groupPath = join(root, ".sandbox-child-group"); sidecarPgid = Number((await eventuallyRead(`${groupPath}.held`, 20_000)).trim()); assert.ok(Number.isSafeInteger(sidecarPgid) && sidecarPgid > 0); @@ -1132,61 +1021,46 @@ test("SIGINT waits for the sidecar to publish its group before the launcher clea wrapper.kill("SIGTERM"); await result; } - await rm(runnerTemp, { force: true, recursive: true }); } }); -test("a sandbox that exits before publishing a command group leaves no scratch and propagates its status", async () => { - const runnerTemp = await temporaryParent(); - const stubs = await temporaryParent(); - try { - // A bwrap that cannot create namespaces makes TEST_SANDBOX_REQUIRE=1 exit 3 before the command starts. - await writeFile(join(stubs, "bwrap"), "#!/bin/sh\nexit 1\n", { mode: 0o755 }); - const wrapper = spawn(process.execPath, ["--import", "tsx", wrapperPath, ...command(["--exit=0"])], { - env: { - ...process.env, - PATH: `${stubs}:${process.env.PATH}`, - PDPP_TEST_SCRATCH_ROOT: undefined, - PDPP_TEST_SCRATCH_SCHEMA: undefined, - PDPP_TEST_SCRATCH_MARKER: undefined, - PDPP_TEST_SCRATCH_NONCE: undefined, - PDPP_TEST_SCRATCH_OWNER_PID: undefined, - RUNNER_TEMP: runnerTemp, - ...FRESH_SANDBOX_CONTEXT, - TEST_SANDBOX_DISABLE: "0", - TEST_SANDBOX_REQUIRE: "1", - }, - stdio: ["ignore", "ignore", "pipe"], - }); - let stderr = ""; - wrapper.stderr?.on("data", (chunk: Buffer) => { - stderr += chunk.toString(); - }); - const started = Date.now(); - assert.deepEqual(await closed(wrapper), { code: 3, signal: null }); - assert.doesNotMatch(stderr, GROUP_STILL_LIVE_OUTPUT); - assert.ok(Date.now() - started < 3000, "the launcher must not wait out the publish grace period"); - assert.deepEqual(await readdir(join(runnerTemp, "pdpp-test-scratch")), []); - } finally { - await rm(runnerTemp, { force: true, recursive: true }); - await rm(stubs, { force: true, recursive: true }); - } +test("a sandbox that exits before publishing a command group leaves no scratch and propagates its status", async (t) => { + const fx = await lifecycleFixture(t); + const stubs = await fx.dir("stubs"); + // A bwrap that cannot create namespaces makes TEST_SANDBOX_REQUIRE=1 exit 3 before the command starts. + await writeFile(join(stubs, "bwrap"), "#!/bin/sh\nexit 1\n", { mode: 0o755 }); + const wrapper = fx.spawn(process.execPath, ["--import", "tsx", wrapperPath, ...command(["--exit=0"])], { + env: fx.env({ + ...NO_INHERITED_OWNER, + ...FRESH_SANDBOX_CONTEXT, + PATH: `${stubs}:${process.env.PATH}`, + TEST_SANDBOX_DISABLE: "0", + TEST_SANDBOX_REQUIRE: "1", + }), + stdio: ["ignore", "ignore", "pipe"], + }); + let stderr = ""; + wrapper.stderr?.on("data", (chunk: Buffer) => { + stderr += chunk.toString(); + }); + const started = Date.now(); + assert.deepEqual(await closed(wrapper), { code: 3, signal: null }); + assert.doesNotMatch(stderr, GROUP_STILL_LIVE_OUTPUT); + assert.ok(Date.now() - started < 3000, "the launcher must not wait out the publish grace period"); + assert.deepEqual(await readdir(fx.scratchParent), []); }); -test("a sidecar killed right after spawning the command does not orphan the command under a deleted scratch", async () => { - const runnerTemp = await temporaryParent(); - // The command runs under a private /tmp where bwrap binds back only the scratch - // parent's top directory; a sibling temp directory would be invisible to it. - const work = join(runnerTemp, "work"); - await mkdir(work); - const recordFile = join(work, "command.record"); +test("a sidecar killed right after spawning the command does not orphan the command under a deleted scratch", async (t) => { + const fx = await lifecycleFixture(t); + const recordFile = fx.path("work", "command.record"); + await fx.dir("work"); let commandPid: number | undefined; try { - const wrapper = spawn( + const wrapper = fx.spawn( process.execPath, ["--import", "tsx", sandboxChildWrapperPath, killAfterSpawnSandboxChild, "sh", "-c", "exec sleep 300"], { - env: { ...noCgroupEnv(runnerTemp), KILL_AFTER_SPAWN_RECORD: recordFile }, + env: noCgroupEnv(fx, { KILL_AFTER_SPAWN_RECORD: recordFile }), stdio: ["ignore", "ignore", "ignore"], } ); @@ -1206,23 +1080,19 @@ test("a sidecar killed right after spawning the command does not orphan the comm } } finally { killQuietly(commandPid); - await reapOwnedGroups(runnerTemp); - await rm(runnerTemp, { force: true, recursive: true }); } }); -test("owner SIGKILL then a command that leaves a background child does not let recovery delete a live group's scratch", async () => { - const runnerTemp = await temporaryParent(); - // The command runs under a private /tmp where bwrap binds back only the scratch - // parent's top directory; a sibling temp directory would be invisible to it. - const work = join(runnerTemp, "work"); - await mkdir(work); - const pidFile = join(work, "background.pid"); - const releaseFile = join(work, "release"); +test("owner SIGKILL then a command that leaves a background child does not let recovery delete a live group's scratch", async (t) => { + const fx = await lifecycleFixture(t); + const pidFile = fx.path("work", "background.pid"); + const releaseFile = fx.path("work", "release"); + await fx.dir("work"); let backgroundPid: number | undefined; let wrapper: ReturnType | undefined; try { wrapper = startRawWrapper( + fx, [ "sh", "-c", @@ -1231,7 +1101,7 @@ test("owner SIGKILL then a command that leaves a background child does not let r pidFile, releaseFile, ], - noCgroupEnv(runnerTemp) + noCgroupEnv(fx) ); const [pid, root] = (await eventuallyRead(pidFile, 20_000)).trim().split(" "); backgroundPid = Number(pid); @@ -1248,7 +1118,7 @@ test("owner SIGKILL then a command that leaves a background child does not let r markerPath, `${JSON.stringify({ ...marker, created_at: new Date(Date.now() - 120_000).toISOString() })}\n` ); - const results = await recoverStaleScratch({ parent: join(runnerTemp, "pdpp-test-scratch") }); + const results = await recoverStaleScratch({ parent: fx.scratchParent }); // Either recovery keeps the scratch, or nothing of the command is left alive in it. if (results.some((entry) => entry.path === root && entry.removed)) { await eventuallyAbsentProcess(backgroundPid); @@ -1260,19 +1130,17 @@ test("owner SIGKILL then a command that leaves a background child does not let r wrapper.kill("SIGKILL"); } killQuietly(backgroundPid); - await reapOwnedGroups(runnerTemp, wrapper); - await rm(runnerTemp, { force: true, recursive: true }); } }); -test("a sandbox killed before its sidecar publishes does not let recovery delete the scratch of a command that starts later", async () => { - const runnerTemp = await temporaryParent(); - const parent = join(runnerTemp, "pdpp-test-scratch"); +test("a sandbox killed before its sidecar publishes does not let recovery delete the scratch of a command that starts later", async (t) => { + const fx = await lifecycleFixture(t); + const parent = fx.scratchParent; let sidecarPgid: number | undefined; let groupPath: string | undefined; let wrapper: ReturnType | undefined; try { - wrapper = spawn( + wrapper = fx.spawn( process.execPath, [ "--import", @@ -1283,7 +1151,7 @@ test("a sandbox killed before its sidecar publishes does not let recovery delete "-c", 'echo $$ > "$PDPP_TEST_SCRATCH_ROOT/command.pid"; exec sleep 300', ], - { env: noCgroupEnv(runnerTemp), stdio: ["ignore", "ignore", "ignore"] } + { env: noCgroupEnv(fx), stdio: ["ignore", "ignore", "ignore"] } ); const wrapperClosed = closed(wrapper); const root = await eventuallyRunRoot(parent); @@ -1326,7 +1194,6 @@ test("a sandbox killed before its sidecar publishes does not let recovery delete if (wrapper?.pid && wrapper.exitCode === null && wrapper.signalCode === null) { wrapper.kill("SIGKILL"); } - await rm(runnerTemp, { force: true, recursive: true }); } }); @@ -1391,17 +1258,13 @@ test("recovery keeps a scratch whose published sidecar group is live although th } }); -test("SIGTERM starts bounded escalation before a TERM-ignoring direct child closes", async () => { +test("SIGTERM starts bounded escalation before a TERM-ignoring direct child closes", async (t) => { + const fx = await lifecycleFixture(t); let output = ""; - const wrapper = spawn( - process.execPath, - [ - "--import", - "tsx", - new URL("./run-command.ts", import.meta.url).pathname, - ...command(["--print-root", "--ignore-term", "--grandchild", "--grandchild-ignore-term", "--wait"]), - ], - { env: { ...process.env, TEST_SANDBOX_DISABLE: "1" }, stdio: ["ignore", "pipe", "ignore"] } + const wrapper = startWrapper( + fx, + ["--print-root", "--ignore-term", "--grandchild", "--grandchild-ignore-term", "--wait"], + fx.env({ TEST_SANDBOX_DISABLE: "1" }) ); wrapper.stdout?.on("data", (chunk: Buffer) => { output += chunk.toString(); @@ -1433,13 +1296,13 @@ test("SIGTERM starts bounded escalation before a TERM-ignoring direct child clos await assert.rejects(access(root)); }); -test("an intentionally detached descendant is outside the process-group containment contract", async () => { - const runnerTemp = await temporaryParent(); - const wrapper = startWrapper(["--print-root", "--escaped-grandchild", "--wait"], { - ...process.env, - RUNNER_TEMP: runnerTemp, - TEST_SANDBOX_DISABLE: "1", - }); +test("an intentionally detached descendant is outside the process-group containment contract", async (t) => { + const fx = await lifecycleFixture(t); + const wrapper = startWrapper( + fx, + ["--print-root", "--escaped-grandchild", "--wait"], + fx.env({ TEST_SANDBOX_DISABLE: "1" }) + ); let escapedPid: number | undefined; try { const root = await reportedRoot(wrapper); @@ -1459,7 +1322,6 @@ test("an intentionally detached descendant is outside the process-group containm } await eventuallyAbsentGroup(escapedPid); } - await rm(runnerTemp, { force: true, recursive: true }); } }); @@ -1473,131 +1335,83 @@ test("a signal latched while the running transition is durable is forwarded afte assert.deepEqual(result, { code: null, signal: "SIGTERM" }); }); -test("SIGTERM latched before recovery creates no allocation or child", async () => { - const runnerTemp = await temporaryParent(); - const parent = join(runnerTemp, "pdpp-test-scratch"); - const previousRunnerTemp = process.env.RUNNER_TEMP; - process.env.RUNNER_TEMP = runnerTemp; - try { - const result = await runScratchCommand(command(["--print-root"]), { - afterSignalHandlersInstalled: async () => { - process.kill(process.pid, "SIGTERM"); - await new Promise((resolve) => setTimeout(resolve, 25)); - }, - }); - assert.deepEqual(result, { code: null, signal: "SIGTERM" }); - assert.deepEqual(await readdir(parent), []); - } finally { - if (previousRunnerTemp === undefined) { - delete process.env.RUNNER_TEMP; - } else { - process.env.RUNNER_TEMP = previousRunnerTemp; - } - await rm(runnerTemp, { force: true, recursive: true }); - } -}); - -test("SIGTERM during allocation cleanup prevents spawn", async () => { - const runnerTemp = await temporaryParent(); - const parent = join(runnerTemp, "pdpp-test-scratch"); - const previousRunnerTemp = process.env.RUNNER_TEMP; - process.env.RUNNER_TEMP = runnerTemp; - try { - const result = await runScratchCommand(command(["--print-root"]), { - afterAllocationBeforeSpawn: async () => { - process.kill(process.pid, "SIGTERM"); - await new Promise((resolve) => setTimeout(resolve, 25)); - }, - }); - assert.deepEqual(result, { code: null, signal: "SIGTERM" }); - assert.deepEqual(await readdir(parent), []); - } finally { - if (previousRunnerTemp === undefined) { - delete process.env.RUNNER_TEMP; - } else { - process.env.RUNNER_TEMP = previousRunnerTemp; - } - await rm(runnerTemp, { force: true, recursive: true }); - } +test("SIGTERM latched before recovery creates no allocation or child", async (t) => { + const fx = await lifecycleFixture(t); + fx.adoptAsRunnerTemp(); + const result = await runScratchCommand(command(["--print-root"]), { + afterSignalHandlersInstalled: async () => { + process.kill(process.pid, "SIGTERM"); + await new Promise((resolve) => setTimeout(resolve, 25)); + }, + }); + assert.deepEqual(result, { code: null, signal: "SIGTERM" }); + assert.deepEqual(await readdir(fx.scratchParent), []); }); -test("SIGTERM during cleanup remains observable after the root is removed", async () => { - const runnerTemp = await temporaryParent(); - const parent = join(runnerTemp, "pdpp-test-scratch"); - const previousRunnerTemp = process.env.RUNNER_TEMP; - process.env.RUNNER_TEMP = runnerTemp; - try { - const result = await runScratchCommand(command(["--exit=0"]), { - cleanupHooks: { - afterJournal: async () => { - process.kill(process.pid, "SIGTERM"); - await new Promise((resolve) => setTimeout(resolve, 25)); - }, - }, - }); - assert.deepEqual(result, { code: null, signal: "SIGTERM" }); - assert.deepEqual(await readdir(parent), []); - } finally { - if (previousRunnerTemp === undefined) { - delete process.env.RUNNER_TEMP; - } else { - process.env.RUNNER_TEMP = previousRunnerTemp; - } - await rm(runnerTemp, { force: true, recursive: true }); - } +test("SIGTERM during allocation cleanup prevents spawn", async (t) => { + const fx = await lifecycleFixture(t); + fx.adoptAsRunnerTemp(); + const result = await runScratchCommand(command(["--print-root"]), { + afterAllocationBeforeSpawn: async () => { + process.kill(process.pid, "SIGTERM"); + await new Promise((resolve) => setTimeout(resolve, 25)); + }, + }); + assert.deepEqual(result, { code: null, signal: "SIGTERM" }); + assert.deepEqual(await readdir(fx.scratchParent), []); }); -test("a latched signal plus a pre-launch error cleans the provably allocated root", async () => { - const runnerTemp = await temporaryParent(); - const parent = join(runnerTemp, "pdpp-test-scratch"); - const previousRunnerTemp = process.env.RUNNER_TEMP; - process.env.RUNNER_TEMP = runnerTemp; - try { - const result = await runScratchCommand(command(["--print-root"]), { - afterAllocationBeforeSpawn: async () => { +test("SIGTERM during cleanup remains observable after the root is removed", async (t) => { + const fx = await lifecycleFixture(t); + fx.adoptAsRunnerTemp(); + const result = await runScratchCommand(command(["--exit=0"]), { + cleanupHooks: { + afterJournal: async () => { process.kill(process.pid, "SIGTERM"); await new Promise((resolve) => setTimeout(resolve, 25)); - throw new Error("simulate pre-launch failure"); }, - }); - assert.deepEqual(result, { code: null, signal: "SIGTERM" }); - assert.deepEqual(await readdir(parent), []); - } finally { - if (previousRunnerTemp === undefined) { - delete process.env.RUNNER_TEMP; - } else { - process.env.RUNNER_TEMP = previousRunnerTemp; - } - await rm(runnerTemp, { force: true, recursive: true }); - } + }, + }); + assert.deepEqual(result, { code: null, signal: "SIGTERM" }); + assert.deepEqual(await readdir(fx.scratchParent), []); }); -test("SIGKILL during spawn-before-running retains a launching root even after its group dies", async () => { - const runnerTemp = await temporaryParent(); - const parent = join(runnerTemp, "pdpp-test-scratch"); - const proc = startLaunchingWrapper({ ...process.env, RUNNER_TEMP: runnerTemp, TEST_SANDBOX_DISABLE: "1" }); - try { - const root = await reportedRoot(proc); - const markerPath = join(root, ".pdpp-test-scratch.json"); - const marker = JSON.parse(await readFile(markerPath, "utf8")) as { state: string }; - assert.equal(marker.state, "launching"); - const childPid = Number.parseInt(await eventuallyRead(join(root, "child.txt")), 10); - const close = exited(proc); - proc.kill("SIGKILL"); - assert.deepEqual(await close, { code: null, signal: "SIGKILL" }); - const now = Date.now() + 120_000; - assert.deepEqual(await recoverStaleScratch({ now, parent }), [ - { path: root, reason: "launch-unknown", removed: false }, - ]); - process.kill(-childPid, "SIGKILL"); - await eventuallyAbsentGroup(childPid); - assert.deepEqual(await recoverStaleScratch({ now: now + 120_000, parent }), [ - { path: root, reason: "launch-unknown", removed: false }, - ]); - await access(root); - } finally { - await rm(runnerTemp, { force: true, recursive: true }); - } +test("a latched signal plus a pre-launch error cleans the provably allocated root", async (t) => { + const fx = await lifecycleFixture(t); + fx.adoptAsRunnerTemp(); + const result = await runScratchCommand(command(["--print-root"]), { + afterAllocationBeforeSpawn: async () => { + process.kill(process.pid, "SIGTERM"); + await new Promise((resolve) => setTimeout(resolve, 25)); + throw new Error("simulate pre-launch failure"); + }, + }); + assert.deepEqual(result, { code: null, signal: "SIGTERM" }); + assert.deepEqual(await readdir(fx.scratchParent), []); +}); + +test("SIGKILL during spawn-before-running retains a launching root even after its group dies", async (t) => { + const fx = await lifecycleFixture(t); + const parent = fx.scratchParent; + const proc = startLaunchingWrapper(fx, fx.env({ TEST_SANDBOX_DISABLE: "1" })); + const root = await reportedRoot(proc); + const markerPath = join(root, ".pdpp-test-scratch.json"); + const marker = JSON.parse(await readFile(markerPath, "utf8")) as { state: string }; + assert.equal(marker.state, "launching"); + const childPid = Number.parseInt(await eventuallyRead(join(root, "child.txt")), 10); + const close = exited(proc); + proc.kill("SIGKILL"); + assert.deepEqual(await close, { code: null, signal: "SIGKILL" }); + const now = Date.now() + 120_000; + assert.deepEqual(await recoverStaleScratch({ now, parent }), [ + { path: root, reason: "launch-unknown", removed: false }, + ]); + process.kill(-childPid, "SIGKILL"); + await eventuallyAbsentGroup(childPid); + assert.deepEqual(await recoverStaleScratch({ now: now + 120_000, parent }), [ + { path: root, reason: "launch-unknown", removed: false }, + ]); + await access(root); }); test("journal recovery resumes an interrupted rename and survives embedded-marker removal", async () => { @@ -1675,34 +1489,24 @@ test("a malformed nonempty boot identity cannot bypass the live-group proof", as } }); -test("recovery budgets bound inventory inspection and still permit the new owner", async () => { - const runnerTemp = await temporaryParent(); - const parent = join(runnerTemp, "pdpp-test-scratch"); - const previousRunnerTemp = process.env.RUNNER_TEMP; - process.env.RUNNER_TEMP = runnerTemp; - try { - await mkdir(parent, { mode: 0o700 }); - await Promise.all(Array.from({ length: 12 }, (_, index) => writeFile(join(parent, `foreign-${index}`), "defer"))); - const inspected: string[] = []; - const recovered = await recoverStaleScratch({ - hooks: { onInspect: (path) => inspected.push(path) }, - limits: { maxInspectedEntries: 3 }, - parent, - }); - assert.ok(inspected.length <= 3); - assert.deepEqual(recovered.at(-1), { path: parent, reason: "recovery-budget-exhausted", removed: false }); - assert.deepEqual(await runScratchCommand(command(["--exit=0"]), { recoveryLimits: { maxInspectedEntries: 1 } }), { - code: 0, - signal: null, - }); - } finally { - if (previousRunnerTemp === undefined) { - delete process.env.RUNNER_TEMP; - } else { - process.env.RUNNER_TEMP = previousRunnerTemp; - } - await rm(runnerTemp, { force: true, recursive: true }); - } +test("recovery budgets bound inventory inspection and still permit the new owner", async (t) => { + const fx = await lifecycleFixture(t); + const parent = fx.scratchParent; + fx.adoptAsRunnerTemp(); + await mkdir(parent, { mode: 0o700 }); + await Promise.all(Array.from({ length: 12 }, (_, index) => writeFile(join(parent, `foreign-${index}`), "defer"))); + const inspected: string[] = []; + const recovered = await recoverStaleScratch({ + hooks: { onInspect: (path) => inspected.push(path) }, + limits: { maxInspectedEntries: 3 }, + parent, + }); + assert.ok(inspected.length <= 3); + assert.deepEqual(recovered.at(-1), { path: parent, reason: "recovery-budget-exhausted", removed: false }); + assert.deepEqual(await runScratchCommand(command(["--exit=0"]), { recoveryLimits: { maxInspectedEntries: 1 } }), { + code: 0, + signal: null, + }); }); test("a durable lexical cursor eventually reaches a stale candidate beyond an initial recovery page", async () => { @@ -1791,15 +1595,15 @@ test("startup recovery quarantines a large verified root when recursive removal } }); -test("SIGKILL leaves a live orphan, then a later recovery removes its dead group", async () => { - const runnerTemp = await temporaryParent(); - const sibling = join(runnerTemp, "sibling-sentinel"); +test("SIGKILL leaves a live orphan, then a later recovery removes its dead group", async (t) => { + const fx = await lifecycleFixture(t); + const sibling = fx.path("sibling-sentinel"); await writeFile(sibling, "unchanged"); - const proc = startWrapper(["--print-root", "--grandchild", "--grandchild-ignore-term", "--wait"], { - ...process.env, - RUNNER_TEMP: runnerTemp, - TEST_SANDBOX_DISABLE: "1", - }); + const proc = startWrapper( + fx, + ["--print-root", "--grandchild", "--grandchild-ignore-term", "--wait"], + fx.env({ TEST_SANDBOX_DISABLE: "1" }) + ); const root = await reportedRoot(proc); const markerPath = join(root, ".pdpp-test-scratch.json"); const marker = await eventuallyRunningMarker(markerPath); @@ -1814,7 +1618,7 @@ test("SIGKILL leaves a live orphan, then a later recovery removes its dead group markerPath, `${JSON.stringify({ ...marker, created_at: new Date(Date.now() - 120_000).toISOString() })}\n` ); - const parent = join(runnerTemp, "pdpp-test-scratch"); + const parent = fx.scratchParent; assert.deepEqual(await recoverStaleScratch({ parent }), [{ path: root, reason: "group-live", removed: false }]); assert.equal(await readFile(join(root, "child-root.txt"), "utf8"), `${root}\n`); assert.equal(await readFile(sibling, "utf8"), "unchanged"); @@ -1823,7 +1627,6 @@ test("SIGKILL leaves a live orphan, then a later recovery removes its dead group await eventuallyRecover(parent, root); await assert.rejects(access(root)); assert.equal(await readFile(sibling, "utf8"), "unchanged"); - await rm(runnerTemp, { force: true, recursive: true }); }); test("recovery has stable fail-closed classifications for every stale candidate state", async () => { @@ -1940,11 +1743,11 @@ test("wrong-owner candidates have a stable conservative recovery reason", async } }); -test("parallel outer owners isolate participant worker trees under one inherited root each", async () => { - const runnerTemp = await temporaryParent(); - const env = { ...process.env, RUNNER_TEMP: runnerTemp, TEST_SANDBOX_DISABLE: "1" }; - const first = startWrapper(["--print-root", "--nested-participant", "--wait"], env); - const second = startWrapper(["--print-root", "--nested-participant", "--wait"], env); +test("parallel outer owners isolate participant worker trees under one inherited root each", async (t) => { + const fx = await lifecycleFixture(t); + const env = fx.env({ TEST_SANDBOX_DISABLE: "1" }); + const first = startWrapper(fx, ["--print-root", "--nested-participant", "--wait"], env); + const second = startWrapper(fx, ["--print-root", "--nested-participant", "--wait"], env); const [firstRoot, secondRoot] = await Promise.all([reportedRoot(first), reportedRoot(second)]); try { assert.notEqual(await realpath(firstRoot), await realpath(secondRoot)); @@ -1983,6 +1786,86 @@ test("parallel outer owners isolate participant worker trees under one inherited assert.deepEqual(await secondClose, { code: null, signal: "SIGTERM" }); await assert.rejects(access(firstRoot)); await assert.rejects(access(secondRoot)); - await rm(runnerTemp, { force: true, recursive: true }); + } +}); + +// The tests below protect the fixture that every launch above depends on. + +const OUTSIDE_BOUND_ROOT = /outside the bound runner temp/; +const RUNNER_TEMP_MISMATCH = /RUNNER_TEMP must be/; +// A bare `spawn(`, not the fixture's `fx.spawn(`. +const DIRECT_WRAPPER_LAUNCH = /(? { + const source = await readFile(new URL(import.meta.url), "utf8"); + assert.equal( + source.match(DIRECT_WRAPPER_LAUNCH)?.[0], + undefined, + "start the wrapper with fixture.spawn so its paths are checked and its groups reaped" + ); +}); + +test("a command argument or variable naming a temp path outside the bound runner temp is rejected", async (t) => { + const fx = await lifecycleFixture(t); + const sibling = await mkdtemp(join(tmpdir(), "pdpp-test-scratch-test-sibling-")); + t.after(() => rm(sibling, { force: true, recursive: true })); + const inside = fx.path("work", "record"); + assertCommandSeesOnlyBoundPaths(fx.runnerTemp, ["sh", "-c", "true", inside], fx.env({ RECORD: inside })); + assert.throws( + () => assertCommandSeesOnlyBoundPaths(fx.runnerTemp, ["sh", "-c", "true", join(sibling, "f")], fx.env()), + { + message: OUTSIDE_BOUND_ROOT, + } + ); + assert.throws(() => assertCommandSeesOnlyBoundPaths(fx.runnerTemp, [], fx.env({ RECORD: join(sibling, "f") })), { + message: OUTSIDE_BOUND_ROOT, + }); + assert.throws(() => assertCommandSeesOnlyBoundPaths(fx.runnerTemp, [], fx.env({ PATH: `${sibling}:/usr/bin` })), { + message: OUTSIDE_BOUND_ROOT, + }); + assert.throws(() => fx.path("..", "elsewhere"), { message: OUTSIDE_BOUND_ROOT }); + assert.throws(() => fx.spawn("true", [join(sibling, "f")]), { message: OUTSIDE_BOUND_ROOT }); + assert.throws(() => fx.spawn("true", [], { env: { ...process.env, RUNNER_TEMP: sibling } }), { + message: RUNNER_TEMP_MISMATCH, + }); +}); + +test("fixture teardown stops a command group an owner kill orphaned and closes the wrapper pipes", async () => { + const hooks: Array<() => Promise | void> = []; + const fx = await lifecycleFixture({ + after: (hook: () => Promise | void) => hooks.push(hook), + } as unknown as TestContext); + let teardownRan = false; + try { + // The command leaves a background child that inherits the wrapper's stdout and stderr. + const wrapper = startRawWrapper( + fx, + ["sh", "-c", 'sleep 300 & echo "$!" > "$1"; sleep 300', "sh", fx.path("background.pid")], + fx.env({ TEST_SANDBOX_DISABLE: "1" }) + ); + await eventuallyRead(fx.path("background.pid"), 20_000); + const root = await eventuallyRunRoot(fx.scratchParent); + const marker = await eventuallyRunningMarker(join(root, ".pdpp-test-scratch.json")); + const close = exited(wrapper); + wrapper.kill("SIGKILL"); + await close; + // No owner is left: the command's group is alive and still holds the pipes. + process.kill(-marker.pgid, 0); + for (const hook of hooks) { + // biome-ignore lint/performance/noAwaitInLoops: teardown hooks run in registration order. + await hook(); + } + teardownRan = true; + await eventuallyAbsentGroup(marker.pgid); + assert.equal(wrapper.stdout?.destroyed, true); + assert.equal(wrapper.stderr?.destroyed, true); + await assert.rejects(access(fx.runnerTemp)); + } finally { + if (!teardownRan) { + for (const hook of hooks) { + // biome-ignore lint/performance/noAwaitInLoops: teardown hooks run in registration order. + await hook(); + } + } } }); From 6cb08b30eb9b3aea758ae71839aeb6150723973c Mon Sep 17 00:00:00 2001 From: Tim Nunamaker Date: Fri, 6 Nov 2026 17:05:40 -0600 Subject: [PATCH 05/15] test(scratch): admit a /tmp checkout and reap groups no marker records A review of the lifecycle fixture found two gaps. The path guard rejected a repository checked out under /tmp, although test-sandbox rebinds the top /tmp directory of its working directory and the wrapper scripts are visible there. And the spawn-before-running test starts a command whose marker records no process group yet, so a failed assertion skipped its manual kill and left the group alive. The guard now also admits the working directory's top /tmp directory. The fixture gains trackGroup(), which the test uses to register that group for teardown. Assisted-by: AI Signed-off-by: Tim Nunamaker --- .../fixtures/lifecycle-fixture.ts | 42 +++++++++++++++---- scripts/test-scratch/run-command.test.ts | 36 ++++++++++++++++ 2 files changed, 71 insertions(+), 7 deletions(-) diff --git a/scripts/test-scratch/fixtures/lifecycle-fixture.ts b/scripts/test-scratch/fixtures/lifecycle-fixture.ts index e542d9226..bbb1ea539 100644 --- a/scripts/test-scratch/fixtures/lifecycle-fixture.ts +++ b/scripts/test-scratch/fixtures/lifecycle-fixture.ts @@ -45,6 +45,11 @@ export interface LifecycleFixture { * closed when the test ends. */ spawn: (file: string, args: readonly string[], spawnOptions?: SpawnOptions) => ChildProcess; + /** + * Kill this process group when the test ends. For a group the fixture cannot + * discover: one whose marker has not recorded a group yet. + */ + trackGroup: (pgid: number) => void; } // Host directories whose contents a bwrap-private /tmp hides. A value that names @@ -65,16 +70,28 @@ function pathTokens(value: string): string[] { return value.split(PATH_TOKEN_SEPARATOR).filter((token) => token.startsWith("/")); } +/** `test-sandbox` rebinds the top `/tmp/` of its working directory, so a checkout under /tmp stays visible. */ +function workingDirectoryBind(cwd: string): string | undefined { + const match = resolve(cwd).match(TOP_TMP_DIRECTORY); + return match?.[0]; +} + +const TOP_TMP_DIRECTORY = /^\/tmp\/[^/]+/; + /** * Throw if any argument or non-inherited environment value names a path under a - * host temporary directory that is outside `runnerTemp`. The sandbox would hide - * that path from the command, so a test using it fails by timing out. + * host temporary directory that the sandbox does not bind back: neither inside + * `runnerTemp` nor inside the top `/tmp` directory of the working directory. The + * sandbox would hide that path from the command, so a test using it fails by + * timing out. */ export function assertCommandSeesOnlyBoundPaths( runnerTemp: string, args: readonly string[], - env: NodeJS.ProcessEnv + env: NodeJS.ProcessEnv, + cwd: string = process.cwd() ): void { + const cwdBind = workingDirectoryBind(cwd); const inherited = process.env; const values = [ ...args, @@ -86,9 +103,13 @@ export function assertCommandSeesOnlyBoundPaths( for (const value of values) { for (const token of pathTokens(value)) { const path = resolve(token); - if (roots.some((root) => isInside(root, path)) && !isInside(runnerTemp, path)) { + if ( + roots.some((root) => isInside(root, path)) && + !isInside(runnerTemp, path) && + !(cwdBind !== undefined && isInside(cwdBind, path)) + ) { throw new Error( - `${token} is outside the bound runner temp ${runnerTemp}; the sandbox's private /tmp hides it from the command. ` + + `${token} is outside the bound runner temp ${runnerTemp} and the working directory's /tmp tree; the sandbox's private /tmp hides it from the command. ` + "Create it with fixture.dir() or fixture.path()." ); } @@ -128,6 +149,7 @@ export async function lifecycleFixture( const scratchParent = join(runnerTemp, "pdpp-test-scratch"); const children: ChildProcess[] = []; const restoreEnv: Array<() => void> = []; + const trackedGroups = new Set(); const path = (...segments: string[]): string => { const resolved = resolve(runnerTemp, ...segments); @@ -154,7 +176,8 @@ export async function lifecycleFixture( killQuietly(proc.pid); } } - for (const pgid of await recordedGroups(scratchParent)) { + const recorded = await recordedGroups(scratchParent); + for (const pgid of [...trackedGroups, ...recorded]) { killQuietly(-pgid); } await rm(runnerTemp, { force: true, recursive: true }); @@ -175,11 +198,16 @@ export async function lifecycleFixture( if (launchEnv.RUNNER_TEMP !== runnerTemp) { throw new Error(`RUNNER_TEMP must be the fixture's runner temp ${runnerTemp}; got ${launchEnv.RUNNER_TEMP}`); } - assertCommandSeesOnlyBoundPaths(runnerTemp, args, launchEnv); + assertCommandSeesOnlyBoundPaths(runnerTemp, args, launchEnv, String(spawnOptions.cwd ?? process.cwd())); const proc = nodeSpawn(file, args, { ...spawnOptions, env: launchEnv }); children.push(proc); return proc; }, + trackGroup(pgid) { + if (Number.isSafeInteger(pgid) && pgid > 1 && pgid !== process.pid && pgid !== process.ppid) { + trackedGroups.add(pgid); + } + }, adoptAsRunnerTemp() { const previous = process.env.RUNNER_TEMP; process.env.RUNNER_TEMP = runnerTemp; diff --git a/scripts/test-scratch/run-command.test.ts b/scripts/test-scratch/run-command.test.ts index 7655c8dd2..0ce6835e8 100644 --- a/scripts/test-scratch/run-command.test.ts +++ b/scripts/test-scratch/run-command.test.ts @@ -1399,6 +1399,8 @@ test("SIGKILL during spawn-before-running retains a launching root even after it const marker = JSON.parse(await readFile(markerPath, "utf8")) as { state: string }; assert.equal(marker.state, "launching"); const childPid = Number.parseInt(await eventuallyRead(join(root, "child.txt")), 10); + // The marker is still "launching" and records no group, so teardown cannot find this one itself. + fx.trackGroup(childPid); const close = exited(proc); proc.kill("SIGKILL"); assert.deepEqual(await close, { code: null, signal: "SIGKILL" }); @@ -1830,6 +1832,40 @@ test("a command argument or variable naming a temp path outside the bound runner }); }); +test("a checkout under /tmp stays visible to a sandboxed command", async (t) => { + const fx = await lifecycleFixture(t); + const checkout = "/tmp/pdpp-checkout-example"; + const script = `${checkout}/scripts/test-scratch/run-command.ts`; + assertCommandSeesOnlyBoundPaths(fx.runnerTemp, [script], fx.env(), `${checkout}/scripts`); + assert.throws(() => assertCommandSeesOnlyBoundPaths(fx.runnerTemp, ["/tmp/another-dir/f"], fx.env(), checkout), { + message: OUTSIDE_BOUND_ROOT, + }); + assert.throws(() => assertCommandSeesOnlyBoundPaths(fx.runnerTemp, [script], fx.env(), "/home/runner/work/pdpp"), { + message: OUTSIDE_BOUND_ROOT, + }); +}); + +test("fixture teardown stops a tracked group that no marker records", async () => { + const hooks: Array<() => Promise | void> = []; + const fx = await lifecycleFixture({ + after: (hook: () => Promise | void) => hooks.push(hook), + } as unknown as TestContext); + const group = spawn("sh", ["-c", "exec sleep 300"], { detached: true, stdio: "ignore" }); + assert.ok(group.pid); + const pgid = group.pid; + try { + fx.trackGroup(pgid); + process.kill(-pgid, 0); + for (const hook of hooks) { + // biome-ignore lint/performance/noAwaitInLoops: teardown hooks run in registration order. + await hook(); + } + await eventuallyAbsentGroup(pgid); + } finally { + killQuietly(-pgid); + } +}); + test("fixture teardown stops a command group an owner kill orphaned and closes the wrapper pipes", async () => { const hooks: Array<() => Promise | void> = []; const fx = await lifecycleFixture({ From fe1dcee1f244aadd0c4ca18bdac5aa4818b79a6c Mon Sep 17 00:00:00 2001 From: Tim Nunamaker Date: Fri, 6 Nov 2026 23:59:52 -0600 Subject: [PATCH 06/15] test(scratch): reap by launch environment, not recorded ids The lifecycle fixture guessed which strings in a command's arguments and environment named a path the sandbox hides, and killed process groups by numbers it had read from marker files or been handed. Both were wrong in ways a reviewer could construct: a group id that died and was reused before teardown could name an unrelated group, the check for the runner's own group compared two pids instead of the group id, a regex ratchet missed `spawn (process.execPath, ...)`, and the working-directory exception resolved symlinks differently from the sandbox script. The fixture now decides ownership by one fact it creates itself. `spawn` requires `RUNNER_TEMP` to be the fixture's unique runner temp directory. Every descendant, including a command inside the bubblewrap sandbox, inherits that value. Teardown reads `/proc//environ`, kills exactly the processes that carry it (never itself), and scans again because a dying parent may fork once more. It never signals a stored pid or group id. When `/proc` cannot be read it kills nothing and says so through the test's diagnostics. The path guard shrinks to what a test states: `spawn` takes a `helpers` list and throws before launching if a listed file is outside the runner temp. `path` and `dir` already refuse anything outside it. The argument and environment inference, the working-directory exception and the source-text ratchet are removed. `adoptAsRunnerTemp` becomes the idempotent `useRunnerTempForThisTest`, one restore instead of a stack. Tests replace the ratchet with behaviour. An outside helper is rejected through the public API. A process the fixture launched is killed while a process it did not launch survives. A child `node --test` run kills a wrapper owner while the command's group is alive and then fails an assertion; the parent requires exit code 1 within 60 s and the group gone, with the sandbox on and off. With reaping disabled, both child-run tests fail. Assisted-by: AI Signed-off-by: Tim Nunamaker --- .../fixtures/leaked-group-after-failure.ts | 47 ++++ .../fixtures/lifecycle-fixture.ts | 203 +++++++----------- scripts/test-scratch/run-command.test.ts | 161 ++++++-------- 3 files changed, 187 insertions(+), 224 deletions(-) create mode 100644 scripts/test-scratch/fixtures/leaked-group-after-failure.ts diff --git a/scripts/test-scratch/fixtures/leaked-group-after-failure.ts b/scripts/test-scratch/fixtures/leaked-group-after-failure.ts new file mode 100644 index 000000000..9d7c85e48 --- /dev/null +++ b/scripts/test-scratch/fixtures/leaked-group-after-failure.ts @@ -0,0 +1,47 @@ +// Copyright The PDP-Connect Contributors +// SPDX-License-Identifier: Apache-2.0 + +// Run only by run-command.test.ts, as a child `node --test`: kill a wrapper's owner while +// its command group is alive, then fail an assertion. The parent checks the run still +// exits and the group is gone. + +import assert from "node:assert/strict"; +import { readdir, readFile, writeFile } from "node:fs/promises"; +import { join } from "node:path"; +import test from "node:test"; +import { setTimeout as sleep } from "node:timers/promises"; +import { lifecycleFixture } from "./lifecycle-fixture.ts"; + +const wrapperPath = new URL("../run-command.ts", import.meta.url).pathname; + +async function runningGroup(scratchParent: string): Promise { + for (;;) { + // biome-ignore lint/performance/noAwaitInLoops: bounded by the parent test's timeout. + const [root] = (await readdir(scratchParent).catch(() => [] as string[])).filter((name) => name.startsWith("run-")); + const text = root + ? await readFile(join(scratchParent, root, ".pdpp-test-scratch.json"), "utf8").catch(() => "{}") + : "{}"; + const marker = JSON.parse(text) as { pgid?: unknown; state?: unknown }; + if (marker.state === "running" && typeof marker.pgid === "number") { + return marker.pgid; + } + await sleep(25); + } +} + +test("owner killed while its command group runs, then an assertion fails", async (t) => { + const fx = await lifecycleFixture(t); + // The background child inherits the wrapper's pipes, as it did in the CI hang. + const wrapper = fx.spawn(process.execPath, [ + "--import", + "tsx", + wrapperPath, + "--", + "sh", + "-c", + "sleep 300 & sleep 300", + ]); + await writeFile(process.env.GROUP_OUT as string, String(await runningGroup(fx.scratchParent))); + wrapper.kill("SIGKILL"); + assert.fail("deliberate failure while an orphaned command group holds the wrapper's pipes"); +}); diff --git a/scripts/test-scratch/fixtures/lifecycle-fixture.ts b/scripts/test-scratch/fixtures/lifecycle-fixture.ts index bbb1ea539..63171bfcf 100644 --- a/scripts/test-scratch/fixtures/lifecycle-fixture.ts +++ b/scripts/test-scratch/fixtures/lifecycle-fixture.ts @@ -1,24 +1,22 @@ // Copyright The PDP-Connect Contributors // SPDX-License-Identifier: Apache-2.0 -// The one place the lifecycle oracle creates temporary directories for, launches, -// and cleans up after a command that runs through the scratch wrapper. +// The one place the lifecycle oracle creates directories for, launches, and cleans up +// after a command that runs through the scratch wrapper. It makes two mistakes that hung +// CI on a runner image with bubblewrap impossible rather than something each test remembers: // -// Two mistakes broke this suite on a runner image that ships bubblewrap, and this -// fixture makes both of them impossible rather than something each test remembers: +// 1. A helper the command cannot see. `scripts/test-sandbox/test-sandbox` gives the command +// a private /tmp and binds back only the top directory of its scratch root. `path` and +// `dir` hand out paths only inside `runnerTemp`, and `spawn` rejects a declared helper +// outside it. +// 2. A process the test leaves behind. A leftover command group keeps the wrapper's pipes +// open, so the test process never exits. Teardown closes the pipes and kills what the +// fixture launched. // -// 1. Path visibility. `scripts/test-sandbox/test-sandbox` gives the command a -// private /tmp and binds back only the top directory of its scratch root. A -// helper file in any other directory under /tmp is invisible to the command. -// The fixture hands out paths only inside `runnerTemp` (the scratch root's top -// directory) and `spawn` rejects any argument or environment value that names -// another path under a host temporary directory. -// 2. Leaked groups. A test that kills an owner on purpose leaves the command's -// process group running if an assertion then fails. That group still holds the -// stdout/stderr pipes of the wrapper, so the test process never exits and the -// CI step hangs instead of ending red. The fixture closes every pipe it opened, -// kills every wrapper it started, and kills every process group recorded under -// its scratch root when the test ends, whatever the outcome. +// Ownership is the launch environment: `spawn` requires `RUNNER_TEMP` to be this fixture's +// unique `runnerTemp`, and every descendant, including one in the sandbox, inherits it. +// Teardown kills exactly the processes carrying that value. It never signals a recorded +// pid or group id, so a stale or reused id cannot name an unrelated process. import { type ChildProcess, spawn as nodeSpawn, type SpawnOptions } from "node:child_process"; import { mkdir, mkdtemp, readdir, readFile, rm } from "node:fs/promises"; @@ -27,109 +25,48 @@ import { isAbsolute, join, relative, resolve } from "node:path"; import type { TestContext } from "node:test"; export interface LifecycleFixture { - /** Point this process's `RUNNER_TEMP` at `runnerTemp` until the test ends, for in-process `runScratchCommand`. */ - adoptAsRunnerTemp: () => void; /** Create `name` inside `runnerTemp` and return its path. */ dir: (name: string) => Promise; /** `process.env` with `RUNNER_TEMP` pointing at `runnerTemp`, plus `overrides`. */ env: (overrides?: NodeJS.ProcessEnv) => NodeJS.ProcessEnv; /** A path inside `runnerTemp`; throws for anything that would resolve outside it. */ path: (...segments: string[]) => string; - /** The only directory tree a command launched through this fixture may name. */ + /** The only directory tree a helper handed to a launched command may live in. */ readonly runnerTemp: string; /** Where the wrapper allocates its per-run scratch roots. */ readonly scratchParent: string; - /** - * Start a process that runs the wrapper, or a stand-in for it. The process is - * guarded (see `assertCommandSeesOnlyBoundPaths`) and killed with its pipes - * closed when the test ends. - */ - spawn: (file: string, args: readonly string[], spawnOptions?: SpawnOptions) => ChildProcess; - /** - * Kill this process group when the test ends. For a group the fixture cannot - * discover: one whose marker has not recorded a group yet. - */ - trackGroup: (pgid: number) => void; + /** Start a process, killed with its pipes closed when the test ends. Each of `helpers` (files the command uses) must be inside `runnerTemp`. */ + spawn: ( + file: string, + args: readonly string[], + options?: SpawnOptions & { helpers?: readonly string[] } + ) => ChildProcess; + /** Point this process's `RUNNER_TEMP` at `runnerTemp` until the test ends; repeated calls change nothing. */ + useRunnerTempForThisTest: () => void; } -// Host directories whose contents a bwrap-private /tmp hides. A value that names -// one of these but is not inside `runnerTemp` is the mistake the guard rejects. -function hostTemporaryRoots(): string[] { - return [...new Set([tmpdir(), "/tmp", "/var/tmp"].map((root) => resolve(root)))]; -} +const DIGITS = /^\d+$/; function isInside(root: string, candidate: string): boolean { const rel = relative(root, candidate); return rel === "" || !(rel.startsWith("..") || isAbsolute(rel)); } -/** Absolute-path tokens in one argument or value: a PATH-style list or `--flag=/path` yields each path. */ -const PATH_TOKEN_SEPARATOR = /[:=]/; - -function pathTokens(value: string): string[] { - return value.split(PATH_TOKEN_SEPARATOR).filter((token) => token.startsWith("/")); -} - -/** `test-sandbox` rebinds the top `/tmp/` of its working directory, so a checkout under /tmp stays visible. */ -function workingDirectoryBind(cwd: string): string | undefined { - const match = resolve(cwd).match(TOP_TMP_DIRECTORY); - return match?.[0]; -} - -const TOP_TMP_DIRECTORY = /^\/tmp\/[^/]+/; - -/** - * Throw if any argument or non-inherited environment value names a path under a - * host temporary directory that the sandbox does not bind back: neither inside - * `runnerTemp` nor inside the top `/tmp` directory of the working directory. The - * sandbox would hide that path from the command, so a test using it fails by - * timing out. - */ -export function assertCommandSeesOnlyBoundPaths( - runnerTemp: string, - args: readonly string[], - env: NodeJS.ProcessEnv, - cwd: string = process.cwd() -): void { - const cwdBind = workingDirectoryBind(cwd); - const inherited = process.env; - const values = [ - ...args, - ...Object.entries(env) - .filter(([key, value]) => value !== undefined && value !== inherited[key]) - .map(([, value]) => value as string), - ]; - const roots = hostTemporaryRoots(); - for (const value of values) { - for (const token of pathTokens(value)) { - const path = resolve(token); - if ( - roots.some((root) => isInside(root, path)) && - !isInside(runnerTemp, path) && - !(cwdBind !== undefined && isInside(cwdBind, path)) - ) { - throw new Error( - `${token} is outside the bound runner temp ${runnerTemp} and the working directory's /tmp tree; the sandbox's private /tmp hides it from the command. ` + - "Create it with fixture.dir() or fixture.path()." - ); - } - } +/** Pids other than this process whose environment carries `entry`; undefined when `/proc` is unreadable. */ +async function pidsCarrying(entry: string): Promise { + const names = await readdir("/proc").catch(() => undefined); + if (names === undefined) { + return; } -} - -async function recordedGroups(scratchParent: string): Promise { - const roots = (await readdir(scratchParent).catch(() => [] as string[])).map((entry) => join(scratchParent, entry)); - const texts = await Promise.all( - roots.flatMap((root) => [ - readFile(join(root, ".pdpp-test-scratch.json"), "utf8") - .then((text) => String((JSON.parse(text) as { pgid?: unknown }).pgid)) - .catch(() => ""), - readFile(join(root, ".sandbox-child-group"), "utf8").catch(() => ""), - ]) + const pids = await Promise.all( + names + .filter((name) => DIGITS.test(name)) + .map(async (name) => { + const environment = await readFile(`/proc/${name}/environ`, "utf8").catch(() => ""); + return environment.split("\0").includes(entry) ? Number(name) : 0; + }) ); - return texts - .map((text) => Number(text.trim())) - .filter((pgid) => Number.isSafeInteger(pgid) && pgid > 1 && pgid !== process.pid && pgid !== process.ppid); + return pids.filter((pid) => pid > 0 && pid !== process.pid); } function killQuietly(pid: number): void { @@ -140,24 +77,42 @@ function killQuietly(pid: number): void { } } +/** + * Kill every process whose environment carries `entry`, scanning again because a parent may + * fork once more before it dies. False when ownership cannot be established (no `/proc`). + */ +async function killCarriers(entry: string, passesLeft = 5): Promise { + const pids = await pidsCarrying(entry); + if (pids === undefined) { + return false; + } + if (pids.length === 0 || passesLeft === 0) { + return true; + } + for (const pid of pids) { + killQuietly(pid); + } + return killCarriers(entry, passesLeft - 1); +} + /** Create a fixture for one test; everything it handed out is cleaned up in the test's `after` hook. */ export async function lifecycleFixture( t: TestContext, options: { /** Parent of `runnerTemp`; defaults to the host temporary directory. */ under?: string } = {} ): Promise { const runnerTemp = await mkdtemp(join(options.under ?? tmpdir(), "pdpp-test-scratch-test-")); - const scratchParent = join(runnerTemp, "pdpp-test-scratch"); const children: ChildProcess[] = []; - const restoreEnv: Array<() => void> = []; - const trackedGroups = new Set(); + let restoreRunnerTemp: (() => void) | undefined; - const path = (...segments: string[]): string => { - const resolved = resolve(runnerTemp, ...segments); - if (!isInside(runnerTemp, resolved)) { - throw new Error(`${resolved} is outside the bound runner temp ${runnerTemp}`); + const inside = (candidate: string): string => { + if (!isInside(runnerTemp, candidate)) { + throw new Error( + `${candidate} is outside the bound runner temp ${runnerTemp}; the sandbox's private /tmp hides it from the command` + ); } - return resolved; + return candidate; }; + const path = (...segments: string[]): string => inside(resolve(runnerTemp, ...segments)); const env = (overrides: NodeJS.ProcessEnv = {}): NodeJS.ProcessEnv => ({ ...process.env, ...overrides, @@ -165,27 +120,27 @@ export async function lifecycleFixture( }); t.after(async () => { - for (const restore of restoreEnv.reverse()) { - restore(); - } + const mayHaveLaunched = children.length > 0 || restoreRunnerTemp !== undefined; + restoreRunnerTemp?.(); for (const proc of children) { proc.stdin?.destroy(); proc.stdout?.destroy(); proc.stderr?.destroy(); + // An unreaped child still owns its pid, so signalling it by handle cannot hit another process. if (proc.pid !== undefined && proc.exitCode === null && proc.signalCode === null) { killQuietly(proc.pid); } } - const recorded = await recordedGroups(scratchParent); - for (const pgid of [...trackedGroups, ...recorded]) { - killQuietly(-pgid); + // A test that launched nothing and never adopted RUNNER_TEMP has nothing to reap. + if (mayHaveLaunched && !(await killCarriers(`RUNNER_TEMP=${runnerTemp}`))) { + t.diagnostic(`/proc is unreadable, so descendants of ${runnerTemp} cannot be proven ours; none were killed`); } await rm(runnerTemp, { force: true, recursive: true }); }); return { runnerTemp, - scratchParent, + scratchParent: join(runnerTemp, "pdpp-test-scratch"), path, env, async dir(name) { @@ -193,31 +148,31 @@ export async function lifecycleFixture( await mkdir(target, { recursive: true }); return target; }, - spawn(file, args, spawnOptions = {}) { + spawn(file, args, { helpers = [], ...spawnOptions } = {}) { const launchEnv = spawnOptions.env ?? env(); if (launchEnv.RUNNER_TEMP !== runnerTemp) { throw new Error(`RUNNER_TEMP must be the fixture's runner temp ${runnerTemp}; got ${launchEnv.RUNNER_TEMP}`); } - assertCommandSeesOnlyBoundPaths(runnerTemp, args, launchEnv, String(spawnOptions.cwd ?? process.cwd())); + for (const helper of helpers) { + inside(resolve(helper)); + } const proc = nodeSpawn(file, args, { ...spawnOptions, env: launchEnv }); children.push(proc); return proc; }, - trackGroup(pgid) { - if (Number.isSafeInteger(pgid) && pgid > 1 && pgid !== process.pid && pgid !== process.ppid) { - trackedGroups.add(pgid); + useRunnerTempForThisTest() { + if (restoreRunnerTemp) { + return; } - }, - adoptAsRunnerTemp() { const previous = process.env.RUNNER_TEMP; process.env.RUNNER_TEMP = runnerTemp; - restoreEnv.push(() => { + restoreRunnerTemp = () => { if (previous === undefined) { delete process.env.RUNNER_TEMP; } else { process.env.RUNNER_TEMP = previous; } - }); + }; }, }; } diff --git a/scripts/test-scratch/run-command.test.ts b/scripts/test-scratch/run-command.test.ts index 0ce6835e8..74edfb050 100644 --- a/scripts/test-scratch/run-command.test.ts +++ b/scripts/test-scratch/run-command.test.ts @@ -22,11 +22,7 @@ import { import { platform, tmpdir } from "node:os"; import { join, posix, win32 } from "node:path"; import test, { type TestContext } from "node:test"; -import { - assertCommandSeesOnlyBoundPaths, - type LifecycleFixture, - lifecycleFixture, -} from "./fixtures/lifecycle-fixture.ts"; +import { type LifecycleFixture, lifecycleFixture } from "./fixtures/lifecycle-fixture.ts"; import { allocateScratchOwnership, cleanupScratchOwnership, @@ -56,6 +52,7 @@ const lifecycleWrapperPath = new URL("./fixtures/lifecycle-wrapper.ts", import.m const sandboxChildWrapperPath = new URL("./fixtures/sandbox-child-wrapper.ts", import.meta.url).pathname; const delayedPublishSandboxChild = new URL("./fixtures/delayed-publish-sandbox-child.ts", import.meta.url).pathname; const killAfterSpawnSandboxChild = new URL("./fixtures/kill-after-spawn-sandbox-child.ts", import.meta.url).pathname; +const leakedGroupAfterFailure = new URL("./fixtures/leaked-group-after-failure.ts", import.meta.url).pathname; interface ChildResult { code: number | null; @@ -536,7 +533,7 @@ test("launch failure removes its allocated root and returns the infrastructure e test("an unproven group shutdown retains scratch and makes a successful command infrastructure-failed", async (t) => { const fx = await lifecycleFixture(t); - fx.adoptAsRunnerTemp(); + fx.useRunnerTempForThisTest(); const result = await runScratchCommand(command(["--exit=0"]), { stopGroup: async () => false }); assert.deepEqual(result, { code: 74, signal: null }); const roots = (await readdir(fx.scratchParent)).filter((entry) => entry.startsWith("run-")); @@ -545,7 +542,7 @@ test("an unproven group shutdown retains scratch and makes a successful command test("a rejected group proof retains scratch but preserves the child result", async (t) => { const fx = await lifecycleFixture(t); - fx.adoptAsRunnerTemp(); + fx.useRunnerTempForThisTest(); for (const exitCode of [0, 42]) { // biome-ignore lint/performance/noAwaitInLoops: each case must retain and reap its own live group before the next case. const result = await runScratchCommand(["--", "sh", "-c", `sleep 30 & exit ${exitCode}`], { @@ -1037,6 +1034,7 @@ test("a sandbox that exits before publishing a command group leaves no scratch a TEST_SANDBOX_DISABLE: "0", TEST_SANDBOX_REQUIRE: "1", }), + helpers: [stubs], stdio: ["ignore", "ignore", "pipe"], }); let stderr = ""; @@ -1061,6 +1059,7 @@ test("a sidecar killed right after spawning the command does not orphan the comm ["--import", "tsx", sandboxChildWrapperPath, killAfterSpawnSandboxChild, "sh", "-c", "exec sleep 300"], { env: noCgroupEnv(fx, { KILL_AFTER_SPAWN_RECORD: recordFile }), + helpers: [recordFile], stdio: ["ignore", "ignore", "ignore"], } ); @@ -1337,7 +1336,7 @@ test("a signal latched while the running transition is durable is forwarded afte test("SIGTERM latched before recovery creates no allocation or child", async (t) => { const fx = await lifecycleFixture(t); - fx.adoptAsRunnerTemp(); + fx.useRunnerTempForThisTest(); const result = await runScratchCommand(command(["--print-root"]), { afterSignalHandlersInstalled: async () => { process.kill(process.pid, "SIGTERM"); @@ -1350,7 +1349,7 @@ test("SIGTERM latched before recovery creates no allocation or child", async (t) test("SIGTERM during allocation cleanup prevents spawn", async (t) => { const fx = await lifecycleFixture(t); - fx.adoptAsRunnerTemp(); + fx.useRunnerTempForThisTest(); const result = await runScratchCommand(command(["--print-root"]), { afterAllocationBeforeSpawn: async () => { process.kill(process.pid, "SIGTERM"); @@ -1363,7 +1362,7 @@ test("SIGTERM during allocation cleanup prevents spawn", async (t) => { test("SIGTERM during cleanup remains observable after the root is removed", async (t) => { const fx = await lifecycleFixture(t); - fx.adoptAsRunnerTemp(); + fx.useRunnerTempForThisTest(); const result = await runScratchCommand(command(["--exit=0"]), { cleanupHooks: { afterJournal: async () => { @@ -1378,7 +1377,7 @@ test("SIGTERM during cleanup remains observable after the root is removed", asyn test("a latched signal plus a pre-launch error cleans the provably allocated root", async (t) => { const fx = await lifecycleFixture(t); - fx.adoptAsRunnerTemp(); + fx.useRunnerTempForThisTest(); const result = await runScratchCommand(command(["--print-root"]), { afterAllocationBeforeSpawn: async () => { process.kill(process.pid, "SIGTERM"); @@ -1399,8 +1398,6 @@ test("SIGKILL during spawn-before-running retains a launching root even after it const marker = JSON.parse(await readFile(markerPath, "utf8")) as { state: string }; assert.equal(marker.state, "launching"); const childPid = Number.parseInt(await eventuallyRead(join(root, "child.txt")), 10); - // The marker is still "launching" and records no group, so teardown cannot find this one itself. - fx.trackGroup(childPid); const close = exited(proc); proc.kill("SIGKILL"); assert.deepEqual(await close, { code: null, signal: "SIGKILL" }); @@ -1494,7 +1491,7 @@ test("a malformed nonempty boot identity cannot bypass the live-group proof", as test("recovery budgets bound inventory inspection and still permit the new owner", async (t) => { const fx = await lifecycleFixture(t); const parent = fx.scratchParent; - fx.adoptAsRunnerTemp(); + fx.useRunnerTempForThisTest(); await mkdir(parent, { mode: 0o700 }); await Promise.all(Array.from({ length: 12 }, (_, index) => writeFile(join(parent, `foreign-${index}`), "defer"))); const inspected: string[] = []; @@ -1795,113 +1792,77 @@ test("parallel outer owners isolate participant worker trees under one inherited const OUTSIDE_BOUND_ROOT = /outside the bound runner temp/; const RUNNER_TEMP_MISMATCH = /RUNNER_TEMP must be/; -// A bare `spawn(`, not the fixture's `fx.spawn(`. -const DIRECT_WRAPPER_LAUNCH = /(? { - const source = await readFile(new URL(import.meta.url), "utf8"); - assert.equal( - source.match(DIRECT_WRAPPER_LAUNCH)?.[0], - undefined, - "start the wrapper with fixture.spawn so its paths are checked and its groups reaped" - ); -}); -test("a command argument or variable naming a temp path outside the bound runner temp is rejected", async (t) => { +test("a helper outside the bound runner temp is rejected before the command is launched", async (t) => { const fx = await lifecycleFixture(t); const sibling = await mkdtemp(join(tmpdir(), "pdpp-test-scratch-test-sibling-")); t.after(() => rm(sibling, { force: true, recursive: true })); - const inside = fx.path("work", "record"); - assertCommandSeesOnlyBoundPaths(fx.runnerTemp, ["sh", "-c", "true", inside], fx.env({ RECORD: inside })); - assert.throws( - () => assertCommandSeesOnlyBoundPaths(fx.runnerTemp, ["sh", "-c", "true", join(sibling, "f")], fx.env()), - { - message: OUTSIDE_BOUND_ROOT, - } - ); - assert.throws(() => assertCommandSeesOnlyBoundPaths(fx.runnerTemp, [], fx.env({ RECORD: join(sibling, "f") })), { - message: OUTSIDE_BOUND_ROOT, - }); - assert.throws(() => assertCommandSeesOnlyBoundPaths(fx.runnerTemp, [], fx.env({ PATH: `${sibling}:/usr/bin` })), { - message: OUTSIDE_BOUND_ROOT, - }); + assert.throws(() => fx.spawn("true", [], { helpers: [join(sibling, "f")] }), { message: OUTSIDE_BOUND_ROOT }); assert.throws(() => fx.path("..", "elsewhere"), { message: OUTSIDE_BOUND_ROOT }); - assert.throws(() => fx.spawn("true", [join(sibling, "f")]), { message: OUTSIDE_BOUND_ROOT }); assert.throws(() => fx.spawn("true", [], { env: { ...process.env, RUNNER_TEMP: sibling } }), { message: RUNNER_TEMP_MISMATCH, }); + // A helper made with the fixture is accepted. + fx.spawn("true", [], { helpers: [fx.path("work", "record")] }); }); -test("a checkout under /tmp stays visible to a sandboxed command", async (t) => { - const fx = await lifecycleFixture(t); - const checkout = "/tmp/pdpp-checkout-example"; - const script = `${checkout}/scripts/test-scratch/run-command.ts`; - assertCommandSeesOnlyBoundPaths(fx.runnerTemp, [script], fx.env(), `${checkout}/scripts`); - assert.throws(() => assertCommandSeesOnlyBoundPaths(fx.runnerTemp, ["/tmp/another-dir/f"], fx.env(), checkout), { - message: OUTSIDE_BOUND_ROOT, - }); - assert.throws(() => assertCommandSeesOnlyBoundPaths(fx.runnerTemp, [script], fx.env(), "/home/runner/work/pdpp"), { - message: OUTSIDE_BOUND_ROOT, - }); -}); - -test("fixture teardown stops a tracked group that no marker records", async () => { +test("fixture teardown kills what the fixture launched and leaves a process it did not launch alone", { + skip: process.platform !== "linux", +}, async () => { const hooks: Array<() => Promise | void> = []; const fx = await lifecycleFixture({ after: (hook: () => Promise | void) => hooks.push(hook), } as unknown as TestContext); - const group = spawn("sh", ["-c", "exec sleep 300"], { detached: true, stdio: "ignore" }); - assert.ok(group.pid); - const pgid = group.pid; + // Neither has a marker or a registered id: the fixture knows only the environment it gave the first. + const launched = fx.spawn("sh", ["-c", "exec sleep 300"], { detached: true }); + const stranger = spawn("sleep", ["300"], { + detached: true, + env: { ...process.env, RUNNER_TEMP: "/not/a/fixture" }, + stdio: "ignore", + }); try { - fx.trackGroup(pgid); - process.kill(-pgid, 0); + assert.ok(launched.pid && stranger.pid); for (const hook of hooks) { // biome-ignore lint/performance/noAwaitInLoops: teardown hooks run in registration order. await hook(); } - await eventuallyAbsentGroup(pgid); + await eventuallyAbsentGroup(launched.pid); + assert.equal(launched.stdout?.destroyed, true); + await assert.rejects(access(fx.runnerTemp)); + process.kill(-stranger.pid, 0); } finally { - killQuietly(-pgid); + killQuietly(stranger.pid); } }); -test("fixture teardown stops a command group an owner kill orphaned and closes the wrapper pipes", async () => { - const hooks: Array<() => Promise | void> = []; - const fx = await lifecycleFixture({ - after: (hook: () => Promise | void) => hooks.push(hook), - } as unknown as TestContext); - let teardownRan = false; - try { - // The command leaves a background child that inherits the wrapper's stdout and stderr. - const wrapper = startRawWrapper( - fx, - ["sh", "-c", 'sleep 300 & echo "$!" > "$1"; sleep 300', "sh", fx.path("background.pid")], - fx.env({ TEST_SANDBOX_DISABLE: "1" }) - ); - await eventuallyRead(fx.path("background.pid"), 20_000); - const root = await eventuallyRunRoot(fx.scratchParent); - const marker = await eventuallyRunningMarker(join(root, ".pdpp-test-scratch.json")); - const close = exited(wrapper); - wrapper.kill("SIGKILL"); - await close; - // No owner is left: the command's group is alive and still holds the pipes. - process.kill(-marker.pgid, 0); - for (const hook of hooks) { - // biome-ignore lint/performance/noAwaitInLoops: teardown hooks run in registration order. - await hook(); - } - teardownRan = true; - await eventuallyAbsentGroup(marker.pgid); - assert.equal(wrapper.stdout?.destroyed, true); - assert.equal(wrapper.stderr?.destroyed, true); - await assert.rejects(access(fx.runnerTemp)); - } finally { - if (!teardownRan) { - for (const hook of hooks) { - // biome-ignore lint/performance/noAwaitInLoops: teardown hooks run in registration order. - await hook(); - } +for (const sandboxDisabled of ["0", "1"]) { + test(`a failed assertion ends the test run and leaves no process group behind (TEST_SANDBOX_DISABLE=${sandboxDisabled})`, { + skip: process.platform !== "linux", + }, async (t) => { + const fx = await lifecycleFixture(t); + const groupOut = fx.path("group.pid"); + // Before the fixture reaped what a test left behind, this run never exited. + const run = fx.spawn(process.execPath, ["--import", "tsx", "--test", leakedGroupAfterFailure], { + env: fx.env({ + ...NO_INHERITED_OWNER, + ...FRESH_SANDBOX_CONTEXT, + GROUP_OUT: groupOut, + // The outer test runner's context would make the child run skip its test file. + NODE_TEST_CONTEXT: undefined, + TEST_SANDBOX_DISABLE: sandboxDisabled, + }), + helpers: [groupOut], + killSignal: "SIGKILL", + stdio: ["ignore", "pipe", "pipe"], + timeout: 60_000, + }); + let output = ""; + for (const stream of [run.stdout, run.stderr]) { + stream?.on("data", (chunk: Buffer) => { + output += chunk.toString(); + }); } - } -}); + assert.deepEqual(await closed(run), { code: 1, signal: null }, output); + await eventuallyAbsentGroup(Number(await readFile(groupOut, "utf8"))); + }); +} From 5fe676488a2affd1c7d75ddaa102e6b482d98295 Mon Sep 17 00:00:00 2001 From: Tim Nunamaker Date: Sat, 7 Nov 2026 02:00:20 -0600 Subject: [PATCH 07/15] test(scratch): match and kill each carrier synchronously; prove the leak was real Teardown now reads each process's environment and signals it in one synchronous step, so no await separates the match from the kill. A pid read earlier can no longer be reassigned while other reads are pending. The window between one read and its kill remains; it is the same one Jenkins' ProcessTreeKiller accepts. The failed-assertion regression had a way to pass without its leak. The shell now writes a readiness sentinel holding its own pid and its background child's only after both exist. The test waits for it before killing the owner, requires both pids to disappear, and requires the deliberate-failure message so a setup failure cannot stand in for it. Two tests drop their fallback finally blocks; the fixture owns failure cleanup. The fixture header now says what is enforced (fixture-created paths stay inside RUNNER_TEMP; declared outside helpers are rejected), and the positive probe launches a real wrapper `cat` of a fixture file. Assisted-by: AI Signed-off-by: Tim Nunamaker --- .../fixtures/leaked-group-after-failure.ts | 52 +++--- .../fixtures/lifecycle-fixture.ts | 85 +++++----- scripts/test-scratch/run-command.test.ts | 150 +++++++++--------- 3 files changed, 148 insertions(+), 139 deletions(-) diff --git a/scripts/test-scratch/fixtures/leaked-group-after-failure.ts b/scripts/test-scratch/fixtures/leaked-group-after-failure.ts index 9d7c85e48..0b2ea6f58 100644 --- a/scripts/test-scratch/fixtures/leaked-group-after-failure.ts +++ b/scripts/test-scratch/fixtures/leaked-group-after-failure.ts @@ -2,46 +2,50 @@ // SPDX-License-Identifier: Apache-2.0 // Run only by run-command.test.ts, as a child `node --test`: kill a wrapper's owner while -// its command group is alive, then fail an assertion. The parent checks the run still -// exits and the group is gone. +// its command and a background child are alive, then fail an assertion. The parent checks +// the run still exits and both processes are gone. import assert from "node:assert/strict"; -import { readdir, readFile, writeFile } from "node:fs/promises"; -import { join } from "node:path"; +import { readFile, writeFile } from "node:fs/promises"; import test from "node:test"; import { setTimeout as sleep } from "node:timers/promises"; import { lifecycleFixture } from "./lifecycle-fixture.ts"; const wrapperPath = new URL("../run-command.ts", import.meta.url).pathname; +const PIDS = /^\d+ \d+\n$/; -async function runningGroup(scratchParent: string): Promise { +/** Wait for the shell's readiness sentinel: its own pid and its background child's, written once both exist. */ +async function readyPids(sentinel: string): Promise { for (;;) { // biome-ignore lint/performance/noAwaitInLoops: bounded by the parent test's timeout. - const [root] = (await readdir(scratchParent).catch(() => [] as string[])).filter((name) => name.startsWith("run-")); - const text = root - ? await readFile(join(scratchParent, root, ".pdpp-test-scratch.json"), "utf8").catch(() => "{}") - : "{}"; - const marker = JSON.parse(text) as { pgid?: unknown; state?: unknown }; - if (marker.state === "running" && typeof marker.pgid === "number") { - return marker.pgid; + const text = await readFile(sentinel, "utf8").catch(() => ""); + if (PIDS.test(text)) { + return text; } await sleep(25); } } -test("owner killed while its command group runs, then an assertion fails", async (t) => { +test("owner killed while its command and a background child run, then an assertion fails", async (t) => { const fx = await lifecycleFixture(t); + const sentinel = fx.path("ready"); // The background child inherits the wrapper's pipes, as it did in the CI hang. - const wrapper = fx.spawn(process.execPath, [ - "--import", - "tsx", - wrapperPath, - "--", - "sh", - "-c", - "sleep 300 & sleep 300", - ]); - await writeFile(process.env.GROUP_OUT as string, String(await runningGroup(fx.scratchParent))); + const wrapper = fx.spawn( + process.execPath, + [ + "--import", + "tsx", + wrapperPath, + "--", + "sh", + "-c", + 'sleep 300 & echo "$$ $!" > "$1.tmp" && mv "$1.tmp" "$1"; sleep 300', + "sh", + sentinel, + ], + { helpers: [sentinel] } + ); + await writeFile(process.env.PIDS_OUT as string, await readyPids(sentinel)); wrapper.kill("SIGKILL"); - assert.fail("deliberate failure while an orphaned command group holds the wrapper's pipes"); + assert.fail("deliberate failure while an orphaned command holds the wrapper's pipes"); }); diff --git a/scripts/test-scratch/fixtures/lifecycle-fixture.ts b/scripts/test-scratch/fixtures/lifecycle-fixture.ts index 63171bfcf..d2f0ee2da 100644 --- a/scripts/test-scratch/fixtures/lifecycle-fixture.ts +++ b/scripts/test-scratch/fixtures/lifecycle-fixture.ts @@ -2,24 +2,26 @@ // SPDX-License-Identifier: Apache-2.0 // The one place the lifecycle oracle creates directories for, launches, and cleans up -// after a command that runs through the scratch wrapper. It makes two mistakes that hung -// CI on a runner image with bubblewrap impossible rather than something each test remembers: +// after a command that runs through the scratch wrapper. It guards against two mistakes that +// hung CI on a runner image with bubblewrap, so no test has to remember them: // // 1. A helper the command cannot see. `scripts/test-sandbox/test-sandbox` gives the command // a private /tmp and binds back only the top directory of its scratch root. `path` and -// `dir` hand out paths only inside `runnerTemp`, and `spawn` rejects a declared helper -// outside it. +// `dir` hand out paths only inside `runnerTemp`, and `spawn` rejects a helper declared +// outside it. Fixture-created paths stay inside `runnerTemp`; declared outside helpers +// are rejected. A path a test builds by hand and does not declare is not checked. // 2. A process the test leaves behind. A leftover command group keeps the wrapper's pipes // open, so the test process never exits. Teardown closes the pipes and kills what the // fixture launched. // // Ownership is the launch environment: `spawn` requires `RUNNER_TEMP` to be this fixture's // unique `runnerTemp`, and every descendant, including one in the sandbox, inherits it. -// Teardown kills exactly the processes carrying that value. It never signals a recorded -// pid or group id, so a stale or reused id cannot name an unrelated process. +// Teardown kills exactly the processes carrying that value. It never signals a pid or group +// id recorded earlier: each process is matched and signalled in one synchronous step. import { type ChildProcess, spawn as nodeSpawn, type SpawnOptions } from "node:child_process"; -import { mkdir, mkdtemp, readdir, readFile, rm } from "node:fs/promises"; +import { readdirSync, readFileSync } from "node:fs"; +import { mkdir, mkdtemp, rm } from "node:fs/promises"; import { tmpdir } from "node:os"; import { isAbsolute, join, relative, resolve } from "node:path"; import type { TestContext } from "node:test"; @@ -52,21 +54,42 @@ function isInside(root: string, candidate: string): boolean { return rel === "" || !(rel.startsWith("..") || isAbsolute(rel)); } -/** Pids other than this process whose environment carries `entry`; undefined when `/proc` is unreadable. */ -async function pidsCarrying(entry: string): Promise { - const names = await readdir("/proc").catch(() => undefined); - if (names === undefined) { - return; +/** + * Kill every process other than this one whose environment carries `entry`, scanning again + * because a parent may fork once more before it dies. False when `/proc` is unreadable, so + * ownership cannot be established. + */ +function killCarriers(entry: string): boolean { + for (let scan = 0; scan < 6; scan += 1) { + let names: string[]; + try { + names = readdirSync("/proc"); + } catch { + return false; + } + let killed = 0; + for (const name of names) { + const pid = Number(name); + if (!DIGITS.test(name) || pid === process.pid) { + continue; + } + try { + // Match and signal with nothing awaited between, so the pid cannot be reassigned while + // we wait. The remaining window, between this read and the kill, is the one Jenkins' + // ProcessTreeKiller accepts. + if (readFileSync(`/proc/${name}/environ`, "utf8").split("\0").includes(entry)) { + process.kill(pid, "SIGKILL"); + killed += 1; + } + } catch { + // The environment is unreadable or the process already exited. + } + } + if (killed === 0) { + break; + } } - const pids = await Promise.all( - names - .filter((name) => DIGITS.test(name)) - .map(async (name) => { - const environment = await readFile(`/proc/${name}/environ`, "utf8").catch(() => ""); - return environment.split("\0").includes(entry) ? Number(name) : 0; - }) - ); - return pids.filter((pid) => pid > 0 && pid !== process.pid); + return true; } function killQuietly(pid: number): void { @@ -77,24 +100,6 @@ function killQuietly(pid: number): void { } } -/** - * Kill every process whose environment carries `entry`, scanning again because a parent may - * fork once more before it dies. False when ownership cannot be established (no `/proc`). - */ -async function killCarriers(entry: string, passesLeft = 5): Promise { - const pids = await pidsCarrying(entry); - if (pids === undefined) { - return false; - } - if (pids.length === 0 || passesLeft === 0) { - return true; - } - for (const pid of pids) { - killQuietly(pid); - } - return killCarriers(entry, passesLeft - 1); -} - /** Create a fixture for one test; everything it handed out is cleaned up in the test's `after` hook. */ export async function lifecycleFixture( t: TestContext, @@ -132,7 +137,7 @@ export async function lifecycleFixture( } } // A test that launched nothing and never adopted RUNNER_TEMP has nothing to reap. - if (mayHaveLaunched && !(await killCarriers(`RUNNER_TEMP=${runnerTemp}`))) { + if (mayHaveLaunched && !killCarriers(`RUNNER_TEMP=${runnerTemp}`)) { t.diagnostic(`/proc is unreadable, so descendants of ${runnerTemp} cannot be proven ours; none were killed`); } await rm(runnerTemp, { force: true, recursive: true }); diff --git a/scripts/test-scratch/run-command.test.ts b/scripts/test-scratch/run-command.test.ts index 74edfb050..60f48e463 100644 --- a/scripts/test-scratch/run-command.test.ts +++ b/scripts/test-scratch/run-command.test.ts @@ -1052,33 +1052,28 @@ test("a sidecar killed right after spawning the command does not orphan the comm const fx = await lifecycleFixture(t); const recordFile = fx.path("work", "command.record"); await fx.dir("work"); - let commandPid: number | undefined; - try { - const wrapper = fx.spawn( - process.execPath, - ["--import", "tsx", sandboxChildWrapperPath, killAfterSpawnSandboxChild, "sh", "-c", "exec sleep 300"], - { - env: noCgroupEnv(fx, { KILL_AFTER_SPAWN_RECORD: recordFile }), - helpers: [recordFile], - stdio: ["ignore", "ignore", "ignore"], - } - ); - await closed(wrapper); - // Written by the sidecar before the kill, so it exists even if cleanup stops the command before it runs. - const [pid, root] = (await eventuallyRead(recordFile, 20_000)).trim().split(" "); - commandPid = Number(pid); - assert.ok(Number.isSafeInteger(commandPid) && commandPid > 0); - assert.ok(root); - // Either the scratch is kept, or the command is gone: never a live command with its scratch deleted. - if ( - !(await access(root) - .then(() => true) - .catch(() => false)) - ) { - await eventuallyAbsentProcess(commandPid); + const wrapper = fx.spawn( + process.execPath, + ["--import", "tsx", sandboxChildWrapperPath, killAfterSpawnSandboxChild, "sh", "-c", "exec sleep 300"], + { + env: noCgroupEnv(fx, { KILL_AFTER_SPAWN_RECORD: recordFile }), + helpers: [recordFile], + stdio: ["ignore", "ignore", "ignore"], } - } finally { - killQuietly(commandPid); + ); + await closed(wrapper); + // Written by the sidecar before the kill, so it exists even if cleanup stops the command before it runs. + const [pid, root] = (await eventuallyRead(recordFile, 20_000)).trim().split(" "); + const commandPid = Number(pid); + assert.ok(Number.isSafeInteger(commandPid) && commandPid > 0); + assert.ok(root); + // Either the scratch is kept, or the command is gone: never a live command with its scratch deleted. + if ( + !(await access(root) + .then(() => true) + .catch(() => false)) + ) { + await eventuallyAbsentProcess(commandPid); } }); @@ -1087,48 +1082,37 @@ test("owner SIGKILL then a command that leaves a background child does not let r const pidFile = fx.path("work", "background.pid"); const releaseFile = fx.path("work", "release"); await fx.dir("work"); - let backgroundPid: number | undefined; - let wrapper: ReturnType | undefined; - try { - wrapper = startRawWrapper( - fx, - [ - "sh", - "-c", - 'sleep 300 /dev/null 2>&1 & echo "$! $PDPP_TEST_SCRATCH_ROOT" > "$1"; while [ ! -e "$2" ]; do sleep 0.05; done', - "sh", - pidFile, - releaseFile, - ], - noCgroupEnv(fx) - ); - const [pid, root] = (await eventuallyRead(pidFile, 20_000)).trim().split(" "); - backgroundPid = Number(pid); - assert.ok(root); - const markerPath = join(root, ".pdpp-test-scratch.json"); - const marker = await eventuallyRunningMarker(markerPath); - const close = exited(wrapper); - wrapper.kill("SIGKILL"); - await close; - // The command now exits, leaving its background child behind; no owner is left to stop it. - await writeFile(releaseFile, "release\n"); - await eventuallyAbsentGroup(marker.pgid); - await writeFile( - markerPath, - `${JSON.stringify({ ...marker, created_at: new Date(Date.now() - 120_000).toISOString() })}\n` - ); - const results = await recoverStaleScratch({ parent: fx.scratchParent }); - // Either recovery keeps the scratch, or nothing of the command is left alive in it. - if (results.some((entry) => entry.path === root && entry.removed)) { - await eventuallyAbsentProcess(backgroundPid); - } - } finally { - // Release the command's wait loop and stop an owner a failed assertion left running. - await writeFile(releaseFile, "release\n").catch(() => undefined); - if (wrapper?.pid && wrapper.exitCode === null && wrapper.signalCode === null) { - wrapper.kill("SIGKILL"); - } - killQuietly(backgroundPid); + const wrapper = startRawWrapper( + fx, + [ + "sh", + "-c", + 'sleep 300 /dev/null 2>&1 & echo "$! $PDPP_TEST_SCRATCH_ROOT" > "$1"; while [ ! -e "$2" ]; do sleep 0.05; done', + "sh", + pidFile, + releaseFile, + ], + noCgroupEnv(fx) + ); + const [pid, root] = (await eventuallyRead(pidFile, 20_000)).trim().split(" "); + const backgroundPid = Number(pid); + assert.ok(root); + const markerPath = join(root, ".pdpp-test-scratch.json"); + const marker = await eventuallyRunningMarker(markerPath); + const close = exited(wrapper); + wrapper.kill("SIGKILL"); + await close; + // The command now exits, leaving its background child behind; no owner is left to stop it. + await writeFile(releaseFile, "release\n"); + await eventuallyAbsentGroup(marker.pgid); + await writeFile( + markerPath, + `${JSON.stringify({ ...marker, created_at: new Date(Date.now() - 120_000).toISOString() })}\n` + ); + const results = await recoverStaleScratch({ parent: fx.scratchParent }); + // Either recovery keeps the scratch, or nothing of the command is left alive in it. + if (results.some((entry) => entry.path === root && entry.removed)) { + await eventuallyAbsentProcess(backgroundPid); } }); @@ -1792,6 +1776,7 @@ test("parallel outer owners isolate participant worker trees under one inherited const OUTSIDE_BOUND_ROOT = /outside the bound runner temp/; const RUNNER_TEMP_MISMATCH = /RUNNER_TEMP must be/; +const DELIBERATE_FAILURE = /deliberate failure while an orphaned command holds/; test("a helper outside the bound runner temp is rejected before the command is launched", async (t) => { const fx = await lifecycleFixture(t); @@ -1802,8 +1787,16 @@ test("a helper outside the bound runner temp is rejected before the command is l assert.throws(() => fx.spawn("true", [], { env: { ...process.env, RUNNER_TEMP: sibling } }), { message: RUNNER_TEMP_MISMATCH, }); - // A helper made with the fixture is accepted. - fx.spawn("true", [], { helpers: [fx.path("work", "record")] }); + // A helper made with the fixture is accepted, and the launched command can read it. + const record = join(await fx.dir("work"), "record"); + await writeFile(record, "from the fixture\n"); + const reader = fx.spawn("cat", [record], { helpers: [record] }); + let output = ""; + reader.stdout?.on("data", (chunk: Buffer) => { + output += chunk.toString(); + }); + assert.deepEqual(await closed(reader), { code: 0, signal: null }); + assert.equal(output, "from the fixture\n"); }); test("fixture teardown kills what the fixture launched and leaves a process it did not launch alone", { @@ -1836,22 +1829,22 @@ test("fixture teardown kills what the fixture launched and leaves a process it d }); for (const sandboxDisabled of ["0", "1"]) { - test(`a failed assertion ends the test run and leaves no process group behind (TEST_SANDBOX_DISABLE=${sandboxDisabled})`, { + test(`a failed assertion ends the test run and leaves no command process behind (TEST_SANDBOX_DISABLE=${sandboxDisabled})`, { skip: process.platform !== "linux", }, async (t) => { const fx = await lifecycleFixture(t); - const groupOut = fx.path("group.pid"); + const pidsOut = fx.path("pids"); // Before the fixture reaped what a test left behind, this run never exited. const run = fx.spawn(process.execPath, ["--import", "tsx", "--test", leakedGroupAfterFailure], { env: fx.env({ ...NO_INHERITED_OWNER, ...FRESH_SANDBOX_CONTEXT, - GROUP_OUT: groupOut, + PIDS_OUT: pidsOut, // The outer test runner's context would make the child run skip its test file. NODE_TEST_CONTEXT: undefined, TEST_SANDBOX_DISABLE: sandboxDisabled, }), - helpers: [groupOut], + helpers: [pidsOut], killSignal: "SIGKILL", stdio: ["ignore", "pipe", "pipe"], timeout: 60_000, @@ -1863,6 +1856,13 @@ for (const sandboxDisabled of ["0", "1"]) { }); } assert.deepEqual(await closed(run), { code: 1, signal: null }, output); - await eventuallyAbsentGroup(Number(await readFile(groupOut, "utf8"))); + // The deliberate failure, not a setup failure, ended the run. + assert.match(output, DELIBERATE_FAILURE); + await Promise.all( + (await readFile(pidsOut, "utf8")) + .trim() + .split(" ") + .map((pid) => eventuallyAbsentProcess(Number(pid))) + ); }); } From 2dde1db541d1e0dfbbfbb6d1152130ab68a891ae Mon Sep 17 00:00:00 2001 From: Tim Nunamaker Date: Sat, 7 Nov 2026 02:04:33 -0600 Subject: [PATCH 08/15] test(scratch): shorten the carrier kill loop and its comment Rulings asked for a kill path shorter than the async version and a one-line comment naming the remaining window. Fold the empty-scan exit into the loop condition, filter candidate pids up front, and drop the header sentence that restated the honest path claim. Behavior is unchanged. Assisted-by: AI Signed-off-by: Tim Nunamaker --- .../fixtures/lifecycle-fixture.ts | 33 ++++++------------- 1 file changed, 10 insertions(+), 23 deletions(-) diff --git a/scripts/test-scratch/fixtures/lifecycle-fixture.ts b/scripts/test-scratch/fixtures/lifecycle-fixture.ts index d2f0ee2da..76093b8e1 100644 --- a/scripts/test-scratch/fixtures/lifecycle-fixture.ts +++ b/scripts/test-scratch/fixtures/lifecycle-fixture.ts @@ -6,10 +6,9 @@ // hung CI on a runner image with bubblewrap, so no test has to remember them: // // 1. A helper the command cannot see. `scripts/test-sandbox/test-sandbox` gives the command -// a private /tmp and binds back only the top directory of its scratch root. `path` and -// `dir` hand out paths only inside `runnerTemp`, and `spawn` rejects a helper declared -// outside it. Fixture-created paths stay inside `runnerTemp`; declared outside helpers -// are rejected. A path a test builds by hand and does not declare is not checked. +// a private /tmp and binds back only the top directory of its scratch root. Fixture-created +// paths stay inside `runnerTemp`; declared outside helpers are rejected. A path a test +// builds by hand and does not declare is not checked. // 2. A process the test leaves behind. A leftover command group keeps the wrapper's pipes // open, so the test process never exits. Teardown closes the pipes and kills what the // fixture launched. @@ -54,40 +53,28 @@ function isInside(root: string, candidate: string): boolean { return rel === "" || !(rel.startsWith("..") || isAbsolute(rel)); } -/** - * Kill every process other than this one whose environment carries `entry`, scanning again - * because a parent may fork once more before it dies. False when `/proc` is unreadable, so - * ownership cannot be established. - */ +/** Kill every other process whose environment carries `entry`, rescanning while a parent forks; false when `/proc` is unreadable. */ function killCarriers(entry: string): boolean { - for (let scan = 0; scan < 6; scan += 1) { + let killed = -1; + for (let scan = 0; scan < 6 && killed !== 0; scan += 1) { + killed = 0; let names: string[]; try { names = readdirSync("/proc"); } catch { return false; } - let killed = 0; - for (const name of names) { - const pid = Number(name); - if (!DIGITS.test(name) || pid === process.pid) { - continue; - } + for (const name of names.filter((n) => DIGITS.test(n) && Number(n) !== process.pid)) { try { - // Match and signal with nothing awaited between, so the pid cannot be reassigned while - // we wait. The remaining window, between this read and the kill, is the one Jenkins' - // ProcessTreeKiller accepts. + // Nothing is awaited between this read and the kill; that gap is the window Jenkins' ProcessTreeKiller accepts. if (readFileSync(`/proc/${name}/environ`, "utf8").split("\0").includes(entry)) { - process.kill(pid, "SIGKILL"); + process.kill(Number(name), "SIGKILL"); killed += 1; } } catch { // The environment is unreadable or the process already exited. } } - if (killed === 0) { - break; - } } return true; } From 21b55e10b41abe20a1c4ba4e6d8292c968fb4f91 Mon Sep 17 00:00:00 2001 From: Tim Nunamaker Date: Sat, 7 Nov 2026 02:39:04 -0600 Subject: [PATCH 09/15] test(scratch): probe /tmp visibility through the wrapper and reject a failed leak setup The positive helper probe ran host cat, not the sandboxed wrapper, so a broken /tmp bind-back still passed. It now runs cat through run-command.ts with the sandbox enabled and the fixture under /tmp. The leak regression now requires both recorded pids alive and the wrapper still running before its deliberate failure, with its own 20 s readiness deadline; the parent test rejects any "setup failed" output. The two fallback finally blocks keep only the conditional sidecar-group SIGKILL. Assisted-by: AI Signed-off-by: Tim Nunamaker --- .../fixtures/leaked-group-after-failure.ts | 30 +++++++++++++-- scripts/test-scratch/run-command.test.ts | 37 +++++++------------ 2 files changed, 40 insertions(+), 27 deletions(-) diff --git a/scripts/test-scratch/fixtures/leaked-group-after-failure.ts b/scripts/test-scratch/fixtures/leaked-group-after-failure.ts index 0b2ea6f58..c49908109 100644 --- a/scripts/test-scratch/fixtures/leaked-group-after-failure.ts +++ b/scripts/test-scratch/fixtures/leaked-group-after-failure.ts @@ -14,16 +14,30 @@ import { lifecycleFixture } from "./lifecycle-fixture.ts"; const wrapperPath = new URL("../run-command.ts", import.meta.url).pathname; const PIDS = /^\d+ \d+\n$/; +// Shorter than the parent's 60 s timeout, so a stuck setup fails here and runs this file's after-hooks. +const READY_DEADLINE_MS = 20_000; + /** Wait for the shell's readiness sentinel: its own pid and its background child's, written once both exist. */ async function readyPids(sentinel: string): Promise { - for (;;) { - // biome-ignore lint/performance/noAwaitInLoops: bounded by the parent test's timeout. + const deadline = Date.now() + READY_DEADLINE_MS; + while (Date.now() < deadline) { + // biome-ignore lint/performance/noAwaitInLoops: bounded by READY_DEADLINE_MS. const text = await readFile(sentinel, "utf8").catch(() => ""); if (PIDS.test(text)) { return text; } await sleep(25); } + throw new Error("setup failed: the command never reported its pids"); +} + +function alive(pid: number): boolean { + try { + process.kill(pid, 0); + return true; + } catch { + return false; + } } test("owner killed while its command and a background child run, then an assertion fails", async (t) => { @@ -45,7 +59,17 @@ test("owner killed while its command and a background child run, then an asserti ], { helpers: [sentinel] } ); - await writeFile(process.env.PIDS_OUT as string, await readyPids(sentinel)); + const pids = await readyPids(sentinel); + // Let a command that fails at startup finish failing, then require the leak this test needs. + await sleep(200); + const gone = pids + .trim() + .split(" ") + .filter((pid) => !alive(Number(pid))); + if (gone.length > 0 || wrapper.exitCode !== null || wrapper.signalCode !== null) { + throw new Error(`setup failed: the command or its background child is not running (gone: ${gone.join(",")})`); + } + await writeFile(process.env.PIDS_OUT as string, pids); wrapper.kill("SIGKILL"); assert.fail("deliberate failure while an orphaned command holds the wrapper's pipes"); }); diff --git a/scripts/test-scratch/run-command.test.ts b/scripts/test-scratch/run-command.test.ts index 60f48e463..c208a30ef 100644 --- a/scripts/test-scratch/run-command.test.ts +++ b/scripts/test-scratch/run-command.test.ts @@ -985,10 +985,9 @@ test("SIGINT waits for the sidecar to publish its group before the launcher clea settled = true; }); let sidecarPgid: number | undefined; - let groupPath: string | undefined; try { const root = await eventuallyRunRoot(fx.scratchParent); - groupPath = join(root, ".sandbox-child-group"); + const groupPath = join(root, ".sandbox-child-group"); sidecarPgid = Number((await eventuallyRead(`${groupPath}.held`, 20_000)).trim()); assert.ok(Number.isSafeInteger(sidecarPgid) && sidecarPgid > 0); await assert.rejects(access(groupPath)); @@ -1002,11 +1001,6 @@ test("SIGINT waits for the sidecar to publish its group before the launcher clea await eventuallyAbsentGroup(sidecarPgid); await assert.rejects(access(root)); } finally { - if (groupPath) { - await writeFile(`${groupPath}.release`, "release\n").catch(() => { - // The owner may already have removed its scratch root. - }); - } if (sidecarPgid) { try { process.kill(-sidecarPgid, "SIGKILL"); @@ -1014,10 +1008,6 @@ test("SIGINT waits for the sidecar to publish its group before the launcher clea // The group may have exited before this failure-path cleanup. } } - if (wrapper.exitCode === null && wrapper.signalCode === null) { - wrapper.kill("SIGTERM"); - await result; - } } }); @@ -1120,10 +1110,8 @@ test("a sandbox killed before its sidecar publishes does not let recovery delete const fx = await lifecycleFixture(t); const parent = fx.scratchParent; let sidecarPgid: number | undefined; - let groupPath: string | undefined; - let wrapper: ReturnType | undefined; try { - wrapper = fx.spawn( + const wrapper = fx.spawn( process.execPath, [ "--import", @@ -1138,7 +1126,7 @@ test("a sandbox killed before its sidecar publishes does not let recovery delete ); const wrapperClosed = closed(wrapper); const root = await eventuallyRunRoot(parent); - groupPath = join(root, ".sandbox-child-group"); + const groupPath = join(root, ".sandbox-child-group"); const markerPath = join(root, ".pdpp-test-scratch.json"); const marker = await eventuallyRunningMarker(markerPath); sidecarPgid = Number((await eventuallyRead(`${groupPath}.held`, 20_000)).trim()); @@ -1164,9 +1152,6 @@ test("a sandbox killed before its sidecar publishes does not let recovery delete await access(root); assert.ok(processAlive(commandPid)); } finally { - if (groupPath) { - await writeFile(`${groupPath}.release`, "release\n").catch(() => undefined); - } if (sidecarPgid) { try { process.kill(-sidecarPgid, "SIGKILL"); @@ -1174,9 +1159,6 @@ test("a sandbox killed before its sidecar publishes does not let recovery delete // The group may have exited before this failure-path cleanup. } } - if (wrapper?.pid && wrapper.exitCode === null && wrapper.signalCode === null) { - wrapper.kill("SIGKILL"); - } } }); @@ -1777,9 +1759,10 @@ test("parallel outer owners isolate participant worker trees under one inherited const OUTSIDE_BOUND_ROOT = /outside the bound runner temp/; const RUNNER_TEMP_MISMATCH = /RUNNER_TEMP must be/; const DELIBERATE_FAILURE = /deliberate failure while an orphaned command holds/; +const SETUP_FAILED = /setup failed/; test("a helper outside the bound runner temp is rejected before the command is launched", async (t) => { - const fx = await lifecycleFixture(t); + const fx = await lifecycleFixture(t, { under: "/tmp" }); const sibling = await mkdtemp(join(tmpdir(), "pdpp-test-scratch-test-sibling-")); t.after(() => rm(sibling, { force: true, recursive: true })); assert.throws(() => fx.spawn("true", [], { helpers: [join(sibling, "f")] }), { message: OUTSIDE_BOUND_ROOT }); @@ -1787,10 +1770,15 @@ test("a helper outside the bound runner temp is rejected before the command is l assert.throws(() => fx.spawn("true", [], { env: { ...process.env, RUNNER_TEMP: sibling } }), { message: RUNNER_TEMP_MISMATCH, }); - // A helper made with the fixture is accepted, and the launched command can read it. + // A helper made with the fixture is accepted, and a command run through the sandboxed wrapper can read it + // below /tmp, so a broken /tmp bind-back fails here. const record = join(await fx.dir("work"), "record"); await writeFile(record, "from the fixture\n"); - const reader = fx.spawn("cat", [record], { helpers: [record] }); + const reader = fx.spawn(process.execPath, ["--import", "tsx", wrapperPath, "--", "cat", record], { + env: fx.env({ ...NO_INHERITED_OWNER, ...FRESH_SANDBOX_CONTEXT, TEST_SANDBOX_DISABLE: "0" }), + helpers: [record], + stdio: ["ignore", "pipe", "pipe"], + }); let output = ""; reader.stdout?.on("data", (chunk: Buffer) => { output += chunk.toString(); @@ -1857,6 +1845,7 @@ for (const sandboxDisabled of ["0", "1"]) { } assert.deepEqual(await closed(run), { code: 1, signal: null }, output); // The deliberate failure, not a setup failure, ended the run. + assert.doesNotMatch(output, SETUP_FAILED); assert.match(output, DELIBERATE_FAILURE); await Promise.all( (await readFile(pidsOut, "utf8")) From f7441a8086c0b3b2597ab21723278d8e0635b010 Mon Sep 17 00:00:00 2001 From: Tim Nunamaker Date: Sat, 7 Nov 2026 03:13:44 -0600 Subject: [PATCH 10/15] test(scratch): require bubblewrap for the /tmp probe and self-report leak readiness The /tmp visibility probe ran with TEST_SANDBOX_DISABLE=0, which still allows the no-bubblewrap host fallback, so it could pass without any isolation. It now sets TEST_SANDBOX_REQUIRE=1, and the lifecycle workflow installs bubblewrap and proves it works before the oracle runs. The leak regression no longer publishes a shell's background `$!` after a 200 ms settle. The command and its keep-alive child are Node programs that write their own pid from inside the running process; the fixture waits for both under the existing deadline and rejects a child that is gone. Assisted-by: AI Signed-off-by: Tim Nunamaker --- .github/workflows/test-scratch-lifecycle.yml | 11 ++++ .../fixtures/leaked-group-after-failure.ts | 50 ++++++++----------- scripts/test-scratch/run-command.test.ts | 10 +++- 3 files changed, 41 insertions(+), 30 deletions(-) diff --git a/.github/workflows/test-scratch-lifecycle.yml b/.github/workflows/test-scratch-lifecycle.yml index 267e2ee40..ebfb1b431 100644 --- a/.github/workflows/test-scratch-lifecycle.yml +++ b/.github/workflows/test-scratch-lifecycle.yml @@ -76,6 +76,17 @@ jobs: - name: Scoped strict TypeScript run: pnpm exec tsc --project scripts/test-scratch/tsconfig.json --pretty false + - name: Require bubblewrap + # The /tmp visibility probe runs with TEST_SANDBOX_REQUIRE=1. Without a working bubblewrap it must fail, + # never fall back to the host /tmp, so install it, lift Ubuntu's unprivileged-userns restriction and + # prove it here with the flags test-sandbox uses. + run: | + sudo apt-get update -qq + sudo apt-get install -y --no-install-recommends bubblewrap + sudo sysctl -w kernel.apparmor_restrict_unprivileged_userns=0 || true + bwrap --dev-bind / / --tmpfs /tmp --unshare-net true + echo "bubblewrap verified: $(bwrap --version)" + - name: Lifecycle owner oracle # The whole file runs in well under a minute; a hung process must fail the step, not the job. timeout-minutes: 5 diff --git a/scripts/test-scratch/fixtures/leaked-group-after-failure.ts b/scripts/test-scratch/fixtures/leaked-group-after-failure.ts index c49908109..7237f8a5f 100644 --- a/scripts/test-scratch/fixtures/leaked-group-after-failure.ts +++ b/scripts/test-scratch/fixtures/leaked-group-after-failure.ts @@ -12,19 +12,28 @@ import { setTimeout as sleep } from "node:timers/promises"; import { lifecycleFixture } from "./lifecycle-fixture.ts"; const wrapperPath = new URL("../run-command.ts", import.meta.url).pathname; -const PIDS = /^\d+ \d+\n$/; +const PID_FILE = /^\d+\n$/; // Shorter than the parent's 60 s timeout, so a stuck setup fails here and runs this file's after-hooks. const READY_DEADLINE_MS = 20_000; -/** Wait for the shell's readiness sentinel: its own pid and its background child's, written once both exist. */ -async function readyPids(sentinel: string): Promise { +// Each program publishes its own pid from inside the running process, atomically, once it is up. +const PUBLISH = + 'const f=require("node:fs");const w=(p)=>{f.writeFileSync(p+".tmp",process.pid+"\\n");f.renameSync(p+".tmp",p)};'; +// The background keep-alive. It inherits the wrapper's pipes, as the background child did in the CI hang. +const KEEP_ALIVE = `${PUBLISH}w(process.argv[1]);setInterval(()=>{},1e6)`; +// The command (owner). It publishes only after its keep-alive child reports ready, and exits at once if that child exits. +const COMMAND = `${PUBLISH}const c=require("node:child_process").spawn(process.execPath,["-e",${JSON.stringify(KEEP_ALIVE)},process.argv[2]],{stdio:"inherit"});c.on("exit",()=>process.exit(1));c.on("error",()=>process.exit(1));const t=setInterval(()=>{if(f.existsSync(process.argv[2])){clearInterval(t);w(process.argv[1]);setInterval(()=>{},1e6)}},10)`; + +/** Wait for both self-reported pids: the command's and its background child's. */ +async function readyPids(ownerFile: string, childFile: string): Promise<[number, number]> { const deadline = Date.now() + READY_DEADLINE_MS; while (Date.now() < deadline) { // biome-ignore lint/performance/noAwaitInLoops: bounded by READY_DEADLINE_MS. - const text = await readFile(sentinel, "utf8").catch(() => ""); - if (PIDS.test(text)) { - return text; + const owner = await readFile(ownerFile, "utf8").catch(() => ""); + const child = await readFile(childFile, "utf8").catch(() => ""); + if (PID_FILE.test(owner) && PID_FILE.test(child)) { + return [Number(owner), Number(child)]; } await sleep(25); } @@ -42,34 +51,19 @@ function alive(pid: number): boolean { test("owner killed while its command and a background child run, then an assertion fails", async (t) => { const fx = await lifecycleFixture(t); - const sentinel = fx.path("ready"); - // The background child inherits the wrapper's pipes, as it did in the CI hang. + const ownerFile = fx.path("owner-pid"); + const childFile = fx.path("child-pid"); const wrapper = fx.spawn( process.execPath, - [ - "--import", - "tsx", - wrapperPath, - "--", - "sh", - "-c", - 'sleep 300 & echo "$$ $!" > "$1.tmp" && mv "$1.tmp" "$1"; sleep 300', - "sh", - sentinel, - ], - { helpers: [sentinel] } + ["--import", "tsx", wrapperPath, "--", process.execPath, "-e", COMMAND, ownerFile, childFile], + { helpers: [ownerFile, childFile] } ); - const pids = await readyPids(sentinel); - // Let a command that fails at startup finish failing, then require the leak this test needs. - await sleep(200); - const gone = pids - .trim() - .split(" ") - .filter((pid) => !alive(Number(pid))); + const pids = await readyPids(ownerFile, childFile); + const gone = pids.filter((pid) => !alive(pid)); if (gone.length > 0 || wrapper.exitCode !== null || wrapper.signalCode !== null) { throw new Error(`setup failed: the command or its background child is not running (gone: ${gone.join(",")})`); } - await writeFile(process.env.PIDS_OUT as string, pids); + await writeFile(process.env.PIDS_OUT as string, `${pids.join(" ")}\n`); wrapper.kill("SIGKILL"); assert.fail("deliberate failure while an orphaned command holds the wrapper's pipes"); }); diff --git a/scripts/test-scratch/run-command.test.ts b/scripts/test-scratch/run-command.test.ts index c208a30ef..2b4c09df6 100644 --- a/scripts/test-scratch/run-command.test.ts +++ b/scripts/test-scratch/run-command.test.ts @@ -1771,11 +1771,17 @@ test("a helper outside the bound runner temp is rejected before the command is l message: RUNNER_TEMP_MISMATCH, }); // A helper made with the fixture is accepted, and a command run through the sandboxed wrapper can read it - // below /tmp, so a broken /tmp bind-back fails here. + // below /tmp, so a broken /tmp bind-back fails here. TEST_SANDBOX_REQUIRE=1 makes a host without a usable + // bubblewrap fail this probe instead of running the command on the unisolated host /tmp. const record = join(await fx.dir("work"), "record"); await writeFile(record, "from the fixture\n"); const reader = fx.spawn(process.execPath, ["--import", "tsx", wrapperPath, "--", "cat", record], { - env: fx.env({ ...NO_INHERITED_OWNER, ...FRESH_SANDBOX_CONTEXT, TEST_SANDBOX_DISABLE: "0" }), + env: fx.env({ + ...NO_INHERITED_OWNER, + ...FRESH_SANDBOX_CONTEXT, + TEST_SANDBOX_DISABLE: "0", + TEST_SANDBOX_REQUIRE: "1", + }), helpers: [record], stdio: ["ignore", "pipe", "pipe"], }); From 83e7295887213ecc6cfa90ae622a37d25db50369 Mon Sep 17 00:00:00 2001 From: Tim Nunamaker Date: Sat, 7 Nov 2026 03:19:42 -0600 Subject: [PATCH 11/15] test(scratch): do not require nested bubblewrap for the /tmp probe Bubblewrap cannot nest, so inside the outer sandbox of the test-accounting run the probe's TEST_SANDBOX_REQUIRE=1 exits 3. The probe requires bubblewrap only when no outer sandbox is active; the test-scratch-lifecycle workflow runs the file outside any sandbox and gates it. Assisted-by: AI Signed-off-by: Tim Nunamaker --- scripts/test-scratch/run-command.test.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/scripts/test-scratch/run-command.test.ts b/scripts/test-scratch/run-command.test.ts index 2b4c09df6..76a40d395 100644 --- a/scripts/test-scratch/run-command.test.ts +++ b/scripts/test-scratch/run-command.test.ts @@ -1772,7 +1772,9 @@ test("a helper outside the bound runner temp is rejected before the command is l }); // A helper made with the fixture is accepted, and a command run through the sandboxed wrapper can read it // below /tmp, so a broken /tmp bind-back fails here. TEST_SANDBOX_REQUIRE=1 makes a host without a usable - // bubblewrap fail this probe instead of running the command on the unisolated host /tmp. + // bubblewrap fail this probe instead of running the command on the unisolated host /tmp. Inside an outer + // sandbox (the test-accounting run) bubblewrap cannot nest, so the probe cannot require it there; the + // test-scratch-lifecycle workflow runs this file outside any sandbox and is the gate for this probe. const record = join(await fx.dir("work"), "record"); await writeFile(record, "from the fixture\n"); const reader = fx.spawn(process.execPath, ["--import", "tsx", wrapperPath, "--", "cat", record], { @@ -1780,7 +1782,7 @@ test("a helper outside the bound runner temp is rejected before the command is l ...NO_INHERITED_OWNER, ...FRESH_SANDBOX_CONTEXT, TEST_SANDBOX_DISABLE: "0", - TEST_SANDBOX_REQUIRE: "1", + TEST_SANDBOX_REQUIRE: process.env.TEST_SANDBOX_ACTIVE ? "0" : "1", }), helpers: [record], stdio: ["ignore", "pipe", "pipe"], From 178be8228332b7a8b518fff77c7195f173b9d426 Mon Sep 17 00:00:00 2001 From: Tim Nunamaker Date: Sat, 7 Nov 2026 03:25:41 -0600 Subject: [PATCH 12/15] test(scratch): skip the bind-back probe inside an outer sandbox Bubblewrap cannot nest inside the outer test sandbox, so the probe could not require it there and ran without isolation. A probe that runs unenforced and reports a pass is a false pass. Move the probe into its own test, always run it with TEST_SANDBOX_REQUIRE=1, and skip it with a reason when TEST_SANDBOX_ACTIVE is set. The lifecycle job runs it outside any sandbox and is its gate. Assisted-by: AI Signed-off-by: Tim Nunamaker --- scripts/test-scratch/run-command.test.ts | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/scripts/test-scratch/run-command.test.ts b/scripts/test-scratch/run-command.test.ts index 76a40d395..c7dfe3e2c 100644 --- a/scripts/test-scratch/run-command.test.ts +++ b/scripts/test-scratch/run-command.test.ts @@ -1762,7 +1762,7 @@ const DELIBERATE_FAILURE = /deliberate failure while an orphaned command holds/; const SETUP_FAILED = /setup failed/; test("a helper outside the bound runner temp is rejected before the command is launched", async (t) => { - const fx = await lifecycleFixture(t, { under: "/tmp" }); + const fx = await lifecycleFixture(t); const sibling = await mkdtemp(join(tmpdir(), "pdpp-test-scratch-test-sibling-")); t.after(() => rm(sibling, { force: true, recursive: true })); assert.throws(() => fx.spawn("true", [], { helpers: [join(sibling, "f")] }), { message: OUTSIDE_BOUND_ROOT }); @@ -1770,11 +1770,16 @@ test("a helper outside the bound runner temp is rejected before the command is l assert.throws(() => fx.spawn("true", [], { env: { ...process.env, RUNNER_TEMP: sibling } }), { message: RUNNER_TEMP_MISMATCH, }); - // A helper made with the fixture is accepted, and a command run through the sandboxed wrapper can read it - // below /tmp, so a broken /tmp bind-back fails here. TEST_SANDBOX_REQUIRE=1 makes a host without a usable - // bubblewrap fail this probe instead of running the command on the unisolated host /tmp. Inside an outer - // sandbox (the test-accounting run) bubblewrap cannot nest, so the probe cannot require it there; the - // test-scratch-lifecycle workflow runs this file outside any sandbox and is the gate for this probe. +}); + +test("a helper made with the fixture is readable through the sandboxed wrapper below /tmp", async (t) => { + if (process.env.TEST_SANDBOX_ACTIVE) { + t.skip("bind-back isolation is gated by the lifecycle job; bwrap cannot nest inside the outer sandbox"); + return; + } + // A broken /tmp bind-back fails here. TEST_SANDBOX_REQUIRE=1 makes a host without a usable bubblewrap fail + // instead of running the command on the unisolated host /tmp. + const fx = await lifecycleFixture(t, { under: "/tmp" }); const record = join(await fx.dir("work"), "record"); await writeFile(record, "from the fixture\n"); const reader = fx.spawn(process.execPath, ["--import", "tsx", wrapperPath, "--", "cat", record], { @@ -1782,7 +1787,7 @@ test("a helper outside the bound runner temp is rejected before the command is l ...NO_INHERITED_OWNER, ...FRESH_SANDBOX_CONTEXT, TEST_SANDBOX_DISABLE: "0", - TEST_SANDBOX_REQUIRE: process.env.TEST_SANDBOX_ACTIVE ? "0" : "1", + TEST_SANDBOX_REQUIRE: "1", }), helpers: [record], stdio: ["ignore", "pipe", "pipe"], From 3376fa6254068784dac0d1f3adee97c0d3c0b53e Mon Sep 17 00:00:00 2001 From: Tim Nunamaker Date: Sat, 7 Nov 2026 03:28:50 -0600 Subject: [PATCH 13/15] test(accounting): declare the bind-back probe skip in the scratch-lifecycle baseline Inside the outer test sandbox the bind-back probe skips with a stated reason, because bubblewrap cannot nest there; the lifecycle job runs it outside any sandbox. Test accounting requires every skip to match the profile baseline exactly, so declare this one. Assisted-by: AI Signed-off-by: Tim Nunamaker --- test-accounting.manifest.json | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test-accounting.manifest.json b/test-accounting.manifest.json index 4efdfcce4..ffbfb15a2 100644 --- a/test-accounting.manifest.json +++ b/test-accounting.manifest.json @@ -44,7 +44,9 @@ { "id": "default", "required": true, - "skip_reasons": {} + "skip_reasons": { + "bind-back isolation is gated by the lifecycle job; bwrap cannot nest inside the outer sandbox": 1 + } } ], "environment_unset": [ From c12628d01016885b984afcb25b352a9855f9b677 Mon Sep 17 00:00:00 2001 From: Tim Nunamaker Date: Sat, 7 Nov 2026 03:53:24 -0600 Subject: [PATCH 14/15] fix(test-sandbox): fail closed under TEST_SANDBOX_REQUIRE on bash older than 5 On bash older than 5 (stock macOS ships 3.2) the script ran the command directly before it reached the TEST_SANDBOX_REQUIRE check, so a run that required isolation got none and could pass. Check the requirement first on that path and exit 3, as the other unsupported paths do. TEST_SANDBOX_DISABLE=1 still runs directly. Assisted-by: AI Signed-off-by: Tim Nunamaker --- scripts/test-sandbox/test-sandbox | 3 +++ 1 file changed, 3 insertions(+) diff --git a/scripts/test-sandbox/test-sandbox b/scripts/test-sandbox/test-sandbox index b2403cfce..e20b8cc23 100755 --- a/scripts/test-sandbox/test-sandbox +++ b/scripts/test-sandbox/test-sandbox @@ -20,6 +20,9 @@ # unless --receipt is given. # Old bash (macOS ships 3.2) cannot parse what follows: run the command directly. if [ -z "${BASH_VERSINFO:-}" ] || [ "${BASH_VERSINFO[0]}" -lt 5 ]; then + if [ "${TEST_SANDBOX_REQUIRE:-0}" = 1 ] && [ "${TEST_SANDBOX_DISABLE:-0}" != 1 ]; then + echo "test-sandbox: bash 5 not found; required enforcement layer unavailable" >&2; exit 3 + fi echo "test-sandbox: bash 5 not found; running without enforcement" >&2 # Drop our options (bash 3.2 syntax only): everything after "--" is the command. # The scratch contract still holds: TMPDIR is --scratch (created 0700 if From 01cb04f24e732b69ae1ae5f57434ad6d6806f126 Mon Sep 17 00:00:00 2001 From: Tim Nunamaker Date: Sat, 7 Nov 2026 03:57:55 -0600 Subject: [PATCH 15/15] chore(test-sandbox): record the digest of the fail-closed old-bash fix The vendored executable now carries the upstream fix that makes TEST_SANDBOX_REQUIRE=1 exit 3 on bash older than 5. Update its recorded sha256 and name that upstream commit in the provenance note. Assisted-by: AI Signed-off-by: Tim Nunamaker --- scripts/test-sandbox/README.md | 2 +- scripts/test-sandbox/test-sandbox.sha256 | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/test-sandbox/README.md b/scripts/test-sandbox/README.md index 46a2c7814..a4eb07158 100644 --- a/scripts/test-sandbox/README.md +++ b/scripts/test-sandbox/README.md @@ -10,7 +10,7 @@ names the enforcement that remains. `TEST_SANDBOX_REQUIRE=1` makes missing enforcement an error. `TEST_SANDBOX_DISABLE=1` runs the command directly while keeping scratch ownership and cleanup. -The executable is copied byte-for-byte from `ai/skills/local/test-architecture/scripts/test-sandbox` in Tim Nunamaker's dotfiles at commit `ce0d966`. Change and test that source first, then update this copy and digest: +The executable is copied from `ai/skills/local/test-architecture/scripts/test-sandbox` in Tim Nunamaker's dotfiles at commit `ce0d966`, plus one later upstream fix applied as-is: commit `e4b320a` makes `TEST_SANDBOX_REQUIRE=1` exit 3 on bash older than 5 instead of running the command without isolation. Change and test that source first, then update this copy and digest: ```sh cp /ai/skills/local/test-architecture/scripts/test-sandbox scripts/test-sandbox/test-sandbox diff --git a/scripts/test-sandbox/test-sandbox.sha256 b/scripts/test-sandbox/test-sandbox.sha256 index 55259c461..771a7d78e 100644 --- a/scripts/test-sandbox/test-sandbox.sha256 +++ b/scripts/test-sandbox/test-sandbox.sha256 @@ -1 +1 @@ -b51043419454d9836b9a90014fe7a077129711913751bf9c59cba8c747203941 test-sandbox +f4ede419008088fdaa751087b202098adfe5b65075edb9f057b7b26eb2c4284d test-sandbox