Skip to content

fix(FR-3214): kill the whole dev process group on shutdown so leaked watch orphans don't exhaust file descriptors#8156

Open
nowgnuesLee wants to merge 2 commits into
mainfrom
06-26-fix_fr-3214_exclude_pnpm_store_from_type-check_watcher_so_multi-repo_dev_doesn_t_exhaust_file_descriptors
Open

fix(FR-3214): kill the whole dev process group on shutdown so leaked watch orphans don't exhaust file descriptors#8156
nowgnuesLee wants to merge 2 commits into
mainfrom
06-26-fix_fr-3214_exclude_pnpm_store_from_type-check_watcher_so_multi-repo_dev_doesn_t_exhaust_file_descriptors

Conversation

@nowgnuesLee

@nowgnuesLee nowgnuesLee commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Resolves #8047 (FR-3214)

Problem

Running several repos' dev servers at once fails with EMFILE: too many open files. A major driver turned out to be leaked orphan watch processes accumulating across dev restarts: on a live machine, ps showed relay watch chains from dev sessions started 3 and 8 days earlier still alive with PPID 1nodemon → pnpm run relay --watch → relay-compiler cli.js → native relay --watch binary — each holding its full set of file-watcher FDs forever.

Root cause

dev.mjs forwarded SIGINT/SIGTERM to one PID only (child.kill(sig) on the concurrently process), and concurrently --kill-others likewise signals only its direct children. The relay watch tree is unusually deep (concurrently → pnpm → nodemon → pnpm → node cli.js → native relay binary), and per-PID signal relay breaks at the nodemon hop — killing the direct child orphans everything below it.

The leak fires on every shutdown path that doesn't broadcast to the terminal's foreground process group:

  • IDE/agent task stop (signals the top PID only) — the common case when dev runs under VS Code / code-server / an agent session
  • concurrently --kill-others after one child dies
  • graceful-shutdown chains that hang (a deep child swallowing SIGINT while its parents wait), which leaks even under a real terminal Ctrl+C
  • double Ctrl+C interrupting a cleanup in progress

Fix

