fix(worker): re-index repos whose zoekt shards are missing from disk - #1304
Closed
rudraasakariya wants to merge 2 commits into
Closed
Conversation
Copy link
Copy Markdown
Sorry, something went wrong.
When the index directory is lost (e.g., it lives on ephemeral storage in a k8s deployment) while the database still marks repos as indexed, search silently returns empty results and nothing triggers a rebuild until the reindex interval elapses. Add a reconciliation step that runs on scheduler startup and on every scheduler poll: repos marked as indexed in the DB that have no index shards on disk get their indexedAt cleared, so the existing scheduler re-indexes them with its usual dedup and backoff guards.
Copy link
Copy Markdown
Contributor
WalkthroughRepoIndexManager detects repos marked as indexed in the database but missing corresponding shard files on disk, clears their indexedAt status on startup and during scheduler polling, and allows the scheduler to re-index them. The changelog documents the fix and test infrastructure validates the behavior. ChangesMissing Shard Reconciliation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches 🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agentsVerify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/backend/src/repoIndexManager.ts`:
- Around line 718-720: Replace the two-step existsSync + readdir pattern so the
directory scan tolerates a race: call readdir(INDEX_CACHE_DIR) directly inside a
try/catch around the call used to populate entries (the block that constructs
repoIdsWithShards), and if the catch error.code === 'ENOENT' treat it as an
empty directory (i.e. leave repoIdsWithShards empty) instead of aborting the
poll; for other errors rethrow or log as before. Ensure you update the logic
that uses entries to handle the empty-case correctly.
- Around line 702-744: staleRepos were selected from a snapshot but the
subsequent updateMany only filters by id, which can clear indexedAt incorrectly
if a repo lost its connections or its indexedAt changed; instead, re-apply the
preconditions at write time by updating only rows that still match both the
original indexedAt value and still have connections: for example iterate
staleRepos and for each call this.db.repo.updateMany (or a single updateMany
with a where: { OR: [...] }) where each clause is { id: repo.id, indexedAt:
repo.indexedAt, connections: { some: {} } } so you only set indexedAt: null when
the repo still has the same indexedAt and at least one connection.
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info ⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8047b0d6-9b12-4aaf-a6bd-3c3de86c7d38
📥 CommitsReviewing files that changed from the base of the PR and between 90a5afe and 5e1624e.
📒 Files selected for processing (3)- CHANGELOG.md
- packages/backend/src/repoIndexManager.test.ts
- packages/backend/src/repoIndexManager.ts
Sorry, something went wrong.
Comment thread
packages/backend/src/repoIndexManager.ts
Comment on lines
+702
to
+744
| const indexedRepos = await this.db.repo.findMany({ | ||
| where: { | ||
| indexedAt: { not: null }, | ||
| indexedCommitHash: { not: null }, | ||
| connections: { some: {} }, | ||
| }, | ||
| select: { | ||
| id: true, | ||
| name: true, | ||
| }, | ||
| }); | ||
|
|
||
| if (indexedRepos.length === 0) { | ||
| return; | ||
| } | ||
|
|
||
| const repoIdsWithShards = new Set<number>(); | ||
| if (existsSync(INDEX_CACHE_DIR)) { | ||
| const entries = await readdir(INDEX_CACHE_DIR); | ||
| for (const entry of entries) { | ||
| // Ignore temporary files (e.g., `.tmp` files from in-flight or | ||
| // failed indexing operations) - only completed shards count. | ||
| if (!entry.endsWith('.zoekt')) { | ||
| continue; | ||
| } | ||
| const repoId = getRepoIdFromShardFileName(entry); | ||
| if (repoId !== undefined) { | ||
| repoIdsWithShards.add(repoId); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| const staleRepos = indexedRepos.filter(repo => !repoIdsWithShards.has(repo.id)); | ||
| if (staleRepos.length === 0) { | ||
| return; | ||
| } | ||
|
|
||
| logger.warn(`Found ${staleRepos.length} repo(s) marked as indexed but with no index shards on disk. Marking as stale for re-indexing: ${staleRepos.map(repo => repo.name).join(', ')}`); | ||
|
|
||
| await this.db.repo.updateMany({ | ||
| where: { id: { in: staleRepos.map(repo => repo.id) } }, | ||
| data: { indexedAt: null }, | ||
| }); |
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment.
⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
Re-apply the stale-marking preconditions at write time.
staleRepos is derived from a snapshot, but Line 741 later clears indexedAt by id only. If a repo loses its last connection or finishes a reindex after the findMany() but before updateMany(), this write can either bypass the GC grace period or clobber a freshly-written indexedAt, which then triggers an unnecessary extra reindex. The update should re-check the repo is still connected and still has the same indexedAt value that was observed during selection.
Suggested fix const indexedRepos = await this.db.repo.findMany({
where: {
indexedAt: { not: null },
indexedCommitHash: { not: null },
connections: { some: {} },
},
select: {
id: true,
name: true,
+ indexedAt: true,
},
});
@@
- await this.db.repo.updateMany({
- where: { id: { in: staleRepos.map(repo => repo.id) } },
- data: { indexedAt: null },
- });
+ await this.db.$transaction(
+ staleRepos.map((repo) =>
+ this.db.repo.updateMany({
+ where: {
+ id: repo.id,
+ indexedAt: repo.indexedAt,
+ indexedCommitHash: { not: null },
+ connections: { some: {} },
+ },
+ data: { indexedAt: null },
+ })
+ )
+ );‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const indexedRepos = await this.db.repo.findMany({ | |
| where: { | |
| indexedAt: { not: null }, | |
| indexedCommitHash: { not: null }, | |
| connections: { some: {} }, | |
| }, | |
| select: { | |
| id: true, | |
| name: true, | |
| }, | |
| }); | |
| if (indexedRepos.length === 0) { | |
| return; | |
| } | |
| const repoIdsWithShards = new Set<number>(); | |
| if (existsSync(INDEX_CACHE_DIR)) { | |
| const entries = await readdir(INDEX_CACHE_DIR); | |
| for (const entry of entries) { | |
| // Ignore temporary files (e.g., `.tmp` files from in-flight or | |
| // failed indexing operations) - only completed shards count. | |
| if (!entry.endsWith('.zoekt')) { | |
| continue; | |
| } | |
| const repoId = getRepoIdFromShardFileName(entry); | |
| if (repoId !== undefined) { | |
| repoIdsWithShards.add(repoId); | |
| } | |
| } | |
| } | |
| const staleRepos = indexedRepos.filter(repo => !repoIdsWithShards.has(repo.id)); | |
| if (staleRepos.length === 0) { | |
| return; | |
| } | |
| logger.warn(`Found ${staleRepos.length} repo(s) marked as indexed but with no index shards on disk. Marking as stale for re-indexing: ${staleRepos.map(repo => repo.name).join(', ')}`); | |
| await this.db.repo.updateMany({ | |
| where: { id: { in: staleRepos.map(repo => repo.id) } }, | |
| data: { indexedAt: null }, | |
| }); | |
| const indexedRepos = await this.db.repo.findMany({ | |
| where: { | |
| indexedAt: { not: null }, | |
| indexedCommitHash: { not: null }, | |
| connections: { some: {} }, | |
| }, | |
| select: { | |
| id: true, | |
| name: true, | |
| indexedAt: true, | |
| }, | |
| }); | |
| if (indexedRepos.length === 0) { | |
| return; | |
| } | |
| const repoIdsWithShards = new Set<number>(); | |
| if (existsSync(INDEX_CACHE_DIR)) { | |
| const entries = await readdir(INDEX_CACHE_DIR); | |
| for (const entry of entries) { | |
| // Ignore temporary files (e.g., `.tmp` files from in-flight or | |
| // failed indexing operations) - only completed shards count. | |
| if (!entry.endsWith('.zoekt')) { | |
| continue; | |
| } | |
| const repoId = getRepoIdFromShardFileName(entry); | |
| if (repoId !== undefined) { | |
| repoIdsWithShards.add(repoId); | |
| } | |
| } | |
| } | |
| const staleRepos = indexedRepos.filter(repo => !repoIdsWithShards.has(repo.id)); | |
| if (staleRepos.length === 0) { | |
| return; | |
| } | |
| logger.warn(`Found ${staleRepos.length} repo(s) marked as indexed but with no index shards on disk. Marking as stale for re-indexing: ${staleRepos.map(repo => repo.name).join(', ')}`); | |
| await this.db.$transaction( | |
| staleRepos.map((repo) => | |
| this.db.repo.updateMany({ | |
| where: { | |
| id: repo.id, | |
| indexedAt: repo.indexedAt, | |
| indexedCommitHash: { not: null }, | |
| connections: { some: {} }, | |
| }, | |
| data: { indexedAt: null }, | |
| }) | |
| ) | |
| ); |
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/backend/src/repoIndexManager.ts` around lines 702 - 744, staleRepos
were selected from a snapshot but the subsequent updateMany only filters by id,
which can clear indexedAt incorrectly if a repo lost its connections or its
indexedAt changed; instead, re-apply the preconditions at write time by updating
only rows that still match both the original indexedAt value and still have
connections: for example iterate staleRepos and for each call
this.db.repo.updateMany (or a single updateMany with a where: { OR: [...] })
where each clause is { id: repo.id, indexedAt: repo.indexedAt, connections: {
some: {} } } so you only set indexedAt: null when the repo still has the same
indexedAt and at least one connection.
Sorry, something went wrong.
Comment thread
packages/backend/src/repoIndexManager.ts
Comment on lines
+718
to
+720
| const repoIdsWithShards = new Set<number>(); | ||
| if (existsSync(INDEX_CACHE_DIR)) { | ||
| const entries = await readdir(INDEX_CACHE_DIR); |
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment.
⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Handle ENOENT from the directory scan directly.
If INDEX_CACHE_DIR disappears between Line 719 and Line 720, readdir() throws and this poll aborts instead of recovering by marking repos stale. That is the exact failure mode this reconciliation is meant to survive. Prefer calling readdir() directly and treating ENOENT as “no shards present”.
Suggested fix- const repoIdsWithShards = new Set<number>();
- if (existsSync(INDEX_CACHE_DIR)) {
- const entries = await readdir(INDEX_CACHE_DIR);
- for (const entry of entries) {
- // Ignore temporary files (e.g., `.tmp` files from in-flight or
- // failed indexing operations) - only completed shards count.
- if (!entry.endsWith('.zoekt')) {
- continue;
- }
- const repoId = getRepoIdFromShardFileName(entry);
- if (repoId !== undefined) {
- repoIdsWithShards.add(repoId);
- }
- }
- }
+ const repoIdsWithShards = new Set<number>();
+ let entries: string[] = [];
+ try {
+ entries = await readdir(INDEX_CACHE_DIR);
+ } catch (error) {
+ if ((error as NodeJS.ErrnoException).code !== 'ENOENT') {
+ throw error;
+ }
+ }
+
+ for (const entry of entries) {
+ // Ignore temporary files (e.g., `.tmp` files from in-flight or
+ // failed indexing operations) - only completed shards count.
+ if (!entry.endsWith('.zoekt')) {
+ continue;
+ }
+ const repoId = getRepoIdFromShardFileName(entry);
+ if (repoId !== undefined) {
+ repoIdsWithShards.add(repoId);
+ }
+ }‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const repoIdsWithShards = new Set<number>(); | |
| if (existsSync(INDEX_CACHE_DIR)) { | |
| const entries = await readdir(INDEX_CACHE_DIR); | |
| const repoIdsWithShards = new Set<number>(); | |
| let entries: string[] = []; | |
| try { | |
| entries = await readdir(INDEX_CACHE_DIR); | |
| } catch (error) { | |
| if ((error as NodeJS.ErrnoException).code !== 'ENOENT') { | |
| throw error; | |
| } | |
| } | |
| for (const entry of entries) { | |
| // Ignore temporary files (e.g., `.tmp` files from in-flight or | |
| // failed indexing operations) - only completed shards count. | |
| if (!entry.endsWith('.zoekt')) { | |
| continue; | |
| } | |
| const repoId = getRepoIdFromShardFileName(entry); | |
| if (repoId !== undefined) { | |
| repoIdsWithShards.add(repoId); | |
| } | |
| } |
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/backend/src/repoIndexManager.ts` around lines 718 - 720, Replace the two-step existsSync + readdir pattern so the directory scan tolerates a race: call readdir(INDEX_CACHE_DIR) directly inside a try/catch around the call used to populate entries (the block that constructs repoIdsWithShards), and if the catch error.code === 'ENOENT' treat it as an empty directory (i.e. leave repoIdsWithShards empty) instead of aborting the poll; for other errors rethrow or log as before. Ensure you update the logic that uses entries to handle the empty-case correctly.
Sorry, something went wrong.
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
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.
Fixes #1210
Problem
If the zoekt index directory is deleted while the database and repo clones remain intact (e.g., .sourcebot/index placed on ephemeral/node-local storage in a Kubernetes deployment, lost on pod replacement), Sourcebot starts normally but repos stay marked as indexed in the DB. No shard files exist, so search silently returns empty/incomplete results, and nothing schedules a rebuild until reindexIntervalMs elapses.
Fix
Adds a reconciliation step, markReposWithMissingShardsAsStale, to RepoIndexManager. It runs on scheduler startup and on every scheduler poll (so an index directory lost mid-run is also caught), and:
Design notes:
Test plan
Summary by CodeRabbit