Skip to content

feat(spurd): survive spurd restarts via a per-job stepd supervisor - #754

Open
yansun1996 wants to merge 16 commits into
ROCm:mainfrom
yansun1996:feat/stepd-core
Open

feat(spurd): survive spurd restarts via a per-job stepd supervisor#754
yansun1996 wants to merge 16 commits into
ROCm:mainfrom
yansun1996:feat/stepd-core

Conversation

@yansun1996

@yansun1996 yansun1996 commented Aug 26, 2026

Copy link
Copy Markdown
Member

Summary

Batch, container, and PMIx multi-task jobs now survive an spurd restart or upgrade. Each job is handed off to spurstepd, a per-job supervisor process spawned by spurd that runs for the job's whole lifetime independent of spurd's own restarts — spurd discovers and reattaches to any still-running spurstepd on startup instead of losing track of the job. Gated behind SPUR_STEPD=1 (off by default); no behavior change for existing deployments that don't set it.

Approach

  • spurstepd is a genuinely separate binary (not a subcommand), split out via a new spurd lib crate + spur-stepd bin crate — a different trust/threat boundary than spurd's network-facing RPC and cluster-admin (k0s) surface. It's spawned via the same double-fork+setsid+exec pattern Slurm uses for slurmstepd, and discovered/reattached to via a persisted descriptor (pid + /proc/<pid>/stat start-ticks, since the discovering spurd is never the real OS parent).
  • Crash-watchdog fencing (dead/displaced/superseded stepd), crash-safe completion replay, and controller-side recovery reporting with node-token identity verification round out the restart-survival guarantee.
  • Cgroup membership (cgroup.procs) is the primary kill-target set for any signal, not just a SIGKILL backstop — this reaches descendants that detached from the job's process group (e.g. via setsid) the way a plain process-group signal can't.
  • A missing/bad spurstepd binary now fails a launch in milliseconds (an exec-failure byte over the handoff pipe) instead of waiting out the full readiness timeout.

Known limitations / follow-ups

  • spurstepd still depends on the whole spurd lib crate (including tonic/tower/spur-net), so the isolation achieved today is that its process never runs that code, not that it isn't linked. Genuinely splitting the runtime-only surface (executor/container/stepd) into its own crate is a follow-up.
  • Logical steps (srun inside an existing allocation) and interactive PTY attach (srun --pty) are not yet supported for a stepd-backed job — both fail with a clear, explicit error rather than silently falling back or hanging.
  • This is PR 1 of a planned stack; PTY sessions and PMIx/steps get their own follow-up PRs.
  • Requires [auth] jwt_key (or jwt_key_file) set to the same value on the controller and every agent. The controller signs each node an identity token at registration and the agent presents it when reporting a recovered supervisor; without the key spurd refuses to start rather than run with a guarantee it cannot honour. The key is independent of [admission] mode — restart survival works under open admission, though there the token attests the name a node registered under rather than one an operator admitted.
  • KillMode=process is required in the spurd unit. systemd's default kills every process in the unit's cgroup, which takes the supervisors with it; the next agent then reclaims the surviving job as orphaned. The documented unit sets it, and moving supervisors into their own delegated scope is left as follow-up.

Deferred from review

  • Per-step supervisors. This ships one supervisor per job. step_id now travels through both new RPCs and the on-disk layout, but re-keying the runtime maps is deferred with the numbered-step work: per-step recovery needs a policy for "one step's supervisor died but the batch step is fine", and displacement has to decide whether a new attempt displaces all of a job's steps or only the matching key.
  • Durable dispatch/completion generations. No replicated record names the attempt and cohort before the launch RPC, and JobNodeComplete carries no attempt. Both are pre-existing on main; this PR narrows a sibling gap by threading run_attempt into the cancel RPCs. Record-before-dispatch belongs to the allocation-reconciliation cutover.
  • Attempt identity at the cancel/suspend/step-cancel/PMIx sinks. Signalling still happens before the attempt is compared, and three request messages carry no attempt at all. Also pre-existing; closing it means appending proto fields and re-checking identity at each sink, which deserves its own review unit.

Testing

Unit: 309 tests in spurd, 942 in spurctld, all passing; cargo clippy --workspace --exclude spur-ffi --all-targets --locked clean. Every behavioral fix in this PR has a test that was confirmed to fail against the prior code and pass against the fix.

