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

src/services/mcp/McpHub.ts

@@ -506,13 +506,32 @@
 		)
 		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
+						// Arrays satisfy `typeof === "object"` but are not a valid
+						// mcpServers map; preserve only a plain object, otherwise the
+						// file would be rewritten with a value McpSettingsSchema
+						// rejects on the next load.
+						if (
+							parsed &&
+							parsed.mcpServers &&
+							!Array.isArray(parsed.mcpServers) &&
+							typeof parsed.mcpServers === "object"
+						) {
+							return existing
+						}
+						return { mcpServers: {} }
+					},
+				},
 			)
 		}
 		return mcpSettingsFilePath

src/services/mcp/__tests__/McpHub.settingsCreation.integration.spec.ts

@@ -0,0 +1,176 @@
+import * as fs from "fs/promises"
+import * as os from "os"
+import * as path from "path"
+
+import * as lockfile from "proper-lockfile"
+
+import type { ClineProvider } from "../../../core/webview/ClineProvider"
+
+import { GlobalFileNames } from "../../../shared/globalFileNames"
+import { McpHub } from "../McpHub"
+
+// McpHub loads the vscode module at import time. Watcher setup is skipped
+// because Vitest runs with NODE_ENV=test; the stub only keeps the module
+// graph loadable.
+vi.mock("vscode", () => ({
+	workspace: {
+		createFileSystemWatcher: vi.fn().mockReturnValue({
+			onDidChange: vi.fn(),
+			onDidCreate: vi.fn(),
+			onDidDelete: vi.fn(),
+			dispose: vi.fn(),
+		}),
+		onDidChangeWorkspaceFolders: vi.fn(),
+		workspaceFolders: [],
+	},
+	window: {
+		showErrorMessage: vi.fn(),
+		showInformationMessage: vi.fn(),
+		showWarningMessage: vi.fn(),
+	},
+	ProgressLocation: { Notification: 15 },
+	Disposable: { from: vi.fn() },
+}))
+
+// Pass-through spy on the real proper-lockfile. Production safeWriteJson and
+// this test share the same instrumented lock function, so the advisory lock,
+// its retry contention, and its release behavior all stay real.
+vi.mock("proper-lockfile", async () => {
+	const actual = await vi.importActual<typeof import("proper-lockfile")>("proper-lockfile")
+	return { ...actual, lock: vi.fn(actual.lock) }
+})
+
+describe("McpHub initial settings creation (real filesystem)", () => {
+	let tempDir: string
+	let mcpHub: McpHub
+	let mockProvider: Partial<ClineProvider>
+
+	beforeEach(async () => {
+		vi.clearAllMocks()
+
+		tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "mcp-hub-settings-"))
+
+		mockProvider = {
+			cwd: tempDir,
+			ensureSettingsDirectoryExists: vi.fn().mockResolvedValue(tempDir),
+			postMessageToWebview: vi.fn().mockResolvedValue(undefined),
+			getState: vi.fn().mockResolvedValue({ mcpEnabled: true }),
+		}
+
+		mcpHub = new McpHub(mockProvider as ClineProvider)
+		// The constructor creates the stub settings file through the real
+		// safeWriteJson creation path. Remove it so the test starts from a
+		// machine without any MCP settings file.
+		await mcpHub.waitUntilReady()
+		await fs.rm(path.join(tempDir, GlobalFileNames.mcpSettings), { force: true })
+	})
+
+	afterEach(async () => {
+		await mcpHub.dispose()
+		await fs.rm(tempDir, { recursive: true, force: true })
+		vi.restoreAllMocks()
+	})
+
+	it("preserves a config written under the advisory lock by a concurrent creator (#1371)", async () => {
+		const settingsPath = path.join(tempDir, GlobalFileNames.mcpSettings)
+		const concurrentConfig = {
+			mcpServers: {
+				"concurrent-server": { type: "stdio", command: "node", args: ["server.js"] },
+			},
+		}
+
+		let releaseB: () => Promise<void> = async () => {}
+		let bLockReleased = false
+		let creation: Promise<string> | undefined
+		try {
+			// Writer B (a lock-aware competing process) takes the real advisory
+			// lock before writer A's creation pass starts. proper-lockfile can
+			// lock a path whose target does not exist yet, so the file is still
+			// absent at this point.
+			releaseB = await lockfile.lock(settingsPath, { realpath: false })
+
+			// Gate writer A's lock attempt. The wrapper forwards to the real
+			// proper-lockfile lock, records the in-flight acquisition promise,
+			// and signals the moment A calls it inside production safeWriteJson.
+			const actualLock = (await vi.importActual<typeof import("proper-lockfile")>("proper-lockfile")).lock
+			let aLockAttempt: ReturnType<typeof lockfile.lock> | undefined
+			let signalALockAttempted: () => void = () => {}
+			const aLockAttempted = new Promise<void>((resolve) => {
+				signalALockAttempted = resolve
+			})
+			vi.mocked(lockfile.lock).mockImplementationOnce(async (filePath, options) => {
+				aLockAttempt = actualLock(filePath, options)
+				signalALockAttempted()
+				return aLockAttempt
+			})
+
+			// Writer A enters the initial-creation path. The file is still
+			// absent, so its existence check resolves false while B holds the
+			// lock; the result is stale the moment B writes.
+			creation = mcpHub.getMcpSettingsFilePath()
+
+			// A must reach the real lock acquisition while B holds the lock.
+			// If production safeWriteJson ever bypassed lockfile.lock, A would
+			// finish the clobbering write first and this race rejects.
+			await Promise.race([
+				aLockAttempted,
+				creation.then(() => {
+					throw new Error("safeWriteJson completed without acquiring the advisory lock")
+				}),
+			])
+
+			// The lock B holds is genuinely enforced at the filesystem level.
+			// realpath: false stats only the .lock path; the settings file is
+			// still absent at this point.
+			expect(await lockfile.check(settingsPath, { realpath: false })).toBe(true)
+
+			// Writer B commits a real configuration under its lock while A
+			// waits, which is what makes A's earlier false result stale.
+			await fs.writeFile(settingsPath, JSON.stringify(concurrentConfig), "utf-8")
+
+			// A must still be blocked on B's lock. Acquiring the lock needs
+			// B's release, which only a macrotask can complete, so a settled
+			// attempt would flip the flag during this microtask drain.
+			let aLockSettled = false
+			void aLockAttempt?.then(
+				() => {
+					aLockSettled = true
+				},
+				() => {
+					aLockSettled = true
+				},
+			)
+			await Promise.resolve()
+			expect(aLockSettled).toBe(false)
+
+			// Releasing B lets A's retry acquire the lock. The locked merge
+			// read then sees B's committed config instead of clobbering it.
+			await releaseB()
+			bLockReleased = true
+
+			const returnedPath = await creation
+
+			expect(returnedPath).toBe(settingsPath)
+
+			// The creation pass must not clobber the concurrent config with the
+			// empty default stub.
+			const content = JSON.parse(await fs.readFile(settingsPath, "utf-8"))
+			expect(content).toEqual(concurrentConfig)
+
+			// The creation pass leaves no temp, backup, or lock artifacts
+			// behind.
+			const leftovers = (await fs.readdir(tempDir)).filter((entry) => entry !== GlobalFileNames.mcpSettings)
+			expect(leftovers).toEqual([])
+		} finally {
+			// Failure paths must not leak B's lock or leave writer A pending as
+			// an unhandled rejection. Awaiting a settled creation is free; on a
+			// failure path it unblocks A before afterEach removes the temp dir.
+			if (!bLockReleased) {
+				await releaseB().catch(() => {})
+			}
+			if (creation) {
+				await creation.catch(() => {})
+			}
+		}
+	})
+})

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,44 @@ 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 (error) {
+					// Mirror the production safeWriteJson merge contract: only ENOENT
+					// and SyntaxError are recoverable; an EACCES or I/O failure must
+					// reject before the merge callback runs.
+					// unknown-safe narrowing: no cast on the caught value (the "in"
+					// check narrows to object & Record<"code", unknown>).
+					const code =
+						error && typeof error === "object" && "code" in error && typeof error.code === "string"
+							? error.code
+							: undefined
+					if (!(error instanceof SyntaxError) && code !== "ENOENT") {
+						throw error
+					}
+					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 +245,122 @@ 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)
+		})
+
+		it("writes the default stub when no settings file exists yet", async () => {
+			const settingsPath = path.join("/mock/settings/path", "mcp_settings.json")
+
+			// Existence check and the locked read both see an absent file.
+			vi.mocked(fs.access).mockRejectedValueOnce(
+				Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }),
+			)
+			vi.mocked(fs.readFile).mockRejectedValueOnce(
+				Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }),
+			)
+
+			const returnedPath = await mcpHub.getMcpSettingsFilePath()
+
+			expect(returnedPath).toBe(settingsPath)
+			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({ mcpServers: {} })
+		})
+
+		it("writes the default stub when the existing content has no mcpServers object", async () => {
+			const settingsPath = path.join("/mock/settings/path", "mcp_settings.json")
+
+			// Existence check sees an absent file, but the locked read finds content
+			// that does not carry a mcpServers object (e.g. a torn or foreign write).
+			vi.mocked(fs.access).mockRejectedValueOnce(
+				Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }),
+			)
+			vi.mocked(fs.readFile).mockResolvedValueOnce(JSON.stringify({ someOtherKey: true }))
+
+			await mcpHub.getMcpSettingsFilePath()
+
+			expect(fs.writeFile).toHaveBeenCalledTimes(1)
+			const [, writtenData] = vi.mocked(fs.writeFile).mock.calls[0]
+			expect(JSON.parse(writtenData as string)).toEqual({ mcpServers: {} })
+		})
+
+		it("writes the default stub when the existing mcpServers value is not an object", async () => {
+			const settingsPath = path.join("/mock/settings/path", "mcp_settings.json")
+
+			vi.mocked(fs.access).mockRejectedValueOnce(
+				Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }),
+			)
+			vi.mocked(fs.readFile).mockResolvedValueOnce(JSON.stringify({ mcpServers: "corrupted" }))
+
+			await mcpHub.getMcpSettingsFilePath()
+
+			expect(fs.writeFile).toHaveBeenCalledTimes(1)
+			const [, writtenData] = vi.mocked(fs.writeFile).mock.calls[0]
+			expect(JSON.parse(writtenData as string)).toEqual({ mcpServers: {} })
+		})
+
+		it("writes the default stub when the existing mcpServers value is an array", async () => {
+			const settingsPath = path.join("/mock/settings/path", "mcp_settings.json")
+
+			// Arrays satisfy `typeof === "object"`; an mcpServers map must be a
+			// plain object, so an array is invalid and replaced by the stub
+			// instead of being preserved and rejected by McpSettingsSchema later.
+			vi.mocked(fs.access).mockRejectedValueOnce(
+				Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }),
+			)
+			vi.mocked(fs.readFile).mockResolvedValueOnce(JSON.stringify({ mcpServers: [] }))
+
+			await mcpHub.getMcpSettingsFilePath()
+
+			expect(fs.writeFile).toHaveBeenCalledTimes(1)
+			const [, writtenData] = vi.mocked(fs.writeFile).mock.calls[0]
+			expect(JSON.parse(writtenData as string)).toEqual({ mcpServers: {} })
+		})
+
+		it("rejects creation when the locked read fails with an I/O error (EACCES)", async () => {
+			const settingsPath = path.join("/mock/settings/path", "mcp_settings.json")
+
+			vi.mocked(fs.access).mockRejectedValueOnce(
+				Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }),
+			)
+			// The locked read fails with a real I/O error (not ENOENT): the
+			// safeWriteJson mock mirrors the production contract — reject before
+			// the merge callback runs instead of treating the file as absent.
+			vi.mocked(fs.readFile).mockRejectedValueOnce(
+				Object.assign(new Error("EACCES: permission denied"), { code: "EACCES" }),
+			)
+
+			await expect(mcpHub.getMcpSettingsFilePath()).rejects.toThrow("EACCES: permission denied")
+			expect(fs.writeFile).not.toHaveBeenCalled()
+		})
+	})
+
 	describe("Discriminated union type handling", () => {
 		it("should create connected connections with proper type", async () => {
 			// Mock StdioClientTransport