Skip daemon RPC calls for servers on maintenance-mode nodes - #2371
Conversation
- retrieveStatus(): return Missing immediately if node is under maintenance - retrieveResources(): return empty if server status is not starting/running, which includes Missing (from maintenance guard) and all stopped states
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe Server model now short-circuits daemon queries when a server's node is under maintenance: ChangesServer state guards
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Models/Server.php (1)
477-486:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid double daemon
getDetails()on cold cache inretrieveResources().At Line 477,
retrieveResources()callsretrieveStatus(), which can already hit daemongetDetails()(Line 466) on a status cache miss; then Line 483 callsgetDetails()again for utilization. This doubles RPCs for active servers and can reintroduce the dashboard latency this PR is trying to reduce.Suggested direction (single daemon fetch path)
public function retrieveResources(): array { - if (!$this->retrieveStatus()->isStartingOrRunning()) { - return []; - } - - return cache()->remember("servers.$this->uuid.resources", now()->addSeconds(15), function () { - $details = app(DaemonServerRepository::class)->setServer($this)->getDetails(); - return Arr::get($details, 'utilization', []); - }); + if ($this->node->isUnderMaintenance()) { + return []; + } + + return cache()->remember("servers.$this->uuid.resources", now()->addSeconds(15), function () { + $details = app(DaemonServerRepository::class)->setServer($this)->getDetails(); + $status = ContainerStatus::tryFrom(Arr::get($details, 'state')) ?? ContainerStatus::Missing; + + // Keep status cache in sync and avoid a second daemon call in this request path. + cache()->put("servers.$this->uuid.status", $status, now()->addSeconds(15)); + + return $status->isStartingOrRunning() ? Arr::get($details, 'utilization', []) : []; + }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Models/Server.php` around lines 477 - 486, retrieveResources() is causing two daemon RPCs on a cold cache because retrieveStatus() can already call DaemonServerRepository::getDetails(), and retrieveResources() calls getDetails() again; to fix, make retrieveStatus() store the fetched daemon details (e.g., set a transient property like $this->daemonDetails or return the details alongside the status) when it calls DaemonServerRepository::getDetails(), then update retrieveResources() to reuse that stored details if present instead of invoking DaemonServerRepository::getDetails() again; keep the existing cache key ("servers.$this->uuid.resources"), preserve the phpstan-ignore comment where needed, and ensure behavior is unchanged when details were not previously fetched.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@app/Models/Server.php`:
- Around line 477-486: retrieveResources() is causing two daemon RPCs on a cold
cache because retrieveStatus() can already call
DaemonServerRepository::getDetails(), and retrieveResources() calls getDetails()
again; to fix, make retrieveStatus() store the fetched daemon details (e.g., set
a transient property like $this->daemonDetails or return the details alongside
the status) when it calls DaemonServerRepository::getDetails(), then update
retrieveResources() to reuse that stored details if present instead of invoking
DaemonServerRepository::getDetails() again; keep the existing cache key
("servers.$this->uuid.resources"), preserve the phpstan-ignore comment where
needed, and ensure behavior is unchanged when details were not previously
fetched.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3f9ed00f-21ff-4cc6-a288-17e10a544a57
📒 Files selected for processing (1)
app/Models/Server.php
|
I don't know if this is desirable. When a node is under maintenance this would restrict what admins can do with servers. |
As far as this do, this will only disrupt report of statistics and status. For actual actions block, it would be introduced in |
Boy132
left a comment
Boy132
left a comment
There was a problem hiding this comment.
You're right. This is probably fine.
Just one small nitpick.
| */ | ||
| public function retrieveResources(): array | ||
| { | ||
| if (!$this->retrieveStatus()->isStartingOrRunning()) { |
There was a problem hiding this comment.
Resources should still be retrieved when stopping.
…aintenance check instead of container state check
95f735a to
0631073
Compare
|
My fault, I rebased main onto this branch, should be fine now. |
4 participants
Discussion: Node disruption causes UX delay
Purpose: Practical way to handle Wings node down, yet. Due to the nature of "Maintenance mode" option, runtime consequences must be known by operator, which should make it useful. This option will disengage most of the query process from the dashboard to the node in order to let the Wings instance in a safe state.
What changed:
retrieveStatus(): return Missing immediately if node is under maintenanceretrieveResources(): return empty if server status is not starting/running, which includes Missing (from maintenance guard) and all stopped states