Skip to content

Commit 9dee1b4

Browse files
committed
fix(account-deletion): read durable storage keys before the deletion locks
- collectAccountStorageObjects runs the exact-key keyset reads before the transaction, beside collectAccountStorageKeys; inside it, enqueueAccountStorageCleanupInTx now only inserts the prefix-sweep events and the pre-collected exact-key events, so teammates' writes in shared workspaces never wait on those reads - Project prefix cleanup completes on a short listing page again, as before - the file-serve route keeps its original public-prefix check
1 parent f8a8d64 commit 9dee1b4

7 files changed

Lines changed: 168 additions & 125 deletions

File tree

‎apps/sim/app/api/files/serve/[...path]/route.ts‎

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -25,11 +25,7 @@ import { FILE_CACHE_CONTROL, workspaceFileCacheControl } from '@/lib/uploads/ser
2525
import { resolveServableImageBytes } from '@/lib/uploads/server/image-derivative'
2626
import { resolveStoredFileContext } from '@/lib/uploads/server/metadata'
2727
import { MAX_BUFFERED_TRANSFER_BYTES } from '@/lib/uploads/shared/types'
28-
import {
29-
inferContextFromKey,
30-
isPublicStorageKey,
31-
resolveEffectiveMimeType,
32-
} from '@/lib/uploads/utils/file'
28+
import { inferContextFromKey, resolveEffectiveMimeType } from '@/lib/uploads/utils/file'
3329
import { internalWorkspaceFileServeAuth } from '@/lib/workspace-files/api'
3430
import { fetchAuthorizedServableWorkspaceFileBuffer } from '@/lib/workspace-files/application/fetch-servable-workspace-file-buffer'
3531
import { finishFileDelivery } from '@/lib/workspace-files/application/finish-file-delivery'
@@ -303,7 +299,13 @@ export const GET = withRouteHandler(
303299
})
304300
}
305301

