From 22c918a1841b76353622f8529ace72b96053b202 Mon Sep 17 00:00:00 2001 From: xiaobochen-amd Date: Thu, 6 Aug 2026 15:27:32 +0000 Subject: [PATCH 01/10] fix(tests): a tier that cannot run must not report PASS _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 Signed-off-by: xiaobochen-amd --- tests/run_tests.sh | 49 +++++++++++++++++++++++++++++++++++++++------- 1 file changed, 42 insertions(+), 7 deletions(-) diff --git a/tests/run_tests.sh b/tests/run_tests.sh index 52da8d3d..50e118bf 100755 --- a/tests/run_tests.sh +++ b/tests/run_tests.sh @@ -195,6 +195,24 @@ QOS_WAIT="${INFERA_E2E_QOS_WAIT:-30}" # than a single-node srun, and giving up early only churns the pair-hold race. HOLD_WAIT="${INFERA_E2E_HOLD_WAIT:-60}" +# A tier that could not run is not a tier that passed: returning 0 here is how a +# runner whose python3 lacked pytest turned every e2e-disag leg green in 7s. Fail +# and name the cause; a dev box that really has no SLURM opts out explicitly. +# $1=label $2=what is wrong $3=how to fix it +_SKIPPED_TIERS="" +_skip_or_fail() { + local label="$1" why="$2" fix="$3" + if [ "${INFERA_E2E_ALLOW_SKIP:-}" = 1 ]; then + _SKIPPED_TIERS="${_SKIPPED_TIERS:+$_SKIPPED_TIERS, }$label" + echo "[$label] SKIPPED (INFERA_E2E_ALLOW_SKIP=1): $why" >&2 + return 0 + fi + echo "[$label] FATAL: $why" >&2 + echo "[$label] fix: $fix" >&2 + echo "[$label] (or INFERA_E2E_ALLOW_SKIP=1 to skip this tier instead of failing)" >&2 + return 1 +} + _have_slurm() { command -v srun >/dev/null 2>&1; } # The nodes reservation $1 covers, one per line ('' if it is gone/expired). # Spur ignores the NAME arg and dumps all reservations; match the exact block. @@ -358,8 +376,10 @@ _watch_job() { _dispatch_slurm() { local label="$1"; shift if ! _have_slurm; then - echo "[$label] WARNING: no SLURM (srun) — skipping" >&2 - return 0 + _skip_or_fail "$label" \ + "no SLURM: srun is not on PATH, so this tier cannot be dispatched to a GPU node" \ + "expose the SLURM client on this host, or run where docker + >=8 AMD GPUs are present" + return $? fi # srun's own client banners/errors (job id, "running on ", ...). local out="$SCRATCH/.dispatch-$label.out" @@ -603,11 +623,20 @@ run_e2e_disagg() { local engines=("$@") echo "===== e2e PD-disaggregated (cross-node, 2 nodes): ${engines[*]} =====" if ! _have_slurm; then - echo "[e2e disagg] WARNING: no SLURM (srun) — skipping PD-disaggregated tests" >&2 - return 0 + _skip_or_fail "e2e disagg" \ + "no SLURM: srun is not on PATH, so the PD-disaggregated tests cannot run" \ + "expose the SLURM client on this host" + return $? + fi + # Name the interpreter actually consulted and quote its ImportError: "missing + # host deps" is true of every python3 on the box, and sent the last triage wrong. + local deps_err + if ! deps_err=$(python3 -c "import pytest, pytest_asyncio, httpx" 2>&1); then + _skip_or_fail "e2e disagg" \ + "the disagg orchestrator runs pytest on THIS host, and $(command -v python3 || echo 'python3 (not on PATH)') cannot import its deps: ${deps_err##*$'\n'}" \ + "pip install pytest pytest-asyncio httpx" + return $? fi - python3 -c "import pytest, pytest_asyncio, httpx" >/dev/null 2>&1 \ - || { echo "[e2e disagg] WARNING: missing host deps (pytest/pytest-asyncio/httpx) — skipping" >&2; return 0; } if [ -n "$SHARED_LOG_DIR" ]; then exec > >(stdbuf -oL tee -a "$SHARED_LOG_DIR/dispatch-disag-$$.log") 2>&1 @@ -777,5 +806,11 @@ if [ -d "$E2E_LOG_DIR" ]; then ls -1 "$E2E_LOG_DIR"/*.log 2>/dev/null | sed 's|^| |' || true fi -[ "$rc" -eq 0 ] && echo "RESULT: PASS" || echo "RESULT: FAIL" +if [ "$rc" -ne 0 ]; then + echo "RESULT: FAIL" +elif [ -n "$_SKIPPED_TIERS" ]; then + echo "RESULT: PASS (SKIPPED: $_SKIPPED_TIERS)" +else + echo "RESULT: PASS" +fi exit "$rc" From d8d68c31ac0d49dddc32e1bbda29d0df51dda58f Mon Sep 17 00:00:00 2001 From: xiaobochen-amd Date: Thu, 6 Aug 2026 15:38:42 +0000 Subject: [PATCH 02/10] =?UTF-8?q?ci:=20a=20failed=20squeue=20is=20not=20an?= =?UTF-8?q?=20empty=20queue=20=E2=80=94=20reclaim=20has=20to=20keep=20tryi?= =?UTF-8?q?ng?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: xiaobochen-amd --- .github/scripts/reclaim_slurm_jobs.sh | 47 +++++++++++++++++++++++++++ .github/workflows/ci.yml | 36 +++----------------- .github/workflows/release.yml | 20 ++---------- 3 files changed, 54 insertions(+), 49 deletions(-) create mode 100755 .github/scripts/reclaim_slurm_jobs.sh diff --git a/.github/scripts/reclaim_slurm_jobs.sh b/.github/scripts/reclaim_slurm_jobs.sh new file mode 100755 index 00000000..8f07181c --- /dev/null +++ b/.github/scripts/reclaim_slurm_jobs.sh @@ -0,0 +1,47 @@ +#!/usr/bin/env bash +# Copyright (c) 2026, Advanced Micro Devices, Inc. All rights reserved. +# SPDX-License-Identifier: MIT +# Cancel the SLURM jobs one CI job dispatched, and keep at it until the queue +# confirms they are gone. +# reclaim_slurm_jobs.sh +# +# The five inline copies this replaces discarded squeue's stderr, which made a +# FAILED query indistinguishable from an EMPTY one: a transient controller error +# read as "nothing to reclaim" and broke the retry loop on its first pass -- that +# error being the only reason the loop existed. On 2026-08-06 four jobs sitting in +# the queue when reclaim ran survived it and held 3 of the reservation's 4 nodes +# until their time limit expired. +set -uo pipefail + +prefix="${1:?usage: $0 }" +suffix="${2:?usage: $0 }" +budget="${RECLAIM_TIMEOUT:-120}" +interval="${RECLAIM_INTERVAL:-5}" +me="$(id -un)" +deadline=$(( SECONDS + budget )) + +echo "reclaiming SLURM jobs named ${prefix}*${suffix}" + +while :; do + # Exit code, not emptiness, is what separates an unreachable controller from a + # clean queue; stderr is folded in so the CI log names the failure. + if ! queue=$(squeue -h -u "$me" -o '%i %j' 2>&1); then + echo "squeue failed, retrying (this is NOT an empty queue): $queue" + else + ids=$(printf '%s\n' "$queue" | awk -v p="$prefix" -v s="$suffix" ' + index($2, p) == 1 && length($2) >= length(s) && + substr($2, length($2) - length(s) + 1) == s { print $1 }') + [ -z "$ids" ] && { echo "confirmed: no ${prefix}*${suffix} jobs left"; exit 0; } + echo "cancelling: $ids" + scancel $ids 2>&1 || echo "scancel returned non-zero, retrying" + fi + if [ "$SECONDS" -ge "$deadline" ]; then + # A leaked job holds a reserved GPU node until its time limit, so this has to + # be findable in the log rather than inferred later from a reservation that + # looks idle and is not. + echo "::error::could not confirm reclaim of ${prefix}*${suffix} within ${budget}s; check for leaked SLURM jobs" + squeue -u "$me" -o '%.10i %.44j %.2t %.10M %R' 2>&1 || true + exit 1 + fi + sleep "$interval" +done diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9c820eb8..4cfb4f28 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -220,15 +220,7 @@ jobs: run: exec bash tests/run_tests.sh engine - name: reclaim this job's SLURM jobs (on cancel/failure) if: always() && (cancelled() || failure()) - run: | - suf="-${{ github.run_id }}-engine" - for i in 1 2 3 4 5; do - ids=$(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}') - [ -z "$ids" ] && { echo "no (more) SLURM jobs to reclaim"; break; } - echo "reclaiming SLURM job(s): $ids (try $i)"; scancel $ids 2>&1 || true - sleep 5 - done + run: .github/scripts/reclaim_slurm_jobs.sh infera-ci- "-${{ github.run_id }}-engine" # Full PD-mixed e2e (per engine, parallel). When it runs is e2e_gate's call: # every PR into main, plus a push that lands untested code on main. @@ -272,16 +264,7 @@ jobs: run: exec bash tests/run_tests.sh e2e ${{ matrix.engine }} mixed - name: reclaim this job's SLURM jobs (on cancel/failure) if: always() && (cancelled() || failure()) - run: | - # Retry: a single scancel can hit a transient Spur controller error. - suf="-${{ github.run_id }}-${{ matrix.engine }}" - for i in 1 2 3 4 5; do - ids=$(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}') - [ -z "$ids" ] && { echo "no (more) SLURM jobs to reclaim"; break; } - echo "reclaiming SLURM job(s): $ids (try $i)"; scancel $ids 2>&1 || true - sleep 5 - done + run: .github/scripts/reclaim_slurm_jobs.sh infera-ci- "-${{ github.run_id }}-${{ matrix.engine }}" e2e-disag: # Gates mirror e2e-mixed, `!cancelled()` included: `always()` would keep this @@ -328,18 +311,9 @@ jobs: run: exec bash tests/run_tests.sh e2e ${{ matrix.engine }} disag - name: reclaim this job's SLURM jobs (on cancel/failure) if: always() && (cancelled() || failure()) - run: | - # Catches the infera-ci-hold-* pair holder too: it is a -N2 --gres=gpu:8 - # batch job, so a leaked one keeps TWO reserved nodes out of the pool. - # Retry: a single scancel can hit a transient Spur controller error. - suf="-${{ github.run_id }}-${{ matrix.engine }}-disag" - for i in 1 2 3 4 5; do - ids=$(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}') - [ -z "$ids" ] && { echo "no (more) SLURM jobs to reclaim"; break; } - echo "reclaiming SLURM job(s): $ids (try $i)"; scancel $ids 2>&1 || true - sleep 5 - done + # Catches the infera-ci-hold-* pair holder too: it is a -N2 --gres=gpu:8 + # batch job, so a leaked one keeps TWO reserved nodes out of the pool. + run: .github/scripts/reclaim_slurm_jobs.sh infera-ci- "-${{ github.run_id }}-${{ matrix.engine }}-disag" unit-torch-cpu: needs: [lint, pre_check, changes] diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index c456dd15..79d1af61 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -166,15 +166,7 @@ jobs: - name: reclaim this job's SLURM job (on cancel/failure) if: always() && (cancelled() || failure()) - run: | - suf="-${{ github.run_id }}-${{ matrix.engine }}" - for i in 1 2 3 4 5; do - ids=$(squeue -h -u "$(id -un)" -o '%i %j' 2>/dev/null \ - | awk -v suf="$suf" '$2 ~ /^infera-build-/ && substr($2, length($2)-length(suf)+1)==suf {print $1}') - [ -z "$ids" ] && { echo "no (more) SLURM jobs to reclaim"; break; } - echo "reclaiming SLURM job(s): $ids (try $i)"; scancel $ids 2>&1 || true - sleep 5 - done + run: .github/scripts/reclaim_slurm_jobs.sh infera-build- "-${{ github.run_id }}-${{ matrix.engine }}" # The base-agnostic overlay payload (deploy/overlay/). Unlike the engine # images this one is not independent: it builds its Python trees inside the @@ -236,15 +228,7 @@ jobs: - name: reclaim this job's SLURM job (on cancel/failure) if: always() && (cancelled() || failure()) - run: | - suf="-${{ github.run_id }}-overlay" - for i in 1 2 3 4 5; do - ids=$(squeue -h -u "$(id -un)" -o '%i %j' 2>/dev/null \ - | awk -v suf="$suf" '$2 ~ /^infera-build-/ && substr($2, length($2)-length(suf)+1)==suf {print $1}') - [ -z "$ids" ] && { echo "no (more) SLURM jobs to reclaim"; break; } - echo "reclaiming SLURM job(s): $ids (try $i)"; scancel $ids 2>&1 || true - sleep 5 - done + run: .github/scripts/reclaim_slurm_jobs.sh infera-build- "-${{ github.run_id }}-overlay" # Build the /manual Sphinx site alongside the images. Always uploads the HTML # as a workflow artifact; on a tag (release) it also attaches a tarball to the From 0eb5eebf9e1dcbb7780f96705eb7de01aa5d1059 Mon Sep 17 00:00:00 2001 From: xiaobochen-amd Date: Thu, 6 Aug 2026 15:54:33 +0000 Subject: [PATCH 03/10] fix(tests): an unreachable controller is not a deleted reservation _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 Signed-off-by: xiaobochen-amd --- tests/run_tests.sh | 55 ++++++++++++++++++++++++++++++++-------------- 1 file changed, 39 insertions(+), 16 deletions(-) diff --git a/tests/run_tests.sh b/tests/run_tests.sh index 50e118bf..9e98900b 100755 --- a/tests/run_tests.sh +++ b/tests/run_tests.sh @@ -214,10 +214,16 @@ _skip_or_fail() { } _have_slurm() { command -v srun >/dev/null 2>&1; } -# The nodes reservation $1 covers, one per line ('' if it is gone/expired). -# Spur ignores the NAME arg and dumps all reservations; match the exact block. +# The nodes reservation $1 covers, one per line. Non-zero means the QUERY failed; +# exit 0 with no output means the reservation genuinely is not there. Spur keeps +# the two separable: it ignores the NAME arg and dumps every reservation, so a +# missing name still exits 0 and the awk below simply matches nothing. +# Capture before parsing: under pipefail a later stage's status would otherwise +# masquerade as a failed query, and callers now act on that distinction. _reservation_nodes() { - scontrol show reservation "$1" 2>/dev/null | awk -v r="ReservationName=$1" ' + local out + out=$(scontrol show reservation "$1" 2>/dev/null) || return 1 + printf '%s\n' "$out" | awk -v r="ReservationName=$1" ' BEGIN{RS="";FS="\n"} $1==r { for(i=1;i<=NF;i++) if($i ~ /Nodes=/){ n=$i; sub(/.*Nodes=/,"",n); sub(/[[:space:]].*/,"",n); print n; exit } }' \ | tr ',' '\n' | sed '/^$/d' @@ -235,7 +241,11 @@ _node_free() { # partition's idle ones. _candidate_nodes() { local n nodes="" - [ -n "${INFERA_E2E_RESERVATION:-}" ] && nodes="$(_reservation_nodes "$INFERA_E2E_RESERVATION")" + if [ -n "${INFERA_E2E_RESERVATION:-}" ]; then + # Query failed: offer nothing and let the caller keep waiting. Falling + # through would hand the PD pair unreserved nodes off the open partition. + nodes=$(_reservation_nodes "$INFERA_E2E_RESERVATION") || return 0 + fi if [ -z "$nodes" ]; then sinfo -h -N -p "$SLURM_PART" -t idle -o '%n' 2>/dev/null | awk 'NF && !seen[$0]++' return @@ -328,11 +338,11 @@ _amd_gpu_count() { _local_eligible() { [ "$(_amd_gpu_count)" -ge 8 ] && command -v docker >/dev/null 2>&1; } # Spill helper (Spur has no srun --immediate): free count, -1 if the reservation -# is gone/expired, -2 if scontrol is unavailable. +# is gone/expired, -2 if scontrol is unavailable, -3 if the query itself failed. _reservation_free() { local rname="$1" nodes n free=0 command -v scontrol >/dev/null 2>&1 || { echo -2; return; } - nodes=$(_reservation_nodes "$rname") + nodes=$(_reservation_nodes "$rname") || { echo -3; return; } [ -n "$nodes" ] || { echo -1; return; } for n in $nodes; do _node_free "$n" && free=$((free + 1)) @@ -413,10 +423,12 @@ _dispatch_slurm() { rfree=$(_reservation_free "$INFERA_E2E_RESERVATION") smax="${INFERA_E2E_SPILL_MAX:-2}" if [ "$rfree" = "-1" ]; then - echo "[$label] WARNING: reservation '$INFERA_E2E_RESERVATION' not found — falling back to open partition '$SLURM_PART'" >&2 + echo "[$label] WARNING: reservation '$INFERA_E2E_RESERVATION' does not exist — falling back to open partition '$SLURM_PART'" >&2 mode="resv-gone->open" elif [ "$rfree" != "0" ]; then - # free>0, or -2 (no scontrol): use the reservation. + # free>0, or -2/-3 (cannot tell): keep the reservation. Only a query that + # answered may drop it -- reading a controller blink as "gone" is what + # sent a whole run to the open partition on 2026-08-06. resv=(--reservation="$INFERA_E2E_RESERVATION"); mode="resv" else inflight=$(_spill_inflight) @@ -643,10 +655,16 @@ run_e2e_disagg() { fi # An expired reservation is worse than none — every step's `srun --reservation` - # would fail. Drop it, as _dispatch_slurm does for the mixed tier. - if [ -n "${INFERA_E2E_RESERVATION:-}" ] && [ -z "$(_reservation_nodes "$INFERA_E2E_RESERVATION")" ]; then - echo "[e2e disagg] WARNING: reservation '$INFERA_E2E_RESERVATION' not found — falling back to open partition '$SLURM_PART'" >&2 - unset INFERA_E2E_RESERVATION + # would fail. Drop it, as _dispatch_slurm does for the mixed tier, but only on + # a query that answered: a failed one says nothing about the pool. + local resv_nodes + if [ -n "${INFERA_E2E_RESERVATION:-}" ]; then + if ! resv_nodes=$(_reservation_nodes "$INFERA_E2E_RESERVATION"); then + echo "[e2e disagg] WARNING: cannot reach the scheduler to check reservation '$INFERA_E2E_RESERVATION' — keeping it" >&2 + elif [ -z "$resv_nodes" ]; then + echo "[e2e disagg] WARNING: reservation '$INFERA_E2E_RESERVATION' does not exist — falling back to open partition '$SLURM_PART'" >&2 + unset INFERA_E2E_RESERVATION + fi fi local rc=0 e prc out="$SCRATCH/.e2e-disag.out" @@ -713,10 +731,15 @@ run_e2e_disagg() { # Report-only: both tiers can still run (degraded) without a reservation or with # a nearly full /home, and a hard exit here would cost a whole CI run to find out. _e2e_preflight() { - local avail - if [ -n "${INFERA_E2E_RESERVATION:-}" ] && command -v scontrol >/dev/null 2>&1 \ - && [ -z "$(_reservation_nodes "$INFERA_E2E_RESERVATION")" ]; then - echo "[e2e] ERROR: reservation '$INFERA_E2E_RESERVATION' does not exist (gone or expired)" >&2 + local avail resv_nodes + if [ -n "${INFERA_E2E_RESERVATION:-}" ] && command -v scontrol >/dev/null 2>&1; then + if ! resv_nodes=$(_reservation_nodes "$INFERA_E2E_RESERVATION"); then + # This line used to say "does not exist" for an unreachable controller too, + # which sent triage looking for a deleted reservation. + echo "[e2e] ERROR: cannot reach the scheduler to check reservation '$INFERA_E2E_RESERVATION' (scontrol failed)" >&2 + elif [ -z "$resv_nodes" ]; then + echo "[e2e] ERROR: reservation '$INFERA_E2E_RESERVATION' does not exist (gone or expired)" >&2 + fi fi avail=$(df -Pk /home 2>/dev/null | awk 'NR==2{print $4}') case "$avail" in From a9c38af95f2cd45ca77ec4e27e6fddf6800c68d8 Mon Sep 17 00:00:00 2001 From: xiaobochen-amd Date: Thu, 6 Aug 2026 16:05:44 +0000 Subject: [PATCH 04/10] fix(tests): finish separating a failed SLURM query from an empty result 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 Signed-off-by: xiaobochen-amd --- tests/run_tests.sh | 47 ++++++++++++++++++++++++++++++++++------------ 1 file changed, 35 insertions(+), 12 deletions(-) diff --git a/tests/run_tests.sh b/tests/run_tests.sh index 9e98900b..c2bb29f5 100755 --- a/tests/run_tests.sh +++ b/tests/run_tests.sh @@ -100,16 +100,21 @@ _cleanup_scratch() { # has to scancel it: job id from $_CUR_DISPATCH_OUT, else from the job tag. _CUR_DISPATCH_OUT="" _cancel_dispatched() { - local jids="" i suf csv + local jids="" i suf csv left if [ -n "$_CUR_DISPATCH_OUT" ] && [ -f "$_CUR_DISPATCH_OUT" ]; then jids=$(grep -oE 'srun: job [0-9]+' "$_CUR_DISPATCH_OUT" 2>/dev/null \ | grep -oE '[0-9]+' | sort -u | tr '\n' ' ') fi if [ -z "$jids" ] && [ -n "${INFERA_E2E_JOB_TAG:-}" ]; then suf="-${INFERA_E2E_JOB_TAG}" - jids=$(squeue -h -u "$(id -un)" -o '%i %j' 2>/dev/null \ + # 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 + echo "[cleanup] squeue failed — cannot list this run's jobs, leaving them to the workflow's reclaim step" >&2 + return 1 + fi fi [ -n "$jids" ] || return 0 echo "[cleanup] cancelling dispatched SLURM job(s): $jids" >&2 @@ -118,8 +123,13 @@ _cancel_dispatched() { for i in 1 2 3 4 5; do scancel $jids >/dev/null 2>&1 || true sleep 2 - [ -z "$(squeue -h -j "$csv" -o '%i' 2>/dev/null)" ] && return 0 + # Only a query that answered may confirm the cancel: on Spur a gone job still + # exits 0 with no output. (Stock SLURM errors on an invalid id, so there this + # never confirms and the warning below is a false alarm -- the workflow's + # reclaim step is the backstop either way.) + left=$(squeue -h -j "$csv" -o '%i' 2>/dev/null) && [ -z "$left" ] && return 0 done + echo "[cleanup] could not confirm the cancel of $jids" >&2 } # Nodes the running PD-disagg attempt placed containers on. A killed run skips # pytest's teardown, so without this a cancel leaves prefill+decode on the GPUs. @@ -291,6 +301,9 @@ _rival_holder() { # Hold both PD nodes' GPUs for the whole run: disagg's per-step sruns leave them # idle in between, so SLURM would hand one out and the fixed ports (etcd 2379, # router 8000, ...) collide. Our own no-gres steps co-schedule. Sets _HOLDER_JID. +# 0 = held, 1 = another holder won the pair, 2 = SLURM never placed the hold. +# The caller reports 1 and 2 differently: they used to read alike, so a refused +# sbatch was announced as a lost race and pointed triage away from the scheduler. _hold_pair() { local pair="$1" script="$SCRATCH/hold.sh" jid st rs waited qos=() i other # A real script file, not --wrap: on Spur --wrap always NODE_FAILs at -N2. @@ -323,7 +336,7 @@ _hold_pair() { scancel "$jid" >/dev/null 2>&1 echo "[e2e disagg] hold attempt $i on $pair not started (${st:-?}/${rs:-?}) — retrying" >&2 done - return 1 + return 2 } # One renderD* per GPU; PCI vendor 0x1002 == AMD. _amd_gpu_count() { @@ -350,8 +363,12 @@ _reservation_free() { echo "$free" } # Caps borrowed nodes at INFERA_E2E_SPILL_MAX; concurrent dispatchers can race it. +# Non-zero if the query failed, so the caller does not read that as "none in +# flight" and borrow past the cap exactly when the scheduler is already unwell. _spill_inflight() { - squeue -h -u "$(id -un)" -o '%j' 2>/dev/null | grep -c -- 'spill' || true + local out + out=$(squeue -h -u "$(id -un)" -o '%j' 2>/dev/null) || return 1 + printf '%s\n' "$out" | grep -c -- 'spill' || true } # Report why the dispatch is still queued (a waiting job prints NOTHING, so a CI @@ -431,8 +448,8 @@ _dispatch_slurm() { # sent a whole run to the open partition on 2026-08-06. resv=(--reservation="$INFERA_E2E_RESERVATION"); mode="resv" else - inflight=$(_spill_inflight) - if [ "$smax" -gt 0 ] && [ "$inflight" -lt "$smax" ]; then + # A failed count must not authorise a spill: queue on the reservation. + if inflight=$(_spill_inflight) && [ "$smax" -gt 0 ] && [ "$inflight" -lt "$smax" ]; then # spill marker sits before the run_id-engine suffix so ci.yml reclaim matches. jobname="infera-ci-${label}-spill${INFERA_E2E_JOB_TAG:+-$INFERA_E2E_JOB_TAG}" mode="spill($((inflight + 1))/$smax)" @@ -669,7 +686,7 @@ run_e2e_disagg() { local rc=0 e prc out="$SCRATCH/.e2e-disag.out" local max_attempts=3 attempt exclude n1 n2 nodes ok - local races max_races="${INFERA_E2E_HOLD_RACE_MAX:-10}" + local races max_races="${INFERA_E2E_HOLD_RACE_MAX:-10}" hold_rc for e in "${engines[@]}"; do echo "----- e2e disagg — tests/e2e/pd_disag/$e -----" attempt=0; ok=0; exclude=""; races=0 @@ -690,13 +707,19 @@ run_e2e_disagg() { # Losing the race is not a node fault, so the pair must NOT join $exclude: # with a small pool the engine would exclude every node and then starve on # an idle cluster. Bounded so a pathological loser fails loudly instead. - if ! _hold_pair "$n1,$n2"; then + # Not `if ! _hold_pair`: inside that, $? is the negation's, not the call's. + _hold_pair "$n1,$n2"; hold_rc=$? + if [ "$hold_rc" -ne 0 ]; then races=$((races + 1)) if [ "$races" -ge "$max_races" ]; then - echo "[e2e disagg] lost the node-hold race $races times — giving up on $e" >&2 + if [ "$hold_rc" -eq 2 ]; then + echo "[e2e disagg] SLURM never placed a node hold in $races attempts — giving up on $e" >&2 + else + echo "[e2e disagg] lost the node-hold race $races times — giving up on $e" >&2 + fi break fi - echo "[e2e disagg] could not hold $n1,$n2 (race $races/$max_races) — re-picking in 30s" >&2 + echo "[e2e disagg] could not hold $n1,$n2 (attempt $races/$max_races) — re-picking in 30s" >&2 attempt=$((attempt - 1)); sleep 30; continue fi races=0 From 2a085567c34a94fcb0ff86bd9cb521d3bd871176 Mon Sep 17 00:00:00 2001 From: xiaobochen-amd Date: Thu, 6 Aug 2026 16:38:10 +0000 Subject: [PATCH 05/10] fix(tests): say why a SLURM query failed, not just that it did 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 Signed-off-by: xiaobochen-amd --- tests/run_tests.sh | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/tests/run_tests.sh b/tests/run_tests.sh index c2bb29f5..81af2458 100755 --- a/tests/run_tests.sh +++ b/tests/run_tests.sh @@ -100,21 +100,23 @@ _cleanup_scratch() { # has to scancel it: job id from $_CUR_DISPATCH_OUT, else from the job tag. _CUR_DISPATCH_OUT="" _cancel_dispatched() { - local jids="" i suf csv left + local jids="" i suf csv left queue if [ -n "$_CUR_DISPATCH_OUT" ] && [ -f "$_CUR_DISPATCH_OUT" ]; then jids=$(grep -oE 'srun: job [0-9]+' "$_CUR_DISPATCH_OUT" 2>/dev/null \ | grep -oE '[0-9]+' | sort -u | tr '\n' ' ') fi if [ -z "$jids" ] && [ -n "${INFERA_E2E_JOB_TAG:-}" ]; then suf="-${INFERA_E2E_JOB_TAG}" - # 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' ' '); then - echo "[cleanup] squeue failed — cannot list this run's jobs, leaving them to the workflow's reclaim step" >&2 + # A failed lookup is not an empty queue. Capture before parsing: awk would + # filter the error away, leaving a bare "squeue failed" that has to be + # reproduced to diagnose. Then hand off to ci.yml's reclaim step. + if ! queue=$(squeue -h -u "$(id -un)" -o '%i %j' 2>&1); then + echo "[cleanup] squeue failed, leaving this run's jobs to the workflow's reclaim step: $queue" >&2 return 1 fi + jids=$(printf '%s\n' "$queue" \ + | awk -v suf="$suf" '$2 ~ /^infera-ci-/ && substr($2, length($2)-length(suf)+1)==suf {print $1}' \ + | tr '\n' ' ') fi [ -n "$jids" ] || return 0 echo "[cleanup] cancelling dispatched SLURM job(s): $jids" >&2 @@ -127,9 +129,9 @@ _cancel_dispatched() { # exits 0 with no output. (Stock SLURM errors on an invalid id, so there this # never confirms and the warning below is a false alarm -- the workflow's # reclaim step is the backstop either way.) - left=$(squeue -h -j "$csv" -o '%i' 2>/dev/null) && [ -z "$left" ] && return 0 + left=$(squeue -h -j "$csv" -o '%i' 2>&1) && [ -z "$left" ] && return 0 done - echo "[cleanup] could not confirm the cancel of $jids" >&2 + echo "[cleanup] could not confirm the cancel of $jids: ${left:-no output}" >&2 } # Nodes the running PD-disagg attempt placed containers on. A killed run skips # pytest's teardown, so without this a cancel leaves prefill+decode on the GPUs. @@ -232,7 +234,9 @@ _have_slurm() { command -v srun >/dev/null 2>&1; } # masquerade as a failed query, and callers now act on that distinction. _reservation_nodes() { local out - out=$(scontrol show reservation "$1" 2>/dev/null) || return 1 + # Forward scontrol's own words: callers can only say "cannot reach the + # scheduler", which is not enough to act on. + out=$(scontrol show reservation "$1" 2>&1) || { printf '%s\n' "$out" >&2; return 1; } printf '%s\n' "$out" | awk -v r="ReservationName=$1" ' BEGIN{RS="";FS="\n"} $1==r { for(i=1;i<=NF;i++) if($i ~ /Nodes=/){ n=$i; sub(/.*Nodes=/,"",n); sub(/[[:space:]].*/,"",n); print n; exit } }' \ @@ -367,7 +371,7 @@ _reservation_free() { # flight" and borrow past the cap exactly when the scheduler is already unwell. _spill_inflight() { local out - out=$(squeue -h -u "$(id -un)" -o '%j' 2>/dev/null) || return 1 + out=$(squeue -h -u "$(id -un)" -o '%j' 2>&1) || { printf '%s\n' "$out" >&2; return 1; } printf '%s\n' "$out" | grep -c -- 'spill' || true } From ae25ae5ebda5604d4b51795612aa49dc657eab89 Mon Sep 17 00:00:00 2001 From: xiaobochen-amd Date: Fri, 7 Aug 2026 01:59:47 +0000 Subject: [PATCH 06/10] ci: one event per state of the code, not two per commit 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 Signed-off-by: xiaobochen-amd --- .github/workflows/ci.yml | 134 +++++++++++++++------------------------ 1 file changed, 51 insertions(+), 83 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4cfb4f28..116cfa07 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1,10 +1,19 @@ name: CI on: + # One event per state of the code: the PR is the pre-merge gate, main is the + # post-merge check. Firing push on every branch ran both for the same commit, + # so the jobs were split by hand -- unit on push, GPU on the PR -- which left + # the PR's own check list reading "skipped" for tests that ran in a push run + # it does not link to, and two check runs named `unit` on one commit. + # A branch with no PR now runs nothing; open a draft to get CI. push: - branches: ["**"] # e2e only for code reaching main untested (see e2e_gate) + branches: [main] pull_request: - branches: [main] # e2e runs here, pre-merge — the usual path + branches: [main] + # ready_for_review so marking a draft ready re-runs and picks up the GPU + # tiers that drafts skip (see e2e_gate). + types: [opened, synchronize, reopened, ready_for_review] workflow_dispatch: # manual "Run workflow" button (Actions tab) inputs: run_e2e_mixed: @@ -22,7 +31,6 @@ concurrency: permissions: contents: read - actions: read jobs: # Classify the change: does it touch package code, or only docs/examples? A @@ -39,6 +47,16 @@ jobs: fetch-depth: 0 - id: f run: | + # Someone pressed "Run workflow": run everything, whatever the last + # commit happened to touch. There is no diff base on this event, so the + # logic below would fall back to HEAD~1 and skip the whole run off a + # docs-only commit -- and since a branch push no longer starts CI, this + # button is the only way to force one. + if [ "${{ github.event_name }}" = "workflow_dispatch" ]; then + echo "manual run — treating as a code change" + echo "code=true" >> "$GITHUB_OUTPUT" + exit 0 + fi if [ "${{ github.event_name }}" = "pull_request" ]; then range="${{ github.event.pull_request.base.sha }}...${{ github.event.pull_request.head.sha }}" else @@ -63,65 +81,35 @@ jobs: echo "=> code=$code" echo "code=$code" >> "$GITHUB_OUTPUT" - # Should the e2e tiers run for THIS event? Every PR into main is tested - # pre-merge, so landing that same code must not test it twice — but "same" has - # to mean the CONTENT, not the PR: if main moved on while the PR sat open, what - # lands is a combination no e2e ever saw. Compare git TREES, which are exactly - # the content, and are equal iff the merge changed nothing versus the PR head. + # Should the GPU tiers run for THIS event? Every PR into main, and every merge + # into main. Re-running on main is deliberate duplication: two PRs can each + # pass alone and break together, and only the merged result shows that. + # Drafts are the exception — iterating on one must not cost a GPU run per + # push, so they get lint and unit only until they are marked ready. e2e_gate: runs-on: ubuntu-latest - permissions: - contents: read - pull-requests: read outputs: run: ${{ steps.decide.outputs.run }} steps: - id: decide env: - GH_TOKEN: ${{ github.token }} - REPO: ${{ github.repository }} - SHA: ${{ github.sha }} + EVENT: ${{ github.event_name }} + REF: ${{ github.ref }} + DRAFT: ${{ github.event.pull_request.draft }} run: | - tree_of() { gh api "repos/$REPO/commits/$1" --jq .commit.tree.sha 2>/dev/null; } - run=false - if [ "${{ github.event_name }}" = "pull_request" ]; then - run=true - echo "pull_request into main — e2e runs pre-merge" - elif [ "${{ github.event_name }}" = "push" ] && [ "${{ github.ref }}" = "refs/heads/main" ]; then - # Squash and merge commits both report the PR they came from. - head=$(gh api "repos/$REPO/commits/$SHA/pulls" --jq '.[0].head.sha // empty' 2>/dev/null) - landed=$(tree_of "$SHA") - tested=""; [ -n "$head" ] && tested=$(tree_of "$head") - if [ -z "$head" ]; then - run=true; echo "no PR behind this commit — its code was never e2e'd" - elif [ -z "$landed" ] || [ -z "$tested" ]; then - # Never infer "already tested" from a failed lookup: re-testing costs - # GPU minutes, shipping untested code costs more. - run=true; echo "could not read both trees — running e2e to be safe" - elif [ "$landed" = "$tested" ]; then - echo "tree $landed is what PR head $head already e2e'd — skipping" - else - run=true - echo "tree $landed != PR head $head's $tested — main moved under the PR" - fi + if [ "$EVENT" = pull_request ] && [ "$DRAFT" = true ]; then + echo "draft pull request — lint and unit only until it is marked ready" + elif [ "$EVENT" = pull_request ]; then + run=true; echo "pull request into main — GPU tiers run pre-merge" + elif [ "$EVENT" = push ] && [ "$REF" = refs/heads/main ]; then + run=true; echo "merged into main — GPU tiers run again on the result" else - echo "not a PR into main, and not a push to main — no e2e" + echo "neither a pull request into main nor a push to main — no GPU tiers" fi echo "=> run_e2e=$run" echo "run=$run" >> "$GITHUB_OUTPUT" - pre_check: - runs-on: ubuntu-latest - outputs: - should_skip: ${{ steps.skip.outputs.should_skip }} - steps: - - id: skip - uses: fkirc/skip-duplicate-actions@f75f66ce1886f00957d99748a42c724f4330bdcf # v5 - with: - skip_after_successful_duplicate: "true" - concurrent_skipping: "never" - # Sign-off gate for the GPU tiers below — `needs:` cannot reach a job in # another workflow, so dco.yml is called here as one. Skipped off a PR (there # is nothing to check), which the tiers below read as "did not fail". @@ -130,10 +118,8 @@ jobs: uses: ./.github/workflows/dco.yml lint: - needs: [pre_check, changes] - if: >- - needs.pre_check.outputs.should_skip != 'true' && - needs.changes.outputs.code == 'true' + needs: [changes] + if: needs.changes.outputs.code == 'true' runs-on: ubuntu-latest steps: - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 @@ -145,14 +131,9 @@ jobs: run: pre-commit run --all-files --show-diff-on-failure unit: - needs: [lint, pre_check, changes] - # Skip on docs-only changes, and on same-repo PRs (already covered by the - # branch push event); still run on push and on fork PRs. - if: >- - needs.pre_check.outputs.should_skip != 'true' && - needs.changes.outputs.code == 'true' && - (github.event_name != 'pull_request' || - github.event.pull_request.head.repo.full_name != github.repository) + needs: [lint, changes] + # Skip on docs-only changes. + if: needs.changes.outputs.code == 'true' runs-on: ubuntu-latest steps: - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 @@ -168,14 +149,9 @@ jobs: # Rust router: clippy + unit/integration tests. GH-hosted ubuntu-latest # ships a stable toolchain (with clippy), so no toolchain setup is needed. rust: - needs: [lint, pre_check, changes] - # Skip on docs-only changes, and on same-repo PRs (already covered by the - # branch push event); still run on push and on fork PRs. - if: >- - needs.pre_check.outputs.should_skip != 'true' && - needs.changes.outputs.code == 'true' && - (github.event_name != 'pull_request' || - github.event.pull_request.head.repo.full_name != github.repository) + needs: [lint, changes] + # Skip on docs-only changes. + if: needs.changes.outputs.code == 'true' runs-on: ubuntu-latest defaults: run: @@ -190,13 +166,11 @@ jobs: run: cargo test # Engine GPU tests. Same schedule as the e2e tiers (see e2e_gate): every PR - # into main, and any push that lands untested code on main — never on a plain - # branch push, which is where these GPU minutes used to go. + # into main once it is out of draft, and every merge into main. engine: - needs: [lint, pre_check, changes, dco, e2e_gate] + needs: [lint, changes, dco, e2e_gate] if: >- !cancelled() && - needs.pre_check.outputs.should_skip != 'true' && needs.changes.outputs.code == 'true' && needs.lint.result != 'failure' && needs.lint.result != 'cancelled' && needs.dco.result != 'failure' && needs.dco.result != 'cancelled' && @@ -222,12 +196,11 @@ jobs: if: always() && (cancelled() || failure()) run: .github/scripts/reclaim_slurm_jobs.sh infera-ci- "-${{ github.run_id }}-engine" - # Full PD-mixed e2e (per engine, parallel). When it runs is e2e_gate's call: - # every PR into main, plus a push that lands untested code on main. + # Full PD-mixed e2e (per engine, parallel). When it runs is e2e_gate's call. e2e-mixed: # Skipped for docs-only changes. Gated behind lint and dco: run only if neither # failed. `!cancelled()` + result checks (instead of a plain success dependency) - # is needed so e2e still runs when lint is *skipped* as a duplicate (pre_check) + # is needed so e2e still runs when lint is *skipped* for a docs-only change # or dco is skipped off a PR, but is held back when either fails. NOT # `always()`: on cancel the server re-evaluates job-level `if`, and `always()` # evaluates true, so the job is never cancelled — it keeps (or even starts) @@ -316,14 +289,9 @@ jobs: run: .github/scripts/reclaim_slurm_jobs.sh infera-ci- "-${{ github.run_id }}-${{ matrix.engine }}-disag" unit-torch-cpu: - needs: [lint, pre_check, changes] - # Skip on docs-only changes, and on same-repo PRs (already covered by the - # branch push event); still run on push and on fork PRs. - if: >- - needs.pre_check.outputs.should_skip != 'true' && - needs.changes.outputs.code == 'true' && - (github.event_name != 'pull_request' || - github.event.pull_request.head.repo.full_name != github.repository) + needs: [lint, changes] + # Skip on docs-only changes. + if: needs.changes.outputs.code == 'true' runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 From 58677596892840b07faeb23e6ef553a339ca53d7 Mon Sep 17 00:00:00 2001 From: xiaobochen-amd Date: Fri, 7 Aug 2026 02:00:13 +0000 Subject: [PATCH 07/10] ci: key the concurrency group on the PR, not the branch name 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 Signed-off-by: xiaobochen-amd --- .github/workflows/ci.yml | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 116cfa07..c46ed61e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -26,7 +26,11 @@ on: default: false concurrency: - group: ${{ github.workflow }}-${{ github.head_ref || github.ref }} + # PR number, not branch name: head_ref carries no repository, so two forks + # that both call a branch `main` or `fix-ci` would share a group and cancel + # each other's runs -- which reads as "my CI vanished" and is near impossible + # to trace back. Falls back to the ref for pushes to main. + group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} cancel-in-progress: true permissions: From 38963774b9da116ff3094ddf5ef301300ec280da Mon Sep 17 00:00:00 2001 From: xiaobochen-amd Date: Fri, 7 Aug 2026 02:00:34 +0000 Subject: [PATCH 08/10] ci: run the DCO check once per pull request, not twice 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 Signed-off-by: xiaobochen-amd --- .github/workflows/ci.yml | 29 +++++++++-------------------- .github/workflows/dco.yml | 6 ++++-- 2 files changed, 13 insertions(+), 22 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c46ed61e..b652010f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -114,13 +114,6 @@ jobs: echo "=> run_e2e=$run" echo "run=$run" >> "$GITHUB_OUTPUT" - # Sign-off gate for the GPU tiers below — `needs:` cannot reach a job in - # another workflow, so dco.yml is called here as one. Skipped off a PR (there - # is nothing to check), which the tiers below read as "did not fail". - dco: - if: github.event_name == 'pull_request' - uses: ./.github/workflows/dco.yml - lint: needs: [changes] if: needs.changes.outputs.code == 'true' @@ -172,12 +165,11 @@ jobs: # Engine GPU tests. Same schedule as the e2e tiers (see e2e_gate): every PR # into main once it is out of draft, and every merge into main. engine: - needs: [lint, changes, dco, e2e_gate] + needs: [lint, changes, e2e_gate] if: >- !cancelled() && needs.changes.outputs.code == 'true' && needs.lint.result != 'failure' && needs.lint.result != 'cancelled' && - needs.dco.result != 'failure' && needs.dco.result != 'cancelled' && (needs.e2e_gate.outputs.run == 'true' || github.event_name == 'workflow_dispatch') runs-on: [self-hosted, crusoe] timeout-minutes: 60 @@ -202,19 +194,17 @@ jobs: # Full PD-mixed e2e (per engine, parallel). When it runs is e2e_gate's call. e2e-mixed: - # Skipped for docs-only changes. Gated behind lint and dco: run only if neither - # failed. `!cancelled()` + result checks (instead of a plain success dependency) - # is needed so e2e still runs when lint is *skipped* for a docs-only change - # or dco is skipped off a PR, but is held back when either fails. NOT - # `always()`: on cancel the server re-evaluates job-level `if`, and `always()` - # evaluates true, so the job is never cancelled — it keeps (or even starts) - # burning GPU nodes after "Cancel workflow". - needs: [lint, changes, dco, e2e_gate] + # Skipped for docs-only changes, and held back if lint failed. Checking + # lint's *result* rather than depending on its success keeps `!cancelled()` + # meaningful; `always()` would not work here, because on cancel the server + # re-evaluates job-level `if` and `always()` is true, so the job would never + # be cancelled — it would keep (or even start) burning GPU nodes after + # "Cancel workflow". + needs: [lint, changes, e2e_gate] if: >- !cancelled() && needs.changes.outputs.code == 'true' && needs.lint.result != 'failure' && needs.lint.result != 'cancelled' && - needs.dco.result != 'failure' && needs.dco.result != 'cancelled' && (needs.e2e_gate.outputs.run == 'true' || (github.event_name == 'workflow_dispatch' && inputs.run_e2e_mixed)) strategy: @@ -246,12 +236,11 @@ jobs: e2e-disag: # Gates mirror e2e-mixed, `!cancelled()` included: `always()` would keep this # holding a two-node pair after "Cancel workflow". - needs: [lint, changes, dco, e2e_gate] + needs: [lint, changes, e2e_gate] if: >- !cancelled() && needs.changes.outputs.code == 'true' && needs.lint.result != 'failure' && needs.lint.result != 'cancelled' && - needs.dco.result != 'failure' && needs.dco.result != 'cancelled' && (needs.e2e_gate.outputs.run == 'true' || (github.event_name == 'workflow_dispatch' && inputs.run_e2e_disag)) strategy: diff --git a/.github/workflows/dco.yml b/.github/workflows/dco.yml index ded8c652..a8e0b863 100644 --- a/.github/workflows/dco.yml +++ b/.github/workflows/dco.yml @@ -5,8 +5,10 @@ name: DCO # See CONTRIBUTING.md > Developer Certificate of Origin. on: - # Standalone on a PR into any branch; ci.yml additionally calls this one as a - # job, which is what lets its GPU tiers gate on the sign-off via `needs:`. + # One check, on a PR into any branch. ci.yml used to call this as a job too so + # its GPU tiers could gate on the sign-off, which ran it twice per PR under two + # different check names; lint already holds those tiers back, so the second + # copy bought nothing. workflow_call stays for any future caller. pull_request: branches: ["**"] workflow_call: From 8fcb80f081a8e9358ccdbe3c210141d668c641b4 Mon Sep 17 00:00:00 2001 From: xiaobochen-amd Date: Fri, 7 Aug 2026 02:00:54 +0000 Subject: [PATCH 09/10] ci: pin the last three actions to a commit 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 Signed-off-by: xiaobochen-amd --- .github/workflows/ci.yml | 4 ++-- .github/workflows/release.yml | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b652010f..10710ba0 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -287,8 +287,8 @@ jobs: if: needs.changes.outputs.code == 'true' runs-on: ubuntu-latest steps: - - uses: actions/checkout@v4 - - uses: actions/setup-python@v5 + - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + - uses: actions/setup-python@ece7cb06caefa5fff74198d8649806c4678c61a1 # v6.3.0 with: python-version: "3.10" - run: pip install -e ".[dev]" diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 79d1af61..35ccd8d1 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -321,7 +321,7 @@ jobs: run: pip install -r manual/sphinx/requirements.txt - name: Build manual (warnings = errors) run: make -C manual html - - uses: actions/upload-artifact@v4 + - uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4 with: name: infera-manual-html path: manual/_build/html From f377d11c74b68044f705785ed5c66f1eccc2e215 Mon Sep 17 00:00:00 2001 From: xiaobochen-amd Date: Fri, 7 Aug 2026 02:42:09 +0000 Subject: [PATCH 10/10] fix(tests): the engine tier must not pass on zero collected tests 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 Signed-off-by: xiaobochen-amd --- tests/run_tests.sh | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/tests/run_tests.sh b/tests/run_tests.sh index 81af2458..3fec8b08 100755 --- a/tests/run_tests.sh +++ b/tests/run_tests.sh @@ -568,7 +568,15 @@ run_engine() { cd /workspace PYT="python3 -m pytest -p no:cacheprovider -o addopts= -q -rfE" rc=0 - for f in $(find "$INFERA_TEST_SCOPE" -name "test_*.py" | sort); do + # A scope that matches nothing iterates zero times and exits 0, so a rename + # or a typo in engine_tier would report PASS having tested nothing. Capture + # before sorting: through a pipe, find's own exit code would be sort's 0. + if ! files=$(find "$INFERA_TEST_SCOPE" -name "test_*.py") || [ -z "$files" ]; then + echo "[engine $INFERA_TEST_SCOPE] FATAL: scope unreadable or holds no test_*.py" >&2 + echo "[engine $INFERA_TEST_SCOPE] scope unreadable or holds no test_*.py" >> /scratch/failures.txt + exit 1 + fi + for f in $(printf "%s\n" "$files" | sort); do echo "----- pytest $f -----" # tee: stream live for CI, keep a copy for the classification below. $PYT "$f" 2>&1 | stdbuf -oL tee /scratch/.engine_f.out; code=${PIPESTATUS[0]} @@ -577,7 +585,11 @@ run_engine() { 139|134|137) line="CRASH(exit=$code)"; rc=1 echo "[engine $INFERA_TEST_SCOPE] CRASH(exit=$code) $f" >> /scratch/failures.txt ;; 0) line=$(printf "%s" "$out" | grep -E "passed|failed|skipped|no tests ran" | tail -1) ;; - 5) line="no tests ran (whole file skipped — not a failure)" ;; + # Nothing collected. Each guarded file importorskips a module its own + # image ships, so a 5 means the image is broken — exactly when this must + # go red. Say so plainly: pytest words it "1 skipped", which reads benign. + 5) line="FAIL: no tests collected (exit=5)"; rc=1 + echo "[engine $INFERA_TEST_SCOPE] $f (exit=5, no tests collected)" >> /scratch/failures.txt ;; *) line=$(printf "%s" "$out" | grep -E "passed|failed|error|skipped" | tail -1) [ -z "$line" ] && line="(exit=$code)"; rc=1 fails=$(printf "%s\n" "$out" | grep -aE "^(FAILED|ERROR) ")