From e11ef14d469b582310e6d347d088fa3f21862070 Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Thu, 27 Aug 2026 09:30:22 +0800 Subject: [PATCH 1/5] 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 From 332253af77c516a4d9ebc6ac36eecc10d0126d99 Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Thu, 27 Aug 2026 09:58:43 +0800 Subject: [PATCH 2/5] test(mcp): cover getMcpSettingsFilePath fallback branches Codecov reported 2 patch lines (1 missing, 1 partial) in the safeWriteJson merge callback. Add the three remaining fallback cases: absent file (merge sees null), existing content without an mcpServers object, and mcpServers present but not an object - all must write the default stub. All changed lines and branches of the merge callback are now covered. --- src/services/mcp/__tests__/McpHub.spec.ts | 52 +++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/src/services/mcp/__tests__/McpHub.spec.ts b/src/services/mcp/__tests__/McpHub.spec.ts index cab1501b53..1ff277a761 100644 --- a/src/services/mcp/__tests__/McpHub.spec.ts +++ b/src/services/mcp/__tests__/McpHub.spec.ts @@ -260,6 +260,58 @@ describe("McpHub", () => { 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: {} }) + }) }) describe("Discriminated union type handling", () => { From 9dd9825c4fdfe7b5964d702f8fc63f32caa365c4 Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Fri, 28 Aug 2026 10:06:38 +0800 Subject: [PATCH 3/5] fix(fws): mcp_settings merge array guard, spec-mock production parity, unknown-safe code read The mcp_settings merge callback now requires a plain object (!Array.isArray), so an existing mcpServers: [] is replaced by the empty stub instead of being preserved and later rejected by McpSettingsSchema; the safeWriteJson spec mock mirrors the production merge contract (only ENOENT and SyntaxError are recoverable, any other read failure rejects before the merge callback runs, with an EACCES regression); the error.code read in the mock uses an unknown-safe type guard instead of an object cast. (CodeRabbit findings on trial #1413). --- src/services/mcp/McpHub.ts | 11 ++++- src/services/mcp/__tests__/McpHub.spec.ts | 49 ++++++++++++++++++++++- 2 files changed, 58 insertions(+), 2 deletions(-) diff --git a/src/services/mcp/McpHub.ts b/src/services/mcp/McpHub.ts index 5d6c31a3e5..42786cfaa5 100644 --- a/src/services/mcp/McpHub.ts +++ b/src/services/mcp/McpHub.ts @@ -517,7 +517,16 @@ export class McpHub { prettyPrint: true, merge: (existing) => { const parsed = existing as { mcpServers?: unknown } | null - if (parsed && parsed.mcpServers && typeof parsed.mcpServers === "object") { + // 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: {} } diff --git a/src/services/mcp/__tests__/McpHub.spec.ts b/src/services/mcp/__tests__/McpHub.spec.ts index 1ff277a761..116c87c1ec 100644 --- a/src/services/mcp/__tests__/McpHub.spec.ts +++ b/src/services/mcp/__tests__/McpHub.spec.ts @@ -52,7 +52,19 @@ vi.mock("../../../utils/safeWriteJson", () => ({ try { const fs = await import("fs/promises") existing = JSON.parse(await fs.readFile(filePath, "utf8")) - } catch { + } 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) @@ -312,6 +324,41 @@ describe("McpHub", () => { 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", () => { From 046b78cad8c7b9f5e8c5d51cf9f4860abd31c877 Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Sun, 30 Aug 2026 12:32:57 +0800 Subject: [PATCH 4/5] =?UTF-8?q?chore(ci):=20empty=20commit=20=E2=80=94=20r?= =?UTF-8?q?e-trigger=20CI=20and=20the=20CodeRabbit=20current-head=20review?= =?UTF-8?q?=20gate=20(no=20code=20change)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From 351942864cf7a7b42e412c9f5d201cc9473d0bcb Mon Sep 17 00:00:00 2001 From: Elliott de Launay Date: Sat, 12 Sep 2026 22:16:21 +0000 Subject: [PATCH 5/5] test(mcp): cover concurrent settings creation with real lock --- ...cpHub.settingsCreation.integration.spec.ts | 176 ++++++++++++++++++ 1 file changed, 176 insertions(+) create mode 100644 src/services/mcp/__tests__/McpHub.settingsCreation.integration.spec.ts 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(() => {}) + } + } + }) +})