A single failed save diverts every pending account into SafeStorage, which cannot be read back

[HarleyGilpin]

SaveQueue's exception handler closes over pending rather than the batch that failed:

private val handler = CoroutineExceptionHandler { _, exception ->
    logger.error(exception) { "Error saving players!" }
    scope.fallback(pending.values.toList())
}

run() snapshots pending.values.toList() and hands that list to storage.save. If the write throws, the handler ignores the failed list and re-reads pending, which by then also holds accounts queued after the snapshot was taken — ones storage was never asked to write. All of them go to fallback, which writes them out and clears them:

private fun CoroutineScope.fallback(accounts: List<PlayerSave>) = launch(fallbackHandler) {
    withContext(NonCancellable) {
        fallback.save(accounts)
        clearPending(accounts)
    }
}

The fallback is SafeStorage (EngineModules.kt:34), and it is write-only:

override fun exists(accountName: String): Boolean = false
override fun load(accountName: String): PlayerSave? = null
override fun names(): Map<String, AccountDefinition> = emptyMap()

It writes to a timestamped filename, "$current-${account.name}.toml", so it isn't a save file the server would find even if it did read the directory.

So one failed batch means those accounts are removed from pending, never retried against real storage, and invisible to every later load. The next login reads whatever the account file held before the session. Nothing surfaces to the player, and the only trace is Error saving players!.

The blast radius is wider than the failure. A transient error, disk momentarily full or a permissions flip, takes out every account queued at that moment rather than the one batch that failed.

#1212 has a real instance of the trigger: a host batch job stalled the machine for about two minutes, ticks reached Tick 1402 took 25918ms, and overlapping TRANSACTION_SERIALIZABLE upserts conflicted in DatabaseStorage.kt:591.

Two parts to fixing it:

Carry the failed batch through to the handler instead of re-reading pending, so a failure only touches the accounts that were actually attempted.

Decide what the fallback means. Right now it acts as an authoritative handoff, but nothing can read it back, so it behaves as data loss. Leaving the entries in pending after dumping them would let the next tick retry real storage and self-heal a transient error, at the cost of a SafeStorage file per tick per account while storage stays broken. A bounded retry before giving up would cap that. Worth deciding which of those you want, since it changes what the existing test Failed save falls back and doesn't kill the queue should assert about queue.empty().

Found while working on #1249, which touches clearPending and direct() in the same file but deliberately leaves this alone.