From bdde3d32e9a3f5c6437ea328d401a9e915d81c40 Mon Sep 17 00:00:00 2001 From: Don Bowman Date: Sun, 18 Oct 2026 18:12:17 -0400 Subject: [PATCH] fix(serve): adopt an existing index on POST /repos instead of force-rebuilding Registering a repo through the running serve (codesearch index add, the git-hook path, or a script adding many repos) unconditionally ran the destructive force reindex. A repo that already had a valid .codesearch.db - for example one indexed directly before being registered - lost its index and had to re-embed every file, serialized behind CODESEARCH_INDEX_JOBS. With a batch of registrations that looked like no progress at all for a long time. The local 'codesearch index add' path already reuses an existing index; the serve path now matches it. Adopt when metadata.json records a model and the request is not changing it, and run the gated incremental refresh instead of the clear-and-rebuild. A new index (no recorded model) and an explicit model change still take the force path, because changing the model invalidates every stored vector. Tests: decision table for the adopt rule, plus an end-to-end add that seeds an unchanged tracked file and asserts its chunk survives (the old force path clears it). --- CHANGELOG.md | 6 ++ src/serve/mod.rs | 99 +++++++++++++++++++++++-------- src/serve/tests.rs | 143 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 225 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7408bc7c..9ea43a87 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,12 @@ more PRs land; when the release is actually tagged, the same section is finalized in place with a date — no renaming/migration step needed. --> +## [1.6.1] + +### Fixed + +- **`POST /repos` adopts an index that is already on disk instead of clearing and re-embedding it.** Registering a repo (`codesearch index add`, the git-hook path, or a script adding many repos at once) unconditionally ran the destructive force reindex, so a repo that already had a valid `.codesearch.db` — for example one indexed directly before being registered — lost its index and had to re-embed every file, serialized behind the process-wide job gate. The local `codesearch index add` path already reused an existing index; the serve path now matches it. A new index (no `metadata.json`) and an explicit model change still take the force-rebuild path, because changing the embedding model invalidates every stored vector. + ## [1.6.0] - 2026-10-18 ### Fixed diff --git a/src/serve/mod.rs b/src/serve/mod.rs index fab9d10a..ed07b456 100644 --- a/src/serve/mod.rs +++ b/src/serve/mod.rs @@ -5664,12 +5664,28 @@ fn resolve_add_repo_model( }) } +/// Whether `POST /repos` may keep an index already on disk instead of +/// force-rebuilding it. +/// +/// Adopt when the index recorded a model and no model change was requested; +/// `None` from `from_index_metadata` means there is no usable index (or a +/// pre-model legacy index), which the destructive force path rebuilds. An +/// explicit/override model that differs from the recorded one invalidates every +/// stored vector (dimensions and vector space change), so it must rebuild. +fn should_adopt_existing_index( + recorded: Option, + model_override: Option, +) -> bool { + recorded.is_some() && model_override.is_none_or(|requested| recorded == Some(requested)) +} + /// Add-repo handler: POST /repos /// /// Registers a new repo in repos.json, opens the LMDB/Tantivy stores inline /// (fast — prevents the double-open race from the old `index_quiet` path), -/// then spawns a full reindex + vector index build + FSW start in the -/// background. Returns 202 Accepted immediately. +/// then spawns the indexing work in the background: an incremental refresh when +/// an index already exists (adopted), a full reindex otherwise. Returns 202 +/// Accepted immediately. async fn add_repo_handler( axum::extract::State(state): axum::extract::State>, axum::extract::Json(body): axum::extract::Json, @@ -5735,11 +5751,19 @@ async fn add_repo_handler( // creating a brand-new index: an existing index keeps the model recorded in // its `metadata.json`, exactly as if the flag had not been set. An explicit // `model` still wins and rebuilds at that model's dimension. - let model_override = resolve_add_repo_model( - explicit_model, - crate::embed::ModelType::from_index_metadata(&db_path), - state.default_model(), - ); + let recorded_model = crate::embed::ModelType::from_index_metadata(&db_path); + let model_override = + resolve_add_repo_model(explicit_model, recorded_model, state.default_model()); + + // A usable index already on disk is adopted (incremental refresh), never + // thrown away: `POST /repos` is also the registration path behind + // `codesearch index add`, whose local path explicitly reuses an existing + // index. Force-rebuilding every registration meant adding N already-indexed + // repos cleared all N indexes and re-embedded them, serialized behind the + // heavy-job gate — minutes-to-hours of no visible progress for work that was + // already on disk. Build fresh only when there is no index yet or the caller + // asked for a different embedding model (the dimensions change). + let adopt_existing = should_adopt_existing_index(recorded_model, model_override); // Register in repos.json let alias = { @@ -5890,26 +5914,55 @@ async fn add_repo_handler( let task_db_path = db_path.clone(); let index_handle = tokio::spawn(async move { - tracing::info!( - "Indexing newly added repo '{}' ({}) in background", - alias_bg, - project_path.display() - ); + if adopt_existing { + tracing::info!( + "Refreshing existing index for newly added repo '{}' ({}) in background", + alias_bg, + project_path.display() + ); + } else { + tracing::info!( + "Indexing newly added repo '{}' ({}) in background", + alias_bg, + project_path.display() + ); + } let pool = state_bg.embedding_pool(); - match IndexManager::force_reindex_with_stores( - &project_path, - &db_path, - &stores, - model_override, - &token_for_task, - Some(&pool), - ) - .await - { + let indexed = if adopt_existing { + // Incremental refresh: unchanged files stay in the index, only real + // changes are re-chunked/re-embedded. Same heavy-job gate and + // cancellation semantics as the force path (the public wrapper + // acquires the permit). + IndexManager::perform_incremental_refresh_with_stores( + &project_path, + &db_path, + &stores, + &token_for_task, + Some(&pool), + None, + ) + .await + } else { + IndexManager::force_reindex_with_stores( + &project_path, + &db_path, + &stores, + model_override, + &token_for_task, + Some(&pool), + ) + .await + }; + match indexed { Ok(()) => { tracing::info!( - "Index created for '{}' ({})", + "{} for '{}' ({})", + if adopt_existing { + "Existing index refreshed" + } else { + "Index created" + }, alias_bg, project_path.display() ); diff --git a/src/serve/tests.rs b/src/serve/tests.rs index e093117f..fb3d9f29 100644 --- a/src/serve/tests.rs +++ b/src/serve/tests.rs @@ -1227,6 +1227,127 @@ async fn add_repo_handler_keeps_recorded_model_over_serve_default() { ); } +/// `POST /repos` for a path whose `.codesearch.db` already holds a usable index +/// must adopt it (incremental refresh), not clear and re-embed it. The old +/// force path re-indexed every re-registration, so registering a batch of +/// already-indexed repos threw away all their work and re-ran it serialized +/// behind the heavy-job gate. +/// +/// Fixture: one unchanged whitespace `lib.rs` tracked by an index holding one +/// chunk. Adoption sees "up to date" and keeps the chunk (no embedding model is +/// loaded). The force path clears the store first; re-indexing the file yields +/// zero chunks, so the chunk count drops to zero. +#[tokio::test] +#[serial] +async fn add_repo_handler_adopts_existing_index_instead_of_rebuilding() { + let _env = crate::testing::EnvRestore::remove(&[ALLOWED_ROOTS_ENV]); + let tmp = tempfile::tempdir().unwrap(); + let repo_path = tmp.path().join("adopted"); + std::fs::create_dir(&repo_path).unwrap(); + let db_path = repo_path.join(DB_DIR_NAME); + std::fs::create_dir_all(&db_path).unwrap(); + + // Non-empty (the walker skips empty files) but yields no chunks, so the + // force path cannot re-embed and needs no ONNX session. + let file_path = repo_path.join("lib.rs"); + std::fs::write(&file_path, "\n\n").unwrap(); + std::fs::write( + db_path.join("metadata.json"), + r#"{"schema_version":1,"dimensions":384,"model_short_name":"minilm-l6-q","indexed_at":"2026-01-01T00:00:00Z"}"#, + ) + .unwrap(); + + let chunk_id = { + let stores = crate::index::SharedStores::new(&db_path, 384).expect("fixture stores"); + let chunk = crate::chunker::Chunk::new( + "\n\n".to_string(), + 1, + 2, + crate::chunker::ChunkKind::Function, + "lib.rs".to_string(), + ); + let ids = { + let mut vs = stores.vector_store.write().await; + let ids = vs + .insert_chunks_with_ids(vec![crate::embed::EmbeddedChunk::new( + chunk, + vec![0.5f32; 384], + )]) + .expect("seed vector chunk"); + vs.build_index().expect("build fixture index"); + ids + }; + { + let mut fts = stores.fts_store.write().await; + fts.add_chunk(ids[0], "\n\n", "lib.rs", Some("adopted"), "Function") + .expect("seed fts doc"); + fts.commit().expect("commit fts"); + } + { + let mut fm = crate::cache::FileMetaStore::load_or_create(&db_path, "minilm-l6-q", 384) + .expect("seed file meta"); + let key = crate::cache::storage_key(&file_path, &repo_path); + fm.update_file(&file_path, &key, ids.clone()) + .expect("track the unchanged file"); + fm.save(&db_path).expect("save file meta"); + } + // Release the writer lock so the handler can open the stores itself. + ids[0] + }; + + let state = Arc::new(state_with_config(ReposConfig::default())); + let (status, body) = add_repo_handler( + axum::extract::State(state.clone()), + axum::extract::Json(AddRepoRequest { + path: repo_path.clone(), + alias: Some("adopted".to_string()), + model: None, + }), + ) + .await; + assert_eq!( + status, + axum::http::StatusCode::ACCEPTED, + "add must be accepted, got {}: {}", + status, + body.0 + ); + + // The indexing task runs in the background; wait for it to release the + // alias marker (bounded so a hang fails the test instead of stalling it). + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(30); + while state.is_indexing("adopted") { + assert!( + std::time::Instant::now() < deadline, + "adopted-index refresh did not finish within 30s" + ); + tokio::time::sleep(std::time::Duration::from_millis(25)).await; + } + + let stores = state + .get_opened_stores("adopted") + .expect("store must still be open after the add"); + let stats = stores.vector_store.try_read().unwrap().stats().unwrap(); + assert_eq!( + stats.total_chunks, 1, + "an existing index must be adopted, not cleared and rebuilt" + ); + assert_eq!( + stats.total_files, 1, + "the tracked unchanged file must survive the adoption refresh" + ); + assert!( + stores + .vector_store + .try_read() + .unwrap() + .get_chunk(chunk_id) + .expect("read back adopted chunk") + .is_some(), + "the previously indexed chunk must still be present (id {chunk_id})" + ); +} + /// Precedence contract for the model a `POST /repos` add indexes with. #[test] fn resolve_add_repo_model_precedence() { @@ -1249,6 +1370,28 @@ fn resolve_add_repo_model_precedence() { assert_eq!(resolve_add_repo_model(Some(mini), None, None), Some(mini)); } +/// Adoption contract for `POST /repos`: an index already on disk is kept unless +/// the add changes its embedding model. Rebuilding a registered-but-already- +/// indexed repo used to clear and re-embed everything. +#[test] +fn should_adopt_existing_index_precedence() { + use crate::embed::ModelType; + let gemma = ModelType::EmbeddingGemma300MQ4; + let mini = ModelType::AllMiniLML6V2Q; + + // No index on disk → nothing to adopt, force-build. + assert!(!should_adopt_existing_index(None, None)); + assert!(!should_adopt_existing_index(None, Some(gemma))); + + // Existing index, no model change → adopt and refresh incrementally. + assert!(should_adopt_existing_index(Some(mini), None)); + // An explicit model that matches the recorded one is not a change. + assert!(should_adopt_existing_index(Some(mini), Some(mini))); + + // A model change invalidates the vectors → force rebuild. + assert!(!should_adopt_existing_index(Some(mini), Some(gemma))); +} + /// The serve-wide default is the scope-free fallback model in serve mode (the /// unpinned status summary, a call with no routed alias). It is deliberately /// NOT the query fallback for a repo that records no model — see