diff --git a/src/core/scheduler.rs b/src/core/scheduler.rs index 118e433..43fa477 100644 --- a/src/core/scheduler.rs +++ b/src/core/scheduler.rs @@ -34,6 +34,14 @@ 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. +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; + /// How a finished reconciliation turned out. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum SyncOutcome { @@ -118,6 +126,11 @@ struct SchedulerInner { inotify_during_sync: bool, feedback_followup_pending: bool, keyring_locked: bool, + /// Issue #170: how many times the keyring retry has run back-to-back. + /// A transient locked service recovers on its own, but a genuinely + /// locked collection must not retry forever: once the budget is spent the + /// folder parks in the keyring-locked state until a manual action. + keyring_retry_count: u32, delete_alert: Option, delete_bypass_once: bool, /// Whether a synchronization has ever completed successfully. Drives the @@ -186,6 +199,7 @@ impl Scheduler { inotify_during_sync: false, feedback_followup_pending: false, keyring_locked: false, + keyring_retry_count: 0, delete_alert: None, delete_bypass_once: false, ever_synced: false, @@ -515,6 +529,36 @@ impl SchedulerInner { self.start_source = Some(id); } + /// 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 + /// visible state, so the informative keyring-locked label is preserved + /// while it re-attempts. + fn schedule_keyring_retry(&mut self) { + if self.stopped + || self.start_source.is_some() + || self.preparing + || self.running + || self.keyring_retry_count >= KEYRING_RETRY_MAX + { + 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); + } + fn start(&mut self) { self.start_source = None; if self.stopped || self.preparing || self.running || self.queue.is_empty() || !self.online { @@ -611,13 +655,20 @@ impl SchedulerInner { }; if !acquired { self.queue.extend(reasons.iter().copied()); - // Issue #165: name the folder the queue is waiting on (when it - // is a tracked holder), instead of the generic "another - // account" wording. Falls back to the generic message when no - // tracked folder is available. - let message = match permit.active_folder_name() { - Some(name) => t("Waiting for {folder} to finish…").replace("{folder}", &name), - None => t("Waiting for another folder to finish…").to_string(), + // Issue #165/#169: name the folder the queue is waiting on only + // when this scheduler is the next in line (no waiters ahead of + // it). Behind it the message stays generic, so a run of many + // queued folders does not repeat the same folder name on every + // row and does not name one that may already be done. + let message = if permit.waiter_count() == 0 { + match permit.active_folder_name() { + Some(name) => { + t("Waiting for {folder} to finish…").replace("{folder}", &name) + } + None => t("Waiting for another folder to finish…").to_string(), + } + } else { + t("Waiting for another folder to finish…").to_string() }; self.state.set(AppState::SyncQueued, message); let source = self.source.clone(); @@ -660,6 +711,7 @@ impl SchedulerInner { let (ran, conflicted) = match outcome { SyncOutcome::Success => { self.keyring_locked = false; + self.keyring_retry_count = 0; self.auth_required = false; self.ever_synced = true; self.set_idle_state(); @@ -667,6 +719,7 @@ impl SchedulerInner { } SyncOutcome::Conflict => { self.keyring_locked = false; + self.keyring_retry_count = 0; self.auth_required = false; self.ever_synced = true; self.state.set( @@ -778,6 +831,17 @@ impl SchedulerInner { if let Some(permit) = &self.permit { permit.release(); } + // Issue #170: a keyring-locked outcome is often transient — the + // Secret Service may not be up right after login. Rather than + // waiting for the next external trigger (which may never come + // within the session), schedule a capped backoff retry so the next + // run re-reads the credentials and clears the flag once the + // service is ready. The retry does not touch the visible + // keyring-locked state. + if matches!(outcome, SyncOutcome::KeyringLocked) { + self.queue.add(Trigger::Retry); + self.schedule_keyring_retry(); + } } } @@ -1278,18 +1342,70 @@ mod tests { assert!(scheduler.keyring_locked()); assert_eq!(scheduler.state().snapshot().state, AppState::KeyringLocked); - // Issue #85: an automatic trigger retries the run instead of parking - // the folder in needs-attention; the keyring may have come up in the - // meantime and the lookup is cheap. + // An automatic trigger queues up (the keyring retry is already + // pending from the locked outcome), so it does not start a second, + // parallel run; the existing retry still re-enters the engine and + // recovers once the service is ready. scheduler.request(Trigger::LocalInotify); - assert_eq!(scheduler.queue_len(), 1); + // The keyring retry (queued from the locked outcome) plus this trigger. + assert_eq!(scheduler.queue_len(), 2); + let retry_id = source.borrow().only_id(); + fire_timer(&source, retry_id); + assert_eq!(runner.0.borrow().start_calls, 2); + finish(&runner, SyncOutcome::Success); + assert!(!scheduler.keyring_locked()); + assert_eq!(scheduler.state().snapshot().state, AppState::IdleOk); + } + + /// Issue #170: a keyring-locked outcome schedules its own backoff retry, + /// so the folder recovers as soon as the Secret Service is ready even if + /// no external trigger arrives later in the session. + #[test] + fn keyring_locked_schedules_its_own_retry() { + 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::KeyringLocked); + assert!(scheduler.keyring_locked()); + assert_eq!(scheduler.state().snapshot().state, AppState::KeyringLocked); + + // A backoff retry is already pending without any new external trigger: + // firing it re-enters the engine (the visible state stays keyring + // locked while it re-attempts). + assert_eq!(source.borrow().pending(), 1); + let retry_id = source.borrow().only_id(); + fire_timer(&source, retry_id); assert_eq!(runner.0.borrow().start_calls, 2); finish(&runner, SyncOutcome::Success); assert!(!scheduler.keyring_locked()); 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. + #[test] + fn keyring_retry_budget_is_capped() { + 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 { + 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!(scheduler.keyring_locked()); + } + #[test] fn shared_permit_queues_a_second_account_until_release() { let permit = SyncPermit::try_new(1).unwrap(); @@ -1312,6 +1428,45 @@ mod tests { assert_eq!(runner_second.0.borrow().start_calls, 1); } + /// Issue #169: only the head of the wait queue names the folder it is + /// waiting on; the schedulers behind it use the generic wording instead + /// of repeating the same folder name / naming one that may already finish. + #[test] + fn queued_schedulers_only_name_the_holder_at_the_head() { + let permit = SyncPermit::try_new(1).unwrap(); + let root = tempfile::tempdir().expect("tempdir"); + let (first, source_first, runner_first) = make_scheduler(Some(permit.clone())); + first.set_local_root(Some(std::path::PathBuf::from(root.path()))); + let (second, source_second, _runner_second) = make_scheduler(Some(permit.clone())); + second.set_local_root(Some(root.path().join("a"))); + let (third, source_third, _runner_third) = make_scheduler(Some(permit.clone())); + third.set_local_root(Some(root.path().join("b"))); + + // The holder runs; two schedulers queue behind it. + first.request(Trigger::Manual); + run_idle(&source_first); + assert_eq!(runner_first.0.borrow().start_calls, 1); + assert!(permit.active_folder_name().is_some()); + + second.request(Trigger::Manual); + run_idle(&source_second); + // Only one waiter so far: it is the head and names the folder. + let expected = format!( + "Waiting for {} to finish…", + root.path().file_name().unwrap().to_string_lossy() + ); + assert_eq!(second.state().snapshot().message, expected); + + third.request(Trigger::Manual); + run_idle(&source_third); + // The third is behind the second: it uses the generic wording, not the + // same folder name as the head. + assert_eq!( + third.state().snapshot().message, + "Waiting for another folder to finish…" + ); + } + /// Issue #154: the startup backlog runs strictly one folder at a time in /// the exact order the startup triggers were requested. Every scheduler /// defers its start through one idle hop (dispatched FIFO, like the GLib diff --git a/src/core/sync_permit.rs b/src/core/sync_permit.rs index acc1168..21b4ebf 100644 --- a/src/core/sync_permit.rs +++ b/src/core/sync_permit.rs @@ -155,6 +155,14 @@ impl SyncPermit { .map(str::to_owned) } + /// How many schedulers are currently waiting for a release (issue #169). + /// A scheduler with zero waiters ahead of it is the head of the queue and + /// can name the folder it is waiting on; anyone behind it should use the + /// generic wording instead of repeating the same folder name. + pub fn waiter_count(&self) -> usize { + self.inner.borrow().waiters.len() + } + /// Release one slot and wake the oldest waiter, if any. /// /// The woken callback runs synchronously (Python semantics); it must not