Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] Retried successful uploads don't free space in /tmp folder #4341

Merged

Conversation

Aitorbp
Copy link
Contributor

@Aitorbp Aitorbp commented Mar 15, 2024

Related Issues

App:#4335

  • Add changelog files for the fixed issues in folder changelog/unreleased. More info here
  • Add feature to Release Notes in ReleaseNotesViewModel.kt creating a new ReleaseNote() with String resources (if required)

QA

Checks:

#4341 (comment)

Test Matrix:

#4341 (comment)

Reports:

@Aitorbp Aitorbp self-assigned this Mar 15, 2024
@Aitorbp Aitorbp linked an issue Mar 15, 2024 that may be closed by this pull request
@Aitorbp Aitorbp force-pushed the fix/retried_successful_uploads_dont_free_space_tmp_folder branch 3 times, most recently from f7e06eb to 721cfca Compare March 20, 2024 10:07
@JuancaG05 JuancaG05 self-requested a review March 27, 2024 16:27
Copy link
Collaborator

@JuancaG05 JuancaG05 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a change in wording in calens and the release note and ready to go @Aitorbp

changelog/unreleased/4341 Outdated Show resolved Hide resolved
owncloudApp/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
@Aitorbp Aitorbp force-pushed the fix/retried_successful_uploads_dont_free_space_tmp_folder branch 2 times, most recently from a42ae0d to 8f67b89 Compare April 1, 2024 07:00
@Aitorbp Aitorbp requested a review from JuancaG05 April 1, 2024 08:22
Copy link
Collaborator

@JuancaG05 JuancaG05 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jesmrec
Copy link
Collaborator

jesmrec commented Apr 2, 2024

QA checks:

  • Upload success at the first try -> /tmp cleaned up
  • Upload success at the second try -> /tmp cleaned up
  • Upload success at the third try -> /tmp cleaned up
  • Upload from camera: nothing in /tmp (check behaviour before)
  • Upload from 3rd party app: saved in /tmp and resumed from there (1st and sucessive tries)
  • Upload from auto uploads (keep)
  • Upload from auto uploads (move)

@jesmrec
Copy link
Collaborator

jesmrec commented Apr 2, 2024

(1) [FIXED]

  1. Enable auto uploads and fill up all the fields
  2. Select kept in original folder in Original file will be
  3. Take same pictures from camera
  4. Switch server off (or any other condition that makes upload file, except removing device connection)
  5. Wait 15 minutes till uploads are enqueued (check in uploads view)
  6. When uploads are enqueued and failed, switch server on (or recover the condition that made them fail)
  7. Retry all

Current:

When all uploads are finished after the 2nd try, the /tmp folder contains all the uploaded files. They are not removed.

Expected:

After the 2nd try with success, all files are removed from the /tmp folder

Pixel 2 Android11
Galaxy Tab A8, Android 13
8cba150e0

@jesmrec
Copy link
Collaborator

jesmrec commented Apr 2, 2024

1st try success ✅ 1st try fail ❌
2nd try success ✅
1st & 2nd try fail ❌
3rd try success ✅
Manual uploads Result: ✅
/tmp: cleaned up ✅
Device: Files kept ✅
Result: ✅
/tmp: cleaned up ✅
Device: Files kept ✅
Result: ✅
/tmp: cleaned up ✅
Device: Files kept ✅
3rd party app upload Result: ✅
/tmp: ✅
Device: Files kept ✅
Result: ✅
/tmp: cleaned up ✅
Device: Files kept ✅
Result: ✅
/tmp: cleaned up ✅
Device: Files kept ✅
Auto upload (keep) Result: ✅
/tmp: cleaned up ✅
Device: Files kept ✅
Result: ✅
/tmp: cleaned up ✅
Device: Files kept ✅
Result: ✅
/tmp: cleaned up ✅
Device: Files kept ✅
Auto upload (move) Result: ✅
/tmp: cleaned up ✅
Device: Files removed ✅
Result: ✅
/tmp: cleaned up ✅
Device: Files removed ✅
Result: ✅
/tmp: cleaned up ✅
Device: Files removed ✅
Picture Upload from Camera Result: ✅
/tmp: clean ✅
Device: not in device ✅
Result: ✅
/tmp: clean ✅
Device: not in device ✅
Result: ✅
/tmp: clean ✅
Device: not in device ✅
Documents provider
(new)
Result: ✅
/tmp: cleaned up ✅
Device: not in device ✅
Result: ✅
/tmp: cleaned up ✅
Device: not in device ✅
Result: ✅
/tmp: cleaned up ✅
Device: not in device ✅
Documents provider
(from other provider - copy)
Result: ✅
/tmp: cleaned up ✅
Device: Files kept ✅
Result: ✅
/tmp: cleaned up ✅
Device: Files kept ✅
Result: ✅
/tmp: cleaned up ✅
Device: Files kept ✅
Documents provider
(from other provider - move)
Result: ✅
/tmp: cleaned up ✅
Device: Files removed ✅
Result: ✅
/tmp: cleaned up ✅
Device: Files removed ✅
Result: ✅
/tmp: cleaned up ✅
Device: Files removed ✅

Some notes about how uploads work:

  • When something is going to be uploaded, is first copied/moved to /tmp inside the internal storage and pushed to the server from there. There is one exception: picture uploads. In this case, the picture is not copied to /tmp (probably it stays in memory) before being pushed to the server

  • For auto uploads with move behaviour (removing local copy), in the moment the files are moved to /tmp, they are removed from local storage. In case of a release build, pictures and videos are not reachable anymore in case some strange failure happens. I'd add some kind of warning if user chooses this option . Is this feasible @Aitorbp @JuancaG05 @joragua ? (to address in another issue), wdyt?

  • For "from document provider": copy and move options make the same effect in the app (one uploaad). Just to keep consistency with the other cases, i tested also the move operation in the doc. provider.

@jesmrec
Copy link
Collaborator

jesmrec commented Apr 4, 2024

Approved on my side

Take the chart above as reference of the uploads behaviour in reference to the internal storage.

@Aitorbp Aitorbp force-pushed the fix/retried_successful_uploads_dont_free_space_tmp_folder branch from 0ab6d55 to f739074 Compare April 4, 2024 09:26
@Aitorbp Aitorbp merged commit ba20b9e into master Apr 4, 2024
3 checks passed
@Aitorbp Aitorbp deleted the fix/retried_successful_uploads_dont_free_space_tmp_folder branch April 4, 2024 10:26
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.

[BUG] Retried successful uploads don't free space in /tmp folder
3 participants