Skip to content

Commit 678c1a4

Browse files
authored
fix(account-deletion): durably purge a deleted account's storage without touching live objects (#9146)
* fix(account-deletion): durably purge a deleted workspace's storage by prefix Account deletion found the objects to erase by reading their rows before the teardown transaction and deleting them in memory after commit. Anything that read could not see stayed in storage forever: rows written between the read and the workspace lock, rowless objects (table snapshots, compiled documents, table-import leftovers, execution objects whose metadata insert failed), everything past the 100k in-memory cap, everything after a crash following commit, and the account's private files in workspaces that survive it, whose rows cascade away with the user. Inside the deletion transaction, after the workspace rows are locked, account deletion now queues delayed outbox work that commits or rolls back with it: - one prefix sweep per deleted workspace across workspace/, table-import/, execution/, table-snapshots/, copilot-doc-compiled/ and workspace-logos/ in the bucket serving each, with a trailing-slash prefix built from a validated id; - exact-key cleanup for what cascades outside those prefixes: knowledge-base documents, Copilot uploads and other non-prefixed files of deleted workspaces, and the account's private files elsewhere (attachments of its own chats, private contexts, files with no workspace). Handed-off, Project, organization files and attachments of someone else's chat are not queued. Both wait out every upload URL signed before the deletion, refuse as a no-op when the workspace or account row exists again (restore, PITR), and keep any key a surviving workspace_files, workspace_file, workspace_file_version, document or execution_large_values row still references. The sweep reuses the Project prefix cleanup's provider listing and deletion, generalized by storage context and given an in-run provider cursor; Project cleanup still lists one page per run and deletes exactly as before. The existing in-memory purge is unchanged and still runs. Large execution values now pass cleanupOnMetadataFailure, so a failed metadata insert deletes the freshly keyed object instead of orphaning it, as execution files already do. * 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. * fix(account-deletion): bound and harden the storage reference guard A stored URL with malformed percent-encoding (for example `/api/files/serve/%E0%A4%A`, which any workspace admin can store as a logo URL) made extractStorageKey throw. The guard then rejected on every call, so every storage purge on the platform failed closed and deleted nothing. The guard now parses each value under its own try and falls back to the raw, undecoded path. extractStorageKey itself is unchanged for its other callers. The URL-column check read six unindexed tables on almost every guard call. Only keys under the prefixes the file-serve route answers without a row or a session (profile-pictures/, og-images/, workspace-logos/, organization-logos/, per isPublicByKeyPrefix in the serve route) can be live through a stored URL alone. The guard now reads URL columns only for such keys, and only rows containing the key's longest URL-safe run, which percent-encoding never splits. Each run is read at most once per guard, 50 per query. A key with no run long enough falls back to the unfiltered read. Lookup failures still throw, so nothing is deleted. The workspace sweep now defers with a distinct reason when a full local page removes nothing, so the test for it can tell a stall from a deadline. * refactor(account-deletion): split the storage reference guard and share storage primitives - keysNamedByUrl also yields a leniently decoded candidate when a stored URL's percent-encoding is malformed, so a legacy key holding a literal '%' is kept - correct the guard's TSDoc: rowless keys stay reachable through key paths under a live workspace and object metadata, not only through a row - move the reference guard to lib/users/storage-reference-guard.ts - move prefix listing/deletion, the upload-TTL cleanup delay, and the outbox deadline margin from lib/projects/files/prefix-cleanup.ts to lib/uploads/core/prefix-sweep.ts; drop the projectUploadCleanupAvailableAt alias - share WORKSPACE_SHARED_FILE_CONTEXTS between member handoff and account cleanup - isPublicStorageKey replaces the public-prefix lists in the serve route and guard * 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 * fix(account-deletion): close the storage cleanup gaps review found - integration fixture rows get unique names: the active-name unique index on workspace_files rejected the second 'fixture.txt' row, failing seed() - the reference guard reads stored asset URLs unfiltered and decodes them, so a URL that percent-encodes the key's characters still keeps its object; the world-readable-key gate still decides whether any URL column is read - enqueueAccountStorageCleanupInTx re-reads the account's rows written since the pre-transaction collection (via the user_id and uploaded_by indexes) under the account lock, so a late Copilot or knowledge-base upload is queued - local listings walk keys in sorted order and page after the last key, so a workspace sweep advances past a full page of kept objects * docs(account-deletion): describe the in-transaction delta read accurately * fix(account-deletion): narrow stored-URL reads and re-check workspaces per page - fix TS2769: the collection scope passed a readonly id list to notInArray - the reference guard reads stored asset URL rows only where they mention a public key's owning id run, paged, and keeps a public key with no such run instead of reading every URL column - the account deletion request no longer purges world-readable keys; the account's own avatar is queued for the delayed durable cleanup instead - the workspace prefix sweep re-checks that the workspace is still gone before every page it deletes from * fix(account-deletion): queue an avatar only when the account uploaded it user.image accepts any serve URL, so it names a key without proving whose it is. The avatar is queued for durable cleanup only when the account's completed profile_picture upload session, or an older profile-picture workspace_files row of the account, names the same key; otherwise it is left in place. * fix(account-deletion): apply every purge guard on every call - the reference guard keeps keys under a live workspace's keyed roots itself, so the request purge gets the check the durable cleanup already had - stored asset URLs are re-read for every guard call rather than cached for the sweep, so a reference written mid-sweep still keeps its object - an avatar key that is not a safe profile-pictures key is never queued * fix(account-deletion): queue the account's avatar and let the guard keep shared ones An ownership lookup left avatars uploaded more than a week before deletion in storage, since their upload sessions are gone by then. The key user.image names is queued again, and the run-time reference guard keeps it whenever another account's user.image or a stored asset URL still names it.
1 parent 99ed5fb commit 678c1a4

