fix: Response.clone() no longer locks body when body was accessed before clone by robobun · Pull Request #25484 · oven-sh/bun

robobun · 2025-12-12T05:37:17Z

Summary

Fix bug where Response.clone() would lock the original response's body when response.body was accessed before cloning
Apply the same fix to Request.clone()

Root Cause

When response.body was accessed before calling response.clone(), the original response's body would become locked after cloning. This happened because:

When the cloned response was wrapped with toJS(), checkBodyStreamRef() was called which moved the stream from Locked.readable to js.gc.stream and cleared Locked.readable
The subsequent code tried to get the stream from Locked.readable, which was now empty, so the body cache update was skipped
The JavaScript-level body property cache still held the old locked stream

Fix

Updated the cache update logic to:

For the cloned response: use js.gc.stream.get() instead of Locked.readable.get() since toJS() already moved the stream
For the original response: use Locked.readable.get() which still holds the teed stream since checkBodyStreamRef hasn't been called yet

Reproduction

const readableStream = new ReadableStream({
  start(controller) {
    controller.enqueue(new TextEncoder().encode("Hello, world!"));
    controller.close();
  },
});

const response = new Response(readableStream);
console.log(response.body?.locked); // Accessing body before clone
const cloned = response.clone();
console.log(response.body?.locked); // Expected: false, Actual: true ❌
console.log(cloned.body?.locked);   // Expected: false, Actual: false ✅

Test plan

Added regression tests for Response.clone() in test/js/web/fetch/response.test.ts
Added regression test for Request.clone() in test/js/web/request/request.test.ts
Verified tests fail with system bun (before fix) and pass with debug build (after fix)

🤖 Generated with Claude Code

…ore clone When `response.body` was accessed before calling `response.clone()`, the original response's body would become locked after cloning. This happened because: 1. When the cloned response was wrapped with `toJS()`, `checkBodyStreamRef` was called which moved the stream from `Locked.readable` to `js.gc.stream` and cleared `Locked.readable` 2. The subsequent code tried to get the stream from `Locked.readable`, which was now empty, so the body cache update was skipped 3. The JavaScript-level body property cache still held the old locked stream The fix updates the cache update logic to: 1. For the cloned response: use `js.gc.stream.get()` instead of `Locked.readable.get()` since `toJS()` already moved the stream 2. For the original response: use `Locked.readable.get()` which still holds the teed stream since `checkBodyStreamRef` hasn't been called yet This also applies the same fix to Request.clone(). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>

robobun · 2025-12-12T05:37:26Z

^{Updated 1:26 PM PT - Dec 15th, 2025}

@Jarred-Sumner, your commit dccdf82 is building: #33356

coderabbitai · 2025-12-12T05:40:38Z

Walkthrough

The PR refactors cloning logic in Request and Response by updating how body caches are synchronized during clone operations. It replaces direct stream reuse with retrieving teed streams via js.gc.stream and ensures both cloned and original body caches are updated before stream relocation. Tests validate that cloning preserves body readability without locking the original.

Changes

Cohort / File(s)	Change Summary
Body cache handling in clone methods `src/bun.js/webcore/Request.zig`, `src/bun.js/webcore/Response.zig`	Refactored cloning logic to use `js.gc.stream` for retrieving teed streams and `js.bodySetCached` for cache updates. Added finalization steps to update original body cache when `.Locked` and a readable stream exists. Reordered `checkBodyStreamRef` call to occur after cache updates.
Clone behavior tests `test/js/web/fetch/response.test.ts`, `test/js/web/request/request.test.ts`	Updated Response size snapshot (4.15 KB → 5.83 KB). Added tests for `clone()` verifying that cloning a request or response with an accessed or unaccessed body leaves both original and clone unlocked and readable with identical content.

Possibly related PRs

