Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix unique job lock is not released on model not found exception, lock gets stuck. #53999

Closed
wants to merge 7 commits into from

Conversation

zackAJ
Copy link
Contributor

@zackAJ zackAJ commented Dec 22, 2024

Overview

Description

When using SerializesModels on a Unique job and the model is deleted before the job is being processed, ModelNotFoundException will be thrown and the job will fail and that's okay and well documented.
However the unique lock is not released and gets stuck until expired, this happens a lot with delayed jobs.

Related issues: #50211, #49890, possibly #37729

Steps to reproduce

1- Create or open a laravel project, for ease of inspecting use database cache driver.

2- Create TestJob.php in your app/Jobs :

<?php

namespace App\Jobs;

use App\Models\User;
use Illuminate\Contracts\Queue\ShouldBeUnique;
use Illuminate\Contracts\Queue\ShouldQueue;
use Illuminate\Foundation\Bus\Dispatchable;
use Illuminate\Queue\InteractsWithQueue;
use Illuminate\Queue\SerializesModels;

class TestJob implements ShouldQueue, ShouldBeUnique
{
    use InteractsWithQueue, Dispatchable, SerializesModels;

    public function __construct(public User $user) {}

    public function handle()
    {
    }

    public function uniqueId(): string
    {
        return "some_key";
    }
}

3- Add a test route to your web.php:

<?php

use App\Jobs\TestJob;
use App\Models\User;
use Illuminate\Support\Facades\Route;

Route::get('test', function () {
    /** @var \App\Models\User */
    $user = User::factory()->create();

    dispatch(new TestJob($user));

    $user->delete();

    return "job will fail and lock will be stuck, if you refresh the page it won't dispatch again";
});

4- Run the web and queue servers via composer run dev and hit the /test route

5- Refresh the page and inspect your database's cache_locks and failed_jobs tables

image

Cause

TLDR

No serialized job command instance = no unique lock to release

    /**
     * Ensure the lock for a unique job is released.
     *
     * @param  mixed  $command
     * @return void
     */
    protected function ensureUniqueJobLockIsReleased($command)
    {
        if ($command instanceof ShouldBeUnique) {
            (new UniqueLock($this->container->make(Cache::class)))->release($command);
        }
    }

In depth

callQueuedHanlder.php's call() is responsible for handling the queued job, when we try to unserialize the payload of the job ModelNotFoundException is thrown because the model was deleted.

In this case we don't have a job command instance which is currently required to release the lock.

    /**
     * Handle the queued job.
     *
     * @param  \Illuminate\Contracts\Queue\Job  $job
     * @param  array  $data
     * @return void
     */
    public function call(Job $job, array $data)
    {
        try {
            $command = $this->setJobInstanceIfNecessary(
                $job, $this->getCommand($data) //this fails
            );
        } catch (ModelNotFoundException $e) {
            return $this->handleModelNotFound($job, $e); //early return: this will call $job->fail() eventually triggering failed()
        }
        
      // unreachable code (depends on a job command instance and releases the lock)
      if ($command instanceof ShouldBeUniqueUntilProcessing) {
            $this->ensureUniqueJobLockIsReleased($command);
      }
      ...
    }

after $this->handleMedelNotFound(...) is done the job is marked as failed and the failed() method will be called:

    /**
     * Call the failed method on the job instance.
     *
     * The exception that caused the failure will be passed.
     *
     * @param  array  $data
     * @param  \Throwable|null  $e
     * @param  string  $uuid
     * @return void
     */
    public function failed(array $data, $e, string $uuid)
    {
        $command = $this->getCommand($data); // this will fail again
        
        // unreachable code (depends on a job command instance and releases the lock)
        
        if (! $command instanceof ShouldBeUniqueUntilProcessing) { 
            $this->ensureUniqueJobLockIsReleased($command);
        }
        ...
     }

Solution

The idea

Wrap the job with a hidden context in order to have access to the lock key and the cache driver used, here's a simple overview of the flow:

context()->addHidden([
    'lock_key' => $lockKey,
    'cache_driver' => $cacheDriver
]);

dispatch(new TestJob($user));

context()->forgetHidden(['lock_key','cache_driver']);

Now when handling ModelNotFoundException via the handleModelNotFound() method, we can forceRelease the lock by using the provided cache_driver and lock_key from the hidden context

        $driver = context()->getHidden('cache_driver');

        $key = context()->getHidden('lock_key');

        if ($driver && $key) {
            cache()->driver($driver)->lock($key)->forceRelease();
        }

Implementation

See code diff.

If this gets closed for any reason use my test to research and implement another possible solution, add this to tests/Integration/Queue/UniqueTestJob.php

    use Orchestra\Testbench\Factories\UserFactory;

    public function testLockIsReleasedOnModelNotFoundException()
    {
        UniqueTestSerializesModelsJob::$handled = false;

        /**  @var  \Illuminate\Foundation\Auth\User */
        $user = UserFactory::new()->create();
        $job = new UniqueTestSerializesModelsJob($user);

        $this->expectException(ModelNotFoundException::class);

        try {
            $user->delete();
            dispatch($job);
        } finally {
            $this->assertFalse($job::$handled);
            $this->assertModelMissing($user);
            $this->assertTrue($this->app->get(Cache::class)->lock($this->getLockKey($job), 10)->get());
        }
    }


class UniqueTestSerializesModelsJob extends UniqueTestJob
{
    use SerializesModels;

    public function __construct(public User $user) {}
}

Copy link

Thanks for submitting a PR!

In order to review and merge PRs most efficiently, we require that all PRs grant maintainer edit access before we review them. For information on how to do this, see the relevant GitHub documentation. Additionally, GitHub doesn't allow maintainer permissions from organization accounts. Please resubmit this PR from a personal GitHub account with maintainer permissions enabled.

@github-actions github-actions bot closed this Dec 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant