fix(dev-tools): dev-server incident chain — graceful shutdown, watch scope, process-group isolation, test-isolation lint - #31
Merged
Conversation
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.
…ion lint 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 -- -<launcher-pgid>` 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://…:<port>" 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 <pid>` 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-pgid>: 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.
…dren
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.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Closes the complete dev-server incident chain established by independent forensics (3 wedges + 2 whole-instance deaths in one day). Root-cause chain: any worktree's
pnpm installrewrites pnpm-global-store hardlinks (shared inode across 6 worktrees) →node --watchmisfires → SIGTERM →server.close()never returns (SSE sockets hold it) → wedge; operator pattern/process-group kills then take down the whole PGID (launcher + backend + next-server); respawn during a wedge hits pinned-port EADDRINUSE leaving twonode server.jswith neither listening.server/lib/graceful-shutdown.js): per-socket tracking, 1.5s grace force-destroy, 4s hardprocess.exitbound, idempotent SIGINT+SIGTERM — a hang can never wedge the watcher again.scripts/lib/dev-watch-scope.mjs): in-processfs.watchwith a pinned include filter replacingnode --watch; node_modules/.next/dist/webapp/third_party/pnpm-store excluded — the original trigger is gone.detached: truechildren with own PGIDs; external group kills no longer cascade (live-verified); SIGINT still forwards for Ctrl+C;signalChildGroup()reaps next-server grandchildren on every teardown path (ESRCH-safe).listening on ...:<port>line; mismatch/timeout = failed start, no dual-process states.planned_restartmarkers (exact sender needs kernel auditd — documented).scripts/test-isolation-lint.check.mjs): tests spawning server.js must setMCODE_WEBUI_{SETTINGS_PATH,EVENTS_PATH,SESSIONS_DB,UPLOAD_DIR}tmp overrides — wired into the roottest:release-toolsgate (CI-everywhere) with synthetic-fixture tests pinning both directions; dead globs removed; the also-gate-deadscripts/dev-webui.test.mjsre-wired.Acceptance (3 rounds, independent agent)
Rounds 1–2 FAIL→fix: lint wiring was dead (globs resolved from the wrong cwd — "pass 0"); next-server grandchildren survived teardown. Round 3 PASS: wiring proven end-to-end against the real tree (synthetic violation → gate exit 1; removal → pass); process-group teardown live-verified (grandchild reaped); the external-group-kill immunity re-verified. Incident evidence included a real orchestrator pattern-kill that the attribution log correctly recorded (
signal=SIGKILL planned_restart=false).Gates: typecheck 0 ·
test:release-tools70/70 ·test:webapp207/207 · build ✓ · check:source ✓. Known follow-up:graceful-shutdown.test.jsleaves a pending handle (test hygiene; suites pass with--test-force-exit; functionality unaffected).Full
pnpm verifydeferred to CI.