refactor(Response) isolate body usage #23313: Directly related refactor of Request/Response cloning and body-readable-stream handling codepaths, including additions of cloneWithReadableStream, getBodyReadableStream, and checkBodyStreamRef logic.

Suggested reviewers

nektro

Pre-merge checks

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically describes the main bug fix: Response.clone() no longer locking the body when accessed before clone.
Description check	✅ Passed	The description comprehensively covers required sections with summary, root cause analysis, fix explanation, reproduction steps, and test plan verification.

📜 Recent 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 7dcd49f and 8cbc5fa.

📒 Files selected for processing (4)

src/bun.js/webcore/Request.zig (1 hunks)
src/bun.js/webcore/Response.zig (1 hunks)
test/js/web/fetch/response.test.ts (2 hunks)
test/js/web/request/request.test.ts (1 hunks)

🧰 Additional context used

📓 Path-based instructions (6)

test/**/*.{js,ts,jsx,tsx}

📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)

test/**/*.{js,ts,jsx,tsx}: Write tests as JavaScript and TypeScript files using Jest-style APIs (test, describe, expect) and import from bun:test
Use test.each and data-driven tests to reduce boilerplate when testing multiple similar cases

Files:

test/js/web/request/request.test.ts
test/js/web/fetch/response.test.ts

test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}

📄 CodeRabbit inference engine (test/CLAUDE.md)

test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Use bun:test with files that end in *.test.{ts,js,jsx,tsx,mjs,cjs}
Do not write flaky tests. Never wait for time to pass in tests; always wait for the condition to be met instead of using an arbitrary amount of time
Never use hardcoded port numbers in tests. Always use port: 0 to get a random port
Prefer concurrent tests over sequential tests using test.concurrent or describe.concurrent when multiple tests spawn processes or write files, unless it's very difficult to make them concurrent
When spawning Bun processes in tests, use bunExe and bunEnv from harness to ensure the same build of Bun is used and debug logging is silenced
Use -e flag for single-file tests when spawning Bun processes
Use tempDir() from harness to create temporary directories with files for multi-file tests instead of creating files manually
Prefer async/await over callbacks in tests
When callbacks must be used and it's just a single callback, use Promise.withResolvers to create a promise that can be resolved or rejected from a callback
Do not set a timeout on tests. Bun already has timeouts
Use Buffer.alloc(count, fill).toString() instead of 'A'.repeat(count) to create repetitive strings in tests, as ''.repeat is very slow in debug JavaScriptCore builds
Use describe blocks for grouping related tests
Always use await using or using to ensure proper resource cleanup in tests for APIs like Bun.listen, Bun.connect, Bun.spawn, Bun.serve, etc
Always check exit codes and test error scenarios in error tests
Use describe.each() for parameterized tests
Use toMatchSnapshot() for snapshot testing
Use beforeAll(), afterEach(), beforeEach() for setup/teardown in tests
Track resources (servers, clients) in arrays for cleanup in afterEach()

Files:

test/js/web/request/request.test.ts
test/js/web/fetch/response.test.ts

test/**/*.test.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

test/**/*.test.{ts,tsx}: For single-file tests in Bun test suite, prefer using -e flag over tempDir
For multi-file tests in Bun test suite, prefer using tempDir and Bun.spawn
Always use port: 0 when spawning servers in tests - do not hardcode ports or use custom random port functions
Use normalizeBunSnapshot to normalize snapshot output in tests instead of manual output comparison
Never write tests that check for no 'panic', 'uncaught exception', or similar strings in test output - that is not a valid test
Use tempDir from harness to create temporary directories in tests - do not use tmpdirSync or fs.mkdtempSync
In tests, call expect(stdout).toBe(...) before expect(exitCode).toBe(0) when spawning processes for more useful error messages on failure
Do not write flaky tests - do not use setTimeout in tests; instead await the condition to be met since you're testing the CONDITION, not TIME PASSING
Verify your test fails with USE_SYSTEM_BUN=1 bun test <file> and passes with bun bd test <file> - tests are not valid if they pass with USE_SYSTEM_BUN=1
Avoid shell commands in tests - do not use find or grep; use Bun's Glob and built-in tools instead
Test files must end in .test.ts or .test.tsx and be created in the appropriate test folder structure

