Inconsistent ShouldBeUnique behavior for missing models

[naquad]

Laravel Version

10.42.0

PHP Version

8.3.2

Database Driver & Version

Redis 7.2.4

Description

Queued jobs with ShouldBeUnique may cause stale unique locks in the case when the dependent model is missing.

According to the source code of the CallQueuedHandler::call() unique lock cleanup may never be reached in case if the job depends on the missing model AND the job is configured to be deleted when the model is missing (public $deleteWhenMissingModels = true).

Steps To Reproduce

The PoC is made using Redis queue, a similar approach may work with other drivers.

1. Create a new project: laravel new poc.
2. Ensure Redis is configured & reachable in your .env file. WARNING: PoC will flush all keys.
3. Create poc.php file with the following contents:

<?php

use Illuminate\Bus\Queueable;
use Illuminate\Queue\WorkerOptions;
use Illuminate\Support\Facades\DB;
use Illuminate\Support\Facades\Queue;
use Illuminate\Support\Facades\Redis;
use Illuminate\Queue\SerializesModels;
use Illuminate\Database\Eloquent\Model;
use Illuminate\Contracts\Console\Kernel;
use Illuminate\Contracts\Queue\ShouldQueue;
use Illuminate\Foundation\Bus\Dispatchable;
use Illuminate\Contracts\Queue\ShouldBeUnique;

require 'vendor/autoload.php';
$app = require __DIR__.'/bootstrap/app.php';
$cli = $app->make(Kernel::class);
$cli->bootstrap();

config([
    'database.default' => 'sqlite',
    'queue.default' => 'redis',
    'cache.default' => 'redis',
]);

@unlink(config('database.connections.sqlite.database'));
$cli->call('migrate', ['--force' => true]);

