Issues to solve with locking

[PVince81]

• TODO: lock scanner: prevent the scanner to scan or modify/delete locked elements (see Data loss on rename of a 49 GB folder #13391 (comment)). Maybe the scanner should also use acquireLock to make sure other processes do not try and do something with the cache entries that are about to be changed. This also means that concurrent scanner processes would exclude each other.
• TODO: also acquire shared lock before rename pre-hooks (as we did in basicOperation): Acquire locks before post-hooks, release afterwards #16829
• TODO: wrap touch operation from upload (chunking mode) in the transaction: touch file early after upload to avoid having wrong time #13498 (non-chunking should be fine). This will also need the "scanner locking" to avoid the scanner finding the non-touched-file with a different timestamp. => PR here locking for chunked dav upload #17104
• TODO: also properly wrap $view->file_put_contents with locks. I see it calls $view->fopen() but the cache update is done after fclose(). Might be safer to also include the cache update and post hooks inside a transaction/lock. add proper locking to file_put_contents when using streams #16727
• BUG: rename folder during upload still has inconsistency between cache and on-disk data (Rename a folder during upload race condition #11795)

1. Add sleep(10); here: https://github.com/owncloud/core/blob/master/lib/private/files/cache/cache.php#L551
2. Setup sync client
3. Close sync client
4. Copy a big folder "test" locally
5. Start the sync client
6. Quickly, while it starts uploading: refresh the web UI
7. Rename "test" to "re"
8. Sync client fails with error "could not find folder 'test'" after a bunch of uploads, which is expected
9. In web UI: "re" is still empty, but "data/$user/re" contains files
10. Sync client resumes upload, but in the end not all files are present in "test"

We need to tweak the locking to be able to avoid such situations.

• rename folder with slow cache update still loses contents, see Data loss on rename of a 49 GB folder #13391 (comment)
• BUG: properly handle LockedException in web UI: when renaming a locked folder in the web UI: exception returned as "500 Internal Server Error" but logged properly:
• BUG: make etag precondition + final rename atomic: Slow transfer can bypass etag precondition check, no conflict file #16569 (the current locking did not help, might need to retry after acquireLock is set for small files)
• BUG: sleep after chunk assembly but before final rename, then rename parent folder: sync fails with "could not rename part file to final file". We should bundle chunking inside a transaction. But the current behavior might be ok as it doesn't cause any data corruption.
• BUG: [minor] move big folder to ext storage in web UI, sync client running: sync client cannot download any of the moved files as long as the move is happening (even if the files are there already). Once the move is done, the sync client retries and can get the file. Would still be nice to have to be able to download files as soon as they are moved (but keep the lock on the folder)
  To clarify: download should be forbidden in the source folder, but not in the target folder.

Tests:

• TEST: move big folder to ext storage. During the slow move, cannot rename source or target folder: locked.
• TEST: cannot rename folder while it is being moved to trash (tested with slow ext storage, copy from storage to trash)
• TEST: cannot upload into a folder while it is being moved to trash
• TEST: cannot rename a folder while chunks are being assembled (added sleep() in the chunk assembly loop), LockedException
• TEST: slow down streamCopy for WebDAV upload and try renaming the parent folder
• TEST: delete folder while uploading inside (with slow streamCopy): denied by locked error
• TEST: kill sync client during upload (with slow streamCopy): lock expires after PHP timeout
• TEST: simulate "expected filesize did not match" error: lock was properly released despite exception (shutdown handler)
• TEST: cannot rename target folder when restoring it from trash
• TEST: cannot download files inside a folder that is being moved to trash (source folder)
• TEST: restore big folder from trash to SFTP (slow), try restoring it at the same time. Restore fails due to lock.
• TEST: rename "test" to "test2", sleep before the actual rename: cannot create "test2" in another thread because locked

Sharing (locks must work across users):

• TEST: slow upload of small file as owner, recipient cannot download while locked
• TEST: slow upload of small file as recipient, owner cannot download while locked
• TEST: owner moves folder, recipient cannot rename source or target folder
• TEST: recipient moves folder, owner cannot rename source or target folder
• TEST: delete subfolder as recipient, owner cannot rename/download contents
• TEST: delete subfolder as owner, recipient cannot rename/download contents
• TEST: owner unshares folder (can be slow with encryption), recipient cannot rename/download folder contents => raised as Unshare from user must lock path to avoid recipient access #17244
• TEST: delete file in thread A, try and share it in thread B. Share must fail because of lock. 🚫 => raised as Lock sharing operations when file is locked #17243
• TEST: delete subfolder as owner in thread A, recipent can still unshare from self in thread B(TBC?) ❓
• TEST: delete subfolder as recipient in thread A, owner can still unshare from that recipient in thread B (TBC?) ❓ works but produces warning (sleep in move2trash after setupTrash)
• TEST: unshare from self twice
• TEST: share with group/user with encryption enabled (make key creation slow with sleep): recipient should not get access to the files until this is done 🚫 FAIL, raised as [concurrency] Access newly shared encrypted folder before all keys are ready #16621
• TEST: as owner, rename parent folder of shared folder when locked by recipient
• TEST: as recipient, rename parent folder of shared folder when locked by owner

Versions:

• TEST: cannot download version of a file being moved (neither from source nor target, single and cross-storage) 🚫 FAIL, missing lock (sleep after acquireLock in $view->rename()) => raised as Prevent version app operations when file is locked #17245
• TEST: cannot restore version of a file being moved (neither from source nor target, single and cross-storage). 🚫 FAIL: restore is blocked by write lock, but the original file gets deleted! (sleep after acquireLock in $view->rename()) => Prevent version app operations when file is locked #17245
• TEST: restore version (with sleep in restore code) in thread A, cannot rename or overwrite the file in thread B => Lock original file when doing operations on versions #17269
• TEST: sleep in loop that moves each version (https://github.com/owncloud/core/blob/master/apps/files_versions/lib/storage.php#L270); thread A moves file, thread B overwrites new file name. While the versions are moved (post hook), the file is still locked with a share lock: Keep shared lock for post-hooks #17030
• TEST: sleep in version expiration loop (https://github.com/owncloud/core/blob/master/apps/files_versions/lib/storage.php#L617), trigger cron.php in thread A, try moving the file in thread B 🚫

Previews:

• TEST: if file was locked (not downloadable) during preview creation, preview needs to retry later
  (achieved by overwriting a pic file that has no preview yet, and sleep in streamCopy)

Encryption:

• TEST: general testing if locking works like

Zip download:

• TEST: download zip of a folder containing a locked new file (sleep after acquireLock before final rename) => new file does not appear in zip
• TEST: download zip of a folder containing a locked file to be overwritten (sleep after acquireLock before final rename) => zip file is empty 🚫 FAIL, should skip locked file ?! => Download directory as zip while an inner file is being overwritten causes the download to be corrupted #16960

s2s and locking

• TEST: share "local" from server A to server B. On server A, slow-upload+overwrite file (sleep after acquireLock in sabre/file). Server B cannot download. 🚫 FAIL for some reason server B can still download => raised as Put file on remote share seems to bypass lock #17275

[PVince81]

@icewind1991 see above

[PVince81]

I suggest to focus on the scanner first so we can get the most critical issue solved at last.

[PVince81]

• BUG: properly show locking error message in the web UI: Properly show locked file error message in web UI #16701

[icewind1991]

BUG: rename folder during upload still has inconsistency between cache and on-disk data

Not really something we can fix without having the client manually acquire a lock on the folder it's uploading to, it's not an issue with concurrent requests

[PVince81]

@icewind1991 I still think it should be possible to recover from this situation without having missing files, even if one half is in one folder and the other half in the other one. Goal is to avoid having a situation where files are completely lost. Either the rename operation needs to stop/fail or the sync client will give up once one of the uploads doesn't reach its target.
Not sure, but this might be related to scanner locking too.

[icewind1991]

Not sure, but this might be related to scanner locking too.

It's not

The problem has nothing to do with concurrent requests, it's caused because uploading a folder from the sync client takes multiple requests, there is no way to make that atomic without manually managing a lock for it.

A different way to fix it unrelated to locking would be to have the sync client specify the fileid of the target folder, that way the server can either return a proper error (folder renamed to ...., try again with that path) or get the target folder from that id

[PVince81]

The upload doesn't have to be atomic. The sync client would upload 5 files of 10, then get the locked error because someone started renaming the folder (the folder rename must be atomic), and it would give up until the folder is freed again. So in the end the first folder would have 5 files, and then the sync client would recreate the original folder name (once unlocked) to reupload all files.
Hmm I guess it means in the end we'd have a folder with 5 files (the renamed one) and another folder with the original name but all the 10 ones.
But then there's the etag/rename change that might be detected.
Tricky indeed.

[PVince81]

I'll try and go through the list again and raise separate tickets.

But before I do we need to fix the regressions: #16998
and also #16963 which has a high risk of influencing a lot of the code paths, which means that retesting only makes sense once that one is finished.

[ghost]

thank you @PVince81

[PVince81]

Assigning to self to verify the failed cases I found before.
If they still happen, I'll make separate tickets for them.

I expect the following to still have issues: sharing (share and unshare currently do not set locks) and also versions (version operations aren't transactional yet). Hopefully these should not be critical.

[PVince81]

Moving to 8.1.1.

Remaining issues are related to versions that is lacking locking.

[PVince81]

All outstanding issues have been raised as separate tickets, closing this one.