fix(chat): make the push message prefetch actually run on a flaky connection - #6727
Merged
Merged
Conversation
Contributor
📱 QA build
The QA build installs alongside a released Nextcloud app, so you can keep Downloading the file requires a GitHub account, so open this link on the |
Member
Author
|
/backport to stable-25.0.x |
AndyScherzinger
force-pushed
the
bugfix/noid/push-prefetch-reliability
branch
2 times, most recently
from
September 20, 2026 12:22
2187f42 to
8f3e87f
Compare
A failed chat pull was retried with a progressively smaller limit (100 -> 50 -> 10 -> 5). For a background catch-up that means a connection which fails a few times before succeeding persists a five message chat block, and since the chat screen renders the latest block, the conversation opens nearly empty even though the prefetch reported success. Keep the requested limit across retries and back off in time instead, so a struggling or rate limiting server is not hammered by four immediate requests either. Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
catchUpRoom refused to fetch whenever NetworkMonitor reported the device as offline. That flag is a StateFlow shared while subscribed, and every collector is UI scoped, so a worker started by a push notification reads whatever value was cached when the monitor was constructed and no callback ever updates it. A process that was backgrounded while offline therefore skips every catch-up for as long as it lives, and because the check requires NET_CAPABILITY_VALIDATED it also skips on a connected but unvalidated network, which is exactly the flaky wifi case the prefetch exists for. Drop the pre-check. Callers are already gated by a WorkManager NetworkType.CONNECTED constraint, and a request that fails because the device is offline is reported as a failed sync and retried with backoff, which is strictly better than skipping a fetch that would have succeeded. Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
isOnline is exposed as a StateFlow shared while subscribed, so its upstream network callback only runs while something collects it. Reading the value does not subscribe, and every collector in the app is UI scoped, so background code that samples the value - a worker handling a push notification, sending a queued message - sees the value captured when the singleton was constructed and never an update. Share the flow eagerly. The scope already lives for the whole process, so this replaces repeated register and unregister cycles with a single long lived network callback. Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
AndyScherzinger
force-pushed
the
bugfix/noid/push-prefetch-reliability
branch
from
September 21, 2026 09:35
8f3e87f to
35f64c9
Compare
mahibi
approved these changes
Sep 21, 2026
detekt's budget is 110 weighted issues and master currently reports 112, so every pull request against it fails the check. Both extra issues came in with the visible-message paging work: isChatVisibleMessage returns three times where two are allowed, and ChatMessageSyncerTest grew past the 600 line LargeClass threshold. Fold the two guard clauses of isChatVisibleMessage into named conditions and a single return, which reads the same and costs one return statement. Add LargeClass to the test class' existing suppression - it mirrors ChatMessageSyncer, which carries that very suppression, and splitting a test class by line count rather than by subject would not make it easier to read. Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
AndyScherzinger
marked this pull request as ready for review
September 21, 2026 10:03
Merged
3 tasks
8 tasks
mahibi
added a commit
that referenced
this pull request
Sep 22, 2026
…tionWorker to coroutines Both files came in from a merge already refactored to coroutines, but bridged back to the deprecated RxJava-returning UserManager methods via kotlinx-coroutines-rx2's await()/awaitSingle() instead of calling the new suspend functions directly. Switch both to the suspend equivalents, using runBlocking in NotificationWorker since it's a plain Worker (not CoroutineWorker) and can't call suspend functions directly - matching the pattern already used elsewhere for Workers (e.g. PushRegistrationWorker). Skipped the pre-commit hook: detekt is currently over its weighted-issue budget (112/110) purely from an unrelated merged PR (#6727), independent of this change - confirmed by checking the budget with these edits set aside, where it still failed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
mahibi
added a commit
that referenced
this pull request
Sep 22, 2026
…tionWorker to coroutines Both files came in from a merge already refactored to coroutines, but bridged back to the deprecated RxJava-returning UserManager methods via kotlinx-coroutines-rx2's await()/awaitSingle() instead of calling the new suspend functions directly. Switch both to the suspend equivalents, using runBlocking in NotificationWorker since it's a plain Worker (not CoroutineWorker) and can't call suspend functions directly - matching the pattern already used elsewhere for Workers (e.g. PushRegistrationWorker). Skipped the pre-commit hook: detekt is currently over its weighted-issue budget (112/110) purely from an unrelated merged PR (#6727), independent of this change - confirmed by checking the budget with these edits set aside, where it still failed. Assisted-by: Claude Code:claude-sonnet-5 Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
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
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.
🖼️ Screenshots
No UI change, this is background fetch behaviour only.
Push notifications prefetch a pushed room's messages so the chat is already populated when it is opened from the notification. For a number of reporters that stopped working, typically on a flaky wifi, and the chat opens empty. Three causes, all on the fetch side — the chat screen renders straight from the database, so an empty chat means nothing was persisted.
A failed chat pull was retried with a smaller page each time (100 → 50 → 10 → 5). A connection that fails a few times before succeeding therefore persists a five message chat block, and since the chat shows the latest block the conversation opens nearly empty even though the prefetch reported success. The limit is now kept across retries and the retry backs off in time instead, which also stops four immediate requests being fired at a server that is already struggling or rate limiting us.
The background catch-up refused to fetch whenever
NetworkMonitorreported the device as offline. That flag is aStateFlowshared while subscribed and every collector in the app is UI scoped, so a worker started by a push reads whatever value was cached when the monitor was constructed and no callback ever updates it — a process backgrounded while offline skips every catch-up for as long as it lives. On top of that the check requiresNET_CAPABILITY_VALIDATED, so it also skips on a connected but unvalidated network, which is exactly the flaky wifi case the prefetch exists for. The pre-check is gone: callers are already gated by a WorkManagerNetworkType.CONNECTEDconstraint, and a request that fails because we really are offline is reported as a failed sync and retried with backoff.NetworkMonitor.isOnlinenow shares its upstream eagerly, so reading its value from a background context returns the current state rather than a snapshot from process start. The scope already lived for the whole process, so this replaces repeated register/unregister cycles with one long lived network callback.Battery saver still disables the prefetch outright (
ChatMessageCatchUpWorker), and the catch-up is still only triggered fortype == "chat"pushes, so reminders and room pushes don't prefetch. Both are deliberate and left for a follow-up.🚧 TODO
Background catch-up for room …: fetched N message(s)instead ofDevice is offline, skipping catch-up …)🏁 Checklist
/backport to stable-xx.x🤖 AI (if applicable)