diff --git a/CHANGELOG.md b/CHANGELOG.md index 5b70e0bc..325232af 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -114,6 +114,26 @@ finalized in place with a date — no renaming/migration step needed. ### Fixed +- **Group fan-out hits resolve in their origin store, with the alias-prefixed path.** Chunk ids are per-repo counters, so across a group every repo answers for id 0 and any two non-empty repos collide. The fan-out helpers deduplicated by (alias, chunk_id) but flattened to bare results, so every downstream resolver probed all stores and kept the first get_chunk(id) hit — the alphabetically-first repo stole the hit, the true definition died in the KINDS filter, unrelated content rode the stolen score, and group paths never picked up their alias prefix. MultiReadOutcome now carries SourcedResult{alias, result} from the helpers themselves: find definition/usages, literal search (BM25 and scan paths), group semantic in all three modes ((alias, chunk_id) interned to unique synthetic ids for the bare-u32 RRF fusion), graph imports/dependents/similar, explore outline dedup and get_chunk auto-routing all resolve the chunk only in its origin store and prefix the path from that store's alias; merge_exact_into_fts gains a (alias, chunk_id)-keyed sibling for the group lexical path. +- **Mixed-model groups warn that scores are not comparable.** A group whose repos embed with different models answers with cosine distances from different vector spaces, which the merged ranking presents as one scale — an agent reading the top hit's score as "strongest match overall" is comparing coordinates from unrelated spaces. The group path now collects the distinct models that actually answered and appends a warning naming every one of them; RRF fusion in auto/hybrid partially hides the problem by fusing ranks only, so semantic_mode's raw cosine scores are where it bites hardest. +- **Literal search relaxes to OR when the exact AND query matches nothing.** Business queries arrive as sentences, and a chunk holding most of their terms is a better answer than silence. When the conjunctive pass returns nothing and the query carries at least two significant (non-stopword) terms, the search retries as a disjunction and keeps candidates covering 60% of those terms (minimum two); coverage counts terms in the stored searchable_text (signature + docstring + content — the same fields the relaxed pass queries and rescues by), with a bare-content fallback for legacy blobs, so a candidate rescued BY its signature is no longer dropped by a gate that only read its content. The response flags relaxed_fallback and explains the mode in a note. Single-term queries never relax, so an empty answer for an unknown identifier stays an honest refusal. +- **All responses emit 1-based line numbers.** Chunks carry 0-indexed start/end lines by design and every consumer emitted them raw — a symbol defined on the first line of a file showed start_line 0. The conversion happens exactly once at each final emit boundary (semantic funnel, literal match lines, find, explore outline, get_chunk, similar chunks, imports, CLI renderer); intermediate vectordb results, store metadata, chunk-internal context slicing and SCIP-based positions stay untouched, and the Chunk doc comment pins the convention so the offset cannot be applied twice. +- **get_chunk reads surrounding context from the owning repo's root.** Chunk paths are stored project-relative, and the group-scoped answer rewrites them with the alias prefix before the context read — which then joined the prefixed path onto the serve process's own project root, a location that never exists. Every group-mode get_chunk with context_lines therefore answered "source file not readable, returning indexed content only", always. The context read now resolves the pre-prefix stored path against the owning alias's root from the routing context (`alias_roots`), keeping the historical join only for stdio single-repo mode where it was already correct. +- **Optional min_score threshold for semantic search, with an explicit refusal.** Nearest-neighbour search always answers something, so junk queries got confidently scored garbage and negative probes looked like weak hits. Hits below the opt-in threshold are dropped in the single funnel every local mode shares, and an answer emptied by the filter carries a note naming the threshold instead of reading as "no match in the index"; scores are mode-specific (cosine similarity for semantic_mode="semantic", RRF points for auto/hybrid/lexical) and the parameter docs spell out both scales. Federated group/project queries now forward the threshold to every leg and surface each leg's refusal note in `warnings` (the local leg hardcoded None and the peer body omitted the field, so both silently ignored it); literal mode has no calibratable scale and refuses the parameter by name instead of silently dropping it. Default behaviour is unchanged. +- **Debounced config persist no longer clobbers racing registrations.** The persist worker cloned its config snapshot and wrote repos.json without the config lock, so a POST /repos landing in that window existed only in memory while the stale snapshot hit disk — every reader-side reload_if_changed then trusted the fresh mtime as an external edit and swapped the disk copy in, silently dropping the registration (alias 404s in /info while its build ended with "repo removed or cancelled mid-index"). persist_config now adopts its own write's mtime so an internal persist never masquerades as an external edit, and the persist pass re-verifies memory against the snapshot after each write, rewriting until the file converges. +- **DB directories that survive the delete budget are quarantined.** When remove_repo's delete retries expired with the directory still held by an external process, it stayed in place as a fully valid index: the next registration adopted its stale metadata.json as false readiness, group fan-outs served an empty or stale store, and re-registration against legacy data ended in arroy EINVAL. The survivor is renamed to a .codesearch.db.removed- sibling instead (atomic on unix; live holders keep their fds and exit with them), so a fresh registration builds from scratch; deferred cleanups owned by a running index task are excluded, and the DELETE response reports status "removed_db_quarantined" with a db_quarantined field so the caller sees the honest outcome. serve also sweeps the `.removed-*` remnants on startup (best-effort delete, failures warn): the sibling was left "for manual cleanup", and manual cleanup never happened — every budget-expired delete leaked a full stale index copy forever. +- **The db_discovery verbatim-path registration test is platform-independent.** register() stores the canonical form by contract, and on macOS the tempdir sits behind the /var -> /private/var TMPDIR symlink, so the byte-exact comparison against the raw path was testing an OS quirk rather than the invariant and always failed there. The input is pre-canonicalized (safe_canonicalize is idempotent on a canonical path), so any remaining byte difference is a genuine rewrite. +- **Crash-atomic file metadata with .bak recovery.** file_meta.json was written with plain truncate-and-write on the FSW update path that saves after every indexed file — a wide window for an unclean shutdown to leave a zero-byte or torn file. The pilot hit exactly this: warmup marked every file of a repo Changed (323/323 files fully re-embedded) or failed the incremental refresh outright, and a corrupt file never self-heals, so warmup kept skipping the repo until the file was deleted by hand. save now keeps one best-effort .bak generation, writes a pid+counter-unique temp sibling (the refresh task and FSW updates may save the same repo concurrently), fsyncs and renames with a bounded retry; load_or_create falls back to the .bak generation with a warning, and only when no generation parses does it degrade to an empty store — letting the callers' guard clear the vector/FTS stores deliberately and re-index instead of poisoning warmup with a permanent hard error. +- **codesearch setup --model persists the choice and serve adopts it.** setup downloaded the model and silently forgot it, so a bare codesearch serve fell back to the built-in default for every new index and the operator re-typed --model embeddinggemma-q4 on every serve launch. The choice persists to /default_model.json (tmp+fsync+rename) and serve adopts it when its own --model flag is absent (the flag still wins); serve also runs an offline pre-flight probe of the effective default (flag > persisted > built-in) against the hf-hub cache layout fastembed downloads into, warning up front with the setup hint instead of surfacing the absence mid-warmup as parallel index jobs hitting the network behind a proxy. +- **Managed post-checkout hooks are regenerated on serve startup.** codesearch hooks git install was the only writer, so blocks written by older binaries stayed stale in every repo forever — the pilot kept 515 of them, and the only upgrade path was re-running the install command per repo by hand. serve now refreshes codesearch-managed blocks for all registered repos in a background spawn_blocking pass at startup: an outdated managed region is rewritten to this build's block in place (idempotent), repos with no hook or a foreign hook are left untouched, failures warn and never block startup. +- **--global index stores live under CODESEARCH_HOME with collision-proof slugs.** They leaked into a hardcoded ~/.codesearch.dbs that ignored the CODESEARCH_HOME relocation knob every other global artifact honours, and named each store directory by the bare project file_name — two same-named projects at different paths silently shared one store. New stores go to /dbs/-, with the historic layout still winning when it already exists so old indexes stay reachable; get_global_db_path no longer registers the project in repos.json (discovery and serve only ever look at /.codesearch.db, so a running serve built a second, empty local store while the global one sat unused), and rm detects a real global store by existence and actually deletes its files instead of printing Global-index-removed over surviving data. +- **Per-model embedding init locks and a bounded model-load timeout (CODESEARCH_MODEL_LOAD_TIMEOUT_SECS).** A cold-cache model resolves through hf-hub, and on networks that black-hole the model host that fetch never completes and never errors; the old get() held the services map lock across the whole load, so one wedged fetch poisoned every embedding query — the whole-night serve stall. The map lock now covers lookups only, loading runs under a per-model init lock (double-checked after acquisition), and the load is bounded (default 600 s), failing fast with a hint to pre-populate the cache via codesearch setup; on timeout the loader thread is leaked deliberately — it holds no pool locks. Search callers get get_if_cached(), a pure lookup that never initialises. +- **Interactive fan-outs try-lock past busy stores instead of queueing behind indexing.** Group fan-outs waited up to STORE_LOCK_WAIT_SECS (300 s) per busy store and waits sum — a handful of repos held by an indexing run stalled every other repo answer. Every interactive multi-repo fan-out now try-locks each store through one shared helper and skips busy repos with a warning naming them: semantic search in all modes, find (definition/usages, imports, dependents), explore similar, get_chunk's candidate scan, and the literal BM25/tokenless metadata reads; single-project queries keep the bounded wait — there is no fan-out to wedge behind. A repo whose model was not loaded turned the first semantic query into a network fetch of model files, unbounded on a proxied network, holding the map lock and poisoning unrelated queries: search now accepts only loaded models or models provably on disk (is_model_in_cache) — groups skip such repos with one warning per distinct model (FTS fusion still covers them in hybrid/auto/lexical), a lone project fails fast with a setup hint, and the no-download gate guards the serve/MCP query paths (the CLI search command manages its own model loading). embed_query now runs ONNX inference in spawn_blocking instead of pinning the async worker for the whole pass. +- **Idle MCP sessions are reaped after 30 minutes.** keep_alive=None assumed TCP liveness would reap dead clients, but a wedged request keeps its socket and its session worker alive indefinitely (observed active_sessions=4 with a single client, each leaked session pinning an FIFO-serialized rmcp task). 30 minutes of idle is far beyond any local interactive pause yet guarantees a stuck session — and its queued requests — eventually goes away without a serve restart. +- **Empty stores answer queries instead of erroring.** An indexing run that ends with zero embeddable files never builds the arroy tree, and search() treated the missing graph as a hard error — a group fan-out reported each of the pilot's 31 empty repos as a per-repo failure on every semantic query. A chunkless store now returns an empty result (last-chunk probe: chunks present without a graph is a genuinely broken state and stays a loud error), so empty repos read as "no hits there", like they always did on the FTS path. +- **Visible shutdown notice after q.** q is the only graceful-exit trigger — no SIGINT/SIGTERM handler exists, and while the TUI holds raw mode Ctrl-C is not even delivered as a signal. After q, axum waits out open MCP SSE sessions (long-lived, they never close on their own) until the 3 s forced-exit deadline, which from the restored terminal reads as a hung process; the natural reflex — a second Ctrl-C, delivered once raw mode is off — hard-kills serve mid-drain. The TUI now prints right after restoring the terminal how many sessions are being drained and that the process exits by itself, so the pause is visibly bounded. +- **Ignore-sensitive tests are hermetic against a developer's global .codesearchignore.** The walker and watcher tests walk tempdirs and assert exact file sets and matcher presence, but FileWalker/FileWatcher also load the machine's global ignore — so a developer who actually customizes that file got permanent red local runs (the pilot's list starts with *.bin, which pruned test.bin before the binary check ever ran, and any global file at all broke the no-matcher assertion) while CI, with a clean home, stayed green. Both test families now pin CODESEARCH_HOME and its HOME/USERPROFILE fallback to fresh empty directories via the crate's #[serial] + EnvRestore idiom. +- **Serve no longer parks 9 tantivy OS threads per warm repo (~4.5k resident threads on a 500-repo pool).** Every open store held an eager IndexWriter — 3 indexing workers sized from the CPU count, a segment updater and a 4-thread merge pool that NoMergePolicy never schedules — plus a reader-side meta.json poller waking ~2x/s, all idle forever in a search-only pool while observability tools (ps -M, sample) choked on the thread census. The FTS writer is lazy again: it materializes only on the first actual write; readers of self-owned stores switch to ReloadPolicy::Manual because every commit path already reloads its own reader explicitly, and only genuinely externally-written readonly stores keep the OnCommitWithDelay watcher; a created writer now runs one worker and one merge thread instead of tantivy's derived defaults. Cross-process locking is unchanged (the codesearch-level .writer.lock still guards the database), and this process's own commits stay immediately visible to its searches. - **Blazor/Razor projects no longer produce false "Compilation errors" index warnings.** The workspace load never ran source generators, so Razor components' generated partial halves were missing and every code-behind use of an `@code` member collapsed into CS0103/CS0115 cascades while `dotnet build` stayed clean. `scip-csharp` now drives the project's source generators itself (with a tolerant loader for analyzers built against a newer Roslyn than the pinned one) and indexes the generated output compilation; a failed generator pipeline falls back to the plain compilation and downgrades the resulting diagnostics to INFO notes instead of warnings. The index-warning lead entry no longer asserts a user-fixable build problem — it distinguishes real breaks from tool-side gaps — and error specimens are truncated at word boundaries instead of mid-identifier. ## [1.5.2] - 2026-10-05 diff --git a/README.md b/README.md index 2398b4a1..3df1ca6e 100644 --- a/README.md +++ b/README.md @@ -578,6 +578,8 @@ In the `codesearch serve` TUI, mounts appear in **italic/cyan**, distinguishing | `CODESEARCH_HOME` | Relocate the entire global root (`repos.json`, models cache, logs, `serve_url`, global ignore/extension files) away from `~/.codesearch`. Must be an absolute path; unset = `~/.codesearch`. Per-file overrides like `CODESEARCH_REPOS_CONFIG` still win | | `CODESEARCH_REPOS_CONFIG` | Path to repos.json | | `CODESEARCH_REPO_IDLE_TIMEOUT_SECS` | Idle eviction timeout (default: 1800) | +| `CODESEARCH_STORE_LOCK_WAIT_SECS` | Bounded wait for a store's write lock before a single-project query reports the store busy (default: 300). Multi-repo fan-outs never wait — they skip the busy repo with a warning | +| `CODESEARCH_MODEL_LOAD_TIMEOUT_SECS` | Bound on a cold-cache embedding-model load (default: 600). On expiry the loader fails fast with a hint to pre-populate the cache via `codesearch setup`; search itself only uses models already loaded or provably on disk | | `CODESEARCH_CACHE_MAX_MEMORY` | Embedding cache MB (default: 500) | | `CODESEARCH_MAX_LMDB_MAP_SIZE_MB` | Hard cap (MB) for LMDB auto-resize on `MDB_MAP_FULL`, applied to both the vector store and the persistent embedding cache (default: 16384 = 16GB; clamped to at least 1024). Raise this for very large corpora (millions of chunks) that legitimately exceed the default cap — see #189. | | `CODESEARCH_BATCH_SIZE` | Embedding batch size | diff --git a/src/cache/file_meta.rs b/src/cache/file_meta.rs index 5ed0283d..a94c27f3 100644 --- a/src/cache/file_meta.rs +++ b/src/cache/file_meta.rs @@ -6,9 +6,8 @@ use std::fs; use std::path::{Path, PathBuf}; use std::time::SystemTime; -use crate::constants::{ - FILE_META_DB_NAME, FILE_META_SAVE_RENAME_ATTEMPTS, FILE_META_SAVE_RENAME_DELAY_MS, -}; +use crate::constants::FILE_META_DB_NAME; +use crate::vectordb::{is_transient_rename_error, RENAME_RETRY_ATTEMPTS, RENAME_RETRY_DELAY_MS}; // ─── CANONICAL PATH POLICY ──────────────────────────────────────────────────── // @@ -273,13 +272,10 @@ pub struct FileMetaStore { pub last_full_index: Option, /// Version for format compatibility version: u32, - /// The codesearch build that produced these records. A mismatch on load - /// (deploy, or a store from before this field existed) wipes the - /// records: the chunker's output convention can change without a single - /// source byte changing (observed live: 0-based → 1-based chunk start - /// lines), and every content signal — mtime, hash, HEAD — then - /// wrongly reports "fresh". Same invalidation policy as the - /// model-name check below. + /// Chunker/SCIP conventions stamp of the build that produced these + /// records. A foreign builder's conventions (line bases, key shapes) + /// are not ours — same invalidation policy as the model-name check. + /// `#[serde(default)]` keeps pre-stamp stores loadable. #[serde(default)] builder_version: String, } @@ -300,80 +296,171 @@ impl FileMetaStore { } } - /// Load from database directory, or create new if doesn't exist + /// Load from database directory, or create new if doesn't exist. + /// + /// A torn or unreadable main file (unclean serve shutdown mid-`save`; + /// see [`Self::save`]) falls back to the one-generation `.bak` instead of + /// failing hard. Only when no generation parses do we degrade to an empty + /// store — callers' B1 guard then deliberately clears the vector/FTS + /// stores and re-indexes from scratch. A hard error here used to be worse: + /// the corrupt file never self-heals, so warmup skipped the repo on every + /// start until someone deleted the file by hand (pilot incident, + /// payment-identification: 323/323 files re-embedded after an unclean + /// shutdown). pub fn load_or_create(db_path: &Path, model_name: &str, dimensions: usize) -> Result { let meta_path = db_path.join(Self::FILENAME); - if meta_path.exists() { - let content = fs::read_to_string(&meta_path)?; - let mut store: FileMetaStore = serde_json::from_str(&content) - .map_err(|e| anyhow!("Failed to parse file metadata: {}", e))?; - - // Check if model changed - if so, invalidate everything. - // warn_print!: stderr + quiet-aware — a raw println would - // corrupt the serve TUI's screen (observed live when the - // builder-version wipe below fired for the whole fleet). - if store.model_name != model_name || store.dimensions != dimensions { - crate::warn_print!( - "⚠️ Model changed ({} -> {}), full re-index required", - store.model_name, - model_name - ); - store = Self::new(model_name.to_string(), dimensions); + let mut store = if meta_path.exists() { + match Self::read_store_file(&meta_path) { + Ok(store) => Some(store), + Err(main_err) => { + let bak_path = Self::backup_path(&meta_path); + match Self::read_store_file(&bak_path) { + Ok(bak) => { + tracing::warn!( + "file metadata {} is corrupt ({}); recovered the previous \ + generation from {}", + meta_path.display(), + main_err, + bak_path.display() + ); + Some(bak) + } + Err(_) => { + tracing::warn!( + "file metadata {} is corrupt ({}) with no recoverable backup; \ + starting with an empty store, full re-index will follow", + meta_path.display(), + main_err + ); + None + } + } + } } + } else { + None + } + .unwrap_or_else(|| Self::new(model_name.to_string(), dimensions)); + + // Check if model changed - if so, invalidate everything. + // warn_print!: stderr + quiet-aware — a raw println would corrupt the + // serve TUI's screen (observed live when the builder-version wipe + // below fired for the whole fleet). + if store.model_name != model_name || store.dimensions != dimensions { + crate::warn_print!( + "⚠️ Model changed ({} -> {}), full re-index required", + store.model_name, + model_name + ); + store = Self::new(model_name.to_string(), dimensions); + } - // Check if another build produced these records — its chunker - // conventions (line bases, key shapes) are not ours. - if store.builder_version != crate::constants::INDEX_BUILDER_VERSION { - crate::warn_print!( - "⚠️ Indexer version changed ({} -> {}), full re-index required", - if store.builder_version.is_empty() { - "pre-stamp".to_string() - } else { - store.builder_version.clone() - }, - crate::constants::INDEX_BUILDER_VERSION - ); - store = Self::new(model_name.to_string(), dimensions); - } + // Check if another build produced these records — its chunker + // conventions (line bases, key shapes) are not ours. + if store.builder_version != crate::constants::INDEX_BUILDER_VERSION { + crate::warn_print!( + "⚠️ Indexer version changed ({} -> {}), full re-index required", + if store.builder_version.is_empty() { + "pre-stamp".to_string() + } else { + store.builder_version.clone() + }, + crate::constants::INDEX_BUILDER_VERSION + ); + store = Self::new(model_name.to_string(), dimensions); + } - // Migrate stored paths to normalized format (strip UNC prefix, forward slashes). - // Existing stores may have Windows backslash paths or \\?\ prefixed paths. - store.migrate_paths(); + // Migrate stored paths to normalized format (strip UNC prefix, forward slashes). + // Existing stores may have Windows backslash paths or \\?\ prefixed paths. + store.migrate_paths(); - Ok(store) - } else { - Ok(Self::new(model_name.to_string(), dimensions)) - } + Ok(store) + } + + /// Read and parse a store file (the main generation or its `.bak`). + fn read_store_file(path: &Path) -> Result { + let content = fs::read_to_string(path)?; + serde_json::from_str(&content).map_err(|e| anyhow!("Failed to parse file metadata: {}", e)) } - /// Save to database directory + /// Sibling path of the one-generation crash-recovery backup for this + /// store's file (see [`Self::save`]). + fn backup_path(meta_path: &Path) -> PathBuf { + let mut bak = meta_path.as_os_str().to_os_string(); + bak.push(".bak"); + PathBuf::from(bak) + } + + /// Save to database directory. + /// + /// Crash-atomic write, hardened after a pilot incident: plain `fs::write` + /// is truncate-then-write, so a serve killed mid-save — and the FSW + /// update path saves after *every* indexed file, making the window wide — + /// left a zero-byte or torn `file_meta.json`. On the next start that + /// either failed warmup outright (the corrupt file never self-heals) or + /// forced a deliberate full wipe, re-embedding an entire unchanged repo + /// (payment-identification: 323/323 files). + /// + /// Mirrors `ReposConfig::save_to`: keep one `.bak` generation + /// (best-effort — a failed copy never blocks saving fresh state), write a + /// unique temp sibling, fsync it, then rename into place with a bounded + /// retry for transient handle races. The temp name embeds pid + counter + /// because the refresh task and the FSW update path may save the same + /// repo concurrently and must not share one temp file. pub fn save(&self, db_path: &Path) -> Result<()> { + use std::io::Write; + let meta_path = db_path.join(Self::FILENAME); - let content = serde_json::to_string_pretty(self)?; - // Atomic replace (tmp + rename): the store is now saved once per - // index batch, so a crash mid-write must never leave a truncated - // file — a truncated store parses as broken on the next open and - // would trip the empty-meta wipe guard. - let tmp_path = db_path.join(format!("{}.tmp", Self::FILENAME)); - fs::write(&tmp_path, content)?; - // Windows: an indexer can briefly hold the target (os error 5/32). - let mut attempt = 1; - loop { + + // (1) One-generation backup, best-effort. + if meta_path.exists() { + let _ = fs::copy(&meta_path, Self::backup_path(&meta_path)); + } + + // (2) Atomic replace: temp file + rename over the target. + static TMP_COUNTER: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(0); + let seq = TMP_COUNTER.fetch_add(1, std::sync::atomic::Ordering::Relaxed); + let tmp_path = db_path.join(format!( + "{}.{}.{}.tmp", + Self::FILENAME, + std::process::id(), + seq + )); + + let data = serde_json::to_string_pretty(self)?; + let write_result = (|| -> std::io::Result<()> { + let mut f = fs::File::create(&tmp_path)?; + f.write_all(data.as_bytes())?; + f.sync_all()?; + Ok(()) + })(); + if let Err(e) = write_result { + let _ = fs::remove_file(&tmp_path); + return Err(e.into()); + } + + for attempt in 0..RENAME_RETRY_ATTEMPTS { match fs::rename(&tmp_path, &meta_path) { Ok(()) => return Ok(()), - Err(e) if attempt >= FILE_META_SAVE_RENAME_ATTEMPTS => { + Err(e) => { + if is_transient_rename_error(&e) && attempt + 1 < RENAME_RETRY_ATTEMPTS { + tracing::warn!( + "file metadata rename to {} hit a transient error (attempt {}/{}): {}", + meta_path.display(), + attempt + 1, + RENAME_RETRY_ATTEMPTS, + e + ); + std::thread::sleep(std::time::Duration::from_millis(RENAME_RETRY_DELAY_MS)); + continue; + } let _ = fs::remove_file(&tmp_path); return Err(e.into()); } - Err(_) => { - attempt += 1; - std::thread::sleep(std::time::Duration::from_millis( - FILE_META_SAVE_RENAME_DELAY_MS, - )); - } } } + unreachable!("retry loop always returns") } /// Migrate stored paths to normalized format. diff --git a/src/cache/file_meta_tests.rs b/src/cache/file_meta_tests.rs index 7265237f..b30ddb7b 100644 --- a/src/cache/file_meta_tests.rs +++ b/src/cache/file_meta_tests.rs @@ -445,8 +445,11 @@ fn builder_version_stamp_is_a_generation_marker_not_the_package_version() { } /// save() must replace atomically (tmp + rename): the store is written once -/// per index batch now, so a crash mid-write must never truncate the file — -/// and a leftover .tmp from a crashed write must not confuse the next load. +/// per index batch now, so a crash mid-write must never truncate the file. +/// Temp names are pid+counter-unique (the refresh task and the FSW update +/// path may save concurrently), so a crashed writer's leftover .tmp is +/// never consumed by the next save — the invariant is that leftovers are +/// ignored on load, while the save's own tmp is consumed by its rename. #[test] fn save_replaces_atomically_and_leaves_no_tmp_behind() { let dir = tempdir().unwrap(); @@ -459,12 +462,29 @@ fn save_replaces_atomically_and_leaves_no_tmp_behind() { store.update_file(&test_file, "a.txt", vec![1, 2]).unwrap(); store.save(db_path).unwrap(); - // A crashed write left a garbage .tmp behind: the next save must - // overwrite it and the load must see the real content. - let tmp = db_path.join(format!("{}.tmp", crate::constants::FILE_META_DB_NAME)); - fs::write(&tmp, "{ not json").unwrap(); + // A crashed write left a garbage .tmp behind (another pid's unique + // name): the next save must not touch it, the load must ignore it, and + // the real content must survive. + let leftover = db_path.join(format!( + "{}.999999.0.tmp", + crate::constants::FILE_META_DB_NAME + )); + fs::write(&leftover, "{ not json").unwrap(); store.save(db_path).unwrap(); - assert!(!tmp.exists(), "save must consume the tmp file via rename"); + + let own_tmp_left = std::fs::read_dir(db_path) + .unwrap() + .filter_map(|e| e.ok()) + .any(|e| { + let name = e.file_name().to_string_lossy().to_string(); + name.starts_with(&format!("{}.", crate::constants::FILE_META_DB_NAME)) + && name.ends_with(".tmp") + && name != leftover.file_name().unwrap().to_string_lossy() + }); + assert!( + !own_tmp_left, + "save must consume its own tmp file via rename" + ); let loaded = FileMetaStore::load_or_create(db_path, "test-model", 384).unwrap(); assert_eq!(loaded.files.len(), 1, "second save must fully replace"); @@ -661,3 +681,104 @@ fn test_path_matches_filter_matches_exact_directory_name() { let filter = normalize_filter_path("src"); assert!(path_matches_filter("/repo/src/main.rs", &filter, &root)); } + +// ========================================================================= +// Crash-recovery tests — the pilot incident behind the hardening: +// a serve killed mid-`save` left a torn `file_meta.json` and the next +// warmup either skipped the repo forever (hard parse error) or fell into +// the deliberate full-wipe path, re-embedding an entire unchanged repo +// (payment-identification: 323/323 files). `save` now writes tmp+fsync+ +// rename with one `.bak` generation, and `load_or_create` recovers from +// that backup instead of failing or silently losing all incremental state. +// ========================================================================= + +#[test] +fn torn_main_file_recovers_from_backup_generation() { + let dir = tempdir().unwrap(); + let db_path = dir.path(); + let f1 = dir.path().join("a.txt"); + fs::write(&f1, "one").unwrap(); + + // Generation 1: only `a.txt` tracked. + let mut g1 = FileMetaStore::new("test-model".to_string(), 384); + g1.update_file(&f1, "a.txt", vec![1]).unwrap(); + g1.save(db_path).unwrap(); + + // Generation 2 adds a second file; its save moves gen 1 into `.bak`. + let f2 = dir.path().join("b.txt"); + fs::write(&f2, "two").unwrap(); + let mut g2 = FileMetaStore::new("test-model".to_string(), 384); + g2.update_file(&f1, "a.txt", vec![1]).unwrap(); + g2.update_file(&f2, "b.txt", vec![2, 3]).unwrap(); + g2.save(db_path).unwrap(); + + // Simulate the crash: a truncated write replaces the main file while + // the `.bak` generation survives untouched. + fs::write(db_path.join(FILE_META_DB_NAME), "{\"files\": {").unwrap(); + + let recovered = FileMetaStore::load_or_create(db_path, "test-model", 384).unwrap(); + assert!( + recovered.is_tracked("a.txt"), + "gen-1 entry must survive via the .bak generation" + ); + assert!( + !recovered.is_tracked("b.txt"), + "recovery yields the previous generation, not the torn one" + ); +} + +#[test] +fn corrupt_metadata_without_backup_degrades_to_empty_store_instead_of_error() { + let dir = tempdir().unwrap(); + let db_path = dir.path(); + // Torn file with no `.bak` (e.g. written by a pre-hardening binary). + fs::write(db_path.join(FILE_META_DB_NAME), "not json at all").unwrap(); + + // Previously this was a hard error: warmup skipped the repo on every + // start because the corrupt file never self-heals. Now it degrades to + // an empty store so the B1 guard can re-index deliberately. + let store = FileMetaStore::load_or_create(db_path, "test-model", 384).unwrap(); + assert!(store.is_empty()); + assert_eq!(store.model_name, "test-model"); + assert_eq!(store.dimensions, 384); +} + +#[test] +fn save_is_atomic_and_keeps_one_backup_generation() { + let dir = tempdir().unwrap(); + let db_path = dir.path(); + let f1 = dir.path().join("a.txt"); + fs::write(&f1, "one").unwrap(); + + let mut g1 = FileMetaStore::new("test-model".to_string(), 384); + g1.update_file(&f1, "a.txt", vec![1]).unwrap(); + g1.save(db_path).unwrap(); + + let mut g2 = FileMetaStore::new("test-model".to_string(), 384); + g2.update_file(&f1, "a.txt", vec![1, 7]).unwrap(); + g2.save(db_path).unwrap(); + + // No temp litter from the tmp+rename dance. + let leftovers: Vec = fs::read_dir(db_path) + .unwrap() + .filter_map(|e| e.ok()) + .map(|e| e.file_name().to_string_lossy().into_owned()) + .filter(|name| name.ends_with(".tmp") || name.ends_with(".new")) + .collect(); + assert!( + leftovers.is_empty(), + "temp files left behind: {leftovers:?}" + ); + + // Main file holds the fresh generation; `.bak` holds the previous one. + assert!(db_path.join(FILE_META_DB_NAME).exists()); + let bak_content = fs::read_to_string(db_path.join(format!("{FILE_META_DB_NAME}.bak"))).unwrap(); + let bak_store: FileMetaStore = serde_json::from_str(&bak_content).unwrap(); + assert_eq!(bak_store.tracked_chunk_ids(), [1].iter().copied().collect()); + let main_content = fs::read_to_string(db_path.join(FILE_META_DB_NAME)).unwrap(); + let main_store: FileMetaStore = serde_json::from_str(&main_content).unwrap(); + assert_eq!( + main_store.tracked_chunk_ids(), + [1, 7].iter().copied().collect() + ); +} diff --git a/src/chunker/mod.rs b/src/chunker/mod.rs index 026187c4..07589dad 100644 --- a/src/chunker/mod.rs +++ b/src/chunker/mod.rs @@ -24,10 +24,11 @@ pub struct Chunk { /// The actual content of the chunk pub content: String, - /// Starting line number (0-indexed) + /// Starting line number (0-indexed; responses convert to 1-based editor + /// lines at the emit boundary) pub start_line: usize, - /// Ending line number (0-indexed) + /// Ending line number (0-indexed; converted to 1-based on emit) pub end_line: usize, /// Type of chunk diff --git a/src/cli/mod.rs b/src/cli/mod.rs index 6c065e81..c82efe34 100644 --- a/src/cli/mod.rs +++ b/src/cli/mod.rs @@ -2025,6 +2025,68 @@ async fn run_hook_git_install(path: Option) -> Result<()> { Ok(()) } +/// Outcome of [`refresh_codesearch_post_checkout_hook`]. +#[derive(Debug, PartialEq, Eq)] +pub(crate) enum HookRefreshOutcome { + /// A codesearch-managed block was present and already current. + UpToDate, + /// An outdated codesearch-managed block was rewritten with this build's + /// version (e.g. the `pwd -W` fix or the CODESEARCH_HOME serve_url path). + Upgraded, + /// No post-checkout hook exists — never installed or removed by hand. + NotInstalled, + /// A post-checkout hook exists but carries no codesearch-managed block. + Foreign, + /// The hooks dir could not be resolved or the rewrite failed. + Failed(String), +} + +/// Upgrade a codesearch-managed `post-checkout` block written by an OLDER +/// binary to the current block, without touching anything else. +/// +/// The pilot kept 515 repos whose hooks were installed once and then left +/// behind as new binaries fixed the block (`pwd -W`, `$CODESEARCH_HOME` +/// serve_url lookup) — the only upgrade path was re-running +/// `codesearch hooks git install` per repo by hand. serve now calls this for +/// every registered repo at startup so a new binary regenerates its own +/// blocks automatically. +/// +/// Deliberately conservative: a repo with NO hook or a foreign hook is left +/// untouched — automatic installation into user repos is too invasive to do +/// from a background pass; only OUR region is rewritten, foreign content in +/// the same file is preserved byte-for-byte. +pub(crate) fn refresh_codesearch_post_checkout_hook( + repo_path: &std::path::Path, +) -> HookRefreshOutcome { + let hooks_dir = match resolve_hooks_dir(repo_path) { + Ok(dir) => dir, + Err(e) => return HookRefreshOutcome::Failed(format!("{e:#}")), + }; + let hook_path = hooks_dir.join("post-checkout"); + let Ok(existing) = std::fs::read_to_string(&hook_path) else { + return HookRefreshOutcome::NotInstalled; + }; + if !existing.contains(HOOK_BEGIN) || !existing.contains(HOOK_END) { + return HookRefreshOutcome::Foreign; + } + let updated = match replace_hook_block(&existing, &codesearch_hook_block()) { + Some(updated) => updated, + None => { + return HookRefreshOutcome::Failed( + "managed markers present but the region could not be located".to_string(), + ) + } + }; + if updated == existing { + return HookRefreshOutcome::UpToDate; + } + if let Err(e) = std::fs::write(&hook_path, &updated) { + return HookRefreshOutcome::Failed(format!("{e:#}")); + } + let _ = make_executable(&hook_path); + HookRefreshOutcome::Upgraded +} + pub mod claude_hooks; pub mod doctor; pub mod setup; @@ -2106,6 +2168,80 @@ mod tests { assert!(replace_hook_block("#!/bin/sh\necho hi\n", &codesearch_hook_block()).is_none()); } + // --- startup hook self-heal (serve refreshes blocks written by older binaries) + + /// Fixture: a temp "repo" whose hooks dir resolves without a real git + /// binary — `git rev-parse` fails in a bare tempdir, so the manual + /// `.git/hooks` fallback kicks in (a plain `.git` directory is enough). + fn hook_refresh_fixture() -> (tempfile::TempDir, std::path::PathBuf) { + let tmp = tempfile::tempdir().unwrap(); + let hooks = tmp.path().join(".git").join("hooks"); + std::fs::create_dir_all(&hooks).unwrap(); + (tmp, hooks.join("post-checkout")) + } + + #[test] + fn refresh_upgrades_an_outdated_managed_block_idempotently() { + let (tmp, hook_path) = hook_refresh_fixture(); + let repo = tmp.path(); + // A hook written by an OLDER binary: our markers, stale body. + let stale = format!( + "#!/bin/sh\n# foreign header\necho mine\n{}\nREPO_PATH=\"$(pwd)\"\n{}\necho bye\n", + HOOK_BEGIN, HOOK_END + ); + std::fs::write(&hook_path, stale).unwrap(); + + assert_eq!( + refresh_codesearch_post_checkout_hook(repo), + HookRefreshOutcome::Upgraded + ); + let updated = std::fs::read_to_string(&hook_path).unwrap(); + // Foreign lines preserved, fresh body swapped in, stale body gone. + assert!(updated.contains("echo mine")); + assert!(updated.contains("echo bye")); + assert!(updated.contains("pwd -W 2>/dev/null || pwd")); + assert!(!updated.contains("REPO_PATH=\"$(pwd)\"")); + + // Second pass over the same file must be a no-op. + assert_eq!( + refresh_codesearch_post_checkout_hook(repo), + HookRefreshOutcome::UpToDate + ); + } + + #[test] + fn refresh_never_touches_foreign_or_missing_hooks() { + let (tmp, hook_path) = hook_refresh_fixture(); + let repo = tmp.path(); + + // No hook at all → nothing to upgrade, nothing created. + assert_eq!( + refresh_codesearch_post_checkout_hook(repo), + HookRefreshOutcome::NotInstalled + ); + assert!(!hook_path.exists(), "refresh must not install hooks"); + + // A purely foreign hook → left byte-for-byte alone. + let foreign = "#!/bin/sh\necho untouched\n"; + std::fs::write(&hook_path, foreign).unwrap(); + assert_eq!( + refresh_codesearch_post_checkout_hook(repo), + HookRefreshOutcome::Foreign + ); + assert_eq!(std::fs::read_to_string(&hook_path).unwrap(), foreign); + } + + #[test] + fn refresh_reports_failure_for_a_non_repository() { + let tmp = tempfile::tempdir().unwrap(); + // Neither `git rev-parse` nor the manual `.git` fallback can resolve + // a hooks dir here — must degrade to Failed, never panic. + assert!(matches!( + refresh_codesearch_post_checkout_hook(tmp.path()), + HookRefreshOutcome::Failed(_) + )); + } + #[test] fn test_chain_inserts_before_trailing_exit_0() { let block = codesearch_hook_block(); diff --git a/src/cli/setup.rs b/src/cli/setup.rs index 5bc7315f..5b4f5401 100644 --- a/src/cli/setup.rs +++ b/src/cli/setup.rs @@ -46,11 +46,21 @@ pub async fn run(model: Option) -> Result<()> { )); } + // Record the choice so serve picks it up automatically on future + // launches — without this the operator had to re-type `serve --model X` + // every time (setup used to download and then silently forget). + crate::embed::save_default_model(model_type)?; + println!( "✅ Setup complete! Model '{}' ({dimensions} dims) ready at {}", model_type.short_name(), cache_dir.display() ); + println!( + "🧠 Default model for new indexes persisted: {}. `codesearch serve` now uses it \ + automatically (override per-run with --model).", + model_type.short_name() + ); Ok(()) } diff --git a/src/constants.rs b/src/constants.rs index a4240490..549fbb0b 100644 --- a/src/constants.rs +++ b/src/constants.rs @@ -45,11 +45,12 @@ pub const CONFIG_DIR_NAME: &str = ".codesearch"; /// Name of the file metadata database pub const FILE_META_DB_NAME: &str = "file_meta.json"; -/// Rename attempts when publishing the file metadata (AV/Search indexer can hold the file) -pub const FILE_META_SAVE_RENAME_ATTEMPTS: u32 = 5; - -/// Delay between file metadata rename attempts, in milliseconds -pub const FILE_META_SAVE_RENAME_DELAY_MS: u64 = 100; +/// Name of the persisted default-model choice in `/`. +/// +/// Written by `codesearch setup`, adopted by `codesearch serve` when its own +/// `--model` flag is absent — so an operator's setup choice survives across +/// serve launches instead of being silently dropped. +pub const DEFAULT_MODEL_FILE: &str = "default_model.json"; /// Subdirectory name for embedding models within the global config dir const MODELS_SUBDIR: &str = "models"; @@ -450,6 +451,13 @@ pub const FIND_IMPACT_PATH: &str = "/find-impact"; /// Override with `CODESEARCH_REPO_IDLE_TIMEOUT_SECS`. pub const REPO_IDLE_TIMEOUT_SECS: u64 = 30 * 60; // 30 minutes +/// Idle MCP session reap bound (rmcp `LocalSessionManager` keep_alive). +/// A wedged session keeps its socket and its FIFO-serialized worker alive +/// indefinitely; 30 minutes is far beyond any human pause in local +/// interactive use, yet guarantees a stuck session eventually goes away +/// without a serve restart. +pub const MCP_IDLE_SESSION_SECS: u64 = 30 * 60; // 30 minutes + /// How often the idle-reaper background task checks for repos to evict. pub const REAPER_INTERVAL_SECS: u64 = 5 * 60; // 5 minutes @@ -591,6 +599,21 @@ pub const STORE_LOCK_WAIT_SECS: u64 = 300; /// Environment variable overriding [`STORE_LOCK_WAIT_SECS`]. pub const STORE_LOCK_WAIT_SECS_ENV: &str = "CODESEARCH_STORE_LOCK_WAIT_SECS"; +/// How long a lazy embedding-model load may take before it is abandoned +/// with an actionable error. +/// +/// `EmbeddingService::with_cache_dir` resolves model files through hf-hub, +/// which on a cold cache means a network download. On networks that +/// black-hole the model host this fetch never completes — and never errors — +/// so an unbounded load wedges every caller stuck on the model's init lock +/// (observed as a whole-night serve stall). The bound turns that into a +/// fail-fast error pointing at `codesearch setup`; generous enough to let a +/// legitimate first-time download through. +pub const MODEL_LOAD_TIMEOUT_SECS: u64 = 600; + +/// Environment variable overriding [`MODEL_LOAD_TIMEOUT_SECS`]. +pub const MODEL_LOAD_TIMEOUT_SECS_ENV: &str = "CODESEARCH_MODEL_LOAD_TIMEOUT_SECS"; + /// Total number of attempts (initial request + retries) the federation client /// makes against a remote peer that answers with a transient HTTP status /// (502/503/504). Federated peers commonly run on scale-to-zero hosts (Azure diff --git a/src/db_discovery/mod.rs b/src/db_discovery/mod.rs index 8b12eef0..f1a76e0a 100644 --- a/src/db_discovery/mod.rs +++ b/src/db_discovery/mod.rs @@ -269,7 +269,14 @@ fn find_global_databases() -> Result> { Ok(databases) } -/// Register a repository in the global tracking file +/// Register a repository in the global tracking file. +/// +/// No longer called by the `--global` index path: registering a project whose +/// store lives out-of-tree actively harmed serve — discovery looks for the +/// store at `/.codesearch.db` and would build an empty local one for +/// the alias. Kept `pub` for CLI/admin tooling that wants a plain register +/// with no file side effect (mirrors [`unregister_repository`]). +#[allow(dead_code)] // CLI/admin tooling, see the doc comment above pub fn register_repository(project_path: &Path) -> Result<()> { let mut config = ReposConfig::load()?; config.register(project_path.to_path_buf()); diff --git a/src/db_discovery/repos.rs b/src/db_discovery/repos.rs index 318c8208..4608e1db 100644 --- a/src/db_discovery/repos.rs +++ b/src/db_discovery/repos.rs @@ -13,7 +13,7 @@ use crate::constants::REPOS_CONFIG_FILE; /// (the leading `@` marks it as a remote reference rather than a local alias). /// Queries against such a group fan out to each remote peer over HTTP(S) and /// the results are merged with the local results. -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] pub struct RemotePeer { /// Base URL of the remote serve instance, e.g. `https://codesearch.example.com`. #[serde(alias = "base_url")] @@ -74,7 +74,7 @@ pub fn remote_project_name(peer_name: &str, remote_alias: &str) -> String { format!("{peer_name}{REMOTE_PROJECT_SEPARATOR}{remote_alias}") } -#[derive(Debug, Clone, Serialize, Deserialize, Default)] +#[derive(Debug, Clone, Serialize, Deserialize, Default, PartialEq)] pub struct ReposConfig { pub repos: HashMap, #[serde(default)] diff --git a/src/db_discovery/repos_tests.rs b/src/db_discovery/repos_tests.rs index d1e31a1a..55300013 100644 --- a/src/db_discovery/repos_tests.rs +++ b/src/db_discovery/repos_tests.rs @@ -288,12 +288,20 @@ fn unregister_path_matches_msys_posix_form() { /// On Unix, `/c/Users/...` is a legitimate absolute path (not an MSYS-ism), /// so `register()` must store it verbatim. This guards against the Windows /// fix accidentally rewriting paths on the wrong platform. +/// +/// The input is pre-canonicalized because register()'s contract is to store +/// the canonical form, and on macOS the tempdir lives behind a symlink +/// (`TMPDIR=/var/folders/...` → `/private/var/folders/...`): comparing the +/// stored value against the RAW path would test the OS's symlinked-TMP quirk, +/// not register(). On a canonical input `safe_canonicalize` is idempotent, so +/// any byte difference left is a genuine rewrite. #[test] #[cfg(not(windows))] fn register_leaves_unix_path_untouched() { let tmp = tempfile::tempdir().unwrap(); - let repo = tmp.path().join("propagate-tmp-repo"); - std::fs::create_dir(&repo).unwrap(); + let raw_repo = tmp.path().join("propagate-tmp-repo"); + std::fs::create_dir(&raw_repo).unwrap(); + let repo = safe_canonicalize(&raw_repo).unwrap(); let path_str = repo.to_string_lossy().to_string(); let mut cfg = ReposConfig::default(); diff --git a/src/embed/embedder.rs b/src/embed/embedder.rs index 96065ebf..10aac9b0 100644 --- a/src/embed/embedder.rs +++ b/src/embed/embedder.rs @@ -486,10 +486,173 @@ impl Default for FastEmbedder { } } +// ─── Persisted default model (setup → serve) ───────────────────────────────── +// +// `codesearch setup --model X` downloads X but used to record the choice +// nowhere, so serve started without its own `--model` flag silently fell +// back to the built-in default for every NEW index — the pilot operator had +// to re-type `--model embeddinggemma-q4` on every serve launch. The helpers +// below persist the setup choice under `` and let serve +// adopt it automatically. + +/// Path of the persisted default-model choice. +fn default_model_file() -> Result { + Ok(crate::constants::codesearch_home()?.join(crate::constants::DEFAULT_MODEL_FILE)) +} + +/// Persist `model` as the default for new indexes (called by `codesearch setup`). +/// +/// tmp+fsync+rename so a crash mid-write can never leave a torn choice file +/// (same discipline as the other codesearch-owned JSON files). +pub fn save_default_model(model: ModelType) -> Result<()> { + use std::io::Write; + + let path = default_model_file()?; + if let Some(parent) = path.parent() { + std::fs::create_dir_all(parent)?; + } + let mut tmp = path.as_os_str().to_os_string(); + tmp.push(".new"); + let tmp_path = std::path::PathBuf::from(tmp); + let data = format!("{{\"model\":\"{}\"}}", model.short_name()); + let write_result = (|| -> std::io::Result<()> { + let mut f = std::fs::File::create(&tmp_path)?; + f.write_all(data.as_bytes())?; + f.sync_all()?; + Ok(()) + })(); + if let Err(e) = write_result { + let _ = std::fs::remove_file(&tmp_path); + return Err(e.into()); + } + std::fs::rename(&tmp_path, &path)?; + Ok(()) +} + +/// Read the default model persisted by `codesearch setup`. +/// +/// `None` when no choice was recorded or the file names an unknown model — +/// serve then keeps its built-in fallback default for new indexes instead of +/// failing to start over a config file it can revive by re-running setup. +pub fn load_default_model() -> Option { + let path = default_model_file().ok()?; + let content = std::fs::read_to_string(path).ok()?; + let parsed: serde_json::Value = serde_json::from_str(&content).ok()?; + let short_name = parsed.get("model")?.as_str()?; + match ModelType::parse(short_name) { + Some(model) => Some(model), + None => { + tracing::warn!( + "Persisted default model '{}' is not known to this build; ignoring it", + short_name + ); + None + } + } +} + +/// Whether `model`'s ONNX files are already present in the global models cache. +/// +/// Resolves fastembed's own model table (`model_code`, `model_file`, +/// `additional_files`) and probes the hf-hub cache layout fastembed downloads +/// into (`models----/snapshots//…`) instead of duplicating repo +/// ids here. Purely a filesystem probe — never touches the network, so serve +/// can warn about a missing download before warmup starts one mid-flight. +pub fn is_model_in_cache(model: ModelType) -> bool { + let cache_dir = match crate::constants::get_global_models_cache_dir() { + Ok(dir) => dir, + Err(_) => return false, + }; + let fe_model = model.to_fastembed_model(); + let info = match ::get_model_info(&fe_model) { + Some(info) => info, + None => return false, + }; + let slug = format!("models--{}", info.model_code.replace('/', "--")); + let snapshots_dir = cache_dir.join(slug).join("snapshots"); + let Ok(revisions) = std::fs::read_dir(&snapshots_dir) else { + return false; + }; + for revision in revisions.flatten() { + let snapshot = revision.path(); + if !snapshot.join(&info.model_file).exists() { + continue; + } + if info + .additional_files + .iter() + .all(|f| snapshot.join(f).exists()) + { + return true; + } + } + false +} + #[cfg(test)] mod tests { use super::*; + #[test] + #[serial_test::serial] + fn default_model_choice_round_trips_and_degrades_on_unknown_name() { + let tmp = tempfile::tempdir().unwrap(); + let _env = crate::testing::EnvRestore::set(&[( + crate::constants::CODESEARCH_HOME_ENV, + tmp.path().to_str().unwrap(), + )]); + + assert_eq!(load_default_model(), None, "no choice recorded yet"); + + save_default_model(ModelType::EmbeddingGemma300MQ4).unwrap(); + assert_eq!( + load_default_model(), + Some(ModelType::EmbeddingGemma300MQ4), + "the setup choice must survive across processes" + ); + + // A file naming an unknown model (e.g. written by a newer/older build) + // must degrade to None instead of poisoning serve startup. + std::fs::write( + tmp.path().join(crate::constants::DEFAULT_MODEL_FILE), + "{\"model\":\"not-a-model\"}", + ) + .unwrap(); + assert_eq!(load_default_model(), None); + } + + #[test] + #[serial_test::serial] + fn is_model_in_cache_matches_hf_hub_download_layout() { + let tmp = tempfile::tempdir().unwrap(); + let _env = crate::testing::EnvRestore::set(&[( + crate::constants::CODESEARCH_HOME_ENV, + tmp.path().to_str().unwrap(), + )]); + + // Empty models cache → nothing cached (cache dir gets created, but no + // model directories inside). + assert!(!is_model_in_cache(ModelType::EmbeddingGemma300MQ4)); + + // Reproduce the exact hf-hub layout fastembed downloads into (verified + // against a live cache with a manually-installed snapshot): + // models--onnx-community--embeddinggemma-300m-ONNX/snapshots//onnx/… + let model_dir = tmp + .path() + .join("models") + .join("models--onnx-community--embeddinggemma-300m-ONNX"); + let snapshot = model_dir.join("snapshots").join("manual"); + std::fs::create_dir_all(snapshot.join("onnx")).unwrap(); + std::fs::write(snapshot.join("onnx/model_q4.onnx"), "x").unwrap(); + std::fs::write(snapshot.join("onnx/model_q4.onnx_data"), "x").unwrap(); + assert!(is_model_in_cache(ModelType::EmbeddingGemma300MQ4)); + + // A half-downloaded model (additional file missing) must not report + // cached — the pool would hit the network on first use otherwise. + std::fs::remove_file(snapshot.join("onnx/model_q4.onnx_data")).unwrap(); + assert!(!is_model_in_cache(ModelType::EmbeddingGemma300MQ4)); + } + #[test] fn test_model_type_dimensions() { // 384 dimension models diff --git a/src/embed/mod.rs b/src/embed/mod.rs index b653e7ba..4273051c 100644 --- a/src/embed/mod.rs +++ b/src/embed/mod.rs @@ -2,14 +2,15 @@ mod batch; mod cache; mod embedder; +use anyhow::Result; pub use batch::{BatchEmbedder, EmbeddedChunk}; pub use cache::{ CacheStats, CachedBatchEmbedder, PersistentCacheStats, PersistentEmbeddingCache, QueryCache, QueryCacheStats, }; -pub use embedder::{FastEmbedder, ModelType}; - -use anyhow::Result; +pub use embedder::{ + is_model_in_cache, load_default_model, save_default_model, FastEmbedder, ModelType, +}; use std::collections::HashMap; use std::env; use std::sync::{Arc, Mutex}; @@ -311,6 +312,11 @@ impl Default for EmbeddingService { #[derive(Default)] pub struct EmbeddingServicePool { services: Mutex>>>, + /// Per-model init locks. The `services` map lock is only ever held for a + /// lookup — loading a model runs under the per-model init lock, so a slow + /// (or wedged) load of one model cannot block lookups or loads of any + /// other model. + init_locks: Mutex>>>, cache_dir: Option, } @@ -320,28 +326,89 @@ impl EmbeddingServicePool { pub fn new(cache_dir: Option) -> Self { Self { services: Mutex::new(HashMap::new()), + init_locks: Mutex::new(HashMap::new()), cache_dir, } } + /// Return the service for `model` if it is already loaded, without ever + /// triggering a load. Search handlers use this to guarantee that a query + /// never turns into a network fetch of model files. + pub fn get_if_cached(&self, model: ModelType) -> Option>> { + self.services.lock().ok()?.get(&model).cloned() + } + /// Return the service for `model`, loading its ONNX model on first use. /// /// The returned `Arc` is locked independently per model, so a caller can /// hold it across an `embed_query` without blocking other models. pub fn get(&self, model: ModelType) -> Result>> { + if let Some(existing) = self.get_if_cached(model) { + return Ok(existing); + } + let init_lock = { + let mut guard = self + .init_locks + .lock() + .map_err(|e| anyhow::anyhow!("Embedding service pool mutex poisoned: {e}"))?; + guard.entry(model).or_default().clone() + }; + // Held across the load: same-model callers wait here instead of + // racing into duplicate loads. They block their own thread only — + // no map lock and no other model is involved. + let _init = init_lock + .lock() + .map_err(|e| anyhow::anyhow!("Embedding model init mutex poisoned: {e}"))?; + // Someone may have finished the load while we waited on the init lock. + if let Some(existing) = self.get_if_cached(model) { + return Ok(existing); + } + let service = self.load_bounded(model)?; + let arc = Arc::new(Mutex::new(service)); let mut guard = self .services .lock() .map_err(|e| anyhow::anyhow!("Embedding service pool mutex poisoned: {e}"))?; - if let Some(existing) = guard.get(&model) { - return Ok(existing.clone()); - } - let service = EmbeddingService::with_cache_dir(model, self.cache_dir.as_deref())?; - let arc = Arc::new(Mutex::new(service)); guard.insert(model, arc.clone()); Ok(arc) } + /// Load `model` off the async critical path, bounded in time. + /// + /// `EmbeddingService::with_cache_dir` resolves files through hf-hub; on a + /// cold cache that is a network download which can stall indefinitely on + /// networks that black-hole the model host. Running it on a dedicated + /// thread behind `recv_timeout` bounds the damage: callers get a + /// fail-fast error pointing at `codesearch setup` instead of hanging. + /// On timeout the loader thread is deliberately leaked — there is no safe + /// way to kill a thread mid-download — but it holds no pool locks, so it + /// can only waste its own resources until the underlying fetch gives up. + fn load_bounded(&self, model: ModelType) -> Result { + let timeout_secs = std::env::var(crate::constants::MODEL_LOAD_TIMEOUT_SECS_ENV) + .ok() + .and_then(|s| s.parse::().ok()) + .filter(|&s| s > 0) + .unwrap_or(crate::constants::MODEL_LOAD_TIMEOUT_SECS); + let (tx, rx) = std::sync::mpsc::channel(); + let cache_dir = self.cache_dir.clone(); + std::thread::spawn(move || { + // The receiver may already be gone on timeout — nothing to do. + let _ = tx.send(EmbeddingService::with_cache_dir( + model, + cache_dir.as_deref(), + )); + }); + match rx.recv_timeout(std::time::Duration::from_secs(timeout_secs)) { + Ok(Ok(service)) => Ok(service), + Ok(Err(e)) => Err(e), + Err(_) => Err(anyhow::anyhow!( + "embedding model '{model:?}' did not initialise within {timeout_secs}s — likely a \ + blocked network fetch of an uncached model; pre-populate the \ + model cache via `codesearch setup` and retry" + )), + } + } + /// Embed `chunks`, releasing the service lock between ONNX mini-batches. /// /// The per-model `Mutex` is what serialises embedding @@ -435,6 +502,61 @@ mod tests { assert!(out.is_empty()); } + /// `get_if_cached` is the search-path gate: it must answer "not loaded" + /// without ever triggering a load. A lookup that could initialise a + /// model would be useless as a "can I embed without a download?" + /// preflight — the whole point is that search never fetches. + #[test] + fn get_if_cached_reports_fresh_pool_as_unloaded() { + let pool = EmbeddingServicePool::new(None); + assert!(pool.get_if_cached(ModelType::default()).is_none()); + assert!(pool + .get_if_cached(ModelType::EmbeddingGemma300MQ4) + .is_none()); + } + + #[test] + #[ignore] // Requires EmbeddingGemma300MQ4 in the local models cache + fn get_publishes_the_service_to_get_if_cached() { + let pool = EmbeddingServicePool::new(Some(test_cache_dir())); + let model = ModelType::EmbeddingGemma300MQ4; + assert!( + pool.get_if_cached(model).is_none(), + "fresh pool must report unloaded" + ); + pool.get(model).expect("model load"); + assert!( + pool.get_if_cached(model).is_some(), + "get must publish the loaded service for later no-load lookups" + ); + } + + /// Double-checked init: threads racing `get()` on the same model must all + /// receive THE SAME service instance — a duplicate load would mean two + /// ONNX runtimes for one model (double memory) and `Arc::ptr_eq` is the + /// directly observable contract. + #[test] + #[ignore] // Requires EmbeddingGemma300MQ4 in the local models cache + fn concurrent_get_returns_one_shared_service_per_model() { + let pool = std::sync::Arc::new(EmbeddingServicePool::new(Some(test_cache_dir()))); + let model = ModelType::EmbeddingGemma300MQ4; + let mut handles = Vec::new(); + for _ in 0..4 { + let pool = pool.clone(); + handles.push(std::thread::spawn(move || { + pool.get(model).expect("model load") + })); + } + let first = handles.remove(0).join().expect("loader thread"); + for handle in handles { + let service = handle.join().expect("loader thread"); + assert!( + Arc::ptr_eq(&first, &service), + "concurrent get() must share one service instance per model" + ); + } + } + /// The index-metadata reader must invert `write_metadata_fields`, and must /// report "no answer" (None) for unknown/missing names rather than silently /// claiming the default — callers decide the fallback. diff --git a/src/federation/mod.rs b/src/federation/mod.rs index e6456fc6..0061386c 100644 --- a/src/federation/mod.rs +++ b/src/federation/mod.rs @@ -108,6 +108,21 @@ struct RemoteSearchResponse { /// Set by the REST layer when the remote tool returned an MCP error. #[serde(default)] _mcp_is_error: Option, + /// Response-level refusal note from the peer (e.g. every candidate fell + /// below its `min_score`). Must be relayed to the caller — an honest + /// refusal silently dropped reads as "no hits on that peer". + #[serde(default)] + note: Option, +} + +/// A peer's answer to a federated search: the ranked hits plus the +/// response-level `note` a refusing peer attached. The query path used to +/// return bare items, dropping that note, so a leg refusing by min_score +/// was indistinguishable from a leg with no matches. +#[derive(Debug, Default)] +pub struct SearchReply { + pub items: Vec, + pub note: Option, } /// The outcome of a single remote fan-out call. @@ -298,7 +313,7 @@ impl FederationClient { peer: &RemotePeer, mut body: serde_json::Value, remote_alias: &str, - ) -> Outcome> { + ) -> Outcome { if let Some(obj) = body.as_object_mut() { obj.insert( "project".into(), @@ -322,7 +337,7 @@ impl FederationClient { &self, peer: &RemotePeer, body: serde_json::Value, - ) -> Outcome> { + ) -> Outcome { let url = Self::peer_url(peer, crate::constants::SEARCH_PATH); let attempts = crate::constants::REMOTE_PEER_RETRY_ATTEMPTS.max(1); for attempt in 0..attempts { @@ -360,7 +375,10 @@ impl FederationClient { Ok(parsed) if status.is_success() && !parsed._mcp_is_error.unwrap_or(false) => { - return Outcome::Ok(parsed.results) + return Outcome::Ok(SearchReply { + items: parsed.results, + note: parsed.note.filter(|n| !n.is_empty()), + }) } Ok(parsed) => { // Tool-level error on the remote (e.g. scope_required). @@ -798,10 +816,45 @@ mod tests { ) .await; match outcome { - Outcome::Ok(items) => { - assert_eq!(items.len(), 1); - assert_eq!(items[0].chunk_id, Some(7)); - assert_eq!(items[0].path, "kb/doc.md"); + Outcome::Ok(reply) => { + assert_eq!(reply.items.len(), 1); + assert_eq!(reply.items[0].chunk_id, Some(7)); + assert_eq!(reply.items[0].path, "kb/doc.md"); + } + other => panic!("expected Ok, got {:?}", other), + } + } + + #[tokio::test] + async fn search_relays_a_peer_refusal_note() { + // A peer refusing by min_score answers with an explicit note and no + // results; dropping the note made the honest refusal read as "no + // hits on that peer". The reply must carry it through. + let app = axum::Router::new().route( + crate::constants::SEARCH_PATH, + axum::routing::post(|| async { + axum::Json(serde_json::json!({ + "results": [], + "note": "2 candidate hit(s) found but all scored below min_score 0.800; \ + refusing instead of returning the nearest neighbours." + })) + }), + ); + let addr = spawn_test_server(app).await; + + let client = FederationClient::new().unwrap(); + let outcome = client + .search_project( + &peer(format!("http://{addr}")), + serde_json::json!({"query": "x", "min_score": 0.8}), + "kb", + ) + .await; + match outcome { + Outcome::Ok(reply) => { + assert!(reply.items.is_empty()); + let note = reply.note.expect("refusal note must be relayed"); + assert!(note.contains("min_score 0.800"), "got: {note}"); } other => panic!("expected Ok, got {:?}", other), } @@ -1451,7 +1504,7 @@ mod tests { ) .await; match outcome { - Outcome::Ok(items) => assert_eq!(items.len(), 1), + Outcome::Ok(reply) => assert_eq!(reply.items.len(), 1), other => panic!("expected Ok after retry, got {other:?}"), } assert_eq!(hits.load(std::sync::atomic::Ordering::SeqCst), 2); diff --git a/src/file/mod.rs b/src/file/mod.rs index 6cfa21e3..b1c7b98b 100644 --- a/src/file/mod.rs +++ b/src/file/mod.rs @@ -247,11 +247,38 @@ impl FileWalker { #[cfg(test)] mod tests { use super::*; + use crate::testing::EnvRestore; + use serial_test::serial; use std::fs; use tempfile::TempDir; + /// Pin every location the walker can find a global `.codesearchignore` + /// (CODESEARCH_HOME and its HOME/USERPROFILE fallback) to fresh empty + /// directories, so a developer's real global ignore list cannot prune the + /// test fixtures — CI has no such file, which is how the ambient dependency + /// went unnoticed (the pilot's global list starts with `*.bin`, which + /// silently killed test_skip_binary_files on the dev machine). + fn isolated_global_ignore_env() -> (TempDir, EnvRestore) { + let scratch = TempDir::new().unwrap(); + let cs_home = scratch.path().join("cs-home"); + let bare_home = scratch.path().join("bare-home"); + fs::create_dir_all(&cs_home).unwrap(); + fs::create_dir_all(&bare_home).unwrap(); + let env = EnvRestore::set(&[ + ( + crate::constants::CODESEARCH_HOME_ENV, + cs_home.to_str().unwrap(), + ), + ("HOME", bare_home.to_str().unwrap()), + ("USERPROFILE", bare_home.to_str().unwrap()), + ]); + (scratch, env) + } + #[test] + #[serial] fn test_file_walker_basic() { + let _isolation = isolated_global_ignore_env(); let dir = TempDir::new().unwrap(); // Create some test files @@ -267,7 +294,9 @@ mod tests { } #[test] + #[serial] fn test_skip_binary_files() { + let _isolation = isolated_global_ignore_env(); let dir = TempDir::new().unwrap(); // Create text file @@ -286,7 +315,9 @@ mod tests { } #[test] + #[serial] fn test_language_detection() { + let _isolation = isolated_global_ignore_env(); let dir = TempDir::new().unwrap(); fs::write(dir.path().join("main.rs"), "fn main() {}").unwrap(); @@ -303,7 +334,9 @@ mod tests { } #[test] + #[serial] fn test_excluded_directories() { + let _isolation = isolated_global_ignore_env(); let dir = TempDir::new().unwrap(); // Create file in excluded directory @@ -326,7 +359,9 @@ mod tests { /// (.yaml/.json) are indexable on their own, so without the name match the /// walker would index every churn commit of a generated lock file. #[test] + #[serial] fn test_excluded_lock_files() { + let _isolation = isolated_global_ignore_env(); let dir = TempDir::new().unwrap(); fs::write(dir.path().join("pnpm-lock.yaml"), "lockVersion: '9.0'").unwrap(); @@ -346,7 +381,9 @@ mod tests { /// in `filter_entry` would let every internal file be indexed. /// Covers Aikido group 30641794. #[test] + #[serial] fn test_rejects_excluded_named_root() { + let _isolation = isolated_global_ignore_env(); let parent = TempDir::new().unwrap(); let git_root = parent.path().join(".git"); fs::create_dir(&git_root).unwrap(); diff --git a/src/fts/tantivy_store.rs b/src/fts/tantivy_store.rs index 4844678c..7d2194ac 100644 --- a/src/fts/tantivy_store.rs +++ b/src/fts/tantivy_store.rs @@ -6,16 +6,25 @@ //! Always use `FtsStore::new()` which opens in R/W mode. This ensures only one //! connection type exists, avoiding Windows file locking issues between readers //! and writers. The writer is lazy-initialized on first write operation. +//! +//! # Thread footprint +//! `new`/`new_with_writer` readers use `ReloadPolicy::Manual`: this process owns +//! every commit and [`FtsStore::commit`] reloads the reader right after +//! publishing, so no resident `meta.json` poller thread is kept. `new_readonly` +//! keeps the default `OnCommitWithDelay` watcher (polling ~2x/s) because its +//! commits may come from other processes. use anyhow::{anyhow, Result}; use std::path::Path; use tantivy::{ collector::TopDocs, directory::MmapDirectory, + indexer::IndexWriterOptions, merge_policy::NoMergePolicy, query::QueryParser, schema::{Field, NumericOptions, Schema, Value, STORED, STRING, TEXT}, - DocAddress, Index, IndexReader, IndexSettings, IndexWriter, TantivyDocument, Term, + DocAddress, Index, IndexReader, IndexSettings, IndexWriter, ReloadPolicy, TantivyDocument, + Term, }; use crate::chunker::ChunkKind; @@ -115,7 +124,28 @@ impl FtsStore { /// /// Opens in a mode that supports both reading and writing. /// Writer is lazy-initialized on first write operation. + /// + /// The reader uses `ReloadPolicy::Manual`: this store owns its commits and + /// [`FtsStore::commit`] reloads the reader right after publishing them, so + /// no resident watcher thread is kept. Do not use this constructor for an + /// index written by another process — those commits would never become + /// visible here; use [`FtsStore::new_readonly`] instead. pub fn new(db_path: &Path) -> Result { + Self::with_reload_policy(db_path, ReloadPolicy::Manual) + } + + /// Open an FTS index whose commits may be made by OTHER processes. + /// + /// This is the FTS-store analogue of + /// [`crate::index::SharedStores::new_readonly`]: the reader keeps + /// tantivy's `OnCommitWithDelay` policy, spending one resident poller + /// thread (~2 wakeups/s) so that foreign commits become visible without + /// reopening the store. + pub fn new_readonly(db_path: &Path) -> Result { + Self::with_reload_policy(db_path, ReloadPolicy::OnCommitWithDelay) + } + + fn with_reload_policy(db_path: &Path, reload_policy: ReloadPolicy) -> Result { let fts_path = db_path.join("fts"); std::fs::create_dir_all(&fts_path)?; @@ -126,7 +156,10 @@ impl FtsStore { let index = Self::open_or_create_index_with_retry(&fts_path, &schema)?; // Create reader for searching - let reader = index.reader()?; + let reader = index + .reader_builder() + .reload_policy(reload_policy) + .try_into()?; Ok(Self { index, @@ -149,7 +182,6 @@ impl FtsStore { #[cfg(test)] pub fn new_in_memory() -> Result { use tantivy::directory::RamDirectory; - use tantivy::ReloadPolicy; let (schema, chunk_id_field, content_field, path_field, signature_field, kind_field) = Self::build_schema(); @@ -275,10 +307,18 @@ impl FtsStore { std::thread::sleep(std::time::Duration::from_millis(200 * (1 << attempt))); } - // 50MB writer heap (tantivy default). + // 50MB writer heap (tantivy default) but a fixed single worker and a + // single merge thread instead of tantivy's derived defaults. A plain + // `writer(50MB)` sizes its worker pool from the CPU count (then shrinks + // it to heap/15MB ≈ 3 workers on this budget) and unconditionally + // spawns a 4-thread merge pool — ~8 parked OS threads per open writer. + // With one writer per warm repo, a serve pool of 500 repos held ~4500 + // resident threads. FTS work is negligible next to embedding, so one + // worker is plenty; with NoMergePolicy merges are never scheduled, so + // more than one merge thread would only ever idle. // - // CRITICAL: Set NoMergePolicy to prevent tantivy from spawning background - // merge threads. On Windows, these threads encounter I/O errors (antivirus + // CRITICAL: Set NoMergePolicy to prevent tantivy from running background + // merges. On Windows, these threads encounter I/O errors (antivirus // interference, file locking on mmap'd segment files) which panic the merge // thread and kill the IndexWriter — causing the intermittent // "An index writer was killed" error (~1/5 indexing runs). @@ -286,7 +326,12 @@ impl FtsStore { // With NoMergePolicy, all segment management is explicit: we accumulate // segments during indexing and they're consolidated at commit points. // This trades slightly more segments for 100% reliability. - match index.writer(50_000_000) { + let options = IndexWriterOptions::builder() + .num_worker_threads(1) + .num_merge_threads(1) + .memory_budget_per_thread(50_000_000) + .build(); + match index.writer_with_options(options) { Ok(writer) => { writer.set_merge_policy(Box::new(NoMergePolicy)); return Ok(writer); @@ -595,6 +640,41 @@ impl FtsStore { self.collect_fts_results(top_docs) } + /// Disjunction (OR) search over the same fields as [`FtsStore::search`]. + /// + /// Used by the literal relaxed fallback: when the exact AND pass matches + /// nothing, candidates holding only PART of the query terms are still + /// worth surfacing. The caller filters them by term coverage afterwards — + /// raw OR ranking alone would drown precise queries in noise, which is + /// why `search` stays conjunctive by default. + pub fn search_relaxed(&self, query: &str, limit: usize) -> Result> { + let searcher = self.reader.searcher(); + + let mut query_parser = QueryParser::for_index( + &self.index, + vec![self.content_field, self.signature_field, self.kind_field], + ); + query_parser.set_field_boost(self.signature_field, 2.0); + + let parsed_query = match query_parser.parse_query(query) { + Ok(q) => q, + Err(_) => { + let escaped = query.replace( + [ + ':', '(', ')', '[', ']', '{', '}', '^', '"', '~', '*', '?', '\\', '/', + ], + " ", + ); + query_parser.parse_query(&escaped)? + } + }; + + let top_docs = + searcher.search(&parsed_query, &TopDocs::with_limit(limit).order_by_score())?; + + self.collect_fts_results(top_docs) + } + /// Search for exact identifier matches (boosted) /// /// Used for improving exact name matching (e.g., "BaseRestClient", "UserService"). @@ -1008,4 +1088,41 @@ mod tests { Ok(()) } + + /// `new` must not materialize the tantivy writer until the first write: + /// an eager writer parked its pipeline threads (indexing workers, segment + /// updater, merge pool) for every warm repo in a serve pool — thousands of + /// resident OS threads on a large pool. Its Manual reload policy must + /// also make our own commit searchable with no reliance on the + /// OnCommitWithDelay poll loop. + #[test] + fn new_is_lazy_and_manual_commit_is_immediately_searchable() -> Result<()> { + let tmp = tempfile::tempdir()?; + let tantivy_lock = tmp.path().join("fts").join(".tantivy-writer.lock"); + + let mut store = FtsStore::new(tmp.path())?; + assert!( + !tantivy_lock.exists(), + "opening the store must not take the tantivy writer lock" + ); + + store.add_chunk( + 1, + "lazy tantivy writer probe", + "a.rs", + Some("fn a()"), + "function", + )?; + assert!( + tantivy_lock.exists(), + "first write must materialize the writer and its lockfile" + ); + store.commit()?; + + let results = store.search("lazy tantivy", 10, None)?; + assert_eq!(results.len(), 1); + assert_eq!(results[0].chunk_id, 1); + + Ok(()) + } } diff --git a/src/index/manager.rs b/src/index/manager.rs index 4690d3c6..9d2647a3 100644 --- a/src/index/manager.rs +++ b/src/index/manager.rs @@ -331,7 +331,12 @@ impl SharedStores { } let vector_store = VectorStore::new(db_path, dimensions)?; - let fts_store = FtsStore::new_with_writer(db_path)?; + // The FTS writer stays lazy on purpose: creating it eagerly parks + // tantivy's writer-pipeline threads (workers + segment updater + merge + // pool) for as long as serve holds the store — with one store per warm + // repo that was thousands of resident threads on a large pool. It + // materializes on the first actual write (FtsStore::ensure_writer). + let fts_store = FtsStore::new(db_path)?; info!("📦 SharedStores created in read-write mode"); @@ -351,7 +356,10 @@ impl SharedStores { /// File watching is not supported in readonly mode. pub fn new_readonly(db_path: &Path, dimensions: usize) -> Result { let vector_store = VectorStore::open_readonly(db_path, dimensions)?; - let fts_store = FtsStore::new(db_path)?; // Read-only without writer + // The OnCommitWithDelay watcher is deliberate here: by construction + // another process holds the writer lock and may commit at any time, + // and this reader must pick those commits up without a reopen. + let fts_store = FtsStore::new_readonly(db_path)?; info!("📦 SharedStores created in readonly mode"); @@ -3340,6 +3348,40 @@ mod tests { ); } + /// `SharedStores::new` must keep the FTS writer lazy: an eager tantivy + /// writer parked its pipeline threads (workers + segment updater + merge + /// pool) for every warm repo for as long as serve held the store — a + /// 500-repo pool was observed with ~4 500 resident OS threads. The + /// writer must materialize only on the first actual write, and the + /// Manual-policy reader must see our own commit without any watcher. + #[tokio::test] + async fn shared_stores_new_keeps_fts_writer_lazy_until_first_write() { + let temp = tempdir().unwrap(); + let db_path = temp.path().join(DB_DIR_NAME); + let stores = SharedStores::new(&db_path, 384).unwrap(); + let tantivy_lock = db_path.join("fts").join(".tantivy-writer.lock"); + assert!( + !tantivy_lock.exists(), + "SharedStores::new must not materialize the tantivy writer" + ); + + { + let mut fts = stores.fts_store.write().await; + fts.add_chunk(7, "shared lazy writer probe", "probe.rs", None, "block") + .unwrap(); + fts.commit().unwrap(); + } + + assert!( + tantivy_lock.exists(), + "first FTS write must materialize the writer and its lockfile" + ); + let fts = stores.fts_store.read().await; + let hits = fts.search("shared lazy", 10, None).unwrap(); + assert_eq!(hits.len(), 1); + assert_eq!(hits[0].chunk_id, 7); + } + #[test] fn acquire_writer_lock_succeeds_for_missing_directory() { // acquire_writer_lock must create the db directory before placing the lock diff --git a/src/index/mod.rs b/src/index/mod.rs index ec089068..14491c0b 100644 --- a/src/index/mod.rs +++ b/src/index/mod.rs @@ -9,7 +9,7 @@ use tracing::{debug, info}; use crate::cache::{normalize_path, safe_canonicalize, storage_key, FileMetaStore}; use crate::chunker::SemanticChunker; -use crate::db_discovery::{find_best_database, is_registered_repository, register_repository}; +use crate::db_discovery::find_best_database; use crate::embed::{EmbeddingService, ModelType}; use crate::file::FileWalker; use crate::fts::FtsStore; @@ -466,35 +466,102 @@ fn find_project_root(start_path: &Path) -> Option { None } -/// Get the global database path for a given directory -/// Uses ~/.codesearch.dbs// for storage -fn get_global_db_path(path: Option) -> Result<(PathBuf, PathBuf)> { - use dirs::home_dir; +/// Root of the `--global` store area. +/// +/// Honours `CODESEARCH_HOME` like every other global artifact (repos.json, +/// models cache, serve logs); the historic hardcoded `~/.codesearch.dbs` +/// wins when it already exists so indexes written by older binaries stay +/// reachable. Before this the root ignored the relocation knob entirely and +/// leaked stores into `$HOME` on machines that keep codesearch artifacts +/// elsewhere (the pilot's machine runs everything under `CODESEARCH_HOME`). +fn global_dbs_root() -> Result { + global_dbs_root_for( + dirs::home_dir().as_deref(), + &crate::constants::codesearch_home()?, + ) +} - let project_path = path.unwrap_or_else(|| PathBuf::from(".")); - let canonical_path = safe_canonicalize(&project_path)?; +/// Decision core of [`global_dbs_root`], split out so tests can drive the +/// legacy-vs-relocated choice: Windows `dirs::home_dir()` reads the profile +/// from the OS, not the environment, so no env override can redirect it. +fn global_dbs_root_for(home: Option<&Path>, cs_home: &Path) -> Result { + if let Some(home) = home { + let legacy = home.join(".codesearch.dbs"); + if legacy.exists() { + return Ok(legacy); + } + } + Ok(cs_home.join("dbs")) +} - // Create a unique name for the project based on its path - // Use the directory name as the project identifier - let project_name = canonical_path +/// Per-project directory name inside [`global_dbs_root`]. +/// +/// Bare `file_name()` collides: two same-named projects at different paths +/// would silently share ONE store (and one model stamp), corrupting each +/// other's index. The slug keeps the readable name and appends a short digest +/// of the full canonical path so same-named projects stay distinct. +fn global_store_slug(canonical_path: &Path) -> String { + use sha2::{Digest, Sha256}; + + let name = canonical_path .file_name() .and_then(|n| n.to_str()) .unwrap_or("unknown"); + let digest = Sha256::digest(canonical_path.to_string_lossy().as_bytes()); + let suffix: String = digest.iter().take(4).map(|b| format!("{b:02x}")).collect(); + format!("{name}-{suffix}") +} - // Create global database directory - let home = home_dir().ok_or_else(|| anyhow::anyhow!("No home directory found"))?; - let global_db_dir = home.join(".codesearch.dbs").join(project_name); - let db_path = global_db_dir.join(".codesearch.db"); +/// Directory holding the `--global` store for a canonical project path. +/// +/// Tries the historic `` layout first so indexes from older +/// binaries keep resolving; new projects get the collision-proof slug. +fn global_db_dir_for(canonical_path: &Path) -> Result { + global_db_dir_in(&global_dbs_root()?, canonical_path) +} - // Register this repository in the global tracking - register_repository(&canonical_path)?; +/// [`global_db_dir_for`] against an explicit root — the testable seam for +/// the legacy-layout preference (see [`global_dbs_root_for`]). +fn global_db_dir_in(root: &Path, canonical_path: &Path) -> Result { + let legacy_name = canonical_path + .file_name() + .and_then(|n| n.to_str()) + .unwrap_or("unknown"); + let legacy = root.join(legacy_name); + if legacy.join(crate::constants::DB_DIR_NAME).exists() { + return Ok(legacy); + } + Ok(root.join(global_store_slug(canonical_path))) +} + +/// Whether a `--global` store exists for a canonical project path. +fn has_global_store(canonical_path: &Path) -> bool { + global_db_dir_for(canonical_path) + .map(|dir| dir.join(crate::constants::DB_DIR_NAME).exists()) + .unwrap_or(false) +} + +/// Get the global database path for a given directory. +/// +/// Lives under [`global_dbs_root`] (CODESEARCH_HOME-aware with a legacy +/// `~/.codesearch.dbs` fallback), one directory per project. Deliberately does +/// NOT register the repository: global indexes are documented as "not +/// auto-registered", and the old registration actively harmed serve — +/// discovery looks for the store at `/.codesearch.db`, never finds +/// the out-of-tree one, and a running serve then builds a SECOND, empty local +/// store for the registered alias while the global index sits unused. +fn get_global_db_path(path: Option) -> Result<(PathBuf, PathBuf)> { + let project_path = path.unwrap_or_else(|| PathBuf::from(".")); + let canonical_path = safe_canonicalize(&project_path)?; + + let db_path = global_db_dir_for(&canonical_path)?.join(".codesearch.db"); println!( "{}", format!( "🌍 Using global database: {}\n (project: {})", db_path.display(), - project_name + canonical_path.display() ) .dimmed() ); @@ -1689,7 +1756,9 @@ pub async fn add_to_index( let local_db = canonical_path.join(".codesearch.db"); let has_local = local_db.exists(); - let has_global = is_registered_repository(&canonical_path)?; + // Real existence check of the out-of-tree `--global` store (see + // remove_from_index for why the repos.json registration is not it). + let has_global = has_global_store(&canonical_path); // Conflict checks if global && has_local { @@ -1845,9 +1914,15 @@ pub async fn remove_from_index(path: Option, keep_config: bool) -> Resu let local_db = canonical_path.join(".codesearch.db"); let has_local = local_db.exists(); - let has_global = is_registered_repository(&canonical_path)?; + // Real existence check of the out-of-tree `--global` store — the old + // `is_registered_repository` here conflated the repos.json registration + // with a global store and then printed "Global index removed!" without + // deleting any files. Registration is tracked separately below so a + // stale repos.json entry can still be cleaned up without any store. + let has_global = has_global_store(&canonical_path); + let registered = crate::db_discovery::is_registered_repository(&canonical_path)?; - if !has_local && !has_global { + if !has_local && !has_global && !registered { println!("\n{}", "⚠️ No index found for this project.".yellow()); return Ok(()); } @@ -1905,11 +1980,25 @@ pub async fn remove_from_index(path: Option, keep_config: bool) -> Resu println!("{}", "ℹ️ Config entry preserved.".cyan()); } - // `!keep_config` matters: with --keep-config the entry is deliberately - // left in repos.json, so claiming the global index was removed would - // contradict the "Config entry preserved." line printed just above and - // send the caller looking for a cleanup that never happened. - if !has_local && has_global && !keep_config { + // The `--global` store lives out-of-tree; delete its files too. The old + // code only printed "Global index removed!" after the unregister — the + // actual store survived on disk, and once registration stopped implying a + // global store the message became pure fiction. + if has_global && !keep_config { + if let Ok(global_dir) = global_db_dir_for(&canonical_path) { + let global_db = global_dir.join(".codesearch.db"); + if global_db.exists() { + println!("\n{}", "Removing global index...".cyan()); + if let Err(e) = fs::remove_dir_all(&global_db) { + eprintln!( + "{}", + "⚠️ Global store files may be locked by a running codesearch serve." + .yellow() + ); + return Err(anyhow::anyhow!("Failed to remove global index: {}", e)); + } + } + } println!("{}", "✅ Global index removed!".green()); } @@ -3434,6 +3523,118 @@ mod remove_order_tests { ); assert_global_config_unchanged(_canary); } + + // --- --global store root: CODESEARCH_HOME-aware, collision-proof slugs ---- + + /// `index --global` must keep every artifact under the relocatable + /// codesearch root and must not map two same-named projects onto ONE + /// store (the bare `file_name()` layout did both). + #[test] + #[serial] + fn global_db_path_honours_codesearch_home_and_slug_is_collision_proof() { + let tmp = tempfile::tempdir().unwrap(); + let cs_home = tmp.path().join("cs-home"); + std::fs::create_dir_all(&cs_home).unwrap(); + // A separate $HOME with no legacy `.codesearch.dbs` keeps the test + // hermetic on machines that happen to have the old layout at home. + let bare_home = tmp.path().join("bare-home"); + std::fs::create_dir_all(&bare_home).unwrap(); + let repos_cfg = tmp.path().join("repos.json"); + let _env = EnvRestore::set(&[ + ( + crate::constants::CODESEARCH_HOME_ENV, + cs_home.to_str().unwrap(), + ), + ("HOME", bare_home.to_str().unwrap()), + ( + crate::constants::REPOS_CONFIG_ENV, + repos_cfg.to_str().unwrap(), + ), + ]); + + std::fs::create_dir_all(tmp.path().join("a").join("foo")).unwrap(); + std::fs::create_dir_all(tmp.path().join("b").join("foo")).unwrap(); + let foo_a = crate::cache::safe_canonicalize(&tmp.path().join("a").join("foo")) + .expect("project dir must exist"); + let foo_b = crate::cache::safe_canonicalize(&tmp.path().join("b").join("foo")) + .expect("project dir must exist"); + + let (db_a, proj_a) = super::get_global_db_path(Some(foo_a.clone())).unwrap(); + let (db_b, _) = super::get_global_db_path(Some(foo_b.clone())).unwrap(); + + assert_eq!(proj_a, foo_a, "project path round-trips canonicalized"); + assert_ne!( + db_a, db_b, + "same-named projects must not share one global store" + ); + for db in [&db_a, &db_b] { + assert!( + db.starts_with(cs_home.join("dbs")), + "global stores must live under CODESEARCH_HOME/dbs, got {}", + db.display() + ); + } + let slug = db_a + .parent() + .unwrap() + .file_name() + .unwrap() + .to_string_lossy() + .into_owned(); + assert!( + slug.starts_with("foo-") && slug.len() > "foo-".len(), + "slug must keep the readable name plus a path digest: {slug}" + ); + + // No auto-registration side effect: a running serve would otherwise + // resolve the alias to /.codesearch.db and build an empty + // LOCAL store while the out-of-tree global one sits unused. + assert!( + !repos_cfg.exists(), + "get_global_db_path must not register the repo in repos.json" + ); + + // Existence-based detection: only the project whose store was + // materialized reports a global index. + std::fs::create_dir_all(&db_a).unwrap(); + assert!(super::has_global_store(&foo_a)); + assert!(!super::has_global_store(&foo_b)); + } + + /// Stores written by older binaries into the hardcoded `~/.codesearch.dbs` + /// keep resolving after the fix — the legacy layout wins when present. + /// + /// Windows `dirs::home_dir()` reads the profile from the OS, not the + /// environment, so the decision is driven through the injected seam + /// ([`global_dbs_root_for`] / [`global_db_dir_in`]) with a simulated home. + #[test] + fn global_db_path_prefers_legacy_home_layout_when_it_exists() { + let tmp = tempfile::tempdir().unwrap(); + let old_home = tmp.path().join("old-home"); + let legacy_root = old_home.join(".codesearch.dbs"); + let proj = tmp.path().join("proj").join("foo"); + std::fs::create_dir_all(&proj).unwrap(); + std::fs::create_dir_all(legacy_root.join("foo").join(".codesearch.db")).unwrap(); + let cs_home = tmp.path().join("cs-home"); + + let canonical = crate::cache::safe_canonicalize(&proj).unwrap(); + let root = super::global_dbs_root_for(Some(old_home.as_path()), &cs_home).unwrap(); + assert_eq!( + root, legacy_root, + "an existing legacy store must keep resolving from ~/.codesearch.dbs" + ); + let db = super::global_db_dir_in(&root, &canonical).unwrap(); + assert!( + db.starts_with(&legacy_root), + "an existing legacy store must keep resolving from ~/.codesearch.dbs, got {}", + db.display() + ); + assert_eq!( + db.file_name().unwrap(), + "foo", + "legacy layout uses the bare project name" + ); + } } /// Model resolution for the CLI `index` path: the model recorded in the index's diff --git a/src/mcp/explore.rs b/src/mcp/explore.rs index 6d80e21a..395c304f 100644 --- a/src/mcp/explore.rs +++ b/src/mcp/explore.rs @@ -86,25 +86,31 @@ impl CodesearchService { if let Some(ref sv) = ctx.stores_vec { let aliases = ctx.aliases(); let mut all_items: Vec = Vec::new(); - let mut seen_ids: std::collections::HashSet = std::collections::HashSet::new(); + // Dedup keys on (store, id): chunk ids are per-repo counters and + // collide across the fan-out, so a bare-id set would drop a later + // repo's outline entries. + let mut seen_ids: std::collections::HashSet<(usize, u32)> = + std::collections::HashSet::new(); for (store_idx, store_arc) in sv.iter().enumerate() { - let store = match bounded_vector_read(&store_arc.vector_store).await { - Ok(store) => store, - Err(e) => { - note_store_failure(warnings, aliases, store_idx, "outline scan", &e); - continue; - } + let Some(store) = try_vector_read_or_note( + &store_arc.vector_store, + aliases, + store_idx, + warnings, + "outline scan", + ) else { + continue; }; match store.chunks_for_file(normalized) { Ok(metas) => { for c in metas { - if seen_ids.insert(c.id) { + if seen_ids.insert((store_idx, c.id)) { all_items.push(FileOutlineItem { chunk_id: c.id, kind: c.kind, signature: c.signature, - start_line: c.start_line, - end_line: c.end_line, + start_line: c.start_line + 1, + end_line: c.end_line + 1, }); } } @@ -127,8 +133,8 @@ impl CodesearchService { chunk_id: c.id, kind: c.kind, signature: c.signature, - start_line: c.start_line, - end_line: c.end_line, + start_line: c.start_line + 1, + end_line: c.end_line + 1, }) .collect(); out.sort_by_key(|i| i.start_line); diff --git a/src/mcp/federation_helpers.rs b/src/mcp/federation_helpers.rs index 6fa8dd61..d86fb0b8 100644 --- a/src/mcp/federation_helpers.rs +++ b/src/mcp/federation_helpers.rs @@ -103,6 +103,20 @@ pub(crate) fn parse_search_items_from_call_result( } } +/// Extract an explicit refusal `note` from a rendered search payload (a +/// leg emptied by `min_score` explains itself in the note). The federated +/// response surfaces it as a warning so a leg's honest refusal never reads +/// as "no hits there". +pub(crate) fn parse_note_from_call_result(result: &CallToolResult) -> Option { + let text = extract_call_tool_text(result); + let value: serde_json::Value = serde_json::from_str(&text).ok()?; + value + .get("note") + .and_then(|n| n.as_str()) + .map(str::to_string) + .filter(|n| !n.is_empty()) +} + /// Convert a remote search hit into a local `SearchResultItem`, tagging it with /// its origin (`source`) and a project-namespaced `chunk_ref` for later /// retrieval. diff --git a/src/mcp/federation_helpers_tests.rs b/src/mcp/federation_helpers_tests.rs index 01491d7d..b30b3f2f 100644 --- a/src/mcp/federation_helpers_tests.rs +++ b/src/mcp/federation_helpers_tests.rs @@ -1,7 +1,10 @@ //! Unit tests for the federation merge/parse/convert helpers. The //! FederationClient HTTP layer + resolve_group_targets are covered //! separately (federation/mod.rs and db_discovery/repos.rs respectively). -use super::{convert_remote_item, merge_ranked_lists, parse_search_items_from_call_result}; +use super::{ + convert_remote_item, merge_ranked_lists, parse_note_from_call_result, + parse_search_items_from_call_result, +}; use crate::federation::RemoteSearchItem; use crate::mcp::types::SearchResultItem; use rmcp::model::{CallToolResult, ContentBlock}; @@ -143,6 +146,31 @@ fn parse_federated_chunk_ref_rejects_garbage() { assert!(super::parse_federated_chunk_ref("cloud:notanumber").is_none()); } +// === parse_note_from_call_result (leg refusal relay) =================== + +#[test] +fn parse_note_extracts_a_leg_refusal_and_ignores_noteless_payloads() { + // The semantic refusal shape: results emptied by min_score, note naming + // the threshold. A federated leg answering this must not read as + // "no hits there" — the note is the difference. + let refusal = serde_json::json!({ + "results": [], + "note": "2 candidate hit(s) found but all scored below min_score 0.800; \ + refusing instead of returning the nearest neighbours." + }) + .to_string(); + let note = parse_note_from_call_result(&call_result_with_json(&refusal)); + let note = note.expect("refusal note must be extracted"); + assert!(note.contains("min_score 0.800"), "got: {note}"); + + // A normal answer carries no note — and an empty-string note (a peer + // that echoes the field unconditionally) must not surface as one. + let plain = serde_json::json!({ "results": [{ "path": "a.rs", "score": 0.9 }] }).to_string(); + assert!(parse_note_from_call_result(&call_result_with_json(&plain)).is_none()); + let blank = serde_json::json!({ "results": [], "note": "" }).to_string(); + assert!(parse_note_from_call_result(&call_result_with_json(&blank)).is_none()); +} + // === parse_search_items_from_call_result (serve-delegation re-parse) === fn call_result_with_json(json: &str) -> CallToolResult { diff --git a/src/mcp/find.rs b/src/mcp/find.rs index fa58fe74..0a894817 100644 --- a/src/mcp/find.rs +++ b/src/mcp/find.rs @@ -137,7 +137,18 @@ impl CodesearchService { ) .await { - Ok(r) => r, + // Single store: the tag is still written for uniformity (the + // resolution block below reads it in both modes); the routed + // project alias is the honest origin when one exists. + Ok(r) => { + let alias = ctx + .project_alias + .clone() + .unwrap_or_else(|| "local".to_string()); + r.into_iter() + .map(|hit| SourcedResult::new(alias.clone(), hit)) + .collect() + } Err(e) => { return Ok(CallToolResult::success(vec![ContentBlock::text(format!( "Error searching: {e:#}" @@ -161,39 +172,34 @@ impl CodesearchService { // Resolve chunk metadata and filter by definition kinds let requested_kind = request.kind.clone(); let mut items: Vec = if let Some(ref sv) = ctx.stores_vec { + // Group fan-out: resolve each hit ONLY in its origin store. Chunk + // ids are per-repo counters, so probing every store for a bare id + // misattributes on collision: the first repo holding that id won + // the hit, true definitions were filtered out as "wrong kind" + // (they resolved to the OTHER repo's chunk), and unrelated + // content rode the stolen score. let aliases = ctx.aliases(); let mut items: Vec = Vec::new(); - 'outer: for fts_result in &fts_results { - for (store_idx, store_arc) in sv.iter().enumerate() { - let store = match bounded_vector_read(&store_arc.vector_store).await { - Ok(store) => store, - Err(e) => { - note_store_failure( - &mut find_warnings, - aliases, - store_idx, - "chunk lookup", - &e, - ); - continue; - } - }; - let looked_up = store.get_chunk(fts_result.chunk_id); - if let Err(ref e) = looked_up { - // `Ok(None)` = chunk not in this store (normal during - // fan-out); `Err` = broken store. Skipping the `Err` - // silently made a dead store look like "symbol not - // found" — carry it in the warnings channel instead. - note_store_failure( - &mut find_warnings, - aliases, - store_idx, - "chunk lookup", - e, - ); - } - if let Ok(Some(chunk)) = looked_up { - // Skip non-definition kinds — try next FTS result, not next store + 'outer: for fts_hit in &fts_results { + let Some(origin_idx) = aliases.iter().position(|a| *a == fts_hit.alias) else { + // The fan-out only emits aliases from this same vec; a + // miss means routing changed under our feet. Skip the hit + // rather than guess where it came from. + continue 'outer; + }; + let store_arc = &sv[origin_idx]; + let Some(store) = try_vector_read_or_note( + &store_arc.vector_store, + aliases, + origin_idx, + &mut find_warnings, + "chunk lookup", + ) else { + continue 'outer; + }; + match store.get_chunk(fts_hit.result.chunk_id) { + Ok(Some(chunk)) => { + // Skip non-definition kinds — try next FTS result if !DEFINITION_KINDS.contains(&chunk.kind.as_str()) { continue 'outer; } @@ -203,22 +209,29 @@ impl CodesearchService { } } items.push(ReferenceItem { - chunk_id: fts_result.chunk_id, - path: chunk.path, - line: chunk.start_line, + chunk_id: fts_hit.result.chunk_id, + path: ctx.prefix_sourced_path(&fts_hit.alias, &chunk.path), + line: chunk.start_line + 1, kind: chunk.kind, signature: chunk.signature, - score: fts_result.score, + score: fts_hit.result.score, }); if items.len() >= limit { break 'outer; } - break; // Found in this store — move to next FTS result + } + Ok(None) => continue 'outer, // not held anywhere — skip + Err(ref e) => { + note_store_failure( + &mut find_warnings, + aliases, + origin_idx, + "chunk lookup", + e, + ); + continue 'outer; } } - // If we get here, the chunk was Ok(None) in every store (not - // held anywhere — skip it) or its lookups failed (noted in - // find_warnings above). } items } else { @@ -231,9 +244,9 @@ impl CodesearchService { // missing chunk — `Ok(None)` alone is a true miss. let resolved: anyhow::Result> = fts_results .iter() - .map(|fts_result| { - let chunk = store.get_chunk(fts_result.chunk_id)?; - Ok((chunk, fts_result.chunk_id, fts_result.score)) + .map(|fts_hit| { + let chunk = store.get_chunk(fts_hit.result.chunk_id)?; + Ok((chunk, fts_hit.result.chunk_id, fts_hit.result.score)) }) .collect(); let items = resolved? @@ -251,7 +264,7 @@ impl CodesearchService { Some(ReferenceItem { chunk_id, path: chunk.path, - line: chunk.start_line, + line: chunk.start_line + 1, kind: chunk.kind, signature: chunk.signature, score, @@ -274,9 +287,13 @@ impl CodesearchService { } }; - // Prefix paths with alias for multi-repo identification - for item in &mut items { - item.path = ctx.prefix_result_path(&item.path); + // Prefix paths with the routing alias. Group items were already + // attributed to their origin repo at resolution time; re-prefixing + // here would double the alias onto those paths. + if ctx.stores_vec.is_none() { + for item in &mut items { + item.path = ctx.prefix_result_path(&item.path); + } } respond_with_items(&items, &find_warnings, || { @@ -348,7 +365,17 @@ impl CodesearchService { ) .await { - Ok(r) => r, + // Single store — tagged for uniformity with the resolution + // block below (see find_definition). + Ok(r) => { + let alias = ctx + .project_alias + .clone() + .unwrap_or_else(|| "local".to_string()); + r.into_iter() + .map(|hit| SourcedResult::new(alias.clone(), hit)) + .collect() + } Err(e) => { return Ok(CallToolResult::success(vec![ContentBlock::text(format!( "Error searching: {e:#}" @@ -370,45 +397,48 @@ impl CodesearchService { let mut items: Vec = if let Some(ref sv) = ctx.stores_vec { let aliases = ctx.aliases(); let mut items: Vec = Vec::new(); - for fts_result in &fts_results { - for (store_idx, store_arc) in sv.iter().enumerate() { - let store = match bounded_vector_read(&store_arc.vector_store).await { - Ok(store) => store, - Err(e) => { - note_store_failure( - &mut find_warnings, - aliases, - store_idx, - "chunk lookup", - &e, - ); - continue; + // Same rule as find_definition: resolve ONLY in the origin store — + // probing all stores for a bare chunk id misattributes on the + // cross-repo id collisions. + for fts_hit in &fts_results { + let Some(origin_idx) = aliases.iter().position(|a| *a == fts_hit.alias) else { + continue; + }; + let store_arc = &sv[origin_idx]; + let Some(store) = try_vector_read_or_note( + &store_arc.vector_store, + aliases, + origin_idx, + &mut find_warnings, + "chunk lookup", + ) else { + continue; + }; + match store.get_chunk(fts_hit.result.chunk_id) { + Ok(Some(chunk)) => { + if !is_definition_chunk(&chunk.kind, &chunk.signature, &symbol) { + items.push(ReferenceItem { + chunk_id: fts_hit.result.chunk_id, + path: ctx.prefix_sourced_path(&fts_hit.alias, &chunk.path), + line: chunk.start_line + 1, + kind: chunk.kind, + signature: chunk.signature, + score: fts_hit.result.score, + }); } - }; - let looked_up = store.get_chunk(fts_result.chunk_id); - if let Err(ref e) = looked_up { + } + Ok(None) => continue, // not held anywhere — skip + Err(ref e) => { // Same rule as find_definition: `Err` is a broken // store, not "no usages" — carry it in the channel. note_store_failure( &mut find_warnings, aliases, - store_idx, + origin_idx, "chunk lookup", e, ); - } - if let Ok(Some(chunk)) = looked_up { - if !is_definition_chunk(&chunk.kind, &chunk.signature, &symbol) { - items.push(ReferenceItem { - chunk_id: fts_result.chunk_id, - path: chunk.path, - line: chunk.start_line, - kind: chunk.kind, - signature: chunk.signature, - score: fts_result.score, - }); - } - break; + continue; } } } @@ -422,9 +452,9 @@ impl CodesearchService { // chunk or a miss — `Ok(None)` alone is a true miss. let resolved: anyhow::Result> = fts_results .iter() - .map(|fts_result| { - let chunk = store.get_chunk(fts_result.chunk_id)?; - Ok((chunk, fts_result.chunk_id, fts_result.score)) + .map(|fts_hit| { + let chunk = store.get_chunk(fts_hit.result.chunk_id)?; + Ok((chunk, fts_hit.result.chunk_id, fts_hit.result.score)) }) .collect(); let items = resolved? @@ -437,7 +467,7 @@ impl CodesearchService { Some(ReferenceItem { chunk_id, path: chunk.path, - line: chunk.start_line, + line: chunk.start_line + 1, kind: chunk.kind, signature: chunk.signature, score, @@ -459,9 +489,13 @@ impl CodesearchService { } }; - // Prefix paths with alias for multi-repo identification - for item in &mut items { - item.path = ctx.prefix_result_path(&item.path); + // Prefix paths with the routing alias. Group items were already + // attributed to their origin repo at resolution time; re-prefixing + // here would double the alias onto those paths. + if ctx.stores_vec.is_none() { + for item in &mut items { + item.path = ctx.prefix_result_path(&item.path); + } } // Lexical FTS ranks docs, comments and code by the same text score, diff --git a/src/mcp/get_chunk.rs b/src/mcp/get_chunk.rs index e6cdf9fd..1a9ebb2e 100644 --- a/src/mcp/get_chunk.rs +++ b/src/mcp/get_chunk.rs @@ -107,18 +107,34 @@ impl CodesearchService { // Look up chunk — multi-store: smart candidate detection for chunk_id collision. // chunk_ids are local per database, not globally unique. When no project is specified // and multiple stores are active, scan all stores to find which ones have this chunk_id. + // + // The store that answers becomes `owner_alias` so the context read below + // can resolve the project-relative stored path against the OWNING + // repo's root (`ctx.alias_roots`) — the serve process's own + // project_path is not any repo's root, so joining there always failed + // and group-scoped answers never carried surrounding lines. + // `stored_path_raw` is published only by the unique-owner group arm, + // which prefixes chunk.path inside itself before returning. + let mut owner_alias: Option = None; + let mut stored_path_raw: Option = None; let chunk = if let Some(ref sv) = ctx.stores_vec { if sv.len() > 1 && request.project.is_none() { // Smart candidate detection: find which stores actually contain this chunk_id let mut candidates: Vec<(&Arc, String)> = Vec::new(); let aliases = ctx.aliases(); for (i, store_arc) in sv.iter().enumerate() { - let store = match bounded_vector_read(&store_arc.vector_store).await { - Ok(store) => store, - Err(e) => { - note_store_failure(&mut chunk_warnings, aliases, i, "chunk lookup", &e); - continue; - } + // Try-lock, never wait: this loop scans EVERY repo in the + // group, so bounded waits summed to N×300 s while a single + // indexing run held them — the same interactive wedge the + // search fan-outs killed. + let Some(store) = try_vector_read_or_note( + &store_arc.vector_store, + aliases, + i, + &mut chunk_warnings, + "chunk lookup", + ) else { + continue; }; match store.get_chunk(request.chunk_id) { Ok(Some(_)) => { @@ -165,18 +181,39 @@ impl CodesearchService { serve_state.record_tool_call(alias, "get_chunk"); serve_state.touch_access(alias); } - let store = match bounded_vector_read(&store_arc.vector_store).await { + // Try-lock like the scan above: the store answered the + // scan moments ago; waiting here reintroduces the very + // wedge the scan just removed whenever indexing took + // the lock in between. + let store = match store_arc.vector_store.try_read() { Ok(store) => Some(store), - Err(e) => { + Err(_) => { push_store_warning( &mut chunk_warnings, - &store_warning(alias, "chunk lookup", &format!("{e:#}")), + &store_warning( + alias, + "chunk lookup", + "store busy: active indexing holds the write lock — \ + repo skipped in this fan-out, retry shortly", + ), ); None } }; + // The owning alias is known here — prefix with it + // directly. `prefix_result_path` infers the alias by + // path-root matching, which never matches a + // project-relative stored path, so the group-scoped + // answer used to ship unprefixed. + let root = ctx.alias_roots.get(alias).map(String::as_str).unwrap_or(""); match store.as_ref().map(|s| s.get_chunk(request.chunk_id)) { - Some(Ok(c)) => c, + Some(Ok(Some(mut c))) => { + owner_alias = Some(alias.clone()); + stored_path_raw = Some(c.path.clone()); + c.path = prefix_path_with_alias(&c.path, Some(alias), root); + Some(c) + } + Some(Ok(None)) => None, Some(Err(ref e)) => { push_store_warning( &mut chunk_warnings, @@ -219,6 +256,7 @@ impl CodesearchService { }; match store.get_chunk(request.chunk_id) { Ok(Some(c)) => { + owner_alias = aliases.get(i).cloned(); found = Some(c); break; } @@ -242,7 +280,10 @@ impl CodesearchService { ) .await { - Ok(c) => c, + Ok(c) => { + owner_alias = ctx.project_alias.clone(); + c + } Err(e) => { push_store_warning( &mut chunk_warnings, @@ -272,6 +313,12 @@ impl CodesearchService { } }; + // The ORIGINAL project-relative stored path for the context read + // below, captured before display prefixing rewrites chunk.path. The + // group's unique-owner arm already rewrote its chunk's path inside + // itself, so it publishes the pre-prefix copy via `stored_path_raw`. + let stored_path = stored_path_raw.take().unwrap_or_else(|| chunk.path.clone()); + // Prefix path with alias for multi-repo identification chunk.path = ctx.prefix_result_path(&chunk.path); @@ -280,9 +327,19 @@ impl CodesearchService { let mut note = None; if context_lines > 0 { - // Resolve relative chunk paths against project root (not process CWD). + // Resolve relative chunk paths against the OWNING repo's root — + // chunk paths are project-relative, and in serve mode the service's + // own project_path is not that repo's root, so the old join never + // found the file. Stdio single-repo mode has no alias roots and + // keeps the historical project_path join. let source_path = if Path::new(&chunk.path).is_absolute() { PathBuf::from(&chunk.path) + } else if let Some(root) = owner_alias + .as_deref() + .and_then(|alias| ctx.alias_roots.get(alias)) + .filter(|root| !root.is_empty()) + { + Path::new(root).join(stored_path.trim_start_matches('/')) } else { self.project_path.join(&chunk.path) }; @@ -290,11 +347,12 @@ impl CodesearchService { Ok(src) => { let lines: Vec<&str> = src.lines().collect(); if !lines.is_empty() { - // Chunk lines are 1-based; slices need the 0-based - // row before the chunk's first line. - let before_end = chunk.start_line.saturating_sub(1); + // Chunk lines are 0-based (the 1-based conversion + // happens once at the emit boundary below); the + // slice ends at the row before the chunk's first + // line, which is start_line itself. + let before_end = chunk.start_line.min(lines.len()); let before_start = before_end.saturating_sub(context_lines); - let before_end = before_end.min(lines.len()); if before_start < before_end { context_before = Some(lines[before_start..before_end].join("\n")); } @@ -317,8 +375,8 @@ impl CodesearchService { let response = GetChunkResponse { chunk_id: request.chunk_id, path: chunk.path, - start_line: chunk.start_line, - end_line: chunk.end_line, + start_line: chunk.start_line + 1, + end_line: chunk.end_line + 1, kind: chunk.kind, signature: chunk.signature, content: chunk.content, diff --git a/src/mcp/graph.rs b/src/mcp/graph.rs index cffca07b..db90acf0 100644 --- a/src/mcp/graph.rs +++ b/src/mcp/graph.rs @@ -53,23 +53,23 @@ impl CodesearchService { let mut import_warnings: Vec = Vec::new(); let mut items = if let Some(ref sv) = ctx.stores_vec { - // Multi-store group fan-out: collect import items from all stores + // Multi-store group fan-out: collect import items from all stores. + // Dedup keys on (store, id): chunk ids are per-repo counters and + // collide across repos, so a bare-id set would drop a later repo's + // imports wholesale. let import_aliases = ctx.aliases(); let mut all_items: Vec = Vec::new(); - let mut seen_ids: std::collections::HashSet = std::collections::HashSet::new(); + let mut seen_ids: std::collections::HashSet<(usize, u32)> = + std::collections::HashSet::new(); for (store_idx, store_arc) in sv.iter().enumerate() { - let store = match bounded_vector_read(&store_arc.vector_store).await { - Ok(store) => store, - Err(e) => { - note_store_failure( - &mut import_warnings, - import_aliases, - store_idx, - "chunk lookup", - &e, - ); - continue; - } + let Some(store) = try_vector_read_or_note( + &store_arc.vector_store, + import_aliases, + store_idx, + &mut import_warnings, + "chunk lookup", + ) else { + continue; }; match store.chunks_for_file(&normalized) { Ok(metas) => { @@ -77,7 +77,7 @@ impl CodesearchService { if !is_import_kind(&meta.kind) { continue; } - if seen_ids.insert(meta.id) { + if seen_ids.insert((store_idx, meta.id)) { match store.get_chunk(meta.id) { Ok(Some(chunk)) => all_items.extend(parse_import_lines( &chunk.content, @@ -141,8 +141,11 @@ impl CodesearchService { // Limitation: this only finds chunks containing these literal words; // language-specific import forms that lack these keywords will be missed. let fallback_limit = 40usize; - let mut all_hits: Vec<(u32, f32)> = Vec::new(); - let mut seen_fts_ids: HashSet = HashSet::new(); + // Origin-tagged FTS hits (dedup key includes the repo: ids collide + // across stores). + let mut all_hits: Vec> = Vec::new(); + let mut seen_fts_ids: std::collections::HashSet<(String, u32)> = + std::collections::HashSet::new(); if let Some(ref sv) = ctx.stores_vec { let import_aliases = ctx.aliases(); @@ -158,51 +161,49 @@ impl CodesearchService { .unwrap_or_default() .into_results(&mut import_warnings, "imports search"); for h in hits { - if seen_fts_ids.insert(h.chunk_id) { - all_hits.push((h.chunk_id, h.score)); + if seen_fts_ids.insert((h.alias.clone(), h.result.chunk_id)) { + all_hits.push(h); } } } - // Resolve FTS hits via vector stores + // Resolve each FTS hit in its ORIGIN store — probing every + // store for a bare chunk id answers from the wrong repo on the + // cross-repo id collisions. let mut resolved: Vec = Vec::new(); - for (chunk_id, _) in &all_hits { - for (store_idx, store_arc) in sv.iter().enumerate() { - let store = match bounded_vector_read(&store_arc.vector_store).await { - Ok(store) => store, - Err(e) => { - note_store_failure( - &mut import_warnings, - import_aliases, - store_idx, - "chunk lookup", - &e, - ); - continue; - } - }; - match store.get_chunk(*chunk_id) { - Ok(Some(chunk)) => { - if crate::cache::normalize_path_str(&chunk.path) == normalized { - resolved.extend(parse_import_lines( - &chunk.content, - chunk.start_line, - )); - } - break; - } - Ok(None) => continue, - Err(ref e) => { - note_store_failure( - &mut import_warnings, - import_aliases, - store_idx, - "chunk lookup", - e, - ); - continue; + for hit in &all_hits { + let Some(origin_idx) = import_aliases.iter().position(|a| *a == hit.alias) + else { + continue; + }; + let store_arc = &sv[origin_idx]; + let Some(store) = try_vector_read_or_note( + &store_arc.vector_store, + import_aliases, + origin_idx, + &mut import_warnings, + "chunk lookup", + ) else { + continue; + }; + match store.get_chunk(hit.result.chunk_id) { + Ok(Some(chunk)) => { + if crate::cache::normalize_path_str(&chunk.path) == normalized { + resolved + .extend(parse_import_lines(&chunk.content, chunk.start_line)); } } + Ok(None) => continue, + Err(ref e) => { + note_store_failure( + &mut import_warnings, + import_aliases, + origin_idx, + "chunk lookup", + e, + ); + continue; + } } } items = resolved; @@ -229,9 +230,12 @@ impl CodesearchService { Vec::new() } }; + // Single store: tag with the routed alias for uniformity; + // resolution below runs in this same store. + let own_alias = ctx.project_alias.clone().unwrap_or_default(); for h in hits { - if seen_fts_ids.insert(h.chunk_id) { - all_hits.push((h.chunk_id, h.score)); + if seen_fts_ids.insert((own_alias.clone(), h.chunk_id)) { + all_hits.push(SourcedResult::new(own_alias.clone(), h)); } } } @@ -240,8 +244,8 @@ impl CodesearchService { .with_vector_store_read_for( |store| { let mut out = Vec::new(); - for (chunk_id, _) in &all_hits { - if let Some(chunk) = store.get_chunk(*chunk_id)? { + for hit in &all_hits { + if let Some(chunk) = store.get_chunk(hit.result.chunk_id)? { if crate::cache::normalize_path_str(&chunk.path) == normalized { out.extend(parse_import_lines( &chunk.content, @@ -358,11 +362,20 @@ impl CodesearchService { exact_hits } } else { - // Single-store FTS search - let alias = ctx.project_alias.as_deref().unwrap_or("unknown"); + // Single-store FTS search — tagged for uniformity with the + // resolution block below. let mut run = |r: anyhow::Result>| match r { - Ok(hits) => hits, + Ok(hits) => { + let alias = ctx + .project_alias + .clone() + .unwrap_or_else(|| "local".to_string()); + hits.into_iter() + .map(|hit| SourcedResult::new(alias.clone(), hit)) + .collect() + } Err(e) => { + let alias = ctx.project_alias.as_deref().unwrap_or("unknown"); push_store_warning( &mut dep_warnings, &store_warning(alias, "dependents search", &format!("{e:#}")), @@ -390,73 +403,69 @@ impl CodesearchService { }; let mut items = if let Some(ref sv) = ctx.stores_vec { - // Multi-store: resolve chunks across all stores + // Multi-store: resolve each hit in its ORIGIN store only — a bare + // chunk id collides across repos and "first store that answers" + // steals the hit (see SourcedResult). let dep_aliases = ctx.aliases(); let mut seen_paths = HashSet::new(); let mut out = Vec::new(); + let term_lower = search_term.to_lowercase(); for f in &fts_results { - for (store_idx, store_arc) in sv.iter().enumerate() { - let store = match bounded_vector_read(&store_arc.vector_store).await { - Ok(store) => store, - Err(e) => { - note_store_failure( - &mut dep_warnings, - dep_aliases, - store_idx, - "chunk lookup", - &e, - ); - continue; + let Some(origin_idx) = dep_aliases.iter().position(|a| *a == f.alias) else { + continue; + }; + let store_arc = &sv[origin_idx]; + let Some(store) = try_vector_read_or_note( + &store_arc.vector_store, + dep_aliases, + origin_idx, + &mut dep_warnings, + "chunk lookup", + ) else { + continue; + }; + match store.get_chunk(f.result.chunk_id) { + Ok(Some(chunk)) => { + if !is_import_kind(&chunk.kind) { + continue; // try next FTS result } - }; - match store.get_chunk(f.chunk_id) { - Ok(Some(chunk)) => { - if !is_import_kind(&chunk.kind) { - break; // try next FTS result - } - - let norm = crate::cache::normalize_path_str(&chunk.path); - if !seen_paths.insert(norm) { - break; - } - let term_lower = search_term.to_lowercase(); - let import_statement = - if chunk.content.to_lowercase().contains(&term_lower) { - chunk - .content - .lines() - .find(|l| l.to_lowercase().contains(&term_lower)) - .unwrap_or("") - .to_string() - } else { - chunk.signature.filter(|s| !s.is_empty()).unwrap_or( - chunk.content.lines().next().unwrap_or("").to_string(), - ) - }; - - out.push(DependentItem { - path: chunk.path, - line: chunk.start_line, - import_statement, - }); - - break; // found in this store, move to next FTS result - } - Ok(None) => {} // try next store - // One broken store says nothing about the others; a - // `break` here silently drops a chunk that lives in a - // healthy store later in the list. - Err(ref e) => { - note_store_failure( - &mut dep_warnings, - dep_aliases, - store_idx, - "chunk lookup", - e, - ); + let norm = crate::cache::normalize_path_str(&chunk.path); + if !seen_paths.insert(norm) { continue; } + + // Extract the specific import line(s) that mention the + // module name, rather than returning the entire chunk content. + let import_statement = + if chunk.content.to_lowercase().contains(&term_lower) { + chunk + .content + .lines() + .find(|l| l.to_lowercase().contains(&term_lower)) + .unwrap_or("") + .to_string() + } else { + chunk.signature.filter(|s| !s.is_empty()).unwrap_or( + chunk.content.lines().next().unwrap_or("").to_string(), + ) + }; + + out.push(DependentItem { + path: ctx.prefix_sourced_path(&f.alias, &chunk.path), + line: chunk.start_line + 1, + import_statement, + }); + } + Ok(None) => {} // not held anywhere — skip + Err(ref e) => { + note_store_failure( + &mut dep_warnings, + dep_aliases, + origin_idx, + "chunk lookup", + e, + ); } } if out.len() >= limit { @@ -472,7 +481,7 @@ impl CodesearchService { let mut out = Vec::new(); let term_lower = search_term.to_lowercase(); for f in &fts_results { - if let Some(chunk) = store.get_chunk(f.chunk_id)? { + if let Some(chunk) = store.get_chunk(f.result.chunk_id)? { if !is_import_kind(&chunk.kind) { continue; } @@ -500,7 +509,7 @@ impl CodesearchService { out.push(DependentItem { path: chunk.path, - line: chunk.start_line, + line: chunk.start_line + 1, import_statement, }); @@ -524,9 +533,12 @@ impl CodesearchService { } }; - // Prefix paths with alias for multi-repo identification - for item in &mut items { - item.path = ctx.prefix_result_path(&item.path); + // Prefix paths with the routing alias; group items were already + // attributed to their origin repo at resolution time. + if ctx.stores_vec.is_none() { + for item in &mut items { + item.path = ctx.prefix_result_path(&item.path); + } } items.sort_by(|a, b| a.path.cmp(&b.path)); @@ -563,29 +575,26 @@ impl CodesearchService { let mut similar_warnings: Vec = Vec::new(); let mut results = if let Some(ref sv) = ctx.stores_vec { - // Multi-store: find the embedding in whichever store has it, - // then search across all stores for similar chunks. + // Multi-store: chunk ids are per-repo counters, so the source + // embedding cannot be resolved by "first store that answers this + // id" — that silently picks the WRONG repo's chunk on collision. + // Same contract as get_chunk: exactly one holder auto-routes; + // several holders are an ambiguity the caller must resolve with + // `project=`. let aliases = ctx.aliases(); - let mut embedding: Option> = None; + let mut candidates: Vec<(usize, Vec)> = Vec::new(); for (i, store_arc) in sv.iter().enumerate() { - let store = match bounded_vector_read(&store_arc.vector_store).await { - Ok(store) => store, - Err(e) => { - note_store_failure( - &mut similar_warnings, - aliases, - i, - "embedding lookup", - &e, - ); - continue; - } + let Some(store) = try_vector_read_or_note( + &store_arc.vector_store, + aliases, + i, + &mut similar_warnings, + "embedding lookup", + ) else { + continue; }; match store.get_embedding(request.chunk_id) { - Ok(Some(emb)) => { - embedding = Some(emb); - break; - } + Ok(Some(emb)) => candidates.push((i, emb)), Ok(None) => continue, Err(ref e) => { note_store_failure( @@ -600,9 +609,8 @@ impl CodesearchService { } } - let embedding = match embedding { - Some(e) => e, - None => { + let embedding = match candidates.len() { + 0 => { return Ok(CallToolResult::success(vec![ContentBlock::text( qualify_empty_result( format!( @@ -613,29 +621,56 @@ impl CodesearchService { ), )])); } + 1 => candidates.pop().expect("len == 1 checked").1, + _ => { + let holder_names: Vec<&str> = candidates + .iter() + .map(|(i, _)| { + aliases + .get(*i) + .map(String::as_str) + .unwrap_or("unnamed store") + }) + .collect(); + return Ok(CallToolResult::success(vec![ContentBlock::text(format!( + "Ambiguous chunk_id {}: found in {} repositories ({}). \ + Pass `project=` to pick one.", + request.chunk_id, + holder_names.len(), + holder_names.join(", ") + ))])); + } }; - // Search across all stores with the found embedding + // Search across all stores with the found embedding. Dedup keys on + // (store, id): a bare id collides across repos and would drop a + // whole repo's neighbours whenever another repo reused the id. let mut all_results: Vec = Vec::new(); - let mut seen_ids: std::collections::HashSet = std::collections::HashSet::new(); + let mut seen_ids: HashSet<(usize, u32)> = HashSet::new(); for (store_idx, store_arc) in sv.iter().enumerate() { - let store = match bounded_vector_read(&store_arc.vector_store).await { - Ok(store) => store, - Err(e) => { - note_store_failure(&mut similar_warnings, aliases, store_idx, "search", &e); - continue; - } + let alias = aliases + .get(store_idx) + .map(String::as_str) + .unwrap_or_default(); + let Some(store) = try_vector_read_or_note( + &store_arc.vector_store, + aliases, + store_idx, + &mut similar_warnings, + "search", + ) else { + continue; }; match store.search(&embedding, limit + 1) { Ok(mut neighbors) => { neighbors.retain(|r| r.id != request.chunk_id); for r in neighbors { - if seen_ids.insert(r.id) { + if seen_ids.insert((store_idx, r.id)) { all_results.push(SearchResultItem { chunk_id: Some(r.id), - path: r.path, - start_line: r.start_line, - end_line: r.end_line, + path: ctx.prefix_sourced_path(alias, &r.path), + start_line: r.start_line + 1, + end_line: r.end_line + 1, kind: r.kind, score: r.score, signature: r.signature, @@ -691,8 +726,8 @@ impl CodesearchService { .map(|r| SearchResultItem { chunk_id: Some(r.id), path: r.path, - start_line: r.start_line, - end_line: r.end_line, + start_line: r.start_line + 1, + end_line: r.end_line + 1, kind: r.kind, score: r.score, signature: r.signature, @@ -718,9 +753,12 @@ impl CodesearchService { } }; - // Prefix paths with alias for multi-repo identification - for item in &mut results { - item.path = ctx.prefix_result_path(&item.path); + // Prefix paths with the routing alias; group items were already + // attributed to their origin repo at resolution time. + if ctx.stores_vec.is_none() { + for item in &mut results { + item.path = ctx.prefix_result_path(&item.path); + } } // Every exit carries the channel: the earlier read sat in an diff --git a/src/mcp/helpers.rs b/src/mcp/helpers.rs index 77b5efbf..d3e82b99 100644 --- a/src/mcp/helpers.rs +++ b/src/mcp/helpers.rs @@ -2,6 +2,30 @@ use std::path::Path; // === Multi-store fan-out traits === +/// A fan-out result tagged with the repo whose store produced it. +/// +/// Chunk ids are store-local counters, so a bare id identifies a chunk only +/// within one repo's store — across a group fan-out, two different chunks +/// share an id as soon as both repos have that many chunks. Every downstream +/// resolution (`get_chunk`, path prefixing, content attribution) must therefore +/// use `alias` to address the ORIGIN store, never "whichever store answers +/// first". +pub(crate) struct SourcedResult { + /// Alias of the repo whose store produced this result. + pub(crate) alias: String, + /// The result itself. + pub(crate) result: R, +} + +impl SourcedResult { + pub(crate) fn new(alias: impl Into, result: R) -> Self { + Self { + alias: alias.into(), + result, + } + } +} + /// Trait for types that have a chunk ID (used for deduplication in group fan-out). pub(crate) trait HasChunkId { fn chunk_id(&self) -> u32; diff --git a/src/mcp/literal_search.rs b/src/mcp/literal_search.rs index f2903d8f..965204b4 100644 --- a/src/mcp/literal_search.rs +++ b/src/mcp/literal_search.rs @@ -76,6 +76,13 @@ impl CodesearchService { && (!regex_has_anchorable_token(&effective_query) || regex_has_disjunctive_or(&effective_query)); + // Relaxed-fallback state, visible to both the scan and BM25 branches + // (the scan path never sets it). Terms are taken from the effective + // query; when regex auto-promotion rewrites it the fallback trigger + // is disabled anyway, so the exactness of bm25_query does not matter. + let significant_terms = significant_query_terms(&effective_query); + let mut relaxed_fallback = false; + let mut items: Vec = if tokenless_regex { // ── Scan path ────────────────────────────────────────────── // Tokenless regex (e.g. \bfn\s+\w+) — BM25 cannot produce useful @@ -83,12 +90,24 @@ impl CodesearchService { // Score is 0.0 for all results (no BM25 ranking applies). tracing::debug!("literal_search: tokenless regex detected, using scan path"); if let Some(ref sv) = ctx.stores_vec { - // Multi-store scan + // Multi-store scan. Chunks come from their own store (no id + // lookups), so attribution is correct by construction — only + // the path prefix needs the owning alias. + let scan_aliases = ctx.aliases(); let mut items: Vec = Vec::new(); - for store_arc in sv { - let store = match bounded_vector_read(&store_arc.vector_store).await { - Ok(store) => store, - Err(_) => continue, + for (store_idx, store_arc) in sv.iter().enumerate() { + let alias = scan_aliases + .get(store_idx) + .map(String::as_str) + .unwrap_or_default(); + let Some(store) = try_vector_read_or_note( + &store_arc.vector_store, + scan_aliases, + store_idx, + &mut literal_warnings, + "chunk scan", + ) else { + continue; }; let all_chunks = match store.iter_all_chunks() { Ok(chunks) => chunks, @@ -116,9 +135,9 @@ impl CodesearchService { &effective_query, snippet_regex.as_ref(), ) { - let match_line = chunk.start_line + match_offset; + let match_line = chunk.start_line + match_offset + 1; items.push(LiteralSearchResultItem { - path: chunk.path, + path: ctx.prefix_sourced_path(alias, &chunk.path), start_line: match_line, end_line: match_line, snippet, @@ -170,7 +189,7 @@ impl CodesearchService { &effective_query, snippet_regex.as_ref(), ) { - let match_line = chunk.start_line + match_offset; + let match_line = chunk.start_line + match_offset + 1; items.push(LiteralSearchResultItem { path: chunk.path, start_line: match_line, @@ -221,7 +240,7 @@ impl CodesearchService { } else { effective_query.clone() }; - let fts_results = if let Some(ref sv) = ctx.stores_vec { + let mut fts_results = if let Some(ref sv) = ctx.stores_vec { let sa = ctx.store_aliases.as_ref().unwrap(); let outcome = self .with_fts_store_read_multi( @@ -257,7 +276,17 @@ impl CodesearchService { ) .await { - Ok(r) => r, + // Single store — tagged for uniformity with the + // resolution block below. + Ok(r) => { + let alias = ctx + .project_alias + .clone() + .unwrap_or_else(|| "local".to_string()); + r.into_iter() + .map(|hit| SourcedResult::new(alias.clone(), hit)) + .collect() + } Err(e) => { return Ok(CallToolResult::success(vec![ContentBlock::text(format!( "Error searching: {e:#}" @@ -266,77 +295,150 @@ impl CodesearchService { } }; + // Relaxed fallback. The exact AND pass returns silence for + // multi-word business queries whose relevant chunks hold only + // MOST of the terms. When it matches nothing, retry as a + // disjunction and keep candidates covering >=60% of the query's + // significant (non-stopword) terms, at least two of them. + // Single-term queries never relax: an empty exact answer stays + // an honest refusal instead of a wall of partial matches. + if !regex_enabled + && !request.phrase.unwrap_or(false) + && fts_results.is_empty() + && significant_terms.len() >= 2 + { + tracing::debug!( + "literal_search: exact AND empty for {} significant terms, retrying relaxed OR", + significant_terms.len() + ); + fts_results = if let Some(ref sv) = ctx.stores_vec { + let sa = ctx.store_aliases.as_ref().unwrap(); + let outcome = self + .with_fts_store_read_multi( + |fts_store| fts_store.search_relaxed(&bm25_query, limit * 3), + sv.clone(), + sa, + ) + .await + .unwrap_or_default(); + for (alias, err) in &outcome.failures { + let msg = format!("repo '{alias}' relaxed search failed: {err}"); + tracing::error!("MCP: {}", msg); + literal_warnings.push(msg); + } + outcome.results + } else { + match self + .with_fts_store_read_for( + |fts_store| fts_store.search_relaxed(&bm25_query, limit * 3), + ctx.stores.clone(), + ) + .await + { + Ok(r) => { + let alias = ctx + .project_alias + .clone() + .unwrap_or_else(|| "local".to_string()); + r.into_iter() + .map(|hit| SourcedResult::new(alias.clone(), hit)) + .collect() + } + Err(e) => { + return Ok(CallToolResult::success(vec![ContentBlock::text(format!( + "Error searching: {e:#}" + ))])); + } + } + }; + relaxed_fallback = true; + } + // Resolve chunk metadata and apply post-filters if let Some(ref sv) = ctx.stores_vec { // Multi-store: resolve chunks from all stores let mut items: Vec = Vec::new(); - 'outer: for fts_result in &fts_results { + 'outer: for fts_hit in &fts_results { + // Group fan-out: resolve ONLY in the origin store — a + // bare chunk id collides across repos, and probing every + // store answered from whoever was first, stealing another + // repo's content and path. let sa = ctx.store_aliases.as_ref().unwrap(); - for (idx, store_arc) in sv.iter().enumerate() { - let store = match bounded_vector_read(&store_arc.vector_store).await { - Ok(store) => store, - Err(e) => { - note_store_failure( - &mut literal_warnings, - sa, - idx, - "chunk lookup", - &e, - ); - continue; - } - }; - let looked_up = store.get_chunk(fts_result.chunk_id); - if let Err(ref e) = looked_up { - note_store_failure(&mut literal_warnings, sa, idx, "chunk lookup", e); + let Some(origin_idx) = sa.iter().position(|a| *a == fts_hit.alias) else { + continue 'outer; + }; + let store_arc = &sv[origin_idx]; + let Some(store) = try_vector_read_or_note( + &store_arc.vector_store, + sa, + origin_idx, + &mut literal_warnings, + "chunk lookup", + ) else { + continue 'outer; + }; + let looked_up = store.get_chunk(fts_hit.result.chunk_id); + if let Err(ref e) = looked_up { + note_store_failure( + &mut literal_warnings, + sa, + origin_idx, + "chunk lookup", + e, + ); + } + if let Some(chunk) = looked_up.ok().flatten() { + if relaxed_fallback + && !chunk_covers_significant_terms( + relaxed_gate_text(&chunk), + &significant_terms, + ) + { + continue 'outer; } - if let Some(chunk) = looked_up.ok().flatten() { - if let Some(ref lang) = lang_filter { - let file_lang = - Language::from_path(std::path::Path::new(&chunk.path)); - if file_lang.name() != lang { - continue; - } - } - if let Some(ref glob) = glob_filter { - let relative_path = chunk - .path - .strip_prefix(&project_root_normalized) - .unwrap_or(&chunk.path) - .trim_start_matches('/'); - if !simple_glob_match(glob, relative_path) { - continue; - } - } - let match_info = match_line_for_literal( - &chunk.content, - &effective_query, - snippet_regex.as_ref(), - ); - if regex_enabled && match_info.is_none() { - continue; + if let Some(ref lang) = lang_filter { + let file_lang = Language::from_path(std::path::Path::new(&chunk.path)); + if file_lang.name() != lang { + continue 'outer; } - let (match_offset, snippet) = match_info.unwrap_or_else(|| { - (0, chunk.content.lines().next().unwrap_or("").to_string()) - }); - let match_line = chunk.start_line + match_offset; - items.push(LiteralSearchResultItem { - path: chunk.path, - start_line: match_line, - end_line: match_line, - snippet, - score: fts_result.score, - kind: if chunk.kind.is_empty() { - None - } else { - Some(chunk.kind) - }, - signature: chunk.signature.filter(|s| !s.is_empty()), - }); - if items.len() >= limit { - break 'outer; + } + if let Some(ref glob) = glob_filter { + let relative_path = chunk + .path + .strip_prefix(&project_root_normalized) + .unwrap_or(&chunk.path) + .trim_start_matches('/'); + if !simple_glob_match(glob, relative_path) { + continue 'outer; } - break; // Found in this store + } + let match_info = match_line_for_literal( + &chunk.content, + &effective_query, + snippet_regex.as_ref(), + ); + if regex_enabled && match_info.is_none() { + continue 'outer; + } + let (match_offset, snippet) = match_info.unwrap_or_else(|| { + (0, chunk.content.lines().next().unwrap_or("").to_string()) + }); + let match_line = chunk.start_line + match_offset + 1; + items.push(LiteralSearchResultItem { + path: ctx.prefix_sourced_path(&fts_hit.alias, &chunk.path), + start_line: match_line, + end_line: match_line, + snippet, + score: fts_hit.result.score, + kind: if chunk.kind.is_empty() { + None + } else { + Some(chunk.kind) + }, + signature: chunk.signature.filter(|s| !s.is_empty()), + }); + if items.len() >= limit { + break 'outer; } } } @@ -352,9 +454,9 @@ impl CodesearchService { // miss ("chunk not in this store"). let resolved: anyhow::Result> = fts_results .iter() - .map(|fts_result| { - let chunk = store.get_chunk(fts_result.chunk_id)?; - Ok((chunk, fts_result.score)) + .map(|fts_hit| { + let chunk = store.get_chunk(fts_hit.result.chunk_id)?; + Ok((chunk, fts_hit.result.score)) }) .collect(); let items: Vec = resolved? @@ -364,6 +466,14 @@ impl CodesearchService { Some((chunk, score)) }) .filter(|(chunk, _)| { + if relaxed_fallback + && !chunk_covers_significant_terms( + relaxed_gate_text(chunk), + &significant_terms, + ) + { + return false; + } if let Some(ref lang) = lang_filter { let file_lang = Language::from_path(std::path::Path::new(&chunk.path)); @@ -396,7 +506,7 @@ impl CodesearchService { let (match_offset, snippet) = match_info.unwrap_or_else(|| { (0, chunk.content.lines().next().unwrap_or("").to_string()) }); - let match_line = chunk.start_line + match_offset; + let match_line = chunk.start_line + match_offset + 1; Some(LiteralSearchResultItem { path: chunk.path, start_line: match_line, @@ -428,9 +538,13 @@ impl CodesearchService { } }; - // Prefix paths with alias for multi-repo identification - for item in &mut items { - item.path = ctx.prefix_result_path(&item.path); + // Prefix paths with the routing alias. Group items were already + // attributed to their origin repo at resolution time; re-prefixing + // here would double the alias onto those paths. + if ctx.stores_vec.is_none() { + for item in &mut items { + item.path = ctx.prefix_result_path(&item.path); + } } // Compute low-confidence signal @@ -445,6 +559,11 @@ impl CodesearchService { The query contained code-like punctuation that BM25 would tokenize incorrectly.", request.query, effective_query )) + } else if relaxed_fallback { + Some(format!( + "Exact AND search matched nothing; showing relaxed OR results covering >=60% of the significant terms ({}) and at least two of them.", + significant_terms.join(", ") + )) } else if low_confidence == Some(true) { suggested_tool.as_ref().map(|tool| { format!( @@ -459,6 +578,7 @@ impl CodesearchService { let response = LiteralSearchResponse { results: items, auto_promoted_to_regex: if auto_promoted { Some(true) } else { None }, + relaxed_fallback: if relaxed_fallback { Some(true) } else { None }, note, low_confidence, suggested_tool: if low_confidence == Some(true) { @@ -493,6 +613,12 @@ impl CodesearchService { .to_string(), ); } + if response.relaxed_fallback == Some(true) { + lines.push( + "# exact AND matched nothing — relaxed OR fallback (>=60% term coverage)" + .to_string(), + ); + } if response.low_confidence == Some(true) { if let Some(ref hint) = response.suggested_tool { lines.push(format!("# low confidence — consider: {}", hint)); @@ -512,3 +638,132 @@ impl CodesearchService { Ok(CallToolResult::success(vec![ContentBlock::text(output)])) } } + +/// Function words that carry no retrieval signal. Dropped before relaxed +/// coverage accounting so natural-language questions are judged by their +/// domain terms — otherwise "на кого ставится задача по ..." needs a chunk +/// containing "на" and "по" to reach the 60% bar. +const LITERAL_STOPWORDS: &[&str] = &[ + "и", + "в", + "во", + "с", + "со", + "к", + "у", + "о", + "об", + "от", + "до", + "из", + "за", + "на", + "по", + "для", + "как", + "что", + "кто", + "кого", + "кому", + "чем", + "чему", + "где", + "когда", + "или", + "же", + "бы", + "ли", + "а", + "но", + "да", + "это", + "этот", + "эта", + "эти", + "там", + "так", + "такой", + "тоже", + "уже", + "еще", + "ещё", + "был", + "была", + "быть", + "есть", + "the", + "a", + "an", + "of", + "to", + "in", + "on", + "for", + "and", + "or", + "is", + "are", + "was", + "were", + "be", + "been", + "with", + "from", + "by", + "at", + "as", + "that", + "this", + "it", + "its", + "there", +]; + +/// Split a query into deduplicated lowercase terms the same way the FTS +/// analyzer would (SimpleTokenizer splits on non-alphanumeric boundaries), +/// dropping stopwords. This is the matched set and denominator for relaxed +/// coverage. +pub(crate) fn significant_query_terms(query: &str) -> Vec { + let mut seen = std::collections::HashSet::new(); + let mut terms = Vec::new(); + for raw in query.split(|c: char| !c.is_alphanumeric()) { + let term = raw.to_lowercase(); + if term.is_empty() || LITERAL_STOPWORDS.contains(&term.as_str()) { + continue; + } + if seen.insert(term.clone()) { + terms.push(term); + } + } + terms +} + +/// Relaxed-fallback gate: a candidate counts only when it holds at least two +/// distinct significant terms and covers >=60% of them. +pub(crate) fn chunk_covers_significant_terms(content: &str, terms: &[String]) -> bool { + if terms.len() < 2 { + return false; + } + let tokens: std::collections::HashSet = content + .split(|c: char| !c.is_alphanumeric()) + .map(str::to_lowercase) + .collect(); + let matched = terms.iter().filter(|t| tokens.contains(*t)).count(); + matched >= 2 && matched * 10 >= terms.len() * 6 +} + +/// Text the relaxed-fallback gate matches against. The relaxed BM25 pass +/// queries content, signature and kind fields (signature boosted 2.0), so a +/// candidate is often rescued BY its signature — gating only on `content` +/// then dropped exactly those candidates, defeating the fallback for the +/// identifier queries it exists for. The gate therefore runs over the stored +/// `searchable_text` (signature + docstring + content) when this index +/// generation wrote one; legacy blobs predate the field (`#[serde(default)]` +/// gives them an empty string) and fall back to bare content. +pub(crate) fn relaxed_gate_text(chunk: &crate::vectordb::ChunkMetadata) -> &str { + if chunk.searchable_text.is_empty() { + &chunk.content + } else { + &chunk.searchable_text + } +} diff --git a/src/mcp/mod.rs b/src/mcp/mod.rs index 530e1b1e..59290fb6 100644 --- a/src/mcp/mod.rs +++ b/src/mcp/mod.rs @@ -74,6 +74,84 @@ pub(crate) async fn bounded_fts_read( }) } +/// Try-lock a vector store for an INTERACTIVE multi-repo fan-out, skipping +/// and reporting a busy repo instead of waiting for it. +/// +/// Every interactive multi-store handler (find, graph, explore, get_chunk, +/// the literal metadata lookups) runs on an agent's request path: waits sum +/// per busy store — N repos held by an indexing run meant N×300 s — the +/// exact interactive wedge the search fan-outs already killed with try_read. +/// Single-store paths (`with_*_store_read_for`, project-scoped queries) keep +/// the bounded wait by design: one repo, no summation. +/// +/// Returns `None` after appending the same "store busy" warning the search +/// fan-outs emit, so every skipped repo is named in the response. +pub(crate) fn try_vector_read_or_note<'a>( + lock: &'a tokio::sync::RwLock, + aliases: &[String], + idx: usize, + warnings: &mut Vec, + what: &str, +) -> Option> { + match lock.try_read() { + Ok(store) => Some(store), + Err(_) => { + note_store_failure( + warnings, + aliases, + idx, + what, + &anyhow::anyhow!( + "store busy: active indexing holds the write lock — \ + repo skipped in this fan-out, retry shortly" + ), + ); + None + } + } +} + +/// Warning for a group whose answering repos embed with more than one model. +/// +/// Cosine distance is only meaningful inside a single model's vector space, +/// yet the group merge ranks hits from different models side by side as if +/// one scale sorted them. Extracted so the contract — fire only when the +/// models are genuinely mixed, name every distinct model, explain that each +/// score is within-repo relevance only — is testable without standing up +/// two on-disk embedding models. +pub(crate) fn mixed_model_group_warning>( + models: I, +) -> Option { + let distinct: std::collections::HashSet = models.into_iter().collect(); + if distinct.len() < 2 { + return None; + } + let mut names: Vec<&str> = distinct.iter().map(|m| m.short_name()).collect(); + names.sort_unstable(); + Some(format!( + "group mixes {} embedding models ({}) — scores are NOT comparable \ + across repos; read each hit's score as within-repo relevance only", + names.len(), + names.join(", ") + )) +} + +/// Intern a (repo, chunk_id) pair to a unique synthetic id for the group +/// RRF fusion call. +/// +/// The pure fusion functions key on a single bare u32, but chunk ids are +/// per-repo counters and collide across a group — two repos' chunk id 0 +/// fused into ONE entry. Every (alias, id) pair maps to its own synthetic +/// id; the fused ids map straight back through the same table. Extracted +/// so the collision contract is unit-testable without standing up stores. +pub(crate) fn intern_group_chunk_id( + map: &mut std::collections::HashMap<(String, u32), u32>, + key: (String, u32), +) -> u32 { + let next = map.len() as u32; + *map.entry(key).or_insert(next) +} + use crate::db_discovery::{find_best_database, load_repos_config}; use crate::embed::{EmbeddingServicePool, ModelType}; use crate::file::Language; @@ -282,6 +360,32 @@ fn merge_exact_into_fts( } } +/// Group-fan-out sibling of [`merge_exact_into_fts`]: merge exact-identifier +/// hits into the lexical result set, deduplicating by `(alias, chunk_id)` — +/// a bare chunk_id is store-local and collides across repos, so keying on it +/// alone would fold one repo's chunk into another's entry. +fn merge_exact_into_fts_multi( + fts_results: &mut Vec>, + exact: Vec>, +) { + let mut positions: std::collections::HashMap<(String, u32), usize> = fts_results + .iter() + .enumerate() + .map(|(idx, r)| ((r.alias.clone(), r.result.chunk_id), idx)) + .collect(); + + for r in exact { + let key = (r.alias.clone(), r.result.chunk_id); + if let Some(&existing_idx) = positions.get(&key) { + let existing = &mut fts_results[existing_idx]; + existing.result.score = existing.result.score.max(r.result.score); + } else { + positions.insert(key, fts_results.len()); + fts_results.push(r); + } + } +} + /// Compute low-confidence signaling based on the top result's score. /// /// Returns `(low_confidence, suggested_tool)` where both are `None` when @@ -871,7 +975,9 @@ fn parse_import_lines(content: &str, start_line: usize) -> Vec { if let Some((kind, imported)) = parsed { items.push(ImportItem { imported, - line: start_line + offset, + // `start_line` is the chunk's 0-based first line and `offset` + // is 0-based within it; the response emits 1-based lines. + line: start_line + offset + 1, kind, }); } @@ -1049,16 +1155,34 @@ impl CodesearchService { } } - /// Get (lazily initializing) the embedding service for `model`. + /// Resolve the embedding service for a SEARCH query — or `None` when the + /// model cannot be used without a network fetch. /// - /// The returned `Arc>` is per-model, so concurrent queries against - /// different models do not serialise on one global lock. Callers MUST pass - /// the model the target index was built with — see [`Self::query_model`]. - pub(crate) fn embedding_service_for( + /// Search is interactive: a cold-cache model resolves through hf-hub, and + /// on networks that black-hole the model host that fetch never completes + /// and never errors — the whole-night serve stall. So search accepts only + /// a model that is already loaded or whose files are provably on disk + /// ([`crate::embed::is_model_in_cache`], a pure fs probe). Downloading + /// stays on the indexing path (`codesearch index`, `codesearch setup`), + /// where it belongs to a background task with progress, not to a query. + pub(crate) async fn embedding_service_for_query( &self, model: ModelType, - ) -> Result>> { - self.embedding_pool.get(model) + ) -> Result>>> { + if let Some(service) = self.embedding_pool.get_if_cached(model) { + return Ok(Some(service)); + } + if !crate::embed::is_model_in_cache(model) { + return Ok(None); + } + // Files are on disk, so this load cannot become a network fetch. It + // is still ONNX init work (hundreds of MB mapped) — keep it off the + // async worker; the pool bounds it by MODEL_LOAD_TIMEOUT_SECS. + let pool = self.embedding_pool.clone(); + match tokio::task::spawn_blocking(move || pool.get(model)).await { + Ok(result) => result.map(Some), + Err(e) => Err(anyhow::anyhow!("embedding model load task failed: {e}")), + } } /// Return the current MCP mode as a string for diagnostics. @@ -1393,16 +1517,28 @@ impl CodesearchService { R: Clone + HasChunkId + HasScore, { let mut failures: Vec<(String, String)> = Vec::new(); - let mut all_results: Vec = Vec::new(); + let mut all_results: Vec> = Vec::new(); let mut seen_ids: std::collections::HashMap<(String, u32), usize> = std::collections::HashMap::new(); for (idx, store_arc) in stores.iter().enumerate() { let alias = aliases.get(idx).map(|s| s.as_str()).unwrap_or("unknown"); - let store = match bounded_vector_read(&store_arc.vector_store).await { + // Try-lock, never wait: a multi-repo fan-out is an interactive + // query, and waiting for one busy repo's write lock would stall + // every other repo's result behind it (waits sum up per busy + // store, up to the full bounded wait each). A store held by a + // running indexing batch is skipped and reported instead; the + // same request against that single repo alone still waits, via + // the bounded single-store helpers. + let store = match store_arc.vector_store.try_read() { Ok(store) => store, - Err(e) => { - failures.push((alias.to_string(), format!("{e:#}"))); + Err(_) => { + failures.push(( + alias.to_string(), + "store busy: active indexing holds the write lock — \ + repo skipped in this fan-out, retry shortly" + .to_string(), + )); continue; } }; @@ -1412,12 +1548,15 @@ impl CodesearchService { let key = (alias.to_string(), r.chunk_id()); if let Some(&existing_idx) = seen_ids.get(&key) { // Keep the one with higher score - if r.score() > all_results[existing_idx].score() { - all_results[existing_idx] = r; + if r.score() > all_results[existing_idx].result.score() { + all_results[existing_idx].result = r; } } else { seen_ids.insert(key, all_results.len()); - all_results.push(r); + // The tag is load-bearing: chunk ids repeat across + // repos, so a bare id cannot address the chunk that + // produced this hit at resolution time. + all_results.push(SourcedResult::new(alias.to_string(), r)); } } } @@ -1438,8 +1577,9 @@ impl CodesearchService { // Sort by score descending all_results.sort_by(|a, b| { - b.score() - .partial_cmp(&a.score()) + b.result + .score() + .partial_cmp(&a.result.score()) .unwrap_or(std::cmp::Ordering::Equal) }); @@ -1469,16 +1609,24 @@ impl CodesearchService { R: Clone + HasChunkId + HasScore, { let mut failures: Vec<(String, String)> = Vec::new(); - let mut all_results: Vec = Vec::new(); + let mut all_results: Vec> = Vec::new(); let mut seen_ids: std::collections::HashMap<(String, u32), usize> = std::collections::HashMap::new(); for (idx, store_arc) in stores.iter().enumerate() { let alias = aliases.get(idx).map(|s| s.as_str()).unwrap_or("unknown"); - let fts = match bounded_fts_read(&store_arc.fts_store).await { + // Try-lock, never wait — same rationale as the vector fan-out: + // a busy repo is skipped and reported, not allowed to stall the + // whole group's literal results. + let fts = match store_arc.fts_store.try_read() { Ok(fts) => fts, - Err(e) => { - failures.push((alias.to_string(), format!("{e:#}"))); + Err(_) => { + failures.push(( + alias.to_string(), + "store busy: active indexing holds the write lock — \ + repo skipped in this fan-out, retry shortly" + .to_string(), + )); continue; } }; @@ -1487,12 +1635,14 @@ impl CodesearchService { for r in results { let key = (alias.to_string(), r.chunk_id()); if let Some(&existing_idx) = seen_ids.get(&key) { - if r.score() > all_results[existing_idx].score() { - all_results[existing_idx] = r; + if r.score() > all_results[existing_idx].result.score() { + all_results[existing_idx].result = r; } } else { seen_ids.insert(key, all_results.len()); - all_results.push(r); + // Same contract as the vector fan-out: the tag, not + // the bare id, is what identifies the chunk later. + all_results.push(SourcedResult::new(alias.to_string(), r)); } } } @@ -1511,8 +1661,9 @@ impl CodesearchService { // Sort by score descending all_results.sort_by(|a, b| { - b.score() - .partial_cmp(&a.score()) + b.result + .score() + .partial_cmp(&a.result.score()) .unwrap_or(std::cmp::Ordering::Equal) }); @@ -1566,6 +1717,7 @@ impl CodesearchService { "mode": mode, "compact": request.compact, "semantic_mode": request.semantic_mode, + "min_score": request.min_score, "regex": request.regex, "phrase": request.phrase, "file_glob": request.file_glob, @@ -1605,6 +1757,10 @@ impl CodesearchService { // repos. Skip entirely when the group has no local repos. let (locals, _) = cfg.split_group_targets(&group); let mut local_items: Vec = Vec::new(); + // Refusal notes from legs that answered empty by their own threshold + // (min_score): merged into the response warnings, so an honest refusal + // never reads as "no hits there". + let mut leg_notes: Vec = Vec::new(); if !locals.is_empty() { let local_result = match mode.as_str() { "semantic" => { @@ -1616,6 +1772,7 @@ impl CodesearchService { mode: request.semantic_mode.clone(), project: None, group: Some(group.clone()), + min_score: request.min_score, }; self.semantic_search(Parameters(req)).await? } @@ -1641,6 +1798,9 @@ impl CodesearchService { } }; local_items = parse_search_items_from_call_result(&local_result, &mode); + if let Some(note) = parse_note_from_call_result(&local_result) { + leg_notes.push(format!("local group: {note}")); + } retain_by_filter_path(&mut local_items, request.filter_path.as_deref()); } @@ -1687,13 +1847,19 @@ impl CodesearchService { let mut all_lists: Vec> = vec![local_items]; while let Some(res) = join.join_next().await { match res { - Ok((peer_name, remote_alias, Outcome::Ok(items))) => { - let mut converted: Vec = items + Ok((peer_name, remote_alias, Outcome::Ok(reply))) => { + let mut converted: Vec = reply + .items .into_iter() .map(|it| convert_remote_item(&peer_name, &remote_alias, it)) .collect(); retain_by_filter_path(&mut converted, request.filter_path.as_deref()); all_lists.push(converted); + if let Some(note) = reply.note { + warnings.push(format!( + "remote project '{peer_name}/{remote_alias}': {note}" + )); + } } Ok((peer_name, remote_alias, Outcome::Unreachable(reason))) => { warnings.push(format!( @@ -1707,7 +1873,9 @@ impl CodesearchService { } } - // 4) RRF-interleave the disjoint ranked lists and render. + // 4) RRF-interleave the disjoint ranked lists and render. Leg refusal + // notes ride along as warnings, after the per-store failures. + warnings.extend(leg_notes); let merged = merge_ranked_lists(all_lists, DEFAULT_RRF_K, limit); Ok(self.build_federated_response(merged, warnings)) } @@ -1770,13 +1938,22 @@ impl CodesearchService { let outcome = client.search_project(&peer, body, &remote_alias).await; let (mut items, warnings) = match outcome { - Outcome::Ok(items) => ( - items - .into_iter() - .map(|it| convert_remote_item(&peer_name, &remote_alias, it)) - .collect::>(), - Vec::new(), - ), + Outcome::Ok(reply) => { + let mut peer_warnings = Vec::new(); + if let Some(note) = reply.note { + peer_warnings.push(format!( + "remote project '{peer_name}/{remote_alias}': {note}" + )); + } + ( + reply + .items + .into_iter() + .map(|it| convert_remote_item(&peer_name, &remote_alias, it)) + .collect::>(), + peer_warnings, + ) + } Outcome::Unreachable(reason) => ( Vec::new(), vec![format!( @@ -1872,6 +2049,7 @@ impl CodesearchService { low_confidence: if items.is_empty() { Some(true) } else { None }, results: items, suggested_tool: None, + note: None, warnings: if warnings.is_empty() { None } else { diff --git a/src/mcp/responses.rs b/src/mcp/responses.rs index dcb5f09b..6c0c24ba 100644 --- a/src/mcp/responses.rs +++ b/src/mcp/responses.rs @@ -1,4 +1,4 @@ -use super::helpers::prefix_path_with_alias; +use super::helpers::{prefix_path_with_alias, SourcedResult}; use crate::embed::ModelType; use crate::index::SharedStores; use crate::vectordb::VectorStore; @@ -155,8 +155,11 @@ pub(crate) const DEFINITION_KINDS: &[&str] = &[ /// investigation chasing an indexing problem that did not exist. #[must_use] pub(crate) struct MultiReadOutcome { - /// Merged, deduplicated, score-sorted results from the stores that worked. - pub(crate) results: Vec, + /// Merged, deduplicated, score-sorted results from the stores that worked, + /// each tagged with its origin repo. Chunk ids are store-local counters, so + /// the tag — not the id — is what downstream resolution must key on; see + /// [`crate::mcp::SourcedResult`]. + pub(crate) results: Vec>, /// `(alias, full error chain)` for every store that failed. Empty on a /// clean run. pub(crate) failures: Vec<(String, String)>, @@ -578,7 +581,11 @@ impl MultiReadOutcome { /// Deliberately the only ergonomic way to get at `results`: reaching for /// the field directly and dropping `failures` is `unwrap_or_default()` /// under a new name, and that is the bug this whole type exists to stop. - pub(crate) fn into_results(self, warnings: &mut Vec, what: &str) -> Vec { + pub(crate) fn into_results( + self, + warnings: &mut Vec, + what: &str, + ) -> Vec> { for (alias, err) in &self.failures { push_store_warning(warnings, &store_warning(alias, what, err)); } @@ -659,4 +666,20 @@ impl MultiStoreContext { } crate::cache::normalize_path_str(path) } + + /// Prefix a chunk path with its KNOWN source alias (group fan-out results). + /// + /// `prefix_result_path` infers the alias by matching the path against every + /// repo root — which never matches a project-relative stored path, so group + /// results silently shipped unprefixed. A fan-out result already carries + /// its origin alias, so no inference is needed here: strip the root when + /// the path is absolute, treat it as repo-relative otherwise. + pub(crate) fn prefix_sourced_path(&self, alias: &str, path: &str) -> String { + let root = self + .alias_roots + .get(alias) + .map(String::as_str) + .unwrap_or(""); + prefix_path_with_alias(path, Some(alias), root) + } } diff --git a/src/mcp/search.rs b/src/mcp/search.rs index d6610f89..06b4f99b 100644 --- a/src/mcp/search.rs +++ b/src/mcp/search.rs @@ -23,7 +23,7 @@ impl CodesearchService { idempotent_hint = true, open_world_hint = false ), - description = "Unified code search. Set `mode` to choose the backend:\n\n- `semantic` (default): vector embeddings + BM25 FTS + exact-identifier boosting, fused with RRF. Best for conceptual queries, identifier lookups, and mixed natural-language + symbol queries.\n- `literal`: pure FTS, no embeddings. Fast and works without an embedding model. Sub-mode selection:\n * Queries with operators, brackets, or punctuation (`foo = null`, `Vec`, `return x;`, `a::b`) -> set `regex=true` and write the query as a regex. BM25 tokenizes on punctuation otherwise, producing noisy results.\n * Multi-word exact phrases -> set `phrase=true`.\n * Plain identifier lookups (`CodesearchService`) -> leave both false.\n\nFor semantic mode, optionally set `semantic_mode`: \"auto\" (default) | \"semantic\" | \"lexical\" | \"hybrid\".\nReturns metadata only by default (`compact=true`). Use `get_chunk` to read full code. Prefer `search(mode=\"literal\", regex=true)` over external grep/ripgrep for code patterns.\n\nIMPORTANT (multi-repo): always specify either `project` (single repo) or `group` (cross-repo). Omitting both in multi-repo mode returns a `scope_required` error with the list of available projects and groups. If the user has not indicated which repository to search, ask them to choose." + description = "Unified code search. Set `mode` to choose the backend:\n\n- `semantic` (default): vector embeddings + BM25 FTS + exact-identifier boosting, fused with RRF. Best for conceptual queries, identifier lookups, and mixed natural-language + symbol queries.\n- `literal`: pure FTS, no embeddings. Fast and works without an embedding model. Sub-mode selection:\n * Queries with operators, brackets, or punctuation (`foo = null`, `Vec`, `return x;`, `a::b`) -> set `regex=true` and write the query as a regex. BM25 tokenizes on punctuation otherwise, producing noisy results.\n * Multi-word exact phrases -> set `phrase=true`.\n * Plain identifier lookups (`CodesearchService`) -> leave both false.\n\nFor semantic mode, optionally set `semantic_mode`: \"auto\" (default) | \"semantic\" | \"lexical\" | \"hybrid\".\nSet `min_score` to drop weak hits and answer with an explicit refusal (empty results + note) instead of nearest-neighbour noise; the score scale depends on `semantic_mode` — cosine similarity for \"semantic\", RRF points (rarely above 0.2) otherwise. Federated group/project queries forward `min_score` to every leg and surface each leg's refusal note in `warnings`. Not supported in `literal` mode — passing `min_score` with mode=\"literal\" returns an explicit error naming the field.\nReturns metadata only by default (`compact=true`). Use `get_chunk` to read full code. Prefer `search(mode=\"literal\", regex=true)` over external grep/ripgrep for code patterns.\n\nIMPORTANT (multi-repo): always specify either `project` (single repo) or `group` (cross-repo). Omitting both in multi-repo mode returns a `scope_required` error with the list of available projects and groups. If the user has not indicated which repository to search, ask them to choose." )] pub(crate) async fn search( &self, @@ -37,6 +37,27 @@ impl CodesearchService { request.group, ); + // `min_score` has no honest meaning in literal mode — BM25 scores are + // corpus-relative and unbounded, so a caller-supplied threshold cannot + // be calibrated the way semantic thresholds are. Refuse by name + // instead of silently dropping the field: the tool advertises it, and + // a silently ignored filter reads as "no weak hits existed". Checked + // before any routing so local, federated-group and mounted-project + // paths all behave identically. + if request.min_score.is_some() + && request + .mode + .as_deref() + .is_some_and(|m| m.eq_ignore_ascii_case("literal")) + { + return Ok(CallToolResult::success(vec![ContentBlock::text( + "min_score is not supported in literal mode: BM25 scores are corpus-relative \ + and unbounded, so a threshold would drop hits by an arbitrary rule. Re-run \ + without min_score, or use mode=\"semantic\" — there an emptied result set \ + comes back with an explicit refusal note naming the threshold.", + )])); + } + // Federation: when the query targets a group that resolves to one or more // remote peers, merge local + remote results (RRF-interleave) instead of // searching local repos only. Only `group` federates; `project` stays @@ -82,6 +103,7 @@ impl CodesearchService { mode: request.semantic_mode, project: request.project, group: request.group, + min_score: request.min_score, }; self.semantic_search(Parameters(semantic_req)).await } @@ -186,8 +208,17 @@ impl CodesearchService { let model_resolution = self.resolve_query_model(ctx.project_alias.as_deref()); let query_embedding = { let model = model_resolution.model; - let service = match self.embedding_service_for(model) { - Ok(s) => s, + let service = match self.embedding_service_for_query(model).await { + Ok(Some(s)) => s, + Ok(None) => { + return Ok(CallToolResult::success(vec![ContentBlock::text(format!( + "Embedding model '{}' is not available locally, and search never \ + downloads models. Populate the model cache (codesearch setup) or \ + re-index this repo, then retry. Literal search (mode=\"literal\") \ + works without embedding models.", + model.short_name() + ))])); + } Err(e) => { tracing::error!("MCP: Failed to get embedding service: {:?}", e); return Ok(CallToolResult::success(vec![ContentBlock::text(format!( @@ -196,19 +227,35 @@ impl CodesearchService { } }; - let mut service = service.lock().unwrap(); tracing::debug!( "MCP: Embedding query with model '{}'...", model.short_name() ); - match service.embed_query(&request.query) { - Ok(e) => e, - Err(e) => { + // ONNX inference is CPU-bound; running it inline pins an async + // worker for the whole pass. Poison is recovered rather than + // propagated — same contract as embed_chunks_yielding. + let query_text = request.query.clone(); + match tokio::task::spawn_blocking(move || { + let mut guard = service + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + guard.embed_query(&query_text) + }) + .await + { + Ok(Ok(e)) => e, + Ok(Err(e)) => { tracing::error!("MCP: Failed to embed query: {:?}", e); return Ok(CallToolResult::success(vec![ContentBlock::text(format!( "Error embedding query: {e:#}" ))])); } + Err(e) => { + tracing::error!("MCP: Embedding task failed: {:?}", e); + return Ok(CallToolResult::success(vec![ContentBlock::text(format!( + "Error embedding query: {e}" + ))])); + } } }; @@ -505,14 +552,6 @@ impl CodesearchService { let mode = request.mode.as_deref().unwrap_or("auto"); let structural_intent = detect_structural_intent(&request.query); - // Which repo (alias) owns each surviving chunk id, recorded at - // fan-out and chunk-resolution time. The flat fan-out merge drops the - // store of origin, so without this map a group=all response cannot - // attribute a hit to its repo — `project=` results are prefixed - // "alias/path", group results must be too. - let mut alias_by_chunk: std::collections::HashMap = - std::collections::HashMap::new(); - // === Lexical mode: FTS only across all stores === if mode == "lexical" { // Lexical has no second backend, so a failed store here is invisible @@ -550,12 +589,13 @@ impl CodesearchService { .await .unwrap_or_default(); lexical_warnings.extend(exact_outcome.warnings("exact-identifier search")); - merge_exact_into_fts(&mut all_fts, exact_outcome.results); + merge_exact_into_fts_multi(&mut all_fts, exact_outcome.results); } all_fts.sort_by(|a, b| { - b.score - .partial_cmp(&a.score) + b.result + .score + .partial_cmp(&a.result.score) .unwrap_or(std::cmp::Ordering::Equal) }); @@ -565,11 +605,10 @@ impl CodesearchService { limit, &stores, aliases, + alias_roots, &mut lexical_warnings, - &mut alias_by_chunk, ) .await; - prefix_paths_with_alias(&mut results, &alias_by_chunk, alias_roots); if let Some(target_kind) = structural_intent { boost_kind(&mut results, target_kind); @@ -608,9 +647,25 @@ impl CodesearchService { // here and folded into `search_warnings` below so an agent sees the // assumption alongside the results it applies to. let mut model_warnings: Vec = Vec::new(); + // Repos whose model is unavailable to search (not loaded and not on + // disk). Dropped from the VECTOR fan-out only — the FTS pass still + // covers them, since literal search needs no embeddings. + let mut no_model_aliases: Vec = Vec::new(); + // Distinct models that actually answered (embedding resolved): a + // group mixing them produces scores from different vector spaces, + // which the merge presents as one ranking. + let mut answering_models: std::collections::HashSet = + std::collections::HashSet::new(); { let mut by_model: std::collections::HashMap> = std::collections::HashMap::new(); + // One warning per distinct missing model, not per repo: a group + // often shares a single model, and N copies of the same warning is + // noise an agent has to read past. + let mut missing_by_model: std::collections::HashMap< + crate::embed::ModelType, + Vec, + > = std::collections::HashMap::new(); for alias in aliases { let model_resolution = self.resolve_query_model(Some(alias)); let model = model_resolution.model; @@ -619,36 +674,105 @@ impl CodesearchService { } let embedding = match by_model.get(&model) { Some(cached) => cached.clone(), - None => { - let service = match self.embedding_service_for(model) { - Ok(s) => s, - Err(e) => { - return Ok(CallToolResult::success(vec![ContentBlock::text( - format!( - "Error initializing embedding service for '{alias}': {e:#}" - ), - )])); - } - }; - let mut service = service.lock().unwrap(); - let embedding = match service.embed_query(&request.query) { - Ok(e) => e, - Err(e) => { - return Ok(CallToolResult::success(vec![ContentBlock::text( - format!("Error embedding query: {e:#}"), - )])); - } - }; - by_model.insert(model, embedding.clone()); - embedding - } + None => match self.embedding_service_for_query(model).await { + Ok(Some(service)) => { + // ONNX inference is CPU-bound; running it inline + // pins an async worker for the whole pass. Poison + // recovery — same contract as the single-project + // path. + let query_text = request.query.clone(); + let embedding = match tokio::task::spawn_blocking(move || { + let mut guard = service + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + guard.embed_query(&query_text) + }) + .await + { + Ok(Ok(e)) => e, + Ok(Err(e)) => { + return Ok(CallToolResult::success(vec![ContentBlock::text( + format!("Error embedding query: {e:#}"), + )])); + } + Err(e) => { + return Ok(CallToolResult::success(vec![ContentBlock::text( + format!("Embedding task failed: {e}"), + )])); + } + }; + by_model.insert(model, embedding.clone()); + embedding + } + Ok(None) => { + missing_by_model + .entry(model) + .or_default() + .push(alias.clone()); + continue; + } + Err(e) => { + return Ok(CallToolResult::success(vec![ContentBlock::text(format!( + "Error initializing embedding service for '{alias}': {e:#}" + ))])); + } + }, }; embeddings_by_alias.insert(alias.clone(), embedding); + answering_models.insert(model); + } + for (model, missing) in missing_by_model { + model_warnings.push(format!( + "semantic skipped for {} repo(s) using model '{}' not available \ + locally (search never downloads models; populate the cache via \ + codesearch setup or re-index): {}", + missing.len(), + model.short_name(), + missing.join(", ") + )); + no_model_aliases.extend(missing); } } + // A mixed-model group answers with scores from different vector + // spaces, which the merged ranking presents as one scale. Warn in + // every mode: RRF fusion (auto/hybrid) partially hides it by fusing + // ranks only; semantic_mode's raw cosine scores expose it directly. + if let Some(warning) = mixed_model_group_warning(answering_models) { + model_warnings.push(warning); + } + // Search vector stores across all repos, each with its own model's - // query embedding. + // query embedding. Results stay origin-tagged (see SourcedResult) — + // the tag replaces the old alias_by_chunk side-map, which keyed on + // bare chunk ids and could not survive the cross-repo collisions. + // + // Only repos that got a query embedding take part: a repo whose model + // is unavailable locally must not force a download — or, on a + // black-holed network, an indefinite wedge — for the whole group. + // The FTS pass below still covers skipped repos in hybrid/auto/ + // lexical, so they degrade, not disappear. + let skip_set: std::collections::HashSet<&str> = + no_model_aliases.iter().map(String::as_str).collect(); + let vector_stores: Vec> = stores + .iter() + .zip(aliases.iter()) + .filter(|(_, alias)| !skip_set.contains(alias.as_str())) + .map(|(store, _)| Arc::clone(store)) + .collect(); + let vector_aliases: Vec = aliases + .iter() + .filter(|alias| !skip_set.contains(alias.as_str())) + .cloned() + .collect(); + if mode == "semantic" && !no_model_aliases.is_empty() && vector_stores.is_empty() { + let detail = model_warnings.join("\n"); + return Ok(CallToolResult::success(vec![ContentBlock::text(format!( + "Semantic search could not run: every repo in this group was skipped \ + because its embedding model is missing locally.\n{detail}\nLiteral \ + search (mode=\"literal\") works without embedding models." + ))])); + } let outcome = self .with_vector_store_read_multi( |alias, store| { @@ -660,13 +784,10 @@ impl CodesearchService { let found = store .search(embedding, limit * 5) .context("Error searching vector store")?; - for r in &found { - alias_by_chunk.insert(r.id, alias.to_string()); - } Ok(found) }, - stores.clone(), - aliases, + vector_stores, + &vector_aliases, ) .await; @@ -682,7 +803,7 @@ impl CodesearchService { tracing::error!( "MCP: vector fan-out degraded — {} of {} repo(s) failed: {:?}", o.failures.len(), - stores.len(), + vector_aliases.len(), o.failures ); // Only "semantic" has no second backend to fall back on. In @@ -700,7 +821,7 @@ impl CodesearchService { "Error searching vector store: {} of {} repo(s) in scope failed \ and none returned results:\n{}", o.failures.len(), - stores.len(), + vector_aliases.len(), detail ))])); } @@ -720,19 +841,24 @@ impl CodesearchService { // === Mode: "semantic" — vector only === if mode == "semantic" { - let fused = vector_only(&vector_results); - let chunk_to_result: std::collections::HashMap = - vector_results.iter().map(|r| (r.id, r)).collect(); - + // Vector-only fan-out: no second backend to fuse with, so the RRF + // pass would be an identity function. Iterate the origin-tagged + // hits directly, prefixing each path from its own tag — the old + // `vector_only` + id-keyed map folded different repos' chunks + // onto one entry whenever their local ids collided. let mut results: Vec = Vec::new(); - for f in fused.into_iter().take(limit) { - if let Some(result) = chunk_to_result.get(&f.chunk_id) { - let mut r = (*result).clone(); - r.score = f.rrf_score; - results.push(r); - } + for hit in vector_results.iter().take(limit) { + let mut r = hit.result.clone(); + r.path = prefix_path_with_alias( + &r.path, + Some(&hit.alias), + alias_roots + .get(&hit.alias) + .map(String::as_str) + .unwrap_or(""), + ); + results.push(r); } - prefix_paths_with_alias(&mut results, &alias_by_chunk, alias_roots); return self.build_semantic_response( results, request, @@ -771,7 +897,7 @@ impl CodesearchService { // Exact identifier search across all stores let all_exact = if !identifiers.is_empty() { - let mut exact_results: Vec = Vec::new(); + let mut exact_results: Vec> = Vec::new(); for ident in identifiers { let exact_outcome = self .with_fts_store_read_multi( @@ -783,7 +909,11 @@ impl CodesearchService { .unwrap_or_default(); search_warnings.extend(exact_outcome.warnings("exact-identifier search")); for r in exact_outcome.results { - if !exact_results.iter().any(|e| e.chunk_id == r.chunk_id) { + let key = (r.alias.clone(), r.result.chunk_id); + if !exact_results + .iter() + .any(|e| (e.alias.clone(), e.result.chunk_id) == key) + { exact_results.push(r); } } @@ -793,40 +923,101 @@ impl CodesearchService { Vec::new() }; + // The pure fusion functions key on a single bare u32 — but chunk ids + // are per-repo counters and collide across a group, which fused + // different repos' chunks into one entry. Intern every (repo, chunk) + // pair to a unique synthetic id for the fusion call; the fused ids + // map straight back to their owning repo below. + let mut interned: std::collections::HashMap<(String, u32), u32> = + std::collections::HashMap::new(); + + let vector_translated: Vec = vector_results + .iter() + .map(|hit| { + let mut r = hit.result.clone(); + r.id = intern_group_chunk_id(&mut interned, (hit.alias.clone(), r.id)); + r + }) + .collect(); + let fts_translated: Vec = fts_results + .iter() + .map(|hit| crate::fts::FtsResult { + chunk_id: intern_group_chunk_id( + &mut interned, + (hit.alias.clone(), hit.result.chunk_id), + ), + score: hit.result.score, + }) + .collect(); + let exact_translated: Vec = all_exact + .iter() + .map(|hit| crate::fts::FtsResult { + chunk_id: intern_group_chunk_id( + &mut interned, + (hit.alias.clone(), hit.result.chunk_id), + ), + score: hit.result.score, + }) + .collect(); + // RRF fusion let fused = if identifiers.is_empty() { - rrf_fusion(&vector_results, &fts_results, vector_k as f32) + rrf_fusion(&vector_translated, &fts_translated, vector_k as f32) } else { rrf_fusion_with_exact( - &vector_results, - &fts_results, - &all_exact, + &vector_translated, + &fts_translated, + &exact_translated, vector_k as f32, fts_k as f32, EXACT_MATCH_RRF_K, ) }; - // Map FusedResult back to SearchResult via chunk lookup across all stores - let chunk_to_result: std::collections::HashMap = - vector_results.iter().map(|r| (r.id, r)).collect(); + // Reverse side of the interning table: synthetic id -> (repo, chunk id). + let origins: Vec<(String, u32)> = { + let mut v: Vec<(String, u32)> = vec![Default::default(); interned.len()]; + for ((alias, chunk_id), synth) in &interned { + v[*synth as usize] = (alias.clone(), *chunk_id); + } + v + }; + + // Fused ids that came from the vector half resolve by direct lookup; + // FTS-only hits are resolved in their ORIGIN store — never "the + // first store that answers this id". + let by_synth: std::collections::HashMap< + u32, + &SourcedResult, + > = vector_results + .iter() + .map(|hit| (interned[&(hit.alias.clone(), hit.result.id)], hit)) + .collect(); let mut mapped: Vec = Vec::new(); for f in fused.into_iter().take(limit) { - if let Some(result) = chunk_to_result.get(&f.chunk_id) { - let mut r = (*result).clone(); + let Some((alias, real_id)) = origins.get(f.chunk_id as usize).cloned() else { + continue; + }; + let root = alias_roots.get(&alias).map(String::as_str).unwrap_or(""); + if let Some(hit) = by_synth.get(&f.chunk_id) { + let mut r = hit.result.clone(); + r.id = real_id; r.score = f.rrf_score; + r.path = prefix_path_with_alias(&r.path, Some(&alias), root); mapped.push(r); } else { - // Chunk from FTS but not in vector results — resolve from stores + // Chunk from FTS but not in vector results — resolve from its + // origin store only. if let Some(resolved) = self - .resolve_chunk_from_stores( - f.chunk_id, + .resolve_chunk_in_origin( + &alias, + real_id, f.rrf_score, &stores, aliases, + alias_roots, &mut search_warnings, - &mut alias_by_chunk, ) .await { @@ -840,8 +1031,6 @@ impl CodesearchService { boost_kind(&mut mapped, target_kind); } - prefix_paths_with_alias(&mut mapped, &alias_by_chunk, alias_roots); - self.build_semantic_response( mapped, request, @@ -853,18 +1042,80 @@ impl CodesearchService { ) } - /// Resolve a single chunk from multiple stores (used for FTS-only hits in multi-store fusion). + /// Resolve a fused FTS-only hit to full metadata, in its ORIGIN store. + /// + /// The old name ("from_stores") probed every store for the bare chunk id + /// and kept the first answer — on a group, ids are per-repo counters, so + /// that answered from the wrong repo and content/path attribution went to + /// whoever was probed first, not to the repo whose FTS actually matched. #[allow(clippy::too_many_arguments)] - async fn resolve_chunk_from_stores( + async fn resolve_chunk_in_origin( &self, + alias: &str, chunk_id: u32, score: f32, stores: &[Arc], aliases: &[String], + alias_roots: &std::collections::HashMap, warnings: &mut Vec, - alias_by_chunk: &mut std::collections::HashMap, ) -> Option { - for (idx, store_arc) in stores.iter().enumerate() { + let idx = aliases.iter().position(|a| a == alias)?; + let store_arc = &stores[idx]; + let store = match bounded_vector_read(&store_arc.vector_store).await { + Ok(store) => store, + Err(e) => { + note_store_failure(warnings, aliases, idx, "chunk lookup", &e); + return None; + } + }; + let root = alias_roots.get(alias).map(String::as_str).unwrap_or(""); + match store.get_chunk(chunk_id) { + Ok(Some(chunk)) => Some(crate::vectordb::SearchResult { + id: chunk_id, + content: chunk.content, + path: prefix_path_with_alias(&chunk.path, Some(alias), root), + start_line: chunk.start_line, + end_line: chunk.end_line, + kind: chunk.kind, + signature: chunk.signature, + docstring: chunk.docstring, + context: chunk.context, + hash: chunk.hash, + distance: 0.0, + score, + context_prev: chunk.context_prev, + context_next: chunk.context_next, + }), + Ok(None) => None, + Err(ref e) => { + note_store_failure(warnings, aliases, idx, "chunk lookup", e); + None + } + } + } + + /// Resolve origin-tagged FTS results to SearchResult, each in its own + /// repo's store, with the alias prefix applied from the tag. + async fn resolve_fts_to_search_results_multi( + &self, + fts_results: &[SourcedResult], + limit: usize, + stores: &[Arc], + aliases: &[String], + alias_roots: &std::collections::HashMap, + warnings: &mut Vec, + ) -> Vec { + let mut results = Vec::new(); + for fts in fts_results.iter().take(limit) { + // `Ok(None)` means "the ORIGIN store no longer holds this chunk" + // and is skipped; `Err` means the store is broken — collapsing + // the two is how a dead vector store renders as an empty literal + // search (the step-8 incident shape), so `Err` stays loud. + let idx = match aliases.iter().position(|a| *a == fts.alias) { + Some(idx) => idx, + None => continue, + }; + let store_arc = &stores[idx]; let store = match bounded_vector_read(&store_arc.vector_store).await { Ok(store) => store, Err(e) => { @@ -872,21 +1123,19 @@ impl CodesearchService { continue; } }; - let looked_up = store.get_chunk(chunk_id); + let looked_up = store.get_chunk(fts.result.chunk_id); if let Err(ref e) = looked_up { note_store_failure(warnings, aliases, idx, "chunk lookup", e); } if let Ok(Some(chunk)) = looked_up { - // Record the owning alias with the content actually chosen: - // path attribution below must match this very chunk, and the - // probe order here is what decided it. - if let Some(alias) = aliases.get(idx) { - alias_by_chunk.insert(chunk_id, alias.clone()); - } - return Some(crate::vectordb::SearchResult { - id: chunk_id, + let root = alias_roots + .get(&fts.alias) + .map(String::as_str) + .unwrap_or(""); + results.push(crate::vectordb::SearchResult { + id: fts.result.chunk_id, content: chunk.content, - path: chunk.path, + path: prefix_path_with_alias(&chunk.path, Some(&fts.alias), root), start_line: chunk.start_line, end_line: chunk.end_line, kind: chunk.kind, @@ -895,71 +1144,12 @@ impl CodesearchService { context: chunk.context, hash: chunk.hash, distance: 0.0, - score, + score: fts.result.score, context_prev: chunk.context_prev, context_next: chunk.context_next, }); } } - None - } - - /// Resolve FTS results to SearchResult using multiple stores. - #[allow(clippy::too_many_arguments)] - async fn resolve_fts_to_search_results_multi( - &self, - fts_results: &[crate::fts::FtsResult], - limit: usize, - stores: &[Arc], - aliases: &[String], - warnings: &mut Vec, - alias_by_chunk: &mut std::collections::HashMap, - ) -> Vec { - let mut results = Vec::new(); - for fts in fts_results.iter().take(limit) { - for (idx, store_arc) in stores.iter().enumerate() { - let store = match bounded_vector_read(&store_arc.vector_store).await { - Ok(store) => store, - Err(e) => { - note_store_failure(warnings, aliases, idx, "chunk lookup", &e); - continue; - } - }; - let looked_up = store.get_chunk(fts.chunk_id); - if let Err(ref e) = looked_up { - // `Ok(None)` means "this store does not hold that chunk" and - // is normal during fan-out; `Err` means the store is broken. - // Collapsing the two is how a dead vector store renders as - // an empty literal search — the exact shape of the step-8 - // incident, which tantivy-side checks cannot detect. - note_store_failure(warnings, aliases, idx, "chunk lookup", e); - } - if let Ok(Some(chunk)) = looked_up { - // Same as resolve_chunk_from_stores: attribute the alias of - // the store this content was actually read from. - if let Some(alias) = aliases.get(idx) { - alias_by_chunk.insert(fts.chunk_id, alias.clone()); - } - results.push(crate::vectordb::SearchResult { - id: fts.chunk_id, - content: chunk.content, - path: chunk.path, - start_line: chunk.start_line, - end_line: chunk.end_line, - kind: chunk.kind, - signature: chunk.signature, - docstring: chunk.docstring, - context: chunk.context, - hash: chunk.hash, - distance: 0.0, - score: fts.score, - context_prev: chunk.context_prev, - context_next: chunk.context_next, - }); - break; // Found in this store, skip remaining stores - } - } - } results } @@ -1054,7 +1244,7 @@ impl CodesearchService { #[allow(clippy::too_many_arguments)] fn build_semantic_response( &self, - results: Vec, + mut results: Vec, request: &SemanticSearchRequest, compact: bool, has_identifiers: bool, @@ -1072,9 +1262,25 @@ impl CodesearchService { } else { Some(warnings.to_vec()) }; + // `min_score` turns nearest-neighbour noise into an honest refusal: + // weak hits are dropped before the response is built, so the empty + // arm below explains a deliberate threshold refusal instead of + // presenting the closest-but-irrelevant chunks as matches. + let hits_before_min_score = results.len(); + if let Some(min_score) = request.min_score { + results.retain(|r| r.score >= min_score); + } if results.is_empty() { + let note = match (request.min_score, hits_before_min_score) { + (Some(min_score), n) if n > 0 => Some(format!( + "{n} candidate hit(s) found but all scored below min_score {min_score:.3}; \ + refusing instead of returning the nearest neighbours." + )), + _ => None, + }; let response = SemanticSearchResponse { results: vec![], + note, low_confidence: Some(true), suggested_tool: retry_hint(Some("literal_search".to_string()), &warnings), warnings, @@ -1114,8 +1320,10 @@ impl CodesearchService { .map(|r| SearchResultItem { chunk_id: Some(r.id), path: r.path, - start_line: r.start_line, - end_line: r.end_line, + // Chunks store 0-indexed lines; every response emits 1-based + // editor lines, so the conversion happens once, here. + start_line: r.start_line + 1, + end_line: r.end_line + 1, kind: r.kind, score: r.score, signature: r.signature, @@ -1152,6 +1360,7 @@ impl CodesearchService { results: items, low_confidence, suggested_tool, + note: None, warnings, }; @@ -1215,24 +1424,3 @@ impl CodesearchService { } } } - -/// Prefix group-fan-out result paths with the repo alias each chunk was -/// recorded from, mirroring the `project=` routing ("alias/repo-relative"). -/// -/// Results whose alias was not recorded (every survivor is recorded at -/// fan-out or resolution time, so this indicates an internal slip) keep their -/// raw path — `build_semantic_response` still normalizes it. -fn prefix_paths_with_alias( - results: &mut [crate::vectordb::SearchResult], - alias_by_chunk: &std::collections::HashMap, - alias_roots: &std::collections::HashMap, -) { - for r in results.iter_mut() { - let Some(alias) = alias_by_chunk.get(&r.id) else { - continue; - }; - if let Some(root) = alias_roots.get(alias) { - r.path = prefix_path_with_alias(&r.path, Some(alias), root); - } - } -} diff --git a/src/mcp/tests.rs b/src/mcp/tests.rs index 277f7442..6a4c48a8 100644 --- a/src/mcp/tests.rs +++ b/src/mcp/tests.rs @@ -396,6 +396,7 @@ fn test_low_confidence_response_serialization() { results: vec![], low_confidence: Some(true), suggested_tool: Some("literal_search".to_string()), + note: None, warnings: None, }; let json = serde_json::to_string(&response).unwrap(); @@ -422,6 +423,7 @@ fn test_normal_response_omits_confidence_fields() { }], low_confidence: None, suggested_tool: None, + note: None, warnings: None, }; let json = serde_json::to_string(&response).unwrap(); @@ -692,6 +694,7 @@ async fn group_lexical_search_attributes_each_hit_to_its_repo() { mode: Some("lexical".to_string()), project: None, group: Some("all".to_string()), + min_score: None, }; let mut alias_roots = std::collections::HashMap::new(); alias_roots.insert( @@ -1166,6 +1169,7 @@ fn test_semantic_search_response_with_results() { }], low_confidence: None, suggested_tool: None, + note: None, warnings: None, }; let json = serde_json::to_string(&response).unwrap(); @@ -1180,6 +1184,7 @@ fn test_semantic_search_response_empty_with_low_confidence() { results: vec![], low_confidence: Some(true), suggested_tool: Some("find_definition".to_string()), + note: None, warnings: None, }; let json = serde_json::to_string(&response).unwrap(); @@ -1215,18 +1220,19 @@ fn test_match_line_for_literal_regex() { #[test] fn test_parse_import_lines_detects_common_forms() { let content = "use std::fs;\nimport os\nfrom pkg import thing\n#include \nconst x = require('x')\nlet y = 1;"; + // `10` is the chunk's 0-based first line; emitted lines are 1-based. let imports = super::parse_import_lines(content, 10); assert_eq!(imports.len(), 5); assert_eq!(imports[0].kind, "use"); - assert_eq!(imports[0].line, 10); + assert_eq!(imports[0].line, 11); assert_eq!(imports[1].kind, "import"); - assert_eq!(imports[1].line, 11); + assert_eq!(imports[1].line, 12); assert_eq!(imports[2].kind, "import"); - assert_eq!(imports[2].line, 12); + assert_eq!(imports[2].line, 13); assert_eq!(imports[3].kind, "include"); - assert_eq!(imports[3].line, 13); + assert_eq!(imports[3].line, 14); assert_eq!(imports[4].kind, "require"); - assert_eq!(imports[4].line, 14); + assert_eq!(imports[4].line, 15); } // === Project/group routing tests === @@ -1990,6 +1996,7 @@ fn test_literal_response_json_has_lc_fields() { let response = super::LiteralSearchResponse { results: vec![], auto_promoted_to_regex: None, + relaxed_fallback: None, note: None, low_confidence: Some(true), suggested_tool: Some("search with mode='semantic'".to_string()), @@ -2005,6 +2012,7 @@ fn test_literal_response_json_omits_lc_fields_when_none() { let response = super::LiteralSearchResponse { results: vec![], auto_promoted_to_regex: None, + relaxed_fallback: None, note: None, low_confidence: None, suggested_tool: None, @@ -2153,6 +2161,7 @@ fn test_literal_search_response_shape_json() { signature: None, }], auto_promoted_to_regex: None, + relaxed_fallback: None, note: None, low_confidence: None, suggested_tool: None, @@ -2169,6 +2178,7 @@ fn test_literal_search_response_carries_note_when_promoted() { let response = super::LiteralSearchResponse { results: vec![], auto_promoted_to_regex: Some(true), + relaxed_fallback: None, note: Some("auto-promoted".to_string()), low_confidence: None, suggested_tool: None, @@ -2237,6 +2247,251 @@ fn note_store_failure_survives_a_short_alias_list() { assert!(warnings[0].contains("unknown")); } +// === relaxed-fallback coverage gate ===================================== +// +// The relaxed BM25 pass rescues candidates via signature/kind matches, so +// the coverage gate must see the same fields the rescue saw. These pin the +// searchable_text gate text against both the signature-only candidate (the +// exact misattribution class the fallback exists for) and legacy blobs that +// predate the stored field. + +#[test] +fn relaxed_gate_counts_signature_only_matches() { + use crate::vectordb::ChunkMetadata; + + let chunk = ChunkMetadata { + content: "let x = compute();".to_string(), + path: "src/dispatch.rs".to_string(), + start_line: 0, + end_line: 2, + kind: "Function".to_string(), + signature: Some("fn transfer_overdue_process()".to_string()), + docstring: None, + context: None, + hash: String::new(), + context_prev: None, + context_next: None, + searchable_text: "fn transfer_overdue_process()\nFunction\nlet x = compute();".to_string(), + }; + let terms = super::literal_search::significant_query_terms("transfer overdue process"); + + // The signature carries all three terms; the gate over searchable_text + // must keep this candidate... + assert!(super::literal_search::chunk_covers_significant_terms( + super::literal_search::relaxed_gate_text(&chunk), + &terms, + )); + // ...while the old content-only gate dropped it — the self-defeating + // behaviour this fixes. + assert!(!super::literal_search::chunk_covers_significant_terms( + &chunk.content, + &terms, + )); +} + +#[test] +fn relaxed_gate_falls_back_to_content_for_legacy_blobs() { + use crate::vectordb::ChunkMetadata; + + let chunk = ChunkMetadata { + content: "transfer_overdue_process(ctx)".to_string(), + path: "src/legacy.rs".to_string(), + start_line: 0, + end_line: 0, + kind: "Function".to_string(), + signature: Some("fn transfer_overdue_process(ctx)".to_string()), + docstring: None, + context: None, + hash: String::new(), + context_prev: None, + context_next: None, + searchable_text: String::new(), + }; + let terms = super::literal_search::significant_query_terms("transfer overdue process"); + + assert!(super::literal_search::chunk_covers_significant_terms( + super::literal_search::relaxed_gate_text(&chunk), + &terms, + )); +} + +// === unified search dispatch: min_score honesty ======================== +// +// min_score used to be silently ignored on the literal path and dropped on +// both federated legs; the tool advertised it anyway. These pin the +// by-name refusal and the peer body composition. + +#[test] +fn build_remote_search_body_forwards_min_score_to_peers() { + let request = crate::mcp::types::SearchRequest { + query: "transfer overdue".to_string(), + mode: Some("semantic".to_string()), + compact: None, + semantic_mode: None, + filter_path: None, + min_score: Some(0.42), + regex: None, + phrase: None, + file_glob: None, + language: None, + format: None, + limit: None, + project: None, + group: None, + }; + let body = super::CodesearchService::build_remote_search_body(&request, "semantic", Some(50)); + let got = body["min_score"] + .as_f64() + .expect("min_score must be numeric in the peer body"); + assert!( + (got - 0.42).abs() < 1e-6, + "body must forward min_score: {body}" + ); + assert_eq!(body["mode"], "semantic", "body must keep its shape: {body}"); +} + +#[test] +fn group_fusion_interning_gives_colliding_chunk_ids_distinct_synthetic_ids() { + // The group fusion feeds the pure rrf_fusion, which keys on a single + // bare u32 — two repos both holding local chunk id 0 used to fuse into + // ONE entry, silently dropping a whole repo's hit. The interning table + // is the fix; this pins its contract at the unit level, without + // standing up stores. + use std::collections::HashMap; + + fn vector_hit(id: u32, score: f32) -> crate::vectordb::SearchResult { + crate::vectordb::SearchResult { + id, + content: String::new(), + path: format!("file{id}.rs"), + start_line: 0, + end_line: 1, + kind: "Function".to_string(), + signature: None, + docstring: None, + context: None, + hash: String::new(), + distance: 1.0 - score, + score, + context_prev: None, + context_next: None, + } + } + + // The collision the old bare-u32 fusion actually hit: same chunk id, + // different repos — one fused entry. + let colliding = vec![vector_hit(0, 0.9), vector_hit(0, 0.8)]; + let fused_colliding = crate::rerank::rrf_fusion(&colliding, &[], 60.0); + assert_eq!( + fused_colliding.len(), + 1, + "bare ids must collide in rrf_fusion — this is what interning exists to fix" + ); + + let mut interned: HashMap<(String, u32), u32> = HashMap::new(); + let a = super::intern_group_chunk_id(&mut interned, ("alpha".to_string(), 0)); + let b = super::intern_group_chunk_id(&mut interned, ("beta".to_string(), 0)); + assert_ne!(a, b, "same local id in different repos must intern apart"); + + // Repeat lookups return the SAME synthetic id — the reverse table maps + // fused ids back to owners, so re-interning would orphan results. + let a_again = super::intern_group_chunk_id(&mut interned, ("alpha".to_string(), 0)); + assert_eq!(a, a_again, "re-interning the same pair must be stable"); + + // With interned ids both hits survive the fusion. + let interned_hits = vec![vector_hit(a, 0.9), vector_hit(b, 0.8)]; + let fused = crate::rerank::rrf_fusion(&interned_hits, &[], 60.0); + assert_eq!(fused.len(), 2, "interned ids must keep both repos' hits"); + assert_eq!(fused[0].chunk_id, a, "rank order follows the input scores"); + assert_eq!(fused[1].chunk_id, b, "rank order follows the input scores"); +} + +#[test] +fn mixed_model_group_warning_fires_only_for_genuinely_mixed_groups() { + use crate::embed::ModelType; + + // One model — even repeated by several repos — is one scale. + assert!( + super::mixed_model_group_warning([ModelType::AllMiniLML6V2Q]).is_none(), + "a single-model group must not warn" + ); + assert!( + super::mixed_model_group_warning([ModelType::AllMiniLML6V2Q, ModelType::AllMiniLML6V2Q,]) + .is_none(), + "the same model across repos is still one scale" + ); + + let warning = + super::mixed_model_group_warning([ModelType::AllMiniLML6V2Q, ModelType::BGESmallENV15]) + .expect("a genuinely mixed group must warn"); + assert!( + warning.contains("NOT comparable"), + "the warning must say the scores are not comparable: {warning}" + ); + for name in [ + ModelType::AllMiniLML6V2Q.short_name(), + ModelType::BGESmallENV15.short_name(), + ] { + assert!( + warning.contains(name), + "every distinct model must be named ('{name}' missing): {warning}" + ); + } +} + +#[tokio::test] +async fn search_refuses_min_score_in_literal_mode_by_name() { + use rmcp::handler::server::wrapper::Parameters; + + // Minimal hermetic service fixture: the refusal fires before any store + // access, so an empty index is enough. + let tmp = tempfile::tempdir().unwrap(); + let db = tmp.path().join(".codesearch.db"); + std::fs::create_dir_all(&db).unwrap(); + std::fs::write( + db.join("metadata.json"), + r#"{"schema_version":1,"dimensions":2,"model_short_name":"minilm-l6-q"}"#, + ) + .unwrap(); + let stores = std::sync::Arc::new(crate::index::SharedStores::new(&db, 2).expect("stores")); + let service = + super::CodesearchService::new_with_stores(Some(tmp.path().to_path_buf()), Some(stores)) + .expect("service"); + + let request = crate::mcp::types::SearchRequest { + query: "handle_transfer".to_string(), + mode: Some("literal".to_string()), + compact: None, + semantic_mode: None, + filter_path: None, + min_score: Some(0.5), + regex: None, + phrase: None, + file_glob: None, + language: None, + format: None, + limit: None, + project: None, + group: None, + }; + let res = service + .search(Parameters(request)) + .await + .expect("search must answer"); + let text = match res.content.first() { + Some(rmcp::model::ContentBlock::Text(t)) => t.text.clone(), + other => panic!("expected text content, got {other:?}"), + }; + assert!( + text.contains("min_score"), + "refusal must name the field: {text}" + ); + assert!( + text.contains("literal"), + "refusal must name the mode: {text}" + ); +} + // === status(kind="index") multi-store summary ========================== // // Follow-up 16: a store failing mid-fan-out used to render identically to @@ -2428,13 +2683,24 @@ fn record_stats_or_warn_does_not_duplicate_the_same_warning() { #[test] fn into_results_routes_failures_into_warnings() { let outcome = super::MultiReadOutcome { - results: vec![1u32, 2], + results: vec![ + super::SourcedResult::new("inriver", 1u32), + super::SourcedResult::new("example-repo", 2), + ], failures: vec![("inriver".to_string(), "os error 22".to_string())], }; let mut warnings = Vec::new(); let results = outcome.into_results(&mut warnings, "chunk lookup"); - assert_eq!(results, vec![1, 2]); + // Unwrap the origin tags the way every consumer now reads them. + let unwrapped: Vec<(String, u32)> = results + .into_iter() + .map(|tagged| (tagged.alias, tagged.result)) + .collect(); + assert_eq!( + unwrapped, + vec![("inriver".to_string(), 1), ("example-repo".to_string(), 2)] + ); assert_eq!( warnings, vec!["repo 'inriver' chunk lookup failed: os error 22".to_string()], @@ -2718,6 +2984,7 @@ fn semantic_response_emits_warnings_to_the_caller() { results: vec![], low_confidence: Some(true), suggested_tool: None, + note: None, warnings: Some(vec!["repo 'inriver' search failed: os error 22".to_string()]), }; let json = serde_json::to_string(&response).unwrap(); @@ -2733,10 +3000,12 @@ fn semantic_response_omits_warnings_when_healthy() { results: vec![], low_confidence: None, suggested_tool: None, + note: None, warnings: None, }; let json = serde_json::to_string(&response).unwrap(); assert!(!json.contains("warnings"), "got: {json}"); + assert!(!json.contains("note"), "got: {json}"); } #[test] @@ -2744,6 +3013,7 @@ fn literal_response_emits_warnings_and_omits_them_when_healthy() { let failed = super::LiteralSearchResponse { results: vec![], auto_promoted_to_regex: None, + relaxed_fallback: None, note: None, low_confidence: None, suggested_tool: None, @@ -2758,6 +3028,7 @@ fn literal_response_emits_warnings_and_omits_them_when_healthy() { let healthy = super::LiteralSearchResponse { results: vec![], auto_promoted_to_regex: None, + relaxed_fallback: None, note: None, low_confidence: None, suggested_tool: None, @@ -2772,6 +3043,7 @@ fn test_literal_search_response_omits_fields_when_not_promoted() { let response = super::LiteralSearchResponse { results: vec![], auto_promoted_to_regex: None, + relaxed_fallback: None, note: None, low_confidence: None, suggested_tool: None, @@ -2795,6 +3067,7 @@ fn test_grep_format_includes_comment_when_promoted() { signature: None, }], auto_promoted_to_regex: Some(true), + relaxed_fallback: None, note: None, low_confidence: None, suggested_tool: None, @@ -2829,6 +3102,7 @@ fn test_grep_format_no_comment_when_plain() { signature: None, }], auto_promoted_to_regex: None, + relaxed_fallback: None, note: None, low_confidence: None, suggested_tool: None, @@ -3067,6 +3341,209 @@ fn single_index_status_requires_a_built_graph_to_report_ready() { assert_eq!(status, "building"); } +// === group fan-out degradation under write locks === +// +// A multi-repo fan-out is an interactive query: it must never queue behind a +// repo held by a running indexing batch. Before the fix the fan-out used the +// bounded 300s read wait per store, so N busy repos summed to N waits and +// stalled every other repo's answer — the MCP wedge. + +#[tokio::test] +async fn vector_fan_out_skips_write_locked_store_instead_of_waiting() { + // alpha's vector write lock is held for the whole call, as an indexing + // batch would hold it; beta is free. The fan-out must skip alpha (one + // failure naming it) instead of blocking on it. + let root_a = tempfile::tempdir().expect("tempdir a"); + let stores_a = std::sync::Arc::new( + crate::index::SharedStores::new(&root_a.path().join(".codesearch.db"), 2) + .expect("stores a"), + ); + let root_b = tempfile::tempdir().expect("tempdir b"); + let stores_b = std::sync::Arc::new( + crate::index::SharedStores::new(&root_b.path().join(".codesearch.db"), 2) + .expect("stores b"), + ); + let service = super::CodesearchService::new_with_stores( + Some(root_b.path().to_path_buf()), + Some(stores_b.clone()), + ) + .expect("service"); + + let _alpha_write = stores_a.vector_store.write().await; + + let outcome = service + .with_vector_store_read_multi( + |_alias, _store| Ok::<_, anyhow::Error>(Vec::::new()), + vec![stores_a.clone(), stores_b.clone()], + &["alpha".to_string(), "beta".to_string()], + ) + .await + .expect("fan-out itself must not error"); + + assert!( + outcome.results.is_empty(), + "the locked store must be skipped, not answered from" + ); + assert_eq!( + outcome.failures.len(), + 1, + "only alpha must fail, failures: {:?}", + outcome.failures + ); + assert_eq!(outcome.failures[0].0, "alpha"); + assert!( + outcome.failures[0].1.contains("store busy"), + "failure must explain itself: {}", + outcome.failures[0].1 + ); +} + +#[tokio::test] +async fn lexical_group_search_degrades_per_repo_when_a_store_is_write_locked() { + use rmcp::model::ContentBlock; + + // Same contract through the full lexical flow: alpha mid-indexing, beta + // searchable. The group answers from beta alone, with a warning naming + // alpha — the skipped repo must degrade visibly, never silently. + let root_a = tempfile::tempdir().expect("tempdir a"); + let stores_a = std::sync::Arc::new( + crate::index::SharedStores::new(&root_a.path().join(".codesearch.db"), 2) + .expect("stores a"), + ); + { + let mut vs = stores_a.vector_store.write().await; + let chunk = crate::chunker::Chunk::new( + "fn gadget() {}".to_string(), + 0, + 0, + crate::chunker::ChunkKind::Function, + "src/lib.rs".to_string(), + ); + vs.insert_chunks(vec![crate::embed::EmbeddedChunk::new( + chunk, + vec![0.0, 1.0], + )]) + .expect("insert chunk a"); + vs.build_index().expect("build index a"); + } + { + let mut fts = stores_a.fts_store.write().await; + fts.add_chunk(0, "fn gadget() {}", "src/lib.rs", None, "Function") + .expect("fts doc a"); + fts.commit().expect("commit a"); + } + let root_b = tempfile::tempdir().expect("tempdir b"); + let stores_b = std::sync::Arc::new( + crate::index::SharedStores::new(&root_b.path().join(".codesearch.db"), 2) + .expect("stores b"), + ); + { + // insert_chunks assigns ids from 0 per store, so a filler chunk keeps + // the real one at id 1 and the two test repos never share a chunk id. + let mut vs = stores_b.vector_store.write().await; + let filler = crate::chunker::Chunk::new( + "filler".to_string(), + 0, + 0, + crate::chunker::ChunkKind::Function, + "src/filler.rs".to_string(), + ); + let chunk = crate::chunker::Chunk::new( + "gadget helper".to_string(), + 0, + 0, + crate::chunker::ChunkKind::Function, + "src/util.rs".to_string(), + ); + vs.insert_chunks(vec![crate::embed::EmbeddedChunk::new( + filler, + vec![0.5, 0.5], + )]) + .expect("insert filler b"); + vs.insert_chunks(vec![crate::embed::EmbeddedChunk::new( + chunk, + vec![1.0, 0.0], + )]) + .expect("insert chunk b"); + vs.build_index().expect("build index b"); + } + { + let mut fts = stores_b.fts_store.write().await; + fts.add_chunk(1, "gadget helper", "src/util.rs", None, "Function") + .expect("fts doc b"); + fts.commit().expect("commit b"); + } + + let service = super::CodesearchService::new_with_stores( + Some(root_b.path().to_path_buf()), + Some(stores_b.clone()), + ) + .expect("service"); + + let request = super::SemanticSearchRequest { + query: "gadget".to_string(), + limit: Some(10), + compact: Some(false), + filter_path: None, + mode: Some("lexical".to_string()), + project: None, + group: Some("all".to_string()), + min_score: None, + }; + let mut alias_roots = std::collections::HashMap::new(); + alias_roots.insert( + "alpha".to_string(), + crate::cache::normalize_path_str(&root_a.path().to_string_lossy()), + ); + alias_roots.insert( + "beta".to_string(), + crate::cache::normalize_path_str(&root_b.path().to_string_lossy()), + ); + + // Hold alpha's FTS write lock across the call, as a live indexing run + // would. The query must still return promptly from beta. + let _alpha_fts_write = stores_a.fts_store.write().await; + + let result = service + .semantic_search_multi( + &request, + &[], + 10, + false, + vec![stores_a.clone(), stores_b.clone()], + &["alpha".to_string(), "beta".to_string()], + &alias_roots, + ) + .await + .expect("group search must succeed despite one busy store"); + + let json = match &result.content[0] { + ContentBlock::Text(t) => t.text.clone(), + other => panic!("expected text content, got {other:?}"), + }; + let parsed: serde_json::Value = serde_json::from_str(&json).expect("valid json response"); + let paths: Vec<&str> = parsed["results"] + .as_array() + .expect("results array") + .iter() + .map(|r| r["path"].as_str().expect("path string")) + .collect(); + assert_eq!(paths, vec!["beta/src/util.rs"], "paths: {paths:?}"); + let warnings_text = parsed["warnings"] + .as_array() + .map(|ws| { + ws.iter() + .map(|w| w.as_str().unwrap_or_default()) + .collect::>() + .join("\n") + }) + .unwrap_or_default(); + assert!( + warnings_text.contains("alpha") && warnings_text.contains("store busy"), + "warnings must name the skipped repo and why: {warnings_text}" + ); +} + /// A running rebuild overrides "ready": the stores are still being rewritten /// even though the previous generation's graph reports built. #[test] diff --git a/src/mcp/types.rs b/src/mcp/types.rs index 67df7f68..f49c2cf9 100644 --- a/src/mcp/types.rs +++ b/src/mcp/types.rs @@ -40,6 +40,15 @@ pub struct SearchRequest { /// Only return results from files under this path prefix (semantic mode). pub filter_path: Option, + /// Drop semantic hits scoring below this threshold (semantic mode only). + /// When nothing survives, the answer is an explicit refusal — an empty + /// `results` array plus a `note` explaining the threshold — instead of + /// the nearest neighbours posing as matches. Scores are mode-specific: + /// cosine similarity under `semantic_mode="semantic"` (0.0-1.0), RRF + /// points under `"auto"`/`"hybrid"`/`"lexical"` (rarely above 0.2), so + /// calibrate per mode before reusing a value. + pub min_score: Option, + // ── Literal-mode options (ignored in semantic mode) ── /// Treat `query` as a regex pattern (literal mode only). pub regex: Option, @@ -196,6 +205,8 @@ pub struct SemanticSearchRequest { pub project: Option, #[serde(default)] pub group: Option, + #[serde(default)] + pub min_score: Option, } /// Internal params for literal search (used by `search` tool dispatch). @@ -391,8 +402,14 @@ pub struct LiteralSearchResponse { #[serde(skip_serializing_if = "Option::is_none")] pub auto_promoted_to_regex: Option, - /// Actionable note for the LLM caller (present iff auto_promoted_to_regex - /// or low_confidence is set). + /// True when the exact AND pass matched nothing and these results come + /// from the relaxed OR fallback: every kept hit covers >=60% of the + /// query's significant (non-stopword) terms, at least two of them. + #[serde(skip_serializing_if = "Option::is_none")] + pub relaxed_fallback: Option, + + /// Actionable note for the LLM caller (present iff auto_promoted_to_regex, + /// relaxed_fallback or low_confidence is set). #[serde(skip_serializing_if = "Option::is_none")] pub note: Option, @@ -420,6 +437,11 @@ pub struct SemanticSearchResponse { pub low_confidence: Option, #[serde(skip_serializing_if = "Option::is_none")] pub suggested_tool: Option, + /// Explains an outcome the caller must not mistake for "no match in the + /// index": e.g. candidate hits existed but all fell below `min_score` + /// and were refused. + #[serde(skip_serializing_if = "Option::is_none")] + pub note: Option, /// Federation health warnings — populated when one or more remote peers in /// the queried group were unreachable. The query still returns (degraded) /// local + remaining-remote results; these warnings explain the gap so an diff --git a/src/search/mod.rs b/src/search/mod.rs index 3011bb49..a779affb 100644 --- a/src/search/mod.rs +++ b/src/search/mod.rs @@ -935,8 +935,8 @@ pub async fn search(query: &str, path: Option, options: SearchOptions) .iter() .map(|r| JsonResult { path: r.path.clone(), - start_line: r.start_line, - end_line: r.end_line, + start_line: r.start_line + 1, + end_line: r.end_line + 1, kind: r.kind.clone(), content: if compact { None @@ -1276,8 +1276,8 @@ fn print_result( // Show location and kind let location = format!( " Lines {}-{} • {}", - result.start_line, - result.end_line, + result.start_line + 1, + result.end_line + 1, sanitize_for_terminal(&result.kind) ); println!("{}", location.dimmed()); diff --git a/src/serve/mod.rs b/src/serve/mod.rs index 0fb4af09..8087be02 100644 --- a/src/serve/mod.rs +++ b/src/serve/mod.rs @@ -41,8 +41,9 @@ use crate::constants::{ DB_DIR_NAME, DEFAULT_SERVE_PORT, DISABLE_HOST_VALIDATION_ENV, EXPLORE_PATH, FIND_IMPACT_PATH, FIND_PATH, HEALTHZ_PATH, HEALTH_PATH, INDEXING_MARKER_HEARTBEAT_SECS, INDEXING_PATH, LANG_CSHARP, LANG_TYPESCRIPT, MAX_INDEXING_SECS, MAX_INDEXING_SECS_ENV, MCP_ENDPOINT_PATH, - PERSIST_DEBOUNCE_SECS, REAPER_INTERVAL_SECS, REMOTES_PATH, REPO_IDLE_TIMEOUT_ENV, - REPO_IDLE_TIMEOUT_SECS, SEARCH_PATH, SERVE_API_KEY_ENV, SERVE_PORT_ENV, STATUS_PATH, + MCP_IDLE_SESSION_SECS, PERSIST_DEBOUNCE_SECS, REAPER_INTERVAL_SECS, REMOTES_PATH, + REPO_IDLE_TIMEOUT_ENV, REPO_IDLE_TIMEOUT_SECS, SEARCH_PATH, SERVE_API_KEY_ENV, SERVE_PORT_ENV, + STATUS_PATH, }; use crate::db_discovery::repos::{config_dir, ReposConfig}; use crate::index::{ @@ -1660,25 +1661,7 @@ impl ServeState { continue; } - let cfg = match state.config.read() { - Ok(c) => c.clone(), - Err(e) => { - tracing::warn!("repos persist skipped: config lock poisoned: {}", e); - state.persist_deadline_unix_ms.store(0, Ordering::Relaxed); - continue; - } - }; - // Route through persist_config so the override path is honored - // (keeps the metadata-persist worker hermetic in tests; identical - // to cfg.save() in production where the override is None). - let state_persist = state.clone(); - let save_res = - tokio::task::spawn_blocking(move || state_persist.persist_config(&cfg)).await; - match save_res { - Ok(Ok(())) => tracing::debug!("repos.json metadata persisted"), - Ok(Err(e)) => tracing::warn!("repos persist failed: {}", e), - Err(e) => tracing::warn!("repos persist task join failed: {}", e), - } + state.run_persist_pass().await; state.persist_deadline_unix_ms.store(0, Ordering::Relaxed); state.persist_worker_started.store(false, Ordering::Release); @@ -1698,6 +1681,66 @@ impl ServeState { }); } + /// One debounced-write pass over the in-memory repos config. + /// + /// The snapshot clone and the file write deliberately happen WITHOUT the + /// config lock (the write is blocking I/O), so a registration landing in + /// between exists only in memory while the stale snapshot lands on disk — + /// a lost update. `persist_config` adopting its own mtime already keeps + /// readers from clobbering memory with that stale file; this pass then + /// re-verifies memory against the persisted snapshot and rewrites until + /// they agree, so the disk converges too. Bounded: under a continuous + /// stream of mutations it gives up after a few passes and the next + /// scheduled pass picks up whatever remained. + async fn run_persist_pass(self: &Arc) { + const MAX_DIVERGENT_PASSES: usize = 5; + for attempt in 1..=MAX_DIVERGENT_PASSES { + let cfg = match self.config.read() { + Ok(c) => c.clone(), + Err(e) => { + tracing::warn!("repos persist skipped: config lock poisoned: {}", e); + return; + } + }; + // Route through persist_config so the override path is honored + // (keeps the metadata-persist worker hermetic in tests; identical + // to cfg.save() in production where the override is None). + let state_pass = self.clone(); + // Clone for the blocking write: `cfg` itself stays here for the + // post-write divergence check below. + let snapshot = cfg.clone(); + let save_res = + tokio::task::spawn_blocking(move || state_pass.persist_config(&snapshot)).await; + match save_res { + Ok(Ok(())) => tracing::debug!("repos.json metadata persisted"), + Ok(Err(e)) => { + tracing::warn!("repos persist failed: {}", e); + return; + } + Err(e) => { + tracing::warn!("repos persist task join failed: {}", e); + return; + } + } + let diverged = match self.config.read() { + Ok(current) => *current != cfg, + // A poisoned lock says nothing about divergence; don't spin. + Err(_) => false, + }; + if !diverged { + return; + } + tracing::warn!( + "repos.json diverged from memory during persist (pass {attempt}); rewriting" + ); + tokio::time::sleep(Duration::from_millis(100)).await; + } + tracing::warn!( + "repos persist gave up after {MAX_DIVERGENT_PASSES} divergent passes; \ + the next scheduled pass will rewrite" + ); + } + /// Reconcile registered repo paths against the filesystem before warmup. /// /// For each alias whose stored path no longer exists (folder renamed/moved), @@ -2349,6 +2392,10 @@ impl ServeState { // report honestly. let mut db_deleted = !db_path.exists(); let mut db_delete_error: Option = None; + // True when the DB dir's cleanup is owned by the still-running index + // task's post-build guard (or the sweeper) — the dir must stay in + // place for them, so step 4b must not quarantine it either. + let mut delete_deferred = false; if db_path.exists() && !index_task_exited { // The index task is still alive (typically parked in the // uninterruptible `build_index`). Deleting the directory out from @@ -2366,6 +2413,7 @@ impl ServeState { alias ); db_delete_error = Some("index task still running; DB cleanup deferred".to_string()); + delete_deferred = true; } else if db_path.exists() { // Deadline-bounded exponential-backoff retry. We ONLY retry on // lock-class errors (sharing/lock violation or access-denied on @@ -2475,11 +2523,53 @@ impl ServeState { } } + // 4b. Quarantine fallback. If the retry budget expired with the + // directory still on disk (an external holder: another process, an AV + // scanner), leaving it in place silently resurrects a "valid" index on + // the next registration — metadata.json model stamps and stale chunk + // stores read as ready, fan-out then serves an empty or stale store, + // and re-registration against that mismatched legacy data ends in + // arroy EINVAL (the pilot's false-ready repos and its os-error-22 + // wave). Renaming achieves DELETE's semantic goal atomically on unix: + // existing holders keep their open file descriptors until they exit, + // while a fresh registration sees no db dir at all and builds from + // scratch. The quarantined sibling stays for manual cleanup, same + // convention as the .codesearch.db.bak-* directories. Deferred + // cleanups (live index task) are excluded — their post-build guard + // owns the dir. + let mut db_quarantined: Option = None; + if !db_deleted && !delete_deferred { + let sibling = db_path.with_file_name(format!( + "{}.removed-{}", + DB_DIR_NAME, + Self::now_unix_millis() + )); + match std::fs::rename(&db_path, &sibling) { + Ok(()) => { + tracing::warn!( + "Database dir for '{}' survived the delete budget; quarantined to {} \ + (safe to delete manually)", + alias, + sibling.display() + ); + db_quarantined = Some(sibling); + } + Err(e) => { + tracing::warn!( + "Failed to quarantine database dir for '{}' (kept in place): {}", + alias, + e + ); + } + } + } + Ok(RepoRemovalOutcome { project_path, db_path, db_deleted, db_delete_error, + db_quarantined, }) } @@ -3639,9 +3729,28 @@ impl ServeState { /// assert on persistence without touching the user's real config. pub(crate) fn persist_config(&self, config: &ReposConfig) -> anyhow::Result<()> { match self.config_path_override.as_ref() { - Some(path) => config.save_to(path), - None => config.save(), + Some(path) => config.save_to(path)?, + None => config.save()?, + }; + // Adopt this write's mtime so reload_if_changed never mistakes our own + // persist for an external edit of repos.json. Before this, every + // debounced or handler write left the stored mtime stale, the next + // reader (warmup / aliases / info / group resolve) reloaded the file we + // ourselves had just written, and registrations that had landed in + // memory after that snapshot were silently dropped — the pilot's + // "ghost loss" of freshly registered aliases. + let written_path = match self.config_path_override.as_ref() { + Some(p) => Some(p.clone()), + None => ReposConfig::path().ok(), + }; + if let Some(path) = written_path { + if let Ok(mtime) = std::fs::metadata(path).and_then(|m| m.modified()) { + if let Ok(mut guard) = self.config_mtime.write() { + *guard = Some(mtime); + } + } } + Ok(()) } /// Resolve a group name to its constituent aliases. @@ -5925,6 +6034,12 @@ pub(crate) struct RepoRemovalOutcome { /// The last error from `remove_dir_all`. `Some` exactly when /// `db_deleted == false`; `None` once a delete succeeds. pub db_delete_error: Option, + /// When the DB dir survived the delete budget it was renamed out of the + /// way (`.removed-` sibling) so a fresh + /// registration builds from scratch instead of adopting the stale index. + /// `None` when the dir was deleted, never existed, or its cleanup is + /// deferred to a live index task's post-build guard. + pub db_quarantined: Option, } /// Remove-repo handler: DELETE /repos/{alias} @@ -5955,6 +6070,17 @@ async fn remove_repo_handler( "Repo removed: FSW stopped, evicted from memory, unregistered, DB deleted" .to_string(), ) + } else if let Some(quarantine) = &outcome.db_quarantined { + ( + "removed_db_quarantined", + format!( + "Repo removed: FSW stopped, evicted from memory, unregistered; \ + DB dir survived the delete budget and was quarantined to {} \ + (a fresh registration builds from scratch; delete it manually \ + when convenient)", + quarantine.display() + ), + ) } else { ( "removed_db_locked", @@ -5977,6 +6103,7 @@ async fn remove_repo_handler( "path": outcome.project_path, "db_deleted": outcome.db_deleted, "db_delete_error": outcome.db_delete_error, + "db_quarantined": outcome.db_quarantined, "message": message, })), ) @@ -6561,6 +6688,63 @@ fn keep_warm_foreign_target(ping_url: &str, self_host: &str) -> Option { } } +// rmcp session manager bounded to reap idle MCP sessions. Extracted so the +// reap contract is pinned by a test instead of only by the CHANGELOG claim: +// `keep_alive = None` assumed TCP liveness would collect wedged sessions, +// but a stuck request keeps its socket and its FIFO-serialized worker alive +// indefinitely (observed active_sessions=4 with a single client). +pub(crate) fn session_manager_with_idle_reap() -> LocalSessionManager { + let mut manager = LocalSessionManager::default(); + manager.session_config.keep_alive = Some(std::time::Duration::from_secs(MCP_IDLE_SESSION_SECS)); + manager +} + +// Best-effort startup sweep of quarantined DB remnants. remove_repo's +// delete-budget fallback renames a surviving db dir aside as +// `.removed-` "for manual cleanup" — and manual is +// exactly what never happened, so every budget-expired delete leaked a +// full stale index copy forever. By startup the external holders that +// forced the rename (AV scanner, another process) have exited with the +// previous session, so the tombstone has no readers left to protect: +// serve deletes the remnants on its way up. Failures warn and never +// block startup — a remnant that still cannot be removed just waits for +// the next restart. +pub(crate) fn sweep_quarantined_db_remnants(repo_paths: &[PathBuf]) { + let prefix = format!("{DB_DIR_NAME}.removed-"); + for repo_path in repo_paths { + let entries = match std::fs::read_dir(repo_path) { + Ok(entries) => entries, + Err(e) => { + warn!( + "quarantine sweep: failed to list {}: {}", + repo_path.display(), + e + ); + continue; + } + }; + for entry in entries.flatten() { + let name = entry.file_name(); + let Some(name) = name.to_str() else { continue }; + if !name.starts_with(&prefix) { + continue; + } + let remnant = entry.path(); + match std::fs::remove_dir_all(&remnant) { + Ok(()) => info!( + "🧹 swept quarantined db remnant at startup: {}", + remnant.display() + ), + Err(e) => warn!( + "quarantine sweep: could not remove {} (will retry next startup): {}", + remnant.display(), + e + ), + } + } + } +} + // `run_serve` is the single startup entry point, so its parameter list is the // serve CLI surface (bind host/port, registration, default model, TUI, // keep-warm, shutdown). Bundling them into a struct would only move the @@ -6650,6 +6834,91 @@ pub async fn run_serve( #[cfg(unix)] raise_fd_limit(config.repos.len()); + // Hook self-heal: upgrade codesearch-managed post-checkout blocks that + // were written by an OLDER binary (the pilot's 515 hooks kept the pre-fix + // `pwd`/serve_url bodies because nothing regenerated them — re-running + // `hooks git install` per repo by hand does not scale). Background and + // best-effort: only OUR block region is rewritten, a repo with no hook or + // a foreign hook is never touched, and failures never block startup. + let hook_repo_paths: Vec = config.repos.values().cloned().collect(); + tokio::spawn(async move { + let refreshed = tokio::task::spawn_blocking(move || { + let mut upgraded = 0usize; + let mut up_to_date = 0usize; + for repo_path in &hook_repo_paths { + match crate::cli::refresh_codesearch_post_checkout_hook(repo_path) { + crate::cli::HookRefreshOutcome::Upgraded => upgraded += 1, + crate::cli::HookRefreshOutcome::UpToDate => up_to_date += 1, + crate::cli::HookRefreshOutcome::NotInstalled + | crate::cli::HookRefreshOutcome::Foreign => {} + crate::cli::HookRefreshOutcome::Failed(e) => warn!( + "post-checkout hook refresh failed for {}: {}", + repo_path.display(), + e + ), + } + } + (upgraded, up_to_date) + }) + .await; + if let Ok((upgraded, up_to_date)) = refreshed { + if upgraded > 0 { + info!( + "🪝 post-checkout hooks: {upgraded} upgraded to this binary, \ + {up_to_date} already up to date" + ); + } + } + }); + + // Startup sweep of quarantined DB remnants — the `.removed-*` siblings + // remove_repo's delete-budget fallback leaves "for manual cleanup". Run + // on its own blocking task: never blocks the listener, and a leftover + // holder just means the remnant waits for the next restart. + let sweep_repo_paths: Vec = config.repos.values().cloned().collect(); + tokio::task::spawn_blocking(move || sweep_quarantined_db_remnants(&sweep_repo_paths)); + + // The explicit `--model` flag wins; otherwise adopt the choice persisted + // by `codesearch setup`. Without this bridge the pilot operator had to + // re-type `--model embeddinggemma-q4` on every serve launch: setup + // downloaded the model and then silently forgot the choice, so a bare + // `codesearch serve` built every NEW index with the built-in default. + let default_model = match default_model { + Some(flag) => Some(flag), + None => { + let persisted = crate::embed::load_default_model(); + if let Some(model) = persisted { + let line = format!( + "🧠 Default model from `codesearch setup`: {} ({} dims)", + model.short_name(), + model.dimensions() + ); + info!("{}", line); + eprintln!("{}", line); + Some(model) + } else { + None + } + } + }; + + // Pre-flight cache check for the effective default of new indexes. A + // missing model previously surfaced only mid-warmup as N parallel index + // jobs all hitting the network at once; warn up front with the fix hint + // instead (the operator may be behind a proxy where HF is unreachable). + let effective_default = default_model.unwrap_or_default(); + if !crate::embed::is_model_in_cache(effective_default) { + let line = format!( + "⚠️ Default embedding model '{}' is not in the models cache; the first \ + index/query will download it from the network. Run `codesearch setup --model {}` \ + to pre-download it.", + effective_default.short_name(), + effective_default.short_name(), + ); + warn!("{}", line); + eprintln!("{}", line); + } + // The idle-suspend window is resolved by the keep-warm task alone (flag > // env > default); nothing else consumes it, so `ServeState` does not carry // it. In particular the embedded TUI must NOT derive a poll cadence from it @@ -6746,12 +7015,15 @@ pub async fn run_serve( Ok(svc) }; - // Build session manager without keep_alive timeout. The default rmcp timeout - // (5 min) kills idle sessions too aggressively for a local long-running serve. - // We run single-user local, so abandoned sessions cost nothing — let TCP - // liveness determine when a session is truly dead. - let mut session_manager = LocalSessionManager::default(); - session_manager.session_config.keep_alive = None; + // Bound idle MCP session lifetime. The previous "no keep-alive" stance + // assumed TCP liveness would reap dead clients, but a wedged session + // (a request the client already gave up on) keeps its socket and its + // session worker alive indefinitely — observed as active_sessions=4 with + // a single client, each leaked session pinning an rmcp worker task. + // 30 minutes is far beyond any human pause in local interactive use, + // yet guarantees a stuck session — and its FIFO-serialized request + // queue — eventually goes away without a serve restart. + let session_manager = session_manager_with_idle_reap(); let session_manager = Arc::new(session_manager); // Configure the rmcp Streamable HTTP server's DNS-rebinding defence diff --git a/src/serve/tests.rs b/src/serve/tests.rs index 5fd88350..b950ca57 100644 --- a/src/serve/tests.rs +++ b/src/serve/tests.rs @@ -113,6 +113,58 @@ async fn get_chunk_routes_mounted_remote_projects_through_federation() { ); } +#[test] +fn idle_mcp_sessions_are_bounded_to_reap_after_30_minutes() { + // The idle-reap fix is exactly this configuration: keep_alive=None + // assumed TCP liveness would collect wedged sessions, but a stuck + // request keeps its socket and its FIFO-serialized worker alive + // indefinitely (observed active_sessions=4 with a single client). + // The bound must stay a plain half hour — far beyond any human pause in + // local interactive use, yet a guarantee the session eventually goes + // away without a serve restart. + let manager = crate::serve::session_manager_with_idle_reap(); + assert_eq!( + manager.session_config.keep_alive, + Some(std::time::Duration::from_secs( + crate::constants::MCP_IDLE_SESSION_SECS + )), + "the reap bound must be configured on the rmcp session manager" + ); + assert_eq!( + crate::constants::MCP_IDLE_SESSION_SECS, + 30 * 60, + "the reap bound must stay at thirty minutes" + ); +} + +#[test] +fn shutdown_drain_notice_bounds_the_pause_and_forbids_the_reflex_ctrl_c() { + // The notice exists because the ~3 s drain after `q` reads as a hang + // from the restored terminal, and the reflex second Ctrl-C hard-kills + // serve mid-drain (raw mode is already off, so it IS delivered). Every + // element matters: the bounded wait, the session count, and the + // explicit "exits by itself". + let quiet = crate::serve::tui::shutdown_drain_notice(0); + assert_eq!( + quiet, "🛑 Shutting down…", + "no sessions — no drain to explain" + ); + + let draining = crate::serve::tui::shutdown_drain_notice(4); + assert!( + draining.contains("4 open MCP session(s)"), + "the count of draining sessions must be named, got: {draining}" + ); + assert!( + draining.contains("~3 s"), + "the visibly bounded wait is the whole point, got: {draining}" + ); + assert!( + draining.contains("exiting by itself"), + "the user must be told NOT to intervene, got: {draining}" + ); +} + #[test] fn tracked_session_drop_balances_active_sessions() { // A genuine MCP session increments on connect and the serve factory @@ -264,6 +316,120 @@ async fn remove_repo_reports_db_locked_when_delete_fails() { ); } +#[tokio::test] +async fn remove_repo_quarantines_db_dir_when_delete_budget_expires() { + // When the delete budget expires (external holder: another process, an + // AV scanner), the directory used to stay in place as a "valid" index — + // the next registration adopted its stale metadata and chunk stores as a + // false readiness, and re-registration against that legacy data ended in + // arroy EINVAL (the pilot's bare-stamp repos). The quarantine rename + // makes a fresh registration build from scratch; the quarantined sibling + // stays for manual cleanup, same convention as .codesearch.db.bak-*. + let (_tmp, repo_path, state) = state_with_repo("ghostrepo"); + // db_path is a FILE: remove_dir_all fails every retry with a non-lock + // error, deterministically exhausting the budget branch on every OS. + let db_path = repo_path.join(DB_DIR_NAME); + std::fs::write(&db_path, "not a directory").unwrap(); + + let outcome = state + .remove_repo("ghostrepo") + .await + .expect("remove_repo returns Ok(outcome); delete failure is non-fatal"); + + assert!(!outcome.db_deleted); + let quarantine = outcome + .db_quarantined + .expect("a surviving db path must be quarantined, not left in place"); + assert!(!db_path.exists(), "the original db path must be gone"); + assert!(quarantine.exists(), "the quarantined sibling must exist"); + assert!( + quarantine + .file_name() + .unwrap() + .to_string_lossy() + .starts_with(".codesearch.db.removed-"), + "quarantine follows the .removed-* convention: {}", + quarantine.display() + ); +} + +#[tokio::test] +async fn delete_repo_http_mapping_reports_a_quarantined_db_honestly() { + // The DELETE /repos/{alias} payload must not read "removed" when the DB + // dir survived the delete budget: the caller (an operator or an agent + // driving /repos) decides whether the disk still holds a stale index by + // `status` and `db_quarantined`. The quarantine outcome itself is pinned + // by remove_repo_quarantines_db_dir_when_delete_budget_expires; this + // pins its HTTP mapping. + let (_tmp, repo_path, state) = state_with_repo("qmap"); + let state = Arc::new(state); + // A FILE as the db path: remove_dir_all fails every retry with a + // non-lock error, deterministically exhausting the budget into the + // quarantine branch on every OS. + std::fs::write(repo_path.join(DB_DIR_NAME), "not a directory").unwrap(); + + let (status, body) = crate::serve::remove_repo_handler( + axum::extract::Path("qmap".to_string()), + axum::extract::State(state), + ) + .await; + + assert_eq!(status, axum::http::StatusCode::OK, "got body: {body:?}"); + assert_eq!( + body["status"], "removed_db_quarantined", + "the status must name the honest outcome, got: {body:?}" + ); + assert_eq!( + body["db_deleted"], false, + "a quarantined dir was NOT deleted, got: {body:?}" + ); + let quarantine = body["db_quarantined"] + .as_str() + .expect("db_quarantined must be a string path") + .to_string(); + assert!( + quarantine.contains(&format!("{DB_DIR_NAME}.removed-")), + "the payload must point at the quarantined sibling, got: {quarantine}" + ); +} + +#[test] +fn sweep_removes_quarantine_remnants_and_leaves_everything_else() { + // The quarantine rename leaves the survivor "for manual cleanup", and + // manual cleanup never happens — every budget-expired delete leaked a + // full stale index copy forever. The startup sweep is the cleanup: + // by serve start the previous session's external holders are gone, so + // the tombstone has no readers left to protect. + let tmp = tempfile::tempdir().unwrap(); + let repo = tmp.path().join("repo"); + std::fs::create_dir_all(&repo).unwrap(); + + let remnant = repo.join(format!("{DB_DIR_NAME}.removed-1700000000000")); + std::fs::create_dir_all(&remnant).unwrap(); + std::fs::write(remnant.join("metadata.json"), "{}").unwrap(); + let live_db = repo.join(DB_DIR_NAME); + std::fs::create_dir_all(&live_db).unwrap(); + // Similar name, different prefix family — must not be touched. + let other = repo.join("notes.removed-manual"); + std::fs::create_dir_all(&other).unwrap(); + + crate::serve::sweep_quarantined_db_remnants(std::slice::from_ref(&repo)); + + assert!( + !remnant.exists(), + "the .codesearch.db.removed-* sibling must be swept" + ); + assert!(live_db.exists(), "the live db dir must stay untouched"); + assert!( + other.exists(), + "unrelated .removed names must not be touched" + ); + + // A repo path that cannot be listed warns but does not panic. + let missing = tmp.path().join("gone"); + crate::serve::sweep_quarantined_db_remnants(&[missing]); +} + #[test] fn remove_orphaned_db_dir_deletes_a_present_directory() { // Regression guard for the self-cleanup backstop: when a background @@ -3109,6 +3275,87 @@ async fn a_second_format_recovery_for_the_same_alias_is_refused() { ); } +#[tokio::test(flavor = "current_thread", start_paused = true)] +async fn format_recovery_refuses_to_wipe_while_an_index_task_is_alive() { + // recover_repo_format's safety guard: wiping the DB dir from under a + // live indexing task can wedge the task while it holds the store handles + // and the process-wide job permit. The refusal branch ("did not stop + // within the cooperative budget; refusing to wipe") was pinned nowhere. + let (_tmp, repo_path, state) = state_with_repo("liverun"); + let state = Arc::new(state); + let db_path = repo_path.join(DB_DIR_NAME); + std::fs::create_dir_all(&db_path).unwrap(); + std::fs::write(db_path.join("data.mdb"), "fake").unwrap(); + + // A task with no cancellation point: it ignores the token the way a real + // build_index parked inside its synchronous arroy pass does. The sleep is + // virtual — with paused time the 5 s cooperative budget expires the + // instant the timeout is awaited. + let token = CancellationToken::new(); + let handle = tokio::spawn(async move { + tokio::time::sleep(std::time::Duration::from_secs(600)).await; + }); + state.index_tasks.insert( + "liverun".to_string(), + IndexTask { + handle, + token, + db_path: db_path.clone(), + started_at: Instant::now(), + }, + ); + + let result = state.recover_repo_format("liverun").await; + + let err = result.expect_err("a live indexing task must block the wipe"); + assert!( + err.contains("refusing to wipe"), + "the refusal must say what it refused, got: {err}" + ); + assert!( + db_path.exists(), + "the DB dir must survive a refused format-recovery wipe" + ); + assert!( + !state.format_recovery_done.contains_key("liverun"), + "a refused wipe must not tombstone the alias" + ); + + // Cleanup: the parked task must not outlive the test. await_index_task + // put it back into the map ("keeping it tracked"), so abort from there. + if let Some((_, task)) = state.index_tasks.remove("liverun") { + task.handle.abort(); + } +} + +#[tokio::test] +async fn format_recovery_permanent_delete_failure_surfaces_and_stays_retryable() { + // A non-lock delete failure (here: the db path is a regular file) must + // NOT burn the lock-class retry budget, must surface as an explicit + // error, and — because format_recovery_done records at the WIPE, not at + // the rebuild — the alias stays eligible for another attempt instead of + // entering the one-wipe-per-process tombstone on a wipe that never + // happened. + let (_tmp, repo_path, state) = state_with_repo("filerun"); + let state = Arc::new(state); + let db_path = repo_path.join(DB_DIR_NAME); + // A FILE: remove_dir_all fails immediately with a non-lock error on + // every OS, deterministically taking the permanent-failure branch. + std::fs::write(&db_path, "not a directory").unwrap(); + + let result = state.recover_repo_format("filerun").await; + let err = result.expect_err("a permanent delete failure must surface"); + assert!( + err.contains("could not wipe"), + "the error must name the failed wipe, got: {err}" + ); + assert!( + state.enqueue_format_recovery("filerun"), + "a wipe that never happened must not tombstone the alias — the next \ + corruption detection has to be able to retry once the holder is gone" + ); +} + #[tokio::test] #[serial] async fn await_index_task_cancels_and_releases_the_task_handles() { @@ -5011,6 +5258,855 @@ async fn sweeper_reasserts_cancellation_for_a_removed_live_task() { } } +// === Group fan-out chunk-id collision (misattribution) ================= +// +// Chunk ids are per-repo counters, so every repo in a group holds chunk id 0. +// The group tools used to resolve a bare id by probing every store and taking +// the first answer — the alphabetically-first repo won the hit, the true +// definition was filtered out as "wrong kind", and unrelated content rode the +// stolen score. These fixtures seed BOTH repos' only chunk at id 0 so any +// resolution that ignores the origin repo fails deterministically. + +/// Seed `/.codesearch.db` with one chunk (id 0) whose content mentions +/// the search term, then release the writer handle so ServeState can open it. +async fn seed_collision_repo( + root: &std::path::Path, + rel_path: &str, + kind: crate::chunker::ChunkKind, + signature: Option<&str>, + content: &str, +) { + let db_path = root.join(DB_DIR_NAME); + std::fs::create_dir_all(&db_path).unwrap(); + std::fs::write( + db_path.join("metadata.json"), + r#"{"schema_version":1,"dimensions":2,"model_short_name":"minilm-l6-q"}"#, + ) + .unwrap(); + let stores = crate::index::SharedStores::new(&db_path, 2).expect("shared stores"); + { + let mut vs = stores.vector_store.write().await; + let mut chunk = + crate::chunker::Chunk::new(content.to_string(), 0, 4, kind, rel_path.to_string()); + chunk.signature = signature.map(str::to_string); + vs.insert_chunks(vec![crate::embed::EmbeddedChunk::new( + chunk, + vec![0.25, 0.75], + )]) + .expect("insert chunk"); + vs.build_index().expect("build index"); + } + { + let mut fts = stores.fts_store.write().await; + fts.add_chunk(0, content, rel_path, signature, &format!("{:?}", kind)) + .expect("fts doc"); + fts.commit().expect("fts commit"); + } + drop(stores); +} + +/// Two-repo uvz-shaped fixture where both repos hold exactly one chunk at the +/// SAME id 0: `accounting-operations` (alphabetically first — the thief before +/// the fix) has a prose comment mentioning the symbol, `bankruptcy` holds the +/// real Java interface definition. +async fn colliding_group_fixture() -> (tempfile::TempDir, crate::mcp::CodesearchService) { + let (tmp, _state, service) = colliding_group_fixture_with_state().await; + (tmp, service) +} + +/// Same fixture, keeping the `ServeState` handle so tests can reach the very +/// `SharedStores` instances routing opens (`get_opened_stores`) and hold +/// their vector write guards — a fresh `SharedStores::new` would build other +/// RwLocks and the held guard would gate nothing. +async fn colliding_group_fixture_with_state() -> ( + tempfile::TempDir, + std::sync::Arc, + crate::mcp::CodesearchService, +) { + let tmp = tempfile::tempdir().unwrap(); + let root_a = tmp.path().join("accounting-operations"); + let root_b = tmp.path().join("bankruptcy"); + std::fs::create_dir_all(&root_a).unwrap(); + std::fs::create_dir_all(&root_b).unwrap(); + + seed_collision_repo( + &root_a, + "src/hooks/useBankruptcyFolder.ts", + crate::chunker::ChunkKind::Comment, + None, + "// TODO migrate this hook to BankruptcyFolderService from the bankruptcy service", + ) + .await; + seed_collision_repo( + &root_b, + "src/main/java/ru/sberbank/bankruptcy/BankruptcyFolderService.java", + crate::chunker::ChunkKind::Interface, + Some("public interface BankruptcyFolderService"), + "public interface BankruptcyFolderService { BankruptcyFolder folderFor(String inn); }", + ) + .await; + + let mut config = ReposConfig::default(); + config + .register_with_alias(root_a.clone(), Some("accounting-operations".to_string())) + .unwrap(); + config + .register_with_alias(root_b.clone(), Some("bankruptcy".to_string())) + .unwrap(); + config.groups.insert( + "uvz".to_string(), + vec![ + "accounting-operations".to_string(), + "bankruptcy".to_string(), + ], + ); + let config_file = tmp.path().join("repos.json"); + config.save_to(&config_file).unwrap(); + let state = std::sync::Arc::new(ServeState::new(config, Some(config_file))); + let service = crate::mcp::CodesearchService::new_for_serve(state.clone()).unwrap(); + (tmp, state, service) +} + +fn tool_text(res: &rmcp::model::CallToolResult) -> String { + match res.content.first() { + Some(rmcp::model::ContentBlock::Text(t)) => t.text.clone(), + other => panic!("expected text content, got {other:?}"), + } +} + +#[tokio::test] +async fn group_find_definition_attributes_hit_to_origin_repo_on_id_collision() { + use rmcp::handler::server::wrapper::Parameters; + + let (_tmp, service) = colliding_group_fixture().await; + + let request = crate::mcp::types::FindRequest { + kind: Some("definition".to_string()), + symbol: "BankruptcyFolderService".to_string(), + definition_kind: None, + limit: Some(10), + project: None, + group: Some("uvz".to_string()), + }; + let res = service + .find(Parameters(request)) + .await + .expect("group find must succeed"); + let text = tool_text(&res); + + // Before the fix this was the empty-result arm ("No definition found"): + // both hits resolved in accounting-operations' comment chunk, failed the + // DEFINITION_KINDS filter, and the true interface never surfaced. + let parsed: serde_json::Value = + serde_json::from_str(&text).unwrap_or_else(|_| panic!("non-JSON answer: {text}")); + let items = parsed + .as_array() + .unwrap_or_else(|| panic!("expected a bare result array, got: {text}")); + assert_eq!( + items.len(), + 1, + "exactly the interface definition, got: {text}" + ); + assert_eq!( + items[0]["path"], + "bankruptcy/src/main/java/ru/sberbank/bankruptcy/BankruptcyFolderService.java", + "the hit must carry its origin repo's alias prefix: {text}" + ); + assert_eq!(items[0]["kind"], "Interface", "got: {text}"); + assert_eq!( + items[0]["line"], 1, + "responses must emit 1-based editor lines (chunk starts at 0-based line 0): {text}" + ); +} + +/// Group-scoped get_chunk must read surrounding lines from the OWNING repo's +/// root. Stored chunk paths are project-relative, and the old context read +/// joined them onto the serve process's own project_path after alias +/// prefixing — a path that never exists — so every group answer came back +/// with the "source file not readable" note and zero context. Here chunk id 5 +/// lives only in accounting-operations (bankruptcy holds just id 0), so the +/// smart-candidate detection routes to the unique owner. +#[tokio::test] +async fn group_get_chunk_resolves_context_against_the_owning_repo_root() { + use rmcp::handler::server::wrapper::Parameters; + + let tmp = tempfile::tempdir().unwrap(); + let root_a = tmp.path().join("accounting-operations"); + let root_b = tmp.path().join("bankruptcy"); + std::fs::create_dir_all(root_a.join("src/service")).unwrap(); + std::fs::create_dir_all(&root_b).unwrap(); + + // The real source file the context read has to find. + std::fs::write( + root_a.join("src/service/TargetService.java"), + "line one\nline two\nTARGET LINE THREE\nTARGET LINE FOUR\nTARGET LINE FIVE\nline six\nline seven\nline eight\n", + ) + .unwrap(); + + // Repo A: five filler chunks (ids 0..=4) then the target at id 5, + // spanning 0-based lines [2, 4] of the file above. + let db_a = root_a.join(DB_DIR_NAME); + std::fs::create_dir_all(&db_a).unwrap(); + std::fs::write( + db_a.join("metadata.json"), + r#"{"schema_version":1,"dimensions":2,"model_short_name":"minilm-l6-q"}"#, + ) + .unwrap(); + let stores = crate::index::SharedStores::new(&db_a, 2).expect("shared stores"); + { + let mut vs = stores.vector_store.write().await; + for i in 0..5u32 { + vs.insert_chunks(vec![crate::embed::EmbeddedChunk::new( + crate::chunker::Chunk::new( + format!("filler {i}"), + 0, + 0, + crate::chunker::ChunkKind::Comment, + "src/Filler.java".to_string(), + ), + vec![0.25, 0.75], + )]) + .expect("insert filler"); + } + let mut target = crate::chunker::Chunk::new( + "TARGET LINE THREE\nTARGET LINE FOUR\nTARGET LINE FIVE".to_string(), + 2, + 4, + crate::chunker::ChunkKind::Function, + "src/service/TargetService.java".to_string(), + ); + target.signature = Some("fn target()".to_string()); + vs.insert_chunks(vec![crate::embed::EmbeddedChunk::new( + target, + vec![0.5, 0.5], + )]) + .expect("insert target"); + vs.build_index().expect("build index"); + } + { + let mut fts = stores.fts_store.write().await; + fts.add_chunk( + 5, + "target", + "src/service/TargetService.java", + Some("fn target()"), + "Function", + ) + .expect("fts doc"); + fts.commit().expect("fts commit"); + } + drop(stores); + + // Repo B holds only id 0, so chunk id 5 is unique to repo A. + seed_collision_repo( + &root_b, + "src/B.java", + crate::chunker::ChunkKind::Comment, + None, + "// b", + ) + .await; + + let mut config = ReposConfig::default(); + config + .register_with_alias(root_a.clone(), Some("accounting-operations".to_string())) + .unwrap(); + config + .register_with_alias(root_b.clone(), Some("bankruptcy".to_string())) + .unwrap(); + config.groups.insert( + "uvz".to_string(), + vec![ + "accounting-operations".to_string(), + "bankruptcy".to_string(), + ], + ); + let config_file = tmp.path().join("repos.json"); + config.save_to(&config_file).unwrap(); + let state = std::sync::Arc::new(ServeState::new(config, Some(config_file))); + let service = crate::mcp::CodesearchService::new_for_serve(state).unwrap(); + + let req = crate::mcp::types::GetChunkRequest { + chunk_id: 5, + chunk_ref: None, + context_lines: Some(2), + project: None, + group: Some("uvz".to_string()), + }; + let res = service + .get_chunk(Parameters(req)) + .await + .expect("group get_chunk must succeed"); + let text = tool_text(&res); + let parsed: serde_json::Value = + serde_json::from_str(&text).unwrap_or_else(|_| panic!("non-JSON answer: {text}")); + + assert_eq!( + parsed["path"], "accounting-operations/src/service/TargetService.java", + "got: {text}" + ); + assert_eq!( + parsed["context_before"], "line one\nline two", + "context above must come from the owning repo's real file: {text}" + ); + assert_eq!( + parsed["context_after"], "TARGET LINE FIVE\nline six", + "context below must come from the owning repo's real file: {text}" + ); + assert!( + parsed.get("note").is_none(), + "the context read resolved — no fallback note expected, got: {text}" + ); +} + +// ═══ Interactive fan-outs skip busy stores instead of queueing behind the +// write lock (review cluster B). The priming call opens the group's stores +// so `get_opened_stores` hands back the SAME SharedStores the handlers +// resolve; holding its vector write guard then reproduces an active indexing +// run. A regression to bounded waits wedges each test on the 300 s lock +// timeout instead of returning the skip-with-warning answer asserted here. + +#[tokio::test] +async fn group_find_skips_a_write_locked_store_instead_of_waiting() { + use rmcp::handler::server::wrapper::Parameters; + + let (_tmp, state, service) = colliding_group_fixture_with_state().await; + + // Prime routing so the group's stores are opened and lockable. + let prime = crate::mcp::types::FindRequest { + kind: Some("definition".to_string()), + symbol: "BankruptcyFolderService".to_string(), + definition_kind: None, + limit: Some(10), + project: None, + group: Some("uvz".to_string()), + }; + service + .find(Parameters(prime)) + .await + .expect("priming find must succeed"); + + let alpha = state + .get_opened_stores("accounting-operations") + .expect("priming call must open accounting-operations"); + let _guard = alpha.vector_store.write().await; + + let request = crate::mcp::types::FindRequest { + kind: Some("definition".to_string()), + symbol: "BankruptcyFolderService".to_string(), + definition_kind: None, + limit: Some(10), + project: None, + group: Some("uvz".to_string()), + }; + let res = service + .find(Parameters(request)) + .await + .expect("group find must succeed without waiting on the locked store"); + let text = tool_text(&res); + + // The healthy repo still answers; the busy one is named in `warnings`, + // not silently folded into a plausible-looking short result list. + let parsed: serde_json::Value = + serde_json::from_str(&text).unwrap_or_else(|_| panic!("non-JSON answer: {text}")); + let items = parsed + .as_object() + .and_then(|o| o.get("results")) + .and_then(|r| r.as_array()) + .unwrap_or_else(|| panic!("expected a {{results, warnings}} object, got: {text}")); + assert_eq!( + items.len(), + 1, + "only the bankruptcy definition must be reported, got: {text}" + ); + assert_eq!( + items[0]["path"], + "bankruptcy/src/main/java/ru/sberbank/bankruptcy/BankruptcyFolderService.java", + "got: {text}" + ); + assert!( + text.contains("store busy") && text.contains("accounting-operations"), + "the skipped repo must be named with a store-busy warning, got: {text}" + ); +} + +#[tokio::test] +async fn group_get_chunk_skips_a_write_locked_candidate_repo() { + use rmcp::handler::server::wrapper::Parameters; + + let (_tmp, state, service) = colliding_group_fixture_with_state().await; + + // Prime routing so the group's stores are opened and lockable. + let prime = crate::mcp::types::FindRequest { + kind: Some("definition".to_string()), + symbol: "BankruptcyFolderService".to_string(), + definition_kind: None, + limit: Some(10), + project: None, + group: Some("uvz".to_string()), + }; + service + .find(Parameters(prime)) + .await + .expect("priming find must succeed"); + + let alpha = state + .get_opened_stores("accounting-operations") + .expect("priming call must open accounting-operations"); + let _guard = alpha.vector_store.write().await; + + // Both repos hold id 0; with accounting-operations locked, the candidate + // scan must skip it (not wait) and auto-route to bankruptcy alone. + let req = crate::mcp::types::GetChunkRequest { + chunk_id: 0, + chunk_ref: None, + context_lines: None, + project: None, + group: Some("uvz".to_string()), + }; + let res = service + .get_chunk(Parameters(req)) + .await + .expect("group get_chunk must succeed without waiting on the locked store"); + let text = tool_text(&res); + let parsed: serde_json::Value = + serde_json::from_str(&text).unwrap_or_else(|_| panic!("non-JSON answer: {text}")); + + assert_eq!( + parsed["path"], + "bankruptcy/src/main/java/ru/sberbank/bankruptcy/BankruptcyFolderService.java", + "the only answering candidate must own the answer, got: {text}" + ); + assert!( + text.contains("store busy") && text.contains("accounting-operations"), + "the skipped candidate must be named with a store-busy warning, got: {text}" + ); +} + +#[tokio::test] +async fn group_similar_skips_a_write_locked_store_instead_of_waiting() { + use rmcp::handler::server::wrapper::Parameters; + + let (_tmp, state, service) = colliding_group_fixture_with_state().await; + + // Prime routing so the group's stores are opened and lockable; the + // unprimed answer would be an id-0 ambiguity report, which is fine — + // only the side effect (stores opened) matters here. + let prime = crate::mcp::types::ExploreRequest { + kind: Some("similar".to_string()), + target: "0".to_string(), + limit: Some(5), + project: None, + group: Some("uvz".to_string()), + }; + service + .explore(Parameters(prime)) + .await + .expect("priming explore must succeed"); + + let alpha = state + .get_opened_stores("accounting-operations") + .expect("priming call must open accounting-operations"); + let _guard = alpha.vector_store.write().await; + + // Covers BOTH converted fan-out sites of similar_chunks in one call: + // the embedding lookup and the neighbour search both try-lock alpha. + let req = crate::mcp::types::SimilarChunksRequest { + chunk_id: 0, + limit: Some(5), + project: None, + group: Some("uvz".to_string()), + }; + let res = service + .similar_chunks(Parameters(req)) + .await + .expect("group similar must succeed without waiting on the locked store"); + let text = tool_text(&res); + + // The embedding resolves against bankruptcy alone (unique holder), and + // its neighbour set is just the source chunk itself — filtered out — so + // the answer is the empty message PLUS the store-busy warnings naming + // the skipped repo. + assert!( + text.contains("No similar chunks found for chunk_id 0"), + "expected the empty-result answer from the sole answering repo, got: {text}" + ); + assert!( + text.contains("store busy") && text.contains("accounting-operations"), + "the skipped repo must be named with a store-busy warning, got: {text}" + ); +} + +#[tokio::test] +async fn group_literal_search_attributes_each_hit_to_its_origin_repo() { + use rmcp::handler::server::wrapper::Parameters; + + let (_tmp, service) = colliding_group_fixture().await; + + let request = crate::mcp::types::LiteralSearchRequest { + query: "BankruptcyFolderService".to_string(), + regex: None, + phrase: None, + limit: Some(10), + file_glob: None, + language: None, + format: None, + project: None, + group: Some("uvz".to_string()), + }; + let res = service + .literal_search(Parameters(request)) + .await + .expect("group literal search must succeed"); + let text = tool_text(&res); + + // Before the fix BOTH hits resolved against accounting-operations' chunk + // (first store answering id 0): two identical hook paths, the Java file's + // content lost. After the fix each hit resolves in its own repo and every + // path carries its origin alias. + let parsed: serde_json::Value = + serde_json::from_str(&text).unwrap_or_else(|_| panic!("non-JSON answer: {text}")); + let items = parsed["results"] + .as_array() + .unwrap_or_else(|| panic!("expected a results array, got: {text}")) + .clone(); + assert_eq!(items.len(), 2, "both repos must answer, got: {text}"); + + let mut paths: Vec = items + .iter() + .map(|i| i["path"].as_str().unwrap_or_default().to_string()) + .collect(); + paths.sort(); + assert_eq!( + paths, + vec![ + "accounting-operations/src/hooks/useBankruptcyFolder.ts".to_string(), + "bankruptcy/src/main/java/ru/sberbank/bankruptcy/BankruptcyFolderService.java" + .to_string(), + ], + "each hit must keep its own repo's path and alias prefix: {text}" + ); + + // Snippet attribution: the prefixed java hit must be the interface content, + // not the accounting comment that stole it before the fix. + let java = items + .iter() + .find(|i| i["path"].as_str().unwrap_or("").starts_with("bankruptcy/")) + .expect("java hit present"); + assert!( + java["snippet"] + .as_str() + .unwrap_or_default() + .contains("public interface BankruptcyFolderService"), + "the bankruptcy hit must carry bankruptcy's own content: {text}" + ); +} + +/// Exact-AND literal search returns silence for multi-word queries whose +/// relevant chunks hold only MOST of the terms. The relaxed fallback must +/// surface those partial hits (>=60% significant-term coverage, >=2 terms) +/// instead of an empty answer, flag `relaxed_fallback` and explain itself +/// in the note — while single-identifier queries keep their honest refusal. +#[tokio::test] +async fn group_literal_search_relaxed_fallback_covers_partial_term_hits() { + use rmcp::handler::server::wrapper::Parameters; + + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path().join("partial"); + std::fs::create_dir_all(&root).unwrap(); + { + let db_path = root.join(DB_DIR_NAME); + std::fs::create_dir_all(&db_path).unwrap(); + std::fs::write( + db_path.join("metadata.json"), + r#"{"schema_version":1,"dimensions":2,"model_short_name":"minilm-l6-q"}"#, + ) + .unwrap(); + let stores = crate::index::SharedStores::new(&db_path, 2).expect("shared stores"); + { + let mut vs = stores.vector_store.write().await; + let rich = crate::chunker::Chunk::new( + "create package documents quickly".to_string(), + 0, + 3, + crate::chunker::ChunkKind::Comment, + "docs/create.md".to_string(), + ); + let poor = crate::chunker::Chunk::new( + "package".to_string(), + 0, + 1, + crate::chunker::ChunkKind::Comment, + "docs/other.md".to_string(), + ); + vs.insert_chunks(vec![ + crate::embed::EmbeddedChunk::new(rich, vec![0.25, 0.75]), + crate::embed::EmbeddedChunk::new(poor, vec![0.5, 0.5]), + ]) + .expect("insert chunks"); + vs.build_index().expect("build index"); + } + { + let mut fts = stores.fts_store.write().await; + fts.add_chunk( + 0, + "create package documents quickly", + "docs/create.md", + None, + "Comment", + ) + .expect("fts doc"); + fts.add_chunk(1, "package", "docs/other.md", None, "Comment") + .expect("fts doc"); + fts.commit().expect("fts commit"); + } + drop(stores); + } + + let mut config = ReposConfig::default(); + config + .register_with_alias(root.clone(), Some("partial".to_string())) + .unwrap(); + config + .groups + .insert("solo".to_string(), vec!["partial".to_string()]); + let config_file = tmp.path().join("repos.json"); + config.save_to(&config_file).unwrap(); + let state = std::sync::Arc::new(ServeState::new(config, Some(config_file))); + let service = crate::mcp::CodesearchService::new_for_serve(state).unwrap(); + + // Four significant terms; no chunk holds all four, so exact AND is empty. + // The rich chunk covers 3/4 (>=60%), the poor one 1/4 (<2 terms). + let request = crate::mcp::types::LiteralSearchRequest { + query: "create package documents signature".to_string(), + regex: None, + phrase: None, + limit: Some(10), + file_glob: None, + language: None, + format: None, + project: None, + group: Some("solo".to_string()), + }; + let res = service + .literal_search(Parameters(request)) + .await + .expect("relaxed literal search must succeed"); + let text = tool_text(&res); + let parsed: serde_json::Value = + serde_json::from_str(&text).unwrap_or_else(|_| panic!("non-JSON answer: {text}")); + let items = parsed["results"] + .as_array() + .cloned() + .unwrap_or_else(|| panic!("expected a results array, got: {text}")); + assert_eq!( + items.len(), + 1, + "only the >=60%-coverage chunk survives the relaxed gate: {text}" + ); + assert_eq!(items[0]["path"], "partial/docs/create.md", "got: {text}"); + assert_eq!( + items[0]["start_line"], 1, + "responses must emit 1-based editor lines (chunk starts at 0-based line 0): {text}" + ); + assert_eq!( + parsed["relaxed_fallback"], + serde_json::json!(true), + "the fallback must be visible to the caller: {text}" + ); + assert!( + parsed["note"] + .as_str() + .unwrap_or_default() + .contains("relaxed"), + "the note must explain the mode: {text}" + ); + + // A single-identifier query never relaxes: the empty exact answer stays + // an honest refusal, not a wall of partial matches. + let request = crate::mcp::types::LiteralSearchRequest { + query: "QzMissingUvzBusinessSymbol99Xy".to_string(), + regex: None, + phrase: None, + limit: Some(10), + file_glob: None, + language: None, + format: None, + project: None, + group: Some("solo".to_string()), + }; + let res = service + .literal_search(Parameters(request)) + .await + .expect("refusal literal search must succeed"); + let text = tool_text(&res); + let parsed: serde_json::Value = + serde_json::from_str(&text).unwrap_or_else(|_| panic!("non-JSON answer: {text}")); + assert!( + parsed["results"] + .as_array() + .map(Vec::is_empty) + .unwrap_or(true), + "an unknown identifier must stay empty: {text}" + ); + assert!( + parsed.get("relaxed_fallback").is_none(), + "single-term queries must not trigger the fallback: {text}" + ); +} + +/// `min_score` lets a caller turn the nearest-neighbour firehose into an +/// honest refusal: hits below the threshold are dropped and the empty +/// answer explains itself in `note`, instead of presenting irrelevant +/// neighbours as if they were matches. Lexical mode rides the same +/// `build_semantic_response` funnel as the embedding-backed modes, so it +/// exercises the threshold without needing a real embedding model. +#[tokio::test] +async fn group_semantic_search_min_score_refuses_instead_of_returning_noise() { + use rmcp::handler::server::wrapper::Parameters; + + let (_tmp, service) = colliding_group_fixture().await; + + let make_request = |min_score: Option| crate::mcp::types::SearchRequest { + query: "BankruptcyFolderService".to_string(), + mode: None, + compact: None, + semantic_mode: Some("lexical".to_string()), + filter_path: None, + min_score, + regex: None, + phrase: None, + file_glob: None, + language: None, + format: None, + limit: Some(10), + project: None, + group: Some("uvz".to_string()), + }; + + // An unreachable threshold must refuse with an explanation, not return + // the (weakly scored) candidates it found. + let res = service + .search(Parameters(make_request(Some(10_000.0)))) + .await + .expect("min_score search must succeed"); + let text = tool_text(&res); + let parsed: serde_json::Value = + serde_json::from_str(&text).unwrap_or_else(|_| panic!("non-JSON answer: {text}")); + assert!( + parsed["results"] + .as_array() + .map(Vec::is_empty) + .unwrap_or(false), + "an unreachable threshold must yield an explicit refusal: {text}" + ); + assert_eq!( + parsed["low_confidence"], + serde_json::json!(true), + "a refusal is a low-confidence outcome: {text}" + ); + assert!( + parsed["note"] + .as_str() + .unwrap_or_default() + .contains("min_score"), + "the refusal must name the threshold so the caller can retune it: {text}" + ); + + // Without the threshold the same query must still answer with hits: + // `min_score` adds a refusal path, never a silent filter. + let res = service + .search(Parameters(make_request(None))) + .await + .expect("unfiltered search must succeed"); + let text = tool_text(&res); + let parsed: serde_json::Value = + serde_json::from_str(&text).unwrap_or_else(|_| panic!("non-JSON answer: {text}")); + assert!( + !parsed["results"] + .as_array() + .cloned() + .unwrap_or_default() + .is_empty(), + "the same query without a threshold must return its hits: {text}" + ); +} + +/// Regression for the pilot's "ghost loss" of freshly registered aliases. +/// +/// The debounced persist clones its snapshot and writes the file WITHOUT the +/// config lock, so a registration landing in that window used to exist only +/// in memory while the stale snapshot hit disk. Every reader-side +/// `reload_if_changed` (warmup / aliases / info / group resolve) then saw the +/// fresh mtime, trusted "the file changed", and swapped the stale disk copy +/// into memory — silently dropping the registration (alias 404s in /info, its +/// build ending with "repo removed or cancelled mid-index"). +/// +/// Two guards fix it: `persist_config` adopts its own write's mtime so an +/// internal persist never masquerades as an external edit, and +/// `run_persist_pass` re-verifies memory against the snapshot and rewrites +/// until disk converges. +#[tokio::test] +async fn own_persist_never_clobbers_a_racing_registration() { + let tmp = tempfile::tempdir().unwrap(); + let root_a = tmp.path().join("alpha-repo"); + let root_b = tmp.path().join("beta-repo"); + std::fs::create_dir_all(&root_a).unwrap(); + std::fs::create_dir_all(&root_b).unwrap(); + + let mut config = ReposConfig::default(); + config + .register_with_alias(root_a.clone(), Some("alpha".to_string())) + .unwrap(); + let config_file = tmp.path().join("repos.json"); + config.save_to(&config_file).unwrap(); + let state = std::sync::Arc::new(ServeState::new(config, Some(config_file.clone()))); + + // Warm the mtime cache exactly as production does after its first reader + // passes (ServeState starts with a None mtime; this reload adopts it). + let _ = state.aliases(); + + // The debounce worker clones its snapshot... + let stale = state.config.read().unwrap().clone(); + + // ...and a registration lands + persists while the worker is mid-write. + { + let mut cfg = state.config.write().unwrap(); + cfg.register_with_alias(root_b.clone(), Some("beta".to_string())) + .unwrap(); + } + let current = state.config.read().unwrap().clone(); + state.persist_config(¤t).unwrap(); + + // The worker's stale snapshot finally hits the file — on disk, beta is gone. + state.persist_config(&stale).unwrap(); + let disk = ReposConfig::load_from(&config_file).unwrap(); + assert!( + !disk.repos.contains_key("beta"), + "fixture premise: the stale snapshot must have won the disk race" + ); + + // Before the fix this reader-side reload saw the fresh mtime and clobbered + // the in-memory registration with the disk copy. + let aliases = state.aliases(); + assert!( + aliases.iter().any(|a| a == "beta"), + "the in-memory registration must survive the worker's stale write" + ); + assert!( + state.config_snapshot().repos.contains_key("beta"), + "beta must still resolve from the live config" + ); + + // The divergence check converges disk back to memory. + state.run_persist_pass().await; + let disk = ReposConfig::load_from(&config_file).unwrap(); + assert!( + disk.repos.contains_key("beta"), + "run_persist_pass must rewrite the racing registration to disk" + ); +} + // ── status snapshot ────────────────────────────────────────────────────── #[test] diff --git a/src/serve/tui.rs b/src/serve/tui.rs index 8310da1f..a2ad516a 100644 --- a/src/serve/tui.rs +++ b/src/serve/tui.rs @@ -59,12 +59,38 @@ pub async fn run_tui( // Run main loop. Errors (e.g. from terminal.draw) propagate up // and are caught below to ensure restoration. + let session_probe = Arc::clone(&state); let result = run_tui_loop(&mut terminal, state, cancel_token, &serve_url).await; // Always restore terminal, even on error tui_common::restore_terminal(&mut terminal)?; - result + // After `q` the serve keeps draining open MCP sessions for up to ~3 s before + // force-exiting — from the terminal this pause looks like a hang, so say so. + let user_quit = result.as_ref().is_ok_and(|quit| *quit); + if user_quit { + println!( + "{}", + shutdown_drain_notice(session_probe.active_session_count()) + ); + } + + result.map(|_| ()) +} + +/// The post-`q` shutdown notice. Extracted so its contract is pinned by a +/// test: name the bounded wait (~3 s), count the draining sessions, and say +/// the process exits by itself — the notice exists precisely to stop the +/// reflex second Ctrl-C that hard-kills serve mid-drain once raw mode is off. +pub(crate) fn shutdown_drain_notice(active_sessions: u64) -> String { + if active_sessions == 0 { + "🛑 Shutting down…".to_string() + } else { + format!( + "🛑 Shutting down: waiting up to ~3 s for {active_sessions} open MCP session(s) to \ + drain, then exiting by itself — no Ctrl-C needed." + ) + } } async fn run_tui_loop( @@ -72,10 +98,11 @@ async fn run_tui_loop( state: Arc, cancel_token: CancellationToken, serve_url: &str, -) -> io::Result<()> { +) -> io::Result { // TUI-local state let mut table_state = ratatui::widgets::TableState::default(); table_state.select(Some(0)); + let mut user_quit = false; let tick_interval = Duration::from_millis(500); let poll_timeout = Duration::from_millis(100); @@ -409,6 +436,7 @@ async fn run_tui_loop( if should_quit { // User pressed q — signal shutdown to the whole serve process + user_quit = true; cancel_token.cancel(); break; } @@ -420,7 +448,7 @@ async fn run_tui_loop( tokio::time::sleep(tick_interval).await; } - Ok(()) + Ok(user_quit) } // --------------------------------------------------------------------------- diff --git a/src/vectordb/mod.rs b/src/vectordb/mod.rs index 330021bb..be500ad5 100644 --- a/src/vectordb/mod.rs +++ b/src/vectordb/mod.rs @@ -3,3 +3,8 @@ mod store; pub use store::{ merge_metadata_atomic, ChunkMetadata, SearchResult, StoreStats, VectorStore, PATH_REWRITE_BATCH, }; + +// Crash-atomic JSON writers outside this module (`FileMetaStore::save`) +// reuse the same transient-rename classification and retry budget instead +// of duplicating the raw-error-code list per call site. +pub(crate) use store::{is_transient_rename_error, RENAME_RETRY_ATTEMPTS, RENAME_RETRY_DELAY_MS}; diff --git a/src/vectordb/store.rs b/src/vectordb/store.rs index 58f51370..d328f806 100644 --- a/src/vectordb/store.rs +++ b/src/vectordb/store.rs @@ -120,7 +120,10 @@ fn read_metadata_u32(db_path: &Path, key: &str) -> Option { /// fail with "Access is denied" purely from timing, not a real conflict — /// most visible under `cargo test --lib --bins` parallel load. Unix renames /// are atomic replace and never hit this path, so the retry is a no-op there. -fn is_transient_rename_error(e: &std::io::Error) -> bool { +/// +/// `pub(crate)` so `FileMetaStore::save` can share the same classification +/// for its rename step instead of growing a third copy of the raw-code list. +pub(crate) fn is_transient_rename_error(e: &std::io::Error) -> bool { if let Some(raw) = e.raw_os_error() { if matches!(raw, 5 | 32 | 33) { return true; @@ -132,9 +135,10 @@ fn is_transient_rename_error(e: &std::io::Error) -> bool { /// Bounded retry budget for the rename step below: short, since a genuine /// conflict (not a transient handle) should surface quickly rather than -/// stall the caller. -const RENAME_RETRY_ATTEMPTS: u32 = 5; -const RENAME_RETRY_DELAY_MS: u64 = 20; +/// stall the caller. Shared with `FileMetaStore::save`, which mirrors this +/// tmp+fsync+rename pattern. +pub(crate) const RENAME_RETRY_ATTEMPTS: u32 = 5; +pub(crate) const RENAME_RETRY_DELAY_MS: u64 = 20; fn atomic_write_json(path: &Path, json: &serde_json::Value) -> Result<()> { use std::io::Write; @@ -836,13 +840,24 @@ impl VectorStore { )); } + let rtxn = self.env.read_txn()?; + if !self.indexed { + // An indexing run that ends with zero embeddable files never + // builds the arroy tree; such repos are normal (packaging junk, + // docs-only trees), so an empty store must answer "no hits", not + // an error — otherwise every group fan-out reported each empty + // repo as a per-repo failure on every query. Chunks present + // without a graph stays a loud error: that is data missing its + // index, not an empty repo. + if self.chunks.last(&rtxn)?.is_none() { + return Ok(Vec::new()); + } return Err(anyhow!( "Index not built. Call build_index() after inserting chunks." )); } - let rtxn = self.env.read_txn()?; let reader = Reader::open(&rtxn, 0, self.vectors)?; // Perform ANN search with quality boost @@ -1499,6 +1514,49 @@ mod tests { ); } + /// Empty-but-complete: a store whose indexing ended with zero chunks + /// never built the arroy tree. Such a repo is normal (packaging junk, + /// docs-only trees) and must answer queries with an empty vec — the old + /// hard error made every group fan-out report each empty repo as a + /// per-repo failure on every query. + #[test] + fn search_on_a_chunkless_store_returns_empty_instead_of_an_error() { + let temp_dir = tempdir().unwrap(); + let db_path = temp_dir.path().join("empty.db"); + let store = VectorStore::new(&db_path, 4).unwrap(); + assert!( + !store.is_indexed(), + "fixture: nothing was ever inserted or built" + ); + + let hits = store + .search(&[0.1, 0.2, 0.3, 0.4], 10) + .expect("an empty store is searchable, it just finds nothing"); + assert!(hits.is_empty()); + } + + /// Chunks without a graph is a genuinely broken state (crashed build, + /// partial index) and must stay loud — silently empty would hide data + /// loss behind what looks like "no match in this repo". + #[test] + fn search_with_chunks_but_no_graph_still_errors() { + let temp_dir = tempdir().unwrap(); + let db_path = temp_dir.path().join("unbuilt.db"); + let mut store = VectorStore::new(&db_path, 4).unwrap(); + store + .insert_chunks_with_ids(vec![drift_chunk("src/a.rs", "fn a() {}", 0)]) + .expect("insert"); + assert!(!store.is_indexed()); + + let err = store + .search(&[0.1, 0.2, 0.3, 0.4], 10) + .expect_err("chunks present without a graph must fail loudly"); + assert!( + format!("{err:#}").contains("Index not built"), + "got: {err:#}" + ); + } + #[test] fn test_vector_store_creation() { let temp_dir = tempdir().unwrap(); diff --git a/src/watch/mod.rs b/src/watch/mod.rs index eeb5b8d7..96068dfc 100644 --- a/src/watch/mod.rs +++ b/src/watch/mod.rs @@ -689,14 +689,24 @@ mod tests { Ok(()) } - /// Build a `FileWatcher` with the home directory pointed at an empty - /// temporary directory, so the developer's real - /// `~/.codesearch/.codesearchignore` cannot leak into the assertions. + /// Build a `FileWatcher` with the home directory AND `CODESEARCH_HOME` + /// pointed at empty temporary directories, so the developer's real global + /// `.codesearchignore` (from either root) cannot leak into the assertions. /// CI has no such file, which is why the ambient dependency went unnoticed. fn watcher_with_isolated_home(root: PathBuf) -> FileWatcher { - let home = tempdir().unwrap(); - let home = home.path().to_string_lossy().into_owned(); - let _guard = EnvRestore::set(&[("HOME", &home), ("USERPROFILE", &home)]); + let scratch = tempdir().unwrap(); + let bare_home = scratch.path().join("bare-home"); + let cs_home = scratch.path().join("cs-home"); + fs::create_dir_all(&bare_home).unwrap(); + fs::create_dir_all(&cs_home).unwrap(); + let _guard = EnvRestore::set(&[ + ( + crate::constants::CODESEARCH_HOME_ENV, + cs_home.to_str().unwrap(), + ), + ("HOME", bare_home.to_str().unwrap()), + ("USERPROFILE", bare_home.to_str().unwrap()), + ]); FileWatcher::new(root) }