fix(jobs): expire overlap locks, and back off between retries

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

app/Jobs/Concerns/LocksPerSubject.php

@@ -0,0 +1,31 @@
+<?php
+
+namespace Convoy\Jobs\Concerns;
+
+use Illuminate\Queue\Middleware\WithoutOverlapping;
+
+/**
+ * Keeps two jobs of the same class from running on the same server (or backup,
+ * or ISO) at once, with a lock that can't outlive a killed job.
+ *
+ * WithoutOverlapping's lock never expires by default, and it's only released
+ * when the job finishes. A job the worker kills for running past its timeout
+ * never gets there, so the lock stayed forever and every later job of that
+ * class for that subject failed straight away -- a slow Proxmox call during a
+ * force-stop could break reinstalling and deleting that server until the lock
+ * was cleared from Redis by hand.
+ *
+ * The lock now expires shortly after the job's own timeout, and a job that finds
+ * it held waits a few seconds and tries again instead of giving up at once.
+ * Laravel already scopes the key by the job's class, so the subject's ID is key
+ * enough.
+ */
+trait LocksPerSubject
+{
+    protected function lockPerSubject(int|string $subjectId): WithoutOverlapping
+    {
+        return (new WithoutOverlapping($subjectId))
+            ->expireAfter($this->timeout + 30)
+            ->releaseAfter(5);
+    }
+}

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

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

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

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

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

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

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

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

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

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

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