From e11ef14d469b582310e6d347d088fa3f21862070 Mon Sep 17 00:00:00 2001
From: Eason Liang <easonliang28@gmail.com>
Date: Thu, 27 Aug 2026 09:30:22 +0800
Subject: [PATCH] fix(mcp): preserve concurrent MCP settings during initial
 creation (fixes #1371)

getMcpSettingsFilePath() created the default mcp_settings.json with a check-then-write: fileExistsAtPath() followed by an unconditional fs.writeFile of the empty stub. Two windows racing at startup both saw the file as absent, and the second blind write truncated the first window's config to the 122-byte stub.

The stub write now goes through safeWriteJson with a merge callback: the read happens under the advisory lock, and any config already on disk (written by a concurrent process after the existence check) is preserved instead of clobbered. The fast path (file exists -> no write) is unchanged, so no watcher-triggered reloads or write amplification.

Test: regression test reproduces the interleaving (existence check sees absent file, locked read sees the concurrent config) and asserts the creation write carries the concurrent config, not the stub. The safeWriteJson spec mock now honors options.merge.
---
 src/services/mcp/McpHub.ts                | 22 +++++---
 src/services/mcp/__tests__/McpHub.spec.ts | 62 ++++++++++++++++++++---
 2 files changed, 72 insertions(+), 12 deletions(-)

diff --git a/src/services/mcp/McpHub.ts b/src/services/mcp/McpHub.ts
index 1374e430fe..5d6c31a3e5 100644
--- a/src/services/mcp/McpHub.ts
+++ b/src/services/mcp/McpHub.ts
@@ -506,13 +506,23 @@ export class McpHub {
 		)
 		const fileExists = await fileExistsAtPath(mcpSettingsFilePath)
 		if (!fileExists) {
-			await fs.writeFile(
+			// Create the default settings file under the advisory lock. The merge
+			// callback preserves any config a concurrent process wrote between the
+			// existence check above and the locked read, instead of blindly
+			// truncating it (see #1371).
+			await safeWriteJson(
 				mcpSettingsFilePath,
-				`{
-  "mcpServers": {
-
-  }
-}`,
+				{ mcpServers: {} },
+				{
+					prettyPrint: true,
+					merge: (existing) => {
+						const parsed = existing as { mcpServers?: unknown } | null
+						if (parsed && parsed.mcpServers && typeof parsed.mcpServers === "object") {
+							return existing
+						}
+						return { mcpServers: {} }
+					},
+				},
 			)
 		}
 		return mcpSettingsFilePath
diff --git a/src/services/mcp/__tests__/McpHub.spec.ts b/src/services/mcp/__tests__/McpHub.spec.ts
index 96589d8dd6..cab1501b53 100644
--- a/src/services/mcp/__tests__/McpHub.spec.ts
+++ b/src/services/mcp/__tests__/McpHub.spec.ts
@@ -1,4 +1,5 @@
 import * as fs from "fs/promises"
+import * as path from "path"
 
 import type { Mock } from "vitest"
 import type { ExtensionContext, Uri } from "vscode"
@@ -34,12 +35,32 @@ import { safeWriteJson } from "../../../utils/safeWriteJson"
 
 // Mock safeWriteJson
 vi.mock("../../../utils/safeWriteJson", () => ({
-	safeWriteJson: vi.fn(async (filePath, data) => {
-		// Instead of trying to write to the file system, just call fs.writeFile mock
-		// This avoids the complex file locking and temp file operations
-		const fs = await import("fs/promises")
-		return fs.writeFile(filePath, JSON.stringify(data), "utf8")
-	}),
+	safeWriteJson: vi.fn(
+		async (
+			filePath: string,
+			data: unknown,
+			options?: { merge?: (existing: unknown, incoming: unknown) => unknown },
+		) => {
+			// Instead of trying to write to the file system, just call fs.writeFile mock
+			// This avoids the complex file locking and temp file operations.
+			// When a merge callback is provided, honor it: read the current on-disk
+			// content via the fs.readFile mock (simulating the read under the lock)
+			// and let the callback decide the final value.
+			let value = data
+			if (options?.merge) {
+				let existing: unknown = null
+				try {
+					const fs = await import("fs/promises")
+					existing = JSON.parse(await fs.readFile(filePath, "utf8"))
+				} catch {
+					existing = null
+				}
+				value = options.merge(existing, data)
+			}
+			const fs = await import("fs/promises")
+			return fs.writeFile(filePath, JSON.stringify(value), "utf8")
+		},
+	),
 }))
 
 vi.mock("delay", () => ({ default: vi.fn().mockResolvedValue(undefined) }))
@@ -212,6 +233,35 @@ describe("McpHub", () => {
 		watchSpy.mockRestore()
 	})
 
+	describe("getMcpSettingsFilePath", () => {
+		it("preserves a config written by a concurrent process during initial creation (#1371)", async () => {
+			const settingsPath = path.join("/mock/settings/path", "mcp_settings.json")
+			const concurrentConfig = {
+				mcpServers: {
+					"concurrent-server": { type: "stdio", command: "node", args: ["server.js"] },
+				},
+			}
+
+			// Window A's existence check sees the settings file as absent...
+			// (One-shot overrides: the factory defaults apply to all other tests.)
+			vi.mocked(fs.access).mockRejectedValueOnce(
+				Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }),
+			)
+			// ...but by the time the locked read runs (safeWriteJson merge),
+			// window B's config is already on disk.
+			vi.mocked(fs.readFile).mockResolvedValueOnce(JSON.stringify(concurrentConfig))
+
+			const returnedPath = await mcpHub.getMcpSettingsFilePath()
+
+			expect(returnedPath).toBe(settingsPath)
+			// The creation write must carry the concurrent config, not the empty stub.
+			expect(fs.writeFile).toHaveBeenCalledTimes(1)
+			const [writtenPath, writtenData] = vi.mocked(fs.writeFile).mock.calls[0]
+			expect(writtenPath).toBe(settingsPath)
+			expect(JSON.parse(writtenData as string)).toEqual(concurrentConfig)
+		})
+	})
+
 	describe("Discriminated union type handling", () => {
 		it("should create connected connections with proper type", async () => {
 			// Mock StdioClientTransport