Skip to content

fix(mcp): coordinate background indexing per project - #316

Merged
Helweg merged 3 commits into
Helweg:mainfrom
Nicolas-nwb:codex/fix-mcp-background-worker-lease
Aug 23, 2026
Merged

fix(mcp): coordinate background indexing per project#316
Helweg merged 3 commits into
Helweg:mainfrom
Nicolas-nwb:codex/fix-mcp-background-worker-lease

Conversation

@Nicolas-nwb

Copy link
Copy Markdown
Contributor

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

  • Adds one background worker per project through a filesystem lease distinct from indexing.lock: only the leader starts file/Git watching and automatic indexing; other processes remain normal MCP readers.
  • A follower search no longer receives INDEX_BUSY merely because another process is refreshing the index. The leader handles the follower's refresh request, after which the follower can read the published snapshot.
  • Handles leader replacement after normal shutdown or process death through heartbeat renewal, dead-PID recovery, and follower promotion.
  • Requires no daemon, index migration, path change, or user configuration change.

Intentional limitation

Searches against a merely stale index (files-changed or branch-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 codex transports 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 preserving indexing.lock as the only mutation-safety lock. Manual index, --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

  • No MCP tool schema, name, argument, response format, index format, index path, migration, global configuration, or local runtime installation changes.
  • Worktree isolation remains based on the existing absolute-path/index identity rules.
  • autoIndex: false and watchFiles: false retain their existing semantics.
  • Stdio lifecycle coverage includes transport close and signal-driven teardown.

Validation

  • Added real multi-process Codex stdio coverage for one leader, one watcher, one background embedding batch, follower refresh, clean promotion, dead-leader recovery, explicit follower indexing, and lease expiry.
  • Added coverage for all supported host modes, worktree paths, safe-project gating, watcher startup, and Pi repeated agent starts.
  • npm run build, npm run typecheck, npm run lint, and git diff --check pass.
  • The full Vitest suite passes serially: 102 files and 1,592 tests.
  • npm run test:run still hits the repository's known macOS FSEvents EMFILE limit 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.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/adapters/mcp/server.ts Outdated
Comment thread src/adapters/mcp/cli.ts
@Helweg Helweg added the bug Bug fix label Aug 20, 2026
Helweg
Helweg previously approved these changes Aug 20, 2026

@Helweg Helweg left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed locally: build, MCP CLI lifecycle smoke coverage, typecheck, lint, and the full 1,592-test suite pass. Approved pending required hosted checks.

@Helweg Helweg left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Helweg
Helweg dismissed their stale review August 20, 2026 10:59

Superseded by a request-changes review after confirming the two P1 inline findings.

@Helweg Helweg left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Nicolas-nwb

Copy link
Copy Markdown
Contributor Author

Full validation gate run on head 82ab3e7:

  • npm run build
  • npm run typecheck
  • npm run lint
  • npm run test:run ✅ (102 test files, 1,602 tests passed)

@Helweg Helweg left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Final re-review of head 82ab3e7 is clean. The two prior P1 lifecycle races have regression coverage, and the complete hosted CI workflow is green.

@Helweg
Helweg merged commit 0eb4cb4 into Helweg:main Aug 23, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants