Skip to content

Stop requeuing a folder on its own engine-generated feedback (#181) #191

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
gnacho merged 1 commit into from
Aug 26, 2026
Merged

Stop requeuing a folder on its own engine-generated feedback (#181) #191

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
159 changes: 119 additions & 40 deletions src/core/scheduler.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand All @@ -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;
}
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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);
Expand Down
Loading