fix(binding-mqtt): stop reserving padding once the publish stream's window is exhausted - #2545
Merged
Merged
Conversation
…indow is exhausted MqttPublishStream.initialBudget() returns initialMax - (initialSeq - initialAck), unlike MqttSessionStream's version which also subtracts initialPad. decodePublishPayload computed the reserved size as Math.max(publisher.initialPad, Math.min(lengthMax + initialPad, initialBudget)), which floors at initialPad even once the window can no longer hold it. Once the granted window is exhausted, this kept emitting zero-length DATA frames that still reserved initialPad bytes, advancing initialSeq with no decode progress. Every subsequent WINDOW re-entered the same path and burned another initialPad, walking initialSeq past initialAck + initialMax. The downstream peer eventually sees sequence > acknowledge + maximum and RESETs, which propagates to onDecodeError with a reason code the MQTT v3.1.1 CONNACK encoding can't carry (reasonCode > MAX_CONNACK_REASONCODE_V4), so doNetworkEnd tears down the connection with no message sent to the client at all - the client just sees the TCP/TLS connection disappear. The threshold is bytes, not messages: it trips as soon as cumulative payload + padding first exhausts the granted initial window, so message count before the cutoff scales inversely with payload size - independent of any will-message handling. Compute the payload cap as Math.min(lengthMax, initialBudget - initialPad) and only proceed once it and the resulting claimed size are non-negative, so once the window is exhausted no frame is emitted and no forward progress is recorded - the stream just waits for the next WINDOW like every other backpressured decode path in this file. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Renamed the cherry-picked publish.many.messages scenario to publish.multiple.messages.100c, matching this suite's existing publish.multiple.messages family (.disconnect, .unfragmented, .with.delay) rather than introducing "many" as new vocabulary. The suffix introduces a "count" convention (Nc) distinct from this suite's established "size" convention (Nk, e.g. publish.10k, session.will.message.32k) -- a bare number would read as a byte size given that precedent holds without exception across the whole specs tree. Verified the existing publish.10k scenario does not itself already exercise this defect (passes unmodified on pre-fix code), so this is not a redundant scenario: the bug requires many small messages landing on a specific cumulative-budget boundary, not simply a large payload. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015YVNaqKvEXZVzmg3HoGVnt
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.
Fixes #2521
MqttPublishStream.initialBudget()returnsinitialMax - (initialSeq - initialAck), unlikeMqttSessionStream's version which also subtractsinitialPad.decodePublishPayloadcomputed the reserved size asMath.max(publisher.initialPad, Math.min(lengthMax + initialPad, initialBudget)), which floors atinitialPadeven once the window can no longer hold it.Once the granted window was exhausted, this kept emitting zero-length DATA frames that still reserved
initialPadbytes, advancinginitialSeqwith no decode progress. Every subsequent WINDOW re-entered the same path and burned anotherinitialPad, walkinginitialSeqpastinitialAck + initialMax. The downstream peer eventually sees sequence > acknowledge + maximum and RESETs, which propagates toonDecodeErrorwith a reason code the MQTT v3.1.1 CONNACK encoding can't carry, sodoNetworkEndtears down the connection with no message sent to the client at all — the client just sees the TCP/TLS connection disappear.The threshold is bytes, not messages: it trips as soon as cumulative payload + padding first exhausts the granted initial window, so message count before the cutoff scales inversely with payload size.
Fix computes the payload cap as
Math.min(lengthMax, initialBudget - initialPad)and only proceeds once it and the resulting claimed size are non-negative, so once the window is exhausted no frame is emitted and no forward progress is recorded — the stream just waits for the next WINDOW like every other backpressured decode path in this file.Credit to community contributor @sfr-oc, who diagnosed and fixed this as part of the combined #2523. This PR cherry-picks that fix's commit standalone (unmodified,
01745473), since #2523 bundles seven independently-scoped defects across three bindings into one PR — each deserves its own focused review, and this one is ready on its own.Test coverage
Includes the original commit's coverage, plus a naming-consistency rename in a separate commit: the new scenario is
publish.multiple.messages.100c(originallypublish.many.messages), matching this suite's existingpublish.multiple.messagesfamily (.disconnect,.unfragmented,.with.delay) rather than introducing new vocabulary. The100csuffix establishes a "count" convention (distinct from the suite-wide "size" convention, e.g.publish.10k) for reuse in future scenarios that vary message count rather than payload size.client.rpt+server.rpt) in bothspecs/binding-mqtt.spec'sapplication/andnetwork/v4/treesruntime/binding-mqtt'sserver/v4/PublishIT#shouldPublishMultipleMessages100c)Verification
AssertionErrorindoPublishData, then resource-leak and protocol-frame-corruption errors on retry) — matching the described sequence-walks-past-window mechanism — then restored the fix and confirmed the test passespublish.10kscenario: ran that test unmodified against the pre-fix code and it passes, since the defect requires many small messages landing on a specific cumulative-budget boundary, not simply a large single payload🤖 Generated with Claude Code
https://claude.ai/code/session_015YVNaqKvEXZVzmg3HoGVnt
Generated by Claude Code