fix(spurctld): enforce partition walltime limits and validate time config

[shiv-tyagi]

What this fixes

Partition MaxTime was never enforced, and invalid time strings in config silently became UNLIMITED. Jobs could exceed a partition's wall-time cap, and a typo like max_time = "1h" quietly removed the cap instead of applying it. Refs SPUR-122.

This is Group 1 of the fix (submit/schedule-time validation and defaulting). Group 2 (interactive allocation reaping via InactiveLimit) is a follow-up.

Approach

• enforce_part_limits (NO/ALL/ANY, default NO): gates submit-time rejection of over-limit jobs, mirroring Slurm's EnforcePartLimits. ALL rejects unless the job fits every requested partition; ANY rejects only when it fits none. Default NO keeps the prior admit-and-pend behavior. Deserialization is case-insensitive and accepts Slurm's YES alias.
• Specific pending reason: time violations now report PartitionTimeLimit instead of the generic PartitionConfig.
• Loud config validation: SlurmConfig::validate() rejects malformed partition max_time/default_time at load rather than silently dropping the cap.
• Default-time fallback (opt-in): apply_default_time_limit follows DefaultTime -> MaxTime -> scheduler.default_time_limit_minutes. That cluster fallback now defaults to 0 (disabled), so on upgrade a -t-less job on an unlimited partition stays unbounded. Sites opt into reclamation by setting it > 0.
• Suffixed durations: time strings now accept 1h, 90m, 1h40m, 2d12h, 30s in addition to Slurm grammar, across config, CLI -t, scontrol, and REST. Slurm grammar is tried first (disjoint, no reinterpretation of existing values) and output still renders HH:MM:SS.

Backward compatibility

Non-breaking and backward compatible by default. With an unchanged config (enforce_part_limits = NO, default_time_limit_minutes = 0) no submit-time rejections are added and no job gets a wall-time it wouldn't have had before. No persisted-state, proto, or CLI/REST-output changes.

One intended behavior change worth a release note: a deployed max_time = "1h" was previously silently UNLIMITED; it now parses as a 60-minute cap (honoring intent). A genuinely invalid string (e.g. "1 hour") now fails loudly at startup instead of removing the cap.

Testing

• Unit tests for enforcement (ALL/ANY accept/reject incl. no-partition-fits), the default-time fallback branches, config validation, and suffixed-duration parsing (plus updated existing tests). Full suite green; clippy clean.
• E2E: TestEnforcePartLimits in tests/native_host/e2e/test_partitions.py.
• Bare-metal (4 nodes): confirmed ALL rejects -t 2h/-t 90m on a 1h partition and accepts -t 45m/-t 1h; default_time = "10m" applies to -t-less jobs; default_time_limit_minutes = 0 leaves unlimited-partition jobs unbounded while = 60 bounds them; max_time = "1h" loads as a 60-min cap; and "1 hour" fails startup loudly.

Review follow-ups

Two rounds of review addressed in later commits:

• Submit-time enforcement runs only for a user-supplied -t (never an auto-filled default), and the -t-less default derives from the minimum resolved limit across all requested partitions. Error messages render at second precision.
• Enforcement considers only Up partitions (matching the pending-reason path): a Down partition's MaxTime no longer rejects a job that fits an Up one, and an all-inactive request pends as PartitionInactive rather than reporting "exceeds MaxTime". This is a minor, intended behavior change under ALL/ANY.
• submit_job's config/partitions snapshot is threaded through all validators, so the whole submit path sees one consistent view under a concurrent reconfigure().
• validate() rejects a partition whose finite default_time exceeds its finite max_time (which would otherwise auto-fill a -t-less job past its own cap and pend it forever).

Follow-up #604 tracks moving to Slurm-style per-partition default resolution at schedule time.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 98.47793% with 10 lines in your changes missing coverage. Please review.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #593      +/-   ##
==========================================
+ Coverage   76.24%   77.26%   +1.01%     
==========================================
  Files         166      171       +5     
  Lines       65418    70448    +5030     
==========================================
+ Hits        49876    54426    +4550     
- Misses      15542    16022     +480     

🚀 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 closes a Slurm-compat gap in spurctld by enforcing partition wall-time (MaxTime) semantics more correctly, adding submit-time enforcement gating (enforce_part_limits), and making time parsing/validation fail loudly rather than silently degrading to UNLIMITED.

Changes:

• Add scheduler.enforce_part_limits (NO/ALL/ANY) and enforce partition MaxTime at submit when enabled; improve pending reasons to use PartitionTimeLimit for wall-time violations.
• Make config parsing reject malformed partition max_time / default_time values at load; extend time parsing to accept suffixed durations (1h, 90m, 30s, etc.).
• Change cluster-wide defaulting behavior so scheduler.default_time_limit_minutes defaults to 0 (disabled) and only bounds -t-less jobs when explicitly enabled.

Reviewed changes

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

Show a summary per file

File	Description
tests/native_host/e2e/test_partitions.py	Adds E2E coverage for submit-time rejection/acceptance when EnforcePartLimits=ALL.
docs/admin-guide/configuration.rst	Documents enforce_part_limits, updates the default for default_time_limit_minutes, and notes suffixed duration support.
crates/spurctld/src/cluster.rs	Implements submit-time MaxTime enforcement, refines pending-reason selection, and updates default-time fallback behavior.
crates/spur-tests/src/t52_config.rs	Updates config-default expectations for default_time_limit_minutes (now 0).
crates/spur-core/src/config.rs	Introduces EnforcePartLimits, adds loud partition time validation, and extends time parsing to accept suffixed durations.

---

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

[shiv-tyagi]