Live-tested end-to-end on an isolated 2-node deployment:

Job type Restart survival Cancel / teardown Notes
Single-node batch ✅ SIGTERM → grace → SIGKILL reconnects and resumes tracking after an spurd restart
Container (squashfs) privilege correctly dropped to the submitting user
PMIx multi-task fan-out (--mpi=pmix --ntasks-per-node) real PMIx server, correct universe/local-proc counts, clean exit
Multi-node salloc allocation both nodes register and tear down cleanly
spur exec into a stepd-backed job N/A (deferred capability) fails closed: "exec is not yet supported for a stepd-backed job"
srun --pty attach to a stepd-backed job N/A (deferred capability) fails closed: "interactive attach is not yet supported for a stepd-backed job"

Also verified directly against a real Slurm cluster, side by side: spurstepd's session/process-group structure (its own session leader, reparented to init, job subtree in its own process group within that session) matches slurmstepd exactly.

@yansun1996
yansun1996 requested a review from sgopinath1 as a code owner August 26, 2026 00:36
Copilot AI lite review requested due to automatic review settings August 26, 2026 00:36

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 introduces a per-job supervisor process (spurstepd) to allow jobs to survive spurd restarts/upgrades, with supporting protocol/API changes for stepd recovery probing/fencing and attempt-scoped runtime isolation (cgroups, cancellations, resource reservations).

Changes:

  • Add a new spurstepd binary (via a new spurd library crate + spur-stepd bin crate) and stepd runtime state/discovery machinery.
  • Extend the gRPC proto and controller/agent implementations for stepd recovery reporting and probe RPCs, plus run_attempt plumbing for safer redispatch/cancel behavior.
  • Add file-backed JWT secret configuration (auth.jwt_key_file) and propagate resolved-key handling across controller and agent.

Reviewed changes

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

Show a summary per file
File Description
proto/slurm.proto Adds stepd recovery/probe RPCs and run_attempt fields to relevant requests.
install.sh Installs the new spurstepd binary.
Dockerfile Builds and ships spurstepd in container images and dist artifacts.
crates/spurd/src/stepd.rs Implements stepd descriptor/state store, Unix-socket protocol, and supervisor loop.
crates/spurd/src/reporter.rs Labels stepd capability on registration and adds stepd recovery report RPC client.
crates/spurd/src/main.rs Integrates stepd discovery/adoption, completion replay, and recovery reporting at startup.
crates/spurd/src/lib.rs Introduces spurd as a library crate to share runtime code with spurstepd.
crates/spurd/src/executor.rs Adds run_attempt to launch config; attempt-scoped cgroups; cgroup signaling/cleanup improvements; per-job scratch workdir fallback.
crates/spurd/src/container.rs Makes container config types serializable for persistence into stepd launch specs.
crates/spurd/src/cluster.rs Renames role parsing helper (from_strparse_role).
crates/spurd/Cargo.toml Adds [lib] target and new deps used by stepd/capability handling.
crates/spurctld/src/server.rs Adds controller-side recovery cohort tracking, probing, fencing, and node-identity authorization; switches JWT key resolution to file-aware path.
crates/spurctld/src/scheduler_loop.rs Threads run_attempt into cancel paths and allocation registration to avoid attempt races.
crates/spurctld/src/main.rs Resolves JWT key at startup and passes it into server::serve.
crates/spurctld/src/cluster.rs Splits preemption API to allow optional provenance (needed for controller-initiated fencing).
crates/spur-tests/src/t01_run.rs Updates scheduler allocation tests for new run_attempt parameter.
crates/spur-stepd/src/main.rs Adds the spurstepd binary entrypoint that runs the stepd supervisor logic.
crates/spur-stepd/Cargo.toml Declares the new spur-stepd crate/binary and depends on spurd as a library.
crates/spur-sched/src/cons_tres.rs Tracks reservations by (job_id, run_attempt) semantics to prevent superseded attempts from clobbering current allocations.
crates/spur-k8s/src/agent.rs Implements ProbeStepd for the virtual agent (returns inactive).
crates/spur-devices/src/inject.rs Makes injection plan types serializable for persistence into stepd launch specs.
crates/spur-core/src/step.rs Adds a default step id helper for backwards-compatible deserialization.
crates/spur-core/src/config.rs Adds auth.jwt_key_file and key resolution/validation helpers (+ tests).
crates/spur-cli/src/sinfo.rs Updates mock controller expectations for the new RPC.
crates/spur-cli/src/mock_controller.rs Updates mock controller interface for the new RPC.
Cargo.toml Adds crates/spur-stepd to the workspace members.
Cargo.lock Adds the new crate and new dependencies pulled in by stepd work.
AGENTS.md Documents the new spurstepd binary and its purpose/threat boundary.

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

