Remove lib.findUnusedName

The name it returns may already be taken by a concurrently running process
by the time the caller uses it. Callers now try the name and retry with a
-2, -3, ... suffix on EEXIST instead.

Save transfers previously renamed with fs.rename, which silently overwrites
a save that appears at the target, and copied without COPYFILE_EXCL. Moves
now use link + unlink like save rename does, and copies pass COPYFILE_EXCL,
so both fail with EEXIST rather than overwrite.

Supersedes #880.

Co-authored-by: Hornwitser <code@hornwitser.no>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

packages/host/src/Host.ts

@@ -499,7 +499,7 @@ export default class Host extends lib.Link {
 
 		let instancesDir = this.config.get("host.instances_directory");
 		for (let i = 0; i < 10; i++) { // Limit attempts in case this is somehow an infinite loop
-			let candidateDir = path.join(instancesDir, await lib.findUnusedName(instancesDir, name));
+			let candidateDir = path.join(instancesDir, i === 0 ? name : `${name}-${i + 1}`);
 			try {
 				await fs.mkdir(candidateDir);
 			} catch (err: any) {
@@ -1064,6 +1064,32 @@ export default class Host extends lib.Link {
 		await this.sendSaveListUpdate(instanceId, path.join(instance.path, "saves"));
 	}
 
+	async _moveOrCopySave(sourcePath: string, targetPath: string, copy: boolean) {
+		const { dir, name, ext } = path.parse(targetPath);
+		for (let i = 0; i < 10; i++) {
+			targetPath = path.format({ dir, name: i === 0 ? name : `${name}-${i + 1}`, ext });
+			try {
+				if (copy) {
+					await fs.copyFile(
+						sourcePath,
+						targetPath,
+						fs.constants.COPYFILE_EXCL | fs.constants.COPYFILE_FICLONE
+					);
+				} else {
+					// Unlike rename, link fails if the target exists.
+					await fs.link(sourcePath, targetPath);
+					await fs.unlink(sourcePath);
+				}
+				return path.basename(targetPath);
+			} catch (err: any) {
+				if (err.code !== "EEXIST") {
+					throw err;
+				}
+			}
+		}
+		throw new lib.RequestError("Unable to transfer save, retry threshold reached");
+	}
+
 	async handleInstanceTransferSaveRequest(request: lib.InstanceTransferSaveRequest) {
 		let { sourceName, targetName, copy, sourceInstanceId, targetInstanceId } = request;
 		checkRequestSaveName(sourceName);
@@ -1073,29 +1099,19 @@ export default class Host extends lib.Link {
 
 		// For consistency with remote transfer initiated through pullSave the
 		// target is renamed if it already exists.
-		targetName = await lib.findUnusedName(
-			path.join(targetInstance.path, "saves"), targetName, ".zip"
-		);
-
 		try {
-			if (copy) {
-				await fs.copyFile(
-					path.join(sourceInstance.path, "saves", sourceName),
-					path.join(targetInstance.path, "saves", targetName),
-					fs.constants.COPYFILE_FICLONE
-				);
-			} else {
-				await fs.rename(
-					path.join(sourceInstance.path, "saves", sourceName),
-					path.join(targetInstance.path, "saves", targetName),
-				);
-			}
+			targetName = await this._moveOrCopySave(
+				path.join(sourceInstance.path, "saves", sourceName),
+				path.join(targetInstance.path, "saves", targetName),
+				copy,
+			);
 		} catch (err: any) {
 			if (err.code === "ENOENT") {
 				throw new lib.RequestError(`${sourceName} does not exist`);
 			}
 			throw err;
 		}
+
 		await this.sendSaveListUpdate(sourceInstanceId, path.join(sourceInstance.path, "saves"));
 		await this.sendSaveListUpdate(targetInstanceId, path.join(targetInstance.path, "saves"));
 

packages/lib/src/file_ops.ts

@@ -70,48 +70,7 @@ export async function directorySize(directory: string) {
 }
 
 /**
- * Modifies name in case it already exisist at the given directory
- *
- * Checks the directory passed if it already contains a file or folder with
- * the given name, if it does not returns name unmodified, otherwise it
- * returns a modified name that does not exist in the directory.
- *
- * Warning: this function should not be relied upon to be accurate for
- * security sensitive applications.  The selection process is inherently
- * racy and a file or folder may have been created in the folder by the time
- * this function returns.
- *
- * @param directory - directory to check in.
- * @param name - file name to check for, may have extension.
- * @param extension - dot extension used for the file name.
- * @returns modified name with extension that likely does
- *     not exist in the folder
- */
-export async function findUnusedName(directory: string, name: string, extension = "") {
-	if (extension && name.endsWith(extension)) {
-		name = name.slice(0, -extension.length);
-	}
-
-	while (true) {
-		try {
-			await fs.access(path.join(directory, `${name}${extension}`));
-		} catch (err: any) {
-			if (err.code === "ENOENT") {
-				return `${name}${extension}`;
-			}
-		}
-
-		let match = /^(.*?)(-(\d+))?$/.exec(name)!;
-		if (!match[2]) {
-			name = `${match[1]}-2`;
-		} else {
-			name = `${match[1]}-${Number.parseInt(match[3], 10) + 1}`;
-		}
-	}
-}
-
-/**
- * Generate collision resistante temporary file name
+ * Generate collision resistant temporary file name
  *
  * Warning: this function should not be relied upon to be accurate for
  * security sensitive applications.  The selection process is inherently
@@ -125,8 +84,7 @@ export async function findUnusedName(directory: string, name: string, extension
  */
 export async function getTempFile(prefix = "tmp.", suffix = "", tmpdir = "./") {
 	let fileName = path.join(prefix + crypto.randomBytes(16).toString("hex") + suffix);
-	let freeFile = await findUnusedName(tmpdir, fileName);
-	let fullPath = path.join(tmpdir, freeFile);
+	let fullPath = path.join(tmpdir, fileName);
 	return fullPath;
 }
 

test/integration/clusterio.js

@@ -1469,6 +1469,16 @@ describe("Integration of Clusterio", function() {
 						await deleteSave(pri, "transfer.zip");
 						await deleteSave(sec, "transfer.zip");
 					});
+					it("should auto-rename copy if target exists", async function() {
+						await uploadSave(pri, "transfer.zip", "transfer.zip");
+						await uploadSave(sec, "transfer.zip", "transfer.zip");
+						await execCtlProcess(`instance save transfer ${pri} transfer.zip ${sec} --copy`);
+						await assert.doesNotReject(fs.access(path.join(priSaves, "transfer.zip")), "save not at pri");
+						await assert.doesNotReject(fs.access(path.join(secSaves, "transfer-2.zip")), "save not at sec");
+						await deleteSave(pri, "transfer.zip");
+						await deleteSave(sec, "transfer.zip");
+						await deleteSave(sec, "transfer-2.zip");
+					});
 					it("should fail if save does not exist", async function() {
 						await assert.rejects(execCtl(`instance save transfer ${pri} not-here.zip ${sec}`));
 					});

test/lib/file_ops.js

@@ -17,13 +17,6 @@ describe("lib/file_ops", function() {
 		await fs.writeFile(path.join(baseDir, "test", "file.txt"), "contents");
 		await fs.writeFile(path.join(baseDir, "test", "another file.txt"), "more contents");
 
-		await fs.mkdir(path.join(baseDir, "find"), { recursive: true });
-		await fs.writeFile(path.join(baseDir, "find", "file"), "contents");
-		await fs.writeFile(path.join(baseDir, "find", "file.txt"), "contents");
-		await fs.writeFile(path.join(baseDir, "find", "foo-1"), "contents");
-		await fs.writeFile(path.join(baseDir, "find", "foo-2"), "contents");
-		await fs.writeFile(path.join(baseDir, "find", "bar-1.txt"), "contents");
-		await fs.writeFile(path.join(baseDir, "find", "bar-2.txt"), "contents");
 	}
 
 	before(setupTestingEnv);
@@ -58,34 +51,6 @@ describe("lib/file_ops", function() {
 		});
 	});
 
-	describe("findUnusedName()", function() {
-		it("should return named unchanged if it does not exist", async function() {
-			let cases = [
-				[["file"], "file"],
-				[["file", ".txt"], "file.txt"],
-				[["file.txt", ".txt"], "file.txt"],
-			];
-			for (let [args, expected] of cases) {
-				let actual = await lib.findUnusedName(path.join(baseDir, "test", "folder"), ...args);
-				assert.equal(actual, expected);
-			}
-		});
-		it("should return changed name if it does exist", async function() {
-			let cases = [
-				[["file"], "file-2"],
-				[["file", ".txt"], "file-2.txt"],
-				[["file.txt", ".txt"], "file-2.txt"],
-				[["foo-1"], "foo-3"],
-				[["bar-1", ".txt"], "bar-3.txt"],
-				[["bar-1.txt", ".txt"], "bar-3.txt"],
-			];
-			for (let [args, expected] of cases) {
-				let actual = await lib.findUnusedName(path.join(baseDir, "find"), ...args);
-				assert.equal(actual, expected);
-			}
-		});
-	});
-
 	describe("safeOutputFile()", function() {
 		async function assertNoTemporary(target) {
 			let { dir, name } = path.parse(target);