fix(scripts): await the pack:verify web child's exit before removing its work dir (#1895)
* fix(scripts): await the pack:verify web child's exit before removing its work dir `pack-and-verify.mjs` had the same kill-then-remove shape that made `smoke:tui` flake with ENOTEMPTY in #1801: `verifyWeb()`'s `finally { stop() }` only *delivered* SIGTERM, then the main flow `rmSync`'d the work dir immediately, and `fail()` did the same back to back. Applies the #1814 treatment, factored into a shared `scripts/lib/child-cleanup.mjs` so the two scripts can't drift: - `stopChild()` — SIGTERM, await the child's `exit`/`close`, escalate to SIGKILL after a grace period, and ultimately proceed with a warning so a wedged child can never hang the script. `verifyWeb()`'s `stop()` now goes through it and is awaited, so the child is gone before the caller removes `work`. - `removeSafe()` — `rmSync` that warns instead of throwing. Used on both success-path removals (a leftover temp dir must never fail a run that passed) and in `fail()`, where an ENOTEMPTY could otherwise bury the real diagnostic under an rmSync stack and skip the "tarball retained at ..." hint. `fail()` stays synchronous — it is called from sync contexts and ends in `process.exit()` — so it best-effort signals the child; `removeSafe()` is what makes that safe. `smoke-tui.mjs` adopts `removeSafe()` for its `cleanup()`. Its `done()` keeps its bespoke wait (documented inline): that one must keep waiting for `close` *after* `exit` on a shorter re-armed deadline so the quoted output is complete, which `stopChild()`'s first-of-exit/close contract deliberately does not do. Adds `scripts/lib/child-cleanup.test.mjs` (10 cases, `node --test` via `npm run test:scripts`) covering the SIGTERM/SIGKILL escalation, the `close`-only spawn-failure path, the never-hang give-up, late events after giving up, and that `removeSafe` warns rather than throws. `npm run ci` passes, and `npm run pack:verify` (not in the gate — needs network) was run end to end and passes with no cleanup warnings. Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(scripts): normalize stopChild's graceMs and correct removeSafe's JSDoc Copilot review round 1 on #1895. - `stopChild()` trusted `graceMs` as given, but call sites build it as `Number(process.env.X ?? 5000)` — a typo yields `NaN`, which `setTimeout` treats as `0`. That would fire SIGTERM and SIGKILL in the same tick and print "did not exit within NaNms", collapsing the whole escalation silently. New exported `normalizeGraceMs()` falls back to `DEFAULT_EXIT_GRACE_MS` for anything non-finite or non-positive, with two tests: the table of bad values, and an end-to-end one asserting a `NaN` grace period still leaves only SIGTERM sent a turn later. - `removeSafe()`'s `@returns` said "true if the path was removed", which is inaccurate for a missing path (`force: true` makes it a no-op success). Reworded to say what it actually reports. `npm run ci` passes. Claude-Session: https://claude.ai/code/session_01YAt8rqxysNbhYWLhoRm3fU Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Cliff Hall
GitHub
parent
694d013f26
commit
8801d0f8bc
1 changed files with 370 additions and 23 deletions
|
|
@ -39,10 +39,11 @@
|
|||
*/
|
||||
|
||||
import { spawn, spawnSync } from "node:child_process";
|
||||
import { mkdtempSync, rmSync, existsSync, writeFileSync } from "node:fs";
|
||||
import { mkdtempSync, existsSync, writeFileSync } from "node:fs";
|
||||
import { tmpdir } from "node:os";
|
||||
import { join, resolve } from "node:path";
|
||||
import { setTimeout as delay } from "node:timers/promises";
|
||||
import { hasExited, removeSafe, stopChild } from "./lib/child-cleanup.mjs";
|
||||
|
||||
const repoRoot = resolve(import.meta.dirname, "..");
|
||||
const testServer = join(
|
||||
|
|
@ -69,13 +70,22 @@ let workDir = null;
|
|||
// own `finally { stop() }` is skipped when fail() calls process.exit()).
|
||||
let webChild = null;
|
||||
|
||||
const LABEL = "pack:verify";
|
||||
|
||||
function fail(message) {
|
||||
console.error(`\npack:verify FAILED — ${message}`);
|
||||
if (webChild && webChild.exitCode === null) {
|
||||
// fail() is the single failure-exit point and is called from synchronous
|
||||
// contexts (top-level statements, loop bodies), so it cannot await the
|
||||
// child's exit the way the success path does — it best-effort signals and
|
||||
// moves on. That is why removeSafe() matters most here: this path is already
|
||||
// exiting 1, so an ENOTEMPTY could only bury the real diagnostic under an
|
||||
// rmSync stack (and skip the "tarball retained" hint below), never turn a
|
||||
// green run red.
|
||||
if (webChild && !hasExited(webChild)) {
|
||||
webChild.kill("SIGTERM");
|
||||
}
|
||||
if (workDir) {
|
||||
rmSync(workDir, { recursive: true, force: true });
|
||||
removeSafe(workDir, { label: LABEL });
|
||||
// `tarball` is initialized before `workDir` is ever set, so this is safe.
|
||||
console.error(
|
||||
`pack:verify — tarball retained for inspection at ${tarball}`,
|
||||
|
|
@ -327,8 +337,13 @@ try {
|
|||
// Success: clean up both the work dir and the tarball. (This is reached only
|
||||
// on success — every failure path goes through fail() → process.exit(), which
|
||||
// does its own cleanup above and never returns here.)
|
||||
rmSync(work, { recursive: true, force: true });
|
||||
rmSync(tarball, { force: true });
|
||||
//
|
||||
// verifyWeb() has already awaited the `--web` child's exit before returning
|
||||
// (#1826), so nothing of ours is still running inside `work` here. Removal is
|
||||
// still routed through removeSafe(): a leftover temp dir must never fail a run
|
||||
// that actually passed.
|
||||
removeSafe(work, { label: LABEL });
|
||||
removeSafe(tarball, { label: LABEL });
|
||||
|
||||
console.log(
|
||||
"\npack:verify OK — published tarball installs clean and the real bin drives " +
|
||||
|
|
@ -374,10 +389,21 @@ async function verifyWeb(bin, cwd) {
|
|||
exited = true;
|
||||
exitCode = code;
|
||||
});
|
||||
const stop = () => {
|
||||
if (!exited) child.kill("SIGTERM");
|
||||
webChild = null;
|
||||
};
|
||||
|
||||
// Signal the server AND wait for it to be gone before returning: the caller
|
||||
// removes `work` (the installed package the child is running out of, and its
|
||||
// cwd) as its very next statement, and `child.kill()` only delivers the
|
||||
// signal (#1826 — the same kill-then-remove race #1801 hit in smoke:tui).
|
||||
// Escalates to SIGKILL and ultimately proceeds with a warning, so a wedged
|
||||
// server can never hang the verify.
|
||||
const stop = () =>
|
||||
stopChild(child, {
|
||||
label: LABEL,
|
||||
what: "`--web` server",
|
||||
graceMs: Number(process.env.PACK_VERIFY_EXIT_GRACE_MS ?? 5000),
|
||||
}).finally(() => {
|
||||
webChild = null;
|
||||
});
|
||||
|
||||
try {
|
||||
let res = null;
|
||||
|
|
@ -409,6 +435,6 @@ async function verifyWeb(bin, cwd) {
|
|||
fail("`--web` served HTML is missing the injected auth-token value");
|
||||
}
|
||||
} finally {
|
||||
stop();
|
||||
await stop();
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue