From 65e7f37d36b8a31058589c3a666827ca065793fe Mon Sep 17 00:00:00 2001
From: denkfabrik-li <274324701+denkfabrik-li@users.noreply.github.com>
Date: Wed, 26 Aug 2026 02:15:34 +0200
Subject: [PATCH] Delete a file's bytes when its transaction commits, not
 before
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

File::booted() removed the bytes the moment a row was deleted. For a
single file that is right. Two paths delete files inside a transaction,
though, and both delete many at once: FolderService::delete() takes a
folder's whole subtree, and DeletedAccountContent::cascadeDelete() takes
everything an account uploaded.

Anything that rolls either transaction back puts every row back while the
bytes are already gone. A transaction exists to make a set of writes
undoable, and removing the bytes was the one write in that set that
nothing can undo. The account path is the sharper one: since content
disposal is nested inside the caller's transaction, the write that fails
need not be in this code at all.

The two failure directions are not equal. Bytes gone with the rows
restored leaves rows pointing at nothing and no way back. Rows gone with
the bytes left leaves orphans on disk, which OrphanFileScanner already
exists to find. Defer to the recoverable one.

Three properties this relies on, all of them checked rather than assumed:
without a pending transaction the callback runs immediately, so a single
delete is unchanged; a savepoint committing inside a larger transaction
does not fire it, which is exactly the account case; and the connection
is the row's own rather than whichever is default.

detachOnDelete stays inside the transaction — it repairs the version
chain's pointers, which is database work that must roll back with
everything else.
---
 app/Modules/Files/Models/File.php           | 15 ++++++-
 tests/Feature/Files/FileDiskCleanupTest.php | 43 +++++++++++++++++++++
 2 files changed, 57 insertions(+), 1 deletion(-)

diff --git a/app/Modules/Files/Models/File.php b/app/Modules/Files/Models/File.php
index 05dbb24c..53802a4e 100644
--- a/app/Modules/Files/Models/File.php
+++ b/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)
+            );
         });
     }
 
diff --git a/tests/Feature/Files/FileDiskCleanupTest.php b/tests/Feature/Files/FileDiskCleanupTest.php
index 00e59568..122643d9 100644
--- a/tests/Feature/Files/FileDiskCleanupTest.php
+++ b/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