The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment

Project	Deployment	Actions	Updated (UTC)
docs	Skipped		Jun 6, 2026 12:01am

PR Summary

Medium Risk
Changes permanent data deletion in the cleanup cron, but adds guards, budgets, and deferral so restores and partial drains should not wipe active tables; incorrect drain logic could still leave orphaned rows or delay cleanup.

Overview
Archived user tables are no longer hard-deleted through the generic soft-delete sweep. Cleanup now drains user_table_rows in bounded batches before removing the table definition, so a single delete does not cascade through millions of rows (and related table_row_executions).

A new drainRowsByColumn helper deletes child rows in per-batch transactions with an optional guard (parent still archived), a row budget, and a fullyDrained flag. The soft-delete job uses it at 5k rows per batch and up to 1M rows per run; definitions are deleted only when the drain completes, with archivedAt checks on both drain batches and the final definition delete so a mid-run restore does not keep purging an active table.

If the budget runs out, a table errors mid-drain, or the guarded definition delete no-ops, cleanup defers that table to a later cron run instead of forcing a large cascade.

^{Reviewed by Cursor Bugbot for commit e5d3eb8. Bugbot is set up for automated code reviews on this repo. Configure here.}

@greptile review

Greptile Summary

This PR replaces the single-statement ON DELETE CASCADE path for archived userTableDefinitions with a drain-before-delete strategy: rows in user_table_rows are deleted in 5k-row batches (each its own transaction) before the parent definition is hard-deleted, capping cascade size at the batch level. A per-batch EXISTS guard ensures a concurrent restoreTable commit stops the drain atomically, and the final definition delete is guarded by an archivedAt IS NOT NULL predicate so a restored table's record is never dropped.

• drainRowsByColumn — new primitive in batch-delete.ts that loops over bounded batches, returns fullyDrained: true/false based on a short-batch signal or an existence probe (handles the exact-divisor edge case that would have caused spurious deferrals), and correctly propagates fullyDrained: false on batch errors.
• cleanupArchivedUserTables — new coordinator in cleanup-soft-deletes.ts that sequences the drain then the guarded definition delete per table, defers on budget exhaustion or mid-drain errors, and removes userTableDefinitions from the generic CLEANUP_TARGETS sweep.
• Tests — 13 tests total covering full drain, budget deferral, mid-drain error continuation, and the restored-table no-op path.

Confidence Score: 5/5

Safe to merge. The drain-before-delete flow is logically sound, the per-batch guard and guarded definition delete form two independent layers of protection against the restore race, and all critical edge cases are covered by tests.

The three previously-flagged issues (unsafe error return, off-by-one on exact-divisor budget, stale archive eligibility after restore) are all addressed in this revision. Error handling correctly returns fullyDrained: false. The existence probe after budget exhaustion disambiguates the divisible case. The per-batch EXISTS guard plus the conditional RETURNING delete on the definition form a solid two-layer guard against concurrent restores. No remaining logic gaps or unsafe paths identified.

No files require special attention.

Important Files Changed

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A([cleanupArchivedUserTables]) --> B[SELECT archived+expired table IDs]
    B --> C{doomedTables empty?}
    C -- Yes --> Z([return 0])
    C -- No --> D[rowBudget = 1_000_000]
    D --> E{For each tableId\nrowBudget > 0?}
    E -- No --> LOG
    E -- Yes --> F[Build stillArchived EXISTS guard]
    F --> G[drainRowsByColumn\nwith guard + budget]
    G --> H{batch DELETE\nsuccess?}
    H -- Error --> I[return fullyDrained: false\ndeleted so far]
    H -- OK --> J{batchDeleted.length\n< limit?}
    J -- Yes short batch --> K[return fullyDrained: true]
    J -- No full batch --> L{remaining == 0?}
    L -- No --> H
    L -- Yes budget hit --> M[Existence probe\nSELECT 1 WHERE matchCol = val]
    M --> N{leftover row?}
    N -- No --> O[fullyDrained: true]
    N -- Yes --> P[fullyDrained: false]
    I --> Q{drain.fullyDrained?}
    K --> Q
    O --> Q
    P --> Q
    Q -- true --> R["db.delete(userTableDefinitions)\nWHERE id=? AND archivedAt IS NOT NULL\nRETURNING id"]
    R --> S{deletedDef.length == 1?}
    S -- Yes --> T[definitionsDeleted++]
    S -- No restored table no-op --> E
    T --> E
    Q -- false --> U{rowBudget <= 0?}
    U -- Yes --> LOG
    U -- No error continue --> E
    LOG([log + return definitionsDeleted])

