Conversation
There was a problem hiding this comment.
Pull request overview
This PR finalizes the standalone server “maintenance + prepublish cleanup” layer by wiring durable Saved/ directory persistence, adding standalone snapshot upload packets, and tightening up join-point/autosave behavior and debugging support across client/server.
Changes:
- Add
StandalonePersistenceand server bootstrap logic to load/seed durable Saved/ state and persist join points/snapshots. - Introduce typed packets and plumbing for standalone autosave/join-point reasons and standalone world/map snapshot uploads.
- Apply a set of join/control/reconnect/debugging adjustments (protocol OK metadata, command execution catch-up, extra diagnostics).
Reviewed changes
Copilot reviewed 30 out of 31 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| Source/Tests/StandalonePersistenceTest.cs | Adds tests validating standalone join-point persistence and tick reload behavior. |
| Source/Tests/PacketTest.cs | Updates packet roundtrip coverage for the updated ServerProtocolOkPacket. |
| Source/Tests/packet-serializations/ServerProtocolOkPacket.verified.txt | Updates verified serialization output for the extended protocol-ok packet. |
| Source/Server/Server.cs | Switches standalone bootstrap to load from Saved/, seed from save.zip, and set debug logging flags. |
| Source/Common/WorldData.cs | Adds standalone snapshot state + hash validation, standalone persistence write-on-join-point, and tick basis changes. |
| Source/Common/StandalonePersistence.cs | New durable persistence manager for Saved/ with temp-file writes, seeding, load, and cleanup. |
| Source/Common/ServerSettings.cs | Adds EnforceStandaloneRequirements for standalone-specific setting adjustments. |
| Source/Common/PlayerManager.cs | Adjusts standalone join faction assignment for the first non-arbiter player. |
| Source/Common/Networking/State/ServerPlayingState.cs | Broadens standalone upload policy, adds standalone snapshot handlers, and switches autosaving handling to a typed packet with reasons. |
| Source/Common/Networking/State/ServerJoiningState.cs | Avoids join-point blocking for first standalone join; extends protocol OK to include standalone flag. |
| Source/Common/Networking/Packets.cs | Adds packet IDs for standalone snapshot uploads. |
| Source/Common/Networking/Packet/StandaloneSnapshotPackets.cs | New packet definitions for standalone world/map snapshot uploads (fragmented). |
| Source/Common/Networking/Packet/ProtocolPacket.cs | Extends ServerProtocolOkPacket with an isStandaloneServer flag. |
| Source/Common/Networking/Packet/AutosavingPacket.cs | New typed ClientAutosavingPacket carrying a JoinPointRequestReason. |
| Source/Common/MultiplayerServer.cs | Adds StandalonePersistence? persistence on the server. |
| Source/Common/JoinPointRequestReason.cs | New enum for autosave/join-point request reasons. |
| Source/Client/Windows/SaveGameWindow.cs | On standalone, triggers join-point/save signaling without writing a local replay zip. |
| Source/Client/Windows/BootstrapConfiguratorWindow.SettingsUi.cs | Enforces standalone requirements during settings UI + upload. |
| Source/Client/Windows/BootstrapConfiguratorWindow.cs | Enforces standalone requirements in bootstrap defaults. |
| Source/Client/Session/MultiplayerSession.cs | Tracks whether the connected server is standalone. |
| Source/Client/Session/Autosaving.cs | Makes save return success/failure, sends typed autosaving packets, and uploads standalone snapshots after autosave. |
| Source/Client/Saving/SaveLoad.cs | Implements standalone world/map snapshot creation, hashing, and upload. |
| Source/Client/Patches/VTRSyncPatch.cs | Triggers join-point requests on standalone world travel transitions. |
| Source/Client/Patches/TickPatch.cs | Executes queued commands with <= tick handling and adds stale-cmd warnings in dev mode. |
| Source/Client/Patches/GravshipTravelSessionPatches.cs | Adjusts player control behavior in gravship landing logic. |
| Source/Client/Patches/Designators.cs | Adds WarningOnce diagnostics when designators run while InInterface=false. |
| Source/Client/Networking/State/ClientJoiningState.cs | Applies isStandaloneServer from ServerProtocolOkPacket into session state. |
| Source/Client/MultiplayerGame.cs | Ensures FactionManager.ofPlayer is updated when changing real player faction. |
| Source/Client/ConstantTicker.cs | Adds a standalone-only synthetic autosave timer. |
| Source/Client/AsyncTime/AsyncWorldTimeComp.cs | Uploads standalone snapshots after join-point creation when connected to standalone. |
| .gitignore | Ignores *.lscache. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // File.Move with overwrite is .NET 5+; Common targets .NET Framework 4.8 | ||
| if (File.Exists(targetPath)) | ||
| File.Delete(targetPath); | ||
| File.Move(tmpPath, targetPath); |
There was a problem hiding this comment.
AtomicWrite is not actually atomic/crash-safe: it deletes the existing target file before moving the temp file. If the process crashes between File.Delete and File.Move, the persisted state can be lost/corrupted, which defeats the crash-safety goal stated in the class docs.
Consider using File.Replace(tmpPath, targetPath, backupPath: null) when the target exists (and File.Move when it doesn’t), or another true atomic replace strategy appropriate for the supported OSes/FSes.
| // File.Move with overwrite is .NET 5+; Common targets .NET Framework 4.8 | |
| if (File.Exists(targetPath)) | |
| File.Delete(targetPath); | |
| File.Move(tmpPath, targetPath); | |
| // File.Move with overwrite is .NET 5+; Common targets .NET Framework 4.8. | |
| // Use File.Replace when the destination already exists so the update is | |
| // an atomic replace rather than a delete-then-move sequence. | |
| if (File.Exists(targetPath)) | |
| File.Replace(tmpPath, targetPath, null); | |
| else | |
| File.Move(tmpPath, targetPath); |
| public void Bind(PacketBuffer buf) | ||
| { | ||
| buf.Bind(ref hasPassword); | ||
| buf.Bind(ref isStandaloneServer); |
There was a problem hiding this comment.
ServerProtocolOkPacket now always binds isStandaloneServer. Because packet deserialization doesn’t enforce full consumption, old clients can ignore the extra byte, but new clients connecting to an older server will attempt to read a bool that isn’t present and likely throw.
To keep backward compatibility without forcing a protocol bump, gate the second bind behind if (buf.DataRemaining) (similar to ClientUsernamePacket). Otherwise, bump MpVersion.Protocol alongside this schema change.
| buf.Bind(ref isStandaloneServer); | |
| if (buf.DataRemaining) | |
| buf.Bind(ref isStandaloneServer); |
| public static bool SaveGameToFile_Overwrite(string fileNameNoExtension, bool currentReplay) | ||
| { | ||
| Log.Message($"Multiplayer: saving to file {fileNameNoExtension}"); | ||
|
|
||
| try | ||
| { | ||
| var tmp = new FileInfo(Path.Combine(Multiplayer.ReplaysDir, $"{fileNameNoExtension}.tmp.zip")); | ||
| if (tmp.Exists) | ||
| tmp.Delete(); | ||
|
|
||
| Replay.ForSaving(tmp).WriteData( | ||
| currentReplay ? | ||
| Multiplayer.session.dataSnapshot : | ||
| SaveLoad.CreateGameDataSnapshot(SaveLoad.SaveGameData(), false) | ||
| ); | ||
|
|
||
| var dst = new FileInfo(Path.Combine(Multiplayer.ReplaysDir, $"{fileNameNoExtension}.zip")); | ||
| if (!dst.Exists) dst.Open(FileMode.Create).Close(); | ||
| tmp.Replace(dst.FullName, null); | ||
| if (dst.Exists) | ||
| dst.Delete(); | ||
|
|
||
| tmp.MoveTo(dst.FullName); | ||
|
|
||
| Messages.Message("MpGameSaved".Translate(fileNameNoExtension), MessageTypeDefOf.SilentInput, false); | ||
| Multiplayer.session.lastSaveAt = Time.realtimeSinceStartup; | ||
| return true; | ||
| } | ||
| catch (Exception e) | ||
| { | ||
| Log.Error($"Exception saving multiplayer game as {fileNameNoExtension}: {e}"); | ||
| Messages.Message("MpGameSaveFailed".Translate(), MessageTypeDefOf.SilentInput, false); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
SaveGameToFile_Overwrite now swallows all exceptions and returns false instead. There are existing call sites that rely on exceptions and don’t check the return value (e.g. BootstrapConfiguratorWindow.BootstrapFlow.cs:285-288), which will now proceed as if a save succeeded even when it failed.
Either restore exception propagation (and keep the bool return only for callers that want it), or update all call sites to check the return value and abort their workflows on failure.
| __result = true; | ||
| return false; |
There was a problem hiding this comment.
The comment says "use vanilla logic for player control", but the code forces __result = true and skips the original getter. This changes behavior beyond the landing-session message suppression and may re-enable control in situations where vanilla would intentionally disable it.
If the intent is to keep vanilla behavior outside landing confirmation, return true to run the original getter (or set __result = Current.Game.PlayerHasControl). If the intent is to always allow control, update the comment to match.
| __result = true; | |
| return false; | |
| return true; |
| if (!Multiplayer.InInterface) | ||
| { | ||
| Log.WarningOnce( | ||
| "Multiplayer: blocked DesignateSingleCell because InInterface=false " + | ||
| $"designator={__instance.GetType().Name}, reloading={Multiplayer.reloading}, ticking={Multiplayer.Ticking}, " + | ||
| $"executingCmds={Multiplayer.ExecutingCmds}, simulating={TickPatch.Simulating}, frozen={TickPatch.Frozen}, " + | ||
| $"hasCurrentEvent={LongEventHandler.currentEvent != null}, programState={Current.ProgramState}", | ||
| 18000101); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
The warning message says the action was "blocked", but this prefix returns true, which allows the original DesignateSingleCell to run (i.e., it isn’t blocked). This is misleading when diagnosing desyncs.
Either change the log text to reflect that sync interception was skipped (not blocked), or return false if the designator really should be blocked when InInterface is false.
| if (!Multiplayer.InInterface) | ||
| { | ||
| Log.WarningOnce( | ||
| "Multiplayer: blocked DesignateMultiCell because InInterface=false " + | ||
| $"designator={__instance.GetType().Name}, reloading={Multiplayer.reloading}, ticking={Multiplayer.Ticking}, " + | ||
| $"executingCmds={Multiplayer.ExecutingCmds}, simulating={TickPatch.Simulating}, frozen={TickPatch.Frozen}, " + | ||
| $"hasCurrentEvent={LongEventHandler.currentEvent != null}, programState={Current.ProgramState}", | ||
| 18000102); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Same issue as above: log says "blocked" but returning true allows the original DesignateMultiCell to run. This message is misleading for debugging.
Adjust the message or change the return value depending on the intended behavior.
| if (!Multiplayer.InInterface) | ||
| { | ||
| Log.WarningOnce( | ||
| "Multiplayer: blocked DesignateThing because InInterface=false " + | ||
| $"designator={__instance.GetType().Name}, thing={__0}, reloading={Multiplayer.reloading}, ticking={Multiplayer.Ticking}, " + | ||
| $"executingCmds={Multiplayer.ExecutingCmds}, simulating={TickPatch.Simulating}, frozen={TickPatch.Frozen}, " + | ||
| $"hasCurrentEvent={LongEventHandler.currentEvent != null}, programState={Current.ProgramState}", | ||
| 18000103); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Same issue as above: log says "blocked" but returning true allows the original DesignateThing to run. This message is misleading for debugging.
Adjust the message or change the return value depending on the intended behavior.
| public struct StandaloneWorldSnapshotState | ||
| { | ||
| public int tick; | ||
| public int leaseVersion; | ||
| public int producerPlayerId; | ||
| public string producerUsername; | ||
| public byte[] sha256Hash; | ||
| } | ||
|
|
||
| public struct StandaloneMapSnapshotState | ||
| { | ||
| public int tick; | ||
| public int leaseVersion; | ||
| public int producerPlayerId; | ||
| public string producerUsername; | ||
| public byte[] sha256Hash; | ||
| } |
There was a problem hiding this comment.
These structs have non-nullable reference fields (producerUsername, sha256Hash) but are instantiated via new()/object initializers that don’t always set them (e.g. StandalonePersistence.LoadInto sets only tick). Under nullable reference types this can lead to nulls in non-nullable fields and future NREs.
Mark these fields as nullable (string?, byte[]?) or initialize them to safe defaults (e.g. string.Empty / Array.Empty<byte>()).
| public static void DoAutosave() | ||
| { | ||
| LongEventHandler.QueueLongEvent(() => | ||
| { | ||
| SaveGameToFile_Overwrite(GetNextAutosaveFileName(), false); | ||
| Multiplayer.Client.Send(Packets.Client_Autosaving); | ||
| if (!SaveGameToFile_Overwrite(GetNextAutosaveFileName(), false)) | ||
| return; | ||
|
|
||
| Multiplayer.Client.Send(new ClientAutosavingPacket(JoinPointRequestReason.Save)); | ||
|
|
||
| // When connected to a standalone server, also upload fresh snapshots | ||
| if (Multiplayer.session?.ConnectedToStandaloneServer == true) | ||
| { | ||
| var snapshot = SaveLoad.CreateGameDataSnapshot(SaveLoad.SaveGameData(), false); | ||
| SaveLoad.SendStandaloneMapSnapshots(snapshot); | ||
| SaveLoad.SendStandaloneWorldSnapshot(snapshot); | ||
| } |
There was a problem hiding this comment.
In the standalone-server path, DoAutosave currently generates game data twice: once inside SaveGameToFile_Overwrite (via SaveLoad.SaveGameData()), and again immediately after to build the snapshot for upload. Creating snapshots is typically expensive and this doubles autosave cost.
Consider having SaveGameToFile_Overwrite optionally return the GameDataSnapshot it wrote (or accept one as an argument), so the same snapshot can be reused for standalone snapshot uploads.
ffcda21 to
22fa058
Compare
22fa058 to
720fccc
Compare
This PR contains the final standalone server maintenance and prepublish cleanup on top of the earlier standalone server slices.
What this branch does:
Notes:
Validation: