diff --git a/crates/fff-core/src/file_picker.rs b/crates/fff-core/src/file_picker.rs index 18a213133..6e8e8f514 100644 --- a/crates/fff-core/src/file_picker.rs +++ b/crates/fff-core/src/file_picker.rs @@ -2198,6 +2198,24 @@ pub struct ScanProgress { } impl FileSync { + pub(crate) fn classify_binary_files(&self, base_path: &Path, cancelled: &AtomicBool) { + let arena = self.arena_base_ptr(); + let files = self.files(); + // Leave room in the background pool for watcher work and other scans. + let chunk_size = files.len().div_ceil(2).max(256); + BACKGROUND_THREAD_POOL.install(|| { + files.par_chunks(chunk_size).for_each(|files| { + crate::index::sniff_binary_for_non_indexable( + files, + base_path, + arena, + cancelled, + |file| file.size, + ); + }); + }); + } + pub(crate) fn discover_git(base_path: &Path, disabled: bool) -> Option { if disabled { return None; diff --git a/crates/fff-core/src/index/bigram_filter.rs b/crates/fff-core/src/index/bigram_filter.rs index 9017cd441..26406da34 100644 --- a/crates/fff-core/src/index/bigram_filter.rs +++ b/crates/fff-core/src/index/bigram_filter.rs @@ -1174,6 +1174,7 @@ pub(crate) fn sniff_binary_for_non_indexable( base_path: &std::path::Path, arena: crate::simd_path::ArenaPtr, cancelled: &std::sync::atomic::AtomicBool, + read_limit: impl Fn(&FileItem) -> u64, ) { // Non-indexable files are few in a typical repo, so a serial pass with a // single reused chunk buffer beats spinning up the thread pool. @@ -1188,12 +1189,12 @@ pub(crate) fn sniff_binary_for_non_indexable( } // check only the files that we are able to grep - if file.size == 0 || file.size > constants::MAX_FFFILE_SIZE { + if file.is_binary() || file.size == 0 || file.size > constants::MAX_FFFILE_SIZE { continue; } let abs = file.write_absolute_path(arena, base_path, &mut path_buf); - file.detect_binary_per_byte(abs, &mut chunk); + file.detect_binary_with_limit(abs, &mut chunk, read_limit(file)); } } diff --git a/crates/fff-core/src/scan.rs b/crates/fff-core/src/scan.rs index 6af44a3a0..3433d76b3 100644 --- a/crates/fff-core/src/scan.rs +++ b/crates/fff-core/src/scan.rs @@ -163,7 +163,7 @@ impl ScanJob { trace_span: _, } = self; - let _scanning = ScanningGuard::new(&signals); + let mut scanning = Some(ScanningGuard::new(&signals)); scanned_files_counter.store(0, Ordering::Relaxed); // 1. Walk the file system and collect the list of files @@ -184,6 +184,10 @@ impl ScanJob { } }; + if !config.content_indexing { + sync.classify_binary_files(&base_path, &signals.cancelled); + } + // 2. Populate the file list let git_status_worker; if let Ok(mut guard) = shared_picker.write() @@ -213,9 +217,8 @@ impl ScanJob { 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) { + // Pin the index before clearing the scan signal so a rescan cannot replace it. + let snapshot = if config.content_indexing && !signals.cancelled.load(Ordering::Acquire) { shared_picker.read().ok().and_then(|guard| { guard .as_ref() @@ -225,7 +228,9 @@ impl ScanJob { None }; - signals.scanning.store(false, Ordering::Relaxed); // file are searchable + if config.content_indexing { + drop(scanning.take()); + } // 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 (this is internally deduped) @@ -233,7 +238,9 @@ impl ScanJob { rescubscribe_watcher_post_scan(&shared_picker); } - // 3. Runs post scna in parallel with git status collection + drop(scanning); + + // 3. Build the content index in parallel with git status collection. if !signals.cancelled.load(Ordering::Acquire) && let Some(snap) = snapshot.as_ref() { @@ -300,11 +307,7 @@ impl ScanJob { } } - /// THIS IS VERY VERY IMPORTANT THAT ANYTHING INSIDE THIS FUNCTION TO NOT READ ANYTHING CLEARABLE OUTSIDE - /// this is a very silly off lock implementation that actually matters, and that's why it is crafted - /// to never read anything from the picker, it can only WRITE information using single instructions - /// - /// Things that are safe and immutable - file list, indexes of files, paths, and signals. + // The snapshot pins files and paths while the index builds off-lock. #[tracing::instrument(skip_all, fields(warmup = ?config.warmup, indexing = ?config.content_indexing))] fn run_post_scan( shared_picker: &SharedFilePicker, @@ -322,43 +325,30 @@ impl ScanJob { }; let files: &[crate::types::FileItem] = &unsafe_snapshot.files[..unsafe_snapshot.base_count]; - if signals.cancelled.load(Ordering::Acquire) { + if !config.content_indexing || signals.cancelled.load(Ordering::Acquire) { return; } - if config.content_indexing { - let indexable_count = unsafe_snapshot.indexable_count.min(files.len()); - let (indexable_files, non_indexable_files) = files.split_at(indexable_count); - let index = build_bigram_index(indexable_files, &unsafe_snapshot.base_path, arena); - - // A root change may have replaced the picker while this index was building. - if let Ok(mut guard) = shared_picker.write() - && let Some(picker) = guard.as_mut() - && !signals.cancelled.load(Ordering::Acquire) - && picker.base_path() == unsafe_snapshot.base_path - { - picker.set_bigram_index(index); - } - crate::index::release_thread_buffers(); - - // Bigram only sniffs files <= MAX_INDEXABLE_FILE_SIZE; large - // unknown-extension binaries slip past it and would otherwise be - // grep-able as text. Cheap header sniff catches those. - if !signals.cancelled.load(Ordering::Acquire) { - sniff_binary_for_non_indexable( - non_indexable_files, - &unsafe_snapshot.base_path, - arena, - &signals.cancelled, - ); - } - } else { - // this potentially a long running as we are not parallelizing it but it's okay + let indexable_count = unsafe_snapshot.indexable_count.min(files.len()); + let (indexable_files, non_indexable_files) = files.split_at(indexable_count); + let index = build_bigram_index(indexable_files, &unsafe_snapshot.base_path, arena); + + if let Ok(mut guard) = shared_picker.write() + && let Some(picker) = guard.as_mut() + && !signals.cancelled.load(Ordering::Acquire) + && picker.base_path() == unsafe_snapshot.base_path + { + picker.set_bigram_index(index); + } + crate::index::release_thread_buffers(); + + if !signals.cancelled.load(Ordering::Acquire) { sniff_binary_for_non_indexable( - files, + non_indexable_files, &unsafe_snapshot.base_path, arena, &signals.cancelled, + |_| crate::constants::MAX_FFFILE_SIZE, ); } @@ -413,11 +403,11 @@ mod tests { use super::*; use crate::file_picker::{FilePicker, FilePickerOptions}; - fn collected(root: &std::path::Path) -> FilePicker { + fn collected(root: &std::path::Path, content_indexing: bool) -> FilePicker { let mut picker = FilePicker::new(FilePickerOptions { base_path: root.to_string_lossy().into_owned(), enable_mmap_cache: false, - enable_content_indexing: true, + enable_content_indexing: content_indexing, watch: false, ..Default::default() }) @@ -426,6 +416,23 @@ mod tests { picker } + #[test] + fn disabled_content_indexing_skips_bigram_build() { + let root = tempfile::tempdir().unwrap(); + std::fs::write(root.path().join("text.txt"), "some words").unwrap(); + + let shared = SharedFilePicker::default(); + let picker = collected(root.path(), false); + let signals = picker.scan_signals(); + let snapshot = unsafe { picker.post_scan_snapshot() }.unwrap(); + *shared.write().unwrap() = Some(picker); + + ScanJob::run_post_scan(&shared, &signals, &ScanConfig::default(), &snapshot); + + let guard = shared.read().unwrap(); + assert!(guard.as_ref().unwrap().bigram_index().is_none()); + } + #[test] fn replaced_picker_does_not_install_its_bigram_index_into_the_successor() { let old_root = tempfile::tempdir().unwrap(); @@ -436,7 +443,7 @@ mod tests { std::fs::write(new_root.path().join("new.txt"), "isolated_marker").unwrap(); let shared = SharedFilePicker::default(); - let old = collected(old_root.path()); + let old = collected(old_root.path(), true); let signals = old.scan_signals(); *shared.write().unwrap() = Some(old); let snapshot = shared @@ -446,7 +453,10 @@ mod tests { .and_then(|picker| unsafe { picker.post_scan_snapshot() }) .unwrap(); // The root changes while the old post-scan is still building its index. - let old = shared.write().unwrap().replace(collected(new_root.path())); + let old = shared + .write() + .unwrap() + .replace(collected(new_root.path(), true)); let config = ScanConfig { warmup: false, content_indexing: true, diff --git a/crates/fff-core/src/types.rs b/crates/fff-core/src/types.rs index 9f166400d..f5b686074 100644 --- a/crates/fff-core/src/types.rs +++ b/crates/fff-core/src/types.rs @@ -609,6 +609,10 @@ impl FileItem { /// Chunked classifier of the binary content of the file chunk by chunk /// accepts path which to reuse the allocated buffer for absolute path read pub(crate) fn detect_binary_per_byte(&self, path: &Path, chunk: &mut [u8]) { + self.detect_binary_with_limit(path, chunk, MAX_FFFILE_SIZE); + } + + pub(crate) fn detect_binary_with_limit(&self, path: &Path, chunk: &mut [u8], read_limit: u64) { if self.size == 0 { return; } @@ -621,8 +625,7 @@ impl FileItem { tracing::error!(path = ?path.display(), "Failed to open indexed file"); return; }; - // only classify the first MAX_FFFILE_SIZE bytes, never read large files to EOF - let mut file = file.take(MAX_FFFILE_SIZE); + let mut file = file.take(read_limit.min(MAX_FFFILE_SIZE)); loop { match file.read(chunk) { diff --git a/crates/fff-core/tests/real_binary_fixtures.rs b/crates/fff-core/tests/real_binary_fixtures.rs index 83f64cf00..ab158c900 100644 --- a/crates/fff-core/tests/real_binary_fixtures.rs +++ b/crates/fff-core/tests/real_binary_fixtures.rs @@ -18,7 +18,9 @@ use std::time::Duration; use fff_search::file_picker::{FFFMode, FilePicker}; use fff_search::grep::{GrepMode, GrepSearchOptions, parse_grep_query}; -use fff_search::{FilePickerOptions, SharedFilePicker, SharedFrecency}; +use fff_search::{ + FilePickerOptions, FuzzySearchOptions, QueryParser, SharedFilePicker, SharedFrecency, +}; const MARKER: &str = "__jai_runtime_init"; @@ -45,6 +47,74 @@ fn plain_opts() -> GrepSearchOptions { } } +#[test] +fn file_search_reports_binary_without_content_indexing() { + let tmp = tempfile::TempDir::new().unwrap(); + let blob = tmp.path().join("sample-blob.txt"); + let note = tmp.path().join("sample-note.txt"); + let text = vec![b'a'; 64 * 1024]; + let mut binary = text.clone(); + *binary.last_mut().unwrap() = 0; + fs::write(&blob, &binary).unwrap(); + fs::write(¬e, &text).unwrap(); + + let shared = SharedFilePicker::default(); + let frecency = SharedFrecency::noop(); + FilePicker::new_with_shared_state( + shared.clone(), + frecency.clone(), + FilePickerOptions { + base_path: tmp.path().to_string_lossy().into_owned(), + enable_content_indexing: false, + watch: false, + mode: FFFMode::Ai, + ..Default::default() + }, + ) + .unwrap(); + + let file_flags = || { + assert!(shared.wait_for_scan(Duration::from_secs(10))); + let guard = shared.read().unwrap(); + let picker = guard.as_ref().unwrap(); + let query = QueryParser::default().parse("sample"); + let result = picker.fuzzy_search(&query, None, FuzzySearchOptions::default()); + let mut flags: Vec<_> = result + .items + .iter() + .map(|file| (file.relative_path(picker), file.is_binary())) + .collect(); + flags.sort_unstable(); + flags + }; + assert_eq!( + file_flags(), + [ + ("sample-blob.txt".to_owned(), true), + ("sample-note.txt".to_owned(), false), + ] + ); + + for (path, replacement) in [(&blob, &text), (¬e, &binary)] { + let mtime = fs::metadata(path).unwrap().modified().unwrap(); + fs::write(path, replacement).unwrap(); + fs::File::options() + .write(true) + .open(path) + .unwrap() + .set_modified(mtime) + .unwrap(); + } + shared.trigger_full_rescan_async(&frecency).unwrap(); + assert_eq!( + file_flags(), + [ + ("sample-blob.txt".to_owned(), false), + ("sample-note.txt".to_owned(), true), + ] + ); +} + #[test] fn real_binary_fixtures_are_detected_and_excluded_from_grep() { let fixtures = fixtures_dir();