Skip to content

Commit 18e0fcd

Browse files
author
Eric Wheeler
committed
refactor: replace fs.readFile + JSON.parse with safeReadJson
Replace manual file reading and JSON parsing with the safer safeReadJson utility across multiple files in the codebase. This change: - Provides atomic file access with proper locking to prevent race conditions - Streams file contents efficiently for better memory usage - Improves error handling consistency - Reduces code duplication Fixes: #5331 Signed-off-by: Eric Wheeler <[email protected]>
1 parent 7db09e5 commit 18e0fcd

File tree

13 files changed

+105
-99
lines changed

13 files changed

+105
-99
lines changed

‎src/api/providers/fetchers/modelCache.ts‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,12 @@ import * as path from "path"
22
import fs from "fs/promises"
33

44
import NodeCache from "node-cache"
5+
import { safeReadJson } from "../../../utils/safeReadJson"
56
import { safeWriteJson } from "../../../utils/safeWriteJson"
67

78
import { ContextProxy } from "../../../core/config/ContextProxy"
89
import { getCacheDirectoryPath } from "../../../utils/storage"
910
import { RouterName, ModelRecord } from "../../../shared/api"
10-
import { fileExistsAtPath } from "../../../utils/fs"
1111

1212
import { getOpenRouterModels } from "./openrouter"
1313
import { getRequestyModels } from "./requesty"
@@ -30,8 +30,14 @@ async function readModels(router: RouterName): Promise<ModelRecord | undefined>
3030
const filename = `${router}_models.json`
3131
const cacheDir = await getCacheDirectoryPath(ContextProxy.instance.globalStorageUri.fsPath)
3232
const filePath = path.join(cacheDir, filename)
33-
const exists = await fileExistsAtPath(filePath)
34-
return exists ? JSON.parse(await fs.readFile(filePath, "utf8")) : undefined
33+
try {
34+
return await safeReadJson(filePath)
35+
} catch (error: any) {
36+
if (error.code === "ENOENT") {
37+
return undefined
38+
}
39+
throw error
40+
}
3541
}
3642

