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