From 16787cf697e3cf8fcc7f127820bc1d8c4383ab28 Mon Sep 17 00:00:00 2001 From: denkfabrik-li <274324701+denkfabrik-li@users.noreply.github.com> Date: Wed, 26 Aug 2026 01:57:05 +0200 Subject: [PATCH 1/2] Record which files a zip actually contains MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A zip download's row stores what was asked for — some file ids, some folder ids — and the download action resolved that selection a second time, when the archive was collected, to decide what to log as downloaded. The two are not the same thing. Folder contents are resolved against the scope as it stands at that moment, and an archive is written some time before it is fetched. Add a file to the folder in between and it was logged as downloaded without ever having been in the zip. Move one out of the folder and it was handed over without being logged at all. The same goes for a file that expired or otherwise left the requester's scope after the build: its bytes are in the archive either way. Nothing about this is visible to anyone — the download count on the file is simply wrong. The job already walks exactly the set that goes in, and already counted it for file_count. It now keeps the ids rather than a tally, and the download action logs those. count() gives back the number it was keeping before. Rows written before this column existed fall back to resolving the selection, which is what they were built for; the purge command clears them within a day. --- .../Controllers/ZipDownloadsController.php | 52 ++++++++++++++++--- .../Files/Jobs/BuildZipDownloadJob.php | 23 ++++---- app/Modules/Files/Models/ZipDownload.php | 2 + ...contained_files_to_zip_downloads_table.php | 32 ++++++++++++ tests/Feature/Files/ZipDownloadsTest.php | 38 ++++++++++++++ 5 files changed, 132 insertions(+), 15 deletions(-) create mode 100644 database/migrations/2026_09_04_090000_add_contained_files_to_zip_downloads_table.php diff --git a/app/Modules/Files/Http/Controllers/ZipDownloadsController.php b/app/Modules/Files/Http/Controllers/ZipDownloadsController.php index b31f11cc..b8eeae81 100644 --- a/app/Modules/Files/Http/Controllers/ZipDownloadsController.php +++ b/app/Modules/Files/Http/Controllers/ZipDownloadsController.php @@ -18,6 +18,7 @@ use App\Modules\Files\Uploads\StoreUploadedFile; use App\Modules\Platform\Settings\Setting; use App\Modules\Platform\Settings\Settings; use App\Support\ContentDisposition; +use Illuminate\Database\Eloquent\Collection; use Illuminate\Http\JsonResponse; use Illuminate\Http\Request; use Illuminate\Http\Response; @@ -200,9 +201,50 @@ class ZipDownloadsController extends Controller */ private function logContainedDownloads(ZipDownload $zipDownload, User $requester): void { - // Same per-file filter the job used to decide what actually went - // into the archive, so the log records what was really downloaded - // rather than everything that happened to sit in the folder. + foreach ($this->containedFiles($zipDownload, $requester) as $file) { + $this->activity->log(Action::FileDownloaded, subject: $file); + } + } + + /** + * The files this archive actually holds. + * + * The job records them as it writes them, so this describes a + * delivery rather than a selection. Resolving the folders again here + * would answer a different question — what the selection means *now* + * — and the two drift the moment a folder changes between the build + * and the fetch: a file added afterwards was counted as downloaded + * without ever being in the archive, and one moved out of the folder + * was handed over without being counted. + * + * Deliberately not filtered by what the requester may see today + * either. The bytes are in the archive already; a file that has since + * expired or left their scope is still being given to them, and a + * count that quietly dropped it would understate what was taken. + * + * @return Collection + */ + private function containedFiles(ZipDownload $zipDownload, User $requester): Collection + { + $contained = $zipDownload->contained_file_ids; + + if ($contained === null) { + return $this->resolveSelection($zipDownload, $requester); + } + + return File::query()->whereIn('id', $contained)->get(); + } + + /** + * What an archive built before the job recorded its contents is taken + * to hold: the selection, resolved again, which is how this worked + * throughout. Only reachable for rows written by an older release, + * and PurgeZipDownloadsCommand removes those within a day. + * + * @return Collection + */ + private function resolveSelection(ZipDownload $zipDownload, User $requester): Collection + { $visible = $this->viewable->for($requester); $fileIds = collect($zipDownload->file_ids); @@ -220,9 +262,7 @@ class ZipDownloadsController extends Controller // further past it. $skipped = collect($zipDownload->skipped_files ?? [])->pluck('id')->all(); - foreach ((clone $visible)->whereIn('id', $fileIds->unique())->whereNotIn('id', $skipped)->get() as $file) { - $this->activity->log(Action::FileDownloaded, subject: $file); - } + return (clone $visible)->whereIn('id', $fileIds->unique())->whereNotIn('id', $skipped)->get(); } private function filenameFor(ZipDownload $zipDownload): string diff --git a/app/Modules/Files/Jobs/BuildZipDownloadJob.php b/app/Modules/Files/Jobs/BuildZipDownloadJob.php index 23f15826..a72674e5 100644 --- a/app/Modules/Files/Jobs/BuildZipDownloadJob.php +++ b/app/Modules/Files/Jobs/BuildZipDownloadJob.php @@ -109,9 +109,12 @@ class BuildZipDownloadJob implements ShouldQueue $tempFiles = []; $skipped = []; - // Counted rather than derived from $usedNames, which also - // holds the folder entry names. - $added = 0; + // Collected rather than derived from $usedNames, which also + // holds the folder entry names. Recording the ids, not just a + // count, is what lets the download action log exactly what it + // hands over instead of resolving the selection a second time + // against a scope that may have moved since. + $addedIds = []; foreach ((clone $visible)->whereIn('id', $zipDownload->file_ids)->get() as $file) { // Re-checked here for the same reason visibility is: the @@ -126,11 +129,11 @@ class BuildZipDownloadJob implements ShouldQueue $entryName = $this->dedupeName($usedNames, $this->entrySegment($file->original_name)); $zip->addFile($this->localPathFor($file, $tempFiles), $entryName); $totalSize += $file->size; - $added++; + $addedIds[] = $file->id; } foreach (Folder::query()->whereIn('id', $zipDownload->folder_ids)->get() as $folder) { - $totalSize += $this->addFolder($zip, $folder, $requester, $usedNames, $tempFiles, $visible, $skipped, $added); + $totalSize += $this->addFolder($zip, $folder, $requester, $usedNames, $tempFiles, $visible, $skipped, $addedIds); } // Re-checked here, not only in ZipDownloadsController: the @@ -173,7 +176,7 @@ class BuildZipDownloadJob implements ShouldQueue @unlink($tempFile); } - if ($written !== true || $added === 0) { + if ($written !== true || $addedIds === []) { if ($written !== true) { // What the requester sees stays generic: a libzip // string means nothing to them and can name a server @@ -205,7 +208,8 @@ class BuildZipDownloadJob implements ShouldQueue 'status' => ZipDownload::STATUS_READY, 'path' => $relativePath, 'total_size' => $totalSize, - 'file_count' => $added, + 'file_count' => count($addedIds), + 'contained_file_ids' => $addedIds, 'skipped_files' => $skipped === [] ? null : $skipped, ]); } catch (Throwable $e) { @@ -304,8 +308,9 @@ class BuildZipDownloadJob implements ShouldQueue * @param array $tempFiles * @param Builder $visible every file the requester may read * @param list $skipped + * @param list $addedIds every file really written into the archive */ - private function addFolder(ZipArchive $zip, Folder $folder, User $requester, array &$usedNames, array &$tempFiles, Builder $visible, array &$skipped, int &$added): int + private function addFolder(ZipArchive $zip, Folder $folder, User $requester, array &$usedNames, array &$tempFiles, Builder $visible, array &$skipped, array &$addedIds): int { $allowance = app(DownloadAllowance::class); @@ -331,7 +336,7 @@ class BuildZipDownloadJob implements ShouldQueue $entryPath = $this->dedupeName($usedNames, $entryPath); $zip->addFile($this->localPathFor($file, $tempFiles), $entryPath); $totalSize += $file->size; - $added++; + $addedIds[] = $file->id; } return $totalSize; diff --git a/app/Modules/Files/Models/ZipDownload.php b/app/Modules/Files/Models/ZipDownload.php index 41c0e5fc..46db22a8 100644 --- a/app/Modules/Files/Models/ZipDownload.php +++ b/app/Modules/Files/Models/ZipDownload.php @@ -22,6 +22,7 @@ use Illuminate\Support\Carbon; * @property string|null $error * @property list $file_ids * @property list $folder_ids + * @property list|null $contained_file_ids * @property list|null $skipped_files * @property Carbon|null $delivered_at */ @@ -40,6 +41,7 @@ class ZipDownload extends Model return [ 'file_ids' => 'array', 'folder_ids' => 'array', + 'contained_file_ids' => 'array', 'skipped_files' => 'array', 'delivered_at' => 'datetime', ]; diff --git a/database/migrations/2026_09_04_090000_add_contained_files_to_zip_downloads_table.php b/database/migrations/2026_09_04_090000_add_contained_files_to_zip_downloads_table.php new file mode 100644 index 00000000..e346f927 --- /dev/null +++ b/database/migrations/2026_09_04_090000_add_contained_files_to_zip_downloads_table.php @@ -0,0 +1,32 @@ +json('contained_file_ids')->nullable()->after('folder_ids'); + }); + } + + public function down(): void + { + Schema::table('zip_downloads', function (Blueprint $table) { + $table->dropColumn('contained_file_ids'); + }); + } +}; diff --git a/tests/Feature/Files/ZipDownloadsTest.php b/tests/Feature/Files/ZipDownloadsTest.php index 905981c4..5911d14b 100644 --- a/tests/Feature/Files/ZipDownloadsTest.php +++ b/tests/Feature/Files/ZipDownloadsTest.php @@ -176,6 +176,44 @@ test('the download endpoint logs a FileDownloaded entry for every bundled file', ->assertHeader('Content-Disposition', 'attachment; filename="Reports.zip"'); expect(ActivityLog::query()->where('action', Action::FileDownloaded)->where('subject_id', $file->id)->exists())->toBeTrue(); + + expect($zipDownload->fresh()->contained_file_ids)->toBe([$file->id]); +}); + +test('a file added to the folder after the build is not counted as downloaded', function () { + $folder = Folder::query()->create(['name' => 'Reports']); + $bundled = zipUploadFile($this->admin, 'a.pdf', $folder->id); + + $response = $this->actingAs($this->admin)->postJson('/zip-downloads', ['folder_ids' => [$folder->id]])->assertOk(); + $zipDownload = ZipDownload::query()->findOrFail($response->json('id')); + + // The archive is written by now. Anything that lands in the folder + // from here on is not in it. + $late = zipUploadFile($this->admin, 'b.pdf'); + $late->update(['folder_id' => $folder->id]); + + $this->actingAs($this->admin)->get("/zip-downloads/{$zipDownload->id}/download")->assertOk(); + + expect(zipEntryNames($zipDownload))->toBe(['Reports/a.pdf']) + ->and($bundled->downloads()->count())->toBe(1) + ->and($late->downloads()->count())->toBe(0); +}); + +test('a file moved out of the folder after the build is still counted', function () { + $folder = Folder::query()->create(['name' => 'Reports']); + $elsewhere = Folder::query()->create(['name' => 'Elsewhere']); + $file = zipUploadFile($this->admin, 'a.pdf', $folder->id); + + $response = $this->actingAs($this->admin)->postJson('/zip-downloads', ['folder_ids' => [$folder->id]])->assertOk(); + $zipDownload = ZipDownload::query()->findOrFail($response->json('id')); + + // Moving it does not take its bytes back out of the archive, so the + // download still hands it over and still has to say so. + $file->update(['folder_id' => $elsewhere->id]); + + $this->actingAs($this->admin)->get("/zip-downloads/{$zipDownload->id}/download")->assertOk(); + + expect($file->downloads()->count())->toBe(1); }); test('a pending zip download 404s until ready', function () { From acda732ff45dc2125b86fcf3c9c98541693af8fc Mon Sep 17 00:00:00 2001 From: denkfabrik-li <274324701+denkfabrik-li@users.noreply.github.com> Date: Wed, 26 Aug 2026 02:04:41 +0200 Subject: [PATCH 2/2] Enforce the download limit when a zip is delivered MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A download limit is checked when an archive is ordered and again while it is built, but it is only spent when the archive is collected. Nothing about ordering or building moves the count, so every check along the way sees an allowance that is still untouched. That turns a prepared archive into a voucher. Order the same limited file into ten archives and all ten pass, because at the point each one is checked nothing has been taken yet. Collect them all and the file has been downloaded ten times against a limit of one. The three endpoints are independent of the interface that normally drives them, so this needs nothing more than calling store() in a loop — and no timing luck at all, since the archives can be collected minutes apart. DownloadAllowance says of itself that six routes put a file's bytes on the wire and that every one of them asks, precisely because there is no choke point to put the rule in. The zip pair asked in the two places that do not count and not in the one that does. So the delivery re-checks what the archive holds, where the count actually moves. Refusing is 403, matching the single-file download route for the same situation. It is also the only one of the two candidates that reaches the person: an archive is fetched by navigating to it, and there is no error view for 422, so the message would be replaced by the framework's generic "something is broken" page. One refused file refuses the whole delivery, because nothing can be taken out of a finished archive without building it again. Ordering the same selection afresh is the way through — the build leaves the spent file out and names it in skipped_files, which the poll already reports. This is stricter than store(), which drops spent files from a selection and refuses only when nothing survives: there, a selection can still be narrowed, and here it cannot. Checking costs nothing where nothing is limited. An unlimited file is answered from its own column and never reaches a count. Claiming the delivery is a conditional update now rather than a read followed by a write. Two fetches of one archive arriving together both saw delivered_at unset and both wrote a full set of downloads, counting a single delivery twice — the same shape as the conditional increment that guards a share link's max_downloads. Only the fetch that moves the column logs anything; the other still receives the archive, which is the existing rule that re-fetching one prepared zip is one delivery. Two things this deliberately leaves alone. Simultaneous downloads of one file can still both pass before either is logged: that race is documented in DownloadAllowance, and closing it needs the counter column it explains why it does not have. And an archive already delivered stays fetchable for its 24 hours even once the limit is spent — one delivery, re-fetched, which is what that rule is for. An archive built before the job recorded its contents is handed over the way it always was, without this check. What it holds can only be guessed at by resolving the selection a second time, and guessing is exactly what must not decide a refusal: the same reconstruction refuses over files the archive does not hold and misses files it does. Those rows stop existing within a day or two of an upgrade, and until then they behave as they did before this change rather than worse. --- .../Controllers/ZipDownloadsController.php | 95 +++++++----- .../Files/DownloadLimitEnforcementTest.php | 143 ++++++++++++++++++ 2 files changed, 203 insertions(+), 35 deletions(-) diff --git a/app/Modules/Files/Http/Controllers/ZipDownloadsController.php b/app/Modules/Files/Http/Controllers/ZipDownloadsController.php index b8eeae81..7baf8509 100644 --- a/app/Modules/Files/Http/Controllers/ZipDownloadsController.php +++ b/app/Modules/Files/Http/Controllers/ZipDownloadsController.php @@ -181,8 +181,7 @@ class ZipDownloadsController extends Controller // Only the first time. Re-fetching one prepared archive is the // same delivery, not a fresh download of everything inside it. if ($zipDownload->delivered_at === null) { - $this->logContainedDownloads($zipDownload, $user); - $zipDownload->forceFill(['delivered_at' => now()])->save(); + $this->deliverOnce($zipDownload, $user); } $size = Storage::disk('files')->size($path); @@ -196,45 +195,71 @@ class ZipDownloadsController extends Controller } /** - * Every file actually bundled gets a FileDownloaded entry — otherwise - * a file's download history/count would silently miss zip downloads. + * Hand the archive over, once: refuse it if anything inside is out of + * allowance, otherwise count everything it holds as downloaded. + * + * This is the only point that spends a download limit, which is why + * it also has to be the point that enforces it. Building an archive + * takes nothing, so ordering the same limited file into any number of + * archives passes every check on the way — store() and the job both + * look at an allowance nothing has drawn on yet — and collecting them + * all afterwards would hand over more copies than the limit allows. + * + * One refused file refuses the whole delivery, because nothing can be + * taken out of a finished archive without building it again. Ordering + * the same selection afresh is the way through: the build leaves the + * spent file out and names it in skipped_files. + * + * An archive from before the job recorded its contents is handed over + * the way it always was, without this check. Its contents can only be + * guessed at by resolving the selection again, and guessing is exactly + * what must not decide a refusal: the same reconstruction both refuses + * over files the archive does not hold and misses files it does. Those + * rows stop existing within a day or two of an upgrade, and until then + * they behave as they did before this change rather than worse. */ - private function logContainedDownloads(ZipDownload $zipDownload, User $requester): void + private function deliverOnce(ZipDownload $zipDownload, User $requester): void { - foreach ($this->containedFiles($zipDownload, $requester) as $file) { + $recorded = $zipDownload->contained_file_ids; + + // What the job wrote down, read back as it stands — deliberately + // not filtered by what the requester may see today. The bytes are + // in the archive already, so a file that has since expired or left + // their scope is still being given to them, and a count that + // quietly dropped it would understate what was taken. + $contained = $recorded === null + ? $this->resolveSelection($zipDownload, $requester) + : File::query()->whereIn('id', $recorded)->get(); + + abort_if( + $recorded !== null + && $contained->contains(fn (File $file): bool => ! $this->allowance->allows($file, $requester)), + 403, + __('Those files have reached their download limit.'), + ); + + // Atomic, so two fetches arriving together are still one delivery: + // only the request that actually moves delivered_at logs anything. + // Same reasoning as the conditional increment guarding a share + // link's max_downloads in PublicShareController. The other request + // still receives the archive — that is the re-fetch rule above. + $claimed = ZipDownload::query() + ->whereKey($zipDownload->id) + ->whereNull('delivered_at') + ->update(['delivered_at' => now()]); + + if ($claimed === 0) { + return; + } + + // Every file actually bundled gets a FileDownloaded entry — + // otherwise a file's download history/count would silently miss + // zip downloads. + foreach ($contained as $file) { $this->activity->log(Action::FileDownloaded, subject: $file); } } - /** - * The files this archive actually holds. - * - * The job records them as it writes them, so this describes a - * delivery rather than a selection. Resolving the folders again here - * would answer a different question — what the selection means *now* - * — and the two drift the moment a folder changes between the build - * and the fetch: a file added afterwards was counted as downloaded - * without ever being in the archive, and one moved out of the folder - * was handed over without being counted. - * - * Deliberately not filtered by what the requester may see today - * either. The bytes are in the archive already; a file that has since - * expired or left their scope is still being given to them, and a - * count that quietly dropped it would understate what was taken. - * - * @return Collection - */ - private function containedFiles(ZipDownload $zipDownload, User $requester): Collection - { - $contained = $zipDownload->contained_file_ids; - - if ($contained === null) { - return $this->resolveSelection($zipDownload, $requester); - } - - return File::query()->whereIn('id', $contained)->get(); - } - /** * What an archive built before the job recorded its contents is taken * to hold: the selection, resolved again, which is how this worked diff --git a/tests/Feature/Files/DownloadLimitEnforcementTest.php b/tests/Feature/Files/DownloadLimitEnforcementTest.php index 913e35d3..c05b533a 100644 --- a/tests/Feature/Files/DownloadLimitEnforcementTest.php +++ b/tests/Feature/Files/DownloadLimitEnforcementTest.php @@ -236,3 +236,146 @@ test('re-fetching one prepared zip does not count as downloading everything agai expect($file->downloads()->count())->toBe(1) ->and($zip->fresh()->delivered_at)->not->toBeNull(); }); + +test('a prepared zip cannot be collected once its files have been spent elsewhere', function () { + $file = limitedFile(); + shareFileWith($file, $this->client); + + // Two archives of the same file, both ordered and both built before + // either is collected. Nothing is spent yet, so every check made + // while ordering and building passes for both of them. + $first = $this->actingAs($this->client) + ->postJson('/zip-downloads', ['file_ids' => [$file->id]])->assertOk(); + $second = $this->actingAs($this->client) + ->postJson('/zip-downloads', ['file_ids' => [$file->id]])->assertOk(); + + $firstZip = ZipDownload::query()->findOrFail($first->json('id')); + $secondZip = ZipDownload::query()->findOrFail($second->json('id')); + + expect($firstZip->file_count)->toBe(1) + ->and($secondZip->file_count)->toBe(1); + + $this->actingAs($this->client)->get("/zip-downloads/{$firstZip->id}/download")->assertOk(); + + // The single allowed download is now spent. The second archive was + // built while it still was not, and holding it must not be a way to + // take a copy that is no longer allowed. + $this->actingAs($this->client)->get("/zip-downloads/{$secondZip->id}/download")->assertForbidden(); + + expect($file->downloads()->count())->toBe(1) + ->and($secondZip->fresh()->delivered_at)->toBeNull(); +}); + +test('one spent file refuses the whole archive rather than part of it', function () { + $folder = makeFolder('Shared'); + + $spendable = limitedFile(['name' => 'Spendable', 'original_name' => 'spendable.pdf']); + $uncapped = limitedFile(['name' => 'Uncapped', 'original_name' => 'uncapped.pdf', 'download_limit' => null]); + + $spendable->update(['folder_id' => $folder->id]); + $uncapped->update(['folder_id' => $folder->id]); + + $staff = staffWithPermissions(['upload', 'edit_files', 'edit_others_files']); + + $response = $this->actingAs($staff) + ->postJson('/zip-downloads', ['folder_ids' => [$folder->id]])->assertOk(); + + $zip = ZipDownload::query()->findOrFail($response->json('id')); + expect($zip->file_count)->toBe(2); + + // Spent after the archive was built, so the copy inside it is one the + // limit no longer covers. + spend($spendable, $staff); + + $this->actingAs($staff)->get("/zip-downloads/{$zip->id}/download")->assertForbidden(); + + // Refused as a whole: the file that was still free is not counted as + // downloaded either, since nothing was handed over. + expect($uncapped->downloads()->count())->toBe(0); +}); + +test('a spent file that is not in the archive does not refuse it', function () { + $folder = makeFolder('Shared'); + + $bundled = limitedFile(['name' => 'Bundled', 'original_name' => 'bundled.pdf', 'download_limit' => null]); + $bundled->update(['folder_id' => $folder->id]); + + $staff = staffWithPermissions(['upload', 'edit_files', 'edit_others_files']); + + $response = $this->actingAs($staff) + ->postJson('/zip-downloads', ['folder_ids' => [$folder->id]])->assertOk(); + + $zip = ZipDownload::query()->findOrFail($response->json('id')); + + // Lands in the folder after the archive was written, and is already + // spent. The selection would resolve to it now; the archive does not + // hold it, so it has no say over handing that archive over. + $late = limitedFile(['name' => 'Late', 'original_name' => 'late.pdf']); + $late->update(['folder_id' => $folder->id]); + spend($late, $staff); + + $this->actingAs($staff)->get("/zip-downloads/{$zip->id}/download")->assertOk(); + + // Still only the download that spent it — the delivery neither + // refused over it nor counted it. + expect($bundled->downloads()->count())->toBe(1) + ->and($late->downloads()->count())->toBe(1); +}); + +test('an archive from before its contents were recorded is handed over as it always was', function () { + $folder = makeFolder('Shared'); + + $bundled = limitedFile(['name' => 'Bundled', 'original_name' => 'bundled.pdf', 'download_limit' => null]); + $bundled->update(['folder_id' => $folder->id]); + + $staff = staffWithPermissions(['upload', 'edit_files', 'edit_others_files']); + + $response = $this->actingAs($staff) + ->postJson('/zip-downloads', ['folder_ids' => [$folder->id]])->assertOk(); + + $zip = ZipDownload::query()->findOrFail($response->json('id')); + + // Stands in for a row written by an older release, which has no + // record of what went into the archive. + ZipDownload::query()->whereKey($zip->id)->update(['contained_file_ids' => null]); + + // A spent file joins the folder afterwards. Resolving the selection + // again — all such a row can do — would sweep it up and refuse over a + // file the archive does not hold, so this path refuses nothing. + $late = limitedFile(['name' => 'Late', 'original_name' => 'late.pdf']); + $late->update(['folder_id' => $folder->id]); + spend($late, $staff); + + $this->actingAs($staff)->get("/zip-downloads/{$zip->id}/download")->assertOk(); + + expect($bundled->downloads()->count())->toBe(1) + ->and($zip->fresh()->delivered_at)->not->toBeNull(); +}); + +test('two fetches of one archive arriving together still count as one delivery', function () { + $file = limitedFile(['download_limit' => null]); + shareFileWith($file, $this->client); + + $response = $this->actingAs($this->client) + ->postJson('/zip-downloads', ['file_ids' => [$file->id]])->assertOk(); + + $zip = ZipDownload::query()->findOrFail($response->json('id')); + + // Stands in for a second fetch of the same archive that arrives at + // the same moment and claims the delivery first. The request below + // has already read the row by then, so its own copy still says the + // archive has never been handed over — the check-then-set this + // replaced would believe it and count everything a second time. + ZipDownload::retrieved(function (ZipDownload $retrieved): void { + ZipDownload::query() + ->whereKey($retrieved->id) + ->whereNull('delivered_at') + ->update(['delivered_at' => now()]); + }); + + $this->actingAs($this->client)->get("/zip-downloads/{$zip->id}/download")->assertOk(); + + // Losing the claim means not logging: the fetch that won it is the + // one that counts, and here that is the stand-in, which logs nothing. + expect($file->downloads()->count())->toBe(0); +});