fix(net): make container route state single-owned

[AprilNEA]

Summary

• make one daemon controller the sole owner of container route reconciliation and SetupState.route_installed
• cache the bridge identity as {name, ifindex} for one System VM generation, independent of FDB expiry
• revalidate that identity at the privileged helper mutation boundary and route by ifindex
• remove the competing boot, recovery, lifecycle-event, and periodic state writers
• bump the required ArcBoxHelper version to 1.0.3 and update lifecycle/helper documentation

Root cause

The old periodic guard treated bridge FDB discovery as durable state. Once the FDB entry aged out, it could publish route_installed = false even though the bridge and kernel route were healthy. At the same time, several lifecycle paths could mutate or publish route state, so stale results could win after a VM or interface replacement.

The new controller owns discovery, reconciliation, retries, and publication as one lifecycle-scoped state machine. Results are discarded when the System VM generation changes, and the helper verifies name → ifindex immediately before mutation.

User impact

Container routing no longer becomes falsely unhealthy after FDB expiry, and stale VM/interface results cannot mutate or report the replacement route. Desktop identity presentation is handled by the companion draft PR arcboxlabs/arcbox-desktop#365.

Linear: ABX-510

Validation

• devenv shell -- cargo fmt --all -- --check
• devenv shell -- cargo check -p arcbox-daemon -p arcbox-core -p arcbox-api -p arcbox-helper -p arcbox-route
• devenv shell -- cargo test -p arcbox-daemon -p arcbox-core -p arcbox-api -p arcbox-helper -p arcbox-route
• devenv shell -- cargo clippy -p arcbox-daemon -p arcbox-core -p arcbox-api -p arcbox-helper -p arcbox-route --all-targets -- -D warnings
• devenv shell -- cargo check -p arcbox-daemon --no-default-features

[greptile-apps]

Greptile Summary

The PR consolidates container-route reconciliation under a lifecycle-aware daemon controller.

• Caches bridge name and interface index for each System VM generation and rejects stale reconciliation results.
• Revalidates bridge identity at the privileged helper boundary and performs route mutations by interface index.
• Removes competing lifecycle, recovery, event, and polling writers for SetupState.route_installed.
• Adds route-socket event parsing, reconciliation tests, and helper protocol/version documentation updates.

Confidence Score: 5/5

The PR appears safe to merge, with no concrete changed-code failure identified.

The new controller consistently scopes cached bridge identity and reconciliation results to the current ready VM generation, while the helper rechecks interface identity before mutation and the removed paths no longer compete to publish route state.

Important Files Changed

Filename	Overview
app/arcbox-daemon/src/container_route.rs	Introduces the single route-state owner with generation checks, bridge caching, event-driven reconciliation, polling fallback, and retry handling.
app/arcbox-core/src/route_reconciler.rs	Refactors reconciliation around a bridge name-and-ifindex identity and delegates retry cadence to the daemon controller.
app/arcbox-core/src/vm.rs	Caches and invalidates bridge identity with the lifetime of each VMM incarnation.
app/arcbox-helper/src/server/mutations/route.rs	Revalidates the selected interface identity immediately before privileged route mutation.
common/arcbox-route/src/lib.rs	Adds explicit interface-index route insertion while retaining the interface name for link-layer diagnostics.
common/arcbox-route/src/msg.rs	Adds route-event decoding and preserves bounded validation of routing-socket messages.
app/arcbox-daemon/src/services.rs	Replaces the previous route guard and status mirror with the sole route controller.
app/arcbox-daemon/src/shutdown.rs	Integrates the new controller handle into graceful draining and timeout abort handling.

Sequence Diagram

sequenceDiagram
    participant VM as System VM lifecycle
    participant C as Route controller
    participant Core as Route reconciler
    participant H as Privileged helper
    participant K as macOS kernel
    VM->>C: lifecycle state and generation
    C->>C: clear cached bridge on stop or generation change
    C->>Core: discover bridge name and ifindex
    C->>Core: inspect current route state
    Core->>H: add route(name, expected ifindex)
    H->>K: resolve current name to ifindex
    alt identity matches
        H->>K: mutate route by ifindex
        H-->>Core: success
        Core-->>C: reconciled route mode
        C->>C: "publish route_installed=true"
    else identity changed
        H-->>Core: RouteInterfaceChanged
        Core-->>C: BridgeNotReady
        C->>C: clear bridge and retry discovery
    end

Loading

_{Reviews (1): Last reviewed commit: "fix(net): make container route state sin..." | Re-trigger Greptile}

[pullfrog]

Important

The controller itself is solid, but the privileged-boundary hardening is only half-migrated: the unvalidated route_add RPC is left registered on the root helper with zero production callers.

Reviewed changes

• New single-owner route controller — app/arcbox-daemon/src/container_route.rs adds a tokio::select! loop over shutdown, a 30s ticker, the VM lifecycle watch, and a PF_ROUTE event stream, owning ControllerState { generation, bridge, mode } as the sole writer of SetupStatus.route_installed.

• Removal of the previous writers — spawn_route_reconciler and both boot.rs call sites, route_status_loop/container_route_guard in services.rs, the cold-start reconcile block in recovery.rs, and Event::ContainerRouteInstalled are all deleted with no dangling references left behind.

• ifindex pinning across the helper boundary — a new route_add_for_interface(subnet, iface, expected_ifindex) tarpc method re-resolves the interface name inside the root helper and rejects with HelperError::RouteInterfaceChanged when the index moved.

• u32 → u16 ifindex narrowing — BridgeTarget.ifindex narrows to match sockaddr_dl::sdl_index, and make_gateway_dl + the test-only make_gateway_dl_with_index + build_sdl collapse into one infallible constructor.

