feat(minimald,minimal2): boringtun WireGuard mesh peer, subnet-router advertisement, and minimal mesh CLI - #561
Conversation
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a feature-gated WireGuard mesh runtime, status RPC, CLI support, tests, and CI coverage, and changes gvproxy stop timeout handling to always escalate with ChangesWireGuard Mesh Feature
Gvproxy Stop Timeout
Sequence Diagram(s)sequenceDiagram
participant User
participant minimal2 as minimal CLI
participant minimald as minimald RPC server
participant MeshHandle
participant pump
participant Tunn as boringtun Tunn
User->>minimal2: mesh status
minimal2->>minimald: GetMeshStatus
minimald->>MeshHandle: mesh_status()
MeshHandle-->>minimald: MeshStatus
minimald-->>minimal2: MeshStatus response
MeshHandle->>pump: send_outbound(packet)
pump->>Tunn: encapsulate(packet)
Tunn-->>pump: encrypted datagram
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/minvmd/src/net.rs (1)
557-575: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAlways escalate on timeout after a prior teardown claim.
If a last
PtaskAttachmentalready setstoppingand sent SIGTERM,stop()skips SIGKILL and then awaits the supervisor unbounded after the timeout. A gvproxy that ignores SIGTERM can hang shutdown indefinitely. Once the supervisor timed out, the child has not been reaped, so SIGKILL is still safe to send.Proposed fix
if tokio::time::timeout(self.term_timeout, &mut supervisor) .await .is_err() { - if !already_claimed { - signal_child(pid, libc::SIGKILL, "SIGKILL"); - } + signal_child(pid, libc::SIGKILL, "SIGKILL"); let _ = supervisor.await; }🤖 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/minvmd/src/net.rs` around lines 557 - 575, The stop() flow in net.rs only sends SIGKILL when it did not already claim shutdown, which lets a prior teardown claim skip escalation and then wait on supervisor.await indefinitely after the timeout. Update the stop() logic around stopping.swap, self.supervisor.take, and the tokio::time::timeout over supervisor so that timeout always triggers a SIGKILL attempt before the final await, regardless of whether SIGTERM was already sent. Keep the existing signal_child calls and reuse the pid/supervisor handling, but ensure a timed-out child is force-killed even after a prior PtaskAttachment initiated teardown.
🤖 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/main.rs`:
- Around line 550-557: The `cmd_mesh_join` flow is persisting `args.address`
without validating that it matches the expected `host:port` format. Add
validation in `cmd_mesh_join` before `std::fs::write` to ensure the endpoint has
a non-empty host and a valid `u16` port, and return an error early if parsing
fails so invalid enrolments are never written.
In `@crates/minimald/src/net/wg.rs`:
- Around line 816-823: The loopback test is still hard-coding fixed ports, which
can make it flaky when ports are occupied or tests run in parallel. Update the
test around the wg socket setup to use pre-bound ephemeral sockets instead of
`a_port` and `b_port`, and wire those sockets into `start_with_socket()` so the
test keeps using real bound addresses without depending on specific port
numbers.
- Around line 553-603: The inbound routing logic only returns one candidate in
route_inbound(), so roaming peers with changed source addresses and later
endpoint-less peers are never retried. Update the WireGuard receive path in
route_inbound() and the decapsulate loop to iterate through multiple peer
candidates (exact match first, then other possible peers) until one
authenticates or all fail, instead of stopping at the first endpoint=None peer.
Keep the existing logging in the wg receive handler tied to the authenticated
peer once decapsulation succeeds.
In `@crates/minimald/src/server.rs`:
- Around line 163-172: The mesh status reporting in mesh_status currently treats
a populated mesh slot as proof that WireGuard is still running, which can leave
GetMeshStatus showing stale configured/live state after pump failures. Update
the status logic in mesh_status to reflect actual runtime health from the
networking-wg path, using the state managed by the pump in
net::wg::status_response and/or the pump lifecycle instead of only checking
self.0.lock().await.mesh. Make sure the status is cleared or marked unconfigured
when the wg pump exits on socket errors so stale peer state is not reported.
In `@crates/minimald/tests/mesh_uc7.rs`:
- Around line 136-138: The namespace/veth cleanup in mesh_uc7::test path is only
called manually, so a panic after setup_namespaces() can leave root-created
state behind. Wrap the setup/use section in a small Drop guard (or equivalent
RAII cleanup helper) so teardown_namespaces() is guaranteed to run on normal
completion, early panic, and timeout paths, and update the test flow around
setup_namespaces() and the later shutdown/finish block to use that guard.
In `@crates/minvmd/src/net.rs`:
- Around line 662-670: Close the PID-reuse window in the `GvproxySwitch`
teardown path: the last-drop SIGTERM logic in the `swap`/`stopping` check can
still target a recycled PID after an independent crash. Add a shared
`child_reaped` or `child_exited` flag that `supervise_switch` sets immediately
after `wait()`, then have the drop/termination path in `net.rs` check that flag
before calling `signal_child(self.pid, ...)` so PID-only signaling is skipped
once the child is known to have exited.
---
Outside diff comments:
In `@crates/minvmd/src/net.rs`:
- Around line 557-575: The stop() flow in net.rs only sends SIGKILL when it did
not already claim shutdown, which lets a prior teardown claim skip escalation
and then wait on supervisor.await indefinitely after the timeout. Update the
stop() logic around stopping.swap, self.supervisor.take, and the
tokio::time::timeout over supervisor so that timeout always triggers a SIGKILL
attempt before the final await, regardless of whether SIGTERM was already sent.
Keep the existing signal_child calls and reuse the pid/supervisor handling, but
ensure a timed-out child is force-killed even after a prior PtaskAttachment
initiated teardown.
🪄 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: 1ab7cbb6-7b18-494c-b66c-e95fa3eaa605
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
Cargo.tomlcrates/minimal2/src/main.rscrates/minimald-rpc/src/lib.rscrates/minimald/Cargo.tomlcrates/minimald/src/net/mod.rscrates/minimald/src/net/wg.rscrates/minimald/src/rpc.rscrates/minimald/src/server.rscrates/minimald/tests/mesh_uc7.rscrates/minvmd/src/net.rs
|
Revise claim for head 44700c2. |
|
Auto-revise 1 of 3. |
This comment has been minimized.
This comment has been minimized.
|
Clearing |
|
sdd-validate · Implementation boundary · PR #561 Boundary resolved: ImplementationAll changed files are code-level (Rust crates, CI config, Cargo lockfile). No files under Findings⛔ Blocker — Gate 1: Declared proof artifact has no consumer CI coverageThe declared Test proof artifact —
The regular CI step added by this PR ( No consumer gate covers this declared two-namespace proof. Per gate 1, a proof artifact covered by no gate (neither agent execution nor consumer CI) is a Blocker. Evidence: ⛔ Blocker — Gate 2: Change to protected path
|
|
Revise claim for head 0f535e2. |
|
Auto-revise 2 of 3. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/main.rs`:
- Around line 558-561: The mesh join validation in main should reject port 0
before saving enrolment, since port.parse::<u16>() currently accepts it as
valid. Update the existing host/port check in the join flow to parse the port,
then explicitly require it to be greater than 0 before continuing to persist the
enrolment, and keep the current error path for invalid addresses.
🪄 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: 2c387f3e-35d6-42f5-b37c-dbaa2a5f6b52
📒 Files selected for processing (5)
crates/minimal2/src/main.rscrates/minimald/src/net/wg.rscrates/minimald/src/server.rscrates/minimald/tests/mesh_uc7.rscrates/minvmd/src/net.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/minimald/src/server.rs
- crates/minimald/tests/mesh_uc7.rs
- crates/minimald/src/net/wg.rs
The WireGuard mesh peer is behind the non-default networking-wg feature, so the workspace test job never compiled or ran its proof artifacts (two_meshes_handshake_and_relay_a_packet, rpc get_mesh_status). Add an explicit step so the mesh proofs run in CI (sdd-validate Gate 1). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0143kv2BRrRqGxmVwwHskQtS
…sh proof Restore main's crates/minvmd/src/net.rs: this branch had reverted #555's pidfd-based gvproxy signalling (a stale-base content-revert unrelated to the WireGuard mesh task), re-introducing the PID-recycle bug. WireGuard lives in minimald; minvmd net.rs is out of scope here. Wire the UC7 two-namespace mesh proof into ci-netns.yml: it was covered by no gate (mesh_uc7.rs is networking-wg-gated, the netns name filter did not match remote_ptask_packet_crosses_the_mesh_tunnel, and the test read MINIMAL_NETNS_TESTS while the lane sets MINIMALD_NETNS_TEST). Add a dedicated --features networking-wg --test mesh_uc7 step and fix the env var name in the test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0143kv2BRrRqGxmVwwHskQtS
mesh_uc7 binds sockets inside the namespaces via in-process setns, so the test binary itself needs root (the sudo-per-command model the UC6 netns tests use does not cover an in-process setns). Build unprivileged, then run the test binary under the netns runner's passwordless sudo, fixing the 'mkdir /run/netns: Permission denied' / 'ip netns add' failures. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0143kv2BRrRqGxmVwwHskQtS
- mesh join: validate host:port at entry before persisting the enrolment so a typo never lands a bad address on disk (CR/bot R4.3 input check). - wg route_inbound: probe every candidate peer (exact-endpoint match first, then each endpoint-less peer) until one authenticates, instead of stopping at the first endpoint-less peer — handles roaming source addresses and multiple endpoint-less peers. Still exactly one decapsulate for the owning peer. - wg mesh_status: treat a finished pump as unconfigured (MeshHandle:: is_alive) so GetMeshStatus never serves frozen, stale peer state after the pump exits on a socket error. - wg loopback test: pre-bind ephemeral sockets via start_with_socket instead of hard-coding 51820/51821, removing CI port-collision flakes. - mesh_uc7: RAII Drop guard tears down namespaces on every exit path (success, panic, timeout); fix stale env-var name in the doc comment. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
When a PtaskAttachment drop already claimed teardown (SIGTERM sent) and gvproxy ignores SIGTERM, stop() would hit its grace timeout but skip SIGKILL under the `!already_claimed` guard, then block forever on `supervisor.await` and hang daemon shutdown. Escalate to SIGKILL on timeout unconditionally; on Linux the fd-based signal targets the exact process instance (ESRCH after exit is benign), so it never lands on a recycled PID. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
port.parse::<u16>() accepts 0, but a WireGuard endpoint on port zero is unusable; was still written as a successful enrolment. Require non-zero. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0143kv2BRrRqGxmVwwHskQtS
893d4f7 to
148b52b
Compare
`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>
…d to opt into the VM (#571) * feat(minimal2,minimald): default to native minimald on Linux; --minvmd 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 * fix(minimal2,minimald): forward --minimal-dir to spawned minimald; reject --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 * fix(minimal2): route mesh status through ensure_daemon_running `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> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
What this lands
Unit 4's WireGuard mesh (R4.1, R4.2, R4.5, R4.6, R4.7, R4.8).
minimaldjoins a mesh as a subnet-router peer that advertises its gvproxy switch
subnet, so a packet for a remote PTask's switch IP rides an encrypted tunnel
to the
minimaldthat owns that switch.crates/minimald/src/net/wg.rs(new, featurenetworking-wg) — apure-Rust [
boringtun] peer: x25519 key generation, one async UDP pumpdriving a per-peer
Tunn, AllowedIPs subnet-router advertisement andoutbound routing, handshake/peer-change
tracing, auth-failure loggingthat reveals no switch IPs or PTask names (R4.5), and a live status
snapshot (R4.1, R4.2, R4.7).
crates/minimald-rpc—GetMeshStatusRPC withMeshStatus/MeshPeerStatuswire types. They carry no WireGuard dependency, so theycompile in every build; a daemon built without the feature answers
configured = false(R4.6).crates/minimald/src/rpc.rs,server.rs— serveGetMeshStatusfrom afeature-gated mesh slot on the server state.
crates/minimal2—minimal mesh join,minimal mesh leave,minimal mesh statuswith help text and examples;statusrenders thedaemon's own key, advertised subnets, and peers (R4.8).
Cargo.toml—boringtunpinned behind thenetworking-wgfeature sothe default build carries no WireGuard code (R4.7). Lockfile change is
purely additive (boringtun's dependency subtree only).
Scope note (honest)
This lands the mesh tunnel data path (key exchange → handshake →
AllowedIPs routing → encrypt/decrypt over UDP), the status RPC, and the CLI.
Splicing the tunnel sink into a live gvproxy switch so a literal
TCP connectbetween two
minimald-managed PTasks traverses it — the full six-hop UC7 pathin R4.2 — is the follow-up that builds directly on the data path proven here.
The
#[ignore]two-namespace test (tests/mesh_uc7.rs) is the netns-CI entrypoint for that path; it is
#[ignore]by spec design (needs root + twonetwork namespaces) and was not executed in this sandbox (no root).
Proof artifacts
Proof 1 — Test (UC7 remote PTask-to-PTask over the tunnel). The mesh data
path is proven by a runnable loopback test plus the
#[ignore]two-namespacetest. Both fail on base (no
wgmodule).cargo test -p minimald --features networking-wg:two_meshes_handshake_and_relay_a_packetstands up two mesh peers over realUDP, completes a WireGuard handshake, and asserts an IP packet for one peer's
advertised switch
/32arrives decrypted on the other peer's tunnel sink— the encrypted transport that carries UC7.
Proof 2 — CLI (
minimal mesh statusreturns own key + peer list, R4.6).Asserted at the RPC layer (the CLI is a thin renderer over this RPC).
rpc::tests::get_mesh_status_reports_own_key_and_peersinstalls a configuredmesh and asserts the
GetMeshStatusresponse carriesconfigured = true, thenode's own base64 public key, the advertised subnet, and the configured peer.
Fails on base (RPC absent).
Gate
Run against the touched crates in both feature states:
Full-workspace
cargo test -- --include-ignoredwas not run end-to-end herebecause the sandbox disk filled during the multi-profile builds; the three
affected crates pass fmt, clippy, and tests in both feature states, and the
merged-change revert guard reports clean.
Next step
Merging this PR closes #501. Once every task sub-issue of the tracking issue
(#478) is closed, the pipeline advances #478 to
sdd:donefor a final humanreview.
Closes #501
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
crates.ioSee Network Configuration for more information.
Summary by CodeRabbit
minimal meshCLI subcommand withjoin,leave, andstatus.networking-wgbuild feature to enable WireGuard mesh support.