fix(sysproxy): stop turning a missing network service into a hard failure

src-tauri/src/core/manager/lifecycle.rs

@@ -4,7 +4,7 @@ use crate::core::handle::Handle;
 use crate::core::manager::CLASH_LOGGER;
 use crate::core::proxy_control::{self, SysproxyFailure};
 use crate::core::service::{SERVICE_MANAGER, ServiceStatus};
-use anyhow::{Result, ensure};
+use anyhow::Result;
 use clash_verge_logging::{Type, logging};
 use scopeguard::defer;
 use smartstring::alias::String;
@@ -129,20 +129,24 @@ where
 {
     match (running_mode, proxy_intent) {
         (RunningMode::NotRunning, _) => {}
-        // Do not hand the OS to another owner while the old guard may still write.
-        (RunningMode::Sidecar, ProxyStopIntent::HandOverToService) => ensure!(
-            stop_guard().await,
-            "the system proxy guard did not stop in time; not handing the proxy to the service"
-        ),
-        (RunningMode::Service, _) if is_macos => ensure!(
-            stop_guard().await,
-            "the system proxy guard did not stop in time; not stopping the service-owned core"
-        ),
+        // A guard that will not stop must not block the core from stopping.
+        (RunningMode::Sidecar, ProxyStopIntent::HandOverToService) => warn_if_guard_lingers(stop_guard().await),
+        (RunningMode::Service, _) if is_macos => warn_if_guard_lingers(stop_guard().await),
         (RunningMode::Service | RunningMode::Sidecar, _) => clear_proxy().await?,
     }
     stop_core().await
 }
 
