From fbdd647168c86031d64db4eb1ebb7b5ee494afb4 Mon Sep 17 00:00:00 2001 From: bbassie <17990055+bbassie@users.noreply.github.com> Date: Sun, 13 Sep 2026 01:36:54 +0000 Subject: [PATCH] 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) --- packages/controller/src/ControlConnection.ts | 3 ++ packages/controller/src/InstanceManager.ts | 14 +++++-- test/controller/ControlConnection.test.js | 39 +++++++++++++++++++- test/controller/InstanceManager.test.js | 39 ++++++++++++++++++++ 4 files changed, 91 insertions(+), 4 deletions(-) diff --git a/packages/controller/src/ControlConnection.ts b/packages/controller/src/ControlConnection.ts index b502429b..f07eda68 100644 --- a/packages/controller/src/ControlConnection.ts +++ b/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); diff --git a/packages/controller/src/InstanceManager.ts b/packages/controller/src/InstanceManager.ts index 0736c493..adc3d7d2 100644 --- a/packages/controller/src/InstanceManager.ts +++ b/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"; } diff --git a/test/controller/ControlConnection.test.js b/test/controller/ControlConnection.test.js index 2ad0db9d..3026e17e 100644 --- a/test/controller/ControlConnection.test.js +++ b/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); + }); + }); }); diff --git a/test/controller/InstanceManager.test.js b/test/controller/InstanceManager.test.js index 59e05063..237b3dca 100644 --- a/test/controller/InstanceManager.test.js +++ b/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 () {