QueueTenancyBootstrapper Tenant Data Change Not Reflected

[mannis17]

Bug description

QueueTenancyBootstrapper will not recognize changes to Tenant Data until tenancy()->initialized is called.

1. Tenant1 job arrives on the queue and is processed
2. Tenant1 attribute data is updated.
3. Tenant1 job arrives on the queue but since tenancy was already intialized the tenant data does not get updated for the currently initialized tenant

Looks to be the result of QueueTenancyBootstrapper::initializeTenancyForQueue code snippet (or setUpJobListener in 3.4.6)

  if (tenancy()->initialized) {
      if (tenant()->getTenantKey() === $tenantId) {
          // Tenancy is already initialized for the tenant (e.g. dispatchNow was used)
          return;
      }
  }

This prevents the tenancy initialization that occurs after this snippet.

Instead of return here what would be the implications of calling tenant()->end() ?

Unsure whether the If statement was only performance related. In the scenario above if Tenant2 job arrives and is processed prior to the 2nd Tenant1 job then when 2nd Tenant1 job is processed tenancy is intialized and correct attribute data is reflected.

Steps to reproduce

1. Send job for Tenant1 to a queue
2. Run Queue worker (and leave running) and output to log the data attribute you will change in point 3 below
3. Edit Tenant1 data attribute
4. Send job for Tenant1 to queue and output the data attirubte in point 3 below 3
5. The old data attribute will appear

Expected behavior

1. Send job for Tenant1 to a queue
2. Run Queue worker (and leave running) and output to log the data attribute you will change in point 3 below
3. Edit Tenant1 data attribute
4. Send job for Tenant1 to queue and output the data attirubte in from point 3
5. The new data attribute will appear

Laravel version

8

stancl/tenancy version

3.4.6 and 3.5.1

[stancl]

Can you show some actual code that reproduces this?

[mannis17]

hmmm, its more a process flow that triggers it. Will take some time to put together a sample project that demonstrates the issue. If you have a an existing project that handles a queue if you output the tenant data that was changed in the job handler you will be able to see the issue is

    Log::info("Config Value: " . Config::get('Tenant_data_attribute_changed'));            
    Log::info("Tenant value: " . tenant('Tenant_data_attribute_changed'));

Using Nova to manage tenants and change the tenant data. If the above code is in the job handler, following the process documented neither log will have the updated tenant data attributes until either the queue worker is restarted or a different tenant job is processed first.

What would be the implications of the suggested tenant()->end()?

[stancl]

It depends on what you mean by changing the tenant data. In the reproduction steps, you wrote:

Edit Tenant1 data attribute
Which sounds like changing the property without saving it to the DB.

So I wonder how you're actually changing the tenant and for what use case.

[mannis17]

The tenant data is being saved to the database prior to the 2nd queue job beginning to be processed. Consider the scenario where the tenant data contains specific data required by the job. This could be anything from a tenant webhook that needs to be called while/after procesing the job or something as simple as an email that needs to be sent using a specific from to address that is tenant specific. Regardless of the use case tenant data can periodically change, the queue workers need to be able to respect the change in tenant data and not serve stale tenant configuration data, which is what is occuring in the scenario described.

[stancl]

Ah, so it's specifically values being used for config? How are the properties mapped?

[mannis17]

storageToConfigMap in TenancyServiceProvider

[stancl]

Yeah, I mean which ones are being mapped.

[mannis17]

Not sure I understand the question but all the tenant properties are mapped in storageToConfigMap

[stancl]

I'm asking if you can show the exact mapping.

Mapping "all the tenant properties" doesn't make sense.

[mannis17]

For example:

if someValue = 'myvalue'

Job arrives and is processed for Tenant1
Config::get('app.someValue') === 'myvalue'

In nova change someValue for the tenant1 to 'myNewValue'

2nd job arrives for Tenant1 prior to any other Tenant jobs, Config::get('app.someValue') still = 'myvalue'

[mannis17]

Without showing our specific mappings this would be identical:

\Stancl\Tenancy\Features\TenantConfig::$storageToConfigMap = [
'someValue' => 'app.someValue'
],

[stancl]

I don't think I can help with this with these vague examples. I need to see realistic examples of what you're setting there and how you're using them afterwards. Show the actual code from the provider & the job class

[mannis17]

I'll get a sample done up using the saas-boilerplate code to demonstrate the problem

[stancl]

No need to use the saas boilerplate, I just need to see a few lines of code that will actually explain what you're doing.

I vaguely understand the issue that you're suggesting, but I don't know why you're using this code in the first place. It seems pretty non-standard so I need to see a real example of why you would want to do this.

[mannis17]

Sample code below, since tenant config data can occasionaly change (IE example below need to update the fromToEmailaddress, or maybe a mistake in a tenant config value entered), the queue tenancy needs to be able to serve accurate tenant config values. In the scenario below if there were 100s or 1000s of jobs for 1 tenant in a queue and the tenant fromToEmail address was updated betwen jobs, all jobs would be sent with the stale fromToEmailAddress until either a different tenants job was processed or the queue restarted.

