ci: verify the reboot, split machinery failures from test failures

Acts on pre-push review findings from codex + correctness/security subagents.

.github/workflows/package-currency.yml

@@ -34,9 +34,12 @@ on:
         type: boolean
         default: false
 
+# packages: write is granted per-job to container-image, the only job that
+# pushes. The self-hosted jobs get read-only tokens: they SSH into freshly
+# upgraded nodes and should not be able to overwrite published GHCR images.
 permissions:
   contents: read
-  packages: write
+  packages: read
 
 env:
   SCRIPTS_DIR: tests/infrastructure/scripts
@@ -74,6 +77,9 @@ jobs:
   container-image:
     runs-on: ubuntu-latest
     timeout-minutes: 20
+    permissions:
+      contents: read
+      packages: write
     steps:
       - uses: actions/checkout@v7
 
@@ -107,7 +113,12 @@ jobs:
         - /opt/pve-integration:/opt/pve-integration
 
     steps:
+      # persist-credentials: false on the self-hosted jobs — nothing downstream
+      # uses git, and the token would otherwise be written into .git/config on
+      # a runner that outlives the job's container.
       - uses: actions/checkout@v7
+        with:
+          persist-credentials: false
 
       - name: Provision PVE instances and dist-upgrade
         shell: bash
@@ -156,6 +167,8 @@ jobs:
 
     steps:
       - uses: actions/checkout@v7
+        with:
+          persist-credentials: false
 
       - name: Download module artifact
         uses: actions/download-artifact@v8
@@ -163,24 +176,33 @@ jobs:
           name: module-currency
           path: ./publish/netstandard2.0/
 
-      # continue-on-error is the report-only decision made concrete: a module
-      # broken against current PVE must not turn this cron red.
+      # Report-only applies to TEST failures, not to the lane's own machinery.
+      # cmd_test returns 3 for genuine Pester failures and 4 when it cannot
+      # reach or authenticate to a node. A blanket continue-on-error would
+      # swallow 4 as well — and an unreachable node is exactly the symptom of a
+      # reboot gone wrong, so the lane would go green having learned nothing.
       - name: Run integration tests (PVE ${{ matrix.pve_version }})
         id: suite
-        continue-on-error: true
         shell: bash
         env:
           PVE_PASSWORD: ${{ secrets.PVE_TEST_PASSWORD }}
           MODULE_ARTIFACT: ./publish/netstandard2.0
-        run: bash ${SCRIPTS_DIR}/run-integration.sh test ${{ matrix.pve_version }}
-
-      - name: Record suite outcome
-        shell: bash
         run: |
-          echo "Suite outcome against current PVE: ${{ steps.suite.outcome }}"
+          set +e
+          bash ${SCRIPTS_DIR}/run-integration.sh test ${{ matrix.pve_version }}
+          rc=$?
+          echo "suite_rc=${rc}" >> "$GITHUB_OUTPUT"
+          if [ "${rc}" -eq 0 ]; then
+            echo "Suite passed against current PVE."
+          elif [ "${rc}" -eq 3 ]; then
+            echo "::warning title=Suite failed against current PVE::Report-only - tests failed (rc=3). See the uploaded results."
+          else
+            echo "::error title=Currency lane machinery failed::run-integration.sh exited ${rc}, which is not a test failure. The lane learned nothing about package currency."
+          fi
+          [ "${rc}" -eq 0 ] || [ "${rc}" -eq 3 ]
 
       - name: Diagnose cluster state
-        if: steps.suite.outcome == 'failure'
+        if: always() && steps.suite.outputs.suite_rc != '0'
         shell: bash
         env:
           PVE_PASSWORD: ${{ secrets.PVE_TEST_PASSWORD }}
@@ -209,6 +231,8 @@ jobs:
 
     steps:
       - uses: actions/checkout@v7
+        with:
+          persist-credentials: false
 
       - name: Cleanup PVE instances
         shell: bash

