From a49450ef5ebbd6a2850c916967d4c5e180fa9de0 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Sat, 13 Jun 2026 15:21:00 -0700 Subject: [PATCH] Address PR review feedback (#1669) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - watchdog: escalate forwarded SIGINT/SIGTERM/SIGHUP to SIGKILL after grace so external cancellation can't hang for the full budget (coderabbit major) - watchdog: route onProcExit through signalGroup for injection consistency (greptile) - watchdog: add cwd option; test-changed passes rootDir so pnpm runs from repo root regardless of invocation cwd (coderabbit major — preserved original run() cwd) - dashboard runner: validate/clamp FUSION_RUN_VITEST_* env so a malformed value can't NaN-disable the watchdog (coderabbit) - tests: verify exit-listener cleanup, forwarded-signal escalation, cwd passthrough - plan doc: per-class-ceiling fallback wording (not median); label Output Structure fence Co-Authored-By: Claude Opus 4.8 --- ...6-13-001-fix-test-timeout-failures-plan.md | 4 +- .../scripts/run-vitest-with-heap.mjs | 17 +++++- .../__tests__/run-vitest-watchdog.test.mjs | 52 ++++++++++++++++++- scripts/lib/run-vitest-watchdog.mjs | 34 +++++++++--- scripts/test-changed.mjs | 3 ++ 5 files changed, 97 insertions(+), 13 deletions(-) diff --git a/docs/plans/2026-06-13-001-fix-test-timeout-failures-plan.md b/docs/plans/2026-06-13-001-fix-test-timeout-failures-plan.md index dfa9d2c7bf..6322d9fd0b 100644 --- a/docs/plans/2026-06-13-001-fix-test-timeout-failures-plan.md +++ b/docs/plans/2026-06-13-001-fix-test-timeout-failures-plan.md @@ -128,7 +128,7 @@ flowchart TD New/changed shared infrastructure (illustrative — per-unit `Files` lists are authoritative): -``` +```text scripts/ lib/ run-vitest-watchdog.mjs # NEW (U1) — shared bounded-invocation runner + process-group killer; @@ -170,7 +170,7 @@ scripts/lib/test-quarantine.json # MODIFIED (U3) — rescue/delet - `.github/workflows/full-suite.yml` (modify) — add `timeout-minutes` to `test-shards`, `test-slow`, `test-inventory-guard`. - `scripts/__tests__/run-vitest-watchdog.test.mjs` (new). -**Approach:** Extract the dashboard killer's process-group lifecycle into the shared async helper, parameterized by command, env, heap flag, and budget. Budget = `max(perClassFloor, min(perClassCeiling, expectedDurationMs × multiplier))` per KTD-2 — the per-class floor/ceiling (shard / changed-file / dashboard-lane) are the safety net; the timings term (aggregated across all packages in a multi-package `plain` command, median fallback when absent, multiplier 3-4×) only tightens within the band, and only when the snapshot is fresh. **Refresh `test-timings.json` before deriving budgets.** CI `timeout-minutes` must exceed the worst-case L2 ceiling so L2 always fires first; document the ordering in a comment. Forwards external signals; cleans up on exit/SIGINT/SIGTERM like the existing runners. Note the two runners import each other and are imported by tests — verify the async conversion doesn't break any synchronous-import caller. +**Approach:** Extract the dashboard killer's process-group lifecycle into the shared async helper, parameterized by command, env, heap flag, and budget. Budget = `max(perClassFloor, min(perClassCeiling, expectedDurationMs × multiplier))` per KTD-2 — the per-class floor/ceiling (shard / changed-file / dashboard-lane) are the safety net; the timings term (aggregated across all packages in a multi-package `plain` command, multiplier 3-4×) only tightens within the band, and only when the snapshot is fresh; when timings are absent or stale, `deriveBudgetMs` falls back to the per-class **ceiling** (never a median). **Refresh `test-timings.json` before deriving budgets.** CI `timeout-minutes` must exceed the worst-case L2 ceiling so L2 always fires first; document the ordering in a comment. Forwards external signals; cleans up on exit/SIGINT/SIGTERM like the existing runners. Note the two runners import each other and are imported by tests — verify the async conversion doesn't break any synchronous-import caller. **Execution note:** Start with a failing test for the watchdog contract (spawns a deliberately-hanging child, asserts `SIGTERM`-then-`SIGKILL` and exit 124 within budget) before extracting the helper. diff --git a/packages/dashboard/scripts/run-vitest-with-heap.mjs b/packages/dashboard/scripts/run-vitest-with-heap.mjs index be48b18af6..0b8fd929fb 100644 --- a/packages/dashboard/scripts/run-vitest-with-heap.mjs +++ b/packages/dashboard/scripts/run-vitest-with-heap.mjs @@ -18,8 +18,21 @@ if (vitestArgs.length === 0) { const nodeOptions = [`--max-old-space-size=${heapMb}`, process.env.NODE_OPTIONS || ""] .join(" ") .trim(); -const timeoutMs = Number.parseInt(process.env.FUSION_RUN_VITEST_TIMEOUT_MS || "900000", 10); -const graceMs = Number.parseInt(process.env.FUSION_RUN_VITEST_KILL_GRACE_MS || "5000", 10); +// Clamp to the default on a missing/malformed value. A bad env value must never +// produce NaN — the watchdog only arms when budgetMs is finite and > 0, so a +// NaN here would silently disable the killer and bring back the very hang this +// wrapper exists to prevent. +function positiveIntEnv(name, fallback) { + const raw = process.env[name]; + if (raw == null || raw === "") return fallback; + const parsed = Number.parseInt(raw, 10); + if (Number.isInteger(parsed) && parsed > 0) return parsed; + console.error(`[dashboard-vitest] ignoring invalid ${name}=${JSON.stringify(raw)}; using ${fallback}`); + return fallback; +} + +const timeoutMs = positiveIntEnv("FUSION_RUN_VITEST_TIMEOUT_MS", 900000); +const graceMs = positiveIntEnv("FUSION_RUN_VITEST_KILL_GRACE_MS", 5000); function resolveSpawnCommand() { const override = process.env.FUSION_RUN_VITEST_SPAWN_OVERRIDE; diff --git a/scripts/__tests__/run-vitest-watchdog.test.mjs b/scripts/__tests__/run-vitest-watchdog.test.mjs index eaefc835f8..fa4c8411a6 100644 --- a/scripts/__tests__/run-vitest-watchdog.test.mjs +++ b/scripts/__tests__/run-vitest-watchdog.test.mjs @@ -166,7 +166,8 @@ test("runWithWatchdog: child error rejects", async () => { }); test("runWithWatchdog: removes its process listeners after settling", async () => { - const before = process.listenerCount("SIGTERM"); + const beforeTerm = process.listenerCount("SIGTERM"); + const beforeExit = process.listenerCount("exit"); const child = makeFakeChild(); const p = runWithWatchdog({ command: "fake", @@ -179,5 +180,52 @@ test("runWithWatchdog: removes its process listeners after settling", async () = }); child.emit("close", 0, null); await p; - assert.equal(process.listenerCount("SIGTERM"), before); + assert.equal(process.listenerCount("SIGTERM"), beforeTerm); + assert.equal(process.listenerCount("exit"), beforeExit); +}); + +test("runWithWatchdog: forwarded signal escalates to SIGKILL after grace", async () => { + const child = makeFakeChild(); + const killed = []; + const p = runWithWatchdog({ + command: "pnpm", + args: [], + budgetMs: 10_000, + graceMs: 15, + heartbeatMs: 1000, + label: "cancel", + log: () => {}, + spawn: fakeSpawn(child), + killGroup: (sig) => { + killed.push(sig); + // The child ignores SIGHUP; only SIGKILL takes it down. + if (sig === "SIGKILL") child.emit("close", null, "SIGKILL"); + }, + }); + // Simulate external cancellation (Ctrl-C / CI cancel) reaching the wrapper. + process.emit("SIGHUP"); + await new Promise((resolve) => setTimeout(resolve, 50)); + await p; + assert.deepEqual(killed, ["SIGHUP", "SIGKILL"]); +}); + +test("runWithWatchdog: passes cwd through to spawn when provided", async () => { + let capturedOpts = null; + const child = makeFakeChild(); + const p = runWithWatchdog({ + command: "pnpm", + args: ["test"], + cwd: "/tmp/repo-root", + budgetMs: 10_000, + label: "cwd", + log: () => {}, + spawn: (_cmd, _args, opts) => { + capturedOpts = opts; + return child; + }, + killGroup: () => {}, + }); + child.emit("close", 0, null); + await p; + assert.equal(capturedOpts.cwd, "/tmp/repo-root"); }); diff --git a/scripts/lib/run-vitest-watchdog.mjs b/scripts/lib/run-vitest-watchdog.mjs index fbd4b54fae..b047a70b07 100644 --- a/scripts/lib/run-vitest-watchdog.mjs +++ b/scripts/lib/run-vitest-watchdog.mjs @@ -139,6 +139,8 @@ export function captureHangDiagnostics({ label, command, args, budgetMs, started * @param {string} [opts.label] * @param {(msg: string) => void} [opts.log] * @param {object} opts.spawn injected spawn (node:child_process spawn); required for testability + * @param {string} [opts.cwd] working directory for the spawned child (preserves callers that + * ran the test command from a fixed root, e.g. test-changed.mjs's rootDir) * @param {() => number} [opts.now] injected clock (defaults to Date.now) * @param {(signal: string) => void} [opts.killGroup] injected group-signaller * (defaults to a process-group `process.kill(-pid)` with child.kill fallback); @@ -148,6 +150,7 @@ export function runWithWatchdog({ command, args, env = process.env, + cwd = null, budgetMs, graceMs = DEFAULT_GRACE_MS, heartbeatMs = DEFAULT_HEARTBEAT_MS, @@ -171,7 +174,12 @@ export function runWithWatchdog({ // process-supervisor-allowlist: foreground wrapper signals the whole vitest // process group on death/timeout; not a background daemon. - const child = spawn(command, args, { detached: true, stdio: "inherit", env }); + const child = spawn(command, args, { + detached: true, + stdio: "inherit", + env, + ...(cwd ? { cwd } : {}), + }); const heartbeat = setInterval(() => { lastHeartbeatAt = now(); @@ -195,6 +203,20 @@ export function runWithWatchdog({ } const signalGroup = typeof killGroup === "function" ? killGroup : defaultSignalGroup; + // Arm the SIGTERM→SIGKILL grace ladder once. Used by BOTH the timeout path + // and external-cancellation forwarding so a child that ignores SIGTERM can't + // keep the wrapper pending until the full budget (the original handlers + // suppressed Node's default exit behavior, so Ctrl-C / CI cancellation could + // otherwise hang for the whole per-command ceiling). + function armForceKill(triggerSignal) { + if (forceKillTimer) return; + forceKillTimer = setTimeout(() => { + log(`[watchdog] grace expired after ${triggerSignal}; SIGKILL: ${label}`); + signalGroup("SIGKILL"); + }, Math.max(1, graceMs)); + forceKillTimer.unref?.(); + } + const watchdog = Number.isFinite(budgetMs) && budgetMs > 0 ? setTimeout(() => { @@ -210,11 +232,7 @@ export function runWithWatchdog({ }); log(diagnostics); signalGroup("SIGTERM"); - forceKillTimer = setTimeout(() => { - log(`[watchdog] grace expired; SIGKILL: ${label}`); - signalGroup("SIGKILL"); - }, Math.max(1, graceMs)); - forceKillTimer.unref?.(); + armForceKill("timeout"); }, budgetMs) : null; watchdog?.unref?.(); @@ -225,6 +243,7 @@ export function runWithWatchdog({ const handler = () => { log(`[watchdog] received ${sig}; forwarding to group: ${label}`); signalGroup(sig); + armForceKill(sig); }; signalHandlers.set(sig, handler); process.on(sig, handler); @@ -232,8 +251,9 @@ export function runWithWatchdog({ function onProcExit() { // Best-effort: don't leave an orphaned group if the wrapper itself dies. + // Route through signalGroup so the injection contract holds everywhere. try { - process.kill(-child.pid, "SIGTERM"); + signalGroup("SIGTERM"); } catch { /* group already gone */ } diff --git a/scripts/test-changed.mjs b/scripts/test-changed.mjs index 3797c193b6..0809fad862 100644 --- a/scripts/test-changed.mjs +++ b/scripts/test-changed.mjs @@ -213,6 +213,9 @@ async function runWatchedTest(command, commandArgs, { env, budgetMs, label } = { command, args: commandArgs, env: env ?? process.env, + // Preserve the original `run`'s fixed working directory; pnpm must execute + // from the repo root regardless of where test-changed was invoked. + cwd: rootDir, budgetMs, label: label ?? `${command} ${commandArgs.join(" ")}`, log: console.error,