feat(e2e-prewarm): pin the SDK version instead of always tracking latest (ROCMAI-430)

[juhovainio]

Depends on #415

In plain English

When we warm up a test GPU with a ROCm SDK build ahead of time, it always grabs whatever version is newest. This adds the ability to pin a specific SDK version instead, which we need to test older but still-supported versions. A follow-up feature (automatically testing several pinned versions at once) is intentionally left for later, since there is no reliable way yet to look up which version number counts as "one release back" or "two releases back". Needs #415 merged first.

ROCMAI-430: pin the SDK version used by cargo xtask e2e-prewarm instead of
always tracking the channel's latest.

What changed

• Added mutually exclusive --version/--build-date flags to
  cargo xtask e2e-prewarm, mirroring the existing rocm install sdk
conflicts_with pattern, and threaded the pin through to that same
  rocm install sdk invocation.
• Folded the pin into decide()'s runtime-key logic: a pin now overrides
  every status-driven branch (Update/UpToDate/etc.), since rocm update
  always resolves a channel against the index's latest and has no notion of
  "stay on the version I originally pinned." Without this, an n-1 pin would
  show up as status=update_available against a release index that has
  moved on, and the existing Update branch would silently replace it with
  latest.

Deferred: the sdk_version matrix axis (5c)

This PR does not add strategy.matrix.sdk_version: [current, n-1, n-2] to
the release-branch self-hosted lanes. There is no existing mechanism in this
repo to map "n-1"/"n-2" to concrete --version/--build-date pairs:
therock.rs's load_simple_index_versions/parse_simple_index_versions
parse and sort the channel index's available versions, but they are private
functions with no CLI exposure a workflow step could call. Hardcoding a
version mapping would be fragile and go stale silently. Per this phase's own
escape hatch, landing the flag passthrough and the runtime-key fix now, and
deferring the matrix axis to a follow-up once a real version-lookup
mechanism exists, rather than shipping something brittle.

Verification

• cargo fmt --all -- --check
• cargo clippy --locked --workspace --all-targets -- -D warnings
• cargo test -p xtask --all-targets (947 passed including the two new
  regression tests below)
• cargo xtask e2e-prewarm --help shows both --version and --build-date,
  and passing both together fails with a clap conflicts_with error.
• Two new regression tests in xtask/src/e2e_prewarm.rs, written first so
  they fail first:
  • distinct_version_pins_yield_distinct_runtime_keys: two different pins
    against the same channel/format/family resolve to distinct runtime keys.
    Verified failing before the fix (left: Some("...gfx94x-dcgpu-7-13-0"); right: Some("...gfx94x-dcgpu-7-11-0") assertion mismatch when the pin
    branch was disabled) and passing after.
  • unpinned_prewarm_key_is_unchanged: an unpinned run's key is
    byte-identical to the pre-change key, proving backward compatibility.
    Passes both before and after (as expected, since it is a snapshot of
    unpinned behavior, not exercising the new branch).
• Leak scan against origin/main..HEAD: diff clean.

AGENTS.md section 3

