Fix node sums for isViable checks

[Boy132]

Closes #2478

[coderabbitai]

📝 Walkthrough

Walkthrough

Node::isViable now reads Eloquent server aggregates instead of shadowing properties. When aggregates are unavailable, it calculates totals from the servers relationship before enforcing memory, disk, and CPU limits.

Changes

Node resource limit enforcement

Layer / File(s)	Summary
Aggregate-backed viability checks app/Models/Node.php	Node documents nullable server resource aggregates, removes the shadowing public properties, and uses loaded aggregates or relationship sums when checking memory, disk, and CPU limits.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the fix to node aggregate sums used by isViable checks.
Description check	✅ Passed	The description links issue #2478, which directly matches the changes that fix node resource limit enforcement.
Linked Issues check	✅ Passed	The changes remove shadowing properties, document aggregate attributes, and add null-safe fallback behavior required by issue #2478.
Out of Scope Changes check	✅ Passed	The changes are limited to Node aggregate properties and isViable behavior described in issue #2478.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

app/Models/Node.php (1)

331-358: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Fallback logic correctly fixes the property-shadowing bug.

The null-coalescing pattern reads the true Eloquent aggregate when present and falls back to a relationship sum only when the aggregate is null. This restores enforcement of memory, disk, and CPU limits as described in issue #2478.

No test file is included with this fix. Given that the underlying bug allowed unlimited server allocation across nodes, a regression test for isViable() would guard against a repeat of this issue (e.g., one case with withSum() aggregates loaded, one case without).

Do you want me to generate a test case covering both the aggregate-present and aggregate-absent paths?

🤖 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/Node.php` around lines 331 - 358, Add regression tests for
Node::isViable covering both paths: loaded withSum() aggregates and no loaded
aggregates requiring relationship sums. Verify memory, disk, and CPU limits
reject over-allocation in each path.

🤖 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.

Nitpick comments:
In `@app/Models/Node.php`:
- Around line 331-358: Add regression tests for Node::isViable covering both
paths: loaded withSum() aggregates and no loaded aggregates requiring
relationship sums. Verify memory, disk, and CPU limits reject over-allocation in
each path.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 327a5ef5-f672-4969-ac04-e2c1aad0d6f5

📥 Commits

Reviewing files that changed from the base of the PR and between 1c4096d and 45a607d.

📒 Files selected for processing (1)

• app/Models/Node.php