tests/infrastructure/scripts/prepare-test-environment.sh

@@ -35,25 +35,57 @@ if [[ "${DIST_UPGRADE}" == "1" ]]; then
 
     if [[ -n "${PKG_OUT}" ]]; then
         echo "Recording package set to ${PKG_OUT}..."
-        ${SSH_CMD} "dpkg-query -W -f='\${binary:Package}\t\${Version}\n' | sort" > "${PKG_OUT}"
+        # pipefail must be set in the REMOTE shell. ssh returns the remote
+        # pipeline's status, which is sort's — and sort succeeds on the empty
+        # input a failed dpkg-query produces, so a broken query would otherwise
+        # leave a zero-byte file and still exit 0.
+        ${SSH_CMD} "set -o pipefail; dpkg-query -W -f='\${binary:Package}\t\${Version}\n' | sort" > "${PKG_OUT}"
+        if [[ ! -s "${PKG_OUT}" ]]; then
+            echo "ERROR: empty package set from ${NESTED_IP}" >&2
+            exit 1
+        fi
     fi
 
     # A PVE dist-upgrade pulls proxmox-kernel-*; without a reboot the node runs
     # new userspace on the old kernel. Reboot unconditionally rather than
     # testing /var/run/reboot-required — that file comes from
     # update-notifier-common, which is not guaranteed on a PVE node.
+    #
+    # boot_id is the evidence that the reboot happened. Without it the `|| true`
+    # below swallows every ssh failure, the node stays up, and wait-for-api.sh
+    # matches the still-running pre-reboot pveproxy on its first poll.
+    boot_before="$(${SSH_CMD} "cat /proc/sys/kernel/random/boot_id")"
+
     echo "Rebooting after dist-upgrade..."
     ${SSH_CMD} "systemctl reboot" || true
 
-    # The API stays up for a few seconds after the reboot is issued, so polling
-    # immediately would match the pre-reboot node and return at once.
-    sleep 30
+    # Order matters: prove the reboot first (ssh returns before pveproxy does),
+    # then wait for the API, then for pmxcfs.
+    boot_after=""
+    for _ in $(seq 1 60); do
+        boot_after="$(${SSH_CMD} "cat /proc/sys/kernel/random/boot_id" 2>/dev/null || true)"
+        [[ -n "${boot_after}" && "${boot_after}" != "${boot_before}" ]] && break
+        sleep 5
+    done
+    if [[ -z "${boot_after}" || "${boot_after}" == "${boot_before}" ]]; then
+        echo "ERROR: ${NESTED_IP} did not reboot (boot_id unchanged)" >&2
+        exit 1
+    fi
+
     bash "${SCRIPT_DIR}/wait-for-api.sh" "${NESTED_IP}" 8006 600
 
-    # The running kernel is the point of the reboot: the package set alone
-    # cannot show whether the node actually booted what it installed.
+    # wait-for-api.sh only proves pveproxy answers. `pvesm set` below writes
+    # /etc/pve/storage.cfg, which needs pmxcfs to have mounted /etc/pve — on a
+    # freshly rebooted node those are seconds apart.
+    for _ in $(seq 1 30); do
+        ${SSH_CMD} "test -f /etc/pve/storage.cfg" 2>/dev/null && break
+        sleep 5
+    done
+
+    # printf, not echo: bash's builtin echo does not interpret \t without -e,
+    # which would make this the one row in the file without a real tab.
     if [[ -n "${PKG_OUT}" ]]; then
-        ${SSH_CMD} "echo \"# running-kernel\t\$(uname -r)\"" >> "${PKG_OUT}"
+        ${SSH_CMD} "printf '# running-kernel\t%s\n' \"\$(uname -r)\"" >> "${PKG_OUT}"
     fi
 fi
 

tests/infrastructure/scripts/prepare-test-environment.test.sh

