Report unbounded node load as unmeasured - #1445
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughFleet heartbeat load is now optional. Zero-capacity nodes omit ChangesFleet heartbeat load reporting
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/broker/src/fleet_wire.rs (2)
171-179: 🗄️ Data Integrity & Integration | 🔵 TrivialKeep the Relaycast rollout prerequisite in the release gate.
This change omits
loadwhenmax_agents == 0. Release the broker only after Relaycast PR#307is deployed and accepts omitted ornullload values. Otherwise, unbounded-node heartbeats can fail at the downstream wire boundary.The PR objective states that Relaycast PR
#307must be deployed first.🤖 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 `@crates/broker/src/fleet_wire.rs` around lines 171 - 179, Preserve the Relaycast rollout prerequisite in the release gate: do not release this broker change until Relaycast PR `#307` is deployed and its downstream wire handling accepts omitted or null load values. Ensure the release checklist or gating configuration explicitly records this dependency alongside the fleet-wire load change.
840-861: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd broker-side decode coverage for unreported load.
node_heartbeat_omits_unreported_loadchecks only serialization fromload: None. Add cases that deserialize a heartbeat with noloadfield and with"load": null. The SDK test inpackages/sdk/src/__tests__/messaging.test.tsLine 488 covers null normalization, but this Rust wire test does not protect the broker-side deserialization contract.🤖 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 `@crates/broker/src/fleet_wire.rs` around lines 840 - 861, Extend node_heartbeat_omits_unreported_load to deserialize heartbeat payloads both without a load field and with "load": null, asserting each produces load: None while preserving the existing active_agents and max_agents expectations. Use the existing BrokerToRelaycast/NodeHeartbeat serde path and keep the current serialization assertions intact.
🤖 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 `@crates/broker/src/fleet_wire.rs`:
- Around line 171-179: Preserve the Relaycast rollout prerequisite in the
release gate: do not release this broker change until Relaycast PR `#307` is
deployed and its downstream wire handling accepts omitted or null load values.
Ensure the release checklist or gating configuration explicitly records this
dependency alongside the fleet-wire load change.
- Around line 840-861: Extend node_heartbeat_omits_unreported_load to
deserialize heartbeat payloads both without a load field and with "load": null,
asserting each produces load: None while preserving the existing active_agents
and max_agents expectations. Use the existing BrokerToRelaycast/NodeHeartbeat
serde path and keep the current serialization assertions intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 990e3ed5-ee0b-4e12-bf2f-448a96c7e0d8
📒 Files selected for processing (4)
CHANGELOG.mdcrates/broker/src/fleet_wire.rscrates/broker/src/node_control.rspackages/sdk/src/__tests__/messaging.test.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
…ploy-order dependency Extends node_heartbeat_omits_unreported_load with round-trip decode assertions for a heartbeat with load absent entirely and one with an explicit `"load": null`, both of which must decode to `load: None`. Previously only the serialize-side (Rust -> JSON) was covered. Also records the Relaycast #307 deploy-order dependency inline in the NodeHeartbeat field doc, not just the PR description, so it survives merge. Addresses CodeRabbit nitpicks on PR #1445.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/broker/src/fleet_wire.rs (1)
744-762: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd tests for the inclusive load boundaries.
The test rejects
1.1, but it does not prove that0.0and1.0are accepted. Add successful encode/decode cases for both boundary values.🤖 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 `@crates/broker/src/fleet_wire.rs` around lines 744 - 762, Add successful encode/decode coverage in the heartbeat validation tests around the existing over-capacity case for load values 0.0 and 1.0. Use valid heartbeat payloads and assert both boundary values deserialize successfully through BrokerToRelaycast, while preserving the existing rejection assertions.
🤖 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 `@crates/broker/src/fleet_wire.rs`:
- Around line 744-762: Add successful encode/decode coverage in the heartbeat
validation tests around the existing over-capacity case for load values 0.0 and
1.0. Use valid heartbeat payloads and assert both boundary values deserialize
successfully through BrokerToRelaycast, while preserving the existing rejection
assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9d29d1f8-22ef-4d49-9d25-1e0baf4cc5d9
📒 Files selected for processing (1)
crates/broker/src/fleet_wire.rs
node_heartbeat_rejects_invalid_loads only proved 1.1 and -0.1 are rejected; it never proved the inclusive boundaries 0.0 and 1.0 validate and decode successfully. Adds both to the existing test. Addresses a CodeRabbit nitpick from the PR #1445 re-review.
|
Addressed CodeRabbit's nitpicks from both review passes:
Validation for all three: Cubic reported no issues across both passes. No unresolved review feedback remains. Leaving open for human review/merge per policy. |
2 participants
What changed
loadwhenmax_agents == 0instead of emitting a false0active_agents / max_agents, clamped and validated in[0,1]load: nullinto an absent optional field, soagent-relay fleet nodescannot render unknown utilization as zeroRoot cause
The broker already measured
active_agentsfrom the live worker map and read an optional finite capacity fromAGENT_RELAY_NODE_MAX_AGENTS. Runtime state intentionally normalized an absent capacity tomax_agents: 0(unlimited), butFleetLoadSnapshot::heartbeatthen substituted0.0for the undefined ratio. That made “not measured” indistinguishable from “measured idle” all the way through the fleet roster.loadnow means only managed-agent capacity utilization. It is numeric in[0,1]when a finite positive maximum exists and absent for an unlimited node. It does not represent CPU, memory, queue depth, or dispatch latency.Compatibility and deployment order
Companion Relaycast PR: AgentWorkforce/relaycast#307
Relaycast must land and deploy first so the server accepts omitted/null load and persists it honestly. This PR must not be deployed independently before that compatibility layer is live.
Validation
No deployment or merge is included.