fix(cleanup): re-root child folders too, and guard the hook's own reads

Two gaps a cold review found in the re-root hook:

- folder.parentId is itself ON DELETE SET NULL and
  folder_workspace_resource_parent_name_active_unique keys on
  coalesce(parent_id,''), so purging a parent can collide a surviving active
  subfolder at the root — the identical 23505 stall. The hook covered workflows
  and files only while its TSDoc presented the class as closed.
- the hook's own SELECTs sat outside any guard, so a transient read failure
  rejected onBatch and produced exactly the aborted batch it exists to prevent.

Also adds the first tests for lib/cleanup: nothing verified that onBatch is
forwarded through batchDeleteByWorkspaceAndTimestamp's ...rest, or that
chunkedBatchDelete runs it BEFORE the delete. Every existing test invokes the
captured callback directly with that module mocked, so a regression in either
would have left the whole suite green.

Author: waleedlatif1

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

@@ -26,7 +26,9 @@ const {
   mockSelectRowsByIdChunks,
   mockDeduplicateWorkflowName,
   mockAllocateUniqueWorkspaceFileName,
+  mockDeduplicateFolderName,
 } = vi.hoisted(() => ({
+  mockDeduplicateFolderName: vi.fn(async (_tx, _ws, _parent, name: string) => name),
   mockDeduplicateWorkflowName: vi.fn(async (name: string) => name),
   mockAllocateUniqueWorkspaceFileName: vi.fn(async (_ws: string, name: string) => name),
   mockBatchDeleteByWorkspaceAndTimestamp: vi.fn(async () => ({ deleted: 0, failed: 0 })),
@@ -80,6 +82,8 @@ vi.mock('@/lib/workflows/utils', () => ({
   deduplicateWorkflowName: mockDeduplicateWorkflowName,
 }))
 
+vi.mock('@/lib/folders/naming', () => ({ deduplicateFolderName: mockDeduplicateFolderName }))
+
 vi.mock('@/lib/uploads/contexts/workspace/workspace-file-manager', () => ({
   allocateUniqueWorkspaceFileName: mockAllocateUniqueWorkspaceFileName,
 }))
@@ -433,6 +437,35 @@ describe('folder cleanup target', () => {
       })
     })
 
+    it('re-roots an active SUBFOLDER, which hits the same unique index', async () => {
+      /**
+       * `folder.parentId` is also ON DELETE SET NULL and
+       * `folder_workspace_resource_parent_name_active_unique` keys on `coalesce(parent_id,'')`,
+       * so purging a parent can collide a surviving child at the root exactly like a workflow
+       * or file. Covering only those two would leave the class half-closed.
+       */
+      const onBatch = await getFolderOnBatch()
+      queueTableRows(schemaMock.folder, [{ id: 'folder-1' }])
+      queueTableRows(schemaMock.workflow, [])
+      queueTableRows(schemaMock.workspaceFiles, [])
+      queueTableRows(schemaMock.folder, [
+        { id: 'sub-1', name: 'Reports', workspaceId: 'ws-1', resourceType: 'knowledge_base' },
+      ])
+      mockDeduplicateFolderName.mockResolvedValueOnce('Reports (1)')
+
+      await onBatch([{ id: 'folder-1' }])
+
+      // Deduped against the ROOT of the child's OWN resourceType, which is where SET NULL lands it.
+      expect(mockDeduplicateFolderName).toHaveBeenCalledWith(
+        expect.anything(),
+        'ws-1',
+        null,
+        'Reports',
+        'knowledge_base'
+      )
+      expect(dbChainMockFns.set).toHaveBeenCalledWith({ parentId: null, name: 'Reports (1)' })
+    })
+
     it('recovers when the allocator RETURNS a colliding name and the update raises', async () => {
       /**
        * `allocateUniqueWorkspaceFileName` fails open — `fileExistsInWorkspace` swallows query

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

@@ -29,6 +29,7 @@ import {
   selectRowsByIdChunks,
 } from '@/lib/cleanup/batch-delete'
 import { prepareChatCleanup } from '@/lib/cleanup/chat-cleanup'
+import { deduplicateFolderName } from '@/lib/folders/naming'
 import { hardDeleteDocuments } from '@/lib/knowledge/documents/service'
 import type { StorageContext } from '@/lib/uploads'
 import { isUsingCloudStorage, StorageService } from '@/lib/uploads'
@@ -488,7 +489,23 @@ async function reRootActiveFolderChildren(
   label: string
 ): Promise<void> {
   if (folderIds.length === 0) return
+  /**
+   * The SELECTs below are guarded for the same reason `reRootOne` swallows: this hook rejecting
+   * IS the aborted batch it exists to prevent. A transient read failure should leave the DELETE
+   * to succeed or fail on its own merits, not turn into a guaranteed stall.
+   */
+  try {
+    await reRootActiveFolderChildrenUnguarded(folderIds, retentionDate, label)
+  } catch (error) {
+    logger.error(`[${label}] Re-rooting children of purged folders failed`, { error })
+  }
+}
 
+async function reRootActiveFolderChildrenUnguarded(
+  folderIds: string[],
+  retentionDate: Date,
+  label: string
+): Promise<void> {
   /**
    * Re-asserted here, not just on the DELETE. `deleteFilter` already skips a folder restored
    * between the SELECT and this hook — but without the same check here, this hook would still
@@ -581,6 +598,48 @@ async function reRootActiveFolderChildren(
     )
   }
 
+  /**
+   * Subfolders are exposed to exactly the same failure. `folder.parentId` is itself
+   * `ON DELETE SET NULL`, and `folder_workspace_resource_parent_name_active_unique` keys on
+   * `coalesce(parent_id, '')`, so purging a parent re-roots a surviving active child into a
+   * namespace where its name may already be taken — the identical 23505 stall. Covering only
+   * workflows and files would leave the class half-closed.
+   */
+  const childFolders = await cleanupDb
+    .select({
+      id: folderTable.id,
+      name: folderTable.name,
+      workspaceId: folderTable.workspaceId,
+      resourceType: folderTable.resourceType,
+    })
+    .from(folderTable)
+    .where(and(inArray(folderTable.parentId, expiredIds), isNull(folderTable.deletedAt)))
+
+  for (const row of childFolders) {
+    await reRootOne(
+      async () => {
+        const name = await deduplicateFolderName(
+          cleanupDb,
+          row.workspaceId,
+          null,
+          row.name,
+          row.resourceType
+        )
+        await cleanupDb
+          .update(folderTable)
+          .set({ parentId: null, name })
+          .where(eq(folderTable.id, row.id))
+      },
+      () =>
+        cleanupDb
+          .update(folderTable)
+          .set({ parentId: null, name: `${row.name} (${row.id})` })
+          .where(eq(folderTable.id, row.id)),
+      `folder ${row.id}`,
+      label
+    )
+  }
+
   if (workflows.length > 0 || files.length > 0) {
     logger.warn(
       `[${label}] Re-rooted ${workflows.length} workflow(s) and ${files.length} file(s) out of folders being purged`

apps/sim/lib/cleanup/batch-delete.test.ts

@@ -0,0 +1,80 @@
+/**
+ * @vitest-environment node
+ */
+
+import { schemaMock } from '@sim/testing'
+import { describe, expect, it, vi } from 'vitest'
+import { batchDeleteByWorkspaceAndTimestamp, chunkedBatchDelete } from '@/lib/cleanup/batch-delete'
+
+/**
+ * Minimal stand-in for the drizzle client `chunkedBatchDelete` calls. Only the DELETE path is
+ * modelled — the SELECT arrives through the caller-supplied `selectChunk`.
+ */
+function createDbClient(onDelete: () => void, selectRows: Array<{ id: string }> = []) {
+  return {
+    // `batchDeleteByWorkspaceAndTimestamp` builds its own selectChunk on this client.
+    select: () => ({
+      from: () => ({ where: () => ({ limit: async () => selectRows }) }),
+    }),
+    delete: () => {
+      onDelete()
+      return { where: () => ({ returning: async () => [{ id: 'row-1' }] }) }
+    },
+  } as never
+}
+
+/**
+ * These two assertions guard the single link that makes the folder re-root hook live. Every
+ * test in `cleanup-soft-deletes.test.ts` invokes the captured `onBatch` directly with
+ * `batchDeleteByWorkspaceAndTimestamp` mocked out, so a regression that stopped forwarding the
+ * hook — or ran it after the DELETE — would leave that whole suite green.
+ */
+describe('chunkedBatchDelete onBatch contract', () => {
+  it('runs onBatch BEFORE the delete for the same rows', async () => {
+    const order: string[] = []
+    const onBatch = vi.fn(async (rows: Array<{ id: string }>) => {
+      order.push(`onBatch:${rows.map((r) => r.id).join(',')}`)
+    })
+
+    await chunkedBatchDelete({
+      tableDef: schemaMock.folder as never,
+      workspaceIds: ['ws-1'],
+      tableName: 'test/folder',
+      dbClient: createDbClient(() => order.push('delete')),
+      selectChunk: async () => [{ id: 'row-1' }],
+      onBatch,
+      batchSize: 1,
+      maxBatches: 1,
+      totalRowLimit: 1,
+    })
+
+    expect(onBatch).toHaveBeenCalledWith([{ id: 'row-1' }])
+    // Ordering is the load-bearing half: renaming children after the DELETE would be useless.
+    expect(order).toEqual(['onBatch:row-1', 'delete'])
+  })
+
+  it('forwards onBatch through batchDeleteByWorkspaceAndTimestamp', async () => {
+    const order: string[] = []
+    const onBatch = vi.fn(async () => {
+      order.push('onBatch')
+    })
+
+    await batchDeleteByWorkspaceAndTimestamp({
+      tableDef: schemaMock.folder as never,
+      workspaceIdCol: schemaMock.folder.workspaceId as never,
+      timestampCol: schemaMock.folder.deletedAt as never,
+      workspaceIds: ['ws-1'],
+      retentionDate: new Date(0),
+      tableName: 'test/folder',
+      requireTimestampNotNull: true,
+      dbClient: createDbClient(() => order.push('delete'), [{ id: 'row-1' }]) as never,
+      onBatch,
+      batchSize: 1,
+      maxBatches: 1,
+    })
+
+    // The wrapper spreads `...rest` into chunkedBatchDelete; `onBatch` must survive that hop.
+    expect(onBatch).toHaveBeenCalled()
+    expect(order[0]).toBe('onBatch')
+  })
+})