ci: stop reporting green for tests that never ran, and fire one CI run per commit - #97
Merged
Conversation
_dispatch_slurm and run_e2e_disagg returned 0 when srun was absent, or when the host python3 could not import pytest/pytest-asyncio/httpx. The tier then propagated rc=0 all the way to RESULT: PASS having executed nothing. This is not hypothetical. On 2026-08-06 a runner fleet whose python3 lacked pytest turned all three e2e-disag legs green in 7 seconds each -- a required check passing without running a single test. The run was only caught because the mixed and engine tiers failed hard for an unrelated reason and took the whole run red with them. Both guards now fail and name the cause. The dependency message prints the python3 that was actually consulted plus its real ImportError: the old "missing host deps" line is true of every python3 on the box and does not say which one was asked, which is precisely what sent that triage the wrong way. The skip behaviour still exists for a dev box that genuinely has no SLURM, but it has to be asked for: INFERA_E2E_ALLOW_SKIP=1. Even then it is not silent -- skipped tiers are recorded and the last line reads "RESULT: PASS (SKIPPED: e2e disagg)" instead of a clean pass. Verified on five paths: no srun (disag), no srun (engine, the _dispatch_slurm guard), missing deps, missing deps with the opt-out, and a healthy host where neither guard fires and dispatch still reaches "mode=resv". No workflow or script parses the RESULT string, so changing the last line is safe. Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: xiaobochen-amd <xiaobo.chen@amd.com>
The reclaim step existed to survive a transient Spur controller error, and was
defeated by exactly that. `ids=$(squeue ... 2>/dev/null)` leaves a failed query
looking identical to an empty one, so the loop read the error as "nothing to
reclaim", printed that line, and broke out on its first pass.
On 2026-08-06 four jobs were sitting in the queue when their reclaim ran and
survived it. They held 3 of the reservation's 4 nodes until their 1h50m limit
expired; every PR behind them failed waiting for a node that was not coming.
Two smaller holes helped: the loop only spanned ~25s, and `scancel ... || true`
kept the step green whether or not anything was actually cancelled.
The five inline copies are now one script, called with the job-name prefix and
suffix each job already computes:
- retry on a non-zero squeue, printing its stderr instead of discarding it
- exit 0 only after a *successful* query comes back empty
- on timeout (RECLAIM_TIMEOUT, default 120s) emit ::error:: and dump the queue
- never exit 0 unconfirmed: a leaked job squats a reserved GPU node, which is
otherwise only discovered later, as a reservation that looks idle and is not
The match keeps the old prefix+suffix shape, so infera-ci-hold-* (a -N2 holder,
two nodes) and the spill and wipe jobs are still caught; a length guard stops a
suffix longer than the job name from matching through substr.
Verified with a stubbed squeue/scancel: clean queue exits 0 immediately; a
permanently failing squeue retries and exits 1 where the old code reported
success; of three queued jobs only the one matching both ends is cancelled, and
the run exits 0 once a follow-up query confirms it is gone.
Co-authored-by: Cursor <cursoragent@cursor.com>
Signed-off-by: xiaobochen-amd <xiaobo.chen@amd.com>
_reservation_nodes already returned non-zero when scontrol failed -- pipefail
saw to that -- but all four callers reached for it as `[ -z "$(...)" ]`, which
keeps the output and throws the status away. An unreachable controller and a
reservation that is genuinely gone both arrive as an empty string, and the
callers acted on the more destructive reading of the two.
On 2026-08-06 a runner without SPUR_CONTROLLER_ADDR made every scontrol call
answer "failed to connect to spurctld". The dispatcher read that as the pool
having disappeared, printed mode=resv-gone->open, and sent the tier to the open
partition; _e2e_preflight said "reservation does not exist (gone or expired)",
which is where triage went first and lost time.
Spur keeps the two distinguishable, which is what makes this fixable: it
ignores the NAME argument and dumps every reservation, so asking for a name
that is not there still exits 0 and the awk simply matches nothing. Only an
unreachable controller exits non-zero. Verified on the live cluster.
Each caller now separates them:
_reservation_free -3 for a failed query, still -1 for a missing one
_dispatch_slurm -3 joins the "keep the reservation" branch; only a
query that answered may drop it
_candidate_nodes yields nothing so the caller keeps waiting, instead of
falling through to the open partition's idle list and
handing the PD pair unreserved nodes
run_e2e_disagg unsets INFERA_E2E_RESERVATION only on a confirmed
absence; a failed query warns and keeps it
_e2e_preflight says which of the two actually happened
_reservation_nodes captures scontrol's output before parsing it. The status was
already correct, but callers now depend on it, and under pipefail any stage of
that pipeline can set it -- one `grep` added later would report every query as
failed and leave the run clinging to a reservation that really had been deleted.
Verified against the live cluster: an existing reservation resolves 4 nodes and
_reservation_free returns 4; a name that does not exist exits 0 empty and
returns -1; with SPUR_CONTROLLER_ADDR unset the query exits non-zero and returns
-3, and the dispatcher prints mode=resv where it used to print
mode=resv-gone->open. A healthy host is unchanged.
Co-authored-by: Cursor <cursoragent@cursor.com>
Signed-off-by: xiaobochen-amd <xiaobo.chen@amd.com>
Three sites left over from the reservation and reclaim fixes, all reading a query that did not answer as a result that came back empty. _cancel_dispatched. Two of them, and this one runs from the INT/TERM trap, so it fires exactly when the scheduler is most likely to be unhappy. Listing the run's jobs returned as though there were nothing to cancel; confirming the cancel read a failed squeue as "gone". It now says what happened and hands off to the workflow's reclaim step, and only an answered query may confirm. That confirmation leans on Spur behaviour worth writing down: a job that is gone still exits 0 with no output, so empty-and-successful is real proof. Stock SLURM errors on an invalid id instead, so there the loop never confirms and the new warning is a false alarm -- noted in the comment, and the workflow's reclaim step covers both. _spill_inflight returned 0 on a failed count, which reads as "nothing borrowed" and authorises another spill -- borrowing harder from the open partition at the one moment the scheduler is already struggling. It returns non-zero now and the caller queues on the reservation instead. _hold_pair collapsed every outcome into 1, so a refused sbatch, a hold that never started, and genuinely losing the pair to another engine all surfaced as "lost the node-hold race N times". That sent triage looking for contention when the scheduler was the problem -- it is why the review could not use that line as evidence of a full pool. 2 now means SLURM never placed the hold, 1 stays for a real race, and the caller words the two differently. The caller uses `_hold_pair ...; hold_rc=$?` rather than `if ! _hold_pair`, where $? is the negation's status and never the function's. Verified, and commented so it does not come back. Verified with a stubbed squeue/sbatch: a failing squeue makes _cancel_dispatched report and return non-zero instead of claiming success, and makes _spill_inflight return non-zero so no spill is authorised; a gone job (exit 0, empty) confirms the cancel; a permanently refused sbatch returns 2. A healthy host still reaches mode=resv. Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: xiaobochen-amd <xiaobo.chen@amd.com>
xiaobochen-amd
requested review from
JohnQinAMD,
jiejingzhangamd and
limou102
as code owners
August 6, 2026 16:10
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens SLURM-related CI cleanup and e2e dispatch behavior by distinguishing “scheduler/query failed” from “queue is empty”, and by centralizing SLURM job reclamation into a reusable script to reduce duplicated workflow logic.
Changes:
- Improve
tests/run_tests.shSLURM dispatch/cleanup logic to avoid treating failed scheduler queries as “nothing to cancel / reservation missing”, and to fail (or explicitly skip) tiers that cannot actually run. - Replace duplicated workflow reclaim loops with a shared
.github/scripts/reclaim_slurm_jobs.shhelper. - Add a new SLURM reclaim helper script that retries until the queue confirms the jobs are gone (or times out with actionable logs).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
tests/run_tests.sh |
Makes SLURM query failures explicit (vs empty), adds skip-or-fail gating, and improves cancellation confirmation logic. |
.github/workflows/ci.yml |
Replaces inline SLURM reclaim loops with the shared reclaim script. |
.github/workflows/release.yml |
Replaces inline SLURM reclaim loops with the shared reclaim script for build/overlay jobs. |
.github/scripts/reclaim_slurm_jobs.sh |
New reusable reclaim helper that retries on controller errors and confirms cleanup. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+110
to
+114
| # A failed lookup is not an empty queue. Say so and leave it to ci.yml's | ||
| # reclaim step rather than returning as if there were nothing to cancel. | ||
| if ! jids=$(squeue -h -u "$(id -un)" -o '%i %j' 2>/dev/null \ | ||
| | awk -v suf="$suf" '$2 ~ /^infera-ci-/ && substr($2, length($2)-length(suf)+1)==suf {print $1}' \ | ||
| | tr '\n' ' ') | ||
| | tr '\n' ' '); then |
Review feedback on #97: _cancel_dispatched still had squeue's stderr going to /dev/null, so the new message read "squeue failed" and stopped there. Knowing a query failed without knowing why means reproducing a scheduler blink to debug one, which is the thing these commits set out to remove. It was also inconsistent with the same series: reclaim_slurm_jobs.sh already keeps stderr, and the review that motivated all of this said to stop discarding it. Three more sites had the same gap, so all four are fixed together: _cancel_dispatched both the job listing and the post-scancel confirmation _reservation_nodes forwards scontrol's own words; callers could only say "cannot reach the scheduler", which is not actionable _spill_inflight same, for the borrowed-node count Merging stderr into the pipeline would not have worked for the listing: awk filters to job names, so the error text is exactly what gets dropped. Each of these captures the command's output first and parses the copy -- the shape _reservation_nodes already used, now applied consistently. Verified with a stubbed squeue/scontrol: a refused connection now appears in full at every one of the four sites, including the "could not confirm the cancel" line, which previously ended without a reason. A healthy host still reaches mode=resv. Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: xiaobochen-amd <xiaobo.chen@amd.com>
push fired on every branch alongside pull_request, so a push to a branch with an open PR started two runs for the same commit. To keep the work from doubling, the jobs were split by hand between them -- unit, rust and torch-cpu on the push event, GPU and DCO on the PR -- and three of them carried a same-repo/fork test to enforce the split. That split has costs the split itself cannot pay back. The PR's own check list shows "skipped" for unit, rust and torch-cpu: their real result lives in a push run the PR does not link to. Both runs report check runs named `unit` on one commit, which GitHub's own docs call ambiguous and advise against, and the PR run's skip resolves in seconds while the push run's real test takes a minute -- long enough for auto-merge to act on the wrong one. And `changes`, `e2e_gate` and `lint` simply ran twice. Scope push to main and let the PR event carry everything it gates. The fork conditions go with it, since there is nothing left to deduplicate. A branch with no PR now runs nothing; a draft PR gives the same feedback and puts it on the PR. e2e_gate loses the tree comparison. It existed to avoid re-running e2e on main when the merge produced the same content the PR had tested; the team's call is that main re-tests unconditionally, because two PRs can each pass alone and fail together and only the merged result shows it. What is left is the event, plus a draft check so iterating on a draft does not cost a GPU run per push -- ready_for_review is in `types` so marking it ready picks the tiers back up. pre_check goes too. skip-duplicate-actions matched on tree alone, ignoring the event, and it was there for the double-run this commit removes. Left in, it would have read the post-merge run as a duplicate of the PR that produced it and skipped the very re-test main exists to provide. It was also how `engine` could silently not run: a successful branch-push run (where engine never runs, e2e_gate being false off main) made the later PR look like a duplicate. Its `actions: read` permission goes with it. One pre-existing bug had to be fixed here rather than left: `changes` has no diff base on workflow_dispatch, so it fell back to HEAD~1 and would skip the whole run off a docs-only last commit. That was survivable while a branch push started CI. It is not now that the button is the only way to force a run, so a manual run is treated as a code change. Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: xiaobochen-amd <xiaobo.chen@amd.com>
github.head_ref is the source branch name with no repository attached, so two pull requests from different forks that both use a branch called `main` or `fix-ci` -- neither an unusual name -- land in one concurrency group and cancel each other. The symptom is somebody else's push killing your run, with nothing in either run pointing at the other. The PR number is unique across forks. Pushes to main have no pull_request in the payload and fall through to the ref, which is what they used before. Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: xiaobochen-amd <xiaobo.chen@amd.com>
dco.yml triggers itself on any pull request, and ci.yml called it again as a job so the GPU tiers could gate on the sign-off through `needs:`. Every PR therefore carried two DCO check runs under two names, `dco` and `dco / dco`. The gate it bought is redundant. lint already holds those tiers back, so a PR with a missing sign-off is stopped well before a GPU node is touched; the second DCO run only added a check name to keep straight in branch protection. Drop the call. dco.yml's own trigger is unchanged, so every PR into any branch is still checked -- once. Verified against the repository rulesets first: no required check is named `dco / dco`, so removing it cannot leave a PR waiting on a check that will never report. Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: xiaobochen-amd <xiaobo.chen@amd.com>
Eighteen of the twenty-one action references are pinned to a commit with the version in a trailing comment. Three were left on a moving tag: checkout@v4 and setup-python@v5 in unit-torch-cpu, upload-artifact@v4 in release.yml. A tag can be repointed by the action's owner, or by whoever takes over the account, and the workflow would run different code with nothing changed in this repository. The two in unit-torch-cpu take the same pins the rest of the file already uses, which also brings that job up from checkout v4 and setup-python v5 to the v7.0.0 and v6.3.0 every other job runs. Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: xiaobochen-amd <xiaobo.chen@amd.com>
Two ways run_engine reported PASS having tested nothing. `for f in $(find "$INFERA_TEST_SCOPE" ...)` iterates zero times when the scope matches nothing, and rc stays 0. find does report a missing path -- it exits 1 -- but nothing was in a position to see it: the loop reads only the output, and the `| sort` had already replaced find's status with its own. engine_tier hard-codes tests/engine/vllm and tests/engine/sglang, so one rename is all it takes for the tier to go green having run nothing. Capture the list before sorting and fail on either a find that could not read the scope or a scope with no test files -- the first case is not hypothetical padding: an unreadable subdirectory makes find list the files it could reach and still exit 1, which the old code would have run as if it were the whole suite. pytest's exit 5, "collected nothing", was written down as "whole file skipped -- not a failure". Three files carry a module-level importorskip: the two test_disagg_allow_tcp_args.py on vllm and sglang, and test_kvd_fp8_passthrough.py on torch. Each guard names a module that the image running that scope ships, so a 5 there means the image is broken -- precisely the moment the tier must go red. It also has to be labelled rather than merely counted, because pytest words a module-level skip as "1 skipped": left alone, the per-file summary would repeat that verbatim while the tier failed for a reason the log never states. (A fourth module-level guard, on test_kv_metadata_block_size.py, already failed correctly: importing infera.engine.vllm.__main__ raises something other than ImportError, which pytest reports as a collection error, exit 2.) Neither path has ever fired. No dispatch log under the shared CI log directory contains the old branch's "no tests ran" line, and all four complete engine runs report 21 files and 21 passed, the three guarded files among them. That is also what makes this safe: no file currently depends on exit 5 being forgiven. Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: xiaobochen-amd <xiaobo.chen@amd.com>
xiaobochen-amd
changed the title
weilei0120
pushed a commit
that referenced
this pull request
Aug 10, 2026
Review feedback on #97: _cancel_dispatched still had squeue's stderr going to /dev/null, so the new message read "squeue failed" and stopped there. Knowing a query failed without knowing why means reproducing a scheduler blink to debug one, which is the thing these commits set out to remove. It was also inconsistent with the same series: reclaim_slurm_jobs.sh already keeps stderr, and the review that motivated all of this said to stop discarding it. Three more sites had the same gap, so all four are fixed together: _cancel_dispatched both the job listing and the post-scancel confirmation _reservation_nodes forwards scontrol's own words; callers could only say "cannot reach the scheduler", which is not actionable _spill_inflight same, for the borrowed-node count Merging stderr into the pipeline would not have worked for the listing: awk filters to job names, so the error text is exactly what gets dropped. Each of these captures the command's output first and parses the copy -- the shape _reservation_nodes already used, now applied consistently. Verified with a stubbed squeue/scontrol: a refused connection now appears in full at every one of the four sites, including the "could not confirm the cancel" line, which previously ended without a reason. A healthy host still reaches mode=resv. Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: xiaobochen-amd <xiaobo.chen@amd.com>
weilei0120
pushed a commit
that referenced
this pull request
Aug 10, 2026
ci: stop reporting green for tests that never ran, and fire one CI run per commit Signed-off-by: leiwei12 <lei.wei@amd.com>
weilei0120
pushed a commit
that referenced
this pull request
Aug 10, 2026
Review feedback on #97: _cancel_dispatched still had squeue's stderr going to /dev/null, so the new message read "squeue failed" and stopped there. Knowing a query failed without knowing why means reproducing a scheduler blink to debug one, which is the thing these commits set out to remove. It was also inconsistent with the same series: reclaim_slurm_jobs.sh already keeps stderr, and the review that motivated all of this said to stop discarding it. Three more sites had the same gap, so all four are fixed together: _cancel_dispatched both the job listing and the post-scancel confirmation _reservation_nodes forwards scontrol's own words; callers could only say "cannot reach the scheduler", which is not actionable _spill_inflight same, for the borrowed-node count Merging stderr into the pipeline would not have worked for the listing: awk filters to job names, so the error text is exactly what gets dropped. Each of these captures the command's output first and parses the copy -- the shape _reservation_nodes already used, now applied consistently. Verified with a stubbed squeue/scontrol: a refused connection now appears in full at every one of the four sites, including the "could not confirm the cancel" line, which previously ended without a reason. A healthy host still reaches mode=resv. Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: xiaobochen-amd <xiaobo.chen@amd.com>
weilei0120
pushed a commit
that referenced
this pull request
Aug 10, 2026
ci: stop reporting green for tests that never ran, and fire one CI run per commit Signed-off-by: leiwei12 <lei.wei@amd.com> Signed-off-by: Zhang, Jiejing <jiejing.zhang@amd.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
On 2026-08-06 four of the five self-hosted runners went down after two login
nodes rebooted. Bringing them back exposed a second, worse problem: the CI had
been reporting green for tests that never executed. The three
e2e-disaglegson PR #95 each turned green in 7 seconds having run no test at all, and the run
was only caught because unrelated tiers failed hard and took it red with them.
Digging in, that turned out not to be one bug but a family of six, all the same
shape: a command that failed and a command that found nothing look identical,
and the code took the optimistic reading. A
squeuethat errored read as anempty queue. A
scontrolthat could not reach the controller read as a deletedreservation. A
findover a missing directory read as a directory with notests. In every case the run continued as if there were nothing to do.
The second half of this PR is unrelated to that: the workflow fired
pushonevery branch alongside
pull_request, so one commit started two runs, and thejobs had to be split by hand between them. That left the PR's own check list
showing "skipped" for tests whose real result lived in a run the PR does not
link to.
Type of change
Changes
Tests that could not run no longer report PASS
_dispatch_slurmandrun_e2e_disaggreturned 0 whensrunwas absent orthe host
python3could not import pytest. Both now fail and name the cause;the skip still exists for a dev box without SLURM but must be asked for with
INFERA_E2E_ALLOW_SKIP=1, and even then the last line readsRESULT: PASS (SKIPPED: ...).enginetier iterated zero times over a scope that matched nothing, andwrote pytest's exit 5 ("collected nothing") down as "not a failure". Both are
failures now. Exit 5 in the engine image means the image is broken, which is
exactly when the tier must go red.
A failed scheduler query is no longer read as an empty result
_reservation_nodesas[ -z "$(...)" ], whichkeeps the output and throws the status away. Spur keeps the two cases
distinguishable — a missing reservation exits 0 empty, only an unreachable
controller exits non-zero — so each caller now separates them.
_cancel_dispatched,_spill_inflightand_hold_pairhad the same defect._hold_pairin particular collapsed a refusedsbatchinto "lost thenode-hold race", which sent triage looking for contention when the scheduler
was the problem.
stderrinstead of discarding it, so the log sayswhy a query failed rather than only that it did.
Reclaim keeps trying instead of exiting on the first error
.github/scripts/reclaim_slurm_jobs.sh. It retries on a non-zerosqueue,exits 0 only after a successful query comes back empty, and on timeout emits
::error::and dumps the queue. A leaked job squats a reserved GPU node andis otherwise only found later, as a reservation that looks idle and is not.
One event per state of the code
pushis scoped tomain; the pull request event carries everything itgates. The three same-repo/fork conditions go with it, and so does
pre_check— left in,skip-duplicate-actionswould have read thepost-merge run as a duplicate of the PR that produced it and skipped the very
re-test
mainexists to provide.ready_for_reviewis intypesso markingone ready picks the GPU tiers back up.
head_refcarries norepository, so two forks that both use a branch called
mainwould canceleach other's runs.
repository rulesets first: no required check is named
dco / dco.pinned.
Checklist: