diff --git a/CHANGELOG.md b/CHANGELOG.md index 6dcb88c2..fea55164 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,11 @@ ## Unreleased +### Fixed + +- Fixed SSH config writes failing on Windows when antivirus, cloud sync software, + or another process briefly locks the file. + ### Added - Automatically set `reconnectionGraceTime`, `serverShutdownTimeout`, and `maxReconnectionAttempts` diff --git a/src/core/cliCredentialManager.ts b/src/core/cliCredentialManager.ts index 5b723637..cd7bbe63 100644 --- a/src/core/cliCredentialManager.ts +++ b/src/core/cliCredentialManager.ts @@ -7,7 +7,7 @@ import * as semver from "semver"; import { isKeyringEnabled } from "../cliConfig"; import { featureSetForVersion } from "../featureSet"; import { getHeaderArgs } from "../headers"; -import { tempFilePath, toSafeHost } from "../util"; +import { renameWithRetry, tempFilePath, toSafeHost } from "../util"; import * as cliUtils from "./cliUtils"; @@ -256,7 +256,7 @@ export class CliCredentialManager { const tempPath = tempFilePath(filePath, "temp"); try { await fs.writeFile(tempPath, content, { mode: 0o600 }); - await fs.rename(tempPath, filePath); + 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); diff --git a/src/remote/remote.ts b/src/remote/remote.ts index d661a013..395a9da1 100644 --- a/src/remote/remote.ts +++ b/src/remote/remote.ts @@ -51,7 +51,7 @@ import { vscodeProposed } from "../vscodeProposed"; import { WorkspaceMonitor } from "../workspace/workspaceMonitor"; import { - SSHConfig, + SshConfig, type SSHValues, mergeSshConfigValues, parseCoderSshOptions, @@ -766,7 +766,7 @@ export class Remote { sshConfigFile = path.join(os.homedir(), sshConfigFile.slice(1)); } - const sshConfig = new SSHConfig(sshConfigFile); + const sshConfig = new SshConfig(sshConfigFile, this.logger); await sshConfig.load(); // Merge SSH config from three sources (highest to lowest priority): diff --git a/src/remote/sshConfig.ts b/src/remote/sshConfig.ts index 2ac221ff..ee26eb25 100644 --- a/src/remote/sshConfig.ts +++ b/src/remote/sshConfig.ts @@ -1,9 +1,18 @@ -import { mkdir, readFile, rename, stat, writeFile } from "node:fs/promises"; +import { + mkdir, + readFile, + rename, + stat, + unlink, + writeFile, +} from "node:fs/promises"; import path from "node:path"; -import { countSubstring, tempFilePath } from "../util"; +import { countSubstring, renameWithRetry, tempFilePath } from "../util"; -class SSHConfigBadFormat extends Error {} +import type { Logger } from "../logging/logger"; + +class SshConfigBadFormat extends Error {} interface Block { raw: string; @@ -25,6 +34,7 @@ export interface FileSystem { readFile: typeof readFile; rename: typeof rename; stat: typeof stat; + unlink: typeof unlink; writeFile: typeof writeFile; } @@ -33,6 +43,7 @@ const defaultFileSystem: FileSystem = { readFile, rename, stat, + unlink, writeFile, }; @@ -163,9 +174,10 @@ export function mergeSshConfigValues( return merged; } -export class SSHConfig { +export class SshConfig { private readonly filePath: string; private readonly fileSystem: FileSystem; + private readonly logger: Logger; private raw: string | undefined; private startBlockComment(safeHostname: string): string { @@ -179,16 +191,25 @@ export class SSHConfig { : `# --- END CODER VSCODE ---`; } - constructor(filePath: string, fileSystem: FileSystem = defaultFileSystem) { + constructor( + filePath: string, + logger: Logger, + fileSystem: FileSystem = defaultFileSystem, + ) { this.filePath = filePath; + this.logger = logger; this.fileSystem = fileSystem; } async load() { try { this.raw = await this.fileSystem.readFile(this.filePath, "utf-8"); + this.logger.debug("Loaded SSH config", this.filePath); } catch { - // Probably just doesn't exist! + this.logger.debug( + "SSH config file not found, starting fresh", + this.filePath, + ); this.raw = ""; } } @@ -204,8 +225,10 @@ export class SSHConfig { const block = this.getBlock(safeHostname); const newBlock = this.buildBlock(safeHostname, values, overrides); if (block) { + this.logger.debug("Replacing SSH config block", safeHostname); this.replaceBlock(block, newBlock); } else { + this.logger.debug("Appending new SSH config block", safeHostname); this.appendBlock(newBlock); } await this.save(); @@ -222,13 +245,13 @@ export class SSHConfig { const startBlockCount = countSubstring(startBlock, raw); const endBlockCount = countSubstring(endBlock, raw); if (startBlockCount !== endBlockCount) { - throw new SSHConfigBadFormat( + throw new SshConfigBadFormat( `Malformed config: ${this.filePath} has an unterminated START CODER VSCODE ${safeHostname ? safeHostname + " " : ""}block. Each START block must have an END block.`, ); } if (startBlockCount > 1 || endBlockCount > 1) { - throw new SSHConfigBadFormat( + throw new SshConfigBadFormat( `Malformed config: ${this.filePath} has ${startBlockCount} START CODER VSCODE ${safeHostname ? safeHostname + " " : ""}sections. Please remove all but one.`, ); } @@ -241,15 +264,15 @@ export class SSHConfig { } if (startBlockIndex === -1) { - throw new SSHConfigBadFormat("Start block not found"); + throw new SshConfigBadFormat("Start block not found"); } if (startBlockIndex === -1) { - throw new SSHConfigBadFormat("End block not found"); + throw new SshConfigBadFormat("End block not found"); } if (endBlockIndex < startBlockIndex) { - throw new SSHConfigBadFormat( + throw new SshConfigBadFormat( "Malformed config, end block is before start block", ); } @@ -357,8 +380,20 @@ export class SSHConfig { } try { - await this.fileSystem.rename(tempPath, this.filePath); + await renameWithRetry( + (src, dest) => this.fileSystem.rename(src, dest), + tempPath, + this.filePath, + ); + this.logger.debug("Saved SSH config", this.filePath); } catch (err) { + await this.fileSystem.unlink(tempPath).catch((unlinkErr: unknown) => { + this.logger.warn( + "Failed to clean up temp SSH config file", + tempPath, + unlinkErr, + ); + }); throw new Error( `Failed to rename temporary SSH config file at ${tempPath} to ${this.filePath}: ${ err instanceof Error ? err.message : String(err) @@ -370,7 +405,7 @@ export class SSHConfig { public getRaw() { if (this.raw === undefined) { - throw new Error("SSHConfig is not loaded. Try sshConfig.load()"); + throw new Error("SshConfig is not loaded. Try sshConfig.load()"); } return this.raw; diff --git a/src/util.ts b/src/util.ts index 01d2db16..f9aa549d 100644 --- a/src/util.ts +++ b/src/util.ts @@ -150,6 +150,51 @@ 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)); + } + } +} + export function escapeCommandArg(arg: string): string { const escapedString = arg.replaceAll('"', String.raw`\"`); return `"${escapedString}"`; diff --git a/test/unit/remote/sshConfig.test.ts b/test/unit/remote/sshConfig.test.ts index a68c0306..a460e4c6 100644 --- a/test/unit/remote/sshConfig.test.ts +++ b/test/unit/remote/sshConfig.test.ts @@ -1,12 +1,14 @@ -import { it, afterEach, vi, expect, describe } from "vitest"; +import { it, afterEach, vi, expect, describe, beforeEach } from "vitest"; import { - SSHConfig, + SshConfig, parseCoderSshOptions, parseSshConfig, mergeSshConfigValues, } from "@/remote/sshConfig"; +import { createMockLogger } from "../../mocks/testHelpers"; + // This is not the usual path to ~/.ssh/config, but // setting it to a different path makes it easier to test // and makes mistakes abundantly clear. @@ -21,9 +23,12 @@ const mockFileSystem = { readFile: vi.fn(), rename: vi.fn(), stat: vi.fn(), + unlink: vi.fn().mockResolvedValue(undefined), writeFile: vi.fn(), }; +const mockLogger = createMockLogger(); + afterEach(() => { vi.clearAllMocks(); }); @@ -32,7 +37,7 @@ it("creates a new file and adds config with empty label", async () => { mockFileSystem.readFile.mockRejectedValueOnce("No file found"); mockFileSystem.stat.mockRejectedValueOnce({ code: "ENOENT" }); - const sshConfig = new SSHConfig(sshFilePath, mockFileSystem); + const sshConfig = new SshConfig(sshFilePath, mockLogger, mockFileSystem); await sshConfig.load(); await sshConfig.update("", { Host: "coder-vscode--*", @@ -53,11 +58,11 @@ Host coder-vscode--* UserKnownHostsFile /dev/null # --- END CODER VSCODE ---`; - expect(mockFileSystem.readFile).toBeCalledWith( + expect(mockFileSystem.readFile).toHaveBeenCalledWith( sshFilePath, expect.anything(), ); - expect(mockFileSystem.writeFile).toBeCalledWith( + expect(mockFileSystem.writeFile).toHaveBeenCalledWith( expect.stringContaining(sshTempFilePrefix), expectedOutput, expect.objectContaining({ @@ -65,7 +70,7 @@ Host coder-vscode--* mode: 0o600, // Default mode for new files. }), ); - expect(mockFileSystem.rename).toBeCalledWith( + expect(mockFileSystem.rename).toHaveBeenCalledWith( expect.stringContaining(sshTempFilePrefix), sshFilePath, ); @@ -75,7 +80,7 @@ it("creates a new file and adds the config", async () => { mockFileSystem.readFile.mockRejectedValueOnce("No file found"); mockFileSystem.stat.mockRejectedValueOnce({ code: "ENOENT" }); - const sshConfig = new SSHConfig(sshFilePath, mockFileSystem); + const sshConfig = new SshConfig(sshFilePath, mockLogger, mockFileSystem); await sshConfig.load(); await sshConfig.update("dev.coder.com", { Host: "coder-vscode.dev.coder.com--*", @@ -96,11 +101,11 @@ Host coder-vscode.dev.coder.com--* UserKnownHostsFile /dev/null # --- END CODER VSCODE dev.coder.com ---`; - expect(mockFileSystem.readFile).toBeCalledWith( + expect(mockFileSystem.readFile).toHaveBeenCalledWith( sshFilePath, expect.anything(), ); - expect(mockFileSystem.writeFile).toBeCalledWith( + expect(mockFileSystem.writeFile).toHaveBeenCalledWith( expect.stringContaining(sshTempFilePrefix), expectedOutput, expect.objectContaining({ @@ -108,7 +113,7 @@ Host coder-vscode.dev.coder.com--* mode: 0o600, // Default mode for new files. }), ); - expect(mockFileSystem.rename).toBeCalledWith( + expect(mockFileSystem.rename).toHaveBeenCalledWith( expect.stringContaining(sshTempFilePrefix), sshFilePath, ); @@ -125,7 +130,7 @@ it("adds a new coder config in an existent SSH configuration", async () => { mockFileSystem.readFile.mockResolvedValueOnce(existentSSHConfig); mockFileSystem.stat.mockResolvedValueOnce({ mode: 0o644 }); - const sshConfig = new SSHConfig(sshFilePath, mockFileSystem); + const sshConfig = new SshConfig(sshFilePath, mockLogger, mockFileSystem); await sshConfig.load(); await sshConfig.update("dev.coder.com", { Host: "coder-vscode.dev.coder.com--*", @@ -148,7 +153,7 @@ Host coder-vscode.dev.coder.com--* UserKnownHostsFile /dev/null # --- END CODER VSCODE dev.coder.com ---`; - expect(mockFileSystem.writeFile).toBeCalledWith( + expect(mockFileSystem.writeFile).toHaveBeenCalledWith( expect.stringContaining(sshTempFilePrefix), expectedOutput, { @@ -156,7 +161,7 @@ Host coder-vscode.dev.coder.com--* mode: 0o644, }, ); - expect(mockFileSystem.rename).toBeCalledWith( + expect(mockFileSystem.rename).toHaveBeenCalledWith( expect.stringContaining(sshTempFilePrefix), sshFilePath, ); @@ -196,7 +201,7 @@ Host * mockFileSystem.readFile.mockResolvedValueOnce(existentSSHConfig); mockFileSystem.stat.mockResolvedValueOnce({ mode: 0o644 }); - const sshConfig = new SSHConfig(sshFilePath, mockFileSystem); + const sshConfig = new SshConfig(sshFilePath, mockLogger, mockFileSystem); await sshConfig.load(); await sshConfig.update("dev.coder.com", { Host: "coder-vscode.dev-updated.coder.com--*", @@ -222,7 +227,7 @@ Host coder-vscode.dev-updated.coder.com--* Host * SetEnv TEST=1`; - expect(mockFileSystem.writeFile).toBeCalledWith( + expect(mockFileSystem.writeFile).toHaveBeenCalledWith( expect.stringContaining(sshTempFilePrefix), expectedOutput, { @@ -230,7 +235,7 @@ Host * mode: 0o644, }, ); - expect(mockFileSystem.rename).toBeCalledWith( + expect(mockFileSystem.rename).toHaveBeenCalledWith( expect.stringContaining(sshTempFilePrefix), sshFilePath, ); @@ -254,7 +259,7 @@ Host coder-vscode--* mockFileSystem.readFile.mockResolvedValueOnce(existentSSHConfig); mockFileSystem.stat.mockResolvedValueOnce({ mode: 0o644 }); - const sshConfig = new SSHConfig(sshFilePath, mockFileSystem); + const sshConfig = new SshConfig(sshFilePath, mockLogger, mockFileSystem); await sshConfig.load(); await sshConfig.update("dev.coder.com", { Host: "coder-vscode.dev.coder.com--*", @@ -277,7 +282,7 @@ Host coder-vscode.dev.coder.com--* UserKnownHostsFile /dev/null # --- END CODER VSCODE dev.coder.com ---`; - expect(mockFileSystem.writeFile).toBeCalledWith( + expect(mockFileSystem.writeFile).toHaveBeenCalledWith( expect.stringContaining(sshTempFilePrefix), expectedOutput, { @@ -285,7 +290,7 @@ Host coder-vscode.dev.coder.com--* mode: 0o644, }, ); - expect(mockFileSystem.rename).toBeCalledWith( + expect(mockFileSystem.rename).toHaveBeenCalledWith( expect.stringContaining(sshTempFilePrefix), sshFilePath, ); @@ -297,7 +302,7 @@ it("it does not remove a user-added block that only matches the host of an old c mockFileSystem.readFile.mockResolvedValueOnce(existentSSHConfig); mockFileSystem.stat.mockResolvedValueOnce({ mode: 0o644 }); - const sshConfig = new SSHConfig(sshFilePath, mockFileSystem); + const sshConfig = new SshConfig(sshFilePath, mockLogger, mockFileSystem); await sshConfig.load(); await sshConfig.update("dev.coder.com", { Host: "coder-vscode.dev.coder.com--*", @@ -321,7 +326,7 @@ Host coder-vscode.dev.coder.com--* UserKnownHostsFile /dev/null # --- END CODER VSCODE dev.coder.com ---`; - expect(mockFileSystem.writeFile).toBeCalledWith( + expect(mockFileSystem.writeFile).toHaveBeenCalledWith( expect.stringContaining(sshTempFilePrefix), expectedOutput, { @@ -329,7 +334,7 @@ Host coder-vscode.dev.coder.com--* mode: 0o644, }, ); - expect(mockFileSystem.rename).toBeCalledWith( + expect(mockFileSystem.rename).toHaveBeenCalledWith( expect.stringContaining(sshTempFilePrefix), sshFilePath, ); @@ -354,7 +359,7 @@ Host afterconfig HostName after.config.tld User after`; - const sshConfig = new SSHConfig(sshFilePath, mockFileSystem); + const sshConfig = new SshConfig(sshFilePath, mockLogger, mockFileSystem); mockFileSystem.readFile.mockResolvedValueOnce(existentSSHConfig); await sshConfig.load(); @@ -409,7 +414,7 @@ Host afterconfig HostName after.config.tld User after`; - const sshConfig = new SSHConfig(sshFilePath, mockFileSystem); + const sshConfig = new SshConfig(sshFilePath, mockLogger, mockFileSystem); mockFileSystem.readFile.mockResolvedValueOnce(existentSSHConfig); await sshConfig.load(); @@ -460,7 +465,7 @@ Host afterconfig HostName after.config.tld User after`; - const sshConfig = new SSHConfig(sshFilePath, mockFileSystem); + const sshConfig = new SshConfig(sshFilePath, mockLogger, mockFileSystem); mockFileSystem.readFile.mockResolvedValueOnce(existentSSHConfig); await sshConfig.load(); @@ -510,7 +515,7 @@ Host afterconfig HostName after.config.tld User after`; - const sshConfig = new SSHConfig(sshFilePath, mockFileSystem); + const sshConfig = new SshConfig(sshFilePath, mockLogger, mockFileSystem); mockFileSystem.readFile.mockResolvedValueOnce(existentSSHConfig); await sshConfig.load(); @@ -560,7 +565,7 @@ Host afterconfig HostName after.config.tld User after`; - const sshConfig = new SSHConfig(sshFilePath, mockFileSystem); + const sshConfig = new SshConfig(sshFilePath, mockLogger, mockFileSystem); mockFileSystem.readFile.mockResolvedValueOnce(existentSSHConfig); mockFileSystem.stat.mockResolvedValueOnce({ mode: 0o644 }); await sshConfig.load(); @@ -605,7 +610,7 @@ Host afterconfig LogLevel: "ERROR", }); - expect(mockFileSystem.writeFile).toBeCalledWith( + expect(mockFileSystem.writeFile).toHaveBeenCalledWith( expect.stringContaining(sshTempFilePrefix), expectedOutput, { @@ -613,7 +618,7 @@ Host afterconfig mode: 0o644, }, ); - expect(mockFileSystem.rename).toBeCalledWith( + expect(mockFileSystem.rename).toHaveBeenCalledWith( expect.stringContaining(sshTempFilePrefix), sshFilePath, ); @@ -623,7 +628,7 @@ it("override values", async () => { mockFileSystem.readFile.mockRejectedValueOnce("No file found"); mockFileSystem.stat.mockRejectedValueOnce({ code: "ENOENT" }); - const sshConfig = new SSHConfig(sshFilePath, mockFileSystem); + const sshConfig = new SshConfig(sshFilePath, mockLogger, mockFileSystem); await sshConfig.load(); await sshConfig.update( "dev.coder.com", @@ -659,11 +664,11 @@ Host coder-vscode.dev.coder.com--* loglevel DEBUG # --- END CODER VSCODE dev.coder.com ---`; - expect(mockFileSystem.readFile).toBeCalledWith( + expect(mockFileSystem.readFile).toHaveBeenCalledWith( sshFilePath, expect.anything(), ); - expect(mockFileSystem.writeFile).toBeCalledWith( + expect(mockFileSystem.writeFile).toHaveBeenCalledWith( expect.stringContaining(sshTempFilePrefix), expectedOutput, expect.objectContaining({ @@ -671,7 +676,7 @@ Host coder-vscode.dev.coder.com--* mode: 0o600, // Default mode for new files. }), ); - expect(mockFileSystem.rename).toBeCalledWith( + expect(mockFileSystem.rename).toHaveBeenCalledWith( expect.stringContaining(sshTempFilePrefix), sshFilePath, ); @@ -682,14 +687,14 @@ it("fails if we are unable to write the temporary file", async () => { HostName before.config.tld User before`; - const sshConfig = new SSHConfig(sshFilePath, mockFileSystem); + const sshConfig = new SshConfig(sshFilePath, mockLogger, mockFileSystem); mockFileSystem.readFile.mockResolvedValueOnce(existentSSHConfig); mockFileSystem.stat.mockResolvedValueOnce({ mode: 0o600 }); mockFileSystem.writeFile.mockRejectedValueOnce(new Error("EACCES")); await sshConfig.load(); - expect(mockFileSystem.readFile).toBeCalledWith( + expect(mockFileSystem.readFile).toHaveBeenCalledWith( sshFilePath, expect.anything(), ); @@ -705,28 +710,72 @@ it("fails if we are unable to write the temporary file", async () => { ).rejects.toThrow(/Failed to write temporary SSH config file.*EACCES/); }); -it("fails if we are unable to rename the temporary file", async () => { - const existentSSHConfig = `Host beforeconfig - HostName before.config.tld - User before`; - - const sshConfig = new SSHConfig(sshFilePath, mockFileSystem); - mockFileSystem.readFile.mockResolvedValueOnce(existentSSHConfig); +it("cleans up temp file when rename fails", async () => { + mockFileSystem.readFile.mockResolvedValueOnce("Host existing\n HostName x"); mockFileSystem.stat.mockResolvedValueOnce({ mode: 0o600 }); mockFileSystem.writeFile.mockResolvedValueOnce(""); - mockFileSystem.rename.mockRejectedValueOnce(new Error("EACCES")); + const err = new Error("EXDEV"); + (err as NodeJS.ErrnoException).code = "EXDEV"; + mockFileSystem.rename.mockRejectedValueOnce(err); + const sshConfig = new SshConfig(sshFilePath, mockLogger, mockFileSystem); await sshConfig.load(); await expect( sshConfig.update("dev.coder.com", { Host: "coder-vscode.dev.coder.com--*", - ProxyCommand: "some-command-here", + ProxyCommand: "cmd", ConnectTimeout: "0", StrictHostKeyChecking: "no", UserKnownHostsFile: "/dev/null", LogLevel: "ERROR", }), - ).rejects.toThrow(/Failed to rename temporary SSH config file.*EACCES/); + ).rejects.toThrow(/Failed to rename temporary SSH config file/); + expect(mockFileSystem.unlink).toHaveBeenCalledWith( + expect.stringContaining(sshTempFilePrefix), + ); +}); + +describe("rename retry on Windows", () => { + const realPlatform = process.platform; + + beforeEach(() => { + Object.defineProperty(process, "platform", { value: "win32" }); + vi.useFakeTimers(); + }); + afterEach(() => { + vi.useRealTimers(); + Object.defineProperty(process, "platform", { value: realPlatform }); + }); + + it("retries on transient EPERM and succeeds", async () => { + mockFileSystem.readFile.mockResolvedValueOnce( + "Host existing\n HostName x", + ); + mockFileSystem.stat.mockResolvedValueOnce({ mode: 0o600 }); + mockFileSystem.writeFile.mockResolvedValueOnce(""); + const err = new Error("EPERM"); + (err as NodeJS.ErrnoException).code = "EPERM"; + mockFileSystem.rename + .mockRejectedValueOnce(err) + .mockResolvedValueOnce(undefined); + + const sshConfig = new SshConfig(sshFilePath, mockLogger, mockFileSystem); + await sshConfig.load(); + const promise = sshConfig.update("dev.coder.com", { + Host: "coder-vscode.dev.coder.com--*", + ProxyCommand: "cmd", + ConnectTimeout: "0", + StrictHostKeyChecking: "no", + UserKnownHostsFile: "/dev/null", + LogLevel: "ERROR", + }); + + await vi.advanceTimersByTimeAsync(100); + await promise; + + expect(mockFileSystem.rename).toHaveBeenCalledTimes(2); + expect(mockFileSystem.unlink).not.toHaveBeenCalled(); + }); }); describe("parseSshConfig", () => { diff --git a/test/unit/util.test.ts b/test/unit/util.test.ts index cf887ca5..8e07e145 100644 --- a/test/unit/util.test.ts +++ b/test/unit/util.test.ts @@ -1,5 +1,5 @@ import os from "node:os"; -import { describe, it, expect } from "vitest"; +import { afterEach, beforeEach, describe, it, expect, vi } from "vitest"; import { countSubstring, @@ -7,6 +7,7 @@ import { expandPath, findPort, parseRemoteAuthority, + renameWithRetry, tempFilePath, toSafeHost, } from "@/util"; @@ -260,3 +261,83 @@ describe("tempFilePath", () => { 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 as NodeJS.ErrnoException; + } + + 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); + }, + ); + }); +});