fix: preserve unknown health status in Sentinel updates (PushServerUpdateJob)
## Problem Services with "running (unknown)" status were periodically changing to "running (healthy)" every ~30 seconds when Sentinel pushed updates. This was confusing for users and inconsistent with SSH-based status checks. ## Root Cause `PushServerUpdateJob::aggregateMultiContainerStatuses()` was missing logic to track "unknown" health state. It only tracked "unhealthy" and defaulted everything else to "healthy". When Sentinel pushed updates with "running (unknown)" containers: - The job saw `hasRunning = true` and `hasUnhealthy = false` - It incorrectly returned "running (healthy)" instead of "running (unknown)" ## Solution Updated `PushServerUpdateJob` to match the logic in `GetContainersStatus`: 1. Added `$hasUnknown` tracking variable 2. Check for "unknown" in status strings (alongside "unhealthy") 3. Implement 3-way priority: unhealthy > unknown > healthy This ensures consistency between: - SSH-based updates (`GetContainersStatus`) - Sentinel-based updates (`PushServerUpdateJob`) - UI display logic ## Changes - **app/Jobs/PushServerUpdateJob.php**: Added unknown status tracking - **tests/Unit/PushServerUpdateJobStatusAggregationTest.php**: New comprehensive tests - **tests/Unit/ExcludeFromHealthCheckTest.php**: Updated to match current implementation ## Testing All 31 status-related unit tests passing: - 18 tests in ContainerHealthStatusTest - 8 tests in ExcludeFromHealthCheckTest (updated) - 6 tests in PushServerUpdateJobStatusAggregationTest (new) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
parent
7f00ef2f34
commit
6b62847a11
3 changed files with 121 additions and 2 deletions
|
|
@ -269,6 +269,7 @@ private function aggregateMultiContainerStatuses()
|
||||||
// Aggregate status: if any container is running, app is running
|
// Aggregate status: if any container is running, app is running
|
||||||
$hasRunning = false;
|
$hasRunning = false;
|
||||||
$hasUnhealthy = false;
|
$hasUnhealthy = false;
|
||||||
|
$hasUnknown = false;
|
||||||
|
|
||||||
foreach ($relevantStatuses as $status) {
|
foreach ($relevantStatuses as $status) {
|
||||||
if (str($status)->contains('running')) {
|
if (str($status)->contains('running')) {
|
||||||
|
|
@ -276,12 +277,21 @@ private function aggregateMultiContainerStatuses()
|
||||||
if (str($status)->contains('unhealthy')) {
|
if (str($status)->contains('unhealthy')) {
|
||||||
$hasUnhealthy = true;
|
$hasUnhealthy = true;
|
||||||
}
|
}
|
||||||
|
if (str($status)->contains('unknown')) {
|
||||||
|
$hasUnknown = true;
|
||||||
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
$aggregatedStatus = null;
|
$aggregatedStatus = null;
|
||||||
if ($hasRunning) {
|
if ($hasRunning) {
|
||||||
$aggregatedStatus = $hasUnhealthy ? 'running (unhealthy)' : 'running (healthy)';
|
if ($hasUnhealthy) {
|
||||||
|
$aggregatedStatus = 'running (unhealthy)';
|
||||||
|
} elseif ($hasUnknown) {
|
||||||
|
$aggregatedStatus = 'running (unknown)';
|
||||||
|
} else {
|
||||||
|
$aggregatedStatus = 'running (healthy)';
|
||||||
|
}
|
||||||
} else {
|
} else {
|
||||||
// All containers are exited
|
// All containers are exited
|
||||||
$aggregatedStatus = 'exited (unhealthy)';
|
$aggregatedStatus = 'exited (unhealthy)';
|
||||||
|
|
|
||||||
|
|
@ -19,7 +19,9 @@
|
||||||
->toContain('// If all containers are excluded, calculate status from excluded containers')
|
->toContain('// If all containers are excluded, calculate status from excluded containers')
|
||||||
->toContain('// but mark it with :excluded to indicate monitoring is disabled')
|
->toContain('// but mark it with :excluded to indicate monitoring is disabled')
|
||||||
->toContain('if ($relevantContainerCount === 0) {')
|
->toContain('if ($relevantContainerCount === 0) {')
|
||||||
->toContain("return 'running:excluded';")
|
->toContain("return 'running:unhealthy:excluded';")
|
||||||
|
->toContain("return 'running:unknown:excluded';")
|
||||||
|
->toContain("return 'running:healthy:excluded';")
|
||||||
->toContain("return 'degraded:excluded';")
|
->toContain("return 'degraded:excluded';")
|
||||||
->toContain("return 'exited:excluded';");
|
->toContain("return 'exited:excluded';");
|
||||||
});
|
});
|
||||||
|
|
|
||||||
107
tests/Unit/PushServerUpdateJobStatusAggregationTest.php
Normal file
107
tests/Unit/PushServerUpdateJobStatusAggregationTest.php
Normal file
|
|
@ -0,0 +1,107 @@
|
||||||
|
<?php
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Unit tests for PushServerUpdateJob status aggregation logic.
|
||||||
|
*
|
||||||
|
* These tests verify that the job correctly aggregates container statuses
|
||||||
|
* when processing Sentinel updates, with proper handling of:
|
||||||
|
* - running (healthy) - all containers running and healthy
|
||||||
|
* - running (unhealthy) - some containers unhealthy
|
||||||
|
* - running (unknown) - some containers with unknown health status
|
||||||
|
*
|
||||||
|
* The aggregation follows a priority system: unhealthy > unknown > healthy
|
||||||
|
*
|
||||||
|
* This ensures consistency with GetContainersStatus::aggregateApplicationStatus()
|
||||||
|
* and prevents the bug where "unknown" status was incorrectly converted to "healthy".
|
||||||
|
*/
|
||||||
|
it('aggregates status with unknown health state correctly', function () {
|
||||||
|
$jobFile = file_get_contents(__DIR__.'/../../app/Jobs/PushServerUpdateJob.php');
|
||||||
|
|
||||||
|
// Verify that hasUnknown tracking variable exists
|
||||||
|
expect($jobFile)
|
||||||
|
->toContain('$hasUnknown = false;')
|
||||||
|
->toContain('if (str($status)->contains(\'unknown\')) {')
|
||||||
|
->toContain('$hasUnknown = true;');
|
||||||
|
|
||||||
|
// Verify 3-way status priority logic (unhealthy > unknown > healthy)
|
||||||
|
expect($jobFile)
|
||||||
|
->toContain('if ($hasUnhealthy) {')
|
||||||
|
->toContain('$aggregatedStatus = \'running (unhealthy)\';')
|
||||||
|
->toContain('} elseif ($hasUnknown) {')
|
||||||
|
->toContain('$aggregatedStatus = \'running (unknown)\';')
|
||||||
|
->toContain('} else {')
|
||||||
|
->toContain('$aggregatedStatus = \'running (healthy)\';');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('checks for unknown status alongside unhealthy status', function () {
|
||||||
|
$jobFile = file_get_contents(__DIR__.'/../../app/Jobs/PushServerUpdateJob.php');
|
||||||
|
|
||||||
|
// Verify unknown check is placed alongside unhealthy check
|
||||||
|
expect($jobFile)
|
||||||
|
->toContain('if (str($status)->contains(\'unhealthy\')) {')
|
||||||
|
->toContain('if (str($status)->contains(\'unknown\')) {');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('follows same priority as GetContainersStatus', function () {
|
||||||
|
$jobFile = file_get_contents(__DIR__.'/../../app/Jobs/PushServerUpdateJob.php');
|
||||||
|
$getContainersFile = file_get_contents(__DIR__.'/../../app/Actions/Docker/GetContainersStatus.php');
|
||||||
|
|
||||||
|
// Both should track hasUnknown
|
||||||
|
expect($jobFile)->toContain('$hasUnknown = false;');
|
||||||
|
expect($getContainersFile)->toContain('$hasUnknown = false;');
|
||||||
|
|
||||||
|
// Both should check for 'unknown' in status strings
|
||||||
|
expect($jobFile)->toContain('if (str($status)->contains(\'unknown\')) {');
|
||||||
|
expect($getContainersFile)->toContain('if (str($status)->contains(\'unknown\')) {');
|
||||||
|
|
||||||
|
// Both should prioritize unhealthy over unknown over healthy
|
||||||
|
expect($jobFile)->toContain('} elseif ($hasUnknown) {');
|
||||||
|
expect($getContainersFile)->toContain('} elseif ($hasUnknown) {');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('does not default unknown to healthy status', function () {
|
||||||
|
$jobFile = file_get_contents(__DIR__.'/../../app/Jobs/PushServerUpdateJob.php');
|
||||||
|
|
||||||
|
// The old buggy code was:
|
||||||
|
// $aggregatedStatus = $hasUnhealthy ? 'running (unhealthy)' : 'running (healthy)';
|
||||||
|
// This would make unknown -> healthy
|
||||||
|
|
||||||
|
// Verify we're NOT using ternary operator for status assignment
|
||||||
|
expect($jobFile)
|
||||||
|
->not->toContain('$aggregatedStatus = $hasUnhealthy ? \'running (unhealthy)\' : \'running (healthy)\';');
|
||||||
|
|
||||||
|
// Verify we ARE using if-elseif-else with proper unknown handling
|
||||||
|
expect($jobFile)
|
||||||
|
->toContain('if ($hasUnhealthy) {')
|
||||||
|
->toContain('} elseif ($hasUnknown) {')
|
||||||
|
->toContain('$aggregatedStatus = \'running (unknown)\';');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('initializes all required status tracking variables', function () {
|
||||||
|
$jobFile = file_get_contents(__DIR__.'/../../app/Jobs/PushServerUpdateJob.php');
|
||||||
|
|
||||||
|
// Verify all three tracking variables are initialized together
|
||||||
|
$pattern = '/\$hasRunning\s*=\s*false;\s*\$hasUnhealthy\s*=\s*false;\s*\$hasUnknown\s*=\s*false;/s';
|
||||||
|
|
||||||
|
expect(preg_match($pattern, $jobFile))->toBe(1,
|
||||||
|
'All status tracking variables ($hasRunning, $hasUnhealthy, $hasUnknown) should be initialized together');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('preserves unknown status through sentinel updates', function () {
|
||||||
|
$jobFile = file_get_contents(__DIR__.'/../../app/Jobs/PushServerUpdateJob.php');
|
||||||
|
|
||||||
|
// The critical path: when a status contains 'running' AND 'unknown',
|
||||||
|
// both flags should be set
|
||||||
|
expect($jobFile)
|
||||||
|
->toContain('if (str($status)->contains(\'running\')) {')
|
||||||
|
->toContain('$hasRunning = true;')
|
||||||
|
->toContain('if (str($status)->contains(\'unhealthy\')) {')
|
||||||
|
->toContain('$hasUnhealthy = true;')
|
||||||
|
->toContain('if (str($status)->contains(\'unknown\')) {')
|
||||||
|
->toContain('$hasUnknown = true;');
|
||||||
|
|
||||||
|
// And then unknown should have priority over healthy in aggregation
|
||||||
|
expect($jobFile)
|
||||||
|
->toContain('} elseif ($hasUnknown) {')
|
||||||
|
->toContain('$aggregatedStatus = \'running (unknown)\';');
|
||||||
|
});
|
||||||
Loading…
Reference in a new issue