🐛 fix: retry a conflicted repo instead of replaying the cached failure forever - #195
Merged
Merged
Conversation
…e forever A repo whose database failed to open (typically a transient write lock - an indexing run holding the DB when a query arrived) was cached as RepoState::Conflicted, and the fast path in get_or_open_stores replayed that error on every later call without ever retrying the open. The state's only documented exit was idle eviction, and it was unreachable: evict_idle_repos iterates last_access, but both paths that mark a repo Conflicted (warmup_repo and the get_or_open_stores slow path) propagate the failure with `?` before reaching their touch_access call. A repo that conflicts on first open therefore never gets a last_access entry and is never considered for eviction, however long it sits idle. Querying it did not help either: the fast path replayed the cached error while calling touch_access on the way, so the only queries that would have registered it for eviction were also the ones resetting its idle timer. Net effect: a momentary lock was indistinguishable from permanent corruption and could only be cleared by restarting serve - while the error text promised "the next query will retry automatically". A cached conflict is now dropped on next access and the open genuinely retried. Retrying is cheap when it still fails (a refused file lock), and this mirrors the missing-DB path, which already refuses to cache Conflicted for the same reason (missing_db_not_cached_as_conflicted). Regression test asserts recovery without a restart and without an idle wait. It carries two preconditions so it cannot pass vacuously (the first open must genuinely fail, and that failure must actually be cached as Conflicted), and was confirmed to FAIL with the fix neutralised - reproducing the exact user-visible error string. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Review-fixes: - [Important] Non-atomic get()+remove() on the Conflicted cache entry could race with a concurrent insert (e.g. add_repo_handler or force-reindex installing a fresh RepoState::Write) and delete that live entry instead, dropping its cancel_token without cancelling it. Fixed by using DashMap::remove_if with a matches!(v, RepoState::Conflicted) predicate — same atomic check-and-remove primitive already used by is_indexing() in this file — so removal can only ever affect an entry still Conflicted at removal time.
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
RepoState::Conflictedand that cached failure was replayed on every subsequent call forever — never retried, even after the lock was released.Conflicted(warmup_repoand theget_or_open_storesslow path) return early via?before reaching theirtouch_accesscall, so the repo never gets alast_accessentry and is therefore never picked up by the idle reaper (evict_idle_repos), regardless of how long it sits idle.get_or_open_storesnow atomically drops a cachedConflictedentry (viaDashMap::remove_if, matching the existingis_indexingpattern) and retries the open fresh on every call, instead of replaying the stale cached error.conflicted_repo_recovers_after_lock_released) that holds a real file lock, confirms the first open fails and cachesConflicted, releases the lock, and confirms the next call recovers without a restart or idle wait.Test plan