diff --git a/CHANGELOG.md b/CHANGELOG.md index 99b13249..f7c4aa57 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,6 +36,18 @@ a version is cut. ceiling is, rather than simply refused; each person can have one archive being prepared at a time, for the same reason. +- **Zip downloads no longer hold up your email.** Preparing a large archive can take a while, and it + used to run on the same queue as everything else — so one big zip could delay every notification + email behind it. Zip building now has a queue of its own, and the Docker images run a second + background worker for it. + + **Upgrade note, manual installs only:** your background worker has to be told about the new queue, + or zips will never finish and nothing will say why. Edit + `/etc/systemd/system/projectsend-worker.service` so the `ExecStart` line reads + `queue:work --queue=default,zips …`, then `sudo systemctl daemon-reload && sudo systemctl restart + projectsend-worker`. Docker installations need no change. See INSTALL.md for the two-worker setup + if you would rather keep the two kinds of work apart. + ### Fixed - **Sessions no longer break behind a reverse proxy.** Signing in, or submitting the first-run setup diff --git a/INSTALL.md b/INSTALL.md index 2d67ee96..648a8c40 100644 --- a/INSTALL.md +++ b/INSTALL.md @@ -384,7 +384,7 @@ User=www-data Group=www-data Restart=always WorkingDirectory=/var/www/projectsend -ExecStart=/usr/bin/php artisan queue:work --tries=3 --backoff=3 +ExecStart=/usr/bin/php artisan queue:work --queue=default,zips --tries=3 --backoff=3 [Install] WantedBy=multi-user.target @@ -396,6 +396,15 @@ Then: sudo systemctl enable --now projectsend-worker ``` +`--queue=default,zips` matters. Building a zip runs on its own queue, so a worker that is not told +to watch `zips` will send email happily and never finish a single zip download — with nothing in any +log to say why. One worker watching both is fine for most installations; ordinary work is taken +first, and a large zip simply holds the worker while it runs. + +If zip downloads are heavily used and you would rather they never delayed email, run a second unit +with `--queue=zips` and narrow the first one to `--queue=default`. That is what the Docker images +do. + **Without this, no email is ever sent** and zip downloads never finish. `Restart=always` matters too: saving your email settings restarts the worker so it picks up the new values, and it needs to come back on its own. diff --git a/app/Modules/Files/Jobs/BuildZipDownloadJob.php b/app/Modules/Files/Jobs/BuildZipDownloadJob.php index 23f15826..8a4c607b 100644 --- a/app/Modules/Files/Jobs/BuildZipDownloadJob.php +++ b/app/Modules/Files/Jobs/BuildZipDownloadJob.php @@ -63,7 +63,20 @@ class BuildZipDownloadJob implements ShouldQueue public function __construct( private readonly int $zipDownloadId, - ) {} + ) { + // Its own queue, because $timeout is an hour and every shipped + // topology runs one worker: on the default queue a single large + // build holds up every notification email behind it. Set in the + // constructor rather than at the dispatch site so a second caller + // cannot forget it. + // + // A worker has to be listening. The images run a second one; a + // manual install whose worker command still says plain + // `queue:work` consumes `default` only, so INSTALL.md documents + // `--queue=default,zips` for the single-worker case — see the + // upgrade note in CHANGELOG.md. + $this->onQueue('zips'); + } public function handle(): void { diff --git a/compose.yaml b/compose.yaml index 4ff7b255..8fae83e5 100644 --- a/compose.yaml +++ b/compose.yaml @@ -56,7 +56,7 @@ services: args: WWWUSER: ${WWWUSER:-1000} WWWGROUP: ${WWWGROUP:-1000} - command: php artisan queue:work --tries=3 --backoff=3 + command: php artisan queue:work --queue=default --tries=3 --backoff=3 volumes: - .:/var/www/html - ../packages:/var/www/packages @@ -70,6 +70,27 @@ services: redis: condition: service_started + # Zip builds get their own worker: BuildZipDownloadJob allows itself an + # hour, and on a shared queue one large archive holds up every + # notification email behind it. + worker-zips: + build: + context: . + dockerfile: docker/app/Dockerfile + args: + WWWUSER: ${WWWUSER:-1000} + WWWGROUP: ${WWWGROUP:-1000} + command: php artisan queue:work --queue=zips --tries=1 + volumes: + - .:/var/www/html + - ../packages:/var/www/packages + restart: unless-stopped + depends_on: + db: + condition: service_healthy + redis: + condition: service_started + scheduler: build: context: . diff --git a/docker/production/supervisord.conf b/docker/production/supervisord.conf index 9c3f949e..62f56106 100644 --- a/docker/production/supervisord.conf +++ b/docker/production/supervisord.conf @@ -36,7 +36,7 @@ stderr_logfile_maxbytes=0 ; state, and a worker started before a deploy keeps running the old code ; until it exits; an hour bounds both without thrashing. [program:queue] -command=su-exec www-data php /var/www/html/artisan queue:work --max-time=3600 --tries=3 +command=su-exec www-data php /var/www/html/artisan queue:work --queue=default --max-time=3600 --tries=3 autostart=true autorestart=true priority=30 @@ -46,6 +46,21 @@ stdout_logfile_maxbytes=0 stderr_logfile=/dev/stderr stderr_logfile_maxbytes=0 +; Zip builds get their own worker. BuildZipDownloadJob allows itself an +; hour, and on the shared queue one large archive holds up every +; notification email queued behind it. --tries=1 matches the job, which +; records its own failure rather than being retried. +[program:queue-zips] +command=su-exec www-data php /var/www/html/artisan queue:work --queue=zips --max-time=3600 --tries=1 +autostart=true +autorestart=true +priority=31 +stopwaitsecs=3630 +stdout_logfile=/dev/stdout +stdout_logfile_maxbytes=0 +stderr_logfile=/dev/stderr +stderr_logfile_maxbytes=0 + ; schedule:work is the long-running equivalent of a per-minute cron entry, ; which is what a container should use — there is no crond here. [program:scheduler] diff --git a/tests/Feature/Files/ZipDownloadsTest.php b/tests/Feature/Files/ZipDownloadsTest.php index 905981c4..6746f65d 100644 --- a/tests/Feature/Files/ZipDownloadsTest.php +++ b/tests/Feature/Files/ZipDownloadsTest.php @@ -12,6 +12,7 @@ use App\Modules\Files\Models\ZipDownload; use App\Modules\Platform\Settings\Setting; use App\Modules\Platform\Settings\Settings; use Illuminate\Http\UploadedFile; +use Illuminate\Support\Facades\Queue; use Illuminate\Support\Facades\Storage; // QUEUE_CONNECTION=sync in phpunit.xml — BuildZipDownloadJob runs @@ -540,3 +541,20 @@ test('a traversing filename cannot escape the archive as a zip entry', function ->and($segments)->not->toContain('..') ->and($names[0])->toStartWith('Reports/'); }); + +// Its own queue, so one hour-long build cannot hold up every notification +// email behind it. Both shipped topologies run a worker for it; a manual +// install is told to watch `default,zips` (INSTALL.md, and the upgrade +// note in CHANGELOG.md), because a worker that is not listening finishes +// no zips and says nothing about why. +test('a zip build is queued away from ordinary work', function () { + Queue::fake(); + + $file = zipUploadFile($this->admin, 'a.pdf'); + + $this->actingAs($this->admin) + ->postJson('/zip-downloads', ['file_ids' => [$file->id]]) + ->assertOk(); + + Queue::assertPushed(BuildZipDownloadJob::class, fn (BuildZipDownloadJob $job): bool => $job->queue === 'zips'); +});