fix: resident C# workspace pool can serve stale find_impact results after rebuild - #277
Merged
Merged
Conversation
flupkede
commented
Sep 21, 2026
flupkede
commented
Owner
- WorkspacePool::evict adds a per-solution generation counter to close a spawn-in-flight race so a stale resident workspace can never be installed after eviction.
- scip_ref_cache is now cleared unconditionally on both full and incremental rebuilds; previously only full rebuilds cleared it, so a newly-added reference to an already-cached symbol from a changed file could be missed.
- Old selective ref-cache invalidation logic removed as dead/insufficient.
…cted Evicting a stale `active_reindexes` marker only corrected what the TUI and the reindex guard believed: the background task kept running and kept the `Arc<SharedStores>` it captured, so the LMDB env and `.writer.lock` stayed held for the process lifetime. The repo then logged as idle and "DB closed" while every write failed with "Database is locked by another process" — observed on a repo left at 0 chunks for two days. Format recovery no longer logs "rebuild complete" over an index whose rebuild was cancelled, and idle eviction warns with the LMDB holder list when the env is still open behind a live index task. Review-fixes: - [Critical] Cancelling the index token made the post-build guards treat a still-registered repo as removed and delete its .codesearch.db -> self_clean_if_unregistered() gates the deletion on the alias being absent from config; applied at the force-reindex, incremental-reindex, add_repo and TUI guards plus restart_fsw (now self: &Arc<Self>). - [Important] confirm_rebuild_finished could report a healthy index as cancelled on the AlreadyRunning branch (leftover index_tasks entry) -> that branch no longer checks completion. - [Note] The cold-open FSW guard in get_or_open_stores keeps the token-only rule: that fn takes &self and cannot reach the config. Its token is the FSW token, which the stale-marker cleanup never cancels. Converting it would make &self viral across 8 callers. - [Important] self_clean_if_unregistered treated a poisoned config lock as "not registered" and would delete a live repo's DB dir -> the fallback is now unwrap_or(true), failing towards keeping the directory.
… MAP_FULL give-up
MDB_BAD_VALSIZE was read as "written by an older arroy/heed major" and answered
with a full wipe + rebuild. On 2026-09-17 the same five C# repos were wiped
twice: the second failure hit a DB this process created hours earlier, so the
error is write-side (LMDB rejects an empty or >511-byte key), not an old format.
Each misdiagnosis cost ~20 minutes of reindex per repo and re-armed itself.
- one format recovery per alias per process; a repeat is logged as a writer bug
- the raw LMDB error is logged, not just the conclusion
- every SCIP put carries its table name and the offending key's size
- all three MDB_MAP_FULL retry loops log their final give-up and each retry
- a failed insert hands back the chunk ids it consumed (the txn aborted)
Validation: cargo fmt clean, clippy -D warnings exit 0, 725 + 721 tests pass.
Defect proof: both new tests fail with the guards disabled (exit 101).
Review-fixes:
- [Critical] put_ctx context hid the MDB_* code from anyhow's plain Display, disabling the corruption classifier -> r...
- [Important] delete_chunks' give-up log lacked the batch size and its max-size branch was still warn! -> batch size ...
- [Important] the incremental per-csproj rebuild path still stringified its error with plain {} -> now {:#} as well
… exceed it The SCIP keys were simply too long. scip-csharp writes fully qualified parameter types into the symbol key, so real methods reach 912 bytes against LMDB's stock 511-byte limit -- that is every MDB_BAD_VALSIZE in the incident. - heed feature `longer-keys` (MDB_MAXKEYSIZE=0) derives the limit from the page size: ~1980 bytes. A build without the feature cannot read what this one writes. - a key still over env.max_key_size() is skipped with a counted warning instead of failing the whole rebuild; its position entries are skipped too, and both ref-cache write paths apply the same rule - test pins the feature: the key limit must be >= 1024 and a 1000-byte key must be writable Validation: cargo fmt clean, clippy -D warnings exit 0, 726 + 722 tests pass.
A resident Roslyn workspace (todo #115 pool) is loaded once per repo and reused across find_refs calls; nothing tied it to repo changes. After a symbol rebuild the LMDB definitions layer advances to the new HEAD, but the resident workspace could still answer from the pre-change compilation for up to the full idle TTL, with no warning surfaced (discovered via todo #165's BAYR.Aprimo end-to-end test: a newly extracted method's call site was missing from find_impact even though index_head_sha already matched current_head_sha). Fix: WorkspacePool::evict(solution), called unconditionally at the end of every successful rebuild (full or incremental) so the next find_refs respawns against current on-disk source. Known residual gap (todo #168): an incremental rebuild does not clear scip_ref_cache, so a symbol already cached before the change can still replay a stale cached answer on a cache hit. Full rebuilds already clear that cache; narrowing incremental invalidation to the affected symbols is unscoped follow-up work. Review-fixes: - [Important] evict() only removed an already-resident entry; a workspace whose spawn started before evict but installed after it would still become resident with stale content -> added a per-solution generation counter bumped unconditionally by evict(); find_refs re-checks it right before installing and, on mismatch, answers the caller from the freshly spawned client once without caching it resident. - [Important] evict()'s in-flight/doomed branch had no test coverage -> added two tests using a blocking mock client: one drives evict while a find_refs call is in flight (proves deferred kill fires exactly once on release), the other drives evict while a spawn is in flight (proves the post-race workspace is discarded, not installed resident). - [Debt] evict()'s doc comment had been inserted above enforce_admission's existing doc comment, leaving enforce_admission undocumented -> restored enforce_admission's comment on itself, gave evict its own.
Closes the residual gap noted in the previous commit and in AGENTS.md/ CHANGELOG.md: a full rebuild already cleared scip_ref_cache, but an incremental rebuild (the normal file-watcher path) did not, so a symbol already cached before the change could keep replaying its pre-change answer on every cache hit even after the resident-workspace eviction fix. Now cleared unconditionally in the incremental branch too — this cache is a pure derived artifact (re-resolved lazily on next lookup), so dropping it on any rebuild is always safe. New gated integration test (tests/symbols_csharp_test.rs, csharp_helper_integration feature): rebuilds the SmallSolution fixture, populates the cache for Calculator.Add via a real find_references_for_key call, runs an incremental rebuild, and asserts collect_uncached_symbol_keys shows the symbol uncached again. Review-fixes: - [Important] the test used Calculator.Add, whose definition site is inside the changed file — the pre-existing selective ref-cache invalidation (removed below) would have purged its cache entry anyway, so the test didn't discriminate the fix -> switched to Program.Main (defined in the untouched Main.cs, no cached reference into the changed file), which only a whole-table clear catches. - [Debt] the old selective ref-cache invalidation (definition-site + reference-site scan) was left in place beside the new blanket clear, making ~55 lines unreachable and its debug logs report constant zeros -> removed; the blanket clear strictly subsumes it (it cannot catch an existing cached symbol gaining a brand-new reference FROM the changed file, which the blanket clear does).
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.