feat(minimald): networking clean up and DNS plumbing - #717

crates/minimald/src/net/switch.rs (1)

626-653: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

UdpConntrack sweep can degrade to O(n) per packet under sustained high flow counts.

record_egress only sweeps when flows.len() > UDP_FLOW_SWEEP_AT; if live (unexpired) flows persist above that threshold, retain re-scans the entire map on every subsequent egress datagram rather than amortizing, since none of the still-fresh entries get removed.

Consider rate-limiting the sweep itself (e.g., only sweep once per N inserts or once per fixed interval) so a session with many concurrent UDP flows doesn't pay a full scan per packet indefinitely.

♻️ Sketch: amortize the sweep with a counter/interval instead of a length check alone

 struct UdpConntrack {
     flows: Mutex<HashMap<(Ipv4Addr, u16, u16), Instant>>,
+    last_sweep: Mutex<Instant>,
 }

Gate the retain call on now.duration_since(*last_sweep) > SOME_MIN_INTERVAL in addition to (or instead of) the length check, so it can't fire on every insert once the table is persistently large.

🤖 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/minimald/src/net/switch.rs` around lines 626 - 653, Rate-limit the
cleanup in UdpConntrack::record_egress so a persistently large flow table does
not trigger flows.retain on every insert. Track the last sweep time or an
equivalent insert interval in UdpConntrack, and only sweep when the table
exceeds UDP_FLOW_SWEEP_AT and the configured minimum interval has elapsed;
preserve existing TTL-based removal and flow recording behavior.

crates/minimald/src/net/policy.rs (1)

340-355: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Body-size cap overflow is silently truncated, unlike the header-size cap.

Hitting MAX_CONTROL_RESPONSE while reading headers returns an explicit Err, but hitting it while reading the body just breaks the loop and returns whatever was buffered so far — with no signal that the response is incomplete. A non-2xx status in that case would report a truncated error body without indicating truncation, and a 2xx would be treated as fully successful even though the body was cut short.

🛡️ Suggested fix: surface the truncation as an error

     while remaining > 0 {
         if response.len() as u64 >= MAX_CONTROL_RESPONSE {
-            break;
+            return Err(io::Error::other(
+                "gvproxy control response exceeded the size cap before its body completed",
+            ));
         }
         let n = stream.read(&mut buf).await?;
         if n == 0 {
             break;
         }
         response.extend_from_slice(&buf[..n]);
         remaining = remaining.saturating_sub(n);
     }

🤖 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/minimald/src/net/policy.rs` around lines 340 - 355, Update the
body-reading loop in the response buffering flow to return an explicit error
when MAX_CONTROL_RESPONSE is reached before the Content-Length body is fully
buffered, instead of breaking and returning truncated data. Preserve normal EOF
handling and successful completion when the declared body length has been read.

crates/minimald/src/net/gvproxy_network.rs (2)

256-271: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Best-effort DNS registration still blocks the attach path synchronously.

Even though DNS registration failures are swallowed (tracing::warn! only), the .await on register_dns_name still runs inline before OwnIpGuard is returned, so a sluggish/unresponsive gvproxy control channel adds real latency (bounded by the 5s GVPROXY_CONTROL_TIMEOUT) to every own-IP session launch, purely for a "best-effort" side effect. Consider spawning this off as a detached background task so a hiccup here can't delay attach completion at all.

♻️ Suggested non-blocking registration

-    if let Err(e) = crate::net::policy::register_dns_name(
-        &control,
-        crate::net::dns::DEFAULT_HOST_ID,
-        session_name,
-        lease_ip,
-    )
-    .await
-    {
-        tracing::warn!(error = %e, session = session_name, "registering *.min.internal name on gvproxy");
-    }
+    let control_for_dns = control.clone(); // requires ControlChannel: Clone
+    let session_name_owned = session_name.to_string();
+    tokio::spawn(async move {
+        if let Err(e) = crate::net::policy::register_dns_name(
+            &control_for_dns,
+            crate::net::dns::DEFAULT_HOST_ID,
+            &session_name_owned,
+            lease_ip,
+        )
+        .await
+        {
+            tracing::warn!(error = %e, session = %session_name_owned, "registering *.min.internal name on gvproxy");
+        }
+    });

Please verify ControlChannel derives/implements Clone (or restructure to avoid needing it) before applying.

🤖 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/minimald/src/net/gvproxy_network.rs` around lines 256 - 271, Make the
DNS registration in the own-IP attach flow non-blocking by spawning it as a
detached background task instead of awaiting register_dns_name inline before
returning OwnIpGuard. Verify ControlChannel supports Clone and clone or
otherwise safely transfer the control handle into the task; retain the existing
best-effort error warning within the task.

---

256-271: 🗄️ Data Integrity & Integration | 🔵 Trivial

Stale DNS records never expire — no invalidation on teardown.

Register this PTask's <name>.<host-id>.min.internal → its current lease so peer sessions can resolve it (finding #3 / UC6). Done for every own-IP PTask, even with no ingress: resolvable names are how peers find each other, and the ingress gate independently governs reachability. Best-effort — a DNS hiccup must not fail an otherwise-working attach. OwnIpGuard::teardown only removes ingress forwards, never the DNS record (there's no remove verb per the dns_add_body doc comment), so a name keeps resolving to a now-freed lease until/unless something re-registers the same name. If that lease is recycled to a different session before the old name is ever re-registered, the stale record briefly points a peer at the wrong tenant (mitigated, but not eliminated, by the target's own ingress gate). Worth confirming this residual staleness window is an accepted tradeoff, and whether gvproxy's DNS table growth (records accumulate indefinitely across the daemon's lifetime with no reaping) is bounded in practice.

🤖 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/minimald/src/net/gvproxy_network.rs` around lines 256 - 271, Ensure
the DNS record registered by register_dns_name is invalidated when the
corresponding OwnIpGuard is torn down, rather than leaving stale lease mappings
behind. Add or use the appropriate gvproxy DNS removal/expiry mechanism
alongside OwnIpGuard::teardown, preserving best-effort error handling, and
verify records cannot accumulate indefinitely when sessions end.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes