Using RedisTenancyBoostrapper causes problems with queue worker

[pr4xx]

Bug description

When calling queue:restart, it will cause all future queue:work commands to exit after only one job, if the job was dispatched in tenant context.

Steps to reproduce

See this demo repository: https://github.com/pr4xx/tenancy-problem-demo

Expected behavior

queue:work does not stop itself

Laravel version

11.9

stancl/tenancy version

3.8

[stancl]

For context, we are currently looking into a way to solve this nicely without removing the optimizations in the queue bootstrapper. As a quick fix, you can either avoid using the Redis bootstrapper (and only use e.g. the cache bootstrapper) or you can use your own QueueTenancyBootstrapper with this line changed to if (true) {:

tenancy/src/Bootstrappers/QueueTenancyBootstrapper.php

Line 74 in 8f9c7ef

if ($runningTests) {

[stancl]

The more I think about this the more I feel it's best to just remove the optimization altogether, since optimizations that cause issues aren't good optimizations. The overhead of initializing tenancy is also very low compared to how long it takes the queue worker to pick up a job, so it might be worth just getting rid of this logic altogether. If we do that, the bootstrapper also becomes a lot simpler:

diff --git a/src/Bootstrappers/QueueTenancyBootstrapper.php b/src/Bootstrappers/QueueTenancyBootstrapper.php
index f747faea..5297509c 100644
--- a/src/Bootstrappers/QueueTenancyBootstrapper.php
+++ b/src/Bootstrappers/QueueTenancyBootstrapper.php
@@ -4,7 +4,6 @@ declare(strict_types=1);
 
 namespace Stancl\Tenancy\Bootstrappers;
 
-use Illuminate\Support\Str;
 use Illuminate\Config\Repository;
 use Illuminate\Queue\QueueManager;
 use Stancl\Tenancy\Contracts\Tenant;
@@ -25,16 +24,6 @@ class QueueTenancyBootstrapper implements TenancyBootstrapper
     /** @var QueueManager */
     protected $queue;
 
-    /**
-     * Don't persist the same tenant across multiple jobs even if they have the same tenant ID.
-     *
-     * This is useful when you're changing the tenant's state (e.g. properties in the `data` column) and want the next job to initialize tenancy again
-     * with the new data. Features like the Tenant Config are only executed when tenancy is initialized, so the re-initialization is needed in some cases.
-     *
-     * @var bool
-     */
-    public static $forceRefresh = false;
-
     /**
      * The normal constructor is only executed after tenancy is bootstrapped.
      * However, we're registering a hook to initialize tenancy. Therefore,
@@ -42,7 +31,7 @@ class QueueTenancyBootstrapper implements TenancyBootstrapper
      */
     public static function __constructStatic(Application $app)
     {
-        static::setUpJobListener($app->make(Dispatcher::class), $app->runningUnitTests());
+        static::setUpJobListener($app->make(Dispatcher::class));
     }
 
     public function __construct(Repository $config, QueueManager $queue)
@@ -53,87 +42,30 @@ class QueueTenancyBootstrapper implements TenancyBootstrapper
         $this->setUpPayloadGenerator();
     }
 
-    protected static function setUpJobListener($dispatcher, $runningTests)
+    protected static function setUpJobListener($dispatcher)
     {
-        $previousTenant = null;
+        $dispatcher->listen(JobProcessing::class, function ($event) {
+            $tenant = $event->job->payload()['tenant_id'] ?? null;
 
-        $dispatcher->listen(JobProcessing::class, function ($event) use (&$previousTenant) {
-            $previousTenant = tenant();
-
-            static::initializeTenancyForQueue($event->job->payload()['tenant_id'] ?? null);
+            if ($tenant) {
+                tenancy()->initialize($tenant);
+            }
         });
 
-        $dispatcher->listen(JobRetryRequested::class, function ($event) use (&$previousTenant) {
-            $previousTenant = tenant();
+        $dispatcher->listen(JobRetryRequested::class, function ($event) {
+            $tenant = $event->payload()['tenant_id'] ?? null;
 
-            static::initializeTenancyForQueue($event->payload()['tenant_id'] ?? null);
+            if ($tenant) {
+                tenancy()->initialize($tenant);
+            }
         });
 
-        // If we're running tests, we make sure to clean up after any artisan('queue:work') calls
-        $revertToPreviousState = function ($event) use (&$previousTenant, $runningTests) {
-            if ($runningTests) {
-                static::revertToPreviousState($event, $previousTenant);
-            }
+        $revertToCentralContext = function () {
+            tenancy()->end();
         };
 
-        $dispatcher->listen(JobProcessed::class, $revertToPreviousState); // artisan('queue:work') which succeeds
-        $dispatcher->listen(JobFailed::class, $revertToPreviousState); // artisan('queue:work') which fails
-    }
-
-    protected static function initializeTenancyForQueue($tenantId)
-    {
-        if (! $tenantId) {
-            // The job is not tenant-aware
-            if (tenancy()->initialized) {
-                // Tenancy was initialized, so we revert back to the central context
-                tenancy()->end();
-            }
-
-            return;
-        }
-
-        if (static::$forceRefresh) {
-            // Re-initialize tenancy between all jobs
-            if (tenancy()->initialized) {
-                tenancy()->end();
-            }
-
-            tenancy()->initialize(tenancy()->find($tenantId));
-
-            return;
-        }
-
-        if (tenancy()->initialized) {
-            // Tenancy is already initialized
-            if (tenant()->getTenantKey() === $tenantId) {
-                // It's initialized for the same tenant (e.g. dispatchNow was used, or the previous job also ran for this tenant)
-                return;
-            }
-        }
-
-        // Tenancy was either not initialized, or initialized for a different tenant.
-        // Therefore, we initialize it for the correct tenant.
-        tenancy()->initialize(tenancy()->find($tenantId));
-    }
-
-    protected static function revertToPreviousState($event, &$previousTenant)
-    {
-        $tenantId = $event->job->payload()['tenant_id'] ?? null;
-
-        // The job was not tenant-aware
-        if (! $tenantId) {
-            return;
-        }
-
-        // Revert back to the previous tenant
-        if (tenant() && $previousTenant && $previousTenant->isNot(tenant())) {
-            tenancy()->initialize($previousTenant);
-        }
-
-        // End tenancy
-        if (tenant() && (! $previousTenant)) {
-            tenancy()->end();
-        }
+        $dispatcher->listen(JobProcessed::class, $revertToCentralContext); // artisan('queue:work') which succeeds
+        $dispatcher->listen(JobFailed::class, $revertToCentralContext); // artisan('queue:work') which fails
     }
 
     protected function setUpPayloadGenerator()

Only issue is that removing $forceRefresh is a breaking change, so I should probably keep it even if it doesn't do anything.

[stancl]

@pr4xx If you could test if this version of the bootstrapper works for you:

<?php

declare(strict_types=1);

namespace Stancl\Tenancy\Bootstrappers;

use Illuminate\Config\Repository;
use Illuminate\Queue\QueueManager;
use Stancl\Tenancy\Contracts\Tenant;
use Illuminate\Queue\Events\JobFailed;
use Illuminate\Queue\Events\JobProcessed;
use Illuminate\Queue\Events\JobProcessing;
use Illuminate\Contracts\Events\Dispatcher;
use Illuminate\Queue\Events\JobRetryRequested;
use Illuminate\Support\Testing\Fakes\QueueFake;
use Illuminate\Contracts\Foundation\Application;
use Stancl\Tenancy\Contracts\TenancyBootstrapper;

class QueueTenancyBootstrapper implements TenancyBootstrapper
{
    /** @var Repository */
    protected $config;

    /** @var QueueManager */
    protected $queue;

    /**
     * The normal constructor is only executed after tenancy is bootstrapped.
     * However, we're registering a hook to initialize tenancy. Therefore,
     * we need to register the hook at service provider execution time.
     */
    public static function __constructStatic(Application $app)
    {
        static::setUpJobListener($app->make(Dispatcher::class));
    }

    public function __construct(Repository $config, QueueManager $queue)
    {
        $this->config = $config;
        $this->queue = $queue;

        $this->setUpPayloadGenerator();
    }

    protected static function setUpJobListener($dispatcher)
    {
        $dispatcher->listen(JobProcessing::class, function ($event) {
            $tenant = $event->job->payload()['tenant_id'] ?? null;

            if ($tenant) {
                tenancy()->initialize($tenant);
            }
        });

        $dispatcher->listen(JobRetryRequested::class, function ($event) {
            $tenant = $event->payload()['tenant_id'] ?? null;

            if ($tenant) {
                tenancy()->initialize($tenant);
            }
        });

        $revertToCentralContext = function () {
            tenancy()->end();
        };

        $dispatcher->listen(JobProcessed::class, $revertToCentralContext); // artisan('queue:work') which succeeds
        $dispatcher->listen(JobFailed::class, $revertToCentralContext); // artisan('queue:work') which fails
    }

    protected function setUpPayloadGenerator()
    {
        $bootstrapper = &$this;

        if (! $this->queue instanceof QueueFake) {
            $this->queue->createPayloadUsing(function ($connection) use (&$bootstrapper) {
                return $bootstrapper->getPayload($connection);
            });
        }
    }

    public function bootstrap(Tenant $tenant)
    {
        //
    }

    public function revert()
    {
        //
    }

    public function getPayload(string $connection)
    {
        if (! tenancy()->initialized) {
            return [];
        }

        if ($this->config["queue.connections.$connection.central"]) {
            return [];
        }

        $id = tenant()->getTenantKey();

        return [
            'tenant_id' => $id,
        ];
    }
}

[niconico291]

@stancl Hey there, thanks for the update!

I tried your code and have an issue: since the new code does not make use of a $previousTenant field, my short test failed when dispatching a job within tenant context and having QUEUE_CONNECTION set to sync. It reverts back to central context but my controller code is not done yet in this scenario. It tries to hit the tenant database (in controller, outside job which has completed) and fails because the provider called tenancy()->end().

Might not be the best test case because I use redis as my queue connection in production but would be nice to have for local development. I guess the restoring of any previous tenant cannot be skipped?

[stancl]

Thanks for testing the code and letting me know about this. I see, so it seems like we want to do this?

• If queue connection is sync, we do want to revert to the previous tenant (but we can do so unconditionally, removing a lot of the original optimizations)
• If queue connection is not sync, we revert to the central context after each job

[niconico291]

It seems like we should revert back to any previous state. I am not sure about that split between sync and not sync. Though I cannot think of any case outside of sync where it would matter tbh.
Is the overhead of storing any previous tenant, unaware of the queue driver, problematic?

[stancl]

It's not, but then if we reverted to the previous tenant in a queue worker context we'd just get back to the original issue here - the queue worker remaining in the central context. This could be solved by keeping track of the original state properly, since it'd be central in the queue worker but tenant in your sync example, but that might be error prone so I'd prefer to simplify or remove the optimizations as much as possible.

[niconico291]

We can try checking for sync and then not revert to central. Doing nothing would probably be enough I think?

[AdamRollinson]

@pr4xx If you could test if this version of the bootstrapper works for you:

<?php

declare(strict_types=1);

namespace Stancl\Tenancy\Bootstrappers;

use Illuminate\Config\Repository;
use Illuminate\Queue\QueueManager;
use Stancl\Tenancy\Contracts\Tenant;
use Illuminate\Queue\Events\JobFailed;
use Illuminate\Queue\Events\JobProcessed;
use Illuminate\Queue\Events\JobProcessing;
use Illuminate\Contracts\Events\Dispatcher;
use Illuminate\Queue\Events\JobRetryRequested;
use Illuminate\Support\Testing\Fakes\QueueFake;
use Illuminate\Contracts\Foundation\Application;
use Stancl\Tenancy\Contracts\TenancyBootstrapper;

class QueueTenancyBootstrapper implements TenancyBootstrapper
{
    /** @var Repository */
    protected $config;

    /** @var QueueManager */
    protected $queue;

    /**
     * The normal constructor is only executed after tenancy is bootstrapped.
     * However, we're registering a hook to initialize tenancy. Therefore,
     * we need to register the hook at service provider execution time.
     */
    public static function __constructStatic(Application $app)
    {
        static::setUpJobListener($app->make(Dispatcher::class));
    }

    public function __construct(Repository $config, QueueManager $queue)
    {
        $this->config = $config;
        $this->queue = $queue;

        $this->setUpPayloadGenerator();
    }

    protected static function setUpJobListener($dispatcher)
    {
        $dispatcher->listen(JobProcessing::class, function ($event) {
            $tenant = $event->job->payload()['tenant_id'] ?? null;

            if ($tenant) {
                tenancy()->initialize($tenant);
            }
        });

        $dispatcher->listen(JobRetryRequested::class, function ($event) {
            $tenant = $event->payload()['tenant_id'] ?? null;

            if ($tenant) {
                tenancy()->initialize($tenant);
            }
        });

        $revertToCentralContext = function () {
            tenancy()->end();
        };

        $dispatcher->listen(JobProcessed::class, $revertToCentralContext); // artisan('queue:work') which succeeds
        $dispatcher->listen(JobFailed::class, $revertToCentralContext); // artisan('queue:work') which fails
    }

    protected function setUpPayloadGenerator()
    {
        $bootstrapper = &$this;

        if (! $this->queue instanceof QueueFake) {
            $this->queue->createPayloadUsing(function ($connection) use (&$bootstrapper) {
                return $bootstrapper->getPayload($connection);
            });
        }
    }

    public function bootstrap(Tenant $tenant)
    {
        //
    }

    public function revert()
    {
        //
    }

    public function getPayload(string $connection)
    {
        if (! tenancy()->initialized) {
            return [];
        }

        if ($this->config["queue.connections.$connection.central"]) {
            return [];
        }

        $id = tenant()->getTenantKey();

        return [
            'tenant_id' => $id,
        ];
    }
}

Just wanted to confirm, this had solved the issue in production using the redis tenancy bootstrapper and this version of the queue tenancy bootstrapper.

[stancl]

I guess the easiest way to make this work in both contexts is to keep the existing logic for reverting to to the previous context since it's well written and tested, and I'll just make the logic for reverting to the previous context execute every time, rendering forceRefresh deprecated and ignored.