diff --git a/src/config/schema/hooks.ts b/src/config/schema/hooks.ts index 80e4c71dd..641825da1 100644 --- a/src/config/schema/hooks.ts +++ b/src/config/schema/hooks.ts @@ -55,6 +55,7 @@ export const HookNameSchema = z.enum([ "read-image-resizer", "todo-description-override", "webfetch-redirect-guard", + "fsync-skip-warning", "legacy-plugin-toast", ]) diff --git a/src/features/team-mode/team-state-store/locks.ts b/src/features/team-mode/team-state-store/locks.ts index 0fca98494..2f7a4d4a0 100644 --- a/src/features/team-mode/team-state-store/locks.ts +++ b/src/features/team-mode/team-state-store/locks.ts @@ -1,6 +1,8 @@ import { randomUUID } from "node:crypto" import { open, readFile, rename, rm, unlink, writeFile } from "node:fs/promises" +import { tolerantFsync } from "../../../shared/tolerant-fsync" + type LockOptions = { staleAfterMs?: number ownerTag?: string @@ -51,7 +53,7 @@ async function acquireLock(lockPath: string, ownerTag: string, staleAfterMs: num const fileHandle = await open(lockPath, "wx") try { await fileHandle.writeFile(buildOwnerContent(ownerTag)) - await fileHandle.sync() + await tolerantFsync(fileHandle, `acquireLock:${lockPath}`) } finally { await fileHandle.close() } @@ -116,7 +118,7 @@ export async function atomicWrite( await writeFile(tmpPath, content) const fileHandle = await open(tmpPath, "r") try { - await fileHandle.sync() + await tolerantFsync(fileHandle, `atomicWrite:${filePath}`) } finally { await fileHandle.close() } diff --git a/src/hooks/fsync-skip-warning/index.test.ts b/src/hooks/fsync-skip-warning/index.test.ts new file mode 100644 index 000000000..e7c241e83 --- /dev/null +++ b/src/hooks/fsync-skip-warning/index.test.ts @@ -0,0 +1,98 @@ +import { beforeEach, describe, expect, it } from "bun:test" + +import { classifyPathEnvironment } from "../../shared/classify-path-environment" +import { clearAllSkips, recordFsyncSkip } from "../../shared/fsync-skip-tracker" +import { createFsyncSkipWarningHook } from "./index" + +describe("createFsyncSkipWarningHook", () => { + beforeEach(() => { + clearAllSkips() + }) + + it("records callID start timestamp in tool.execute.before", async () => { + const hook = createFsyncSkipWarningHook() + const input = { tool: "bash", sessionID: "ses1", callID: "call-1" } + const output = { args: {} as Record } + + await hook["tool.execute.before"](input, output) + await Bun.sleep(2) + + recordFsyncSkip({ + filePath: "/tmp/a", + contextLabel: "atomicWrite:/tmp/a", + errorCode: "EPERM", + message: "operation not permitted", + pathClassification: classifyPathEnvironment("/tmp/a"), + }) + + const afterOutput = { title: "ok", output: "done", metadata: {} as Record } + await hook["tool.execute.after"](input, afterOutput) + + expect(afterOutput.output).toContain("[fsync-skipped]") + }) + + it("drains skips after start time and appends warning to output text", async () => { + const hook = createFsyncSkipWarningHook() + const input = { tool: "write", sessionID: "ses1", callID: "call-2" } + const beforeOutput = { args: {} as Record } + const afterOutput = { title: "ok", output: "base", metadata: {} as Record } + + await hook["tool.execute.before"](input, beforeOutput) + await Bun.sleep(2) + + recordFsyncSkip({ + filePath: "/Users/x/OneDrive/a", + contextLabel: "atomicWrite:/Users/x/OneDrive/a", + errorCode: "EPERM", + message: "operation not permitted", + pathClassification: classifyPathEnvironment("/Users/x/OneDrive/a"), + }) + + await hook["tool.execute.after"](input, afterOutput) + + expect(afterOutput.output).toContain("base\n\n---") + expect(afterOutput.output).toContain("OneDrive") + }) + + it("leaves output unchanged when no skips happen during window", async () => { + const hook = createFsyncSkipWarningHook() + const input = { tool: "write", sessionID: "ses1", callID: "call-3" } + const beforeOutput = { args: {} as Record } + const afterOutput = { title: "ok", output: "base", metadata: {} as Record } + + await hook["tool.execute.before"](input, beforeOutput) + await hook["tool.execute.after"](input, afterOutput) + + expect(afterOutput.output).toBe("base") + }) + + it("isolates multiple parallel calls by callID watermark", async () => { + const hook = createFsyncSkipWarningHook() + const beforeOutput = { args: {} as Record } + + const inputA = { tool: "write", sessionID: "ses1", callID: "call-A" } + const inputB = { tool: "write", sessionID: "ses1", callID: "call-B" } + + await hook["tool.execute.before"](inputA, beforeOutput) + await Bun.sleep(2) + await hook["tool.execute.before"](inputB, beforeOutput) + await Bun.sleep(2) + + recordFsyncSkip({ + filePath: "/tmp/a", + contextLabel: "atomicWrite:/tmp/a", + errorCode: "EPERM", + message: "operation not permitted", + pathClassification: classifyPathEnvironment("/tmp/a"), + }) + + const outputA = { title: "ok", output: "A", metadata: {} as Record } + const outputB = { title: "ok", output: "B", metadata: {} as Record } + + await hook["tool.execute.after"](inputA, outputA) + await hook["tool.execute.after"](inputB, outputB) + + expect(outputA.output).toContain("[fsync-skipped]") + expect(outputB.output).toBe("B") + }) +}) diff --git a/src/hooks/fsync-skip-warning/index.ts b/src/hooks/fsync-skip-warning/index.ts new file mode 100644 index 000000000..fb59399be --- /dev/null +++ b/src/hooks/fsync-skip-warning/index.ts @@ -0,0 +1,50 @@ +import { drainSkipsAfter } from "../../shared/fsync-skip-tracker" +import { formatFsyncSkipWarning } from "../../shared/fsync-skip-warning-formatter" + +type ToolExecuteInput = { + tool: string + sessionID: string + callID: string +} + +type ToolBeforeOutput = { + args: Record +} + +type ToolAfterOutput = { + title: string + output: string + metadata: unknown +} + +export function createFsyncSkipWarningHook() { + const startTimesByCallId = new Map() + + const toolExecuteBefore = async ( + input: ToolExecuteInput, + _output: ToolBeforeOutput, + ): Promise => { + startTimesByCallId.set(input.callID, Date.now()) + } + + const toolExecuteAfter = async ( + input: ToolExecuteInput, + output: ToolAfterOutput, + ): Promise => { + if (typeof output.output !== "string") return + + const startTimestamp = startTimesByCallId.get(input.callID) ?? 0 + startTimesByCallId.delete(input.callID) + + const skips = drainSkipsAfter(startTimestamp) + const warning = formatFsyncSkipWarning(skips) + if (warning.length === 0) return + + output.output = `${output.output}\n\n${warning}` + } + + return { + "tool.execute.before": toolExecuteBefore, + "tool.execute.after": toolExecuteAfter, + } +} diff --git a/src/hooks/index.ts b/src/hooks/index.ts index 98473c67a..5ed94b813 100644 --- a/src/hooks/index.ts +++ b/src/hooks/index.ts @@ -65,3 +65,4 @@ export { createReadImageResizerHook } from "./read-image-resizer" export { createTodoDescriptionOverrideHook } from "./todo-description-override" export { createWebFetchRedirectGuardHook } from "./webfetch-redirect-guard" export { createLegacyPluginToastHook } from "./legacy-plugin-toast" +export { createFsyncSkipWarningHook } from "./fsync-skip-warning" diff --git a/src/plugin/hooks/create-tool-guard-hooks.ts b/src/plugin/hooks/create-tool-guard-hooks.ts index 8f6675350..7cd8ea166 100644 --- a/src/plugin/hooks/create-tool-guard-hooks.ts +++ b/src/plugin/hooks/create-tool-guard-hooks.ts @@ -18,6 +18,7 @@ import { createTodoDescriptionOverrideHook, createWebFetchRedirectGuardHook, createTeamToolGating, + createFsyncSkipWarningHook, } from "../../hooks" import { getOpenCodeVersion, @@ -42,6 +43,7 @@ export type ToolGuardHooks = { readImageResizer: ReturnType | null todoDescriptionOverride: ReturnType | null webfetchRedirectGuard: ReturnType | null + fsyncSkipWarning: ReturnType | null teamToolGating: ReturnType | null } @@ -139,6 +141,10 @@ export function createToolGuardHooks(args: { ? safeHook("team-tool-gating", () => createTeamToolGating(ctx, pluginConfig.team_mode)) : null + const fsyncSkipWarning = isHookEnabled("fsync-skip-warning") + ? safeHook("fsync-skip-warning", () => createFsyncSkipWarningHook()) + : null + return { commentChecker, toolOutputTruncator, @@ -154,6 +160,7 @@ export function createToolGuardHooks(args: { readImageResizer, todoDescriptionOverride, webfetchRedirectGuard, + fsyncSkipWarning, teamToolGating, } } diff --git a/src/plugin/tool-execute-after.ts b/src/plugin/tool-execute-after.ts index f230d90db..7dabc7545 100644 --- a/src/plugin/tool-execute-after.ts +++ b/src/plugin/tool-execute-after.ts @@ -153,6 +153,7 @@ export function createToolExecuteAfterHandler(args: { await hooks.readImageResizer?.["tool.execute.after"]?.(hookInput, output) await hooks.hashlineReadEnhancer?.["tool.execute.after"]?.(hookInput, output) await hooks.webfetchRedirectGuard?.["tool.execute.after"]?.(hookInput, output) + await hooks.fsyncSkipWarning?.["tool.execute.after"]?.(hookInput, output) await hooks.jsonErrorRecovery?.["tool.execute.after"]?.(hookInput, output) } diff --git a/src/plugin/tool-execute-before.ts b/src/plugin/tool-execute-before.ts index e903571fe..093c3b157 100644 --- a/src/plugin/tool-execute-before.ts +++ b/src/plugin/tool-execute-before.ts @@ -72,8 +72,9 @@ export function createToolExecuteBeforeHandler(args: { await hooks.directoryReadmeInjector?.["tool.execute.before"]?.(input, output) await hooks.rulesInjector?.["tool.execute.before"]?.(input, output) await hooks.tasksTodowriteDisabler?.["tool.execute.before"]?.(input, output) - await hooks.webfetchRedirectGuard?.["tool.execute.before"]?.(input, output) - await hooks.prometheusMdOnly?.["tool.execute.before"]?.(input, output) + await hooks.webfetchRedirectGuard?.["tool.execute.before"]?.(input, output) + await hooks.fsyncSkipWarning?.["tool.execute.before"]?.(input, output) + await hooks.prometheusMdOnly?.["tool.execute.before"]?.(input, output) await hooks.sisyphusJuniorNotepad?.["tool.execute.before"]?.(input, output) await hooks.atlasHook?.["tool.execute.before"]?.(input, output) await hooks.teamToolGating?.["tool.execute.before"]?.(input, output) diff --git a/src/shared/classify-path-environment.test.ts b/src/shared/classify-path-environment.test.ts new file mode 100644 index 000000000..0fc45c4b7 --- /dev/null +++ b/src/shared/classify-path-environment.test.ts @@ -0,0 +1,56 @@ +import { describe, expect, it } from "bun:test" + +import { + classifyPathEnvironment, + describePathClassification, +} from "./classify-path-environment" + +describe("classifyPathEnvironment", () => { + it("classifies macOS iCloud path as icloud", () => { + expect( + classifyPathEnvironment( + "/Users/x/Library/Mobile Documents/com~apple~CloudDocs/project/file.txt", + ), + ).toBe("icloud") + }) + + it("classifies OneDrive path on unix style", () => { + expect(classifyPathEnvironment("/Users/x/OneDrive/foo")).toBe("onedrive") + }) + + it("classifies OneDrive path on windows style", () => { + expect(classifyPathEnvironment("C:\\Users\\x\\OneDrive\\foo")).toBe("onedrive") + }) + + it("classifies macOS Desktop path as desktop-sync", () => { + expect(classifyPathEnvironment("/Users/x/Desktop/foo")).toBe("desktop-sync") + }) + + it("classifies /Volumes path as network-drive", () => { + expect(classifyPathEnvironment("/Volumes/NetworkShare/foo")).toBe("network-drive") + }) + + it("classifies random path as unknown", () => { + expect(classifyPathEnvironment("/tmp/foo")).toBe("unknown") + }) + + it("classifies empty string as unknown", () => { + expect(classifyPathEnvironment("")).toBe("unknown") + }) + + it("matches OneDrive case-insensitively", () => { + expect(classifyPathEnvironment("/Users/x/oNeDrIvE/foo")).toBe("onedrive") + }) +}) + +describe("describePathClassification", () => { + it("returns human-readable descriptions", () => { + expect(describePathClassification("icloud")).toBe("iCloud Drive") + expect(describePathClassification("onedrive")).toBe("OneDrive") + expect(describePathClassification("desktop-sync")).toBe("Desktop sync (macOS)") + expect(describePathClassification("network-drive")).toBe("Network drive") + expect(describePathClassification("unknown")).toBe( + "filesystem that does not support fsync", + ) + }) +}) diff --git a/src/shared/classify-path-environment.ts b/src/shared/classify-path-environment.ts new file mode 100644 index 000000000..fe2974d54 --- /dev/null +++ b/src/shared/classify-path-environment.ts @@ -0,0 +1,68 @@ +import { homedir } from "node:os" +import path from "node:path" + +export type PathClassification = + | "icloud" + | "onedrive" + | "desktop-sync" + | "network-drive" + | "unknown" + +function normalizeInputPath(absolutePath: string): string { + return absolutePath.replaceAll("\\", "/") +} + +function isUnderPath(normalizedPath: string, normalizedParentPath: string): boolean { + return normalizedPath === normalizedParentPath || normalizedPath.startsWith(`${normalizedParentPath}/`) +} + +export function classifyPathEnvironment(absolutePath: string): PathClassification { + if (absolutePath.length === 0) return "unknown" + + const normalizedPath = normalizeInputPath(absolutePath) + const lowercasePath = normalizedPath.toLowerCase() + if (lowercasePath.includes("/onedrive") || lowercasePath.includes("/onedrive/")) { + return "onedrive" + } + + if (normalizedPath.includes("/Library/Mobile Documents/")) { + return "icloud" + } + + if (isUnderPath(normalizedPath, "/Volumes")) { + return "network-drive" + } + + if ( + normalizedPath.startsWith("/Users/") + && (normalizedPath.includes("/Desktop/") || normalizedPath.endsWith("/Desktop") + || normalizedPath.includes("/Documents/") || normalizedPath.endsWith("/Documents")) + ) { + return "desktop-sync" + } + + const normalizedHome = normalizeInputPath(homedir()) + const desktopPath = normalizeInputPath(path.join(normalizedHome, "Desktop")) + const documentsPath = normalizeInputPath(path.join(normalizedHome, "Documents")) + + if (isUnderPath(normalizedPath, desktopPath) || isUnderPath(normalizedPath, documentsPath)) { + return "desktop-sync" + } + + return "unknown" +} + +export function describePathClassification(pathClassification: PathClassification): string { + switch (pathClassification) { + case "icloud": + return "iCloud Drive" + case "onedrive": + return "OneDrive" + case "desktop-sync": + return "Desktop sync (macOS)" + case "network-drive": + return "Network drive" + case "unknown": + return "filesystem that does not support fsync" + } +} diff --git a/src/shared/fsync-skip-tracker.test.ts b/src/shared/fsync-skip-tracker.test.ts new file mode 100644 index 000000000..897e5efe8 --- /dev/null +++ b/src/shared/fsync-skip-tracker.test.ts @@ -0,0 +1,100 @@ +import { beforeEach, describe, expect, it } from "bun:test" + +import { + clearAllSkips, + drainSkipsAfter, + recordFsyncSkip, +} from "./fsync-skip-tracker" + +type PathClassification = + | "icloud" + | "onedrive" + | "desktop-sync" + | "network-drive" + | "unknown" + +function recordSkip(index: number, pathClassification: PathClassification = "unknown"): void { + recordFsyncSkip({ + filePath: `/tmp/file-${index}.txt`, + contextLabel: `atomicWrite:/tmp/file-${index}.txt`, + errorCode: "EPERM", + message: "operation not permitted", + pathClassification, + }) +} + +describe("fsync-skip-tracker", () => { + beforeEach(() => { + clearAllSkips() + }) + + it("recordFsyncSkip adds entry with timestamp", () => { + const before = Date.now() + recordSkip(1) + const entries = drainSkipsAfter(0) + + expect(entries).toHaveLength(1) + expect(entries[0]?.filePath).toBe("/tmp/file-1.txt") + expect(entries[0]?.timestamp).toBeGreaterThanOrEqual(before) + }) + + it("drainSkipsAfter(timestamp) returns entries strictly after the timestamp", async () => { + recordSkip(1) + const firstTimestamp = Date.now() + + await Bun.sleep(2) + + recordSkip(2) + const drained = drainSkipsAfter(firstTimestamp) + expect(drained).toHaveLength(1) + expect(drained[0]?.filePath).toBe("/tmp/file-2.txt") + }) + + it("drainSkipsAfter removes drained entries from buffer", () => { + recordSkip(1) + recordSkip(2) + + const drained = drainSkipsAfter(0) + expect(drained).toHaveLength(2) + expect(drainSkipsAfter(0)).toEqual([]) + }) + + it("buffer is bounded to max 200 entries and drops oldest on overflow", () => { + for (let index = 1; index <= 205; index += 1) { + recordSkip(index) + } + + const drained = drainSkipsAfter(0) + expect(drained).toHaveLength(200) + expect(drained[0]?.filePath).toBe("/tmp/file-6.txt") + expect(drained[199]?.filePath).toBe("/tmp/file-205.txt") + }) + + it("multiple records with same path are kept", () => { + recordSkip(1) + recordFsyncSkip({ + filePath: "/tmp/file-1.txt", + contextLabel: "acquireLock:/tmp/file-1.txt", + errorCode: "EPERM", + message: "second", + pathClassification: "unknown", + }) + + const drained = drainSkipsAfter(0) + expect(drained).toHaveLength(2) + expect(drained[0]?.filePath).toBe("/tmp/file-1.txt") + expect(drained[1]?.filePath).toBe("/tmp/file-1.txt") + }) + + it("drainSkipsAfter(0) returns all entries", () => { + recordSkip(1) + recordSkip(2) + + const drained = drainSkipsAfter(0) + expect(drained).toHaveLength(2) + }) + + it("empty buffer returns empty array", () => { + expect(drainSkipsAfter(0)).toEqual([]) + }) +}) diff --git a/src/shared/fsync-skip-tracker.ts b/src/shared/fsync-skip-tracker.ts new file mode 100644 index 000000000..3ee7a7125 --- /dev/null +++ b/src/shared/fsync-skip-tracker.ts @@ -0,0 +1,42 @@ +import type { PathClassification } from "./classify-path-environment" + +export type FsyncSkipEntry = { + filePath: string + contextLabel: string + errorCode: string + message: string + pathClassification: PathClassification + timestamp: number +} + +const MAX_SKIPS = 200 +const fsyncSkips: FsyncSkipEntry[] = [] + +export function recordFsyncSkip(entry: Omit): void { + fsyncSkips.push({ ...entry, timestamp: Date.now() }) + + if (fsyncSkips.length > MAX_SKIPS) { + fsyncSkips.splice(0, fsyncSkips.length - MAX_SKIPS) + } +} + +export function drainSkipsAfter(timestampMs: number): FsyncSkipEntry[] { + const drainedEntries: FsyncSkipEntry[] = [] + const retainedEntries: FsyncSkipEntry[] = [] + + for (const entry of fsyncSkips) { + if (entry.timestamp > timestampMs) { + drainedEntries.push(entry) + continue + } + + retainedEntries.push(entry) + } + + fsyncSkips.splice(0, fsyncSkips.length, ...retainedEntries) + return drainedEntries +} + +export function clearAllSkips(): void { + fsyncSkips.length = 0 +} diff --git a/src/shared/fsync-skip-warning-formatter.test.ts b/src/shared/fsync-skip-warning-formatter.test.ts new file mode 100644 index 000000000..57b626e02 --- /dev/null +++ b/src/shared/fsync-skip-warning-formatter.test.ts @@ -0,0 +1,78 @@ +import { describe, expect, it } from "bun:test" + +import type { FsyncSkipEntry } from "./fsync-skip-tracker" +import { formatFsyncSkipWarning } from "./fsync-skip-warning-formatter" + +function makeEntry(index: number, classification: FsyncSkipEntry["pathClassification"]): FsyncSkipEntry { + return { + filePath: `/path/${index}`, + contextLabel: `atomicWrite:/path/${index}`, + errorCode: "EPERM", + message: "operation not permitted", + pathClassification: classification, + timestamp: 1000 + index, + } +} + +describe("formatFsyncSkipWarning", () => { + it("returns empty string for zero entries", () => { + expect(formatFsyncSkipWarning([])).toBe("") + }) + + it("includes iCloud environment, path, and code for one entry", () => { + const warning = formatFsyncSkipWarning([makeEntry(1, "icloud")]) + expect(warning).toContain("iCloud Drive") + expect(warning).toContain("/path/1") + expect(warning).toContain("EPERM") + }) + + it("shows all five paths when exactly five entries exist", () => { + const warning = formatFsyncSkipWarning([ + makeEntry(1, "icloud"), + makeEntry(2, "icloud"), + makeEntry(3, "icloud"), + makeEntry(4, "icloud"), + makeEntry(5, "icloud"), + ]) + + expect(warning).toContain("/path/1") + expect(warning).toContain("/path/5") + expect(warning).not.toContain("and 1 more") + }) + + it("shows five paths plus overflow summary when six entries exist", () => { + const warning = formatFsyncSkipWarning([ + makeEntry(1, "icloud"), + makeEntry(2, "icloud"), + makeEntry(3, "icloud"), + makeEntry(4, "icloud"), + makeEntry(5, "icloud"), + makeEntry(6, "icloud"), + ]) + + expect(warning).toContain("/path/5") + expect(warning).not.toContain("/path/6") + expect(warning).toContain("... and 1 more") + }) + + it("uses the most common classification when entries are mixed", () => { + const warning = formatFsyncSkipWarning([ + makeEntry(1, "onedrive"), + makeEntry(2, "onedrive"), + makeEntry(3, "icloud"), + ]) + + expect(warning).toContain("Detected environment: OneDrive") + }) + + it("matches required section format", () => { + const warning = formatFsyncSkipWarning([makeEntry(1, "unknown")]) + + expect(warning).toContain("[fsync-skipped] 1 write(s) bypassed fsync") + expect(warning).toContain("Affected paths:") + expect(warning).toContain("What this means:") + expect(warning).toContain("The write+rename succeeded") + expect(warning).not.toContain("Detected environment:") + expect(warning).toContain("filesystem does not support fsync") + }) +}) diff --git a/src/shared/fsync-skip-warning-formatter.ts b/src/shared/fsync-skip-warning-formatter.ts new file mode 100644 index 000000000..91bd869d4 --- /dev/null +++ b/src/shared/fsync-skip-warning-formatter.ts @@ -0,0 +1,61 @@ +import { describePathClassification } from "./classify-path-environment" +import type { FsyncSkipEntry } from "./fsync-skip-tracker" + +const MAX_PATH_LINES = 5 + +function selectMostCommonClassification( + entries: FsyncSkipEntry[], +): FsyncSkipEntry["pathClassification"] { + const counts = new Map() + + for (const entry of entries) { + const currentCount = counts.get(entry.pathClassification) ?? 0 + counts.set(entry.pathClassification, currentCount + 1) + } + + let selected: FsyncSkipEntry["pathClassification"] = "unknown" + let selectedCount = -1 + for (const [classification, count] of counts.entries()) { + if (count > selectedCount) { + selected = classification + selectedCount = count + } + } + + return selected +} + +export function formatFsyncSkipWarning(entries: FsyncSkipEntry[]): string { + if (entries.length === 0) return "" + + const selectedClassification = selectMostCommonClassification(entries) + const selectedDescription = describePathClassification(selectedClassification) + const shownEntries = entries.slice(0, MAX_PATH_LINES) + const hiddenCount = Math.max(entries.length - shownEntries.length, 0) + const pathLines = shownEntries.map((entry) => ` - ${entry.filePath} (code: ${entry.errorCode})`) + if (hiddenCount > 0) { + pathLines.push(` ... and ${hiddenCount} more`) + } + + const environmentLines = selectedClassification === "unknown" + ? [] + : [`Detected environment: ${selectedDescription}`] + + const durabilityLine = selectedClassification === "unknown" + ? " - Crash durability is best-effort because this filesystem does not support fsync." + : " - Crash durability is best-effort on this filesystem (this is normal for iCloud, OneDrive, network drives, antivirus-locked paths)." + + return [ + "---", + `[fsync-skipped] ${entries.length} write(s) bypassed fsync because the underlying filesystem rejected the syscall.`, + "", + ...environmentLines, + "Affected paths:", + ...pathLines, + "", + "What this means:", + " - The write+rename succeeded — the file is on disk, atomicity is preserved.", + durabilityLine, + " - No action required. Operation completed successfully.", + ].join("\n") +} diff --git a/src/shared/tolerant-fsync.test.ts b/src/shared/tolerant-fsync.test.ts new file mode 100644 index 000000000..0c785ec2e --- /dev/null +++ b/src/shared/tolerant-fsync.test.ts @@ -0,0 +1,158 @@ +import { beforeEach, describe, expect, it } from "bun:test" +import { fsyncSync } from "node:fs" +import type { FileHandle } from "node:fs/promises" + +import { clearAllSkips, drainSkipsAfter } from "./fsync-skip-tracker" +import { isToleratedFsyncError, tolerantFsync, tolerantFsyncSync } from "./tolerant-fsync" + +function makeFsError(code: string, message?: string): NodeJS.ErrnoException { + const error = new Error(message ?? `${code}: simulated`) as NodeJS.ErrnoException + error.code = code + return error +} + +function fakeHandleWithSyncError(error: NodeJS.ErrnoException): FileHandle { + return { + sync: async () => { + throw error + }, + } as FileHandle +} + +describe("isToleratedFsyncError", () => { + it("#given EPERM error #when checked #then returns true", () => { + expect(isToleratedFsyncError(makeFsError("EPERM"))).toBe(true) + }) + + it("#given EACCES error #when checked #then returns true", () => { + expect(isToleratedFsyncError(makeFsError("EACCES"))).toBe(true) + }) + + it("#given ENOTSUP error #when checked #then returns true", () => { + expect(isToleratedFsyncError(makeFsError("ENOTSUP"))).toBe(true) + }) + + it("#given EINVAL error #when checked #then returns true", () => { + expect(isToleratedFsyncError(makeFsError("EINVAL"))).toBe(true) + }) + + it("#given EIO error #when checked #then returns false", () => { + expect(isToleratedFsyncError(makeFsError("EIO"))).toBe(false) + }) + + it("#given ENOSPC error (disk full) #when checked #then returns false", () => { + expect(isToleratedFsyncError(makeFsError("ENOSPC"))).toBe(false) + }) + + it("#given EBADF error (bad fd) #when checked #then returns false", () => { + expect(isToleratedFsyncError(makeFsError("EBADF"))).toBe(false) + }) + + it("#given non-Error value #when checked #then returns false", () => { + expect(isToleratedFsyncError("EPERM string")).toBe(false) + expect(isToleratedFsyncError(null)).toBe(false) + expect(isToleratedFsyncError(undefined)).toBe(false) + expect(isToleratedFsyncError({ code: "EPERM" })).toBe(false) + }) + + it("#given Error without code #when checked #then returns false", () => { + expect(isToleratedFsyncError(new Error("no code"))).toBe(false) + }) +}) + +describe("tolerantFsync (async)", () => { + beforeEach(() => { + clearAllSkips() + }) + + it("#given fsync throws EPERM #when called #then resolves without throwing", async () => { + const handle = fakeHandleWithSyncError(makeFsError("EPERM", "operation not permitted, fsync")) + await expect(tolerantFsync(handle, "test:async-eperm")).resolves.toBeUndefined() + }) + + it("#given fsync throws EACCES #when called #then resolves without throwing", async () => { + const handle = fakeHandleWithSyncError(makeFsError("EACCES")) + await expect(tolerantFsync(handle, "test:async-eacces")).resolves.toBeUndefined() + }) + + it("#given fsync throws ENOTSUP #when called #then resolves without throwing", async () => { + const handle = fakeHandleWithSyncError(makeFsError("ENOTSUP")) + await expect(tolerantFsync(handle, "test:async-enotsup")).resolves.toBeUndefined() + }) + + it("#given fsync throws EINVAL #when called #then resolves without throwing", async () => { + const handle = fakeHandleWithSyncError(makeFsError("EINVAL")) + await expect(tolerantFsync(handle, "test:async-einval")).resolves.toBeUndefined() + }) + + it("#given fsync throws EIO #when called #then propagates the error", async () => { + const handle = fakeHandleWithSyncError(makeFsError("EIO")) + await expect(tolerantFsync(handle, "test:async-eio")).rejects.toThrow("EIO: simulated") + }) + + it("#given fsync throws ENOSPC #when called #then propagates the error", async () => { + const handle = fakeHandleWithSyncError(makeFsError("ENOSPC")) + await expect(tolerantFsync(handle, "test:async-enospc")).rejects.toThrow("ENOSPC: simulated") + }) + + it("#given fsync succeeds #when called #then resolves and sync was invoked", async () => { + let syncCalled = false + const handle = { + sync: async () => { + syncCalled = true + }, + } as FileHandle + await tolerantFsync(handle, "test:async-success") + expect(syncCalled).toBe(true) + }) + + it("#given fsync throws EPERM #when called #then tracker records one skip", async () => { + const handle = fakeHandleWithSyncError(makeFsError("EPERM", "operation not permitted, fsync")) + + await tolerantFsync(handle, "atomicWrite:/Users/x/Library/Mobile Documents/com~apple~CloudDocs/file.txt") + + const entries = drainSkipsAfter(0) + expect(entries).toHaveLength(1) + expect(entries[0]?.errorCode).toBe("EPERM") + }) + + it("#given fsync throws EIO #when called #then tracker remains empty", async () => { + const handle = fakeHandleWithSyncError(makeFsError("EIO")) + + await expect(tolerantFsync(handle, "atomicWrite:/tmp/file.txt")).rejects.toThrow("EIO: simulated") + + expect(drainSkipsAfter(0)).toHaveLength(0) + }) +}) + +describe("tolerantFsyncSync (synchronous)", () => { + it("#given fsyncSync throws EPERM #when called #then returns without throwing", () => { + const fakeFsync = ((_fileDescriptor: number): void => { + throw makeFsError("EPERM", "operation not permitted, fsync") + }) as typeof fsyncSync + expect(() => tolerantFsyncSync(123, "test:sync-eperm", fakeFsync)).not.toThrow() + }) + + it("#given fsyncSync throws EACCES #when called #then returns without throwing", () => { + const fakeFsync = ((_fileDescriptor: number): void => { + throw makeFsError("EACCES") + }) as typeof fsyncSync + expect(() => tolerantFsyncSync(123, "test:sync-eacces", fakeFsync)).not.toThrow() + }) + + it("#given fsyncSync throws EIO #when called #then propagates the error", () => { + const fakeFsync = ((_fileDescriptor: number): void => { + throw makeFsError("EIO") + }) as typeof fsyncSync + expect(() => tolerantFsyncSync(123, "test:sync-eio", fakeFsync)).toThrow("EIO: simulated") + }) + + it("#given fsyncSync succeeds #when called #then returns and impl was invoked", () => { + let called = false + const fakeFsync = ((_fileDescriptor: number): void => { + called = true + }) as typeof fsyncSync + tolerantFsyncSync(123, "test:sync-success", fakeFsync) + expect(called).toBe(true) + }) +}) diff --git a/src/shared/tolerant-fsync.ts b/src/shared/tolerant-fsync.ts new file mode 100644 index 000000000..e47b791b5 --- /dev/null +++ b/src/shared/tolerant-fsync.ts @@ -0,0 +1,85 @@ +import { fsyncSync } from "node:fs" +import type { FileHandle } from "node:fs/promises" + +import { classifyPathEnvironment } from "./classify-path-environment" +import { recordFsyncSkip } from "./fsync-skip-tracker" +import { log } from "./logger" + +const TOLERATED_FSYNC_CODES: ReadonlySet = new Set([ + "EPERM", + "EACCES", + "ENOTSUP", + "EINVAL", +]) + +export function isToleratedFsyncError(error: unknown): boolean { + if (!(error instanceof Error)) return false + const code = (error as NodeJS.ErrnoException).code + return code !== undefined && TOLERATED_FSYNC_CODES.has(code) +} + +function extractPathFromContextLabel(contextLabel: string): string { + const separatorIndex = contextLabel.indexOf(":") + if (separatorIndex < 0) return contextLabel + + return contextLabel.slice(separatorIndex + 1) +} + +export async function tolerantFsync( + fileHandle: FileHandle, + contextLabel: string, +): Promise { + try { + await fileHandle.sync() + } catch (error) { + if (!isToleratedFsyncError(error)) throw error + const errorCode = (error as NodeJS.ErrnoException).code ?? "UNKNOWN" + const message = error instanceof Error ? error.message : String(error) + const filePath = extractPathFromContextLabel(contextLabel) + + log("fsync skipped due to filesystem limitation", { + event: "fsync-skipped", + contextLabel, + code: errorCode, + message, + }) + + recordFsyncSkip({ + filePath, + contextLabel, + errorCode, + message, + pathClassification: classifyPathEnvironment(filePath), + }) + } +} + +export function tolerantFsyncSync( + fileDescriptor: number, + contextLabel: string, + fsyncImpl: typeof fsyncSync = fsyncSync, +): void { + try { + fsyncImpl(fileDescriptor) + } catch (error) { + if (!isToleratedFsyncError(error)) throw error + const errorCode = (error as NodeJS.ErrnoException).code ?? "UNKNOWN" + const message = error instanceof Error ? error.message : String(error) + const filePath = extractPathFromContextLabel(contextLabel) + + log("fsync skipped due to filesystem limitation", { + event: "fsync-skipped", + contextLabel, + code: errorCode, + message, + }) + + recordFsyncSkip({ + filePath, + contextLabel, + errorCode, + message, + pathClassification: classifyPathEnvironment(filePath), + }) + } +} diff --git a/src/shared/write-file-atomically.test.ts b/src/shared/write-file-atomically.test.ts index ce4a5c8f9..2c13cd2ff 100644 --- a/src/shared/write-file-atomically.test.ts +++ b/src/shared/write-file-atomically.test.ts @@ -51,4 +51,39 @@ describe("writeFileAtomically", () => { // when/then expect(() => writeFileAtomically(filePath, "content")).toThrow() }) + + it("#given fsync fails with EPERM (synced folder) #when writeFileAtomically called #then write succeeds", () => { + // given + const filePath = join(testDir, "synced-folder.txt") + const content = "content from a synced folder where fsync is rejected" + + // when + writeFileAtomically(filePath, content, { + fsyncSync: () => { + const error = new Error("EPERM: operation not permitted, fsync") as NodeJS.ErrnoException + error.code = "EPERM" + throw error + }, + }) + + // then + expect(existsSync(filePath)).toBe(true) + expect(readFileSync(filePath, "utf-8")).toBe(content) + }) + + it("#given fsync fails with EIO (real I/O error) #when writeFileAtomically called #then propagates the error", () => { + // given + const filePath = join(testDir, "io-error.txt") + + // when/then + expect(() => + writeFileAtomically(filePath, "content", { + fsyncSync: () => { + const error = new Error("EIO: input/output error") as NodeJS.ErrnoException + error.code = "EIO" + throw error + }, + }), + ).toThrow("EIO") + }) }) diff --git a/src/shared/write-file-atomically.ts b/src/shared/write-file-atomically.ts index 9e9f123bc..09ce5b7d5 100644 --- a/src/shared/write-file-atomically.ts +++ b/src/shared/write-file-atomically.ts @@ -1,11 +1,24 @@ -import { closeSync, fsyncSync, openSync, renameSync, unlinkSync, writeFileSync } from "node:fs" +import { + closeSync, + type fsyncSync as FsyncSync, + openSync, + renameSync, + unlinkSync, + writeFileSync, +} from "node:fs" -export function writeFileAtomically(filePath: string, content: string): void { - const tempPath = `${filePath}.tmp` - writeFileSync(tempPath, content, "utf-8") +import { tolerantFsyncSync } from "./tolerant-fsync" + +export function writeFileAtomically( + filePath: string, + content: string, + deps: { fsyncSync?: typeof FsyncSync } = {}, +): void { + const tempPath = `${filePath}.tmp` + writeFileSync(tempPath, content, "utf-8") const tempFileDescriptor = openSync(tempPath, "r") try { - fsyncSync(tempFileDescriptor) + tolerantFsyncSync(tempFileDescriptor, `writeFileAtomically:${filePath}`, deps.fsyncSync) } finally { closeSync(tempFileDescriptor) }