-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
[10.0.9]Trying to get owner from an erased file #32346
Comments
GitMate.io thinks the contributor most likely able to help you is @PVince81. Possibly related issues are #27770 (File version preview icon not showing up after upgrading from 9.1.5 to 10.0.0), #30537 (Bump phpseclib/phpseclib from 2.0.9 to 2.0.10), #28331 (DB issues after upgrade from 9.1.5 to 10.0.2), #5176 (Preview for avi files), and #4785 (OC 5.0.10 Gallery bug: Gallery treats a ZIP file as image and tries to show a preview image for a zip file .zip). |
seems the version expiry code is triggering the thumbnail/preview deletion code which itself is running into the error. we should probably catch the error in the preview handler in case we try to delete a preview for an already deleted version. now it's not guaranteed that the current state works, maybe it's actually leaving the previews on-disk... needs to be tested. |
Steps to reproduce
Expected resultNo errors Actual result
Versionv10.0.10 from git Bisect points at commit b09846b which points to b09846b regarding the change related to version previews. This is a regression from v10.0.8 |
From what I see in past versions we didn't call |
The issue happens because the $path seems to be a relative path instead of absolute and looks like "/test.txt.v123456678". It also only does this There are two fixes we can do here:
|
hmmm, making I diffed the files_versions/lib/Storage.php class with v10.0.8 and there was no notable changes, so the path was always absolute. It seems that in v10.0.8 the Preview::post_delete was mostly based on the logged in user. Not sure how this could even work back then with cron where there is no logged in user, maybe it didn't and just failed silently. |
I've checked v10.0.8 and observed the following:
The assumption that $path is relative in v10.0.9+ is wrong. We better not change the semantics of the hook as it could break other implementations. When running in cron I suggest to pass in the user id argument as we have it. If no user id argument is given, the handler should abort and log a debug message about a missing arg. |
Hmm even user id is not enough, and neither is the relative path "/test.txt.v123456789" because it's missing the "/files_version" prefix. Looks like the prefix specified in "post_delete_versions" is incorrect. |
It seems that even fixing the prefix and passing in the user is not enough: the v10.0.10 code tries to get "/$user/files/test.txt.v123456789" which of course it cannot find. We'll likely need to implement version-specific code here, maybe in "post_delete_versions" instead of relying on "post_delete". |
also from what I saw, the preview folders location for versions has changed and is now a subdir of "$fileid/$timestamp" instead of having its own fileid, so it is likely that deletion needs to be done completely differently. @DeepDiver1975 I'll need your guidance to continue here. Here's my PR so far: #33188 Next up would be to write new methods "Preview::prepare_delete_versions" and "Preview::post_delete_versions" with its own specific code. I'm not sure if Preview::deleteAllPreviews() is the right approach now with the new versions location. Also it seems that |
and since we're using the node API now I'm wondering whether the query should rather go to "meta" to get the versioned node, then have logic in the preview delete methods that detect the versioned node and know where to delete the thumbnails in "$fileid/$timestamp". |
The previews behavior is also a little bit strange when restoring a file: since the original file gets overwritten, the folder "thumbnails/$fileid" gets deleted, so all version previews also get deleted. At that stage, the versions tab still displays cached previews. Once I refresh the page, it will regenerate a preview for each version displayed in the versions tab. So version previews aren't kept. This is only a minor issue as regenerating is only for few entries and the final outcome is the same: all previews are displayed correctly in the web UI. I mostly wanted to see if there was an actual explicit code path for deleting a version thumbnail when restoring a version file. Since the parent folder gets deleted such logic wouldn't be needed. |
Once this is fixed, we'll likely also want an occ command and/or repair step to clean up all orphaned previews that failed to be deleted on expiration. Raised here: #33240 |
@PVince81 what is your expiration config? THX |
@DeepDiver1975 all default. I use the trick where I reset oc_jobs timestamp to make sure the cron job will run. |
I tried this with default behavior - and the occ command ... |
in the meantime, here's an attempt at mitigation: #34118 this would prevent the exception but would also leave some stray previews |
I realize that I missed one crucial step for reproduction, added now: |
After upgrading from 10.0.8 to 10.0.9 I'm getting a lot of this errors:
It doesn't look like a big error, but the log is full of those messages
The text was updated successfully, but these errors were encountered: