Skip to content

fix(packaging): report rootfs extraction failures in the arch smoke test - #2137

Open
nullPointerEnjoyer wants to merge 2 commits into
masterfrom
fix/smoke-arch-reporting
Open

nullPointerEnjoyer wants to merge 2 commits into
masterfrom
fix/smoke-arch-reporting

Conversation

@nullPointerEnjoyer

Copy link
Copy Markdown
Collaborator

Summary

Fixes the findings from the review of #2134, which are pre-existing in
packaging/checks/smoke-arch.sh as shipped in v1.4.1 (that PR only realigns
master with the release branch and intentionally carries no fixes).

Merge after #2134 — commit 1 (326a28a) syncs the script with the
release-v1.4.1 version so this PR's diff shows only the fixes once #2134
lands; until then the diff includes the sync.

Fixes (all on smoke-arch.sh):

  • tar extraction failures are fatal: the previous 2>/dev/null || true
    silently passed the smoke test on a corrupt/truncated rootfs. Any non-zero
    tar exit now fails the run with the exit code logged.
  • chroot cleanup on failure: an EXIT trap removes the chroot and tarball
    on every path; INT/TERM mapped to clean exits. The chroot's exit code is
    propagated.
  • SIGPIPE-safe package checks: the pacman -Ql | grep -q man-page checks
    race with set -o pipefail; the file list is captured first and grepped
    from a variable.
  • argument validation: usage message with exit 64 for wrong arg counts or
    an invalid kind (node/gui), before any state is touched.
  • rootfs verification: the script downloads the mutable rolling
    ArchLinuxARM-aarch64-latest.tar.gz and executes it as root in the chroot,
    with no verification. It now pins a sha256 (bump deliberately, same policy
    as packaging/images.env), verifies each mirror's download before use, and
    falls through to the next mirror on failure or mismatch (the computed hash
    is logged). ALARM_ROOTFS_URL/ALARM_ROOTFS_SHA256 overrides for local
    testing are visible as job-log warnings.
  • download hardening: https-only, retries with a bounded cumulative
    window (--retry-max-time), connect/transfer timeouts.
  • binfmt pre-flight: the emulated chroot needs qemu-aarch64 registered
    with F (fix-binary) flags; the script checks and fails fast with a precise
    message instead of dying mid-pacman (skipped with a warning when
    binfmt_misc is not mounted, e.g. plain docker run).
  • chroot /dev: the rootfs's device nodes are excluded by the tarball's
    own design decision (--exclude='./dev/*'); the essential nodes
    (null, zero, random, urandom) are recreated with mknod so the
    chroot's redirects and hooks work.
  • pacman capability probe: a functional probe
    (pacman --disable-sandbox -Sh, needs pacman >= 6.1) replaces help-text
    matching, with probe output included in the failure message.

Notes / deliberate non-changes

  • The sha256 pin requires a deliberate bump when upstream rebuilds the
    rolling artifact — that is the point of the pin; the failure message says
    so.
  • Follow-ups (repo infra, not this script): centralize the rootfs pin in
    packaging/images.env if more consumers appear; wire shellcheck + a shell
    test harness into CI; per-run unique paths or a lockfile if the node/gui
    smoke tests are ever run concurrently in one container.