Fixed in 1e08eff. Added format_time_seconds and render both the requested time and the cap at second precision. A 0:01:30 request against a 00:01:00 cap now reports 00:01:30 exceeds MaxTime 00:01:00 instead of the contradictory 00:01:00.

[biluriuday]

Important (fix before merge)

1. Auto default-time + EnforcePartLimits=ALL can reject an under-specified job. apply_default_time_limit (crates/spurctld/src/cluster.rs:5689) resolves against only the first requested partition and, when default_time_limit_minutes > 0, assigns that partition's MaxTime (cluster.rs:5716). But ALL enforcement then checks every partition. A -t-less job to big,small (MaxTime 120 vs 60) gets auto-assigned 120 from big, then rejected against small — despite the user never requesting any wall time.

Fix: for a multi-partition request, derive the default from the minimum MaxTime across requested partitions, or skip ALL enforcement for an auto-derived (non-user-supplied) limit. Add a test pinning the chosen semantics.

Minor
2. Self-contradictory error message. The over-limit message builds the requested string at minute granularity (cluster.rs:597, format_time) even though enforcement is second-precision — so 10m30s vs a 00:10:00 cap correctly rejects but prints "00:10:00 exceeds MaxTime 00:10:00". Render request/cap at second precision.

3. Docs undersell the fallback's reach. docs/admin-guide/configuration.rst says default_time_limit_minutes bounds "otherwise-unlimited jobs," but enabling it also makes -t-less jobs on finite-MaxTime partitions (no DefaultTime) default to that MaxTime (cluster.rs:5716). Add a sentence so operators aren't surprised.

Release note
default_time_limit_minutes is now actually enforced (was inert). Untouched configs are unchanged, but any site that had set it expecting an effect will now see new behavior — worth a one-line note.

Bottom line: issue 1 is the only thing that can produce a genuinely surprising rejection and should be resolved (plus a test) before merge; 2 and 3 are cheap UX/doc wins

[yansun1996]

Thanks for closing this gap — the enforcement/validation/duration-parsing work looks solid and stays backward compatible by default. One correctness issue around multi-partition auto-default-time + EnforcePartLimits=ALL should be fixed before merge (see inline); the rest below are smaller polish items and test-coverage suggestions.

A couple things without a good inline anchor:

• partition_block's doc comment ("limits rejected by every Up alternative return PartitionConfig") is now stale — the body can also return the more specific PartitionTimeLimit.
• Several new doc comments run 3+ lines (EnforcePartLimits enum doc, parse_partition_time/parse_time_minutes/parse_suffix_duration_seconds, apply_default_time_limit) — worth trimming to a line or two each.
• The EnforcePartLimits::No arm inside the satisfied match in validate_partition_time_limit is unreachable (the function already returns early for No) — harmless, just dead code.
• Agree with the existing Copilot comment on the error message's minute- vs. second-precision mismatch.

Missing test coverage worth adding:

• The multi-partition + ALL + auto-default-time scenario below.
• Case-insensitive parsing of enforce_part_limits ("all", "Yes", "any") and the reject-on-garbage-value path — the Deserialize impl looks correct but nothing exercises it.
• A partition with both default_time and a smaller max_time (plus a nonzero cluster default), to pin that DefaultTime always wins in the fallback chain.

Also noting as a follow-up, not a blocker: spec.time_limit can be negative (no non-negativity check on the client-supplied duration), which would trivially satisfy partition_time_allows regardless of mode — pre-existing, adjacent to this PR's changes.

[shiv-tyagi]

Correction on the default_time_limit_minutes point (the 60 -> 0 default change).

default_time_limit_minutes was never enforced before this PR. On main it appears only as the struct field, its Default of 60, and a test asserting that default. No submit or scheduler path reads it, so a -t-less job on a partition with no DefaultTime was already unbounded.

So there is no lost 60-minute cap on upgrade:

• A site that never set it: unbounded before (the 60 was inert), unbounded after (0 disables the fallback). No change.
• This PR wires the knob up for the first time. The default moves 60 -> 0 specifically so -t-less jobs stay unbounded exactly as they do today.
• The only new behavior is for a site that had set it > 0 expecting an effect it never had.

I kept the default at 0 for that reason. Restoring 60 would itself be the behavior change, since it would start capping -t-less jobs that run unbounded today. Added a release note to the config docs spelling this out.

[shiv-tyagi]

@yansun1996 I have addressed your comments. Can you please take a look?

[yansun1996]

Re-reviewed after the latest fixes — the 5 prior points are all genuinely addressed (verified against the code, not just the commit message). Two smaller things turned up in this pass, left as inline comments: an Up-state filtering gap in validate_partition_time_limit, and the single-snapshot fix being incomplete. Also a doc-validation gap and a couple of doc-comment nits.

[yansun1996]

Re-reviewed at d4d9875 — all points from the last round check out (Up-partition filtering, single submit-time snapshot, default>max validation). Nice work tightening this up. Two small non-blocking follow-ups if you want to fold them in: a few new doc comments are still 3+ lines (worth trimming to 1-2), and the admin doc could mention that a malformed max_time/default_time now fails config load at startup instead of silently becoming UNLIMITED. Approving.

[biluriuday]

New "enforce only against Up partitions" scoping diverges from Slurm

underEnforcePartLimits=ALL, filtering to onlyUppartitions means a DOWN/DRAINED partition's (possibly stricter)MaxTimeis silently ignored. Slurm'sEnforcePartLimitsvalidates against the requested partitions regardless of current state. Also, ifnorequested partition is currently Up, the job is admitted completely unchecked (requested.is_empty() => Ok).

enforcement outcome becomes time-dependent on transient node/partition state — the samesbatchis accepted or rejected depending on whether a partition happens to be down at submit. That's surprising and hard to diagnose.