Repository navigation
feat(minimal2,minimald): default to native minimald on Linux; --minvmd to opt into the VM - #571
Merged
Merged
Conversation
…d to opt into the VM On Linux the CLI unconditionally autospawned minvmd, yet resolved the native minimald socket — self-inconsistent, and broken on hosts where minvmd is the non-libkrun stub. Make native minimald (DM2) the Linux default and the minvmd microVM (DM1) an explicit opt-in. - minimal2: `--minvmd` global flag. Linux default autospawns native minimald; `--minvmd` autospawns minvmd. macOS is unaffected (minvmd is the only backend). - minimal2: `ensure_daemon_running()` dispatches by platform + flag; `resolve_socket_path()` takes the backend so Linux+--minvmd resolves the bridge UDS, not the native path. - minimald: add `run --detach` — re-execs in a new session (setsid), stdio to null, and returns once the SSH socket accepts connections. Mirrors `minvmd run --detach`; used by the native autospawn path. Verified in Docker (rust:1.95, --locked): fmt, workspace clippy -D warnings, build -p minimald -p minimal2, test -p minimal2 (incl. the backend socket-path test). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0143kv2BRrRqGxmVwwHskQtS
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds detached startup support to ChangesDaemon backend selection and detached startup
Sequence Diagram(s)sequenceDiagram
participant async_main as "async_main"
participant spawn_detached as "spawn_detached"
participant cli_listen_on as "cli.listen_on"
participant unix_stream_connect as "UnixStream::connect"
async_main->>spawn_detached: listen_args.detach == true
spawn_detached->>spawn_detached: re-exec binary without --detach
spawn_detached->>spawn_detached: setsid and redirect stdio to null
spawn_detached->>cli_listen_on: poll SSH socket path
cli_listen_on->>unix_stream_connect: connect(socket)
unix_stream_connect-->>spawn_detached: success or retry
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
crates/minimal2/src/client.rs (1)
188-194: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the full override-derived paths.
The current
ends_withchecks do not prove thatminimal_dir_overridewas honored; a default socket path with the same suffix would still pass.Strengthen the test
- assert!(native.ends_with("providers/local-0/ssh.sock"), "{native:?}"); - assert!(bridged.ends_with("minimald.sock"), "{bridged:?}"); + assert_eq!(native, dir.join("providers/local-0/ssh.sock")); + assert_eq!(bridged, dir.join("minimald.sock"));🤖 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/minimal2/src/client.rs` around lines 188 - 194, The linux_socket_path_honors_backend_with_override test only checks suffixes, so it can pass even if minimal_dir_override is ignored. Update the assertions in resolve_socket_path’s test to verify the complete override-derived paths for both the native and bridged cases, using the same symbols resolve_socket_path and linux_socket_path_honors_backend_with_override so the test proves the override is actually applied.
🤖 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/minimal2/src/autospawn.rs`:
- Around line 153-159: The native daemon autospawn path is resolving the socket
with minimal_dir in ensure_daemon_running, but ensure_minimald_running/startup
still launches minimald without that same state directory, causing a mismatch
when --minimal-dir is used. Update the autospawn flow so the resolved
minimal_dir is forwarded into the spawned backend path used by
ensure_minimald_running (and the minimald launch logic it calls), keeping the
socket resolution and daemon startup aligned.
- Around line 177-185: Serialize the native autospawn path in
minimal2::autospawn before invoking minimald run --detach, since concurrent
callers can both pass the UnixStream::connect check and spawn duplicate daemons.
Add a per-socket or state-dir lock around the connect-and-spawn logic in the
autospawn flow, then re-check the socket after acquiring the lock to avoid
racing. Keep the fix localized to the autospawn code that uses socket_path and
Command::new("minimald").
In `@crates/minimald/src/main.rs`:
- Around line 318-323: `spawn_detached` currently assumes `cli.listen_on()` is a
Unix socket, so `minimald run --detach --vsock` can time out even though the
detached child is alive. Update the `main` flow around `listen_args.detach` to
either reject `--detach` when `cli.vsock` is set or make `spawn_detached`/its
readiness check aware of vsock by polling the actual vsock listener instead of
`UnixStream::connect`. Keep the fix localized to `main`, `spawn_detached`, and
the `listen_on` readiness logic so the detached path matches the listener type.
---
Nitpick comments:
In `@crates/minimal2/src/client.rs`:
- Around line 188-194: The linux_socket_path_honors_backend_with_override test
only checks suffixes, so it can pass even if minimal_dir_override is ignored.
Update the assertions in resolve_socket_path’s test to verify the complete
override-derived paths for both the native and bridged cases, using the same
symbols resolve_socket_path and linux_socket_path_honors_backend_with_override
so the test proves the override is actually applied.
🪄 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: 40a23532-64ab-4a8f-acd1-ddd995684726
📒 Files selected for processing (4)
crates/minimal2/src/autospawn.rscrates/minimal2/src/client.rscrates/minimal2/src/main.rs
crates/minimald/src/main.rs
norrietaylor
marked this pull request as draft
June 24, 2026 23:38
…ject --detach --vsock - autospawn: forward the resolved minimal_dir into the spawned `minimald run --detach` as `--minimal-state-dir`, so the daemon binds the same socket resolve_socket_path resolved (otherwise it defaults to $XDG_STATE_HOME and the CLI connects to the override socket and fails). (CR thread r3470936701) - minimald: reject `run --detach --vsock` up front. spawn_detached polls the UDS for readiness, but a --vsock child binds vsock instead, so the parent would always hit the 8s timeout while leaving an orphaned detached child. (CR thread r3470936703) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0143kv2BRrRqGxmVwwHskQtS
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
`cmd_mesh_status` (added in #561, after this branch forked) still called the pre-#571 `ensure_minvmd_running()`, so `minimal mesh status` ignored the `--minvmd` flag and always autospawned the minvmd VM. Convert it to `ensure_daemon_running(global.minvmd, ...)` like every other command, so it respects the native-default / `--minvmd`-opt-in backend dispatch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
norrietaylor
marked this pull request as ready for review
June 25, 2026 07:48
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
On Linux the CLI unconditionally autospawned minvmd (
autospawn::ensure_minvmd_running) whileresolve_socket_pathresolved the native minimald socket — self-inconsistent, and unusable on hosts where minvmd is the non-libkrun stub. This makes native minimald (DM2) the Linux default and the minvmd microVM (DM1) an explicit--minvmdopt-in. macOS is unchanged (minvmd is the only backend there).Changes
crates/minimal2--minvmdglobal flag onGlobalArgs.autospawn::ensure_daemon_running(use_minvmd, minimal_dir)dispatches: macOS → minvmd; Linux → native minimald by default, minvmd when--minvmd. Replaces the 6 directensure_minvmd_running()call sites.ensure_minimald_running()(Linux): if the SSH socket isn't already accepting, spawnsminimald run --detach --instance-num 0.resolve_socket_path(minimal_dir, use_minvmd)— Linux+--minvmdresolves the minvmd bridge UDS; otherwise the nativeproviders/local-0/ssh.sock.crates/minimaldminimald run --detach: re-execs in a new session (setsidviapre_exec), stdio→null, and returns once the SSH socket accepts connections or an 8s timeout elapses. Mirrorsminvmd run --detach.Behavior
minimal2 activateon Linux → native minimald (DM2), no VM.minimal2 --minvmd activateon Linux → minvmd microVM (DM1); requires/dev/kvm+ libkrun.--minvmdis a no-op (minvmd always).Verification
Docker
rust:1.95+protobuf-compiler,--locked,CARGO_INCREMENTAL=0:cargo fmt --all -- --check— cleancargo clippy --workspace --all-targets -- -D warnings— cleancargo build -p minimald -p minimal2— cleancargo test -p minimal2— pass (incl. a newresolve_socket_pathbackend-split test)Follow-up
A native-detach integration proof (spawn
minimald run --detach, drive a CLI round-trip without minvmd) is worth a netns-lane test; not included here.🤖 Generated with Claude Code
Summary by CodeRabbit
--minvmdoption to select the daemon backend on supported platforms.--detachto daemon listening to start in the background and wait until the daemon is ready.