Skip to content

fix: guard workspace auth refresh races #11726

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
benceruleanlu wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix: guard workspace auth refresh races #11726

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
59a52e5
fix: guard workspace auth refresh races
benceruleanlu Apr 28, 2026
5584911
Merge remote-tracking branch 'origin/main' into ben/fe-485-workspace-…
benceruleanlu Apr 28, 2026
c3bcc28
fix: retry preserved workspace token refreshes
benceruleanlu Apr 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
118 changes: 76 additions & 42 deletions src/platform/workspace/stores/useWorkspaceAuth.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -671,14 +671,7 @@ describe('useWorkspaceAuthStore', () => {
})

describe('refreshToken retry/race paths', () => {
// NOTE: This test documents the CURRENT behavior — exhausted refresh
// retries clear the workspace context unconditionally, even when the
// existing workspace token is still within its expiry window. That is a
// UX gap (transient backend outage manifests as forced logout) and the
// store should preserve a still-valid token across transient
// TOKEN_EXCHANGE_FAILED errors. Update the assertion alongside any source
// change that tracks token expiry to skip the context clear.
it('retries up to 3 times with exponential backoff on TOKEN_EXCHANGE_FAILED, then clears context', async () => {
it('retries up to 3 times with exponential backoff on TOKEN_EXCHANGE_FAILED, then preserves valid context', async () => {
mockGetIdToken.mockResolvedValue('firebase-token-xyz')

// Initial successful switchWorkspace establishes context.
Expand All @@ -689,10 +682,11 @@ describe('useWorkspaceAuthStore', () => {
vi.stubGlobal('fetch', mockFetch)

const store = useWorkspaceAuthStore()
const { currentWorkspace } = storeToRefs(store)
const { currentWorkspace, workspaceToken, error } = storeToRefs(store)

await store.switchWorkspace('workspace-123')
expect(currentWorkspace.value).not.toBeNull()
expect(currentWorkspace.value).toEqual(mockWorkspaceWithRole)
expect(workspaceToken.value).toBe('workspace-token-abc')

// Subsequent refresh attempts all fail with 500 (TOKEN_EXCHANGE_FAILED).
mockFetch.mockResolvedValue({
Expand All @@ -711,8 +705,11 @@ describe('useWorkspaceAuthStore', () => {

const refreshPromise = store.refreshToken()

// Drain the four attempts (initial + 3 retries) and their backoff delays.
await vi.runAllTimersAsync()
// Drain only the retry backoff delays; do not advance to the scheduled
// proactive refresh timer for the still-valid token.
await vi.advanceTimersByTimeAsync(1000)
await vi.advanceTimersByTimeAsync(2000)
await vi.advanceTimersByTimeAsync(4000)
await refreshPromise

// 1 initial switchWorkspace + 4 refresh attempts = 5 total fetch calls.
Expand All @@ -734,8 +731,32 @@ describe('useWorkspaceAuthStore', () => {
)
).toBe(true)

// After the final failure the context is cleared.
expect(currentWorkspace.value).toBeNull()
// After the final transient failure the still-valid context is preserved.
expect(currentWorkspace.value).toEqual(mockWorkspaceWithRole)
expect(workspaceToken.value).toBe('workspace-token-abc')
expect(
sessionStorage.getItem(WORKSPACE_STORAGE_KEYS.CURRENT_WORKSPACE)
).toBe(JSON.stringify(mockWorkspaceWithRole))
expect(sessionStorage.getItem(WORKSPACE_STORAGE_KEYS.TOKEN)).toBe(
'workspace-token-abc'
)
expect(error.value).toBeNull()
expect(consoleErrorSpy).not.toHaveBeenCalled()

mockFetch.mockResolvedValueOnce({
ok: true,
json: () =>
Promise.resolve({ ...mockTokenResponse, token: 'retry-token' })
})

await vi.advanceTimersByTimeAsync(7999)
Copy link
Copy Markdown
Contributor

@christian-byrne christian-byrne May 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick (non-blocking): the 7999 / 1 split asserts the scheduled retry fires at exactly 8000ms (= baseDelayMs * 2^maxRetries). Would a one-line comment naming the formula make a future failure here self-explanatory?

expect(mockFetch).toHaveBeenCalledTimes(5)

await vi.advanceTimersByTimeAsync(1)
expect(mockFetch).toHaveBeenCalledTimes(6)
await vi.waitFor(() => {
expect(workspaceToken.value).toBe('retry-token')
})

consoleErrorSpy.mockRestore()
consoleWarnSpy.mockRestore()
Expand Down Expand Up @@ -776,16 +797,7 @@ describe('useWorkspaceAuthStore', () => {
consoleErrorSpy.mockRestore()
})

// KNOWN BUG (.fails): when an in-flight refresh's switchWorkspace call is
// already past its requestId-staleness check and awaiting the token-exchange
// fetch, switchWorkspace has no post-await commit guard. If the user
// switches workspaces and the stale refresh's fetch resolves AFTER the new
// switch has committed, the stale response will overwrite the new
// workspace's currentWorkspace/workspaceToken/sessionStorage. Mark this
// expected-fail until switchWorkspace gains a commit-time staleness check
// (e.g. compare captured requestId or expected workspaceId before
// assigning state). Removing `.fails` once fixed will catch regressions.
it.fails('the new workspace wins when the stale refresh resolves last', async () => {
it('the new workspace wins when the stale refresh resolves last', async () => {
Copy link
Copy Markdown
Contributor

@christian-byrne christian-byrne May 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (non-blocking): the new catch-block ABA guard (workspaceAuthStore.ts:296-301) is not exercised by tests. This race test resolves the stale fetch with ok: true, which only covers the success-path guard. Would adding a test where the hung refresh fetch rejects or returns 500 after a new switchWorkspace has committed help lock in this branch?

mockGetIdToken.mockResolvedValue('firebase-token-xyz')

const mockFetch = vi.fn().mockResolvedValueOnce({
Expand Down Expand Up @@ -824,21 +836,30 @@ describe('useWorkspaceAuthStore', () => {
// New workspace is committed at this point.
expect(currentWorkspace.value?.id).toBe('workspace-other')
expect(workspaceToken.value).toBe('new-workspace-token')
expect(
sessionStorage.getItem(WORKSPACE_STORAGE_KEYS.CURRENT_WORKSPACE)
).toBe(JSON.stringify({ ...newWorkspace, role: 'owner' }))
expect(sessionStorage.getItem(WORKSPACE_STORAGE_KEYS.TOKEN)).toBe(
'new-workspace-token'
)

// Now resolve the stale refresh fetch — it carries an OLD-workspace
// token, and the source has no commit-time staleness check, so it
// clobbers the new workspace state.
// token. It must not clobber the new workspace state or sessionStorage.
resolveRefreshFetch({
ok: true,
json: () =>
Promise.resolve({ ...mockTokenResponse, token: 'stale-token' })
})
await refreshPromise

// Once the source-side guard is added, both of these become true
// (the test stops failing) and `.fails` should be dropped.
expect(currentWorkspace.value?.id).toBe('workspace-other')
expect(workspaceToken.value).toBe('new-workspace-token')
expect(
sessionStorage.getItem(WORKSPACE_STORAGE_KEYS.CURRENT_WORKSPACE)
).toBe(JSON.stringify({ ...newWorkspace, role: 'owner' }))
expect(sessionStorage.getItem(WORKSPACE_STORAGE_KEYS.TOKEN)).toBe(
'new-workspace-token'
)
})
})

Expand All @@ -853,27 +874,40 @@ describe('useWorkspaceAuthStore', () => {
})
)

const setItemSpy = vi
.spyOn(sessionStorage, 'setItem')
.mockImplementation(() => {
const originalSessionStorage = globalThis.sessionStorage
const throwingSessionStorage = {
get length() {
return originalSessionStorage.length
},
key: originalSessionStorage.key.bind(originalSessionStorage),
getItem: originalSessionStorage.getItem.bind(originalSessionStorage),
setItem: vi.fn(() => {
throw new Error('QuotaExceededError')
})
}),
removeItem: originalSessionStorage.removeItem.bind(
originalSessionStorage
),
clear: originalSessionStorage.clear.bind(originalSessionStorage)
} satisfies Storage
vi.stubGlobal('sessionStorage', throwingSessionStorage)
Copy link
Copy Markdown
Contributor

@christian-byrne christian-byrne May 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick (non-blocking): this stubGlobal swap-out is heavier than the original vi.spyOn(sessionStorage, 'setItem'). If happy-dom Storage-prototype spy flakiness is the motivation, would a one-line comment help future readers? Otherwise reverting to spyOn would keep the test smaller.

const consoleWarnSpy = vi
.spyOn(console, 'warn')
.mockImplementation(() => {})

const store = useWorkspaceAuthStore()
const { workspaceToken } = storeToRefs(store)

await store.switchWorkspace('workspace-123')
try {
const store = useWorkspaceAuthStore()
const { workspaceToken } = storeToRefs(store)

expect(workspaceToken.value).toBe('workspace-token-abc')
expect(consoleWarnSpy).toHaveBeenCalledWith(
'Failed to persist workspace context to sessionStorage'
)
await store.switchWorkspace('workspace-123')

setItemSpy.mockRestore()
consoleWarnSpy.mockRestore()
expect(workspaceToken.value).toBe('workspace-token-abc')
expect(consoleWarnSpy).toHaveBeenCalledWith(
'Failed to persist workspace context to sessionStorage'
)
} finally {
vi.stubGlobal('sessionStorage', originalSessionStorage)
consoleWarnSpy.mockRestore()
}
})
})

Expand Down
56 changes: 56 additions & 0 deletions src/platform/workspace/stores/workspaceAuthStore.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ export const useWorkspaceAuthStore = defineStore('workspaceAuth', () => {
// State
const currentWorkspace = shallowRef<WorkspaceWithRole | null>(null)
const workspaceToken = ref<string | null>(null)
const workspaceTokenExpiresAt = ref<number | null>(null)
Copy link
Copy Markdown
Contributor

@christian-byrne christian-byrne May 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick (non-blocking): workspaceTokenExpiresAt is not exported and not read by any computed/watch (only imperatively from scheduleTokenRefreshRetry and hasValidWorkspaceToken). The sibling internal-control variables refreshRequestId and refreshTimerId are plain let. Would let workspaceTokenExpiresAt: number | null = null be more consistent and avoid the .value noise?

const isLoading = ref(false)
const error = ref<Error | null>(null)

Expand Down Expand Up @@ -82,6 +83,25 @@ export const useWorkspaceAuthStore = defineStore('workspaceAuth', () => {
}, delay)
}

function scheduleTokenRefreshRetry(delayMs: number): void {
if (workspaceTokenExpiresAt.value === null) {
return
}

const timeUntilExpiry = workspaceTokenExpiresAt.value - Date.now()
if (timeUntilExpiry <= 0) {
Copy link
Copy Markdown
Contributor

@christian-byrne christian-byrne May 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: when timeUntilExpiry is already <= 0 here, the function returns silently. The token is past expiry but currentWorkspace and workspaceToken remain non-null with no scheduled refresh, so isAuthenticated stays true while the backend will reject the token.

Reachable after 4 fetch attempts plus 1+2+4s backoff (~7s) on a slow network when the token had little headroom. Could clearWorkspaceContext() be called here instead of returning silently? The preserve-token contract only holds while the token is still valid.

return
}

stopRefreshTimer()
refreshTimerId = setTimeout(
() => {
void refreshToken()
},
Math.min(delayMs, timeUntilExpiry)
Copy link
Copy Markdown
Contributor

@christian-byrne christian-byrne May 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (non-blocking): scheduleTokenRefresh refreshes at expiresAt - TOKEN_REFRESH_BUFFER_MS, but this retry path schedules at up to expiresAt itself. A retry firing within the buffer window has near-zero margin. Would using Math.min(delayMs, timeUntilExpiry - TOKEN_REFRESH_BUFFER_MS) keep the two scheduling policies aligned?

)
}

function persistToSession(
workspace: WorkspaceWithRole,
token: string,
Expand Down Expand Up @@ -155,6 +175,7 @@ export const useWorkspaceAuthStore = defineStore('workspaceAuth', () => {

currentWorkspace.value = parseResult.data
workspaceToken.value = token
workspaceTokenExpiresAt.value = expiresAt
error.value = null

scheduleTokenRefresh(expiresAt)
Expand All @@ -176,6 +197,7 @@ export const useWorkspaceAuthStore = defineStore('workspaceAuth', () => {
if (currentWorkspace.value?.id !== workspaceId) {
refreshRequestId++
}
const capturedRequestId = refreshRequestId

isLoading.value = true
error.value = null
Comment on lines 202 to 203
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Apr 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

isLoading can be cleared by a stale request while a newer switch is still pending

Line 306 always sets isLoading to false, so with overlapping switchWorkspace calls, an older stale request can hide loading state for the active request.

💡 Proposed fix (track in-flight switches) 
   // Timer state
   let refreshTimerId: ReturnType<typeof setTimeout> | null = null
+  let inFlightSwitchCount = 0
@@
-    isLoading.value = true
+    inFlightSwitchCount += 1
+    isLoading.value = true
     error.value = null
@@
     } finally {
-      isLoading.value = false
+      inFlightSwitchCount = Math.max(0, inFlightSwitchCount - 1)
+      isLoading.value = inFlightSwitchCount > 0
     }
   }

Also applies to: 305-307

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/platform/workspace/stores/workspaceAuthStore.ts` around lines 202 - 203,
The loading state can be cleared by stale overlapping calls to switchWorkspace;
modify switchWorkspace to generate a unique request id/token at start (set
isLoading.value = true; error.value = null), store it on the store (e.g.,
currentSwitchId), and capture it in the async call; only clear isLoading.value
and update error.value when the captured id matches the store.currentSwitchId.
Apply the same request-id check to the success and error branches where
isLoading is set to false (the code around switchWorkspace and the lines that
currently set isLoading.value = false / error.value = ...).

Expand Down Expand Up @@ -257,12 +279,27 @@ export const useWorkspaceAuthStore = defineStore('workspaceAuth', () => {
role: data.role
}

if (capturedRequestId !== refreshRequestId) {
console.warn(
'Aborting stale workspace switch: workspace context changed before commit'
)
return
Comment on lines +282 to +286
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot Apr 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Don't discard old-workspace refresh when new switch fails

This guard drops any in-flight refresh response as soon as refreshRequestId changes, but refreshRequestId is incremented before a new switchWorkspace request is known to succeed. If refreshToken() for workspace A is in flight, then switchWorkspace('B') fails (e.g. 403), workspace A remains active but the successful refresh response for A is discarded here; refreshToken() then returns without committing a new token/schedule, so A can continue with an aging token until expiration. The stale check should also account for whether currentWorkspace actually changed away from the refreshed workspace before aborting.

Useful? React with 👍 / 👎.

}

currentWorkspace.value = workspaceWithRole
workspaceToken.value = data.token
workspaceTokenExpiresAt.value = expiresAt

persistToSession(workspaceWithRole, data.token, expiresAt)
scheduleTokenRefresh(expiresAt)
} catch (err) {
if (capturedRequestId !== refreshRequestId) {
console.warn(
'Aborting stale workspace switch: workspace context changed before error commit'
Copy link
Copy Markdown
Contributor

@christian-byrne christian-byrne May 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick (non-blocking): would including err in this warn payload help diagnostics? The underlying failure may indicate a real backend problem worth logging, e.g. console.warn('Aborting stale workspace switch: workspace context changed before error commit', err).

)
return
}

error.value = err instanceof Error ? err : new Error(String(err))
throw error.value
} finally {
Expand Down Expand Up @@ -327,6 +364,16 @@ export const useWorkspaceAuthStore = defineStore('workspaceAuth', () => {

// Only clear context if this refresh is still for the current workspace
if (capturedRequestId === refreshRequestId) {
if (isTransientError && hasValidWorkspaceToken()) {
error.value = null
Copy link
Copy Markdown
Contributor

@christian-byrne christian-byrne May 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (non-blocking): error.value is never written by the transient retry path inside the loop, so this null-out has nothing to clear. If the intent is to clear a leftover error from a prior failure, would clearing it at the top of refreshToken() be cleaner?

scheduleTokenRefreshRetry(baseDelayMs * Math.pow(2, maxRetries))
Copy link
Copy Markdown
Contributor

@christian-byrne christian-byrne May 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (non-blocking): the preserve branch reschedules another retry on every failure, with no max-attempts ceiling across scheduled retries and no jitter. Against a backend returning 500 for the full token lifetime this can produce many hundreds of fetches and warn entries before the token finally expires. Would tracking consecutiveScheduledRetries and giving up after N (e.g. 3-5) consecutive failures be reasonable?

console.warn(
'Failed to refresh workspace token after retries; preserving existing valid token:',
err
)
return
}
Comment thread
benceruleanlu marked this conversation as resolved.

console.error('Failed to refresh workspace token after retries:', err)
clearWorkspaceContext()
}
Expand All @@ -347,12 +394,21 @@ export const useWorkspaceAuthStore = defineStore('workspaceAuth', () => {
return workspaceToken.value ?? undefined
}

function hasValidWorkspaceToken(): boolean {
return (
workspaceToken.value !== null &&
workspaceTokenExpiresAt.value !== null &&
workspaceTokenExpiresAt.value > Date.now()
)
}

function clearWorkspaceContext(): void {
// Increment request ID to invalidate any in-flight stale refresh operations
refreshRequestId++
stopRefreshTimer()
currentWorkspace.value = null
workspaceToken.value = null
workspaceTokenExpiresAt.value = null
error.value = null
clearSessionStorage()
}
Expand Down
Loading