Skip to content

Commit b8226be

Browse files
andrasbacsaiandrasbacsai
committed
refactor(server): dispatch event for reachability notifications, drop retry loop
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.
1 parent 5c89a70 commit b8226be

5 files changed

Lines changed: 253 additions & 24 deletions

File tree

‎app/Jobs/ServerConnectionCheckJob.php‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
namespace App\Jobs;
44

5+
use App\Events\ServerReachabilityChanged;
56
use App\Helpers\SshMultiplexingHelper;
67
use App\Models\Server;
78
use App\Services\ConfigurationRepository;
@@ -43,6 +44,9 @@ private function disableSshMux(): void
4344

4445
public function handle()
4546
{
47+
$wasReachable = (bool) $this->server->settings->is_reachable;
48+
$wasNotified = (bool) $this->server->unreachable_notification_sent;
49+
4650
try {
4751
// Check if server is disabled
4852
if ($this->server->settings->force_disabled) {
@@ -84,6 +88,8 @@ public function handle()
8488
'server_ip' => $this->server->ip,
8589
]);
8690

91+
$this->dispatchReachabilityChangedIfNeeded($wasReachable, $wasNotified, false);
92+
8793
return;
8894
}
8995

@@ -99,6 +105,8 @@ public function handle()
99105
$this->server->update(['unreachable_count' => 0]);
100106
}
101107

108+
$this->dispatchReachabilityChangedIfNeeded($wasReachable, $wasNotified, true);
109+
102110
} catch (\Throwable $e) {
103111

104112
Log::error('ServerConnectionCheckJob failed', [
@@ -111,24 +119,50 @@ public function handle()
111119
]);
112120
$this->server->increment('unreachable_count');
113121

122+
$this->dispatchReachabilityChangedIfNeeded($wasReachable, $wasNotified, false);
123+
114124
return;
115125
}
116126
}
117127

118128
public function failed(?\Throwable $exception): void
119129
{
120130
if ($exception instanceof TimeoutExceededException) {
131+
$wasReachable = (bool) $this->server->settings->is_reachable;
132+
$wasNotified = (bool) $this->server->unreachable_notification_sent;
133+
121134
$this->server->settings->update([
122135
'is_reachable' => false,
123136
'is_usable' => false,
124137
]);
125138
$this->server->increment('unreachable_count');
126139

140+
$this->dispatchReachabilityChangedIfNeeded($wasReachable, $wasNotified, false);
141+
127142
// Delete the queue job so it doesn't appear in Horizon's failed list.
128143
$this->job?->delete();
129144
}
130145
}
131146

147+
/**
148+
* Fire ServerReachabilityChanged when state crosses the unreachable threshold (count >= 2)
149+
* or when a previously-notified server recovers. Skips noise from single transient flaps.
150+
*/
151+
private function dispatchReachabilityChangedIfNeeded(bool $wasReachable, bool $wasNotified, bool $isReachable): void
152+
{
153+
if ($isReachable) {
154+
if (! $wasReachable || $wasNotified) {
155+
ServerReachabilityChanged::dispatch($this->server);
156+
}
157+
158+
return;
159+
}
160+
161+
if ($this->server->unreachable_count >= 2 && ! $wasNotified) {
162+
ServerReachabilityChanged::dispatch($this->server);
163+
}
164+
}
165+
132166
private function checkHetznerStatus(): void
133167
{
134168
$status = null;

‎app/Models/Server.php‎

Lines changed: 2 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -1236,39 +1236,17 @@ public function isReachableChanged()
12361236
$this->refresh();
12371237
$unreachableNotificationSent = (bool) $this->unreachable_notification_sent;
12381238
$isReachable = (bool) $this->settings->is_reachable;
1239-
if ($isReachable === true) {
1240-
$this->unreachable_count = 0;
1241-
$this->save();
12421239

1240+
if ($isReachable === true) {
12431241
if ($unreachableNotificationSent === true) {
12441242
$this->sendReachableNotification();
12451243
}
12461244

12471245
return;
12481246
}
12491247

1250-
$this->increment('unreachable_count');
1251-
1252-
if ($this->unreachable_count === 1) {
1253-
$this->settings->is_reachable = true;
1254-
$this->settings->save();
1255-
1256-
return;
1257-
}
1258-
12591248
if ($this->unreachable_count >= 2 && ! $unreachableNotificationSent) {
1260-
$failedChecks = 0;
1261-
for ($i = 0; $i < 3; $i++) {
1262-
$status = $this->serverStatus();
1263-
sleep(5);
1264-
if (! $status) {
1265-
$failedChecks++;
1266-
}
1267-
}
1268-
1269-
if ($failedChecks === 3 && ! $unreachableNotificationSent) {
1270-
$this->sendUnreachableNotification();
1271-
}
1249+
$this->sendUnreachableNotification();
12721250
}
12731251
}
12741252

‎tests/Feature/ServerReachabilityNotificationTest.php‎

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,105 @@
1+
<?php
2+
3+
use App\Events\ServerReachabilityChanged;
4+
use App\Models\Server;
5+
use App\Models\Team;
6+
use App\Notifications\Channels\EmailChannel;
7+
use App\Notifications\Server\Reachable;
8+
use App\Notifications\Server\Unreachable;
9+
use Illuminate\Foundation\Testing\RefreshDatabase;
10+
use Illuminate\Support\Facades\Notification;
11+
12+
uses(RefreshDatabase::class);
13+
14+
beforeEach(function () {
15+
$this->team = Team::factory()->create();
16+
$this->team->emailNotificationSettings()->update([
17+
'use_instance_email_settings' => true,
18+
'server_unreachable_email_notifications' => true,
19+
'server_reachable_email_notifications' => true,
20+
]);
21+
22+
$this->server = Server::factory()->create(['team_id' => $this->team->id]);
23+
24+
Notification::fake();
25+
});
26+
27+
it('sends Unreachable notification when threshold reached and not yet notified', function () {
28+
$this->server->settings()->update(['is_reachable' => false]);
29+
$this->server->forceFill([
30+
'unreachable_count' => 2,
31+
'unreachable_notification_sent' => false,
32+
])->save();
33+
34+
ServerReachabilityChanged::dispatch($this->server->fresh());
35+
36+
Notification::assertSentTo($this->team, Unreachable::class);
37+
expect($this->server->fresh()->unreachable_notification_sent)->toBeTrue();
38+
});
39+
40+
it('does not send Unreachable on first transient failure (count=1)', function () {
41+
$this->server->settings()->update(['is_reachable' => false]);
42+
$this->server->forceFill([
43+
'unreachable_count' => 1,
44+
'unreachable_notification_sent' => false,
45+
])->save();
46+
47+
ServerReachabilityChanged::dispatch($this->server->fresh());
48+
49+
Notification::assertNothingSent();
50+
});
51+
52+
it('does not send Unreachable when already notified', function () {
53+
$this->server->settings()->update(['is_reachable' => false]);
54+
$this->server->forceFill([
55+
'unreachable_count' => 5,
56+
'unreachable_notification_sent' => true,
57+
])->save();
58+
59+
ServerReachabilityChanged::dispatch($this->server->fresh());
60+
61+
Notification::assertNothingSent();
62+
});
63+
64+
it('sends Reachable notification on recovery when previously notified', function () {
65+
$this->server->settings()->update(['is_reachable' => true]);
66+
$this->server->forceFill([
67+
'unreachable_count' => 0,
68+
'unreachable_notification_sent' => true,
69+
])->save();
70+
71+
$fresh = $this->server->fresh();
72+
expect($fresh->unreachable_notification_sent)->toBeTrue();
73+
expect((bool) $fresh->settings->is_reachable)->toBeTrue();
74+
75+
ServerReachabilityChanged::dispatch($fresh);
76+
77+
Notification::assertSentTo($this->team, Reachable::class);
78+
expect($this->server->fresh()->unreachable_notification_sent)->toBeFalse();
79+
});
80+
81+
it('does not send Reachable when never notified', function () {
82+
$this->server->settings()->update(['is_reachable' => true]);
83+
$this->server->forceFill([
84+
'unreachable_count' => 0,
85+
'unreachable_notification_sent' => false,
86+
])->save();
87+
88+
ServerReachabilityChanged::dispatch($this->server->fresh());
89+
90+
Notification::assertNothingSent();
91+
});
92+
93+
it('routes Unreachable notification through EmailChannel when email toggle is on', function () {
94+
$this->server->settings()->update(['is_reachable' => false]);
95+
$this->server->forceFill([
96+
'unreachable_count' => 2,
97+
'unreachable_notification_sent' => false,
98+
])->save();
99+
100+
ServerReachabilityChanged::dispatch($this->server->fresh());
101+
102+
Notification::assertSentTo($this->team, Unreachable::class, function ($notification, $channels) {
103+
return in_array(EmailChannel::class, $channels);
104+
});
105+
});

‎tests/Unit/IsReachableChangedTest.php‎

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
<?php
2+
3+
use App\Models\Server;
4+
5+
afterEach(function () {
6+
Mockery::close();
7+
});
8+
9+
function makeServerForReachabilityTest(bool $isReachable, bool $notificationSent, int $unreachableCount): Server
10+
{
11+
$settings = Mockery::mock();
12+
$settings->is_reachable = $isReachable;
13+
14+
$server = Mockery::mock(Server::class)->makePartial()->shouldAllowMockingProtectedMethods();
15+
$server->shouldReceive('refresh')->andReturnSelf();
16+
$server->shouldReceive('getAttribute')->with('settings')->andReturn($settings);
17+
$server->shouldReceive('getAttribute')->with('unreachable_notification_sent')->andReturn($notificationSent);
18+
$server->shouldReceive('getAttribute')->with('unreachable_count')->andReturn($unreachableCount);
19+
20+
return $server;
21+
}
22+
23+
it('sends Reachable notification when reachable and notification was previously sent', function () {
24+
$server = makeServerForReachabilityTest(isReachable: true, notificationSent: true, unreachableCount: 0);
25+
$server->shouldReceive('sendReachableNotification')->once();
26+
$server->shouldNotReceive('sendUnreachableNotification');
27+
28+
$server->isReachableChanged();
29+
});
30+
31+
it('does not send any notification when reachable and notification was never sent', function () {
32+
$server = makeServerForReachabilityTest(isReachable: true, notificationSent: false, unreachableCount: 0);
33+
$server->shouldNotReceive('sendReachableNotification');
34+
$server->shouldNotReceive('sendUnreachableNotification');
35+
36+
$server->isReachableChanged();
37+
});
38+
39+
it('sends Unreachable notification when count >= 2 and not yet notified', function () {
40+
$server = makeServerForReachabilityTest(isReachable: false, notificationSent: false, unreachableCount: 2);
41+
$server->shouldReceive('sendUnreachableNotification')->once();
42+
$server->shouldNotReceive('sendReachableNotification');
43+
44+
$server->isReachableChanged();
45+
});
46+
47+
it('does not send Unreachable notification on first transient failure (count=1)', function () {
48+
$server = makeServerForReachabilityTest(isReachable: false, notificationSent: false, unreachableCount: 1);
49+
$server->shouldNotReceive('sendUnreachableNotification');
50+
$server->shouldNotReceive('sendReachableNotification');
51+
52+
$server->isReachableChanged();
53+
});
54+
55+
it('does not double-send Unreachable when already notified', function () {
56+
$server = makeServerForReachabilityTest(isReachable: false, notificationSent: true, unreachableCount: 5);
57+
$server->shouldNotReceive('sendUnreachableNotification');
58+
$server->shouldNotReceive('sendReachableNotification');
59+
60+
$server->isReachableChanged();
61+
});

‎tests/Unit/ServerBackoffTest.php‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,13 @@
11
<?php
22

3+
use App\Events\ServerReachabilityChanged;
34
use App\Jobs\ServerCheckJob;
45
use App\Jobs\ServerConnectionCheckJob;
56
use App\Jobs\ServerManagerJob;
67
use App\Models\Server;
78
use Illuminate\Queue\TimeoutExceededException;
89
use Illuminate\Support\Carbon;
10+
use Illuminate\Support\Facades\Event;
911
use Tests\TestCase;
1012

1113
uses(TestCase::class);
@@ -126,16 +128,21 @@
126128

127129
describe('ServerConnectionCheckJob unreachable_count', function () {
128130
it('increments unreachable_count on timeout', function () {
131+
Event::fake([ServerReachabilityChanged::class]);
132+
129133
$settings = Mockery::mock();
134+
$settings->is_reachable = true;
130135
$settings->shouldReceive('update')
131136
->with(['is_reachable' => false, 'is_usable' => false])
132137
->once();
133138

134139
$server = Mockery::mock(Server::class)->makePartial()->shouldAllowMockingProtectedMethods();
135140
$server->shouldReceive('getAttribute')->with('settings')->andReturn($settings);
141+
$server->shouldReceive('getAttribute')->with('unreachable_notification_sent')->andReturn(false);
136142
$server->shouldReceive('increment')->with('unreachable_count')->once();
137143
$server->id = 1;
138144
$server->name = 'test-server';
145+
$server->unreachable_count = 1; // Will become 2 after increment in real code; mock keeps value as-is
139146

140147
$job = new ServerConnectionCheckJob($server);
141148
$job->failed(new TimeoutExceededException);
@@ -152,6 +159,50 @@
152159
});
153160
});
154161

162+
describe('ServerConnectionCheckJob ServerReachabilityChanged dispatch', function () {
163+
// ServerReachabilityChanged's constructor calls $server->isReachableChanged() — verifying that
164+
// call is a clean proxy for "the event was dispatched", and avoids serializing a Mockery proxy
165+
// through the event dispatcher (which trips Eloquent static method lookups on the proxy class).
166+
$invoke = function (bool $wasReachable, bool $wasNotified, bool $isReachable, int $unreachableCount, bool $expectDispatch) {
167+
$server = Mockery::mock(Server::class)->makePartial()->shouldAllowMockingProtectedMethods();
168+
$server->shouldReceive('getAttribute')->with('unreachable_count')->andReturn($unreachableCount);
169+
$server->shouldReceive('getAttribute')->with('id')->andReturn(1);
170+
if ($expectDispatch) {
171+
$server->shouldReceive('isReachableChanged')->once()->andReturnNull();
172+
} else {
173+
$server->shouldNotReceive('isReachableChanged');
174+
}
175+
176+
$job = new ServerConnectionCheckJob($server);
177+
$method = new ReflectionMethod($job, 'dispatchReachabilityChangedIfNeeded');
178+
$method->invoke($job, $wasReachable, $wasNotified, $isReachable);
179+
};
180+
181+
it('dispatches event when count crosses unreachable threshold', function () use ($invoke) {
182+
$invoke(true, false, false, 2, true);
183+
});
184+
185+
it('does not dispatch on first transient failure (count=1)', function () use ($invoke) {
186+
$invoke(true, false, false, 1, false);
187+
});
188+
189+
it('does not dispatch when already notified and still unreachable', function () use ($invoke) {
190+
$invoke(false, true, false, 5, false);
191+
});
192+
193+
it('dispatches recovery event when previously unreachable', function () use ($invoke) {
194+
$invoke(false, false, true, 0, true);
195+
});
196+
197+
it('dispatches recovery event when previously notified', function () use ($invoke) {
198+
$invoke(true, true, true, 0, true);
199+
});
200+
201+
it('does not dispatch when consistently reachable and never notified', function () use ($invoke) {
202+
$invoke(true, false, true, 0, false);
203+
});
204+
});
205+
155206
describe('ServerCheckJob unreachable_count', function () {
156207
it('increments unreachable_count on timeout', function () {
157208
$server = Mockery::mock(Server::class)->makePartial()->shouldAllowMockingProtectedMethods();

0 commit comments

Comments
 (0)