Fix saved account loss across concurrent CLI sessions

.changeset/preserve-auth-sessions.md

@@ -0,0 +1,6 @@
+---
+'@shopify/cli': patch
+'@shopify/cli-kit': patch
+---
+
+Preserve saved accounts when CLI instances authenticate at the same time or token refresh fails.

packages/cli-kit/package.json

@@ -147,6 +147,7 @@
     "node-fetch": "3.3.2",
     "open": "8.4.2",
     "pathe": "1.1.2",
+    "proper-lockfile": "4.1.2",
     "react": "19.2.4",
     "semver": "7.8.4",
     "react-dom": "19.2.4",
@@ -163,6 +164,7 @@
     "@types/fs-extra": "9.0.13",
     "@types/gradient-string": "^1.1.2",
     "@types/lodash": "4.17.24",
+    "@types/proper-lockfile": "4.1.4",
     "@types/react": "^19.0.0",
     "@types/react-dom": "^19.0.0",
     "@types/semver": "^7.5.2",

packages/cli-kit/src/private/node/conf-store.ts

@@ -2,6 +2,7 @@ import {isLocalEnvironment} from './context/service.js'
 import {isUnitTest} from '../../public/node/context/local.js'
 import {LocalStorage} from '../../public/node/local-storage.js'
 import {outputContent, outputDebug} from '../../public/node/output.js'
