fix(minimald,minimal): power the microVM off when the guest daemon stops - #734
Conversation
minimald is the microVM's pid-1, so returning from `main` after the
Shutdown RPC drained the server killed init: the kernel panicked
("Attempted to kill init!") and, with no `panic=` on the cmdline, spun
in the panic handler. The VMM child stayed alive on a dead guest, the
host kept reporting the VM as Running, and every later CLI command
blocked on a bridge socket nothing was behind — so `min stop --force`
bricked the daemon until the VM was killed by hand.
Power the VM off instead (sync + reboot(RB_POWER_OFF)), on the clean
and the failed exit alike: libkrun's VMM exits with the guest, the
supervisor reaps it and writes Stopped, and the next command autospawns
a fresh VM.
Two supporting changes on the CLI side:
- Bound the SSH handshake (10s). A connect is not proof of a live
daemon — libkrun's bridge accepts even when the guest behind it is
wedged — so any future guest wedge now errors instead of hanging.
minvmd's own client already guards this; the CLI's did not.
- `min stop` waits for the VM to reach a terminal lifecycle state
before returning, so the next command spawns a fresh VM rather than
racing a Running state whose VM is already going down.
The session e2e now asserts both halves: after `minimal stop` the VM is
Stopped, and the next `minimal ls` brings a daemon back.
Closes: #730
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 13 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe changes bound SSH handshake timeouts, wait for daemon shutdown completion, explicitly power off microVM guests, and verify that stopped VM targets can autospawn successfully. ChangesShutdown recovery flow
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
scripts/session-e2e.sh (1)
144-154: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
minvmd statusexit code check conflates "stopped" with "any non-zero exit."If
minvmd statusexits with a code other than 0/1 (e.g., binary missing, permission error), this branch treats it identically to "VM stopped" rather than surfacing the real failure.🤖 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 `@scripts/session-e2e.sh` around lines 144 - 154, The minvmd status check in the E2E VM validation must distinguish a stopped VM from command failures. Capture the exit status from minvmd status, accept 1 as the expected stopped state, retain the failure for status 0, and surface any other exit code as an error before calling fail.crates/minimal/src/autospawn.rs (1)
185-203: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueBlocking
thread::sleeppolling loop called from asynccmd_stop.
wait_for_minvmd_stoppedblocks its calling OS thread for up toSTOPPED_WAIT_SECS(20s) viathread::sleep, but it's invoked synchronously from the asynccmd_stop(crates/minimal/src/lib.rs, lines 1139-1140) withoutspawn_blocking. This mirrors the pre-existingWaitForStoppingpolling pattern inensure_minvmd_running(same file), so it's consistent with prior convention and likely harmless for a single-command CLI process, but it will stall the Tokio worker thread executing this future (and any signal-handling tasks sharing that runtime) for the wait duration.🤖 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 `@crates/minimal/src/autospawn.rs` around lines 185 - 203, Make the synchronous wait in wait_for_minvmd_stopped non-blocking when called by async cmd_stop: invoke it through Tokio’s spawn_blocking boundary and await the result, propagating both join and io errors. Preserve the existing lifecycle polling, timeout, and shutdown behavior.
🤖 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 `@crates/minimal/src/autospawn.rs`:
- Around line 185-203: Make the synchronous wait in wait_for_minvmd_stopped
non-blocking when called by async cmd_stop: invoke it through Tokio’s
spawn_blocking boundary and await the result, propagating both join and io
errors. Preserve the existing lifecycle polling, timeout, and shutdown behavior.
In `@scripts/session-e2e.sh`:
- Around line 144-154: The minvmd status check in the E2E VM validation must
distinguish a stopped VM from command failures. Capture the exit status from
minvmd status, accept 1 as the expected stopped state, retain the failure for
status 0, and surface any other exit code as an error before calling fail.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1c835597-3008-4414-b2e9-f9d1ceaf657b
📒 Files selected for processing (7)
crates/minimal/Cargo.tomlcrates/minimal/src/autospawn.rscrates/minimal/src/client.rscrates/minimal/src/lib.rscrates/minimald/src/guest.rscrates/minimald/src/main.rsscripts/session-e2e.sh
`Client::connect(...).unwrap_err()` requires `Client: Debug`, which it
does not implement, so `cargo test -p minimal` failed to build and the
test never ran. Map the Ok side to `()` to drop the bound.
Verified against a real microVM: the pre-fix guest panics the kernel
("Attempted to kill init!") on shutdown, while the fixed guest powers
off and the session e2e passes on the VM lane.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`RB_POWER_OFF` only takes the VM down on aarch64. It needs a `pm_power_off` handler, and the x86_64 guest kernel has none: the kernel logs "Power off not available: System halted instead" and halts the vCPU with the VMM still alive — which is #730 again, the very wedge this was meant to fix. aarch64 has PSCI SYSTEM_OFF, so the arch gap was invisible locally and only surfaced on CI's x86_64 KVM lane. Reset instead: a guest-initiated reset is what makes a firecracker-family VMM exit, and it reaches KVM_EXIT_SHUTDOWN on both arches (PSCI SYSTEM_RESET on aarch64, i8042 reset / triple fault on x86_64). libkrun exits on it rather than restarting the guest, so it ends the VM despite the name. Rename `power_off` to `shut_down_vm`, since it no longer powers off, and note that it must only be called as the microVM's pid-1 — on an ordinary host it reboots the machine. Verified on aarch64: the session e2e passes on the VM lane, the guest logs "reboot: Restarting system", the VM reaches Stopped and the next command autospawns a fresh one. The x86_64 KVM lane is the check that caught this. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@crates/minimald/src/guest.rs`:
- Around line 555-556: Replace the argv[0]-based reboot protection around the
microVM shutdown/reboot path and is_minimal_microvm with a guest-only,
non-spoofable signal. Ensure ordinary privileged host processes cannot reach
reboot even when argv[0] is set to init, and add a test covering that
spoofed-argv scenario.
In `@crates/minimald/src/main.rs`:
- Around line 245-249: Update the shutdown-failure path around
minimald::guest::shut_down_vm so PID 1 cannot return result after shutdown
fails. Make this branch non-returning by retrying shutdown or delegating to the
supervisor, while preserving the existing behavior for successful shutdown and
normal async_main completion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fc5d1916-ab4b-4fc7-916d-7809adb563f4
📒 Files selected for processing (3)
crates/minimal/src/client.rscrates/minimald/src/guest.rscrates/minimald/src/main.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/minimal/src/client.rs
Review follow-ups on #730. The guard that decides "am I the microVM's init" now also gates `reboot(2)`, and it keyed on `argv[0]` alone — which the caller controls. `exec -a init minimald` on a host with CAP_SYS_BOOT would have reset the machine when the process exited. Require pid-1 as well: that cannot be spoofed from userspace, and while a native daemon could be a container's init, it would not also be named `init`. Only the microVM's init satisfies both. Split out a pure `is_microvm_init(pid, argv0)` so the spoofing cases are testable. If the reset syscall fails, pid-1 no longer falls through and returns: that would exit init and panic the guest kernel — the exact wedge #730 is about. Park instead, as the boot path's degraded arms already do, so the kernel stays idle (no panic-handler spin) and `minvmd stop`'s SIGTERM can still reap the VMM. Also: - The handshake-timeout error no longer claims the peer is a daemon; the connect path is shared with providers that are not (Tom). - `min stop`'s wait runs on the blocking pool rather than stalling an async worker for up to 20s (rust-coding-standards). - session-e2e.sh matches `minvmd status`'s exit code exactly (0 running, 1 stopped) instead of reading any non-zero exit as "stopped", which would mask a missing binary or a lock-contention (2) failure.
|
Addressed all review comments in fd28da3 — replies on each thread. Summary:
Verified: |
Resolve the conflicts from the commits that landed on main since this branch's merge-base (#721, #732, #734, #735, #722), keeping main's content and re-applying the `min` binary-target rename on top. - justfile: main folded `up` into `dm1` (#722), so the branch's older `up` recipe is dropped rather than resurrected. Main's `dm1` invoked the `{{minimal}}` variable this branch renames, which would have left `just` unable to resolve it; it now invokes `{{min-bin}}`. - CI lanes: keep main's rewritten jobs and steps, renaming only the CLI build flags and built-binary paths (`--bin min`, `target/debug/min`). Also point the sessions example project at `./target/debug/min`; the binary path it documented no longer exists after the rename. Published release asset names (`minimal-linux-amd64`, ...), the macOS `minimal` shim, and the `minimal` crate and lib target are deliberately left alone.
Closes: #730
The bug
minimaldis the microVM's pid-1 (it ships as the initramfs/init). After theShutdownRPC drained the server,Server::runreturned,mainreturned — and init exited. The kernel panicked (Attempted to kill init!) and, with nopanic=on the cmdline, spun in the panic handler forever.Everything downstream followed from that: the VMM child never exited, so the supervisor never reached its
Running → Stoppedtransition; the state file still saidRunning, so the CLI never autospawned; and libkrun's bridge socket still accepted connections that nothing behind them answered.min lsthen blocked in the SSH handshake — which has no timeout — forever. Somin stop --forcebricked the daemon until the VM was killed by hand.The fix
crates/minimald— the guest powers the VM off instead of exiting init.guest::power_off()(sync(2)thenreboot(RB_POWER_OFF)) is called whenever minimald is the microVM's pid-1, on the clean and the failed exit alike, since either way there is no init left to run. libkrun's VMM exits with the guest, the supervisor reaps it and writesStopped, and the next command boots a fresh VM.crates/minimal/src/client.rs— bound the SSH handshake at 10s. A completed connect is not proof of a live daemon: libkrun's bridge accepts even when the guest behind it is wedged.minvmd's own RPC client already guards exactly this (with a comment explaining why, and a mute-listener regression test); the CLI's client did not. Any future guest wedge now surfaces an error instead of an infinite block.crates/minimal(stop) —min stopnow waits for the VM to reach a terminal lifecycle state before returning, dropping its SSH connection first (the daemon's drain holds the shutdown open while a client is still attached). Without this, a command run immediately aftermin stopwould race aRunningstate whose VM was already going down.Verification
Ran the real thing on macOS/HVF:
E2E_VM=1 ./scripts/session-e2e.shagainst aminvmdand initramfs built from this branch. Two live VM boots,session e2e OK. The guest console for the stop that used to panic:No
Kernel panic, noAttempted to kill init. The volume still quiesces first, so the ext4 journal stays clean.scripts/session-e2e.shnow asserts both halves of the bug, so it stays fixed: afterminimal stopthe VM must beStopped(minvmd statusexits 1), and the nextminimal lsmust bring a daemon back. Onmainthe first assert fails.Three new unit tests: the client's handshake deadline against a mute listener (the CLI half of the hang, run in virtual time), and the stop-wait resolving both a stopped VM and a dead supervisor's stale
Runningstate.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests