Skip to content

fix(chat): make the push message prefetch actually run on a flaky connection - #6727

Merged
AndyScherzinger merged 4 commits into
masterfrom
bugfix/noid/push-prefetch-reliability
Sep 21, 2026
Merged

AndyScherzinger merged 4 commits into
masterfrom
bugfix/noid/push-prefetch-reliability

Conversation

@AndyScherzinger

@AndyScherzinger AndyScherzinger commented Sep 19, 2026

Copy link
Copy Markdown
Member

🖼️ 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 NetworkMonitor reported the device as offline. That flag is a StateFlow shared 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 requires NET_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 WorkManager NetworkType.CONNECTED constraint, and a request that fails because we really are offline is reported as a failed sync and retried with backoff.

NetworkMonitor.isOnline now 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 for type == "chat" pushes, so reminders and room pushes don't prefetch. Both are deliberate and left for a follow-up.

🚧 TODO

  • Confirm against a reporter log that the catch-up now runs (Background catch-up for room …: fetched N message(s) instead of Device is offline, skipping catch-up …)

🏁 Checklist

  • ⛑️ Tests (unit and/or integration) are included or not needed
  • 🔖 Capability is checked or not needed
  • 🔙 Backport requests are created or not needed: /backport to stable-xx.x
  • 📅 Milestone is set
  • 🌸 PR title is meaningful (if it should be in the changelog: is it meaningful to users?)

🤖 AI (if applicable)

  • The content of this PR was partly or fully generated using AI

@github-actions

github-actions Bot commented Sep 19, 2026

Copy link
Copy Markdown
Contributor

📱 QA build

Download app-qa-debug.apk
QR code Open the QR code for this download
Commit 35f64c9
Version 6727
Available until 7 days after this build

The QA build installs alongside a released Nextcloud app, so you can keep
using your existing install while testing.

Downloading the file requires a GitHub account, so open this link on the
device you want to test on, or transfer the APK to it.

@AndyScherzinger AndyScherzinger added this to the 25.1.0 milestone Sep 19, 2026
@AndyScherzinger

Copy link
Copy Markdown
Member Author

/backport to stable-25.0.x

@AndyScherzinger
AndyScherzinger force-pushed the bugfix/noid/push-prefetch-reliability branch 2 times, most recently from 2187f42 to 8f3e87f Compare September 20, 2026 12:22
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
AndyScherzinger force-pushed the bugfix/noid/push-prefetch-reliability branch from 8f3e87f to 35f64c9 Compare September 21, 2026 09:35
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
AndyScherzinger marked this pull request as ready for review September 21, 2026 10:03
@AndyScherzinger
AndyScherzinger merged commit 5b9b7d3 into master Sep 21, 2026
16 of 20 checks passed
@AndyScherzinger
AndyScherzinger deleted the bugfix/noid/push-prefetch-reliability branch September 21, 2026 10:12
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants