(WIP) Attempt to fix hard upload failures during quota events #1
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is an attempt to fix a backend fail triggered by quota issues while uploading.
Summary
I discovered this while trying to reproduce nextcloud#37578. Technically this may fix that issue as well (it's the same call/code path with a null parameter), but I was only able to reproduce the issue reported there under one condition: if upload would cause the user quota to be exceeded.
In the course of looking into the issue I could reproduce, I came up with this "fix". This area of code is not one I was at all
familiar with previously so I can't say I'd even give a "LGTM" even though I'm the one submitting it. There is an issue, but the best fix may me a variation.
In
prepareUpload()
the$request->getPath()
will contain something like:uploads/ncadmin/web-file-upload-464f2b6a78f780318dc46e3424dbc0fc-1680893450217
(note it doesn't have the chunk # unlike beforePut())uploads/ncadmin
Currently we pass the whole thing (1st one) here to prepareUpload() which fails under at least one confirmed scenario (quota exceeded situations).
Without this patch, uploads fail in an ugly way on the backend when the quota is exceeded:
TypeError: OCA\DAV\Upload\ChunkingV2Plugin::getUploadStorage(): Argument #1 ($targetPath) must be of type string, null given, called in /var/www/html/apps/dav/lib/Upload/ChunkingV2Plugin.php on line 285
This is not an issue if the quota isn't triggered. Though, interestingly, $uploadMetadata is always blank
As far as I can tell $uploadMetadata is always null.
uploadPath gets set in prepareUpload()
It gets set to either:
$uploadMetadata[self::UPLOAD_TARGET_PATH] or null
checkPrerequisuites() is called by beforePut() + beforeMove
But not by beforeDelete()
Among other things it checks if uploadPath is null
and throws a PreconditionFailed exception if so:
"Missing metadata for chunked upload"
Since checkPrerequisites() is never called in beforeDelete()...
beforeDelete() happily calls getUploadStorage() with a null...
uploadPath
Observations:
The main way uploadPath ends up null is...
...if $uploadMetadata[self::UPLOAD_TARGET_PATH] isn't set...
...in prepareUpload()...
...which gets $uploadMetadata via...
$this->cache->get($this->uploadFolder->getName());
TODO
Checklist