Comment thread crates/spurd/src/main.rs Outdated
Comment thread crates/spurd/src/main.rs Outdated
@codecov-commenter

codecov-commenter commented Aug 26, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 77.71234% with 719 lines in your changes missing coverage. Please review.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #754      +/-   ##
==========================================
- Coverage   80.15%   80.08%   -0.07%     
==========================================
  Files         184      186       +2     
  Lines       87772    92980    +5208     
==========================================
+ Hits        70348    74460    +4112     
- Misses      17424    18520    +1096     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@biluriuday biluriuday left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Overall very good job with the PR. Few points mentioned in-line. Apart from those, please make sure you add the spurstepd to the official CI and release atrifacts

Comment thread crates/spurd/src/agent_server.rs
Comment thread crates/spurd/src/agent_server.rs
Comment thread crates/spurd/src/stepd.rs Outdated
Ok(Response::new(ReleasePmixResponse {}))
}

async fn cancel_job(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

run_attempt is not authoritative at cancellation sink

Failure scenario: A delayed cancel for attempt N arrives after attempt N+1 is tracked. The handler signals by job_id first; the requested attempt is consulted only later when releasing an uncommitted reservation. Heartbeat reclaim explicitly sends attempt 0.

Impact: A stale controller operation can terminate the current job. Suspend, step cancel, PMIx rollback, and heartbeat reconciliation have the same identity gap.

Action: Carry {job_id, run_attempt, step_id} through every control RPC and compare exact identity immediately before every signal, release, or cleanup. Do not let zero target a nonzero current attempt.

Regression test: Delay explicit cancel, graceful cancel, suspend, step cancel, PMIx release, and heartbeat reclaim for N until N+1 is live; N+1 must remain untouched.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The ordering is exactly as you describe — the signal goes out first and the attempt is consulted only for the reservation release below it — and suspend, step cancel, and PMIx release carry no attempt identity at all.

Same scoping call as the dispatch-generation thread: the pre-PR handler had the identical signal-then-check shape, and this PR narrows the gap by making the reservation release epoch-guarded where it previously wasn't. Closing it properly means appending attempt fields to three more request messages and re-checking identity at each sink, which I'd rather review as its own change than attach to this one.

Deferring the cancel/suspend/step-cancel/PMIx identity work.

One thing from this thread I did fix here, because it was actively misleading rather than merely incomplete: the comment at the heartbeat-reclaim site claimed signal 0 is a no-op on an unknown id. It isn't — signal 0 takes the graceful-cancel branch, which is a full SIGTERM/SIGKILL teardown. That path isn't behind the supervisor flag, so it's live today. The comment now says what the code does and what the narrowing actually relies on.

}
}

// `start_job_impl` advances the run epoch after the allocation has been

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Dispatch and Completion generations are not durable in Raft

Failure scenario: Agents launch before a replicated dispatch intent exists. A leader can die after launch acknowledgement and the next leader can dispatch the same attempt to another cohort. Separately, JobNodeComplete omits the attempt, so its pre-proposal check cannot be repeated when Raft applies it.

Impact: The same attempt may execute twice, or an old completion may finalize and free resources belonging to a newer attempt.

Action: Add replicated Dispatching and Fencing states keyed by exact attempt/cohort. Make start metadata atomic, and append a defaulted attempt to the completion WAL operation with equality checked during state-machine apply.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The facts here are right: nothing durable names the attempt and cohort before the launch RPC goes out, and JobNodeComplete carries no attempt, so the pre-proposal check can't be repeated when Raft applies it.

Scoping, though — this is pre-existing on main rather than something this PR introduces. wal.rs isn't in this diff at all, and the scheduler_loop change here moves in the direction you're asking for: it threads run_attempt into the cancel RPCs, which previously carried none.

