diff --git a/app/Modules/Files/Console/PurgeZipDownloadsCommand.php b/app/Modules/Files/Console/PurgeZipDownloadsCommand.php index 41330062..c00927a4 100644 --- a/app/Modules/Files/Console/PurgeZipDownloadsCommand.php +++ b/app/Modules/Files/Console/PurgeZipDownloadsCommand.php @@ -18,11 +18,25 @@ class PurgeZipDownloadsCommand extends Command { $stale = ZipDownload::query()->where('created_at', '<', now()->subDay())->get(); + // Listed once up front: the loop only deletes, so nothing it does + // changes what a later row would match. + $builtZips = collect(Storage::disk('files')->files('zips')); + foreach ($stale as $zipDownload) { + // Every artifact tied to this row's id, not just the recorded + // path: a build killed before it finished (worker timeout, disk + // full) leaves a partial archive — and libzip's temp file + // alongside it — with no path ever written back to the row. + $artifacts = $builtZips + ->filter(fn (string $path): bool => str_starts_with(basename($path), $zipDownload->id.'.zip')) + ->all(); + if ($zipDownload->path !== null) { - Storage::disk('files')->delete($zipDownload->path); + $artifacts[] = $zipDownload->path; } + Storage::disk('files')->delete(array_values(array_unique($artifacts))); + $zipDownload->delete(); } diff --git a/app/Modules/Files/Jobs/BuildZipDownloadJob.php b/app/Modules/Files/Jobs/BuildZipDownloadJob.php index e14f8a59..f29b5729 100644 --- a/app/Modules/Files/Jobs/BuildZipDownloadJob.php +++ b/app/Modules/Files/Jobs/BuildZipDownloadJob.php @@ -40,6 +40,24 @@ class BuildZipDownloadJob implements ShouldQueue { use Dispatchable, InteractsWithQueue, Queueable, SerializesModels; + /** + * A zip build is not usefully retryable — a source file that went + * missing mid-build, or an allowance spent while the job waited, makes + * a second attempt no likelier to succeed — so a failure is recorded + * once and surfaced to the requester rather than silently retried. + */ + public int $tries = 1; + + /** + * Building the archive is the whole job, and a large selection (up to + * ZipDownloadsController::MAX_FILES sources, some stream-copied from a + * remote disk) runs well past the queue worker's default 60s timeout. + * Without room the worker kills the process mid-build before the catch + * can run, stranding the row as PENDING forever; failed() is the + * backstop for when the kill lands anyway. + */ + public int $timeout = 3600; + public function __construct( private readonly int $zipDownloadId, ) {} @@ -112,12 +130,38 @@ class BuildZipDownloadJob implements ShouldQueue $totalSize += $this->addFolder($zip, $folder, $requester, $usedNames, $tempFiles, $visible, $skipped, $added); } - $zip->close(); + // ZipArchive defers every write to close(): a source file + // deleted after its addFile() (a concurrent staff delete runs + // FileDiskCleanup at once) or a full disk only surfaces here, + // as a false return. Its low-level warning is silenced (as with + // the @unlink cleanup below) so the return value is the signal + // we act on, deterministically, rather than an exception whose + // firing depends on the error_reporting level. An archive that + // ended up with no entries is the same kind of non-result — + // libzip writes no file for one at all, even though close() + // still returns true. Either way there is nothing to serve, so + // the row must not be marked ready over a missing or empty + // archive: the download controller would X-Accel a file that + // isn't there. + $written = @$zip->close(); foreach ($tempFiles as $tempFile) { @unlink($tempFile); } + if ($written !== true || $added === 0) { + Storage::disk('files')->delete($relativePath); + + $zipDownload->update([ + 'status' => ZipDownload::STATUS_FAILED, + 'error' => $added === 0 + ? 'None of the selected files were available to add to the archive.' + : 'The zip archive could not be written.', + ]); + + return; + } + $zipDownload->update([ 'status' => ZipDownload::STATUS_READY, 'path' => $relativePath, @@ -137,6 +181,27 @@ class BuildZipDownloadJob implements ShouldQueue } } + /** + * Runs when the queue gives up on the job — most importantly when the + * worker kills it for exceeding $timeout, which skips handle()'s own + * catch and would otherwise leave the row PENDING forever, polled by + * the frontend with no end. Only a row still pending is touched: a + * build that already resolved itself (ready or failed) is left alone. + */ + public function failed(?Throwable $exception): void + { + $zipDownload = ZipDownload::query()->find($this->zipDownloadId); + + if ($zipDownload === null || $zipDownload->status !== ZipDownload::STATUS_PENDING) { + return; + } + + $zipDownload->update([ + 'status' => ZipDownload::STATUS_FAILED, + 'error' => 'The zip archive could not be built.', + ]); + } + /** * A local-disk file is added by its real path (fast path). Anything * else gets stream-copied to a temp file first — ZipArchive::addFile() diff --git a/tests/Feature/Files/ZipDownloadsTest.php b/tests/Feature/Files/ZipDownloadsTest.php index ef2102a4..5474f269 100644 --- a/tests/Feature/Files/ZipDownloadsTest.php +++ b/tests/Feature/Files/ZipDownloadsTest.php @@ -5,6 +5,7 @@ declare(strict_types=1); use App\Models\User; use App\Modules\Audit\Action; use App\Modules\Audit\ActivityLog; +use App\Modules\Files\Jobs\BuildZipDownloadJob; use App\Modules\Files\Models\File; use App\Modules\Files\Models\Folder; use App\Modules\Files\Models\ZipDownload; @@ -239,6 +240,117 @@ test('the purge command removes zip downloads and files older than 24 hours', fu ->and(Storage::disk('files')->exists('zips/new.zip'))->toBeTrue(); }); +// The store guard rejects an empty selection, but a file selected and then +// removed before the queued job runs leaves nothing to add. libzip writes +// no file at all for a zero-entry archive, so a "ready" row would point the +// download controller at a path that does not exist. +test('a build with no available files is marked failed rather than ready over an empty archive', function () { + $file = zipUploadFile($this->admin, 'gone.pdf'); + + $zipDownload = ZipDownload::query()->create([ + 'requested_by' => $this->admin->id, + 'status' => ZipDownload::STATUS_PENDING, + 'file_ids' => [$file->id], + 'folder_ids' => [], + ]); + + // Gone from the requester's view by the time the job builds the archive. + $file->delete(); + + (new BuildZipDownloadJob($zipDownload->id))->handle(); + + $zipDownload->refresh(); + + expect($zipDownload->status)->toBe(ZipDownload::STATUS_FAILED) + ->and($zipDownload->path)->toBeNull() + ->and(Storage::disk('files')->exists("zips/{$zipDownload->id}.zip"))->toBeFalse(); +}); + +// ZipArchive reads each source only at close(); if the bytes vanish in +// between (a staff delete triggers FileDiskCleanup at once) close() returns +// false. The row must fail rather than go ready over an archive that was +// never actually written to disk. +test('a source file deleted after it was queued fails the build instead of serving a broken archive', function () { + $file = zipUploadFile($this->admin, 'vanishing.pdf'); + + $zipDownload = ZipDownload::query()->create([ + 'requested_by' => $this->admin->id, + 'status' => ZipDownload::STATUS_PENDING, + 'file_ids' => [$file->id], + 'folder_ids' => [], + ]); + + // The row stays visible, but its bytes are gone before the build closes. + Storage::disk('files')->delete($file->path); + + (new BuildZipDownloadJob($zipDownload->id))->handle(); + + $zipDownload->refresh(); + + expect($zipDownload->status)->toBe(ZipDownload::STATUS_FAILED) + ->and($zipDownload->path)->toBeNull() + ->and(Storage::disk('files')->exists("zips/{$zipDownload->id}.zip"))->toBeFalse(); +}); + +// A build that blows the worker timeout is killed mid-run: handle()'s own +// catch never executes, so without this hook the row would poll as pending +// forever and the frontend would never stop. +test('the failed hook fails a still-pending row when the worker gives up on the job', function () { + $zipDownload = ZipDownload::query()->create([ + 'requested_by' => $this->admin->id, + 'status' => ZipDownload::STATUS_PENDING, + ]); + + (new BuildZipDownloadJob($zipDownload->id))->failed(new RuntimeException('timed out')); + + $zipDownload->refresh(); + + expect($zipDownload->status)->toBe(ZipDownload::STATUS_FAILED) + ->and($zipDownload->error)->not->toBeNull(); +}); + +// A late failure signal (a retry racing a build that already finished) must +// not overwrite a row that already delivered a ready archive. +test('the failed hook leaves a row that already finished alone', function () { + $zipDownload = ZipDownload::query()->create([ + 'requested_by' => $this->admin->id, + 'status' => ZipDownload::STATUS_READY, + 'path' => 'zips/whatever.zip', + ]); + + (new BuildZipDownloadJob($zipDownload->id))->failed(new RuntimeException('too late')); + + expect($zipDownload->refresh()->status)->toBe(ZipDownload::STATUS_READY); +}); + +test('the build job runs once and allows enough time for a large archive', function () { + $job = new BuildZipDownloadJob(1); + + expect($job->tries)->toBe(1) + ->and($job->timeout)->toBeGreaterThan(60); +}); + +// A build killed before it finished (worker timeout, full disk) leaves a +// partial archive — and libzip's temp file beside it — on disk but never +// writes a path back to the row. Purge keys off the row id so it clears +// them anyway. +test('the purge command removes leftover archives even when the row never recorded a path', function () { + $row = ZipDownload::query()->create([ + 'requested_by' => $this->admin->id, + 'status' => ZipDownload::STATUS_FAILED, + ]); + $row->forceFill(['created_at' => now()->subDays(2)])->save(); + + Storage::disk('files')->put("zips/{$row->id}.zip", 'partial'); + Storage::disk('files')->put("zips/{$row->id}.zip.tmp0a1b2c", 'libzip temp'); + + $this->artisan('projectsend:purge-zip-downloads')->assertSuccessful(); + + expect(ZipDownload::query()->find($row->id))->toBeNull() + ->and(Storage::disk('files')->exists("zips/{$row->id}.zip"))->toBeFalse() + ->and(Storage::disk('files')->exists("zips/{$row->id}.zip.tmp0a1b2c"))->toBeFalse(); +}); + // original_name is uploader-chosen and validated only for length, so it // must not be able to steer where an entry lands inside the archive. test('a traversing filename cannot escape the archive as a zip entry', function () {