From 880cf2bc91bce1a0925a189f9f3c14f6ed18fcd6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ignacy=20=C5=81=C4=85tka?= Date: Wed, 6 May 2026 18:11:03 +0200 Subject: [PATCH 1/4] fix: stop argent mcp from looping at 100% CPU on broken stdio When `argent mcp` runs as an orphaned process whose stderr pipe is broken (e.g. parent died), every `stderr.write` from the uncaughtException handler emits an async 'error' event. Without an 'error' listener that becomes another uncaughtException, runs the handler again, and so on forever. Move the handlers into `fatal-handlers.ts` and add three guards: - 'error' listeners on stdout/stderr exit cleanly when stdio is broken, before the failure becomes another uncaughtException - try/catch around `stderr.write` so a sync write failure exits instead of escaping - try/catch around the formatter so a throwing `.stack` getter or `toString` (the production trace pointed at defaultPrepareStackTrace) can't take down the handler Idempotent: a second `installFatalHandlers` call is a no-op. --- packages/argent/src/cli.ts | 12 +- packages/argent/src/fatal-handlers.ts | 53 ++++++++ packages/argent/test/fatal-handlers.test.ts | 141 ++++++++++++++++++++ 3 files changed, 196 insertions(+), 10 deletions(-) create mode 100644 packages/argent/src/fatal-handlers.ts create mode 100644 packages/argent/test/fatal-handlers.test.ts diff --git a/packages/argent/src/cli.ts b/packages/argent/src/cli.ts index 7901037e..8411777c 100644 --- a/packages/argent/src/cli.ts +++ b/packages/argent/src/cli.ts @@ -26,6 +26,7 @@ import type * as Installer from "@argent/installer"; import type * as Mcp from "@argent/mcp"; import type * as Cli from "@argent/cli"; import { BUNDLED_RUNTIME_PATHS } from "./bundled-paths.js"; +import { installFatalHandlers } from "./fatal-handlers.js"; const PACKAGE_NAME = "@swmansion/argent"; @@ -44,16 +45,7 @@ function getInstalledVersion(): string | null { const [, , command, ...rest] = process.argv; const isMcpServer = command === "mcp"; -process.on("uncaughtException", (err) => { - process.stderr.write(`[argent] Uncaught exception: ${err.stack ?? err}\n`); - if (!isMcpServer) process.exit(1); -}); -process.on("unhandledRejection", (reason) => { - process.stderr.write( - `[argent] Unhandled rejection: ${reason instanceof Error ? (reason.stack ?? reason.message) : reason}\n` - ); - if (!isMcpServer) process.exit(1); -}); +installFatalHandlers({ isMcpServer }); function printHelp(): void { const version = getInstalledVersion() ?? "unknown"; diff --git a/packages/argent/src/fatal-handlers.ts b/packages/argent/src/fatal-handlers.ts new file mode 100644 index 00000000..fd441d47 --- /dev/null +++ b/packages/argent/src/fatal-handlers.ts @@ -0,0 +1,53 @@ +// Process-level handlers for uncaughtException and unhandledRejection. +// +// In MCP-server mode we do NOT exit on every uncaught error — many are +// transient and the editor expects the server to keep running. But the naive +// version (`stderr.write(...); if (!isMcp) exit`) burns 100% CPU when stderr +// itself is broken (e.g. the parent process is gone): the write emits an +// async 'error' event on the stream, which without a listener becomes another +// uncaughtException, runs the handler, writes to the same broken stream, ... +// +// The fix has three pieces: +// 1. `'error'` listeners on stdout/stderr — broken stdio is fatal; exit +// before the failure round-trips into uncaughtException. This is what +// breaks the production loop. +// 2. try/catch around `stderr.write` — synchronous write failures also exit +// cleanly instead of escaping into another uncaughtException. +// 3. try/catch around the formatter — a throwing `.stack` getter or +// `toString` (the production trace pointed at defaultPrepareStackTrace) +// can't take down the handler. + +let installed = false; + +export function installFatalHandlers(opts: { isMcpServer: boolean }): void { + if (installed) return; + installed = true; + + for (const stream of [process.stdout, process.stderr] as const) { + stream.on("error", () => process.exit(1)); + } + + function reportFatal(label: string, getDetail: () => string): void { + try { + let detail: string; + try { + detail = getDetail(); + } catch { + detail = ""; + } + process.stderr.write(`[argent] ${label}: ${detail}\n`); + } catch { + process.exit(1); + } + if (!opts.isMcpServer) process.exit(1); + } + + process.on("uncaughtException", (err) => { + reportFatal("Uncaught exception", () => String((err as Error)?.stack ?? err)); + }); + process.on("unhandledRejection", (reason) => { + reportFatal("Unhandled rejection", () => + reason instanceof Error ? (reason.stack ?? reason.message) : String(reason) + ); + }); +} diff --git a/packages/argent/test/fatal-handlers.test.ts b/packages/argent/test/fatal-handlers.test.ts new file mode 100644 index 00000000..ce7e9348 --- /dev/null +++ b/packages/argent/test/fatal-handlers.test.ts @@ -0,0 +1,141 @@ +import { spawn } from "node:child_process"; +import * as path from "node:path"; +import { describe, expect, it } from "vitest"; + +// Resolves to dist/fatal-handlers.js — the test runs the *built* artifact +// inside child node processes so it exercises the same code that ships. +const DIST_HANDLER = path.resolve(import.meta.dirname, "../dist/fatal-handlers.js"); + +interface RunResult { + exitCode: number | null; + signal: NodeJS.Signals | null; + stdout: string; + stderr: string; + durationMs: number; +} + +function runChild(scriptBody: string, timeoutMs = 3_000): Promise { + return new Promise((resolve, reject) => { + const start = Date.now(); + const child = spawn(process.execPath, ["--input-type=module", "-e", scriptBody], { + stdio: ["ignore", "pipe", "pipe"], + }); + let stdout = ""; + let stderr = ""; + child.stdout.on("data", (chunk) => (stdout += chunk.toString())); + child.stderr.on("data", (chunk) => (stderr += chunk.toString())); + const killer = setTimeout(() => { + child.kill("SIGKILL"); + reject( + new Error(`child still running after ${timeoutMs}ms — fix is broken; output: ${stderr}`) + ); + }, timeoutMs); + child.on("exit", (code, signal) => { + clearTimeout(killer); + resolve({ + exitCode: code, + signal, + stdout, + stderr, + durationMs: Date.now() - start, + }); + }); + child.on("error", (err) => { + clearTimeout(killer); + reject(err); + }); + }); +} + +const HANDLER_URL = JSON.stringify(`file://${DIST_HANDLER}`); + +describe("installFatalHandlers", () => { + it("exits cleanly when stderr is broken (the orphaned-process loop scenario)", async () => { + // Reproduces the production bug: an orphaned MCP process whose stderr + // pipe broke entered an exception loop. Each stderr.write emitted an + // async 'error' event, which without a listener became another + // uncaughtException, calling the handler again, ... + const script = ` + import { installFatalHandlers } from ${HANDLER_URL}; + const origWrite = process.stderr.write.bind(process.stderr); + let writes = 0; + process.stderr.write = (chunk, ...rest) => { + writes++; + if (writes > 50) { + // Hardstop: if we get here the fix is broken. + process._rawDebug("LOOP NOT FIXED writes=" + writes); + process.exit(99); + } + // Synthesize the broken-pipe behavior: every write emits an async error. + setImmediate(() => process.stderr.emit("error", new Error("synthetic EPIPE"))); + return true; + }; + installFatalHandlers({ isMcpServer: true }); + throw new Error("initial boom"); + `; + const result = await runChild(script); + expect(result.exitCode).toBe(1); + expect(result.durationMs).toBeLessThan(2_000); + }); + + it("exits with code 1 in non-mcp mode on uncaught exception", async () => { + const script = ` + import { installFatalHandlers } from ${HANDLER_URL}; + installFatalHandlers({ isMcpServer: false }); + throw new Error("boom"); + `; + const result = await runChild(script); + expect(result.exitCode).toBe(1); + expect(result.stderr).toContain("[argent] Uncaught exception"); + expect(result.stderr).toContain("boom"); + }); + + it("does NOT exit in mcp mode when stderr works (transient errors stay non-fatal)", async () => { + // Regression guard: the bug fix must not change the existing "MCP server + // keeps running on uncaught exceptions" semantics. After throwing once, + // the process should still be alive and able to do work. + const script = ` + import { installFatalHandlers } from ${HANDLER_URL}; + installFatalHandlers({ isMcpServer: true }); + process.nextTick(() => { throw new Error("transient"); }); + setTimeout(() => { + process.stdout.write("STILL_ALIVE"); + process.exit(0); + }, 200); + `; + const result = await runChild(script); + expect(result.exitCode).toBe(0); + expect(result.stdout).toContain("STILL_ALIVE"); + expect(result.stderr).toContain("[argent] Uncaught exception"); + expect(result.stderr).toContain("transient"); + }); + + it("formats unhandled rejections", async () => { + const script = ` + import { installFatalHandlers } from ${HANDLER_URL}; + installFatalHandlers({ isMcpServer: false }); + Promise.reject(new Error("rejected-thing")); + `; + const result = await runChild(script); + expect(result.exitCode).toBe(1); + expect(result.stderr).toContain("[argent] Unhandled rejection"); + expect(result.stderr).toContain("rejected-thing"); + }); + + it("survives errors thrown from inside the stack-formatter (defaultPrepareStackTrace)", async () => { + // Real production trace showed time spent in defaultPrepareStackTrace, + // suggesting the original error's .stack getter was throwing. The fix + // wraps the formatter in a try/catch so a failing .stack does not start + // the loop. + const script = ` + import { installFatalHandlers } from ${HANDLER_URL}; + installFatalHandlers({ isMcpServer: false }); + const err = new Error("boom"); + Object.defineProperty(err, "stack", { get() { throw new Error("stack-getter-blew-up"); } }); + throw err; + `; + const result = await runChild(script); + expect(result.exitCode).toBe(1); + expect(result.stderr).toContain("[argent] Uncaught exception"); + }); +}); From 491587bb177509c31d41a3abe09be5fd5d043fd7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ignacy=20=C5=81=C4=85tka?= Date: Wed, 6 May 2026 18:53:34 +0200 Subject: [PATCH 2/4] test: build dispatcher before vitest so npm test works cold MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `fatal-handlers.test.ts` imports the built dist/fatal-handlers.js so it exercises the same artifact that ships, but `npm test` would fail with ERR_MODULE_NOT_FOUND if the dispatcher hadn't been built first. CI is fine — it runs `tsc --build` at the workspace level — but local devs running `npm test` cold hit a confusing error. Add `pretest`/`pretest:watch` hooks that run `tsc` (incremental, non-destructive — unlike `build:dispatcher` which `rm -rf dist` would nuke any bundle outputs the dev had built). --- packages/argent/package.json | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/argent/package.json b/packages/argent/package.json index 5ee77ed7..e007c05a 100644 --- a/packages/argent/package.json +++ b/packages/argent/package.json @@ -18,7 +18,9 @@ "build:bundles": "node scripts/bundle-tools.cjs", "sync-readme": "node scripts/sync-readme.cjs", "prepack": "node scripts/sync-readme.cjs", + "pretest": "tsc", "test": "vitest run", + "pretest:watch": "tsc", "test:watch": "vitest", "observe": "node scripts/observe.cjs", "benchmark": "node scripts/benchmark.cjs", From 7abeaba753cadfda25668f2a88bfb8e83cf3c086 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ignacy=20=C5=81=C4=85tka?= Date: Thu, 7 May 2026 14:12:51 +0200 Subject: [PATCH 3/4] revert package.json --- packages/argent/package.json | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/argent/package.json b/packages/argent/package.json index e007c05a..5ee77ed7 100644 --- a/packages/argent/package.json +++ b/packages/argent/package.json @@ -18,9 +18,7 @@ "build:bundles": "node scripts/bundle-tools.cjs", "sync-readme": "node scripts/sync-readme.cjs", "prepack": "node scripts/sync-readme.cjs", - "pretest": "tsc", "test": "vitest run", - "pretest:watch": "tsc", "test:watch": "vitest", "observe": "node scripts/observe.cjs", "benchmark": "node scripts/benchmark.cjs", From 2cf7ab073fa75c630ff142d49371c2b850817f4f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ignacy=20=C5=81=C4=85tka?= Date: Thu, 7 May 2026 14:46:12 +0200 Subject: [PATCH 4/4] test: bundle fatal-handlers from src so the test is self-contained MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Drop the pretest/pretest:watch hooks added in the previous commit. Instead of importing from dist/fatal-handlers.js (which required a prior build), the test now uses esbuild — already a devDependency — to transform src/fatal-handlers.ts into a tmp .mjs once in beforeAll, and spawned children import from there. `npm test` now works cold without touching dist/, no build dependency on the test, no script-hook artifacts. --- packages/argent/test/fatal-handlers.test.ts | 42 +++++++++++++++------ 1 file changed, 31 insertions(+), 11 deletions(-) diff --git a/packages/argent/test/fatal-handlers.test.ts b/packages/argent/test/fatal-handlers.test.ts index ce7e9348..44290851 100644 --- a/packages/argent/test/fatal-handlers.test.ts +++ b/packages/argent/test/fatal-handlers.test.ts @@ -1,10 +1,32 @@ import { spawn } from "node:child_process"; +import * as fs from "node:fs"; +import * as os from "node:os"; import * as path from "node:path"; -import { describe, expect, it } from "vitest"; +import * as esbuild from "esbuild"; +import { afterAll, beforeAll, describe, expect, it } from "vitest"; -// Resolves to dist/fatal-handlers.js — the test runs the *built* artifact -// inside child node processes so it exercises the same code that ships. -const DIST_HANDLER = path.resolve(import.meta.dirname, "../dist/fatal-handlers.js"); +// Compile src/fatal-handlers.ts to a tmp .mjs once before all tests so the +// spawned child node processes can `import` it without depending on a prior +// `npm run build` step. Self-contained: no dist/ build dependency. +let handlerUrl = ""; +let tmpDir = ""; + +beforeAll(async () => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "argent-fatal-handlers-")); + const srcPath = path.resolve(import.meta.dirname, "../src/fatal-handlers.ts"); + const out = await esbuild.transform(fs.readFileSync(srcPath, "utf8"), { + loader: "ts", + format: "esm", + target: "node20", + }); + const handlerPath = path.join(tmpDir, "fatal-handlers.mjs"); + fs.writeFileSync(handlerPath, out.code); + handlerUrl = JSON.stringify(`file://${handlerPath}`); +}); + +afterAll(() => { + if (tmpDir) fs.rmSync(tmpDir, { recursive: true, force: true }); +}); interface RunResult { exitCode: number | null; @@ -47,8 +69,6 @@ function runChild(scriptBody: string, timeoutMs = 3_000): Promise { }); } -const HANDLER_URL = JSON.stringify(`file://${DIST_HANDLER}`); - describe("installFatalHandlers", () => { it("exits cleanly when stderr is broken (the orphaned-process loop scenario)", async () => { // Reproduces the production bug: an orphaned MCP process whose stderr @@ -56,7 +76,7 @@ describe("installFatalHandlers", () => { // async 'error' event, which without a listener became another // uncaughtException, calling the handler again, ... const script = ` - import { installFatalHandlers } from ${HANDLER_URL}; + import { installFatalHandlers } from ${handlerUrl}; const origWrite = process.stderr.write.bind(process.stderr); let writes = 0; process.stderr.write = (chunk, ...rest) => { @@ -80,7 +100,7 @@ describe("installFatalHandlers", () => { it("exits with code 1 in non-mcp mode on uncaught exception", async () => { const script = ` - import { installFatalHandlers } from ${HANDLER_URL}; + import { installFatalHandlers } from ${handlerUrl}; installFatalHandlers({ isMcpServer: false }); throw new Error("boom"); `; @@ -95,7 +115,7 @@ describe("installFatalHandlers", () => { // keeps running on uncaught exceptions" semantics. After throwing once, // the process should still be alive and able to do work. const script = ` - import { installFatalHandlers } from ${HANDLER_URL}; + import { installFatalHandlers } from ${handlerUrl}; installFatalHandlers({ isMcpServer: true }); process.nextTick(() => { throw new Error("transient"); }); setTimeout(() => { @@ -112,7 +132,7 @@ describe("installFatalHandlers", () => { it("formats unhandled rejections", async () => { const script = ` - import { installFatalHandlers } from ${HANDLER_URL}; + import { installFatalHandlers } from ${handlerUrl}; installFatalHandlers({ isMcpServer: false }); Promise.reject(new Error("rejected-thing")); `; @@ -128,7 +148,7 @@ describe("installFatalHandlers", () => { // wraps the formatter in a try/catch so a failing .stack does not start // the loop. const script = ` - import { installFatalHandlers } from ${HANDLER_URL}; + import { installFatalHandlers } from ${handlerUrl}; installFatalHandlers({ isMcpServer: false }); const err = new Error("boom"); Object.defineProperty(err, "stack", { get() { throw new Error("stack-getter-blew-up"); } });