Skip to content

Commit d5a22f4

Browse files
committed
fix(mship): age a resolved attempt from when it resolved, not when it was requested
Review round 1 findings. A pending attempt held by the 24h grace is by definition older than the 15m cutoff, so the moment a late verdict landed on it the record became instantly sweepable and the next create erased it. Every chip re-reads its row on the event create dispatches, so a connected chip dropped straight back to unset -- the grace period was defeating its own purpose. Resolved records now age from resolvedAt, giving the UI the full window to observe a verdict however late it arrives. Legacy records without the field fall back to requestedAt. Also makes the multi-record sweep test a real guard: its records are written directly so they occupy consecutive storage slots. The earlier version interleaved latest-pointers between them, which masked the index shift -- a remove-during-scan regression passed it.
1 parent afd7ffc commit d5a22f4

2 files changed

Lines changed: 63 additions & 2 deletions

File tree

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

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -167,6 +167,16 @@ describe('OAuth chat attempts', () => {
167167
const SWEEP_LATEST_KEY = 'sim.oauth-chat-latest.workspace-1.slack.message-1%3A0%3A0.'
168168
const PENDING_GRACE_MS = 24 * 60 * 60 * 1000
169169

170+
/** Storage is index-addressed and its keys are not own-enumerable in jsdom. */
171+
function storageKeysWithPrefix(prefix: string): string[] {
172+
const keys: string[] = []
173+
for (let index = 0; index < window.localStorage.length; index++) {
174+
const key = window.localStorage.key(index)
175+
if (key?.startsWith(prefix)) keys.push(key)
176+
}
177+
return keys.sort()
178+
}
179+
170180
/** Backdates a stored attempt, then starts an unrelated one to trigger the sweep. */
171181
function ageAttemptThenSweep(
172182
attempt: OAuthChatAttempt,
@@ -201,6 +211,42 @@ describe('OAuth chat attempts', () => {
201211
expect(setOAuthChatAttemptStatus(parked.id, 'connected')?.status).toBe('connected')
202212
})
203213

214+
it('keeps a late verdict alive after it lands on a grace-preserved attempt', () => {
215+
const parked = createOAuthChatAttempt(SWEEP_INPUT)
216+
ageAttemptThenSweep(parked, OAUTH_CHAT_ATTEMPT_MAX_AGE_MS + 1, 'pending')
217+
218+
// The verdict arrives long after the request, so the record is only young
219+
// by resolution. Aging it from requestedAt would make it sweepable at once.
220+
expect(setOAuthChatAttemptStatus(parked.id, 'connected')?.status).toBe('connected')
221+
createOAuthChatAttempt({ ...SWEEP_INPUT, controlId: 'message-1:8:8' })
222+
223+
expect(readOAuthChatAttempt(parked.id)?.status).toBe('connected')
224+
})
225+
226+
it('sweeps many adjacent stale records in one pass', () => {
227+
const template = createOAuthChatAttempt(SWEEP_INPUT)
228+
const staleIds = [0, 1, 2, 3, 4, 5].map((slot) => `stale-attempt-${slot}`)
229+
// Written directly, so the records land in consecutive storage slots with
230+
// no latest-pointer between them — interleaved keys would mask the skip.
231+
for (const staleId of staleIds) {
232+
window.localStorage.setItem(
233+
`sim.oauth-chat-attempt.${staleId}`,
234+
JSON.stringify({
235+
...template,
236+
id: staleId,
237+
status: 'connected',
238+
resolvedAt: template.requestedAt - OAUTH_CHAT_ATTEMPT_MAX_AGE_MS - 1,
239+
})
240+
)
241+
}
242+
243+
createOAuthChatAttempt({ ...SWEEP_INPUT, controlId: 'message-1:9:9' })
244+
245+
// Removing entries mid-scan would shift every later key down a slot and
246+
// skip the next one, leaving about half of these behind.
247+
expect(storageKeysWithPrefix('sim.oauth-chat-attempt.stale-attempt-')).toEqual([])
248+
})
249+
204250
it('sweeps a pending attempt once it is past the abandoned grace period', () => {
205251
const abandoned = createOAuthChatAttempt(SWEEP_INPUT)
206252

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

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,8 @@ export interface OAuthChatAttempt {
4343
baselineCredentialUpdatedAt?: string
4444
requestedAt: number
4545
status: OAuthChatAttemptStatus
46+
/** When {@link status} last left 'pending'. Absent on records written before this was tracked. */
47+
resolvedAt?: number
4648
}
4749

4850
interface CreateOAuthChatAttemptInput {
@@ -136,6 +138,7 @@ function isOAuthChatAttempt(value: unknown): value is OAuthChatAttempt {
136138
candidate.status === 'connected' ||
137139
candidate.status === 'failed') &&
138140
(candidate.credentialId === undefined || typeof candidate.credentialId === 'string') &&
141+
(candidate.resolvedAt === undefined || typeof candidate.resolvedAt === 'number') &&
139142
Array.isArray(candidate.baselineCredentialIds) &&
140143
candidate.baselineCredentialIds.every((credentialId) => typeof credentialId === 'string') &&
141144
(candidate.baselineCredentialUpdatedAt === undefined ||
@@ -191,7 +194,15 @@ function pruneExpiredOAuthChatAttempts(now: number): void {
191194
// An unparseable or malformed record can never be read back, so it is
192195
// collected too rather than left behind forever.
193196
if (attempt) {
194-
const age = now - attempt.requestedAt
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)
195206
const maxAge =
196207
attempt.status === 'pending'
197208
? OAUTH_CHAT_ATTEMPT_PENDING_GRACE_MS
@@ -267,7 +278,11 @@ export function setOAuthChatAttemptStatus(
267278
): OAuthChatAttempt | null {
268279
const attempt = readOAuthChatAttempt(attemptId)
269280
if (!attempt) return null
270-
const updated = { ...attempt, status }
281+
const updated: OAuthChatAttempt = {
282+
...attempt,
283+
status,
284+
resolvedAt: status === 'pending' ? undefined : Date.now(),
285+
}
271286
writeOAuthChatAttempt(updated)
272287
return updated
273288
}

0 commit comments

Comments
 (0)