feat(spurd): survive agent restart without interrupting running jobs

[yansun1996]

Summary

spurd's SIGTERM handler unconditionally deregistered the node from the controller, which force-evicts every running job on that node immediately — so any graceful restart (e.g. a binary upgrade via systemctl restart spurd) failed in-flight jobs, even though the underlying process could survive it (it already runs in its own process group and a cgroup outside spurd's own systemd cgroup).

This PR makes a graceful restart transparent to running jobs, as long as the restart completes within heartbeat_timeout_secs (the existing heartbeat-timeout remains the backstop for a restart that takes too long — the same bound Slurm documents for a slurmd upgrade).

Approach

• SIGTERM: deregisters only when the agent has no active jobs (none running and none mid-launch). An idle node still deregisters immediately as before; a busy one stops serving and relies on the controller's heartbeat timeout as the safety net if it doesn't come back.
• Manifest (identity + obligation ledger): a small on-disk record is written per job right after a successful launch, split into two concerns. Identity — how to find and verify the process on restart (pid, /proc/<pid>/stat start time to detect PID reuse, cgroup path, exit-status sentinel path). Obligations — what the agent still owes the job once its process ends (release resources, run epilog + SPANK, report completion), each recorded as a pending flag alongside the resolved exit and any drain intent. Structuring the record around "what must still happen" rather than only "what the process looks like" keeps restart teardown honest: every obligation has an explicit home in the schema instead of being re-derived by the reconcile code (which is where obligations were being silently dropped). Written via a temp file + rename so a crash mid-write can't leave a torn manifest that startup would silently skip.
• Completion detection without waitpid: a restarted agent is no longer the parent of a still-running job, so it can't wait() it (a kernel restriction, not a design choice). Liveness is a tri-state read of the cgroup (cgroup.events): populated → alive, empty → done, and unreadable → fall back to a direct /proc/<pid> liveness check rather than assume the job finished (a non-cgroup-v2 host, or a process that was never moved into the cgroup, would otherwise be misread as complete). The exit code is recovered from a sentinel file the job's own wrapper script writes — with a re-exit so the normal (non-restarted) path's exit-status reporting is unchanged.
• Reconciliation as obligation discharge: before accepting new launches, the agent scans manifests and either re-adopts a still-alive job (restoring its exact resource allocation all-or-nothing, so a restarted agent can't double-book GPUs/CPUs a surviving job still holds) or, for a job that finished while the agent was down, drives its obligation ledger to empty — epilog + SPANK, then the completion report — persisting progress after each step. Reports are delivered inline before the gRPC server starts serving (bounded so an unreachable controller can't block startup), and the spool/manifest is removed only once every obligation is discharged. If the report is lost, the manifest stays for the next startup to retry — and because epilog is marked discharged before the report is attempted, the retry resends only the report, never re-running a non-idempotent epilog (GPU reset, scratch purge). The same teardown (epilog/SPANK/drain-on-failure) now runs for both live-monitored and restart-adopted completions via one shared path.
• Container jobs report real exit codes across a restart too. A container job runs its script inside the pivoted rootfs, so the same exit-status wrapper plain jobs already get is applied to the container script, writing to an in-container path that is host-visible in the rootfs and recorded in the manifest. A re-adopted container job now reports its true exit code (success or failure) instead of an approximate -1.
• Cgroups and per-job spool directories are keyed by (job_id, run_attempt), not job_id alone — otherwise a same-node redispatch while an old run is still finishing could collide on the cgroup (a cgroup-wide kill would take the new run down too) or read a stale exit-status sentinel from the prior run.
• The per-job spool directory stays root-owned and non-writable by the job (0o711); the job's exit-status sentinel is pre-created and chowned to it, so the job never needs to create files there and a co-located user can't plant a symlink over the root-authored manifest.
• spurctld warns at startup if heartbeat_timeout_secs is below 30s, since it's a more load-bearing safety net now than before.

Known limitations (not solved here)