scripts/dev.mjs now owns the whole tree's lifecycle:

  1. Spawn concurrently as a detached process-group leader (detached: true, non-Windows). Every descendant — however deep — inherits the group.
  2. Signal the group, not the PID. SIGINT/SIGTERM/SIGHUP handlers forward via process.kill(-child.pid, sig), so every member receives the signal directly, with no reliance on intermediate layers relaying it. (SIGHUP is now forwarded explicitly because a detached group no longer receives the terminal's hangup.)
  3. Unconditional kill guarantee. Two backstops ensure nothing outlives dev.mjs:
    • On any forwarded signal, a 5s timer escalates the whole group to SIGKILL if graceful shutdown hangs.
    • On concurrently exit (covers the --kill-others path, where dev.mjs itself never received a signal), dev.mjs probes the group (kill(-pgid, 0)), SIGTERMs survivors, waits up to 5s, then SIGKILLs the remainder before exiting.

Windows falls back to direct child.kill (negative-PID kill throws there); macOS/Linux — where the leak was observed — get the full group semantics.

This PR previously took a different approach (excluding the pnpm store from vite-plugin-checker's TS watch program via a generated tsconfig.checker.json). That reduced per-process FDs but left the actual leak — orphaned watch trees — unfixed; it has been reverted in favor of this root-cause fix.

Measurements (live, this machine)

Scenario 1 — single-PID SIGTERM to dev.mjs (the IDE-stop leak path):

before fix after fix
group members while running 11 (tsc, vite, esbuild, portless, nodemon, pnpm×3, relay cli, native relay) 11
survivors 8s after kill -TERM <dev.mjs pid> relay chain leaked (observed as day-old PPID 1 zombies) 0

Scenario 2 — kill -9 one child (tsc) to trigger concurrently --kill-others:

  • All 11 group members gone within 10s, dev.mjs exited with the child's status, log shows the normal --kill-others SIGTERM cascade. 0 survivors.

Verification

  • bash scripts/verify.sh=== ALL PASS === (Relay / Lint / Format / TypeScript).
  • node --check scripts/dev.mjs passes.
  • Both live shutdown scenarios above measured on a real dev boot of this branch (ps ax -o pid,ppid,pgid group-membership check before/after).

🤖 Generated with Claude Code

@github-actions github-actions Bot added the size:M 30~100 LoC label Jul 6, 2026

nowgnuesLee commented Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • flow:merge-queue - adds this PR to the back of the merge queue
  • flow:hotfix - for urgent changes, fast-track this PR to the front of the merge queue

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has required the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@nowgnuesLee
nowgnuesLee marked this pull request as ready for review July 6, 2026 04:36
Copilot AI review requested due to automatic review settings July 6, 2026 04:36

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes EMFILE: too many open files failures that occur when running several repositories' Vite dev servers concurrently. The root cause (measured via lsof) is that vite-plugin-checker's TypeScript watch program installs a per-file watcher on every file in the program — including thousands of immutable dependency .d.ts files in the pnpm global store — consuming ~3,800 file descriptors per repo. The fix tells the checker's TS watch program not to watch the store (while still reading it for type-checking) via watchOptions.excludeDirectories/excludeFiles.

Because vite-plugin-checker 0.9.3 has no inline watchOptions API and the exclude spec must be an absolute, machine-specific store path (TS anchors **/-globs under the project basePath), the config is generated at dev startup into a gitignored react/tsconfig.checker.json that extends the base tsconfig.json, and the checker is pointed at it. It gracefully falls back to the base tsconfig for vite build, npm/yarn flat installs, or any write failure.

Changes:

  • Added resolveCheckerTsconfigPath() in react/vite.config.ts that (in serve mode) writes a derived tsconfig.checker.json extending the base config with watchOptions excludes for the resolved pnpm store path, and points vite-plugin-checker at it.
  • Preserved original behavior via graceful fallback to the committed base tsconfig when no store path is available or the write fails.
  • Added the derived react/tsconfig.checker.json to .gitignore.

Reviewed changes

Copilot reviewed 1 out of 2 changed files in this pull request and generated no comments.

File Description
react/vite.config.ts Adds resolveCheckerTsconfigPath() helper, computes checkerTsconfigPath in serve mode, and wires it into the checker() plugin; updates surrounding comments.
.gitignore Ignores the machine-specific derived react/tsconfig.checker.json generated at dev startup.

Notes on correctness reviewed: The derived file is written to react/tsconfig.checker.json and extends: './tsconfig.json' resolves to react/tsconfig.json (same directory), so inherited include/exclude/paths remain correct. The store path is normalized to forward slashes for cross-platform JSON validity, console.warn matches the existing pattern in this file (resolvePnpmStorePath), one-shot tsc/verify.sh/CI are unaffected (they use the base tsconfig), and build mode passes undefined so no file is written. No blocking defects were found.

…o dev doesn't exhaust file descriptors

A single vite dev process held ~5,900 open files, ~3,800 of them immutable
pnpm-store dependency .d.ts watched by vite-plugin-checker's TS watch program
(per-file kqueue, 1 FD/file, because FSEvents is failing on macOS). Running
several repos' dev servers at once multiplied that toward the kernel/launchd
open-file limit and failed with EMFILE ("too many watched files").

Generate a gitignored react/tsconfig.checker.json at dev startup that extends
the base tsconfig and adds watchOptions excluding the (machine-specific,
absolute) pnpm store root, then point vite-plugin-checker at it during serve.
The exclude only stops *watching* the store, not *reading* it, so type-checking
stays complete and source files keep their normal fast per-file watch.

Real-stack boot test: vite REG open files 5,884 -> 1,404 (-76%), files held
open under the pnpm store 3,825 -> 2, checker still 'Found 0 errors'.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@nowgnuesLee
nowgnuesLee force-pushed the 06-26-fix_fr-3214_exclude_pnpm_store_from_type-check_watcher_so_multi-repo_dev_doesn_t_exhaust_file_descriptors branch from f053174 to ea7ed7d Compare July 23, 2026 07:43
@nowgnuesLee nowgnuesLee changed the title fix(FR-3214): exclude pnpm store from type-check watcher so multi-repo dev doesn't exhaust file descriptors fix(FR-3214): kill the whole dev process group on shutdown so leaked watch orphans don't exhaust file descriptors Jul 23, 2026
…watch orphans don't exhaust file descriptors

Replaces the previous pnpm-store watch-exclude approach: revert the
tsconfig.checker.json generation in react/vite.config.ts and instead fix
the root leak — dev.mjs now spawns concurrently as a detached process
group leader, forwards SIGINT/SIGTERM/SIGHUP to the whole group via
negative-PID kill, sweeps surviving orphans on child exit, and escalates
to SIGKILL after a 5s grace period so nothing in the watch tree can
outlive dev.mjs.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Ar6Rr2uEGLPBXcWNpASXDD
@nowgnuesLee
nowgnuesLee force-pushed the 06-26-fix_fr-3214_exclude_pnpm_store_from_type-check_watcher_so_multi-repo_dev_doesn_t_exhaust_file_descriptors branch from ea7ed7d to 555dee0 Compare July 23, 2026 07:57
@github-actions

Copy link
Copy Markdown
Contributor

Coverage Report for root-coverage

Status Category Percentage Covered / Total
🔵 Lines 8.3% 28 / 337
🔵 Statements 9.14% 32 / 350
🔵 Functions 11.53% 6 / 52
🔵 Branches 9.09% 18 / 198
File CoverageNo changed files found.
Generated in workflow #3122 for commit 555dee0 by the Vitest Coverage Report Action

@agatha197 agatha197 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Solid fix that targets the real root cause — dev.mjs not owning the whole watch tree. Making concurrently a detached process-group leader and signaling -pgid with a SIGKILL escalation cleanly covers the --kill-others, SIGHUP, and Windows-degrade paths. node --check passes, and the remaining points are all edge cases outside real-world usage, so this is good to merge.

Before merging, please just document these two residual risks in the PR description / a comment:

1. If dev.mjs is SIGKILLed or crashes, the group is orphaned (not a regression).
The whole fix assumes dev.mjs receives a catchable signal (TERM/INT/HUP) and lives long enough to run the sweep. Because the group is now detached, it's decoupled from dev.mjs's death and the terminal hangup — so a supervisor that sends SIGKILL (uncatchable) to dev.mjs skips the sweep and reproduces exactly this leak. Most IDEs/agents send SIGTERM, so real usage is covered, but this is the one hole left open.

2. The setsid escape assumption is implicit.
The fix relies entirely on every descendant staying in the concurrently group. If anything in the chain calls setsid() / spawns its own detached group, -pgid can't reach it and it still leaks. The current toolchain (concurrently/pnpm/nodemon/relay/vite/tsc) doesn't do this, so it holds today, but it's a hidden assumption that could silently reintroduce the leak on a future tooling change — worth a one-line comment.

FYI (no change needed):

  • The exit handler signals -child.pid after the leader has already been reaped, so there's a theoretical PID/PGID reuse race, but the window is microtask-sized and negligible.
  • The missing child.on('error') handler (when the concurrently binary is absent) is a pre-existing gap, not a regression.
  • Verified and fine: escalation ??= single-flight, no 5s wait on normal shutdown, and SIGKILL releasing FDs immediately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Dev server fails with too-many-watched-files when running multiple repos concurrently

3 participants