Current tenant model not refreshed in container before each job (stale model properties)

[ionutantohi]

When a tenant is made current, there is a check to see if tenant we want to make current is already loaded.

While this somehow makes sense, it might cause an issue in jobs, because jobs are processed by long running processes.

There was a back and forth around this check. Initially was added in #111 then removed in #115 and added back in 85c4e5c (cc @masterix21)

@telkins you asked in your pr #111 whether or not is a good idea. Let me give you an example.

Say we have the following tenant
id: 1
name: My tenant
stripe_plan_id: abc

Now, if we dispatch a job, for this tenant, then

• the worker will pick up the job
• there is no current tenant, then the tenant made current (loaded into container)
• job is processed

Now, let's say that some info at tenant level are changed (by an user in his account etc)

stripe_plan_id: xyz

When a second job is dispatched for the same tenant

• the worker will pick the job
• but since a tenant is already loaded into container and is the same (by looking at its id) it just returns early. But that means when calling Tenant::current inside the job, on second job stripe_plan_id is still abc.

Note: The tenant is queried every time and before each job in MakeQueueTenantAwareAction@listenForJobsBeingProcessed but the object returned by findTenant (fresh from db) is never set to container because of id match.

Possible workarounds?

• remove the isCurrent check entirely, but I am not sure if that's ok.
• update isCurrent check to also look at the tenant/model properties and return true only if all model properties are identical
• introduce a forceMakeCurrent method which should be called from MakeQueueTenantAwareAction which should bypass isCurrent check
• other?

[masterix21]

Hi @ionutantohi, please create a repository to test the issue. Thanks

[ionutantohi]

Can you try this out: https://github.com/ionutantohi/spatie-multitenancy-issue-422

1. clone the repository
2. migrate the database
3. make sure you are using a redis queue connection

Then in 2 terminal windows run

• php artisan queue:work -q
• php artisan dispatch-jobs

In the DispatchJobs command, I dispatch 2 jobs, but between them I change the tenant name to a different value. Notice that when the second job is processed, the updated name is still the old value.

[masterix21]

@freekmurze, one fix to this issue is to change the MakeQueueTenantAwareAction.php like so:

class MakeQueueTenantAwareAction
{
    /// ...

    protected function listenForJobsBeingProcessed(): static
    {
        app('events')->listen(JobProcessing::class, function (JobProcessing $event) {
            $this->getTenantModel()::forgetCurrent();

            if (array_key_exists('tenantId', $event->job->payload())) {
                $this->findTenant($event)->makeCurrent();
            }
        });

        return $this;
    }

    protected function listenForJobsRetryRequested(): static
    {
        app('events')->listen(JobRetryRequested::class, function (JobRetryRequested $event) {
            $this->getTenantModel()::forgetCurrent();

            if (array_key_exists('tenantId', $event->payload())) {
                $this->findTenant($event)->makeCurrent();
            }
        });

        return $this;
    }

    /// ...
}

What do you think about? (PR: #438 - Tests are ok!)

[masterix21]

@ionutantohi: PR's merge is on the way to fixing the issue. I'll keep you in the loop.

[masterix21]

@ionutantohi issue fixed by 3.0.2. Thanks for your time on it.

[ionutantohi]

Thanks @masterix21. I confirm it works as expected on 3.0.2

[moisish]

When a second job is dispatched for the same tenant the worker will pick the job but since a tenant is already loaded into container

On the second job the framework will boot again which means the container does not have anything loaded. Is my understanding wrong?

[ionutantohi]

No, the framework will not boot again before each job.

The framework is booted once, when php artisan queue:work process/worker starts. Then all jobs executed by this worker are handled on the same framework instance. That's why you need to restart your workers after each code change

[moisish]

Right, makes sense.

The solution produced a breaking change. Can you check my proposed solution on the comment bellow ?

#438 (comment)
#446
#444

[masterix21]

Try again with the release 3.0.3. Thanks!