Reject an upload for a player who is back, and take an empty reply as stored

A player rejoining before the controller's reply was skipped by
reject_upload, and the join then kept the copy the controller refused.
The rejection now counts while the join still waits for its acquire
reply, which the controller sends after it.

A controller from before the upload response replies with nothing. That
failed validation on a newer host, so its uploads were never confirmed.
It is taken as stored, as before.

Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JyNnVzRW4jPWaoWTxCyhM7

plugins/player_sync/README.md

@@ -6,7 +6,7 @@ As a player, it mostly just works. Most important to know is that factorio data
 
 A note on crashing servers:
 
-When a server crashes while you are online there won't be time to immediately upload the inventory. Instead, the inventory is uploaded as soon as the server restarts. This means if you have been playing on a different server from the one that crashed and gathered/consumed items but are offline at the time of the first server coming online again your inventory will reset to the autosave.
+When a server crashes while you are online there won't be time to immediately upload the inventory. Instead, the inventory is uploaded as soon as the server restarts. If you have been playing on a different server in the meantime the controller keeps what that server uploaded, and the crashed server downloads it the next time you join it.
 
 ## Installation
 

plugins/player_sync/controller.ts

@@ -258,6 +258,7 @@ export default async function(context: ControllerPluginContext) {
 				prefetch(instanceId, request.destinationInstanceId, merged);
 			}
 		}
