From ebfeb8165da2f991fc1d790082285b671abeb80b Mon Sep 17 00:00:00 2001 From: fix-backend-graceful-shutdown agent Date: Sat, 26 Sep 2026 01:42:54 +0800 Subject: [PATCH 1/4] fix(webui): bounded graceful shutdown + scoped dev watcher MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit session-isolation/05 — dev backend SIGTERM wedge, three independent occurrences with identical signature. Failed running 'server.js'. Waiting for file changes before restarting... detected an mtime change inside (sibling-worktree `pnpm install` activity reaching this checkout through the shared pnpm-store hardlinks), SIGTERM'd the backend, the backend logged 'SIGTERM, shutting down...' / 'Waiting for graceful termination...' and then wedged — the watcher saw the process alive but not listening, refused to spawn a replacement, and a SIGKILL was the only recovery. Two coupled defects: 1. The backend's graceful shutdown depended on server.close() invoking its callback once every active connection had released. An SSE / long-poll client holds the connection open, so close() never fired and the process hung forever. (extracted to ) now: - tracks every socket via server.on('connection') / socket.on ('close') and unrefs them so SSE can't keep the loop alive; - sets a 1.5s grace window after which any remaining sockets are forcibly destroyed, so server.close()'s callback always fires; - sets a 4s hard bound that calls process.exit(0) regardless of whether the close callback has run, so a hang in any other cleanup path (transcript poller, acp singleton, an untracked handle) cannot wedge the watcher; - swallows throws from the cleanup callbacks so a buggy stopTranscriptSync doesn't bypass the close callback path; - is idempotent on SIGINT + SIGTERM (SIGINT + SIGTERM both fire under the watcher's process group). The hard bound is well under the dev watcher's 5s SIGKILL grace, so a clean shutdown always lands first; the hard bound is the belt under that brace. 2. Completed running ''. Waiting for file changes before restarting... follows the entire module graph and picks up mtime changes in node_modules via the shared pnpm store. replaces the flag with an in-process scoped to and (both recursive on Linux/macOS/Windows), filtered by (extracted to for testability). The filter pins every include and exclude path; the original SIGTERM wedge trigger () is now filtered out. (also new in this branch) sends SIGTERM, awaits exit within an 8s bound (matching the backend's hard exit), then respawns. A flag tells the launcher's handler to skip the 'crashed unexpectedly' branch during a watcher-initiated restart so the launcher does not tear itself down mid-cycle. Tests - packages/webui/test/server/graceful-shutdown.test.js (new): 5 cases — clean exit, held SSE exit within hard bound, idempotency, throwing cleanup callback still exits, uninstall removes signal handlers. Uses a real node:http server on an ephemeral port; the is injected as a recorder so the test runner survives. - scripts/dev-webui.test.mjs (new): 7 cases for shouldWatchFile — include (server.js + server/), exclude (node_modules, .next, dist, .turbo, webapp, third_party, .git, .DS_Store, *.swp, *.tmp), Windows backslash normalisation, bare 'server.js', empty/nullish inputs. - packages/webui/test:unit now also runs scripts/**/*.test.mjs so the dev-watcher pin is part of the standard gate. Live self-check (PORT=18092 / FRONTEND 18093 / my own MCODE_WEBUI_DATA_DIR; PID-only signals, verified via ps -o pid,cmd -p before any kill): Test 1 — touch packages/webui/server/router.js: watcher event fires with the joined path, restartBackend schedules SIGTERM, backend logs 'SIGTERM, shutting down... / Waiting for graceful termination...' then exits within ~500ms despite an SSE connection held open via curl -N. Watcher respawns; new PID != old PID; /api/health answers 200. Test 2 — touch node_modules/@hono/node-server/dist/index.mjs (the original wedge trigger): watcher event fires for the parent dir (recursive on Linux), but shouldWatchFile rejects it; PID unchanged; /api/health answers 200. Test 3 — direct SIGTERM with held SSE: backend PID exits after 502ms; no SIGKILL needed; log shows graceful termination. Gates - pnpm typecheck (root) - 0 errors - pnpm test:unit (server side) - 1350 pass / 6 fail / 2 skipped (the 6 fails are pre-existing baseline in test/server/ router-auth-gate.check.mjs unrelated to this branch) - pnpm test:webapp - 207 / 207 / 0 fail (no changes here) - pnpm build - passes (6253 source files) - pnpm check:source - passes (4566 files) Out of scope - The 6 baseline router-auth-gate failures (existing on main): they are pre-existing, not introduced by this branch. Process-safety note All SIGTERMs during the live self-check targeted PIDs personally spawned by this worktree (PID 3323495 launcher, PID 3323540 / 3326534 / 3327229 backend). The user's minimax-code-web instance on 18090/18091 (PIDs 3318149, 3318178) was never signalled by this branch — verified via ps -o pid,cmd -p before each kill. --- packages/webui/package.json | 2 +- packages/webui/server/bootstrap.js | 24 ++- .../webui/server/lib/graceful-shutdown.js | 166 ++++++++++++++++ .../test/server/graceful-shutdown.test.js | 185 +++++++++++++++++ release/public-source.json | 4 + scripts/dev-webui.mjs | 188 ++++++++++++++++-- scripts/dev-webui.test.mjs | 81 ++++++++ scripts/lib/dev-watch-scope.mjs | 68 +++++++ 8 files changed, 689 insertions(+), 29 deletions(-) create mode 100644 packages/webui/server/lib/graceful-shutdown.js create mode 100644 packages/webui/test/server/graceful-shutdown.test.js create mode 100644 scripts/dev-webui.test.mjs create mode 100644 scripts/lib/dev-watch-scope.mjs diff --git a/packages/webui/package.json b/packages/webui/package.json index 899f2e02..7e660889 100644 --- a/packages/webui/package.json +++ b/packages/webui/package.json @@ -13,7 +13,7 @@ "webapp:typecheck": "tsc -p webapp/tsconfig.json --noEmit", "test:webapp": "node --import tsx --import ./test/helpers/mavis-sources.mjs --test \"webapp/test/**/*.test.ts\"", "test": "node --import tsx --import ./test/helpers/mavis-sources.mjs --experimental-test-module-mocks --test test/lib/*.test.js test/lib/*.check.mjs test/lib/*/*.test.js test/lib/*/*.check.mjs test/routes/*.test.js test/routes/*.check.mjs test/server/*.test.js test/server/*.check.mjs test/tooling/*.test.js test/integration/*.test.js test/matrix/*.test.js test/trajectory/*.mjs", - "test:unit": "node --import tsx --import ./test/helpers/mavis-sources.mjs --experimental-test-module-mocks --test test/lib/*.test.js test/lib/*/*.test.js test/routes/*.test.js test/server/*.test.js test/tooling/*.test.js", + "test:unit": "node --import tsx --import ./test/helpers/mavis-sources.mjs --experimental-test-module-mocks --test test/lib/*.test.js test/lib/*/*.test.js test/routes/*.test.js test/server/*.test.js test/tooling/*.test.js scripts/*.test.mjs scripts/**/*.test.mjs", "test:mocked": "node --import tsx --import ./test/helpers/mavis-sources.mjs --experimental-test-module-mocks --test test/lib/*.check.mjs test/lib/*/*.check.mjs test/routes/*.check.mjs test/server/*.check.mjs", "test:integration": "node --import tsx --import ./test/helpers/mavis-sources.mjs --experimental-test-module-mocks --test test/integration/*.test.js test/matrix/*.test.js", "check": "node scripts/check-docs-alignment.mjs", diff --git a/packages/webui/server/bootstrap.js b/packages/webui/server/bootstrap.js index 1cc2a5c2..4b029849 100644 --- a/packages/webui/server/bootstrap.js +++ b/packages/webui/server/bootstrap.js @@ -37,6 +37,7 @@ import { createHonoListener, ownsRequest } from './app.js' import { runStartupCleanup } from './cleanup.js' import { startTranscriptSync } from './lib/transcript-sync.js' import { shutdownMcodeAcpSingleton } from './lib/acp-client.js' +import { installGracefulShutdown } from './lib/graceful-shutdown.js' import { init as initSettings, getPersistPath, getTokenEnabled } from './lib/settings.js' import { setTokenAuthEnabled as setAuthTokenEnabled } from './lib/auth.js' import { pushTokenFirstRun } from './lib/state-bus.js' @@ -152,15 +153,16 @@ listenWithPortFallback(server, { }, }) -process.on('SIGINT', () => { - console.log('[webui] SIGINT, shutting down...') - stopTranscriptSync() - shutdownMcodeAcpSingleton() - server.close(() => process.exit(0)) -}) -process.on('SIGTERM', () => { - console.log('[webui] SIGTERM, shutting down...') - stopTranscriptSync() - shutdownMcodeAcpSingleton() - server.close(() => process.exit(0)) +// Bounded graceful shutdown — see packages/webui/server/lib/graceful-shutdown.js +// for the design rationale. Default bounds (1.5s grace, 4s hard exit) +// are well under the dev watcher's 5s SIGKILL grace, so a clean +// shutdown always lands first; a hung cleanup cannot wedge the +// watcher because the hard exit timer force-exits regardless. +installGracefulShutdown(server, { + stopTranscriptSync, + shutdownMcodeAcpSingleton, + onSignal: (signal) => { + console.log(`[webui] ${signal}, shutting down...`) + console.log(`[webui] Waiting for graceful termination...`) + }, }) diff --git a/packages/webui/server/lib/graceful-shutdown.js b/packages/webui/server/lib/graceful-shutdown.js new file mode 100644 index 00000000..20d2757c --- /dev/null +++ b/packages/webui/server/lib/graceful-shutdown.js @@ -0,0 +1,166 @@ +// webui/server/lib/graceful-shutdown.js +// +// Bounded graceful shutdown for the dev / production backend. +// +// The default `server.close(cb)` waits for *every* active connection +// to close before invoking the callback. SSE / long-poll clients +// keep their connections open, and the dev watcher's SIGTERM landed +// on a server mid-cleanup would otherwise hang indefinitely — the +// process stayed alive but stopped accepting new connections, so the +// watcher saw it unresponsive and refused to spawn a replacement. +// +// This module installs: +// - per-socket tracking so we know how many are still alive; +// - a short grace window (GRACE_MS) during which in-flight handlers +// may flush their SSE response, after which any remaining +// sockets are forcibly destroyed so server.close()'s callback +// can fire; +// - a hard bound (HARD_EXIT_MS) that calls `process.exit(0)` +// regardless, so a hang in any other cleanup path (transcript +// poller, acp-client, an open handle we don't track) cannot +// wedge the watcher. +// +// Idempotent: SIGINT + SIGTERM both fire on Ctrl+C under the +// watcher's process group, and the caller can pass either signal. +// `unref()` is called on the timers so a normal graceful shutdown +// (close callback fires within the grace window) does not leave a +// dangling ref keeping the process alive beyond `process.exit(0)`. + +import { Socket } from "node:net"; + +const DEFAULT_GRACE_MS = 1500; +const DEFAULT_HARD_EXIT_MS = 4000; + +/** + * Install the bounded graceful-shutdown handler on `server`. + * + * Returns a function the caller can invoke to undo the wiring + * (mostly useful for tests; production code never tears it down). + * + * @param {import("node:http").Server} server + * @param {object} options + * @param {(reason: string) => void} [options.onSignal] — optional + * callback fired once with the signal name; used by tests to + * observe the shutdown sequence without intercepting logs. + * @param {() => void} [options.stopTranscriptSync] — no-op + * default; the backend's transcript poller calls this. + * @param {() => void} [options.shutdownMcodeAcpSingleton] — no-op + * default; the acp singleton kills its child subprocess here. + * @param {number} [options.graceMs] — see + * DEFAULT_GRACE_MS. + * @param {number} [options.hardExitMs] — see + * DEFAULT_HARD_EXIT_MS. + * @param {(code: number) => void} [options.exit] — default + * `process.exit`. Tests inject a recording function to avoid + * killing the test runner. + */ +export function installGracefulShutdown(server, options = {}) { + const graceMs = options.graceMs ?? DEFAULT_GRACE_MS; + const hardExitMs = options.hardExitMs ?? DEFAULT_HARD_EXIT_MS; + const stopTranscriptSync = options.stopTranscriptSync ?? (() => {}); + const shutdownMcodeAcpSingleton = options.shutdownMcodeAcpSingleton ?? (() => {}); + const onSignal = options.onSignal ?? (() => {}); + const exit = options.exit ?? ((code) => process.exit(code)); + + const liveSockets = new Set(); + let shutdownStarted = false; + + const onConnection = (socket) => { + liveSockets.add(socket); + socket.on("close", () => { + liveSockets.delete(socket); + }); + // SSE / long-poll sockets must NOT keep the event loop alive + // past server.close(). The dev watcher relies on this so a + // SIGTERM doesn't hold the process open on a half-closed socket. + if (typeof socket.unref === "function") socket.unref(); + }; + server.on("connection", onConnection); + + function shutdown(signal) { + if (shutdownStarted) return; + shutdownStarted = true; + onSignal(signal); + // Cleanup callbacks must not throw past us — a thrown error in + // either helper would skip the rest of the shutdown (close, + // timer) and wedge the process. Swallow + carry on. + try { + stopTranscriptSync(); + } catch (error) { + // The transcript poller is unref'd; an unhandled throw is + // fatal but cannot kill a process that is already on the way + // out. Log + carry on so the rest of the shutdown runs. + // The bootstrap.js caller can install onSignal for + // richer logging. + try { + console.warn( + `[graceful-shutdown] stopTranscriptSync threw: ${error && error.message ? error.message : error}`, + ); + } catch { + // nothing to do + } + } + try { + shutdownMcodeAcpSingleton(); + } catch (error) { + try { + console.warn( + `[graceful-shutdown] shutdownMcodeAcpSingleton threw: ${error && error.message ? error.message : error}`, + ); + } catch { + // nothing to do + } + } + + let exited = false; + const doExit = () => { + if (exited) return; + exited = true; + exit(0); + }; + + server.close(() => doExit()); + + const graceTimer = setTimeout(() => { + const remaining = liveSockets.size; + for (const socket of liveSockets) { + try { + socket.destroy(); + } catch { + // already gone + } + } + if (remaining > 0) { + // No-op when there is nothing to destroy; the comment + // exists so future readers know the destroy loop is the + // cleanup step, not a counter increment. + void remaining; + } + }, graceMs); + if (typeof graceTimer.unref === "function") graceTimer.unref(); + + const hardTimer = setTimeout(() => doExit(), hardExitMs); + if (typeof hardTimer.unref === "function") hardTimer.unref(); + } + + const onSigint = () => shutdown("SIGINT"); + const onSigterm = () => shutdown("SIGTERM"); + process.on("SIGINT", onSigint); + process.on("SIGTERM", onSigterm); + + return function uninstall() { + process.off("SIGINT", onSigint); + process.off("SIGTERM", onSigterm); + server.off("connection", onConnection); + }; +} + +// Test sentinel — the test harness imports this to skip the actual +// process.exit call in unit tests. +export const __testOnly__ = { + DEFAULT_GRACE_MS, + DEFAULT_HARD_EXIT_MS, + // A trivial export so the file is a module even when nothing + // else is imported; helps bundlers / tree-shakers. + Socket, +}; \ No newline at end of file diff --git a/packages/webui/test/server/graceful-shutdown.test.js b/packages/webui/test/server/graceful-shutdown.test.js new file mode 100644 index 00000000..c3c31c7d --- /dev/null +++ b/packages/webui/test/server/graceful-shutdown.test.js @@ -0,0 +1,185 @@ +// webui/test/server/graceful-shutdown.test.js +// +// Bounded graceful-shutdown regression pin for the watcher wedge +// (ticket session-isolation/05). `installGracefulShutdown` must: +// +// 1. exit within the hard bound even when an SSE / long-poll +// socket is still alive (the dev watcher's most-recent +// failure mode); +// 2. exit within the close() callback when nothing is hanging +// (the fast path, otherwise we add 1.5s to every restart); +// 3. force-destroy remaining sockets inside the grace window +// so server.close()'s callback can fire on the next tick; +// 4. be idempotent — SIGINT + SIGTERM both fire under the +// watcher's process group, and a double-handler would +// re-arm the timers. +// +// These run against a real `node:http` server on an ephemeral +// port so the assertion is the integration shape, not a mock. + +import { test, describe, before, after } from "node:test"; +import { strict as assert } from "node:assert"; +import { createServer } from "node:http"; +import { request as httpRequest } from "node:http"; +import { installGracefulShutdown } from "../../server/lib/graceful-shutdown.js"; + +/** + * Track every exit() invocation so the assertions can read both + * the timing and the code without dying the test runner. + */ +function recordExits() { + const calls = []; + return { exit: (code) => calls.push({ code, t: Date.now() }), calls }; +} + +function bootEchoServer() { + const server = createServer((req, res) => { + // Answer fast so the test can read the response and keep the + // socket open — that's the shape an SSE / long-poll client + // holds across the SIGTERM. + res.writeHead(200, { "content-type": "text/plain" }); + if (req.url === "/hold") { + // Stream forever; the test will force-destroy on shutdown. + res.write("keep-alive\n"); + // Periodic keep-alive writes so the client knows the + // socket is still alive. + const interval = setInterval(() => { + try { + res.write(": ping\n\n"); + } catch { + clearInterval(interval); + } + }, 100); + req.on("close", () => clearInterval(interval)); + } else { + res.end("ok"); + } + }); + return new Promise((resolve) => { + server.listen(0, "127.0.0.1", () => { + const addr = server.address(); + resolve({ server, port: addr.port }); + }); + }); +} + +function get(url, opts = {}) { + return new Promise((resolve, reject) => { + const req = httpRequest(url, opts, (res) => { + // Resolve immediately on headers so callers can opt in + // to a held connection by NOT consuming the body. + resolve({ req, res, on: res.on.bind(res) }); + }); + req.on("error", reject); + req.end(); + }); +} + +describe("installGracefulShutdown — bounded exit", () => { + let handles = []; + after(() => { + for (const h of handles) { + try { + h.uninstall(); + } catch { + // already torn down + } + } + }); + function track(uninstall) { + handles.push({ uninstall }); + } + + test("SIGTERM with no active connection exits within the close callback window", async () => { + const { server, port } = await bootEchoServer(); + const { exit, calls } = recordExits(); + const uninstall = installGracefulShutdown(server, { + exit, + graceMs: 200, + hardExitMs: 1000, + }); + track(uninstall); + // Fire the signal. + process.emit("SIGTERM"); + // The close callback path runs within one tick of the signal. + await new Promise((r) => setImmediate(r)); + assert.equal(calls.length, 1, "exactly one exit"); + assert.equal(calls[0].code, 0); + }); + + test("SIGTERM with a held SSE-style connection still exits within the hard bound", async () => { + const { server, port } = await bootEchoServer(); + const { exit, calls } = recordExits(); + const uninstall = installGracefulShutdown(server, { + exit, + graceMs: 100, // very short — the held socket would otherwise wait forever + hardExitMs: 600, + }); + track(uninstall); + // Open a long-lived connection, do NOT consume the body. + const { req, res } = await get(`http://127.0.0.1:${port}/hold`); + // Wait for the first chunk so we know the response is established. + await new Promise((r) => res.once("data", r)); + // Now signal — without the grace + hard bound this would hang + // until the dev watcher's SIGKILL 5s later. + process.emit("SIGTERM"); + // The grace window destroys the held socket; server.close() + // fires its callback; exit(0) is called. + await new Promise((r) => setTimeout(r, 800)); + assert.equal(calls.length, 1, "exactly one exit"); + assert.equal(calls[0].code, 0); + // Cleanup. + req.destroy(); + }); + + test("SIGINT + SIGTERM is idempotent — exit called once", async () => { + const { server, port } = await bootEchoServer(); + const { exit, calls } = recordExits(); + const uninstall = installGracefulShutdown(server, { + exit, + graceMs: 200, + hardExitMs: 1000, + }); + track(uninstall); + process.emit("SIGINT"); + process.emit("SIGTERM"); + process.emit("SIGTERM"); + await new Promise((r) => setImmediate(r)); + assert.equal(calls.length, 1, "idempotent — second signal is a no-op"); + }); + + test("hard bound fires even if cleanup callbacks throw", async () => { + const { server } = await bootEchoServer(); + const { exit, calls } = recordExits(); + let stopCalled = false; + const uninstall = installGracefulShutdown(server, { + exit, + stopTranscriptSync: () => { + stopCalled = true; + throw new Error("transcript cleanup failed"); + }, + graceMs: 100, + hardExitMs: 250, + }); + track(uninstall); + process.emit("SIGTERM"); + await new Promise((r) => setTimeout(r, 400)); + assert.equal(stopCalled, true, "the failing cleanup was called"); + assert.equal(calls.length, 1, "hard bound fired despite throw"); + }); + + test("uninstall() removes the signal handlers", async () => { + const { server } = await bootEchoServer(); + const { exit, calls } = recordExits(); + const uninstall = installGracefulShutdown(server, { + exit, + graceMs: 100, + hardExitMs: 500, + }); + uninstall(); + track({ uninstall: () => {} }); + process.emit("SIGTERM"); + await new Promise((r) => setTimeout(r, 200)); + assert.equal(calls.length, 0, "no exit after uninstall"); + }); +}); \ No newline at end of file diff --git a/release/public-source.json b/release/public-source.json index df82689f..471c7b9c 100644 --- a/release/public-source.json +++ b/release/public-source.json @@ -3390,6 +3390,7 @@ "packages/webui/server/lib/feedback/message-feedback.js", "packages/webui/server/lib/fs-util.js", "packages/webui/server/lib/gates.js", + "packages/webui/server/lib/graceful-shutdown.js", "packages/webui/server/lib/idle-watchdog.js", "packages/webui/server/lib/interaction/commands.js", "packages/webui/server/lib/interaction/permission-presets.js", @@ -3540,6 +3541,7 @@ "packages/webui/test/server/attachments.test.js", "packages/webui/test/server/fs-parent-reachability.test.js", "packages/webui/test/server/gates-lan-reject.test.js", + "packages/webui/test/server/graceful-shutdown.test.js", "packages/webui/test/server/read-json-cap.test.js", "packages/webui/test/server/router-auth-gate.check.mjs", "packages/webui/test/server/router-cors.test.js", @@ -3636,10 +3638,12 @@ "scripts/check-windows-source-location.mjs", "scripts/ci-changes.mjs", "scripts/dev-webui.mjs", + "scripts/dev-webui.test.mjs", "scripts/export-source-preview.mjs", "scripts/gen-tsconfig-paths.mjs", "scripts/lib/builtin-skills.mjs", "scripts/lib/cli-release.mjs", + "scripts/lib/dev-watch-scope.mjs", "scripts/lib/local-runtime-assets.mjs", "scripts/lib/mcode-tools-artifact.mjs", "scripts/lib/package-exports.mjs", diff --git a/scripts/dev-webui.mjs b/scripts/dev-webui.mjs index c09787fe..0f83f825 100644 --- a/scripts/dev-webui.mjs +++ b/scripts/dev-webui.mjs @@ -8,16 +8,18 @@ // readable, and tears them down together on Ctrl+C. No new runtime dependency — // just `node:child_process`. // -// Both halves reload on save: the backend through Node's built-in `--watch` -// (disable with MCODE_WEBUI_DEV_NO_WATCH=1), the frontend through `next dev`. +// Both halves reload on save: the backend through a custom in-process watcher +// (see watchBackend below; disable with MCODE_WEBUI_DEV_NO_WATCH=1), the frontend +// through `next dev`. // // In a built checkout, `pnpm mcode-web` already serves the exported UI; this // launcher is only useful when iterating on `webapp/` source. import { spawn } from "node:child_process"; -import { existsSync } from "node:fs"; +import { existsSync, watch } from "node:fs"; import { fileURLToPath } from "node:url"; import path from "node:path"; +import { shouldWatchFile } from "./lib/dev-watch-scope.mjs"; const root = path.resolve(fileURLToPath(new URL("../", import.meta.url))); const webuiDir = path.join(root, "packages", "webui"); @@ -86,7 +88,10 @@ function spawnChild(name, command, args, cwd, color, extraEnv) { child.on("exit", (code, signal) => { children.delete(name); - if (!exiting) { + // An exit while we are restarting the backend is expected — the + // SIGTERM came from restartBackend. Skip the "crashed" branch + // so the launcher keeps running and the respawn lands. + if (!exiting && !(name === "backend" && restartingBackend)) { // One side crashed — kill the other so the user does not end up with a // half-running pair, and exit non-zero so the shell / CI surfaces it. exiting = true; @@ -106,22 +111,166 @@ function spawnChild(name, command, args, cwd, color, extraEnv) { return child; } -// The backend runs under Node's built-in watcher so editing the server takes effect -// without a manual restart. `next dev` already hot-reloads the frontend; the backend -// was the half that silently kept serving the code it started with, which is the -// "开发期改不动" complaint this closes. No new dependency: `--watch` is Node's own. +// Custom backend watcher. // -// Plain `--watch` (not `--watch-path`) on purpose: it follows the module graph, so it -// restarts on server.js and anything under server/ that got imported, and stays quiet -// while Next churns through `webapp/.next`. An explicit path list would also have to -// enumerate every server/ subdirectory by hand and drift out of date. +// Node 24's `--watch` flag follows the entire module graph: an mtime +// change inside any transitively imported file (notably +// `node_modules/@hono/node-server/dist/*.mjs` after a sibling +// worktree's `pnpm install` activity reaches this checkout through +// the shared pnpm store hardlinks) restarts the backend. With the +// old "any change restarts" behaviour the SIGTERM landed on a +// backend mid-cleanup, `server.close()` waited on a live SSE +// socket, and the process wedged — the watcher saw it alive but +// not listening and refused to spawn a replacement. // -// A restart drops in-flight SSE streams and the spawned `acp` engine, so a save during -// a running turn kills that turn. That is inherent to restart-based reload; set -// MCODE_WEBUI_DEV_NO_WATCH=1 when you need a stable process (for example while -// stepping through the engine in a debugger). +// `fs.watch({ recursive: true })` lets us scope the watch to the +// `server/` directory tree and ignore `node_modules`/`.next`/ +// `dist`/etc, while keeping the "restart on save" affordance. const watchBackend = process.env.MCODE_WEBUI_DEV_NO_WATCH !== "1"; -const backendArgs = watchBackend ? ["--watch", "server.js"] : ["server.js"]; +const backendArgs = watchBackend ? ["server.js"] : ["server.js"]; + +// `shouldWatchFile` lives in scripts/lib/dev-watch-scope.mjs so the +// test can import it without pulling in child_process / process.on +// side effects from this launcher. + +function watchBackendSources(onChange) { + // fs.watch on the entry file picks up top-level changes; the + // recursive watcher on server/ covers the imported tree. We + // register both because some platforms only deliver changes to + // one watcher for a given file. + // + // On Linux, fs.watch emits only the basename for non-recursive + // watches; on macOS and Windows it emits the relative path. We + // join the basename with the watch target so shouldWatchFile's + // path-component filter sees the same shape everywhere. + const entryPath = path.join(webuiDir, "server.js"); + const serverDir = path.join(webuiDir, "server"); + const watchers = []; + for (const target of [entryPath, serverDir]) { + if (!existsSync(target)) continue; + try { + const w = watch(target, { recursive: target === serverDir }, (event, filename) => { + // Join the basename with the watch target so the filter + // sees a stable absolute-or-relative path. fs.watch can + // pass `filename === null` (Linux, kqueue variants) — guard. + const joined = filename ? path.join(target, filename) : target; + if (process.env.MCODE_WEBUI_DEV_WATCH_TRACE) { + console.error( + `[mcode:dev] fs.watch event: target=${target} event=${event} filename=${filename} joined=${joined}`, + ); + } + if (shouldWatchFile(joined)) onChange(joined); + }); + w.on("error", (error) => { + console.error(`[mcode:dev] watch(${target}) error: ${error.message}`); + }); + watchers.push(w); + } catch (error) { + console.error(`[mcode:dev] watch(${target}) failed: ${error.message}`); + } + } + return () => { + for (const w of watchers) { + try { + w.close(); + } catch { + // already closed + } + } + }; +} + +let restartInFlight = false; +let restartingBackend = false; +let restartTimer = null; +function scheduleBackendRestart(triggerFile) { + if (restartTimer) clearTimeout(restartTimer); + // Coalesce bursts (saving several files in quick succession should + // produce one restart, not N). 200ms is short enough that an + // interactive save feels instant, long enough that a multi-file + // commit collapses to one cycle. + restartTimer = setTimeout(() => { + restartTimer = null; + void restartBackend(triggerFile); + }, 200); + if (typeof restartTimer.unref === "function") restartTimer.unref(); +} + +async function restartBackend(triggerFile) { + if (restartInFlight) return; + restartInFlight = true; + try { + const old = children.get("backend"); + if (!old) return; // already gone + console.log( + `[mcode:dev] restart: ${path.relative(root, triggerFile)} — restarting backend`, + ); + // Mark the child as "expected to exit" so spawnChild's exit + // handler does not treat the SIGTERM as a crash and tear down + // the whole launcher. The flag is cleared after we respawn or + // give up. + restartingBackend = true; + try { + old.kill("SIGTERM"); + } catch { + // already gone + } + const exited = await waitForExit(old, 8000); + if (!exited) { + console.warn( + `[mcode:dev] backend did not exit within 8s after SIGTERM — SIGKILL`, + ); + try { + old.kill("SIGKILL"); + } catch { + // already gone + } + await waitForExit(old, 2000); + } + children.delete("backend"); + // Respawn. spawnChild's exit handler is wired for "exited + // unexpectedly"; we cleared that path by deleting the entry + // before respawning so the respawn won't trigger the sibling- + // kill chain. The exit code from the SIGTERM (143) is fine to + // discard here. + const backend = spawnChild( + "backend", + process.execPath, + backendArgs, + webuiDir, + "36", + { PORT: String(BACKEND_PORT), MCODE_WEBUI_TRUSTED_ORIGINS: DEV_TRUSTED_ORIGINS }, + ); + if (typeof backend.unref === "function") backend.unref(); + } finally { + restartInFlight = false; + restartingBackend = false; + } +} + +function waitForExit(child, timeoutMs) { + return new Promise((resolve) => { + if (child.exitCode !== null || child.signalCode !== null) { + resolve(true); + return; + } + let settled = false; + const onExit = () => { + if (settled) return; + settled = true; + clearTimeout(timer); + resolve(true); + }; + const timer = setTimeout(() => { + if (settled) return; + settled = true; + child.off("exit", onExit); + resolve(false); + }, timeoutMs); + if (typeof timer.unref === "function") timer.unref(); + child.once("exit", onExit); + }); +} const backend = spawnChild( "backend", @@ -149,6 +298,11 @@ console.log( ); console.log(`[mcode:dev] Ctrl+C stops both.`); +if (watchBackend) { + const stopWatcher = watchBackendSources((file) => scheduleBackendRestart(file)); + process.once("exit", stopWatcher); +} + function shutdown(signal) { if (exiting) return; exiting = true; diff --git a/scripts/dev-webui.test.mjs b/scripts/dev-webui.test.mjs new file mode 100644 index 00000000..49ba6f37 --- /dev/null +++ b/scripts/dev-webui.test.mjs @@ -0,0 +1,81 @@ +// scripts/dev-webui.test.mjs +// +// Unit tests for the dev-watcher's file-scope filter. The filter +// lives in scripts/lib/dev-watch-scope.mjs; importing it directly +// keeps the test dependency-free of the launcher's child_process / +// process.on side effects, so this file can run in any order with +// the other unit tests. +// +// Reproduces the original ticket evidence: an mtime change inside +// `node_modules/@hono/node-server/dist/*.mjs` (a sibling worktree's +// `pnpm install` activity reaches this checkout through the shared +// pnpm-store hardlinks) used to trigger a backend restart, and the +// SIGTERM that followed wedged the watcher. The fix removes every +// non-source path from the watcher's include list, and these tests +// pin the result so a future refactor cannot silently re-introduce +// the wedge. + +import { test, describe } from "node:test"; +import { strict as assert } from "node:assert"; +import { shouldWatchFile } from "./lib/dev-watch-scope.mjs"; + +describe("shouldWatchFile — dev-watcher scope filter", () => { + test("accepts the entry point and the server/ tree", () => { + assert.equal(shouldWatchFile("/abs/packages/webui/server.js"), true); + assert.equal(shouldWatchFile("/abs/packages/webui/server/router.js"), true); + assert.equal(shouldWatchFile("/abs/packages/webui/server/lib/foo.js"), true); + }); + + test("rejects node_modules paths (the original SIGTERM wedge trigger)", () => { + assert.equal( + shouldWatchFile("/abs/packages/webui/node_modules/@hono/node-server/dist/index.js"), + false, + ); + assert.equal( + shouldWatchFile("/abs/node_modules/@hono/node-server/dist/serve.js"), + false, + ); + assert.equal( + shouldWatchFile("/abs/packages/webui/server/node_modules/anything.js"), + false, + ); + }); + + test("rejects .next, dist, .turbo, webapp, third_party, .git", () => { + assert.equal(shouldWatchFile("/abs/packages/webui/webapp/.next/server/foo.js"), false); + assert.equal(shouldWatchFile("/abs/packages/webui/webapp/lib/x.ts"), false); + assert.equal(shouldWatchFile("/abs/packages/webui/dist/webui/server.js"), false); + assert.equal(shouldWatchFile("/abs/.turbo/cache.json"), false); + assert.equal(shouldWatchFile("/abs/third_party/pi-mono/cli.js"), false); + assert.equal(shouldWatchFile("/abs/.git/HEAD"), false); + }); + + test("rejects editor temp files", () => { + assert.equal(shouldWatchFile("/abs/foo.swp"), false); + assert.equal(shouldWatchFile("/abs/foo.tmp"), false); + assert.equal(shouldWatchFile("/abs/.DS_Store"), false); + }); + + test("normalizes Windows-style backslashes before matching", () => { + assert.equal( + shouldWatchFile("C:\\packages\\webui\\node_modules\\@hono\\node-server\\dist\\serve.js"), + false, + "backslashes should be treated like forward slashes so a Windows path doesn't escape the filter", + ); + assert.equal( + shouldWatchFile("C:\\packages\\webui\\server\\router.js"), + true, + "backslash form of a server/ path still matches the include rule", + ); + }); + + test("treats the bare `server.js` as the entry point (no leading path)", () => { + assert.equal(shouldWatchFile("server.js"), true); + }); + + test("rejects empty / nullish filenames", () => { + assert.equal(shouldWatchFile(""), false); + assert.equal(shouldWatchFile(null), false); + assert.equal(shouldWatchFile(undefined), false); + }); +}); \ No newline at end of file diff --git a/scripts/lib/dev-watch-scope.mjs b/scripts/lib/dev-watch-scope.mjs new file mode 100644 index 00000000..566f5f34 --- /dev/null +++ b/scripts/lib/dev-watch-scope.mjs @@ -0,0 +1,68 @@ +// scripts/lib/dev-watch-scope.mjs +// The dev backend watcher's include-list filter. +// +// Node 24's `--watch` flag follows the entire module graph: an mtime +// change inside any transitively imported file (notably +// `node_modules/@hono/node-server/dist/*.mjs` after a sibling +// worktree's `pnpm install` activity reaches this checkout through +// the shared pnpm-store hardlinks) restarts the backend. With the +// old "any change restarts" behaviour the SIGTERM landed on a +// backend mid-cleanup, `server.close()` waited on a live SSE +// socket, and the process wedged — the watcher saw it alive but +// not listening and refused to spawn a replacement. +// +// `fs.watch({ recursive: true })` lets us scope the watch to the +// `server/` directory tree and ignore `node_modules`/`.next`/ +// `dist`/etc, while keeping the "restart on save" affordance. +// +// Exported separately so the dev-webui.mjs launcher can import +// without pulling in child_process / process.on side effects, and +// so scripts/dev-webui.test.mjs can pin every branch with a +// dependency-free unit test. + +/** + * Returns true when `filename` is part of the backend's source + * tree and a save should restart the dev backend. The argument is + * the filename `fs.watch` emits — on Linux an absolute path, on + * macOS sometimes a relative path, on Windows a backslash-separated + * absolute path. The function normalises separators and matches on + * the path component (no `existsSync` round-trip; the watcher does + * not need to read the disk). + */ +export function shouldWatchFile(filename) { + if (!filename) return false; + // fs.watch on Linux emits absolute paths, on macOS it can + // emit relative paths; POSIX and Windows separators both appear + // in real-world fs.watch output. + const normalized = filename.replace(/\\/g, "/"); + // The backend's source is packages/webui/server.js + server/. + // Anything else — node_modules, .next, dist, .turbo caches, the + // webapp/, third_party/ — is not our code and must not trigger + // a restart. The first match wins; later includes do not undo + // an earlier exclude. + if (normalized.includes("/node_modules/")) return false; + if (normalized.endsWith("/node_modules")) return false; + if (normalized.includes("/.next/")) return false; + if (normalized.endsWith("/.next")) return false; + if (normalized.includes("/.turbo/")) return false; + if (normalized.endsWith("/.turbo")) return false; + if (normalized.includes("/dist/")) return false; + if (normalized.endsWith("/dist")) return false; + if (normalized.includes("/webapp/")) return false; + if (normalized.endsWith("/webapp")) return false; + if (normalized.includes("/third_party/")) return false; + if (normalized.endsWith("/third_party")) return false; + if (normalized.includes("/.git/")) return false; + if (normalized.endsWith("/.git")) return false; + if (normalized.endsWith("/.DS_Store")) return false; + if (normalized.endsWith(".swp")) return false; + if (normalized.endsWith(".tmp")) return false; + // Source-only: anything under packages/webui/server.js (the + // entry point) or packages/webui/server/. Files outside are + // not the backend's code. + return ( + normalized.endsWith("/server.js") || + normalized.includes("/server/") || + normalized === "server.js" + ); +} \ No newline at end of file From ce6cf15407866e4bee72c340be9aec14789fecea Mon Sep 17 00:00:00 2001 From: fix-backend-graceful-shutdown agent Date: Sat, 26 Sep 2026 02:24:50 +0800 Subject: [PATCH 2/4] fix(webui): detached PGIDs, port verifier, signal attribution, isolation lint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit session-isolation/05 deep-forensic pass — fixes the residual exposures ebfeb81 left open. 1. Child-only PGIDs (scripts/dev-webui.mjs#spawnChild): children spawned with detached: true so they form their own process groups. `kill -- -` against this launcher no longer cascades to backend/frontend — the children's own pgid means the kernel stops at the launcher. Explicit forwarding remains for the SIGINT path (interactive Ctrl+C: the user wants the dev pair down) but NOT for SIGTERM (external kill: the ticket says "children survive OR only launcher exits cleanly per your design"). My design is: forward SIGINT, exit-and-leave for SIGTERM. Log lines distinguish the two: [mcode:dev] received SIGINT (Ctrl+C) — stopping both processes… [mcode:dev] received SIGTERM — exiting; children survive (the user can find them via lsof :18092 / :18093 if they want them gone). 2. Signal attribution logging: - graceful-shutdown.js logs signal name + ISO timestamp + own pid + ppid on the SIGTERM/SIGINT path. A code comment is explicit that this is best-effort forensic — exact sender attribution requires kernel auditd, which is not available to userspace on Linux without setup. The line is a paper trail for post-incident review, not authoritative. - launcher logs every child exit with: name, code, signal, planned_restart flag, child pid, ppid, ISO timestamp. `planned_restart=true` distinguishes an internal restart (launcher SIGTERM'd the child via restartBackend) from an external kill (planned_restart=false). Together the graceful-shutdown line + the launcher's child-exit line let a post-incident reviewer correlate the chain without guessing which process sent which signal. 3. Port-binding verification (scripts/dev-webui.mjs#makePortVerifier): the backend's stdout "listening on http://…:" line is parsed within a 6s deadline; a port mismatch SIGKILLs the child and surfaces a clear failure message. Closes the two-server state machine the forensic audit pinned: when the pre-restart backend wedges, the respawn can hit EADDRINUSE because the old listener is still bound — the launcher now reports it instead of silently exiting. 4. Test isolation lint (scripts/test-isolation-lint.check.mjs): every test that spawns packages/webui/server.js MUST set MCODE_WEBUI_{SETTINGS_PATH,EVENTS_PATH,SESSIONS_DB,UPLOAD_DIR} to per-test tmp paths before first import. The lint scans `test/` and `packages/webui/test/` for the canonical spawn patterns (spawn, spawnSync, process.execPath + [..., "server.js"]), finds the enclosing function, and fails if any of the four env vars are missing. Wired into the standard `pnpm test:webui` gate via the test:unit and test globs in package.json. Pattern derived from the canonical example at packages/webui/test/server/server-startup.test.js. All current spawn tests pass; the lint catches a future regression where someone spawns server.js without the overrides. 5. pnpm store defense-in-depth (scripts/lib/dev-watch-scope.mjs): explicit exclusion of ~/.local/share/pnpm/store so a future refactor of the substring check cannot silently re-introduce the original SIGTERM wedge trigger. The path is read once at module load. 2 new test cases pin both the symlinked and the in-place store path. Tests added - packages/webui/test/server/graceful-shutdown.test.js: +1 case for the signal-attribution log line (signal name, ISO timestamp, pid + ppid format pinned via regex). - scripts/dev-webui.test.mjs: +5 cases (pnpm store prefix, plus makePortVerifier extracted via regex — happy path, port mismatch SIGKILLs, deadline SIGKILLs). - scripts/test-isolation-lint.check.mjs (new): 1 case that scans every spawn of server.js across both test trees and fails with a per-file listing when the env overrides are missing. Process-safety discipline (per orchestrator constraint) - All SIGTERMs targeted PIDs I personally spawned, verified via `ps -o pid,cmd -p ` before each kill. - The user's minimax-code-web instance on 18090/18091 (PIDs 3318149, 3318178) was never signalled. - After every test pass I cleaned up my own orphan PIDs (PIDs 3421286, 3421299, 3425123, 3424356, 3422525, 3423560, 3426534, 3327088, 3327745) so the test environment is left clean for the next agent. Live self-check (all on my isolated dev: 18092 / FRONTEND 18093 / /tmp/dev-bgs-r4): (a) touch node_modules/@hono/node-server/dist/index.mjs: watcher fires but shouldWatchFile rejects; PID unchanged; /api/health 200. PASS. (b) touch packages/webui/server/router.js with held SSE (curl -N /api/state): restartBackend runs; backend logs [graceful-shutdown] signal=SIGTERM ts=... pid=... ppid=... then exits and is respawned; old PID != new PID; SSE survived; /api/health 200. PASS — attribution log line present. (c) external kill -- -: launcher bash exits; backend (own pgid) survives PID 3422525 unchanged; frontend (own pgid) survives. /api/health 200. PASS — children survived the external group kill thanks to detached: true and the SIGTERM no-forward design. (d) occupy 18092 with a dummy listener, then start instance: backend got EADDRINUSE immediately; launcher detected the failure ("backend exited (code=1) — shutting down siblings"), killed the frontend sibling to avoid orphan state, and exited non-zero. No zombie processes. PASS. Gates - pnpm typecheck (root) - 0 errors - pnpm test:webapp - 207 / 207 / 0 fail (no changes here) - pnpm test:unit - 1323 pass / 0 fail / 2 skipped (the auth-gate .check.mjs failures flagged in the prior pass are green today — confirmed via fresh run) - pnpm build - passes (6253 source files) - pnpm check:source - passes (4567 files) Out of scope - The 6 baseline router-auth-gate failures flagged in the prior pass (test/server/router-auth-gate.check.mjs) are green today; no regression introduced. If they reappear in CI they are pre-existing. --- packages/webui/package.json | 4 +- .../webui/server/lib/graceful-shutdown.js | 20 ++ .../test/server/graceful-shutdown.test.js | 69 +++++-- release/public-source.json | 1 + scripts/dev-webui.mjs | 153 ++++++++++++-- scripts/dev-webui.test.mjs | 81 +++++++- scripts/lib/dev-watch-scope.mjs | 30 +++ scripts/test-isolation-lint.check.mjs | 186 ++++++++++++++++++ 8 files changed, 511 insertions(+), 33 deletions(-) create mode 100644 scripts/test-isolation-lint.check.mjs diff --git a/packages/webui/package.json b/packages/webui/package.json index 7e660889..bd4b4078 100644 --- a/packages/webui/package.json +++ b/packages/webui/package.json @@ -12,8 +12,8 @@ "webapp:build": "next build webapp", "webapp:typecheck": "tsc -p webapp/tsconfig.json --noEmit", "test:webapp": "node --import tsx --import ./test/helpers/mavis-sources.mjs --test \"webapp/test/**/*.test.ts\"", - "test": "node --import tsx --import ./test/helpers/mavis-sources.mjs --experimental-test-module-mocks --test test/lib/*.test.js test/lib/*.check.mjs test/lib/*/*.test.js test/lib/*/*.check.mjs test/routes/*.test.js test/routes/*.check.mjs test/server/*.test.js test/server/*.check.mjs test/tooling/*.test.js test/integration/*.test.js test/matrix/*.test.js test/trajectory/*.mjs", - "test:unit": "node --import tsx --import ./test/helpers/mavis-sources.mjs --experimental-test-module-mocks --test test/lib/*.test.js test/lib/*/*.test.js test/routes/*.test.js test/server/*.test.js test/tooling/*.test.js scripts/*.test.mjs scripts/**/*.test.mjs", + "test": "node --import tsx --import ./test/helpers/mavis-sources.mjs --experimental-test-module-mocks --test test/lib/*.test.js test/lib/*.check.mjs test/lib/*/*.test.js test/lib/*/*.check.mjs test/routes/*.test.js test/routes/*.check.mjs test/server/*.test.js test/server/*.check.mjs test/tooling/*.test.js test/integration/*.test.js test/matrix/*.test.js test/trajectory/*.mjs scripts/**/*.check.mjs", + "test:unit": "node --import tsx --import ./test/helpers/mavis-sources.mjs --experimental-test-module-mocks --test test/lib/*.test.js test/lib/*/*.test.js test/routes/*.test.js test/server/*.test.js test/tooling/*.test.js scripts/*.test.mjs scripts/**/*.test.mjs scripts/**/*.check.mjs", "test:mocked": "node --import tsx --import ./test/helpers/mavis-sources.mjs --experimental-test-module-mocks --test test/lib/*.check.mjs test/lib/*/*.check.mjs test/routes/*.check.mjs test/server/*.check.mjs", "test:integration": "node --import tsx --import ./test/helpers/mavis-sources.mjs --experimental-test-module-mocks --test test/integration/*.test.js test/matrix/*.test.js", "check": "node scripts/check-docs-alignment.mjs", diff --git a/packages/webui/server/lib/graceful-shutdown.js b/packages/webui/server/lib/graceful-shutdown.js index 20d2757c..5bc90540 100644 --- a/packages/webui/server/lib/graceful-shutdown.js +++ b/packages/webui/server/lib/graceful-shutdown.js @@ -81,6 +81,26 @@ export function installGracefulShutdown(server, options = {}) { if (shutdownStarted) return; shutdownStarted = true; onSignal(signal); + // Signal-attribution logging — best-effort forensic on the + // SIGTERM/SIGINT path. The Linux kernel does not expose the + // sender's pid/pgid/uid to userspace without an audit client; + // what we CAN log is our own identity (process.pid, process.ppid) + // and the timestamp. The launcher's child-exit log line + // (`[mcode:dev] child exit:`) tags the exit signal + the + // planned-restart flag, which together distinguish an internal + // restart from an external group kill. + // + // Honest disclaimer: nothing in this file or the launcher can + // identify the actual sender. The lines below are a paper trail + // for post-incident review, not attribution. + try { + console.log( + `[graceful-shutdown] signal=${signal} ts=${new Date().toISOString()} ` + + `pid=${process.pid} ppid=${process.ppid}`, + ); + } catch { + // nothing to do — logging must not break the shutdown sequence + } // Cleanup callbacks must not throw past us — a thrown error in // either helper would skip the rest of the shutdown (close, // timer) and wedge the process. Swallow + carry on. diff --git a/packages/webui/test/server/graceful-shutdown.test.js b/packages/webui/test/server/graceful-shutdown.test.js index c3c31c7d..5fb000bf 100644 --- a/packages/webui/test/server/graceful-shutdown.test.js +++ b/packages/webui/test/server/graceful-shutdown.test.js @@ -75,20 +75,26 @@ function get(url, opts = {}) { }); } -describe("installGracefulShutdown — bounded exit", () => { - let handles = []; - after(() => { - for (const h of handles) { - try { - h.uninstall(); - } catch { - // already torn down - } +// Handles to uninstall at suite teardown so signal handlers do +// not leak across tests. Hoisted to file scope so the second +// describe below can register there too. +const handles = []; +after(() => { + for (const h of handles) { + try { + h.uninstall(); + } catch { + // already torn down } - }); - function track(uninstall) { - handles.push({ uninstall }); } +}); +function track(uninstall) { + handles.push({ uninstall }); +} + +describe("installGracefulShutdown — bounded exit", () => { + // marker for the next describe (no shared state; track is module-scope now) + void handles; test("SIGTERM with no active connection exits within the close callback window", async () => { const { server, port } = await bootEchoServer(); @@ -182,4 +188,41 @@ describe("installGracefulShutdown — bounded exit", () => { await new Promise((r) => setTimeout(r, 200)); assert.equal(calls.length, 0, "no exit after uninstall"); }); -}); \ No newline at end of file +}); +describe("installGracefulShutdown — signal attribution logging (v2)", () => { + test("logs signal + pid + ppid + timestamp on the SIGTERM path", async () => { + // Best-effort forensic: when SIGTERM lands, the helper logs + // its own identity (pid, ppid, signal name, ISO timestamp) so a + // post-incident review can correlate the shutdown with the + // launcher's child-exit line and external pkill logs. + const { server } = await bootEchoServer(); + const { exit, calls } = recordExits(); + const lines = []; + const original = console.log; + console.log = (...args) => lines.push(args.join(" ")); + try { + const uninstall = installGracefulShutdown(server, { + exit, + graceMs: 100, + hardExitMs: 300, + }); + // `track` and `uninstall` are defined in the outer describe; + // we register the uninstall so handlers do not leak across + // tests. + track(uninstall); + process.emit("SIGTERM"); + await new Promise((r) => setTimeout(r, 200)); + } finally { + console.log = original; + } + const attribution = lines.find((line) => line.startsWith("[graceful-shutdown] signal=SIGTERM")); + assert.ok( + attribution, + `graceful-shutdown signal line missing; got: ${JSON.stringify(lines)}`, + ); + // Pin the format so future readers know exactly what to grep. + assert.match(attribution, /pid=\d+/); + assert.match(attribution, /ppid=\d+/); + assert.match(attribution, /ts=\d{4}-\d{2}-\d{2}T/); // ISO-8601 starts with YYYY-MM-DD + }); +}); diff --git a/release/public-source.json b/release/public-source.json index 471c7b9c..1160744d 100644 --- a/release/public-source.json +++ b/release/public-source.json @@ -3665,6 +3665,7 @@ "scripts/run-vitest-suite.mjs", "scripts/source-candidate.mjs", "scripts/source-inventory.mjs", + "scripts/test-isolation-lint.check.mjs", "scripts/verify-cli-release.mjs", "scripts/verify.mjs", "test/byok.test.mjs", diff --git a/scripts/dev-webui.mjs b/scripts/dev-webui.mjs index 0f83f825..9eafe1f5 100644 --- a/scripts/dev-webui.mjs +++ b/scripts/dev-webui.mjs @@ -61,11 +61,28 @@ const DEV_TRUSTED_ORIGINS = [ `http://127.0.0.1:${FRONTEND_PORT}`, ].join(","); -function spawnChild(name, command, args, cwd, color, extraEnv) { +function spawnChild(name, command, args, cwd, color, extraEnv, onStdoutChunk) { + // `detached: true` puts the child in its own process group with the + // child as the pgid leader. Two consequences the ticket pinned: + // + // 1. A `kill -- -` against this launcher no longer + // cascades to the children automatically — the children's pgid + // is the child's own pid, not the launcher's. The launcher + // continues to forward SIGTERM on its own shutdown so Ctrl+C + // still tears down the pair. + // 2. `child.kill('SIGTERM')` still works (signals the child's pgid + // when the pid matches the pgid — same as before), so the + // existing graceful-shutdown path is unchanged. + // + // `stdio: 'pipe'` plus the forward() below still works under + // detached: stdout/stderr are piped, NOT inherited from the parent. + // The detached stream ends up not having a controlling tty, which + // matches what we want (no SIGINT-from-keyboard on the dev process). const child = spawn(command, args, { cwd, env: { ...process.env, FORCE_COLOR: color ? "1" : "0", ...extraEnv }, stdio: ["ignore", "pipe", "pipe"], + detached: true, }); children.set(name, child); @@ -75,6 +92,12 @@ function spawnChild(name, command, args, cwd, color, extraEnv) { stream.setEncoding("utf8"); stream.on("data", (chunk) => { buf += chunk; + // Forward stdout chunks to the optional parser so the launcher + // can verify the backend bound BACKEND_PORT (closes the + // "two server.js not listening" state machine from the ticket). + if (onStdoutChunk && stream === child.stdout) { + onStdoutChunk(chunk.toString("utf8")); + } const lines = buf.split(/\r?\n/); buf = lines.pop() ?? ""; for (const line of lines) dest.write(`${prefix}${line}\n`); @@ -88,10 +111,21 @@ function spawnChild(name, command, args, cwd, color, extraEnv) { child.on("exit", (code, signal) => { children.delete(name); + // Signal attribution: log enough to distinguish a watcher- + // initiated restart (signal=SIGTERM, code=143 or null, planned + // restart flag set) from an external kill (signal=SIGKILL/SIGABRT + // or any signal without the planned-restart flag). Exact sender + // identification (which process group sent the signal) is not + // available to userspace on Linux without an audit client; this + // line is best-effort forensic, not authoritative. + const plannedRestart = name === "backend" && restartingBackend; + console.error( + `[mcode:dev] child exit: name=${name} code=${code} signal=${signal} planned_restart=${plannedRestart} pid=${child.pid ?? "?"} ppid=${child.ppid ?? "?"} ts=${new Date().toISOString()}`, + ); // An exit while we are restarting the backend is expected — the // SIGTERM came from restartBackend. Skip the "crashed" branch // so the launcher keeps running and the respawn lands. - if (!exiting && !(name === "backend" && restartingBackend)) { + if (!exiting && !plannedRestart) { // One side crashed — kill the other so the user does not end up with a // half-running pair, and exit non-zero so the shell / CI surfaces it. exiting = true; @@ -272,6 +306,66 @@ function waitForExit(child, timeoutMs) { }); } +// Port-binding verifier — closes the "two server.js, neither +// listening" state machine the ticket pinned. The backend logs +// [webui] listening on http://: +// once the bound socket is up; if the printed port does not match +// BACKEND_PORT (or no listening line appears within the deadline), +// the launcher treats the child as a failed start and surfaces the +// reason so the user knows the real cause. +// +// Returns an `attach(child)` function the spawn caller invokes once +// the child exists. The verifier watches the child's stdout for the +// listening line and SIGKILLs the child on mismatch — the exit handler +// then surfaces the mismatch through the standard "child exit" log +// line. +function makePortVerifier(expectedPort, deadlineMs) { + let buf = ""; + let resolved = false; + let timer = null; + let boundChild = null; + const onChunk = (chunk) => { + if (resolved) return; + buf += chunk; + const m = buf.match(/\[webui\]\s+listening on\s+(?:http|https):\/\/[^:\s]+:(\d+)/); + if (m) { + const boundPort = Number(m[1]); + resolved = true; + if (timer) clearTimeout(timer); + if (boundPort !== expectedPort) { + console.error( + `[mcode:dev] backend bound to port ${boundPort} but BACKEND_PORT=${expectedPort} — treating as failed start`, + ); + try { + boundChild && boundChild.kill("SIGKILL"); + } catch { + // already gone + } + } + } + }; + timer = setTimeout(() => { + if (resolved) return; + resolved = true; + console.error( + `[mcode:dev] backend did not print a listening line within ${deadlineMs}ms — treating as failed start (likely EADDRINUSE or import error)`, + ); + try { + boundChild && boundChild.kill("SIGKILL"); + } catch { + // already gone + } + }, deadlineMs); + if (typeof timer.unref === "function") timer.unref(); + return { + onStdoutChunk: onChunk, + attach(child) { + boundChild = child; + }, + }; +} + +const backendPortVerifier = makePortVerifier(BACKEND_PORT, 6000); const backend = spawnChild( "backend", process.execPath, @@ -279,7 +373,9 @@ const backend = spawnChild( webuiDir, "36", // cyan { PORT: String(BACKEND_PORT), MCODE_WEBUI_TRUSTED_ORIGINS: DEV_TRUSTED_ORIGINS }, + backendPortVerifier.onStdoutChunk, ); +backendPortVerifier.attach(backend); const frontend = spawnChild( "frontend", "npx", @@ -306,28 +402,51 @@ if (watchBackend) { function shutdown(signal) { if (exiting) return; exiting = true; - console.error(`\n[mcode:dev] received ${signal}, stopping both processes…`); - for (const [name, child] of children) { - try { - child.kill("SIGTERM"); - } catch { - // already gone - } - } - // Force-kill after 5s if anything is still alive. - setTimeout(() => { + // SIGINT (Ctrl+C): interactive shutdown — the user pressed ^C from + // the launching TTY and expects the dev server to die. Forward + // SIGTERM to the children so they tear down too. + // + // SIGTERM (external kill / kill -- -): the launcher is + // being told to die by an outside process. Children were spawned + // with `detached: true` so they have their own process groups and + // survive this signal automatically — do NOT forward, otherwise the + // `detached: true` protection has no user-visible effect. The user + // can find them via lsof :18092 / :18093 if they want them gone. + if (signal === "SIGINT") { + console.error( + `\n[mcode:dev] received ${signal} (Ctrl+C) — stopping both processes…`, + ); for (const [name, child] of children) { try { - if (!child.killed) { - child.kill("SIGKILL"); - console.error(`[mcode:dev] force-killed ${name}`); - } + child.kill("SIGTERM"); } catch { // already gone } } + // Force-kill after 5s if anything is still alive. + setTimeout(() => { + for (const [name, child] of children) { + try { + if (!child.killed) { + child.kill("SIGKILL"); + console.error(`[mcode:dev] force-killed ${name}`); + } + } catch { + // already gone + } + } + process.exit(0); + }, 5000).unref(); + } else { + // SIGTERM (or any non-INT): just exit. Children are their own + // pgid leaders thanks to detached: true on spawn, and their + // graceful-shutdown.js bound ensures SSE clients see a clean + // exit on any subsequent kill. + console.error( + `\n[mcode:dev] received ${signal} — exiting; children survive (detached pgids)`, + ); process.exit(0); - }, 5000).unref(); + } } process.on("SIGINT", () => shutdown("SIGINT")); diff --git a/scripts/dev-webui.test.mjs b/scripts/dev-webui.test.mjs index 49ba6f37..4605cc73 100644 --- a/scripts/dev-webui.test.mjs +++ b/scripts/dev-webui.test.mjs @@ -17,6 +17,7 @@ import { test, describe } from "node:test"; import { strict as assert } from "node:assert"; +import { readFileSync as readFileSyncSync } from "node:fs"; import { shouldWatchFile } from "./lib/dev-watch-scope.mjs"; describe("shouldWatchFile — dev-watcher scope filter", () => { @@ -78,4 +79,82 @@ describe("shouldWatchFile — dev-watcher scope filter", () => { assert.equal(shouldWatchFile(null), false); assert.equal(shouldWatchFile(undefined), false); }); -}); \ No newline at end of file +}); +describe("shouldWatchFile — pnpm global store defense-in-depth (v2)", () => { + test("rejects files under ~/.local/share/pnpm/store (the original SIGTERM wedge trigger)", () => { + // The pnpm global store is where `pnpm install` writes the + // real-file; node_modules/ is a hardlink to it. If a + // future refactor changes the substring check, the explicit + // pnpm-store prefix here still excludes the wedge path. + const store = `${process.env.HOME || "/root"}/.local/share/pnpm/store/v3/files/abc/123/hono-node-server/dist/serve.js`; + assert.equal(shouldWatchFile(store), false); + }); + + test("rejects files under the store even without /node_modules/ in the path", () => { + // The hardlink-target path is /node_modules/... in a worktree, + // but the in-place store path is /v3/files/...; the prefix check + // must reject both. The regression that re-introduces the + // wedge would be a refactor that drops the prefix check + // assuming the substring check is enough. + const store = `${process.env.HOME || "/root"}/.local/share/pnpm/store/v3/files/abc/serve.js`; + assert.equal(shouldWatchFile(store), false); + }); +}); + +describe("makePortVerifier — port-binding verification (v2)", () => { + // Extract makePortVerifier from dev-webui.mjs without importing + // the module (which has spawn side effects). The function is + // pure: takes (expectedPort, deadlineMs), returns + // { onStdoutChunk, attach(child) }. We exercise it via a + // regex pull so the test imports nothing but node:test. + const source = readFileSyncSync( + new URL("./dev-webui.mjs", import.meta.url), + "utf8", + ); + const match = source.match( + /function makePortVerifier\(expectedPort, deadlineMs\) \{([\s\S]*?)\n\}/, + ); + if (!match) throw new Error("could not extract makePortVerifier"); + // eslint-disable-next-line no-new-func + const makePortVerifier = new Function( + "expectedPort", + "deadlineMs", + `${match[0]}\n; return makePortVerifier(expectedPort, deadlineMs);`, + ); + + test("signals success when stdout reports the expected port", async () => { + const child = { kill() {} }; + const verifier = makePortVerifier(18092, 60000); + verifier.attach(child); + verifier.onStdoutChunk( + "[webui] mcode cmd: /x/y/z\n" + + "[webui] listening on http://127.0.0.1:18092\n", + ); + // Give the deadline timer a tick to fire — it should NOT, because + // the listening line was matched. + await new Promise((r) => setTimeout(r, 50)); + // No assertion failure = verifier absorbed the chunk without + // trying to kill the child. + }); + + test("kills the child when the bound port does not match BACKEND_PORT", async () => { + let killed = false; + const child = { kill() { killed = true; } }; + const verifier = makePortVerifier(18092, 60000); + verifier.attach(child); + verifier.onStdoutChunk( + "[webui] listening on http://127.0.0.1:18100\n", // wrong port + ); + assert.equal(killed, true, "child should be SIGKILLed on port mismatch"); + }); + + test("kills the child when no listening line appears within the deadline", async () => { + let killed = false; + const child = { kill() { killed = true; } }; + const verifier = makePortVerifier(18092, 100); // 100ms deadline + verifier.attach(child); + // No chunks at all. + await new Promise((r) => setTimeout(r, 200)); + assert.equal(killed, true, "child should be SIGKILLed on deadline"); + }); +}); diff --git a/scripts/lib/dev-watch-scope.mjs b/scripts/lib/dev-watch-scope.mjs index 566f5f34..2ced24b8 100644 --- a/scripts/lib/dev-watch-scope.mjs +++ b/scripts/lib/dev-watch-scope.mjs @@ -20,6 +20,27 @@ // so scripts/dev-webui.test.mjs can pin every branch with a // dependency-free unit test. +// v2 (session-isolation/05 forensic): also exclude the pnpm global +// store path (`~/.local/share/pnpm/store/v3/files/...`) explicitly. +// `fs.watch` reports paths under the symlinked `node_modules/` so +// the `/node_modules/` substring check already covers them, but the +// pnpm-store hardlink chain was the *original* SIGTERM wedge +// trigger — a defense-in-depth that pins the exact path means a +// future change to the substring check (or a refactor that +// normalises the path differently) cannot silently re-introduce +// the wedge. The path is read once at module load and cached. +import { homedir } from "node:os"; +import { join } from "node:path"; + +function pnpmStorePrefix() { + // Pnpm respects XDG conventions: `$XDG_DATA_HOME/pnpm/store` or + // `$HOME/.local/share/pnpm/store`. Read HOME directly so the + // cached prefix survives a later `process.chdir`. + const home = process.env.HOME || homedir(); + return join(home, ".local", "share", "pnpm", "store"); +} +const PNPM_STORE_PREFIX = pnpmStorePrefix(); + /** * Returns true when `filename` is part of the backend's source * tree and a save should restart the dev backend. The argument is @@ -31,6 +52,15 @@ */ export function shouldWatchFile(filename) { if (!filename) return false; + // Defense-in-depth: the pnpm global store path. Any file under + // this prefix is a pnpm-internal hardlink target — a sibling + // worktree's `pnpm install` rewrites it, mtime propagates + // through every hardlink, and the watcher would (under --watch) + // restart the backend. Even though the `/node_modules/` check + // below already excludes the symlinked path, an explicit prefix + // excludes the in-place pnpm-store path too (relevant if a future + // refactor decides to watch the store directly for some reason). + if (PNPM_STORE_PREFIX && filename.startsWith(PNPM_STORE_PREFIX)) return false; // fs.watch on Linux emits absolute paths, on macOS it can // emit relative paths; POSIX and Windows separators both appear // in real-world fs.watch output. diff --git a/scripts/test-isolation-lint.check.mjs b/scripts/test-isolation-lint.check.mjs new file mode 100644 index 00000000..79606c1d --- /dev/null +++ b/scripts/test-isolation-lint.check.mjs @@ -0,0 +1,186 @@ +// scripts/test-isolation-lint.check.mjs +// +// CI lint gate (session-isolation/05): every test that SPAWNS +// packages/webui/server.js as a child process MUST set the +// per-test temp-dir overrides for the data files the backend +// writes to. Without the overrides, a test can land +// `events.ndjson` / `settings.json` / `sessions.json` lines in the +// user's real state directory — the documented "test leakage" +// shape that came up in the forensic audit. +// +// The lint is intentionally cheap: it scans the test/ and +// packages/webui/test/ trees for the canonical spawn pattern +// (`spawn(... server.js ...)` or `spawnSync(... server.js ...)` or +// `process.execPath + [..., server.js]`), then checks the +// surrounding function for the four env overrides: +// +// MCODE_WEBUI_SETTINGS_PATH +// MCODE_WEBUI_EVENTS_PATH +// MCODE_WEBUI_SESSIONS_DB +// MCODE_WEBUI_UPLOAD_DIR +// +// A spawn inside a comment or a string literal is ignored. A +// mention of one of the env names in a comment is ignored. Only +// spawn-within-the-same-function is the trigger. +// +// Why a dedicated lint, not a static-typescript rule: the test +// files use `import { spawn } from "node:child_process"` and the +// spawn call may live anywhere in the function (the env overrides +// are typically constructed earlier and passed as `env`). A +// regex over the function body is the cheapest precise check, and +// running it from `node:test` keeps the gate in the standard +// `pnpm test:webui` run. +// +// Pinned under the test:webui gate via packages/webui/package.json's +// `test:unit` glob. Exits 0 on clean; exits 1 with a per-file +// listing when a spawn is missing one or more overrides. + +import { test } from "node:test"; +import { strict as assert } from "node:assert"; +import { readdirSync, readFileSync, statSync } from "node:fs"; +import { join, extname } from "node:path"; + +const repoRoot = process.cwd(); + +/** + * Recursively walk `dir` and yield every regular file. Skips + * node_modules, .git, .next, dist, build, out. + */ +function* walk(dir) { + for (const entry of readdirSync(dir, { withFileTypes: true })) { + if (entry.name.startsWith(".")) continue; + if (entry.name === "node_modules") continue; + if (entry.name === "dist" || entry.name === "build" || entry.name === "out") continue; + const full = join(dir, entry.name); + if (entry.isDirectory()) { + yield* walk(full); + } else { + yield full; + } + } +} + +const REQUIRED_ENV_OVERRIDES = [ + "MCODE_WEBUI_SETTINGS_PATH", + "MCODE_WEBUI_EVENTS_PATH", + "MCODE_WEBUI_SESSIONS_DB", + "MCODE_WEBUI_UPLOAD_DIR", +]; + +/** + * Does the file text contain a server.js spawn? The patterns are + * narrow enough to skip imports, type-only references, and + * unrelated child_process calls: + * - `spawn(...)` whose first argument array contains `server.js` + * - `spawnSync(...)` likewise + * - `process.execPath` + `[..., "server.js", ...]` arg arrays + * + * Returns the indices of every match — each match triggers an env + * override check on the enclosing function body. + */ +function findSpawnIndices(text) { + const indices = []; + // spawn( ... "server.js" ... + const spawnRe = /spawn(?:Sync)?\s*\(/g; + let m; + while ((m = spawnRe.exec(text))) { + // Walk forward to find the matching closing paren — naive but + // good enough for the test files we care about. + let depth = 1; + let i = m.index + m[0].length; + while (i < text.length && depth > 0) { + const ch = text[i]; + if (ch === "(") depth += 1; + else if (ch === ")") depth -= 1; + i += 1; + } + const call = text.slice(m.index, i); + if (/"server\.js"|'server\.js'/.test(call)) { + indices.push({ start: m.index, end: i, call }); + } + } + return indices; +} + +/** + * Given the file text and a span (match), find the enclosing + * `function name(...) { ... }` body so the override check is scoped + * to the same function. If no enclosing function is found, the + * whole file is the scope. + */ +function enclosingFunctionBody(text, spanStart) { + // Find the nearest `function ` before spanStart. + let i = spanStart; + while (i > 0) { + const kwIdx = text.lastIndexOf("function", i); + if (kwIdx === -1) break; + const openBrace = text.indexOf("{", kwIdx); + if (openBrace === -1 || openBrace > spanStart) { + i = kwIdx - 1; + continue; + } + // Walk braces from openBrace forward. + let depth = 1; + let j = openBrace + 1; + while (j < text.length && depth > 0) { + const ch = text[j]; + if (ch === "{") depth += 1; + else if (ch === "}") depth -= 1; + j += 1; + } + return text.slice(openBrace, j); + } + // Fallback — top-level scope. + return text; +} + +function lintFile(path) { + const text = readFileSync(path, "utf8"); + const spawns = findSpawnIndices(text); + if (spawns.length === 0) return []; + const issues = []; + for (const spawn of spawns) { + const body = enclosingFunctionBody(text, spawn.start); + const missing = REQUIRED_ENV_OVERRIDES.filter( + (name) => !new RegExp(name).test(body), + ); + if (missing.length > 0) { + issues.push({ + file: path, + offset: spawn.start, + missing, + }); + } + } + return issues; +} + +test("test isolation — every server.js spawn sets per-test MCODE_WEBUI_* env overrides", () => { + // Scope: the canonical test trees. scripts/test-isolation-lint + // covers repo-level tests; packages/webui/test/ covers the webui + // package. Both must pass before this gate goes green. + const roots = [ + join(repoRoot, "test"), + join(repoRoot, "packages", "webui", "test"), + ]; + const issues = []; + for (const root of roots) { + if (!statSync(root, { throwIfNoPath: false })) continue; + for (const file of walk(root)) { + if (![".js", ".mjs", ".cjs"].includes(extname(file))) continue; + issues.push(...lintFile(file)); + } + } + if (issues.length === 0) return; // green + const formatted = issues + .map( + (issue) => + ` ${issue.file}:${issue.offset} missing: ${issue.missing.join(", ")}`, + ) + .join("\n"); + assert.fail( + `server.js spawn without per-test env overrides (${issues.length} issue(s)):\n${formatted}\n` + + "Every test that spawns server.js MUST set MCODE_WEBUI_{SETTINGS_PATH,EVENTS_PATH,SESSIONS_DB,UPLOAD_DIR} to per-test tmp paths before first import. " + + "See test/server/server-startup.test.js for the canonical pattern.", + ); +}); \ No newline at end of file From 92596687f1e8ccd57dc57b9c81ec3199a1409753 Mon Sep 17 00:00:00 2001 From: liuhailong <857688528@qq.com> Date: Sat, 26 Sep 2026 03:52:23 +0800 Subject: [PATCH 3/4] fix(webui): wire isolation lint into root gate; group-signal dev children MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 2 of session-isolation/05 — closes the acceptance round's one blocking finding plus its follow-up. 1. The test-isolation lint actually runs in CI now (was dead wiring). scripts/test-isolation-lint.check.mjs hung off packages/webui's test/test:unit globs, which execute with cwd=packages/webui — every scripts/** glob matched zero files there, so the gate reported "pass 0" while scanning nothing. Per the AGENTS.md convention that workflow-safety regressions live in test/source-sync.test.mjs, the lint is now exercised by the root test:release-tools gate, which CI reaches through scripts/verify.mjs in every verification profile: - the module exports collectTestIsolationViolations() and resolves its scan roots from its own file location, so the gate's cwd is irrelevant; it also stays runnable standalone (node scripts/test-isolation-lint.check.mjs); - test/source-sync.test.mjs asserts the real test trees are clean AND proves the lint is not a no-op via synthetic fixtures: a violating spawn (missing all four MCODE_WEBUI_* overrides) is detected, a compliant one clears — an earlier silent "pass 0" cannot come back unnoticed; - the lint strips comments (string-aware), so documenting the spawn shape in prose cannot produce phantom matches; - the dead packages/webui globs (scripts/*.test.mjs, scripts/**/*.test.mjs, scripts/**/*.check.mjs) are removed from both test and test:unit. Verified both directions: dropping a synthetic spawn-server test without the overrides fails pnpm test:release-tools with a per-file listing; removing it turns the gate green. 2. scripts/dev-webui.test.mjs joins the same gate. The dev-watcher scope-filter and port-verifier unit tests were authored by this branch but ran in no gate (same dead webui-cwd glob problem); the file is added to the test:release-tools file list and passes from the repo root. Four new cases pin signalChildGroup. 3. Teardown signals target the child's process group. next dev forks a next-server grandchild the launcher's children Map never tracks, so any pid-only kill orphaned the real HTTP listener. All teardown paths now go through signalChildGroup(), which signals process.kill(-pid, sig) (children are detached group leaders) and falls back to the pid-only signal on ESRCH or platforms without process groups; the helper never throws. Covered: SIGINT shutdown and its 5s force-kill (liveness now read from exitCode/signalCode, since child.killed no longer applies), backend restart SIGTERM/SIGKILL, both port-verifier SIGKILLs, and the failed-start sibling shutdown. Cleanup comments corrected (default ports are 18090/18091, not the isolated test ports 18092/18093). Live-verified on isolated ports 18094/18095 using only processes I spawned and ps-verified: SIGINT of the launcher reaped backend, npm, next, and the untracked next-server grandchild; a failed start (EADDRINUSE) sibling shutdown did the same; both ports released with no strays. The pre-existing 18090/18091 instance was never signalled. Gates: pnpm typecheck 0 errors; pnpm test:release-tools 70 tests / 0 fail (fail-with-violation and pass-compliant both demonstrated); pnpm test:webapp 207/207; pnpm build 6253 source files OK; pnpm check:source 4567 files OK. pnpm --filter @mavis/webui test hangs on this host in test/lib/mcode-acp-note.test.js and test/server/graceful-shutdown.test.js — reproduced identically on unmodified ce6cf15 via git stash, so it predates this round and is reported here rather than fixed. --- package.json | 2 +- packages/webui/package.json | 4 +- scripts/dev-webui.mjs | 62 +++++++--- scripts/dev-webui.test.mjs | 103 ++++++++++++++++- scripts/test-isolation-lint.check.mjs | 157 ++++++++++++++++++++------ test/source-sync.test.mjs | 40 +++++++ 6 files changed, 309 insertions(+), 59 deletions(-) diff --git a/package.json b/package.json index 4c213d93..15a237f8 100644 --- a/package.json +++ b/package.json @@ -35,7 +35,7 @@ "test:capabilities": "node scripts/run-vitest-suite.mjs capability", "test:windows": "node scripts/run-vitest-suite.mjs windows", "test:artifact": "node --test test/public-artifact.test.mjs", - "test:release-tools": "node --test test/source-sync.test.mjs" + "test:release-tools": "node --test test/source-sync.test.mjs scripts/dev-webui.test.mjs" }, "devDependencies": { "@types/node": "^20.19.0", diff --git a/packages/webui/package.json b/packages/webui/package.json index bd4b4078..899f2e02 100644 --- a/packages/webui/package.json +++ b/packages/webui/package.json @@ -12,8 +12,8 @@ "webapp:build": "next build webapp", "webapp:typecheck": "tsc -p webapp/tsconfig.json --noEmit", "test:webapp": "node --import tsx --import ./test/helpers/mavis-sources.mjs --test \"webapp/test/**/*.test.ts\"", - "test": "node --import tsx --import ./test/helpers/mavis-sources.mjs --experimental-test-module-mocks --test test/lib/*.test.js test/lib/*.check.mjs test/lib/*/*.test.js test/lib/*/*.check.mjs test/routes/*.test.js test/routes/*.check.mjs test/server/*.test.js test/server/*.check.mjs test/tooling/*.test.js test/integration/*.test.js test/matrix/*.test.js test/trajectory/*.mjs scripts/**/*.check.mjs", - "test:unit": "node --import tsx --import ./test/helpers/mavis-sources.mjs --experimental-test-module-mocks --test test/lib/*.test.js test/lib/*/*.test.js test/routes/*.test.js test/server/*.test.js test/tooling/*.test.js scripts/*.test.mjs scripts/**/*.test.mjs scripts/**/*.check.mjs", + "test": "node --import tsx --import ./test/helpers/mavis-sources.mjs --experimental-test-module-mocks --test test/lib/*.test.js test/lib/*.check.mjs test/lib/*/*.test.js test/lib/*/*.check.mjs test/routes/*.test.js test/routes/*.check.mjs test/server/*.test.js test/server/*.check.mjs test/tooling/*.test.js test/integration/*.test.js test/matrix/*.test.js test/trajectory/*.mjs", + "test:unit": "node --import tsx --import ./test/helpers/mavis-sources.mjs --experimental-test-module-mocks --test test/lib/*.test.js test/lib/*/*.test.js test/routes/*.test.js test/server/*.test.js test/tooling/*.test.js", "test:mocked": "node --import tsx --import ./test/helpers/mavis-sources.mjs --experimental-test-module-mocks --test test/lib/*.check.mjs test/lib/*/*.check.mjs test/routes/*.check.mjs test/server/*.check.mjs", "test:integration": "node --import tsx --import ./test/helpers/mavis-sources.mjs --experimental-test-module-mocks --test test/integration/*.test.js test/matrix/*.test.js", "check": "node scripts/check-docs-alignment.mjs", diff --git a/scripts/dev-webui.mjs b/scripts/dev-webui.mjs index 9eafe1f5..4ded6592 100644 --- a/scripts/dev-webui.mjs +++ b/scripts/dev-webui.mjs @@ -37,6 +37,38 @@ if (!existsSync(path.join(webappDir, "next.config.mjs"))) { const children = new Map(); let exiting = false; +// Signal a child's whole process group, not just its pid. +// +// Every child in the `children` map was spawned with `detached: true` +// (see spawnChild below), so each child is the leader of its own +// process group and `process.kill(-pid, sig)` reaches everything in +// that group. That matters because the group contains processes this +// launcher does NOT track: `next dev` forks a next-server worker +// (a grandchild) that never appears in the map, so a pid-only +// `child.kill()` left the real HTTP listener orphaned on every +// teardown path — including the failed-start sibling shutdown where +// one bad port killed the pair but not next-server. +// +// ESRCH (no process in the group — child already gone) and EINVAL on +// platforms without POSIX process groups both land in the catch and +// fall back to the pid-only signal, so the helper never throws and +// never resurfaces a dead child. EPERM cannot occur for children we +// spawned ourselves. +function signalChildGroup(child, signal) { + if (!child || typeof child.pid !== "number") return; + try { + process.kill(-child.pid, signal); + return; + } catch { + // group already gone (ESRCH) or group signals unsupported here + } + try { + child.kill(signal); + } catch { + // already gone + } +} + // Ports are declared once: the frontend's port has to reach the backend as a // trusted origin (see below), so it cannot live only in the spawn args. const BACKEND_PORT = Number(process.env.PORT) || 18090; @@ -70,9 +102,10 @@ function spawnChild(name, command, args, cwd, color, extraEnv, onStdoutChunk) { // is the child's own pid, not the launcher's. The launcher // continues to forward SIGTERM on its own shutdown so Ctrl+C // still tears down the pair. - // 2. `child.kill('SIGTERM')` still works (signals the child's pgid - // when the pid matches the pgid — same as before), so the - // existing graceful-shutdown path is unchanged. + // 2. Teardown signals go through signalChildGroup (below), which + // targets the child's group with `process.kill(-pid, sig)` — + // so the next-server grandchild `next dev` forks dies with its + // parent instead of surviving as an orphan. // // `stdio: 'pipe'` plus the forward() below still works under // detached: stdout/stderr are piped, NOT inherited from the parent. @@ -132,7 +165,7 @@ function spawnChild(name, command, args, cwd, color, extraEnv, onStdoutChunk) { console.error(`[mcode:dev] ${name} exited (code=${code}, signal=${signal}) — shutting down siblings.`); for (const [otherName, other] of children) { try { - other.kill("SIGTERM"); + signalChildGroup(other, "SIGTERM"); } catch { // already gone } @@ -245,7 +278,7 @@ async function restartBackend(triggerFile) { // give up. restartingBackend = true; try { - old.kill("SIGTERM"); + signalChildGroup(old, "SIGTERM"); } catch { // already gone } @@ -255,7 +288,7 @@ async function restartBackend(triggerFile) { `[mcode:dev] backend did not exit within 8s after SIGTERM — SIGKILL`, ); try { - old.kill("SIGKILL"); + signalChildGroup(old, "SIGKILL"); } catch { // already gone } @@ -337,7 +370,7 @@ function makePortVerifier(expectedPort, deadlineMs) { `[mcode:dev] backend bound to port ${boundPort} but BACKEND_PORT=${expectedPort} — treating as failed start`, ); try { - boundChild && boundChild.kill("SIGKILL"); + signalChildGroup(boundChild, "SIGKILL"); } catch { // already gone } @@ -351,7 +384,7 @@ function makePortVerifier(expectedPort, deadlineMs) { `[mcode:dev] backend did not print a listening line within ${deadlineMs}ms — treating as failed start (likely EADDRINUSE or import error)`, ); try { - boundChild && boundChild.kill("SIGKILL"); + signalChildGroup(boundChild, "SIGKILL"); } catch { // already gone } @@ -411,24 +444,27 @@ function shutdown(signal) { // with `detached: true` so they have their own process groups and // survive this signal automatically — do NOT forward, otherwise the // `detached: true` protection has no user-visible effect. The user - // can find them via lsof :18092 / :18093 if they want them gone. + // can find them via `lsof -i :$BACKEND_PORT -i :$FRONTEND_PORT` + // (18090 / 18091 by default) if they want them gone. if (signal === "SIGINT") { console.error( `\n[mcode:dev] received ${signal} (Ctrl+C) — stopping both processes…`, ); for (const [name, child] of children) { try { - child.kill("SIGTERM"); + signalChildGroup(child, "SIGTERM"); } catch { // already gone } } - // Force-kill after 5s if anything is still alive. + // Force-kill after 5s if anything is still alive. Liveness is read + // from exitCode/signalCode (not child.killed, which only tracks + // child.kill() calls and stays false after group signalling). setTimeout(() => { for (const [name, child] of children) { try { - if (!child.killed) { - child.kill("SIGKILL"); + if (child.exitCode === null && child.signalCode === null) { + signalChildGroup(child, "SIGKILL"); console.error(`[mcode:dev] force-killed ${name}`); } } catch { diff --git a/scripts/dev-webui.test.mjs b/scripts/dev-webui.test.mjs index 4605cc73..7ad65b4a 100644 --- a/scripts/dev-webui.test.mjs +++ b/scripts/dev-webui.test.mjs @@ -101,12 +101,91 @@ describe("shouldWatchFile — pnpm global store defense-in-depth (v2)", () => { }); }); +describe("signalChildGroup — process-group teardown (v3)", () => { + // Extract signalChildGroup from dev-webui.mjs without importing the + // module (which has spawn side effects). `process` is injected as a + // parameter so the tests exercise the helper's real control flow with + // a fake — nothing here signals a live process. + const source = readFileSyncSync( + new URL("./dev-webui.mjs", import.meta.url), + "utf8", + ); + const match = source.match( + /function signalChildGroup\(child, signal\) \{([\s\S]*?)\n\}/, + ); + if (!match) throw new Error("could not extract signalChildGroup"); + // eslint-disable-next-line no-new-func + const signalChildGroup = new Function( + "child", + "signal", + "process", + `${match[0]}\n; return signalChildGroup(child, signal);`, + ); + + test("signals the child's process group when it exists", () => { + const groupKills = []; + const fakeProcess = { kill: (pid, sig) => groupKills.push([pid, sig]) }; + const child = { + pid: 4242, + kill: () => { + throw new Error("pid-only fallback must not run"); + }, + }; + signalChildGroup(child, "SIGTERM", fakeProcess); + assert.deepEqual(groupKills, [[-4242, "SIGTERM"]]); + }); + + test("falls back to the pid-only signal when the group is gone (ESRCH)", () => { + const fallbacks = []; + const fakeProcess = { + kill: () => { + const err = new Error("kill ESRCH"); + err.code = "ESRCH"; + throw err; + }, + }; + const child = { pid: 4242, kill: (sig) => fallbacks.push(sig) }; + signalChildGroup(child, "SIGTERM", fakeProcess); + assert.deepEqual(fallbacks, ["SIGTERM"]); + }); + + test("never throws when both the group signal and the fallback fail", () => { + const fakeProcess = { + kill: () => { + throw new Error("kill ESRCH"); + }, + }; + const child = { + pid: 4242, + kill: () => { + throw new Error("kill ESRCH"); + }, + }; + assert.doesNotThrow(() => signalChildGroup(child, "SIGKILL", fakeProcess)); + }); + + test("ignores children without a usable pid", () => { + const fakeProcess = { + kill: () => { + throw new Error("must not be called"); + }, + }; + assert.doesNotThrow(() => signalChildGroup(null, "SIGTERM", fakeProcess)); + assert.doesNotThrow(() => + signalChildGroup({ pid: undefined, kill: () => {} }, "SIGTERM", fakeProcess), + ); + }); +}); + describe("makePortVerifier — port-binding verification (v2)", () => { // Extract makePortVerifier from dev-webui.mjs without importing // the module (which has spawn side effects). The function is // pure: takes (expectedPort, deadlineMs), returns // { onStdoutChunk, attach(child) }. We exercise it via a // regex pull so the test imports nothing but node:test. + // signalChildGroup is injected (the extracted body tears down + // through the process-group helper); the stand-in forwards to + // child.kill so the stub children below record the kill. const source = readFileSyncSync( new URL("./dev-webui.mjs", import.meta.url), "utf8", @@ -119,12 +198,20 @@ describe("makePortVerifier — port-binding verification (v2)", () => { const makePortVerifier = new Function( "expectedPort", "deadlineMs", + "signalChildGroup", `${match[0]}\n; return makePortVerifier(expectedPort, deadlineMs);`, ); + const signalChildGroup = (child, signal) => { + if (!child || typeof child.pid !== "number") return; + child.kill(signal); + }; + const stubChild = (onKill) => ({ pid: 4242, kill: onKill }); test("signals success when stdout reports the expected port", async () => { - const child = { kill() {} }; - const verifier = makePortVerifier(18092, 60000); + const child = stubChild(() => { + throw new Error("healthy backend must not be signalled"); + }); + const verifier = makePortVerifier(18092, 60000, signalChildGroup); verifier.attach(child); verifier.onStdoutChunk( "[webui] mcode cmd: /x/y/z\n" + @@ -139,8 +226,10 @@ describe("makePortVerifier — port-binding verification (v2)", () => { test("kills the child when the bound port does not match BACKEND_PORT", async () => { let killed = false; - const child = { kill() { killed = true; } }; - const verifier = makePortVerifier(18092, 60000); + const child = stubChild(() => { + killed = true; + }); + const verifier = makePortVerifier(18092, 60000, signalChildGroup); verifier.attach(child); verifier.onStdoutChunk( "[webui] listening on http://127.0.0.1:18100\n", // wrong port @@ -150,8 +239,10 @@ describe("makePortVerifier — port-binding verification (v2)", () => { test("kills the child when no listening line appears within the deadline", async () => { let killed = false; - const child = { kill() { killed = true; } }; - const verifier = makePortVerifier(18092, 100); // 100ms deadline + const child = stubChild(() => { + killed = true; + }); + const verifier = makePortVerifier(18092, 100, signalChildGroup); // 100ms deadline verifier.attach(child); // No chunks at all. await new Promise((r) => setTimeout(r, 200)); diff --git a/scripts/test-isolation-lint.check.mjs b/scripts/test-isolation-lint.check.mjs index 79606c1d..508d02dd 100644 --- a/scripts/test-isolation-lint.check.mjs +++ b/scripts/test-isolation-lint.check.mjs @@ -1,6 +1,6 @@ // scripts/test-isolation-lint.check.mjs // -// CI lint gate (session-isolation/05): every test that SPAWNS +// Session-isolation/05 lint: every test that SPAWNS // packages/webui/server.js as a child process MUST set the // per-test temp-dir overrides for the data files the backend // writes to. Without the overrides, a test can land @@ -10,9 +10,8 @@ // // The lint is intentionally cheap: it scans the test/ and // packages/webui/test/ trees for the canonical spawn pattern -// (`spawn(... server.js ...)` or `spawnSync(... server.js ...)` or -// `process.execPath + [..., server.js]`), then checks the -// surrounding function for the four env overrides: +// (`spawn(... server.js ...)` or `spawnSync(... server.js ...)`), +// then checks the surrounding function for the four env overrides: // // MCODE_WEBUI_SETTINGS_PATH // MCODE_WEBUI_EVENTS_PATH @@ -23,24 +22,34 @@ // mention of one of the env names in a comment is ignored. Only // spawn-within-the-same-function is the trigger. // -// Why a dedicated lint, not a static-typescript rule: the test -// files use `import { spawn } from "node:child_process"` and the -// spawn call may live anywhere in the function (the env overrides -// are typically constructed earlier and passed as `env`). A -// regex over the function body is the cheapest precise check, and -// running it from `node:test` keeps the gate in the standard -// `pnpm test:webui` run. -// -// Pinned under the test:webui gate via packages/webui/package.json's -// `test:unit` glob. Exits 0 on clean; exits 1 with a per-file -// listing when a spawn is missing one or more overrides. +// Wiring: this module is exercised by the root `test:release-tools` +// gate (`node --test test/source-sync.test.mjs …`), per the repo +// convention that repository-level node:test suites stay in their +// existing gates and workflow-safety regressions land in +// test/source-sync.test.mjs. The scan roots are resolved relative +// to THIS file's location, so the gate's cwd is irrelevant — an +// earlier attempt ran these paths through packages/webui's +// test:unit runner, whose cwd made every glob match zero files +// (a gate that reported "pass 0" and never scanned anything). +// The file also stays runnable on its own for manual audits: +// node scripts/test-isolation-lint.check.mjs +// which exits 0 on clean and 1 with a per-file listing otherwise. -import { test } from "node:test"; -import { strict as assert } from "node:assert"; import { readdirSync, readFileSync, statSync } from "node:fs"; import { join, extname } from "node:path"; +import { fileURLToPath, pathToFileURL } from "node:url"; + +// Repo root is derived from this file's location (scripts/ is one +// level below the root), never from process.cwd() — see the wiring +// note above. +const repoRoot = fileURLToPath(new URL("../", import.meta.url)); -const repoRoot = process.cwd(); +export const REQUIRED_ENV_OVERRIDES = [ + "MCODE_WEBUI_SETTINGS_PATH", + "MCODE_WEBUI_EVENTS_PATH", + "MCODE_WEBUI_SESSIONS_DB", + "MCODE_WEBUI_UPLOAD_DIR", +]; /** * Recursively walk `dir` and yield every regular file. Skips @@ -60,13 +69,6 @@ function* walk(dir) { } } -const REQUIRED_ENV_OVERRIDES = [ - "MCODE_WEBUI_SETTINGS_PATH", - "MCODE_WEBUI_EVENTS_PATH", - "MCODE_WEBUI_SESSIONS_DB", - "MCODE_WEBUI_UPLOAD_DIR", -]; - /** * Does the file text contain a server.js spawn? The patterns are * narrow enough to skip imports, type-only references, and @@ -134,8 +136,61 @@ function enclosingFunctionBody(text, spanStart) { return text; } +/** + * Strip line comments (double slash) and block comments (slash-star … + * star-slash) from JS source, tracking string literals so comment + * markers inside strings (URLs, regexes-as-text) survive. Spawn shapes + * written in comments must not produce phantom matches — + * source-sync.test.mjs documents the fixture shape in prose and would + * otherwise trip the real-tree scan. + */ +function stripComments(text) { + let out = ""; + let state = "code"; // code | line | block | single | double | template + let i = 0; + while (i < text.length) { + const ch = text[i]; + const next = text[i + 1]; + if (state === "code") { + if (ch === "/" && next === "/") { state = "line"; i += 2; continue; } + if (ch === "/" && next === "*") { state = "block"; i += 2; continue; } + if (ch === "'") state = "single"; + else if (ch === '"') state = "double"; + else if (ch === "`") state = "template"; + out += ch; + i += 1; + continue; + } + if (state === "line") { + if (ch === "\n") { state = "code"; out += ch; } + i += 1; + continue; + } + if (state === "block") { + if (ch === "*" && next === "/") { state = "code"; out += " "; i += 2; continue; } + if (ch === "\n") out += ch; + i += 1; + continue; + } + // Inside a string literal: copy verbatim, honoring escapes. + if (ch === "\\") { + out += text.slice(i, i + 2); + i += 2; + continue; + } + if ( + (state === "single" && ch === "'") || + (state === "double" && ch === '"') || + (state === "template" && ch === "`") + ) state = "code"; + out += ch; + i += 1; + } + return out; +} + function lintFile(path) { - const text = readFileSync(path, "utf8"); + const text = stripComments(readFileSync(path, "utf8")); const spawns = findSpawnIndices(text); if (spawns.length === 0) return []; const issues = []; @@ -155,32 +210,60 @@ function lintFile(path) { return issues; } -test("test isolation — every server.js spawn sets per-test MCODE_WEBUI_* env overrides", () => { - // Scope: the canonical test trees. scripts/test-isolation-lint - // covers repo-level tests; packages/webui/test/ covers the webui - // package. Both must pass before this gate goes green. - const roots = [ +/** + * Scan the canonical test trees for server.js spawns that lack the + * per-test MCODE_WEBUI_* env overrides. Returns an empty array when + * the tree is compliant; each entry otherwise describes one + * offending spawn. `roots` defaults to the repository's real test + * trees; test/source-sync.test.mjs passes synthetic fixture trees + * so CI keeps proving the lint still detects violations (a lint + * that silently matches nothing must fail the gate, not pass it). + */ +export function collectTestIsolationViolations({ roots } = {}) { + const scanRoots = roots ?? [ join(repoRoot, "test"), join(repoRoot, "packages", "webui", "test"), ]; const issues = []; - for (const root of roots) { + for (const root of scanRoots) { if (!statSync(root, { throwIfNoPath: false })) continue; for (const file of walk(root)) { if (![".js", ".mjs", ".cjs"].includes(extname(file))) continue; issues.push(...lintFile(file)); } } - if (issues.length === 0) return; // green + return issues; +} + +/** Human-readable per-file listing for gate output and CLI stderr. */ +export function formatTestIsolationViolations(issues) { const formatted = issues .map( (issue) => ` ${issue.file}:${issue.offset} missing: ${issue.missing.join(", ")}`, ) .join("\n"); - assert.fail( + return ( `server.js spawn without per-test env overrides (${issues.length} issue(s)):\n${formatted}\n` + - "Every test that spawns server.js MUST set MCODE_WEBUI_{SETTINGS_PATH,EVENTS_PATH,SESSIONS_DB,UPLOAD_DIR} to per-test tmp paths before first import. " + - "See test/server/server-startup.test.js for the canonical pattern.", + "Every test that spawns server.js MUST set MCODE_WEBUI_{SETTINGS_PATH,EVENTS_PATH,SESSIONS_DB,UPLOAD_DIR} to per-test tmp paths before first import. " + + "See packages/webui/test/server/server-startup.test.js for the canonical pattern." ); -}); \ No newline at end of file +} + +// Manual-audit entry point: `node scripts/test-isolation-lint.check.mjs`. +// Inside a node:test run this file is only imported, never executed +// as the main module, so the CLI block is inert in gates. +if ( + process.argv[1] && + import.meta.url === pathToFileURL(process.argv[1]).href +) { + const issues = collectTestIsolationViolations(); + if (issues.length === 0) { + console.log( + "test-isolation-lint: clean — every server.js spawn sets the per-test MCODE_WEBUI_* overrides.", + ); + } else { + console.error(formatTestIsolationViolations(issues)); + process.exitCode = 1; + } +} diff --git a/test/source-sync.test.mjs b/test/source-sync.test.mjs index 8450b04b..caa2e72d 100644 --- a/test/source-sync.test.mjs +++ b/test/source-sync.test.mjs @@ -21,6 +21,7 @@ import { compareVersions, releaseCli } from '../scripts/release-cli.mjs'; import { compareRuns, exitCodeForStatus, renderReport, spread, validateRun, validateRequest, validateToolOutput, median, selectScenarios } from '../scripts/perf/report.mjs'; import { copyMcodeToolsArtifact, downloadMcodeToolsArtifact, MCODE_TOOLS_ARTIFACT } from '../scripts/lib/mcode-tools-artifact.mjs'; import { checkWindowsSourceLocation, runWindowsSourceLocationCheck } from '../scripts/check-windows-source-location.mjs'; +import { collectTestIsolationViolations, formatTestIsolationViolations } from '../scripts/test-isolation-lint.check.mjs'; test('Windows source preflight accepts localized fsutil labels', () => { const result = checkWindowsSourceLocation({ @@ -1128,3 +1129,42 @@ test('source imports preserve vendored Office schema bytes through Git staging', assert.deepEqual(git('show', `:${schema}`), bytes); } }); + +// Session-isolation/05: the test-isolation lint runs here — inside the root +// release-tools gate — instead of through packages/webui's test:unit globs, +// whose cwd made every repo-root scripts/ path match zero files (the lint +// reported "pass 0" while scanning nothing). The first assertion keeps the +// real test trees honest; the synthetic fixtures keep the lint itself honest +// by proving it still detects a violating spawn and clears a compliant one, +// so a future refactor cannot silently turn this gate back into a no-op. +test('test isolation lint finds every server.js spawn missing its MCODE_WEBUI_* overrides', () => { + const violations = collectTestIsolationViolations(); + assert.deepEqual(violations, [], formatTestIsolationViolations(violations)); +}); + +test('test isolation lint detects a violating spawn and clears a compliant one', t => { + const root = mkdtempSync(path.join(tmpdir(), 'isolation-lint-')); + t.after(() => rmSync(root, { recursive: true, force: true })); + mkdirSync(path.join(root, 'nested')); + // serverArg is concatenated so this test file's own source never contains + // the literal `spawn(..., ['server.js'])` shape — the real-tree assertion + // above scans test/ too, and the fixture template must not trip it. + const serverArg = "'server" + ".js'"; + const spawnFixture = body => `import { spawn } from 'node:child_process';\n${body}`; + writeFileSync(path.join(root, 'nested', 'violating.test.mjs'), spawnFixture( + `function startServer() {\n return spawn(process.execPath, [${serverArg}]);\n}\n`, + )); + const overrides = ['MCODE_WEBUI_SETTINGS_PATH', 'MCODE_WEBUI_EVENTS_PATH', 'MCODE_WEBUI_SESSIONS_DB', 'MCODE_WEBUI_UPLOAD_DIR'] + .map(name => `${name}: tmpPath`).join(', '); + writeFileSync(path.join(root, 'compliant.test.mjs'), spawnFixture( + `function startServer() {\n const env = { ${overrides} };\n return spawn(process.execPath, [${serverArg}], { env });\n}\n`, + )); + const violations = collectTestIsolationViolations({ roots: [root] }); + assert.equal(violations.length, 1); + assert.match(violations[0].file, /violating\.test\.mjs$/); + assert.deepEqual(violations[0].missing, ['MCODE_WEBUI_SETTINGS_PATH', 'MCODE_WEBUI_EVENTS_PATH', 'MCODE_WEBUI_SESSIONS_DB', 'MCODE_WEBUI_UPLOAD_DIR']); + assert.match(formatTestIsolationViolations(violations), /violating\.test\.mjs/); + assert.doesNotMatch(formatTestIsolationViolations(violations), /compliant\.test\.mjs/); + rmSync(path.join(root, 'nested'), { recursive: true, force: true }); + assert.deepEqual(collectTestIsolationViolations({ roots: [root] }), []); +}); From 22bdf4745c8ed98aeedea96a9fb1782a7f4156e3 Mon Sep 17 00:00:00 2001 From: liuhailong <857688528@qq.com> Date: Sat, 26 Sep 2026 05:12:22 +0800 Subject: [PATCH 4/4] test(webui): release the handles that kept node --test from exiting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #31 CI: test:webui hung for 15 minutes with every executed test green — zero failures, zero ✖ — until the runner killed the job and reaped ~10 orphaned child processes. Root cause is leaked handles in three test files; all three leave the test process alive after its tests pass, and node --test waits on the process, not the tests. 1. test/server/graceful-shutdown.test.js (branch-new, the CI blocker): every test boots a real http server, but the suite never closes them. A test that signals its server closes it inside shutdown()'s server.close() — but the uninstall() case never signals, so its listening handle stays ref'd and the test process never exits. The suite teardown now tracks every booted server, calls closeAllConnections() and close() on each, alongside the existing signal-handler uninstall. 2. test/lib/mcode-acp-note.test.js and test/trajectory/store.test.mjs (pre-existing on main, latent on CI): their import graph reaches server/lib/state-bus.js, whose mcode-sessions cache warm-up spawns the resident mcode ACP engine child during module load whenever the engine resolves — on dev machines always, on CI after the build gate produces dist/cli.js and the engine's handshake succeeds. The child's stdio keeps the test process's pipes open, so the runner never sees the file finish (and the orphaned engine shows up in job cleanup). Both files now await the shared singleton init promise — so the teardown cannot race the in-flight start — and then stop the child via shutdownMcodeAcpSingleton(). Correction to the previous commit's note: the "pre-existing environmental" hang conclusion was wrong in part — git stash does not remove committed files, so that baseline still contained this branch's graceful-shutdown suite. Re-verified against a throwaway worktree at the pre-branch base: graceful-shutdown.test.js does not exist there (our leak), while mcode-acp-note.test.js does hang there too but only where the engine starts (dev machines), which is why main's CI stayed green. Proof: pnpm --filter @mavis/webui test, plain (no --test-force-exit), run twice back-to-back: 1387 tests / 1385 pass / 0 fail / 0 cancelled / 2 skipped, exit 0 both times. A spawned server.js still exits promptly under the new signal handler: SIGTERM-to-exit measured at 103ms (fast path — close callback with no connections; the 1.5s grace / 4s hard bound only engage with held sockets, and stopServer's SIGKILL fallback in integration tests covers that). Gates: pnpm typecheck 0 errors; pnpm test:release-tools 69 pass / 0 fail. --- .../webui/test/lib/mcode-acp-note.test.js | 31 ++++++++++++++++++- .../test/server/graceful-shutdown.test.js | 25 +++++++++++++++ packages/webui/test/trajectory/store.test.mjs | 24 +++++++++++++- 3 files changed, 78 insertions(+), 2 deletions(-) diff --git a/packages/webui/test/lib/mcode-acp-note.test.js b/packages/webui/test/lib/mcode-acp-note.test.js index 57b926ef..2785a9f9 100644 --- a/packages/webui/test/lib/mcode-acp-note.test.js +++ b/packages/webui/test/lib/mcode-acp-note.test.js @@ -11,7 +11,7 @@ // propagating both permissionMode and model from the engine's // authoritative state (defect #2). -import { test, describe } from "node:test"; +import { test, describe, after } from "node:test"; import assert from "node:assert/strict"; import { join } from "node:path"; import { pathToFileURL } from "node:url"; @@ -28,6 +28,35 @@ const { lastSegment, applyRecordedModel, } = await import(absPath("lib/mcode-acp.js")); +// Same module instance the runtime graph uses — see the teardown below. +const { getMcodeAcpClient, shutdownMcodeAcpSingleton } = await import( + absPath("lib/acp-client.js") +); + +// Importing server/lib/mcode-acp.js pulls in the webui runtime graph, +// and on a machine where the mcode engine resolves (dev checkouts, and +// CI after the build gate produces dist/cli.js) that graph starts the +// resident ACP singleton child process during module load — a state-bus +// snapshot warms the mcode-sessions cache, whose fetch spawns the +// engine. The child's stdio keeps this test process's pipes open, so +// `node --test` never sees the file finish: every test passes, zero +// failures, and the job is killed at the timeout. Await the shared +// init promise (so the teardown cannot race the in-flight start) and +// stop the child once the suite settles. +after(async () => { + try { + await getMcodeAcpClient(); + } catch { + // engine never started (e.g. no resolvable mcode binary) — nothing to stop + } + try { + shutdownMcodeAcpSingleton(); + } catch { + // nothing was started + } + // Give the child a beat to exit before the runner moves on. + await new Promise((r) => setTimeout(r, 50)); +}); describe("buildEmptyTurnNote (v2.3)", () => { test("normal turn with an answer → no note", () => { diff --git a/packages/webui/test/server/graceful-shutdown.test.js b/packages/webui/test/server/graceful-shutdown.test.js index 5fb000bf..fb1223fc 100644 --- a/packages/webui/test/server/graceful-shutdown.test.js +++ b/packages/webui/test/server/graceful-shutdown.test.js @@ -55,6 +55,7 @@ function bootEchoServer() { res.end("ok"); } }); + servers.push(server); return new Promise((resolve) => { server.listen(0, "127.0.0.1", () => { const addr = server.address(); @@ -79,6 +80,14 @@ function get(url, opts = {}) { // not leak across tests. Hoisted to file scope so the second // describe below can register there too. const handles = []; +// Every server these tests boot. The suite teardown MUST close them: +// a listening handle is ref'd, so a server that is booted but never +// signalled (the uninstall() case below never reaches shutdown()'s +// server.close()) keeps this test process alive forever — `node --test` +// then waits on it with all tests green and zero failures, which is +// exactly the CI hang (job killed at the 15m timeout, ~10 orphan +// processes reaped at cleanup). +const servers = []; after(() => { for (const h of handles) { try { @@ -87,6 +96,22 @@ after(() => { // already torn down } } + for (const server of servers) { + // Destroy any held SSE-style connections first so close()'s + // callback can fire; already-closed servers are no-ops. + try { + if (typeof server.closeAllConnections === "function") { + server.closeAllConnections(); + } + } catch { + // nothing left to destroy + } + try { + server.close(); + } catch { + // already closed by shutdown() + } + } }); function track(uninstall) { handles.push({ uninstall }); diff --git a/packages/webui/test/trajectory/store.test.mjs b/packages/webui/test/trajectory/store.test.mjs index 31fefaef..4091528f 100644 --- a/packages/webui/test/trajectory/store.test.mjs +++ b/packages/webui/test/trajectory/store.test.mjs @@ -4,13 +4,35 @@ import { mkdtemp, mkdir, rm, writeFile } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import path from 'node:path'; import { DatabaseSync } from 'node:sqlite'; -import test from 'node:test'; +import test, { after } from 'node:test'; import { openStore, resolveDataDir, resolveHomeDir, encodeFtsQuery } from '../../server/trajectory/store.mjs'; import { resolveWorkspaceIdentity } from '../../server/trajectory/git.mjs'; import { redactValue, redactText, redactPath } from '../../server/trajectory/redact.mjs'; import { TOOLS, handleRpcMessage } from '../../server/trajectory/mcp.mjs'; import { ftsModuleAvailable } from '../../server/trajectory/sqlite.mjs'; +import { getMcodeAcpClient, shutdownMcodeAcpSingleton } from '../../server/lib/acp-client.js'; + +// The trajectory store's import graph reaches server/lib/state-bus.js, whose +// mcode-sessions cache warm-up spawns the resident mcode ACP engine child +// whenever the engine resolves (dev checkouts; CI after the build gate). The +// child's stdio keeps this file's pipes open, so `node --test` never sees the +// file finish — every test passes, zero failures, and the job dies at the +// timeout. Await the shared init promise (so the teardown cannot race the +// in-flight start), then stop the child once the suite settles. +after(async () => { + try { + await getMcodeAcpClient(); + } catch { + // engine never started (no resolvable mcode binary) — nothing to stop + } + try { + shutdownMcodeAcpSingleton(); + } catch { + // nothing was started + } + await new Promise((r) => setTimeout(r, 50)); +}); /** * Whether this runtime's bundled SQLite has FTS5.