Don't retry tasks when running in foreground #6900

seadowg · 2025-09-21T16:15:12Z

This should fix a problem (pointed out by @lognaturel on Slack) where error notifications are not being shown after manually refreshing a project from the "Start new form" screen.

Why is this the best possible solution? Were any other approaches considered?

Comments inline.

Also: the 3 changes here aren't tested - initially this was just because I wasn't confident about how to fix so was relying on manual testing between myself and QA to get things right. When then trying to write tests, I discovered we're going to need to rework things - our current TaskSpecWorkerTest crashes if setForegroundAsync is called and our TestScheduler is not set up to test real foreground service behaviour. I'll follow up with a PR that adds coverage for all three changes here.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

The only thing affected here is that notifications should be shown for errors that happen when manually refreshing.

Before submitting this PR, please make sure you have:

added or modified tests for any new or changed behavior
run ./gradlew connectedAndroidTest (or ./gradlew testLab) and confirmed all checks still pass
added a comment above any new strings describing it for translators
added any new strings with date formatting to DateFormatsTest
verified that any code or assets from external sources are properly credited in comments and/or in the about file.
verified that any new UI elements use theme colors. UI Components Style guidelines

dbemke · 2025-09-22T08:24:57Z

To keep track of steps to reproduce and the expected behavior:

Steps to reproduce:

Create a draft of a form in Central.
Click "try on device" and scan the QR code in Collect.
After download the project(draft form) in Collect, go to Central and delete the form.
Go to Collect to "Start new form" and tap the arrow icon.

Expected behavior
The arrow (in "Start new form") changes to arrow+exclamation mark and there's the notification "Form update failed" Show details.
After tapping show details the user sees information about the [Drafts] and form "Collect can't reach the server...."

@seadowg Please verify if the step to reproduce and expected behavior are ok.

dbemke · 2025-09-22T08:27:13Z

If the steps to reproduce and the expected behavior described above are ok, then the notifications isn't shown in this case on Android 10 and 16.

seadowg · 2025-09-22T08:30:27Z

@seadowg Please verify if the step to reproduce and expected behavior are ok.

That looks right to me. That's consistent with Collect v2025.2.3 right?

If the steps to reproduce and the expected behavior described above are ok, then the notifications isn't shown in this case on Android 10 and 16.

In this PR? I'll have a look.

dbemke · 2025-09-22T08:30:53Z

In this PR? I'll have a look.

Yes in the PR

seadowg · 2025-09-22T08:47:26Z

@dbemke yeah looks like there's a second bug, to fix to get this working. I'll remove "needs testing" while I deal with that.

seadowg · 2025-09-22T09:04:28Z

@dbemke ok that should all be working now!

dbemke · 2025-09-22T11:47:27Z

ok that should all be working now!

First test - it seems to be working well so I'll add "needs testing" label

seadowg · 2025-09-22T12:13:37Z