_{Reviews (6): Last reviewed commit: "fix(tables): guard drain and definition ..." | Re-trigger Greptile}

Greptile Summary

This PR replaces a single-statement hard-delete of archived userTableDefinitions (which could cascade across millions of userTableRows) with a bounded drain-then-delete approach: rows are removed in 5 k-row batches (each its own transaction) before the parent definition is deleted, keeping every cascade small.

• drainRowsByColumn (new primitive in batch-delete.ts): loops with a subquery-based batch DELETE until the match set is empty or the per-run row budget (1 M) is exhausted; returns { deleted, budgetExhausted } to signal whether the definition is safe to delete.
• cleanupArchivedUserTables (new function in cleanup-soft-deletes.ts): discovers archived definitions via selectRowsByIdChunks, drains each one's rows, and hard-deletes the definition only when the drain completes — deferring any table whose rows can't be fully drained within the run budget to the next cron run; userTableDefinitions is removed from the generic CLEANUP_TARGETS sweep accordingly.
• service.ts receives a small cosmetic filter simplification (key !== null && key.startsWith('kb/') → key?.startsWith('kb/')) with no behavioral change.

Confidence Score: 3/5

The drain logic is architecturally correct, but a transient database error during a batch delete causes the definition to be hard-deleted with rows still present, firing the same large cascade the PR was designed to prevent.

The error path in drainRowsByColumn returns { budgetExhausted: false }, which cleanupArchivedUserTables treats as a complete drain and proceeds to delete the parent definition. On a production table with millions of rows, a single network blip mid-drain would trigger the exact multi-million-row ON DELETE CASCADE the PR was written to avoid.

apps/sim/lib/cleanup/batch-delete.ts — the catch block in drainRowsByColumn and the budgetExhausted signal it sends back to the caller.

Important Files Changed

Sequence Diagram

sequenceDiagram
    participant Cron as Cron Job
    participant CSD as cleanupArchivedUserTables
    participant DRC as drainRowsByColumn
    participant DB as Postgres

    Cron->>CSD: runCleanupSoftDeletes(payload)
    CSD->>DB: "SELECT id FROM userTableDefinitions WHERE archivedAt < retentionDate"
    DB-->>CSD: [tbl-1, tbl-2, ...]

    loop for each archived table
        CSD->>DRC: "drainRowsByColumn({ tableId, rowBudget })"
        loop "while remaining > 0"
            DRC->>DB: DELETE FROM userTableRows WHERE id IN (SELECT id ... LIMIT batch) RETURNING id
            DB-->>DRC: deleted rows (cascades table_row_executions)
            alt short batch — set exhausted
                DRC-->>CSD: "{ deleted, budgetExhausted: false }"
            else budget hit
                DRC-->>CSD: "{ deleted, budgetExhausted: true }"
            else DB error
                DRC-->>CSD: "{ deleted, budgetExhausted: false } ⚠️"
            end
        end
        alt "budgetExhausted = false"
            CSD->>DB: "DELETE FROM userTableDefinitions WHERE id = tableId"
        else "budgetExhausted = true"
            CSD-->>Cron: defer definition to next run
        end
    end
    CSD-->>Cron: definitionsDeleted count

_{Reviews (1): Last reviewed commit: "improvement(tables): drain rows in batch..." | Re-trigger Greptile}

Good catch — both Bugbot and Greptile flagged the same real bug. Fixed in d660542.