Record-before-dispatch is the cutover the allocation-reconciliation work is scoped around, and it's a breaking enough change to need its own review unit rather than being folded into a PR about the supervisor.

Deferring both halves to that work. The cheaper half — appending a defaulted run_attempt to JobNodeComplete and re-checking it at apply time — is small enough to land on its own first, and I'd rather do that than bundle it here. The replicated Dispatching/Fencing states are the larger design change.

Comment thread crates/spurd/src/stepd.rs
Arc::new(Mutex::new(HashMap::new()))
#[derive(Clone)]
pub struct StepdRecoveryCleanup {
running: RunningJobs,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The model can not grow to Slurm's one-supervisor-per-step design

Failure scenario: Runtime maps and recovery RPCs are keyed by job/attempt but not consistently by step; a future numbered Stepd would overwrite the batch/extern slot.

Impact: The stated follow-up stack requires another protocol/state-model refactor and cannot support simultaneous batch, extern, interactive, and numbered steps.

Action: Introduce one RuntimeId { job_id, run_attempt, step_id } now and use it in maps, notifications, probes, journals, and cleanup.

Regression test: Track batch, extern, and two numbered steps concurrently and independently signal/recover each one.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Agreed on the direction — the design calls for one supervisor per running step and this ships one per job, which is why steps are declared as follow-up work.

I've taken the half that gets more expensive to defer: both new RPCs now carry step_id (appended tags, nothing renumbered), and the session-directory parser keeps the component it was already reading and discarding. Doing that now avoids baking in a permanent "0 means batch" ambiguity once these RPCs have deployed callers.

Re-keying the runtime maps I'd rather land with the numbered-step work. The mechanical part is genuinely small — the on-disk layout and the local socket protocol already carry step_id end to end — but the semantics aren't: fence_stepd_recovery requeues the whole job, so per-step recovery needs a policy for "one step's supervisor died but the batch step is fine", and displacement has to decide whether a new attempt displaces all of a job's steps or only the matching key. Those are design calls, not plumbing, and I don't want to answer them implicitly here.

Deferring the map re-keying and per-step fencing to the follow-up. Leaving this thread open to track it.

Adds ReportStepdRecovery and ProbeStepd RPCs plus run_attempt fields to
AgentCancelJobRequest/RegisterJobAllocationRequest, laying the wire
groundwork for a per-step supervisor process (spurd's stepd, named
after and modeled on Slurm's slurmstepd) that survives an spurd
restart.

AuthConfig gains jwt_key_file so a node's recovery reports can be
signed without inlining a secret in spur.conf. cons_tres.rs tags
allocations with run_attempt so a redispatch can never be confused
with a stale reservation from a prior attempt. spur-core::step gains
default_step_id() for a stepd descriptor deserialized from a payload
written before the step_id field existed.
Introduces stepd.rs: a detached, systemd-managed supervisor process
(spawned as `spurd __stepd <state_dir> <job_id> <run_attempt>
<launch.json>`) that owns a job's process tree independently of the
spurd that launched it, so a plain batch, container, or GPU job keeps
running across an spurd restart or upgrade.

Named and modeled after Slurm's slurmstepd: each stepd supervises
exactly one step's process tree, identified by (job_id, run_attempt,
step_id). The batch script is step STEP_BATCH; a pure allocation with
no script (interactive/srun-only) is STEP_EXTERN, matching Slurm's
extern-step convention for tracking an allocation's lifetime without a
process of its own. An interactive PTY session or a numbered srun step
gets its own separate stepd process in follow-up work, not a second
process hosted inside this one — so the coordination methods
(signal/begin_teardown/poll_completion) act on a single tracked
process and need no change when that work lands.

Covers the durable core: a local control-socket protocol for
query/signal/teardown, a private per-attempt descriptor + obligation
log so a restarted spurd can rediscover and safely reconnect to a live
stepd (or fence a dead/superseded one), and crash-safe completion
replay so an exit that lands while spurd is down is never lost.
Adds the `__stepd` subcommand dispatch, a startup scan that discovers
descriptors left behind by a prior spurd instance, cgroup reaping for
stale (supervisor-dead) stepds, and a background loop that reports
recovered/stale stepds to the controller and retries on failure. Runs
before job launch is even possible, so a restarted spurd reconciles
its held stepds before anything else touches them.
When SPUR_STEPD=1, launch_job and register_job_allocation spawn the
job under a stepd supervisor instead of tracking it as a direct spurd
child, and cancel_job/heartbeat/monitor learn to query and signal
through the supervisor's control socket. Adoption on startup, liveness
watchdog fencing, and crash-safe completion replay plug into the same
tracking path an ordinary job uses today.

exec (logical steps) and interactive attach against a stepd-backed job
are explicitly rejected with Unimplemented for now — their launch path
is a follow-up PR — rather than silently falling back to a legacy path
that has no real pid to act on. Direct `--mpi=pmix` batch launch also
stays on the legacy path until PMIx-in-stepd lands.
Adds the ReportStepdRecovery handler: a restarted spurd's node identity
is verified against a signed node token, the job's expected node
cohort is polled for a live stepd, and an incomplete or unconfirmed
cohort is fenced (requeued) rather than left to run unsupervised
indefinitely. Node-token minting now happens whenever a signing key is
configured, not only under token-based admission.

cancel_job_on_nodes/send_cancel_to_nodes thread a run_attempt through
every cancel path so a cancel can never land on a node that has since
moved on to a newer attempt of the same job id. preempt_job splits into
a thin wrapper and a preempt_job_with_provenance that also serves the
controller-initiated fencing path (no triggering job to record).
NodeReporter grows report_stepd_recovery and requires a signed node
token before registering with SPUR_STEPD=1 set — a stepd's recovery
report has to prove which node it's calling from, which an
unauthenticated agent can't do.
…affolding

VirtualAgent, MockController, and StubController need every trait
method implemented; wires the new ProbeStepd/ReportStepdRecovery RPCs
into each (k8s-backed jobs never run under a native stepd, so the
agent stub always reports inactive) and fixes
cons_tres::allocate_for_job call sites for its new run_attempt
parameter.
validate_admission minted a signed node token for whatever hostname a
RegisterAgent caller asserted, regardless of admission mode. Since
register_node lets a node (re-)register under an existing name with no
identity proof, this let any RPC caller mint a valid token for a
victim node's hostname and use it to falsely report stepd recovery,
force-fencing (preempting/requeuing) a healthy, unrelated job. Minting
now requires the caller to already hold a valid join token under
[admission] mode = "token" — the one registration path that actually
proves an identity claim.

Also fixes the general RPC auth key to honor jwt_key_file (it was
reading the raw jwt_key field, so a jwt_key_file-only deployment got a
working node-identity path but a broken/empty-keyed AuthLayer), tights
the stepd-recovery cohort tracker so two concurrent reports past grace
can't both fence the same run, replaces a sleep-based race test with a
deterministic wait, logs a swallowed config error instead of silently
treating it as "no key", and adds the recovery-RPC test coverage that
was previously entirely missing (cohort expiry, hostname mismatch,
node-token verification failure, and the admission-mode fix itself).
stepd was previously a hidden `spurd __stepd` subcommand of the same
binary — same dependency surface, same trust boundary as the node
agent that terminates network RPCs and runs cluster-admin (k0s) work.
A per-job supervisor that sits beside a job's execution for its whole
lifetime and a network-facing control-plane daemon have different
threat models and failure domains; sharing one binary means a
compromise of either inherits the other's full capability set, and
nothing stops a future change from reaching across the (until now
merely conventional) protocol boundary between them.

spurd now ships as a lib+bin pair: agent_server/executor/container/
stepd and friends move to a `spurd` library crate, and the new
`spur-stepd` crate depends on it for the launch/supervision primitives
only — no gRPC/proto/mesh/k0s dependency, no controller-RPC surface.
The resulting `spurstepd` binary is ~6x smaller than spurd (3.6MB vs
21.9MB), a rough proxy for how much control-plane surface it no longer
links. spurd invokes it as a genuinely separate executable (resolved
next to its own binary path, falling back to PATH), not via argv/
current_exe() self-relaunch.

Scope: process/crate boundary only. No wire-format change — the
Unix-socket protocol and StepdLaunchSpec/StepdDescriptor JSON shapes
are unchanged, just now crossing an explicit crate dependency edge
instead of an implicit intra-binary one. Privilege-dropping (spurstepd
setuid-ing to the job's uid, matching Slurm's slurmstepd) is a
deliberate follow-up, not bundled here — flagged as `!` since the
`__stepd` subcommand invocation this replaces was never part of a
release.
…rimary

spurstepd was launched through systemd-run (unit under
spur-runtime.slice) and torn down via systemctl stop — a real
dependency on systemd/dbus that a per-job supervisor shouldn't need,
and one that already surfaced as an "interactive authentication
required" failure under a non-root spurd. Replaced with the same
double-fork+setsid+exec pattern slurmd uses for slurmstepd: spurd
forks, the intermediate child forks again and setsid()s in the
grandchild before exec, and the intermediate child exits immediately
so the grandchild reparents to init. The grandchild's real pid comes
back over a pipe, since Command::spawn's return value is only the
immediately-exiting intermediate child. stop_stepd_unit's systemctl
call is replaced by a direct SIGTERM against that pid, gated on the
same pid+start-ticks liveness check the crash watchdog already uses.

Also promotes cgroup membership to the primary kill-target set for
every signal, not just a SIGKILL backstop: cgroup.procs is read and
each tracked pid signaled directly, falling back to process-group
signaling only when there's no cgroup to read. cgroup.kill (SIGKILL-
only) remains as an atomic backstop. This reaches descendants that
detached from the job's process group (e.g. via setsid) the way a
plain process-group signal can't.

