fix(installer): stop the ROCm package lookup from aborting the installer

[LukasWodka]

Summary

_find_package_name in scripts/lib/gpu-amd.sh ran the fetch, the match and head -1 as one pipeline and returned its status. scripts/install-k8s.sh sources this lib under set -euo pipefail, so that pipeline could take the installer down two different ways:

1. A failed fetch aborted the installer before the friendly error could report it. On a 404, timeout or proxy block the command substitution is non-zero, the caller's assignment inherits that status, and set -e aborts — before the [[ -z "$deb_name" ]] && error "No amdgpu-install .deb found at …" sitting on the very next line. The operator saw a silent abort mid-GPU-step instead of an actionable message. The RHEL major-version fallback (/rhel/9/ after /rhel/9.4/ misses) was unreachable for the same reason — the first empty result killed the script rather than falling through.

2. head -1 could SIGPIPE grep, failing a lookup that had actually succeeded. head -1 closes the pipe as soon as it has its line; if grep is still writing it takes SIGPIPE (141), and pipefail propagates that as a pipeline failure even though the filename was found. This only triggers once grep's output exceeds the pipe buffer, so it fails on large mirror indexes and passes everywhere else — the worst failure mode to leave in an installer.

The fix

Capture the fetch and the match separately, let neither fail the function, and take the first match with ${matches%%$'\n'*} so head leaves the pipeline entirely:

_find_package_name() {
  local dir_url="$1" ext="$2" index="" matches=""
  index="$(curl_secure -fsSL --max-time 30 "$dir_url")" || return 0
  matches="$(printf '%s\n' "$index" | grep -oE "amdgpu-install[^\"<>]*\.${ext}")" || return 0
  printf '%s\n' "${matches%%$'\n'*}"
}

Chosen over sprinkling || true, per the ticket discussion: || true on the original pipeline would have silenced the fetch failure but left the head/SIGPIPE interaction in place. The contract is unchanged — the filename on stdout, nothing at all when not found — so emptiness stays the single signal all three call sites already test for, and each one now reaches its own error message.

