fetch investigation #23433

cirospaciari · 2025-10-10T04:00:19Z

What does this PR do?

Investigate memory usage in fetch

How did you verify your code works?

Test

robobun · 2025-10-10T04:00:28Z

^{Updated 11:11 PM PT - Oct 14th, 2025}

❌ @Jarred-Sumner, your commit 35551f8 has 3 failures in Build #29233 (All Failures):

test/js/web/fetch/fetch-leak.test.ts - 1 failing on 🐧 25.04 x64
test/js/web/fetch/fetch-leak.test.ts - 1 failing on 🐧 24.04 x64-baseline
test/js/web/fetch/fetch-leak.test.ts - 1 failing on 🐧 12 x64
test/js/web/fetch/fetch-leak.test.ts - 1 failing on 🐧 12 x64-baseline
test/js/web/fetch/fetch-leak.test.ts - 1 failing on 🐧 3.21 x64-baseline
test/js/web/fetch/fetch-leak.test.ts - 1 failing on 🍎 13 aarch64
test/regression/issue/11297/11297.test.ts - 1 failing on 🪟 2019 x64-baseline
CMakeLists.txt - cmake error on 🪟 x64 - build-bun
CMakeLists.txt - cmake error on 🪟 x64 - build-bun

🧪 To try this PR locally:

bunx bun-pr 23433

That installs a local version of the PR into your bun-23433 executable, so you can run:

bun-23433 --bun

coderabbitai · 2025-10-15T02:35:16Z

Walkthrough

Removed a non-mimalloc fast-path in ArrayBuffer.toJS, changed fetch streaming final-chunk handling to use the owned scheduled buffer path, added a streaming fetch memory-leak fixture and a test that runs it, and changed HTTP thread allocator initialization.

Changes

Cohort / File(s)	Summary
ArrayBuffer toJS path unification `src/bun.js/jsc/array_buffer.zig`	Removed the early-return non-mimalloc branch in ArrayBuffer.toJS; all conversions now delegate to the standard toJSUnchecked path (removes non-owning/no-copy branch and its log).
Fetch streaming final-chunk ownership `src/bun.js/webcore/fetch.zig`	In onBodyReceived the final-chunk handling now uses `owned_and_done = bun.ByteList.moveFromList(scheduled_response_buffer)` and no longer assigns a temporary non-owned slice for the final chunk; a commented note about potential memory concerns was added. Non-final chunk temporary path unchanged.
Fetch leak tests (fixture + harness integration) `test/js/web/fetch/fetch-leak-test-fixture-6.js`, `test/js/web/fetch/fetch-leak.test.ts`	Added a fixture that performs 500 streamed fetches and checks RSS growth after GC; test updated to spawn the fixture via `bunRun` with environment vars and assert expected stdout/stderr.
HTTP thread allocator source change `src/http/HTTPThread.zig`	onStart now assigns `bun.http.default_allocator` from `bun.default_allocator` instead of using the allocator from the newly-created `bun.http.default_arena`.

Possibly related PRs

replace jsDoubleNumber with jsNumber to use NumberTag if possible #22476: Changes to ArrayBuffer-to-JS conversion sites that interact with the removed non-owning/no-copy toJS path.
ci: instrument being able to run leaksanitizer #21142: Related edits touching ArrayBuffer-to-JS conversion and buffer ownership/deallocator handling.

Suggested reviewers

taylordotfish

Pre-merge checks

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Description Check	⚠️ Warning	The description includes the required headings but the content is too brief and does not summarize the actual code changes or the detailed verification steps, making both sections incomplete and unhelpful for reviewers.	Please expand the “What does this PR do?” section to outline the specific code changes such as removal of the non-copy buffer path in ArrayBuffer.toJS, updates to fetch body handling, and the new leak-test fixtures, and enhance the “How did you verify your code works?” section with details about running the memory-leak tests, expected RSS measurements, and assertion outcomes.
Title Check	❓ Inconclusive	The title “fetch investigation” is vague and does not convey the specific changes introduced in this PR, such as modifications to ArrayBuffer.toJS behavior, fetch body handling adjustments, and the addition of memory-leak tests.	Please update the title to clearly and concisely reflect the primary change in this PR, for example summarizing the memory-leak fixes in fetch streams or the unified ArrayBuffer deallocation behavior that addresses the investigation outcomes.

