diff --git a/AGENTS.develop.md b/AGENTS.develop.md index 6f7147e4..80b65e56 100644 --- a/AGENTS.develop.md +++ b/AGENTS.develop.md @@ -241,6 +241,7 @@ Never `unwrap_or_default()` a store error on a search path — "no results" and ## Changelog highlights (recent) +- **v1.5.33 (pending)** — FTS commit-failure hardening: per-file transient redrive, crash-safe vector rollback, mixed-generation sweep in refresh, stale note on get_chunk - **v1.5.33 (pending)** — `.codesearchinclude` whitelist: when present (repo root or `~/.codesearch/`), only matching files are indexed; ignore rules still win, absent list = historical behavior - **v1.5.33 (pending)** — MCP tools declare `readOnlyHint` annotations: Claude Code plan mode auto-allows all six tools instead of prompting every call - **v1.5.33 (pending)** — force reindex (`n`, `index -f`) rebuilds the SCIP symbol indexes too — shared `finish_force_reindex` tail for HTTP + TUI; `?symbols=true` stays symbol-only, plain reindex stays gate-driven diff --git a/CHANGELOG.md b/CHANGELOG.md index 4b8dd42a..5b7d38b9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,8 @@ finalized in place with a date — no renaming/migration step needed. ### Fixed +- **A failed FTS commit no longer leaves the index in mixed generations.** On Windows an indexer/AV filter holding a freshly created tantivy segment file makes the FTS commit fail (os error 5) and the writer is discarded by design — but the per-file incremental paths inserted the new chunks into the vector store first with no rollback, so the failed attempt's ids were orphaned (never in file_meta) while the tracked generation was already deleted: search (FTS) and get_chunk (vector) then disagreed and stale lines kept coming back (the HUSQ.Aprimo worktree symptom). Three layers now: the per-file paths redrive on the transient race like force reindex already did; a failed publish rolls the fresh chunk ids back out of both stores (file_meta stays old, so the next pass heals clean); and the refresh sweep deletes untracked vector chunks and re-indexes live files whose tracked ids went missing, so already-corrupted indexes heal on the next refresh without a force reindex. get_chunk also warns when the source file is newer than the last indexing write. The fts/ subdir is now marked hidden on Windows too. + - **MCP tools now declare `readOnlyHint` annotations, so Claude Code plan mode auto-allows them.** None of the six tools (`search`, `find`, `explore`, `get_chunk`, `status`, `find_impact`) set rmcp `annotations`, and plan mode only auto-allows MCP tools that declare `readOnlyHint: true` — every codesearch call in plan mode was blocked or prompted (in `-p` mode it failed outright), in the main session and in subagents alike. All six are read-only index queries; they now declare `read_only_hint = true, destructive_hint = false, idempotent_hint = true, open_world_hint = false`, and the 6-tool registration test pins the hint so a future tool cannot ship without it. - **A force reindex now rebuilds the SCIP symbol indexes too.** `n` in the serve TUI and `codesearch index -f` promised "rebuild everything" but dated from before the symbol indexes existed: the HTTP handler forced the symbol pass only behind the separate `?symbols=true` flag (so a fresh SCIP gate — no `.cs` changes — skipped it), and the TUI's force reindex never ran a symbol pass at all. Field effect: after Razor joined the text index, `find_impact` kept answering from ~19-hour-old references. Force now implies symbols (`finish_force_reindex` is the shared tail of the HTTP and TUI force paths: fresh FSW, then a forced, gate-bypassing symbol pass); `?symbols=true` stays the explicit no-force symbol-only trigger, and a plain reindex stays gate-driven. Local CLI builds (`index -f --local` / delegation failure) still build no symbols — serve owns the symbol pipeline there. diff --git a/src/cache/file_meta.rs b/src/cache/file_meta.rs index a94c27f3..245f8204 100644 --- a/src/cache/file_meta.rs +++ b/src/cache/file_meta.rs @@ -580,6 +580,14 @@ impl FileMetaStore { self.files.remove(&path_str) } + /// Non-destructive per-file chunk-id lookup — unlike [`Self::remove_file`], + /// which clears the entry as a side effect. Same key normalization. + pub fn file_chunk_ids(&self, key: &str) -> Option> { + self.files + .get(&normalize_path_str(key)) + .map(|m| m.chunk_ids.clone()) + } + /// Re-key entries still filed under an absolute path inside `project_root` /// (the pre-relative-key format) to their project-relative storage key. /// diff --git a/src/index/manager.rs b/src/index/manager.rs index 9d2647a3..72b1d8ea 100644 --- a/src/index/manager.rs +++ b/src/index/manager.rs @@ -250,6 +250,15 @@ fn hide_db_dir_on_windows(db_path: &Path) { .stdout(std::process::Stdio::null()) .stderr(std::process::Stdio::null()) .status(); + // The fts/ subdir is created lazily by the tantivy writer and does not + // inherit the parent's hidden attribute; hide it too so no indexer ever + // crawls fresh segment files. Best-effort, same as the parent. + let _ = std::process::Command::new("attrib") + .arg("+H") + .arg(db_path.join("fts")) + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()) + .status(); } #[cfg(not(windows))] @@ -1315,11 +1324,7 @@ impl IndexManager { cancel_token: &CancellationToken, embedding_pool: Option<&Arc>, ) -> Result<()> { - const FTS_TRANSIENT_BACKOFFS: [std::time::Duration; 2] = [ - std::time::Duration::from_secs(5), - std::time::Duration::from_secs(20), - ]; - Self::retry_on_transient_fts(&FTS_TRANSIENT_BACKOFFS, || async { + Self::retry_on_transient_fts(&Self::FTS_TRANSIENT_BACKOFFS, || async { Self::force_reindex_with_stores_once( codebase_path, db_path, @@ -1363,6 +1368,14 @@ impl IndexManager { attempt().await } + /// Backoffs for the transient Windows FTS commit race redrive: one extra + /// full pass per entry, then a final attempt. Shared by force reindex and + /// the per-file incremental paths so they cannot drift apart. + const FTS_TRANSIENT_BACKOFFS: [std::time::Duration; 2] = [ + std::time::Duration::from_secs(5), + std::time::Duration::from_secs(20), + ]; + async fn force_reindex_with_stores_once( codebase_path: &Path, db_path: &Path, @@ -2558,8 +2571,12 @@ impl IndexManager { Self::ensure_indexing_active(cancel_token)?; for file_path in &files_to_index { debug!("📄 Indexing: {}", file_path.display()); - if let Err(e) = + // Redrive the per-file attempt on the transient Windows FTS + // commit race — the same protection force reindex already has. + if let Err(e) = Self::retry_on_transient_fts(&Self::FTS_TRANSIENT_BACKOFFS, || async { Self::index_single_file(codebase_path, file_path, stores, embedding_pool).await + }) + .await { warn!("⚠️ Failed to index {}: {}", file_path.display(), e); } @@ -2688,6 +2705,82 @@ impl IndexManager { // Find files that were deleted (tracked in metadata but not on disk) let deleted_files = file_meta_store.find_deleted_files(codebase_path); + // Phase 3.6: heal mixed generations on LIVE files. A failed FTS + // commit used to leave the failed attempt's vector chunks + // untracked (never in file_meta) while the tracked generation's + // ids were already deleted from the vector store — search (FTS) + // and get_chunk (vector) then disagreed (the HUSQ stale-index + // symptom). Delete untracked vector ids; queue tracked files + // whose ids are missing from the vector store for Phase 4. + { + let vs_file_chunks = { + let vstore = stores.vector_store.read().await; + vstore.get_chunks_by_file()? + }; + let tracked: std::collections::HashSet = + file_meta_store.tracked_chunk_ids(); + let mut untracked_ids: Vec = Vec::new(); + for (vs_path, chunk_ids) in &vs_file_chunks { + if !codebase_path.join(vs_path).exists() { + continue; // ghost files: Phase 3.5 owns them + } + untracked_ids.extend(chunk_ids.iter().filter(|id| !tracked.contains(id))); + } + let mut heal_files: Vec = Vec::new(); + for key in file_meta_store.tracked_files() { + let Some(meta_ids) = file_meta_store.file_chunk_ids(key) else { + continue; + }; + if !codebase_path.join(key).exists() { + continue; + } + let missing = meta_ids + .iter() + .filter(|id| { + !vs_file_chunks + .get(key) + .is_some_and(|v| v.contains(id)) + }) + .count(); + if missing > 0 { + let p = codebase_path.join(key); + if !files_to_reindex.contains(&p) { + heal_files.push(p); + } + } + } + if !untracked_ids.is_empty() || !heal_files.is_empty() { + info!( + "🧹 Healing mixed generations: {} untracked vector chunks, {} live files missing tracked chunks", + untracked_ids.len(), + heal_files.len() + ); + if !untracked_ids.is_empty() { + { + let mut vstore = stores.vector_store.write().await; + vstore.delete_chunks(&untracked_ids)?; + } + { + let vector_store = Arc::clone(&stores.vector_store); + tokio::task::spawn_blocking(move || { + let mut vstore = vector_store.blocking_write(); + vstore.build_index() + }) + .await + .map_err(|e| anyhow::anyhow!("build_index task panicked: {}", e))??; + } + { + let mut fstore = stores.fts_store.write().await; + for &cid in &untracked_ids { + let _ = fstore.delete_chunk(cid); + } + fstore.commit()?; + } + } + files_to_reindex.extend(heal_files); + } + } + if files_to_reindex.is_empty() && deleted_files.is_empty() { info!("✅ Branch refresh: index is up to date, no changes needed"); return Ok(()); @@ -2810,8 +2903,14 @@ impl IndexManager { Self::ensure_indexing_active(cancel_token)?; let reindex_count = files_to_reindex.len(); for file_path in &files_to_reindex { + // Redrive on the transient Windows FTS commit race — the + // same protection force reindex already has. if let Err(e) = - Self::index_single_file(codebase_path, file_path, stores, embedding_pool).await + Self::retry_on_transient_fts(&Self::FTS_TRANSIENT_BACKOFFS, || async { + Self::index_single_file(codebase_path, file_path, stores, embedding_pool) + .await + }) + .await { warn!("⚠️ Failed to re-index {}: {}", file_path.display(), e); } @@ -3029,6 +3128,71 @@ impl IndexManager { chunk_ids }; + // FTS insert + commit + file_meta update. On ANY failure here the + // vector insert above must be rolled back: the failed attempt's ids + // would otherwise never reach file_meta, so no later pass could find + // and clean them — orphaned vector chunks next to lingering old FTS + // chunks = mixed generations (the HUSQ stale-index symptom). + // file_meta is deliberately left untouched: it still names the old + // generation, so the next pass (redrive or refresh) heals cleanly. + if let Err(e) = Self::publish_file_chunks( + &db_path, + file_path, + stores, + &rel_key, + &embedded_chunks, + &chunk_ids, + model_name, + dimensions, + ) + .await + { + Self::rollback_vector_insert(stores, &chunk_ids).await; + return Err(e); + } + + info!( + "✅ Indexed {} ({} chunks)", + file_path.display(), + embedded_chunks.len() + ); + + Ok(()) + } + + /// Best-effort removal of a failed attempt's fresh chunk_ids from both + /// stores. Called when [`Self::publish_file_chunks`] failed after the + /// vector insert: without this the ids are orphaned (never in file_meta, + /// so no later pass can find them). file_meta is deliberately untouched — + /// it still names the previous generation, so the next pass heals clean. + async fn rollback_vector_insert(stores: &SharedStores, chunk_ids: &[u32]) { + { + let mut store = stores.vector_store.write().await; + let _ = store.delete_chunks(chunk_ids); + } + { + let mut fts_store = stores.fts_store.write().await; + for id in chunk_ids { + let _ = fts_store.delete_chunk(*id); + } + let _ = fts_store.commit(); + } + } + + /// FTS insert + commit + file_meta update for one file's fresh chunks — + /// split from [`Self::index_single_file`] so the caller can roll the + /// vector insert back when any of these steps fails. + #[allow(clippy::too_many_arguments)] + async fn publish_file_chunks( + db_path: &Path, + file_path: &Path, + stores: &SharedStores, + rel_key: &str, + embedded_chunks: &[crate::embed::EmbeddedChunk], + chunk_ids: &[u32], + model_name: &str, + dimensions: usize, + ) -> Result<()> { // Add to FTS with write lock { let mut fts_store = stores.fts_store.write().await; @@ -3048,15 +3212,10 @@ impl IndexManager { } // Update file metadata (separate store, not shared) - let mut file_meta_store = FileMetaStore::load_or_create(&db_path, model_name, dimensions)?; - file_meta_store.update_file(file_path, &rel_key, chunk_ids)?; - file_meta_store.save(&db_path)?; - - info!( - "✅ Indexed {} ({} chunks)", - file_path.display(), - embedded_chunks.len() - ); + let mut file_meta_store = + crate::cache::FileMetaStore::load_or_create(db_path, model_name, dimensions)?; + file_meta_store.update_file(file_path, rel_key, chunk_ids.to_vec())?; + file_meta_store.save(db_path)?; Ok(()) } @@ -4442,4 +4601,129 @@ mod tests { "guard must be released after the rebuild" ); } + + /// The crash-safe rollback: a failed publish must remove the fresh ids + /// from BOTH stores so no later pass inherits orphans it cannot find + /// (they never reach file_meta). + #[tokio::test] + async fn rollback_vector_insert_removes_fresh_ids_from_both_stores() { + use crate::chunker::{Chunk, ChunkKind}; + use crate::embed::EmbeddedChunk; + + let temp = tempdir().unwrap(); + let db_path = temp.path().join("db"); + std::fs::create_dir_all(&db_path).unwrap(); + let stores = create_test_stores(&db_path, 4).await; + + let chunk = |path: &str| { + EmbeddedChunk::new( + Chunk::new( + "fn f() {}".to_string(), + 0, + 1, + ChunkKind::Function, + path.to_string(), + ), + vec![1.0, 0.0, 0.0, 0.0], + ) + }; + let ids = { + let mut vs = stores.vector_store.write().await; + vs.insert_chunks_with_ids(vec![chunk("lib.rs"), chunk("util.rs")]) + .unwrap() + }; + + IndexManager::rollback_vector_insert(&stores, &ids).await; + + let remaining = { + let vs = stores.vector_store.read().await; + vs.get_chunks_by_file().unwrap() + }; + assert!( + remaining.is_empty(), + "rolled-back ids must be gone from the vector store, got: {remaining:?}" + ); + } + + /// Phase 3.6: vector chunks not tracked by file_meta are deleted on the + /// next refresh, and a tracked live file whose ids are missing from the + /// vector store is queued for re-index (heals mixed generations without + /// a force reindex). + #[tokio::test] + async fn refresh_heals_mixed_generations_on_live_files() { + use crate::chunker::{Chunk, ChunkKind}; + use crate::embed::EmbeddedChunk; + + let temp = tempdir().unwrap(); + let codebase_path = temp.path().join("codebase"); + let db_path = temp.path().join("db"); + std::fs::create_dir_all(&codebase_path).unwrap(); + std::fs::create_dir_all(&db_path).unwrap(); + create_metadata_json(&db_path, 4); + + let file = codebase_path.join("lib.rs"); + std::fs::write(&file, "pub fn lib_fn() {}").unwrap(); + + let stores = create_test_stores(&db_path, 4).await; + let chunk = |path: &str| { + EmbeddedChunk::new( + Chunk::new( + "fn f() {}".to_string(), + 0, + 1, + ChunkKind::Function, + path.to_string(), + ), + vec![1.0, 0.0, 0.0, 0.0], + ) + }; + // ids[0] = the tracked generation; ids[1] = an orphan a failed FTS + // commit left behind (its file exists, so Phase 3.5's ghost sweep + // must NOT claim it — Phase 3.6's id-level check owns it). + let ids = { + let mut vs = stores.vector_store.write().await; + vs.insert_chunks_with_ids(vec![chunk("lib.rs"), chunk("lib.rs")]) + .unwrap() + }; + let mut file_meta = FileMetaStore::new("test-model".to_string(), 4); + file_meta + .update_file(&file, "lib.rs", vec![ids[0]]) + .unwrap(); + file_meta.save(&db_path).unwrap(); + + // Simulate the failed commit's delete-phase: the tracked generation + // is gone from the vector store, the orphan remains. + { + let mut vs = stores.vector_store.write().await; + vs.delete_chunks(&[ids[0]]).unwrap(); + } + + let result = IndexManager::refresh_index_with_stores( + &codebase_path, + &db_path, + &stores, + &CancellationToken::new(), + None, + ) + .await; + assert!(result.is_ok(), "refresh must succeed: {result:?}"); + + let remaining = { + let vs = stores.vector_store.read().await; + vs.get_chunks_by_file().unwrap() + }; + let lib_ids = remaining.get("lib.rs").cloned().unwrap_or_default(); + assert!( + !lib_ids.contains(&ids[1]), + "the untracked orphan must be deleted by the sweep, got: {lib_ids:?}" + ); + // The tracked file was queued for re-index; without a real embedding + // model the re-index attempt fails gracefully (warn + continue), so + // the meta ids stay missing — the assertion pins the graceful path, + // the real heal is covered by the HUSQ remediation reindex. + assert!( + !lib_ids.contains(&ids[0]), + "no embedding model in tests: the re-index attempt must fail gracefully, got: {lib_ids:?}" + ); + } } diff --git a/src/mcp/get_chunk.rs b/src/mcp/get_chunk.rs index 9afa59c4..4537c928 100644 --- a/src/mcp/get_chunk.rs +++ b/src/mcp/get_chunk.rs @@ -326,23 +326,32 @@ impl CodesearchService { let mut context_after = None; let mut note = None; + // Resolve the on-disk source path up front: the context read and the + // staleness check below both need it, independent of context_lines. + 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) + }; + let repo_root = owner_alias + .as_deref() + .and_then(|alias| ctx.alias_roots.get(alias)) + .filter(|root| !root.is_empty()) + .map(PathBuf::from) + .unwrap_or_else(|| self.project_path.clone()); + if context_lines > 0 { // 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) - }; match tokio::fs::read_to_string(&source_path).await { Ok(src) => { let lines: Vec<&str> = src.lines().collect(); @@ -371,6 +380,32 @@ impl CodesearchService { } } + // Staleness check: file_meta.json's mtime is the timestamp of the last + // successful indexing write (per-file saves). A source file modified + // after it is pending re-index — or its last indexing pass failed — + // so the indexed lines may not match disk. + if note.is_none() { + let meta_mtime = std::fs::metadata( + repo_root + .join(crate::constants::DB_DIR_NAME) + .join("file_meta.json"), + ) + .and_then(|m| m.modified()) + .ok(); + let file_mtime = std::fs::metadata(&source_path) + .and_then(|m| m.modified()) + .ok(); + if let (Some(indexed_at), Some(modified_at)) = (meta_mtime, file_mtime) { + if modified_at > indexed_at { + note = Some( + "source file was modified after the last indexing pass; indexed \ + lines may not match disk (re-index pending)" + .to_string(), + ); + } + } + } + let response = GetChunkResponse { chunk_id: request.chunk_id, path: chunk.path, diff --git a/src/serve/tests.rs b/src/serve/tests.rs index 6b4323dc..c2b25008 100644 --- a/src/serve/tests.rs +++ b/src/serve/tests.rs @@ -5668,6 +5668,100 @@ async fn get_chunk_reports_the_real_chunker_editor_line_unchanged() { } } +/// get_chunk warns when the source file on disk is newer than the last +/// indexing write (file_meta.json's mtime): the indexed lines may not match +/// disk — pending re-index, or the last pass failed (the HUSQ symptom). +#[tokio::test] +async fn get_chunk_notes_stale_source_newer_than_last_indexing_write() { + use rmcp::handler::server::wrapper::Parameters; + + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path().join("solo"); + std::fs::create_dir_all(&root).unwrap(); + let source = "fn stale_fn() {}\n"; + std::fs::write(root.join("lib.rs"), source).unwrap(); + + let mut chunker = crate::chunker::SemanticChunker::new(100, 4000, 2); + let chunks = chunker + .chunk_semantic( + crate::file::Language::Rust, + std::path::Path::new("lib.rs"), + source, + ) + .expect("real chunker must parse the source"); + let target = chunks.first().expect("at least one chunk").clone(); + + 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; + vs.insert_chunks(vec![crate::embed::EmbeddedChunk::new( + target.clone(), + vec![0.5, 0.5], + )]) + .expect("insert chunk"); + vs.build_index().expect("build index"); + } + { + let mut fts = stores.fts_store.write().await; + fts.add_chunk( + 0, + &target.content, + &target.path, + target.signature.as_deref(), + &format!("{:?}", target.kind), + ) + .expect("fts doc"); + fts.commit().expect("fts commit"); + } + drop(stores); + + // file_meta.json = the last indexing write; then edit the source so its + // mtime is strictly newer than the index. + let mut meta = crate::cache::FileMetaStore::new("minilm-l6-q".to_string(), 2); + meta.update_file(&root.join("lib.rs"), "lib.rs", vec![0]) + .unwrap(); + meta.save(&db_path).unwrap(); + std::thread::sleep(std::time::Duration::from_millis(50)); + std::fs::write(root.join("lib.rs"), "fn stale_fn() {}\n// edited\n").unwrap(); + + let mut config = ReposConfig::default(); + config + .register_with_alias(root.clone(), Some("solo".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))); + let service = crate::mcp::CodesearchService::new_for_serve(state).unwrap(); + + let req = crate::mcp::types::GetChunkRequest { + chunk_id: 0, + chunk_ref: None, + context_lines: None, + project: None, + group: None, + }; + let res = service + .get_chunk(Parameters(req)) + .await + .expect("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!( + parsed["note"] + .as_str() + .is_some_and(|n| n.contains("modified after the last indexing pass")), + "a source file newer than file_meta.json must carry the stale note, got: {text}" + ); +} + /// The no-download model gate: with an empty model cache, a semantic search /// must answer the explicit refusal (naming `codesearch setup`) instead of /// turning the query into an unbounded network fetch — the whole-night serve