fix: full review remarks — concurrency, tests, metadata consistency, doc-comments#102
Merged
Conversation
- repos.rs: correct "Pure (no disk I/O)" doc-comments on relocate_missing and prune_stale — both transitively call scan_for_remote which does read_dir and spawns a git subprocess; callers must use spawn_blocking in async contexts. - serve/mod.rs: replace raw std::fs::canonicalize with safe_canonicalize in reload_if_changed so Windows UNC prefix (\?\) is stripped before comparison, consistent with the project-wide safe_canonicalize rule. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ts + metadata consistency + cancel best-effort - Extract the safety-net HNSW rebuild into ensure_hnsw_index_if_needed() so the logic is unit-testable; add 3 tests (unindexed with chunks rebuilds, already-indexed is idempotent, empty DB skips rebuild). - metadata.json schema consistency: add "partial": false to the normal (non-cancelled) path so readers always see the field regardless of how indexing ended. - Cancellation finalisation path: change non-critical ? propagations to log-and-continue (metadata.json write, FileMetaStore update/save, stats read) — keeps partial chunks searchable even if any recovery step fails. store.build_index() still propagates errors as before. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…_index in spawn_blocking Three related concurrency fixes in serve/mod.rs, consistent with the reconcile_all_paths fix from PR #101: 1. evaluate_csharp_rebuild: bootstrap_last_changed (git subprocess + ≤10k file fs-walk) was running while holding config.write(), blocking every concurrent config.read() for the scan duration. Fixed by checking whether bootstrap is needed under a read lock, running the slow I/O with no lock held, then taking the write lock only for the brief config update. 2. evaluate_csharp_rebuild call site in run_phase_2_csharp_scip: even with the above fix the function still ran synchronously on a Tokio worker thread. Wrapped in spawn_blocking so the async runtime stays responsive while scanning all C# candidates at startup. 3. warmup_repo and add_repo_handler background task: two build_index() calls were running directly on async threads while holding a tokio RwLock write guard. build_index() is CPU-heavy (HNSW construction). Both are now offloaded via spawn_blocking + blocking_write(), matching the established pattern at serve/mod.rs:1249. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes all issues found in the full code review of
develop(v1.0.156):Stage 1 — Doc-comments + safe_canonicalize
repos.rs: corrected "Pure (no disk I/O)" doc-comments onrelocate_missingandprune_stale— both transitively perform disk I/O (filesystem traversal + git subprocess); callers in async contexts must usespawn_blockingserve/mod.rs: replaced rawstd::fs::canonicalizewithsafe_canonicalizeinreload_if_changedso Windows\?\UNC prefix is stripped before path comparisonStage 2 —
index/mod.rsqualityensure_hnsw_index_if_needed()(was inline, untestable); added 3 unit tests: unindexed-with-chunks rebuilds, already-indexed is idempotent, empty DB skipsmetadata.jsonschema consistency: normal (non-cancelled) path now writes"partial": falseso readers always see the field?propagations to log-and-continue (metadata write, FileMetaStore save, stats read) — keeps partial chunks searchable even if any recovery step failsStage 3 — Concurrency
evaluate_csharp_rebuild:bootstrap_last_changed(git subprocess + ≤10k entry fs-walk) was running while holdingconfig.write(), blocking all concurrent readers; fixed to run with no lock heldevaluate_csharp_rebuildcall site inrun_phase_2_csharp_scip: wrapped inspawn_blockingso the async runtime is not stalled during C# candidate scanwarmup_repoandadd_repo_handlerbackground task: twobuild_index()calls on async threads moved tospawn_blocking+blocking_write(), matching the established patternTest plan
cargo fmt --check— PASSEDcargo check --all-targets— PASSEDcargo clippy --all-targets -- -D warnings— PASSEDcargo test --lib— 432 passed, 0 failed (3 new tests added)🤖 Generated with Claude Code