📜 Recent review details

Configuration used: CodeRabbit UI

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 4f6551b and 35551f8.

📒 Files selected for processing (2)

src/bun.js/webcore/fetch.zig (1 hunks)
src/http/HTTPThread.zig (1 hunks)

🧰 Additional context used

📓 Path-based instructions (3)

**/*.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/http/HTTPThread.zig
src/bun.js/webcore/fetch.zig

src/**/*.zig

📄 CodeRabbit inference engine (CLAUDE.md)

In Zig code, manage memory carefully and use defer for cleanup of allocations/resources

When adding debug logs in Zig, create a scoped logger and log via Bun APIs: const log = bun.Output.scoped(.${SCOPE}, .hidden); then log("...", .{})

src/**/*.zig: Use private fields in Zig with the # prefix (e.g., struct { #foo: u32 };)
Prefer decl literals in Zig (e.g., const decl: Decl = .{ .binding = 0, .value = 0 };)
Prefer placing @import statements at the bottom of the Zig file (formatter may reorder automatically)
Prefer @import("bun") rather than @import("root").bun or @import("../bun.zig")

Files:

src/http/HTTPThread.zig
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

🧠 Learnings (2)

📚 Learning: 2025-09-03T05:09:24.272Z

Learnt from: Jarred-Sumner
PR: oven-sh/bun#22335
File: src/http.zig:1376-1393
Timestamp: 2025-09-03T05:09:24.272Z
Learning: In bun's HTTP module, `bun.http.default_allocator` is the correct allocator to use for internal HTTP buffers like MutableString initialization for response_message_buffer and compressed_body. This allocator is initialized in HTTPThread.zig as `bun.http.default_arena.allocator()` and is used consistently across the HTTP module, separate from any local `default_allocator` variables.

Applied to files:

src/http/HTTPThread.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/bindings/generated_classes_list.zig : Update src/bun.js/bindings/generated_classes_list.zig to include new classes

Applied to 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/http/HTTPThread.zig (1)
198-198: Remove unused HTTP arena and audit allocator usage

Delete bun.http.default_arena = Arena.init(); in HTTPThread.zig (line 197).

Audit all MutableString allocations using bun.http.default_allocator to ensure they deallocate correctly.

Evaluate performance and memory impact of switching from arena-based to direct allocation.
⛔ Skipped due to learnings
Learnt from: Jarred-Sumner
PR: oven-sh/bun#22335
File: src/http.zig:1376-1393
Timestamp: 2025-09-03T05:09:24.272Z
Learning: In bun's HTTP module, `bun.http.default_allocator` is the correct allocator to use for internal HTTP buffers like MutableString initialization for response_message_buffer and compressed_body. This allocator is initialized in HTTPThread.zig as `bun.http.default_arena.allocator()` and is used consistently across the HTTP module, separate from any local `default_allocator` variables.

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

coderabbitai

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

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 37ad295 and 17088e2.

📒 Files selected for processing (4)

src/bun.js/jsc/array_buffer.zig (0 hunks)
src/bun.js/webcore/fetch.zig (1 hunks)
test/js/web/fetch/fetch-leak-test-fixture-6.js (1 hunks)
test/js/web/fetch/fetch-leak.test.ts (2 hunks)

💤 Files with no reviewable changes (1)

src/bun.js/jsc/array_buffer.zig

🧰 Additional context used

📓 Path-based instructions (11)

