Group fan-out correctness, MCP wedge elimination, serve lifecycle hardening and a 4.5k-thread footprint fix by belovaf · Pull Request #308 · flupkede/codesearch

belovaf · 2026-10-06T16:06:29Z

Summary

Fixes accumulated during the pilot — a 504-repo codesearch serve pool — plus two test-hermeticity commits and the changelog section. Rebased onto develop after the 1.5.2 release (now builds against rmcp 3.5.0 / libc / uuid refresh). Every fix has a bold-lead entry in CHANGELOG.md under [1.5.3]; this body is the guided tour.

Group-search correctness

Resolve fan-out hits in their origin store (#the pilot's loudest bug): chunk ids are per-repo counters, so across a group every repo answered for id 0; resolvers probed all stores and kept the first hit, the alphabetically-first repo stole it, and paths lost their alias prefix. MultiReadOutcome now carries SourcedResult{alias, result} end to end.
Literal search relaxes to OR when the exact AND query matches nothing (60% term coverage, flagged as relaxed_fallback) — business-sentence queries stop returning silence.
All responses emit 1-based line numbers — the conversion happens exactly once at each final emit boundary.
Optional min_score with an explicit refusal note instead of confidently-scored garbage.
Empty stores answer queries instead of erroring — 31 empty pilot repos stopped surfacing as per-repo failures on every semantic query.

Serve liveness under a big pool

Per-model embedding init locks + bounded model load (CODESEARCH_MODEL_LOAD_TIMEOUT_SECS, default 600 s): one wedged model fetch no longer poisons every embedding query.
Non-blocking search: fan-outs try-lock past busy stores (reported as warnings) and skip uncached models instead of turning the first semantic query into an unbounded network fetch holding the map lock.
Idle MCP sessions reaped after 30 minutes — wedged sessions no longer pin FIFO-serialized workers forever.
Visible shutdown notice after q: the up-to-3 s MCP drain no longer looks like a hang (and no more reflex Ctrl-C hard-kills mid-drain).

Data & config integrity

Debounced config persist stops clobbering racing registrations (persist adopts its own mtime and converges memory→disk).
DB dirs that survive the delete budget are quarantined (.codesearch.db.removed-<unix_ms>) instead of being adopted as stale-but-valid stores.
Crash-atomic file_meta.json with .bak recovery — the pilot's "warmup fully re-embeds a repo forever" incident can't recur.
--global stores live under CODESEARCH_HOME with collision-proof <name>-<path-digest> slugs.

Operability

codesearch setup --model persists its choice; serve adopts it (flag still wins, offline pre-flight probe warns early).
Managed post-checkout hooks are regenerated on serve startup — 515 stale blocks from older binaries stop being a manual per-repo upgrade.
Tantivy thread footprint: ~4544 → ~50 OS threads on the pilot serve. Every warm repo held an eager IndexWriter — 3 CPU-sized indexing workers, a segment updater and a 4-thread merge pool NoMergePolicy never schedules — plus a reader-side meta.json poller waking twice a second: 9 resident threads × 500 repos, all idle in a search-only pool, choking ps -M/sample. Writers are lazy again (first actual write), run 1 worker + 1 merge thread when created, and self-owned readers use ReloadPolicy::Manual since every commit path already reloads its own reader; only genuinely externally-written readonly stores keep the OnCommitWithDelay watcher. Cross-process locking (codesearch-level .writer.lock) is unchanged.

Test hygiene

db_discovery registration test is platform-independent (pre-canonicalized input — macOS /var → /private/var no longer fails it).
Walker/watcher tests are hermetic against a developer's global .codesearchignore.

Verification

Rebased onto origin/develop (post-1.5.2); cargo test: 835 lib + 831 integration passed, 0 failed; cargo clippy --all-targets: clean.
Live pilot pool (504 repos): warm group semantic latency 1.8–2.0 s; relevance spot-checked (jimmer upsert semantics, ЕФРСБ/court-decision probes).
After the rebuilt serve restarts: ps -M is expected to show ~50 threads where it showed 4544.

Closes the known limitation left by a0c00b1: chunk ids are per-repo counters, so across a group every repo answers for id 0 and any two repos collide as soon as both are non-empty. The fan-out helpers deduplicated by (alias, chunk_id) but flattened to bare results, and every downstream resolver probed all stores taking the first get_chunk(id) hit — the alphabetically-first repo stole the hit, the true definition died in the DEFINITION_KINDS filter (it resolved to the other repo's chunk), and unrelated content rode the stolen score. Group paths also never picked up their alias prefix: the recorded-alias map keyed on bare chunk ids, and the prefix pass matched absolute roots against project-relative stored paths. MultiReadOutcome now carries SourcedResult{alias, result} from the helpers themselves, so every consumer resolves the chunk ONLY in its origin store and prefixes the path from its own origin tag (MultiStoreContext::prefix_sourced_path): find definition/usages, literal_search (BM25 and scan paths), semantic_search_multi in all three modes — with (alias, chunk_id) interned to unique synthetic ids for the bare-u32 RRF fusion, replacing alias_by_chunk and prefix_paths_with_alias — graph imports/dependents/similar (the source-embedding lookup now follows get_chunk's contract: one holder auto-routes, several are an ambiguity), explore's outline dedup, and get_chunk's auto-routed answer. merge_exact_into_fts gains a keyed- by-(alias, chunk_id) sibling for the group lexical path. Pinned by repro tests: two repos whose only chunks share id 0 — group find must answer bankruptcy's interface under its alias prefix, and group literal must return each repo's own content and path. 813 lib tests pass, integration suites green, clippy clean; the single failure (db_discovery register_leaves_unix_path_untouched) is a pre-existing macOS /var-symlink canonicalization issue untouched by this change.

Business queries arrive as sentences; a chunk holding most of their terms is a better answer than silence. When the conjunctive pass returns nothing and the query carries >=2 significant (non-stopword) terms, retry as a disjunction and keep candidates covering >=60% of those terms (at least two). The response flags relaxed_fallback and explains the mode in a note; grep output gains a comment line. Single-term queries never relax, so an empty answer for an unknown identifier stays an honest refusal.

Chunks carry 0-indexed start/end lines by design, and every consumer emitted them raw: the semantic funnel, literal match lines, find, explore outline, get_chunk, similar chunks, imports, and the CLI renderer all showed start_line 0 for a symbol defined on the first line of a file. Convert exactly once at each final emit boundary; intermediate vectordb::SearchResult values, store metadata, chunk- internal context slicing and SCIP-based positions (1-based by origin) stay untouched. The Chunk doc comment now pins the convention so the conversion cannot be applied twice.

Semantic search could never refuse: the vector store always returns nearest neighbours, so junk queries got confidently scored garbage and negative probes looked like weak hits. Add an opt-in min_score to search/SemanticSearchRequest: hits below the threshold are dropped in build_semantic_response (the single funnel every local mode shares), and when nothing survives the empty answer 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 — so the parameter docs spell out both scales; default behaviour is unchanged. The federated response builder keeps its own no-cutoff rule (peer-merged RRF scores are not comparable, as its docs already state).

The pilot's "ghost loss" of freshly registered aliases: the debounced persist worker clones its config snapshot and writes 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 (warmup, aliases, info, group resolve) then saw the fresh mtime, trusted "the file changed", and swapped the disk copy into memory - silently dropping the registration (alias 404s in /info while its build ended with "repo removed or cancelled mid-index"). Two guards: persist_config 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 (bounded; next scheduled pass picks up any remainder). ReposConfig gains PartialEq for the divergence check.

When remove_repo's delete retries expired with the directory still on disk (external holder: another process, AV scanner), the dir stayed in place as a fully valid index. The next registration adopted its stale metadata.json and chunk stores as a false readiness, group fan-out served an empty or stale store, and re-registration against that legacy data ended in arroy EINVAL - the pilot's bare-stamp repos and its os-error-22 wave. Rename the survivor to a .codesearch.db.removed-<unix_ms> 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 - their post-build guard still owns the dir. The HTTP DELETE response gains status "removed_db_quarantined" and a db_quarantined field so the caller sees the honest outcome.

…-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 the OS quirk rather than the verbatim-storage invariant and always failed there. Pre-canonicalize the input: safe_canonicalize is idempotent on a canonical path, so any remaining byte difference is a genuine rewrite - on Linux the tempdir has no symlinks and the input is unchanged.

FileMetaStore::save used plain fs::write (truncate-then-write) while the FSW update path saves after every indexed file — a wide window for an unclean serve shutdown to leave a zero-byte or torn file_meta.json. The pilot hit exactly this: after a killed serve, the next warmup marked every file of payment-identification as Changed (323/323 files fully re-embedded) or failed its 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 mirrors ReposConfig::save_to: keep one best-effort .bak generation, write a pid+counter-unique temp sibling (the refresh task and FSW updates may save the same repo concurrently), fsync it, and rename into place with a bounded retry — reusing vectordb's is_transient_rename_error classification instead of growing a third copy of the raw-error-code list. load_or_create recovers: a torn/unreadable main file falls back to the .bak generation with a warning; only when no generation parses does it degrade to an empty store so the callers' B1 guard can clear the vector/FTS stores deliberately and re-index, instead of poisoning warmup with a permanent hard error.

codesearch setup --model X downloaded the model and then silently forgot the choice: a bare codesearch serve fell back to the built-in default for every new index, so the pilot operator re-typed --model embeddinggemma-q4 on every serve launch. setup now persists the choice to <codesearch_home>/default_model.json (tmp+fsync+rename) and serve adopts it when its own --model flag is absent; the flag still wins. The absence of the default model in the cache was equally unchecked — it surfaced only mid-warmup as parallel index jobs hitting the network at once behind a corporate proxy. serve now runs an offline pre-flight probe of the effective default (flag > persisted > built-in) against the hf-hub cache layout fastembed downloads into, resolving the model table from fastembed itself instead of duplicating repo ids, and warns up front with the codesearch setup fix hint.

codesearch hooks git install was the only writer of the post-checkout hook, so blocks written by older binaries (pre pwd -W fix, pre CODESEARCH_HOME serve_url lookup) 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 block in place (idempotent), while repos with no hook or a foreign hook are left untouched — automatic installation into user repos is too invasive for a background pass. Failures warn and never block startup; a summary line logs how many hooks were upgraded.

…proof slugs The --global index feature leaked its stores into a hardcoded ~/.codesearch.dbs, ignoring the CODESEARCH_HOME relocation knob every other global artifact honours (repos.json, models cache, serve logs), and named each store directory by the bare project file_name — two same-named projects at different paths silently shared ONE store. New stores now live under <codesearch_home>/dbs/<name>-<path-digest>; the historic ~/.codesearch.dbs layout keeps winning when it already exists so old indexes stay reachable. It also registered the project in repos.json while storing the index out-of-tree: discovery and serve only ever look at <project>/.codesearch.db, so a running serve built a second, empty LOCAL store for the alias while the global one sat unused. get_global_db_path no longer registers; rm now detects a real global store by existence, actually deletes its files instead of printing Global-index-removed over surviving data, and still cleans a stale repos.json entry on its own.

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 itself is bounded by MODEL_LOAD_TIMEOUT_SECS (env CODESEARCH_MODEL_LOAD_TIMEOUT_SECS), 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.

The MCP wedge had two server-side amplifiers on top of rmcp per-session FIFO serialization: - Group fan-outs waited up to STORE_LOCK_WAIT_SECS (300s) per busy store, and waits sum: a handful of repos held by an indexing run stalled every other repo answer. Both multi fan-outs now try-lock and skip, reporting each busy repo as a failure warning; single-project queries keep the bounded wait. - A repo whose model was not loaded turned the first semantic query into a network fetch of model files - unbounded on this network, and the map lock it held poisoned 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. Downloading stays on the indexing path. - embed_query ran ONNX inference inline on the async worker, pinning it for the whole pass; both call sites now spawn_blocking with the same poison-recovery contract as embed_chunks_yielding.

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.

An indexing run that ends with zero embeddable files never builds the arroy tree, and search() treated the missing graph as a hard error - so every group fan-out reported each of the 31 empty repos in the uvz pool as a per-repo failure on every semantic query. An empty store now returns Ok(vec![]) (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.

…not look like a hang 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 instead of letting the deadline finish the job. The TUI now prints a shutdown notice right after restoring the terminal, with the live session count, so the pause is visibly bounded.

The changelog-check PR gate requires every PR into develop to touch CHANGELOG.md, and none of the 16 fix commits did. One entry per fix appended to the pending [1.5.2] section in commit order, each distilled from its commit body into the established bold-lead style (user-facing behaviour first, then the incident and mechanism). The heading stays [1.5.2]: patch versions auto-bump on merge while the section accumulates and is finalized in place with a date only when the release is tagged.

…lobal codesearchignore Both families walk tempdirs but FileWalker/FileWatcher also load the machine-wide global ignore (CODESEARCH_HOME first, HOME fallback), so a developer who actually customizes that file got two permanently red tests locally while CI, with a clean home, stayed green - the pilot global 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. The watch helper now pins CODESEARCH_HOME next to its existing HOME/USERPROFILE isolation, and the walker tests adopt the same #[serial] + EnvRestore idiom with a fresh empty codesearch root per test.

….2 release The rebase onto the released upstream merged cleanly but landed the 17 entries inside the dated [1.5.2] - 2026-10-05 section: both sides had appended content at adjacent offsets, so ort saw non-overlapping insertions. Move them into a fresh undated [1.5.3] section - the version Cargo.toml on develop now builds toward - and restore [1.5.2] to its merged upstream shape (the two C# entries plus the flupkede#305 dependency refresh).

… per warm repo 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 twice a second, all idle forever in a search-only pool; a 500-repo pilot serve was observed with 4544 OS threads, ~4500 of them tantivy's. The FTS writer is lazy again: SharedStores::new opens without it and it materializes on the first actual write, running one worker and one merge thread instead of tantivy's derived defaults. Readers of self-owned stores switch to ReloadPolicy::Manual because every commit path already reloads its own reader; only genuinely externally-written readonly stores keep the OnCommitWithDelay watcher (new FtsStore::new_readonly), so foreign commits stay visible. Cross-process locking is unchanged - the codesearch-level .writer.lock still guards the database.

flupkede

Huge thanks for this PR — this is serious, well-engineered work. We did a full chunked review (8 chunks + an independent top-down pass, all claims verified against the post-image): 0 blockers, and the core themes all hold up. Below is the short version; happy to share the full per-chunk review with file:line evidence.

What we verified as solid ✅

The (alias, chunk_id) interning before RRF fusion is the right design, and the fan-out dedup documenting the tag as load-bearing shows the two layers were built together (search.rs:886-922, mod.rs:1476-1479)
The search wedge is structurally closed: try_read() fan-outs with honest per-store failures, loaded-or-on-disk model gate (no download on the query path), spawn_blocking on both embed sites, empty stores answering empty with warnings
The collision case — the exact misattribution this PR fixes — is now deterministically covered by the new thief fixture end-to-end through real rmcp handlers (serve/tests.rs:4275-4367). Nice.
file_meta hardening (tmp+fsync+rename, two-layer recovery, deliberate full-reindex degradation) breaks the torn-file → skipped-warmup chain, all three crash paths tested
Quarantine with honest removed_db_quarantined status; persist convergence pass + mtime adoption; zero existing tests weakened

Must-fix before merge — 4 clusters (all major, none blocker)

A. min_score is silently ignored on the federated and literal paths. The field only exists on SemanticSearchRequest (types.rs:43-50). federated_search hardcodes min_score: None and build_remote_search_body omits it (mcp/mod.rs:1690, 1635-1646); mode="literal" has no field at all (search.rs:85-96, types.rs:212-226); the federated response parser also drops the refusal note (mod.rs:1715). The tool description and CHANGELOG advertise it as a general option. Fix: pass it through both federated legs; for literal, either support it or refuse it by name.
B. "Non-blocking fan-outs" only holds for the two search fan-outs. try_read() lives only in with_vector_store_read_multi/with_fts_store_read_multi (mod.rs:1453, 1541). The explore outline (explore.rs:89), find_imports (graph.rs:65) and find_similar in both passes (graph.rs:599, 672) still wait up to 300 s per busy store (STORE_LOCK_WAIT_SECS, constants.rs:581) — N busy repos ⇒ N×300 s, the exact interactive wedge this PR kills for search. Fix: apply the same try_read + report contract there; narrow the CHANGELOG:33 heading.
C. The relaxed fallback defeats itself for signature/kind matches. search_relaxed queries content+signature+kind (fts/tantivy_store.rs:589, sig boost 2.0) but the coverage gate tokenizes only chunk.content (literal_search.rs:380, 662-672); signature lives in the separate searchable_text field (vectordb/store.rs:360) — candidates get rescued and then dropped, exactly for the identifier queries the fallback exists for. Fix: gate on searchable_text with a content fallback for legacy blobs (#[serde(default)]), or narrow the query to content_field.
D. get_chunk group mode never returns context (live bug). The chunk path is alias-prefixed (get_chunk.rs:184/284) and then read as self.project_path.join("ALIAS/src/…") (get_chunk.rs:295), which never exists → always the "source file not readable" note (314-318). Fix: resolve against ctx.alias_roots[alias] — the root lookup right above at :177-181 already shows the pattern.

Suggested order if useful: C (~1-2 h) → D (~30 min) → A (~2-3 h) → B (~2-4 h) — roughly a day total.

Non-blocking follow-ups

Windows quarantine is powerless with open handles (rename is unix-only, serve/mod.rs:2390) and nothing cleans up .removed-* dirs (~100 MB each) — a startup sweep would help
Untested claims worth a test: keep_alive idle reap (mod.rs:6602), deadline-wipe remnant, removed_db_quarantined HTTP mapping, TUI drain note; MCP-unit-level RRF fusion on collision (end-to-end is covered, unit level isn't)
Mixed-model groups sort incomparable cosine scores (resolve_query_model per repo, mod.rs:1050-1076 + sort 1499-1504) — a model-uniformity warning would help
CHANGELOG:33 "downloading stays on the indexing path" is untrue for CLI codesearch search (search/mod.rs:516, :1123 bypass the gate)
Docs: CODESEARCH_MODEL_LOAD_TIMEOUT_SECS and CODESEARCH_STORE_LOCK_WAIT_SECS are missing from the README env table

Overall: conditionally mergeable — A and B touch the PR's own claims, so we'd love to see those closed before merge; C and D are quick wins. Again, great work.

The relaxed BM25 pass rescues candidates by querying content, signature and kind (signature boosted 2.0), but the coverage gate tokenized only chunk.content — a candidate rescued BY its signature was then dropped by the gate for not covering the terms in its body, defeating the fallback exactly for the identifier queries it exists for. The gate now runs over the stored searchable_text (signature + docstring + content), with a bare-content fallback for legacy blobs that predate the field and deserialize it as an empty string.

Stored chunk paths are 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_path, a location that never exists. Every group-mode get_chunk with context_lines answered "source file not readable, returning indexed content only", always. The store that answers now publishes its alias and its pre-prefix stored path; the context read resolves that 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.

… in literal mode The tool description and CHANGELOG advertised min_score as a general option, but the field only lived on SemanticSearchRequest: federated_search hardcoded min_score None into its local leg, build_remote_search_body omitted it from every peer body, and the response parsers (local re-parse and FederationClient alike) dropped the peers refusal notes - so a threshold either did nothing or silently emptied a leg that then read as "no hits there". Both federated legs now forward request.min_score, the federation client returns a SearchReply { items, note } that relays a refusing peer's note into the response warnings, and the local leg's refusal note surfaces the same way. Literal mode has no calibratable scale (BM25 scores are corpus-relative), so passing min_score with mode="literal" now returns an explicit error naming the field instead of silently ignoring it.

…exing Group-scoped find, explore similar, get_chunk's candidate scan, and the literal BM25/tokenless metadata reads all bounded-waited on each store's vector write lock, so a single indexing run wedged every other repo's answer for up to N x STORE_LOCK_WAIT_SECS while the query looked hung. Every multi-repo fan-out site now goes through try_vector_read_or_note: try_read, and on contention a per-repo warning ("store busy: active indexing holds the write lock - repo skipped in this fan-out, retry shortly") is recorded and the repo skipped, matching the search fan-outs' established contract. Deliberately unchanged: single-project queries and get_chunk's direct/project arm keep the bounded wait - there is no second store to answer for when the one store is busy, so waiting is the honest move. The changelog bullet also drops its false "downloading stays on the indexing path" claim for CLI search: the model gate guards the serve/MCP query paths; the CLI search command manages its own model loading. Tests lock the very SharedStores routing opens (get_opened_stores after a priming call) and assert find/get_chunk/similar still answer from the healthy repo with the busy repo named in warnings.

remove_repo's delete-budget fallback renames a surviving db dir aside as .codesearch.db.removed-<unix_ms> "for manual cleanup" - and manual cleanup never happened: 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. run_serve sweeps the remnants on its own blocking task; failures warn and never block startup - a remnant that still cannot be removed waits for the next restart.

A group whose repos embed with different models answers with cosine distances from different vector spaces, and the merged ranking presents them as one scale - an agent reading the top hit's score as "strongest match overall" is comparing coordinates from unrelated spaces. The group embedding pass now collects the distinct models that actually answered (missing models never reach the fan-out and stay covered by their own per-model skip warning) and appends a warning naming every distinct model. RRF fusion in auto/hybrid partially hides the problem by fusing ranks only; semantic_mode's raw cosine scores are where it bites hardest. The contract lives in mixed_model_group_warning() so it is unit-testable without standing up two on-disk embedding models.

Five statements shipped with code and CHANGELOG text but no test: - the rmcp session manager's idle-reap bound (keep_alive used to be None; a wedged session kept its socket and worker alive forever) - extracted into session_manager_with_idle_reap() + MCP_IDLE_SESSION_SECS; - recover_repo_format's refusal to wipe under a live index task and the permanent-delete-failure path (format_recovery_done records at the wipe, so a failed wipe must not tombstone the alias); - DELETE /repos/{alias} reporting "removed_db_quarantined" with the db_quarantined field instead of reading as "removed"; - the post-q TUI drain notice - extracted into shutdown_drain_notice() (the visible ~3 s bound, the session count, "exits by itself"); - the group RRF interning contract - intern_chunk lifted to intern_group_chunk_id() so the collision case (two repos' chunk id 0 fusing into one entry without interning, both surviving with it) is unit-pinned. Also documents CODESEARCH_MODEL_LOAD_TIMEOUT_SECS and CODESEARCH_STORE_LOCK_WAIT_SECS in the README env table - both knobs existed in constants.rs but were absent from every operator surface.

belovaf · 2026-10-07T07:26:06Z

Review response: all four must-fix clusters (A–D) and every non-blocking
follow-up are addressed, one commit per item, on top of the reviewed head.

A — min_score silently ignored on federated/literal paths (`2d6ca1b`)

Both federated legs now forward the threshold: the peer body carries
min_score and the local leg passes request.min_score into its
SemanticSearchRequest instead of hardcoding None. Each leg's refusal
note is relayed to the caller (SearchReply { items, note } from the
federation client, parse_note_from_call_result for the local leg) and
lands in warnings tagged with the leg's name. Literal mode has no
calibratable scale (BM25 is corpus-relative), so it refuses the parameter
by name before any routing instead of silently dropping it.

B — only the search fan-outs were non-blocking (`b3357a3`)

Every interactive multi-repo fan-out now try-locks through one shared
helper (try_vector_read_or_note) and skips a busy repo with the same
"store busy" warning the search fan-outs emit: find definition/usages,
graph imports/dependents/similar (both the embedding lookup and the
neighbour search), explore similar, get_chunk's candidate scan, and the
literal BM25/tokenless metadata reads. Deliberately unchanged:
single-project queries and get_chunk's direct/project arm keep the
bounded wait — with one store there is no fan-out to degrade, waiting is
the honest move. The changelog bullet was also rewritten to cover the
real scope, and its "downloading stays on the indexing path" claim was
corrected for the CLI (F4, text-only).

C — relaxed fallback dropped signature-rescued candidates (`abcc0c3`)

The ≥60% coverage gate now runs against relaxed_gate_text() — the
stored searchable_text (signature + docstring + content), i.e. exactly
what the relaxed pass queries and rescues by — with a bare-content
fallback for legacy blobs that predate the field.

D — group-mode get_chunk never returned context (`ed76ab0`)

Chunk paths are project-relative, and the unique-owner arm prefixed
chunk.path before the shared context read, which then joined the
already-prefixed path onto the serve process's own project root — a path
that never exists. The arm now publishes owner_alias and the
pre-prefix stored_path_raw from inside itself; the context read joins
the raw stored path against the owning alias's root (alias_roots).
The historical project_path join stays for stdio single-repo mode,
where it was already correct.

Follow-ups (all in)

F1 (ef3be37): serve sweeps .codesearch.db.removed-* quarantine
remnants at startup (best-effort delete, warn on failure) — the
"manual cleanup" the rename left behind never happened, so every
budget-expired delete leaked a full stale index copy forever.
F2 (7dbac84): tests pinning the previously-untested claims — the
idle-session reap bound (extracted session_manager_with_idle_reap,
MCP_IDLE_SESSION_SECS), recover_repo_format's refusal to wipe under a
live index task plus its failed-wipe-doesn't-tombstone contract,
DELETE /repos reporting removed_db_quarantined with db_quarantined,
the post-q TUI drain notice (shutdown_drain_notice extracted), and the
group RRF interning contract at unit level (intern_group_chunk_id).
F3 (e4ac68e): mixed-model group answers append a warning that
scores are NOT comparable across repos, naming every distinct model
that actually answered.
F5: README env table gains CODESEARCH_MODEL_LOAD_TIMEOUT_SECS and
CODESEARCH_STORE_LOCK_WAIT_SECS.

Testing

The lib suite went from 838 to 853 passing on this branch (+15 new unit
and handler-level tests, zero failures). The busy-store tests lock the
very SharedStores routing opens (get_opened_stores after a priming
call), so a regression back to bounded waits fails on the lock timeout
instead of passing slowly. cargo clippy --all-targets is clean.

The remaining local work (chunker/embedding wedge fixes) is intentionally
not part of this PR — it will follow as a separate PR once this one
lands, to keep this diff review-sized.

flupkede

Systematic audit of the fix batch — 21/21 items judged: 19 PASS, 2 must-fix

First: thank you for turning the four review clusters around this quickly and this cleanly. I re-audited the full branch end-to-end (27 commits, 2ee0837..7dbac84) after the fix batch and verified every fix against the post-image:

min_score on federated + literal — flows on the federated leg now, literal refuses by name instead of silently dropping (2d6ca1b) ✅
try-lock fan-outs — try_vector_read_or_note reaches all 11 read sites including explore/find/get_chunk; busy stores are skipped with a reported failure instead of blocking up to 300 s (b3357a3) ✅
relaxed fallback self-defeat — the coverage gate now reads searchable_text with a content fallback for legacy blobs, so signature-rescued hits survive (abcc0c3) ✅
get_chunk group context — resolves against the owning repo's root; covered by its own end-to-end test (ed76ab0) ✅
plus the new pins (7dbac84), the mixed-model warning (e4ac68e), the quarantine sweep (ef3be37), and the tantivy thread reduction (76410fd).

Verdict: conditionally mergeable. Two findings need a fix first; both are small and local. Everything else below is non-blocking.

Must-fix before merge

1. Merge-blocking: the quarantine sweep never visits the directories where quarantine actually leaves remnants — src/serve/mod.rs:6524
The startup sweep iterates config.repos.values() only, but every quarantine is created inside remove_repo after config.unregister_alias(alias) (:2221, quarantine at :2397). So on the next serve start the removed repo's path is absent from the config and its .codesearch.db.removed-<ms> sibling is never swept — the permanent ~100 MB-per-delete leak this commit's CHANGELOG line says it fixes. The new test feeds paths directly, which is why the wiring gap is invisible.
Fix: derive sweep roots from a source that survives unregistration (a tombstone file, the discovery roots, or a removed-paths backup).

2. The tantivy Manual-reload failure path logs a false "non-fatal" — src/fts/tantivy_store.rs:512-515
Under the new ReloadPolicy::Manual, a failed reader.reload() logs debug!("Non-fatal: reader will eventually catch up") — but the background poller that made that true was removed with OnCommitWithDelay. Nothing else ever reloads a self-owned store, so one transient I/O error (the Windows AV/sharing-violation class this codebase documents elsewhere) leaves every search answering a stale segment until the next commit, silently. clear() at :798 already propagates the same error — the seam is inconsistent within the file.
Fix: retry the reload once, escalate to a visible warning on persistent failure, and correct the comment.

Quarantine on Windows (second finding of the same item-11 cluster)

The rename's atomicity is unix-only per the code's own comment (serve/mod.rs:2390). On Windows, renaming a directory with any open handle fails with a sharing violation — the same failure class that exhausted the delete budget — so the dir stays in place and the next registration can adopt the stale index the quarantine exists to prevent. The fallback status removed_db_locked is honest, but the limitation never reaches the operator surface.
Suggestion: on a failed rename write a tombstone marker (or quarantine at next start once holders are gone) and name the platform limitation in the handler message.

Design notes (non-blocking, fine as follow-ups)

min_score doc names the wrong scale for lexical mode — "RRF points rarely above 0.2" (types.rs:443-447), but lexical emits raw BM25 >1.0 through the same filter (search.rs:1400); a doc-calibrated threshold filters nothing.
A response emptied by filter_path after min_score survived carries no explanatory note (search.rs:1232 vs :1262).
A crashed model loader arrives as the timeout text ("likely a blocked network fetch") — Disconnected should get its own message (embed/mod.rs:398-405).
A poisoned per-model init lock is permanently fatal for that model until restart; consider PoisonError::into_inner (embed/mod.rs:359-361).
Persist deadline-wipe residual window is microseconds and self-healing (serve/mod.rs:1556-1562) — documenting it is enough.
The "~3 s" in the TUI drain notice duplicates the 3 s shutdown deadline as a second literal (tui.rs:77) — shared constant.
The hook rewrite is a plain truncate-and-write (cli/mod.rs:2083) while the same PR introduces tmp+fsync+rename elsewhere; a torn write permanently misclassifies the block as Foreign. Also let _ = make_executable swallows a failing chmod (:2085).
After a .bak recovery, the first save copies the still-corrupt main over the just-recovered backup (file_meta.rs:393-395); a second crash in that window forces full reindex. Plus tmp-litter has no startup sweep ([Debt]).
Legacy-layout global stores keep a name-collision path (index/mod.rs:509-519); and global-store deletion runs after unregister and is skipped on the delegation path (:1947, :1864).
A failing save_default_model after a successful download surfaces as a bare io-error with no "the model itself is fine" context (cli/setup.rs:52).

Caveat

Clippy did not run on the review machine (no MSVC toolchain), so the static-analysis dimension is unmeasured here — your CI's clippy is the authority there.

Again — the fix batch is genuinely solid work; the two must-fix items are the last seams, not a pattern.

flupkede · 2026-10-09T22:21:26Z

Windows-only test failure found while validating this branch locally (blocks the dev-machine pre-push QC gate, CI is green because Linux resolves HOME):

index::remove_order_tests::global_db_path_prefers_legacy_home_layout_when_it_exists fails on Windows — the test sets HOME only, but dirs::home_dir() reads USERPROFILE there. global_dbs_root() therefore never sees the legacy root and falls through to CODESEARCH_HOME/dbs:

an existing legacy store must keep resolving from ~/.codesearch.dbs, got ...\cs-home\dbs\foo-06b76a98\.codesearch.db

Production logic is correct; the fix is test-only — isolate USERPROFILE alongside HOME (pattern already used by isolated_global_ignore_env in src/file/mod.rs and src/watch/mod.rs):

("USERPROFILE", old_home.to_str().unwrap()),

# Conflicts: # CHANGELOG.md # src/cache/file_meta.rs # src/constants.rs # src/mcp/search.rs # src/mcp/tests.rs # src/serve/mod.rs # src/serve/tests.rs

- file_meta: restore the builder_version stamp + TUI-safe warn_print the merge dropped from the PR's crash-atomic rewrite; the save test moves to the pid+counter-unique tmp scheme (leftovers are ignored on load, the save's own tmp is consumed by its rename). - get_chunk: the context-before window treated chunk.start_line as 1-based; chunks are 0-based (the 1-based conversion happens once at the emit boundary) — the context pin caught the off-by-one. - global stores: split the legacy-layout decision into an injectable seam (global_dbs_root_for / global_db_dir_in) — Windows dirs::home_dir() reads the profile from the OS, so no env override can redirect the legacy probe in a test.

belovaf added 19 commits October 6, 2026 18:22

belovaf requested a review from flupkede as a code owner October 6, 2026 16:06

belovaf changed the base branch from master to develop October 6, 2026 16:06

belovaf changed the title ~~Group fan-out correctness, MCP wedge elimination, serve lifecycle hardening~~ Group fan-out correctness, MCP wedge elimination, serve lifecycle hardening and a 4.5k-thread footprint fix Oct 6, 2026

flupkede reviewed Oct 6, 2026

View reviewed changes

belovaf added 6 commits October 7, 2026 02:05

flupkede reviewed Oct 9, 2026

View reviewed changes

flupkede added 2 commits October 17, 2026 15:19

Merge remote-tracking branch 'origin/develop' into pilot-308

c18f9b2

# Conflicts: # CHANGELOG.md # src/cache/file_meta.rs # src/constants.rs # src/mcp/search.rs # src/mcp/tests.rs # src/serve/mod.rs # src/serve/tests.rs

flupkede merged commit 2d78345 into flupkede:develop Oct 17, 2026
2 of 3 checks passed

flupkede pushed a commit that referenced this pull request Oct 17, 2026

chore: bump version to 1.5.38 (auto, PR #308 merged to develop)

81d85f6

This was referenced Oct 17, 2026

feat: .codesearchinclude whitelist — index only matching files when present #346

Merged

fix: reconcile the line-number convention — chunkers emit 1-based, responses pass through #347

Merged

Navigation Menu

Repository navigation

Group fan-out correctness, MCP wedge elimination, serve lifecycle hardening and a 4.5k-thread footprint fix - #308

Group fan-out correctness, MCP wedge elimination, serve lifecycle hardening and a 4.5k-thread footprint fix#308
flupkede merged 29 commits into
flupkede:developfrom
belovaf:develop

belovaf commented Oct 6, 2026 •

edited

Loading

Uh oh!

flupkede left a comment

Uh oh!

belovaf commented Oct 7, 2026

Uh oh!

flupkede left a comment

Uh oh!

flupkede commented Oct 9, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

belovaf commented Oct 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Group-search correctness

Serve liveness under a big pool

Data & config integrity

Operability

Test hygiene

Verification

Uh oh!

flupkede left a comment

Choose a reason for hiding this comment

What we verified as solid ✅

Must-fix before merge — 4 clusters (all major, none blocker)

Non-blocking follow-ups

Uh oh!

belovaf commented Oct 7, 2026

A — min_score silently ignored on federated/literal paths (2d6ca1b)

B — only the search fan-outs were non-blocking (b3357a3)

C — relaxed fallback dropped signature-rescued candidates (abcc0c3)

D — group-mode get_chunk never returned context (ed76ab0)

Follow-ups (all in)

Testing

Uh oh!

flupkede left a comment

Choose a reason for hiding this comment

Systematic audit of the fix batch — 21/21 items judged: 19 PASS, 2 must-fix

Must-fix before merge

Quarantine on Windows (second finding of the same item-11 cluster)

Design notes (non-blocking, fine as follow-ups)

Caveat

Uh oh!

flupkede commented Oct 9, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

belovaf commented Oct 6, 2026 •

edited

Loading

A — min_score silently ignored on federated/literal paths (`2d6ca1b`)

B — only the search fan-outs were non-blocking (`b3357a3`)

C — relaxed fallback dropped signature-rescued candidates (`abcc0c3`)

D — group-mode get_chunk never returned context (`ed76ab0`)