Skip to content

Commit 1652f4e

Browse files
committed
fix(mship): retain attempt records for a day regardless of status
The 15m resolved cutoff was still short enough to pull a record out from under a mounted row. A chip recomputes itself from storage on every attempt event, and on the reconnect path connectedFromWorkspaceChange is forced false, so connected collapses to exactly the stored status. Any connect click 15m after a reconnect swept that record -- via the CustomEvent in the same tab or the storage event these removals now fire in others -- and reverted the row from Connected. Non-reconnect rows kept the label but silently lost their lock. OAUTH_CHAT_ATTEMPT_MAX_AGE_MS governs what a lookup honours, not how long a record has readers. Retention is now one flat window measured from last activity, which also drops the status branch.
1 parent d5a22f4 commit 1652f4e

2 files changed

Lines changed: 39 additions & 29 deletions

File tree

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

Lines changed: 18 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -165,7 +165,7 @@ describe('OAuth chat attempts', () => {
165165
baselineCredentialIds: [],
166166
}
167167
const SWEEP_LATEST_KEY = 'sim.oauth-chat-latest.workspace-1.slack.message-1%3A0%3A0.'
168-
const PENDING_GRACE_MS = 24 * 60 * 60 * 1000
168+
const RETENTION_MS = 24 * 60 * 60 * 1000
169169

170170
/** Storage is index-addressed and its keys are not own-enumerable in jsdom. */
171171
function storageKeysWithPrefix(prefix: string): string[] {
@@ -190,16 +190,27 @@ describe('OAuth chat attempts', () => {
190190
createOAuthChatAttempt({ ...SWEEP_INPUT, controlId: 'message-1:9:9' })
191191
}
192192

193-
it('sweeps resolved expired attempts and their latest-pointers when a new one starts', () => {
193+
it('sweeps resolved attempts and their latest-pointers past the retention window', () => {
194194
const stale = createOAuthChatAttempt(SWEEP_INPUT)
195195
expect(window.localStorage.getItem(SWEEP_LATEST_KEY)).toBe(stale.id)
196196

197-
ageAttemptThenSweep(stale, OAUTH_CHAT_ATTEMPT_MAX_AGE_MS + 1, 'connected')
197+
ageAttemptThenSweep(stale, RETENTION_MS + 1, 'connected')
198198

199199
expect(window.localStorage.getItem(`sim.oauth-chat-attempt.${stale.id}`)).toBeNull()
200200
expect(window.localStorage.getItem(SWEEP_LATEST_KEY)).toBeNull()
201201
})
202202

203+
it('keeps a resolved attempt a mounted row still reads past the lookup cutoff', () => {
204+
const settled = createOAuthChatAttempt(SWEEP_INPUT)
205+
206+
ageAttemptThenSweep(settled, OAUTH_CHAT_ATTEMPT_MAX_AGE_MS + 1, 'connected')
207+
208+
// A chip recomputes itself from storage on every attempt event, and on the
209+
// reconnect path the record is its only source of connected state — so
210+
// sweeping at the lookup cutoff would revert a row that is still on screen.
211+
expect(readOAuthChatAttempt(settled.id)?.status).toBe('connected')
212+
})
213+
203214
it('keeps a pending attempt past the read cutoff so a late verdict still lands', () => {
204215
const parked = createOAuthChatAttempt(SWEEP_INPUT)
205216

@@ -211,7 +222,7 @@ describe('OAuth chat attempts', () => {
211222
expect(setOAuthChatAttemptStatus(parked.id, 'connected')?.status).toBe('connected')
212223
})
213224

214-
it('keeps a late verdict alive after it lands on a grace-preserved attempt', () => {
225+
it('keeps a late verdict alive after it lands on a long-parked attempt', () => {
215226
const parked = createOAuthChatAttempt(SWEEP_INPUT)
216227
ageAttemptThenSweep(parked, OAUTH_CHAT_ATTEMPT_MAX_AGE_MS + 1, 'pending')
217228

@@ -235,7 +246,7 @@ describe('OAuth chat attempts', () => {
235246
...template,
236247
id: staleId,
237248
status: 'connected',
238-
resolvedAt: template.requestedAt - OAUTH_CHAT_ATTEMPT_MAX_AGE_MS - 1,
249+
resolvedAt: template.requestedAt - RETENTION_MS - 1,
239250
})
240251
)
241252
}
@@ -247,10 +258,10 @@ describe('OAuth chat attempts', () => {
247258
expect(storageKeysWithPrefix('sim.oauth-chat-attempt.stale-attempt-')).toEqual([])
248259
})
249260

250-
it('sweeps a pending attempt once it is past the abandoned grace period', () => {
261+
it('sweeps a pending attempt once it is past the retention window', () => {
251262
const abandoned = createOAuthChatAttempt(SWEEP_INPUT)
252263

253-
ageAttemptThenSweep(abandoned, PENDING_GRACE_MS + 1, 'pending')
264+
ageAttemptThenSweep(abandoned, RETENTION_MS + 1, 'pending')
254265

255266
expect(window.localStorage.getItem(`sim.oauth-chat-attempt.${abandoned.id}`)).toBeNull()
256267
expect(window.localStorage.getItem(SWEEP_LATEST_KEY)).toBeNull()

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

Lines changed: 21 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -161,15 +161,23 @@ function writeOAuthChatAttempt(attempt: OAuthChatAttempt): void {
161161
}
162162

163163
/**
164-
* Grace period before a still-pending attempt is collected. A pending record is
165-
* a flow whose window may yet return: {@link readOAuthChatAttempt} applies no
166-
* age gate, so a popup parked past the read cutoff can still publish a verdict
167-
* through {@link setOAuthChatAttemptStatus}. Sweeping on age alone — as the
168-
* common OIDC client implementations do — would drop that record and strand the
169-
* row on 'pending' with no event to correct it. A day is far past any live
170-
* consent flow while still bounding what an abandoned one can leave behind.
164+
* How long a record is kept after its last meaningful activity, whatever its
165+
* status. Deliberately far longer than {@link OAUTH_CHAT_ATTEMPT_MAX_AGE_MS},
166+
* which only governs what a *lookup* will honour — a record still has readers
167+
* after that cutoff, and removing one out from under them is visible:
168+
*
169+
* - A pending record is a flow whose window may yet return.
170+
* {@link readOAuthChatAttempt} applies no age gate, so a parked popup can
171+
* still publish a verdict through {@link setOAuthChatAttemptStatus}.
172+
* - A resolved record still backs a mounted chip. That row recomputes itself
173+
* from storage on every attempt event, and for a reconnect its connected
174+
* state has no other source, so sweeping the record reverts the row.
175+
*
176+
* Sweeping on the lookup cutoff — as the common OIDC client implementations do
177+
* — breaks both. A day is past any live consent flow or session in which a row
178+
* is still on screen, while still bounding what abandoned flows leave behind.
171179
*/
172-
const OAUTH_CHAT_ATTEMPT_PENDING_GRACE_MS = 24 * 60 * 60 * 1000
180+
const OAUTH_CHAT_ATTEMPT_RETENTION_MS = 24 * 60 * 60 * 1000
173181

174182
/**
175183
* Drops attempt records that can no longer inform a reader, along with the
@@ -194,20 +202,11 @@ function pruneExpiredOAuthChatAttempts(now: number): void {
194202
// An unparseable or malformed record can never be read back, so it is
195203
// collected too rather than left behind forever.
196204
if (attempt) {
197-
// A resolved record ages from when it was resolved, not from when it was
198-
// requested. Aging it from `requestedAt` would make the verdict on a
199-
// grace-preserved pending record sweepable the instant it landed, and the
200-
// next create would erase it — every chip re-reads its row on the event
201-
// that create dispatches, so the row would drop straight back to unset.
202-
const age =
203-
attempt.status === 'pending'
204-
? now - attempt.requestedAt
205-
: now - (attempt.resolvedAt ?? attempt.requestedAt)
206-
const maxAge =
207-
attempt.status === 'pending'
208-
? OAUTH_CHAT_ATTEMPT_PENDING_GRACE_MS
209-
: OAUTH_CHAT_ATTEMPT_MAX_AGE_MS
210-
if (age <= maxAge) continue
205+
// Age from the last thing that happened to the record. Measuring a
206+
// resolved one from `requestedAt` would make a verdict that landed late
207+
// sweepable the instant it arrived.
208+
const lastActivityAt = attempt.resolvedAt ?? attempt.requestedAt
209+
if (now - lastActivityAt <= OAUTH_CHAT_ATTEMPT_RETENTION_MS) continue
211210
}
212211
expiredAttemptIds.add(attemptId)
213212
staleKeys.push(key)

0 commit comments

Comments
 (0)