Skip to content

fix(binding-mqtt-kafka): account for the retained stream when granting the publish window - #2548

Merged
jfallows merged 3 commits into
developfrom
fix/2518-mqtt-kafka-retained-stream-publish-window
Sep 5, 2026
Merged

fix(binding-mqtt-kafka): account for the retained stream when granting the publish window#2548
jfallows merged 3 commits into
developfrom
fix/2518-mqtt-kafka-retained-stream-publish-window

Conversation

@jfallows

@jfallows jfallows commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Description

Fixes #2518

When a route has a retained topic configured, MqttKafkaPublishFactory maintains two downstream Kafka streams per publish connection: messages (the main topic — every publish goes here) and retained (the retained-messages topic — even a non-retained publish still issues a doKafkaFlush on this stream carrying the message's full reserved bytes, so its budget shrinks in lockstep with messages). doMqttWindow combines both streams' windows into the one granted upstream to the MQTT client, and had two independent defects:

  1. Wrong gate. The retained-aware branch was guarded by hasPublishFlagRetained(publishFlags) — a per-message flag, true only while a message carrying RETAIN is being processed and reset after every FIN. In steady state that branch was effectively dead code. Fixed to gate on retainAvailable, a per-stream capability decided once at BEGIN, so the retained stream's budget is factored in for the stream's whole life.

  2. Wrong arithmetic. Even when reachable, the computation was min(messages.ack, retained.ack) paired with max(messages.max, retained.max). But messages holds its ack pinned at zero and grows its max, while retained advances its ack against a fixed max — two different representations of "room left". Minimizing ack and maximizing max independently combines the ack of one stream with the max of the other, describing neither stream's actual budget, and (since retained.max never changes) permanently caps the connection's lifetime publishable bytes once it exceeds messages.max, rather than bounding bytes in flight. Fixed to compute each stream's own available budget uniformly as max - (seq - ack), take the smaller of the two, and express it against the chosen max via a monotonic ack.

Credit to community contributor @sfr-oc, who diagnosed and fixed this as part of the combined #2523. This PR cherry-picks both commits standalone (unmodified, 98148a89 and 208f10f6), 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

The first commit's own scenario (publish.many.messages.retain.available) still passed with the second commit reverted: both messages and retained are accepted children of the same accept "zilla://streams/kafka0" in the k3po test stub, so they always share the identical declared window and messages.max can never diverge from retained.max. I can show algebraically that the pre-fix formula reduces to the exact same expression as the fix whenever max_m == max_r, regardless of how far the ack values diverge — so the defect was structurally unreachable from that scenario as written.

Reproduced the divergence using existing k3po options, no new harness code: option zilla:update "handshake" on the shared accept stops the automatic acknowledge a STREAM-mode DATA read would otherwise trigger, so messages' own ack stays pinned at its initial handshake value once its max is explicitly grown via read option zilla:window. retained's flush-triggered acknowledge is unconditional regardless of update mode, so it keeps advancing normally against its original, unchanged max — reproducing exactly the "messages holds ack, grows max; retained advances ack, fixed max" mismatch the second commit's own description names.

  • Modified scenario: specs/binding-mqtt-kafka.spec's streams/kafka/publish.many.messages.retain.available/server.rpt (2 additions, no new scenario needed — the existing runtime IT MqttKafkaPublishProxyIT#shouldPublishManyMessagesRetainAvailable and self-consistency KafkaIT#shouldPublishManyMessagesRetainAvailable both exercise it)

Verification

  • Reverted just the second commit and confirmed the runtime IT now fails for the right reason: it times out publishing partway through (the pre-fix budget understates the real combined credit until the client can no longer make progress) — then restored the fix and confirmed it passes in ~1.3s
  • Confirmed the self-consistency KafkaIT scenario still passes independent of the engine, so the two scripts remain mutually consistent
  • Ran the full binding-mqtt-kafka and binding-mqtt-kafka.spec IT suites — both clean

🤖 Generated with Claude Code

https://claude.ai/code/session_015YVNaqKvEXZVzmg3HoGVnt


Generated by Claude Code

sfr-oc and others added 3 commits September 5, 2026 01:18
…g the publish window

A publish proxy on a route that has a retained topic configured charges the
retained stream a full reserved on every non-retained PUBLISH, because
onMqttData issues a flush on it for each message that does not carry the
retain flag. The window granted upstream, however, was derived from the
messages stream alone: doMqttWindow guarded the retained-aware computation
with hasPublishFlagRetained, a per-message flag that onMqttData resets after
every FIN, leaving the branch unreachable in steady state. The retained
stream's sequence could therefore advance past the window it was granted,
and its credit never constrained what the client was allowed to send.

Gate the computation on retainAvailable, the per-stream capability that
actually decides whether the retained stream is charged.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… publish window

The messages and retained streams advertise credit in different shapes: the
messages stream holds its acknowledge at zero and grows its maximum, while
the retained stream advances its acknowledge against a fixed maximum.
Minimizing acknowledge and maximum independently across the two therefore
pairs the acknowledge of one with the maximum of the other, describing
neither stream's actual budget, and caps the window offered upstream at a
fixed maximum whose acknowledge never moves - a limit on the total bytes a
connection can publish rather than on bytes in flight.

Compare the budget each stream offers and express the smaller of the two
against the chosen maximum, keeping the acknowledge monotonic.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…een messages and retained

The prior commit's own scenario (publish.many.messages.retain.available)
still passes with the second commit reverted: both streams share the k3po
stub's declared window, so messages.max and retained.max can never diverge
under the scenario's default flow control, and the fix is a no-op whenever
they're equal (provably: min(max_m,max_r) - seq + min(ack_m,ack_r) reduces
to the same expression on both sides of the fix when max_m == max_r,
regardless of how far ack_m/ack_r diverge).

Reproduce the divergence using existing k3po options rather than new
harness code: `option zilla:update "handshake"` on the shared accept stops
the auto-acknowledge that a STREAM-mode DATA read would otherwise trigger,
so messages' own ack stays pinned at its initial handshake value once we
explicitly grow its max via `read option zilla:window` -- retained's
flush-triggered auto-acknowledge is unconditional regardless of update mode,
so it keeps advancing its ack against its original, unchanged max. This
reproduces "messages holds ack, grows max; retained advances ack, fixed
max" -- the exact mismatch the second commit's own description names --
without needing a second address, a second accept, or any new option.

Reverting just the second commit now times out publishing partway through:
the pre-fix budget mixes messages' pinned ack with retained's smaller
fixed max, understating the real combined budget until the client can no
longer make progress. Restoring the fix keeps the scenario passing.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015YVNaqKvEXZVzmg3HoGVnt
@jfallows
jfallows merged commit 1a816b1 into develop Sep 5, 2026
43 checks passed
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.

Publish window granted upstream ignores the retained stream's credit

3 participants