From fbdc9fcd3bf83aadfa4ac358fdec2462b5bf8106 Mon Sep 17 00:00:00 2001 From: paul-0320 Date: Fri, 24 Jul 2026 03:38:15 +0900 Subject: [PATCH] fix(scripts): capture check/shard rc before watchdog teardown in no-timeout fallback (#2864) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On machines with neither gtimeout nor timeout on PATH, run-verify-parallel.sh and run-unit-parallel.sh fall back to a bg-pid + sleep-watchdog cap. Both read $? only after tearing the watchdog down (kill + wait on cap_pid), so the sentinel .exit files recorded the killed watchdog's status — 143 — instead of the check/shard's own exit code. Every run reported total failure (verify: pass=0 fail=31; unit: rc=143 per shard) while every per-check/shard log showed success. Capture rc immediately after `wait $pid` in both scripts, and reap the watchdog's sleep child (pkill -P, children-first — the same orphan quirk the heartbeat cleanup documents) so the fallback stops leaking one sleep per check/shard. Regression tests force the fallback branch hermetically on any host via a curated PATH with no timeout binaries: the verify dispatcher runs from a tempdir copy with a stubbed `bun`, pinning exit 0 + all-zero sentinels when checks pass and the check's own rc (not 143) when one fails; the unit wrapper runs real two-shard fixture passes, pinning rc=0 sentinels and a real failure's rc=1. Co-authored-by: YMYD <53603073+OJ-OnJourney@users.noreply.github.com> Co-authored-by: Claude Fable 5 --- scripts/run-unit-parallel.sh | 13 ++- scripts/run-verify-parallel.sh | 13 ++- test/scripts/run-unit-parallel.test.ts | 92 +++++++++++++++++++- test/scripts/run-verify-parallel.test.ts | 103 ++++++++++++++++++++++- 4 files changed, 217 insertions(+), 4 deletions(-) diff --git a/scripts/run-unit-parallel.sh b/scripts/run-unit-parallel.sh index 007d5c9e1..fb6deeade 100755 --- a/scripts/run-unit-parallel.sh +++ b/scripts/run-unit-parallel.sh @@ -133,6 +133,7 @@ for i in $(seq 1 "$N"); do env SHARD="$i/$N" \ bash scripts/run-unit-shard.sh --max-concurrency="$INTRA_CONC" \ > "$SHARD_LOG" 2>&1 + rc=$? else env SHARD="$i/$N" \ bash scripts/run-unit-shard.sh --max-concurrency="$INTRA_CONC" \ @@ -142,10 +143,20 @@ for i in $(seq 1 "$N"); do sleep 5 && kill -KILL "$pid" 2>/dev/null ) & cap_pid=$! wait "$pid" 2>/dev/null + # Capture the shard's exit code from ITS `wait`, before any watchdog + # teardown runs. The teardown commands below overwrite $? — the killed + # watchdog reports 143 — which used to get stamped into every shard's + # sentinel on machines with no gtimeout/timeout: every run "failed" + # with rc=143 summaries even when all tests passed. + rc=$? + # Reap the watchdog's `sleep` child too (pkill -P), then the watchdog. + # Killing only the subshell leaves the sleep orphaned until + # $SHARD_TIMEOUT elapses — same quirk the heartbeat cleanup below works + # around; CI's orphan-process sweep flags those. + pkill -P "$cap_pid" 2>/dev/null kill "$cap_pid" 2>/dev/null wait "$cap_pid" 2>/dev/null fi - rc=$? echo "$rc" > "$LOG_DIR/shard-$i.exit" [ "$rc" = "124" ] && echo "WEDGED" > "$LOG_DIR/shard-$i.wedged" ) & diff --git a/scripts/run-verify-parallel.sh b/scripts/run-verify-parallel.sh index f03c560b9..61392801f 100755 --- a/scripts/run-verify-parallel.sh +++ b/scripts/run-verify-parallel.sh @@ -126,6 +126,7 @@ for c in "${CHECKS[@]}"; do ( if [ -n "$TIMEOUT_BIN" ]; then "$TIMEOUT_BIN" "${TIMEOUT}s" bun run "$c" > "$LOG_FILE" 2>&1 + rc=$? else bun run "$c" > "$LOG_FILE" 2>&1 & pid=$! @@ -133,10 +134,20 @@ for c in "${CHECKS[@]}"; do sleep 5 && kill -KILL "$pid" 2>/dev/null ) & cap_pid=$! wait "$pid" 2>/dev/null + # Capture the check's exit code from ITS `wait`, before any watchdog + # teardown runs. The teardown commands below overwrite $? — the killed + # watchdog reports 143 — which used to get stamped into every sentinel + # on machines with no gtimeout/timeout: verify reported pass=0 + # fail= while every per-check log said OK. + rc=$? + # Reap the watchdog's `sleep` child too (pkill -P), then the watchdog. + # Killing only the subshell leaves the sleep orphaned until $TIMEOUT + # elapses — same quirk the heartbeat cleanup in run-unit-parallel.sh + # works around; CI's orphan-process sweep flags those. + pkill -P "$cap_pid" 2>/dev/null kill "$cap_pid" 2>/dev/null wait "$cap_pid" 2>/dev/null fi - rc=$? echo "$rc" > "$EXIT_FILE" ) & PIDS+=($!) diff --git a/test/scripts/run-unit-parallel.test.ts b/test/scripts/run-unit-parallel.test.ts index e19925a2f..4227ba655 100644 --- a/test/scripts/run-unit-parallel.test.ts +++ b/test/scripts/run-unit-parallel.test.ts @@ -22,7 +22,7 @@ import { describe, it, expect, beforeAll, afterAll } from 'bun:test'; import { execFileSync, spawnSync } from 'child_process'; -import { mkdtempSync, mkdirSync, writeFileSync, readFileSync, existsSync, rmSync, copyFileSync, chmodSync } from 'fs'; +import { mkdtempSync, mkdirSync, writeFileSync, readFileSync, existsSync, rmSync, copyFileSync, chmodSync, symlinkSync } from 'fs'; import { tmpdir } from 'os'; import { join, resolve } from 'path'; @@ -154,3 +154,93 @@ describe('failing-on-purpose', () => { expect(summary).toMatch(/shard 2\/2: pass=\d+ fail=\d+ skip=\d+ rc=\d+/); }); }); + +describe('run-unit-parallel.sh no-timeout-binary fallback (rc from shard wait, not watchdog teardown)', () => { + // Forces the no-gtimeout/no-timeout branch by running the wrapper under a + // curated PATH that has every tool the scripts call EXCEPT timeout + // binaries (real `bun` symlinked in), so the fallback executes even on + // hosts with coreutils installed. + // + // Regression pinned here: the shard's sentinel .exit file must record the + // exit code read right after `wait $pid` (the shard's own rc). The + // watchdog subshell is killed with SIGTERM and reports 143; reading `$?` + // after that teardown stamped rc=143 into every shard's sentinel — the + // wrapper exited non-zero with rc=143 summaries even when every test + // passed. + let FROOT: string; + let FENV: Record; + + beforeAll(() => { + FROOT = mkdtempSync(join(tmpdir(), 'gbrain-parallel-fallback-')); + mkdirSync(join(FROOT, 'scripts'), { recursive: true }); + mkdirSync(join(FROOT, 'test'), { recursive: true }); + for (const s of ['run-unit-parallel.sh', 'run-unit-shard.sh', 'run-serial-tests.sh']) { + copyFileSync(resolve(REPO_ROOT, 'scripts', s), join(FROOT, 'scripts', s)); + chmodSync(join(FROOT, 'scripts', s), 0o755); + } + const passing = `import { describe, it, expect } from 'bun:test'; +describe('passing', () => { + it('arithmetic works', () => { expect(1 + 1).toBe(2); }); +});`; + writeFileSync(join(FROOT, 'test', 'a-pass.test.ts'), passing); + writeFileSync(join(FROOT, 'test', 'b-pass.test.ts'), passing); + + const bin = join(FROOT, 'bin'); + mkdirSync(bin); + for (const tool of ['bash', 'sh', 'env', 'dirname', 'basename', 'mktemp', 'date', 'sleep', 'cat', 'tail', 'head', 'rm', 'mkdir', 'pkill', 'grep', 'sed', 'awk', 'wc', 'tr', 'seq', 'find', 'sort', 'bun']) { + const p = Bun.which(tool); + if (p) symlinkSync(p, join(bin, tool)); + } + FENV = { + PATH: bin, + HOME: process.env.HOME ?? FROOT, + TMPDIR: process.env.TMPDIR ?? '/tmp', + GBRAIN_TEST_SHARD_TIMEOUT: '300', + }; + }); + + afterAll(() => { + if (FROOT) rmSync(FROOT, { recursive: true, force: true }); + }); + + function runFallbackWrapper(): { code: number; stdout: string; stderr: string } { + const result = spawnSync( + 'bash', + [join(FROOT, 'scripts', 'run-unit-parallel.sh'), '--shards', '2'], + { cwd: FROOT, encoding: 'utf-8', env: FENV }, + ); + return { + code: result.status ?? -1, + stdout: result.stdout || '', + stderr: result.stderr || '', + }; + } + + it('exits zero with rc=0 shard sentinels when all shards pass', () => { + const r = runFallbackWrapper(); + const summary = readFileSync(join(FROOT, '.context', 'test-summary.txt'), 'utf-8'); + expect(summary).toMatch(/shard 1\/2: pass=\d+ fail=0 skip=0 rc=0/); + expect(summary).toMatch(/shard 2\/2: pass=\d+ fail=0 skip=0 rc=0/); + expect(summary).not.toContain('rc=143'); + expect(r.code).toBe(0); + }); + + it('propagates a failing shard rc as the test runner rc (1), not the watchdog 143', () => { + const failing = `import { describe, it, expect } from 'bun:test'; +describe('failing-on-purpose', () => { + it('expects 1 to equal 2', () => { expect(1).toBe(2); }); +});`; + writeFileSync(join(FROOT, 'test', 'z-fail.test.ts'), failing); + try { + const r = runFallbackWrapper(); + expect(r.code).not.toBe(0); + const summary = readFileSync(join(FROOT, '.context', 'test-summary.txt'), 'utf-8'); + expect(summary).toMatch(/shard \d\/2: pass=\d+ fail=1 skip=0 rc=1/); + expect(summary).not.toContain('rc=143'); + const failureLog = readFileSync(join(FROOT, '.context', 'test-failures.log'), 'utf-8'); + expect(failureLog).toContain('failing-on-purpose'); + } finally { + rmSync(join(FROOT, 'test', 'z-fail.test.ts'), { force: true }); + } + }); +}); diff --git a/test/scripts/run-verify-parallel.test.ts b/test/scripts/run-verify-parallel.test.ts index 949fc3d83..8b8e64ac3 100644 --- a/test/scripts/run-verify-parallel.test.ts +++ b/test/scripts/run-verify-parallel.test.ts @@ -15,7 +15,16 @@ import { describe, expect, it } from "bun:test"; import { spawnSync } from "node:child_process"; -import { writeFileSync, mkdtempSync, rmSync } from "node:fs"; +import { + copyFileSync, + mkdirSync, + mkdtempSync, + readFileSync, + readdirSync, + rmSync, + symlinkSync, + writeFileSync, +} from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; @@ -173,3 +182,95 @@ exit 0 } }); }); + +describe("run-verify-parallel.sh — no-timeout-binary fallback rc capture (regression)", () => { + // macOS ships no `timeout`; without brew coreutils (`gtimeout`) — stock + // machines, minimal containers, restricted/sandboxed PATHs — the dispatcher + // degrades to the bg-pid + sleep-watchdog branch. + // + // Regression pinned here: each check's sentinel .exit file must record the + // exit code of the CHECK (read right after `wait $pid`), not of the + // watchdog teardown. The watchdog subshell is killed with SIGTERM and so + // reports 143; reading `$?` after the teardown stamped 143 into every + // sentinel — verify reported pass=0 fail= while every per-check log + // said OK. + // + // Hermetic on any host: the script runs from a tempdir copy with `bun` + // stubbed (checks complete instantly, no repo needed) and PATH set to a + // curated symlink dir containing everything the script calls EXCEPT + // gtimeout/timeout — forcing the fallback branch even where coreutils is + // installed. + + function makeFallbackHarness(): { root: string; env: Record } { + const root = mkdtempSync(join(tmpdir(), "verify-fallback-")); + mkdirSync(join(root, "scripts"), { recursive: true }); + copyFileSync(SCRIPT, join(root, "scripts", "run-verify-parallel.sh")); + + const bin = join(root, "bin"); + mkdirSync(bin); + // Everything the dispatcher and its subshells invoke, minus timeout bins. + for (const tool of ["bash", "sh", "env", "dirname", "mktemp", "date", "sleep", "cat", "tail", "head", "rm", "mkdir", "pkill", "grep", "sed", "awk"]) { + const p = Bun.which(tool); + if (p) symlinkSync(p, join(bin, tool)); + } + // `bun run ` stand-in: instant, prints OK, exits 7 for the check + // named in $STUB_FAIL_CHECK (if any). + writeFileSync( + join(bin, "bun"), + `#!/usr/bin/env bash +name="\${2:-}" +echo "stub check OK: $name" +if [ -n "\${STUB_FAIL_CHECK:-}" ] && [ "$name" = "\${STUB_FAIL_CHECK}" ]; then + echo "stub check failing: $name" >&2 + exit 7 +fi +exit 0 +`, + { mode: 0o755 }, + ); + + return { + root, + env: { + PATH: bin, + HOME: process.env.HOME ?? root, + TMPDIR: process.env.TMPDIR ?? "/tmp", + GBRAIN_VERIFY_TIMEOUT: "30", + GBRAIN_VERIFY_LOG_DIR: join(root, "logs"), + }, + }; + } + + it("all checks passing → exit 0, every sentinel records 0 (not the watchdog's 143)", () => { + const { root, env } = makeFallbackHarness(); + try { + const r = spawnSync("bash", [join(root, "scripts", "run-verify-parallel.sh")], { encoding: "utf8", env }); + expect(r.stderr).toMatch(/pass=\d+ fail=0/); + expect(r.status).toBe(0); + const exits = readdirSync(join(root, "logs")).filter((f) => f.endsWith(".exit")); + expect(exits.length).toBeGreaterThan(10); + for (const f of exits) { + expect(readFileSync(join(root, "logs", f), "utf8").trim()).toBe("0"); + } + } finally { + rmSync(root, { recursive: true, force: true }); + } + }); + + it("one check failing → exit 1, sentinel records the check's own rc (7), not 143", () => { + const { root, env } = makeFallbackHarness(); + try { + const r = spawnSync("bash", [join(root, "scripts", "run-verify-parallel.sh")], { + encoding: "utf8", + env: { ...env, STUB_FAIL_CHECK: "check:jsonb" }, + }); + expect(r.status).toBe(1); + expect(r.stderr).toContain("--- check:jsonb (rc=7)"); + expect(r.stderr).toContain("stub check failing: check:jsonb"); + expect(r.stderr).toMatch(/fail=1\b/); + expect(readFileSync(join(root, "logs", "check_jsonb.exit"), "utf8").trim()).toBe("7"); + } finally { + rmSync(root, { recursive: true, force: true }); + } + }); +});