• A restart slower than heartbeat_timeout_secs still falls back to today's eviction path.
• A job redispatched to a different node during such a slow restart isn't proactively cleaned up on the original node.
• PMI rendezvous state for a job that's mid-launch (not yet running) during a restart is still lost.
• Exit-status truth for a re-adopted job is inferred, not observed. Because a restarted agent can't wait() a job it no longer parents, the exit code comes from the sentinel file (or, failing that, /proc/cgroup inference). Two consequences: (a) on a non-cgroup-v2 host, or if the sentinel was never written (e.g. the process was SIGKILLed before its wrapper's trap armed), a re-adopted job can only report an approximate outcome; (b) the container sentinel lives in the job-writable rootfs, so a container job could write an arbitrary integer there and have the agent report it as that job's own exit code after a restart. The read is hardened against a planted symlink/FIFO/oversized file, but the value is the job's to state — bounded impact (a job can only lie about its own status), noted for completeness. Authoritative exit truth would require a live per-job supervisor (see below).
• The completion-detection fallback without a cgroup uses /proc/<pid> existence plus start-time. Start-time has clock-tick granularity, so a PID recycled within the same tick could in principle be mistaken for the original — realistically only across a reboot, which this feature doesn't cover (a rebooted node's jobs are already gone). The cgroup path, used whenever a cgroup is present, is not affected.
• An interactive srun allocation (no batch process) is not written to a manifest, so it isn't restored on restart; interactive sessions don't survive a restart in any case (the client connection drops). A node holding only such a reservation now deregisters on SIGTERM so the controller reclaims it, rather than stranding the allocation.
• A container job's rootfs directory is keyed by job id alone (not run attempt), unlike its cgroup and spool dir, so a same-node redispatch reuses one rootfs. Pre-existing; a follow-up could align the keying.
• No crash isolation. This covers a graceful restart (SIGTERM/upgrade). A spurd panic or OOM is survived by the job process itself (it re-parents to init), but the agent's authoritative wait()-based exit truth for that job is lost at that instant, degrading it to the inference path above.

