Chat message pre-fetching - #6454
Conversation
|
APK file: https://github.com/nextcloud/talk-android/actions/runs/30940554955/artifacts/8925256420 |
Move the fetch-and-persist core (getAndPersistMessages, persistChatMessagesAndHandleSystemMessages, updateBlocks) out of OfflineFirstChatRepository into a singleton ChatMessageSyncer that takes (user, roomToken, threadId) per call instead of relying on lateinit state set by ChatActivity. The repository delegates to it so open-chat and background paths share a single write path. Assisted-by: Claude Code:claude-fable-5 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
Add ChatMessageSyncer.catchUpRoom as entry point for syncing a room without an open chat: delta fetch from the newest locally known message when a chat block exists, or an initial fetch of the newest messages that creates the first chat block for never-opened rooms. Guard the chat block update against an empty persist result (conversation not yet in DB) instead of crashing. Move field map construction into the syncer. Assisted-by: Claude Code:claude-fable-5 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
Send markNotificationsAsRead=0 on background message fetches and gate the behavior on the chat-keep-notifications server capability, so a background sync neither moves the read marker (setReadMarker=0 is already sent) nor dismisses the user's push notifications. Assisted-by: Claude Code:claude-fable-5 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
After upserting conversations from GET /room, catch up messages of rooms whose lastActivity advanced since the last sync (matching the iOS behavior) and of unread rooms that have no cached messages yet. The catch-up runs via ChatMessageSyncer.catchUpRoom after the room list was emitted, so unread messages are already in the local database when a chat is opened. Assisted-by: Claude Code:claude-fable-5 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
Limit the room-list message prefetch to the 20 most recently active rooms with at most 3 concurrent requests, and skip it entirely in battery saver mode or when background data is restricted on a metered network — mirroring the Low Power Mode guard on iOS. Assisted-by: Claude Code:claude-fable-5 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
Skip the initial newest-100 request when cached messages already reach the conversation's lastMessage.id, and replace the forced full fetch on chat-relay servers with a delta fetch from the newest cached message. This keeps the relay path's backlog guarantee while making chat open network-free after a successful prefetch. Assisted-by: Claude Code:claude-fable-5 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
Start loading messages immediately when a chat is opened and decide the live-update mode (chat relay vs long polling) in a parallel coroutine once the websocket state is known. The backlog delta fetch is now made regardless of the mode — required for chat relay, and on long-polling servers it only front-loads what the first poll request would have fetched — so loadInitialMessages no longer needs to know about chat relay at all. Assisted-by: Claude Code:claude-fable-5 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
After displaying a message notification, trigger a single-room catch-up so the pushed message and any backlog are persisted to the local database while the app is backgrounded. Best effort only: failures never delay or suppress the notification. Skipped without the chat-keep-notifications capability or in battery saver. Assisted-by: Claude Code:claude-fable-5 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
Skip deleteLeftConversations when GET /room unexpectedly returns no conversations while some exist locally. A broken or partial server response would otherwise delete every local conversation and, via foreign key cascade, wipe the cached chat messages and chat blocks that the message prefetch relies on. Assisted-by: Claude Code:claude-fable-5 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
Trim chat block boundaries to the oldest/newest message that still exists after deleteExpiredMessages and delete blocks whose messages are all gone, so block boundaries never point to rows that no longer exist. The cleanup moved into ChatMessageSyncer so future background callers share it. Assisted-by: Claude Code:claude-fable-5 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
Replace the in-memory latestKnownMessageIdFromSync with the newest message id from the chat blocks. The field lived in the unscoped repository and was reset to zero on every chat open, so an insurance request or signaling-triggered refresh running before the first successful sync of the session queried with lastKnownMessageId=0. The database is always at least as fresh because messages and chat blocks are persisted together. Assisted-by: Claude Code:claude-fable-5 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
Add unit tests for ChatMessageSyncer: field map safety flags, offline and capability gates of catchUpRoom, delta fetch for rooms with a chat block, initial fetch with block creation for never-opened rooms, merging of connected chat blocks, the not-modified and constraint violation paths, and chat block reconciliation after message expiry. Assisted-by: Claude Code:claude-fable-5 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
Assisted-by: Claude Code:claude-fable-5 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
Assisted-by: Claude Code:claude-fable-5 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
6492492 to
569655c
Compare
Run at most one catch-up per room at a time: requests arriving while one runs only mark a rerun that the running catch-up executes after finishing, consecutive fetches are paced by a five second cooldown and a burst performs at most three fetches. A flood of push notifications for an active room now causes one or two delta fetches instead of one per push. Assisted-by: Claude Code:claude-fable-5 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
mahibi
left a comment
There was a problem hiding this comment.
Most of it works really good and it's really promising 👍
However there are some major problems with the chatrelay and insurance request (see my comments).
These are edge cases what won't happen too often, but they would create permanent gaps.
For now i just identified this by code reviewing (not yet reproduced by testing).
Fixing it should not be too hard, might be just reverting some lines..
I did not finish the code review yet, so there might be more on monday (wont finish it today).
| val target = ChatMessageSyncer.SyncTarget( | ||
| user = currentUser, | ||
| roomToken = roomToken, | ||
| threadId = null, |
There was a problem hiding this comment.
threadId must not be null.
Try to apply
parseThreadId(ncNotification.objectId)
| @@ -0,0 +1,87 @@ | |||
| /* | |||
There was a problem hiding this comment.
this class accidentally made it into this PR?
| timeout = 0, | ||
| includeLastKnown = false, | ||
| lastKnown = latestKnownMessageIdFromSync.toInt(), | ||
| lastKnown = newestMessageIdFromDb.toInt(), |
There was a problem hiding this comment.
Replacing latestKnownMessageIdFromSync with newestMessageIdFromDb is a major problem.
newestMessageIdFromDb can't be used because it will destroy the insurance request. The insurance request has to know the lastMessage from the last sync (omitting the messages that came in via signaling!).
But chatBlocksDao.getNewestMessageIdFromChatBlocks will include the messages that came in via signaling. So the insurance request will load messages newer than the last known one from signaling -> This will create persistent gaps!
To understand this also see the comment in onSignalingChatMessageReceived:
// we assume that the signaling messages are on top of the latest chatblock and include them inside it.
// If for whatever reason the assumption was not correct and there would be messages in between, the
// insurance request should fix this by adding the missing messages and updating the chatblocks.
| } | ||
|
|
||
| weAlreadyHaveSomeOfflineMessages && weHaveAtLeastTheLastReadMessage -> { | ||
| // Close the backlog since the newest offline message with a delta fetch. This is |
There was a problem hiding this comment.
This is a major problem:
it reads like the gap will definitely be closed but keep in mind that only happens when the message limit is not exceeded. So if the gap is >limit it narrows the gap but it will not close it.
So what happens when the gap is higher than the limit..:
-
For long polling this is not a problem. Long polling will just load all messages until it reached the newest, so it "self heals".
-
But for chat relay this is a problem:
- combined with the change in line 353 (replacing latestKnownMessageIdFromSync with newestMessageIdFromDb) this will immediately create permanent gaps when a signaling message comes in before the gaps are closed.
- but even without replacing latestKnownMessageIdFromSync with newestMessageIdFromDb this could result in 2 minute stop and go fetching by the insurance request until the newest message is reached.
- combined with the change in line 353 (replacing latestKnownMessageIdFromSync with newestMessageIdFromDb) this will immediately create permanent gaps when a signaling message comes in before the gaps are closed.
So in short, the weAlreadyHaveSomeOfflineMessages && weHaveAtLeastTheLastReadMessage -> { block might have to be reverted or modified.
ChatMessageSyncer: fetch-and-persist core extracted fromOfflineFirstChatRepositoryinto a stateless singleton usable without an open chat; both paths share one write path.lastActivity(matching iOS) and unread rooms without cached messages get a background catch-up - delta fetch, or initial fetch creating the first chat block for never-opened rooms (beyond iOS). Capped at 20 rooms / 3 concurrent; skipped in battery saver / restricted background data.markNotificationsAsRead=0, read marker untouched, gated on thechat-keep-notificationscapability.lastMessage.id; proportional delta fetch otherwise; initial load never waits for the websocket./roomresponses no longer cascade-delete the cache; insurance fetches anchor on the DB.How to verify: check logcat -> expect "Catching up messages for N rooms", per-room "Background catch-up … fetched N message(s)", and the "Initial online request is skipped … until the conversation's last message" line on open. Most visible on cold opens from push notifications and throttled networks. (for logging, see commit 64a3c85). Alternatively, deploy it on the phone, wait for a while, put the phone in flight-mode, then open the chats. You should then see the unread messages.
Assumptions to be aware of
markNotificationsAsRead=0server semantics - assumed the server keeps push notifications for background-fetched messages wheneverchat-keep-notificationsis announced; taken from iOS usage, not server code. If wrong, prefetching silently dismisses notifications. Most critical - please confirm.lastMessage.idfrom the room list is compared against local chat-block IDs; divergence via the federation proxy could wrongly skip the initial fetch (unread tail delayed to polling) or fetch redundantly (harmless). Needs a federated-room test.GET /chatlong polling still works against HPB servers - the fallback when the websocket is slow/mis-detected; pre-existing behavior, unverified server-side.lastActivityadvances for new chat messages and its persisted copy is a valid cross-restart baseline; missed candidates degrade to pre-PR open behavior, non-message bumps cost one empty delta.pushMessage.idis always a fetchable room token for TYPE_CHAT pushes (incl. federated/invite edge cases); wrong token costs one failed, caught request; the notification is unaffected.NotificationWorkerlifetime accommodates the synchronous catch-up - notification dispatched first, but ordering isn't formally guaranteed; an expedited separate work item is the fallback design.getConnectedChatBlocksmerging + insurance requests are the intended self-healing, without formal proof. Transaction boundaries deserve review.CapabilitiesWorkerwindow; a server downgrade inside it exposes assumption 1's impact until refresh.CHAT_API_VERSION = 1hardcoded in prefetch/push paths, mirroringChatViewModel; compile-time visible, low risk.🚧 TODO
🏁 Checklist
/backport to stable-xx.x🤖 AI (if applicable)