Skip to content

Commit d4475f0

Browse files
committed
fix(account-deletion): never purge objects that live data still names
Review of the durable storage sweep found ways it, or the in-memory purge it backs up, could delete an object another tenant still uses. Reference guard. Every account-deletion purge now runs one check before deleting a key: the durable prefix sweep (per listed key), the durable exact-key cleanup, and the post-commit in-memory purge. A key is kept when a row still names it (workspace_files.key, workspace_file.key, workspace_file_version.key, document.storage_key, execution_large_values.key) or a stored asset URL names it: user.image, workspace.logo_url, organization.logo, organization.whitelabel_settings, organization.metadata, custom_block.icon_url, chat.customizations (deployed-chat logo and avatar) and oauth_client.icon. URLs are mapped to keys with extractStorageKey, so serve paths with or without a provider segment and direct bucket URLs both match; over-matching only keeps an object. References embedded in free-form JSON or text (workflow sub-block values, chat messages, Markdown links) are not consulted; for the deleted account's own chat attachments that is the intended erasure. This keeps whitelabel logos that were uploaded under an admin's personal workspace prefix, and legacy kb/ objects shared across workspaces, which the in-memory purge used to delete unchecked. Exact-key cleanup also keeps any key under a workspace-keyed root whose workspace still exists, since its members can read it by key through the metadata fallback. Workspace-less rows are queued only for the Copilot context: the workspace binding check was added NOT VALID, so legacy shared-context rows without a workspace can name a surviving workspace's object. Profile pictures are taken only from the account's own user.image, never from rows, because whitelabel logos were once stored as profile-pictures rows under an admin's account; organization logos are never queued either. Prefix deletion now counts only objects actually removed. A tolerated ENOENT (locally, any file whose on-disk name no sanitized key resolves to) used to count as progress, so a page of such files re-listed forever without spending an attempt. The workspace sweep and the Project prefix cleanup (which had the same flaw) now defer and spend an attempt when a full page removes nothing. A local metadata sidecar is kept whenever its object is. Versions of the account's private files are queued in keyset pages that stop at the cap instead of one unbounded query inside the deletion transaction.
1 parent 6de1f2f commit d4475f0

7 files changed

Lines changed: 567 additions & 104 deletions

