Merge uploads by the components they were serialized with

The controller merged uploads using the uploading instance's current config. An upload serialized under different settings, for example one sent from the save right after an admin turned on sync_inventories and restarted the instance, was then merged as if it held a character and wiped the stored one. Uploads now carry the components they were serialized with, the controller merges by those and drops the key before storing.

The scenario keeps the components a player's synced session started with and serializes their upload with them, so changing the options while a player is online takes effect the next time they join. A player whose session lacks a component that is now synced downloads again even if the generation matches. Downloads snapshot the components when they start, so a change during a download can't mix two sets.

A failed acquire unsyncs the player again regardless of the inventories setting, as before this branch.

The god and dead player inventories move from the inventories component to the controller one, matching what the scenario syncs, which removes the special case in mergePlayerData.

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

plugins/player_sync/README.md

@@ -55,6 +55,8 @@ Should an error occur during this process the player is given the option to use
 
 Instances can leave parts of the player out of the sync with the `player_sync.sync_*` options below.
 A part which is not synced is left as it is when the player joins, and the instance's copy of it is not uploaded, so the stored copy from other instances stays as it was.
+A player online when the options change keeps the old ones until they leave and join again, and a part that was turned on is downloaded when they join.
+Turning off `player_sync.sync_inventories` leaves the items players have on the instance in place while the stored inventory still holds them too.
 
 Communication between the server and instance goes over stdout or rcon, depending on the size of the data.
 Communication between the instance and the controller goes over websockets.
@@ -83,7 +85,7 @@ Defaults to `1000`.
 
 ### player_sync.sync_controller
 
-Sync whether the player is a character, god or spectator, their respawn timer and cheat mode.
+Sync whether the player is a character, god or spectator, their respawn timer, cheat mode, the god inventory and the inventory filters of a dead player.
 Switching controller can destroy the character holding the inventory, so this only takes effect while `player_sync.sync_inventories` is also enabled.
 
 Defaults to `true`.
@@ -102,8 +104,8 @@ Defaults to `true`.
 
 ### player_sync.sync_inventories
 
-Sync the character and god inventories, the character bonuses and the crafting queue.
-While the controller is not synced only the inventory of a character is synced, a player without a character keeps the inventory they have on the instance and does not change the stored one.
+Sync the character inventories, the character bonuses and the crafting queue.
+While the controller is not synced a player without a character keeps what they have on the instance and does not change the stored character.
 A player waiting to respawn is respawned straight away if there is a synced character to give them.
 
 Defaults to `true`.

plugins/player_sync/components.ts

