Download at the next join when the controller did not store an upload

The controller gave an upload it did not store the same empty reply as
one it stored, and the instance confirmed either to the save. An instance
restarting after a crash uploads the players it had online, and when
another instance had since uploaded the same generation the save took its
own copy for the stored one: the next join kept it, and the leave after
that stored it over the newer data.

The reply now says whether the upload was stored. One that was not marks
the save's copy outdated, so it is not uploaded again and the next join
downloads.

Fixes #1218

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,29 @@ 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,
+	) {
+	}
+
+	static jsonSchema = Type.Object({
+		"stored": Type.Boolean(),
+	});
+
+	static fromJSON(json: Static<typeof UploadResponse.jsonSchema>) {
+		return new this(json.stored);
+	}
+}
+
 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,27 @@ 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 or player.connected then
+		return
+	end
+	local script_data = get_script_data()
+	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() {