Structurally, the exit-truth gap, the mid-launch PMI loss, and crash isolation all stem from the same root: reconstructing state from a manifest after the fact, rather than querying a live per-job supervisor (as Slurm does with slurmstepd). That trade-off is deliberate and adequate for the graceful-upgrade goal this PR targets; a supervisor process is out of scope here. The obligation-ledger schema above is the piece that carries forward under either model, and the natural next step if authoritative exit codes or crash isolation become requirements is a minimal per-job exit-recorder (a supervisor's embryo), not a rewrite.

Testing

• Unit tests in executor.rs, agent_server.rs, and cons_tres.rs, several exercising real spawned processes rather than simulating behavior — including: a resumed container job reading its real exit code from the sentinel; a mixed reconcile pass that re-adopts a surviving job and discharges only the finished one; the obligation ledger running the epilog exactly once across repeated report-retries (not once per retry); tri-state cgroup liveness falling back to a /proc check when cgroup.events is unreadable; all-or-nothing GPU restore rejecting a double-book; the exit sentinel refusing a planted FIFO/symlink and capping the read; run-attempt-scoped spool isolation; and atomic manifest write (no leftover temp file).
• E2E (test_deregistration.py): restart mid-job and assert the job survives, completes, and its allocation is neither stranded nor double-booked; exit code preserved across a restart for the full matrix of plain and container jobs × success and failure; and a job that finishes entirely while the agent is down still reports its true exit code on restart.
• cargo clippy --workspace --exclude spur-ffi --all-targets and cargo fmt clean; targeted cargo test passes.
• Verified end-to-end on an isolated, throwaway 2-node deployment (non-production ports): happy-path exit-code capture through the trap wrapper; a job re-adopted across an agent restart, running to completion and reporting its real exit code; a job that finished while the agent was down retaining its manifest across a controller-unreachable restart and reporting correctly once the controller returned; and a reservation-only node deregistering on SIGTERM so the controller reclaims it.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 90.16163% with 140 lines in your changes missing coverage. Please review.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #492      +/-   ##
==========================================
+ Coverage   76.03%   76.41%   +0.38%     
==========================================
  Files         166      166              
  Lines       63002    64336    +1334     
==========================================
+ Hits        47898    49159    +1261     
- Misses      15104    15177      +73     

🚀 New features to boost your workflow:

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

[Copilot]

Pull request overview

This PR updates spurd to support transparent graceful restarts (e.g., systemctl restart spurd) without force-evicting currently running jobs, by persisting per-job launch state to disk and reconciling/resuming those jobs on startup while relying on the existing heartbeat timeout as the safety net for overly slow restarts.

Changes:

• Update spurd shutdown/startup flow: avoid SIGTERM deregistration when jobs are running, and reconcile manifests on startup to re-adopt surviving jobs and restore their allocations.
• Add manifest + resume plumbing in the executor (exit-status sentinel, cgroup-based completion detection, run-attempt-scoped cgroups) and allocation restoration support in the scheduler.
• Add/extend unit and e2e coverage for restart survival, manifest scanning/reconciliation, and restore-allocation behavior; add a controller startup warning for low heartbeat timeouts.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
tests/native_host/e2e/test_deregistration.py	Adds an e2e acceptance test asserting agent restart mid-job does not interrupt the job and does not strand/double-book CPU allocation.
crates/spurd/src/main.rs	Reconciles running jobs before serving; changes SIGTERM handling to deregister only when no jobs are running.
crates/spurd/src/executor.rs	Implements job manifests, exit-status sentinel wrapping, resumed-job completion detection via cgroup/proc liveness, and run-attempt-scoped cgroups; adjusts spool-dir behavior.
crates/spurd/src/container.rs	Makes RootfsMode serializable for inclusion in job manifests.
crates/spurd/src/agent_server.rs	Adds startup reconciliation to restore allocations/track resumed jobs; writes manifests immediately after launch.
crates/spurctld/src/main.rs	Warns at startup when heartbeat_timeout_secs is configured below 30s.
crates/spur-sched/src/cons_tres.rs	Adds restore_committed() to pin/restore exact resource IDs after agent restart.

---

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

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

crates/spurd/src/executor.rs:483

• write_job_manifest() uses std::fs::write(), which creates manifest.json with default permissions (typically 0644). Because per-job spool dirs are 0o711, any local user who can guess the path (job IDs are enumerable) can read another job’s manifest and learn paths/UID/GID/cgroup/resource details. The manifest should be root-only readable (0600) and ideally written via an OpenOptions path that refuses symlinks.

pub fn write_job_manifest(spool_dir: &Path, manifest: &JobManifest) {
    let path = manifest_path(spool_dir);
    let tmp = path.with_extension("json.tmp");
    let bytes = match serde_json::to_vec(manifest) {
        Ok(b) => b,

[biluriuday]

Couple of comment on the design decisions:

1. The cost is paid on the universal path for a benefit on the rare path. Surviving a restart is a rare event; wrapping every job's script in a subshell, writing a manifest for every launch, and re-keying every spool and cgroup path are universal changes. That trade is only acceptable if the universal change is provably neutral, and it isn't. I reproduced this locally with the exact generated wrapper shape: bash executes a script file incrementally, but a subshell is one compound command that must parse completely before any of it runs. An unwrapped script with a syntax error on line 2 still runs line 1; the wrapped version runs nothing. Every job on the cluster now has different failure semantics so that restarts can recover an exit code.

2. The manifest captures process identity but not the job's lifecycle obligations. The author is candid that reconstructing state after the fact is the trade-off versus a slurmstepd-style supervisor, and that's a reasonable call for this PR. But three of the blocking bugs below are the same failure repeated: the manifest records what a job is (pid, cgroup, resources) and not what the agent still owes it (run the epilog, report the completion, hold the srun reservation). If the manifest is going to be the durable record, its schema should be driven by "what must still happen to this job," not "what does this process look like."

Few issues identified. Please check

1. A missing cgroup.events is read as "job finished." executor.rs:520-527 returns false when the file can't be read, and Resumed::try_wait turns that into completion. setup_cgroup returns Ok(Some(path)) after a bare create_dir_all with no cgroup-v2 detection, and every attribute write below it only warns. On a v1 or hybrid host that path is an ordinary directory that never gets a cgroup.events, so on the first monitor tick after a restart every adopted job is reported complete with exit -1 and its GPUs are released to another job while the real workload keeps running. move_to_cgroup failing silently (executor.rs:1657 discards the error outright) produces the same outcome on a v2 host. Make cgroup_populated tri-state and fall back to proc_alive, and record cgroup_path: None when the process was never actually moved in.

2. The exit sentinel is a denial-of-service on spurd startup. read_exit_status opens with O_NOFOLLOW, which refuses a symlink but says nothing about a FIFO, and opening a FIFO read-only blocks until a writer appears — I confirmed this blocks indefinitely. For container jobs the sentinel path is inside the job-writable rootfs, so mkfifo /tmp/spur_exit_status in a batch script is enough. Because reconcile_running_jobs() is awaited at main.rs:294 before the server future is ever polled, the agent never starts again. Open with O_NONBLOCK, fstat the fd, and refuse anything that isn't a regular file with st_nlink == 1.

3. restore_committed double-books the exact GPU the PR exists to protect. cons_tres.rs:221-237 validates nothing before mutating. Two manifests claiming the same device both land in owners mapped to one bit; releasing the first clears the bit while the second job still holds the device, and the next allocate_for_job hands it out. This was verified with a passing test on PR head. allocate_for_job already guards this at lines 178-180 — restore_committed needs the same all-or-nothing validation and a Result return so adopt_manifests can refuse to adopt.

4. The manifest is deleted before the completion is reported. agent_server.rs:334 calls cleanup_job_spool inside adopt_manifests; the report only happens afterwards at lines 252-266. If the 30-second budget fires, or the report exhausts its retries, or it comes back InvalidArgument (which completion_report_retryable excludes), the completion is gone and there is no backstop — the node is registered and heartbeating, so the controller will never time it out and the job sits in Running forever. The code comment at line 250 asserts the opposite of what the code does. Delete the spool directory only after a successful report.

5. srun/salloc reservations are a new double-booking regression. register_job_allocation inserts RunningJob::AllocationOnly into running at agent_server.rs:1618, so has_no_active_jobs now returns false for a node holding only a reservation and SIGTERM skips deregistration. But the manifest write is gated on result.job.pid() and AllocationOnly has none, so nothing persists. After restart the agent's table is empty while the controller still holds those CPUs and GPUs, and the agent first-fits the same device ids to the next dispatch. The PR description lists this as "the allocation is simply absent rather than double-counted" — that is exactly backwards, and it's a regression from the old unconditional-deregister behavior.

6. Adopted jobs skip the epilog and SPANK hooks. The monitor path runs hooks.epilog, SpankHook::TaskExit, and SpankHook::JobEpilog, and drains the node if the epilog fails. The Dead branch at agent_server.rs:321-341 does none of it. Epilog is where sites do GPU reset and scratch purge, so the next job onto those GPUs inherits dirty state, and the drain safety valve is bypassed for precisely the jobs most likely to need it. The manifest already carries every field HookContext needs. Factor the completion teardown into a shared function called from both paths — this is the refactor AGENTS.md asks for rather than a band-aid.

7. The subshell wrapper's parse-model change, described above. A brace group plus an outer trap ... EXIT gives you the "internal exit still writes the sentinel" property without changing when bash parses.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Suppressed comments (5)

crates/spurd/src/agent_server.rs:554

• In the live monitor path, the manifest is only updated when report_completion fails. If spurd crashes after run_teardown_hooks() succeeds but before cleanup_job_spool() runs, the stale on-disk manifest still says epilog is owed, so the next startup's reconcile will run the epilog/SPANK hooks a second time (the exact non-idempotency the obligation ledger is meant to prevent). Consider marking epilog discharged in the manifest unconditionally after running teardown hooks, regardless of whether the completion report succeeds.

                    // Keep the spool (and its manifest) until the controller has
                    // the completion: if the report was lost, a restarted spurd
                    // re-adopts the manifest and retries rather than stranding the
                    // job in Running forever. Record that the epilog is already
                    // done so the restart retries only the report, not the
                    // (possibly non-idempotent) epilog.
                    if reported {
                        crate::executor::cleanup_job_spool(c.job_id, c.run_attempt);
                    } else {
                        crate::executor::mark_manifest_epilog_discharged(
                            c.job_id,
                            c.run_attempt,
                            (c.exit_code, c.signal),
                            drain,
                        );
                    }

crates/spurd/src/agent_server.rs:1596

• Job manifests are written with a hard-coded schema_version of 1 here (and in several tests below). Since scan_job_manifests() filters by executor::MANIFEST_SCHEMA_VERSION, bumping the schema in executor.rs without updating all of these literals will silently stop reconcile from seeing newly-written manifests. Consider exporting MANIFEST_SCHEMA_VERSION (pub(crate)) from executor and using it everywhere manifests are constructed (runtime + tests) to avoid drift.

                        &executor::JobManifest {
                            schema_version: 1,
                            job_id,

crates/spurd/src/agent_server.rs:1600

• start_time is used to disambiguate PID reuse during reconcile; defaulting it to 0 when /proc//stat can't be read will make proc_alive() fail and can misclassify a still-running job as dead on restart (releasing resources / reporting completion incorrectly). If proc_start_time() fails, it's safer to skip writing the manifest (best-effort: the job just won't survive a spurd restart) than to write one that guarantees a false negative.

                            pid: pid as i32,
                            start_time: executor::proc_start_time(pid as i32).unwrap_or(0),
                            cgroup_path: result.job.cgroup_path().map(|p| p.to_path_buf()),

crates/spurd/src/agent_server.rs:344

• discharge_obligations() clears manifest.pending.report_completion after a successful report but never persists that change. If spurd crashes (or cleanup_job_spool fails) before the spool dir is removed, the next restart will re-run the completion report even though it already succeeded. Persist the updated manifest before proceeding to cleanup so retries resume from the correct ledger state.

This issue also appears on line 539 of the same file.

            if !reported {
                // Report lost; leave the manifest (epilog already marked done, so
                // the retry won't re-run it) for the next startup to resend.
                return;
            }
            manifest.pending.report_completion = false;
        }

crates/spurd/src/agent_server.rs:4696

• This test writes the manifest under $TMPDIR/spur, but scan_job_manifests() intentionally does not scan the temp-dir base when running as root (to avoid trusting user-writable manifests). If the test suite runs as root, reconcile_running_jobs() won't see this manifest and the test will fail. Make the spool_dir base conditional on euid so it matches executor::spool_bases() behavior.

        let spool_dir = std::env::temp_dir()
            .join("spur")
            .join(format!("job{job_id}_1"));
        std::fs::create_dir_all(&spool_dir).unwrap();

[yansun1996]

Thanks for the review — this was the kind of scrutiny the PR needed, and objection B in particular changed the shape of the fix rather than just adding patches.

On the 7 issues — all addressed:

• [1] cgroup liveness is now tri-state; an unreadable cgroup.events (non-v2 host, or a process never moved into the cgroup) falls back to a /proc liveness check instead of being read as "finished." setup_cgroup also detects a real cgroup-v2 mount before creating anything, and a failed move_to_cgroup records cgroup_path: None so the manifest never claims a cgroup the process isn't in.
• [2] the exit sentinel is opened O_NONBLOCK | O_NOFOLLOW and fstat-checked for a single-link regular file, so a planted FIFO can't block startup.
• [3] restore_committed validates all GPUs all-or-nothing and returns a Result; adoption is refused on conflict rather than double-booking.
• [4] the spool/manifest is deleted only after the controller acks the completion.
• [5] has_no_active_jobs excludes reservation-only entries, so such a node deregisters on SIGTERM and the controller reclaims it.
• [6] epilog + SPANK + drain now run for adopted jobs too, via a shared teardown path.
• [7] the wrapper is a top-level trap … EXIT, not a subshell. I checked the brace-group suggestion — it parses non-incrementally exactly like the subshell (a syntax error still runs nothing), so it wouldn't have solved it; the bare trap keeps incremental parsing while still capturing an internal exit and a parse-time failure.

On objection B (the important one) — you're right, and I reworked the schema around it. The manifest is now split into identity (how to find/verify the process) and an obligation ledger (pending { epilog, report_completion, drain } plus the resolved exit), each cleared as it's discharged. Reconcile drives that ledger to empty and rewrites the manifest after each step, so a teardown interrupted by a lost completion report resumes at the still-owed step instead of repeating a non-idempotent one — concretely, a retried report no longer re-runs the epilog. That's what turns [4], [5], [6] into consequences of the schema rather than three separate patches. Writes stay off the hot path (one per launch; the ledger is only rewritten on the rare failure/adoption path), so the universal cost you flagged in objection A stays at ~one write per job.

On the slurmstepd point — agreed that a persistent per-step supervisor is the structurally stronger answer, and I want to be explicit about where this design stops rather than imply the manifest closes the gap. What it can't do, and a supervisor could: give waitpid-accurate exit truth for a re-adopted job (across a restart spurd is no longer the parent, so exit comes from the sentinel or /proc/cgroup inference), and provide crash isolation (a spurd panic/OOM is survived by the job process but loses the authoritative exit path). I've written both up under Known limitations. My read is that a supervisor is the right move once accurate exit codes or crash isolation become actual requirements — the obligation-ledger schema is deliberately the piece that carries forward into that model, so this isn't a dead end. It felt out of scope for a graceful-upgrade PR, but I'm happy to open a follow-up issue to track it if you agree that's the right sequencing.

Everything above has unit tests, and I verified restart survival, the epilog-once-on-retry path, and reservation deregistration end-to-end on a 2-node setup.

[biluriuday]

Thanks @yansun1996 for addressing the comments. Few more comments. Please check:

1. When bash is killed by a signal it does run the EXIT trap, but $? at that moment is the last completed command's status, not 128+N. I verified this directly: SIGTERM to a script shell blocked in sleep 30 produces a sentinel of 0.

graceful_cancel sends SIGTERM, and for a Resumed job that goes through cgroup_signal_all to every pid including the batch shell. The monitor then reads 0 from the sentinel and report_completion derives Completed from exit_code == 0. scancel on any job that survived a restart now records it as a successful completion. The previous revision at least reported -1 here. This is silent corruption of job state on the feature's primary path.

The fix is to stop inferring a signal from $?. Have the wrapper write a two-field sentinel (exited N versus signaled N) and install explicit signal traps that record 128+N; that also resolves the long-standing asymmetry where decode_shell_exit is applied only on the resumed path.

Two related gaps in the same wrapper: a user script installing its own trap ... EXIT silently replaces the wrapper's trap, and a trailing exec replaces the shell — both verified, both produce no sentinel and a -1. Scratch-cleanup traps are common enough that this is the dominant real-world case, and the doc comment currently claims the trap "fires on any exit."

2. restore_committed now validates GPUs all-or-nothing and returns Result — but the CPU loop underneath is untouched, with no conflict check and no range error. Two manifests claiming cpus 0 and 1 both return Ok, releasing the first frees cores the second is still running on, and a third job gets pinned to them. Memory is still saturating_add with no rejection of an over-total value, so releasing a saturated job still wipes every other job's accounting — also reproduced, and a new test now enshrines the saturating behavior as intended.

The right fix is the shared helper: extract resolve_indices and apply so allocate_for_job and restore_committed cannot diverge.
Relatedly, when validation does fail, the caller logs and continues, leaving the process running but absent from running — unkillable via cancel_job, unaccounted for in the allocator, and recoverable only on the next restart. A conflict means one of the two jobs has to die; neither adopting nor ignoring is that decision.

3. has_no_active_jobs still evaluates the running lock as a temporary inside the if condition, and Rust drops condition temporaries before the block, so the guard is released before has_launching() runs. The doc comment above it still asserts that "checking both closes the window." The fix is one line: bind the guard to a variable.

4. reconcile_manifest checks proc_alive before consulting manifest.exit, so a pid collision can flip an already-completed job back to Alive and re-run its teardown; and the displaced-attempt path kills the old run but never removes its spool directory, which is a guaranteed unbounded leak plus a spurious Dead report on every subsequent restart.