drainRowsByColumn now returns a single positive signal fullyDrained, true only when a short final batch confirms the match set is empty. Both budget-exhaustion and batch errors return fullyDrained: false, and cleanupArchivedUserTables only deletes the definition when fullyDrained === true — so a mid-drain error can no longer let the parent's ON DELETE CASCADE fire on the leftovers. Added a unit test asserting the error path returns fullyDrained: false.

Re the P2 exact-budget edge: when a table's row count equals the budget exactly, it now defers by one extra cron run (safe, harmless) rather than risk an unsafe delete — accepting that over an extra COUNT probe.

@greptile review

Addressed the Bugbot Medium ("Failed drain aborts later tables") in 4ad8c0b.

The loop no longer breaks on every non-complete drain. Since a budget stop consumes the entire per-run budget, a fullyDrained: false result with budget still remaining can only mean that one table errored mid-drain — so we now skip just that table (leaving its archived definition for a later run) and keep cleaning the rest, only stopping once the budget is actually spent. Added a test covering errored-table-skip with a follow-on table still processed.

@greptile review

Addressed Bugbot's Medium ("Budget exhaustion misreports empty drain") in ed095a3.

When the per-run budget is spent on a full final batch, drainRowsByColumn now runs a single indexed LIMIT 1 existence probe to decide fullyDrained precisely — so a table whose row count divides the budget exactly is deleted this run instead of deferred. The probe only runs in the budget-exhaustion path (not per batch), so it's effectively free. Added two unit tests covering rows-remaining vs emptied-exactly at the budget boundary.

Note: the two Greptile P1 comments still referencing budgetExhausted are from the first review and were already resolved by the rename to fullyDrained (d660542) — GitHub just re-anchored them to the current diff.

@greptile review

Fixed the CI build failure in b26cbda.

Root cause: an earlier bun run lint (biome --write --unsafe) silently rewrote a type guard in the unrelated lib/knowledge/documents/service.ts from key !== null && key.startsWith('kb/') to key?.startsWith('kb/'). The ?. makes the predicate return boolean | undefined, which violates the key is string guard — caught by the Next.js build typecheck (local incremental tsc false-greened it). Reverted both occurrences. Verified with a full bun run build (compiles + typechecks clean).

Bugbot now passes; the remaining Greptile comments reference the old budgetExhausted symbol and are stale (resolved by the rename in d660542).

Addressed Bugbot's High ("Stale archive eligibility after restore") in e5d3eb8.

The snapshot race is now closed atomically:

• Per-batch guard: each drain batch's row selection is ANDed with an EXISTS (… archivedAt IS NOT NULL AND archivedAt < retention) check on the parent definition. Since every batch is its own statement/transaction, once a restoreTable commits, the next batch selects (and deletes) zero rows — a concurrent restore can't keep purging an active table.
• Conditional definition delete: the definition is removed with WHERE id = … AND archivedAt IS NOT NULL AND archivedAt < retention … RETURNING, counted only when it removes a row. A restored (or re-archived) table makes this a no-op, so an active table's definition is never dropped.

Added the optional guard predicate to drainRowsByColumn and a test asserting the guard is passed and the definition delete is archivedAt-conditional, plus a restored-table no-op test (13 tests total). Verified with a full bun run build (typecheck clean).

Test and Build is green; the remaining Greptile comments reference the old budgetExhausted symbol and are stale.

@greptile review

✅ CI is green on e5d3eb8ac — Test and Build, Bugbot, GitGuardian, and Vercel all pass.

All review findings addressed across the iterations:

• High/P1 — drain error no longer signals "complete" (fullyDrained only true when rows are provably gone)
• Medium — a single table's drain error skips just that table, not the whole run
• Medium/P2 — budget-exhaustion does a LIMIT 1 probe so exact-boundary tables aren't needlessly deferred
• High — concurrent restoreTable race closed via a per-batch archivedAt guard + conditional definition delete
• Fixed an unrelated CI build break from an earlier unsafe lint autofix

The two remaining Greptile threads reference budgetExhausted and the old unconditional db.delete(userTableDefinitions) — both no longer exist (renamed to fullyDrained and replaced with the guarded conditional delete). They're stale re-anchors of the first review, not open issues.

Ready for human review.