Fix: windows mmap and search correctness

[Shengyu Fu (shengyfu)]

fix: Windows mmap correctness and search reliability (v0.1.17)

Summary

Fixes a critical bug where ~98% of trigram searches returned 0 candidates on Windows after an index flush cycle. Root cause: NTFS metadata caching causes File::metadata().len() to transiently return 0 after std::fs::rename, producing zero-length mmaps for lookup.bin and index.bin. Also fixes several correctness bugs in search query planning, glob filtering, and reader snapshot consistency discovered during investigation.

Impact: Confirmed fix against a 285K-file / 921K-trigram production index. Searches now return valid candidates after flush.

Changes

fix: three correctness bugs in trigram search (aed3202)

• expected_next used raw regex pattern instead of parsed literal bytes — next_byte_after_trigram was called with the user's regex string, not the extracted literal. Introduced TrigramQuery struct with expected_next computed at plan-build time from actual literal bytes.
• Reader snapshot race during concurrent flush — searches could see a partially-swapped reader mid-flush. Now snapshots the Arc at search start.
• Mask data lost through snapshot/flush cycles — write_index_from_snapshot discarded loc_mask/next_mask, writing 0xFF for all entries. Now preserves PostingEntry masks end-to-end.
• O(N²) regression in full_snapshot() — added get_masks() O(1) accessor to LiveIndex to avoid scanning the overlay per posting.

fix: prevent degenerate reader swap after flush on Windows (9cc2360)

• Use fstat (open file handle) instead of stat (path-based) for file size checks — avoids stale NTFS metadata.
• Add is_degenerate() validation to detect structurally inconsistent readers (mmap sections present but counters say zero entries).
• Retry with backoff in flush path when a newly-opened reader is degenerate.

fix: bypass stale metadata in IndexReader::open and warm lookup mmap (df2d6f5)

This is the primary fix for the zero-candidate bug.

• Root cause: File::metadata().len() returns 0 after rename() on Windows NTFS due to stale metadata cache. memmap2::Mmap::map() also calls metadata() internally, so even mapping first doesn't help.
• Fix: Use file.seek(SeekFrom::End(0)) to read the true file size from the kernel file object (calls SetFilePointerEx on Windows), then MmapOptions::new().len(size) to create the mmap with an explicit size, bypassing memmap2's internal metadata path.
• Add validate_lookup() method that validates sort order + posting-list bounds and warms all lookup.bin pages into the working set.
• Call validation at all reader-open paths: startup, flush, auto-save.
• Use usize::try_from() instead of as usize for file sizes to prevent silent truncation on 32-bit targets.

fix: consistent reader snapshot for query + path resolution (d8f817a)

• execute_query_with_masks now returns the Arc<IndexReader> snapshot alongside file IDs.
• Search handler uses that snapshot for path resolution instead of acquiring a fresh one, eliminating a race where a concurrent swap_reader between query and path resolution would silently drop all candidates.

fix: glob filter correctness (f851c0c, 9f48a25, 72bf727)

• Negation pattern handling: The runtime sends glob patterns like !.git meaning "exclude .git paths". The old simple_glob_match treated !.git as a literal pattern, rejecting ALL candidates. New glob_filter_matches() splits patterns into include/exclude lists.
• Windows backslash normalization: Normalize \ to / in glob patterns before regex matching so Windows-style globs work with forward-slash index paths.
• Aligned local and server glob semantics: Updated passes_glob_filters in search.rs to support ! prefix negation and backslash normalization, matching the server-side glob_filter_matches implementation.

fix: safe UTF-8 truncation for pattern preview (439fdad)

• &pattern[..N] byte-index slice panics on multi-byte UTF-8. Use chars().take(40) instead.

fix: remove unnecessary OVERLAY_BIT masking from trigram keys (806566c)

• OVERLAY_BIT is only meaningful for file IDs; trigram hashes use only the low 24 bits. The masking was a no-op that created a hidden coupling.

chore: reduce diagnostic log volume (bccc7fe, 72bf727)

• Remove per-search filter diagnostics that fired on ~98% of normal searches.
• Remove unconditional eprintln! trace in execute_plan_with_masks that spammed stderr on normal no-match queries.
• Retain narrow-condition logging for actual corruption/degenerate indicators.

chore: improve test coverage and reliability

• Use assert_cmd::cargo_bin() instead of manual binary path derivation in integration tests.
• Redirect server stderr to null to prevent pipe buffer deadlock under concurrent load.
• is_degenerate test now fabricates inconsistent IndexReader state to exercise the positive path (not just non-degenerate cases).
• Add validate_lookup bounds-checking in u64 to prevent truncation on 32-bit targets.

chore: bump version to 0.1.17 (517dc2d)

Testing

• All 108 tests pass (46 core unit + 1 case-insensitive roundtrip + 5 snapshot consistency + 6 concurrent search integration + 17 CLI ripgrep-compat + 33 CLI unit).
• Verified against large index: searches return 1K–99K candidates after flush where they previously returned 0.

Files changed

