Fix: Prevent ServerManagerJob executionTime mutation across server loop
Fixed a critical bug where $this->executionTime was being mutated during the server processing loop, causing incorrect scheduling calculations for subsequent servers. The issue occurred at line 123 where subSeconds() was called directly on the shared executionTime instance. This caused the baseline time to shift by waitTime seconds with each server iteration, resulting in compounding scheduling errors (e.g., 1680 seconds drift over 5 servers). Changed: - app/Jobs/ServerManagerJob.php:123 Added .copy() before .subSeconds() to prevent mutation Added comprehensive unit tests that verify: - Immutability when using .copy() - Demonstration of the bug without .copy() - Correct behavior across multiple iterations This follows the existing pattern in shouldRunNow() (line 167) and aligns with other jobs in the codebase. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
parent
b47181c790
commit
ed5796739f
2 changed files with 98 additions and 1 deletions
|
|
@ -120,7 +120,7 @@ private function processServerTasks(Server $server): void
|
|||
// Check if we should run sentinel-based checks
|
||||
$lastSentinelUpdate = $server->sentinel_updated_at;
|
||||
$waitTime = $server->waitBeforeDoingSshCheck();
|
||||
$sentinelOutOfSync = Carbon::parse($lastSentinelUpdate)->isBefore($this->executionTime->subSeconds($waitTime));
|
||||
$sentinelOutOfSync = Carbon::parse($lastSentinelUpdate)->isBefore($this->executionTime->copy()->subSeconds($waitTime));
|
||||
|
||||
if ($sentinelOutOfSync) {
|
||||
// Dispatch ServerCheckJob if Sentinel is out of sync
|
||||
|
|
|
|||
97
tests/Unit/ServerManagerJobExecutionTimeTest.php
Normal file
97
tests/Unit/ServerManagerJobExecutionTimeTest.php
Normal file
|
|
@ -0,0 +1,97 @@
|
|||
<?php
|
||||
|
||||
use Illuminate\Support\Carbon;
|
||||
|
||||
afterEach(function () {
|
||||
Carbon::setTestNow();
|
||||
});
|
||||
|
||||
it('does not mutate Carbon instance when using copy() before subSeconds()', function () {
|
||||
// This test verifies the fix for the bug where subSeconds() was mutating executionTime
|
||||
$originalTime = Carbon::parse('2024-12-02 12:00:00');
|
||||
$originalTimeString = $originalTime->toDateTimeString();
|
||||
|
||||
// Simulate what happens in processServerTasks() with the FIX applied
|
||||
$waitTime = 360;
|
||||
$threshold = $originalTime->copy()->subSeconds($waitTime);
|
||||
|
||||
// The original time should remain unchanged after using copy()
|
||||
expect($originalTime->toDateTimeString())->toBe($originalTimeString);
|
||||
expect($threshold->toDateTimeString())->toBe('2024-12-02 11:54:00');
|
||||
});
|
||||
|
||||
it('demonstrates mutation bug when not using copy()', function () {
|
||||
// This test shows what would happen WITHOUT the fix (the bug)
|
||||
$originalTime = Carbon::parse('2024-12-02 12:00:00');
|
||||
|
||||
// Simulate what would happen WITHOUT copy() (the bug)
|
||||
$waitTime = 360;
|
||||
$threshold = $originalTime->subSeconds($waitTime);
|
||||
|
||||
// Without copy(), the original time is mutated!
|
||||
expect($originalTime->toDateTimeString())->toBe('2024-12-02 11:54:00');
|
||||
expect($threshold->toDateTimeString())->toBe('2024-12-02 11:54:00');
|
||||
expect($originalTime)->toBe($threshold); // They're the same object
|
||||
});
|
||||
|
||||
it('preserves executionTime across multiple subSeconds calls with copy()', function () {
|
||||
// Simulate processing multiple servers with different wait times
|
||||
$executionTime = Carbon::parse('2024-12-02 12:00:00');
|
||||
$originalTimeString = $executionTime->toDateTimeString();
|
||||
|
||||
// Server 1: waitTime = 360s
|
||||
$threshold1 = $executionTime->copy()->subSeconds(360);
|
||||
expect($executionTime->toDateTimeString())->toBe($originalTimeString);
|
||||
expect($threshold1->toDateTimeString())->toBe('2024-12-02 11:54:00');
|
||||
|
||||
// Server 2: waitTime = 300s (should still use original time)
|
||||
$threshold2 = $executionTime->copy()->subSeconds(300);
|
||||
expect($executionTime->toDateTimeString())->toBe($originalTimeString);
|
||||
expect($threshold2->toDateTimeString())->toBe('2024-12-02 11:55:00');
|
||||
|
||||
// Server 3: waitTime = 360s (should still use original time)
|
||||
$threshold3 = $executionTime->copy()->subSeconds(360);
|
||||
expect($executionTime->toDateTimeString())->toBe($originalTimeString);
|
||||
expect($threshold3->toDateTimeString())->toBe('2024-12-02 11:54:00');
|
||||
|
||||
// Server 4: waitTime = 300s (should still use original time)
|
||||
$threshold4 = $executionTime->copy()->subSeconds(300);
|
||||
expect($executionTime->toDateTimeString())->toBe($originalTimeString);
|
||||
expect($threshold4->toDateTimeString())->toBe('2024-12-02 11:55:00');
|
||||
|
||||
// Server 5: waitTime = 360s (should still use original time)
|
||||
$threshold5 = $executionTime->copy()->subSeconds(360);
|
||||
expect($executionTime->toDateTimeString())->toBe($originalTimeString);
|
||||
expect($threshold5->toDateTimeString())->toBe('2024-12-02 11:54:00');
|
||||
|
||||
// The executionTime should STILL be exactly the original time
|
||||
expect($executionTime->toDateTimeString())->toBe('2024-12-02 12:00:00');
|
||||
});
|
||||
|
||||
it('demonstrates compounding bug without copy() across multiple calls', function () {
|
||||
// This shows the compounding bug that happens WITHOUT the fix
|
||||
$executionTime = Carbon::parse('2024-12-02 12:00:00');
|
||||
|
||||
// Server 1: waitTime = 360s
|
||||
$threshold1 = $executionTime->subSeconds(360);
|
||||
expect($executionTime->toDateTimeString())->toBe('2024-12-02 11:54:00'); // MUTATED!
|
||||
|
||||
// Server 2: waitTime = 300s (uses already-mutated time)
|
||||
$threshold2 = $executionTime->subSeconds(300);
|
||||
expect($executionTime->toDateTimeString())->toBe('2024-12-02 11:49:00'); // Further mutated!
|
||||
|
||||
// Server 3: waitTime = 360s (uses even more mutated time)
|
||||
$threshold3 = $executionTime->subSeconds(360);
|
||||
expect($executionTime->toDateTimeString())->toBe('2024-12-02 11:43:00'); // Even more mutated!
|
||||
|
||||
// Server 4: waitTime = 300s
|
||||
$threshold4 = $executionTime->subSeconds(300);
|
||||
expect($executionTime->toDateTimeString())->toBe('2024-12-02 11:38:00');
|
||||
|
||||
// Server 5: waitTime = 360s
|
||||
$threshold5 = $executionTime->subSeconds(360);
|
||||
expect($executionTime->toDateTimeString())->toBe('2024-12-02 11:32:00');
|
||||
|
||||
// The executionTime is now 1680 seconds (28 minutes) earlier than it should be!
|
||||
expect($executionTime->diffInSeconds(Carbon::parse('2024-12-02 12:00:00')))->toEqual(1680);
|
||||
});
|
||||
Loading…
Reference in a new issue