No user-facing CLI surface beyond the two new flags above (documented inline
via clap doc comments, mirroring rocm install sdk's existing flags), so no
additional no-scenario section applies.

ROCMAI-430

[r0x0r]

What the change does. Adds mutually exclusive --version/--build-date to cargo xtask e2e-prewarm, threads them into the rocm install sdk call, and gives decide() a pin branch that overrides every status-driven branch. The motivation is right: rocm update only ever resolves a channel to the index's latest, so an n-1 pin would read as update_available forever and the Update branch would quietly replace it with latest. The pin branch is also correctly placed after the unattributed-status=error floor, so an unreachable index still reuses the tree rather than re-downloading.

Coverage. Full read of both changed files at head, checked against apps/rocm/src/therock.rs (runtime_key, wheel_runtime_key, slugify, normalize_requested_build_date, runtime_version_build_date, RuntimeVersionSelector), every e2e-prewarm invocation in .github/workflows/, and docs/ci-hardware-testing.md. Reviewed against the PR's own base branch (eai-8761-rc-e2e-trigger), not main, so #415's commits are excluded. No tests run.

Assessment: needs work — one confirmed defect.

Blocking

--build-date pins never match, so a date-pinned pre-warm reinstalls from scratch on every run. xtask/src/e2e_prewarm.rs, key_matches_pin.

The pin is matched against the rendered runtime key, but a build date reaches the key as eight consecutive digits, while the flag's own documented format is dashed:

• normalize_requested_build_date (therock.rs:3742) returns format!("{year:04}-{month:02}-{day:02}") → 2026-06-05.
• A build date selects a package version of the form 7.13.0a20260605 (see pip_runtime_rejects_requested_build_date_without_matching_stack, and runtime_version_build_date, which recovers the date by scanning for an 8-digit window).
• slugify maps every non-alphanumeric to - but leaves digit runs intact, so the key is release-wheel-multi-arch-7-13-0a20260605-<fp16>.

Both arms of the match then fail: key.contains("2026-06-05") is false (the key holds 20260605), and pin.replace('.', "-") is a no-op on a string with no dots. key_matches_pin therefore returns false for every build-date pin, decide falls to None => Decision::Install, and the lane does a fresh multi-GiB install on every invocation — the exact cost this PR exists to avoid.

Sharpest version of the problem: #[arg(long, value_name = "YYYY-MM-DD")] advertises the one spelling that can never match. --build-date 20260605 happens to work, because the raw user string is passed to decide before rocm install sdk normalises it; --build-date 2026-06-05 silently does not. Two spellings of the same date, two different behaviours.

The doc comment above the helper states the opposite and should be corrected with it:

--build-date is already dash-separated and matches as given

Suggested fix — also compare the digits-only form:

fn key_matches_pin(runtime_key: &str, pin: &str) -> bool {
    let digits_only = pin.replace(['.', '-'], "");
    runtime_key.contains(pin)
        || runtime_key.contains(&pin.replace('.', "-"))
        || (!digits_only.is_empty() && runtime_key.contains(&digits_only))
}

A decide(..., Some("2026-06-05")) test against a key carrying 7-13-0a20260605 would have caught this, and is the test I would add alongside the fix.

Non-blocking

• A version pin matches as a prefix. "7.13.0" → "7-13-0", which is a substring of "...-7-13-0a20260605-...". Within the nightly channel, --version 7.13.0 would reuse a nightly build of 7.13.0 and report runtime already matches the pinned version 7.13.0. The doc comment argues a false match would need "another pin to contain this one as a literal substring, which two real pins never do" — a release version is a literal prefix of its own nightlies, so that does happen. Matching on a delimited segment rather than a bare substring would close both this and the build-date case.
• The flags have no caller. All twelve cargo xtask e2e-prewarm invocations across e2e-selfhosted.yml and nightly.yml pass only --channel/--prewarm-dir. That is consistent with the deferral the PR describes, but nothing in-tree exercises the new path, which is partly why the build-date defect is invisible.
• Untested branches: pin miss → Install, a pin against a non-matching channel, and the --build-date passthrough in run() (the unit tests cover pin in decide, not the subprocess argv).
• unpinned_prewarm_key_is_unchanged asserts on decide, not on a key; unpinned_decide_is_unchanged would describe it better.

Tradeoffs

• On a pin miss with an attributable status=error line, the pinned path returns Install where the unpinned path would return Reuse. That looks defensible — reusing a different version would silently serve the wrong SDK — but it does mean a pinned lane turns an unreachable index into a failed install rather than a quiet reuse. Worth confirming that is the intent.
• Collapsing version/build_date into one pin: Option<&str> for decide is right for a substring test, and run() correctly keeps the two apart when building the install sdk argv.

Positive signals

• Matching on serving_runtime_key() rather than the raw key means a tree that a repair has already migrated still takes the pin-hit branch instead of reinstalling.
• The comment explaining why a pin miss must be Install and never update/repair (both of which only ever apply the report's target= key) is the kind of reasoning that stops this branch being "simplified" later.
• The clap wiring mirrors rocm install sdk exactly — same flag names, same value_name, same conflicts_with pair — so there is one spelling to learn rather than two.

The merge decision is yours; this review is posted as a comment and files no approval or change request.

---

🤖 by agent-hub on AMD AgentHub

[juhovainio]

Addressed the review in 17239c01: key_matches_pin now also compares the pin's digits-only form against the runtime key, so a --build-date pin like 2026-06-05 (which reaches the key as the dash-less run 20260605) matches instead of always falling through to Decision::Install. Added build_date_pin_matches_the_digits_only_key as a regression test.

[r0x0r]

Thanks for the quick turnaround on the build-date pin — I re-reviewed at 585da4ad. The original problem is genuinely fixed: a YYYY-MM-DD pin now matches the eight-digit run in the key, and build_date_pin_matches_the_digits_only_key locks it in. The corrected doc comment about where the dashes go is accurate too.

Two things the new digits-only comparison brought with it, though, and I'd like them sorted before this merges. I checked both by lifting key_matches_pin out and running it against real key shapes rather than reasoning about it, so these aren't hypotheticals.

The one I care about most is that stripping punctuation off the pin also lets it match digit runs that aren't a version at all. wheel_runtime_key ends every wheel key with 16 hex characters of a SHA-256, and hex includes 0-9, so a short pin can land inside the fingerprint of a completely different runtime. That turns a cache miss into something worse than the re-download this change set out to fix: decide reuses and activates the wrong SDK, and the reason string says it matched the pin, so the lane looks correct while testing the wrong thing.

The second is smaller: MMDDYYYY is still a miss, so that spelling re-downloads every run exactly like YYYY-MM-DD used to.

One thing I looked at and concluded was fine: the pin branch sits after the unattributed status=error floor, so an unreachable index still reuses the tree. And a pinned runtime whose line reports status=error is matched on key alone, which is the right call.

Details inline.

[r0x0r]

This can match a digit run that isn't a version.

wheel_runtime_key (apps/rocm/src/therock.rs:6159) appends fingerprint[..16] of a SHA-256, and slugify leaves alphanumerics alone, so those 16 hex characters sit in the key and contain decimal digits. A pin that strips to a short digit string can therefore match a runtime it has nothing to do with. Compiling this function as-is and running it:

key_matches_pin("release-wheel-multi-arch-7-9-0-aa7130bbccddeeff", "7.13.0") == true

7.13.0 strips to 7130, which appears inside that fingerprint — but the runtime is 7.9.0. decide then returns Reuse with activate pointing at it and the reason runtime already matches the pinned version 7.13.0, so the lane pre-warms the wrong SDK and reports success. That's a silently wrong test result rather than a loud failure, which is why I'd rank it above the original bug.

It also means the doc comment just above no longer holds: it argues a false match "would need another pin to contain this one as a literal substring, which two real pins never do." The digits-only branch matches against text that isn't a pin at all, so that reasoning doesn't cover this case any more.

Suggest anchoring the digits-only arm instead of leaving it as a free substring search — require the run to be delimited (a dash or the end of the key on both sides) and at least 8 digits long, so it can only ever match a build-date run and never a fragment of a fingerprint.

[r0x0r]

MMDDYYYY still never matches here.

normalize_requested_build_date (apps/rocm/src/therock.rs:3742) accepts YYYY-MM-DD, YYYYMMDD and MMDDYYYY, and there's a test using the MMDDYYYY spelling at therock.rs:8519. Stripping non-digits preserves order, so that form can't line up with the YYYYMMDD run the key carries. Running the function:

key_matches_pin("nightly-wheel-multi-arch-7-13-0a20260605-0123456789abcdef", "2026-06-05") == true
key_matches_pin("nightly-wheel-multi-arch-7-13-0a20260605-0123456789abcdef", "20260605")   == true
key_matches_pin("nightly-wheel-multi-arch-7-13-0a20260605-0123456789abcdef", "06052026")   == false

So a CI author who copies the MMDDYYYY spelling from rocm install sdk gets Decision::Install on every run — the same multi-GiB re-download this PR fixed, reached by a different spelling.

Either canonicalise the pin to YYYYMMDD before comparing (same branch normalize_requested_build_date uses: check whether the first or the last four digits start with 20), or reject anything but the advertised YYYY-MM-DD at the flag so the unsupported spellings fail loudly instead of silently re-downloading.

[juhovainio]

Addressed the review in bdc482eb: key_matches_pin's digits-only path now only fires for an exact 8-digit run (the shape a recovered build date actually has), so a short digit fragment from a --version pin (e.g. 7130 from 7.13.0) can no longer land inside an unrelated runtime's hex fingerprint suffix. It also reorders an MMDDYYYY-spelled --build-date to the key's YYYYMMDD order before comparing, and only matches a maximal digit run so it can't be a fragment of a longer, unrelated one. Added regression tests for both cases.