@@ -3,10 +3,11 @@ import type { IpcPlayerData } from "./messages.js";
 
 /** Parts of the player data an instance can choose to sync, and the fields of the player data holding them. */
 export const components = {
-	controller: ["controller", "ticks_to_respawn", "cheat_mode"],
+	// inventories holds the god inventory and the hidden ghost inventory, both belong to the controller
+	controller: ["controller", "ticks_to_respawn", "cheat_mode", "inventories"],
 	force: ["force"],
 	appearance: ["color", "chat_color", "tag"],
-	inventories: ["character", "inventories", "crafting_queue"],
+	inventories: ["character", "crafting_queue"],
 	logistics: ["personal_logistic_slots"],
 	quick_bar: ["quick_bar", "hotbar"],
 	settings: ["flashlight", "shortcuts", "game_view_settings"],
@@ -19,15 +20,15 @@ export type Component = keyof typeof components;
 const characterFields = ["character", "crafting_queue", "personal_logistic_slots"] as const;
 
 /**
- * Components synced on an instance.
+ * Build the set of synced components.
  *
  * The controller is only synced together with the inventories, as
  * switching controller can destroy the character holding them.
  */
-export function enabledComponents(config: lib.InstanceConfig) {
+function componentSet(isEnabled: (component: Component) => boolean) {
 	const enabled = new Set<Component>();
 	for (const component of Object.keys(components) as Component[]) {
-		if (config.get(`player_sync.sync_${component}`)) {
+		if (isEnabled(component)) {
 			enabled.add(component);
 		}
 	}
@@ -37,6 +38,23 @@ export function enabledComponents(config: lib.InstanceConfig) {
 	return enabled;
 }
 
+/** Components synced on an instance. */
+export function enabledComponents(config: lib.InstanceConfig) {
+	return componentSet(component => Boolean(config.get(`player_sync.sync_${component}`)));
+}
+
+/**
+ * Components an upload was serialized with, all of them when not given.
+ *
+ * Lua may encode an empty table as an array, which reads as no components.
+ */
+export function uploadedComponents(uploaded: Record<string, boolean> | unknown[] | undefined) {
+	if (uploaded === undefined) {
+		return componentSet(() => true);
+	}
+	return componentSet(component => !Array.isArray(uploaded) && uploaded[component] === true);
+}
+
 /** Remove the fields of components which are not synced. */
 export function stripPlayerData(playerData: IpcPlayerData, enabled: Set<Component>) {
 	const fields = playerData as Record<string, unknown>;
@@ -53,13 +71,13 @@ export function stripPlayerData(playerData: IpcPlayerData, enabled: Set<Componen
 /**
  * Combine data uploaded by an instance with the stored data.
  *
- * Components the instance does not sync keep the stored fields. Without
- * the controller synced the god and ghost inventories are left as the
- * synced controller had them, and character fields are only taken from a
+ * Components the upload was not serialized with keep the stored fields.
+ * Without the controller synced character fields are only taken from a
  * player which has a character.
  */
-export function mergePlayerData(stored: IpcPlayerData | undefined, uploaded: IpcPlayerData, enabled: Set<Component>) {
-	const merged: Record<string, unknown> = { ...uploaded };
+export function mergePlayerData(stored: IpcPlayerData | undefined, uploaded: IpcPlayerData) {
+	const { components: uploadComponents, ...merged } = uploaded as IpcPlayerData & Record<string, unknown>;
+	const enabled = uploadedComponents(uploadComponents);
 	function keep(keys: readonly string[]) {
 		for (const key of keys) {
 			const value = (stored as Record<string, unknown> | undefined)?.[key];
@@ -76,11 +94,8 @@ export function mergePlayerData(stored: IpcPlayerData | undefined, uploaded: Ipc
 			keep(keys);
 		}
 	}
-	if (!enabled.has("controller")) {
-		keep(["inventories"]);
-		if (!uploaded.character) {
-			keep(characterFields);
-		}
+	if (!enabled.has("controller") && !uploaded.character) {
+		keep(characterFields);
 	}
 	return merged as IpcPlayerData;
 }

plugins/player_sync/controller.ts

@@ -1,6 +1,6 @@
 import type { ControllerPluginContext, InstanceRecord } from "@clusterio/controller";
 import type { IpcPlayerData } from "./messages.js";
-import { enabledComponents, mergePlayerData } from "./components.js";
+import { mergePlayerData } from "./components.js";
 
 import fs from "node:fs/promises";
 import path from "path";
@@ -138,8 +138,7 @@ export default async function(context: ControllerPluginContext) {
 
 	controller.handle(msg.UploadRequest, async (request: msg.UploadRequest) => {
 		let { instanceId, playerName, playerData } = request;
-		let instanceConfig = controller.instances.get(instanceId)!.config;
-		let instanceName = instanceConfig.get("instance.name");
+		let instanceName = controller.instances.get(instanceId)!.config.get("instance.name");
 		let store = true;
 		let acquisitionRecord = acquiredPlayers.get(playerName);
 		if (!acquisitionRecord) {
@@ -167,9 +166,7 @@ export default async function(context: ControllerPluginContext) {
 
 		if (store) {
 			logger.verbose(`Received player data for ${playerName} from ${instanceName}`);
-			playerDatastore.set(
-				playerName, mergePlayerData(oldPlayerData, playerData, enabledComponents(instanceConfig))
-			);
+			playerDatastore.set(playerName, mergePlayerData(oldPlayerData, playerData));
 			playerDatastoreDirty = true;
 		}
 	});

plugins/player_sync/index.ts

@@ -39,8 +39,8 @@ export const plugin: lib.PluginDeclaration = {
 		"player_sync.sync_controller": {
 			title: "Sync controller",
 			description:
-				"Sync whether the player is a character, god or spectator, their respawn timer and cheat " +
-				"mode. Only synced while inventories are synced too.",
+				"Sync whether the player is a character, god or spectator, their respawn timer, cheat mode " +
+				"and god inventory. Only synced while inventories are synced too.",
 			type: "boolean",
 			initialValue: true,
 		},
@@ -59,8 +59,8 @@ export const plugin: lib.PluginDeclaration = {
 		"player_sync.sync_inventories": {
 			title: "Sync inventories",
 			description:
-				"Sync the character and god inventories, character bonuses and crafting queue. Without the " +
-				"controller synced only the inventory of a character is synced.",
+				"Sync the character inventories, character bonuses and crafting queue. Without the " +
+				"controller synced a player without a character keeps their local inventory.",
 			type: "boolean",
 			initialValue: true,
 		},

plugins/player_sync/messages.ts

@@ -22,6 +22,8 @@ export type IpcPlayerData = {
 	personal_logistic_slots?: {name:string, min:number, max:number}[],
 	crafting_queue?: any,
 	recipe_notifications?: string,
+	// Components the upload was serialized with, all when missing
+	components?: Record<string, boolean> | unknown[],
 }
 
 const jsonPlayerData = Type.Object({

plugins/player_sync/module/download_inventory.lua

@@ -34,7 +34,7 @@ local function download_inventory(player_name, data, number, total)
 		progress_dialog.remove(player)
 
 		-- Give the player a character and pretend that's the synced player data
-		if serialize.syncs(script_data.components, "controller") or player_record.local_controller then
+		if serialize.syncs(record.components, "controller") or player_record.local_controller then
 			ensure_character(player)
 		end
 		player_record.local_controller = nil
@@ -45,6 +45,7 @@ local function download_inventory(player_name, data, number, total)
 		script_data.active_downloads[player_name] = nil
 		player_record.dirty = player.connected
 		player_record.sync = true
+		player_record.components = record.components
 		player_record.generation = record.generation
 		return
 	end

plugins/player_sync/module/player_sync.lua

@@ -24,9 +24,10 @@ local function is_in_cutscene(player)
 	return player.controller_type == defines.controllers.cutscene
 end
 
--- Returns true if the component is synced on this instance
-local function syncs(name)
-	return serialize.syncs(get_script_data().components, name)
+-- Remember the components a player's synced session uses, uploads are serialized with these
+local function start_sync(player_record, components)
+	player_record.sync = true
+	player_record.components = components
 end
 
 -- Remember the controller the player had before sync code replaced it, restored when the controller is not synced
@@ -95,7 +96,7 @@ function player_sync.serialize_player(player, player_record)
 	end
 
 	local failed_deserialization = get_script_data().failed_deserialization[player.name] or {}
-	local serialized_player = serialize.serialize_player(player, failed_deserialization, get_script_data().components)
+	local serialized_player = serialize.serialize_player(player, failed_deserialization, player_record.components)
 	serialized_player.generation = player_record.generation
 
 	return serialized_player
@@ -118,7 +119,7 @@ function player_sync.deserialize_player(player, finished_record)
 
 	-- Stash temporary inventory if it exists, without inventory sync the current inventory is the one to keep
 	local stashed_corpse
-	if syncs("inventories") and player_record.dirty and player.character then
+	if serialize.syncs(finished_record.components, "inventories") and player_record.dirty and player.character then
 		local character = player.character
 		local surface = character.surface
 		local position = character.position
@@ -132,7 +133,7 @@ function player_sync.deserialize_player(player, finished_record)
 	local serialized_player = compat.json_to_table(finished_record.data)
 	assert(type(serialized_player) == "table", "wrong type for serialized_player")
 	script_data.failed_deserialization[player.name] =
-		serialize.deserialize_player(player, serialized_player, script_data.components, local_controller)
+		serialize.deserialize_player(player, serialized_player, finished_record.components, local_controller)
 	player_record.local_controller = nil
 
 	-- Restore player position and driving state
@@ -312,8 +313,10 @@ function player_sync.sync_player(acquire_response)
 
 	local player_record = script_data.players[acquire_response.player_name]
 	if acquire_response.status ~= "acquired" then
-		if player_record.sync and not player_record.dirty and syncs("inventories") then
-			if player.controller_type == defines.controllers.character then
+		if player_record.sync and not player_record.dirty then
+			if serialize.syncs(player_record.components, "inventories")
+				and player.controller_type == defines.controllers.character
+			then
 				save_local_controller(player, player_record)
 				local character = player.character
 				player.set_controller({ type = defines.controllers.spectator })
@@ -327,7 +330,11 @@ function player_sync.sync_player(acquire_response)
 
 	if
 		acquire_response.has_data
-		and (not player_record.sync or acquire_response.generation > player_record.generation)
+		and (
+			not player_record.sync
+			or acquire_response.generation > player_record.generation
+			or serialize.newly_synced(player_record.components, script_data.components)
+		)
 	then
 		player_sync.initiate_inventory_download(player, player_record, acquire_response.generation)
 
@@ -340,7 +347,7 @@ function player_sync.sync_player(acquire_response)
 			)
 		end
 
-		player_record.sync = true
+		start_sync(player_record, script_data.components)
 		player_record.dirty = true
 	end
 end
@@ -361,7 +368,7 @@ function player_sync.finish_download(player, finished_record)
 	end
 
 	player_record.dirty = true
-	player_record.sync = true
+	start_sync(player_record, finished_record.components)
 	player_record.generation = finished_record.generation
 end
 
@@ -469,14 +476,15 @@ function player_sync.initiate_inventory_download(player, player_record, generati
 		started = game.ticks_played,
 		last_active = game.ticks_played,
 		generation = generation,
-		data = ""
+		data = "",
 	}
 	local script_data = get_script_data()
+	record.components = script_data.components
 	script_data.active_downloads[player.name] = record
 
 	-- The plugin only sends back what differs from the current notification state
 	local recipe_notifications
-	if recipe_notifications_api and syncs("recipe_notifications") then
+	if recipe_notifications_api and serialize.syncs(record.components, "recipe_notifications") then
 		recipe_notifications = serialize.serialize_crafting_notifications(player)
 	end
 
@@ -488,7 +496,10 @@ function player_sync.initiate_inventory_download(player, player_record, generati
 	-- If this is a synced player turn them into a spectator while the
 	-- player data is downloading
 	-- Without the controller synced a player without a character has nothing synced to protect
-	if player_record.sync and syncs("inventories") and (syncs("controller") or player.character) then
+	local components = record.components
+	if player_record.sync and serialize.syncs(components, "inventories")
+		and (serialize.syncs(components, "controller") or player.character)
+	then
 		-- Store original position to teleport back to
 		if v2_remote_controller then
 			record.surface = player.physical_surface

plugins/player_sync/module/serialize.lua

@@ -730,6 +730,7 @@ end
 --- @field shortcuts table<string, boolean>?
 --- @field game_view_settings table<string, boolean>?
 --- @field ticks_to_respawn number?
+--- @field components SyncComponents Components this was serialized with
 --- @field character table<string, any>?
 --- @field inventories table<string, table>?
 --- @field hotbar table<string, string>?
@@ -795,6 +796,23 @@ local function syncs(components, name)
 end
 serialize.syncs = syncs
 
+local component_names = {
+	"controller", "force", "appearance", "inventories", "logistics", "quick_bar", "settings", "recipe_notifications",
+}
+
+--- True if a component synced in new_components was not synced in old_components
+--- @param old_components SyncComponents
+--- @param new_components SyncComponents
+--- @return boolean
+function serialize.newly_synced(old_components, new_components)
+	for _, name in ipairs(component_names) do
+		if syncs(new_components, name) and not syncs(old_components, name) then
+			return true
+		end
+	end
+	return false
+end
+
 --- @param player LuaPlayer
 --- @param failed_deserialization FailedDeserializationPlayerData
 --- @param components SyncComponents
@@ -808,6 +826,7 @@ function serialize.serialize_player(player, failed_deserialization, components)
 	local serialized = {
 		generation = 0, -- Gets replaced later
 		name = player.name,
+		components = components,
 	}
 
 	if sync_controller then

plugins/player_sync/test/components.js

@@ -1,6 +1,8 @@
 import assert from "node:assert/strict";
 
-import { components, enabledComponents, mergePlayerData, stripPlayerData } from "../dist/node/components.js";
+import {
+	components, enabledComponents, mergePlayerData, stripPlayerData, uploadedComponents,
+} from "../dist/node/components.js";
 
 const allComponents = Object.keys(components);
 
@@ -69,15 +71,45 @@ describe("player_sync components", function() {
 			}
 			assert.equal(playerData.force, "enemy");
 		});
+		it("should remove the god inventory with the controller", function() {
+			const playerData = structuredClone(stored);
+			stripPlayerData(playerData, new Set(allComponents.filter(c => c !== "controller")));
+			assert.equal("inventories" in playerData, false);
+			assert.equal("controller" in playerData, false);
+			assert.equal(playerData.tag, "stored");
+		});
+	});
+
+	describe("uploadedComponents()", function() {
+		it("should sync everything when the upload has no components", function() {
+			assert.deepEqual([...uploadedComponents(undefined)], allComponents);
+		});
+		it("should sync the components marked true", function() {
+			const enabled = uploadedComponents({ force: true, quick_bar: true, settings: false });
+			assert.deepEqual([...enabled], ["force", "quick_bar"]);
+		});
+		it("should sync nothing for an empty table encoded as an array", function() {
+			assert.equal(uploadedComponents([]).size, 0);
+		});
+		it("should not sync the controller without the inventories", function() {
+			assert.equal(uploadedComponents({ controller: true }).has("controller"), false);
+		});
 	});
 
 	describe("mergePlayerData()", function() {
-		it("should store the upload as is when all components are synced", function() {
-			assert.deepEqual(mergePlayerData(stored, uploaded, new Set(allComponents)), uploaded);
+		function withComponents(data, disabled) {
+			const enabled = allComponents.filter(c => !disabled.includes(c));
+			return { ...data, components: Object.fromEntries(enabled.map(c => [c, true])) };
+		}
+
+		it("should store the upload as is when it has no components", function() {
+			assert.deepEqual(mergePlayerData(stored, uploaded), uploaded);
+		});
+		it("should not store the components of the upload", function() {
+			assert.deepEqual(mergePlayerData(stored, withComponents(uploaded, [])), uploaded);
 		});
 		it("should keep the stored fields of disabled components", function() {
-			const enabled = new Set(allComponents.filter(c => !["force", "appearance"].includes(c)));
-			const merged = mergePlayerData(stored, uploaded, enabled);
+			const merged = mergePlayerData(stored, withComponents(uploaded, ["force", "appearance"]));
 			assert.equal(merged.force, "player");
 			assert.deepEqual(merged.color, [1, 0, 0]);
 			assert.equal(merged.tag, "stored");
@@ -85,27 +117,29 @@ describe("player_sync components", function() {
 			assert.equal(merged.generation, 2);
 		});
 		it("should drop disabled components without stored data", function() {
-			const merged = mergePlayerData(undefined, uploaded, new Set(allComponents.filter(c => c !== "force")));
+			const merged = mergePlayerData(undefined, withComponents(uploaded, ["force"]));
 			assert.equal("force" in merged, false);
 			assert.equal(merged.tag, "uploaded");
 		});
 		it("should take the character but not the controller when the controller is not synced", function() {
-			const merged = mergePlayerData(stored, uploaded, new Set(allComponents.filter(c => c !== "controller")));
+			const merged = mergePlayerData(stored, withComponents(uploaded, ["controller"]));
 			assert.equal(merged.controller, "god");
 			assert.equal(merged.cheat_mode, true);
 			assert.equal(merged.character, character);
 			assert.deepEqual(merged.inventories, { main: ["stored god item"] }, "god inventory is kept");
 		});
 		it("should keep the character data for a player without one when the controller is not synced", function() {
-			const storedCharacter = { ...stored, character, crafting_queue: ["stored"], personal_logistic_slots: [] };
+			const storedCharacter = {
+				...stored, character, crafting_queue: ["stored"], personal_logistic_slots: [{ name: "stored" }],
+			};
 			const god = { ...uploaded, controller: "god", inventories: { main: ["new god item"] } };
 			delete god.character;
 			delete god.crafting_queue;
-			delete god.personal_logistic_slots;
-			const enabled = new Set(allComponents.filter(c => c !== "controller"));
-			const merged = mergePlayerData(storedCharacter, god, enabled);
+			god.personal_logistic_slots = [{ name: "uploaded" }];
+			const merged = mergePlayerData(storedCharacter, withComponents(god, ["controller"]));
 			assert.equal(merged.character, character);
 			assert.deepEqual(merged.crafting_queue, ["stored"]);
+			assert.deepEqual(merged.personal_logistic_slots, [{ name: "stored" }], "logistics stay with the character");
 			assert.deepEqual(merged.inventories, { main: ["stored god item"] });
 		});
 	});

plugins/player_sync/test/controller.js

@@ -5,6 +5,7 @@ import path from "node:path";
 import * as lib from "@clusterio/lib";
 import * as mock from "../../../test/mock.js";
 import controllerEntrypoint from "../dist/node/controller.js";
+import { components as allComponents } from "../dist/node/components.js";
 import { plugin as info } from "../dist/node/index.js";
 import { DatabaseStatsRequest, DownloadRequest, UploadRequest } from "../dist/node/messages.js";
 
@@ -75,14 +76,15 @@ describe("player_sync controller", function() {
 				.playerData;
 		}
 
-		it("should store the upload as is when everything is synced", async function() {
+		it("should store the upload as is when it has no components", async function() {
 			const playerData = { generation: 2, name: "test", force: "enemy", tag: "uploaded" };
 			assert.deepEqual(await upload(playerData), playerData);
 		});
-		it("should keep stored fields the uploading instance does not sync", async function() {
-			disabled = ["force"];
+		it("should merge by the payload's components and ignore the instance config", async function() {
+			disabled = ["appearance"];
+			const components = Object.fromEntries(Object.keys(allComponents).map(c => [c, c !== "force"]));
 			assert.deepEqual(
-				await upload({ generation: 2, name: "test", tag: "uploaded" }),
+				await upload({ generation: 2, name: "test", tag: "uploaded", components }),
 				{ generation: 2, name: "test", force: "player", tag: "uploaded" },
 			);
 		});

plugins/player_sync/test/instance.js

@@ -169,5 +169,14 @@ describe("player_sync", function() {
 			await instance.hooks.instanceConfigFieldChanged.invoke("player_sync.sync_force", false, true);
 			assert.equal(instance.server.rconCommands.length, 1);
 		});
+		it("should not send the components when the instance is not running", async function() {
+			instance.status = "stopped";
+			try {
+				await instance.hooks.instanceConfigFieldChanged.invoke("player_sync.sync_force", false, true);
+			} finally {
+				instance.status = "running";
+			}
+			assert.equal(instance.server.rconCommands.length, 0);
+		});
 	});
 });