http: clear the request stream drain callback under the buffer mutex

[robobun]

What does this PR do?

Fixes a data race on ThreadSafeStreamBuffer.callback (the streaming request body's drain callback) between the HTTP thread and the JS thread.

Cause. The HTTP thread reads callback in report_drain() while holding the buffer mutex (src/http/lib.rs write_to_stream, and the h2/h3 drain_send_body). The JS thread cleared it in clear_drain_callback() without the mutex; the comments in FetchTasklet claimed the field was main-thread-only. FetchTasklet::start_request_stream calls clear_sink() when assign_to_stream fails, and at that point the request is still in flight: the HTTP thread still owns its ref on the buffer and may be flushing the bytes that were just written, so the unlocked write of None raced the locked read. Option<Callback> is two words (fn pointer + context), so a stale or torn read calls on_write_request_data_drain(tasklet) during teardown. Today the tasklet is still referenced on that path (the HTTP side releases its ref only after the abort completes), so the worst case is a spurious resume task, but nothing in the code enforces that. The other caller, deinit -> clear_data -> clear_sink, is already ordered after the HTTP thread's last access by the tasklet refcount release.

A user-reachable way onto that path is a direct stream request body whose pull() writes and then throws synchronously:

await fetch(url, {
  method: "POST",
  body: new ReadableStream({
    type: "direct",
    pull(controller) {
      for (let i = 0; i < 4; i++) controller.write(chunk); // wakes the HTTP thread
      throw error; // assign_to_stream fails -> write_end_request -> clear_sink
    },
  }),
});

Fix. clear_drain_callback takes the buffer mutex, so it is serialised against report_drain; once it returns the HTTP thread can no longer call back into the tasklet through the buffer. The field is now private so every access goes through the locked accessors, and report_drain debug-asserts that the caller holds the lock (it already had to, per its doc comment). The stale comments in FetchTasklet are corrected.

How did you verify your code works?

New test in test/js/web/fetch/fetch-abort-stream-body.test.ts drives the snippet above 50 times against a server that does not answer until the upload is torn down, and checks that every fetch rejects with pull()'s own error, that pull() ran every time (it only runs once the HTTP thread has asked for the body, so the teardown really happens in flight), and, with the fix, that clearing the callback does not deadlock against the HTTP thread holding the buffer. There is no thread sanitizer build, so this test is regression coverage of the path rather than a detector for the race itself; it also passes on the unfixed build.

To confirm the window is actually hit, I temporarily logged both accesses in a debug build and ran the same scenario 500 times:

Observed interleavings (temporary instrumentation, not part of the PR)

Per request, in logging order (drain = HTTP thread report_drain, clear = JS thread clear_drain_callback):

 298  drain > drain > drain > clear
 161  drain > drain > clear
  28  drain > drain > drain > drain > clear
   7  drain > clear
   3  clear > drain            <- HTTP thread read the field after the JS thread cleared it
   3  drain > clear > drain    <- same

500/500 requests had the HTTP thread reading the callback in the same window as the JS-side clear; in 6 the read landed after the clear (with the fix it observes None; before it was an unsynchronised read of a field being written).

Also ran under the debug build: fetch-abort-stream-body.test.ts, fetch-http2-client.test.ts, fetch-http3-client.test.ts (their streaming request body tests exercise the new report_drain assertion on h1, h2 and h3), body.test.ts, fetch-cyclic-reference.test.ts, fetch-keepalive.test.ts. fetch-backpressure.test.ts's four h3 receive backpressure cases time out locally when the whole file runs, identically on unmodified main (they send no request body, so they do not touch this code); they pass in isolation.

A larger follow-up could make the mutex own buffer and callback outright (Guarded<_>), which would also remove the &mut ThreadSafeStreamBuffer both threads currently hold at once; this PR only closes the race.

---

no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/web/fetch/fetch-abort-stream-body.test.ts

[coderabbitai]

Walkthrough

Changes

The stream buffer now synchronizes drain callback clearing and reporting with its mutex. Fetch tasklet comments document this lifecycle. A regression test covers concurrent request-body stream failures.

Fetch drain callback synchronization

Layer / File(s)	Summary
Synchronize drain callback lifecycle src/http/ThreadSafeStreamBuffer.rs, src/runtime/webcore/fetch/FetchTasklet.rs	clear_drain_callback locks the buffer before clearing. report_drain requires the lock while accessing and invoking the callback. Fetch tasklet comments document in-flight teardown behavior.
Validate concurrent upload errors test/js/web/fetch/fetch-abort-stream-body.test.ts	A repeated request-body stream test writes data, throws from pull(), and verifies that fetch() preserves the thrown error and executes each pull once.

Possibly related issues

• [CRASH] Worker termination races in-flight fetch(), corrupting the event loop's concurrent task queue (two crash signatures) #33911 — Both changes address synchronization during in-flight fetch() callback teardown, but cover different race scenarios.

Possibly related PRs

• oven-sh/bun#36087 — Directly overlaps in request-body stream handling and FetchTasklet/ThreadSafeStreamBuffer synchronization.
• oven-sh/bun#36899 — Shares ThreadSafeStreamBuffer drain-callback synchronization code.
• oven-sh/bun#36946 — Shares regression coverage for streaming request-body failure rejection.

Suggested reviewers: jarred-sumner

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly and concisely identifies the primary fix: clearing the request stream drain callback under the buffer mutex.
Description check	✅ Passed	The description includes both required sections and clearly explains the cause, fix, verification steps, test coverage, and known limitations.

---

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

[robobun]

Status: ready for review.

Reproduced how: a direct stream request body whose pull() writes and then throws puts FetchTasklet::start_request_stream on the assign_to_stream failure path while the upload is in flight; with both accesses temporarily logged in a debug build, 500/500 requests had the HTTP thread's report_drain read of callback land in the same window as the JS thread's unlocked clear, and in 6 of them the read came after the clear. Numbers and the interleavings are in the PR description.

Fix: clear_drain_callback now takes the buffer mutex (src/http/ThreadSafeStreamBuffer.rs); the field is private and report_drain debug-asserts the lock is held. Test: test/js/web/fetch/fetch-abort-stream-body.test.ts.

[robobun]

^{Updated 11:22 PM PT - Aug 10th, 2026}

✅ @robobun, your commit b0c14895bafec4de37da6ab7947c02d9716d91f9 passed in Build #91993! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 37435

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

bun-37435 --bun

[claude]

I reviewed this PR and didn't find any bugs. The fix is small and looks correct — the mutex guard in clear_drain_callback properly serialises against report_drain, and none of the JS-thread callers hold the buffer lock so there's no self-deadlock. That said, this is a cross-thread synchronisation change in the fetch request-body path and the added test is (as noted) path coverage rather than a race detector, so a human look would still be worthwhile.

What was reviewed:

• Mutex::lock_guard and is_held_by_current_thread exist and match the usage here.
• Both clear_sink callers (assign-failure at FetchTasklet.rs:701 and the deinit path) run on the JS thread without the buffer mutex held — no recursive lock.
• on_write_request_data_drain only enqueues a task and doesn't re-enter the mutex, so calling it under the lock (unchanged behaviour) is safe.
• Making callback private breaks no other accessors — the only reads/writes are inside ThreadSafeStreamBuffer.rs.

Extended reasoning...

Overview

The PR closes a data race on ThreadSafeStreamBuffer.callback by taking the buffer mutex in clear_drain_callback(), making the field private, and adding a debug_assert!(mutex.is_held_by_current_thread()) to report_drain(). Two SAFETY comments in FetchTasklet.rs are updated to drop the now-false "main-thread-only" claim, and a new test in fetch-abort-stream-body.test.ts exercises the assign-failure teardown path 50 times.

Security risks

None. This is internal synchronisation of a two-word Option<Callback> field; no user-controlled input reaches the changed code beyond what already flowed through it.

Level of scrutiny

High. Per the repo review guide, native memory safety and thread affinity are the most-blocked category. The functional change is tiny (one lock guard, one debug assert, one visibility tightening) and strictly adds synchronisation rather than removing it, which limits the blast radius. But it sits on a path where both the HTTP thread and the JS thread hold &mut ThreadSafeStreamBuffer simultaneously — the PR description itself flags that a follow-up should move to Guarded<_> to remove that aliasing — and the new test admittedly passes on the unfixed build (no TSan), so a maintainer confirming the lock ordering and the h2/h3 report_drain call sites is warranted.

Other factors

I verified that bun_threading::Mutex provides both lock_guard() and is_held_by_current_thread() (the latter is a no-op returning true in release, so the debug_assert! is sound). All three report_drain callers (h1 write_to_stream, h2 and h3 drain_send_body) call it between acquire() and release(), so the new assertion holds. The drain callback (on_write_request_data_drain) only refs the tasklet and posts a concurrent task — it does not touch the buffer mutex — so invoking it while the HTTP thread holds the lock cannot deadlock against the new clear_drain_callback lock. The comment-cop bot flags on earlier commits were addressed in b0c1489 and are all resolved. The test follows harness conventions (port: 0, Buffer.alloc, test.concurrent, await using, exact-identity .rejects.toBe).