fix(spurctld): reject unusable --nodelist/--exclude at submission (follow-up to #679)

[SumonAMD]

Summary

Follow-up to #679, addressing both review items @sajmera-pensando raised:

1. In-loop hostlist guard was off-by-one. The pre-loop guard rejects when the
   total would exceed MAX_HOSTLIST_SIZE, but the in-loop backstop ran after
   the push and used >, so results could transiently hold MAX_HOSTLIST_SIZE + 1
   before erroring. The two guards disagreed.
2. Over-cap / malformed --nodelist / --exclude silently hung the job. An
   unexpandable pattern fell back to a literal name (via
   node_match::expand_hostlist_or_split) that matches no node, so the job sat in
   the queue forever with no error. Slurm rejects this at submission.

Changes

• hostlist.rs — move the in-loop size check before the push and compare with
  >=, so results never grows past the cap. Also drop the count field from
  TooLarge: the in-loop path can't know the true request size without expanding
  (the OOM we're preventing), so it reported a misleading MAX + 1. Both guards now
  report the limit only — hostlist too large: exceeds maximum {max} hosts.
• server.rs — validate --nodelist and --exclude in proto_to_job_spec,
  returning InvalidArgument with the offending pattern and reason before the spec
  reaches the scheduler.

Note: the suggested one-character > → >= could not be applied in place. In the
old position (after the push), >= rejects a list of exactly MAX_HOSTLIST_SIZE,
which the pre-loop guard deliberately allows and expand_allows_cap_boundary asserts.
Moving the check before the push is what makes >= both consistent and
boundary-correct.

Test plan

• expand_allows_cap_boundary — a list of exactly 1,000,000 hosts still expands
• expand_rejects_nested_product_over_cap, expand_rejects_recursive_overshoot_past_cap — nested patterns that multiply past the cap are rejected
• New proto_to_job_spec_rejects_unexpandable_node_patterns — over-cap nodelist and malformed exclude are rejected, a valid pattern still passes
• cargo fmt --check clean, cargo clippy -D warnings clean
• Full spur-core + spurctld suites pass single-threaded (1,536 tests, 0 failures)

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #722   +/-   ##
=======================================
  Coverage   79.36%   79.36%           
=======================================
  Files         180      180           
  Lines       82240    82264   +24     
=======================================
+ Hits        65266    65287   +21     
- Misses      16974    16977    +3     

🚀 New features to boost your workflow:

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

[spraveenio]

Review — recall pass

Strengths / verification: The core fix is correct. Moving the in-loop size guard before the push and switching to >= keeps results from ever transiently holding MAX_HOSTLIST_SIZE + 1, and I verified it still admits an exactly-1,000,000-host list (expand_allows_cap_boundary) while rejecting the nested-product and recursive-overshoot cases. Dropping the count field from TooLarge is internal-only — it isn't part of persisted Raft/WAL state, the proto, config, or any CLI/REST surface, and no test asserts on the old {count} hosts exceeds maximum {max} string — so it's not a breaking change. proto_to_job_spec is called only from submit_job (not on update or Raft replay), so the new validation can't reject a previously-accepted spec on upgrade/replay.

Three items below.

---

1. (question) Is the stricter rejection of an unexpandable / over-cap --exclude intentional?

proto_to_job_spec now runs the same submission-time rejection over both --nodelist and --exclude:

for (flag, pattern) in [("nodelist", &spec.nodelist), ("exclude", &spec.exclude)] {
    if !pattern.is_empty() {
        spur_core::hostlist::expand(pattern).map_err(|e| {
            Status::invalid_argument(format!("invalid --{flag} '{pattern}': {e}"))
        })?;
    }
}

The PR's justification — an unexpandable pattern falls back to a literal that matches no node, so the job "sat in the queue forever with no error" — holds cleanly for --nodelist: no match means never schedulable. But --exclude is different. Today an unexpandable/over-cap exclude falls back (via expand_hostlist_or_split) to a literal token that matches no node, so it excludes nothing and the job still schedules and runs. Rejecting it at submission is therefore a behavior change beyond the two items the PR states it's fixing: a script that passes a malformed or oversized --exclude and currently runs would now get InvalidArgument.

Could you confirm whether folding --exclude into the same hard rejection is intended? If it is, it'd be worth calling out explicitly (it's a stricter contract than "fix the hang"); if not, the loop could validate nodelist only.

---

2. New user-visible submission-time rejection is undocumented

docs/user-guide/submitting-jobs.rst (the --nodelist / --exclude rows, ~lines 178 and 181) still describe these flags with no mention of validation. This PR changes their behavior from "accepted, then hangs / silently ignored" to "rejected at submission with InvalidArgument" when the pattern is malformed or exceeds 1,000,000 hosts.

AGENTS.md: "User-facing changes ship with docs. Whenever you change ... user-visible behavior, check docs/ and update the pages that describe it." Please add a note to those rows (or a short paragraph) documenting that an invalid or over-cap pattern is rejected at submission, so users migrating Slurm scripts get a documented signal.

---

3. Double materialization of the hostlist on the submit hot path

The validation calls hostlist::expand(pattern) purely for its Err, discarding the Ok(Vec<String>). For a legitimate large --nodelist (e.g. node[0-999999]), that allocates a ~1M-element Vec<String> and immediately drops it; the scheduler then re-expands the same string via expand_hostlist_or_split. So a ~14-byte pattern forces two ~15 MB expansions per submit, and this validation runs before the MAX_JOB_SPEC_SIZE gate in cluster submit can reject anything.

A validate-only path that bounds the count without materializing the full list (the cap arithmetic in expand_single already computes the range size before pushing) would avoid the throwaway allocation. Not a blocker, but worth considering given it's on the submission path with attacker-influenced input.