Keep instances assignable after a failed assign or create

assignInstance sets instance.assigned_host to the new host before
sending InstanceAssignInternalRequest. If the host fails that request
the controller kept the new host id while the host never added the
instance. Assigning to the same host again then returned early as a
no-op. Repro: make the host's instances directory read-only, assign
instance 3001 to host 10 (EACCES on mkdir), make it writable, assign
again (ok, nothing happens), start (instance does not exist on host).
Now a failed send leaves the instance unassigned and rethrows.

InstanceCreateRequest applied instance.assigned_host from the request
config, creating an instance assigned to a host that never heard of
it. Repro: InstanceCreateRequest({"instance.id": 4001, "instance.name":
"c1", "instance.assigned_host": 10}). Reject a non-null value with the
same error the config set requests use.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

packages/controller/src/ControlConnection.ts

@@ -288,6 +288,9 @@ export default class ControlConnection extends BaseConnection {
 	}
 
 	async handleInstanceCreateRequest(request: lib.InstanceCreateRequest) {
+		if ((request.config["instance.assigned_host"] ?? null) !== null) {
+			throw new lib.RequestError("instance.assigned_host must be set through the assign-host interface");
+		}
 		const instanceConfig = new lib.InstanceConfig("controller");
 		if (request.cloneFromId) {
 			const baseInstance = this._controller.instances.get(request.cloneFromId);

packages/controller/src/InstanceManager.ts

@@ -208,9 +208,17 @@ export default class InstanceManager {
 		instance.config.set("instance.assigned_host", hostId);
 
 		if (newHostConnection) {
-			await newHostConnection.send(
-				new lib.InstanceAssignInternalRequest(instanceId, instance.config.toRemote("host")),
-			);
+			try {
+				await newHostConnection.send(
+					new lib.InstanceAssignInternalRequest(instanceId, instance.config.toRemote("host")),
+				);
+			} catch (err) {
+				// Leave unassigned so the assign can be retried
+				instance.config.set("instance.assigned_host", null);
+				instance.status = "unassigned";
+				this.records.set(instance);
+				throw err;
+			}
 		} else {
 			instance.status = "unassigned";
 		}

test/controller/ControlConnection.test.js

@@ -1,5 +1,7 @@
 import assert from "node:assert/strict";
-import { ControlConnection } from "@clusterio/controller";
+import * as lib from "@clusterio/lib";
+import { ControlConnection, InstanceManager } from "@clusterio/controller";
+import { MockController } from "../mock.js";
 
 describe("controller/src/ControlConnection", function() {
 	describe(".handleControllerRestartRequest()", function() {
@@ -38,4 +40,39 @@ describe("controller/src/ControlConnection", function() {
 			assert.equal(mockController.stopped, false);
 		});
 	});
+
+	describe(".handleInstanceCreateRequest()", function() {
+		let mockController;
+
+		beforeEach(function() {
+			mockController = new MockController();
+			mockController.mockConfigEntries.set("controller.name", "Test");
+			mockController.instances = new InstanceManager(new lib.SubscribableDatastore(), mockController);
+		});
+
+		async function create(config, cloneFromId) {
+			await ControlConnection.prototype.handleInstanceCreateRequest.call(
+				{ _controller: mockController },
+				new lib.InstanceCreateRequest(config, cloneFromId),
+			);
+		}
+
+		it("rejects instance.assigned_host in the config", async function() {
+			await assert.rejects(
+				create({ "instance.id": 4001, "instance.name": "c1", "instance.assigned_host": 10 }),
+				new lib.RequestError("instance.assigned_host must be set through the assign-host interface"),
+			);
+			assert.equal(mockController.instances.has(4001), false);
+		});
+
+		it("creates an unassigned clone of an assigned instance", async function() {
+			await create({ "instance.id": 4001, "instance.name": "base" });
+			mockController.instances.getMutable(4001).config.set("instance.assigned_host", 10);
+
+			await create({ "instance.id": 4002, "instance.name": "clone", "instance.assigned_host": null }, 4001);
+			const clone = mockController.instances.get(4002);
+			assert.equal(clone.config.get("instance.name"), "clone");
+			assert.equal(clone.config.get("instance.assigned_host"), null);
+		});
+	});
 });

test/controller/InstanceManager.test.js

@@ -298,6 +298,45 @@ describe("controller/InstanceManager", function () {
 			assert(assignRequest instanceof lib.InstanceAssignInternalRequest);
 			assert.equal(assignRequest.instanceId, 1);
 		});
+
+		it("should leave instance unassigned if the new host fails the assign", async function () {
+			let unassignRequest;
+			controller.wsServer.hostConnections.set(5, {
+				send: async (request) => {
+					unassignRequest = request;
+				},
+				connector: { closing: false },
+			});
+			let failAssign = true;
+			let assignRequest;
+			controller.wsServer.hostConnections.set(6, {
+				send: async (request) => {
+					if (failAssign) {
+						throw new lib.RequestError("EACCES: permission denied");
+					}
+					assignRequest = request;
+				},
+				connector: { closing: false },
+			});
+
+			instance.config.set("instance.assigned_host", 5);
+			instance.status = "stopped";
+			await assert.rejects(
+				() => instances.assignInstance(1, 6),
+				{ message: "EACCES: permission denied" }
+			);
+
+			assert(unassignRequest instanceof lib.InstanceUnassignInternalRequest);
+			const stored = instances.records.get(1);
+			assert.equal(stored.config.get("instance.assigned_host"), null);
+			assert.equal(stored.status, "unassigned");
+
+			failAssign = false;
+			await instances.assignInstance(1, 6);
+			assert.equal(instance.config.get("instance.assigned_host"), 6);
+			assert(assignRequest instanceof lib.InstanceAssignInternalRequest);
+			assert.equal(assignRequest.instanceId, 1);
+		});
 	});
 
 	describe(".unassignInstance()", function () {