Merge codex-security-s11-presence-discovery: honest presence/discovery state on apply failure (S11)

src/app/mod.rs

@@ -955,13 +955,15 @@ fn update(state: &mut AppState, message: AppMessage) -> Task<AppMessage> {
                     state.friend_presence.insert(id, presence);
                 }
                 UiEvent::PresenceModeReverted { mode } => {
-                    // The Discoverable time-box elapsed; core dropped us back to
+                    // Core corrected the committed presence mode. Mirror + persist so
-                    // `mode` (Normal) and stopped publishing. Mirror + persist so the
+                    // the picker reflects the discovery state the endpoint actually has.
-                    // presence picker reflects it, and tell the user why it changed.
                     state.config.presence_mode = mode;
                     state.config.save();
-                    state.status_message =
+                    state.status_message = if mode == PresenceMode::Normal {
-                        "Discoverable timed out — back to Normal".to_string();
+                        "Discoverable timed out — back to Normal".to_string()
+                    } else {
+                        format!("Presence mode stayed {mode}")
+                    };
                 }
                 UiEvent::ShutdownComplete => {
                     if state.closing {

src/core/messages.rs

@@ -124,11 +124,10 @@ pub enum UiEvent {
     /// joinable gathering (with a one-click ticket). Emitted by the outbound ping
     /// scheduler; absence of a recent event = treat as offline.
     FriendPresence { id: EndpointId, presence: FriendPresence },
-    /// The Discoverable time-box elapsed (W7 P6): the core auto-reverted our presence
+    /// Core corrected the committed presence posture. Usually the Discoverable
-    /// posture to the carried `mode` (always `Normal`) and stopped publishing. The
+    /// time-box elapsed and the core auto-reverted to `Normal`; on discovery apply
-    /// GUI must mirror + persist this so its presence picker stops showing
+    /// failure, this carries the previous truthful mode. The GUI must mirror +
-    /// Discoverable. Distinct from a user-driven change so the GUI knows to update
+    /// persist this so its presence picker matches the endpoint's discovery state.
-    /// without having issued the command itself.
     PresenceModeReverted { mode: PresenceMode },
     /// Core finished orderly app shutdown and the GUI can exit.
     ShutdownComplete,

src/core/mod.rs

@@ -13,6 +13,7 @@ use crate::network::{
 use crate::core::messages::{CoreCommand, UiEvent};
 
 use crate::config::{NetworkMode, RecordingMode};
+use crate::presence::PresenceMode;
 use crate::audio::multitrack::MultitrackRecorder;
 use iroh::{Endpoint, EndpointAddr, EndpointId, RelayMode, SecretKey, endpoint::presets, protocol::Router};
 use iroh_gossip::net::Gossip;
@@ -69,10 +70,27 @@ const RECONNECT_GRACE: Duration = Duration::from_secs(45);
 /// 48 kHz / 60 ms frame) while bounding malicious datagram copy/decode churn.
 const MAX_OPUS_PAYLOAD: usize = 4000;
 
+/// If the Discoverable time-box tries to revert but discovery service reconfiguration
+/// fails, retry soon while keeping the UI in the still-possible publishing state.
+const DISCOVERY_REVERT_RETRY: Duration = Duration::from_secs(60);
+
 fn audio_datagram_len_ok(len: usize) -> bool {
     (4..=4 + MAX_OPUS_PAYLOAD).contains(&len)
 }
 
+fn arm_discovery_retry(
+    discovery_deadline: &mut Option<tokio::time::Instant>,
+    now: tokio::time::Instant,
+) {
+    let retry_deadline = now + DISCOVERY_REVERT_RETRY;
+    if discovery_deadline
+        .map(|current| current > retry_deadline)
+        .unwrap_or(true)
+    {
+        *discovery_deadline = Some(retry_deadline);
+    }
+}
+
 /// Per-peer reconnect grace timers (see [`RECONNECT_GRACE`]). Shared between the
 /// room-event task (which arms one on a transient drop and cancels it on a
 /// gossip rejoin) and the conn-event task (which cancels it when the audio link
@@ -466,12 +484,11 @@ impl NetStack {
 /// `DnsAddressLookup`, mirroring the `N0` preset) is added when `plan.resolver`; the
 /// n0 DNS *publisher* (`PkarrPublisher`) when `plan.publisher`.
 ///
-/// Idempotent and reversible: it clears the whole service set and reinstalls exactly
+/// Idempotent and reversible: it builds the replacement services first, then clears
-/// what the plan wants, so flipping `publisher` off simply drops the publisher (its
+/// the service set and reinstalls exactly what the plan wants. Flipping `publisher`
-/// republish task ends when the last clone is dropped, and the already-published
+/// off drops the publisher (its republish task ends when the last clone is dropped,
-/// record TTL-expires within ~30s) without an endpoint rebuild and without disturbing
+/// and the already-published record TTL-expires within ~30s) without an endpoint
-/// resolution. The brief clear→re-add window is a few synchronous calls; presence
+/// rebuild and without disturbing resolution.
-/// toggles are rare, so a concurrent dial racing it is not a practical concern.
 fn apply_discovery(
     endpoint: &Endpoint,
     memory_lookup: &iroh::address_lookup::memory::MemoryLookup,
@@ -482,16 +499,34 @@ fn apply_discovery(
         pkarr::{PkarrPublisher, PkarrResolver},
     };
     let services = endpoint.address_lookup()?;
+    let pkarr_resolver = if plan.resolver {
+        Some(PkarrResolver::n0_dns().into_address_lookup(endpoint)?)
+    } else {
+        None
+    };
+    let dns_resolver = if plan.resolver {
+        Some(DnsAddressLookup::n0_dns().into_address_lookup(endpoint)?)
+    } else {
+        None
+    };
+    let publisher = if plan.publisher {
+        Some(PkarrPublisher::n0_dns().into_address_lookup(endpoint)?)
+    } else {
+        None
+    };
+
     services.clear();
     // Always keep the local, server-free lookup (this is what ticket/gossip dialing
     // depends on — it must survive every posture, including DirectOnly).
     services.add(memory_lookup.clone());
-    if plan.resolver {
+    if let Some(pkarr_resolver) = pkarr_resolver {
-        services.add(PkarrResolver::n0_dns().into_address_lookup(endpoint)?);
+        services.add(pkarr_resolver);
-        services.add(DnsAddressLookup::n0_dns().into_address_lookup(endpoint)?);
     }
-    if plan.publisher {
+    if let Some(dns_resolver) = dns_resolver {
-        services.add(PkarrPublisher::n0_dns().into_address_lookup(endpoint)?);
+        services.add(dns_resolver);
+    }
+    if let Some(publisher) = publisher {
+        services.add(publisher);
     }
     Ok(())
 }
@@ -851,22 +886,64 @@ async fn run_core_loop(
             // W7 P6 time-box: Discoverable auto-reverts to Normal after DISCOVERY_TIMEBOX
             // so a publish beacon never stands indefinitely. The branch is disabled
             // (`if` guard) unless a deadline is armed; `unwrap_or_else` is unreachable
-            // belt-and-braces. On fire: stop publishing, drop to Normal, tell the GUI.
+            // belt-and-braces. On fire: stop publishing first, then commit Normal only
+            // if the endpoint's discovery services accepted the non-publishing plan.
             _ = tokio::time::sleep_until(
                 discovery_deadline.unwrap_or_else(tokio::time::Instant::now),
             ), if discovery_deadline.is_some() => {
+                let previous_mode = *presence_mode.lock().unwrap();
+                if previous_mode != PresenceMode::Discoverable {
                     discovery_deadline = None;
-                *presence_mode.lock().unwrap() = crate::presence::PresenceMode::Normal;
+                    continue;
-                let plan = crate::discovery::lookup_plan(network_mode, false);
-                if let Err(e) = apply_discovery(&net.endpoint, &net.memory_lookup, plan) {
-                    crate::log_msg(&format!("discovery: time-box revert failed: {e:#}"));
                 }
-                crate::log_msg("discovery: Discoverable time-box elapsed → reverting to Normal");
+
+                let requested_mode = PresenceMode::Normal;
+                let now = tokio::time::Instant::now();
+                let plan = crate::discovery::lookup_plan(
+                    network_mode,
+                    requested_mode.publishes_to_discovery(),
+                );
+                let apply_result = apply_discovery(&net.endpoint, &net.memory_lookup, plan);
+                let (committed_mode, transition_error) =
+                    crate::discovery::resolve_presence_transition(
+                        previous_mode,
+                        requested_mode,
+                        apply_result.is_ok(),
+                    );
+                *presence_mode.lock().unwrap() = committed_mode;
+                discovery_deadline = if committed_mode == PresenceMode::Discoverable {
+                    Some(now + DISCOVERY_REVERT_RETRY)
+                } else {
+                    None
+                };
+
+                match apply_result {
+                    Ok(()) => {
+                        crate::log_msg(
+                            "discovery: Discoverable time-box elapsed → reverting to Normal",
+                        );
                         let _ = ui_tx
                             .send(UiEvent::PresenceModeReverted {
-                        mode: crate::presence::PresenceMode::Normal,
+                                mode: PresenceMode::Normal,
                             })
                             .await;
+                    }
+                    Err(e) => {
+                        crate::log_msg(&format!("discovery: time-box revert failed: {e:#}"));
+                        if committed_mode != requested_mode {
+                            let _ = ui_tx
+                                .send(UiEvent::PresenceModeReverted {
+                                    mode: committed_mode,
+                                })
+                                .await;
+                        }
+                        if let Some(message) = transition_error {
+                            let _ = ui_tx
+                                .send(UiEvent::Error(format!("{message} ({e:#})")))
+                                .await;
+                        }
+                    }
+                }
                 continue;
             }
         };
@@ -1798,22 +1875,60 @@ async fn run_core_loop(
             }
 
             CoreCommand::SetPresenceMode(mode) => {
-                *presence_mode.lock().unwrap() = mode;
+                let previous_mode = *presence_mode.lock().unwrap();
-                // W7 P6: re-apply n0 DNS discovery for the new posture (publish on iff
+                let now = tokio::time::Instant::now();
-                // Discoverable). Runtime — no endpoint rebuild; clears + reinstalls the
+
-                // address-lookup services. The resolver stays on regardless so we can
+                if previous_mode == mode {
-                // still look up moved friends.
+                    // Same-mode requests are no-ops for discovery wiring, but keep the
-                let plan = crate::discovery::lookup_plan(network_mode, mode.publishes_to_discovery());
+                    // existing UX: re-selecting Discoverable restarts the clock.
-                if let Err(e) = apply_discovery(&net.endpoint, &net.memory_lookup, plan) {
+                    discovery_deadline = if mode == PresenceMode::Discoverable {
-                    crate::log_msg(&format!("discovery: apply failed: {e:#}"));
+                        Some(now + crate::discovery::DISCOVERY_TIMEBOX)
-                }
-                // Arm (Discoverable) or cancel (any other posture) the auto-revert
-                // time-box. Re-selecting Discoverable restarts the clock.
-                discovery_deadline = if mode == crate::presence::PresenceMode::Discoverable {
-                    Some(tokio::time::Instant::now() + crate::discovery::DISCOVERY_TIMEBOX)
                     } else {
                         None
                     };
+                    continue;
+                }
+
+                // W7 P6/S11: re-apply n0 DNS discovery for the requested posture
+                // first, then commit the presence mode only if the endpoint accepted
+                // that discovery plan. This keeps the UI truthful when dropping the
+                // publisher fails.
+                let plan =
+                    crate::discovery::lookup_plan(network_mode, mode.publishes_to_discovery());
+                let apply_result = apply_discovery(&net.endpoint, &net.memory_lookup, plan);
+                let (committed_mode, transition_error) =
+                    crate::discovery::resolve_presence_transition(
+                        previous_mode,
+                        mode,
+                        apply_result.is_ok(),
+                    );
+                *presence_mode.lock().unwrap() = committed_mode;
+
+                if committed_mode == PresenceMode::Discoverable {
+                    if apply_result.is_ok() && mode == PresenceMode::Discoverable {
+                        discovery_deadline = Some(now + crate::discovery::DISCOVERY_TIMEBOX);
+                    } else {
+                        arm_discovery_retry(&mut discovery_deadline, now);
+                    }
+                } else {
+                    discovery_deadline = None;
+                }
+
+                if let Err(e) = apply_result {
+                    crate::log_msg(&format!("discovery: apply failed: {e:#}"));
+                    if committed_mode != mode {
+                        let _ = ui_tx
+                            .send(UiEvent::PresenceModeReverted {
+                                mode: committed_mode,
+                            })
+                            .await;
+                    }
+                    if let Some(message) = transition_error {
+                        let _ = ui_tx
+                            .send(UiEvent::Error(format!("{message} ({e:#})")))
+                            .await;
+                    }
+                }
             }
 
             CoreCommand::SetRecordingMode(mode) => {

src/discovery.rs

@@ -8,14 +8,19 @@
 //! The model (from `docs/contacts-plan.md` P6, decided 2026-06-16):
 //! - **Resolving is always allowed on relay-capable modes** — a stationary friend
 //!   (typically in `Normal`) must be able to look up a friend who moved networks. A
-//!   resolve is a DNS query to n0 that publishes nothing; it only fires when a saved
+//!   resolve is a DNS query to n0 that publishes nothing, but still exposes query
-//!   address is stale and the dial falls through to discovery.
+//!   timing/source metadata to n0; it only fires when a saved address is stale and
+//!   the dial falls through to discovery.
 //! - **Publishing is gated on `Discoverable`** and asymmetric: only the mover
 //!   publishes their address to n0 DNS; everyone else just looks it up.
+//! - **Stopping publishing removes the local publisher service**; iroh does not
+//!   expose an explicit unpublish call here, so already-published pkarr records can
+//!   linger until their default ~30s TTL expires.
 //! - **`DirectOnly` is the explicit no-server posture** — neither resolve nor publish
 //!   ever touches n0 there, regardless of the Discoverable toggle.
 
 use crate::config::NetworkMode;
+use crate::presence::PresenceMode;
 use std::time::Duration;
 
 /// How long `Discoverable` stays on before auto-reverting to `Normal`. Discovery is
@@ -46,13 +51,44 @@ pub fn lookup_plan(network_mode: NetworkMode, want_publish: bool) -> LookupPlan
     match network_mode {
         // The explicit serverless posture: no n0 contact at all, even to resolve.
         // A Discoverable toggle here is intentionally inert.
-        NetworkMode::DirectOnly => LookupPlan { resolver: false, publisher: false },
+        NetworkMode::DirectOnly => LookupPlan {
+            resolver: false,
+            publisher: false,
+        },
         // Relay-capable: always resolve (so a stationary friend can find a mover);
         // publish only when the user opted into Discoverable.
-        NetworkMode::RelayNoDiscovery | NetworkMode::N0Full => {
+        NetworkMode::RelayNoDiscovery | NetworkMode::N0Full => LookupPlan {
-            LookupPlan { resolver: true, publisher: want_publish }
+            resolver: true,
+            publisher: want_publish,
+        },
     }
 }
+
+/// Decide which presence mode may be committed after attempting to apply discovery
+/// services for `requested`.
+///
+/// On failure, keep the previous mode: it is the only locally truthful state because
+/// the endpoint's discovery services may still reflect the old posture. Same-mode
+/// requests are no-ops from a presence-truth perspective and do not surface an error.
+pub fn resolve_presence_transition(
+    previous: PresenceMode,
+    requested: PresenceMode,
+    apply_ok: bool,
+) -> (PresenceMode, Option<String>) {
+    if previous == requested {
+        return (previous, None);
+    }
+
+    if apply_ok {
+        (requested, None)
+    } else {
+        (
+            previous,
+            Some(format!(
+                "Couldn't update discovery mode; keeping {previous}."
+            )),
+        )
+    }
 }
 
 #[cfg(test)]
@@ -64,12 +100,18 @@ mod tests {
         for mode in [NetworkMode::RelayNoDiscovery, NetworkMode::N0Full] {
             assert_eq!(
                 lookup_plan(mode, false),
-                LookupPlan { resolver: true, publisher: false },
+                LookupPlan {
+                    resolver: true,
+                    publisher: false
+                },
                 "{mode:?}: resolve always on, no publish when not Discoverable"
             );
             assert_eq!(
                 lookup_plan(mode, true),
-                LookupPlan { resolver: true, publisher: true },
+                LookupPlan {
+                    resolver: true,
+                    publisher: true
+                },
                 "{mode:?}: Discoverable adds publish on top of resolve"
             );
         }
@@ -79,12 +121,18 @@ mod tests {
     fn direct_only_never_touches_n0_even_when_discoverable() {
         assert_eq!(
             lookup_plan(NetworkMode::DirectOnly, false),
-            LookupPlan { resolver: false, publisher: false }
+            LookupPlan {
+                resolver: false,
+                publisher: false
+            }
         );
         // The serverless posture overrides the Discoverable request entirely.
         assert_eq!(
             lookup_plan(NetworkMode::DirectOnly, true),
-            LookupPlan { resolver: false, publisher: false }
+            LookupPlan {
+                resolver: false,
+                publisher: false
+            }
         );
     }
 
@@ -92,4 +140,42 @@ mod tests {
     fn timebox_is_thirty_minutes() {
         assert_eq!(DISCOVERY_TIMEBOX, Duration::from_secs(1800));
     }
+
+    #[test]
+    fn presence_transition_commits_requested_mode_after_successful_apply() {
+        assert_eq!(
+            resolve_presence_transition(PresenceMode::Normal, PresenceMode::Discoverable, true),
+            (PresenceMode::Discoverable, None)
+        );
+    }
+
+    #[test]
+    fn presence_transition_keeps_previous_mode_when_apply_fails() {
+        let (mode, err) =
+            resolve_presence_transition(PresenceMode::Normal, PresenceMode::Discoverable, false);
+
+        assert_eq!(mode, PresenceMode::Normal);
+        assert!(err.unwrap().contains("keeping Normal"));
+    }
+
+    #[test]
+    fn presence_transition_keeps_discoverable_when_off_transition_fails() {
+        let (mode, err) =
+            resolve_presence_transition(PresenceMode::Discoverable, PresenceMode::Normal, false);
+
+        assert_eq!(mode, PresenceMode::Discoverable);
+        assert!(err.unwrap().contains("keeping Discoverable"));
+    }
+
+    #[test]
+    fn presence_transition_same_mode_is_noop_without_error() {
+        assert_eq!(
+            resolve_presence_transition(
+                PresenceMode::Discoverable,
+                PresenceMode::Discoverable,
+                false
+            ),
+            (PresenceMode::Discoverable, None)
+        );
+    }
 }