ci: remove redundant work from the macOS and KVM lanes - #678
Conversation
The boot-e2e and autospawn-e2e jobs each independently checked out, installed the toolchain, brewed libkrun, downloaded the same three guest artifacts, compiled minvmd, and codesigned it — all on the single shared Apple Silicon runner. Merge them into one `e2e` job that builds and codesigns exactly once. Also removed as redundant: - the standalone boot_e2e run: Session E2E boots the identical kernel/rootfs/initramfs through the same HVF path and additionally asserts a real session (auth + exec, stdout + exit status), so a passing session gates the boot; MINVMD_BOOT_LOG moves onto it to keep a stuck boot diagnosable - the 10-boot informational latency benchmark (non-gating, output consumed by nothing, pure runner time) - the three "Verify libkrun" steps: a failed brew install already fails the job Net per run on the bottleneck runner: 1 build + 1 codesign instead of 3/2, 3 artifact downloads instead of 6, ~11 fewer VM boots. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Non-gating (`|| true`) 10-boot benchmark whose output nothing consumes — no artifact, no trend tracking, no threshold. Correctness stays gated by the boot/session/relay/lifecycle e2e steps. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughCI workflow files for macOS and Linux KVM pipelines were updated. The macOS workflow consolidates boot and session E2E jobs into a single unified ChangesCI Workflow Consolidation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/ci-macos.yml (1)
181-182: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider selecting the test binary from cargo's JSON output for robustness.
ls -1t ... | grep -v '\.d$' | head -1picks the newest matching entry by mtime. On a persistent self-hosted runner this is usually the freshly built binary, but it can also match stale artifacts or (if split-debuginfo is packed) aminimald_session_e2e-*.dSYMdirectory —test -xsucceeds on directories, so the guard wouldn't catch that. Selecting the executable fromcargo test --no-run --message-format=json(theexecutablefield) removes the ambiguity.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci-macos.yml around lines 181 - 182, The test binary selection in the CI macOS workflow is too ambiguous because the current ls/grep/head pipeline can pick stale artifacts or a non-executable dSYM directory. Update the binary discovery logic in the workflow step to use cargo’s JSON output from the minimald_session_e2e test build and select the executable field instead, so the test runner always uses the actual built binary. Keep the existing check and error handling around the testbin variable, but source testbin from the cargo --message-format=json result rather than filesystem ordering.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/ci-macos.yml:
- Around line 181-182: The test binary selection in the CI macOS workflow is too
ambiguous because the current ls/grep/head pipeline can pick stale artifacts or
a non-executable dSYM directory. Update the binary discovery logic in the
workflow step to use cargo’s JSON output from the minimald_session_e2e test
build and select the executable field instead, so the test runner always uses
the actual built binary. Keep the existing check and error handling around the
testbin variable, but source testbin from the cargo --message-format=json result
rather than filesystem ordering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 090bc543-58b2-46c2-adde-20d1d417c2fb
📒 Files selected for processing (2)
.github/workflows/ci-linux-kvm.yml.github/workflows/ci-macos.yml
…ipt (#685) * ci: extract shared setup composites for rust, libkrun, and guest artifacts Four new composite actions dissolve copy-pasted setup blocks: - setup-rust: free-disk + protoc + rust-cache preamble, previously inlined in ci.yml clippy AND duplicated inside core-tests - setup-libkrun-macos: the brew slp/krun tap install, previously copy-pasted across ci-macos.yml and release.yml (the release job's separate dylib-existence check is dropped like ci-macos's was in #678: a failed brew install already fails the step, and a missing libkrun still fails loudly at link/rewrite time) - setup-libkrun-linux: fetch-libkrun.sh + the LIBKRUN_PREFIX / LD_LIBRARY_PATH exports, previously duplicated between ci-linux-kvm.yml and release.yml's amd64 build - guest-artifacts: the kernel + rootfs cache pulls, previously repeated in ci-macos.yml, ci-linux-kvm.yml, and release.yml The two fetch composites retry 3x to absorb transient cache/network hiccups; commands, arguments, and env are otherwise unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci(release): extract minvmd linkage rewrite into a script The ~60-line inline install_name_tool rewrite + otool verification in build-release-macos-arm64 (grown across #664/#668/#670) moves to scripts/rewrite-macos-linkage.sh, command-for-command. A script lets CI exercise the production @rpath rewrite on a throwaway copy of the debug binary later, instead of the rewrite only ever running at release time. Signing stays the caller's job (Developer ID, last mutation before upload, per #680). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci(setup-minimal): drop dead cache-minimal install conditional The Install Minimal step guarded on steps.cache-minimal.outputs.cache-hit, but no step with id cache-minimal exists (leftover from a removed cache step), so the condition was always true. Remove it; the step runs unconditionally as it already did in practice. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(scripts): harden rewrite-macos-linkage.sh Three latent bugs surfaced by review once the previously-inline code became a parameterized, reusable script: - otool -L's header line is the binary's own path; a binary living under a path containing "libkrun" (e.g. a throwaway CI copy) would false-match and silently skip the real rewrite. Skip the header (NR>1). - install_name_tool -rpath errors when the bare @loader_path entry is already gone, so a second run on an already-rewritten binary hard-failed. Retarget only when the dev rpath is present. - set -o pipefail aborted the `current=` capture before the annotated "no libkrun load command" diagnostic could fire when otool itself fails. Guard the capture with || true; the -z check handles both. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci: address review findings in shared composites and their callers - Trigger the VM lanes when their composite actions change: ci-macos.yml and ci-linux-kvm.yml path filters now include the composites their setup runs through, so a composite-only edit can no longer merge unexercised and break the lanes post-merge. - Restore the libkrun.dylib existence check in setup-libkrun-macos: brew exits 0 for an installed-but-unlinked keg, so "a failed install fails the step" was unsound on a persistent shared runner; without the check a release build dies later with a raw `ld: library 'krun' not found` instead of a provisioning pointer. - Fail fast on deterministic errors: the mip CLI is validated/built once, outside the retry loops (guest-artifacts, setup-libkrun-linux), so a compile error or bad mip path fails in one attempt instead of being re-run into the job timeout. Only the network-bound materialize retries. - One retry implementation: scripts/ci/retry.sh replaces the three hand-copied divergent loops, and the previously-unretried gvproxy downloads (ci-linux-kvm + release) now use it too. - release.yml: drop the inline LIBKRUN_PREFIX re-hardcode (the composite publishes it); document why the job-wide LD_LIBRARY_PATH is safe (the prefix holds only libkrun/libkrunfw by construction); fix the linkage-step comment that implied a CI consumer exists. Not fixed: ci-netns.yml still inlines its rust preamble — its free-disk config differs (no remove_tool_cache) and the file is slated for deletion when the networking proofs are mothballed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci: drop the fetch retries — lane history shows zero fetch failures Actions history for both VM lanes (last ~200 runs each, plus the first attempts of every manually re-run run) records not a single failure in the retried steps: the kernel/rootfs cache pulls, the libkrun prefix fetch, and the gvproxy download have never failed. The retries were insurance without receipts; remove them and the now-unused scripts/ci/retry.sh, returning the composites to plain extractions of the original steps (the mip prebuild and input pre-validation existed only to keep deterministic work out of the retry loops, so they go too). The transient failures the history DOES show are apt-get installs — the only main-branch KVM lane failure in the window and both of its manual reruns died in "Install build dependencies". Retry belongs there if anywhere, left for a separate change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci(macos): reap the whole Session-E2E process tree before auto-spawn The __krun-vmm-only reap from #672 is insufficient: main run 29032763009 — on the very commit that added it — still failed the cold `minimal ls` with `ssh connect: Disconnected` against a fully healthy guest (READY emitted, minimald listening on vsock:2222, no connection ever accepted): the #588 bridge wedge. The remaining leftover is the host gvproxy switch, which minvmd owns and Guest::drop never kills; the failing runs' proxy-publish WARN corroborates a lingering gvproxy. Reap any stray minvmd first (so nothing respawns), then the VMM and gvproxy. The proper fix — the session harness reaping its own process group — stays tracked under #588; this keeps the merged e2e job's steps isolated in the meantime. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
* ci: extract shared setup composites for rust, libkrun, and guest artifacts Four new composite actions dissolve copy-pasted setup blocks: - setup-rust: free-disk + protoc + rust-cache preamble, previously inlined in ci.yml clippy AND duplicated inside core-tests - setup-libkrun-macos: the brew slp/krun tap install, previously copy-pasted across ci-macos.yml and release.yml (the release job's separate dylib-existence check is dropped like ci-macos's was in #678: a failed brew install already fails the step, and a missing libkrun still fails loudly at link/rewrite time) - setup-libkrun-linux: fetch-libkrun.sh + the LIBKRUN_PREFIX / LD_LIBRARY_PATH exports, previously duplicated between ci-linux-kvm.yml and release.yml's amd64 build - guest-artifacts: the kernel + rootfs cache pulls, previously repeated in ci-macos.yml, ci-linux-kvm.yml, and release.yml The two fetch composites retry 3x to absorb transient cache/network hiccups; commands, arguments, and env are otherwise unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci(release): extract minvmd linkage rewrite into a script The ~60-line inline install_name_tool rewrite + otool verification in build-release-macos-arm64 (grown across #664/#668/#670) moves to scripts/rewrite-macos-linkage.sh, command-for-command. A script lets CI exercise the production @rpath rewrite on a throwaway copy of the debug binary later, instead of the rewrite only ever running at release time. Signing stays the caller's job (Developer ID, last mutation before upload, per #680). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci(setup-minimal): drop dead cache-minimal install conditional The Install Minimal step guarded on steps.cache-minimal.outputs.cache-hit, but no step with id cache-minimal exists (leftover from a removed cache step), so the condition was always true. Remove it; the step runs unconditionally as it already did in practice. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(scripts): harden rewrite-macos-linkage.sh Three latent bugs surfaced by review once the previously-inline code became a parameterized, reusable script: - otool -L's header line is the binary's own path; a binary living under a path containing "libkrun" (e.g. a throwaway CI copy) would false-match and silently skip the real rewrite. Skip the header (NR>1). - install_name_tool -rpath errors when the bare @loader_path entry is already gone, so a second run on an already-rewritten binary hard-failed. Retarget only when the dev rpath is present. - set -o pipefail aborted the `current=` capture before the annotated "no libkrun load command" diagnostic could fire when otool itself fails. Guard the capture with || true; the -z check handles both. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci: address review findings in shared composites and their callers - Trigger the VM lanes when their composite actions change: ci-macos.yml and ci-linux-kvm.yml path filters now include the composites their setup runs through, so a composite-only edit can no longer merge unexercised and break the lanes post-merge. - Restore the libkrun.dylib existence check in setup-libkrun-macos: brew exits 0 for an installed-but-unlinked keg, so "a failed install fails the step" was unsound on a persistent shared runner; without the check a release build dies later with a raw `ld: library 'krun' not found` instead of a provisioning pointer. - Fail fast on deterministic errors: the mip CLI is validated/built once, outside the retry loops (guest-artifacts, setup-libkrun-linux), so a compile error or bad mip path fails in one attempt instead of being re-run into the job timeout. Only the network-bound materialize retries. - One retry implementation: scripts/ci/retry.sh replaces the three hand-copied divergent loops, and the previously-unretried gvproxy downloads (ci-linux-kvm + release) now use it too. - release.yml: drop the inline LIBKRUN_PREFIX re-hardcode (the composite publishes it); document why the job-wide LD_LIBRARY_PATH is safe (the prefix holds only libkrun/libkrunfw by construction); fix the linkage-step comment that implied a CI consumer exists. Not fixed: ci-netns.yml still inlines its rust preamble — its free-disk config differs (no remove_tool_cache) and the file is slated for deletion when the networking proofs are mothballed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci: drop the fetch retries — lane history shows zero fetch failures Actions history for both VM lanes (last ~200 runs each, plus the first attempts of every manually re-run run) records not a single failure in the retried steps: the kernel/rootfs cache pulls, the libkrun prefix fetch, and the gvproxy download have never failed. The retries were insurance without receipts; remove them and the now-unused scripts/ci/retry.sh, returning the composites to plain extractions of the original steps (the mip prebuild and input pre-validation existed only to keep deterministic work out of the retry loops, so they go too). The transient failures the history DOES show are apt-get installs — the only main-branch KVM lane failure in the window and both of its manual reruns died in "Install build dependencies". Retry belongs there if anywhere, left for a separate change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci(macos): reap the whole Session-E2E process tree before auto-spawn The __krun-vmm-only reap from #672 is insufficient: main run 29032763009 — on the very commit that added it — still failed the cold `minimal ls` with `ssh connect: Disconnected` against a fully healthy guest (READY emitted, minimald listening on vsock:2222, no connection ever accepted): the #588 bridge wedge. The remaining leftover is the host gvproxy switch, which minvmd owns and Guest::drop never kills; the failing runs' proxy-publish WARN corroborates a lingering gvproxy. Reap any stray minvmd first (so nothing respawns), then the VMM and gvproxy. The proper fix — the session harness reaping its own process group — stays tracked under #588; this keeps the merged e2e job's steps isolated in the meantime. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci(macos): consolidate libkrun on our own pinned source build CI previously tested against the slp/krun Homebrew bottle (a full-feature build, version drifting with the tap) while the release shipped our trimmed source build (blk,net; no gpu, no init-blob) — which nothing executed before it reached users. Consolidate both on one build: - vendor/libkrun/libkrun.lock pins the containers/libkrun version AND its resolved commit (replaces the LIBKRUN_REF env pin; the build fetches the commit, so a moved tag cannot change what we build). - scripts/build-libkrun-macos.sh builds the trimmed dylib at the pin, asserts self-containment and the krun_add_disk3 (>= 1.19.0) API floor, sets the install name to @rpath/libkrun.1.dylib, ad-hoc signs, and stages libkrun.1.dylib + a libkrun.dylib linker symlink into a prefix. - setup-libkrun-macos builds on miss into a commit-keyed prefix ($HOME/.cache/minimal-ci/libkrun/<commit>) — actions/cache on GitHub-hosted runners, the persistent directory itself on the mini — verifies the staged dylib, and publishes LIBKRUN_PREFIX. minvmd's build.rs prefers LIBKRUN_PREFIX over /opt/homebrew and bakes it as an rpath, so a leftover brew libkrun on the runner is ignored; brew leaves the CI path entirely (and with it the installed-but-unlinked keg failure mode). - release build-libkrun-macos-arm64 ships from the same composite: the shipped dylib is by construction the one every macOS CI lane linked and booted. Developer ID signing flow unchanged (#680). With the @rpath install name, minvmd records @rpath/libkrun.1.dylib at link time — verified locally: the script builds a self-contained dylib (Hypervisor.framework/libiconv/libSystem only), minvmd links with the @rpath load command + @loader_path and prefix rpaths, and the binary loads. rewrite-macos-linkage.sh's -change becomes a natural no-op; only its @loader_path -> @loader_path/../lib retarget still mutates release binaries. Refs: #687 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci(macos): address CodeRabbit review on the libkrun build - Key the on-disk libkrun prefix by BOTH build inputs (pinned commit + build-script hash), not commit alone: the persistent mini could otherwise keep serving a dylib built by an older build-libkrun-macos.sh after a script change. This mirrors the invalidation the actions/cache key already gave GitHub-hosted runners; stale sibling prefixes on the mini are inert. - cargo build --locked: build exactly upstream's committed Cargo.lock (verified present at the pin) so a silent dependency re-resolve cannot undermine the reproducible-build guarantee. Verified locally: the --locked build completes at the pin and stages into the new hash-suffixed prefix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci(macos): build minimal in its own cargo invocation in the e2e job The e2e's combined `cargo build -p minvmd --bin minvmd -p minimal --bin minimal` unified minvmd's default `libkrun` feature into the CLI's default-features=false opt-out, so `minimal` linked libkrun — the exact footgun release.yml already documents and avoids with separate invocations. The brew era masked it: brew's libkrun carries an absolute /opt/homebrew install name, so the mislinked CLI still loaded. The own-build dylib's @rpath install name exposed it — the autospawn e2e died with dyld "Library not loaded: @rpath/libkrun.1.dylib / no LC_RPATH's found" from target/debug/ minimal, which bakes no rpaths. Split the build (mirroring release.yml) and add the release job's "minimal links only system libraries" assert to the e2e, so a unification regression fails at build time with a pointed message instead of a dyld error mid-test. Net effect of this PR's own-build switch: CI now catches a mislinked CLI that brew silently tolerated. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
What
Immediate, coverage-neutral cleanup of the Mac CI path (plus one step in the KVM lane), extracted from a broader CI-refactor review.
ci-macos.ymlboot-e2e+autospawn-e2einto onee2ejob. Each job previously did its own checkout, toolchain,brew install libkrun, the same three artifact downloads, a minvmd compile, and a codesign — on the single shared Apple Silicon runner. The merged job builds everything (session harness + minvmd + minimal) before one codesign, preserving the sign-last ordering constraint (cargo after codesign relinks and unsigns minvmd →krun_start_enterEINVAL).Boot E2Estep. Session E2E boots the identical kernel/rootfs/initramfs through the same HVF path and goes further (russh auth + exec, stdout + exit status) — a passing session gates the boot.MINVMD_BOOT_LOGmoves onto the session step so a stuck boot stays diagnosable; both console logs upload as oneminvmd-boot-logsartifact.|| true, output consumed by nothing).Verify libkrunsteps — a failedbrew installalready fails the job.ci-linux-kvm.ymlWhy
Per mac-path PR on the bottleneck runner: 1 build + 1 codesign instead of 3/2, 3 artifact downloads instead of 6, ~11 fewer VM boots (10 bench + 1 standalone boot), two fewer checkout/brew cycles. Zero coverage lost: boot correctness, session round-trip, and the R4.5 autospawn cold/warm proofs all still gate.
Verification
Both edited files are in their own workflows' path filters, so this PR's own run exercises the merged
e2ejob on the mini and the KVM lane. Check the mac job log for exactly one cargo build phase and one codesign, Session E2E green withboot.logcaptured, and autospawn cold/warm green.actionlintpasses with no new warnings (the two remaining shellcheck notes pre-exist onmain).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes