Fix CLI session loss on background refresh failures

.changeset/calm-dogs-tell.md

@@ -0,0 +1,6 @@
+---
+'@shopify/cli-kit': patch
+'@shopify/theme': patch
+---
+
+Keep other saved CLI sessions when a background refresh fails, and warn when theme dev needs interactive reauthentication.

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 envPaths from 'env-paths'
 
 interface CacheValue<T> {
   value: T
@@ -47,6 +48,17 @@ function cliKitStore() {
   return _instance
 }
 
+/**
+ * Returns the configuration directory used to coordinate session-store updates across CLI processes.
+ *
+ * @returns The CLI Kit configuration directory.
+ */
+export function getSessionStoreLockPath(): string {
+  const projectName = `shopify-cli-kit${isUnitTest() ? '-test' : ''}`
+  cliKitStore()
+  return envPaths(projectName, {suffix: 'nodejs'}).config
+}
+
 function sessionStoreKey(): 'devSessionStore' | 'sessionStore' {
   return isLocalEnvironment() ? 'devSessionStore' : 'sessionStore'
 }

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

@@ -13,9 +13,18 @@ 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'
+import {
+  store as storeAllSessions,
+  storeSession as storeSessions,
+  fetch as fetchSessions,
+  remove as secureRemove,
+  removeSession as removeFailedSession,
+  withSessionStoreLock,
+  type LockedSessionStore,
+} from './session/store.js'
 import {ApplicationToken, IdentityToken, Sessions} from './session/schema.js'
 import {validateSession} from './session/validate.js'
 import {applicationId} from './session/identity.js'
@@ -138,6 +147,18 @@ beforeEach(() => {
   })
   vi.mocked(partnersRequest).mockResolvedValue(undefined)
   vi.mocked(allDefaultScopes).mockImplementation((scopes) => scopes ?? [])
+  vi.mocked(withSessionStoreLock).mockImplementation(async (operation) => {
+    const store: LockedSessionStore = {
+      fetch: () => fetchSessions(),
+      store: (sessions) => storeAllSessions(sessions),
+      storeSession: (fqdn, id, session) => storeSessions(fqdn, id, session),
+      getCurrentSessionId,
+      setCurrentSessionId,
+      remove: () => secureRemove(),
+      removeSession: (fqdn, id, session) => removeFailedSession(fqdn, id, session),
+    }
+    return operation(store)
+  })
   setLastSeenUserIdAfterAuth(undefined as any)
   setLastSeenAuthMethod('none')
   setCommandSessionId(undefined)
@@ -207,16 +228,16 @@ describe('ensureAuthenticated when previous session is invalid', () => {
     expect(got).toEqual(validTokens)
 
     // Verify the session was stored with email as alias
-    const storedSession = vi.mocked(storeSessions).mock.calls[0]![0]
-    expect(storedSession[fqdn]![userId]!.identity.alias).toBe('user@example.com')
+    const storedSession = vi.mocked(storeSessions).mock.calls[0]![2]
+    expect(storedSession.identity.alias).toBe('user@example.com')
 
     // The userID is cached in memory and the secureStore is not accessed again
     await expect(getLastSeenUserIdAfterAuth()).resolves.toBe('1234-5678')
     await expect(getLastSeenAuthMethod()).resolves.toEqual('device_auth')
     expect(fetchSessions).toHaveBeenCalledOnce()
   })
 
-  test('throws an error and logs out if there is no session and prompting is disabled,', async () => {
+  test('throws an error if there is no session and prompting is disabled', async () => {
     // Given
     vi.mocked(validateSession).mockResolvedValueOnce('needs_full_auth')
     vi.mocked(fetchSessions).mockResolvedValue(undefined)
@@ -228,7 +249,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')
@@ -260,7 +281,7 @@ The CLI is currently unable to prompt for reauthentication.`,
     // Then
     expect(exchangeAccessForApplicationTokens).toBeCalled()
     expect(refreshAccessToken).not.toBeCalled()
-    expect(storeSessions).toBeCalledWith(expectedSessions)
+    expect(storeSessions).toBeCalledWith(fqdn, userId, expectedSessions[fqdn][userId])
     expect(got).toEqual(validTokens)
     await expect(getLastSeenUserIdAfterAuth()).resolves.toBe('1234-5678')
     await expect(getLastSeenAuthMethod()).resolves.toEqual('device_auth')
@@ -281,8 +302,8 @@ The CLI is currently unable to prompt for reauthentication.`,
     expect(businessPlatformRequest).toHaveBeenCalled()
     expect(storeSessions).toHaveBeenCalledOnce()
 
-    const storedSession = vi.mocked(storeSessions).mock.calls[0]![0]
-    expect(storedSession[fqdn]![userId]!.identity.alias).toBe(userId)
+    const storedSession = vi.mocked(storeSessions).mock.calls[0]![2]
+    expect(storedSession.identity.alias).toBe(userId)
 
     expect(got).toEqual(validTokens)
   })
@@ -305,8 +326,8 @@ The CLI is currently unable to prompt for reauthentication.`,
     expect(businessPlatformRequest).not.toHaveBeenCalled()
 
     // Verify the session was stored with userId as alias (fallback)
-    const storedSession = vi.mocked(storeSessions).mock.calls[0]![0]
-    expect(storedSession[fqdn]![userId]!.identity.alias).toBe(userId)
+    const storedSession = vi.mocked(storeSessions).mock.calls[0]![2]
+    expect(storedSession.identity.alias).toBe(userId)
   })
 
   test('executes complete auth flow if requesting additional scopes', async () => {
@@ -324,8 +345,8 @@ The CLI is currently unable to prompt for reauthentication.`,
     expect(storeSessions).toHaveBeenCalledOnce()
 
     // Verify the session was stored with email as alias
-    const storedSession = vi.mocked(storeSessions).mock.calls[0]![0]
-    expect(storedSession[fqdn]![userId]!.identity.alias).toBe('user@example.com')
+    const storedSession = vi.mocked(storeSessions).mock.calls[0]![2]
+    expect(storedSession.identity.alias).toBe('user@example.com')
 
     expect(got).toEqual(validTokens)
     await expect(getLastSeenUserIdAfterAuth()).resolves.toBe('1234-5678')
@@ -411,11 +432,11 @@ describe('when existing session is valid', () => {
     // Then
     expect(refreshAccessToken).toBeCalled()
     expect(exchangeAccessForApplicationTokens).toBeCalled()
-    expect(storeSessions).toBeCalledWith(validSessions)
+    expect(storeSessions).toBeCalledWith(fqdn, userId, validSessions[fqdn]![userId])
     expect(got).toEqual(validTokens)
     await expect(getLastSeenUserIdAfterAuth()).resolves.toBe('1234-5678')
     await expect(getLastSeenAuthMethod()).resolves.toEqual('device_auth')
-    expect(fetchSessions).toHaveBeenCalledOnce()
+    expect(fetchSessions).toHaveBeenCalledTimes(2)
   })
 
   test('refreshes the command selected session without changing the current session ID', async () => {
@@ -439,10 +460,82 @@ 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})
   })
+
+  test('preserves an account selection made while refreshing another session', async () => {
+    // Given
+    const otherUserId = 'other-user-id'
+    const sessions: Sessions = {
+      [fqdn]: {
+        ...validSessions[fqdn],
+        [otherUserId]: {
+          identity: {...validIdentityToken, userId: otherUserId},
+          applications: appTokens,
+        },
+      },
+    }
+    vi.mocked(getCurrentSessionId).mockReturnValue(userId)
+    vi.mocked(validateSession).mockResolvedValueOnce('needs_refresh')
+    vi.mocked(fetchSessions).mockResolvedValue(sessions)
+
+    let lockCalls = 0
+    vi.mocked(withSessionStoreLock).mockImplementation(async (operation) => {
+      const store: LockedSessionStore = {
+        fetch: () => fetchSessions(),
+        store: (allSessions) => storeAllSessions(allSessions),
+        storeSession: (fqdn, id, session) => storeSessions(fqdn, id, session),
+        getCurrentSessionId,
+        setCurrentSessionId,
+        remove: () => secureRemove(),
+        removeSession: (fqdn, id, session) => removeFailedSession(fqdn, id, session),
+      }
+      const result = await operation(store)
+      if (++lockCalls === 1) vi.mocked(getCurrentSessionId).mockReturnValue(otherUserId)
+      return result
+    })
+
+    // When
+    await ensureAuthenticated(defaultApplications)
+
+    // Then
+    expect(setCurrentSessionId).not.toHaveBeenCalled()
+    expect(getCurrentSessionId()).toBe(otherUserId)
+  })
+
+  test('uses a session refreshed by another process instead of refreshing a stale copy', async () => {
+    // Given
+    const latestSession = {
+      ...validSessions[fqdn]![userId]!,
+      identity: {
+        ...validIdentityToken,
+        accessToken: 'refreshed_identity_token',
+        refreshToken: 'rotated_refresh_token',
+        expiresAt: new Date(Date.now() + 60_000),
+      },
+      applications: {
+        ...appTokens,
+        'mystore.myshopify.com-admin': {
+          ...appTokens['mystore.myshopify.com-admin']!,
+          accessToken: 'refreshed_admin_token',
+        },
+      },
+    }
+    vi.mocked(validateSession).mockResolvedValueOnce('needs_refresh').mockResolvedValueOnce('ok')
+    vi.mocked(fetchSessions)
+      .mockResolvedValueOnce(validSessions)
+      .mockResolvedValueOnce({[fqdn]: {[userId]: latestSession}})
+
+    // When
+    const got = await ensureAuthenticated(defaultApplications)
+
+    // Then
+    expect(refreshAccessToken).not.toHaveBeenCalled()
+    expect(storeSessions).not.toHaveBeenCalled()
+    expect(got.admin?.token).toBe('refreshed_admin_token')
+  })
 })
 
 describe('when existing session is expired', () => {
@@ -457,11 +550,11 @@ describe('when existing session is expired', () => {
     // Then
     expect(refreshAccessToken).toBeCalled()
     expect(exchangeAccessForApplicationTokens).toBeCalled()
-    expect(storeSessions).toBeCalledWith(validSessions)
+    expect(storeSessions).toBeCalledWith(fqdn, userId, validSessions[fqdn]![userId])
     expect(got).toEqual(validTokens)
     await expect(getLastSeenUserIdAfterAuth()).resolves.toBe('1234-5678')
     await expect(getLastSeenAuthMethod()).resolves.toEqual('device_auth')
-    expect(fetchSessions).toHaveBeenCalledOnce()
+    expect(fetchSessions).toHaveBeenCalledTimes(2)
   })
 
   test('attempts to refresh the token and executes a complete flow if identity returns an invalid grant error', async () => {
@@ -482,13 +575,60 @@ describe('when existing session is expired', () => {
     expect(storeSessions).toHaveBeenCalledOnce()
 
     // Verify the session was stored with email as alias
-    const storedSession = vi.mocked(storeSessions).mock.calls[0]![0]
-    expect(storedSession[fqdn]![userId]!.identity.alias).toBe('user@example.com')
+    const storedSession = vi.mocked(storeSessions).mock.calls[0]![2]
+    expect(storedSession.identity.alias).toBe('user@example.com')
 
     expect(got).toEqual(validTokens)
     await expect(getLastSeenUserIdAfterAuth()).resolves.toBe('1234-5678')
     await expect(getLastSeenAuthMethod()).resolves.toEqual('device_auth')
-    expect(fetchSessions).toHaveBeenCalledOnce()
+    expect(fetchSessions).toHaveBeenCalledTimes(2)
+  })
+
+  test('does not remove other sessions when a non-interactive refresh fails', async () => {
+    // Given
+    const otherUserId = 'another-user'
+    const sessions: Sessions = {
+      [fqdn]: {
+        ...validSessions[fqdn],
+        [otherUserId]: {
+          identity: {...validIdentityToken, userId: otherUserId},
+          applications: appTokens,
+        },
+      },
+      'another.fqdn.com': validSessions[fqdn]!,
+    }
+
+    vi.mocked(validateSession).mockResolvedValueOnce('needs_refresh')
+    vi.mocked(fetchSessions).mockResolvedValue(sessions)
+    vi.mocked(refreshAccessToken).mockRejectedValueOnce(new InvalidGrantError())
+
+    // When
+    await expect(ensureAuthenticated(defaultApplications, process.env, {noPrompt: true})).rejects.toThrow(
+      `The currently available CLI credentials are invalid.
+
+The CLI is currently unable to prompt for reauthentication.`,
+    )
+
+    // Then
+    expect(secureRemove).not.toHaveBeenCalled()
+    expect(removeFailedSession).toHaveBeenCalledWith(fqdn, userId, validSessions[fqdn]![userId])
+  })
+
+  test('clears only the invalid session when token exchange returns an invalid request', async () => {
+    // Given
+    vi.mocked(validateSession).mockResolvedValueOnce('needs_refresh')
+    vi.mocked(fetchSessions).mockResolvedValue(validSessions)
+    vi.mocked(refreshAccessToken).mockRejectedValueOnce(new InvalidRequestError())
+
+    // When
+    await expect(ensureAuthenticated(defaultApplications)).rejects.toMatchObject({
+      message: '\nError validating auth session',
+      tryMessage: "We've cleared the current session, please try again",
+    })
+
+    // Then
+    expect(secureRemove).not.toHaveBeenCalled()
+    expect(removeFailedSession).toHaveBeenCalledWith(fqdn, userId, validSessions[fqdn]![userId])
   })
 })
 
@@ -735,8 +875,8 @@ describe('ensureAuthenticated email fetch functionality', () => {
     const got = await ensureAuthenticated(defaultApplications)
 
     // Then
-    const storedSession = vi.mocked(storeSessions).mock.calls[0]![0]
-    expect(storedSession[fqdn]![userId]!.identity.alias).toBe('work@example.com')
+    const storedSession = vi.mocked(storeSessions).mock.calls[0]![2]
+    expect(storedSession.identity.alias).toBe('work@example.com')
     expect(got).toEqual(validTokens)
   })
 
@@ -749,7 +889,7 @@ describe('ensureAuthenticated email fetch functionality', () => {
     const got = await ensureAuthenticated(defaultApplications)
 
     // Then
-    expect(storeSessions).toBeCalledWith(validSessions)
+    expect(storeSessions).toBeCalledWith(fqdn, userId, validSessions[fqdn]![userId])
     expect(got).toEqual(validTokens)
   })
 
@@ -771,8 +911,8 @@ describe('ensureAuthenticated email fetch functionality', () => {
 
     // Then
     expect(businessPlatformRequest).not.toHaveBeenCalled()
-    const storedSession = vi.mocked(storeSessions).mock.calls[0]![0]
-    expect(storedSession[fqdn]![userId]!.identity.alias).toBe('user@example.com')
+    const storedSession = vi.mocked(storeSessions).mock.calls[0]![2]
+    expect(storedSession.identity.alias).toBe('user@example.com')
     expect(got).toEqual(validTokens)
   })
 
@@ -792,8 +932,8 @@ describe('ensureAuthenticated email fetch functionality', () => {
     const got = await ensureAuthenticated(defaultApplications)
 
     // Then
-    const storedSession = vi.mocked(storeSessions).mock.calls[0]![0]
-    expect(storedSession[fqdn]![userId]!.identity.alias).toBe('fallback@example.com')
+    const storedSession = vi.mocked(storeSessions).mock.calls[0]![2]
+    expect(storedSession.identity.alias).toBe('fallback@example.com')
     expect(got).toEqual(validTokens)
   })
 
@@ -817,8 +957,8 @@ describe('ensureAuthenticated email fetch functionality', () => {
     const got = await ensureAuthenticated(defaultApplications)
 
     // Then
-    const storedSession = vi.mocked(storeSessions).mock.calls[0]![0]
-    expect(storedSession[fqdn]![userId]!.identity.alias).toBe('my-custom-alias')
+    const storedSession = vi.mocked(storeSessions).mock.calls[0]![2]
+    expect(storedSession.identity.alias).toBe('my-custom-alias')
     expect(got).toEqual(validTokens)
   })
 
@@ -836,8 +976,8 @@ describe('ensureAuthenticated email fetch functionality', () => {
     const got = await ensureAuthenticated(defaultApplications)
 
     // Then
-    const storedSession = vi.mocked(storeSessions).mock.calls[0]![0]
-    expect(storedSession[fqdn]![userId]!.identity.alias).toBe(userId)
+    const storedSession = vi.mocked(storeSessions).mock.calls[0]![2]
+    expect(storedSession.identity.alias).toBe(userId)
     expect(got).toEqual(validTokens)
   })
 })