Skip to content

test: cover fetch() recursion depth after PathBuffer scratch moved to the pool - #41493

Open
robobun wants to merge 2 commits into
mainfrom
robobun/26d741f2/fetch-stack-depth-test
Open

robobun wants to merge 2 commits into
mainfrom
robobun/26d741f2/fetch-stack-depth-test

Conversation

@robobun

@robobun robobun commented Sep 6, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

Fix

  • Test only. test/js/web/fetch/body-async-iterator.test.ts gains one test. It spawns bun with a request body whose Symbol.asyncIterator getter calls fetch() again, so each level adds one native fetch() frame. It asserts at least 32 levels before the RangeError.
  • Verified on Windows x64: bun 1.4.3 canary 76e9dcc (before Take path scratch buffers from the pool instead of uninitialized stack arrays #41442) reaches 16 and fails the test. A debug build of main at 0d0b28b reaches 97 and passes.
  • Verified on Linux x64: release 1.4.3 reaches 387, a debug build of main reaches 57. The whole file passes with bun bd test test/js/web/fetch/body-async-iterator.test.ts on both platforms.

Background

  • bun_paths::PathBuffer is a 96 KB byte array. fetch_impl used to keep up to four of them as locals, and NodeFS embedded one as a field. The Windows main thread has a small stack, so the frame size set the recursion limit.
  • bun_paths::path_buffer_pool::get() returns a Guard that borrows a heap buffer from a thread-local pool and returns it on drop. The frame then holds one pointer per buffer.

[auto-merge] gate passed · iteration 2 · 1 files touched

passes on PR (with fix)
Test-only change.

Debug/ASAN (expected pass):
$ bun bd test 'test/js/web/fetch/body-async-iterator.test.ts'
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test test/js/web/fetch/body-async-iterator.test.ts
bun test v1.4.3 (f42e98025)

test/js/web/fetch/body-async-iterator.test.ts:
(pass) Response.bytes() with async iterable body does not crash with null deref [321.50ms]
(pass) fetch() called from a body's Symbol.asyncIterator getter recurses deeply before the stack limit [346.76ms]
(pass) Response.arrayBuffer() with async iterable body does not crash with null deref [273.71ms]

 3 pass
 0 fail
 7 expect() calls
Ran 3 tests across 1 file. [3.53s]
Exit: 0
diff hotspot
test/js/web/fetch/body-async-iterator.test.ts | 36 +++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

gate history · 2 passed · 0 rejected · iteration 2

evidence per changed file
file                                           reads  edits  tests
test/js/web/fetch/body-async-iterator.test.ts      1      1      6

… the pool

This is the test from #40680 for the source change that landed in #41442.
It spawns bun with a body whose Symbol.asyncIterator getter calls fetch()
again and asserts at least 32 levels before the RangeError. Windows got 16
before #41442 and gets 97 on a debug build of main.
@robobun

robobun commented Sep 6, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status: test only, no source change. The source change landed in #41442. Verified on Windows x64: bun 1.4.3 canary 76e9dcc (before #41442) reaches 16 levels and fails this test, a debug build of main at 0d0b28b reaches 97 and passes. Linux passes both before and after.

CI: the new test passes on every lane in both builds (110767 and 110870). The red lanes in build 110870 are a bundler_compile.test.ts timeout on Windows 11 aarch64 and ws-proxy.test.ts on darwin x64. This diff touches neither, and both are reported separately. The other failures are flakes that passed on retry. Ready for review.

@coderabbitai

coderabbitai Bot commented Sep 6, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

  • Run on-demand review

On-demand reviews are free for the next 14 days. After that, they cost $0.25 per reviewed file.

Or wait 29 seconds for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Essentials

Run ID: 29928b47-28d6-4d0e-93ab-877d8bd03e6e

📥 Commits

Reviewing files that changed from the base of the PR and between 0d0b28b and cd5273d.

📒 Files selected for processing (1)
  • test/js/web/fetch/body-async-iterator.test.ts

Comment @coderabbitai help to get the list of available commands.

@claude claude Bot left a comment

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.

LGTM — test-only addition that backfills coverage for the PathBuffer-pool change already landed in #41442.

What was reviewed:

  • Harness conventions: bunExe()/bunEnv, -e for single-file, await using, concurrent Promise.all drain, output asserted before exit code — all match the file's existing pattern.
  • Hermeticity: AbortSignal.abort() is passed and the getter returns undefined, so every fetch() rejects before any DNS/connect; http://localhost/ is never contacted.
  • Threshold: >= 32 sits between the unfixed Windows value (16) and the lowest fixed value cited (Linux debug: 57), so it fails on the regressed build and clears every fixed platform.
Extended reasoning...

Overview

This PR appends one test to test/js/web/fetch/body-async-iterator.test.ts. The test spawns a Bun subprocess that recursively calls fetch() from inside a Symbol.asyncIterator getter on the request body, counts how many native fetch() frames fit on the stack before RangeError, and asserts at least 32. It backfills the test that was dropped when #41442 (moving PathBuffer locals in fetch_impl from the stack to the pool) superseded #40680. No production code changes.

Security risks

None. The subprocess passes an already-aborted AbortSignal and a body whose Symbol.asyncIterator getter returns undefined, so every fetch() call rejects locally without ever reaching DNS or a socket — the http://localhost/ URL is inert. No untrusted input is parsed, no filesystem writes, no credentials.

Level of scrutiny

Low. This is a small, self-contained test addition to an existing file, following the exact spawn/drain/assert pattern of the two sibling tests already in the file. It touches no runtime code, no CODEOWNERS-covered paths, and the bug-hunt exited on dry_streak with zero findings.

Other factors

The threshold choice is the only judgment call: the comment documents unfixed Windows at 16 and the lowest fixed platform (Linux debug) at 57, so >= 32 has comfortable margin on both sides. The comment is load-bearing — it names the mechanism (three 96 KB path buffers per frame) and the concrete before/after numbers a future maintainer would need to re-derive the bound. Assertions are ordered stderr → depth → exit code, so a failure surfaces the actual depth rather than just a nonzero exit.

@robobun

robobun commented Sep 6, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 2:31 AM PT - Sep 6th, 2026

❌ @robobun, your commit cd5273d has 2 failures in Build #110870 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 41493

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

bun-41493 --bun

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant