fix(sync): notification can get stuck.

[Giyutomioka-SS]

Purpose / Description

This change makes sure the sync notifications (“Syncing…” and “Syncing media…”) are always removed, even when sync is cancelled or fails with an error. Previously they were only cleared on a clean, successful sync, so in rare cases they could stay stuck for a long time.

Fixes

• Related to Sync notification can get stuck #19830

Approach

When the main sync worker and the media sync worker finish, we now always call notificationManager.cancel(...) from a finally block. This runs no matter how the work ends: success, error, or when the user presses the Cancel button on the notification, so the ongoing sync notifications are reliably dismissed.

How Has This Been Tested?

Verified that both sync workers now always cancel their notifications from a finally block on all exit paths (success, error, and user cancel), and that the project builds and ktlint passes locally.

but i haven’t been able to reliably reproduce the original stuck-notification issue, so I’d really appreciate it if someone who has seen it could help confirm the fix end to end, or share the exact steps to reproduce it.

Related:

• Syncing media notification gets stuck sometimes #17639

Checklist

Please, go through these checks before submitting the PR.

• You have a descriptive commit message with a short title (first line, max 50 chars).
• You have commented your code, particularly in hard-to-understand areas
• You have performed a self-review of your own code
• UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
• UI Changes: You have tested your change using the Google Accessibility Scanner

[criticalAY]

Logically seems correct to me but can you add tests to verify this, add tests to see if you can reproduce the error there and your solution later should pass the tests.

[Giyutomioka-SS]

Logically seems correct to me but can you add tests to verify this, add tests to see if you can reproduce the error there and your solution later should pass the tests.

Added Robolectric test for SyncMediaWorker that simulates an error after the foreground notification is active and verifies the notification is always cancelled from the finally block.

[lukstbit]

When we encounter an error in the workers we are currently showing a notification but your code would immediately cancel this notification.

I'm also less inclined to accept a fix if there's no reproduction case.

[Giyutomioka-SS]

When we encounter an error in the workers we are currently showing a notification but your code would immediately cancel this notification.

I'm also less inclined to accept a fix if there's no reproduction case.

To avoid the error notification being cancelled, can we use separate notification IDs for failed cases so the finally block only cancels the progress notification?? @lukstbit

I’ve tried multiple times to reproduce the original stuck-notification bug, but I haven’t been able to so far. The added test covers the same scenario (error after the notification is shown) and it passes with the fix, i'll keep testing on a real device and if I’m able to reproduce it i'll share a video here.

[lukstbit]

I don't think different notifications ids would fix this, feels something related to the WorkManager system itself when the worker is cancelled(maybe due to the constraint). I propose to just add a call to notificationManager?.cancel(NotificationId.SYNC_MEDIA) just before the cancelMediaSync(CollectionManager.getBackend()) line in the catch that deals with the coroutine cancellation and see how this goes in production.

The test should be removed, it mocks so much that it isn't real life behavior.

[Giyutomioka-SS]

I don't think different notifications ids would fix this, feels something related to the WorkManager system itself when the worker is cancelled(maybe due to the constraint). I propose to just add a call to notificationManager?.cancel(NotificationId.SYNC_MEDIA) just before the cancelMediaSync(CollectionManager.getBackend()) line in the catch that deals with the coroutine cancellation and see how this goes in production.

The test should be removed, it mocks so much that it isn't real life behavior.

Got it, thanks for the explanation and suggestion.

[david-allison]

It'd be great to see this fixed!

Drafting this until it's ready.

Please mark it as ready for review once a reproduction case is found (even if this is just a patch to the code).

---

In addition:

• Consider more logging if it's currently insufficient.
• Figure out why 'cancel'/swiping the notification wasn't working, and potentially also fix this failure case.