Skip to content

fix(binding-mqtt): bound will-message continuation read to the declared payload length - #2534

Merged
jfallows merged 1 commit into
developfrom
fix/2522-mqtt-will-message-continuation-bound
Sep 4, 2026
Merged

fix(binding-mqtt): bound will-message continuation read to the declared payload length#2534
jfallows merged 1 commit into
developfrom
fix/2522-mqtt-will-message-continuation-bound

Conversation

@jfallows

@jfallows jfallows commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Fixes #2522

onDecodeConnectWillPayload's continuation branch (used when a will message must be split across multiple frames because the session stream's window is too small to hold it in one write) bounded the network buffer read using the raw decode-window limit instead of willPayloadDeferred, the actual number of will-payload bytes still outstanding. When the decode buffer held more bytes than that (e.g. a pipelined control packet immediately behind the CONNECT), those extra bytes were forwarded downstream as if they were part of the will payload — producing a session-state record longer than its own declared deferred length, which Kafka rejects as CORRUPT_MESSAGE, and desynchronizing the network decoder's own progress tracking. Also fixes a latent out-of-bounds read in the first-chunk branch using the wrong accessor as the copy length.

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, 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 applied here: the new scenario is session.will.message.disconnect.while.deferred (originally session.will.message.deferred.trailing.packet), matching this file's existing <event>.while.<state> naming template (e.g. session.will.message.abort.while.deferred) rather than introducing new vocabulary.

  • New paired k3po scripts (client.rpt + server.rpt) in both specs/binding-mqtt.spec's application/ and network/v5/ trees
  • Spec-level self-consistency IT for each pair (specs/binding-mqtt.spec's application/SessionIT and network/v5/SessionIT)
  • Runtime IT against a live engine (runtime/binding-mqtt's server/v5/SessionIT#shouldDisconnectWhileDeferred)

Verification

  • Reverted just the production fix and confirmed the IT fails for the right reason: ComparisonFailure on the exact-match of the deferred remainder, then TestTimedOutException (matching the corrupted-boundary/decoder-desync description above) — then restored the fix and confirmed all three IT variants (runtime + both spec-level pairs) pass
  • Full reactor build compiles clean

🤖 Generated with Claude Code

https://claude.ai/code/session_015YVNaqKvEXZVzmg3HoGVnt


Generated by Claude Code

…ed payload length

onDecodeConnectWillPayload's continuation branch (used when the will
message must be split across multiple frames because the session
stream's window is too small to hold it in one write) wrapped the
network buffer using the raw decode-window limit instead of bounding
it to willPayloadDeferred, the actual number of will-payload bytes
still outstanding. When the decode buffer holds more bytes than that
(e.g. a pipelined control packet immediately behind the CONNECT, or
any other field arriving in the same read), those extra bytes were
forwarded downstream as if they were part of the will payload,
producing a session-state record longer than its own declared
deferred length -- which Kafka rejects as CORRUPT_MESSAGE -- and
desynchronizing the network decoder's own progress tracking.

Bound the continuation read the same way the first-chunk branch is
already bounded: by available network bytes, the session window, and
what's actually left of the declared will payload. Also fixes a
latent out-of-bounds read in the first-chunk branch, where the wrong
accessor was used as the copy length.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Claude-Session: https://claude.ai/code/session_015YVNaqKvEXZVzmg3HoGVnt

jfallows commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

The testing (mcp.proxy) check is failing here, but it's not this PR's failure — it's red on develop HEAD itself, independent of this diff:

This lines up with those two binding-mcp commits changing south-exit capability-handshake and MCP session-lookup behavior — plausibly a real regression, not flakiness — but it's unrelated to binding-mqtt's will-message continuation-read fix here, and root-causing it is out of scope for this PR. No fix is being ported in since none exists yet for this failure; flagging it here rather than widening this PR's scope.


Generated by Claude Code

@jfallows
jfallows merged commit e661b89 into develop Sep 4, 2026
123 of 125 checks passed

jfallows commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Update: root-caused and fixed the mcp.proxy regression in #2539 (npm-registry fetch racing the everything service's healthcheck window — see that PR for the full writeup; it was unrelated to binding-mcp despite the timing correlation I noted above). Ported the same fix into this branch and pushed — should be green now once CI re-runs.


Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Will message continuation read is not bounded by the declared payload length

2 participants