From 49c1db20279f09943d409cb09e58a5c56097eb77 Mon Sep 17 00:00:00 2001 From: Jayson Jacobs Date: Mon, 6 Jul 2026 15:01:18 -0600 Subject: [PATCH 01/11] feat: periodic resource-stats logging for mcp-api and spawned MCP servers 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 --- src/env.ts | 1 + src/index.ts | 9 +++ src/services/mcp.ts | 20 ++++++ src/services/resourceStats.ts | 126 ++++++++++++++++++++++++++++++++++ src/utils/resourceStats.ts | 123 +++++++++++++++++++++++++++++++++ test/resource-stats.spec.ts | 124 +++++++++++++++++++++++++++++++++ 6 files changed, 403 insertions(+) create mode 100644 src/services/resourceStats.ts create mode 100644 src/utils/resourceStats.ts create mode 100644 test/resource-stats.spec.ts diff --git a/src/env.ts b/src/env.ts index 6b56964..aee271a 100644 --- a/src/env.ts +++ b/src/env.ts @@ -19,6 +19,7 @@ export const env = { return { repo, name } }), SEARXNG_URL: process.env.SEARXNG_URL, + RESOURCE_STATS_INTERVAL_MS: Number(process.env.RESOURCE_STATS_INTERVAL_MS ?? 60000) || 0, PYTHON_BIN: process.env.PYTHON_BIN, PYTHON_VENV_DIR: process.env.PYTHON_VENV_DIR || 'packages/python', PIP_INDEX_URL: process.env.PIP_INDEX_URL, diff --git a/src/index.ts b/src/index.ts index 02a3f75..2fad254 100644 --- a/src/index.ts +++ b/src/index.ts @@ -13,6 +13,7 @@ import { McpOAuthTokens } from './services/oauthTokens' import { McpUserSessions } from './services/userSessions' import { McpUserServerInstalls } from './services/userServerInstalls' import { McpDcrClients } from './services/dcrClients' +import { ResourceStatsService } from './services/resourceStats' export type Resource = { init: () => Promise @@ -93,6 +94,14 @@ export class API { // Set up circular dependency between MCPService and PackageService mcpController.getMcpService().setPackageService(packagesController.getPackageService()) + // Periodic resource-stats logging for mcp-api and its spawned MCP servers + const resourceStatsService = new ResourceStatsService({ + intervalMs: env.RESOURCE_STATS_INTERVAL_MS, + getTrackedProcesses: () => mcpController.getMcpService().getStdioServerPids() + }) + await resourceStatsService.init() + this.resources.push(resourceStatsService) + app.get('/healthz', (req, res) => { console.log('Health checked') res.status(200).send('OK') diff --git a/src/services/mcp.ts b/src/services/mcp.ts index 1a07f1d..d30e394 100644 --- a/src/services/mcp.ts +++ b/src/services/mcp.ts @@ -42,6 +42,7 @@ import { McpServerAlreadyExistsError } from './mcpErrors' import { validateExternalMcpUrl } from '../utils/ssrf' +import { getStdioTransportPid } from '../utils/resourceStats' export interface MCPConnection { client: Client @@ -3816,6 +3817,25 @@ export class MCPService implements Resource { return updatedServer } + /** + * Lists the OS process ids of currently running stdio MCP servers, + * labeled by server name, for resource monitoring. + */ + public getStdioServerPids(): { name: string; pid: number }[] { + const tracked: { name: string; pid: number }[] = [] + 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) + if (pid !== undefined) { + tracked.push({ name: server.name, pid }) + } + } + return tracked + } + public async stop() { log({ level: 'info', msg: 'Stopping MCP servers' }) diff --git a/src/services/resourceStats.ts b/src/services/resourceStats.ts new file mode 100644 index 0000000..e934447 --- /dev/null +++ b/src/services/resourceStats.ts @@ -0,0 +1,126 @@ +import { Resource } from '..' +import { log } from '../utils/general' +import { + SubtreeSample, + aggregateDirectChildSubtrees, + formatMebibytes, + sampleProcessTree +} from '../utils/resourceStats' + +export interface TrackedProcess { + name: string + pid: number +} + +export interface ResourceStatsServiceOptions { + /** Sampling interval in milliseconds. Values <= 0 disable sampling entirely. */ + intervalMs: number + /** Returns the currently running MCP server child processes to label by name. */ + getTrackedProcesses: () => TrackedProcess[] +} + +/** + * Periodically logs memory/CPU usage for the mcp-api process itself, every + * child process subtree it has spawned (installed stdio MCP servers, the + * Puppeteer-managed browser, package installs, ...), and a machine-facing + * total, so resource consumption is visible in the service logs over time. + */ +export class ResourceStatsService implements Resource { + private readonly intervalMs: number + private readonly getTrackedProcesses: () => TrackedProcess[] + private timer?: NodeJS.Timeout + private lastCpuUsage = process.cpuUsage() + private lastCpuSampleAt = process.hrtime.bigint() + private treeSamplingBroken = false + + constructor({ intervalMs, getTrackedProcesses }: ResourceStatsServiceOptions) { + this.intervalMs = intervalMs + this.getTrackedProcesses = getTrackedProcesses + } + + public async init(): Promise { + if (!Number.isFinite(this.intervalMs) || this.intervalMs <= 0) { + log({ level: 'info', msg: '[resource-stats] disabled (RESOURCE_STATS_INTERVAL_MS <= 0)' }) + return + } + 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) + // Never keep the process alive just to report stats + this.timer.unref() + log({ level: 'info', msg: `[resource-stats] sampling every ${this.intervalMs}ms` }) + } + + public async stop(): Promise { + if (this.timer) { + clearInterval(this.timer) + this.timer = undefined + } + } + + private sampleSelfCpuPercent(): number { + const now = process.hrtime.bigint() + const elapsedMicros = Number(now - this.lastCpuSampleAt) / 1000 + const usage = process.cpuUsage(this.lastCpuUsage) + this.lastCpuUsage = process.cpuUsage() + this.lastCpuSampleAt = now + if (elapsedMicros <= 0) { + return 0 + } + return ((usage.user + usage.system) / elapsedMicros) * 100 + } + + private async logSample(): Promise { + const memory = process.memoryUsage() + const cpuPercent = this.sampleSelfCpuPercent() + log({ + level: 'info', + msg: + `[resource-stats] self pid=${process.pid} rss=${formatMebibytes(memory.rss)} ` + + `heapUsed=${formatMebibytes(memory.heapUsed)} heapTotal=${formatMebibytes(memory.heapTotal)} ` + + `external=${formatMebibytes(memory.external)} cpu=${cpuPercent.toFixed(1)}%` + }) + + let subtrees: SubtreeSample[] + try { + const tree = await sampleProcessTree() + subtrees = aggregateDirectChildSubtrees(tree, process.pid) + this.treeSamplingBroken = false + } catch (error) { + // Warn once, then stay quiet: without `ps` only self stats are available + if (!this.treeSamplingBroken) { + this.treeSamplingBroken = true + log({ level: 'warn', msg: '[resource-stats] cannot sample child processes (is `ps` available?)', error }) + } + return + } + + const nameByPid = new Map() + for (const tracked of this.getTrackedProcesses()) { + nameByPid.set(tracked.pid, tracked.name) + } + + for (const subtree of subtrees) { + const name = nameByPid.get(subtree.rootPid) ?? subtree.command + log({ + level: 'info', + msg: + `[resource-stats] child name=${name} pid=${subtree.rootPid} ` + + `rss=${formatMebibytes(subtree.rssBytes)} cpu=${subtree.cpuPercent.toFixed(1)}% procs=${subtree.processCount}` + }) + } + + const childRss = subtrees.reduce((sum, subtree) => sum + subtree.rssBytes, 0) + const childProcs = subtrees.reduce((sum, subtree) => sum + subtree.processCount, 0) + log({ + level: 'info', + msg: + `[resource-stats] total rss=${formatMebibytes(memory.rss + childRss)} ` + + `procs=${childProcs + 1} (self + ${childProcs} descendants)` + }) + } +} diff --git a/src/utils/resourceStats.ts b/src/utils/resourceStats.ts new file mode 100644 index 0000000..2c010ac --- /dev/null +++ b/src/utils/resourceStats.ts @@ -0,0 +1,123 @@ +import { execFile } from 'node:child_process' +import { StdioClientTransport } from '@modelcontextprotocol/sdk/client/stdio.js' +import { Transport } from '@modelcontextprotocol/sdk/shared/transport.js' + +export interface ProcessSample { + pid: number + ppid: number + rssBytes: number + cpuPercent: number + command: string +} + +export interface SubtreeSample { + rootPid: number + command: string + rssBytes: number + cpuPercent: number + processCount: number +} + +/** + * Extracts the OS process id from a stdio client transport. + * + * @modelcontextprotocol/sdk 1.13.0 does not expose a public accessor for the + * spawned child process (verified against dist/cjs/client/stdio.d.ts: `_process` + * is private and no `pid` getter exists). This helper is the single, isolated + * place that reaches into that private field, and it validates the value at + * runtime so an SDK upgrade that changes internals degrades to `undefined` + * instead of misbehaving. + */ +export function getStdioTransportPid(transport: Transport): number | undefined { + if (!(transport instanceof StdioClientTransport)) { + return undefined + } + const internals = transport as unknown as { _process?: { pid?: unknown } } + const pid = internals._process?.pid + return typeof pid === 'number' && Number.isInteger(pid) && pid > 0 ? pid : undefined +} + +/** + * Parses `ps -eo pid=,ppid=,rss=,pcpu=,comm=` output. + * rss is reported by ps in kilobytes on both Linux (procps) and macOS (BSD ps). + * The command field may contain spaces, so it is everything after the fourth column. + */ +export function parsePsOutput(output: string): ProcessSample[] { + const samples: ProcessSample[] = [] + for (const line of output.split('\n')) { + const trimmed = line.trim() + if (!trimmed) continue + const match = trimmed.match(/^(\d+)\s+(\d+)\s+(\d+)\s+([\d.]+)\s+(.+)$/) + if (!match) continue + samples.push({ + pid: Number(match[1]), + ppid: Number(match[2]), + rssBytes: Number(match[3]) * 1024, + cpuPercent: Number(match[4]), + command: match[5].trim() + }) + } + return samples +} + +/** + * Groups every process descending from rootPid into one aggregate per direct + * child of rootPid: the child's own usage plus everything below it. This keeps + * one log line per spawned server (or browser) even when it forks helpers of + * its own (e.g. Chromium renderer processes). + */ +export function aggregateDirectChildSubtrees(samples: ProcessSample[], rootPid: number): SubtreeSample[] { + const childrenByPpid = new Map() + for (const sample of samples) { + const siblings = childrenByPpid.get(sample.ppid) + if (siblings) { + siblings.push(sample) + } else { + childrenByPpid.set(sample.ppid, [sample]) + } + } + + const collectSubtree = (pid: number, into: ProcessSample[], seen: Set): void => { + for (const child of childrenByPpid.get(pid) ?? []) { + if (seen.has(child.pid)) continue + seen.add(child.pid) + into.push(child) + collectSubtree(child.pid, into, seen) + } + } + + const subtrees: SubtreeSample[] = [] + for (const directChild of childrenByPpid.get(rootPid) ?? []) { + const members: ProcessSample[] = [directChild] + collectSubtree(directChild.pid, members, new Set([directChild.pid])) + subtrees.push({ + rootPid: directChild.pid, + command: directChild.command, + rssBytes: members.reduce((sum, m) => sum + m.rssBytes, 0), + cpuPercent: members.reduce((sum, m) => sum + m.cpuPercent, 0), + processCount: members.length + }) + } + return subtrees +} + +/** + * Samples the full OS process tree with a single `ps` invocation. + * + * @throws Error when `ps` is unavailable or exits abnormally. + */ +export function sampleProcessTree(): Promise { + return new Promise((resolve, reject) => { + execFile('ps', ['-eo', 'pid=,ppid=,rss=,pcpu=,comm='], { maxBuffer: 4 * 1024 * 1024 }, (error, stdout) => { + if (error) { + reject(error) + return + } + resolve(parsePsOutput(stdout)) + }) + }) +} + +export function formatMebibytes(bytes: number): string { + return `${(bytes / (1024 * 1024)).toFixed(1)}MB` +} diff --git a/test/resource-stats.spec.ts b/test/resource-stats.spec.ts new file mode 100644 index 0000000..ec06948 --- /dev/null +++ b/test/resource-stats.spec.ts @@ -0,0 +1,124 @@ +import { StdioClientTransport } from '@modelcontextprotocol/sdk/client/stdio.js' +import { + aggregateDirectChildSubtrees, + formatMebibytes, + getStdioTransportPid, + parsePsOutput, + sampleProcessTree +} from '../src/utils/resourceStats' + +describe('parsePsOutput', () => { + it('parses pid, ppid, rss (KB -> bytes), pcpu and command', () => { + const output = [ + ' 1 0 1024 0.5 node', + ' 4321 1 20480 12.3 /usr/local/bin/node dist/index.js', + '' + ].join('\n') + + const samples = parsePsOutput(output) + expect(samples).toEqual([ + { pid: 1, ppid: 0, rssBytes: 1024 * 1024, cpuPercent: 0.5, command: 'node' }, + { pid: 4321, ppid: 1, rssBytes: 20480 * 1024, cpuPercent: 12.3, command: '/usr/local/bin/node dist/index.js' } + ]) + }) + + it('preserves spaces inside the command column', () => { + const samples = parsePsOutput('99 1 512 0.0 Google Chrome Helper (Renderer)') + expect(samples).toHaveLength(1) + expect(samples[0].command).toBe('Google Chrome Helper (Renderer)') + }) + + it('skips malformed lines', () => { + const samples = parsePsOutput('not a process line\n\n12 abc 100 0.0 zsh') + expect(samples).toEqual([]) + }) +}) + +describe('aggregateDirectChildSubtrees', () => { + const sample = (pid: number, ppid: number, rssKb: number, cpu: number, command: string) => ({ + pid, + ppid, + rssBytes: rssKb * 1024, + cpuPercent: cpu, + command + }) + + it('aggregates each direct child with all of its descendants', () => { + const root = 100 + const tree = [ + sample(root, 1, 400_000, 2.0, 'node dist/index.js'), // self (excluded from subtrees) + sample(200, root, 50_000, 0.5, 'node mcp-github'), // stdio server + sample(300, root, 300_000, 3.0, 'chrome'), // browser + sample(301, 300, 150_000, 1.5, 'chrome'), // renderer under browser + sample(302, 301, 10_000, 0.1, 'chrome'), // nested helper + sample(999, 1, 999_000, 9.9, 'unrelated') // not in our tree + ] + + const subtrees = aggregateDirectChildSubtrees(tree, root) + expect(subtrees).toHaveLength(2) + + const server = subtrees.find((s) => s.rootPid === 200) + expect(server).toEqual({ + rootPid: 200, + command: 'node mcp-github', + rssBytes: 50_000 * 1024, + cpuPercent: 0.5, + processCount: 1 + }) + + const browser = subtrees.find((s) => s.rootPid === 300) + expect(browser).toBeDefined() + expect(browser!.rssBytes).toBe((300_000 + 150_000 + 10_000) * 1024) + expect(browser!.cpuPercent).toBeCloseTo(4.6) + expect(browser!.processCount).toBe(3) + }) + + it('returns an empty list when the root has no children', () => { + expect(aggregateDirectChildSubtrees([], 100)).toEqual([]) + }) + + it('does not loop forever on cyclic ppid data', () => { + const tree = [sample(200, 100, 1, 0, 'a'), sample(201, 200, 1, 0, 'b'), sample(200, 201, 1, 0, 'a-again')] + const subtrees = aggregateDirectChildSubtrees(tree, 100) + expect(subtrees).toHaveLength(1) + }) +}) + +describe('getStdioTransportPid', () => { + it('returns undefined for a transport that has not been started', () => { + const transport = new StdioClientTransport({ command: 'true' }) + expect(getStdioTransportPid(transport)).toBeUndefined() + }) + + it('returns the pid of a started transport process', () => { + const transport = Object.create(StdioClientTransport.prototype) as StdioClientTransport + ;(transport as unknown as { _process: { pid: number } })._process = { pid: 4321 } + expect(getStdioTransportPid(transport)).toBe(4321) + }) + + it('rejects non-numeric pid values', () => { + const transport = Object.create(StdioClientTransport.prototype) as StdioClientTransport + ;(transport as unknown as { _process: { pid: unknown } })._process = { pid: 'nope' } + expect(getStdioTransportPid(transport)).toBeUndefined() + }) + + it('returns undefined for non-stdio transports', () => { + const fake = { start: async () => undefined, send: async () => undefined, close: async () => undefined } + expect(getStdioTransportPid(fake)).toBeUndefined() + }) +}) + +describe('sampleProcessTree', () => { + it('samples the live process tree and includes this process', async () => { + const samples = await sampleProcessTree() + expect(samples.length).toBeGreaterThan(0) + expect(samples.some((s) => s.pid === process.pid)).toBe(true) + }) +}) + +describe('formatMebibytes', () => { + it('formats bytes as MB with one decimal', () => { + expect(formatMebibytes(412.3 * 1024 * 1024)).toBe('412.3MB') + expect(formatMebibytes(0)).toBe('0.0MB') + }) +}) From 2311c3dec7e630ddab86a353d445b7776cb005b3 Mon Sep 17 00:00:00 2001 From: Jayson Jacobs Date: Mon, 6 Jul 2026 15:21:13 -0600 Subject: [PATCH 02/11] chore: bump version to 1.11.9 Co-Authored-By: Claude Fable 5 --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index b9f87be..5827486 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@missionsquad/mcp-api", - "version": "1.11.8", + "version": "1.11.9", "description": "MCP Servers exposed via HTTP API", "main": "dist/index.js", "repository": "missionsquad/mcp-api", From 6527db925f8252893fb5eda810a43aec140c06e4 Mon Sep 17 00:00:00 2001 From: Jayson Jacobs Date: Mon, 6 Jul 2026 15:20:30 -0600 Subject: [PATCH 03/11] fix: cap in-memory MCP server logs at 500 lines; close Chromium on webtools 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 --- package.json | 2 +- src/builtin-servers/servers/searxng.ts | 5 +- src/services/mcp.ts | 29 ++++++++++-- test/server-logs-cap.spec.ts | 63 ++++++++++++++++++++++++++ 4 files changed, 93 insertions(+), 6 deletions(-) create mode 100644 test/server-logs-cap.spec.ts diff --git a/package.json b/package.json index b9f87be..5827486 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@missionsquad/mcp-api", - "version": "1.11.8", + "version": "1.11.9", "description": "MCP Servers exposed via HTTP API", "main": "dist/index.js", "repository": "missionsquad/mcp-api", diff --git a/src/builtin-servers/servers/searxng.ts b/src/builtin-servers/servers/searxng.ts index 4d2211c..62aa349 100644 --- a/src/builtin-servers/servers/searxng.ts +++ b/src/builtin-servers/servers/searxng.ts @@ -99,12 +99,13 @@ export class BuiltInSearxngServer extends BaseBuiltInServer { async stop(): Promise { 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' }) } catch (error) { log({ level: 'error', msg: 'Error stopping Puppeteer scraper', error }) } + this.scraper = null + this.scraperReady = false } } diff --git a/src/services/mcp.ts b/src/services/mcp.ts index 1a07f1d..be2ee72 100644 --- a/src/services/mcp.ts +++ b/src/services/mcp.ts @@ -917,6 +917,29 @@ const normalizeExternalAuthError = ( return error } +/** + * 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. + */ +export const MAX_SERVER_LOG_LINES = 500 + +/** + * Append a log line to an in-memory logs array, capping the array at + * {@link MAX_SERVER_LOG_LINES} entries by COUNT ONLY (no time-based eviction). + * When the cap is exceeded, the oldest lines are dropped so the array retains + * the most recent {@link MAX_SERVER_LOG_LINES} lines in insertion order. + * + * @param logs - The logs array to append to. A no-op when `undefined`. + * @param message - The log line to append. + */ +export function appendServerLog(logs: string[] | undefined, message: string): void { + if (!logs) return + logs.push(message) + if (logs.length > MAX_SERVER_LOG_LINES) { + logs.splice(0, logs.length - MAX_SERVER_LOG_LINES) + } +} + export class MCPService implements Resource { public servers: Record = {} public userConnections: Record = {} @@ -2248,7 +2271,7 @@ export class MCPService implements Resource { const transportErrorHandler = async (error: Error) => { log({ level: 'error', msg: `${serverKey} transport error: ${error.message}`, error }) if (this.servers[serverKey]) { - this.servers[serverKey].logs?.push(error.message) + appendServerLog(this.servers[serverKey].logs, error.message) } } @@ -2320,7 +2343,7 @@ export class MCPService implements Resource { const logMsg = data.toString().trim() log({ level: 'error', msg: `[${server.name}] stderr: ${logMsg}` }) if (this.servers[serverKey]) { - this.servers[serverKey].logs?.push(logMsg) + appendServerLog(this.servers[serverKey].logs, logMsg) } } @@ -2446,7 +2469,7 @@ export class MCPService implements Resource { log({ level: 'error', msg: `[${username}:${server.name}] transport error: ${error.message}`, error }) const conn = this.userConnections[userKey] if (conn) { - conn.logs?.push(error.message) + appendServerLog(conn.logs, error.message) } } diff --git a/test/server-logs-cap.spec.ts b/test/server-logs-cap.spec.ts new file mode 100644 index 0000000..d9a8601 --- /dev/null +++ b/test/server-logs-cap.spec.ts @@ -0,0 +1,63 @@ +import { appendServerLog, MAX_SERVER_LOG_LINES } from '../src/services/mcp' + +describe('appendServerLog', () => { + it('exposes a cap of 500 lines', () => { + expect(MAX_SERVER_LOG_LINES).toBe(500) + }) + + it('appends normally when under the cap', () => { + const logs: string[] = [] + appendServerLog(logs, 'a') + appendServerLog(logs, 'b') + appendServerLog(logs, 'c') + expect(logs).toEqual(['a', 'b', 'c']) + expect(logs.length).toBe(3) + }) + + it('keeps exactly the last 500 entries in order once the cap is exceeded (oldest dropped)', () => { + const logs: string[] = [] + const total = MAX_SERVER_LOG_LINES + 250 // 750 entries pushed + for (let i = 0; i < total; i++) { + appendServerLog(logs, `line-${i}`) + } + expect(logs.length).toBe(MAX_SERVER_LOG_LINES) + // The oldest 250 (line-0 .. line-249) must have been dropped. + expect(logs[0]).toBe(`line-${total - MAX_SERVER_LOG_LINES}`) // line-250 + expect(logs[logs.length - 1]).toBe(`line-${total - 1}`) // line-749 + // Contents are the last 500 lines in insertion order. + const expected: string[] = [] + for (let i = total - MAX_SERVER_LOG_LINES; i < total; i++) { + expected.push(`line-${i}`) + } + expect(logs).toEqual(expected) + }) + + it('tolerates an undefined logs array without throwing', () => { + expect(() => appendServerLog(undefined, 'ignored')).not.toThrow() + }) + + it('maintains a steady-state length of 500 when called repeatedly one-at-a-time past the cap', () => { + const logs: string[] = [] + // Fill exactly to the cap. + for (let i = 0; i < MAX_SERVER_LOG_LINES; i++) { + appendServerLog(logs, `seed-${i}`) + } + expect(logs.length).toBe(MAX_SERVER_LOG_LINES) + + // Each subsequent single append must keep the length pinned at the cap, + // dropping exactly one oldest line and retaining the newest. + for (let i = 0; i < 1000; i++) { + appendServerLog(logs, `stream-${i}`) + expect(logs.length).toBe(MAX_SERVER_LOG_LINES) + expect(logs[logs.length - 1]).toBe(`stream-${i}`) + } + + // After 1000 single appends past a full buffer, every seed line is gone + // and only the last 500 streamed lines remain, in order. + const expected: string[] = [] + for (let i = 1000 - MAX_SERVER_LOG_LINES; i < 1000; i++) { + expected.push(`stream-${i}`) + } + expect(logs).toEqual(expected) + }) +}) From e20399e760e9d1120d8d84a803fdbf0a3e0236c4 Mon Sep 17 00:00:00 2001 From: Jayson Jacobs Date: Mon, 6 Jul 2026 15:59:45 -0600 Subject: [PATCH 04/11] fix: require @missionsquad/puppeteer-scraper ^1.1.2 (Chromium page-leak 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 --- package.json | 2 +- yarn.lock | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/package.json b/package.json index 5827486..561ae19 100644 --- a/package.json +++ b/package.json @@ -30,7 +30,7 @@ "author": "Jayson Jacobs", "license": "Apache-2.0", "dependencies": { - "@missionsquad/puppeteer-scraper": "^1.1.1", + "@missionsquad/puppeteer-scraper": "^1.1.2", "@modelcontextprotocol/sdk": "^1.13.0", "@types/bcrypt": "^5.0.2", "base64-js": "^1.5.1", diff --git a/yarn.lock b/yarn.lock index 9678391..5f2ea4f 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1195,10 +1195,10 @@ dependencies: nanoid "3.3.10" -"@missionsquad/puppeteer-scraper@^1.1.1": - version "1.1.1" - resolved "https://registry.yarnpkg.com/@missionsquad/puppeteer-scraper/-/puppeteer-scraper-1.1.1.tgz#55165637a4530c22a28dc1112c6509257ec47f10" - integrity sha512-IfmqCt65ucxvZmHKkeUo61EhZE+3n6nt4ZTTZ0OsxemEdOmWNcio4cq5pvOinDHXYackA2Z7BLogChiFBOMPYA== +"@missionsquad/puppeteer-scraper@^1.1.2": + version "1.1.2" + resolved "https://registry.yarnpkg.com/@missionsquad/puppeteer-scraper/-/puppeteer-scraper-1.1.2.tgz#8569a5cade6715b5d4369df898975b8ebdec65d9" + integrity sha512-EUXDT3S5p+SIVEOqSGlF8zivNH2BsxagrExYamSeS9/e0NNKYCglizhSll+q+Pqn5LAj4ZK2goPDVKgMqQ7SGg== dependencies: "@missionsquad/common" "^1.1.0" browsers "^1.0.2" From b1200d0f5f6a380a7eda276637d5124cd06c25a6 Mon Sep 17 00:00:00 2001 From: Jayson Jacobs Date: Mon, 6 Jul 2026 18:08:27 -0600 Subject: [PATCH 05/11] fix: address PR review feedback on resource-stats + log cap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- Dockerfile | 1 + package.json | 2 +- src/env.ts | 14 ++++++++++++- src/services/mcp.ts | 2 +- src/services/resourceStats.ts | 7 ++++--- src/utils/resourceStats.ts | 36 +++++++++++++++++++++++++++++--- test/resource-stats.spec.ts | 39 ++++++++++++++++++++++++++++++++++- test/server-logs-cap.spec.ts | 19 +++++++++++++++++ 8 files changed, 110 insertions(+), 10 deletions(-) diff --git a/Dockerfile b/Dockerfile index bdc8c9e..61b200f 100644 --- a/Dockerfile +++ b/Dockerfile @@ -52,6 +52,7 @@ RUN apt-get update \ libssl3 \ zlib1g \ lsb-release \ + procps \ wget \ xdg-utils \ && rm -rf /var/lib/apt/lists/* diff --git a/package.json b/package.json index 1bd0eee..39ea117 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@missionsquad/mcp-api", - "version": "1.11.12", + "version": "1.11.13", "description": "MCP Servers exposed via HTTP API", "main": "dist/index.js", "repository": "missionsquad/mcp-api", diff --git a/src/env.ts b/src/env.ts index aee271a..9b210d0 100644 --- a/src/env.ts +++ b/src/env.ts @@ -2,6 +2,18 @@ import dotenv from 'dotenv' dotenv.config() +/** + * Parses a millisecond-interval env var. Returns `fallback` when the var is + * unset, blank, or not a finite number (so a typo defaults to the intended + * interval rather than silently disabling the feature). An explicit finite + * value — including `0`, which callers treat as "disabled" — is returned as-is. + */ +function parseIntervalMs(raw: string | undefined, fallback: number): number { + if (raw === undefined || raw.trim() === '') return fallback + const parsed = Number(raw) + return Number.isFinite(parsed) ? parsed : fallback +} + export const env = { DEBUG: /true/i.test(process.env.DEBUG || 'false'), ENABLE_OAUTH_LOGGING: /true/i.test(process.env.ENABLE_OAUTH_LOGGING || 'false'), @@ -19,7 +31,7 @@ export const env = { return { repo, name } }), SEARXNG_URL: process.env.SEARXNG_URL, - RESOURCE_STATS_INTERVAL_MS: Number(process.env.RESOURCE_STATS_INTERVAL_MS ?? 60000) || 0, + RESOURCE_STATS_INTERVAL_MS: parseIntervalMs(process.env.RESOURCE_STATS_INTERVAL_MS, 60000), PYTHON_BIN: process.env.PYTHON_BIN, PYTHON_VENV_DIR: process.env.PYTHON_VENV_DIR || 'packages/python', PIP_INDEX_URL: process.env.PIP_INDEX_URL, diff --git a/src/services/mcp.ts b/src/services/mcp.ts index 6989095..a214c02 100644 --- a/src/services/mcp.ts +++ b/src/services/mcp.ts @@ -920,7 +920,7 @@ const normalizeExternalAuthError = ( } /** - * Maximum number of in-memory log lines retained per MCP server and per + * Maximum number of in-memory log lines retained for each MCP server and each * per-user connection. Older lines are displaced once this count is exceeded. */ export const MAX_SERVER_LOG_LINES = 500 diff --git a/src/services/resourceStats.ts b/src/services/resourceStats.ts index e934447..94dba7d 100644 --- a/src/services/resourceStats.ts +++ b/src/services/resourceStats.ts @@ -1,10 +1,11 @@ -import { Resource } from '..' +import type { Resource } from '..' import { log } from '../utils/general' import { SubtreeSample, aggregateDirectChildSubtrees, formatMebibytes, - sampleProcessTree + sampleProcessTree, + shortenProcessLabel } from '../utils/resourceStats' export interface TrackedProcess { @@ -105,7 +106,7 @@ export class ResourceStatsService implements Resource { } for (const subtree of subtrees) { - const name = nameByPid.get(subtree.rootPid) ?? subtree.command + const name = nameByPid.get(subtree.rootPid) ?? shortenProcessLabel(subtree.command) log({ level: 'info', msg: diff --git a/src/utils/resourceStats.ts b/src/utils/resourceStats.ts index 2c010ac..d511523 100644 --- a/src/utils/resourceStats.ts +++ b/src/utils/resourceStats.ts @@ -1,4 +1,5 @@ import { execFile } from 'node:child_process' +import { basename } from 'node:path' import { StdioClientTransport } from '@modelcontextprotocol/sdk/client/stdio.js' import { Transport } from '@modelcontextprotocol/sdk/shared/transport.js' @@ -38,9 +39,11 @@ export function getStdioTransportPid(transport: Transport): number | undefined { } /** - * Parses `ps -eo pid=,ppid=,rss=,pcpu=,comm=` output. + * Parses `ps -eo pid=,ppid=,rss=,pcpu=,command=` output. * rss is reported by ps in kilobytes on both Linux (procps) and macOS (BSD ps). - * The command field may contain spaces, so it is everything after the fourth column. + * `command` is the full command line (may contain spaces), so it is everything + * after the fourth column. Use {@link shortenProcessLabel} to derive a concise, + * argument-free label for logging. */ export function parsePsOutput(output: string): ProcessSample[] { const samples: ProcessSample[] = [] @@ -101,14 +104,41 @@ export function aggregateDirectChildSubtrees(samples: ProcessSample[], rootPid: return subtrees } +const RUNTIME_EXECUTABLES = new Set(['node', 'nodejs', 'python', 'python3', 'bun', 'deno', 'ts-node']) + +/** + * Derives a concise, human-readable label from a full `ps command=` string for + * use in log lines. Returns the executable basename; for language runtimes + * (node/python/...) it also appends the script basename so that otherwise + * identical `node` processes stay distinguishable. Only executable and script + * basenames are used — full arguments are intentionally dropped so they never + * reach the logs (avoids leaking any sensitive values passed on the command + * line and keeps labels readable). + */ +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 +} + /** * Samples the full OS process tree with a single `ps` invocation. + * Requires the `ps` utility (procps on Linux images — installed in the + * Dockerfile; present by default on macOS/BSD). * * @throws Error when `ps` is unavailable or exits abnormally. */ export function sampleProcessTree(): Promise { return new Promise((resolve, reject) => { - execFile('ps', ['-eo', 'pid=,ppid=,rss=,pcpu=,comm='], { maxBuffer: 4 * 1024 * 1024 }, (error, stdout) => { + execFile('ps', ['-eo', 'pid=,ppid=,rss=,pcpu=,command='], { maxBuffer: 4 * 1024 * 1024 }, (error, stdout) => { if (error) { reject(error) return diff --git a/test/resource-stats.spec.ts b/test/resource-stats.spec.ts index ec06948..54ad047 100644 --- a/test/resource-stats.spec.ts +++ b/test/resource-stats.spec.ts @@ -4,7 +4,8 @@ import { formatMebibytes, getStdioTransportPid, parsePsOutput, - sampleProcessTree + sampleProcessTree, + shortenProcessLabel } from '../src/utils/resourceStats' describe('parsePsOutput', () => { @@ -122,3 +123,39 @@ describe('formatMebibytes', () => { expect(formatMebibytes(0)).toBe('0.0MB') }) }) + +describe('shortenProcessLabel', () => { + it('returns the executable basename for a non-runtime command', () => { + expect(shortenProcessLabel('/opt/google/chrome/chrome --type=renderer --no-sandbox')).toBe('chrome') + expect(shortenProcessLabel('/usr/lib/chromium/chromium --headless')).toBe('chromium') + }) + + it('appends the script basename for language runtimes so siblings stay distinct', () => { + expect( + shortenProcessLabel('node ./packages/missionsquad-mcp-rss/node_modules/@missionsquad/mcp-rss/dist/index.js') + ).toBe('node index.js') + expect(shortenProcessLabel('/usr/local/bin/node /app/dist/server.js --port 8080')).toBe('node server.js') + expect(shortenProcessLabel('python3 /app/tools/worker.py')).toBe('python3 worker.py') + }) + + it('skips leading flags when locating the script token', () => { + expect(shortenProcessLabel('node --inspect --enable-source-maps /app/dist/index.js')).toBe('node index.js') + }) + + it('returns just the runtime when it has no script argument', () => { + expect(shortenProcessLabel('node')).toBe('node') + expect(shortenProcessLabel('node --version')).toBe('node') + }) + + it('does not leak trailing command-line arguments into the label', () => { + const label = shortenProcessLabel('node /app/dist/index.js --api-key=SUPER_SECRET --token abc123') + expect(label).toBe('node index.js') + expect(label).not.toContain('SUPER_SECRET') + expect(label).not.toContain('abc123') + }) + + it('falls back to "unknown" for an empty command', () => { + expect(shortenProcessLabel('')).toBe('unknown') + expect(shortenProcessLabel(' ')).toBe('unknown') + }) +}) diff --git a/test/server-logs-cap.spec.ts b/test/server-logs-cap.spec.ts index d9a8601..75edabd 100644 --- a/test/server-logs-cap.spec.ts +++ b/test/server-logs-cap.spec.ts @@ -60,4 +60,23 @@ describe('appendServerLog', () => { } expect(logs).toEqual(expected) }) + + 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. + const logs: string[] = [] + const VOLUME = 100_000 + let peak = 0 + for (let i = 0; i < VOLUME; i++) { + appendServerLog(logs, `evt-${i}`) + if (logs.length > peak) peak = logs.length + } + // Peak length never exceeded the cap at any point during the run. + expect(peak).toBe(MAX_SERVER_LOG_LINES) + expect(logs.length).toBe(MAX_SERVER_LOG_LINES) + // Only the most recent 500 of the 100k lines survive, in order. + expect(logs[0]).toBe(`evt-${VOLUME - MAX_SERVER_LOG_LINES}`) + expect(logs[logs.length - 1]).toBe(`evt-${VOLUME - 1}`) + }) }) From f6c398d71cdf49bc739ea7b2882c46cabf423741 Mon Sep 17 00:00:00 2001 From: Jayson Jacobs Date: Mon, 6 Jul 2026 18:24:18 -0600 Subject: [PATCH 06/11] =?UTF-8?q?fix:=20address=20round-2=20review=20?= =?UTF-8?q?=E2=80=94=20arg-leak,=20sampling=20overlap,=20retry-after-stop?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 `, `python -c `), 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 --- package.json | 2 +- src/builtin-servers/servers/searxng.ts | 63 +++++++++++++----- src/services/resourceStats.ts | 17 ++++- src/utils/resourceStats.ts | 23 +++++-- test/resource-stats.spec.ts | 17 +++++ test/searxng-lifecycle.spec.ts | 89 ++++++++++++++++++++++++++ 6 files changed, 186 insertions(+), 25 deletions(-) create mode 100644 test/searxng-lifecycle.spec.ts diff --git a/package.json b/package.json index 39ea117..ed66942 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@missionsquad/mcp-api", - "version": "1.11.13", + "version": "1.11.14", "description": "MCP Servers exposed via HTTP API", "main": "dist/index.js", "repository": "missionsquad/mcp-api", diff --git a/src/builtin-servers/servers/searxng.ts b/src/builtin-servers/servers/searxng.ts index 62aa349..351d7af 100644 --- a/src/builtin-servers/servers/searxng.ts +++ b/src/builtin-servers/servers/searxng.ts @@ -12,6 +12,8 @@ export class BuiltInSearxngServer extends BaseBuiltInServer { private scraper: PuppeteerScraper | null = null private scraperReady = false + private stopped = false + private retryTimer: NodeJS.Timeout | null = null private readonly MAX_PUPPETEER_RETRIES = 5 private readonly INITIAL_RETRY_DELAY_MS = 15000 @@ -97,6 +99,13 @@ export class BuiltInSearxngServer extends BaseBuiltInServer { } async stop(): Promise { + // Mark stopped and cancel any pending init retry so a scheduled attempt + // cannot recreate the scraper/browser after shutdown. + this.stopped = true + if (this.retryTimer) { + clearTimeout(this.retryTimer) + this.retryTimer = null + } if (this.scraper) { try { await this.scraper.closeBrowser() @@ -274,38 +283,60 @@ export class BuiltInSearxngServer extends BaseBuiltInServer { } private async initializePuppeteerWithRetries(retryCount = 0): Promise { + // Bail out if the server was stopped before this (possibly delayed) attempt ran. + if (this.stopped) { + return + } try { - log({ - level: 'info', - msg: `Starting Puppeteer initialization (Attempt ${retryCount + 1}/${this.MAX_PUPPETEER_RETRIES})...` + log({ + level: 'info', + msg: `Starting Puppeteer initialization (Attempt ${retryCount + 1}/${this.MAX_PUPPETEER_RETRIES})...` }) - - this.scraper = new PuppeteerScraper({ + + const scraper = new PuppeteerScraper({ headless: true, ignoreHTTPSErrors: true, blockResources: false, cacheSize: 1000, enableGPU: false }) - - await this.scraper.init() + + await scraper.init() + + // stop() may have been called while init() was in flight; if so, close the + // freshly created browser instead of publishing it. + if (this.stopped) { + await scraper.closeBrowser() + return + } + + this.scraper = scraper this.scraperReady = true log({ level: 'info', msg: 'Puppeteer initialized successfully.' }) } catch (error) { - log({ - level: 'error', - msg: `Failed to initialize Puppeteer on attempt ${retryCount + 1}:`, - error + log({ + level: 'error', + msg: `Failed to initialize Puppeteer on attempt ${retryCount + 1}:`, + error }) - + + if (this.stopped) { + return + } + if (retryCount < this.MAX_PUPPETEER_RETRIES - 1) { const delay = this.INITIAL_RETRY_DELAY_MS * Math.pow(2, retryCount) log({ level: 'info', msg: `Retrying in ${delay / 1000} seconds...` }) - setTimeout(() => this.initializePuppeteerWithRetries(retryCount + 1), delay) + this.retryTimer = setTimeout(() => { + this.retryTimer = null + this.initializePuppeteerWithRetries(retryCount + 1) + }, delay) + // Do not keep the process alive solely for a pending retry. + this.retryTimer.unref() } else { - log({ - level: 'error', - msg: 'Max retries reached. Puppeteer initialization failed permanently for this session.' + log({ + level: 'error', + msg: 'Max retries reached. Puppeteer initialization failed permanently for this session.' }) } } diff --git a/src/services/resourceStats.ts b/src/services/resourceStats.ts index 94dba7d..b763eeb 100644 --- a/src/services/resourceStats.ts +++ b/src/services/resourceStats.ts @@ -30,6 +30,7 @@ export class ResourceStatsService implements Resource { private readonly intervalMs: number private readonly getTrackedProcesses: () => TrackedProcess[] private timer?: NodeJS.Timeout + private sampling = false private lastCpuUsage = process.cpuUsage() private lastCpuSampleAt = process.hrtime.bigint() private treeSamplingBroken = false @@ -47,9 +48,19 @@ export class ResourceStatsService implements Resource { 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 }) - }) + // Skip this tick if the previous sample is still running (e.g. a slow or + // blocked `ps`), so overlapping samples never pile up. + if (this.sampling) { + return + } + this.sampling = true + this.logSample() + .catch((error) => { + log({ level: 'debug', msg: '[resource-stats] sampling failed', error }) + }) + .finally(() => { + this.sampling = false + }) }, this.intervalMs) // Never keep the process alive just to report stats this.timer.unref() diff --git a/src/utils/resourceStats.ts b/src/utils/resourceStats.ts index d511523..2b85c11 100644 --- a/src/utils/resourceStats.ts +++ b/src/utils/resourceStats.ts @@ -105,15 +105,28 @@ export function aggregateDirectChildSubtrees(samples: ProcessSample[], rootPid: } const RUNTIME_EXECUTABLES = new Set(['node', 'nodejs', 'python', 'python3', 'bun', 'deno', 'ts-node']) +const SCRIPT_FILE_PATTERN = /\.(m?[jt]s|cjs|py|sh)$/i + +/** + * True when a token looks like a path to a script file rather than an arbitrary + * argument value: it either contains a path separator or ends in a known script + * extension. Used to ensure {@link shortenProcessLabel} only surfaces genuine + * script paths — never inline code passed to eval flags (`node -e `, + * `python -c `) or other argument values. + */ +function looksLikeScriptPath(token: string): boolean { + return token.includes('/') || SCRIPT_FILE_PATTERN.test(token) +} /** * Derives a concise, human-readable label from a full `ps command=` string for * use in log lines. Returns the executable basename; for language runtimes * (node/python/...) it also appends the script basename so that otherwise - * identical `node` processes stay distinguishable. Only executable and script - * basenames are used — full arguments are intentionally dropped so they never - * reach the logs (avoids leaking any sensitive values passed on the command - * line and keeps labels readable). + * identical `node` processes stay distinguishable. Only executable and + * script-file basenames are used — arbitrary arguments (including inline code + * after `-e`/`-c`) are intentionally dropped so they never reach the logs + * (avoids leaking sensitive values passed on the command line and keeps labels + * readable). */ export function shortenProcessLabel(command: string): string { const trimmed = command.trim() @@ -121,7 +134,7 @@ export function shortenProcessLabel(command: string): string { const tokens = trimmed.split(/\s+/) const exe = basename(tokens[0]) if (RUNTIME_EXECUTABLES.has(exe)) { - const scriptToken = tokens.slice(1).find((token) => !token.startsWith('-')) + const scriptToken = tokens.slice(1).find((token) => !token.startsWith('-') && looksLikeScriptPath(token)) if (scriptToken) { return `${exe} ${basename(scriptToken)}` } diff --git a/test/resource-stats.spec.ts b/test/resource-stats.spec.ts index 54ad047..7f8ff94 100644 --- a/test/resource-stats.spec.ts +++ b/test/resource-stats.spec.ts @@ -154,6 +154,23 @@ describe('shortenProcessLabel', () => { expect(label).not.toContain('abc123') }) + it('does not treat inline eval/print code as a script (no arg leakage)', () => { + // The token after -e/-c is code, not a script path; it must never appear in the label. + expect(shortenProcessLabel('node -e SUPER_SECRET_CODE')).toBe('node') + expect(shortenProcessLabel('node --eval SUPER_SECRET_CODE')).toBe('node') + expect(shortenProcessLabel('python -c import os,sys;print(SECRET)')).toBe('python') + expect(shortenProcessLabel('node -p process.env.TOKEN')).toBe('node') + expect(shortenProcessLabel('node -e SUPER_SECRET_CODE')).not.toContain('SUPER_SECRET_CODE') + }) + + it('only appends a token that looks like a script path (has a separator or known extension)', () => { + // Bare non-file arg -> just the runtime. + expect(shortenProcessLabel('node server')).toBe('node') + // Recognizable script forms -> runtime + script basename. + expect(shortenProcessLabel('node worker.mjs')).toBe('node worker.mjs') + expect(shortenProcessLabel('ts-node ./src/index.ts')).toBe('ts-node index.ts') + }) + it('falls back to "unknown" for an empty command', () => { expect(shortenProcessLabel('')).toBe('unknown') expect(shortenProcessLabel(' ')).toBe('unknown') diff --git a/test/searxng-lifecycle.spec.ts b/test/searxng-lifecycle.spec.ts new file mode 100644 index 0000000..7d61d7e --- /dev/null +++ b/test/searxng-lifecycle.spec.ts @@ -0,0 +1,89 @@ +// Verifies BuiltInSearxngServer's Puppeteer lifecycle guards without launching a +// real browser: @missionsquad/puppeteer-scraper is fully mocked. +const mockInit = jest.fn() +const mockCloseBrowser = jest.fn() +const mockScraperCtor = jest.fn() + +jest.mock('@missionsquad/puppeteer-scraper', () => ({ + PuppeteerScraper: jest.fn().mockImplementation((...args: unknown[]) => { + mockScraperCtor(...args) + return { init: mockInit, closeBrowser: mockCloseBrowser } + }) +})) + +import { BuiltInSearxngServer } from '../src/builtin-servers/servers/searxng' + +type Internals = { + initializePuppeteerWithRetries(retryCount: number): Promise + scraper: unknown + scraperReady: boolean +} +const internals = (server: BuiltInSearxngServer) => server as unknown as Internals + +describe('BuiltInSearxngServer Puppeteer lifecycle', () => { + beforeEach(() => { + jest.useFakeTimers() + mockScraperCtor.mockClear() + mockInit.mockReset() + mockCloseBrowser.mockReset() + mockCloseBrowser.mockResolvedValue(undefined) + }) + + afterEach(() => { + jest.clearAllTimers() + jest.useRealTimers() + }) + + it('schedules a retry timer after a failed init attempt', async () => { + mockInit.mockRejectedValue(new Error('init failed')) + const server = new BuiltInSearxngServer() + + await internals(server).initializePuppeteerWithRetries(0) + + expect(mockScraperCtor).toHaveBeenCalledTimes(1) + // A pending retry is scheduled (not yet fired). + expect(jest.getTimerCount()).toBe(1) + }) + + it('cancels the pending retry on stop() and ignores any later retry attempt', async () => { + mockInit.mockRejectedValue(new Error('init failed')) + const server = new BuiltInSearxngServer() + + await internals(server).initializePuppeteerWithRetries(0) + expect(jest.getTimerCount()).toBe(1) + + await server.stop() + // The scheduled retry timer is cleared by stop(). + expect(jest.getTimerCount()).toBe(0) + + // Even if a retry callback still fired after stop(), the stopped guard makes it + // a no-op: no new scraper/browser is ever constructed. + mockScraperCtor.mockClear() + await internals(server).initializePuppeteerWithRetries(1) + expect(mockScraperCtor).not.toHaveBeenCalled() + }) + + it('closes a browser created during init if stop() ran mid-init (no leak, not published)', async () => { + let resolveInit: () => void = () => undefined + mockInit.mockImplementation( + () => + new Promise((resolve) => { + resolveInit = resolve + }) + ) + const server = new BuiltInSearxngServer() + + const pending = internals(server).initializePuppeteerWithRetries(0) + expect(mockScraperCtor).toHaveBeenCalledTimes(1) + + // stop() runs while init() is still in flight; the scraper is not yet published. + await server.stop() + resolveInit() + await pending + + // The freshly created browser is closed by the mid-init guard, not published. + expect(mockCloseBrowser).toHaveBeenCalledTimes(1) + expect(internals(server).scraper).toBeNull() + expect(internals(server).scraperReady).toBe(false) + }) +}) From ea3167c231bc1a8a4a1dd3500b9b043e60c58051 Mon Sep 17 00:00:00 2001 From: Jayson Jacobs Date: Mon, 6 Jul 2026 18:32:30 -0600 Subject: [PATCH 07/11] =?UTF-8?q?fix:=20address=20round-3=20review=20?= =?UTF-8?q?=E2=80=94=20flag-value=20parsing,=20MiB=20unit,=20test=20naming?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- package.json | 2 +- src/utils/resourceStats.ts | 68 ++++++++++++++++++++++++++++-------- test/resource-stats.spec.ts | 21 +++++++++-- test/server-logs-cap.spec.ts | 7 ++-- 4 files changed, 77 insertions(+), 21 deletions(-) diff --git a/package.json b/package.json index ed66942..361b3dd 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@missionsquad/mcp-api", - "version": "1.11.14", + "version": "1.11.15", "description": "MCP Servers exposed via HTTP API", "main": "dist/index.js", "repository": "missionsquad/mcp-api", diff --git a/src/utils/resourceStats.ts b/src/utils/resourceStats.ts index 2b85c11..8cc0b30 100644 --- a/src/utils/resourceStats.ts +++ b/src/utils/resourceStats.ts @@ -106,13 +106,24 @@ export function aggregateDirectChildSubtrees(samples: ProcessSample[], rootPid: const RUNTIME_EXECUTABLES = new Set(['node', 'nodejs', 'python', 'python3', 'bun', 'deno', 'ts-node']) const SCRIPT_FILE_PATTERN = /\.(m?[jt]s|cjs|py|sh)$/i +// Flags whose following token is inline code — the process has no script file and +// the value must never be surfaced (it can contain arbitrary code/secrets). +const EVAL_FLAGS = new Set(['-e', '--eval', '-p', '--print', '-c']) +// Flags whose following token is a value that is NOT the main script (a preloaded +// module, loader, etc.); the value must be skipped so the real script is found. +const VALUE_FLAGS = new Set([ + ...EVAL_FLAGS, + '-r', + '--require', + '--import', + '--loader', + '--experimental-loader' +]) /** * True when a token looks like a path to a script file rather than an arbitrary * argument value: it either contains a path separator or ends in a known script - * extension. Used to ensure {@link shortenProcessLabel} only surfaces genuine - * script paths — never inline code passed to eval flags (`node -e `, - * `python -c `) or other argument values. + * extension. */ function looksLikeScriptPath(token: string): boolean { return token.includes('/') || SCRIPT_FILE_PATTERN.test(token) @@ -121,23 +132,47 @@ function looksLikeScriptPath(token: string): boolean { /** * Derives a concise, human-readable label from a full `ps command=` string for * use in log lines. Returns the executable basename; for language runtimes - * (node/python/...) it also appends the script basename so that otherwise - * identical `node` processes stay distinguishable. Only executable and - * script-file basenames are used — arbitrary arguments (including inline code - * after `-e`/`-c`) are intentionally dropped so they never reach the logs - * (avoids leaking sensitive values passed on the command line and keeps labels - * readable). + * (node/python/...) it also appends the basename of the main script so that + * otherwise identical `node` processes stay distinguishable. + * + * Only executable and genuine script-file basenames are surfaced. Flags and + * their values are parsed so that inline code (`node -e `, `python -c + * `) and preloaded-module values (`node -r `) are never mistaken + * for the script — this keeps arbitrary/sensitive command-line values out of the + * logs even when they contain path separators or script-like extensions. */ 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)}` + if (!RUNTIME_EXECUTABLES.has(exe)) { + return exe + } + + let sawEvalFlag = false + let script: string | undefined + for (let i = 1; i < tokens.length; i++) { + const token = tokens[i] + if (token.startsWith('-')) { + const name = token.includes('=') ? token.slice(0, token.indexOf('=')) : token + if (EVAL_FLAGS.has(name)) { + sawEvalFlag = true + } + // A separate value token (no inline `=`) belongs to this flag, not the script. + if (!token.includes('=') && VALUE_FLAGS.has(name)) { + i++ + } + continue } + // First positional token — the main script/entrypoint. + script = token + break + } + + // Eval/print invocations have no script file; never surface the inline code. + if (!sawEvalFlag && script && looksLikeScriptPath(script)) { + return `${exe} ${basename(script)}` } return exe } @@ -161,6 +196,11 @@ export function sampleProcessTree(): Promise { }) } +/** + * Formats a byte count as base-1024 mebibytes with a `MiB` suffix (matching the + * unit the math actually produces, so operators can compare against `docker + * stats` / `top` without a base-1000 vs base-1024 mismatch). + */ export function formatMebibytes(bytes: number): string { - return `${(bytes / (1024 * 1024)).toFixed(1)}MB` + return `${(bytes / (1024 * 1024)).toFixed(1)}MiB` } diff --git a/test/resource-stats.spec.ts b/test/resource-stats.spec.ts index 7f8ff94..497c23e 100644 --- a/test/resource-stats.spec.ts +++ b/test/resource-stats.spec.ts @@ -118,9 +118,9 @@ describe('sampleProcessTree', () => { }) describe('formatMebibytes', () => { - it('formats bytes as MB with one decimal', () => { - expect(formatMebibytes(412.3 * 1024 * 1024)).toBe('412.3MB') - expect(formatMebibytes(0)).toBe('0.0MB') + it('formats bytes as base-1024 MiB with one decimal', () => { + expect(formatMebibytes(412.3 * 1024 * 1024)).toBe('412.3MiB') + expect(formatMebibytes(0)).toBe('0.0MiB') }) }) @@ -171,6 +171,21 @@ describe('shortenProcessLabel', () => { expect(shortenProcessLabel('ts-node ./src/index.ts')).toBe('ts-node index.ts') }) + it('skips flag values (e.g. -r/--require preloads) and selects the real script', () => { + expect(shortenProcessLabel('node -r ts-node/register /app/index.js')).toBe('node index.js') + expect(shortenProcessLabel('node --require ./setup.js /srv/main.js')).toBe('node main.js') + expect(shortenProcessLabel('node --import tsx /app/dist/server.js')).toBe('node server.js') + // The preloaded module name must not become the label. + expect(shortenProcessLabel('node -r ts-node/register /app/index.js')).not.toContain('register') + }) + + it('never surfaces inline eval code even when it contains a slash or extension', () => { + expect(shortenProcessLabel('node -e require("/etc/passwd")')).toBe('node') + expect(shortenProcessLabel('node -e require("/etc/passwd")')).not.toContain('passwd') + expect(shortenProcessLabel('python -c open("/secret/key.pem")')).toBe('python') + expect(shortenProcessLabel('python -c open("/secret/key.pem")')).not.toContain('key.pem') + }) + it('falls back to "unknown" for an empty command', () => { expect(shortenProcessLabel('')).toBe('unknown') expect(shortenProcessLabel(' ')).toBe('unknown') diff --git a/test/server-logs-cap.spec.ts b/test/server-logs-cap.spec.ts index 75edabd..a823339 100644 --- a/test/server-logs-cap.spec.ts +++ b/test/server-logs-cap.spec.ts @@ -61,10 +61,11 @@ describe('appendServerLog', () => { expect(logs).toEqual(expected) }) - it('stays bounded and correct under sustained high-volume appends (memory does not grow)', () => { + it('never exceeds the cap length under sustained high-volume appends (bounds retained lines)', () => { // 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. + // streaming stderr lines for the whole process lifetime. The array length must + // never exceed the cap regardless of how many lines are pushed — which is what + // bounds the retained-line count (and therefore this buffer's memory). const logs: string[] = [] const VOLUME = 100_000 let peak = 0 From 4972dcbe812d7a50103da4eba02d90c387ac0735 Mon Sep 17 00:00:00 2001 From: Jayson Jacobs Date: Mon, 6 Jul 2026 18:37:23 -0600 Subject: [PATCH 08/11] fix: stop ResourceStatsService first on shutdown 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 --- package.json | 2 +- src/index.ts | 7 +++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/package.json b/package.json index 361b3dd..7049635 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@missionsquad/mcp-api", - "version": "1.11.15", + "version": "1.11.16", "description": "MCP Servers exposed via HTTP API", "main": "dist/index.js", "repository": "missionsquad/mcp-api", diff --git a/src/index.ts b/src/index.ts index 2fad254..1ed9e8c 100644 --- a/src/index.ts +++ b/src/index.ts @@ -94,13 +94,16 @@ export class API { // Set up circular dependency between MCPService and PackageService mcpController.getMcpService().setPackageService(packagesController.getPackageService()) - // Periodic resource-stats logging for mcp-api and its spawned MCP servers + // Periodic resource-stats logging for mcp-api and its spawned MCP servers. + // Registered at the FRONT of resources so shutDown() stops it first: sampling + // must cease before the MCP/package services it samples begin tearing down, + // otherwise a tick mid-shutdown would spawn `ps` and log stale stats. const resourceStatsService = new ResourceStatsService({ intervalMs: env.RESOURCE_STATS_INTERVAL_MS, getTrackedProcesses: () => mcpController.getMcpService().getStdioServerPids() }) await resourceStatsService.init() - this.resources.push(resourceStatsService) + this.resources.unshift(resourceStatsService) app.get('/healthz', (req, res) => { console.log('Health checked') From 02053118d40bcdebac1ae4c00d1093b1eedf4c31 Mon Sep 17 00:00:00 2001 From: Jayson Jacobs Date: Mon, 6 Jul 2026 18:44:14 -0600 Subject: [PATCH 09/11] fix: await in-flight sample in ResourceStatsService.stop() 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 --- package.json | 2 +- src/services/resourceStats.ts | 9 +++- test/resource-stats-service.spec.ts | 78 +++++++++++++++++++++++++++++ 3 files changed, 87 insertions(+), 2 deletions(-) create mode 100644 test/resource-stats-service.spec.ts diff --git a/package.json b/package.json index 7049635..e62f5d5 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@missionsquad/mcp-api", - "version": "1.11.16", + "version": "1.11.17", "description": "MCP Servers exposed via HTTP API", "main": "dist/index.js", "repository": "missionsquad/mcp-api", diff --git a/src/services/resourceStats.ts b/src/services/resourceStats.ts index b763eeb..76effde 100644 --- a/src/services/resourceStats.ts +++ b/src/services/resourceStats.ts @@ -31,6 +31,7 @@ export class ResourceStatsService implements Resource { private readonly getTrackedProcesses: () => TrackedProcess[] private timer?: NodeJS.Timeout private sampling = false + private samplingPromise?: Promise private lastCpuUsage = process.cpuUsage() private lastCpuSampleAt = process.hrtime.bigint() private treeSamplingBroken = false @@ -54,12 +55,13 @@ export class ResourceStatsService implements Resource { return } this.sampling = true - this.logSample() + this.samplingPromise = this.logSample() .catch((error) => { log({ level: 'debug', msg: '[resource-stats] sampling failed', error }) }) .finally(() => { this.sampling = false + this.samplingPromise = undefined }) }, this.intervalMs) // Never keep the process alive just to report stats @@ -72,6 +74,11 @@ export class ResourceStatsService implements Resource { clearInterval(this.timer) this.timer = undefined } + // Wait for any in-flight sample to finish so no sampling (spawning `ps`, + // logging) overlaps the teardown of the services this samples. + if (this.samplingPromise) { + await this.samplingPromise + } } private sampleSelfCpuPercent(): number { diff --git a/test/resource-stats-service.spec.ts b/test/resource-stats-service.spec.ts new file mode 100644 index 0000000..df1eb85 --- /dev/null +++ b/test/resource-stats-service.spec.ts @@ -0,0 +1,78 @@ +// Verifies ResourceStatsService.stop() waits for an in-flight sample so no +// sampling overlaps service teardown. sampleProcessTree is mocked so the test +// controls exactly when a sample completes. +jest.mock('../src/utils/resourceStats', () => { + const actual = jest.requireActual('../src/utils/resourceStats') + return { ...actual, sampleProcessTree: jest.fn() } +}) + +import { sampleProcessTree } from '../src/utils/resourceStats' +import { ResourceStatsService } from '../src/services/resourceStats' + +const mockSampleProcessTree = sampleProcessTree as jest.Mock + +describe('ResourceStatsService.stop', () => { + beforeEach(() => { + jest.useFakeTimers() + mockSampleProcessTree.mockReset() + mockSampleProcessTree.mockResolvedValue([]) + }) + + afterEach(() => { + jest.clearAllTimers() + jest.useRealTimers() + }) + + const flushMicrotasks = async () => { + for (let i = 0; i < 5; i++) { + await Promise.resolve() + } + } + + it('waits for an in-flight sample to finish before resolving', async () => { + let resolveSample: (value: unknown[]) => void = () => undefined + mockSampleProcessTree.mockReturnValue( + new Promise((resolve) => { + resolveSample = resolve + }) + ) + + const service = new ResourceStatsService({ intervalMs: 1000, getTrackedProcesses: () => [] }) + await service.init() + + // Fire one tick: logSample() starts and suspends on the pending sampleProcessTree(). + jest.advanceTimersByTime(1000) + await flushMicrotasks() + expect(mockSampleProcessTree).toHaveBeenCalledTimes(1) + + let stopped = false + const stopPromise = service.stop().then(() => { + stopped = true + }) + + // stop() must not resolve while the sample is still in flight. + await flushMicrotasks() + expect(stopped).toBe(false) + + // Completing the sample lets stop() resolve. + resolveSample([]) + await stopPromise + expect(stopped).toBe(true) + }) + + it('resolves immediately when no sample is in flight', async () => { + const service = new ResourceStatsService({ intervalMs: 1000, getTrackedProcesses: () => [] }) + await service.init() + await expect(service.stop()).resolves.toBeUndefined() + expect(mockSampleProcessTree).not.toHaveBeenCalled() + }) + + it('is disabled (no timer, never samples) when the interval is 0', async () => { + const service = new ResourceStatsService({ intervalMs: 0, getTrackedProcesses: () => [] }) + await service.init() + jest.advanceTimersByTime(60000) + await flushMicrotasks() + expect(mockSampleProcessTree).not.toHaveBeenCalled() + await expect(service.stop()).resolves.toBeUndefined() + }) +}) From bc56c946a70ba81992a8ecacd874f55d0874341f Mon Sep 17 00:00:00 2001 From: Jayson Jacobs Date: Mon, 6 Jul 2026 18:49:52 -0600 Subject: [PATCH 10/11] =?UTF-8?q?fix:=20address=20round-6=20review=20?= =?UTF-8?q?=E2=80=94=20init-failure=20browser=20close,=20ps=20timeout,=20d?= =?UTF-8?q?isable=20msg?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- package.json | 2 +- src/builtin-servers/servers/searxng.ts | 14 +++++++++++++- src/services/resourceStats.ts | 5 ++++- src/utils/resourceStats.ts | 25 ++++++++++++++++++------- test/resource-stats-service.spec.ts | 8 ++++++++ test/searxng-lifecycle.spec.ts | 12 ++++++++++++ 6 files changed, 56 insertions(+), 10 deletions(-) diff --git a/package.json b/package.json index e62f5d5..db7e182 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@missionsquad/mcp-api", - "version": "1.11.17", + "version": "1.11.18", "description": "MCP Servers exposed via HTTP API", "main": "dist/index.js", "repository": "missionsquad/mcp-api", diff --git a/src/builtin-servers/servers/searxng.ts b/src/builtin-servers/servers/searxng.ts index 351d7af..5c4d716 100644 --- a/src/builtin-servers/servers/searxng.ts +++ b/src/builtin-servers/servers/searxng.ts @@ -287,13 +287,14 @@ export class BuiltInSearxngServer extends BaseBuiltInServer { if (this.stopped) { return } + let scraper: PuppeteerScraper | undefined try { log({ level: 'info', msg: `Starting Puppeteer initialization (Attempt ${retryCount + 1}/${this.MAX_PUPPETEER_RETRIES})...` }) - const scraper = new PuppeteerScraper({ + scraper = new PuppeteerScraper({ headless: true, ignoreHTTPSErrors: true, blockResources: false, @@ -320,6 +321,17 @@ export class BuiltInSearxngServer extends BaseBuiltInServer { error }) + // Close any partially-created browser before retrying so repeated init + // failures cannot leak Chromium processes. closeBrowser() is a no-op if the + // scraper already cleaned up internally. + if (scraper) { + try { + await scraper.closeBrowser() + } catch (closeError) { + log({ level: 'error', msg: 'Error closing partially-initialized Puppeteer scraper', error: closeError }) + } + } + if (this.stopped) { return } diff --git a/src/services/resourceStats.ts b/src/services/resourceStats.ts index 76effde..5f217a9 100644 --- a/src/services/resourceStats.ts +++ b/src/services/resourceStats.ts @@ -43,7 +43,10 @@ export class ResourceStatsService implements Resource { public async init(): Promise { if (!Number.isFinite(this.intervalMs) || this.intervalMs <= 0) { - log({ level: 'info', msg: '[resource-stats] disabled (RESOURCE_STATS_INTERVAL_MS <= 0)' }) + log({ + level: 'info', + msg: `[resource-stats] disabled (RESOURCE_STATS_INTERVAL_MS must be a positive number of ms; got ${this.intervalMs})` + }) return } this.lastCpuUsage = process.cpuUsage() diff --git a/src/utils/resourceStats.ts b/src/utils/resourceStats.ts index 8cc0b30..4d65380 100644 --- a/src/utils/resourceStats.ts +++ b/src/utils/resourceStats.ts @@ -177,22 +177,33 @@ export function shortenProcessLabel(command: string): string { return exe } +// Bounds a single `ps` invocation. A normal `ps -eo` completes in milliseconds; +// this ceiling exists only so a wedged `ps` cannot stall sampling (and, via +// stop() awaiting the in-flight sample, shutdown) indefinitely. +const PS_TIMEOUT_MS = 5000 + /** * Samples the full OS process tree with a single `ps` invocation. * Requires the `ps` utility (procps on Linux images — installed in the * Dockerfile; present by default on macOS/BSD). * - * @throws Error when `ps` is unavailable or exits abnormally. + * @throws Error when `ps` is unavailable, times out ({@link PS_TIMEOUT_MS}), or + * exits abnormally. */ export function sampleProcessTree(): Promise { return new Promise((resolve, reject) => { - execFile('ps', ['-eo', 'pid=,ppid=,rss=,pcpu=,command='], { maxBuffer: 4 * 1024 * 1024 }, (error, stdout) => { - if (error) { - reject(error) - return + execFile( + 'ps', + ['-eo', 'pid=,ppid=,rss=,pcpu=,command='], + { maxBuffer: 4 * 1024 * 1024, timeout: PS_TIMEOUT_MS }, + (error, stdout) => { + if (error) { + reject(error) + return + } + resolve(parsePsOutput(stdout)) } - resolve(parsePsOutput(stdout)) - }) + ) }) } diff --git a/test/resource-stats-service.spec.ts b/test/resource-stats-service.spec.ts index df1eb85..1d25691 100644 --- a/test/resource-stats-service.spec.ts +++ b/test/resource-stats-service.spec.ts @@ -75,4 +75,12 @@ describe('ResourceStatsService.stop', () => { expect(mockSampleProcessTree).not.toHaveBeenCalled() await expect(service.stop()).resolves.toBeUndefined() }) + + it('is disabled when the interval is not a finite number', async () => { + const service = new ResourceStatsService({ intervalMs: NaN, getTrackedProcesses: () => [] }) + await service.init() + jest.advanceTimersByTime(60000) + await flushMicrotasks() + expect(mockSampleProcessTree).not.toHaveBeenCalled() + }) }) diff --git a/test/searxng-lifecycle.spec.ts b/test/searxng-lifecycle.spec.ts index 7d61d7e..1b4d3c6 100644 --- a/test/searxng-lifecycle.spec.ts +++ b/test/searxng-lifecycle.spec.ts @@ -45,6 +45,18 @@ describe('BuiltInSearxngServer Puppeteer lifecycle', () => { expect(jest.getTimerCount()).toBe(1) }) + it('closes a partially-initialized browser when init fails, before retrying', async () => { + mockInit.mockRejectedValue(new Error('init failed')) + const server = new BuiltInSearxngServer() + + await internals(server).initializePuppeteerWithRetries(0) + + // The failed attempt's browser is closed in the catch so repeated failures + // cannot leak Chromium processes. + expect(mockScraperCtor).toHaveBeenCalledTimes(1) + expect(mockCloseBrowser).toHaveBeenCalledTimes(1) + }) + it('cancels the pending retry on stop() and ignores any later retry attempt', async () => { mockInit.mockRejectedValue(new Error('init failed')) const server = new BuiltInSearxngServer() From bad88d7f7b16f2d05d65a0e587a93ad3f7eb5aa7 Mon Sep 17 00:00:00 2001 From: Jayson Jacobs Date: Mon, 6 Jul 2026 18:55:03 -0600 Subject: [PATCH 11/11] fix: only report connected stdio servers in getStdioServerPids 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 --- package.json | 2 +- src/services/mcp.ts | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/package.json b/package.json index db7e182..552c378 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@missionsquad/mcp-api", - "version": "1.11.18", + "version": "1.11.19", "description": "MCP Servers exposed via HTTP API", "main": "dist/index.js", "repository": "missionsquad/mcp-api", diff --git a/src/services/mcp.ts b/src/services/mcp.ts index a214c02..170d46b 100644 --- a/src/services/mcp.ts +++ b/src/services/mcp.ts @@ -3849,12 +3849,17 @@ export class MCPService implements Resource { /** * Lists the OS process ids of currently running stdio MCP servers, * labeled by server name, for resource monitoring. + * + * Only servers with status 'connected' are included: `connection` is not + * cleared on transport close, so a disconnected/error server could otherwise + * contribute a stale pid and mislabel an unrelated process if the OS reused it. */ public getStdioServerPids(): { name: string; pid: number }[] { const tracked: { name: string; pid: number }[] = [] for (const serverKey of Object.keys(this.servers)) { const server = this.servers[serverKey] if (server.transportType !== 'stdio') continue + if (server.status !== 'connected') continue const transport = server.connection?.transport if (!transport) continue const pid = getStdioTransportPid(transport)