Skip to content
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 10 additions & 8 deletions test/js/bun/http/req-url-leak-fixture.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

78 changes: 40 additions & 38 deletions test/js/bun/http/req-url-leak.test.ts
Original file line number Diff line number Diff line change
@@ -1,57 +1,59 @@
import { expect, test } from "bun:test";
import { bunEnv, bunExe, isASAN } from "harness";
import { bunEnv, bunExe, isASAN, isDebug } from "harness";
import path from "path";

test(
"req.url doesn't leak memory",
async () => {
const { promise, resolve } = Promise.withResolvers();
await using process = Bun.spawn({
const { promise, resolve, reject } = Promise.withResolvers<string>();
await using proc = Bun.spawn({
cmd: [bunExe(), path.join(import.meta.dir, "req-url-leak-fixture.js")],
env: bunEnv,
stdout: "inherit",
stderr: "inherit",
stdin: "inherit",
ipc(message, child) {
if (message.url) {
resolve(message.url);
}
stdout: "pipe",
stderr: "pipe",
ipc(message) {
if (message.url) resolve(message.url);
},
});
proc.exited.then(code => reject(new Error(`req-url-leak fixture exited (${code}) before sending its URL`)));

Check warning on line 18 in test/js/bun/http/req-url-leak.test.ts

View check run for this annotation

Claude / Claude Code Review

Fixture stderr swallowed on early-exit path

On the early-exit path (`proc.exited` → reject → `await promise` throws at line 20), `proc.stderr` is never drained — only the exit code appears in the error, so the fixture's actual stack trace / panic text is discarded. Before this PR stderr was `"inherit"` and the crash output landed in CI logs, so this is a small diagnosability regression on exactly the path the new fail-fast wiring targets. Consider making the `.then` callback async and folding stderr into the message: ``reject(new Error(`r
Comment thread
robobun marked this conversation as resolved.
Resolved

const baseURL = await promise;

const url = new URL(Buffer.alloc(1024 * 15, "Z").toString(), baseURL);
// POST = GC-then-report RSS (routed by method so the long-URL GET path keeps
// the exact #16787 scenario: no req.url access in the handler).
const report = async () => parseInt(await (await fetch(baseURL, { method: "POST" })).text());

let maxRSS = 0;

for (let i = 0; i < 256; i++) {
const batchSize = 64;
const promises = [];
for (let j = 0; j < batchSize; j++) {
promises.push(
fetch(url)
.then(r => r.text())
.then(rssText => {
const rss = parseFloat(rssText);
if (Number.isSafeInteger(rss)) {
maxRSS = Math.max(maxRSS, rss);
}
}),
);
// 6 windows of 8×64 = 3072 GETs total. A leaked 15 KB url per request grows
// RSS linearly; between samples[1] (after 1024 requests, allocator warmed up)
// and samples[5] that is 2048×15 KB ≈ 30 MB. With the fix the post-GC samples
// are flat within ±3 MB on ASAN, so the old 16 K sweep's absolute-RSS check
// becomes a growth bound at a fifth of the requests.
const batchSize = 64;
const samples: number[] = [];
for (let window = 0; window < 6; window++) {
for (let i = 0; i < 8; i++) {
const batch = new Array(batchSize);
for (let j = 0; j < batchSize; j++) batch[j] = fetch(url).then(r => r.text());
await Promise.all(batch);
}
await Promise.all(promises);
samples.push(await report());
}

console.log("Max RSS", (maxRSS / 1024 / 1024) | 0, "MB");
proc.kill();
const [stderr] = await Promise.all([proc.stderr.text(), proc.stdout.text(), proc.exited]);

const growthMB = (samples.at(-1)! - samples[1]) / 1024 / 1024;
console.log(
"RSS samples (MB):",
samples.map(s => (s / 1024 / 1024) | 0).join(" "),
"growth:",
growthMB.toFixed(1) + "MB",
);

// 297 MB on Bun 1.2
// 44 MB on Bun 1.3
// ASAN's quarantine + shadow memory raise the absolute RSS floor; widen there.
expect(maxRSS).toBeLessThan(1024 * 1024 * (isASAN ? 450 : 150));
// Same calibration for the deadline: the 16K fetches that finish in ~3s on a
// release build run ~10x slower under ASAN's instrumentation (observed
// ~12-13s), and the run used to die on this timeout with the RSS assertion
// itself passing.
expect(stderr).toBe("");
// 297 MB on Bun 1.2 vs 44 MB on Bun 1.3 for the old 16 K absolute-RSS check;
// as a post-GC delta the leak shows as ~30 MB here versus ~0 on a fixed build.
expect(growthMB).toBeLessThan(15);
},
isASAN ? 90_000 : 10_000,
isASAN || isDebug ? 90_000 : 10_000,
);
Loading