From 36ebed3a208188dab1b340fb5fc8aef365936d23 Mon Sep 17 00:00:00 2001 From: gnacho Date: Thu, 27 Aug 2026 00:09:14 +0200 Subject: [PATCH] fix(sync): stop requeuing a folder on its own engine-generated feedback (#181) --- src/core/scheduler.rs | 159 +++++++++++++++++++++++++++++++----------- 1 file changed, 119 insertions(+), 40 deletions(-) diff --git a/src/core/scheduler.rs b/src/core/scheduler.rs index 08f42e6..6992e8c 100644 --- a/src/core/scheduler.rs +++ b/src/core/scheduler.rs @@ -141,8 +141,6 @@ struct SchedulerInner { preparing: bool, running: bool, stopped: bool, - 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 @@ -233,8 +231,6 @@ impl Scheduler { preparing: false, running: false, stopped: false, - inotify_during_sync: false, - feedback_followup_pending: false, keyring_locked: false, keyring_retry_count: 0, delete_alert: None, @@ -512,6 +508,18 @@ impl SchedulerInner { if trigger == Trigger::RemoteInterval && self.remote_push_ready { return; } + // Issue #181: local feedback generated by the run itself. The engine + // writes its sync journal (`.sync_*.db`) and materializes downloads + // inside the watched folder, so every run emits `LocalInotify` events + // while it is in flight and, asynchronously, during the post-run + // cooldown. Re-queuing them makes a heavy folder run back-to-back (it + // re-arms itself on every pass). Drop them: only a genuine local + // change arriving after the cooldown re-queues the folder. + if trigger == Trigger::LocalInotify + && (self.running || self.preparing || self.debounce().in_cooldown()) + { + return; + } if self.delete_alert.is_some() && !self.delete_bypass_once { self.queue.add(trigger); let message = self @@ -523,9 +531,6 @@ impl SchedulerInner { return; } if self.running || self.preparing { - if trigger == Trigger::LocalInotify { - self.inotify_during_sync = true; - } self.queue.add(trigger); return; } @@ -835,29 +840,27 @@ impl SchedulerInner { return; } } - let feedback_followup = self.feedback_followup_pending; - self.feedback_followup_pending = false; self.running = true; let weak = self.self_ref.clone(); self.runner.start( &reasons, Box::new(move |outcome| { if let Some(inner) = weak.upgrade() { - inner.borrow_mut().finished(outcome, feedback_followup); + inner.borrow_mut().finished(outcome); } }), ); } - fn finished(&mut self, outcome: SyncOutcome, feedback_followup: bool) { + fn finished(&mut self, outcome: SyncOutcome) { if self.stopped { return; } - // `conflicted` drives the post-run feedback handling: a run that ended - // with conflicted copies must not queue another reconciliation in a - // loop (issue #165); the row already carries the "review the log" - // message and the sync stays parked until the user addresses it. - let (ran, conflicted) = match outcome { + // `conflicted` marks a run that ended with conflicted copies: the row + // carries the "review the log" message and the sync stays parked until + // the user addresses the conflict (issue #165), instead of re-running + // in a loop. + let (ran, _conflicted) = match outcome { SyncOutcome::Success => { self.keyring_locked = false; self.keyring_retry_count = 0; @@ -958,25 +961,14 @@ impl SchedulerInner { flag.set(false); } if ran { - if conflicted { - // Issue #165: a conflicted run must not trigger a follow-up - // reconciliation from local feedback (that is what keeps a - // folder with unresolved conflicts bouncing). Drop any local - // feedback queued during the run and let the interval/remote - // triggers retry on their own cadence. - self.queue.discard(Trigger::LocalInotify); - self.feedback_followup_pending = false; - } else if self.inotify_during_sync { - if feedback_followup { - // Suppress only the local feedback from the reconciliation - // itself; manual/remote triggers stay queued. - self.queue.discard(Trigger::LocalInotify); - } else { - self.feedback_followup_pending = true; - self.queue.add(Trigger::LocalInotify); - } - } - self.inotify_during_sync = false; + // Issue #165 + #181: local feedback generated by the run itself + // (the engine writing its journal or materializing downloads) is + // dropped in `request` while the run is in flight or in cooldown, + // so it never sits in the queue. A conflicted run additionally + // clears any leftover local-feedback reason so a folder with + // unresolved conflicts does not bounce; interval/remote triggers + // retry on their own cadence. + self.queue.discard(Trigger::LocalInotify); self.state.set_progress(None); if let Some(permit) = &self.permit { permit.release(); @@ -1238,7 +1230,6 @@ impl SchedulerInner { self.queue.clear(); self.local_dirty = false; self.remote_pending = false; - self.feedback_followup_pending = false; self.delete_alert = None; if self.running { self.runner.cancel(); @@ -1454,14 +1445,15 @@ mod tests { assert_eq!(source.borrow().pending(), 1); run_idle(&source); assert_eq!(runner.0.borrow().start_calls, 1); - // LOCAL_INOTIFY during the run is coalesced, no new start. + // LOCAL_INOTIFY during the run is the engine's own feedback: dropped + // (issue #181), it must not coalesce into a follow-up run. scheduler.request(Trigger::LocalInotify); assert_eq!(runner.0.borrow().start_calls, 1); - assert_eq!(scheduler.queue_len(), 1); - // The run finishes; the feedback follow-up keeps the queue non-empty. + assert_eq!(scheduler.queue_len(), 0); + // The run finishes; self-generated feedback leaves the queue empty. finish(&runner, SyncOutcome::Success); assert_eq!(scheduler.state().snapshot().state, AppState::IdleOk); - assert_eq!(scheduler.queue_len(), 1); + assert_eq!(scheduler.queue_len(), 0); } /// Issue #165: a run that ends with conflicted copies must not re-queue @@ -1495,6 +1487,93 @@ mod tests { ); } + /// Issue #181: the heavy-folder bounce. The engine writes its sync + /// journal (`.sync_*.db`) and materializes downloads inside the watched + /// folder, so every run emits `LocalInotify` feedback that arrives while + /// the run is in flight and, asynchronously, during the 4s cooldown. + /// Before the fix the mid-run feedback was re-queued as a follow-up and + /// the delayed cooldown events kept the queue non-empty, so + /// `cooldown_finished` started another run immediately (a folder ran + /// back-to-back, 8 engine runs in 27s in the field). With only + /// self-generated feedback the folder must NOT re-run: it stays parked + /// until a genuine trigger (a real local edit after the cooldown, a + /// remote push, an interval) arrives. + #[test] + fn finished_folder_with_only_self_generated_feedback_is_not_requeued() { + let (scheduler, source, runner) = make_scheduler(None); + scheduler.request(Trigger::Manual); + run_idle(&source); + assert_eq!(runner.0.borrow().start_calls, 1); + // The engine writes inside the watched folder while it runs: inotify + // feedback arrives mid-run. + scheduler.request(Trigger::LocalInotify); + // The run ends cleanly; more engine feedback lands during the 4s + // cooldown (the journal flush is asynchronous). + finish(&runner, SyncOutcome::Success); + scheduler.request(Trigger::LocalInotify); + // Both bursts are self-generated feedback: the queue must stay empty. + assert_eq!( + scheduler.queue_len(), + 0, + "self-generated feedback must not re-queue the folder" + ); + // Fire the cooldown; with an empty queue the folder parks idle + // instead of starting another run. + run_idle(&source); + assert_eq!(runner.0.borrow().start_calls, 1); + // A genuine change arriving after the cooldown still reconciles. + scheduler.request(Trigger::LocalInotify); + run_idle(&source); // debounce elapsed -> ready -> idle armed + run_idle(&source); // idle -> start + assert_eq!(runner.0.borrow().start_calls, 2); + } + + /// Issue #181: engine feedback that lands during the post-run cooldown + /// (delayed journal flush) must not re-queue the folder either; only + /// feedback arriving after the cooldown counts as a real local change. + #[test] + fn local_feedback_during_cooldown_is_not_requeued() { + let (scheduler, source, runner) = make_scheduler(None); + scheduler.request(Trigger::Manual); + run_idle(&source); + finish(&runner, SyncOutcome::Success); + // The run ended with an empty queue; the engine's delayed writes land + // during the 4s cooldown. + assert_eq!(scheduler.queue_len(), 0); + scheduler.request(Trigger::LocalInotify); + assert_eq!( + scheduler.queue_len(), + 0, + "cooldown feedback must not re-queue the folder" + ); + run_idle(&source); // cooldown elapses -> idle + assert_eq!(runner.0.borrow().start_calls, 1); + } + + /// Issue #181: a conflicted heavy folder stays parked (issue #165) and + /// does not re-run back-to-back even when the engine keeps writing + /// feedback inside its tree. This is the field case that ran 6 times + /// consecutively: the conflict suppression only cleared the mid-run + /// feedback, but the delayed cooldown events re-armed the queue. + #[test] + fn conflicted_folder_does_not_rerun_on_its_own_feedback() { + let (scheduler, source, runner) = make_scheduler(None); + scheduler.request(Trigger::Manual); + run_idle(&source); + // Mid-run engine feedback (the journal write), like the success case. + scheduler.request(Trigger::LocalInotify); + finish(&runner, SyncOutcome::Conflict); + // Delayed feedback landing during the cooldown. + scheduler.request(Trigger::LocalInotify); + assert_eq!( + scheduler.queue_len(), + 0, + "conflicted folder must not re-queue itself" + ); + run_idle(&source); // cooldown elapses -> idle + assert_eq!(runner.0.borrow().start_calls, 1); + } + #[test] fn stop_removes_pending_sources_and_cancels_process() { let (scheduler, source, runner) = make_scheduler(None);