Skip to content

ci: split self-hosted E2E lanes into their own workflow (EAI-7548) - #193

Merged
fredespi merged 1 commit into
mainfrom
fix-ci-selfhosted-lane-timeout
Aug 10, 2026
Merged

ci: split self-hosted E2E lanes into their own workflow (EAI-7548)#193
fredespi merged 1 commit into
mainfrom
fix-ci-selfhosted-lane-timeout

Conversation

@fredespi

@fredespi fredespi commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

Summary

A job queued on an offline self-hosted runner cannot be cancelled by GitHub. While the self-hosted GPU E2E lanes shared ci.yml's concurrency group, a superseded run whose GPU job was queued on an offline runner kept holding the group — so the newer run's merge-required (GitHub-hosted) checks sat pending with zero jobs and had to be force-cancelled by hand (seen on #138).

timeout-minutes cannot fix this: the job-timeout timer only counts running time, never the queued/"waiting for a runner" phase, so it never reaps the stuck job. The only robust fix is structural separation.

What changed

  • New workflow e2e-selfhosted.yml holds the three self-hosted GPU lanes (e2e-gpu, e2e-gpu-strix-ubuntu, e2e-gpu-strix-windows) plus their own consolidated report, with its own concurrency group. An offline runner can now only ever stall this workflow's own supersession, never ci.yml's required checks.
  • ci.yml keeps the GitHub-hosted mock e2e lane and its required E2E consolidated report. Its shared concurrency group now holds only cancellable hosted jobs, so supersession works cleanly and the required checks always start.
  • The self-hosted workflow carries its own trimmed changes gate (cross-workflow needs is unavailable) and builds the rocm binary itself (no build-and-test dependency); ci.yml's required build-and-test + mock e2e remain the authoritative pre-merge build gate.
  • xtask e2e-report discovery now handles download-artifact@v8's single-artifact layout: when exactly one artifact matches, v8 extracts it into the artifacts-dir root (no per-artifact subdir), which each report job now hits after the split. A root-level report is labeled from its platform.json slug, or a neutral Unknown/Unknown identity when the sidecar is absent (a GPU run can error before writing it) so a hardware failure is never misattributed.
  • New regression tests: xtask/src/workflow_contract.rs (dependency-free) fails if a self-hosted lane reappears in ci.yml or the two workflows lose their distinct concurrency namespaces; plus discovery + descriptor tests in e2e-report.
  • Updated docs/ci-hardware-testing.md and the e2e-cucumber README for the split.

Note for reviewers

The four self-hosted checks remain in the branch's required-status-check list, so an offline runner can still block a merge via a missing required check until they are removed from that list — a separate branch-protection change, out of scope for this workflow change.

Test plan

  • clippy --workspace --all-targets --exclude e2e-cucumber -- -D warnings clean (Linux)
  • Full workspace tests + e2e-cucumber lib tests pass (Linux)
  • New workflow_contract + e2e-report discovery/descriptor tests pass
  • Both workflow files parse; all five required check names still produced, one workflow each

A job queued on an OFFLINE self-hosted runner cannot be cancelled by
GitHub. While the self-hosted GPU E2E lanes shared ci.yml's concurrency
group, a superseded run whose GPU job was queued on an offline runner kept
holding the group, so the newer run's merge-required (GitHub-hosted) checks
sat pending with zero jobs and had to be force-cancelled by hand (PR #138).

timeout-minutes cannot fix this: the job-timeout timer only counts running
time, never the queued/"waiting for a runner" phase, so it never reaps the
stuck job. The only robust fix is structural separation.

Move e2e-gpu, e2e-gpu-strix-ubuntu, and e2e-gpu-strix-windows into a new
e2e-selfhosted.yml with its own concurrency group. ci.yml's shared group now
holds only cancellable GitHub-hosted jobs, so supersession works cleanly and
the required checks always start. The new workflow carries its own trimmed
changes gate (cross-workflow needs is unavailable) and its own consolidated
report (named distinctly to avoid colliding with ci.yml's required report).

Teach xtask e2e-report discovery to handle download-artifact@v8's
single-artifact layout: when exactly one artifact matches, v8 extracts it into
the artifacts-dir root (no per-artifact subdir), which each report job now hits
after the split. Discovery labels a root-level report from its platform.json
slug, or a neutral Unknown/Unknown identity when the sidecar is absent (a GPU
run can error before writing it) so a hardware failure is never misattributed.

Add dependency-free contract tests (workflow_contract.rs) that fail if a
self-hosted lane reappears in ci.yml or the two workflows lose their distinct
concurrency namespaces, plus discovery and descriptor regression tests. Update
docs/ci-hardware-testing.md and the e2e-cucumber README for the split.

Note: the four self-hosted checks remain in the branch's required list, so an
offline runner can still block a merge via a missing required check until they
are removed from that list (a separate branch-protection change).

Signed-off-by: fredespi <fredrik.espinoza@gmail.com>
@fredespi
fredespi requested a review from a team as a code owner August 7, 2026 09:43

@r0x0r r0x0r left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strong approve — the change targets the root cause and is unusually well-tested for a CI change.

What's good

  • Root cause, not symptom. The #138 stall was an offline (uncancellable) self-hosted job holding ci.yml's shared concurrency group. timeout-minutes genuinely can't fix that (it doesn't count queued time), so structural separation into e2e-selfhosted.yml with its own concurrency group is the right fix. Confirmed live: the two workflows now run as separate runs with distinct check names, and all three GPU lanes ran green.
  • Excellent regression guarding. xtask/src/workflow_contract.rs is dependency-free and asserts the actual invariant (no self-hosted/amd-gpu/strix-halo labels in ci.yml, distinct concurrency.group + distinct name:, cancel-in-progress preserved). It even includes extractor self-tests so the contract can't false-pass on a multiline runs-on or folded group:.
  • The download-artifact@v8 single-artifact flattening is handled correctly. After the split each report job has exactly one artifact, so v8 flattens into the root; discover() + label_for_root_report() handle both layouts, with tests.
  • The e2e-unknown-report neutral fallback is a genuinely good call — a GPU run that errors before writing platform.json must not be misattributed to Mock/Linux in the grid. Covered by parse_descriptor_unknown_is_not_falsely_linux.
  • Report job name collision avoided (E2E consolidated report vs ... (self-hosted)), actions pinned by SHA + version comment, and no new internal-path leaks introduced (the host-specific paths were moved verbatim from ci.yml).

Verified locally against the PR head: full xtask and e2e-report test suites pass, and both workflow YAMLs parse.

Notes / suggestions (non-blocking)

  1. Duplicated heavy filter list. The changes job and its path-filter list are now copied into both workflows (cross-workflow needs isn't possible, so this is unavoidable), but nothing guards against drift — if a new heavy path category is later added only to ci.yml, the GPU lanes' gating silently diverges. Consider a small contract test asserting the two heavy: lists are identical, since workflow_contract.rs is already the natural home for it.
  2. The merge-blocking hole is only half-closed by this PR (as noted in the description). The four self-hosted checks remain in the required-status-check list, so an offline runner can still block a merge via a missing required check. This PR fixes the specific #138 supersession stall (its stated scope), but "an offline runner can't block a merge" isn't fully true until the branch-protection change lands. Worth confirming that follow-up is tracked.
  3. Minor/defensive: discover() additively handles a root report.json and subdirs in the same directory. That mix shouldn't occur with download-artifact@v8, so it's harmless — just noting it isn't a layout the action actually produces.

@fredespi
fredespi added this pull request to the merge queue Aug 10, 2026
Merged via the queue into main with commit bb914c9 Aug 10, 2026
23 checks passed
@fredespi
fredespi deleted the fix-ci-selfhosted-lane-timeout branch August 10, 2026 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants