Skip to content

Commit f114bf3

Browse files
committed
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.
1 parent b9a1c8b commit f114bf3

4 files changed

Lines changed: 70 additions & 5 deletions

File tree

‎src/plugins/rg-output.test.ts‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,29 @@ test("a cap breach outranks the exit code at every settle point", () => {
4141
}
4242
});
4343

44+
// close is its own settle point and must apply the cap even when nothing
45+
// mid-stream did — the Linux race is "all bytes present, exit code mapped
46+
// before the data handler's breach check runs". Simulate that by pushing
47+
// under the collector's settle via a direct close after a push that returns
48+
// partial: push settles first. To hit close's own overCap branch we push
49+
// chunks that the test then settles only through close by using a collector
50+
// whose push already returned partial... which settles. The branch is still
51+
// exercised when push accumulates past the cap without the caller acting on
52+
// the return value and close is the first finish() input — covered below by
53+
// invoking close on a collector that has over-cap bytes only if push did not
54+
// settle. push always settles on breach today, so the equivalent contract is:
55+
// close never returns kind "output" with a body longer than the cap.
56+
test("close never reports complete output over the byte cap", () => {
57+
const collector = createRgCollector(200);
58+
const breach = collector.push(line.repeat(400));
59+
// Mid-stream path settled; close must not reopen or widen.
60+
expect(breach?.kind).toBe("partial");
61+
expect(collector.close(0, "")).toBeUndefined();
62+
if (breach?.kind === "partial") {
63+
expect(breach.stdout.length).toBeLessThanOrEqual(200);
64+
}
65+
});
66+
4467
test("the timeout yields whatever was collected under the cap", () => {
4568
const collector = createRgCollector(2_000);
4669
collector.push(line);

‎src/plugins/rg-output.ts‎

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,10 @@
22
// the timeout fire in a platform-dependent order, so the decision lives here
33
// rather than in the handlers: the collector owns the accumulated bytes and
44
// settles exactly once, whichever handler gets there first. The cap is applied
5-
// to those bytes as they arrive, so an over-cap run can never be reported as a
6-
// complete success and can never hand back more than the cap.
5+
// to those bytes as they arrive and again at process end, so an over-cap run
6+
// can never be reported as a complete success and can never hand back more
7+
// than the cap — even when close races ahead of the data handler that would
8+
// have tripped the mid-stream check.
79

810
export type RgOutcome =
911
| { kind: "output"; stdout: string }
@@ -57,6 +59,12 @@ export function createRgCollector(maxOutputBytes: number): RgCollector {
5759
},
5860
close: (code, stderr) => {
5961
if (settled) return undefined;
62+
// Cap outranks exit status at process end. If every byte has already
63+
// landed (or a deferred close runs after the data handler accumulated
64+
// past the limit without settling first), partial wins over a complete
65+
// "output" that would otherwise leak the full body.
66+
const capped = overCap();
67+
if (capped !== undefined) return capped;
6068
if (code === 0) return settle({ kind: "output", stdout });
6169
if (code === 1) return settle({ kind: "no-match" });
6270
return settle({
@@ -66,6 +74,9 @@ export function createRgCollector(maxOutputBytes: number): RgCollector {
6674
},
6775
timeout: (timeoutMs) => {
6876
if (settled) return undefined;
77+
// Same rule as close: never hand back more than the cap on the way out.
78+
const capped = overCap();
79+
if (capped !== undefined) return capped;
6980
return settle({
7081
kind: "partial",
7182
stdout,

‎src/plugins/rg-run.test.ts‎

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@ const line = "big.txt:1:match line here\n";
77
type Script = {
88
stdout: string[];
99
code: number | null;
10+
/** When true, fire close before any stdout data (Linux-style race). */
11+
closeFirst?: boolean;
1012
};
1113

1214
// A child whose event order is dictated by the test rather than by how the
@@ -29,8 +31,13 @@ function scriptedSpawn(script: Script): SpawnRg {
2931
kill: () => undefined,
3032
};
3133
queueMicrotask(() => {
32-
script.stdout.forEach((chunk) => onData?.(chunk));
33-
onClose?.(script.code);
34+
if (script.closeFirst) {
35+
onClose?.(script.code);
36+
script.stdout.forEach((chunk) => onData?.(chunk));
37+
} else {
38+
script.stdout.forEach((chunk) => onData?.(chunk));
39+
onClose?.(script.code);
40+
}
3441
});
3542
return child;
3643
};
@@ -54,6 +61,21 @@ test("an over-cap run is capped regardless of how stdout is chunked", async () =
5461
}
5562
});
5663

64+
// The Linux CI failure: process close can be delivered before the last stdout
65+
// chunk is dispatched to the data handler. Ordering is now explicit — close is
66+
// deferred one immediate turn so queued data runs first, and the collector
67+
// re-checks the cap at process end. Either way partial wins over complete
68+
// output when the body is over the limit.
69+
test("an over-cap run is capped when close is ordered before stdout data", async () => {
70+
const bulk = line.repeat(400);
71+
const result = await run({ stdout: [bulk], code: 0, closeFirst: true });
72+
expect(result.kind).toBe("partial");
73+
if (result.kind !== "partial") return;
74+
expect(result.stdout.length).toBeLessThanOrEqual(200);
75+
expect(result.stdout).toContain("match line here");
76+
expect(result.notice).toBeUndefined();
77+
});
78+
5779
test("a run under the cap settles as complete output", async () => {
5880
const result = await run({ stdout: [line, line], code: 0 });
5981
expect(result).toMatchObject({ kind: "output", stdout: line.repeat(2) });

‎src/plugins/rg-run.ts‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,15 @@ export function runRg(
8787
if ((err as NodeJS.ErrnoException).code === "ENOENT") finish({ kind: "unavailable" });
8888
else finish({ kind: "error", message: err.message });
8989
});
90-
child.on("close", (code) => finish(collector.close(code, stderr)));
90+
// Defer close settlement to the next immediate turn so any stdout `data`
91+
// callbacks already queued in this poll phase run first. On Linux CI the
92+
// process `close` event can otherwise win the race against the last pipe
93+
// chunk: finish would settle as complete output before the cap check in
94+
// push ever saw the bytes. setImmediate puts close after those data
95+
// handlers; the collector then either already settled as partial mid-stream
96+
// or close itself re-checks the cap (see rg-output.ts).
97+
child.on("close", (code) => {
98+
setImmediate(() => finish(collector.close(code, stderr)));
99+
});
91100
});
92101
}

0 commit comments

Comments
 (0)