`TenanyServiceProvider
\Stancl\Tenancy\Features\TenantConfig::$storageToConfigMap = [   
    'fromToEmailAddesss' => 'app.fromToEmailAddesss'
]

//tenant route
Route::post('/sendEmail', function () {
    return SendEmailJob::dispatch($someData);
});

//Job
class SendEmailJob implements ShouldQueue
{
    use Dispatchable, InteractsWithQueue, Queueable,  SerializesModels;

    public someData

    public function __construct($someData)
    {
        $this->someData = $someData;        
    }

    public function handle()
    {
        Mail::send('emails.welcome', $data, function($message)
        {
            // fromToEmailAddesss will use stale tenant data in scenario described
            $message->from( Config::get('app.fromToEmailAddesss'), 'Tenancy');           
        });
    }
}`

[stancl]

I see. Is the config the only state that you're having trouble with?

Also, is the data on tenant() correct? Or is it outdated in both tenant() and the config?

[mannis17]

Data is outdated both in config and tenant()

[stancl]

I see. I think I'll solve this in two ways:

1. quick solution: add an option to the QueueTenancyBootstrapper to force-end and reinitialize tenancy between all queues
2. complex solution: add behavior that updates the tenant() model between queues and runs the config feature using some hook

1. will be a slight performance slowdown, but won't matter in queued jobs. 2) will be better, but will take me longer to add

I'll try to get this done within next week 👍🏻

[mannis17]

Ok let me know if there is anything you need from my end.

[waterbuckit]

I actually think I stumbled across something similar a week or so ago - very specifically with AWS credentials. Logging the config during the job showed that the tenancy had indeed initialised fine and the credentials were as expected, but I would intermittently get files being uploaded to arbitrary tenant's S3 buckets (Presumably tenants that had previously run jobs and something was lingering?).

This was only happening in production where I have workers being controlled by supervisord. I had to fix this by running my workers with --max-jobs=1 to force my workers to restart once a single job was processed to prevent state from previous tenants lingering.

[stancl]

Presumably tenants that had previously run jobs and something was lingering

No, state between different tenants should not be kept. The issue here is that tenancy doesn't get ended & reinitialized for the same tenant. Which is generally the wanted behavior, but it can cause issues when the tenant object changes.

[mannis17]

@stancl any update on a solution to this?

[sca1235]

We have same issue for this. Hopefully update fixes queues.

[stancl]

@sca1235 What's the exact behavior in your jobs, for more context?

[Andrei-Dr]

I ran into a similar issue - only using queue:work. QueueTenancyBootstrapper loaded, jobs go to the central db, and get the tenant_id field added if fired from a tenant. However, when the next job is queued (to the same queue name), from a central domain (no tenant_id added to job payload as expected), then the 'central domain' job still runs as the previous job tenant. This seems to be related to framework cache between the calls which gets reinitialized otherwise when using queue:listen

laravel/framework v8.83.1
stancl/tenancy v3.5.1
stancl/jobpipeline v1.5.1

If I manually add in the tenant_id on job __construct, then initialize within handle() and tenancy()->end() after there's no issue but I have to end the job "cleanly" that way each time (vs being able to return out of handle/throw an exception/etc)

[stancl]

However, when the next job is queued (to the same queue name), from a central domain (no tenant_id added to job payload as expected), then the 'central domain' job still runs as the previous job tenant.

This is a slightly different issue, but I'll look into it as well. Appreciate the clear description

[stancl]

@Andrei-Dr Try composer require stancl/tenancy:3.x-dev

[stancl]

Also re: this #790 (comment), I actually don't like the second solution. It adds a lot of complexity (that may add a lot of issues) for a very specific use case.

Instead, I might add an interface that can be selectively applied on jobs, to solve OP's issue.

@mannis17 You can also try composer require stancl/tenancy:3.x-dev now. And add this to any service provider:

\Stancl\Tenancy\Bootstrappers\QueueTenancyBootstrapper::$forceRefresh = true;

And let me know if that fixes the issue for you. If it does, I'll add the interface and tag a release

[Andrei-Dr]

@Andrei-Dr Try composer require stancl/tenancy:3.x-dev

Worked like a charm! tenants are no longer "sticking" across jobs with queue:work now. fwiw, I didn't set $forceRefresh = true, only switched to 3.x-dev and started the worker using queue:work vs queue:listen

[stancl]

Yeah, your issue should be solved just by using 3.x-dev and @mannis17's issue requires that extra setting.

You should switch back to the stable versions when I release this though. So probably watch the repo's releases and then remember to update your composer.json 👍🏻

[Andrei-Dr]

Yeah, definitely sticking to non-dev and keeping an eye out :) Thanks again!
Any idea when you might be releasing this particular fix?

[stancl]

Probably within a week. I'll see if #802 gets done and release them together. Otherwise, I'll release this separately before #802

[mannis17]

@stancl Tested and forceRefresh flag solves the issue

[mannis17]

@stancl Do you have a timeline for getting this merged into a release?

[stancl]

I think today or tomorrow 👍🏻

[stancl]

So, just to confirm: everyone's issues related to this are solved in 3.x, right?

[Andrei-Dr]

So, just to confirm: everyone's issues related to this are solved in 3.x, right?

yup

[mannis17]

So, just to confirm: everyone's issues related to this are solved in 3.x, right?

Correct

[stancl]

Thanks! Will include this in a release soon then

[Andrei-Dr]

Did this get pulled into a release yet?

[mannis17]

Wondering same.

[stancl]

Think I'll be tagging it within 24h, possibly earlier 👍🏻

[Andrei-Dr]

Goooootcha. Sry, I thought it went out already and I was having issues but this explains it. I’ll keep an eye out for the tag update