Test plan

  • bash -n; behavioral checks: usage/exit-64 paths (no args, wrong counts,
    bad kind), binfmt pre-flight (exact qemu-aarch64 entry, enabled state,
    F flag), download-loop fallthrough (download-failed vs hash-mismatch
    warnings), override warnings (each var independently)
  • rootfs download + sha256 verification exercised against the live mirror
    (829 MB, hash matches the mirror's published md5 cross-check)
  • ./do_checks.sh green
  • OCR review to 0 findings

Brings in the chroot-based foreign-architecture smoke test shipped in
v1.4.1 (via the release realignment merge), so the reporting fixes on
top apply to the code that actually runs in CI.
@github-actions

github-actions Bot commented Oct 8, 2026 •

Copy link
Copy Markdown

🔍 OpenCodeReview found 5 issue(s) in this PR.

  • ✅ Successfully posted inline: 2 comment(s)
  • 📋 Routed to summary by policy: 3 comment(s)

maintainability · low

📄 packaging/checks/smoke-arch.sh (L221-L223)

⚠️ GitHub could not post this as an inline comment: Routed to summary (severity low · category maintainability)

The cleanup trap is installed after the initial 'rm -rf' + 'mkdir -p', so a failure (or signal) landing between the mkdir and the trap line — or between tar extraction start and trap setup, since tar runs after this — is fine here only because the trap happens to be registered before tar; but the initial rm/mkdir pair duplicates what the EXIT trap would do and can leak a partial directory if this block is ever reordered (e.g. mkdir moved above the traps). Registering the traps before touching $CHROOT_DIR makes the 'reclaim on every exit path' guarantee structural rather than incidental.

💡 Suggested Change

Before:

trap cleanup EXIT
rm -rf "$CHROOT_DIR"
mkdir -p "$CHROOT_DIR"

After:

trap cleanup EXIT
rm -rf "$CHROOT_DIR"
mkdir -p "$CHROOT_DIR"
# (move the rm/mkdir below the trap registrations so any early failure is
# still cleaned up)

maintainability · low

📄 packaging/checks/smoke-arch.sh (L315-L317)

⚠️ GitHub could not post this as an inline comment: Routed to summary (severity low · category maintainability)

This loop hard-codes the assumption that the ARM rootfs ships keyrings at usr/share/pacman/keyrings/{archlinuxarm.gpg,-trusted,-revoked}. If upstream renames or repackages them, the failure surfaces as a bare 'cp: cannot stat' with no pointer at the rootfs pin. A clearer guard (test -f per file with a message referencing the pinned rootfs build) would make this failure mode self-explanatory.

💡 Suggested Change

Before:

for f in archlinuxarm.gpg archlinuxarm-trusted archlinuxarm-revoked; do
    cp "$CHROOT_DIR/usr/share/pacman/keyrings/$f" /usr/share/pacman/keyrings/
done

After:

for f in archlinuxarm.gpg archlinuxarm-trusted archlinuxarm-revoked; do
    keyring="$CHROOT_DIR/usr/share/pacman/keyrings/$f"
    test -f "$keyring" || { echo "ERROR: $f missing from the pinned ALARM rootfs; layout changed?" >&2; exit 1; }
    cp "$keyring" /usr/share/pacman/keyrings/
done

maintainability · low

📄 packaging/checks/smoke-arch.sh (L315-L317)

⚠️ GitHub could not post this as an inline comment: Routed to summary (severity low · category maintainability)

The archlinuxarm keyring files are copied into the host container's /usr/share/pacman/keyrings and never restored or removed. This mutation persists after the script exits: it silently overwrites any pre-existing archlinuxarm.gpg/-trusted/-revoked files in the host image, and it also makes the amd64 container's own pacman-key --populate aware of the archlinuxarm keyring, which is a side effect unrelated to this smoke test. Prefer copying into a temporary keyrings directory (e.g. /tmp/alarm-keyrings/) and pointing the host pacman-key --populate at it via that layout, or snapshot/restore the originals in the cleanup trap.

💡 Suggested Change

Before:

for f in archlinuxarm.gpg archlinuxarm-trusted archlinuxarm-revoked; do
    cp "$CHROOT_DIR/usr/share/pacman/keyrings/$f" /usr/share/pacman/keyrings/
done

After:

host_keyring_dir="$(mktemp -d)"
for f in archlinuxarm.gpg archlinuxarm-trusted archlinuxarm-revoked; do
    cp "$CHROOT_DIR/usr/share/pacman/keyrings/$f" "$host_keyring_dir/"
done
pacman-key --populate --config ... # or arrange for the temp dir to back /usr/share/pacman/keyrings
# and add "$host_keyring_dir" removal to the cleanup() trap

Comment on lines +159 to +162
if ! probe_out="$(pacman --disable-sandbox -Sh 2>&1)"; then
echo "ERROR: chroot pacman probe failed; if it rejected --disable-sandbox, pacman >= 6.1 is required. Probe output: $probe_out" >&2
exit 1
fi

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

bug · medium
The probe treats any non-zero exit as 'pacman rejected --disable-sandbox' but the same failure signature occurs for unrelated causes: no network for the -Sh sync-database path in an offline chroot, a corrupted rootfs pacman.conf, or a missing downloader binary. In those cases the operator is sent chasing a pacman-version problem that does not exist. Since the rootfs is sha256-pinned this is a narrow window, but including the probe output in the message (or a distinct hint) would save a debugging cycle.

Suggestion:

Suggested change
if ! probe_out="$(pacman --disable-sandbox -Sh 2>&1)"; then
echo "ERROR: chroot pacman probe failed; if it rejected --disable-sandbox, pacman >= 6.1 is required. Probe output: $probe_out" >&2
exit 1
fi
if ! probe_out="$(pacman --disable-sandbox -Sh 2>&1)"; then
echo "ERROR: chroot pacman probe failed (pacman >= 6.1 required for --disable-sandbox; network/config problems produce the same signature). Probe output: $probe_out" >&2
exit 1
fi

Comment on lines +304 to +305
rm -f "$CHROOT_DIR/etc/mtab"
cp /proc/self/mounts "$CHROOT_DIR/etc/mtab"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

maintainability · medium
This static copy of /proc/self/mounts is the host container's mount table (overlay root, /work bind mount, docker virtual filesystems), none of which exist inside the chroot. Modern pacman computes its free-space check with statvfs() on the target directory, so a wrong mtab is mostly harmless today, but if pacman (or any hook/script run during -U, e.g. ldconfig under emulation) ever consults it, the bogus entries could mislead diagnostics or trigger a spurious disk-space failure that is very hard to debug in CI. Consider documenting this explicitly here (or minimizing the table), since the comment currently implies the copy is a faithful stand-in.

Suggestion:

Suggested change
rm -f "$CHROOT_DIR/etc/mtab"
cp /proc/self/mounts "$CHROOT_DIR/etc/mtab"
rm -f "$CHROOT_DIR/etc/mtab"
# Host mount table; modern pacman uses statvfs() on the target dir, so this
# is a best-effort stand-in only — entries reference host paths absent from
# the chroot.
cp /proc/self/mounts "$CHROOT_DIR/etc/mtab"

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.

1 participant