diff --git a/src/core/scheduler.rs b/src/core/scheduler.rs index 21255e6..6df32bf 100644 --- a/src/core/scheduler.rs +++ b/src/core/scheduler.rs @@ -34,13 +34,18 @@ pub const DEBOUNCE_MS: u64 = 2000; /// Cooldown after a sync finishes before the next one may start (s). pub const COOLDOWN_SECONDS: u64 = 4; -/// Issue #170: budget of back-to-back keyring retries before the folder parks -/// in the keyring-locked state. A transiently-unavailable Secret Service (bus -/// coming up after login) recovers within these, but a genuinely locked -/// collection must not retry forever. +/// Issue #170: budget of back-to-back keyring retries before the folder falls +/// back to the slow keyring watch (issue #214). A transiently-unavailable +/// Secret Service (bus coming up after login) recovers within these, but a +/// genuinely locked collection must not retry at a fast pace forever. 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 keyring re-checks once the fast retry budget is +/// exhausted (issue #214). Mirrors the server probe (issue #179): uncapped, +/// so the folder recovers without a restart whenever the keyring is unlocked +/// later, by any means. +pub const KEYRING_WATCH_INTERVAL_MS: u64 = 60_000; /// 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. @@ -665,26 +670,53 @@ impl SchedulerInner { } /// Schedule a keyring retry with a capped exponential backoff (issue - /// #170). Once the retry budget is exhausted the folder stays in the - /// keyring-locked state and waits for a manual action instead of retrying - /// forever. The retry re-enters `start` on a timer WITHOUT changing the + /// #170). The retry re-enters `start` on a timer WITHOUT changing the /// visible state, so the informative keyring-locked label is preserved - /// while it re-attempts. + /// while it re-attempts. Once the fast budget is exhausted the folder + /// falls back to a slow periodic watch (issue #214) instead of parking + /// until a restart: a locked keyring is often unlocked later in the + /// session (by the user, the session itself, or the engine's own unlock + /// attempt), and the folder must notice on its own. fn schedule_keyring_retry(&mut self) { + if self.stopped || self.start_source.is_some() || self.preparing || self.running { + return; + } + if self.keyring_retry_count >= KEYRING_RETRY_MAX { + self.schedule_keyring_watch(); + return; + } + let attempt = self.keyring_retry_count; + self.keyring_retry_count += 1; + let delay = Duration::from_millis(KEYRING_RETRY_BASE_MS << attempt.min(4)); + let weak = self.self_ref.clone(); + let id = self.source.borrow_mut().add_timeout( + delay, + Box::new(move || { + if let Some(inner) = weak.upgrade() { + inner.borrow_mut().start(); + } + }), + ); + self.start_source = Some(id); + } + + /// Slow periodic re-check while the keyring stays locked (issue #214), + /// mirroring the server probe (issue #179): each tick re-enters `start` + /// WITHOUT changing the visible state; a run that still finds the keyring + /// locked re-arms the watch from `finished`, and the first run that + /// resolves credentials clears the gate. + fn schedule_keyring_watch(&mut self) { if self.stopped || self.start_source.is_some() || self.preparing || self.running - || self.keyring_retry_count >= KEYRING_RETRY_MAX + || !self.keyring_locked { return; } - let attempt = self.keyring_retry_count; - self.keyring_retry_count += 1; - let delay = Duration::from_millis(KEYRING_RETRY_BASE_MS << attempt.min(4)); let weak = self.self_ref.clone(); let id = self.source.borrow_mut().add_timeout( - delay, + Duration::from_millis(KEYRING_WATCH_INTERVAL_MS), Box::new(move || { if let Some(inner) = weak.upgrade() { inner.borrow_mut().start(); @@ -1727,28 +1759,62 @@ mod tests { assert_eq!(scheduler.state().snapshot().state, AppState::IdleOk); } - /// Issue #170: the keyring retry has a capped budget, so a genuinely - /// locked collection does not retry forever. + /// Issue #170: the fast keyring retry has a capped budget, so a genuinely + /// locked collection does not retry back-to-back forever. Issue #214: + /// once the budget is exhausted the folder falls back to a slow periodic + /// watch instead of parking until a restart. #[test] - fn keyring_retry_budget_is_capped() { + fn keyring_retry_budget_is_capped_and_falls_back_to_a_watch() { let (scheduler, source, runner) = make_scheduler(None); scheduler.request(Trigger::Startup); run_idle(&source); - // Burn the whole retry budget with back-to-back locked outcomes. - let mut calls = 1; - loop { + // Burn the whole fast retry budget with back-to-back locked outcomes. + for _ in 0..KEYRING_RETRY_MAX { finish(&runner, SyncOutcome::KeyringLocked); - if source.borrow().pending() == 0 { - break; - } let retry_id = source.borrow().only_id(); fire_timer(&source, retry_id); - calls += 1; } - assert_eq!(calls, 1 + KEYRING_RETRY_MAX as usize); - // No more retries are scheduled; the folder is parked keyring-locked. - assert_eq!(source.borrow().pending(), 0); + assert_eq!( + runner.0.borrow().start_calls, + 1 + KEYRING_RETRY_MAX as usize + ); + // The next locked outcome exhausts the budget: no further fast retry, + // but a slow watch keeps re-checking the keyring. + finish(&runner, SyncOutcome::KeyringLocked); + assert!(scheduler.keyring_locked()); + assert_eq!( + source.borrow().pending(), + 1, + "a keyring watch must keep re-checking after the retry budget" + ); + } + + /// Issue #214: after the fast retry budget is exhausted, the slow keyring + /// watch keeps re-entering the engine on its own, so the folder recovers + /// without restarting the app as soon as the keyring is unlocked - by the + /// user, the session, or the app's own unlock attempt. + #[test] + fn keyring_watch_recovers_without_restart_once_unlocked() { + let (scheduler, source, runner) = make_scheduler(None); + scheduler.request(Trigger::Startup); + run_idle(&source); + for _ in 0..KEYRING_RETRY_MAX { + finish(&runner, SyncOutcome::KeyringLocked); + let retry_id = source.borrow().only_id(); + fire_timer(&source, retry_id); + } + // The budget is exhausted: the folder sits keyring-locked with only + // the slow watch pending, no external trigger in sight. + finish(&runner, SyncOutcome::KeyringLocked); + let calls = runner.0.borrow().start_calls; assert!(scheduler.keyring_locked()); + // The keyring gets unlocked; the next watch tick reconciles. + let watch_id = source.borrow().only_id(); + fire_timer(&source, watch_id); + assert_eq!(runner.0.borrow().start_calls, calls + 1); + finish(&runner, SyncOutcome::Success); + assert!(!scheduler.keyring_locked()); + assert_eq!(scheduler.state().snapshot().state, AppState::IdleOk); } /// Issue #179: a NetworkError outcome arms the server-unreachable gate: diff --git a/src/nextcloud/credentials.rs b/src/nextcloud/credentials.rs index 4deb371..e2980f2 100644 --- a/src/nextcloud/credentials.rs +++ b/src/nextcloud/credentials.rs @@ -184,6 +184,27 @@ impl CredentialsStore { Ok(Some(password)) } + /// Try to unlock every locked collection (issue #214). + /// + /// GNOME collections created by the desktop session usually share the + /// login password: the Secret Service then completes the unlock prompt + /// on its own as long as the client connection stays alive through it, + /// so the common case returns without any user interaction. Returns + /// `Ok(true)` when at least one collection was locked and got unlocked. + /// Per-collection failures are skipped so one stubborn collection does + /// not block the rest. + pub fn unlock_locked_collections() -> Result { + let service = SecretService::connect(EncryptionType::Dh)?; + let mut unlocked_any = false; + for collection in service.get_all_collections()? { + let was_locked = collection.is_locked().unwrap_or(false); + if was_locked && collection.unlock().is_ok() { + unlocked_any = true; + } + } + Ok(unlocked_any) + } + /// Drop the cached password for an account (issue #178). /// /// Called when a sync run proves the credential wrong (authentication diff --git a/src/nextcloud/sync_engine.rs b/src/nextcloud/sync_engine.rs index 16b04d6..364dc1b 100644 --- a/src/nextcloud/sync_engine.rs +++ b/src/nextcloud/sync_engine.rs @@ -80,6 +80,18 @@ impl CredentialLookup { pub trait CredentialSource: Send + Sync + 'static { /// Resolve the password for the given account. fn lookup(&self, account: &AccountConfig) -> CredentialLookup; + + /// Issue #214: best-effort unlock of the keyring collections. Returns + /// `true` when at least one locked collection was unlocked, so the + /// caller re-runs [`CredentialSource::lookup`] once. GNOME collections + /// created by the desktop session usually share the login password: the + /// Secret Service then completes the unlock prompt on its own as long as + /// the client connection stays alive through it, so the common case + /// needs no user interaction. Sources without an unlock facility keep + /// the default: no unlock, no retry. + fn unlock_keyring(&self) -> bool { + false + } } /// [`CredentialSource`] backed by the desktop Secret Service. @@ -100,6 +112,10 @@ impl CredentialSource for KeyringCredentialSource { Err(_) => CredentialLookup::Unavailable, } } + + fn unlock_keyring(&self) -> bool { + CredentialsStore::unlock_locked_collections().unwrap_or(false) + } } /// Outcome of a finished `nextcloudcmd` run, mirroring `SyncResult`. @@ -458,7 +474,16 @@ fn engine_thread( process: Arc>>, ) -> EngineRun { let started = Instant::now(); - let password = match credentials.lookup(&inputs.account) { + let mut lookup = credentials.lookup(&inputs.account); + // Issue #214: a locked collection may share the session password, in + // which case the Secret Service unlocks it without prompting while the + // client connection stays alive through the prompt. When the unlock + // succeeds, resolve the password once more instead of reporting the + // keyring as locked. + if matches!(lookup, CredentialLookup::Locked) && credentials.unlock_keyring() { + lookup = credentials.lookup(&inputs.account); + } + let password = match lookup { CredentialLookup::Found(password) => password, // Transient secret-service trouble (bus not ready at startup, locked // collection, agent hiccup) must not arm the credential gate @@ -729,6 +754,44 @@ mod tests { } } + /// Issue #214: a source whose lookup is locked once but whose keyring can + /// be unlocked on demand (the collections share the session password, so + /// the Secret Service unlocks them without prompting). + struct UnlockingCredentials { + locked: std::sync::atomic::AtomicBool, + lookups: std::sync::atomic::AtomicUsize, + } + + impl CredentialSource for UnlockingCredentials { + fn lookup(&self, _account: &AccountConfig) -> CredentialLookup { + self.lookups + .fetch_add(1, std::sync::atomic::Ordering::SeqCst); + if self.locked.swap(false, std::sync::atomic::Ordering::SeqCst) { + CredentialLookup::Locked + } else { + CredentialLookup::Found("secret".to_string()) + } + } + + fn unlock_keyring(&self) -> bool { + true + } + } + + /// Issue #214: a source that stays locked and cannot unlock anything must + /// be asked exactly once per run (no unlock retry loop). + struct LockedCredentials { + lookups: std::sync::atomic::AtomicUsize, + } + + impl CredentialSource for LockedCredentials { + fn lookup(&self, _account: &AccountConfig) -> CredentialLookup { + self.lookups + .fetch_add(1, std::sync::atomic::Ordering::SeqCst); + CredentialLookup::Locked + } + } + fn account() -> AccountConfig { AccountConfig { id: "test-account".to_string(), @@ -953,6 +1016,65 @@ mod tests { assert!(events.is_empty()); } + /// Issue #214: when the unlock attempt succeeds, the lookup is retried + /// once and the run proceeds with the resolved password (here surfacing + /// as EngineMissing, which is only reachable after a successful lookup). + #[test] + fn locked_lookup_retries_once_after_a_successful_unlock() { + let (progress_tx, progress_rx) = async_channel::unbounded(); + let credentials = Arc::new(UnlockingCredentials { + locked: std::sync::atomic::AtomicBool::new(true), + lookups: std::sync::atomic::AtomicUsize::new(0), + }); + let engine = SyncEngine::new( + account(), + folder(), + NetworkConfig::default(), + None, + Some("/nonexistent/nextcloudcmd".into()), + progress_tx, + ) + .with_credentials(credentials.clone()); + let (outcome, _events) = run_engine(engine, &progress_rx); + assert_eq!(outcome, SyncOutcome::EngineMissing); + assert_eq!( + credentials + .lookups + .load(std::sync::atomic::Ordering::SeqCst), + 2, + "the lookup must be retried once after a successful unlock" + ); + } + + /// Issue #214: a lookup that stays locked and an unlock that cannot help + /// must not loop: one lookup per run, keyring-locked outcome. + #[test] + fn locked_lookup_without_unlock_is_not_retried() { + let (progress_tx, progress_rx) = async_channel::unbounded(); + let credentials = Arc::new(LockedCredentials { + lookups: std::sync::atomic::AtomicUsize::new(0), + }); + let engine = SyncEngine::new( + account(), + folder(), + NetworkConfig::default(), + None, + None, + progress_tx, + ) + .with_credentials(credentials.clone()); + let (outcome, events) = run_engine(engine, &progress_rx); + assert_eq!(outcome, SyncOutcome::KeyringLocked); + assert!(events.is_empty()); + assert_eq!( + credentials + .lookups + .load(std::sync::atomic::Ordering::SeqCst), + 1, + "a failed unlock must not re-run the lookup" + ); + } + #[test] fn unreachable_secret_service_maps_to_keyring_locked() { // Issue #85: at startup the session bus may not be ready yet; that