File tree

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,79 @@
1+
import { mkdir, mkdtemp, rm, stat, writeFile } from 'node:fs/promises'
2+
import { tmpdir } from 'node:os'
3+
import { dirname, join } from 'node:path'
4+
import { resetDbChainMock } from '@sim/testing'
5+
import { uploadsConfigMock } from '@sim/testing/mocks/uploads-config.mock'
6+
import { uploadsMetadataMock } from '@sim/testing/mocks/uploads-metadata.mock'
7+
import { setUploadDirServer, uploadsSetupMock } from '@sim/testing/mocks/uploads-setup.mock'
8+
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
9+
10+
vi.mock('@/lib/uploads/core/setup.server', () => uploadsSetupMock)
11+
vi.mock('@/lib/uploads/config', () => uploadsConfigMock)
12+
vi.mock('@/lib/uploads/server/metadata', () => uploadsMetadataMock)
13+
14+
import type { OutboxEventContext } from '@/lib/core/outbox/service'
15+
import { PROJECT_FILE_PREFIX_CLEANUP_EVENT } from '@/lib/projects/files/outbox-events'
16+
import { projectFilePrefixCleanupOutboxHandlers } from '@/lib/projects/files/prefix-cleanup'
17+
18+
const cleanup = projectFilePrefixCleanupOutboxHandlers[PROJECT_FILE_PREFIX_CLEANUP_EVENT]
19+
20+
let root: string
21+
22+
function outboxContext(deadlineAt?: number): OutboxEventContext {
23+
return {
24+
eventId: 'event-1',
25+
eventType: PROJECT_FILE_PREFIX_CLEANUP_EVENT,
26+
attempts: 0,
27+
maxAttempts: 10,
28+
signal: new AbortController().signal,
29+
...(deadlineAt ? { deadlineAt } : {}),
30+
checkpointPayload: vi.fn(async () => {}),
31+
}
32+
}
33+
34+
async function put(key: string): Promise<void> {
35+
await mkdir(dirname(join(root, key)), { recursive: true })
36+
await writeFile(join(root, key), 'x')
37+
}
38+
39+
async function exists(key: string): Promise<boolean> {
40+
try {
41+
await stat(join(root, key))
42+
return true
43+
} catch {
44+
return false
45+
}
46+
}
47+
48+
describe('Project prefix cleanup on local storage', () => {
49+
beforeEach(async () => {
50+
resetDbChainMock()
51+
root = await mkdtemp(join(tmpdir(), 'sim-project-prefix-'))
52+
setUploadDirServer(root)
53+
})
54+
55+
afterEach(async () => {
56+
await rm(root, { recursive: true, force: true })
57+
})
58+
59+
it('deletes the retired Project prefix and completes', async () => {
60+
await put('project/p1/a.txt')
61+
await put('project/p1/nested/b.txt')
62+
await put('project/p10/keep.txt')
63+
64+
await expect(cleanup({ projectId: 'p1' }, outboxContext())).resolves.toBeUndefined()
65+
66+
expect(await exists('project/p1/a.txt')).toBe(false)
67+
expect(await exists('project/p1/nested/b.txt')).toBe(false)
68+
expect(await exists('project/p10/keep.txt')).toBe(true)
69+
})
70+
71+
it('spends an attempt when a full page holds only objects it cannot remove', async () => {
72+
for (let index = 0; index < 520; index++) await put(`project/p1/legacy file ${index}.txt`)
73+
74+
const result = await cleanup({ projectId: 'p1' }, outboxContext(Date.now() + 60_000))
75+
76+
expect(result).toMatchObject({ outcome: 'deferred' })
77+
expect(result).not.toMatchObject({ consumeAttempt: false })
78+
})
79+
})

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

