fix(binding-mqtt-kafka): guard doKafkaData against insufficient window budget - #2550
Merged
Merged
Conversation
…w budget KafkaSessionStream.doKafkaData (three overloads) called doData(kafka, ...) unconditionally, then asserted initialSeq - padding <= initialAck + initialMax after the fact. KafkaSessionStateProxy's own kafka producer stream (topic mqtt-sessions) gets its kafka field assigned synchronously as soon as doKafkaBeginIfNecessary runs, but initialMax stays at its Java default 0 until the merged Kafka binding's async partition/metadata DESCRIBE resolves and grants a real window. A client that aborts with its Will flag set during that gap reaches onMqttAbort -> sendWillSignal -> doKafkaData with a real (non-null) kafka stream but zero window, tripping the assert and crashing the entire engine worker via AgentTerminationException. Guard each doKafkaData overload with the same inequality the assert already expressed, checked before attempting the write instead of after. This subsumes the narrower kafka == null case for free (an unattached stream always has initialMax == 0) while also covering the more general, more reachable "attached but zero window" case. New coverage: session.will.message.clean.start.abort.zero.window grants the session's kafka stream a real window just large enough for the self-covering expiry-signal writes but too small for the will signal, then aborts. Verified red (reverting the fix reproduces the exact AssertionError/AgentTerminationException crash) then green, plus MqttIT/KafkaIT peer-to-peer self-consistency coverage. 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.
Description
KafkaSessionStream.doKafkaData(three overloads, inMqttKafkaSessionFactory) calleddoData(kafka, ...)unconditionally, then assertedinitialSeq - padding <= initialAck + initialMaxafter the fact.KafkaSessionStateProxy's own kafka producer stream (topicmqtt-sessions) gets itskafkafield assigned synchronously as soon asdoKafkaBeginIfNecessaryruns, butinitialMaxstays at its Java default0until the merged Kafka binding's async partition/metadata DESCRIBE resolves and grants a real window (seeKafkaMergedFactory.onMergedInitialBegin, which kicks offdescribeStream.doDescribeInitialBeginrather than synchronously granting credit). A client that aborts with its Will flag set during that gap reachesonMqttAbort→sendWillSignal→doKafkaDatawith a real (non-null)kafkastream but zero window, tripping the assert and crashing the entire engine worker viaAgentTerminationException— every connection on that worker goes down with it.The original community-reported fix (credit to @sfr-oc for identifying and reporting this in #2520) guarded only the narrower
kafka == nullcase (reachable viaEngineWorker.onClose's synthetic-abort sweep hitting a stream mid-registration). This PR instead guards eachdoKafkaDataoverload with the same inequality the existingassertalready expressed, checked before attempting the write instead of after. This subsumes thekafka == nullcase for free (an unattached stream always hasinitialMax == 0) while also covering the more general, more reachable "attached but zero window" case that the narrower guard didn't touch.New coverage:
session.will.message.clean.start.abort.zero.windowgrants the session's kafka stream a real window just large enough for the self-covering expiry-signal writes but too small for the will signal, then aborts. Verified red (reverting the fix reproduces the exactAssertionError/AgentTerminationExceptioncrash, killingengine/worker#0) then green, plusMqttIT/KafkaITpeer-to-peer self-consistency coverage. Fullruntime/binding-mqtt-kafkaandspecs/binding-mqtt-kafka.specmodule suites pass.Fixes #2520
🤖 Generated with Claude Code
https://claude.ai/code/session_015YVNaqKvEXZVzmg3HoGVnt
Generated by Claude Code