Conversation
…into ordered state enums
Both sinks encoded a monotone lifecycle as a set of dependent bool fields
whose invariants (aborted implies done; done implies ended) were enforced
only by convention. Replace them with Ord-derived enums so the ordering is
the type.
NetworkSink: ended/done/cancel become NetworkSinkState { Open, Ended, Done,
Cancelled }. The cancel flag was write-only; nothing ever read it. The
!ended branch in end_from_js was dead (end(None) immediately above always
leaves the sink at least Ended) and is now a debug_assert.
HTTPServerWritable: done/requested_end/aborted become HttpSinkState
{ Writing, EndRequested, Done, Aborted }. The repeated 'done ||
requested_end' write guard is now !state.is_writing(); mark_done() advances
with max() so it never regresses an Aborted sink. end()/end_from_js() now
short-circuit on any non-Writing state, which drops a redundant second
source.close()/finalize() on a sink that an earlier write path already
marked done.
Callers in RequestContext and s3/client updated to use the state
accessors; ended_response/has_backpressure/source_pending_pull stay as
independent bools.
|
Warning Review limit reached
Next review available in: 25 minutes 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 (2)
WalkthroughChangesStreaming sinks now use monotonic lifecycle enums instead of boolean flags. HTTP and network stream paths, request handling, and S3 completion logic use state predicates and transitions. Sink lifecycle migration
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Found 4 issues this PR may fix:
🤖 Generated with Claude Code |
![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/s3/client.rs`:
- Line 723: Update the sink completion assignment in the failure callback to use
the existing monotonic completion helper rather than directly setting
NetworkSinkState::Done. Preserve NetworkSinkState::Cancelled when
NetworkSink::abort() ran first, so later is_aborted() checks remain true.
In `@src/runtime/webcore/streams.rs`:
- Around line 2136-2163: Shorten the multi-sentence documentation on
NetworkSinkState and its Done variant while preserving the lifecycle meaning.
Add a mark_done method to NetworkSinkState that advances via the state’s max
operation, then update S3UploadStreamWrapper::resolve to use this guarded setter
instead of assigning Done directly, preserving Cancelled when already reached.
🪄 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: ee2073f3-7440-4a37-a527-7999a452ac77
📒 Files selected for processing (3)
src/runtime/server/RequestContext.rssrc/runtime/webcore/s3/client.rssrc/runtime/webcore/streams.rs
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline nit, I checked the other direct NetworkSinkState assignments for monotonicity regressions: end_from_stream's state = Ended at streams.rs:2476 is guarded by if ended { return } above it, so state is always Open there; and the state = Done at s3/client.rs:723 (flagged by CodeRabbit) is behaviorally unchanged from the old ended = true; done = true since cancel was write-only — nothing reads Cancelled. The end_from_js dead-branch removal also checks out: self.end(None) at line 2515 already sends EOF via task.write_bytes(b"", true) and calls source.close(). The one intentional behavior change (HTTPServerWritable::end() now short-circuits on Done instead of re-running source.close()+finalize()) is the piece that most warrants human eyes.
Extended reasoning...
This is a non-trivial lifecycle refactor across ~40 guard sites in HTTPServerWritable (the Bun.serve response sink) and NetworkSink (S3 upload), with one acknowledged behavior change and one dead-code removal. The mapping from the old bool triple to the ordered enum looks correct at every site I traced, and the ruled-out concerns above hold. Deferring rather than approving because streaming teardown ordering in this file has a history of subtle bugs, and the intentional short-circuit in end()/end_from_js() (skipping source.close()+finalize() when already Done) deserves a maintainer's sign-off even though finalize() is idempotent.
…lure path The enum doc comments were flagged for carrying refactor history; the variant names plus the Ord derive already encode the monotone order, so keep a single-line invariant note on each. In S3UploadStreamWrapper::resolve's failure branch, advance the state with max(Done) instead of a direct assignment so a sink that already reached Cancelled is never regressed.
|
Updated 8:07 AM PT - Aug 2nd, 2026
✅ @robobun, your commit 56fe17b1ec939b9ae05fa7068bb9c8b8fe36a91d passed in 🧪 To try this PR locally: bunx bun-pr 36770That installs a local version of the PR into your bun-36770 --bun |
|
Status: ready for review. This is a behaviour-preserving refactor (two bool triples collapsed into ordered enums), so there is no fail-before test to add; the PR body lists the existing suites it was verified against. Self-review raised no surviving concerns. All line comments addressed in 81cc1d8 (doc comments trimmed to single-line invariants; the one external state write in The first CI run (#87692) went red only on Cloudflare R2 |
|
Closing per the no-test requirement: this is a behaviour-preserving type-system refactor (two bool triples collapsed into The branch is left in place; reopen if the refactor is wanted on its own merits. |
There was a problem hiding this comment.
I didn't find bugs, but this touches the HTTP response-streaming and S3-upload sink lifecycle and includes an acknowledged behaviour change (HTTPServerWritable::end()/end_from_js() now short-circuit on any non-Writing state, skipping the source.close() + finalize() path that previously ran when done && !requested_end), so it's worth a human look.
What was reviewed:
- Verified
NetworkSink::end_from_js's removed branch is genuinely unreachable —self.end(None)at the top always advances toEndedand already sends EOF + closes the source. - Verified
is_end_requested()is== EndRequested(not>=), sosend_*/on_writable/on_auto_flushguards match the oldrequested_endbool exactly. - Verified
finalize()'s directstate = Doneassignment can't regressAborted(guarded by!is_done()), and the s3resolve()write now usesmax(Done)soCancelledis preserved. - Checked
mark_done()callers inwrite_latin1/write_utf16/flush— the skippedfinalize()on subsequentend()only defers buffer-pool return todestroy();resis alreadyNonethere so noend_streamis lost.
Extended reasoning...
Overview
The PR collapses two sets of dependent lifecycle bools into Ord-derived state enums across three files: HTTPServerWritable (done/requested_end/aborted → HttpSinkState), NetworkSink (ended/done/cancel → NetworkSinkState), and their external readers in RequestContext.rs and s3/client.rs. Roughly 200 lines of diff. My previous inline comment (drop the refactor-history clause from the enum doc) was addressed in 81cc1d8, along with CodeRabbit's Cancelled-regression concern.
Security risks
None identified. This is internal lifecycle bookkeeping for response/upload sinks; no auth, no untrusted-input parsing, no crypto.
Level of scrutiny
High. HTTPServerWritable is the write side of every Bun.serve streamed response — a mis-mapped guard here manifests as hung responses, double-end_stream, or leaked pooled buffers under load, none of which the unit suites reliably catch. Most of the mapping is mechanical (done || requested_end → !is_writing(), aborted → is_aborted()), and I traced those 1:1. The exception is the end()/end_from_js() change: previously done && !requested_end fell through to source.close() + finalize() (which returns the buffer to the pool even when the not-done block is skipped); now it early-returns. The PR body argues this is safe (destroy() frees the buffer, the second source.close() was redundant), and I couldn't construct a counterexample from the mark_done() call sites — but this is the one place the refactor changes control flow rather than just re-encoding it, and it ships without a targeted test.
Other factors
- The
NetworkSink::end_from_jsdead-branch removal checks out:end(None)at line 2499 unconditionally reachesEndedand already performs the EOF write +source.close()the removed block would have. - The old
cancelfield was verified write-only (no readers in tree). is_end_requested()uses==not>=, so it correctly matches only the transientEndRequestedstate (the oldrequested_end && !donereads insend_*were guarded by earlierdonechecks ordebug_assert!(!done)).- No new test; the author ran the existing serve/stream/leak suites and
rust:check-all, but per REVIEW.md a behaviour change (even a claimed-benign one) normally ships with a fail-before/pass-after test.
|
Traced the one spot the review calls out, the end() / end_from_js() short-circuit, since it is the only place this PR changes control flow instead of re-encoding it. The old
Neither case is observable from JS (both variants return the same value and the buffer pool is internal), which is why the description argues a fail-before test cannot exist for this diff. The suites listed there cover the unchanged guards. Agreed the merge call on that spot belongs to a human reviewer. |
There was a problem hiding this comment.
No bugs found, but this refactor touches HTTP server / S3 sink lifecycle (finalize/abort/end ordering) and includes one intentional control-flow change in end() / end_from_js() that the author has already flagged for a human sign-off — deferring on that basis.
Checked: end_from_stream's direct state = Ended/Done writes are gated by the if ended { return } at the top, so they can't regress Cancelled. finalize()'s state = Done sits inside !is_done(), which excludes Aborted. The s3/client.rs failure path now advances via max(Done). The removed !self.ended branch in NetworkSink::end_from_js is unreachable after the preceding self.end(None).
Extended reasoning...
Overview
Collapses two dependent-bool triples into Ord-derived lifecycle enums: HTTPServerWritable's done/requested_end/aborted → HttpSinkState { Writing, EndRequested, Done, Aborted }, and NetworkSink's ended/done/cancel → NetworkSinkState { Open, Ended, Done, Cancelled }. Touches src/runtime/webcore/streams.rs (the sinks themselves), src/runtime/server/RequestContext.rs (four external readers), and src/runtime/webcore/s3/client.rs (five external readers/writers). Most of the diff is a mechanical predicate rewrite; the non-mechanical parts are mark_done() using max(), the end() / end_from_js() early-return widening, and the removal of a provably-dead branch in NetworkSink::end_from_js.
Security risks
None identified. This is internal lifecycle bookkeeping; no untrusted-input parsing, no auth/crypto, no allocation-size arithmetic.
Level of scrutiny
High. The PR description itself notes this file "has accumulated several lifecycle-ordering bugs over time (drain-after-abort, double-finalize, aborted-but-not-done)", and REVIEW.md treats refactors as guilty until proven behaviour-preserving. The one place control flow actually changes — HTTPServerWritable::end() / end_from_js() now short-circuit on any non-Writing state, so the old done && !requested_end fallthrough to source.close() + mark_done() + finalize() no longer runs — has been carefully traced by the author (both reachable paths lead to a source that's already closed and a finalize that will still run before teardown), and they explicitly said the merge call on that spot belongs to a human. I agree.
Other factors
I audited every direct state = assignment for monotonicity regressions: the two in end_from_stream sit behind an is_ended() early return; finalize()'s sits behind !is_done(); abort() and start() are the intended terminal/reset writers; the s3/client.rs failure branch was fixed to use max() per earlier review. My prior inline comment (refactor-history in the HttpSinkState doc) was addressed in 81cc1d8. No new tests ship, which is defensible for the mechanical majority but leaves the end() short-circuit change resting on the argument that the deferred finalize() is guaranteed by RequestContext teardown — that's the piece a maintainer familiar with the pooled-buffer lifecycle should confirm.
…EOF branch (#37407) ### What Two pieces of dead code in `NetworkSink`: - `cancel` was set in `abort()` and never read anywhere in the tree. - In `end_from_js`, `self.end(None)` runs first and always leaves `ended` set (it returns early when it already was, otherwise it sets it, and nothing ever clears it), so the `if !self.ended { ... send EOF ... }` block below could not execute. Both removed; `end_from_js` now 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.ts` passes, including the local-server upload cases that go through `NetworkSink`.
…ink start (#37406) ### What `NetworkSink::start` and `FetchRequestBodySink::start` both return early when `self.ended` is set and then assigned `self.ended = false` a few lines later, so the assignment could only ever write `false` over `false`. Removed in both places. No behaviour change. Related: #36770 reworks `NetworkSink`'s lifecycle flags more broadly; this is just the two dead lines. ### Verification `bun bd test test/js/bun/s3/s3.test.ts test/js/web/fetch/fetch.stream.test.ts test/js/web/fetch/body.test.ts test/js/web/fetch/fetch-abort-stream-body.test.ts` passes (the S3 cases that run against a local server included).
|
Closing as superseded. Main has since landed this PR's content through separate PRs, and the remaining piece was decided differently:
The only part not on main is collapsing NetworkSink's ended/done pair into an enum; if that is still wanted it belongs in a small fresh PR against the current code rather than a rebase of this one. |
2 participants
What
Two sinks in
src/runtime/webcore/streams.rstracked their lifecycle as a set of dependentboolfields whose invariants were enforced only by convention. This PR collapses each set into a singleOrd-derived enum so the ordering is the type and an out-of-order combination (e.g.aborted && !done) is unrepresentable.NetworkSink:ended/done/cancel→NetworkSinkStateThe invariant was
cancel ⇒ done ⇒ ended; every site that setdonealso setended, andcancelwas only set inabort()after both. Predicates becomestate >= Ended/state >= Done.Two bits of dead code fell out:
cancelwas write-only; nothing in the tree ever read it.if !self.ended { … send EOF … }branch inend_from_jswas unreachable:self.end(None)two lines above always leaves the sink at leastEnded. Replaced with adebug_assert!.HTTPServerWritable:done/requested_end/aborted→HttpSinkStateabort()always setdone = truebeforeaborted = true, soaborted ⇒ done. The compound guardself.done || self.requested_end(write / write_latin1 / write_utf16 / on_writable / writable_result) is now!state.is_writing().mark_done()advances withmax()so it never regresses anAbortedsink.The one intentional behaviour change:
end()/end_from_js()now short-circuit on any non-Writingstate. Previously, callingend()after a write path had alreadymark_done()d the sink (deadresonwrite_latin1/write_utf16/flush) would fire a secondsource.close()and afinalize()on an already-closed source. The redundant close was never observed to matter, andfinalize()is idempotent;destroy()still frees the buffer. The independentended_response/has_backpressure/source_pending_pullbools are untouched.External readers in
RequestContext.rsands3/client.rsupdated to use the accessors.Why
This file has accumulated several lifecycle-ordering bugs over time (drain-after-abort, double-finalize, aborted-but-not-done), and the bool soup made each one hard to reason about in review. Encoding the monotone order in the type removes the class of "set one flag, forgot the other" mistakes and makes every guard (
is_writing(),is_done()) name what it actually checks.Verification
No user-visible behaviour change; this is a structural refactor. Verified against the existing suites:
test/js/bun/http/serve.test.ts(280 pass; 4 pre-existing environment-bound failures unrelated to streaming)test/js/bun/http/serve-body-leak.test.ts,bun-serve-direct-stream.test.ts,serve-readable-stream.test.ts,bun-serve-abort-signal.test.ts,async-iterator-stream.test.tstest/js/web/fetch/body-stream.test.ts,fetch.stream.test.ts,stream-fast-path.test.tstest/js/web/streams/streams.test.jsbun run rust:check-all(all 10 targets)No new test is added because there is no observable failure mode this fixes; the gate's fail-before/pass-after proof is not applicable to a behaviour-preserving refactor.