streams: remove NetworkSink's write-only cancel flag and unreachable EOF branch - #37407
Conversation
…EOF branch cancel was set in abort() and never read. In end_from_js, self.end(None) runs first and always leaves ended set (it either returns early because it already was, or sets it), so the if !self.ended block that re-sent EOF could not execute.
|
Warning Review limit reached
Next review available in: 22 seconds Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — verified both removals are dead code with no behavior change.
What was reviewed:
- Grepped the tree for reads of
NetworkSink::cancel— none; only the removed write inabort()and the field/default. - Traced
end(): both paths (early-return when already ended, otherwise setended = truebefore any fallible/re-entrant call) leaveendedtrue, so the removedif !self.endedblock afterself.end(None)was unreachable. - Checked the only
ended = falsewrite (start()) — guarded byif self.ended { return }, so cannot flip it back.
Extended reasoning...
Overview
Removes two pieces of dead code from NetworkSink in src/runtime/webcore/streams.rs: (1) the pub(crate) cancel: bool field, its default initializer, and the single write in abort(); (2) the if !self.ended { ... send EOF ... } block in end_from_js that follows self.end(None). Net -12 lines, no other files touched.
Verification of claims
For the cancel field: grepped src/**/*.rs for .cancel and \bcancel\b in streams.rs. Every hit is a method call on unrelated types (ReadableStream, timers, napi, etc.) or a comment; there is no read of NetworkSink::cancel. The field is pub(crate) so C++/codegen cannot reach it either.
For the unreachable branch: end() at line 2425 either returns early because self.ended is already true, or sets self.ended = true on line 2431 before doing anything else and never clears it. end_from_js calls self.end(None) first, so self.ended is guaranteed true on the next line. The only write of ended = false in the file is in start(), which itself early-returns when ended is true, so no re-entrant path through pending.run() or task.write_bytes can flip it back. The removed block duplicated exactly what end() already did (write EOF, close source), so even conceptually there's no lost side effect.
Security risks
None. Pure dead-code removal in the S3 upload sink; no input handling, auth, or crypto touched.
Level of scrutiny
Low. Small, mechanical, provably behavior-preserving. Aligns with the repo rule that dead code should be deleted; the PR description also links #36770 which is folding these flags into a state enum, so this narrows that diff.
Other factors
Author ran the S3 test suite that exercises NetworkSink (upload, stream-cancel-leak, connection-close). No prior review comments on the PR. No test changes needed since there is no observable behavior change to assert on.
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
3 participants
What
Two pieces of dead code in
NetworkSink:cancelwas set inabort()and never read anywhere in the tree.end_from_js,self.end(None)runs first and always leavesendedset (it returns early when it already was, otherwise it sets it, and nothing ever clears it), so theif !self.ended { ... send EOF ... }block below could not execute.Both removed;
end_from_jsnow just creates the end promise and returns it on that path. No behaviour change.Related: #36770 folds the same flags into a state enum as part of a larger change; this PR is only the removal of the dead parts.
Verification
bun bd test test/js/bun/s3/s3.test.ts test/js/bun/s3/s3-stream-cancel-leak.test.ts test/js/bun/s3/s3-connection-close.test.tspasses, including the local-server upload cases that go throughNetworkSink.