perf(reaper): bound /proc scan concurrency at startup - #395
Conversation
Both orphan reapers walked /proc with an unbounded Promise.all over every pid, holding a pending promise plus a read buffer per process while the libuv threadpool retired four at a time. On a host with ~1600 processes that peak was ~200 MB, and it stayed resident for the whole process lifetime because V8 does not hand grown arenas back to the OS. Capping reads at 32 in flight cuts the two reapers' overhead from 203 MB to 32 MB and is faster (254 ms vs 377 ms); a freshly started stdio server drops from 287 MB to 190 MB RSS. Signed-off-by: Finomosec <1665799+Finomosec@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
… + pin with test A rejection does not leave the remaining items unprocessed: Promise.all rejects early but cancels nothing, so only the failing worker stops and the surviving workers keep draining items in the background after the returned promise has rejected. Moot for the reapers (their callback never throws) but the helper is generic and the old comment would mislead the next caller. New test pins the actual behavior. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
debugmcpdev
left a comment
There was a problem hiding this comment.
Thank you — this is a model performance PR: honest measurements, alternatives weighed and rejected for stated reasons, and a real bug caught by its own tests (Array.from({length: NaN})). And beyond the fix itself, your observation unlocked a rich seam: it prompted a full memory-footprint audit of the server, which produced a repeatable benchmark harness (PR to follow) and seven tracked follow-ups (#399–#405). Much appreciated.
Independent verification
I wanted the claims reproducible in a controlled environment before merging, so I built scripts/mem-bench.mjs (separate PR): it spawns N sleep infinity processes inside the docker image before exec'ing the server (so exactly N+4 /proc entries exist when the reapers run, node as PID 1), connects a real MCP client, and samples /proc/1/status after initialize — which is a guaranteed barrier for reaper completion, since both reapers are awaited before argv parsing. 5 trials per variant, control built from this PR's base (a4d5737d) so the images differ by exactly this diff:
| scenario | variant | metric | base (median) | this PR (median) | Δ |
|---|---|---|---|---|---|
| busy-proc (1504 procs) | busy | RSS MB | 136.3 (136.1..137.4) | 106.8 (106.5..107.2) | −29.5 (−21.6%) ✅ |
| busy-proc (1504 procs) | busy | peak (VmHWM) MB | 136.3 | 106.8 | −29.5 ✅ |
| busy-proc (4 procs) | quiet | RSS MB | 98.2 (97.9..98.7) | 97.9 (97.7..98.2) | within noise ✅ |
Confirmed on all three axes: the scan scar shrinks by ~78% (+38 MB → +8.6 MB over quiet), VmHWM drops identically — the spike is prevented, not released after the fact (exactly your V8-arena argument), and the empty-/proc case doesn't regress. The absolute delta is smaller than on your host because synthetic sleepers have 15-byte cmdlines; the mechanism is the same.
What I pushed to your branch (maintainer edits)
docs(bounded-concurrency): the helper's comment said a rejection "leaves the remaining items unprocessed" — actually only the failing worker stops; the surviving workers keep draining all remaining items in the background after the returned promise has rejected (Promise.allrejects early but cancels nothing). Moot for the reapers, but the helper is generic, so I corrected the comment and added a test pinning the real semantics.- Merged
main(branch protection requires up-to-date branches).
One narrative correction (no code change)
The note that "the reapers run concurrently with server construction, so those peaks overlap" isn't quite right — they're awaited at the top of main() before createCLI()/parseAsync(), so they run for every invocation (--version included) and complete before server construction. That, your sync-scan follow-up (57 MB / 25 ms), and sharing a single /proc walk between the two reapers are now tracked in #399.
Merging once CI is green on the updated head.
…dvisory workflow) (#406) * feat(bench): repeatable memory-footprint benchmark (scripts/mem-bench.mjs) Measures server RSS at lifecycle checkpoints (after-initialize = reaper barrier, after-tools-list, session-cycle retention) across targets (dist / npx bundle / docker), with a synthesized busy-/proc docker scenario (N sleeper processes spawned before exec, server as PID 1) to exercise the startup orphan reapers under a realistic process count. 5-trial median + min..max sampling, JSON output, and a compare subcommand whose significance test is median-shift vs full spread. Advisory weekly/dispatch CI workflow (never a required check). Motivated by PR #395's finding that unbounded /proc scans leave a permanent RSS scar; this makes such claims measurable and repeatable. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(bench): note that stored git info describes the bench-runner checkout Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: JF <john.franklin@gmail.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
📋 Description
The two startup orphan reapers walk
/procwith an unboundedPromise.allover every pid:On a busy host that means one pending promise plus a read buffer for every process at once, while the libuv threadpool retires only four filesystem requests at a time. Both reapers do this independently and run concurrently from
main(), so the cost is paid twice.I found this while profiling why a freshly started stdio server sits at ~287 MB RSS before serving a single request. Measured on Linux with ~1600 processes:
mainThe peak is not retained data —
heapUseddrops back to 4 MB right after the scan — but it stays resident for the entire process lifetime, because V8 does not return a grown arena to the OS. An explicitglobal.gc()does not move RSS at all. A 380 ms burst therefore leaves a permanent ~200 MB scar on every server instance, which matters for a stdio server that a client may start once per session.Capping the reads at 32 in flight is also faster (254 ms vs 377 ms):
/procreads are served synchronously from kernel memory, so there is nothing to overlap — the extra 1500 queued requests only add scheduling overhead.Isolated comparison of the scan alone, same host:
readFile, unbounded (current)readFile, 32 in flight (this PR)Net effect on a freshly started stdio server: 287 MB → 190 MB RSS.
No existing issue — happy to open one if you prefer that for tracking.
Notes on alternatives I measured and rejected
/proc/<pid>/comm(only scannode/java) cuts the scan from 1588 files to 72 but saves no memory (70 MB vs 69 MB) and is slower, because it needs two passes. Worse, it is incorrect: a node process whose main thread has been renamed reportscomm=MainThread, so the filter would skip exactly the orphans the reaper exists to collect. The cost was never in the number of files.openSync/readSyncwith one recycled buffer is the cheapest option measured (57 MB, 25 ms) but changes the I/O model and would require rewriting thefs/promisesmocks in the existing reaper tests. Happy to follow up with that separately if you want it.🔄 Type of Change
✅ Checklist
[Unreleased])🧪 Testing
New
forEachBoundedhelper insrc/utils/bounded-concurrency.tswith unit tests covering the in-flight bound, index/coverage guarantees, degenerate limits, and error propagation. One of those tests caught a real bug during development:Array.from({ length: NaN })yields an empty array, so a non-finitelimitwould have silently processed nothing — non-finite limits now fall back to sequential.Both reapers get a regression test asserting that a 500-pid
/procnever exceedsPROC_SCAN_CONCURRENCYconcurrent reads.Full suite: 3687 passed / 208 files. Two Python integration tests fail on my machine (
python_debug_workflow.test.ts,debugpy not installed for /usr/bin/python3) — I verified they fail identically on unmodifiedmain, so they are environmental and unrelated to this change.Test Configuration:
📝 Additional Notes
PROC_SCAN_CONCURRENCY = 32lives in its own tiny module so both reapers and both regression tests share one value. 32 is comfortably above the libuv default threadpool width of 4, so the pool never idles.The remaining gap between the reapers' 88 MB and the server's 190 MB is a separate matter — the reapers run concurrently with server construction, so those peaks overlap. Out of scope here.