fix(binding-kafka): emit a window when a produce flush is acknowledged - #2547
Merged
Conversation
onClientInitialFlush advanced initialAck itself before asking doClientInitialWindow to publish it. That method only writes a WINDOW frame when it observes the acknowledge or the maximum change, and it derives the new acknowledge from the caller's noAck: with initialAck already advanced, initialSeq - noAck resolves back to the current initialAck, so the guard never fires and the frame is never written. The credit is released internally and never reaches the sender, whose view of the window shrinks by every flush until it reaches zero. Pass the outstanding byte count without pre-applying it, so doClientInitialWindow advances the acknowledge and emits the frame, and keep the maximum from regressing below the one already advertised. With a zero-reserved flush this resolves to the previous behaviour. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The zilla k3po test harness's advisory FLUSH write always emitted a FlushFW with reserved left at its default of zero, regardless of the connect/accept side's configured padding. A binding receiving that frame therefore never sees the nonzero-reserved case a real upstream binding produces when it flushes a produce entry with no payload, leaving that code path untestable from a .rpt script. Mirror the DATA write path: derive reserved from the channel's own padding/window accounting via reservedBytes(0), include it on the FlushFW, and advance the channel's written-bytes bookkeeping so subsequent writes see the consumed budget. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015YVNaqKvEXZVzmg3HoGVnt
KafkaCacheClientProduceFactory's produce entry point accepts either a DATA or a FLUSH frame carrying reserved bytes on the initial direction, but no scenario exercised the FLUSH side of that window accounting. Add message.value.after.flushes: two produce flushes each consuming half the configured window (via padding), followed by a small message value. Requires the fix in the prior commit to emit a window when a produce flush is acknowledged -- without it the second flush leaves no budget for the message, and the client blocks until the test times out. Fixes #2517. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015YVNaqKvEXZVzmg3HoGVnt
…d option The prior commit derived reserved bytes for every advisory FLUSH write from the channel's configured padding unconditionally. That changed observable behavior for every existing "write advise zilla:flush" script repo-wide -- including ones relying on padding learned from a real WINDOW rather than an explicit option -- and broke binding-sse's AdvisoryIT#shouldFlushResponse, whose flush had never carried reserved bytes before. Replace the always-on behavior with an explicit "paddable" option (space-separated frame-kind tokens: "data", "flush") defaulting to "data" only, matching every existing script's implicit behavior. Padding on setting "flushPadding" as a separate boolean concept would invert the directional intent of the existing padding option, so fold it into one generalized, opt-in mechanism instead. Our new message.value.after.flushes scenario opts in with "option zilla:paddable \"data flush\""; every other scenario keeps the default and is unaffected. Also apply the same isPaddable(DATA) gate to the DATA write path for symmetry, though no existing script can currently disable it. 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
3 participants
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.
Description
Fixes #2517
KafkaCacheClientProduceFactory$KafkaCacheClientProduceStream.onClientInitialFlushadvancedinitialAckby the flush'sreservedbytes itself before askingdoClientInitialWindowto publish it. That method only writes a WINDOW frame when it observes the acknowledge or the maximum change, and it derives the new acknowledge from the caller'snoAck: withinitialAckalready advanced,initialSeq - noAckresolves back to the currentinitialAck, so the guard never fires and the frame is never written. The credit is released internally and never reaches the sender, whose view of the window shrinks by every flush until it reaches zero.This is the underlying cause behind the MQTT QoS0-publish-through-mqtt-kafka failure reported in #2523 (a produce entry can be committed via a FLUSH carrying no payload, e.g. for a retained-message tombstone or a bare commit point — every one of those silently starves the sender's window).
Fix passes the outstanding byte count without pre-applying it, so
doClientInitialWindowadvances the acknowledge and emits the frame, and keeps the maximum from regressing below the one already advertised. With a zero-reserved flush this resolves to the previous behaviour.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,
9d013389), 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 k3po test harness had no way to author an application-level FLUSH frame with nonzero
reservedbytes —write advise zilla:flushalways emittedreserved=0regardless of any configured padding, since that path never consulted it (unlike the DATA write path). That made this fix's own regression untestable from a.rptscript.Rather than special-case FLUSH, added a generalized, opt-in
paddableconnect/accept option (space-separated frame-kind tokens, e.g."data flush") that controls which frame kinds derive their reserved bytes from the channel's padding accounting — defaulting to"data"only, matching every existing script's implicit behavior, so no other scenario changes semantics. The DATA write path now honors the sameisPaddable(DATA)gate for symmetry.New scenario
message.value.after.flushes(runtime/binding-kafka'sCacheProduceIT#shouldSendMessageValueAfterFlushes): two produce flushes, each consuming half the configured window via padding, followed by a small message value that only fits once the fix restores the window after each flush.client.rpt+server.rpt) inspecs/binding-kafka.spec'sstreams/application/produce/message.value.after.flushes/ZillaFrameKindenum,paddableoption plumbed throughZillaTypeSystem,ZillaChannelConfig,DefaultZillaChannelConfig,DefaultZillaServerChannelConfig, andZillaTargetVerification
binding-kafkaandbinding-sseIT suites (the latter to catch any behavior change from the harness's own advisory-flush path) — both clean./mvnw clean install, all ITs) compiles and passes clean🤖 Generated with Claude Code
https://claude.ai/code/session_015YVNaqKvEXZVzmg3HoGVnt
Generated by Claude Code