Repository: archtechx/tenancy (language: php)

Issue:
tenants:run with jobs combined with queue:work keeps multiple database connections open
### Bug description

I am facing a problem while running some jobs with queues combined with `tenants:run`.

I am executing a company command with a heavy job inside (usually i had a supervisor making this task but has been disabled for debugging), all jobs are sent to Redis before will be processed.

The problemas arrives when command is executed for all tenants, in that case, every time I try to run the command, a new connection is open for each tenant keeping previous one open, and finally provoking a collapse in MySQL throwing "Too many connections" when running via supervisor.

Currently problem is solved using queue:listen instead but the performance is better with queue:work command.

Example: 3 tenants in my system, 2 `tenants:run` iterations executed and you can see duplicates connections.

![capture](https://user-images.githubusercontent.com/7864787/204240014-34a987ec-e8ca-442a-9302-8247c96a9f73.JPG)


### Steps to reproduce

- Execute php artisan queue:work myCustomQueue --tries=3 --sleep=3 (queue is ready to accept jobs)

- php artisan tenants:run company:myCommand

### Expected behavior

Keep connections open by tenant and use it instead of create new one.

### Laravel version

8.83.18

### stancl/tenancy version

3.5.7


Discussion / hints:
> Currently problem is solved using queue:listen instead but the performance is better with queue:work command

What's the difference between those two in how the DB connections work?

The problem sounds like the DB connections don't get cleaned up, but `$tenant->run()` *should* be properly cleaning up the old connection.
> > Currently problem is solved using queue:listen instead but the performance is better with queue:work command
> 
> What's the difference between those two in how the DB connections work?
> 
> The problem sounds like the DB connections don't get cleaned up, but `$tenant->run()` _should_ be properly cleaning up the old connection.

Hi,

Main difference between `queue:listen` and `queue:work` is that first one the framework is booted each job (closing connection also) while `queue:work` framework is booted only one time, so it is much more efficient.

I believe that connection need to be closed by tenant run or manually but I am stuck.
```
> isset(app('db')->getConnections()['tenant']);
= false

> $tenant->run(fn () => app('db')->getConnections()['tenant']);
= Illuminate\Database\MySqlConnection {#3828} // tenant connection exists inside the run() Closure

> isset(app('db')->getConnections()['tenant']);
= false // cleaned up afterwards
```

Could it be that you're using `tenants:run` instead of `$tenant->run()`? Could you show the exact code?

It might be that in the example above, the connection is removed from *Laravel's* `DatabaseManager` but is actually still kept open on MySQL.
Hi, I am using `tenants:run` and database connection is open in MySQL (listing from `SHOW FULL PROCESSLIST`)

Here you have an example:

Command class:

```
class MyCommand extends Command
{
    /**
     * {@inheritdoc}
     * @var string
     */
    protected $signature = 'myCompany:my-command';

    /**
     * Execute the console command.
     *
     * @return void
     */
    public function handle(): void
    {
        MyJob::dispatch();
    }
}
```

Job class:
```
class MyJob implements ShouldQueue
{
    use Dispatchable, InteractsWithQueue, Queueable, SerializesModels;

    public function handle()
    {
        logger()->info("running");
    }
}
```

Steps to reproduce:

1. php artisan queue:work --tries=3 --sleep=3 --daemon
2. tenants:run myCompany:my-command
3. Every time I run `tenants:run`, a new connection (per tenant) is open keeping previous one open forever
What if you did:
```php
Tenant::all()->runForEach(function () {
    MyJob::dispatch();
});
```
and called `myCompany:my-command` **without** `tenants:run`.

Does that also reproduce the issue?
Hi again,

Yes, following your advice produce same result
Then my guess is that it's this:

> It might be that in the example above, the connection is removed from Laravel's DatabaseManager but is actually still kept open on MySQL.

We'll look into it 👍🏻 @lukinovec could you try to reproduce this in a testing app?
FWIW regarding the solution, I'm not sure if this will be feasible:
> Keep connections open by tenant and use it instead of create new one.

We may have to close the connections completely and then reopen them as needed.
Yes maybe keep connections open is not feasible, close connects will be a good choise.


@jacksnchz Can you try using[Tenant Aware Commands](https://tenancyforlaravel.com/docs/v3/tenant-aware-commands)?


I am not entirely sure it will work, but I was facing similar problem on my staging (PHP 8).

And this helped. further testing pending
Hi @chinmaypurav,

Thank you for your help but the behaviour using `TenantAwareCommand` is worst than original idea, more connections are kept open.

My temporary solution was replace `queue:work` by `queue:listen` command (loosing performance but it does not overloads the database connections).

Fix patch:
diff --git a/src/Bootstrappers/QueueTenancyBootstrapper.php b/src/Bootstrappers/QueueTenancyBootstrapper.php
index 92c95ef62..3f46a1126 100644
--- a/src/Bootstrappers/QueueTenancyBootstrapper.php
+++ b/src/Bootstrappers/QueueTenancyBootstrapper.php
@@ -41,7 +41,7 @@ class QueueTenancyBootstrapper implements TenancyBootstrapper
      */
     public static function __constructStatic(Application $app): void
     {
-        static::setUpJobListener($app->make(Dispatcher::class), $app->runningUnitTests());
+        static::setUpJobListener($app->make(Dispatcher::class));
     }
 
     public function __construct(Repository $config, QueueManager $queue)
@@ -52,7 +52,7 @@ public function __construct(Repository $config, QueueManager $queue)
         $this->setUpPayloadGenerator();
     }
 
-    protected static function setUpJobListener(Dispatcher $dispatcher, bool $runningTests): void
+    protected static function setUpJobListener(Dispatcher $dispatcher): void
     {
         $previousTenant = null;
 
@@ -69,10 +69,8 @@ protected static function setUpJobListener(Dispatcher $dispatcher, bool $running
         });
 
         // 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);
-            }
+        $revertToPreviousState = function ($event) use (&$previousTenant) {
+            static::revertToPreviousState($event, $previousTenant);
         };
 
         $dispatcher->listen(JobProcessed::class, $revertToPreviousState); // artisan('queue:work') which succeeds


