fix(tables): defer definition delete unless rows are provably drained

Author: TheodoreSpeaks

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

@@ -36,7 +36,7 @@ const {
     mockDeleteFileMetadata: vi.fn(async () => true),
     mockDeleteFiles: vi.fn(async () => ({ deleted: 0, failed: [] as Array<{ key: string }> })),
     mockDeleteRowsById: vi.fn(async () => ({ deleted: 0, failed: 0 })),
-    mockDrainRowsByColumn: vi.fn(async () => ({ deleted: 0, budgetExhausted: false })),
+    mockDrainRowsByColumn: vi.fn(async () => ({ deleted: 0, fullyDrained: false })),
     mockIsUsingCloudStorage: vi.fn(() => true),
     mockLimit,
     mockOrderBy,
@@ -186,7 +186,7 @@ describe('cleanup soft deletes — archived user tables', () => {
   })
 
   it('drains rows before deleting the table definition', async () => {
-    mockDrainRowsByColumn.mockResolvedValue({ deleted: 5, budgetExhausted: false })
+    mockDrainRowsByColumn.mockResolvedValue({ deleted: 5, fullyDrained: true })
 
     await runCleanupSoftDeletes(basePayload)
 
@@ -200,8 +200,9 @@ describe('cleanup soft deletes — archived user tables', () => {
     )
   })
 
-  it('defers the definition delete when the row budget is exhausted', async () => {
-    mockDrainRowsByColumn.mockResolvedValue({ deleted: 200_000, budgetExhausted: true })
+  it('defers the definition delete when the drain does not fully complete', async () => {
+    // Budget stop or a drain error both surface as fullyDrained: false.
+    mockDrainRowsByColumn.mockResolvedValue({ deleted: 200_000, fullyDrained: false })
 
     await runCleanupSoftDeletes(basePayload)
 

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

@@ -307,8 +307,9 @@ async function cleanupArchivedUserTables(
     })
     rowBudget -= drain.deleted
 
-    // Rows still remain — defer the definition delete so its cascade stays small.
-    if (drain.budgetExhausted) break
+    // Only delete the definition once its rows are provably gone. A budget stop
+    // or a drain error leaves rows behind, so defer to keep the cascade small.
+    if (!drain.fullyDrained) break
 
     await db.delete(userTableDefinitions).where(eq(userTableDefinitions.id, tableId))
     definitionsDeleted++

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

@@ -26,44 +26,44 @@ describe('drainRowsByColumn', () => {
     vi.clearAllMocks()
   })
 
-  it('drains in batches until a short batch and reports the set exhausted', async () => {
+  it('drains in batches until a short batch and reports the set fully drained', async () => {
     dbChainMockFns.returning
       .mockResolvedValueOnce(returnRows(2))
       .mockResolvedValueOnce(returnRows(1))
 
     const result = await drainRowsByColumn({ ...baseOpts, batchSize: 2, rowBudget: 10 })
 
-    expect(result).toEqual({ deleted: 3, budgetExhausted: false })
+    expect(result).toEqual({ deleted: 3, fullyDrained: true })
     expect(dbChainMockFns.returning).toHaveBeenCalledTimes(2)
   })
 
-  it('stops at the row budget and reports it exhausted', async () => {
+  it('stops at the row budget and reports the set not fully drained', async () => {
     dbChainMockFns.returning
       .mockResolvedValueOnce(returnRows(2))
       .mockResolvedValueOnce(returnRows(2))
 
     const result = await drainRowsByColumn({ ...baseOpts, batchSize: 2, rowBudget: 4 })
 
-    expect(result).toEqual({ deleted: 4, budgetExhausted: true })
+    expect(result).toEqual({ deleted: 4, fullyDrained: false })
     expect(dbChainMockFns.returning).toHaveBeenCalledTimes(2)
   })
 
-  it('returns immediately when the match set is already empty', async () => {
+  it('reports fully drained immediately when the match set is already empty', async () => {
     dbChainMockFns.returning.mockResolvedValueOnce([])
 
     const result = await drainRowsByColumn({ ...baseOpts, batchSize: 2, rowBudget: 10 })
 
-    expect(result).toEqual({ deleted: 0, budgetExhausted: false })
+    expect(result).toEqual({ deleted: 0, fullyDrained: true })
     expect(dbChainMockFns.returning).toHaveBeenCalledTimes(1)
   })
 
-  it('bails without throwing when a batch delete fails', async () => {
+  it('reports not fully drained on a batch error so the caller defers the cascade', async () => {
     dbChainMockFns.returning
       .mockResolvedValueOnce(returnRows(2))
       .mockRejectedValueOnce(new Error('db down'))
 
     const result = await drainRowsByColumn({ ...baseOpts, batchSize: 2, rowBudget: 10 })
 
-    expect(result).toEqual({ deleted: 2, budgetExhausted: false })
+    expect(result).toEqual({ deleted: 2, fullyDrained: false })
   })
 })

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

@@ -278,8 +278,13 @@ export interface DrainByColumnOptions {
 
 export interface DrainResult {
   deleted: number
-  /** True if the budget ran out before the match set was fully drained. */
-  budgetExhausted: boolean
+  /**
+   * True only when the match set was confirmed empty (a short final batch).
+   * Budget exhaustion and batch errors both yield `false` — callers must treat
+   * `false` as "rows may remain" and defer any dependent parent-delete (whose
+   * ON DELETE CASCADE would otherwise fire on the leftovers) to a later run.
+   */
+  fullyDrained: boolean
 }
 
 /**
@@ -316,15 +321,16 @@ export async function drainRowsByColumn({
         .returning({ id: idCol })
     } catch (error) {
       logger.error(`[${tableName}] Drain batch failed for ${matchValue}:`, { error })
-      return { deleted, budgetExhausted: false }
+      return { deleted, fullyDrained: false }
     }
 
     deleted += batchDeleted.length
     remaining -= batchDeleted.length
 
     // Short batch means the match set is exhausted.
-    if (batchDeleted.length < limit) return { deleted, budgetExhausted: false }
+    if (batchDeleted.length < limit) return { deleted, fullyDrained: true }
   }
 
-  return { deleted, budgetExhausted: true }
+  // Hit the per-call budget on a full batch — rows may remain.
+  return { deleted, fullyDrained: false }
 }