Skip to content

Stop player save snapshots sharing mutable state with the player - #1253

Merged
GregHib merged 1 commit into
GregHib:mainfrom
HarleyGilpin:fix/player-save-friends-aliasing
Sep 2, 2026
Merged

Stop player save snapshots sharing mutable state with the player#1253
GregHib merged 1 commit into
GregHib:mainfrom
HarleyGilpin:fix/player-save-friends-aliasing

Conversation

@HarleyGilpin

Copy link
Copy Markdown
Contributor

Partially addresses #1252. The atomic write half of that issue is untouched, so it stays open.

Player.copy() builds the snapshot that gets serialized on Dispatchers.IO. Every field is defensively copied except two:

variables = variables.data.toMap(),
inventories = inventories.instances.mapValues { it.value.items.map { itm -> itm.copy() }.toTypedArray() },
friends = friends,
ignores = ignores.toList(),
offers = offers.copyOf(),

friends is val friends: MutableMap<String, ClanRank> = mutableMapOf() (Player.kt:44), handed straight to the IO thread by reference. The game thread keeps writing to it: FriendsList.kt:67 and ClanChat.kt:106 both do player.friends[account.accountName] = .... Adding a friend during a save can throw ConcurrentModificationException while the file is being written.

offers.copyOf() copies the array but not the ExchangeOffer elements, and state, completed and coins are var, so a GE update mid-save tears the snapshot.

Neither fails loudly. The friends case throws on the IO thread after Config.fileWriter has truncated the target, which is the part that turns it into a corrupt save rather than a retried one. That interaction is why #1252 covers both.

friends = friends.toMap(),
offers = Array(offers.size) { offers[it].copy() },

ExchangeOffer is a data class whose remaining fields are val, so an elementwise copy() fully detaches it. ExchangeHistory is entirely val, so history.toList() was already safe and is left as is.

Both tests fail without the change:

Snapshot changed after the player edited their friends list. Expected <{first=Friend}>, actual <{first=Friend, second=Friend}>.
Snapshot changed after the player's offer progressed. Expected <0>, actual <7>.

Full suite green.

Player.copy() defensively copies every field except friends, which was
passed by reference, and offers, where copyOf() copies the array but not
the ExchangeOffer elements behind it.

The snapshot is serialized on Dispatchers.IO while the game thread can
still write to both. FriendsList and ClanChat assign into player.friends,
and an offer's state, completed and coins are var, so a save can observe
a half-updated snapshot or throw ConcurrentModificationException - after
Config.fileWriter has already truncated the target file.

friends is copied with toMap() and offers elementwise. ExchangeHistory
is entirely val, so history.toList() is already safe.
@GregHib
GregHib merged commit fd77946 into GregHib:main Sep 2, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants