fix(sync): recover from a locked keyring without restarting the app (closes #214) (#215)

When the Secret Service restarts (e.g. gnome-keyring-daemon crashing
outside the PAM phase), its collections come back locked and the app
parks in the keyring-locked state. Issue #170's capped retry exhausts
long before the user unlocks the keyring, and afterwards nothing
re-checks the credentials: new triggers either never arrive (push-ready
configs drop the remote interval) or bounce off the parked state, so
only relaunching the app recovered.

Two complementary changes:

- The engine now attempts to unlock the locked collections via the
  Secret Service when a lookup reports Locked, then retries the lookup
  once. GNOME collections created by the desktop session share the
  login password, so the service completes the unlock prompt on its own
  while the client connection stays alive: the common case heals with
  no user interaction.
- Once #170's fast backoff budget is exhausted, the scheduler falls
  back to a slow periodic keyring watch (60 s, uncapped, like the
  server probe of #179) that re-enters the engine until a run resolves
  credentials, so the folder also recovers when the keyring is unlocked
  by any other means. No new visible states or strings.

Co-authored-by: gnacho <hnacho@proton.me>

Author: gnacho

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:

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<bool, CredentialError> {
+        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

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<Mutex<Option<Child>>>,
 ) -> 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