File	Summary
tgrep-core/src/reader.rs	seek-based file size, MmapOptions::len(), validate_lookup() (u64 math), is_degenerate(), usize::try_from() for sizes, 7 new tests
tgrep-core/src/query.rs	TrigramQuery struct, literals_to_query_plan(), mask filtering, removed unconditional trace log
tgrep-core/src/hybrid.rs	Reader snapshot helpers, validate_lookup() + degenerate check at open, full_snapshot with masks, removed OVERLAY_BIT masking
tgrep-core/src/live.rs	get_masks() O(1) accessor
tgrep-core/src/builder.rs	write_index_from_snapshot preserves PostingEntry masks
tgrep-core/src/ondisk.rs	PostingEntry encode/decode helpers
tgrep-core/src/gitignore.rs	New: gitignore matcher for watcher filtering
tgrep-core/src/filetypes.rs	Added csproj to builtin file types
tgrep-cli/src/serve.rs	Flush retry + validation, auto-save validation, glob negation + backslash normalization, reduced log noise, filter stage breakdown diagnostics, safe UTF-8 truncation
tgrep-cli/src/search.rs	Aligned glob filter semantics (negation + backslash normalization)
tgrep-cli/tests/concurrent_search.rs	New: 6 concurrent search integration tests
tgrep-core/tests/snapshot_consistency.rs	New: 5 snapshot consistency tests
tgrep-core/tests/case_insensitive_roundtrip.rs	Updated for PostingEntry API
Cargo.toml / Cargo.lock	Version bump 0.1.16 → 0.1.17

[Copilot]

Pull request overview

This PR targets Windows index reliability by fixing a zero-candidate search failure after flush (degenerate zero-length mmaps) and tightening trigram query planning/execution correctness, including mask preservation across snapshot/flush cycles.

Changes:

• Update IndexReader to open mmaps using explicit lengths (seek-based sizing), add degenerate-reader detection, and add lookup validation/warming.
• Introduce TrigramQuery with literal-derived expected_next, snapshot the active reader per search, and preserve posting masks end-to-end.
• Add retry/validation logic around reader reopen after flush and reduce noisy diagnostics; bump workspace version to 0.1.17.

Show a summary per file

File	Description
tgrep-core/src/reader.rs	Seek-based mmap sizing; add is_degenerate() and validate_lookup(); add tests
tgrep-core/src/query.rs	New TrigramQuery + expected-next handling; mask-aware execution changes
tgrep-core/src/hybrid.rs	Snapshot reader per query; full snapshot preserves masks
tgrep-core/src/live.rs	Add O(1) get_masks() accessor for snapshot/flush
tgrep-core/src/builder.rs	Persist PostingEntry masks when writing snapshot indexes
tgrep-cli/src/serve.rs	Use new mask-aware query API; add flush reopen retry + validation; reduce logging noise
tgrep-cli/src/search.rs	Update callers for new query API and plan summary
tgrep-core/tests/case_insensitive_roundtrip.rs	Update test to build masked postings
Cargo.toml / Cargo.lock	Version bump to 0.1.17

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comments suppressed due to low confidence (1)

tgrep-core/src/reader.rs:219