Files:

test/js/web/request/request.test.ts
test/js/web/fetch/response.test.ts

src/**/*.{cpp,zig}

📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)

src/**/*.{cpp,zig}: Use bun bd or bun run build:debug to build debug versions for C++ and Zig source files; creates debug build at ./build/debug/bun-debug
Run tests using bun bd test <test-file> with the debug build; never use bun test directly as it will not include your changes
Execute files using bun bd <file> <...args>; never use bun <file> directly as it will not include your changes
Enable debug logs for specific scopes using BUN_DEBUG_$(SCOPE)=1 environment variable
Code generation happens automatically as part of the build process; no manual code generation commands are required

Files:

src/bun.js/webcore/Request.zig
src/bun.js/webcore/Response.zig

src/**/*.zig

📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)

Use bun.Output.scoped(.${SCOPE}, .hidden) for creating debug logs in Zig code

Implement core functionality in Zig, typically in its own directory in src/

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 @import at the bottom of the file (auto formatter will move them automatically)

Be careful with memory management in Zig code - use defer for cleanup with allocators

Files:

src/bun.js/webcore/Request.zig
src/bun.js/webcore/Response.zig

**/*.zig

📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)

**/*.zig: Expose generated bindings in Zig structs using pub const js = JSC.Codegen.JS<ClassName> with trait conversion methods: toJS, fromJS, and fromJSDirect
Use consistent parameter name globalObject instead of ctx in Zig constructor and method implementations
Use bun.JSError!JSValue return type for Zig methods and constructors to enable proper error handling and exception propagation
Implement resource cleanup using deinit() method that releases resources, followed by finalize() called by the GC that invokes deinit() and frees the pointer
Use JSC.markBinding(@src()) in finalize methods for debugging purposes before calling deinit()
For methods returning cached properties in Zig, declare external C++ functions using extern fn and callconv(JSC.conv) calling convention
Implement getter functions with naming pattern get<PropertyName> in Zig that accept this and globalObject parameters and return JSC.JSValue
Access JavaScript CallFrame arguments using callFrame.argument(i), check argument count with callFrame.argumentCount(), and get this with callFrame.thisValue()
For reference-counted objects, use .deref() in finalize instead of destroy() to release references to other JS objects

Files:

src/bun.js/webcore/Request.zig
src/bun.js/webcore/Response.zig

🧠 Learnings (16)

📚 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/request/request.test.ts
test/js/web/fetch/response.test.ts
src/bun.js/webcore/Request.zig

📚 Learning: 2025-11-24T18:35:50.422Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:50.422Z
Learning: When fixing a Node.js compatibility bug, first verify the expected behavior works in Node.js before testing against Bun's canary version

Applied to files:

test/js/web/request/request.test.ts

📚 Learning: 2025-11-24T18:37:30.259Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Always use `await using` or `using` to ensure proper resource cleanup in tests for APIs like Bun.listen, Bun.connect, Bun.spawn, Bun.serve, etc

Applied to files:

test/js/web/request/request.test.ts

📚 Learning: 2025-11-24T18:35:08.612Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:08.612Z
Learning: Applies to test/bake/**/*.test.ts : Use `dev.fetch()` for running HTTP requests in tests

Applied to files:

test/js/web/fetch/response.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/response.test.ts

📚 Learning: 2025-12-02T05:59:51.485Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-02T05:59:51.485Z
Learning: Applies to test/**/*.test.{ts,tsx} : Use `normalizeBunSnapshot` to normalize snapshot output in tests instead of manual output comparison

Applied to files:

test/js/web/fetch/response.test.ts

