Write plugin versions while loading configs

The staging copy made by Config.set() went through the constructor, so
plugin migrations ran a second time on the live fields. A version-gated
migration was applied twice when setPluginVersions() changed the version.
Staging now copies the fields with update() instead.

A config saved without <plugin>.version got the running version as its
default, so setPluginVersions() saw no change and nothing saved the stamp.
The constructor now writes the registered plugin versions after the
migrations run and marks the config dirty when that changes a loaded value.
Controller.bootstrap() saves the instances that came out dirty, and
setPluginVersions() is gone.

Restructure the SyncHook and plugin config tests by method, and suggest the
controller config version for role migrations in the docs.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

docs/writing-plugins.md

@@ -357,8 +357,28 @@ hooks.instanceConfig.attach(plugin.name, config => {
 });
 ```
 
-The version field is missing in configs saved before the plugin was loaded with Clusterio versions that track it.
-Roles have no version.
+The version field is missing or empty in configs saved before Clusterio tracked it, and in configs last loaded in recovery mode.
+
+Roles have no version of their own.
+They are stored on the controller, so use the version in the controller config, which is loaded before the roles:
+
+```js
+let controllerVersion;
+hooks.controllerConfig.attach(plugin.name, config => {
+    controllerVersion ??= config["foo_frobber.version"] ?? "";
+    return config;
+});
+hooks.role.attach(plugin.name, role => {
+    // foo_frobber.edit was split out of foo_frobber.view in 1.2.0
+    if (
+        controllerVersion && lib.integerFullVersion(controllerVersion) < lib.integerFullVersion("1.2.0")
+        && role.permissions.includes("foo_frobber.view")
+    ) {
+        role.permissions.push("foo_frobber.edit");
+    }
+    return role;
+});
+```
 
 Data the plugin stores itself, like files in the database directory or values in the save, is up to the plugin to migrate from its other entrypoints.
 Modules don't need migrating, save patching replaces the files of every module when an instance starts.

packages/controller/controller.ts

@@ -430,9 +430,6 @@ async function initialize(): Promise<InitializeParameters> {
 	);
 
 	controllerConfig.set("controller.version", packageConfig.version); // Allows tracking last loaded version
-	if (!args.recovery) {
-		lib.setPluginVersions(controllerConfig, pluginInfos);
-	}
 
 	if (!controllerConfig.get("controller.auth_secret")) {
 		logger.info("Generating new controller authentication secret");
@@ -483,7 +480,7 @@ async function startup() {
 		controllerConfig,
 		Boolean(args.canRestart),
 		Boolean(args.recovery),
-		...await Controller.bootstrap(controllerConfig, args.recovery ? [] : pluginInfos)
+		...await Controller.bootstrap(controllerConfig)
 	);
 
 	// Refuse to start if there are no users loaded

packages/controller/src/Controller.ts

@@ -148,15 +148,7 @@ export default class Controller {
 		}
 	});
 
-	/**
-	 * Load the controller datastores
-	 *
-	 * @param config - Controller config.
-	 * @param pluginInfos -
-	 *     Plugins whose migrations ran, their versions are saved to the
-	 *     instance configs.
-	 */
-	static async bootstrap(config: lib.ControllerConfig, pluginInfos: lib.PluginNodeEnvInfo[] = []) {
+	static async bootstrap(config: lib.ControllerConfig) {
 		let databaseDirectory = config.get("controller.database_directory");
 		await fs.mkdir(databaseDirectory, { recursive: true });
 
@@ -183,8 +175,9 @@ export default class Controller {
 			InstanceRecord.fromJSON.bind(InstanceRecord),
 			this.migrateInstances, this.finaliseInstances,
 		).bootstrap());
+		// Save plugin versions written while loading
 		for (const instance of instances.values()) {
-			if (lib.setPluginVersions(instance.config, pluginInfos)) {
+			if (instance.config.dirty) {
 				instances.set(instance);
 			}
 		}

packages/host/host.ts

@@ -188,9 +188,6 @@ async function startHost() {
 	);
 
 	hostConfig.set("host.version", packageConfig.version); // Allows tracking last loaded version
-	if (!args.recovery) {
-		lib.setPluginVersions(hostConfig, pluginInfos);
-	}
 
 	if (command === "config") {
 		await lib.handleConfigCommand(args, hostConfig, hostConfigLock);

packages/lib/src/config/classes.ts

@@ -231,6 +231,12 @@ export class Config<
 	 */
 	declare static pluginMigrations?: SyncHook<ConfigSchema>;
 
+	/**
+	 * Versions of the plugins whose migrations are attached, keyed by
+	 * plugin name. Written to `<plugin>.version` after the migrations run.
+	 */
+	static pluginVersions = new Map<string, string>();
+
 	/** Defines the default access locations for the config class */
 	static defaultAccess: ConfigLocation[] = ["controller", "host", "control"];
 
@@ -241,10 +247,15 @@ export class Config<
 	/** Set to true during validators to prevent edits */
 	private _readonly = false; // TODO replace with "using config.asReadonly()"
 	/** Set to true when a field in the config is changed. */
-	private dirty = false;
+	private _dirty = false;
 	/** Set to true when a 'restart required' field in the config is changed. */
 	restartRequired = false;
 
+	/** True if the config has changes that have not been saved. */
+	get dirty() {
+		return this._dirty;
+	}
+
 	/**
 	 * Create a new instance of the given config
 	 *
@@ -282,6 +293,16 @@ export class Config<
 			}
 			this.update(migrated, false, location);
 		}
+
+		for (const [pluginName, version] of this.constructor.pluginVersions) {
+			const name = `${pluginName}.version` as keyof Fields & string;
+			if (name in this.fields && this.fields[name] !== version) {
+				this.fields[name] = version as Fields[typeof name];
+				if (fields) {
+					this._dirty = true;
+				}
+			}
+		}
 	}
 
 	get filepath() {
@@ -289,7 +310,7 @@ export class Config<
 	}
 
 	set filepath(filepath: string | undefined) {
-		this.dirty = true;
+		this._dirty = true;
 		this._filepath = filepath;
 	}
 
@@ -300,7 +321,9 @@ export class Config<
 	// TODO replace with "using config.startStating()"
 	get staging(): Config<Fields> {
 		if (this._staging) { return this._staging.config; }
-		const config = new this.constructor(this.location, this.fields);
+		// Copied with update so migrations don't run again on the live fields
+		const config = new this.constructor(this.location);
+		config.update(this.fields, false);
 		const changes = new Set<string>();
 		config.on("fieldChanged", name => changes.add(name));
 		this._staging = { config, changes };
@@ -454,7 +477,7 @@ export class Config<
 			if (def.restartRequired) {
 				this.restartRequired = true;
 			}
-			this.dirty = true;
+			this._dirty = true;
 			this.emit("fieldChanged", name, value, prev);
 		}
 	}
@@ -476,14 +499,14 @@ export class Config<
 	 * @param force - Set to true to save the file even if no changes have been made.
 	 */
 	async save(force = false) {
-		if (!force && !this.dirty) {
+		if (!force && !this._dirty) {
 			return;
 		}
 		if (!this.filepath) {
 			throw new Error("Cannot save config which has no filepath");
 		}
 		await safeOutputFile(this.filepath, JSON.stringify(this, null, "\t"), { mode: 0o600 });
-		this.dirty = false;
+		this._dirty = false;
 	}
 
 	static jsonSchema = ConfigSchema;

packages/lib/src/config/definitions.ts

@@ -812,10 +812,10 @@ export function addPluginFieldDefinitions(
 
 	Config.fieldDefinitions[`${pluginInfo.name}.load_plugin`] = fieldDef;
 
-	// Version of the plugin the config was last loaded with, see setPluginVersions
+	// Version of the plugin the config was last loaded with, see Config.pluginVersions
 	Config.fieldDefinitions[`${pluginInfo.name}.version`] = {
 		type: "string",
-		initialValue: "version" in pluginInfo ? pluginInfo.version : "",
+		initialValue: "",
 		readonly: ["controller", "host"],
 		hidden: true,
 	};
@@ -845,24 +845,3 @@ export function addPluginConfigFields(pluginInfos: PluginNodeEnvInfo[] | PluginW
 	}
 }
 
-/**
- * Set the version field of each plugin to the version that was loaded
- *
- * Done after loading a config so the plugin migrations can compare the
- * stored version against the one now running.
- *
- * @param config - Config to update.
- * @param pluginInfos - Plugins loaded.
- * @returns true if any version changed.
- */
-export function setPluginVersions(config: classes.Config<any>, pluginInfos: PluginNodeEnvInfo[]) {
-	let changed = false;
-	for (const pluginInfo of pluginInfos) {
-		const name = `${pluginInfo.name}.version`;
-		if (config.constructor.fieldDefinitions[name] && config.get(name) !== pluginInfo.version) {
-			config.set(name, pluginInfo.version);
-			changed = true;
-		}
-	}
-	return changed;
-}

packages/lib/src/plugin_loader.ts

@@ -10,6 +10,7 @@ import * as libErrors from "./errors.js";
 import * as libPlugin from "./plugin.js";
 import { type Logger, logger } from "./logging.js";
 import { loadPluginEntrypoint, loadPluginClass } from "./loadPlugin.js";
+import { Config } from "./config/classes.js";
 import { ControllerConfig, HostConfig, InstanceConfig } from "./config/definitions.js";
 import Role from "./data/Role.js";
 
@@ -97,7 +98,8 @@ export async function loadPluginInfos(pluginList: Map<string, string>) {
  *
  * Must be called before configs and roles are loaded for the migrations
  * to run on them. Loaded for every plugin, including those disabled with
- * load_plugin.
+ * load_plugin. Also registers the plugin versions written to each config
+ * after the migrations run.
  *
  * @param pluginInfos - Plugins to load migrations for.
  */
@@ -110,6 +112,7 @@ export async function loadPluginMigrations(pluginInfos: libPlugin.PluginNodeEnvI
 	};
 
 	for (const pluginInfo of pluginInfos) {
+		Config.pluginVersions.set(pluginInfo.name, pluginInfo.version);
 		if (!pluginInfo.migrationEntrypoint) {
 			continue;
 		}

test/lib/SyncHook.js

@@ -10,43 +10,56 @@ describe("lib/SyncHook", function() {
 			hook = new lib.SyncHook();
 		});
 
-		it("should track attached handlers", function() {
-			assert.equal(hook.size, 0);
-			hook.attach("alpha", value => value);
-			hook.attach("beta", value => value);
-			assert.equal(hook.size, 2);
-			assert.deepEqual([...hook.attached], ["alpha", "beta"]);
+		describe(".attach()", function() {
+			it("should track attached handlers", function() {
+				assert.equal(hook.size, 0);
+				hook.attach("alpha", value => value);
+				hook.attach("beta", value => value);
+				assert.equal(hook.size, 2);
+				assert.deepEqual([...hook.attached], ["alpha", "beta"]);
+			});
+			it("should reject duplicate handler names", function() {
+				hook.attach("alpha", value => value);
+				assert.throws(
+					() => hook.attach("alpha", value => value),
+					new Error("Handler with name alpha is already attached")
+				);
+			});
 		});
-		it("should reject duplicate handler names", function() {
-			hook.attach("alpha", value => value);
-			assert.throws(
-				() => hook.attach("alpha", value => value),
-				new Error("Handler with name alpha is already attached")
-			);
+		describe(".detach()", function() {
+			it("should remove the named handler", function() {
+				hook.attach("alpha", value => value);
+				hook.attach("beta", value => value);
+				hook.detach("alpha");
+				assert.deepEqual([...hook.attached], ["beta"]);
+			});
 		});
-		it("should detach and clear handlers", function() {
-			hook.attach("alpha", value => value);
-			hook.attach("beta", value => value);
-			hook.detach("alpha");
-			assert.deepEqual([...hook.attached], ["beta"]);
-			hook.clear();
-			assert.equal(hook.size, 0);
+		describe(".clear()", function() {
+			it("should remove all handlers", function() {
+				hook.attach("alpha", value => value);
+				hook.attach("beta", value => value);
+				hook.clear();
+				assert.equal(hook.size, 0);
+			});
 		});
-		it("should pass the value through handlers in attach order", function() {
-			hook.attach("alpha", value => [...value, "alpha"]);
-			hook.attach("beta", value => [...value, "beta"]);
-			assert.deepEqual(hook.reduce([]), ["alpha", "beta"]);
-		});
-		it("should return the value unchanged with no handlers", function() {
-			const value = {};
-			assert.equal(hook.reduce(value), value);
-		});
-		it("should rethrow errors as a PluginError named after the handler", function() {
-			hook.attach("alpha", () => { throw new Error("failed"); });
-			assert.throws(
-				() => hook.reduce(1),
-				err => err instanceof lib.PluginError && err.pluginName === "alpha" && err.original.message === "failed"
-			);
+		describe(".reduce()", function() {
+			it("should pass the value through handlers in attach order", function() {
+				hook.attach("alpha", value => [...value, "alpha"]);
+				hook.attach("beta", value => [...value, "beta"]);
+				assert.deepEqual(hook.reduce([]), ["alpha", "beta"]);
+			});
+			it("should return the value unchanged with no handlers", function() {
+				const value = {};
+				assert.equal(hook.reduce(value), value);
+			});
+			it("should rethrow errors as a PluginError named after the handler", function() {
+				hook.attach("alpha", () => { throw new Error("failed"); });
+				assert.throws(
+					() => hook.reduce(1),
+					err => err instanceof lib.PluginError && err.pluginName === "alpha"
+						&& err.original.message === "failed"
+				);
+			});
 		});
 	});
 });

test/lib/config/migrations.js

@@ -45,7 +45,7 @@ describe("lib/config/definitions/migrations", function() {
 	describe("Ctl Config", function() {
 
 	});
-	describe("Plugin migrations", function() {
+	describe("Plugin Config", function() {
 		const pluginInfo = {
 			name: "test_plugin",
 			version: "1.2.0",
@@ -59,6 +59,7 @@ describe("lib/config/definitions/migrations", function() {
 		});
 		afterEach(function() {
 			lib.InstanceConfig.pluginMigrations.clear();
+			lib.Config.pluginVersions.clear();
 		});
 		after(function() {
 			for (const Config of [lib.ControllerConfig, lib.HostConfig, lib.InstanceConfig]) {
@@ -70,44 +71,81 @@ describe("lib/config/definitions/migrations", function() {
 			}
 		});
 
-		it("should run attached migrations on load", function() {
-			lib.InstanceConfig.pluginMigrations.attach("test_plugin", config => (
-				lib.renameConfigPrefix(config, "old_plugin.", "test_plugin.")
-			));
-			const config = lib.InstanceConfig.fromJSON({
-				"old_plugin.load_plugin": false,
-				"old_plugin.delay": 5,
-			}, "controller");
-			assert.equal(config.get("test_plugin.load_plugin"), false);
-			assert.equal(config.get("test_plugin.delay"), 5);
-			assert.equal(config.toJSON()["old_plugin.delay"], undefined);
-		});
-		it("should run plugin migrations after core migrations", function() {
-			lib.InstanceConfig.pluginMigrations.attach("test_plugin", config => {
-				assert.equal(config["factorio.sync_adminlist"], "enabled");
-				return config;
+		describe("static .pluginMigrations", function() {
+			it("should run on load", function() {
+				lib.InstanceConfig.pluginMigrations.attach("test_plugin", config => (
+					lib.renameConfigPrefix(config, "old_plugin.", "test_plugin.")
+				));
+				const config = lib.InstanceConfig.fromJSON({
+					"old_plugin.load_plugin": false,
+					"old_plugin.delay": 5,
+				}, "controller");
+				assert.equal(config.get("test_plugin.load_plugin"), false);
+				assert.equal(config.get("test_plugin.delay"), 5);
+				assert.equal(config.toJSON()["old_plugin.delay"], undefined);
 			});
-			lib.InstanceConfig.fromJSON({ "factorio.sync_adminlist": true }, "controller");
-		});
-		it("should give migrations the version the config was saved with", function() {
-			let seen;
-			lib.InstanceConfig.pluginMigrations.attach("test_plugin", config => {
-				seen = config["test_plugin.version"];
-				return config;
+			it("should run after core migrations", function() {
+				lib.InstanceConfig.pluginMigrations.attach("test_plugin", config => {
+					assert.equal(config["factorio.sync_adminlist"], "enabled");
+					return config;
+				});
+				lib.InstanceConfig.fromJSON({ "factorio.sync_adminlist": true }, "controller");
+			});
+			it("should see the version the config was saved with", function() {
+				let seen;
+				lib.InstanceConfig.pluginMigrations.attach("test_plugin", config => {
+					seen = config["test_plugin.version"];
+					return config;
+				});
+				lib.InstanceConfig.fromJSON({ "test_plugin.version": "1.1.0" }, "controller");
+				assert.equal(seen, "1.1.0");
+			});
+			it("should not run again when the config is changed", function() {
+				lib.Config.pluginVersions.set("test_plugin", "1.2.0");
+				let calls = 0;
+				lib.InstanceConfig.pluginMigrations.attach("test_plugin", config => {
+					calls += 1;
+					if (config["test_plugin.version"] === "1.1.0") {
+						config["test_plugin.delay"] *= 60;
+					}
+					return config;
+				});
+				const config = lib.InstanceConfig.fromJSON({
+					"test_plugin.version": "1.1.0",
+					"test_plugin.delay": 2,
+				}, "controller");
+				config.set("instance.name", "changed");
+				assert.equal(config.get("test_plugin.delay"), 120);
+				assert.equal(calls, 1);
 			});
-			const config = lib.InstanceConfig.fromJSON({ "test_plugin.version": "1.1.0" }, "controller");
-			assert.equal(seen, "1.1.0");
-			assert.equal(config.get("test_plugin.version"), "1.1.0");
 		});
-		it("should default the version of new configs to the loaded plugin version", function() {
-			const config = new lib.InstanceConfig("controller");
-			assert.equal(config.get("test_plugin.version"), "1.2.0");
-		});
-		it("should update versions with setPluginVersions", function() {
-			const config = lib.InstanceConfig.fromJSON({ "test_plugin.version": "1.1.0" }, "controller");
-			assert.equal(lib.setPluginVersions(config, [pluginInfo]), true);
-			assert.equal(config.get("test_plugin.version"), "1.2.0");
-			assert.equal(lib.setPluginVersions(config, [pluginInfo]), false);
+		describe("static .pluginVersions", function() {
+			it("should write the version after migrations run", function() {
+				lib.Config.pluginVersions.set("test_plugin", "1.2.0");
+				const config = lib.InstanceConfig.fromJSON({ "test_plugin.version": "1.1.0" }, "controller");
+				assert.equal(config.get("test_plugin.version"), "1.2.0");
+				assert.equal(config.dirty, true);
+			});
+			it("should mark configs saved without a version as dirty", function() {
+				lib.Config.pluginVersions.set("test_plugin", "1.2.0");
+				const config = lib.InstanceConfig.fromJSON({}, "controller");
+				assert.equal(config.get("test_plugin.version"), "1.2.0");
+				assert.equal(config.dirty, true);
+			});
+			it("should not mark configs already at the version as dirty", function() {
+				lib.Config.pluginVersions.set("test_plugin", "1.2.0");
+				const config = lib.InstanceConfig.fromJSON({ "test_plugin.version": "1.2.0" }, "controller");
+				assert.equal(config.dirty, false);
+			});
+			it("should write the version to new configs", function() {
+				lib.Config.pluginVersions.set("test_plugin", "1.2.0");
+				const config = new lib.InstanceConfig("controller");
+				assert.equal(config.get("test_plugin.version"), "1.2.0");
+			});
+			it("should leave the version empty when no versions are registered", function() {
+				const config = new lib.InstanceConfig("controller");
+				assert.equal(config.get("test_plugin.version"), "");
+			});
 		});
 	});
 });

test/lib/plugin_loader.js

@@ -128,11 +128,13 @@ describe("lib/plugin_loader", function() {
 		});
 		afterEach(function() {
 			lib.ControllerConfig.pluginMigrations.clear();
+			lib.Config.pluginVersions.clear();
 		});
 
 		function info(entrypoint) {
 			return {
 				name: "test",
+				version: "1.0.0",
 				packagePath: path.join(baseDir, "package.json"),
 				migrationEntrypoint: entrypoint,
 			};
@@ -143,6 +145,10 @@ describe("lib/plugin_loader", function() {
 			assert.deepEqual([...lib.ControllerConfig.pluginMigrations.attached], ["test"]);
 			assert.deepEqual(lib.ControllerConfig.pluginMigrations.reduce({}), { migratedBy: "test" });
 		});
+		it("should register the version of every plugin", async function() {
+			await lib.loadPluginMigrations([info("migrations.js"), { name: "other", version: "2.0.0" }]);
+			assert.deepEqual([...lib.Config.pluginVersions], [["test", "1.0.0"], ["other", "2.0.0"]]);
+		});
 		it("should reject with a PluginError if the entrypoint throws", async function() {
 			await assert.rejects(
 				lib.loadPluginMigrations([info("throwing.js")]),