From b5393e24ec64a7442e52d8434e40de685afd31c4 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Wed, 30 Sep 2026 23:45:55 +0200 Subject: [PATCH] fix(server): recheck unreachable servers before failing deployments Deployments failed with "Server is not functional" when the server was still flagged unreachable after a single failed scheduled check. Add Server::isFunctionalAfterRecheck(), which runs a live SSH and Docker connection check when the server is marked unreachable. It skips the check for disabled servers and servers with placeholder IPs, then re-evaluates isFunctional(). ApplicationDeploymentJob now uses it. ServerManagerJob no longer skips the SSH connection check for servers with a live Sentinel when the server is flagged unreachable or unusable. The Sentinel heartbeat does not restore these flags, so without the check they would never recover. Add feature tests for the recheck behavior and unit tests covering the Sentinel skip condition. --- app/Jobs/ApplicationDeploymentJob.php | 2 +- app/Jobs/ServerManagerJob.php | 7 +- app/Models/Server.php | 19 ++++ tests/Feature/ServerFunctionalRecheckTest.php | 94 +++++++++++++++++++ .../ServerManagerJobSentinelCheckTest.php | 17 ++++ 5 files changed, 136 insertions(+), 3 deletions(-) create mode 100644 tests/Feature/ServerFunctionalRecheckTest.php diff --git a/app/Jobs/ApplicationDeploymentJob.php b/app/Jobs/ApplicationDeploymentJob.php index a5f421da3d..145f08093c 100644 --- a/app/Jobs/ApplicationDeploymentJob.php +++ b/app/Jobs/ApplicationDeploymentJob.php @@ -329,7 +329,7 @@ class ApplicationDeploymentJob implements ShouldBeEncrypted, ShouldQueue 'status' => ApplicationDeploymentStatus::IN_PROGRESS->value, 'horizon_job_worker' => gethostname(), ]); - if ($this->server->isFunctional() === false) { + if ($this->server->isFunctionalAfterRecheck() === false) { $this->application_deployment_queue->addLogEntry('Server is not functional.'); $this->fail('Server is not functional.'); diff --git a/app/Jobs/ServerManagerJob.php b/app/Jobs/ServerManagerJob.php index 20b60b9fd2..a3710532b5 100644 --- a/app/Jobs/ServerManagerJob.php +++ b/app/Jobs/ServerManagerJob.php @@ -110,8 +110,11 @@ class ServerManagerJob implements ShouldBeEncrypted, ShouldQueue return; } - // Skip SSH connection check if Sentinel is healthy — its heartbeat already proves connectivity - if ($server->isSentinelEnabled() && $server->isSentinelLive()) { + // Skip SSH connection check if Sentinel is healthy — its heartbeat already proves connectivity. + // Never skip while the server is marked unreachable or unusable: the heartbeat does not + // restore these flags, so deployments would fail with "Server is not functional" forever. + if ($server->isSentinelEnabled() && $server->isSentinelLive() + && $server->settings->is_reachable && $server->settings->is_usable) { return; } if ($this->shouldSkipDueToBackoff($server)) { diff --git a/app/Models/Server.php b/app/Models/Server.php index 141b3c77c5..04683c8e2c 100644 --- a/app/Models/Server.php +++ b/app/Models/Server.php @@ -1630,6 +1630,25 @@ $siteAddress { return $isFunctional; } + /** + * Like isFunctional(), but runs a live SSH and Docker check when the server is marked + * unreachable. The flag can be stale after one failed scheduled check. + */ + public function isFunctionalAfterRecheck(): bool + { + if ($this->isFunctional()) { + return true; + } + if ($this->settings->is_reachable || $this->settings->force_disabled || $this->hasPlaceholderIp()) { + return false; + } + + (new ServerConnectionCheckJob($this, disableMux: false))->handle(); + $this->settings->refresh(); + + return $this->isFunctional(); + } + public function isLogDrainEnabled() { return $this->settings->is_logdrain_newrelic_enabled || $this->settings->is_logdrain_highlight_enabled || $this->settings->is_logdrain_axiom_enabled || $this->settings->is_logdrain_custom_enabled; diff --git a/tests/Feature/ServerFunctionalRecheckTest.php b/tests/Feature/ServerFunctionalRecheckTest.php new file mode 100644 index 0000000000..8e1b09bdc9 --- /dev/null +++ b/tests/Feature/ServerFunctionalRecheckTest.php @@ -0,0 +1,94 @@ +create(); + $privateKey = PrivateKey::factory()->create(['team_id' => $team->id]); + $server = Server::factory()->create([ + 'team_id' => $team->id, + 'private_key_id' => $privateKey->id, + 'ip' => '203.0.113.10', + ]); + $server->settings->update($settings); + + return $server->refresh(); +} + +it('marks a server functional again when the live SSH and Docker checks pass', function () { + $server = createServerForFunctionalRecheckTest(['is_reachable' => false, 'is_usable' => false]); + + Process::fake([ + '*' => Process::result(output: '{"Server":{"Version":"29.4.3"}}', exitCode: 0), + ]); + + expect($server->isFunctionalAfterRecheck())->toBeTrue() + ->and((bool) $server->settings->fresh()->is_reachable)->toBeTrue() + ->and((bool) $server->settings->fresh()->is_usable)->toBeTrue(); +}); + +it('keeps a server not functional when the live SSH check fails', function () { + $server = createServerForFunctionalRecheckTest(['is_reachable' => false, 'is_usable' => false]); + + Process::fake([ + '*' => Process::result(errorOutput: 'Connection refused', exitCode: 255), + ]); + + expect($server->isFunctionalAfterRecheck())->toBeFalse() + ->and((bool) $server->settings->fresh()->is_reachable)->toBeFalse(); +}); + +it('does not connect to a reachable server that is marked unusable', function () { + $server = createServerForFunctionalRecheckTest(['is_reachable' => true, 'is_usable' => false]); + + Process::fake(); + + expect($server->isFunctionalAfterRecheck())->toBeFalse(); + + Process::assertNothingRan(); +}); + +it('keeps a server not functional when Docker does not respond', function () { + $server = createServerForFunctionalRecheckTest(['is_reachable' => false, 'is_usable' => false]); + + Process::fake([ + '*docker*' => Process::result(output: '', exitCode: 1), + '*' => Process::result(output: '', exitCode: 0), + ]); + + expect($server->isFunctionalAfterRecheck())->toBeFalse() + ->and((bool) $server->settings->fresh()->is_usable)->toBeFalse(); +}); + +it('does not connect to a server that is already functional', function () { + $server = createServerForFunctionalRecheckTest(['is_reachable' => true, 'is_usable' => true]); + + Process::fake(); + + expect($server->isFunctionalAfterRecheck())->toBeTrue(); + + Process::assertNothingRan(); +}); + +it('does not connect to a disabled server', function () { + $server = createServerForFunctionalRecheckTest(['is_reachable' => false, 'is_usable' => false, 'force_disabled' => true]); + + Process::fake(); + + expect($server->isFunctionalAfterRecheck())->toBeFalse(); + + Process::assertNothingRan(); +}); diff --git a/tests/Unit/ServerManagerJobSentinelCheckTest.php b/tests/Unit/ServerManagerJobSentinelCheckTest.php index 5e87bcf7fe..df1b1c3ee6 100644 --- a/tests/Unit/ServerManagerJobSentinelCheckTest.php +++ b/tests/Unit/ServerManagerJobSentinelCheckTest.php @@ -35,6 +35,8 @@ function createSentinelCheckServer(Team $team, Carbon $sentinelUpdatedAt, Server 'server_timezone' => 'UTC', 'server_role' => $role, 'is_build_server' => $role === ServerRole::BUILD, + 'is_reachable' => true, + 'is_usable' => true, ]); return $server->refresh(); @@ -76,6 +78,21 @@ it('skips ServerConnectionCheckJob when sentinel is live', function () { Queue::assertNotPushed(ServerConnectionCheckJob::class); }); +it('dispatches ServerConnectionCheckJob when sentinel is live but the server is marked unusable', function (string $flag) { + // A Sentinel heartbeat does not restore these flags, so only the SSH check can recover them + $server = createSentinelCheckServer($this->team, Carbon::now()); + $server->settings->update([$flag => false]); + + expect($server->isSentinelLive())->toBeTrue(); + + $job = new ServerManagerJob; + $job->handle(); + + Queue::assertPushed(ServerConnectionCheckJob::class, function ($job) use ($server) { + return $job->server->id === $server->id; + }); +})->with(['is_reachable', 'is_usable']); + it('dispatches ServerConnectionCheckJob when sentinel is not live', function () { $server = createSentinelCheckServer($this->team, Carbon::now()->subMinutes(10));