📚 Learning: 2025-09-30T22:53:19.887Z

Learnt from: pfgithub
Repo: oven-sh/bun PR: 23117
File: src/bun.js/test/snapshot.zig:265-276
Timestamp: 2025-09-30T22:53:19.887Z
Learning: In Bun's snapshot testing (src/bun.js/test/snapshot.zig), multiple inline snapshots at the same line and column (same call position) must have identical values. However, multiple inline snapshots on the same line at different columns are allowed to have different values. The check is position-specific (line+col), not line-wide.

Applied to files:

test/js/web/fetch/response.test.ts

📚 Learning: 2025-11-24T18:35:50.422Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:50.422Z
Learning: Applies to test/**/*.{js,ts,jsx,tsx} : Write tests as JavaScript and TypeScript files using Jest-style APIs (`test`, `describe`, `expect`) and import from `bun:test`

Applied to files:

test/js/web/fetch/response.test.ts

📚 Learning: 2025-11-24T18:37:30.259Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `toMatchSnapshot()` for snapshot testing

Applied to files:

test/js/web/fetch/response.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/response.test.ts

📚 Learning: 2025-08-31T09:08:12.104Z

Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 22279
File: test/js/web/structured-clone-fastpath.test.ts:12-12
Timestamp: 2025-08-31T09:08:12.104Z
Learning: In Bun, process.memoryUsage.rss() is called as a direct function, not as process.memoryUsage().rss like in Node.js. This is the correct API shape for Bun.

Applied to files:

test/js/web/fetch/response.test.ts

📚 Learning: 2025-12-02T05:59:51.485Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-02T05:59:51.485Z
Learning: Applies to test/**/*.test.{ts,tsx} : Verify your test fails with `USE_SYSTEM_BUN=1 bun test <file>` and passes with `bun bd test <file>` - tests are not valid if they pass with `USE_SYSTEM_BUN=1`

Applied to files:

test/js/web/fetch/response.test.ts

📚 Learning: 2025-09-05T18:45:29.200Z

Learnt from: markovejnovic
Repo: oven-sh/bun PR: 21728
File: src/valkey/js_valkey.zig:0-0
Timestamp: 2025-09-05T18:45:29.200Z
Learning: In JSValkeyClient.cloneWithoutConnecting() in src/valkey/js_valkey.zig, the address/username/password fields must be repointed to the duplicated connection_strings buffer to avoid use-after-free when the original client is destroyed. The original client properly frees connection_strings in ValkeyClient.deinit().

Applied to files:

src/bun.js/webcore/Request.zig
src/bun.js/webcore/Response.zig

📚 Learning: 2025-12-02T05:59:51.485Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-02T05:59:51.485Z
Learning: Applies to src/bun.js/bindings/**/*.cpp : Cache structures in ZigGlobalObject for C++ JavaScript classes

Applied to files:

src/bun.js/webcore/Request.zig
src/bun.js/webcore/Response.zig

📚 Learning: 2025-11-24T18:35:39.205Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-11-24T18:35:39.205Z
Learning: Applies to src/bun.js/api/BunObject.zig : Implement getter functions in `src/bun.js/api/BunObject.zig` that return your feature, and export them in the `exportAll()` function

Applied to files:

src/bun.js/webcore/Request.zig

📚 Learning: 2025-10-19T03:01:29.084Z

Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: src/bun.js/telemetry.zig:286-289
Timestamp: 2025-10-19T03:01:29.084Z
Learning: In src/bun.js/telemetry.zig, the guard preventing double configuration (lines 213-216) is intentional. The telemetry API uses a single-shot configuration model where configure() is called once during application startup. Users must call Bun.telemetry.configure(null) to reset before reconfiguring. This design ensures: (1) predictable state—callbacks don't change mid-request, avoiding race conditions; (2) zero overhead when disabled—no checking for callback changes on every request; (3) clear ownership—one adapter (e.g., bun-otel) owns the telemetry config. Merge-style reconfiguration would require atomic updates during active requests, adding overhead to the hot path.
<!-- [/add_learning]

