diff --git a/.changeset/lazy-pagers-relax.md b/.changeset/lazy-pagers-relax.md new file mode 100644 index 000000000..eb08fab28 --- /dev/null +++ b/.changeset/lazy-pagers-relax.md @@ -0,0 +1,9 @@ +--- +"hunkdiff": patch +--- + +Fix `hunk pager` pegging a CPU core and growing to gigabytes of memory on large color-heavy +input. Restoring preserved ANSI styling rescanned and reallocated the whole document once per +sequence, so a `git log --graph --color=always` stream from a host like LazyGit took minutes of +solid CPU per process and never produced output. Styling is now restored in a single pass: a 3 MB +branch log pages through in well under a second. diff --git a/.changeset/loud-pipes-deliver.md b/.changeset/loud-pipes-deliver.md new file mode 100644 index 000000000..fb1b33649 --- /dev/null +++ b/.changeset/loud-pipes-deliver.md @@ -0,0 +1,7 @@ +--- +"hunkdiff": patch +--- + +Fix `hunk pager` truncating its output at 64 KB when a host reads it through a pipe, which cut off +large documents for Git's pager contract, LazyGit, and `| less`. Headless commands now hand the +whole document to the stdout descriptor before exiting, so a piped consumer receives every byte. diff --git a/src/core/process/pager.test.ts b/src/core/process/pager.test.ts index 16eeacf1f..84c7d2960 100644 --- a/src/core/process/pager.test.ts +++ b/src/core/process/pager.test.ts @@ -179,6 +179,43 @@ describe("plain text pager fallback", () => { expectNoUnsafeTerminalControls(written); }); + test("pages ANSI-dense git log output promptly and with its color intact", async () => { + // LazyGit and similar hosts point Git's pager at `hunk pager` for whole branch logs, so a + // non-patch `git log --graph --color=always` stream arrives here carrying one SGR sequence + // every ~18 bytes. Sanitizing that used to be quadratic in the sequence count, which pegged a + // core for minutes per concurrent LazyGit job instead of paging the text straight through. + const gitLog = Array.from( + { length: 5_000 }, + (_, index) => + `\x1b[33m* commit ${index}\x1b[m \x1b[1;36m(\x1b[1;32mHEAD\x1b[1;36m)\x1b[m\n` + + `\x1b[32m| Author: someone\x1b[m\n`, + ).join(""); + let written = ""; + + const startedAt = performance.now(); + await pagePlainText( + gitLog, + { PAGER: "less -R" }, + createPagerDeps({ + spawnImpl() { + const pager = new EventEmitter() as EventEmitter & { stdin: PassThrough }; + pager.stdin = new PassThrough(); + pager.stdin.on("data", (chunk) => { + written += String(chunk); + }); + pager.stdin.on("finish", () => { + queueMicrotask(() => pager.emit("close", 0)); + }); + return pager as never; + }, + }), + ); + const elapsedMs = performance.now() - startedAt; + + expect(written).toBe(gitLog); + expect(elapsedMs).toBeLessThan(2_000); + }); + test("spawns pager commands without a shell", async () => { const pager = new EventEmitter() as EventEmitter & { stdin: PassThrough }; pager.stdin = new PassThrough(); diff --git a/src/core/process/pager.ts b/src/core/process/pager.ts index 4b2be67b1..a946ca1d1 100644 --- a/src/core/process/pager.ts +++ b/src/core/process/pager.ts @@ -2,6 +2,7 @@ import { spawn, type ChildProcess, type SpawnOptions } from "node:child_process" import { parse as parseShellCommand, type ParseEntry } from "shell-quote"; import { stripTerminalControl } from "../patch/sanitize"; import { sanitizeTerminalText } from "../../lib/terminalText"; +import { writeStdout } from "./stdout"; /** Detect whether generic pager stdin looks like a diff/patch that Hunk should review. */ export function looksLikePatchInput(text: string) { @@ -146,7 +147,15 @@ export async function pagePlainText( text: string, env: NodeJS.ProcessEnv = process.env, deps: PlainTextPagerDeps = { - stdout: process.stdout, + // Write through the descriptor rather than `process.stdout`: a piped consumer takes one + // buffer at a time, and the caller exits as soon as this returns. + stdout: { + isTTY: process.stdout.isTTY, + write: (chunk) => { + writeStdout(String(chunk)); + return true; + }, + }, spawnImpl: spawn, }, ) { diff --git a/src/core/process/stdout.test.ts b/src/core/process/stdout.test.ts new file mode 100644 index 000000000..7288f89e1 --- /dev/null +++ b/src/core/process/stdout.test.ts @@ -0,0 +1,100 @@ +import { describe, expect, test } from "bun:test"; +import { writeStdout } from "./stdout"; + +/** + * Record descriptor writes, optionally accepting only part of each chunk. + * + * Chunks are kept as bytes and decoded once at the end: a partial write can split a multi-byte + * character, so decoding each chunk on its own would report corruption the descriptor never saw. + */ +function createRecordingWrite(acceptBytes?: number) { + const chunks: Buffer[] = []; + const writeImpl = (_fd: number, buffer: Uint8Array, offset: number, length: number) => { + const written = acceptBytes === undefined ? length : Math.min(acceptBytes, length); + chunks.push(Buffer.from(Buffer.from(buffer).subarray(offset, offset + written))); + return written; + }; + return { chunks, writeImpl, text: () => Buffer.concat(chunks).toString("utf8") }; +} + +/** Build an errno failure the way `writeSync` reports one. */ +function errnoError(code: string) { + return Object.assign(new Error(code), { code }); +} + +describe("writeStdout", () => { + test("hands the whole document to the descriptor", () => { + const recorder = createRecordingWrite(); + + writeStdout("hello pager", { writeImpl: recorder.writeImpl }); + + expect(recorder.text()).toBe("hello pager"); + }); + + test("resumes partial writes until the consumer has taken every byte", () => { + // A pipe accepts one buffer at a time, so a large document is always written in pieces. + const document = "x".repeat(200_000); + const recorder = createRecordingWrite(65_536); + + writeStdout(document, { writeImpl: recorder.writeImpl }); + + expect(recorder.text()).toBe(document); + expect(recorder.chunks.length).toBeGreaterThan(1); + }); + + test("preserves multi-byte characters split across partial writes", () => { + const document = "日本語".repeat(1_000); + const recorder = createRecordingWrite(7); + + writeStdout(document, { writeImpl: recorder.writeImpl }); + + expect(recorder.text()).toBe(document); + }); + + test("waits for room instead of spinning when the descriptor is non-blocking", () => { + const recorder = createRecordingWrite(); + const sleeps: number[] = []; + let refusals = 2; + + writeStdout("deferred", { + writeImpl: (fd, buffer, offset, length) => { + if (refusals > 0) { + refusals -= 1; + throw errnoError("EAGAIN"); + } + return recorder.writeImpl(fd, buffer, offset, length); + }, + sleepImpl: (ms) => sleeps.push(ms), + }); + + expect(recorder.text()).toBe("deferred"); + expect(sleeps).toEqual([1, 1]); + }); + + test("stops quietly when the consumer closes early", () => { + const recorder = createRecordingWrite(4); + + expect(() => + writeStdout("long document", { + writeImpl: (fd, buffer, offset, length) => { + if (offset > 0) { + throw errnoError("EPIPE"); + } + return recorder.writeImpl(fd, buffer, offset, length); + }, + }), + ).not.toThrow(); + + expect(recorder.text()).toBe("long"); + }); + + test("surfaces unexpected descriptor failures", () => { + expect(() => + writeStdout("text", { + writeImpl: () => { + throw errnoError("ENOSPC"); + }, + }), + ).toThrow("ENOSPC"); + }); +}); diff --git a/src/core/process/stdout.ts b/src/core/process/stdout.ts new file mode 100644 index 000000000..d082113af --- /dev/null +++ b/src/core/process/stdout.ts @@ -0,0 +1,48 @@ +/** + * Hands a finished document to stdout and waits for the consumer to take all of it. + * + * Headless commands write their whole document in one call and then exit. Bun's `process.stdout` + * reports no backpressure for a pipe — `write` returns true and `writableLength` stays 0 while only + * one pipe buffer (64 KB on Linux) has actually been handed over — so the exit discarded the rest + * and silently truncated output for every consumer reading through a pipe: Git's pager contract, + * LazyGit, `| less`. A file or a terminal takes the whole document at once, which is why the loss + * only appeared under a pipe. Writing straight to the descriptor blocks until the consumer has + * taken every byte, so the caller can exit as soon as this returns. + */ +import { writeSync } from "node:fs"; + +const STDOUT_FD = 1; +/** Pause before retrying a descriptor that is momentarily full, rather than spinning on it. */ +const NON_BLOCKING_RETRY_MS = 1; + +/** Test seams for descriptor writes; production always targets the real stdout descriptor. */ +export interface WriteStdoutDeps { + writeImpl?: (fd: number, buffer: Uint8Array, offset: number, length: number) => number; + sleepImpl?: (ms: number) => void; +} + +/** Write text to stdout, resuming partial writes until the consumer has taken the whole document. */ +export function writeStdout(text: string, deps: WriteStdoutDeps = {}) { + const write = deps.writeImpl ?? writeSync; + const sleep = deps.sleepImpl ?? Bun.sleepSync; + const buffer = Buffer.from(text, "utf8"); + let offset = 0; + + while (offset < buffer.length) { + try { + offset += write(STDOUT_FD, buffer, offset, buffer.length - offset); + } catch (error) { + const code = (error as NodeJS.ErrnoException).code; + // A consumer that stops reading early (`| head`) leaves nothing left to deliver. + if (code === "EPIPE") { + return; + } + // Only a non-blocking descriptor reports this, and only until it has room again. + if (code === "EAGAIN") { + sleep(NON_BLOCKING_RETRY_MS); + continue; + } + throw error; + } + } +} diff --git a/src/lib/terminalText.test.ts b/src/lib/terminalText.test.ts index 36df3ff62..5ba95c562 100644 --- a/src/lib/terminalText.test.ts +++ b/src/lib/terminalText.test.ts @@ -84,6 +84,30 @@ describe("sanitizeTerminalText", () => { expect(output).toBe("safe0\x1b[31mred\x1b[m"); }); + test("restores dense ANSI styling in linear time", () => { + // `hunk pager` receives whole `git log --graph --color=always` streams from hosts like + // LazyGit: a few megabytes carrying ~170k SGR sequences. Restoring styles one at a time + // rescanned the entire document per sequence, so this input pegged a core for minutes and + // grew to gigabytes. Assert both the styled output and a wall-clock budget that only + // quadratic restoration can exceed. + const commitCount = 5_000; + const input = Array.from( + { length: commitCount }, + (_, index) => + `\x1b[33m* commit ${index}\x1b[m \x1b[1;36m(\x1b[1;32mHEAD\x1b[1;36m)\x1b[m\n` + + `\x1b[32m| Author: someone\x1b[m\n`, + ).join(""); + const sequenceCount = input.match(/\x1b\[[0-9;:]*m/g)?.length ?? 0; + expect(sequenceCount).toBeGreaterThan(25_000); + + const startedAt = performance.now(); + const output = sanitizeTerminalText(input, { preserveAnsiStyle: true }); + const elapsedMs = performance.now() - startedAt; + + expect(output).toBe(input); + expect(elapsedMs).toBeLessThan(2_000); + }); + test("renders path controls as visible escapes without confusing literal backslashes", () => { const output = formatTerminalPath("dir/literal\\t-tab\tline\nescape\x1b"); diff --git a/src/lib/terminalText.ts b/src/lib/terminalText.ts index 9edfc606c..515e2ae3b 100644 --- a/src/lib/terminalText.ts +++ b/src/lib/terminalText.ts @@ -15,6 +15,7 @@ const sevenBitControlStrings = const c1ControlStrings = /[\x90\x98\x9d\x9e\x9f][\s\S]*?(?:\x07|\x1b\\|\x9c)/g; const c1Csi = /\x9b[0-?]*[ -/]*[@-~]/g; const preservedStyleTokenDelimiters = /[\u{f0000}\u{f0001}]/gu; +const preservedStyleTokens = /\u{f0000}(\d+)\u{f0001}/gu; /** Normalize untrusted terminal-bound text before rendering it in Hunk UI surfaces. */ export function sanitizeTerminalText( @@ -51,17 +52,25 @@ export function sanitizeTerminalText( // an internal token that later restores an ANSI sequence at the wrong location. const tokenSafeText = preserveAnsiStyle ? text.replace(preservedStyleTokenDelimiters, "") : text; - let sanitized = tokenSafeText + const sanitized = tokenSafeText .replace(sevenBitControlStrings, preserveStyle) .replace(c1ControlStrings, "") .replace(c1Csi, "") .replace(controlCharacters, ""); - for (const [index, sequence] of preservedStyles.entries()) { - sanitized = sanitized.replaceAll(`\u{f0000}${index}\u{f0001}`, sequence); + if (preservedStyles.length === 0) { + return sanitized; } - return sanitized; + // Restore every placeholder in a single pass. Replacing one style at a time rescans and + // reallocates the whole document per preserved sequence, which is quadratic in ANSI-dense + // input: a few megabytes of `git log --graph --color=always` piped through `hunk pager` + // carries ~170k sequences and would peg a core for minutes while churning gigabytes. + // Input delimiters were stripped above, so every surviving token indexes a captured style. + return sanitized.replace( + preservedStyleTokens, + (_token, index: string) => preservedStyles[Number(index)] ?? "", + ); } /** Sanitize a single terminal row or cell where newlines must never be preserved. */ diff --git a/src/main.tsx b/src/main.tsx index 848f1f905..2e923b5cb 100644 --- a/src/main.tsx +++ b/src/main.tsx @@ -2,6 +2,7 @@ import { formatCliError } from "./core/run/errors"; import { pagePlainText } from "./core/process/pager"; +import { writeStdout } from "./core/process/stdout"; import { prepareStartupPlan } from "./app/startup"; import { sanitizeTerminalText } from "./lib/terminalText"; import { serveSessionBrokerDaemon } from "./session/broker/brokerServer"; @@ -11,7 +12,7 @@ async function main() { const startupPlan = await prepareStartupPlan(); if (startupPlan.kind === "help") { - process.stdout.write(startupPlan.text); + writeStdout(startupPlan.text); process.exit(0); } @@ -27,7 +28,7 @@ async function main() { } if (startupPlan.kind === "session-command") { - process.stdout.write(await runSessionCommand(startupPlan.input)); + writeStdout(await runSessionCommand(startupPlan.input)); process.exit(0); } @@ -40,7 +41,7 @@ async function main() { const canConfirm = Boolean(process.stdin.isTTY) && Boolean(process.stdout.isTTY); process.exit( await runExtensionManageCommand(startupPlan.input, { - stdout: (text) => process.stdout.write(text), + stdout: (text) => writeStdout(text), stderr: (text) => process.stderr.write(text), confirm: canConfirm ? async (question) => { @@ -64,7 +65,7 @@ async function main() { const { runSelfUpdateCommand } = await import("./core/install/selfUpdate"); process.exit( await runSelfUpdateCommand(startupPlan.input, { - stdout: (text) => process.stdout.write(text), + stdout: (text) => writeStdout(text), stderr: (text) => process.stderr.write(text), }), ); @@ -72,14 +73,14 @@ async function main() { if (startupPlan.kind === "markup-guide") { const { runMarkupGuideCommand } = await import("./ui/lib/stml/cli"); - process.exit(runMarkupGuideCommand({ stdout: (text) => process.stdout.write(text) })); + process.exit(runMarkupGuideCommand({ stdout: (text) => writeStdout(text) })); } if (startupPlan.kind === "markup-render") { const { runMarkupRenderCommand } = await import("./ui/lib/stml/cli"); process.exit( await runMarkupRenderCommand(startupPlan.input, { - stdout: (text) => process.stdout.write(text), + stdout: (text) => writeStdout(text), stderr: (text) => process.stderr.write(text), stdoutIsTTY: Boolean(process.stdout.isTTY), readStdinText: () => new Response(Bun.stdin.stream()).text(), @@ -93,7 +94,7 @@ async function main() { } if (startupPlan.kind === "passthrough") { - process.stdout.write( + writeStdout( sanitizeTerminalText(startupPlan.text, { preserveAnsiStyle: startupPlan.preserveColor }), ); process.exit(0); @@ -101,7 +102,7 @@ async function main() { if (startupPlan.kind === "static-diff-pager") { const { renderStaticDiffPager } = await import("./ui/staticDiffPager"); - process.stdout.write( + writeStdout( await renderStaticDiffPager(startupPlan.text, startupPlan.options, { customThemes: startupPlan.customThemes, stderr: process.stderr, diff --git a/src/session/broker/brokerClient.test.ts b/src/session/broker/brokerClient.test.ts index cd6a6cb17..e6a6a0264 100644 --- a/src/session/broker/brokerClient.test.ts +++ b/src/session/broker/brokerClient.test.ts @@ -580,7 +580,7 @@ describe("Hunk session daemon client", () => { appId: "dev.hunk", appRevision: HUNK_SESSION_DAEMON_VERSION, producerEndpoint: `ws://127.0.0.1:${port}/session`, - idleTimeoutMs: 150, + idleTimeoutMs: 0, helloAuthenticator: { async issueChallenge() { helloAttempts += 1; @@ -608,7 +608,7 @@ describe("Hunk session daemon client", () => { try { if ((await fetch(`http://127.0.0.1:${port}/health`)).ok) return; } catch { - // Launch the successor after the incumbent's short test-only quiescent lifetime. + // Launch the successor after the test retires the incompatible incumbent. } successor ??= await serveHunkSessionBrokerDaemon({ idleTimeoutMs: 0 }); }; @@ -620,6 +620,11 @@ describe("Hunk session daemon client", () => { await Bun.sleep(35); expect(helloAttempts).toBe(1); + incumbentDaemon.shutdown(); + await incumbentDaemon.stopped; + incumbent.stop(true); + await incumbent.stopped; + await waitUntil( "successor session registration", async () => { @@ -667,6 +672,7 @@ describe("Hunk session daemon client", () => { expect(clientTestAccess(client).connection).toBe(retainedConnection); } finally { client.stop(); + incumbentDaemon.shutdown(); incumbent.stop(true); const runningSuccessor = successor as Awaited< ReturnType diff --git a/test/cli/pager-pipe-output.test.ts b/test/cli/pager-pipe-output.test.ts new file mode 100644 index 000000000..439fcb93c --- /dev/null +++ b/test/cli/pager-pipe-output.test.ts @@ -0,0 +1,68 @@ +import { describe, expect, test } from "bun:test"; +import { mkdtempSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +// One Linux pipe buffer. Output used to stop here because `process.exit` dropped whatever stdout +// had not yet handed to the consumer, and Bun reports no backpressure to wait on. +const PIPE_BUFFER_BYTES = 65_536; + +/** Build a non-patch document large enough to outgrow several pipe buffers. */ +function createGitLogDocument(lineCount: number) { + return `${Array.from( + { length: lineCount }, + (_, index) => `commit ${String(index).padStart(8, "0")} some subject line for padding`, + ).join("\n")}\n`; +} + +async function readAll(stream: ReadableStream) { + const reader = stream.getReader(); + const chunks: Uint8Array[] = []; + try { + for (;;) { + const next = await reader.read(); + if (next.done) { + break; + } + chunks.push(next.value); + } + } finally { + reader.releaseLock(); + } + return Buffer.concat(chunks).toString("utf8"); +} + +describe("pager output through a pipe", () => { + test("delivers the whole document to a piped consumer", async () => { + const dir = mkdtempSync(join(tmpdir(), "hunk-pager-pipe-")); + const document = createGitLogDocument(6_000); + expect(document.length).toBeGreaterThan(PIPE_BUFFER_BYTES * 3); + + const proc = Bun.spawn(["bun", "run", "src/main.tsx", "--", "pager"], { + cwd: process.cwd(), + stdin: new TextEncoder().encode(document), + stdout: "pipe", + stderr: "pipe", + env: { + ...process.env, + // A captured pager host such as LazyGit reads Hunk's stdout from a pipe. + TERM: "dumb", + GIT_PAGER: "hunk pager", + LAZYGIT_LOG_LEVEL: "info", + HUNK_MCP_DISABLE: "1", + HUNK_DISABLE_UPDATE_NOTICE: "1", + XDG_CONFIG_HOME: dir, + }, + }); + + try { + const [output, exitCode] = await Promise.all([readAll(proc.stdout), proc.exited]); + + expect(exitCode).toBe(0); + expect(output.length).toBeGreaterThan(PIPE_BUFFER_BYTES); + expect(output).toBe(document); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }, 30_000); +}); diff --git a/test/smoke/tty.test.ts b/test/smoke/tty.test.ts index 928278ee8..73a7c0538 100644 --- a/test/smoke/tty.test.ts +++ b/test/smoke/tty.test.ts @@ -263,7 +263,6 @@ async function writeTtyInputUntil( label: string, predicate: (output: string) => boolean, ) { - let attempts = 0; let lastAttemptAt = 0; try { @@ -279,14 +278,13 @@ async function writeTtyInputUntil( throw new Error(`TTY process exited with ${proc.exitCode} before ${label}.`); } - if (attempts < 4 && (attempts === 0 || Date.now() - lastAttemptAt >= 150)) { + if (lastAttemptAt === 0 || Date.now() - lastAttemptAt >= 500) { await writeTtyInput(proc, input); - attempts += 1; lastAttemptAt = Date.now(); } return null; }, - 2_000, + 5_000, 25, ); } catch (error) {