docs(server): say what the evict-on-full check actually tests - #1209
Merged
mykytanetipa merged 4 commits intoAug 31, 2026
Merged
Conversation
The eviction predicate is queue fullness at delivery time. The docstrings and the warning described it as an abandoned subscriber whose consumer stopped draining, which is a claim about the consumer that the check never makes: a consumer that is reading steadily but has fallen max_queue_size events behind satisfies the same predicate and is evicted the same way. Name the queue bound instead, and say explicitly that eviction does not imply the consumer stopped reading, so an operator reading the warning does not go hunting for a dead consumer and find a live one. No behavior change. Signed-off-by: Sankalp Gilda <sankalp.gilda@gmail.com>
🧪 Code Coverage (vs
|
mykytanetipa
requested changes
Aug 31, 2026
The warning fires once per eviction and can repeat under load, so it is now one line naming the condition and the consequence. The wording is the reviewer's own suggestion. The docstring paragraph says the eviction condition once instead of restating it for two consumer cases, and the comment at the tap call site is a single line naming the flag and pointing at the function that explains the mechanism.
Our own prose check flagged the negation-contrast in the previous wording. The comment now says what the flag achieves instead of what it avoids, which is shorter and reads the same way the rest of this module does.
mykytanetipa
approved these changes
Aug 31, 2026
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
2 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.
Follow-up to #1137, documentation only. No behavior change: the diff touches two docstrings, one comment and one log format string. Ruff's linter and formatter are both clean on the two files, and the 73 tests in the event-queue and active-task modules pass.
#1137 added eviction so one sink whose queue has filled cannot wedge the dispatcher for every other subscriber. The behavior is right and the words around it are not. The guard is
if sink._evict_on_full and sink.queue.full():, which tests queue fullness at delivery time and says nothing at all about the consumer. The docstring and the warning both described it as an abandoned subscriber whose consumer had stopped draining, which is a claim the check never makes. A consumer that is alive and reading steadily, merelymax_queue_sizeevents behind the dispatcher, satisfies the same predicate and is evicted the same way.That gap costs an operator real time. The warning as written sends someone looking for a dead consumer, and what they will find is a live one that is slow, with nothing in the message saying that is expected. So the warning now says the queue was full at delivery time and that the sink is therefore at least max_queue_size events behind, and it states outright that this does not mean the consumer stopped reading.
Alongside it, the delivery docstring gains one paragraph saying what the test is and what it is not, the evict_on_full parameter doc names the queue bound rather than abandonment, and the comment at the subscriber tap site says the same thing. Abandonment stays where it is genuinely the subject: the teardown docstring still describes a producer wedged closing an abandoned sink, and the regression test still calls its never-drained sink abandoned, because in both of those cases that is what is happening.