diff --git a/src-tauri/src/core/manager/lifecycle.rs b/src-tauri/src/core/manager/lifecycle.rs index 1eeeb4f7c..b80cd53f2 100644 --- a/src-tauri/src/core/manager/lifecycle.rs +++ b/src-tauri/src/core/manager/lifecycle.rs @@ -288,10 +288,11 @@ impl CoreManager { if !can_allow_sidecar_for_session(&mode, &status) { anyhow::bail!("Sidecar continuation is not allowed from {mode:?} / {status:?}"); } - proxy_control::stop_guard().await; Config::suppress_tun_for_session().await; Config::generate().await?; SERVICE_MANAGER.allow_sidecar_for_session()?; + // Drop the guard last so an earlier failure cannot leave a running Sidecar unguarded. + proxy_control::stop_guard().await; let result = async { self.start_core_inner().await?; if !matches!(*self.get_running_mode(), RunningMode::Sidecar) { @@ -301,7 +302,8 @@ impl CoreManager { } .await; if let Err(error) = &result { - SERVICE_MANAGER.mark_unavailable(format!("Sidecar startup failed: {error:#}")); + // Revoke only the failed Sidecar allowance; its startup says nothing about Service health. + SERVICE_MANAGER.withdraw_sidecar_allowance(); if let Err(cleanup_error) = self.rollback_failed_sidecar_transition().await { return Err(anyhow::anyhow!( "Sidecar startup failed: {error:#}; failed to clear proxy before rollback: {cleanup_error:#}" @@ -1256,6 +1258,25 @@ mod tests { )); } + #[test] + fn a_sidecar_that_failed_to_start_can_be_tried_again() { + // Revoking a failed attempt's allowance keeps retries reachable without changing Service health. + for health in [ + ServiceStatus::NotInstalled, + ServiceStatus::NeedsReinstall, + ServiceStatus::Unavailable("boom".into()), + ] { + assert!( + can_allow_sidecar_for_session(&RunningMode::NotRunning, &health), + "{health:?} must still admit a second attempt" + ); + } + assert!(!can_allow_sidecar_for_session( + &RunningMode::NotRunning, + &ServiceStatus::SidecarAllowed, + )); + } + #[test] fn proxy_restore_requires_the_same_ready_core_generation() { let sidecar = diff --git a/src-tauri/src/core/runstate/env.rs b/src-tauri/src/core/runstate/env.rs index 998731fd7..46f87b46c 100644 --- a/src-tauri/src/core/runstate/env.rs +++ b/src-tauri/src/core/runstate/env.rs @@ -5,7 +5,7 @@ //! this trait rather than reaching for globals, the state machine can be exercised without //! a running app, an installed Service, or elevation. -use anyhow::Result; +use anyhow::{Context as _, Result}; use super::health::{PendingAction, RunState}; use super::probe::ServiceVersionReply; @@ -19,7 +19,9 @@ pub trait RunStateEnv: Send + Sync + 'static { /// /// Fails when the registry itself could not be inspected, which is different from /// "no installation" and is treated as unavailable rather than not-installed. - fn trusted_install_evidence(&self) -> Result; + /// + /// Async so blocking systemd and Windows SCM checks can run off the runtime workers. + fn trusted_install_evidence(&self) -> impl Future> + Send; /// Whether this app process is running elevated. fn is_elevated(&self) -> bool; @@ -58,8 +60,10 @@ impl RunStateEnv for RealEnv { }) } - fn trusted_install_evidence(&self) -> Result { - crate::core::service::trusted_service_evidence() + async fn trusted_install_evidence(&self) -> Result { + tokio::task::spawn_blocking(crate::core::service::trusted_service_evidence) + .await + .context("service registration probe did not finish")? } fn is_elevated(&self) -> bool { @@ -259,7 +263,7 @@ mod fake { reply.map_err(|error| anyhow!(error)) } - fn trusted_install_evidence(&self) -> Result { + async fn trusted_install_evidence(&self) -> Result { self.evidence.clone().map_err(|error| anyhow!(error)) } diff --git a/src-tauri/src/core/runstate/mod.rs b/src-tauri/src/core/runstate/mod.rs index 7023ef547..6d7d2d032 100644 --- a/src-tauri/src/core/runstate/mod.rs +++ b/src-tauri/src/core/runstate/mod.rs @@ -66,18 +66,29 @@ impl std::fmt::Display for ReadyWaitError { } } -/// The service-owned state together with the counter that versions it. +/// The service-owned state together with the counters that version it. /// /// They share one lock so a reader can never see a change without its version bump. +/// `generation` tracks visible changes; `observation` also tracks assertions that invalidate +/// older health probes without changing visible state. #[derive(Debug, Default)] struct VersionedService { service: StoredService, generation: u64, + observation: u64, } impl VersionedService { + /// Record a visible change and invalidate older health probes. const fn bump(&mut self) { self.generation = self.generation.wrapping_add(1); + self.observation = self.observation.wrapping_add(1); + } + + /// Reserve a revision that invalidates older health probes. + const fn next_observation(&mut self) -> u64 { + self.observation = self.observation.wrapping_add(1); + self.observation } } @@ -194,7 +205,7 @@ impl RunStateStore { /// Platform evidence that cannot be inspected is *unavailable*, not *absent* — assuming /// "not installed" there would offer the user an install that is already there. pub async fn detect_service_health(&self) -> ServiceHealth { - let has_marker = match self.env.trusted_install_evidence() { + let has_marker = match self.env.trusted_install_evidence().await { Ok(exists) => exists, Err(error) => { logging!( @@ -225,10 +236,52 @@ impl RunStateStore { // ─────────────────────────── writes ─────────────────────────── + /// Probe and record Service health unless a newer observation arrived first. + /// + /// Reserving before I/O prevents slower probes from overwriting later observations, including + /// assertions that leave visible state unchanged. Returns the health that remains recorded. + pub async fn observe_current_health(&self) -> ServiceHealth { + let reservation = self.reserve_health_observation(); + let health = self.detect_service_health().await; + self.commit_reserved_observation(health, reservation) + } + + /// Reserve the revision for a health probe. + fn reserve_health_observation(&self) -> u64 { + self.service.lock().next_observation() + } + + /// Commit `health` only if `reservation` is still latest; otherwise return the newer health. + /// + /// The comparison and write share one lock. A successful commit reuses its reserved revision. + fn commit_reserved_observation(&self, health: ServiceHealth, reservation: u64) -> ServiceHealth { + let mut state = self.service.lock(); + if state.observation != reservation { + let newer = state.service.health.clone(); + drop(state); + logging!( + debug, + Type::Service, + "discarding a stale service reading of {health:?}; {newer:?} arrived while it ran" + ); + return newer; + } + if state.service.health == health && state.service.pending.is_none() && !state.service.sidecar_allowed { + return health; + } + state.service.observe(health.clone()); + state.generation = state.generation.wrapping_add(1); + drop(state); + self.announce(); + health + } + /// Record an observation about the Service, clearing any request it answers. pub fn observe(&self, health: ServiceHealth) { let mut state = self.service.lock(); if state.service.health == health && state.service.pending.is_none() && !state.service.sidecar_allowed { + // Reassertion is invisible but must invalidate an older health probe. + state.next_observation(); return; } state.service.observe(health); @@ -245,6 +298,11 @@ impl RunStateStore { pub fn request_action(&self, action: PendingAction) -> bool { let mut state = self.service.lock(); let displaced = state.service.sidecar_allowed; + if state.service.pending == Some(action) && !displaced { + // Reassertion must prevent an older probe from retiring the request. + state.next_observation(); + return displaced; + } state.service.request(action); state.bump(); drop(state); @@ -258,6 +316,8 @@ impl RunStateStore { pub fn accept_sidecar(&self) { let mut state = self.service.lock(); if state.service.sidecar_allowed && state.service.pending.is_none() { + // Reassertion must prevent an older probe from clearing the allowance. + state.next_observation(); return; } state.service.allow_sidecar(); @@ -272,7 +332,13 @@ impl RunStateStore { /// Service. Returns whether the allowance was restored. pub fn restore_sidecar_allowance(&self) -> bool { let mut state = self.service.lock(); - if state.service.sidecar_allowed || matches!(state.service.health, ServiceHealth::Ready) { + if state.service.sidecar_allowed { + // Reassertion must prevent an older probe from clearing the allowance. + state.next_observation(); + return false; + } + if matches!(state.service.health, ServiceHealth::Ready) { + // Cached Ready health does not invalidate an in-flight probe. return false; } state.service.allow_sidecar(); @@ -282,6 +348,21 @@ impl RunStateStore { true } + /// Revoke an allowance after Sidecar startup fails without changing Service health. + /// + /// Returns whether the allowance was revoked. + pub fn withdraw_sidecar_allowance(&self) -> bool { + let mut state = self.service.lock(); + if !state.service.sidecar_allowed { + return false; + } + state.service.sidecar_allowed = false; + state.bump(); + drop(state); + self.announce(); + true + } + /// Accept Sidecar for the rest of this app session. /// /// Rejected while an operation is in flight, or from a state where the Service is @@ -324,6 +405,9 @@ impl RunStateStore { { state.service.request(PendingAction::Install); state.bump(); + } else if state.service.pending == Some(PendingAction::Install) { + // Reassertion must prevent an older probe from clearing the install request. + state.next_observation(); } drop(state); self.announce(); @@ -341,13 +425,12 @@ impl RunStateStore { Ok(()) if matches!(action, PendingAction::Uninstall) => self.observe(ServiceHealth::NotInstalled), Ok(()) => {} Err(error) => { - let health = self.detect_service_health().await; + let health = self.observe_current_health().await; logging!( warn, Type::Service, "privileged service action {action:?} did not complete ({error:#}); service is {health:?}" ); - self.observe(health); } } outcome @@ -504,6 +587,146 @@ mod tests { } } + #[tokio::test] + async fn a_reading_nothing_overtook_is_recorded() { + let store = with_env(FakeEnv::service_ready(FakeEnv::new())); + + let health = store.observe_current_health().await; + + assert_eq!(health, ServiceHealth::Ready); + assert_eq!(store.state().health, ServiceHealth::Ready); + } + + #[test] + fn a_reading_overtaken_by_a_changed_observation_is_discarded() { + // A slower earlier probe must not overwrite a newer Ready observation. + let store = with_env(FakeEnv::new()); + let reservation = store.reserve_health_observation(); + store.observe(ServiceHealth::Ready); + + let recorded = store.commit_reserved_observation(ServiceHealth::NotInstalled, reservation); + + assert_eq!(recorded, ServiceHealth::Ready, "the newer observation is reported back"); + assert_eq!(store.state().health, ServiceHealth::Ready, "and it is what stands"); + } + + #[test] + fn a_reading_overtaken_by_an_unchanged_observation_is_also_discarded() { + // Even an unchanged "still Ready" observation must invalidate an older probe. + let store = with_env(FakeEnv::new()); + store.observe(ServiceHealth::Ready); + let reservation = store.reserve_health_observation(); + let generation = store.generation_count(); + + store.observe(ServiceHealth::Ready); + assert_eq!(store.generation_count(), generation, "nothing visible changed"); + + let recorded = store.commit_reserved_observation(ServiceHealth::NotInstalled, reservation); + + assert_eq!(recorded, ServiceHealth::Ready); + assert_eq!(store.state().health, ServiceHealth::Ready); + } + + #[test] + fn a_reading_overtaken_by_an_already_satisfied_sidecar_restore_is_discarded() { + // Reasserting an existing allowance must stop an older probe from clearing it. + let store = with_env(FakeEnv::new()); + store.observe(ServiceHealth::VersionMismatch); + store.accept_sidecar(); + let reservation = store.reserve_health_observation(); + + assert!(!store.restore_sidecar_allowance(), "the allowance is already there"); + + store.commit_reserved_observation(ServiceHealth::VersionMismatch, reservation); + + assert!(store.state().sidecar_allowed, "the confirmed allowance survives"); + assert!(!store.state().service_needs_attention()); + } + + #[test] + fn re_requesting_the_same_action_changes_nothing_visible_but_still_orders() { + // The same request leaves `generation` unchanged but must invalidate an older probe. + let store = with_env(FakeEnv::new()); + store.observe(ServiceHealth::NotInstalled); + store.require_install_for_session().expect("absent service"); + let generation = store.generation_count(); + let reservation = store.reserve_health_observation(); + + assert!( + !store.request_action(PendingAction::Install), + "no Sidecar allowance was displaced" + ); + assert_eq!(store.generation_count(), generation, "nothing visible changed"); + + store.commit_reserved_observation(ServiceHealth::NotInstalled, reservation); + + assert_eq!(store.state().pending, Some(PendingAction::Install)); + } + + #[test] + fn a_reading_overtaken_by_an_already_pending_install_is_discarded() { + // Requiring an existing install request must stop an older probe from clearing it. + let store = with_env(FakeEnv::new()); + store.observe(ServiceHealth::NotInstalled); + store.require_install_for_session().expect("absent service"); + let reservation = store.reserve_health_observation(); + + store.require_install_for_session().expect("already requested"); + + store.commit_reserved_observation(ServiceHealth::NotInstalled, reservation); + + assert_eq!(store.state().pending, Some(PendingAction::Install)); + assert!(store.state().service_needs_attention()); + } + + #[test] + fn a_later_reading_supersedes_one_that_is_still_running() { + // The last probe started wins regardless of completion order. + let store = with_env(FakeEnv::new()); + store.observe(ServiceHealth::Ready); + let first = store.reserve_health_observation(); + let second = store.reserve_health_observation(); + + assert_eq!( + store.commit_reserved_observation(ServiceHealth::NotInstalled, first), + ServiceHealth::Ready, + "the earlier reading is already superseded" + ); + assert_eq!( + store.commit_reserved_observation(ServiceHealth::VersionMismatch, second), + ServiceHealth::VersionMismatch + ); + assert_eq!(store.state().health, ServiceHealth::VersionMismatch); + } + + #[test] + fn withdrawing_a_sidecar_allowance_says_nothing_about_the_service() { + // A failed Sidecar startup must not change Service health. + let store = with_env(FakeEnv::new()); + store.observe(ServiceHealth::NotInstalled); + store.accept_sidecar(); + + assert!(store.withdraw_sidecar_allowance()); + + assert!(!store.state().sidecar_allowed); + assert_eq!(store.state().health, ServiceHealth::NotInstalled, "health is untouched"); + assert!( + !store.state().service_needs_attention(), + "an absent Service still asks nothing" + ); + } + + #[test] + fn withdrawing_an_allowance_that_was_never_granted_changes_nothing() { + let store = with_env(FakeEnv::new()); + store.observe(ServiceHealth::NotInstalled); + let generation = store.generation_count(); + + assert!(!store.withdraw_sidecar_allowance()); + + assert_eq!(store.generation_count(), generation); + } + #[test] fn a_fresh_store_knows_nothing() { let store = with_env(FakeEnv::new()); diff --git a/src-tauri/src/core/service.rs b/src-tauri/src/core/service.rs index 746b997a8..79e83a4e2 100644 --- a/src-tauri/src/core/service.rs +++ b/src-tauri/src/core/service.rs @@ -1299,8 +1299,8 @@ impl ServiceManager { RUN_STATE.require_install_for_session() } - pub(crate) fn mark_unavailable(&self, reason: impl Into) { - self.set_status(ServiceStatus::Unavailable(reason.into())); + pub(crate) fn withdraw_sidecar_allowance(&self) -> bool { + RUN_STATE.withdraw_sidecar_allowance() } pub async fn detect_startup_status(&self) { @@ -1308,7 +1308,7 @@ impl ServiceManager { RUN_STATE.accept_sidecar(); return; } - RUN_STATE.observe(RUN_STATE.detect_service_health().await); + RUN_STATE.observe_current_health().await; } fn set_status(&self, status: ServiceStatus) { diff --git a/src/components/layout/service-migration-dialog.tsx b/src/components/layout/service-migration-dialog.tsx index 37adb93e8..7bb705202 100644 --- a/src/components/layout/service-migration-dialog.tsx +++ b/src/components/layout/service-migration-dialog.tsx @@ -34,14 +34,15 @@ export const ServiceMigrationDialog = () => { // snapshot; a failed refresh is treated as needing one, since we cannot tell otherwise. const needsDecision = stateRefreshFailed || Boolean(runState?.serviceNeedsAttention) - // Which of the three remedies the dialog offers. A refresh we could not complete is - // treated as an unreachable service, which is what 'repair' is for. + // Treat refresh failures as unreachable; an absent Service still needs install after a failed Sidecar attempt. const remedy: 'install' | 'repair' | 'reinstall' = runState?.pendingAction === 'install' ? 'install' : stateRefreshFailed || runState?.service === 'unavailable' ? 'repair' - : 'reinstall' + : runState?.service === 'notInstalled' + ? 'install' + : 'reinstall' const open = loading || workflowIncomplete || needsDecision const showCheckingMessage = loading || !needsDecision