From 042daeee71a3cd498d96a72a3eba9dcc0fcf52a9 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Tue, 8 Sep 2026 15:08:02 +0000 Subject: [PATCH] fix: eliminate browser and process-cleanup test flakes Pin every CI surface to Bun 1.4.0 to avoid extra-stdio finalizers closing reused live sockets. Add an isolated GC/listener regression that fails on Bun 1.3.13, and prevent coordinated rollback to an affected CI runtime. Check renderer cleanup against the render's own staging directory so concurrent renders cannot invalidate the assertion. Make the no-pgrep process-tree walk tolerate disappearing /proc entries, and synchronize its test fixture through child readiness and pipe EOF instead of sleeps. Validation: 9,157 passed, 31 skipped, zero failures across 556 files with retries disabled. Build, all-host generation freshness, and skill checks passed. All three races have failing-before/passing-after regressions. --- .github/docker/Dockerfile.ci | 4 +- .github/workflows/evals-periodic.yml | 2 +- .github/workflows/evals.yml | 2 +- .github/workflows/free-tests.yml | 2 +- .github/workflows/make-pdf-gate.yml | 2 +- .github/workflows/quality-gate.yml | 2 +- .github/workflows/skill-docs.yml | 2 +- .github/workflows/version-gate.yml | 2 +- .github/workflows/windows-free-tests.yml | 2 +- .github/workflows/windows-setup-e2e.yml | 2 +- .gitlab-ci.yml | 2 +- CONTRIBUTING.md | 5 ++ setup | 15 +++- test/aside-render.test.ts | 89 ++++++++++++++++----- test/bun-subprocess-fd-lifetime.test.ts | 81 +++++++++++++++++++ test/bun-version-drift.test.ts | 14 ++++ test/setup-playwright-best-effort.test.ts | 97 ++++++++++++++++------- 17 files changed, 262 insertions(+), 63 deletions(-) create mode 100644 test/bun-subprocess-fd-lifetime.test.ts diff --git a/.github/docker/Dockerfile.ci b/.github/docker/Dockerfile.ci index 39247f4a1..ff4b4c87f 100644 --- a/.github/docker/Dockerfile.ci +++ b/.github/docker/Dockerfile.ci @@ -69,10 +69,10 @@ RUN curl --retry 5 --retry-delay 5 --retry-connrefused -fsSL "https://nodejs.org # The version MUST be passed as a positional arg — bun.sh/install ignores a # BUN_VERSION env var, so the old `| BUN_VERSION=x.y.z bash` form silently # installed latest on every image rebuild (observed: 1.3.13/1.3.14 drift vs -# the 1.3.10 devs run locally). +# the 1.3.10 devs ran locally). ENV BUN_INSTALL="/usr/local" RUN curl --retry 5 --retry-delay 5 --retry-connrefused -fsSL https://bun.sh/install \ - | bash -s "bun-v1.3.13" + | bash -s "bun-v1.4.0" # Claude CLI — pinned to an EXACT version, same discipline as the bun pin # above. The PTY harness (test/helpers/claude-pty-runner.ts) screen-scrapes diff --git a/.github/workflows/evals-periodic.yml b/.github/workflows/evals-periodic.yml index 67c92ef1a..8b114aa63 100644 --- a/.github/workflows/evals-periodic.yml +++ b/.github/workflows/evals-periodic.yml @@ -272,7 +272,7 @@ jobs: - uses: oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6 # v2 with: - bun-version: 1.3.13 + bun-version: 1.4.0 - run: bun install --frozen-lockfile diff --git a/.github/workflows/evals.yml b/.github/workflows/evals.yml index 71231a87a..4194ac092 100644 --- a/.github/workflows/evals.yml +++ b/.github/workflows/evals.yml @@ -261,7 +261,7 @@ jobs: - uses: oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6 # v2 with: - bun-version: 1.3.13 + bun-version: 1.4.0 - run: bun install --frozen-lockfile diff --git a/.github/workflows/free-tests.yml b/.github/workflows/free-tests.yml index 3dcfc1ad5..fd0368df0 100644 --- a/.github/workflows/free-tests.yml +++ b/.github/workflows/free-tests.yml @@ -54,7 +54,7 @@ jobs: - uses: oven-sh/setup-bun@v2 with: - bun-version: 1.3.13 + bun-version: 1.4.0 - uses: actions/cache@v6 with: diff --git a/.github/workflows/make-pdf-gate.yml b/.github/workflows/make-pdf-gate.yml index e35f6d590..f9ee442c7 100644 --- a/.github/workflows/make-pdf-gate.yml +++ b/.github/workflows/make-pdf-gate.yml @@ -53,7 +53,7 @@ jobs: - uses: oven-sh/setup-bun@v2 with: - bun-version: 1.3.13 + bun-version: 1.4.0 - name: Install dependencies run: bun install --frozen-lockfile diff --git a/.github/workflows/quality-gate.yml b/.github/workflows/quality-gate.yml index 6fc745e75..9168e832a 100644 --- a/.github/workflows/quality-gate.yml +++ b/.github/workflows/quality-gate.yml @@ -38,7 +38,7 @@ jobs: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v4 - uses: oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6 # v2 with: - bun-version: 1.3.13 + bun-version: 1.4.0 - name: Install frozen dependencies run: bun install --frozen-lockfile --ignore-scripts diff --git a/.github/workflows/skill-docs.yml b/.github/workflows/skill-docs.yml index bdd3ec3b7..966b878be 100644 --- a/.github/workflows/skill-docs.yml +++ b/.github/workflows/skill-docs.yml @@ -28,7 +28,7 @@ jobs: - uses: actions/checkout@v7 - uses: oven-sh/setup-bun@v2 with: - bun-version: 1.3.13 + bun-version: 1.4.0 - run: bun install --frozen-lockfile # One generation pass for ALL 10 hosts. gen-skill-docs --host all # hard-fails on any per-host generation error (scripts/gen-skill-docs.ts diff --git a/.github/workflows/version-gate.yml b/.github/workflows/version-gate.yml index 3c8dcb1f6..fb715c03d 100644 --- a/.github/workflows/version-gate.yml +++ b/.github/workflows/version-gate.yml @@ -29,7 +29,7 @@ jobs: - name: Setup Bun uses: oven-sh/setup-bun@v2 with: - bun-version: 1.3.13 + bun-version: 1.4.0 - name: Read versions id: versions diff --git a/.github/workflows/windows-free-tests.yml b/.github/workflows/windows-free-tests.yml index 25170f364..8879f79a0 100644 --- a/.github/workflows/windows-free-tests.yml +++ b/.github/workflows/windows-free-tests.yml @@ -47,7 +47,7 @@ jobs: - uses: oven-sh/setup-bun@v2 with: - bun-version: 1.3.13 + bun-version: 1.4.0 # bun install was 35s of a 55s job, all network. Cache keyed on the # lockfile; bun's install cache lives under ~/.bun/install/cache on diff --git a/.github/workflows/windows-setup-e2e.yml b/.github/workflows/windows-setup-e2e.yml index 6eeac3b2e..9f41c1d03 100644 --- a/.github/workflows/windows-setup-e2e.yml +++ b/.github/workflows/windows-setup-e2e.yml @@ -45,7 +45,7 @@ jobs: - uses: oven-sh/setup-bun@v2 with: - bun-version: 1.3.13 + bun-version: 1.4.0 # Same lockfile-keyed install cache as windows-free-tests.yml (install # was 45s of a 64s job, all network). diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 583d5d78d..4f9345feb 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -6,7 +6,7 @@ stages: - check variables: - BUN_VERSION: "1.3.13" + BUN_VERSION: "1.4.0" .setup-bun: &setup-bun - apt-get update -qq && apt-get install -qq -y curl jq git diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 5d388f4f1..9d4264d54 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -138,6 +138,11 @@ never touches a symlinked or non-gstack directory. ### Setup +Development and tests require Bun 1.4.0 or newer; CI pins and tests 1.4.0. +Earlier Linux versions can close unrelated live file descriptors during +subprocess garbage collection, causing intermittent browser and HTTP fixture +failures ([upstream diagnosis](https://github.com/oven-sh/bun/issues/34785#issuecomment-5020318035)). + ```bash # 1. Copy .env.example and add your API key cp .env.example .env diff --git a/setup b/setup index cd4765ee1..57383a419 100755 --- a/setup +++ b/setup @@ -675,9 +675,9 @@ fi # potentially leaving Playwright cache locks held. macOS ships no setsid # binary, so a portable group-kill isn't available; walk `pgrep -P` children # depth-first instead (pgrep exists on macOS and Linux). Falls back to a -# plain kill of the root pid when pgrep is unavailable. +# /proc walk where available, otherwise a plain kill of the root pid. _kill_tree() { - local pid="$1" child + local pid="$1" child stat_file stat_line proc_state proc_parent proc_rest if command -v pgrep >/dev/null 2>&1; then for child in $(pgrep -P "$pid" 2>/dev/null); do _kill_tree "$child" @@ -686,8 +686,15 @@ _kill_tree() { # debian-slim and git-bash ship no pgrep: walk /proc for children. The # comm field "(name)" may contain spaces and parens, so strip through the # LAST closing paren (proc(5)) before reading the ppid (second field after it). - for child in $(awk -v p="$pid" '{ s=$0; sub(/^.*\) /, "", s); split(s, f, " "); if (f[2]==p) print $1 }' /proc/[0-9]*/stat 2>/dev/null); do - _kill_tree "$child" + # A process may exit after glob expansion. One awk over every stat file + # aborts at that missing entry and never visits the remaining live children. + for stat_file in /proc/[0-9]*/stat; do + IFS= read -r stat_line 2>/dev/null < "$stat_file" || continue + child="${stat_line%% *}" + IFS=' ' read -r proc_state proc_parent proc_rest <<< "${stat_line##*) }" + if [ "$proc_parent" = "$pid" ]; then + _kill_tree "$child" + fi done fi kill -9 "$pid" 2>/dev/null || true diff --git a/test/aside-render.test.ts b/test/aside-render.test.ts index 53460c267..70003824f 100644 --- a/test/aside-render.test.ts +++ b/test/aside-render.test.ts @@ -6,7 +6,7 @@ * installed and open (macOS dev machines); the live fallback render runs * wherever a browse binary resolves (Linux CI builds one via build:gates). */ -import { describe, test, expect, beforeAll, afterAll, setDefaultTimeout } from 'bun:test'; +import { describe, test, expect, beforeAll, afterAll, setDefaultTimeout, spyOn } from 'bun:test'; import * as fs from 'fs'; import * as os from 'os'; import * as path from 'path'; @@ -114,8 +114,7 @@ async function liveRoundTrip(engine: 'aside' | 'browse', renderFn: typeof render ], timeoutMs: 90_000, }); - expect(out.error).toBeUndefined(); - expect(out.ok).toBe(true); + expectOk(out); expect(out.engine).toBe(engine); expect(out.outputs).toEqual([path.join(dir, 'out.pdf'), path.join(dir, 'm.jpg'), path.join(dir, 'v.txt'), path.join(dir, 'bytes.bin')]); expect(fs.readFileSync(path.join(dir, 'out.pdf')).subarray(0, 4).toString()).toBe('%PDF'); @@ -135,8 +134,7 @@ async function lateReadiness(engine: 'aside' | 'browse', renderFn: typeof render fs.writeFileSync(path.join(dir, 'late.html'), 'Late'); try { const out = await renderFn({ file: path.join(dir, 'late.html'), waitFor: { expression: 'window.later.ok', timeoutMs: 10_000 }, steps: [{ kind: 'eval', expression: 'document.title' }], timeoutMs: 60_000 }); - expect(out.error).toBeUndefined(); - expect(out.ok).toBe(true); + expectOk(out); expect(out.engine).toBe(engine); expect(out.evals[0]).toBe('Late'); } finally { @@ -318,7 +316,32 @@ const expectOk = (r: RenderResult): void => { // not latency. Bun's 5s default once failed a CI run whose render was merely slow // under a full six-shard load, so the budget is generous and hangs still fail. setDefaultTimeout(30_000); -const browseWorkDirs = (): string[] => fs.readdirSync(SAFE_TMP_DIR).filter((n) => n.startsWith('gstack-render-browse-')); + +/** Check this render's staging directory, regardless of other renders using /tmp. */ +async function renderCheckingCleanup(spec: RenderSpec, bin: string, renderFn = renderWithBrowse): Promise { + const workDirs: string[] = []; + const mkdtemp = fs.mkdtempSync; + // The renderer allocates before its first await. Observe that synchronous + // call, then restore immediately so unrelated async work is never captured. + const allocation = spyOn(fs, 'mkdtempSync').mockImplementation(((...args: Parameters) => { + const dir = mkdtemp(...args); + if (args[0] === path.join(SAFE_TMP_DIR, 'gstack-render-browse-')) workDirs.push(dir.toString()); + return dir; + }) as typeof fs.mkdtempSync); + let pending: Promise; + try { + pending = renderFn(spec, bin); + } finally { + allocation.mockRestore(); + } + try { + return await pending; + } finally { + // A changed allocation boundary must fail, not silently skip leak checks. + expect(workDirs, 'expected to observe this render\'s staging directory').toHaveLength(1); + for (const dir of workDirs) expect(fs.existsSync(dir), `render leaked staging directory: ${dir}`).toBe(false); + } +} /** The subprocess driver: one job per process, so the module's engine cache and the spawn-time PATH are both under the test's control. */ function writeDriver(dir: string): string { @@ -656,10 +679,47 @@ describe.skipIf(!HERMETIC)('aside-render: renderWithBrowse — daemon CLI contra }; const T = '--tab-id 7'; + test('cleanup remains verifiable when another render removes its staging directory', async () => { + const sibling = fs.mkdtempSync(path.join(SAFE_TMP_DIR, 'gstack-render-browse-')); + try { + // Synchronize the other render's cleanup with our newtab command, so + // this reproduces the shared-/tmp race without relying on timing. + const b = fake({ newtab: `rmdir '${sibling.replaceAll("'", "'\\''")}'\necho '{"tabId":7}'` }); + const r = await renderCheckingCleanup({ file: doc, steps: [] }, b); + expectOk(r); + expect(fs.existsSync(sibling)).toBe(false); + } finally { + fs.rmSync(sibling, { recursive: true, force: true }); + } + }); + + test('cleanup leaves another render\'s new staging directory intact', async () => { + const sibling = fs.mkdtempSync(path.join(SAFE_TMP_DIR, 'gstack-render-browse-')); + fs.rmdirSync(sibling); + try { + const b = fake({ newtab: `mkdir '${sibling.replaceAll("'", "'\\''")}'\necho '{"tabId":7}'` }); + expectOk(await renderCheckingCleanup({ file: doc, steps: [] }, b)); + expect(fs.existsSync(sibling)).toBe(true); + } finally { + fs.rmSync(sibling, { recursive: true, force: true }); + } + }); + + test('cleanup check still rejects an owned staging directory leak', async () => { + let leaked: string | undefined; + try { + await expect(renderCheckingCleanup({ file: doc, steps: [] }, 'unused', async () => { + leaked = fs.mkdtempSync(path.join(SAFE_TMP_DIR, 'gstack-render-browse-')); + return { ok: true, engine: 'browse', outputs: [], evals: {}, stdout: '' }; + })).rejects.toThrow('render leaked staging directory'); + } finally { + if (leaked) fs.rmSync(leaked, { recursive: true, force: true }); + } + }); + test('happy path: newtab → goto → per-step CLI calls → closetab; artifacts copied, evals inline, work dir and server released', async () => { - const before = browseWorkDirs(); const b = fake(); - const r = await renderWithBrowse({ + const r = await renderCheckingCleanup({ file: doc, steps: [ { kind: 'pdf', out: path.join(outDir, 'doc.pdf'), options: { paperWidth: 8.5, paperHeight: 11 } }, @@ -693,23 +753,19 @@ describe.skipIf(!HERMETIC)('aside-render: renderWithBrowse — daemon CLI contra const payload = fs.readFileSync(`${log}.payloads`, 'utf8'); expect(payload).toContain('"width":"8.5in"'); expect(payload).toMatch(/"output":"\/tmp\/gstack-render-browse-[^"]+\/gstack-render-0\.pdf"/); - expect(browseWorkDirs()).toEqual(before); // /tmp staging dir removed await expect(fetch(goto.slice('goto '.length, -` ${T}`.length))).rejects.toThrow(); // loopback server stopped }); test('`newtab --json` without a tabId → the named error, no closetab, no staging dir left in /tmp', async () => { - const before = browseWorkDirs(); - const r = await renderWithBrowse({ file: doc, steps: [{ kind: 'eval', expression: '1' }] }, fake({ newtab: `echo '{"ok":true}'` })); + const r = await renderCheckingCleanup({ file: doc, steps: [{ kind: 'eval', expression: '1' }] }, fake({ newtab: `echo '{"ok":true}'` })); expect(r.ok).toBe(false); expect(r.engine).toBe('browse'); expect(r.error).toBe('browse newtab --json returned no tabId'); expect(readLines(log)).toEqual(['newtab --json']); - expect(browseWorkDirs()).toEqual(before); }); test('a failing goto → "browse goto failed: ", the tab is still closed, /tmp is left clean', async () => { - const before = browseWorkDirs(); - const r = await renderWithBrowse({ file: doc, steps: [{ kind: 'pdf', out: path.join(outDir, 'x.pdf') }] }, fake({ goto: 'echo "net::ERR_CONNECTION_REFUSED at http://127.0.0.1" >&2; echo "second line" >&2; exit 1' })); + const r = await renderCheckingCleanup({ file: doc, steps: [{ kind: 'pdf', out: path.join(outDir, 'x.pdf') }] }, fake({ goto: 'echo "net::ERR_CONNECTION_REFUSED at http://127.0.0.1" >&2; echo "second line" >&2; exit 1' })); expect(r.ok).toBe(false); expect(r.error!.startsWith('browse goto failed:')).toBe(true); expect(r.error).toContain('net::ERR_CONNECTION_REFUSED'); @@ -719,7 +775,6 @@ describe.skipIf(!HERMETIC)('aside-render: renderWithBrowse — daemon CLI contra expect(lines.some((l) => l.startsWith('goto '))).toBe(true); expect(lines.at(-1)).toBe('closetab 7'); expect(lines.some((l) => l.startsWith('pdf '))).toBe(false); - expect(browseWorkDirs()).toEqual(before); expect(fs.existsSync(path.join(outDir, 'x.pdf'))).toBe(false); }); @@ -803,19 +858,17 @@ describe.skipIf(!HERMETIC)('aside-render: renderWithBrowse — daemon CLI contra // runProc is not exported: its timeout + kill path is observed through a hanging fake. test('a CLI call that hangs past spec.timeoutMs is killed and reported as timed out — even when a grandchild keeps the pipes open', async () => { - const before = browseWorkDirs(); // `sleep` is a CHILD of the sh fake, so SIGTERM kills sh while sleep still holds stdout/stderr: // the read must give up on its own (timeout + 10s) rather than wait for EOF. 14s (not 30s) so no orphan outlives this file. const b = fake({ newtab: 'sleep 14' }); const t0 = Date.now(); - const r = await renderWithBrowse({ file: doc, steps: [{ kind: 'eval', expression: '1' }], timeoutMs: 1_500 }, b); + const r = await renderCheckingCleanup({ file: doc, steps: [{ kind: 'eval', expression: '1' }], timeoutMs: 1_500 }, b); const elapsed = Date.now() - t0; expect(r.ok).toBe(false); expect(r.error!.startsWith('browse newtab failed:')).toBe(true); expect(r.error).toContain('timed out'); expect(elapsed).toBeLessThan(25_000); expect(readLines(log)).toEqual(['newtab --json']); // no tab → nothing to close - expect(browseWorkDirs()).toEqual(before); }, 40_000); test('a hanging CLI that honours SIGTERM is reaped promptly at the budget', async () => { diff --git a/test/bun-subprocess-fd-lifetime.test.ts b/test/bun-subprocess-fd-lifetime.test.ts new file mode 100644 index 000000000..6714298c2 --- /dev/null +++ b/test/bun-subprocess-fd-lifetime.test.ts @@ -0,0 +1,81 @@ +import { expect, test } from 'bun:test'; + +// Run the ownership probe in a child: an affected Bun can close arbitrary +// recycled descriptors, including the test runner's own sockets and pipes. +// Playwright uses the same extra-stdio slots for Chromium's CDP transport. +// Upstream ownership fixes: oven-sh/bun#32520 and oven-sh/bun#33828. +const fixture = String.raw` + const { spawn } = require('node:child_process'); + const rounds = 4; + const listenersPerRound = 8; + for (let round = 0; round < rounds; round++) { + let child = spawn(process.execPath, ['-e', ''], { + stdio: ['ignore', 'ignore', 'ignore', 'pipe', 'pipe', 'pipe'], + }); + await new Promise((resolve, reject) => { + child.once('exit', resolve); + child.once('error', reject); + }); + await Promise.all(child.stdio.slice(3).map(socket => { + if (socket.closed) return; + return new Promise(resolve => { + // Subscribe before destroy: descriptor reuse must follow the actual + // close event, not a delay that may expire before close under load. + socket.once('close', resolve); + socket.destroy(); + }); + })); + + // The OS can now reuse the closed extra-stdio descriptors for these + // listeners. Capture their URLs before GC; affected runtimes can also + // invalidate server.port when the underlying listener vanishes. + const listeners = Array.from({ length: listenersPerRound }, () => { + const server = Bun.serve({ + hostname: '127.0.0.1', port: 0, fetch: () => new Response('alive'), + }); + return { server, url: 'http://127.0.0.1:' + server.port + '/' }; + }); + child = null; + Bun.gc(true); + await Bun.sleep(0); + Bun.gc(true); + + const failures = []; + for (const { url } of listeners) { + try { + const response = await fetch(url, { signal: AbortSignal.timeout(1_000) }); + const body = await response.text(); + if (response.status !== 200 || body !== 'alive') { + failures.push({ url, status: response.status, body }); + } + } catch (error) { + failures.push({ url, error: String(error) }); + } + } + if (failures.length) { + console.error(JSON.stringify({ bun: Bun.version, round, failures })); + // Only this isolated process is affected. Avoid asking the broken + // runtime to close descriptors again; process exit releases them. + process.exit(1); + } + for (const { server } of listeners) await server.stop(true); + } + console.log(JSON.stringify({ checkedListeners: rounds * listenersPerRound })); +`; + +test.skipIf(process.platform === 'win32')('extra-stdio cleanup preserves unrelated HTTP listeners after GC', async () => { + const child = Bun.spawn([process.execPath, '-e', fixture], { + stdin: 'ignore', stdout: 'pipe', stderr: 'pipe', timeout: 10_000, killSignal: 'SIGKILL', + }); + const [stdout, stderr, code] = await Promise.all([ + new Response(child.stdout).text(), new Response(child.stderr).text(), child.exited, + ]); + const diagnosis = `Bun ${Bun.version} failed the subprocess descriptor-ownership probe. ` + + 'Install the repository\'s pinned Bun version (1.4.0 or newer); ' + + 'older Bun can double-close extra stdio and destroy unrelated browser/server sockets ' + + '(oven-sh/bun#32520, #33828).\n' + + `exit=${code}\nstdout:\n${stdout}\nstderr:\n${stderr}`; + expect(code, diagnosis).toBe(0); + expect(JSON.parse(stdout), diagnosis).toEqual({ checkedListeners: 32 }); + expect(stderr, diagnosis).toBe(''); +}, 15_000); diff --git a/test/bun-version-drift.test.ts b/test/bun-version-drift.test.ts index ce3dd4dd5..98a80f917 100644 --- a/test/bun-version-drift.test.ts +++ b/test/bun-version-drift.test.ts @@ -73,4 +73,18 @@ describe('bun version pins', () => { expect(versions, `bun version drift across CI surfaces:\n${detail}`).toHaveLength(1); expect(versions[0]).toMatch(/^\d+\.\d+\.\d+$/); }); + + test('every CI surface requires Bun 1.4.0 or newer for safe extra-stdio ownership', () => { + // Matching pins alone would allow every lane to regress together. Older + // Linux Bun releases double-close extra stdio FDs during subprocess GC, + // which can close unrelated listeners after the OS reuses an FD number. + // https://github.com/oven-sh/bun/issues/34785#issuecomment-5020318035 + for (const pin of collectPins()) { + expect(pin.version, `${pin.surface} must pin a stable numeric version`).toMatch(/^\d+\.\d+\.\d+$/); + expect( + Bun.semver.satisfies(pin.version, '>=1.4.0'), + `${pin.surface} pins Bun ${pin.version}; Bun >=1.4.0 is required for safe extra-stdio ownership`, + ).toBe(true); + } + }); }); diff --git a/test/setup-playwright-best-effort.test.ts b/test/setup-playwright-best-effort.test.ts index bb5829f3d..746d04102 100644 --- a/test/setup-playwright-best-effort.test.ts +++ b/test/setup-playwright-best-effort.test.ts @@ -180,6 +180,62 @@ function runBlock(opts: { return { stdout: r.stdout ?? '', stderr: r.stderr ?? '', status: r.status ?? -1, elapsedMs: Date.now() - t0, tmp }; } +/** Run the real fallback against a child that reports readiness over a pipe. */ +function runKillTreeFallback(disappearingEntry = false): { status: number | null; stdout: string; stderr: string } { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-killtree-')); + try { + const bin = path.join(tmp, 'bin'); + fs.mkdirSync(bin); + for (const name of ['bash', 'sleep', 'awk', 'mkdir']) { + const real = (spawnSync('which', [name], { encoding: 'utf-8', timeout: 10_000 }).stdout ?? '').trim(); + if (!real) throw new Error(`kill-tree fixture needs ${name}`); + fs.symlinkSync(real, path.join(bin, name)); + } + const procRoot = path.join(tmp, 'proc'); + if (disappearingEntry) { + fs.mkdirSync(path.join(procRoot, '0'), { recursive: true }); + // Glob expansion still includes this symlink, but opening it fails just + // like /proc//stat when that process exits before the reader opens it. + fs.symlinkSync(path.join(tmp, 'exited-process'), path.join(procRoot, '0', 'stat')); + } + const killTree = disappearingEntry + ? extractFn('_kill_tree').replace('/proc/[0-9]*/stat', `"${procRoot}"/[0-9]*/stat`) + : extractFn('_kill_tree'); + const script = [ + `export PATH="${bin}"`, + 'hash -r', + 'command -v pgrep >/dev/null 2>&1 && { echo "PGREP_PRESENT"; exit 1; }', + killTree, + // The intermediate shell reports the real grandchild PID after fork. + // Both processes hold the output pipe open until they are terminated. + 'coproc TREE { sleep 30 & printf "%s\\n" "$!"; wait; }', + 'pid=$TREE_PID; child=', + 'trap \'kill -9 "${child:-}" "$pid" 2>/dev/null || true; wait "$pid" 2>/dev/null || true\' EXIT', + 'exec {reader}<&"${TREE[0]}"', + 'IFS= read -r -t 5 child <&"$reader" || { echo "CHILD_NOT_READY"; exit 1; }', + '[[ "$child" =~ ^[1-9][0-9]*$ ]] || { echo "INVALID_CHILD_PID"; exit 1; }', + 'kill -0 "$child" 2>/dev/null || { echo "CHILD_EXITED_BEFORE_KILL"; exit 1; }', + ...(disappearingEntry ? [ + `mkdir "${procRoot}/$child"`, + 'IFS= read -r stat_line < "/proc/$child/stat"', + // Keep the real pid/ppid and remaining fields. A comm containing both + // spaces and parentheses must not shift the parser's parent PID field. + `printf '%s (fixture child ) with (parens)) %s\\n' "$child" "${'$'}{stat_line##*) }" > "${procRoot}/$child/stat"`, + ] : []), + '_kill_tree "$pid"', + // EOF witnesses termination even if init has not yet reaped the child. + // A missed descendant still holds the pipe, causing a bounded failure. + 'if IFS= read -r -t 5 remaining <&"$reader"; then echo "UNEXPECTED_CHILD_OUTPUT"; exit 1; else read_code=$?; fi', + '[ "$read_code" -eq 1 ] || { echo "CHILD_ALIVE"; exit 1; }', + 'echo "CHILD_DEAD"', + ].join('\n'); + const r = spawnSync('/bin/bash', ['-c', script], { encoding: 'utf-8', timeout: 20_000 }); + return { status: r.status, stdout: r.stdout ?? '', stderr: (r.stderr ?? '') + (r.error ? `\n${r.error.message}` : '') }; + } finally { + fs.rmSync(tmp, { recursive: true, force: true }); + } +} + describe('setup: Chromium bootstrap block executes best-effort', () => { test('probe ok: no reason recorded, no install attempted', () => { const r = runBlock({ probe: 'ok', bunx: 'exit 0' }); @@ -263,35 +319,18 @@ describe('setup: Chromium bootstrap block executes best-effort', () => { test('_kill_tree without pgrep on PATH still kills the grandchild (walks /proc)', () => { if (!fs.existsSync('/proc')) return; - const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-killtree-')); - try { - const bin = path.join(tmp, 'bin'); - fs.mkdirSync(bin); - for (const name of ['bash', 'sleep', 'awk']) { - const real = (spawnSync('which', [name], { encoding: 'utf-8', timeout: 10_000 }).stdout ?? '').trim(); - if (real) fs.symlinkSync(real, path.join(bin, name)); - } - const script = [ - `export PATH="${bin}"`, - 'hash -r', - 'command -v pgrep >/dev/null 2>&1 && { echo "PGREP_PRESENT"; exit 0; }', - extractFn('_kill_tree'), - // two commands so bash forks a real subshell instead of exec-ing sleep directly - '( sleep 30; true ) & pid=$!', - 'sleep 0.3', - // find the sleep grandchild via /proc, the same way the fallback does - 'child=$(awk -v p="$pid" \'{ s=$0; sub(/^[^)]*\\) /, "", s); split(s, f, " "); if (f[2]==p) { print $1; exit } }\' /proc/[0-9]*/stat 2>/dev/null)', - '[ -n "$child" ] || { echo "NO_CHILD"; exit 0; }', - '_kill_tree "$pid"', - 'sleep 0.3', - 'if kill -0 "$child" 2>/dev/null; then echo "CHILD_ALIVE"; else echo "CHILD_DEAD"; fi', - ].join('\n'); - const r = spawnSync('/bin/bash', ['-c', script], { encoding: 'utf-8', timeout: 20_000 }); - expect(r.stdout).toContain('CHILD_DEAD'); - expect(r.stdout).not.toContain('PGREP_PRESENT'); - } finally { - fs.rmSync(tmp, { recursive: true, force: true }); - } + const r = runKillTreeFallback(); + expect(r.status, `${r.stdout}\n${r.stderr}`).toBe(0); + expect(r.stdout).toContain('CHILD_DEAD'); + expect(r.stdout).not.toContain('PGREP_PRESENT'); + }, 30_000); + + test('_kill_tree skips vanished /proc entries and parses a child comm with spaces and parentheses', () => { + if (!fs.existsSync('/proc')) return; + const r = runKillTreeFallback(true); + expect(r.status, `${r.stdout}\n${r.stderr}`).toBe(0); + expect(r.stdout).toContain('CHILD_DEAD'); + expect(r.stdout).not.toContain('PGREP_PRESENT'); }, 30_000); test('install succeeds but the post-install probe fails: reason post-install-launch with the userns hint', () => {