Applied to files:

src/bun.js/webcore/Request.zig

🧬 Code graph analysis (2)

test/js/web/request/request.test.ts (1)

packages/bun-inspector-protocol/src/protocol/jsc/index.d.ts (1)

Request (2787-2791)

test/js/web/fetch/response.test.ts (1)

packages/bun-inspector-protocol/src/protocol/jsc/index.d.ts (1)

Response (2793-2806)

🔇 Additional comments (8)

src/bun.js/webcore/Response.zig (2)

384-390: LGTM! Cloned body cache correctly updated from js.gc.stream.

The logic correctly retrieves the teed stream from js.gc.stream after makeMaybePooled has called toJS() → checkBodyStreamRef(), which moved the stream out of Locked.readable.

392-399: LGTM! Original body cache updated before stream relocation.

This ensures the original response's cached body property points to the new teed stream by reading from Locked.readable before checkBodyStreamRef moves it. This is the key fix that prevents the original body from appearing locked after clone.

test/js/web/fetch/response.test.ts (3)

52-52: Snapshot size update is expected.

The Response size increased from 4.15 KB to 5.83 KB because the test file grew with the new clone() tests added below.

127-153: Excellent regression test for the clone() body-locking bug.

This test directly reproduces the bug: accessing response.body before calling clone() would cause the original body to appear locked. The fix ensures both original and cloned bodies remain unlocked and readable.

155-178: Good coverage for the normal clone path.

This test verifies that cloning still works correctly when the body is not accessed before clone.

test/js/web/request/request.test.ts (1)

51-76: Excellent regression test for Request.clone() body-locking bug.

This test mirrors the Response test and verifies the same fix applies to Request: accessing request.body before calling clone() should not cause the original body to become locked. Both original and cloned requests remain unlocked and readable.

src/bun.js/webcore/Request.zig (2)

869-875: LGTM! Cloned request body cache correctly updated from js.gc.stream.

The logic correctly retrieves the teed stream from js.gc.stream after toJS() has called checkBodyStreamRef(), which moved the stream out of Locked.readable. This matches the fix in Response.zig.

877-884: LGTM! Original request body cache updated before stream relocation.

This ensures the original request's cached body property points to the new teed stream by reading from Locked.readable before checkBodyStreamRef moves it. The fix is consistent with Response.zig and correctly prevents the original body from appearing locked after clone.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

github-actions Bot added the claude label Dec 12, 2025

Merge branch 'main' into claude/fix-response-clone-body-locked

dccdf82

Jarred-Sumner merged commit 344b2c1 into main Dec 16, 2025
54 of 55 checks passed

Jarred-Sumner deleted the claude/fix-response-clone-body-locked branch December 16, 2025 02:46

coderabbitai Bot mentioned this pull request Jan 15, 2026

fix(Request): set cache and mode options correctly #26099

Merged

7 tasks

coderabbitai Bot mentioned this pull request Jan 23, 2026

fix(http): set #js_ref in Request.toJS for server-created requests #26390

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Response.clone() no longer locks body when body was accessed before clone#25484

fix: Response.clone() no longer locks body when body was accessed before clone#25484
Jarred-Sumner merged 2 commits into
mainfrom
claude/fix-response-clone-body-locked

robobun commented Dec 12, 2025

Uh oh!

robobun commented Dec 12, 2025 •

edited

Loading

Uh oh!

coderabbitai Bot commented Dec 12, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

robobun commented Dec 12, 2025

Summary

Root Cause

Fix

Reproduction

Test plan

Uh oh!

robobun commented Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented Dec 12, 2025

Walkthrough

Changes

Possibly related PRs

Suggested reviewers

Pre-merge checks

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

robobun commented Dec 12, 2025 •

edited

Loading