From e51860a81d6a6a4df39530a2d1d7f48b5f709b1c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Efe=20G=C3=B6kdemir?= Date: Mon, 19 Oct 2026 08:13:35 +0300 Subject: [PATCH 1/3] Fix isolation for concurrent CLI session refreshes --- .changeset/calm-dogs-tell.md | 6 + packages/cli-kit/package.json | 2 + .../cli-kit/src/private/node/conf-store.ts | 12 ++ .../cli-kit/src/private/node/session.test.ts | 161 +++++++++++++---- packages/cli-kit/src/private/node/session.ts | 84 ++++++--- .../private/node/session/store.lock.test.ts | 63 +++++++ .../src/private/node/session/store.test.ts | 107 ++++++++++- .../cli-kit/src/private/node/session/store.ts | 168 ++++++++++++++++-- .../src/public/node/session-prompt.test.ts | 4 + .../cli-kit/src/public/node/session-prompt.ts | 5 +- .../dev-server-session.test.ts | 30 ++++ .../theme-environment/dev-server-session.ts | 12 +- pnpm-lock.yaml | 6 + 13 files changed, 586 insertions(+), 74 deletions(-) create mode 100644 .changeset/calm-dogs-tell.md create mode 100644 packages/cli-kit/src/private/node/session/store.lock.test.ts diff --git a/.changeset/calm-dogs-tell.md b/.changeset/calm-dogs-tell.md new file mode 100644 index 00000000000..70a875a78eb --- /dev/null +++ b/.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. diff --git a/packages/cli-kit/package.json b/packages/cli-kit/package.json index c7cc524f7c5..37ae101c481 100644 --- a/packages/cli-kit/package.json +++ b/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", diff --git a/packages/cli-kit/src/private/node/conf-store.ts b/packages/cli-kit/src/private/node/conf-store.ts index 72b583f1f5f..b54d7135e13 100644 --- a/packages/cli-kit/src/private/node/conf-store.ts +++ b/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 { 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' } diff --git a/packages/cli-kit/src/private/node/session.test.ts b/packages/cli-kit/src/private/node/session.test.ts index cf0d9fc6445..f3330efeeb3 100644 --- a/packages/cli-kit/src/private/node/session.test.ts +++ b/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,17 @@ 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), + setCurrentSessionId, + remove: () => secureRemove(), + removeSession: (fqdn, id, session) => removeFailedSession(fqdn, id, session), + } + return operation(store) + }) setLastSeenUserIdAfterAuth(undefined as any) setLastSeenAuthMethod('none') setCommandSessionId(undefined) @@ -207,8 +227,8 @@ 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') @@ -216,7 +236,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 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 +248,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 +280,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 +301,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 +325,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 +344,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 +431,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 +459,42 @@ 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('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 +509,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 +534,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 +834,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 +848,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 +870,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 +891,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 +916,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 +935,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) }) }) diff --git a/packages/cli-kit/src/private/node/session.ts b/packages/cli-kit/src/private/node/session.ts index d8562e6c17f..e1b47ed7994 100644 --- a/packages/cli-kit/src/private/node/session.ts +++ b/packages/cli-kit/src/private/node/session.ts @@ -9,19 +9,19 @@ 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' import {isThemeAccessSession} from './api/rest.js' -import {getCurrentSessionId, setCurrentSessionId} from './conf-store.js' +import {getCurrentSessionId} from './conf-store.js' import {UserEmailQueryString, UserEmailQuery} from './api/graphql/business-platform-destinations/user-email.js' import {outputContent, outputToken, outputDebug, outputCompleted} from '../../public/node/output.js' 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' @@ -248,6 +248,8 @@ ${outputToken.json(applications)} const validationResult = await validateSession(scopes, applications, currentSession) let newSession = {} + let sessionWasRefreshed = false + let refreshedSession: Session | undefined if (validationResult === 'needs_full_auth') { await throwOnNoPrompt(noPrompt) @@ -255,32 +257,62 @@ ${outputToken.json(applications)} newSession = await executeCompleteFlow(applications, currentSession?.identity.alias) } else if (validationResult === 'needs_refresh' || forceRefresh) { outputDebug(outputContent`The current session is valid but needs refresh. Refreshing...`) - try { - newSession = await refreshTokens(currentSession!, applications) - } catch (error) { - if (error instanceof InvalidGrantError) { - 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") - } else { + const refreshResult = await sessionStore.withSessionStoreLock(async (store) => { + const latestSessions = (await store.fetch()) ?? {} + const latestSession = currentSessionId ? latestSessions[fqdn]?.[currentSessionId] : undefined + if (!latestSession) { + return {kind: 'reauthenticate' as const, alias: currentSession?.identity.alias} + } + + if (!currentSession || !hasSameSessionCredentials(latestSession, currentSession)) { + const latestValidationResult = await validateSession(scopes, applications, latestSession) + if (latestValidationResult === 'needs_full_auth') { + return {kind: 'reauthenticate' as const, alias: latestSession.identity.alias} + } + if (latestValidationResult === 'ok' && !forceRefresh) { + return {kind: 'session' as const, session: latestSession, refreshed: false} + } + } + + try { + const refreshedSession = {...latestSession, ...(await refreshTokens(latestSession, applications))} + await store.storeSession(fqdn, latestSession.identity.userId, refreshedSession) + return {kind: 'session' as const, session: refreshedSession, refreshed: true} + } catch (error) { + if (error instanceof InvalidGrantError) { + if (noPrompt) { + await store.removeSession(fqdn, latestSession.identity.userId, latestSession) + await throwOnNoPrompt(true) + } + return {kind: 'reauthenticate' as const, alias: latestSession.identity.alias} + } + if (error instanceof InvalidRequestError) { + await store.removeSession(fqdn, latestSession.identity.userId, latestSession) + throw new AbortError('\nError validating auth session', "We've cleared the current session, please try again") + } throw error } + }) + + if (refreshResult.kind === 'reauthenticate') { + await throwOnNoPrompt(noPrompt) + newSession = await executeCompleteFlow(applications, refreshResult.alias) + } else { + refreshedSession = refreshResult.session + sessionWasRefreshed = refreshResult.refreshed } } - const completeSession = {...currentSession, ...newSession} as Session + const completeSession = {...(refreshedSession ?? 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.withSessionStoreLock(async (store) => { + await store.storeSession(fqdn, newSessionId, completeSession) + if (!commandSessionId) store.setCurrentSessionId(newSessionId) + }) + } else if (sessionWasRefreshed && !commandSessionId) { + await sessionStore.withSessionStoreLock(async (store) => store.setCurrentSessionId(newSessionId)) } const tokens = await tokensFor(applications, completeSession) @@ -297,7 +329,6 @@ ${outputToken.json(applications)} async function throwOnNoPrompt(noPrompt: boolean) { if (!noPrompt) return - await logout() throw new AbortError( `The currently available CLI credentials are invalid. @@ -306,6 +337,15 @@ The CLI is currently unable to prompt for reauthentication.`, ) } +function hasSameSessionCredentials(first: Session, second: Session): boolean { + return ( + first.identity.userId === second.identity.userId && + first.identity.accessToken === second.identity.accessToken && + first.identity.refreshToken === second.identity.refreshToken && + first.identity.expiresAt.getTime() === second.identity.expiresAt.getTime() + ) +} + /** * Execute the full authentication flow. * diff --git a/packages/cli-kit/src/private/node/session/store.lock.test.ts b/packages/cli-kit/src/private/node/session/store.lock.test.ts new file mode 100644 index 00000000000..ba9c9886d07 --- /dev/null +++ b/packages/cli-kit/src/private/node/session/store.lock.test.ts @@ -0,0 +1,63 @@ +import {withSessionStoreLock} from './store.js' +import {inTemporaryDirectory} from '../../../public/node/fs.js' +import {describe, expect, test} from 'vitest' + +function deferred() { + let resolvePromise!: () => void + const promise = new Promise((resolve) => { + resolvePromise = resolve + }) + return {promise, resolve: resolvePromise} +} + +describe('withSessionStoreLock', () => { + test('serializes session-store operations across callers', async () => { + await inTemporaryDirectory(async (lockPath) => { + // Given + const firstEntered = deferred() + const releaseFirst = deferred() + const secondRequested = deferred() + const events: string[] = [] + + // When + const first = withSessionStoreLock(async () => { + events.push('first:start') + firstEntered.resolve() + await releaseFirst.promise + events.push('first:end') + }, lockPath) + + await firstEntered.promise + const second = (async () => { + secondRequested.resolve() + await withSessionStoreLock(async () => { + events.push('second:start') + events.push('second:end') + }, lockPath) + })() + await secondRequested.promise + releaseFirst.resolve() + await Promise.all([first, second]) + + // Then + expect(events).toEqual(['first:start', 'first:end', 'second:start', 'second:end']) + }) + }) + + test('releases the lock when the operation fails', async () => { + await inTemporaryDirectory(async (lockPath) => { + // Given + const operationError = new Error('session update failed') + + // When + await expect( + withSessionStoreLock(async () => { + throw operationError + }, lockPath), + ).rejects.toBe(operationError) + + // Then + await expect(withSessionStoreLock(async () => 'completed', lockPath)).resolves.toBe('completed') + }) + }) +}) diff --git a/packages/cli-kit/src/private/node/session/store.test.ts b/packages/cli-kit/src/private/node/session/store.test.ts index 2d2ab6b0fd8..50b716b4299 100644 --- a/packages/cli-kit/src/private/node/session/store.test.ts +++ b/packages/cli-kit/src/private/node/session/store.test.ts @@ -1,12 +1,30 @@ 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 { + store, + storeSession, + fetch, + remove, + removeSession, + getSessionAlias, + setSessionAlias, + findSessionByAlias, +} from './store.js' +import { + getCurrentSessionId, + getSessionStoreLockPath, + getSessions, + removeSessions, + setSessions, + removeCurrentSessionId, +} from '../conf-store.js' import {identityFqdn} from '../../../public/node/context/fqdn.js' +import lockfile from 'proper-lockfile' import {describe, expect, vi, test, beforeEach} from 'vitest' vi.mock('../conf-store.js') vi.mock('../../../public/node/context/fqdn.js') +vi.mock('proper-lockfile') const mockSessions: Sessions = { 'identity.fqdn.com': { @@ -37,6 +55,8 @@ const mockSessions: Sessions = { describe('session store', () => { beforeEach(() => { vi.mocked(identityFqdn).mockResolvedValue('identity.fqdn.com') + vi.mocked(getSessionStoreLockPath).mockReturnValue('/tmp/shopify-cli-kit-test') + vi.mocked(lockfile.lock).mockResolvedValue(vi.fn()) }) describe('store', () => { @@ -49,6 +69,89 @@ describe('session store', () => { }) }) + describe('storeSession', () => { + test('updates one session while preserving the latest sessions for other accounts', async () => { + // Given + const otherFqdn = 'other.identity.com' + const latestSessions: Sessions = { + ...mockSessions, + [otherFqdn]: mockSessions['identity.fqdn.com']!, + } + vi.mocked(getSessions).mockReturnValue(JSON.stringify(latestSessions)) + const updatedSession = { + ...mockSessions['identity.fqdn.com']!.user1!, + identity: {...mockSessions['identity.fqdn.com']!.user1!.identity, accessToken: 'updated-token'}, + } + + // When + await storeSession('identity.fqdn.com', 'user1', updatedSession) + + // Then + const storedSessions = JSON.parse(vi.mocked(setSessions).mock.calls[0]![0]) as Sessions + expect(storedSessions['identity.fqdn.com']!.user1!.identity.accessToken).toBe('updated-token') + expect(storedSessions['identity.fqdn.com']!.user2).toEqual( + JSON.parse(JSON.stringify(mockSessions['identity.fqdn.com']!.user2)), + ) + expect(storedSessions[otherFqdn]).toEqual(JSON.parse(JSON.stringify(latestSessions[otherFqdn]))) + }) + }) + + describe('removeSession', () => { + test('removes only the matching session and preserves other accounts', async () => { + // Given + const otherFqdn = 'other.identity.com' + vi.mocked(getSessions).mockReturnValue( + JSON.stringify({...mockSessions, [otherFqdn]: mockSessions['identity.fqdn.com']}), + ) + + // When + const removed = await removeSession('identity.fqdn.com', 'user1', mockSessions['identity.fqdn.com']!.user1) + + // Then + expect(removed).toBe(true) + const storedSessions = JSON.parse(vi.mocked(setSessions).mock.calls[0]![0]) as Sessions + expect(storedSessions['identity.fqdn.com']!.user1).toBeUndefined() + expect(storedSessions['identity.fqdn.com']!.user2).toEqual( + JSON.parse(JSON.stringify(mockSessions['identity.fqdn.com']!.user2)), + ) + expect(storedSessions[otherFqdn]).toEqual(JSON.parse(JSON.stringify(mockSessions['identity.fqdn.com']))) + }) + + test('clears the current session id when removing that session', async () => { + // Given + vi.mocked(getSessions).mockReturnValue(JSON.stringify(mockSessions)) + vi.mocked(getCurrentSessionId).mockReturnValue('user1') + + // When + await removeSession('identity.fqdn.com', 'user1', mockSessions['identity.fqdn.com']!.user1) + + // Then + expect(removeCurrentSessionId).toHaveBeenCalledOnce() + }) + + test('keeps a session that another process has already refreshed', async () => { + // Given + const refreshedSessions: Sessions = { + ...mockSessions, + 'identity.fqdn.com': { + ...mockSessions['identity.fqdn.com'], + user1: { + ...mockSessions['identity.fqdn.com']!.user1!, + identity: {...mockSessions['identity.fqdn.com']!.user1!.identity, refreshToken: 'rotated-token'}, + }, + }, + } + vi.mocked(getSessions).mockReturnValue(JSON.stringify(refreshedSessions)) + + // When + const removed = await removeSession('identity.fqdn.com', 'user1', mockSessions['identity.fqdn.com']!.user1) + + // Then + expect(removed).toBe(false) + expect(setSessions).not.toHaveBeenCalled() + }) + }) + describe('fetch', () => { test('returns undefined when no sessions exist', async () => { // Given diff --git a/packages/cli-kit/src/private/node/session/store.ts b/packages/cli-kit/src/private/node/session/store.ts index 86902433c44..65c91c29361 100644 --- a/packages/cli-kit/src/private/node/session/store.ts +++ b/packages/cli-kit/src/private/node/session/store.ts @@ -1,15 +1,72 @@ import {SessionsSchema} from './schema.js' -import {getSessions, removeCurrentSessionId, removeSessions, setSessions} from '../conf-store.js' +import { + getSessionStoreLockPath, + getCurrentSessionId, + getSessions, + removeCurrentSessionId, + removeSessions, + setCurrentSessionId, + setSessions, +} from '../conf-store.js' import {identityFqdn} from '../../../public/node/context/fqdn.js' +import lockfile from 'proper-lockfile' import type {Session, Sessions} from './schema.js' +export interface LockedSessionStore { + fetch(): Promise + store(sessions: Sessions): Promise + storeSession(fqdn: string, userId: string, session: Session): Promise + setCurrentSessionId(sessionId: string): void + remove(): Promise + removeSession(fqdn: string, userId: string, expectedSession?: Session): Promise +} + +/** + * Runs a session-store operation while holding a lock shared by all CLI processes. + * + * @param operation - The operation to run while the store is locked. + * @param lockPath - The lock target. This parameter is exposed for isolated tests. + * @returns The operation result. + */ +export async function withSessionStoreLock( + operation: (store: LockedSessionStore) => Promise, + lockPath = getSessionStoreLockPath(), +): Promise { + const release = await lockfile.lock(lockPath, { + retries: {retries: 20, factor: 1.2, minTimeout: 25, maxTimeout: 500}, + }) + + let result: T + try { + result = await operation({ + fetch: fetchUnlocked, + store: storeUnlocked, + storeSession: storeSessionUnlocked, + setCurrentSessionId, + remove: removeUnlocked, + removeSession: removeSessionUnlocked, + }) + } catch (error) { + try { + await release() + // Preserve the operation error if releasing the lock also fails. + // eslint-disable-next-line no-catch-all/no-catch-all + } catch { + // The operation error is more useful to the caller than a cleanup error. + } + throw error + } + + await release() + return result +} + /** * Serializes the session as a JSON and stores it in the system. * @param session - the session to store. */ export async function store(sessions: Sessions) { - const jsonSessions = JSON.stringify(sessions) - setSessions(jsonSessions) + await withSessionStoreLock((store) => store.store(sessions)) } /** @@ -18,6 +75,38 @@ export async function store(sessions: Sessions) { * @returns Returns a promise that resolves with the sessions object if it exists and is valid. */ export async function fetch(): Promise { + return withSessionStoreLock((store) => store.fetch()) +} + +/** + * Stores one session without replacing sessions saved by another CLI process. + * + * @param fqdn - The identity service hostname for the session. + * @param userId - The user ID for the session. + * @param session - The session to store. + */ +export async function storeSession(fqdn: string, userId: string, session: Session): Promise { + await withSessionStoreLock((store) => store.storeSession(fqdn, userId, session)) +} + +/** + * Removes a session without affecting other users or identity services. + * + * @param fqdn - The identity service hostname for the session. + * @param userId - The user ID for the session. + * @param expectedSession - If provided, remove only if the stored credentials still match. + * @returns Whether the session was removed. + */ +export async function removeSession(fqdn: string, userId: string, expectedSession?: Session): Promise { + return withSessionStoreLock((store) => store.removeSession(fqdn, userId, expectedSession)) +} + +async function storeUnlocked(sessions: Sessions): Promise { + const jsonSessions = JSON.stringify(sessions) + setSessions(jsonSessions) +} + +async function fetchUnlocked(): Promise { const content = getSessions() if (!content) { @@ -29,7 +118,7 @@ export async function fetch(): Promise { contentJson = JSON.parse(content) } catch (error) { if (!(error instanceof SyntaxError)) throw error - await remove() + await removeUnlocked() return undefined } @@ -37,17 +126,66 @@ export async function fetch(): Promise { if (parsedSessions.success) { return parsedSessions.data } else { - await remove() + await removeUnlocked() return undefined } } +async function storeSessionUnlocked(fqdn: string, userId: string, session: Session): Promise { + const sessions = (await fetchUnlocked()) ?? {} + await storeUnlocked({ + ...sessions, + [fqdn]: {...sessions[fqdn], [userId]: session}, + }) +} + +async function removeSessionUnlocked(fqdn: string, userId: string, expectedSession?: Session): Promise { + const sessions = await fetchUnlocked() + const currentSession = sessions?.[fqdn]?.[userId] + if (!sessions || !currentSession || (expectedSession && !sameSessionCredentials(currentSession, expectedSession))) { + return false + } + + const remainingFqdnSessions = Object.fromEntries( + Object.entries(sessions[fqdn]!).filter(([sessionId]) => sessionId !== userId), + ) + const updatedSessions = {...sessions} + if (Object.keys(remainingFqdnSessions).length === 0) { + const remainingSessions = Object.fromEntries( + Object.entries(sessions).filter(([sessionFqdn]) => sessionFqdn !== fqdn), + ) + await storeUnlocked(remainingSessions) + } else { + updatedSessions[fqdn] = remainingFqdnSessions + await storeUnlocked(updatedSessions) + } + + if (getCurrentSessionId() === userId) { + removeCurrentSessionId() + } + + return true +} + +function sameSessionCredentials(first: Session, second: Session): boolean { + return ( + first.identity.userId === second.identity.userId && + first.identity.accessToken === second.identity.accessToken && + first.identity.refreshToken === second.identity.refreshToken && + first.identity.expiresAt.getTime() === second.identity.expiresAt.getTime() + ) +} + +async function removeUnlocked(): Promise { + removeSessions() + removeCurrentSessionId() +} + /** * Removes a session from the system. */ export async function remove() { - removeSessions() - removeCurrentSessionId() + await withSessionStoreLock((store) => store.remove()) } /** @@ -72,15 +210,17 @@ export async function getSessionAlias(userId: string): Promise { - const sessions = await fetch() - if (!sessions) return + await withSessionStoreLock(async (store) => { + const sessions = await store.fetch() + if (!sessions) return - const fqdn = await identityFqdn() - const session: Session | undefined = sessions[fqdn]?.[userId] - if (!session) return + const fqdn = await identityFqdn() + const session: Session | undefined = sessions[fqdn]?.[userId] + if (!session) return - session.identity.alias = alias - await store(sessions) + session.identity.alias = alias + await store.store(sessions) + }) } /** diff --git a/packages/cli-kit/src/public/node/session-prompt.test.ts b/packages/cli-kit/src/public/node/session-prompt.test.ts index afd1d72aecd..8e61d0b17ff 100644 --- a/packages/cli-kit/src/public/node/session-prompt.test.ts +++ b/packages/cli-kit/src/public/node/session-prompt.test.ts @@ -7,6 +7,7 @@ import * as sessionStore from '../../private/node/session/store.js' import {Sessions} from '../../private/node/session/schema.js' import {describe, expect, vi, test, beforeEach} from 'vitest' +import type {LockedSessionStore} from '../../private/node/session/store.js' vi.mock('./ui.js') vi.mock('./session.js') @@ -46,6 +47,9 @@ describe('promptSessionSelect', () => { vi.mocked(ensureAuthenticatedUser).mockResolvedValue({userId: 'new-user-id'}) vi.mocked(sessionStore.getSessionAlias).mockResolvedValue('new-alias') vi.mocked(sessionStore.findSessionByAlias).mockResolvedValue(undefined) + vi.mocked(sessionStore.withSessionStoreLock).mockImplementation(async (operation) => + operation({setCurrentSessionId} as LockedSessionStore), + ) }) test('prompts user to create new session when no existing sessions', async () => { diff --git a/packages/cli-kit/src/public/node/session-prompt.ts b/packages/cli-kit/src/public/node/session-prompt.ts index 278be669125..59a4ba31750 100644 --- a/packages/cli-kit/src/public/node/session-prompt.ts +++ b/packages/cli-kit/src/public/node/session-prompt.ts @@ -2,7 +2,6 @@ import {renderSelectPrompt, renderTextPrompt} from './ui.js' import {ensureAuthenticatedUser} from './session.js' import {identityFqdn} from './context/fqdn.js' import * as sessionStore from '../../private/node/session/store.js' -import {setCurrentSessionId} from '../../private/node/conf-store.js' import type {Sessions} from '../../private/node/session/schema.js' const NEW_LOGIN_VALUE = 'NEW_LOGIN' @@ -93,7 +92,7 @@ export async function promptSessionSelect(alias?: string): Promise { if (alias) { const userId = await sessionStore.findSessionByAlias(alias) if (userId) { - setCurrentSessionId(userId) + await sessionStore.withSessionStoreLock(async (store) => store.setCurrentSessionId(userId)) return alias } } @@ -110,6 +109,6 @@ export async function promptSessionSelect(alias?: string): Promise { return handleNewLogin() } - setCurrentSessionId(selectedValue) + await sessionStore.withSessionStoreLock(async (store) => store.setCurrentSessionId(selectedValue)) return choices.find((choice) => choice.value === selectedValue)?.label ?? selectedValue } diff --git a/packages/theme/src/cli/utilities/theme-environment/dev-server-session.test.ts b/packages/theme/src/cli/utilities/theme-environment/dev-server-session.test.ts index 17d8d65417a..6578f41c7b3 100644 --- a/packages/theme/src/cli/utilities/theme-environment/dev-server-session.test.ts +++ b/packages/theme/src/cli/utilities/theme-environment/dev-server-session.test.ts @@ -11,6 +11,7 @@ import {fetchThemeAssets, themeDelete} from '@shopify/cli-kit/node/themes/api' import {describe, expect, test, vi, beforeEach} from 'vitest' import {AbortError} from '@shopify/cli-kit/node/error' import {outputContent, outputToken} from '@shopify/cli-kit/node/output' +import * as outputModule from '@shopify/cli-kit/node/output' vi.mock('@shopify/cli-kit/node/session') vi.mock('@shopify/cli-kit/node/themes/api') @@ -260,5 +261,34 @@ describe('dev server session', async () => { ) expect(fetchOrCreateCrawlerSignatureHeaders).toHaveBeenCalledOnce() }) + + test('warns when the background refresh needs interactive reauthentication', async () => { + // Given + vi.useFakeTimers() + const outputWarn = vi.spyOn(outputModule, 'outputWarn').mockImplementation(() => {}) + try { + vi.mocked(ensureAuthenticatedStorefront).mockResolvedValue('storefront_token') + vi.mocked(getStorefrontSessionCookies).mockResolvedValue({ + _shopify_essential: ':AABBCCDDEEFFGGHH==123:', + }) + vi.mocked(ensureAuthenticatedThemes).mockResolvedValue({token: 'token_1', storeFqdn}) + await initializeDevServerSession(themeId, adminSession) + vi.mocked(ensureAuthenticatedThemes).mockRejectedValueOnce( + new AbortError('The credentials are invalid.', 'Sign in again in an interactive terminal.'), + ) + + // When + await vi.advanceTimersByTimeAsync(30 * 60 * 1000) + + // Then + expect(outputWarn).toHaveBeenCalledWith( + 'Theme dev could not refresh its Shopify session. Restart it in an interactive terminal to sign in again.', + ) + } finally { + vi.clearAllTimers() + vi.useRealTimers() + outputWarn.mockRestore() + } + }) }) }) diff --git a/packages/theme/src/cli/utilities/theme-environment/dev-server-session.ts b/packages/theme/src/cli/utilities/theme-environment/dev-server-session.ts index e60705a24df..d1d74c3207f 100644 --- a/packages/theme/src/cli/utilities/theme-environment/dev-server-session.ts +++ b/packages/theme/src/cli/utilities/theme-environment/dev-server-session.ts @@ -4,7 +4,7 @@ import {getStorefrontSessionCookies, ShopifyEssentialError} from './storefront-s import {buildBaseStorefrontUrl} from './storefront-renderer.js' import {fetchThemeAssets} from '@shopify/cli-kit/node/themes/api' import {AbortError} from '@shopify/cli-kit/node/error' -import {outputDebug, outputContent, outputToken} from '@shopify/cli-kit/node/output' +import {outputDebug, outputContent, outputToken, outputWarn} from '@shopify/cli-kit/node/output' import {AdminSession, ensureAuthenticatedStorefront, ensureAuthenticatedThemes} from '@shopify/cli-kit/node/session' import {recordError, recordEvent} from '@shopify/cli-kit/node/analytics' @@ -59,7 +59,15 @@ export async function initializeDevServerSession( session .refresh() .then(() => outputDebug('Refreshed theme session via auto-refresher...')) - .catch(() => outputDebug('Session could not be refreshed.')) + .catch((error: unknown) => { + if (error instanceof AbortError) { + outputWarn( + 'Theme dev could not refresh its Shopify session. Restart it in an interactive terminal to sign in again.', + ) + } else { + outputDebug('Session could not be refreshed.') + } + }) }, SESSION_TIMEOUT_IN_MS) return session diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 7a1f983b4f9..6671b1afa94 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -448,6 +448,9 @@ importers: pathe: specifier: 1.1.2 version: 1.1.2 + proper-lockfile: + specifier: 4.1.2 + version: 4.1.2 react: specifier: 19.2.4 version: 19.2.4 @@ -491,6 +494,9 @@ importers: '@types/lodash': specifier: 4.17.24 version: 4.17.24 + '@types/proper-lockfile': + specifier: 4.1.4 + version: 4.1.4 '@types/react': specifier: 18.3.12 version: 18.3.12 From 6bda2f8e39def123248020c02fffbfcd5b7f2f40 Mon Sep 17 00:00:00 2001 From: Codex Date: Tue, 27 Oct 2026 20:11:23 +0300 Subject: [PATCH 2/3] Wait for session refresh lock retries --- .../private/node/session/store.lock.test.ts | 30 +++++++++++++++++++ .../cli-kit/src/private/node/session/store.ts | 12 ++++++-- 2 files changed, 39 insertions(+), 3 deletions(-) diff --git a/packages/cli-kit/src/private/node/session/store.lock.test.ts b/packages/cli-kit/src/private/node/session/store.lock.test.ts index ba9c9886d07..8569f16a986 100644 --- a/packages/cli-kit/src/private/node/session/store.lock.test.ts +++ b/packages/cli-kit/src/private/node/session/store.lock.test.ts @@ -44,6 +44,36 @@ describe('withSessionStoreLock', () => { }) }) + test('waits for a session operation that outlasts the default lock retry window', async () => { + await inTemporaryDirectory(async (lockPath) => { + // Given + const firstEntered = deferred() + const releaseFirst = deferred() + const events: string[] = [] + + // When + const first = withSessionStoreLock(async () => { + events.push('first:start') + firstEntered.resolve() + await releaseFirst.promise + events.push('first:end') + }, lockPath) + + await firstEntered.promise + const second = withSessionStoreLock(async () => { + events.push('second:start') + events.push('second:end') + }, lockPath) + + await new Promise((resolve) => setTimeout(resolve, 5000)) + releaseFirst.resolve() + await Promise.all([first, second]) + + // Then + expect(events).toEqual(['first:start', 'first:end', 'second:start', 'second:end']) + }) + }, 10_000) + test('releases the lock when the operation fails', async () => { await inTemporaryDirectory(async (lockPath) => { // Given diff --git a/packages/cli-kit/src/private/node/session/store.ts b/packages/cli-kit/src/private/node/session/store.ts index 65c91c29361..c52cd4035ce 100644 --- a/packages/cli-kit/src/private/node/session/store.ts +++ b/packages/cli-kit/src/private/node/session/store.ts @@ -12,6 +12,13 @@ import {identityFqdn} from '../../../public/node/context/fqdn.js' import lockfile from 'proper-lockfile' import type {Session, Sessions} from './schema.js' +const SESSION_STORE_LOCK_RETRIES = { + retries: 360, + factor: 1.2, + minTimeout: 25, + maxTimeout: 500, +} + export interface LockedSessionStore { fetch(): Promise store(sessions: Sessions): Promise @@ -32,9 +39,8 @@ export async function withSessionStoreLock( operation: (store: LockedSessionStore) => Promise, lockPath = getSessionStoreLockPath(), ): Promise { - const release = await lockfile.lock(lockPath, { - retries: {retries: 20, factor: 1.2, minTimeout: 25, maxTimeout: 500}, - }) + // Refreshes hold this lock while auth and application-token requests use their own timeout and retry budgets. + const release = await lockfile.lock(lockPath, {retries: SESSION_STORE_LOCK_RETRIES}) let result: T try { From e197af311adf847710b124a8d043ce1b63f3a579 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Efe=20G=C3=B6kdemir?= Date: Wed, 28 Oct 2026 22:14:00 +0300 Subject: [PATCH 3/3] Preserve concurrent CLI session selection --- .../cli-kit/src/private/node/session.test.ts | 41 +++++++++++++++++++ packages/cli-kit/src/private/node/session.ts | 7 +++- .../cli-kit/src/private/node/session/store.ts | 2 + 3 files changed, 48 insertions(+), 2 deletions(-) diff --git a/packages/cli-kit/src/private/node/session.test.ts b/packages/cli-kit/src/private/node/session.test.ts index f3330efeeb3..f7600bd5961 100644 --- a/packages/cli-kit/src/private/node/session.test.ts +++ b/packages/cli-kit/src/private/node/session.test.ts @@ -152,6 +152,7 @@ beforeEach(() => { 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), @@ -464,6 +465,46 @@ describe('when existing session is valid', () => { 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 = { diff --git a/packages/cli-kit/src/private/node/session.ts b/packages/cli-kit/src/private/node/session.ts index e1b47ed7994..64ad0158355 100644 --- a/packages/cli-kit/src/private/node/session.ts +++ b/packages/cli-kit/src/private/node/session.ts @@ -230,7 +230,8 @@ export async function ensureAuthenticated( const sessions = (await sessionStore.fetch()) ?? {} - let currentSessionId = forceNewSession ? undefined : (commandSessionId ?? getCurrentSessionId()) + const storedCurrentSessionId = commandSessionId ? undefined : getCurrentSessionId() + let currentSessionId = forceNewSession ? undefined : (commandSessionId ?? storedCurrentSessionId) if (!currentSessionId && !commandSessionId) { const userIds = Object.keys(sessions[fqdn] ?? {}) if (userIds.length > 0) currentSessionId = userIds[0] @@ -312,7 +313,9 @@ ${outputToken.json(applications)} if (!commandSessionId) store.setCurrentSessionId(newSessionId) }) } else if (sessionWasRefreshed && !commandSessionId) { - await sessionStore.withSessionStoreLock(async (store) => store.setCurrentSessionId(newSessionId)) + await sessionStore.withSessionStoreLock(async (store) => { + if (store.getCurrentSessionId() === storedCurrentSessionId) store.setCurrentSessionId(newSessionId) + }) } const tokens = await tokensFor(applications, completeSession) diff --git a/packages/cli-kit/src/private/node/session/store.ts b/packages/cli-kit/src/private/node/session/store.ts index c52cd4035ce..8032f367446 100644 --- a/packages/cli-kit/src/private/node/session/store.ts +++ b/packages/cli-kit/src/private/node/session/store.ts @@ -23,6 +23,7 @@ export interface LockedSessionStore { fetch(): Promise store(sessions: Sessions): Promise storeSession(fqdn: string, userId: string, session: Session): Promise + getCurrentSessionId(): string | undefined setCurrentSessionId(sessionId: string): void remove(): Promise removeSession(fqdn: string, userId: string, expectedSession?: Session): Promise @@ -48,6 +49,7 @@ export async function withSessionStoreLock( fetch: fetchUnlocked, store: storeUnlocked, storeSession: storeSessionUnlocked, + getCurrentSessionId, setCurrentSessionId, remove: removeUnlocked, removeSession: removeSessionUnlocked,