fix(index): safely seed nested worktree indexes - #178
Merged
Conversation
…-embedding Working in a fresh git worktree of an already-indexed repo re-embedded every file from scratch (~minutes on a local embedder) instead of reusing a sibling worktree's embeddings. Two independent bugs defeated the existing donor-seeding path, and both bite Claude Code's default repo/.claude/worktrees/<name> layout. Bug 1 — donor discovery picked the wrong worktree (internal/config/seed.go). FindDonorIndexBase selected the FIRST `git worktree list` entry containing the project. git lists the main checkout first, so for a worktree nested inside the repo it identified the main checkout as "self", searched for donors at nonexistent <sibling>/.claude/worktrees/<name> paths, and skipped the one real donor (the parent repo's index). Fix: pick the deepest (most specific) containing worktree — the longest matching path, since every match is an ancestor of the project and they form a prefix chain. Bug 2 — the CLI indexer never seeded (cmd/index.go, cmd/seed.go). Seeding only ran in the MCP search handler, but the SessionStart hook spawns `lumen index`, which created the DB first; SeedFromDonor then no-ops because the DB exists, so the hook permanently won the race and forced a full rebuild. Fix: seed from a donor in runIndexer, under the index lock, before the DB is created. Because both the CLI indexer and the MCP handler can now seed the same fresh worktree concurrently, harden SeedFromDonor (internal/index/seed.go) to copy to a unique temp file (os.CreateTemp) and publish via a create-if-absent hard link (os.Link fails on EEXIST). The loser of the race no-ops instead of renaming a fresh copy over a database the winner already opened for writing. Adds unit tests for the nested-worktree layout, concurrent seeding, and the runIndexer seed helper. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- SeedFromDonor: copy into the open temp descriptor instead of closing and re-opening it by name (avoids a Windows sharing violation), defer the descriptor's close, and check the Close error before publishing via os.Link so a short write can't be linked into place. Removes the now-unused copyFile. - cmd/seed_test.go: consolidate the four seedFromDonorIfNew cases into a single table-driven test, per the repo's Go testing guideline. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe indexer now performs optional donor-index seeding before indexing. Seeding supports nested worktrees, cancellation, concurrent processes, SQLite metadata updates, and non-overwriting publication. ChangesDonor-index seeding
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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. Comment |
aeneasr
marked this pull request as ready for review
August 6, 2026 12:40
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
cmd/seed_test.go (1)
32-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the status messages for the success paths.
The table only checks
statuseswhen the seed stub fails. The branches inseedFromDonorIfNewat lines 82-90 ofcmd/seed.gostay untested: the seeded case, the "Index was seeded by another process." case, and the "Sibling index could not be reused" case. AwantStatuses []stringfield on the table covers all of them.♻️ Proposed test extension
tests := []struct { name string setupDB bool // pre-create the destination DB donor string seedErr error // error returned by the seed stub wantFind bool // donor discovery should run wantSeed bool // seed should run + wantStatuses []string }{{ name: "seeds when DB missing and donor found", donor: "/donor.db", wantFind: true, wantSeed: true, + wantStatuses: []string{ + "Seeding index from sibling worktree...", + "Seeded index from sibling worktree.", + }, },} else if warning != "" { t.Errorf("unexpected warning: %q", warning) } + if tt.wantStatuses != nil && !slices.Equal(statuses, tt.wantStatuses) { + t.Errorf("statuses = %v, want %v", statuses, tt.wantStatuses) + }Also applies to: 114-123
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/seed_test.go` around lines 32 - 66, Extend the seedFromDonorIfNew test table with a wantStatuses []string field and assert the collected statuses for every case, including successful seeding, an index seeded by another process, and donor-reuse failure. Populate expected status messages for each branch while preserving the existing seed and donor-discovery assertions.internal/indexlock/lock.go (1)
57-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReturn a sentinel error instead of an inline string.
errors.New("lock not acquired")allocates a new, uncomparable error on every call. Callers cannot detect this condition. Declare a package-level sentinel so callers can useerrors.Is.The coding guidelines require proper error types instead of generic error strings.
♻️ Proposed refactor
+// ErrNotAcquired reports that the lock could not be taken. +var ErrNotAcquired = errors.New("index lock not acquired") + // Acquire waits for an exclusive lock on lockPath or for ctx to be cancelled.if err := ctx.Err(); err != nil { return nil, err } - return nil, errors.New("lock not acquired") + return nil, ErrNotAcquired🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/indexlock/lock.go` around lines 57 - 63, Replace the inline errors.New("lock not acquired") return in the lock acquisition flow with a package-level sentinel error, and declare that sentinel in internal/indexlock. Return the sentinel unchanged so callers can reliably detect the condition with errors.Is, while preserving the existing ctx.Err() handling.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify 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 `@cmd/stdio.go`:
- Around line 497-500: Update the seeding call in getOrCreate to pass the
indexerCache’s closeCtx instead of context.Background(), ensuring
SeedFromDonorContext can be cancelled when Close() runs. Thread this existing
cancellation context through the asynchronous seed operation; optionally apply a
bounded timeout if consistent with the cache’s locking behavior.
In `@internal/index/seed.go`:
- Around line 89-126: Replace the checkpoint-and-io.Copy donor snapshot flow in
the seed creation function with a transactionally consistent SQLite snapshot,
preferably using VACUUM INTO on the donor connection and removing the
now-unnecessary writable donor handle and checkpoint. Ensure the generated seed
is written to the existing temporary path and only published after the operation
succeeds; alternatively, hold indexlock.LockPathForDB(donorPath) for the entire
copy.
---
Nitpick comments:
In `@cmd/seed_test.go`:
- Around line 32-66: Extend the seedFromDonorIfNew test table with a
wantStatuses []string field and assert the collected statuses for every case,
including successful seeding, an index seeded by another process, and
donor-reuse failure. Populate expected status messages for each branch while
preserving the existing seed and donor-discovery assertions.
In `@internal/indexlock/lock.go`:
- Around line 57-63: Replace the inline errors.New("lock not acquired") return
in the lock acquisition flow with a package-level sentinel error, and declare
that sentinel in internal/indexlock. Return the sentinel unchanged so callers
can reliably detect the condition with errors.Is, while preserving the existing
ctx.Err() handling.
🪄 Autofix
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 Plus
Run ID: 1a0b92c9-3618-4bb9-b63c-23cee3e32050
📒 Files selected for processing (10)
cmd/index.gocmd/seed.gocmd/seed_test.gocmd/stdio.gocmd/stdio_test.gointernal/config/seed.gointernal/config/seed_test.gointernal/index/seed.gointernal/index/seed_test.gointernal/indexlock/lock.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify 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 `@cmd/stdio_test.go`:
- Around line 1378-1380: Update the test around getOrCreate so it retrieves the
index.Indexer written to created after receiving getDone, then closes that
indexer before the test exits. Preserve the existing error assertion and ensure
cleanup occurs even when subsequent test logic fails.
🪄 Autofix
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 Plus
Run ID: 13aae39b-859e-4b08-886d-bbb045d1ab80
📒 Files selected for processing (4)
cmd/stdio.gocmd/stdio_test.gointernal/index/seed.gointernal/index/seed_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/stdio.go
- internal/index/seed.go
Comment on lines
+1378
to
+1380
| if err := <-getDone; err != nil { | ||
| t.Fatalf("getOrCreate: %v", err) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Close the indexer that getOrCreate returns.
getOrCreate creates a real index.Indexer after seeding is cancelled. The test discards it and never closes it. The SQLite handle stays open for the remainder of the test binary, and t.TempDir cleanup can fail on Windows because of the open file.
🧹 Proposed fix to release the indexer
getDone := make(chan error, 1)
+ var created *index.Indexer
go func() {
- _, _, _, err := ic.getOrCreate(projectDir, "")
+ idx, _, _, err := ic.getOrCreate(projectDir, "")
+ created = idx
getDone <- err
}() if err := <-getDone; err != nil {
t.Fatalf("getOrCreate: %v", err)
}
+ if created != nil {
+ _ = created.Close()
+ }
}The write to created happens before the send on getDone, and the read happens after the receive, so no data race exists.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/stdio_test.go` around lines 1378 - 1380, Update the test around
getOrCreate so it retrieves the index.Indexer written to created after receiving
getDone, then closes that indexer before the test exits. Preserve the existing
error assertion and ensure cleanup occurs even when subsequent test logic fails.
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
2 participants
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.
Seeds new nested-worktree indexes from the deepest indexed sibling for both CLI and MCP paths, avoiding full re-embedding.
Hardens publication with context-aware advisory locking, SQLite WAL checkpointing, a portable rename fallback, bounded temp cleanup, and correct
project_pathmetadata.Skips donor copies for
--forceand reports interactive seed status while preserving MCP warnings.Tests:
make test;make lint.Summary by CodeRabbit
New Features
Bug Fixes