From f114bf3b0b8b0b03bf78e86cf29d9277ccf8e3b4 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Sun, 9 Aug 2026 18:56:52 -0700 Subject: [PATCH] Make the ripgrep stdout byte cap deterministic across platforms Close could settle as complete output before the last stdout chunk was handled, so Linux CI never tripped the cap. Defer close one turn so queued data runs first, and re-check the cap at every settle path so an over-cap body can never be reported as a full success. --- src/plugins/rg-output.test.ts | 23 +++++++++++++++++++++++ src/plugins/rg-output.ts | 15 +++++++++++++-- src/plugins/rg-run.test.ts | 26 ++++++++++++++++++++++++-- src/plugins/rg-run.ts | 11 ++++++++++- 4 files changed, 70 insertions(+), 5 deletions(-) diff --git a/src/plugins/rg-output.test.ts b/src/plugins/rg-output.test.ts index 77f2250fd..0d94d964a 100644 --- a/src/plugins/rg-output.test.ts +++ b/src/plugins/rg-output.test.ts @@ -41,6 +41,29 @@ test("a cap breach outranks the exit code at every settle point", () => { } }); +// close is its own settle point and must apply the cap even when nothing +// mid-stream did — the Linux race is "all bytes present, exit code mapped +// before the data handler's breach check runs". Simulate that by pushing +// under the collector's settle via a direct close after a push that returns +// partial: push settles first. To hit close's own overCap branch we push +// chunks that the test then settles only through close by using a collector +// whose push already returned partial... which settles. The branch is still +// exercised when push accumulates past the cap without the caller acting on +// the return value and close is the first finish() input — covered below by +// invoking close on a collector that has over-cap bytes only if push did not +// settle. push always settles on breach today, so the equivalent contract is: +// close never returns kind "output" with a body longer than the cap. +test("close never reports complete output over the byte cap", () => { + const collector = createRgCollector(200); + const breach = collector.push(line.repeat(400)); + // Mid-stream path settled; close must not reopen or widen. + expect(breach?.kind).toBe("partial"); + expect(collector.close(0, "")).toBeUndefined(); + if (breach?.kind === "partial") { + expect(breach.stdout.length).toBeLessThanOrEqual(200); + } +}); + test("the timeout yields whatever was collected under the cap", () => { const collector = createRgCollector(2_000); collector.push(line); diff --git a/src/plugins/rg-output.ts b/src/plugins/rg-output.ts index 88c6b4a61..9379d7945 100644 --- a/src/plugins/rg-output.ts +++ b/src/plugins/rg-output.ts @@ -2,8 +2,10 @@ // the timeout fire in a platform-dependent order, so the decision lives here // rather than in the handlers: the collector owns the accumulated bytes and // settles exactly once, whichever handler gets there first. The cap is applied -// to those bytes as they arrive, so an over-cap run can never be reported as a -// complete success and can never hand back more than the cap. +// to those bytes as they arrive and again at process end, so an over-cap run +// can never be reported as a complete success and can never hand back more +// than the cap — even when close races ahead of the data handler that would +// have tripped the mid-stream check. export type RgOutcome = | { kind: "output"; stdout: string } @@ -57,6 +59,12 @@ export function createRgCollector(maxOutputBytes: number): RgCollector { }, close: (code, stderr) => { if (settled) return undefined; + // Cap outranks exit status at process end. If every byte has already + // landed (or a deferred close runs after the data handler accumulated + // past the limit without settling first), partial wins over a complete + // "output" that would otherwise leak the full body. + const capped = overCap(); + if (capped !== undefined) return capped; if (code === 0) return settle({ kind: "output", stdout }); if (code === 1) return settle({ kind: "no-match" }); return settle({ @@ -66,6 +74,9 @@ export function createRgCollector(maxOutputBytes: number): RgCollector { }, timeout: (timeoutMs) => { if (settled) return undefined; + // Same rule as close: never hand back more than the cap on the way out. + const capped = overCap(); + if (capped !== undefined) return capped; return settle({ kind: "partial", stdout, diff --git a/src/plugins/rg-run.test.ts b/src/plugins/rg-run.test.ts index 5e55b2986..5e17f9050 100644 --- a/src/plugins/rg-run.test.ts +++ b/src/plugins/rg-run.test.ts @@ -7,6 +7,8 @@ const line = "big.txt:1:match line here\n"; type Script = { stdout: string[]; code: number | null; + /** When true, fire close before any stdout data (Linux-style race). */ + closeFirst?: boolean; }; // A child whose event order is dictated by the test rather than by how the @@ -29,8 +31,13 @@ function scriptedSpawn(script: Script): SpawnRg { kill: () => undefined, }; queueMicrotask(() => { - script.stdout.forEach((chunk) => onData?.(chunk)); - onClose?.(script.code); + if (script.closeFirst) { + onClose?.(script.code); + script.stdout.forEach((chunk) => onData?.(chunk)); + } else { + script.stdout.forEach((chunk) => onData?.(chunk)); + onClose?.(script.code); + } }); return child; }; @@ -54,6 +61,21 @@ test("an over-cap run is capped regardless of how stdout is chunked", async () = } }); +// The Linux CI failure: process close can be delivered before the last stdout +// chunk is dispatched to the data handler. Ordering is now explicit — close is +// deferred one immediate turn so queued data runs first, and the collector +// re-checks the cap at process end. Either way partial wins over complete +// output when the body is over the limit. +test("an over-cap run is capped when close is ordered before stdout data", async () => { + const bulk = line.repeat(400); + const result = await run({ stdout: [bulk], code: 0, closeFirst: true }); + expect(result.kind).toBe("partial"); + if (result.kind !== "partial") return; + expect(result.stdout.length).toBeLessThanOrEqual(200); + expect(result.stdout).toContain("match line here"); + expect(result.notice).toBeUndefined(); +}); + test("a run under the cap settles as complete output", async () => { const result = await run({ stdout: [line, line], code: 0 }); expect(result).toMatchObject({ kind: "output", stdout: line.repeat(2) }); diff --git a/src/plugins/rg-run.ts b/src/plugins/rg-run.ts index 80c9c6170..d6c460683 100644 --- a/src/plugins/rg-run.ts +++ b/src/plugins/rg-run.ts @@ -87,6 +87,15 @@ export function runRg( if ((err as NodeJS.ErrnoException).code === "ENOENT") finish({ kind: "unavailable" }); else finish({ kind: "error", message: err.message }); }); - child.on("close", (code) => finish(collector.close(code, stderr))); + // Defer close settlement to the next immediate turn so any stdout `data` + // callbacks already queued in this poll phase run first. On Linux CI the + // process `close` event can otherwise win the race against the last pipe + // chunk: finish would settle as complete output before the cap check in + // push ever saw the bytes. setImmediate puts close after those data + // handlers; the collector then either already settled as partial mid-stream + // or close itself re-checks the cap (see rg-output.ts). + child.on("close", (code) => { + setImmediate(() => finish(collector.close(code, stderr))); + }); }); }