Skip to content

Commit c1dc251

Browse files
committed
chore(db): drop the legacy folder tables and adopt the deferred folder_id FKs
Contract migration closing out the generic-folders cutover: drops workflow_folder and workspace_file_folders, and adopts the two folder_id foreign keys 0272 deliberately deferred. Runs a final INSERT-ONLY reconcile first so no stranded legacy folder is lost by the drop. Insert-only because 0272 deliberately renamed 47 workflow folders to dedupe them, and an upsert would revert all 47. In production this is a no-op (verified 0 stranded); it exists for deployments that never ran the post-drain reconcile as an operational step, self-hosted upgrades above all. Hand-written rather than drizzle-generated: the generated form drops the tables BEFORE adding the FKs with no step to re-root unresolvable folder_ids, so one dangling id fails ADD CONSTRAINT after the legacy data is already gone; it also validates the FK inline, holding ACCESS EXCLUSIVE across a full scan of the 1.7M-row workspace_files, and uses DROP TABLE CASCADE. The generated snapshot is kept so drizzle-kit generate reports no schema changes. Every re-root path dedupes, because all three destinations are partial unique indexes keyed on a coalesced nullable column: folder, workflow, and workspace_files. Parents must also be reachable — an active row re-roots off a soft-deleted parent, while a soft-deleted row may keep one. Also hardens the retention sweep, which the new ON DELETE SET NULL exposes: a surviving active child re-rooted by the FK can collide at the workspace root, and chunkedBatchDelete turns that 23505 into a permanent per-chunk stall. The folder cleanup target now renames children first, covering workflows, files and subfolders, re-asserts eligibility so a restore mid-batch is not stripped, and treats the deduplicated name as a hint since both allocators can return a colliding one.
1 parent ee7c061 commit c1dc251

10 files changed

Lines changed: 18947 additions & 143 deletions

File tree

apps/sim/background/cleanup-soft-deletes.test.ts

Lines changed: 197 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,13 @@
22
* @vitest-environment node
33
*/
44

5-
import { dbChainMock, dbChainMockFns, resetDbChainMock, schemaMock } from '@sim/testing'
5+
import {
6+
dbChainMock,
7+
dbChainMockFns,
8+
queueTableRows,
9+
resetDbChainMock,
10+
schemaMock,
11+
} from '@sim/testing'
612
import { afterAll, beforeEach, describe, expect, it, vi } from 'vitest'
713

