From b8226be774eb2fc5cc735d11bd85e250347a7ac4 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Tue, 28 Apr 2026 15:28:22 +0200 Subject: [PATCH] refactor(server): dispatch event for reachability notifications, drop retry loop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move reachability notification triggering out of isReachableChanged into a dedicated ServerReachabilityChanged event dispatched by ServerConnectionCheckJob. Remove the blocking 3-attempt sleep loop from isReachableChanged — unreachable_count threshold alone now gates the Unreachable notification. Add feature and unit tests covering all notification dispatch paths. --- app/Jobs/ServerConnectionCheckJob.php | 34 ++++++ app/Models/Server.php | 26 +---- .../ServerReachabilityNotificationTest.php | 105 ++++++++++++++++++ tests/Unit/IsReachableChangedTest.php | 61 ++++++++++ tests/Unit/ServerBackoffTest.php | 51 +++++++++ 5 files changed, 253 insertions(+), 24 deletions(-) create mode 100644 tests/Feature/ServerReachabilityNotificationTest.php create mode 100644 tests/Unit/IsReachableChangedTest.php diff --git a/app/Jobs/ServerConnectionCheckJob.php b/app/Jobs/ServerConnectionCheckJob.php index 7ce316dcd..98ad60fff 100644 --- a/app/Jobs/ServerConnectionCheckJob.php +++ b/app/Jobs/ServerConnectionCheckJob.php @@ -2,6 +2,7 @@ namespace App\Jobs; +use App\Events\ServerReachabilityChanged; use App\Helpers\SshMultiplexingHelper; use App\Models\Server; use App\Services\ConfigurationRepository; @@ -43,6 +44,9 @@ private function disableSshMux(): void public function handle() { + $wasReachable = (bool) $this->server->settings->is_reachable; + $wasNotified = (bool) $this->server->unreachable_notification_sent; + try { // Check if server is disabled if ($this->server->settings->force_disabled) { @@ -84,6 +88,8 @@ public function handle() 'server_ip' => $this->server->ip, ]); + $this->dispatchReachabilityChangedIfNeeded($wasReachable, $wasNotified, false); + return; } @@ -99,6 +105,8 @@ public function handle() $this->server->update(['unreachable_count' => 0]); } + $this->dispatchReachabilityChangedIfNeeded($wasReachable, $wasNotified, true); + } catch (\Throwable $e) { Log::error('ServerConnectionCheckJob failed', [ @@ -111,6 +119,8 @@ public function handle() ]); $this->server->increment('unreachable_count'); + $this->dispatchReachabilityChangedIfNeeded($wasReachable, $wasNotified, false); + return; } } @@ -118,17 +128,41 @@ public function handle() public function failed(?\Throwable $exception): void { if ($exception instanceof TimeoutExceededException) { + $wasReachable = (bool) $this->server->settings->is_reachable; + $wasNotified = (bool) $this->server->unreachable_notification_sent; + $this->server->settings->update([ 'is_reachable' => false, 'is_usable' => false, ]); $this->server->increment('unreachable_count'); + $this->dispatchReachabilityChangedIfNeeded($wasReachable, $wasNotified, false); + // Delete the queue job so it doesn't appear in Horizon's failed list. $this->job?->delete(); } } + /** + * Fire ServerReachabilityChanged when state crosses the unreachable threshold (count >= 2) + * or when a previously-notified server recovers. Skips noise from single transient flaps. + */ + private function dispatchReachabilityChangedIfNeeded(bool $wasReachable, bool $wasNotified, bool $isReachable): void + { + if ($isReachable) { + if (! $wasReachable || $wasNotified) { + ServerReachabilityChanged::dispatch($this->server); + } + + return; + } + + if ($this->server->unreachable_count >= 2 && ! $wasNotified) { + ServerReachabilityChanged::dispatch($this->server); + } + } + private function checkHetznerStatus(): void { $status = null; diff --git a/app/Models/Server.php b/app/Models/Server.php index 06426f211..74e8ba5b0 100644 --- a/app/Models/Server.php +++ b/app/Models/Server.php @@ -1236,10 +1236,8 @@ public function isReachableChanged() $this->refresh(); $unreachableNotificationSent = (bool) $this->unreachable_notification_sent; $isReachable = (bool) $this->settings->is_reachable; - if ($isReachable === true) { - $this->unreachable_count = 0; - $this->save(); + if ($isReachable === true) { if ($unreachableNotificationSent === true) { $this->sendReachableNotification(); } @@ -1247,28 +1245,8 @@ public function isReachableChanged() return; } - $this->increment('unreachable_count'); - - if ($this->unreachable_count === 1) { - $this->settings->is_reachable = true; - $this->settings->save(); - - return; - } - if ($this->unreachable_count >= 2 && ! $unreachableNotificationSent) { - $failedChecks = 0; - for ($i = 0; $i < 3; $i++) { - $status = $this->serverStatus(); - sleep(5); - if (! $status) { - $failedChecks++; - } - } - - if ($failedChecks === 3 && ! $unreachableNotificationSent) { - $this->sendUnreachableNotification(); - } + $this->sendUnreachableNotification(); } } diff --git a/tests/Feature/ServerReachabilityNotificationTest.php b/tests/Feature/ServerReachabilityNotificationTest.php new file mode 100644 index 000000000..e996ba028 --- /dev/null +++ b/tests/Feature/ServerReachabilityNotificationTest.php @@ -0,0 +1,105 @@ +team = Team::factory()->create(); + $this->team->emailNotificationSettings()->update([ + 'use_instance_email_settings' => true, + 'server_unreachable_email_notifications' => true, + 'server_reachable_email_notifications' => true, + ]); + + $this->server = Server::factory()->create(['team_id' => $this->team->id]); + + Notification::fake(); +}); + +it('sends Unreachable notification when threshold reached and not yet notified', function () { + $this->server->settings()->update(['is_reachable' => false]); + $this->server->forceFill([ + 'unreachable_count' => 2, + 'unreachable_notification_sent' => false, + ])->save(); + + ServerReachabilityChanged::dispatch($this->server->fresh()); + + Notification::assertSentTo($this->team, Unreachable::class); + expect($this->server->fresh()->unreachable_notification_sent)->toBeTrue(); +}); + +it('does not send Unreachable on first transient failure (count=1)', function () { + $this->server->settings()->update(['is_reachable' => false]); + $this->server->forceFill([ + 'unreachable_count' => 1, + 'unreachable_notification_sent' => false, + ])->save(); + + ServerReachabilityChanged::dispatch($this->server->fresh()); + + Notification::assertNothingSent(); +}); + +it('does not send Unreachable when already notified', function () { + $this->server->settings()->update(['is_reachable' => false]); + $this->server->forceFill([ + 'unreachable_count' => 5, + 'unreachable_notification_sent' => true, + ])->save(); + + ServerReachabilityChanged::dispatch($this->server->fresh()); + + Notification::assertNothingSent(); +}); + +it('sends Reachable notification on recovery when previously notified', function () { + $this->server->settings()->update(['is_reachable' => true]); + $this->server->forceFill([ + 'unreachable_count' => 0, + 'unreachable_notification_sent' => true, + ])->save(); + + $fresh = $this->server->fresh(); + expect($fresh->unreachable_notification_sent)->toBeTrue(); + expect((bool) $fresh->settings->is_reachable)->toBeTrue(); + + ServerReachabilityChanged::dispatch($fresh); + + Notification::assertSentTo($this->team, Reachable::class); + expect($this->server->fresh()->unreachable_notification_sent)->toBeFalse(); +}); + +it('does not send Reachable when never notified', function () { + $this->server->settings()->update(['is_reachable' => true]); + $this->server->forceFill([ + 'unreachable_count' => 0, + 'unreachable_notification_sent' => false, + ])->save(); + + ServerReachabilityChanged::dispatch($this->server->fresh()); + + Notification::assertNothingSent(); +}); + +it('routes Unreachable notification through EmailChannel when email toggle is on', function () { + $this->server->settings()->update(['is_reachable' => false]); + $this->server->forceFill([ + 'unreachable_count' => 2, + 'unreachable_notification_sent' => false, + ])->save(); + + ServerReachabilityChanged::dispatch($this->server->fresh()); + + Notification::assertSentTo($this->team, Unreachable::class, function ($notification, $channels) { + return in_array(EmailChannel::class, $channels); + }); +}); diff --git a/tests/Unit/IsReachableChangedTest.php b/tests/Unit/IsReachableChangedTest.php new file mode 100644 index 000000000..76f9863bf --- /dev/null +++ b/tests/Unit/IsReachableChangedTest.php @@ -0,0 +1,61 @@ +is_reachable = $isReachable; + + $server = Mockery::mock(Server::class)->makePartial()->shouldAllowMockingProtectedMethods(); + $server->shouldReceive('refresh')->andReturnSelf(); + $server->shouldReceive('getAttribute')->with('settings')->andReturn($settings); + $server->shouldReceive('getAttribute')->with('unreachable_notification_sent')->andReturn($notificationSent); + $server->shouldReceive('getAttribute')->with('unreachable_count')->andReturn($unreachableCount); + + return $server; +} + +it('sends Reachable notification when reachable and notification was previously sent', function () { + $server = makeServerForReachabilityTest(isReachable: true, notificationSent: true, unreachableCount: 0); + $server->shouldReceive('sendReachableNotification')->once(); + $server->shouldNotReceive('sendUnreachableNotification'); + + $server->isReachableChanged(); +}); + +it('does not send any notification when reachable and notification was never sent', function () { + $server = makeServerForReachabilityTest(isReachable: true, notificationSent: false, unreachableCount: 0); + $server->shouldNotReceive('sendReachableNotification'); + $server->shouldNotReceive('sendUnreachableNotification'); + + $server->isReachableChanged(); +}); + +it('sends Unreachable notification when count >= 2 and not yet notified', function () { + $server = makeServerForReachabilityTest(isReachable: false, notificationSent: false, unreachableCount: 2); + $server->shouldReceive('sendUnreachableNotification')->once(); + $server->shouldNotReceive('sendReachableNotification'); + + $server->isReachableChanged(); +}); + +it('does not send Unreachable notification on first transient failure (count=1)', function () { + $server = makeServerForReachabilityTest(isReachable: false, notificationSent: false, unreachableCount: 1); + $server->shouldNotReceive('sendUnreachableNotification'); + $server->shouldNotReceive('sendReachableNotification'); + + $server->isReachableChanged(); +}); + +it('does not double-send Unreachable when already notified', function () { + $server = makeServerForReachabilityTest(isReachable: false, notificationSent: true, unreachableCount: 5); + $server->shouldNotReceive('sendUnreachableNotification'); + $server->shouldNotReceive('sendReachableNotification'); + + $server->isReachableChanged(); +}); diff --git a/tests/Unit/ServerBackoffTest.php b/tests/Unit/ServerBackoffTest.php index bdcefb74f..9f1f747d4 100644 --- a/tests/Unit/ServerBackoffTest.php +++ b/tests/Unit/ServerBackoffTest.php @@ -1,11 +1,13 @@ is_reachable = true; $settings->shouldReceive('update') ->with(['is_reachable' => false, 'is_usable' => false]) ->once(); $server = Mockery::mock(Server::class)->makePartial()->shouldAllowMockingProtectedMethods(); $server->shouldReceive('getAttribute')->with('settings')->andReturn($settings); + $server->shouldReceive('getAttribute')->with('unreachable_notification_sent')->andReturn(false); $server->shouldReceive('increment')->with('unreachable_count')->once(); $server->id = 1; $server->name = 'test-server'; + $server->unreachable_count = 1; // Will become 2 after increment in real code; mock keeps value as-is $job = new ServerConnectionCheckJob($server); $job->failed(new TimeoutExceededException); @@ -152,6 +159,50 @@ }); }); +describe('ServerConnectionCheckJob ServerReachabilityChanged dispatch', function () { + // ServerReachabilityChanged's constructor calls $server->isReachableChanged() — verifying that + // call is a clean proxy for "the event was dispatched", and avoids serializing a Mockery proxy + // through the event dispatcher (which trips Eloquent static method lookups on the proxy class). + $invoke = function (bool $wasReachable, bool $wasNotified, bool $isReachable, int $unreachableCount, bool $expectDispatch) { + $server = Mockery::mock(Server::class)->makePartial()->shouldAllowMockingProtectedMethods(); + $server->shouldReceive('getAttribute')->with('unreachable_count')->andReturn($unreachableCount); + $server->shouldReceive('getAttribute')->with('id')->andReturn(1); + if ($expectDispatch) { + $server->shouldReceive('isReachableChanged')->once()->andReturnNull(); + } else { + $server->shouldNotReceive('isReachableChanged'); + } + + $job = new ServerConnectionCheckJob($server); + $method = new ReflectionMethod($job, 'dispatchReachabilityChangedIfNeeded'); + $method->invoke($job, $wasReachable, $wasNotified, $isReachable); + }; + + it('dispatches event when count crosses unreachable threshold', function () use ($invoke) { + $invoke(true, false, false, 2, true); + }); + + it('does not dispatch on first transient failure (count=1)', function () use ($invoke) { + $invoke(true, false, false, 1, false); + }); + + it('does not dispatch when already notified and still unreachable', function () use ($invoke) { + $invoke(false, true, false, 5, false); + }); + + it('dispatches recovery event when previously unreachable', function () use ($invoke) { + $invoke(false, false, true, 0, true); + }); + + it('dispatches recovery event when previously notified', function () use ($invoke) { + $invoke(true, true, true, 0, true); + }); + + it('does not dispatch when consistently reachable and never notified', function () use ($invoke) { + $invoke(true, false, true, 0, false); + }); +}); + describe('ServerCheckJob unreachable_count', function () { it('increments unreachable_count on timeout', function () { $server = Mockery::mock(Server::class)->makePartial()->shouldAllowMockingProtectedMethods();