diff --git a/src/services/mcp/McpHub.ts b/src/services/mcp/McpHub.ts index 1374e430fe..42786cfaa5 100644 --- a/src/services/mcp/McpHub.ts +++ b/src/services/mcp/McpHub.ts @@ -506,13 +506,32 @@ 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 + // 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 diff --git a/src/services/mcp/__tests__/McpHub.settingsCreation.integration.spec.ts b/src/services/mcp/__tests__/McpHub.settingsCreation.integration.spec.ts new file mode 100644 index 0000000000..1c9ad91390 --- /dev/null +++ b/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("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 + + 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 = async () => {} + let bLockReleased = false + let creation: Promise | 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("proper-lockfile")).lock + let aLockAttempt: ReturnType | undefined + let signalALockAttempted: () => void = () => {} + const aLockAttempted = new Promise((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(() => {}) + } + } + }) +}) diff --git a/src/services/mcp/__tests__/McpHub.spec.ts b/src/services/mcp/__tests__/McpHub.spec.ts index 96589d8dd6..116c87c1ec 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,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