+fn warn_if_guard_lingers(stopped: bool) {
+    if !stopped {
+        logging!(
+            warn,
+            Type::Core,
+            "the system proxy guard did not stop in time; stopping the core anyway"
+        );
+    }
+}
+
 async fn run_core_start_transition<Start, StartFuture, Ready, Apply, ApplyFuture>(
     start_core: Start,
     core_is_ready: Ready,
@@ -951,7 +955,7 @@ mod tests {
     }
 
     #[tokio::test]
-    async fn a_guard_that_would_not_stop_keeps_the_core_where_it_is() {
+    async fn a_guard_that_would_not_stop_does_not_block_the_core_from_stopping() {
         for (mode, intent) in [
             (RunningMode::Sidecar, ProxyStopIntent::HandOverToService),
             (RunningMode::Service, ProxyStopIntent::Clear),
@@ -968,8 +972,8 @@ mod tests {
             )
             .await;
 
-            assert!(result.is_err(), "{mode:?} {intent:?}");
-            assert!(calls.lock().is_empty(), "{mode:?} {intent:?}: {:?}", *calls.lock());
+            assert!(result.is_ok(), "{mode:?} {intent:?}");
+            assert_eq!(&*calls.lock(), &["core_stop"], "{mode:?} {intent:?}");
         }
     }
 

src-tauri/src/core/notification.rs

@@ -59,14 +59,16 @@ impl FailedOperation {
         matches!(self, Self::SystemProxyEnable | Self::SystemProxyDisable) && matches!(other, Self::SystemProxyRestore)
     }
 
-    /// Whether reaching `enabled` resolves this operation's failure.
-    const fn retired_by_system_proxy_success(self, enabled: bool) -> bool {
-        match self {
-            Self::SystemProxyEnable => enabled,
-            Self::SystemProxyDisable => !enabled,
-            Self::SystemProxyRestore => true,
+    /// Whether a successful apply of `asked` resolves this operation's failure.
+    const fn retired_by_success_of(self, asked: Self) -> bool {
+        match (self, asked) {
             // Only replacing or stopping the guard resolves its failure.
-            Self::SystemProxyGuard => false,
+            (Self::SystemProxyGuard, _) | (_, Self::SystemProxyGuard) => false,
+            // The user asked and got an answer, so nothing earlier is still owed to them.
+            (_, Self::SystemProxyEnable | Self::SystemProxyDisable) => true,
+            // An incidental restore says nothing about a request they may not have seen fail.
+            (Self::SystemProxyRestore, Self::SystemProxyRestore) => true,
+            (_, Self::SystemProxyRestore) => false,
         }
     }
 }
@@ -124,10 +126,10 @@ impl FailureTable {
     }
 
     /// Return whether any proxy failure was retired.
-    fn retire_system_proxy(&self, enabled: bool) -> bool {
+    fn retire_system_proxy(&self, asked: FailedOperation) -> bool {
         let mut entries = self.entries.lock();
         let before = entries.len();
-        entries.retain(|_, failure| !failure.operation.retired_by_system_proxy_success(enabled));
+        entries.retain(|_, failure| !failure.operation.retired_by_success_of(asked));
         before != entries.len()
     }
 }
@@ -167,8 +169,8 @@ mod failure_table_tests {
         assert_eq!(entries[0].operation, FailedOperation::SystemProxyEnable);
         assert_eq!(entries[0].detail, "and a restart hit it too");
 
-        assert!(!table.retire_system_proxy(false));
-        assert!(table.retire_system_proxy(true));
+        assert!(!table.retire_system_proxy(FailedOperation::SystemProxyRestore));
+        assert!(table.retire_system_proxy(FailedOperation::SystemProxyEnable));
     }
 
     #[test]
@@ -187,8 +189,7 @@ mod failure_table_tests {
 
         let entries = table.snapshot();
         assert_eq!(entries[0].operation, FailedOperation::SystemProxyDisable);
-        assert!(!table.retire_system_proxy(true));
-        assert!(table.retire_system_proxy(false));
+        assert!(table.retire_system_proxy(FailedOperation::SystemProxyDisable));
     }
 
     #[test]
@@ -251,9 +252,9 @@ pub fn retire_guard_failures() {
     }
 }
 
-/// Retire failures resolved by the resulting proxy state.
-pub fn retire_system_proxy_failures(enabled: bool) {
-    if PENDING_FAILURES.retire_system_proxy(enabled) {
+/// Retire failures resolved by a successful apply of what was asked.
+pub fn retire_system_proxy_failures(asked: FailedOperation) {
+    if PENDING_FAILURES.retire_system_proxy(asked) {
         notify_pending_failures_changed();
     }
 }

src-tauri/src/core/proxy_control.rs

@@ -162,6 +162,24 @@ fn was_refused_locally(error: &anyhow::Error) -> bool {
         })
 }
 
+/// Whether macOS has no network service to write the proxy on.
+///
+/// Offline, mid network switch, or the primary interface is not a configured service. A read
+/// reports "no proxy" and a disable has nothing left to do; an enable still fails, since a proxy
+/// the OS never received must not be recorded as applied.
+#[cfg(target_os = "macos")]
+pub const fn is_missing_network_service(error: &sysproxy::Error) -> bool {
+    matches!(
+        error,
+        sysproxy::Error::NoActiveNetworkService | sysproxy::Error::NetworkInterface
+    )
+}
+
+#[cfg(not(target_os = "macos"))]
+pub const fn is_missing_network_service(_error: &sysproxy::Error) -> bool {
+    false
+}
+
 /// Distinguish a misplaced sidecar from a missing privileged service.
 async fn classify_local_apply_failure(error: anyhow::Error) -> anyhow::Error {
     if SysproxyFailure::from_chain(&error).is_some() || !was_refused_locally(&error) {
@@ -340,6 +358,10 @@ fn service_proxy_config(verge: &IVerge, mixed_port: u16, pac_port: u16) -> Resul
     }
 }
 
+/// Judge the outcome the service reported.
+///
+/// A write it could not roll back arrives as a plain error whose cause is the rollback's, not
+/// the write's, so it stays a failure.
 fn service_apply_result(requested: &MacosProxyConfig, outcome: ProxyApplyOutcome) -> Result<()> {
     match outcome {
         ProxyApplyOutcome::Applied | ProxyApplyOutcome::NotRequested => Ok(()),
@@ -389,16 +411,21 @@ where
     Write: FnOnce() -> WriteFuture,
     WriteFuture: Future<Output = Result<()>>,
 {
-    ensure!(
-        drain_local_guard().await,
-        "the system proxy guard did not stop in time; not writing the system proxy through the service"
-    );
+    if !drain_local_guard().await {
+        // Its `networksetup` calls are separate from the service's one privileged transaction,
+        // so a stale guard write may still land after ours.
+        logging!(
+            warn,
+            Type::Core,
+            "the system proxy guard did not stop in time; writing through the service anyway"
+        );
+    }
     write().await
 }
 
 #[derive(Debug)]
 enum TableEffect<'a> {
-    Retire { enabled: bool },
+    Retire(FailedOperation),
     File(FailedOperation, &'a anyhow::Error),
     Nothing,
 }
@@ -415,19 +442,19 @@ fn clear_table_effect(result: &Result<()>) -> TableEffect<'_> {
 
 fn settle_table<Retire, File>(effect: TableEffect<'_>, retire: Retire, file: File)
 where
-    Retire: FnOnce(bool),
+    Retire: FnOnce(FailedOperation),
     File: FnOnce(FailedOperation, &anyhow::Error),
 {
     match effect {
-        TableEffect::Retire { enabled } => retire(enabled),
+        TableEffect::Retire(asked) => retire(asked),
         TableEffect::File(operation, error) => file(operation, error),
         TableEffect::Nothing => {}
     }
 }
 
-fn table_effect(result: &Result<()>, enabled: bool) -> TableEffect<'_> {
+fn table_effect(result: &Result<()>) -> TableEffect<'_> {
     match result {
-        Ok(()) => TableEffect::Retire { enabled },
+        Ok(()) => TableEffect::Retire(notification::what_was_asked()),
         Err(error) if SysproxyFailure::from_chain(error).is_some() => {
             TableEffect::File(notification::what_was_asked(), error)
         }
@@ -437,9 +464,7 @@ fn table_effect(result: &Result<()>, enabled: bool) -> TableEffect<'_> {
 
 pub async fn apply() -> Result<()> {
     let running_mode = CoreManager::global().get_running_mode();
-    // Use the pre-apply snapshot when retiring matching failures.
     let verge = Config::verge().await.latest_arc();
-    let enabled = verge.enable_system_proxy.unwrap_or_default();
     let result = match proxy_backend_route(cfg!(target_os = "macos"), &running_mode) {
         ProxyBackendRoute::Local => match Sysopt::global().update_sysproxy().await {
             Ok(()) => Ok(()),
@@ -462,7 +487,7 @@ pub async fn apply() -> Result<()> {
     };
     // Settle failures where every proxy-apply path converges.
     settle_table(
-        table_effect(&result, enabled),
+        table_effect(&result),
         notification::retire_system_proxy_failures,
         |operation, error| {
             report_failure(operation, error);
@@ -906,8 +931,8 @@ mod tests {
     }
 
     #[tokio::test]
-    async fn a_local_guard_that_would_not_stop_keeps_root_from_writing() {
-        assert_eq!(record_service_apply(false, true).await, (vec!["drain"], false));
+    async fn a_local_guard_that_would_not_stop_does_not_block_the_service_write() {
+        assert_eq!(record_service_apply(false, true).await, (vec!["drain", "write"], true));
     }
 
     #[test]
@@ -1044,7 +1069,45 @@ mod tests {
         assert_eq!(refusal_classification(None), SysproxyFailure::PrivilegeRequired);
     }
 
-    use super::{SystemProxyStateUnknown, is_reportable_given, rollback_failure};
+    use super::{SystemProxyStateUnknown, is_reportable_given, rollback_failure, service_apply_result};
+
+    #[cfg(target_os = "macos")]
+    #[test]
+    fn an_enable_the_os_never_received_is_never_reported_as_applied() {
+        let requested = MacosProxyConfig::Global {
+            host: "127.0.0.1".to_owned(),
+            port: 7890,
+            bypass: String::new(),
+        };
+
+        let error = service_apply_result(
+            &requested,
+            ProxyApplyOutcome::DirectFallback {
+                message: sysproxy::Error::NoActiveNetworkService.to_string(),
+            },
+        )
+        .err()
+        .unwrap_or_else(|| anyhow::anyhow!("an enable that never reached the OS must fail"));
+
+        assert_eq!(
+            SysproxyFailure::from_chain(&error).map(SysproxyFailure::code),
+            Some("SYSPROXY_DIRECT_FALLBACK")
+        );
+    }
+
+    #[cfg(target_os = "macos")]
+    #[test]
+    fn a_disable_is_satisfied_by_the_os_ending_up_direct() {
+        assert!(
+            service_apply_result(
+                &MacosProxyConfig::Disabled,
+                ProxyApplyOutcome::DirectFallback {
+                    message: sysproxy::Error::NoActiveNetworkService.to_string(),
+                },
+            )
+            .is_ok()
+        );
+    }
 
     fn mappable() -> anyhow::Error {
         anyhow::anyhow!("networksetup refused").context(SysproxyFailure::PrivilegeRequired)
@@ -1054,7 +1117,7 @@ mod tests {
         let events = Mutex::new(Vec::new());
         super::settle_table(
             effect,
-            |enabled| events.lock().push(format!("retire:{enabled}")),
+            |asked| events.lock().push(format!("retire:{asked:?}")),
             |operation, _| events.lock().push(format!("file:{operation:?}")),
         );
         events.into_inner()
@@ -1063,8 +1126,8 @@ mod tests {
     #[test]
     fn each_effect_reaches_the_table_it_names() {
         assert_eq!(
-            record_settlement(super::TableEffect::Retire { enabled: true }),
-            vec!["retire:true"]
+            record_settlement(super::TableEffect::Retire(FailedOperation::SystemProxyEnable)),
+            vec!["retire:SystemProxyEnable"]
         );
         assert_eq!(
             record_settlement(super::TableEffect::File(
@@ -1092,16 +1155,20 @@ mod tests {
         ));
     }
 
-    #[test]
-    fn a_clean_apply_retires_in_the_direction_it_reached() {
+    #[tokio::test]
+    async fn a_clean_apply_retires_in_the_direction_that_was_asked_for() {
         assert!(matches!(
-            super::table_effect(&Ok(()), true),
-            super::TableEffect::Retire { enabled: true }
-        ));
-        assert!(matches!(
-            super::table_effect(&Ok(()), false),
-            super::TableEffect::Retire { enabled: false }
+            super::table_effect(&Ok(())),
+            super::TableEffect::Retire(FailedOperation::SystemProxyRestore)
         ));
+        let asked = super::notification::asking_for(FailedOperation::SystemProxyEnable, async {
+            matches!(
+                super::table_effect(&Ok(())),
+                super::TableEffect::Retire(FailedOperation::SystemProxyEnable)
+            )
+        })
+        .await;
+        assert!(asked);
     }
 
     #[tokio::test]
@@ -1109,7 +1176,7 @@ mod tests {
         let (applied, cleared) = super::notification::asking_for(FailedOperation::SystemProxyEnable, async {
             let (apply_error, clear_error) = (mappable(), mappable());
             (
-                format!("{:?}", super::table_effect(&Err(apply_error), true)),
+                format!("{:?}", super::table_effect(&Err(apply_error))),
                 format!("{:?}", super::clear_table_effect(&Err(clear_error))),
             )
         })
@@ -1129,7 +1196,7 @@ mod tests {
     #[test]
     fn a_failure_with_nothing_to_say_is_not_filed() {
         assert!(matches!(
-            super::table_effect(&Err(anyhow::anyhow!("the core exited immediately")), true),
+            super::table_effect(&Err(anyhow::anyhow!("the core exited immediately"))),
             super::TableEffect::Nothing
         ));
     }

src-tauri/src/core/sysopt.rs

@@ -1,6 +1,6 @@
 use crate::{
     config::{Config, MixedPort},
-    core::proxy_control::SystemProxyStateUnknown,
+    core::proxy_control::{self, SystemProxyStateUnknown},
     singleton,
     utils::server,
 };
@@ -137,10 +137,13 @@ where
         return Ok(());
     }
 
-    // The final disable must happen after the last in-flight guard write.
+    // The final disable should follow the last in-flight guard write, but a guard that will not
+    // stop must not keep the proxy on: disable again either way.
     if !drain_again().await {
-        anyhow::bail!(
-            "the system proxy guard did not finish within {GUARD_DRAIN_TIMEOUT:?}; the OS proxy state is unknown"
+        logging!(
+            warn,
+            Type::Core,
+            "the system proxy guard did not finish within {GUARD_DRAIN_TIMEOUT:?}; disabling anyway"
         );
     }
     disable().await
@@ -171,15 +174,19 @@ where
 }
 
 /// Treats a missing network service as a completed disable: with nothing to write to, the
-/// requested "no proxy" state already holds. Turning a proxy on must still fail.
-fn tolerate_missing_network_service(_enabling: bool, outcome: sysproxy::Result<()>) -> Result<()> {
+/// requested "no proxy" state already holds.
+///
+/// Enabling still fails: the guard is opt-in and nothing else re-applies the proxy when a
+/// service appears, so a skipped enable would claim a proxy the OS never received.
+///
+/// Reports whether the write reached the OS, so recovery does not mistake a skip for evidence.
+fn skip_without_network_service(enabling: bool, outcome: sysproxy::Result<()>) -> Result<bool> {
     match outcome {
-        #[cfg(target_os = "macos")]
-        Err(sysproxy::Error::NoActiveNetworkService) if !_enabling => {
+        Err(error) if !enabling && proxy_control::is_missing_network_service(&error) => {
             logging!(warn, Type::Core, "no active network service, nothing to turn off");
-            Ok(())
+            Ok(false)
         }
-        other => other.map_err(anyhow::Error::from),
+        other => other.map(|()| true).map_err(anyhow::Error::from),
     }
 }
 
@@ -187,8 +194,8 @@ fn tolerate_missing_network_service(_enabling: bool, outcome: sysproxy::Result<(
 async fn disable_all_proxies(sys: Sysproxy, auto: Autoproxy) -> Result<()> {
     tokio::task::spawn_blocking(move || {
         disable_both(
-            || tolerate_missing_network_service(sys.enable, sys.set_system_proxy()),
-            || tolerate_missing_network_service(auto.enable, auto.set_auto_proxy()),
+            || skip_without_network_service(sys.enable, sys.set_system_proxy()).map(|_reached_os| ()),
+            || skip_without_network_service(auto.enable, auto.set_auto_proxy()).map(|_reached_os| ()),
         )
     })
     .await?
@@ -463,16 +470,10 @@ impl Sysopt {
         let guard_was_running = !self.access_guard().read().get_state().is_stopped();
         let idle = self.access_guard().read().shutdown();
         let drained = idle.wait_timeout(GUARD_DRAIN_TIMEOUT).await;
-        if !drained {
-            // A pending guard write makes OS state unknown.
-            self.access_guard().write().set_guard_type(GuardType::None);
-            anyhow::bail!(
-                "the system proxy guard did not finish within {GUARD_DRAIN_TIMEOUT:?}; the OS proxy state is unknown"
-            );
-        }
 
-        // Skip writes only when macOS exactly matches the target.
+        // A slow guard does not stop the write, but it does make the OS read untrustworthy.
         if cfg!(target_os = "macos")
+            && drained
             && current_os_proxy_state(sys.clone(), auto.clone()).await == OsProxyState::AlreadyApplied
         {
             self.access_guard().write().set_guard_type(guard_type);
@@ -489,15 +490,17 @@ impl Sysopt {
             (off_sys, off_auto)
         };
 
-        // Track whether an earlier setter already changed the OS.
+        // Only a step that actually wrote is evidence the OS changed; a skipped one is not.
         let applied = tokio::task::spawn_blocking(move || {
-            for (index, step) in apply_steps.into_iter().enumerate() {
+            let mut earlier_step_reached_os = false;
+            for step in apply_steps {
                 let written = match step {
-                    ProxyApplyStep::Autoproxy => tolerate_missing_network_service(auto.enable, auto.set_auto_proxy()),
-                    ProxyApplyStep::Sysproxy => tolerate_missing_network_service(sys.enable, sys.set_system_proxy()),
+                    ProxyApplyStep::Autoproxy => skip_without_network_service(auto.enable, auto.set_auto_proxy()),
+                    ProxyApplyStep::Sysproxy => skip_without_network_service(sys.enable, sys.set_system_proxy()),
                 };
-                if let Err(error) = written {
-                    return Err((index > 0, error));
+                match written {
+                    Ok(reached_os) => earlier_step_reached_os |= reached_os,
+                    Err(error) => return Err((earlier_step_reached_os, error)),
                 }
             }
             Ok(())
@@ -571,12 +574,50 @@ mod tests {
         AuthoritativeState, BYPASS_SEPARATOR, DEFAULT_BYPASS, OsProxyState, ProxyApplyStep, SystemProxyStateUnknown,
         authoritative_state, authoritative_state_from, classify_os_proxy_state, disable_both,
         disable_until_the_last_write_is_ours, first_failure, format_bypass, proxy_apply_steps,
-        recover_from_failed_write, target_is_already_in_place,
+        recover_from_failed_write, skip_without_network_service, target_is_already_in_place,
     };
     use parking_lot::Mutex;
     use std::collections::VecDeque;
     use sysproxy::{Autoproxy, Sysproxy};
 
+    #[cfg(target_os = "macos")]
+    #[test]
+    fn a_machine_with_no_network_service_has_nothing_to_turn_off() {
+        for missing in [
+            sysproxy::Error::NoActiveNetworkService,
+            sysproxy::Error::NetworkInterface,
+        ] {
+            assert_eq!(skip_without_network_service(false, Err(missing)).ok(), Some(false));
+        }
+        assert_eq!(skip_without_network_service(false, Ok(())).ok(), Some(true));
+        assert!(skip_without_network_service(false, Err(sysproxy::Error::RequiresAdminPrivileges)).is_err());
+    }
+
+    /// Enabling the global proxy disables PAC first; that skipped step must not look like a write.
+    #[cfg(target_os = "macos")]
+    #[test]
+    fn a_skipped_step_is_not_evidence_that_the_os_was_touched() {
+        let skipped = skip_without_network_service(false, Err(sysproxy::Error::NoActiveNetworkService));
+        let earlier_step_reached_os = skipped.unwrap_or(true);
+        assert!(!earlier_step_reached_os);
+
+        let failed = skip_without_network_service(true, Err(sysproxy::Error::NoActiveNetworkService))
+            .err()
+            .unwrap_or_else(|| anyhow::anyhow!("enabling without a network service must fail"));
+
+        assert_eq!(
+            authoritative_state(&failed, earlier_step_reached_os),
+            AuthoritativeState::Unchanged
+        );
+    }
+
+    #[cfg(target_os = "macos")]
+    #[test]
+    fn turning_the_proxy_on_without_a_network_service_still_fails() {
+        assert!(skip_without_network_service(true, Err(sysproxy::Error::NoActiveNetworkService)).is_err());
+        assert!(skip_without_network_service(true, Err(sysproxy::Error::NetworkInterface)).is_err());
+    }
+
     #[test]
     fn empty_custom_bypass_uses_defaults() {
         assert_eq!(format_bypass(false, ""), DEFAULT_BYPASS);
@@ -796,10 +837,10 @@ mod tests {
     }
 
     #[tokio::test]
-    async fn a_guard_that_never_finishes_is_reported_rather_than_written_over() {
+    async fn a_guard_that_never_finishes_still_gets_the_proxy_disabled() {
         assert_eq!(
-            record_disable_sequence(false, false, &[true]).await,
-            (vec!["disable", "drain"], false)
+            record_disable_sequence(false, false, &[true, true]).await,
+            (vec!["disable", "drain", "disable"], true)
         );
     }