• validate_lookup() casts entry.offset (u64) and entry.length into usize before doing checked_add, so an out-of-range offset/length can truncate on 32-bit and incorrectly appear in-bounds. Consider performing the range math in u64 (using checked_mul/checked_add on u64) and comparing against postings_len as u64 to ensure corrupted offsets are reliably rejected.

                }
            }
            let byte_len = (entry.length as usize).checked_mul(POSTING_ENTRY_SIZE);
            let end = byte_len.and_then(|bl| (entry.offset as usize).checked_add(bl));
            match end {
                Some(e) if e <= postings_len => {}
                _ => {
                    return Err(format!(
                        "lookup entry {i} (trigram {:#x}): posting range \
                         [offset={}, length={}] exceeds index.bin length {postings_len}",
                        entry.trigram, entry.offset, entry.length
                    ));

• Files reviewed: 9/10 changed files
• Comments generated: 4

[Copilot]

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

• Files reviewed: 11/12 changed files
• Comments generated: 6

[Copilot]

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

• Files reviewed: 10/11 changed files
• Comments generated: 3

[Copilot]

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

• Files reviewed: 12/13 changed files
• Comments generated: 5

[Copilot]

validate_lookup() narrows entry.offset (u64) to usize via entry.offset as usize before bounds checking. On 32-bit targets or for corrupted indexes with large offsets, this cast can truncate and potentially let an out-of-bounds posting range pass validation. Prefer performing the range math and comparisons in u64 (against postings_len as u64), and only convert to usize after ensuring the values fit.

[Copilot]

is_degenerate_detects_mmap_counter_disagreement doesn’t actually exercise a degenerate reader state (it only asserts non-degenerate cases). As written, it can’t fail even if is_degenerate() is broken. Consider constructing a truly inconsistent reader state (e.g., by writing a non-empty lookup.bin while forcing num_entries/mmap length observed at open to be zero, or by directly instantiating IndexReader in a unit test if the module allows) and asserting is_degenerate() == true.

Suggested change

	// Fabricate a case where lookup.bin has data but num_entries is 0.
	// This simulates the stale-metadata corruption path.
	let (lookup, postings) = make_sorted_index(&[0x616263]);
	// Write non-empty lookup/postings but *no* files — after open,
	// num_entries > 0, so is_degenerate is false. That's fine; we need
	// to test the *other* direction: counters say 0 but mmaps are
	// non-empty. We can't easily create that via public API because
	// open() derives num_entries from lookup len. Instead, verify the
	// "files but empty sections" case is NOT degenerate (valid index).
	let files = ondisk::encode_file_entry(0, "a.rs").unwrap();
	write_index(tmp.path(), &[], &[], &files);
	let reader = IndexReader::open(tmp.path()).expect("should open");
	// Files present but empty lookup/postings is valid (files <3 bytes)
	assert!(
	!reader.is_degenerate(),
	"files with empty sections is valid, not degenerate"
	);
	// Non-empty sections with entries IS also not degenerate
	let tmp2 = TempDir::new().unwrap();
	write_index(tmp2.path(), &lookup, &postings, &files);
	let reader2 = IndexReader::open(tmp2.path()).expect("should open");
	assert!(
	!reader2.is_degenerate(),
	"well-formed index is not degenerate"
	let (lookup, postings) = make_sorted_index(&[0x616263]);
	let files = ondisk::encode_file_entry(0, "a.rs").unwrap();
	// Create a real non-empty mmap-backed reader first.
	write_index(tmp.path(), &lookup, &postings, &files);
	let opened = IndexReader::open(tmp.path()).expect("should open");
	assert!(
	opened.lookup.as_ref().map_or(0, |m| m.len()) >= LOOKUP_ENTRY_SIZE,
	"fixture should have a non-empty lookup mmap"
	);
	// Then fabricate the inconsistent internal state that open() cannot
	// produce: mmap-backed lookup/postings are present, but num_entries is
	// forced to 0. This should be considered degenerate.
	let reader = IndexReader {
	lookup: opened.lookup,
	postings: opened.postings,
	file_paths: opened.file_paths,
	num_entries: 0,
	};
	assert!(
	reader.is_degenerate(),
	"non-empty mmap with num_entries == 0 must be degenerate"

[Copilot]

execute_plan_with_masks now emits an unconditional eprintln! trace whenever an AND plan yields 0 candidates (and has >=3 trigrams). This will fire for normal “no matches” searches and can add significant stderr noise for both the CLI (local index) and server. Consider gating this behind an explicit debug/trace flag, a feature, or a logger with configurable level instead of always printing from the core query engine.

[Copilot]

Server-side glob filtering now implements include+exclude semantics with ! negation, but the local search path (tgrep-cli/src/search.rs’s passes_glob_filters/glob_matches) still uses simple “match any” inclusion semantics and doesn’t handle negation. This makes --glob behave differently depending on whether the query is served locally or via tgrep serve. Consider sharing a single glob-filter implementation (or keeping semantics aligned) to avoid surprising users.

[Copilot]

is_degenerate() currently only returns true when num_entries == 0 and an mmap is present/non-empty. Given open() derives num_entries directly from the lookup mmap length, this condition can’t occur in normal construction, and it does not match the call sites that treat “{files} files but 0 trigrams” as degenerate. Consider redefining this check to align with the intended Windows stale-metadata failure mode (e.g., compare against meta.json’s num_trigrams, or validate on-disk section sizes before/after mmap) so the retry/validation logic can actually trigger when needed.

Suggested change

	/// Returns `true` when the reader state is structurally inconsistent.
	///
	/// A well-formed index may legitimately contain files that produce no
	/// trigrams (for example, files shorter than 3 bytes), so
	/// `num_files() > 0 && num_trigrams() == 0` is not inherently
	/// degenerate. The zero-trigram case is only suspicious when one of the
	/// mmap-backed binary sections is nevertheless present and non-empty,
	/// indicating that the in-memory counters and on-disk metadata disagree
	/// (observed on Windows NTFS after rapid file renames when stale
	/// metadata causes a zero-length mmap despite non-empty files on disk).
	pub fn is_degenerate(&self) -> bool {
	self.num_entries == 0
	&& (self.lookup.as_ref().is_some_and(|m| !m.is_empty())
	|| self.postings.as_ref().is_some_and(|m| !m.is_empty()))
	/// Returns `true` when the reader is in the suspicious "files present
	/// but no trigrams loaded" state.
	///
	/// In normal operation, an index with files but zero trigrams can be
	/// legitimate (for example, when every file is shorter than 3 bytes).
	/// However, callers use this method as a heuristic to detect the stale
	/// Windows metadata failure mode where the reader observes indexed files
	/// but ends up with an empty trigram table and should therefore retry or
	/// force validation.
	pub fn is_degenerate(&self) -> bool {
	self.num_files() > 0 && self.num_trigrams() == 0

[maoquan-ms]

just approve