From c2ebd953ae02e2bf542f080171cde99b40c890f8 Mon Sep 17 00:00:00 2001 From: gnacho Date: Wed, 26 Aug 2026 10:06:49 +0200 Subject: [PATCH] fix: mark the account offline when its server is unreachable (#179) A Failed outcome alone cannot tell a broken folder from an unreachable account (a reverse proxy answering 502, a dead backend). Two halves: - Engine: a 5xx from the remote ensurer, or a run ending Failed, now triggers a short server health probe. A dead probe upgrades the outcome to NetworkError without spawning nextcloudcmd; a live probe keeps the folder-level Failed. - Scheduler: a NetworkError outcome arms a server-unreachable gate. Automatic triggers only queue (the folder stops occupying the global permit, so a healthy account keeps syncing) and a 30s probe re-checks the server until the first success clears the gate. The folder row reads 'Synchronization blocked: the server is unreachable' instead of the generic network message. --- src/core/account_runtime.rs | 3 + src/core/scheduler.rs | 155 +++++++++++++++++++++++++++++-- src/nextcloud/api.rs | 17 ++++ src/nextcloud/sync_engine.rs | 172 ++++++++++++++++++++++++++++++++++- 4 files changed, 338 insertions(+), 9 deletions(-) diff --git a/src/core/account_runtime.rs b/src/core/account_runtime.rs index aeb2f5a..63541a7 100644 --- a/src/core/account_runtime.rs +++ b/src/core/account_runtime.rs @@ -367,6 +367,9 @@ impl FolderRuntime { ) .with_remote_ensurer(Arc::new(|account, folder, password| { crate::nextcloud::sync_engine::ProductionRemoteEnsurer::run(account, folder, password) + })) + .with_health_probe(Arc::new(|account, _password| { + crate::nextcloud::sync_engine::ProductionHealthProbe::run(account) })); (Box::new(engine), rx) } diff --git a/src/core/scheduler.rs b/src/core/scheduler.rs index 43fa477..7743594 100644 --- a/src/core/scheduler.rs +++ b/src/core/scheduler.rs @@ -41,6 +41,10 @@ pub const COOLDOWN_SECONDS: u64 = 4; pub const KEYRING_RETRY_MAX: u32 = 5; /// Starting retry delay for the keyring backoff (ms); each attempt doubles it. pub const KEYRING_RETRY_BASE_MS: u64 = 2000; +/// Interval (ms) between server health probes while a folder is parked as +/// server-unreachable (issue #179). Kept short so recovery is noticed quickly +/// without hammering the network. +pub const SERVER_PROBE_INTERVAL_MS: u64 = 30_000; /// How a finished reconciliation turned out. #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -158,6 +162,13 @@ struct SchedulerInner { /// automatic triggers stay queued until the user signs in again, so a /// revoked password cannot hammer the server into a brute-force lockout. auth_required: bool, + /// The account server is unreachable (issue #179): the last run failed + /// and a health probe confirmed the server is not answering (a reverse + /// proxy 502, a dead backend). Automatic triggers stay queued so the + /// folder stops occupying the global permit (the other account keeps + /// syncing); a recovery probe periodically re-checks the server and + /// clears the gate on the first success. + server_unreachable: bool, /// Shared flag: true while a run is in flight, false the moment it /// finishes. The progress forwarder reads it so stale events drained /// after the run's `set_progress(None)` cannot repaint the label @@ -211,6 +222,7 @@ impl Scheduler { active_ssid: None, quiet_hours: None, auth_required: false, + server_unreachable: false, run_active: None, }; let inner = Rc::new(RefCell::new(inner)); @@ -376,6 +388,12 @@ impl Scheduler { self.inner.borrow().auth_required } + /// Whether automatic syncs are suspended because the account server is + /// unreachable (issue #179). + pub fn server_unreachable(&self) -> bool { + self.inner.borrow().server_unreachable + } + /// Clear the credential-rejection gate and reconcile immediately /// (issue #72). Called after the user re-enters credentials ("Sign in /// again"); the manual reconciliation verifies them right away and @@ -399,6 +417,11 @@ impl Scheduler { self.inner.borrow().queue.len() } + /// Whether a given trigger reason is currently pending in the queue. + pub fn queue_contains(&self, trigger: Trigger) -> bool { + self.inner.borrow().queue.contains(trigger) + } + /// Periodic interval timers wired to this scheduler's `request`. pub fn timers(&self) -> SyncTimers { let weak = Rc::downgrade(&self.inner); @@ -470,6 +493,19 @@ impl SchedulerInner { ); return; } + // Issue #179: while the account server is unreachable, automatic + // triggers only queue up. The folder stops occupying the global + // permit, so the other account keeps syncing, and a periodic probe + // clears the gate when the server answers again. Manual syncs still + // go through so the user can retry explicitly. + if self.server_unreachable && trigger != Trigger::Manual { + self.queue.add(trigger); + self.state.set( + AppState::Offline, + t("Synchronization blocked: the server is unreachable"), + ); + return; + } if !self.online { self.queue.add(trigger); self.state @@ -559,6 +595,33 @@ impl SchedulerInner { self.start_source = Some(id); } + /// Schedule a periodic probe while the account server is unreachable + /// (issue #179). Unlike the keyring retry this has no budget cap: it keeps + /// re-checking the server every [`SERVER_PROBE_INTERVAL_MS`] until a run + /// succeeds and clears the gate. The probe re-enters `start` on a timer + /// WITHOUT changing the visible state, so the Offline label is preserved + /// while it re-attempts. + fn schedule_server_probe(&mut self) { + if self.stopped + || self.start_source.is_some() + || self.preparing + || self.running + || !self.server_unreachable + { + return; + } + let weak = self.self_ref.clone(); + let id = self.source.borrow_mut().add_timeout( + Duration::from_millis(SERVER_PROBE_INTERVAL_MS), + Box::new(move || { + if let Some(inner) = weak.upgrade() { + inner.borrow_mut().start(); + } + }), + ); + self.start_source = Some(id); + } + fn start(&mut self) { self.start_source = None; if self.stopped || self.preparing || self.running || self.queue.is_empty() || !self.online { @@ -713,6 +776,7 @@ impl SchedulerInner { self.keyring_locked = false; self.keyring_retry_count = 0; self.auth_required = false; + self.server_unreachable = false; self.ever_synced = true; self.set_idle_state(); (true, false) @@ -721,6 +785,7 @@ impl SchedulerInner { self.keyring_locked = false; self.keyring_retry_count = 0; self.auth_required = false; + self.server_unreachable = false; self.ever_synced = true; self.state.set( AppState::IdleOk, @@ -763,16 +828,19 @@ impl SchedulerInner { (true, false) } SyncOutcome::NetworkError => { - // Issue #162: the server itself is unreachable even though the - // machine has a network link. Mark this folder Offline (with - // its own message) so the account no longer reads as Connected - // and stops spinning in a wait loop. The periodic/module - // triggers retry on the next tick, so it clears once the - // server answers again. This only applies to the failing + // Issue #162/#179: the account server is unreachable (a + // transport failure, or a 5xx confirmed by the health probe). + // Mark this folder Offline with its own message so the account + // no longer reads as Connected and stops spinning in a wait + // loop; a periodic probe clears the gate once the server + // answers again. This only applies to the failing // account/folder, never to other accounts. self.keyring_locked = false; - self.state - .set(AppState::Offline, t("Waiting for a network connection")); + self.server_unreachable = true; + self.state.set( + AppState::Offline, + t("Synchronization blocked: the server is unreachable"), + ); (false, false) } }; @@ -842,6 +910,15 @@ impl SchedulerInner { self.queue.add(Trigger::Retry); self.schedule_keyring_retry(); } + // Issue #179: while the account server is unreachable, schedule a + // periodic probe that re-checks it without waiting for the next + // external trigger. Each probe runs the engine (which cuts on the + // health probe if the server is still down, or reconciles once it + // answers again); the first success clears the gate. + if matches!(outcome, SyncOutcome::NetworkError) { + self.queue.add(Trigger::Retry); + self.schedule_server_probe(); + } } } @@ -1406,6 +1483,68 @@ mod tests { assert!(scheduler.keyring_locked()); } + /// Issue #179: a NetworkError outcome arms the server-unreachable gate: + /// automatic triggers only queue up (they stop occupying the global permit + /// so the other account keeps syncing) and a recovery probe is scheduled + /// to re-check the server without waiting for the next interval tick. + #[test] + fn network_error_arms_server_unreachable_and_schedules_a_recovery_probe() { + let (scheduler, source, runner) = make_scheduler(None); + scheduler.request(Trigger::Startup); + run_idle(&source); + assert_eq!(runner.0.borrow().start_calls, 1); + + finish(&runner, SyncOutcome::NetworkError); + assert_eq!(scheduler.state().snapshot().state, AppState::Offline); + assert!(scheduler.server_unreachable()); + // A recovery probe is already pending without any new external trigger. + assert_eq!(source.borrow().pending(), 1); + let probe_id = source.borrow().only_id(); + + // Automatic triggers no longer launch the engine while the server is + // down: they queue (the Retry probe trigger is already there) and the + // folder stays Offline (the global permit stays free for the healthy + // account). + scheduler.request(Trigger::RemoteInterval); + assert_eq!(runner.0.borrow().start_calls, 1); + assert_eq!(scheduler.queue_len(), 2); + assert!(scheduler.queue_contains(Trigger::RemoteInterval)); + assert_eq!(scheduler.state().snapshot().state, AppState::Offline); + + // Firing the probe re-enters the engine; a Success clears the gate. + fire_timer(&source, probe_id); + assert_eq!(runner.0.borrow().start_calls, 2); + finish(&runner, SyncOutcome::Success); + assert!(!scheduler.server_unreachable()); + assert_eq!(scheduler.state().snapshot().state, AppState::IdleOk); + } + + /// Issue #179: after the server recovers (Success clears the gate), a + /// fresh automatic trigger runs the engine again. + #[test] + fn server_recovery_allows_automatic_triggers_again() { + let (scheduler, source, runner) = make_scheduler(None); + scheduler.request(Trigger::Startup); + run_idle(&source); + finish(&runner, SyncOutcome::NetworkError); + assert!(scheduler.server_unreachable()); + // Clear the pending recovery probe without firing it (the server came + // back on its own); simulate that by resolving it as a Success run. + let probe_id = source.borrow().only_id(); + fire_timer(&source, probe_id); + finish(&runner, SyncOutcome::Success); + assert!(!scheduler.server_unreachable()); + + scheduler.request(Trigger::RemoteInterval); + // The previous Success left the folder in its 4s cooldown: the first + // idle consumption fires cooldown_finished (which schedules the start + // because the queue is non-empty), the second runs the engine. + run_idle(&source); + run_idle(&source); + assert_eq!(runner.0.borrow().start_calls, 3); + assert_eq!(scheduler.state().snapshot().state, AppState::Syncing); + } + #[test] fn shared_permit_queues_a_second_account_until_release() { let permit = SyncPermit::try_new(1).unwrap(); diff --git a/src/nextcloud/api.rs b/src/nextcloud/api.rs index fedaae0..6cf26ed 100644 --- a/src/nextcloud/api.rs +++ b/src/nextcloud/api.rs @@ -275,6 +275,23 @@ impl NextcloudApi { Ok(display_name) } + /// Server health probe (issue #179): a short GET to the server root. + /// + /// Any HTTP response (2xx/3xx/4xx) means the server is up and answering; + /// a transport failure or a 5xx (a reverse proxy whose backend is down, + /// a dead upstream) means it is not. Used by the engine to tell "the + /// folder broke" from "the account is unreachable". + pub fn server_status(&self, server: &str) -> Result<(), ApiError> { + let url = format!("{}/", server.trim_end_matches('/')); + let response = self.http.request("GET", &url, &[], None)?; + if (500..600).contains(&response.status) { + return Err(ApiError::Http { + status: response.status, + }); + } + Ok(()) + } + /// Account summary from the OCS user endpoint: display name plus quota. /// /// `used`/`total` are bytes; servers with no quota report negative diff --git a/src/nextcloud/sync_engine.rs b/src/nextcloud/sync_engine.rs index 8352efc..e3e906e 100644 --- a/src/nextcloud/sync_engine.rs +++ b/src/nextcloud/sync_engine.rs @@ -140,6 +140,33 @@ impl SyncResult { pub type RemoteEnsurer = Arc Result<(), ApiError> + Send + Sync>; +/// Confirms the account server is alive and answering HTTP. +/// +/// Issue #179: a `Failed` outcome alone cannot distinguish a broken folder +/// from an unreachable server (a reverse proxy answering 502, a backend that +/// stopped responding). When a run fails, the engine asks this probe "is the +/// server actually up?"; a dead answer upgrades the outcome to +/// [`SyncOutcome::NetworkError`] so the scheduler parks the account Offline +/// instead of retrying a dead server forever. +pub type HealthProbe = Arc Result<(), ApiError> + Send + Sync>; + +/// Production [`HealthProbe`]: a short GET to the server's status endpoint. +/// +/// Nextcloud answers `/status.php`; OpenCloud answers `/` with a status +/// payload. Any 2xx/3xx means the server is up; a 5xx (proxy dead, backend +/// down) or a transport error means it is not. Auth responses (401/403) are +/// not part of the health probe - the account's own credential flow reports +/// those. +#[derive(Default)] +pub struct ProductionHealthProbe; + +impl ProductionHealthProbe { + /// Run the health check for one account. + pub fn run(account: &AccountConfig) -> Result<(), ApiError> { + NextcloudApi::new().server_status(&account.server_url) + } +} + /// Production [`RemoteEnsurer`]: MKCOL the folder's remote path before the /// engine runs. Nextcloud creates it under the per-user files tree; OpenCloud /// under the folder's space (issue #55; both verified against real @@ -196,6 +223,7 @@ pub struct SyncEngine { credentials: Arc, process: Arc>>, remote_ensurer: Option, + health_probe: Option, } impl SyncEngine { @@ -222,6 +250,7 @@ impl SyncEngine { credentials: Arc::new(KeyringCredentialSource), process: Arc::new(Mutex::new(None)), remote_ensurer: None, + health_probe: None, } } @@ -237,6 +266,13 @@ impl SyncEngine { self } + /// Install the server health probe (issue #179). Without it a Failed run + /// is never upgraded to NetworkError. + pub fn with_health_probe(mut self, probe: HealthProbe) -> Self { + self.health_probe = Some(probe); + self + } + /// Whether a reconciliation is currently running. pub fn is_running(&self) -> bool { self.process @@ -258,6 +294,7 @@ impl SyncRunner for SyncEngine { let credentials = Arc::clone(&self.credentials); let process = Arc::clone(&self.process); let remote_ensurer = self.remote_ensurer.clone(); + let health_probe = self.health_probe.clone(); let inputs = EngineInputs { account, folder, @@ -265,6 +302,7 @@ impl SyncRunner for SyncEngine { exclude_file, executable, remote_ensurer, + health_probe, }; glib::spawn_future_local(async move { let run = @@ -326,6 +364,7 @@ struct EngineInputs { exclude_file: Option, executable: Option, remote_ensurer: Option, + health_probe: Option, } /// Run the whole reconciliation on the blocking thread pool. @@ -364,6 +403,18 @@ fn engine_thread( Ok(()) => {} Err(ApiError::AuthRejected) => return EngineRun::Direct(SyncOutcome::AuthFailed), Err(ApiError::Transport) => return EngineRun::Direct(SyncOutcome::NetworkError), + // Issue #179: a 5xx from the server/proxy (a reverse proxy + // answering 502 because the backend is down) is not a folder + // problem - it is the account being unreachable. Confirm with a + // health probe before launching nextcloudcmd; a dead probe means + // the server is not answering and the run must not proceed. + Err(ApiError::Http { status }) if (500..600).contains(&status) => { + if let Some(probe) = inputs.health_probe.as_ref() { + if probe(&inputs.account, &password).is_err() { + return EngineRun::Direct(SyncOutcome::NetworkError); + } + } + } Err(_) => {} } } @@ -372,7 +423,7 @@ fn engine_thread( &inputs.account, &inputs.folder, &inputs.network, - password, + password.clone(), inputs.exclude_file.clone(), inputs.executable.clone(), ); @@ -445,6 +496,18 @@ fn engine_thread( output, classification, }; + // Issue #179: a run that failed while the server does not answer a health + // probe means the account is unreachable (a proxy answering 502, the + // backend gone), not that the folder is broken. Upgrade to NetworkError so + // the scheduler parks the folder Offline and stops retrying a dead server + // on every trigger. A live probe keeps the folder-level Failed. + if result.classification == Classification::SyncError { + if let Some(probe) = inputs.health_probe.as_ref() { + if probe(&inputs.account, &password).is_err() { + return EngineRun::Direct(SyncOutcome::NetworkError); + } + } + } EngineRun::Result(result) } @@ -1031,6 +1094,113 @@ mod tests { assert_eq!(outcome, SyncOutcome::Success); } + /// Issue #179: a 5xx from the remote ensurer with a server that does not + /// answer a health probe must short-circuit to NetworkError WITHOUT + /// launching nextcloudcmd (a dead server should not be spawned against). + /// The fake binary writes a marker file, so its presence would prove the + /// engine ran; a dead health probe must prevent the spawn entirely. + #[test] + fn ensurer_http_5xx_with_dead_health_probe_short_circuits_to_network_error() { + let dir = tempfile::tempdir().unwrap(); + let marker = dir.path().join("engine-ran"); + let script = write_script( + dir.path(), + "fake-engine-5xx-dead", + &format!("#!/bin/sh\ntouch {}\nexit 0\n", marker.display()), + ); + let (progress_tx, _progress_rx) = async_channel::unbounded(); + let engine = SyncEngine::new( + account(), + folder(), + NetworkConfig::default(), + None, + Some(script), + progress_tx, + ) + .with_credentials(Arc::new(FakeCredentials(CredentialLookup::Found( + "secret".to_string(), + )))) + .with_remote_ensurer(Arc::new(|_account, _folder, _password| { + Err(ApiError::Http { status: 502 }) + })) + .with_health_probe(Arc::new(|_account, _password| Err(ApiError::Transport))); + let (outcome, _) = run_engine(engine, &async_channel::unbounded().1); + assert_eq!(outcome, SyncOutcome::NetworkError); + assert!(!marker.exists(), "nextcloudcmd must not be spawned"); + } + + /// Issue #179: a 5xx from the ensurer with a live health probe is a + /// folder-specific error, not a connectivity failure - the run proceeds. + #[test] + fn ensurer_http_5xx_with_live_health_probe_keeps_the_run() { + let dir = tempfile::tempdir().unwrap(); + let script = write_script(dir.path(), "fake-ensure-5xx-live", "#!/bin/sh\nexit 0\n"); + let (progress_tx, _progress_rx) = async_channel::unbounded(); + let engine = SyncEngine::new( + account(), + folder(), + NetworkConfig::default(), + None, + Some(script), + progress_tx, + ) + .with_credentials(Arc::new(FakeCredentials(CredentialLookup::Found( + "secret".to_string(), + )))) + .with_remote_ensurer(Arc::new(|_account, _folder, _password| { + Err(ApiError::Http { status: 500 }) + })) + .with_health_probe(Arc::new(|_account, _password| Ok(()))); + let (outcome, _) = run_engine(engine, &async_channel::unbounded().1); + assert_eq!(outcome, SyncOutcome::Success); + } + + /// Issue #179: a run that ends Failed while the server does not answer a + /// health probe must be reported as NetworkError (the account is + /// unreachable, not the folder broken). + #[test] + fn failed_run_with_dead_health_probe_becomes_network_error() { + let dir = tempfile::tempdir().unwrap(); + let script = write_script(dir.path(), "fake-fail", "#!/bin/sh\nexit 1\n"); + let (progress_tx, _progress_rx) = async_channel::unbounded(); + let engine = SyncEngine::new( + account(), + folder(), + NetworkConfig::default(), + None, + Some(script), + progress_tx, + ) + .with_credentials(Arc::new(FakeCredentials(CredentialLookup::Found( + "secret".to_string(), + )))) + .with_health_probe(Arc::new(|_account, _password| Err(ApiError::Transport))); + let (outcome, _) = run_engine(engine, &async_channel::unbounded().1); + assert_eq!(outcome, SyncOutcome::NetworkError); + } + + /// Issue #179: a Failed run with a live server stays a folder error. + #[test] + fn failed_run_with_live_health_probe_stays_failed() { + let dir = tempfile::tempdir().unwrap(); + let script = write_script(dir.path(), "fake-fail-live", "#!/bin/sh\nexit 1\n"); + let (progress_tx, _progress_rx) = async_channel::unbounded(); + let engine = SyncEngine::new( + account(), + folder(), + NetworkConfig::default(), + None, + Some(script), + progress_tx, + ) + .with_credentials(Arc::new(FakeCredentials(CredentialLookup::Found( + "secret".to_string(), + )))) + .with_health_probe(Arc::new(|_account, _password| Ok(()))); + let (outcome, _) = run_engine(engine, &async_channel::unbounded().1); + assert_eq!(outcome, SyncOutcome::Failed); + } + #[test] fn production_ensurer_skips_root_and_spaceless_targets() { // Root-of-account (Nextcloud) and root-of-space (OpenCloud) targets