fix(mcp): preserve concurrent MCP settings during initial creation (fixes #1371)

[easonLiangWorldedtech]

Tracking issue: #1385

Summary

Fixes #1371 (part of epic #1375).

MCP settings initialization has a check-then-create race: getMcpSettingsFilePath() did fileExistsAtPath() followed by an unconditional fs.writeFile of the empty stub. Two VS Code windows (separate extension-host processes) racing at startup both see the file as absent, and the second blind write truncates the first window's config to the 122-byte stub — silent data loss of all configured MCP servers.

Changes

• src/services/mcp/McpHub.ts — the stub creation in getMcpSettingsFilePath() now goes through safeWriteJson with a merge callback:
  • the merge read happens under the existing advisory lock, so any config written by a concurrent process after the existence check is visible to it;
  • if the on-disk content has a proper mcpServers object, it is preserved; otherwise the default stub is written;
  • the fast path is unchanged (file exists → no write at all), so no new writes, no mtime churn, no watcher-triggered MCP reloads.
• src/services/mcp/__tests__/McpHub.spec.ts:
  • regression test reproduces the interleaving: the existence check sees an absent file, but the locked read sees a concurrent window's config; asserts the creation write carries that config, not the stub. (This test fails against the old raw fs.writeFile implementation.)
  • the safeWriteJson spec mock now honors options.merge (it previously ignored options, which masked merge semantics for every test in this file).

Notes

• safeWriteJson was already the established pattern for this file (used by the add/remove-server flows); this change removes the one remaining raw fs.writeFile in McpHub.
• No suppression count changes; eslint --max-warnings=0 clean on both touched files.
• pnpm --dir src exec vitest run services/mcp/__tests__/McpHub.spec.ts → 64/64 passing.

This is an auto-generated comment: release notes by coderabbit.ai -->

Summary by CodeRabbit

• Bug Fixes
  • Improved reliability when creating MCP settings.
  • Preserves settings added concurrently instead of overwriting them.

end of auto-generated comment: release notes by coderabbit.ai -->

---

Update (CodeRabbit-sync from trial #1413): head 9dd9825c4 — mcp_settings merge array guard, spec-mock production error parity, unknown-safe error.code read (trial addenda 178e6f4 + 4fc14c4). Review context: trial PR #1413.

Review-gate re-trigger (2026-08-30): empty commit 046b78c (no code change) re-runs CI and CodeRabbit current-head review under the org new PR review gate; the code head remains 9dd9825.

Summary by CodeRabbit

• Bug Fixes

  • Improved handling of invalid MCP configuration values, including arrays used where server settings are expected.
  • Prevented configuration files from being overwritten when access is denied or another unrecoverable read error occurs.
  • Added safer defaults when configuration files are missing or incomplete.

• Tests

  • Expanded coverage for missing, malformed, and inaccessible MCP settings files.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 291052dc-0ce0-4395-b369-06e7aaa27996

📥 Commits

Reviewing files that changed from the base of the PR and between e11ef14 and 046b78c.

📒 Files selected for processing (2)

• src/services/mcp/McpHub.ts
• src/services/mcp/__tests__/McpHub.spec.ts

Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review.

---

📝 Walkthrough

Walkthrough

MCP settings initialization now uses merge-aware safeWriteJson behavior. Valid concurrent mcpServers configuration is preserved. Invalid values are replaced with an empty object, and non-recoverable read errors are propagated.

Changes

MCP settings preservation

Layer / File(s)	Summary
Locked settings creation src/services/mcp/McpHub.ts	getMcpSettingsFilePath now uses a merge callback that preserves non-array object values for mcpServers. Missing, non-object, and array values fall back to { mcpServers: {} }.
Settings creation validation src/services/mcp/__tests__/McpHub.spec.ts	The safeWriteJson mock models recoverable and non-recoverable read failures. Tests cover concurrent configuration preservation, default stub creation, invalid mcpServers values, and EACCES handling.

Estimated code review effort: 3 (Moderate) | ~15–30 minutes

Merge Risk: 🔵 Low · up to 046b7

The change prevents concurrent startup from overwriting configured MCP servers, but it may preserve malformed server entries that can lead to ambiguous initialization behavior. The PR is mergeable with explicit owner awareness or follow-up to align creation-time validation with the MCP settings schema.

Suggested reviewers: edelauna

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The changes address issue #1371 by replacing the unsafe initial write with locked, atomic safeWriteJson merging and by preserving valid concurrent mcpServers data. Regression tests cover the race, fal…
Out of Scope Changes check	✅ Passed	The production and test changes remain focused on the MCP settings initialization race described in issue #1371. The test mock updates and empty review-trigger commit support the same objective and in…
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2…
Title check	✅ Passed	The title clearly identifies the MCP race-condition fix and the preservation of concurrent settings.
Description check	✅ Passed	The description explains the issue, implementation, testing, and linked issue. It does not reproduce the full template or checklist, but it provides the critical information needed for review.

Full details: Linked Issues check

Explanation

The changes address issue #1371 by replacing the unsafe initial write with locked, atomic safeWriteJson merging and by preserving valid concurrent mcpServers data. Regression tests cover the race, fallback cases, arrays, and read errors.

Full details: Out of Scope Changes check

Explanation

The production and test changes remain focused on the MCP settings initialization race described in issue #1371. The test mock updates and empty review-trigger commit support the same objective and introduce no unrelated code changes.

Full details: Docstring Coverage

Explanation

No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2 files.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[easonLiangWorldedtech]

Patch coverage gap (Codecov: 2 lines, 1 missing + 1 partial in the merge callback) addressed in 332253a: added the three remaining fallback cases of getMcpSettingsFilePath (absent file, existing content without an mcpServers object, and mcpServers present but not an object). All changed lines and all merge-callback branches are now covered locally (v8 lcov: lines 519/520/521/523 all hit; BRDA outcomes all > 0); 67/67 McpHub spec tests pass.

[github-actions]

Review process

Thanks for contributing. This comment tracks the review sequence and the next action.

1. Required CI checks pass.
2. The workflow starts CodeRabbit automatically.
3. For eligible human-authored PRs, CodeRabbit reviews and approves the latest commit.
4. A human maintainer reviews and approves after CodeRabbit.

Current step: Required CI passed. Wait for CodeRabbit to approve the latest commit.