From e16d0bc4107030631376e70d789dfa62de44266f Mon Sep 17 00:00:00 2001
From: bbassie <17990055+bbassie@users.noreply.github.com>
Date: Tue, 20 Oct 2026 19:06:49 +0000
Subject: [PATCH] Address review of save transfer and instance dir naming

Instance dir and save names are tried without a retry cap, as on master
and in downloadFile, so an 11th instance named "New Instance" still gets
a folder.

Moves fall back to an exclusive copy when hard links are unavailable
(EXDEV, EPERM, ENOTSUP), and remove the new target if the source can't
be unlinked, so a failed move leaves no duplicate.

Local transfers keep appending .zip to the target name, as
findUnusedName did.

The copy auto-rename test now checks that the existing target save is
left untouched.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
---
 packages/host/src/Host.ts     | 54 ++++++++++++++++++++++++++---------
 test/integration/clusterio.js |  5 ++--
 2 files changed, 43 insertions(+), 16 deletions(-)

diff --git a/packages/host/src/Host.ts b/packages/host/src/Host.ts
index 507bae82..06261c43 100644
--- a/packages/host/src/Host.ts
+++ b/packages/host/src/Host.ts
@@ -26,6 +26,36 @@ function checkRequestSaveName(name: string) {
 	}
 }
 
+async function copySaveExclusive(sourcePath: string, targetPath: string) {
+	await fs.copyFile(
+		sourcePath,
+		targetPath,
+		fs.constants.COPYFILE_EXCL | fs.constants.COPYFILE_FICLONE
+	);
+}
+
+/**
+ * Move a file without overwriting an existing target, fails with EEXIST
+ * if the target exists.
+ */
+async function moveSaveExclusive(sourcePath: string, targetPath: string) {
+	try {
+		await fs.link(sourcePath, targetPath);
+	} catch (err: any) {
+		// Hard links are not possible across filesystems and on some filesystems.
+		if (!["EXDEV", "EPERM", "ENOTSUP"].includes(err.code)) {
+			throw err;
+		}
+		await copySaveExclusive(sourcePath, targetPath);
+	}
+	try {
+		await fs.unlink(sourcePath);
+	} catch (err: any) {
+		await fs.unlink(targetPath).catch(() => {});
+		throw err;
+	}
+}
+
 /**
  * Checks that no socket is bound to the given UDP port, on IPv4 or IPv6
  *
@@ -498,8 +528,8 @@ 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, i === 0 ? name : `${name}-${i + 1}`);
+		for (let i = 1; ; i++) {
+			let candidateDir = path.join(instancesDir, i === 1 ? name : `${name}-${i}`);
 			try {
 				await fs.mkdir(candidateDir);
 			} catch (err: any) {
@@ -510,7 +540,6 @@ export default class Host extends lib.Link {
 			}
 			return candidateDir;
 		}
-		throw Error("Unable to create instance dir, retry threshold reached");
 	}
 
 	async broadcastEventToInstance<T>(event: lib.Event<T>) {
@@ -1066,19 +1095,13 @@ export default class Host extends lib.Link {
 
 	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 });
+		for (let i = 1; ; i++) {
+			targetPath = path.format({ dir, name: i === 1 ? name : `${name}-${i}`, ext });
 			try {
 				if (copy) {
-					await fs.copyFile(
-						sourcePath,
-						targetPath,
-						fs.constants.COPYFILE_EXCL | fs.constants.COPYFILE_FICLONE
-					);
+					await copySaveExclusive(sourcePath, targetPath);
 				} else {
-					// Unlike rename, link fails if the target exists.
-					await fs.link(sourcePath, targetPath);
-					await fs.unlink(sourcePath);
+					await moveSaveExclusive(sourcePath, targetPath);
 				}
 				return path.basename(targetPath);
 			} catch (err: any) {
@@ -1087,7 +1110,6 @@ export default class Host extends lib.Link {
 				}
 			}
 		}
-		throw new lib.RequestError("Unable to transfer save, retry threshold reached");
 	}
 
 	async handleInstanceTransferSaveRequest(request: lib.InstanceTransferSaveRequest) {
@@ -1097,6 +1119,10 @@ export default class Host extends lib.Link {
 		let sourceInstance = this.getRequestInstance(sourceInstanceId);
 		let targetInstance = this.getRequestInstance(targetInstanceId);
 
+		if (!targetName.endsWith(".zip")) {
+			targetName += ".zip";
+		}
+
 		// For consistency with remote transfer initiated through pullSave the
 		// target is renamed if it already exists.
 		try {
diff --git a/test/integration/clusterio.js b/test/integration/clusterio.js
index b6ed3627..65076237 100644
--- a/test/integration/clusterio.js
+++ b/test/integration/clusterio.js
@@ -1471,10 +1471,11 @@ describe("Integration of Clusterio", function() {
 					});
 					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 uploadSave(sec, "transfer.zip", "existing");
 						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");
+						assert.equal(await fs.readFile(path.join(secSaves, "transfer.zip"), "utf8"), "existing");
+						assert.equal(await fs.readFile(path.join(secSaves, "transfer-2.zip"), "utf8"), "transfer.zip");
 						await deleteSave(pri, "transfer.zip");
 						await deleteSave(sec, "transfer.zip");
 						await deleteSave(sec, "transfer-2.zip");