Repository navigation
fix(manager): stop the design watcher from holding a process open on Linux - #666
Conversation
…Linux With forceExit gone from the test driver (#661), tests/unit/manager-design-routes.test.ts hung its child on ubuntu-latest for the full 180s stall bound: all five tests passed and the process never exited. The same file finishes in 98ms on macOS. startDesignWatcher() relied on watcher.unref() after fs.watch(root, { recursive: true }), with a comment promising it would "never keep a process alive". That is only true on macOS and Windows. Linux has no native recursive watch, so node routes it through lib/internal/fs/recursive_watch.js (lib/fs.js: "if (options.recursive && !isMacOS && !isWindows)"), and that implementation's unref() walks #files looking for StatWatcher entries - #files holds Stats, and the real per-directory handles live in #watchers, which it never touches. A new one is added for every directory that appears, which this suite does on every POST /pages. Node only fixed that on main (nodejs/node#65486); v22 and v24 both have the no-op. persistent is forwarded to every inner watcher and to the native path, so { recursive: true, persistent: false } expresses the intent on all three platforms. The watcher keeps working in the manager server, which stays alive for its listening socket. The test also stops the watcher it starts, and a new case spawns a child that starts the watcher, creates a directory under it after the start, and asserts the child exits by itself. That case fails against the old code on Linux and passes either way on macOS, which is why it belongs in CI rather than a local run.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
What broke
Removing
forceExitfrom the test driver (#661, merged as 2be76dd) exposed a hang it had been hiding. Onubuntu-latest,tests/unit/manager-design-routes.test.tspasses all five of its tests and then its child never exits — the driver's new watchdog reported it precisely:The same file finishes in 98ms on macOS.
Why
startDesignWatcher()calledfs.watch(root, { recursive: true })and thenwatcher.unref(), with a comment promising it would "never keep a process alive just for the watcher (tests, CLI)". That promise only holds on macOS and Windows.Linux has no native recursive watch, so node routes it through a JS implementation:
and that implementation's
unref()walks the wrong map:#filesholdsStats; the live per-directory handles live in#watchersand are never touched.StatWatcheris imported and never constructed there, so the call is a no-op. Worse, a fresh ref'ed handle is added for every directory that appears after the start — and this suite creates one perPOST /pages. Node fixed it onmainonly (nodejs/node#65486, see also #53350); v22 and v24 both ship the no-op, and CI runs Node 22.The change
{ recursive: true, persistent: false }.persistentis forwarded to every inner watcher (watch(file, { persistent: this.#options.persistent })) and to the native macOS/Windows path, so it expresses "do not hold the loop open" on all three platforms, at construction time rather than after. The watcher still works in the manager server, which stays alive for its listening socket.tests/unit/manager-design-routes.test.tsalso stops the watcher it starts, and gains a case that spawns a child which starts the watcher, creates a directory under it after the start, and asserts the child exits on its own. That case fails against the old code on Linux and passes either way on macOS, so CI is where it earns its keep.Validation
npm run typecheckclean,npm run gate:all23/23.root,unitscope locally: 13008 tests, 12988 pass, 0 fail.test 2/4shard is the proof.