Verified live on a 2-node deployment: spurstepd's session/pgid
structure now matches slurmstepd's exactly (session leader, reparented
to init, job subtree in its own process group within the same
session); a job survives an spurd restart and spurd reconnects to the
still-running stepd; scancel tears the job down through the new
cgroup-primary path end to end.
A job with no submitted work_dir (e.g. salloc/interactive sessions,
which never set one) had both the controller and the agent eagerly
default it to the literal, shared /tmp before the job ever reached the
agent's own resolution. Since a job's output path is a relative
pattern (spur-<job_id>.out) anchored to work_dir, two jobs whose IDs
happen to collide with another job's, another user's, or an old run's
leftover file at that same path in /tmp fail with a permission error
the submitter has no way to diagnose — surfaced only as a generic
dispatch-confirmation failure.

Removed both premature defaults and added resolve_effective_work_dir
in executor.rs as the single place that resolves a missing or
uncreatable work_dir, falling back to a per-job scratch directory
(scoped by job_id and run_attempt) instead of the flat /tmp root, and
only falling all the way back to bare /tmp if even that per-job
directory can't be created. Also fixes a related sharp edge:
create_dir_all("") is a silent no-op success, so an empty work_dir has
to be checked explicitly rather than trusted to fail directory
creation on its own.

