Sets restart required when a plugin is updated (#794)

* Template type for load plugin config

* Add plugin update checking

* Add restart icon to instance list

* Add tests

* Log package name when failing to read package json

packages/controller/src/ControlConnection.ts

@@ -1079,9 +1079,7 @@ export default class ControlConnection extends BaseConnection {
 		return this._controller.pluginInfos.map(pluginInfo => lib.PluginDetails.fromNodeEnvInfo(
 			pluginInfo,
 			this._controller.plugins.has(pluginInfo.name),
-			this._controller.config.get(
-				`${pluginInfo.name}.load_plugin` as keyof lib.ControllerConfigFields
-			) as boolean,
+			this._controller.config.get(`${pluginInfo.name}.load_plugin`),
 		));
 	}
 

packages/controller/src/Controller.ts

@@ -549,19 +549,8 @@ export default class Controller {
 				}
 				requests.push(hostConnection.send(new lib.SystemInfoRequest()));
 			}
-			if (!this.config.restartRequired) {
-				// If a restart isn't already required, then test if a new version is installed
-				try {
-					const runningVersion = this.config.get("controller.version");
-					const packageJson = await fs.readJSON(path.join(__dirname, "..", "package.json"));
-					if (runningVersion !== packageJson.version) {
-						this.config.restartRequired = true;
-					}
-				} catch (err: any) {
-					logger.warn(`Failed to read package json:\n${err.stack ?? err.message}`);
-				}
-			}
-			requests.push(lib.gatherSystemInfo("controller", this.canRestart, this.config.restartRequired));
+			const restartRequired = await this.checkRestartRequired();
+			requests.push(lib.gatherSystemInfo("controller", this.canRestart, restartRequired));
 			const newMetrics = await Promise.all(requests);
 			for (const metric of newMetrics) {
 				this.systems.set(metric);
@@ -571,6 +560,44 @@ export default class Controller {
 		}
 	}
 
