Skip to content

feat(files_trashbin): Refactor expire background job to support parallel run - #52825

Merged
artonge merged 2 commits into
masterfrom
artonge/feat/files_trashbin_parallel_expire_job
Sep 8, 2025
Merged

feat(files_trashbin): Refactor expire background job to support parallel run#52825
artonge merged 2 commits into
masterfrom
artonge/feat/files_trashbin_parallel_expire_job

Conversation

@artonge

@artongeartonge commented May 14, 2025

Copy link
Copy Markdown
Collaborator

The original PR introduced the possibility to continue an ExpireTrash job by saving the offset. This was to prevent having to start over the whole user list when the job crashed or was killed.

But on big instances, one process is not enough to go through all the users in a timely manner. Supporting parallel run allows covering more ground faster.

This PR introduced this possibility. We are now storing the offset right away to allow another parallel job to pick up the task at that point. We are arbitrarily cutting the user list in chunk of 10 to not drastically overflow the 30 minutes time limit.

@artongeartonge self-assigned this May 14, 2025
@artongeartonge added 3. to review Waiting for reviews feature: trashbin php Pull requests that update Php code enhancement labels May 14, 2025
@artongeartonge added this to the Nextcloud 32 milestone May 14, 2025
@artonge
artonge marked this pull request as ready for review May 14, 2025 16:24
@artonge
artonge requested a review from a team as a code ownerMay 14, 2025 16:24
@artonge
artonge requested review from nfebe, skjnldsv and sorbaugh and removed request for a teamMay 14, 2025 16:24
@artonge
artongeforce-pushed the artonge/feat/files_trashbin_parallel_expire_job branch 2 times, most recently from 2a3426f to f8acd91CompareMay 14, 2025 16:35
@artonge
artonge requested a review from come-ncMay 14, 2025 16:36
@come-nc

Copy link
Copy Markdown
Contributor

apps/files_trashbin/lib/BackgroundJob/ExpireTrash.php:49:4: AssignmentToVoid: Cannot assign $offset to type void (see https://psalm.dev/121)
Sounds real

@sgyuris

Copy link
Copy Markdown
  1. The 1. job starts, query for 10 user and write down the offset, let's say: 10
  2. The 2. job starts and query for 10 user with offset of 10. Immediately it writes down the offset: 20
  3. The 1. job terminates just after it started to clean up user3's trashbin.

What will happen with the remaining files in the user3's trashbin?
What will happen with the user4 - user10 trashbins?

@artonge

Copy link
Copy Markdown
CollaboratorAuthor

3. The 1. job terminates just after it started to clean up user3's trashbin.

How would that happen ?

@sgyuris

Copy link
Copy Markdown
  1. The 1. job terminates just after it started to clean up user3's trashbin.

How would that happen ?

MySQL has gone away... or db deadlock... storage issues or it is killed by any other means like oomkiller...

I know this indicates some kind of infrastructure error still we need to account for it.

@artonge
artongeforce-pushed the artonge/feat/files_trashbin_parallel_expire_job branch from f8acd91 to 45a0834CompareMay 15, 2025 08:05
@artonge

artonge commented May 15, 2025

Copy link
Copy Markdown
CollaboratorAuthor

MySQL has gone away... or db deadlock... storage issues or it is killed by any other means like oomkiller...

All of those should be caught by the try/catch.

Worth case, like the job being killed, less than 10 users would be skipped, and they will be handled by the next round of jobs.

@sgyuris

Copy link
Copy Markdown

Lets see...

@artonge
artongeforce-pushed the artonge/feat/files_trashbin_parallel_expire_job branch from 2daee64 to ebe5e39CompareAugust 28, 2025 15:48
This was referenced Aug 28, 2025
Signed-off-by: Louis Chemineau <louis@chmn.me>
@artonge
artongeforce-pushed the artonge/feat/files_trashbin_parallel_expire_job branch 2 times, most recently from f996f08 to 2b8c7c4CompareSeptember 5, 2025 13:06
Comment threadapps/files_trashbin/lib/BackgroundJob/ExpireTrash.php Outdated
Comment threadapps/files_trashbin/lib/BackgroundJob/ExpireTrash.php Outdated
Comment threadapps/files_trashbin/lib/BackgroundJob/ExpireTrash.php Outdated
Comment threadapps/files_trashbin/lib/BackgroundJob/ExpireTrash.php Outdated
@artonge
artongeforce-pushed the artonge/feat/files_trashbin_parallel_expire_job branch from 8ec0e54 to 87ff4a1CompareSeptember 8, 2025 09:14
…lel run
- Follow-up of #51600
The original PR introduced the possibility to continue an `ExpireTrash` job by saving the offset. This was to prevent having to start over the whole user list when the job crashed or was killed.
But on big instances, one process is not enough to go through all the users in a timely manner. Supporting parallel run allows covering more ground faster.
This PR introduced this possibility. We are now storing the offset right away to allow another parallel job to pick up the task at that point. We are arbitrarily cutting the user list in chunk of 10 to not drastically overflow the 30 minutes time limit.
Signed-off-by: Louis Chemineau <louis@chmn.me>
@artonge
artongeforce-pushed the artonge/feat/files_trashbin_parallel_expire_job branch from 87ff4a1 to 9b5d118CompareSeptember 8, 2025 10:33
@artonge
artonge merged commit c8eeade into masterSep 8, 2025
202 of 204 checks passed
@artonge
artonge deleted the artonge/feat/files_trashbin_parallel_expire_job branch September 8, 2025 13:53
@artonge

Copy link
Copy Markdown
CollaboratorAuthor

/backport to stable32

@artonge

Copy link
Copy Markdown
CollaboratorAuthor

/backport to stable31

@artonge

Copy link
Copy Markdown
CollaboratorAuthor

/backport to stable30

@backportbot

Copy link
Copy Markdown

The backport to stable31 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable31
git pull origin stable31
# Create the new backport branch
git checkout -b backport/52825/stable31
# Cherry pick the change from the commit sha1 of the change against the default branch# This might cause conflicts, resolve them
git cherry-pick 1d91e40f 9b5d1184
# Push the cherry pick commit to the remote repository and open a pull request
git push origin backport/52825/stable31

Error: Failed to create pull request: Validation Failed: {"resource":"PullRequest","code":"custom","message":"A pull request already exists for nextcloud:backport/52825/stable31."} - https://docs.github.com/rest/pulls/pulls#create-a-pull-request


Learn more about backports at https://docs.nextcloud.com/server/stable/go.php?to=developer-backports.

@backportbot

Copy link
Copy Markdown

The backport to stable30 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable30
git pull origin stable30
# Create the new backport branch
git checkout -b backport/52825/stable30
# Cherry pick the change from the commit sha1 of the change against the default branch# This might cause conflicts, resolve them
git cherry-pick 1d91e40f 9b5d1184
# Push the cherry pick commit to the remote repository and open a pull request
git push origin backport/52825/stable30

Error: Failed to create pull request: Validation Failed: {"resource":"PullRequest","code":"custom","message":"A pull request already exists for nextcloud:backport/52825/stable30."} - https://docs.github.com/rest/pulls/pulls#create-a-pull-request


Learn more about backports at https://docs.nextcloud.com/server/stable/go.php?to=developer-backports.

@nextcloud-botnextcloud-bot mentioned this pull request Jan 6, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to reviewWaiting for reviewsenhancementfeature: trashbinphpPull requests that update Php code♻️ refactorRefactor code (not a bug fix, not a feature just refactoring)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@artonge@come-nc@sgyuris@provokateurin@skjnldsv