Merge external-oauth into main: memory-leak fixes + resource-stats logging#49
Merged
Conversation
…vers Logs a resource snapshot every RESOURCE_STATS_INTERVAL_MS (default 60s, 0 disables): rss/heap/cpu for the mcp-api process itself, one aggregated line per child process subtree (installed stdio MCP servers labeled by server name, other children such as the Puppeteer Chromium tree labeled by command, with descendants like renderer processes rolled up), and a machine-facing total. Sampling uses a single `ps` invocation per tick (works on the node:20 Debian image and macOS). Stdio server pids are read through one isolated, runtime-validated accessor since @modelcontextprotocol/sdk 1.13.0 exposes no public pid on StdioClientTransport. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…btools stop The MCPService kept an in-memory `logs: string[]` per shared MCP server and per per-user connection and appended to it on every child-process stderr line and every transport error with no cap anywhere. Because installed servers such as mcp-msq emit two or more stderr lines per tool call, these arrays grew for the entire life of the process, leaking memory unboundedly. Introduce a module-scoped `appendServerLog` helper (and `MAX_SERVER_LOG_LINES = 500`) that pushes a line and, when the array exceeds the cap, splices off the oldest entries so only the most recent 500 lines remain. The cap is by COUNT ONLY: there is no time-based/TTL eviction, so logs read days later still survive until displaced by newer lines. All three push sites in src/services/mcp.ts (shared-server transport-error handler, stdio stderr handler, and per-user-connection transport-error handler) now route through the helper. The helper is exported so it can be unit-tested. Also fix BuiltInSearxngServer.stop(), which was a no-op containing only comments and never released the Puppeteer-managed Chromium browser on shutdown. It now awaits scraper.closeBrowser() inside the existing try/catch and clears the scraper reference and ready flag. Bump the package version from 1.11.8 to 1.11.9. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ak fix) puppeteer-scraper 1.1.2 fixes scrapePage leaking an open Chromium tab and a pages Map entry on every failed scrape. The Docker build installs from yarn.lock, which pinned 1.1.1, so without this bump the published fix would never reach production. Lockfile entry updated to the 1.1.2 tarball (dependencies are unchanged between 1.1.1 and 1.1.2). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Periodic resource-stats logging for mcp-api and spawned MCP servers
fix: cap in-memory MCP server logs at 500 lines; close Chromium on webtools stop
Brings the memory-leak fixes and resource-stats logging into main so CI runs: - Periodic [resource-stats] logging for mcp-api and spawned MCP servers (PR #47) - Cap in-memory MCP server logs at last 500 lines, count-only (PR #48) - Close Chromium on webtools stop(); require puppeteer-scraper ^1.1.2 (fixes the per-scrape Chromium-tab leak) Only conflict was package.json version: main had advanced to 1.11.11 while external-oauth carried 1.11.9; resolved to 1.11.12 (one above main, superseding both). src/services/mcp.ts and yarn.lock auto-merged cleanly. Verified on the merged tree: tsc --noEmit clean; full jest suite 170/170 across 8 suites (both main-only and external-oauth-only suites green). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
|
Automated review 🤖 Summary of ChangesThis PR brings in operational fixes aimed at reducing memory growth and improving observability: capped in-memory MCP server logs, periodic process/resource stats logging, and explicit Puppeteer browser shutdown in the built-in Searxng server. The overall direction is good and aligned with the stated OOM/tab-leak concerns, but there is one notable performance concern in the log-cap implementation under sustained stderr/error volume. Key Changes & Positives
Potential Issues & Recommendations
Language/Framework Checks
Security & Privacy
Build/CI & Ops
Tests
Approval RecommendationApprove with caveats
|
There was a problem hiding this comment.
Pull request overview
Merges the external-oauth branch work into main to bring CI coverage onto recent reliability fixes: capped in-memory MCP server logs, periodic process resource-stats logging, and Puppeteer/Chromium cleanup improvements.
Changes:
- Added periodic
[resource-stats]logging for the API process and its spawned child-process subtrees (configurable viaRESOURCE_STATS_INTERVAL_MS). - Capped per-server and per-connection in-memory log buffers to the last 500 lines to prevent unbounded growth.
- Improved Puppeteer scraper shutdown behavior and bumped
@missionsquad/puppeteer-scraperto^1.1.2.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| yarn.lock | Updates lockfile for @missionsquad/puppeteer-scraper ^1.1.2. |
| package.json | Bumps package version to 1.11.12 and updates scraper dependency to ^1.1.2. |
| src/builtin-servers/servers/searxng.ts | Ensures Puppeteer browser is closed on server stop and resets scraper state. |
| src/env.ts | Adds RESOURCE_STATS_INTERVAL_MS env parsing with default and disable behavior. |
| src/index.ts | Wires up ResourceStatsService as a managed resource during API init. |
| src/services/mcp.ts | Introduces appendServerLog + cap constant, routes log pushes through it, and exposes stdio child PIDs for stats labeling. |
| src/services/resourceStats.ts | Implements periodic resource stats sampling/logging service. |
| src/utils/resourceStats.ts | Adds ps sampling + parsing and subtree aggregation utilities. |
| test/resource-stats.spec.ts | Adds unit tests for ps parsing, subtree aggregation, pid accessor behavior, and a live sampling sanity check. |
| test/server-logs-cap.spec.ts | Adds tests ensuring the MCP log buffer cap behavior is correct and stable. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+922
to
+925
| /** | ||
| * Maximum number of in-memory log lines retained per MCP server and per | ||
| * per-user connection. Older lines are displaced once this count is exceeded. | ||
| */ |
Copilot and the automated reviewer flagged several items on PR #49: - resourceStats: switch `ps -eo ... comm=` to `command=` and derive a concise, argument-free label via new `shortenProcessLabel()` (executable basename, plus script basename for language runtimes). `comm` only yields the truncated executable name, making sibling `node` processes indistinguishable; dropping args also prevents any command-line values from reaching the logs. - resourceStats service: use `import type { Resource }` — Resource is a type-only export, so a value import risked a runtime circular import with ../index. - mcp.ts: reword MAX_SERVER_LOG_LINES JSDoc to remove the duplicated "per". - env: parse RESOURCE_STATS_INTERVAL_MS via `parseIntervalMs`, defaulting to 60000 on unset/blank/NaN so a typo no longer silently disables sampling; an explicit 0 still disables. - Dockerfile: install `procps` so `ps` is guaranteed in the runtime image (the sampler already warns-once and falls back to self-stats without it). - Tests: add `shortenProcessLabel` coverage (incl. arg non-leakage) and a sustained-volume (100k-append) stress test proving the log cap never exceeds 500 and retains the newest lines. Bumps version to 1.11.13. tsc --noEmit clean; full jest suite 177/177. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
|
Addressed the review feedback in b1200d0 (v1.11.13). Point by point: Copilot
Automated reviewer
|
Contributor
Author
|
Automated review 🤖 Summary of ChangesThis PR adds two operational safeguards: bounded in-memory MCP server logs to prevent unbounded growth, and periodic resource usage logging for the API process plus spawned child-process subtrees. It also updates Puppeteer scraper lifecycle handling to explicitly close the browser and bumps the scraper dependency to pick up an upstream Chromium tab leak fix. Key Changes & Positives
Potential Issues & Recommendations
Language/Framework Checks
Build/CI & Ops
Tests
Approval RecommendationApprove with caveats
|
| { | ||
| "name": "@missionsquad/mcp-api", | ||
| "version": "1.11.11", | ||
| "version": "1.11.13", |
Comment on lines
+118
to
+130
| export function shortenProcessLabel(command: string): string { | ||
| const trimmed = command.trim() | ||
| if (!trimmed) return 'unknown' | ||
| const tokens = trimmed.split(/\s+/) | ||
| const exe = basename(tokens[0]) | ||
| if (RUNTIME_EXECUTABLES.has(exe)) { | ||
| const scriptToken = tokens.slice(1).find((token) => !token.startsWith('-')) | ||
| if (scriptToken) { | ||
| return `${exe} ${basename(scriptToken)}` | ||
| } | ||
| } | ||
| return exe | ||
| } |
Comment on lines
+47
to
+53
| this.lastCpuUsage = process.cpuUsage() | ||
| this.lastCpuSampleAt = process.hrtime.bigint() | ||
| this.timer = setInterval(() => { | ||
| this.logSample().catch((error) => { | ||
| log({ level: 'debug', msg: '[resource-stats] sampling failed', error }) | ||
| }) | ||
| }, this.intervalMs) |
Comment on lines
99
to
103
| async stop(): Promise<void> { | ||
| if (this.scraper) { | ||
| try { | ||
| // Puppeteer cleanup would go here | ||
| // Note: PuppeteerScraper needs a cleanup method | ||
| await this.scraper.closeBrowser() | ||
| log({ level: 'info', msg: 'Stopped Puppeteer scraper' }) |
…-stop Three valid findings from Copilot's re-review of the fixed commit: - resourceStats: shortenProcessLabel() could still surface inline eval code as a "script" (e.g. `node -e <code>`, `python -c <code>`), leaking argument values. It now only appends a token that looks like a script path (contains a path separator or a known extension), so eval/print code and other arg values are never included in the label. - ResourceStatsService: guard the setInterval sampler with a `sampling` flag so a slow/blocked `ps` cannot cause overlapping samples to pile up (the tick is skipped while a previous sample is still running). - searxng: stop() now cancels any pending Puppeteer init-retry timer and sets a stopped flag; initializePuppeteerWithRetries() bails when stopped and closes a browser that finished initializing after stop() was called. Previously a scheduled retry could recreate (and leak) a browser after shutdown. Tests: add eval-flag / script-path coverage for shortenProcessLabel and a new searxng-lifecycle suite (mocked scraper) covering retry scheduling, retry cancellation on stop, and the mid-init close race. Bumps version to 1.11.14. tsc --noEmit clean; full jest suite 182/182 across 9 suites. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
|
Addressed round-2 feedback in f6c398d (v1.11.14):
|
Contributor
Author
|
Automated review 🤖 Summary of ChangesThis PR adds periodic resource-usage logging, caps in-memory MCP server/connection logs to prevent unbounded growth, and hardens the SearxNG/Puppeteer lifecycle to avoid browser/tab leaks during retries and shutdown. Overall, the changes are targeted at memory stability and observability, with appropriate dependency and container updates to support the new process-tree sampling. Key Changes & Positives
Potential Issues & Recommendations
Language/Framework Checks
Build/CI & Ops
Tests
Approval RecommendationApprove with caveats
|
Comment on lines
+131
to
+143
| export function shortenProcessLabel(command: string): string { | ||
| const trimmed = command.trim() | ||
| if (!trimmed) return 'unknown' | ||
| const tokens = trimmed.split(/\s+/) | ||
| const exe = basename(tokens[0]) | ||
| if (RUNTIME_EXECUTABLES.has(exe)) { | ||
| const scriptToken = tokens.slice(1).find((token) => !token.startsWith('-') && looksLikeScriptPath(token)) | ||
| if (scriptToken) { | ||
| return `${exe} ${basename(scriptToken)}` | ||
| } | ||
| } | ||
| return exe | ||
| } |
Comment on lines
+164
to
+166
| export function formatMebibytes(bytes: number): string { | ||
| return `${(bytes / (1024 * 1024)).toFixed(1)}MB` | ||
| } |
Comment on lines
+64
to
+67
| it('stays bounded and correct under sustained high-volume appends (memory does not grow)', () => { | ||
| // Mirrors the exact scenario the cap guards against: a chatty stdio server | ||
| // streaming stderr lines for the whole process lifetime. The array must never | ||
| // exceed the cap regardless of how many lines are pushed. |
Three valid findings from Copilot's re-review: - shortenProcessLabel(): parse flags and their values so a preloaded-module value (e.g. `node -r ts-node/register /app/index.js`) is skipped and the real script is selected, and so inline code after eval/print flags (`-e`/`--eval`/ `-p`/`--print`/`-c`) is never surfaced even when it contains a path separator or a script-like extension. Previously `-r ts-node/register` was logged as "node register" and eval code containing "/" could leak. - formatMebibytes(): the math is base-1024 (MiB) but the suffix rendered "MB". Now renders "MiB" so operators can compare against docker stats / top without a base-1000 vs base-1024 mismatch. - server-logs-cap stress test: renamed from "memory does not grow" (overclaimed — it does not measure bytes) to accurately state it asserts the array length never exceeds the cap. Added tests for `-r`/`--require`/`--import` value skipping and eval code containing slashes/extensions. Bumps version to 1.11.15. tsc --noEmit clean; full jest suite 184/184 across 9 suites. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
|
Addressed round-3 feedback in ea3167c (v1.11.15):
Added tests for |
Contributor
Author
|
Automated review 🤖 Summary of ChangesThis PR adds two production-facing safeguards: bounded in-memory server logs to prevent unbounded growth, and periodic resource-stat logging to make process/subprocess memory and CPU usage visible. It also tightens the SearxNG/Puppeteer lifecycle so shutdown and failed initialization paths actively close browsers and stop retry timers, which should materially reduce the OOM/tab-leak behavior described. Key Changes & Positives
Potential Issues & Recommendations
Language/Framework Checks
Security & Privacy
Tests
Approval RecommendationApprove with caveats
|
Register the resource-stats service at the front of the resources list (unshift) so shutDown(), which stops resources in order, stops sampling before the MCP/package services it samples begin tearing down. Previously it was stopped last, so a sampling tick during SIGTERM handling could spawn `ps` and log stale/incorrect stats while MCPController was mid-shutdown. Bumps version to 1.11.16. tsc --noEmit clean; full jest suite 184/184. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
|
Addressed round-4 feedback in 4972dcb (v1.11.16):
|
Contributor
Author
|
Automated review 🤖 Summary of ChangesThis PR merges the external-oauth branch work into main and primarily addresses two operational issues: unbounded in-memory log growth and Puppeteer/Chromium lifecycle leaks, while adding periodic process/resource visibility for the API and spawned child processes. Overall, the changes are directionally strong for memory stability and diagnosability, with one notable performance concern in the log-capping implementation under sustained high-volume logging. Key Changes & Positives
Potential Issues & Recommendations
Language/Framework Checks
Build/CI & Ops
Tests
Approval RecommendationApprove with caveats
|
Comment on lines
+70
to
+75
| public async stop(): Promise<void> { | ||
| if (this.timer) { | ||
| clearInterval(this.timer) | ||
| this.timer = undefined | ||
| } | ||
| } |
Round-4's shutdown-ordering fix stopped the service first but stop() only cleared the interval; a sample already in flight (launched by the interval callback, which does not await logSample) could still run `ps` and log during teardown. Track the in-flight sampling promise and await it in stop() so no sampling overlaps the teardown of the services it samples. Adds a ResourceStatsService suite (mocked sampleProcessTree) asserting stop() waits for an in-flight sample, resolves immediately when idle, and never samples when disabled. Bumps version to 1.11.17. tsc --noEmit clean; full jest suite 187/187 across 10 suites. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
|
Addressed round-5 feedback in 0205311 (v1.11.17):
Added a |
Contributor
Author
|
Automated review 🤖 Summary of ChangesThis PR adds periodic resource-usage logging for the API process and its child process subtrees, caps retained in-memory MCP logs to prevent unbounded growth, and hardens the SearxNG/Puppeteer lifecycle to avoid browser/tab leaks during retries and shutdown. Overall, the intent is solid and directly targets known memory/OOM failure modes. Key Changes & Positives
Potential Issues & Recommendations
Language/Framework Checks
Build/CI & Ops
Tests
Approval RecommendationApprove with caveats
|
|
|
||
| await this.scraper.init() | ||
|
|
||
| await scraper.init() |
| */ | ||
| export function sampleProcessTree(): Promise<ProcessSample[]> { | ||
| return new Promise((resolve, reject) => { | ||
| execFile('ps', ['-eo', 'pid=,ppid=,rss=,pcpu=,command='], { maxBuffer: 4 * 1024 * 1024 }, (error, stdout) => { |
|
|
||
| public async init(): Promise<void> { | ||
| if (!Number.isFinite(this.intervalMs) || this.intervalMs <= 0) { | ||
| log({ level: 'info', msg: '[resource-stats] disabled (RESOURCE_STATS_INTERVAL_MS <= 0)' }) |
… disable msg Three valid findings from Copilot's re-review: - searxng: initializePuppeteerWithRetries now hoists the scraper handle and, on init() failure, closes the (possibly partially-created) browser in the catch before retrying. Previously the handle was block-scoped to the try and relied entirely on PuppeteerScraper.init()'s internal cleanup; repeated failures could otherwise leak Chromium. closeBrowser() is a no-op if already cleaned up. - resourceStats: sampleProcessTree spawns `ps` with a 5s timeout so a wedged `ps` cannot stall sampling — and, since stop() awaits the in-flight sample, cannot hang shutdown. - ResourceStatsService: the disable log now reports the actual offending value and states the interval must be a positive number of ms (the guard also rejects non-finite values, not just <= 0). Tests: searxng closes the browser on init failure; ResourceStatsService is disabled for a non-finite interval. Bumps version to 1.11.18. tsc --noEmit clean; full jest suite 189/189 across 10 suites. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
|
Addressed round-6 feedback in bc56c94 (v1.11.18):
Added tests: searxng closes the browser on init failure; the service is disabled for a non-finite interval. |
Contributor
Author
|
Automated review 🤖 Summary of ChangesThis PR brings in three operational fixes: bounded in-memory MCP log retention, periodic resource-usage logging for the API and spawned child process trees, and a Puppeteer/Chromium lifecycle fix intended to stop browser/tab leaks. Overall, the changes are practical and well-tested, with the main impact being improved memory stability and observability in production. Key Changes & Positives
Potential Issues & Recommendations
Language/Framework Checks
Security & Privacy
Build/CI & Ops
Tests
Approval RecommendationApprove with caveats
|
Comment on lines
+3855
to
+3860
| for (const serverKey of Object.keys(this.servers)) { | ||
| const server = this.servers[serverKey] | ||
| if (server.transportType !== 'stdio') continue | ||
| const transport = server.connection?.transport | ||
| if (!transport) continue | ||
| const pid = getStdioTransportPid(transport) |
connection is not cleared on transport close, so a disconnected/error stdio server could contribute a stale pid to the resource-stats name map and mislabel an unrelated process if the OS reused that pid. Restrict the mapping to servers with status 'connected'. (ps sampling still counts every process; this only governs which subtrees get the friendly server name.) Bumps version to 1.11.19. tsc --noEmit clean; full jest suite 189/189. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
|
Addressed round-7 feedback in bad88d7 (v1.11.19):
|
Contributor
Author
|
Automated review 🤖 Summary of ChangesThis PR merges in operational hardening around browser/process lifecycle management and memory visibility: it closes Puppeteer browsers on shutdown/init failure, adds periodic process/resource logging, and caps retained in-memory server logs. Overall this should reduce OOM risk and improve diagnosis, with the main runtime change being a new background sampler that shells out to Key Changes & Positives
Potential Issues & Recommendations
Language/Framework Checks
Security & Privacy
Build/CI & Ops
Tests
Approval RecommendationApprove with caveats
|
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
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.
Summary
Merges
external-oauthintomainso CI runs on this work (the underlying fixes were previously merged only intoexternal-oauthvia #47 and #48, which don't trigger main's CI). Brings in:[resource-stats]lines for the mcp-api process, each spawned MCP server subtree, and a machine total.RESOURCE_STATS_INTERVAL_MS(default 60000;0disables).logscapped at the last 500 lines, count-only.webtoolsstop()closes the browser; dependency bumped to@missionsquad/puppeteer-scraper ^1.1.2(fixes the per-scrape Chromium-tab leak — the primary machine-OOM cause).Conflict resolution
Only conflict was the
package.jsonversion (mainat1.11.11vsexternal-oauth1.11.9); merge resolved to1.11.12, review-fix commits bumped to1.11.19.src/services/mcp.tsandyarn.lockauto-merged cleanly.Review feedback addressed
b1200d0):comm=→command=+shortenProcessLabel();import type; JSDoc; robust interval-env parse;procpsin Dockerfile; log-cap stress test.f6c398d): drop inline eval code from labels; sampling-overlap guard;webtoolsretry-timer cancel + mid-init browser close;searxng-lifecycletests.ea3167c): flag/value-awareshortenProcessLabel;formatMebibytes→MiB; log-cap test renamed.4972dcb):ResourceStatsServicestopped first on shutdown.0205311):stop()awaits the in-flight sample; addedResourceStatsServicetests.bc56c94): close partially-created browser on init failure; 5spstimeout; accurate disable log for non-finite intervals.bad88d7):getStdioServerPidsonly mapsconnectedservers (no stale-pid mislabeling).Test plan
tsc --noEmitclean (real install, not a symlink)Buildgreen on the fixed commits🤖 Generated with Claude Code