20 files changed

Lines changed: 2474 additions & 166 deletions

‎apps/sim/lib/core/outbox/handlers.ts‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,10 @@ import {
4545
WORKSPACE_FILE_LIVE_DOC_OUTBOX_EVENT,
4646
WORKSPACE_FILE_STORAGE_CLEANUP_OUTBOX_EVENT,
4747
} from '@/lib/uploads/contexts/workspace/file-outbox-events'
48+
import {
49+
ACCOUNT_STORAGE_OBJECT_CLEANUP_EVENT,
50+
ACCOUNT_WORKSPACE_STORAGE_SWEEP_EVENT,
51+
} from '@/lib/users/storage-sweep-events'
4852
import { WORKFLOW_DEPLOYMENT_OUTBOX_EVENTS } from '@/lib/workflows/deployment-outbox-events'
4953
import { MIGRATED_INVITATION_EMAIL_EVENT_TYPE } from '@/lib/workspaces/admin-move-event'
5054
import {
@@ -239,6 +243,13 @@ export const OUTBOX_HANDLER_GROUPS: readonly LazyOutboxHandlerGroup[] = [
239243
return projectFilePrefixCleanupOutboxHandlers
240244
},
241245
},
246+
{
247+
events: [ACCOUNT_WORKSPACE_STORAGE_SWEEP_EVENT, ACCOUNT_STORAGE_OBJECT_CLEANUP_EVENT],
248+
load: async () => {
249+
const { accountStorageSweepOutboxHandlers } = await import('@/lib/users/storage-sweep')
250+
return accountStorageSweepOutboxHandlers
251+
},
252+
},
242253
{
243254
events: [PROJECT_FILE_DOCUMENT_RETIRE_EVENT],
244255
load: async () => {
Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,80 @@
1+
import { mkdtemp, readdir, rm } from 'node:fs/promises'
2+
import { tmpdir } from 'node:os'
3+
import { join } from 'node:path'
4+
import {
5+
largeValueMetadataMock,
6+
largeValueMetadataMockFns,
7+
} from '@sim/testing/mocks/large-value-metadata.mock'
8+
import { uploadsConfigMock } from '@sim/testing/mocks/uploads-config.mock'
9+
import {
10+
uploadsMetadataMock,
11+
uploadsMetadataMockFns,
12+
} from '@sim/testing/mocks/uploads-metadata.mock'
13+
import { setUploadDirServer, uploadsSetupMock } from '@sim/testing/mocks/uploads-setup.mock'
14+
import { describeError } from '@sim/utils/errors'
15+
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
16+
17+
vi.mock('@/lib/uploads/core/setup.server', () => uploadsSetupMock)
18+
vi.mock('@/lib/uploads/config', () => uploadsConfigMock)
19+
vi.mock('@/lib/uploads/server/metadata', () => uploadsMetadataMock)
20+
vi.mock('@/lib/execution/payloads/large-value-metadata', () => largeValueMetadataMock)
21+
22+
import { storeLargeValue } from '@/lib/execution/payloads/store'
23+
24+
const { mockInsertFileMetadata, mockDeleteFileMetadata } = uploadsMetadataMockFns
25+
const scope = { workspaceId: 'workspace-1', workflowId: 'workflow-1', executionId: 'execution-1' }
26+
27+
let directory: string
28+
29+
async function storedObjects(): Promise<string[]> {
30+
try {
31+
return await readdir(join(directory, 'execution', ...Object.values(scope)))
32+
} catch (error) {
33+
if (describeError(error).code === 'ENOENT') return []
34+
throw error
35+
}
36+
}
37+
38+
describe('large value persistence against real local storage', () => {
39+
beforeEach(async () => {
40+
directory = await mkdtemp(join(tmpdir(), 'sim-large-value-'))
41+
setUploadDirServer(directory)
42+
mockInsertFileMetadata.mockReset().mockResolvedValue({ id: 'file-1' })
43+
mockDeleteFileMetadata.mockReset().mockResolvedValue(true)
44+
largeValueMetadataMockFns.mockRegisterLargeValueOwner.mockResolvedValue(true)
45+
})
46+
47+
afterEach(async () => {
48+
await rm(directory, { recursive: true, force: true })
49+
})
50+
51+
it('removes the uploaded object when its metadata row cannot be written', async () => {
52+
mockInsertFileMetadata.mockRejectedValueOnce(new Error('workspace no longer exists'))
53+
const json = JSON.stringify({ payload: 'x'.repeat(2048) })
54+
55+
await expect(
56+
storeLargeValue(JSON.parse(json), json, Buffer.byteLength(json), {
57+
...scope,
58+
userId: 'user-1',
59+
requireDurable: true,
60+
})
61+
).rejects.toThrow('Failed to persist large execution value')
62+
63+
expect(await storedObjects()).toEqual([])
64+
expect(largeValueMetadataMockFns.mockRegisterLargeValueOwner).not.toHaveBeenCalled()
65+
})
66+
67+
it('keeps the object once its metadata row is written', async () => {
68+
const json = JSON.stringify({ payload: 'y'.repeat(2048) })
69+
70+
const ref = await storeLargeValue(JSON.parse(json), json, Buffer.byteLength(json), {
71+
...scope,
72+
userId: 'user-1',
73+
requireDurable: true,
74+
})
75+
76+
expect((await storedObjects()).filter((name) => name.endsWith('.json'))).toEqual([
77+
ref.key?.split('/').pop(),
78+
])
79+
})
80+
})

‎apps/sim/lib/execution/payloads/store.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,7 @@ async function persistValue(
9090
context: 'execution',
9191
preserveKey: true,
9292
customKey: key,
93+
cleanupOnMetadataFailure: true,
9394
metadata: {
9495
originalName: `large-value-${id}.json`,
9596
uploadedAt: new Date().toISOString(),

‎apps/sim/lib/projects/files/application/uploads.ts‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -29,10 +29,7 @@ import {
2929
prepareProjectFileAccounting,
3030
recordProjectFileWriteEffects,
3131
} from '@/lib/projects/files/application/write-effects'
32-
import {
33-
projectUploadCleanupAvailableAt,
34-
queueRetiredProjectUploadCleanup,
35-
} from '@/lib/projects/files/prefix-cleanup'
32+
import { queueRetiredProjectUploadCleanup } from '@/lib/projects/files/prefix-cleanup'
3633
import { resolveFileFolderTarget } from '@/lib/uploads/contexts/workspace/file-folder-manager'
3734
import {
3835
adoptVerifiedUploadSession,
@@ -43,6 +40,7 @@ import {
4340
} from '@/lib/uploads/contexts/workspace/file-manager'
4441
import type { WorkspaceFileSecretProvenance } from '@/lib/uploads/contexts/workspace/file-secret-provenance'
4542
import { enqueueWorkspaceFileStorageCleanups } from '@/lib/uploads/contexts/workspace/file-storage-cleanup-outbox'
43+
import { storageCleanupAvailableAt } from '@/lib/uploads/core/prefix-sweep'
4644
import { downloadFile } from '@/lib/uploads/core/storage-service'
4745
import { requestOrigin } from '@/lib/uploads/upload-session/application'
4846
import { assertProjectFileUploadBinding } from '@/lib/uploads/upload-session/project-file-binding'
@@ -321,7 +319,7 @@ const registerProjectUpload = defineAuthorizedProjectFileUseCase({
321319
await registration.registerFile(file.id)
322320
const cleanupIds = prepared.restored
323321
? await enqueueWorkspaceFileStorageCleanups(tx, [input.session.finalKey], 'project', {
324-
availableAt: projectUploadCleanupAvailableAt(),
322+
availableAt: storageCleanupAvailableAt(),
325323
})
326324
: []
327325
const result = await mapProjectFileResult(tx, context, file)
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+
})

0 commit comments

Comments
 (0)