fix(fetch): fix ReadableStream memory leak when using stream body#25846
Merged
Conversation
When fetch() is called with a ReadableStream body, the stream objects
were leaking because FetchTasklet's clearData() function had a
conditional that prevented detach() from being called on ReadableStream
request bodies after streaming started.
The issue was in the condition:
if (this.request_body != .ReadableStream or this.is_waiting_request_stream_start)
After startRequestStream() is called:
- is_waiting_request_stream_start becomes false
- request_body is still .ReadableStream
- The condition evaluates to (false or false) = false
- detach() is skipped, causing the leak
This is incorrect because:
1. startRequestStream() creates an independent Strong reference in
ResumableSink (not a transfer of ownership)
2. FetchTasklet's reference becomes redundant after streaming starts
3. Each Strong reference is independent - deinit on one doesn't affect
the other
The fix removes the conditional and always calls detach(), which is safe
because:
- Strong references are independent (detaching FetchTasklet's reference
doesn't affect ResumableSink's reference)
- detach() is idempotent (safe to call on already-detached references)
- clearData() is only called from deinit() after streaming completes
Claude-Generated-By: Claude Code (cli/claude-opus-4-5=100%)
Claude-Steers: 3
Claude-Permission-Prompts: 2
Claude-Escapes: 0
Claude-Plan:
<claude-plan>
# FetchTasklet ReadableStream メモリリーク修正計画
## 問題の概要
`fetch()` に `ReadableStream` bodyを渡すと、ReadableStreamオブジェクトがリークする。
## 根本原因の詳細分析
### 問題のコード
`src/bun.js/webcore/fetch/FetchTasklet.zig` の `clearData()` 関数（237-239行目）：
```zig
if (this.request_body != .ReadableStream or this.is_waiting_request_stream_start) {
this.request_body.detach();
}
```
### ライフサイクルの追跡
1. **初期状態** (`queue` 関数, 1065-1067行目):
- `fetch_tasklet.request_body = fetch_options.body` (HTTPRequestBody.ReadableStream)
- `fetch_tasklet.is_waiting_request_stream_start = true`
2. **`startRequestStream()` が呼ばれるとき** (281-298行目):
- `is_waiting_request_stream_start = false` に設定
- `request_body.ReadableStream.get()` でストリームを取得
- ResumableSinkを作成し、**独自のStrong参照を作成** (ResumableSink.zig:119行目):
```zig
this.stream = jsc.WebCore.ReadableStream.Strong.init(stream, this.globalThis);
```
3. **ResumableSinkが終了するとき**:
- ResumableSinkは自身の `stream` Strong参照を解放
- `writeEndRequest` → `deref()` → `FetchTasklet.deinit()` へ
4. **`FetchTasklet.deinit()` が呼ばれるとき**:
- `clearData()` を呼び出し
- 条件評価: `request_body == .ReadableStream` かつ `is_waiting_request_stream_start == false`
- → 条件は `false or false = false` → **`detach()` がスキップされる！**
### なぜバグなのか
- `startRequestStream()` 後、`request_body.ReadableStream` は**二度と使われない**
- ResumableSink は**独自の独立した** Strong参照を持つ
- Strong参照は独立している - 一方を `deinit()` しても他方には影響しない（src/bun.js/Strong.zig）
- FetchTasklet の Strong参照は不要になったのに `detach()` が呼ばれないため**リーク**
### 元のコードの意図の推測
開発者は以下を誤解していた可能性がある：
- `startRequestStream()` で Strong参照の「所有権が移譲された」と考えた
- 実際は ResumableSink が**コピー**（新しい Strong参照）を作成している
- 元の参照は冗長になるが解放されていない
## 修正方法
### 変更対象ファイル
- `src/bun.js/webcore/fetch/FetchTasklet.zig`
### 修正内容
```zig
// 修正前（237-239行目）:
if (this.request_body != .ReadableStream or this.is_waiting_request_stream_start) {
this.request_body.detach();
}
// 修正後:
this.request_body.detach();
```
### 修正の安全性
1. **Strong参照の独立性**: 各Strong参照は独立したカウント - FetchTaskletがdetachしてもResumableSinkの参照は影響を受けない
2. **冪等性**: `detach()` は既に解放された参照に対して呼んでも安全
3. **タイミング**: `clearData()` は `deinit()` からのみ呼ばれ、その時点でストリーミングは既に完了している
### UAF (Use-After-Free) のリスク分析
**結論: UAFのリスクはない**
#### 参照カウントのライフサイクル
1. 初期状態: ref_count = 1
2. HTTPリクエスト開始時 (line 1257): `node.ref()` → ref_count = 2
3. ストリーミング時 (line 293): `this.ref()` → ref_count = 3 (if streaming)
#### なぜUAFが起きないか
1. `deinit()` は ref_count = 0 のときのみ呼ばれる (line 252 でアサート)
2. `startRequestStream()` は `onProgressUpdate()` から呼ばれる (line 474)
3. `onProgressUpdate()` は実行中に ref を保持している
4. したがって、`startRequestStream()` 実行中は ref_count > 0
5. `clearData()` と `startRequestStream()` は**同時に実行されない**
#### `request_body.ReadableStream` へのアクセス箇所
唯一のアクセス箇所は `startRequestStream()` の line 284:
```zig
if (this.request_body.ReadableStream.get(this.global_this)) |stream| {
```
上記の分析により、この箇所が `clearData()` 後に実行されることはありえない。
#### 防御的プログラミング
仮に何らかの理由で `detach()` 後にアクセスがあった場合:
- `Strong.deinit()` は `impl = null` に設定する
- `Strong.get()` は `impl == null` なら `null` を返す
- クラッシュせずに安全にnullが返される
## 検証方法
### 1. メモリリークテストの実行
```bash
bun bd test test/js/web/fetch/fetch-cyclic-reference.test.ts
```
現在 `test.skip` になっている2つのテストを有効化：
- "fetch with request body stream should not leak with cyclic reference"
- "fetch with ReadableStream body should not leak streams"
### 2. 既存のfetch関連テストの確認
```bash
bun bd test test/js/web/fetch/
```
### 3. 手動確認
```bash
bun bd /tmp/test-fetch-no-cycle.ts
```
## リスク評価
**低リスク**:
- 修正は単純な条件分岐の削除
- Strong参照の独立性によりストリーム処理への影響なし
- 既存のfetchテストスイートで回帰を検出可能
</claude-plan>
Collaborator
|
Updated 6:48 AM PT - Jan 6th, 2026
❌ Your commit
🧪 To try this PR locally: bunx bun-pr 25846That installs a local version of the PR into your bun-25846 --bun |
Contributor
WalkthroughUnconditionally detach Changes
Possibly related PRs
Suggested reviewers
Pre-merge checks✅ Passed checks (2 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @test/js/web/fetch/fetch-cyclic-reference.test.ts:
- Around line 223-225: Remove the outdated inline comment "This currently fails
with ~502 streams leaked" above the assertion that checks readableStreamCount;
locate the heapStats usage and the variable readableStreamCount (and the
subsequent expect(readableStreamCount).toBeLessThanOrEqual(100)) in
fetch-cyclic-reference.test.ts and delete only that stale comment so the test
reflects current behavior.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
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/fetch/FetchTasklet.zigtest/js/web/fetch/fetch-cyclic-reference.test.ts
🧰 Additional context used
📓 Path-based instructions (5)
src/**/*.zig
📄 CodeRabbit inference engine (src/CLAUDE.md)
src/**/*.zig: Private fields in Zig are fully supported using the#prefix:struct { #foo: u32 };
Use decl literals in Zig for declaration initialization:const decl: Decl = .{ .binding = 0, .value = 0 };
Prefer@importat the bottom of the file (auto formatter will move them automatically)
Files:
src/bun.js/webcore/fetch/FetchTasklet.zig
**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, be careful with allocators and use defer for cleanup
Files:
src/bun.js/webcore/fetch/FetchTasklet.zig
**/*.test.ts?(x)
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.ts?(x): Never usebun testdirectly - always usebun bd testto run tests with debug build changes
For single-file tests, prefer-eflag overtempDir
For multi-file tests, prefertempDirandBun.spawnover single-file tests
UsenormalizeBunSnapshotto normalize snapshot output of tests
Never write tests that check for 'panic', 'uncaught exception', or similar strings in test output
UsetempDirfromharnessto create temporary directories - do not usetmpdirSyncorfs.mkdtempSync
When spawning processes in tests, expect stdout before expecting exit code for more useful error messages on test failure
Do not write flaky tests - do not usesetTimeoutin tests; instead await the condition to be met
Verify tests fail withUSE_SYSTEM_BUN=1 bun test <file>and pass withbun bd test <file>- tests are invalid if they pass with USE_SYSTEM_BUN=1
Test files must end with.test.tsor.test.tsx
Avoid shell commands likefindorgrepin tests - use Bun's Glob and built-in tools instead
Files:
test/js/web/fetch/fetch-cyclic-reference.test.ts
test/**/*.test.ts?(x)
📄 CodeRabbit inference engine (CLAUDE.md)
Always use
port: 0in tests - do not hardcode ports or use custom random port number functions
Files:
test/js/web/fetch/fetch-cyclic-reference.test.ts
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Usebun bd test <...test file>to run tests with compiled code changes. Do not usebun testas it will not include your changes.
Usebun:testfor files ending in*.test.{ts,js,jsx,tsx,mjs,cjs}. For test files without .test extension in test/js/node/test/{parallel,sequential}/*.js, usebun bd <file>instead ofbun bd test <file>since they expect exit code 0.
Do not set a timeout on tests. Bun already has timeouts built-in.
Files:
test/js/web/fetch/fetch-cyclic-reference.test.ts
🧠 Learnings (17)
📓 Common learnings
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/BunFetchInstrumentation.ts:126-131
Timestamp: 2025-10-20T01:38:02.660Z
Learning: In BunFetchInstrumentation.ts, the force-restore to ORIGINAL_FETCH in the disable() method is intentionally kept (despite appearing unsafe) because it's required for proper test cleanup when instrumentation is repeatedly enabled/disabled. Without it, 15 distributed tracing and context propagation tests fail. Shimmer's unwrap() doesn't reliably restore the original fetch in Bun's globalThis context. The isBunOtelPatched safety check ensures the restore only happens when the current fetch is still ours, preventing clobbering of other tools' wrappers.
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: test/js/bun/http/node-telemetry.test.ts:27-203
Timestamp: 2025-10-19T04:55:33.099Z
Learning: In test/js/bun/http/node-telemetry.test.ts and the Bun.telemetry._node_binding API, after the architecture refactor, the _node_binding interface only contains two methods: handleIncomingRequest(req, res) and handleWriteHead(res, statusCode). The handleRequestFinish hook and other lifecycle hooks were removed during simplification. Both current methods are fully tested.
📚 Learning: 2025-10-20T01:38:02.660Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/BunFetchInstrumentation.ts:126-131
Timestamp: 2025-10-20T01:38:02.660Z
Learning: In BunFetchInstrumentation.ts, the force-restore to ORIGINAL_FETCH in the disable() method is intentionally kept (despite appearing unsafe) because it's required for proper test cleanup when instrumentation is repeatedly enabled/disabled. Without it, 15 distributed tracing and context propagation tests fail. Shimmer's unwrap() doesn't reliably restore the original fetch in Bun's globalThis context. The isBunOtelPatched safety check ensures the restore only happens when the current fetch is still ours, preventing clobbering of other tools' wrappers.
Applied to files:
src/bun.js/webcore/fetch/FetchTasklet.zigtest/js/web/fetch/fetch-cyclic-reference.test.ts
📚 Learning: 2025-10-19T04:55:33.099Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: test/js/bun/http/node-telemetry.test.ts:27-203
Timestamp: 2025-10-19T04:55:33.099Z
Learning: In test/js/bun/http/node-telemetry.test.ts and the Bun.telemetry._node_binding API, after the architecture refactor, the _node_binding interface only contains two methods: handleIncomingRequest(req, res) and handleWriteHead(res, statusCode). The handleRequestFinish hook and other lifecycle hooks were removed during simplification. Both current methods are fully tested.
Applied to files:
src/bun.js/webcore/fetch/FetchTasklet.zigtest/js/web/fetch/fetch-cyclic-reference.test.ts
📚 Learning: 2026-01-05T16:32:07.551Z
Learnt from: alii
Repo: oven-sh/bun PR: 25474
File: src/bun.js/event_loop/Sigusr1Handler.zig:0-0
Timestamp: 2026-01-05T16:32:07.551Z
Learning: In Zig codebases (e.g., Bun), treat std.posix.sigaction as returning void and do not perform runtime error handling for its failure. The Zig standard library views sigaction failures as programmer errors (unreachable) because they only occur with invalid signals like SIGKILL/SIGSTOP. Apply this pattern across Zig files that call sigaction (e.g., crash_handler.zig, main.zig, filter_run.zig, process.zig) and ensure failures are not handled as recoverable errors; prefer reaching an explicit unreachable/compile-time assumption when such failures are detected.
Applied to files:
src/bun.js/webcore/fetch/FetchTasklet.zig
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add corresponding test cases to test/v8/v8.test.ts using checkSameOutput() function to compare Node.js and Bun output
Applied to files:
test/js/web/fetch/fetch-cyclic-reference.test.ts
📚 Learning: 2025-10-18T05:23:24.403Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: test/js/bun/telemetry-server.test.ts:91-100
Timestamp: 2025-10-18T05:23:24.403Z
Learning: In the Bun codebase, telemetry tests (test/js/bun/telemetry-*.test.ts) should focus on telemetry API behavior: configure/disable/isEnabled, callback signatures and invocation, request ID correlation, and error handling. HTTP protocol behaviors like status code normalization (e.g., 200 with empty body → 204) should be tested in HTTP server tests (test/js/bun/http/), not in telemetry tests. Keep separation of concerns: telemetry tests verify the telemetry API contract; HTTP tests verify HTTP semantics.
Applied to files:
test/js/web/fetch/fetch-cyclic-reference.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `bun bd test <...test file>` to run tests with compiled code changes. Do not use `bun test` as it will not include your changes.
Applied to files:
test/js/web/fetch/fetch-cyclic-reference.test.ts
📚 Learning: 2025-10-19T02:44:46.354Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/context-propagation.test.ts:1-1
Timestamp: 2025-10-19T02:44:46.354Z
Learning: In the Bun repository, standalone packages under packages/ (e.g., bun-vscode, bun-inspector-protocol, bun-plugin-yaml, bun-plugin-svelte, bun-debug-adapter-protocol, bun-otel) co-locate their tests with package source code using *.test.ts files. This follows standard npm/monorepo patterns. The test/ directory hierarchy (test/js/bun/, test/cli/, test/js/node/) is reserved for testing Bun's core runtime APIs and built-in functionality, not standalone packages.
Applied to files:
test/js/web/fetch/fetch-cyclic-reference.test.ts
📚 Learning: 2025-10-25T17:20:19.041Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 24063
File: test/js/bun/telemetry/server-header-injection.test.ts:5-20
Timestamp: 2025-10-25T17:20:19.041Z
Learning: In the Bun telemetry codebase, tests are organized into two distinct layers: (1) Internal API tests in test/js/bun/telemetry/ use numeric InstrumentKind enum values to test Zig↔JS injection points and low-level integration; (2) Public API tests in packages/bun-otel/test/ use string InstrumentKind values ("http", "fetch", etc.) to test the public-facing BunSDK and instrumentation APIs. This separation allows internal tests to use efficient numeric enums for refactoring flexibility while the public API maintains a developer-friendly string-based interface.
Applied to files:
test/js/web/fetch/fetch-cyclic-reference.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Applies to test/**/*-fixture.ts : Test files that spawn Bun processes should end in `*-fixture.ts` to identify them as test fixtures rather than tests themselves.
Applied to files:
test/js/web/fetch/fetch-cyclic-reference.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `bun:test` for files ending in `*.test.{ts,js,jsx,tsx,mjs,cjs}`. For test files without .test extension in test/js/node/test/{parallel,sequential}/*.js, use `bun bd <file>` instead of `bun bd test <file>` since they expect exit code 0.
Applied to files:
test/js/web/fetch/fetch-cyclic-reference.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : For multi-file tests, prefer `tempDir` and `Bun.spawn` over single-file tests
Applied to files:
test/js/web/fetch/fetch-cyclic-reference.test.ts
📚 Learning: 2025-11-06T00:58:23.965Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 24417
File: test/js/bun/spawn/spawn.test.ts:903-918
Timestamp: 2025-11-06T00:58:23.965Z
Learning: In Bun test files, `await using` with spawn() is appropriate for long-running processes that need guaranteed cleanup on scope exit or when explicitly testing disposal behavior. For short-lived processes that exit naturally (e.g., console.log scripts), the pattern `const proc = spawn(...); await proc.exited;` is standard and more common, as evidenced by 24 instances vs 4 `await using` instances in test/js/bun/spawn/spawn.test.ts.
Applied to files:
test/js/web/fetch/fetch-cyclic-reference.test.ts
📚 Learning: 2025-09-03T01:30:58.001Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 21728
File: test/js/valkey/valkey.test.ts:264-271
Timestamp: 2025-09-03T01:30:58.001Z
Learning: For test/js/valkey/valkey.test.ts PUB/SUB tests, avoid arbitrary sleeps and async-forEach. Instead, resolve a Promise from the subscriber callback when the expected number of messages is observed and await it with a bounded timeout (e.g., withTimeout + Promise.withResolvers) to account for Redis server→subscriber propagation.
Applied to files:
test/js/web/fetch/fetch-cyclic-reference.test.ts
📚 Learning: 2025-10-30T03:48:10.513Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 24063
File: packages/bun-otel/test/context-propagation.test.ts:1-7
Timestamp: 2025-10-30T03:48:10.513Z
Learning: In Bun test files, `using` declarations at the describe block level execute during module load/parsing, not during test execution. This means they acquire and dispose resources before any tests run. For test-scoped resource management, use beforeAll/afterAll hooks instead. The pattern `beforeAll(beforeUsingEchoServer); afterAll(afterUsingEchoServer);` is correct for managing ref-counted test resources like the EchoServer in packages/bun-otel/test/ - the using block pattern should not be used at describe-block level for test resources.
<!-- [/add_learning]
Applied to files:
test/js/web/fetch/fetch-cyclic-reference.test.ts
📚 Learning: 2025-10-08T13:48:02.430Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 23373
File: test/js/bun/tarball/extract.test.ts:107-111
Timestamp: 2025-10-08T13:48:02.430Z
Learning: In Bun's test runner, use `expect(async () => { await ... }).toThrow()` to assert async rejections. Unlike Jest/Vitest, Bun does not require `await expect(...).rejects.toThrow()` - the async function wrapper with `.toThrow()` is the correct pattern for async error assertions in Bun tests.
Applied to files:
test/js/web/fetch/fetch-cyclic-reference.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Do not set a timeout on tests. Bun already has timeouts built-in.
Applied to files:
test/js/web/fetch/fetch-cyclic-reference.test.ts
🧬 Code graph analysis (1)
test/js/web/fetch/fetch-cyclic-reference.test.ts (2)
test/js/node/http2/node-http2-memory-leak.js (1)
heapStats(8-8)test/js/node/http/node-fetch.test.js (1)
requestBody(126-130)
🔇 Additional comments (2)
src/bun.js/webcore/fetch/FetchTasklet.zig (1)
237-241: LGTM! Correctly fixes the memory leak.The unconditional detach is safe because:
detach()is idempotent for allHTTPRequestBodyvariantsclearData()runs fromdeinit()after streaming completesResumableSinkholds its own independent Strong reference, so releasing FetchTasklet's reference doesn't cause use-after-freeThe added comment clearly documents the rationale.
test/js/web/fetch/fetch-cyclic-reference.test.ts (1)
1-9: Well-structured test suite for memory leak validation.The tests correctly use:
port: 0as per guidelinesheapStats()frombun:jscfor object counting- Proper server cleanup in
afterAlland between tests- Generous thresholds (100 objects) to avoid flakiness while detecting leaks
The
Bun.sleep(10)usage is appropriate here for GC finalization timing in memory tests.
| describe("FetchTasklet cyclic reference", () => { | ||
| let server: ReturnType<typeof Bun.serve> | null = null; | ||
|
|
||
| afterAll(() => { |
Collaborator
There was a problem hiding this comment.
This is not necessary
| }); | ||
|
|
||
| test("response stream should not leak when response has cyclic reference", async () => { | ||
| server = Bun.serve({ |
Collaborator
There was a problem hiding this comment.
Suggested change
| server = Bun.serve({ | |
| await using server = Bun.serve({ |
| await Bun.sleep(10); | ||
| Bun.gc(true); | ||
|
|
||
| const responseCount = heapStats().objectTypeCounts.Response || 0; |
Collaborator
There was a problem hiding this comment.
It causes the Response to be leaked? Not the ReadableStream?
Jarred-Sumner
requested changes
Jan 6, 2026
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
Collaborator
There was a problem hiding this comment.
Only two of these tests fail on main.
Let's only keep the failing tests:
test/js/web/fetch/fetch-cyclic-reference.test.ts:
✓ FetchTasklet cyclic reference > response stream should not leak when response has cyclic reference [74.37ms]
✓ FetchTasklet cyclic reference > response stream should not leak when streaming response body with cyclic reference [54.61ms]
✓ FetchTasklet cyclic reference > response should not leak when body stream references response [56.90ms]
177 | await Bun.sleep(10);
178 | Bun.gc(true);
179 |
180 | const requestCount = heapStats().objectTypeCounts.Request || 0;
181 | const readableStreamCount = heapStats().objectTypeCounts.ReadableStream || 0;
182 | expect(requestCount).toBeLessThanOrEqual(100);
^
error: expect(received).toBeLessThanOrEqual(expected)
Expected: <= 100
Received: 501
at <anonymous> (/Users/jarred/Code/bun/test/js/web/fetch/fetch-cyclic-reference.test.ts:182:26)
✗ FetchTasklet cyclic reference > fetch with request body stream should not leak with cyclic reference [63.52ms]
220 | await Bun.sleep(10);
221 | Bun.gc(true);
222 |
223 | const readableStreamCount = heapStats().objectTypeCounts.ReadableStream || 0;
224 | // This currently fails with ~502 streams leaked
225 | expect(readableStreamCount).toBeLessThanOrEqual(100);
^
error: expect(received).toBeLessThanOrEqual(expected)
Expected: <= 100
Received: 1002
at <anonymous> (/Users/jarred/Code/bun/test/js/web/fetch/fetch-cyclic-reference.test.ts:225:33)
✗ FetchTasklet cyclic reference > fetch with ReadableStream body should not leak streams [59.81ms]
✓ FetchTasklet cyclic reference > multiple concurrent fetches should not leak with cyclic references [40.42ms]
Removed the 4 passing tests and refactored the 2 failing tests to use `await using server` instead of `afterAll` cleanup. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI Agents
In @test/js/web/fetch/fetch-cyclic-reference.test.ts:
- Around line 39-51: Extract the magic numbers used in the test into named
constants at the top of the test file (e.g., const LEAK_ITERATIONS = 500, const
MAX_LEAKED_OBJECTS = 100, const SLEEP_MS = 10) with a short comment explaining
the rationale (for example, "MAX_LEAKED_OBJECTS = 100 is acceptable after
LEAK_ITERATIONS iterations"). Replace the literals in the loop (for (let i = 0;
i < 500; i++)), the Bun.sleep calls (Bun.sleep(10)), and the assertions
(toBeLessThanOrEqual(100)) with the new constants; keep references to leak(),
Bun.gc(true), and heapStats().objectTypeCounts.Request/ReadableStream unchanged.
- Around line 4-94: Tests duplicate the iteration + double-GC pattern; extract
that logic into helpers to reduce duplication. Add a helper function (e.g.,
forceGarbageCollection) that performs the await Bun.sleep(10); Bun.gc(true);
await Bun.sleep(10); Bun.gc(true) sequence and another helper (e.g.,
runLeakTest) that accepts the leak function and iterations, runs the loop
calling leak(), then calls forceGarbageCollection; update both test cases
("fetch with request body stream should not leak with cyclic reference" and
"fetch with ReadableStream body should not leak streams") to call
runLeakTest(leak, 500) and keep their server setup (Bun.serve), leak()
definitions, and final heapStats assertions unchanged.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
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 (1)
test/js/web/fetch/fetch-cyclic-reference.test.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.test.ts?(x)
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.ts?(x): Never usebun testdirectly - always usebun bd testto run tests with debug build changes
For single-file tests, prefer-eflag overtempDir
For multi-file tests, prefertempDirandBun.spawnover single-file tests
UsenormalizeBunSnapshotto normalize snapshot output of tests
Never write tests that check for 'panic', 'uncaught exception', or similar strings in test output
UsetempDirfromharnessto create temporary directories - do not usetmpdirSyncorfs.mkdtempSync
When spawning processes in tests, expect stdout before expecting exit code for more useful error messages on test failure
Do not write flaky tests - do not usesetTimeoutin tests; instead await the condition to be met
Verify tests fail withUSE_SYSTEM_BUN=1 bun test <file>and pass withbun bd test <file>- tests are invalid if they pass with USE_SYSTEM_BUN=1
Test files must end with.test.tsor.test.tsx
Avoid shell commands likefindorgrepin tests - use Bun's Glob and built-in tools instead
Files:
test/js/web/fetch/fetch-cyclic-reference.test.ts
test/**/*.test.ts?(x)
📄 CodeRabbit inference engine (CLAUDE.md)
Always use
port: 0in tests - do not hardcode ports or use custom random port number functions
Files:
test/js/web/fetch/fetch-cyclic-reference.test.ts
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Usebun bd test <...test file>to run tests with compiled code changes. Do not usebun testas it will not include your changes.
Usebun:testfor files ending in*.test.{ts,js,jsx,tsx,mjs,cjs}. For test files without .test extension in test/js/node/test/{parallel,sequential}/*.js, usebun bd <file>instead ofbun bd test <file>since they expect exit code 0.
Do not set a timeout on tests. Bun already has timeouts built-in.
Files:
test/js/web/fetch/fetch-cyclic-reference.test.ts
🧠 Learnings (21)
📓 Common learnings
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/BunFetchInstrumentation.ts:126-131
Timestamp: 2025-10-20T01:38:02.660Z
Learning: In BunFetchInstrumentation.ts, the force-restore to ORIGINAL_FETCH in the disable() method is intentionally kept (despite appearing unsafe) because it's required for proper test cleanup when instrumentation is repeatedly enabled/disabled. Without it, 15 distributed tracing and context propagation tests fail. Shimmer's unwrap() doesn't reliably restore the original fetch in Bun's globalThis context. The isBunOtelPatched safety check ensures the restore only happens when the current fetch is still ours, preventing clobbering of other tools' wrappers.
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: test/js/bun/http/node-telemetry.test.ts:27-203
Timestamp: 2025-10-19T04:55:33.099Z
Learning: In test/js/bun/http/node-telemetry.test.ts and the Bun.telemetry._node_binding API, after the architecture refactor, the _node_binding interface only contains two methods: handleIncomingRequest(req, res) and handleWriteHead(res, statusCode). The handleRequestFinish hook and other lifecycle hooks were removed during simplification. Both current methods are fully tested.
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 24417
File: test/js/bun/spawn/spawn.test.ts:903-918
Timestamp: 2025-11-06T00:58:23.965Z
Learning: In Bun test files, `await using` with spawn() is appropriate for long-running processes that need guaranteed cleanup on scope exit or when explicitly testing disposal behavior. For short-lived processes that exit naturally (e.g., console.log scripts), the pattern `const proc = spawn(...); await proc.exited;` is standard and more common, as evidenced by 24 instances vs 4 `await using` instances in test/js/bun/spawn/spawn.test.ts.
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add corresponding test cases to test/v8/v8.test.ts using checkSameOutput() function to compare Node.js and Bun output
Applied to files:
test/js/web/fetch/fetch-cyclic-reference.test.ts
📚 Learning: 2025-10-19T04:55:33.099Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: test/js/bun/http/node-telemetry.test.ts:27-203
Timestamp: 2025-10-19T04:55:33.099Z
Learning: In test/js/bun/http/node-telemetry.test.ts and the Bun.telemetry._node_binding API, after the architecture refactor, the _node_binding interface only contains two methods: handleIncomingRequest(req, res) and handleWriteHead(res, statusCode). The handleRequestFinish hook and other lifecycle hooks were removed during simplification. Both current methods are fully tested.
Applied to files:
test/js/web/fetch/fetch-cyclic-reference.test.ts
📚 Learning: 2025-10-20T01:38:02.660Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/BunFetchInstrumentation.ts:126-131
Timestamp: 2025-10-20T01:38:02.660Z
Learning: In BunFetchInstrumentation.ts, the force-restore to ORIGINAL_FETCH in the disable() method is intentionally kept (despite appearing unsafe) because it's required for proper test cleanup when instrumentation is repeatedly enabled/disabled. Without it, 15 distributed tracing and context propagation tests fail. Shimmer's unwrap() doesn't reliably restore the original fetch in Bun's globalThis context. The isBunOtelPatched safety check ensures the restore only happens when the current fetch is still ours, preventing clobbering of other tools' wrappers.
Applied to files:
test/js/web/fetch/fetch-cyclic-reference.test.ts
📚 Learning: 2025-10-18T05:23:24.403Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: test/js/bun/telemetry-server.test.ts:91-100
Timestamp: 2025-10-18T05:23:24.403Z
Learning: In the Bun codebase, telemetry tests (test/js/bun/telemetry-*.test.ts) should focus on telemetry API behavior: configure/disable/isEnabled, callback signatures and invocation, request ID correlation, and error handling. HTTP protocol behaviors like status code normalization (e.g., 200 with empty body → 204) should be tested in HTTP server tests (test/js/bun/http/), not in telemetry tests. Keep separation of concerns: telemetry tests verify the telemetry API contract; HTTP tests verify HTTP semantics.
Applied to files:
test/js/web/fetch/fetch-cyclic-reference.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `bun bd test <...test file>` to run tests with compiled code changes. Do not use `bun test` as it will not include your changes.
Applied to files:
test/js/web/fetch/fetch-cyclic-reference.test.ts
📚 Learning: 2025-10-19T02:44:46.354Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/context-propagation.test.ts:1-1
Timestamp: 2025-10-19T02:44:46.354Z
Learning: In the Bun repository, standalone packages under packages/ (e.g., bun-vscode, bun-inspector-protocol, bun-plugin-yaml, bun-plugin-svelte, bun-debug-adapter-protocol, bun-otel) co-locate their tests with package source code using *.test.ts files. This follows standard npm/monorepo patterns. The test/ directory hierarchy (test/js/bun/, test/cli/, test/js/node/) is reserved for testing Bun's core runtime APIs and built-in functionality, not standalone packages.
Applied to files:
test/js/web/fetch/fetch-cyclic-reference.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Applies to test/**/*-fixture.ts : Test files that spawn Bun processes should end in `*-fixture.ts` to identify them as test fixtures rather than tests themselves.
Applied to files:
test/js/web/fetch/fetch-cyclic-reference.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `bun:test` for files ending in `*.test.{ts,js,jsx,tsx,mjs,cjs}`. For test files without .test extension in test/js/node/test/{parallel,sequential}/*.js, use `bun bd <file>` instead of `bun bd test <file>` since they expect exit code 0.
Applied to files:
test/js/web/fetch/fetch-cyclic-reference.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : For multi-file tests, prefer `tempDir` and `Bun.spawn` over single-file tests
Applied to files:
test/js/web/fetch/fetch-cyclic-reference.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Do not set a timeout on tests. Bun already has timeouts built-in.
Applied to files:
test/js/web/fetch/fetch-cyclic-reference.test.ts
📚 Learning: 2025-09-20T00:57:56.685Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 22568
File: test/js/valkey/valkey.test.ts:268-276
Timestamp: 2025-09-20T00:57:56.685Z
Learning: For test/js/valkey/valkey.test.ts, do not comment on synchronous throw assertions for async Redis methods like ctx.redis.set() - the maintainer has explicitly requested to stop looking at this error pattern.
Applied to files:
test/js/web/fetch/fetch-cyclic-reference.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Use `await using` or `using` statements to ensure proper resource cleanup for Bun APIs like `Bun.listen`, `Bun.connect`, `Bun.spawn`, and `Bun.serve`.
Applied to files:
test/js/web/fetch/fetch-cyclic-reference.test.ts
📚 Learning: 2025-09-02T05:33:37.517Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 22323
File: test/js/web/websocket/websocket-subprotocol.test.ts:74-75
Timestamp: 2025-09-02T05:33:37.517Z
Learning: In Bun's runtime, `await using` with Node.js APIs like `net.createServer()` is properly supported and should not be replaced with explicit cleanup. Bun has extended Node.js APIs with proper async dispose support.
Applied to files:
test/js/web/fetch/fetch-cyclic-reference.test.ts
📚 Learning: 2025-11-08T04:06:33.198Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 24491
File: test/js/bun/transpiler/declare-global.test.ts:17-17
Timestamp: 2025-11-08T04:06:33.198Z
Learning: In Bun test files, `await using` with Bun.spawn() is the preferred pattern for spawned processes regardless of whether they are short-lived or long-running. Do not suggest replacing `await using proc = Bun.spawn(...)` with `const proc = Bun.spawn(...); await proc.exited;`.
Applied to files:
test/js/web/fetch/fetch-cyclic-reference.test.ts
📚 Learning: 2025-11-06T00:58:23.965Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 24417
File: test/js/bun/spawn/spawn.test.ts:903-918
Timestamp: 2025-11-06T00:58:23.965Z
Learning: In Bun test files, `await using` with spawn() is appropriate for long-running processes that need guaranteed cleanup on scope exit or when explicitly testing disposal behavior. For short-lived processes that exit naturally (e.g., console.log scripts), the pattern `const proc = spawn(...); await proc.exited;` is standard and more common, as evidenced by 24 instances vs 4 `await using` instances in test/js/bun/spawn/spawn.test.ts.
Applied to files:
test/js/web/fetch/fetch-cyclic-reference.test.ts
📚 Learning: 2025-11-24T18:37:11.466Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Write JS builtins for Bun's Node.js compatibility and APIs, and run `bun bd` after changes
Applied to files:
test/js/web/fetch/fetch-cyclic-reference.test.ts
📚 Learning: 2025-10-30T03:48:10.513Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 24063
File: packages/bun-otel/test/context-propagation.test.ts:1-7
Timestamp: 2025-10-30T03:48:10.513Z
Learning: In Bun test files, `using` declarations at the describe block level execute during module load/parsing, not during test execution. This means they acquire and dispose resources before any tests run. For test-scoped resource management, use beforeAll/afterAll hooks instead. The pattern `beforeAll(beforeUsingEchoServer); afterAll(afterUsingEchoServer);` is correct for managing ref-counted test resources like the EchoServer in packages/bun-otel/test/ - the using block pattern should not be used at describe-block level for test resources.
<!-- [/add_learning]
Applied to files:
test/js/web/fetch/fetch-cyclic-reference.test.ts
📚 Learning: 2025-11-13T07:23:11.159Z
Learnt from: RiskyMH
Repo: oven-sh/bun PR: 24668
File: docs/quickstart.mdx:181-197
Timestamp: 2025-11-13T07:23:11.159Z
Learning: In Bun.serve's routes configuration, imported HTML files can be assigned directly to routes without wrapping them in a function that returns a Response. For example, `import index from './index.html'` followed by `"/": index` in the routes object is valid Bun syntax for serving HTML content.
Applied to files:
test/js/web/fetch/fetch-cyclic-reference.test.ts
📚 Learning: 2025-09-17T23:42:05.812Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 22568
File: docs/api/redis.md:240-246
Timestamp: 2025-09-17T23:42:05.812Z
Learning: The RedisClient.duplicate() method in Bun's Redis implementation returns Promise<RedisClient>, not RedisClient synchronously, so await is required when calling it.
Applied to files:
test/js/web/fetch/fetch-cyclic-reference.test.ts
📚 Learning: 2025-10-25T17:20:19.041Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 24063
File: test/js/bun/telemetry/server-header-injection.test.ts:5-20
Timestamp: 2025-10-25T17:20:19.041Z
Learning: In the Bun telemetry codebase, tests are organized into two distinct layers: (1) Internal API tests in test/js/bun/telemetry/ use numeric InstrumentKind enum values to test Zig↔JS injection points and low-level integration; (2) Public API tests in packages/bun-otel/test/ use string InstrumentKind values ("http", "fetch", etc.) to test the public-facing BunSDK and instrumentation APIs. This separation allows internal tests to use efficient numeric enums for refactoring flexibility while the public API maintains a developer-friendly string-based interface.
Applied to files:
test/js/web/fetch/fetch-cyclic-reference.test.ts
🧬 Code graph analysis (1)
test/js/web/fetch/fetch-cyclic-reference.test.ts (3)
packages/bun-inspector-protocol/src/protocol/jsc/index.d.ts (2)
Response(2793-2806)Request(2787-2791)test/js/node/http/node-fetch.test.js (1)
requestBody(126-130)test/js/node/http2/node-http2-memory-leak.js (1)
heapStats(8-8)
🔇 Additional comments (1)
test/js/web/fetch/fetch-cyclic-reference.test.ts (1)
1-2: LGTM!Imports are appropriate for heap analysis and testing in Bun.
Comment on lines
+4
to
+94
| describe("FetchTasklet cyclic reference", () => { | ||
| test("fetch with request body stream should not leak with cyclic reference", async () => { | ||
| await using server = Bun.serve({ | ||
| port: 0, | ||
| async fetch(req) { | ||
| const body = await req.text(); | ||
| return new Response(`received: ${body}`); | ||
| }, | ||
| }); | ||
|
|
||
| const url = `http://localhost:${server.port}/`; | ||
|
|
||
| async function leak() { | ||
| const requestBody = new ReadableStream({ | ||
| start(controller) { | ||
| controller.enqueue(new TextEncoder().encode("request body")); | ||
| controller.close(); | ||
| }, | ||
| }); | ||
|
|
||
| const request = new Request(url, { | ||
| method: "POST", | ||
| body: requestBody, | ||
| }); | ||
|
|
||
| // Create cyclic reference | ||
| // @ts-ignore | ||
| requestBody.request = request; | ||
| // @ts-ignore | ||
| request.bodyStream = requestBody; | ||
|
|
||
| const response = await fetch(request); | ||
| return await response.text(); | ||
| } | ||
|
|
||
| for (let i = 0; i < 500; i++) { | ||
| await leak(); | ||
| } | ||
|
|
||
| await Bun.sleep(10); | ||
| Bun.gc(true); | ||
| await Bun.sleep(10); | ||
| Bun.gc(true); | ||
|
|
||
| const requestCount = heapStats().objectTypeCounts.Request || 0; | ||
| const readableStreamCount = heapStats().objectTypeCounts.ReadableStream || 0; | ||
| expect(requestCount).toBeLessThanOrEqual(100); | ||
| expect(readableStreamCount).toBeLessThanOrEqual(100); | ||
| }); | ||
|
|
||
| test("fetch with ReadableStream body should not leak streams", async () => { | ||
| await using server = Bun.serve({ | ||
| port: 0, | ||
| async fetch(req) { | ||
| const body = await req.text(); | ||
| return new Response(`received: ${body}`); | ||
| }, | ||
| }); | ||
|
|
||
| const url = `http://localhost:${server.port}/`; | ||
|
|
||
| async function leak() { | ||
| const requestBody = new ReadableStream({ | ||
| start(controller) { | ||
| controller.enqueue(new TextEncoder().encode("request body")); | ||
| controller.close(); | ||
| }, | ||
| }); | ||
|
|
||
| // Use ReadableStream directly with fetch, no Request object, no cyclic reference | ||
| const response = await fetch(url, { | ||
| method: "POST", | ||
| body: requestBody, | ||
| }); | ||
| return await response.text(); | ||
| } | ||
|
|
||
| for (let i = 0; i < 500; i++) { | ||
| await leak(); | ||
| } | ||
|
|
||
| await Bun.sleep(10); | ||
| Bun.gc(true); | ||
| await Bun.sleep(10); | ||
| Bun.gc(true); | ||
|
|
||
| const readableStreamCount = heapStats().objectTypeCounts.ReadableStream || 0; | ||
| // This currently fails with ~502 streams leaked | ||
| expect(readableStreamCount).toBeLessThanOrEqual(100); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting common test patterns into helper functions.
Both test cases follow an identical pattern:
- Set up server with await using
- Define a leak() function
- Run 500 iterations
- Execute double GC sequence (sleep → gc → sleep → gc)
- Assert heap counts
Consider extracting the GC sequence and iteration logic into helper functions to reduce duplication and improve maintainability.
Example refactor
async function forceGarbageCollection() {
await Bun.sleep(10);
Bun.gc(true);
await Bun.sleep(10);
Bun.gc(true);
}
async function runLeakTest(leakFn: () => Promise<any>, iterations: number) {
for (let i = 0; i < iterations; i++) {
await leakFn();
}
await forceGarbageCollection();
}🤖 Prompt for AI Agents
In @test/js/web/fetch/fetch-cyclic-reference.test.ts around lines 4 - 94, Tests
duplicate the iteration + double-GC pattern; extract that logic into helpers to
reduce duplication. Add a helper function (e.g., forceGarbageCollection) that
performs the await Bun.sleep(10); Bun.gc(true); await Bun.sleep(10);
Bun.gc(true) sequence and another helper (e.g., runLeakTest) that accepts the
leak function and iterations, runs the loop calling leak(), then calls
forceGarbageCollection; update both test cases ("fetch with request body stream
should not leak with cyclic reference" and "fetch with ReadableStream body
should not leak streams") to call runLeakTest(leak, 500) and keep their server
setup (Bun.serve), leak() definitions, and final heapStats assertions unchanged.
Comment on lines
+39
to
+51
| for (let i = 0; i < 500; i++) { | ||
| await leak(); | ||
| } | ||
|
|
||
| await Bun.sleep(10); | ||
| Bun.gc(true); | ||
| await Bun.sleep(10); | ||
| Bun.gc(true); | ||
|
|
||
| const requestCount = heapStats().objectTypeCounts.Request || 0; | ||
| const readableStreamCount = heapStats().objectTypeCounts.ReadableStream || 0; | ||
| expect(requestCount).toBeLessThanOrEqual(100); | ||
| expect(readableStreamCount).toBeLessThanOrEqual(100); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting magic numbers to named constants.
The test uses several magic numbers without explanation:
- 500 iterations
- 100 object threshold
- 10ms sleep duration
Extract these to named constants at the top of the test suite to improve maintainability and document the rationale. For example, MAX_LEAKED_OBJECTS = 100 with a comment explaining why 100 is acceptable after 500 iterations.
🤖 Prompt for AI Agents
In @test/js/web/fetch/fetch-cyclic-reference.test.ts around lines 39 - 51,
Extract the magic numbers used in the test into named constants at the top of
the test file (e.g., const LEAK_ITERATIONS = 500, const MAX_LEAKED_OBJECTS =
100, const SLEEP_MS = 10) with a short comment explaining the rationale (for
example, "MAX_LEAKED_OBJECTS = 100 is acceptable after LEAK_ITERATIONS
iterations"). Replace the literals in the loop (for (let i = 0; i < 500; i++)),
the Bun.sleep calls (Bun.sleep(10)), and the assertions
(toBeLessThanOrEqual(100)) with the new constants; keep references to leak(),
Bun.gc(true), and heapStats().objectTypeCounts.Request/ReadableStream unchanged.
sosukesuzuki
added a commit
that referenced
this pull request
Jan 11, 2026
When proxying streaming responses through Bun's HTTP server, a Strong reference leak occurred in RequestContext.doRenderWithBody(). The issue happened when: 1. A streaming response body was received (e.g., from a proxied fetch) 2. The response_body_readable_stream_ref was initialized (line 1836 or 1841) 3. For .Bytes streams with has_received_last_chunk=false, a new Strong reference was created (line 1902) without deinit'ing the old one This caused the Strong.Impl memory to leak on every streaming response, accumulating over time. The fix adds a deinit() call before creating the new Strong reference. This is safe because the stream variable exists as a stack-local variable and JSC's conservative GC tracks stack-local JSValues, so there's no risk of the stream being collected between deinit and the new Strong.init(). Similar to the fixes in #23313 and #25846 for fetch-related Strong ref leaks. Claude-Generated-By: Claude Code (cli/claude-opus-4-5=100%) Claude-Steers: 6 Claude-Permission-Prompts: 3 Claude-Escapes: 0
Jarred-Sumner
pushed a commit
that referenced
this pull request
Jan 12, 2026
…5965) ## Summary Fix a memory leak in `RequestContext.doRenderWithBody()` where `Strong.Impl` memory was leaked when proxying streaming responses through Bun's HTTP server. ## Problem When a streaming response (e.g., from a proxied fetch request) was forwarded through Bun's server: 1. `response_body_readable_stream_ref` was initialized at line 1836 (from `lock.readable`) or line 1841 (via `Strong.init()`) 2. For `.Bytes` streams with `has_received_last_chunk=false`, a **new** Strong reference was created at line 1902 3. The old Strong reference was **never deinit'd**, causing `Strong.Impl` memory to leak This leak accumulated over time with every streaming response proxied through the server. ## Solution Add `this.response_body_readable_stream_ref.deinit()` before creating the new Strong reference. This is safe because: - `stream` exists as a stack-local variable - JSC's conservative GC tracks stack-local JSValues - No GC can occur between consecutive synchronous Zig statements - Therefore, `stream` won't be collected between `deinit()` and `Strong.init()` ## Test Added `test/js/web/fetch/server-response-stream-leak.test.ts` which: - Creates a backend server that returns delayed streaming responses - Creates a proxy server that forwards the streaming responses - Makes 200 requests and checks that ReadableStream objects don't accumulate - Fails on system Bun v1.3.5 (202 leaked), passes with the fix ## Related Similar to the Strong reference leak fixes in: - #23313 (fetch memory leak) - #25846 (fetch cyclic reference leak)
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR fixes a memory leak that occurs when
fetch()is called with aReadableStreambody. The ReadableStream objects were not being properly released, causing them to accumulate in memory.Problem
When using
fetch()with a ReadableStream body:The ReadableStream objects leak because
FetchTasklet.clearData()has a conditional that preventsdetach()from being called on ReadableStream request bodies after streaming has started.Root Cause
The problematic condition in
clearData():After
startRequestStream()is called:is_waiting_request_stream_startbecomesfalserequest_bodyis still.ReadableStream(false or false) = falsedetach()is skipped → memory leakWhy the Original Code Was Wrong
The original code appears to assume that when
startRequestStream()is called, ownership of the Strong reference is transferred toResumableSink. However, this is incorrect:startRequestStream()creates a new independent Strong reference inResumableSink(seeResumableSink.zig:119)deinit()on one does not affect the otherSolution
Remove the conditional and always call
detach():Safety Analysis
This change is safe because:
detach()is safe to call on already-detached referencesclearData()is only called fromdeinit()after streaming has completed (ref_count = 0)deinit()only runs when ref_count reaches 0, which means all streaming operations have completedTest Results
Before fix (with system Bun):
After fix:
Test Coverage
Added comprehensive tests in
test/js/web/fetch/fetch-cyclic-reference.test.tscovering: