* 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>
267 lines
9.9 KiB
JavaScript
267 lines
9.9 KiB
JavaScript
#!/usr/bin/env node
|
|
/**
|
|
* Boot smoke test for the prod TUI launcher path (#1347).
|
|
*
|
|
* `npm run smoke:launcher` only checks `--tui --help`; it never actually starts
|
|
* the Ink app, so the launcher → TUI → core path and `--catalog` loading went
|
|
* unverified by the launcher smokes. This script launches the built launcher in
|
|
* `--tui` mode against a temp `--catalog`, waits for the app to render its first
|
|
* frame (the "MCP Servers" panel) within a timeout, then sends SIGTERM and
|
|
* exits.
|
|
*
|
|
* It asserts the TUI *boots and renders* without crashing — not full
|
|
* interaction (driving an Ink UI deterministically in CI is flaky, so this is
|
|
* intentionally a shallow render check). Exits non-zero on a crash-before-render
|
|
* or a render timeout.
|
|
*
|
|
* Expects `clients/launcher/build` and `clients/tui/build` to be built first
|
|
* (the validate / CI ordering guarantees this). The bundled stdio test server
|
|
* (`test-servers/build`) is built on demand here if missing.
|
|
*/
|
|
|
|
import { spawn, spawnSync } from "node:child_process";
|
|
import { mkdtempSync, existsSync, writeFileSync } from "node:fs";
|
|
import { tmpdir } from "node:os";
|
|
import { join, resolve } from "node:path";
|
|
import { removeSafe } from "./lib/child-cleanup.mjs";
|
|
|
|
const repoRoot = resolve(import.meta.dirname, "..");
|
|
const launcher = join(repoRoot, "clients", "launcher", "build", "index.js");
|
|
const testServer = join(
|
|
repoRoot,
|
|
"test-servers",
|
|
"build",
|
|
"test-server-stdio.js",
|
|
);
|
|
const RENDER_MARKER = "MCP Servers";
|
|
const TIMEOUT_MS = Number(process.env.SMOKE_TUI_TIMEOUT_MS ?? 15000);
|
|
|
|
function fail(message) {
|
|
console.error(`smoke:tui FAILED — ${message}`);
|
|
process.exit(1);
|
|
}
|
|
|
|
function ensureTestServer() {
|
|
if (existsSync(testServer)) return;
|
|
console.log("smoke:tui — building test-servers (missing build output)...");
|
|
const r = spawnSync("npx", ["tsc", "-p", "test-servers", "--noCheck"], {
|
|
cwd: repoRoot,
|
|
stdio: "inherit",
|
|
});
|
|
if (r.status !== 0 || !existsSync(testServer)) {
|
|
fail(
|
|
"could not build the stdio test server (test-servers/build/test-server-stdio.js). " +
|
|
"Run `npm run test-servers:build` from clients/cli.",
|
|
);
|
|
}
|
|
}
|
|
|
|
// The Ink TUI requires a real TTY for raw-mode keyboard input. Headless CI has
|
|
// none, so the app throws "Raw mode is not supported" on mount and exits before
|
|
// it can render its first frame — making this boot/render check inherently a
|
|
// LOCAL-only smoke (its own design notes call driving Ink in CI flaky). Skip it
|
|
// under CI rather than fail spuriously: the TUI is still built and unit-tested
|
|
// there; only this terminal-dependent render check is local-only.
|
|
if (process.env.CI) {
|
|
console.log(
|
|
"smoke:tui SKIPPED — Ink needs a real TTY (raw mode), unavailable in CI; run it locally",
|
|
);
|
|
process.exit(0);
|
|
}
|
|
|
|
if (!existsSync(launcher)) {
|
|
fail(`launcher build not found at ${launcher} — run \`npm run build\` first`);
|
|
}
|
|
ensureTestServer();
|
|
|
|
const work = mkdtempSync(join(tmpdir(), "smoke-tui-"));
|
|
const catalogPath = join(work, "catalog.json");
|
|
writeFileSync(
|
|
catalogPath,
|
|
JSON.stringify({
|
|
mcpServers: {
|
|
test: { type: "stdio", command: process.execPath, args: [testServer] },
|
|
},
|
|
}),
|
|
);
|
|
|
|
const child = spawn(
|
|
process.execPath,
|
|
[launcher, "--tui", "--catalog", catalogPath],
|
|
{
|
|
cwd: repoRoot,
|
|
// Redirect HOME so the TUI's storage never touches the real ~/.mcp-inspector.
|
|
// Pin MCP_OAUTH_CALLBACK_URL="" (empty reads as unset) so an ambient
|
|
// non-loopback value can't crash the TUI before render via the loopback
|
|
// callback guard — same class smoke-cli.mjs's SMOKE_BASE_ENV neutralizes.
|
|
env: {
|
|
...process.env,
|
|
MCP_OAUTH_CALLBACK_URL: "",
|
|
HOME: work,
|
|
USERPROFILE: work,
|
|
},
|
|
stdio: ["ignore", "pipe", "pipe"],
|
|
},
|
|
);
|
|
|
|
let output = "";
|
|
let settled = false;
|
|
let childExited = false;
|
|
let childClosed = false;
|
|
|
|
// How long to let the TUI wind down after SIGTERM before escalating to SIGKILL,
|
|
// and then before giving up on the close event entirely.
|
|
const EXIT_GRACE_MS = Number(process.env.SMOKE_TUI_EXIT_GRACE_MS ?? 5000);
|
|
// Once the child itself is gone, only the pipe drain remains, so the wait drops
|
|
// to this. `close` is bounded by whoever holds the stdio pipes, which can
|
|
// outlive the direct child: any descendant that inherited them keeps it pending.
|
|
// The TUI spawns nothing at boot today (it does not auto-connect), so this
|
|
// never fires — but that reason lives outside this file, so cap it here rather
|
|
// than inherit an unrelated process's lifetime if that ever changes.
|
|
//
|
|
// The cost of capping: on that path we remove the work dir while a descendant
|
|
// holding HOME=work may still be writing to it — the #1801 race, re-entered
|
|
// deliberately. That is only acceptable because cleanup() warns instead of
|
|
// throwing, so the worst case is a warning plus a leaked temp dir in tmpdir,
|
|
// never a red smoke. The two are load-bearing for each other.
|
|
const DRAIN_MS = 500;
|
|
|
|
function cleanup() {
|
|
// Never turn a passing smoke into a failure over a leftover temp dir; the OS
|
|
// reclaims tmpdir anyway. On the normal path a warning should not fire, since
|
|
// removal waits for the child's `close` (see done()) — but the two give-up
|
|
// branches there call finish() without it, and a process may still hold the
|
|
// dir, so it is expected rather than anomalous on those.
|
|
removeSafe(work, { label: "smoke:tui" });
|
|
}
|
|
|
|
// Tail of the child's output, for quoting in a diagnostic. Slicing an Ink
|
|
// stream can land mid-CSI-sequence, so drop through the first newline rather
|
|
// than lead with an escape-code fragment that mangles the line after it.
|
|
function outputTail(limit = 800) {
|
|
const tail = output.slice(-limit);
|
|
const nl = tail.indexOf("\n");
|
|
return output.length > limit && nl !== -1 ? tail.slice(nl + 1) : tail;
|
|
}
|
|
|
|
// `message` may be a string or a thunk. Callers that quote the child's output
|
|
// pass a thunk so it is rendered here — after the wait below, by which point
|
|
// `close` guarantees stdout/stderr have been fully drained. Building the string
|
|
// at call time instead can truncate the crash reason that matters most.
|
|
let finished = false;
|
|
function finish(code, message) {
|
|
if (finished) return;
|
|
finished = true;
|
|
cleanup();
|
|
const text = typeof message === "function" ? message() : message;
|
|
if (code === 0) {
|
|
console.log(`smoke:tui OK — ${text}`);
|
|
} else {
|
|
console.error(`smoke:tui FAILED — ${text}`);
|
|
}
|
|
process.exit(code);
|
|
}
|
|
|
|
function done(code, message) {
|
|
if (settled) return;
|
|
settled = true;
|
|
clearTimeout(timer);
|
|
if (childClosed) {
|
|
finish(code, message);
|
|
return;
|
|
}
|
|
// Wait for the child to actually exit before removing the work dir (#1801).
|
|
// The dir doubles as the TUI's HOME, so the Ink process is still writing into
|
|
// it when we signal; an rmSync racing those writes fails with ENOTEMPTY when
|
|
// a file lands after a directory has been read but before it is removed.
|
|
//
|
|
// Wait on `close`, not `exit`: a *spawn failure* emits `error` + `close` and
|
|
// never `exit`, so an `exit` wait would hang here until the give-up timer and
|
|
// then blame a SIGTERM the child never received. `close` also guarantees the
|
|
// stdio pipes are drained, which is what makes the thunked messages complete.
|
|
//
|
|
// This deliberately does NOT use lib/child-cleanup.mjs's `stopChild()` (which
|
|
// pack-and-verify.mjs uses against the same race): that helper resolves on the
|
|
// FIRST of exit/close, whereas this wait is entangled with the drain phase —
|
|
// it must keep waiting for `close` after `exit`, on a shorter re-armed
|
|
// deadline, so the quoted output is complete. Only `removeSafe()` is shared.
|
|
const forceKill = setTimeout(() => child.kill("SIGKILL"), EXIT_GRACE_MS);
|
|
|
|
// Deadline for the wait. Re-armed shorter once the child is gone, since from
|
|
// that point we are only draining pipes. Each message describes what was
|
|
// actually being waited on — the timer can fire for a child that already
|
|
// exited and was never signalled, so it must not claim otherwise.
|
|
let deadline;
|
|
const armDeadline = (ms, warning) => {
|
|
clearTimeout(deadline);
|
|
deadline = setTimeout(() => {
|
|
console.warn(`smoke:tui — ${warning}`);
|
|
finish(code, message);
|
|
}, ms);
|
|
};
|
|
armDeadline(
|
|
EXIT_GRACE_MS * 2,
|
|
`TUI did not close its output streams within ${EXIT_GRACE_MS * 2}ms; cleaning up anyway`,
|
|
);
|
|
|
|
const drainOnly = () => {
|
|
clearTimeout(forceKill);
|
|
armDeadline(
|
|
DRAIN_MS,
|
|
`TUI exited but held its output streams open for ${DRAIN_MS}ms (a descendant may have inherited them); cleaning up anyway`,
|
|
);
|
|
};
|
|
if (childExited) drainOnly();
|
|
else child.once("exit", drainOnly);
|
|
|
|
child.once("close", () => {
|
|
clearTimeout(forceKill);
|
|
clearTimeout(deadline);
|
|
finish(code, message);
|
|
});
|
|
// Only signal a child that is still running: on the crash-before-render and
|
|
// spawn-failure paths there is nothing left to signal, and we are here purely
|
|
// to wait out the remaining `close`.
|
|
if (child.exitCode === null && child.signalCode === null && !child.killed) {
|
|
child.kill("SIGTERM");
|
|
}
|
|
}
|
|
|
|
function onData(chunk) {
|
|
output += chunk.toString();
|
|
if (output.includes(RENDER_MARKER)) {
|
|
done(0, `rendered "${RENDER_MARKER}" panel from --catalog within timeout`);
|
|
}
|
|
}
|
|
|
|
child.stdout.on("data", onData);
|
|
child.stderr.on("data", onData);
|
|
|
|
// Registered before done()'s own one-shot listener, so this always runs first —
|
|
// done() can trust `childClosed` even when called from inside a close handler.
|
|
child.on("close", () => {
|
|
childClosed = true;
|
|
});
|
|
|
|
child.on("exit", (code) => {
|
|
childExited = true;
|
|
if (settled) return;
|
|
// Exiting before the render marker appeared is a failure (crash on boot).
|
|
done(
|
|
1,
|
|
() =>
|
|
`TUI exited (code ${code}) before rendering "${RENDER_MARKER}"\n${outputTail()}`,
|
|
);
|
|
});
|
|
|
|
child.on("error", (err) => {
|
|
done(1, `failed to spawn TUI: ${err.message}`);
|
|
});
|
|
|
|
const timer = setTimeout(() => {
|
|
done(
|
|
1,
|
|
() =>
|
|
`TUI did not render "${RENDER_MARKER}" within ${TIMEOUT_MS}ms\n${outputTail()}`,
|
|
);
|
|
}, TIMEOUT_MS);
|