From f8330c266087bdff7433de457d2983b4a7e715f0 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Sun, 17 May 2026 17:08:26 +0000 Subject: [PATCH 1/2] feat(telemetry): add JSON export writer --- src/telemetry/export/writers.ts | 112 +++++++++++++++++++++ test/unit/telemetry/export/writers.test.ts | 91 +++++++++++++++++ 2 files changed, 203 insertions(+) create mode 100644 src/telemetry/export/writers.ts create mode 100644 test/unit/telemetry/export/writers.test.ts diff --git a/src/telemetry/export/writers.ts b/src/telemetry/export/writers.ts new file mode 100644 index 000000000..a049e92bb --- /dev/null +++ b/src/telemetry/export/writers.ts @@ -0,0 +1,112 @@ +import * as fs from "node:fs/promises"; + +import { renameWithRetry, tempFilePath } from "../../util"; + +import { toStoredTelemetryEvent } from "./files"; + +import type { TelemetryEvent } from "../event"; + +export interface ExportCounts { + readonly events: number; + readonly logs: number; + readonly traces: number; + readonly metrics: number; +} + +class JsonEnvelopeWriter { + readonly #filePath: string; + readonly #suffix: string; + #handle: fs.FileHandle | undefined; + #count = 0; + + private constructor(filePath: string, suffix: string) { + this.#filePath = filePath; + this.#suffix = suffix; + } + + public static async open( + filePath: string, + prefix: string, + suffix: string, + ): Promise { + const writer = new JsonEnvelopeWriter(filePath, suffix); + writer.#handle = await fs.open(filePath, "w"); + try { + await writer.#write(prefix); + return writer; + } catch (err) { + await writer.close(); + throw err; + } + } + + public get count(): number { + return this.#count; + } + + public async write(value: unknown): Promise { + if (this.#count > 0) { + await this.#write(","); + } + await this.#write(JSON.stringify(value)); + this.#count += 1; + } + + public async close(): Promise { + if (!this.#handle) { + return; + } + try { + await this.#write(this.#suffix); + } finally { + await this.#handle.close(); + this.#handle = undefined; + } + } + + async #write(chunk: string): Promise { + if (!this.#handle) { + throw new Error(`JSON writer for ${this.#filePath} is closed.`); + } + await this.#handle.writeFile(chunk, "utf8"); + } +} + +export async function writeJsonArrayExport( + outputPath: string, + events: AsyncIterable, +): Promise { + return writeTempOutput(outputPath, async (tempPath) => { + const writer = await JsonEnvelopeWriter.open(tempPath, "[\n", "\n]\n"); + try { + for await (const event of events) { + await writer.write(toStoredTelemetryEvent(event)); + } + } finally { + await writer.close(); + } + return { + events: writer.count, + logs: 0, + traces: 0, + metrics: 0, + }; + }); +} + +async function writeTempOutput( + outputPath: string, + write: (tempPath: string) => Promise, +): Promise { + const tempPath = tempFilePath(outputPath, "tmp"); + try { + const result = await write(tempPath); + await renameWithRetry(fs.rename, tempPath, outputPath); + return result; + } catch (err) { + await fs.rm(tempPath, { force: true }).catch(() => { + // Keep the export failure as the error callers see. + }); + throw err; + } +} diff --git a/test/unit/telemetry/export/writers.test.ts b/test/unit/telemetry/export/writers.test.ts new file mode 100644 index 000000000..142c17a94 --- /dev/null +++ b/test/unit/telemetry/export/writers.test.ts @@ -0,0 +1,91 @@ +import * as fs from "node:fs/promises"; +import * as os from "node:os"; +import * as path from "node:path"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; + +import { toStoredTelemetryEvent } from "@/telemetry/export/files"; +import { writeJsonArrayExport } from "@/telemetry/export/writers"; + +import type { TelemetryEvent } from "@/telemetry/event"; + +let tmpDir: string; + +beforeEach(async () => { + tmpDir = await fs.mkdtemp( + path.join(os.tmpdir(), "telemetry-export-writers-"), + ); +}); + +afterEach(async () => { + await fs.rm(tmpDir, { recursive: true, force: true }); +}); + +describe("telemetry export writers", () => { + it("writes telemetry events as a JSON array using the stored event shape", async () => { + const outputPath = path.join(tmpDir, "telemetry.json"); + + const events = [ + makeEvent({ + eventId: "1111111111111111", + eventName: "first", + properties: { result: "success" }, + measurements: { durationMs: 12 }, + traceId: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + }), + makeEvent({ + eventId: "2222222222222222", + eventName: "second", + parentEventId: "1111111111111111", + error: { message: "boom", type: "Error" }, + }), + ]; + + const counts = await writeJsonArrayExport(outputPath, asyncEvents(events)); + + expect(counts.events).toBe(2); + expect(JSON.parse(await fs.readFile(outputPath, "utf8"))).toEqual( + events.map(toStoredTelemetryEvent), + ); + }); + + it("writes a valid empty JSON array", async () => { + const outputPath = path.join(tmpDir, "empty.json"); + + const counts = await writeJsonArrayExport(outputPath, asyncEvents([])); + + expect(counts.events).toBe(0); + expect(JSON.parse(await fs.readFile(outputPath, "utf8"))).toEqual([]); + }); +}); + +async function* asyncEvents( + events: readonly TelemetryEvent[], +): AsyncGenerator { + for (const event of events) { + await Promise.resolve(); + yield event; + } +} + +function makeEvent(overrides: Partial): TelemetryEvent { + return { + eventId: "1111111111111111", + eventName: "test.event", + timestamp: "2026-05-12T12:00:00.000Z", + eventSequence: 1, + context: { + extensionVersion: "1.2.3", + machineId: "machine", + sessionId: "session", + osType: "linux", + osVersion: "6.0.0", + hostArch: "x64", + platformName: "VS Code", + platformVersion: "1.100.0", + deploymentUrl: "https://coder.example.com", + }, + properties: {}, + measurements: {}, + ...overrides, + }; +} From c9c3b7455164260c8144f7ee7ec12791ef247029 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Tue, 19 May 2026 13:19:05 +0300 Subject: [PATCH 2/2] refactor(util): extract fs helpers and stream JSON export Move `tempFilePath` and `renameWithRetry` from `src/util.ts` into a focused `src/util/fs.ts`, alongside a new `writeAtomically(path, write)` helper that captures the temp-file + rename + best-effort cleanup pattern previously copy-pasted into the telemetry writer and `CliCredentialManager.atomicWriteFile`. Both call sites now use the shared helper; the SSH config / support bundle / CLI manager keep their bespoke flows and just update imports. Rewrite `writeJsonArrayExport` to stream chunks through `Readable.from(...)` into `createWriteStream` via `stream/promises` `pipeline`. Memory stays flat regardless of `maxTotalBytes`, which has no enforced ceiling and can comfortably exceed the default 100 MB cap. Tests for `tempFilePath` and `renameWithRetry` move into `test/unit/util/fs.test.ts` alongside a new `writeAtomically` suite covering success, partial-write rollback, and callback return values. --- src/core/cliCredentialManager.ts | 20 +-- src/core/cliManager.ts | 3 +- src/core/supportBundleLogs.ts | 2 +- src/remote/sshConfig.ts | 3 +- src/telemetry/export/writers.ts | 129 ++++------------- src/util.ts | 54 ------- src/util/fs.ts | 77 ++++++++++ test/unit/core/supportBundleLogs.test.ts | 6 +- test/unit/telemetry/export/writers.test.ts | 111 +++++++------- test/unit/util.test.ts | 103 ------------- test/unit/util/fs.test.ts | 161 +++++++++++++++++++++ 11 files changed, 341 insertions(+), 328 deletions(-) create mode 100644 src/util/fs.ts create mode 100644 test/unit/util/fs.test.ts diff --git a/src/core/cliCredentialManager.ts b/src/core/cliCredentialManager.ts index 6eaef21ab..5572c9c92 100644 --- a/src/core/cliCredentialManager.ts +++ b/src/core/cliCredentialManager.ts @@ -9,7 +9,8 @@ import { isAbortError } from "../error/errorUtils"; import { featureSetForVersion } from "../featureSet"; import { isKeyringEnabled } from "../settings/cli"; import { getHeaderArgs } from "../settings/headers"; -import { renameWithRetry, tempFilePath, toSafeHost } from "../util"; +import { toSafeHost } from "../util"; +import { writeAtomically } from "../util/fs"; import { version } from "./cliExec"; @@ -259,23 +260,14 @@ export class CliCredentialManager { } } - /** - * Atomically write content to a file via temp-file + rename. - */ + /** Atomically write content to a file. */ private async atomicWriteFile( filePath: string, content: string, ): Promise { await fs.mkdir(path.dirname(filePath), { recursive: true }); - const tempPath = tempFilePath(filePath, "temp"); - try { - await fs.writeFile(tempPath, content, { mode: 0o600 }); - await renameWithRetry(fs.rename, tempPath, filePath); - } catch (err) { - await fs.rm(tempPath, { force: true }).catch((rmErr) => { - this.logger.warn("Failed to delete temp file", tempPath, rmErr); - }); - throw err; - } + await writeAtomically(filePath, (tempPath) => + fs.writeFile(tempPath, content, { mode: 0o600 }), + ); } } diff --git a/src/core/cliManager.ts b/src/core/cliManager.ts index 25e585f90..b65f0f0a6 100644 --- a/src/core/cliManager.ts +++ b/src/core/cliManager.ts @@ -13,7 +13,8 @@ import { errToStr } from "../api/api-helper"; import * as pgp from "../pgp"; import { withCancellableProgress, withOptionalProgress } from "../progress"; import { isKeyringEnabled } from "../settings/cli"; -import { tempFilePath, toSafeHost } from "../util"; +import { toSafeHost } from "../util"; +import { tempFilePath } from "../util/fs"; import { vscodeProposed } from "../vscodeProposed"; import { BinaryLock } from "./binaryLock"; diff --git a/src/core/supportBundleLogs.ts b/src/core/supportBundleLogs.ts index ad30a0f92..e21551994 100644 --- a/src/core/supportBundleLogs.ts +++ b/src/core/supportBundleLogs.ts @@ -4,7 +4,7 @@ import * as path from "node:path"; import { promisify } from "node:util"; import { type Logger } from "../logging/logger"; -import { renameWithRetry } from "../util"; +import { renameWithRetry } from "../util/fs"; export interface LogSources { remoteSshLogPath?: string; diff --git a/src/remote/sshConfig.ts b/src/remote/sshConfig.ts index 4eb11027c..9e86cb28f 100644 --- a/src/remote/sshConfig.ts +++ b/src/remote/sshConfig.ts @@ -8,7 +8,8 @@ import { } from "node:fs/promises"; import path from "node:path"; -import { countSubstring, renameWithRetry, tempFilePath } from "../util"; +import { countSubstring } from "../util"; +import { renameWithRetry, tempFilePath } from "../util/fs"; import type { Logger } from "../logging/logger"; diff --git a/src/telemetry/export/writers.ts b/src/telemetry/export/writers.ts index a049e92bb..4c6e97af8 100644 --- a/src/telemetry/export/writers.ts +++ b/src/telemetry/export/writers.ts @@ -1,112 +1,37 @@ -import * as fs from "node:fs/promises"; +import { createWriteStream } from "node:fs"; +import { Readable } from "node:stream"; +import { pipeline } from "node:stream/promises"; -import { renameWithRetry, tempFilePath } from "../../util"; - -import { toStoredTelemetryEvent } from "./files"; +import { writeAtomically } from "../../util/fs"; +import { serializeTelemetryEvent } from "../wireFormat"; import type { TelemetryEvent } from "../event"; -export interface ExportCounts { - readonly events: number; - readonly logs: number; - readonly traces: number; - readonly metrics: number; -} - -class JsonEnvelopeWriter { - readonly #filePath: string; - readonly #suffix: string; - #handle: fs.FileHandle | undefined; - #count = 0; - - private constructor(filePath: string, suffix: string) { - this.#filePath = filePath; - this.#suffix = suffix; - } - - public static async open( - filePath: string, - prefix: string, - suffix: string, - ): Promise { - const writer = new JsonEnvelopeWriter(filePath, suffix); - writer.#handle = await fs.open(filePath, "w"); - try { - await writer.#write(prefix); - return writer; - } catch (err) { - await writer.close(); - throw err; - } - } - - public get count(): number { - return this.#count; - } - - public async write(value: unknown): Promise { - if (this.#count > 0) { - await this.#write(","); - } - await this.#write(JSON.stringify(value)); - this.#count += 1; - } - - public async close(): Promise { - if (!this.#handle) { - return; - } - try { - await this.#write(this.#suffix); - } finally { - await this.#handle.close(); - this.#handle = undefined; - } - } - - async #write(chunk: string): Promise { - if (!this.#handle) { - throw new Error(`JSON writer for ${this.#filePath} is closed.`); - } - await this.#handle.writeFile(chunk, "utf8"); - } -} - +/** + * Writes `events` as a JSON array to `outputPath` via a temp file + atomic + * rename, so a partial write never replaces the destination. Streams chunks + * with backpressure so memory stays flat even for large exports. + * Returns the number of events written. + */ export async function writeJsonArrayExport( outputPath: string, events: AsyncIterable, -): Promise { - return writeTempOutput(outputPath, async (tempPath) => { - const writer = await JsonEnvelopeWriter.open(tempPath, "[\n", "\n]\n"); - try { - for await (const event of events) { - await writer.write(toStoredTelemetryEvent(event)); - } - } finally { - await writer.close(); +): Promise { + let count = 0; + async function* chunks(): AsyncGenerator { + yield "["; + for await (const event of events) { + yield (count === 0 ? "\n" : ",\n") + + JSON.stringify(serializeTelemetryEvent(event)); + count += 1; } - return { - events: writer.count, - logs: 0, - traces: 0, - metrics: 0, - }; - }); -} - -async function writeTempOutput( - outputPath: string, - write: (tempPath: string) => Promise, -): Promise { - const tempPath = tempFilePath(outputPath, "tmp"); - try { - const result = await write(tempPath); - await renameWithRetry(fs.rename, tempPath, outputPath); - return result; - } catch (err) { - await fs.rm(tempPath, { force: true }).catch(() => { - // Keep the export failure as the error callers see. - }); - throw err; + yield count === 0 ? "]\n" : "\n]\n"; } + await writeAtomically(outputPath, async (tempPath) => { + await pipeline( + Readable.from(chunks()), + createWriteStream(tempPath, { encoding: "utf8" }), + ); + }); + return count; } diff --git a/src/util.ts b/src/util.ts index ed9b6f648..405d4f176 100644 --- a/src/util.ts +++ b/src/util.ts @@ -164,51 +164,6 @@ export function countSubstring(needle: string, haystack: string): number { return count; } -const transientRenameCodes: ReadonlySet = new Set([ - "EPERM", - "EACCES", - "EBUSY", -]); - -/** - * Rename with retry for transient Windows filesystem errors (EPERM, EACCES, - * EBUSY). On Windows, antivirus, Search Indexer, cloud sync, or concurrent - * processes can briefly lock files causing renames to fail. - * - * On non-Windows platforms, calls renameFn directly with no retry. - * - * Matches the strategy used by VS Code (pfs.ts) and graceful-fs: 60s - * wall-clock timeout with linear backoff (10ms increments) capped at 100ms. - */ -export async function renameWithRetry( - renameFn: (src: string, dest: string) => Promise, - source: string, - destination: string, - timeoutMs = 60_000, - delayCapMs = 100, -): Promise { - if (process.platform !== "win32") { - return renameFn(source, destination); - } - const startTime = Date.now(); - for (let attempt = 1; ; attempt++) { - try { - return await renameFn(source, destination); - } catch (err) { - const code = (err as NodeJS.ErrnoException).code; - if ( - !code || - !transientRenameCodes.has(code) || - Date.now() - startTime >= timeoutMs - ) { - throw err; - } - const delay = Math.min(delayCapMs, attempt * 10); - await new Promise((resolve) => setTimeout(resolve, delay)); - } - } -} - /** * Wraps `arg` in `"..."` unless every character is in the shell-safe * whitelist (matching Python `shlex.quote`'s set: alphanumerics plus @@ -240,12 +195,3 @@ export function escapeShellArg(arg: string): string { } return `'${arg.replace(/'/g, "'\\''")}'`; } - -/** - * Generate a temporary file path by appending a suffix with a random component. - * The suffix describes the purpose of the temp file (e.g. "temp", "old"). - * Example: tempFilePath("/a/b", "temp") → "/a/b.temp-k7x3f9qw" - */ -export function tempFilePath(basePath: string, suffix: string): string { - return `${basePath}.${suffix}-${crypto.randomUUID().substring(0, 8)}`; -} diff --git a/src/util/fs.ts b/src/util/fs.ts new file mode 100644 index 000000000..a03c269e0 --- /dev/null +++ b/src/util/fs.ts @@ -0,0 +1,77 @@ +import * as fs from "node:fs/promises"; + +const transientRenameCodes: ReadonlySet = new Set([ + "EPERM", + "EACCES", + "EBUSY", +]); + +/** + * Rename with retry for transient Windows filesystem errors (EPERM, EACCES, + * EBUSY). On Windows, antivirus, Search Indexer, cloud sync, or concurrent + * processes can briefly lock files causing renames to fail. + * + * On non-Windows platforms, calls renameFn directly with no retry. + * + * Matches the strategy used by VS Code (pfs.ts) and graceful-fs: 60s + * wall-clock timeout with linear backoff (10ms increments) capped at 100ms. + */ +export async function renameWithRetry( + renameFn: (src: string, dest: string) => Promise, + source: string, + destination: string, + timeoutMs = 60_000, + delayCapMs = 100, +): Promise { + if (process.platform !== "win32") { + return renameFn(source, destination); + } + const startTime = Date.now(); + for (let attempt = 1; ; attempt++) { + try { + return await renameFn(source, destination); + } catch (err) { + const code = (err as NodeJS.ErrnoException).code; + if ( + !code || + !transientRenameCodes.has(code) || + Date.now() - startTime >= timeoutMs + ) { + throw err; + } + const delay = Math.min(delayCapMs, attempt * 10); + await new Promise((resolve) => setTimeout(resolve, delay)); + } + } +} + +/** + * Generate a temporary file path by appending a suffix with a random component. + * The suffix describes the purpose of the temp file (e.g. "temp", "old"). + * Example: tempFilePath("/a/b", "temp") → "/a/b.temp-k7x3f9qw" + */ +export function tempFilePath(basePath: string, suffix: string): string { + return `${basePath}.${suffix}-${crypto.randomUUID().substring(0, 8)}`; +} + +/** + * Writes `outputPath` atomically: `write` builds the content at a sibling + * temp path, then we rename onto the destination. A failure mid-write never + * touches the destination; the temp file is best-effort cleaned up. + */ +export async function writeAtomically( + outputPath: string, + write: (tempPath: string) => Promise, +): Promise { + const tempPath = tempFilePath(outputPath, "tmp"); + try { + const result = await write(tempPath); + await renameWithRetry(fs.rename, tempPath, outputPath); + return result; + } catch (err) { + await fs.rm(tempPath, { force: true }).catch(() => { + // Surface the original failure, not the cleanup failure. + }); + throw err; + } +} diff --git a/test/unit/core/supportBundleLogs.test.ts b/test/unit/core/supportBundleLogs.test.ts index bdd7a1647..e942e40e4 100644 --- a/test/unit/core/supportBundleLogs.test.ts +++ b/test/unit/core/supportBundleLogs.test.ts @@ -5,14 +5,14 @@ import * as path from "node:path"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { appendVsCodeLogs } from "@/core/supportBundleLogs"; -import { renameWithRetry } from "@/util"; +import { renameWithRetry } from "@/util/fs"; import { createMockLogger } from "../../mocks/testHelpers"; // Wrap renameWithRetry so individual tests can override it via // mockRejectedValueOnce; by default it calls through to the real impl. -vi.mock("@/util", async () => { - const actual = await vi.importActual("@/util"); +vi.mock("@/util/fs", async () => { + const actual = await vi.importActual("@/util/fs"); return { ...actual, renameWithRetry: vi.fn(actual.renameWithRetry) }; }); diff --git a/test/unit/telemetry/export/writers.test.ts b/test/unit/telemetry/export/writers.test.ts index 142c17a94..dd07c2df6 100644 --- a/test/unit/telemetry/export/writers.test.ts +++ b/test/unit/telemetry/export/writers.test.ts @@ -1,64 +1,96 @@ -import * as fs from "node:fs/promises"; -import * as os from "node:os"; -import * as path from "node:path"; -import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { vol } from "memfs"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; -import { toStoredTelemetryEvent } from "@/telemetry/export/files"; import { writeJsonArrayExport } from "@/telemetry/export/writers"; +import { serializeTelemetryEvent } from "@/telemetry/wireFormat"; + +import { createTelemetryEventFactory } from "../../../mocks/telemetry"; + +import type * as fs from "node:fs"; import type { TelemetryEvent } from "@/telemetry/event"; -let tmpDir: string; +vi.mock("node:fs/promises", async () => { + const memfs: { fs: typeof fs } = await vi.importActual("memfs"); + return memfs.fs.promises; +}); -beforeEach(async () => { - tmpDir = await fs.mkdtemp( - path.join(os.tmpdir(), "telemetry-export-writers-"), - ); +vi.mock("node:fs", async () => { + const memfs: { fs: typeof fs } = await vi.importActual("memfs"); + return memfs.fs; }); -afterEach(async () => { - await fs.rm(tmpDir, { recursive: true, force: true }); +const DIR = "/exports"; + +let makeEvent: ReturnType; + +beforeEach(() => { + vol.reset(); + vol.mkdirSync(DIR, { recursive: true }); + makeEvent = createTelemetryEventFactory(); }); -describe("telemetry export writers", () => { - it("writes telemetry events as a JSON array using the stored event shape", async () => { - const outputPath = path.join(tmpDir, "telemetry.json"); +afterEach(() => { + vol.reset(); +}); +describe("writeJsonArrayExport", () => { + it("writes events as a JSON array in wire format", async () => { const events = [ makeEvent({ - eventId: "1111111111111111", eventName: "first", properties: { result: "success" }, measurements: { durationMs: 12 }, traceId: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", }), makeEvent({ - eventId: "2222222222222222", eventName: "second", - parentEventId: "1111111111111111", + parentEventId: "id-0", error: { message: "boom", type: "Error" }, }), ]; - const counts = await writeJsonArrayExport(outputPath, asyncEvents(events)); + const count = await writeJsonArrayExport( + `${DIR}/telemetry.json`, + asyncIterable(events), + ); - expect(counts.events).toBe(2); - expect(JSON.parse(await fs.readFile(outputPath, "utf8"))).toEqual( - events.map(toStoredTelemetryEvent), + expect(count).toBe(2); + expect(readJson(`${DIR}/telemetry.json`)).toEqual( + events.map(serializeTelemetryEvent), ); }); - it("writes a valid empty JSON array", async () => { - const outputPath = path.join(tmpDir, "empty.json"); + it("writes a valid empty JSON array when there are no events", async () => { + const count = await writeJsonArrayExport( + `${DIR}/empty.json`, + asyncIterable([]), + ); + + expect(count).toBe(0); + expect(readJson(`${DIR}/empty.json`)).toEqual([]); + }); - const counts = await writeJsonArrayExport(outputPath, asyncEvents([])); + it("leaves the destination untouched when writing fails midway", async () => { + const outputPath = `${DIR}/telemetry.json`; + vol.writeFileSync(outputPath, "previous content"); + + const events = (async function* () { + yield makeEvent(); + await Promise.resolve(); + throw new Error("boom"); + })(); + + await expect(writeJsonArrayExport(outputPath, events)).rejects.toThrow( + /boom/, + ); - expect(counts.events).toBe(0); - expect(JSON.parse(await fs.readFile(outputPath, "utf8"))).toEqual([]); + expect(vol.readFileSync(outputPath, "utf8")).toBe("previous content"); + expect(vol.readdirSync(DIR)).toEqual(["telemetry.json"]); }); }); -async function* asyncEvents( +async function* asyncIterable( events: readonly TelemetryEvent[], ): AsyncGenerator { for (const event of events) { @@ -67,25 +99,6 @@ async function* asyncEvents( } } -function makeEvent(overrides: Partial): TelemetryEvent { - return { - eventId: "1111111111111111", - eventName: "test.event", - timestamp: "2026-05-12T12:00:00.000Z", - eventSequence: 1, - context: { - extensionVersion: "1.2.3", - machineId: "machine", - sessionId: "session", - osType: "linux", - osVersion: "6.0.0", - hostArch: "x64", - platformName: "VS Code", - platformVersion: "1.100.0", - deploymentUrl: "https://coder.example.com", - }, - properties: {}, - measurements: {}, - ...overrides, - }; +function readJson(filePath: string): unknown { + return JSON.parse(vol.readFileSync(filePath, "utf8") as string); } diff --git a/test/unit/util.test.ts b/test/unit/util.test.ts index 213883240..02212df16 100644 --- a/test/unit/util.test.ts +++ b/test/unit/util.test.ts @@ -9,8 +9,6 @@ import { expandPath, findPort, parseRemoteAuthority, - renameWithRetry, - tempFilePath, toSafeHost, } from "@/util"; @@ -399,104 +397,3 @@ describe("findPort", () => { expect(findPort(log)).toBe(3333); }); }); - -describe("tempFilePath", () => { - it("prepends basePath and suffix before the random part", () => { - const result = tempFilePath("/a/b/file", "temp"); - const prefix = "/a/b/file.temp-"; - expect(result.startsWith(prefix)).toBe(true); - // prefix(15) + uuid(8) = 23 - expect(result).toHaveLength(prefix.length + 8); - }); - - it("generates different paths on each call", () => { - const a = tempFilePath("/x", "tmp"); - const b = tempFilePath("/x", "tmp"); - expect(a).not.toBe(b); - }); - - it("uses the provided suffix", () => { - const result = tempFilePath("/base", "old"); - expect(result.startsWith("/base.old-")).toBe(true); - }); -}); - -describe("renameWithRetry", () => { - const realPlatform = process.platform; - - function makeErrno(code: string): NodeJS.ErrnoException { - const err = new Error(code); - (err as NodeJS.ErrnoException).code = code; - return err; - } - - function setPlatform(value: string) { - Object.defineProperty(process, "platform", { value }); - } - - afterEach(() => { - setPlatform(realPlatform); - vi.useRealTimers(); - }); - - it("succeeds on first attempt", async () => { - const renameFn = vi.fn<(s: string, d: string) => Promise>(); - renameFn.mockResolvedValueOnce(undefined); - await renameWithRetry(renameFn, "/a", "/b"); - expect(renameFn).toHaveBeenCalledTimes(1); - expect(renameFn).toHaveBeenCalledWith("/a", "/b"); - }); - - it("skips retry logic on non-Windows platforms", async () => { - setPlatform("linux"); - const renameFn = vi.fn<(s: string, d: string) => Promise>(); - renameFn.mockRejectedValueOnce(makeErrno("EPERM")); - - await expect(renameWithRetry(renameFn, "/a", "/b")).rejects.toThrow( - "EPERM", - ); - expect(renameFn).toHaveBeenCalledTimes(1); - }); - - describe("on Windows", () => { - beforeEach(() => setPlatform("win32")); - - it.each(["EPERM", "EACCES", "EBUSY"])( - "retries on transient %s and succeeds", - async (code) => { - const renameFn = vi.fn<(s: string, d: string) => Promise>(); - renameFn - .mockRejectedValueOnce(makeErrno(code)) - .mockResolvedValueOnce(undefined); - - await renameWithRetry(renameFn, "/a", "/b", 60_000, 10); - expect(renameFn).toHaveBeenCalledTimes(2); - }, - ); - - it("throws after timeout is exceeded", async () => { - vi.useFakeTimers(); - const renameFn = vi.fn<(s: string, d: string) => Promise>(); - const epermError = makeErrno("EPERM"); - renameFn.mockImplementation(() => Promise.reject(epermError)); - - const promise = renameWithRetry(renameFn, "/a", "/b", 5); - const assertion = expect(promise).rejects.toThrow(epermError); - await vi.advanceTimersByTimeAsync(100); - await assertion; - }); - - it.each(["EXDEV", "ENOENT", "EISDIR"])( - "does not retry non-transient %s", - async (code) => { - const renameFn = vi.fn<(s: string, d: string) => Promise>(); - renameFn.mockRejectedValueOnce(makeErrno(code)); - - await expect(renameWithRetry(renameFn, "/a", "/b")).rejects.toThrow( - code, - ); - expect(renameFn).toHaveBeenCalledTimes(1); - }, - ); - }); -}); diff --git a/test/unit/util/fs.test.ts b/test/unit/util/fs.test.ts new file mode 100644 index 000000000..c2cc3f78c --- /dev/null +++ b/test/unit/util/fs.test.ts @@ -0,0 +1,161 @@ +import { vol } from "memfs"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +import { renameWithRetry, tempFilePath, writeAtomically } from "@/util/fs"; + +import type * as fs from "node:fs"; + +vi.mock("node:fs/promises", async () => { + const memfs: { fs: typeof fs } = await vi.importActual("memfs"); + return memfs.fs.promises; +}); + +describe("tempFilePath", () => { + it("prepends basePath and suffix before the random part", () => { + const result = tempFilePath("/a/b/file", "temp"); + const prefix = "/a/b/file.temp-"; + expect(result.startsWith(prefix)).toBe(true); + // prefix + uuid(8) + expect(result).toHaveLength(prefix.length + 8); + }); + + it("generates different paths on each call", () => { + const a = tempFilePath("/x", "tmp"); + const b = tempFilePath("/x", "tmp"); + expect(a).not.toBe(b); + }); + + it("uses the provided suffix", () => { + const result = tempFilePath("/base", "old"); + expect(result.startsWith("/base.old-")).toBe(true); + }); +}); + +describe("renameWithRetry", () => { + const realPlatform = process.platform; + + function makeErrno(code: string): NodeJS.ErrnoException { + const err = new Error(code); + (err as NodeJS.ErrnoException).code = code; + return err; + } + + function setPlatform(value: string) { + Object.defineProperty(process, "platform", { value }); + } + + afterEach(() => { + setPlatform(realPlatform); + vi.useRealTimers(); + }); + + it("succeeds on first attempt", async () => { + const renameFn = vi.fn<(s: string, d: string) => Promise>(); + renameFn.mockResolvedValueOnce(undefined); + await renameWithRetry(renameFn, "/a", "/b"); + expect(renameFn).toHaveBeenCalledTimes(1); + expect(renameFn).toHaveBeenCalledWith("/a", "/b"); + }); + + it("skips retry logic on non-Windows platforms", async () => { + setPlatform("linux"); + const renameFn = vi.fn<(s: string, d: string) => Promise>(); + renameFn.mockRejectedValueOnce(makeErrno("EPERM")); + + await expect(renameWithRetry(renameFn, "/a", "/b")).rejects.toThrow( + "EPERM", + ); + expect(renameFn).toHaveBeenCalledTimes(1); + }); + + describe("on Windows", () => { + beforeEach(() => setPlatform("win32")); + + it.each(["EPERM", "EACCES", "EBUSY"])( + "retries on transient %s and succeeds", + async (code) => { + const renameFn = vi.fn<(s: string, d: string) => Promise>(); + renameFn + .mockRejectedValueOnce(makeErrno(code)) + .mockResolvedValueOnce(undefined); + + await renameWithRetry(renameFn, "/a", "/b", 60_000, 10); + expect(renameFn).toHaveBeenCalledTimes(2); + }, + ); + + it("throws after timeout is exceeded", async () => { + vi.useFakeTimers(); + const renameFn = vi.fn<(s: string, d: string) => Promise>(); + const epermError = makeErrno("EPERM"); + renameFn.mockImplementation(() => Promise.reject(epermError)); + + const promise = renameWithRetry(renameFn, "/a", "/b", 5); + const assertion = expect(promise).rejects.toThrow(epermError); + await vi.advanceTimersByTimeAsync(100); + await assertion; + }); + + it.each(["EXDEV", "ENOENT", "EISDIR"])( + "does not retry non-transient %s", + async (code) => { + const renameFn = vi.fn<(s: string, d: string) => Promise>(); + renameFn.mockRejectedValueOnce(makeErrno(code)); + + await expect(renameWithRetry(renameFn, "/a", "/b")).rejects.toThrow( + code, + ); + expect(renameFn).toHaveBeenCalledTimes(1); + }, + ); + }); +}); + +describe("writeAtomically", () => { + const DIR = "/atomic"; + + beforeEach(() => { + vol.reset(); + vol.mkdirSync(DIR, { recursive: true }); + }); + + afterEach(() => { + vol.reset(); + }); + + it("renames the temp file onto the destination on success", async () => { + const outputPath = `${DIR}/result.txt`; + await writeAtomically(outputPath, (tempPath) => { + expect(tempPath).not.toBe(outputPath); + vol.writeFileSync(tempPath, "hello"); + return Promise.resolve(); + }); + + expect(vol.readFileSync(outputPath, "utf8")).toBe("hello"); + expect(vol.readdirSync(DIR)).toEqual(["result.txt"]); + }); + + it("leaves the destination untouched and cleans up on failure", async () => { + const outputPath = `${DIR}/result.txt`; + vol.writeFileSync(outputPath, "previous"); + + await expect( + writeAtomically(outputPath, (tempPath) => { + vol.writeFileSync(tempPath, "partial"); + return Promise.reject(new Error("boom")); + }), + ).rejects.toThrow(/boom/); + + expect(vol.readFileSync(outputPath, "utf8")).toBe("previous"); + expect(vol.readdirSync(DIR)).toEqual(["result.txt"]); + }); + + it("returns the writer callback's value", async () => { + const result = await writeAtomically(`${DIR}/x`, (tempPath) => { + vol.writeFileSync(tempPath, ""); + return Promise.resolve(42); + }); + + expect(result).toBe(42); + }); +});