fix(account): preserve then rebuild an unreadable account store

src/global.d.ts

@@ -79,10 +79,19 @@ declare global {
    * back, so the player's password was never actually rejected. Collapsing
    * that case into `invalid-credentials` is exactly the bug this type exists
    * to make impossible again.
+   *
+   * `session-store-unreadable` is the same kind of honesty for a different
+   * failure: the credentials were accepted, but the local secret store could
+   * not be read or safely rebuilt, so nothing was saved. `storeRebuilt` on
+   * `success` is not a status of its own on purpose: the login still
+   * succeeded, and a separate status would make every `status === "success"`
+   * check silently drop the account. It flags that the store had to be
+   * rebuilt around this one login, so a previously unreadable file's other
+   * saved accounts are gone and will need to log in again.
    */
   type AccountLoginResult =
-    | { status: "success"; account: AccountPublicType }
-    | { status: "invalid-credentials" | "requires-two-factor" | "wrong-two-factor" | "unexpected-response"; account?: undefined }
+    | { status: "success"; account: AccountPublicType; storeRebuilt?: boolean }
+    | { status: "invalid-credentials" | "requires-two-factor" | "wrong-two-factor" | "unexpected-response" | "session-store-unreadable"; account?: undefined }
 
   type GameVersionType = {
     version: string

src/ipc/accountStore.ts

@@ -4,6 +4,7 @@ import { join } from "node:path"
 
 import { parseStoredSecrets, parseStoredSecretsById, type AccountSecrets, type StoredAccountSecretsEntry } from "@domain/account/credentials"
 import { writeJsonAtomic } from "@src/ipc/atomicJsonFile"
+import { isRecord } from "@src/ipc/validation"
 
 type EncryptedAccountFile = {
   version: 2
@@ -25,13 +26,15 @@ const ACCOUNT_STORE_VERSION = 2
 const LEGACY_ACCOUNT_STORE_VERSION = 1
 
 /**
- * `undefined` means the file has not been read this process. A read that
- * cannot be decrypted, or finds nothing there, caches an empty map rather
- * than `undefined`, so a later call answers from memory instead of hitting
- * the disk again. A miss on one account cannot poison a later hit on another,
- * because the cache holds the whole file's contents, not one account's.
+ * `undefined` means the file has not been read this process. `unreadable`
+ * says whether the file existed but could not be trusted (wrong version,
+ * bad JSON, decrypt failure, a payload with no accounts list): a genuinely
+ * absent file is not unreadable, it is the ordinary no-accounts-yet case,
+ * and `readStore` never sets the flag for it. `saveAccountSecrets` is the
+ * only reader of the flag; every other caller only ever wants the map.
  */
-let cachedAccounts: Map<string, AccountSecrets> | undefined
+type StoreRead = { accounts: Map<string, AccountSecrets>; unreadable: boolean }
+let cachedRead: StoreRead | undefined
 
 function getAccountStorePath(): string {
   return join(app.getPath("userData"), "account-secrets.json")
@@ -49,38 +52,101 @@ function getAccountStoreBackupPath(): string {
   return join(app.getPath("userData"), "account-secrets.pre-migration.bak.json")
 }
 
+/**
+ * Where an unreadable store's bytes are kept before {@link saveAccountSecrets}
+ * rebuilds the file around a new login. A separate file from the
+ * pre-migration backup above on purpose: those are two different events (an
+ * old-format file being upgraded, versus a current-format file that stopped
+ * decrypting), and sharing one path would let whichever happens second
+ * silently erase the first one's snapshot.
+ */
+function getUnreadableStoreBackupPath(): string {
+  return join(app.getPath("userData"), "account-secrets.unreadable.bak.json")
+}
+
+/** The unreadable store's bytes could not be copied aside, so nothing was written over it. */
+export class AccountStoreUnreadableError extends Error {
+  constructor(cause?: unknown) {
+    super("The account store is unreadable and its bytes could not be preserved", { cause })
+    this.name = "AccountStoreUnreadableError"
+  }
+}
+
+function isMissingFileError(error: unknown): boolean {
+  return isRecord(error) && error.code === "ENOENT"
+}
+
 function assertSecureStorage(): void {
   if (!safeStorage.isEncryptionAvailable()) throw new Error("Secure account storage is unavailable")
   if (process.platform === "linux" && safeStorage.getSelectedStorageBackend() === "basic_text") throw new Error("A system password store is required for account storage")
 }
 
-async function readAccounts(): Promise<Map<string, AccountSecrets>> {
-  if (cachedAccounts !== undefined) return cachedAccounts
+async function readStore(): Promise<StoreRead> {
+  if (cachedRead !== undefined) return cachedRead
 
   try {
     assertSecureStorage()
+  } catch {
+    // Not the file's fault, and not cached: the keyring becoming available later must still
+    // reach the real file, and writeAccounts asserts the same thing before it could ever
+    // overwrite anything, so no save can destroy the store while this is true.
+    return { accounts: new Map(), unreadable: false }
+  }
+
+  try {
     const stored = (await fse.readJSON(getAccountStorePath(), "utf8")) as Partial<EncryptedAccountFile>
     if (stored.version !== ACCOUNT_STORE_VERSION || typeof stored.ciphertext !== "string") throw new Error("Invalid account store")
 
     const decrypted = safeStorage.decryptString(Buffer.from(stored.ciphertext, "base64"))
-    cachedAccounts = parseStoredSecretsById(JSON.parse(decrypted))
-  } catch {
-    cachedAccounts = new Map()
+    const payload: unknown = JSON.parse(decrypted)
+    // An entry parseStoredSecretsById itself drops is not corruption: a file holding one
+    // broken entry beside three good ones is still a store worth writing to. A payload with
+    // no accounts list at all is the file failing to be this store's shape in the first place.
+    if (!isRecord(payload) || !Array.isArray(payload.accounts)) throw new Error("Invalid account store payload")
+    cachedRead = { accounts: parseStoredSecretsById(payload), unreadable: false }
+  } catch (error) {
+    cachedRead = { accounts: new Map(), unreadable: !isMissingFileError(error) }
   }
 
-  return cachedAccounts
+  return cachedRead
+}
+
+async function readAccounts(): Promise<Map<string, AccountSecrets>> {
+  return (await readStore()).accounts
+}
+
+/**
+ * Copies the current, unreadable store file aside once, so a rebuild around a
+ * new login never destroys bytes that might still hold other accounts'
+ * sessions. `overwrite: false` keeps the first snapshot and skips every later
+ * one. That is a deliberate one-shot: a second corruption event can land after
+ * the store has rebuilt and grown, so the skipped snapshot sometimes holds
+ * more than the kept one, which for a version-mismatch file (recoverable by
+ * the newer build that wrote it) is a real loss. The trade is accepted here
+ * because a single predictable recovery file beats an unbounded pile of them,
+ * and nothing in the app surfaces or clears these yet regardless (tracked as a
+ * follow-up on #259).
+ *
+ * Throws {@link AccountStoreUnreadableError} when the copy itself fails (a
+ * permissions problem, most likely): that is the one case where proceeding
+ * to rebuild would destroy bytes rather than merely fail to read them, so
+ * the caller must not write anything.
+ */
+async function preserveUnreadableStore(): Promise<void> {
+  try {
+    await fse.copy(getAccountStorePath(), getUnreadableStoreBackupPath(), { overwrite: false, errorOnExist: false })
+  } catch (error) {
+    if (isMissingFileError(error)) return // Vanished since the read; nothing left to preserve.
+    throw new AccountStoreUnreadableError(error)
+  }
 }
 
 /**
  * Encrypts and writes the whole account map, atomically.
  *
- * If the file on disk exists but cannot be decrypted, this still overwrites
- * it: the old bytes are already unrecoverable, so refusing to write would
- * only leave the launcher permanently unable to save any account. At
- * multi-account scale this means one undecryptable file now costs every
- * saved account's session instead of the one account the old single-account
- * store held, which is a real change in blast radius worth this comment,
- * even though it is not a new kind of risk.
+ * Callers decide what "the whole account map" should contain before calling
+ * this; it always overwrites the file with exactly what it is given. See
+ * {@link saveAccountSecrets} for the policy on when that is safe to do.
  */
 async function writeAccounts(accounts: Map<string, AccountSecrets>): Promise<void> {
   assertSecureStorage()
@@ -95,14 +161,39 @@ async function writeAccounts(accounts: Map<string, AccountSecrets>): Promise<voi
   // matching what this store did before.
   await writeJsonAtomic(storePath, contents, { mode: 0o600 })
   await fse.chmod(storePath, 0o600).catch(() => undefined)
-  cachedAccounts = accounts
+  cachedRead = { accounts, unreadable: false }
 }
 
-/** Saves or replaces one account's secrets. Logging into an already-saved account overwrites its entry: a session refresh, not a duplicate. */
-export async function saveAccountSecrets(accountId: string, secrets: AccountSecrets): Promise<void> {
-  const accounts = new Map(await readAccounts())
+/** What {@link saveAccountSecrets} actually did: a plain save, or a save that first had to rebuild an unreadable store around it. */
+export type AccountSaveOutcome = "saved" | "saved-after-rebuild"
+
+/**
+ * Saves or replaces one account's secrets. Logging into an already-saved
+ * account overwrites its entry: a session refresh, not a duplicate.
+ *
+ * When the store is present but unreadable, the sessions it held are already
+ * gone the moment it stopped decrypting: refusing this write would not bring
+ * any of them back, only leave the launcher unable to save any account ever
+ * again, with nothing in the app to clear the dead file for it. So this
+ * preserves the unreadable bytes once (see {@link preserveUnreadableStore})
+ * and rebuilds the store around just the account logging in now, the same
+ * "never destroy without a snapshot" rule the rest of this file already
+ * follows. The caller is told which happened, so a login that quietly wiped
+ * a housemate's session is never reported as an ordinary one.
+ */
+export async function saveAccountSecrets(accountId: string, secrets: AccountSecrets): Promise<AccountSaveOutcome> {
+  const store = await readStore()
+  const accounts = new Map(store.accounts)
   accounts.set(accountId, secrets)
+
+  if (!store.unreadable) {
+    await writeAccounts(accounts)
+    return "saved"
+  }
+
+  await preserveUnreadableStore()
   await writeAccounts(accounts)
+  return "saved-after-rebuild"
 }
 
 /** Reads one account's secrets, or null when nothing is stored for it. */
@@ -111,21 +202,31 @@ export async function getAccountSecrets(accountId: string): Promise<AccountSecre
 }
 
 /**
- * Drops one account's secrets. `true` when there was nothing to remove,
- * matching the old single-account store's `clearAccountSecrets`: a caller
- * asking to remove an account that already has no stored session is not a
- * failure. When the map empties, the file itself is removed rather than left
- * behind holding an empty list, so a never-used store and a fully-logged-out
- * one are byte-for-byte the same on disk.
+ * Drops one account's secrets. `true` when there was nothing to remove from a
+ * store that could actually be read, matching the old single-account store's
+ * `clearAccountSecrets`: a caller asking to remove an account that already has
+ * no stored session is not a failure. When the map empties, the file itself is
+ * removed rather than left behind holding an empty list, so a never-used store
+ * and a fully-logged-out one are byte-for-byte the same on disk.
+ *
+ * `false` when the account is not in the map only because the store could not
+ * be read (a locked keyring, or bytes that stopped decrypting). The entry may
+ * well be sitting in those bytes, so "nothing to remove" is not the truth, and
+ * reporting success would have the renderer drop the account from config and
+ * tell the player it is gone while its session is still on disk under a uid
+ * nothing names any more. Refusing keeps the account and surfaces the store
+ * problem instead. This never rebuilds the file: only `saveAccountSecrets`,
+ * running for a login the player actually asked for, does that.
  */
 export async function removeAccountSecrets(accountId: string): Promise<boolean> {
-  const accounts = new Map(await readAccounts())
-  if (!accounts.delete(accountId)) return true
+  const store = await readStore()
+  const accounts = new Map(store.accounts)
+  if (!accounts.delete(accountId)) return !store.unreadable
 
   try {
     if (accounts.size === 0) {
       await fse.remove(getAccountStorePath())
-      cachedAccounts = new Map()
+      cachedRead = { accounts: new Map(), unreadable: false }
     } else {
       await writeAccounts(accounts)
     }

src/ipc/handlers/accountHandlers.ts

@@ -2,13 +2,14 @@ import { ipcMain } from "electron"
 
 import { interpretFirstPass, interpretSecondPass } from "@domain/account/login"
 import type { LoginVerdict } from "@domain/account/login"
-import { badCredentialsResult, needsTwoFactorResult, twoFactorRejectedResult, unexpectedResponseOutcome } from "@src/ipc/handlers/accountLoginOutcome"
+import { badCredentialsResult, needsTwoFactorResult, sessionStoreUnreadableResult, twoFactorRejectedResult, unexpectedResponseOutcome } from "@src/ipc/handlers/accountLoginOutcome"
 import { buildLoginRequestBody } from "@src/ipc/handlers/loginRequestBody"
 import { IPC_CHANNELS } from "@src/ipc/ipcChannels"
 import { assertTrustedIpcSender } from "@src/ipc/ipcSecurity"
 import { requestBoundedTextViaNode } from "@src/ipc/network"
 import { assertString, MAX_LOGIN_RESPONSE_BYTES } from "@src/ipc/validation"
-import { removeAccountSecrets, saveAccountSecrets } from "@src/ipc/accountStore"
+import { AccountStoreUnreadableError, removeAccountSecrets, saveAccountSecrets } from "@src/ipc/accountStore"
+import type { AccountSaveOutcome } from "@src/ipc/accountStore"
 import { getErrorMessage, logMessage } from "@src/utils/logManager"
 
 const LOGIN_URL = new URL("https://auth3.vintagestory.at/v2/gamelogin")
@@ -54,12 +55,28 @@ async function requestLoginPass(email: string, password: string, twoFactorCode?:
  */
 async function settle(verdict: LoginVerdict): Promise<AccountLoginResult> {
   switch (verdict.status) {
-    case "success":
+    case "success": {
       // Keyed by the uid this same response just carried, so the store can never
       // disagree with what the renderer is told. Logging into an account already
       // saved overwrites its entry in place: a session refresh, not a duplicate.
-      await saveAccountSecrets(verdict.credentials.publicAccount.playerUid, verdict.credentials.secrets)
-      return { status: "success", account: verdict.credentials.publicAccount }
+      let outcome: AccountSaveOutcome
+      try {
+        outcome = await saveAccountSecrets(verdict.credentials.publicAccount.playerUid, verdict.credentials.secrets)
+      } catch (error) {
+        // Only the narrow "could not even copy the unreadable file aside" case gets its own
+        // wire status: anything else (no keyring, disk full) is a genuine storage failure and
+        // falls through to the caller's catch, same as before this file could rebuild a store.
+        if (!(error instanceof AccountStoreUnreadableError)) throw error
+        logMessage("error", "[back] [ipc] [accountHandlers.ts] [LOGIN] The account store is unreadable and could not be copied aside, so it was left untouched. The session was not saved.")
+        logMessage("debug", `[back] [ipc] [accountHandlers.ts] [LOGIN] ${getErrorMessage(error)}`)
+        return sessionStoreUnreadableResult()
+      }
+
+      if (outcome === "saved-after-rebuild")
+        logMessage("warn", "[back] [ipc] [accountHandlers.ts] [LOGIN] The account store could not be read; it was copied aside and rebuilt around this login. Other saved accounts must log in again.")
+
+      return { status: "success", account: verdict.credentials.publicAccount, ...(outcome === "saved-after-rebuild" ? { storeRebuilt: true } : {}) }
+    }
     case "needs-two-factor":
       return needsTwoFactorResult()
     case "two-factor-rejected":

src/ipc/handlers/accountLoginOutcome.ts

@@ -44,3 +44,8 @@ export function unexpectedResponseOutcome(verdict: UnreadableResponse): { result
     logMessage: `Login response claimed success but could not be read${verdict.diagnosis ? `: ${verdict.diagnosis}` : ""}.`
   }
 }
+
+/** The credentials were accepted, but the local secret store could not be read or safely rebuilt, so nothing was saved. */
+export function sessionStoreUnreadableResult(): AccountLoginResult {
+  return { status: "session-store-unreadable" }
+}

src/ipc/handlers/gameHandlers.ts

@@ -89,7 +89,12 @@ function realJsonFile(): JsonFile {
  */
 async function adoptRefreshedSession(accountId: string, secrets: AccountSecrets): Promise<void> {
   try {
-    await saveAccountSecrets(accountId, secrets)
+    const outcome = await saveAccountSecrets(accountId, secrets)
+    if (outcome === "saved-after-rebuild")
+      logMessage(
+        "warn",
+        `[back] [ipc] [ipc/handlers/gameHandlers.ts] [EXECUTE_GAME] The account store could not be read; it was copied aside and rebuilt around this adoption. Other saved accounts must log in again.`
+      )
     logMessage("info", `[back] [ipc] [ipc/handlers/gameHandlers.ts] [EXECUTE_GAME] The game had already refreshed this account's session. Adopted it instead of overwriting it.`)
   } catch (err) {
     logMessage("error", `[back] [ipc] [ipc/handlers/gameHandlers.ts] [EXECUTE_GAME] Could not store the session the game refreshed. Launching anyway.`)

src/renderer/src/components/ui/SessionButton.tsx

@@ -61,8 +61,10 @@ function SessionButton(): JSX.Element {
       if (result.status === "invalid-credentials") return addNotification(t("features.config.invalidEmailPass"), "error")
       if (result.status === "requires-two-factor") return addNotification(t("features.config.wrongtwofa"), "error")
       if (result.status === "unexpected-response") return addNotification(t("features.config.unexpectedResponse"), "error")
+      if (result.status === "session-store-unreadable") return addNotification(t("features.config.sessionStoreUnreadable"), "error")
       if (result.status !== "success") return
 
+      if (result.storeRebuilt) addNotification(t("features.config.sessionStoreRebuilt"), "warning")
       await saveLogin(result.account)
     } catch {
       // A throw here means the request never produced a verdict (network down,

src/renderer/src/locales/en-US.json

@@ -275,6 +275,8 @@
       "invalidEmailPass": "Invalid email or password!",
       "loginUnreachable": "Couldn't reach the login service. Check your connection or firewall and try again.",
       "unexpectedResponse": "The account service answered in a way this launcher couldn't read. Your password was not rejected. Try again after an update.",
+      "sessionStoreUnreadable": "Your login worked, but the saved-accounts file can't be read and couldn't be backed up, so the launcher left it alone rather than overwrite it. Nothing was saved this time.",
+      "sessionStoreRebuilt": "The saved-accounts file couldn't be read. A copy was kept and the launcher started a fresh one, so any other accounts on this device need to log in again.",
       "loggedin": "Logged in as {{user}}!",
       "onlyIfEnabledTwoFA": "Fill this field only if you have 2FA enabled!",
       "uiScale": "UI Scale",