fix(pack-verify): actually clean up on failure (fail() bypasses finally)

Re-review caught that the previous fix was inert: fail() calls process.exit(1),
which skips finally blocks — so `cleanupTarball = false` never took effect and,
worse, the temp work dir (a full node_modules from the real install) leaked on
every failed run.

Move cleanup into fail() itself — the single failure-exit point, called from
everywhere including deep inside the run. On failure it removes the heavy work
dir and leaves the packed tarball in place for post-mortem inspection; the
success path removes both inline before the OK log. Drop the dead
`cleanupTarball` flag entirely.

Verified both paths: success removes work dir + tarball (exit 0); a forced
failure removes the work dir, retains the tarball, and exits 1.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw
This commit is contained in:
cliffhall 2026-07-10 18:32:03 -04:00
commit 3dead1f83d

View file

@ -51,8 +51,21 @@ const testServer = join(repoRoot, "test-servers", "build", "test-server-stdio.js
// literal because this plain .mjs script can't import the TS source. // literal because this plain .mjs script can't import the TS source.
const TOKEN_GLOBAL = "__INSPECTOR_API_TOKEN__"; const TOKEN_GLOBAL = "__INSPECTOR_API_TOKEN__";
// Set once the throwaway consumer dir exists. `fail()` is the single
// failure-exit point (called from everywhere, including deep inside the run),
// and it calls process.exit(), which skips any `finally` — so cleanup has to
// happen here, not in a trailing `finally`. On failure we remove the heavy work
// dir (a full node_modules from the real install) but leave the packed tarball
// in place for post-mortem inspection.
let workDir = null;
function fail(message) { function fail(message) {
console.error(`\npack:verify FAILED — ${message}`); console.error(`\npack:verify FAILED — ${message}`);
if (workDir) {
rmSync(workDir, { recursive: true, force: true });
// `tarball` is initialized before `workDir` is ever set, so this is safe.
console.error(`pack:verify — tarball retained for inspection at ${tarball}`);
}
process.exit(1); process.exit(1);
} }
@ -151,9 +164,7 @@ console.log(
// runs the package's postinstall). // runs the package's postinstall).
// --------------------------------------------------------------------------- // ---------------------------------------------------------------------------
const work = mkdtempSync(join(tmpdir(), "pack-verify-")); const work = mkdtempSync(join(tmpdir(), "pack-verify-"));
// Remove the packed tarball on success; retain it on failure so a packaging workDir = work; // from now on, fail() cleans this up
// problem can be inspected (unpack it, diff the file list).
let cleanupTarball = true;
try { try {
ensureTestServer(); ensureTestServer();
step(`installing the tarball into a clean consumer at ${work} (pulls runtime deps)...`); step(`installing the tarball into a clean consumer at ${work} (pulls runtime deps)...`);
@ -250,18 +261,20 @@ try {
// it) and inject the auth token. Run non-blocking and poll `/`. // it) and inject the auth token. Run non-blocking and poll `/`.
await verifyWeb(bin, work); await verifyWeb(bin, work);
// 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 });
console.log( console.log(
"\npack:verify OK — published tarball installs clean and the real bin drives " + "\npack:verify OK — published tarball installs clean and the real bin drives " +
"web (prod / served dist), cli (stdio tools/list), and tui (help) end to end.", "web (prod / served dist), cli (stdio tools/list), and tui (help) end to end.",
); );
} catch (err) { } catch (err) {
// Keep the tarball around for post-mortem inspection of the packaging failure. // Unexpected throw (not via fail()) — route through fail() for consistent
cleanupTarball = false; // cleanup + exit.
console.error(`pack:verify — tarball retained for inspection at ${tarball}`);
fail(err instanceof Error ? err.message : String(err)); fail(err instanceof Error ? err.message : String(err));
} finally {
rmSync(work, { recursive: true, force: true });
if (cleanupTarball) rmSync(tarball, { force: true });
} }
/** /**