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); + } +});