fix(fetch): prevent ReadableStream memory leak when reusing Response.body#23145
fix(fetch): prevent ReadableStream memory leak when reusing Response.body#23145robobun wants to merge 2 commits into
Conversation
…body Fixes a memory leak where creating a new Response with another Response's body would create duplicate Strong references to the same ReadableStream, preventing garbage collection. The issue occurred in this pattern: ```js const r1 = new Response(stream); const r2 = new Response(r1.body); ``` Both r1 and r2 would create Strong references to the same ReadableStream JSValue. When r1 was garbage collected, only its Strong reference would be released, but r2's Strong reference would keep the stream alive indefinitely. The fix transfers ownership of the ReadableStream when accessing response.body. When `toReadableStream` is called on a Locked body with an existing stream, it now releases the Body's Strong reference before returning the stream JSValue. This ensures only one Strong reference exists per stream. Fixes TanStack/router#5289 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
robobun
force-pushed
the
claude/fix-response-body-stream-leak
branch
from
9694531 to
78c512d
Compare
WalkthroughTransferred ownership of existing ReadableStream in Body.Value.Locked by releasing the Strong reference before returning the stream value. Added regression tests that exercise reusing Response bodies and Bun.serve responses to assert no ReadableStream reference leaks using fullGC and heapStats. Changes
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/bun.js/webcore/Body.zig(1 hunks)test/regression/issue/response-body-stream-leak.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/regression/issue/response-body-stream-leak.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/regression/issue/response-body-stream-leak.test.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.ts: Name test files*.test.tsand usebun:test
Do not write flaky tests: never wait for arbitrary time; wait for conditions instead
Never hardcode port numbers in tests; useport: 0to get a random port
When spawning Bun in tests, usebunExe()andbunEnvfromharness
Preferasync/awaitin tests; for a single callback, usePromise.withResolvers()
Do not set explicit test timeouts; rely on Bun’s built-in timeouts
UsetempDir/tempDirWithFilesfromharnessfor temporary files and directories in tests
For large/repetitive strings in tests, preferBuffer.alloc(count, fill).toString()over"A".repeat(count)
Import common test utilities fromharness(e.g.,bunExe,bunEnv,tempDirWithFiles,tmpdirSync, platform checks, GC helpers)
In error tests, assert non-zero exit codes for failing processes and usetoThrowfor synchronous errors
Usedescribeblocks for grouping,describe.eachfor parameterized tests, snapshots withtoMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach); track resources for cleanup inafterEach
Useusing/await usingwith Bun resources (e.g., Bun.listen/connect/spawn/serve) to ensure cleanup in tests
Files:
test/regression/issue/response-body-stream-leak.test.ts
test/regression/issue/**
📄 CodeRabbit inference engine (test/CLAUDE.md)
Place regression tests under
test/regression/issue/and organize by issue number
Files:
test/regression/issue/response-body-stream-leak.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must be placed under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or use custom random port functions
In tests, use normalizeBunSnapshot when asserting snapshots
Never write tests that merely assert absence of "panic" or "uncaught exception" in output
Avoid shell commands (e.g., find, grep) in tests; use Bun.Glob and built-ins instead
Prefer snapshot tests over exact stdout equality assertions
Files:
test/regression/issue/response-body-stream-leak.test.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Format JavaScript/TypeScript files with Prettier (bun run prettier)
Files:
test/regression/issue/response-body-stream-leak.test.ts
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})
Files:
src/bun.js/webcore/Body.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
**/*.zig: Format Zig files with zig-format (bun run zig-format)
In Zig, manage memory carefully with allocators and use defer for cleanup
Files:
src/bun.js/webcore/Body.zig
src/bun.js/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy
Files:
src/bun.js/webcore/Body.zig
🧬 Code graph analysis (1)
test/regression/issue/response-body-stream-leak.test.ts (1)
test/js/node/http2/node-http2-memory-leak.js (1)
heapStats(8-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
| const stream_value = readable.value; | ||
| // Transfer ownership of the stream by releasing our Strong reference | ||
| // This prevents creating duplicate Strong references when the stream | ||
| // is passed to another Response/Request constructor | ||
| if (!locked.deinit) { | ||
| locked.deinit = true; | ||
| locked.readable.deinit(); | ||
| locked.readable = .{}; | ||
| } |
There was a problem hiding this comment.
Transfer breaks subsequent Body reads
Releasing the Strong and zeroing locked.readable on Line 482 removes the only handle setPromise has to feed .text(), .json(), etc. If user code does:
const r1 = new Response(stream);
const r2 = new Response(r1.body);
await r1.text();PendingValue.setPromise (Line 160) now finds locked.readable.get(...) is null and falls into the fallback path, which never resolves for bodies created from an existing ReadableStream. Result: r1.text() hangs forever instead of consuming or rejecting. Either keep a way to recover the ReadableStream (e.g., re-wrap stream_value on demand) or mark the body used so callers get an immediate rejection, but we can’t just drop the reference.
🤖 Prompt for AI Agents
In src/bun.js/webcore/Body.zig around lines 477-485, releasing the Strong
reference and setting locked.readable to empty removes the only handle
PendingValue.setPromise relies on when resolving .text()/.json(), causing reads
to hang; instead of deinitializing and zeroing locked.readable, preserve a
recoverable reference to the original ReadableStream (e.g., keep stream_value or
re-wrap it into locked.readable so locked.readable.get(...) still returns the
stream), or explicitly mark the body as used and reject subsequent read attempts
immediately; in short, do not drop the sole readable handle — either
retain/restore it for PendingValue or set a used/rejected flag that causes
PendingValue.setPromise to reject rather than fall into the unresolved fallback.
|
I've tried |
|
Closing PR - implementation breaks core streaming functionality with widespread test failures (timeouts in streams.test.js, failures in fetch.stream.test.ts, wasm-streaming.test.ts, and bun-install.test.ts across all platforms). |
|
Fixed in #23313 |
3 participants
Summary
Fixes a memory leak where creating a new Response with another Response's body would create duplicate Strong references to the same ReadableStream, preventing garbage collection.
The Problem
The issue occurred in this pattern:
What was happening:
r1 = new Response(stream)- r1's body creates a Strong reference to the streamr1.body- returns the same stream JSValue, but r1 still holds its Strong referencer2 = new Response(r1.body)- r2's body creates a SECOND Strong reference to the same streamThis resulted in ReadableStreams accumulating in memory and never being collected, particularly noticeable in server applications using
Bun.serve().The Fix
Transfer ownership of the ReadableStream when accessing
response.body. WhenBody.Value.toReadableStreamis called on a Locked body with an existing stream, it now releases the Body's Strong reference before returning the stream JSValue. This ensures only one Strong reference exists per stream at any given time.Changes:
src/bun.js/webcore/Body.zig: ModifiedtoReadableStream()to release the Strong reference when returning an existing ReadableStream from a Locked bodytest/regression/issue/response-body-stream-leak.test.ts: Added regression tests to prevent this issue from recurringTest Results
Before the fix:
After the fix:
Bun.serve()with Response body reuse no longer leaks streamsTest plan
bun test test/regression/issue/response-body-stream-leak.test.tsBoth tests should pass, verifying that:
Related Issues
Fixes TanStack/router#5289
🤖 Generated with Claude Code