fix(spurd): refresh node inventory and re-register when it changes

[powderluv]

Summary

Node resources were discovered once at spurd startup and never refreshed, and the 30s heartbeat carried no resource payload — so a device count that changed out of band (a GPU partition-mode switch, a GPU dropping off the bus) never reached the controller, which kept scheduling against hardware that no longer existed (issue #800). The WireGuard key is already re-read live each heartbeat; inventory had no equivalent.

Fix

A periodic (60s) re-discovery task: rebuild the device registry (re-running CDI / KFD discovery), recompute the ResourceSet, and when it differs from what was reported, swap the shared registry (so GPU injection uses the new set) and re-register with the controller so it converges on the new inventory.

• resources moves behind a RwLock; update_resources() returns whether it actually changed; register() snapshots the inventory before its await.

Testing

• Unit test: update_resources reports a real change and no-ops an unchanged set.
• build + clippy + fmt green.

Closes #800.

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

The refresh loop can permanently stop retrying re-registration after a transient register() failure, and allocation state appears to remain stale across inventory changes (risking dispatch failures on inventory increases).

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR addresses stale node inventory reporting in spurd by periodically re-discovering local resources and re-registering with spurctld when the inventory changes, so the controller stops scheduling against hardware that no longer exists.

Changes:

• Put NodeReporter.resources behind an RwLock and add update_resources() to swap the inventory only when it changes.
• Add a 60s background task in spurd to rebuild the device registry, re-discover resources, and re-register on changes.
• Update agent-side resource reads to acquire the RwLock and add a unit test for update_resources() behavior.

File summaries

File	Description
crates/spurd/src/reporter.rs	Wrap resources in RwLock, add update_resources(), snapshot inventory for registration, and add unit test.
crates/spurd/src/main.rs	Add periodic inventory rediscovery and re-registration loop; swap shared device registry on change.
crates/spurd/src/agent_server.rs	Update resource reads to use the new RwLock-protected inventory.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 76.41509% with 25 lines in your changes missing coverage. Please review.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #834      +/-   ##
==========================================
+ Coverage   80.06%   80.52%   +0.46%     
==========================================
  Files         184      186       +2     
  Lines       89592    92017    +2425     
==========================================
+ Hits        71731    74092    +2361     
- Misses      17861    17925      +64     

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[pre]

Overlap worth flagging: #809 adds a re-registration path as well, on the operator side (crates/spur-k8s/src/heartbeat.rs). The trigger there is different, the controller answering NotFound to a heartbeat, which happens after spur node remove or after a rebuild from older state. The operator keeps the RegisterAgentRequest it built at startup and replays it. This PR's trigger is a local inventory change inside spurd.

There is no file conflict, the two live in different crates, so both can merge as they are. The concern is the end state: two independent re-registration mechanisms, with different triggers, different retry behaviour and different logging, in the two agents that both register with the same controller.

If both land, it is worth consolidating them into one convergence path. One shape that covers both cases is a heartbeat that carries the inventory, so the controller notices a disagreement or a missing record and asks for a re-register, with spurd and the operator sharing that path instead of each owning its own.

[yansun1996]

Thanks — #800 is a real gap and the shape here is right. One suggestion plus a few notes.

Could the apply be gated on whether anything is allocated?

change seen	action
discovery errored	skip the tick
seen once	wait for a second identical reading
confirmed, nothing allocated	swap registry, update capacity, re-register
confirmed, something allocated	drain; don't apply, don't report new totals

Row 4 self-converges: draining stops new work, the jobs finish, the next tick takes row 3. So there's never an apply-while-running path — which removes most of the notes below instead of needing fixes for them, and collapses update_capacity to a rebuild from the fresh ResourceSet.

Why not apply under a live allocation: the controller can't reconcile it. NodeUpdate sets total_resources and stops — alloc_resources isn't pruned, so can_satisfy_with_allocated stops subtracting the vanished devices and over-reports free GPUs.

That's a missing evidence channel, not a missing calculation. The agent only sends RunningJobStatus { job_id, state, exit_code }, so the controller can only diff totals. The end state worth building toward is the agent re-collecting what each running job still holds and the controller reconciling per job — prune what's genuinely gone, keep the rest, leave healthy jobs alone. The agent already has the data (NodeAllocation.owners is job_id -> {cpu_ids, gpu_ids, memory_mb}), just no wire format for it. Until then drain is the placeholder — so it'd help to keep detection separate from policy here, e.g. refresh_once() returning a classified outcome and a small policy fn acting on it. That also makes both paths testable.

Drain and auto-recovery need no new plumbing. register_node already sees the resource diff and alloc_resources, and cluster.drain_node() returns Draining when jobs are running. Set it as a system drain (admin_locked = false) and lift it when the cause clears, the way check_node_health recovers a node from Down — an operator's own drain stays put. Idle nodes never drain; busy ones resume themselves.

Labels (out of diff). register() sends labels captured at startup. Under default permissive auth the agent is treated as privileged, so a re-register silently reverts an admin relabel; under auth.mode = required it fails PermissionDenied, and reregister_pending never clears — a warning every 60s indefinitely.

Admission (out of diff). validate_admission runs on every register_agent. Harmless under the default open mode, but on a token-mode cluster an expired join token means re-registration can never succeed. A bounded backoff would cover both this and the labels case.

Upgrade compat is clean. No proto change, no new persisted field or WAL variant; NodeUpdate preserves Drain/Down, admin_locked, agent_start_time. Raft growth is bounded — discovery ordering is deterministic. One edge: discover_memory_mb returns 0 on a /proc/meminfo read failure, which would flap and write every tick.

[yansun1996]

I'd treat this as blocking, since everything else depends on how far this signal can be trusted. init_device_registry swallows its errors — the CDI cache warns on parse failures and skips unreadable dirs, autodetect returns an empty vec — so "discovery failed" and "no GPUs" are the same value, and it's committed unconditionally. A driver reload rolling across the fleet would report every node as zero-GPU at once. It's also what makes the cons_tres.rs issues likely rather than rare.

Could discovery return a Result so the tick skips on error, and require two consecutive identical readings before acting? Also worth spawn_blocking here — both calls do synchronous sysfs/proc/CDI reads on a runtime worker, and the startup path already wraps comparable work.

[yansun1996]

Reporting a growth is always safe; reporting a shrink while a job still holds the vanished devices is what makes the controller's totals disagree with its own alloc_resources. Note the decision needn't live here — register_node already sees both the resource diff and alloc_resources, so the controller could drain instead, with no new plumbing. Longer term this is the seam where per-job evidence replaces the totals diff, so it's worth returning a classified outcome rather than deciding inline.

[yansun1996]

The 60s cadence is hardcoded while comparable timings (heartbeat_timeout_secs, agent_keepalive_interval_secs, ...) are config fields — worth an inventory_refresh_secs with #[serde(default)], plus a short docs note since inventory changing on its own is new user-visible behaviour. Minor: interval defaults to MissedTickBehavior::Burst, so delayed ticks fire back-to-back.

[yansun1996]

allocated_gpu_ids() is derived from gpus + gpu_allocated, so seeding from it loses any device not in the current list — a GPU that vanishes on one refresh and returns on the next comes back free while its job is running, and the first job's release then clears the second job's bit. Line 79 is the same shape: resize truncates allocated CPU entries on shrink, so free_cpus() over-reports. Both bite a healthy job on a busy node.

Gating on "nothing allocated" makes both impossible. Otherwise, owners is the documented source of truth:

let allocated_ids: HashSet<u32> =
    self.owners.values().flat_map(|a| a.gpu_ids.iter().copied()).collect();

[yansun1996]

device_id isn't stable identity — assign_device_ids hands out 0..N positionally over the BDF-sorted registry, so a device added with a lower BDF renumbers everything above it and a partition switch re-enumerates wholesale. It isn't only removals; growth renumbers too. Steps compound it, re-resolving cached ids through the swapped registry into build_job_injection_plans, so a running job's srun step can bind to the wrong device. Only applying when nothing is allocated avoids needing a durable key here; PCI BDF would be the key if you want per-job precision later.

[yansun1996]

This covers growth on an idle node — the safe case. The gap is the allocated one: if the refresh becomes gated, assert a confirmed change is a local no-op while a job holds a device and applies once it releases. (Minor: assert_eq!(node.free_memory_mb(), 256_000 - node.allocated_memory_mb) reduces to asserting total_memory_mb == 256_000; asserting that directly reads more clearly.)

[yansun1996]

This runs to 8 lines where the repo keeps comments to 1-2 lines of why, and the last sentence narrates the fix — same text is repeated on allocation_handle(). Two claims are also inaccurate: the id follows the slot, not the device; and while the release itself is fine, the vanish-then-return path can hand the device to a second job first. Same length note for main.rs 425-431 and 440-443 (the latter reflowed into a trailing-comment continuation at column 34), reporter.rs 34-36, agent_server.rs 734-737, and the test comment at cons_tres.rs 354-356.

[yansun1996]

This only varies cpus. Since change detection now drives a controller-side write, cases varying gpus alone and generic alone would be worth adding — that's where an unstable discovery order would show up as a re-register every tick.