From 175219856f2727474fd2f8d53811d668b2840998 Mon Sep 17 00:00:00 2001 From: Kirill Klimuk Date: Fri, 25 Sep 2026 10:56:51 -0700 Subject: [PATCH] fix: flush large piped output without duplication or hangs --- .github/workflows/ci.yml | 15 +++ CLAUDE.md | 2 +- CONTRIBUTING.md | 3 +- src/cli/respond.ts | 32 ++--- tests/cli/CLAUDE.md | 5 +- tests/cli/output-contract.test.ts | 203 ++++++++++++++++++++++++++++++ 6 files changed, 243 insertions(+), 17 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6db563d..d76753d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -44,6 +44,21 @@ jobs: - run: bun install --frozen-lockfile - run: bun run test:unit + macos-output: + name: macOS output contract (Bun ${{ matrix.bun-version }}) + runs-on: macos-latest + strategy: + fail-fast: false + matrix: + bun-version: ["1.3.10", "latest"] + steps: + - uses: actions/checkout@v6 + - uses: oven-sh/setup-bun@v2 + with: + bun-version: ${{ matrix.bun-version }} + - run: bun install --frozen-lockfile + - run: bun test tests/cli/output-contract.test.ts + integration-tests: name: Integration Tests (LibreOffice) runs-on: ubuntu-latest diff --git a/CLAUDE.md b/CLAUDE.md index 796aef2..7437ab9 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -10,7 +10,7 @@ Subsystem-specific guidance lives in nested CLAUDE.md files that load when you e These conventions are NOT SUGGESTIONS. These are rules. -- **All stdout goes through `respond()` (JSON ack) or `writeStdout()` (text)** from `src/cli/respond.ts` — never `process.stdout.write`. Both use `Bun.write(Bun.stdout, ...)`; the 64 KB truncation that bites on early exit is real and silent, and these helpers are the only safe path. +- **All stdout goes through `respond()` (JSON ack) or `writeStdout()` (text)** from `src/cli/respond.ts` — never `process.stdout.write`. The production sinks use lazy `Bun.stdout.writer()` / `Bun.stderr.writer()` FileSinks and await each write and flush before exit. Never switch these to `Bun.write(Bun.stdout, ...)` or `Bun.stdout.write(...)`: large OS-pipe writes can repeat bytes or hang after a dependency accesses `process.stdout` (issue #8). Stderr goes through `writeStderr()` for the same reason. - **File naming**: kebab-case, named after the primary export (`xml-node.ts` → `XmlNode`). - **Newspaper ordering.** The entry point (primary export) goes at the top; its dependencies follow in the order it uses them, then _their_ dependencies, and so on — a file reads top-to-bottom like a newspaper. Use hoisted `function` declarations for internal helpers so this works at runtime; arrow functions only for inline callbacks and short utilities. Types are usually not the primary exports and should go below the functions/classes that are. - **Feature nesting** When a file accumulates too many dependencies to be read well with newspaper ordering (> 300 lines), split them into a separate folder/file named after the feature they're working on. It should be a folder if it is going to represent a logical feature of dependencies. This nesting can continue indefinitely if subfeatures have subfeatures of their own. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index c1dca77..a7d2616 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -95,12 +95,13 @@ version-independent; on a release version bump, also bump `version` in ## CI -GitHub Actions (`.github/workflows/ci.yml`) runs four jobs on push to `main` and on PRs: +GitHub Actions (`.github/workflows/ci.yml`) runs these jobs on push to `main` and on PRs: | Job | What | | ------------------- | ----------------------------------------------------------- | | `check` | `biome check . && knip-bun && tsc --noEmit` | | `unit-tests` | `bun run test:unit` (core + cli, fast) | +| `macos-output` | Output contract and real-pipe regressions on macOS with Bun 1.3.10 and latest | | `integration-tests` | Installs LibreOffice, runs `bun run test:integration` | | `build-binary` | Smoke-builds via `bun build --compile` and runs `--version` | diff --git a/src/cli/respond.ts b/src/cli/respond.ts index 1d52e4d..e0a4c90 100644 --- a/src/cli/respond.ts +++ b/src/cli/respond.ts @@ -41,20 +41,24 @@ export type ErrorCode = | "UNSUPPORTED" | "UNHANDLED"; -// Output sinks. Production leaves these null and writes straight to the real -// streams; the test harness redirects them to run the CLI in-process (no -// subprocess spawn). All CLI output funnels through here, so capturing these -// two captures everything. -const stdout = async (text: string) => { - await Bun.stdout.write(text); -}; -const stderr = async (text: string) => { - await Bun.stderr.write(text); -}; -const sinks = { - stdout, - stderr, -}; +// All CLI output goes through these sinks so in-process tests can capture it. +// Create FileSinks lazily: importing this module under captureOutput should +// neither open writers nor change the real streams' lifecycle. +const stdout = outputSink(Bun.stdout); +const stderr = outputSink(Bun.stderr); +const sinks = { stdout, stderr }; + +function outputSink(file: Bun.BunFile): (text: string) => Promise { + let writer: Bun.FileSink | undefined; + return async (text) => { + writer ??= file.writer(); + // BunFile.write/Bun.write can repeat bytes or spin on a large pipe write + // after process.stdout/stderr is accessed (issue #8). FileSink handles + // partial writes; flush MUST finish before src/index.ts calls exit(). + await writer.write(text); + await writer.flush(); + }; +} /** Redirect CLI stdout/stderr (for in-process testing). */ export function captureOutput( diff --git a/tests/cli/CLAUDE.md b/tests/cli/CLAUDE.md index ef9a17d..e8d70e5 100644 --- a/tests/cli/CLAUDE.md +++ b/tests/cli/CLAUDE.md @@ -27,7 +27,10 @@ only when the thing under test holds _regardless of which verb runs_: (resolve-first / re-read-between / pin-then-splice). - [output-contract.test.ts](output-contract.test.ts) — the output **contract** (quiet-vs-verbose, exit codes, bare-minted-locator, 64 KB boundary, `--help` - matrix), exercised at the real process boundary. + matrix), exercised at the real process boundary. Its bounded POSIX shell-pipe + probes also cover large UTF-8 writes after `process.stdout`/`stderr` is + accessed, flushing before exit, and an early-closing consumer (issue #8). + Ordinary subprocess capture may use sockets and miss the OS-pipe bug. - [invariants.test.ts](invariants.test.ts) — the in-place-mutation **invariant** (unmodeled-XML survival, transparent wrappers, docx validity). - [end-to-end.test.ts](end-to-end.test.ts) — the full document **lifecycle** diff --git a/tests/cli/output-contract.test.ts b/tests/cli/output-contract.test.ts index fe1117a..0d88566 100644 --- a/tests/cli/output-contract.test.ts +++ b/tests/cli/output-contract.test.ts @@ -1,4 +1,5 @@ import { describe, expect, test } from "bun:test"; +import { spawn } from "node:child_process"; import { join } from "node:path"; import { spawnCli as rawCli, @@ -233,6 +234,208 @@ describe("binary smoke (real subprocess)", () => { }); }); +// Use real OS pipes, bounded in both time and bytes: the old BunFile.write +// path could repeat a prefix forever after process.stdout was materialized. +const RESPOND_MODULE = join(import.meta.dir, "../../src/cli/respond.ts"); +const PIPE_PAYLOAD = pipePayload(); + +function pipePayload(): string { + return Array.from( + { length: 12000 }, + (_, index) => `${index}: héllo 漢 🦊\n`, + ).join(""); +} + +function pipeScript(body: string): string { + return ` + import { writeStdout, writeStderr, respond, captureOutput } from ${JSON.stringify(RESPOND_MODULE)}; + const text = (${pipePayload.toString()})(); + ${body} + `; +} + +function runPipeScript( + body: string, + stream: "stdout" | "stderr" = "stdout", + consumer?: string, +) { + return runPipeCommand( + [process.execPath, "-e", pipeScript(body)], + stream, + consumer, + ); +} + +async function runPipeCommand( + command: string[], + stream: "stdout" | "stderr" = "stdout", + consumer = "{ dd bs=1 count=1 2>/dev/null; sleep 0.1; cat; }", +) { + // Bun's subprocess capture can use sockets, which hide this OS-pipe bug. + // bash supplies a real pipe on the chosen descriptor; pipefail preserves the + // producer's exit status. A separate process group lets the bounds kill + // the entire pipeline, even if a regressed producer spins forever. The + // consumer pauses after its first byte so the producer meets backpressure. + const producer = spawn( + "bash", + [ + "-o", + "pipefail", + "-c", + `${stream === "stderr" ? '"$@" 3>&1 1>&2 2>&3' : '"$@"'} | ${consumer}`, + "docx-output-test", + ...command, + ], + { detached: true, stdio: ["ignore", "pipe", "pipe"] }, + ); + const chunks: { stdout: Buffer[]; stderr: Buffer[] } = { + stdout: [], + stderr: [], + }; + let size = 0; + let error: Error | undefined; + function stop(reason: Error) { + error ??= reason; + if (producer.pid) { + try { + process.kill(-producer.pid, "SIGKILL"); + } catch { + /* already exited */ + } + } + } + const timer = setTimeout( + () => stop(new Error("Pipe producer timed out")), + 3000, + ); + for (const stream of ["stdout", "stderr"] as const) { + producer[stream]?.on("data", (chunk: Buffer) => { + size += chunk.length; + if (size > 2 * 1024 * 1024) { + stop(new Error("Pipe producer exceeded output limit")); + return; + } + chunks[stream].push(chunk); + }); + } + try { + const status = await new Promise((resolve, reject) => { + producer.once("error", reject); + producer.once("close", resolve); + }); + const output = { + stdout: Buffer.concat(chunks.stdout).toString("utf8"), + stderr: Buffer.concat(chunks.stderr).toString("utf8"), + }; + // Undo the shell's descriptor swap for a stderr-pipe probe. + if (stream === "stderr") + [output.stdout, output.stderr] = [output.stderr, output.stdout]; + return { status, error, ...output }; + } finally { + clearTimeout(timer); + stop(new Error("Pipe test cleanup")); + } +} + +// bash's OS-pipe setup is POSIX-only. The regular CLI smoke tests above also +// cover Windows; these specifically guard the macOS/Linux pipe regression. +describe.skipIf(process.platform === "win32")( + "output sinks through OS pipes", + () => { + test("large CLI Markdown and AST match captured output through a slow pipe", async () => { + const workspace = tempWorkspace("large-read-pipe"); + const docPath = join(workspace, "large.docx"); + const inputPath = join(workspace, "input.txt"); + await Bun.write(inputPath, PIPE_PAYLOAD); + expect( + (await rawCli("create", docPath, "--text-file", inputPath)).exitCode, + ).toBe(0); + for (const flags of [[], ["--ast"]]) { + const args = ["read", docPath, ...flags]; + const expected = await runCli(...args); + expect(expected.exitCode).toBe(0); + expect(Buffer.byteLength(expected.stdout)).toBeGreaterThan(65536); + const result = await runPipeCommand([ + process.execPath, + join(import.meta.dir, "../../src/index.ts"), + ...args, + ]); + expect(result.error).toBeUndefined(); + expect(result.status).toBe(0); + expect(result.stdout === expected.stdout).toBe(true); + expect(result.stderr).toBe(""); + } + }); + + for (const materialize of [false, true]) { + for (const stream of ["stdout", "stderr"] as const) { + test(`${stream} delivers large UTF-8 writes exactly (process streams touched: ${materialize})`, async () => { + const write = stream === "stdout" ? "writeStdout" : "writeStderr"; + const result = await runPipeScript( + ` + ${materialize ? "void process.stdout; void process.stderr;" : ""} + await ${write}("start\\n"); + await ${write}(text); + await ${write}("end\\n"); + process.exit(0); + `, + stream, + ); + expect(result.error).toBeUndefined(); + expect(result.status, result.stderr.slice(0, 1000)).toBe(0); + expect(result[stream].length).toBe(PIPE_PAYLOAD.length + 10); + expect(result[stream] === `start\n${PIPE_PAYLOAD}end\n`).toBe(true); + expect(result[stream === "stdout" ? "stderr" : "stdout"]).toBe(""); + }); + } + } + + test("respond flushes large JSON before immediate exit", async () => { + const result = await runPipeScript(` + void process.stdout; + await respond({ text }); + process.exit(0); + `); + expect(result.error).toBeUndefined(); + expect(result.status).toBe(0); + expect( + result.stdout === `${JSON.stringify({ text: PIPE_PAYLOAD })}\n`, + ).toBe(true); + }); + + test("captureOutput can restore real sinks and allow natural exit", async () => { + const result = await runPipeScript(` + let captured = ""; + captureOutput(async value => { captured += value; }, async value => { captured += value; }); + await writeStdout("captured-out"); + await writeStderr("captured-err"); + if (captured !== "captured-outcaptured-err") process.exit(9); + captureOutput(); + await writeStdout(text); + await writeStderr("restored"); + `); + expect(result.error).toBeUndefined(); + expect(result.status).toBe(0); + expect(result.stdout === PIPE_PAYLOAD).toBe(true); + expect(result.stderr).toBe("restored"); + }); + + test("a consumer closing early terminates the producer without hanging", async () => { + const result = await runPipeScript( + ` + void process.stdout; + for (let index = 0; index < 100; index++) await writeStdout(text); + process.exit(0); + `, + "stdout", + "head -c 1 > /dev/null", + ); + expect(result.error).toBeUndefined(); + expect(result.status).not.toBe(0); + }); + }, +); + // The full command tree. Every command and sub-verb must answer `--help` with a // usable screen — this is the regression guard for the help-drift bug class // (an implemented flag with no docs, or docs for a flag that doesn't exist).