fix(accounts): account removal must stick — incoming set authoritative + lock-held read-modify-write

packages/opencode/src/cli.ts

@@ -3,8 +3,8 @@
 import {
   getAccountStoragePath,
   loadAccounts,
+  mutateAccounts,
   type OAuthAccount,
-  saveAccounts,
 } from './core/accounts'
 import { beginAccountLogin, upsertAccount } from './core/oauth'
 import { openUrl } from './util/open-url'
@@ -70,30 +70,31 @@ async function main() {
 
       const account = await completion
 
-      const storage = (await loadAccounts()) ?? {
-        version: 1 as const,
-        accounts: [],
-      }
+      let rejected = false
+      let rejectedMsg = ''
+
+      await mutateAccounts((fresh) => {
+        if (
+          account.accountId &&
+          fresh.mainAccountId &&
+          account.accountId === fresh.mainAccountId
+        ) {
+          rejected = true
+          rejectedMsg =
+            '\nError: that account is already your main (same ChatGPT account).'
+          return
+        }
+        upsertAccount(fresh.accounts, account as unknown as OAuthAccount)
+      })
 
-      // Reject self-fallback: adding main's ChatGPT account as a fallback
-      // would let routing retry on the account that just returned 429.
-      if (
-        account.accountId &&
-        storage.mainAccountId &&
-        account.accountId === storage.mainAccountId
-      ) {
-        console.error(
-          '\nError: that account is already your main (same ChatGPT account).',
-        )
+      if (rejected) {
+        console.error(rejectedMsg)
         console.error(
           'A self-fallback would retry on the account that just returned 429.',
         )
         process.exit(1)
       }
 
-      upsertAccount(storage.accounts, account as unknown as OAuthAccount)
-      await saveAccounts(storage)
-
       console.log(`\n✓ Added account ${account.id}`)
       if (account.label) console.log(`  Label: ${account.label}`)
       break
@@ -123,20 +124,21 @@ async function main() {
         process.exit(1)
       }
 
-      const storage = await loadAccounts()
-      if (!storage) {
-        console.error('No account store found.')
-        process.exit(1)
-      }
+      let notFoundRemove = false
+      await mutateAccounts((fresh) => {
+        const idx = fresh.accounts.findIndex((a) => a.id === targetId)
+        if (idx === -1) {
+          notFoundRemove = true
+          return
+        }
+        fresh.accounts.splice(idx, 1)
+      })
 
-      const idx = storage.accounts.findIndex((a) => a.id === targetId)
-      if (idx === -1) {
+      if (notFoundRemove) {
         console.error(`No account with id "${targetId}".`)
         process.exit(1)
       }
 
-      storage.accounts.splice(idx, 1)
-      await saveAccounts(storage)
       console.log(`Removed account ${targetId}.`)
       break
     }

packages/opencode/src/commands.ts

@@ -77,6 +77,13 @@ export interface CommandContext {
   setCacheKeepEnabled?: (enabled: boolean) => void
   /** Updates the live loader's persisted-subagent cachekeep gate. */
   setCacheKeepSubagents?: (enabled: boolean) => void
+  /** Lock-held read-modify-write; preserves concurrent additions. */
+  mutateAccounts: (
+    mutator: (
+      storage: import('./core/accounts').AccountStorage,
+    ) => void | Promise<void>,
+    path: string,
+  ) => Promise<import('./core/accounts').AccountStorage>
 }
 
 const log = createLogger('commands')
@@ -196,97 +203,114 @@ async function executeAccountCommand(
   if (tokens[0] === 'switch' && tokens[1]) {
     const targetId = tokens[1]
 
-    if (targetId === 'main') {
-      storage.routing = { ...(storage.routing ?? {}), activeId: 'main' }
-      await defaultSaveAccounts(storage, ctx.accountStoragePath)
-      log.info('account switched', { activeId: 'main' })
-      void ctx.refreshSidebar?.().catch(() => {})
-      return {
-        command: 'openai-account',
-        text: '## Account Switched\n\nActive account is now main.',
-        knobs: { accounts, activeId: 'main' },
+    let notFoundSwitch = false
+    const fresh = await ctx.mutateAccounts((fresh) => {
+      if (targetId === 'main') {
+        fresh.routing = { ...(fresh.routing ?? {}), activeId: 'main' }
+        return
       }
-    }
+      const account = fresh.accounts.find((a) => a.id === targetId)
+      if (!account) {
+        notFoundSwitch = true
+        return
+      }
+      fresh.routing = { ...(fresh.routing ?? {}), activeId: targetId }
+    }, ctx.accountStoragePath)
 
-    const account = accounts.find((a) => a.id === targetId)
-    if (!account) {
+    if (notFoundSwitch) {
       return {
         command: 'openai-account',
         text: `## Account Not Found\n\nNo account with id \`${targetId}\` exists.`,
         knobs: { accounts },
       }
     }
 
-    // Persist the active account id
-    storage.routing = { ...(storage.routing ?? {}), activeId: targetId }
-    await defaultSaveAccounts(storage, ctx.accountStoragePath)
     log.info('account switched', { activeId: targetId })
     void ctx.refreshSidebar?.().catch(() => {})
 
     return {
       command: 'openai-account',
-      text: `## Account Switched\n\nActive account is now \`${targetId}\`.`,
-      knobs: { accounts, activeId: targetId },
+      text:
+        targetId === 'main'
+          ? '## Account Switched\n\nActive account is now main.'
+          : `## Account Switched\n\nActive account is now \`${targetId}\`.`,
+      knobs: { accounts: fresh.accounts, activeId: targetId },
     }
   }
 
   if (tokens[0] === 'remove' && tokens[1]) {
     const targetId = tokens[1]
-    const idx = accounts.findIndex((a) => a.id === targetId)
-    if (idx === -1) {
+    let notFoundRemove = false
+
+    const fresh = await ctx.mutateAccounts((fresh) => {
+      const idx = fresh.accounts.findIndex((a) => a.id === targetId)
+      if (idx === -1) {
+        notFoundRemove = true
+        return
+      }
+
+      const wasActive = fresh.routing?.activeId === targetId
+      fresh.accounts.splice(idx, 1)
+
+      if (wasActive) {
+        const next = fresh.accounts.find(isOAuthAccount)
+        fresh.routing = {
+          ...(fresh.routing ?? {}),
+          activeId: next?.id ?? 'main',
+        }
+      }
+    }, ctx.accountStoragePath)
+
+    if (notFoundRemove) {
       return {
         command: 'openai-account',
         text: `## Account Not Found\n\nNo account with id \`${targetId}\` exists.`,
         knobs: { accounts },
       }
     }
 
-    const wasActive = storage.routing?.activeId === targetId
-    accounts.splice(idx, 1)
-
-    // If removing the active account, repoint to the next OAuth fallback or main.
-    if (wasActive) {
-      const next = accounts.find(isOAuthAccount)
-      storage.routing = {
-        ...(storage.routing ?? {}),
-        activeId: next?.id ?? 'main',
-      }
-    }
-
-    await defaultSaveAccounts(storage, ctx.accountStoragePath)
     log.info('account removed', { id: targetId })
     void ctx.refreshSidebar?.().catch(() => {})
 
     return {
       command: 'openai-account',
       text: `## Account Removed\n\nRemoved account \`${targetId}\`.`,
-      knobs: { accounts },
+      knobs: { accounts: fresh.accounts },
     }
   }
 
   if (tokens[0] === 'order' && tokens.length >= 3) {
-    // Reorder: swap positions of two accounts
-    const a = accounts.findIndex((ac) => ac.id === tokens[1])
-    const b = accounts.findIndex((ac) => ac.id === tokens[2])
-    if (a === -1 || b === -1) {
+    let notFoundOrder = false
+
+    const fresh = await ctx.mutateAccounts((fresh) => {
+      const a = fresh.accounts.findIndex((ac) => ac.id === tokens[1])
+      const b = fresh.accounts.findIndex((ac) => ac.id === tokens[2])
+      if (a === -1 || b === -1) {
+        notFoundOrder = true
+        return
+      }
+      // biome-ignore lint/style/noNonNullAssertion: a,b validated in-bounds by findIndex above
+      const tmp = fresh.accounts[a]!
+      // biome-ignore lint/style/noNonNullAssertion: a,b validated in-bounds by findIndex above
+      fresh.accounts[a] = fresh.accounts[b]!
+      fresh.accounts[b] = tmp
+    }, ctx.accountStoragePath)
+
+    if (notFoundOrder) {
       return {
         command: 'openai-account',
         text: '## Invalid Order\n\nBoth account IDs must exist.',
         knobs: { accounts },
       }
     }
-    // biome-ignore lint/style/noNonNullAssertion: a,b validated in-bounds by findIndex above
-    const tmp = accounts[a]!
-    // biome-ignore lint/style/noNonNullAssertion: a,b validated in-bounds by findIndex above
-    accounts[a] = accounts[b]!
-    accounts[b] = tmp
-    await defaultSaveAccounts(storage, ctx.accountStoragePath)
+
     log.info('accounts reordered', { a: tokens[1], b: tokens[2] })
     void ctx.refreshSidebar?.().catch(() => {})
+
     return {
       command: 'openai-account',
       text: `## Accounts Reordered\n\nSwapped positions of \`${tokens[1]}\` and \`${tokens[2]}\`.`,
-      knobs: { accounts },
+      knobs: { accounts: fresh.accounts },
     }
   }
 
@@ -307,32 +331,36 @@ async function executeAccountCommand(
     // never reach the user.
     completion
       .then(async (account) => {
-        const store = (await ctx.loadAccounts(ctx.accountStoragePath)) ?? {
-          version: 1 as const,
-          accounts: [],
-        }
-
-        if (
-          account.accountId &&
-          store.mainAccountId &&
-          account.accountId === store.mainAccountId
-        ) {
-          const msg =
-            'That account is already your main account — not added as a fallback.'
+        let rejected = false
+        let rejectedMsg = ''
+
+        await ctx.mutateAccounts((fresh) => {
+          if (
+            account.accountId &&
+            fresh.mainAccountId &&
+            account.accountId === fresh.mainAccountId
+          ) {
+            rejected = true
+            rejectedMsg =
+              'That account is already your main account — not added as a fallback.'
+            return
+          }
+          upsertAccount(fresh.accounts, account as OAuthAccount)
+        }, ctx.accountStoragePath)
+
+        if (rejected) {
           log.warn('account add rejected (main identity)', {
             accountId: account.accountId,
             sessionId,
           })
           notify?.({
             command: 'openai-account',
-            text: `## Add Failed\n\n${msg}`,
+            text: `## Add Failed\n\n${rejectedMsg}`,
             knobs: {},
           })
           return
         }
 
-        upsertAccount(store.accounts, account as OAuthAccount)
-        await defaultSaveAccounts(store, ctx.accountStoragePath)
         log.info('account added', {
           id: account.id,
           label: account.label,

packages/opencode/src/core/accounts.ts

@@ -642,14 +642,21 @@ function mergeStorageForSave(
 ): AccountStorage {
   if (!latest) return incoming
 
-  const accounts = new Map<string, FallbackAccount>()
-  for (const account of latest.accounts) accounts.set(account.id, account)
-  for (const account of incoming.accounts) accounts.set(account.id, account)
-
+  // Incoming storage is authoritative for the account set and for every
+  // top-level field it carries.  Fields absent from incoming fall back to
+  // the latest on-disk snapshot.  Because the spread is { ...latest,
+  // ...incoming }, incoming can overwrite any field that was concurrently
+  // written to disk — callers that need to preserve concurrent writes must
+  // use mutateAccounts (which re-reads under the lock) instead of a
+  // load→mutate→saveAccounts sequence.
+  //
+  // Per-account runtime state (quota snapshots, refresh errors) is written
+  // by background timers through the scoped saveAccountState path and is
+  // unaffected by this merge.
   return {
     ...latest,
     ...incoming,
-    accounts: [...accounts.values()],
+    accounts: incoming.accounts,
   }
 }
 
@@ -745,6 +752,45 @@ export async function saveAccounts(
   }
 }
 
+export async function mutateAccounts(
+  mutator: (storage: AccountStorage) => void | Promise<void>,
+  path = getAccountStoragePath(),
+): Promise<AccountStorage> {
+  const statePath = getAccountStatePath(path)
+  const lock = await acquireSaveAccountsLock(path)
+  try {
+    const stateLock = await acquireSaveAccountsLock(statePath)
+    try {
+      const configJson = await readJsonIfPresent(path)
+      const stateJson = await readJsonIfPresent(statePath)
+      const fresh: AccountStorage = configJson.exists
+        ? (normalizeStorage(
+            mergeConfigAndState(configJson.value, stateJson.value),
+          ) ??
+          ({
+            version: 1,
+            main: { type: 'opencode', provider: 'openai' },
+            accounts: [],
+          } as AccountStorage))
+        : {
+            version: 1,
+            main: { type: 'opencode', provider: 'openai' },
+            accounts: [],
+          }
+      await mutator(fresh)
+      const existing = isRecord(configJson.value) ? configJson.value : {}
+      const nextConfig = { ...existing, ...configFromStorage(fresh) }
+      await writeJsonAtomic(path, nextConfig)
+      await writeJsonAtomic(statePath, stateFromStorage(fresh))
+      return fresh
+    } finally {
+      await stateLock.release()
+    }
+  } finally {
+    await lock.release()
+  }
+}
+
 function applyMainQuotaStatePatch(
   state: AccountRuntimeState,
   storage: AccountStorage,
@@ -820,6 +866,10 @@ export async function saveAccountState(
         )
       }
       if (ids) {
+        // Scoped save: only prune ids that are in the requested scope and
+        // absent from storage.accounts.  Do NOT touch ids outside the scope
+        // so that a partial save never prunes state for accounts it was not
+        // asked to manage.
         for (const id of ids) {
           if (!storage.accounts.some((account) => account.id === id)) {
             delete next.accounts[id]