diff --git a/CHANGELOG.md b/CHANGELOG.md index 0722ae60ce8..091d0e1b039 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,6 +41,15 @@ follows [Semantic Versioning](https://semver.org) guidelines. - Fixed a delete treating any error from Proxmox as "the virtual machine is gone". Only Proxmox's own "does not exist" counts now, so a node that is briefly unreachable can no longer get a server removed from Convoy while its virtual machine keeps running. +- Fixed a server's jobs getting permanently stuck after one slow request to Proxmox. The locks that keep two jobs of the + same kind from running on a server at once never expired, so a job the queue killed for running past its time limit + left its lock behind for good: every later job of that kind for the server -- including the force-stop that reinstall + and delete start with -- failed instantly until the lock was cleared from Redis by hand. Locks now expire shortly + after their job's time limit, a job that finds one held waits and retries, and the power and password jobs' time + limits no longer fall below a single Proxmox request. +- Fixed failing jobs retrying with no pause. A job whose request to Proxmox failed was retried at once, again and + again, until it ran out of time -- thousands of requests a minute against a node that was already struggling. Jobs + now wait a few seconds, then longer, between retries. ## v4.7.0-rc.2 diff --git a/app/Jobs/Concerns/LocksPerSubject.php b/app/Jobs/Concerns/LocksPerSubject.php new file mode 100644 index 00000000000..31d4a1030fd --- /dev/null +++ b/app/Jobs/Concerns/LocksPerSubject.php @@ -0,0 +1,31 @@ +expireAfter($this->timeout + 30) + ->releaseAfter(5); + } +} diff --git a/app/Jobs/Node/MonitorIsoDownloadJob.php b/app/Jobs/Node/MonitorIsoDownloadJob.php index e648dc1cbd9..ebf21cdd5ad 100644 --- a/app/Jobs/Node/MonitorIsoDownloadJob.php +++ b/app/Jobs/Node/MonitorIsoDownloadJob.php @@ -8,13 +8,15 @@ use Illuminate\Contracts\Queue\ShouldQueue; use Illuminate\Foundation\Bus\Dispatchable; use Illuminate\Queue\InteractsWithQueue; -use Illuminate\Queue\Middleware\WithoutOverlapping; +use Convoy\Jobs\Concerns\LocksPerSubject; use Illuminate\Queue\SerializesModels; use Illuminate\Support\Carbon; class MonitorIsoDownloadJob implements ShouldQueue { - use Dispatchable, InteractsWithQueue, Queueable, SerializesModels; + use Dispatchable, InteractsWithQueue, Queueable, SerializesModels, LocksPerSubject; + + public int $timeout = 60; public function retryUntil(): Carbon { @@ -25,9 +27,18 @@ public function __construct(protected int $isoId, protected string $upid) { } + /** + * Wait before retrying after an error instead of retrying in a tight loop. + * The last delay repeats until the job gives up. + */ + public function backoff(): array + { + return [3, 10, 30]; + } + public function middleware() { - return [new WithoutOverlapping("node:iso.download#{$this->isoId}")]; + return [$this->lockPerSubject($this->isoId)]; } public function handle(IsoMonitorService $service): void diff --git a/app/Jobs/Node/PruneUsersJob.php b/app/Jobs/Node/PruneUsersJob.php index 0c1e7183974..f93588368f1 100644 --- a/app/Jobs/Node/PruneUsersJob.php +++ b/app/Jobs/Node/PruneUsersJob.php @@ -22,6 +22,14 @@ public function __construct(protected int $nodeId) { } + /** + * Wait before retrying a failed sync instead of retrying at once. + */ + public function backoff(): array + { + return [10, 30]; + } + public function handle(UserPruneService $service): void { $node = Node::findOrFail($this->nodeId); diff --git a/app/Jobs/Node/SyncServerRateLimitsJob.php b/app/Jobs/Node/SyncServerRateLimitsJob.php index 82829755105..7c4ea63596a 100644 --- a/app/Jobs/Node/SyncServerRateLimitsJob.php +++ b/app/Jobs/Node/SyncServerRateLimitsJob.php @@ -20,6 +20,14 @@ public function __construct(protected int $nodeId) { } + /** + * Wait before retrying a failed sync instead of retrying at once. + */ + public function backoff(): array + { + return [10, 30]; + } + public function handle(ServerRateLimitsSyncService $service): void { $node = Node::findOrFail($this->nodeId); diff --git a/app/Jobs/Node/SyncServerUsagesJob.php b/app/Jobs/Node/SyncServerUsagesJob.php index 710ddaa5205..305b143bf05 100644 --- a/app/Jobs/Node/SyncServerUsagesJob.php +++ b/app/Jobs/Node/SyncServerUsagesJob.php @@ -20,6 +20,14 @@ public function __construct(protected int $nodeId) { } + /** + * Wait before retrying a failed sync instead of retrying at once. + */ + public function backoff(): array + { + return [10, 30]; + } + public function handle(ServerUsagesSyncService $service): void { $node = Node::findOrFail($this->nodeId); diff --git a/app/Jobs/Server/BuildServerJob.php b/app/Jobs/Server/BuildServerJob.php index 20abf957c83..fff07fdb44e 100644 --- a/app/Jobs/Server/BuildServerJob.php +++ b/app/Jobs/Server/BuildServerJob.php @@ -11,26 +11,32 @@ use Illuminate\Foundation\Bus\Dispatchable; use Illuminate\Queue\InteractsWithQueue; use Illuminate\Queue\Middleware\SkipIfBatchCancelled; -use Illuminate\Queue\Middleware\WithoutOverlapping; +use Convoy\Jobs\Concerns\LocksPerSubject; use Illuminate\Queue\SerializesModels; class BuildServerJob implements ShouldQueue { - use Dispatchable, InteractsWithQueue, Queueable, SerializesModels; + use Dispatchable, InteractsWithQueue, Queueable, SerializesModels, LocksPerSubject; public int $tries = 3; - public int $timeout = 20; + public int $timeout = 60; public function __construct(protected int $serverId, protected int $templateId) { } + /** + * Wait before retrying a failed Proxmox call instead of retrying at once. + */ + public function backoff(): array + { + return [5, 15]; + } + public function middleware(): array { - return [new SkipIfBatchCancelled(), new WithoutOverlapping( - "server.build#{$this->serverId}", - )]; + return [new SkipIfBatchCancelled(), $this->lockPerSubject($this->serverId)]; } public function handle(ServerBuildService $service): void diff --git a/app/Jobs/Server/DeleteServerJob.php b/app/Jobs/Server/DeleteServerJob.php index 1918889742e..6fb1bd60aa0 100644 --- a/app/Jobs/Server/DeleteServerJob.php +++ b/app/Jobs/Server/DeleteServerJob.php @@ -10,27 +10,33 @@ use Illuminate\Foundation\Bus\Dispatchable; use Illuminate\Queue\InteractsWithQueue; use Illuminate\Queue\Middleware\SkipIfBatchCancelled; -use Illuminate\Queue\Middleware\WithoutOverlapping; +use Convoy\Jobs\Concerns\LocksPerSubject; use Illuminate\Queue\SerializesModels; class DeleteServerJob implements ShouldQueue { - use Dispatchable, InteractsWithQueue, Queueable, SerializesModels, Batchable; + use Dispatchable, InteractsWithQueue, Queueable, SerializesModels, Batchable, LocksPerSubject; public int $tries = 3; - public int $timeout = 20; + public int $timeout = 60; public function __construct(protected int $serverId) { // } + /** + * Wait before retrying a failed Proxmox call instead of retrying at once. + */ + public function backoff(): array + { + return [5, 15]; + } + public function middleware(): array { - return [new SkipIfBatchCancelled(), new WithoutOverlapping( - "server.delete#{$this->serverId}", - )]; + return [new SkipIfBatchCancelled(), $this->lockPerSubject($this->serverId)]; } public function handle(ServerBuildService $service): void diff --git a/app/Jobs/Server/MonitorBackupJob.php b/app/Jobs/Server/MonitorBackupJob.php index e68d56ca281..2d5c07d6af3 100644 --- a/app/Jobs/Server/MonitorBackupJob.php +++ b/app/Jobs/Server/MonitorBackupJob.php @@ -8,13 +8,15 @@ use Illuminate\Contracts\Queue\ShouldQueue; use Illuminate\Foundation\Bus\Dispatchable; use Illuminate\Queue\InteractsWithQueue; -use Illuminate\Queue\Middleware\WithoutOverlapping; +use Convoy\Jobs\Concerns\LocksPerSubject; use Illuminate\Queue\SerializesModels; use Illuminate\Support\Carbon; class MonitorBackupJob implements ShouldQueue { - use Dispatchable, InteractsWithQueue, Queueable, SerializesModels; + use Dispatchable, InteractsWithQueue, Queueable, SerializesModels, LocksPerSubject; + + public int $timeout = 60; public function retryUntil(): Carbon { @@ -25,9 +27,18 @@ public function __construct(protected int $backupId, protected string $upid) { } + /** + * Wait before retrying after an error instead of retrying in a tight loop. + * The last delay repeats until the job gives up. + */ + public function backoff(): array + { + return [3, 10, 30]; + } + public function middleware(): array { - return [new WithoutOverlapping("server:backup.create#{$this->backupId}")]; + return [$this->lockPerSubject($this->backupId)]; } public function handle(BackupMonitorService $service): void diff --git a/app/Jobs/Server/MonitorBackupRestorationJob.php b/app/Jobs/Server/MonitorBackupRestorationJob.php index 189a9789bcf..ef88de2666b 100644 --- a/app/Jobs/Server/MonitorBackupRestorationJob.php +++ b/app/Jobs/Server/MonitorBackupRestorationJob.php @@ -8,13 +8,15 @@ use Illuminate\Contracts\Queue\ShouldQueue; use Illuminate\Foundation\Bus\Dispatchable; use Illuminate\Queue\InteractsWithQueue; -use Illuminate\Queue\Middleware\WithoutOverlapping; +use Convoy\Jobs\Concerns\LocksPerSubject; use Illuminate\Queue\SerializesModels; use Illuminate\Support\Carbon; class MonitorBackupRestorationJob implements ShouldQueue { - use Dispatchable, InteractsWithQueue, Queueable, SerializesModels; + use Dispatchable, InteractsWithQueue, Queueable, SerializesModels, LocksPerSubject; + + public int $timeout = 60; public function retryUntil(): Carbon { @@ -25,9 +27,18 @@ public function __construct(protected int $serverId, protected string $upid) { } + /** + * Wait before retrying after an error instead of retrying in a tight loop. + * The last delay repeats until the job gives up. + */ + public function backoff(): array + { + return [3, 10, 30]; + } + public function middleware(): array { - return [new WithoutOverlapping("server:backup.restore#{$this->serverId}")]; + return [$this->lockPerSubject($this->serverId)]; } public function handle(BackupMonitorService $service): void diff --git a/app/Jobs/Server/MonitorStateJob.php b/app/Jobs/Server/MonitorStateJob.php index 9375cc992d0..95c6d70d4d7 100644 --- a/app/Jobs/Server/MonitorStateJob.php +++ b/app/Jobs/Server/MonitorStateJob.php @@ -19,6 +19,8 @@ class MonitorStateJob implements ShouldQueue { use Dispatchable, InteractsWithQueue, Queueable, SerializesModels, Batchable; + public int $timeout = 60; + public function retryUntil(): Carbon { return now()->addMinutes(2); @@ -33,6 +35,15 @@ public function __construct( // } + /** + * Wait before retrying after an error instead of retrying in a tight loop. + * The last delay repeats until the job gives up. + */ + public function backoff(): array + { + return [3, 10, 30]; + } + public function middleware(): array { return [new SkipIfBatchCancelled()]; diff --git a/app/Jobs/Server/PurgeBackupsJob.php b/app/Jobs/Server/PurgeBackupsJob.php index 86012581bce..f14d170b51c 100644 --- a/app/Jobs/Server/PurgeBackupsJob.php +++ b/app/Jobs/Server/PurgeBackupsJob.php @@ -10,12 +10,12 @@ use Illuminate\Foundation\Bus\Dispatchable; use Illuminate\Queue\InteractsWithQueue; use Illuminate\Queue\Middleware\SkipIfBatchCancelled; -use Illuminate\Queue\Middleware\WithoutOverlapping; +use Convoy\Jobs\Concerns\LocksPerSubject; use Illuminate\Queue\SerializesModels; class PurgeBackupsJob implements ShouldQueue { - use Dispatchable, InteractsWithQueue, Queueable, SerializesModels, Batchable; + use Dispatchable, InteractsWithQueue, Queueable, SerializesModels, Batchable, LocksPerSubject; public int $tries = 3; @@ -29,11 +29,17 @@ public function __construct(protected int $serverId, protected bool $force = fal // } + /** + * Wait before retrying a failed Proxmox call instead of retrying at once. + */ + public function backoff(): array + { + return [5, 15]; + } + public function middleware(): array { - return [new SkipIfBatchCancelled(), new WithoutOverlapping( - "server:backups.purge#{$this->serverId}", - )]; + return [new SkipIfBatchCancelled(), $this->lockPerSubject($this->serverId)]; } public function handle(PurgeBackupsService $service): void diff --git a/app/Jobs/Server/SendPowerCommandJob.php b/app/Jobs/Server/SendPowerCommandJob.php index d63b5c78d7b..1d5154b8e26 100644 --- a/app/Jobs/Server/SendPowerCommandJob.php +++ b/app/Jobs/Server/SendPowerCommandJob.php @@ -11,26 +11,32 @@ use Illuminate\Foundation\Bus\Dispatchable; use Illuminate\Queue\InteractsWithQueue; use Illuminate\Queue\Middleware\SkipIfBatchCancelled; -use Illuminate\Queue\Middleware\WithoutOverlapping; +use Convoy\Jobs\Concerns\LocksPerSubject; use Illuminate\Queue\SerializesModels; class SendPowerCommandJob implements ShouldQueue { - use Dispatchable, InteractsWithQueue, Queueable, SerializesModels, Batchable; + use Dispatchable, InteractsWithQueue, Queueable, SerializesModels, Batchable, LocksPerSubject; public int $tries = 3; - public int $timeout = 15; + public int $timeout = 60; public function __construct(protected int $serverId, protected PowerAction $power) { } + /** + * Wait before retrying a failed Proxmox call instead of retrying at once. + */ + public function backoff(): array + { + return [5, 15]; + } + public function middleware(): array { - return [new SkipIfBatchCancelled(), new WithoutOverlapping( - "server.send-power-command#{$this->serverId}", - )]; + return [new SkipIfBatchCancelled(), $this->lockPerSubject($this->serverId)]; } public function handle(ProxmoxPowerRepository $repository): void diff --git a/app/Jobs/Server/SyncBuildJob.php b/app/Jobs/Server/SyncBuildJob.php index a9f47bbc0e6..7d7245a5b29 100644 --- a/app/Jobs/Server/SyncBuildJob.php +++ b/app/Jobs/Server/SyncBuildJob.php @@ -9,13 +9,15 @@ use Illuminate\Foundation\Bus\Dispatchable; use Illuminate\Queue\InteractsWithQueue; use Illuminate\Queue\Middleware\SkipIfBatchCancelled; -use Illuminate\Queue\Middleware\WithoutOverlapping; +use Convoy\Jobs\Concerns\LocksPerSubject; use Illuminate\Queue\SerializesModels; use Illuminate\Support\Carbon; class SyncBuildJob implements ShouldQueue { - use Dispatchable, InteractsWithQueue, Queueable, SerializesModels; + use Dispatchable, InteractsWithQueue, Queueable, SerializesModels, LocksPerSubject; + + public int $timeout = 300; public function retryUntil(): Carbon { @@ -26,11 +28,18 @@ public function __construct(protected int $serverId) { } + /** + * Wait before retrying after an error instead of retrying in a tight loop. + * The last delay repeats until the job gives up. + */ + public function backoff(): array + { + return [3, 10, 30]; + } + public function middleware(): array { - return [new SkipIfBatchCancelled(), new WithoutOverlapping( - "server.sync#{$this->serverId}", - )]; + return [new SkipIfBatchCancelled(), $this->lockPerSubject($this->serverId)]; } public function handle(SyncBuildService $service): void diff --git a/app/Jobs/Server/SyncNetworkSettings.php b/app/Jobs/Server/SyncNetworkSettings.php index ec30b39f00d..03946e17975 100644 --- a/app/Jobs/Server/SyncNetworkSettings.php +++ b/app/Jobs/Server/SyncNetworkSettings.php @@ -9,13 +9,15 @@ use Illuminate\Foundation\Bus\Dispatchable; use Illuminate\Queue\InteractsWithQueue; use Illuminate\Queue\Middleware\SkipIfBatchCancelled; -use Illuminate\Queue\Middleware\WithoutOverlapping; +use Convoy\Jobs\Concerns\LocksPerSubject; use Illuminate\Queue\SerializesModels; use Illuminate\Support\Carbon; class SyncNetworkSettings implements ShouldQueue { - use Dispatchable, InteractsWithQueue, Queueable, SerializesModels; + use Dispatchable, InteractsWithQueue, Queueable, SerializesModels, LocksPerSubject; + + public int $timeout = 60; public function retryUntil(): Carbon { @@ -26,11 +28,18 @@ public function __construct(protected int $serverId) { } + /** + * Wait before retrying after an error instead of retrying in a tight loop. + * The last delay repeats until the job gives up. + */ + public function backoff(): array + { + return [3, 10, 30]; + } + public function middleware(): array { - return [new SkipIfBatchCancelled(), new WithoutOverlapping( - "server.sync-network-settings#$this->serverId", - )]; + return [new SkipIfBatchCancelled(), $this->lockPerSubject($this->serverId)]; } public function handle(NetworkService $service): void diff --git a/app/Jobs/Server/UpdatePasswordJob.php b/app/Jobs/Server/UpdatePasswordJob.php index 996580f2435..46c144800f8 100644 --- a/app/Jobs/Server/UpdatePasswordJob.php +++ b/app/Jobs/Server/UpdatePasswordJob.php @@ -9,16 +9,16 @@ use Illuminate\Foundation\Bus\Dispatchable; use Illuminate\Queue\InteractsWithQueue; use Illuminate\Queue\Middleware\SkipIfBatchCancelled; -use Illuminate\Queue\Middleware\WithoutOverlapping; +use Convoy\Jobs\Concerns\LocksPerSubject; use Illuminate\Queue\SerializesModels; class UpdatePasswordJob implements ShouldQueue { - use Dispatchable, InteractsWithQueue, Queueable, SerializesModels; + use Dispatchable, InteractsWithQueue, Queueable, SerializesModels, LocksPerSubject; public int $tries = 15; - public int $timeout = 10; + public int $timeout = 60; public function backoff(): int { @@ -32,9 +32,7 @@ public function __construct(protected int $serverId, protected string $password) public function middleware(): array { - return [new SkipIfBatchCancelled(), new WithoutOverlapping( - "server.update-password#{$this->serverId}", - )]; + return [new SkipIfBatchCancelled(), $this->lockPerSubject($this->serverId)]; } public function handle(ServerAuthService $service): void diff --git a/app/Jobs/Server/WaitUntilVmIsCreatedJob.php b/app/Jobs/Server/WaitUntilVmIsCreatedJob.php index 9f6d81076bf..3ec03ed1922 100644 --- a/app/Jobs/Server/WaitUntilVmIsCreatedJob.php +++ b/app/Jobs/Server/WaitUntilVmIsCreatedJob.php @@ -16,11 +16,22 @@ class WaitUntilVmIsCreatedJob implements ShouldQueue { use Dispatchable, InteractsWithQueue, Queueable, SerializesModels; + public int $timeout = 60; + public function retryUntil(): Carbon { return now()->addMinutes(30); } + /** + * Wait before retrying after an error instead of retrying in a tight loop. + * The last delay repeats until the job gives up. + */ + public function backoff(): array + { + return [3, 10, 30]; + } + public function middleware(): array { return [new SkipIfBatchCancelled()]; diff --git a/tests/Unit/Jobs/LocksPerSubjectTest.php b/tests/Unit/Jobs/LocksPerSubjectTest.php new file mode 100644 index 00000000000..0e828d7258d --- /dev/null +++ b/tests/Unit/Jobs/LocksPerSubjectTest.php @@ -0,0 +1,36 @@ +middleware())->first(fn ($m) => $m instanceof WithoutOverlapping); + + expect($lock)->not->toBeNull() + ->and($lock->key)->toBe(42) + ->and($lock->expiresAfter)->toBeGreaterThan($job->timeout) + ->and($lock->releaseAfter)->toBeGreaterThan(0); +})->with([ + 'power' => fn () => new SendPowerCommandJob(42, PowerAction::START), + 'password' => fn () => new UpdatePasswordJob(42, 'Rt-Aa1!bcdefgh9'), + 'build' => fn () => new BuildServerJob(42, 1), + 'delete' => fn () => new DeleteServerJob(42), + 'purge backups' => fn () => new PurgeBackupsJob(42), + 'sync build' => fn () => new SyncBuildJob(42), + 'sync network' => fn () => new SyncNetworkSettings(42), + 'monitor backup' => fn () => new MonitorBackupJob(42, 'UPID:x'), + 'monitor restore' => fn () => new MonitorBackupRestorationJob(42, 'UPID:x'), + 'monitor ISO download' => fn () => new MonitorIsoDownloadJob(42, 'UPID:x'), +]);