+import lockfile from 'proper-lockfile'
 
 interface CacheValue<T> {
   value: T
@@ -55,6 +56,22 @@ function currentSessionIdKey(): 'currentDevSessionId' | 'currentSessionId' {
   return isLocalEnvironment() ? 'currentDevSessionId' : 'currentSessionId'
 }
 
+/** Serializes session mutations across CLI processes without holding a lock during authentication. */
+export async function withSessionStoreLock<T>(
+  action: () => Promise<T>,
+  config: LocalStorage<ConfSchema> = cliKitStore(),
+): Promise<T> {
+  const release = await lockfile.lock(config.path, {
+    realpath: false,
+    retries: {retries: 20, minTimeout: 10, maxTimeout: 100},
+  })
+  try {
+    return await action()
+  } finally {
+    await release()
+  }
+}
+
 /**
  * Get session.
  *

packages/cli-kit/src/private/node/session.test.ts

@@ -13,6 +13,7 @@ import {
   exchangeCustomPartnerToken,
   refreshAccessToken,
   InvalidGrantError,
+  InvalidRequestError,
 } from './session/exchange.js'
 import {allDefaultScopes} from './session/scopes.js'
 import {store as storeSessions, fetch as fetchSessions, remove as secureRemove} from './session/store.js'
@@ -204,6 +205,7 @@ describe('ensureAuthenticated when previous session is invalid', () => {
     expect(refreshAccessToken).not.toBeCalled()
     expect(businessPlatformRequest).toHaveBeenCalled()
     expect(storeSessions).toHaveBeenCalledOnce()
+    expect(setCurrentSessionId).toHaveBeenCalledWith(userId)
     expect(got).toEqual(validTokens)
 
     // Verify the session was stored with email as alias
@@ -216,7 +218,7 @@ describe('ensureAuthenticated when previous session is invalid', () => {
     expect(fetchSessions).toHaveBeenCalledOnce()
   })
 
-  test('throws an error and logs out if there is no session and prompting is disabled,', async () => {
+  test('throws an error without removing saved accounts when prompting is disabled', async () => {
     // Given
     vi.mocked(validateSession).mockResolvedValueOnce('needs_full_auth')
     vi.mocked(fetchSessions).mockResolvedValue(undefined)
@@ -228,7 +230,7 @@ describe('ensureAuthenticated when previous session is invalid', () => {
 
 The CLI is currently unable to prompt for reauthentication.`,
     )
-    expect(secureRemove).toHaveBeenCalled()
+    expect(secureRemove).not.toHaveBeenCalled()
 
     // Then
     await expect(getLastSeenAuthMethod()).resolves.toEqual('none')
@@ -439,13 +441,68 @@ describe('when existing session is valid', () => {
 
     // Then
     expect(refreshAccessToken).toHaveBeenCalled()
-    expect(storeSessions).toHaveBeenCalledWith(sessions)
+    expect(storeSessions).toHaveBeenCalledWith({[fqdn]: {[selectedUserId]: sessions[fqdn]![selectedUserId]}})
     expect(setCurrentSessionId).not.toHaveBeenCalled()
     expect(got).toEqual({...validTokens, userId: selectedUserId})
   })
 })
 
 describe('when existing session is expired', () => {
+  test.each([
+    ['invalid_grant', InvalidGrantError],
+    ['invalid_request', InvalidRequestError],
+  ] as const)(
+    'preserves all saved accounts when a background refresh fails with %s',
+    async (_errorName, ErrorClass) => {
+      vi.mocked(validateSession).mockResolvedValueOnce('needs_refresh')
+      vi.mocked(fetchSessions).mockResolvedValue({
+        ...validSessions,
+        [fqdn]: {
+          ...validSessions[fqdn],
+          'another-user': {identity: {...validIdentityToken, userId: 'another-user'}, applications: {}},
+        },
+      })
+      vi.mocked(refreshAccessToken).mockRejectedValueOnce(new ErrorClass())
+
+      await expect(ensureAuthenticated(defaultApplications, process.env, {noPrompt: true})).rejects.toThrow()
+
+      expect(secureRemove).not.toHaveBeenCalled()
+      expect(storeSessions).not.toHaveBeenCalled()
+      expect(setCurrentSessionId).not.toHaveBeenCalled()
+      expect(requestDeviceAuthorization).not.toHaveBeenCalled()
+    },
+  )
+
+  test('preserves saved accounts when an interactive refresh returns invalid_request', async () => {
+    vi.mocked(validateSession).mockResolvedValueOnce('needs_refresh')
+    vi.mocked(fetchSessions).mockResolvedValue(validSessions)
+    vi.mocked(refreshAccessToken).mockRejectedValueOnce(new InvalidRequestError())
+
+    await expect(ensureAuthenticated(defaultApplications)).rejects.toThrow('Error validating auth session')
+
+    expect(secureRemove).not.toHaveBeenCalled()
+    expect(storeSessions).not.toHaveBeenCalled()
+  })
+
+  test('saves only the refreshed account without changing the selected account', async () => {
+    const otherSession = {identity: {...validIdentityToken, userId: 'another-user'}, applications: {}}
+    vi.mocked(validateSession).mockResolvedValueOnce('needs_refresh')
+    vi.mocked(fetchSessions).mockResolvedValue({
+      [fqdn]: {...validSessions[fqdn], 'another-user': otherSession},
+    })
+    vi.mocked(getCurrentSessionId).mockReturnValue(userId)
+    vi.mocked(refreshAccessToken).mockImplementationOnce(async () => {
+      vi.mocked(getCurrentSessionId).mockReturnValue('another-user')
+      return validIdentityToken
+    })
+
+    await ensureAuthenticated(defaultApplications)
+
+    expect(storeSessions).toHaveBeenCalledWith(validSessions)
+    expect(setCurrentSessionId).not.toHaveBeenCalled()
+    expect(getCurrentSessionId()).toBe('another-user')
+  })
+
   test('refreshes the tokens', async () => {
     // Given
     vi.mocked(validateSession).mockResolvedValueOnce('needs_refresh')

packages/cli-kit/src/private/node/session.ts

@@ -9,7 +9,7 @@ import {
   InvalidGrantError,
   InvalidRequestError,
 } from './session/exchange.js'
-import {IdentityToken, Session, Sessions} from './session/schema.js'
+import {IdentityToken, Session} from './session/schema.js'
 import * as sessionStore from './session/store.js'
 import {pollForDeviceAuthorization, requestDeviceAuthorization} from './session/device-authorization.js'
 import {automationTokenAuthMethod, automationTokenVariablesProblem} from './session/automation-token.js'
@@ -21,7 +21,7 @@ import {themeToken} from '../../public/node/context/local.js'
 import {AbortError} from '../../public/node/error.js'
 import {normalizeStoreFqdn, identityFqdn} from '../../public/node/context/fqdn.js'
 import {getIdentityTokenInformation, getAppAutomationToken} from '../../public/node/environment.js'
-import {AdminSession, ensureNoServiceAccountToken, logout} from '../../public/node/session.js'
+import {AdminSession, ensureNoServiceAccountToken} from '../../public/node/session.js'
 import {nonRandomUUID} from '../../public/node/crypto.js'
 import {isEmpty} from '../../public/common/object.js'
 import {businessPlatformRequest} from '../../public/node/api/business-platform.js'
@@ -262,8 +262,10 @@ ${outputToken.json(applications)}
         await throwOnNoPrompt(noPrompt)
         newSession = await executeCompleteFlow(applications, currentSession?.identity.alias)
       } else if (error instanceof InvalidRequestError) {
-        await sessionStore.remove()
-        throw new AbortError('\nError validating auth session', "We've cleared the current session, please try again")
+        throw new AbortError(
+          '\nError validating auth session',
+          'Run `shopify auth login` and select "Log in with a different account" to sign in again.',
+        )
       } else {
         throw error
       }
@@ -272,15 +274,12 @@ ${outputToken.json(applications)}
 
   const completeSession = {...currentSession, ...newSession} as Session
   const newSessionId = completeSession.identity.userId
-  const updatedSessions: Sessions = {
-    ...sessions,
-    [fqdn]: {...sessions[fqdn], [newSessionId]: completeSession},
-  }
-
   // Save the new session info if it has changed
   if (!isEmpty(newSession)) {
-    await sessionStore.store(updatedSessions)
-    if (!commandSessionId) setCurrentSessionId(newSessionId)
+    await sessionStore.store({[fqdn]: {[newSessionId]: completeSession}})
+    if (!commandSessionId && (!currentSession || newSessionId !== currentSession.identity.userId)) {
+      setCurrentSessionId(newSessionId)
+    }
   }
 
   const tokens = await tokensFor(applications, completeSession)
@@ -297,7 +296,6 @@ ${outputToken.json(applications)}
 
 async function throwOnNoPrompt(noPrompt: boolean) {
   if (!noPrompt) return
-  await logout()
   throw new AbortError(
     `The currently available CLI credentials are invalid.
 

packages/cli-kit/src/private/node/session/store.integration.test.ts

@@ -0,0 +1,101 @@
+import {fetch, remove, setSessionAlias, store} from './store.js'
+import {getCurrentSessionId, getSessions, setCurrentSessionId, setSessions} from '../conf-store.js'
+import {identityFqdn} from '../../../public/node/context/fqdn.js'
+import {inTemporaryDirectory} from '../../../public/node/fs.js'
+import {LocalStorage} from '../../../public/node/local-storage.js'
+import {describe, expect, test, vi} from 'vitest'
+import type {ConfSchema} from '../conf-store.js'
+import type {Session} from './schema.js'
+
+vi.mock('../../../public/node/context/fqdn.js')
+
+const fqdn = 'accounts.shopify.com'
+
+function session(userId: string): Session {
+  return {
+    identity: {
+      userId,
+      accessToken: 'fake-access-token',
+      refreshToken: 'fake-refresh-token',
+      expiresAt: new Date('2100-01-01'),
+      scopes: [],
+    },
+    applications: {},
+  }
+}
+
+describe('session storage with real files', () => {
+  test('merges overlapping account writes from separate storage instances', async () => {
+    await inTemporaryDirectory(async (cwd) => {
+      const first = new LocalStorage<ConfSchema>({cwd})
+      const second = new LocalStorage<ConfSchema>({cwd})
+      const alice = session('alice')
+      const bob = session('bob')
+
+      await Promise.all([store({[fqdn]: {alice}}, first), store({[fqdn]: {bob}}, second)])
+
+      await expect(fetch(first)).resolves.toEqual({[fqdn]: {alice, bob}})
+    })
+  })
+
+  test('preserves accounts saved after authentication started', async () => {
+    await inTemporaryDirectory(async (cwd) => {
+      const config = new LocalStorage<ConfSchema>({cwd})
+      const alice = session('alice')
+      const bob = session('bob')
+      await store({[fqdn]: {alice}}, config)
+      const beforeRefresh = await fetch(config)
+      await store({[fqdn]: {bob}}, new LocalStorage<ConfSchema>({cwd}))
+      setCurrentSessionId('bob', config)
+
+      await store({[fqdn]: {alice: {...beforeRefresh![fqdn]!.alice!, applications: {}}}}, config)
+
+      await expect(fetch(config)).resolves.toEqual({[fqdn]: {alice, bob}})
+      expect(getCurrentSessionId(config)).toBe('bob')
+    })
+  })
+
+  test('updates an alias without overwriting an overlapping account write', async () => {
+    await inTemporaryDirectory(async (cwd) => {
+      vi.mocked(identityFqdn).mockResolvedValue(fqdn)
+      const config = new LocalStorage<ConfSchema>({cwd})
+      const alice = session('alice')
+      const bob = session('bob')
+      await store({[fqdn]: {alice}}, config)
+
+      await Promise.all([
+        setSessionAlias('alice', 'work', config),
+        store({[fqdn]: {bob}}, new LocalStorage<ConfSchema>({cwd})),
+      ])
+
+      await expect(fetch(config)).resolves.toEqual({
+        [fqdn]: {alice: {...alice, identity: {...alice.identity, alias: 'work'}}, bob},
+      })
+    })
+  })
+
+  test.each(['{invalid json', JSON.stringify({unexpected: 'session format'})])(
+    'preserves unreadable session content and releases the write lock',
+    async (content) => {
+      await inTemporaryDirectory(async (cwd) => {
+        const config = new LocalStorage<ConfSchema>({cwd})
+        setSessions(content, config)
+        setCurrentSessionId('alice', config)
+
+        await expect(fetch(config)).rejects.toThrow('The saved CLI accounts could not be read')
+        await expect(store({[fqdn]: {bob: session('bob')}}, config)).rejects.toThrow(
+          'The saved CLI accounts could not be read',
+        )
+
+        expect(getSessions(config)).toBe(content)
+        expect(getCurrentSessionId(config)).toBe('alice')
+
+        await remove(config)
+        expect(getSessions(config)).toBeUndefined()
+        expect(getCurrentSessionId(config)).toBeUndefined()
+        await store({[fqdn]: {bob: session('bob')}}, config)
+        await expect(fetch(config)).resolves.toEqual({[fqdn]: {bob: session('bob')}})
+      })
+    },
+  )
+})

packages/cli-kit/src/private/node/session/store.test.ts

@@ -1,6 +1,6 @@
 import {Sessions} from './schema.js'
 import {store, fetch, remove, getSessionAlias, setSessionAlias, findSessionByAlias} from './store.js'
-import {getSessions, removeSessions, setSessions, removeCurrentSessionId} from '../conf-store.js'
+import {getSessions, removeSessions, setSessions, removeCurrentSessionId, withSessionStoreLock} from '../conf-store.js'
 import {identityFqdn} from '../../../public/node/context/fqdn.js'
 
 import {describe, expect, vi, test, beforeEach} from 'vitest'
@@ -37,6 +37,7 @@ const mockSessions: Sessions = {
 describe('session store', () => {
   beforeEach(() => {
     vi.mocked(identityFqdn).mockResolvedValue('identity.fqdn.com')
+    vi.mocked(withSessionStoreLock).mockImplementation(async (action) => action())
   })
 
   describe('store', () => {
@@ -45,7 +46,7 @@ describe('session store', () => {
       await store(mockSessions)
 
       // Then
-      expect(setSessions).toHaveBeenCalledWith(JSON.stringify(mockSessions))
+      expect(setSessions).toHaveBeenCalledWith(JSON.stringify(mockSessions), undefined)
     })
   })
 
@@ -72,32 +73,26 @@ describe('session store', () => {
       expect(result).toEqual(mockSessions)
     })
 
-    test('returns undefined and discards malformed JSON session content', async () => {
+    test('preserves malformed JSON session content and reports an error', async () => {
       // Given
       vi.mocked(getSessions).mockReturnValue('{not valid json')
 
       // When
-      const result = await fetch()
-
-      // Then
-      expect(result).toBeUndefined()
-      expect(removeSessions).toHaveBeenCalled()
-      expect(removeCurrentSessionId).toHaveBeenCalled()
+      await expect(fetch()).rejects.toThrow('The saved CLI accounts could not be read')
+      expect(removeSessions).not.toHaveBeenCalled()
+      expect(removeCurrentSessionId).not.toHaveBeenCalled()
     })
 
-    test('returns undefined and discards schema-invalid session content', async () => {
+    test('preserves schema-invalid session content and reports an error', async () => {
       // Given
       vi.mocked(getSessions).mockReturnValue(JSON.stringify({not: 'a valid sessions object'}))
       vi.mocked(removeSessions).mockClear()
       vi.mocked(removeCurrentSessionId).mockClear()
 
       // When
-      const result = await fetch()
-
-      // Then
-      expect(result).toBeUndefined()
-      expect(removeSessions).toHaveBeenCalled()
-      expect(removeCurrentSessionId).toHaveBeenCalled()
+      await expect(fetch()).rejects.toThrow('The saved CLI accounts could not be read')
+      expect(removeSessions).not.toHaveBeenCalled()
+      expect(removeCurrentSessionId).not.toHaveBeenCalled()
     })
 
     test('rethrows non-SyntaxError parse failures without discarding sessions', async () => {