Test patch:
diff --git a/tests/QueueTest.php b/tests/QueueTest.php
index f88b3934d..00678876c 100644
--- a/tests/QueueTest.php
+++ b/tests/QueueTest.php
@@ -110,6 +110,8 @@
 
     pest()->artisan('queue:work --once');
 
+    expect(! tenancy()->initialized)->toBe($shouldEndTenancy);
+
     expect(DB::connection('central')->table('failed_jobs')->count())->toBe(0);
 
     expect(pest()->valuestore->get('tenant_id'))->toBe('The current tenant id is: ' . $tenant->id);
@@ -117,7 +119,7 @@
     $tenant->run(function () use ($user) {
         expect($user->fresh()->name)->toBe('Bar');
     });
-})->with([true, false]);;
+})->with([true, false]);
 
 test('tenancy is initialized when retrying jobs', function (bool $shouldEndTenancy) {
     withFailedJobs();
@@ -144,12 +146,21 @@
 
     pest()->artisan('queue:work --once');
 
+    expect(! tenancy()->initialized)->toBe($shouldEndTenancy);
+
     expect(DB::connection('central')->table('failed_jobs')->count())->toBe(1);
     expect(pest()->valuestore->get('tenant_id'))->toBeNull(); // job failed
 
     pest()->artisan('queue:retry all');
+
+    if ($shouldEndTenancy) {
+        tenancy()->end();
+    }
+
     pest()->artisan('queue:work --once');
 
+    expect(! tenancy()->initialized)->toBe($shouldEndTenancy);
+
     expect(DB::connection('central')->table('failed_jobs')->count())->toBe(0);
 
     expect(pest()->valuestore->get('tenant_id'))->toBe('The current tenant id is: ' . $tenant->id); // job succeeded
@@ -182,6 +193,22 @@
     expect(pest()->valuestore->get('tenant_id'))->toBe('The current tenant id is: acme');
 });
 
+test('tenant connections do not persist after tenant jobs get processed', function() {
+    withTenantDatabases();
+
+    $tenant = Tenant::create();
+
+    tenancy()->initialize($tenant);
+
+    dispatch(new TestJob(pest()->valuestore));
+
+    tenancy()->end();
+
+    pest()->artisan('queue:work --once');
+
+    expect(collect(DB::select('SHOW FULL PROCESSLIST'))->pluck('db'))->not()->toContain($tenant->database()->getName());
+});
+
 function createValueStore(): void
 {
     $valueStorePath = __DIR__ . '/Etc/tmp/queuetest.json';