Skip to content

Commit 83e7a85

Browse files
committed
fix(tables): harden reference deletion coordination
1 parent 1e07a7b commit 83e7a85

10 files changed

Lines changed: 262 additions & 7 deletions

File tree

‎apps/sim/app/workspace/[workspaceId]/tables/[tableId]/table.tsx‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,7 @@ import {
5252
} from '@/app/workspace/[workspaceId]/tables/[tableId]/view-state'
5353
import { ImportCsvDialog } from '@/app/workspace/[workspaceId]/tables/components/import-csv-dialog'
5454
import { ImportProgressMenu } from '@/app/workspace/[workspaceId]/tables/components/import-progress-menu'
55+
import { useWorkspaceTablesRoom } from '@/app/workspace/[workspaceId]/tables/hooks/use-workspace-tables-room'
5556
import { useLogByExecutionId } from '@/hooks/queries/logs'
5657
import {
5758
downloadExportResult,
@@ -204,6 +205,7 @@ export function Table({
204205
const tableId = propTableId || (params.tableId as string)
205206
const hostContext = useOptionalWorkspaceHostContext()
206207
const referenceColumnsEnabled = hostContext?.features?.referenceColumns ?? false
208+
useWorkspaceTablesRoom(workspaceId)
207209

208210
const posthog = usePostHog()
209211
const tableRowTtlEnabled = useFeatureFlag('table-row-ttl')

‎apps/sim/lib/folders/bulk.ts‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -251,6 +251,8 @@ export async function bulkDeleteFolders(params: {
251251
userId: string
252252
folders: readonly BulkFolderAffected[]
253253
countKey: 'tables' | 'knowledgeBases'
254+
/** Reuse a reference scan completed for this batch while retaining each folder's lock guard. */
255+
referenceCheckCompleted?: boolean
254256
}): Promise<BulkFolderDeleteOutcome> {
255257
const succeeded: BulkFolderAffected[] = []
256258
const failed: BulkFolderFailure[] = []
@@ -267,7 +269,11 @@ export async function bulkDeleteFolders(params: {
267269
userId: params.userId,
268270
folderName: folder.name,
269271
},
270-
{ projectAudit: false, notify: false }
272+
{
273+
projectAudit: false,
274+
notify: false,
275+
referenceCheckCompleted: params.referenceCheckCompleted,
276+
}
271277
)
272278
if (result.success) {
273279
succeeded.push(folder)

‎apps/sim/lib/folders/cascade.test.ts‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
* @vitest-environment node
33
*/
44
import {
5+
dbChainMockFns,
56
flattenMockConditions,
67
hasMockCondition,
78
queueTableRows,
@@ -586,6 +587,31 @@ describe('knowledge_base and table folder resources', () => {
586587
}
587588
)
588589
})
590+
591+
it('preserves a concurrent table lock as a locked folder failure', async () => {
592+
resetDbChainMock()
593+
queueTableRows(schemaMock.userTableDefinitions, [{ id: 'tbl_accounts' }])
594+
tableServiceMocks.deleteTables.mockResolvedValueOnce({
595+
archived: [],
596+
failed: [
597+
{
598+
id: 'tbl_accounts',
599+
name: 'Accounts',
600+
reason: 'Table deletion is locked',
601+
code: 'locked',
602+
},
603+
],
604+
notFound: [],
605+
})
606+
607+
await expect(
608+
tableConfig.archiveChildren?.({
609+
workspaceId: 'ws-1',
610+
folderIds: ['folder-1'],
611+
timestamp: TIMESTAMP,
612+
})
613+
).rejects.toMatchObject({ code: 'locked', message: 'Table deletion is locked' })
614+
})
589615
})
590616

591617
describe('table folder deletion guard', () => {
@@ -631,4 +657,18 @@ describe('table folder deletion guard', () => {
631657
errorCode: 'conflict',
632658
})
633659
})
660+
661+
it('reuses a completed bulk reference check while still checking table locks', async () => {
662+
queueTableRows(schemaMock.userTableDefinitions, [])
663+
664+
await expect(
665+
FOLDER_RESOURCES.table.guardDelete?.({
666+
workspaceId: 'ws-1',
667+
folderIds: ['folder-root'],
668+
referenceCheckCompleted: true,
669+
})
670+
).resolves.toBeNull()
671+
672+
expect(dbChainMockFns.select).toHaveBeenCalledOnce()
673+
})
634674
})

‎apps/sim/lib/folders/config.ts‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -147,6 +147,8 @@ export interface FolderResourceConfig {
147147
guardDelete?: (context: {
148148
workspaceId: string
149149
folderIds: string[]
150+
/** The caller already checked active references for this exact folder selection. */
151+
referenceCheckCompleted?: boolean
150152
}) => Promise<FolderDeleteRejection | null>
151153
}
152154

@@ -354,7 +356,10 @@ async function archiveTableChildren(context: CascadeChildrenContext): Promise<nu
354356
})
355357
if (result.terminalError) throw result.terminalError
356358
if (result.failed[0]) {
357-
throw new OrchestrationError('conflict', result.failed[0].reason)
359+
throw new OrchestrationError(
360+
result.failed[0].code === 'locked' ? 'locked' : 'conflict',
361+
result.failed[0].reason
362+
)
358363
}
359364

360365
return result.archived.length
@@ -393,9 +398,11 @@ async function restoreTableChildren(context: CascadeChildrenContext): Promise<nu
393398
async function guardTableDeletion({
394399
workspaceId,
395400
folderIds,
401+
referenceCheckCompleted,
396402
}: {
397403
workspaceId: string
398404
folderIds: string[]
405+
referenceCheckCompleted?: boolean
399406
}): Promise<FolderDeleteRejection | null> {
400407
const [
401408
{ db },
@@ -427,6 +434,8 @@ async function guardTableDeletion({
427434
}
428435
}
429436

437+
if (referenceCheckCompleted) return null
438+
430439
const [blocker] = await findActiveTableReferenceBlockers(db, workspaceId, {
431440
folderIds: new Set(folderIds),
432441
})

‎apps/sim/lib/folders/orchestration.test.ts‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import {
1111
schemaMock,
1212
} from '@sim/testing'
1313
import { afterAll, beforeEach, describe, expect, it, vi } from 'vitest'
14+
import { OrchestrationError } from '@/lib/core/orchestration/types'
1415
import { FolderCollectionFullError, FolderCollectionLimitExceededError } from '@/lib/folders/errors'
1516
import { folderMutationStatus } from '@/lib/folders/status'
1617

@@ -770,6 +771,21 @@ describe('deleteFolder', () => {
770771
expect(mockArchiveFolderCascade).not.toHaveBeenCalled()
771772
})
772773

774+
it('returns a classified failure when a table lock appears after the guard', async () => {
775+
queueTableRows(schemaMock.folder, [{ deletedAt: null }])
776+
mockArchiveFolderCascade.mockRejectedValueOnce(
777+
new OrchestrationError('locked', 'Table deletion is locked')
778+
)
779+
780+
const result = await deleteFolder(baseDelete)
781+
782+
expect(result).toEqual({
783+
success: false,
784+
error: 'Table deletion is locked',
785+
errorCode: 'locked',
786+
})
787+
})
788+
773789
it('hands the guard the whole resolved subtree, not just the root', async () => {
774790
setConfig({ guardDelete: mockGuardDelete })
775791
mockGuardDelete.mockResolvedValueOnce(null)
@@ -784,6 +800,20 @@ describe('deleteFolder', () => {
784800
})
785801
})
786802

803+
it('passes a completed reference preflight through to the table guard', async () => {
804+
setConfig({ guardDelete: mockGuardDelete })
805+
mockGuardDelete.mockResolvedValueOnce(null)
806+
queueTableRows(schemaMock.folder, [{ deletedAt: null }])
807+
808+
await deleteFolder(baseDelete, { referenceCheckCompleted: true })
809+
810+
expect(mockGuardDelete).toHaveBeenCalledWith({
811+
workspaceId: 'ws-1',
812+
folderIds: ['folder-1'],
813+
referenceCheckCompleted: true,
814+
})
815+
})
816+
787817
it('reuses an already-archived folder’s timestamp so a retry rejoins the same snapshot', async () => {
788818
// A fresh stamp would be unrecoverable: the folder row keeps its original deletedAt, so
789819
// anything archived under the new stamp would never match on restore.

‎apps/sim/lib/folders/orchestration.ts‎

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -792,8 +792,11 @@ export async function deleteFolder(
792792
*
793793
* `notify: false` for a caller deleting several folders in one gesture that
794794
* sends a single batch notification of its own — see {@link bulkDeleteFolders}.
795+
* `referenceCheckCompleted: true` reuses that batch's reference scan; the
796+
* table guard still checks deletion locks, and the table service rechecks
797+
* references under its own transaction locks before archiving the cohort.
795798
*/
796-
options?: { projectAudit?: boolean; notify?: boolean }
799+
options?: { projectAudit?: boolean; notify?: boolean; referenceCheckCompleted?: boolean }
797800
): Promise<DeleteFolderResult> {
798801
const existing = await withFolderTreeLock(params.workspaceId, params.resourceType, async (tx) => {
799802
const [row] = await tx
@@ -817,13 +820,14 @@ export async function deleteFolder(
817820
return deleteFolderWithoutTreeLock(params, existing.deletedAt, {
818821
projectAudit: options?.projectAudit ?? true,
819822
notify: options?.notify ?? true,
823+
referenceCheckCompleted: options?.referenceCheckCompleted,
820824
})
821825
}
822826

823827
async function deleteFolderWithoutTreeLock(
824828
params: DeleteFolderParams,
825829
deletedAt: Date | null,
826-
options: { projectAudit: boolean; notify: boolean }
830+
options: { projectAudit: boolean; notify: boolean; referenceCheckCompleted?: boolean }
827831
): Promise<DeleteFolderResult> {
828832
const { resourceType, folderId, workspaceId, userId, folderName, folderPath } = params
829833
const config = folderResourceConfig(resourceType)
@@ -844,12 +848,25 @@ async function deleteFolderWithoutTreeLock(
844848
params.maxFolderRows
845849
)
846850

847-
const rejection = await config.guardDelete?.({ workspaceId, folderIds })
851+
const rejection = await config.guardDelete?.({
852+
workspaceId,
853+
folderIds,
854+
...(options.referenceCheckCompleted ? { referenceCheckCompleted: true } : {}),
855+
})
848856
if (rejection) {
849857
return { success: false, error: rejection.error, errorCode: rejection.errorCode }
850858
}
851859

852-
const counts = await archiveFolderCascade(db, config, workspaceId, folderIds, timestamp)
860+
let counts: Awaited<ReturnType<typeof archiveFolderCascade>>
861+
try {
862+
counts = await archiveFolderCascade(db, config, workspaceId, folderIds, timestamp)
863+
} catch (error) {
864+
const classified = asOrchestrationError(error)
865+
if (classified?.code === 'locked' || classified?.code === 'conflict') {
866+
return { success: false, error: classified.message, errorCode: classified.code }
867+
}
868+
throw error
869+
}
853870

854871
logger.info('Deleted folder and all contents', { folderId, resourceType, counts })
855872

‎apps/sim/lib/table/application/bulk.test.ts‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -221,6 +221,63 @@ describe('table bulk application use cases', () => {
221221
)
222222
})
223223

224+
it('retries an explicit target after its selected-folder referrer is archived', async () => {
225+
mocks.planFolderSelection.mockResolvedValue({
226+
selected: [{ id: 'folder-1', name: 'Reports' }],
227+
notFound: [],
228+
contained: [],
229+
covered: new Set(['folder-1']),
230+
coveredBySelected: new Map([['folder-1', new Set(['folder-1'])]]),
231+
})
232+
mocks.deleteTables
233+
.mockResolvedValueOnce({
234+
archived: [],
235+
failed: [
236+
{
237+
id: 'table-target',
238+
name: 'Target',
239+
reason: 'Target is referenced by Referrer',
240+
code: 'reference',
241+
},
242+
],
243+
notFound: [],
244+
})
245+
.mockResolvedValueOnce(successfulTableDeleteBatch(['table-target']))
246+
mocks.bulkDeleteFolders.mockResolvedValue({
247+
succeeded: [{ id: 'folder-1', name: 'Reports' }],
248+
failed: [],
249+
folderCount: 1,
250+
resourceCount: 1,
251+
})
252+
253+
const result = await bulkDeleteTables.execute({
254+
principal,
255+
input: {
256+
assertedWorkspaceId: 'workspace-1',
257+
tableIds: ['table-target'],
258+
folderKeying: 'ids' as const,
259+
folders: ['folder-1'],
260+
},
261+
})
262+
263+
expect(result.deleted).toEqual([
264+
{ kind: 'folder', id: 'folder-1', name: 'Reports' },
265+
{ kind: 'table', id: 'table-target', name: 'Archived' },
266+
])
267+
expect(result.failed).toEqual([])
268+
expect(result.deletedItems).toEqual({ tables: 2, folders: 1 })
269+
expect(mocks.deleteTables).toHaveBeenCalledTimes(2)
270+
expect(mocks.deleteTables).toHaveBeenLastCalledWith(
271+
['table-target'],
272+
'request-1',
273+
expect.anything()
274+
)
275+
expect(mocks.findReferenceBlockers).toHaveBeenCalledOnce()
276+
expect(mocks.bulkDeleteFolders).toHaveBeenCalledWith(
277+
expect.objectContaining({ referenceCheckCompleted: true })
278+
)
279+
})
280+
224281
/**
225282
* The whole point of taking both id lists in one request: a table that is
226283
* also inside a selected folder must be archived exactly once, under the

‎apps/sim/lib/table/application/bulk.ts‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -529,7 +529,9 @@ export const bulkDeleteTables = defineAuthorizedTableUseCase({
529529
for (const table of tableResult.archived) {
530530
deleted.push({ kind: 'table', id: table.id, name: table.name })
531531
}
532+
const referenceFailures = tableResult.failed.filter((table) => table.code === 'reference')
532533
for (const table of tableResult.failed) {
534+
if (table.code === 'reference') continue
533535
outcome.failed.push({ kind: 'table', id: table.id, name: table.name, reason: table.reason })
534536
}
535537
for (const tableId of tableResult.notFound) {
@@ -567,6 +569,7 @@ export const bulkDeleteTables = defineAuthorizedTableUseCase({
567569
})
568570
)
569571
const deletedItems = { tables: deleted.length, folders: 0 }
572+
let deletedFolder = false
570573
if (terminalError === undefined && plan.selected.length > 0) {
571574
const deletableFolders = plan.selected.filter((folder) => {
572575
const blocker = blockedFolderIds.get(folder.id)
@@ -589,11 +592,49 @@ export const bulkDeleteTables = defineAuthorizedTableUseCase({
589592
}).attributedUserId,
590593
folders: deletableFolders,
591594
countKey: 'tables',
595+
referenceCheckCompleted: true,
592596
})
593597
for (const folder of folders.succeeded) deleted.push({ kind: 'folder', ...folder })
594598
for (const folder of folders.failed) outcome.failed.push({ kind: 'folder', ...folder })
595599
deletedItems.folders = folders.folderCount
596600
deletedItems.tables += folders.resourceCount
601+
deletedFolder = folders.succeeded.length > 0
602+
}
603+
}
604+
605+
if (terminalError === undefined && deletedFolder && referenceFailures.length > 0) {
606+
const retryResult = await deleteTables(
607+
referenceFailures.map((table) => table.id),
608+
generateRequestId(),
609+
{
610+
expectedWorkspaceId: context.workspaceId,
611+
skipNotify: true,
612+
}
613+
)
614+
for (const table of retryResult.archived) {
615+
deleted.push({ kind: 'table', id: table.id, name: table.name })
616+
deletedItems.tables += 1
617+
}
618+
for (const table of retryResult.failed) {
619+
outcome.failed.push({
620+
kind: 'table',
621+
id: table.id,
622+
name: table.name,
623+
reason: table.reason,
624+
})
625+
}
626+
for (const tableId of retryResult.notFound) {
627+
outcome.notFound.push({ kind: 'table', id: tableId })
628+
}
629+
terminalError = retryResult.terminalError
630+
} else {
631+
for (const table of referenceFailures) {
632+
outcome.failed.push({
633+
kind: 'table',
634+
id: table.id,
635+
name: table.name,
636+
reason: table.reason,
637+
})
597638
}
598639
}
599640

0 commit comments

Comments
 (0)