+	async checkRestartRequired() {
+		if (this.config.restartRequired) {
+			return true;
+		}
+
+		let packageName = "@clusterio/controller";
+		try {
+			// First check the clusterio version
+			const runningVersion = this.config.get("controller.version");
+			const packageJson = await fs.readJSON(path.join(__dirname, "..", "package.json"));
+			if (runningVersion !== packageJson.version) {
+				this.config.restartRequired = true;
+				return true;
+			}
+
+			// Second check plugin versions
+			for (const pluginInfo of this.pluginInfos) {
+				if (
+					!pluginInfo.controllerEntrypoint && !pluginInfo.webEntrypoint
+					|| !this.config.get(`${pluginInfo.name}.load_plugin`)
+				) {
+					continue;
+				}
+
+				packageName = pluginInfo.npmPackage ?? pluginInfo.name;
+				const pluginPackageJson = await fs.readJSON(path.join(pluginInfo.requirePath, "package.json"));
+				if (pluginInfo.version !== pluginPackageJson.version) {
+					this.config.restartRequired = true;
+					return true;
+				}
+			}
+		} catch (err: any) {
+			logger.warn(`Failed to read package json for ${packageName}:\n${err.stack ?? err.message}`);
+		}
+
+		return false;
+	}
+
 	onAutosaveIntervalChanged() {
 		if (this.autosaveInterval) {
 			clearInterval(this.autosaveInterval);
@@ -1127,7 +1154,7 @@ export default class Controller {
 				logger.warn(`Unable to load dist/web/manifest.json for plugin ${pluginInfo.name}`);
 			}
 
-			if (!this.config.get(`${pluginInfo.name}.load_plugin` as keyof lib.ControllerConfigFields)) {
+			if (!this.config.get(`${pluginInfo.name}.load_plugin`)) {
 				continue;
 			}
 

packages/controller/src/routes.ts

@@ -109,9 +109,7 @@ function getPlugins(req: Request, res: Response) {
 	for (let pluginInfo of req.app.locals.controller.pluginInfos) {
 		let name = pluginInfo.name;
 		let loaded = req.app.locals.controller.plugins.has(name);
-		let enabled = loaded && req.app.locals.controller.config.get(
-			`${pluginInfo.name}.load_plugin` as keyof lib.ControllerConfigFields,
-		) as unknown as boolean;
+		let enabled = loaded && req.app.locals.controller.config.get(`${pluginInfo.name}.load_plugin`);
 		// Note: Cast through unknown is needed because load_plugin is
 		// defined at runtime and no other fields in the controller config
 		// currently have the boolean type.

packages/host/src/Host.ts

@@ -410,7 +410,7 @@ export default class Host extends lib.Link {
 		for (let pluginInfo of this.pluginInfos) {
 			if (
 				!pluginInfo.hostEntrypoint && !pluginInfo.instanceEntrypoint
-				|| !this.config.get(`${pluginInfo.name}.load_plugin` as keyof lib.HostConfigFields)
+				|| !this.config.get(`${pluginInfo.name}.load_plugin`)
 			) {
 				continue;
 			}
@@ -805,19 +805,46 @@ export default class Host extends lib.Link {
 	}
 
 	async handleSystemInfoRequest() {
-		if (!this.config.restartRequired) {
-			// If a restart isn't already required, then test if a new version is installed
-			try {
-				const runningVersion = this.config.get("host.version");
-				const packageJson = await fs.readJSON(path.join(__dirname, "..", "package.json"));
-				if (runningVersion !== packageJson.version) {
-					this.config.restartRequired = true;
-				}
-			} catch (err: any) {
-				logger.warn(`Failed to read package json:\n${err.stack ?? err.message}`);
-			}
+		const restartRequired = await this.checkRestartRequired();
+		return lib.gatherSystemInfo(this.config.get("host.id"), this.canRestart, restartRequired);
+	}
+
+	async checkRestartRequired() {
+		if (this.config.restartRequired) {
+			return true;
 		}
-		return lib.gatherSystemInfo(this.config.get("host.id"), this.canRestart, this.config.restartRequired);
+
+		let packageName = "@clusterio/host";
+		try {
+			// First check the clusterio version
+			const runningVersion = this.config.get("host.version");
+			const packageJson = await fs.readJSON(path.join(__dirname, "..", "package.json"));
+			if (runningVersion !== packageJson.version) {
+				this.config.restartRequired = true;
+				return true;
+			}
+
+			// Second check plugin versions
+			for (const pluginInfo of this.pluginInfos) {
+				if (
+					!pluginInfo.hostEntrypoint && !pluginInfo.instanceEntrypoint
+					|| !this.config.get(`${pluginInfo.name}.load_plugin`)
+				) {
+					continue;
+				}
+
+				packageName = pluginInfo.npmPackage ?? pluginInfo.name;
+				const pluginPackageJson = await fs.readJSON(path.join(pluginInfo.requirePath, "package.json"));
+				if (pluginInfo.version !== pluginPackageJson.version) {
+					this.config.restartRequired = true;
+					return true;
+				}
+			}
+		} catch (err: any) {
+			logger.warn(`Failed to read package json for ${packageName}:\n${err.stack ?? err.message}`);
+		}
+
+		return false;
 	}
 
 	async handleHostMetricsRequest() {
@@ -1087,7 +1114,7 @@ export default class Host extends lib.Link {
 		return this.pluginInfos.map(pluginInfo => lib.PluginDetails.fromNodeEnvInfo(
 			pluginInfo,
 			this.plugins.has(pluginInfo.name),
-			this.config.get(`${pluginInfo.name}.load_plugin` as keyof lib.HostConfigFields) as boolean,
+			this.config.get(`${pluginInfo.name}.load_plugin`),
 		));
 	}
 

packages/host/src/Instance.ts

@@ -651,7 +651,7 @@ end`.replace(/\r?\n/g, " ");
 			if (
 				!pluginInfo.instanceEntrypoint
 				|| !this._host.serverPlugins.has(pluginInfo.name)
-				|| !this.config.get(`${pluginInfo.name}.load_plugin` as keyof lib.InstanceConfigFields)
+				|| !this.config.get(`${pluginInfo.name}.load_plugin`)
 			) {
 				continue;
 			}

packages/lib/src/config/definitions.ts

@@ -35,6 +35,7 @@ export interface ControllerConfigFields {
 	"controller.allow_remote_updates": boolean;
 	"controller.allow_plugin_updates": boolean;
 	"controller.allow_plugin_install": boolean;
+	[key: `${string}.load_plugin`]: boolean;
 }
 
 /**
@@ -267,6 +268,7 @@ export interface HostConfigFields {
 	"host.allow_remote_updates": boolean;
 	"host.allow_plugin_updates": boolean;
 	"host.allow_plugin_install": boolean;
+	[key: `${string}.load_plugin`]: boolean;
 }
 
 /**
@@ -399,6 +401,7 @@ export interface InstanceConfigFields {
 	"instance.assigned_host": number | null;
 	"instance.auto_start": boolean;
 	"instance.exclude_from_start_all": boolean;
+	[key: `${string}.load_plugin`]: boolean;
 
 	"factorio.version": string;
 	"factorio.executable_path": string | null;

packages/web_ui/src/components/ControllerPage.tsx

@@ -34,9 +34,10 @@ function ControllerControlButton({ canRestart, restartRequired }: ControllerCont
 		actions.push({
 			key: "restart",
 			label: (
-				<Tooltip title={restartRequired ? "Restart Required" : null}>
-					Restart {restartRequired ? <ExclamationCircleOutlined style={{ color: "yellow" }}/> : undefined}
-				</Tooltip>
+				!restartRequired ? "Restart" : <Tooltip title={"Restart Required"}><Space>
+					<ExclamationCircleOutlined style={{ color: "yellow" }}/>
+					Restart
+				</Space></Tooltip>
 			),
 			onClick: () => {
 				control.send(

packages/web_ui/src/components/HostViewPage.tsx

@@ -37,9 +37,10 @@ function HostControlButton({ hostId, canRestart, restartRequired }: HostControlB
 		actions.push({
 			key: "restart",
 			label: (
-				<Tooltip title={restartRequired ? "Restart Required" : null}>
-					Restart {restartRequired ? <ExclamationCircleOutlined style={{ color: "yellow" }}/> : undefined}
-				</Tooltip>
+				!restartRequired ? "Restart" : <Tooltip title={"Restart Required"}><Space>
+					<ExclamationCircleOutlined style={{ color: "yellow" }}/>
+					Restart
+				</Space></Tooltip>
 			),
 			onClick: () => {
 				control.sendTo(

packages/web_ui/src/components/InstanceList.tsx

@@ -1,12 +1,14 @@
 import React from "react";
 import { useNavigate } from "react-router-dom";
-import { message, Button, Table } from "antd";
+import { message, Button, Table, Space } from "antd";
 import CopyOutlined from "@ant-design/icons/CopyOutlined";
 import type { SizeType } from "antd/es/config-provider/SizeContext";
 import type { ColumnsType } from "antd/es/table";
 
 import { useAccount } from "../model/account";
+import { useSystems } from "../model/system";
 import { useHosts } from "../model/host";
+import { RestartRequired } from "./system_metrics";
 import InstanceStatusTag from "./InstanceStatusTag";
 import InstanceControlButton, { InstanceControlButtonPermissions } from "./InstanceControlButton";
 import * as lib from "@clusterio/lib";
@@ -25,6 +27,7 @@ export default function InstanceList(props: InstanceListProps) {
 	let account = useAccount();
 	let navigate = useNavigate();
 	let [hosts] = useHosts();
+	const [systems] = useSystems();
 
 	function hostName(hostId?: number) {
 		if (hostId === undefined) {
@@ -50,12 +53,15 @@ export default function InstanceList(props: InstanceListProps) {
 		{
 			title: "Assigned Host",
 			key: "assignedHost",
-			render: (_, instance) => <Link
-				to={`/hosts/${instance.assignedHost}/view`}
-				onClick={e => e.stopPropagation()}
-			>
-				{hostName(instance.assignedHost)}
-			</Link>,
+			render: (_, instance) => <Space>
+				<Link
+					to={`/hosts/${instance.assignedHost}/view`}
+					onClick={e => e.stopPropagation()}
+				>
+					{hostName(instance.assignedHost)}
+				</Link>
+				<RestartRequired system={instance.assignedHost ? systems.get(instance.assignedHost) : undefined}/>
+			</Space>,
 			sorter: (a, b) => strcmp(hostName(a.assignedHost), hostName(b.assignedHost)),
 			responsive: ["sm"],
 		},

test/controller/Controller.js

@@ -1,4 +1,5 @@
 "use strict";
+const path = require("node:path");
 const assert = require("assert").strict;
 const { Controller, HostInfo, InstanceInfo } = require("@clusterio/controller");
 const { ControllerConfig, Address, RequestError, InstanceConfig, SystemInfo, Role } = require("@clusterio/lib");
@@ -27,8 +28,10 @@ describe("controller/src/Controller", function() {
 	describe("class Controller", function() {
 		/** @type {Controller} */
 		let controller, mockInstanceConfig;
+		const controllerVersion = require("@clusterio/controller/package.json").version;
 		before(async function() {
-			controller = new Controller({}, [], new ControllerConfig("controller"));
+			const controllerConfig = new ControllerConfig("controller", { "controller.version": controllerVersion });
+			controller = new Controller({}, [], controllerConfig);
 			mockInstanceConfig = new MockInstanceConfig(new Map([
 				["instance.id", 100], ["instance.name", "test"], ["factorio.settings", []],
 			]));
@@ -233,6 +236,62 @@ describe("controller/src/Controller", function() {
 				assert(instanceInfo.updatedAtMs === 0, "updatedAtMs incremented");
 			});
 		});
+		describe(".checkRestartRequired()", function() {
+			beforeEach(function() {
+				// Would not be needed if controller setup was beforeEach
+				controller.config.restartRequired = false;
+				controller.config.set("controller.version", controllerVersion);
+				controller.config.set("global_chat.load_plugin", true);
+				controller.pluginInfos[0] = {
+					name: "global_chat",
+					requirePath: path.dirname(require.resolve("@clusterio/controller/package.json")),
+					version: controllerVersion,
+				};
+			});
+			it("returns false when no changes are present", async function() {
+				controller.pluginInfos[0].webEntrypoint = true;
+				controller.pluginInfos[0].controllerEntrypoint = true;
+				const result = await controller.checkRestartRequired();
+				assert.equal(result, false);
+				assert.equal(controller.config.restartRequired, false);
+			});
+			it("returns false when an unloaded plugin version changes", async function() {
+				controller.pluginInfos[0].version = "0.0.0";
+				controller.pluginInfos[0].webEntrypoint = true;
+				controller.pluginInfos[0].controllerEntrypoint = true;
+				controller.config.set("global_chat.load_plugin", false);
+				controller.config.restartRequired = false; // Setting load_plugin requires a restart
+				const result = await controller.checkRestartRequired();
+				assert.equal(result, false);
+				assert.equal(controller.config.restartRequired, false);
+			});
+			it("returns true when the config flag is set", async function() {
+				controller.config.restartRequired = true;
+				const result = await controller.checkRestartRequired();
+				assert.equal(result, true);
+				assert.equal(controller.config.restartRequired, true);
+			});
+			it("returns true when clusterio version changes", async function() {
+				controller.config.set("controller.version", "0.0.0");
+				const result = await controller.checkRestartRequired();
+				assert.equal(result, true);
+				assert.equal(controller.config.restartRequired, true);
+			});
+			it("returns true when a controller plugin version changes", async function() {
+				controller.pluginInfos[0].version = "0.0.0";
+				controller.pluginInfos[0].controllerEntrypoint = true;
+				const result = await controller.checkRestartRequired();
+				assert.equal(result, true);
+				assert.equal(controller.config.restartRequired, true);
+			});
+			it("returns true when a web plugin version changes", async function() {
+				controller.pluginInfos[0].version = "0.0.0";
+				controller.pluginInfos[0].webEntrypoint = true;
+				const result = await controller.checkRestartRequired();
+				assert.equal(result, true);
+				assert.equal(controller.config.restartRequired, true);
+			});
+		});
 	});
 	describe("migrations", function() {
 		describe("SystemInfo", function() {

test/host.js

@@ -6,6 +6,7 @@ const lib = require("@clusterio/lib");
 const Host = require("@clusterio/host/dist/node/src/Host").default;
 const Instance = require("@clusterio/host/dist/node/src/Instance").default;
 const { _discoverInstances } = require("@clusterio/host/dist/node/src/Host");
+const { HostConnector } = require("@clusterio/host/dist/node/host");
 
 describe("Host testing", function() {
 	describe("discoverInstances()", function() {
@@ -115,5 +116,63 @@ describe("Host testing", function() {
 				]);
 			});
 		});
+		describe(".checkRestartRequired()", function() {
+			let host;
+			beforeEach(function() {
+				// This can be used more generally, but i did not want to interfere with handleSyncUserListsEvent
+				const hostVersion = require("@clusterio/host/package.json").version;
+				const pluginInfos = [{
+					name: "global_chat",
+					requirePath: path.dirname(require.resolve("@clusterio/host/package.json")),
+					version: hostVersion,
+				}];
+				const hostConfig = new lib.HostConfig("host", { "host.version": hostVersion });
+				const hostConnector = new HostConnector(hostConfig, undefined, pluginInfos);
+				host = new Host(hostConnector, hostConfig, undefined, pluginInfos);
+			});
+			it("returns false when no changes are present", async function() {
+				host.pluginInfos[0].hostEntrypoint = true;
+				host.pluginInfos[0].instanceEntrypoint = true;
+				const result = await host.checkRestartRequired();
+				assert.equal(result, false);
+				assert.equal(host.config.restartRequired, false);
+			});
+			it("returns false when an unloaded plugin version changes", async function() {
+				host.pluginInfos[0].version = "0.0.0";
+				host.pluginInfos[0].hostEntrypoint = true;
+				host.pluginInfos[0].instanceEntrypoint = true;
+				host.config.set("global_chat.load_plugin", false);
+				host.config.restartRequired = false; // Setting load_plugin requires a restart
+				const result = await host.checkRestartRequired();
+				assert.equal(result, false);
+				assert.equal(host.config.restartRequired, false);
+			});
+			it("returns true when the config flag is set", async function() {
+				host.config.restartRequired = true;
+				const result = await host.checkRestartRequired();
+				assert.equal(result, true);
+				assert.equal(host.config.restartRequired, true);
+			});
+			it("returns true when clusterio version changes", async function() {
+				host.config.set("host.version", "0.0.0");
+				const result = await host.checkRestartRequired();
+				assert.equal(result, true);
+				assert.equal(host.config.restartRequired, true);
+			});
+			it("returns true when a host plugin version changes", async function() {
+				host.pluginInfos[0].version = "0.0.0";
+				host.pluginInfos[0].hostEntrypoint = true;
+				const result = await host.checkRestartRequired();
+				assert.equal(result, true);
+				assert.equal(host.config.restartRequired, true);
+			});
+			it("returns true when an instance plugin version changes", async function() {
+				host.pluginInfos[0].version = "0.0.0";
+				host.pluginInfos[0].instanceEntrypoint = true;
+				const result = await host.checkRestartRequired();
+				assert.equal(result, true);
+				assert.equal(host.config.restartRequired, true);
+			});
+		});
 	});
 });