• Helper version floor bump — helper package 1.0.2 → 1.0.3 with a matching MIN_HELPER_VERSION, so a stale helper is rejected by Client::ensure_compatible before any RPC dispatch rather than misdecoding the new method.

I traced the state machine specifically for wedge conditions and it holds up. A VM stop+start pair landing entirely between two loop iterations is caught, because system_vm_restart_generation bumps on stop and observe_vm compares with != rather than >. Every continue path calls ticker.reset_after(...), so no iteration falls through unarmed. External route deletion recovers correctly: the watcher fires, plan_reconciliation sees preferred == Missing under mode Preferred and returns AddPreferred. And no stale route_installed = true can survive a generation change.

Two things I checked and decided not to raise, recorded so they don't get re-litigated. The readiness gate is genuinely later than the old post-machine_manager.start() trigger, but containers require dockerd readiness (strictly later still), so nothing can use the route in the widened window — and the daemon-restart-with-VM-running case is still covered, since the controller subscribes at spawn and sees Running/Idle on its first iteration. Separately, bridge_discovery::if_nametoindex silently returns None above u16::MAX where arcbox_route::interface_index returns an error string; the inconsistency is real but >65535 interfaces is unreachable on macOS.
<comments">[{"path":"app/arcbox-helper/src/lib.rs","line":111,"body":"After this PR, route_add — the name-only variant with no ifindex revalidation — has zero production callers. route_reconciler.rs:205 uses route_add_for_interface exclusively; the only remaining route_add callers are tests/route_test.rs, tests/e2e_fs_test.rs, and the mock in tests/common/mod.rs.\n\nIt is still registered and reachable on the root helper, so the TOCTOU this PR closes on one RPC stays open on its sibling. That leaves the migration half-finished: an unvalidated privileged route mutation stays live indefinitely with no stated deletion plan.\n\nWorth noting the constraint you're already working under — it can't simply be deleted. Per the trait's own doc at lines 51-54, bincode encodes the request enum's variant index, so removing ordinal 0 shifts every later method and an old daemon's route_add would silently decode as route_remove. The ordinal-preserving fix is to keep the slot and make the handler reject.\n\n

Technical details

\n\nIn app/arcbox-helper/src/server/handler.rs, stub the existing route_add arm rather than removing it:\n\nrust\nasync fn route_add(\n self,\n _: tarpc::context::Context,\n _subnet: String,\n _iface: String,\n) -> Result<(), HelperError> {\n Err(HelperError::other(\n \"route_add is superseded by route_add_for_interface\",\n ))\n}\n\n\nThat keeps the variant index stable for the upgrade window while closing the bypass. The four test call sites then need updating to assert the rejection instead of success. route_remove needs no equivalent — it takes no interface, so there is no index to pin.\n\n

"},{"path":"app/arcbox-daemon/src/container_route.rs","line":192,"body":"RouteConflict correctly gets the 30s POLL_INTERVAL, but every other error falls to a flat 2s RETRY_INTERVAL with no backoff — including conditions that are permanent rather than transient.\n\nThe concrete case is an incompatible helper. ClientError::IncompatibleVersion maps to RouteError::HelperUnavailable (route_reconciler.rs:47-50, lumped in with Connection and Rpc), so a daemon on this build against a still-installed 1.0.2 helper retries every 2 seconds indefinitely — and it can never succeed without user action. The bridge-never-resolves path (line 155) has the same shape.\n\nEach of those 2s iterations is not free:\n\n- it re-runs resolve_container_bridge, which on the uncached path does a kernel FDB scan while holding the vms write lock (see the comment on vm.rs);\n- it calls setup_state.set_route_installed(false), and SetupState::publish has no dedupe (arcbox-api/src/connect/system.rs:67-73 sends unconditionally), so every WatchSetupStatus subscriber receives an identical SetupStatus every 2 seconds forever.\n\nconsecutive_failures is already tracked right here but only feeds should_log_failure. Reusing it for backoff up to POLL_INTERVAL is a small change and would bound all three costs:\n\nrust\nlet retry_after = if matches!(&error, RouteError::RouteConflict { .. }) {\n POLL_INTERVAL\n} else {\n RETRY_INTERVAL\n .saturating_mul(1 << consecutive_failures.min(4))\n .min(POLL_INTERVAL)\n};\n\n\nSeparately worth a thought: IncompatibleVersion is a permanent, user-actionable condition currently indistinguishable from a transient helper hiccup. Right now the only signal is a warn! on failure 1 and every 30th, and route_installed stuck at false with no explanation on the SetupStatus surface."},{"path":"app/arcbox-core/src/vm.rs","line":1080,"body":"This upgrades the old read lock to a write lock and holds it across two blocking calls — vmnet_interface_info() and resolve_bridge_by_mac(), the latter enumerating kernel bridges and querying each bridge's FDB.\n\nEvery other vms accessor takes a read lock and blocks behind it: connect_vsock, debug_snapshot (which serves GetVirtioDebug), get_balloon_stats, read_console_output, and friends — several on Docker API and gRPC request paths.\n\nOn the happy path the entry.bridge_target cache means the scan runs once per VM incarnation, so this is genuinely minor. It only bites when discovery keeps failing, since the cache never populates and the scan then recurs on every controller retry — which compounds with the flat 2s interval flagged in container_route.rs.\n\nIf you want the lock scope tightened, the shape is to take the MAC under a read lock, drop the guard, run resolve_bridge_by_mac unlocked, then re-acquire a write lock to store the result (re-checking the entry still exists). Fine to leave as-is if you'd rather not carry the re-check — the caller already runs this inside spawn_blocking, so the reactor is never held."}]

  ^{｜ Fix it ➔ ｜ View workflow run ｜ Using Claude Opus ｜ 𝕏}