From 6de1f2f6b7480ba91a53124a7204052e9a3541f6 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Fri, 16 Oct 2026 14:55:43 -0700 Subject: [PATCH 01/11] 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. --- apps/sim/lib/core/outbox/handlers.ts | 11 + .../execution/payloads/store.local.test.ts | 80 +++ apps/sim/lib/execution/payloads/store.ts | 1 + apps/sim/lib/projects/files/prefix-cleanup.ts | 117 +++- apps/sim/lib/users/account-deletion.ts | 6 + apps/sim/lib/users/storage-sweep-events.ts | 2 + .../lib/users/storage-sweep.integration.ts | 327 +++++++++ apps/sim/lib/users/storage-sweep.test.ts | 211 ++++++ apps/sim/lib/users/storage-sweep.ts | 618 ++++++++++++++++++ 9 files changed, 1349 insertions(+), 24 deletions(-) create mode 100644 apps/sim/lib/execution/payloads/store.local.test.ts create mode 100644 apps/sim/lib/users/storage-sweep-events.ts create mode 100644 apps/sim/lib/users/storage-sweep.integration.ts create mode 100644 apps/sim/lib/users/storage-sweep.test.ts create mode 100644 apps/sim/lib/users/storage-sweep.ts diff --git a/apps/sim/lib/core/outbox/handlers.ts b/apps/sim/lib/core/outbox/handlers.ts index 77c088f6b52..e4596596e33 100644 --- a/apps/sim/lib/core/outbox/handlers.ts +++ b/apps/sim/lib/core/outbox/handlers.ts @@ -44,6 +44,10 @@ import { WORKSPACE_FILE_LIVE_DOC_OUTBOX_EVENT, WORKSPACE_FILE_STORAGE_CLEANUP_OUTBOX_EVENT, } from '@/lib/uploads/contexts/workspace/file-outbox-events' +import { + ACCOUNT_STORAGE_OBJECT_CLEANUP_EVENT, + ACCOUNT_WORKSPACE_STORAGE_SWEEP_EVENT, +} from '@/lib/users/storage-sweep-events' import { WORKFLOW_DEPLOYMENT_OUTBOX_EVENTS } from '@/lib/workflows/deployment-outbox-events' import { MIGRATED_INVITATION_EMAIL_EVENT_TYPE } from '@/lib/workspaces/admin-move-event' import { @@ -231,6 +235,13 @@ export const OUTBOX_HANDLER_GROUPS: readonly LazyOutboxHandlerGroup[] = [ return projectFilePrefixCleanupOutboxHandlers }, }, + { + events: [ACCOUNT_WORKSPACE_STORAGE_SWEEP_EVENT, ACCOUNT_STORAGE_OBJECT_CLEANUP_EVENT], + load: async () => { + const { accountStorageSweepOutboxHandlers } = await import('@/lib/users/storage-sweep') + return accountStorageSweepOutboxHandlers + }, + }, { events: [PROJECT_FILE_DOCUMENT_RETIRE_EVENT], load: async () => { diff --git a/apps/sim/lib/execution/payloads/store.local.test.ts b/apps/sim/lib/execution/payloads/store.local.test.ts new file mode 100644 index 00000000000..38f061364da --- /dev/null +++ b/apps/sim/lib/execution/payloads/store.local.test.ts @@ -0,0 +1,80 @@ +import { mkdtemp, readdir, rm } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { + largeValueMetadataMock, + largeValueMetadataMockFns, +} from '@sim/testing/mocks/large-value-metadata.mock' +import { uploadsConfigMock } from '@sim/testing/mocks/uploads-config.mock' +import { + uploadsMetadataMock, + uploadsMetadataMockFns, +} from '@sim/testing/mocks/uploads-metadata.mock' +import { setUploadDirServer, uploadsSetupMock } from '@sim/testing/mocks/uploads-setup.mock' +import { describeError } from '@sim/utils/errors' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +vi.mock('@/lib/uploads/core/setup.server', () => uploadsSetupMock) +vi.mock('@/lib/uploads/config', () => uploadsConfigMock) +vi.mock('@/lib/uploads/server/metadata', () => uploadsMetadataMock) +vi.mock('@/lib/execution/payloads/large-value-metadata', () => largeValueMetadataMock) + +import { storeLargeValue } from '@/lib/execution/payloads/store' + +const { mockInsertFileMetadata, mockDeleteFileMetadata } = uploadsMetadataMockFns +const scope = { workspaceId: 'workspace-1', workflowId: 'workflow-1', executionId: 'execution-1' } + +let directory: string + +async function storedObjects(): Promise { + try { + return await readdir(join(directory, 'execution', ...Object.values(scope))) + } catch (error) { + if (describeError(error).code === 'ENOENT') return [] + throw error + } +} + +describe('large value persistence against real local storage', () => { + beforeEach(async () => { + directory = await mkdtemp(join(tmpdir(), 'sim-large-value-')) + setUploadDirServer(directory) + mockInsertFileMetadata.mockReset().mockResolvedValue({ id: 'file-1' }) + mockDeleteFileMetadata.mockReset().mockResolvedValue(true) + largeValueMetadataMockFns.mockRegisterLargeValueOwner.mockResolvedValue(true) + }) + + afterEach(async () => { + await rm(directory, { recursive: true, force: true }) + }) + + it('removes the uploaded object when its metadata row cannot be written', async () => { + mockInsertFileMetadata.mockRejectedValueOnce(new Error('workspace no longer exists')) + const json = JSON.stringify({ payload: 'x'.repeat(2048) }) + + await expect( + storeLargeValue(JSON.parse(json), json, Buffer.byteLength(json), { + ...scope, + userId: 'user-1', + requireDurable: true, + }) + ).rejects.toThrow('Failed to persist large execution value') + + expect(await storedObjects()).toEqual([]) + expect(largeValueMetadataMockFns.mockRegisterLargeValueOwner).not.toHaveBeenCalled() + }) + + it('keeps the object once its metadata row is written', async () => { + const json = JSON.stringify({ payload: 'y'.repeat(2048) }) + + const ref = await storeLargeValue(JSON.parse(json), json, Buffer.byteLength(json), { + ...scope, + userId: 'user-1', + requireDurable: true, + }) + + expect((await storedObjects()).filter((name) => name.endsWith('.json'))).toEqual([ + ref.key?.split('/').pop(), + ]) + }) +}) diff --git a/apps/sim/lib/execution/payloads/store.ts b/apps/sim/lib/execution/payloads/store.ts index 0aedf8bc19e..b59416371cf 100644 --- a/apps/sim/lib/execution/payloads/store.ts +++ b/apps/sim/lib/execution/payloads/store.ts @@ -90,6 +90,7 @@ async function persistValue( context: 'execution', preserveKey: true, customKey: key, + cleanupOnMetadataFailure: true, metadata: { originalName: `large-value-${id}.json`, uploadedAt: new Date().toISOString(), diff --git a/apps/sim/lib/projects/files/prefix-cleanup.ts b/apps/sim/lib/projects/files/prefix-cleanup.ts index 65b62c6b8d2..ea352b2ee2a 100644 --- a/apps/sim/lib/projects/files/prefix-cleanup.ts +++ b/apps/sim/lib/projects/files/prefix-cleanup.ts @@ -19,6 +19,7 @@ import { } from '@/lib/projects/files/outbox-events' import { getStorageConfig, + type StorageContext, USE_BLOB_STORAGE, USE_GCS_STORAGE, USE_S3_STORAGE, @@ -45,6 +46,11 @@ interface CleanupPayload { /** Expired signatures stop new transfers, but a transfer already in progress may finish later. */ export function projectUploadCleanupAvailableAt(now = new Date()): Date { + return storageCleanupAvailableAt(now) +} + +/** Earliest moment no upload URL signed before `now` can still write an object. */ +export function storageCleanupAvailableAt(now = new Date()): Date { return new Date(now.getTime() + UPLOAD_URL_TTL_MS + CLOCK_SKEW_MS) } @@ -242,57 +248,113 @@ async function localKeys(prefix: string, signal: AbortSignal): Promise return keys } -async function listKeys(prefix: string, signal: AbortSignal): Promise { +/** One provider page of object keys; `done` once the listing has nothing after this page. */ +interface StorageKeyPage { + keys: string[] + done: boolean + /** Provider cursor for the following page, valid only within the current run. */ + nextPageToken?: string +} + +/** + * Lists one page of object keys under `prefix` in the bucket serving `context`. A local listing + * has no cursor: it returns the first keys still on disk, so callers advance by deleting them. + */ +export async function listStorageKeyPage( + prefix: string, + context: StorageContext, + signal: AbortSignal, + pageToken?: string +): Promise { signal.throwIfAborted() if (USE_S3_STORAGE) { const { ListObjectsV2Command } = await import('@aws-sdk/client-s3') const { getS3Client } = await import('@/lib/uploads/providers/s3/client') - const config = getStorageConfig('project') + const config = getStorageConfig(context) const page = await getS3Client().send( - new ListObjectsV2Command({ Bucket: config.bucket, Prefix: prefix, MaxKeys: PAGE_SIZE }), + new ListObjectsV2Command({ + Bucket: config.bucket, + Prefix: prefix, + MaxKeys: PAGE_SIZE, + ...(pageToken ? { ContinuationToken: pageToken } : {}), + }), { abortSignal: signal } ) - return (page.Contents ?? []).flatMap((object) => (object.Key ? [object.Key] : [])) + const nextPageToken = page.IsTruncated ? page.NextContinuationToken : undefined + return { + keys: (page.Contents ?? []).flatMap((object) => (object.Key ? [object.Key] : [])), + done: !nextPageToken, + ...(nextPageToken ? { nextPageToken } : {}), + } } if (USE_BLOB_STORAGE) { const { getBlobServiceClient } = await import('@/lib/uploads/providers/blob/client') - const config = getStorageConfig('project') - if (!config.containerName) throw new Error('Project blob container missing') + const config = getStorageConfig(context) + if (!config.containerName) throw new Error(`Blob container missing for ${context} storage`) const pages = (await getBlobServiceClient()) .getContainerClient(config.containerName) .listBlobsFlat({ prefix, abortSignal: signal }) - .byPage({ maxPageSize: PAGE_SIZE }) + .byPage({ maxPageSize: PAGE_SIZE, ...(pageToken ? { continuationToken: pageToken } : {}) }) const page = await pages.next() - return page.done ? [] : page.value.segment.blobItems.map((object) => object.name) + if (page.done) return { keys: [], done: true } + const nextPageToken = page.value.continuationToken || undefined + return { + keys: page.value.segment.blobItems.map((object) => object.name), + done: !nextPageToken, + ...(nextPageToken ? { nextPageToken } : {}), + } } if (USE_GCS_STORAGE) { const { getGcsClient } = await import('@/lib/uploads/providers/google-cloud-storage/client') - const config = getStorageConfig('project') - if (!config.bucket) throw new Error('Project bucket missing') - const [files] = await (await getGcsClient()) - .bucket(config.bucket) - .getFiles({ prefix, maxResults: PAGE_SIZE, autoPaginate: false }) + const config = getStorageConfig(context) + if (!config.bucket) throw new Error(`Bucket missing for ${context} storage`) + const [files, nextQuery] = await (await getGcsClient()).bucket(config.bucket).getFiles({ + prefix, + maxResults: PAGE_SIZE, + autoPaginate: false, + ...(pageToken ? { pageToken } : {}), + }) signal.throwIfAborted() - return files.map((file) => file.name) + const nextPageToken = + isRecordLike(nextQuery) && typeof nextQuery.pageToken === 'string' && nextQuery.pageToken + ? nextQuery.pageToken + : undefined + return { + keys: files.map((file) => file.name), + done: !nextPageToken, + ...(nextPageToken ? { nextPageToken } : {}), + } } - return localKeys(prefix, signal) + const keys = await localKeys(prefix, signal) + return { keys, done: keys.length < PAGE_SIZE } } -async function sweepPrefix(projectId: string, context: OutboxEventContext): Promise { - const prefix = prefixFor(projectId) - const keys = await listKeys(prefix, context.signal) +/** + * Deletes listed keys, each of which must sit under `prefix`, and prunes local directories the + * deletions empty, never above the prefix itself. Stops before another deletion once the handler + * deadline is near; `complete` is false when it did. + */ +export async function deleteStorageKeysUnderPrefix( + prefix: string, + context: StorageContext, + keys: readonly string[], + outbox: Pick +): Promise<{ deleted: number; complete: boolean }> { + if (prefix.length < 2 || !prefix.endsWith('/')) throw new Error('Unbounded storage prefix') let deleted = 0 for (const key of keys) { - if (!key.startsWith(prefix)) throw new Error('Storage provider returned a foreign Project key') - context.signal.throwIfAborted() - if (deleted > 0 && context.deadlineAt && Date.now() + 5000 >= context.deadlineAt) return false + if (!key.startsWith(prefix)) + throw new Error('Storage provider returned a key outside the prefix') + outbox.signal.throwIfAborted() + if (deleted > 0 && outbox.deadlineAt && Date.now() + 5000 >= outbox.deadlineAt) + return { deleted, complete: false } try { - await deleteFile({ key, context: 'project', signal: context.signal }) + await deleteFile({ key, context, signal: outbox.signal }) } catch (error) { if (describeError(error).code !== 'ENOENT') throw error } if (!USE_S3_STORAGE && !USE_BLOB_STORAGE && !USE_GCS_STORAGE) { - const root = join(UPLOAD_DIR_SERVER, 'project', projectId) + const root = join(UPLOAD_DIR_SERVER, prefix.slice(0, -1)) let directory = dirname(join(UPLOAD_DIR_SERVER, key)) while (directory === root || directory.startsWith(`${root}/`)) { await removeEmptyDirectory(directory) @@ -301,7 +363,14 @@ async function sweepPrefix(projectId: string, context: OutboxEventContext): Prom } deleted++ } - return keys.length < PAGE_SIZE + return { deleted, complete: true } +} + +async function sweepPrefix(projectId: string, context: OutboxEventContext): Promise { + const prefix = prefixFor(projectId) + const { keys } = await listStorageKeyPage(prefix, 'project', context.signal) + const { complete } = await deleteStorageKeysUnderPrefix(prefix, 'project', keys, context) + return complete && keys.length < PAGE_SIZE } const cleanupPrefix: OutboxHandler = async (raw, context) => { diff --git a/apps/sim/lib/users/account-deletion.ts b/apps/sim/lib/users/account-deletion.ts index ca0c883f95f..87b212ee4fe 100644 --- a/apps/sim/lib/users/account-deletion.ts +++ b/apps/sim/lib/users/account-deletion.ts @@ -54,6 +54,7 @@ import { } from '@/lib/table/rows/executions' import type { StorageContext } from '@/lib/uploads' import { isUsingCloudStorage, StorageService } from '@/lib/uploads' +import { enqueueAccountStorageCleanupInTx } from '@/lib/users/storage-sweep' import { listSharedResourceWorkspaceIdsForUser, listUnresolvedSharedResourceWorkspaces, @@ -861,6 +862,11 @@ export async function deleteUserAccount(userId: string): Promise 0) { /** * Re-checked here rather than trusted from the plan: a workspace that diff --git a/apps/sim/lib/users/storage-sweep-events.ts b/apps/sim/lib/users/storage-sweep-events.ts new file mode 100644 index 00000000000..b709df01425 --- /dev/null +++ b/apps/sim/lib/users/storage-sweep-events.ts @@ -0,0 +1,2 @@ +export const ACCOUNT_WORKSPACE_STORAGE_SWEEP_EVENT = 'account-deletion.storage.workspace-sweep' +export const ACCOUNT_STORAGE_OBJECT_CLEANUP_EVENT = 'account-deletion.storage.object-cleanup' diff --git a/apps/sim/lib/users/storage-sweep.integration.ts b/apps/sim/lib/users/storage-sweep.integration.ts new file mode 100644 index 00000000000..8fd3cbe6b60 --- /dev/null +++ b/apps/sim/lib/users/storage-sweep.integration.ts @@ -0,0 +1,327 @@ +import { mkdtempSync } from 'node:fs' +import { mkdir, readFile, rm, stat, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { dirname, join } from 'node:path' +import { db } from '@sim/db' +import { + copilotChats, + outboxEvent, + permissions, + user, + workspace, + workspaceFiles, +} from '@sim/db/schema' +import { readTestDatabaseUrl } from '@sim/db/testing/test-infrastructure' +import { deleteWorkspaceFixture, insertWorkspaceFixture } from '@sim/db/testing/workspace-fixtures' +import { setUploadDirServer, uploadsSetupMock } from '@sim/testing/mocks/uploads-setup.mock' +import { getErrorMessage } from '@sim/utils/errors' +import { generateId } from '@sim/utils/id' +import { and, eq, inArray, or, sql } from 'drizzle-orm' +import { afterAll, describe, expect, it, vi } from 'vitest' + +vi.mock('@/lib/uploads/core/setup.server', () => uploadsSetupMock) + +import { processOutboxEventById } from '@/lib/core/outbox/service' +import { UPLOAD_URL_TTL_MS } from '@/lib/uploads/upload-session/provider' +import { deleteUserAccount } from '@/lib/users/account-deletion' +import { accountStorageSweepOutboxHandlers } from '@/lib/users/storage-sweep' +import { + ACCOUNT_STORAGE_OBJECT_CLEANUP_EVENT, + ACCOUNT_WORKSPACE_STORAGE_SWEEP_EVENT, +} from '@/lib/users/storage-sweep-events' + +readTestDatabaseUrl() + +const storageRoot = mkdtempSync(join(tmpdir(), 'sim-account-storage-sweep-')) +setUploadDirServer(storageRoot) + +const userIds: string[] = [] +const workspaceIds: string[] = [] +const checks: { name: string; status: 'passed' | 'failed'; durationMs: number; error?: string }[] = + [] + +function check(name: string, run: () => Promise) { + it(name, async () => { + const started = performance.now() + try { + await run() + checks.push({ name, status: 'passed', durationMs: performance.now() - started }) + } catch (error) { + checks.push({ + name, + status: 'failed', + durationMs: performance.now() - started, + error: getErrorMessage(error), + }) + throw error + } + }) +} + +async function put(key: string, body = 'x'): Promise { + await mkdir(dirname(join(storageRoot, key)), { recursive: true }) + await writeFile(join(storageRoot, key), body) +} + +async function exists(key: string): Promise { + try { + await stat(join(storageRoot, key)) + return true + } catch { + return false + } +} + +async function insertUsers(ids: string[]) { + userIds.push(...ids) + await db.insert(user).values( + ids.map((id) => ({ + id, + name: 'Storage sweep fixture', + email: `${id}@sweep.invalid`, + emailVerified: true, + createdAt: new Date(), + updatedAt: new Date(), + })) + ) +} + +async function insertAdminWorkspace(id: string, ownerId: string) { + workspaceIds.push(id) + await insertWorkspaceFixture(db, { + id, + name: 'Storage sweep fixture', + ownerId, + billedAccountUserId: ownerId, + }) + await db.insert(permissions).values({ + id: generateId(), + userId: ownerId, + entityType: 'workspace', + entityId: id, + permissionType: 'admin', + }) +} + +function fileRow(values: { + key: string + userId: string + workspaceId: string | null + context: string + chatId?: string +}) { + return { + id: generateId(), + originalName: 'fixture.txt', + contentType: 'text/plain', + sizeBytes: 1, + chatId: values.chatId ?? null, + ...values, + } +} + +/** + * A departing account with a private workspace, and files it left in another person's workspace + * after departing it: one handed to the workspace's billing account, one private chat attachment, + * one attached to the other person's chat, and one with no workspace at all. + */ +async function seed() { + const ownerId = generateId() + const departingId = generateId() + const doomedId = generateId() + const survivingId = generateId() + await insertUsers([ownerId, departingId]) + await insertAdminWorkspace(doomedId, departingId) + await insertAdminWorkspace(survivingId, ownerId) + const departingChatId = generateId() + const ownerChatId = generateId() + await db.insert(copilotChats).values([ + { id: departingChatId, userId: departingId, workspaceId: survivingId }, + { id: ownerChatId, userId: ownerId, workspaceId: survivingId }, + ]) + const keys = { + doomedFile: `workspace/${doomedId}/${generateId()}.txt`, + doomedCopilot: `copilot/${generateId()}/doomed.txt`, + rowlessExecution: `execution/${doomedId}/wf/ex/large-value-lv_rowless.json`, + rowlessSnapshot: `table-snapshots/${doomedId}/table/v1-hash.csv`, + rowlessCompiled: `copilot-doc-compiled/${doomedId}/hash.pdf`, + referencedElsewhere: `workspace/${doomedId}/moved-${generateId()}.txt`, + neighbor: `workspace/${doomedId}x/keep.txt`, + handedOff: `workspace/${survivingId}/${generateId()}.txt`, + privateAttachment: `workspace/${survivingId}/${generateId()}/private.txt`, + othersChatAttachment: `workspace/${survivingId}/${generateId()}/others.txt`, + personalCopilot: `copilot/${generateId()}/personal.txt`, + } + await db.insert(workspaceFiles).values([ + fileRow({ + key: keys.doomedFile, + userId: departingId, + workspaceId: doomedId, + context: 'workspace', + }), + fileRow({ + key: keys.doomedCopilot, + userId: departingId, + workspaceId: doomedId, + context: 'copilot', + }), + fileRow({ + key: keys.referencedElsewhere, + userId: ownerId, + workspaceId: survivingId, + context: 'workspace', + }), + fileRow({ + key: keys.handedOff, + userId: departingId, + workspaceId: survivingId, + context: 'workspace', + }), + fileRow({ + key: keys.privateAttachment, + userId: departingId, + workspaceId: survivingId, + context: 'mothership', + chatId: departingChatId, + }), + fileRow({ + key: keys.othersChatAttachment, + userId: departingId, + workspaceId: survivingId, + context: 'mothership', + chatId: ownerChatId, + }), + fileRow({ + key: keys.personalCopilot, + userId: departingId, + workspaceId: null, + context: 'copilot', + }), + ]) + for (const key of Object.values(keys)) await put(key) + return { ownerId, departingId, doomedId, survivingId, keys } +} + +function sweepEvents(workspaceId: string) { + return db + .select() + .from(outboxEvent) + .where( + and( + eq(outboxEvent.eventType, ACCOUNT_WORKSPACE_STORAGE_SWEEP_EVENT), + sql`${outboxEvent.payload}->>'workspaceId' = ${workspaceId}` + ) + ) +} + +function objectEvents(userId: string) { + return db + .select() + .from(outboxEvent) + .where( + and( + eq(outboxEvent.eventType, ACCOUNT_STORAGE_OBJECT_CLEANUP_EVENT), + sql`${outboxEvent.payload}->>'userId' = ${userId}` + ) + ) +} + +async function runNow(eventIds: string[]) { + for (const id of eventIds) { + await db + .update(outboxEvent) + .set({ availableAt: new Date(0) }) + .where(eq(outboxEvent.id, id)) + expect(await processOutboxEventById(id, accountStorageSweepOutboxHandlers)).toBe('completed') + } +} + +describe('account deletion storage sweep', () => { + check( + 'purges the deleted workspace by prefix and the account private files by key, keeping every live object', + async () => { + const f = await seed() + const startedAt = Date.now() + await deleteUserAccount(f.departingId) + + const sweeps = await sweepEvents(f.doomedId) + const objects = await objectEvents(f.departingId) + expect(sweeps).toHaveLength(1) + expect(objects.length).toBeGreaterThan(0) + for (const event of [...sweeps, ...objects]) { + expect(event.availableAt.getTime()).toBeGreaterThanOrEqual(startedAt + UPLOAD_URL_TTL_MS) + expect(await processOutboxEventById(event.id, accountStorageSweepOutboxHandlers)).toBe( + 'pending' + ) + } + const queuedKeys = objects.flatMap( + (event) => (event.payload as { objects: { key: string }[] }).objects + ) + expect(queuedKeys.map((object) => object.key)).not.toContain(f.keys.othersChatAttachment) + expect(queuedKeys.map((object) => object.key)).not.toContain(f.keys.handedOff) + + const lateUpload = `workspace/${f.doomedId}/late-${generateId()}.bin` + await put(lateUpload) + await runNow([...sweeps, ...objects].map((event) => event.id)) + + for (const key of [ + f.keys.doomedFile, + f.keys.doomedCopilot, + f.keys.rowlessExecution, + f.keys.rowlessSnapshot, + f.keys.rowlessCompiled, + f.keys.privateAttachment, + f.keys.personalCopilot, + lateUpload, + ]) + expect({ key, exists: await exists(key) }).toEqual({ key, exists: false }) + for (const key of [ + f.keys.referencedElsewhere, + f.keys.neighbor, + f.keys.handedOff, + f.keys.othersChatAttachment, + ]) + expect(await readFile(join(storageRoot, key), 'utf8')).toBe('x') + expect( + await db.select().from(workspaceFiles).where(eq(workspaceFiles.key, f.keys.handedOff)) + ).toMatchObject([{ userId: f.ownerId }]) + } + ) + + check('a sweep for a workspace that exists again completes without deleting', async () => { + const f = await seed() + await deleteUserAccount(f.departingId) + const [sweep] = await sweepEvents(f.doomedId) + await insertAdminWorkspace(f.doomedId, f.ownerId) + const restored = `workspace/${f.doomedId}/restored.txt` + await put(restored) + + await runNow([sweep.id]) + + expect(await readFile(join(storageRoot, restored), 'utf8')).toBe('x') + expect(await readFile(join(storageRoot, f.keys.rowlessExecution), 'utf8')).toBe('x') + }) +}) + +afterAll(async () => { + try { + await db + .delete(outboxEvent) + .where( + or( + inArray(sql`${outboxEvent.payload}->>'workspaceId'`, workspaceIds), + inArray(sql`${outboxEvent.payload}->>'userId'`, userIds) + ) + ) + await db.delete(workspaceFiles).where(inArray(workspaceFiles.userId, userIds)) + await deleteWorkspaceFixture(db, inArray(workspace.id, workspaceIds)) + await db.delete(user).where(inArray(user.id, userIds)) + await rm(storageRoot, { recursive: true, force: true }) + } finally { + const reportPath = process.env.ACCOUNT_STORAGE_SWEEP_REPORT_PATH + if (reportPath) { + await mkdir(dirname(reportPath), { recursive: true }) + await writeFile(reportPath, JSON.stringify({ checks }, null, 2)) + } + } +}) diff --git a/apps/sim/lib/users/storage-sweep.test.ts b/apps/sim/lib/users/storage-sweep.test.ts new file mode 100644 index 00000000000..81fa5bf6d48 --- /dev/null +++ b/apps/sim/lib/users/storage-sweep.test.ts @@ -0,0 +1,211 @@ +import { mkdir, mkdtemp, readFile, rm, stat, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { dirname, join } from 'node:path' +import { queueTableRows, resetDbChainMock, schemaMock } from '@sim/testing' +import { uploadsConfigMock } from '@sim/testing/mocks/uploads-config.mock' +import { uploadsMetadataMock } from '@sim/testing/mocks/uploads-metadata.mock' +import { setUploadDirServer, uploadsSetupMock } from '@sim/testing/mocks/uploads-setup.mock' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +vi.mock('@/lib/uploads/core/setup.server', () => uploadsSetupMock) +vi.mock('@/lib/uploads/config', () => uploadsConfigMock) +vi.mock('@/lib/uploads/server/metadata', () => uploadsMetadataMock) + +import type { OutboxEventContext } from '@/lib/core/outbox/service' +import { accountStorageSweepOutboxHandlers } from '@/lib/users/storage-sweep' +import { + ACCOUNT_STORAGE_OBJECT_CLEANUP_EVENT, + ACCOUNT_WORKSPACE_STORAGE_SWEEP_EVENT, +} from '@/lib/users/storage-sweep-events' + +const sweep = accountStorageSweepOutboxHandlers[ACCOUNT_WORKSPACE_STORAGE_SWEEP_EVENT] +const cleanup = accountStorageSweepOutboxHandlers[ACCOUNT_STORAGE_OBJECT_CLEANUP_EVENT] + +let root: string + +function outboxContext(overrides: Partial = {}): OutboxEventContext { + return { + eventId: 'event-1', + eventType: ACCOUNT_WORKSPACE_STORAGE_SWEEP_EVENT, + attempts: 0, + maxAttempts: 10, + signal: new AbortController().signal, + checkpointPayload: vi.fn(async () => {}), + ...overrides, + } +} + +async function put(key: string, body = 'x'): Promise { + await mkdir(dirname(join(root, key)), { recursive: true }) + await writeFile(join(root, key), body) +} + +async function exists(key: string): Promise { + try { + await stat(join(root, key)) + return true + } catch { + return false + } +} + +const DOOMED_KEYS = [ + 'workspace/ws1/a.txt', + 'workspace/ws1/nested/b.txt', + 'table-import/ws1/import-1/rows.csv', + 'execution/ws1/wf/ex/large-value-lv_1.json', + 'table-snapshots/ws1/table-1/v1-hash.csv', + 'copilot-doc-compiled/ws1/hash.pdf', + 'workspace-logos/ws1/logo.png', +] + +const NEIGHBOR_KEYS = [ + 'workspace/ws10/keep.txt', + 'workspace/ws1-archive/keep.txt', + 'execution/ws10/wf/ex/keep.json', + 'project/ws1/keep.txt', + 'kb/ws1/keep.txt', +] + +describe('deleted workspace storage sweep', () => { + beforeEach(async () => { + resetDbChainMock() + root = await mkdtemp(join(tmpdir(), 'sim-account-sweep-')) + setUploadDirServer(root) + for (const key of [...DOOMED_KEYS, ...NEIGHBOR_KEYS]) await put(key) + }) + + afterEach(async () => { + await rm(root, { recursive: true, force: true }) + }) + + it('deletes every family under the workspace prefix and nothing under a neighboring id', async () => { + await expect(sweep({ version: 1, workspaceId: 'ws1' }, outboxContext())).resolves.toBe( + undefined + ) + + for (const key of DOOMED_KEYS) expect(await exists(key)).toBe(false) + for (const key of NEIGHBOR_KEYS) expect(await exists(key)).toBe(true) + expect(await exists('workspace/ws1')).toBe(false) + }) + + it('touches nothing while a workspace row with that id exists', async () => { + queueTableRows(schemaMock.workspace, [{ id: 'ws1' }]) + + await expect(sweep({ version: 1, workspaceId: 'ws1' }, outboxContext())).resolves.toBe( + undefined + ) + + for (const key of DOOMED_KEYS) expect(await exists(key)).toBe(true) + }) + + it('keeps an object a surviving row still references', async () => { + queueTableRows(schemaMock.document, [{ key: 'workspace/ws1/a.txt' }]) + + await sweep({ version: 1, workspaceId: 'ws1' }, outboxContext()) + + expect(await exists('workspace/ws1/a.txt')).toBe(true) + expect(await exists('workspace/ws1/nested/b.txt')).toBe(false) + }) + + it('walks past a full listing page within one run', async () => { + const keys = Array.from({ length: 510 }, (_, index) => `workspace/ws1/bulk/${index}.bin`) + for (const key of keys) await put(key) + + await expect(sweep({ version: 1, workspaceId: 'ws1' }, outboxContext())).resolves.toBe( + undefined + ) + + for (const key of keys) expect(await exists(key)).toBe(false) + }) + + it('resumes from its checkpointed family', async () => { + await sweep({ version: 1, workspaceId: 'ws1', family: 2 }, outboxContext()) + + expect(await exists('workspace/ws1/a.txt')).toBe(true) + expect(await exists('table-import/ws1/import-1/rows.csv')).toBe(true) + expect(await exists('execution/ws1/wf/ex/large-value-lv_1.json')).toBe(false) + }) + + it.each([ + { version: 1, workspaceId: '' }, + { version: 1, workspaceId: '../ws1' }, + { version: 1, workspaceId: 'ws1/' }, + { version: 1, workspaceId: '*' }, + { workspaceId: 'ws1' }, + ])('rejects an unbounded or malformed payload %o', async (payload) => { + await expect(sweep(payload, outboxContext())).rejects.toThrow() + for (const key of DOOMED_KEYS) expect(await exists(key)).toBe(true) + }) +}) + +describe('account storage object cleanup', () => { + beforeEach(async () => { + resetDbChainMock() + root = await mkdtemp(join(tmpdir(), 'sim-account-objects-')) + setUploadDirServer(root) + }) + + afterEach(async () => { + await rm(root, { recursive: true, force: true }) + }) + + const payload = (objects: { key: string; context: string }[]) => ({ + version: 1, + userId: 'user-1', + objects, + }) + + it('deletes unreferenced objects and keeps referenced ones', async () => { + await put('copilot/abc/private.txt') + await put('workspace/ws2/handed-off.txt') + queueTableRows(schemaMock.workspaceFiles, [{ key: 'workspace/ws2/handed-off.txt' }]) + + await cleanup( + payload([ + { key: 'copilot/abc/private.txt', context: 'copilot' }, + { key: 'workspace/ws2/handed-off.txt', context: 'workspace' }, + { key: 'copilot/abc/already-gone.txt', context: 'copilot' }, + ]), + outboxContext() + ) + + expect(await exists('copilot/abc/private.txt')).toBe(false) + expect(await readFile(join(root, 'workspace/ws2/handed-off.txt'), 'utf8')).toBe('x') + }) + + it('touches nothing while the account exists', async () => { + await put('copilot/abc/private.txt') + queueTableRows(schemaMock.user, [{ id: 'user-1' }]) + + await cleanup( + payload([{ key: 'copilot/abc/private.txt', context: 'copilot' }]), + outboxContext() + ) + + expect(await exists('copilot/abc/private.txt')).toBe(true) + }) + + it('fails so the outbox retries when a deletion fails, then completes', async () => { + await mkdir(join(root, 'copilot/abc/blocked.txt/child'), { recursive: true }) + const objects = [{ key: 'copilot/abc/blocked.txt', context: 'copilot' }] + + await expect(cleanup(payload(objects), outboxContext())).rejects.toThrow() + + await rm(join(root, 'copilot/abc/blocked.txt'), { recursive: true }) + await put('copilot/abc/blocked.txt') + await cleanup(payload(objects), outboxContext()) + expect(await exists('copilot/abc/blocked.txt')).toBe(false) + }) + + it.each([ + { key: 'project/p1/file.txt', context: 'workspace' }, + { key: 'assistant/org/user/file.png', context: 'mothership' }, + { key: '../outside.txt', context: 'workspace' }, + { key: 'workspace/ws2/file.txt', context: 'og-images' }, + ])('refuses a batch naming %o', async (object) => { + await expect(cleanup(payload([object]), outboxContext())).rejects.toThrow( + 'Invalid account storage cleanup object' + ) + }) +}) diff --git a/apps/sim/lib/users/storage-sweep.ts b/apps/sim/lib/users/storage-sweep.ts new file mode 100644 index 00000000000..db58b42d38f --- /dev/null +++ b/apps/sim/lib/users/storage-sweep.ts @@ -0,0 +1,618 @@ +import { db } from '@sim/db' +import { + copilotChats, + document, + executionLargeValues, + knowledgeBase, + user, + workspaceFile, + workspaceFiles, + workspaceFileVersion, + workspace as workspaceTable, +} from '@sim/db/schema' +import { createLogger } from '@sim/logger' +import { describeError } from '@sim/utils/errors' +import { chunkArray } from '@sim/utils/helpers' +import { isRecordLike } from '@sim/utils/object' +import { + and, + eq, + gt, + inArray, + isNotNull, + isNull, + notInArray, + or, + type SQL, + type SQLWrapper, + sql, +} from 'drizzle-orm' +import { + continueOutboxHandler, + deferOutboxHandler, + enqueueOutboxEvents, + MAX_BULK_ENQUEUE_EVENTS, + type OutboxEventContext, + type OutboxHandler, + type OutboxHandlerRegistry, +} from '@/lib/core/outbox/service' +import type { DbTransaction } from '@/lib/db/types' +import { + deleteStorageKeysUnderPrefix, + listStorageKeyPage, + storageCleanupAvailableAt, +} from '@/lib/projects/files/prefix-cleanup' +import type { StorageContext } from '@/lib/uploads/config' +import { deleteFile } from '@/lib/uploads/core/storage-service' +import { + ACCOUNT_STORAGE_OBJECT_CLEANUP_EVENT, + ACCOUNT_WORKSPACE_STORAGE_SWEEP_EVENT, +} from '@/lib/users/storage-sweep-events' + +const logger = createLogger('AccountStorageSweep') + +const ROW_PAGE_SIZE = 1000 +const OBJECTS_PER_EVENT = 100 +const DEADLINE_MARGIN_MS = 5000 + +/** + * Ceiling on exact keys one deletion records for durable cleanup. Past it the rest are logged + * and left; the workspace prefix sweeps are unaffected, since they list storage instead. + */ +const MAX_DURABLE_OBJECTS = 100_000 + +const SAFE_ID = /^[A-Za-z0-9_-]{1,128}$/ + +/** + * Every storage family whose keys embed their workspace as the second path segment, in the + * bucket that serves it. A deleted workspace's objects in these families are found by listing, + * so neither a missing row nor a row written after the deletion read its keys can strand one. + */ +const WORKSPACE_STORAGE_FAMILIES = [ + { root: 'workspace', context: 'workspace' }, + { root: 'table-import', context: 'table-import' }, + { root: 'execution', context: 'execution' }, + { root: 'table-snapshots', context: 'execution' }, + { root: 'copilot-doc-compiled', context: 'copilot' }, + { root: 'workspace-logos', context: 'workspace-logos' }, +] as const satisfies readonly { root: string; context: StorageContext }[] + +/** + * Mirrors the contexts `reassignSharedResourceOwnershipForWorkspaceMemberRemovalTx` hands to a + * successor. Drift toward fewer contexts only queues keys the reference guard then keeps. + */ +const HANDED_OFF_FILE_CONTEXTS = [ + 'workspace', + 'knowledge-base', + 'execution', + 'chat', + 'workspace-logos', +] as const + +const OBJECT_CONTEXTS: ReadonlySet = new Set([ + 'workspace', + 'mothership', + 'copilot', + 'chat', + 'knowledge-base', + 'execution', + 'table-import', + 'workspace-logos', + 'profile-pictures', +]) + +/** Keys another lifecycle owns: Project prefix cleanup and the Assistant upload-session sweep. */ +const FOREIGN_LIFECYCLE_ROOTS = ['project/', 'assistant/'] as const + +interface StorageObject { + key: string + context: StorageContext +} + +interface WorkspaceSweepPayload { + workspaceId: string + family: number +} + +interface ObjectCleanupPayload { + userId: string + objects: StorageObject[] + cursor: number +} + +/** The trailing slash keeps `workspace/ws1/` from matching `workspace/ws10/...`. */ +function workspaceStoragePrefix(root: string, workspaceId: string): string { + if (!SAFE_ID.test(workspaceId)) throw new Error('Invalid workspace storage identity') + return `${root}/${workspaceId}/` +} + +function deadlineNear(context: Pick): boolean { + return Boolean(context.deadlineAt && Date.now() + DEADLINE_MARGIN_MS >= context.deadlineAt) +} + +function parseWorkspaceSweep(value: unknown): WorkspaceSweepPayload { + if (!isRecordLike(value) || value.version !== 1 || typeof value.workspaceId !== 'string') + throw new Error('Invalid workspace storage sweep payload') + workspaceStoragePrefix('workspace', value.workspaceId) + const family = value.family === undefined ? 0 : value.family + if ( + typeof family !== 'number' || + !Number.isSafeInteger(family) || + family < 0 || + family > WORKSPACE_STORAGE_FAMILIES.length + ) + throw new Error('Invalid workspace storage sweep cursor') + return { workspaceId: value.workspaceId, family } +} + +function isDisposableKey(key: string): boolean { + return ( + key.length > 0 && + key.length <= 2048 && + !key.startsWith('/') && + !key.split('/').includes('..') && + !FOREIGN_LIFECYCLE_ROOTS.some((root) => key.startsWith(root)) + ) +} + +function parseObjectCleanup(value: unknown): ObjectCleanupPayload { + if ( + !isRecordLike(value) || + value.version !== 1 || + typeof value.userId !== 'string' || + !SAFE_ID.test(value.userId) || + !Array.isArray(value.objects) || + value.objects.length > OBJECTS_PER_EVENT + ) + throw new Error('Invalid account storage cleanup payload') + const objects = value.objects.map((object): StorageObject => { + if ( + !isRecordLike(object) || + typeof object.key !== 'string' || + !isDisposableKey(object.key) || + typeof object.context !== 'string' || + !OBJECT_CONTEXTS.has(object.context) + ) + throw new Error('Invalid account storage cleanup object') + return { key: object.key, context: object.context as StorageContext } + }) + const cursor = value.cursor === undefined ? 0 : value.cursor + if ( + typeof cursor !== 'number' || + !Number.isSafeInteger(cursor) || + cursor < 0 || + cursor > objects.length + ) + throw new Error('Invalid account storage cleanup cursor') + return { userId: value.userId, objects, cursor } +} + +/** + * Keys some surviving row still names. Every row of a deleted workspace or account is gone by + * the time a sweep runs, so a remaining reference belongs to data that outlived the deletion — + * a moved knowledge base, a handed-off file, a soft-deleted file elsewhere — and its object stays. + */ +async function findReferencedStorageKeys(keys: readonly string[]): Promise> { + const referenced = new Set() + if (keys.length === 0) return referenced + const list = [...keys] + const rows = [ + ...(await db + .select({ key: workspaceFiles.key }) + .from(workspaceFiles) + .where(inArray(workspaceFiles.key, list))), + ...(await db + .select({ key: workspaceFile.key }) + .from(workspaceFile) + .where(inArray(workspaceFile.key, list))), + ...(await db + .select({ key: workspaceFileVersion.key }) + .from(workspaceFileVersion) + .where(inArray(workspaceFileVersion.key, list))), + ...(await db + .select({ key: document.storageKey }) + .from(document) + .where(inArray(document.storageKey, list))), + ...(await db + .select({ key: executionLargeValues.key }) + .from(executionLargeValues) + .where(inArray(executionLargeValues.key, list))), + ] + for (const row of rows) if (row.key) referenced.add(row.key) + return referenced +} + +/** + * Walks one prefix to its end, deleting every listed object no surviving row references. Cloud + * listings follow the provider cursor within a run; a local listing restarts from the first key + * still on disk, so a page made only of kept keys ends the walk rather than looping on it. + */ +async function sweepWorkspacePrefix( + prefix: string, + storageContext: StorageContext, + context: OutboxEventContext +): Promise<{ deleted: number; complete: boolean }> { + let deleted = 0 + let pageToken: string | undefined + for (;;) { + if (deadlineNear(context)) return { deleted, complete: false } + const page = await listStorageKeyPage(prefix, storageContext, context.signal, pageToken) + if (page.keys.some((key) => !key.startsWith(prefix))) + throw new Error('Storage provider returned a key outside the workspace prefix') + const referenced = await findReferencedStorageKeys(page.keys) + if (referenced.size > 0) + logger.warn('Keeping objects a surviving row still references', { + prefix, + kept: referenced.size, + }) + const result = await deleteStorageKeysUnderPrefix( + prefix, + storageContext, + page.keys.filter((key) => !referenced.has(key)), + context + ) + deleted += result.deleted + if (!result.complete) return { deleted, complete: false } + if (page.done) return { deleted, complete: true } + if (page.nextPageToken) { + pageToken = page.nextPageToken + } else if (result.deleted === 0) { + logger.error('Workspace storage sweep stopped at a page of referenced objects', { prefix }) + return { deleted, complete: true } + } + } +} + +/** + * Deletes a deleted workspace's objects by prefix in each bucket. A workspace row with the same + * id means the database no longer agrees the workspace is gone (a restore, a point-in-time + * recovery), so the sweep completes without touching storage. + */ +const sweepWorkspaceStorage: OutboxHandler = async (raw, context) => { + const payload = parseWorkspaceSweep(raw) + const [existing] = await db + .select({ id: workspaceTable.id }) + .from(workspaceTable) + .where(eq(workspaceTable.id, payload.workspaceId)) + .limit(1) + if (existing) { + logger.error('Refusing to sweep storage for a workspace that exists', { + workspaceId: payload.workspaceId, + }) + return + } + let progressed = false + for (let family = payload.family; family < WORKSPACE_STORAGE_FAMILIES.length; family++) { + const { root, context: storageContext } = WORKSPACE_STORAGE_FAMILIES[family] + const prefix = workspaceStoragePrefix(root, payload.workspaceId) + const result = await sweepWorkspacePrefix(prefix, storageContext, context) + if (result.deleted > 0) progressed = true + if (!result.complete) { + return progressed + ? continueOutboxHandler('Continuing workspace storage sweep') + : deferOutboxHandler('Workspace storage sweep made no progress before its deadline') + } + await context.checkpointPayload({ family: family + 1 }) + progressed = true + } +} + +/** + * Deletes exact keys recorded by an account deletion. The account reappearing refuses the batch + * as a whole; a key any surviving row references is kept. + */ +const cleanupAccountObjects: OutboxHandler = async (raw, context) => { + const payload = parseObjectCleanup(raw) + const [existing] = await db + .select({ id: user.id }) + .from(user) + .where(eq(user.id, payload.userId)) + .limit(1) + if (existing) { + logger.error('Refusing to delete storage for an account that exists', { + userId: payload.userId, + }) + return + } + const pending = payload.objects.slice(payload.cursor) + const referenced = await findReferencedStorageKeys(pending.map((object) => object.key)) + if (referenced.size > 0) + logger.warn('Keeping objects a surviving row still references', { + userId: payload.userId, + kept: referenced.size, + }) + for (let index = payload.cursor; index < payload.objects.length; index++) { + context.signal.throwIfAborted() + if (index > payload.cursor && deadlineNear(context)) { + await context.checkpointPayload({ cursor: index }) + return continueOutboxHandler('Continuing account storage cleanup') + } + const { key, context: storageContext } = payload.objects[index] + if (referenced.has(key)) continue + try { + await deleteFile({ key, context: storageContext, signal: context.signal }) + } catch (error) { + if (describeError(error).code !== 'ENOENT') throw error + } + } +} + +export const accountStorageSweepOutboxHandlers = { + [ACCOUNT_WORKSPACE_STORAGE_SWEEP_EVENT]: sweepWorkspaceStorage, + [ACCOUNT_STORAGE_OBJECT_CLEANUP_EVENT]: cleanupAccountObjects, +} satisfies OutboxHandlerRegistry + +/** True when `key` sits under one of `workspaceId`'s swept prefixes. */ +function coveredByWorkspaceSweep(key: SQLWrapper, workspaceId: SQLWrapper): SQL { + return sql`(${sql.join( + WORKSPACE_STORAGE_FAMILIES.map( + ({ root }) => sql`starts_with(${key}, ${`${root}/`} || ${workspaceId} || '/')` + ), + sql` OR ` + )})` +} + +interface ObjectRow { + id: string + key: string | null + context: string | null +} + +/** Batches recorded objects into delayed cleanup events inside the deletion transaction. */ +class ObjectRecorder { + private recorded = 0 + private skipped = 0 + private capped = false + + constructor( + private readonly tx: DbTransaction, + private readonly userId: string, + private readonly availableAt: Date + ) {} + + get full(): boolean { + return this.capped + } + + async record(rows: readonly ObjectRow[]): Promise { + const objects: StorageObject[] = [] + for (const row of rows) { + if (!row.key || !isDisposableKey(row.key)) continue + if (!row.context || !OBJECT_CONTEXTS.has(row.context)) { + this.skipped++ + continue + } + if (this.recorded + objects.length >= MAX_DURABLE_OBJECTS) { + this.capped = true + break + } + objects.push({ key: row.key, context: row.context as StorageContext }) + } + this.recorded += objects.length + const payloads = chunkArray(objects, OBJECTS_PER_EVENT).map((chunk) => ({ + version: 1, + userId: this.userId, + objects: chunk, + })) + for (const batch of chunkArray(payloads, MAX_BULK_ENQUEUE_EVENTS)) { + await enqueueOutboxEvents(this.tx, ACCOUNT_STORAGE_OBJECT_CLEANUP_EVENT, batch, { + availableAt: this.availableAt, + }) + } + } + + summary() { + return { recorded: this.recorded, skippedContexts: this.skipped, capped: this.capped } + } +} + +async function recordPages( + recorder: ObjectRecorder, + page: (afterId: string) => Promise, + afterPage?: (rows: ObjectRow[]) => Promise +): Promise { + let afterId = '' + while (!recorder.full) { + const rows = await page(afterId) + if (rows.length === 0) return + await recorder.record(rows) + if (afterPage) await afterPage(rows) + if (rows.length < ROW_PAGE_SIZE) return + afterId = rows[rows.length - 1].id + } +} + +async function recordVersionsOf( + tx: DbTransaction, + recorder: ObjectRecorder, + files: readonly ObjectRow[] +): Promise { + if (files.length === 0 || recorder.full) return + const contexts = new Map(files.map((file) => [file.id, file.context])) + const versions = await tx + .select({ + id: workspaceFileVersion.id, + key: workspaceFileVersion.key, + fileId: workspaceFileVersion.fileId, + }) + .from(workspaceFileVersion) + .where( + inArray( + workspaceFileVersion.fileId, + files.map((file) => file.id) + ) + ) + await recorder.record( + versions.map((version) => ({ + id: version.id, + key: version.key, + context: contexts.get(version.fileId) ?? null, + })) + ) +} + +/** + * Queues the durable storage cleanup for an account deletion, inside its transaction and after + * the workspace rows are locked, so the work commits or rolls back with the deletion and no row + * can join a doomed workspace unseen. Takes no locks of its own. + * + * - One prefix sweep per deleted workspace, covering every object under its keyed families + * whether or not a row ever named it. + * - Exact keys for the rest of what cascades away: objects in deleted workspaces outside those + * prefixes (knowledge-base documents, Copilot uploads), and the account's private files + * elsewhere — chat attachments of its own chats, private contexts, and files with no + * workspace. Files handed to a successor, Project files, organization files, and attachments + * of somebody else's chat are never queued. + * + * Both wait out every upload URL signed before the deletion, and both re-check at run time that + * the owner is still gone and that no surviving row references a key before deleting it. + */ +export async function enqueueAccountStorageCleanupInTx( + tx: DbTransaction, + params: { userId: string; workspaceIds: readonly string[] } +): Promise { + const { userId } = params + const availableAt = storageCleanupAvailableAt() + const workspaceIds = params.workspaceIds.filter((id) => SAFE_ID.test(id)) + if (workspaceIds.length !== params.workspaceIds.length) { + logger.error('Skipping storage sweeps for workspaces with unsafe ids', { + userId, + skipped: params.workspaceIds.length - workspaceIds.length, + }) + } + for (const batch of chunkArray(workspaceIds, MAX_BULK_ENQUEUE_EVENTS)) { + await enqueueOutboxEvents( + tx, + ACCOUNT_WORKSPACE_STORAGE_SWEEP_EVENT, + batch.map((workspaceId) => ({ version: 1, workspaceId })), + { availableAt } + ) + } + + const recorder = new ObjectRecorder(tx, userId, availableAt) + if (workspaceIds.length > 0) { + await recordPages(recorder, (afterId) => + tx + .select({ id: workspaceFiles.id, key: workspaceFiles.key, context: workspaceFiles.context }) + .from(workspaceFiles) + .where( + and( + inArray(workspaceFiles.workspaceId, workspaceIds), + isNull(workspaceFiles.projectId), + sql`NOT ${coveredByWorkspaceSweep(workspaceFiles.key, workspaceFiles.workspaceId)}`, + gt(workspaceFiles.id, afterId) + ) + ) + .orderBy(workspaceFiles.id) + .limit(ROW_PAGE_SIZE) + ) + await recordPages(recorder, (afterId) => + tx + .select({ + id: workspaceFileVersion.id, + key: workspaceFileVersion.key, + context: workspaceFiles.context, + }) + .from(workspaceFileVersion) + .innerJoin(workspaceFiles, eq(workspaceFiles.id, workspaceFileVersion.fileId)) + .where( + and( + inArray(workspaceFileVersion.workspaceId, workspaceIds), + isNull(workspaceFiles.projectId), + sql`NOT ${coveredByWorkspaceSweep(workspaceFileVersion.key, workspaceFileVersion.workspaceId)}`, + gt(workspaceFileVersion.id, afterId) + ) + ) + .orderBy(workspaceFileVersion.id) + .limit(ROW_PAGE_SIZE) + ) + await recordPages(recorder, (afterId) => + tx + .select({ id: workspaceFile.id, key: workspaceFile.key, context: sql`'workspace'` }) + .from(workspaceFile) + .where( + and( + inArray(workspaceFile.workspaceId, workspaceIds), + sql`NOT ${coveredByWorkspaceSweep(workspaceFile.key, workspaceFile.workspaceId)}`, + gt(workspaceFile.id, afterId) + ) + ) + .orderBy(workspaceFile.id) + .limit(ROW_PAGE_SIZE) + ) + await recordPages(recorder, (afterId) => + tx + .select({ + id: document.id, + key: document.storageKey, + context: sql`'knowledge-base'`, + }) + .from(document) + .innerJoin(knowledgeBase, eq(knowledgeBase.id, document.knowledgeBaseId)) + .where( + and( + inArray(knowledgeBase.workspaceId, workspaceIds), + isNotNull(document.storageKey), + sql`NOT ${coveredByWorkspaceSweep(document.storageKey, knowledgeBase.workspaceId)}`, + gt(document.id, afterId) + ) + ) + .orderBy(document.id) + .limit(ROW_PAGE_SIZE) + ) + } + + await recordPages( + recorder, + (afterId) => + tx + .select({ id: workspaceFiles.id, key: workspaceFiles.key, context: workspaceFiles.context }) + .from(workspaceFiles) + .where( + and( + eq(workspaceFiles.userId, userId), + isNull(workspaceFiles.projectId), + isNull(workspaceFiles.organizationId), + workspaceIds.length > 0 + ? or( + isNull(workspaceFiles.workspaceId), + notInArray(workspaceFiles.workspaceId, workspaceIds) + ) + : undefined, + or( + and( + isNull(workspaceFiles.chatId), + or( + isNull(workspaceFiles.workspaceId), + notInArray(workspaceFiles.context, [...HANDED_OFF_FILE_CONTEXTS]) + ) + ), + inArray( + workspaceFiles.chatId, + tx + .select({ id: copilotChats.id }) + .from(copilotChats) + .where(eq(copilotChats.userId, userId)) + ) + ), + gt(workspaceFiles.id, afterId) + ) + ) + .orderBy(workspaceFiles.id) + .limit(ROW_PAGE_SIZE), + (files) => recordVersionsOf(tx, recorder, files) + ) + + const summary = recorder.summary() + if (summary.capped) { + logger.error('Account storage cleanup hit its key ceiling; the remainder is left', { + userId, + cap: MAX_DURABLE_OBJECTS, + }) + } + logger.info('Queued account storage cleanup', { + userId, + workspaceSweeps: workspaceIds.length, + ...summary, + }) +} From d4475f04a34e31c14700c354422584f90ae2f520 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Fri, 16 Oct 2026 15:20:21 -0700 Subject: [PATCH 02/11] 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. --- .../lib/projects/files/prefix-cleanup.test.ts | 79 ++++ apps/sim/lib/projects/files/prefix-cleanup.ts | 30 +- .../account-deletion-attachments.test.ts | 12 + apps/sim/lib/users/account-deletion.ts | 23 +- .../lib/users/storage-sweep.integration.ts | 26 +- apps/sim/lib/users/storage-sweep.test.ts | 155 +++++++- apps/sim/lib/users/storage-sweep.ts | 346 +++++++++++++----- 7 files changed, 567 insertions(+), 104 deletions(-) create mode 100644 apps/sim/lib/projects/files/prefix-cleanup.test.ts diff --git a/apps/sim/lib/projects/files/prefix-cleanup.test.ts b/apps/sim/lib/projects/files/prefix-cleanup.test.ts new file mode 100644 index 00000000000..0305976c60a --- /dev/null +++ b/apps/sim/lib/projects/files/prefix-cleanup.test.ts @@ -0,0 +1,79 @@ +import { mkdir, mkdtemp, rm, stat, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { dirname, join } from 'node:path' +import { resetDbChainMock } from '@sim/testing' +import { uploadsConfigMock } from '@sim/testing/mocks/uploads-config.mock' +import { uploadsMetadataMock } from '@sim/testing/mocks/uploads-metadata.mock' +import { setUploadDirServer, uploadsSetupMock } from '@sim/testing/mocks/uploads-setup.mock' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +vi.mock('@/lib/uploads/core/setup.server', () => uploadsSetupMock) +vi.mock('@/lib/uploads/config', () => uploadsConfigMock) +vi.mock('@/lib/uploads/server/metadata', () => uploadsMetadataMock) + +import type { OutboxEventContext } from '@/lib/core/outbox/service' +import { PROJECT_FILE_PREFIX_CLEANUP_EVENT } from '@/lib/projects/files/outbox-events' +import { projectFilePrefixCleanupOutboxHandlers } from '@/lib/projects/files/prefix-cleanup' + +const cleanup = projectFilePrefixCleanupOutboxHandlers[PROJECT_FILE_PREFIX_CLEANUP_EVENT] + +let root: string + +function outboxContext(deadlineAt?: number): OutboxEventContext { + return { + eventId: 'event-1', + eventType: PROJECT_FILE_PREFIX_CLEANUP_EVENT, + attempts: 0, + maxAttempts: 10, + signal: new AbortController().signal, + ...(deadlineAt ? { deadlineAt } : {}), + checkpointPayload: vi.fn(async () => {}), + } +} + +async function put(key: string): Promise { + await mkdir(dirname(join(root, key)), { recursive: true }) + await writeFile(join(root, key), 'x') +} + +async function exists(key: string): Promise { + try { + await stat(join(root, key)) + return true + } catch { + return false + } +} + +describe('Project prefix cleanup on local storage', () => { + beforeEach(async () => { + resetDbChainMock() + root = await mkdtemp(join(tmpdir(), 'sim-project-prefix-')) + setUploadDirServer(root) + }) + + afterEach(async () => { + await rm(root, { recursive: true, force: true }) + }) + + it('deletes the retired Project prefix and completes', async () => { + await put('project/p1/a.txt') + await put('project/p1/nested/b.txt') + await put('project/p10/keep.txt') + + await expect(cleanup({ projectId: 'p1' }, outboxContext())).resolves.toBeUndefined() + + expect(await exists('project/p1/a.txt')).toBe(false) + expect(await exists('project/p1/nested/b.txt')).toBe(false) + expect(await exists('project/p10/keep.txt')).toBe(true) + }) + + it('spends an attempt when a full page holds only objects it cannot remove', async () => { + for (let index = 0; index < 520; index++) await put(`project/p1/legacy file ${index}.txt`) + + const result = await cleanup({ projectId: 'p1' }, outboxContext(Date.now() + 60_000)) + + expect(result).toMatchObject({ outcome: 'deferred' }) + expect(result).not.toMatchObject({ consumeAttempt: false }) + }) +}) diff --git a/apps/sim/lib/projects/files/prefix-cleanup.ts b/apps/sim/lib/projects/files/prefix-cleanup.ts index ea352b2ee2a..6997cec99c5 100644 --- a/apps/sim/lib/projects/files/prefix-cleanup.ts +++ b/apps/sim/lib/projects/files/prefix-cleanup.ts @@ -7,6 +7,7 @@ import { isRecordLike } from '@sim/utils/object' import { and, asc, eq, gt, inArray, isNull, sql } from 'drizzle-orm' import { continueOutboxHandler, + deferOutboxHandler, enqueueOutboxEvent, type OutboxEventContext, type OutboxHandler, @@ -332,7 +333,9 @@ export async function listStorageKeyPage( /** * Deletes listed keys, each of which must sit under `prefix`, and prunes local directories the * deletions empty, never above the prefix itself. Stops before another deletion once the handler - * deadline is near; `complete` is false when it did. + * deadline is near; `complete` is false when it did. `deleted` counts only objects actually + * removed: a key already gone (or, locally, one whose on-disk name no storage key resolves to) is + * no progress, so a caller re-listing the same page can tell that it is stuck. */ export async function deleteStorageKeysUnderPrefix( prefix: string, @@ -341,17 +344,21 @@ export async function deleteStorageKeysUnderPrefix( outbox: Pick ): Promise<{ deleted: number; complete: boolean }> { if (prefix.length < 2 || !prefix.endsWith('/')) throw new Error('Unbounded storage prefix') + let attempted = 0 let deleted = 0 for (const key of keys) { if (!key.startsWith(prefix)) throw new Error('Storage provider returned a key outside the prefix') outbox.signal.throwIfAborted() - if (deleted > 0 && outbox.deadlineAt && Date.now() + 5000 >= outbox.deadlineAt) + if (attempted > 0 && outbox.deadlineAt && Date.now() + 5000 >= outbox.deadlineAt) return { deleted, complete: false } + attempted++ + let removed = true try { await deleteFile({ key, context, signal: outbox.signal }) } catch (error) { if (describeError(error).code !== 'ENOENT') throw error + removed = false } if (!USE_S3_STORAGE && !USE_BLOB_STORAGE && !USE_GCS_STORAGE) { const root = join(UPLOAD_DIR_SERVER, prefix.slice(0, -1)) @@ -361,16 +368,21 @@ export async function deleteStorageKeysUnderPrefix( directory = dirname(directory) } } - deleted++ + if (removed) deleted++ } return { deleted, complete: true } } -async function sweepPrefix(projectId: string, context: OutboxEventContext): Promise { +/** `stalled` when a page that must be re-listed removed nothing, so continuing cannot progress. */ +async function sweepPrefix( + projectId: string, + context: OutboxEventContext +): Promise<'done' | 'continue' | 'stalled'> { const prefix = prefixFor(projectId) const { keys } = await listStorageKeyPage(prefix, 'project', context.signal) - const { complete } = await deleteStorageKeysUnderPrefix(prefix, 'project', keys, context) - return complete && keys.length < PAGE_SIZE + const { deleted, complete } = await deleteStorageKeysUnderPrefix(prefix, 'project', keys, context) + if (complete && keys.length < PAGE_SIZE) return 'done' + return deleted > 0 ? 'continue' : 'stalled' } const cleanupPrefix: OutboxHandler = async (raw, context) => { @@ -410,8 +422,10 @@ const cleanupPrefix: OutboxHandler = async (raw, context) => { return continueOutboxHandler('Continuing retired Project upload cleanup') } } - if (!(await sweepPrefix(payload.projectId, context))) - return continueOutboxHandler('Continuing Project storage cleanup') + const sweep = await sweepPrefix(payload.projectId, context) + if (sweep === 'continue') return continueOutboxHandler('Continuing Project storage cleanup') + if (sweep === 'stalled') + return deferOutboxHandler('Project storage cleanup removed nothing from a full listing page') } /** diff --git a/apps/sim/lib/users/account-deletion-attachments.test.ts b/apps/sim/lib/users/account-deletion-attachments.test.ts index ff37d00302a..61f43e3fb99 100644 --- a/apps/sim/lib/users/account-deletion-attachments.test.ts +++ b/apps/sim/lib/users/account-deletion-attachments.test.ts @@ -90,6 +90,18 @@ describe('account deletion of private organization Assistant images', () => { } ) + it('keeps an object a surviving row still references', async () => { + queueTableRows(schemaMock.uploadSession, [ + { id: 'upload-1', key: IMAGE_KEY }, + { id: 'upload-2', key: FAILED_IMAGE_KEY }, + ]) + queueTableRows(schemaMock.document, [{ key: IMAGE_KEY }]) + + await deleteUserAccount('user-1') + + expect(mockDeleteFiles).toHaveBeenCalledExactlyOnceWith([FAILED_IMAGE_KEY], 'mothership') + }) + it('retains ownership while an issued upload URL could recreate a purged object', async () => { queueTableRows(schemaMock.uploadSession, [{ id: 'upload-1', key: IMAGE_KEY }]) diff --git a/apps/sim/lib/users/account-deletion.ts b/apps/sim/lib/users/account-deletion.ts index 87b212ee4fe..cef801f5c37 100644 --- a/apps/sim/lib/users/account-deletion.ts +++ b/apps/sim/lib/users/account-deletion.ts @@ -54,7 +54,10 @@ import { } from '@/lib/table/rows/executions' import type { StorageContext } from '@/lib/uploads' import { isUsingCloudStorage, StorageService } from '@/lib/uploads' -import { enqueueAccountStorageCleanupInTx } from '@/lib/users/storage-sweep' +import { + createStorageReferenceGuard, + enqueueAccountStorageCleanupInTx, +} from '@/lib/users/storage-sweep' import { listSharedResourceWorkspaceIdsForUser, listUnresolvedSharedResourceWorkspaces, @@ -638,11 +641,25 @@ async function collectAccountStorageKeys( * time this runs the account is already gone, and the request must not report a * failure for work that cannot be undone. An orphaned object is recoverable from * the log; a deletion the caller believes failed is not. + * + * Keys a surviving row or stored asset URL still names are kept (a legacy `kb/` + * object can back documents in more than one workspace); see + * `createStorageReferenceGuard`. */ async function purgeStorageObjects(userId: string, batches: StorageKeyBatch[]): Promise { - for (const { context, keys } of batches) { - if (keys.length === 0) continue + const isReferenced = createStorageReferenceGuard() + for (const { context, keys: collected } of batches) { + if (collected.length === 0) continue try { + const referenced = await isReferenced(collected) + if (referenced.size > 0) { + logger.warn('Keeping objects a surviving reference still names', { + context, + kept: referenced.size, + }) + } + const keys = collected.filter((key) => !referenced.has(key)) + if (keys.length === 0) continue const { failed } = await StorageService.deleteFiles(keys, context) for (const { key, error } of failed) { logger.error('Failed to erase stored object during account deletion', { diff --git a/apps/sim/lib/users/storage-sweep.integration.ts b/apps/sim/lib/users/storage-sweep.integration.ts index 8fd3cbe6b60..5d9bb0a6d52 100644 --- a/apps/sim/lib/users/storage-sweep.integration.ts +++ b/apps/sim/lib/users/storage-sweep.integration.ts @@ -151,6 +151,9 @@ async function seed() { privateAttachment: `workspace/${survivingId}/${generateId()}/private.txt`, othersChatAttachment: `workspace/${survivingId}/${generateId()}/others.txt`, personalCopilot: `copilot/${generateId()}/personal.txt`, + legacyWorkspacelessKb: `kb/${generateId()}-legacy.txt`, + profilePictureRowLogo: `profile-pictures/${generateId()}/org-logo.png`, + logoNamedByUrl: `workspace-logos/${doomedId}/${generateId()}-org-logo.png`, } await db.insert(workspaceFiles).values([ fileRow({ @@ -197,7 +200,23 @@ async function seed() { workspaceId: null, context: 'copilot', }), + fileRow({ + key: keys.legacyWorkspacelessKb, + userId: departingId, + workspaceId: null, + context: 'knowledge-base', + }), + fileRow({ + key: keys.profilePictureRowLogo, + userId: departingId, + workspaceId: null, + context: 'profile-pictures', + }), ]) + await db + .update(user) + .set({ image: `/api/files/serve/${encodeURIComponent(keys.logoNamedByUrl)}` }) + .where(eq(user.id, ownerId)) for (const key of Object.values(keys)) await put(key) return { ownerId, departingId, doomedId, survivingId, keys } } @@ -259,6 +278,8 @@ describe('account deletion storage sweep', () => { ) expect(queuedKeys.map((object) => object.key)).not.toContain(f.keys.othersChatAttachment) expect(queuedKeys.map((object) => object.key)).not.toContain(f.keys.handedOff) + expect(queuedKeys.map((object) => object.key)).not.toContain(f.keys.legacyWorkspacelessKb) + expect(queuedKeys.map((object) => object.key)).not.toContain(f.keys.profilePictureRowLogo) const lateUpload = `workspace/${f.doomedId}/late-${generateId()}.bin` await put(lateUpload) @@ -270,7 +291,6 @@ describe('account deletion storage sweep', () => { f.keys.rowlessExecution, f.keys.rowlessSnapshot, f.keys.rowlessCompiled, - f.keys.privateAttachment, f.keys.personalCopilot, lateUpload, ]) @@ -280,6 +300,10 @@ describe('account deletion storage sweep', () => { f.keys.neighbor, f.keys.handedOff, f.keys.othersChatAttachment, + f.keys.privateAttachment, + f.keys.legacyWorkspacelessKb, + f.keys.profilePictureRowLogo, + f.keys.logoNamedByUrl, ]) expect(await readFile(join(storageRoot, key), 'utf8')).toBe('x') expect( diff --git a/apps/sim/lib/users/storage-sweep.test.ts b/apps/sim/lib/users/storage-sweep.test.ts index 81fa5bf6d48..e00ab19e33d 100644 --- a/apps/sim/lib/users/storage-sweep.test.ts +++ b/apps/sim/lib/users/storage-sweep.test.ts @@ -1,7 +1,13 @@ import { mkdir, mkdtemp, readFile, rm, stat, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { dirname, join } from 'node:path' -import { queueTableRows, resetDbChainMock, schemaMock } from '@sim/testing' +import { + dbChainMock, + dbChainMockFns, + queueTableRows, + resetDbChainMock, + schemaMock, +} from '@sim/testing' import { uploadsConfigMock } from '@sim/testing/mocks/uploads-config.mock' import { uploadsMetadataMock } from '@sim/testing/mocks/uploads-metadata.mock' import { setUploadDirServer, uploadsSetupMock } from '@sim/testing/mocks/uploads-setup.mock' @@ -12,7 +18,11 @@ vi.mock('@/lib/uploads/config', () => uploadsConfigMock) vi.mock('@/lib/uploads/server/metadata', () => uploadsMetadataMock) import type { OutboxEventContext } from '@/lib/core/outbox/service' -import { accountStorageSweepOutboxHandlers } from '@/lib/users/storage-sweep' +import type { DbTransaction } from '@/lib/db/types' +import { + accountStorageSweepOutboxHandlers, + enqueueAccountStorageCleanupInTx, +} from '@/lib/users/storage-sweep' import { ACCOUNT_STORAGE_OBJECT_CLEANUP_EVENT, ACCOUNT_WORKSPACE_STORAGE_SWEEP_EVENT, @@ -127,6 +137,67 @@ describe('deleted workspace storage sweep', () => { expect(await exists('execution/ws1/wf/ex/large-value-lv_1.json')).toBe(false) }) + it.each([ + { + name: 'an organization whitelabel logo serve URL', + table: 'organization', + row: { + logo: null, + whitelabel: { + logoUrl: 'https://sim.test/api/files/serve/s3/workspace-logos%2Fws1%2Flogo.png', + }, + metadata: null, + }, + }, + { + name: 'a custom block icon stored as a direct bucket URL', + table: 'customBlock', + row: { value: 'https://bucket.s3.amazonaws.com/workspace-logos/ws1/logo.png' }, + }, + { + name: 'a deployed chat avatar', + table: 'chat', + row: { value: { imageUrl: '/api/files/serve/workspace-logos/ws1/logo.png?context=chat' } }, + }, + { + name: 'another account image', + table: 'user', + row: { value: '/api/files/serve/workspace-logos/ws1/logo.png' }, + }, + ] as const)('keeps an object named by $name', async ({ table, row }) => { + queueTableRows(schemaMock[table], [row]) + + await sweep({ version: 1, workspaceId: 'ws1' }, outboxContext()) + + expect(await exists('workspace-logos/ws1/logo.png')).toBe(true) + expect(await exists('workspace/ws1/a.txt')).toBe(false) + }) + + it('keeps the local metadata sidecar of a kept object', async () => { + await put('workspace/ws1/a.txt.upload-metadata.json', '{}') + await put('workspace/ws1/nested/b.txt.upload-metadata.json', '{}') + queueTableRows(schemaMock.document, [{ key: 'workspace/ws1/a.txt' }]) + + await sweep({ version: 1, workspaceId: 'ws1' }, outboxContext()) + + expect(await exists('workspace/ws1/a.txt')).toBe(true) + expect(await exists('workspace/ws1/a.txt.upload-metadata.json')).toBe(true) + expect(await exists('workspace/ws1/nested/b.txt.upload-metadata.json')).toBe(false) + }) + + it('spends an attempt instead of continuing when a full local page removes nothing', async () => { + for (let index = 0; index < 520; index++) + await put(`workspace/ws1/legacy/legacy file ${index}.txt`) + + const result = await sweep( + { version: 1, workspaceId: 'ws1' }, + outboxContext({ deadlineAt: Date.now() + 5_500 }) + ) + + expect(result).toMatchObject({ outcome: 'deferred' }) + expect(result).not.toMatchObject({ consumeAttempt: false }) + }) + it.each([ { version: 1, workspaceId: '' }, { version: 1, workspaceId: '../ws1' }, @@ -198,14 +269,94 @@ describe('account storage object cleanup', () => { expect(await exists('copilot/abc/blocked.txt')).toBe(false) }) + it('keeps a key under a workspace that still exists', async () => { + await put('workspace/ws2/private.txt') + await put('copilot/abc/private.txt') + queueTableRows(schemaMock.workspace, [{ id: 'ws2' }]) + + await cleanup( + payload([ + { key: 'workspace/ws2/private.txt', context: 'mothership' }, + { key: 'copilot/abc/private.txt', context: 'copilot' }, + ]), + outboxContext() + ) + + expect(await exists('workspace/ws2/private.txt')).toBe(true) + expect(await exists('copilot/abc/private.txt')).toBe(false) + }) + + it('keeps a key a stored asset URL names', async () => { + await put('copilot/abc/icon.png') + queueTableRows(schemaMock.oauthClient, [{ value: '/api/files/serve/copilot%2Fabc%2Ficon.png' }]) + + await cleanup(payload([{ key: 'copilot/abc/icon.png', context: 'copilot' }]), outboxContext()) + + expect(await exists('copilot/abc/icon.png')).toBe(true) + }) + it.each([ { key: 'project/p1/file.txt', context: 'workspace' }, { key: 'assistant/org/user/file.png', context: 'mothership' }, { key: '../outside.txt', context: 'workspace' }, { key: 'workspace/ws2/file.txt', context: 'og-images' }, + { key: 'profile-pictures/user-9/logo.png', context: 'profile-pictures' }, + { key: 'profile-pictures/user-9/logo.png', context: 'workspace' }, + { key: 'organization-logos/org-1/logo.png', context: 'workspace' }, ])('refuses a batch naming %o', async (object) => { await expect(cleanup(payload([object]), outboxContext())).rejects.toThrow( 'Invalid account storage cleanup object' ) }) }) + +describe('queueing account storage cleanup', () => { + beforeEach(() => { + resetDbChainMock() + }) + + function queuedObjectKeys(): string[] { + return dbChainMockFns.values.mock.calls.flatMap(([rows]) => + (rows as { payload: { objects?: { key: string }[] } }[]).flatMap( + (row) => row.payload.objects?.map((object) => object.key) ?? [] + ) + ) + } + + it('queues every version of a private file across version pages', async () => { + queueTableRows(schemaMock.workspaceFiles, [ + { id: 'file-1', key: 'copilot/abc/file.txt', context: 'copilot' }, + ]) + const versionPage = (start: number, count: number) => + Array.from({ length: count }, (_, index) => ({ + id: `v-${String(start + index).padStart(5, '0')}`, + key: `copilot/abc/v${start + index}.txt`, + fileId: 'file-1', + })) + queueTableRows(schemaMock.workspaceFileVersion, versionPage(0, 1000)) + queueTableRows(schemaMock.workspaceFileVersion, versionPage(1000, 5)) + + await enqueueAccountStorageCleanupInTx(dbChainMock.db as unknown as DbTransaction, { + userId: 'user-1', + workspaceIds: [], + }) + + const keys = queuedObjectKeys() + expect(keys).toHaveLength(1006) + expect(keys).toContain('copilot/abc/v1004.txt') + }) + + it('never queues a profile picture row', async () => { + queueTableRows(schemaMock.workspaceFiles, [ + { id: 'file-1', key: 'profile-pictures/org-logo.png', context: 'profile-pictures' }, + { id: 'file-2', key: 'copilot/abc/file.txt', context: 'copilot' }, + ]) + + await enqueueAccountStorageCleanupInTx(dbChainMock.db as unknown as DbTransaction, { + userId: 'user-1', + workspaceIds: [], + }) + + expect(queuedObjectKeys()).toEqual(['copilot/abc/file.txt']) + }) +}) diff --git a/apps/sim/lib/users/storage-sweep.ts b/apps/sim/lib/users/storage-sweep.ts index db58b42d38f..dc292943c9f 100644 --- a/apps/sim/lib/users/storage-sweep.ts +++ b/apps/sim/lib/users/storage-sweep.ts @@ -1,9 +1,13 @@ import { db } from '@sim/db' import { + chat, copilotChats, + customBlock, document, executionLargeValues, knowledgeBase, + oauthClient, + organization, user, workspaceFile, workspaceFiles, @@ -21,6 +25,7 @@ import { inArray, isNotNull, isNull, + like, notInArray, or, type SQL, @@ -43,7 +48,9 @@ import { storageCleanupAvailableAt, } from '@/lib/projects/files/prefix-cleanup' import type { StorageContext } from '@/lib/uploads/config' +import { LOCAL_UPLOAD_METADATA_SUFFIX } from '@/lib/uploads/core/storage-key' import { deleteFile } from '@/lib/uploads/core/storage-service' +import { extractStorageKey } from '@/lib/uploads/utils/file' import { ACCOUNT_STORAGE_OBJECT_CLEANUP_EVENT, ACCOUNT_WORKSPACE_STORAGE_SWEEP_EVENT, @@ -98,11 +105,24 @@ const OBJECT_CONTEXTS: ReadonlySet = new Set([ 'execution', 'table-import', 'workspace-logos', - 'profile-pictures', ]) -/** Keys another lifecycle owns: Project prefix cleanup and the Assistant upload-session sweep. */ -const FOREIGN_LIFECYCLE_ROOTS = ['project/', 'assistant/'] as const +/** + * Keys another lifecycle owns: Project prefix cleanup, the Assistant upload-session sweep, + * organization logos, and profile pictures — which account deletion takes only from the + * account's own `user.image`, because whitelabel logos were once uploaded as `profile-pictures` + * rows under an admin's account. + */ +const FOREIGN_LIFECYCLE_ROOTS = [ + 'project/', + 'assistant/', + 'profile-pictures/', + 'organization-logos/', +] as const + +const WORKSPACE_KEYED_ROOTS: ReadonlySet = new Set( + WORKSPACE_STORAGE_FAMILIES.map(({ root }) => root) +) interface StorageObject { key: string @@ -187,78 +207,213 @@ function parseObjectCleanup(value: unknown): ObjectCleanupPayload { return { userId: value.userId, objects, cursor } } +/** Every string inside a URL column or a JSON settings blob, including JSON held as a string. */ +function collectStrings(value: unknown, into: string[]): void { + if (typeof value === 'string') { + into.push(value) + const trimmed = value.trim() + if (trimmed.startsWith('{') || trimmed.startsWith('[')) { + try { + collectStrings(JSON.parse(trimmed), into) + } catch {} + } + } else if (Array.isArray(value)) { + for (const item of value) collectStrings(item, into) + } else if (isRecordLike(value)) { + for (const item of Object.values(value)) collectStrings(item, into) + } +} + /** - * Keys some surviving row still names. Every row of a deleted workspace or account is gone by - * the time a sweep runs, so a remaining reference belongs to data that outlived the deletion — - * a moved knowledge base, a handed-off file, a soft-deleted file elsewhere — and its object stays. + * The storage keys a stored URL can name: a `/api/files/serve/...` path (relative or absolute, + * with or without a provider segment), or a direct bucket URL in virtual-host or path style. + * Over-matching only keeps an object. */ -async function findReferencedStorageKeys(keys: readonly string[]): Promise> { - const referenced = new Set() - if (keys.length === 0) return referenced - const list = [...keys] - const rows = [ - ...(await db - .select({ key: workspaceFiles.key }) - .from(workspaceFiles) - .where(inArray(workspaceFiles.key, list))), - ...(await db - .select({ key: workspaceFile.key }) - .from(workspaceFile) - .where(inArray(workspaceFile.key, list))), - ...(await db - .select({ key: workspaceFileVersion.key }) - .from(workspaceFileVersion) - .where(inArray(workspaceFileVersion.key, list))), - ...(await db - .select({ key: document.storageKey }) - .from(document) - .where(inArray(document.storageKey, list))), - ...(await db - .select({ key: executionLargeValues.key }) - .from(executionLargeValues) - .where(inArray(executionLargeValues.key, list))), +function keysNamedByUrl(value: string): string[] { + const keys = new Set() + const add = (candidate: string) => { + if (!candidate) return + keys.add(candidate) + try { + keys.add(decodeURIComponent(candidate)) + } catch {} + } + const path = extractStorageKey(value.trim()).replace(/^\/+/, '') + add(path) + const slash = path.indexOf('/') + if (slash > 0) add(path.slice(slash + 1)) + return [...keys] +} + +/** + * Keys named by the columns that hold a stored asset as a URL rather than a row: + * `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`. + */ +async function loadUrlReferencedKeys(): Promise> { + const values: string[] = [] + const sources = [ + db + .select({ value: user.image }) + .from(user) + .where(or(like(user.image, '%/api/files/serve/%'), like(user.image, '%profile-pictures/%'))), + db + .select({ value: workspaceTable.logoUrl }) + .from(workspaceTable) + .where(isNotNull(workspaceTable.logoUrl)), + db + .select({ value: customBlock.iconUrl }) + .from(customBlock) + .where(isNotNull(customBlock.iconUrl)), + db + .select({ value: chat.customizations }) + .from(chat) + .where(sql`${chat.customizations}::text LIKE '%Url%'`), + db.select({ value: oauthClient.icon }).from(oauthClient).where(isNotNull(oauthClient.icon)), ] - for (const row of rows) if (row.key) referenced.add(row.key) - return referenced + for (const source of sources) for (const row of await source) collectStrings(row.value, values) + const organizations = await db + .select({ + logo: organization.logo, + whitelabel: organization.whitelabelSettings, + metadata: organization.metadata, + }) + .from(organization) + .where( + or( + isNotNull(organization.logo), + isNotNull(organization.whitelabelSettings), + isNotNull(organization.metadata) + ) + ) + for (const row of organizations) collectStrings([row.logo, row.whitelabel, row.metadata], values) + const keys = new Set() + for (const value of values) for (const key of keysNamedByUrl(value)) keys.add(key) + return keys +} + +/** + * Builds the check every account-deletion purge runs before deleting a key. 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 (see {@link loadUrlReferencedKeys}), loaded once per guard. + * + * Every row of a deleted workspace or account is gone when this runs, so a remaining reference + * belongs to data that outlived the deletion — a moved knowledge base, a handed-off file, an + * organization logo. 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. + */ +export function createStorageReferenceGuard(): (keys: readonly string[]) => Promise> { + let urlKeys: Promise> | undefined + return async (keys) => { + const referenced = new Set() + if (keys.length === 0) return referenced + const list = [...new Set(keys)] + const rows = [ + ...(await db + .select({ key: workspaceFiles.key }) + .from(workspaceFiles) + .where(inArray(workspaceFiles.key, list))), + ...(await db + .select({ key: workspaceFile.key }) + .from(workspaceFile) + .where(inArray(workspaceFile.key, list))), + ...(await db + .select({ key: workspaceFileVersion.key }) + .from(workspaceFileVersion) + .where(inArray(workspaceFileVersion.key, list))), + ...(await db + .select({ key: document.storageKey }) + .from(document) + .where(inArray(document.storageKey, list))), + ...(await db + .select({ key: executionLargeValues.key }) + .from(executionLargeValues) + .where(inArray(executionLargeValues.key, list))), + ] + for (const row of rows) if (row.key) referenced.add(row.key) + urlKeys ??= loadUrlReferencedKeys() + const named = await urlKeys + for (const key of list) if (named.has(key)) referenced.add(key) + return referenced + } +} + +/** + * Exact keys under a workspace-keyed root whose workspace still exists. Such an object stays + * readable to that workspace's members by key, so an exact-key cleanup never deletes it, even when + * the row that named it cascaded away with the account. A workspace segment that is not a valid id + * is kept too. + */ +async function keysUnderLiveWorkspaces(keys: readonly string[]): Promise> { + const kept = new Set() + const keysByWorkspace = new Map() + for (const key of keys) { + const [root, workspaceId] = key.split('/') + if (!WORKSPACE_KEYED_ROOTS.has(root)) continue + if (!workspaceId || !SAFE_ID.test(workspaceId)) { + kept.add(key) + continue + } + keysByWorkspace.set(workspaceId, [...(keysByWorkspace.get(workspaceId) ?? []), key]) + } + if (keysByWorkspace.size === 0) return kept + const live = await db + .select({ id: workspaceTable.id }) + .from(workspaceTable) + .where(inArray(workspaceTable.id, [...keysByWorkspace.keys()])) + for (const { id } of live) for (const key of keysByWorkspace.get(id) ?? []) kept.add(key) + return kept +} + +/** A local sidecar belongs to its object: it is kept exactly when the object is. */ +function sidecarOwner(key: string): string | null { + return key.endsWith(LOCAL_UPLOAD_METADATA_SUFFIX) + ? key.slice(0, -LOCAL_UPLOAD_METADATA_SUFFIX.length) + : null } /** - * Walks one prefix to its end, deleting every listed object no surviving row references. Cloud - * listings follow the provider cursor within a run; a local listing restarts from the first key - * still on disk, so a page made only of kept keys ends the walk rather than looping on it. + * Walks one prefix to its end, deleting every listed object the reference guard does not keep. + * Cloud listings follow the provider cursor within a run; a local listing restarts from the first + * key still on disk, so a page that removes nothing is `stalled` rather than re-listed forever. */ async function sweepWorkspacePrefix( prefix: string, storageContext: StorageContext, - context: OutboxEventContext -): Promise<{ deleted: number; complete: boolean }> { + context: OutboxEventContext, + isReferenced: (keys: readonly string[]) => Promise> +): Promise<{ deleted: number; complete: boolean; stalled: boolean }> { let deleted = 0 let pageToken: string | undefined for (;;) { - if (deadlineNear(context)) return { deleted, complete: false } + if (deadlineNear(context)) return { deleted, complete: false, stalled: false } const page = await listStorageKeyPage(prefix, storageContext, context.signal, pageToken) if (page.keys.some((key) => !key.startsWith(prefix))) throw new Error('Storage provider returned a key outside the workspace prefix') - const referenced = await findReferencedStorageKeys(page.keys) - if (referenced.size > 0) - logger.warn('Keeping objects a surviving row still references', { - prefix, - kept: referenced.size, - }) + const owners = page.keys.flatMap((key) => sidecarOwner(key) ?? []) + const referenced = await isReferenced([...page.keys, ...owners]) + const kept = (key: string) => referenced.has(key) || referenced.has(sidecarOwner(key) ?? key) + const keptCount = page.keys.filter(kept).length + if (keptCount > 0) + logger.warn('Keeping objects a surviving reference still names', { prefix, kept: keptCount }) const result = await deleteStorageKeysUnderPrefix( prefix, storageContext, - page.keys.filter((key) => !referenced.has(key)), + page.keys.filter((key) => !kept(key)), context ) deleted += result.deleted - if (!result.complete) return { deleted, complete: false } - if (page.done) return { deleted, complete: true } + if (!result.complete) return { deleted, complete: false, stalled: false } + if (page.done) return { deleted, complete: true, stalled: false } if (page.nextPageToken) { pageToken = page.nextPageToken } else if (result.deleted === 0) { - logger.error('Workspace storage sweep stopped at a page of referenced objects', { prefix }) - return { deleted, complete: true } + logger.error('Workspace storage sweep removed nothing from a full local page', { prefix }) + return { deleted, complete: false, stalled: true } } } } @@ -281,14 +436,15 @@ const sweepWorkspaceStorage: OutboxHandler = async (raw, context) => { }) return } + const isReferenced = createStorageReferenceGuard() let progressed = false for (let family = payload.family; family < WORKSPACE_STORAGE_FAMILIES.length; family++) { const { root, context: storageContext } = WORKSPACE_STORAGE_FAMILIES[family] const prefix = workspaceStoragePrefix(root, payload.workspaceId) - const result = await sweepWorkspacePrefix(prefix, storageContext, context) + const result = await sweepWorkspacePrefix(prefix, storageContext, context, isReferenced) if (result.deleted > 0) progressed = true if (!result.complete) { - return progressed + return progressed && !result.stalled ? continueOutboxHandler('Continuing workspace storage sweep') : deferOutboxHandler('Workspace storage sweep made no progress before its deadline') } @@ -299,7 +455,7 @@ const sweepWorkspaceStorage: OutboxHandler = async (raw, context) => { /** * Deletes exact keys recorded by an account deletion. The account reappearing refuses the batch - * as a whole; a key any surviving row references is kept. + * as a whole; a key the reference guard keeps, or one under a workspace that still exists, stays. */ const cleanupAccountObjects: OutboxHandler = async (raw, context) => { const payload = parseObjectCleanup(raw) @@ -314,10 +470,11 @@ const cleanupAccountObjects: OutboxHandler = async (raw, context) => { }) return } - const pending = payload.objects.slice(payload.cursor) - const referenced = await findReferencedStorageKeys(pending.map((object) => object.key)) + const pending = payload.objects.slice(payload.cursor).map((object) => object.key) + const referenced = await keysUnderLiveWorkspaces(pending) + for (const key of await createStorageReferenceGuard()(pending)) referenced.add(key) if (referenced.size > 0) - logger.warn('Keeping objects a surviving row still references', { + logger.warn('Keeping objects a surviving reference still names', { userId: payload.userId, kept: referenced.size, }) @@ -427,27 +584,29 @@ async function recordVersionsOf( recorder: ObjectRecorder, files: readonly ObjectRow[] ): Promise { - if (files.length === 0 || recorder.full) return + if (files.length === 0) return const contexts = new Map(files.map((file) => [file.id, file.context])) - const versions = await tx - .select({ - id: workspaceFileVersion.id, - key: workspaceFileVersion.key, - fileId: workspaceFileVersion.fileId, - }) - .from(workspaceFileVersion) - .where( - inArray( - workspaceFileVersion.fileId, - files.map((file) => file.id) + const fileIds = files.map((file) => file.id) + await recordPages(recorder, (afterId) => + tx + .select({ + id: workspaceFileVersion.id, + key: workspaceFileVersion.key, + fileId: workspaceFileVersion.fileId, + }) + .from(workspaceFileVersion) + .where( + and(inArray(workspaceFileVersion.fileId, fileIds), gt(workspaceFileVersion.id, afterId)) + ) + .orderBy(workspaceFileVersion.id) + .limit(ROW_PAGE_SIZE) + .then((versions) => + versions.map((version) => ({ + id: version.id, + key: version.key, + context: contexts.get(version.fileId) ?? null, + })) ) - ) - await recorder.record( - versions.map((version) => ({ - id: version.id, - key: version.key, - context: contexts.get(version.fileId) ?? null, - })) ) } @@ -460,12 +619,14 @@ async function recordVersionsOf( * whether or not a row ever named it. * - Exact keys for the rest of what cascades away: objects in deleted workspaces outside those * prefixes (knowledge-base documents, Copilot uploads), and the account's private files - * elsewhere — chat attachments of its own chats, private contexts, and files with no - * workspace. Files handed to a successor, Project files, organization files, and attachments - * of somebody else's chat are never queued. + * elsewhere — chat attachments of its own chats, private contexts, and Copilot files with no + * workspace. Files handed to a successor, Project and organization files, attachments of + * somebody else's chat, profile pictures, and workspace-less rows of any other context (a + * legacy shared row can lack its workspace) are never queued. * - * Both wait out every upload URL signed before the deletion, and both re-check at run time that - * the owner is still gone and that no surviving row references a key before deleting it. + * Both wait out every upload URL signed before the deletion and re-check at run time that the + * owner is still gone and that {@link createStorageReferenceGuard} does not keep the key; the + * exact-key cleanup also keeps any key under a workspace that still exists. */ export async function enqueueAccountStorageCleanupInTx( tx: DbTransaction, @@ -562,6 +723,10 @@ export async function enqueueAccountStorageCleanupInTx( ) } + const ownChats = tx + .select({ id: copilotChats.id }) + .from(copilotChats) + .where(eq(copilotChats.userId, userId)) await recordPages( recorder, (afterId) => @@ -581,18 +746,19 @@ export async function enqueueAccountStorageCleanupInTx( : undefined, or( and( - isNull(workspaceFiles.chatId), + isNull(workspaceFiles.workspaceId), + eq(workspaceFiles.context, 'copilot'), + or(isNull(workspaceFiles.chatId), inArray(workspaceFiles.chatId, ownChats)) + ), + and( + isNotNull(workspaceFiles.workspaceId), or( - isNull(workspaceFiles.workspaceId), - notInArray(workspaceFiles.context, [...HANDED_OFF_FILE_CONTEXTS]) + inArray(workspaceFiles.chatId, ownChats), + and( + isNull(workspaceFiles.chatId), + notInArray(workspaceFiles.context, [...HANDED_OFF_FILE_CONTEXTS]) + ) ) - ), - inArray( - workspaceFiles.chatId, - tx - .select({ id: copilotChats.id }) - .from(copilotChats) - .where(eq(copilotChats.userId, userId)) ) ), gt(workspaceFiles.id, afterId) From 91c6d07f3a99bde4544896fa894eb3a77ddbb9e4 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Fri, 16 Oct 2026 15:33:14 -0700 Subject: [PATCH 03/11] 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. --- apps/sim/lib/users/storage-sweep.test.ts | 41 +++++++- apps/sim/lib/users/storage-sweep.ts | 114 +++++++++++++++++++---- 2 files changed, 131 insertions(+), 24 deletions(-) diff --git a/apps/sim/lib/users/storage-sweep.test.ts b/apps/sim/lib/users/storage-sweep.test.ts index e00ab19e33d..fa2caf9381c 100644 --- a/apps/sim/lib/users/storage-sweep.test.ts +++ b/apps/sim/lib/users/storage-sweep.test.ts @@ -191,11 +191,13 @@ describe('deleted workspace storage sweep', () => { const result = await sweep( { version: 1, workspaceId: 'ws1' }, - outboxContext({ deadlineAt: Date.now() + 5_500 }) + outboxContext({ deadlineAt: Date.now() + 120_000 }) ) - expect(result).toMatchObject({ outcome: 'deferred' }) - expect(result).not.toMatchObject({ consumeAttempt: false }) + expect(result).toEqual({ + outcome: 'deferred', + reason: 'Workspace storage sweep removed nothing from a full listing page', + }) }) it.each([ @@ -286,13 +288,42 @@ describe('account storage object cleanup', () => { expect(await exists('copilot/abc/private.txt')).toBe(false) }) - it('keeps a key a stored asset URL names', async () => { + it('keeps a publicly served key a stored asset URL names', async () => { + await put('workspace-logos/ws9/icon.png') + queueTableRows(schemaMock.oauthClient, [ + { value: '/api/files/serve/workspace-logos%2Fws9%2Ficon.png' }, + ]) + + await cleanup( + payload([{ key: 'workspace-logos/ws9/icon.png', context: 'workspace-logos' }]), + outboxContext() + ) + + expect(await exists('workspace-logos/ws9/icon.png')).toBe(true) + }) + + it('reads no URL column for keys that are never served without a row', async () => { await put('copilot/abc/icon.png') queueTableRows(schemaMock.oauthClient, [{ value: '/api/files/serve/copilot%2Fabc%2Ficon.png' }]) await cleanup(payload([{ key: 'copilot/abc/icon.png', context: 'copilot' }]), outboxContext()) - expect(await exists('copilot/abc/icon.png')).toBe(true) + expect(await exists('copilot/abc/icon.png')).toBe(false) + for (const table of ['organization', 'customBlock', 'chat', 'oauthClient'] as const) + expect(dbChainMockFns.from).not.toHaveBeenCalledWith(schemaMock[table]) + }) + + it('still purges when a stored URL has malformed percent-encoding', async () => { + await put('workspace-logos/ws9/old.png') + queueTableRows(schemaMock.user, []) + queueTableRows(schemaMock.user, [{ value: '/api/files/serve/%E0%A4%A' }]) + + await cleanup( + payload([{ key: 'workspace-logos/ws9/old.png', context: 'workspace-logos' }]), + outboxContext() + ) + + expect(await exists('workspace-logos/ws9/old.png')).toBe(false) }) it.each([ diff --git a/apps/sim/lib/users/storage-sweep.ts b/apps/sim/lib/users/storage-sweep.ts index dc292943c9f..a2dac1be7de 100644 --- a/apps/sim/lib/users/storage-sweep.ts +++ b/apps/sim/lib/users/storage-sweep.ts @@ -224,10 +224,13 @@ function collectStrings(value: unknown, into: string[]): void { } } +const SERVE_PATH = '/api/files/serve/' + /** * The storage keys a stored URL can name: a `/api/files/serve/...` path (relative or absolute, * with or without a provider segment), or a direct bucket URL in virtual-host or path style. - * Over-matching only keeps an object. + * Over-matching only keeps an object. Never throws: a value whose percent-encoding is malformed + * falls back to its raw path, so one bad stored string cannot disable every purge. */ function keysNamedByUrl(value: string): string[] { const keys = new Set() @@ -238,39 +241,85 @@ function keysNamedByUrl(value: string): string[] { keys.add(decodeURIComponent(candidate)) } catch {} } - const path = extractStorageKey(value.trim()).replace(/^\/+/, '') + const trimmed = value.trim() + let path: string + try { + path = extractStorageKey(trimmed) + } catch { + const raw = trimmed.split('?')[0] + const serveAt = raw.indexOf(SERVE_PATH) + path = serveAt >= 0 ? raw.slice(serveAt + SERVE_PATH.length) : raw + } + path = path.replace(/^\/+/, '') add(path) const slash = path.indexOf('/') if (slash > 0) add(path.slice(slash + 1)) return [...keys] } +/** + * Key prefixes the file-serve route answers without a row or a session + * (`isPublicByKeyPrefix` in `app/api/files/serve/[...path]/route.ts`). Only these objects can be + * live through a stored URL alone, so only they need the URL-column check. + */ +const PUBLIC_SERVE_PREFIXES = [ + 'profile-pictures/', + 'og-images/', + 'workspace-logos/', + 'organization-logos/', +] as const + +const MIN_NEEDLE_LENGTH = 4 +const NEEDLES_PER_QUERY = 50 + +/** + * The longest run of URL-safe characters after a key's root. Percent-encoding never splits such a + * run, so any stored URL naming the key contains it; `null` when the key has none long enough. + */ +function urlNeedle(key: string): string | null { + let best = '' + for (const run of key.slice(key.indexOf('/') + 1).match(/[A-Za-z0-9_-]+/g) ?? []) + if (run.length > best.length) best = run + return best.length >= MIN_NEEDLE_LENGTH ? best : null +} + /** * Keys named by the columns that hold a stored asset as a URL rather than a row: * `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`. + * and avatar), and `oauth_client.icon`. With `needles`, reads only rows whose value contains one; + * without, every row that holds a URL. */ -async function loadUrlReferencedKeys(): Promise> { +async function loadUrlReferencedKeys(needles: readonly string[] | null): Promise> { + const matching = (column: SQLWrapper, fallback: SQL | undefined): SQL | undefined => + needles ? or(...needles.map((needle) => sql`${column}::text LIKE ${`%${needle}%`}`)) : fallback const values: string[] = [] const sources = [ db .select({ value: user.image }) .from(user) - .where(or(like(user.image, '%/api/files/serve/%'), like(user.image, '%profile-pictures/%'))), + .where( + matching( + user.image, + or(like(user.image, '%/api/files/serve/%'), like(user.image, '%profile-pictures/%')) + ) + ), db .select({ value: workspaceTable.logoUrl }) .from(workspaceTable) - .where(isNotNull(workspaceTable.logoUrl)), + .where(matching(workspaceTable.logoUrl, isNotNull(workspaceTable.logoUrl))), db .select({ value: customBlock.iconUrl }) .from(customBlock) - .where(isNotNull(customBlock.iconUrl)), + .where(matching(customBlock.iconUrl, isNotNull(customBlock.iconUrl))), db .select({ value: chat.customizations }) .from(chat) - .where(sql`${chat.customizations}::text LIKE '%Url%'`), - db.select({ value: oauthClient.icon }).from(oauthClient).where(isNotNull(oauthClient.icon)), + .where(matching(chat.customizations, sql`${chat.customizations}::text LIKE '%Url%'`)), + db + .select({ value: oauthClient.icon }) + .from(oauthClient) + .where(matching(oauthClient.icon, isNotNull(oauthClient.icon))), ] for (const source of sources) for (const row of await source) collectStrings(row.value, values) const organizations = await db @@ -282,9 +331,9 @@ async function loadUrlReferencedKeys(): Promise> { .from(organization) .where( or( - isNotNull(organization.logo), - isNotNull(organization.whitelabelSettings), - isNotNull(organization.metadata) + matching(organization.logo, isNotNull(organization.logo)), + matching(organization.whitelabelSettings, isNotNull(organization.whitelabelSettings)), + matching(organization.metadata, isNotNull(organization.metadata)) ) ) for (const row of organizations) collectStrings([row.logo, row.whitelabel, row.metadata], values) @@ -298,16 +347,37 @@ async function loadUrlReferencedKeys(): Promise> { * * - 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 (see {@link loadUrlReferencedKeys}), loaded once per guard. + * - it sits under a {@link PUBLIC_SERVE_PREFIXES} root and a stored asset URL names it (see + * {@link loadUrlReferencedKeys}). Only rows containing the key's longest URL-safe run are read, + * each run at most once per guard. Any other key is unreadable without a row, so no URL column + * is read for it. * * Every row of a deleted workspace or account is gone when this runs, so a remaining reference * belongs to data that outlived the deletion — a moved knowledge base, a handed-off file, an * organization logo. 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. + * attachments that is the intended erasure. A failed lookup throws, so nothing is deleted. */ export function createStorageReferenceGuard(): (keys: readonly string[]) => Promise> { - let urlKeys: Promise> | undefined + const loadedNeedles = new Set() + const named = new Set() + let loadedAll = false + const loadNamedKeys = async (publicKeys: readonly string[]) => { + if (loadedAll || publicKeys.length === 0) return + const needles = publicKeys.map(urlNeedle) + if (needles.includes(null)) { + for (const key of await loadUrlReferencedKeys(null)) named.add(key) + loadedAll = true + return + } + const pending = [...new Set(needles.flatMap((needle) => needle ?? []))].filter( + (needle) => !loadedNeedles.has(needle) + ) + for (const batch of chunkArray(pending, NEEDLES_PER_QUERY)) { + for (const key of await loadUrlReferencedKeys(batch)) named.add(key) + for (const needle of batch) loadedNeedles.add(needle) + } + } return async (keys) => { const referenced = new Set() if (keys.length === 0) return referenced @@ -335,9 +405,11 @@ export function createStorageReferenceGuard(): (keys: readonly string[]) => Prom .where(inArray(executionLargeValues.key, list))), ] for (const row of rows) if (row.key) referenced.add(row.key) - urlKeys ??= loadUrlReferencedKeys() - const named = await urlKeys - for (const key of list) if (named.has(key)) referenced.add(key) + const publicKeys = list.filter((key) => + PUBLIC_SERVE_PREFIXES.some((prefix) => key.startsWith(prefix)) + ) + await loadNamedKeys(publicKeys) + for (const key of publicKeys) if (named.has(key)) referenced.add(key) return referenced } } @@ -444,7 +516,11 @@ const sweepWorkspaceStorage: OutboxHandler = async (raw, context) => { const result = await sweepWorkspacePrefix(prefix, storageContext, context, isReferenced) if (result.deleted > 0) progressed = true if (!result.complete) { - return progressed && !result.stalled + if (result.stalled) + return deferOutboxHandler( + 'Workspace storage sweep removed nothing from a full listing page' + ) + return progressed ? continueOutboxHandler('Continuing workspace storage sweep') : deferOutboxHandler('Workspace storage sweep made no progress before its deadline') } From f8a8d6498ebc494396ef9490b601daa7068c6dd8 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Fri, 16 Oct 2026 15:50:33 -0700 Subject: [PATCH 04/11] 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 --- .../app/api/files/serve/[...path]/route.ts | 14 +- .../lib/projects/files/application/uploads.ts | 8 +- apps/sim/lib/projects/files/prefix-cleanup.ts | 201 ++------------ apps/sim/lib/uploads/core/prefix-sweep.ts | 184 +++++++++++++ apps/sim/lib/uploads/shared/types.ts | 13 + apps/sim/lib/uploads/utils/file.ts | 6 + apps/sim/lib/users/account-deletion.ts | 6 +- apps/sim/lib/users/storage-reference-guard.ts | 224 ++++++++++++++++ apps/sim/lib/users/storage-sweep.test.ts | 14 + apps/sim/lib/users/storage-sweep.ts | 248 +----------------- apps/sim/lib/workspaces/resource-handoff.ts | 14 +- packages/testing/src/mocks/file-utils.mock.ts | 5 + 12 files changed, 488 insertions(+), 449 deletions(-) create mode 100644 apps/sim/lib/uploads/core/prefix-sweep.ts create mode 100644 apps/sim/lib/users/storage-reference-guard.ts diff --git a/apps/sim/app/api/files/serve/[...path]/route.ts b/apps/sim/app/api/files/serve/[...path]/route.ts index 478bab67a90..fb1b19563dd 100644 --- a/apps/sim/app/api/files/serve/[...path]/route.ts +++ b/apps/sim/app/api/files/serve/[...path]/route.ts @@ -25,7 +25,11 @@ import { FILE_CACHE_CONTROL, workspaceFileCacheControl } from '@/lib/uploads/ser import { resolveServableImageBytes } from '@/lib/uploads/server/image-derivative' import { resolveStoredFileContext } from '@/lib/uploads/server/metadata' import { MAX_BUFFERED_TRANSFER_BYTES } from '@/lib/uploads/shared/types' -import { inferContextFromKey, resolveEffectiveMimeType } from '@/lib/uploads/utils/file' +import { + inferContextFromKey, + isPublicStorageKey, + resolveEffectiveMimeType, +} from '@/lib/uploads/utils/file' import { internalWorkspaceFileServeAuth } from '@/lib/workspace-files/api' import { fetchAuthorizedServableWorkspaceFileBuffer } from '@/lib/workspace-files/application/fetch-servable-workspace-file-buffer' import { finishFileDelivery } from '@/lib/workspace-files/application/finish-file-delivery' @@ -299,13 +303,7 @@ export const GET = withRouteHandler( }) } - const isPublicByKeyPrefix = - cloudKey.startsWith('profile-pictures/') || - cloudKey.startsWith('og-images/') || - cloudKey.startsWith('workspace-logos/') || - cloudKey.startsWith('organization-logos/') - - if (isPublicByKeyPrefix) { + if (isPublicStorageKey(cloudKey)) { const context = inferContextFromKey(cloudKey) logger.info(`Serving public ${context}:`, { cloudKey }) if (isUsingCloudStorage() || isCloudPath) { diff --git a/apps/sim/lib/projects/files/application/uploads.ts b/apps/sim/lib/projects/files/application/uploads.ts index b3d41dab88a..188904907a1 100644 --- a/apps/sim/lib/projects/files/application/uploads.ts +++ b/apps/sim/lib/projects/files/application/uploads.ts @@ -29,10 +29,7 @@ import { prepareProjectFileAccounting, recordProjectFileWriteEffects, } from '@/lib/projects/files/application/write-effects' -import { - projectUploadCleanupAvailableAt, - queueRetiredProjectUploadCleanup, -} from '@/lib/projects/files/prefix-cleanup' +import { queueRetiredProjectUploadCleanup } from '@/lib/projects/files/prefix-cleanup' import { resolveFileFolderTarget } from '@/lib/uploads/contexts/workspace/file-folder-manager' import { adoptVerifiedUploadSession, @@ -43,6 +40,7 @@ import { } from '@/lib/uploads/contexts/workspace/file-manager' import type { WorkspaceFileSecretProvenance } from '@/lib/uploads/contexts/workspace/file-secret-provenance' import { enqueueWorkspaceFileStorageCleanups } from '@/lib/uploads/contexts/workspace/file-storage-cleanup-outbox' +import { storageCleanupAvailableAt } from '@/lib/uploads/core/prefix-sweep' import { downloadFile } from '@/lib/uploads/core/storage-service' import { requestOrigin } from '@/lib/uploads/upload-session/application' import { assertProjectFileUploadBinding } from '@/lib/uploads/upload-session/project-file-binding' @@ -321,7 +319,7 @@ const registerProjectUpload = defineAuthorizedProjectFileUseCase({ await registration.registerFile(file.id) const cleanupIds = prepared.restored ? await enqueueWorkspaceFileStorageCleanups(tx, [input.session.finalKey], 'project', { - availableAt: projectUploadCleanupAvailableAt(), + availableAt: storageCleanupAvailableAt(), }) : [] const result = await mapProjectFileResult(tx, context, file) diff --git a/apps/sim/lib/projects/files/prefix-cleanup.ts b/apps/sim/lib/projects/files/prefix-cleanup.ts index 6997cec99c5..f5f2b867280 100644 --- a/apps/sim/lib/projects/files/prefix-cleanup.ts +++ b/apps/sim/lib/projects/files/prefix-cleanup.ts @@ -1,5 +1,3 @@ -import { opendir, rmdir } from 'node:fs/promises' -import { dirname, join } from 'node:path' import { db } from '@sim/db' import { project, uploadSession } from '@sim/db/schema' import { describeError } from '@sim/utils/errors' @@ -18,22 +16,18 @@ import { PROJECT_FILE_PREFIX_CLEANUP_EVENT, PROJECT_STORAGE_RECONCILE_EVENT, } from '@/lib/projects/files/outbox-events' -import { - getStorageConfig, - type StorageContext, - USE_BLOB_STORAGE, - USE_GCS_STORAGE, - USE_S3_STORAGE, -} from '@/lib/uploads/config' import { isObjectNotFoundError } from '@/lib/uploads/core/errors' -import { UPLOAD_DIR_SERVER } from '@/lib/uploads/core/setup.server' -import { deleteFile } from '@/lib/uploads/core/storage-service' -import { abortProviderUpload, UPLOAD_URL_TTL_MS } from '@/lib/uploads/upload-session/provider' +import { + deleteStorageKeysUnderPrefix, + listStorageKeyPage, + outboxDeadlineNear, + storageCleanupAvailableAt, +} from '@/lib/uploads/core/prefix-sweep' +import { abortProviderUpload } from '@/lib/uploads/upload-session/provider' import type { UploadSessionRecord } from '@/lib/uploads/upload-session/service' import { PROJECT_FILE_UPLOAD_BINDING_KEY } from '@/lib/uploads/upload-session/types' const PAGE_SIZE = 500 -const CLOCK_SKEW_MS = 60_000 type UploadCleanup = Pick< UploadSessionRecord, @@ -45,16 +39,6 @@ interface CleanupPayload { uploadCursor?: number } -/** Expired signatures stop new transfers, but a transfer already in progress may finish later. */ -export function projectUploadCleanupAvailableAt(now = new Date()): Date { - return storageCleanupAvailableAt(now) -} - -/** Earliest moment no upload URL signed before `now` can still write an object. */ -export function storageCleanupAvailableAt(now = new Date()): Date { - return new Date(now.getTime() + UPLOAD_URL_TTL_MS + CLOCK_SKEW_MS) -} - function prefixFor(projectId: string): string { if (!/^[a-zA-Z0-9_-]+$/.test(projectId)) throw new Error('Invalid Project cleanup identity') return `project/${projectId}/` @@ -108,7 +92,7 @@ async function enqueueSweeps( ) { await enqueueOutboxEvent(executor, PROJECT_FILE_PREFIX_CLEANUP_EVENT, payload) await enqueueOutboxEvent(executor, PROJECT_FILE_PREFIX_CLEANUP_EVENT, payload, { - availableAt: projectUploadCleanupAvailableAt(retiredAt), + availableAt: storageCleanupAvailableAt(retiredAt), }) } @@ -217,171 +201,20 @@ export async function queueRetiredProjectUploadCleanup( }) } -async function removeEmptyDirectory(path: string): Promise { - try { - await rmdir(path) - } catch (error) { - if (!['ENOENT', 'ENOTEMPTY', 'EEXIST'].includes(describeError(error).code ?? '')) throw error - } -} - -async function localKeys(prefix: string, signal: AbortSignal): Promise { - const keys: string[] = [] - async function visit(key: string): Promise { - signal.throwIfAborted() - let directory - try { - directory = await opendir(join(UPLOAD_DIR_SERVER, key)) - } catch (error) { - if (describeError(error).code === 'ENOENT') return - throw error - } - for await (const entry of directory) { - signal.throwIfAborted() - const child = `${key}${entry.name}` - if (entry.isDirectory()) await visit(`${child}/`) - else keys.push(child) - if (keys.length >= PAGE_SIZE) return - } - await removeEmptyDirectory(join(UPLOAD_DIR_SERVER, key)) - } - await visit(prefix) - return keys -} - -/** One provider page of object keys; `done` once the listing has nothing after this page. */ -interface StorageKeyPage { - keys: string[] - done: boolean - /** Provider cursor for the following page, valid only within the current run. */ - nextPageToken?: string -} - -/** - * Lists one page of object keys under `prefix` in the bucket serving `context`. A local listing - * has no cursor: it returns the first keys still on disk, so callers advance by deleting them. - */ -export async function listStorageKeyPage( - prefix: string, - context: StorageContext, - signal: AbortSignal, - pageToken?: string -): Promise { - signal.throwIfAborted() - if (USE_S3_STORAGE) { - const { ListObjectsV2Command } = await import('@aws-sdk/client-s3') - const { getS3Client } = await import('@/lib/uploads/providers/s3/client') - const config = getStorageConfig(context) - const page = await getS3Client().send( - new ListObjectsV2Command({ - Bucket: config.bucket, - Prefix: prefix, - MaxKeys: PAGE_SIZE, - ...(pageToken ? { ContinuationToken: pageToken } : {}), - }), - { abortSignal: signal } - ) - const nextPageToken = page.IsTruncated ? page.NextContinuationToken : undefined - return { - keys: (page.Contents ?? []).flatMap((object) => (object.Key ? [object.Key] : [])), - done: !nextPageToken, - ...(nextPageToken ? { nextPageToken } : {}), - } - } - if (USE_BLOB_STORAGE) { - const { getBlobServiceClient } = await import('@/lib/uploads/providers/blob/client') - const config = getStorageConfig(context) - if (!config.containerName) throw new Error(`Blob container missing for ${context} storage`) - const pages = (await getBlobServiceClient()) - .getContainerClient(config.containerName) - .listBlobsFlat({ prefix, abortSignal: signal }) - .byPage({ maxPageSize: PAGE_SIZE, ...(pageToken ? { continuationToken: pageToken } : {}) }) - const page = await pages.next() - if (page.done) return { keys: [], done: true } - const nextPageToken = page.value.continuationToken || undefined - return { - keys: page.value.segment.blobItems.map((object) => object.name), - done: !nextPageToken, - ...(nextPageToken ? { nextPageToken } : {}), - } - } - if (USE_GCS_STORAGE) { - const { getGcsClient } = await import('@/lib/uploads/providers/google-cloud-storage/client') - const config = getStorageConfig(context) - if (!config.bucket) throw new Error(`Bucket missing for ${context} storage`) - const [files, nextQuery] = await (await getGcsClient()).bucket(config.bucket).getFiles({ - prefix, - maxResults: PAGE_SIZE, - autoPaginate: false, - ...(pageToken ? { pageToken } : {}), - }) - signal.throwIfAborted() - const nextPageToken = - isRecordLike(nextQuery) && typeof nextQuery.pageToken === 'string' && nextQuery.pageToken - ? nextQuery.pageToken - : undefined - return { - keys: files.map((file) => file.name), - done: !nextPageToken, - ...(nextPageToken ? { nextPageToken } : {}), - } - } - const keys = await localKeys(prefix, signal) - return { keys, done: keys.length < PAGE_SIZE } -} - -/** - * Deletes listed keys, each of which must sit under `prefix`, and prunes local directories the - * deletions empty, never above the prefix itself. Stops before another deletion once the handler - * deadline is near; `complete` is false when it did. `deleted` counts only objects actually - * removed: a key already gone (or, locally, one whose on-disk name no storage key resolves to) is - * no progress, so a caller re-listing the same page can tell that it is stuck. - */ -export async function deleteStorageKeysUnderPrefix( - prefix: string, - context: StorageContext, - keys: readonly string[], - outbox: Pick -): Promise<{ deleted: number; complete: boolean }> { - if (prefix.length < 2 || !prefix.endsWith('/')) throw new Error('Unbounded storage prefix') - let attempted = 0 - let deleted = 0 - for (const key of keys) { - if (!key.startsWith(prefix)) - throw new Error('Storage provider returned a key outside the prefix') - outbox.signal.throwIfAborted() - if (attempted > 0 && outbox.deadlineAt && Date.now() + 5000 >= outbox.deadlineAt) - return { deleted, complete: false } - attempted++ - let removed = true - try { - await deleteFile({ key, context, signal: outbox.signal }) - } catch (error) { - if (describeError(error).code !== 'ENOENT') throw error - removed = false - } - if (!USE_S3_STORAGE && !USE_BLOB_STORAGE && !USE_GCS_STORAGE) { - const root = join(UPLOAD_DIR_SERVER, prefix.slice(0, -1)) - let directory = dirname(join(UPLOAD_DIR_SERVER, key)) - while (directory === root || directory.startsWith(`${root}/`)) { - await removeEmptyDirectory(directory) - directory = dirname(directory) - } - } - if (removed) deleted++ - } - return { deleted, complete: true } -} - /** `stalled` when a page that must be re-listed removed nothing, so continuing cannot progress. */ async function sweepPrefix( projectId: string, context: OutboxEventContext ): Promise<'done' | 'continue' | 'stalled'> { const prefix = prefixFor(projectId) - const { keys } = await listStorageKeyPage(prefix, 'project', context.signal) - const { deleted, complete } = await deleteStorageKeysUnderPrefix(prefix, 'project', keys, context) - if (complete && keys.length < PAGE_SIZE) return 'done' + const page = await listStorageKeyPage(prefix, 'project', context.signal) + const { deleted, complete } = await deleteStorageKeysUnderPrefix( + prefix, + 'project', + page.keys, + context + ) + if (complete && page.done) return 'done' return deleted > 0 ? 'continue' : 'stalled' } @@ -417,7 +250,7 @@ const cleanupPrefix: OutboxHandler = async (raw, context) => { } if ((index + 1) % 20 === 0 || index === uploads.length - 1) await context.checkpointPayload({ uploadCursor: index + 1 }) - if (context.deadlineAt && Date.now() + 5000 >= context.deadlineAt) { + if (outboxDeadlineNear(context)) { await context.checkpointPayload({ uploadCursor: index + 1 }) return continueOutboxHandler('Continuing retired Project upload cleanup') } diff --git a/apps/sim/lib/uploads/core/prefix-sweep.ts b/apps/sim/lib/uploads/core/prefix-sweep.ts new file mode 100644 index 00000000000..3e9a88bfd1c --- /dev/null +++ b/apps/sim/lib/uploads/core/prefix-sweep.ts @@ -0,0 +1,184 @@ +import { opendir, rmdir } from 'node:fs/promises' +import { dirname, join } from 'node:path' +import { describeError } from '@sim/utils/errors' +import { isRecordLike } from '@sim/utils/object' +import type { OutboxEventContext } from '@/lib/core/outbox/service' +import { + getStorageConfig, + type StorageContext, + USE_BLOB_STORAGE, + USE_GCS_STORAGE, + USE_S3_STORAGE, +} from '@/lib/uploads/config' +import { UPLOAD_DIR_SERVER } from '@/lib/uploads/core/setup.server' +import { deleteFile } from '@/lib/uploads/core/storage-service' +import { UPLOAD_URL_TTL_MS } from '@/lib/uploads/upload-session/provider' + +const PAGE_SIZE = 500 +const CLOCK_SKEW_MS = 60_000 +const DEADLINE_MARGIN_MS = 5000 + +/** + * Earliest moment no upload URL signed before `now` can still write an object: expired signatures + * stop new transfers, but a transfer already in progress may finish later. + */ +export function storageCleanupAvailableAt(now = new Date()): Date { + return new Date(now.getTime() + UPLOAD_URL_TTL_MS + CLOCK_SKEW_MS) +} + +/** True once an outbox handler should stop starting new storage work before its deadline. */ +export function outboxDeadlineNear(context: Pick): boolean { + return Boolean(context.deadlineAt && Date.now() + DEADLINE_MARGIN_MS >= context.deadlineAt) +} + +async function removeEmptyDirectory(path: string): Promise { + try { + await rmdir(path) + } catch (error) { + if (!['ENOENT', 'ENOTEMPTY', 'EEXIST'].includes(describeError(error).code ?? '')) throw error + } +} + +async function localKeys(prefix: string, signal: AbortSignal): Promise { + const keys: string[] = [] + async function visit(key: string): Promise { + signal.throwIfAborted() + let directory + try { + directory = await opendir(join(UPLOAD_DIR_SERVER, key)) + } catch (error) { + if (describeError(error).code === 'ENOENT') return + throw error + } + for await (const entry of directory) { + signal.throwIfAborted() + const child = `${key}${entry.name}` + if (entry.isDirectory()) await visit(`${child}/`) + else keys.push(child) + if (keys.length >= PAGE_SIZE) return + } + await removeEmptyDirectory(join(UPLOAD_DIR_SERVER, key)) + } + await visit(prefix) + return keys +} + +/** One provider page of object keys; `done` once the listing has nothing after this page. */ +interface StorageKeyPage { + keys: string[] + done: boolean + /** Provider cursor for the following page, valid only within the current run. */ + nextPageToken?: string +} + +function keyPage(keys: string[], nextPageToken: string | undefined): StorageKeyPage { + return nextPageToken ? { keys, done: false, nextPageToken } : { keys, done: true } +} + +/** + * Lists one page of object keys under `prefix` in the bucket serving `context`. A local listing + * has no cursor: it returns the first keys still on disk, so callers advance by deleting them. + */ +export async function listStorageKeyPage( + prefix: string, + context: StorageContext, + signal: AbortSignal, + pageToken?: string +): Promise { + signal.throwIfAborted() + if (USE_S3_STORAGE) { + const { ListObjectsV2Command } = await import('@aws-sdk/client-s3') + const { getS3Client } = await import('@/lib/uploads/providers/s3/client') + const config = getStorageConfig(context) + const page = await getS3Client().send( + new ListObjectsV2Command({ + Bucket: config.bucket, + Prefix: prefix, + MaxKeys: PAGE_SIZE, + ...(pageToken ? { ContinuationToken: pageToken } : {}), + }), + { abortSignal: signal } + ) + return keyPage( + (page.Contents ?? []).flatMap((object) => (object.Key ? [object.Key] : [])), + page.IsTruncated ? page.NextContinuationToken : undefined + ) + } + if (USE_BLOB_STORAGE) { + const { getBlobServiceClient } = await import('@/lib/uploads/providers/blob/client') + const config = getStorageConfig(context) + if (!config.containerName) throw new Error(`Blob container missing for ${context} storage`) + const pages = (await getBlobServiceClient()) + .getContainerClient(config.containerName) + .listBlobsFlat({ prefix, abortSignal: signal }) + .byPage({ maxPageSize: PAGE_SIZE, ...(pageToken ? { continuationToken: pageToken } : {}) }) + const page = await pages.next() + if (page.done) return { keys: [], done: true } + return keyPage( + page.value.segment.blobItems.map((object) => object.name), + page.value.continuationToken || undefined + ) + } + if (USE_GCS_STORAGE) { + const { getGcsClient } = await import('@/lib/uploads/providers/google-cloud-storage/client') + const config = getStorageConfig(context) + if (!config.bucket) throw new Error(`Bucket missing for ${context} storage`) + const [files, nextQuery] = await (await getGcsClient()).bucket(config.bucket).getFiles({ + prefix, + maxResults: PAGE_SIZE, + autoPaginate: false, + ...(pageToken ? { pageToken } : {}), + }) + signal.throwIfAborted() + return keyPage( + files.map((file) => file.name), + isRecordLike(nextQuery) && typeof nextQuery.pageToken === 'string' + ? nextQuery.pageToken || undefined + : undefined + ) + } + const keys = await localKeys(prefix, signal) + return { keys, done: keys.length < PAGE_SIZE } +} + +/** + * Deletes listed keys, each of which must sit under `prefix`, and prunes local directories the + * deletions empty, never above the prefix itself. Stops before another deletion once the handler + * deadline is near; `complete` is false when it did. `deleted` counts only objects actually + * removed: a key already gone (or, locally, one whose on-disk name no storage key resolves to) is + * no progress, so a caller re-listing the same page can tell that it is stuck. + */ +export async function deleteStorageKeysUnderPrefix( + prefix: string, + context: StorageContext, + keys: readonly string[], + outbox: Pick +): Promise<{ deleted: number; complete: boolean }> { + if (prefix.length < 2 || !prefix.endsWith('/')) throw new Error('Unbounded storage prefix') + let attempted = 0 + let deleted = 0 + for (const key of keys) { + if (!key.startsWith(prefix)) + throw new Error('Storage provider returned a key outside the prefix') + outbox.signal.throwIfAborted() + if (attempted > 0 && outboxDeadlineNear(outbox)) return { deleted, complete: false } + attempted++ + let removed = true + try { + await deleteFile({ key, context, signal: outbox.signal }) + } catch (error) { + if (describeError(error).code !== 'ENOENT') throw error + removed = false + } + if (!USE_S3_STORAGE && !USE_BLOB_STORAGE && !USE_GCS_STORAGE) { + const root = join(UPLOAD_DIR_SERVER, prefix.slice(0, -1)) + let directory = dirname(join(UPLOAD_DIR_SERVER, key)) + while (directory === root || directory.startsWith(`${root}/`)) { + await removeEmptyDirectory(directory) + directory = dirname(directory) + } + } + if (removed) deleted++ + } + return { deleted, complete: true } +} diff --git a/apps/sim/lib/uploads/shared/types.ts b/apps/sim/lib/uploads/shared/types.ts index 45e5df6ad45..02502593d30 100644 --- a/apps/sim/lib/uploads/shared/types.ts +++ b/apps/sim/lib/uploads/shared/types.ts @@ -91,6 +91,19 @@ export type StorageContext = */ export const WORKSPACE_SCOPED_CONTEXTS = ['workspace', 'mothership'] as const +/** + * Contexts whose chat-less `workspace_files` rows belong to their workspace rather than their + * uploader: a departing member hands them to a successor. Private chat attachments keep their + * user's lifecycle even when stored under a workspace key. + */ +export const WORKSPACE_SHARED_FILE_CONTEXTS = [ + 'workspace', + 'knowledge-base', + 'execution', + 'chat', + 'workspace-logos', +] as const + export type WorkspaceScopedContext = (typeof WORKSPACE_SCOPED_CONTEXTS)[number] export function isWorkspaceScopedContext( diff --git a/apps/sim/lib/uploads/utils/file.ts b/apps/sim/lib/uploads/utils/file.ts index 43b15b67df9..97c373ba0ab 100644 --- a/apps/sim/lib/uploads/utils/file.ts +++ b/apps/sim/lib/uploads/utils/file.ts @@ -789,6 +789,12 @@ export function isPublicStorageContext(context: StorageContext): boolean { return PUBLIC_STORAGE_CONTEXTS.has(context) } +/** Whether a stored key sits under a world-readable prefix, served without a row or a session. */ +export function isPublicStorageKey(key: string): boolean { + const context = tryInferContextFromKey(key) + return context !== null && isPublicStorageContext(context) +} + /** * Resolve the storage context for a stored file from its trusted key prefix. * diff --git a/apps/sim/lib/users/account-deletion.ts b/apps/sim/lib/users/account-deletion.ts index cef801f5c37..8a83d0004a0 100644 --- a/apps/sim/lib/users/account-deletion.ts +++ b/apps/sim/lib/users/account-deletion.ts @@ -54,10 +54,8 @@ import { } from '@/lib/table/rows/executions' import type { StorageContext } from '@/lib/uploads' import { isUsingCloudStorage, StorageService } from '@/lib/uploads' -import { - createStorageReferenceGuard, - enqueueAccountStorageCleanupInTx, -} from '@/lib/users/storage-sweep' +import { createStorageReferenceGuard } from '@/lib/users/storage-reference-guard' +import { enqueueAccountStorageCleanupInTx } from '@/lib/users/storage-sweep' import { listSharedResourceWorkspaceIdsForUser, listUnresolvedSharedResourceWorkspaces, diff --git a/apps/sim/lib/users/storage-reference-guard.ts b/apps/sim/lib/users/storage-reference-guard.ts new file mode 100644 index 00000000000..cc3fad52b1d --- /dev/null +++ b/apps/sim/lib/users/storage-reference-guard.ts @@ -0,0 +1,224 @@ +import { db } from '@sim/db' +import { + chat, + customBlock, + document, + executionLargeValues, + oauthClient, + organization, + user, + workspaceFile, + workspaceFiles, + workspaceFileVersion, + workspace as workspaceTable, +} from '@sim/db/schema' +import { chunkArray } from '@sim/utils/helpers' +import { isRecordLike } from '@sim/utils/object' +import { inArray, isNotNull, like, or, type SQL, type SQLWrapper, sql } from 'drizzle-orm' +import { extractStorageKey, isPublicStorageKey } from '@/lib/uploads/utils/file' + +const MIN_NEEDLE_LENGTH = 4 +const NEEDLES_PER_QUERY = 50 + +/** Every string inside a URL column or a JSON settings blob, including JSON held as a string. */ +function collectStrings(value: unknown, into: string[]): void { + if (typeof value === 'string') { + into.push(value) + const trimmed = value.trim() + if (trimmed.startsWith('{') || trimmed.startsWith('[')) { + try { + collectStrings(JSON.parse(trimmed), into) + } catch {} + } + } else if (Array.isArray(value)) { + for (const item of value) collectStrings(item, into) + } else if (isRecordLike(value)) { + for (const item of Object.values(value)) collectStrings(item, into) + } +} + +const SERVE_PATH = '/api/files/serve/' + +/** + * The storage keys a stored URL can name: a `/api/files/serve/...` path (relative or absolute, + * with or without a provider segment), or a direct bucket URL in virtual-host or path style. + * Over-matching only keeps an object. Never throws: a value whose percent-encoding is malformed + * yields its raw path and that path with each valid escape decoded (a legacy key can hold a + * literal `%`), so one bad stored string cannot disable every purge. + */ +function keysNamedByUrl(value: string): string[] { + const keys = new Set() + const add = (candidate: string) => { + if (!candidate) return + keys.add(candidate) + try { + keys.add(decodeURIComponent(candidate)) + } catch { + keys.add( + candidate.replace(/(%[0-9A-Fa-f]{2})+/g, (escapes) => { + try { + return decodeURIComponent(escapes) + } catch { + return escapes + } + }) + ) + } + } + const trimmed = value.trim() + let path: string + try { + path = extractStorageKey(trimmed) + } catch { + const raw = trimmed.split('?')[0] + const serveAt = raw.indexOf(SERVE_PATH) + path = serveAt >= 0 ? raw.slice(serveAt + SERVE_PATH.length) : raw + } + path = path.replace(/^\/+/, '') + add(path) + const slash = path.indexOf('/') + if (slash > 0) add(path.slice(slash + 1)) + return [...keys] +} + +/** + * The longest run of URL-safe characters after a key's root. Percent-encoding never splits such a + * run, so any stored URL naming the key contains it; `null` when the key has none long enough. + */ +function urlNeedle(key: string): string | null { + let best = '' + for (const run of key.slice(key.indexOf('/') + 1).match(/[A-Za-z0-9_-]+/g) ?? []) + if (run.length > best.length) best = run + return best.length >= MIN_NEEDLE_LENGTH ? best : null +} + +/** + * Keys named by the columns that hold a stored asset as a URL rather than a row: + * `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`. With `needles`, reads only rows whose value contains one; + * without, every row that holds a URL. + */ +async function loadUrlReferencedKeys(needles: readonly string[] | null): Promise> { + const matching = (column: SQLWrapper, fallback: SQL | undefined): SQL | undefined => + needles ? or(...needles.map((needle) => sql`${column}::text LIKE ${`%${needle}%`}`)) : fallback + const values: string[] = [] + const sources = [ + db + .select({ value: user.image }) + .from(user) + .where( + matching( + user.image, + or(like(user.image, '%/api/files/serve/%'), like(user.image, '%profile-pictures/%')) + ) + ), + db + .select({ value: workspaceTable.logoUrl }) + .from(workspaceTable) + .where(matching(workspaceTable.logoUrl, isNotNull(workspaceTable.logoUrl))), + db + .select({ value: customBlock.iconUrl }) + .from(customBlock) + .where(matching(customBlock.iconUrl, isNotNull(customBlock.iconUrl))), + db + .select({ value: chat.customizations }) + .from(chat) + .where(matching(chat.customizations, sql`${chat.customizations}::text LIKE '%Url%'`)), + db + .select({ value: oauthClient.icon }) + .from(oauthClient) + .where(matching(oauthClient.icon, isNotNull(oauthClient.icon))), + ] + for (const source of sources) for (const row of await source) collectStrings(row.value, values) + const organizations = await db + .select({ + logo: organization.logo, + whitelabel: organization.whitelabelSettings, + metadata: organization.metadata, + }) + .from(organization) + .where( + or( + matching(organization.logo, isNotNull(organization.logo)), + matching(organization.whitelabelSettings, isNotNull(organization.whitelabelSettings)), + matching(organization.metadata, isNotNull(organization.metadata)) + ) + ) + for (const row of organizations) collectStrings([row.logo, row.whitelabel, row.metadata], values) + const keys = new Set() + for (const value of values) for (const key of keysNamedByUrl(value)) keys.add(key) + return keys +} + +/** + * Builds the check every account-deletion purge runs before deleting a key. 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 + * - it sits under a world-readable root (`isPublicStorageKey`) and a stored asset URL names it (see + * {@link loadUrlReferencedKeys}). Only rows containing the key's longest URL-safe run are read, + * each run at most once per guard. No URL column is read for any other key: + * `verifyFileAccess` serves it only through a row, by its key path under a workspace that still + * exists (which exact-key cleanup keeps; see `keysUnderLiveWorkspaces`), or by object metadata + * to a signed-in holder of a hand-pasted URL to a private file this deletion means to erase. + * + * Every row of a deleted workspace or account is gone when this runs, so a remaining reference + * belongs to data that outlived the deletion — a moved knowledge base, a handed-off file, an + * organization logo. 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. A failed lookup throws, so nothing is deleted. + */ +export function createStorageReferenceGuard(): (keys: readonly string[]) => Promise> { + const loadedNeedles = new Set() + const named = new Set() + let loadedAll = false + const loadNamedKeys = async (publicKeys: readonly string[]) => { + if (loadedAll || publicKeys.length === 0) return + const needles = publicKeys.map(urlNeedle) + if (needles.includes(null)) { + for (const key of await loadUrlReferencedKeys(null)) named.add(key) + loadedAll = true + return + } + const pending = [...new Set(needles.flatMap((needle) => needle ?? []))].filter( + (needle) => !loadedNeedles.has(needle) + ) + for (const batch of chunkArray(pending, NEEDLES_PER_QUERY)) { + for (const key of await loadUrlReferencedKeys(batch)) named.add(key) + for (const needle of batch) loadedNeedles.add(needle) + } + } + return async (keys) => { + const referenced = new Set() + if (keys.length === 0) return referenced + const list = [...new Set(keys)] + const rows = [ + ...(await db + .select({ key: workspaceFiles.key }) + .from(workspaceFiles) + .where(inArray(workspaceFiles.key, list))), + ...(await db + .select({ key: workspaceFile.key }) + .from(workspaceFile) + .where(inArray(workspaceFile.key, list))), + ...(await db + .select({ key: workspaceFileVersion.key }) + .from(workspaceFileVersion) + .where(inArray(workspaceFileVersion.key, list))), + ...(await db + .select({ key: document.storageKey }) + .from(document) + .where(inArray(document.storageKey, list))), + ...(await db + .select({ key: executionLargeValues.key }) + .from(executionLargeValues) + .where(inArray(executionLargeValues.key, list))), + ] + for (const row of rows) if (row.key) referenced.add(row.key) + const publicKeys = list.filter(isPublicStorageKey) + await loadNamedKeys(publicKeys) + for (const key of publicKeys) if (named.has(key)) referenced.add(key) + return referenced + } +} diff --git a/apps/sim/lib/users/storage-sweep.test.ts b/apps/sim/lib/users/storage-sweep.test.ts index fa2caf9381c..27c2021c3bd 100644 --- a/apps/sim/lib/users/storage-sweep.test.ts +++ b/apps/sim/lib/users/storage-sweep.test.ts @@ -326,6 +326,20 @@ describe('account storage object cleanup', () => { expect(await exists('workspace-logos/ws9/old.png')).toBe(false) }) + it('keeps a legacy key holding a literal percent sign that a malformed URL names', async () => { + const key = 'workspace-logos/ws9/brand logo%.png' + const storedAs = 'workspace-logos/ws9/brand_logo_.png' + await put(storedAs) + queueTableRows(schemaMock.user, []) + queueTableRows(schemaMock.user, [ + { value: '/api/files/serve/workspace-logos/ws9/brand%20logo%.png' }, + ]) + + await cleanup(payload([{ key, context: 'workspace-logos' }]), outboxContext()) + + expect(await exists(storedAs)).toBe(true) + }) + it.each([ { key: 'project/p1/file.txt', context: 'workspace' }, { key: 'assistant/org/user/file.png', context: 'mothership' }, diff --git a/apps/sim/lib/users/storage-sweep.ts b/apps/sim/lib/users/storage-sweep.ts index a2dac1be7de..9c698ff4b61 100644 --- a/apps/sim/lib/users/storage-sweep.ts +++ b/apps/sim/lib/users/storage-sweep.ts @@ -1,13 +1,8 @@ import { db } from '@sim/db' import { - chat, copilotChats, - customBlock, document, - executionLargeValues, knowledgeBase, - oauthClient, - organization, user, workspaceFile, workspaceFiles, @@ -25,7 +20,6 @@ import { inArray, isNotNull, isNull, - like, notInArray, or, type SQL, @@ -42,15 +36,17 @@ import { type OutboxHandlerRegistry, } from '@/lib/core/outbox/service' import type { DbTransaction } from '@/lib/db/types' +import type { StorageContext } from '@/lib/uploads/config' import { deleteStorageKeysUnderPrefix, listStorageKeyPage, + outboxDeadlineNear, storageCleanupAvailableAt, -} from '@/lib/projects/files/prefix-cleanup' -import type { StorageContext } from '@/lib/uploads/config' +} from '@/lib/uploads/core/prefix-sweep' import { LOCAL_UPLOAD_METADATA_SUFFIX } from '@/lib/uploads/core/storage-key' import { deleteFile } from '@/lib/uploads/core/storage-service' -import { extractStorageKey } from '@/lib/uploads/utils/file' +import { WORKSPACE_SHARED_FILE_CONTEXTS } from '@/lib/uploads/shared/types' +import { createStorageReferenceGuard } from '@/lib/users/storage-reference-guard' import { ACCOUNT_STORAGE_OBJECT_CLEANUP_EVENT, ACCOUNT_WORKSPACE_STORAGE_SWEEP_EVENT, @@ -60,7 +56,6 @@ const logger = createLogger('AccountStorageSweep') const ROW_PAGE_SIZE = 1000 const OBJECTS_PER_EVENT = 100 -const DEADLINE_MARGIN_MS = 5000 /** * Ceiling on exact keys one deletion records for durable cleanup. Past it the rest are logged @@ -84,18 +79,6 @@ const WORKSPACE_STORAGE_FAMILIES = [ { root: 'workspace-logos', context: 'workspace-logos' }, ] as const satisfies readonly { root: string; context: StorageContext }[] -/** - * Mirrors the contexts `reassignSharedResourceOwnershipForWorkspaceMemberRemovalTx` hands to a - * successor. Drift toward fewer contexts only queues keys the reference guard then keeps. - */ -const HANDED_OFF_FILE_CONTEXTS = [ - 'workspace', - 'knowledge-base', - 'execution', - 'chat', - 'workspace-logos', -] as const - const OBJECT_CONTEXTS: ReadonlySet = new Set([ 'workspace', 'mothership', @@ -146,10 +129,6 @@ function workspaceStoragePrefix(root: string, workspaceId: string): string { return `${root}/${workspaceId}/` } -function deadlineNear(context: Pick): boolean { - return Boolean(context.deadlineAt && Date.now() + DEADLINE_MARGIN_MS >= context.deadlineAt) -} - function parseWorkspaceSweep(value: unknown): WorkspaceSweepPayload { if (!isRecordLike(value) || value.version !== 1 || typeof value.workspaceId !== 'string') throw new Error('Invalid workspace storage sweep payload') @@ -207,213 +186,6 @@ function parseObjectCleanup(value: unknown): ObjectCleanupPayload { return { userId: value.userId, objects, cursor } } -/** Every string inside a URL column or a JSON settings blob, including JSON held as a string. */ -function collectStrings(value: unknown, into: string[]): void { - if (typeof value === 'string') { - into.push(value) - const trimmed = value.trim() - if (trimmed.startsWith('{') || trimmed.startsWith('[')) { - try { - collectStrings(JSON.parse(trimmed), into) - } catch {} - } - } else if (Array.isArray(value)) { - for (const item of value) collectStrings(item, into) - } else if (isRecordLike(value)) { - for (const item of Object.values(value)) collectStrings(item, into) - } -} - -const SERVE_PATH = '/api/files/serve/' - -/** - * The storage keys a stored URL can name: a `/api/files/serve/...` path (relative or absolute, - * with or without a provider segment), or a direct bucket URL in virtual-host or path style. - * Over-matching only keeps an object. Never throws: a value whose percent-encoding is malformed - * falls back to its raw path, so one bad stored string cannot disable every purge. - */ -function keysNamedByUrl(value: string): string[] { - const keys = new Set() - const add = (candidate: string) => { - if (!candidate) return - keys.add(candidate) - try { - keys.add(decodeURIComponent(candidate)) - } catch {} - } - const trimmed = value.trim() - let path: string - try { - path = extractStorageKey(trimmed) - } catch { - const raw = trimmed.split('?')[0] - const serveAt = raw.indexOf(SERVE_PATH) - path = serveAt >= 0 ? raw.slice(serveAt + SERVE_PATH.length) : raw - } - path = path.replace(/^\/+/, '') - add(path) - const slash = path.indexOf('/') - if (slash > 0) add(path.slice(slash + 1)) - return [...keys] -} - -/** - * Key prefixes the file-serve route answers without a row or a session - * (`isPublicByKeyPrefix` in `app/api/files/serve/[...path]/route.ts`). Only these objects can be - * live through a stored URL alone, so only they need the URL-column check. - */ -const PUBLIC_SERVE_PREFIXES = [ - 'profile-pictures/', - 'og-images/', - 'workspace-logos/', - 'organization-logos/', -] as const - -const MIN_NEEDLE_LENGTH = 4 -const NEEDLES_PER_QUERY = 50 - -/** - * The longest run of URL-safe characters after a key's root. Percent-encoding never splits such a - * run, so any stored URL naming the key contains it; `null` when the key has none long enough. - */ -function urlNeedle(key: string): string | null { - let best = '' - for (const run of key.slice(key.indexOf('/') + 1).match(/[A-Za-z0-9_-]+/g) ?? []) - if (run.length > best.length) best = run - return best.length >= MIN_NEEDLE_LENGTH ? best : null -} - -/** - * Keys named by the columns that hold a stored asset as a URL rather than a row: - * `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`. With `needles`, reads only rows whose value contains one; - * without, every row that holds a URL. - */ -async function loadUrlReferencedKeys(needles: readonly string[] | null): Promise> { - const matching = (column: SQLWrapper, fallback: SQL | undefined): SQL | undefined => - needles ? or(...needles.map((needle) => sql`${column}::text LIKE ${`%${needle}%`}`)) : fallback - const values: string[] = [] - const sources = [ - db - .select({ value: user.image }) - .from(user) - .where( - matching( - user.image, - or(like(user.image, '%/api/files/serve/%'), like(user.image, '%profile-pictures/%')) - ) - ), - db - .select({ value: workspaceTable.logoUrl }) - .from(workspaceTable) - .where(matching(workspaceTable.logoUrl, isNotNull(workspaceTable.logoUrl))), - db - .select({ value: customBlock.iconUrl }) - .from(customBlock) - .where(matching(customBlock.iconUrl, isNotNull(customBlock.iconUrl))), - db - .select({ value: chat.customizations }) - .from(chat) - .where(matching(chat.customizations, sql`${chat.customizations}::text LIKE '%Url%'`)), - db - .select({ value: oauthClient.icon }) - .from(oauthClient) - .where(matching(oauthClient.icon, isNotNull(oauthClient.icon))), - ] - for (const source of sources) for (const row of await source) collectStrings(row.value, values) - const organizations = await db - .select({ - logo: organization.logo, - whitelabel: organization.whitelabelSettings, - metadata: organization.metadata, - }) - .from(organization) - .where( - or( - matching(organization.logo, isNotNull(organization.logo)), - matching(organization.whitelabelSettings, isNotNull(organization.whitelabelSettings)), - matching(organization.metadata, isNotNull(organization.metadata)) - ) - ) - for (const row of organizations) collectStrings([row.logo, row.whitelabel, row.metadata], values) - const keys = new Set() - for (const value of values) for (const key of keysNamedByUrl(value)) keys.add(key) - return keys -} - -/** - * Builds the check every account-deletion purge runs before deleting a key. 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 - * - it sits under a {@link PUBLIC_SERVE_PREFIXES} root and a stored asset URL names it (see - * {@link loadUrlReferencedKeys}). Only rows containing the key's longest URL-safe run are read, - * each run at most once per guard. Any other key is unreadable without a row, so no URL column - * is read for it. - * - * Every row of a deleted workspace or account is gone when this runs, so a remaining reference - * belongs to data that outlived the deletion — a moved knowledge base, a handed-off file, an - * organization logo. 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. A failed lookup throws, so nothing is deleted. - */ -export function createStorageReferenceGuard(): (keys: readonly string[]) => Promise> { - const loadedNeedles = new Set() - const named = new Set() - let loadedAll = false - const loadNamedKeys = async (publicKeys: readonly string[]) => { - if (loadedAll || publicKeys.length === 0) return - const needles = publicKeys.map(urlNeedle) - if (needles.includes(null)) { - for (const key of await loadUrlReferencedKeys(null)) named.add(key) - loadedAll = true - return - } - const pending = [...new Set(needles.flatMap((needle) => needle ?? []))].filter( - (needle) => !loadedNeedles.has(needle) - ) - for (const batch of chunkArray(pending, NEEDLES_PER_QUERY)) { - for (const key of await loadUrlReferencedKeys(batch)) named.add(key) - for (const needle of batch) loadedNeedles.add(needle) - } - } - return async (keys) => { - const referenced = new Set() - if (keys.length === 0) return referenced - const list = [...new Set(keys)] - const rows = [ - ...(await db - .select({ key: workspaceFiles.key }) - .from(workspaceFiles) - .where(inArray(workspaceFiles.key, list))), - ...(await db - .select({ key: workspaceFile.key }) - .from(workspaceFile) - .where(inArray(workspaceFile.key, list))), - ...(await db - .select({ key: workspaceFileVersion.key }) - .from(workspaceFileVersion) - .where(inArray(workspaceFileVersion.key, list))), - ...(await db - .select({ key: document.storageKey }) - .from(document) - .where(inArray(document.storageKey, list))), - ...(await db - .select({ key: executionLargeValues.key }) - .from(executionLargeValues) - .where(inArray(executionLargeValues.key, list))), - ] - for (const row of rows) if (row.key) referenced.add(row.key) - const publicKeys = list.filter((key) => - PUBLIC_SERVE_PREFIXES.some((prefix) => key.startsWith(prefix)) - ) - await loadNamedKeys(publicKeys) - for (const key of publicKeys) if (named.has(key)) referenced.add(key) - return referenced - } -} - /** * Exact keys under a workspace-keyed root whose workspace still exists. Such an object stays * readable to that workspace's members by key, so an exact-key cleanup never deletes it, even when @@ -430,7 +202,9 @@ async function keysUnderLiveWorkspaces(keys: readonly string[]): Promise !key.startsWith(prefix))) throw new Error('Storage provider returned a key outside the workspace prefix') @@ -556,7 +330,7 @@ const cleanupAccountObjects: OutboxHandler = async (raw, context) => { }) for (let index = payload.cursor; index < payload.objects.length; index++) { context.signal.throwIfAborted() - if (index > payload.cursor && deadlineNear(context)) { + if (index > payload.cursor && outboxDeadlineNear(context)) { await context.checkpointPayload({ cursor: index }) return continueOutboxHandler('Continuing account storage cleanup') } @@ -832,7 +606,7 @@ export async function enqueueAccountStorageCleanupInTx( inArray(workspaceFiles.chatId, ownChats), and( isNull(workspaceFiles.chatId), - notInArray(workspaceFiles.context, [...HANDED_OFF_FILE_CONTEXTS]) + notInArray(workspaceFiles.context, [...WORKSPACE_SHARED_FILE_CONTEXTS]) ) ) ) diff --git a/apps/sim/lib/workspaces/resource-handoff.ts b/apps/sim/lib/workspaces/resource-handoff.ts index ffbfcc1bac0..4b12ba56cc8 100644 --- a/apps/sim/lib/workspaces/resource-handoff.ts +++ b/apps/sim/lib/workspaces/resource-handoff.ts @@ -20,20 +20,12 @@ import { lockProjectsForResourceDepartureTx, } from '@/lib/projects/resource-handoff' import { handoffFileCreatorsInTx } from '@/lib/uploads/contexts/workspace/creator-handoff' +import { WORKSPACE_SHARED_FILE_CONTEXTS } from '@/lib/uploads/shared/types' import { findWorkspaceBillingReplacement, reassignWorkflowOwnershipForWorkspaceMemberRemovalTx, } from '@/lib/workspaces/utils' -/** Private chat attachments keep their user's lifecycle even when stored under a workspace key. */ -const SHARED_FILE_CONTEXTS = [ - 'workspace', - 'knowledge-base', - 'execution', - 'chat', - 'workspace-logos', -] as const - /** Finds surviving containers from ownership references, including departures predating handoff. */ export async function listSharedResourceWorkspaceIdsForUser(executor: DbOrTx, userId: string) { const rows = await executor.execute<{ workspaceId: string }>(sql` @@ -43,7 +35,7 @@ export async function listSharedResourceWorkspaceIdsForUser(executor: DbOrTx, us UNION ALL SELECT ${userTableDefinitions.workspaceId} FROM ${userTableDefinitions} WHERE ${userTableDefinitions.createdBy} = ${userId} UNION ALL SELECT ${knowledgeBase.workspaceId} FROM ${knowledgeBase} WHERE ${knowledgeBase.userId} = ${userId} UNION ALL SELECT ${workspaceFiles.workspaceId} FROM ${workspaceFiles} - WHERE ${workspaceFiles.userId} = ${userId} AND ${inArray(workspaceFiles.context, SHARED_FILE_CONTEXTS)} AND ${workspaceFiles.chatId} IS NULL + WHERE ${workspaceFiles.userId} = ${userId} AND ${inArray(workspaceFiles.context, WORKSPACE_SHARED_FILE_CONTEXTS)} AND ${workspaceFiles.chatId} IS NULL UNION ALL SELECT ${workspaceFile.workspaceId} FROM ${workspaceFile} WHERE ${workspaceFile.uploadedBy} = ${userId} UNION ALL SELECT ${workflowMcpServer.workspaceId} FROM ${workflowMcpServer} WHERE ${workflowMcpServer.createdBy} = ${userId} AND NOT ${workflowMcpServer.isPublic} @@ -206,7 +198,7 @@ export async function reassignSharedResourceOwnershipForWorkspaceMemberRemovalTx and( eq(workspaceFiles.workspaceId, row.id), eq(workspaceFiles.userId, departingUserId), - inArray(workspaceFiles.context, SHARED_FILE_CONTEXTS), + inArray(workspaceFiles.context, WORKSPACE_SHARED_FILE_CONTEXTS), isNull(workspaceFiles.chatId) ), userId diff --git a/packages/testing/src/mocks/file-utils.mock.ts b/packages/testing/src/mocks/file-utils.mock.ts index 2a7bd1d7a15..f1878846366 100644 --- a/packages/testing/src/mocks/file-utils.mock.ts +++ b/packages/testing/src/mocks/file-utils.mock.ts @@ -576,6 +576,10 @@ export const fileUtilsMockFns = { mockInferContextFromKey: vi.fn(inferContextFromKey), mockTryInferContextFromKey: vi.fn(tryInferContextFromKey), mockIsPublicStorageContext: vi.fn(isPublicStorageContext), + mockIsPublicStorageKey: vi.fn((key: string) => { + const context = tryInferContextFromKey(key) + return context !== null && isPublicStorageContext(context) + }), mockResolveTrustedFileContext: vi.fn((key: string, context?: string): string => { try { return inferContextFromKey(key) @@ -735,6 +739,7 @@ export const fileUtilsMock = { inferContextFromKey: fileUtilsMockFns.mockInferContextFromKey, tryInferContextFromKey: fileUtilsMockFns.mockTryInferContextFromKey, isPublicStorageContext: fileUtilsMockFns.mockIsPublicStorageContext, + isPublicStorageKey: fileUtilsMockFns.mockIsPublicStorageKey, resolveTrustedFileContext: fileUtilsMockFns.mockResolveTrustedFileContext, parseInternalFileUrl: fileUtilsMockFns.mockParseInternalFileUrl, resolveHttpsUrlFromFileInput: fileUtilsMockFns.mockResolveHttpsUrlFromFileInput, From 9dee1b4c8df1f7cb78f1fe1805de2173c6647591 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Fri, 16 Oct 2026 15:54:51 -0700 Subject: [PATCH 05/11] 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 --- .../app/api/files/serve/[...path]/route.ts | 17 +- apps/sim/lib/projects/files/prefix-cleanup.ts | 3 +- apps/sim/lib/uploads/core/prefix-sweep.ts | 16 +- apps/sim/lib/users/account-deletion.ts | 20 +- apps/sim/lib/users/storage-reference-guard.ts | 3 +- apps/sim/lib/users/storage-sweep.test.ts | 36 +++- apps/sim/lib/users/storage-sweep.ts | 198 +++++++++--------- 7 files changed, 168 insertions(+), 125 deletions(-) diff --git a/apps/sim/app/api/files/serve/[...path]/route.ts b/apps/sim/app/api/files/serve/[...path]/route.ts index fb1b19563dd..ec99bbfb84b 100644 --- a/apps/sim/app/api/files/serve/[...path]/route.ts +++ b/apps/sim/app/api/files/serve/[...path]/route.ts @@ -25,11 +25,7 @@ import { FILE_CACHE_CONTROL, workspaceFileCacheControl } from '@/lib/uploads/ser import { resolveServableImageBytes } from '@/lib/uploads/server/image-derivative' import { resolveStoredFileContext } from '@/lib/uploads/server/metadata' import { MAX_BUFFERED_TRANSFER_BYTES } from '@/lib/uploads/shared/types' -import { - inferContextFromKey, - isPublicStorageKey, - resolveEffectiveMimeType, -} from '@/lib/uploads/utils/file' +import { inferContextFromKey, resolveEffectiveMimeType } from '@/lib/uploads/utils/file' import { internalWorkspaceFileServeAuth } from '@/lib/workspace-files/api' import { fetchAuthorizedServableWorkspaceFileBuffer } from '@/lib/workspace-files/application/fetch-servable-workspace-file-buffer' import { finishFileDelivery } from '@/lib/workspace-files/application/finish-file-delivery' @@ -303,7 +299,13 @@ export const GET = withRouteHandler( }) } - if (isPublicStorageKey(cloudKey)) { + const isPublicByKeyPrefix = + cloudKey.startsWith('profile-pictures/') || + cloudKey.startsWith('og-images/') || + cloudKey.startsWith('workspace-logos/') || + cloudKey.startsWith('organization-logos/') + + if (isPublicByKeyPrefix) { const context = inferContextFromKey(cloudKey) logger.info(`Serving public ${context}:`, { cloudKey }) if (isUsingCloudStorage() || isCloudPath) { @@ -387,6 +389,7 @@ export const GET = withRouteHandler( request.method === 'HEAD' ) } catch (error) { + if (request.signal.aborted) throw error if (error instanceof InternalUnauthenticatedError) { logger.warn('Unauthorized file access attempt', { error: error.message }) return NextResponse.json( @@ -597,6 +600,7 @@ async function handleLocalFile( options.ifNoneMatch ) } catch (error) { + if (signal?.aborted) throw error logServeFailure('Error reading local file:', error) throw error } @@ -687,6 +691,7 @@ async function handleCloudProxy( options.ifNoneMatch ) } catch (error) { + if (signal?.aborted) throw error logServeFailure('Error downloading from cloud storage:', error) throw error } diff --git a/apps/sim/lib/projects/files/prefix-cleanup.ts b/apps/sim/lib/projects/files/prefix-cleanup.ts index f5f2b867280..8f096a050a0 100644 --- a/apps/sim/lib/projects/files/prefix-cleanup.ts +++ b/apps/sim/lib/projects/files/prefix-cleanup.ts @@ -21,6 +21,7 @@ import { deleteStorageKeysUnderPrefix, listStorageKeyPage, outboxDeadlineNear, + STORAGE_KEY_PAGE_SIZE, storageCleanupAvailableAt, } from '@/lib/uploads/core/prefix-sweep' import { abortProviderUpload } from '@/lib/uploads/upload-session/provider' @@ -214,7 +215,7 @@ async function sweepPrefix( page.keys, context ) - if (complete && page.done) return 'done' + if (complete && page.keys.length < STORAGE_KEY_PAGE_SIZE) return 'done' return deleted > 0 ? 'continue' : 'stalled' } diff --git a/apps/sim/lib/uploads/core/prefix-sweep.ts b/apps/sim/lib/uploads/core/prefix-sweep.ts index 3e9a88bfd1c..267dee8230b 100644 --- a/apps/sim/lib/uploads/core/prefix-sweep.ts +++ b/apps/sim/lib/uploads/core/prefix-sweep.ts @@ -14,7 +14,8 @@ import { UPLOAD_DIR_SERVER } from '@/lib/uploads/core/setup.server' import { deleteFile } from '@/lib/uploads/core/storage-service' import { UPLOAD_URL_TTL_MS } from '@/lib/uploads/upload-session/provider' -const PAGE_SIZE = 500 +/** Most keys one {@link listStorageKeyPage} call returns. */ +export const STORAGE_KEY_PAGE_SIZE = 500 const CLOCK_SKEW_MS = 60_000 const DEADLINE_MARGIN_MS = 5000 @@ -55,7 +56,7 @@ async function localKeys(prefix: string, signal: AbortSignal): Promise const child = `${key}${entry.name}` if (entry.isDirectory()) await visit(`${child}/`) else keys.push(child) - if (keys.length >= PAGE_SIZE) return + if (keys.length >= STORAGE_KEY_PAGE_SIZE) return } await removeEmptyDirectory(join(UPLOAD_DIR_SERVER, key)) } @@ -94,7 +95,7 @@ export async function listStorageKeyPage( new ListObjectsV2Command({ Bucket: config.bucket, Prefix: prefix, - MaxKeys: PAGE_SIZE, + MaxKeys: STORAGE_KEY_PAGE_SIZE, ...(pageToken ? { ContinuationToken: pageToken } : {}), }), { abortSignal: signal } @@ -111,7 +112,10 @@ export async function listStorageKeyPage( const pages = (await getBlobServiceClient()) .getContainerClient(config.containerName) .listBlobsFlat({ prefix, abortSignal: signal }) - .byPage({ maxPageSize: PAGE_SIZE, ...(pageToken ? { continuationToken: pageToken } : {}) }) + .byPage({ + maxPageSize: STORAGE_KEY_PAGE_SIZE, + ...(pageToken ? { continuationToken: pageToken } : {}), + }) const page = await pages.next() if (page.done) return { keys: [], done: true } return keyPage( @@ -125,7 +129,7 @@ export async function listStorageKeyPage( if (!config.bucket) throw new Error(`Bucket missing for ${context} storage`) const [files, nextQuery] = await (await getGcsClient()).bucket(config.bucket).getFiles({ prefix, - maxResults: PAGE_SIZE, + maxResults: STORAGE_KEY_PAGE_SIZE, autoPaginate: false, ...(pageToken ? { pageToken } : {}), }) @@ -138,7 +142,7 @@ export async function listStorageKeyPage( ) } const keys = await localKeys(prefix, signal) - return { keys, done: keys.length < PAGE_SIZE } + return { keys, done: keys.length < STORAGE_KEY_PAGE_SIZE } } /** diff --git a/apps/sim/lib/users/account-deletion.ts b/apps/sim/lib/users/account-deletion.ts index 8a83d0004a0..87a70813ccc 100644 --- a/apps/sim/lib/users/account-deletion.ts +++ b/apps/sim/lib/users/account-deletion.ts @@ -55,7 +55,10 @@ import { import type { StorageContext } from '@/lib/uploads' import { isUsingCloudStorage, StorageService } from '@/lib/uploads' import { createStorageReferenceGuard } from '@/lib/users/storage-reference-guard' -import { enqueueAccountStorageCleanupInTx } from '@/lib/users/storage-sweep' +import { + collectAccountStorageObjects, + enqueueAccountStorageCleanupInTx, +} from '@/lib/users/storage-sweep' import { listSharedResourceWorkspaceIdsForUser, listUnresolvedSharedResourceWorkspaces, @@ -794,6 +797,10 @@ export async function deleteUserAccount(userId: string): Promise workspace.id) const doomed = new Set(doomedWorkspaceIds) const storageKeys = await collectAccountStorageKeys(userId, doomedWorkspaceIds) + const durableStorageObjects = await collectAccountStorageObjects({ + userId, + workspaceIds: doomedWorkspaceIds, + }) let cancelledDispatches: CancelledDispatch[] = [] let cancelledMarkers: CancelledCellMarker[] = [] @@ -877,11 +884,12 @@ export async function deleteUserAccount(userId: string): Promise 0) { /** * Re-checked here rather than trusted from the plan: a workspace that diff --git a/apps/sim/lib/users/storage-reference-guard.ts b/apps/sim/lib/users/storage-reference-guard.ts index cc3fad52b1d..8344f50680d 100644 --- a/apps/sim/lib/users/storage-reference-guard.ts +++ b/apps/sim/lib/users/storage-reference-guard.ts @@ -156,7 +156,8 @@ async function loadUrlReferencedKeys(needles: readonly string[] | null): Promise * * - a row still names it: `workspace_files.key`, `workspace_file.key`, * `workspace_file_version.key`, `document.storage_key`, `execution_large_values.key`; or - * - it sits under a world-readable root (`isPublicStorageKey`) and a stored asset URL names it (see + * - it sits under a world-readable root (`isPublicStorageKey`, the same prefixes the file-serve + * route's `isPublicByKeyPrefix` answers without a session) and a stored asset URL names it (see * {@link loadUrlReferencedKeys}). Only rows containing the key's longest URL-safe run are read, * each run at most once per guard. No URL column is read for any other key: * `verifyFileAccess` serves it only through a row, by its key path under a workspace that still diff --git a/apps/sim/lib/users/storage-sweep.test.ts b/apps/sim/lib/users/storage-sweep.test.ts index 27c2021c3bd..301151bb762 100644 --- a/apps/sim/lib/users/storage-sweep.test.ts +++ b/apps/sim/lib/users/storage-sweep.test.ts @@ -21,6 +21,7 @@ import type { OutboxEventContext } from '@/lib/core/outbox/service' import type { DbTransaction } from '@/lib/db/types' import { accountStorageSweepOutboxHandlers, + collectAccountStorageObjects, enqueueAccountStorageCleanupInTx, } from '@/lib/users/storage-sweep' import { @@ -360,6 +361,15 @@ describe('queueing account storage cleanup', () => { resetDbChainMock() }) + async function queueCleanup(): Promise { + const objects = await collectAccountStorageObjects({ userId: 'user-1', workspaceIds: [] }) + await enqueueAccountStorageCleanupInTx(dbChainMock.db as unknown as DbTransaction, { + userId: 'user-1', + workspaceIds: [], + objects, + }) + } + function queuedObjectKeys(): string[] { return dbChainMockFns.values.mock.calls.flatMap(([rows]) => (rows as { payload: { objects?: { key: string }[] } }[]).flatMap( @@ -381,26 +391,36 @@ describe('queueing account storage cleanup', () => { queueTableRows(schemaMock.workspaceFileVersion, versionPage(0, 1000)) queueTableRows(schemaMock.workspaceFileVersion, versionPage(1000, 5)) - await enqueueAccountStorageCleanupInTx(dbChainMock.db as unknown as DbTransaction, { - userId: 'user-1', - workspaceIds: [], - }) + await queueCleanup() const keys = queuedObjectKeys() expect(keys).toHaveLength(1006) expect(keys).toContain('copilot/abc/v1004.txt') }) + it('only inserts inside the deletion transaction', async () => { + queueTableRows(schemaMock.workspaceFiles, [ + { id: 'file-1', key: 'copilot/abc/file.txt', context: 'copilot' }, + ]) + const objects = await collectAccountStorageObjects({ userId: 'user-1', workspaceIds: ['ws1'] }) + const insertOnly = { insert: dbChainMock.db.insert } as unknown as DbTransaction + + await enqueueAccountStorageCleanupInTx(insertOnly, { + userId: 'user-1', + workspaceIds: ['ws1'], + objects, + }) + + expect(queuedObjectKeys()).toEqual(['copilot/abc/file.txt']) + }) + it('never queues a profile picture row', async () => { queueTableRows(schemaMock.workspaceFiles, [ { id: 'file-1', key: 'profile-pictures/org-logo.png', context: 'profile-pictures' }, { id: 'file-2', key: 'copilot/abc/file.txt', context: 'copilot' }, ]) - await enqueueAccountStorageCleanupInTx(dbChainMock.db as unknown as DbTransaction, { - userId: 'user-1', - workspaceIds: [], - }) + await queueCleanup() expect(queuedObjectKeys()).toEqual(['copilot/abc/file.txt']) }) diff --git a/apps/sim/lib/users/storage-sweep.ts b/apps/sim/lib/users/storage-sweep.ts index 9c698ff4b61..d65fc477a88 100644 --- a/apps/sim/lib/users/storage-sweep.ts +++ b/apps/sim/lib/users/storage-sweep.ts @@ -365,80 +365,59 @@ interface ObjectRow { context: string | null } -/** Batches recorded objects into delayed cleanup events inside the deletion transaction. */ -class ObjectRecorder { - private recorded = 0 - private skipped = 0 - private capped = false - - constructor( - private readonly tx: DbTransaction, - private readonly userId: string, - private readonly availableAt: Date - ) {} - - get full(): boolean { - return this.capped - } +/** Exact keys an account deletion hands to durable cleanup, bounded by {@link MAX_DURABLE_OBJECTS}. */ +interface AccountStorageObjects { + objects: StorageObject[] + skippedContexts: number + capped: boolean +} + +class ObjectCollector implements AccountStorageObjects { + readonly objects: StorageObject[] = [] + skippedContexts = 0 + capped = false - async record(rows: readonly ObjectRow[]): Promise { - const objects: StorageObject[] = [] + add(rows: readonly ObjectRow[]): void { for (const row of rows) { if (!row.key || !isDisposableKey(row.key)) continue if (!row.context || !OBJECT_CONTEXTS.has(row.context)) { - this.skipped++ + this.skippedContexts++ continue } - if (this.recorded + objects.length >= MAX_DURABLE_OBJECTS) { + if (this.objects.length >= MAX_DURABLE_OBJECTS) { this.capped = true - break + return } - objects.push({ key: row.key, context: row.context as StorageContext }) + this.objects.push({ key: row.key, context: row.context as StorageContext }) } - this.recorded += objects.length - const payloads = chunkArray(objects, OBJECTS_PER_EVENT).map((chunk) => ({ - version: 1, - userId: this.userId, - objects: chunk, - })) - for (const batch of chunkArray(payloads, MAX_BULK_ENQUEUE_EVENTS)) { - await enqueueOutboxEvents(this.tx, ACCOUNT_STORAGE_OBJECT_CLEANUP_EVENT, batch, { - availableAt: this.availableAt, - }) - } - } - - summary() { - return { recorded: this.recorded, skippedContexts: this.skipped, capped: this.capped } } } -async function recordPages( - recorder: ObjectRecorder, +async function collectPages( + collector: ObjectCollector, page: (afterId: string) => Promise, afterPage?: (rows: ObjectRow[]) => Promise ): Promise { let afterId = '' - while (!recorder.full) { + while (!collector.capped) { const rows = await page(afterId) if (rows.length === 0) return - await recorder.record(rows) + collector.add(rows) if (afterPage) await afterPage(rows) if (rows.length < ROW_PAGE_SIZE) return afterId = rows[rows.length - 1].id } } -async function recordVersionsOf( - tx: DbTransaction, - recorder: ObjectRecorder, +async function collectVersionsOf( + collector: ObjectCollector, files: readonly ObjectRow[] ): Promise { if (files.length === 0) return const contexts = new Map(files.map((file) => [file.id, file.context])) const fileIds = files.map((file) => file.id) - await recordPages(recorder, (afterId) => - tx + await collectPages(collector, (afterId) => + db .select({ id: workspaceFileVersion.id, key: workspaceFileVersion.key, @@ -461,49 +440,30 @@ async function recordVersionsOf( } /** - * Queues the durable storage cleanup for an account deletion, inside its transaction and after - * the workspace rows are locked, so the work commits or rolls back with the deletion and no row - * can join a doomed workspace unseen. Takes no locks of its own. - * - * - One prefix sweep per deleted workspace, covering every object under its keyed families - * whether or not a row ever named it. - * - Exact keys for the rest of what cascades away: objects in deleted workspaces outside those - * prefixes (knowledge-base documents, Copilot uploads), and the account's private files - * elsewhere — chat attachments of its own chats, private contexts, and Copilot files with no - * workspace. Files handed to a successor, Project and organization files, attachments of - * somebody else's chat, profile pictures, and workspace-less rows of any other context (a - * legacy shared row can lack its workspace) are never queued. + * Reads the exact keys an account deletion hands to durable cleanup, before its transaction, so + * these unbounded keyset reads never run while the deletion holds workspace locks that + * teammates' writes in shared workspaces wait on. Covers what cascades away outside the + * workspace prefix sweeps: objects in deleted workspaces outside those prefixes (knowledge-base + * documents, Copilot uploads), and the account's private files elsewhere — chat attachments of + * its own chats, private contexts, and Copilot files with no workspace. Files handed to a + * successor, Project and organization files, attachments of somebody else's chat, profile + * pictures, and workspace-less rows of any other context (a legacy shared row can lack its + * workspace) are never collected. * - * Both wait out every upload URL signed before the deletion and re-check at run time that the - * owner is still gone and that {@link createStorageReferenceGuard} does not keep the key; the - * exact-key cleanup also keeps any key under a workspace that still exists. + * A row written between this read and the deletion's locks is missed, which only leaves its + * object behind: the writers that stay active through a deletion — running executions, table + * imports — write under the swept prefixes, which list storage instead of rows. */ -export async function enqueueAccountStorageCleanupInTx( - tx: DbTransaction, - params: { userId: string; workspaceIds: readonly string[] } -): Promise { +export async function collectAccountStorageObjects(params: { + userId: string + workspaceIds: readonly string[] +}): Promise { const { userId } = params - const availableAt = storageCleanupAvailableAt() const workspaceIds = params.workspaceIds.filter((id) => SAFE_ID.test(id)) - if (workspaceIds.length !== params.workspaceIds.length) { - logger.error('Skipping storage sweeps for workspaces with unsafe ids', { - userId, - skipped: params.workspaceIds.length - workspaceIds.length, - }) - } - for (const batch of chunkArray(workspaceIds, MAX_BULK_ENQUEUE_EVENTS)) { - await enqueueOutboxEvents( - tx, - ACCOUNT_WORKSPACE_STORAGE_SWEEP_EVENT, - batch.map((workspaceId) => ({ version: 1, workspaceId })), - { availableAt } - ) - } - - const recorder = new ObjectRecorder(tx, userId, availableAt) + const collector = new ObjectCollector() if (workspaceIds.length > 0) { - await recordPages(recorder, (afterId) => - tx + await collectPages(collector, (afterId) => + db .select({ id: workspaceFiles.id, key: workspaceFiles.key, context: workspaceFiles.context }) .from(workspaceFiles) .where( @@ -517,8 +477,8 @@ export async function enqueueAccountStorageCleanupInTx( .orderBy(workspaceFiles.id) .limit(ROW_PAGE_SIZE) ) - await recordPages(recorder, (afterId) => - tx + await collectPages(collector, (afterId) => + db .select({ id: workspaceFileVersion.id, key: workspaceFileVersion.key, @@ -537,8 +497,8 @@ export async function enqueueAccountStorageCleanupInTx( .orderBy(workspaceFileVersion.id) .limit(ROW_PAGE_SIZE) ) - await recordPages(recorder, (afterId) => - tx + await collectPages(collector, (afterId) => + db .select({ id: workspaceFile.id, key: workspaceFile.key, context: sql`'workspace'` }) .from(workspaceFile) .where( @@ -551,8 +511,8 @@ export async function enqueueAccountStorageCleanupInTx( .orderBy(workspaceFile.id) .limit(ROW_PAGE_SIZE) ) - await recordPages(recorder, (afterId) => - tx + await collectPages(collector, (afterId) => + db .select({ id: document.id, key: document.storageKey, @@ -573,14 +533,14 @@ export async function enqueueAccountStorageCleanupInTx( ) } - const ownChats = tx + const ownChats = db .select({ id: copilotChats.id }) .from(copilotChats) .where(eq(copilotChats.userId, userId)) - await recordPages( - recorder, + await collectPages( + collector, (afterId) => - tx + db .select({ id: workspaceFiles.id, key: workspaceFiles.key, context: workspaceFiles.context }) .from(workspaceFiles) .where( @@ -616,19 +576,63 @@ export async function enqueueAccountStorageCleanupInTx( ) .orderBy(workspaceFiles.id) .limit(ROW_PAGE_SIZE), - (files) => recordVersionsOf(tx, recorder, files) + (files) => collectVersionsOf(collector, files) ) - const summary = recorder.summary() - if (summary.capped) { + if (collector.capped) { logger.error('Account storage cleanup hit its key ceiling; the remainder is left', { userId, cap: MAX_DURABLE_OBJECTS, }) } + return collector +} + +/** + * Queues the durable storage cleanup for an account deletion inside its transaction, so the work + * commits or rolls back with the deletion: one prefix sweep per deleted workspace, covering every + * object under its keyed families whether or not a row ever named it, and delayed events for the + * exact keys {@link collectAccountStorageObjects} read beforehand. Only inserts; reads nothing + * and takes no locks of its own. + * + * Both wait out every upload URL signed before the deletion and re-check at run time that the + * owner is still gone and that {@link createStorageReferenceGuard} does not keep the key; the + * exact-key cleanup also keeps any key under a workspace that still exists. + */ +export async function enqueueAccountStorageCleanupInTx( + tx: DbTransaction, + params: { userId: string; workspaceIds: readonly string[]; objects: AccountStorageObjects } +): Promise { + const { userId, objects } = params + const availableAt = storageCleanupAvailableAt() + const workspaceIds = params.workspaceIds.filter((id) => SAFE_ID.test(id)) + if (workspaceIds.length !== params.workspaceIds.length) { + logger.error('Skipping storage sweeps for workspaces with unsafe ids', { + userId, + skipped: params.workspaceIds.length - workspaceIds.length, + }) + } + for (const batch of chunkArray(workspaceIds, MAX_BULK_ENQUEUE_EVENTS)) { + await enqueueOutboxEvents( + tx, + ACCOUNT_WORKSPACE_STORAGE_SWEEP_EVENT, + batch.map((workspaceId) => ({ version: 1, workspaceId })), + { availableAt } + ) + } + const payloads = chunkArray(objects.objects, OBJECTS_PER_EVENT).map((chunk) => ({ + version: 1, + userId, + objects: chunk, + })) + for (const batch of chunkArray(payloads, MAX_BULK_ENQUEUE_EVENTS)) { + await enqueueOutboxEvents(tx, ACCOUNT_STORAGE_OBJECT_CLEANUP_EVENT, batch, { availableAt }) + } logger.info('Queued account storage cleanup', { userId, workspaceSweeps: workspaceIds.length, - ...summary, + recorded: objects.objects.length, + skippedContexts: objects.skippedContexts, + capped: objects.capped, }) } From 3aa8b9fefbaf1b2290407d9d886a484e8251daed Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Fri, 16 Oct 2026 16:18:19 -0700 Subject: [PATCH 06/11] 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 --- apps/sim/lib/uploads/core/prefix-sweep.ts | 51 +++- apps/sim/lib/users/storage-reference-guard.ts | 86 ++---- .../lib/users/storage-sweep.integration.ts | 18 +- apps/sim/lib/users/storage-sweep.test.ts | 48 ++-- apps/sim/lib/users/storage-sweep.ts | 244 ++++++++++++------ 5 files changed, 275 insertions(+), 172 deletions(-) diff --git a/apps/sim/lib/uploads/core/prefix-sweep.ts b/apps/sim/lib/uploads/core/prefix-sweep.ts index 267dee8230b..40a183ee3dd 100644 --- a/apps/sim/lib/uploads/core/prefix-sweep.ts +++ b/apps/sim/lib/uploads/core/prefix-sweep.ts @@ -1,7 +1,8 @@ -import { opendir, rmdir } from 'node:fs/promises' +import { readdir, rmdir } from 'node:fs/promises' import { dirname, join } from 'node:path' import { describeError } from '@sim/utils/errors' import { isRecordLike } from '@sim/utils/object' +import { compareStrings } from '@sim/utils/string' import type { OutboxEventContext } from '@/lib/core/outbox/service' import { getStorageConfig, @@ -40,22 +41,50 @@ async function removeEmptyDirectory(path: string): Promise { } } -async function localKeys(prefix: string, signal: AbortSignal): Promise { +/** Orders keys path segment by segment, the order a sorted depth-first walk visits them in. */ +function compareKeyPaths(left: readonly string[], right: readonly string[]): number { + for (let index = 0; index < Math.min(left.length, right.length); index++) { + const order = compareStrings(left[index], right[index]) + if (order !== 0) return order + } + return left.length - right.length +} + +/** + * The first local keys under `prefix` in {@link compareKeyPaths} order, after `startAfter` when + * given, so a caller can page past objects it keeps. Prunes directories it walks to the end empty. + */ +async function localKeys( + prefix: string, + signal: AbortSignal, + startAfter?: string +): Promise { + const cursor = startAfter?.split('/') const keys: string[] = [] async function visit(key: string): Promise { signal.throwIfAborted() - let directory + let entries try { - directory = await opendir(join(UPLOAD_DIR_SERVER, key)) + entries = await readdir(join(UPLOAD_DIR_SERVER, key), { withFileTypes: true }) } catch (error) { if (describeError(error).code === 'ENOENT') return throw error } - for await (const entry of directory) { + entries.sort((left, right) => compareStrings(left.name, right.name)) + for (const entry of entries) { signal.throwIfAborted() const child = `${key}${entry.name}` - if (entry.isDirectory()) await visit(`${child}/`) - else keys.push(child) + if (entry.isDirectory()) { + const segments = child.split('/') + if ( + !cursor || + segments.every((segment, index) => cursor[index] === segment) || + compareKeyPaths(segments, cursor) > 0 + ) + await visit(`${child}/`) + } else if (!cursor || compareKeyPaths(child.split('/'), cursor) > 0) { + keys.push(child) + } if (keys.length >= STORAGE_KEY_PAGE_SIZE) return } await removeEmptyDirectory(join(UPLOAD_DIR_SERVER, key)) @@ -77,8 +106,8 @@ function keyPage(keys: string[], nextPageToken: string | undefined): StorageKeyP } /** - * Lists one page of object keys under `prefix` in the bucket serving `context`. A local listing - * has no cursor: it returns the first keys still on disk, so callers advance by deleting them. + * Lists one page of object keys under `prefix` in the bucket serving `context`. A local listing's + * page token is the page's last key, so a caller keeping objects still advances past them. */ export async function listStorageKeyPage( prefix: string, @@ -141,8 +170,8 @@ export async function listStorageKeyPage( : undefined ) } - const keys = await localKeys(prefix, signal) - return { keys, done: keys.length < STORAGE_KEY_PAGE_SIZE } + const keys = await localKeys(prefix, signal, pageToken) + return keyPage(keys, keys.length < STORAGE_KEY_PAGE_SIZE ? undefined : keys[keys.length - 1]) } /** diff --git a/apps/sim/lib/users/storage-reference-guard.ts b/apps/sim/lib/users/storage-reference-guard.ts index 8344f50680d..174464782f4 100644 --- a/apps/sim/lib/users/storage-reference-guard.ts +++ b/apps/sim/lib/users/storage-reference-guard.ts @@ -12,14 +12,10 @@ import { workspaceFileVersion, workspace as workspaceTable, } from '@sim/db/schema' -import { chunkArray } from '@sim/utils/helpers' import { isRecordLike } from '@sim/utils/object' -import { inArray, isNotNull, like, or, type SQL, type SQLWrapper, sql } from 'drizzle-orm' +import { inArray, isNotNull, like, or } from 'drizzle-orm' import { extractStorageKey, isPublicStorageKey } from '@/lib/uploads/utils/file' -const MIN_NEEDLE_LENGTH = 4 -const NEEDLES_PER_QUERY = 50 - /** Every string inside a URL column or a JSON settings blob, including JSON held as a string. */ function collectStrings(value: unknown, into: string[]): void { if (typeof value === 'string') { @@ -81,54 +77,40 @@ function keysNamedByUrl(value: string): string[] { return [...keys] } -/** - * The longest run of URL-safe characters after a key's root. Percent-encoding never splits such a - * run, so any stored URL naming the key contains it; `null` when the key has none long enough. - */ -function urlNeedle(key: string): string | null { - let best = '' - for (const run of key.slice(key.indexOf('/') + 1).match(/[A-Za-z0-9_-]+/g) ?? []) - if (run.length > best.length) best = run - return best.length >= MIN_NEEDLE_LENGTH ? best : null -} - /** * Keys named by the columns that hold a stored asset as a URL rather than a row: * `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`. With `needles`, reads only rows whose value contains one; - * without, every row that holds a URL. + * and avatar), and `oauth_client.icon`. Every non-empty value is read and decoded here rather than + * matched in SQL, since a URL may percent-encode any character of the key it names. Only + * `user.image`, mostly external OAuth avatars, is narrowed, to values that point at this app's + * file-serve route or name a world-readable root. */ -async function loadUrlReferencedKeys(needles: readonly string[] | null): Promise> { - const matching = (column: SQLWrapper, fallback: SQL | undefined): SQL | undefined => - needles ? or(...needles.map((needle) => sql`${column}::text LIKE ${`%${needle}%`}`)) : fallback +async function loadUrlReferencedKeys(): Promise> { const values: string[] = [] const sources = [ db .select({ value: user.image }) .from(user) .where( - matching( - user.image, - or(like(user.image, '%/api/files/serve/%'), like(user.image, '%profile-pictures/%')) + or( + like(user.image, '%/api/files/serve/%'), + like(user.image, '%profile-pictures%'), + like(user.image, '%og-images%'), + like(user.image, '%workspace-logos%'), + like(user.image, '%organization-logos%') ) ), db .select({ value: workspaceTable.logoUrl }) .from(workspaceTable) - .where(matching(workspaceTable.logoUrl, isNotNull(workspaceTable.logoUrl))), + .where(isNotNull(workspaceTable.logoUrl)), db .select({ value: customBlock.iconUrl }) .from(customBlock) - .where(matching(customBlock.iconUrl, isNotNull(customBlock.iconUrl))), - db - .select({ value: chat.customizations }) - .from(chat) - .where(matching(chat.customizations, sql`${chat.customizations}::text LIKE '%Url%'`)), - db - .select({ value: oauthClient.icon }) - .from(oauthClient) - .where(matching(oauthClient.icon, isNotNull(oauthClient.icon))), + .where(isNotNull(customBlock.iconUrl)), + db.select({ value: chat.customizations }).from(chat).where(isNotNull(chat.customizations)), + db.select({ value: oauthClient.icon }).from(oauthClient).where(isNotNull(oauthClient.icon)), ] for (const source of sources) for (const row of await source) collectStrings(row.value, values) const organizations = await db @@ -140,9 +122,9 @@ async function loadUrlReferencedKeys(needles: readonly string[] | null): Promise .from(organization) .where( or( - matching(organization.logo, isNotNull(organization.logo)), - matching(organization.whitelabelSettings, isNotNull(organization.whitelabelSettings)), - matching(organization.metadata, isNotNull(organization.metadata)) + isNotNull(organization.logo), + isNotNull(organization.whitelabelSettings), + isNotNull(organization.metadata) ) ) for (const row of organizations) collectStrings([row.logo, row.whitelabel, row.metadata], values) @@ -158,8 +140,8 @@ async function loadUrlReferencedKeys(needles: readonly string[] | null): Promise * `workspace_file_version.key`, `document.storage_key`, `execution_large_values.key`; or * - it sits under a world-readable root (`isPublicStorageKey`, the same prefixes the file-serve * route's `isPublicByKeyPrefix` answers without a session) and a stored asset URL names it (see - * {@link loadUrlReferencedKeys}). Only rows containing the key's longest URL-safe run are read, - * each run at most once per guard. No URL column is read for any other key: + * {@link loadUrlReferencedKeys}), read at most once per guard and only once a public key comes + * up. No URL column is read for any other key: * `verifyFileAccess` serves it only through a row, by its key path under a workspace that still * exists (which exact-key cleanup keeps; see `keysUnderLiveWorkspaces`), or by object metadata * to a signed-in holder of a hand-pasted URL to a private file this deletion means to erase. @@ -171,25 +153,7 @@ async function loadUrlReferencedKeys(needles: readonly string[] | null): Promise * attachments that is the intended erasure. A failed lookup throws, so nothing is deleted. */ export function createStorageReferenceGuard(): (keys: readonly string[]) => Promise> { - const loadedNeedles = new Set() - const named = new Set() - let loadedAll = false - const loadNamedKeys = async (publicKeys: readonly string[]) => { - if (loadedAll || publicKeys.length === 0) return - const needles = publicKeys.map(urlNeedle) - if (needles.includes(null)) { - for (const key of await loadUrlReferencedKeys(null)) named.add(key) - loadedAll = true - return - } - const pending = [...new Set(needles.flatMap((needle) => needle ?? []))].filter( - (needle) => !loadedNeedles.has(needle) - ) - for (const batch of chunkArray(pending, NEEDLES_PER_QUERY)) { - for (const key of await loadUrlReferencedKeys(batch)) named.add(key) - for (const needle of batch) loadedNeedles.add(needle) - } - } + let named: Set | null = null return async (keys) => { const referenced = new Set() if (keys.length === 0) return referenced @@ -218,8 +182,10 @@ export function createStorageReferenceGuard(): (keys: readonly string[]) => Prom ] for (const row of rows) if (row.key) referenced.add(row.key) const publicKeys = list.filter(isPublicStorageKey) - await loadNamedKeys(publicKeys) - for (const key of publicKeys) if (named.has(key)) referenced.add(key) + if (publicKeys.length > 0) { + named ??= await loadUrlReferencedKeys() + for (const key of publicKeys) if (named.has(key)) referenced.add(key) + } return referenced } } diff --git a/apps/sim/lib/users/storage-sweep.integration.ts b/apps/sim/lib/users/storage-sweep.integration.ts index 5d9bb0a6d52..5519fb6b3cb 100644 --- a/apps/sim/lib/users/storage-sweep.integration.ts +++ b/apps/sim/lib/users/storage-sweep.integration.ts @@ -103,6 +103,14 @@ async function insertAdminWorkspace(id: string, ownerId: string) { }) } +/** A legal URL for `key` that shares no literal run with it, as a hand-written link can be. */ +function percentEncodeEveryCharacter(key: string): string { + return Array.from( + key, + (character) => `%${character.charCodeAt(0).toString(16).padStart(2, '0')}` + ).join('') +} + function fileRow(values: { key: string userId: string @@ -112,7 +120,7 @@ function fileRow(values: { }) { return { id: generateId(), - originalName: 'fixture.txt', + originalName: `fixture-${generateId()}.txt`, contentType: 'text/plain', sizeBytes: 1, chatId: values.chatId ?? null, @@ -154,6 +162,7 @@ async function seed() { legacyWorkspacelessKb: `kb/${generateId()}-legacy.txt`, profilePictureRowLogo: `profile-pictures/${generateId()}/org-logo.png`, logoNamedByUrl: `workspace-logos/${doomedId}/${generateId()}-org-logo.png`, + logoNamedByEncodedUrl: `workspace-logos/${doomedId}/${generateId()}-workspace-logo.png`, } await db.insert(workspaceFiles).values([ fileRow({ @@ -217,6 +226,12 @@ async function seed() { .update(user) .set({ image: `/api/files/serve/${encodeURIComponent(keys.logoNamedByUrl)}` }) .where(eq(user.id, ownerId)) + await db + .update(workspace) + .set({ + logoUrl: `/api/files/serve/s3/${percentEncodeEveryCharacter(keys.logoNamedByEncodedUrl)}`, + }) + .where(eq(workspace.id, survivingId)) for (const key of Object.values(keys)) await put(key) return { ownerId, departingId, doomedId, survivingId, keys } } @@ -304,6 +319,7 @@ describe('account deletion storage sweep', () => { f.keys.legacyWorkspacelessKb, f.keys.profilePictureRowLogo, f.keys.logoNamedByUrl, + f.keys.logoNamedByEncodedUrl, ]) expect(await readFile(join(storageRoot, key), 'utf8')).toBe('x') expect( diff --git a/apps/sim/lib/users/storage-sweep.test.ts b/apps/sim/lib/users/storage-sweep.test.ts index 301151bb762..e617920b849 100644 --- a/apps/sim/lib/users/storage-sweep.test.ts +++ b/apps/sim/lib/users/storage-sweep.test.ts @@ -186,19 +186,31 @@ describe('deleted workspace storage sweep', () => { expect(await exists('workspace/ws1/nested/b.txt.upload-metadata.json')).toBe(false) }) - it('spends an attempt instead of continuing when a full local page removes nothing', async () => { - for (let index = 0; index < 520; index++) - await put(`workspace/ws1/legacy/legacy file ${index}.txt`) + it('pages past a full page of kept objects to the ones behind it', async () => { + const keptKeys = Array.from( + { length: 520 }, + (_, index) => `workspace/ws1/a-kept/${String(index).padStart(3, '0')}.bin` + ) + const unkeptKeys = Array.from( + { length: 10 }, + (_, index) => `workspace/ws1/z-unkept/${index}.bin` + ) + for (const key of [...keptKeys, ...unkeptKeys]) await put(key) + queueTableRows( + schemaMock.document, + keptKeys.map((key) => ({ key })) + ) + queueTableRows( + schemaMock.document, + keptKeys.map((key) => ({ key })) + ) - const result = await sweep( - { version: 1, workspaceId: 'ws1' }, - outboxContext({ deadlineAt: Date.now() + 120_000 }) + await expect(sweep({ version: 1, workspaceId: 'ws1' }, outboxContext())).resolves.toBe( + undefined ) - expect(result).toEqual({ - outcome: 'deferred', - reason: 'Workspace storage sweep removed nothing from a full listing page', - }) + for (const key of unkeptKeys) expect(await exists(key)).toBe(false) + for (const key of keptKeys) expect(await exists(key)).toBe(true) }) it.each([ @@ -398,20 +410,22 @@ describe('queueing account storage cleanup', () => { expect(keys).toContain('copilot/abc/v1004.txt') }) - it('only inserts inside the deletion transaction', async () => { + it('queues a file committed between the early read and the deletion locks, once', async () => { + const early = { id: 'file-1', key: 'copilot/abc/early.txt', context: 'copilot' } + queueTableRows(schemaMock.workspaceFiles, [early]) + const objects = await collectAccountStorageObjects({ userId: 'user-1', workspaceIds: [] }) queueTableRows(schemaMock.workspaceFiles, [ - { id: 'file-1', key: 'copilot/abc/file.txt', context: 'copilot' }, + early, + { id: 'file-2', key: 'copilot/abc/late.txt', context: 'copilot' }, ]) - const objects = await collectAccountStorageObjects({ userId: 'user-1', workspaceIds: ['ws1'] }) - const insertOnly = { insert: dbChainMock.db.insert } as unknown as DbTransaction - await enqueueAccountStorageCleanupInTx(insertOnly, { + await enqueueAccountStorageCleanupInTx(dbChainMock.db as unknown as DbTransaction, { userId: 'user-1', - workspaceIds: ['ws1'], + workspaceIds: [], objects, }) - expect(queuedObjectKeys()).toEqual(['copilot/abc/file.txt']) + expect(queuedObjectKeys()).toEqual(['copilot/abc/early.txt', 'copilot/abc/late.txt']) }) it('never queues a profile picture row', async () => { diff --git a/apps/sim/lib/users/storage-sweep.ts b/apps/sim/lib/users/storage-sweep.ts index d65fc477a88..cd00d438b9b 100644 --- a/apps/sim/lib/users/storage-sweep.ts +++ b/apps/sim/lib/users/storage-sweep.ts @@ -17,6 +17,7 @@ import { and, eq, gt, + gte, inArray, isNotNull, isNull, @@ -35,7 +36,7 @@ import { type OutboxHandler, type OutboxHandlerRegistry, } from '@/lib/core/outbox/service' -import type { DbTransaction } from '@/lib/db/types' +import type { DbOrTx, DbTransaction } from '@/lib/db/types' import type { StorageContext } from '@/lib/uploads/config' import { deleteStorageKeysUnderPrefix, @@ -223,9 +224,10 @@ function sidecarOwner(key: string): string | null { } /** - * Walks one prefix to its end, deleting every listed object the reference guard does not keep. - * Cloud listings follow the provider cursor within a run; a local listing restarts from the first - * key still on disk, so a page that removes nothing is `stalled` rather than re-listed forever. + * Walks one prefix to its end, deleting every listed object the reference guard does not keep, + * following the listing's page token within a run so kept objects never hold back later ones. A + * page that neither advances the token nor removes anything is `stalled` rather than re-listed + * forever. */ async function sweepWorkspacePrefix( prefix: string, @@ -255,10 +257,11 @@ async function sweepWorkspacePrefix( deleted += result.deleted if (!result.complete) return { deleted, complete: false, stalled: false } if (page.done) return { deleted, complete: true, stalled: false } - if (page.nextPageToken) { + const advanced = page.nextPageToken !== undefined && page.nextPageToken !== pageToken + if (advanced) { pageToken = page.nextPageToken } else if (result.deleted === 0) { - logger.error('Workspace storage sweep removed nothing from a full local page', { prefix }) + logger.error('Workspace storage sweep made no progress on a full listing page', { prefix }) return { deleted, complete: false, stalled: true } } } @@ -365,18 +368,40 @@ interface ObjectRow { context: string | null } -/** Exact keys an account deletion hands to durable cleanup, bounded by {@link MAX_DURABLE_OBJECTS}. */ +/** + * How far before its read the pre-transaction collection's in-transaction delta looks back, so + * clock skew between this host and the database, and an insert whose transaction began earlier, + * cannot slip between the two reads. Re-reading a row only collects its key once. + */ +const DELTA_LOOKBACK_MS = 5 * 60_000 + +/** + * Exact keys an account deletion hands to durable cleanup, bounded by {@link MAX_DURABLE_OBJECTS}, + * and the moment from which the in-transaction delta must look for rows the read missed. + */ interface AccountStorageObjects { - objects: StorageObject[] - skippedContexts: number - capped: boolean + readonly objects: readonly StorageObject[] + readonly skippedContexts: number + readonly capped: boolean + readonly since: Date } class ObjectCollector implements AccountStorageObjects { readonly objects: StorageObject[] = [] + private readonly seen = new Set() skippedContexts = 0 capped = false + constructor(readonly since: Date) {} + + static from(collected: AccountStorageObjects): ObjectCollector { + const collector = new ObjectCollector(collected.since) + collector.skippedContexts = collected.skippedContexts + collector.capped = collected.capped + for (const object of collected.objects) collector.push(object) + return collector + } + add(rows: readonly ObjectRow[]): void { for (const row of rows) { if (!row.key || !isDisposableKey(row.key)) continue @@ -384,13 +409,19 @@ class ObjectCollector implements AccountStorageObjects { this.skippedContexts++ continue } + if (this.seen.has(row.key)) continue if (this.objects.length >= MAX_DURABLE_OBJECTS) { this.capped = true return } - this.objects.push({ key: row.key, context: row.context as StorageContext }) + this.push({ key: row.key, context: row.context as StorageContext }) } } + + private push(object: StorageObject): void { + this.seen.add(object.key) + this.objects.push(object) + } } async function collectPages( @@ -410,6 +441,7 @@ async function collectPages( } async function collectVersionsOf( + executor: DbOrTx, collector: ObjectCollector, files: readonly ObjectRow[] ): Promise { @@ -417,7 +449,7 @@ async function collectVersionsOf( const contexts = new Map(files.map((file) => [file.id, file.context])) const fileIds = files.map((file) => file.id) await collectPages(collector, (afterId) => - db + executor .select({ id: workspaceFileVersion.id, key: workspaceFileVersion.key, @@ -439,37 +471,38 @@ async function collectVersionsOf( ) } -/** - * Reads the exact keys an account deletion hands to durable cleanup, before its transaction, so - * these unbounded keyset reads never run while the deletion holds workspace locks that - * teammates' writes in shared workspaces wait on. Covers what cascades away outside the - * workspace prefix sweeps: objects in deleted workspaces outside those prefixes (knowledge-base - * documents, Copilot uploads), and the account's private files elsewhere — chat attachments of - * its own chats, private contexts, and Copilot files with no workspace. Files handed to a - * successor, Project and organization files, attachments of somebody else's chat, profile - * pictures, and workspace-less rows of any other context (a legacy shared row can lack its - * workspace) are never collected. - * - * A row written between this read and the deletion's locks is missed, which only leaves its - * object behind: the writers that stay active through a deletion — running executions, table - * imports — write under the swept prefixes, which list storage instead of rows. - */ -export async function collectAccountStorageObjects(params: { +interface CollectionScope { userId: string workspaceIds: readonly string[] -}): Promise { - const { userId } = params - const workspaceIds = params.workspaceIds.filter((id) => SAFE_ID.test(id)) - const collector = new ObjectCollector() + /** Set for the in-transaction delta: only the account's rows written since then. */ + since?: Date +} + +/** + * The exact-key reads behind {@link collectAccountStorageObjects}. With `since`, each read is + * narrowed to the account's own rows written from then on (`workspace_files.user_id`, + * `document.uploaded_by`, both indexed), and the doomed workspaces' version and legacy-file + * tables are skipped: a content write in a workspace stores its bytes under `workspace//`, + * which the prefix sweep lists. + */ +async function collectInto( + executor: DbOrTx, + collector: ObjectCollector, + { userId, workspaceIds, since }: CollectionScope +): Promise { + const filesSince = since + ? and(eq(workspaceFiles.userId, userId), gte(workspaceFiles.uploadedAt, since)) + : undefined if (workspaceIds.length > 0) { await collectPages(collector, (afterId) => - db + executor .select({ id: workspaceFiles.id, key: workspaceFiles.key, context: workspaceFiles.context }) .from(workspaceFiles) .where( and( inArray(workspaceFiles.workspaceId, workspaceIds), isNull(workspaceFiles.projectId), + filesSince, sql`NOT ${coveredByWorkspaceSweep(workspaceFiles.key, workspaceFiles.workspaceId)}`, gt(workspaceFiles.id, afterId) ) @@ -477,42 +510,48 @@ export async function collectAccountStorageObjects(params: { .orderBy(workspaceFiles.id) .limit(ROW_PAGE_SIZE) ) - await collectPages(collector, (afterId) => - db - .select({ - id: workspaceFileVersion.id, - key: workspaceFileVersion.key, - context: workspaceFiles.context, - }) - .from(workspaceFileVersion) - .innerJoin(workspaceFiles, eq(workspaceFiles.id, workspaceFileVersion.fileId)) - .where( - and( - inArray(workspaceFileVersion.workspaceId, workspaceIds), - isNull(workspaceFiles.projectId), - sql`NOT ${coveredByWorkspaceSweep(workspaceFileVersion.key, workspaceFileVersion.workspaceId)}`, - gt(workspaceFileVersion.id, afterId) + if (!since) { + await collectPages(collector, (afterId) => + executor + .select({ + id: workspaceFileVersion.id, + key: workspaceFileVersion.key, + context: workspaceFiles.context, + }) + .from(workspaceFileVersion) + .innerJoin(workspaceFiles, eq(workspaceFiles.id, workspaceFileVersion.fileId)) + .where( + and( + inArray(workspaceFileVersion.workspaceId, workspaceIds), + isNull(workspaceFiles.projectId), + sql`NOT ${coveredByWorkspaceSweep(workspaceFileVersion.key, workspaceFileVersion.workspaceId)}`, + gt(workspaceFileVersion.id, afterId) + ) ) - ) - .orderBy(workspaceFileVersion.id) - .limit(ROW_PAGE_SIZE) - ) - await collectPages(collector, (afterId) => - db - .select({ id: workspaceFile.id, key: workspaceFile.key, context: sql`'workspace'` }) - .from(workspaceFile) - .where( - and( - inArray(workspaceFile.workspaceId, workspaceIds), - sql`NOT ${coveredByWorkspaceSweep(workspaceFile.key, workspaceFile.workspaceId)}`, - gt(workspaceFile.id, afterId) + .orderBy(workspaceFileVersion.id) + .limit(ROW_PAGE_SIZE) + ) + await collectPages(collector, (afterId) => + executor + .select({ + id: workspaceFile.id, + key: workspaceFile.key, + context: sql`'workspace'`, + }) + .from(workspaceFile) + .where( + and( + inArray(workspaceFile.workspaceId, workspaceIds), + sql`NOT ${coveredByWorkspaceSweep(workspaceFile.key, workspaceFile.workspaceId)}`, + gt(workspaceFile.id, afterId) + ) ) - ) - .orderBy(workspaceFile.id) - .limit(ROW_PAGE_SIZE) - ) + .orderBy(workspaceFile.id) + .limit(ROW_PAGE_SIZE) + ) + } await collectPages(collector, (afterId) => - db + executor .select({ id: document.id, key: document.storageKey, @@ -524,6 +563,9 @@ export async function collectAccountStorageObjects(params: { and( inArray(knowledgeBase.workspaceId, workspaceIds), isNotNull(document.storageKey), + since + ? and(eq(document.uploadedBy, userId), gte(document.uploadedAt, since)) + : undefined, sql`NOT ${coveredByWorkspaceSweep(document.storageKey, knowledgeBase.workspaceId)}`, gt(document.id, afterId) ) @@ -533,19 +575,20 @@ export async function collectAccountStorageObjects(params: { ) } - const ownChats = db + const ownChats = executor .select({ id: copilotChats.id }) .from(copilotChats) .where(eq(copilotChats.userId, userId)) await collectPages( collector, (afterId) => - db + executor .select({ id: workspaceFiles.id, key: workspaceFiles.key, context: workspaceFiles.context }) .from(workspaceFiles) .where( and( eq(workspaceFiles.userId, userId), + filesSince, isNull(workspaceFiles.projectId), isNull(workspaceFiles.organizationId), workspaceIds.length > 0 @@ -576,15 +619,37 @@ export async function collectAccountStorageObjects(params: { ) .orderBy(workspaceFiles.id) .limit(ROW_PAGE_SIZE), - (files) => collectVersionsOf(collector, files) + (files) => collectVersionsOf(executor, collector, files) ) +} - if (collector.capped) { - logger.error('Account storage cleanup hit its key ceiling; the remainder is left', { - userId, - cap: MAX_DURABLE_OBJECTS, - }) - } +function sweepableWorkspaceIds(workspaceIds: readonly string[]): string[] { + return workspaceIds.filter((id) => SAFE_ID.test(id)) +} + +/** + * Reads the exact keys an account deletion hands to durable cleanup, before its transaction, so + * these unbounded keyset reads never run while the deletion holds workspace locks that + * teammates' writes in shared workspaces wait on. Covers what cascades away outside the + * workspace prefix sweeps: objects in deleted workspaces outside those prefixes (knowledge-base + * documents, Copilot uploads), and the account's private files elsewhere — chat attachments of + * its own chats, private contexts, and Copilot files with no workspace. Files handed to a + * successor, Project and organization files, attachments of somebody else's chat, profile + * pictures, and workspace-less rows of any other context (a legacy shared row can lack its + * workspace) are never collected. + * + * A row committed after this read is picked up by the narrow delta + * {@link enqueueAccountStorageCleanupInTx} reads under the deletion's locks. + */ +export async function collectAccountStorageObjects(params: { + userId: string + workspaceIds: readonly string[] +}): Promise { + const collector = new ObjectCollector(new Date(Date.now() - DELTA_LOOKBACK_MS)) + await collectInto(db, collector, { + userId: params.userId, + workspaceIds: sweepableWorkspaceIds(params.workspaceIds), + }) return collector } @@ -592,8 +657,10 @@ export async function collectAccountStorageObjects(params: { * Queues the durable storage cleanup for an account deletion inside its transaction, so the work * commits or rolls back with the deletion: one prefix sweep per deleted workspace, covering every * object under its keyed families whether or not a row ever named it, and delayed events for the - * exact keys {@link collectAccountStorageObjects} read beforehand. Only inserts; reads nothing - * and takes no locks of its own. + * exact keys {@link collectAccountStorageObjects} read beforehand plus a delta of the account's + * own rows written since. The delta runs after the account row is locked, so no further row + * naming the account can commit; it reads through the `user_id` and `uploaded_by` indexes and + * takes no locks of its own. * * Both wait out every upload URL signed before the deletion and re-check at run time that the * owner is still gone and that {@link createStorageReferenceGuard} does not keep the key; the @@ -603,9 +670,9 @@ export async function enqueueAccountStorageCleanupInTx( tx: DbTransaction, params: { userId: string; workspaceIds: readonly string[]; objects: AccountStorageObjects } ): Promise { - const { userId, objects } = params + const { userId } = params const availableAt = storageCleanupAvailableAt() - const workspaceIds = params.workspaceIds.filter((id) => SAFE_ID.test(id)) + const workspaceIds = sweepableWorkspaceIds(params.workspaceIds) if (workspaceIds.length !== params.workspaceIds.length) { logger.error('Skipping storage sweeps for workspaces with unsafe ids', { userId, @@ -620,7 +687,17 @@ export async function enqueueAccountStorageCleanupInTx( { availableAt } ) } - const payloads = chunkArray(objects.objects, OBJECTS_PER_EVENT).map((chunk) => ({ + + const collector = ObjectCollector.from(params.objects) + const collected = collector.objects.length + await collectInto(tx, collector, { userId, workspaceIds, since: collector.since }) + if (collector.capped) { + logger.error('Account storage cleanup hit its key ceiling; the remainder is left', { + userId, + cap: MAX_DURABLE_OBJECTS, + }) + } + const payloads = chunkArray(collector.objects, OBJECTS_PER_EVENT).map((chunk) => ({ version: 1, userId, objects: chunk, @@ -631,8 +708,9 @@ export async function enqueueAccountStorageCleanupInTx( logger.info('Queued account storage cleanup', { userId, workspaceSweeps: workspaceIds.length, - recorded: objects.objects.length, - skippedContexts: objects.skippedContexts, - capped: objects.capped, + recorded: collector.objects.length, + fromDelta: collector.objects.length - collected, + skippedContexts: collector.skippedContexts, + capped: collector.capped, }) } From c3e5e65e70f2230866334a4a29f907cc49b74edc Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Fri, 16 Oct 2026 16:27:11 -0700 Subject: [PATCH 07/11] docs(account-deletion): describe the in-transaction delta read accurately --- apps/sim/lib/users/account-deletion.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/sim/lib/users/account-deletion.ts b/apps/sim/lib/users/account-deletion.ts index 87a70813ccc..235b9b3b740 100644 --- a/apps/sim/lib/users/account-deletion.ts +++ b/apps/sim/lib/users/account-deletion.ts @@ -884,7 +884,7 @@ export async function deleteUserAccount(userId: string): Promise Date: Fri, 16 Oct 2026 16:45:00 -0700 Subject: [PATCH 08/11] 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 --- apps/sim/lib/users/account-deletion.test.ts | 2 +- apps/sim/lib/users/account-deletion.ts | 46 +---- apps/sim/lib/users/storage-reference-guard.ts | 187 ++++++++++++------ .../lib/users/storage-sweep.integration.ts | 10 +- apps/sim/lib/users/storage-sweep.test.ts | 180 ++++++++++------- apps/sim/lib/users/storage-sweep.ts | 128 ++++++++---- 6 files changed, 345 insertions(+), 208 deletions(-) diff --git a/apps/sim/lib/users/account-deletion.test.ts b/apps/sim/lib/users/account-deletion.test.ts index 1991d10b78a..1b67261fbb1 100644 --- a/apps/sim/lib/users/account-deletion.test.ts +++ b/apps/sim/lib/users/account-deletion.test.ts @@ -2,10 +2,10 @@ import { describe, expect, it } from 'vitest' import { type AccountDeletionFacts, classifyAccountDeletion, - extractProfilePictureKey, type WorkspaceCompany, type WorkspaceRow, } from '@/lib/users/account-deletion' +import { extractProfilePictureKey } from '@/lib/users/storage-sweep' function workspace(overrides: Partial = {}): WorkspaceRow { return { id: 'ws-1', name: 'My workspace', organizationId: null, ...overrides } diff --git a/apps/sim/lib/users/account-deletion.ts b/apps/sim/lib/users/account-deletion.ts index 235b9b3b740..89c70597874 100644 --- a/apps/sim/lib/users/account-deletion.ts +++ b/apps/sim/lib/users/account-deletion.ts @@ -54,6 +54,7 @@ import { } from '@/lib/table/rows/executions' import type { StorageContext } from '@/lib/uploads' import { isUsingCloudStorage, StorageService } from '@/lib/uploads' +import { isPublicStorageKey } from '@/lib/uploads/utils/file' import { createStorageReferenceGuard } from '@/lib/users/storage-reference-guard' import { collectAccountStorageObjects, @@ -497,31 +498,6 @@ function countKeys(batches: StorageKeyBatch[]): number { return total } -/** - * The account's own uploaded avatar, as a storage key. - * - * `user.image` holds either a `/api/files/serve/...` path (an upload we own) or - * an absolute URL from an OAuth provider (which we must not try to delete). Only - * the former yields a key, and only under the `profile-pictures/` prefix — the - * image is personal data, so an erasure that leaves it in the bucket is not an - * erasure. - */ -export function extractProfilePictureKey(image: string | null): string | null { - if (!image) return null - try { - const parsed = new URL(image, 'http://placeholder') - if (parsed.origin !== 'http://placeholder') return null - const segments = parsed.pathname.split('/') - if (segments[1] !== 'api' || segments[2] !== 'files' || segments[3] !== 'serve') return null - let keySegments = segments.slice(4) - if (['s3', 'blob', 'gcs'].includes(keySegments[0])) keySegments = keySegments.slice(1) - const key = decodeURIComponent(keySegments.join('/')) - return key.startsWith('profile-pictures/') ? key : null - } catch { - return null - } -} - /** * Collects the account's private images and stored objects in workspaces that go with it. * @@ -560,16 +536,6 @@ async function collectAccountStorageKeys( if (!isUsingCloudStorage()) return batches - const [profile] = await db - .select({ image: user.image }) - .from(user) - .where(eq(user.id, userId)) - .limit(1) - const profilePictureKey = extractProfilePictureKey(profile?.image ?? null) - if (profilePictureKey) { - batches.push({ context: 'profile-pictures', keys: [profilePictureKey] }) - } - if (workspaceIds.length === 0) return batches await collectPages( @@ -643,13 +609,15 @@ async function collectAccountStorageKeys( * failure for work that cannot be undone. An orphaned object is recoverable from * the log; a deletion the caller believes failed is not. * - * Keys a surviving row or stored asset URL still names are kept (a legacy `kb/` - * object can back documents in more than one workspace); see - * `createStorageReferenceGuard`. + * Keys a surviving row still names are kept (a legacy `kb/` object can back + * documents in more than one workspace); see `createStorageReferenceGuard`. + * World-readable keys are left to the delayed durable cleanup, whose guard also + * reads stored asset URLs, so this request never does. */ async function purgeStorageObjects(userId: string, batches: StorageKeyBatch[]): Promise { const isReferenced = createStorageReferenceGuard() - for (const { context, keys: collected } of batches) { + for (const { context, keys: listed } of batches) { + const collected = listed.filter((key) => !isPublicStorageKey(key)) if (collected.length === 0) continue try { const referenced = await isReferenced(collected) diff --git a/apps/sim/lib/users/storage-reference-guard.ts b/apps/sim/lib/users/storage-reference-guard.ts index 174464782f4..f4829a6b5c3 100644 --- a/apps/sim/lib/users/storage-reference-guard.ts +++ b/apps/sim/lib/users/storage-reference-guard.ts @@ -12,8 +12,9 @@ import { workspaceFileVersion, workspace as workspaceTable, } from '@sim/db/schema' +import { chunkArray } from '@sim/utils/helpers' import { isRecordLike } from '@sim/utils/object' -import { inArray, isNotNull, like, or } from 'drizzle-orm' +import { and, gt, inArray, or, type SQL, type SQLWrapper, sql } from 'drizzle-orm' import { extractStorageKey, isPublicStorageKey } from '@/lib/uploads/utils/file' /** Every string inside a URL column or a JSON settings blob, including JSON held as a string. */ @@ -77,60 +78,122 @@ function keysNamedByUrl(value: string): string[] { return [...keys] } +const MIN_OWNER_RUN_LENGTH = 8 +const OWNER_RUNS_PER_QUERY = 50 +const URL_ROW_PAGE_SIZE = 500 + +/** + * The run of `[A-Za-z0-9_-]` that opens the segment after a public key's root: the owning + * workspace or organization id (`workspace-logos//...`), or the upload id a flat key starts + * with (`profile-pictures/-`). `encodeURIComponent` and `encodeURI` never escape those + * characters, so every URL this app writes for the key holds the run literally. `null` when the + * run is too short to narrow a lookup. + */ +function ownerRun(key: string): string | null { + const run = /^[A-Za-z0-9_-]+/.exec(key.split('/')[1] ?? '')?.[0] ?? '' + return run.length >= MIN_OWNER_RUN_LENGTH ? run : null +} + +type UrlRowPage = ( + mentions: (column: SQLWrapper) => SQL, + afterId: string +) => Promise<{ id: string; values: unknown[] }[]> + /** - * Keys named by the columns that hold a stored asset as a URL rather than a row: - * `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`. Every non-empty value is read and decoded here rather than - * matched in SQL, since a URL may percent-encode any character of the key it names. Only - * `user.image`, mostly external OAuth avatars, is narrowed, to values that point at this app's - * file-serve route or name a world-readable root. + * The columns that hold a stored asset as a URL rather than a row: `user.image`, + * `workspace.logo_url`, `custom_block.icon_url`, `chat.customizations` (deployed-chat logo and + * avatar), `oauth_client.icon`, and `organization.logo`, `.whitelabel_settings`, `.metadata`. */ -async function loadUrlReferencedKeys(): Promise> { - const values: string[] = [] - const sources = [ +const URL_ROW_PAGES: readonly UrlRowPage[] = [ + (mentions, afterId) => db - .select({ value: user.image }) + .select({ id: user.id, value: user.image }) .from(user) - .where( - or( - like(user.image, '%/api/files/serve/%'), - like(user.image, '%profile-pictures%'), - like(user.image, '%og-images%'), - like(user.image, '%workspace-logos%'), - like(user.image, '%organization-logos%') - ) - ), + .where(and(mentions(user.image), gt(user.id, afterId))) + .orderBy(user.id) + .limit(URL_ROW_PAGE_SIZE) + .then((rows) => rows.map((row) => ({ id: row.id, values: [row.value] }))), + (mentions, afterId) => db - .select({ value: workspaceTable.logoUrl }) + .select({ id: workspaceTable.id, value: workspaceTable.logoUrl }) .from(workspaceTable) - .where(isNotNull(workspaceTable.logoUrl)), + .where(and(mentions(workspaceTable.logoUrl), gt(workspaceTable.id, afterId))) + .orderBy(workspaceTable.id) + .limit(URL_ROW_PAGE_SIZE) + .then((rows) => rows.map((row) => ({ id: row.id, values: [row.value] }))), + (mentions, afterId) => db - .select({ value: customBlock.iconUrl }) + .select({ id: customBlock.id, value: customBlock.iconUrl }) .from(customBlock) - .where(isNotNull(customBlock.iconUrl)), - db.select({ value: chat.customizations }).from(chat).where(isNotNull(chat.customizations)), - db.select({ value: oauthClient.icon }).from(oauthClient).where(isNotNull(oauthClient.icon)), - ] - for (const source of sources) for (const row of await source) collectStrings(row.value, values) - const organizations = await db - .select({ - logo: organization.logo, - whitelabel: organization.whitelabelSettings, - metadata: organization.metadata, - }) - .from(organization) - .where( - or( - isNotNull(organization.logo), - isNotNull(organization.whitelabelSettings), - isNotNull(organization.metadata) + .where(and(mentions(customBlock.iconUrl), gt(customBlock.id, afterId))) + .orderBy(customBlock.id) + .limit(URL_ROW_PAGE_SIZE) + .then((rows) => rows.map((row) => ({ id: row.id, values: [row.value] }))), + (mentions, afterId) => + db + .select({ id: chat.id, value: chat.customizations }) + .from(chat) + .where(and(mentions(chat.customizations), gt(chat.id, afterId))) + .orderBy(chat.id) + .limit(URL_ROW_PAGE_SIZE) + .then((rows) => rows.map((row) => ({ id: row.id, values: [row.value] }))), + (mentions, afterId) => + db + .select({ id: oauthClient.id, value: oauthClient.icon }) + .from(oauthClient) + .where(and(mentions(oauthClient.icon), gt(oauthClient.id, afterId))) + .orderBy(oauthClient.id) + .limit(URL_ROW_PAGE_SIZE) + .then((rows) => rows.map((row) => ({ id: row.id, values: [row.value] }))), + (mentions, afterId) => + db + .select({ + id: organization.id, + logo: organization.logo, + whitelabel: organization.whitelabelSettings, + metadata: organization.metadata, + }) + .from(organization) + .where( + and( + or( + mentions(organization.logo), + mentions(organization.whitelabelSettings), + mentions(organization.metadata) + ), + gt(organization.id, afterId) + ) ) - ) - for (const row of organizations) collectStrings([row.logo, row.whitelabel, row.metadata], values) - const keys = new Set() - for (const value of values) for (const key of keysNamedByUrl(value)) keys.add(key) - return keys + .orderBy(organization.id) + .limit(URL_ROW_PAGE_SIZE) + .then((rows) => + rows.map((row) => ({ id: row.id, values: [row.logo, row.whitelabel, row.metadata] })) + ), +] + +/** + * Adds to `named` every key a stored asset URL names, reading only rows whose URL columns mention + * one of `runs` (see {@link ownerRun}), page by page. + */ +async function loadKeysNamedByUrls(runs: readonly string[], named: Set): Promise { + const mentions = (column: SQLWrapper): SQL => + sql`(${sql.join( + runs.map((run) => sql`${column}::text LIKE ${`%${run}%`}`), + sql` OR ` + )})` + for (const page of URL_ROW_PAGES) { + let afterId = '' + for (;;) { + const rows = await page(mentions, afterId) + for (const row of rows) { + const values: string[] = [] + collectStrings(row.values, values) + for (const value of values) for (const key of keysNamedByUrl(value)) named.add(key) + } + if (rows.length < URL_ROW_PAGE_SIZE) break + afterId = rows[rows.length - 1].id + } + } } /** @@ -139,12 +202,16 @@ async function loadUrlReferencedKeys(): Promise> { * - a row still names it: `workspace_files.key`, `workspace_file.key`, * `workspace_file_version.key`, `document.storage_key`, `execution_large_values.key`; or * - it sits under a world-readable root (`isPublicStorageKey`, the same prefixes the file-serve - * route's `isPublicByKeyPrefix` answers without a session) and a stored asset URL names it (see - * {@link loadUrlReferencedKeys}), read at most once per guard and only once a public key comes - * up. No URL column is read for any other key: - * `verifyFileAccess` serves it only through a row, by its key path under a workspace that still - * exists (which exact-key cleanup keeps; see `keysUnderLiveWorkspaces`), or by object metadata - * to a signed-in holder of a hand-pasted URL to a private file this deletion means to erase. + * route's `isPublicByKeyPrefix` answers without a session) and a stored asset URL names it. + * Only URL rows that mention the key's {@link ownerRun} are read, each run at most once per + * guard; a public key without one is kept unread. A URL that hand-encodes characters of that + * run is missed, and the run belongs to the workspace, organization, or upload being erased, so + * only an object of the erased account can be lost that way. + * + * No URL column is read for any other key: `verifyFileAccess` serves it only through a row, by its + * key path under a workspace that still exists (which exact-key cleanup keeps; see + * `keysUnderLiveWorkspaces`), or by object metadata to a signed-in holder of a hand-pasted URL to + * a private file this deletion means to erase. * * Every row of a deleted workspace or account is gone when this runs, so a remaining reference * belongs to data that outlived the deletion — a moved knowledge base, a handed-off file, an @@ -153,7 +220,8 @@ async function loadUrlReferencedKeys(): Promise> { * attachments that is the intended erasure. A failed lookup throws, so nothing is deleted. */ export function createStorageReferenceGuard(): (keys: readonly string[]) => Promise> { - let named: Set | null = null + const loadedRuns = new Set() + const named = new Set() return async (keys) => { const referenced = new Set() if (keys.length === 0) return referenced @@ -182,10 +250,17 @@ export function createStorageReferenceGuard(): (keys: readonly string[]) => Prom ] for (const row of rows) if (row.key) referenced.add(row.key) const publicKeys = list.filter(isPublicStorageKey) - if (publicKeys.length > 0) { - named ??= await loadUrlReferencedKeys() - for (const key of publicKeys) if (named.has(key)) referenced.add(key) + const pendingRuns = new Set() + for (const key of publicKeys) { + const run = ownerRun(key) + if (!run) referenced.add(key) + else if (!loadedRuns.has(run)) pendingRuns.add(run) + } + for (const runs of chunkArray([...pendingRuns], OWNER_RUNS_PER_QUERY)) { + await loadKeysNamedByUrls(runs, named) + for (const run of runs) loadedRuns.add(run) } + for (const key of publicKeys) if (named.has(key)) referenced.add(key) return referenced } } diff --git a/apps/sim/lib/users/storage-sweep.integration.ts b/apps/sim/lib/users/storage-sweep.integration.ts index 5519fb6b3cb..b25f3c7fd7b 100644 --- a/apps/sim/lib/users/storage-sweep.integration.ts +++ b/apps/sim/lib/users/storage-sweep.integration.ts @@ -103,14 +103,6 @@ async function insertAdminWorkspace(id: string, ownerId: string) { }) } -/** A legal URL for `key` that shares no literal run with it, as a hand-written link can be. */ -function percentEncodeEveryCharacter(key: string): string { - return Array.from( - key, - (character) => `%${character.charCodeAt(0).toString(16).padStart(2, '0')}` - ).join('') -} - function fileRow(values: { key: string userId: string @@ -229,7 +221,7 @@ async function seed() { await db .update(workspace) .set({ - logoUrl: `/api/files/serve/s3/${percentEncodeEveryCharacter(keys.logoNamedByEncodedUrl)}`, + logoUrl: `/api/files/serve/s3/${encodeURIComponent(keys.logoNamedByEncodedUrl)}`, }) .where(eq(workspace.id, survivingId)) for (const key of Object.values(keys)) await put(key) diff --git a/apps/sim/lib/users/storage-sweep.test.ts b/apps/sim/lib/users/storage-sweep.test.ts index e617920b849..cdd18fc1531 100644 --- a/apps/sim/lib/users/storage-sweep.test.ts +++ b/apps/sim/lib/users/storage-sweep.test.ts @@ -61,21 +61,21 @@ async function exists(key: string): Promise { } const DOOMED_KEYS = [ - 'workspace/ws1/a.txt', - 'workspace/ws1/nested/b.txt', - 'table-import/ws1/import-1/rows.csv', - 'execution/ws1/wf/ex/large-value-lv_1.json', - 'table-snapshots/ws1/table-1/v1-hash.csv', - 'copilot-doc-compiled/ws1/hash.pdf', - 'workspace-logos/ws1/logo.png', + 'workspace/wsdoomed1/a.txt', + 'workspace/wsdoomed1/nested/b.txt', + 'table-import/wsdoomed1/import-1/rows.csv', + 'execution/wsdoomed1/wf/ex/large-value-lv_1.json', + 'table-snapshots/wsdoomed1/table-1/v1-hash.csv', + 'copilot-doc-compiled/wsdoomed1/hash.pdf', + 'workspace-logos/wsdoomed1/logo.png', ] const NEIGHBOR_KEYS = [ - 'workspace/ws10/keep.txt', - 'workspace/ws1-archive/keep.txt', - 'execution/ws10/wf/ex/keep.json', - 'project/ws1/keep.txt', - 'kb/ws1/keep.txt', + 'workspace/wsdoomed10/keep.txt', + 'workspace/wsdoomed1-archive/keep.txt', + 'execution/wsdoomed10/wf/ex/keep.json', + 'project/wsdoomed1/keep.txt', + 'kb/wsdoomed1/keep.txt', ] describe('deleted workspace storage sweep', () => { @@ -91,19 +91,19 @@ describe('deleted workspace storage sweep', () => { }) it('deletes every family under the workspace prefix and nothing under a neighboring id', async () => { - await expect(sweep({ version: 1, workspaceId: 'ws1' }, outboxContext())).resolves.toBe( + await expect(sweep({ version: 1, workspaceId: 'wsdoomed1' }, outboxContext())).resolves.toBe( undefined ) for (const key of DOOMED_KEYS) expect(await exists(key)).toBe(false) for (const key of NEIGHBOR_KEYS) expect(await exists(key)).toBe(true) - expect(await exists('workspace/ws1')).toBe(false) + expect(await exists('workspace/wsdoomed1')).toBe(false) }) it('touches nothing while a workspace row with that id exists', async () => { - queueTableRows(schemaMock.workspace, [{ id: 'ws1' }]) + queueTableRows(schemaMock.workspace, [{ id: 'wsdoomed1' }]) - await expect(sweep({ version: 1, workspaceId: 'ws1' }, outboxContext())).resolves.toBe( + await expect(sweep({ version: 1, workspaceId: 'wsdoomed1' }, outboxContext())).resolves.toBe( undefined ) @@ -111,19 +111,35 @@ describe('deleted workspace storage sweep', () => { }) it('keeps an object a surviving row still references', async () => { - queueTableRows(schemaMock.document, [{ key: 'workspace/ws1/a.txt' }]) + queueTableRows(schemaMock.document, [{ key: 'workspace/wsdoomed1/a.txt' }]) - await sweep({ version: 1, workspaceId: 'ws1' }, outboxContext()) + await sweep({ version: 1, workspaceId: 'wsdoomed1' }, outboxContext()) - expect(await exists('workspace/ws1/a.txt')).toBe(true) - expect(await exists('workspace/ws1/nested/b.txt')).toBe(false) + expect(await exists('workspace/wsdoomed1/a.txt')).toBe(true) + expect(await exists('workspace/wsdoomed1/nested/b.txt')).toBe(false) + }) + + it('stops deleting once the workspace reappears mid-sweep', async () => { + const keys = Array.from({ length: 510 }, (_, index) => `workspace/wsdoomed1/bulk/${index}.bin`) + for (const key of keys) await put(key) + queueTableRows(schemaMock.workspace, []) + queueTableRows(schemaMock.workspace, []) + queueTableRows(schemaMock.workspace, [{ id: 'wsdoomed1' }]) + + await expect(sweep({ version: 1, workspaceId: 'wsdoomed1' }, outboxContext())).resolves.toBe( + undefined + ) + + const remaining = await Promise.all(keys.map(exists)) + expect(remaining.filter(Boolean).length).toBeGreaterThan(0) + expect(await exists('workspace-logos/wsdoomed1/logo.png')).toBe(true) }) it('walks past a full listing page within one run', async () => { - const keys = Array.from({ length: 510 }, (_, index) => `workspace/ws1/bulk/${index}.bin`) + const keys = Array.from({ length: 510 }, (_, index) => `workspace/wsdoomed1/bulk/${index}.bin`) for (const key of keys) await put(key) - await expect(sweep({ version: 1, workspaceId: 'ws1' }, outboxContext())).resolves.toBe( + await expect(sweep({ version: 1, workspaceId: 'wsdoomed1' }, outboxContext())).resolves.toBe( undefined ) @@ -131,11 +147,11 @@ describe('deleted workspace storage sweep', () => { }) it('resumes from its checkpointed family', async () => { - await sweep({ version: 1, workspaceId: 'ws1', family: 2 }, outboxContext()) + await sweep({ version: 1, workspaceId: 'wsdoomed1', family: 2 }, outboxContext()) - expect(await exists('workspace/ws1/a.txt')).toBe(true) - expect(await exists('table-import/ws1/import-1/rows.csv')).toBe(true) - expect(await exists('execution/ws1/wf/ex/large-value-lv_1.json')).toBe(false) + expect(await exists('workspace/wsdoomed1/a.txt')).toBe(true) + expect(await exists('table-import/wsdoomed1/import-1/rows.csv')).toBe(true) + expect(await exists('execution/wsdoomed1/wf/ex/large-value-lv_1.json')).toBe(false) }) it.each([ @@ -145,7 +161,7 @@ describe('deleted workspace storage sweep', () => { row: { logo: null, whitelabel: { - logoUrl: 'https://sim.test/api/files/serve/s3/workspace-logos%2Fws1%2Flogo.png', + logoUrl: 'https://sim.test/api/files/serve/s3/workspace-logos%2Fwsdoomed1%2Flogo.png', }, metadata: null, }, @@ -153,47 +169,49 @@ describe('deleted workspace storage sweep', () => { { name: 'a custom block icon stored as a direct bucket URL', table: 'customBlock', - row: { value: 'https://bucket.s3.amazonaws.com/workspace-logos/ws1/logo.png' }, + row: { value: 'https://bucket.s3.amazonaws.com/workspace-logos/wsdoomed1/logo.png' }, }, { name: 'a deployed chat avatar', table: 'chat', - row: { value: { imageUrl: '/api/files/serve/workspace-logos/ws1/logo.png?context=chat' } }, + row: { + value: { imageUrl: '/api/files/serve/workspace-logos/wsdoomed1/logo.png?context=chat' }, + }, }, { name: 'another account image', table: 'user', - row: { value: '/api/files/serve/workspace-logos/ws1/logo.png' }, + row: { value: '/api/files/serve/workspace-logos/wsdoomed1/logo.png' }, }, ] as const)('keeps an object named by $name', async ({ table, row }) => { queueTableRows(schemaMock[table], [row]) - await sweep({ version: 1, workspaceId: 'ws1' }, outboxContext()) + await sweep({ version: 1, workspaceId: 'wsdoomed1' }, outboxContext()) - expect(await exists('workspace-logos/ws1/logo.png')).toBe(true) - expect(await exists('workspace/ws1/a.txt')).toBe(false) + expect(await exists('workspace-logos/wsdoomed1/logo.png')).toBe(true) + expect(await exists('workspace/wsdoomed1/a.txt')).toBe(false) }) it('keeps the local metadata sidecar of a kept object', async () => { - await put('workspace/ws1/a.txt.upload-metadata.json', '{}') - await put('workspace/ws1/nested/b.txt.upload-metadata.json', '{}') - queueTableRows(schemaMock.document, [{ key: 'workspace/ws1/a.txt' }]) + await put('workspace/wsdoomed1/a.txt.upload-metadata.json', '{}') + await put('workspace/wsdoomed1/nested/b.txt.upload-metadata.json', '{}') + queueTableRows(schemaMock.document, [{ key: 'workspace/wsdoomed1/a.txt' }]) - await sweep({ version: 1, workspaceId: 'ws1' }, outboxContext()) + await sweep({ version: 1, workspaceId: 'wsdoomed1' }, outboxContext()) - expect(await exists('workspace/ws1/a.txt')).toBe(true) - expect(await exists('workspace/ws1/a.txt.upload-metadata.json')).toBe(true) - expect(await exists('workspace/ws1/nested/b.txt.upload-metadata.json')).toBe(false) + expect(await exists('workspace/wsdoomed1/a.txt')).toBe(true) + expect(await exists('workspace/wsdoomed1/a.txt.upload-metadata.json')).toBe(true) + expect(await exists('workspace/wsdoomed1/nested/b.txt.upload-metadata.json')).toBe(false) }) it('pages past a full page of kept objects to the ones behind it', async () => { const keptKeys = Array.from( { length: 520 }, - (_, index) => `workspace/ws1/a-kept/${String(index).padStart(3, '0')}.bin` + (_, index) => `workspace/wsdoomed1/a-kept/${String(index).padStart(3, '0')}.bin` ) const unkeptKeys = Array.from( { length: 10 }, - (_, index) => `workspace/ws1/z-unkept/${index}.bin` + (_, index) => `workspace/wsdoomed1/z-unkept/${index}.bin` ) for (const key of [...keptKeys, ...unkeptKeys]) await put(key) queueTableRows( @@ -205,7 +223,7 @@ describe('deleted workspace storage sweep', () => { keptKeys.map((key) => ({ key })) ) - await expect(sweep({ version: 1, workspaceId: 'ws1' }, outboxContext())).resolves.toBe( + await expect(sweep({ version: 1, workspaceId: 'wsdoomed1' }, outboxContext())).resolves.toBe( undefined ) @@ -215,10 +233,10 @@ describe('deleted workspace storage sweep', () => { it.each([ { version: 1, workspaceId: '' }, - { version: 1, workspaceId: '../ws1' }, - { version: 1, workspaceId: 'ws1/' }, + { version: 1, workspaceId: '../wsdoomed1' }, + { version: 1, workspaceId: 'wsdoomed1/' }, { version: 1, workspaceId: '*' }, - { workspaceId: 'ws1' }, + { workspaceId: 'wsdoomed1' }, ])('rejects an unbounded or malformed payload %o', async (payload) => { await expect(sweep(payload, outboxContext())).rejects.toThrow() for (const key of DOOMED_KEYS) expect(await exists(key)).toBe(true) @@ -244,20 +262,20 @@ describe('account storage object cleanup', () => { it('deletes unreferenced objects and keeps referenced ones', async () => { await put('copilot/abc/private.txt') - await put('workspace/ws2/handed-off.txt') - queueTableRows(schemaMock.workspaceFiles, [{ key: 'workspace/ws2/handed-off.txt' }]) + await put('workspace/wsliving2/handed-off.txt') + queueTableRows(schemaMock.workspaceFiles, [{ key: 'workspace/wsliving2/handed-off.txt' }]) await cleanup( payload([ { key: 'copilot/abc/private.txt', context: 'copilot' }, - { key: 'workspace/ws2/handed-off.txt', context: 'workspace' }, + { key: 'workspace/wsliving2/handed-off.txt', context: 'workspace' }, { key: 'copilot/abc/already-gone.txt', context: 'copilot' }, ]), outboxContext() ) expect(await exists('copilot/abc/private.txt')).toBe(false) - expect(await readFile(join(root, 'workspace/ws2/handed-off.txt'), 'utf8')).toBe('x') + expect(await readFile(join(root, 'workspace/wsliving2/handed-off.txt'), 'utf8')).toBe('x') }) it('touches nothing while the account exists', async () => { @@ -285,34 +303,55 @@ describe('account storage object cleanup', () => { }) it('keeps a key under a workspace that still exists', async () => { - await put('workspace/ws2/private.txt') + await put('workspace/wsliving2/private.txt') await put('copilot/abc/private.txt') - queueTableRows(schemaMock.workspace, [{ id: 'ws2' }]) + queueTableRows(schemaMock.workspace, [{ id: 'wsliving2' }]) await cleanup( payload([ - { key: 'workspace/ws2/private.txt', context: 'mothership' }, + { key: 'workspace/wsliving2/private.txt', context: 'mothership' }, { key: 'copilot/abc/private.txt', context: 'copilot' }, ]), outboxContext() ) - expect(await exists('workspace/ws2/private.txt')).toBe(true) + expect(await exists('workspace/wsliving2/private.txt')).toBe(true) expect(await exists('copilot/abc/private.txt')).toBe(false) }) it('keeps a publicly served key a stored asset URL names', async () => { - await put('workspace-logos/ws9/icon.png') + await put('workspace-logos/wsforeign9/icon.png') queueTableRows(schemaMock.oauthClient, [ - { value: '/api/files/serve/workspace-logos%2Fws9%2Ficon.png' }, + { value: '/api/files/serve/workspace-logos%2Fwsforeign9%2Ficon.png' }, ]) await cleanup( - payload([{ key: 'workspace-logos/ws9/icon.png', context: 'workspace-logos' }]), + payload([{ key: 'workspace-logos/wsforeign9/icon.png', context: 'workspace-logos' }]), outboxContext() ) - expect(await exists('workspace-logos/ws9/icon.png')).toBe(true) + expect(await exists('workspace-logos/wsforeign9/icon.png')).toBe(true) + }) + + it('keeps a public key without an owner id to narrow on, reading no URL column', async () => { + await put('og-images/logo.png') + + await cleanup(payload([{ key: 'og-images/logo.png', context: 'workspace' }]), outboxContext()) + + expect(await exists('og-images/logo.png')).toBe(true) + for (const table of ['organization', 'customBlock', 'chat', 'oauthClient'] as const) + expect(dbChainMockFns.from).not.toHaveBeenCalledWith(schemaMock[table]) + }) + + it("deletes the account's own uploaded avatar", async () => { + await put('profile-pictures/upload-0001-avatar.png') + + await cleanup( + payload([{ key: 'profile-pictures/upload-0001-avatar.png', context: 'profile-pictures' }]), + outboxContext() + ) + + expect(await exists('profile-pictures/upload-0001-avatar.png')).toBe(false) }) it('reads no URL column for keys that are never served without a row', async () => { @@ -327,25 +366,25 @@ describe('account storage object cleanup', () => { }) it('still purges when a stored URL has malformed percent-encoding', async () => { - await put('workspace-logos/ws9/old.png') + await put('workspace-logos/wsforeign9/old.png') queueTableRows(schemaMock.user, []) queueTableRows(schemaMock.user, [{ value: '/api/files/serve/%E0%A4%A' }]) await cleanup( - payload([{ key: 'workspace-logos/ws9/old.png', context: 'workspace-logos' }]), + payload([{ key: 'workspace-logos/wsforeign9/old.png', context: 'workspace-logos' }]), outboxContext() ) - expect(await exists('workspace-logos/ws9/old.png')).toBe(false) + expect(await exists('workspace-logos/wsforeign9/old.png')).toBe(false) }) it('keeps a legacy key holding a literal percent sign that a malformed URL names', async () => { - const key = 'workspace-logos/ws9/brand logo%.png' - const storedAs = 'workspace-logos/ws9/brand_logo_.png' + const key = 'workspace-logos/wsforeign9/brand logo%.png' + const storedAs = 'workspace-logos/wsforeign9/brand_logo_.png' await put(storedAs) queueTableRows(schemaMock.user, []) queueTableRows(schemaMock.user, [ - { value: '/api/files/serve/workspace-logos/ws9/brand%20logo%.png' }, + { value: '/api/files/serve/workspace-logos/wsforeign9/brand%20logo%.png' }, ]) await cleanup(payload([{ key, context: 'workspace-logos' }]), outboxContext()) @@ -357,8 +396,7 @@ describe('account storage object cleanup', () => { { key: 'project/p1/file.txt', context: 'workspace' }, { key: 'assistant/org/user/file.png', context: 'mothership' }, { key: '../outside.txt', context: 'workspace' }, - { key: 'workspace/ws2/file.txt', context: 'og-images' }, - { key: 'profile-pictures/user-9/logo.png', context: 'profile-pictures' }, + { key: 'workspace/wsliving2/file.txt', context: 'og-images' }, { key: 'profile-pictures/user-9/logo.png', context: 'workspace' }, { key: 'organization-logos/org-1/logo.png', context: 'workspace' }, ])('refuses a batch naming %o', async (object) => { @@ -428,6 +466,16 @@ describe('queueing account storage cleanup', () => { expect(queuedObjectKeys()).toEqual(['copilot/abc/early.txt', 'copilot/abc/late.txt']) }) + it("queues the account's own uploaded avatar", async () => { + queueTableRows(schemaMock.user, [ + { image: '/api/files/serve/s3/profile-pictures%2Fupload-0001-avatar.png' }, + ]) + + await queueCleanup() + + expect(queuedObjectKeys()).toEqual(['profile-pictures/upload-0001-avatar.png']) + }) + it('never queues a profile picture row', async () => { queueTableRows(schemaMock.workspaceFiles, [ { id: 'file-1', key: 'profile-pictures/org-logo.png', context: 'profile-pictures' }, diff --git a/apps/sim/lib/users/storage-sweep.ts b/apps/sim/lib/users/storage-sweep.ts index cd00d438b9b..44dbcd20194 100644 --- a/apps/sim/lib/users/storage-sweep.ts +++ b/apps/sim/lib/users/storage-sweep.ts @@ -92,10 +92,10 @@ const OBJECT_CONTEXTS: ReadonlySet = new Set([ ]) /** - * Keys another lifecycle owns: Project prefix cleanup, the Assistant upload-session sweep, - * organization logos, and profile pictures — which account deletion takes only from the - * account's own `user.image`, because whitelabel logos were once uploaded as `profile-pictures` - * rows under an admin's account. + * Keys no row collected here may name: Project prefix cleanup, the Assistant upload-session + * sweep, organization logos, and profile pictures — which account deletion takes only from the + * account's own `user.image` (see {@link extractProfilePictureKey}), because whitelabel logos were + * once uploaded as `profile-pictures` rows under an admin's account. */ const FOREIGN_LIFECYCLE_ROOTS = [ 'project/', @@ -145,16 +145,42 @@ function parseWorkspaceSweep(value: unknown): WorkspaceSweepPayload { return { workspaceId: value.workspaceId, family } } -function isDisposableKey(key: string): boolean { +function isSafeKey(key: string): boolean { return ( - key.length > 0 && - key.length <= 2048 && - !key.startsWith('/') && - !key.split('/').includes('..') && - !FOREIGN_LIFECYCLE_ROOTS.some((root) => key.startsWith(root)) + key.length > 0 && key.length <= 2048 && !key.startsWith('/') && !key.split('/').includes('..') ) } +function isDisposableKey(key: string): boolean { + return isSafeKey(key) && !FOREIGN_LIFECYCLE_ROOTS.some((root) => key.startsWith(root)) +} + +function isAccountProfilePicture(key: string, context: string): boolean { + return context === 'profile-pictures' && key.startsWith('profile-pictures/') +} + +/** + * The account's own uploaded avatar, as a storage key. `user.image` holds either a + * `/api/files/serve/...` path (an upload this app owns) or an absolute URL from an OAuth provider, + * which must not be deleted; only the former yields a key, and only under `profile-pictures/`. + * The image is personal data, so an erasure that leaves it in the bucket is not an erasure. + */ +export function extractProfilePictureKey(image: string | null): string | null { + if (!image) return null + try { + const parsed = new URL(image, 'http://placeholder') + if (parsed.origin !== 'http://placeholder') return null + const segments = parsed.pathname.split('/') + if (segments[1] !== 'api' || segments[2] !== 'files' || segments[3] !== 'serve') return null + let keySegments = segments.slice(4) + if (['s3', 'blob', 'gcs'].includes(keySegments[0])) keySegments = keySegments.slice(1) + const key = decodeURIComponent(keySegments.join('/')) + return key.startsWith('profile-pictures/') ? key : null + } catch { + return null + } +} + function parseObjectCleanup(value: unknown): ObjectCleanupPayload { if ( !isRecordLike(value) || @@ -169,9 +195,11 @@ function parseObjectCleanup(value: unknown): ObjectCleanupPayload { if ( !isRecordLike(object) || typeof object.key !== 'string' || - !isDisposableKey(object.key) || typeof object.context !== 'string' || - !OBJECT_CONTEXTS.has(object.context) + !( + (isDisposableKey(object.key) && OBJECT_CONTEXTS.has(object.context)) || + (isSafeKey(object.key) && isAccountProfilePicture(object.key, object.context)) + ) ) throw new Error('Invalid account storage cleanup object') return { key: object.key, context: object.context as StorageContext } @@ -233,12 +261,13 @@ async function sweepWorkspacePrefix( prefix: string, storageContext: StorageContext, context: OutboxEventContext, - isReferenced: (keys: readonly string[]) => Promise> -): Promise<{ deleted: number; complete: boolean; stalled: boolean }> { + isReferenced: (keys: readonly string[]) => Promise>, + workspaceExists: () => Promise +): Promise<{ deleted: number; status: 'done' | 'partial' | 'stalled' | 'refused' }> { let deleted = 0 let pageToken: string | undefined for (;;) { - if (outboxDeadlineNear(context)) return { deleted, complete: false, stalled: false } + if (outboxDeadlineNear(context)) return { deleted, status: 'partial' } const page = await listStorageKeyPage(prefix, storageContext, context.signal, pageToken) if (page.keys.some((key) => !key.startsWith(prefix))) throw new Error('Storage provider returned a key outside the workspace prefix') @@ -248,6 +277,7 @@ async function sweepWorkspacePrefix( const keptCount = page.keys.filter(kept).length if (keptCount > 0) logger.warn('Keeping objects a surviving reference still names', { prefix, kept: keptCount }) + if (await workspaceExists()) return { deleted, status: 'refused' } const result = await deleteStorageKeysUnderPrefix( prefix, storageContext, @@ -255,14 +285,14 @@ async function sweepWorkspacePrefix( context ) deleted += result.deleted - if (!result.complete) return { deleted, complete: false, stalled: false } - if (page.done) return { deleted, complete: true, stalled: false } + if (!result.complete) return { deleted, status: 'partial' } + if (page.done) return { deleted, status: 'done' } const advanced = page.nextPageToken !== undefined && page.nextPageToken !== pageToken if (advanced) { pageToken = page.nextPageToken } else if (result.deleted === 0) { logger.error('Workspace storage sweep made no progress on a full listing page', { prefix }) - return { deleted, complete: false, stalled: true } + return { deleted, status: 'stalled' } } } } @@ -270,37 +300,44 @@ async function sweepWorkspacePrefix( /** * Deletes a deleted workspace's objects by prefix in each bucket. A workspace row with the same * id means the database no longer agrees the workspace is gone (a restore, a point-in-time - * recovery), so the sweep completes without touching storage. + * recovery), so the sweep stops without touching storage; it looks again before every page it + * deletes from. */ const sweepWorkspaceStorage: OutboxHandler = async (raw, context) => { const payload = parseWorkspaceSweep(raw) - const [existing] = await db - .select({ id: workspaceTable.id }) - .from(workspaceTable) - .where(eq(workspaceTable.id, payload.workspaceId)) - .limit(1) - if (existing) { - logger.error('Refusing to sweep storage for a workspace that exists', { - workspaceId: payload.workspaceId, - }) - return + const workspaceExists = async () => { + const [existing] = await db + .select({ id: workspaceTable.id }) + .from(workspaceTable) + .where(eq(workspaceTable.id, payload.workspaceId)) + .limit(1) + if (existing) + logger.error('Refusing to sweep storage for a workspace that exists', { + workspaceId: payload.workspaceId, + }) + return Boolean(existing) } + if (await workspaceExists()) return const isReferenced = createStorageReferenceGuard() let progressed = false for (let family = payload.family; family < WORKSPACE_STORAGE_FAMILIES.length; family++) { const { root, context: storageContext } = WORKSPACE_STORAGE_FAMILIES[family] const prefix = workspaceStoragePrefix(root, payload.workspaceId) - const result = await sweepWorkspacePrefix(prefix, storageContext, context, isReferenced) + const result = await sweepWorkspacePrefix( + prefix, + storageContext, + context, + isReferenced, + workspaceExists + ) + if (result.status === 'refused') return if (result.deleted > 0) progressed = true - if (!result.complete) { - if (result.stalled) - return deferOutboxHandler( - 'Workspace storage sweep removed nothing from a full listing page' - ) + if (result.status === 'stalled') + return deferOutboxHandler('Workspace storage sweep made no progress on a full listing page') + if (result.status === 'partial') return progressed ? continueOutboxHandler('Continuing workspace storage sweep') : deferOutboxHandler('Workspace storage sweep made no progress before its deadline') - } await context.checkpointPayload({ family: family + 1 }) progressed = true } @@ -402,6 +439,15 @@ class ObjectCollector implements AccountStorageObjects { return collector } + addProfilePicture(key: string): void { + if (this.seen.has(key)) return + if (this.objects.length >= MAX_DURABLE_OBJECTS) { + this.capped = true + return + } + this.push({ key, context: 'profile-pictures' }) + } + add(rows: readonly ObjectRow[]): void { for (const row of rows) { if (!row.key || !isDisposableKey(row.key)) continue @@ -473,7 +519,7 @@ async function collectVersionsOf( interface CollectionScope { userId: string - workspaceIds: readonly string[] + workspaceIds: string[] /** Set for the in-transaction delta: only the account's rows written since then. */ since?: Date } @@ -490,6 +536,14 @@ async function collectInto( collector: ObjectCollector, { userId, workspaceIds, since }: CollectionScope ): Promise { + const [account] = await executor + .select({ image: user.image }) + .from(user) + .where(eq(user.id, userId)) + .limit(1) + const profilePicture = extractProfilePictureKey(account?.image ?? null) + if (profilePicture) collector.addProfilePicture(profilePicture) + const filesSince = since ? and(eq(workspaceFiles.userId, userId), gte(workspaceFiles.uploadedAt, since)) : undefined From 1d55a45c4aab38d096bf6ecc5d5053684efa8919 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Fri, 16 Oct 2026 16:54:09 -0700 Subject: [PATCH 09/11] 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. --- apps/sim/lib/users/storage-reference-guard.ts | 6 +- .../lib/users/storage-sweep.integration.ts | 58 +++++++++++++++++++ apps/sim/lib/users/storage-sweep.test.ts | 11 ++++ apps/sim/lib/users/storage-sweep.ts | 47 ++++++++++++++- 4 files changed, 117 insertions(+), 5 deletions(-) diff --git a/apps/sim/lib/users/storage-reference-guard.ts b/apps/sim/lib/users/storage-reference-guard.ts index f4829a6b5c3..7339d1d1cd4 100644 --- a/apps/sim/lib/users/storage-reference-guard.ts +++ b/apps/sim/lib/users/storage-reference-guard.ts @@ -205,8 +205,10 @@ async function loadKeysNamedByUrls(runs: readonly string[], named: Set): * route's `isPublicByKeyPrefix` answers without a session) and a stored asset URL names it. * Only URL rows that mention the key's {@link ownerRun} are read, each run at most once per * guard; a public key without one is kept unread. A URL that hand-encodes characters of that - * run is missed, and the run belongs to the workspace, organization, or upload being erased, so - * only an object of the erased account can be lost that way. + * run is missed. Account deletion asks about public keys only under a deleted workspace's + * prefix (its logos) and for an avatar the account is proven to have uploaded, so such a URL can + * cost an object of a deleted workspace or of the erased account's own uploads, never one a + * surviving workspace, organization, or user uploaded. * * No URL column is read for any other key: `verifyFileAccess` serves it only through a row, by its * key path under a workspace that still exists (which exact-key cleanup keeps; see diff --git a/apps/sim/lib/users/storage-sweep.integration.ts b/apps/sim/lib/users/storage-sweep.integration.ts index b25f3c7fd7b..4fb8137d5af 100644 --- a/apps/sim/lib/users/storage-sweep.integration.ts +++ b/apps/sim/lib/users/storage-sweep.integration.ts @@ -7,6 +7,7 @@ import { copilotChats, outboxEvent, permissions, + uploadSession, user, workspace, workspaceFiles, @@ -103,6 +104,30 @@ async function insertAdminWorkspace(id: string, ownerId: string) { }) } +/** The ownership record a finished profile-picture upload leaves behind. */ +function completedAvatarUpload(userId: string, key: string) { + return { + id: generateId(), + tokenHash: generateId(), + userId, + purpose: 'profile_picture' as const, + method: 'put' as const, + storageContext: 'profile-pictures', + finalKey: key, + storageProvider: 'local' as const, + fileName: 'avatar.png', + contentType: 'image/png', + fileSize: 1, + status: 'completed' as const, + expiresAt: new Date(), + completedAt: new Date(), + } +} + +function avatarUrl(key: string): string { + return `/api/files/serve/${encodeURIComponent(key)}?context=profile-pictures` +} + function fileRow(values: { key: string userId: string @@ -155,6 +180,8 @@ async function seed() { profilePictureRowLogo: `profile-pictures/${generateId()}/org-logo.png`, logoNamedByUrl: `workspace-logos/${doomedId}/${generateId()}-org-logo.png`, logoNamedByEncodedUrl: `workspace-logos/${doomedId}/${generateId()}-workspace-logo.png`, + ownAvatar: `profile-pictures/${generateId()}-departing-avatar.png`, + othersAvatar: `profile-pictures/${generateId()}-owner-avatar.png`, } await db.insert(workspaceFiles).values([ fileRow({ @@ -224,6 +251,16 @@ async function seed() { logoUrl: `/api/files/serve/s3/${encodeURIComponent(keys.logoNamedByEncodedUrl)}`, }) .where(eq(workspace.id, survivingId)) + await db + .insert(uploadSession) + .values([ + completedAvatarUpload(departingId, keys.ownAvatar), + completedAvatarUpload(ownerId, keys.othersAvatar), + ]) + await db + .update(user) + .set({ image: avatarUrl(keys.ownAvatar) }) + .where(eq(user.id, departingId)) for (const key of Object.values(keys)) await put(key) return { ownerId, departingId, doomedId, survivingId, keys } } @@ -299,6 +336,7 @@ describe('account deletion storage sweep', () => { f.keys.rowlessSnapshot, f.keys.rowlessCompiled, f.keys.personalCopilot, + f.keys.ownAvatar, lateUpload, ]) expect({ key, exists: await exists(key) }).toEqual({ key, exists: false }) @@ -312,6 +350,7 @@ describe('account deletion storage sweep', () => { f.keys.profilePictureRowLogo, f.keys.logoNamedByUrl, f.keys.logoNamedByEncodedUrl, + f.keys.othersAvatar, ]) expect(await readFile(join(storageRoot, key), 'utf8')).toBe('x') expect( @@ -320,6 +359,24 @@ describe('account deletion storage sweep', () => { } ) + check('never purges an avatar the account points at but did not upload', async () => { + const f = await seed() + await db + .update(user) + .set({ image: avatarUrl(f.keys.othersAvatar) }) + .where(eq(user.id, f.departingId)) + await deleteUserAccount(f.departingId) + + const objects = await objectEvents(f.departingId) + const queuedKeys = objects.flatMap((event) => + (event.payload as { objects: { key: string }[] }).objects.map((object) => object.key) + ) + expect(queuedKeys).not.toContain(f.keys.othersAvatar) + await runNow(objects.map((event) => event.id)) + + expect(await readFile(join(storageRoot, f.keys.othersAvatar), 'utf8')).toBe('x') + }) + check('a sweep for a workspace that exists again completes without deleting', async () => { const f = await seed() await deleteUserAccount(f.departingId) @@ -346,6 +403,7 @@ afterAll(async () => { ) ) await db.delete(workspaceFiles).where(inArray(workspaceFiles.userId, userIds)) + await db.delete(uploadSession).where(inArray(uploadSession.userId, userIds)) await deleteWorkspaceFixture(db, inArray(workspace.id, workspaceIds)) await db.delete(user).where(inArray(user.id, userIds)) await rm(storageRoot, { recursive: true, force: true }) diff --git a/apps/sim/lib/users/storage-sweep.test.ts b/apps/sim/lib/users/storage-sweep.test.ts index cdd18fc1531..8e3294b5979 100644 --- a/apps/sim/lib/users/storage-sweep.test.ts +++ b/apps/sim/lib/users/storage-sweep.test.ts @@ -470,12 +470,23 @@ describe('queueing account storage cleanup', () => { queueTableRows(schemaMock.user, [ { image: '/api/files/serve/s3/profile-pictures%2Fupload-0001-avatar.png' }, ]) + queueTableRows(schemaMock.uploadSession, [{ id: 'upload-0001' }]) await queueCleanup() expect(queuedObjectKeys()).toEqual(['profile-pictures/upload-0001-avatar.png']) }) + it('never queues an avatar key the account did not upload', async () => { + queueTableRows(schemaMock.user, [ + { image: '/api/files/serve/s3/profile-pictures%2Fupload-0002-someone-else.png' }, + ]) + + await queueCleanup() + + expect(queuedObjectKeys()).toEqual([]) + }) + it('never queues a profile picture row', async () => { queueTableRows(schemaMock.workspaceFiles, [ { id: 'file-1', key: 'profile-pictures/org-logo.png', context: 'profile-pictures' }, diff --git a/apps/sim/lib/users/storage-sweep.ts b/apps/sim/lib/users/storage-sweep.ts index 44dbcd20194..7126710e5c8 100644 --- a/apps/sim/lib/users/storage-sweep.ts +++ b/apps/sim/lib/users/storage-sweep.ts @@ -3,6 +3,7 @@ import { copilotChats, document, knowledgeBase, + uploadSession, user, workspaceFile, workspaceFiles, @@ -94,8 +95,9 @@ const OBJECT_CONTEXTS: ReadonlySet = new Set([ /** * Keys no row collected here may name: Project prefix cleanup, the Assistant upload-session * sweep, organization logos, and profile pictures — which account deletion takes only from the - * account's own `user.image` (see {@link extractProfilePictureKey}), because whitelabel logos were - * once uploaded as `profile-pictures` rows under an admin's account. + * account's own `user.image`, and only once {@link uploadedProfilePicture} proves the account + * uploaded it, because whitelabel logos were once uploaded as `profile-pictures` rows under an + * admin's account. */ const FOREIGN_LIFECYCLE_ROOTS = [ 'project/', @@ -517,6 +519,44 @@ async function collectVersionsOf( ) } +/** + * Whether the account itself uploaded `key`. `user.image` is settable to any serve URL, so it + * names a key without proving whose it is; the proof is the account's completed `profile_picture` + * upload session (kept for a week after completion), or the `workspace_files` row an older + * profile-picture upload wrote. Without either the avatar is left in place. + */ +async function uploadedProfilePicture( + executor: DbOrTx, + userId: string, + key: string +): Promise { + const [session] = await executor + .select({ id: uploadSession.id }) + .from(uploadSession) + .where( + and( + eq(uploadSession.finalKey, key), + eq(uploadSession.userId, userId), + eq(uploadSession.purpose, 'profile_picture'), + eq(uploadSession.status, 'completed') + ) + ) + .limit(1) + if (session) return true + const [row] = await executor + .select({ id: workspaceFiles.id }) + .from(workspaceFiles) + .where( + and( + eq(workspaceFiles.key, key), + eq(workspaceFiles.userId, userId), + eq(workspaceFiles.context, 'profile-pictures') + ) + ) + .limit(1) + return Boolean(row) +} + interface CollectionScope { userId: string workspaceIds: string[] @@ -542,7 +582,8 @@ async function collectInto( .where(eq(user.id, userId)) .limit(1) const profilePicture = extractProfilePictureKey(account?.image ?? null) - if (profilePicture) collector.addProfilePicture(profilePicture) + if (profilePicture && (await uploadedProfilePicture(executor, userId, profilePicture))) + collector.addProfilePicture(profilePicture) const filesSince = since ? and(eq(workspaceFiles.userId, userId), gte(workspaceFiles.uploadedAt, since)) From 0e93240dc4c7215f3ed50c127c84b4519fae6c1f Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Fri, 16 Oct 2026 16:57:07 -0700 Subject: [PATCH 10/11] 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 --- apps/sim/lib/users/storage-reference-guard.ts | 28 ++++----- apps/sim/lib/users/storage-sweep.test.ts | 46 ++++++++++++++ apps/sim/lib/users/storage-sweep.ts | 62 +++---------------- apps/sim/lib/users/workspace-storage.ts | 54 ++++++++++++++++ 4 files changed, 121 insertions(+), 69 deletions(-) create mode 100644 apps/sim/lib/users/workspace-storage.ts diff --git a/apps/sim/lib/users/storage-reference-guard.ts b/apps/sim/lib/users/storage-reference-guard.ts index 7339d1d1cd4..e7c210dab28 100644 --- a/apps/sim/lib/users/storage-reference-guard.ts +++ b/apps/sim/lib/users/storage-reference-guard.ts @@ -16,6 +16,7 @@ import { chunkArray } from '@sim/utils/helpers' import { isRecordLike } from '@sim/utils/object' import { and, gt, inArray, or, type SQL, type SQLWrapper, sql } from 'drizzle-orm' import { extractStorageKey, isPublicStorageKey } from '@/lib/uploads/utils/file' +import { keysUnderLiveWorkspaces } from '@/lib/users/workspace-storage' /** Every string inside a URL column or a JSON settings blob, including JSON held as a string. */ function collectStrings(value: unknown, into: string[]): void { @@ -200,19 +201,20 @@ async function loadKeysNamedByUrls(runs: readonly string[], named: Set): * Builds the check every account-deletion purge runs before deleting a key. 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 + * `workspace_file_version.key`, `document.storage_key`, `execution_large_values.key`; + * - it sits under a workspace-keyed root of a workspace that still exists + * ({@link keysUnderLiveWorkspaces}); or * - it sits under a world-readable root (`isPublicStorageKey`, the same prefixes the file-serve * route's `isPublicByKeyPrefix` answers without a session) and a stored asset URL names it. - * Only URL rows that mention the key's {@link ownerRun} are read, each run at most once per - * guard; a public key without one is kept unread. A URL that hand-encodes characters of that + * Only URL rows that mention the key's {@link ownerRun} are read, on every call so a reference + * written mid-sweep still counts; a public key without one is kept unread. A URL that hand-encodes characters of that * run is missed. Account deletion asks about public keys only under a deleted workspace's * prefix (its logos) and for an avatar the account is proven to have uploaded, so such a URL can * cost an object of a deleted workspace or of the erased account's own uploads, never one a * surviving workspace, organization, or user uploaded. * * No URL column is read for any other key: `verifyFileAccess` serves it only through a row, by its - * key path under a workspace that still exists (which exact-key cleanup keeps; see - * `keysUnderLiveWorkspaces`), or by object metadata to a signed-in holder of a hand-pasted URL to + * key path under a workspace that still exists (kept above), or by object metadata to a signed-in holder of a hand-pasted URL to * a private file this deletion means to erase. * * Every row of a deleted workspace or account is gone when this runs, so a remaining reference @@ -222,8 +224,6 @@ async function loadKeysNamedByUrls(runs: readonly string[], named: Set): * attachments that is the intended erasure. A failed lookup throws, so nothing is deleted. */ export function createStorageReferenceGuard(): (keys: readonly string[]) => Promise> { - const loadedRuns = new Set() - const named = new Set() return async (keys) => { const referenced = new Set() if (keys.length === 0) return referenced @@ -252,17 +252,17 @@ export function createStorageReferenceGuard(): (keys: readonly string[]) => Prom ] for (const row of rows) if (row.key) referenced.add(row.key) const publicKeys = list.filter(isPublicStorageKey) - const pendingRuns = new Set() + const runs = new Set() for (const key of publicKeys) { const run = ownerRun(key) - if (!run) referenced.add(key) - else if (!loadedRuns.has(run)) pendingRuns.add(run) - } - for (const runs of chunkArray([...pendingRuns], OWNER_RUNS_PER_QUERY)) { - await loadKeysNamedByUrls(runs, named) - for (const run of runs) loadedRuns.add(run) + if (run) runs.add(run) + else referenced.add(key) } + const named = new Set() + for (const batch of chunkArray([...runs], OWNER_RUNS_PER_QUERY)) + await loadKeysNamedByUrls(batch, named) for (const key of publicKeys) if (named.has(key)) referenced.add(key) + for (const key of await keysUnderLiveWorkspaces(list)) referenced.add(key) return referenced } } diff --git a/apps/sim/lib/users/storage-sweep.test.ts b/apps/sim/lib/users/storage-sweep.test.ts index 8e3294b5979..df6cae253e1 100644 --- a/apps/sim/lib/users/storage-sweep.test.ts +++ b/apps/sim/lib/users/storage-sweep.test.ts @@ -19,6 +19,7 @@ vi.mock('@/lib/uploads/server/metadata', () => uploadsMetadataMock) import type { OutboxEventContext } from '@/lib/core/outbox/service' import type { DbTransaction } from '@/lib/db/types' +import { createStorageReferenceGuard } from '@/lib/users/storage-reference-guard' import { accountStorageSweepOutboxHandlers, collectAccountStorageObjects, @@ -135,6 +136,23 @@ describe('deleted workspace storage sweep', () => { expect(await exists('workspace-logos/wsdoomed1/logo.png')).toBe(true) }) + it('keeps a public object a URL written mid-sweep names', async () => { + const logos = Array.from( + { length: 510 }, + (_, index) => `workspace-logos/wsdoomed1/l/${String(index).padStart(3, '0')}.png` + ) + for (const key of logos) await put(key) + queueTableRows(schemaMock.user, []) + queueTableRows(schemaMock.user, [ + { id: 'user-9', value: '/api/files/serve/workspace-logos%2Fwsdoomed1%2Fl%2F505.png' }, + ]) + + await sweep({ version: 1, workspaceId: 'wsdoomed1' }, outboxContext()) + + expect(await exists('workspace-logos/wsdoomed1/l/505.png')).toBe(true) + expect(await exists('workspace-logos/wsdoomed1/l/504.png')).toBe(false) + }) + it('walks past a full listing page within one run', async () => { const keys = Array.from({ length: 510 }, (_, index) => `workspace/wsdoomed1/bulk/${index}.bin`) for (const key of keys) await put(key) @@ -406,6 +424,23 @@ describe('account storage object cleanup', () => { }) }) +describe('storage reference guard', () => { + beforeEach(() => { + resetDbChainMock() + }) + + it('keeps a key under a workspace that still exists for every caller', async () => { + queueTableRows(schemaMock.workspace, [{ id: 'wsliving2' }]) + + const kept = await createStorageReferenceGuard()([ + 'execution/wsliving2/wf/ex/file.json', + 'execution/wsdoomed1/wf/ex/file.json', + ]) + + expect([...kept]).toEqual(['execution/wsliving2/wf/ex/file.json']) + }) +}) + describe('queueing account storage cleanup', () => { beforeEach(() => { resetDbChainMock() @@ -487,6 +522,17 @@ describe('queueing account storage cleanup', () => { expect(queuedObjectKeys()).toEqual([]) }) + it('never queues an avatar key that could escape its root', async () => { + queueTableRows(schemaMock.user, [ + { image: '/api/files/serve/profile-pictures%2Fx%2F..%2F..%2Fworkspace%2Fother.png' }, + ]) + queueTableRows(schemaMock.uploadSession, [{ id: 'upload-0003' }]) + + await queueCleanup() + + expect(queuedObjectKeys()).toEqual([]) + }) + it('never queues a profile picture row', async () => { queueTableRows(schemaMock.workspaceFiles, [ { id: 'file-1', key: 'profile-pictures/org-logo.png', context: 'profile-pictures' }, diff --git a/apps/sim/lib/users/storage-sweep.ts b/apps/sim/lib/users/storage-sweep.ts index 7126710e5c8..fc63c2fe68a 100644 --- a/apps/sim/lib/users/storage-sweep.ts +++ b/apps/sim/lib/users/storage-sweep.ts @@ -53,6 +53,7 @@ import { ACCOUNT_STORAGE_OBJECT_CLEANUP_EVENT, ACCOUNT_WORKSPACE_STORAGE_SWEEP_EVENT, } from '@/lib/users/storage-sweep-events' +import { STORAGE_ID_PATTERN, WORKSPACE_STORAGE_FAMILIES } from '@/lib/users/workspace-storage' const logger = createLogger('AccountStorageSweep') @@ -65,22 +66,6 @@ const OBJECTS_PER_EVENT = 100 */ const MAX_DURABLE_OBJECTS = 100_000 -const SAFE_ID = /^[A-Za-z0-9_-]{1,128}$/ - -/** - * Every storage family whose keys embed their workspace as the second path segment, in the - * bucket that serves it. A deleted workspace's objects in these families are found by listing, - * so neither a missing row nor a row written after the deletion read its keys can strand one. - */ -const WORKSPACE_STORAGE_FAMILIES = [ - { root: 'workspace', context: 'workspace' }, - { root: 'table-import', context: 'table-import' }, - { root: 'execution', context: 'execution' }, - { root: 'table-snapshots', context: 'execution' }, - { root: 'copilot-doc-compiled', context: 'copilot' }, - { root: 'workspace-logos', context: 'workspace-logos' }, -] as const satisfies readonly { root: string; context: StorageContext }[] - const OBJECT_CONTEXTS: ReadonlySet = new Set([ 'workspace', 'mothership', @@ -106,10 +91,6 @@ const FOREIGN_LIFECYCLE_ROOTS = [ 'organization-logos/', ] as const -const WORKSPACE_KEYED_ROOTS: ReadonlySet = new Set( - WORKSPACE_STORAGE_FAMILIES.map(({ root }) => root) -) - interface StorageObject { key: string context: StorageContext @@ -128,7 +109,7 @@ interface ObjectCleanupPayload { /** The trailing slash keeps `workspace/ws1/` from matching `workspace/ws10/...`. */ function workspaceStoragePrefix(root: string, workspaceId: string): string { - if (!SAFE_ID.test(workspaceId)) throw new Error('Invalid workspace storage identity') + if (!STORAGE_ID_PATTERN.test(workspaceId)) throw new Error('Invalid workspace storage identity') return `${root}/${workspaceId}/` } @@ -188,7 +169,7 @@ function parseObjectCleanup(value: unknown): ObjectCleanupPayload { !isRecordLike(value) || value.version !== 1 || typeof value.userId !== 'string' || - !SAFE_ID.test(value.userId) || + !STORAGE_ID_PATTERN.test(value.userId) || !Array.isArray(value.objects) || value.objects.length > OBJECTS_PER_EVENT ) @@ -217,35 +198,6 @@ function parseObjectCleanup(value: unknown): ObjectCleanupPayload { return { userId: value.userId, objects, cursor } } -/** - * Exact keys under a workspace-keyed root whose workspace still exists. Such an object stays - * readable to that workspace's members by key, so an exact-key cleanup never deletes it, even when - * the row that named it cascaded away with the account. A workspace segment that is not a valid id - * is kept too. - */ -async function keysUnderLiveWorkspaces(keys: readonly string[]): Promise> { - const kept = new Set() - const keysByWorkspace = new Map() - for (const key of keys) { - const [root, workspaceId] = key.split('/') - if (!WORKSPACE_KEYED_ROOTS.has(root)) continue - if (!workspaceId || !SAFE_ID.test(workspaceId)) { - kept.add(key) - continue - } - const workspaceKeys = keysByWorkspace.get(workspaceId) - if (workspaceKeys) workspaceKeys.push(key) - else keysByWorkspace.set(workspaceId, [key]) - } - if (keysByWorkspace.size === 0) return kept - const live = await db - .select({ id: workspaceTable.id }) - .from(workspaceTable) - .where(inArray(workspaceTable.id, [...keysByWorkspace.keys()])) - for (const { id } of live) for (const key of keysByWorkspace.get(id) ?? []) kept.add(key) - return kept -} - /** A local sidecar belongs to its object: it is kept exactly when the object is. */ function sidecarOwner(key: string): string | null { return key.endsWith(LOCAL_UPLOAD_METADATA_SUFFIX) @@ -363,8 +315,7 @@ const cleanupAccountObjects: OutboxHandler = async (raw, context) => { return } const pending = payload.objects.slice(payload.cursor).map((object) => object.key) - const referenced = await keysUnderLiveWorkspaces(pending) - for (const key of await createStorageReferenceGuard()(pending)) referenced.add(key) + const referenced = await createStorageReferenceGuard()(pending) if (referenced.size > 0) logger.warn('Keeping objects a surviving reference still names', { userId: payload.userId, @@ -442,7 +393,8 @@ class ObjectCollector implements AccountStorageObjects { } addProfilePicture(key: string): void { - if (this.seen.has(key)) return + if (!isSafeKey(key) || !isAccountProfilePicture(key, 'profile-pictures') || this.seen.has(key)) + return if (this.objects.length >= MAX_DURABLE_OBJECTS) { this.capped = true return @@ -719,7 +671,7 @@ async function collectInto( } function sweepableWorkspaceIds(workspaceIds: readonly string[]): string[] { - return workspaceIds.filter((id) => SAFE_ID.test(id)) + return workspaceIds.filter((id) => STORAGE_ID_PATTERN.test(id)) } /** diff --git a/apps/sim/lib/users/workspace-storage.ts b/apps/sim/lib/users/workspace-storage.ts new file mode 100644 index 00000000000..0c4d54c2af2 --- /dev/null +++ b/apps/sim/lib/users/workspace-storage.ts @@ -0,0 +1,54 @@ +import { db } from '@sim/db' +import { workspace as workspaceTable } from '@sim/db/schema' +import { inArray } from 'drizzle-orm' +import type { StorageContext } from '@/lib/uploads/config' + +/** An id safe to place in a storage key path or a `LIKE` pattern. */ +export const STORAGE_ID_PATTERN = /^[A-Za-z0-9_-]{1,128}$/ + +/** + * Every storage family whose keys embed their workspace as the second path segment, in the + * bucket that serves it. A deleted workspace's objects in these families are found by listing, + * so neither a missing row nor a row written after the deletion read its keys can strand one. + */ +export const WORKSPACE_STORAGE_FAMILIES = [ + { root: 'workspace', context: 'workspace' }, + { root: 'table-import', context: 'table-import' }, + { root: 'execution', context: 'execution' }, + { root: 'table-snapshots', context: 'execution' }, + { root: 'copilot-doc-compiled', context: 'copilot' }, + { root: 'workspace-logos', context: 'workspace-logos' }, +] as const satisfies readonly { root: string; context: StorageContext }[] + +const WORKSPACE_KEYED_ROOTS: ReadonlySet = new Set( + WORKSPACE_STORAGE_FAMILIES.map(({ root }) => root) +) + +/** + * Exact keys under a workspace-keyed root whose workspace still exists. Such an object stays + * readable to that workspace's members by key, so no account-deletion purge deletes it, even when + * the row that named it cascaded away with the account. A workspace segment that is not a valid id + * is kept too. + */ +export async function keysUnderLiveWorkspaces(keys: readonly string[]): Promise> { + const kept = new Set() + const keysByWorkspace = new Map() + for (const key of keys) { + const [root, workspaceId] = key.split('/') + if (!WORKSPACE_KEYED_ROOTS.has(root)) continue + if (!workspaceId || !STORAGE_ID_PATTERN.test(workspaceId)) { + kept.add(key) + continue + } + const workspaceKeys = keysByWorkspace.get(workspaceId) + if (workspaceKeys) workspaceKeys.push(key) + else keysByWorkspace.set(workspaceId, [key]) + } + if (keysByWorkspace.size === 0) return kept + const live = await db + .select({ id: workspaceTable.id }) + .from(workspaceTable) + .where(inArray(workspaceTable.id, [...keysByWorkspace.keys()])) + for (const { id } of live) for (const key of keysByWorkspace.get(id) ?? []) kept.add(key) + return kept +} From f672c2bbc68dda4f0dd35d57d95042eebed65c40 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Fri, 16 Oct 2026 17:00:04 -0700 Subject: [PATCH 11/11] 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. --- apps/sim/lib/users/storage-reference-guard.ts | 11 ++-- .../lib/users/storage-sweep.integration.ts | 39 +------------ apps/sim/lib/users/storage-sweep.test.ts | 25 ++++---- apps/sim/lib/users/storage-sweep.ts | 57 +++---------------- 4 files changed, 31 insertions(+), 101 deletions(-) diff --git a/apps/sim/lib/users/storage-reference-guard.ts b/apps/sim/lib/users/storage-reference-guard.ts index e7c210dab28..bf55b46af8f 100644 --- a/apps/sim/lib/users/storage-reference-guard.ts +++ b/apps/sim/lib/users/storage-reference-guard.ts @@ -207,11 +207,12 @@ async function loadKeysNamedByUrls(runs: readonly string[], named: Set): * - it sits under a world-readable root (`isPublicStorageKey`, the same prefixes the file-serve * route's `isPublicByKeyPrefix` answers without a session) and a stored asset URL names it. * Only URL rows that mention the key's {@link ownerRun} are read, on every call so a reference - * written mid-sweep still counts; a public key without one is kept unread. A URL that hand-encodes characters of that - * run is missed. Account deletion asks about public keys only under a deleted workspace's - * prefix (its logos) and for an avatar the account is proven to have uploaded, so such a URL can - * cost an object of a deleted workspace or of the erased account's own uploads, never one a - * surviving workspace, organization, or user uploaded. + * written mid-sweep still counts; a public key without one is kept unread. A URL that + * hand-encodes characters of that run is missed. Account deletion asks about public keys only + * under a deleted workspace's prefix (its logos) and for the avatar the erased account's + * `user.image` names, so such a URL can cost one of those, and only when no surviving reference + * names it the way this app writes URLs; another user's own `user.image` naming their avatar + * always keeps it. * * No URL column is read for any other key: `verifyFileAccess` serves it only through a row, by its * key path under a workspace that still exists (kept above), or by object metadata to a signed-in holder of a hand-pasted URL to diff --git a/apps/sim/lib/users/storage-sweep.integration.ts b/apps/sim/lib/users/storage-sweep.integration.ts index 4fb8137d5af..6de24936293 100644 --- a/apps/sim/lib/users/storage-sweep.integration.ts +++ b/apps/sim/lib/users/storage-sweep.integration.ts @@ -7,7 +7,6 @@ import { copilotChats, outboxEvent, permissions, - uploadSession, user, workspace, workspaceFiles, @@ -104,26 +103,6 @@ async function insertAdminWorkspace(id: string, ownerId: string) { }) } -/** The ownership record a finished profile-picture upload leaves behind. */ -function completedAvatarUpload(userId: string, key: string) { - return { - id: generateId(), - tokenHash: generateId(), - userId, - purpose: 'profile_picture' as const, - method: 'put' as const, - storageContext: 'profile-pictures', - finalKey: key, - storageProvider: 'local' as const, - fileName: 'avatar.png', - contentType: 'image/png', - fileSize: 1, - status: 'completed' as const, - expiresAt: new Date(), - completedAt: new Date(), - } -} - function avatarUrl(key: string): string { return `/api/files/serve/${encodeURIComponent(key)}?context=profile-pictures` } @@ -251,12 +230,6 @@ async function seed() { logoUrl: `/api/files/serve/s3/${encodeURIComponent(keys.logoNamedByEncodedUrl)}`, }) .where(eq(workspace.id, survivingId)) - await db - .insert(uploadSession) - .values([ - completedAvatarUpload(departingId, keys.ownAvatar), - completedAvatarUpload(ownerId, keys.othersAvatar), - ]) await db .update(user) .set({ image: avatarUrl(keys.ownAvatar) }) @@ -359,20 +332,15 @@ describe('account deletion storage sweep', () => { } ) - check('never purges an avatar the account points at but did not upload', async () => { + check("keeps another account's avatar the departing image points at", async () => { const f = await seed() await db .update(user) .set({ image: avatarUrl(f.keys.othersAvatar) }) - .where(eq(user.id, f.departingId)) + .where(inArray(user.id, [f.ownerId, f.departingId])) await deleteUserAccount(f.departingId) - const objects = await objectEvents(f.departingId) - const queuedKeys = objects.flatMap((event) => - (event.payload as { objects: { key: string }[] }).objects.map((object) => object.key) - ) - expect(queuedKeys).not.toContain(f.keys.othersAvatar) - await runNow(objects.map((event) => event.id)) + await runNow((await objectEvents(f.departingId)).map((event) => event.id)) expect(await readFile(join(storageRoot, f.keys.othersAvatar), 'utf8')).toBe('x') }) @@ -403,7 +371,6 @@ afterAll(async () => { ) ) await db.delete(workspaceFiles).where(inArray(workspaceFiles.userId, userIds)) - await db.delete(uploadSession).where(inArray(uploadSession.userId, userIds)) await deleteWorkspaceFixture(db, inArray(workspace.id, workspaceIds)) await db.delete(user).where(inArray(user.id, userIds)) await rm(storageRoot, { recursive: true, force: true }) diff --git a/apps/sim/lib/users/storage-sweep.test.ts b/apps/sim/lib/users/storage-sweep.test.ts index df6cae253e1..c9cb20e1d88 100644 --- a/apps/sim/lib/users/storage-sweep.test.ts +++ b/apps/sim/lib/users/storage-sweep.test.ts @@ -361,6 +361,19 @@ describe('account storage object cleanup', () => { expect(dbChainMockFns.from).not.toHaveBeenCalledWith(schemaMock[table]) }) + it("keeps an avatar another account's image still names", async () => { + const key = 'profile-pictures/upload-0002-owner-avatar.png' + await put(key) + queueTableRows(schemaMock.user, []) + queueTableRows(schemaMock.user, [ + { id: 'owner-1', value: `/api/files/serve/s3/${encodeURIComponent(key)}` }, + ]) + + await cleanup(payload([{ key, context: 'profile-pictures' }]), outboxContext()) + + expect(await exists(key)).toBe(true) + }) + it("deletes the account's own uploaded avatar", async () => { await put('profile-pictures/upload-0001-avatar.png') @@ -505,28 +518,16 @@ describe('queueing account storage cleanup', () => { queueTableRows(schemaMock.user, [ { image: '/api/files/serve/s3/profile-pictures%2Fupload-0001-avatar.png' }, ]) - queueTableRows(schemaMock.uploadSession, [{ id: 'upload-0001' }]) await queueCleanup() expect(queuedObjectKeys()).toEqual(['profile-pictures/upload-0001-avatar.png']) }) - it('never queues an avatar key the account did not upload', async () => { - queueTableRows(schemaMock.user, [ - { image: '/api/files/serve/s3/profile-pictures%2Fupload-0002-someone-else.png' }, - ]) - - await queueCleanup() - - expect(queuedObjectKeys()).toEqual([]) - }) - it('never queues an avatar key that could escape its root', async () => { queueTableRows(schemaMock.user, [ { image: '/api/files/serve/profile-pictures%2Fx%2F..%2F..%2Fworkspace%2Fother.png' }, ]) - queueTableRows(schemaMock.uploadSession, [{ id: 'upload-0003' }]) await queueCleanup() diff --git a/apps/sim/lib/users/storage-sweep.ts b/apps/sim/lib/users/storage-sweep.ts index fc63c2fe68a..ef649fda616 100644 --- a/apps/sim/lib/users/storage-sweep.ts +++ b/apps/sim/lib/users/storage-sweep.ts @@ -3,7 +3,6 @@ import { copilotChats, document, knowledgeBase, - uploadSession, user, workspaceFile, workspaceFiles, @@ -80,9 +79,8 @@ const OBJECT_CONTEXTS: ReadonlySet = new Set([ /** * Keys no row collected here may name: Project prefix cleanup, the Assistant upload-session * sweep, organization logos, and profile pictures — which account deletion takes only from the - * account's own `user.image`, and only once {@link uploadedProfilePicture} proves the account - * uploaded it, because whitelabel logos were once uploaded as `profile-pictures` rows under an - * admin's account. + * account's own `user.image` (see {@link extractProfilePictureKey}), because whitelabel logos were + * once uploaded as `profile-pictures` rows under an admin's account. */ const FOREIGN_LIFECYCLE_ROOTS = [ 'project/', @@ -143,10 +141,12 @@ function isAccountProfilePicture(key: string, context: string): boolean { } /** - * The account's own uploaded avatar, as a storage key. `user.image` holds either a - * `/api/files/serve/...` path (an upload this app owns) or an absolute URL from an OAuth provider, - * which must not be deleted; only the former yields a key, and only under `profile-pictures/`. - * The image is personal data, so an erasure that leaves it in the bucket is not an erasure. + * The account's avatar, as a storage key. `user.image` holds either a `/api/files/serve/...` path + * (an upload this app owns) or an absolute URL from an OAuth provider, which must not be deleted; + * only the former yields a key, and only under `profile-pictures/`. The image is personal data, so + * an erasure that leaves it in the bucket is not an erasure. `user.image` is user-settable, so the + * key can name someone else's avatar; the cleanup's reference guard keeps any key another + * account's `user.image` or a stored asset URL still names. */ export function extractProfilePictureKey(image: string | null): string | null { if (!image) return null @@ -471,44 +471,6 @@ async function collectVersionsOf( ) } -/** - * Whether the account itself uploaded `key`. `user.image` is settable to any serve URL, so it - * names a key without proving whose it is; the proof is the account's completed `profile_picture` - * upload session (kept for a week after completion), or the `workspace_files` row an older - * profile-picture upload wrote. Without either the avatar is left in place. - */ -async function uploadedProfilePicture( - executor: DbOrTx, - userId: string, - key: string -): Promise { - const [session] = await executor - .select({ id: uploadSession.id }) - .from(uploadSession) - .where( - and( - eq(uploadSession.finalKey, key), - eq(uploadSession.userId, userId), - eq(uploadSession.purpose, 'profile_picture'), - eq(uploadSession.status, 'completed') - ) - ) - .limit(1) - if (session) return true - const [row] = await executor - .select({ id: workspaceFiles.id }) - .from(workspaceFiles) - .where( - and( - eq(workspaceFiles.key, key), - eq(workspaceFiles.userId, userId), - eq(workspaceFiles.context, 'profile-pictures') - ) - ) - .limit(1) - return Boolean(row) -} - interface CollectionScope { userId: string workspaceIds: string[] @@ -534,8 +496,7 @@ async function collectInto( .where(eq(user.id, userId)) .limit(1) const profilePicture = extractProfilePictureKey(account?.image ?? null) - if (profilePicture && (await uploadedProfilePicture(executor, userId, profilePicture))) - collector.addProfilePicture(profilePicture) + if (profilePicture) collector.addProfilePicture(profilePicture) const filesSince = since ? and(eq(workspaceFiles.userId, userId), gte(workspaceFiles.uploadedAt, since))