async/src/main/java/org/odk/collect/async/TaskSpecWorker.kt

        try {
            val completed =
-                taskSpec.getTask(applicationContext, stringInputData, isLastUniqueExecution(taskSpec)) { isStopped }.get()
+                taskSpec.getTask(applicationContext, stringInputData, foreground || isLastUniqueExecution(taskSpec)) { isStopped }.get()


We should always count a foreground task as the last execution so that jobs like SyncFormTaskSpec switch to the behaviour they would use in that mode (like notifying about errors).

seadowg · 2025-09-22T12:14:41Z

async/src/main/java/org/odk/collect/async/TaskSpecWorker.kt

            return if (completed) {
                Result.success()
-            } else if (maxRetries == null || runAttemptCount < maxRetries) {
+            } else if (!foreground && (maxRetries == null || runAttemptCount < maxRetries)) {


We don't want to retry when running in the foreground: it doesn't fit with the idea of having more immediate results/feedback, and it also could cause errors (foreground services should not be started in the background).

seadowg · 2025-09-22T12:15:43Z

..._app/src/main/java/org/odk/collect/android/formlists/blankformlist/BlankFormListViewModel.kt

        private const val SYNC_NOTIFICATION_CHANNEL_NAME = "Form updates"

-        private const val SYNC_NOTIFICATION_ID = 1
+        private const val SYNC_NOTIFICATION_ID = 3


It seems like we've made an assumption in the code base that notifications are identified by their ID and their channel. In fact, they are only identified by ID, so this should be unique.

Could you elaborate on this? Why 3? What was the issue here?

If this is 1, it has the same ID as the error notification (FORM_SYNC_NOTIFICATION_ID) so the error gets cleared when the foreground service hides its notification. We're going to want to work out a centralized source for these IDs, but I wanted to leave that for once we've got tests (#6902).

We're going to want to work out a centralized source for these IDs

Yes, this would be important to make it clear. Will that be added to #6902 or a separate issue?

I'll add that to #6902. Right now it's just failing tests (one of which drives out this change). That PR will be the master version of these changes.

WKobus · 2025-09-22T13:37:43Z

Tested with success

Verified on device with android 16

Verified cases:

Manual refresh after deleting draft from central
Wrong project url
Airplane mode during form download
Successful form download after form download fail

dbemke · 2025-09-22T13:38:15Z

Tested with success

Verified on device with android 10

Don't retry tasks when running in foreground

Don't retry tasks when running in foreground

5f78789

seadowg added needs testing high priority Should be looked at before other PRs/issues labels Sep 21, 2025

seadowg changed the base branch from master to v2025.3.x September 21, 2025 16:18

seadowg removed the needs testing label Sep 22, 2025

Use unique ID for form updates notification

9a6647e

dbemke added the needs testing label Sep 22, 2025

seadowg commented Sep 22, 2025

View reviewed changes

seadowg marked this pull request as ready for review September 22, 2025 12:19

seadowg requested a review from grzesiek2010 September 22, 2025 12:19

dbemke added behavior verified and removed needs testing labels Sep 22, 2025

seadowg mentioned this pull request Sep 22, 2025

Add tests for foreground notifications #6902

Open

6 tasks

grzesiek2010 approved these changes Sep 23, 2025

View reviewed changes

grzesiek2010 merged commit ada3a13 into getodk:v2025.3.x Sep 23, 2025
6 checks passed

seadowg deleted the error-notification branch September 23, 2025 11:03

grzesiek2010 added a commit to grzesiek2010/collect that referenced this pull request Sep 29, 2025

Merge pull request getodk#6900 from seadowg/error-notification

5b48031

Don't retry tasks when running in foreground

grzesiek2010 mentioned this pull request Sep 29, 2025

Merge changes from v2025.3.1 #6909

Merged

Don't retry tasks when running in foreground #6900

Don't retry tasks when running in foreground #6900

Uh oh!

Conversation

seadowg commented Sep 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why is this the best possible solution? Were any other approaches considered?

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Before submitting this PR, please make sure you have:

Uh oh!

dbemke commented Sep 22, 2025

Uh oh!

dbemke commented Sep 22, 2025

Uh oh!

seadowg commented Sep 22, 2025

Uh oh!

dbemke commented Sep 22, 2025

Uh oh!

seadowg commented Sep 22, 2025

Uh oh!

seadowg commented Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dbemke commented Sep 22, 2025

Uh oh!

seadowg Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

seadowg Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

seadowg Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

grzesiek2010 Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

seadowg Sep 23, 2025

Choose a reason for hiding this comment

Uh oh!

grzesiek2010 Sep 23, 2025

Choose a reason for hiding this comment

Uh oh!

seadowg Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WKobus commented Sep 22, 2025

Uh oh!

dbemke commented Sep 22, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

seadowg commented Sep 21, 2025 •

edited

Loading

seadowg commented Sep 22, 2025 •

edited

Loading

seadowg Sep 23, 2025 •

edited

Loading