Cancel the JS source when a ReadableStream pump is dropped mid-stream#6833
Cancel the JS source when a ReadableStream pump is dropped mid-stream#6833cnluzhang wants to merge 1 commit into
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: abd06069ce
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
019da73 to
fc0a147
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc0a147a3d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
fc0a147 to
9d32413
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
8eafc3e to
73a39aa
Compare
73a39aa to
7bbdb50
Compare
|
@danlapid — tagging you as you know this path best. This is about the This PR restores that behavior by cancelling the source from For what it's worth on the practical side: I run WDL, a self-hosted Workers platform on stock workerd, and this is the one thing that regressed for me on |
7bbdb50 to
5591cf6
Compare
|
@danlapid heads-up before you get to this — the patch has been revised since my earlier ping (
Re-ran the full streams matrix (13/13), |
5591cf6 to
a5e7657
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a5e76574ca
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
443fa14 to
31dde98
Compare
After the draining-read pump path replaced PumpToReader (commit c329332), dropping the pump coroutine while the JS ReadableStream source was suspended awaiting more data no longer cancelled the source. This regressed client-disconnect handling: when the HTTP layer drops the response-body pump, the underlying source's cancel() algorithm never ran and the stream kept producing data until natural completion. Restore the behavior from pumpToImpl's teardown: when the coroutine is dropped before settling, schedule the source cancel as a waitUntil task so IncomingRequest::drain() keeps the context open until the JS cancel() runs -- the same scheduling WorkerEntrypoint uses for its disconnect abort task. The isolate lock is unavailable during coroutine teardown, and the pump promise may itself be owned by the IoContext (response-body pumps live in the context's task sets), so the defer can run during ~IoContext; an IoContext::WeakRef guard (the pattern documented at IoContext::getWeakRef()) turns that case into a safe no-op. A pumpSettled flag suppresses the cancel on the normal-completion and error paths, which already finalize the stream themselves. Both cancel sites tolerate a rejecting cancel: cancelling a stream whose source already errored rejects with the stored error, so the error path ignores that rejection to let the failure that actually broke the pump propagate (and to reliably reach the pumpSettled suppression), and the teardown task swallows it so best-effort disconnect cleanup cannot land as a failed waitUntil task when the stream errored between the drop and the task acquiring the isolate lock. Cancelling with kj::none (undefined reason) matches the pre-regression PumpToReader drop behavior. Regression tests pin: the drop-mid-stream cancel running, clean completion not invoking cancel(), the original sink failure surviving a rejecting cancel algorithm, the already-errored teardown window leaving the waitUntil status untouched, and request drain completing only after the teardown cancel has finished. Fixes cloudflare#6832 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
31dde98 to
d0e09ea
Compare
Problem
Fixes #6832.
When a client disconnects from a streaming response whose body is an async
JavaScript
ReadableStream, the stream'scancel()algorithm is no longerinvoked. Because
cancel()never runs, the worker cannot observe thedisconnect: the source keeps producing data (enqueuing chunks until natural
completion) long after the client is gone.
This is a regression between
1.20260617.1and1.20260619.1, introduced bycommit c329332 ("Remove draining read standard streams autogate"), and the
pump path is unchanged through
1.20260717.1:1.20260617.1: stream source receivescancelon disconnect1.20260619.1and later: stream source runs to natural completion, nocancelRoot cause
c32933263removed the legacyPumpToReaderpath and leftReadableStreamJsController::pumpTo()with only theDrainingReader+pumpToImplcoroutine.When the client disconnects, the HTTP layer proactively drops the
response-body pump promise. The old
PumpToReadereffectively preservedcancellation on drop: tearing it down left a pending read continuation that,
on running, observed the reader was gone and canceled the controller.
The new
pumpToImplcoroutine instead just tears down its frame, whichdestroys the
DrainingReader.~DrainingReadercallsreleaseReader(maybeJs = kj::none)— with no isolate lock available it onlyclears the lock refs and never cancels the JS source. The coroutine only
canceled from its
KJ_CATCH(exception) path, which a quiet disconnect neverreaches.
Fix
Cancel the source from
pumpToImpl's teardown when the coroutine is droppedbefore it settles. The isolate lock is unavailable during coroutine teardown,
so the cancel is scheduled onto the IoContext, with two details that matter
for lifetime correctness:
IncomingRequest::drain()keepsthe context open until the JS
cancel()has actually run — the samescheduling
WorkerEntrypointuses for its disconnect abort task. A plaintask would race context teardown (nothing awaits the
tasksset after therequest ends), so the cancel could be silently dropped in exactly the
disconnect scenario this fixes.
IoContext::WeakRef(the patterndocumented at
IoContext::getWeakRef()). Pump promises can be owned by theIoContext's own task sets (e.g.
ReadableStream::serialize, outboundfetch()with a JS-stream body), so the coroutine can be dropped during~IoContextitself; the guard turns that case into a safe no-op instead oftouching a context mid-destruction.
A
pumpSettledflag suppresses the deferred cancel on the normal-completionand error paths, which already finalize the stream themselves (the error path
cancels the reader directly). Both cancel sites tolerate a rejecting cancel —
cancelling a stream whose source already errored rejects with the stored
error:
the pump propagates (previously the rejection replaced it and the explicit
rethrow was unreachable) and
pumpSettledreliably suppresses the teardowndefer.
cannot land as a failed waitUntil task when the stream errored between the
drop and the task acquiring the isolate lock.
Cancelling with
kj::none(undefined reason) deliberately matches thepre-regression
PumpToReaderdrop behavior; for comparison, the (currentlyunwired)
ReadableSourceKjAdapterdrop path passes aDISCONNECTEDreason,so that precedent exists if a reason is preferred here.
Testing
Five regression tests in
src/workerd/api/streams-test.c++, each verified tofail without the corresponding part of the fix:
ReadableStream pumpTo cancels the JS source when dropped mid-stream: a JSReadableStreamsource suspends on its next read, the pump is dropped, andthe test asserts the source's
cancel()runs.ReadableStream pumpTo does not cancel the JS source on clean completion:pumps a closing stream to completion, drops the settled pump, turns the
event loop, and asserts
cancel()was not invoked — pinning thepumpSettledsuppression.ReadableStream pumpTo propagates the original failure when cancel also rejects: a failing sink breaks the pump while the source's cancelalgorithm rejects; the pump must reject with the sink failure, not the
cancel rejection.
ReadableStream pumpTo teardown cancel tolerates an already-errored stream: the pump is dropped after the source errored but before thesuspended read observes it; the teardown cancel's rejection must not flip
the context's
waitUntilStatus.ReadableStream pumpTo teardown cancel completes before request drain finishes: the cancel algorithm is held open on an external gate and thetest asserts
IncomingRequest::drain()does not complete until the cancelhas finished, with the request as the context's only owner so
~IoContextruns right after the drain — pinning the waitUntil scheduling (a plain task
would let drain finish first and context teardown would drop the cancel).
The branch is rebased onto Release 2026-07-17. Against this head:
End-to-end against the reporter's repro
(https://github.com/cnluzhang/workerd-stream-cancel-repro): a workerd binary
built from this branch reports
stream=cancelin both the direct and proxiedtopologies where stock
1.20260619.1and later reportended-normally.