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

fix(sharing): Avoid (dead)locking during orphan deletion #43252

Merged
merged 1 commit into from
Mar 5, 2024

Conversation

ChristophWurst
Copy link
Member

@ChristophWurst ChristophWurst commented Feb 1, 2024

Summary

Concurrent modifications of shared entries in oc_filecache makes the share orphan background job lock. This is fine. If the row is UPDATED, we don't care. If the row is DELETEd, the next background job will catch it.

This replaces widely locking DELETE with SELECT+DELETE. See code comments.

How to test

The real world (dead)lock is hard to reproduce in a lab setup. You have to simulate concurrent modifications of filecache.

  1. Log into Nextcloud
  2. Create two folders f1 and f2
  3. Share both folders with another user
  4. Delete f2
  5. Go to trash bin and delete f2 permanently
  6. Open a database shell
    1. Run SELECT id FROM oc_jobs WHERE class = 'OCA\\Files_Sharing\\DeleteOrphanedSharesJob'. Note down the job id.
    2. Run SELECT file_target, file_source FROM oc_share ORDER BY id DESC LIMIT 2;. These are your f1 and f2 shares. Note down the file_source of f1.
    3. Run SELECT storage, path_hash FROM oc_filecache WHERE fileid=<file_source>;. Note down storage and path_hash values.
    4. Run START TRANSACTION;
    5. Run UPDATE `oc_filecache` SET `mtime` = GREATEST(`mtime`, 1706777193), `etag` = '659cf751000cd' WHERE (`storage` = <storage>) AND (`path_hash` IN (<path_hash>));
  7. Run php occ background-job:execute <id> --force-execute

master: job will stall. Run ROLLBACK; in the datbase shell to unlock the occ command.
here: job will finish and clean up the oc_share entry of f2 despite the lock on f1's filecache entry. Run ROLLBACK; to unlock your dev env for other operations ;-)

Checklist

@ChristophWurst ChristophWurst added the 2. developing Work in progress label Feb 1, 2024
@ChristophWurst ChristophWurst added this to the Nextcloud 29 milestone Feb 1, 2024
@ChristophWurst ChristophWurst self-assigned this Feb 1, 2024
@ChristophWurst
Copy link
Member Author

The old test still passes. This is good to go.

@ChristophWurst
Copy link
Member Author

/backport to stable28

@ChristophWurst
Copy link
Member Author

/backport to stable27

@ChristophWurst
Copy link
Member Author

/backport to stable26

Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

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

Is it not cleaner to select everything and chunk the deletes with array_chunk? It avoids the premature stop I think.

@ChristophWurst
Copy link
Member Author

Avoids the stop but could exhaust memory if there are lots of orphans

@solracsf
Copy link
Member

solracsf commented Feb 1, 2024

Avoids the stop but could exhaust memory if there are lots of orphans

Agree, same as #41272

Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

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

Hum ok but that would mean like a LOT of orphans. Other things will break first :-P

Anyway, fine with either approach.

@icewind1991
Copy link
Member

I've also created #43605 to reduce the interval from every 15m to daily

@skjnldsv skjnldsv added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Feb 23, 2024
@ChristophWurst ChristophWurst added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Feb 28, 2024
@ChristophWurst
Copy link
Member Author

This is done

@nickvergessen
Copy link
Member

Conflicting due to #43605

@icewind1991 icewind1991 force-pushed the fix/sharing/locking-orphan-share-delete branch from f0743d5 to 5073fdb Compare February 28, 2024 12:59
@icewind1991
Copy link
Member

rebased

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst ChristophWurst force-pushed the fix/sharing/locking-orphan-share-delete branch from 5073fdb to 5c20f5b Compare March 5, 2024 19:19
@ChristophWurst ChristophWurst added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Mar 5, 2024
@skjnldsv skjnldsv disabled auto-merge March 5, 2024 21:02
@skjnldsv skjnldsv merged commit 7efb36b into master Mar 5, 2024
158 of 160 checks passed
@skjnldsv skjnldsv deleted the fix/sharing/locking-orphan-share-delete branch March 5, 2024 21:02
@blizzz blizzz mentioned this pull request Mar 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish bug feature: files feature: sharing
Projects
None yet
8 participants