fix(llc): fix skipped message retries due to premature queue removal #2308
Conversation
Previously, the RetryQueue listened to channel message events and removed messages from the queue when they were successfully sent. This introduced a race condition: when the retry loop reached `finally`, it would call `removeFirst()` assuming the message was still in the queue — but the event listener had already removed it. As a result, the next (unrelated) message would be incorrectly removed and skipped. This change removes the `_listenMessageEvents()` logic and ensures that message removal is done explicitly inside `_processQueue()` using `removeMessage(message)`. This guarantees that only the message currently being retried is removed. Additionally: - Optimized `add()` to filter out duplicates more efficiently. - Improved `containsMessage` and `removeMessage` using `unorderedElements`. - Fixed `_byDate()` comparator to ensure null dates are sorted to the bottom.
WalkthroughThe changes refactor the message retry queue logic by removing the event listener for message events, simplifying message filtering, updating how messages are removed from the queue, and improving the internal queue element lookup and removal methods. The message comparison logic for ordering has also been adjusted. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RetryQueue
participant MessageQueue
Client->>RetryQueue: Add message to retry queue
RetryQueue->>MessageQueue: Insert message (with new filtering)
loop On network reconnect or retry trigger
RetryQueue->>MessageQueue: Process next message (by date)
MessageQueue-->>RetryQueue: Provide message
RetryQueue->>Client: Attempt send
alt Send successful or max retries
RetryQueue->>MessageQueue: Remove specific message
end
end
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Suggested reviewers
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/stream_chat/lib/src/client/retry_queue.dart (1)
92-94: Critical fix for the race condition issue.This change correctly removes the specific message being processed instead of assuming it's the first in the queue, which prevents accidentally removing unrelated messages. This is the core fix for the reported issue.
Note: The
removeMessagemethod now returns a boolean, but the return value is not being checked here. Consider logging a warning if the removal fails for debugging purposes.} finally { // remove the message from the queue after it's handled. - _messageQueue.removeMessage(message); + if (!_messageQueue.removeMessage(message)) { + logger?.warning('Failed to remove message ${message.id} from retry queue'); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/stream_chat/lib/src/client/retry_queue.dart(4 hunks)
🔇 Additional comments (5)
packages/stream_chat/lib/src/client/retry_queue.dart (5)
51-51: Good refactoring to improve readability.The use of
whereNotwith the method reference makes the filtering logic more concise and declarative.
116-119: Correct adjustment to null date handling in the comparator.The inverted null handling now ensures that messages with null dates are sorted to the bottom of the priority queue, preventing them from blocking the processing of properly dated messages. This is a sensible improvement to the queue processing order.
135-143: Efficient implementation using unorderedElements.Good performance improvement by using
unorderedElementsinstead of converting to a list. The boolean return value also provides useful feedback about the operation's success.
145-151: Performance improvement for message lookup.Using
unorderedElementsavoids unnecessary list creation and provides better performance for the containment check.
14-17: Confirmed: Event listener removal prevents race condition.The removal of
_listenMessageEvents()from the constructor is the key fix for the race condition. By eliminating the event-driven message removal and relying solely on explicit removal in_processQueue(), the queue management is now deterministic and thread-safe.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2308 +/- ##
==========================================
+ Coverage 63.50% 63.55% +0.04%
==========================================
Files 409 409
Lines 25584 25571 -13
==========================================
+ Hits 16247 16251 +4
+ Misses 9337 9320 -17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/stream_chat/lib/src/client/retry_queue.dart(4 hunks)packages/stream_chat/lib/stream_chat.dart(1 hunks)packages/stream_chat/test/src/client/channel_test.dart(0 hunks)packages/stream_chat/test/src/client/retry_queue_test.dart(0 hunks)sample_app/lib/app.dart(1 hunks)
💤 Files with no reviewable changes (2)
- packages/stream_chat/test/src/client/retry_queue_test.dart
- packages/stream_chat/test/src/client/channel_test.dart
✅ Files skipped from review due to trivial changes (1)
- packages/stream_chat/lib/stream_chat.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/stream_chat/lib/src/client/retry_queue.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: stream_chat_flutter
- GitHub Check: stream_chat_localizations
- GitHub Check: stream_chat_persistence
- GitHub Check: stream_chat_flutter_core
- GitHub Check: test
- GitHub Check: analyze_legacy_versions
- GitHub Check: stream_chat
- GitHub Check: build (ios)
- GitHub Check: build (android)
- GitHub Check: analyze
3 participants
Submit a pull request
Fixes: #2106
Description of the pull request
Previously, the RetryQueue listened to channel message events and removed messages from the queue when they were successfully sent. This introduced a race condition: when the retry loop reached
finally, it would callremoveFirst()assuming the message was still in the queue — but the event listener had already removed it. As a result, the next (unrelated) message would be incorrectly removed and skipped.This change removes the
_listenMessageEvents()logic and ensures that message removal is done explicitly inside_processQueue()usingremoveMessage(message). This guarantees that only the message currently being retried is removed.Additionally:
add()to filter out duplicates more efficiently.containsMessageandremoveMessageusingunorderedElements._byDate()comparator to ensure null dates are sorted to the bottom.Screenshots / Videos
Screen_recording_20250717_022159.mp4
Screen_recording_20250717_021731.mp4
Summary by CodeRabbit
Summary by CodeRabbit