Repository navigation
fix(conversations): do not apply server responses older than the stored entry - #6852
ToteMeiSter wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to Older room responses cannot restore entries deleted by a newer full sync. No actionable issue remains from the reviewed evidence. Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 28.85% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 52 functions across 8 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
-
Check name Status Explanation Title check The title clearly and concisely describes the main change: preventing older conversation server responses from overwriting newer stored entries. Description check The description explains the problem, implementation, testing, manual verification, and checklist status. The omitted screenshots and TODO sections are non-critical, so the description is sufficiently… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
- Fix all pre-merge checks with AI
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
d567795e-8657-4b6d-87e9-ffce11ab0f64
📒 Files selected for processing (8)
app/src/main/java/com/nextcloud/talk/chat/data/network/ChatMessageSyncer.ktapp/src/main/java/com/nextcloud/talk/chat/domain/ChatPullResult.ktapp/src/main/java/com/nextcloud/talk/conversationlist/data/network/ConversationListUpdater.ktapp/src/main/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepository.ktapp/src/test/java/com/nextcloud/talk/conversationlist/data/network/ConversationListFreshnessIntegrationTest.ktapp/src/test/java/com/nextcloud/talk/conversationlist/data/network/ConversationListUpdaterPinTest.ktapp/src/test/java/com/nextcloud/talk/conversationlist/data/network/ConversationListUpdaterUnreadTest.ktapp/src/test/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepositoryTest.kt
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.
rapterjet2004
left a comment
rapterjet2004
left a comment
There was a problem hiding this comment.
LGTM, thanks. The ordering by request start, the per-account lock covering read, merge, write and mark, and the NonCancellable write + mark look right. Lock order is consistent (sync lock / catch-up mutex first, then the server write lock), and taking requestedAtNanos inside withRetry gives each attempt its own start.
Follow-up (pre-existing, outside this PR): a few local writers still do a read-modify-write of the whole conversation entry without the lock, so a sync that lands between their read and their write is reverted:
ChatViewModel.saveMessageDraftandsaveHiddenUpcomingEvent- the optimistic update and rollback in
ConversationsListViewModel(favorite, archive, ...) andConversationTagsViewModel, where the rollback writes back the completeoriginalentry
Field-level UPDATE queries for these (or taking the same lock) would close the same class of bug for local writes. Worth a separate issue.
…ed entry A room list sync and a single room refresh both replace the whole conversation entry. The one that was requested earlier but answered later overwrote the newer entry with stale activity, read state, call state, archive flag and so on, and a full sync that was requested before a room was refreshed deleted the room again. The pending local state guards only cover what the user changed, not the order of two server responses. Remember per room the start of the request behind the newest state written to its entry, and leave out of the merge, and out of the deletions, a response whose request started before that. The start is taken from System.nanoTime: it only orders requests inside the process. A background catch-up writes only the last message, its activity and the unread count, so it marks just that group of fields, with the start of the request that fetched the message. A response that is older than the group but newer than the entry applies, and keeps the stored values of the group. The unread count follows from the last read message: when the response moved the marker, the count is derived again from it, and taken from the response where that is not possible. Reading the stored entries, the merge, the write and the mark happen under one lock per account, and the write and the mark cannot be cancelled in between. Assisted-by: Claude-Code:claude-opus-5-5 Signed-off-by: Krainov Gleb <krajnov.g@kontentplus.ru>
A catch-up is a writer of the conversation entry like a sync or a room refresh, but it was not ordered against them by the start of its request. When a room refresh requested after the catch-up request was written first, the catch-up still ran its UPDATE, which compares only the last activity. A newer response can have a lower activity, for example after the newest message was deleted, and the catch-up then brought back the deleted preview and the unread count. Under the account lock, skip the catch-up when the entry holds state from a request started after the one that fetched its message, with the same isSuperseded check that drops an older response. Assisted-by: Claude-Code:claude-sonnet-5-5 Signed-off-by: Krainov Gleb <krajnov.g@kontentplus.ru>
0d39142 to
a223391
Compare
2 participants
Problem
The room list sync (
syncRoomList) and the single room refresh (getRoom) both replace the whole conversation entry (ConversationsDao.upsertConversations/syncConversationsForUser). Syncs of one account are serialized byroomListSyncLocks, butgetRoomdoes not take that lock and nothing compares the order of the two requests. A response that was requested earlier but written later therefore reverts the whole entry: last message, activity, read marker and unread count, call state, archive flag and so on. A full sync requested before a refresh also deletes a room the refresh has just written, and a late refresh brings back a room the full sync has deleted.The pending local state guards in
ConversationListUpdater.preservePendingLocalStateonly protect what the user changed locally, not the order of two server responses. This was raised by CodeRabbit in #6846; it exists on master independently of that PR.Change
ConversationListUpdaterremembers per room theSystem.nanoTime()start of the request behind the newest state written to the entry. A response whose request started earlier is left out of the merge and out of the deletions of a full sync.FieldGroup.PREVIEW), with the start of the request that fetched the message (ChatPullResult.Success.requestedAtNanos→SyncOutcome.newestMessageRequestedAtNanos). A response older than the group but newer than the entry still applies and keeps the stored values of the group. When such a response moved the read marker, the unread count is derived again from the new marker (samederiveUnreadMessagesCountas the catch-up); if it cannot be derived, the count of the response is used (the higher of the two when the marker went down).withServerWriteLock), and the write and the mark run inNonCancellable, so a time budget cannot cancel between them.Relation to #6846: that PR adds its own lock (
syncWriteLock) and an answer timestamp forlastCommonReadMessagefrom thePOST readresponse. With this change it becomes one more field group (FieldGroup.COMMON_READ) written underwithServerWriteLockand marked byrecordServerWrite. Whichever of the two is merged second will be adjusted accordingly.How to test
./gradlew :app:testGplayDebugUnitTest --tests "com.nextcloud.talk.conversationlist.data.network.ConversationListFreshnessIntegrationTest". With the new tests on unchanged master code 9 of them fail (refresh vs. sync in both orders, deletions by a full sync, catch-up vs. sync and refresh, delta sync); the remaining new tests guard against dropping too much. Removing the lock makes the lock test fail; removing the unread recount makes two tests fail.testGplayDebugUnitTest,detektandktlintCheckpass locally.🏁 Checklist
/backport to stable-xx.x🤖 AI (if applicable)
The change and the tests were prepared with AI assistance (Claude Code,
claude-opus-5-5).🤖 Generated with Claude Code