Fix 11 watcher bugs: event drops, rename mispairs, stale panel state#48
Conversation
There was a problem hiding this comment.
Sorry @leszek3737, you have reached your weekly rate limit of 1500000 diff characters.
Please try again later or upgrade to continue using Sourcery
There was a problem hiding this comment.
Code Review
This pull request significantly improves the filesystem watcher by implementing per-directory rename pairing and handling event queue overflows. It introduces a canonical_path cache and a needs_full_refresh flag to PanelState, standardizing path updates through a new set_path method. The review feedback correctly identifies a potential infinite loop in the error handling logic of full_refresh_panel that could lead to high CPU usage during persistent failures. Additionally, an optimization was suggested to minimize locking overhead when processing stale rename entries in the watcher.
| Err(err) => { | ||
| panel.entries.clear(); | ||
| panel.unfiltered_entries.clear(); | ||
| panel.unfiltered_dirty = true; | ||
| panel.path_index.clear(); | ||
| panel.needs_rebuild = false; | ||
| panel.needs_full_refresh = true; | ||
| panel.last_error = Some(err.to_string()); | ||
| } |
There was a problem hiding this comment.
Setting needs_full_refresh = true when a directory read fails creates an infinite loop of refresh attempts in poll_watcher_events, which is called on every tick of the main loop. This will cause high CPU usage and log spam if a directory becomes permanently inaccessible. It is better to clear the entries and show the error, letting the user manually refresh or waiting for a new filesystem event to trigger a retry.
Err(err) => {
panel.entries.clear();
panel.unfiltered_entries.clear();
panel.unfiltered_dirty = true;
panel.path_index.clear();
panel.needs_rebuild = false;
panel.last_error = Some(err.to_string());
}| send_expired_events(&self.event_tx, &self.debounce_state, flushed); | ||
|
|
||
| let stale_from = { | ||
| let stale_entries: Vec<(PathBuf, PathBuf, Instant)> = { | ||
| let pending = lock_or_recover(&self.pending_from, "pending_from"); | ||
| let pending_time = lock_or_recover(&self.pending_from_time, "pending_from_time"); | ||
| if let (Some(path), Some(time)) = (pending.as_ref(), pending_time.as_ref()) | ||
| && time.elapsed() >= PENDING_FROM_TIMEOUT | ||
| { | ||
| Some((path.clone(), *time)) | ||
| } else { | ||
| None | ||
| } | ||
| pending | ||
| .iter() | ||
| .filter(|(_, entry)| entry.time.elapsed() >= PENDING_FROM_TIMEOUT) | ||
| .map(|(parent, entry)| (parent.clone(), entry.path.clone(), entry.time)) | ||
| .collect() | ||
| }; | ||
|
|
||
| if let Some((path, time)) = stale_from { | ||
| for (parent_key, path, time) in stale_entries { | ||
| debug_log!( | ||
| "stale rename From timed out: emitting Deleted for {}", | ||
| "stale rename From timed out: emitting Deleted for {} (parent {})", | ||
| path.display(), | ||
| parent_key.display(), | ||
| ); | ||
| match try_send_event(&self.event_tx, WatchEvent::Deleted(path.clone())) { | ||
| SendStatus::Full(_) => {} |
There was a problem hiding this comment.
Locking pending_from inside the loop for every stale entry is inefficient. It is better to lock it once outside the loop and perform all removals while holding the lock. Since try_send_event is non-blocking, holding the lock for the duration of the loop is acceptable.
if !stale_entries.is_empty() {
let mut pending = lock_or_recover(&self.pending_from, "pending_from");
for (parent_key, path, time) in stale_entries {
debug_log!(
"stale rename From timed out: emitting Deleted for {} (parent {})",
path.display(),
parent_key.display(),
);
match try_send_event(&self.event_tx, WatchEvent::Deleted(path.clone())) {
SendStatus::Full(_) => {}
SendStatus::Sent | SendStatus::Disconnected => {
if let Some(entry) = pending.get(&parent_key)
&& entry.path == path
&& entry.time == time
{
pending.remove(&parent_key);
}
}
}
}
}…el state Bug fixes: - 2.1: Panel dir delete/rename now triggers full_refresh_panel - 2.2: last_synced only set when all watches succeed - 2.3: Per-parent pending_from prevents cross-dir rename mispairing - 2.4: Overflow marker with retry flag prevents silent event loss - 2.5: full_refresh error clears entries instead of rebuilding stale data - 2.6: unwatch errors propagated instead of silently ignored - 2.7: canonical_path cached on PanelState, set_path() helper - 2.8: NonRecursive watch contract documented - 2.9: pause() clears debounce_state and pending_from - 2.10: Poll limit increased from 64 to 256 - 2.11: 20 new integration tests for watcher sync Additional fixes from review: - Panel recovery after read error via needs_full_refresh flag - Overflow marker retry via overflow_pending AtomicBool - canonical_path refreshed on full_refresh_panel success - all_paths_present prevents stale last_synced on missing paths - 36 test sites migrated from direct path assignment to set_path() - Root deletion edge case test added - last_synced failure path test added - pending_from pause clearing test added
Bug fixes:
Additional fixes from review: