From 906ec5e63af3bc67dcded2d10315d4de9c4c5bb2 Mon Sep 17 00:00:00 2001 From: wonfen Date: Mon, 17 Aug 2026 19:19:54 +0800 Subject: [PATCH] fix: serialize concurrent configuration updates --- src-tauri/src/config/config.rs | 8 ++++- src-tauri/src/config/port.rs | 12 +++---- src-tauri/src/feat/config.rs | 38 +++++++++----------- src-tauri/src/feat/listener.rs | 25 ++++---------- src-tauri/src/feat/tun.rs | 63 +++++++++++----------------------- 5 files changed, 55 insertions(+), 91 deletions(-) diff --git a/src-tauri/src/config/config.rs b/src-tauri/src/config/config.rs index 7116ce588..33269a393 100644 --- a/src-tauri/src/config/config.rs +++ b/src-tauri/src/config/config.rs @@ -23,7 +23,7 @@ use std::{ path::PathBuf, sync::atomic::{AtomicBool, Ordering}, }; -use tokio::sync::OnceCell; +use tokio::sync::{Mutex, MutexGuard, OnceCell}; use tokio::time::sleep; pub struct Config { @@ -34,6 +34,7 @@ pub struct Config { } static TUN_SESSION_SUPPRESSED: AtomicBool = AtomicBool::new(false); +static CONFIG_WRITE_LOCK: Mutex<()> = Mutex::const_new(()); impl Config { pub async fn global() -> &'static Self { @@ -66,6 +67,11 @@ impl Config { Self::global().await.runtime_config.clone() } + /// Serialize transactions that share configuration draft layers. + pub(crate) async fn lock_config_write() -> MutexGuard<'static, ()> { + CONFIG_WRITE_LOCK.lock().await + } + /// 初始化订阅 pub async fn init_config() -> Result<()> { Self::init_config_before_window().await?; diff --git a/src-tauri/src/config/port.rs b/src-tauri/src/config/port.rs index 68d2bd23a..a40464b69 100644 --- a/src-tauri/src/config/port.rs +++ b/src-tauri/src/config/port.rs @@ -47,6 +47,7 @@ impl Config { } async fn resolve_startup_mixed_port_inner() -> Result { + let _config_write = Self::lock_config_write().await; let clash = Self::clash().await.latest_arc(); let verge = Self::verge().await.latest_arc(); let selected_port = clash.get_mixed_port(); @@ -89,7 +90,7 @@ impl Config { let clash = Self::clash().await; let verge = Self::verge().await; let runtime = Self::runtime().await; - // Every failure below leaves the three layers as they were, without saying so. + // Roll back all three drafts if fallback fails. let transaction = DraftTransaction::begin(vec![&clash, &verge, &runtime])?; clash.edit_draft(|draft| { @@ -131,9 +132,7 @@ impl Config { } .await; - // Files are not part of the transaction, so a failed write is restored explicitly. - // The drafts are rolled back first: restoring reads the committed configuration, and - // must not see a candidate that is being abandoned. + // Restore files after rollback so regeneration sees committed values. if let Err(error) = persist_result { transaction.rollback(); return match restore_files(&snapshots).await { @@ -210,7 +209,7 @@ fn report_fallback_error(message: String) { }); } -// Only service-managed cores can survive into this startup phase; this app has not spawned its sidecar yet. +// Only a service-managed core can still be running during startup. async fn owned_service_core_uses_port(port: u16) -> bool { if !matches!(SERVICE_MANAGER.current().await, ServiceStatus::Ready) { return false; @@ -261,8 +260,7 @@ async fn owned_service_core_uses_port(port: u16) -> bool { fn configured_listener_ports(clash: &IClashTemp, verge: &IVerge) -> HashSet { let mut ports = HashSet::new(); - // Every listener except the Mixed Port itself: that is the one being reassigned, so - // colliding with its current value is exactly what we are trying to do. + // Exclude the Mixed Port because it is being reassigned. for key in proxy_listener_keys().filter(|key| *key != MIXED_PORT_KEY) { if let Some(port) = mapping_port(&clash.0, key) { ports.insert(port); diff --git a/src-tauri/src/feat/config.rs b/src-tauri/src/feat/config.rs index b571ea3cd..1719b6a59 100644 --- a/src-tauri/src/feat/config.rs +++ b/src-tauri/src/feat/config.rs @@ -8,6 +8,7 @@ use bitflags::bitflags; use clash_verge_draft::{DraftTransaction, SharedDraft}; use clash_verge_logging::{Type, logging, logging_error}; use serde_yaml_ng::Mapping; +use tokio::sync::MutexGuard; /// Patch Clash configuration pub async fn patch_clash(patch: &Mapping) -> Result<()> { @@ -265,19 +266,9 @@ async fn process_terminated_flags(update_flags: UpdateFlags, patch: &IVerge) -> Ok(()) } -/// Apply a patch to the app's configuration, then re-check anything it can invalidate. +/// Apply a patch, then reconcile TUN when its setting changes. /// -/// Today that is TUN, which is a question about the *setting* rather than about the Run State: -/// no Run State transition follows a TUN patch, so the reconciliation that reacts to those -/// would never see it. Switching TUN on from the global hotkey is the case that reaches here — -/// unlike the tray item and the settings switch it is not gated on availability, and the flags -/// the patch raises are answered by a Core reload that changes no Run State at all. Left alone -/// the setting stays on and every surface reports TUN as enabled while nothing carries its -/// traffic. -/// -/// The reconciliation writes configuration of its own, so it goes through -/// [`apply_verge_patch`] rather than back through here. That makes the absence of a cycle a -/// property of the call graph rather than of a runtime early-return. +/// TUN patches do not always produce a Run State transition, so reconciliation is explicit. pub async fn patch_verge(patch: &IVerge, not_save_file: bool) -> Result<()> { apply_verge_patch(patch, not_save_file).await?; if patch.enable_tun_mode.is_some() { @@ -286,18 +277,21 @@ pub async fn patch_verge(patch: &IVerge, not_save_file: bool) -> Result<()> { Ok(()) } -/// Apply a patch and nothing else. For callers that are themselves a reconciliation. +/// Apply a patch without post-update reconciliation. pub(super) async fn apply_verge_patch(patch: &IVerge, not_save_file: bool) -> Result<()> { + let config_write = Config::lock_config_write().await; + apply_verge_patch_locked(&config_write, patch, not_save_file).await +} + +/// Apply a patch with the shared configuration write lock already held. +/// Callers must pass the guard returned by [`Config::lock_config_write`]. +pub(super) async fn apply_verge_patch_locked( + _config_write: &MutexGuard<'_, ()>, + patch: &IVerge, + not_save_file: bool, +) -> Result<()> { let verge = Config::verge().await; - // Applying the flags can fail, and until now that `?` returned straight out of here past - // an `if let Err(..) { discard() }` the compiler could never reach — leaving the failed - // edit sitting in the draft, where every later reader saw a value that was never applied - // and never written to disk. - // - // Claiming the layer is what makes the rollback safe. This function holds its draft across - // `process_terminated_flags`, which restarts the Core and can take seconds; a second patch - // arriving in that window used to share the one draft slot, and whichever of the two failed - // first discarded the other's staged edit too. + // Hold the claim across side effects so concurrent transactions cannot share this draft. let transaction = DraftTransaction::begin(vec![&verge])?; verge.edit_draft(|d| d.patch_config(patch)); diff --git a/src-tauri/src/feat/listener.rs b/src-tauri/src/feat/listener.rs index 7ebc4150b..65876485a 100644 --- a/src-tauri/src/feat/listener.rs +++ b/src-tauri/src/feat/listener.rs @@ -30,6 +30,7 @@ pub async fn probe_listener(request: ListenerProbe) -> Result Result { settings.validate()?; + let _config_write = Config::lock_config_write().await; let manager = CoreManager::global(); if !manager.try_start_config_update() { bail!("configuration update is already running"); @@ -38,27 +39,19 @@ pub async fn save_proxy_ports(settings: ProxyPortSettings) -> Result Result Result<()> { Ok(()) } -/// Roll the drafts back *now*, rather than on the way out. -/// -/// The transaction discards when it drops, but rollback has to regenerate files from the -/// committed configuration first, so the drafts must be gone before that happens. +/// Discard candidate drafts before restoring files from committed configuration. async fn discard_proxy_port_drafts() { Config::clash().await.discard(); Config::verge().await.discard(); diff --git a/src-tauri/src/feat/tun.rs b/src-tauri/src/feat/tun.rs index 4ec8baf9c..e911c24a4 100644 --- a/src-tauri/src/feat/tun.rs +++ b/src-tauri/src/feat/tun.rs @@ -1,12 +1,7 @@ -//! Turning TUN off when it stops being possible. -//! -//! The decision is [`RunState::tun_should_be_disabled`]; this is the action it triggers. They -//! are separate because the decision is a question about Service capability, which Run State -//! owns, while writing user configuration is not something Run State should do. +//! Disables TUN when the current Run State cannot support it. use std::sync::atomic::{AtomicBool, Ordering}; -use clash_verge_draft::DraftBusy; use clash_verge_logging::{Type, logging}; use scopeguard::defer; @@ -15,48 +10,41 @@ use crate::{ core::{handle::Handle, manager::RunningMode, runstate::RUN_STATE}, }; -/// Set while a disable is being written. +/// Set while an automatic TUN disable is being written. /// -/// Patching the config produces further Run State transitions, each of which reconciles again; -/// without this the first loss of capability would start an unbounded cascade. Cleared on the -/// way out however that happens — an early return, an error, or a panic — because a flag stuck -/// on would silence every later reconciliation for the life of the process. +/// Drops new requests during the write to prevent recursive retries. No-op reconciliations leave +/// it clear so later state changes can still be processed. static DISABLING_TUN: AtomicBool = AtomicBool::new(false); -/// Turn TUN off if the Run State says it cannot work. -/// -/// Reads the current Run State rather than taking a snapshot: this is invoked from a spawned -/// task, so whatever transition triggered it may be several states out of date by the time it -/// runs, and acting on a stale "cannot work" is how a transient startup state would silently -/// rewrite the user's configuration. -/// -/// A no-op in every case except a settled one, so it is safe to call on every transition. +/// Disable TUN when the current settled Run State cannot support it. pub async fn reconcile_tun_availability() { + // Coalesce all reconciliation requests while a disable write is in flight. + if DISABLING_TUN.load(Ordering::Acquire) { + return; + } + + // Wait for any config transaction, then decide from fresh state and committed config. + let config_write = Config::lock_config_write().await; let state = RUN_STATE.state(); - // Before the Core is running the startup decision has not been taken yet: the Service may - // be about to be installed, and `prepare_startup` reads `enable_tun_mode` to decide - // whether to ask. Turning TUN off here would both undo the user's setting and remove the - // very prompt that would have made it work. + // Startup may still install the Service and uses this setting to decide whether to prompt. if matches!(state.mode, RunningMode::NotRunning) { return; } - // Falling back to Sidecar already suppresses TUN for this session. That is deliberately - // not a persistent change — `Config::disable_tun_and_persist` is the separate, explicit - // act — so honour the decision that was already taken rather than escalating it. + // Sidecar fallback suppresses TUN for this session without changing the saved preference. if Config::tun_suppressed_for_session() { return; } - let tun_enabled = Config::verge().await.latest_arc().enable_tun_mode.unwrap_or(false); + // Do not rewrite the saved preference based on an uncommitted draft from a legacy writer. + let tun_enabled = Config::verge().await.data_arc().enable_tun_mode.unwrap_or(false); if !state.tun_should_be_disabled(tun_enabled) { return; } - if DISABLING_TUN.swap(true, Ordering::AcqRel) { - return; - } + // The config lock already serializes writers; the flag only sheds newly spawned work. + DISABLING_TUN.store(true, Ordering::Release); defer! { DISABLING_TUN.store(false, Ordering::Release); } @@ -71,20 +59,9 @@ pub async fn reconcile_tun_availability() { ..IVerge::default() }; - // The patch-and-reconcile form would call straight back into here; this is the plain apply. - match super::apply_verge_patch(&patch, false).await { + // Avoid recursively reconciling this internal patch. + match super::apply_verge_patch_locked(&config_write, &patch, false).await { Ok(()) => Handle::notice_message("tun_mode::auto_disabled", ""), - // Losing the race for the config layer is not a failure the user can act on: this runs - // on every Run State transition and again whenever the setting itself is patched, so - // the writer that won will be followed by another reconciliation. Telling the user that - // turning TUN off "failed" would be both alarming and wrong. - Err(error) if error.downcast_ref::().is_some() => { - logging!( - debug, - Type::Core, - "deferring the TUN reconciliation, configuration is busy: {error:#}" - ); - } Err(error) => { logging!(error, Type::Core, "failed to turn TUN mode off: {error:#}"); Handle::notice_message("tun_mode::auto_disable_failed", "");