Skip to content

fix(spurd): redeliver completion reports across a controller outage - #521

Open
yansun1996 wants to merge 1 commit into
ROCm:mainfrom
yansun1996:fix/completion-report-recovery
Open

fix(spurd): redeliver completion reports across a controller outage#521
yansun1996 wants to merge 1 commit into
ROCm:mainfrom
yansun1996:fix/completion-report-recovery

Conversation

@yansun1996

@yansun1996 yansun1996 commented Jul 28, 2026

Copy link
Copy Markdown
Member

What this fixes

Closes #501.

When a job finishes, the node agent (spurd) reports completion to the controller via a ReportJobStatus call. That report was best-effort: it retried only 3 times over ~3 seconds, then dropped permanently. If the controller was unreachable longer than that window — a routine Raft leader failover, a spurctld restart, or a brief network blip — the completion was lost forever. The job then hung in COMPLETING until the complete_wait_secs force-finish timer fired (default 300s), during which the node could be handed a new job it would reject as "resource unavailable" (the reported 2–5 minute node hold).

Approach

Completion delivery is now durable against a controller outage:

  • Retry until resolved. Instead of giving up after 3 attempts, delivery retries with capped exponential backoff (1s doubling to a 30s cap) until the controller either acks or returns a permanent (non-retryable) error. A recoverable outage can no longer permanently lose a completion. Controller-side handling is already idempotent — a duplicate or late report for a terminal or superseded run is acked (AlreadyTerminal / StaleReport), so unbounded retry is safe.
  • Decoupled from detection. The monitor loop now spawns delivery instead of awaiting it inline, so a slow or unreachable controller no longer stalls detection and cleanup of other completions on the node.
  • NotFound is retryable. A just-restarted controller may not have replayed a job's log entry yet when the report arrives; since jobs are never purged from the controller's map, retrying resolves once replay finishes rather than dropping the report.

No proto change — the wire message and RPC contract are unchanged, so old/new agents and controllers interoperate without version coordination.

Slurm parity

For reference, Slurm delivers a completion (REQUEST_COMPLETE_BATCH_SCRIPT for batch jobs, REQUEST_STEP_COMPLETE for steps) from slurmstepd with an unbounded in-process retry loop at a fixed 15s interval, and its controller stays robust through a second, independent layer: node-registration reconcile. On each node re-registration (~SlurmdTimeout/3, ~100s), slurmctld compares the jobs the node reports running against what it believes is running and actively finalizes any it no longer sees (validate_node_specs / _purge_missing_jobsjob_complete()). Notably, Slurm's retry queue is in-memory on the slurmstepd stack, not on disk — and Slurm has no time-based force-finish timer (CompleteWait only gates scheduling new work, it does not finalize a stuck job).

Slurm Spur before Spur after
Agent retry unbounded, fixed 15s 3× over ~3s then drop unbounded, 1→30s backoff
Idempotent controller ack yes yes yes
Survives agent restart no (in-memory stepd loop) no no ⚠️ (deferred, item [1])
Node-registration reconcile yes (real recovery) no no ⚠️ (deferred, item [2])
Time-based backstop none (uses reconcile) 300s force-finish 300s force-finish

This PR brings the delivery path to parity (and slightly beyond — backoff vs. Slurm's flat 15s) for the common transient-outage case. The remaining parity gap is the two deferred items below.

Known limitations (follow-up)

  • [1] Survives a controller outage, not an agent restart. The retry state is in-memory; if spurd itself crashes or is upgraded mid-retry, the pending completion is lost and the job falls back to the force-finish timer. (Slurm is no stronger here — its queue is also in-memory — so a persistent on-disk agent queue would actually exceed Slurm, not just match it.)
  • [2] No controller-side node-registration reconcile. Spur's only controller-side backstop is the 300s force-finish timer. It has no equivalent to Slurm's reconcile: the running_jobs field exists on HeartbeatRequest but the agent sends it empty and the controller ignores it. Wiring that up (node reports what it is actually running; controller finalizes jobs the node no longer runs) is the higher-value follow-up — it recovers both the controller-outage case and the agent-restart case [1] leaves open, and matches how Slurm stays robust without durable agent state.
  • Retry tasks are unbounded. Each completed job spawns one retry task with no global cap. Bounded in practice by the node's concurrent job count and the 30s backoff cap, but a very long outage on a busy node would accumulate tasks.

Testing

  • Unit tests for the retry loop (drives the real loop with an injected per-attempt result): retry-until-acked, stop-on-permanent-error, success-first-try, backoff doubling/saturation, error classification (including NotFound now retryable), and that the built request carries the drain fields.
  • New native-host e2e (test_completion_recovery.py): submit a job, stop the controller while it is running so the completion is generated during the outage, restart the controller, and assert the job reaches CD well under complete_wait_secs — proving the redelivered report finalized it, not the force-finish timer.
  • Validated end-to-end on an isolated 2-node deployment: with the controller killed mid-run, the agent logs the completion retrying with the expected 1→2→4s backoff while the controller is down, and the job finalizes within seconds of the controller returning (versus hanging to the 300s force-finish on the prior behavior).

The agent reported job completion with only 3 retries over ~3s, then
dropped the report permanently. A controller unreachable longer than that
window (Raft leader failover, restart, or a network blip) lost the
completion, leaving the job stuck in COMPLETING until the complete_wait_secs
force-finish timer (default 300s) — during which the node could be handed a
new job it would reject.

Retry delivery with capped exponential backoff (1s doubling to 30s) until
the controller acks or returns a permanent error, and spawn the delivery so
a slow controller no longer stalls the monitor loop's detection of other
completions. Controller-side handling is idempotent (duplicate/late reports
for a terminal or superseded run are acked), so unbounded retry is safe. A
NotFound is now retryable to cover the brief cold-start window before a
restarted controller replays the job's log entry.
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 95.17241% with 7 lines in your changes missing coverage. Please review.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #521      +/-   ##
==========================================
+ Coverage   73.72%   73.84%   +0.13%     
==========================================
  Files         158      158              
  Lines       55327    55417      +90     
==========================================
+ Hits        40785    40921     +136     
+ Misses      14542    14496      -46     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens spurd’s job-completion reporting so transient controller outages (restart/failover/network blip) don’t permanently drop a terminal ReportJobStatus, preventing jobs from getting stranded in COMPLETING until complete_wait_secs fires.

Changes:

  • Replace the fixed 3-attempt completion report with an ack-or-permanent-error retry loop using capped exponential backoff, and treat NotFound as retryable.
  • Decouple completion delivery from the monitor loop by spawning delivery tasks so controller unavailability doesn’t stall completion detection/cleanup.
  • Add coverage: unit tests for retry/backoff/classification and a new native-host e2e that restarts the controller mid-run and asserts prompt completion.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
crates/spurd/src/agent_server.rs Spawned completion delivery plus a capped-backoff retry loop and expanded retryability (incl. NotFound) with unit tests.
tests/native_host/e2e/test_completion_recovery.py New e2e test validating completion report redelivery across a controller restart.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +42 to +46
# Stop the controller while the job is still running so the completion is
# generated during the outage and must be redelivered after restart.
cluster.stop_controller()
time.sleep(8)
cluster.restart_controller()
@yansun1996
yansun1996 marked this pull request as ready for review July 29, 2026 22:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

spurd: lost completion report strands a job in COMPLETING until the 300s force-finish

3 participants