+		return new msg.UploadRequest.Response(store);
 	});
 
 	controller.handle(msg.DownloadRequest, async (request: msg.DownloadRequest) => {

plugins/player_sync/instance.ts

@@ -200,11 +200,12 @@ export default async function(context: InstancePluginContext) {
 		delete player_data.destination_instance_id;
 
 		logger.verbose(`Uploading ${player_data.name} (${JSON.stringify(player_data).length / 1000}kB)`);
+		let stored: boolean;
 		try {
-			await instance.sendTo(
+			({ stored } = await instance.sendTo(
 				"controller",
 				new UploadRequest(instance.id, player_data.name, player_data, destinationInstanceId),
-			);
+			));
 
 		} catch (err: any) {
 			if (!(err instanceof lib.SessionLost)) {
@@ -213,8 +214,10 @@ export default async function(context: InstancePluginContext) {
 			return;
 		}
 
+		// A copy the controller did not store must not pass for the stored one: the save downloads at the next join
+		const reply = stored ? "confirm_upload" : "reject_upload";
 		await instance.sendRcon(
-			`/sc player_sync.confirm_upload("${player_data.name}", ${player_data.generation})`, true, plugin.name
+			`/sc player_sync.${reply}("${player_data.name}", ${player_data.generation})`, true, plugin.name
 		);
 	}
 

plugins/player_sync/messages.ts

@@ -262,12 +262,30 @@ export class ReleaseRequest {
 	}
 }
 
+export class UploadResponse {
+	constructor(
+		/** False when the controller kept what it has: another copy is newer, locked or from another epoch */
+		public stored: boolean,
+	) {
+	}
+
+	// No type: a controller from before this response replies with nothing, which counts as stored
+	static jsonSchema = Type.Unsafe<{ stored: boolean } | undefined>({
+		properties: { "stored": { type: "boolean" } },
+	});
+
+	static fromJSON(json: Static<typeof UploadResponse.jsonSchema>) {
+		return new this(json?.stored ?? true);
+	}
+}
+
 export class UploadRequest {
 	declare ["constructor"]: typeof UploadRequest;
 	static type = "request" as const;
 	static src = "instance" as const;
 	static dst = "controller" as const;
 	static plugin = "player_sync" as const;
+	static Response = UploadResponse;
 
 	constructor(
 		public instanceId: number,

plugins/player_sync/module/player_sync.lua

@@ -384,12 +384,14 @@ function player_sync.check_players_in_cutscene()
 end
 
 -- Whether a join should take the stored data: there is some, and this save hasn't synced the player in that epoch at
--- that generation or newer with every component synced here now. A player without a record here has nothing to keep.
+-- that generation or newer with every component synced here now, or the controller did not store its last upload
+-- (reject_upload). A player without a record here has nothing to keep.
 local function download_needed(player_record, acquire_response, script_data)
 	return acquire_response.has_data
 		and (
 			not player_record
 			or not player_record.sync
+			or player_record.outdated
 			or (player_record.epoch or 0) ~= (acquire_response.epoch or 0)
 			or acquire_response.generation > player_record.generation
 			or serialize.newly_synced(player_record.components, script_data.components)
@@ -543,6 +545,8 @@ function player_sync.sync_player(acquire_response)
 		start_sync(player_record, script_data.components)
 		player_record.dirty = true
 		player_record.sync_in_progress = false
+		-- Nothing stored to download: the copy here is all there is
+		player_record.outdated = nil
 		-- Deleted data keeps its generation and epoch so the next upload is accepted as the new baseline
 		player_record.epoch = epoch
 		-- A copy from another epoch has nothing to do with the stored generation
@@ -573,6 +577,7 @@ function player_sync.finish_download(player, finished_record)
 
 	player_record.dirty = true
 	player_record.sync_in_progress = false
+	player_record.outdated = nil
 	start_sync(player_record, finished_record.components)
 	player_record.generation = finished_record.generation
 	player_record.epoch = finished_record.epoch
@@ -724,6 +729,32 @@ function player_sync.confirm_upload(player_name, generation)
 	player_record.dirty = false
 end
 
+-- Invoked by the instance code when the controller did not store the uploaded data: it holds another copy that is
+-- newer, locked by another instance or from another epoch. This save's copy lost, even at the same generation, so it
+-- is not uploaded again and the next join downloads. A retry of an upload the controller already stored ends up here
+-- too, and then downloads what this save sent.
+function player_sync.reject_upload(player_name, generation)
+	local player = game.get_player(player_name)
+	if not player then
+		return
+	end
+	local script_data = get_script_data()
+	-- A player who is back already still waits for the acquire reply, which the controller sent after this one, and
+	-- that join then downloads. Past that reply the join has decided and this is not about the copy here any more.
+	if player.connected and not script_data.players_waiting_for_acquire[player_name] then
+		return
+	end
+	local player_record = script_data.players[player.name]
+	if not player_record or player_record.generation ~= generation then
+		return
+	end
+
+	log("Upload of " .. player_name .. " was not stored, the next join downloads")
+	script_data.active_uploads[player_name] = nil
+	player_record.dirty = false
+	player_record.outdated = true
+end
+
 function player_sync.initiate_inventory_download(player, player_record, generation, epoch)
 	player.print("Initiating inventory download...")
 	local record = {

plugins/player_sync/test/controller.js

@@ -77,6 +77,18 @@ describe("player_sync controller", function() {
 				.playerData;
 		}
 
+		it("should reply whether the upload was stored", async function() {
+			const handler = mock.getHandler(controller, msg.UploadRequest);
+			const send = (instanceId, generation) => handler(
+				{ instanceId, playerName: "test", playerData: { generation, name: "test" } }
+			);
+			assert.deepEqual(await send(1, 2), new msg.UploadRequest.Response(true));
+			// A crashed instance restarting with the generation another instance already uploaded
+			assert.deepEqual(await send(1, 2), new msg.UploadRequest.Response(false));
+			await mock.getHandler(controller, msg.AcquireRequest)(new msg.AcquireRequest(1, "test"));
+			controller.instances.set(2, controller.instances.get(1));
+			assert.deepEqual(await send(2, 3), new msg.UploadRequest.Response(false));
+		});
 		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);

plugins/player_sync/test/instance.js

@@ -5,7 +5,7 @@ import * as lib from "@clusterio/lib";
 
 import * as mock from "../../../test/mock.js";
 import loadInstancePlugin, { recipeNotificationDelta, applyRecipeNotificationDelta } from "../dist/node/instance.js";
-import { AcquireResponse, DownloadResponse, PrefetchEvent } from "../dist/node/messages.js";
+import { AcquireResponse, DownloadResponse, PrefetchEvent, UploadResponse } from "../dist/node/messages.js";
 import { components } from "../dist/node/components.js";
 import { plugin as info } from "../dist/node/index.js";
 
@@ -275,6 +275,25 @@ describe("player_sync", function() {
 			const request = await upload({ name: "test", generation: 2 });
 			assert.equal(request.destinationInstanceId, undefined);
 		});
+		async function rconAfterReply(stored) {
+			instance.server.reset();
+			instance.connector.once("send", message => {
+				instance.connector.emit("message", new lib.MessageResponse(
+					1, message.dst, message.src, new UploadResponse(stored)
+				));
+			});
+			instance.server.emit("ipc-player_sync_upload", { name: "test", generation: 2 });
+			for (let i = 0; i < 100 && !instance.server.rconCommands.length; i++) {
+				await lib.wait(1);
+			}
+			return instance.server.rconCommands;
+		}
+		it("should confirm an upload the controller stored", async function() {
+			assert.deepEqual(await rconAfterReply(true), ['/sc player_sync.confirm_upload("test", 2)']);
+		});
+		it("should reject an upload the controller did not store", async function() {
+			assert.deepEqual(await rconAfterReply(false), ['/sc player_sync.reject_upload("test", 2)']);
+		});
 	});
 
 	describe("PrefetchEvent", function() {

plugins/player_sync/test/plugin.js

@@ -60,6 +60,14 @@ describe("player_sync plugin", function() {
 				assert(!validate({ instanceId: 1, playerName: "player", playerData: data }), `accepted ${deleted}`);
 			}
 		});
+		it("should take the empty reply of an older controller as a stored upload", function() {
+			const validate = lib.compile(msg.UploadResponse.jsonSchema);
+			assert(validate(undefined));
+			assert.equal(msg.UploadResponse.fromJSON(undefined).stored, true);
+			assert(validate({ stored: false }));
+			assert.equal(msg.UploadResponse.fromJSON({ stored: false }).stored, false);
+			assert(!validate({ stored: "no" }));
+		});
 		describe("SetPlayerDataRequest schema", function() {
 			const validate = lib.compile(msg.SetPlayerDataRequest.jsonSchema);
 			// Shaped like module/serialize.lua output