fix(spurctld): report wall-time expiry as TIMEOUT

[maybeharshit]

Motivation

A job killed by its wall-time limit landed in FAILED with ExitCode=0:15, and whether it ever reached TIMEOUT depended on how it reacted to SIGTERM — the well-behaved job was the one misreported.

The time-limit watchdog tracked which jobs it had signalled in two in-memory maps (warned_jobs, warn_times) owned by the scheduler task, so the SIGTERM decision was invisible to the completion path. A job that exits promptly on that SIGTERM is finalized from its agent's completion report, where Job::derived_completion sees only "signaled" and returns Failed. Only a job that ignored SIGTERM survived to the kill phase, which sets Timeout explicitly.

Fixes #546.

Technical Details

The expiry is now recorded on the job itself as time_limit_signaled_at, written through a new JobTimeLimitSignaled WAL entry before the SIGTERM goes out.

Replicating it is not optional: completions are finalized inside the Raft apply, so a non-replicated flag could not be consulted there without replicas disagreeing on the outcome. Deriving the grace period from the same field then falls out for free — it removes both in-memory maps and makes the window survive a leadership change, which previously restarted it from scratch.

Two smaller pieces follow from that:

• Job::completion_verdict decides the final state and reason together, so they cannot drift. Completing -> Timeout was missing from the state machine, since every completion routes through Completing (Slurm's JOB_TIMEOUT | JOB_COMPLETING).
• PendingReason::TimeLimit mirrors Slurm's FAIL_TIMEOUT, replacing the RaisedSignal:15(Terminated) that would otherwise sit incongruously next to JobState=TIMEOUT.

Design choices worth flagging:

[1] The agent's real exit status survives the verdict rather than being normalized, so a job that ended on SIGTERM stays distinguishable from one the watchdog had to SIGKILL. The forced kill-phase path keeps its synthetic -1, because it is the backstop that guarantees finalization when no agent report is coming.

[2] An OOM kill still outranks the time limit. It is direct kernel evidence of a distinct failure the user has to act on, whereas the timeout is the controller's own reason for signalling.

[3] Correct attribution routes these jobs into the existing Timeout requeue path, which --requeue jobs dying on SIGTERM previously skipped. The issue names this as affected behaviour. time_limit_signaled_at is cleared in clear_run_state_for_requeue, so a requeued run starts with a fresh budget instead of being marked timed out the moment it ends.

Test Plan

• Unit tests in spur-core for the verdict itself: SIGTERM death, a trapped-SIGTERM clean exit, an unrelated signal death, OOM precedence, and the Completing -> Timeout transition.
• Unit tests in spurctld driving the real WAL apply: completion after a signal reports Timeout/TimeLimit, a signal arriving after the run ended is a no-op, the forced kill path carries the reason, and requeue clears the marker.
• New tests/native_host/e2e/test_time_limit.py covering the issue's two scripts plus a control job that fails inside its limit.
• Clippy and the full workspace suite.
• Live two-node LXD cluster (Ubuntu 24.04), --time=00:00:20.

Test Result

Clippy clean; full workspace suite passes.

On the LXD cluster:

job	script	result
tl-plain	sleep 300	JobState=TIMEOUT Reason=TimeLimit ExitCode=143:0
tl-trap	trap "" TERM; sleep 300	JobState=TIMEOUT Reason=TimeLimit ExitCode=-1:0
tl-fastfail	exit 3	JobState=FAILED Reason=NonZeroExitCode ExitCode=3:0

The controller log shows tl-plain reaching TIMEOUT with no grace period expired line, confirming it was finalized from the agent's completion report — the exact path that used to yield FAILED. tl-trap was force-killed 30s after its SIGTERM, as before.

ExitCode for tl-plain is 143:0 rather than the issue's 0:15 because this environment's bash exits 128+15 instead of dying from the signal. That is a property of the shell, not of the fix, so the e2e test only rules out the synthetic -1 rather than asserting an exact status.

A --requeue job at the same limit was requeued repeatedly with from=TIMEOUT, each run logging a full elapsed_secs=27 against limit_secs=20, which confirms both the new requeue routing and that the marker is cleared between runs.

Submission Checklist

• Look over the contributing guidelines at https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests.

Made with Cursor

[yansun1996]

Hi @maybeharshit , you marked this PR as draft, are you still working on more changes ?

[shiv-tyagi]

Posted comments. PTAL.

[shiv-tyagi]

LGTM.

[shiv-tyagi]

@yansun1996 Please review and merge.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 96.52510% with 9 lines in your changes missing coverage. Please review.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #554      +/-   ##
==========================================
+ Coverage   74.73%   76.10%   +1.37%     
==========================================
  Files         165      166       +1     
  Lines       60486    63172    +2686     
==========================================
+ Hits        45203    48074    +2871     
+ Misses      15283    15098     -185     

🚀 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 fixes Spur’s wall-time expiry attribution so jobs that exceed their time limit reliably finalize as TIMEOUT (with Reason=TimeLimit) even when they exit promptly on the watchdog’s initial SIGTERM, by persisting a replicated “time-limit signaled” marker that the completion path can consult during Raft apply.

Changes:

• Add a replicated time_limit_signaled_at marker on Job, written via a new WalOperation::JobTimeLimitSignaled before SIGTERM is sent, and cleared on requeue.
• Centralize final completion attribution in Job::completion_verdict so final JobState and PendingReason are derived together (including Completing -> Timeout).
• Add unit coverage (WAL + apply-path tests) and a new native-host E2E test suite for both “dies on SIGTERM” and “ignores SIGTERM” timeout paths.

Reviewed changes

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

Show a summary per file

File	Description
tests/native_host/e2e/test_time_limit.py	Adds E2E coverage ensuring both SIGTERM and SIGKILL timeout paths report TIMEOUT and that in-limit failures remain FAILED.
crates/spurctld/src/scheduler_loop.rs	Removes in-memory timeout tracking and keys watchdog behavior off replicated time_limit_signaled_at.
crates/spurctld/src/cluster.rs	Introduces signal_time_limit() (WAL-backed), applies the new WAL entry, and ensures timeout reason attribution on forced completion.
crates/spur-core/src/wal.rs	Adds JobTimeLimitSignaled WAL operation and a round-trip serialization test.
crates/spur-core/src/job.rs	Adds PendingReason::TimeLimit, the time_limit_signaled_at field, and completion_verdict() plus state-machine transition coverage.
crates/spur-tests/src/t55_format.rs	Extends formatting tests to include PendingReason::TimeLimit.

---

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