ByteStream: take the pending buffer action before signal_drained() in on_data - #37165
Conversation
on_data called signal_drained() while buffer_action was still in its cell. signal_drained() dispatches to the producer, which can run JS or synchronously deliver more data, re-entering on_data/on_cancel; either consumes the action, so the buffer_action.replace(None).unwrap() that followed hit None and panicked (core::option::unwrap_failed), aborting the process. Seen in the wild when aborting an in-flight fetch whose streaming response body had a parked read. Move the action out of the cell before signal_drained() on the Err path, and re-take it with let-else instead of unwrap() after the non-Err signal_drained(), bailing out when a re-entrant consumer already settled it. This is the same rule the existing R-2 comment states for reject(). Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
Walkthrough
ChangesFetch abort re-entrancy
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/webcore/ByteStream.rs`:
- Around line 334-345: In the on_data flow around signal_drained, store or move
the current stream chunk into self.buffer before signaling the producer, while
leaving buffer_action installed during the signal. After signal_drained returns,
re-take buffer_action with the existing replace/let-else pattern and return when
re-entry consumed it, preserving chunk order for terminal and non-terminal
re-entry. Add a regression case using at least two distinct chunks and assert
the resulting buffered body order.
In `@test/js/web/fetch/fetch.stream.test.ts`:
- Around line 1432-1449: Remove the explicit 30_000 timeout argument from the
test.concurrent call for “aborting streaming fetches with parked body consumers
settles them without crashing”; retain the test body and runner-managed timeout
behavior unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cbb377f0-6666-409a-a763-549a099171d1
📒 Files selected for processing (3)
src/runtime/webcore/ByteStream.rstest/js/web/fetch/fetch-abort-parked-reads-fixture.tstest/js/web/fetch/fetch.stream.test.ts
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Updated 8:34 PM PT - Aug 7th, 2026
✅ @robobun, your commit 5d5f7fd95fa702dc0a82c050d0167586210aaaac passed in 🧪 To try this PR locally: bunx bun-pr 37165That installs a local version of the PR into your bun-37165 --bun |
|
Review sweep is done: removed the per-test timeout (the runner default is the bound), tightened the re-entrancy comments, and kept the on_data structure as is. The chunk-ordering suggestion was withdrawn after verification that no in-tree producer delivers synchronously from the drain signal, and the interleaving it described predates this change, which only replaces the panic with a clean return. Details are in the resolved threads. Current head is 859129f; tests pass locally with the debug build. |
|
I checked whether the new test reproduces the bug on an unfixed build: it doesn't. So The code change itself matches the analysis (take the action out of the cell before
For reference, the in-the-wild trigger was: streaming |
…-for-testing producer No in-tree producer re-enters ByteStream::on_data synchronously from the drain signal (fetch and server bodies schedule socket resumes, the rewriter defers behind its driving flag), so the stress fixture could not turn the panic red. Add SourceHandle::TestingCancelOnDrain, installed through bun:internal-for-testing, whose ready() re-enters on_cancel, and a fixture that parks body.text() on a streaming fetch response and then severs the connection: the tasklet delivers Err straight to on_data with the action still in its cell (unlike abort, which errors the JS stream first and consumes the action via done() -> cancel() before on_data runs). On the old ordering this panics every run (Option::unwrap() on None in ByteStream::on_data, via FetchTasklet::on_body_received); with the fix the text() promise rejects with the network error and the process exits cleanly.
|
Deterministic red/green is in now, via your option 2: a stub producer installed through Two findings from making it deterministic:
On the old ordering the fixture panics every run: With the fix it prints |
|
@robobun fix clippy |
… on_data (oven-sh#37165) ### Symptom Aborting or losing an in-flight `fetch` whose streaming response body has a parked consumer can abort the whole process: ``` Body::Value::to_error_instance (or FetchTasklet::on_body_received) -> ByteStream::on_data(Err) -> panic (core::option::unwrap_failed) ``` Observed in the wild when Claude Code shuts down its MCP streamable-HTTP transports: `abort()` on a fetch whose SSE response body has a parked consumer. ### Cause In `ByteStream::on_data`, the branch guarded by `self.buffer_action.get().is_some()` called `self.signal_drained()` before moving the action out of the cell. `signal_drained()` dispatches to the producer (`SourceHandle::ready`), and a producer that re-enters `on_data` or `on_cancel` from that signal consumes `buffer_action`, so the `self.buffer_action.replace(None).unwrap()` that followed (on both the `Err` path and the `has_received_last_chunk` path) could observe `None` and panic. The existing R-2 comment already states this rule for `reject()`; it was not applied to `signal_drained()`. ### Fix - `Err` path: move the action out of the cell first, then `signal_drained()`, then `reject`. - Non-`Err` path: keep `signal_drained()` before the fulfill (a backpressure-gated producer must observe the installed action), but re-take the action with `let`-`else` and return if a re-entrant consumer already settled it. The non-last-chunk tail only appends to the buffer and never takes the action, so it needs no guard. ### Verification The in-tree producers do not re-enter synchronously from the drain signal (fetch and server request bodies schedule socket resumes, the rewriter defers behind its `driving` flag), so the interleaving is not reachable from plain JS. The deterministic test therefore installs a stub producer through `bun:internal-for-testing` (`SourceHandle::TestingCancelOnDrain`, whose `ready()` re-enters `on_cancel`), parks `body.text()` on a streaming fetch response, and severs the connection. The severed connection matters: the tasklet then delivers `Err` straight to `on_data` with the action still in its cell, while `abort()` first errors the JS stream, whose `done()` path cancels the native source and consumes the action before `on_data` runs. On the unfixed ordering the fixture panics on every run: ``` panic: called `Option::unwrap()` on a `None` value core::option::unwrap_failed <bun_runtime::webcore::byte_stream::ByteStream>::on_data src/runtime/webcore/ByteStream.rs:317 <bun_runtime::webcore::fetch::fetch_tasklet::FetchTasklet>::on_body_received ``` With the fix it prints `rejected:TypeError` and exits 0. ``` bun bd test test/js/web/fetch/fetch.stream.test.ts -t "buffer action consumed re-entrantly" # deterministic red/green bun bd test test/js/web/fetch/fetch.stream.test.ts -t "parked body consumers" # abort-path stress coverage ``` <!-- robobun:evidence:begin --> --- **no test proof** · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/web/fetch/fetch.stream.test.ts <!-- robobun:evidence:end --> --------- Co-authored-by: Jarred Sumner <jarred@jarredsumner.com> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Takes main's ByteStream fix (#37165) over the branch's variant, and moves the process.argv passthrough-offset decision to the list the VM actually receives: deciding it in the argument parser broke `bun -e code a b` (positionals are merged back in later), which test/cli/run/run-eval.test.ts and the streaming fetch tests caught. The snapshot request's termination unwind/clear go through two HeapImage.cpp exports now that main removed the Rust wrappers.
2 participants
Symptom
Aborting or losing an in-flight
fetchwhose streaming response body has a parked consumer can abort the whole process:Observed in the wild when Claude Code shuts down its MCP streamable-HTTP transports:
abort()on a fetch whose SSE response body has a parked consumer.Cause
In
ByteStream::on_data, the branch guarded byself.buffer_action.get().is_some()calledself.signal_drained()before moving the action out of the cell.signal_drained()dispatches to the producer (SourceHandle::ready), and a producer that re-enterson_dataoron_cancelfrom that signal consumesbuffer_action, so theself.buffer_action.replace(None).unwrap()that followed (on both theErrpath and thehas_received_last_chunkpath) could observeNoneand panic. The existing R-2 comment already states this rule forreject(); it was not applied tosignal_drained().Fix
Errpath: move the action out of the cell first, thensignal_drained(), thenreject.Errpath: keepsignal_drained()before the fulfill (a backpressure-gated producer must observe the installed action), but re-take the action withlet-elseand return if a re-entrant consumer already settled it.The non-last-chunk tail only appends to the buffer and never takes the action, so it needs no guard.
Verification
The in-tree producers do not re-enter synchronously from the drain signal (fetch and server request bodies schedule socket resumes, the rewriter defers behind its
drivingflag), so the interleaving is not reachable from plain JS. The deterministic test therefore installs a stub producer throughbun:internal-for-testing(SourceHandle::TestingCancelOnDrain, whoseready()re-enterson_cancel), parksbody.text()on a streaming fetch response, and severs the connection. The severed connection matters: the tasklet then deliversErrstraight toon_datawith the action still in its cell, whileabort()first errors the JS stream, whosedone()path cancels the native source and consumes the action beforeon_dataruns.On the unfixed ordering the fixture panics on every run:
With the fix it prints
rejected:TypeErrorand exits 0.no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/web/fetch/fetch.stream.test.ts