From 4164678ebc91a85caf2e60ef40bcd77e97eb6423 Mon Sep 17 00:00:00 2001 From: denkfabrik-li <274324701+denkfabrik-li@users.noreply.github.com> Date: Fri, 28 Aug 2026 03:14:09 +0200 Subject: [PATCH] Delete a file's renditions even when its own disk cannot be resolved FileDiskCleanup wraps both deletions in one try. The first is the original upload, on whatever disk the row names; the second is every cached rendition, always on the local files disk. Storage::disk() throws outright for a name with no configured driver -- which is the state the original's disk is in whenever this fails at all -- so the catch swallowed it and the renditions were never reached. Nothing looks for them afterwards. OrphanFileScanner skips the rendition directories on purpose (they are derived artifacts, never orphaned uploads), so a file whose external disk had been removed or renamed kept every cached copy of itself, indefinitely, on the disk that was working. The two attempts are now separate, each with the same tolerance the class was written for: a storage failure still never turns a delete click into a 500, and the warning is still the report. While here, the comment in File::booted() that justifies deferring the byte removal claimed "the worst case is bytes left on disk with no row, which OrphanFileScanner already finds and reports". Not on this path: the row is soft-deleted, and knownPaths() counts a trashed row's path as claimed -- deliberately, so a scan never offers to double-adopt a file still inside its erasure grace period. The comment now says what actually happens. One test: a file whose disk cannot be resolved loses its renditions. It goes red without the fix, next to the existing test that the delete itself still succeeds. --- app/Modules/Files/FileDiskCleanup.php | 32 +++++++++++++++++---- app/Modules/Files/Models/File.php | 8 ++++-- tests/Feature/Files/FileDiskCleanupTest.php | 24 ++++++++++++++++ 3 files changed, 57 insertions(+), 7 deletions(-) diff --git a/app/Modules/Files/FileDiskCleanup.php b/app/Modules/Files/FileDiskCleanup.php index 19ed513d..0f25435c 100644 --- a/app/Modules/Files/FileDiskCleanup.php +++ b/app/Modules/Files/FileDiskCleanup.php @@ -24,15 +24,37 @@ class FileDiskCleanup { public function delete(File $file): void { - try { - Storage::disk($file->disk)->delete($file->path); + $this->attempt($file, fn () => Storage::disk($file->disk)->delete($file->path)); - // Every rendition, for every audience — a deleted file's bytes - // must not survive on disk because whoever wrote the cleanup - // only knew about the one copy they had in mind. + // Every rendition, for every audience — a deleted file's bytes must + // not survive on disk because whoever wrote the cleanup only knew + // about the one copy they had in mind. + // + // Attempted separately from the original above, not because the two + // are unrelated but because they are on different disks: renditions + // are always local, and Storage::disk() throws outright for a name + // with no configured driver — which is exactly the state the + // original's disk is in when this fails at all. Sharing one `try` + // meant a file whose source disk had been removed kept every cached + // copy of itself, and nothing looks for those again: + // OrphanFileScanner skips the rendition directories on purpose. + $this->attempt($file, function () use ($file): void { foreach (ThumbnailGenerator::pathsFor($file->id, $file->mime_type) as $renditionPath) { Storage::disk('files')->delete($renditionPath); } + }); + } + + /** + * Deliberately tolerant, as the class docblock says: the warning is the + * whole report. Nothing else will find these bytes -- the row is + * soft-deleted, and OrphanFileScanner::knownPaths() counts a trashed + * row's path as claimed, so a scan never lists it. + */ + private function attempt(File $file, callable $work): void + { + try { + $work(); } catch (Throwable $exception) { Log::warning('Could not remove disk bytes for deleted file '.$file->id.': '.$exception->getMessage()); } diff --git a/app/Modules/Files/Models/File.php b/app/Modules/Files/Models/File.php index 53802a4e..9f628b76 100644 --- a/app/Modules/Files/Models/File.php +++ b/app/Modules/Files/Models/File.php @@ -110,8 +110,12 @@ class File extends Model // an account's content — deletes many rows in one transaction, // and anything that rolls it back afterwards puts every row // back while the bytes are already gone: a loss nothing can - // undo. Deferred, the worst case is bytes left on disk with no - // row, which OrphanFileScanner already finds and reports. + // undo. Deferred, the worst case is bytes left on disk with a + // row that is only trashed, and a scan will not offer those: + // OrphanFileScanner::knownPaths() counts a trashed row's path + // as claimed, on purpose, so nothing double-adopts a file still + // inside its erasure grace period. FileDiskCleanup's warning is + // therefore the only record that it happened. // // Outside a transaction the callback runs immediately, so // deleting one file is unchanged. Nested transactions only fire diff --git a/tests/Feature/Files/FileDiskCleanupTest.php b/tests/Feature/Files/FileDiskCleanupTest.php index 122643d9..17e3326b 100644 --- a/tests/Feature/Files/FileDiskCleanupTest.php +++ b/tests/Feature/Files/FileDiskCleanupTest.php @@ -138,3 +138,27 @@ test('a storage failure while cleaning up disk bytes never blocks the file from expect(File::withTrashed()->findOrFail($file->id)->trashed())->toBeTrue(); }); + +// The renditions are always on the local disk, whatever the original's +// disk is — so a source disk that cannot even be resolved is no reason to +// keep them. Nothing else would: OrphanFileScanner skips the rendition +// directories on purpose, and the trashed row still claims its own path. +test('a source disk that cannot be resolved does not keep the renditions alive', function () { + $file = makeStoredFile([ + 'disk' => 'nonexistent-disk', + 'path' => 'whatever.jpg', + 'mime_type' => 'image/jpeg', + ]); + + $paths = ThumbnailGenerator::pathsFor($file->id, 'image/jpeg'); + + foreach ($paths as $path) { + Storage::disk('files')->put($path, 'fake-thumbnail-bytes'); + } + + $this->actingAs($this->admin)->delete("/files/{$file->id}")->assertRedirect(); + + foreach ($paths as $path) { + Storage::disk('files')->assertMissing($path); + } +});