Verified live: planted a stale, unrelated-user-owned file at the exact
path an affected job would have collided on, then repeated the
previously-always-failing sequence — both runs now succeed, each
writing to its own per-job scratch directory without touching the old
file.
…Ok(0) bug

Three fixes from a review pass over the stepd binary split / native
spawn / cgroup-signal work:

- AGENTS.md and spurd's lib.rs doc comment claimed spurstepd "links a
  deliberately minimal dependency set" / builds "without linking
  spurd's controller-RPC, k0s, or mesh-networking surface." cargo tree
  shows spur-stepd still depends on the whole spurd lib crate,
  including tonic/tower/spur-net — the isolation actually achieved is
  that spurstepd's process never runs that code, not that it isn't
  linked. Corrected both to describe a runtime, not link-time,
  boundary; genuinely splitting the runtime-only surface into its own
  crate is a larger follow-up, not done here.

- spawn_stepd_process reported the grandchild's pid over the pipe
  before attempting execv, then closed it — so a failed exec (missing
  binary, bad perms) was silently indistinguishable from "still
  starting up," and the caller only found out after the full ~10s
  readiness timeout. The write end is now marked FD_CLOEXEC instead of
  closed: a successful exec closes it for free with no byte sent; a
  failed exec falls through to write an explicit error byte before
  exiting. The parent does a short bounded poll for that byte right
  after reading the pid, so a missing/bad executable now fails in
  milliseconds instead of ~10 seconds.

- Stepd::signal()/begin_teardown() treated cgroup_signal returning
  Ok(0) (an empty cgroup.procs) the same as a delivered signal, with
  no fallback. move_to_cgroup writes the job's pid into cgroup.procs
  only after the process is already spawned, so a signal landing in
  that window silently no-opped instead of reaching the process group
  the way it always did before cgroup-primary signaling was added.
  Both now fall back to process-group signaling whenever cgroup_signal
  reports zero pids, not just on a read error.

