From 56d399767570f3de17aef5c26c027012bbee1bdb Mon Sep 17 00:00:00 2001 From: Hornwitser Date: Fri, 24 Nov 2023 18:15:50 +0100 Subject: [PATCH 1/7] Refactor to keep handle to Instance in connection Keep a handle to the Instance in the connection the host has to the instance so that methods can be called directly on the Instance. This simplifies logic that exists outside of the link system like the host shutting down all of its instances when it exits. Fixes validation error thrown on host shutdown with running instances. --- packages/host/src/Host.ts | 22 +++++++++------------- packages/host/src/InstanceConnection.ts | 8 +++----- 2 files changed, 12 insertions(+), 18 deletions(-) diff --git a/packages/host/src/Host.ts b/packages/host/src/Host.ts index 602d1217..c834b25b 100644 --- a/packages/host/src/Host.ts +++ b/packages/host/src/Host.ts @@ -214,6 +214,9 @@ class HostRouter { ); } logger.warn(`Failed to deliver ${(message as any).name || "message"} (${message.type}): ${err.message}`); + if (err instanceof lib.InvalidMessage && err.errors) { + logger.warn(JSON.stringify(err.errors, null, 4)); + } } } @@ -470,7 +473,7 @@ export default class Host extends lib.Link { let instanceConnection = this.instanceConnections.get(instanceId); this.send( new lib.InstanceStatusChangedEvent( - instanceId, instanceConnection ? instanceConnection.status : "stopped", undefined + instanceId, instanceConnection ? instanceConnection.instance.status : "stopped", undefined ) ); @@ -490,7 +493,7 @@ export default class Host extends lib.Link { let instanceInfo = this.instanceInfos.get(instanceId); if (instanceInfo) { let instanceConnection = this.instanceConnections.get(instanceId); - if (instanceConnection && ["starting", "running"].includes(instanceConnection.status)) { + if (instanceConnection && ["starting", "running"].includes(instanceConnection.instance.status)) { await instanceConnection.send(new lib.InstanceStopRequest()); } @@ -530,10 +533,10 @@ export default class Host extends lib.Link { let hostAddress = new lib.Address(lib.Address.host, this.config.get("host.id")); let instanceAddress = new lib.Address(lib.Address.instance, instanceId); let [connectionClient, connectionServer] = lib.VirtualConnector.makePair(instanceAddress, hostAddress); - let instanceConnection = new InstanceConnection(connectionServer, this, instanceId); let instance = new Instance( this, connectionClient, instanceInfo.path, this.config.get("host.factorio_directory"), instanceInfo.config ); + let instanceConnection = new InstanceConnection(connectionServer, this, instance); this.instanceConnections.set(instanceId, instanceConnection); await instance.init(this.pluginInfos); @@ -750,13 +753,6 @@ export default class Host extends lib.Link { }); } - async stopInstance(instanceId: number) { - let instanceConnection = this.instanceConnections.get(instanceId); - if (instanceConnection) { - await instanceConnection.send(new lib.InstanceStopRequest()); - } - } - async handleInstanceDeleteInternalRequest(request: lib.InstanceDeleteInternalRequest) { let instanceId = request.instanceId; if (this.instanceConnections.has(instanceId)) { @@ -786,7 +782,7 @@ export default class Host extends lib.Link { let instanceConnection = this.instanceConnections.get(instanceId); list.push(new lib.RawInstanceInfo( instanceInfo.config.serialize("controller"), - instanceConnection ? instanceConnection.status : "stopped", + instanceConnection ? instanceConnection.instance.status : "stopped", )); } await this.send(new lib.InstancesUpdateRequest(list)); @@ -828,9 +824,9 @@ export default class Host extends lib.Link { } this._shuttingDown = true; - for (let instanceId of this.instanceConnections.keys()) { + for (let instanceConnection of this.instanceConnections.values()) { try { - await this.stopInstance(instanceId); + await instanceConnection.instance.stop(); } catch (err: any) { logger.error(`Unexpected error stopping instance:\n${err.stack}`); } diff --git a/packages/host/src/InstanceConnection.ts b/packages/host/src/InstanceConnection.ts index 57780263..e9f01053 100644 --- a/packages/host/src/InstanceConnection.ts +++ b/packages/host/src/InstanceConnection.ts @@ -5,12 +5,11 @@ import type Instance from "./Instance"; export default class InstanceConnection extends lib.Link { plugins = new Map(); - status: Instance["status"] = "stopped"; constructor( connector: lib.VirtualConnector, public host: Host, - public instanceId: number + public instance: Instance, ) { super(connector); this.router = this.host.router; @@ -24,9 +23,8 @@ export default class InstanceConnection extends lib.Link { } async snoopInstanceStatusChangedEvent(event: lib.InstanceStatusChangedEvent) { - this.status = event.status as Instance["status"]; - if (this.status === "stopped") { - this.host.instanceConnections.delete(this.instanceId); + if (this.instance.status === "stopped") { + this.host.instanceConnections.delete(this.instance.id); } } } From 142cf8a626bee48b69c2a252f386fd9993e5c695 Mon Sep 17 00:00:00 2001 From: Hornwitser Date: Fri, 24 Nov 2023 18:49:49 +0100 Subject: [PATCH 2/7] Fix log splitting not happening on UTC boundnary --- packages/controller/controller.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/controller/controller.ts b/packages/controller/controller.ts index 2d9a849f..726e7c11 100644 --- a/packages/controller/controller.ts +++ b/packages/controller/controller.ts @@ -260,6 +260,7 @@ async function initialize(): Promise { levels, }); clusterLogger.add(new DailyRotateFile({ + format: winston.format.json(), filename: "cluster-%DATE%.log", utc: true, dirname: path.join(args.logDirectory, "cluster"), @@ -269,6 +270,7 @@ async function initialize(): Promise { logger.add(new DailyRotateFile({ format: winston.format.json(), filename: "controller-%DATE%.log", + utc: true, dirname: path.join(args.logDirectory, "controller"), })); logger.add(new winston.transports.Stream({ From 6eda1f78d5ef8b0d91e9ecd79e01f5203cc836be Mon Sep 17 00:00:00 2001 From: Hornwitser Date: Sun, 19 Nov 2023 05:14:17 +0100 Subject: [PATCH 3/7] Turn on --enable-source-maps Turn on source map deduction in stack traces from thrown errors. This feature is off by default due to potential performance degration in certain corner cases. For the dev environment add instructions for passing the option to node. --- docs/contributing.md | 2 ++ packages/create/create.js | 16 +++++++++++++--- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/docs/contributing.md b/docs/contributing.md index d8729a33..34ac895d 100644 --- a/docs/contributing.md +++ b/docs/contributing.md @@ -72,6 +72,8 @@ The documentation uses the binaries provided by the packages, but for the develo npx clusteriohost -> node packages/host npx clusterioctl -> node packages/ctl +By default node does not enable source maps, to get stack traces pointing to the `.ts` files you will need to pass `--enable-source-maps` to `node` or set the environment variable `NODE_OPTIONS=--enable-source-maps`. + Note that to use `clusterioctl` you will have to create a control config first: node packages/controller bootstrap create-ctl-config [admin-user] diff --git a/packages/create/create.js b/packages/create/create.js index cacb19ef..4c252f93 100755 --- a/packages/create/create.js +++ b/packages/create/create.js @@ -375,17 +375,26 @@ async function writeScripts(mode) { if (process.platform === "win32") { await safeOutputFile( "run-controller.cmd", - "@echo off\n.\\node_modules\\.bin\\clusteriocontroller.cmd run\n" + `\ +@echo off. +set "NODE_OPTIONS=--enable-source-maps %NODE_OPTIONS%" +\\node_modules\\.bin\\clusteriocontroller.cmd run +` ); } else { await safeOutputFile( "run-controller.sh", - "#!/bin/sh\nexec ./node_modules/.bin/clusteriocontroller run\n", + `\ +#!/bin/sh +export "NODE_OPTIONS=--enable-source-maps $NODE_OPTIONS" +exec ./node_modules/.bin/clusteriocontroller run +`, { mode: 0o755 }, ); await safeOutputFile( "systemd/clusteriocontroller.service", - `[Unit] + `\ +[Unit] Description=Clusterio Controller [Service] @@ -394,6 +403,7 @@ Group=${await groupIdToName(os.userInfo().gid)} WorkingDirectory=${process.cwd()} KillMode=mixed KillSignal=SIGINT +Environment=NODE_OPTIONS=--enable-source-maps ExecStart=${process.cwd()}/node_modules/.bin/clusteriocontroller run --log-level=warn [Install] From f23e442c4c115e6929c48e1299988c9ee56a5331 Mon Sep 17 00:00:00 2001 From: Hornwitser Date: Mon, 20 Nov 2023 15:57:51 +0100 Subject: [PATCH 4/7] Ignore BOM in locale files Ignore BOM at the start of Factorio locale files as this is a thing Factorio also does. Fixes #507 --- packages/lib/src/ini.ts | 5 +++++ test/file/bom.cfg | 2 ++ test/lib/ini.js | 7 +++++++ 3 files changed, 14 insertions(+) create mode 100644 test/file/bom.cfg diff --git a/packages/lib/src/ini.ts b/packages/lib/src/ini.ts index 592d7fb1..ecf11052 100644 --- a/packages/lib/src/ini.ts +++ b/packages/lib/src/ini.ts @@ -11,6 +11,11 @@ * @returns sections parsed from the ini file. */ export function parse(input: string) { + // Ignore BOM at the start of the file + if (input[0] === "\uFEFF") { + input = input.slice(1); + } + const sections: Record> = {}; let currentSection: Record = sections as Record; const lines = input.split(/\r\n|\n/); diff --git a/test/file/bom.cfg b/test/file/bom.cfg new file mode 100644 index 00000000..e559eb73 --- /dev/null +++ b/test/file/bom.cfg @@ -0,0 +1,2 @@ +[spam] +foo=bar diff --git a/test/lib/ini.js b/test/lib/ini.js index c7cd39f5..6b0d9697 100644 --- a/test/lib/ini.js +++ b/test/lib/ini.js @@ -1,5 +1,6 @@ "use strict"; const assert = require("assert").strict; +const fs = require("fs-extra"); const lib = require("@clusterio/lib"); describe("lib/ini", function() { @@ -58,6 +59,12 @@ describe("lib/ini", function() { new Error("Duplicated key key on line 2") ); }); + it("should parse a file with BOM", async function() { + assert.deepEqual( + lib.parse(await fs.readFile("test/file/bom.cfg", "utf8")), + { spam: { foo: "bar" }}, + ); + }); }); describe("stringify()", function() { it("should serialize to reference output", function() { From fc6e9d000259235fed25eeacecdce0e6715a0ba7 Mon Sep 17 00:00:00 2001 From: Hornwitser Date: Fri, 24 Nov 2023 19:05:30 +0100 Subject: [PATCH 5/7] Fix type checking on Request and Event types When defining a generic type where a type argument is never used as a part of the type itself, TypeScript will consider two instances of the generic type with different values for the type to be the same. This caused the handle method to accept any handler for any Request/Event, ignoring the event/request type completely. Fix by specifying the constructor to return the intersection of the type itself and the Request/Event type. --- packages/lib/src/link/link.ts | 8 ++++---- packages/lib/src/subscriptions.ts | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/lib/src/link/link.ts b/packages/lib/src/link/link.ts index 998e3a63..ebc5cecf 100644 --- a/packages/lib/src/link/link.ts +++ b/packages/lib/src/link/link.ts @@ -11,8 +11,8 @@ import type { User } from "../users"; import type { AddressType, JSONDeserialisable, MessageRoutable, MessageRequest, MessageEvent } from "../data"; export interface Request { - constructor: Partial>> & { - new (...args: any): Request, + constructor: Partial>> & { + new (...args: any): Req & Request, name: string, type: "request"; src: AddressType | readonly AddressType[]; @@ -25,8 +25,8 @@ export interface Request { export type RequestClass = Request["constructor"]; export interface Event { - constructor: Partial>> & { - new (...args: any): Event, + constructor: Partial>> & { + new (...args: any): T & Event, name: string, type: "event"; src: AddressType | readonly AddressType[]; diff --git a/packages/lib/src/subscriptions.ts b/packages/lib/src/subscriptions.ts index eb82e02e..ab7b4c7f 100644 --- a/packages/lib/src/subscriptions.ts +++ b/packages/lib/src/subscriptions.ts @@ -215,7 +215,7 @@ export class SubscriptionController { * Channels are an optional feature which can be implemented to only get notifications of reliant updates * Multiple handlers can be subscribed at the same time, on the same or different channels */ -export class EventSubscriber { +export class EventSubscriber, V=T> { _callbacks = new Array>(); _channelCallbacks = new Map>>(); lastResponse: Event | null = null; @@ -365,7 +365,7 @@ export class EventSubscriber { )); if (response.eventReplay) { - await this._handle(response.eventReplay); + await this._handle(response.eventReplay as any); } } } From 51f7430a4b79d0ed1c9038e69b5585e650c6180f Mon Sep 17 00:00:00 2001 From: Hornwitser Date: Fri, 24 Nov 2023 21:02:15 +0100 Subject: [PATCH 6/7] Add warning to generated server-settings.json Leave a warning in case someone opens and tries to modify the server-settings.json file in the instance folder, as any changes made will be lost when it's overwritten. --- packages/host/src/Instance.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/host/src/Instance.ts b/packages/host/src/Instance.ts index dd9e7e21..50965248 100644 --- a/packages/host/src/Instance.ts +++ b/packages/host/src/Instance.ts @@ -592,7 +592,11 @@ rcon.print(game.table_to_json(players))`.replace(/\r?\n/g, " "); * factorio_settings config entry. */ async writeServerSettings() { - let serverSettings = await this.resolveServerSettings(this.config.get("factorio.settings")); + const warning = "Changes to this file will be overwitten by the factorio.settings config on the instance."; + const serverSettings = { + "_comment_warning": warning, + ...await this.resolveServerSettings(this.config.get("factorio.settings")), + }; await lib.safeOutputFile( this.server.writePath("server-settings.json"), JSON.stringify(serverSettings, null, 4) From d47fd20926b3d44ed0ab2a7333cfed2dacf989d2 Mon Sep 17 00:00:00 2001 From: Hornwitser Date: Fri, 24 Nov 2023 21:11:56 +0100 Subject: [PATCH 7/7] Fix --dev broken after 7517917 --- packages/controller/src/Controller.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/controller/src/Controller.ts b/packages/controller/src/Controller.ts index 29249626..580c02e8 100644 --- a/packages/controller/src/Controller.ts +++ b/packages/controller/src/Controller.ts @@ -265,7 +265,7 @@ export default class Controller { if (args.dev) { // eslint-disable-next-line node/no-missing-require - webpackConfigs.push(require("../../../webpack.config")({})); // Path outside of build + webpackConfigs.push(require("../../webpack.config")({})); // Path outside of build } if (args.devPlugin) { let devPlugins = new Map();