fix(sentinel): prevent concurrent starts and duplicate restarts (#12100)

app/Actions/Server/StartSentinel.php

@@ -4,12 +4,25 @@ namespace App\Actions\Server;
 
 use App\Events\SentinelRestarted;
 use App\Models\Server;
+use Illuminate\Contracts\Cache\LockTimeoutException;
+use Illuminate\Support\Facades\Cache;
 use Lorisleiva\Actions\Concerns\AsAction;
 
 class StartSentinel
 {
     use AsAction;
 
+    /** Longer than one start, including the image pull. */
+    public const LOCK_SECONDS = 300;
+
+    /** Shorter than the 120 second timeout of CheckAndStartSentinelJob. */
+    public const LOCK_WAIT_SECONDS = 90;
+
+    public static function lockKey(Server $server): string
+    {
+        return 'sentinel-start:'.$server->uuid;
+    }
+
     /**
      * Sentinel and the proxy both mount this host path, and Sentinel reads the access log at the same path.
      */
@@ -74,11 +87,27 @@ class StartSentinel
         ];
     }
 
-    public function handle(Server $server, bool $restart = false, ?string $latestVersion = null, ?string $customImage = null)
+    /**
+     * @throws LockTimeoutException when another start for the server does not finish in time
+     */
+    public function handle(Server $server, bool $restart = false, ?string $latestVersion = null, ?string $customImage = null): void
     {
         if ($server->isSwarm() || $server->isBuildServer()) {
             return;
         }
+
+        // Two starts at the same time both remove the container, and then the second `docker run`
+        // fails because the first one already uses the coolify-sentinel name.
+        Cache::lock(self::lockKey($server), self::LOCK_SECONDS)->block(
+            self::LOCK_WAIT_SECONDS,
+            fn () => $this->start($server, $restart, $latestVersion, $customImage),
+        );
+    }
+
+    private function start(Server $server, bool $restart, ?string $latestVersion, ?string $customImage): void
+    {
+        // A start that waited for the lock must use the settings saved in the meantime.
+        $server->refresh();
         if ($restart) {
             StopSentinel::run($server);
         }

app/Livewire/Server/Charts.php

@@ -69,7 +69,8 @@ class Charts extends Component
         try {
             $this->authorize('update', $this->server);
             $this->server->settings->is_metrics_enabled = ! $this->server->settings->is_metrics_enabled;
-            $this->server->settings->save();
+            // Saved quietly: Sentinel is started below, so the settings hook must not start it a second time.
+            $this->server->settings->saveQuietly();
             $this->auditSentinelUpdate(['is_metrics_enabled']);
             $this->server->refresh();
 

app/Livewire/Server/Sentinel.php

@@ -188,12 +188,15 @@ class Sentinel extends Component
         try {
             $this->authorize('update', $this->server);
             $changedFields = $this->syncData(true);
-            if ($changedFields !== []) {
-                auditLog('ui.server.sentinel.updated', $this->auditContext([
-                    'changed_fields' => $changedFields,
-                ]));
+            if ($changedFields === []) {
+                return;
             }
-            $this->restartSentinel();
+            auditLog('ui.server.sentinel.updated', $this->auditContext([
+                'changed_fields' => $changedFields,
+            ]));
+            // Saving the setting restarts Sentinel (ServerSetting::booted()).
+            $this->setSentinelRestarting();
+            $this->dispatch('info', 'Restarting Sentinel.');
         } catch (\Throwable $e) {
             return handleError($e, $this);
         }

app/Livewire/Server/Show.php

@@ -453,8 +453,8 @@ class Show extends Component
     public function updatedIsSentinelDebugEnabled($value)
     {
         try {
+            // Saving the setting restarts Sentinel (ServerSetting::booted()).
             $this->submit();
-            $this->restartSentinel();
         } catch (\Throwable $e) {
             return handleError($e, $this);
         }
@@ -463,8 +463,8 @@ class Show extends Component
     public function updatedIsMetricsEnabled($value)
     {
         try {
+            // Saving the setting restarts Sentinel (ServerSetting::booted()).
             $this->submit();
-            $this->restartSentinel();
         } catch (\Throwable $e) {
             return handleError($e, $this);
         }

app/Models/ServerSetting.php

@@ -207,6 +207,8 @@ class ServerSetting extends Model
             if (
                 $settings->wasChanged('sentinel_token') ||
                 $settings->wasChanged('sentinel_custom_url') ||
+                $settings->wasChanged('is_metrics_enabled') ||
+                $settings->wasChanged('is_sentinel_debug_enabled') ||
                 $settings->wasChanged('sentinel_metrics_refresh_rate_seconds') ||
                 $settings->wasChanged('sentinel_metrics_history_days') ||
                 $settings->wasChanged('sentinel_push_interval_seconds') ||

tests/Feature/Api/ServerSubsystemsApiTest.php

@@ -1,5 +1,6 @@
 <?php
 
+use App\Actions\Server\StartSentinel;
 use App\Jobs\DockerCleanupJob;
 use App\Models\DockerCleanupExecution;
 use App\Models\InstanceSettings;
@@ -8,6 +9,7 @@ use App\Models\Team;
 use App\Models\User;
 use Illuminate\Foundation\Testing\RefreshDatabase;
 use Illuminate\Support\Facades\Queue;
+use Lorisleiva\Actions\Decorators\JobDecorator;
 
 uses(RefreshDatabase::class);
 
@@ -201,12 +203,12 @@ describe('Log drains API', function () {
 
 describe('Sentinel API', function () {
     test('GET returns sentinel settings without token without read:sensitive', function () {
-        // Avoid fields that trigger restartSentinel() on save (token/url/metrics timing).
-        $this->server->settings->update([
+        // Saved quietly: a metrics change restarts Sentinel, which needs SSH.
+        $this->server->settings->forceFill([
             'is_sentinel_enabled' => true,
             'is_metrics_enabled' => true,
             'is_sentinel_debug_enabled' => false,
-        ]);
+        ])->saveQuietly();
 
         $readToken = $this->user->createToken('server-subsystems-read', ['read'])->plainTextToken;
 
@@ -225,8 +227,9 @@ describe('Sentinel API', function () {
             ->and($response->json())->not->toHaveKey('sentinel_custom_url');
     });
 
-    test('PATCH updates sentinel settings for own team server', function () {
-        // Only toggle fields that do not restart Sentinel (avoids remote StartSentinel on sync queue).
+    test('PATCH updates sentinel settings for own team server and restarts Sentinel once', function () {
+        Queue::fake();
+
         $this->withHeaders(serverSubsystemsHeaders())
             ->patchJson("/api/v1/servers/{$this->server->uuid}/sentinel", [
                 'is_metrics_enabled' => true,
@@ -239,6 +242,9 @@ describe('Sentinel API', function () {
         $settings = $this->server->settings->fresh();
         expect((bool) $settings->is_metrics_enabled)->toBeTrue()
             ->and((bool) $settings->is_sentinel_debug_enabled)->toBeTrue();
+        // Sentinel reads both settings from its environment, so it must be recreated.
+        expect(Queue::pushed(JobDecorator::class, fn (JobDecorator $job): bool => $job->getAction() instanceof StartSentinel))
+            ->toHaveCount(1);
     });
 
     test('PATCH rejects disabling mandatory Sentinel', function () {

tests/Feature/Server/ServerSettingSentinelRestartTest.php

@@ -192,3 +192,14 @@ it('does not detect changes when sentinel field is set to same value', function
 
     expect($changeDetected)->toBeFalse();
 });
+
+it('restarts sentinel when a setting in its environment changes while sentinel is enabled', function (string $field) {
+    $settings = $this->server->settings;
+    $settings->is_sentinel_enabled = true;
+    $settings->save();
+
+    $settings->{$field} = ! $settings->{$field};
+    $settings->save();
+
+    Queue::assertPushed(JobDecorator::class, fn ($job) => isStartSentinelJob($job));
+})->with(['is_metrics_enabled', 'is_sentinel_debug_enabled']);

tests/Feature/Server/StartSentinelLockTest.php

@@ -0,0 +1,98 @@
+<?php
+
+use App\Actions\Server\StartSentinel;
+use App\Events\SentinelRestarted;
+use App\Livewire\Server\Charts;
+use App\Livewire\Server\Sentinel;
+use App\Models\InstanceSettings;
+use App\Models\PrivateKey;
+use App\Models\Server;
+use App\Models\User;
+use Illuminate\Foundation\Testing\RefreshDatabase;
+use Illuminate\Support\Facades\Cache;
+use Illuminate\Support\Facades\Event;
+use Illuminate\Support\Facades\Process;
+use Illuminate\Support\Facades\Queue;
+use Livewire\Livewire;
+use Lorisleiva\Actions\Decorators\JobDecorator;
+
+uses(RefreshDatabase::class);
+
+beforeEach(function () {
+    Server::flushIdentityMap();
+    Queue::fake();
+    Event::fake([SentinelRestarted::class]);
+    InstanceSettings::forceCreate(['id' => 0]);
+    config(['constants.ssh.mux_enabled' => false]);
+
+    $this->user = User::factory()->create();
+    $this->team = $this->user->teams()->first();
+    $this->actingAs($this->user);
+    session(['currentTeam' => $this->team]);
+
+    $this->server = Server::factory()->create([
+        'team_id' => $this->team->id,
+        'private_key_id' => PrivateKey::factory()->create(['team_id' => $this->team->id])->id,
+    ]);
+    $this->server->settings->forceFill([
+        'is_sentinel_enabled' => true,
+        'sentinel_custom_url' => 'https://coolify.example.com',
+    ])->saveQuietly();
+});
+
+function queuedSentinelStarts(): int
+{
+    return Queue::pushed(JobDecorator::class, fn (JobDecorator $job): bool => $job->getAction() instanceof StartSentinel)->count();
+}
+
+it('holds the per-server lock while it replaces the Sentinel container', function () {
+    $lockHeldDuringDockerRun = null;
+    Process::fake(function ($process) use (&$lockHeldDuringDockerRun) {
+        $script = is_array($process->command) ? implode(' ', $process->command) : $process->command;
+        if (str_contains($script, 'docker run -d')) {
+            $lockHeldDuringDockerRun = ! Cache::lock(StartSentinel::lockKey($this->server), 1)->get();
+        }
+
+        return Process::result(output: '');
+    });
+
+    StartSentinel::run($this->server, restart: true, latestVersion: '1.0.2');
+
+    expect($lockHeldDuringDockerRun)->toBeTrue()
+        ->and(Cache::lock(StartSentinel::lockKey($this->server), 1)->get())->toBeTrue();
+});
+
+it('starts Sentinel with the settings saved after the server was loaded', function () {
+    $staleServer = Server::query()->findOrFail($this->server->id);
+    $staleServer->settings;
+    Server::query()->findOrFail($this->server->id)->settings->forceFill(['sentinel_push_interval_seconds' => 33])->saveQuietly();
+
+    $scripts = [];
+    Process::fake(function ($process) use (&$scripts) {
+        $scripts[] = is_array($process->command) ? implode(' ', $process->command) : $process->command;
+
+        return Process::result(output: '');
+    });
+
+    StartSentinel::run($staleServer, restart: true, latestVersion: '1.0.2');
+
+    expect(collect($scripts)->first(fn (string $script): bool => str_contains($script, 'docker run -d')))
+        ->toContain('PUSH_INTERVAL_SECONDS=33');
+});
+
+it('starts Sentinel once when the metrics page toggles metrics', function () {
+    StartSentinel::shouldRun()->once();
+
+    Livewire::test(Charts::class, ['server_uuid' => $this->server->uuid])
+        ->call('toggleMetrics');
+
+    expect(queuedSentinelStarts())->toBe(0);
+});
+
+it('restarts Sentinel once when the Sentinel page changes debug logging', function () {
+    Livewire::test(Sentinel::class, ['server' => $this->server])
+        ->set('isSentinelDebugEnabled', true)
+        ->call('instantSave');
+
+    expect(queuedSentinelStarts())->toBe(1);
+});