Skip to content

fix(minimald,minimal): power the microVM off when the guest daemon stops - #734

Merged
norrietaylor merged 4 commits into
mainfrom
fix/730-guest-poweroff-on-shutdown
Jul 13, 2026
Merged

fix(minimald,minimal): power the microVM off when the guest daemon stops#734
norrietaylor merged 4 commits into
mainfrom
fix/730-guest-poweroff-on-shutdown

Conversation

@norrietaylor

@norrietaylor norrietaylor commented Jul 13, 2026

Copy link
Copy Markdown
Member

Closes: #730

The bug

minimald is the microVM's pid-1 (it ships as the initramfs /init). After the Shutdown RPC drained the server, Server::run returned, main returned — and init exited. The kernel panicked (Attempted to kill init!) and, with no panic= 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 → Stopped transition; the state file still said Running, so the CLI never autospawned; and libkrun's bridge socket still accepted connections that nothing behind them answered. min ls then blocked in the SSH handshake — which has no timeout — forever. So min stop --force bricked 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) then reboot(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 writes Stopped, 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 stop now 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 after min stop would race a Running state whose VM was already going down.

Verification

Ran the real thing on macOS/HVF: E2E_VM=1 ./scripts/session-e2e.sh against a minvmd and initramfs built from this branch. Two live VM boots, session e2e OK. The guest console for the stop that used to panic:

minimald::rpc: state volume quiesced for shutdown
minimald::server: draining connections for shutdown live=1
minimald: microVM init finished; powering the VM off
[    0.126149] reboot: Power down

No Kernel panic, no Attempted to kill init. The volume still quiesces first, so the ext4 journal stays clean.

scripts/session-e2e.sh now asserts both halves of the bug, so it stays fixed: after minimal stop the VM must be Stopped (minvmd status exits 1), and the next minimal ls must bring a daemon back. On main the 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 Running state.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Added a stop-wait mechanism to ensure the daemon reaches a fully stopped state before continuing.
    • Added guest shutdown handling when running as the microVM init process.
  • Bug Fixes

    • Added a fixed timeout to prevent the CLI from hanging during SSH UDS handshake/auth.
    • Improved stop-command behavior by dropping the client connection and waiting for daemon shutdown; enhanced validation of stop/restart outcomes.
  • Tests

    • Extended unit and Tokio paused-time tests for stopped lifecycle, stale states, and handshake timeout failures.
    • Strengthened session end-to-end checks for VM stop and successful autospawn.

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
@coderabbitai

coderabbitai Bot commented Jul 13, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 13 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 600c19bf-6146-4d42-a8a9-600b5515a614

📥 Commits

Reviewing files that changed from the base of the PR and between 01fee38 and fd28da3.

📒 Files selected for processing (4)
  • crates/minimal/src/client.rs
  • crates/minimal/src/lib.rs
  • crates/minimald/src/main.rs
  • scripts/session-e2e.sh
📝 Walkthrough

Walkthrough

The changes bound SSH handshake timeouts, wait for daemon shutdown completion, explicitly power off microVM guests, and verify that stopped VM targets can autospawn successfully.

Changes

Shutdown recovery flow

Layer / File(s) Summary
Bound SSH handshake and authentication
crates/minimal/src/client.rs, crates/minimal/Cargo.toml
SSH connection setup now uses a Tokio deadline, with paused-time regression coverage.
Power off the microVM init path
crates/minimald/src/guest.rs, crates/minimald/src/main.rs
MicroVM init synchronizes and resets the guest after daemon execution completes.
Wait for stopped state and verify restart
crates/minimal/src/autospawn.rs, crates/minimal/src/lib.rs, scripts/session-e2e.sh
The stop command waits for a terminal lifecycle state, while tests and VM checks validate shutdown and autospawn.

Estimated code review effort: 3 (Moderate) | ~30 minutes

Possibly related PRs

Suggested reviewers: gominimal-aw-bot[bot], twitchyliquid64, evanspearman

Poem

I’m a shutdown rabbit, hopping through the state,
Watching sleepy daemons finish at the gate.
SSH gets a timer, guests power down bright,
Then ls wakes the service for another night.
“Stopped,” says the burrow—everything’s all right!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning [#730] The clean shutdown, stop-wait, and VM exit flow are implemented; [#39] the requested root Cargo dependency bumps are not shown in the changes. Add the root Cargo.toml dependency bumps for nickel-lang-core, toml_edit, petgraph, and nix, then refresh the lockfile and verify checks.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title matches the main change: making minimald/minimal cleanly stop the microVM instead of leaving it running.
Out of Scope Changes check ✅ Passed All visible changes support shutdown handling, handshake timeout, stop waiting, and tests; no unrelated code changes are apparent.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (2)
scripts/session-e2e.sh (1)

144-154: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value

minvmd status exit code check conflates "stopped" with "any non-zero exit."

If minvmd status exits 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 value

Blocking thread::sleep polling loop called from async cmd_stop.

wait_for_minvmd_stopped blocks its calling OS thread for up to STOPPED_WAIT_SECS (20s) via thread::sleep, but it's invoked synchronously from the async cmd_stop (crates/minimal/src/lib.rs, lines 1139-1140) without spawn_blocking. This mirrors the pre-existing WaitForStopping polling pattern in ensure_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

📥 Commits

Reviewing files that changed from the base of the PR and between f95835e and 8a70569.

📒 Files selected for processing (7)
  • crates/minimal/Cargo.toml
  • crates/minimal/src/autospawn.rs
  • crates/minimal/src/client.rs
  • crates/minimal/src/lib.rs
  • crates/minimald/src/guest.rs
  • crates/minimald/src/main.rs
  • scripts/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>
Comment thread crates/minimal/src/client.rs Outdated
`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>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

📥 Commits

Reviewing files that changed from the base of the PR and between 8a70569 and 01fee38.

📒 Files selected for processing (3)
  • crates/minimal/src/client.rs
  • crates/minimald/src/guest.rs
  • crates/minimald/src/main.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • crates/minimal/src/client.rs

Comment thread crates/minimald/src/guest.rs
Comment thread crates/minimald/src/main.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.
@norrietaylor

Copy link
Copy Markdown
Member Author

Addressed all review comments in fd28da3 — replies on each thread. Summary:

Concern Resolution
Spoofable argv[0] gates reboot(2) (CodeRabbit, Major) Guard now requires pid-1 and argv[0] == init. Pure is_microvm_init(pid, argv0) + 3 tests, including the spoofed-argv case.
pid-1 returns after a failed reset (CodeRabbit) It now parks instead of exiting — exiting init would panic the kernel, i.e. re-create #730.
Handshake error claims the peer is a daemon (@twitchyliquid64) Now connect to {path}: SSH handshake timed out after {…} — no claim about what's behind the socket.
Blocking sleep-loop in async cmd_stop (CodeRabbit, nitpick) Moved to spawn_blocking, per the repo's "no blocking in an async context" standard.
minvmd status exit code conflated (CodeRabbit, nitpick) Matches the code exactly (0 running / 1 stopped); any other code now surfaces as a failure rather than reading as "stopped".

Verified: cross test -p minimal -p minimald green (the two env::tests failures in the container are environmental — no writable cache dir; they pass with XDG_CACHE_HOME set, and the diff doesn't touch env.rs). cross clippy --all-targets -- -D warnings clean, cargo fmt clean, shellcheck scripts/session-e2e.sh clean.

@norrietaylor
norrietaylor enabled auto-merge (squash) July 13, 2026 22:58
@norrietaylor
norrietaylor merged commit 2283de9 into main Jul 13, 2026
25 checks passed
@norrietaylor
norrietaylor deleted the fix/730-guest-poweroff-on-shutdown branch July 13, 2026 23:06
norrietaylor added a commit that referenced this pull request Jul 14, 2026
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.
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.

min command hang after min stop --force due to minvmd kernel panic

2 participants