Skip to content

Commit b552339

Browse files
committed
fix(account-deletion): retry a saved sweep position once before restarting its family
A single failed listing from a saved page token restarted the family from the prefix start, so a transient provider error at a deep position threw the position away. The first failure now records tokenFailures: 1 and rethrows, so the outbox retries from the same position under its normal attempt budget; only a second failure from that token restarts the family. Any successful listing clears the count.
1 parent 75a029f commit b552339

2 files changed

Lines changed: 72 additions & 22 deletions

File tree

‎apps/sim/lib/users/storage-sweep.test.ts‎

Lines changed: 42 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -331,28 +331,50 @@ describe('deleted workspace storage sweep', () => {
331331
expect(checkpointPayload).toHaveBeenCalledWith({ family: 1, pageToken: null })
332332
})
333333

334-
it('walks the family from the start when its saved position no longer lists', async () => {
335-
vi.mocked(listStorageKeyPage).mockRejectedValueOnce(
336-
new Error('The continuation token is invalid')
337-
)
334+
it('retries a saved position that failed to list once instead of walking from the start', async () => {
335+
let payload: Record<string, unknown> = {
336+
version: 1,
337+
workspaceId: 'wsdoomed1',
338+
pageToken: 'workspace/wsdoomed1/a.txt',
339+
}
340+
const checkpointPayload = vi.fn(async (patch: Record<string, unknown>) => {
341+
payload = { ...payload, ...patch }
342+
})
343+
vi.mocked(listStorageKeyPage).mockRejectedValueOnce(new Error('Service unavailable'))
338344

339-
await expect(
340-
sweep({ version: 1, workspaceId: 'wsdoomed1', pageToken: 'expired-token' }, outboxContext())
341-
).resolves.toBe(undefined)
345+
await expect(sweep(payload, outboxContext({ checkpointPayload }))).rejects.toThrow(
346+
'Service unavailable'
347+
)
348+
for (const key of DOOMED_KEYS) expect(await exists(key)).toBe(true)
349+
expect(payload).toMatchObject({ pageToken: 'workspace/wsdoomed1/a.txt', tokenFailures: 1 })
342350

343-
for (const key of DOOMED_KEYS) expect(await exists(key)).toBe(false)
351+
await expect(sweep(payload, outboxContext({ checkpointPayload }))).resolves.toBe(undefined)
352+
expect(await exists('workspace/wsdoomed1/a.txt')).toBe(true)
353+
expect(await exists('workspace/wsdoomed1/nested/b.txt')).toBe(false)
354+
expect(payload).toMatchObject({ tokenFailures: 0 })
344355
})
345356

346-
it('walks the family from the start when its saved position is not a key under the prefix', async () => {
347-
await expect(
348-
sweep(
349-
{ version: 1, workspaceId: 'wsdoomed1', pageToken: 'zz-other-provider' },
350-
outboxContext()
351-
)
352-
).resolves.toBe(undefined)
357+
it.each([
358+
{ name: 'no longer lists', pageToken: 'expired-token', rejects: true },
359+
{ name: 'is not a key under the prefix', pageToken: 'zz-other-provider', rejects: false },
360+
])(
361+
'walks the family from the start once its saved position $name on a second attempt',
362+
async ({ pageToken, rejects }) => {
363+
let payload: Record<string, unknown> = { version: 1, workspaceId: 'wsdoomed1', pageToken }
364+
const checkpointPayload = vi.fn(async (patch: Record<string, unknown>) => {
365+
payload = { ...payload, ...patch }
366+
})
367+
const invalid = new Error('The continuation token is invalid')
368+
if (rejects)
369+
vi.mocked(listStorageKeyPage).mockRejectedValueOnce(invalid).mockRejectedValueOnce(invalid)
353370

354-
for (const key of DOOMED_KEYS) expect(await exists(key)).toBe(false)
355-
})
371+
await expect(sweep(payload, outboxContext({ checkpointPayload }))).rejects.toThrow()
372+
for (const key of DOOMED_KEYS) expect(await exists(key)).toBe(true)
373+
374+
await expect(sweep(payload, outboxContext({ checkpointPayload }))).resolves.toBe(undefined)
375+
for (const key of DOOMED_KEYS) expect(await exists(key)).toBe(false)
376+
}
377+
)
356378

357379
it('spends an attempt when a restart from a rejected position only walks kept objects', async () => {
358380
const keptKeys = await putKept(500)
@@ -362,7 +384,7 @@ describe('deleted workspace storage sweep', () => {
362384
const checkpointPayload = vi.fn(async () => {})
363385

364386
const outcome = await sweep(
365-
{ version: 1, workspaceId: 'wsdoomed1', pageToken: 'expired-token' },
387+
{ version: 1, workspaceId: 'wsdoomed1', pageToken: 'expired-token', tokenFailures: 1 },
366388
deadlineAfter(2, { checkpointPayload })
367389
)
368390

@@ -404,6 +426,8 @@ describe('deleted workspace storage sweep', () => {
404426
{ version: 1, workspaceId: 'wsdoomed1', pageToken: 'x'.repeat(4097) },
405427
{ version: 1, workspaceId: 'wsdoomed1', resumes: -1 },
406428
{ version: 1, workspaceId: 'wsdoomed1', resumes: '1' },
429+
{ version: 1, workspaceId: 'wsdoomed1', tokenFailures: 2 },
430+
{ version: 1, workspaceId: 'wsdoomed1', tokenFailures: '1' },
407431
])('rejects an unbounded or malformed payload %o', async (payload) => {
408432
await expect(sweep(payload, outboxContext())).rejects.toThrow()
409433
for (const key of DOOMED_KEYS) expect(await exists(key)).toBe(true)

‎apps/sim/lib/users/storage-sweep.ts‎

Lines changed: 30 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -110,6 +110,11 @@ interface WorkspaceSweepPayload {
110110
pageToken?: string
111111
/** Runs continued on a moved listing position alone; see {@link MAX_POSITION_RESUMES}. */
112112
resumes: number
113+
/**
114+
* Set once listing from `pageToken` failed. A second failure from the same token restarts the
115+
* family; a single one is retried in place, since providers do not reliably mark a dead cursor.
116+
*/
117+
tokenFailed: boolean
113118
}
114119

115120
interface ObjectCleanupPayload {
@@ -147,7 +152,16 @@ function parseWorkspaceSweep(value: unknown): WorkspaceSweepPayload {
147152
const resumes = value.resumes ?? 0
148153
if (typeof resumes !== 'number' || !Number.isSafeInteger(resumes) || resumes < 0)
149154
throw new Error('Invalid workspace storage sweep resume count')
150-
return { workspaceId: value.workspaceId, family, resumes, ...(pageToken ? { pageToken } : {}) }
155+
const tokenFailures = value.tokenFailures ?? 0
156+
if (tokenFailures !== 0 && tokenFailures !== 1)
157+
throw new Error('Invalid workspace storage sweep page token failure count')
158+
return {
159+
workspaceId: value.workspaceId,
160+
family,
161+
resumes,
162+
tokenFailed: tokenFailures === 1,
163+
...(pageToken ? { pageToken } : {}),
164+
}
151165
}
152166

153167
function isSafeKey(key: string): boolean {
@@ -243,19 +257,22 @@ interface PrefixSweepResult {
243257
* does not keep, following the listing's page token so kept objects never hold back later ones. A
244258
* page that neither advances the token nor removes anything is `stalled` rather than re-listed
245259
* forever. A saved token is provider-specific and may no longer list (an expired or rejected
246-
* cursor), so a failed first listing from one retries once from the start of the prefix.
260+
* cursor). The first failed listing from one is recorded and rethrown, so the outbox retries from
261+
* the same position; when that token has failed before, the walk restarts from the prefix start.
247262
*/
248263
async function sweepWorkspacePrefix(
249264
prefix: string,
250265
storageContext: StorageContext,
251266
context: OutboxEventContext,
252267
isReferenced: (keys: readonly string[]) => Promise<Set<string>>,
253268
workspaceExists: () => Promise<boolean>,
254-
startToken: string | undefined
269+
startToken: string | undefined,
270+
startTokenFailed: boolean
255271
): Promise<PrefixSweepResult> {
256272
let deleted = 0
257273
let pageToken = startToken
258274
let restarted = false
275+
let failureRecorded = startTokenFailed
259276
for (;;) {
260277
if (outboxDeadlineNear(context)) return { deleted, status: 'partial', pageToken, restarted }
261278
let page: StorageKeyPage
@@ -264,6 +281,10 @@ async function sweepWorkspacePrefix(
264281
} catch (error) {
265282
if (pageToken === undefined || pageToken !== startToken || restarted) throw error
266283
context.signal.throwIfAborted()
284+
if (!failureRecorded) {
285+
await context.checkpointPayload({ tokenFailures: 1 })
286+
throw error
287+
}
267288
logger.warn('Workspace storage sweep could not resume its saved listing position', {
268289
prefix,
269290
error: describeError(error),
@@ -272,6 +293,10 @@ async function sweepWorkspacePrefix(
272293
pageToken = undefined
273294
continue
274295
}
296+
if (failureRecorded) {
297+
await context.checkpointPayload({ tokenFailures: 0 })
298+
failureRecorded = false
299+
}
275300
if (page.keys.some((key) => !key.startsWith(prefix)))
276301
throw new Error('Storage provider returned a key outside the workspace prefix')
277302
const owners = page.keys.flatMap((key) => sidecarOwner(key) ?? [])
@@ -336,7 +361,8 @@ const sweepWorkspaceStorage: OutboxHandler<unknown> = async (raw, context) => {
336361
context,
337362
isReferenced,
338363
workspaceExists,
339-
startToken
364+
startToken,
365+
family === payload.family && payload.tokenFailed
340366
)
341367
if (result.status === 'refused') return
342368
if (result.deleted > 0) progressed = true

0 commit comments

Comments
 (0)