test/**

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

Place all tests under the test/ directory

Files:

test/js/web/fetch/fetch-leak.test.ts
test/js/web/fetch/fetch-leak-test-fixture-6.js

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/fetch-leak.test.ts
test/js/web/fetch/fetch-leak-test-fixture-6.js

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/fetch-leak.test.ts
test/js/web/fetch/fetch-leak-test-fixture-6.js

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/fetch-leak.test.ts
test/js/web/fetch/fetch-leak-test-fixture-6.js

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/fetch-leak.test.ts

test/js/web/**

📄 CodeRabbit inference engine (CLAUDE.md)

Place Web API tests under test/js/web/

Files:

test/js/web/fetch/fetch-leak.test.ts
test/js/web/fetch/fetch-leak-test-fixture-6.js

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 for files ending with *.test.{ts,js,jsx,tsx,mjs,cjs}
Prefer concurrent tests (test.concurrent/describe.concurrent) over sequential when feasible
Organize tests with describe blocks to group related tests
Use utilities like describe.each, toMatchSnapshot, and lifecycle hooks (beforeAll, beforeEach, afterEach) and track resources for cleanup

Files:

test/js/web/fetch/fetch-leak.test.ts

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

📄 CodeRabbit inference engine (test/CLAUDE.md)

For large/repetitive strings, use Buffer.alloc(count, fill).toString() instead of "A".repeat(count)

Files:

test/js/web/fetch/fetch-leak.test.ts
test/js/web/fetch/fetch-leak-test-fixture-6.js

**/*.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

src/**/*.zig

📄 CodeRabbit inference engine (CLAUDE.md)

In Zig code, manage memory carefully and use defer for cleanup of allocations/resources

When adding debug logs in Zig, create a scoped logger and log via Bun APIs: const log = bun.Output.scoped(.${SCOPE}, .hidden); then log("...", .{})

src/**/*.zig: Use private fields in Zig with the # prefix (e.g., struct { #foo: u32 };)
Prefer decl literals in Zig (e.g., const decl: Decl = .{ .binding = 0, .value = 0 };)
Prefer placing @import statements at the bottom of the Zig file (formatter may reorder automatically)
Prefer @import("bun") rather than @import("root").bun or @import("../bun.zig")

Files:

src/bun.js/webcore/fetch.zig

🧠 Learnings (12)

📚 Learning: 2025-10-12T02:22:34.373Z

Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js} : When spawning Bun in tests, use `bunExe()` and `bunEnv` from `harness`

Applied to files:

test/js/web/fetch/fetch-leak.test.ts

📚 Learning: 2025-10-12T02:22:34.373Z

Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js,test/**/*-fixture.ts} : Use `using`/`await using` for resource cleanup with Bun APIs (e.g., `Bun.spawn`, `Bun.listen`, `Bun.serve`)

Applied to files:

test/js/web/fetch/fetch-leak.test.ts
test/js/web/fetch/fetch-leak-test-fixture-6.js

📚 Learning: 2025-08-30T00:12:56.803Z

Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/cli/**/*.{js,ts} : When testing Bun as a CLI, use spawn with bunExe() and bunEnv from harness, and capture stdout/stderr via pipes

Applied to files:

test/js/web/fetch/fetch-leak.test.ts

📚 Learning: 2025-10-04T09:51:30.294Z

Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to test/**/*.test.{ts,tsx} : Use Bun’s Jest-compatible runner (import { test, expect } from "bun:test") for tests

Applied to files:

test/js/web/fetch/fetch-leak.test.ts

📚 Learning: 2025-10-04T09:51:30.294Z

Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to test/**/*.test.{ts,tsx} : Use Bun.spawn with proper stdio handling and await proc.exited in process-spawning tests

Applied to files:

test/js/web/fetch/fetch-leak.test.ts

📚 Learning: 2025-10-04T09:51:30.294Z

Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to test/**/*.test.{ts,tsx} : Prefer running tests via bun bd test <file> and use provided harness utilities (bunEnv, bunExe, tempDir)

Applied to files:

test/js/web/fetch/fetch-leak.test.ts

