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