306-
if (isPublicStorageKey(cloudKey)) {
302+
const isPublicByKeyPrefix =
303+
cloudKey.startsWith('profile-pictures/') ||
304+
cloudKey.startsWith('og-images/') ||
305+
cloudKey.startsWith('workspace-logos/') ||
306+
cloudKey.startsWith('organization-logos/')
307+
308+
if (isPublicByKeyPrefix) {
307309
const context = inferContextFromKey(cloudKey)
308310
logger.info(`Serving public ${context}:`, { cloudKey })
309311
if (isUsingCloudStorage() || isCloudPath) {
@@ -387,6 +389,7 @@ export const GET = withRouteHandler(
387389
request.method === 'HEAD'
388390
)
389391
} catch (error) {
392+
if (request.signal.aborted) throw error
390393
if (error instanceof InternalUnauthenticatedError) {
391394
logger.warn('Unauthorized file access attempt', { error: error.message })
392395
return NextResponse.json(
@@ -597,6 +600,7 @@ async function handleLocalFile(
597600
options.ifNoneMatch
598601
)
599602
} catch (error) {
603+
if (signal?.aborted) throw error
600604
logServeFailure('Error reading local file:', error)
601605
throw error
602606
}
@@ -687,6 +691,7 @@ async function handleCloudProxy(
687691
options.ifNoneMatch
688692
)
689693
} catch (error) {
694+
if (signal?.aborted) throw error
690695
logServeFailure('Error downloading from cloud storage:', error)
691696
throw error
692697
}

‎apps/sim/lib/projects/files/prefix-cleanup.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import {
2121
deleteStorageKeysUnderPrefix,
2222
listStorageKeyPage,
2323
outboxDeadlineNear,
24+
STORAGE_KEY_PAGE_SIZE,
2425
storageCleanupAvailableAt,
2526
} from '@/lib/uploads/core/prefix-sweep'
2627
import { abortProviderUpload } from '@/lib/uploads/upload-session/provider'
@@ -214,7 +215,7 @@ async function sweepPrefix(
214215
page.keys,
215216
context
216217
)
217-
if (complete && page.done) return 'done'
218+
if (complete && page.keys.length < STORAGE_KEY_PAGE_SIZE) return 'done'
218219
return deleted > 0 ? 'continue' : 'stalled'
219220
}
220221

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

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,8 @@ import { UPLOAD_DIR_SERVER } from '@/lib/uploads/core/setup.server'
1414
import { deleteFile } from '@/lib/uploads/core/storage-service'
1515
import { UPLOAD_URL_TTL_MS } from '@/lib/uploads/upload-session/provider'
1616

17-
const PAGE_SIZE = 500
17+
/** Most keys one {@link listStorageKeyPage} call returns. */
18+
export const STORAGE_KEY_PAGE_SIZE = 500
1819
const CLOCK_SKEW_MS = 60_000
1920
const DEADLINE_MARGIN_MS = 5000
2021

@@ -55,7 +56,7 @@ async function localKeys(prefix: string, signal: AbortSignal): Promise<string[]>
5556
const child = `${key}${entry.name}`
5657
if (entry.isDirectory()) await visit(`${child}/`)
5758
else keys.push(child)
58-
if (keys.length >= PAGE_SIZE) return
59+
if (keys.length >= STORAGE_KEY_PAGE_SIZE) return
5960
}
6061
await removeEmptyDirectory(join(UPLOAD_DIR_SERVER, key))
6162
}
@@ -94,7 +95,7 @@ export async function listStorageKeyPage(
9495
new ListObjectsV2Command({
9596
Bucket: config.bucket,
9697
Prefix: prefix,
97-
MaxKeys: PAGE_SIZE,
98+
MaxKeys: STORAGE_KEY_PAGE_SIZE,
9899
...(pageToken ? { ContinuationToken: pageToken } : {}),
99100
}),
100101
{ abortSignal: signal }
@@ -111,7 +112,10 @@ export async function listStorageKeyPage(
111112
const pages = (await getBlobServiceClient())
112113
.getContainerClient(config.containerName)
113114
.listBlobsFlat({ prefix, abortSignal: signal })
114-
.byPage({ maxPageSize: PAGE_SIZE, ...(pageToken ? { continuationToken: pageToken } : {}) })
115+
.byPage({
116+
maxPageSize: STORAGE_KEY_PAGE_SIZE,
117+
...(pageToken ? { continuationToken: pageToken } : {}),
118+
})
115119
const page = await pages.next()
116120
if (page.done) return { keys: [], done: true }
117121
return keyPage(
@@ -125,7 +129,7 @@ export async function listStorageKeyPage(
125129
if (!config.bucket) throw new Error(`Bucket missing for ${context} storage`)
126130
const [files, nextQuery] = await (await getGcsClient()).bucket(config.bucket).getFiles({
127131
prefix,
128-
maxResults: PAGE_SIZE,
132+
maxResults: STORAGE_KEY_PAGE_SIZE,
129133
autoPaginate: false,
130134
...(pageToken ? { pageToken } : {}),
131135
})
@@ -138,7 +142,7 @@ export async function listStorageKeyPage(
138142
)
139143
}
140144
const keys = await localKeys(prefix, signal)
141-
return { keys, done: keys.length < PAGE_SIZE }
145+
return { keys, done: keys.length < STORAGE_KEY_PAGE_SIZE }
142146
}
143147

144148
/**

‎apps/sim/lib/users/account-deletion.ts‎

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,10 @@ import {
5555
import type { StorageContext } from '@/lib/uploads'
5656
import { isUsingCloudStorage, StorageService } from '@/lib/uploads'
5757
import { createStorageReferenceGuard } from '@/lib/users/storage-reference-guard'
58-
import { enqueueAccountStorageCleanupInTx } from '@/lib/users/storage-sweep'
58+
import {
59+
collectAccountStorageObjects,
60+
enqueueAccountStorageCleanupInTx,
61+
} from '@/lib/users/storage-sweep'
5962
import {
6063
listSharedResourceWorkspaceIdsForUser,
6164
listUnresolvedSharedResourceWorkspaces,
@@ -794,6 +797,10 @@ export async function deleteUserAccount(userId: string): Promise<AccountDeletion
794797
const doomedWorkspaceIds = plan.workspacesToDelete.map((workspace) => workspace.id)
795798
const doomed = new Set(doomedWorkspaceIds)
796799
const storageKeys = await collectAccountStorageKeys(userId, doomedWorkspaceIds)
800+
const durableStorageObjects = await collectAccountStorageObjects({
801+
userId,
802+
workspaceIds: doomedWorkspaceIds,
803+
})
797804

798805
let cancelledDispatches: CancelledDispatch[] = []
799806
let cancelledMarkers: CancelledCellMarker[] = []
@@ -877,11 +884,12 @@ export async function deleteUserAccount(userId: string): Promise<AccountDeletion
877884
* workspace and user deletes cascade their rows away; delivered after commit.
878885
*/
879886
stoppedRuns = await stopLiveRunsTx(tx, { userId })
880-
/**
881-
* Queued while the doomed workspaces are locked and their rows still exist: every object
882-
* that cascades away below gets a durable, delayed cleanup that commits with the deletion.
883-
*/
884-
await enqueueAccountStorageCleanupInTx(tx, { userId, workspaceIds: doomedWorkspaceIds })
887+
/** Inserts only: the exact keys were read before the transaction took its locks. */
888+
await enqueueAccountStorageCleanupInTx(tx, {
889+
userId,
890+
workspaceIds: doomedWorkspaceIds,
891+
objects: durableStorageObjects,
892+
})
885893
if (doomedWorkspaceIds.length > 0) {
886894
/**
887895
* Re-checked here rather than trusted from the plan: a workspace that

‎apps/sim/lib/users/storage-reference-guard.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -156,7 +156,8 @@ async function loadUrlReferencedKeys(needles: readonly string[] | null): Promise
156156
*
157157
* - a row still names it: `workspace_files.key`, `workspace_file.key`,
158158
* `workspace_file_version.key`, `document.storage_key`, `execution_large_values.key`; or
159-
* - it sits under a world-readable root (`isPublicStorageKey`) and a stored asset URL names it (see
159+
* - it sits under a world-readable root (`isPublicStorageKey`, the same prefixes the file-serve
160+
* route's `isPublicByKeyPrefix` answers without a session) and a stored asset URL names it (see
160161
* {@link loadUrlReferencedKeys}). Only rows containing the key's longest URL-safe run are read,
161162
* each run at most once per guard. No URL column is read for any other key:
162163
* `verifyFileAccess` serves it only through a row, by its key path under a workspace that still

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

Lines changed: 28 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import type { OutboxEventContext } from '@/lib/core/outbox/service'
2121
import type { DbTransaction } from '@/lib/db/types'
2222
import {
2323
accountStorageSweepOutboxHandlers,
24+
collectAccountStorageObjects,
2425
enqueueAccountStorageCleanupInTx,
2526
} from '@/lib/users/storage-sweep'
2627
import {
@@ -360,6 +361,15 @@ describe('queueing account storage cleanup', () => {
360361
resetDbChainMock()
361362
})
362363

364+
async function queueCleanup(): Promise<void> {
365+
const objects = await collectAccountStorageObjects({ userId: 'user-1', workspaceIds: [] })
366+
await enqueueAccountStorageCleanupInTx(dbChainMock.db as unknown as DbTransaction, {
367+
userId: 'user-1',
368+
workspaceIds: [],
369+
objects,
370+
})
371+
}
372+
363373
function queuedObjectKeys(): string[] {
364374
return dbChainMockFns.values.mock.calls.flatMap(([rows]) =>
365375
(rows as { payload: { objects?: { key: string }[] } }[]).flatMap(
@@ -381,26 +391,36 @@ describe('queueing account storage cleanup', () => {
381391
queueTableRows(schemaMock.workspaceFileVersion, versionPage(0, 1000))
382392
queueTableRows(schemaMock.workspaceFileVersion, versionPage(1000, 5))
383393

384-
await enqueueAccountStorageCleanupInTx(dbChainMock.db as unknown as DbTransaction, {
385-
userId: 'user-1',
386-
workspaceIds: [],
387-
})
394+
await queueCleanup()
388395

389396
const keys = queuedObjectKeys()
390397
expect(keys).toHaveLength(1006)
391398
expect(keys).toContain('copilot/abc/v1004.txt')
392399
})
393400

401+
it('only inserts inside the deletion transaction', async () => {
402+
queueTableRows(schemaMock.workspaceFiles, [
403+
{ id: 'file-1', key: 'copilot/abc/file.txt', context: 'copilot' },
404+
])
405+
const objects = await collectAccountStorageObjects({ userId: 'user-1', workspaceIds: ['ws1'] })
406+
const insertOnly = { insert: dbChainMock.db.insert } as unknown as DbTransaction
407+
408+
await enqueueAccountStorageCleanupInTx(insertOnly, {
409+
userId: 'user-1',
410+
workspaceIds: ['ws1'],
411+
objects,
412+
})
413+
414+
expect(queuedObjectKeys()).toEqual(['copilot/abc/file.txt'])
415+
})
416+
394417
it('never queues a profile picture row', async () => {
395418
queueTableRows(schemaMock.workspaceFiles, [
396419
{ id: 'file-1', key: 'profile-pictures/org-logo.png', context: 'profile-pictures' },
397420
{ id: 'file-2', key: 'copilot/abc/file.txt', context: 'copilot' },
398421
])
399422

400-
await enqueueAccountStorageCleanupInTx(dbChainMock.db as unknown as DbTransaction, {
401-
userId: 'user-1',
402-
workspaceIds: [],
403-
})
423+
await queueCleanup()
404424

405425
expect(queuedObjectKeys()).toEqual(['copilot/abc/file.txt'])
406426
})

0 commit comments

Comments
 (0)