5128: Fix stuck uploads #5257

RitikaPahwa4444 · 2023-07-07T13:22:38Z

Description (required)

What changes did you make and why?
Uploads often get stuck when users turn their screen off or simply navigate away from their screen, especially when the battery usage for the app is not set as "Unrestricted".

Changed compileSdkVersion to 33 as suggested in the documentation
Upgraded to the latest stable release of WorkManager
Passed FLAG_IMMUTABLE with PendingIntent for version code S and above. Android has made it mandatory for apps targeting S+ (API level 31 and above) to pass either FLAG_MUTABLE or FLAG_IMMUTABLE with PendingIntent (based on my logcat error).

The above changes apparently fixed stuck uploads. The uploads, however, still fail with a SocketTimeoutException.

Implemented retries in onResume() so that failed uploads resume as soon as the user returns to the app. The code related to this may be improved with correct usage of WorkManager, but I need to investigate its behaviour on various devices under different network and battery conditions.

Key improvements:
Uploads no longer remain stuck and resume automatically in case the user returns to the app within 2 minutes (the connection time associated with the SocketTimeoutException). After 2 minutes, a "Failed to upload" notification is shown for all the uploads but these failed uploads, too, start as soon as the user returns to the app because of the retry logic.

This may not be the best solution, so I'm adding a WIP/Draft status to it.

Further improvements I am looking forward to:
I'll be researching more about network conditions when the screen is turned off. Even if Android provides a small window to resume work under "Optimised" battery conditions, we can fix the SocketTimeout issue.

Tests performed (required)

Tested prodDebug on Moto g62 with API level 31.

nicolas-raoul · 2023-07-07T15:08:40Z

Thanks!
I started testing it.
At first in mode "Unrestricted", as a baseline, it correctly uploaded a large set. :-)
Next I will try in optimized mode, but it might slip to after the weekend.

nicolas-raoul · 2023-07-09T14:20:56Z

Trying in Restricted mode, I am getting failures, and retry does not seem to work:

screen-20230709-211850.mp4

sivaraam

Just leaving some comments based on my initial observation.

sivaraam · 2023-07-09T19:23:03Z

app/src/main/java/fr/free/nrw/commons/upload/worker/UploadWorker.kt