@@ -1,9 +1,12 @@
 #!/usr/bin/env bash
 # Self-check for prepare-test-environment.sh's opt-in dist-upgrade branch.
 #
-# Stubs sshpass/curl/sleep on PATH so both paths run offline in ~0s, then
+# Stubs sshpass/curl/sleep on PATH so every path runs offline in ~0s, then
 # asserts on the commands the script actually issued.
 #
+# The stub is deliberately stateful: boot_id must differ across the reboot, and
+# case 3 pins the failure by returning the SAME boot_id twice.
+#
 # Run: bash tests/infrastructure/scripts/prepare-test-environment.test.sh
 
 set -euo pipefail
@@ -16,19 +19,45 @@ trap 'rm -rf "$TMP"' EXIT
 
 mkdir -p "$TMP/bin"
 
-# Fake sshpass: log every invocation, emit a plausible dpkg-query result.
+# Fake sshpass. Logs every invocation; emits plausible output per command.
+# BOOT_ID_STUCK=1 makes it return an unchanging boot_id, simulating a node that
+# never rebooted.
 cat > "$TMP/bin/sshpass" <<'STUB'
 #!/usr/bin/env bash
 echo "$*" >> "$STUB_LOG"
 case "$*" in
-    *dpkg-query*) echo -e "proxmox-kernel-6.14\t6.14.11-1\npve-manager\t9.2.1" ;;
+    *boot_id*)
+        if [[ "${BOOT_ID_STUCK:-0}" == "1" ]]; then
+            echo "11111111-1111-1111-1111-111111111111"
+        else
+            n=0
+            [[ -f "$STUB_STATE/boot_calls" ]] && n=$(cat "$STUB_STATE/boot_calls")
+            n=$((n + 1))
+            echo "$n" > "$STUB_STATE/boot_calls"
+            if [[ "$n" -le 1 ]]; then
+                echo "11111111-1111-1111-1111-111111111111"
+            else
+                echo "22222222-2222-2222-2222-222222222222"
+            fi
+        fi
+        ;;
+    *dpkg-query*)
+        # DPKG_EMPTY=1 simulates a failed query whose output is swallowed by
+        # the remote `| sort`, which succeeds on empty input.
+        [[ "${DPKG_EMPTY:-0}" == "1" ]] || printf 'proxmox-kernel-6.14\t6.14.11-1\npve-manager\t9.2.1\n'
+        ;;
+    *uname*)
+        printf '# running-kernel\t6.14.11-1-pve\n'
+        ;;
 esac
 exit 0
 STUB
 
-# wait-for-api.sh greps curl output for "version"; sleep must not really sleep.
+# wait-for-api.sh greps curl output for "version"; log the call so the test can
+# assert the wait actually ran. sleep must not really sleep.
 cat > "$TMP/bin/curl" <<'STUB'
 #!/usr/bin/env bash
+echo "curl $*" >> "$STUB_LOG"
 echo '{"data":{"version":"9.2.1"}}'
 STUB
 cat > "$TMP/bin/sleep" <<'STUB'
@@ -38,11 +67,13 @@ STUB
 
 chmod +x "$TMP/bin/"*
 export PATH="$TMP/bin:$PATH"
