Merge pull request #1691 from denkfabrik-li/fix/file-bytes-after-commit

Delete a file's bytes when its transaction commits, not before

app/Modules/Files/Models/File.php

@@ -105,7 +105,20 @@ class File extends Model
             // because it needs the row's own pointers intact.
             app(FileVersions::class)->detachOnDelete($file);
 
-            app(FileDiskCleanup::class)->delete($file);
+            // The bytes go once the transaction holding this row commits,
+            // not alongside the row itself. A cascade — a folder subtree,
+            // 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.
+            //
+            // Outside a transaction the callback runs immediately, so
+            // deleting one file is unchanged. Nested transactions only fire
+            // it at the outermost commit, which is the case this is for.
+            $file->getConnection()->afterCommit(
+                fn () => app(FileDiskCleanup::class)->delete($file)
+            );
         });
     }
 

tests/Feature/Files/FileDiskCleanupTest.php

@@ -7,6 +7,7 @@ use App\Modules\Files\Models\File;
 use App\Modules\Files\Thumbnails\ImageAudience;
 use App\Modules\Files\Thumbnails\ImageRendition;
 use App\Modules\Files\Thumbnails\ThumbnailGenerator;
+use Illuminate\Support\Facades\DB;
 use Illuminate\Support\Facades\Storage;
 use Illuminate\Support\Str;
 
@@ -84,6 +85,48 @@ test('a file stored on the external disk is deleted from that disk, not the loca
     Storage::disk('files_external')->assertMissing($file->path);
 });
 
+// The row comes back on a rollback; the bytes have to still be under it.
+// Nothing restores them, so this is the one failure in the whole cleanup
+// path that cannot be repaired afterwards.
+test('a transaction that rolls back leaves the bytes where they were', function () {
+    $file = makeStoredFile();
+
+    try {
+        DB::transaction(function () use ($file): void {
+            $file->delete();
+
+            throw new RuntimeException('something later in the transaction failed');
+        });
+    } catch (RuntimeException) {
+        // The point of the test is what survives it.
+    }
+
+    expect(File::query()->find($file->id))->not->toBeNull();
+    Storage::disk('files')->assertExists($file->path);
+});
+
+// The shape an account deletion has: content disposal runs in its own
+// transaction, nested as a savepoint inside the caller's, and the write
+// that fails afterwards belongs to the caller.
+test('an outer rollback leaves the bytes even after the inner transaction committed', function () {
+    $file = makeStoredFile();
+
+    try {
+        DB::transaction(function () use ($file): void {
+            DB::transaction(function () use ($file): void {
+                $file->delete();
+            });
+
+            throw new RuntimeException('the account write after it failed');
+        });
+    } catch (RuntimeException) {
+        // Same.
+    }
+
+    expect(File::query()->find($file->id))->not->toBeNull();
+    Storage::disk('files')->assertExists($file->path);
+});
+
 test('a storage failure while cleaning up disk bytes never blocks the file from being deleted', function () {
     // A disk with no configured driver at all throws immediately on
     // resolution — proves a storage-layer failure (e.g. external storage