@@ -540,7 +541,11 @@ class UploadWorker(var appContext: Context, workerParams: WorkerParameters) :
        val intent = Intent(appContext,toClass)
        return TaskStackBuilder.create(appContext).run {
             addNextIntentWithParentStack(intent)
-             getPendingIntent(0, PendingIntent.FLAG_UPDATE_CURRENT)
+             if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.S) {
+                 getPendingIntent(0, PendingIntent.FLAG_IMMUTABLE)


IIUC, I believe we should be keeping both the FLAG_IMMUTABLE and FLAG_UPDATE_CURRENT flags [ ref ]. Is there any reason behind not sending the FLAG_UPDATE_CURRENT here? Have you explored on what difference including / not including it makes ?

sivaraam · 2023-07-09T19:27:36Z

app/src/main/java/fr/free/nrw/commons/upload/worker/UploadWorker.kt

@@ -540,7 +541,11 @@ class UploadWorker(var appContext: Context, workerParams: WorkerParameters) :
        val intent = Intent(appContext,toClass)
        return TaskStackBuilder.create(appContext).run {
             addNextIntentWithParentStack(intent)
-             getPendingIntent(0, PendingIntent.FLAG_UPDATE_CURRENT)
+             if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.S) {


While the mandate has been since Android S, can we not send the flag for VERSION_CODES.M and above since the IMMUTABLE flag is available since that version?

RitikaPahwa4444 · 2023-07-10T06:04:03Z

Thank you for testing, I'll fix this.

I'd read about these possibilities related to PendingIntent on Stack Overflow. Since uploads were not getting stuck on older Android versions and FLAG_IMMUTABLE apparently fixed stuck uploads, I just went for the bare minimum. Will test it with the changes suggested above on my older device too. Thank you for the hints!

RitikaPahwa4444 · 2023-07-10T12:35:50Z

I could not manage to get the retry button, it simply restarts the process on my Android 12 device. I have modified the flag and version code, though.

This reverts commit 1ebb117.

This reverts commit c785d6f.

…ge mode

…ntent

nicolas-raoul · 2023-08-19T10:09:04Z

app/src/main/res/values/strings.xml

@@ -75,6 +76,9 @@
  <string name="login_success">Login success!</string>
  <string name="login_failed">Login failed!</string>
  <string name="upload_failed">File not found. Please try another file.</string>
+  <string name="retry_limit_reached">Maximum retry limit reached! Please cancel the upload and try again</string>
+  <string name="unrestricted_battery_mode">Turn battery optimisations off</string>


I would add a ? at the end to make it friendlier. Also, "optimization" (singular not plural) is less freightening and reflects the wording in the OS settings.

Oops, I'll use 'z' instead of 's' everywhere.

Ah sorry, I only meant about the singular. Either optimization or optimisation is fine for me 😊

nicolas-raoul · 2023-08-19T10:11:31Z

app/src/main/res/values/strings.xml

@@ -75,6 +76,9 @@
  <string name="login_success">Login success!</string>
  <string name="login_failed">Login failed!</string>
  <string name="upload_failed">File not found. Please try another file.</string>
+  <string name="retry_limit_reached">Maximum retry limit reached! Please cancel the upload and try again</string>
+  <string name="unrestricted_battery_mode">Turn battery optimisations off</string>
+  <string name="suggest_unrestricted_mode">Uploading more than 3 images works more reliably when the battery optimisations are turned off. Please turn battery optimisations off for the Commons app or fr.free.nrw.commons from the settings for a smooth upload experience. \n\nPossible steps to turn battery optimisation off:\n\nStep 1: Tap on the \'Settings\' button below.\n\nStep 2: Switch from \'Not optimised\' to \'All apps\'.\n\nStep 3: Scroll to find the Commons app or fr.free.nrw.commons.\n\nStep 4: Tap it and select \'Don\'t optimise\'.\n\nStep 5: Press \'Done\'.</string>


The first "or fr.etc" is not needed I think

nicolas-raoul

A few minor suggestions

nicolas-raoul · 2023-08-21T06:32:25Z

app/src/main/java/fr/free/nrw/commons/contributions/MainActivity.java

@@ -138,6 +148,9 @@ public void onCreate(Bundle savedInstanceState) {
            setTitle(getString(R.string.navigation_item_explore));
            setUpLoggedOutPager();
        } else {
+            if (applicationKvStore.getBoolean("firstrun", true)) {
+                applicationKvStore.putBoolean("firstBigUploadSet", true);


I appreciate the consistency with firstrun, but it is troubling to have firstBigUploadSet being true during small uploads.
How about renaming it to hasAlreadyLaunchedBigMultiupload and inverting true/false?
It would be easier to understand, I believe.
Thanks!

nicolas-raoul · 2023-08-21T06:38:20Z

app/src/main/java/fr/free/nrw/commons/upload/UploadActivity.java

@@ -364,6 +376,24 @@ private void receiveSharedItems() {
                    .getQuantityString(R.plurals.upload_count_title, uploadableFiles.size(), uploadableFiles.size()));

            fragments = new ArrayList<>();
+            // Suggest users to switch to Unrestricted battery usage mode


Append when uploading more than a few files. That's because we have noticed that many-files uploads have a much higher probability of failing than uploads with less files.

nicolas-raoul · 2023-08-21T06:40:41Z

app/src/main/java/fr/free/nrw/commons/upload/UploadActivity.java

+                    getString(R.string.title_activity_settings),
+                    getString(R.string.cancel),
+                    () -> {
+                        Intent batteryOptimisationSettingsIntent = new Intent(


Please add a comment about the library you tried, explaining how it did not make the setting dialog appear on Pixel nor Xiaomi. Someone may want to revisit that in the future.

nicolas-raoul · 2023-08-21T07:00:53Z

app/src/main/res/values/strings.xml

@@ -75,6 +76,9 @@
  <string name="login_success">Login success!</string>
  <string name="login_failed">Login failed!</string>
  <string name="upload_failed">File not found. Please try another file.</string>
+  <string name="retry_limit_reached">Maximum retry limit reached! Please cancel the upload and try again</string>
+  <string name="unrestricted_battery_mode">Turn battery optimization off?</string>
+  <string name="suggest_unrestricted_mode">Uploading more than 3 images works more reliably when the battery optimization are turned off. Please turn battery optimization off for the Commons app from the settings for a smooth upload experience. \n\nPossible steps to turn battery optimization off:\n\nStep 1: Tap on the \'Settings\' button below.\n\nStep 2: Switch from \'Not optimized\' to \'All apps\'.\n\nStep 3: Scroll to find the Commons app or fr.free.nrw.commons.\n\nStep 4: Tap it and select \'Don\'t optimize\'.\n\nStep 5: Press \'Done\'.</string>


are turned off -> is turned off

Sorry, I should have read the sentence again after removing the 's' :')

…me firstBigUploadSet

kartikaykaushik14

Good work overall. Well commented and relevant TODOs added. Please check the comments.

app/src/main/java/fr/free/nrw/commons/contributions/ContributionsPresenter.java

app/src/main/java/fr/free/nrw/commons/contributions/ContributionsFragment.java

kartikaykaushik14 · 2023-08-21T17:49:01Z

app/src/main/java/fr/free/nrw/commons/contributions/ContributionsFragment.java

+                   will never be successful */
+                if(retries < MAX_RETRIES) {
+                    contribution.setRetries(retries + 1);
+                    Timber.d("Retried %d times", retries + 1);


Can it be changed to "Retried upload %d times" so it's clear what we're trying to retry?

Indeed the debug log line could say that, and also say what file, because often several files are being retried at the same time.

sivaraam

I've left a couple of comments based on my initial look at the changes. Kindly take a look.

Also, have you checked the interaction of these changes with the "Limited Connection mode" @RitikaPahwa4444 ? Do they work as intended even after these changes ?

sivaraam · 2023-08-23T05:54:10Z

app/src/main/java/fr/free/nrw/commons/contributions/Contribution.kt

+    /**
+     * Number of times a contribution has been retried after a failure
+     */
+    var retries: Int = 0


This seems like a schema change to the contribution table and requires a bump of the db version number [ref]. Kindly take care of the same.

sivaraam · 2023-08-23T06:02:51Z

app/src/main/java/fr/free/nrw/commons/upload/UploadActivity.java

+            if (uploadableFiles.size() > 3
+                && !defaultKvStore.getBoolean("hasAlreadyLaunchedBigMultiupload")) {


I guess we should show this dialog only for relevant Android versions. The intent itself is only available since API 23 (6+). Kindly check and make sure we only invoke this for the relevant Android versions.

RitikaPahwa4444 · 2023-08-23T06:27:42Z

I often switch to limited connection mode, never found any of these changes interfering with its behaviour. Will test more since you feel these changes could affect its functioning👍🏻

sivaraam · 2023-08-24T17:36:03Z

app/src/main/java/fr/free/nrw/commons/contributions/ContributionsFragment.java

-                Timber.d("Restarting for %s", contribution.toString());
+            if (contribution.getState() == STATE_PAUSED || contribution.getState()==Contribution.STATE_QUEUED_LIMITED_CONNECTION_MODE) {
+                restartUpload(contribution);
+            } else if (contribution.getState() == STATE_FAILED) {


I was trying to test the change in my device. Blindly retrying all failed uploads does seem to be affecting the UX badly. The app seems to just retrying genuinely failed uploads again-and-again each time I open the app.

Is there really no other attribute that we have that could help with identifying if an upload failed for a genuine reason? (I'm presuming its because of beta cluster not supporting stashed uploads).

Also, I remember cancelling all uploads manually one time. The cancel didn't seem to have worked. Has anyone else observed this too?

nicolas-raoul

Tested on 100+ pictures, this pull request brings a great improvement in upload reliability.
The code also looks good to me.

RitikaPahwa4444 · 2023-09-10T17:23:54Z

Closing this as these changes have already been merged with #5272.

RitikaPahwa4444 added 2 commits July 6, 2023 14:24

fix stuck uploads

4aa4ea0

automate retries for failed uploads once the user returns to the app

96058ff

RitikaPahwa4444 marked this pull request as draft July 7, 2023 13:22

sivaraam reviewed Jul 9, 2023

View reviewed changes

UploadWorker: modify PendingIntent flag and Android version code

870f286

RitikaPahwa4444 added 13 commits July 13, 2023 21:13

MainActivity: remove automatic retry logic

1ebb117

Revert "MainActivity: remove automatic retry logic"

2dd2be1

This reverts commit 1ebb117.

set work request as expedited

6b9e5ab

merge branch 'master' into issue-5128_fix_stuck_uploads

2ec4667

handle notification for foreground service on older versions of Android

f1b2e03

set backoff criteria for work requests

0a64989

enqueue failed uploads for a retry

c785d6f

revert "enqueue failed uploads for a retry"

e1855b9

This reverts commit c785d6f.

limit the number of retries for a failed upload

6d013e6

add a popup that suggests users to switch to unrestricted battery usa…

4cafd2a

…ge mode

take users to the battery settings page on the first big upload

04b7255

take users to battery optimisation settings page using the standard i…

533e976

…ntent

add instructions to the battery optimisation settings popup

4fbe261

nicolas-raoul requested changes Aug 19, 2023

View reviewed changes

RitikaPahwa4444 added 2 commits August 19, 2023 15:44

remove the first usage of fr.free.nrw.commons from the popup

c35d936

comply with the wording in the OS settings

d1bfb48

nicolas-raoul requested changes Aug 21, 2023

View reviewed changes

modify battery optimisation popup instructions, add comments and rena…

3ff04d3

…me firstBigUploadSet

kartikaykaushik14 reviewed Aug 21, 2023

View reviewed changes

add filename to the retry log statement

ab2f114

sivaraam requested changes Aug 23, 2023

View reviewed changes

RitikaPahwa4444 added 2 commits August 23, 2023 12:06

update database version

faf0cde

make battery optimisation dialog appear only on Android 6 and above

f441eaa

sivaraam reviewed Aug 24, 2023

View reviewed changes

use foreground service instead of setting work request as expedited

5ec4d56

RitikaPahwa4444 mentioned this pull request Aug 27, 2023

5136: Fix retried uploads stuck in queued state #5272

Merged

RitikaPahwa4444 changed the title ~~[WIP] 5128: Fix stuck uploads~~ 5128: Fix stuck uploads Aug 27, 2023

RitikaPahwa4444 marked this pull request as ready for review August 27, 2023 07:49

nicolas-raoul approved these changes Aug 30, 2023

View reviewed changes

forbid retries for images which have got uploaded without caption

034d8d6

RitikaPahwa4444 mentioned this pull request Sep 5, 2023

Prevent retries for genuinely failed uploads #5284

Open

RitikaPahwa4444 closed this Sep 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

5128: Fix stuck uploads #5257

5128: Fix stuck uploads #5257

RitikaPahwa4444 commented Jul 7, 2023

nicolas-raoul commented Jul 7, 2023

nicolas-raoul commented Jul 9, 2023

sivaraam left a comment

sivaraam Jul 9, 2023

sivaraam Jul 9, 2023

RitikaPahwa4444 commented Jul 10, 2023

RitikaPahwa4444 commented Jul 10, 2023

nicolas-raoul Aug 19, 2023

RitikaPahwa4444 Aug 19, 2023

nicolas-raoul Aug 19, 2023

nicolas-raoul Aug 19, 2023

RitikaPahwa4444 Aug 19, 2023

nicolas-raoul left a comment

nicolas-raoul Aug 21, 2023

nicolas-raoul Aug 21, 2023

nicolas-raoul Aug 21, 2023

nicolas-raoul Aug 21, 2023

RitikaPahwa4444 Aug 21, 2023

kartikaykaushik14 left a comment

kartikaykaushik14 Aug 21, 2023

nicolas-raoul Aug 22, 2023

sivaraam left a comment

sivaraam Aug 23, 2023

sivaraam Aug 23, 2023

RitikaPahwa4444 commented Aug 23, 2023

sivaraam Aug 24, 2023

nicolas-raoul left a comment

RitikaPahwa4444 commented Sep 10, 2023

		if (uploadableFiles.size() > 3
		&& !defaultKvStore.getBoolean("hasAlreadyLaunchedBigMultiupload")) {

5128: Fix stuck uploads #5257

5128: Fix stuck uploads #5257

Conversation

RitikaPahwa4444 commented Jul 7, 2023

nicolas-raoul commented Jul 7, 2023

nicolas-raoul commented Jul 9, 2023

sivaraam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RitikaPahwa4444 commented Jul 10, 2023

RitikaPahwa4444 commented Jul 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-raoul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kartikaykaushik14 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sivaraam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RitikaPahwa4444 commented Aug 23, 2023

Choose a reason for hiding this comment

nicolas-raoul left a comment

Choose a reason for hiding this comment

RitikaPahwa4444 commented Sep 10, 2023