Skip to content

Commit cec481c

Browse files
committed
test: make the two retention repairs observable instead of merely correct
The audit's second round showed both resource fixes were mutation-surviving: restoring the per-chunk accumulator, or the shared Promise.race companion, left every test green. Neither property is visible through behavior — both shapes relay and reassemble identically — so each needed its own observable. bounded-body now counts buffer reallocations for tests. A geometric buffer grows a handful of times regardless of how the peer fragments the body; an exact-fit accumulator grows once per chunk, which is the retention shape the repair removed. The new test compares 20k one-byte chunks against the same body in one chunk and pins growth to a small constant. The eager relay's property is structural, so it is pinned structurally, the way this repository already pins the star-consent guard: neither relay may race a read against a shared abort promise (comments stripped first — both files describe the banned shape in prose), and the eager producer must keep the reader-cancel wake-up that replaced it. Both driven red by restoring the old implementations.
1 parent ef851ae commit cec481c

3 files changed

Lines changed: 65 additions & 0 deletions

File tree

src/lib/bounded-body.ts

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,19 @@ export interface BoundedBodyResult {
3939
const TOTAL_TIMEOUT = Symbol("bounded body total timeout");
4040
const INACTIVITY_TIMEOUT = Symbol("bounded body inactivity timeout");
4141

42+
/**
43+
* Test-only instrumentation: how many times the retained buffer was reallocated
44+
* during the most recent read. The accumulator grows geometrically, so this is
45+
* logarithmic in the body size and independent of how many chunks the peer sends.
46+
* The per-chunk array it replaced retained one object per chunk instead, which a
47+
* fragmenting peer can inflate far past the payload ceiling — a property no
48+
* correctness assertion can see, which is why it is observable here.
49+
*/
50+
let bufferGrowthsForTests = 0;
51+
export function boundedBodyBufferGrowthsForTests(): number {
52+
return bufferGrowthsForTests;
53+
}
54+
4255
function timeoutPromise(ms: number, value: symbol): { promise: Promise<symbol>; clear: () => void } {
4356
let timer: ReturnType<typeof setTimeout> | undefined;
4457
const promise = new Promise<symbol>((resolve) => {
@@ -105,6 +118,7 @@ export async function readBoundedResponseBody(
105118
// beyond the payload ceiling on large budgets.
106119
let retained = new Uint8Array(Math.min(maxBytes, 64 * 1024));
107120
let retainedBytes = 0;
121+
bufferGrowthsForTests = 0;
108122
let mustCancel = false;
109123
let cancelReason: unknown;
110124
const total = timeoutPromise(options.totalTimeoutMs ?? BOUNDED_BODY_TIMEOUT_MS, TOTAL_TIMEOUT);
@@ -197,6 +211,7 @@ export async function readBoundedResponseBody(
197211
);
198212
grown.set(retained.subarray(0, retainedBytes));
199213
retained = grown;
214+
bufferGrowthsForTests += 1;
200215
}
201216
retained.set(value, retainedBytes);
202217
retainedBytes += value.byteLength;

tests/bounded-body.test.ts

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import { describe, expect, test } from "bun:test";
22
import {
33
BOUNDED_BODY_MAX_BYTES,
4+
boundedBodyBufferGrowthsForTests,
45
readBoundedResponseBody,
56
} from "../src/lib/bounded-body";
67

@@ -162,6 +163,32 @@ describe("readBoundedResponseBody", () => {
162163
expect(result.text).toBe("a".repeat(chunkCount));
163164
expect(result.oversized).toBe(false);
164165
});
166+
167+
test("retention is logarithmic in the body, not linear in the chunk count", async () => {
168+
// The accumulator is the security property, and correctness cannot see it:
169+
// a per-chunk array reassembles identically while retaining one object per
170+
// transport chunk, which a fragmenting peer inflates far past the payload
171+
// ceiling. Growth count is the observable that separates the two — a single
172+
// geometric buffer doubles a handful of times regardless of fragmentation.
173+
const fine = Array.from({ length: 20_000 }, () => new Uint8Array([0x61]));
174+
await readBoundedResponseBody(responseFromChunks(...fine), { maxBytes: CUSTOM_CAP });
175+
const fineGrowths = boundedBodyBufferGrowthsForTests();
176+
177+
const coarse = [new Uint8Array(20_000).fill(0x61)];
178+
await readBoundedResponseBody(responseFromChunks(...coarse), { maxBytes: CUSTOM_CAP });
179+
const coarseGrowths = boundedBodyBufferGrowthsForTests();
180+
181+
// 20k one-byte chunks fit inside the 64 KiB seed: no growth at all, and the
182+
// same body delivered as one chunk behaves identically.
183+
expect(fineGrowths).toBe(coarseGrowths);
184+
expect(fineGrowths).toBeLessThanOrEqual(2);
185+
186+
// Past the seed, growth stays logarithmic: doubling from 64 KiB to 256 KiB is
187+
// two reallocations no matter how the peer fragments it.
188+
const big = Array.from({ length: 256 }, () => new Uint8Array(1024).fill(0x61));
189+
await readBoundedResponseBody(responseFromChunks(...big), { maxBytes: CUSTOM_CAP });
190+
expect(boundedBodyBufferGrowthsForTests()).toBeLessThanOrEqual(4);
191+
});
165192
});
166193

167194
test("parent abort rejects with the exact reason object", async () => {

tests/relay-eager.test.ts

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,29 @@ async function readAll(stream: ReadableStream<Uint8Array>): Promise<string> {
111111
}
112112

113113
describe("relaySseEagerBounded — inline payload rewrite (#864)", () => {
114+
test("neither relay races reads against a shared abort promise", async () => {
115+
// Retention shape, not behavior: racing every read against ONE never-settled
116+
// promise attaches a reaction per completed read and holds it until abort, so a
117+
// long stream retains O(chunk-count) callbacks. Both relays relay identically
118+
// either way, which is exactly why no behavioral assertion catches a regression
119+
// here — relay.ts already states the rule in prose at its own drain, and this
120+
// pins it for both files. The sanctioned shape is: cancel the reader on abort.
121+
const eager = await Bun.file(new URL("../src/server/relay-eager.ts", import.meta.url)).text();
122+
const relay = await Bun.file(new URL("../src/server/relay.ts", import.meta.url)).text();
123+
124+
// Strip comments first: both files DESCRIBE the banned shape in prose, and the
125+
// rule is about the code, not the explanation of why the code avoids it.
126+
const stripComments = (source: string): string =>
127+
source.replace(/\/\*[\s\S]*?\*\//g, "").replace(/(^|[^:])\/\/.*$/gm, "$1");
128+
129+
for (const [name, source] of [["relay-eager.ts", eager], ["relay.ts", relay]] as const) {
130+
const racesReads = /Promise\.race\(\s*\[\s*reader\.read\(\)/.test(stripComments(source));
131+
expect(`${name} races reads: ${racesReads}`).toBe(`${name} races reads: false`);
132+
}
133+
// And the eager producer must keep the reader-cancel wake-up that replaced it.
134+
expect(eager).toMatch(/reader\.cancel\(upstream\.signal\.reason\)/);
135+
});
136+
114137
test("a terminal frame settling in the same tick as abort is still recorded", async () => {
115138
// Post-cancel drain: the terminal arrives, and the drain deadline aborts
116139
// upstream in the same tick. Honoring the signal before examining the settled

0 commit comments

Comments
 (0)