fetch: abort the request when the response body reader is cancelled - #33231
Conversation
Cancelling a fetch response body reader (reader.cancel() / body.cancel()) routed to ignore_remaining_response_body, which drained the rest of the body to keep the connection alive. For an unbounded or streaming response this held the socket open indefinitely, so the server never observed the cancellation and never fired request.signal's abort or the response stream's cancel. When the body is still arriving, abort the fetch so the connection is shut down and the server sees the close. A fully received body still falls through to the drain/cleanup path so the socket can be reused.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughFetch cancellation now aborts in-flight requests on reader cancellation, skips response draining after abort, and makes abort handling idempotent. Regression tests cover server-side abort/cancel events and later request recovery. ChangesFetch cancellation abort behavior
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 7:08 PM PT - Jul 3rd, 2026
@Jarred-Sumner, your commit 85ecd3f is building: |
#19211 was fixed then regressed, so this is a true regression; isolate it from the timing-sensitive streaming tests in fetch.stream.test.ts.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/runtime/webcore/fetch/FetchTasklet.rs (2)
1755-1804: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winSkip receive-resume once the fetch is aborted
drain_events()processes queued receive resumes before queued shutdowns, so this can still re-arm and drain an already-aborted socket. Guard it withsignal_store.abortedbefore scheduling, or move the check ahead of the resume enqueue.🤖 Prompt for 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. In `@src/runtime/webcore/fetch/FetchTasklet.rs` around lines 1755 - 1804, The ignore_remaining_response_body flow can still enqueue a receive resume after the fetch has already been aborted, because drain_events() may process that resume before shutdown. Update ignore_remaining_response_body to check signal_store.aborted before calling schedule_receive_resume(), or otherwise move the aborted guard ahead of the resume enqueue so the FetchTasklet never re-arms draining on an aborted socket.
1620-1632: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAvoid enqueuing duplicate shutdowns from
abort_task()
on_stream_cancelled_callback()andabort_listener()can both reachabort_task(), but the shutdown path isn’t deduped:schedule_shutdown_by_id()always appends aShutdownMessage, and a second call for the same fetch just turns into an extra no-op drain plushas_pending_queued_abortchurn. Gateabort_task()onsignal_store.abortedor make shutdown scheduling idempotent.🤖 Prompt for 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. In `@src/runtime/webcore/fetch/FetchTasklet.rs` around lines 1620 - 1632, `on_stream_cancelled_callback()` can trigger `abort_task()` more than once for the same fetch, which causes duplicate shutdown scheduling and unnecessary queued-abort churn. Update the shutdown path in `abort_task()` and/or `schedule_shutdown_by_id()` so it is idempotent by checking `signal_store.aborted` before scheduling, or by deduping repeated shutdown requests for the same task; keep the existing behavior in `on_stream_cancelled_callback()` and `abort_listener()` but ensure only the first abort enqueues a shutdown.
🤖 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 `@test/regression/issue/33227.test.ts`:
- Around line 17-24: The test in pull(controller) is still allowing too much
data to buffer before reader.cancel() runs, so it may not exercise the in-flight
abort path. Reduce the number of eagerly enqueued chunks in the response stream
setup and keep the body pending sooner by relying on the existing holdOpen
behavior earlier, so the regression test targets the abort path instead of
buffered cleanup.
---
Outside diff comments:
In `@src/runtime/webcore/fetch/FetchTasklet.rs`:
- Around line 1755-1804: The ignore_remaining_response_body flow can still
enqueue a receive resume after the fetch has already been aborted, because
drain_events() may process that resume before shutdown. Update
ignore_remaining_response_body to check signal_store.aborted before calling
schedule_receive_resume(), or otherwise move the aborted guard ahead of the
resume enqueue so the FetchTasklet never re-arms draining on an aborted socket.
- Around line 1620-1632: `on_stream_cancelled_callback()` can trigger
`abort_task()` more than once for the same fetch, which causes duplicate
shutdown scheduling and unnecessary queued-abort churn. Update the shutdown path
in `abort_task()` and/or `schedule_shutdown_by_id()` so it is idempotent by
checking `signal_store.aborted` before scheduling, or by deduping repeated
shutdown requests for the same task; keep the existing behavior in
`on_stream_cancelled_callback()` and `abort_listener()` but ensure only the
first abort enqueues a shutdown.
🪄 Autofix (Beta)
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: 1fc860ef-a980-4637-b0a2-a8abb8497d25
📒 Files selected for processing (2)
src/runtime/webcore/fetch/FetchTasklet.rstest/regression/issue/33227.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Address review findings on the reader-cancel abort path: - Read result.has_more under self.mutex in on_stream_cancelled_callback. The HTTP thread replaces self.result wholesale under that mutex, so the unsynchronized read was a data race; a stale has_more could skip the abort. - Make abort_task idempotent: reader.cancel() and an AbortSignal can both reach it, and the second call would queue a redundant shutdown. - Skip schedule_receive_resume as well as enable_response_body_streaming once aborted; drain_events processes resumes before shutdowns, so a resume could re-arm draining on an already-closing socket. Test: stream the response body forever (paced) so the unfixed drain path keeps the connection open and the server never sees the close, making the regression deterministic instead of racing a finite body's idle teardown.
|
@robobun verify the behavior in node and deno is the same |
|
Verified against Node and Deno by running the issue's scenario as the client in each runtime (read one chunk, then
Deno and Bun run the issue's server verbatim ( Console output: One Deno nuance: it prints a deprecation note that Net: before this PR, Bun alone kept the connection open and drained the body, so neither callback fired; with the fix, Bun matches Node and Deno. |
reader.cancel() on an in-flight body now aborts the request instead of draining the abandoned body for keep-alive reuse, matching Node/Deno and browsers (#33227). Update the drain-for-keepalive assertion accordingly: the server no longer sends the whole first body, and a later request runs on a fresh connection.
|
CI surfaced a behavioral decision worth calling out. The one real failure was That test was added in #29831 and deliberately asserts the old drain-for-keepalive behavior: cancelling a 16 MiB in-flight body drains the whole thing so the connection can be pooled and reused ( So this is the tradeoff: with the fix, cancelling an in-flight response body aborts the request (closes the connection, server observes the cancel) and does not reuse that connection for keep-alive. A fully received body still drains/cleans up and stays poolable. I updated that test to assert the abort behavior (the first response no longer drains to completion; a follow-up request runs on a fresh connection), which matches Node/Deno/browsers. Flagging in case you'd rather keep draining for small/near-complete bodies (some clients drain when the remainder is tiny and abort otherwise). Happy to add that heuristic if you want it, but the straight abort is what Node and Deno do here. |
… test server.sent() < 2 * TOTAL is flaky on CI hosts with large TCP loopback buffers (tcp_rmem[2]+tcp_wmem[2] up to ~256 MiB): the 16 MiB body fully buffers during the stall, the first request completes, and sent reaches 2 * TOTAL. The sibling drain test uses a 1 GiB body for this exact reason. Keep the recovery check (a later request completes); the abort-vs-drain behavior is asserted directly in test/regression/issue/33227.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/webcore/fetch/FetchTasklet.rs (1)
2275-2287: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRoute cert-check failures through
abort_task()
The threecheck_server_identityfailure branches still bypass the new idempotency guard by writingabortedand callingdid_cancel/schedule_shutdowndirectly. Reuseabort_task()here so an earlier cancel path can’t enqueue the shutdown sequence twice.🤖 Prompt for 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. In `@src/runtime/webcore/fetch/FetchTasklet.rs` around lines 2275 - 2287, The three `check_server_identity` failure branches are still bypassing the idempotent abort path by setting `aborted` and calling `did_cancel`/`schedule_shutdown` directly. Update those failure paths in `FetchTasklet::check_server_identity` to call `abort_task()` instead, so the existing guard in `abort_task()` handles all cancellation and shutdown sequencing exactly once.
🤖 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 `@test/js/web/fetch/fetch-backpressure.test.ts`:
- Around line 303-314: The test in reader.cancel() mid-stream relies on an
arbitrary Bun.sleep(50) delay to keep the request in-flight, which makes it
flaky. Replace the timing-based wait with an actual condition in the
fetch-backpressure test, using the same reader-cancel flow and server streaming
behavior as the related regression test for issue 33227, so the cancel happens
only after confirming the stream is still active via observable I/O state rather
than wall-clock time.
---
Outside diff comments:
In `@src/runtime/webcore/fetch/FetchTasklet.rs`:
- Around line 2275-2287: The three `check_server_identity` failure branches are
still bypassing the idempotent abort path by setting `aborted` and calling
`did_cancel`/`schedule_shutdown` directly. Update those failure paths in
`FetchTasklet::check_server_identity` to call `abort_task()` instead, so the
existing guard in `abort_task()` handles all cancellation and shutdown
sequencing exactly once.
🪄 Autofix (Beta)
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: a93a8931-5301-451d-8631-685c0db7932a
📒 Files selected for processing (3)
src/runtime/webcore/fetch/FetchTasklet.rstest/js/web/fetch/fetch-backpressure.test.tstest/regression/issue/33227.test.ts
…n test - check_server_identity's three failure branches set aborted / did_cancel / schedule_shutdown inline; route them through the now-idempotent abort_task() so every abort path shares one shutdown-once guard. - Drop the Bun.sleep(50) in the backpressure cancel test; the prior reader.read() already confirms the stream is active, so the wait is not load-bearing for the recovery check.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/webcore/fetch/FetchTasklet.rs (1)
1610-1614: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the new comments within the 3-line limit.
These two added explanatory blocks exceed the repository comment-length rule; they can be condensed without losing the invariant.
Proposed wording
- // reader.cancel() on a still-arriving body aborts the fetch so the - // connection closes and the server observes the cancellation. A fully - // received body is left to drain/cleanup so the socket stays reusable. - // `result` is replaced wholesale by the HTTP thread under `mutex`, so - // read `has_more` under the lock like the other JS-thread callbacks. + // reader.cancel() aborts a still-arriving body so the server sees disconnect; + // fully received bodies keep drain/cleanup for reuse. Read `has_more` + // under `mutex` because the HTTP thread replaces `result` wholesale.- // An aborted fetch (reader.cancel() on an in-flight body, or an - // AbortSignal) is already shutting the connection down; don't re-arm the - // receive or resume draining, which would read the rest of an unbounded - // body and hold the socket open (drain_events resumes before shutdowns). + // Aborted fetches are already shutting down; don't re-arm receive/resume + // draining, which can read an unbounded body and hold the socket open + // because drain_events resumes before shutdowns.As per coding guidelines, "Keep code comments to 3 lines max." <coding_guidelines>
Also applies to: 1765-1768
🤖 Prompt for 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. In `@src/runtime/webcore/fetch/FetchTasklet.rs` around lines 1610 - 1614, The newly added explanatory comments in FetchTasklet should be shortened to satisfy the 3-line comment limit without losing the key invariant. Condense the two comment blocks near the `reader.cancel()` logic and the `has_more` read-under-lock note into brief, focused comments, keeping the essential behavior notes only. Update the comment text in `FetchTasklet` where these explanatory blocks were added so each comment stays within the repository guideline while still referencing the cancellation behavior, reusable socket handling, and mutex-protected `result`/`has_more` access.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@src/runtime/webcore/fetch/FetchTasklet.rs`:
- Around line 1610-1614: The newly added explanatory comments in FetchTasklet
should be shortened to satisfy the 3-line comment limit without losing the key
invariant. Condense the two comment blocks near the `reader.cancel()` logic and
the `has_more` read-under-lock note into brief, focused comments, keeping the
essential behavior notes only. Update the comment text in `FetchTasklet` where
these explanatory blocks were added so each comment stays within the repository
guideline while still referencing the cancellation behavior, reusable socket
handling, and mutex-protected `result`/`has_more` access.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6a5c83fa-c83a-4627-a964-62837470fd48
📒 Files selected for processing (2)
src/runtime/webcore/fetch/FetchTasklet.rstest/js/web/fetch/fetch-backpressure.test.ts
💤 Files with no reviewable changes (1)
- test/js/web/fetch/fetch-backpressure.test.ts
|
Heads up on CI: the only red lane is This is the artifact-download step fetching the already-built darwin-aarch64 binary from S3, not the test run, and it's recurred at the identical step across builds (67836 and 67845). This diff is a few lines of fetch cancel logic plus tests, so it can't affect artifact size or the build/upload step. The change itself is verified: locally the new A re-run of the darwin-aarch64 lane (or a merge, if you're satisfied with the review) should clear it. Happy to push a retrigger if you'd prefer I re-roll it. |
…e lock on_stream_cancelled_callback now always calls the idempotent abort_task() instead of reading result.has_more under the mutex. Node/Deno/browsers abort unconditionally, and the cancel handler is cleared on full body delivery (clear_stream_handlers), so the callback effectively only fires mid-stream anyway. This removes both the off-thread result data race and the reentrant mutex lock (reachable when a request-body sink cancel handler cancels the response reader while on_progress_update holds the mutex).
abort_task() also calls tracker.did_cancel (touches JSC), so 'only touches atomics + schedule_shutdown' was wrong and could mislead someone into thinking it is safe from the GC finalizer. Keep only the accurate 'idempotent' note.
|
Update on CI now that more lanes have reported on build 67862: the red is entirely from failures unrelated to this diff.
This diff is a few lines of fetch reader-cancel logic plus its tests; none of the red lanes exercise it (211 test shards passed). A |
2 participants
Fixes #33227 (reopen of #19211).
Repro
Before:
aborted = false,cancelled = false. The client kept theconnection open and drained the body, so the server never saw the cancel.
Cause
Cancelling a fetch response body reader (
reader.cancel()/res.body.cancel()) routes through the stream source's cancel handler toFetchTasklet::on_stream_cancelled_callback, which calledignore_remaining_response_body. That path sets the receive mode toIgnoreand calls
enable_response_body_streaming(), which keeps reading anddiscarding the remaining response body to leave the socket reusable. For an
unbounded or still-streaming response that drain never completes, so the
connection is held open indefinitely and the server never observes a close,
never fires
request.signal'sabort, and never runs the response stream'scancel.Fix
When the response body is still arriving at cancel time (
result.has_more),abort the fetch via the existing
abort_task()(sets the aborted flag, cancelsthe tracker, and
schedule_shutdowns the connection) instead of draining, sothe connection closes and the server observes the cancellation. A fully
received body has nothing left to abort and still falls through to the
drain/cleanup path, leaving the socket reusable for keep-alive.
ignore_remaining_response_bodynow skips the drain when the fetch is aborted,since the connection is already shutting down.
This routes both
reader.cancel()andres.body.cancel(), over HTTP/1 andHTTP/2, through the same shared path.
Verification
New regression test in
test/regression/issue/33227.test.ts: a server streamsan SSE body and records
request.signalabort + the response stream'scancel;the client reads a chunk then calls
reader.cancel()and awaits bothserver-side callbacks firing.
git stashofsrc/): the callbacks never fire and thetest times out.
test/js/web/fetch/fetch-stream-cancel-leak.test.ts(GC afterreader.cancel()/body.cancel()) and the abort-path tests(
fetch-abort-stream-body,fetch-abort-queued,fetch-abort-socket-close-race)still pass.