DB::unprepared("
    CREATE TABLE IF NOT EXISTS main (
        id INTEGER NOT NULL PRIMARY KEY,
        name VARCHAR(255) UNIQUE
    )
");

DB::unprepared("
    CREATE TABLE IF NOT EXISTS secondary (
        id INTEGER NOT NULL PRIMARY KEY,
        name VARCHAR(255) NOT NULL
    )
");

// ensure there are no keys in the DB
Redis::flushdb();

class Main extends Model {
    public $table = 'main';
    public $timestamps = false;
    public $fillable = ['name'];
}

class Secondary extends Model {
    public $table = 'secondary';
    public $timestamps = false;
    public $fillable = ['name'];
}

class MyJob implements ShouldQueue, ShouldBeUnique {
    use Dispatchable, Queueable, SerializesModels;

    public $tries = 1;
    public $maxExceptions = 1;
    public $deleteWhenMissingModels = true;

    public function __construct(
        public Main $main,
        public Secondary $secondary,
    )
    {
    }

    public function handle() {
    }

    public function uniqueId() {
        return 'job-for-' . $this->main->id;
    }
}

// create 2 instances
$main = Main::create(['name' => 'main model']);
$secondary = Secondary::create(['name' => 'secondary model']);

// and schedule a job for them
MyJob::dispatch($main, $secondary);

// delete the secondary model
$secondary->delete();

// run the job
app('queue.worker')->runNextJob('redis', 'default', new WorkerOptions);

echo 'Queue size: ', Queue::size('default'), PHP_EOL; // no tasks left
echo 'Redis contents:', PHP_EOL;
dump(Redis::keys('*')); // unique id is still there

4. Execute the poc.php php poc.php
5. Inspect the output to see the stale unique lock

Output

Queue size: 0
Redis contents:
array:1 [
  0 => "laravel_database_laravel_cache_:laravel_unique_job:MyJobjob-for-1"
] // poc.php:95

Expected result

The unique lock has been removed.

Actual result

A stale unique lock still exists.

[driesvints]

The right approach here is to not make uniqueId dependent on the model and assign the id to the job when constructing it. I don't think you can expect deleteWhenMissingModels to be compatible with a behaviour in your jobs where you at all times need access to the model.

[naquad]

The bug is not about the uniqueId as this can be addressed in the user code.

The bug is that deleteWhenModelMissing configuration may effectively break the unique job lock mechanism causing a particular kind of jobs to be blocked until manual intervention. And there are no log messages or errors logged.

Neither of the cases is mentioned in the documentation and it took a while to figure out what was happening on the server and why there are so many stale unique locks.

[naquad]

To make it more clear, here's another PoC to be executed in the same environment (step 3).

<?php

use Illuminate\Bus\Queueable;
use Illuminate\Queue\WorkerOptions;
use Illuminate\Support\Facades\DB;
use Illuminate\Support\Facades\Queue;
use Illuminate\Support\Facades\Redis;
use Illuminate\Queue\SerializesModels;
use Illuminate\Database\Eloquent\Model;
use Illuminate\Contracts\Console\Kernel;
use Illuminate\Contracts\Queue\ShouldQueue;
use Illuminate\Foundation\Bus\Dispatchable;
use Illuminate\Contracts\Queue\ShouldBeUnique;

require 'vendor/autoload.php';
$app = require __DIR__.'/bootstrap/app.php';
$cli = $app->make(Kernel::class);
$cli->bootstrap();

config([
    'database.default' => 'sqlite',
    'queue.default' => 'redis',
    'cache.default' => 'redis',
]);

@unlink(config('database.connections.sqlite.database'));
$cli->call('migrate', ['--force' => true]);

DB::unprepared("
    CREATE TABLE main (
        id INTEGER NOT NULL PRIMARY KEY,
        name VARCHAR(255) UNIQUE
    )
");

// ensure there are no keys in the DB
Redis::flushdb();

class Main extends Model {
    public $table = 'main';
    public $timestamps = false;
    public $fillable = ['name'];
}

class MyJob implements ShouldQueue, ShouldBeUnique {
    use Dispatchable, Queueable, SerializesModels;

    public $tries = 1;
    public $deleteWhenMissingModels = true;

    public function __construct(
        public Main $main,
    )
    {
    }

    public function handle() {
    }
}

// create the job instance
$instance = Main::create(['name' => 'main model']);

// and schedule a job for them
MyJob::dispatch($instance);

// delete the secondary model
$instance->delete();

// run the job
app('queue.worker')->runNextJob('redis', 'default', new WorkerOptions);

echo 'Queue size: ', Queue::size('default'), PHP_EOL; // no tasks left
echo 'Redis contents:', PHP_EOL;
dump(Redis::keys('*')); // unique id is still there

I would also argue, that unique ID is being created and maintained by the framework. Hence it is framework's job to ensure that the unique ID is properly attached to the job to avoid special cases like the described one.

But skip that for now, the fix for the lock itself would be highly appreciated.

[driesvints]

Thanks @naquad for that explanation. I'll re-open this one. We'd appreciate a PR for a fix if anyone can figure one out.

[github-actions]

Thank you for reporting this issue!

As Laravel is an open source project, we rely on the community to help us diagnose and fix issues as it is not possible to research and fix every issue reported to us via GitHub.

If possible, please make a pull request fixing the issue you have described, along with corresponding tests. All pull requests are promptly reviewed by the Laravel team.

Thank you!

[naquad]

Here's my take on the fix that needs some blessing before I get knee-deep into fixing tests.

A somewhat hackish way would be the following:

1. Make UniqueLock::getKey() public static (the naming scheme is a pure function, so it can be static IMHO).
2. Update UniqueLock::getKey(): if it receives a string that looks like a unique lock key (just a precaution to ensure everything explodes if misused), it will return it as is.
3. Update Queue::createObjectPayload() to save the unique ID as seen during the job queuing.
4. Update CallQueuedHandler to rely on the unique ID in the payload rather than trying to get it from the job.

The patch would look like this:

diff --git a/src/Illuminate/Bus/UniqueLock.php b/src/Illuminate/Bus/UniqueLock.php
index a4066b77c1..f07830caf3 100644
--- a/src/Illuminate/Bus/UniqueLock.php
+++ b/src/Illuminate/Bus/UniqueLock.php
@@ -3,6 +3,7 @@
 namespace Illuminate\Bus;
 
 use Illuminate\Contracts\Cache\Repository as Cache;
+use Illuminate\Support\Str;
 
 class UniqueLock
 {
@@ -40,7 +41,7 @@ public function acquire($job)
                     ? $job->uniqueVia()
                     : $this->cache;
 
-        return (bool) $cache->lock($this->getKey($job), $uniqueFor)->get();
+        return (bool) $cache->lock(static::getKey($job), $uniqueFor)->get();
     }
 
     /**
@@ -55,7 +56,7 @@ public function release($job)
                     ? $job->uniqueVia()
                     : $this->cache;
 
-        $cache->lock($this->getKey($job))->forceRelease();
+        $cache->lock(static::getKey($job))->forceRelease();
     }
 
     /**
@@ -64,8 +65,12 @@ public function release($job)
      * @param  mixed  $job
      * @return string
      */
-    protected function getKey($job)
+    public static function getKey($job)
     {
+        if (is_string($job) && Str::startsWith($job, 'laravel_unique_job:')) {
+            return $job;
+        }
+
         $uniqueId = method_exists($job, 'uniqueId')
                     ? $job->uniqueId()
                     : ($job->uniqueId ?? '');
diff --git a/src/Illuminate/Queue/CallQueuedHandler.php b/src/Illuminate/Queue/CallQueuedHandler.php
index 5bee1d9ebb..6a545f43e8 100644
--- a/src/Illuminate/Queue/CallQueuedHandler.php
+++ b/src/Illuminate/Queue/CallQueuedHandler.php
@@ -10,7 +10,6 @@
 use Illuminate\Contracts\Container\Container;
 use Illuminate\Contracts\Encryption\Encrypter;
 use Illuminate\Contracts\Queue\Job;
-use Illuminate\Contracts\Queue\ShouldBeUnique;
 use Illuminate\Contracts\Queue\ShouldBeUniqueUntilProcessing;
 use Illuminate\Database\Eloquent\ModelNotFoundException;
 use Illuminate\Pipeline\Pipeline;
@@ -60,17 +59,18 @@ public function call(Job $job, array $data)
                 $job, $this->getCommand($data)
             );
         } catch (ModelNotFoundException $e) {
+            $this->ensureUniqueJobLockIsReleased($data);
             return $this->handleModelNotFound($job, $e);
         }
 
         if ($command instanceof ShouldBeUniqueUntilProcessing) {
-            $this->ensureUniqueJobLockIsReleased($command);
+            $this->ensureUniqueJobLockIsReleased($data);
         }
 
         $this->dispatchThroughMiddleware($job, $command);
 
         if (! $job->isReleased() && ! $command instanceof ShouldBeUniqueUntilProcessing) {
-            $this->ensureUniqueJobLockIsReleased($command);
+            $this->ensureUniqueJobLockIsReleased($data);
         }
 
         if (! $job->hasFailed() && ! $job->isReleased()) {
@@ -196,13 +196,14 @@ protected function ensureSuccessfulBatchJobIsRecorded($command)
     /**
      * Ensure the lock for a unique job is released.
      *
-     * @param  mixed  $command
+     * @param  array  $data
      * @return void
      */
-    protected function ensureUniqueJobLockIsReleased($command)
+    protected function ensureUniqueJobLockIsReleased($data)
     {
-        if ($command instanceof ShouldBeUnique) {
-            (new UniqueLock($this->container->make(Cache::class)))->release($command);
+        if (isset($data['uniqueId'])) {
+            (new UniqueLock($this->container->make(Cache::class)))
+                ->release($data['uniqueId']);
         }
     }
 
@@ -246,7 +247,7 @@ public function failed(array $data, $e, string $uuid)
         $command = $this->getCommand($data);
 
         if (! $command instanceof ShouldBeUniqueUntilProcessing) {
-            $this->ensureUniqueJobLockIsReleased($command);
+            $this->ensureUniqueJobLockIsReleased($data);
         }
 
         if ($command instanceof \__PHP_Incomplete_Class) {
diff --git a/src/Illuminate/Queue/Queue.php b/src/Illuminate/Queue/Queue.php
index 09eb245263..a87c81c5f3 100755
--- a/src/Illuminate/Queue/Queue.php
+++ b/src/Illuminate/Queue/Queue.php
@@ -4,9 +4,12 @@
 
 use Closure;
 use DateTimeInterface;
+use Illuminate\Bus\UniqueLock;
 use Illuminate\Container\Container;
 use Illuminate\Contracts\Encryption\Encrypter;
 use Illuminate\Contracts\Queue\ShouldBeEncrypted;
+use Illuminate\Contracts\Queue\ShouldBeUnique;
+use Illuminate\Contracts\Queue\ShouldBeUniqueUntilProcessing;
 use Illuminate\Contracts\Queue\ShouldQueueAfterCommit;
 use Illuminate\Queue\Events\JobQueued;
 use Illuminate\Queue\Events\JobQueueing;
@@ -150,6 +153,7 @@ protected function createObjectPayload($job, $queue)
             'timeout' => $job->timeout ?? null,
             'retryUntil' => $this->getJobExpiration($job),
             'data' => [
+                'uniqueId' => $this->getJobUniqueId($job),
                 'commandName' => $job,
                 'command' => $job,
             ],
@@ -167,6 +171,24 @@ protected function createObjectPayload($job, $queue)
         ]);
     }
 
+    /**
+     * Get the unique ID of the job
+     *
+     * @param object $job
+     * @return string|null
+     */
+    protected function getJobUniqueId($job)
+    {
+        if (
+            $job instanceof ShouldBeUnique ||
+            $job instanceof ShouldBeUniqueUntilProcessing
+        ) {
+            return UniqueLock::getKey($job);
+        }
+
+        return null;
+    }
+
     /**
      * Get the display name for the given job.
      *

EDIT: Patch changes.

[naquad]

Scratch that. uniqueVia breaks in this implementation.

[naquad]

Ok, I think I got it right. The idea is: to create a helper class to manage the lock based on the initial data. This way the UniqueLock doesn't need to be changed at all, UPDATE: it needs a change to properly handle the job class name . Although, some duplicate code is present, but I don't think it's a big issue. The only thing that worries me is the reliance on the cache store name.

diff --git a/src/Illuminate/Bus/UniqueLock.php b/src/Illuminate/Bus/UniqueLock.php
index a4066b77c1..c70a7b252a 100644
--- a/src/Illuminate/Bus/UniqueLock.php
+++ b/src/Illuminate/Bus/UniqueLock.php
@@ -70,6 +70,10 @@ protected function getKey($job)
                     ? $job->uniqueId()
                     : ($job->uniqueId ?? '');
 
-        return 'laravel_unique_job:'.get_class($job).$uniqueId;
+        $jobName = property_exists($job, 'jobName')
+                   ? $job->jobName
+                   : get_class($job);
+
+        return 'laravel_unique_job:'.$jobName.$uniqueId;
     }
 }
diff --git a/src/Illuminate/Queue/CallQueuedHandler.php b/src/Illuminate/Queue/CallQueuedHandler.php
index 5bee1d9ebb..f5d326e9e3 100644
--- a/src/Illuminate/Queue/CallQueuedHandler.php
+++ b/src/Illuminate/Queue/CallQueuedHandler.php
@@ -4,13 +4,10 @@
 
 use Exception;
 use Illuminate\Bus\Batchable;
-use Illuminate\Bus\UniqueLock;
 use Illuminate\Contracts\Bus\Dispatcher;
-use Illuminate\Contracts\Cache\Repository as Cache;
 use Illuminate\Contracts\Container\Container;
 use Illuminate\Contracts\Encryption\Encrypter;
 use Illuminate\Contracts\Queue\Job;
-use Illuminate\Contracts\Queue\ShouldBeUnique;
 use Illuminate\Contracts\Queue\ShouldBeUniqueUntilProcessing;
 use Illuminate\Database\Eloquent\ModelNotFoundException;
 use Illuminate\Pipeline\Pipeline;
@@ -60,17 +57,18 @@ public function call(Job $job, array $data)
                 $job, $this->getCommand($data)
             );
         } catch (ModelNotFoundException $e) {
+            $this->ensureUniqueJobLockIsReleased($data);
             return $this->handleModelNotFound($job, $e);
         }
 
         if ($command instanceof ShouldBeUniqueUntilProcessing) {
-            $this->ensureUniqueJobLockIsReleased($command);
+            $this->ensureUniqueJobLockIsReleased($data);
         }
 
         $this->dispatchThroughMiddleware($job, $command);
 
         if (! $job->isReleased() && ! $command instanceof ShouldBeUniqueUntilProcessing) {
-            $this->ensureUniqueJobLockIsReleased($command);
+            $this->ensureUniqueJobLockIsReleased($data);
         }
 
         if (! $job->hasFailed() && ! $job->isReleased()) {
@@ -83,6 +81,29 @@ public function call(Job $job, array $data)
         }
     }
 
+    /**
+     * Get the unserialized object from the given payload.
+     *
+     * @param string  $key
+     * @param  array  $data
+     * @return mixed
+     */
+    protected function getUnserializedItem(string $key, array $data)
+    {
+        if (isset($data[$key])) {
+            if (str_starts_with($data[$key], 'O:')) {
+                return unserialize($data[$key]);
+            }
+
+            if ($this->container->bound(Encrypter::class)) {
+                return unserialize($this->container[Encrypter::class]
+                                        ->decrypt($data[$key]));
+            }
+        }
+
+        return null;
+    }
+
     /**
      * Get the command from the given payload.
      *
@@ -93,17 +114,25 @@ public function call(Job $job, array $data)
      */
     protected function getCommand(array $data)
     {
-        if (str_starts_with($data['command'], 'O:')) {
-            return unserialize($data['command']);
-        }
-
-        if ($this->container->bound(Encrypter::class)) {
-            return unserialize($this->container[Encrypter::class]->decrypt($data['command']));
+        $command = $this->getUnserializedItem('command', $data);
+        if ($command !== null) {
+            return $command;
         }
 
         throw new RuntimeException('Unable to extract job payload.');
     }
 
+    /**
+     * Get the unique handler from the given payload.
+     *
+     * @param array  $data
+     * @return \Illuminate\Queue\UniqueHandler|null
+     */
+    protected function getUniqueHandler(array $data)
+    {
+        return $this->getUnserializedItem('uniqueHandler', $data);
+    }
+
     /**
      * Dispatch the given job / command through its specified middleware.
      *
@@ -196,13 +225,14 @@ protected function ensureSuccessfulBatchJobIsRecorded($command)
     /**
      * Ensure the lock for a unique job is released.
      *
-     * @param  mixed  $command
+     * @param  array  $data
      * @return void
      */
-    protected function ensureUniqueJobLockIsReleased($command)
+    protected function ensureUniqueJobLockIsReleased($data)
     {
-        if ($command instanceof ShouldBeUnique) {
-            (new UniqueLock($this->container->make(Cache::class)))->release($command);
+        $handler = $this->getUniqueHandler($data);
+        if ($handler !== null) {
+            $handler->withContainer($this->container)->release();
         }
     }
 
@@ -246,7 +276,7 @@ public function failed(array $data, $e, string $uuid)
         $command = $this->getCommand($data);
 
         if (! $command instanceof ShouldBeUniqueUntilProcessing) {
-            $this->ensureUniqueJobLockIsReleased($command);
+            $this->ensureUniqueJobLockIsReleased($data);
         }
 
         if ($command instanceof \__PHP_Incomplete_Class) {
diff --git a/src/Illuminate/Queue/Queue.php b/src/Illuminate/Queue/Queue.php
index 09eb245263..95cd2448ee 100755
--- a/src/Illuminate/Queue/Queue.php
+++ b/src/Illuminate/Queue/Queue.php
@@ -139,6 +139,8 @@ protected function createPayloadArray($job, $queue, $data = '')
      */
     protected function createObjectPayload($job, $queue)
     {
+        $handler = UniqueHandler::forJob($job);
+
         $payload = $this->withCreatePayloadHooks($queue, [
             'uuid' => (string) Str::uuid(),
             'displayName' => $this->getDisplayName($job),
@@ -150,17 +152,27 @@ protected function createObjectPayload($job, $queue)
             'timeout' => $job->timeout ?? null,
             'retryUntil' => $this->getJobExpiration($job),
             'data' => [
+                'uniqueHandler' => $handler,
                 'commandName' => $job,
                 'command' => $job,
             ],
         ]);
 
-        $command = $this->jobShouldBeEncrypted($job) && $this->container->bound(Encrypter::class)
-                    ? $this->container[Encrypter::class]->encrypt(serialize(clone $job))
-                    : serialize(clone $job);
+        $handler = serialize($handler);
+        $command = serialize($job);
+
+        if (
+            $this->jobShouldBeEncrypted($job) &&
+            $this->container->bound(Encrypter::class)
+        ) {
+            $encrypter = $this->container[Encrypter::class];
+            $handler = $encrypter->encrypt($handler);
+            $command = $encrypter->encrypt($command);
+        }
 
         return array_merge($payload, [
             'data' => array_merge($payload['data'], [
+                'uniqueHandler' => $handler,
                 'commandName' => get_class($job),
                 'command' => $command,
             ]),
diff --git a/src/Illuminate/Queue/UniqueHandler.php b/src/Illuminate/Queue/UniqueHandler.php
new file mode 100644
index 0000000000..2ce7156677
--- /dev/null
+++ b/src/Illuminate/Queue/UniqueHandler.php
@@ -0,0 +1,114 @@
+<?php
+
+namespace Illuminate\Queue;
+
+use Illuminate\Bus\UniqueLock;
+use Illuminate\Contracts\Cache\Factory as CacheFactory;
+use Illuminate\Contracts\Container\Container;
+use Illuminate\Contracts\Queue\ShouldBeUnique;
+use Illuminate\Contracts\Queue\ShouldBeUniqueUntilProcessing;
+
+/**
+ * A helper class to manage the unique ID and cache instance for a job
+ * base on the data of the job itself.
+ */
+class UniqueHandler
+{
+    /**
+     * Original job name
+     *
+     * @var string
+     */
+    public $jobName;
+
+    /**
+     * The unique ID for the job.
+     *
+     * @var string|null
+     */
+    public $uniqueId = null;
+
+    /**
+     * Cache connection name for the job.
+     *
+     * @var string|null
+     */
+    protected $uniqueVia = null;
+
+    /**
+     * The container instance.
+     * @var \Illuminate\Contracts\Container\Container
+     */
+    protected $container;
+
+    /**
+     * Create a new handler instance.
+     *
+     * @param object $job
+     */
+    public function __construct(object $job)
+    {
+        $this->jobName = get_class($job);
+
+        if (method_exists($job, 'uniqueId')) {
+            $this->uniqueId = $job->uniqueId();
+        } else if (isset($job->uniqueId)) {
+            $this->uniqueId = $job->uniqueId;
+        }
+
+        if (method_exists($job, 'uniqueVia')) {
+            $this->uniqueVia = $job->uniqueVia()->getName();
+        }
+    }
+
+    /**
+     * Creates a new instance if the job should be unique.
+     *
+     * @param object $job
+     * @return \Illuminate\Queue\UniqueHandler|null
+     */
+    public static function forJob(object $job)
+    {
+        if (
+            $job instanceof ShouldBeUnique ||
+            $job instanceof ShouldBeUniqueUntilProcessing
+        ) {
+            return new static($job);
+        }
+
+        return null;
+    }
+
+    /**
+     * Sets the container instance.
+     *
+     * @param \Illuminate\Contracts\Container\Container $container
+     * @return \Illuminate\Queue\UpdateHandler
+     */
+    public function withContainer(Container $container)
+    {
+        $this->container = $container;
+        return $this;
+    }
+
+    /**
+     * Returns the cache instance for the job.
+     *
+     * @return \Illuminate\Contracts\Cache\Repository
+     */
+    protected function getCacheStore()
+    {
+        return $this->container->make(CacheFactory::class)
+                               ->store($this->uniqueVia);
+    }
+
+    /**
+     * Releases the lock for the job.
+     *
+     * @return void
+     */
+    public function release()
+    {
+        (new UniqueLock($this->getCacheStore()))->release($this);
+    }
+}

EDIT: doc comments types updated.
EDIT 2: pass the cache repository directly, skip the uniqueId method
EDIT 3: encrypt the UniqueHandler instance as it potentially may leak the data from the job (the unique ID)
EDIT 4: docstrings fix

[driesvints]

Hi @naquad, can you attempt that PR? Thanks!