From b081673c989686a1689fcc768c73d87667039a0d Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Sat, 3 Oct 2026 23:39:38 +0200 Subject: [PATCH] fix(server): skip Sentinel checks on unreachable servers - CheckAndStartSentinelJob returns early when the server is not functional. It now reads the container state with `docker ps --filter`, so an SSH failure throws and is no longer treated as a missing container that needs a restart. - ServerManagerJob only dispatches the Sentinel version check for functional servers. - On cloud, ServerPatchCheckJob runs on the maintenance queue and ServerLimitCheckJob runs on the webhooks queue. - The default number of cloud maintenance workers goes from 10 to 60. - excludeCertainErrors drops its unused ignore list and is typed as `never`. - Docs and the Horizon config comments are updated, and tests cover the new behavior. --- app/Jobs/CheckAndStartSentinelJob.php | 9 +- app/Jobs/ServerLimitCheckJob.php | 5 +- app/Jobs/ServerManagerJob.php | 2 + app/Jobs/ServerPatchCheckJob.php | 8 +- bootstrap/helpers/remoteProcess.php | 18 ++-- bootstrap/helpers/shared.php | 6 +- config/horizon.php | 9 +- docs/cloud-horizon-workers.md | 17 ++-- tests/Feature/HorizonConfigurationTest.php | 22 ++++- .../Server/CheckAndStartSentinelJobTest.php | 84 +++++++++++++++++++ .../SentinelUpdateCheckIndependenceTest.php | 4 + .../ServerManagerJobSentinelCheckTest.php | 10 +++ 12 files changed, 155 insertions(+), 39 deletions(-) create mode 100644 tests/Feature/Server/CheckAndStartSentinelJobTest.php diff --git a/app/Jobs/CheckAndStartSentinelJob.php b/app/Jobs/CheckAndStartSentinelJob.php index 72213a9432..49e08ab1a6 100644 --- a/app/Jobs/CheckAndStartSentinelJob.php +++ b/app/Jobs/CheckAndStartSentinelJob.php @@ -32,16 +32,15 @@ class CheckAndStartSentinelJob implements ShouldBeEncrypted, ShouldBeUnique, Sho public function handle(): void { - if (! $this->sentinelIsEnabled()) { + if (! $this->sentinelIsEnabled() || ! $this->server->isFunctional()) { return; } $latestVersion = get_latest_sentinel_version(); - // Check if sentinel is running - $sentinelFound = instant_remote_process_with_timeout(['docker inspect coolify-sentinel'], $this->server, false, 10); - $sentinelFoundJson = json_decode($sentinelFound, true); - $sentinelStatus = data_get($sentinelFoundJson, '0.State.Status', 'exited'); + // An SSH failure throws here, so it is never mistaken for a missing container. + // A missing container gives empty output and exit code 0. + $sentinelStatus = instant_remote_process_with_timeout(["docker ps -a --filter 'name=^coolify-sentinel$' --format '{{.State}}'"], $this->server); if ($sentinelStatus !== 'running') { $this->startSentinel($latestVersion); diff --git a/app/Jobs/ServerLimitCheckJob.php b/app/Jobs/ServerLimitCheckJob.php index 06e94fc937..ff4b7531c3 100644 --- a/app/Jobs/ServerLimitCheckJob.php +++ b/app/Jobs/ServerLimitCheckJob.php @@ -23,7 +23,10 @@ class ServerLimitCheckJob implements ShouldBeEncrypted, ShouldQueue return isDev() ? 1 : 3; } - public function __construct(public Team $team) {} + public function __construct(public Team $team) + { + $this->onQueue(webhooks_queue()); + } public function handle() { diff --git a/app/Jobs/ServerManagerJob.php b/app/Jobs/ServerManagerJob.php index e33e639c61..3d9bae6cad 100644 --- a/app/Jobs/ServerManagerJob.php +++ b/app/Jobs/ServerManagerJob.php @@ -171,9 +171,11 @@ class ServerManagerJob implements ShouldBeEncrypted, ShouldQueue } // Sentinel versions that report their version on push are updated by SentinelController. + // Unreachable servers are skipped: the SSH check would only fail. The connection check recovers them. if ($server->isSentinelEnabled() && ! Cache::has(Server::sentinelReportedVersionCacheKey($server->id)) && shouldRunCronNow(self::sentinelVersionCheckCron($server), $serverTimezone, "sentinel-version-check:{$server->id}", $this->executionTime) + && $server->isFunctional() ) { CheckAndStartSentinelJob::dispatch($server); } diff --git a/app/Jobs/ServerPatchCheckJob.php b/app/Jobs/ServerPatchCheckJob.php index 18999c009e..42caa3861c 100644 --- a/app/Jobs/ServerPatchCheckJob.php +++ b/app/Jobs/ServerPatchCheckJob.php @@ -12,6 +12,7 @@ use Illuminate\Foundation\Bus\Dispatchable; use Illuminate\Queue\InteractsWithQueue; use Illuminate\Queue\Middleware\WithoutOverlapping; use Illuminate\Queue\SerializesModels; +use Illuminate\Support\Facades\Log; class ServerPatchCheckJob implements ShouldBeEncrypted, ShouldQueue { @@ -26,7 +27,10 @@ class ServerPatchCheckJob implements ShouldBeEncrypted, ShouldQueue return [(new WithoutOverlapping('server-patch-check-'.$this->server->uuid))->expireAfter(600)->dontRelease()]; } - public function __construct(public Server $server) {} + public function __construct(public Server $server) + { + $this->onQueue(maintenance_queue()); + } public function handle(): void { @@ -57,7 +61,7 @@ class ServerPatchCheckJob implements ShouldBeEncrypted, ShouldQueue } } catch (\Throwable $e) { // Log error but don't fail the job - \Illuminate\Support\Facades\Log::error('ServerPatchCheckJob failed: '.$e->getMessage(), [ + Log::error('ServerPatchCheckJob failed: '.$e->getMessage(), [ 'server_id' => $this->server->id, 'server_name' => $this->server->name, 'error' => $e->getMessage(), diff --git a/bootstrap/helpers/remoteProcess.php b/bootstrap/helpers/remoteProcess.php index 74f5215f4a..e32d3cf63e 100644 --- a/bootstrap/helpers/remoteProcess.php +++ b/bootstrap/helpers/remoteProcess.php @@ -15,7 +15,6 @@ use Illuminate\Database\Eloquent\Model; use Illuminate\Support\Collection; use Illuminate\Support\Facades\Auth; use Illuminate\Support\Facades\Process; -use Illuminate\Support\Str; use Spatie\Activitylog\Contracts\Activity; function remote_process( @@ -208,24 +207,17 @@ function instant_remote_process(Collection|array $command, Server $server, bool ); } -function excludeCertainErrors(string $errorOutput, ?int $exitCode = null) +/** + * Throws the remote command error. Handler::register() never sends RuntimeException to Sentry, + * so SSH key, DNS, and timeout failures on user servers are not reported. + */ +function excludeCertainErrors(string $errorOutput, ?int $exitCode = null): never { - $ignoredErrors = collect([ - 'Permission denied (publickey', - 'Could not resolve hostname', - ]); - $ignored = $ignoredErrors->contains(fn ($error) => Str::contains($errorOutput, $error)); - - // Ensure we always have a meaningful error message $errorMessage = trim($errorOutput); if (empty($errorMessage)) { $errorMessage = "SSH command failed with exit code: $exitCode"; } - if ($ignored) { - // TODO: Create new exception and disable in sentry - throw new RuntimeException($errorMessage, $exitCode); - } throw new RuntimeException($errorMessage, $exitCode); } diff --git a/bootstrap/helpers/shared.php b/bootstrap/helpers/shared.php index a3be2667ba..eccba95fd1 100644 --- a/bootstrap/helpers/shared.php +++ b/bootstrap/helpers/shared.php @@ -1055,7 +1055,7 @@ function crons_queue(): string /** * Resolve the queue used for slow server maintenance — scheduled, manual and stop-triggered - * Docker cleanups. + * Docker cleanups, and weekly server patch checks. * * On cloud these jobs run on a dedicated `maintenance` queue so a small, bounded Horizon pool * drains them and slow remote prunes cannot occupy the `high` workers; self-hosted keeps them @@ -1071,8 +1071,8 @@ function maintenance_queue(): string } /** - * Resolve the queue used for incoming webhook processing — GitHub pull request webhooks and - * Stripe events. + * Resolve the queue used for incoming webhook processing — GitHub pull request webhooks, + * Stripe events, and the server limit checks that follow subscription changes. * * On cloud these jobs run on a dedicated `webhooks` queue so a busy `high` queue cannot delay * them; self-hosted keeps them on the shared `high` queue, so a custom `HORIZON_QUEUES` does diff --git a/config/horizon.php b/config/horizon.php index c29ad15fc9..6c1a781173 100644 --- a/config/horizon.php +++ b/config/horizon.php @@ -39,12 +39,13 @@ $selfHostedSupervisor = $workerOptions + [ | positive integer) fall back to the default. | | The `maintenance` pool (see maintenance_queue()) runs slow remote Docker -| cleanups. It is small on purpose: it bounds how many cleanups run at once -| per node, and it comes on top of the other pools, so cleanups never take +| cleanups and server patch checks. It bounds how many of them run at once +| per node, and it comes on top of the other pools, so they never take | deployment, cron or high workers. | | The `webhooks` pool (see webhooks_queue()) processes GitHub pull request -| webhooks and Stripe events, so other jobs cannot delay them. +| webhooks, Stripe events, and server limit checks, so other jobs cannot +| delay them. */ $cloudSupervisors = []; @@ -53,7 +54,7 @@ foreach ([ 'crons' => ['HORIZON_CRONS_PROCESSES', 60], 'high' => ['HORIZON_HIGH_PROCESSES', 60], 'default' => ['HORIZON_DEFAULT_PROCESSES', 40], - 'maintenance' => ['HORIZON_MAINTENANCE_PROCESSES', 10], + 'maintenance' => ['HORIZON_MAINTENANCE_PROCESSES', 60], 'webhooks' => ['HORIZON_WEBHOOKS_PROCESSES', 10], ] as $queue => [$processesEnv, $defaultProcesses]) { $processes = filter_var(env($processesEnv), FILTER_VALIDATE_INT, [ diff --git a/docs/cloud-horizon-workers.md b/docs/cloud-horizon-workers.md index fa494c65aa..926e6a9f9c 100644 --- a/docs/cloud-horizon-workers.md +++ b/docs/cloud-horizon-workers.md @@ -10,18 +10,19 @@ one fixed-size supervisor per queue. A busy queue cannot starve the others. | `crons` | `crons` | `HORIZON_CRONS_PROCESSES` | 60 | | `high` | `high` | `HORIZON_HIGH_PROCESSES` | 60 | | `default` | `default` | `HORIZON_DEFAULT_PROCESSES` | 40 | -| `maintenance` | `maintenance` | `HORIZON_MAINTENANCE_PROCESSES` | 10 | +| `maintenance` | `maintenance` | `HORIZON_MAINTENANCE_PROCESSES` | 60 | | `webhooks` | `webhooks` | `HORIZON_WEBHOOKS_PROCESSES` | 10 | -- Counts apply to **each node**. Two nodes with the defaults run 480 workers. +- Counts apply to **each node**. Two nodes with the defaults run 580 workers. - `maintenance` runs Docker cleanups (scheduled, manual, and the cleanup - that runs after a resource stops). Remote prunes can be slow, so this pool - is small on purpose: it limits how many cleanups run at the same time and - keeps them away from the `high` workers. It is added on top of the other + that runs after a resource stops) and the weekly server patch checks. + Remote prunes and package checks can be slow, so this pool limits how many + of them run at the same time and keeps them away from the `high` workers. It is added on top of the other pools. Each cleanup has a 600 s timeout, and only one cleanup runs per server at a time. Self-hosted instances keep cleanups on `high`. -- `webhooks` processes GitHub pull request webhooks and Stripe events, so a - full `high` queue cannot delay them. These jobs are short (60 s timeout or +- `webhooks` processes GitHub pull request webhooks, Stripe events, and the + server limit checks that follow subscription changes, so a full `high` + queue cannot delay them. These jobs are short (60 s timeout or less). Self-hosted instances keep them on `high`. - Each pool has `minProcesses = maxProcesses` and `balance = false`. Horizon does not scale them. @@ -48,7 +49,7 @@ HORIZON_DEPLOYMENTS_PROCESSES=60 HORIZON_CRONS_PROCESSES=60 HORIZON_HIGH_PROCESSES=60 HORIZON_DEFAULT_PROCESSES=40 -HORIZON_MAINTENANCE_PROCESSES=10 +HORIZON_MAINTENANCE_PROCESSES=60 HORIZON_WEBHOOKS_PROCESSES=10 # Worker timeout. Must stay above the longest job timeout (36000 s) and below # retry_after (86400 s). The config clamps it to 36600..85800. diff --git a/tests/Feature/HorizonConfigurationTest.php b/tests/Feature/HorizonConfigurationTest.php index 70a2d4d6ea..52d5742a3c 100644 --- a/tests/Feature/HorizonConfigurationTest.php +++ b/tests/Feature/HorizonConfigurationTest.php @@ -1,7 +1,11 @@ toBe(['deployments', 'crons', 'high', 'default', 'maintenance', 'webhooks']); - foreach (['deployments' => 60, 'crons' => 60, 'high' => 60, 'default' => 40, 'maintenance' => 10, 'webhooks' => 10] as $queue => $processes) { + foreach (['deployments' => 60, 'crons' => 60, 'high' => 60, 'default' => 40, 'maintenance' => 60, 'webhooks' => 10] as $queue => $processes) { $pool = $production[$queue]; expect($pool->queue)->toBe($queue) ->and($pool->balancing())->toBeFalse() @@ -140,7 +144,7 @@ test('cloud pool sizes come from env and invalid values fall back to defaults', 'HORIZON_MAINTENANCE_PROCESSES' => 'ten', ]))['production']; - expect($fallback['maintenance']->maxProcesses)->toBe(10); + expect($fallback['maintenance']->maxProcesses)->toBe(60); }); test('cloud queue helpers route to queues that have a dedicated pool', function () { @@ -185,9 +189,21 @@ test('webhook jobs run on the webhooks queue on cloud and on high on self-hosted fullName: 'coollabsio/coolify', ); + $serverLimitJob = new ServerLimitCheckJob(new Team); + expect($stripeJob->queue)->toBe($queue) - ->and($githubJob->queue)->toBe($queue); + ->and($githubJob->queue)->toBe($queue) + ->and($serverLimitJob->queue)->toBe($queue); })->with([ 'cloud' => [false, 'webhooks'], 'self-hosted' => [true, 'high'], ]); + +test('server patch checks run on the maintenance queue on cloud and on high on self-hosted', function (bool $selfHosted, string $queue) { + config(['constants.coolify.self_hosted' => $selfHosted]); + + expect((new ServerPatchCheckJob(new Server))->queue)->toBe($queue); +})->with([ + 'cloud' => [false, 'maintenance'], + 'self-hosted' => [true, 'high'], +]); diff --git a/tests/Feature/Server/CheckAndStartSentinelJobTest.php b/tests/Feature/Server/CheckAndStartSentinelJobTest.php new file mode 100644 index 0000000000..b2a33f809d --- /dev/null +++ b/tests/Feature/Server/CheckAndStartSentinelJobTest.php @@ -0,0 +1,84 @@ + 0]); + Storage::fake('ssh-keys'); + Storage::fake('ssh-mux'); + config()->set('constants.ssh.max_retries', 1); + Http::fake(['*' => Http::response(['coolify' => ['sentinel' => ['version' => '1.0.0']]])]); + + $user = User::factory()->create(); + $team = $user->teams()->first(); + $this->server = Server::factory()->create([ + 'team_id' => $team->id, + 'private_key_id' => PrivateKey::factory()->create(['team_id' => $team->id])->id, + ]); + $this->server->settings->update([ + 'is_sentinel_enabled' => true, + 'is_reachable' => true, + 'is_usable' => true, + ]); + $this->server->refresh(); +}); + +it('does not connect to an unreachable or unusable server', function (string $flag) { + $this->server->settings->update([$flag => false]); + Process::fake(); + StartSentinel::shouldNotRun(); + + (new CheckAndStartSentinelJob($this->server->refresh()))->handle(); + + Process::assertNothingRan(); +})->with(['is_reachable', 'is_usable']); + +it('starts Sentinel when the container is missing', function () { + Process::fake(['*' => Process::result(output: '')]); + StartSentinel::shouldRun()->once(); + + (new CheckAndStartSentinelJob($this->server))->handle(); +}); + +it('does not treat an SSH failure as a missing container', function () { + Process::fake(['*' => Process::result(errorOutput: 'ssh: connect to host 1.2.3.4 port 22: Connection timed out', exitCode: 255)]); + StartSentinel::shouldNotRun(); + + expect(fn () => (new CheckAndStartSentinelJob($this->server))->handle()) + ->toThrow(RuntimeException::class, 'Connection timed out'); +}); + +it('does not restart a running Sentinel that is up to date', function () { + Process::fake([ + '*docker ps*' => Process::result(output: 'running'), + '*api/version*' => Process::result(output: '1.0.0'), + ]); + StartSentinel::shouldNotRun(); + + (new CheckAndStartSentinelJob($this->server))->handle(); +}); + +it('uses sudo for the status check on a non-root server', function () { + $this->server->update(['user' => 'ubuntu']); + Process::fake([ + '*docker ps*' => Process::result(output: 'running'), + '*api/version*' => Process::result(output: '1.0.0'), + ]); + StartSentinel::shouldNotRun(); + + (new CheckAndStartSentinelJob($this->server->refresh()))->handle(); + + Process::assertRan(fn ($process) => str_contains($process->command, 'sudo docker ps -a')); +}); diff --git a/tests/Feature/Server/SentinelUpdateCheckIndependenceTest.php b/tests/Feature/Server/SentinelUpdateCheckIndependenceTest.php index 65a8157dcd..7ceefef4b7 100644 --- a/tests/Feature/Server/SentinelUpdateCheckIndependenceTest.php +++ b/tests/Feature/Server/SentinelUpdateCheckIndependenceTest.php @@ -30,6 +30,8 @@ beforeEach(function () { $this->server->settings->update([ 'is_sentinel_enabled' => true, 'server_timezone' => 'UTC', + 'is_reachable' => true, + 'is_usable' => true, ]); $this->server->refresh(); @@ -169,6 +171,8 @@ it('handles multiple servers with different sentinel eligibility', function () { $server3->settings->update([ 'is_sentinel_enabled' => true, 'server_timezone' => 'UTC', + 'is_reachable' => true, + 'is_usable' => true, ]); $instanceSettings = InstanceSettings::first(); diff --git a/tests/Unit/ServerManagerJobSentinelCheckTest.php b/tests/Unit/ServerManagerJobSentinelCheckTest.php index f17b3957be..f7d3c1b1af 100644 --- a/tests/Unit/ServerManagerJobSentinelCheckTest.php +++ b/tests/Unit/ServerManagerJobSentinelCheckTest.php @@ -60,6 +60,16 @@ it('dispatches an hourly Sentinel version check for a healthy Sentinel', functio }); }); +it('skips the hourly Sentinel version check when the server is unreachable or unusable', function (string $flag) { + $server = createSentinelCheckServer($this->team, Carbon::now()); + Carbon::setTestNow(Carbon::now()->setMinute($server->id % 60)); + $server->settings->update([$flag => false]); + + (new ServerManagerJob)->handle(); + + Queue::assertNotPushed(CheckAndStartSentinelJob::class); +})->with(['is_reachable', 'is_usable']); + it('skips the hourly SSH version check when Sentinel reports its version on push', function () { $server = createSentinelCheckServer($this->team, Carbon::now()); Cache::put(Server::sentinelReportedVersionCacheKey($server->id), '1.0.2', now()->addHours(2));