No set -euo pipefail added to the lib: scripts/lib/*.sh deliberately have none (the entrypoint sets it at install-k8s.sh:44).

Tests

Adds scripts/tests/gpu-amd.bats — the first coverage this lib has had. Seven cases: the three contract cases (finds the file, returns only the first of several, honours the requested extension), both hazards, and a caller-shaped regression that runs the real assignment under set -euo pipefail and asserts the error path is reached rather than the shell dying.

Mutation-checked. Reverting the function to its previous form fails exactly the two hazard tests (6 and 7) while the five contract tests keep passing — so the new tests detect these defects, and the contract tests independently confirm the happy path is unchanged.

The large-index test generates 20 000 matches so the SIGPIPE case is deterministic here, rather than depending on a real mirror's index size.

Verification

• bats scripts/tests/gpu-amd.bats — 7/7 pass
• bats scripts/tests/*.bats — 427 pass. Three failures (validate_config: valid config passes, un-stamped DEFAULT_REF fails closed, _extract_yaml_value: single-quoted with '' escape) are pre-existing: identical by name on the unmodified base branch, and local-environment artifacts on macOS bash 3.2 — the suite's own notes say Linux CI is the authority.
• shellcheck --severity=error and --severity=warning clean; bash -n clean
• bash scripts/check-style.sh — style + terminology clean
• scripts/gen-manifest.sh re-run; scripts/manifest.sha256 committed (R8 supply-chain gate)

Base branch — please note

This is stacked on fix/1252-curl-secure-wrapper (#400), not on develop, because #400 rewrites the same line to use curl_secure. Targeting develop would have produced a guaranteed conflict on that line, and a careless resolution could silently drop the TLS floor #400 exists to add. Merge #400 first; GitHub will retarget this to develop automatically. The hazard itself predates #400 — it is present on develop today, just with plain curl.

Found while fixing backend#1252 in #400 and deliberately left out of that PR to keep it behaviour-neutral.

---

Note

Medium Risk
Touches the GPU install path in the k8s installer; behavior is intentionally narrowed to always exit 0 with empty output on failure, which is well covered by new tests but still affects production ROCm setup.

Overview
_find_package_name in scripts/lib/gpu-amd.sh no longer runs fetch, grep, and head -1 as one pipeline. The index fetch and regex match are captured in separate steps, each allowed to fail without exiting the function (|| return 0), so callers under set -euo pipefail still get empty stdout instead of a silent abort. The first match is taken with ${matches%%$'\n'*} instead of head -1, avoiding SIGPIPE/pipefail failures on large mirror listings while keeping the same contract (filename on stdout, empty when not found).

Adds scripts/tests/gpu-amd.bats with seven cases covering happy path, extension filtering, failed fetch, no match, large-index pipefail, and a caller-shaped set -euo pipefail regression. Updates scripts/manifest.sha256 for the changed lib.

^{Reviewed by Cursor Bugbot for commit aeb22fe. Bugbot is set up for automated code reviews on this repo. Configure here.}

[LukasWodka]

bugbot run

[cursor]

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit b1b6dec. Configure here.}

[LukasWodka]

Rebased onto the base branch's round-2 commit (f337c15, the kubectl stall-bound fix) so CI validates the combined manifest rather than a state one commit behind — scripts/manifest.sha256 is a line-per-file list, so both hash updates coexist, and gen-manifest.sh --check reports up to date on the merged result. Re-verified after the rebase: gpu-amd.bats 7/7, setup-linux.bats green, check-style.sh clean, shellcheck --severity=error clean.

bugbot run

[LukasWodka]

Correction to the "Base branch" note above

I wrote that "GitHub will retarget this to develop automatically" once #400 merges. That is wrong for this repo. GitHub's auto-retarget fires when the base branch is deleted on merge, and tracebloc/client has delete_branch_on_merge: false. So after #400 merges, this PR will keep pointing at a stale, already-merged branch until someone changes it.

Second, this repo squash-merges (every recent develop commit carries a (#N) suffix). After #400 is squashed into one commit, this branch's history no longer matches develop, so a plain retarget would show #400's changes over again.

So the actual sequence after #400 merges is two steps, not zero:

1. retarget this PR's base to develop, and
2. rebase fix/gpu-amd-pipefail-hazard onto develop so the diff collapses to just the gpu-amd.sh change and its bats file.

I'll do both as soon as #400 lands — no reviewer action needed beyond merging #400 first. Flagging it here so nobody merges this into a dead branch in the meantime.

Nothing can go wrong by accident, which is the point of the stack: this PR's base is #400's branch, so merging it can only ever land these commits there — never in develop ahead of the TLS fix.

The reason for stacking rather than conflicting still stands: #400 rewrites the exact line this PR rewrites, and the merge-conflict resolution would sit on top of the curl_secure call that #400 exists to introduce.

[divyasinghds]

Approve.

_find_package_name split into separate captures (index="$(curl_secure …)" || return 0, matches="$(… grep …)" || return 0, first line via ${matches%%$'\n'*}) genuinely fixes both set -e/pipefail hazards:

• A failed fetch / no-match now returns 0-with-empty output, so callers reach their own [[ -z … ]] && error (and the RHEL major-version fallback is reachable again).
• Dropping head -1 removes the SIGPIPE-on-large-index failure that only bit on big mirrors.

Contract preserved — the empty return happens before printf, so no bare newline is emitted when nothing is found. First test coverage this lib has had (7 cases), and it's mutation-checked: reverting the function fails exactly the two hazard tests while the contract tests stay green.

⚠️ Merge-order: this is stacked on fix/1252-curl-secure-wrapper (#400), not develop — merge #400 first and GitHub will retarget this automatically. The hazard predates #400; #400 just rewrites the same line to curl_secure.

Reviewed with Claude Code.

[LukasWodka]

bugbot run

[cursor]

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit 27d9429. Configure here.}

[aptracebloc]

Reviewed — the fix itself is approve-worthy, but I'm holding because the PR can't be reviewed or merged in its current shape.

The fix (fa0f49b, _find_package_name in gpu-amd.sh) is correct. It fixes both real hazards of running fetch → grep → head -1 as one pipeline under the entrypoint's set -euo pipefail:

1. a failed fetch aborting the installer before the [[ -z ]] && error on the next line (and killing the RHEL major-version fallback); and
2. head -1 SIGPIPE-ing grep on large mirror indexes, failing a lookup that succeeded.

Capturing the fetch and the match separately (each || return 0) and taking the first match with ${matches%%$'\n'*} — so head leaves the pipeline entirely — is the right fix, the contract is preserved (filename or empty, the single signal all three callers test), and gpu-amd.bats adds the lib's first coverage, including a caller-shaped regression under set -euo pipefail that asserts the error path is reached rather than the shell dying. Bugbot clean. 👍

Why I can't approve the branch as-is:

• Base is fix/1252-curl-secure-wrapper, not develop.
• CONFLICTING/DIRTY, and the diff is polluted: 29 files / +830, carrying #398 (already on develop), #400 (curl TLS floor), a kubectl-fetch fix, and a "Merge develop into…" commit — for what is really a ~25-line change plus its test. That isn't a diff anyone can meaningfully review or merge.

Recommend: retarget to develop and rebase so the diff is just the ROCm fix (fa0f49b) + gpu-amd.bats (and decide whether the kubectl-fetch fix rides along or splits into its own PR). Once it's a clean single-purpose diff against develop, this is an easy approve — the underlying change is solid.

— drafted with Claude (Opus 4.8), sent by @aptracebloc

[LukasWodka]

bugbot run

[cursor]

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit ca65b12. Configure here.}

The base branch was changed.

[divyasinghds]

Re-approve. #400 merged, this PR auto-retargeted onto develop, and the rebase (fa0f49bc → ca65b12b) dismissed my prior approval. Re-verified the rebased diff: the _find_package_name fix, the gpu-amd.bats tests, and the manifest are byte-identical to what I approved — the develop rebase changed nothing here.

⚠️ Before merge: get the installer-tests suite to run. It has never run on this PR. installer-tests.yaml only triggers on pull_request to [main, develop, openshift], but this PR's base was fix/1252-curl-secure-wrapper until #400 merged, and the automatic base-retarget doesn't fire a fresh run — so only pii-gate + Bugbot have executed, and the new gpu-amd.bats cases have never been exercised in CI. A single push to the head (e.g. an empty commit or a re-run of installer-tests) will trigger the full suite now that the base is develop. (helm-ci correctly skips — no client/** paths touched.)

Code-wise this is good to go; I just don't want the bats coverage to land unrun.

Reviewed with Claude Code.