fix(dev-tools): dev-server incident chain — graceful shutdown, watch scope, process-group isolation, test-isolation lint

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",

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...`)
+  },
 })

packages/webui/server/lib/graceful-shutdown.js

@@ -0,0 +1,186 @@
+// 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);
+    // 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.
+    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,
+};

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", () => {