Added tests for all three behavioral fixes, each confirmed to fail
against the prior code: a signal reaching a pid only visible via
cgroup.procs (proving the primary path, not just the SIGKILL
backstop), the Ok(0) fallback regression, a fast-failing spawn against
a missing executable, and stop_stepd_process against a genuinely
stale (exited, non-zero) pid rather than the degenerate pid-0 case.

Verified live: the happy path is unaffected (job launch, restart
survival, and cancel all still work end to end), and a deliberately
missing spurstepd binary now fails the launch in milliseconds instead
of stalling out the readiness timeout.
…t dir

parse_session_dir_name split a corrupted descriptor's directory name
on the first '.', but session_dir names directories
<job_id>.<run_attempt>.<step_id> (three components) — so run_attempt
always picked up the trailing ".<step_id>" and failed to parse,
meaning corrupted stepd sessions were silently dropped instead of
being fenced on the next spurd startup. Fixed to read the first two
components and require a third to be present.

The agent completion-notification socket directory was only chmod'd
to 0700 when freshly created; a pre-existing directory (leftover,
operator-configured to a shared location, or a symlink) was trusted
without verification. Routed it through the same create-or-verify
private-directory helper already used for the stepd runtime root and
session directories, generalized to create missing parents too.

Verified live: a corrupted descriptor's directory name is now
correctly parsed and fenced on restart instead of being ignored, and
spurd now refuses to start against a pre-existing, non-private notify
socket directory instead of silently trusting it.
The agent notify-socket directory defaults to spurctld's own
state_dir when SPUR_STEPD_STATE_DIR is unset — a directory
spurctld creates and owns, not spurd. Applying the strict
create-or-verify-private-directory check there meant spurd refused
to start whenever that shared directory's permissions (however
legitimate for spurctld's own purposes) weren't exactly 0700 owned
by spurd's own uid, which is the common case since spurctld doesn't
create it that way.

Moved the check to the runtime/ subdirectory spurd/spurstepd already
create and own exclusively, leaving the shared parent untouched (only
ensured to exist via create_dir_all, same as before). Added a test
locking in that a loosely-permissioned shared parent is tolerated as
long as the exclusive leaf directory is private.

Verified live: reproduced spurctld and spurd sharing one state_dir
with spurctld creating it at its own default (non-strict) permissions
and no SPUR_STEPD_STATE_DIR override, matching the e2e harness's
setup exactly — spurd now starts and dispatches jobs normally instead
of refusing to start.
… state

spurd derived its stepd runtime directory only from SPUR_STEPD_STATE_DIR or
the controller's configured state_dir, with no CLI flag of its own. Add
--state-dir (mirroring spurctld's flag; SPUR_STEPD_STATE_DIR still works via
env fallback) so co-located agents can be given non-colliding state
directories explicitly, and wire the e2e harness to pass it the same way it
already does for spurctld.
…rom review

The agent bound its completion socket under runtime/ while the supervisor
connected at the bare state dir, so every direct push failed and completion
silently degraded to the replay loop, losing the epilog-failure drain with it.
Both sides now derive the path from one accessor.

A job that exited before the agent claimed its descriptor could lose its
completion entirely: cleanup removed the recorded exit, and the unmatched
notification arm acknowledged a completion nobody had reported. The accept
loop is now biased so a queued agent connection is served first, cleanup keeps
any session holding a recorded exit, and an unreported completion defers.

ExitObserved is written immediately after the wait instead of after cleanup,
the epilog, and SPANK, and the epilog result is recorded separately so a late
report still drains. The journal reader tolerates one torn final line rather
than failing the whole file, which previously stopped the agent from starting.
OOM detection was missing from the supervisor path, reporting OOM kills as bare
SIGKILL.

Node identity is minted whenever an identity key is configured rather than only
under token admission, so restart survival works on a default cluster instead of
refusing to start; the error names the real precondition. Both new stepd RPCs
carry step_id, and the session-dir parser keeps the component it already reads.

The documented unit needs KillMode=process or systemd kills supervisors on
restart and the next agent reclaims the orphaned job.
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.

4 participants