Never mark a zip download ready over an archive that was not written

BuildZipDownloadJob deferred every write to ZipArchive::close() but then
marked the row STATUS_READY regardless of the result:

- close() returns false when a source file was deleted between addFile()
  and close() (a concurrent staff delete runs FileDiskCleanup at once) or
  the disk filled up; the row went ready over an archive libzip never
  wrote, and the download controller X-Accel-served a path that isn't there.
- An archive that ended up with no entries (every selected file removed or
  its allowance spent before the queued job ran) is written as no file at
  all by libzip, yet close() still returns true — again marked ready.

Check both the close() return and the added-entry count, and fail the row
(deleting any partial archive) when either says nothing was written.

The job also had no $tries/$timeout/failed(): a build of up to MAX_FILES
sources runs past the worker's default 60s timeout, and the kill skips the
catch, stranding the row as PENDING while the frontend polls forever. Give
it room, run it once, and add a failed() backstop that fails a row still
pending (leaving an already-resolved one alone).

Finally, purge leftover zips/{id}.zip* by row id: a killed build leaves a
partial archive and libzip temp file with no path recorded, so the path
field alone never cleaned them up.

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();
         }
 

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()

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 () {