Merge pull request #1029 from bbassie/fix/instance-assign-failure

Keep instances assignable after a failed assign or create

packages/controller/src/ControlConnection.ts

@@ -292,6 +292,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,7 +1,7 @@
 import assert from "node:assert/strict";
 import events from "node:events";
 import * as lib from "@clusterio/lib";
-import { Controller, ControlConnection, ControllerHooks } from "@clusterio/controller";
+import { Controller, ControlConnection, ControllerHooks, InstanceManager } from "@clusterio/controller";
 import { MockConnector, MockController, MockLogger } from "../mock.js";
 
 const addr = lib.Address.fromShorthand;
@@ -76,6 +76,41 @@ describe("controller/src/ControlConnection", function() {
 		});
 	});
 
+	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);
+		});
+	});
+
 	describe("close cleanup", function() {
 		let connector;
 		let mockController;

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 () {