Fix listener re-delivery loop when returning null - #121
Merged
Conversation
OptimizingAppendListenerDecorator keeps delivering until the listener has reached the target it was notified about, and it learned that only from a non-null return. A listener returning null therefore left the loop with nothing to compare against and nothing to reach: it re-delivered the same target without pausing, measured at ~700.000 deliveries a second on one pinned virtual thread. That is not an exotic listener. Projector.eventsAppended returns run().lastEventReference(), which is null whenever the query matched no events -- so any subscribed projector whose event type had not yet occurred burned a core from the first unrelated append to its stream until the first matching one. Nothing threw and nothing was logged, and it cleared itself the moment a matching event arrived, which is presumably how it survived: it reads as load rather than as a bug. Null and a reference behind the target now mean the same thing, which is what they always meant semantically: caught up to the target. No notification is lost by that -- the next append carries a later reference, which is after this one and so still delivered. The interface has always documented the return as "never null" while Projector has always returned null, so the contract now gives null a defined meaning rather than leaving it to whoever reads more carefully. Pinned by a new scenario in AppendListenerFailureTest, alongside the containment #118 added. Verified in memory only -- no Docker here -- but the decorator sits above the storage and is shared by every backend. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TEqyLx6ahzpdfhog25K5PV
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.
Summary
Fix a performance issue in
OptimizingAppendListenerDecoratorwhere listeners returning null were re-delivered to indefinitely without pausing, causing CPU starvation. The decorator now treats null and references behind the target as equivalent to "caught up", stopping re-delivery until the next append.Changes
OptimizingAppendListenerDecorator.notifyDecoratedListener(): Changed the logic that advanceslastNotifiedReferenceto treat null returns the same as references behind the target — both now mean the listener is caught up to the target. Previously, null left the decorator with nothing to compare against, causing it to re-deliver the same target in a tight loop (~700,000 deliveries/second on a pinned virtual thread).EventStreamEventuallyConsistentAppendListenerinterface documentation: Updated the contract foreventsAppended()to document that null and references behindatLeastUntilboth mean "caught up", and that null is now a defined behavior rather than undefined. Added context thatProjector.eventsAppended()returns null when its query matches no events, making this a real-world issue for any subscribed projector whose event type hasn't occurred yet.AppendListenerFailureTest.testListenerReportingNoProgressIsNotRedeliveredTo(): Added a new TCK test that verifies a listener returning null is not re-delivered to excessively. The test uses a bystander listener to confirm the notification round trip completed, then asserts the null-returning listener received at most 10 deliveries for a single append (separating "restrained" from "spinning" behavior).CLAUDE.md: Documented the issue and fix in the project conventions, explaining the root cause, why it affects ordinary listeners likeProjector, and how the fix maintains backward compatibility.Implementation Details
The fix is minimal and safe: instead of only advancing
lastNotifiedReferencewhen the delegate returns non-null, it now always advances to at least the target. This works because:https://claude.ai/code/session_01TEqyLx6ahzpdfhog25K5PV