From 22911f625dfd0dd0b41e81ed50ef1a7d6b0c0a98 Mon Sep 17 00:00:00 2001 From: Dmitriy Kovalenko Date: Wed, 1 Jul 2026 14:52:46 -0700 Subject: [PATCH 1/2] fix: Fuzz test branches --- Makefile | 2 +- crates/fff-core/src/background_watcher.rs | 120 ++++----- crates/fff-core/src/git_status_worker.rs | 119 +++++++++ crates/fff-core/src/lib.rs | 1 + crates/fff-core/src/scan.rs | 28 ++- crates/fff-core/src/shared.rs | 61 ++++- ...zz_git_watcher_stress.proptest-regressions | 7 + .../fff-core/tests/fuzz_git_watcher_stress.rs | 229 ++++++++++++++++++ crates/fff-core/tests/real_binary_fixtures.rs | 63 ++++- 9 files changed, 541 insertions(+), 89 deletions(-) create mode 100644 crates/fff-core/src/git_status_worker.rs create mode 100644 crates/fff-core/tests/fuzz_git_watcher_stress.proptest-regressions diff --git a/Makefile b/Makefile index 7d5d49802..5b74a7465 100644 --- a/Makefile +++ b/Makefile @@ -236,7 +236,7 @@ test-stress-random: -p fff-search \ --test fuzz_git_watcher_stress \ --no-default-features --features zlob \ - -- --nocapture stress_random + -- --nocapture stress_random stress_regresion test-stress-repos: RUSTFLAGS="$(STRESS_RUSTFLAGS)" \ diff --git a/crates/fff-core/src/background_watcher.rs b/crates/fff-core/src/background_watcher.rs index f79e7115a..1035c88e6 100644 --- a/crates/fff-core/src/background_watcher.rs +++ b/crates/fff-core/src/background_watcher.rs @@ -1,7 +1,7 @@ use crate::constants::MAX_OVERFLOW_FILES; use crate::error::Error; use crate::file_picker::FFFMode; -use crate::git::GitStatusCache; +use crate::git_status_worker::{GitStatusMailbox, spawn_consumer}; use crate::shared::{SharedFilePicker, SharedFrecency}; use crate::sort_buffer::sort_with_buffer; use git2::Repository; @@ -23,6 +23,8 @@ pub struct BackgroundWatcher { debouncer: Arc>>, watch_tx: Option>, owner_thread: Option>, + git_status_mailbox: Arc, + git_status_thread: Option>, } const DEBOUNCE_TIMEOUT: Duration = Duration::from_millis(50); @@ -82,9 +84,20 @@ impl BackgroundWatcher { let (watch_tx, watch_rx) = mpsc::channel::(); let watch_tx_for_debouncer = watch_tx.clone(); + // Single-consumer git-status pipeline. Producers (debouncer + owner + // thread) only enqueue; this dedicated consumer applies updates in + // order, so a stale snapshot can never clobber a fresh one. + let git_status_mailbox = GitStatusMailbox::new(); + let git_status_thread = spawn_consumer( + shared_picker.weaken(), + shared_frecency.clone(), + Arc::clone(&git_status_mailbox), + trace_span.clone(), + ); + let owner_weak_picker = shared_picker.weaken(); - let owner_frecency = shared_frecency.clone(); let owner_git_workdir = git_workdir.clone(); + let owner_mailbox = Arc::clone(&git_status_mailbox); let debouncer = Self::create_debouncer( base_path, @@ -94,6 +107,7 @@ impl BackgroundWatcher { mode, use_recursive, watch_tx_for_debouncer, + Arc::clone(&git_status_mailbox), )?; info!("Background file watcher initialized successfully"); @@ -146,8 +160,8 @@ impl BackgroundWatcher { track_files_from_new_directories( &dir, &strong_picker, - &owner_frecency, &owner_git_workdir, + &owner_mailbox, ); // Transient strong ref drops here, back @@ -162,9 +176,12 @@ impl BackgroundWatcher { debouncer, watch_tx: Some(watch_tx), owner_thread: Some(owner_thread), + git_status_mailbox, + git_status_thread: Some(git_status_thread), }) } + #[allow(clippy::too_many_arguments)] fn create_debouncer( base_path: PathBuf, git_workdir: Option, @@ -173,6 +190,7 @@ impl BackgroundWatcher { mode: FFFMode, use_recursive: bool, watch_tx: mpsc::Sender, + git_status_mailbox: Arc, ) -> Result { let config = Config::default() // do not follow symlinks as then notifiers spawns a bunch of events for symlinked @@ -217,6 +235,7 @@ impl BackgroundWatcher { &strong_picker, &shared_frecency, mode, + &git_status_mailbox, ); // every new directory creates had to be reflected in the picker state @@ -363,6 +382,13 @@ impl BackgroundWatcher { self.owner_thread.take(); + // Wake the git-status consumer so it exits on its next loop, then + // detach it. Never joined here for the same reason the owner thread + // isn't: the consumer takes `SharedFilePicker::write()`, and a caller + // holding that lock would deadlock on a blocking join. + self.git_status_mailbox.signal_shutdown(); + self.git_status_thread.take(); + info!("Background file watcher stop signaled"); } @@ -388,7 +414,7 @@ impl Drop for BackgroundWatcher { } } -#[tracing::instrument(name = "fs_events", skip(events, shared_picker, shared_frecency), level = Level::DEBUG)] +#[tracing::instrument(name = "fs_events", skip(events, shared_picker, shared_frecency, git_status_mailbox), level = Level::DEBUG)] fn handle_debounced_events( events: Vec, base_path: &Path, @@ -396,6 +422,7 @@ fn handle_debounced_events( shared_picker: &SharedFilePicker, shared_frecency: &SharedFrecency, mode: FFFMode, + git_status_mailbox: &Arc, ) -> Vec { // this will be called very often, we have to minimiy the lock time for file picker let repo = git_workdir.as_ref().and_then(|p| Repository::open(p).ok()); @@ -659,54 +686,18 @@ fn handle_debounced_events( } } - // do not try to update the paths if we anyway going to rescan everything from scratch - if !need_full_rescan && (need_full_git_rescan || !files_to_update_git_status.is_empty()) { - let git_workdir = repo - .as_ref() - .map(|r| r.workdir().unwrap_or_else(|| r.path()).to_path_buf()); - - let shared_picker = shared_picker.clone(); - let shared_frecency = shared_frecency.clone(); - - // git status query even with a pathspec could be really slow, if we do this syncrhronously - // within the event handler, we actually risk of forming a snow ball of conflicting events - crate::parallelism::BACKGROUND_THREAD_POOL.spawn(move || { - let Some(git_path) = git_workdir else { return }; - let Ok(repo) = Repository::open(&git_path) else { - error!("Failed to open git repo for async status update"); - return; - }; - - if need_full_git_rescan && !need_full_rescan { - info!("Async: triggering full git rescan"); - if let Err(e) = shared_picker.refresh_git_status(&shared_frecency) { - error!("Failed to refresh git status: {:?}", e); - } - } - - if !files_to_update_git_status.is_empty() { - let status = match GitStatusCache::git_status_for_paths( - &repo, - &files_to_update_git_status, - ) { - Ok(s) => s, - Err(e) => { - error!("Failed to query git status: {:?}", e); - return; - } - }; - - if let Ok(mut guard) = shared_picker.write() - && let Some(ref mut picker) = *guard - { - if let Err(e) = picker.update_git_statuses(status, &shared_frecency) { - error!("Failed to update git statuses: {:?}", e); - } else { - info!("Async: git statuses updated"); - } - } - } - }); + // Enqueue git-status work onto the dedicated consumer instead of doing it + // here: the debouncer thread must never block on a (potentially slow) git + // read, and the single consumer keeps updates ordered. Skip entirely when a + // full filesystem rescan is queued — that path recomputes git status itself. + if !need_full_rescan { + if need_full_git_rescan { + // A full git rescan re-reads every tracked path (including ones that + // just went clean after a commit), so it supersedes per-path work. + git_status_mailbox.request_full_rescan(); + } else if !files_to_update_git_status.is_empty() { + git_status_mailbox.enqueue_paths(files_to_update_git_status); + } } new_dirs_to_watch @@ -717,8 +708,8 @@ fn handle_debounced_events( fn track_files_from_new_directories( dir: &Path, shared_picker: &SharedFilePicker, - shared_frecency: &SharedFrecency, git_workdir: &Option, + git_status_mailbox: &Arc, ) { let Ok(entries) = std::fs::read_dir(dir) else { return; @@ -750,6 +741,8 @@ fn track_files_from_new_directories( return; } + let added = files_to_add.len(); + { let Ok(mut guard) = shared_picker.write() else { return; @@ -764,26 +757,15 @@ fn track_files_from_new_directories( } } - if let Some(repo) = repo.as_ref() { - let status = match GitStatusCache::git_status_for_paths(repo, &files_to_add) { - Ok(status) => status, - Err(e) => { - tracing::error!(?e, "inject_existing_files: git status query failed"); - return; - } - }; - - if let Ok(mut guard) = shared_picker.write() - && let Some(ref mut picker) = *guard - && let Err(e) = picker.update_git_statuses(status, shared_frecency) - { - error!("inject_existing_files: failed to update git statuses: {e:?}"); - } + // Hand the git-status refresh to the dedicated consumer (ordered + off the + // owner thread). It no-ops when there is no git repo. + if repo.is_some() { + git_status_mailbox.enqueue_paths(files_to_add); } debug!( "Injected {} existing files from new directory {}", - files_to_add.len(), + added, dir.display(), ); } diff --git a/crates/fff-core/src/git_status_worker.rs b/crates/fff-core/src/git_status_worker.rs new file mode 100644 index 000000000..380301d08 --- /dev/null +++ b/crates/fff-core/src/git_status_worker.rs @@ -0,0 +1,119 @@ +use crate::shared::{SharedFrecency, WeakFilePicker}; +use ahash::AHashSet; +use parking_lot::{Condvar, Mutex}; +use std::path::PathBuf; +use std::sync::Arc; +use std::thread::JoinHandle; +use tracing::{debug, error, info}; + +#[derive(Default)] +struct Pending { + paths: AHashSet, + full_rescan: bool, + shutdown: bool, +} + +impl Pending { + fn has_work(&self) -> bool { + self.full_rescan || !self.paths.is_empty() + } +} + +/// Single-consumer coalescing queue for git-status updates. Producers (the +/// debouncer + owner threads) push cheaply and return; a dedicated consumer +/// drains the whole accumulated batch at once. A single ordered consumer means +/// a stale pre-commit snapshot can never be applied after a fresh post-commit +/// one — the race the watcher fuzz test caught. +pub(crate) struct GitStatusMailbox { + state: Mutex, + cv: Condvar, +} + +impl GitStatusMailbox { + pub(crate) fn new() -> Arc { + Arc::new(Self { + state: Mutex::new(Pending::default()), + cv: Condvar::new(), + }) + } + + /// Accumulate paths that need a targeted git-status refresh. Cheap: a lock, + /// a set-extend, and a single wakeup. Never blocks on git. + pub(crate) fn enqueue_paths(&self, paths: I) + where + I: IntoIterator, + { + let mut guard = self.state.lock(); + guard.paths.extend(paths); + drop(guard); + self.cv.notify_one(); + } + + /// Request a full git rescan. Supersedes any queued per-path work — a full + /// rescan re-reads every tracked path, so the drained paths are irrelevant. + pub(crate) fn request_full_rescan(&self) { + let mut guard = self.state.lock(); + guard.full_rescan = true; + drop(guard); + self.cv.notify_one(); + } + + pub(crate) fn signal_shutdown(&self) { + let mut guard = self.state.lock(); + guard.shutdown = true; + drop(guard); + self.cv.notify_one(); + } + + /// Block until there is work or shutdown, then drain the whole batch. + /// Returns `None` once shutdown was requested (consumer should exit). + fn wait_and_take(&self) -> Option { + let mut guard = self.state.lock(); + while !guard.shutdown && !guard.has_work() { + self.cv.wait(&mut guard); + } + if guard.shutdown { + return None; + } + Some(std::mem::take(&mut *guard)) + } +} + +/// Spawn the dedicated git-status consumer. It sleeps on the condvar while +/// idle (no polling), wakes to drain the accumulated batch, and applies it via +/// the serialized `SharedFilePicker` methods. Detached on teardown via +/// `signal_shutdown`; it also exits if the picker has been dropped. +pub(crate) fn spawn_consumer( + weak_picker: WeakFilePicker, + frecency: SharedFrecency, + mailbox: Arc, + span: tracing::Span, +) -> JoinHandle<()> { + std::thread::Builder::new() + .name("fff-git-status".into()) + .spawn(move || { + let _g = span.enter(); + while let Some(work) = mailbox.wait_and_take() { + // Upgrade only for one batch; if the picker is gone teardown is + // underway, so exit cleanly. + let Some(picker) = weak_picker.upgrade() else { + break; + }; + + if work.full_rescan { + debug!("git-status worker: full rescan"); + if let Err(e) = picker.refresh_git_status(&frecency) { + error!("git-status worker: full rescan failed: {e:?}"); + } + } else if !work.paths.is_empty() { + let paths: Vec = work.paths.into_iter().collect(); + if let Err(e) = picker.update_git_status_for_paths(&paths, &frecency) { + error!("git-status worker: path update failed: {e:?}"); + } + } + } + + info!("git-status worker stopped"); + }) + .expect("failed to spawn fff-git-status thread") +} diff --git a/crates/fff-core/src/lib.rs b/crates/fff-core/src/lib.rs index e2b26b355..b4e390a50 100644 --- a/crates/fff-core/src/lib.rs +++ b/crates/fff-core/src/lib.rs @@ -95,6 +95,7 @@ //! ``` mod background_watcher; +mod git_status_worker; pub(crate) mod parallelism; mod scan; // public only for benchmarks — the inverted index is still re-exported via diff --git a/crates/fff-core/src/scan.rs b/crates/fff-core/src/scan.rs index 729b0fcaa..54424593f 100644 --- a/crates/fff-core/src/scan.rs +++ b/crates/fff-core/src/scan.rs @@ -208,17 +208,13 @@ impl ScanJob { return; } - // Files are now searchable — flip the scan signal *early* so - // UI progress polls see the picker as "ready" while we run the - // optional post-scan steps in the background. - signals.scanning.store(false, Ordering::Relaxed); - - // in case we do a rescan, we have to resubscribe a watcher to the new set of directories - // all the already watched directories are not going to be resubscribed - if !config.install_watcher && !signals.cancelled.load(Ordering::Acquire) { - rescubscribe_watcher_post_scan(&shared_picker); - } - + // Acquire the post-scan snapshot BEFORE clearing `scanning`. Taking the + // snapshot sets `post_scan_indexing_active`, so at least one readiness + // signal stays set continuously from scan start until post-scan work + // completes. Clearing `scanning` first opened a window where both + // signals read false and `wait_for_indexing_complete` could return + // before the binary sniff / bigram build ran — a Windows-CI flake in + // `real_binary_fixtures`. let mut snapshot = if !signals.cancelled.load(Ordering::Acquire) { shared_picker.read().ok().and_then(|guard| { guard @@ -229,6 +225,16 @@ impl ScanJob { None }; + // Files are now searchable — flip the scan signal so UI progress polls + // see the picker as "ready" while we run the post-scan steps. + signals.scanning.store(false, Ordering::Relaxed); + + // in case we do a rescan, we have to resubscribe a watcher to the new set of directories + // all the already watched directories are not going to be resubscribed + if !config.install_watcher && !signals.cancelled.load(Ordering::Acquire) { + rescubscribe_watcher_post_scan(&shared_picker); + } + // 3. Post-scan warmup + bigram build — runs in parallel with the // git-status thread to overlap the two expensive phases. // Always runs (even with both flags off) so binary-content files diff --git a/crates/fff-core/src/shared.rs b/crates/fff-core/src/shared.rs index 9da70ab8f..632ecc840 100644 --- a/crates/fff-core/src/shared.rs +++ b/crates/fff-core/src/shared.rs @@ -9,6 +9,7 @@ use crate::frecency::FrecencyTracker; use crate::git::GitStatusCache; use crate::query_tracker::QueryTracker; use crate::scan::ScanJob; +use git2::Repository; /// Poll `.git/index.lock` until it disappears (git write completed), giving up /// after [`GIT_LOCK_MAX_WAIT`]. Used by [`SharedPicker::refresh_git_status`] @@ -72,12 +73,18 @@ pub struct SharedFilePicker(pub(crate) Arc); pub struct SharedPickerInner { picker: parking_lot::RwLock>, + // Serializes every git-status read+apply (full rescans and per-path + // updates alike) so the two steps stay atomic as a unit. Without it a + // snapshot read from the pre-commit index can be applied *after* a + // post-commit one, leaving stale INDEX_* bits that never converge. + git_status_update: parking_lot::Mutex<()>, } impl Default for SharedPickerInner { fn default() -> Self { Self { picker: parking_lot::RwLock::new(None), + git_status_update: parking_lot::Mutex::new(()), } } } @@ -220,16 +227,19 @@ impl SharedFilePicker { pub fn refresh_git_status(&self, shared_frecency: &SharedFrecency) -> Result { use tracing::debug; + // Hold the serialization lock across the whole read+apply so no other + // status updater can interleave and clobber us with a stale snapshot. + let _serialize = self.0.git_status_update.lock(); + let git_status = { - let guard = self.read()?; - let Some(ref picker) = *guard else { - return Err(Error::FilePickerMissing); + let git_root = { + let guard = self.read()?; + let Some(ref picker) = *guard else { + return Err(Error::FilePickerMissing); + }; + picker.git_root().map(|p| p.to_path_buf()) }; - let git_root = picker.git_root().map(|p| p.to_path_buf()); - drop(guard); // updating git status could take very long time, there is not risky as we - // do not allow any mutations and deletions of files from the sync - debug!(?git_root, "Refreshing git status for picker"); if let Some(ref root) = git_root { @@ -255,6 +265,43 @@ impl SharedFilePicker { Ok(statuses_count) } + + /// Recompute and apply git status for a specific set of paths. + /// + /// Serialized against [`Self::refresh_git_status`] via the same lock and + /// waits for any in-flight `.git/index.lock` first, so it never reads a + /// half-written index mid-commit and never races a full rescan. + pub fn update_git_status_for_paths( + &self, + paths: &[PathBuf], + shared_frecency: &SharedFrecency, + ) -> Result<(), Error> { + if paths.is_empty() { + return Ok(()); + } + + let _serialize = self.0.git_status_update.lock(); + + let git_root = { + let guard = self.read()?; + let Some(ref picker) = *guard else { + return Err(Error::FilePickerMissing); + }; + picker.git_root().map(|p| p.to_path_buf()) + }; + let Some(git_root) = git_root else { + return Ok(()); + }; + + wait_for_git_index_lock_release(&git_root); + + let repo = Repository::open(&git_root)?; + let status = GitStatusCache::git_status_for_paths(&repo, paths)?; + + let mut guard = self.write()?; + let picker = guard.as_mut().ok_or(Error::FilePickerMissing)?; + picker.update_git_statuses(status, shared_frecency) + } } /// Thread-safe shared handle to an LMDB-backed store. A disabled (`noop`) diff --git a/crates/fff-core/tests/fuzz_git_watcher_stress.proptest-regressions b/crates/fff-core/tests/fuzz_git_watcher_stress.proptest-regressions new file mode 100644 index 000000000..617fd592a --- /dev/null +++ b/crates/fff-core/tests/fuzz_git_watcher_stress.proptest-regressions @@ -0,0 +1,7 @@ +# Seeds for failure cases proptest has generated in the past. It is +# automatically read and these particular cases re-run before any +# novel cases are generated. +# +# It is recommended to check this file in to source control so that +# everyone who runs the test benefits from these saved cases. +cc 2c9d1ea2efbf6161f84b69598e884dbf1bde6039c70625adde0374817e20e2ea # shrinks to ops = [Touch { idx: 11805740887947177360 }, EditFile { idx: 16172331740552606779, content_seed: 658362763 }, EditFile { idx: 15583428591709383853, content_seed: 959932347 }, CreateFile { seed: 3931677194, content_seed: 3741630760 }, RenameFile { idx: 10904465323748885299, new_seed: 2279324670 }, EditFile { idx: 9420001873471413265, content_seed: 2114867802 }, EditFile { idx: 213141289566213885, content_seed: 560732827 }, EditFile { idx: 17035572278797292009, content_seed: 3808313786 }, CreateFile { seed: 1288265487, content_seed: 1533838042 }, EditFile { idx: 16473729680435813293, content_seed: 1008536355 }, Noop, EditFile { idx: 5398337378624802944, content_seed: 1867267587 }, EditFile { idx: 8551649030560368528, content_seed: 2795207371 }, EditFile { idx: 481097026290699171, content_seed: 2327194851 }, EditFile { idx: 16127721863741462430, content_seed: 259019344 }, EditFile { idx: 12499722135620715652, content_seed: 710330684 }, Noop, EditFile { idx: 8570312264332927846, content_seed: 20660474 }, EditFile { idx: 8235918596759853707, content_seed: 3895597068 }, EditFile { idx: 12941454977835821598, content_seed: 974440776 }, CreateFile { seed: 2370169947, content_seed: 3173097146 }, EditFile { idx: 17976810772660212547, content_seed: 1324367109 }, EditFile { idx: 9502968218335991677, content_seed: 4023431064 }, EditFile { idx: 6867642529007482379, content_seed: 207981211 }, CreateFile { seed: 570877898, content_seed: 1583247379 }, EditFile { idx: 7954117886944662117, content_seed: 1193244670 }, EditFile { idx: 2859309324154804229, content_seed: 669423541 }, EditFile { idx: 1701666328639270148, content_seed: 3533870941 }, EditFile { idx: 9409088379085591352, content_seed: 132132514 }, CreateSubdirFile { dir_seed: 16362, file_seed: 52030, content_seed: 1374259669 }, EditFile { idx: 13262913709196533433, content_seed: 2320968118 }, EditFile { idx: 17763858399044545684, content_seed: 356795223 }, GitAddAll, GitAddAll, EditFile { idx: 15775096968456010165, content_seed: 1772666100 }, CreateFile { seed: 2452057527, content_seed: 4111477598 }, EditFile { idx: 10711976377011651566, content_seed: 3912798245 }, EditFile { idx: 9546828938565340524, content_seed: 172130614 }, EditFile { idx: 13343766455516594408, content_seed: 4263257980 }, EditFile { idx: 3019554710255922240, content_seed: 2030698787 }, EditFile { idx: 10405611055126420472, content_seed: 1193589142 }, EditFile { idx: 17138669581083326405, content_seed: 2903728507 }, GitignoreAppend { pattern_seed: 54248 }, EditFile { idx: 13072623131577163943, content_seed: 1483932899 }, DeleteFile { idx: 12869318557852831815 }, EditFile { idx: 6824133502332012157, content_seed: 3150001556 }, EditFile { idx: 14438183751086260985, content_seed: 3158759791 }, EditFile { idx: 3246005478687480560, content_seed: 1237031794 }, EditFile { idx: 18419405778222487428, content_seed: 564314178 }, DeleteFile { idx: 5831672858052749534 }, GitCommit { msg_seed: 6624 }, RenameFile { idx: 9595327710957667341, new_seed: 3574446333 }, EditFile { idx: 11366319046220637326, content_seed: 3919865486 }, CreateFile { seed: 906013144, content_seed: 1799214899 }, EditFile { idx: 6771264237902804639, content_seed: 2901771864 }, EditFile { idx: 5877801695508394108, content_seed: 3324479019 }, CreateSubdirFile { dir_seed: 47588, file_seed: 6835, content_seed: 4279222285 }, GitignoreAppend { pattern_seed: 41743 }, GitCommit { msg_seed: 63391 }] diff --git a/crates/fff-core/tests/fuzz_git_watcher_stress.rs b/crates/fff-core/tests/fuzz_git_watcher_stress.rs index f7ca1e809..6d5075771 100644 --- a/crates/fff-core/tests/fuzz_git_watcher_stress.rs +++ b/crates/fff-core/tests/fuzz_git_watcher_stress.rs @@ -285,6 +285,235 @@ fn stress_seeded() { } } +/// Pinned deterministic regression for the git-status divergence found on +/// Windows CI (run 28264744320): after a `GitCommit` the picker retained stale +/// `INDEX_*` bits because a pre-commit per-path status snapshot was applied +/// after the post-commit full rescan. Unlike `stress_random`, this replays the +/// exact operation sequence directly (independent of proptest RNG/strategy), +/// so the scenario always runs and guards the fix. +#[test] +fn stress_regression_stale_index_after_commit() { + use AbstractOp::*; + let ops: Vec = vec![ + Touch { + idx: 11805740887947177360, + }, + EditFile { + idx: 16172331740552606779, + content_seed: 658362763, + }, + EditFile { + idx: 15583428591709383853, + content_seed: 959932347, + }, + CreateFile { + seed: 3931677194, + content_seed: 3741630760, + }, + RenameFile { + idx: 10904465323748885299, + new_seed: 2279324670, + }, + EditFile { + idx: 9420001873471413265, + content_seed: 2114867802, + }, + EditFile { + idx: 213141289566213885, + content_seed: 560732827, + }, + EditFile { + idx: 17035572278797292009, + content_seed: 3808313786, + }, + CreateFile { + seed: 1288265487, + content_seed: 1533838042, + }, + EditFile { + idx: 16473729680435813293, + content_seed: 1008536355, + }, + Noop, + EditFile { + idx: 5398337378624802944, + content_seed: 1867267587, + }, + EditFile { + idx: 8551649030560368528, + content_seed: 2795207371, + }, + EditFile { + idx: 481097026290699171, + content_seed: 2327194851, + }, + EditFile { + idx: 16127721863741462430, + content_seed: 259019344, + }, + EditFile { + idx: 12499722135620715652, + content_seed: 710330684, + }, + Noop, + EditFile { + idx: 8570312264332927846, + content_seed: 20660474, + }, + EditFile { + idx: 8235918596759853707, + content_seed: 3895597068, + }, + EditFile { + idx: 12941454977835821598, + content_seed: 974440776, + }, + CreateFile { + seed: 2370169947, + content_seed: 3173097146, + }, + EditFile { + idx: 17976810772660212547, + content_seed: 1324367109, + }, + EditFile { + idx: 9502968218335991677, + content_seed: 4023431064, + }, + EditFile { + idx: 6867642529007482379, + content_seed: 207981211, + }, + CreateFile { + seed: 570877898, + content_seed: 1583247379, + }, + EditFile { + idx: 7954117886944662117, + content_seed: 1193244670, + }, + EditFile { + idx: 2859309324154804229, + content_seed: 669423541, + }, + EditFile { + idx: 1701666328639270148, + content_seed: 3533870941, + }, + EditFile { + idx: 9409088379085591352, + content_seed: 132132514, + }, + CreateSubdirFile { + dir_seed: 16362, + file_seed: 52030, + content_seed: 1374259669, + }, + EditFile { + idx: 13262913709196533433, + content_seed: 2320968118, + }, + EditFile { + idx: 17763858399044545684, + content_seed: 356795223, + }, + GitAddAll, + GitAddAll, + EditFile { + idx: 15775096968456010165, + content_seed: 1772666100, + }, + CreateFile { + seed: 2452057527, + content_seed: 4111477598, + }, + EditFile { + idx: 10711976377011651566, + content_seed: 3912798245, + }, + EditFile { + idx: 9546828938565340524, + content_seed: 172130614, + }, + EditFile { + idx: 13343766455516594408, + content_seed: 4263257980, + }, + EditFile { + idx: 3019554710255922240, + content_seed: 2030698787, + }, + EditFile { + idx: 10405611055126420472, + content_seed: 1193589142, + }, + EditFile { + idx: 17138669581083326405, + content_seed: 2903728507, + }, + GitignoreAppend { + pattern_seed: 54248, + }, + EditFile { + idx: 13072623131577163943, + content_seed: 1483932899, + }, + DeleteFile { + idx: 12869318557852831815, + }, + EditFile { + idx: 6824133502332012157, + content_seed: 3150001556, + }, + EditFile { + idx: 14438183751086260985, + content_seed: 3158759791, + }, + EditFile { + idx: 3246005478687480560, + content_seed: 1237031794, + }, + EditFile { + idx: 18419405778222487428, + content_seed: 564314178, + }, + DeleteFile { + idx: 5831672858052749534, + }, + GitCommit { msg_seed: 6624 }, + RenameFile { + idx: 9595327710957667341, + new_seed: 3574446333, + }, + EditFile { + idx: 11366319046220637326, + content_seed: 3919865486, + }, + CreateFile { + seed: 906013144, + content_seed: 1799214899, + }, + EditFile { + idx: 6771264237902804639, + content_seed: 2901771864, + }, + EditFile { + idx: 5877801695508394108, + content_seed: 3324479019, + }, + CreateSubdirFile { + dir_seed: 47588, + file_seed: 6835, + content_seed: 4279222285, + }, + GitignoreAppend { + pattern_seed: 41743, + }, + GitCommit { msg_seed: 63391 }, + ]; + run_stress_scenario(&ops); +} + /// Parse `FFF_STRESS_SEED` as either decimal or `0x`-prefixed hex. fn parse_stress_seed() -> u64 { match std::env::var("FFF_STRESS_SEED") { diff --git a/crates/fff-core/tests/real_binary_fixtures.rs b/crates/fff-core/tests/real_binary_fixtures.rs index d7f383b62..e4da06915 100644 --- a/crates/fff-core/tests/real_binary_fixtures.rs +++ b/crates/fff-core/tests/real_binary_fixtures.rs @@ -105,7 +105,10 @@ fn real_binary_fixtures_are_detected_and_excluded_from_grep() { ) .expect("failed to create FilePicker"); - shared_picker.wait_for_indexing_complete(Duration::from_secs(5)); + assert!( + shared_picker.wait_for_indexing_complete(Duration::from_secs(10)), + "indexing/post-scan did not complete in time — binary classification may not have run yet" + ); let guard = shared_picker.read().unwrap(); let picker = guard.as_ref().unwrap(); @@ -151,3 +154,61 @@ fn contains_subslice(haystack: &[u8], needle: &[u8]) -> bool { .windows(needle.len()) .any(|window| window == needle) } + +/// Deterministic regression for the Windows-CI failure where `codex_view` +/// (a >2 MB no-extension binary) was not flagged `is_binary`. Root cause was a +/// readiness-signal gap: `scanning` was cleared before `post_scan_indexing_active` +/// was set, so `wait_for_indexing_complete` could return before the binary sniff +/// ran. Uses synthetic fixtures (no repo/fixture dependency) covering both the +/// >2 MB non-indexable sniff path and the <2 MB bigram path, repeated to stress +/// the signal ordering. With the fix it must pass every iteration. +#[test] +fn binary_classification_done_before_indexing_wait_returns() { + const ITERATIONS: usize = 8; + // NUL bytes => `detect_binary_content` classifies as binary on every path. + let large = vec![0u8; 3 * 1024 * 1024]; // > 2 MB -> non-indexable sniff + let small = vec![0u8; 64 * 1024]; // < 2 MB -> bigram path + + for iteration in 0..ITERATIONS { + let tmp = tempfile::TempDir::new().unwrap(); + let base = tmp.path(); + fs::write(base.join("large_binary_no_ext"), &large).unwrap(); + fs::write(base.join("small.unknownext"), &small).unwrap(); + fs::write(base.join("readme.txt"), "hello world\n").unwrap(); + + let shared_picker = SharedFilePicker::default(); + let shared_frecency = SharedFrecency::default(); + FilePicker::new_with_shared_state( + shared_picker.clone(), + shared_frecency.clone(), + FilePickerOptions { + base_path: base.to_string_lossy().to_string(), + enable_mmap_cache: false, + enable_content_indexing: true, + mode: FFFMode::Neovim, + watch: false, + ..Default::default() + }, + ) + .expect("failed to create FilePicker"); + + assert!( + shared_picker.wait_for_indexing_complete(Duration::from_secs(10)), + "iteration {iteration}: indexing/post-scan did not complete in time" + ); + + let guard = shared_picker.read().unwrap(); + let picker = guard.as_ref().unwrap(); + for name in ["large_binary_no_ext", "small.unknownext"] { + let flagged = picker + .get_files() + .iter() + .any(|f| f.relative_path(picker).ends_with(name) && f.is_binary()); + assert!( + flagged, + "iteration {iteration}: {name} must be flagged is_binary once \ + wait_for_indexing_complete returns" + ); + } + } +} From ab6ef754add8574c73c69bd565809ba1d88aa5e2 Mon Sep 17 00:00:00 2001 From: Dmitriy Kovalenko Date: Mon, 6 Jul 2026 17:51:22 -0700 Subject: [PATCH 2/2] cleanup --- .github/workflows/external-tests.yml | 13 +- .github/workflows/rust.yml | 22 +- Makefile | 29 +- crates/fff-core/src/background_watcher.rs | 166 +++-------- crates/fff-core/src/file_picker.rs | 42 +-- crates/fff-core/src/git_status_worker.rs | 99 ++++--- crates/fff-core/src/scan.rs | 113 ++----- crates/fff-core/src/shared.rs | 25 +- crates/fff-core/tests/fuzz_file_operations.rs | 11 - ...zz_git_watcher_stress.proptest-regressions | 3 +- .../fff-core/tests/fuzz_git_watcher_stress.rs | 278 ++++-------------- crates/fff-core/tests/fuzz_real_repos.rs | 28 -- package.json | 10 +- packages/fff-bun/src/fff-api.ts | 18 +- packages/fff-node/src/fff-api.ts | 18 +- tests/programmatic_search_spec.lua | 21 +- 16 files changed, 238 insertions(+), 658 deletions(-) diff --git a/.github/workflows/external-tests.yml b/.github/workflows/external-tests.yml index 37907f218..7c21232c7 100644 --- a/.github/workflows/external-tests.yml +++ b/.github/workflows/external-tests.yml @@ -100,10 +100,15 @@ jobs: if: failure() shell: bash run: | - echo "=== fff-test.log ===" - if [ -f fff-test.log ]; then - cat fff-test.log - else + # init_tracing writes session files named fff-test++.log + found=0 + for f in fff-test*.log; do + [ -f "$f" ] || continue + found=1 + echo "=== $f ===" + cat "$f" + done + if [ "$found" = 0 ]; then echo "(no log file produced)" fi diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index a4e7562b4..a63411f9e 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -50,7 +50,7 @@ jobs: run: cargo test --no-default-features --features zlob --workspace --exclude fff-nvim stress-test: - name: Stress Test (Watcher + Git) + name: Fuzz Tests runs-on: ${{ matrix.os }} strategy: # Keep going after one OS fails so we can see whether a bug @@ -61,6 +61,10 @@ jobs: # Long-running; don't let a stuck watcher thread burn a full CI # timeout. Two scenarios should finish well under this limit. timeout-minutes: 20 + env: + FFF_STRESS_CASES: "5" + FFF_STRESS_MIN_OPS: "30" + FFF_STRESS_MAX_OPS: "60" steps: - uses: actions/checkout@v5 @@ -77,21 +81,17 @@ jobs: cache-key: "v1-rust-stress-${{ matrix.os }}" components: rustfmt, clippy - - name: Stress test (seeded / deterministic) + - name: Stress test seeded shell: bash run: make test-stress-seeded - env: - FFF_STRESS_CASES: "3" - FFF_STRESS_MIN_OPS: "30" - FFF_STRESS_MAX_OPS: "50" - - name: Stress test (random / fuzzy) + - name: Stress test random shell: bash run: make test-stress-random - env: - FFF_STRESS_CASES: "5" - FFF_STRESS_MIN_OPS: "30" - FFF_STRESS_MAX_OPS: "60" + + - name: Stress test regressions + shell: bash + run: make test-stress-regressions - name: Upload proptest regressions on failure if: failure() diff --git a/Makefile b/Makefile index 5b74a7465..a3004f483 100644 --- a/Makefile +++ b/Makefile @@ -12,19 +12,15 @@ FFF_STRESS_DEFAULT_SEED ?= 0xDEADBEEFCAFEBABE SHELL := bash # Order matters: `-c` must be last so bash treats the recipe as the script # string rather than the literal `-o` / `pipefail` tokens. -.SHELLFLAGS := -o pipefail -ec +.SHELLFLAGS := -o pipefail -euc -.PHONY: build build-c-lib install uninstall test test-rust test-c-smoke test-c-api test-lua test-lua-snap test-version test-bun test-node prepare-bun prepare-bun-packaged prepare-node set-npm-version header test-stress test-stress-seeded test-stress-random test-stress-repos test-node-stress sync-js-api sync-js-api-check bump-homebrew-formula bump-install-mcp-sh test-bun-compile +.PHONY: build build-c-lib install uninstall test test-rust test-c-smoke test-c-api test-lua test-lua-snap test-version test-bun test-node prepare-bun prepare-bun-packaged prepare-node set-npm-version header test-stress test-stress-seeded test-stress-random test-stress-regressions test-stress-repos test-node-stress sync-js-api sync-js-api-check bump-homebrew-formula bump-install-mcp-sh test-bun-compile all: format test lint -# Single source of truth for the shared FileFinder TS interface lives in -# packages/shared/fff-api.ts. tsc cannot import across a package's -# rootDir and the bun package publishes its raw src/, so the file is copied -# into each package instead of symlinked. SYNC_API_SRC := packages/shared/fff-api.ts SYNC_API_TARGETS := packages/fff-node/src/fff-api.ts packages/fff-bun/src/fff-api.ts -SYNC_API_BANNER := // ----------------------------------------------------------------------------\n// GENERATED FILE - DO NOT EDIT.\n// Source of truth: packages/shared/fff-api.ts\n// Run make sync-js-api from the repo root to regenerate.\n// ----------------------------------------------------------------------------\n\n +SYNC_API_BANNER := // ----------------------------------------------------------------------------\n// GENERATED FILE - DO NOT EDIT.\n// Copied from: ${SYNC_API_SRC}\n// Run make sync-js-api from the repo root to regenerate.\n// ----------------------------------------------------------------------------\n\n sync-js-api: @for target in $(SYNC_API_TARGETS); do \ @@ -128,13 +124,6 @@ test-lua: test-setup build exit 1; \ fi -# mini.test reference_screenshot snapshots. Separate runner because mini.test -# spawns child processes and uses its own collector (incompatible with -# PlenaryBustedDirectory). Streams output live via `tee` so failure diffs -# appear as they happen instead of after a long capture-buffered silence. -# `pcall` catches collect-time errors (e.g. parse error in the test file) -# that would otherwise leave headless nvim hanging in its event loop because -# the reporter's `cquit` never fires. test-lua-snap: test-setup build @logfile=$$(mktemp); \ trap 'rm -f "$$logfile"' EXIT; \ @@ -236,7 +225,15 @@ test-stress-random: -p fff-search \ --test fuzz_git_watcher_stress \ --no-default-features --features zlob \ - -- --nocapture stress_random stress_regresion + -- --nocapture stress_random + +test-stress-regressions: + RUSTFLAGS="$(STRESS_RUSTFLAGS)" \ + cargo test --release \ + -p fff-search \ + --test fuzz_git_watcher_stress \ + --no-default-features --features zlob \ + -- --nocapture stress_regression stress_merge_conflict_convergence test-stress-repos: RUSTFLAGS="$(STRESS_RUSTFLAGS)" \ @@ -246,7 +243,7 @@ test-stress-repos: --no-default-features --features zlob \ -- --nocapture -test-stress: test-stress-seeded test-stress-random test-stress-repos +test-stress: test-stress-seeded test-stress-random test-stress-regressions test-stress-repos # Update version in a package.json, including optionalDependencies. # Usage: make set-npm-version PKG=packages/fff-bun VERSION=1.0.0-nightly.abc1234 diff --git a/crates/fff-core/src/background_watcher.rs b/crates/fff-core/src/background_watcher.rs index 1035c88e6..d9f7b36eb 100644 --- a/crates/fff-core/src/background_watcher.rs +++ b/crates/fff-core/src/background_watcher.rs @@ -1,7 +1,7 @@ use crate::constants::MAX_OVERFLOW_FILES; use crate::error::Error; use crate::file_picker::FFFMode; -use crate::git_status_worker::{GitStatusMailbox, spawn_consumer}; +use crate::git_status_worker::GitStatusWorker; use crate::shared::{SharedFilePicker, SharedFrecency}; use crate::sort_buffer::sort_with_buffer; use git2::Repository; @@ -23,8 +23,6 @@ pub struct BackgroundWatcher { debouncer: Arc>>, watch_tx: Option>, owner_thread: Option>, - git_status_mailbox: Arc, - git_status_thread: Option>, } const DEBOUNCE_TIMEOUT: Duration = Duration::from_millis(50); @@ -46,6 +44,7 @@ impl BackgroundWatcher { mode: FFFMode, enable_fs_root_scanning: bool, enable_home_dir_scanning: bool, + git_status_worker: Arc, trace_span: tracing::Span, ) -> Result { info!( @@ -84,20 +83,9 @@ impl BackgroundWatcher { let (watch_tx, watch_rx) = mpsc::channel::(); let watch_tx_for_debouncer = watch_tx.clone(); - // Single-consumer git-status pipeline. Producers (debouncer + owner - // thread) only enqueue; this dedicated consumer applies updates in - // order, so a stale snapshot can never clobber a fresh one. - let git_status_mailbox = GitStatusMailbox::new(); - let git_status_thread = spawn_consumer( - shared_picker.weaken(), - shared_frecency.clone(), - Arc::clone(&git_status_mailbox), - trace_span.clone(), - ); - let owner_weak_picker = shared_picker.weaken(); let owner_git_workdir = git_workdir.clone(); - let owner_mailbox = Arc::clone(&git_status_mailbox); + let owner_git_worker = Arc::clone(&git_status_worker); let debouncer = Self::create_debouncer( base_path, @@ -107,7 +95,7 @@ impl BackgroundWatcher { mode, use_recursive, watch_tx_for_debouncer, - Arc::clone(&git_status_mailbox), + git_status_worker, )?; info!("Background file watcher initialized successfully"); @@ -161,7 +149,7 @@ impl BackgroundWatcher { &dir, &strong_picker, &owner_git_workdir, - &owner_mailbox, + &owner_git_worker, ); // Transient strong ref drops here, back @@ -176,8 +164,6 @@ impl BackgroundWatcher { debouncer, watch_tx: Some(watch_tx), owner_thread: Some(owner_thread), - git_status_mailbox, - git_status_thread: Some(git_status_thread), }) } @@ -190,7 +176,7 @@ impl BackgroundWatcher { mode: FFFMode, use_recursive: bool, watch_tx: mpsc::Sender, - git_status_mailbox: Arc, + git_status_worker: Arc, ) -> Result { let config = Config::default() // do not follow symlinks as then notifiers spawns a bunch of events for symlinked @@ -201,47 +187,34 @@ impl BackgroundWatcher { // our own grep calls and preview window rendering .with_event_kinds(EventKindMask::CORE); - // `use_recursive` was decided by the caller from a cheap size hint, - // so the event-handler closure can capture it directly. - // - // The closure lives on the debouncer's internal event thread - // for as long as the debouncer exists — i.e. the full - // lifetime of `BackgroundWatcher`. Capturing a strong - // `SharedFilePicker` here would re-introduce the Arc cycle - // we just broke with `owner_picker`'s `downgrade()` above. - // Capture a weak handle instead and upgrade per-batch. let git_workdir_for_handler = git_workdir.clone(); let base_path_for_handler = base_path.clone(); let shared_picker_for_watching = shared_picker.clone(); - let event_picker = shared_picker.weaken(); + let file_picker = shared_picker.weaken(); let mut debouncer = new_debouncer_opt( DEBOUNCE_TIMEOUT, Some(DEBOUNCE_TIMEOUT / 2), // tick rate for the event span { move |result: DebounceEventResult| match result { Ok(events) => { - // Upgrade just long enough to drive one - // debounced batch. Failure means every - // external `SharedFilePicker` has already - // dropped and teardown is already underway. - let Some(strong_picker) = event_picker.upgrade() else { + let Some(file_picker) = file_picker.upgrade() else { return; }; let new_dirs = handle_debounced_events( + mode, events, &base_path_for_handler, &git_workdir_for_handler, - &strong_picker, + &file_picker, &shared_frecency, - mode, - &git_status_mailbox, + &git_status_worker, ); - // every new directory creates had to be reflected in the picker state + // every new directory created has to be reflected in the picker state for dir in new_dirs { if let Err(e) = watch_tx.send(dir) { - warn!(?e, "Failed to send directory update error"); + error!(?e, "Failed to send directory update error"); } } } @@ -258,21 +231,8 @@ impl BackgroundWatcher { config, )?; - // Watching strategy: - // - // For small-to-medium repos we watch each indexed directory individually - // (NonRecursive). This avoids receiving events for gitignored paths like - // node_modules/ and keeps the event volume low. - // - // On macOS, each `watch()` call creates a separate FSEventStream. Large - // repos (e.g. Chromium with 487K+ files) can have tens of thousands of - // directories, which exhausts the per-process FSEvents stream limit and - // causes "unable to start FSEvent stream" errors. So we have to make on recursive scan - // - // On Linux (inotify), RecursiveMode::Recursive creates one kernel watch - // per subdirectory *including* gitignored ones, wasting file descriptors. - // The per-directory NonRecursive approach is always used on Linux. if use_recursive { + // if the platform supports native watcher recursion debouncer.watch(base_path.as_path(), RecursiveMode::Recursive)?; info!( "File watcher initialized with single recursive watch on {} \ @@ -283,22 +243,13 @@ impl BackgroundWatcher { } else { debouncer.watch(base_path.as_path(), RecursiveMode::NonRecursive)?; - // Stream watch-dir registration directly under the picker - // read lock. Only Linux (inotify) reaches this branch - // macOS always takes the recursive path above. `inotify`'s - // `inotify_add_watch()` is fast-fail: on ENOSPC it returns - // immediately, no kernel retry loop, so holding the read - // lock across the stream is O(ms) even for large repos. - // - // Abort the loop after a run of failures. Once ENOSPC hits, - // further calls won't succeed until the user raises - // `fs.inotify.max_user_watches`, so there's no value in continuing. const MAX_CONSECUTIVE_WATCH_FAILURES: usize = 16; let mut watched = 0usize; let mut consecutive_failures = 0usize; - let mut aborted_early = false; + // `inotify` is fast-fail: on ENOSPC it returns + // immediately, no kernel retry loop, so holding this lock is free if let Some(guard) = shared_picker_for_watching.read().ok() && let Some(picker) = guard.as_ref() { @@ -320,11 +271,9 @@ impl BackgroundWatcher { warn!( consecutive_failures, watched, - "Aborting NonRecursive watch loop — per-process \ - watch cap exhausted, further dirs would just burn \ - kernel time for no coverage" + "Giving up setting file watcher for all the directories. Check if your system has enough fs watchers limit." ); - aborted_early = true; + ControlFlow::Break(()) } else { ControlFlow::Continue(()) @@ -334,46 +283,22 @@ impl BackgroundWatcher { }); } - info!( - "File watcher initialized for {} directories (NonRecursive) under {} (aborted_early={})", - watched, - base_path.display(), - aborted_early, + tracing::info!( + ?watched, + path = ?base_path.display(), + "File watcher initialized" ); } // The .git directory is excluded from the file list but we still need // to observe changes that affect git status (staging, unstaging, - // committing, branch switches, merges, etc.). - // When using recursive mode the base watch already covers .git/, - // but these targeted watches are cheap (at most 3 extra streams) - // and ensure we catch status changes even if the recursive backend - // coalesces or delays .git events. + // committing, branch switches, merges, etc) watch_git_status_paths(&mut debouncer, git_workdir.as_ref()); Ok(debouncer) } - /// Signal the watcher to shut down without blocking on its worker - /// threads. Safe to call from any context, including while holding - /// the [`SharedFilePicker`] write lock. - /// - /// Both the debouncer's internal event loop and our owner thread - /// may call `SharedFilePicker::write()` inside their handlers. A - /// blocking join here would deadlock against a caller that already - /// holds that lock (e.g. `stop_background_monitor` under a - /// `shared_picker.write()` guard). Instead we: - /// - /// * drop the `watch_tx` Sender — the owner thread's - /// `watch_rx.recv()` returns `Err` and the thread exits at - /// its next `recv`. - /// * call `debouncer.stop_nonblocking()` — signals the debouncer - /// event loop to exit on its next tick and drops the watcher, - /// closing the FSEvent / inotify / ReadDirectoryChangesW stream. - /// * detach both `JoinHandle`s. - /// - /// In-flight handler invocations finish on their own (at most one - /// more batch) once the caller releases any locks they hold. + /// Signals the background watcher threads to shut down, doesn't guarantee to deallocate immediately pub fn stop(&mut self) { self.watch_tx.take(); if let Some(debouncer) = self.debouncer.lock().take() { @@ -382,24 +307,9 @@ impl BackgroundWatcher { self.owner_thread.take(); - // Wake the git-status consumer so it exits on its next loop, then - // detach it. Never joined here for the same reason the owner thread - // isn't: the consumer takes `SharedFilePicker::write()`, and a caller - // holding that lock would deadlock on a blocking join. - self.git_status_mailbox.signal_shutdown(); - self.git_status_thread.take(); - info!("Background file watcher stop signaled"); } - /// Queue a non-recursive watch registration on `dir`. - /// - /// The owner thread is always blocked on `watch_rx.recv()`, so - /// the `send()` here wakes it immediately via the channel's - /// condvar — no external unpark needed. - /// - /// Returns `false` once `stop()` has dropped our `Sender` — any - /// further request is silently discarded. pub(crate) fn request_watch_dir(&self, dir: PathBuf) -> bool { match self.watch_tx.as_ref() { Some(tx) => tx.send(dir).is_ok(), @@ -414,15 +324,15 @@ impl Drop for BackgroundWatcher { } } -#[tracing::instrument(name = "fs_events", skip(events, shared_picker, shared_frecency, git_status_mailbox), level = Level::DEBUG)] +#[tracing::instrument(name = "fs_events", skip(events, shared_picker, shared_frecency, git_status_worker), level = Level::DEBUG)] fn handle_debounced_events( + mode: FFFMode, events: Vec, base_path: &Path, git_workdir: &Option, shared_picker: &SharedFilePicker, shared_frecency: &SharedFrecency, - mode: FFFMode, - git_status_mailbox: &Arc, + git_status_worker: &Arc, ) -> Vec { // this will be called very often, we have to minimiy the lock time for file picker let repo = git_workdir.as_ref().and_then(|p| Repository::open(p).ok()); @@ -686,17 +596,15 @@ fn handle_debounced_events( } } - // Enqueue git-status work onto the dedicated consumer instead of doing it - // here: the debouncer thread must never block on a (potentially slow) git - // read, and the single consumer keeps updates ordered. Skip entirely when a - // full filesystem rescan is queued — that path recomputes git status itself. - if !need_full_rescan { + // do not try to update the paths if we anyway going to rescan everything from scratch + // no repo => no consumer thread, so don't accumulate paths nobody will drain + if !need_full_rescan && repo.is_some() { if need_full_git_rescan { - // A full git rescan re-reads every tracked path (including ones that - // just went clean after a commit), so it supersedes per-path work. - git_status_mailbox.request_full_rescan(); + // A full git rescan re-reads every tracked path (including ones that just + // went clean after a commit), so it already subsumes the per-path update. + git_status_worker.request_full_rescan(); } else if !files_to_update_git_status.is_empty() { - git_status_mailbox.enqueue_paths(files_to_update_git_status); + git_status_worker.enqueue_paths(files_to_update_git_status); } } @@ -709,7 +617,7 @@ fn track_files_from_new_directories( dir: &Path, shared_picker: &SharedFilePicker, git_workdir: &Option, - git_status_mailbox: &Arc, + git_status_worker: &Arc, ) { let Ok(entries) = std::fs::read_dir(dir) else { return; @@ -757,10 +665,8 @@ fn track_files_from_new_directories( } } - // Hand the git-status refresh to the dedicated consumer (ordered + off the - // owner thread). It no-ops when there is no git repo. if repo.is_some() { - git_status_mailbox.enqueue_paths(files_to_add); + git_status_worker.enqueue_paths(files_to_add); } debug!( diff --git a/crates/fff-core/src/file_picker.rs b/crates/fff-core/src/file_picker.rs index 34409c707..0d51745de 100644 --- a/crates/fff-core/src/file_picker.rs +++ b/crates/fff-core/src/file_picker.rs @@ -484,6 +484,10 @@ pub struct FilePicker { sync_data: FileSync, pub(crate) signals: ScanSignals, pub(crate) background_watcher: Option, + /// Single serialized writer for all git-status updates (scan, watcher, + /// FFI). Owned by the picker so it exists before the first scan; its + /// consumer thread is spawned lazily once a git workdir is discovered. + pub(crate) git_status_worker: Arc, cache_budget: Arc, has_explicit_cache_budget: bool, scanned_files_count: Arc, @@ -768,6 +772,7 @@ impl FilePicker { Ok(FilePicker { background_watcher: None, + git_status_worker: crate::git_status_worker::GitStatusWorker::new(), base_path: path, cache_budget: Arc::new(initial_budget), has_explicit_cache_budget: has_explicit_budget, @@ -909,36 +914,10 @@ impl FilePicker { Ok(()) } - /// Start the background file-system watcher. - /// - /// The picker must already be placed into `shared_picker` (the watcher - /// needs the shared handle to apply live updates). Call after - /// [`collect_files`](Self::collect_files) or after an initial scan. - pub fn spawn_background_watcher( - &mut self, - shared_picker: &SharedFilePicker, - shared_frecency: &SharedFrecency, - ) -> Result<(), Error> { - let git_workdir = self.sync_data.git_workdir.clone(); - let watcher = BackgroundWatcher::new( - self.base_path.clone(), - git_workdir, - shared_picker.clone(), - shared_frecency.clone(), - self.mode, - self.enable_fs_root_scanning, - self.enable_home_dir_scanning, - self.trace_span.clone(), - )?; - self.background_watcher = Some(watcher); - self.signals.watcher_ready.store(true, Ordering::Release); - Ok(()) - } - /// Perform fuzzy search on files with a pre-parsed query. /// - /// The query should be parsed using [`FFFQuery`]::parse() before calling - /// this function. If a [`QueryTracker`] is provided, the search will + /// The query should be parsed using [`crate::FFFQuery`] before calling + /// this function. If a [`crate::QueryTracker`] is provided, the search will /// automatically look up the last selected file for this query and boost it #[tracing::instrument(skip_all, name = "Fuzzy file search", fields(query = query.raw_query))] pub fn fuzzy_search<'q>( @@ -1370,12 +1349,10 @@ impl FilePicker { Some(PostScanUnsafeSnapshot { files: self.sync_data.files.clone(), - dirs: self.sync_data.dirs.clone(), arena: self.sync_data.chunked_paths.as_ref().map(Arc::clone), base_count: self.sync_data.base_count, indexable_count: self.sync_data.indexable_count, base_path: self.base_path.clone(), - cancelled: Arc::clone(&self.signals.cancelled), post_scan_flag: Arc::clone(&self.signals.post_scan_indexing_active), _budget: Arc::clone(&self.cache_budget), }) @@ -1744,6 +1721,9 @@ impl Drop for FilePicker { // Cancel any in-flight ScanJob bound to this picker's signals so // it cannot mutate the replacement picker after a swap. self.signals.cancelled.store(true, Ordering::Release); + // Wake the git-status consumer so it exits; never joined (it takes + // the picker write lock, a blocking join here could deadlock). + self.git_status_worker.signal_shutdown(); } } @@ -1774,14 +1754,12 @@ impl FileSlot { /// `ScanJob::run`, `scan_job_running == false` implies no live snapshot. pub(crate) struct PostScanUnsafeSnapshot { pub files: StableVec, - pub dirs: StableVec, pub arena: Option>, // TODO figure this out pub _budget: Arc, pub base_count: usize, pub indexable_count: usize, pub base_path: PathBuf, - pub cancelled: Arc, post_scan_flag: Arc, } diff --git a/crates/fff-core/src/git_status_worker.rs b/crates/fff-core/src/git_status_worker.rs index 380301d08..b6e74028c 100644 --- a/crates/fff-core/src/git_status_worker.rs +++ b/crates/fff-core/src/git_status_worker.rs @@ -3,9 +3,9 @@ use ahash::AHashSet; use parking_lot::{Condvar, Mutex}; use std::path::PathBuf; use std::sync::Arc; -use std::thread::JoinHandle; -use tracing::{debug, error, info}; +use std::sync::atomic::{AtomicBool, Ordering}; +// we don't really need a queue here #[derive(Default)] struct Pending { paths: AHashSet, @@ -19,26 +19,36 @@ impl Pending { } } -/// Single-consumer coalescing queue for git-status updates. Producers (the -/// debouncer + owner threads) push cheaply and return; a dedicated consumer -/// drains the whole accumulated batch at once. A single ordered consumer means -/// a stale pre-commit snapshot can never be applied after a fresh post-commit -/// one — the race the watcher fuzz test caught. -pub(crate) struct GitStatusMailbox { +/// Condvar based queue that is used for batch processing events +pub(crate) struct GitStatusWorker { state: Mutex, cv: Condvar, + consumer_spawned: AtomicBool, } -impl GitStatusMailbox { +impl GitStatusWorker { pub(crate) fn new() -> Arc { Arc::new(Self { state: Mutex::new(Pending::default()), cv: Condvar::new(), + consumer_spawned: AtomicBool::new(false), }) } - /// Accumulate paths that need a targeted git-status refresh. Cheap: a lock, - /// a set-extend, and a single wakeup. Never blocks on git. + pub(crate) fn spawn_once( + self: &Arc, + weak_picker: WeakFilePicker, + frecency: SharedFrecency, + ) { + if self + .consumer_spawned + .compare_exchange(false, true, Ordering::AcqRel, Ordering::Acquire) + .is_ok() + { + Self::spawn_consumer(Arc::clone(self), weak_picker, frecency); + } + } + pub(crate) fn enqueue_paths(&self, paths: I) where I: IntoIterator, @@ -49,8 +59,6 @@ impl GitStatusMailbox { self.cv.notify_one(); } - /// Request a full git rescan. Supersedes any queued per-path work — a full - /// rescan re-reads every tracked path, so the drained paths are irrelevant. pub(crate) fn request_full_rescan(&self) { let mut guard = self.state.lock(); guard.full_rescan = true; @@ -65,8 +73,6 @@ impl GitStatusMailbox { self.cv.notify_one(); } - /// Block until there is work or shutdown, then drain the whole batch. - /// Returns `None` once shutdown was requested (consumer should exit). fn wait_and_take(&self) -> Option { let mut guard = self.state.lock(); while !guard.shutdown && !guard.has_work() { @@ -77,43 +83,38 @@ impl GitStatusMailbox { } Some(std::mem::take(&mut *guard)) } -} -/// Spawn the dedicated git-status consumer. It sleeps on the condvar while -/// idle (no polling), wakes to drain the accumulated batch, and applies it via -/// the serialized `SharedFilePicker` methods. Detached on teardown via -/// `signal_shutdown`; it also exits if the picker has been dropped. -pub(crate) fn spawn_consumer( - weak_picker: WeakFilePicker, - frecency: SharedFrecency, - mailbox: Arc, - span: tracing::Span, -) -> JoinHandle<()> { - std::thread::Builder::new() - .name("fff-git-status".into()) - .spawn(move || { - let _g = span.enter(); - while let Some(work) = mailbox.wait_and_take() { - // Upgrade only for one batch; if the picker is gone teardown is - // underway, so exit cleanly. - let Some(picker) = weak_picker.upgrade() else { - break; - }; + // the problem: git status update can take a lot of time especially on big repositories + // and there is unpredictable wait time on the lock file if huge commit is going so we have to + // spawn a separate thread to guartee that notify handler is unlocked even if git update takes a + // lot of time on every event burst (pretty cheap as this thread is going to sleep 99.9% of time) + fn spawn_consumer( + mailbox: Arc, + weak_picker: WeakFilePicker, + frecency: SharedFrecency, + ) { + let _ = std::thread::Builder::new() + .name("fff-git-status".into()) + .spawn(move || { + while let Some(work) = mailbox.wait_and_take() { + let Some(picker) = weak_picker.upgrade() else { + break; + }; - if work.full_rescan { - debug!("git-status worker: full rescan"); - if let Err(e) = picker.refresh_git_status(&frecency) { - error!("git-status worker: full rescan failed: {e:?}"); - } - } else if !work.paths.is_empty() { - let paths: Vec = work.paths.into_iter().collect(); - if let Err(e) = picker.update_git_status_for_paths(&paths, &frecency) { - error!("git-status worker: path update failed: {e:?}"); + if work.full_rescan { + if let Err(e) = picker.refresh_git_status(&frecency) { + tracing::error!("git-status worker: full rescan failed: {e:?}"); + } + } else if !work.paths.is_empty() { + let paths: Vec = work.paths.into_iter().collect(); + if let Err(e) = picker.update_git_status_for_paths(&paths, &frecency) { + tracing::error!("git-status worker: path update failed: {e:?}"); + } } } - } - info!("git-status worker stopped"); - }) - .expect("failed to spawn fff-git-status thread") + tracing::info!("git-status worker stopped"); + }) + .inspect_err(|err| tracing::error!(?err, "Failed to spawn git status worker")); + } } diff --git a/crates/fff-core/src/scan.rs b/crates/fff-core/src/scan.rs index 54424593f..3fe6fa37b 100644 --- a/crates/fff-core/src/scan.rs +++ b/crates/fff-core/src/scan.rs @@ -2,7 +2,6 @@ use std::path::PathBuf; use std::sync::Arc; use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; -use rayon::prelude::*; use tracing::{error, info}; use crate::FileSync; @@ -10,10 +9,8 @@ use crate::background_watcher::BackgroundWatcher; use crate::bigram_filter::{build_bigram_index, sniff_binary_for_non_indexable}; use crate::error::Error; use crate::file_picker::FFFMode; -use crate::git::GitStatusCache; use crate::parallelism::BACKGROUND_THREAD_POOL; use crate::shared::{SharedFilePicker, SharedFrecency}; -use crate::simd_path::ArenaPtr; use crate::types::ContentCacheBudget; #[derive(Clone, Default)] @@ -165,14 +162,10 @@ impl ScanJob { } = self; let _scanning = ScanningGuard::new(&signals, config.install_watcher); - - // Reset the UI-visible counter; the walker bumps it per file - // and `get_scan_progress` reads it without locks. scanned_files_counter.store(0, Ordering::Relaxed); - // 1. Start git discovery and walk filesystem off-lock. + // 1. Walk the file system and collect the list of files let git_workdir = FileSync::discover_git_workdir(&base_path); - let status_handle = git_workdir.clone().map(FileSync::spawn_git_status); let sync = match FileSync::walk_filesystem( &base_path, git_workdir.clone(), @@ -188,7 +181,8 @@ impl ScanJob { } }; - // 2. Brief write to install the freshly-walked file list. + // 2. Populate the file list + let git_status_worker; if let Ok(mut guard) = shared_picker.write() && let Some(picker) = guard.as_mut() { @@ -199,6 +193,7 @@ impl ScanJob { let live_count = sync.live_count; picker.commit_new_sync(sync); + git_status_worker = Arc::clone(&picker.git_status_worker); if config.auto_cache_budget && !picker.has_explicit_cache_budget() { picker.set_cache_budget(ContentCacheBudget::new_for_repo(live_count)); @@ -208,14 +203,16 @@ impl ScanJob { return; } - // Acquire the post-scan snapshot BEFORE clearing `scanning`. Taking the - // snapshot sets `post_scan_indexing_active`, so at least one readiness - // signal stays set continuously from scan start until post-scan work - // completes. Clearing `scanning` first opened a window where both - // signals read false and `wait_for_indexing_complete` could return - // before the binary sniff / bigram build ran — a Windows-CI flake in - // `real_binary_fixtures`. - let mut snapshot = if !signals.cancelled.load(Ordering::Acquire) { + // Spawn the git status worker once. BUG PINNNING. If the user initiated git in the folder + // which is a real use case we need to have a way to start the git worker background thread dynamically + if git_workdir.is_some() && !signals.cancelled.load(Ordering::Acquire) { + git_status_worker.spawn_once(shared_picker.weaken(), shared_frecency.clone()); + git_status_worker.request_full_rescan(); // this runs anyway + } + + // BUG pinning: take the snapshot *before* the storing the scan=true, otherwise there is a tiny + // race window when there scanned is set to true, but `post_scan_indexing_active` flag is `false` + let snapshot = if !signals.cancelled.load(Ordering::Acquire) { shared_picker.read().ok().and_then(|guard| { guard .as_ref() @@ -225,36 +222,21 @@ impl ScanJob { None }; - // Files are now searchable — flip the scan signal so UI progress polls - // see the picker as "ready" while we run the post-scan steps. - signals.scanning.store(false, Ordering::Relaxed); + signals.scanning.store(false, Ordering::Relaxed); // file are searchable // in case we do a rescan, we have to resubscribe a watcher to the new set of directories - // all the already watched directories are not going to be resubscribed + // all the already watched directories are not going to be resubscribed (this is internally deduped) if !config.install_watcher && !signals.cancelled.load(Ordering::Acquire) { rescubscribe_watcher_post_scan(&shared_picker); } - // 3. Post-scan warmup + bigram build — runs in parallel with the - // git-status thread to overlap the two expensive phases. - // Always runs (even with both flags off) so binary-content files - // with unknown extensions get reclassified before user search hits. + // 3. Runs post scna in parallel with git status collection if !signals.cancelled.load(Ordering::Acquire) && let Some(snap) = snapshot.as_ref() { Self::run_post_scan(&shared_picker, &signals, &config, snap); } - // 4. Join and git status, this HAS to be done after the post scan - if !signals.cancelled.load(Ordering::Acquire) - && let Some(status_handle) = status_handle - && let Some(snapshot) = snapshot.as_mut() - // THIS DOES WAIT for potentially very long status query - && let Ok(Some(git_status)) = status_handle.join() - { - apply_git_status_and_frecency(git_status, &shared_frecency, mode, snapshot); - } - drop(snapshot); // SNAPSHOT SHOULD NOT BE USED AFTER THIS POINT // 5. Install filesystem watcher (initial scan only). @@ -271,6 +253,7 @@ impl ScanJob { mode, config.enable_fs_root_scanning, config.enable_home_dir_scanning, + git_status_worker, tracing::Span::current(), ) { Ok(watcher) => { @@ -419,63 +402,3 @@ fn rescubscribe_watcher_post_scan(shared_picker: &SharedFilePicker) { std::ops::ControlFlow::Continue(()) }); } - -#[tracing::instrument( - level = "debug", - skip_all, - fields(file_count = tracing::field::Empty, dirty_count = tracing::field::Empty), -)] -fn apply_git_status_and_frecency( - git_cache: GitStatusCache, - shared_frecency: &SharedFrecency, - mode: FFFMode, - unsafe_snapshot: &mut crate::file_picker::PostScanUnsafeSnapshot, -) { - let frecency = shared_frecency.read().ok(); - let frecency_ref = frecency.as_ref().and_then(|f| f.as_ref()); - - let base_count = unsafe_snapshot.base_count; - let files: &mut [crate::types::FileItem] = &mut unsafe_snapshot.files[..base_count]; - // Dir frecency goes through per-entry `AtomicI32`; a shared slice is - // enough and avoids any `&mut` aliasing against the Arc-shared buffer. - let dirs: &[crate::types::DirItem] = &unsafe_snapshot.dirs; - let arena = unsafe_snapshot - .arena - .as_ref() - .map(|s| s.as_arena_ptr()) - .unwrap_or(ArenaPtr::null()); - - // Reset dir frecency before recomputation. - for dir in dirs.iter() { - dir.reset_frecency(); - } - - BACKGROUND_THREAD_POOL.install(|| { - files.par_iter_mut().for_each(|file| { - if unsafe_snapshot.cancelled.load(Ordering::Relaxed) { - return; - } - - let mut buf = [0u8; crate::simd_path::PATH_BUF_SIZE]; - let absolute_path = - file.write_absolute_path(arena, &unsafe_snapshot.base_path, &mut buf); - - file.git_status = git_cache.lookup_status(absolute_path); - if let Some(frecency) = frecency_ref { - let _ = - file.update_frecency_scores(frecency, arena, &unsafe_snapshot.base_path, mode); - } - - let score = file.access_frecency_score as i32; - if score > 0 { - let dir_idx = file.parent_dir_index as usize; - if let Some(dir) = dirs.get(dir_idx) { - dir.update_frecency_if_larger(score); - } - } - }); - }); - - let span = tracing::Span::current(); - span.record("dirty_count", git_cache.statuses_len()); -} diff --git a/crates/fff-core/src/shared.rs b/crates/fff-core/src/shared.rs index 632ecc840..10d6e6500 100644 --- a/crates/fff-core/src/shared.rs +++ b/crates/fff-core/src/shared.rs @@ -73,18 +73,12 @@ pub struct SharedFilePicker(pub(crate) Arc); pub struct SharedPickerInner { picker: parking_lot::RwLock>, - // Serializes every git-status read+apply (full rescans and per-path - // updates alike) so the two steps stay atomic as a unit. Without it a - // snapshot read from the pre-commit index can be applied *after* a - // post-commit one, leaving stale INDEX_* bits that never converge. - git_status_update: parking_lot::Mutex<()>, } impl Default for SharedPickerInner { fn default() -> Self { Self { picker: parking_lot::RwLock::new(None), - git_status_update: parking_lot::Mutex::new(()), } } } @@ -223,14 +217,11 @@ impl SharedFilePicker { Ok(()) } - /// Refresh git statuses for all indexed files. + /// Refresh git statuses for all indexed files + #[tracing::instrument(level = "info", skip_all)] pub fn refresh_git_status(&self, shared_frecency: &SharedFrecency) -> Result { use tracing::debug; - // Hold the serialization lock across the whole read+apply so no other - // status updater can interleave and clobber us with a stale snapshot. - let _serialize = self.0.git_status_update.lock(); - let git_status = { let git_root = { let guard = self.read()?; @@ -267,10 +258,6 @@ impl SharedFilePicker { } /// Recompute and apply git status for a specific set of paths. - /// - /// Serialized against [`Self::refresh_git_status`] via the same lock and - /// waits for any in-flight `.git/index.lock` first, so it never reads a - /// half-written index mid-commit and never races a full rescan. pub fn update_git_status_for_paths( &self, paths: &[PathBuf], @@ -280,8 +267,6 @@ impl SharedFilePicker { return Ok(()); } - let _serialize = self.0.git_status_update.lock(); - let git_root = { let guard = self.read()?; let Some(ref picker) = *guard else { @@ -370,8 +355,7 @@ impl SharedDb { *guard = Some(tracker); } - // GC holds a read guard on this lock, so destroy / re-init wait - // for it naturally — no join handle, no race against file removal. + // GC holds a read guard on this lock, so destroy / re-init wait won't race spawn_lmdb_gc(self.inner.clone()); Ok(()) } @@ -382,8 +366,7 @@ impl SharedDb { /// access) are finished before the LMDB environment is closed and the files /// are removed. /// - /// Returns `Ok(Some(path))` with the deleted path, or `Ok(None)` if no - /// tracker was initialized. + /// Returns `Ok(Some(path))` with the deleted path, or `Ok(None)` if no tracker was initialized. pub fn destroy(&self) -> Result, Error> { let mut guard = self.write()?; let Some(tracker) = guard.take() else { diff --git a/crates/fff-core/tests/fuzz_file_operations.rs b/crates/fff-core/tests/fuzz_file_operations.rs index a8e324e16..846849396 100644 --- a/crates/fff-core/tests/fuzz_file_operations.rs +++ b/crates/fff-core/tests/fuzz_file_operations.rs @@ -1,14 +1,3 @@ -//! Randomized file-system mutation stress test. -//! -//! Seeds a directory with ~40 files across diverse content domains, builds the -//! picker + bigram index, then runs 20 rounds of randomized create / edit / -//! delete / rename / read-only operations. After every round the test verifies -//! that plain-text grep, regex grep, and fuzzy file search all return correct -//! results for every live and dead file. -//! -//! Uses a seeded RNG (`SmallRng::seed_from_u64`) for deterministic -//! reproduction. - use std::fs; use std::path::{Path, PathBuf}; use std::process::Command; diff --git a/crates/fff-core/tests/fuzz_git_watcher_stress.proptest-regressions b/crates/fff-core/tests/fuzz_git_watcher_stress.proptest-regressions index 617fd592a..b092012e1 100644 --- a/crates/fff-core/tests/fuzz_git_watcher_stress.proptest-regressions +++ b/crates/fff-core/tests/fuzz_git_watcher_stress.proptest-regressions @@ -4,4 +4,5 @@ # # It is recommended to check this file in to source control so that # everyone who runs the test benefits from these saved cases. -cc 2c9d1ea2efbf6161f84b69598e884dbf1bde6039c70625adde0374817e20e2ea # shrinks to ops = [Touch { idx: 11805740887947177360 }, EditFile { idx: 16172331740552606779, content_seed: 658362763 }, EditFile { idx: 15583428591709383853, content_seed: 959932347 }, CreateFile { seed: 3931677194, content_seed: 3741630760 }, RenameFile { idx: 10904465323748885299, new_seed: 2279324670 }, EditFile { idx: 9420001873471413265, content_seed: 2114867802 }, EditFile { idx: 213141289566213885, content_seed: 560732827 }, EditFile { idx: 17035572278797292009, content_seed: 3808313786 }, CreateFile { seed: 1288265487, content_seed: 1533838042 }, EditFile { idx: 16473729680435813293, content_seed: 1008536355 }, Noop, EditFile { idx: 5398337378624802944, content_seed: 1867267587 }, EditFile { idx: 8551649030560368528, content_seed: 2795207371 }, EditFile { idx: 481097026290699171, content_seed: 2327194851 }, EditFile { idx: 16127721863741462430, content_seed: 259019344 }, EditFile { idx: 12499722135620715652, content_seed: 710330684 }, Noop, EditFile { idx: 8570312264332927846, content_seed: 20660474 }, EditFile { idx: 8235918596759853707, content_seed: 3895597068 }, EditFile { idx: 12941454977835821598, content_seed: 974440776 }, CreateFile { seed: 2370169947, content_seed: 3173097146 }, EditFile { idx: 17976810772660212547, content_seed: 1324367109 }, EditFile { idx: 9502968218335991677, content_seed: 4023431064 }, EditFile { idx: 6867642529007482379, content_seed: 207981211 }, CreateFile { seed: 570877898, content_seed: 1583247379 }, EditFile { idx: 7954117886944662117, content_seed: 1193244670 }, EditFile { idx: 2859309324154804229, content_seed: 669423541 }, EditFile { idx: 1701666328639270148, content_seed: 3533870941 }, EditFile { idx: 9409088379085591352, content_seed: 132132514 }, CreateSubdirFile { dir_seed: 16362, file_seed: 52030, content_seed: 1374259669 }, EditFile { idx: 13262913709196533433, content_seed: 2320968118 }, EditFile { idx: 17763858399044545684, content_seed: 356795223 }, GitAddAll, GitAddAll, EditFile { idx: 15775096968456010165, content_seed: 1772666100 }, CreateFile { seed: 2452057527, content_seed: 4111477598 }, EditFile { idx: 10711976377011651566, content_seed: 3912798245 }, EditFile { idx: 9546828938565340524, content_seed: 172130614 }, EditFile { idx: 13343766455516594408, content_seed: 4263257980 }, EditFile { idx: 3019554710255922240, content_seed: 2030698787 }, EditFile { idx: 10405611055126420472, content_seed: 1193589142 }, EditFile { idx: 17138669581083326405, content_seed: 2903728507 }, GitignoreAppend { pattern_seed: 54248 }, EditFile { idx: 13072623131577163943, content_seed: 1483932899 }, DeleteFile { idx: 12869318557852831815 }, EditFile { idx: 6824133502332012157, content_seed: 3150001556 }, EditFile { idx: 14438183751086260985, content_seed: 3158759791 }, EditFile { idx: 3246005478687480560, content_seed: 1237031794 }, EditFile { idx: 18419405778222487428, content_seed: 564314178 }, DeleteFile { idx: 5831672858052749534 }, GitCommit { msg_seed: 6624 }, RenameFile { idx: 9595327710957667341, new_seed: 3574446333 }, EditFile { idx: 11366319046220637326, content_seed: 3919865486 }, CreateFile { seed: 906013144, content_seed: 1799214899 }, EditFile { idx: 6771264237902804639, content_seed: 2901771864 }, EditFile { idx: 5877801695508394108, content_seed: 3324479019 }, CreateSubdirFile { dir_seed: 47588, file_seed: 6835, content_seed: 4279222285 }, GitignoreAppend { pattern_seed: 41743 }, GitCommit { msg_seed: 63391 }] +cc 2c9d1ea2efbf6161f84b69598e884dbf1bde6039c70625adde0374817e20e2ea +cc 1ac0f8f02b160dce13ca3f3630266abd24bd32b4e36d72a6a0a5365139ded3a8 diff --git a/crates/fff-core/tests/fuzz_git_watcher_stress.rs b/crates/fff-core/tests/fuzz_git_watcher_stress.rs index 6d5075771..dda736867 100644 --- a/crates/fff-core/tests/fuzz_git_watcher_stress.rs +++ b/crates/fff-core/tests/fuzz_git_watcher_stress.rs @@ -192,8 +192,10 @@ fn op_strategy() -> impl Strategy { } fn ops_strategy() -> impl Strategy> { - let min = stress_min_ops(); - let max = stress_max_ops(); + ops_strategy_bounded(stress_min_ops(), stress_max_ops()) +} + +fn ops_strategy_bounded(min: usize, max: usize) -> impl Strategy> { prop::collection::vec(op_strategy(), min..=max) } @@ -288,232 +290,62 @@ fn stress_seeded() { /// Pinned deterministic regression for the git-status divergence found on /// Windows CI (run 28264744320): after a `GitCommit` the picker retained stale /// `INDEX_*` bits because a pre-commit per-path status snapshot was applied -/// after the post-commit full rescan. Unlike `stress_random`, this replays the -/// exact operation sequence directly (independent of proptest RNG/strategy), -/// so the scenario always runs and guards the fix. +/// after the post-commit full rescan. +/// +/// The op sequence is regenerated from the proptest seed persisted in the +/// regressions file (`cc 2c9d...`) using the CI op bounds (30..=60) that were +/// in effect when the failure was found. The fingerprint assertion fails +/// loudly if `ops_strategy()` ever changes shape — a changed strategy would +/// silently decode the same seed into a *different* scenario, turning this +/// regression guard into a no-op. #[test] fn stress_regression_stale_index_after_commit() { - use AbstractOp::*; - let ops: Vec = vec![ - Touch { - idx: 11805740887947177360, - }, - EditFile { - idx: 16172331740552606779, - content_seed: 658362763, - }, - EditFile { - idx: 15583428591709383853, - content_seed: 959932347, - }, - CreateFile { - seed: 3931677194, - content_seed: 3741630760, - }, - RenameFile { - idx: 10904465323748885299, - new_seed: 2279324670, - }, - EditFile { - idx: 9420001873471413265, - content_seed: 2114867802, - }, - EditFile { - idx: 213141289566213885, - content_seed: 560732827, - }, - EditFile { - idx: 17035572278797292009, - content_seed: 3808313786, - }, - CreateFile { - seed: 1288265487, - content_seed: 1533838042, - }, - EditFile { - idx: 16473729680435813293, - content_seed: 1008536355, - }, - Noop, - EditFile { - idx: 5398337378624802944, - content_seed: 1867267587, - }, - EditFile { - idx: 8551649030560368528, - content_seed: 2795207371, - }, - EditFile { - idx: 481097026290699171, - content_seed: 2327194851, - }, - EditFile { - idx: 16127721863741462430, - content_seed: 259019344, - }, - EditFile { - idx: 12499722135620715652, - content_seed: 710330684, - }, - Noop, - EditFile { - idx: 8570312264332927846, - content_seed: 20660474, - }, - EditFile { - idx: 8235918596759853707, - content_seed: 3895597068, - }, - EditFile { - idx: 12941454977835821598, - content_seed: 974440776, - }, - CreateFile { - seed: 2370169947, - content_seed: 3173097146, - }, - EditFile { - idx: 17976810772660212547, - content_seed: 1324367109, - }, - EditFile { - idx: 9502968218335991677, - content_seed: 4023431064, - }, - EditFile { - idx: 6867642529007482379, - content_seed: 207981211, - }, - CreateFile { - seed: 570877898, - content_seed: 1583247379, - }, - EditFile { - idx: 7954117886944662117, - content_seed: 1193244670, - }, - EditFile { - idx: 2859309324154804229, - content_seed: 669423541, - }, - EditFile { - idx: 1701666328639270148, - content_seed: 3533870941, - }, - EditFile { - idx: 9409088379085591352, - content_seed: 132132514, - }, - CreateSubdirFile { - dir_seed: 16362, - file_seed: 52030, - content_seed: 1374259669, - }, - EditFile { - idx: 13262913709196533433, - content_seed: 2320968118, - }, - EditFile { - idx: 17763858399044545684, - content_seed: 356795223, - }, - GitAddAll, - GitAddAll, - EditFile { - idx: 15775096968456010165, - content_seed: 1772666100, - }, - CreateFile { - seed: 2452057527, - content_seed: 4111477598, - }, - EditFile { - idx: 10711976377011651566, - content_seed: 3912798245, - }, - EditFile { - idx: 9546828938565340524, - content_seed: 172130614, - }, - EditFile { - idx: 13343766455516594408, - content_seed: 4263257980, - }, - EditFile { - idx: 3019554710255922240, - content_seed: 2030698787, - }, - EditFile { - idx: 10405611055126420472, - content_seed: 1193589142, - }, - EditFile { - idx: 17138669581083326405, - content_seed: 2903728507, - }, - GitignoreAppend { - pattern_seed: 54248, - }, - EditFile { - idx: 13072623131577163943, - content_seed: 1483932899, - }, - DeleteFile { - idx: 12869318557852831815, - }, - EditFile { - idx: 6824133502332012157, - content_seed: 3150001556, - }, - EditFile { - idx: 14438183751086260985, - content_seed: 3158759791, - }, - EditFile { - idx: 3246005478687480560, - content_seed: 1237031794, - }, - EditFile { - idx: 18419405778222487428, - content_seed: 564314178, - }, - DeleteFile { - idx: 5831672858052749534, - }, - GitCommit { msg_seed: 6624 }, - RenameFile { - idx: 9595327710957667341, - new_seed: 3574446333, - }, - EditFile { - idx: 11366319046220637326, - content_seed: 3919865486, - }, - CreateFile { - seed: 906013144, - content_seed: 1799214899, - }, - EditFile { - idx: 6771264237902804639, - content_seed: 2901771864, - }, - EditFile { - idx: 5877801695508394108, - content_seed: 3324479019, - }, - CreateSubdirFile { - dir_seed: 47588, - file_seed: 6835, - content_seed: 4279222285, - }, - GitignoreAppend { - pattern_seed: 41743, - }, - GitCommit { msg_seed: 63391 }, - ]; + let ops = ops_from_chacha_seed(REGRESSION_SEED_HEX, 30, 60); + assert_eq!( + (ops.len(), fingerprint_ops(&ops)), + (59, 0xc73f_16ce_b249_78eb), + "ops_strategy() changed shape: the pinned seed no longer decodes to \ + the original Windows-CI scenario. Either revert the strategy change \ + or re-pin this regression (the original literal op list is in git \ + history of this file).", + ); run_stress_scenario(&ops); } +/// 32-byte ChaCha seed persisted by proptest for the Windows CI failure +/// (the `cc 2c9d...` entry in the regressions file). +const REGRESSION_SEED_HEX: &str = + "2c9d1ea2efbf6161f84b69598e884dbf1bde6039c70625adde0374817e20e2ea"; + +/// Regenerate an op sequence from a persisted proptest ChaCha seed by +/// replaying `ops_strategy()` the same way proptest does for regressions. +/// `min`/`max` must match the `FFF_STRESS_{MIN,MAX}_OPS` bounds that were +/// in effect when the seed was persisted — the strategy's value tree +/// depends on them. +fn ops_from_chacha_seed(seed_hex: &str, min: usize, max: usize) -> Vec { + let seed_bytes: Vec = (0..seed_hex.len() / 2) + .map(|i| u8::from_str_radix(&seed_hex[2 * i..2 * i + 2], 16).expect("valid hex seed")) + .collect(); + let mut config = proptest_config(); + config.failure_persistence = Some(Box::new(FileFailurePersistence::Off)); + let rng = TestRng::from_seed(RngAlgorithm::ChaCha, &seed_bytes); + let mut runner = TestRunner::new_with_rng(config, rng); + ops_strategy_bounded(min, max) + .new_tree(&mut runner) + .expect("ops_strategy::new_tree") + .current() +} + +/// FNV-1a over the debug repr of the ops; stable across platforms and runs. +fn fingerprint_ops(ops: &[AbstractOp]) -> u64 { + let mut h = 0xcbf2_9ce4_8422_2325u64; + for b in format!("{ops:?}").bytes() { + h ^= b as u64; + h = h.wrapping_mul(0x0000_0100_0000_01b3); + } + h +} + /// Parse `FFF_STRESS_SEED` as either decimal or `0x`-prefixed hex. fn parse_stress_seed() -> u64 { match std::env::var("FFF_STRESS_SEED") { diff --git a/crates/fff-core/tests/fuzz_real_repos.rs b/crates/fff-core/tests/fuzz_real_repos.rs index f83a0f520..b06851a36 100644 --- a/crates/fff-core/tests/fuzz_real_repos.rs +++ b/crates/fff-core/tests/fuzz_real_repos.rs @@ -3,17 +3,6 @@ //! Clones real repository, runs the simulated close to real user sereies of file system ewvents and //! verifies that fff can still find the correct files. Test cases are randomized and preserved //! using proptest -//! -//! Run: -//! ```sh -//! RUSTFLAGS="--cfg stress" cargo test -p fff-search --test fuzz_real_repos -- --nocapture -//! ``` -//! -//! Increase coverage: -//! ```sh -//! FFF_FUZZ_CASES=4 FFF_FUZZ_MAX_OPS=60 \ -//! RUSTFLAGS="--cfg stress" cargo test -p fff-search --test fuzz_real_repos -- --nocapture -//! ``` #![cfg(stress)] use std::fs; @@ -218,10 +207,6 @@ fn revert_marker(path: &Path, marker: &str, original_line: &str) { let _ = fs::write(path, result); } -// ═══════════════════════════════════════════════════════════════════════════ -// Search helpers -// ═══════════════════════════════════════════════════════════════════════════ - fn grep_opts(mode: GrepMode) -> GrepSearchOptions { GrepSearchOptions { max_file_size: 10 * 1024 * 1024, @@ -689,35 +674,22 @@ fn proptest_config() -> ProptestConfig { #[derive(Debug, Clone)] enum Op { - /// Create a new file with a unique marker CreateFile { seed: u32 }, - /// Edit a tracked file, replacing the marker line with a new marker EditTracked { seed: u32 }, - /// Edit a random repo file, injecting a marker at a deterministic line EditRandom { seed: u32 }, - /// Delete a tracked file DeleteTracked, - /// Revert a tracked edit, restoring the original line (marker disappears) RevertTracked, - /// Burst of writes into ignored directory IgnoredBurst { count: u8, seed: u32 }, - /// Search verification round (no mutation) Verify, } fn op_strategy() -> impl Strategy { prop_oneof![ - // Create new files — exercises overflow path 12 => any::().prop_map(|s| Op::CreateFile { seed: s }), - // Edit tracked files — exercises content invalidation 18 => any::().prop_map(|s| Op::EditTracked { seed: s }), - // Edit random repo files — exercises bigram overlay for base files 18 => any::().prop_map(|s| Op::EditRandom { seed: s }), - // Delete tracked files — exercises tombstoning 8 => Just(Op::DeleteTracked), - // Revert tracked edits — marker must disappear from search 10 => Just(Op::RevertTracked), - // Burst ignored writes — exercises .gitignore filtering under load 9 => (1u8..20, any::()).prop_map(|(c, s)| Op::IgnoredBurst { count: c, seed: s }), // Explicit verification rounds 25 => Just(Op::Verify), diff --git a/package.json b/package.json index c3baa0c64..5369af53e 100644 --- a/package.json +++ b/package.json @@ -5,11 +5,11 @@ "extensions": ["./packages/pi-fff/src/index.ts"] }, "scripts": { - "format": "biome format --write", - "format:check": "biome format", - "lint": "biome lint", - "check": "biome check --write", - "check:ci": "biome check" + "format": "biome format --write packages", + "format:check": "biome format packages", + "lint": "biome lint packages", + "check": "biome check --write packages", + "check:ci": "biome check packages" }, "devDependencies": { "@biomejs/biome": "^2.4.4" diff --git a/packages/fff-bun/src/fff-api.ts b/packages/fff-bun/src/fff-api.ts index 976e6bcce..d8842d184 100644 --- a/packages/fff-bun/src/fff-api.ts +++ b/packages/fff-bun/src/fff-api.ts @@ -1,6 +1,6 @@ // ---------------------------------------------------------------------------- // GENERATED FILE - DO NOT EDIT. -// Source of truth: packages/shared/fff-api.ts +// Copied from: packages/shared/fff-api.ts // Run make sync-js-api from the repo root to regenerate. // ---------------------------------------------------------------------------- @@ -49,23 +49,17 @@ export interface InitOptions { frecencyDbPath?: string; /** Path to query history database (optional, omit to skip query tracker initialization) */ historyDbPath?: string; - /** - * @deprecated No-op. The no-lock LMDB flags showed no measurable win under - * realistic contention and are now ignored. Kept for source-compat. - */ + /** @deprecated no-op */ useUnsafeNoLock?: boolean; /** * Disable mmap cache warmup after the initial scan. When mmap cache is * enabled (the default), the first grep search is as fast as subsequent * ones at the cost of a longer scan time and higher initial memory usage. - * (default: false) */ disableMmapCache?: boolean; /** * Disable the content index built after the initial scan. * Content indexing enables faster content-aware filtering during grep. - * When omitted, follows `disableMmapCache` for backward compatibility. - * (default: follows `disableMmapCache`) */ disableContentIndexing?: boolean; /** @@ -97,10 +91,10 @@ export interface InitOptions { /** Override for the per-file byte cap in the content cache. */ cacheBudgetMaxFileSize?: number; /** - * Allow indexing the filesystem root (`/`). Off by default, having fff instance at the large folder - * will generally require file watcher - * */ - + * Allow indexing the filesystem root (`/`). + * Off by default, having fff instance at the large folder will generally require + * file watcher and indexing which will consume a lot of resources if performed uncontrolled + **/ enableFsRootScanning?: boolean; /** Allow indexing the user's home directory. Same trade-off as `enableFsRootScanning`. */ enableHomeDirScanning?: boolean; diff --git a/packages/fff-node/src/fff-api.ts b/packages/fff-node/src/fff-api.ts index 976e6bcce..d8842d184 100644 --- a/packages/fff-node/src/fff-api.ts +++ b/packages/fff-node/src/fff-api.ts @@ -1,6 +1,6 @@ // ---------------------------------------------------------------------------- // GENERATED FILE - DO NOT EDIT. -// Source of truth: packages/shared/fff-api.ts +// Copied from: packages/shared/fff-api.ts // Run make sync-js-api from the repo root to regenerate. // ---------------------------------------------------------------------------- @@ -49,23 +49,17 @@ export interface InitOptions { frecencyDbPath?: string; /** Path to query history database (optional, omit to skip query tracker initialization) */ historyDbPath?: string; - /** - * @deprecated No-op. The no-lock LMDB flags showed no measurable win under - * realistic contention and are now ignored. Kept for source-compat. - */ + /** @deprecated no-op */ useUnsafeNoLock?: boolean; /** * Disable mmap cache warmup after the initial scan. When mmap cache is * enabled (the default), the first grep search is as fast as subsequent * ones at the cost of a longer scan time and higher initial memory usage. - * (default: false) */ disableMmapCache?: boolean; /** * Disable the content index built after the initial scan. * Content indexing enables faster content-aware filtering during grep. - * When omitted, follows `disableMmapCache` for backward compatibility. - * (default: follows `disableMmapCache`) */ disableContentIndexing?: boolean; /** @@ -97,10 +91,10 @@ export interface InitOptions { /** Override for the per-file byte cap in the content cache. */ cacheBudgetMaxFileSize?: number; /** - * Allow indexing the filesystem root (`/`). Off by default, having fff instance at the large folder - * will generally require file watcher - * */ - + * Allow indexing the filesystem root (`/`). + * Off by default, having fff instance at the large folder will generally require + * file watcher and indexing which will consume a lot of resources if performed uncontrolled + **/ enableFsRootScanning?: boolean; /** Allow indexing the user's home directory. Same trade-off as `enableFsRootScanning`. */ enableHomeDirScanning?: boolean; diff --git a/tests/programmatic_search_spec.lua b/tests/programmatic_search_spec.lua index ff2b58a62..f21fb8e3d 100644 --- a/tests/programmatic_search_spec.lua +++ b/tests/programmatic_search_spec.lua @@ -1,16 +1,21 @@ ---@diagnostic disable: undefined-field, missing-fields +local fff = require('fff') +local fff_rust = require('fff.rust') +local file_picker = require('fff.file_picker') + local plugin_dir = vim.fn.fnamemodify(vim.fn.resolve(debug.getinfo(1, 'S').source:sub(2)), ':h:h') local log_file = vim.fs.normalize(plugin_dir .. '/fff-test.log') -pcall(vim.fn.delete, log_file) --- init_tracing uses OnceLock — first caller wins. Direct rust call BEFORE any --- fff.* require, otherwise core.ensure_initialized() locks tracing to the --- default config path and our trace dump on CI failure stays empty. -pcall(require('fff.rust').init_tracing, log_file, 'trace') +local ok, session_log = pcall(require('fff.rust').init_tracing, log_file, 'trace') +if not ok then session_log = nil end -local fff = require('fff') -local fff_rust = require('fff.rust') -local file_picker = require('fff.file_picker') +-- comment to get a local log of this file +vim.api.nvim_create_autocmd('VimLeavePre', { + once = true, + callback = function() + if os.getenv('CI') == nil and session_log then pcall(vim.fn.delete, session_log) end + end, +}) local function init_picker_at_plugin_dir(timeout_ms) fff_rust.init_file_picker(plugin_dir)