Fix ReadableStream reference management for Request/Response bodies#23254
Fix ReadableStream reference management for Request/Response bodies#23254robobun wants to merge 24 commits into
Conversation
This commit fixes several issues with the new ReadableStream.Ref system that caused body-stream tests to hang: 1. **Body.zig:1020** - Fixed owner mismatch when setting stream after draining - Was using `.strong` owner when should use actual owner (Request/Response) - This caused Ref.get() to fail since Ref type didn't match owner type 2. **Body.zig:964,1026** - Set proper Ref type for cloned bodies - When tee() returns streams via array, cloned body gets proper Ref type - Ensures cloned body can access its stream via GC cache 3. **Request.zig:819** - Fixed stream assignment in doClone - Stream[0] should go to original request, not cloned request 4. **ReadableStream.zig:108** - Always update original Ref after tee() - Original body's Ref must point to its tee'd stream 5. **body-stream.test.ts** - Fixed test discovery hang - Changed from eager to lazy array creation using factory function - Prevents creating hundreds of 2MB arrays during test discovery - Arrays now created on-demand when each test runs 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…alue The issue was that when Request.getBody() or Response.getBody() is called without a valid JSValue owner (e.g., owner.Request == .zero), the code was trying to store streams in the GC cache, which failed silently, leading to null reference panics and hanging tests. Changes: 1. **Body.zig:443-456** - When converting blob to stream in toReadableStream() - Check if owner has valid JSValue - Use .strong ref if JSValue is .zero or owner is .empty - Only set GC cache if owner has valid JSValue 2. **Body.zig:502-519** - When creating stream from drain in toReadableStream() - Same logic: use .strong if no valid owner JSValue - Fixes panics from unwrapping null on lines 504/507 3. **ReadableStream.zig:127-142** - Fix Ref.init() for empty/strong owners - Return .empty instead of trying to create .strong without a stream - Caller should use .set() to populate This fixes the body-stream.test.ts hang. The tests now pass but the full suite (4,883 tests) takes a very long time (~3s/test for large streaming tests = ~4 hours total). Individual test subsets all pass correctly. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
WalkthroughThreads an explicit ReadableStream Owner through Body, ReadableStream, Request, Response, fetch, server, bindings, and related call sites; adds Owner-aware Ref lifecycle and updates APIs/ABI to pass owner/readable_stream_tee for blob/stream operations. Changes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bun.js/webcore/Response.zig (1)
543-577: Constructor never records JS owner
Response.this_jsvaluestays empty when we construct a response from JS, because the constructor never seeds the weak ref. Any later code that triesresponse.this_jsvalue.tryGet()(e.g. the new owner-awaretoBlobIfPossible/abort paths inRequestContext) will therefore fall back to.empty, forcing strong refs and undoing the weak-owner fix this PR is chasing. We need to stashthisValuewhile we still have it—both in the S3 fast path and the general constructor path—so owner-aware logic can recover the JS wrapper.Please initialize
this_jsvalueinside the constructor, e.g.:@@ - var response: Response = .{ + var response: Response = .{ .init = Response.Init{ .status_code = 302, }, .body = Body{ .value = .{ .Empty = {} }, }, .url = bun.String.empty, }; + if (thisValue != .zero) { + response.this_jsvalue = .initWeak(thisValue); + } @@ - var response = bun.new(Response, Response{ + var response = bun.new(Response, Response{ .body = body, .init = init, }); + if (thisValue != .zero) { + response.this_jsvalue = .initWeak(thisValue); + }Also applies to: 626-634
📜 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 (21)
src/bun.js/api/bun/spawn/stdio.zig(3 hunks)src/bun.js/api/html_rewriter.zig(1 hunks)src/bun.js/api/server.zig(1 hunks)src/bun.js/api/server/FileRoute.zig(1 hunks)src/bun.js/api/server/RequestContext.zig(14 hunks)src/bun.js/api/server/StaticRoute.zig(1 hunks)src/bun.js/bindings/JSBunRequest.cpp(2 hunks)src/bun.js/bindings/JSGlobalObject.zig(1 hunks)src/bun.js/node/types.zig(2 hunks)src/bun.js/webcore/BakeResponse.zig(1 hunks)src/bun.js/webcore/Blob.zig(2 hunks)src/bun.js/webcore/Body.zig(42 hunks)src/bun.js/webcore/ReadableStream.zig(2 hunks)src/bun.js/webcore/Request.zig(14 hunks)src/bun.js/webcore/Response.zig(10 hunks)src/bun.js/webcore/fetch.zig(5 hunks)src/bun.js/webcore/response.classes.ts(2 hunks)src/codegen/generate-classes.ts(1 hunks)src/codegen/generate-jssink.ts(1 hunks)src/shell/states/Cmd.zig(1 hunks)test/js/web/fetch/body-stream.test.ts(5 hunks)
🧰 Additional context used
📓 Path-based instructions (17)
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("...", .{})In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: In Zig source files, place all @import statements at the bottom of the file
Use @import("bun") instead of @import("root").bun when importing Bun in Zig
Files:
src/bun.js/api/html_rewriter.zigsrc/bun.js/api/server/FileRoute.zigsrc/bun.js/api/server/StaticRoute.zigsrc/bun.js/webcore/Blob.zigsrc/bun.js/node/types.zigsrc/bun.js/api/bun/spawn/stdio.zigsrc/bun.js/bindings/JSGlobalObject.zigsrc/bun.js/webcore/Response.zigsrc/bun.js/api/server.zigsrc/shell/states/Cmd.zigsrc/bun.js/webcore/BakeResponse.zigsrc/bun.js/api/server/RequestContext.zigsrc/bun.js/webcore/Request.zigsrc/bun.js/webcore/fetch.zigsrc/bun.js/webcore/Body.zigsrc/bun.js/webcore/ReadableStream.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
Files:
src/bun.js/api/html_rewriter.zigsrc/bun.js/api/server/FileRoute.zigsrc/bun.js/api/server/StaticRoute.zigsrc/bun.js/webcore/Blob.zigsrc/bun.js/node/types.zigsrc/bun.js/api/bun/spawn/stdio.zigsrc/bun.js/bindings/JSGlobalObject.zigsrc/bun.js/webcore/Response.zigsrc/bun.js/api/server.zigsrc/shell/states/Cmd.zigsrc/bun.js/webcore/BakeResponse.zigsrc/bun.js/api/server/RequestContext.zigsrc/bun.js/webcore/Request.zigsrc/bun.js/webcore/fetch.zigsrc/bun.js/webcore/Body.zigsrc/bun.js/webcore/ReadableStream.zig
src/{**/js_*.zig,bun.js/api/**/*.zig}
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Files:
src/bun.js/api/html_rewriter.zigsrc/bun.js/api/server/FileRoute.zigsrc/bun.js/api/server/StaticRoute.zigsrc/bun.js/api/bun/spawn/stdio.zigsrc/bun.js/api/server.zigsrc/bun.js/api/server/RequestContext.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/api/html_rewriter.zigsrc/bun.js/api/server/FileRoute.zigsrc/bun.js/api/server/StaticRoute.zigsrc/bun.js/webcore/Blob.zigsrc/bun.js/node/types.zigsrc/bun.js/api/bun/spawn/stdio.zigsrc/bun.js/bindings/JSGlobalObject.zigsrc/bun.js/webcore/Response.zigsrc/bun.js/api/server.zigsrc/bun.js/webcore/BakeResponse.zigsrc/bun.js/api/server/RequestContext.zigsrc/bun.js/webcore/Request.zigsrc/bun.js/webcore/fetch.zigsrc/bun.js/webcore/Body.zigsrc/bun.js/webcore/ReadableStream.zig
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/js/web/fetch/body-stream.test.ts
test/js/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place JavaScript and TypeScript tests under test/js/
Files:
test/js/web/fetch/body-stream.test.ts
test/js/web/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place Web API tests under test/js/web/, separated by category
Files:
test/js/web/fetch/body-stream.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/js/web/fetch/body-stream.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/js/web/fetch/body-stream.test.ts
test/js/**
📄 CodeRabbit inference engine (test/CLAUDE.md)
Organize unit tests for specific features under
test/js/by module
Files:
test/js/web/fetch/body-stream.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must live under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or roll your own random port
Prefer normalizeBunSnapshot for snapshotting test output instead of asserting raw strings
Do not write tests that assert absence of crashes (e.g., 'no panic' or 'no uncaught exception')
Use Bun’s Jest-compatible runner (import { test, expect } from "bun:test") for tests
Avoid shell commands like find or grep in tests; use Bun’s Glob and built-in tools instead
Prefer running tests via bun bd test and use provided harness utilities (bunEnv, bunExe, tempDir)
Use Bun.spawn with proper stdio handling and await proc.exited in process-spawning tests
Files:
test/js/web/fetch/body-stream.test.ts
test/js/web/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place Web API tests under test/js/web/
Files:
test/js/web/fetch/body-stream.test.ts
**/*.{cpp,h}
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.{cpp,h}: When exposing a JS class with public Constructor and Prototype, define three C++ types: class Foo : public JSC::DestructibleObject (if it has C++ fields), class FooPrototype : public JSC::JSNonFinalObject, and class FooConstructor : public JSC::InternalFunction
If the class has C++ data members, inherit from JSC::DestructibleObject and provide proper destruction; if it has no C++ fields (only JS properties), avoid a class and use JSC::constructEmptyObject(vm, structure) with putDirectOffset
Prefer placing the subspaceFor implementation in the .cpp file rather than the header when possible
Files:
src/bun.js/bindings/JSBunRequest.cpp
**/*.cpp
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.cpp: Include "root.h" at the top of C++ binding files to satisfy lints
Define prototype properties using a const HashTableValue array and declare accessors/functions with JSC_DECLARE_* macros
Prototype classes should subclass JSC::JSNonFinalObject, provide create/createStructure, DECLARE_INFO, finishCreation that reifies static properties, and set mayBePrototype on the Structure
Custom getters should use JSC_DEFINE_CUSTOM_GETTER, jsDynamicCast to validate this, and throwThisTypeError on mismatch
Custom setters should use JSC_DEFINE_CUSTOM_SETTER, validate this via jsDynamicCast, and store via WriteBarrier/set semantics
Prototype functions should use JSC_DEFINE_HOST_FUNCTION, validate this with jsDynamicCast, and return encoded JSValue
Constructors should subclass JSC::InternalFunction, return internalFunctionSpace in subspaceFor, set the prototype property as non-configurable/non-writable, and provide create/createStructure
Provide a setup function that builds the Prototype, Constructor, and Structure, and assigns them to the LazyClassStructure initializer
Use the cached Structure via globalObject->m_.get(globalObject) when constructing instances
Expose constructors to Zig via an extern "C" function that returns the constructor from the LazyClassStructure
Provide an extern "C" Bun____toJS function that creates an instance using the cached Structure and returns an EncodedJSValue
Files:
src/bun.js/bindings/JSBunRequest.cpp
src/bun.js/bindings/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
src/bun.js/bindings/**/*.cpp: When a JS class has a public constructor, implement Foo, FooPrototype, and FooConstructor (JSDestructibleObject, JSNonFinalObject, InternalFunction)
Define class properties using HashTableValue arrays in C++ bindings
Add iso subspaces for classes with C++ fields
Cache structures in ZigGlobalObject for C++-backed classes
Files:
src/bun.js/bindings/JSBunRequest.cpp
**/*.classes.ts
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
**/*.classes.ts: Define JavaScript class interfaces in .classes.ts using define({ name, constructor, JSType, finalize, proto: { ... } })
Use cache: true on properties that should use JSC WriteBarrier-backed caching
Files:
src/bun.js/webcore/response.classes.ts
src/**/*.classes.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Define JavaScriptCore class metadata in .classes.ts files to drive code generation
Files:
src/bun.js/webcore/response.classes.ts
🧠 Learnings (3)
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Applied to files:
src/bun.js/bindings/JSGlobalObject.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Applied to files:
src/bun.js/webcore/Response.zig
📚 Learning: 2025-09-19T19:55:22.427Z
Learnt from: markovejnovic
PR: oven-sh/bun#22568
File: src/valkey/js_valkey_functions.zig:853-854
Timestamp: 2025-09-19T19:55:22.427Z
Learning: In Bun's JSPromise API, `resolvedPromiseValue(globalThis: *JSGlobalObject, value: JSValue) JSValue` returns a JSValue directly (not a JSPromise that needs `.toJS()` called on it). This is different from `resolvedPromise()` which returns a *JSPromise that would need `.toJS()`.
Applied to files:
src/bun.js/webcore/Request.zig
🧬 Code graph analysis (1)
src/bun.js/bindings/JSBunRequest.cpp (2)
src/js/builtins/ReadableStreamInternals.ts (1)
readableStreamTee(509-564)src/bun.js/bindings/CookieMap.cpp (2)
clone(205-211)clone(205-205)
⏰ 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
🔇 Additional comments (4)
src/bun.js/node/types.zig (1)
89-105: Correct owner-sensitive blob extractionPassing
.emptyfor owner when a Request body lacks a JS wrapper and deriving the Response owner fromthis_jsvaluealigns the blob conversion with the new Ref semantics, preventing weak-ref caching when the owner is unavailable. Looks solid.src/bun.js/webcore/BakeResponse.zig (1)
47-48: Constructor call updated correctlyForwarding
.zerointoResponse.constructorkeeps the SSR helper in sync with the new signature and avoids stale owner state. Nice catch.src/bun.js/api/server.zig (1)
1194-1203: Clone forwarding keeps owner contextThreading the original
RequestJS value and null cache throughcloneIntomatches the revised API and preserves body ownership during server fetch cloning. Looks good.src/codegen/generate-jssink.ts (1)
419-429: Null-safe memory accountingGuarding both
memoryCostandestimatedSizeagainst null wrapped sinks avoids dereferencing freed controllers without altering behavior when a sink exists. Good defensive tweak.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/bun.js/webcore/ReadableStream.zig (1)
92-111: Correct tee implementation with detailed debug logging.The logic properly handles the tee operation and updates the
Refto point to the first teed stream using the actualownerparameter (line 108), which aligns with the PR objectives. The multiple debug logs (lines 94, 99, 104, 107, 109) are helpful for diagnosing the hangs/crashes being fixed, though you may want to reduce their verbosity once the fix is stable.Consider reducing debug log verbosity once the fix is proven stable in production:
- bun.Output.debug("Ref.tee: get() returned null, this={s}, owner={s}", .{ @tagName(this.*), @tagName(owner) }); + // Stream not available for tee - bun.Output.debug("Ref.tee: stream.tee() returned null", .{}); + // Tee operation returned null - bun.Output.debug("Ref.tee: populated array with tee'd streams", .{}); - bun.Output.debug("Ref.tee: calling this.set(), this was {s}, owner is {s}", .{ @tagName(this.*), @tagName(owner) }); - bun.Output.debug("Ref.tee: after set(), this is now {s}", .{@tagName(this.*)});
📜 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 (1)
src/bun.js/webcore/ReadableStream.zig(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
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("...", .{})In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: In Zig source files, place all @import statements at the bottom of the file
Use @import("bun") instead of @import("root").bun when importing Bun in Zig
Files:
src/bun.js/webcore/ReadableStream.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
Files:
src/bun.js/webcore/ReadableStream.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/ReadableStream.zig
🔇 Additional comments (6)
src/bun.js/webcore/ReadableStream.zig (6)
6-25: Well-structured ownership model.The
Refunion with its nestedOwnerunion andTypeenum provides a clean API for managing ReadableStream ownership across different contexts (strong refs, Response/Request weak refs). The parallel structure makes the type system predictable.
26-51: Correct owner validation and error handling.The method properly validates that the
Refvariant matches theOwnertype before accessing the GC cache (lines 30, 39). This defensive check prevents accessing the wrong cache or attempting invalid casts. The cascading optional checks withorelse nullprovide consistent error handling.
58-90: Fixes the core weak-ref issue.This correctly implements the PR's main fix: when
jsvalue == .zero(no valid owner), the code sets theRefto.emptyrather than attempting to use the weak-ref GC cache (lines 65-68, 75-78). Thedeinit()call before each reassignment ensures proper cleanup. This addresses the "attempt to use null value" panics described in the PR objectives.
127-142: Correct initialization logic per PR objectives.The method correctly returns
.emptyfor.strongand.emptyowners (lines 136-140), as described in the PR objectives: "return.emptyfor.strong/.emptyowners (no stream value yet); caller populates via.set()." The explanatory comment on line 138 clarifies the intended usage pattern.
144-181: Proper resource cleanup and consistent method patterns.The
deinit()method correctly handles cleanup for the.strongvariant (lines 146-147) and safely no-ops for other variants. Theabort(),toAnyBlob(), anddone()methods follow a consistent pattern: retrieve the stream viaget(), perform the operation, and calldeinit()to clean up. This ensures proper resource management.
1022-1023: LGTM: Type aliases for integration.Simple, clear type aliases that establish the integration points between ReadableStream and the Request/Response types from WebCore.
Addresses CodeRabbit review comments: 1. **JSBunRequest.cpp:143** - Fix WriteBarrier owner for original request body - Changed from `this->m_body.set(vm, clone, ...)` to `this->m_body.set(vm, this, ...)` - The WriteBarrier must record `this` as the mutated cell, not `clone` - Prevents GC from missing the strong edge and prematurely reclaiming the body 2. **Request.zig:647** - Fix tee slot assignment for Response clone - Changed from `tee_value[1]` to `tee_value[0]` - When teeing a Response body into a Request, slot[0] is for the original Response - Prevents Response from getting the clone's stream and losing data 3. **Response.zig:577,635** - Initialize this_jsvalue in constructor - Added `response.this_jsvalue = .initWeak(thisValue)` when thisValue != .zero - Response constructor now properly records its JS wrapper - Enables owner-aware code paths to recover the JS wrapper via tryGet() These fixes ensure proper memory management and prevent stream corruption during clone operations. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Fixes hang in async-iterator-stream tests for Response bodies with async generators. ## Root Cause When `new Response(async generator)` is created: 1. Response constructor stores stream in GC cache (weak ref) 2. Body's `readable.Ref` is set to `.Response` (relies on GC cache) 3. RequestContext receives Response and protects the JSValue 4. BUT: Between Response creation and RequestContext using it, GC could collect the stream 5. When RequestContext calls `.get()`, it returns `null` → hangs waiting for stream ## Fix **RequestContext.zig:1833-1836** - Upgrade Ref to `.strong` before using - Call `.get()` to retrieve the stream - Call `.upgrade()` to convert Ref to `.strong` (prevents GC) - Subsequent `.get()` calls succeed with strong reference **ReadableStream.zig:124** - Fix upgrade() bug (pre-existing) - Changed `current.value` to `current.*` - `Strong.init()` expects full ReadableStream struct, not just JSValue ## Results async-iterator-stream tests: - ✅ 45 tests pass (was 0) -⚠️ 41 Request body tests still timeout (different issue) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/bun.js/webcore/ReadableStream.zig (1)
94-110: Switchbun.Output.debugcalls to scoped logger.The new debug statements should go through a scoped logger (
const log = bun.Output.scoped(...)) instead of the globalbun.Output.debughelper so we keep logging consistent with our Zig guidelines. Please introduce a scoped logger (e.g. nearRef) and replace the directbun.Output.debuginvocations.+const log = bun.Output.scoped(.readable_stream_ref, false); ... - bun.Output.debug("Ref.tee: get() returned null, this={s}, owner={s}", .{ @tagName(this.*), @tagName(owner) }); + log("Ref.tee: get() returned null, this={s}, owner={s}", .{ @tagName(this.*), @tagName(owner) }); ... - bun.Output.debug("Ref.tee: stream.tee() returned null", .{}); + log("Ref.tee: stream.tee() returned null", .{}); ... - bun.Output.debug("Ref.tee: populated array with tee'd streams", .{}); + log("Ref.tee: populated array with tee'd streams", .{}); ... - bun.Output.debug("Ref.tee: calling this.set(), this was {s}, owner is {s}", .{ @tagName(this.*), @tagName(owner) }); + log("Ref.tee: calling this.set(), this was {s}, owner is {s}", .{ @tagName(this.*), @tagName(owner) }); ... - bun.Output.debug("Ref.tee: after set(), this is now {s}", .{@tagName(this.*)}); + log("Ref.tee: after set(), this is now {s}", .{@tagName(this.*)});As per coding guidelines
📜 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/api/server/RequestContext.zig(14 hunks)src/bun.js/webcore/ReadableStream.zig(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
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("...", .{})In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: In Zig source files, place all @import statements at the bottom of the file
Use @import("bun") instead of @import("root").bun when importing Bun in Zig
Files:
src/bun.js/webcore/ReadableStream.zigsrc/bun.js/api/server/RequestContext.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
Files:
src/bun.js/webcore/ReadableStream.zigsrc/bun.js/api/server/RequestContext.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/ReadableStream.zigsrc/bun.js/api/server/RequestContext.zig
src/{**/js_*.zig,bun.js/api/**/*.zig}
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Files:
src/bun.js/api/server/RequestContext.zig
⏰ 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
🔇 Additional comments (2)
src/bun.js/api/server/RequestContext.zig (2)
1455-1456: LGTM: Consistent owner validation pattern.The owner computation pattern is correctly and consistently implemented across all four call sites:
const owner = if (response.this_jsvalue.tryGet()) |jsval| jsc.WebCore.ReadableStream.Ref.Owner{ .Response = jsval } else jsc.WebCore.ReadableStream.Ref.Owner{ .empty = {} }; response.body.value.toBlobIfPossible(owner);This aligns with the PR's fix for handling cases where no valid JSValue owner exists, preventing the "attempt to use null value" panics mentioned in the PR description.
Also applies to: 1561-1562, 1621-1622, 2179-2180
1833-1846: Retain the double-get pattern and clarify its necessity.The first
get+upgraderoots the JSValue (preventing GC) and drops the initial wrapper; the secondgetthen creates a fresh, safeReadableStreamwrapper for use. Add an inline comment above this block explaining why both calls are required.Likely an incorrect or invalid review comment.
Fixes remaining 41 test timeouts in async-iterator-stream tests for Request bodies. ## Root Cause Request doesn't track `this_jsvalue` (see line 1160 TODO comment in Body.zig). This causes: 1. Request constructor stores stream in GC cache with thisValue 2. Body's `readable.Ref` is set to `.Request` 3. When `getText()`/`json()`/etc is called, owner is always `.empty` (no JSValue) 4. `.get(.empty)` returns null → timeout waiting for stream ## Fix **Request.zig:800-803** - Upgrade Request body Ref to `.strong` in constructor - After setting GC cache, retrieve the stream - Call `.upgrade()` to convert Ref from `.Request` (unusable) to `.strong` - Now `getText()/etc` can retrieve stream via strong ref ## Results async-iterator-stream tests: - ✅ 85/86 tests pass (was 45/86) - 1 test has unrelated chunk length expectation issue All Request body method tests pass: - Request.text() ✅ - Request.json() ✅ - Request.blob() ✅ - Request.arrayBuffer() ✅ - Request.bytes() ✅ 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
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 (1)
src/bun.js/webcore/Request.zig(14 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
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("...", .{})In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: In Zig source files, place all @import statements at the bottom of the file
Use @import("bun") instead of @import("root").bun when importing Bun in Zig
Files:
src/bun.js/webcore/Request.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
Files:
src/bun.js/webcore/Request.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/Request.zig
🧠 Learnings (1)
📚 Learning: 2025-09-19T19:55:22.427Z
Learnt from: markovejnovic
PR: oven-sh/bun#22568
File: src/valkey/js_valkey_functions.zig:853-854
Timestamp: 2025-09-19T19:55:22.427Z
Learning: In Bun's JSPromise API, `resolvedPromiseValue(globalThis: *JSGlobalObject, value: JSValue) JSValue` returns a JSValue directly (not a JSPromise that needs `.toJS()` called on it). This is different from `resolvedPromise()` which returns a *JSPromise that would need `.toJS()`.
Applied to files:
src/bun.js/webcore/Request.zig
⏰ 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
🔇 Additional comments (5)
src/bun.js/webcore/Request.zig (5)
71-72: LGTM: Signature correctly extended for stream ownership tracking.The signature extension to include
this_valueandteeparameters properly supports the ReadableStream ownership model described in the PR objectives.
783-807: LGTM: Constructor correctly implements strong ref upgrade for stream ownership.The post-construction logic (lines 795-804) properly addresses the core issue from this PR. Since Request doesn't store
thisValueas a field, subsequent operations likegetText()would attempt to retrieve the stream without a valid owner, causing panics. The upgrade to strong ref ensures the stream remains accessible.
203-203: LGTM: Correct owner parameter for size calculations.Using
.emptyowner forsize()calls in formatting contexts is appropriate, as these calls don't perform stream operations and just need the size value.Also applies to: 242-242
250-250: LGTM: Strong ref access appropriate for formatting.Using
.strongto retrieve the readable stream inwriteFormatensures the stream can be accessed for display purposes, even when there's no JS owner context. This aligns with the PR's approach of using strong refs when a valid owner isn't available.
645-648: LGTM: Tee slot assignment correctly addressed from previous review.The code correctly assigns
tee_value[0](the original branch) back to the Response's GC cache, as noted in the previous review comment that was marked as addressed. This ensures the Response retains the correct stream slot whiletee_value[1]goes to the new Request.
- Add field to Body.Value.Locked.PendingValue as fallback strong ref when no owner JSValue exists (e.g., Request) - Add helper to check both GC cache and fallback strong ref - Update to use instead of direct - Update to use and manual stream.tee() instead of Ref.tee() to support fallback strong ref - Remove unnecessary GC cache setting in Request constructor - Properly cleanup fallback strong ref in deinit() This architecture keeps Request/Response holding weak refs (via GC cache or empty) while consumers (RequestContext, .text() operations) hold strong refs. Request bodies work without needing this_jsvalue field by using the fallback strong ref storage. Result: 85/86 async-iterator-stream tests passing (1 flaky test)
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 (3)
src/bun.js/api/server/RequestContext.zig(14 hunks)src/bun.js/webcore/Body.zig(42 hunks)src/bun.js/webcore/Request.zig(14 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
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("...", .{})In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: In Zig source files, place all @import statements at the bottom of the file
Use @import("bun") instead of @import("root").bun when importing Bun in Zig
Files:
src/bun.js/webcore/Request.zigsrc/bun.js/api/server/RequestContext.zigsrc/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
Files:
src/bun.js/webcore/Request.zigsrc/bun.js/api/server/RequestContext.zigsrc/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/Request.zigsrc/bun.js/api/server/RequestContext.zigsrc/bun.js/webcore/Body.zig
src/{**/js_*.zig,bun.js/api/**/*.zig}
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Files:
src/bun.js/api/server/RequestContext.zig
🧠 Learnings (1)
📚 Learning: 2025-09-19T19:55:22.427Z
Learnt from: markovejnovic
PR: oven-sh/bun#22568
File: src/valkey/js_valkey_functions.zig:853-854
Timestamp: 2025-09-19T19:55:22.427Z
Learning: In Bun's JSPromise API, `resolvedPromiseValue(globalThis: *JSGlobalObject, value: JSValue) JSValue` returns a JSValue directly (not a JSPromise that needs `.toJS()` called on it). This is different from `resolvedPromise()` which returns a *JSPromise that would need `.toJS()`.
Applied to files:
src/bun.js/webcore/Request.zig
Instead of fallback strong ref architecture, give Request the same this_jsvalue field that Response has. This allows Request to use the standard GC cache mechanism for storing/retrieving body streams. - Add `this_jsvalue: jsc.JSRef` field to Request struct - Initialize this_jsvalue in Request constructor - Use setValue() to properly set GC cache and update Ref type - Update BodyMixin to use this_jsvalue for both Request and Response - Remove fallback strong ref mechanism (getStream, stream field, etc) This is the correct architecture: containers (Request/Response) hold WEAK refs via GC cache, consumers hold STRONG refs. Result: 85/86 async-iterator-stream tests passing (1 flaky test passes in isolation, fails when run with full suite - likely pre-existing)
All 86 async-iterator-stream tests now passing!
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 (4)
src/bun.js/api/server/RequestContext.zig(14 hunks)src/bun.js/webcore/Body.zig(42 hunks)src/bun.js/webcore/ReadableStream.zig(2 hunks)src/bun.js/webcore/Request.zig(15 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
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("...", .{})In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: In Zig source files, place all @import statements at the bottom of the file
Use @import("bun") instead of @import("root").bun when importing Bun in Zig
Files:
src/bun.js/api/server/RequestContext.zigsrc/bun.js/webcore/ReadableStream.zigsrc/bun.js/webcore/Request.zigsrc/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
Files:
src/bun.js/api/server/RequestContext.zigsrc/bun.js/webcore/ReadableStream.zigsrc/bun.js/webcore/Request.zigsrc/bun.js/webcore/Body.zig
src/{**/js_*.zig,bun.js/api/**/*.zig}
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Files:
src/bun.js/api/server/RequestContext.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/api/server/RequestContext.zigsrc/bun.js/webcore/ReadableStream.zigsrc/bun.js/webcore/Request.zigsrc/bun.js/webcore/Body.zig
🧠 Learnings (1)
📚 Learning: 2025-09-19T19:55:22.427Z
Learnt from: markovejnovic
PR: oven-sh/bun#22568
File: src/valkey/js_valkey_functions.zig:853-854
Timestamp: 2025-09-19T19:55:22.427Z
Learning: In Bun's JSPromise API, `resolvedPromiseValue(globalThis: *JSGlobalObject, value: JSValue) JSValue` returns a JSValue directly (not a JSPromise that needs `.toJS()` called on it). This is different from `resolvedPromise()` which returns a *JSPromise that would need `.toJS()`.
Applied to files:
src/bun.js/webcore/Request.zig
Properly cleanup the JSRef when Request is finalized to prevent memory leaks.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/bun.js/webcore/Request.zig (1)
177-181: Store the wrapper JSValue in this_jsvalue.This issue was flagged in a previous review but remains unaddressed. The
toJSmethod creates a JS wrapper but never stores it inthis.this_jsvalue. Callers likeRequest.clone()and body getters depend on that weak ref when synthesizing the owner forReadableStream.Ref. Without storing it, the cloned request'sBody.Valueloses the owner context, and latergetBody()calls see an.emptyowner, failing to retrieve the tee'd branch from the GC cache.Apply this diff to persist the wrapper:
pub fn toJS(this: *Request, globalObject: *JSGlobalObject) JSValue { this.calculateEstimatedByteSize(); const value = js.toJSUnchecked(globalObject, this); + if (value != .zero) { + this.this_jsvalue.finalize(); + this.this_jsvalue = .initWeak(value); + } return value; }
🧹 Nitpick comments (1)
src/bun.js/webcore/Request.zig (1)
204-204: Minor: Use consistent syntax for empty owner parameter.Line 204 uses
.{ .empty = {} }while Line 243 uses.empty. Both are valid, but using consistent syntax improves readability.Consider standardizing on one form, e.g.:
-try writer.print("{s} ({}) {{\n", .{ class_label, bun.fmt.size(this.body.value.size(.{ .empty = {} }), .{}) }); +try writer.print("{s} ({}) {{\n", .{ class_label, bun.fmt.size(this.body.value.size(.empty), .{}) });Also applies to: 243-243
📜 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 (1)
src/bun.js/webcore/Request.zig(16 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
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("...", .{})In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: In Zig source files, place all @import statements at the bottom of the file
Use @import("bun") instead of @import("root").bun when importing Bun in Zig
Files:
src/bun.js/webcore/Request.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
Files:
src/bun.js/webcore/Request.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/Request.zig
🧠 Learnings (1)
📚 Learning: 2025-09-19T19:55:22.427Z
Learnt from: markovejnovic
PR: oven-sh/bun#22568
File: src/valkey/js_valkey_functions.zig:853-854
Timestamp: 2025-09-19T19:55:22.427Z
Learning: In Bun's JSPromise API, `resolvedPromiseValue(globalThis: *JSGlobalObject, value: JSValue) JSValue` returns a JSValue directly (not a JSPromise that needs `.toJS()` called on it). This is different from `resolvedPromise()` which returns a *JSPromise that would need `.toJS()`.
Applied to files:
src/bun.js/webcore/Request.zig
🔇 Additional comments (9)
src/bun.js/webcore/Request.zig (9)
18-18: LGTM!The
this_jsvaluefield addition enables weak reference tracking of the JS wrapper for proper GC cache support.
72-73: LGTM!The signature correctly accepts
this_valueandteeparameters to support tee'd ReadableStream propagation through cloning.
251-251: LGTM!Using
.strongref type here ensures the ReadableStream remains accessible for formatting/inspection purposes, even when there's no valid owner context. This aligns with the PR's fix for handling streams without valid owners.
357-357: LGTM!Properly finalizes the
this_jsvalueJSRef during Request cleanup.
528-528: LGTM!The signature update enables tee'd ReadableStream propagation during Request construction.
588-588: LGTM!The tee handling correctly:
- Threads the tee parameter through cloning (line 588)
- Uses the valid JSValue as owner when cloning from Request (line 616)
- Stores the original branch (
tee[0]) back to the Response GC cache when cloning from Response (lines 647-650), as correctly fixed per past review- Passes the clone branch (
tee[1]) when constructing the new Request body (line 664)Also applies to: 616-620, 647-654, 664-665
785-807: LGTM!The constructor correctly implements the fix described in the PR objectives:
- Checks owner validity before initializing the weak ref (line 798)
- Only populates the GC cache when the owner is valid (lines 801-803)
- Uses
setValueto properly store the tee'd stream with the Request ownerThis prevents the "attempt to use null value" panics that occurred when owner was
.zeroor.empty.
815-835: LGTM!The
doClonemethod correctly stores both tee'd stream branches in their respective GC caches:
tee[0]for the original Request (line 826)tee[1]for the cloned Request (line 830)The concern about upgrading to a strong ref was addressed in a previous commit.
875-875: LGTM!The remaining changes correctly propagate
this_valueandreadable_stream_teethrough the clone methods:
- Line 875 updates syntax for consistency with other owner parameter usage
cloneIntoandclonemethods thread the parameters through to body cloning operations- All parameter passing is consistent with the tee propagation pattern
Also applies to: 942-980
- Store the wrapper JSValue in this_jsvalue during toJS() so cloned requests and body getters can retrieve the owner from GC cache - Finalize old this_jsvalue before storing new one to prevent leaks - Standardize empty owner syntax to use .empty consistently This fixes the issue where Request.clone() would lose owner context because toJS() creates the wrapper but never stores it for later retrieval.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/bun.js/webcore/Request.zig (1)
255-255: Use .empty owner for read-only stream access in writeFormat
At Request.zig:255, switch the.getcall to use.emptyinstead of.strongfor display‐only formatting—this avoids an unnecessary strong reference and aligns with fetch.zig’s usage.
📜 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 (1)
src/bun.js/webcore/Request.zig(16 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
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("...", .{})In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: In Zig source files, place all @import statements at the bottom of the file
Use @import("bun") instead of @import("root").bun when importing Bun in Zig
Files:
src/bun.js/webcore/Request.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
Files:
src/bun.js/webcore/Request.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/Request.zig
🧠 Learnings (1)
📚 Learning: 2025-09-19T19:55:22.427Z
Learnt from: markovejnovic
PR: oven-sh/bun#22568
File: src/valkey/js_valkey_functions.zig:853-854
Timestamp: 2025-09-19T19:55:22.427Z
Learning: In Bun's JSPromise API, `resolvedPromiseValue(globalThis: *JSGlobalObject, value: JSValue) JSValue` returns a JSValue directly (not a JSPromise that needs `.toJS()` called on it). This is different from `resolvedPromise()` which returns a *JSPromise that would need `.toJS()`.
Applied to files:
src/bun.js/webcore/Request.zig
⏰ 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
🔇 Additional comments (17)
src/bun.js/webcore/Request.zig (17)
18-18: LGTM: this_jsvalue field addition.The
this_jsvaluefield enables proper GC cache tracking for Request instances, which is essential for the ReadableStream.Ref system fixes described in the PR objectives.
72-73: LGTM: Export function signature updated correctly.The updated signature properly threads
this_valueandteeparameters to support the new tee stream propagation model.
177-185: LGTM: Wrapper JSValue now properly persisted.This correctly implements the fix from the previous review. The wrapper is now stored in
this_jsvalueso that callers likeRequest.clone()and body getter can rely on the weak ref for GC-dependent owner lookups.
208-208: LGTM: Owner parameter added for size query.Using
.emptyis appropriate here since this is a display/formatting operation that doesn't require a JS wrapper reference.
247-247: LGTM: Consistent owner parameter usage.Correctly uses
.emptyfor the size query in this display context.
361-361: LGTM: Proper cleanup of JS reference.Correctly finalizes the weak reference before other cleanup, following the pattern established in the codebase.
532-532: LGTM: Signature updated for tee stream propagation.The added
readable_stream_teeparameter enables proper handling of teed streams during Request construction.
592-592: LGTM: Owner and tee parameters properly threaded.Correctly passes
value(the source Request wrapper) andreadable_stream_teetocloneIntofor proper stream handling.
620-620: LGTM: Body clone with correct owner.Properly specifies the source Request wrapper as owner and passes the tee parameter for stream splitting.
668-668: LGTM: Tee stream captured for new Request body.Correctly passes
&tee[1]to capture the cloned branch of any teed ReadableStream.
825-836: Verify: Is upgrade() needed after GC cache set?The past review comment on this section (marked as addressed in commit c470ef9) mentioned that
doCloneshould upgrade the cloned stream to a strong ref after setting the GC cache, similar to the constructor pattern. The current code sets both tee branches in the GC cache but doesn't callupgrade().Please verify whether:
- The
js.gc.body.set()calls now handle ownership internally without needing explicitupgrade(), or- The explicit
upgrade()pattern is still needed for the cloned Request after line 834.Based on past review at lines 825-835.
879-879: LGTM: Empty owner for read-only stream access.Correctly uses
.{ .empty = {} }for inspecting the stream's content type without requiring a JS wrapper reference.
952-953: LGTM: Signature updated for owner and tee propagation.The added parameters enable proper owner tracking and tee stream handling during Request cloning.
958-958: LGTM: Body clone with correct owner in cloneInto.Properly specifies the source Request wrapper as owner and threads the tee parameter.
979-982: LGTM: Clone method signature completed.The updated signature and parameter forwarding complete the tee stream propagation pattern for Request cloning.
651-654: Verify clone owner for Response body during tee
Ensure the cloned body uses the Response wrapper (value) as its owner instead of.emptywhen tee-ing and updatingtee_value[0], to maintain correct ownership semantics.
789-810: No explicit upgrade required:ReadableStream.setValue(owner: .Request, …)populates the GC cache and updates theRefvariant; an explicitupgrade()isn’t needed here, matching theResponse.zigconstructor pattern.
Use proper Request owner from this_jsvalue when getting readable stream to extract content_type, instead of hardcoded .empty owner. This fixes the FormData Content-Type header becoming null after accessing request.body. The issue was that after my changes, Locked bodies have Ref type .Request, but ensureFetchHeaders was trying to get the stream with .empty owner, causing a mismatch and null return. FormData tests now: 108 pass, 1 fail (timeout - pre-existing on main)
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 (1)
src/bun.js/webcore/Request.zig(16 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
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("...", .{})In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: In Zig source files, place all @import statements at the bottom of the file
Use @import("bun") instead of @import("root").bun when importing Bun in Zig
Files:
src/bun.js/webcore/Request.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
Files:
src/bun.js/webcore/Request.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/Request.zig
🧠 Learnings (1)
📚 Learning: 2025-09-19T19:55:22.427Z
Learnt from: markovejnovic
PR: oven-sh/bun#22568
File: src/valkey/js_valkey_functions.zig:853-854
Timestamp: 2025-09-19T19:55:22.427Z
Learning: In Bun's JSPromise API, `resolvedPromiseValue(globalThis: *JSGlobalObject, value: JSValue) JSValue` returns a JSValue directly (not a JSPromise that needs `.toJS()` called on it). This is different from `resolvedPromise()` which returns a *JSPromise that would need `.toJS()`.
Applied to files:
src/bun.js/webcore/Request.zig
…treams Addresses CodeRabbit review comment: doClone must call setValue on both the original and cloned Request's Locked.readable Ref after setting the GC cache, mirroring the constructor pattern. This ensures the Ref is properly initialized with the tee'd stream value, making the body stream accessible via the owner-aware get() path.
|
✅ All CodeRabbit review comments have been addressed:
The |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/bun.js/webcore/Request.zig (1)
532-532: Consider renaming the dual-purpose parameter for clarity.The
readable_stream_teeparameter serves two distinct purposes:
- Returning tee'd stream branches
[0]and[1]when cloning a Request/Response- Returning the extracted stream JSValue in slot
[1]when parsing a plain body objectWhile the current implementation works correctly, the parameter name suggests it's only for tee operations, which can be misleading. Consider renaming it to something more generic like
readable_stream_outor adding a comment explaining the dual usage.Example documentation addition:
+// readable_stream_tee: output parameter for ReadableStream JSValues +// - When cloning: [0] = original branch, [1] = clone branch +// - When extracting: [1] = extracted stream value pub fn constructInto(globalThis: *jsc.JSGlobalObject, arguments: []const jsc.JSValue, readable_stream_tee: ?*[2]jsc.JSValue) bun.JSError!Request {Also applies to: 668-668, 796-796
📜 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 (1)
src/bun.js/webcore/Request.zig(16 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
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("...", .{})In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: In Zig source files, place all @import statements at the bottom of the file
Use @import("bun") instead of @import("root").bun when importing Bun in Zig
Files:
src/bun.js/webcore/Request.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
Files:
src/bun.js/webcore/Request.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/Request.zig
🧠 Learnings (1)
📚 Learning: 2025-09-19T19:55:22.427Z
Learnt from: markovejnovic
PR: oven-sh/bun#22568
File: src/valkey/js_valkey_functions.zig:853-854
Timestamp: 2025-09-19T19:55:22.427Z
Learning: In Bun's JSPromise API, `resolvedPromiseValue(globalThis: *JSGlobalObject, value: JSValue) JSValue` returns a JSValue directly (not a JSPromise that needs `.toJS()` called on it). This is different from `resolvedPromise()` which returns a *JSPromise that would need `.toJS()`.
Applied to files:
src/bun.js/webcore/Request.zig
⏰ 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
🔇 Additional comments (5)
src/bun.js/webcore/Request.zig (5)
789-811: LGTM: Constructor properly initializes owner tracking and tee'd streams.The implementation correctly:
- Accepts
thisValueto track the JS wrapper- Initializes the tee array for stream splitting
- Stores
thisValueas a weak reference inthis_jsvaluefor later owner lookups- Associates the tee'd stream branch with the new Request via
setValueThe conditional check
if (thisValue != .zero)appropriately handles edge cases where the wrapper might not be immediately available.
819-841: LGTM: doClone correctly assigns tee branches and updates both Requests.The implementation properly:
- Creates a tee array to capture the split stream branches
- Assigns
tee[0]to the original Request (this) via GC cache andsetValue- Assigns
tee[1]to the cloned Request via GC cache andsetValueEach Request now maintains the correct stream branch with its owner properly tracked.
651-654: LGTM: Response tee branch assignment is correct.When constructing a Request from a Response, the code properly:
- Clones the Response body (tee'ing if it's a stream)
- Assigns
tee_value[0]to the Response's GC cache (keeping the original branch)- Implicitly reserves
tee_value[1]for the new Request (assigned later in constructor)This correctly implements the tee semantics where the original Response retains branch [0] and the new Request gets branch [1].
177-185: LGTM: toJS properly maintains wrapper tracking.The implementation correctly:
- Finalizes any existing weak reference to prevent leaks
- Stores the new JS wrapper as a weak reference in
this_jsvalue- Returns the wrapper value
This ensures subsequent operations can retrieve the owner context via
this_jsvalue.tryGet().
879-890: LGTM: Owner retrieval with appropriate fallback.The code correctly:
- Attempts to retrieve the owner from
this_jsvalue.tryGet()- Falls back to
.emptyif the wrapper is not available- Uses the owner to safely access the stream content_type
The
.emptyfallback is appropriate for cases where the Request hasn't been fully initialized yet or the wrapper was never set.
… tee'd streams" This reverts commit 100b7fc.
Update on CodeRabbit comment about
|
When a request ends with a pending read on the request body stream, the stream must be aborted to reject the pending read promise. This mirrors the existing response body abort logic but uses the request's this_jsvalue to get the proper owner for the Ref system.
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 (1)
src/bun.js/api/server/RequestContext.zig(14 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
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("...", .{})In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: In Zig source files, place all @import statements at the bottom of the file
Use @import("bun") instead of @import("root").bun when importing Bun in Zig
Files:
src/bun.js/api/server/RequestContext.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
Files:
src/bun.js/api/server/RequestContext.zig
src/{**/js_*.zig,bun.js/api/**/*.zig}
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Files:
src/bun.js/api/server/RequestContext.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/api/server/RequestContext.zig
⏰ 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
Address CodeRabbit review comments to fix memory safety issues: 1. Move request body abort before deref() in RequestContext.finalize() - The abort logic was unreachable because it ran after deref() cleared the weak ref - Now aborts locked request body streams before releasing the weak reference 2. Preserve tee'd stream for owner-less clones in Body.tee() - When readable_stream_tee is supplied but owner is .empty, keep a strong ref - Prevents GC from reclaiming the tee branch before Request/Response gains owner - Fixes issue where cloned bodies appeared empty or disturbed These changes ensure proper stream lifecycle management and prevent premature garbage collection of tee'd ReadableStream branches. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/bun.js/webcore/Body.zig (1)
1053-1066: Critical: Tee'd streams lost for owner-less clones.When
readable_stream_teeis non-null and the owner is.emptyor.strong(lines 1061-1062), both tee branches are lost:
- Original branch: Line 1047 calls
.set()which is a no-op for.empty/.strongowners, andlocked.readableremains.empty(from not being set in lines 502-513), so the first branch is not retained- Clone branch: Line 1061 returns
.empty, soresult.@"1"is not retainedThis causes both branches to be GC'd, leaving the clone without a tracked stream and potentially crashing when the body is read.
Apply this diff to retain both branches via strong refs:
const tee_result = (try locked.readable.tee(owner, globalThis, readable_stream_tee)) orelse return Value{ .Used = {} }; if (readable_stream_tee != null) { + // For owner-less or strong-ref scenarios, must retain the clone branch + const clone_readable: jsc.WebCore.ReadableStream.Ref = switch (owner) { + .Response => .Response, + .Request => .Request, + else => .{ .strong = .init(tee_result.@"1", globalThis) }, + }; + return .{ .Locked = .{ - .readable = switch (owner) { - .Response => .Response, - .Request => .Request, - else => .empty, - }, + .readable = clone_readable, .global = globalThis, }, }; }
📜 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/api/server/RequestContext.zig(15 hunks)src/bun.js/webcore/Body.zig(42 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
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("...", .{})In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: In Zig source files, place all @import statements at the bottom of the file
Use @import("bun") instead of @import("root").bun when importing Bun in Zig
Files:
src/bun.js/api/server/RequestContext.zigsrc/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
Files:
src/bun.js/api/server/RequestContext.zigsrc/bun.js/webcore/Body.zig
src/{**/js_*.zig,bun.js/api/**/*.zig}
📄 CodeRabbit inference engine (.cursor/rules/registering-bun-modules.mdc)
Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Files:
src/bun.js/api/server/RequestContext.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/api/server/RequestContext.zigsrc/bun.js/webcore/Body.zig
⏰ 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
🔇 Additional comments (9)
src/bun.js/api/server/RequestContext.zig (3)
630-631: Verify abort logic executes correctly.The captured
response_jsvalue(line 630) and the new request body abort block (lines 653-663) execute beforederef()is called on line 665, addressing the prior concern about the abort hook never firing. Confirm that:
- The request body abort (lines 656-662) uses
request.this_jsvalue.tryGet()to derive the owner, falling back to.emptywhen no JSValue exists- The response body abort (line 696) uses the captured
response_jsvalueBoth streams are now properly aborted while their owning Request/Response objects are still reachable.
Also applies to: 653-663
1467-1469: LGTM: Consistent owner derivation pattern.The owner derivation logic correctly checks
response.this_jsvalue.tryGet()and falls back to.emptywhen no valid JSValue exists. This pattern is applied consistently across HEAD response rendering, fulfilled promise handling, and error paths, ensuring proper ReadableStream lifecycle management.Also applies to: 1574-1575, 1634-1635, 2186-2187
1230-1230: LGTM: Proper ReadableStream.Ref lifecycle management.The ReadableStream.Ref initialization and cleanup integrates correctly with the owner-aware model:
- Strong refs created when storing streams in response bodies (line 1230, 2517)
- Owner-aware cleanup using
tryGet()to derive owner before callingdone()or falling back todeinit()(lines 1679-1683, 1733-1737)- Proper ref swapping in the scoped block (lines 1847-1852) with old ref cleanup, followed by error-path cleanup (line 1860)
- Byte stream ref management for piping (lines 1904-1907)
The scoped block at lines 1847-1852 correctly deinitializes the old ref before checking for lock status, and the error path at line 1860 properly cleans up the new ref if the stream is already locked.
Also applies to: 1679-1683, 1733-1737, 1846-1852, 1860-1860, 1904-1907, 2517-2517
src/bun.js/webcore/Body.zig (6)
58-58: LGTM: Core type change enables per-owner refs.The change from
ReadableStream.Strong = .{}toReadableStream.Ref = .emptyis the foundational modification that enables the per-owner ReadableStream reference model. The.emptyinitialization indicates no stream is currently tracked, consistent with the new Ref semantics.
443-456: LGTM: Correct owner-aware stream initialization for blob conversion.The blob-to-ReadableStream conversion properly handles all owner scenarios:
- Request/Response with valid JSValue (lines 444-445): Uses type-based ref (
.Request/.Response) and populates GC cache via.set()(lines 453-454)- Request/Response with
.zeroJSValue: Falls back to.strongref to keep stream alive until a real owner exists.strong/.emptyowners (line 446): Uses strong ref; no GC cache population since there's no owner to key onThis prevents premature GC of the stream while ensuring proper lifecycle management once an owner is available.
627-633: LGTM: Deferred stream setup pattern.The new
fromJSWithReadableStreamValuefunction supports deferred ReadableStream setup by returning a.Lockedvalue with.emptyreadable whenreadable_stream_valueis provided (lines 627-633). The caller receives the stream JSValue via the pointer parameter and is responsible for populating the readable ref later (either via GC cache for Request/Response owners or by upgrading to a strong ref).This pattern enables Request/Response constructors to defer stream initialization until after the wrapper JSValue is available.
1132-1140: LGTM: Type-safe owner derivation helper.The
getOwnerinline helper provides compile-time type-safe owner derivation for the Mixin methods. The@compileErrorfor unsupported types ensures the Mixin is only used with Request or Response types, preventing runtime errors from incorrect owner types.
1142-1378: LGTM: Consistent owner-aware Mixin methods.All Mixin methods correctly derive the owner and propagate it through Body.Value operations:
- JS-facing methods (getText, getJSON, getArrayBuffer, getBytes, getFormData, getBlob) use
getOwner(callframe.this())to derive owner from the current call frame- Internal methods (getBody, getBodyUsed, getBlobWithThisValue, getBlobWithoutCallFrame) use
this.this_jsvalue.tryGet()with.zerocheck and.emptyfallback for cases where the JSValue isn't available yetThe pattern ensures proper ReadableStream lifecycle management while handling both JavaScript-invoked and internally-invoked code paths.
1436-1436: LGTM: Appropriate use of.emptyowner in ValueBufferer.ValueBufferer correctly uses
.emptyowner throughout (lines 1436, 1629, 1677, 1696) since it's an internal utility not associated with a specific Request/Response. The stream lifecycle transfer at lines 1635-1637 properly moves the stream to ValueBufferer's strong ref while marking the original value as Used, preventing double-ownership.Also applies to: 1629-1629, 1635-1637, 1677-1677, 1696-1696
When converting a drain result to a ReadableStream, the code was calling .set() to populate the GC cache but never updating locked.readable to the matching owner variant (.Request or .Response). This left it as .empty, breaking subsequent .get(owner, globalThis) calls. Now matches the blob conversion pattern by: 1. Setting locked.readable to the correct owner variant (or strong ref fallback) 2. Only calling .set() when owner has a valid JSValue This ensures the ref type matches the GC cache key for proper stream retrieval. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/bun.js/webcore/fetch.zig (2)
187-204: Make the strong-move explicit and drop redundant checkLogic is correct (owner-aware fallback and guard against Used/Disturbed). Two small cleanups:
- Make the “take strong and clear local” move explicit to avoid aliasing confusion.
- The second
body_value == .Lockedcheck is redundant within the enclosing branch.Suggested diff:
- if (body_value.Locked.readable == .strong) { - // just grab the ref - - defer body_value.Locked.readable = .empty; - return FetchTasklet.HTTPRequestBody{ .ReadableStream = body_value.Locked.readable.strong }; - } + if (body_value.Locked.readable == .strong) { + // move out the strong ref + var prev = body_value.Locked.readable; + body_value.Locked.readable = .{ .empty = {} }; + return FetchTasklet.HTTPRequestBody{ .ReadableStream = prev.strong }; + } @@ - if (!readable.isEmptyOrUndefinedOrNull() and body_value == .Locked and body_value.Locked.readable.has(.empty, globalThis)) { + if (!readable.isEmptyOrUndefinedOrNull() and body_value.Locked.readable.has(.empty, globalThis)) {
2113-2124: Request.body owner-threading LGTM; consider small DRY refactor
- Correctly threads
{ .Request = first_arg }intoisDisturbed,has,get, andtoReadableStream.- Switching
fromJSto useglobalThisis consistent with other call sites.Minor: assign the owner once to avoid repetition.
- if (req.body.value == .Used or (req.body.value == .Locked and (req.body.value.Locked.action != .none or req.body.value.Locked.isDisturbed(.{ .Request = first_arg }, globalThis)))) { + const owner = .{ .Request = first_arg }; + if (req.body.value == .Used or (req.body.value == .Locked and (req.body.value.Locked.action != .none or req.body.value.Locked.isDisturbed(owner, globalThis)))) { @@ - if (req.body.value.Locked.readable.has(.{ .Request = first_arg }, globalThis)) { - break :extract_body FetchTasklet.HTTPRequestBody{ .ReadableStream = jsc.WebCore.ReadableStream.Strong.init(req.body.value.Locked.readable.get(.{ .Request = first_arg }, globalThis).?, globalThis) }; + if (req.body.value.Locked.readable.has(owner, globalThis)) { + break :extract_body FetchTasklet.HTTPRequestBody{ .ReadableStream = jsc.WebCore.ReadableStream.Strong.init(req.body.value.Locked.readable.get(owner, globalThis).?, globalThis) }; } - const readable = try req.body.value.toReadableStream(.{ .Request = first_arg }, globalThis); - if (!readable.isEmptyOrUndefinedOrNull() and req.body.value == .Locked and req.body.value.Locked.readable.has(.{ .Request = first_arg }, globalThis)) { - break :extract_body FetchTasklet.HTTPRequestBody{ .ReadableStream = jsc.WebCore.ReadableStream.Strong.init(req.body.value.Locked.readable.get(.{ .Request = first_arg }, globalThis).?, globalThis) }; + const readable = try req.body.value.toReadableStream(owner, globalThis); + if (!readable.isEmptyOrUndefinedOrNull() and req.body.value == .Locked and req.body.value.Locked.readable.has(owner, globalThis)) { + break :extract_body FetchTasklet.HTTPRequestBody{ .ReadableStream = jsc.WebCore.ReadableStream.Strong.init(req.body.value.Locked.readable.get(owner, globalThis).?, globalThis) }; }Also applies to: 2131-2134
📜 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 (1)
src/bun.js/webcore/fetch.zig(5 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
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("...", .{})In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: In Zig source files, place all @import statements at the bottom of the file
Use @import("bun") instead of @import("root").bun when importing Bun in Zig
Files:
src/bun.js/webcore/fetch.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
Files:
src/bun.js/webcore/fetch.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/fetch.zig
⏰ 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
🔇 Additional comments (1)
src/bun.js/webcore/fetch.zig (1)
456-475: Owner-aware Response readable retrieval looks correctUsing
response.this_jsvalue.tryGet()as owner to fetch the readable, and falling back to.emptyis aligned with the new Ref model. ClearingLocked.readableto explicit empty and deinit’ing the previous ref avoids leaks/dangling refs. LGTM.
|
Fixed in #23313 |
3 participants
Summary
Fixes critical issues with the new
ReadableStream.Refsystem for managing stream references in Request/Response bodies. This resolves hangs and crashes in body-stream tests caused by incorrect strong vs weak reference handling.Problem
When
Request.getBody()orResponse.getBody()is called without a valid JSValue owner (e.g., when the Request/Response wrapper JSValue is.zero), the code was attempting to store streams in the GC cache, which failed silently. This led to:.set(.empty, ...)is a no-op).get(owner).?calls panicking with "attempt to use null value"Root Cause
The
ReadableStream.Refsystem uses different storage strategies:.Request/.Response): Store stream JSValue in GC cache, retrieved via owner JSValue.strong): Direct strong reference to stream JSValueThe code was always trying to use weak refs, even when no owner JSValue was available.
Changes
1. Body.zig - toReadableStream() blob conversion (lines 443-456)
.strongref when owner JSValue is.zeroor owner is.empty2. Body.zig - toReadableStream() drain conversion (lines 502-519)
.strongref when no valid owner JSValue3. Body.zig - tee() operation (line 1020)
.strongowner to actualownerparameter4. ReadableStream.zig - Ref.init() (lines 127-142)
.emptyfor.strong/.emptyowners (no stream value yet).set()to populate with stream5. test/js/web/fetch/body-stream.test.ts
Test Results
✅ All individual tests and filtered test subsets pass:
Request.prototype.texttests: 20 tests in 11s ✅Request.prototype.bytestests: 10 tests in 9.5s ✅Request.prototype.arrayBuffertests: 8 tests in 8.5s ✅Impact
🤖 Generated with Claude Code