📚 Learning: 2025-10-12T02:22:34.373Z

Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js} : Import common utilities from `harness` (e.g., `bunExe`, `bunEnv`, `tempDirWithFiles`, platform helpers, GC helpers)

Applied to files:

test/js/web/fetch/fetch-leak.test.ts

📚 Learning: 2025-08-30T00:12:56.803Z

Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/**/*.{js,ts} : Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test

Applied to files:

test/js/web/fetch/fetch-leak.test.ts

📚 Learning: 2025-08-30T00:09:39.100Z

Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/bake-harness.ts : Provide and maintain shared test utilities: devTest, prodTest, devAndProductionTest, Dev, Client, and helpers in the harness

Applied to files:

test/js/web/fetch/fetch-leak.test.ts

📚 Learning: 2025-10-12T02:22:34.373Z

Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `bun:test` for files ending with `*.test.{ts,js,jsx,tsx,mjs,cjs}`

Applied to files:

test/js/web/fetch/fetch-leak.test.ts

📚 Learning: 2025-08-30T00:09:39.100Z

Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/{dev/*.test.ts,dev-and-prod.ts} : Import testing utilities (devTest, prodTest, devAndProductionTest, Dev, Client) from test/bake/bake-harness.ts

Applied to files:

test/js/web/fetch/fetch-leak.test.ts

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

Learnt from: Jarred-Sumner
PR: oven-sh/bun#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/fetch-leak-test-fixture-6.js

🧬 Code graph analysis (1)

test/js/web/fetch/fetch-leak.test.ts (1)

test/harness.ts (1)

bunRun (292-317)

src/bun.js/webcore/fetch.zig

coderabbitai

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

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 17088e2 and 4f6551b.

📒 Files selected for processing (1)

src/bun.js/webcore/fetch.zig (1 hunks)

🧰 Additional context used

📓 Path-based instructions (3)

**/*.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

src/**/*.zig

📄 CodeRabbit inference engine (CLAUDE.md)

In Zig code, manage memory carefully and use defer for cleanup of allocations/resources

When adding debug logs in Zig, create a scoped logger and log via Bun APIs: const log = bun.Output.scoped(.${SCOPE}, .hidden); then log("...", .{})

src/**/*.zig: Use private fields in Zig with the # prefix (e.g., struct { #foo: u32 };)
Prefer decl literals in Zig (e.g., const decl: Decl = .{ .binding = 0, .value = 0 };)
Prefer placing @import statements at the bottom of the Zig file (formatter may reorder automatically)
Prefer @import("bun") rather than @import("root").bun or @import("../bun.zig")

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

coderabbitai · 2025-10-15T02:51:59Z

src/bun.js/webcore/fetch.zig

+                            // TODO: We are still investigating the memory issues with owned_and_done
+                            // .owned_and_done = bun.ByteList.moveFromList(scheduled_response_buffer),
+                            .temporary_and_done = bun.ByteList.fromBorrowedSliceDangerous(chunk),


⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Temporary workaround wastes memory until accounting is fixed.

The TODO acknowledges ongoing investigation of owned_and_done memory issues. The current temporary_and_done approach keeps scheduled_response_buffer allocated until clearData() deinits it (line 297), even though the stream has already received (and likely copied) the data. This wastes memory for the duration of the fetch lifecycle.

The past review comment identified the root cause: when using owned_and_done, the memory accounting got confused—likely because ownership was transferred but the buffer was later deinit'ed, causing double-deduction in the memory reporter.

To properly re-enable owned_and_done:

Ensure memory_reporter.discard() is NOT called when ownership is transferred via moveFromList

Ensure scheduled_response_buffer is reset/cleared immediately after the move so it won't be deinit'ed later with already-freed memory

Verify no other code paths attempt to access the moved buffer

🏁 Script executed:

#!/bin/bash # Verify scheduled_response_buffer lifecycle and discard calls rg -nP -C5 'scheduled_response_buffer\.(deinit|reset)' src/bun.js/webcore/fetch.zig rg -nP -C5 'memory_reporter\.discard.*scheduled_response_buffer' src/bun.js/webcore/fetch.zig

Length of output: 2960

Do not re-enable owned_and_done yet — discard/deinit calls still exist and will double-account or touch moved buffers.

memory_reporter.discard(scheduled_response_buffer.allocatedSlice()) and scheduled_response_buffer deinit/reset are still called in multiple paths (e.g. src/bun.js/webcore/fetch.zig around ~297, ~369, ~476-486, ~882-889, ~930-940, ~1371-1376). Re-enabling bun.ByteList.moveFromList without changes will double-count or access/mutate a moved buffer.

Fix before enabling owned_and_done:

Stop calling memory_reporter.discard(...) on a buffer that has been moved (guard or remove those calls).

Immediately clear/reset scheduled_response_buffer (empty list / capacity = 0) right after moveFromList so later deinit/reset/discard become no-ops.

Audit/all other deinit/reset paths to ensure they respect moved ownership.

🤖 Prompt for AI Agents

In src/bun.js/webcore/fetch.zig around lines 433-435, do not re-enable .owned_and_done yet because scheduled_response_buffer is still being discard()/deinit/reset in multiple paths; to fix, when you eventually call bun.ByteList.moveFromList(scheduled_response_buffer) ensure (1) any memory_reporter.discard(scheduled_response_buffer.allocatedSlice()) calls are either guarded to check “moved” ownership or removed for that buffer, (2) immediately clear/reset scheduled_response_buffer to an empty state (zero length and zero/cleared capacity or mark as moved) right after the move so subsequent deinit/reset/discard become no-ops, and (3) audit and update all other deinit/reset/discard paths referenced around lines ~297, ~369, ~476-486, ~882-889, ~930-940, ~1371-1376 so they respect moved ownership semantics before enabling .owned_and_done.

This reverts commit 4f6551b.

This reverts commit 17088e2.

cirospaciari and others added 5 commits October 14, 2025 18:27

test

c4d4695

test

de84549

[autofix.ci] apply automated fixes

57d34d6

opsie

d4cdc4f

ok

17088e2

cirospaciari force-pushed the ciro/fetch-fix-maybe branch from 237e2e3 to 17088e2 Compare October 15, 2025 01:27

cirospaciari requested a review from Jarred-Sumner October 15, 2025 02:34

cirospaciari marked this pull request as ready for review October 15, 2025 02:34

coderabbitai bot reviewed Oct 15, 2025

View reviewed changes

src/bun.js/webcore/fetch.zig Show resolved Hide resolved

dont over report

4f6551b

coderabbitai bot reviewed Oct 15, 2025

View reviewed changes

Jarred-Sumner added 3 commits October 14, 2025 19:53

Update HTTPThread.zig

0d8bb27

Revert "dont over report"

b962a28

This reverts commit 4f6551b.

Revert "ok"

35551f8

This reverts commit 17088e2.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fetch investigation #23433

fetch investigation #23433

Uh oh!

cirospaciari commented Oct 10, 2025 •

edited

Loading

Uh oh!

robobun commented Oct 10, 2025 •

edited

Loading

Uh oh!

coderabbitai bot commented Oct 15, 2025 •

edited

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

coderabbitai bot left a comment

Uh oh!

coderabbitai bot Oct 15, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

fetch investigation #23433

Are you sure you want to change the base?

fetch investigation #23433

Uh oh!

Conversation

cirospaciari commented Oct 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

How did you verify your code works?

Uh oh!

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

Uh oh!

Uh oh!

coderabbitai bot commented Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Possibly related PRs

Suggested reviewers

Pre-merge checks

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Oct 15, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cirospaciari commented Oct 10, 2025 •

edited

Loading

robobun commented Oct 10, 2025 •

edited

Loading

coderabbitai bot commented Oct 15, 2025 •

edited

Loading