Lines changed: 22 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import { isRecordLike } from '@sim/utils/object'
77
import { and, asc, eq, gt, inArray, isNull, sql } from 'drizzle-orm'
88
import {
99
continueOutboxHandler,
10+
deferOutboxHandler,
1011
enqueueOutboxEvent,
1112
type OutboxEventContext,
1213
type OutboxHandler,
@@ -332,7 +333,9 @@ export async function listStorageKeyPage(
332333
/**
333334
* Deletes listed keys, each of which must sit under `prefix`, and prunes local directories the
334335
* deletions empty, never above the prefix itself. Stops before another deletion once the handler
335-
* deadline is near; `complete` is false when it did.
336+
* deadline is near; `complete` is false when it did. `deleted` counts only objects actually
337+
* removed: a key already gone (or, locally, one whose on-disk name no storage key resolves to) is
338+
* no progress, so a caller re-listing the same page can tell that it is stuck.
336339
*/
337340
export async function deleteStorageKeysUnderPrefix(
338341
prefix: string,
@@ -341,17 +344,21 @@ export async function deleteStorageKeysUnderPrefix(
341344
outbox: Pick<OutboxEventContext, 'signal' | 'deadlineAt'>
342345
): Promise<{ deleted: number; complete: boolean }> {
343346
if (prefix.length < 2 || !prefix.endsWith('/')) throw new Error('Unbounded storage prefix')
347+
let attempted = 0
344348
let deleted = 0
345349
for (const key of keys) {
346350
if (!key.startsWith(prefix))
347351
throw new Error('Storage provider returned a key outside the prefix')
348352
outbox.signal.throwIfAborted()
349-
if (deleted > 0 && outbox.deadlineAt && Date.now() + 5000 >= outbox.deadlineAt)
353+
if (attempted > 0 && outbox.deadlineAt && Date.now() + 5000 >= outbox.deadlineAt)
350354
return { deleted, complete: false }
355+
attempted++
356+
let removed = true
351357
try {
352358
await deleteFile({ key, context, signal: outbox.signal })
353359
} catch (error) {
354360
if (describeError(error).code !== 'ENOENT') throw error
361+
removed = false
355362
}
356363
if (!USE_S3_STORAGE && !USE_BLOB_STORAGE && !USE_GCS_STORAGE) {
357364
const root = join(UPLOAD_DIR_SERVER, prefix.slice(0, -1))
@@ -361,16 +368,21 @@ export async function deleteStorageKeysUnderPrefix(
361368
directory = dirname(directory)
362369
}
363370
}
364-
deleted++
371+
if (removed) deleted++
365372
}
366373
return { deleted, complete: true }
367374
}
368375

369-
async function sweepPrefix(projectId: string, context: OutboxEventContext): Promise<boolean> {
376+
/** `stalled` when a page that must be re-listed removed nothing, so continuing cannot progress. */
377+
async function sweepPrefix(
378+
projectId: string,
379+
context: OutboxEventContext
380+
): Promise<'done' | 'continue' | 'stalled'> {
370381
const prefix = prefixFor(projectId)
371382
const { keys } = await listStorageKeyPage(prefix, 'project', context.signal)
372-
const { complete } = await deleteStorageKeysUnderPrefix(prefix, 'project', keys, context)
373-
return complete && keys.length < PAGE_SIZE
383+
const { deleted, complete } = await deleteStorageKeysUnderPrefix(prefix, 'project', keys, context)
384+
if (complete && keys.length < PAGE_SIZE) return 'done'
385+
return deleted > 0 ? 'continue' : 'stalled'
374386
}
375387

376388
const cleanupPrefix: OutboxHandler<unknown> = async (raw, context) => {
@@ -410,8 +422,10 @@ const cleanupPrefix: OutboxHandler<unknown> = async (raw, context) => {
410422
return continueOutboxHandler('Continuing retired Project upload cleanup')
411423
}
412424
}
413-
if (!(await sweepPrefix(payload.projectId, context)))
414-
return continueOutboxHandler('Continuing Project storage cleanup')
425+
const sweep = await sweepPrefix(payload.projectId, context)
426+
if (sweep === 'continue') return continueOutboxHandler('Continuing Project storage cleanup')
427+
if (sweep === 'stalled')
428+
return deferOutboxHandler('Project storage cleanup removed nothing from a full listing page')
415429
}
416430

417431
/**

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,18 @@ describe('account deletion of private organization Assistant images', () => {
9090
}
9191
)
9292

93+
it('keeps an object a surviving row still references', async () => {
94+
queueTableRows(schemaMock.uploadSession, [
95+
{ id: 'upload-1', key: IMAGE_KEY },
96+
{ id: 'upload-2', key: FAILED_IMAGE_KEY },
97+
])
98+
queueTableRows(schemaMock.document, [{ key: IMAGE_KEY }])
99+
100+
await deleteUserAccount('user-1')
101+
102+
expect(mockDeleteFiles).toHaveBeenCalledExactlyOnceWith([FAILED_IMAGE_KEY], 'mothership')
103+
})
104+
93105
it('retains ownership while an issued upload URL could recreate a purged object', async () => {
94106
queueTableRows(schemaMock.uploadSession, [{ id: 'upload-1', key: IMAGE_KEY }])
95107

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

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,10 @@ import {
5454
} from '@/lib/table/rows/executions'
5555
import type { StorageContext } from '@/lib/uploads'
5656
import { isUsingCloudStorage, StorageService } from '@/lib/uploads'
57-
import { enqueueAccountStorageCleanupInTx } from '@/lib/users/storage-sweep'
57+
import {
58+
createStorageReferenceGuard,
59+
enqueueAccountStorageCleanupInTx,
60+
} from '@/lib/users/storage-sweep'
5861
import {
5962
listSharedResourceWorkspaceIdsForUser,
6063
listUnresolvedSharedResourceWorkspaces,
@@ -638,11 +641,25 @@ async function collectAccountStorageKeys(
638641
* time this runs the account is already gone, and the request must not report a
639642
* failure for work that cannot be undone. An orphaned object is recoverable from
640643
* the log; a deletion the caller believes failed is not.
644+
*
645+
* Keys a surviving row or stored asset URL still names are kept (a legacy `kb/`
646+
* object can back documents in more than one workspace); see
647+
* `createStorageReferenceGuard`.
641648
*/
642649
async function purgeStorageObjects(userId: string, batches: StorageKeyBatch[]): Promise<void> {
643-
for (const { context, keys } of batches) {
644-
if (keys.length === 0) continue
650+
const isReferenced = createStorageReferenceGuard()
651+
for (const { context, keys: collected } of batches) {
652+
if (collected.length === 0) continue
645653
try {
654+
const referenced = await isReferenced(collected)
655+
if (referenced.size > 0) {
656+
logger.warn('Keeping objects a surviving reference still names', {
657+
context,
658+
kept: referenced.size,
659+
})
660+
}
661+
const keys = collected.filter((key) => !referenced.has(key))
662+
if (keys.length === 0) continue
646663
const { failed } = await StorageService.deleteFiles(keys, context)
647664
for (const { key, error } of failed) {
648665
logger.error('Failed to erase stored object during account deletion', {

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

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -151,6 +151,9 @@ async function seed() {
151151
privateAttachment: `workspace/${survivingId}/${generateId()}/private.txt`,
152152
othersChatAttachment: `workspace/${survivingId}/${generateId()}/others.txt`,
153153
personalCopilot: `copilot/${generateId()}/personal.txt`,
154+
legacyWorkspacelessKb: `kb/${generateId()}-legacy.txt`,
155+
profilePictureRowLogo: `profile-pictures/${generateId()}/org-logo.png`,
156+
logoNamedByUrl: `workspace-logos/${doomedId}/${generateId()}-org-logo.png`,
154157
}
155158
await db.insert(workspaceFiles).values([
156159
fileRow({
@@ -197,7 +200,23 @@ async function seed() {
197200
workspaceId: null,
198201
context: 'copilot',
199202
}),
203+
fileRow({
204+
key: keys.legacyWorkspacelessKb,
205+
userId: departingId,
206+
workspaceId: null,
207+
context: 'knowledge-base',
208+
}),
209+
fileRow({
210+
key: keys.profilePictureRowLogo,
211+
userId: departingId,
212+
workspaceId: null,
213+
context: 'profile-pictures',
214+
}),
200215
])
216+
await db
217+
.update(user)
218+
.set({ image: `/api/files/serve/${encodeURIComponent(keys.logoNamedByUrl)}` })
219+
.where(eq(user.id, ownerId))
201220
for (const key of Object.values(keys)) await put(key)
202221
return { ownerId, departingId, doomedId, survivingId, keys }
203222
}
@@ -259,6 +278,8 @@ describe('account deletion storage sweep', () => {
259278
)
260279
expect(queuedKeys.map((object) => object.key)).not.toContain(f.keys.othersChatAttachment)
261280
expect(queuedKeys.map((object) => object.key)).not.toContain(f.keys.handedOff)
281+
expect(queuedKeys.map((object) => object.key)).not.toContain(f.keys.legacyWorkspacelessKb)
282+
expect(queuedKeys.map((object) => object.key)).not.toContain(f.keys.profilePictureRowLogo)
262283

263284
const lateUpload = `workspace/${f.doomedId}/late-${generateId()}.bin`
264285
await put(lateUpload)
@@ -270,7 +291,6 @@ describe('account deletion storage sweep', () => {
270291
f.keys.rowlessExecution,
271292
f.keys.rowlessSnapshot,
272293
f.keys.rowlessCompiled,
273-
f.keys.privateAttachment,
274294
f.keys.personalCopilot,
275295
lateUpload,
276296
])
@@ -280,6 +300,10 @@ describe('account deletion storage sweep', () => {
280300
f.keys.neighbor,
281301
f.keys.handedOff,
282302
f.keys.othersChatAttachment,
303+
f.keys.privateAttachment,
304+
f.keys.legacyWorkspacelessKb,
305+
f.keys.profilePictureRowLogo,
306+
f.keys.logoNamedByUrl,
283307
])
284308
expect(await readFile(join(storageRoot, key), 'utf8')).toBe('x')
285309
expect(

0 commit comments

Comments
 (0)