From a4c74e9a20b28ff5faacd89cc7a0c61dbd311841 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sat, 25 Jul 2026 01:02:43 -0700 Subject: [PATCH] fix(mcp): flush stdout/stderr before exiting the CLI to stop large output truncation process.exit() right after a command finished could race an in-flight, asynchronous POSIX write to a piped stdout -- the packaged CHANGELOG.md (>64KB, past the OS pipe buffer size) was getting cut off mid-write when `changelog --json` piped its output to a consumer, breaking JSON parsing on the other end. Add waitForStdioFlush() (packages/loopover-mcp/lib/cli-error.ts) and await it before each process.exit() call in the CLI dispatcher. Setting process.exitCode and relying on a natural exit instead isn't safe either: a parent that spawns this CLI via child_process with default stdio (e.g. Node's own execFile) leaves the child's stdin pipe open, which keeps the event loop alive forever. --- packages/loopover-mcp/bin/loopover-mcp.ts | 13 +++++++--- packages/loopover-mcp/lib/cli-error.ts | 19 ++++++++++++++ test/unit/mcp-cli-error.test.ts | 31 ++++++++++++++++++++++- 3 files changed, 58 insertions(+), 5 deletions(-) diff --git a/packages/loopover-mcp/bin/loopover-mcp.ts b/packages/loopover-mcp/bin/loopover-mcp.ts index 0fb025d8b1..82c1245e13 100644 --- a/packages/loopover-mcp/bin/loopover-mcp.ts +++ b/packages/loopover-mcp/bin/loopover-mcp.ts @@ -42,7 +42,7 @@ import { validateIdeaSubmission, buildTaskGraph, buildClaimPlan } from "@loopove import { z } from "zod"; import { buildBranchAnalysisPayload, collectLocalDiff, collectLocalBranchMetadata, probeLocalScorer, referenceScorePreviewExample, resolveScorePreviewCommand, resolveWorkspaceCwd, sanitizeLocalScorerStatus, setupGuidanceForLocalScorer, isTestFile } from "../lib/local-branch.js"; import { formatTable } from "../lib/format-table.js"; -import { argsWantJson, describeCliError, reportCliFailure } from "../lib/cli-error.js"; +import { argsWantJson, describeCliError, reportCliFailure, waitForStdioFlush } from "../lib/cli-error.js"; import { redactKnownLocalPaths, redactLocalPath } from "../lib/redact-local-path.js"; // Aliased: this file's own recordStdioToolTelemetry is the chokepoint that calls it, and the two names sitting // side by side unaliased would read as the same function (#6238). @@ -1673,14 +1673,19 @@ function stdioToolDescription(name: any) { return tool.description; } -/* v8 ignore next 8 -- the CLI dispatch runs only in the launched process (runAsCliEntrypoint); an in-process - unit importer keeps it false and drives runCli/maintainCli directly instead (mcp-cli-plan-issues.test.ts). */ +/* v8 ignore next 11 -- the CLI dispatch runs only in the launched process (runAsCliEntrypoint); an in-process + unit importer keeps it false and drives runCli/maintainCli directly instead (mcp-cli-plan-issues.test.ts). + waitForStdioFlush itself (#8606, the reason process.exit() is no longer called bare here) is exported and + unit-tested directly in mcp-cli-error.test.ts. */ if (runAsCliEntrypoint && cliArgs[0] !== "--stdio") { try { const exitCode = await runCli(cliArgs); + await waitForStdioFlush([process.stdout, process.stderr]); process.exit(typeof exitCode === "number" ? exitCode : 0); } catch (error) { - process.exit(reportCliFailure(argsWantJson(cliArgs), describeCliError(error), 1)); + const failureExitCode = reportCliFailure(argsWantJson(cliArgs), describeCliError(error), 1); + await waitForStdioFlush([process.stdout, process.stderr]); + process.exit(failureExitCode); } } diff --git a/packages/loopover-mcp/lib/cli-error.ts b/packages/loopover-mcp/lib/cli-error.ts index 6ee1ff290e..58fca2f247 100644 --- a/packages/loopover-mcp/lib/cli-error.ts +++ b/packages/loopover-mcp/lib/cli-error.ts @@ -19,3 +19,22 @@ export function argsWantJson(args: readonly string[]): boolean { export function describeCliError(error: unknown): string { return error instanceof Error ? error.message : String(error); } + +/** A writable stream's own buffered-byte count, e.g. `process.stdout`/`process.stderr`. */ +export type FlushableStream = { readonly writableLength: number }; + +/** #8606: resolves once every stream has finished writing everything queued on it. POSIX writes to a piped + * stdout/stderr (as opposed to a TTY or a file) are asynchronous, so calling `process.exit()` right after a + * large write can terminate the process before the OS has actually drained it, truncating what a piped + * consumer reads (e.g. the CLI's packaged CHANGELOG.md output, >64KB). Callers should await this immediately + * before exiting. `setImmediate` is safe to poll on indefinitely here: it costs nothing while the buffer is + * empty (resolves on the first check) and only re-schedules while real bytes are still in flight. */ +export function waitForStdioFlush(streams: readonly FlushableStream[]): Promise { + return new Promise((resolve) => { + const poll = () => { + if (streams.some((stream) => stream.writableLength > 0)) setImmediate(poll); + else resolve(); + }; + poll(); + }); +} diff --git a/test/unit/mcp-cli-error.test.ts b/test/unit/mcp-cli-error.test.ts index 9819935419..a88f960409 100644 --- a/test/unit/mcp-cli-error.test.ts +++ b/test/unit/mcp-cli-error.test.ts @@ -1,5 +1,5 @@ import { afterEach, describe, expect, it, vi } from "vitest"; -import { argsWantJson, describeCliError, reportCliFailure } from "../../packages/loopover-mcp/lib/cli-error.js"; +import { argsWantJson, describeCliError, reportCliFailure, waitForStdioFlush } from "../../packages/loopover-mcp/lib/cli-error.js"; describe("mcp cli-error (#5928)", () => { afterEach(() => { @@ -39,4 +39,33 @@ describe("mcp cli-error (#5928)", () => { expect(describeCliError("plain")).toBe("plain"); expect(describeCliError(42)).toBe("42"); }); + + describe("waitForStdioFlush (#8606)", () => { + it("resolves without waiting a tick when every stream is already fully drained", async () => { + const settled = vi.fn(); + void waitForStdioFlush([{ writableLength: 0 }, { writableLength: 0 }]).then(settled); + // A poll that finds nothing buffered resolves synchronously inside the executor, before any + // setImmediate is ever scheduled -- only a microtask tick (not a macrotask) should be needed. + await Promise.resolve(); + expect(settled).toHaveBeenCalledTimes(1); + }); + + it("keeps polling until every stream drains, then resolves", async () => { + const stdout = { writableLength: 12 }; + const stderr = { writableLength: 0 }; + const settled = vi.fn(); + void waitForStdioFlush([stdout, stderr]).then(settled); + + await new Promise((resolve) => setImmediate(resolve)); + expect(settled).not.toHaveBeenCalled(); + + stdout.writableLength = 0; + await new Promise((resolve) => setImmediate(resolve)); + expect(settled).toHaveBeenCalledTimes(1); + }); + + it("treats an empty stream list as already flushed", async () => { + await expect(waitForStdioFlush([])).resolves.toBeUndefined(); + }); + }); });