3743
/**

‎src/api/providers/fetchers/modelEndpointCache.ts‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,13 +2,13 @@ import * as path from "path"
22
import fs from "fs/promises"
33

44
import NodeCache from "node-cache"
5+
import { safeReadJson } from "../../../utils/safeReadJson"
56
import { safeWriteJson } from "../../../utils/safeWriteJson"
67
import sanitize from "sanitize-filename"
78

89
import { ContextProxy } from "../../../core/config/ContextProxy"
910
import { getCacheDirectoryPath } from "../../../utils/storage"
1011
import { RouterName, ModelRecord } from "../../../shared/api"
11-
import { fileExistsAtPath } from "../../../utils/fs"
1212

1313
import { getOpenRouterModelEndpoints } from "./openrouter"
1414

@@ -26,8 +26,11 @@ async function readModelEndpoints(key: string): Promise<ModelRecord | undefined>
2626
const filename = `${key}_endpoints.json`
2727
const cacheDir = await getCacheDirectoryPath(ContextProxy.instance.globalStorageUri.fsPath)
2828
const filePath = path.join(cacheDir, filename)
29-
const exists = await fileExistsAtPath(filePath)
30-
return exists ? JSON.parse(await fs.readFile(filePath, "utf8")) : undefined
29+
try {
30+
return await safeReadJson(filePath)
31+
} catch (error) {
32+
return undefined
33+
}
3134
}
3235

3336
export const getModelEndpoints = async ({

‎src/core/config/importExport.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import { safeReadJson } from "../../utils/safeReadJson"
12
import { safeWriteJson } from "../../utils/safeWriteJson"
23
import os from "os"
34
import * as path from "path"
@@ -49,7 +50,7 @@ export async function importSettingsFromPath(
4950
const previousProviderProfiles = await providerSettingsManager.export()
5051

5152
const { providerProfiles: newProviderProfiles, globalSettings = {} } = schema.parse(
52-
JSON.parse(await fs.readFile(filePath, "utf-8")),
53+
await safeReadJson(filePath),
5354
)
5455

5556
const providerProfiles = {

‎src/core/context-tracking/FileContextTracker.ts‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,9 @@
1+
import { safeReadJson } from "../../utils/safeReadJson"
12
import { safeWriteJson } from "../../utils/safeWriteJson"
23
import * as path from "path"
34
import * as vscode from "vscode"
45
import { getTaskDirectoryPath } from "../../utils/storage"
56
import { GlobalFileNames } from "../../shared/globalFileNames"
6-
import { fileExistsAtPath } from "../../utils/fs"
7-
import fs from "fs/promises"
87
import { ContextProxy } from "../config/ContextProxy"
98
import type { FileMetadataEntry, RecordSource, TaskMetadata } from "./FileContextTrackerTypes"
109
import { ClineProvider } from "../webview/ClineProvider"
@@ -116,12 +115,14 @@ export class FileContextTracker {
116115
const taskDir = await getTaskDirectoryPath(globalStoragePath, taskId)
117116
const filePath = path.join(taskDir, GlobalFileNames.taskMetadata)
118117
try {
119-
if (await fileExistsAtPath(filePath)) {
120-
return JSON.parse(await fs.readFile(filePath, "utf8"))
121-
}
118+
return await safeReadJson(filePath)
122119
} catch (error) {
123-
console.error("Failed to read task metadata:", error)
120+
if (error.code !== "ENOENT") {
121+
console.error("Failed to read task metadata:", error)
122+
}
124123
}
124+
125+
// On error, return default empty metadata
125126
return { files_in_context: [] }
126127
}
127128

‎src/core/task-persistence/apiMessages.ts‎

Lines changed: 29 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import { safeReadJson } from "../../utils/safeReadJson"
12
import { safeWriteJson } from "../../utils/safeWriteJson"
23
import * as path from "path"
34
import * as fs from "fs/promises"
@@ -21,51 +22,51 @@ export async function readApiMessages({
2122
const taskDir = await getTaskDirectoryPath(globalStoragePath, taskId)
2223
const filePath = path.join(taskDir, GlobalFileNames.apiConversationHistory)
2324

24-
if (await fileExistsAtPath(filePath)) {
25-
const fileContent = await fs.readFile(filePath, "utf8")
26-
try {
27-
const parsedData = JSON.parse(fileContent)
28-
if (Array.isArray(parsedData) && parsedData.length === 0) {
29-
console.error(
30-
`[Roo-Debug] readApiMessages: Found API conversation history file, but it's empty (parsed as []). TaskId: ${taskId}, Path: ${filePath}`,
31-
)
32-
}
33-
return parsedData
34-
} catch (error) {
25+
try {
26+
const parsedData = await safeReadJson(filePath)
27+
if (Array.isArray(parsedData) && parsedData.length === 0) {
3528
console.error(
36-
`[Roo-Debug] readApiMessages: Error parsing API conversation history file. TaskId: ${taskId}, Path: ${filePath}, Error: ${error}`,
29+
`[Roo-Debug] readApiMessages: Found API conversation history file, but it's empty (parsed as []). TaskId: ${taskId}, Path: ${filePath}`,
3730
)
38-
throw error
3931
}
40-
} else {
41-
const oldPath = path.join(taskDir, "claude_messages.json")
32+
return parsedData
33+
} catch (error: any) {
34+
if (error.code === "ENOENT") {
35+
// File doesn't exist, try the old path
36+
const oldPath = path.join(taskDir, "claude_messages.json")
4237

43-
if (await fileExistsAtPath(oldPath)) {
44-
const fileContent = await fs.readFile(oldPath, "utf8")
4538
try {
46-
const parsedData = JSON.parse(fileContent)
39+
const parsedData = await safeReadJson(oldPath)
4740
if (Array.isArray(parsedData) && parsedData.length === 0) {
4841
console.error(
4942
`[Roo-Debug] readApiMessages: Found OLD API conversation history file (claude_messages.json), but it's empty (parsed as []). TaskId: ${taskId}, Path: ${oldPath}`,
5043
)
5144
}
5245
await fs.unlink(oldPath)
5346
return parsedData
54-
} catch (error) {
47+
} catch (oldError: any) {
48+
if (oldError.code === "ENOENT") {
49+
// If we reach here, neither the new nor the old history file was found.
50+
console.error(
51+
`[Roo-Debug] readApiMessages: API conversation history file not found for taskId: ${taskId}. Expected at: ${filePath}`,
52+
)
53+
return []
54+
}
55+
56+
// For any other error with the old file, log and rethrow
5557
console.error(
56-
`[Roo-Debug] readApiMessages: Error parsing OLD API conversation history file (claude_messages.json). TaskId: ${taskId}, Path: ${oldPath}, Error: ${error}`,
58+
`[Roo-Debug] readApiMessages: Error reading OLD API conversation history file (claude_messages.json). TaskId: ${taskId}, Path: ${oldPath}, Error: ${oldError}`,
5759
)
58-
// DO NOT unlink oldPath if parsing failed, throw error instead.
59-
throw error
60+
throw oldError
6061
}
62+
} else {
63+
// For any other error with the main file, log and rethrow
64+
console.error(
65+
`[Roo-Debug] readApiMessages: Error reading API conversation history file. TaskId: ${taskId}, Path: ${filePath}, Error: ${error}`,
66+
)
67+
throw error
6168
}
6269
}
63-
64-
// If we reach here, neither the new nor the old history file was found.
65-
console.error(
66-
`[Roo-Debug] readApiMessages: API conversation history file not found for taskId: ${taskId}. Expected at: ${filePath}`,
67-
)
68-
return []
6970
}
7071

7172
export async function saveApiMessages({

‎src/core/task-persistence/taskMessages.ts‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,9 @@
1+
import { safeReadJson } from "../../utils/safeReadJson"
12
import { safeWriteJson } from "../../utils/safeWriteJson"
23
import * as path from "path"
3-
import * as fs from "fs/promises"
44

55
import type { ClineMessage } from "@roo-code/types"
66

7-
import { fileExistsAtPath } from "../../utils/fs"
8-
97
import { GlobalFileNames } from "../../shared/globalFileNames"
108
import { getTaskDirectoryPath } from "../../utils/storage"
119

@@ -20,13 +18,15 @@ export async function readTaskMessages({
2018
}: ReadTaskMessagesOptions): Promise<ClineMessage[]> {
2119
const taskDir = await getTaskDirectoryPath(globalStoragePath, taskId)
2220
const filePath = path.join(taskDir, GlobalFileNames.uiMessages)
23-
const fileExists = await fileExistsAtPath(filePath)
2421

25-
if (fileExists) {
26-
return JSON.parse(await fs.readFile(filePath, "utf8"))
22+
try {
23+
return await safeReadJson(filePath)
24+
} catch (error) {
25+
if (error.code !== "ENOENT") {
26+
console.error("Failed to read task messages:", error)
27+
}
28+
return []
2729
}
28-
29-
return []
3030
}
3131

3232
export type SaveTaskMessagesOptions = {

‎src/core/webview/ClineProvider.ts‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,7 @@ import type { IndexProgressUpdate } from "../../services/code-index/interfaces/m
5555
import { MdmService } from "../../services/mdm/MdmService"
5656
import { fileExistsAtPath } from "../../utils/fs"
5757
import { setTtsEnabled, setTtsSpeed } from "../../utils/tts"
58+
import { safeReadJson } from "../../utils/safeReadJson"
5859
import { ContextProxy } from "../config/ContextProxy"
5960
import { ProviderSettingsManager } from "../config/ProviderSettingsManager"
6061
import { CustomModesManager } from "../config/CustomModesManager"
@@ -1136,10 +1137,9 @@ export class ClineProvider
11361137
const taskDirPath = await getTaskDirectoryPath(globalStoragePath, id)
11371138
const apiConversationHistoryFilePath = path.join(taskDirPath, GlobalFileNames.apiConversationHistory)
11381139
const uiMessagesFilePath = path.join(taskDirPath, GlobalFileNames.uiMessages)
1139-
const fileExists = await fileExistsAtPath(apiConversationHistoryFilePath)
11401140

1141-
if (fileExists) {
1142-
const apiConversationHistory = JSON.parse(await fs.readFile(apiConversationHistoryFilePath, "utf8"))
1141+
try {
1142+
const apiConversationHistory = await safeReadJson(apiConversationHistoryFilePath)
11431143

11441144
return {
11451145
historyItem,
@@ -1148,6 +1148,10 @@ export class ClineProvider
11481148
uiMessagesFilePath,
11491149
apiConversationHistory,
11501150
}
1151+
} catch (error) {
1152+
if (error.code !== "ENOENT") {
1153+
console.error(`Failed to read API conversation history for task ${id}:`, error)
1154+
}
11511155
}
11521156
}
11531157

‎src/integrations/misc/extract-text.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import mammoth from "mammoth"
55
import fs from "fs/promises"
66
import { isBinaryFile } from "isbinaryfile"
77
import { extractTextFromXLSX } from "./extract-text-from-xlsx"
8+
import { safeReadJson } from "../../utils/safeReadJson"
89

910
async function extractTextFromPDF(filePath: string): Promise<string> {
1011
const dataBuffer = await fs.readFile(filePath)
@@ -18,8 +19,7 @@ async function extractTextFromDOCX(filePath: string): Promise<string> {
1819
}
1920

2021
async function extractTextFromIPYNB(filePath: string): Promise<string> {
21-
const data = await fs.readFile(filePath, "utf8")
22-
const notebook = JSON.parse(data)
22+
const notebook = await safeReadJson(filePath)
2323
let extractedText = ""
2424

2525
for (const cell of notebook.cells) {

‎src/services/code-index/cache-manager.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import * as vscode from "vscode"
22
import { createHash } from "crypto"
33
import { ICacheManager } from "./interfaces/cache"
44
import debounce from "lodash.debounce"
5+
import { safeReadJson } from "../../utils/safeReadJson"
56
import { safeWriteJson } from "../../utils/safeWriteJson"
67
import { TelemetryService } from "@roo-code/telemetry"
78
import { TelemetryEventName } from "@roo-code/types"
@@ -37,8 +38,7 @@ export class CacheManager implements ICacheManager {
3738
*/
3839
async initialize(): Promise<void> {
3940
try {
40-
const cacheData = await vscode.workspace.fs.readFile(this.cachePath)
41-
this.fileHashes = JSON.parse(cacheData.toString())
41+
this.fileHashes = await safeReadJson(this.cachePath.fsPath)
4242
} catch (error) {
4343
this.fileHashes = {}
4444
TelemetryService.instance.captureEvent(TelemetryEventName.CODE_INDEX_ERROR, {

‎src/services/marketplace/MarketplaceManager.ts‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import { GlobalFileNames } from "../../shared/globalFileNames"
99
import { ensureSettingsDirectoryExists } from "../../utils/globalContext"
1010
import { t } from "../../i18n"
1111
import { TelemetryService } from "@roo-code/telemetry"
12+
import { safeReadJson } from "../../utils/safeReadJson"
1213

1314
export class MarketplaceManager {
1415
private configLoader: RemoteConfigLoader
@@ -218,8 +219,7 @@ export class MarketplaceManager {
218219
// Check MCPs in .roo/mcp.json
219220
const projectMcpPath = path.join(workspaceFolder.uri.fsPath, ".roo", "mcp.json")
220221
try {
221-
const content = await fs.readFile(projectMcpPath, "utf-8")
222-
const data = JSON.parse(content)
222+
const data = await safeReadJson(projectMcpPath)
223223
if (data?.mcpServers && typeof data.mcpServers === "object") {
224224
for (const serverName of Object.keys(data.mcpServers)) {
225225
metadata[serverName] = {
@@ -263,8 +263,7 @@ export class MarketplaceManager {
263263
// Check global MCPs
264264
const globalMcpPath = path.join(globalSettingsPath, GlobalFileNames.mcpSettings)
265265
try {
266-
const content = await fs.readFile(globalMcpPath, "utf-8")
267-
const data = JSON.parse(content)
266+
const data = await safeReadJson(globalMcpPath)
268267
if (data?.mcpServers && typeof data.mcpServers === "object") {
269268
for (const serverName of Object.keys(data.mcpServers)) {
270269
metadata[serverName] = {

0 commit comments

Comments
 (0)