Merge pull request #1726 from denkfabrik-li/fix/rendition-cleanup-independent

FileDiskCleanup::delete() wrapped two deletions in one try: the original upload, on whatever disk the row names, and every cached rendition, which is always on the local files disk. Storage::disk() throws outright for a name with no configured driver -- precisely 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, as derived artifacts rather than orphaned uploads. A file whose external disk had been removed or renamed therefore kept every cached copy of itself indefinitely on the disk that still worked, including the client-facing ones, which for a shared image may be the only copies anyone ever generated.

The two attempts are now separate, each with the tolerance the class was written for: a storage failure still never turns a delete click into a 500, and the warning is still the whole report.

Also corrected: File::booted() justified deferring the byte removal with "the worst case is bytes left on disk with no row, which OrphanFileScanner already finds and reports". That is not 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, which is that FileDiskCleanup's warning is the only record.

Verified before merging: 8 passed on the trial-merge, 1 failed / 7 passed with app/ reset.

Reported and fixed by @denkfabrik-li.

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

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

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