Re-land the fix and regression test for vuln-187 - #6934
Merged
Conversation
A draining read delivers its result to pumpToImpl through a kj event, and runs the stream's pull() callback along the way. A pull() that aborts the request's signal makes the canceler destroy the pump's coroutine frame from inside that event, so the event is destroyed while still firing and kj aborts the process with "Promise callback destroyed itself". Neither isolate-lock round trip in the pump is awaited directly any more. Each is handed to the IoContext's task set, and the pump awaits an unrelated fulfiller, so dropping the pump destroys only that await and leaves the firing event to be unwound by its new owner. Those tasks outlive the coroutine frame, so they reach the DrainingReader through a kj::Weak rather than the frame-owned kj::Own. Without that, the read lambda would run against a freed reader whenever the pump is dropped before the isolate lock is acquired. The fix originally landed as 00e86e1 and was swept away by 4447562, a wholesale revert of the streams-cleanup-investigation merge. That first version captured the reader by reference, which is what the kj::Weak above replaces; this follows the later 2de66f75e, which had corrected it. Both are James's, as is the third attempt, f3f408768, which drops the code change and keeps only the test — not an option here, since the abort still reproduces without it. The surrounding tree has moved on twice over: the draining read is no longer autogated, so the test needs no gate, and DrainingReader is a kj::PtrTarget, so the weak pointer comes from addWeakToThis() rather than a hand-rolled cell. Co-authored-by: James M Snell <jsnell@cloudflare.com> Assisted-by: OpenCode:claude-opus-5
Contributor
|
LGTM |
jasnell
approved these changes
Aug 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A draining read delivers its result to pumpToImpl through a kj event, and runs the stream's pull() callback along the way. A pull() that aborts the request's signal makes the canceler destroy the pump's coroutine frame from inside that event, so the event is destroyed while still firing and kj aborts the process with "Promise callback destroyed itself".
Neither isolate-lock round trip in the pump is awaited directly any more. Each is handed to the IoContext's task set, and the pump awaits an unrelated fulfiller, so dropping the pump destroys only that await and leaves the firing event to be unwound by its new owner.
Those tasks outlive the coroutine frame, so they reach the DrainingReader through a kj::Weak rather than the frame-owned kj::Own. Without that, the read lambda would run against a freed reader whenever the pump is dropped before the isolate lock is acquired.
The fix originally landed as 00e86e1 and was swept away by 4447562, a wholesale revert of the streams-cleanup-investigation merge. That first version captured the reader by reference, which is what the kj::Weak above replaces; this follows the later 2de66f75e, which had corrected it. Both are James's, as is the third attempt, f3f408768, which drops the code change and keeps only the test — not an option here, since the abort still reproduces without it.
The surrounding tree has moved on twice over: the draining read is no longer autogated, so the test needs no gate, and DrainingReader is a kj::PtrTarget, so the weak pointer comes from addWeakToThis() rather than a hand-rolled cell.
Assisted-by: OpenCode:claude-opus-5