From 18aed4188978b0ce372b59ef7fefcc88954cbbd6 Mon Sep 17 00:00:00 2001 From: bbassie <17990055+bbassie@users.noreply.github.com> Date: Sun, 13 Sep 2026 01:35:53 +0000 Subject: [PATCH 1/3] Log errors from instance config change handlers The fieldChanged handler in the Instance constructor ran updateFactorioSettings and updateFactorioWhitelist with only a finally attached. A rejection from either went unhandled, and the host exits on unhandled rejections, taking every instance on it down. Two ways to hit it against a real host: - Start an instance and immediately toggle factorio.enable_whitelist. updateFactorioWhitelist calls sendRcon while the server state is still "new" and it throws "Expected state running,stopping but state is new". - Set factorio.settings tags to [1] on a running instance. applyAsConfig called tag.replace outside its try block and threw "tag.replace is not a function". Catch and log both on the instance logger before invoking the hook, and convert tags with String() so non-string entries no longer throw. Co-Authored-By: Claude Opus 5 (1M context) --- packages/host/src/Instance.ts | 10 +++++-- test/host/Instance.js | 52 ++++++++++++++++++++++++++++++++++- 2 files changed, 58 insertions(+), 4 deletions(-) diff --git a/packages/host/src/Instance.ts b/packages/host/src/Instance.ts index 3f78c67b..f0a59d0e 100644 --- a/packages/host/src/Instance.ts +++ b/packages/host/src/Instance.ts @@ -61,7 +61,7 @@ function applyAsConfig(name: string) { // Replace spaces with non-break spaces and delimit by spaces. // This does change the defined tags, but there doesn't seem to // be a way to include a space into a tag from the console. - value = value.map(tag => tag.replace(/ /g, "\u00a0")).join(" "); + value = value.map(tag => String(tag).replace(/ /g, "\u00a0")).join(" "); } try { await instance.sendRcon(`/config set ${name} ${value}`); @@ -199,9 +199,13 @@ export default class Instance extends lib.Link { if (field === "factorio.shutdown_timeout") { this.server.shutdownTimeoutMs = curr as number * 1000; } else if (field === "factorio.settings") { - this.updateFactorioSettings(curr as any, prev as any).finally(hook); + this.updateFactorioSettings(curr as any, prev as any).catch(err => { + this.logger.error(`Error updating server settings:\n${err.stack}`); + }).finally(hook); } else if (field === "factorio.enable_whitelist") { - this.updateFactorioWhitelist(curr as any).finally(hook); + this.updateFactorioWhitelist(curr as any).catch(err => { + this.logger.error(`Error updating whitelist:\n${err.stack}`); + }).finally(hook); } else { if (field === "factorio.max_concurrent_commands") { this.server.maxConcurrentCommands = curr as number; diff --git a/test/host/Instance.js b/test/host/Instance.js index 73e11c1a..de05589a 100644 --- a/test/host/Instance.js +++ b/test/host/Instance.js @@ -4,7 +4,7 @@ import path from "node:path"; import * as lib from "@clusterio/lib"; import { PlayerStats, wait } from "@clusterio/lib"; import Instance from "@clusterio/host/dist/node/src/Instance.js"; -import { MockConnector, MockServer } from "../mock.js"; +import { MockConnector, MockLogger, MockServer } from "../mock.js"; const addr = lib.Address.fromShorthand; @@ -38,6 +38,56 @@ describe("class Instance", function() { }); }); + describe("config fieldChanged", function() { + let errors; + let rejections; + let hookInvoked; + function onRejection(err) { rejections.push(err); } + beforeEach(function() { + errors = []; + rejections = []; + instance.logger = new MockLogger(); + instance.logger.error = message => errors.push(message); + instance.server.exampleSettings = async () => ({}); + hookInvoked = new Promise(resolve => { + instance.hooks.instanceConfigFieldChanged.attach("test", field => resolve(field)); + }); + process.on("unhandledRejection", onRejection); + }); + afterEach(function() { + process.off("unhandledRejection", onRejection); + }); + + it("should log whitelist update errors and invoke the hook", async function() { + instance.server.sendRcon = async () => { throw new Error("Expected state running,stopping"); }; + instance.config.set("factorio.enable_whitelist", true); + assert.equal(await hookInvoked, "factorio.enable_whitelist"); + await wait(10); + assert.deepEqual(rejections, []); + assert.equal(errors.length, 1); + assert.match(errors[0], /^Error updating whitelist:\nError: Expected state running,stopping/); + }); + + it("should log server settings update errors and invoke the hook", async function() { + instance.server.exampleSettings = async () => { throw new Error("no example settings"); }; + instance.config.set("factorio.settings", { name: "bar" }); + assert.equal(await hookInvoked, "factorio.settings"); + await wait(10); + assert.deepEqual(rejections, []); + assert.equal(errors.length, 1); + assert.match(errors[0], /^Error updating server settings:\nError: no example settings/); + }); + + it("should apply tags with non-string elements", async function() { + instance.config.set("factorio.settings", { tags: [1, "a b"] }); + await hookInvoked; + await wait(10); + assert.deepEqual(rejections, []); + assert.deepEqual(errors, []); + assert.deepEqual(instance.server.rconCommands, ["/config set tags 1 a b"]); + }); + }); + describe(".checkModPackVersion()", function() { function modPack(factorioVersion) { return lib.ModPack.fromJSON({ name: "pack", factorio_version: factorioVersion }); From 96362a28352a14685e0a8bee74c56a86dcdab353 Mon Sep 17 00:00:00 2001 From: bbassie <17990055+bbassie@users.noreply.github.com> Date: Sun, 13 Sep 2026 13:20:52 +0000 Subject: [PATCH 2/3] Wait for transitory instance statuses to settle The config changed handler ran updateFactorioSettings and updateFactorioWhitelist as soon as the field changed. While the instance is starting, stopping, creating_save or exporting_data the server is not ready for RCON, so the update threw and was only logged. The three list update events pushed from the controller had the same problem, except Link._processEvent swallowed the rejection and the update was lost without a trace. Add _waitForSettledStatus, which resolves once notifyStatus lands on running or stopped. notifyStatus is the only place _status changes and notifyExit always ends on stopped, so a waiter cannot hang. The settings, whitelist and list updates now go through it. Nothing is applied when the instance settles on stopped, because there is no server to apply it to. factorio.settings is written to server-settings.json and the lists to the list files on the next start, and enableWhitelist is read from the config when the next Instance is constructed. The plugin hook now runs immediately for the two config fields instead of being chained onto the update. Every other field already invokes it right away, and waiting for the instance to settle would hold the notification back for the whole startup. Co-Authored-By: Claude Opus 5 (1M context) --- packages/host/src/Instance.ts | 57 +++++++++++++++++++++++--- test/host/Instance.js | 76 +++++++++++++++++++++++++++++++++++ 2 files changed, 127 insertions(+), 6 deletions(-) diff --git a/packages/host/src/Instance.ts b/packages/host/src/Instance.ts index f0a59d0e..64a03bba 100644 --- a/packages/host/src/Instance.ts +++ b/packages/host/src/Instance.ts @@ -22,6 +22,11 @@ const scriptCommands = [ "/silent-command", "/sc", ]; +// Statuses the instance passes through on its way to running or stopped. +const transitoryStatuses: ReadonlySet = new Set([ + "starting", "stopping", "creating_save", "exporting_data", +]); + const instanceRconCommandDuration = new lib.Histogram( "clusterio_instance_rcon_command_duration_seconds", "Histogram of the RCON command duration from request to response.", @@ -170,6 +175,7 @@ export default class Instance extends lib.Link { _hadPlayersOnline = false; _playerAutosaveSlot = 1; _expectedUserUpdates: { action: string, name: string, reason: string }[] = []; + _settleWaiters: (() => void)[] = []; constructor( host: Host, @@ -199,13 +205,13 @@ export default class Instance extends lib.Link { if (field === "factorio.shutdown_timeout") { this.server.shutdownTimeoutMs = curr as number * 1000; } else if (field === "factorio.settings") { - this.updateFactorioSettings(curr as any, prev as any).catch(err => { - this.logger.error(`Error updating server settings:\n${err.stack}`); - }).finally(hook); + this._applyWhenRunning( + "server settings", () => this.updateFactorioSettings(curr as any, prev as any) + ); + hook(); } else if (field === "factorio.enable_whitelist") { - this.updateFactorioWhitelist(curr as any).catch(err => { - this.logger.error(`Error updating whitelist:\n${err.stack}`); - }).finally(hook); + this._applyWhenRunning("whitelist", () => this.updateFactorioWhitelist(curr as any)); + hook(); } else { if (field === "factorio.max_concurrent_commands") { this.server.maxConcurrentCommands = curr as number; @@ -556,6 +562,13 @@ end`.replace(/\r?\n/g, " "); notifyStatus(status: Instance["_status"]) { this._status = status; + if (!transitoryStatuses.has(status)) { + const waiters = this._settleWaiters; + this._settleWaiters = []; + for (const resolve of waiters) { + resolve(); + } + } this.sendTo( "controller", new lib.InstanceStatusChangedEvent( @@ -577,6 +590,26 @@ end`.replace(/\r?\n/g, " "); return this._status; } + /** Wait for a transitory status to settle and return the status settled on */ + async _waitForSettledStatus() { + while (transitoryStatuses.has(this._status)) { + await new Promise(resolve => this._settleWaiters.push(resolve)); + } + return this._status; + } + + /** Apply work to the running server, waiting out transitory statuses and skipping it if it stopped */ + async _applyWhenRunning(description: string, fn: () => Promise) { + if (await this._waitForSettledStatus() !== "running") { + return; + } + try { + await fn(); + } catch (err: any) { + this.logger.error(`Error updating ${description}:\n${err.stack}`); + } + } + notifyExit() { this._loadedSave = null; this.notifyStatus("stopped"); @@ -1134,6 +1167,10 @@ end`.replace(/\r?\n/g, " "); } async handleInstanceAdminlistUpdateEvent(request: lib.InstanceAdminlistUpdateEvent) { + if (await this._waitForSettledStatus() !== "running") { + return; + } + const { name, admin } = request; const sync = this.config.get("factorio.sync_adminlist"); if (sync === "disabled") { @@ -1147,6 +1184,10 @@ end`.replace(/\r?\n/g, " "); } async handleInstanceBanlistUpdateEvent(request: lib.InstanceBanlistUpdateEvent) { + if (await this._waitForSettledStatus() !== "running") { + return; + } + const { name, banned, reason } = request; const sync = this.config.get("factorio.sync_banlist"); if (sync === "disabled") { @@ -1160,6 +1201,10 @@ end`.replace(/\r?\n/g, " "); } async handleInstanceWhitelistUpdateEvent(request: lib.InstanceWhitelistUpdateEvent) { + if (await this._waitForSettledStatus() !== "running") { + return; + } + const { name, whitelisted } = request; const sync = this.config.get("factorio.sync_whitelist"); if (this.config.get("factorio.sync_whitelist") === "disabled") { diff --git a/test/host/Instance.js b/test/host/Instance.js index de05589a..6712493d 100644 --- a/test/host/Instance.js +++ b/test/host/Instance.js @@ -49,6 +49,7 @@ describe("class Instance", function() { instance.logger = new MockLogger(); instance.logger.error = message => errors.push(message); instance.server.exampleSettings = async () => ({}); + instance.notifyStatus("running"); hookInvoked = new Promise(resolve => { instance.hooks.instanceConfigFieldChanged.attach("test", field => resolve(field)); }); @@ -86,6 +87,81 @@ describe("class Instance", function() { assert.deepEqual(errors, []); assert.deepEqual(instance.server.rconCommands, ["/config set tags 1 a b"]); }); + + it("should defer changes made while starting until the instance is running", async function() { + instance.notifyStatus("starting"); + instance.config.set("factorio.settings", { tags: ["a"] }); + await wait(10); + assert.deepEqual(instance.server.rconCommands, [], "command sent while starting"); + instance.notifyStatus("running"); + await wait(10); + assert.deepEqual(instance.server.rconCommands, ["/config set tags a"]); + assert.deepEqual(errors, []); + assert.deepEqual(rejections, []); + }); + + it("should drop changes made while starting if the instance stops", async function() { + instance.notifyStatus("starting"); + instance.config.set("factorio.settings", { tags: ["a"] }); + await wait(10); + instance.notifyStatus("stopped"); + await wait(10); + assert.deepEqual(instance.server.rconCommands, []); + assert.deepEqual(errors, []); + assert.deepEqual(rejections, []); + }); + + it("should do nothing for changes made while stopped", async function() { + instance.notifyStatus("stopped"); + instance.config.set("factorio.settings", { tags: ["a"] }); + await wait(10); + assert.deepEqual(instance.server.rconCommands, []); + assert.deepEqual(errors, []); + assert.deepEqual(rejections, []); + }); + + it("should invoke the hook while starting without waiting for the instance", async function() { + instance.notifyStatus("starting"); + instance.config.set("factorio.settings", { tags: ["a"] }); + assert.equal(await hookInvoked, "factorio.settings"); + assert.deepEqual(instance.server.rconCommands, [], "command sent while starting"); + instance.notifyStatus("stopped"); + await wait(10); + }); + }); + + describe("list update events", function() { + let errors; + beforeEach(function() { + errors = []; + instance.logger = new MockLogger(); + instance.logger.error = message => errors.push(message); + }); + + it("should apply an update received while starting once running", async function() { + instance.notifyStatus("starting"); + let handled = instance.handleInstanceAdminlistUpdateEvent( + new lib.InstanceAdminlistUpdateEvent("player", true) + ); + await wait(10); + assert.deepEqual(instance.server.rconCommands, [], "command sent while starting"); + instance.notifyStatus("running"); + await handled; + assert.deepEqual(instance.server.rconCommands, ["/promote player"]); + assert.deepEqual(errors, []); + }); + + it("should drop an update received while starting if the instance stops", async function() { + instance.notifyStatus("starting"); + let handled = instance.handleInstanceAdminlistUpdateEvent( + new lib.InstanceAdminlistUpdateEvent("player", true) + ); + await wait(10); + instance.notifyStatus("stopped"); + await handled; + assert.deepEqual(instance.server.rconCommands, []); + assert.deepEqual(errors, []); + }); }); describe(".checkModPackVersion()", function() { From a76b74229412228b3422c175279cdf76b91b1250 Mon Sep 17 00:00:00 2001 From: bbassie <17990055+bbassie@users.noreply.github.com> Date: Sun, 13 Sep 2026 17:08:53 +0000 Subject: [PATCH 3/3] Invoke the config changed hook for every field The fieldChanged handler repeated the hook invoke in each branch, and factorio.shutdown_timeout had no invoke at all, so plugins were never told about a change to it. Do the field specific side effect, then invoke the hook once for every field. Co-Authored-By: Claude Opus 5 (1M context) --- packages/host/src/Instance.ts | 12 +++--------- test/host/Instance.js | 29 +++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 9 deletions(-) diff --git a/packages/host/src/Instance.ts b/packages/host/src/Instance.ts index 64a03bba..c77cdb92 100644 --- a/packages/host/src/Instance.ts +++ b/packages/host/src/Instance.ts @@ -200,24 +200,18 @@ export default class Instance extends lib.Link { this.hooks = new InstanceHooks(this.logger); this._configFieldChanged = (field: string, curr: unknown, prev: unknown) => { - let hook = () => this.hooks.instanceConfigFieldChanged.invoke(field, curr, prev); - if (field === "factorio.shutdown_timeout") { this.server.shutdownTimeoutMs = curr as number * 1000; + } else if (field === "factorio.max_concurrent_commands") { + this.server.maxConcurrentCommands = curr as number; } else if (field === "factorio.settings") { this._applyWhenRunning( "server settings", () => this.updateFactorioSettings(curr as any, prev as any) ); - hook(); } else if (field === "factorio.enable_whitelist") { this._applyWhenRunning("whitelist", () => this.updateFactorioWhitelist(curr as any)); - hook(); - } else { - if (field === "factorio.max_concurrent_commands") { - this.server.maxConcurrentCommands = curr as number; - } - hook(); } + this.hooks.instanceConfigFieldChanged.invoke(field, curr, prev); }; this.config.on("fieldChanged", this._configFieldChanged); diff --git a/test/host/Instance.js b/test/host/Instance.js index 6712493d..b64658d6 100644 --- a/test/host/Instance.js +++ b/test/host/Instance.js @@ -128,6 +128,35 @@ describe("class Instance", function() { instance.notifyStatus("stopped"); await wait(10); }); + + it("should invoke the hook for fields applied to the server directly", async function() { + instance.config.set("factorio.shutdown_timeout", 30); + assert.equal(await hookInvoked, "factorio.shutdown_timeout"); + assert.equal(instance.server.shutdownTimeoutMs, 30000); + await wait(10); + assert.deepEqual(errors, []); + assert.deepEqual(rejections, []); + }); + + it("should invoke the hook exactly once per field changed", async function() { + let invoked = []; + instance._host.whitelist = new Set(); + instance.hooks.instanceConfigFieldChanged.attach("count", field => { invoked.push(field); }); + instance.config.set("factorio.settings", { tags: ["a"] }); + instance.config.set("factorio.enable_whitelist", true); + instance.config.set("factorio.shutdown_timeout", 30); + instance.config.set("factorio.max_concurrent_commands", 3); + await wait(10); + assert.deepEqual(invoked, [ + "factorio.settings", + "factorio.enable_whitelist", + "factorio.shutdown_timeout", + "factorio.max_concurrent_commands", + ]); + assert.equal(instance.server.maxConcurrentCommands, 3); + assert.deepEqual(errors, []); + assert.deepEqual(rejections, []); + }); }); describe("list update events", function() {