Changes to ShouldBeUniqueUntilProcessing in #59567 causing lost jobs in 13.5.0

[nmbrcolin]

Laravel Version

13.5.0

PHP Version

8.5

Database Driver & Version

No response

Description

Changes to ShouldBeUniqueUntilProcessing in #59567 are causing what appears to be lost jobs in 13.5.0.

Scenario

I have a production Laravel application that uses jobs heavily. It's been working properly for >1 year, through many framework upgrades, but started failing after upgrading to 13.5.0 with what appears to be lost jobs.

I have a model with a stale_since timestamp. When something changes the model, I set the model's stale_since to the current date and dispatch a job, which performs some calculations and clears stale_since. The job has ShouldBeUniqueUntilProcessing and WithoutOverlapping middleware, so there should be at most 2 instances of the job for a given model at a given point in time - 0..1 pending and 0..1 processing. A job dispatched after setting stale_since may ultimately not be run - it may be dropped by ShouldBeUniqueUntilProcessing because another job is pending for that model - but in that case the pending job should ultimately clear the model's stale_since.

(I'm aware there are at least 2 logical race conditions in what I'm describing. This is an oversimplification. All that matters is that the the final stale_since that gets set should be cleared by the final job that gets processed, even if that job was already pending when the final stale_since was set. 😄)

In 13.4.0 and earlier, this logic held.

However, after upgrading to 13.5.0, I've suddenly started seeing models with stale_since set long after they should have been cleared, with no jobs pending, and with no exceptions and no failed jobs logged. It seems the jobs were "lost" in the framework.

Downgrading to 13.4.0 immediately resolved the issue.

Patching 13.5.0 to revert #59567 also resolved the issue.

Steps To Reproduce

I've created a self-contained repro that sets up this exact scenario as simply as I can. It passes with 13.4.0, fails with 13.5.0, and passes with 13.5.0 patched to revert #59567:

https://github.com/nmbrcolin/repro-stale

[nmbrcolin]

FYI @kohlerdominik

[kohlerdominik]

Excellent issue description! I think this is related to that WithoutOverlapping increases attempts, if it finds itself overlapping? And therefore, the lock doesn't get released. If you'd release the lock in your job, you'd probably be fine:

use Illuminate\Bus\UniqueLock;
handle() {
  // your logic...
  (new UniqueLock($this->container->make(Cache::class)))->release($this);
}

(not sure if you're able to pass a job to release(), but I think it should work).

I'm gonna be honest, WithoutOverlapping didn't even cross my mind when I made the fix. And I wasn't aware how it works. Any idea how we could upcycle this, so fix both our bugs instead of fixing only one. I must admit that my use-case seems more edge-case than yours.

[kohlerdominik]

I'm not sure if I'm able to fully grasp your requirement, but I think the new DebounceableQueuedJobs could fit your use case better than ShouldBeUniqueUntilProcessing+WithoutOverlapping. It's implicit that.

[nmbrcolin]

It definitely looks useful but I don't think it quite meets my needs. What I need is to

• run jobs as as they come in without delay if possible (so a single update is handled immediately)
• only process one job for a given ID at a time (because the job can't be run in parallel safely)
• deduplicate the queued jobs by ID (so a rapid series of updates doesn't fill the queue when a single job would have the same effect)

The combination of ShouldBeUniqueUntilProcessing and WithoutOverlapping was giving me that.

[nmbrcolin]

I took a stab at a fix for both issues in #60051 using a new record in the cache. I'm a Laravel internals neophyte, so if you have thoughts on ways to improve it or an alternative fix I'd appreciate them. 🙇

[nmbrcolin]

Until this is fixed in the framework itself, you can work around it by creating a new ReleasesUniqueUntilProcessingLock middleware and registering it after WithoutOverlapping:

<?php

namespace App\Jobs\Middleware;

use Illuminate\Bus\UniqueLock;
use Illuminate\Contracts\Cache\Repository as Cache;
use Illuminate\Contracts\Queue\ShouldBeUniqueUntilProcessing;
use Illuminate\Events\CallQueuedListener;

/**
 * Middleware to ensure the unique-until-processing lock is released. Without it, a job
 * that uses both ShouldBeUniqueUntilProcessing and WithoutOverlapping can permanently
 * leak its unique lock if WithoutOverlapping releases the job back to the queue on
 * attempt 1 and the retry succeeds on attempt 2+.
 *
 * See laravel/framework#60042 and the rejected fix PR laravel/framework#60051
 */
class ReleasesUniqueUntilProcessingLock
{
    public const int DEFAULT_MARKER_TTL = 86400;

    public function handle($command, $next)
    {
        if (!$this->commandShouldBeUniqueUntilProcessing($command)) {
            return $next($command);
        }

        $uuid = $command->job?->uuid();
        if ($uuid === null) {
            return $next($command);
        }

        $marker = UniqueLock::getKey($command) . ':released:' . $uuid;

        $cache = $this->getCache($command);
        if (!$cache->get($marker)) {
            new UniqueLock($cache)->release($command);

            $cache->put($marker, true, $this->getMarkerTTL($command));
        }

        return $next($command);
    }

    protected function commandShouldBeUniqueUntilProcessing($command): bool
    {
        if ($command instanceof ShouldBeUniqueUntilProcessing) {
            return true;
        }

        if (
            $command instanceof CallQueuedListener &&
            $command->shouldBeUniqueUntilProcessing()
        ) {
            return true;
        }

        return false;
    }

    protected function getCache($command): Cache
    {
        if (method_exists($command, 'uniqueVia')) {
            $store = $command->uniqueVia();

            if ($store !== null) {
                return $store;
            }
        }

        return app()->make(Cache::class);
    }

    protected function getMarkerTTL($command): int
    {
        $uniqueFor = method_exists($command, 'uniqueFor')
            ? (int) $command->uniqueFor()
            : 0;

        return max($uniqueFor, self::DEFAULT_MARKER_TTL);
    }
}