Conversation
There was a problem hiding this comment.
Pull request overview
Adds a durable “Saved/” persistence layer and standalone snapshot upload flow so a standalone server can persist join points/snapshots and restore state after restart.
Changes:
- Introduces
StandalonePersistenceto manage on-disk Saved/ state (seed/load/write/cleanup). - Adds standalone snapshot upload packets + server/client wiring (autosave/save/world-travel triggers).
- Extends protocol handshake to tell clients they’re connected to a standalone server.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| Source/Tests/StandalonePersistenceTest.cs | Adds coverage for join-point persistence + tick fallback behavior. |
| Source/Tests/PacketTest.cs | Updates packet roundtrip list for new ServerProtocolOkPacket shape. |
| Source/Tests/packet-serializations/ServerProtocolOkPacket.verified.txt | Updates verified bytes to include the new boolean field. |
| Source/Server/Server.cs | Wires standalone persistence into server startup and load/seed logic. |
| Source/Common/WorldData.cs | Adds standalone snapshot state + acceptance/persistence hooks; join point tick handling for standalone. |
| Source/Common/StandalonePersistence.cs | New persistence implementation for Saved/ directory management. |
| Source/Common/PlayerManager.cs | Adjusts faction assignment for standalone “primary” player joining. |
| Source/Common/Networking/State/ServerPlayingState.cs | Accepts standalone snapshot uploads; adjusts autosave handling and world upload policy for standalone. |
| Source/Common/Networking/State/ServerJoiningState.cs | Standalone join-point request behavior; sends standalone flag in protocol OK. |
| Source/Common/Networking/Packets.cs | Adds packet IDs for standalone snapshot uploads. |
| Source/Common/Networking/Packet/StandaloneSnapshotPackets.cs | New packet definitions for world/map snapshot upload. |
| Source/Common/Networking/Packet/ProtocolPacket.cs | Extends ServerProtocolOkPacket with standalone flag. |
| Source/Common/Networking/Packet/AutosavingPacket.cs | New typed ClientAutosavingPacket with reason enum. |
| Source/Common/MultiplayerServer.cs | Adds StandalonePersistence field to server. |
| Source/Common/JoinPointRequestReason.cs | New enum for autosave/join-point trigger reasons. |
| Source/Client/Windows/SaveGameWindow.cs | Sends autosave/join-point request on manual save; special-cases standalone. |
| Source/Client/Session/MultiplayerSession.cs | Tracks whether connection is to a standalone server. |
| Source/Client/Session/Autosaving.cs | Sends typed autosaving packet; uploads standalone snapshots after autosave; makes save return bool. |
| Source/Client/Saving/SaveLoad.cs | Implements snapshot compression + SHA256 hashing + fragmented upload for standalone. |
| Source/Client/Patches/VTRSyncPatch.cs | Triggers join point request on world travel when standalone-connected. |
| Source/Client/Networking/State/ClientJoiningState.cs | Reads standalone flag from ServerProtocolOkPacket. |
| Source/Client/ConstantTicker.cs | Adds synthetic autosave timer for standalone connections. |
| Source/Client/AsyncTime/AsyncWorldTimeComp.cs | Uploads standalone snapshots when a join point snapshot is created/sent. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Atomic write: write to .tmp, then rename over the target. | ||
| /// </summary> | ||
| private static void AtomicWrite(string targetPath, byte[] data) | ||
| { | ||
| var tmpPath = targetPath + ".tmp"; | ||
| File.WriteAllBytes(tmpPath, data); | ||
|
|
||
| // 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 isn’t actually atomic: deleting the target and then moving the .tmp introduces a window where a crash (or IO error) can leave the file missing/corrupted. On .NET Framework 4.8 you can use File.Replace(tmpPath, targetPath, backupPath: null) when the target exists (and File.Move when it doesn’t) to get an atomic swap without a delete gap.
| /// Atomic write: write to .tmp, then rename over the target. | |
| /// </summary> | |
| private static void AtomicWrite(string targetPath, byte[] data) | |
| { | |
| var tmpPath = targetPath + ".tmp"; | |
| File.WriteAllBytes(tmpPath, data); | |
| // File.Move with overwrite is .NET 5+; Common targets .NET Framework 4.8 | |
| if (File.Exists(targetPath)) | |
| File.Delete(targetPath); | |
| File.Move(tmpPath, targetPath); | |
| /// Atomic write: write to .tmp, then atomically replace the target. | |
| /// </summary> | |
| private static void AtomicWrite(string targetPath, byte[] data) | |
| { | |
| var tmpPath = targetPath + ".tmp"; | |
| File.WriteAllBytes(tmpPath, data); | |
| // File.Move with overwrite is .NET 5+; Common targets .NET Framework 4.8. | |
| // Use File.Replace for an atomic swap when the destination already exists. | |
| if (File.Exists(targetPath)) | |
| File.Replace(tmpPath, targetPath, null); | |
| else | |
| File.Move(tmpPath, targetPath); |
| // Persist to disk | ||
| Server.persistence?.WriteWorldSnapshot(worldSnapshot, sessionSnapshot, tick); | ||
|
|
||
| return true; |
There was a problem hiding this comment.
TryAcceptStandaloneWorldSnapshot writes to disk without exception handling. If the filesystem is read-only/full (or transient IO errors occur), this will throw out of the packet handler and can destabilize the server connection loop. Consider wrapping the persistence write in try/catch (similar to EndJoinPointCreation) and returning false/logging when persistence fails.
| // Persist to disk | ||
| Server.persistence?.WriteMapSnapshot(mapId, mapSnapshot); | ||
|
|
||
| return true; |
There was a problem hiding this comment.
TryAcceptStandaloneMapSnapshot writes to disk without exception handling. An IO exception here would bubble up from the packet handler; it’s safer to catch/log and reject (or accept but mark dirty) rather than risking server instability on disk failures.
| public bool TryAcceptStandaloneWorldSnapshot(ServerPlayer player, int tick, int leaseVersion, byte[] worldSnapshot, | ||
| byte[] sessionSnapshot, byte[] expectedHash) | ||
| { | ||
| if (tick < standaloneWorldSnapshot.tick) | ||
| return false; | ||
|
|
There was a problem hiding this comment.
Snapshot acceptance ignores leaseVersion: a snapshot with the same tick as the current one will be accepted and overwrite state regardless of whether it’s stale or from a different producer. Since leaseVersion is already part of the packet/state, consider requiring tick to strictly increase, or (tick == currentTick) only accepting if leaseVersion is greater than the stored leaseVersion (and/or validating the current lease owner).
| public void Bind(PacketBuffer buf) | ||
| { | ||
| buf.Bind(ref hasPassword); | ||
| buf.Bind(ref isStandaloneServer); |
There was a problem hiding this comment.
ServerProtocolOkPacket wire format changed (extra bool) but Bind always reads/writes both fields. If MpVersion.Protocol isn’t bumped, older clients/servers with the same protocol number will fail decoding and likely disconnect/crash. Either bump MpVersion.Protocol for this change, or make Bind backward-compatible (e.g., bind hasPassword first and only bind isStandaloneServer when buf.DataRemaining).
| buf.Bind(ref isStandaloneServer); | |
| if (buf.DataRemaining) | |
| buf.Bind(ref isStandaloneServer); |
| public JoinPointRequestReason reason = reason; | ||
|
|
||
| public void Bind(PacketBuffer buf) | ||
| { |
There was a problem hiding this comment.
ClientAutosavingPacket now carries a payload but Bind always reads it. Without a protocol bump, an older client that sends the legacy empty Client_Autosaving packet will cause the server to read past the end of the buffer. Consider either bumping MpVersion.Protocol, or making Bind tolerant (e.g., if (!buf.DataRemaining) reason = JoinPointRequestReason.Unknown).
| { | |
| { | |
| if (!buf.DataRemaining) | |
| { | |
| reason = JoinPointRequestReason.Unknown; | |
| return; | |
| } |
| if (!SaveGameToFile_Overwrite(GetNextAutosaveFileName(), false)) | ||
| return; | ||
|
|
||
| Multiplayer.Client.Send(new ClientAutosavingPacket(JoinPointRequestReason.Save)); |
There was a problem hiding this comment.
DoAutosave sends JoinPointRequestReason.Save, and ServerPlayingState treats reason==Save as forceJoinPoint (bypassing the join point cooldown). That changes autosave behavior vs. the previous implementation (autosaves were not forced). Consider adding an Autosave enum value (or using Unknown) so only manual saves force join points.
| Multiplayer.Client.Send(new ClientAutosavingPacket(JoinPointRequestReason.Save)); | |
| Multiplayer.Client.Send(new ClientAutosavingPacket(JoinPointRequestReason.Unknown)); |
| 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.
StandaloneWorldSnapshotState/StandaloneMapSnapshotState declare non-nullable reference fields (producerUsername, sha256Hash) but they’ll be null in the default struct value (e.g., the 'new()' initializers above). With enable this produces warnings and can become a runtime NRE if these fields are read before a snapshot is accepted. Consider making these fields nullable or initializing them to "" / Array.Empty() via a constructor/defaults.
This PR adds the standalone snapshot persistence layer on top of the previous standalone save-trigger foundation.
What this branch does:
Notes:
Validation: