Fix deletion of partially-uploaded (pending) files#11655
Fix deletion of partially-uploaded (pending) files#11655
Conversation
Agent-Logs-Url: https://github.com/appwrite/appwrite/sessions/8d14b17a-78d8-48c6-b6d8-51d9697adde0 Co-authored-by: Meldiron <[email protected]>
Greptile SummaryThis PR fixes a server error that occurred when deleting a file whose chunked upload was cancelled before any bytes were written to disk. The root cause was that The fix wraps the Key points:
Confidence Score: 4/5Safe to merge for Local storage deployments; the broad exception catch warrants a follow-up for cloud storage adapters but is not a blocker. The core fix is correct and well-targeted for the described bug. The exception type thrown by the Local device (\Exception) matches what is caught. The e2e test exercises the fixed path end-to-end. The one concrete concern — that catch (\Exception) silently swallows unexpected errors from cloud storage adapters — is a real edge case but not likely to cause problems in normal usage for the majority of deployments. P2 test hygiene issues do not affect merge safety. src/Appwrite/Platform/Modules/Storage/Http/Buckets/Files/Delete.php — review the exception catch breadth if cloud storage adapters are a primary concern. Important Files Changed
Reviews (1): Last reviewed commit: "fix: allow deletion of partially-uploade..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } catch (\Exception) { | ||
| // If the partial upload chunks are already gone from the device | ||
| // (e.g. the upload never wrote anything to disk), treat it as deleted | ||
| // so the pending file document can still be removed from the database. | ||
| $deviceDeleted = true; | ||
| } |
There was a problem hiding this comment.
catch (\Exception) catches every exception thrown by abort(), not just the "directory doesn't exist" case. For the Local device this is safe — the only exception it throws is "File doesn't exist: …" when the tmp directory was never created. However, cloud storage adapters (S3, B2, etc.) can throw \Exception for transient reasons such as network timeouts, credential errors, or throttling. In those cases the multipart upload object may still exist on the provider's side, yet the code sets $deviceDeleted = true and removes the DB document, leaving orphaned (and potentially billable) partial-upload data with no DB reference to clean it up later.
Consider narrowing the catch to a known "not found" scenario, or at minimum logging the exception before swallowing it so unexpected failures surface in the error logs:
} catch (\Exception $e) {
// Chunks were never written to disk / remote storage; safe to remove the document.
// Log unexpected errors so they don't vanish silently.
\Utopia\Logger\Logger::warning('abort() failed for partial upload, treating as deleted: ' . $e->getMessage());
$deviceDeleted = true;
}(Adjust to whatever logging facility is already used in this codebase.)
|
|
||
| $this->assertEquals(404, $getResponse['headers']['status-code']); | ||
| } |
There was a problem hiding this comment.
The bucket created at the start of testDeletePartiallyUploadedFile is not deleted at the end of the test. Other tests in StorageBase that create their own buckets (e.g. testDeleteBucketFile) also clean up the bucket afterward. Leaving this bucket behind could accumulate leftover state across test runs. Consider adding a bucket-deletion call after the 404 assertion.
There was a problem hiding this comment.
@copilot Make sure to leanup bucket as suggested here
✨ Benchmark results
⚡ Benchmark Comparison
|
|
Found 4 test failures on Blacksmith runners: Failures
|
|
@copilot Address two reviews above, I pinged you in both |
Agent-Logs-Url: https://github.com/appwrite/appwrite/sessions/045e63f6-9a81-447b-ba79-7391ab97fb01 Co-authored-by: Meldiron <[email protected]>
Done in ecc76c7:
|
| // If the partial upload chunks are already gone from the device | ||
| // (e.g. the upload never wrote anything to disk), treat it as deleted | ||
| // so the pending file document can still be removed from the database. | ||
| $log->addTag('abortException', $e->getMessage()); |
There was a problem hiding this comment.
Not sure if we really want to add this...
Fixes: SER-1179
abort()exception propagation for partially-uploaded files inDelete.phpLog $logand capture exception as$eto surface unexpectedabort()failures via$log->addTag('abortException', $e->getMessage())StorageBase.php@error-suppression operators fromfopen/fread/fclose; add explicit assertion that file handle is validtestDeletePartiallyUploadedFileto avoid leftover state across test runs📍 Connect Copilot coding agent with Jira, Azure Boards or Linear to delegate work to Copilot in one click without leaving your project management tool.