diff --git a/Cargo.lock b/Cargo.lock index b792e95eb..097e953c3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -682,7 +682,7 @@ dependencies = [ [[package]] name = "arcbox-helper" -version = "1.0.2" +version = "1.0.3" dependencies = [ "arcbox-constants", "arcbox-logging", diff --git a/Cargo.toml b/Cargo.toml index ae5a91f8b..399fcb090 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -261,7 +261,7 @@ arcbox-fleet-control-proto = { version = "0.6.3", path = "fleet/arcbox-fleet-con arcbox-docker = { version = "0.6.3", path = "app/arcbox-docker" } # x-release-please-version # Pinned to app/arcbox-helper's own version (not workspace.package). Do not # attach x-release-please-version — helper releases are manual. -arcbox-helper = { version = "1.0.2", path = "app/arcbox-helper" } +arcbox-helper = { version = "1.0.3", path = "app/arcbox-helper" } arcbox-core = { version = "0.6.3", path = "app/arcbox-core" } # x-release-please-version arcbox-api = { version = "0.6.3", path = "app/arcbox-api" } # x-release-please-version arcbox-migration = { version = "0.6.3", path = "app/arcbox-migration" } # x-release-please-version diff --git a/app/AGENTS.md b/app/AGENTS.md index 2c1eb38eb..a6b3bd067 100644 --- a/app/AGENTS.md +++ b/app/AGENTS.md @@ -36,11 +36,11 @@ must name `abctl`. e2e) may use — never log-grep or sleep. Fatal startup failures MUST call `SetupState::set_failed` before exit (200ms flush grace, `main.rs`) so clients see the cause instead of a bare disconnect. Route-install state is - mirrored into `SetupState.route_installed` by `services::route_status_loop` - (`ContainerRouteInstalled` sets, `MachineStopped` clears) — WHY: VM - restarts install the route outside the cold-start path that sets the flag - directly, so without the bridge the flag goes stale until the next daemon - restart. + exclusively owned by the `container_route` controller. It follows System + VM lifecycle and kernel route changes, caches bridge `{name, ifindex}` by VM + generation, and publishes the result through `SetupState.route_installed`. + Never add a second writer — WHY: competing lifecycle and polling paths can + publish stale state after the VM or bridge identity changes. - **A `SetupStatus.Phase` value that nothing publishes is invisible as a gap** — it simply never arrives, so a client waits forever or reports a plausible zero. Declaring a phase in `api.proto` therefore obliges you to diff --git a/app/arcbox-api/src/connect/machine.rs b/app/arcbox-api/src/connect/machine.rs index 3a8475c40..1d840eb0a 100644 --- a/app/arcbox-api/src/connect/machine.rs +++ b/app/arcbox-api/src/connect/machine.rs @@ -681,7 +681,6 @@ mod tests { #[test] fn ignores_non_machine_events() { assert!(to_machine_event(&Event::VmStarted { id: "vm".into() }).is_none()); - assert!(to_machine_event(&Event::ContainerRouteInstalled { name: "m".into() }).is_none()); } #[test] diff --git a/app/arcbox-core/src/bridge_discovery.rs b/app/arcbox-core/src/bridge_discovery.rs index baf255734..4f8576915 100644 --- a/app/arcbox-core/src/bridge_discovery.rs +++ b/app/arcbox-core/src/bridge_discovery.rs @@ -6,12 +6,20 @@ use std::ffi::CString; /// Information about a bridge interface suitable for container routing. -#[derive(Debug, Clone)] +#[derive(Debug, Clone, PartialEq, Eq)] pub struct BridgeTarget { /// Interface name (e.g. "bridge104"). pub name: String, /// Interface index (from `if_nametoindex`). - pub ifindex: u32, + pub ifindex: u16, +} + +impl BridgeTarget { + /// Returns whether this name still identifies the same kernel interface. + #[must_use] + pub fn is_current(&self) -> bool { + if_nametoindex(&self.name) == Some(self.ifindex) + } } /// Resolve a VM bridge MAC to a bridge interface. @@ -50,11 +58,15 @@ pub fn find_bridge_with_vmenet() -> Option<(String, String)> { None } -fn if_nametoindex(name: &str) -> Option { +fn if_nametoindex(name: &str) -> Option { let cname = CString::new(name).ok()?; // SAFETY: if_nametoindex is a standard POSIX function with a valid C string. let idx = unsafe { libc::if_nametoindex(cname.as_ptr()) }; - if idx == 0 { None } else { Some(idx) } + if idx == 0 { + None + } else { + u16::try_from(idx).ok() + } } #[cfg(test)] diff --git a/app/arcbox-core/src/event.rs b/app/arcbox-core/src/event.rs index 5afbcde3c..99bada39a 100644 --- a/app/arcbox-core/src/event.rs +++ b/app/arcbox-core/src/event.rs @@ -19,8 +19,6 @@ pub enum Event { MachineStopped { name: String }, /// Machine removed (record and disks deleted). MachineRemoved { name: String }, - /// Container subnet route installed on the host for a machine's bridge NIC. - ContainerRouteInstalled { name: String }, } /// Event bus for system-wide event distribution. diff --git a/app/arcbox-core/src/machine.rs b/app/arcbox-core/src/machine.rs index d21b39ad9..c7eaca6bc 100644 --- a/app/arcbox-core/src/machine.rs +++ b/app/arcbox-core/src/machine.rs @@ -743,14 +743,14 @@ impl MachineManager { &self.vm_manager } - /// Returns the vmnet bridge interface name for a machine's VM. + /// Returns the vmnet bridge identity for a machine's VM. /// /// Only available when the `vmnet` feature is enabled and the VM is running. #[cfg(all(target_os = "macos", feature = "vmnet"))] - pub fn vmnet_bridge_name(&self, name: &str) -> Option { + pub fn vmnet_bridge_target(&self, name: &str) -> Option { let machines = self.machines.read().ok()?; let machine = machines.get(name)?; - self.vm_manager.vmnet_bridge_name(&machine.vm_id) + self.vm_manager.vmnet_bridge_target(&machine.vm_id) } /// Returns the bridge NIC MAC address for a machine's VM. diff --git a/app/arcbox-core/src/route_reconciler.rs b/app/arcbox-core/src/route_reconciler.rs index 40b1d4066..62a620168 100644 --- a/app/arcbox-core/src/route_reconciler.rs +++ b/app/arcbox-core/src/route_reconciler.rs @@ -7,13 +7,11 @@ //! `arcbox-helper` is a pure mutation executor — the daemon tells it //! exactly which interface to add/remove a route for via tarpc RPC. -use std::time::Duration; - use arcbox_helper::client::{Client, ClientError}; use arcbox_helper::error::HelperError; use arcbox_route::{Ipv4Net, RouteInfo}; -use crate::bridge_discovery; +use crate::bridge_discovery::BridgeTarget; /// Preferred route covering the complete container address range. pub const CONTAINER_SUBNET: &str = "172.16.0.0/12"; @@ -21,21 +19,13 @@ pub const CONTAINER_SUBNET: &str = "172.16.0.0/12"; /// More-specific routes used when another network service owns the exact `/12`. pub const CONTAINER_SPLIT_SUBNETS: [&str; 2] = ["172.16.0.0/13", "172.24.0.0/13"]; -/// Maximum retry attempts for transient route installation failures. -const MAX_ROUTE_ATTEMPTS: u32 = 5; - -/// Delay between retry attempts. -const ROUTE_RETRY_INTERVAL: Duration = Duration::from_secs(2); - /// Errors from route installation. /// -/// All variants are retryable — the caller decides whether to retry via -/// [`ensure_route_with_retry`]. +/// All variants are retryable — the route controller owns the retry cadence. #[derive(Debug, thiserror::Error)] pub enum RouteError { - /// Bridge MAC not found in kernel FDB. Retryable — the bridge/FDB may - /// not have stabilized yet (VM just started, vmenet member not learned). - #[error("bridge not found in kernel FDB")] + /// The expected bridge identity is not currently available. + #[error("bridge identity is not ready")] BridgeNotReady, /// Helper daemon not reachable. Retryable. #[error("helper unavailable: {0}")] @@ -58,6 +48,7 @@ impl From for RouteError { | ClientError::Rpc(_) | ClientError::UnrecognizedVersion(_) | ClientError::IncompatibleVersion { .. } => Self::HelperUnavailable(e.to_string()), + ClientError::Helper(HelperError::RouteInterfaceChanged { .. }) => Self::BridgeNotReady, ClientError::Helper(err) => Self::RouteFailed(err.to_string()), } } @@ -170,9 +161,15 @@ fn classify_route(route: Option<&RouteInfo>, bridge_ifindex: u16) -> ExactRouteS } } -fn inspect_routes_sync(bridge_name: &str) -> Result { +fn inspect_routes_sync( + bridge_name: &str, + expected_ifindex: Option, +) -> Result { let bridge_ifindex = arcbox_route::interface_index(bridge_name).map_err(|_| RouteError::BridgeNotReady)?; + if expected_ifindex.is_some_and(|expected| expected != bridge_ifindex) { + return Err(RouteError::BridgeNotReady); + } let preferred = parse_network(CONTAINER_SUBNET)?; let split = [ parse_network(CONTAINER_SPLIT_SUBNETS[0])?, @@ -189,15 +186,25 @@ fn inspect_routes_sync(bridge_name: &str) -> Result { }) } -async fn inspect_routes(bridge_name: &str) -> Result { +async fn inspect_routes( + bridge_name: &str, + expected_ifindex: Option, +) -> Result { let bridge_name = bridge_name.to_string(); - tokio::task::spawn_blocking(move || inspect_routes_sync(&bridge_name)) + tokio::task::spawn_blocking(move || inspect_routes_sync(&bridge_name, expected_ifindex)) .await .map_err(|error| RouteError::RouteFailed(format!("route check task failed: {error}")))? } -async fn add_route(client: &Client, subnet: &str, bridge_name: &str) -> Result { - match client.route_add(subnet, bridge_name).await { +async fn add_route( + client: &Client, + subnet: &str, + target: &BridgeTarget, +) -> Result { + match client + .route_add_for_interface(subnet, &target.name, target.ifindex) + .await + { Ok(()) => Ok(true), Err(ClientError::Helper(HelperError::RouteConflict { .. })) => Ok(false), Err(error) => Err(error.into()), @@ -205,7 +212,7 @@ async fn add_route(client: &Client, subnet: &str, bridge_name: &str) -> Result Result<(), RouteError> { if let Some(index) = snapshot @@ -230,12 +237,12 @@ async fn ensure_split_routes( if snapshot.split[index] == ExactRouteState::Owned { continue; } - if add_route(&client, subnet, bridge_name).await? { + if add_route(&client, subnet, target).await? { snapshot.split[index] = ExactRouteState::Owned; continue; } - snapshot = inspect_routes(bridge_name).await?; + snapshot = inspect_routes(&target.name, Some(target.ifindex)).await?; if snapshot.split[index] != ExactRouteState::Owned { return Err(RouteError::RouteConflict { subnet: (*subnet).to_string(), @@ -246,7 +253,7 @@ async fn ensure_split_routes( } async fn reconcile_with_snapshot( - bridge_name: &str, + target: &BridgeTarget, mode: RouteMode, mut snapshot: RouteSnapshot, ) -> Result { @@ -260,154 +267,53 @@ async fn reconcile_with_snapshot( ReconcileAction::EnsureSplit => {} ReconcileAction::AddPreferred => { let client = Client::connect().await?; - if add_route(&client, CONTAINER_SUBNET, bridge_name).await? { + if add_route(&client, CONTAINER_SUBNET, target).await? { return Ok(RouteMode::Preferred); } // Another service won the add race. Re-query before deciding: an // ArcBox route added by a concurrent reconciler is already good. - snapshot = inspect_routes(bridge_name).await?; + snapshot = inspect_routes(&target.name, Some(target.ifindex)).await?; if snapshot.preferred == ExactRouteState::Owned { return Ok(RouteMode::Preferred); } } } - ensure_split_routes(bridge_name, snapshot).await?; + ensure_split_routes(target, snapshot).await?; tracing::info!( preferred = CONTAINER_SUBNET, lower = CONTAINER_SPLIT_SUBNETS[0], upper = CONTAINER_SPLIT_SUBNETS[1], - bridge = bridge_name, + bridge = target.name, "external container route detected; switched to sticky split fallback" ); Ok(RouteMode::SplitFallback) } -/// Reconciles the container routes while preserving a sticky lifecycle mode. -pub async fn reconcile_route_for_bridge( - bridge_name: &str, +/// Reconciles the container routes for a bridge identity while preserving its mode. +pub async fn reconcile_route_for_target( + target: &BridgeTarget, mode: RouteMode, ) -> Result { - let snapshot = inspect_routes(bridge_name).await?; - reconcile_with_snapshot(bridge_name, mode, snapshot).await + let snapshot = inspect_routes(&target.name, Some(target.ifindex)).await?; + reconcile_with_snapshot(target, mode, snapshot).await } -/// Detects the route shape left by this VM lifecycle and reconciles it. -pub async fn initialize_route_for_bridge(bridge_name: &str) -> Result { - let snapshot = inspect_routes(bridge_name).await?; - reconcile_with_snapshot(bridge_name, snapshot.initial_mode(), snapshot).await -} - -/// Checks whether the container subnet is an interface route through `bridge_name`. -/// -/// The routing query is unprivileged but blocking, so it runs outside the async -/// executor. A route through the expected interface still fails the check when -/// `RTF_GATEWAY` remains set, which is the invalid state produced when a VPN's -/// gateway route is changed in place. -pub async fn container_route_matches_bridge(bridge_name: &str) -> Result { - Ok(container_route_mode(bridge_name).await?.is_some()) +/// Detects and reconciles the route shape for a newly attached bridge identity. +pub async fn initialize_route_for_target(target: &BridgeTarget) -> Result { + let snapshot = inspect_routes(&target.name, Some(target.ifindex)).await?; + reconcile_with_snapshot(target, snapshot.initial_mode(), snapshot).await } /// Returns the healthy route shape currently installed through `bridge_name`. pub async fn container_route_mode(bridge_name: &str) -> Result, RouteError> { - let snapshot = inspect_routes(bridge_name).await?; + let snapshot = inspect_routes(bridge_name, None).await?; if !snapshot.is_healthy() { return Ok(None); } Ok(Some(snapshot.initial_mode())) } -/// Performs one route installation attempt through a known bridge interface. -/// -/// Callers that need retries own the retry cadence. Keeping this operation -/// single-shot prevents a continuous route guard from blocking inside a nested -/// retry loop when another network service replaces the route. -pub async fn repair_route_for_bridge(bridge_name: &str) -> Result<(), RouteError> { - initialize_route_for_bridge(bridge_name).await.map(|_| ()) -} - -/// Ensures the container subnet route points to the correct bridge. -/// -/// 1. Resolves bridge MAC → bridge interface via kernel FDB (`ifbridge`) -/// 2. Calls helper via tarpc to add the route -/// -/// Called on: VM ready, VM recovery, daemon cold-start reconcile. -async fn ensure_route(bridge_mac: &str) -> Result<(), RouteError> { - // Step 1: Resolve MAC → bridge via kernel FDB (typed API, no text parsing). - let mac = bridge_mac.to_string(); - let bridge = tokio::task::spawn_blocking(move || bridge_discovery::resolve_bridge_by_mac(&mac)) - .await - .unwrap_or(None) - .ok_or(RouteError::BridgeNotReady)?; - - // Step 2: Tell helper to add the route. - repair_route_for_bridge(&bridge.name).await?; - - tracing::info!( - bridge = %bridge.name, - %bridge_mac, - "container route ensured" - ); - Ok(()) -} - -/// Ensures the container subnet route with automatic retry on transient failures. -/// -/// All [`RouteError`] variants are treated as retryable. Retries up to 5 times -/// with 2-second intervals (~10s total). This covers: -/// - Bridge FDB not yet populated after VM start (~1-2s to learn MAC) -/// - Helper daemon not yet started by launchd (first connection) -pub async fn ensure_route_with_retry(bridge_mac: &str) -> Result<(), RouteError> { - for attempt in 1..=MAX_ROUTE_ATTEMPTS { - match ensure_route(bridge_mac).await { - Ok(()) => return Ok(()), - Err(ref e) if attempt < MAX_ROUTE_ATTEMPTS => { - tracing::debug!( - attempt, - max_attempts = MAX_ROUTE_ATTEMPTS, - error = %e, - "route install failed, retrying" - ); - tokio::time::sleep(ROUTE_RETRY_INTERVAL).await; - } - Err(e) => { - tracing::warn!( - attempt, - error = %e, - "route install failed after all attempts" - ); - return Err(e); - } - } - } - unreachable!() -} - -/// Ensures the container subnet route using a known bridge interface name. -/// -/// When vmnet.framework creates the bridge, we know the interface immediately — -/// no need to scan the kernel FDB. Only retries for helper readiness. -#[cfg(all(feature = "vmnet", target_os = "macos"))] -pub async fn ensure_route_for_bridge(bridge_name: &str) -> Result<(), RouteError> { - for attempt in 1..=2 { - match repair_route_for_bridge(bridge_name).await { - Ok(()) => { - tracing::info!( - bridge = bridge_name, - "container route ensured (vmnet direct)" - ); - return Ok(()); - } - Err(ref e) if attempt < 2 => { - tracing::debug!(attempt, error = %e, "vmnet route install retry"); - tokio::time::sleep(ROUTE_RETRY_INTERVAL).await; - } - Err(e) => return Err(e), - } - } - unreachable!() -} - #[cfg(test)] mod tests { use super::*; diff --git a/app/arcbox-core/src/vm.rs b/app/arcbox-core/src/vm.rs index 1f5cfe9d3..ff87bf17b 100644 --- a/app/arcbox-core/src/vm.rs +++ b/app/arcbox-core/src/vm.rs @@ -23,6 +23,8 @@ struct VmEntry { info: VmInfo, config: VmConfig, vmm: Option, + #[cfg(all(target_os = "macos", feature = "vmnet"))] + bridge_target: Option, } /// VM manager. @@ -59,6 +61,8 @@ impl VmManager { info, config, vmm: None, + #[cfg(all(target_os = "macos", feature = "vmnet"))] + bridge_target: None, }; self.vms @@ -225,6 +229,10 @@ impl VmManager { } entry.info.state = MachineState::Starting; + #[cfg(all(target_os = "macos", feature = "vmnet"))] + { + entry.bridge_target = None; + } let vmm_config = Self::build_vmm_config(entry); @@ -287,6 +295,10 @@ impl VmManager { } entry.vmm = None; + #[cfg(all(target_os = "macos", feature = "vmnet"))] + { + entry.bridge_target = None; + } entry.info.state = MachineState::Stopped; tracing::info!("Stopped VM {}", id); @@ -323,6 +335,10 @@ impl VmManager { let entry = vms .get_mut(id) .ok_or_else(|| CoreError::not_found(id.to_string()))?; + #[cfg(all(target_os = "macos", feature = "vmnet"))] + { + entry.bridge_target = None; + } let vmm = entry .vmm .as_mut() @@ -396,6 +412,10 @@ impl VmManager { } entry.info.state = MachineState::Stopping; + #[cfg(all(target_os = "macos", feature = "vmnet"))] + { + entry.bridge_target = None; + } entry .vmm .take() @@ -566,6 +586,10 @@ impl VmManager { vmm.set_skip_hypervisor_stop(); drop(vmm); } + #[cfg(all(target_os = "macos", feature = "vmnet"))] + { + entry.bridge_target = None; + } entry.info.state = MachineState::Stopped; tracing::warn!("Force-stopped VM {} without hypervisor stop", id); @@ -1032,21 +1056,30 @@ impl VmManager { entry.vmm.as_mut()?.take_inbound_listener_manager() } - /// Returns the vmnet bridge interface name for a running VM. + /// Returns the vmnet bridge identity for a running VM. /// - /// After vmnet creates the shared interface, the system also creates a - /// bridge with a vmnet member. We resolve it via the MAC that vmnet - /// reported. Since vmnet has already started, the bridge is immediately - /// present — no retry needed. + /// FDB discovery attaches the bridge identity to this VMM incarnation. + /// The name and interface index remain cached while that kernel identity + /// exists, independent of later FDB expiry. #[cfg(all(target_os = "macos", feature = "vmnet"))] - pub fn vmnet_bridge_name(&self, id: &VmId) -> Option { - let vms = self.vms.read().ok()?; - let entry = vms.get(id)?; + pub fn vmnet_bridge_target(&self, id: &VmId) -> Option { + let mut vms = self.vms.write().ok()?; + let entry = vms.get_mut(id)?; + if entry + .bridge_target + .as_ref() + .is_some_and(crate::bridge_discovery::BridgeTarget::is_current) + { + return entry.bridge_target.clone(); + } + entry.bridge_target = None; + let vmm = entry.vmm.as_ref()?; let info = vmm.vmnet_interface_info()?; let mac_str = arcbox_net::darwin::format_mac(&info.mac); let bridge = crate::bridge_discovery::resolve_bridge_by_mac(&mac_str)?; - Some(bridge.name) + entry.bridge_target = Some(bridge.clone()); + Some(bridge) } #[cfg(test)] diff --git a/app/arcbox-core/src/vm_lifecycle/boot.rs b/app/arcbox-core/src/vm_lifecycle/boot.rs index 9b4d7c1f4..3d4d00041 100644 --- a/app/arcbox-core/src/vm_lifecycle/boot.rs +++ b/app/arcbox-core/src/vm_lifecycle/boot.rs @@ -123,10 +123,6 @@ impl LifecycleShared { tokio::task::spawn_blocking(move || mm.reboot(&name)) .await .map_err(|e| CoreError::Vm(format!("reboot task panicked: {e}")))??; - // The teardown dropped the bridge along with the VMM; the fresh boot - // created a new one, so the host container-subnet route must be - // reinstalled exactly like after a normal start. - self.spawn_route_reconciler(); self.wait_for_agent(timeout).await?; self.sync_guest_clock().await; Ok(()) @@ -248,7 +244,6 @@ impl LifecycleShared { match self.machine_manager.start(&self.machine_name).await { Ok(()) => { tracing::info!("Default VM started successfully"); - self.spawn_route_reconciler(); return Ok(()); } Err(e) => { @@ -292,53 +287,6 @@ impl LifecycleShared { } } - /// Installs the host route for container subnets via the bridge NIC. - /// - /// Non-blocking: retries transient failures (helper not ready, bridge FDB - /// not populated) but does not gate VM readiness. - #[cfg(all(target_os = "macos", feature = "vmnet"))] - fn spawn_route_reconciler(&self) { - if let Some(bridge) = self.machine_manager.vmnet_bridge_name(&self.machine_name) { - // vmnet path: bridge name is known instantly, only need - // helper retry (1-2 attempts for XPC readiness). - let event_bus = self.event_bus.clone(); - let name = self.machine_name.clone(); - drop(tokio::spawn(async move { - match crate::route_reconciler::ensure_route_for_bridge(&bridge).await { - Ok(()) => { - event_bus.publish(Event::ContainerRouteInstalled { name }); - } - Err(e) => { - tracing::warn!(error = %e, "failed to install container route (vmnet)"); - } - } - })); - } - } - - /// See the vmnet variant; this path discovers the bridge by scanning the - /// kernel FDB (retries up to ~10s for FDB learning). - #[cfg(all(target_os = "macos", not(feature = "vmnet")))] - fn spawn_route_reconciler(&self) { - if let Some(mac) = self.machine_manager.bridge_mac(&self.machine_name) { - let event_bus = self.event_bus.clone(); - let name = self.machine_name.clone(); - drop(tokio::spawn(async move { - match crate::route_reconciler::ensure_route_with_retry(&mac).await { - Ok(()) => { - event_bus.publish(Event::ContainerRouteInstalled { name }); - } - Err(e) => { - tracing::warn!(error = %e, "failed to install container route"); - } - } - })); - } - } - - #[cfg(not(target_os = "macos"))] - fn spawn_route_reconciler(&self) {} - /// Creates the default machine with EROFS rootfs and no initramfs. /// /// Block devices: diff --git a/app/arcbox-daemon/Cargo.toml b/app/arcbox-daemon/Cargo.toml index eabeb40d3..4a4b3eece 100644 --- a/app/arcbox-daemon/Cargo.toml +++ b/app/arcbox-daemon/Cargo.toml @@ -52,9 +52,8 @@ libproc = { workspace = true } # `vmnet` is on by default: the bridge NIC (NIC2) for host→container L3 # routing only compiles behind it, and no build pipeline passes # `--features` — without it release daemons ship without container -# routing, `route_installed` stays false, and the desktop app falls back -# to localhost instead of *.arcbox.local domains. Failure to create the -# vmnet interface at runtime (e.g. unsigned dev builds) is non-fatal. +# routing and Desktop links fall back to published host ports. Failure to +# create the vmnet interface at runtime (e.g. unsigned dev builds) is non-fatal. default = ["gic", "vmnet"] vmnet = ["arcbox-core/vmnet"] gic = ["arcbox-core/gic"] diff --git a/app/arcbox-daemon/src/container_route.rs b/app/arcbox-daemon/src/container_route.rs new file mode 100644 index 000000000..c07dc33c6 --- /dev/null +++ b/app/arcbox-daemon/src/container_route.rs @@ -0,0 +1,320 @@ +//! Host route ownership for the System VM's container networks. + +use std::sync::Arc; +use std::time::Duration; + +use arcbox_api::SetupState; +use arcbox_core::bridge_discovery::BridgeTarget; +use arcbox_core::route_reconciler::{RouteError, RouteMode}; +use arcbox_core::{Runtime, VmLifecycleState}; + +const POLL_INTERVAL: Duration = Duration::from_secs(30); +const RETRY_INTERVAL: Duration = Duration::from_secs(2); +const EVENT_DEBOUNCE: Duration = Duration::from_millis(250); + +#[derive(Default)] +struct ControllerState { + generation: u64, + bridge: Option, + mode: Option, +} + +impl ControllerState { + fn observe_vm(&mut self, generation: u64, lifecycle: VmLifecycleState) -> bool { + if !lifecycle.is_ready() || self.generation != generation { + self.generation = generation; + self.bridge = None; + self.mode = None; + } + lifecycle.is_ready() + } + + fn clear_bridge(&mut self) { + self.bridge = None; + self.mode = None; + } + + fn accept_result( + &mut self, + expected_generation: u64, + current_generation: u64, + lifecycle: VmLifecycleState, + ) -> bool { + if expected_generation == current_generation && lifecycle.is_ready() { + return true; + } + self.observe_vm(current_generation, lifecycle); + false + } +} + +pub fn spawn( + runtime: Arc, + setup_state: Arc, + shutdown: tokio_util::sync::CancellationToken, +) -> tokio::task::JoinHandle<()> { + tokio::spawn(run(runtime, setup_state, shutdown)) +} + +async fn run( + runtime: Arc, + setup_state: Arc, + shutdown: tokio_util::sync::CancellationToken, +) { + let mut vm_state = runtime.subscribe_system_vm_state(); + let mut ticker = tokio::time::interval(POLL_INTERVAL); + ticker.set_missed_tick_behavior(tokio::time::MissedTickBehavior::Skip); + let mut watcher = open_route_watcher(); + let mut state = ControllerState::default(); + let mut consecutive_failures = 0u32; + + loop { + let route_event = tokio::select! { + () = shutdown.cancelled() => break, + _ = ticker.tick() => false, + changed = vm_state.changed() => { + if changed.is_err() { + break; + } + false + } + event = next_managed_route_event(watcher.as_ref()) => { + match event { + Ok(()) => true, + Err(error) if error.raw_os_error() == Some(libc::ENOBUFS) => { + tracing::debug!( + "route event queue overflowed; reconciling current state" + ); + true + } + Err(error) => { + tracing::warn!(%error, "route event watcher failed; polling remains active"); + watcher = None; + false + } + } + } + }; + + if route_event { + tokio::select! { + () = shutdown.cancelled() => break, + () = tokio::time::sleep(EVENT_DEBOUNCE) => {} + } + if let Some(watcher) = watcher.as_ref() { + drain_route_events(watcher); + } + } else if watcher.is_none() { + watcher = open_route_watcher(); + } + + let lifecycle = *vm_state.borrow_and_update(); + let generation = runtime.system_vm_restart_generation(); + if !state.observe_vm(generation, lifecycle) { + setup_state.set_route_installed(false); + consecutive_failures = 0; + ticker.reset_after(POLL_INTERVAL); + continue; + } + + if state + .bridge + .as_ref() + .is_some_and(|bridge| !bridge.is_current()) + { + state.clear_bridge(); + } + + if state.bridge.is_none() { + let runtime_for_bridge = Arc::clone(&runtime); + let bridge = match tokio::task::spawn_blocking(move || { + resolve_container_bridge(&runtime_for_bridge) + }) + .await + { + Ok(bridge) => bridge, + Err(error) => { + tracing::warn!(%error, "container bridge discovery task failed"); + None + } + }; + let current_lifecycle = *vm_state.borrow(); + let current_generation = runtime.system_vm_restart_generation(); + if !state.accept_result(generation, current_generation, current_lifecycle) { + setup_state.set_route_installed(false); + consecutive_failures = 0; + ticker.reset_after(RETRY_INTERVAL); + continue; + } + state.bridge = bridge; + } + + let Some(bridge) = state.bridge.clone() else { + setup_state.set_route_installed(false); + consecutive_failures = 0; + ticker.reset_after(RETRY_INTERVAL); + continue; + }; + + let result = match state.mode { + Some(mode) => { + arcbox_core::route_reconciler::reconcile_route_for_target(&bridge, mode).await + } + None => arcbox_core::route_reconciler::initialize_route_for_target(&bridge).await, + }; + + let current_lifecycle = *vm_state.borrow(); + let current_generation = runtime.system_vm_restart_generation(); + if !state.accept_result(generation, current_generation, current_lifecycle) { + setup_state.set_route_installed(false); + consecutive_failures = 0; + ticker.reset_after(RETRY_INTERVAL); + continue; + } + + match result { + Ok(mode) => { + state.mode = Some(mode); + setup_state.set_route_installed(true); + consecutive_failures = 0; + ticker.reset_after(POLL_INTERVAL); + } + Err(error) => { + setup_state.set_route_installed(false); + consecutive_failures = consecutive_failures.saturating_add(1); + if matches!(&error, RouteError::BridgeNotReady) { + state.clear_bridge(); + } + let retry_after = if matches!(&error, RouteError::RouteConflict { .. }) { + POLL_INTERVAL + } else { + RETRY_INTERVAL + }; + ticker.reset_after(retry_after); + if should_log_failure(consecutive_failures) { + tracing::warn!( + %error, + bridge = %bridge.name, + consecutive_failures, + "container route reconciliation failed" + ); + } + } + } + } +} + +fn open_route_watcher() -> Option> { + match arcbox_route::RouteWatcher::open().and_then(tokio::io::unix::AsyncFd::new) { + Ok(watcher) => Some(watcher), + Err(error) => { + tracing::warn!(%error, "route event watcher unavailable; using polling"); + None + } + } +} + +async fn next_managed_route_event( + watcher: Option<&tokio::io::unix::AsyncFd>, +) -> std::io::Result<()> { + let Some(watcher) = watcher else { + return std::future::pending().await; + }; + + loop { + let mut ready = watcher.readable().await?; + match ready.try_io(|inner| inner.get_ref().read_event()) { + Ok(Ok(Some(event))) if event.network.is_some_and(is_managed_route) => return Ok(()), + Ok(Ok(_)) => {} + Ok(Err(error)) if error.kind() == std::io::ErrorKind::InvalidData => { + tracing::debug!(%error, "ignored malformed route event"); + } + Ok(Err(error)) => return Err(error), + Err(_would_block) => {} + } + } +} + +fn drain_route_events(watcher: &tokio::io::unix::AsyncFd) { + for _ in 0..256 { + match watcher.get_ref().read_event() { + Ok(_) => {} + Err(error) if error.kind() == std::io::ErrorKind::WouldBlock => break, + Err(_) => break, + } + } +} + +fn is_managed_route(network: arcbox_route::Ipv4Net) -> bool { + let network = network.to_string(); + network == arcbox_core::route_reconciler::CONTAINER_SUBNET + || arcbox_core::route_reconciler::CONTAINER_SPLIT_SUBNETS + .iter() + .any(|candidate| network == *candidate) +} + +#[cfg(feature = "vmnet")] +fn resolve_container_bridge(runtime: &Runtime) -> Option { + runtime + .machine_manager() + .vmnet_bridge_target(arcbox_core::DEFAULT_MACHINE_NAME) +} + +#[cfg(not(feature = "vmnet"))] +fn resolve_container_bridge(runtime: &Runtime) -> Option { + let mac = runtime + .machine_manager() + .bridge_mac(arcbox_core::DEFAULT_MACHINE_NAME)?; + arcbox_core::bridge_discovery::resolve_bridge_by_mac(&mac) +} + +fn should_log_failure(consecutive_failures: u32) -> bool { + consecutive_failures == 1 || consecutive_failures.is_multiple_of(30) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn bridge() -> BridgeTarget { + BridgeTarget { + name: "bridge100".to_string(), + ifindex: 42, + } + } + + #[test] + fn bridge_identity_is_cached_for_one_vm_generation() { + let mut state = ControllerState::default(); + + assert!(state.observe_vm(7, VmLifecycleState::Running)); + state.bridge = Some(bridge()); + state.mode = Some(RouteMode::SplitFallback); + + assert!(state.observe_vm(7, VmLifecycleState::Idle)); + assert_eq!(state.bridge, Some(bridge())); + assert_eq!(state.mode, Some(RouteMode::SplitFallback)); + + assert!(state.observe_vm(8, VmLifecycleState::Running)); + assert!(state.bridge.is_none()); + assert!(state.mode.is_none()); + + state.bridge = Some(bridge()); + assert!(!state.observe_vm(8, VmLifecycleState::Stopped)); + assert!(state.bridge.is_none()); + } + + #[test] + fn result_from_replaced_vm_is_discarded() { + let mut state = ControllerState { + generation: 7, + bridge: Some(bridge()), + mode: Some(RouteMode::Preferred), + }; + + assert!(!state.accept_result(7, 8, VmLifecycleState::Starting)); + assert_eq!(state.generation, 8); + assert!(state.bridge.is_none()); + assert!(state.mode.is_none()); + } +} diff --git a/app/arcbox-daemon/src/context.rs b/app/arcbox-daemon/src/context.rs index be6b4fff1..77752ba67 100644 --- a/app/arcbox-daemon/src/context.rs +++ b/app/arcbox-daemon/src/context.rs @@ -129,6 +129,6 @@ pub struct ServiceHandles { pub docker: Option>, pub grpc: tokio::task::JoinHandle<()>, pub kubernetes_proxy: Option>, - /// Host container-route guard; present only on macOS with the Linux VM enabled. - pub route_guard: Option>, + /// Host container-route controller; present only on macOS with the Linux VM enabled. + pub route_controller: Option>, } diff --git a/app/arcbox-daemon/src/main.rs b/app/arcbox-daemon/src/main.rs index f86858792..8d1b83371 100644 --- a/app/arcbox-daemon/src/main.rs +++ b/app/arcbox-daemon/src/main.rs @@ -1,5 +1,7 @@ //! ArcBox daemon — orchestrates VM, networking, and API services. +#[cfg(target_os = "macos")] +mod container_route; mod context; mod control_plane; mod dns_service; diff --git a/app/arcbox-daemon/src/recovery.rs b/app/arcbox-daemon/src/recovery.rs index 05b562a84..bebf5fb47 100644 --- a/app/arcbox-daemon/src/recovery.rs +++ b/app/arcbox-daemon/src/recovery.rs @@ -1,5 +1,4 @@ -//! Post-startup recovery: re-establish networking for surviving containers -//! and reconcile host routes. +//! Post-startup recovery for networking state that survives daemon restarts. use std::path::Path; use std::sync::Arc; @@ -15,7 +14,6 @@ use crate::self_setup::SetupTask as _; /// Runs all recovery and best-effort setup tasks. /// /// - Recovers DNS and port forwarding for containers that survived a daemon restart -/// - Re-installs the container subnet route (cold-start reconcile) /// - Installs DNS resolver and Docker socket via helper (best-effort) pub async fn run(ctx: &DaemonContext, runtime: &Arc) { // Every recovery task here reconciles Linux-VM container networking or @@ -26,41 +24,6 @@ pub async fn run(ctx: &DaemonContext, runtime: &Arc) { recover_container_networking(runtime).await; - // Cold-start route reconcile (non-blocking). This is load-bearing after - // app updates or daemon restarts where the VM survives but the host route - // may have been removed or stolen by another network service. - #[cfg(all(target_os = "macos", feature = "vmnet"))] - { - use arcbox_core::DEFAULT_MACHINE_NAME; - if let Some(ColdStartRoutePlan::VmnetBridge(bridge)) = cold_start_route_plan( - runtime - .machine_manager() - .vmnet_bridge_name(DEFAULT_MACHINE_NAME), - None, - ) { - spawn_vmnet_route_reconcile(Arc::clone(&ctx.setup_state), bridge); - } - } - - #[cfg(all(target_os = "macos", not(feature = "vmnet")))] - { - use arcbox_core::DEFAULT_MACHINE_NAME; - if let Some(ColdStartRoutePlan::BridgeMac(mac)) = cold_start_route_plan( - None, - runtime.machine_manager().bridge_mac(DEFAULT_MACHINE_NAME), - ) { - let setup_state = Arc::clone(&ctx.setup_state); - drop(tokio::spawn(async move { - match arcbox_core::route_reconciler::ensure_route_with_retry(&mac).await { - Ok(()) => setup_state.set_route_installed(true), - Err(e) => { - tracing::warn!(error = %e, "failed to install container route on cold start"); - } - } - })); - } - } - // Best-effort self-setup (non-blocking). // // The `/etc/resolver/` entry belongs to the daemon serving the @@ -168,43 +131,6 @@ pub async fn run(ctx: &DaemonContext, runtime: &Arc) { } } -#[cfg(target_os = "macos")] -#[derive(Debug, PartialEq, Eq)] -enum ColdStartRoutePlan { - #[cfg(feature = "vmnet")] - VmnetBridge(String), - #[cfg(not(feature = "vmnet"))] - BridgeMac(String), -} - -#[cfg(all(target_os = "macos", feature = "vmnet"))] -fn cold_start_route_plan( - vmnet_bridge: Option, - _bridge_mac: Option, -) -> Option { - vmnet_bridge.map(ColdStartRoutePlan::VmnetBridge) -} - -#[cfg(all(target_os = "macos", not(feature = "vmnet")))] -fn cold_start_route_plan( - _vmnet_bridge: Option, - bridge_mac: Option, -) -> Option { - bridge_mac.map(ColdStartRoutePlan::BridgeMac) -} - -#[cfg(all(target_os = "macos", feature = "vmnet"))] -fn spawn_vmnet_route_reconcile(setup_state: Arc, bridge: String) { - drop(tokio::spawn(async move { - match arcbox_core::route_reconciler::ensure_route_for_bridge(&bridge).await { - Ok(()) => setup_state.set_route_installed(true), - Err(e) => { - tracing::warn!(error = %e, "failed to install container route on cold start (vmnet)"); - } - } - })); -} - // ============================================================================= // Docker CLI tools // ============================================================================= @@ -319,35 +245,3 @@ async fn recover_container_networking(runtime: &Arc) { ); } } - -#[cfg(test)] -mod tests { - use super::*; - - #[cfg(all(target_os = "macos", feature = "vmnet"))] - #[test] - fn cold_start_route_plan_uses_vmnet_bridge() { - let plan = cold_start_route_plan(Some("bridge100".to_string()), None); - - assert_eq!( - plan, - Some(ColdStartRoutePlan::VmnetBridge("bridge100".to_string())) - ); - } - - #[cfg(all(target_os = "macos", feature = "vmnet"))] - #[test] - fn cold_start_route_plan_skips_when_vmnet_bridge_is_unknown() { - let plan = cold_start_route_plan(None, Some("fe:b2:14:4d:a4:64".to_string())); - - assert_eq!(plan, None); - } - - #[cfg(all(target_os = "macos", not(feature = "vmnet")))] - #[test] - fn cold_start_route_plan_uses_bridge_mac_without_vmnet() { - let plan = cold_start_route_plan(Some("bridge100".to_string()), Some("mac".to_string())); - - assert_eq!(plan, Some(ColdStartRoutePlan::BridgeMac("mac".to_string()))); - } -} diff --git a/app/arcbox-daemon/src/services.rs b/app/arcbox-daemon/src/services.rs index 08c88685e..a1aa04923 100644 --- a/app/arcbox-daemon/src/services.rs +++ b/app/arcbox-daemon/src/services.rs @@ -17,11 +17,6 @@ use tracing::{info, warn}; use crate::context::{DaemonContext, ServiceHandles}; use crate::dns_service::DnsService; -#[cfg(target_os = "macos")] -const ROUTE_POLL_INTERVAL: std::time::Duration = std::time::Duration::from_secs(30); -#[cfg(target_os = "macos")] -const ROUTE_EVENT_DEBOUNCE: std::time::Duration = std::time::Duration::from_millis(250); - /// Starts the gRPC server with all services. /// /// Called before `init_runtime()` — Machine/Sandbox/Snapshot services will @@ -137,35 +132,23 @@ pub async fn start_services( None }; - // Mirror route-install events into SetupStatus. VM (re)starts install - // the container route from vm_lifecycle, outside the cold-start - // recovery path that sets the flag directly — without this bridge, - // route_installed would stay stale until the next daemon restart. - let route_events = runtime.event_bus().subscribe(); - let route_state = Arc::clone(&ctx.setup_state); - let route_shutdown = ctx.shutdown.clone(); - drop(tokio::spawn(async move { - route_status_loop(route_events, route_state, route_shutdown).await; - })); - #[cfg(target_os = "macos")] - let route_guard = linux_vm.then(|| { - let runtime = Arc::clone(runtime); - let setup_state = Arc::clone(&ctx.setup_state); - let shutdown = ctx.shutdown.clone(); - tokio::spawn(async move { - container_route_guard(runtime, setup_state, shutdown).await; - }) + let route_controller = linux_vm.then(|| { + crate::container_route::spawn( + Arc::clone(runtime), + Arc::clone(&ctx.setup_state), + ctx.shutdown.clone(), + ) }); #[cfg(not(target_os = "macos"))] - let route_guard = None; + let route_controller = None; Ok(ServiceHandles { dns, docker, grpc, kubernetes_proxy, - route_guard, + route_controller, }) } @@ -217,237 +200,6 @@ async fn vm_running_loop( } } -/// Mirrors VM lifecycle events into `SetupState.route_installed`. -/// -/// `ContainerRouteInstalled` sets the flag; `MachineStopped` clears it -/// (the bridge interface — and with it the host route — dies with the VM). -async fn route_status_loop( - mut events: tokio::sync::broadcast::Receiver, - setup_state: Arc, - shutdown: tokio_util::sync::CancellationToken, -) { - use arcbox_core::event::Event; - use tokio::sync::broadcast::error::RecvError; - - loop { - tokio::select! { - () = shutdown.cancelled() => break, - event = events.recv() => match event { - Ok(Event::ContainerRouteInstalled { .. }) => { - setup_state.set_route_installed(true); - } - Ok(Event::MachineStopped { .. }) => { - setup_state.set_route_installed(false); - } - Ok(_) => {} - // Missed events under load; state converges on the next - // route-install or stop event. - Err(RecvError::Lagged(_)) => {} - Err(RecvError::Closed) => break, - }, - } - } -} - -#[cfg(target_os = "macos")] -async fn container_route_guard( - runtime: Arc, - setup_state: Arc, - shutdown: tokio_util::sync::CancellationToken, -) { - use arcbox_core::route_reconciler::RouteMode; - - let mut ticker = tokio::time::interval(ROUTE_POLL_INTERVAL); - ticker.set_missed_tick_behavior(tokio::time::MissedTickBehavior::Skip); - let mut watcher = open_route_watcher(); - let mut active: Option<(String, RouteMode)> = None; - let mut consecutive_failures = 0u32; - - loop { - let route_event = tokio::select! { - () = shutdown.cancelled() => break, - _ = ticker.tick() => false, - event = next_managed_route_event(watcher.as_ref()) => { - match event { - Ok(()) => true, - Err(error) if error.raw_os_error() == Some(libc::ENOBUFS) => { - tracing::debug!( - "route event queue overflowed; reconciling current state" - ); - true - } - Err(error) => { - tracing::warn!(%error, "route event watcher failed; polling remains active"); - watcher = None; - false - } - } - } - }; - - if route_event { - tokio::select! { - () = shutdown.cancelled() => break, - () = tokio::time::sleep(ROUTE_EVENT_DEBOUNCE) => {} - } - if let Some(watcher) = watcher.as_ref() { - drain_route_events(watcher); - } - } else if watcher.is_none() { - watcher = open_route_watcher(); - } - - let runtime_for_bridge = Arc::clone(&runtime); - let bridge = match tokio::task::spawn_blocking(move || { - resolve_container_bridge(&runtime_for_bridge) - }) - .await - { - Ok(bridge) => bridge, - Err(error) => { - tracing::warn!(%error, "container bridge resolution task failed"); - None - } - }; - let Some(bridge) = bridge else { - active = None; - setup_state.set_route_installed(false); - consecutive_failures = 0; - continue; - }; - - let current_mode = active - .as_ref() - .filter(|(active_bridge, _)| active_bridge == &bridge) - .map(|(_, mode)| *mode); - let result = match current_mode { - Some(mode) => { - arcbox_core::route_reconciler::reconcile_route_for_bridge(&bridge, mode).await - } - None => arcbox_core::route_reconciler::initialize_route_for_bridge(&bridge).await, - }; - - match result { - Ok(mode) => { - let was_installed = setup_state.current().route_installed; - active = Some((bridge, mode)); - setup_state.set_route_installed(true); - consecutive_failures = 0; - if !was_installed { - runtime.event_bus().publish( - arcbox_core::event::Event::ContainerRouteInstalled { - name: arcbox_core::DEFAULT_MACHINE_NAME.to_string(), - }, - ); - } - } - Err(error) => { - setup_state.set_route_installed(false); - consecutive_failures = consecutive_failures.saturating_add(1); - if should_log_route_failure(consecutive_failures) { - tracing::warn!( - %error, - %bridge, - consecutive_failures, - "container route reconciliation failed" - ); - } - } - } - } -} - -#[cfg(target_os = "macos")] -fn open_route_watcher() -> Option> { - match arcbox_route::RouteWatcher::open().and_then(tokio::io::unix::AsyncFd::new) { - Ok(watcher) => Some(watcher), - Err(error) => { - tracing::warn!(%error, "route event watcher unavailable; using polling"); - None - } - } -} - -#[cfg(target_os = "macos")] -async fn next_managed_route_event( - watcher: Option<&tokio::io::unix::AsyncFd>, -) -> std::io::Result<()> { - let Some(watcher) = watcher else { - return std::future::pending().await; - }; - - loop { - let mut ready = watcher.readable().await?; - match ready.try_io(|inner| inner.get_ref().read_event()) { - Ok(Ok(Some(event))) if event.network.is_some_and(is_managed_route) => return Ok(()), - Ok(Ok(_)) => {} - Ok(Err(error)) if error.kind() == std::io::ErrorKind::InvalidData => { - tracing::debug!(%error, "ignored malformed route event"); - } - Ok(Err(error)) => return Err(error), - Err(_would_block) => {} - } - } -} - -#[cfg(target_os = "macos")] -fn drain_route_events(watcher: &tokio::io::unix::AsyncFd) { - for _ in 0..256 { - match watcher.get_ref().read_event() { - Ok(_) => {} - Err(error) if error.kind() == std::io::ErrorKind::WouldBlock => break, - Err(_) => break, - } - } -} - -#[cfg(target_os = "macos")] -fn is_managed_route(network: arcbox_route::Ipv4Net) -> bool { - let network = network.to_string(); - network == arcbox_core::route_reconciler::CONTAINER_SUBNET - || arcbox_core::route_reconciler::CONTAINER_SPLIT_SUBNETS - .iter() - .any(|candidate| network == *candidate) -} - -#[cfg(all(target_os = "macos", feature = "vmnet"))] -fn resolve_container_bridge(runtime: &Runtime) -> Option { - let machine = runtime - .machine_manager() - .get(arcbox_core::DEFAULT_MACHINE_NAME)?; - if !matches!( - machine.state, - arcbox_core::machine::MachineState::Starting | arcbox_core::machine::MachineState::Running - ) { - return None; - } - runtime - .machine_manager() - .vmnet_bridge_name(arcbox_core::DEFAULT_MACHINE_NAME) -} - -#[cfg(all(target_os = "macos", not(feature = "vmnet")))] -fn resolve_container_bridge(runtime: &Runtime) -> Option { - let machine = runtime - .machine_manager() - .get(arcbox_core::DEFAULT_MACHINE_NAME)?; - if !matches!( - machine.state, - arcbox_core::machine::MachineState::Starting | arcbox_core::machine::MachineState::Running - ) { - return None; - } - let mac = runtime - .machine_manager() - .bridge_mac(arcbox_core::DEFAULT_MACHINE_NAME)?; - arcbox_core::bridge_discovery::resolve_bridge_by_mac(&mac).map(|bridge| bridge.name) -} - -#[cfg(target_os = "macos")] -fn should_log_route_failure(consecutive_failures: u32) -> bool { - consecutive_failures == 1 || consecutive_failures.is_multiple_of(30) -} - async fn register_host_dns(runtime: &Arc) { let network_cfg = &runtime.config().network; let gateway_ip = network_cfg @@ -491,21 +243,8 @@ fn first_address_in_subnet(subnet: &str) -> Option { mod tests { use std::time::Duration; - use arcbox_api::SetupState; - use arcbox_core::event::{Event, EventBus}; - use super::*; - - /// Polls until `route_installed` matches `want` or times out. - async fn wait_for_route_installed(state: &SetupState, want: bool) -> bool { - for _ in 0..200 { - if state.current().route_installed == want { - return true; - } - tokio::time::sleep(Duration::from_millis(5)).await; - } - false - } + use arcbox_api::SetupState; /// Polls until `vm_running` matches `want` or times out. async fn wait_for_vm_running(state: &SetupState, want: bool) -> bool { @@ -553,43 +292,4 @@ mod tests { .expect("loop exits on shutdown") .expect("loop task panicked"); } - - #[tokio::test] - async fn route_status_loop_mirrors_events_into_setup_state() { - let bus = EventBus::new(); - let setup_state = Arc::new(SetupState::new()); - let shutdown = tokio_util::sync::CancellationToken::new(); - - let task = tokio::spawn(route_status_loop( - bus.subscribe(), - Arc::clone(&setup_state), - shutdown.clone(), - )); - - assert!(!setup_state.current().route_installed); - - bus.publish(Event::ContainerRouteInstalled { - name: "default".into(), - }); - assert!(wait_for_route_installed(&setup_state, true).await); - - // Unrelated events leave the flag untouched. - bus.publish(Event::MachineIdle { - name: "default".into(), - }); - tokio::time::sleep(Duration::from_millis(20)).await; - assert!(setup_state.current().route_installed); - - // The bridge dies with the VM, so MachineStopped clears the flag. - bus.publish(Event::MachineStopped { - name: "default".into(), - }); - assert!(wait_for_route_installed(&setup_state, false).await); - - shutdown.cancel(); - tokio::time::timeout(Duration::from_secs(1), task) - .await - .expect("loop exits on shutdown") - .expect("loop task panicked"); - } } diff --git a/app/arcbox-daemon/src/shutdown.rs b/app/arcbox-daemon/src/shutdown.rs index baa3cd8c0..2f9aa9415 100644 --- a/app/arcbox-daemon/src/shutdown.rs +++ b/app/arcbox-daemon/src/shutdown.rs @@ -223,7 +223,7 @@ async fn drain(handles: &mut ServiceHandles) { if let Some(h) = handles.kubernetes_proxy.as_mut() { let _ = h.await; } - if let Some(h) = handles.route_guard.as_mut() { + if let Some(h) = handles.route_controller.as_mut() { let _ = h.await; } }) @@ -242,7 +242,7 @@ async fn drain(handles: &mut ServiceHandles) { if let Some(h) = handles.kubernetes_proxy.as_mut() { h.abort(); } - if let Some(h) = handles.route_guard.as_mut() { + if let Some(h) = handles.route_controller.as_mut() { h.abort(); } } diff --git a/app/arcbox-helper/Cargo.toml b/app/arcbox-helper/Cargo.toml index 1ca7438fb..5423d1c31 100644 --- a/app/arcbox-helper/Cargo.toml +++ b/app/arcbox-helper/Cargo.toml @@ -6,7 +6,7 @@ description = "Privileged helper daemon for host mutations (routes, DNS, sockets # root helper, or behavior the current daemon hard-requires). Ordinary runtime # bumps must NOT change this, or every Desktop user gets an admin-password # reinstall prompt. -version = "1.0.2" +version = "1.0.3" edition.workspace = true rust-version.workspace = true license.workspace = true diff --git a/app/arcbox-helper/src/client.rs b/app/arcbox-helper/src/client.rs index 43aef8e68..491de44f1 100644 --- a/app/arcbox-helper/src/client.rs +++ b/app/arcbox-helper/src/client.rs @@ -87,6 +87,24 @@ impl Client { .await??) } + /// Adds a host route only if `iface` still has `expected_ifindex`. + pub async fn route_add_for_interface( + &self, + subnet: &str, + iface: &str, + expected_ifindex: u16, + ) -> Result<(), ClientError> { + Ok(self + .inner + .route_add_for_interface( + tarpc::context::current(), + subnet.into(), + iface.into(), + expected_ifindex, + ) + .await??) + } + /// Removes the host route for `subnet`. pub async fn route_remove(&self, subnet: &str) -> Result<(), ClientError> { Ok(self diff --git a/app/arcbox-helper/src/error.rs b/app/arcbox-helper/src/error.rs index 35f62eb05..0dec3c4e4 100644 --- a/app/arcbox-helper/src/error.rs +++ b/app/arcbox-helper/src/error.rs @@ -69,6 +69,16 @@ pub enum HelperError { /// An exact route already exists and was left untouched. #[error("route conflict for {subnet}")] RouteConflict { subnet: String }, + + /// The interface name no longer identifies the caller's expected index. + #[error( + "route interface {iface} changed (expected index {expected_ifindex}, actual {actual_ifindex:?})" + )] + RouteInterfaceChanged { + iface: String, + expected_ifindex: u16, + actual_ifindex: Option, + }, } impl HelperError { @@ -138,6 +148,7 @@ impl HelperError { Self::Other(_) => "other", Self::DockerSocketOccupied { .. } => "docker_socket_occupied", Self::RouteConflict { .. } => "route_conflict", + Self::RouteInterfaceChanged { .. } => "route_interface_changed", } } } diff --git a/app/arcbox-helper/src/lib.rs b/app/arcbox-helper/src/lib.rs index 9ceba042a..b74c50169 100644 --- a/app/arcbox-helper/src/lib.rs +++ b/app/arcbox-helper/src/lib.rs @@ -102,6 +102,13 @@ pub trait HelperService { /// Checks whether the ArcBox `/etc/hosts` alias is installed. async fn hosts_alias_status() -> Result; + + /// Adds a host route only if `iface` still has `expected_ifindex`. + async fn route_add_for_interface( + subnet: String, + iface: String, + expected_ifindex: u16, + ) -> Result<(), HelperError>; } /// Low-level connect — use [`client::Client::connect()`] instead. diff --git a/app/arcbox-helper/src/server/handler.rs b/app/arcbox-helper/src/server/handler.rs index 53088481b..f64acb368 100644 --- a/app/arcbox-helper/src/server/handler.rs +++ b/app/arcbox-helper/src/server/handler.rs @@ -77,6 +77,23 @@ impl HelperService for HelperServer { mutations::hosts::status() } + async fn route_add_for_interface( + self, + _: tarpc::context::Context, + subnet: String, + iface: String, + expected_ifindex: u16, + ) -> Result<(), HelperError> { + let subnet: Subnet = subnet.parse().map_err(HelperError::validation)?; + let iface: BridgeIface = iface.parse().map_err(HelperError::validation)?; + if expected_ifindex == 0 { + return Err(HelperError::validation( + "expected interface index must be non-zero", + )); + } + mutations::route::add_for_interface(&subnet, &iface, expected_ifindex) + } + async fn socket_link( self, _: tarpc::context::Context, diff --git a/app/arcbox-helper/src/server/mutations/route.rs b/app/arcbox-helper/src/server/mutations/route.rs index 9969bb448..d72de8403 100644 --- a/app/arcbox-helper/src/server/mutations/route.rs +++ b/app/arcbox-helper/src/server/mutations/route.rs @@ -15,6 +15,32 @@ pub fn add(subnet: &Subnet, iface: &BridgeIface) -> Result<(), HelperError> { } } +/// Adds a route through the exact bridge identity selected by the daemon. +pub fn add_for_interface( + subnet: &Subnet, + iface: &BridgeIface, + expected_ifindex: u16, +) -> Result<(), HelperError> { + let actual_ifindex = arcbox_route::interface_index(iface.as_str()).ok(); + if actual_ifindex != Some(expected_ifindex) { + return Err(HelperError::RouteInterfaceChanged { + iface: iface.to_string(), + expected_ifindex, + actual_ifindex, + }); + } + + let net = to_ipv4net(subnet)?; + match arcbox_route::add_by_index(net, iface.as_str(), expected_ifindex) + .map_err(HelperError::other)? + { + arcbox_route::AddOutcome::Added => Ok(()), + arcbox_route::AddOutcome::Conflict => Err(HelperError::RouteConflict { + subnet: subnet.to_string(), + }), + } +} + /// Removes the route for `subnet`. pub fn remove(subnet: &Subnet) -> Result<(), HelperError> { let net = to_ipv4net(subnet)?; @@ -26,3 +52,25 @@ fn to_ipv4net(subnet: &Subnet) -> Result { Ipv4Net::new(inner.ip(), inner.prefix()) .map_err(|e| HelperError::other(format!("invalid subnet: {e}"))) } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn changed_bridge_identity_is_rejected_before_mutation() { + let subnet = "172.16.0.0/12".parse().unwrap(); + let iface = "bridge4294967295".parse().unwrap(); + + let error = add_for_interface(&subnet, &iface, 1).unwrap_err(); + + assert!(matches!( + error, + HelperError::RouteInterfaceChanged { + expected_ifindex: 1, + actual_ifindex: None, + .. + } + )); + } +} diff --git a/app/arcbox-helper/tests/common/mod.rs b/app/arcbox-helper/tests/common/mod.rs index cc961ddf4..aea6cb0ba 100644 --- a/app/arcbox-helper/tests/common/mod.rs +++ b/app/arcbox-helper/tests/common/mod.rs @@ -79,6 +79,23 @@ impl HelperService for MockHelperServer { Ok(false) } + async fn route_add_for_interface( + self, + _: tarpc::context::Context, + subnet: String, + iface: String, + expected_ifindex: u16, + ) -> Result<(), HelperError> { + validate::validate_subnet(&subnet).map_err(HelperError::validation)?; + validate::validate_iface(&iface).map_err(HelperError::validation)?; + if expected_ifindex == 0 { + return Err(HelperError::validation( + "expected interface index must be non-zero", + )); + } + Ok(()) + } + async fn socket_link( self, _: tarpc::context::Context, diff --git a/app/arcbox-helper/tests/connection_test.rs b/app/arcbox-helper/tests/connection_test.rs index 0e768be20..4dfd005d8 100644 --- a/app/arcbox-helper/tests/connection_test.rs +++ b/app/arcbox-helper/tests/connection_test.rs @@ -81,7 +81,7 @@ fn helper_version_pins_are_aligned() { ); // Independent of the workspace package version. - assert_eq!(pkg, "1.0.2"); + assert_eq!(pkg, "1.0.3"); } #[tokio::test] diff --git a/app/arcbox-helper/tests/route_test.rs b/app/arcbox-helper/tests/route_test.rs index d58924209..283944418 100644 --- a/app/arcbox-helper/tests/route_test.rs +++ b/app/arcbox-helper/tests/route_test.rs @@ -11,6 +11,18 @@ async fn route_add_valid_input() { client.route_add("10.0.0.0/8", "bridge100").await.unwrap(); } +#[tokio::test] +async fn route_add_for_interface_rejects_zero_index() { + let (client, _dir) = common::setup().await; + let err = client + .route_add_for_interface("10.0.0.0/8", "bridge100", 0) + .await; + assert!(matches!( + err, + Err(ClientError::Helper(HelperError::Validation(_))) + )); +} + #[tokio::test] async fn route_add_rejects_public_subnet() { let (client, _dir) = common::setup().await; diff --git a/common/arcbox-constants/src/helper.rs b/common/arcbox-constants/src/helper.rs index 2c5fa2d0e..adf749124 100644 --- a/common/arcbox-constants/src/helper.rs +++ b/common/arcbox-constants/src/helper.rs @@ -2,7 +2,7 @@ //! string format) the Desktop app. //! //! The helper crate (`arcbox-helper`) owns an **independent** Cargo version -//! (currently `1.0.2`), not `workspace.package.version`. Compare that version +//! (currently `1.0.3`), not `workspace.package.version`. Compare that version //! — never the daemon/workspace crate version — when deciding whether to //! reinstall the root binary. //! @@ -19,7 +19,7 @@ /// /// Must stay in sync with `app/arcbox-helper/Cargo.toml` `version` (and the /// workspace path-dep pin) whenever the floor moves. -pub const MIN_HELPER_VERSION: &str = "1.0.2"; +pub const MIN_HELPER_VERSION: &str = "1.0.3"; /// Strips the optional `arcbox-helper ` prefix and whitespace from a version line. #[must_use] diff --git a/common/arcbox-route/src/lib.rs b/common/arcbox-route/src/lib.rs index d9dba7f3f..f2aaea06d 100644 --- a/common/arcbox-route/src/lib.rs +++ b/common/arcbox-route/src/lib.rs @@ -254,10 +254,23 @@ impl AsRawFd for RouteWatcher { /// /// Returns an error string if the kernel rejects the route operation. pub fn add(net: Ipv4Net, iface: &str) -> Result { + let ifindex = interface_index(iface) + .map_err(|e| format!("RTM_ADD {net} via {iface}: failed to resolve interface: {e}"))?; + add_by_index(net, iface, ifindex) +} + +/// Adds a subnet route using a caller-verified kernel interface identity. +/// +/// `iface` is retained in the link-layer gateway for diagnostics; `ifindex` +/// selects the interface. +/// +/// # Errors +/// +/// Returns an error string if the kernel rejects the route operation. +pub fn add_by_index(net: Ipv4Net, iface: &str, ifindex: u16) -> Result { let dst = sockaddr::make_dst(net); let mask = sockaddr::make_netmask(net); - let gw = sockaddr::make_gateway_dl(iface) - .map_err(|e| format!("RTM_ADD {net} via {iface}: failed to resolve interface: {e}"))?; + let gw = sockaddr::make_gateway_dl(iface, ifindex); let add_msg = msg::build_msg(msg::MsgType::Add, &dst, Some(&gw), &mask) .map_err(|e| format!("RTM_ADD {net} via {iface}: failed to build message: {e}"))?; diff --git a/common/arcbox-route/src/msg.rs b/common/arcbox-route/src/msg.rs index 6db29d26a..3644b489b 100644 --- a/common/arcbox-route/src/msg.rs +++ b/common/arcbox-route/src/msg.rs @@ -379,7 +379,7 @@ mod tests { fn build_msg_add_with_gateway() { let net: Ipv4Net = "172.16.0.0/12".parse().unwrap(); let dst = sockaddr::make_dst(net); - let gw = sockaddr::make_gateway_dl_with_index("bridge100", 42); + let gw = sockaddr::make_gateway_dl("bridge100", 42); let mask = sockaddr::make_netmask(net); let buf = build_msg(MsgType::Add, &dst, Some(&gw), &mask).unwrap(); @@ -419,7 +419,7 @@ mod tests { fn build_msg_sockaddr_order() { let net: Ipv4Net = "192.168.0.0/16".parse().unwrap(); let dst = sockaddr::make_dst(net); - let gw = sockaddr::make_gateway_dl_with_index("bridge100", 5); + let gw = sockaddr::make_gateway_dl("bridge100", 5); let mask = sockaddr::make_netmask(net); let buf = build_msg(MsgType::Add, &dst, Some(&gw), &mask).unwrap(); diff --git a/common/arcbox-route/src/sockaddr.rs b/common/arcbox-route/src/sockaddr.rs index fed3c6122..a97b40568 100644 --- a/common/arcbox-route/src/sockaddr.rs +++ b/common/arcbox-route/src/sockaddr.rs @@ -3,7 +3,6 @@ //! Constructs `sockaddr_in` (IPv4 destination/netmask) and `sockaddr_dl` //! (link-layer interface gateway) for use in PF_ROUTE messages. -use std::io; use std::net::Ipv4Addr; use crate::Ipv4Net; @@ -27,38 +26,12 @@ fn make_sin(addr: Ipv4Addr) -> libc::sockaddr_in { sin } -/// Constructs a `sockaddr_dl` for an interface-based route. -/// -/// Resolves the interface name (e.g. `"bridge100"`) to its kernel index -/// via `if_nametoindex`. The interface name is also stored in `sdl_data` -/// for kernel diagnostics. -pub fn make_gateway_dl(iface: &str) -> io::Result { - let c_name = std::ffi::CString::new(iface) - .map_err(|e| io::Error::new(io::ErrorKind::InvalidInput, e))?; - - // Safety: if_nametoindex is safe with a valid C string. - let idx = unsafe { libc::if_nametoindex(c_name.as_ptr()) }; - if idx == 0 { - return Err(io::Error::last_os_error()); - } - - Ok(build_sdl(iface, idx)) -} - /// Constructs a `sockaddr_dl` with an explicit interface index. -/// -/// Used in tests where the interface may not exist on the host. -#[cfg(test)] -pub fn make_gateway_dl_with_index(iface: &str, index: u32) -> libc::sockaddr_dl { - build_sdl(iface, index) -} - -/// Shared `sockaddr_dl` builder — single source of truth. -fn build_sdl(iface: &str, index: u32) -> libc::sockaddr_dl { +pub fn make_gateway_dl(iface: &str, index: u16) -> libc::sockaddr_dl { let mut sdl: libc::sockaddr_dl = unsafe { std::mem::zeroed() }; sdl.sdl_len = std::mem::size_of::() as u8; sdl.sdl_family = libc::AF_LINK as u8; - sdl.sdl_index = index as u16; + sdl.sdl_index = index; let name_bytes = iface.as_bytes(); let copy_len = name_bytes.len().min(sdl.sdl_data.len()); @@ -147,7 +120,7 @@ mod tests { #[test] fn gateway_dl_with_index_sets_fields() { - let sdl = make_gateway_dl_with_index("bridge100", 42); + let sdl = make_gateway_dl("bridge100", 42); assert_eq!(sdl.sdl_family, libc::AF_LINK as u8); assert_eq!(sdl.sdl_index, 42); assert_eq!(sdl.sdl_nlen, 9); // "bridge100".len() diff --git a/docs/daemon-lifecycle.md b/docs/daemon-lifecycle.md index 1648f40e9..b06f041c2 100644 --- a/docs/daemon-lifecycle.md +++ b/docs/daemon-lifecycle.md @@ -66,18 +66,18 @@ startup failure. `READY = 6`). Clients must match on the value, never compare ordinals. New phases are appended with the next free number regardless of where they sit in the progression — values are additive-only. -- Phases mark the transitions; the boolean `SetupStatus` fields - (`vm_running`, `route_installed`, `dns_resolver_installed`, …) carry the - state that outlives startup, including work `recovery::run` spawns in - the background and anything `route_status_loop` reconciles afterwards. - A client wanting "is the route up *now*" reads the flag, not a phase. - `vm_running` and `route_installed` both track the VM across restarts — - `services::vm_running_loop` mirrors `VmLifecycleState::is_ready`, and - `route_status_loop` mirrors the route events — so both fall on a - lifecycle-managed stop and rise again on the next boot rather than - reporting one cold-start observation forever. Neither is a liveness probe: - a guest that dies without the lifecycle noticing leaves `vm_running` true, - because crash detection is unimplemented (ABX-414). +- Phases mark transitions; `SetupStatus` fields carry state that outlives + startup. `services::vm_running_loop` exclusively mirrors + `VmLifecycleState::is_ready`. The container route controller exclusively + publishes `route_installed` from the reconciled kernel route. It caches the + bridge name and interface index for one System VM generation, so FDB expiry + cannot change route identity; the privileged helper validates that identity + again immediately before mutation. Both states fall on a lifecycle-managed + stop and rise on the next boot rather than preserving a cold-start + observation forever. +- `vm_running` is not a liveness probe: a guest that dies without lifecycle + detection leaves it true because crash detection is unimplemented + (ABX-414). ### Why gRPC starts before resource cleanup @@ -196,7 +196,7 @@ When the daemon is killed without graceful shutdown: | `arcbox.sock` | **stale** | `start_grpc` removes before bind | | disk images | **possibly held by XPC helpers** | `wait_for_resources` waits up to 10 s | | VM | non-graceful termination | Virtualization.framework cleans up | -| Route | **stale** | `recovery::run()` rebuilds | +| Route | **stale** | container route controller reconciles | All residual state is handled automatically on next startup. No manual intervention needed. diff --git a/docs/helper.md b/docs/helper.md index a833e3d41..d1fa048e6 100644 --- a/docs/helper.md +++ b/docs/helper.md @@ -7,19 +7,19 @@ perform: `/usr/local/bin` CLI symlinks, `/var/run/docker.sock`, `/etc/resolver`, ## Independent version `arcbox-helper` owns its **own** Cargo package version -(`app/arcbox-helper/Cargo.toml`), currently `1.0.2`. It is **not** tied to +(`app/arcbox-helper/Cargo.toml`), currently `1.0.3`. It is **not** tied to `workspace.package.version`. | Pin | Location | |-----|----------| -| Helper package version | `app/arcbox-helper/Cargo.toml` → `version = "1.0.2"` | -| Workspace path-dep | root `Cargo.toml` → `arcbox-helper = { version = "1.0.2", path = ... }` (**no** `x-release-please-version`) | +| Helper package version | `app/arcbox-helper/Cargo.toml` → `version = "1.0.3"` | +| Workspace path-dep | root `Cargo.toml` → `arcbox-helper = { version = "1.0.3", path = ... }` (**no** `x-release-please-version`) | | Daemon/CLI floor | `arcbox_constants::helper::MIN_HELPER_VERSION` | `arcbox-helper --version` and the tarpc `version` RPC both print: ```text -arcbox-helper 1.0.2 +arcbox-helper 1.0.3 ``` Desktop and daemon parse that line with @@ -82,6 +82,7 @@ Rejected peers are dropped before any tarpc dispatch; logs include | `socket_link` / `socket_unlink` | Target must parse as `SocketTarget` (`~/.arcbox` / `~/.arcbox-dev`); replace only ArcBox-owned symlink | Only ArcBox-owned symlink; never real sockets | | `dns_install` / `dns_uninstall` | Writes marker `# managed by arcbox-helper`; refuses to overwrite foreign resolvers | Only files carrying the marker | | `hosts_alias_*` | Fixed `127.0.0.1 ArcBox # managed by arcbox-helper` line only | Lines carrying the marker only | +| `route_add_for_interface` | Bridge name must still resolve to the daemon-provided kernel interface index | N/A | `is_arcbox_owned` (shared with `CliTarget`) rejects relative paths, `..`, and anything outside `/Applications/` or `/Users/` without a