Skip to content

fetch investigation #23433

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
cirospaciari wants to merge 9 commits into from
Closed

fetch investigation #23433

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
c4d4695
test
cirospaciari Oct 10, 2025
de84549
test
cirospaciari Oct 10, 2025
57d34d6
[autofix.ci] apply automated fixes
autofix-ci[bot] Oct 10, 2025
d4cdc4f
opsie
cirospaciari Oct 10, 2025
17088e2
ok
cirospaciari Oct 15, 2025
4f6551b
dont over report
cirospaciari Oct 15, 2025
0d8bb27
Update HTTPThread.zig
Jarred-Sumner Oct 15, 2025
b962a28
Revert "dont over report"
Jarred-Sumner Oct 15, 2025
35551f8
Revert "ok"
Jarred-Sumner Oct 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 0 additions & 24 deletions src/bun.js/jsc/array_buffer.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,30 +250,6 @@ pub const ArrayBuffer = extern struct {
return this.value;
}

// If it's not a mimalloc heap buffer, we're not going to call a deallocator
if (this.len > 0 and !bun.mimalloc.mi_is_in_heap_region(this.ptr)) {
log("toJS but will never free: {d} bytes", .{this.len});

if (this.typed_array_type == .ArrayBuffer) {
return makeArrayBufferWithBytesNoCopy(
ctx,
this.ptr,
this.byte_len,
null,
null,
);
}

return makeTypedArrayWithBytesNoCopy(
ctx,
this.typed_array_type.toTypedArrayType(),
this.ptr,
this.byte_len,
null,
null,
);
}

return this.toJSUnchecked(ctx);
}

Expand Down
4 changes: 3 additions & 1 deletion src/bun.js/webcore/fetch.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -431,7 +431,9 @@ pub const FetchTasklet = struct {
this.memory_reporter.discard(scheduled_response_buffer.allocatedSlice());
readable.ptr.Bytes.onData(
.{
.owned_and_done = bun.ByteList.moveFromList(scheduled_response_buffer),
// 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),

@coderabbitai coderabbitai Bot Oct 15, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ 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:

  1. Ensure memory_reporter.discard() is NOT called when ownership is transferred via moveFromList
  2. Ensure scheduled_response_buffer is reset/cleared immediately after the move so it won't be deinit'ed later with already-freed memory
  3. 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.

},
Comment thread
cirospaciari marked this conversation as resolved.
bun.default_allocator,
);
Expand Down
24 changes: 24 additions & 0 deletions test/js/web/fetch/fetch-leak-test-fixture-6.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

20 changes: 19 additions & 1 deletion test/js/web/fetch/fetch-leak.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { describe, expect, test } from "bun:test";
import { bunEnv, bunExe, tls as COMMON_CERT, gc, isCI } from "harness";
import { bunEnv, bunExe, bunRun, tls as COMMON_CERT, gc, isCI } from "harness";
import { once } from "node:events";
import { createServer } from "node:http";
import { join } from "node:path";
Expand Down Expand Up @@ -184,3 +184,21 @@ test("do not leak", async () => {
}
}, 1e3);
});

test("should not leak using readable stream", async () => {
const buffer = Buffer.alloc(1024 * 128, "b");
using server = Bun.serve({
port: 0,
fetch: req => {
return new Response(buffer);
},
});

const { stdout, stderr } = bunRun(join(import.meta.dir, "fetch-leak-test-fixture-6.js"), {
...bunEnv,
SERVER_URL: server.url.href,
MAX_MEMORY_INCREASE: "5", // in MB
});
expect(stderr).toBe("");
expect(stdout).toContain("done");
});