814
const {
@@ -18,7 +24,13 @@ const {
1824
mockPrepareChatCleanup,
1925
mockResolveStorageBillingContext,
2026
mockSelectRowsByIdChunks,
27+
mockDeduplicateWorkflowName,
28+
mockAllocateUniqueWorkspaceFileName,
29+
mockDeduplicateFolderName,
2130
} = vi.hoisted(() => ({
31+
mockDeduplicateFolderName: vi.fn(async (_tx, _ws, _parent, name: string) => name),
32+
mockDeduplicateWorkflowName: vi.fn(async (name: string) => name),
33+
mockAllocateUniqueWorkspaceFileName: vi.fn(async (_ws: string, name: string) => name),
2234
mockBatchDeleteByWorkspaceAndTimestamp: vi.fn(async () => ({ deleted: 0, failed: 0 })),
2335
mockChunkedBatchDelete: vi.fn(async () => ({ deleted: 0, failed: 0 })),
2436
mockDecrementStorageUsageForBillingContextInTx: vi.fn(async () => undefined),
@@ -66,6 +78,16 @@ vi.mock('@/lib/uploads', () => ({
6678

6779
vi.mock('@/lib/uploads/server/metadata', () => ({ deleteFileMetadata: mockDeleteFileMetadata }))
6880

81+
vi.mock('@/lib/workflows/utils', () => ({
82+
deduplicateWorkflowName: mockDeduplicateWorkflowName,
83+
}))
84+
85+
vi.mock('@/lib/folders/naming', () => ({ deduplicateFolderName: mockDeduplicateFolderName }))
86+
87+
vi.mock('@/lib/uploads/contexts/workspace/workspace-file-manager', () => ({
88+
allocateUniqueWorkspaceFileName: mockAllocateUniqueWorkspaceFileName,
89+
}))
90+
6991
import { runCleanupSoftDeletes } from '@/background/cleanup-soft-deletes'
7092

7193
const basePayload = {
@@ -269,6 +291,7 @@ interface BatchDeleteOptions {
269291
tableName: string
270292
requireTimestampNotNull?: boolean
271293
additionalPredicate?: { type: string; column: unknown; values: unknown[] }
294+
onBatch?: (rows: { id: string }[]) => Promise<void>
272295
}
273296

274297
/**
@@ -331,4 +354,177 @@ describe('folder cleanup target', () => {
331354
.map(([options]) => options.tableName)
332355
expect(filtered).toEqual(['free/1/folder'])
333356
})
357+
358+
/**
359+
* `folder_id` is `ON DELETE SET NULL`, so Postgres re-roots surviving children on its own —
360+
* but `workflow` and `workspace_files` each carry a partial unique index keyed on
361+
* `coalesce(folder_id, '')`, so an implicit SET NULL can land a child on a name the workspace
362+
* root already holds. That aborts the whole DELETE with a 23505, which `chunkedBatchDelete`
363+
* turns into `hasMore = false` — folder retention then stalls permanently for that chunk,
364+
* re-failing on every later run. `onBatch` renames first so the SET NULL is a no-op.
365+
*/
366+
describe('re-rooting active children before the delete', () => {
367+
async function getFolderOnBatch() {
368+
const target = await runAndFindFolderTarget()
369+
expect(target?.onBatch).toBeTypeOf('function')
370+
return target!.onBatch!
371+
}
372+
373+
it('is the only cleanup target that re-roots children', async () => {
374+
await runCleanupSoftDeletes(basePayload)
375+
const calls = mockBatchDeleteByWorkspaceAndTimestamp.mock.calls as unknown as Array<
376+
[BatchDeleteOptions]
377+
>
378+
379+
const withOnBatch = calls
380+
.filter(([options]) => options.onBatch !== undefined)
381+
.map(([options]) => options.tableName)
382+
expect(withOnBatch).toEqual(['free/1/folder'])
383+
})
384+
385+
it('re-roots an active workflow under a deduplicated name', async () => {
386+
const onBatch = await getFolderOnBatch()
387+
queueTableRows(schemaMock.folder, [{ id: 'folder-1' }])
388+
queueTableRows(schemaMock.workflow, [{ id: 'w1', name: 'Report', workspaceId: 'ws-1' }])
389+
queueTableRows(schemaMock.workspaceFiles, [])
390+
mockDeduplicateWorkflowName.mockResolvedValueOnce('Report (2)')
391+
392+
await onBatch([{ id: 'folder-1' }])
393+
394+
// Deduped against the workspace ROOT (folderId null), which is where SET NULL would put it.
395+
expect(mockDeduplicateWorkflowName).toHaveBeenCalledWith(
396+
'Report',
397+
'ws-1',
398+
null,
399+
expect.anything()
400+
)
401+
expect(dbChainMockFns.set).toHaveBeenCalledWith({ folderId: null, name: 'Report (2)' })
402+
})
403+
404+
it('re-roots an active workspace file under a deduplicated name', async () => {
405+
const onBatch = await getFolderOnBatch()
406+
queueTableRows(schemaMock.folder, [{ id: 'folder-1' }])
407+
queueTableRows(schemaMock.workflow, [])
408+
queueTableRows(schemaMock.workspaceFiles, [
409+
{ id: 'f1', originalName: 'report.pdf', workspaceId: 'ws-1' },
410+
])
411+
mockAllocateUniqueWorkspaceFileName.mockResolvedValueOnce('report (2).pdf')
412+
413+
await onBatch([{ id: 'folder-1' }])
414+
415+
expect(mockAllocateUniqueWorkspaceFileName).toHaveBeenCalledWith('ws-1', 'report.pdf', null)
416+
expect(dbChainMockFns.set).toHaveBeenCalledWith({
417+
folderId: null,
418+
originalName: 'report (2).pdf',
419+
})
420+
})
421+
422+
it('falls back to an id-suffixed name when the copy-suffix range is exhausted', async () => {
423+
// Letting the allocator throw would abort the sweep — the exact stall this guards against.
424+
const onBatch = await getFolderOnBatch()
425+
queueTableRows(schemaMock.folder, [{ id: 'folder-1' }])
426+
queueTableRows(schemaMock.workflow, [])
427+
queueTableRows(schemaMock.workspaceFiles, [
428+
{ id: 'f1', originalName: 'report.pdf', workspaceId: 'ws-1' },
429+
])
430+
mockAllocateUniqueWorkspaceFileName.mockRejectedValueOnce(new Error('conflict'))
431+
432+
await expect(onBatch([{ id: 'folder-1' }])).resolves.toBeUndefined()
433+
434+
expect(dbChainMockFns.set).toHaveBeenCalledWith({
435+
folderId: null,
436+
originalName: 'report.pdf (f1)',
437+
})
438+
})
439+
440+
it('re-roots an active SUBFOLDER, which hits the same unique index', async () => {
441+
/**
442+
* `folder.parentId` is also ON DELETE SET NULL and
443+
* `folder_workspace_resource_parent_name_active_unique` keys on `coalesce(parent_id,'')`,
444+
* so purging a parent can collide a surviving child at the root exactly like a workflow
445+
* or file. Covering only those two would leave the class half-closed.
446+
*/
447+
const onBatch = await getFolderOnBatch()
448+
queueTableRows(schemaMock.folder, [{ id: 'folder-1' }])
449+
queueTableRows(schemaMock.workflow, [])
450+
queueTableRows(schemaMock.workspaceFiles, [])
451+
queueTableRows(schemaMock.folder, [
452+
{ id: 'sub-1', name: 'Reports', workspaceId: 'ws-1', resourceType: 'knowledge_base' },
453+
])
454+
mockDeduplicateFolderName.mockResolvedValueOnce('Reports (1)')
455+
456+
await onBatch([{ id: 'folder-1' }])
457+
458+
// Deduped against the ROOT of the child's OWN resourceType, which is where SET NULL lands it.
459+
expect(mockDeduplicateFolderName).toHaveBeenCalledWith(
460+
expect.anything(),
461+
'ws-1',
462+
null,
463+
'Reports',
464+
'knowledge_base'
465+
)
466+
expect(dbChainMockFns.set).toHaveBeenCalledWith({ parentId: null, name: 'Reports (1)' })
467+
})
468+
469+
it('recovers when the allocator RETURNS a colliding name and the update raises', async () => {
470+
/**
471+
* `allocateUniqueWorkspaceFileName` fails open — `fileExistsInWorkspace` swallows query
472+
* errors and returns false — so it can hand back a name already taken at the root. Only
473+
* the UPDATE discovers that, and an uncaught 23505 aborts the batch: the exact stall this
474+
* hook prevents. Guarding the name lookup alone is not enough.
475+
*/
476+
const onBatch = await getFolderOnBatch()
477+
queueTableRows(schemaMock.folder, [{ id: 'folder-1' }])
478+
queueTableRows(schemaMock.workflow, [])
479+
queueTableRows(schemaMock.workspaceFiles, [
480+
{ id: 'f1', originalName: 'report.pdf', workspaceId: 'ws-1' },
481+
])
482+
mockAllocateUniqueWorkspaceFileName.mockResolvedValueOnce('taken.pdf')
483+
dbChainMockFns.update.mockImplementationOnce(() => ({
484+
set: () => ({ where: () => Promise.reject(new Error('duplicate key value (23505)')) }),
485+
}))
486+
487+
await expect(onBatch([{ id: 'folder-1' }])).resolves.toBeUndefined()
488+
489+
expect(dbChainMockFns.set).toHaveBeenCalledWith({
490+
folderId: null,
491+
originalName: 'report.pdf (f1)',
492+
})
493+
})
494+
495+
it('leaves children alone when the folder was restored between select and onBatch', async () => {
496+
/**
497+
* The DELETE re-asserts eligibility and so correctly skips a restored folder. Without the
498+
* same re-assertion here, this hook would still strip and rename that folder's children —
499+
* leaving a live folder emptied out. This is the one side effect in the sweep that mutates
500+
* rows which survive, so losing the race is user-visible.
501+
*/
502+
const onBatch = await getFolderOnBatch()
503+
queueTableRows(schemaMock.folder, []) // restored: no longer soft-deleted past retention
504+
// Children ARE queued: without the eligibility re-check these would be re-rooted and
505+
// renamed, so the assertions below fail rather than passing for want of candidate rows.
506+
queueTableRows(schemaMock.workflow, [{ id: 'w1', name: 'Report', workspaceId: 'ws-1' }])
507+
queueTableRows(schemaMock.workspaceFiles, [
508+
{ id: 'f1', originalName: 'report.pdf', workspaceId: 'ws-1' },
509+
])
510+
dbChainMockFns.update.mockClear()
511+
512+
await onBatch([{ id: 'folder-1' }])
513+
514+
expect(mockDeduplicateWorkflowName).not.toHaveBeenCalled()
515+
expect(mockAllocateUniqueWorkspaceFileName).not.toHaveBeenCalled()
516+
expect(dbChainMockFns.update).not.toHaveBeenCalled()
517+
})
518+
519+
it('touches nothing when the batch is empty', async () => {
520+
const onBatch = await getFolderOnBatch()
521+
dbChainMockFns.select.mockClear()
522+
dbChainMockFns.update.mockClear()
523+
524+
await onBatch([])
525+
526+
expect(dbChainMockFns.select).not.toHaveBeenCalled()
527+
expect(dbChainMockFns.update).not.toHaveBeenCalled()
528+
})
529+
})
334530
})

0 commit comments

Comments
 (0)