fix(mcp): coordinate background indexing per project - #316
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d0d9e37e9c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Helweg
left a comment
There was a problem hiding this comment.
Reviewed locally: build, MCP CLI lifecycle smoke coverage, typecheck, lint, and the full 1,592-test suite pass. Approved pending required hosted checks.
Helweg
left a comment
There was a problem hiding this comment.
Withdrawing my earlier approval after reviewing the Codex inline findings. Both P1 reports need to be resolved: (1) preserve MCP ownership/reference through asynchronous background-worker teardown so a concurrently joining server is retained and configured, and (2) connect the stdio MCP transport before awaiting watcher readiness so initialization cannot block on watcher startup. Please add regression coverage for both races.
Superseded by a request-changes review after confirming the two P1 inline findings.
Helweg
left a comment
There was a problem hiding this comment.
Re-reviewed current head 82ab3e74e23c73095585ce7ffc0ab44d9f0b9cfa after the P1 feedback.
The two reported races now appear addressed in source and targeted regression coverage:
- MCP ownership remains joinable during teardown through
mcpWorkerTeardowns, and a replacement requests restart. - The stdio transport connects before background watcher readiness is awaited.
Before approval, please run and report the full validation gate on this final head: npm run build && npm run typecheck && npm run lint && npm run test:run. The PR currently exposes only the release-draft check, so I cannot independently confirm the full suite for the post-fix commit.
|
Full validation gate run on head 82ab3e7:
|
Problem
Each MCP conversation opened on the same project starts its own stdio process, and each process used to start its own watcher and auto-indexer. Because coordination was in memory, nothing was deduplicated across processes. On a real-sized project, this multiplied MCP processes and saturated CPU without useful WAL writes or embedding requests.
What this PR fixes
indexing.lock: only the leader starts file/Git watching and automatic indexing; other processes remain normal MCP readers.INDEX_BUSYmerely because another process is refreshing the index. The leader handles the follower's refresh request, after which the follower can read the published snapshot.Intentional limitation
Searches against a merely stale index (
files-changedorbranch-changed) remain blocked deliberately. SQLite, vectors, and BM25 are not atomically published as one generation, so serving a stale snapshot could mix partial states. Atomic multi-artifact publication is outside this PR's scope.Reproduction and design
Starting two
node dist/cli.js --host codextransports for the same project previously created two independent in-memory coordinators, watchers, and automatic indexing attempts. The new canonical project-root plus index-path lease elects one leader while preservingindexing.lockas the only mutation-safety lock. Manualindex,--force,--estimate-only,--config, and host behavior remain independent of the background lease.The lifecycle integration covers MCP, OpenCode, and Pi. Followers never start background watchers or jobs, retry bounded election after a leader exits, and can still request a refresh from the elected leader. Shutdown stops watcher coordination before releasing the lease, while an unfinished indexing job retains its lease until process exit makes safe takeover possible.
Compatibility
autoIndex: falseandwatchFiles: falseretain their existing semantics.Validation
npm run build,npm run typecheck,npm run lint, andgit diff --checkpass.npm run test:runstill hits the repository's known macOS FSEventsEMFILElimit in four watcher tests when it runs in parallel. The same tests pass in the serial full-suite run.No installation or user reconfiguration is required.