fix(spurctld): reclaim agent allocations the controller no longer tracks on that node - #623
fix(spurctld): reclaim agent allocations the controller no longer tracks on that node#623yansun1996 wants to merge 3 commits into
Conversation
… map An agent that still holds a job the controller has finished with keeps that job's CPU and GPU reservation, and then rejects every subsequent dispatch to the node with a resources-unavailable error. The heartbeat reclaim exists to recover this: the agent reports what it is holding, and the controller re-sends a cancel for anything it considers finished. That reclaim stopped firing once terminal jobs began being aged out of the in-memory job map. The predicate treated an id the controller had no record of as not-reclaimable, so an aged-out job became invisible rather than finished, no cancel was ever sent, and the allocation was stranded for the lifetime of the agent. Treat an untracked id as reclaimable when it is below the next id this controller would assign. Ids at or above that watermark were never issued here -- an agent pointed at a reset or different controller -- and stay spared. Renamed the predicate to match what it now decides. No proto, config, or persisted-state change.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #623 +/- ##
==========================================
+ Coverage 77.59% 77.66% +0.08%
==========================================
Files 172 172
Lines 72153 72375 +222
==========================================
+ Hits 55980 56209 +229
+ Misses 16173 16166 -7 🚀 New features to boost your workflow:
|
The reclaim predicate reasoned only about job state, so a job that is legitimately Running on another node stayed spared on every node that still reported it. A node evicted mid-run keeps the job and its devices, the controller restarts the job elsewhere, and the old node then rejects every GPU dispatch for the lifetime of the agent. Treat an active job as releasable by any node absent from its allocation. Only an active job has an authoritative nodelist: a job that has not started yet may be mid-dispatch to the reporting node, so it stays spared.
yansun1996
changed the title
There was a problem hiding this comment.
Pull request overview
Updates spurctld’s heartbeat-driven reclaim logic so the controller can proactively cancel agent-held allocations that the controller considers stale (including jobs aged out of the in-memory job map and jobs restarted on a different node), preventing long-lived “poisoned” nodes that reject GPU dispatch.
Changes:
- Expand stale-job detection to treat (a) untracked-but-issued-below-watermark IDs and (b) active jobs not allocated to the reporting node as reclaimable.
- Add
ClusterManagerhelpers to support the new predicate (peek_next_job_id,job_holds_node). - Add unit tests covering eviction/unknown-id reclaim behavior and the “job moved to another node” case.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| crates/spurctld/src/server.rs | Adjust reclaim predicate and add targeted tests to ensure stale agent-held jobs are reclaimed safely. |
| crates/spurctld/src/cluster.rs | Expose helpers for determining job-id watermark and whether a job’s allocation includes a given node. |
Suppressed comments (1)
crates/spurctld/src/server.rs:251
- This warning message now claims the controller "no longer tracks" the job, but
is_reclaimablecan also be true for tracked terminal jobs and tracked jobs that are active elsewhere. The log line should be accurate for all reclaim reasons (otherwise it will mislead incident/debugging work).
warn!(
job_id,
node = %node,
"agent still holds a job the controller no longer tracks — re-sending cancel to reclaim its allocation"
);
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
crates/spurctld/src/server.rs:250
- This warning is emitted not only for unknown jobs, but also for terminal jobs still present in the map and active jobs tracked on another node. Saying the controller “no longer tracks” the job is therefore misleading during incident diagnosis; describe it as no longer assigned to this node instead.
"agent still holds a job the controller no longer tracks — re-sending cancel to reclaim its allocation"
Running state and allocated_nodes commit as two separate WAL entries, so a heartbeat landing between them could see Running with no nodelist yet and wrongly cancel the node's legitimate new run. is_reclaimable now reads state and allocation from one job snapshot and spares an active job until its nodelist is populated. Also clarifies the doc comments and log message, which described this as only "finished" jobs when it also covers jobs moved to another node and untracked ids below the watermark.
shiv-tyagi
left a comment
shiv-tyagi
left a comment
There was a problem hiding this comment.
LGTM. Solid fix and thorough tests. Two non-blocking comments inline (a stale signal-0 comment and a Q on node-name matching).
| // Signal 0 = graceful release, no-op on an unknown id. Not | ||
| // epoch-gated — a requeue racing this send is still possible. |
There was a problem hiding this comment.
The comment says signal 0 is a "graceful release, no-op on an unknown id". That's only true for the finished-job case. The new active-elsewhere branch sends this to a node still running the job, where the agent's graceful_cancel does SIGTERM then a delayed SIGKILL. It terminates a live process (the intended heal for a stranded run). Worth updating the comment so this isn't read as release-only.
| // An active job's nodelist is authoritative only once populated: state | ||
| // and allocation commit as two separate WAL entries, so a job can be | ||
| // observed as Running with `allocated_nodes` still empty. Spare it, same | ||
| // as a job earlier than active that may be mid-dispatch to this node. |
There was a problem hiding this comment.
Q: this compares allocated_nodes entries against the heartbeat hostname by exact string equality. If those two ever diverge in representation (FQDN vs short name, casing), a node legitimately running the job would not match its own allocation and get a SIGKILL for a live run. Are both guaranteed to be the same canonical node name here?
There was a problem hiding this comment.
agree to this concern
Actionable: don't compare against the raw heartbeat hostname. Resolve the reporting node to its canonical node name (the key allocated_nodes uses) before the comparison, or route the comparison through a single helper that owns node identity.
What this fixes
A node agent can keep holding a job's CPU/GPU reservation after the controller has finished with that run. The node then rejects every subsequent GPU dispatch with
controller-allocated GPUs unavailable on this node, while the controller still believes those devices are free and keeps selecting it. Jobs bounce off it until they exhaustmax_batch_requeueand are held permanently.The agent has a launch-path self-heal, but it only releases conflicting owners absent from its running map — correct, since releasing a job it believes is alive could double-book live work. The controller-side backstop is the heartbeat reclaim: the agent reports what it holds, and the controller re-sends a cancel for anything it considers finished. That backstop had two holes, fixed here.
1. A job aged out of the controller's job map was never reclaimed.
is_some_andyieldsfalsefor an id the controller has no record of. Since terminal jobs are aged out of the in-memory map to bound memory, a leaked job eventually reads as unknown rather than finished, and the cancel was never sent. The longer a leak survived, the less likely it became to ever be reclaimed.Now an untracked id is reclaimable when it is below the next id this controller would assign. Ids at or above that watermark were never issued here — an agent pointed at a reset or different controller — and stay spared.
2. A job the controller had restarted on a different node was never reclaimed.
The predicate reasoned only about job state, never about which node the job belongs to. When a node is evicted mid-run, the controller requeues the job elsewhere and — correctly, for a node it believes is down — sends no cancel. If that node was in fact alive, it keeps running the job and holding its devices. The job is then
Running(somewhere else), so the reclaim spared it on the original node forever.Now an active job is releasable by any node absent from its allocation. Only an active job has an authoritative nodelist; a job that has not started yet may be mid-dispatch to the reporting node, so it stays spared. That guard is pinned by its own test.
Observed in production: a controller restart caused a large batch of healthy nodes to be marked DOWN on stale heartbeats within one minute of the new leader taking over. Their jobs were requeued elsewhere while the nodes kept running them. Each affected node was then permanently unable to accept GPU work, with the underlying job visible as
RUNNINGon a different node. Left alone, the strand does not clear: nodes were observed still holding devices more than 24 hours later, across a controller restart and a leader election.The upstream trigger — a newly elected leader evaluating nodes against heartbeat timestamps that no non-leader could refresh — is a separate defect and is not addressed here.
Relationship to the dispatch-reject cooldown change
This is the companion to the open PR that cools down a node returning a resources-unavailable rejection [1]. They solve different halves and neither subsumes the other:
[1]'s own description names this as the follow-up: "reconciling the controller's allocation view against the node remains a separate follow-up." With only [1], leaked nodes accumulate quietly and never come back. With only this, a node still hot-loops during the window before its next heartbeat. Recommend landing both.
Known limitations
Deliberately out of scope here, surfaced by review and left for follow-ups:
Controller-side phantom allocations are not addressed. Nodes can hold
alloc_resourceswith no corresponding job, which makes them invisible to the scheduler entirely (they look busy, so no dispatch is ever attempted and no error is logged). That needs a reconcile of the controller's allocation view and is a separate, larger change.A cancel can be sent for an id the controller no longer knows. This is the deliberate trade: the fix acts on absence rather than treating it as a reason to do nothing. It is safe as long as an id below the watermark is never simultaneously absent from the controller and live on an agent. Two ways that can be violated, both accepted for now:
controller.first_job_idand climbs; once it passes a still-running pre-wipe id, that job becomes reclaimable and is cancelled. The damage is deferred and non-deterministic — it lands whenever submission volume crosses the old id, not at wipe time. Operationally: after wiping controller state, recycle the agents before resuming submissions.next_job_idlost-update race.apply_operationdoes a non-atomic read-modify-write on the counter while the submit path usesfetch_add, so it can regress and reissue an id. Pre-existing, worth filing separately. Note the regression direction is safe for this predicate specifically — a lower watermark only spares more ids, it can never manufacture a reclaim.An epoch (
run_attempt) on the cancel would close in-lifetime id reuse and remove the re-check tautology below, but it would not close the wipe case: the agent's reported epoch is its live epoch, so echoing it back proves only that the controller's snapshot is current, not that the job should die. Closing that properly needs a controller-incarnation id, which is a larger change and left as a follow-up.Ids below the watermark were not necessarily issued. Array parent ids are consumed but never stored, as are ids burned when submission fails after the counter bump. These read as reclaimable. Not reachable today because agents only report dispatched task ids; pinned by a test so the behaviour is explicit rather than accidental.
A job aged out while
Preemptedbecomes reclaimable.is_terminal()deliberately excludesPreempted(it may requeue), but eviction filters onis_finalized(), which includes it. Once evicted the id cannot be requeued under that id anyway, so the reclaim targets a run that provably ended.The pre-send re-check is inert for untracked ids. It exists to catch a requeue landing after the snapshot; an absent id can never un-become absent, so it adds nothing on the new branch. Retained because it still guards the terminal-but-present case.
Re-sends are unbounded. A permanently-reclaimable id produces one signal-0 cancel per heartbeat with no dedup or backoff. A healthy agent drops the id on the first cancel.
Controller-side phantom allocations are still not addressed (see above) — that remains the larger reconcile follow-up.
Testing
left: [], expected[20]) and passes with it.cargo clippy --workspace --exclude spur-ffi --all-targets --lockedclean;cargo fmt --all --checkclean; 729spurctldtests pass.