From d47929e2264735197eae06b8d5aaa82964ac0ad1 Mon Sep 17 00:00:00 2001 From: Matt Rubens Date: Mon, 9 Jun 2025 13:46:36 -0400 Subject: [PATCH] Revert "fix: use safeWriteJson for all JSON file writes (#3772)" This reverts commit 1be30fc8c2a274467227fc7d695b323510160fcb. --- .roo/rules-code/use-safeWriteJson.md | 5 - pnpm-lock.yaml | 66 --- src/api/providers/fetchers/modelCache.ts | 3 +- .../providers/fetchers/modelEndpointCache.ts | 3 +- .../config/__tests__/importExport.test.ts | 28 +- src/core/config/importExport.ts | 3 +- .../context-tracking/FileContextTracker.ts | 3 +- src/core/task-persistence/apiMessages.ts | 3 +- src/core/task-persistence/taskMessages.ts | 3 +- .../webview/__tests__/ClineProvider.test.ts | 8 +- src/core/webview/webviewMessageHandler.ts | 3 +- src/package.json | 6 +- .../__tests__/cache-manager.test.ts | 31 +- src/services/code-index/cache-manager.ts | 5 +- src/services/mcp/McpHub.ts | 7 +- src/services/mcp/__tests__/McpHub.test.ts | 32 +- src/utils/__tests__/safeWriteJson.test.ts | 507 ------------------ src/utils/safeWriteJson.ts | 218 -------- 18 files changed, 45 insertions(+), 889 deletions(-) delete mode 100644 .roo/rules-code/use-safeWriteJson.md delete mode 100644 src/utils/__tests__/safeWriteJson.test.ts delete mode 100644 src/utils/safeWriteJson.ts diff --git a/.roo/rules-code/use-safeWriteJson.md b/.roo/rules-code/use-safeWriteJson.md deleted file mode 100644 index 1598bbde58..0000000000 --- a/.roo/rules-code/use-safeWriteJson.md +++ /dev/null @@ -1,5 +0,0 @@ -# JSON File Writing Must Be Atomic - -- MUST ALWAYS use `safeWriteJson(filePath: string, data: any): Promise` from `src/utils/safeWriteJson.ts` instead of `JSON.stringify` with file-write operations -- `safeWriteJson` prevents data corruption via atomic writes with locking and streams the write to minimize memory footprint -- Test files are exempt from this rule diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 82e7df9fca..39ac271dfc 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -702,9 +702,6 @@ importers: pretty-bytes: specifier: ^6.1.1 version: 6.1.1 - proper-lockfile: - specifier: ^4.1.2 - version: 4.1.2 ps-tree: specifier: ^1.2.0 version: 1.2.0 @@ -732,9 +729,6 @@ importers: sound-play: specifier: ^1.1.0 version: 1.1.0 - stream-json: - specifier: ^1.8.0 - version: 1.9.1 string-similarity: specifier: ^4.0.4 version: 4.0.4 @@ -817,15 +811,9 @@ importers: '@types/node-ipc': specifier: ^9.2.3 version: 9.2.3 - '@types/proper-lockfile': - specifier: ^4.1.4 - version: 4.1.4 '@types/ps-tree': specifier: ^1.1.6 version: 1.1.6 - '@types/stream-json': - specifier: ^1.7.8 - version: 1.7.8 '@types/string-similarity': specifier: ^4.0.2 version: 4.0.2 @@ -4466,9 +4454,6 @@ packages: '@types/prop-types@15.7.14': resolution: {integrity: sha512-gNMvNH49DJ7OJYv+KAKn0Xp45p8PLl6zo2YnvDIbTd4J6MER2BmWN49TG7n9LvkyihINxeKW8+3bfS2yDC9dzQ==} - '@types/proper-lockfile@4.1.4': - resolution: {integrity: sha512-uo2ABllncSqg9F1D4nugVl9v93RmjxF6LJzQLMLDdPaXCUIDPeOJ21Gbqi43xNKzBi/WQ0Q0dICqufzQbMjipQ==} - '@types/ps-tree@1.1.6': resolution: {integrity: sha512-PtrlVaOaI44/3pl3cvnlK+GxOM3re2526TJvPvh7W+keHIXdV4TE0ylpPBAcvFQCbGitaTXwL9u+RF7qtVeazQ==} @@ -4483,21 +4468,12 @@ packages: '@types/resolve@1.20.6': resolution: {integrity: sha512-A4STmOXPhMUtHH+S6ymgE2GiBSMqf4oTvcQZMcHzokuTLVYzXTB8ttjcgxOVaAp2lGwEdzZ0J+cRbbeevQj1UQ==} - '@types/retry@0.12.5': - resolution: {integrity: sha512-3xSjTp3v03X/lSQLkczaN9UIEwJMoMCA1+Nb5HfbJEQWogdeQIyVtTvxPXDQjZ5zws8rFQfVfRdz03ARihPJgw==} - '@types/shell-quote@1.7.5': resolution: {integrity: sha512-+UE8GAGRPbJVQDdxi16dgadcBfQ+KG2vgZhV1+3A1XmHbmwcdwhCUwIdy+d3pAGrbvgRoVSjeI9vOWyq376Yzw==} '@types/stack-utils@2.0.3': resolution: {integrity: sha512-9aEbYZ3TbYMznPdcdr3SmIrLXwC/AKZXQeCf9Pgao5CKb8CyHuEX5jzWPTkvregvhRJHcpRO6BFoGW9ycaOkYw==} - '@types/stream-chain@2.1.0': - resolution: {integrity: sha512-guDyAl6s/CAzXUOWpGK2bHvdiopLIwpGu8v10+lb9hnQOyo4oj/ZUQFOvqFjKGsE3wJP1fpIesCcMvbXuWsqOg==} - - '@types/stream-json@1.7.8': - resolution: {integrity: sha512-MU1OB1eFLcYWd1LjwKXrxdoPtXSRzRmAnnxs4Js/ayB5O/NvHraWwuOaqMWIebpYwM6khFlsJOHEhI9xK/ab4Q==} - '@types/string-similarity@4.0.2': resolution: {integrity: sha512-LkJQ/jsXtCVMK+sKYAmX/8zEq+/46f1PTQw7YtmQwb74jemS1SlNLmARM2Zml9DgdDTWKAtc5L13WorpHPDjDA==} @@ -8961,9 +8937,6 @@ packages: resolution: {integrity: sha512-vGrhOavPSTz4QVNuBNdcNXePNdNMaO1xj9yBeH1ScQPjk/rhg9sSlCXPhMkFuaNNW/syTvYqsnbIJxMBfRbbag==} engines: {node: '>= 8'} - proper-lockfile@4.1.2: - resolution: {integrity: sha512-TjNPblN4BwAWMXU8s9AEz4JmQxnD1NNL7bNOY/AKUzyamc379FWASUhc/K1pL2noVb+XmZKLL68cjzLsiOAMaA==} - property-information@5.6.0: resolution: {integrity: sha512-YUHSPk+A30YPv+0Qf8i9Mbfe/C0hdPXk1s1jPVToV8pk8BQtpw10ct89Eo7OWkutrwqvT0eicAxlOg3dOAu8JA==} @@ -9319,10 +9292,6 @@ packages: resolution: {integrity: sha512-oMA2dcrw6u0YfxJQXm342bFKX/E4sG9rbTzO9ptUcR/e8A33cHuvStiYOwH7fszkZlZ1z/ta9AAoPk2F4qIOHA==} engines: {node: '>=18'} - retry@0.12.0: - resolution: {integrity: sha512-9LkiTwjUh6rT555DtE9rTX+BKByPfrMzEAtnlEtdEwr3Nkffwiihqe2bWADg+OQRjt9gl6ICdmB/ZFDCGAtSow==} - engines: {node: '>= 4'} - reusify@1.1.0: resolution: {integrity: sha512-g6QUff04oZpHs0eG5p83rFLhHeV00ug/Yf9nZM6fLeUrPguBTkTQOdpAWWspMh55TZfVQDPaN3NQJfbVRAxdIw==} engines: {iojs: '>=1.0.0', node: '>=0.10.0'} @@ -9656,15 +9625,9 @@ packages: prettier: optional: true - stream-chain@2.2.5: - resolution: {integrity: sha512-1TJmBx6aSWqZ4tx7aTpBDXK0/e2hhcNSTV8+CbFJtDjbb+I1mZ8lHit0Grw9GRT+6JbIrrDd8esncgBi8aBXGA==} - stream-combiner@0.0.4: resolution: {integrity: sha512-rT00SPnTVyRsaSz5zgSPma/aHSOic5U1prhYdRy5HS2kTZviFpmDgzilbtsJsxiroqACmayynDN/9VzIbX5DOw==} - stream-json@1.9.1: - resolution: {integrity: sha512-uWkjJ+2Nt/LO9Z/JyKZbMusL8Dkh97uUBTv3AJQ74y07lVahLY4eEFsPsE97pxYBwr8nnjMAIch5eqI0gPShyw==} - streamsearch@1.1.0: resolution: {integrity: sha512-Mcc5wHehp9aXz1ax6bZUyY5afg9u2rv5cqQI3mRrYkGC8rW2hM02jWuwjtL++LS5qinSyhj2QfLyNsuc+VsExg==} engines: {node: '>=10.0.0'} @@ -14760,10 +14723,6 @@ snapshots: '@types/prop-types@15.7.14': {} - '@types/proper-lockfile@4.1.4': - dependencies: - '@types/retry': 0.12.5 - '@types/ps-tree@1.1.6': {} '@types/react-dom@18.3.7(@types/react@18.3.23)': @@ -14777,21 +14736,10 @@ snapshots: '@types/resolve@1.20.6': {} - '@types/retry@0.12.5': {} - '@types/shell-quote@1.7.5': {} '@types/stack-utils@2.0.3': {} - '@types/stream-chain@2.1.0': - dependencies: - '@types/node': 22.15.20 - - '@types/stream-json@1.7.8': - dependencies: - '@types/node': 22.15.20 - '@types/stream-chain': 2.1.0 - '@types/string-similarity@4.0.2': {} '@types/stylis@4.2.5': {} @@ -20105,12 +20053,6 @@ snapshots: propagate@2.0.1: {} - proper-lockfile@4.1.2: - dependencies: - graceful-fs: 4.2.11 - retry: 0.12.0 - signal-exit: 3.0.7 - property-information@5.6.0: dependencies: xtend: 4.0.2 @@ -20590,8 +20532,6 @@ snapshots: onetime: 7.0.0 signal-exit: 4.1.0 - retry@0.12.0: {} - reusify@1.1.0: {} rfdc@1.4.1: {} @@ -21015,16 +20955,10 @@ snapshots: - supports-color - utf-8-validate - stream-chain@2.2.5: {} - stream-combiner@0.0.4: dependencies: duplexer: 0.1.2 - stream-json@1.9.1: - dependencies: - stream-chain: 2.2.5 - streamsearch@1.1.0: {} streamx@2.22.0: diff --git a/src/api/providers/fetchers/modelCache.ts b/src/api/providers/fetchers/modelCache.ts index c3da51b2b6..12d636bc46 100644 --- a/src/api/providers/fetchers/modelCache.ts +++ b/src/api/providers/fetchers/modelCache.ts @@ -2,7 +2,6 @@ import * as path from "path" import fs from "fs/promises" import NodeCache from "node-cache" -import { safeWriteJson } from "../../../utils/safeWriteJson" import { ContextProxy } from "../../../core/config/ContextProxy" import { getCacheDirectoryPath } from "../../../utils/storage" @@ -20,7 +19,7 @@ const memoryCache = new NodeCache({ stdTTL: 5 * 60, checkperiod: 5 * 60 }) async function writeModels(router: RouterName, data: ModelRecord) { const filename = `${router}_models.json` const cacheDir = await getCacheDirectoryPath(ContextProxy.instance.globalStorageUri.fsPath) - await safeWriteJson(path.join(cacheDir, filename), data) + await fs.writeFile(path.join(cacheDir, filename), JSON.stringify(data)) } async function readModels(router: RouterName): Promise { diff --git a/src/api/providers/fetchers/modelEndpointCache.ts b/src/api/providers/fetchers/modelEndpointCache.ts index 256ae84048..c69e7c82a3 100644 --- a/src/api/providers/fetchers/modelEndpointCache.ts +++ b/src/api/providers/fetchers/modelEndpointCache.ts @@ -2,7 +2,6 @@ import * as path from "path" import fs from "fs/promises" import NodeCache from "node-cache" -import { safeWriteJson } from "../../../utils/safeWriteJson" import sanitize from "sanitize-filename" import { ContextProxy } from "../../../core/config/ContextProxy" @@ -19,7 +18,7 @@ const getCacheKey = (router: RouterName, modelId: string) => sanitize(`${router} async function writeModelEndpoints(key: string, data: ModelRecord) { const filename = `${key}_endpoints.json` const cacheDir = await getCacheDirectoryPath(ContextProxy.instance.globalStorageUri.fsPath) - await safeWriteJson(path.join(cacheDir, filename), data) + await fs.writeFile(path.join(cacheDir, filename), JSON.stringify(data, null, 2)) } async function readModelEndpoints(key: string): Promise { diff --git a/src/core/config/__tests__/importExport.test.ts b/src/core/config/__tests__/importExport.test.ts index cf68279397..0e96ecaae5 100644 --- a/src/core/config/__tests__/importExport.test.ts +++ b/src/core/config/__tests__/importExport.test.ts @@ -12,7 +12,6 @@ import { importSettings, exportSettings } from "../importExport" import { ProviderSettingsManager } from "../ProviderSettingsManager" import { ContextProxy } from "../ContextProxy" import { CustomModesManager } from "../CustomModesManager" -import { safeWriteJson } from "../../../utils/safeWriteJson" jest.mock("vscode", () => ({ window: { @@ -34,8 +33,6 @@ jest.mock("os", () => ({ homedir: jest.fn(() => "/mock/home"), })) -jest.mock("../../../utils/safeWriteJson") - describe("importExport", () => { let mockProviderSettingsManager: jest.Mocked let mockContextProxy: jest.Mocked @@ -375,10 +372,11 @@ describe("importExport", () => { expect(mockContextProxy.export).toHaveBeenCalled() expect(fs.mkdir).toHaveBeenCalledWith("/mock/path", { recursive: true }) - expect(safeWriteJson).toHaveBeenCalledWith("/mock/path/roo-code-settings.json", { - providerProfiles: mockProviderProfiles, - globalSettings: mockGlobalSettings, - }) + expect(fs.writeFile).toHaveBeenCalledWith( + "/mock/path/roo-code-settings.json", + JSON.stringify({ providerProfiles: mockProviderProfiles, globalSettings: mockGlobalSettings }, null, 2), + "utf-8", + ) }) it("should include globalSettings when allowedMaxRequests is null", async () => { @@ -407,10 +405,11 @@ describe("importExport", () => { contextProxy: mockContextProxy, }) - expect(safeWriteJson).toHaveBeenCalledWith("/mock/path/roo-code-settings.json", { - providerProfiles: mockProviderProfiles, - globalSettings: mockGlobalSettings, - }) + expect(fs.writeFile).toHaveBeenCalledWith( + "/mock/path/roo-code-settings.json", + JSON.stringify({ providerProfiles: mockProviderProfiles, globalSettings: mockGlobalSettings }, null, 2), + "utf-8", + ) }) it("should handle errors during the export process", async () => { @@ -425,8 +424,7 @@ describe("importExport", () => { }) mockContextProxy.export.mockResolvedValue({ mode: "code" }) - // Simulate an error during the safeWriteJson operation - ;(safeWriteJson as jest.Mock).mockRejectedValueOnce(new Error("Safe write error")) + ;(fs.writeFile as jest.Mock).mockRejectedValue(new Error("Write error")) await exportSettings({ providerSettingsManager: mockProviderSettingsManager, @@ -437,10 +435,8 @@ describe("importExport", () => { expect(mockProviderSettingsManager.export).toHaveBeenCalled() expect(mockContextProxy.export).toHaveBeenCalled() expect(fs.mkdir).toHaveBeenCalledWith("/mock/path", { recursive: true }) - expect(safeWriteJson).toHaveBeenCalled() // safeWriteJson is called, but it will throw + expect(fs.writeFile).toHaveBeenCalled() // The error is caught and the function exits silently. - // Optionally, ensure no error message was shown if that's part of "silent" - // expect(vscode.window.showErrorMessage).not.toHaveBeenCalled(); }) it("should handle errors during directory creation", async () => { diff --git a/src/core/config/importExport.ts b/src/core/config/importExport.ts index cd92a81ff6..4830a5f987 100644 --- a/src/core/config/importExport.ts +++ b/src/core/config/importExport.ts @@ -1,4 +1,3 @@ -import { safeWriteJson } from "../../utils/safeWriteJson" import os from "os" import * as path from "path" import fs from "fs/promises" @@ -117,6 +116,6 @@ export const exportSettings = async ({ providerSettingsManager, contextProxy }: const dirname = path.dirname(uri.fsPath) await fs.mkdir(dirname, { recursive: true }) - await safeWriteJson(uri.fsPath, { providerProfiles, globalSettings }) + await fs.writeFile(uri.fsPath, JSON.stringify({ providerProfiles, globalSettings }, null, 2), "utf-8") } catch (e) {} } diff --git a/src/core/context-tracking/FileContextTracker.ts b/src/core/context-tracking/FileContextTracker.ts index 5741b62cfc..323bb4122f 100644 --- a/src/core/context-tracking/FileContextTracker.ts +++ b/src/core/context-tracking/FileContextTracker.ts @@ -1,4 +1,3 @@ -import { safeWriteJson } from "../../utils/safeWriteJson" import * as path from "path" import * as vscode from "vscode" import { getTaskDirectoryPath } from "../../utils/storage" @@ -131,7 +130,7 @@ export class FileContextTracker { const globalStoragePath = this.getContextProxy()!.globalStorageUri.fsPath const taskDir = await getTaskDirectoryPath(globalStoragePath, taskId) const filePath = path.join(taskDir, GlobalFileNames.taskMetadata) - await safeWriteJson(filePath, metadata) + await fs.writeFile(filePath, JSON.stringify(metadata, null, 2)) } catch (error) { console.error("Failed to save task metadata:", error) } diff --git a/src/core/task-persistence/apiMessages.ts b/src/core/task-persistence/apiMessages.ts index cead18af78..0ba9628a5d 100644 --- a/src/core/task-persistence/apiMessages.ts +++ b/src/core/task-persistence/apiMessages.ts @@ -1,4 +1,3 @@ -import { safeWriteJson } from "../../utils/safeWriteJson" import * as path from "path" import * as fs from "fs/promises" @@ -47,5 +46,5 @@ export async function saveApiMessages({ }) { const taskDir = await getTaskDirectoryPath(globalStoragePath, taskId) const filePath = path.join(taskDir, GlobalFileNames.apiConversationHistory) - await safeWriteJson(filePath, messages) + await fs.writeFile(filePath, JSON.stringify(messages)) } diff --git a/src/core/task-persistence/taskMessages.ts b/src/core/task-persistence/taskMessages.ts index 63a2eefbaa..3ed5c5099e 100644 --- a/src/core/task-persistence/taskMessages.ts +++ b/src/core/task-persistence/taskMessages.ts @@ -1,4 +1,3 @@ -import { safeWriteJson } from "../../utils/safeWriteJson" import * as path from "path" import * as fs from "fs/promises" @@ -38,5 +37,5 @@ export type SaveTaskMessagesOptions = { export async function saveTaskMessages({ messages, taskId, globalStoragePath }: SaveTaskMessagesOptions) { const taskDir = await getTaskDirectoryPath(globalStoragePath, taskId) const filePath = path.join(taskDir, GlobalFileNames.uiMessages) - await safeWriteJson(filePath, messages) + await fs.writeFile(filePath, JSON.stringify(messages)) } diff --git a/src/core/webview/__tests__/ClineProvider.test.ts b/src/core/webview/__tests__/ClineProvider.test.ts index d555172b77..e84a78057a 100644 --- a/src/core/webview/__tests__/ClineProvider.test.ts +++ b/src/core/webview/__tests__/ClineProvider.test.ts @@ -13,7 +13,6 @@ import { experimentDefault } from "../../../shared/experiments" import { setTtsEnabled } from "../../../utils/tts" import { ContextProxy } from "../../config/ContextProxy" import { Task, TaskOptions } from "../../task/Task" -import { safeWriteJson } from "../../../utils/safeWriteJson" import { ClineProvider } from "../ClineProvider" @@ -42,8 +41,6 @@ jest.mock("axios", () => ({ post: jest.fn(), })) -jest.mock("../../../utils/safeWriteJson") - jest.mock( "@modelcontextprotocol/sdk/types.js", () => ({ @@ -2004,7 +2001,10 @@ describe("Project MCP Settings", () => { ) // Verify file was created with default content - expect(safeWriteJson).toHaveBeenCalledWith(expect.stringContaining("mcp.json"), { mcpServers: {} }) + expect(fs.writeFile).toHaveBeenCalledWith( + expect.stringContaining("mcp.json"), + JSON.stringify({ mcpServers: {} }, null, 2), + ) }) test("handles openProjectMcpSettings when workspace is not open", async () => { diff --git a/src/core/webview/webviewMessageHandler.ts b/src/core/webview/webviewMessageHandler.ts index 7bd99370e2..91834d2fd0 100644 --- a/src/core/webview/webviewMessageHandler.ts +++ b/src/core/webview/webviewMessageHandler.ts @@ -1,4 +1,3 @@ -import { safeWriteJson } from "../../utils/safeWriteJson" import * as path from "path" import fs from "fs/promises" import pWaitFor from "p-wait-for" @@ -500,7 +499,7 @@ export const webviewMessageHandler = async (provider: ClineProvider, message: We const exists = await fileExistsAtPath(mcpPath) if (!exists) { - await safeWriteJson(mcpPath, { mcpServers: {} }) + await fs.writeFile(mcpPath, JSON.stringify({ mcpServers: {} }, null, 2)) } await openFile(mcpPath) diff --git a/src/package.json b/src/package.json index 0f3af8e763..0c9470042c 100644 --- a/src/package.json +++ b/src/package.json @@ -361,11 +361,11 @@ "@google/genai": "^0.13.0", "@mistralai/mistralai": "^1.3.6", "@modelcontextprotocol/sdk": "^1.9.0", - "@qdrant/js-client-rest": "^1.14.0", "@roo-code/cloud": "workspace:^", "@roo-code/ipc": "workspace:^", "@roo-code/telemetry": "workspace:^", "@roo-code/types": "workspace:^", + "@qdrant/js-client-rest": "^1.14.0", "@types/lodash.debounce": "^4.0.9", "@vscode/codicons": "^0.0.36", "async-mutex": "^0.5.0", @@ -398,7 +398,6 @@ "pdf-parse": "^1.1.1", "pkce-challenge": "^4.1.0", "pretty-bytes": "^6.1.1", - "proper-lockfile": "^4.1.2", "ps-tree": "^1.2.0", "puppeteer-chromium-resolver": "^23.0.0", "puppeteer-core": "^23.4.0", @@ -408,7 +407,6 @@ "serialize-error": "^11.0.3", "simple-git": "^3.27.0", "sound-play": "^1.1.0", - "stream-json": "^1.8.0", "string-similarity": "^4.0.4", "strip-ansi": "^7.1.0", "strip-bom": "^5.0.0", @@ -438,9 +436,7 @@ "@types/node": "20.x", "@types/node-cache": "^4.1.3", "@types/node-ipc": "^9.2.3", - "@types/proper-lockfile": "^4.1.4", "@types/ps-tree": "^1.1.6", - "@types/stream-json": "^1.7.8", "@types/string-similarity": "^4.0.2", "@types/tmp": "^0.2.6", "@types/turndown": "^5.0.5", diff --git a/src/services/code-index/__tests__/cache-manager.test.ts b/src/services/code-index/__tests__/cache-manager.test.ts index 35ae7cdefa..3746d949d3 100644 --- a/src/services/code-index/__tests__/cache-manager.test.ts +++ b/src/services/code-index/__tests__/cache-manager.test.ts @@ -2,7 +2,6 @@ import * as vscode from "vscode" import { createHash } from "crypto" import debounce from "lodash.debounce" import { CacheManager } from "../cache-manager" -import { safeWriteJson } from "../../../utils/safeWriteJson" // Mock vscode jest.mock("vscode", () => ({ @@ -12,16 +11,12 @@ jest.mock("vscode", () => ({ workspace: { fs: { readFile: jest.fn(), + writeFile: jest.fn(), delete: jest.fn(), }, }, })) -// Mock safeWriteJson -jest.mock("../../../utils/safeWriteJson", () => ({ - safeWriteJson: jest.fn(), -})) - // Mock debounce to execute immediately jest.mock("lodash.debounce", () => jest.fn((fn) => fn)) @@ -93,7 +88,7 @@ describe("CacheManager", () => { cacheManager.updateHash(filePath, hash) expect(cacheManager.getHash(filePath)).toBe(hash) - expect(safeWriteJson).toHaveBeenCalled() + expect(vscode.workspace.fs.writeFile).toHaveBeenCalled() }) it("should delete hash and trigger save", () => { @@ -104,7 +99,7 @@ describe("CacheManager", () => { cacheManager.deleteHash(filePath) expect(cacheManager.getHash(filePath)).toBeUndefined() - expect(safeWriteJson).toHaveBeenCalled() + expect(vscode.workspace.fs.writeFile).toHaveBeenCalled() }) it("should return shallow copy of hashes", () => { @@ -129,14 +124,18 @@ describe("CacheManager", () => { cacheManager.updateHash(filePath, hash) - expect(safeWriteJson).toHaveBeenCalledWith(mockCachePath.fsPath, { [filePath]: hash }) + expect(vscode.workspace.fs.writeFile).toHaveBeenCalledWith(mockCachePath, expect.any(Uint8Array)) - // No need to parse the data since safeWriteJson receives the object directly + // Verify the saved data + const savedData = JSON.parse( + Buffer.from((vscode.workspace.fs.writeFile as jest.Mock).mock.calls[0][1]).toString(), + ) + expect(savedData).toEqual({ [filePath]: hash }) }) it("should handle save errors gracefully", async () => { const consoleErrorSpy = jest.spyOn(console, "error").mockImplementation() - ;(safeWriteJson as jest.Mock).mockRejectedValue(new Error("Save failed")) + ;(vscode.workspace.fs.writeFile as jest.Mock).mockRejectedValue(new Error("Save failed")) cacheManager.updateHash("test.ts", "hash") @@ -153,19 +152,19 @@ describe("CacheManager", () => { it("should clear cache file and reset state", async () => { cacheManager.updateHash("test.ts", "hash") - // Reset the mock to ensure safeWriteJson succeeds for clearCacheFile - ;(safeWriteJson as jest.Mock).mockClear() - ;(safeWriteJson as jest.Mock).mockResolvedValue(undefined) + // Reset the mock to ensure writeFile succeeds for clearCacheFile + ;(vscode.workspace.fs.writeFile as jest.Mock).mockClear() + ;(vscode.workspace.fs.writeFile as jest.Mock).mockResolvedValue(undefined) await cacheManager.clearCacheFile() - expect(safeWriteJson).toHaveBeenCalledWith(mockCachePath.fsPath, {}) + expect(vscode.workspace.fs.writeFile).toHaveBeenCalledWith(mockCachePath, Buffer.from("{}")) expect(cacheManager.getAllHashes()).toEqual({}) }) it("should handle clear errors gracefully", async () => { const consoleErrorSpy = jest.spyOn(console, "error").mockImplementation() - ;(safeWriteJson as jest.Mock).mockRejectedValue(new Error("Save failed")) + ;(vscode.workspace.fs.writeFile as jest.Mock).mockRejectedValue(new Error("Save failed")) await cacheManager.clearCacheFile() diff --git a/src/services/code-index/cache-manager.ts b/src/services/code-index/cache-manager.ts index 146db4cd2a..f66f933a0b 100644 --- a/src/services/code-index/cache-manager.ts +++ b/src/services/code-index/cache-manager.ts @@ -2,7 +2,6 @@ import * as vscode from "vscode" import { createHash } from "crypto" import { ICacheManager } from "./interfaces/cache" import debounce from "lodash.debounce" -import { safeWriteJson } from "../../utils/safeWriteJson" /** * Manages the cache for code indexing @@ -47,7 +46,7 @@ export class CacheManager implements ICacheManager { */ private async _performSave(): Promise { try { - await safeWriteJson(this.cachePath.fsPath, this.fileHashes) + await vscode.workspace.fs.writeFile(this.cachePath, Buffer.from(JSON.stringify(this.fileHashes, null, 2))) } catch (error) { console.error("Failed to save cache:", error) } @@ -58,7 +57,7 @@ export class CacheManager implements ICacheManager { */ async clearCacheFile(): Promise { try { - await safeWriteJson(this.cachePath.fsPath, {}) + await vscode.workspace.fs.writeFile(this.cachePath, Buffer.from("{}")) this.fileHashes = {} } catch (error) { console.error("Failed to clear cache file:", error, this.cachePath) diff --git a/src/services/mcp/McpHub.ts b/src/services/mcp/McpHub.ts index d1bb705ced..85cefd35f7 100644 --- a/src/services/mcp/McpHub.ts +++ b/src/services/mcp/McpHub.ts @@ -1,4 +1,3 @@ -import { safeWriteJson } from "../../utils/safeWriteJson" import { Client } from "@modelcontextprotocol/sdk/client/index.js" import { StdioClientTransport, getDefaultEnvironment } from "@modelcontextprotocol/sdk/client/stdio.js" import { SSEClientTransport } from "@modelcontextprotocol/sdk/client/sse.js" @@ -1341,7 +1340,7 @@ export class McpHub { mcpServers: config.mcpServers, } - await safeWriteJson(configPath, updatedConfig) + await fs.writeFile(configPath, JSON.stringify(updatedConfig, null, 2)) } public async updateServerTimeout( @@ -1419,7 +1418,7 @@ export class McpHub { mcpServers: config.mcpServers, } - await safeWriteJson(configPath, updatedConfig) + await fs.writeFile(configPath, JSON.stringify(updatedConfig, null, 2)) // Update server connections with the correct source await this.updateServerConnections(config.mcpServers, serverSource) @@ -1561,7 +1560,7 @@ export class McpHub { } // Write updated config back to file - await safeWriteJson(normalizedPath, config) + await fs.writeFile(normalizedPath, JSON.stringify(config, null, 2)) // Update the tools list to reflect the change if (connection) { diff --git a/src/services/mcp/__tests__/McpHub.test.ts b/src/services/mcp/__tests__/McpHub.test.ts index fb22b376bb..cb0997834f 100644 --- a/src/services/mcp/__tests__/McpHub.test.ts +++ b/src/services/mcp/__tests__/McpHub.test.ts @@ -3,35 +3,8 @@ import type { ClineProvider } from "../../../core/webview/ClineProvider" import type { ExtensionContext, Uri } from "vscode" import { ServerConfigSchema } from "../McpHub" -const { McpHub } = require("../McpHub") -const path = require("path") - -// Mock fs/promises before importing anything that uses it -jest.mock("fs/promises", () => ({ - access: jest.fn().mockResolvedValue(undefined), - writeFile: jest.fn().mockResolvedValue(undefined), - readFile: jest.fn().mockResolvedValue("{}"), - unlink: jest.fn().mockResolvedValue(undefined), - rename: jest.fn().mockResolvedValue(undefined), - lstat: jest.fn().mockImplementation(() => - Promise.resolve({ - isDirectory: () => true, - }), - ), - mkdir: jest.fn().mockResolvedValue(undefined), -})) - -// Import the mocked fs const fs = require("fs/promises") - -// Mock safeWriteJson -jest.mock("../../../utils/safeWriteJson", () => ({ - safeWriteJson: jest.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 - return fs.writeFile(filePath, JSON.stringify(data), "utf8") - }), -})) +const { McpHub } = require("../McpHub") jest.mock("vscode", () => ({ workspace: { @@ -73,9 +46,6 @@ describe("McpHub", () => { // Mock console.error to suppress error messages during tests console.error = jest.fn() - // Reset the mock implementations before each test - jest.clearAllMocks() - const mockUri: Uri = { scheme: "file", authority: "", diff --git a/src/utils/__tests__/safeWriteJson.test.ts b/src/utils/__tests__/safeWriteJson.test.ts deleted file mode 100644 index 7cc95b287c..0000000000 --- a/src/utils/__tests__/safeWriteJson.test.ts +++ /dev/null @@ -1,507 +0,0 @@ -const actualFsPromises = jest.requireActual("fs/promises") -const originalFsPromisesRename = actualFsPromises.rename -const originalFsPromisesUnlink = actualFsPromises.unlink -const originalFsPromisesWriteFile = actualFsPromises.writeFile -const _originalFsPromisesAccess = actualFsPromises.access - -jest.mock("fs/promises", () => { - const actual = jest.requireActual("fs/promises") - // Start with all actual implementations. - const mockedFs = { ...actual } - - // Selectively wrap functions with jest.fn() if they are spied on - // or have their implementations changed in tests. - // This ensures that other fs.promises functions used by the SUT - // (like proper-lockfile's internals) will use their actual implementations. - mockedFs.writeFile = jest.fn(actual.writeFile) - mockedFs.readFile = jest.fn(actual.readFile) - mockedFs.rename = jest.fn(actual.rename) - mockedFs.unlink = jest.fn(actual.unlink) - mockedFs.access = jest.fn(actual.access) - mockedFs.mkdtemp = jest.fn(actual.mkdtemp) - mockedFs.rm = jest.fn(actual.rm) - mockedFs.readdir = jest.fn(actual.readdir) - // fs.stat and fs.lstat will be available via { ...actual } - - return mockedFs -}) - -// Mock the 'fs' module for fsSync.createWriteStream -jest.mock("fs", () => { - const actualFs = jest.requireActual("fs") - return { - ...actualFs, // Spread actual implementations - createWriteStream: jest.fn((...args: any[]) => actualFs.createWriteStream(...args)), // Default to actual, but mockable - } -}) - -import * as fs from "fs/promises" // This will now be the mocked version -import * as fsSyncActual from "fs" // This will now import the mocked 'fs' -import * as path from "path" -import * as os from "os" -// import * as lockfile from 'proper-lockfile' // No longer directly used in tests -import { safeWriteJson } from "../safeWriteJson" -import { Writable } from "stream" // For typing mock stream - -describe("safeWriteJson", () => { - jest.useRealTimers() // Use real timers for this test suite - - let tempTestDir: string = "" - let currentTestFilePath = "" - - beforeEach(async () => { - // Create a unique temporary directory for each test - const tempDirPrefix = path.join(os.tmpdir(), "safeWriteJson-test-") - tempTestDir = await fs.mkdtemp(tempDirPrefix) - currentTestFilePath = path.join(tempTestDir, "test-data.json") - // Ensure the file exists for locking purposes by default. - // Tests that need it to not exist must explicitly unlink it. - await fs.writeFile(currentTestFilePath, JSON.stringify({ initial: "content by beforeEach" }), "utf8") - }) - - afterEach(async () => { - if (tempTestDir) { - await fs.rm(tempTestDir, { recursive: true, force: true }) - tempTestDir = "" - } - // activeLocks is no longer used - - // Explicitly reset mock implementations to default (actual) behavior - // This helps prevent state leakage between tests if spy.mockRestore() isn't fully effective - // for functions on the module mock created by the factory. - ;(fs.writeFile as jest.Mock).mockImplementation(actualFsPromises.writeFile) - ;(fs.rename as jest.Mock).mockImplementation(actualFsPromises.rename) - ;(fs.unlink as jest.Mock).mockImplementation(actualFsPromises.unlink) - ;(fs.access as jest.Mock).mockImplementation(actualFsPromises.access) - ;(fs.readFile as jest.Mock).mockImplementation(actualFsPromises.readFile) - ;(fs.mkdtemp as jest.Mock).mockImplementation(actualFsPromises.mkdtemp) - ;(fs.rm as jest.Mock).mockImplementation(actualFsPromises.rm) - ;(fs.readdir as jest.Mock).mockImplementation(actualFsPromises.readdir) - // Ensure all mocks are reset after each test - jest.restoreAllMocks() - }) - - const readJsonFile = async (filePath: string): Promise => { - try { - const content = await fs.readFile(filePath, "utf8") // Now uses the mocked fs - return JSON.parse(content) - } catch (error: any) { - if (error && error.code === "ENOENT") { - return null // File not found - } - throw error - } - } - - const listTempFiles = async (dir: string, baseName: string): Promise => { - const files = await fs.readdir(dir) // Now uses the mocked fs - return files.filter((f: string) => f.startsWith(`.${baseName}.new_`) || f.startsWith(`.${baseName}.bak_`)) - } - - // Success Scenarios - // Note: With the beforeEach change, this test now effectively tests overwriting the initial file. - // If "creation from non-existence" is critical and locking prevents it, safeWriteJson or locking strategy needs review. - test("should successfully write a new file (overwriting initial content from beforeEach)", async () => { - const data = { message: "Hello, new world!" } - await safeWriteJson(currentTestFilePath, data) - - const writtenData = await readJsonFile(currentTestFilePath) - expect(writtenData).toEqual(data) - const tempFiles = await listTempFiles(tempTestDir, "test-data.json") - expect(tempFiles.length).toBe(0) - }) - - test("should successfully overwrite an existing file", async () => { - const initialData = { message: "Initial content" } - await fs.writeFile(currentTestFilePath, JSON.stringify(initialData)) // Now uses the mocked fs for setup - - const newData = { message: "Updated content" } - await safeWriteJson(currentTestFilePath, newData) - - const writtenData = await readJsonFile(currentTestFilePath) - expect(writtenData).toEqual(newData) - const tempFiles = await listTempFiles(tempTestDir, "test-data.json") - expect(tempFiles.length).toBe(0) - }) - - // Failure Scenarios - test("should handle failure when writing to tempNewFilePath", async () => { - // currentTestFilePath exists due to beforeEach, allowing lock acquisition. - const data = { message: "This should not be written" } - - const mockErrorStream = new Writable() as jest.Mocked & { _write?: any } - mockErrorStream._write = (_chunk: any, _encoding: any, callback: (error?: Error | null) => void) => { - // Simulate an error during write - callback(new Error("Simulated Stream Error: createWriteStream failed")) - } - - // Mock createWriteStream to simulate a failure during the streaming of data to the temp file. - ;(fsSyncActual.createWriteStream as jest.Mock).mockImplementationOnce((_path: any, _options: any) => { - const stream = new Writable({ - write(_chunk, _encoding, cb) { - cb(new Error("Simulated Stream Error: createWriteStream failed")) - }, - // Ensure destroy is handled to prevent unhandled rejections in stream internals - destroy(_error, cb) { - if (cb) cb(_error) - }, - }) - return stream as fsSyncActual.WriteStream - }) - - await expect(safeWriteJson(currentTestFilePath, data)).rejects.toThrow( - "Simulated Stream Error: createWriteStream failed", - ) - - const writtenData = await readJsonFile(currentTestFilePath) - // If write to .new fails, original file (from beforeEach) should remain. - expect(writtenData).toEqual({ initial: "content by beforeEach" }) - const tempFiles = await listTempFiles(tempTestDir, "test-data.json") - expect(tempFiles.length).toBe(0) // All temp files should be cleaned up - }) - - test("should handle failure when renaming filePath to tempBackupFilePath (filePath exists)", async () => { - const initialData = { message: "Initial content, should remain" } - await originalFsPromisesWriteFile(currentTestFilePath, JSON.stringify(initialData)) // Use original for setup - - const newData = { message: "This should not be written" } - const renameSpy = jest.spyOn(fs, "rename") - // First rename is target to backup - renameSpy.mockImplementationOnce(async (oldPath: any, newPath: any) => { - if (typeof newPath === "string" && newPath.includes(".bak_")) { - throw new Error("Simulated FS Error: rename to tempBackupFilePath") - } - return originalFsPromisesRename(oldPath, newPath) // Use constant - }) - - await expect(safeWriteJson(currentTestFilePath, newData)).rejects.toThrow( - "Simulated FS Error: rename to tempBackupFilePath", - ) - - const writtenData = await readJsonFile(currentTestFilePath) - expect(writtenData).toEqual(initialData) // Original file should be intact - const tempFiles = await listTempFiles(tempTestDir, "test-data.json") - // tempNewFile was created, but should be cleaned up. Backup was not created. - expect(tempFiles.filter((f: string) => f.includes(".new_")).length).toBe(0) - expect(tempFiles.filter((f: string) => f.includes(".bak_")).length).toBe(0) - - renameSpy.mockRestore() - }) - - test("should handle failure when renaming tempNewFilePath to filePath (filePath exists, backup succeeded)", async () => { - const initialData = { message: "Initial content, should be restored" } - await fs.writeFile(currentTestFilePath, JSON.stringify(initialData)) // Use mocked fs for setup - - const newData = { message: "This is in tempNewFilePath" } - const renameSpy = jest.spyOn(fs, "rename") - let renameCallCountTest1 = 0 - renameSpy.mockImplementation(async (oldPath: any, newPath: any) => { - const oldPathStr = oldPath.toString() - const newPathStr = newPath.toString() - renameCallCountTest1++ - console.log(`[TEST 1] fs.rename spy call #${renameCallCountTest1}: ${oldPathStr} -> ${newPathStr}`) - - // First rename call by safeWriteJson (if target exists) is target -> .bak - if (renameCallCountTest1 === 1 && !oldPathStr.includes(".new_") && newPathStr.includes(".bak_")) { - console.log("[TEST 1] Spy: Call #1 (target->backup), executing original rename.") - return originalFsPromisesRename(oldPath, newPath) - } - // Second rename call by safeWriteJson is .new -> target - else if ( - renameCallCountTest1 === 2 && - oldPathStr.includes(".new_") && - path.resolve(newPathStr) === path.resolve(currentTestFilePath) - ) { - console.log("[TEST 1] Spy: Call #2 (.new->target), THROWING SIMULATED ERROR.") - throw new Error("Simulated FS Error: rename tempNewFilePath to filePath") - } - // Fallback for unexpected calls or if the target file didn't exist (only one rename: .new -> target) - else if ( - renameCallCountTest1 === 1 && - oldPathStr.includes(".new_") && - path.resolve(newPathStr) === path.resolve(currentTestFilePath) - ) { - // This case handles if the initial file didn't exist, so only one rename happens. - // For this specific test, we expect two renames. - console.warn( - "[TEST 1] Spy: Call #1 was .new->target, (unexpected for this test scenario, but handling)", - ) - throw new Error("Simulated FS Error: rename tempNewFilePath to filePath") - } - console.warn( - `[TEST 1] Spy: Unexpected call #${renameCallCountTest1} or paths. Defaulting to original rename. ${oldPathStr} -> ${newPathStr}`, - ) - return originalFsPromisesRename(oldPath, newPath) - }) - - // This scenario should reject because the new data couldn't be written to the final path, - // even if rollback succeeds. - await expect(safeWriteJson(currentTestFilePath, newData)).rejects.toThrow( - "Simulated FS Error: rename tempNewFilePath to filePath", - ) - - const writtenData = await readJsonFile(currentTestFilePath) - expect(writtenData).toEqual(initialData) // Original file should be restored from backup - - const tempFiles = await listTempFiles(tempTestDir, "test-data.json") - expect(tempFiles.length).toBe(0) // All temp/backup files should be cleaned up - - renameSpy.mockRestore() - }) - - test("should handle failure when deleting tempBackupFilePath (filePath exists, all renames succeed)", async () => { - const initialData = { message: "Initial content" } - await fs.writeFile(currentTestFilePath, JSON.stringify(initialData)) // Use mocked fs for setup - - const newData = { message: "This should be the final content" } - const unlinkSpy = jest.spyOn(fs, "unlink") - // The unlink that targets the backup file fails - unlinkSpy.mockImplementationOnce(async (filePath: any) => { - const filePathStr = filePath.toString() - if (filePathStr.includes(".bak_")) { - console.log("[TEST unlink bak] Mock: Simulating failure for unlink backup.") - throw new Error("Simulated FS Error: delete tempBackupFilePath") - } - console.log("[TEST unlink bak] Mock: Condition NOT MET. Using originalFsPromisesUnlink.") - return originalFsPromisesUnlink(filePath) - }) - - // The function itself should still succeed from the user's perspective, - // as the primary operation (writing the new data) was successful. - // The error during backup cleanup is logged but not re-thrown to the caller. - // However, the current implementation *does* re-throw. Let's test that behavior. - // If the desired behavior is to not re-throw on backup cleanup failure, the main function needs adjustment. - // The current safeWriteJson logic is to log the error and NOT reject. - const consoleErrorSpy = jest.spyOn(console, "error").mockImplementation(() => {}) // Suppress console.error - - await expect(safeWriteJson(currentTestFilePath, newData)).resolves.toBeUndefined() - - // The main file should be the new data - const writtenData = await readJsonFile(currentTestFilePath) - expect(writtenData).toEqual(newData) - - // Check that the cleanup failure was logged - expect(consoleErrorSpy).toHaveBeenCalledWith( - expect.stringContaining(`Successfully wrote ${currentTestFilePath}, but failed to clean up backup`), - expect.objectContaining({ message: "Simulated FS Error: delete tempBackupFilePath" }), - ) - - const tempFiles = await listTempFiles(tempTestDir, "test-data.json") - // The .new file is gone (renamed to target), the .bak file failed to delete - expect(tempFiles.filter((f: string) => f.includes(".new_")).length).toBe(0) - expect(tempFiles.filter((f: string) => f.includes(".bak_")).length).toBe(1) // Backup file remains - - unlinkSpy.mockRestore() - consoleErrorSpy.mockRestore() - }) - - // Note: With beforeEach change, currentTestFilePath will exist. - // This test's original intent was "filePath does not exist". - // It will now test the "filePath exists" path for the rename mock. - // The expected error message might need to change if the mock behaves differently. - test("should handle failure when renaming tempNewFilePath to filePath (filePath initially exists)", async () => { - // currentTestFilePath exists due to beforeEach. - // The original test unlinked it; we are removing that unlink to allow locking. - const data = { message: "This should not be written" } - const renameSpy = jest.spyOn(fs, "rename") - - // The rename from tempNew to target fails. - // The mock needs to correctly simulate failure for the "filePath exists" case. - // The original mock was for "no prior file". - // For this test to be meaningful, the rename mock should simulate the failure - // appropriately when the target file (currentTestFilePath) exists. - // The existing complex mock in `test("should handle failure when renaming tempNewFilePath to filePath (filePath exists, backup succeeded)"` - // might be more relevant or adaptable here. - // For simplicity, let's use a direct mock for the second rename call (new->target). - let renameCallCount = 0 - renameSpy.mockImplementation(async (oldPath: any, newPath: any) => { - renameCallCount++ - const oldPathStr = oldPath.toString() - const newPathStr = newPath.toString() - - if (renameCallCount === 1 && !oldPathStr.includes(".new_") && newPathStr.includes(".bak_")) { - // Allow first rename (target to backup) to succeed - return originalFsPromisesRename(oldPath, newPath) - } - if ( - renameCallCount === 2 && - oldPathStr.includes(".new_") && - path.resolve(newPathStr) === path.resolve(currentTestFilePath) - ) { - // Fail the second rename (tempNew to target) - throw new Error("Simulated FS Error: rename tempNewFilePath to existing filePath") - } - return originalFsPromisesRename(oldPath, newPath) - }) - - await expect(safeWriteJson(currentTestFilePath, data)).rejects.toThrow( - "Simulated FS Error: rename tempNewFilePath to existing filePath", - ) - - // After failure, the original content (from beforeEach or backup) should be there. - const writtenData = await readJsonFile(currentTestFilePath) - expect(writtenData).toEqual({ initial: "content by beforeEach" }) // Expect restored content - // The assertion `expect(writtenData).toBeNull()` was incorrect if rollback is successful. - const tempFiles = await listTempFiles(tempTestDir, "test-data.json") - expect(tempFiles.length).toBe(0) // All temp files should be cleaned up - - renameSpy.mockRestore() - }) - - test("should throw an error if an inter-process lock is already held for the filePath", async () => { - jest.resetModules() // Clear module cache to ensure fresh imports for this test - - const data = { message: "test lock" } - // Ensure the resource file exists. - await fs.writeFile(currentTestFilePath, "{}", "utf8") - - // Temporarily mock proper-lockfile for this test only - jest.doMock("proper-lockfile", () => ({ - ...jest.requireActual("proper-lockfile"), - lock: jest.fn().mockRejectedValueOnce(new Error("Failed to get lock.")), - })) - - // Re-require safeWriteJson so it picks up the mocked proper-lockfile - const { safeWriteJson: safeWriteJsonWithMockedLock } = - require("../safeWriteJson") as typeof import("../safeWriteJson") - - try { - await expect(safeWriteJsonWithMockedLock(currentTestFilePath, data)).rejects.toThrow( - /Failed to get lock.|Lock file is already being held/i, - ) - } finally { - jest.unmock("proper-lockfile") // Ensure the mock is removed after this test - } - }) - test("should release lock even if an error occurs mid-operation", async () => { - const data = { message: "test lock release on error" } - - // Mock createWriteStream to simulate a failure during the streaming of data, - // to test if the lock is released despite this mid-operation error. - ;(fsSyncActual.createWriteStream as jest.Mock).mockImplementationOnce((_path: any, _options: any) => { - const stream = new Writable({ - write(_chunk, _encoding, cb) { - cb(new Error("Simulated Stream Error during mid-operation write")) - }, - // Ensure destroy is handled - destroy(_error, cb) { - if (cb) cb(_error) - }, - }) - return stream as fsSyncActual.WriteStream - }) - - await expect(safeWriteJson(currentTestFilePath, data)).rejects.toThrow( - "Simulated Stream Error during mid-operation write", - ) - - // Lock should be released, meaning the .lock file should not exist - const lockPath = `${path.resolve(currentTestFilePath)}.lock` - await expect(fs.access(lockPath)).rejects.toThrow(expect.objectContaining({ code: "ENOENT" })) - }) - - test("should handle fs.access error that is not ENOENT", async () => { - const data = { message: "access error test" } - const accessSpy = jest.spyOn(fs, "access").mockImplementationOnce(async () => { - const err = new Error("Simulated EACCES Error") as NodeJS.ErrnoException - err.code = "EACCES" // Simulate a permissions error, for example - throw err - }) - - await expect(safeWriteJson(currentTestFilePath, data)).rejects.toThrow("Simulated EACCES Error") - - // Lock should be released, meaning the .lock file should not exist - const lockPath = `${path.resolve(currentTestFilePath)}.lock` - await expect(fs.access(lockPath)).rejects.toThrow(expect.objectContaining({ code: "ENOENT" })) - - const tempFiles = await listTempFiles(tempTestDir, "test-data.json") - // .new file might have been created before access check, should be cleaned up - expect(tempFiles.filter((f: string) => f.includes(".new_")).length).toBe(0) - - accessSpy.mockRestore() - }) - - // Test for rollback failure scenario - test("should log error and re-throw original if rollback fails", async () => { - const initialData = { message: "Initial, should be lost if rollback fails" } - await fs.writeFile(currentTestFilePath, JSON.stringify(initialData)) // Use mocked fs for setup - const newData = { message: "New data" } - - const renameSpy = jest.spyOn(fs, "rename") - const consoleErrorSpy = jest.spyOn(console, "error").mockImplementation(() => {}) // Suppress console.error - let renameCallCountTest2 = 0 - - renameSpy.mockImplementation(async (oldPath: any, newPath: any) => { - const oldPathStr = oldPath.toString() - const newPathStr = newPath.toString() - renameCallCountTest2++ - const resolvedOldPath = path.resolve(oldPathStr) - const resolvedNewPath = path.resolve(newPathStr) - const resolvedCurrentTFP = path.resolve(currentTestFilePath) - console.log( - `[TEST 2] fs.promises.rename call #${renameCallCountTest2}: oldPath=${oldPathStr} (resolved: ${resolvedOldPath}), newPath=${newPathStr} (resolved: ${resolvedNewPath}), currentTFP (resolved: ${resolvedCurrentTFP})`, - ) - - if (renameCallCountTest2 === 1) { - // Call 1: Original -> Backup (Succeeds) - if (resolvedOldPath === resolvedCurrentTFP && newPathStr.includes(".bak_")) { - console.log("[TEST 2] Call #1 (Original->Backup): Condition MET. originalFsPromisesRename.") - return originalFsPromisesRename(oldPath, newPath) - } - console.error("[TEST 2] Call #1: UNEXPECTED args.") - throw new Error("Unexpected args for rename call #1 in test") - } else if (renameCallCountTest2 === 2) { - // Call 2: New -> Original (Fails - this is the "original error") - if (oldPathStr.includes(".new_") && resolvedNewPath === resolvedCurrentTFP) { - console.log( - '[TEST 2] Call #2 (New->Original): Condition MET. Throwing "Simulated FS Error: new to original".', - ) - throw new Error("Simulated FS Error: new to original") - } - console.error("[TEST 2] Call #2: UNEXPECTED args.") - throw new Error("Unexpected args for rename call #2 in test") - } else if (renameCallCountTest2 === 3) { - // Call 3: Backup -> Original (Rollback attempt - Fails) - if (oldPathStr.includes(".bak_") && resolvedNewPath === resolvedCurrentTFP) { - console.log( - '[TEST 2] Call #3 (Backup->Original Rollback): Condition MET. Throwing "Simulated FS Error: backup to original (rollback)".', - ) - throw new Error("Simulated FS Error: backup to original (rollback)") - } - console.error("[TEST 2] Call #3: UNEXPECTED args.") - throw new Error("Unexpected args for rename call #3 in test") - } - console.error(`[TEST 2] Unexpected fs.promises.rename call count: ${renameCallCountTest2}`) - return originalFsPromisesRename(oldPath, newPath) - }) - - await expect(safeWriteJson(currentTestFilePath, newData)).rejects.toThrow("Simulated FS Error: new to original") - - // Check that the rollback failure was logged - expect(consoleErrorSpy).toHaveBeenCalledWith( - expect.stringContaining( - `Operation failed for ${path.resolve(currentTestFilePath)}: [Original Error Caught]`, - ), - expect.objectContaining({ message: "Simulated FS Error: new to original" }), // The original error - ) - expect(consoleErrorSpy).toHaveBeenCalledWith( - expect.stringMatching(/\[Catch\] Failed to restore backup .*?\.bak_.*?\s+to .*?:/), // Matches the backup filename pattern - expect.objectContaining({ message: "Simulated FS Error: backup to original (rollback)" }), // The rollback error - ) - // The original error is logged first in safeWriteJson's catch block, then the rollback failure. - - // File system state: original file is lost (backup couldn't be restored and was then unlinked), - // new file was cleaned up. The target path `currentTestFilePath` should not exist. - const finalState = await readJsonFile(currentTestFilePath) - expect(finalState).toBeNull() - - const tempFiles = await listTempFiles(tempTestDir, "test-data.json") - // Backup file should also be cleaned up by the final unlink attempt in safeWriteJson's catch block, - // as that unlink is not mocked to fail. - expect(tempFiles.filter((f: string) => f.includes(".bak_")).length).toBe(0) - expect(tempFiles.filter((f: string) => f.includes(".new_")).length).toBe(0) - - renameSpy.mockRestore() - consoleErrorSpy.mockRestore() - }) -}) diff --git a/src/utils/safeWriteJson.ts b/src/utils/safeWriteJson.ts deleted file mode 100644 index 0be2bf822e..0000000000 --- a/src/utils/safeWriteJson.ts +++ /dev/null @@ -1,218 +0,0 @@ -import * as fs from "fs/promises" -import * as fsSync from "fs" -import * as path from "path" -import * as lockfile from "proper-lockfile" -import Disassembler from "stream-json/Disassembler" -import Stringer from "stream-json/Stringer" - -/** - * Safely writes JSON data to a file. - * - Uses 'proper-lockfile' for inter-process advisory locking to prevent concurrent writes to the same path. - * - Writes to a temporary file first. - * - If the target file exists, it's backed up before being replaced. - * - Attempts to roll back and clean up in case of errors. - * - * @param {string} filePath - The absolute path to the target file. - * @param {any} data - The data to serialize to JSON and write. - * @returns {Promise} - */ - -async function safeWriteJson(filePath: string, data: any): Promise { - const absoluteFilePath = path.resolve(filePath) - let releaseLock = async () => {} // Initialized to a no-op - - // Acquire the lock before any file operations - try { - releaseLock = await lockfile.lock(absoluteFilePath, { - stale: 31000, // Stale after 31 seconds - update: 10000, // Update mtime every 10 seconds to prevent staleness if operation is long - retries: { - // Configuration for retrying lock acquisition - retries: 5, // Number of retries after the initial attempt - factor: 2, // Exponential backoff factor (e.g., 100ms, 200ms, 400ms, ...) - minTimeout: 100, // Minimum time to wait before the first retry (in ms) - maxTimeout: 1000, // Maximum time to wait for any single retry (in ms) - }, - onCompromised: (err) => { - console.error(`Lock at ${absoluteFilePath} was compromised:`, err) - throw err - }, - }) - } catch (lockError) { - // If lock acquisition fails, we throw immediately. - // The releaseLock remains a no-op, so the finally block in the main file operations - // try-catch-finally won't try to release an unacquired lock if this path is taken. - console.error(`Failed to acquire lock for ${absoluteFilePath}:`, lockError) - // Propagate the lock acquisition error - throw lockError - } - - // Variables to hold the actual paths of temp files if they are created. - let actualTempNewFilePath: string | null = null - let actualTempBackupFilePath: string | null = null - - try { - // Step 1: Write data to a new temporary file. - actualTempNewFilePath = path.join( - path.dirname(absoluteFilePath), - `.${path.basename(absoluteFilePath)}.new_${Date.now()}_${Math.random().toString(36).substring(2)}.tmp`, - ) - - await _streamDataToFile(actualTempNewFilePath, data) - - // Step 2: Check if the target file exists. If so, rename it to a backup path. - try { - // Check for target file existence - await fs.access(absoluteFilePath) - // Target exists, create a backup path and rename. - actualTempBackupFilePath = path.join( - path.dirname(absoluteFilePath), - `.${path.basename(absoluteFilePath)}.bak_${Date.now()}_${Math.random().toString(36).substring(2)}.tmp`, - ) - await fs.rename(absoluteFilePath, actualTempBackupFilePath) - } catch (accessError: any) { - // Explicitly type accessError - if (accessError.code !== "ENOENT") { - // An error other than "file not found" occurred during access check. - throw accessError - } - // Target file does not exist, so no backup is made. actualTempBackupFilePath remains null. - } - - // Step 3: Rename the new temporary file to the target file path. - // This is the main "commit" step. - await fs.rename(actualTempNewFilePath, absoluteFilePath) - - // If we reach here, the new file is successfully in place. - // The original actualTempNewFilePath is now the main file, so we shouldn't try to clean it up as "temp". - // Mark as "used" or "committed" - actualTempNewFilePath = null - - // Step 4: If a backup was created, attempt to delete it. - if (actualTempBackupFilePath) { - try { - await fs.unlink(actualTempBackupFilePath) - // Mark backup as handled - actualTempBackupFilePath = null - } catch (unlinkBackupError) { - // Log this error, but do not re-throw. The main operation was successful. - // actualTempBackupFilePath remains set, indicating an orphaned backup. - console.error( - `Successfully wrote ${absoluteFilePath}, but failed to clean up backup ${actualTempBackupFilePath}:`, - unlinkBackupError, - ) - } - } - } catch (originalError) { - console.error(`Operation failed for ${absoluteFilePath}: [Original Error Caught]`, originalError) - - const newFileToCleanupWithinCatch = actualTempNewFilePath - const backupFileToRollbackOrCleanupWithinCatch = actualTempBackupFilePath - - // Attempt rollback if a backup was made - if (backupFileToRollbackOrCleanupWithinCatch) { - try { - await fs.rename(backupFileToRollbackOrCleanupWithinCatch, absoluteFilePath) - // Mark as handled, prevent later unlink of this path - actualTempBackupFilePath = null - } catch (rollbackError) { - // actualTempBackupFilePath (outer scope) remains pointing to backupFileToRollbackOrCleanupWithinCatch - console.error( - `[Catch] Failed to restore backup ${backupFileToRollbackOrCleanupWithinCatch} to ${absoluteFilePath}:`, - rollbackError, - ) - } - } - - // Cleanup the .new file if it exists - if (newFileToCleanupWithinCatch) { - try { - await fs.unlink(newFileToCleanupWithinCatch) - } catch (cleanupError) { - console.error( - `[Catch] Failed to clean up temporary new file ${newFileToCleanupWithinCatch}:`, - cleanupError, - ) - } - } - - // Cleanup the .bak file if it still needs to be (i.e., wasn't successfully restored) - if (actualTempBackupFilePath) { - try { - await fs.unlink(actualTempBackupFilePath) - } catch (cleanupError) { - console.error( - `[Catch] Failed to clean up temporary backup file ${actualTempBackupFilePath}:`, - cleanupError, - ) - } - } - throw originalError // This MUST be the error that rejects the promise. - } finally { - // Release the lock in the main finally block. - try { - // releaseLock will be the actual unlock function if lock was acquired, - // or the initial no-op if acquisition failed. - await releaseLock() - } catch (unlockError) { - // Do not re-throw here, as the originalError from the try/catch (if any) is more important. - console.error(`Failed to release lock for ${absoluteFilePath}:`, unlockError) - } - } -} - -/** - * Helper function to stream JSON data to a file. - * @param targetPath The path to write the stream to. - * @param data The data to stream. - * @returns Promise - */ -async function _streamDataToFile(targetPath: string, data: any): Promise { - // Stream data to avoid high memory usage for large JSON objects. - const fileWriteStream = fsSync.createWriteStream(targetPath, { encoding: "utf8" }) - const disassembler = Disassembler.disassembler() - // Output will be compact JSON as standard Stringer is used. - const stringer = Stringer.stringer() - - return new Promise((resolve, reject) => { - let errorOccurred = false - const handleError = (_streamName: string) => (err: Error) => { - if (!errorOccurred) { - errorOccurred = true - if (!fileWriteStream.destroyed) { - fileWriteStream.destroy(err) - } - reject(err) - } - } - - disassembler.on("error", handleError("Disassembler")) - stringer.on("error", handleError("Stringer")) - fileWriteStream.on("error", (err: Error) => { - if (!errorOccurred) { - errorOccurred = true - reject(err) - } - }) - - fileWriteStream.on("finish", () => { - if (!errorOccurred) { - resolve() - } - }) - - disassembler.pipe(stringer).pipe(fileWriteStream) - - // stream-json's Disassembler might error if `data` is undefined. - // JSON.stringify(undefined) would produce the string "undefined" if it's the root value. - // Writing 'null' is a safer JSON representation for a root undefined value. - if (data === undefined) { - disassembler.write(null) - } else { - disassembler.write(data) - } - disassembler.end() - }) -} - -export { safeWriteJson }