From 3e6a9561162859c4e558597f06430a2252bd7b61 Mon Sep 17 00:00:00 2001 From: robobun Date: Wed, 29 Apr 2026 02:24:31 +0000 Subject: [PATCH 1/3] webcore: free drained buffer in FileReader.onPull memcpy path FileReader.drain() moves ownership of this.buffered (or the reader buffer) into the returned ByteList and leaves the source empty. When onPull copies that data into the JS-provided array it then called this.buffered.clearAndFree(), which is a no-op on the now-empty list, orphaning the moved allocation on every such pull. Free the drained ByteList instead, capturing its length first since deinit() invalidates the struct. --- src/bun.js/webcore/FileReader.zig | 14 ++- .../spawn/spawn-stdout-iterate-leak.test.ts | 102 ++++++++++++++++++ 2 files changed, 111 insertions(+), 5 deletions(-) create mode 100644 test/js/bun/spawn/spawn-stdout-iterate-leak.test.ts diff --git a/src/bun.js/webcore/FileReader.zig b/src/bun.js/webcore/FileReader.zig index c6853d88a06..0d26e3f3d2e 100644 --- a/src/bun.js/webcore/FileReader.zig +++ b/src/bun.js/webcore/FileReader.zig @@ -461,7 +461,7 @@ fn isPulling(this: *const FileReader) bool { pub fn onPull(this: *FileReader, buffer: []u8, array: jsc.JSValue) streams.Result { array.ensureStillAlive(); defer array.ensureStillAlive(); - const drained = this.drain(); + var drained = this.drain(); if (drained.len > 0) { log("onPull({d}) = {d}", .{ buffer.len, drained.len }); @@ -470,13 +470,17 @@ pub fn onPull(this: *FileReader, buffer: []u8, array: jsc.JSValue) streams.Resul this.pending_view = &.{}; if (buffer.len >= @as(usize, drained.len)) { - @memcpy(buffer[0..drained.len], drained.slice()); - this.buffered.clearAndFree(bun.default_allocator); + const drained_len = drained.len; + @memcpy(buffer[0..drained_len], drained.slice()); + // drain() moved ownership of the allocation into `drained` and + // left `this.buffered` / the reader buffer empty, so free + // `drained` here — freeing `this.buffered` would be a no-op. + drained.deinit(bun.default_allocator); if (this.reader.isDone()) { - return .{ .into_array_and_done = .{ .value = array, .len = drained.len } }; + return .{ .into_array_and_done = .{ .value = array, .len = drained_len } }; } else { - return .{ .into_array = .{ .value = array, .len = drained.len } }; + return .{ .into_array = .{ .value = array, .len = drained_len } }; } } diff --git a/test/js/bun/spawn/spawn-stdout-iterate-leak.test.ts b/test/js/bun/spawn/spawn-stdout-iterate-leak.test.ts new file mode 100644 index 00000000000..41f0159654a --- /dev/null +++ b/test/js/bun/spawn/spawn-stdout-iterate-leak.test.ts @@ -0,0 +1,102 @@ +import { expect, test } from "bun:test"; +import { spawn } from "child_process"; +import { once } from "events"; +import { isWindows } from "harness"; + +// Regression test for FileReader.onPull: after `drain()` moves the +// internally buffered data into a local ByteList and the data is memcpy'd +// into the JS-provided pull buffer, that ByteList must be freed. The old +// code freed `this.buffered` instead — but `drain()` had already emptied +// it, so the moved allocation was orphaned on every such pull. +// +// Reaching that branch requires onPull to be called while data is sitting +// in `this.buffered` *and* the JS buffer is large enough to hold it. Most +// consumers route through a JS-side `handle.drain()` guard first, so we +// construct the exact sequence that bypasses it: +// +// - child_process stdout is a NativeReadable whose `_read()` calls +// `ptr.pull()` directly (no drain guard after the first read). +// - With the Readable's highWaterMark set to CHUNK, the first CHUNK-byte +// write fulfils the pending pull via `.into_array` (result == view +// length, so `kRemainingChunk` becomes undefined) and fills the Node +// buffer exactly, so `_read()` is not auto-rescheduled. +// - The second write then lands in `onReadChunk` with no pending pull +// and is appended to `FileReader.buffered`. +// - Draining the Node buffer via `.read()` triggers `_read()` → +// `ptr.pull(CHUNK)` → `onPull`, which now sees `drain().len == CHUNK` +// and takes the memcpy branch. +// +// CHUNK = 32 KiB stays comfortably under the default 64 KiB pipe buffer +// so the round-trip through `cat` never blocks. + +const MB = 1024 * 1024; +const CHUNK = 32768; + +async function run(iters: number) { + const proc = spawn("cat", [], { stdio: ["pipe", "pipe", "ignore"] }); + // Match the Node buffer threshold to CHUNK so one push makes it "full". + (proc.stdout as any)._readableState.highWaterMark = CHUNK; + + const block = Buffer.alloc(CHUNK, 88); + const tick = () => new Promise(resolve => setImmediate(resolve)); + + let total = 0; + // Prime: first chunk is consumed by the one-time JS-side drain in + // NativeReadable's first _read(). + proc.stdin!.write(block); + await once(proc.stdout!, "readable"); + let c: Buffer | null; + while ((c = proc.stdout!.read()) !== null) total += c.length; + await tick(); + await tick(); + + for (let i = 0; i < iters; i++) { + // First write fulfils the pending pull (into_array), second write + // lands in FileReader.buffered. + proc.stdin!.write(block); + await tick(); + await tick(); + proc.stdin!.write(block); + await tick(); + await tick(); + // Draining the Node buffer re-triggers _read → onPull with + // FileReader.buffered populated: the memcpy branch. + while ((c = proc.stdout!.read()) !== null) total += c.length; + await tick(); + await tick(); + } + + proc.stdin!.end(); + proc.stdout!.resume(); + proc.stdout!.on("data", (d: Buffer) => void (total += d.length)); + await once(proc, "close"); + expect(total).toBe((iters * 2 + 1) * CHUNK); +} + +// The leak is in the posix poll-reader path; Windows pipes go through +// libuv with different buffering. +test.todoIf(isWindows)( + "FileReader.onPull frees the drained buffer after memcpy", + async () => { + // Warmup to settle allocator / JIT state. + await run(100); + Bun.gc(true); + const before = process.memoryUsage.rss(); + + await run(2000); + Bun.gc(true); + const after = process.memoryUsage.rss(); + + const deltaMB = (after - before) / MB; + console.log( + `RSS before=${(before / MB).toFixed(1)}MB after=${(after / MB).toFixed(1)}MB delta=${deltaMB.toFixed(1)}MB`, + ); + + // Without the fix each of the 2000 iterations orphans a ~32 KiB + // allocation: ~64 MB of leaked mimalloc memory (≈95 MB RSS under + // ASAN). With the fix RSS stays roughly flat (<30 MB of noise). + expect(deltaMB).toBeLessThan(50); + }, + // Debug+ASAN event-loop ticks are slow; release finishes in ~1s. + 180_000, +); From a2c1de87e6a8d045de05aa838a0a65ef27c1cd09 Mon Sep 17 00:00:00 2001 From: robobun Date: Wed, 29 Apr 2026 03:50:54 +0000 Subject: [PATCH 2/3] test: measure onPull drain leak via subprocess slope under ASAN Release builds recycle the orphaned 32 KiB blocks into later same-size allocations, so RSS growth is sub-linear and flaky to threshold (failed on Alpine 3.23). Under ASAN each leaked block is quarantined and cannot be reused, so the leak shows as clean linear growth (~150 MB over 4x1000 iterations vs <12 MB fixed). Gate the test to debug/ASAN builds and sample RSS across five equal runs in an isolated subprocess. --- .../spawn-stdout-iterate-leak.fixture.ts | 87 +++++++++++++ .../spawn/spawn-stdout-iterate-leak.test.ts | 122 ++++++------------ 2 files changed, 123 insertions(+), 86 deletions(-) create mode 100644 test/js/bun/spawn/spawn-stdout-iterate-leak.fixture.ts diff --git a/test/js/bun/spawn/spawn-stdout-iterate-leak.fixture.ts b/test/js/bun/spawn/spawn-stdout-iterate-leak.fixture.ts new file mode 100644 index 00000000000..0a587d32e83 --- /dev/null +++ b/test/js/bun/spawn/spawn-stdout-iterate-leak.fixture.ts @@ -0,0 +1,87 @@ +// Driven by spawn-stdout-iterate-leak.test.ts. +// +// Exercises the FileReader.onPull drain+memcpy branch: child_process stdout +// is a NativeReadable whose `_read()` calls `ptr.pull()` directly (no JS-side +// drain guard after the first read). With the Readable's highWaterMark set +// to CHUNK, a CHUNK-byte write that fulfils a pending pull via `.into_array` +// exactly fills the Node buffer, so `_read()` is not auto-rescheduled. The +// next write therefore lands in `onReadChunk` with no pending pull and is +// appended to `FileReader.buffered`. Draining the Node buffer via `.read()` +// then re-triggers `_read()` → `ptr.pull(CHUNK)` → `onPull`, which now sees +// `drain().len == CHUNK` and takes the memcpy branch. +// +// CHUNK = 32 KiB stays comfortably under the default 64 KiB pipe buffer so +// the round-trip through `cat` never blocks. + +import { spawn } from "child_process"; +import { once } from "events"; + +const MB = 1024 * 1024; +const CHUNK = 32768; + +async function run(iters: number) { + const proc = spawn("cat", [], { stdio: ["pipe", "pipe", "ignore"] }); + // Match the Node buffer threshold to CHUNK so one push makes it "full". + (proc.stdout as any)._readableState.highWaterMark = CHUNK; + + const block = Buffer.alloc(CHUNK, 88); + const tick = () => new Promise(resolve => setImmediate(resolve)); + + let total = 0; + // Prime: the first chunk is consumed by the one-time JS-side drain in + // NativeReadable's first _read(). + proc.stdin!.write(block); + await once(proc.stdout!, "readable"); + let c: Buffer | null; + while ((c = proc.stdout!.read()) !== null) total += c.length; + await tick(); + await tick(); + + for (let i = 0; i < iters; i++) { + // First write fulfils the pending pull (into_array); second write lands + // in FileReader.buffered. + proc.stdin!.write(block); + await tick(); + await tick(); + proc.stdin!.write(block); + await tick(); + await tick(); + // Draining the Node buffer re-triggers _read → onPull with + // FileReader.buffered populated: the memcpy branch. + while ((c = proc.stdout!.read()) !== null) total += c.length; + await tick(); + await tick(); + } + + proc.stdin!.end(); + proc.stdout!.resume(); + proc.stdout!.on("data", (d: Buffer) => void (total += d.length)); + await once(proc, "close"); + if (total !== (iters * 2 + 1) * CHUNK) { + throw new Error(`wrong total: got ${total}, expected ${(iters * 2 + 1) * CHUNK}`); + } +} + +// A real leak grows RSS on every run; transient allocator growth plateaus. +// After a short warmup we take several equally-sized samples and report the +// delta from the first sample to the last — a leak adds ~ITERS×CHUNK per +// step, non-leaking allocator caching contributes (at most) once. +const ITERS = 1000; +const STEPS = 5; + +await run(200); +Bun.gc(true); + +const samples: number[] = []; +for (let i = 0; i < STEPS; i++) { + await run(ITERS); + Bun.gc(true); + samples.push(process.memoryUsage.rss()); +} + +console.log( + JSON.stringify({ + samples: samples.map(s => s / MB), + delta: (samples[samples.length - 1] - samples[0]) / MB, + }), +); diff --git a/test/js/bun/spawn/spawn-stdout-iterate-leak.test.ts b/test/js/bun/spawn/spawn-stdout-iterate-leak.test.ts index 41f0159654a..dec0eb7a5fc 100644 --- a/test/js/bun/spawn/spawn-stdout-iterate-leak.test.ts +++ b/test/js/bun/spawn/spawn-stdout-iterate-leak.test.ts @@ -1,7 +1,6 @@ import { expect, test } from "bun:test"; -import { spawn } from "child_process"; -import { once } from "events"; -import { isWindows } from "harness"; +import { bunEnv, bunExe, isASAN, isDebug, isWindows } from "harness"; +import { join } from "path"; // Regression test for FileReader.onPull: after `drain()` moves the // internally buffered data into a local ByteList and the data is memcpy'd @@ -9,94 +8,45 @@ import { isWindows } from "harness"; // code freed `this.buffered` instead — but `drain()` had already emptied // it, so the moved allocation was orphaned on every such pull. // -// Reaching that branch requires onPull to be called while data is sitting -// in `this.buffered` *and* the JS buffer is large enough to hold it. Most -// consumers route through a JS-side `handle.drain()` guard first, so we -// construct the exact sequence that bypasses it: -// -// - child_process stdout is a NativeReadable whose `_read()` calls -// `ptr.pull()` directly (no drain guard after the first read). -// - With the Readable's highWaterMark set to CHUNK, the first CHUNK-byte -// write fulfils the pending pull via `.into_array` (result == view -// length, so `kRemainingChunk` becomes undefined) and fills the Node -// buffer exactly, so `_read()` is not auto-rescheduled. -// - The second write then lands in `onReadChunk` with no pending pull -// and is appended to `FileReader.buffered`. -// - Draining the Node buffer via `.read()` triggers `_read()` → -// `ptr.pull(CHUNK)` → `onPull`, which now sees `drain().len == CHUNK` -// and takes the memcpy branch. -// -// CHUNK = 32 KiB stays comfortably under the default 64 KiB pipe buffer -// so the round-trip through `cat` never blocks. - -const MB = 1024 * 1024; -const CHUNK = 32768; - -async function run(iters: number) { - const proc = spawn("cat", [], { stdio: ["pipe", "pipe", "ignore"] }); - // Match the Node buffer threshold to CHUNK so one push makes it "full". - (proc.stdout as any)._readableState.highWaterMark = CHUNK; - - const block = Buffer.alloc(CHUNK, 88); - const tick = () => new Promise(resolve => setImmediate(resolve)); - - let total = 0; - // Prime: first chunk is consumed by the one-time JS-side drain in - // NativeReadable's first _read(). - proc.stdin!.write(block); - await once(proc.stdout!, "readable"); - let c: Buffer | null; - while ((c = proc.stdout!.read()) !== null) total += c.length; - await tick(); - await tick(); - - for (let i = 0; i < iters; i++) { - // First write fulfils the pending pull (into_array), second write - // lands in FileReader.buffered. - proc.stdin!.write(block); - await tick(); - await tick(); - proc.stdin!.write(block); - await tick(); - await tick(); - // Draining the Node buffer re-triggers _read → onPull with - // FileReader.buffered populated: the memcpy branch. - while ((c = proc.stdout!.read()) !== null) total += c.length; - await tick(); - await tick(); - } - - proc.stdin!.end(); - proc.stdout!.resume(); - proc.stdout!.on("data", (d: Buffer) => void (total += d.length)); - await once(proc, "close"); - expect(total).toBe((iters * 2 + 1) * CHUNK); -} +// The measurement runs in a subprocess so RSS is isolated from the test +// harness, and several equally-sized runs are sampled so steady-state +// allocator growth (which plateaus) is distinguished from a real leak +// (which keeps climbing). See spawn-stdout-iterate-leak.fixture.ts for how +// the exact code path is reached. // The leak is in the posix poll-reader path; Windows pipes go through // libuv with different buffering. -test.todoIf(isWindows)( +// +// On release builds without ASAN, mimalloc recycles the orphaned 32 KiB +// blocks into later allocations of the same size class, so RSS growth is +// sub-linear and too close to allocator noise to threshold reliably. +// Under ASAN each leaked block is quarantined behind poisoned redzones and +// cannot be reused, so the leak shows up as clean linear RSS growth +// (~106-119 MB unfixed vs. <10 MB fixed over the sampled window). Debug +// builds always enable ASAN. +test.skipIf(isWindows || !(isDebug || isASAN))( "FileReader.onPull frees the drained buffer after memcpy", async () => { - // Warmup to settle allocator / JIT state. - await run(100); - Bun.gc(true); - const before = process.memoryUsage.rss(); - - await run(2000); - Bun.gc(true); - const after = process.memoryUsage.rss(); - - const deltaMB = (after - before) / MB; - console.log( - `RSS before=${(before / MB).toFixed(1)}MB after=${(after / MB).toFixed(1)}MB delta=${deltaMB.toFixed(1)}MB`, - ); - - // Without the fix each of the 2000 iterations orphans a ~32 KiB - // allocation: ~64 MB of leaked mimalloc memory (≈95 MB RSS under - // ASAN). With the fix RSS stays roughly flat (<30 MB of noise). - expect(deltaMB).toBeLessThan(50); + await using proc = Bun.spawn({ + cmd: [bunExe(), join(import.meta.dir, "spawn-stdout-iterate-leak.fixture.ts")], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + stdin: "ignore", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stderr).toBe(""); + + const { samples, delta } = JSON.parse(stdout.trim()) as { samples: number[]; delta: number }; + console.log(`RSS samples=[${samples.map(s => s.toFixed(1)).join(", ")}]MB delta=${delta.toFixed(1)}MB`); + expect(exitCode).toBe(0); + + // Without the fix, each of the 4×1000 iterations between the first and + // last sample orphans a ~32 KiB allocation, so RSS climbs another + // ~65-120 MB (higher under ASAN). With the fix the samples plateau and + // the first-to-last delta is noise (<10 MB). + expect(delta).toBeLessThan(32); }, // Debug+ASAN event-loop ticks are slow; release finishes in ~1s. - 180_000, + 300_000, ); From 4ea2153d104b95fe4b615b6f47d9ad595d0efb8b Mon Sep 17 00:00:00 2001 From: robobun Date: Wed, 29 Apr 2026 04:00:07 +0000 Subject: [PATCH 3/3] test: attach data listener before resume in fixture cleanup --- test/js/bun/spawn/spawn-stdout-iterate-leak.fixture.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/js/bun/spawn/spawn-stdout-iterate-leak.fixture.ts b/test/js/bun/spawn/spawn-stdout-iterate-leak.fixture.ts index 0a587d32e83..0bd8614fc0f 100644 --- a/test/js/bun/spawn/spawn-stdout-iterate-leak.fixture.ts +++ b/test/js/bun/spawn/spawn-stdout-iterate-leak.fixture.ts @@ -54,8 +54,8 @@ async function run(iters: number) { } proc.stdin!.end(); - proc.stdout!.resume(); proc.stdout!.on("data", (d: Buffer) => void (total += d.length)); + proc.stdout!.resume(); await once(proc, "close"); if (total !== (iters * 2 + 1) * CHUNK) { throw new Error(`wrong total: got ${total}, expected ${(iters * 2 + 1) * CHUNK}`);