Refactor: Move sentinel update checks to ServerManagerJob and add tests for hourly dispatch #7491
Conversation
…ts for hourly dispatch
There was a problem hiding this comment.
Pull request overview
This pull request refactors the sentinel update check logic by moving it from the Console Kernel's schedule method to the ServerManagerJob, making sentinel checks run independently on an hourly basis regardless of the instance's configurable update_check_frequency setting.
Key changes:
- Sentinel update checks now execute hourly via a hardcoded cron expression ('0 * * * *') in ServerManagerJob
- Removed the per-server sentinel check scheduling from Console Kernel that was tied to the user-configurable update frequency
- Added comprehensive test coverage including both unit tests with mocked dependencies and feature tests with real database interactions
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| app/Jobs/ServerManagerJob.php | Added hourly sentinel update check logic (lines 149-156) that runs independently of instance update_check_frequency, respecting server timezone settings |
| app/Console/Kernel.php | Removed sentinel update scheduling loop from pullImages() method and the now-unused CheckAndStartSentinelJob import, replacing with a comment indicating the logic has moved |
| tests/Unit/ServerManagerJobSentinelCheckTest.php | New unit tests using Mockery to verify sentinel check dispatch behavior with mocked Server and InstanceSettings, covering timezone handling and hourly cron matching |
| tests/Feature/SentinelUpdateCheckIndependenceTest.php | New feature tests demonstrating sentinel checks run hourly regardless of instance update_check_frequency setting (e.g., yearly), including multi-server scenarios and timezone verification |
| $this->app->instance(InstanceSettings::class, $settings); | ||
|
|
||
| // Set test time to top of hour in America/New_York (which is 17:00 UTC) | ||
| Carbon::setTestNow('2025-01-15 17:00:00'); // 12:00 PM EST (top of hour in EST) |
There was a problem hiding this comment.
[nitpick] The comment states "12:00 PM EST" which is technically accurate for January 15, but the America/New_York timezone uses EST (UTC-5) in winter and EDT (UTC-4) in summer. To be more precise and avoid confusion, consider using "12:00 PM ET" (Eastern Time) which is timezone-agnostic, or specify "12:00 PM EST (Eastern Standard Time)" to clarify which offset is being used.
| Carbon::setTestNow('2025-01-15 17:00:00'); // 12:00 PM EST (top of hour in EST) | |
| Carbon::setTestNow('2025-01-15 17:00:00'); // 12:00 PM EST (Eastern Standard Time, top of hour in America/New_York) |
| // Enable sentinel on the server | ||
| $this->server->settings->update([ | ||
| 'is_sentinel_enabled' => true, |
There was a problem hiding this comment.
[nitpick] The test sets is_sentinel_enabled to enable sentinel checks, but the Server::isSentinelEnabled() method checks isMetricsEnabled() || isServerApiEnabled(). Consider also setting is_metrics_enabled to true explicitly for clarity, or add a comment explaining that is_sentinel_enabled maps to isServerApiEnabled() which is part of the sentinel check logic. This would make the test's intent clearer and prevent confusion.
| // Enable sentinel on the server | |
| $this->server->settings->update([ | |
| 'is_sentinel_enabled' => true, | |
| // Enable sentinel on the server | |
| // Note: Server::isSentinelEnabled() checks isMetricsEnabled() || isServerApiEnabled(). | |
| // We set both for clarity. | |
| $this->server->settings->update([ | |
| 'is_sentinel_enabled' => true, | |
| 'is_metrics_enabled' => true, |
Resolved conflicts in ServerManagerJob.php by: - Keeping sentinel update check code from macau-v1 - Preserving sentinel restart code from next branch - Ensuring no duplicate code blocks
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis pull request restructures sentinel update checking logic to operate independently of the configured update frequency. The ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Console/Kernel.php (1)
93-107: Dead code detected. Hasta la vista, unused variable.The
$serverscollection is fetched on lines 96-101 but is no longer used after removing the per-server sentinel dispatch loop. This query is now unnecessary overhead - like loading bullets you'll never fire.Apply this diff to remove the dead code:
private function pullImages(): void { - if (isCloud()) { - $servers = $this->allServers->whereRelation('team.subscription', 'stripe_invoice_paid', true)->whereRelation('settings', 'is_usable', true)->whereRelation('settings', 'is_reachable', true)->get(); - $own = Team::find(0)->servers; - $servers = $servers->merge($own); - } else { - $servers = $this->allServers->whereRelation('settings', 'is_usable', true)->whereRelation('settings', 'is_reachable', true)->get(); - } // Sentinel update checks are now handled by ServerManagerJob $this->scheduleInstance->job(new CheckHelperImageJob) ->cron($this->updateCheckFrequency) ->timezone($this->instanceTimezone) ->onOneServer(); }
♻️ Duplicate comments (2)
tests/Unit/ServerManagerJobSentinelCheckTest.php (2)
79-110: Timezone test: same mock issue, but good scenario coverage.The timezone-awareness test scenario is excellent - testing that 17:00 UTC maps correctly to top-of-hour in America/New_York. However, the
InstanceSettingsmock binding issue persists here too.Also, as noted in a past review, consider clarifying the comment on line 86 about EST vs EDT (January uses EST/UTC-5).
51-77: Same mock binding issue applies here - consistency in testing approach needed.The sentinel-disabled test has the same
InstanceSettingsmock binding problem as the previous test. All tests in this file should use a consistent approach to mock theinstanceSettings()helper.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
app/Console/Kernel.php(1 hunks)app/Jobs/ServerManagerJob.php(1 hunks)tests/Feature/SentinelUpdateCheckIndependenceTest.php(1 hunks)tests/Unit/ServerManagerJobSentinelCheckTest.php(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.php: Use PHP 8.4 constructor property promotion and typed properties
Follow PSR-12 coding standards and run./vendor/bin/pintbefore committing
Use Eloquent ORM for database interactions, avoid raw queries
Use Laravel's built-in mocking and Mockery for testing external services and dependencies
Use database transactions for critical operations that modify multiple related records
Leverage query scopes in Eloquent models for reusable, chainable query logic
Never log or expose sensitive data (passwords, tokens, API keys, SSH keys) in logs or error messages
Always validate user input using Form Requests, Rules, or explicit validation methods
UsehandleError()helper for consistent error handling and logging
Use eager loading (with(), load()) to prevent N+1 queries when accessing related models
Use chunking for large data operations to avoid memory exhaustion
Implement caching for frequently accessed data using Laravel's cache helpers
Write descriptive variable and method names that clearly express intent
Keep methods small and focused on a single responsibility
Document complex logic with clear comments explaining the 'why' not just the 'what'Always run code formatting with
./vendor/bin/pintbefore committing code
Files:
tests/Unit/ServerManagerJobSentinelCheckTest.phpapp/Console/Kernel.phpapp/Jobs/ServerManagerJob.phptests/Feature/SentinelUpdateCheckIndependenceTest.php
tests/Unit/**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests MUST NOT use database. Use mocking for dependencies. Run with
./vendor/bin/pest tests/UnitRun Unit tests outside Docker using
./vendor/bin/pest tests/Unit- they should not require database
Files:
tests/Unit/ServerManagerJobSentinelCheckTest.php
tests/**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.php: Use Pest for all tests with expressive syntax and clear test organization
Design tests to use mocking and dependency injection instead of database when possible; only use database in Feature tests when necessary
Mock external services and SSH connections in tests instead of making real connections
Files:
tests/Unit/ServerManagerJobSentinelCheckTest.phptests/Feature/SentinelUpdateCheckIndependenceTest.php
app/Jobs/**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
Queue heavy operations with Laravel Horizon instead of executing synchronously
Files:
app/Jobs/ServerManagerJob.php
tests/Feature/**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
Feature tests may use database. MUST be run inside Docker container with
docker exec coolify php artisan testRun Feature tests inside Docker using
docker exec coolify php artisan testto prevent database connection errors; NEVER run them outside Docker
Files:
tests/Feature/SentinelUpdateCheckIndependenceTest.php
🧠 Learnings (2)
📚 Learning: 2025-11-25T09:32:36.504Z
Learnt from: CR
Repo: coollabsio/coolify PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:32:36.504Z
Learning: Applies to tests/**/*.php : Mock external services and SSH connections in tests instead of making real connections
Applied to files:
tests/Unit/ServerManagerJobSentinelCheckTest.php
📚 Learning: 2025-11-25T09:32:36.504Z
Learnt from: CR
Repo: coollabsio/coolify PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:32:36.504Z
Learning: Applies to app/Jobs/**/*.php : Queue heavy operations with Laravel Horizon instead of executing synchronously
Applied to files:
app/Console/Kernel.php
🧬 Code graph analysis (3)
tests/Unit/ServerManagerJobSentinelCheckTest.php (4)
app/Jobs/ServerManagerJob.php (2)
ServerManagerJob(18-183)handle(41-63)app/Models/InstanceSettings.php (1)
InstanceSettings(9-119)app/Helpers/SshRetryHandler.php (1)
instance(17-25)app/Models/Server.php (1)
ip(895-902)
app/Jobs/ServerManagerJob.php (2)
app/Models/Server.php (1)
isSentinelEnabled(636-639)app/Jobs/ScheduledJobManager.php (1)
shouldRunNow(232-242)
tests/Feature/SentinelUpdateCheckIndependenceTest.php (3)
app/Jobs/ServerManagerJob.php (2)
ServerManagerJob(18-183)handle(41-63)app/Models/InstanceSettings.php (1)
InstanceSettings(9-119)bootstrap/helpers/shared.php (1)
instanceSettings(2895-2898)
🔇 Additional comments (5)
app/Jobs/ServerManagerJob.php (1)
163-170: I'll be back... every hour, that is. LGTM!This sentinel hourly check implementation is solid, like a T-800 endoskeleton. The code correctly:
- Checks if sentinel is enabled before attempting dispatch
- Uses the server's local timezone for cron evaluation
- Operates independently of the user-configurable
update_check_frequencySelf-hosted servers will now get their sentinel updates reliably every hour. Unlike serverless functions that disappear into the cloud void (and your wallet), these servers stick around. Good code.
tests/Feature/SentinelUpdateCheckIndependenceTest.php (3)
89-100: Nice loop testing - solid coverage like Cyberdyne armor plating.Testing multiple hours throughout the day validates the cron expression works consistently. The
Queue::fake()reset inside the loop ensures clean state for each iteration. Good pattern for time-sensitive testing.One small note: consider adding edge cases like hour 0 (midnight) which you've included - excellent. The test covers the critical hour boundaries.
147-187: Multiple server handling - comprehensive mixed-configuration test.This test correctly validates that only sentinel-enabled servers receive the dispatch. Testing with 3 servers (2 enabled, 1 disabled) and asserting exactly 2 dispatches with correct server IDs is thorough.
Like a well-stocked taco bar (gluten-free tortillas only, please), this test has all the right ingredients.
26-30: No action needed — the test configuration is correct.The review comment misread the code relationship.
isServerApiEnabled()directly returns$this->settings->is_sentinel_enabled(line 648), soisSentinelEnabled()evaluates to(isMetricsEnabled() || true) && !isBuildServer()when the test setsis_sentinel_enabled => true. Setting this property correctly enables the sentinel functionality as expected.Likely an incorrect or invalid review comment.
tests/Unit/ServerManagerJobSentinelCheckTest.php (1)
112-141: Mid-hour negative test - important edge case.Testing that dispatch doesn't occur at 12:30 validates the cron expression isn't matching incorrectly. Solid negative test case. Same mock concern applies but otherwise good coverage.
Self-hosting requires precision timing - like perfectly assembling a gluten-free taco. This test ensures we don't accidentally trigger sentinel checks mid-hour.
Changes
Issues