+export STUB_STATE="$TMP"
 
 fail=0
 check() {
     local desc="$1" haystack="$2" needle="$3" want="$4"
-    if grep -q -- "$needle" "$haystack"; then found=yes; else found=no; fi
+    local found=no
+    grep -q -- "$needle" "$haystack" && found=yes
     if [[ "$found" == "$want" ]]; then
         echo "  ok: $desc"
     else
@@ -50,41 +81,91 @@ check() {
         fail=1
     fi
 }
+pass() { echo "  ok: $1"; }
+fatal() { echo "  FAIL: $1"; fail=1; }
 
 echo "case 1: no dist-upgrade argument — lane 1 path must be untouched"
 export STUB_LOG="$TMP/log1"
-: > "$STUB_LOG"
+: > "$STUB_LOG"; rm -f "$TMP/boot_calls"
 bash "$TARGET" 10.0.0.1 secret > "$TMP/out1" 2>&1
 check "no dist-upgrade issued"      "$STUB_LOG" "dist-upgrade"    no
 check "no reboot issued"            "$STUB_LOG" "systemctl reboot" no
 check "no package set recorded"     "$STUB_LOG" "dpkg-query"      no
+check "no boot_id probe"            "$STUB_LOG" "boot_id"         no
 check "storage still configured"    "$STUB_LOG" "pvesm set local" yes
 
 echo "case 2: dist-upgrade requested"
 export STUB_LOG="$TMP/log2"
-: > "$STUB_LOG"
+: > "$STUB_LOG"; rm -f "$TMP/boot_calls"
 bash "$TARGET" 10.0.0.1 secret 1 "$TMP/packages.txt" > "$TMP/out2" 2>&1
 check "dist-upgrade issued"         "$STUB_LOG" "dist-upgrade"    yes
 check "reboot issued"               "$STUB_LOG" "systemctl reboot" yes
 check "package set recorded"        "$STUB_LOG" "dpkg-query"      yes
+check "boot_id checked"             "$STUB_LOG" "boot_id"         yes
+check "waited for the API"          "$STUB_LOG" "api2/json/version" yes
+check "waited for pmxcfs"           "$STUB_LOG" "/etc/pve/storage.cfg" yes
 check "storage still configured"    "$STUB_LOG" "pvesm set local" yes
 
+# The stub replaces the remote shell, so it cannot observe what that shell does
+# with a command — only which command was sent. These two assert at that level,
+# because both defects live in the command string itself:
+#   - without `set -o pipefail`, a failed remote dpkg-query is masked by `sort`,
+#     which succeeds on empty input and makes ssh return 0.
+#   - bash's builtin `echo` does not interpret \t without -e, so `echo` here
+#     would write the one row in the file lacking a real tab.
+check "query sets remote pipefail"  "$STUB_LOG" "set -o pipefail" yes
+check "kernel capture uses printf"  "$STUB_LOG" "printf '# running-kernel" yes
+
 # The reboot must be issued after the upgrade, or the node records a package
 # set it never booted.
 upgrade_line=$(grep -n "dist-upgrade" "$STUB_LOG" | head -1 | cut -d: -f1)
 reboot_line=$(grep -n "systemctl reboot" "$STUB_LOG" | head -1 | cut -d: -f1)
-if [[ "$reboot_line" -gt "$upgrade_line" ]]; then
-    echo "  ok: reboot ordered after dist-upgrade"
+api_line=$(grep -n "api2/json/version" "$STUB_LOG" | head -1 | cut -d: -f1)
+[[ "$reboot_line" -gt "$upgrade_line" ]] \
+    && pass "reboot ordered after dist-upgrade" \
+    || fatal "reboot ordered before dist-upgrade"
+[[ "$api_line" -gt "$reboot_line" ]] \
+    && pass "API wait ordered after reboot" \
+    || fatal "API wait ordered before reboot"
+
+# The running-kernel row must carry a REAL tab, like every dpkg-query row.
+# `echo "...\t..."` in bash emits a literal backslash-t and would fail here.
+if grep -q '^# running-kernel' "$TMP/packages.txt"; then
+    pass "running kernel recorded"
+    if grep -qP '^# running-kernel\t' "$TMP/packages.txt" 2>/dev/null \
+        || awk -F'\t' '/^# running-kernel/ && NF == 2 {found=1} END {exit !found}' "$TMP/packages.txt"; then
+        pass "running-kernel row uses a real tab"
+    else
+        fatal "running-kernel row has a literal backslash-t, not a tab"
+    fi
 else
-    echo "  FAIL: reboot ordered before dist-upgrade"
-    fail=1
+    fatal "running kernel not recorded"
 fi
 
-if [[ -s "$TMP/packages.txt" ]]; then
-    echo "  ok: package file non-empty"
+[[ -s "$TMP/packages.txt" ]] && pass "package file non-empty" || fatal "package file empty or missing"
+
+echo "case 3: node never rebooted — must be fatal"
+export STUB_LOG="$TMP/log3"
+: > "$STUB_LOG"; rm -f "$TMP/boot_calls"
+if BOOT_ID_STUCK=1 bash "$TARGET" 10.0.0.1 secret 1 "$TMP/packages3.txt" > "$TMP/out3" 2>&1; then
+    fatal "script exited 0 despite an unchanged boot_id"
 else
-    echo "  FAIL: package file empty or missing"
-    fail=1
+    pass "unchanged boot_id fails the run"
+    grep -q "did not reboot" "$TMP/out3" \
+        && pass "failure names the cause" \
+        || fatal "failure message does not mention the reboot"
+fi
+
+echo "case 4: dpkg-query produced nothing — must be fatal"
+export STUB_LOG="$TMP/log4"
+: > "$STUB_LOG"; rm -f "$TMP/boot_calls"
+if DPKG_EMPTY=1 bash "$TARGET" 10.0.0.1 secret 1 "$TMP/packages4.txt" > "$TMP/out4" 2>&1; then
+    fatal "script exited 0 despite an empty package set"
+else
+    pass "empty package set fails the run"
+    grep -q "empty package set" "$TMP/out4" \
+        && pass "failure names the cause" \
+        || fatal "failure message does not mention the package set"
 fi
 
 if [[ "$fail" -eq 0 ]]; then

tests/infrastructure/scripts/run-integration.sh

@@ -238,6 +238,11 @@ cmd_provision() {
     ci_mask "$PVE_PASSWORD"
     mkdir -p "$WORK_DIR" "$CACHE_DIR"
 
+    # WORK_DIR is a persistent shared mount. Stale package sets from an earlier
+    # run would otherwise be uploaded as if they were current — the artifact
+    # guard checks existence, not freshness.
+    rm -f "$WORK_DIR"/*-packages.txt
+
     # Ensure base ISOs (one per version, not per node)
     for v in $provision_versions; do
         log "Ensuring base ISO for PVE $v..."
@@ -523,7 +528,9 @@ cmd_test() {
             -d "username=root@pam&password=${PVETEST_PASSWORD}" \
             "https://${PVETEST_HOST}:${PVETEST_PORT}/api2/json/access/ticket" | grep -q '"ticket"'; then
             ci_error "Cannot authenticate to PVE $v node A at ${PVETEST_HOST}:${PVETEST_PORT}"
-            overall_exit=3
+            # 4, not 3: this is a machinery failure, not a test failure. The
+            # currency lane suppresses 3 (report-only) but must fail on 4.
+            overall_exit=4
             continue
         fi
 
@@ -534,7 +541,7 @@ cmd_test() {
                 -d "username=root@pam&password=${PVETEST_PASSWORD}" \
                 "https://${PVETEST_HOST_B}:${PVETEST_PORT}/api2/json/access/ticket" | grep -q '"ticket"'; then
                 ci_error "Cannot authenticate to PVE $v node B at ${PVETEST_HOST_B}:${PVETEST_PORT}"
-                overall_exit=3
+                overall_exit=4
                 continue
             fi
         fi
@@ -733,6 +740,7 @@ cmd_force_cleanup() {
     # Remove work artifacts, including locally cached auto-install ISOs
     rm -f "$CONFIG_FILE" "$WORK_DIR"/instances.tfvars.json "$TARGET_NODE_FILE"
     rm -f "$WORK_DIR"/*-auto-*.iso "$WORK_DIR"/*-http-auto.iso
+    rm -f "$WORK_DIR"/*-packages.txt
 
     log "Force cleanup complete. Next provision will start from scratch."
 }