WebSocket client: reset inflater after Z_STREAM_END with context takeover - #34105
Conversation
RFC 7692 7.2.3 allows a sender to end a DEFLATE stream with BFINAL=1 and start a fresh stream for the next message. The client's PerMessageDeflate::decompress broke out of its zlib loop on Z_STREAM_END but only reset the stream when server_no_context_takeover was negotiated. With plain context takeover the persistent inflater stayed in the finished state, so every subsequent compressed message that reached the zlib path (decompressed size over 128KiB, or sync-flushed input that libdeflate rejects) was delivered to onmessage as an empty payload with no error or close. Reset the inflater whenever Z_STREAM_END is observed, matching npm ws.
|
Updated 9:56 PM PT - Jul 13th, 2026
❌ @robobun, your commit 47fe4b6 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 34105That installs a local version of the PR into your bun-34105 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
WalkthroughChangesThe WebSocket permessage-deflate decompressor now detects WebSocket Deflate Stream Reset
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Re #21376: that issue's symptom is the same message being repeated indefinitely (identical content delivered N times), which points at a frame buffer/offset bug in the receive path. This PR only changes the inflater state after The |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — minimal, well-reasoned fix with a targeted regression test.
What was reviewed:
- Verified
InflateDecoder::reset()wrapsinflateReset(src/zlib/lib.rs:1229), which is the correct call afterZ_STREAM_END— no window to preserve once the sender set BFINAL=1. - Checked the change is purely additive to the existing
server_no_context_takeoverreset condition; the libdeflate fast-path early-return is untouched. - Test wires
onerror/oncloseto reject, cleans up infinally, usesport: 0, and asserts byte-exact content across bothserver_no_context_takeovervariants.
Extended reasoning...
Overview
This PR touches two files: a 3-line native change in src/http_jsc/websocket_client/WebSocketDeflate.rs (add a saw_stream_end flag, set it on zlib::ReturnCode::StreamEnd, OR it into the existing inflater-reset condition) and a ~100-line test.each regression test in test/js/web/websocket/websocket-permessage-deflate-edge-cases.test.ts. The bug: after a BFINAL=1 message reaches the zlib path under context takeover, the persistent inflater stays in the finished state and every subsequent compressed message is delivered as an empty payload. RFC 7692 §7.2.3 explicitly permits a sender to end a stream with BFINAL and start a fresh one; npm ws handles this by recreating its InflateRaw on stream end.
Security risks
None. This is internal decompression state management on the client receive path. No new allocation sizing, no user-input validation changes, no auth/crypto surface. The existing MAX_DECOMPRESSED_SIZE bomb guard is unchanged.
Level of scrutiny
Low-to-medium. The native change is a 3-line additive condition on an existing reset call. I confirmed InflateDecoder::reset() calls zlib's inflateReset (src/zlib/lib.rs:1229-1231), which is exactly the right primitive — it discards the finished-stream state without reallocating. Because BFINAL=1 explicitly terminates the DEFLATE stream, there is no LZ77 window to carry over, so resetting under context takeover loses nothing. The change cannot regress the pre-existing behavior: the only new effect is resetting an inflater that was previously stuck returning 0 bytes forever.
Other factors
- The test is well-constructed per repo conventions: deterministic LCG bytes (not
randomBytes) to force the >128KiB zlib path,test.eachover bothserver_no_context_takeovervalues, failure events wired to reject the awaited promise, resources released infinally, byte-exact.equals()assertions. The PR description states the context-takeover case fails on main withmsg2.len=0, msg3.len=0. - No CODEOWNERS entry covers
src/http_jsc/websocket_client/. - The unrelated clippy failure noted in the thread is from #34067 and does not touch this diff.
- The server-side sibling was handled separately in #33592 per the description.
There was a problem hiding this comment.
LGTM — the runtime change is a single boolean OR'd into the existing reset() condition, directly implementing RFC 7692 §7.2.3, with a fails-before/passes-after regression test covering both server_no_context_takeover values. The inline nit about WEBSOCKET_GUID is cosmetic and non-blocking.
Checked that the libdeflate fast-path early-return is unaffected (it never touches the persistent zlib stream, so saw_stream_end is correctly irrelevant there), and that the error-return paths (TooLarge/InflateFailed) don't need the reset since the connection is torn down. The pre-existing server_no_context_takeover reset path is preserved unchanged.
Extended reasoning...
Overview
The PR touches two files: a 3-line effective change in src/http_jsc/websocket_client/WebSocketDeflate.rs (add a saw_stream_end bool, set it on Z_STREAM_END, OR it into the existing decompress_stream.reset() condition) and a ~105-line test.each regression test in test/js/web/websocket/websocket-permessage-deflate-edge-cases.test.ts. The runtime change extends when an existing reset() call fires — it introduces no new API, no new state, and no new control-flow branches beyond the flag set.
Security risks
None identified. The change resets a zlib inflater more often, not less; it cannot cause the decompressor to accept data it previously rejected. The decompression-bomb guard (MAX_DECOMPRESSED_SIZE) and error paths are untouched. The test's raw TCP server binds 127.0.0.1:0 and is closed in finally.
Level of scrutiny
Moderate — this is protocol state-machine code in the WebSocket client, but the change is effectively one predicate widened on a call that already existed. The RFC citation (§7.2.3) is correct: after a BFINAL block the sender begins a fresh DEFLATE stream, so the receiver must reinitialize its context regardless of context-takeover negotiation. The PR description shows npm ws does the same (recreates InflateRaw on stream end), and the robobun evidence block confirms the new test fails on main (msg2.len=0, msg3.len=0) and passes on the PR under both ASAN-debug and release builds.
Other factors
- No CODEOWNERS cover these paths.
- The only finding from the bug-hunting pass is a cosmetic nit (inline GUID literal vs. the module-scope
WEBSOCKET_GUIDconstant); the same file already inlines it elsewhere, so this is non-blocking. - I traced the libdeflate fast-path: it returns early before the zlib loop and never touches
decompress_stream, sosaw_stream_endstayingfalsethere is correct — the persistent zlib inflater's state is only relevant when the zlib path is actually taken. - Error returns inside the loop (
TooLarge,InflateFailed) skip the reset, but those paths close the connection with 1009/1002, so inflater state is moot. - The test wires
onerror/oncloseto reject the awaited promise, usesport: 0, cleans up intry/finally, and asserts exact byte lengths + content equality across both parameter values — it meets the harness conventions in CLAUDE.md. - The unrelated
cargo clippyfailure noted in the thread is innapi_body.rsfrom a different PR and not caused by this diff.
There was a problem hiding this comment.
LGTM — the earlier WEBSOCKET_GUID nit is addressed, and the remaining inline note is a pre-existing sibling case (npm ws truncates identically), not something this PR introduces or needs to block on.
Extended reasoning...
Overview
Two files: a 3-line logic change in src/http_jsc/websocket_client/WebSocketDeflate.rs (track Z_STREAM_END in the zlib decompress loop and include it in the post-loop reset() condition) plus a 4-line explanatory comment, and a ~105-line test.each in test/js/web/websocket/websocket-permessage-deflate-edge-cases.test.ts that hand-drives a raw TCP WebSocket handshake to send a BFINAL-terminated frame followed by two sync-flushed frames, covering both server_no_context_takeover values.
Security risks
None. The change only adds a case in which the persistent inflater is reset. Resetting is strictly safe — worst case it discards a sliding window the sender already abandoned by emitting BFINAL. No new input is parsed, no bounds change, and the MAX_DECOMPRESSED_SIZE bomb guard is untouched.
Level of scrutiny
Low-to-medium. The Rust change is a textbook zlib pattern (an inflater that returned Z_STREAM_END is terminal and must be inflateReset before reuse), and it mirrors what npm ws does on the same event. The condition is purely additive to the existing server_no_context_takeover reset, so no existing path loses its reset. The robobun gate confirms the new test fails on main (ASAN debug) and passes with the fix.
Other factors
- My prior review's only comment (reuse
WEBSOCKET_GUID) was applied in 47fe4b6 and the thread is resolved. - The one inline finding this run is explicitly pre-existing and non-blocking: intra-message BFINAL (RFC 7692 §7.2.3.5) still truncates because the loop
breaks onStreamEnd. That behavior predates this PR, npmwsunder Node behaves identically, and no known server emits it — reasonable to leave for a follow-up rather than widen scope here. - Test quality is good:
port: 0,onerror/onclosewired to reject, cleanup infinally, deterministic PRNG payload instead ofrandomBytes, and both negotiation variants covered. - The unrelated clippy failure the author noted is in
napi_body.rsfrom #34067, not this diff.
|
CI status: the new
Ready for review. |
2 participants
What does this PR do?
RFC 7692 §7.2.3 allows a permessage-deflate sender to end a DEFLATE stream with
BFINAL=1and begin a fresh stream for the next message.PerMessageDeflate::decompressbroke out of its zlib loop onZ_STREAM_ENDbut only reset the inflater whenserver_no_context_takeoverwas negotiated. With plain context takeover the persistent inflater stayed in the finished state, so every subsequent compressed message that reached the zlib path was silently delivered toonmessageas an empty payload, with no error and no close. The connection stayed open; the data was simply lost.The zlib path is reached when a message's decompressed output exceeds the 128 KiB libdeflate fast-path buffer, or when a sync-flushed message (no BFINAL) makes libdeflate fall through. So one large BFINAL-terminated message poisons the connection for the rest of its lifetime.
Repro
Before:
got 204800 bytes,got 0 bytes,got 0 bytesAfter:
got 204800 bytes,got 570 bytes,got 180 bytesnpm
wsunder Node: identical to "after".Fix
decompressnow resets the inflater wheneverZ_STREAM_ENDis observed, in addition to the existingserver_no_context_takeoverreset. This matches what npmwsdoes (it closes and recreates itsInflateRawon stream end).How did you verify your code works?
New
test.eachintest/js/web/websocket/websocket-permessage-deflate-edge-cases.test.tscovering both values ofserver_no_context_takeover. The context-takeover case fails on main withmsg2.len=0, msg3.len=0and passes with the fix. All existing permessage-deflate tests continue to pass.Server-side sibling: #33592 handles the analogous case in
Bun.serve's uWS decompressor.[review] gate passed · iteration 2 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 0 rejected · iteration 2
evidence per changed file