Skip to content

Commit 75a029f

Browse files
committed
fix(account-deletion): bound position-only sweep resumes and reject foreign local page tokens
Provider cursors are opaque, so a moved listing position alone no longer counts as progress past 1000 resumes per event; past that a run that only moved its position spends an attempt, keeping the sweep bounded even by a cursor that revisits positions. A local page token that is not a key under the prefix (one saved under another storage provider) now fails to list, so the sweep restarts the family instead of skipping every key before it.
1 parent dc0430b commit 75a029f

3 files changed

Lines changed: 76 additions & 24 deletions

File tree

‎apps/sim/lib/uploads/core/prefix-sweep.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -173,6 +173,8 @@ export async function listStorageKeyPage(
173173
: undefined
174174
)
175175
}
176+
if (pageToken && !pageToken.startsWith(prefix))
177+
throw new Error('Local storage page token is not a key under the prefix')
176178
const keys = await localKeys(prefix, signal, pageToken)
177179
return keyPage(keys, keys.length < STORAGE_KEY_PAGE_SIZE ? undefined : keys[keys.length - 1])
178180
}

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

Lines changed: 55 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,22 @@ async function put(key: string, body = 'x'): Promise<void> {
8989
await writeFile(join(root, key), body)
9090
}
9191

92+
/** Writes `count` objects a surviving document names, answering `lookups` reference checks. */
93+
async function putKept(count: number, lookups = 1): Promise<string[]> {
94+
const width = String(count - 1).length
95+
const keys = Array.from(
96+
{ length: count },
97+
(_, index) => `workspace/wsdoomed1/a-kept/${String(index).padStart(width, '0')}.bin`
98+
)
99+
for (const key of keys) await put(key)
100+
for (let lookup = 0; lookup < lookups; lookup++)
101+
queueTableRows(
102+
schemaMock.document,
103+
keys.map((key) => ({ key }))
104+
)
105+
return keys
106+
}
107+
92108
async function exists(key: string): Promise<boolean> {
93109
try {
94110
await stat(join(root, key))
@@ -287,16 +303,8 @@ describe('deleted workspace storage sweep', () => {
287303
})
288304

289305
it('reaches objects behind more kept pages than one run can walk, spending no attempt', async () => {
290-
const keptKeys = Array.from(
291-
{ length: 1000 },
292-
(_, index) => `workspace/wsdoomed1/a-kept/${String(index).padStart(4, '0')}.bin`
293-
)
294-
for (const key of [...keptKeys, 'workspace/wsdoomed1/z-unkept/0.bin']) await put(key)
295-
for (let call = 0; call < 30; call++)
296-
queueTableRows(
297-
schemaMock.document,
298-
keptKeys.map((key) => ({ key }))
299-
)
306+
const keptKeys = await putKept(1000, 30)
307+
await put('workspace/wsdoomed1/z-unkept/0.bin')
300308

301309
const outcomes = await sweepUntilSettled({ version: 1, workspaceId: 'wsdoomed1' }, 2)
302310

@@ -335,16 +343,19 @@ describe('deleted workspace storage sweep', () => {
335343
for (const key of DOOMED_KEYS) expect(await exists(key)).toBe(false)
336344
})
337345

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)
353+
354+
for (const key of DOOMED_KEYS) expect(await exists(key)).toBe(false)
355+
})
356+
338357
it('spends an attempt when a restart from a rejected position only walks kept objects', async () => {
339-
const keptKeys = Array.from(
340-
{ length: 500 },
341-
(_, index) => `workspace/wsdoomed1/a-kept/${String(index).padStart(3, '0')}.bin`
342-
)
343-
for (const key of keptKeys) await put(key)
344-
queueTableRows(
345-
schemaMock.document,
346-
keptKeys.map((key) => ({ key }))
347-
)
358+
const keptKeys = await putKept(500)
348359
vi.mocked(listStorageKeyPage).mockRejectedValueOnce(
349360
new Error('The continuation token is invalid')
350361
)
@@ -357,7 +368,29 @@ describe('deleted workspace storage sweep', () => {
357368

358369
expect(outcome).toMatchObject({ outcome: 'deferred' })
359370
expect(outcome).not.toHaveProperty('consumeAttempt')
360-
expect(checkpointPayload).toHaveBeenCalledWith({ family: 0, pageToken: keptKeys.at(-1) })
371+
expect(checkpointPayload).toHaveBeenCalledWith({
372+
family: 0,
373+
pageToken: keptKeys.at(-1),
374+
resumes: 0,
375+
})
376+
})
377+
378+
it('spends an attempt on a moved position once its resume budget is gone', async () => {
379+
const keptKeys = await putKept(1000)
380+
const checkpointPayload = vi.fn(async () => {})
381+
382+
const outcome = await sweep(
383+
{ version: 1, workspaceId: 'wsdoomed1', resumes: 1000 },
384+
deadlineAfter(1, { checkpointPayload })
385+
)
386+
387+
expect(outcome).toMatchObject({ outcome: 'deferred' })
388+
expect(outcome).not.toHaveProperty('consumeAttempt')
389+
expect(checkpointPayload).toHaveBeenCalledWith({
390+
family: 0,
391+
pageToken: keptKeys[499],
392+
resumes: 1001,
393+
})
361394
})
362395

363396
it.each([
@@ -369,6 +402,8 @@ describe('deleted workspace storage sweep', () => {
369402
{ version: 1, workspaceId: 'wsdoomed1', pageToken: 42 },
370403
{ version: 1, workspaceId: 'wsdoomed1', pageToken: '' },
371404
{ version: 1, workspaceId: 'wsdoomed1', pageToken: 'x'.repeat(4097) },
405+
{ version: 1, workspaceId: 'wsdoomed1', resumes: -1 },
406+
{ version: 1, workspaceId: 'wsdoomed1', resumes: '1' },
372407
])('rejects an unbounded or malformed payload %o', async (payload) => {
373408
await expect(sweep(payload, outboxContext())).rejects.toThrow()
374409
for (const key of DOOMED_KEYS) expect(await exists(key)).toBe(true)

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

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,13 @@ const ROW_PAGE_SIZE = 1000
6161
const OBJECTS_PER_EVENT = 100
6262
const MAX_PAGE_TOKEN_LENGTH = 4096
6363

64+
/**
65+
* Runs one workspace sweep may continue on a moved listing position alone. Provider cursors are
66+
* opaque, so nothing here proves one never revisits a position; past this, a run that only moved
67+
* its position spends an attempt, keeping the sweep bounded even by a cursor that cycles.
68+
*/
69+
const MAX_POSITION_RESUMES = 1000
70+
6471
/**
6572
* Ceiling on exact keys one deletion records for durable cleanup. Past it the rest are logged
6673
* and left; the workspace prefix sweeps are unaffected, since they list storage instead.
@@ -101,6 +108,8 @@ interface WorkspaceSweepPayload {
101108
family: number
102109
/** Listing position inside `family` a stopped run saved; absent from events queued before it. */
103110
pageToken?: string
111+
/** Runs continued on a moved listing position alone; see {@link MAX_POSITION_RESUMES}. */
112+
resumes: number
104113
}
105114

106115
interface ObjectCleanupPayload {
@@ -135,7 +144,10 @@ function parseWorkspaceSweep(value: unknown): WorkspaceSweepPayload {
135144
pageToken.length > MAX_PAGE_TOKEN_LENGTH)
136145
)
137146
throw new Error('Invalid workspace storage sweep page token')
138-
return { workspaceId: value.workspaceId, family, ...(pageToken ? { pageToken } : {}) }
147+
const resumes = value.resumes ?? 0
148+
if (typeof resumes !== 'number' || !Number.isSafeInteger(resumes) || resumes < 0)
149+
throw new Error('Invalid workspace storage sweep resume count')
150+
return { workspaceId: value.workspaceId, family, resumes, ...(pageToken ? { pageToken } : {}) }
139151
}
140152

141153
function isSafeKey(key: string): boolean {
@@ -294,7 +306,8 @@ async function sweepWorkspacePrefix(
294306
* recovery), so the sweep stops without touching storage; it looks again before every page it
295307
* deletes from. A run stopped by its deadline saves its listing position, so a family whose early
296308
* pages are all kept still reaches the objects behind them; moving that position counts as
297-
* progress, except after a restart from the start, so a cursor that never lists stays bounded.
309+
* progress, except after a restart from the start or past {@link MAX_POSITION_RESUMES}, so a
310+
* cursor that never lists or never ends stays bounded by the attempt budget.
298311
*/
299312
const sweepWorkspaceStorage: OutboxHandler<unknown> = async (raw, context) => {
300313
const payload = parseWorkspaceSweep(raw)
@@ -331,8 +344,10 @@ const sweepWorkspaceStorage: OutboxHandler<unknown> = async (raw, context) => {
331344
return deferOutboxHandler('Workspace storage sweep made no progress on a full listing page')
332345
if (result.status === 'partial') {
333346
if (result.pageToken !== startToken) {
334-
await context.checkpointPayload({ family, pageToken: result.pageToken ?? null })
335-
if (!result.restarted) progressed = true
347+
const movedOnly = !progressed && !result.restarted
348+
const resumes = payload.resumes + (movedOnly ? 1 : 0)
349+
await context.checkpointPayload({ family, pageToken: result.pageToken ?? null, resumes })
350+
if (movedOnly && resumes <= MAX_POSITION_RESUMES) progressed = true
336351
}
337352
return progressed
338353
? continueOutboxHandler('Continuing workspace storage sweep')

0 commit comments

Comments
 (0)