Player saves are written non-atomically, and PlayerSave.copy() aliases the live friends map

[HarleyGilpin]

Two separate problems that combine badly: a save can throw partway through writing, and the file it's writing has already been truncated.

The write isn't atomic

PlayerSave.save(file) goes through Config.fileWriter:

fun fileWriter(file: File, block: ConfigWriter.() -> Unit) {
    BufferedWriter(FileWriter(file)).use { output ->
        block.invoke(output)
    }
}

FileWriter(file) truncates the target the moment it opens. There's no temp file and rename, no fsync. If the block throws, or the process dies mid-write, the player's live save is left truncated at whatever had flushed.

save writes accountName, passwordHash, experience, levels and the tile before it reaches variables and inventories, so a truncation partway through tends to leave a file that still parses as a plausible account with a position but no items.

If it lands somewhere that doesn't parse, PlayerSave.load throws IllegalArgumentException (there are 8 require/throw sites in it), but the login path only catches IllegalStateException:

} catch (e: IllegalStateException) {
    logger.trace(e) { "Error loading player account" }
    client.disconnect(Response.COULD_NOT_COMPLETE_LOGIN)
    return null
}

so it escapes uncaught into LoginServer.login.

The snapshot shares mutable state with the live player

Player.copy() defensively copies every field except one:

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() on Player.kt:44, passed straight through by reference. The snapshot is handed to a coroutine on Dispatchers.IO and serialized there, while the game thread can still write to that same map: FriendsList.kt:67 and ClanChat.kt:106 both do player.friends[account.accountName] = ....

Adding a friend while the save is being written can therefore throw ConcurrentModificationException on the IO thread, after FileWriter has already truncated the file. That's the case where the two problems meet.

offers.copyOf() is a shallow array copy over ExchangeOffer, whose state, completed and coins are var, so a GE update during a save can tear the snapshot the same way, with lower stakes. variables.data.toMap() is also shallow, so any variable whose value is a mutable collection stays shared.

Fixing it

friends = friends.toMap() and copying the offers elementwise closes the aliasing. That alone removes the most likely way to throw mid-write.

For atomicity I'd add a separate function rather than changing Config.fileWriter, which is used for definitions, GE and reports across the tree, and have PlayerSave.save call that one: write to a temp file in the same directory, flush, fd.sync(), then Files.move with ATOMIC_MOVE and REPLACE_EXISTING. A failure then leaves the previous complete file in place because the temp is never renamed.

Worth deciding: whether to keep that narrow or make all config writes atomic, and whether the per-file fsync is acceptable at your expected population, since it's a few ms per file and will show up in the Saved N accounts in Xms line. It is off the game thread, and #1249 makes the write one-in-flight again.

SafeStorage uses file.writeText throughout with the same non-atomicity, but nothing reads those files back (see #1250), so it doesn't matter until that's resolved.

Found while working on #1249, which leaves both of these alone.