fix(cleanup): re-assert folder eligibility inside the re-root hook

The hook runs before the DELETE, which re-asserts eligibility and so correctly
skips a folder restored mid-batch. Without the same check in the hook, a restore
landing between the SELECT and onBatch left the folder alive with its children
stripped to the workspace root and renamed.

Every other side effect in this sweep only touches rows on their way out; this
one mutates rows that survive, so it is the one place losing that race is
visible to the user. Narrows the window to match the DELETE's own re-assertion
rather than closing it, which would need a row lock nothing here holds.

Author: waleedlatif1

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

@@ -380,6 +380,7 @@ describe('folder cleanup target', () => {
 
     it('re-roots an active workflow under a deduplicated name', async () => {
       const onBatch = await getFolderOnBatch()
+      queueTableRows(schemaMock.folder, [{ id: 'folder-1' }])
       queueTableRows(schemaMock.workflow, [{ id: 'w1', name: 'Report', workspaceId: 'ws-1' }])
       queueTableRows(schemaMock.workspaceFiles, [])
       mockDeduplicateWorkflowName.mockResolvedValueOnce('Report (2)')
@@ -398,6 +399,7 @@ describe('folder cleanup target', () => {
 
     it('re-roots an active workspace file under a deduplicated name', async () => {
       const onBatch = await getFolderOnBatch()
+      queueTableRows(schemaMock.folder, [{ id: 'folder-1' }])
       queueTableRows(schemaMock.workflow, [])
       queueTableRows(schemaMock.workspaceFiles, [
         { id: 'f1', originalName: 'report.pdf', workspaceId: 'ws-1' },
@@ -416,6 +418,7 @@ describe('folder cleanup target', () => {
     it('falls back to an id-suffixed name when the copy-suffix range is exhausted', async () => {
       // Letting the allocator throw would abort the sweep — the exact stall this guards against.
       const onBatch = await getFolderOnBatch()
+      queueTableRows(schemaMock.folder, [{ id: 'folder-1' }])
       queueTableRows(schemaMock.workflow, [])
       queueTableRows(schemaMock.workspaceFiles, [
         { id: 'f1', originalName: 'report.pdf', workspaceId: 'ws-1' },
@@ -430,6 +433,30 @@ describe('folder cleanup target', () => {
       })
     })
 
+    it('leaves children alone when the folder was restored between select and onBatch', async () => {
+      /**
+       * The DELETE re-asserts eligibility and so correctly skips a restored folder. Without the
+       * same re-assertion here, this hook would still strip and rename that folder's children —
+       * leaving a live folder emptied out. This is the one side effect in the sweep that mutates
+       * rows which survive, so losing the race is user-visible.
+       */
+      const onBatch = await getFolderOnBatch()
+      queueTableRows(schemaMock.folder, []) // restored: no longer soft-deleted past retention
+      // Children ARE queued: without the eligibility re-check these would be re-rooted and
+      // renamed, so the assertions below fail rather than passing for want of candidate rows.
+      queueTableRows(schemaMock.workflow, [{ id: 'w1', name: 'Report', workspaceId: 'ws-1' }])
+      queueTableRows(schemaMock.workspaceFiles, [
+        { id: 'f1', originalName: 'report.pdf', workspaceId: 'ws-1' },
+      ])
+      dbChainMockFns.update.mockClear()
+
+      await onBatch([{ id: 'folder-1' }])
+
+      expect(mockDeduplicateWorkflowName).not.toHaveBeenCalled()
+      expect(mockAllocateUniqueWorkspaceFileName).not.toHaveBeenCalled()
+      expect(dbChainMockFns.update).not.toHaveBeenCalled()
+    })
+
     it('touches nothing when the batch is empty', async () => {
       const onBatch = await getFolderOnBatch()
       dbChainMockFns.select.mockClear()

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

@@ -426,6 +426,12 @@ async function cleanupExpiredKnowledgeBases(
  * workspace files → S3 storage) are handled explicitly so the SELECT that drives
  * the external cleanup and the SELECT that drives the DB delete see the same rows.
  */
+/** Per-run values an `onBatch` hook needs but `CLEANUP_TARGETS` cannot know at module scope. */
+interface CleanupBatchContext {
+  retentionDate: Date
+  label: string
+}
+
 /**
  * Re-roots any still-active workflow or workspace file filed under a folder that is about to be
  * hard-deleted, giving it a collision-free name first.
@@ -440,13 +446,43 @@ async function cleanupExpiredKnowledgeBases(
  * An active child inside a soft-deleted folder is already an anomaly — the delete cascade
  * archives children — so this normally selects nothing, which is why the per-row loop is fine.
  */
-async function reRootActiveFolderChildren(folderIds: string[], label: string): Promise<void> {
+async function reRootActiveFolderChildren(
+  folderIds: string[],
+  retentionDate: Date,
+  label: string
+): Promise<void> {
   if (folderIds.length === 0) return
 
+  /**
+   * 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
+   * strip and rename the children of a folder that then survives, leaving a live folder
+   * emptied out. Every other side effect in this sweep only touches rows that are on their way
+   * out; this one mutates rows that stay, so it is the one place where losing that race is
+   * visible to the user.
+   *
+   * This narrows the window to match the DELETE's own re-assertion rather than closing it.
+   * Closing it properly means holding a row lock across select → onBatch → delete, which
+   * nothing in this sweep does today.
+   */
+  const stillExpired = await cleanupDb
+    .select({ id: folderTable.id })
+    .from(folderTable)
+    .where(
+      and(
+        inArray(folderTable.id, folderIds),
+        isNotNull(folderTable.deletedAt),
+        lt(folderTable.deletedAt, retentionDate)
+      )
+    )
+
+  const expiredIds = stillExpired.map(({ id }) => id)
+  if (expiredIds.length === 0) return
+
   const workflows = await cleanupDb
     .select({ id: workflow.id, name: workflow.name, workspaceId: workflow.workspaceId })
     .from(workflow)
-    .where(and(inArray(workflow.folderId, folderIds), isNull(workflow.archivedAt)))
+    .where(and(inArray(workflow.folderId, expiredIds), isNull(workflow.archivedAt)))
 
   for (const row of workflows) {
     if (!row.workspaceId) continue
@@ -463,7 +499,7 @@ async function reRootActiveFolderChildren(folderIds: string[], label: string): P
     .from(workspaceFiles)
     .where(
       and(
-        inArray(workspaceFiles.folderId, folderIds),
+        inArray(workspaceFiles.folderId, expiredIds),
         isNull(workspaceFiles.deletedAt),
         eq(workspaceFiles.context, 'workspace')
       )
@@ -513,10 +549,11 @@ const CLEANUP_TARGETS = [
       'knowledge_base',
       'table',
     ]),
-    onBatch: (rows: { id: string }[]) =>
+    onBatch: (rows: { id: string }[], ctx: CleanupBatchContext) =>
       reRootActiveFolderChildren(
         rows.map(({ id }) => id),
-        'cleanup'
+        ctx.retentionDate,
+        ctx.label
       ),
     name: 'folder',
   },
@@ -778,7 +815,10 @@ export async function runCleanupSoftDeletes(payload: CleanupJobPayload): Promise
       tableName: `${label}/${target.name}`,
       requireTimestampNotNull: true,
       additionalPredicate: 'additionalPredicate' in target ? target.additionalPredicate : undefined,
-      onBatch: 'onBatch' in target ? target.onBatch : undefined,
+      onBatch:
+        'onBatch' in target
+          ? (rows: { id: string }[]) => target.onBatch(rows, { retentionDate, label })
+          : undefined,
       dbClient: cleanupDb,
     })
     totalDeleted += result.deleted