improvement(mship): sweep spent OAuth chat attempts out of localStorage

apps/sim/lib/credentials/oauth-chat-attempt.test.ts

@@ -4,13 +4,15 @@
  */
 
 import { beforeEach, describe, expect, it } from 'vitest'
+import type { OAuthChatAttempt, OAuthChatAttemptStatus } from '@/lib/credentials/oauth-chat-attempt'
 import {
   addOAuthChatAttemptToAuthorizeUrl,
   buildOAuthChatCompleteAuthorizeUrl,
   createOAuthChatAttempt,
   getOAuthCredentialBaseline,
   hasOAuthCredentialChanged,
   OAUTH_CHAT_ATTEMPT_EVENT,
+  OAUTH_CHAT_ATTEMPT_MAX_AGE_MS,
   OAUTH_CHAT_ATTEMPT_PARAM,
   OAUTH_CHAT_COMPLETE_PATH,
   OAUTH_CHAT_RETURN_TO_PARAM,
@@ -154,6 +156,126 @@ describe('OAuth chat attempts', () => {
     expect(resolveActiveDesktopOAuthChatAttempt('connected')).toBeNull()
   })
 
+  const SWEEP_INPUT = {
+    workspaceId: 'workspace-1',
+    providerId: 'slack',
+    baseProviderId: 'slack',
+    displayName: 'Slack',
+    controlId: 'message-1:0:0',
+    baselineCredentialIds: [],
+  }
+  const SWEEP_LATEST_KEY = 'sim.oauth-chat-latest.workspace-1.slack.message-1%3A0%3A0.'
+  const RETENTION_MS = 24 * 60 * 60 * 1000
+
+  /** Storage is index-addressed and its keys are not own-enumerable in jsdom. */
+  function storageKeysWithPrefix(prefix: string): string[] {
+    const keys: string[] = []
+    for (let index = 0; index < window.localStorage.length; index++) {
+      const key = window.localStorage.key(index)
+      if (key?.startsWith(prefix)) keys.push(key)
+    }
+    return keys.sort()
+  }
+
+  /** Backdates a stored attempt, then starts an unrelated one to trigger the sweep. */
+  function ageAttemptThenSweep(
+    attempt: OAuthChatAttempt,
+    ageMs: number,
+    status: OAuthChatAttemptStatus
+  ): void {
+    window.localStorage.setItem(
+      `sim.oauth-chat-attempt.${attempt.id}`,
+      JSON.stringify({ ...attempt, requestedAt: attempt.requestedAt - ageMs, status })
+    )
+    createOAuthChatAttempt({ ...SWEEP_INPUT, controlId: 'message-1:9:9' })
+  }
+
+  it('sweeps resolved attempts and their latest-pointers past the retention window', () => {
+    const stale = createOAuthChatAttempt(SWEEP_INPUT)
+    expect(window.localStorage.getItem(SWEEP_LATEST_KEY)).toBe(stale.id)
+
+    ageAttemptThenSweep(stale, RETENTION_MS + 1, 'connected')
+
+    expect(window.localStorage.getItem(`sim.oauth-chat-attempt.${stale.id}`)).toBeNull()
+    expect(window.localStorage.getItem(SWEEP_LATEST_KEY)).toBeNull()
+  })
+
+  it('keeps a resolved attempt a mounted row still reads past the lookup cutoff', () => {
+    const settled = createOAuthChatAttempt(SWEEP_INPUT)
+
+    ageAttemptThenSweep(settled, OAUTH_CHAT_ATTEMPT_MAX_AGE_MS + 1, 'connected')
+
+    // A chip recomputes itself from storage on every attempt event, and on the
+    // reconnect path the record is its only source of connected state — so
+    // sweeping at the lookup cutoff would revert a row that is still on screen.
+    expect(readOAuthChatAttempt(settled.id)?.status).toBe('connected')
+  })
+
+  it('keeps a pending attempt past the read cutoff so a late verdict still lands', () => {
+    const parked = createOAuthChatAttempt(SWEEP_INPUT)
+
+    ageAttemptThenSweep(parked, OAUTH_CHAT_ATTEMPT_MAX_AGE_MS + 1, 'pending')
+
+    // The read cutoff already hides it from the latest-pointer lookup, but the
+    // record itself must survive: a popup parked this long can still return.
+    expect(readLatestOAuthChatAttempt(SWEEP_INPUT)).toBeNull()
+    expect(setOAuthChatAttemptStatus(parked.id, 'connected')?.status).toBe('connected')
+  })
+
+  it('keeps a late verdict alive after it lands on a long-parked attempt', () => {
+    const parked = createOAuthChatAttempt(SWEEP_INPUT)
+    ageAttemptThenSweep(parked, OAUTH_CHAT_ATTEMPT_MAX_AGE_MS + 1, 'pending')
+
+    // The verdict arrives long after the request, so the record is only young
+    // by resolution. Aging it from requestedAt would make it sweepable at once.
+    expect(setOAuthChatAttemptStatus(parked.id, 'connected')?.status).toBe('connected')
+    createOAuthChatAttempt({ ...SWEEP_INPUT, controlId: 'message-1:8:8' })
+
+    expect(readOAuthChatAttempt(parked.id)?.status).toBe('connected')
+  })
+
+  it('sweeps many adjacent stale records in one pass', () => {
+    const template = createOAuthChatAttempt(SWEEP_INPUT)
+    const staleIds = [0, 1, 2, 3, 4, 5].map((slot) => `stale-attempt-${slot}`)
+    // Written directly, so the records land in consecutive storage slots with
+    // no latest-pointer between them — interleaved keys would mask the skip.
+    for (const staleId of staleIds) {
+      window.localStorage.setItem(
+        `sim.oauth-chat-attempt.${staleId}`,
+        JSON.stringify({
+          ...template,
+          id: staleId,
+          status: 'connected',
+          resolvedAt: template.requestedAt - RETENTION_MS - 1,
+        })
+      )
+    }
+
+    createOAuthChatAttempt({ ...SWEEP_INPUT, controlId: 'message-1:9:9' })
+
+    // Removing entries mid-scan would shift every later key down a slot and
+    // skip the next one, leaving about half of these behind.
+    expect(storageKeysWithPrefix('sim.oauth-chat-attempt.stale-attempt-')).toEqual([])
+  })
+
+  it('sweeps a pending attempt once it is past the retention window', () => {
+    const abandoned = createOAuthChatAttempt(SWEEP_INPUT)
+
+    ageAttemptThenSweep(abandoned, RETENTION_MS + 1, 'pending')
+
+    expect(window.localStorage.getItem(`sim.oauth-chat-attempt.${abandoned.id}`)).toBeNull()
+    expect(window.localStorage.getItem(SWEEP_LATEST_KEY)).toBeNull()
+  })
+
+  it('keeps unexpired attempts across a sweep', () => {
+    const live = createOAuthChatAttempt(SWEEP_INPUT)
+
+    createOAuthChatAttempt({ ...SWEEP_INPUT, controlId: 'message-1:9:9' })
+
+    expect(readOAuthChatAttempt(live.id)?.id).toBe(live.id)
+    expect(window.localStorage.getItem(SWEEP_LATEST_KEY)).toBe(live.id)
+  })
+
   it('resolves the exact correlated desktop attempt without consuming a sibling', () => {
     const first = createOAuthChatAttempt({
       workspaceId: 'workspace-1',

apps/sim/lib/credentials/oauth-chat-attempt.ts

@@ -43,6 +43,8 @@ export interface OAuthChatAttempt {
   baselineCredentialUpdatedAt?: string
   requestedAt: number
   status: OAuthChatAttemptStatus
+  /** When {@link status} last left 'pending'. Absent on records written before this was tracked. */
+  resolvedAt?: number
 }
 
 interface CreateOAuthChatAttemptInput {
@@ -136,6 +138,7 @@ function isOAuthChatAttempt(value: unknown): value is OAuthChatAttempt {
       candidate.status === 'connected' ||
       candidate.status === 'failed') &&
     (candidate.credentialId === undefined || typeof candidate.credentialId === 'string') &&
+    (candidate.resolvedAt === undefined || typeof candidate.resolvedAt === 'number') &&
     Array.isArray(candidate.baselineCredentialIds) &&
     candidate.baselineCredentialIds.every((credentialId) => typeof credentialId === 'string') &&
     (candidate.baselineCredentialUpdatedAt === undefined ||
@@ -157,11 +160,85 @@ function writeOAuthChatAttempt(attempt: OAuthChatAttempt): void {
   )
 }
 
+/**
+ * How long a record is kept after its last meaningful activity, whatever its
+ * status. Deliberately far longer than {@link OAUTH_CHAT_ATTEMPT_MAX_AGE_MS},
+ * which only governs what a *lookup* will honour — a record still has readers
+ * after that cutoff, and removing one out from under them is visible:
+ *
+ * - A pending record is a flow whose window may yet return.
+ *   {@link readOAuthChatAttempt} applies no age gate, so a parked popup can
+ *   still publish a verdict through {@link setOAuthChatAttemptStatus}.
+ * - A resolved record still backs a mounted chip. That row recomputes itself
+ *   from storage on every attempt event, and for a reconnect its connected
+ *   state has no other source, so sweeping the record reverts the row.
+ *
+ * Sweeping on the lookup cutoff — as the common OIDC client implementations do
+ * — breaks both. A day is past any live consent flow or session in which a row
+ * is still on screen, while still bounding what abandoned flows leave behind.
+ */
+const OAUTH_CHAT_ATTEMPT_RETENTION_MS = 24 * 60 * 60 * 1000
+
+/**
+ * Drops attempt records that can no longer inform a reader, along with the
+ * latest-pointers aimed at them, so resolved flows stop accumulating for the
+ * life of the browser profile.
+ *
+ * Runs on create rather than on resolve: a verdict is published by status
+ * update and the row re-reads the attempt by id immediately afterwards, so
+ * removing it at that point would race the reader.
+ */
+function pruneExpiredOAuthChatAttempts(now: number): void {
+  const expiredAttemptIds = new Set<string>()
+  const staleKeys: string[] = []
+
+  // Keys are collected before any removal: localStorage is index-addressed, and
+  // removing mid-scan shifts every later entry down one slot, skipping it.
+  for (let index = 0; index < window.localStorage.length; index++) {
+    const key = window.localStorage.key(index)
+    if (!key?.startsWith(OAUTH_CHAT_ATTEMPT_KEY_PREFIX)) continue
+    const attemptId = key.slice(OAUTH_CHAT_ATTEMPT_KEY_PREFIX.length)
+    const attempt = readOAuthChatAttempt(attemptId)
+    // An unparseable or malformed record can never be read back, so it is
+    // collected too rather than left behind forever.
+    if (attempt) {
+      // Age from the last thing that happened to the record. Measuring a
+      // resolved one from `requestedAt` would make a verdict that landed late
+      // sweepable the instant it arrived.
+      const lastActivityAt = attempt.resolvedAt ?? attempt.requestedAt
+      if (now - lastActivityAt <= OAUTH_CHAT_ATTEMPT_RETENTION_MS) continue
+    }
+    expiredAttemptIds.add(attemptId)
+    staleKeys.push(key)
+  }
+
+  if (expiredAttemptIds.size === 0) return
+
+  for (let index = 0; index < window.localStorage.length; index++) {
+    const key = window.localStorage.key(index)
+    if (!key?.startsWith(OAUTH_CHAT_LATEST_KEY_PREFIX)) continue
+    const pointedAttemptId = window.localStorage.getItem(key)
+    if (pointedAttemptId && expiredAttemptIds.has(pointedAttemptId)) staleKeys.push(key)
+  }
+
+  for (const key of staleKeys) {
+    window.localStorage.removeItem(key)
+  }
+}
+
 export function createOAuthChatAttempt(input: CreateOAuthChatAttemptInput): OAuthChatAttempt {
+  const requestedAt = Date.now()
+  if (typeof window !== 'undefined') {
+    // Same reasoning as the write path: a blocked or full store must not throw
+    // into the caller, and losing a sweep only defers it to the next attempt.
+    try {
+      pruneExpiredOAuthChatAttempts(requestedAt)
+    } catch {}
+  }
   const attempt: OAuthChatAttempt = {
     ...input,
     id: generateShortId(24),
-    requestedAt: Date.now(),
+    requestedAt,
     status: 'pending',
   }
   writeOAuthChatAttempt(attempt)
@@ -200,7 +277,11 @@ export function setOAuthChatAttemptStatus(
 ): OAuthChatAttempt | null {
   const attempt = readOAuthChatAttempt(attemptId)
   if (!attempt) return null
-  const updated = { ...attempt, status }
+  const updated: OAuthChatAttempt = {
+    ...attempt,
+    status,
+    resolvedAt: status === 'pending' ? undefined : Date.now(),
+  }
   writeOAuthChatAttempt(updated)
   return updated
 }