chore(db): drop the legacy folder tables and adopt the deferred folder_id FKs - #6051
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryHigh Risk Overview Because those FKs use New tests cover the re-root hook and assert Reviewed by Cursor Bugbot for commit 797bdce. Configure here. |
Greptile SummaryCloses the generic-folders cutover by dropping legacy folder tables and adopting deferred
Confidence Score: 5/5This PR appears safe to merge; the prior stranded name-dedup collision is addressed and no blocking failure remains. The stranded active-sibling suffix probe now excludes batch-kept names as well as existing folder rows on both legacy trees, and no remaining blocking failure was identified on the follow-up pass.
|
| Filename | Overview |
|---|---|
| packages/db/migrations/0276_drop_legacy_folder_tables.sql | Hand-written cutover migration with stranded reconcile, dangling re-root/dedup (including kept-name probes), deferred FK validation, and legacy table drops. |
| apps/sim/background/cleanup-soft-deletes.ts | Pre-delete re-root of active folder children with fallback unique names so SET NULL cannot 23505-stall retention. |
| apps/sim/lib/cleanup/batch-delete.ts | Adds onBatch hook wired before each chunk delete for pre-delete side effects. |
| packages/db/schema.ts | Drops legacy workflow_folder and workspace_file_folders definitions and adopts folder_id FKs in schema. |
| apps/sim/background/cleanup-soft-deletes.test.ts | Covers re-root paths, allocator failure fallbacks, restore race, and empty batch behavior. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Step1 insert-only reconcile stranded legacy folders] --> B[Step2 re-root dangling folder_id with name dedup]
B --> C[Step3 ADD FK NOT VALID]
C --> D[COMMIT]
D --> E[VALIDATE FK]
E --> F[DROP legacy workflow_folder and workspace_file_folders]
G[Retention hard-delete folder batch] --> H[onBatch reRootActiveFolderChildren]
H --> I[DELETE folder; SET NULL is no-op]
Reviews (8): Last reviewed commit: "fix(db): stop the re-root dedupe renamin..." | Re-trigger Greptile
9b919b8 to
eeda0c1
Compare
|
@cursor review |
|
The 3/5 finding is real, and it was fixed in Greptile's exact scenario: base name taken in The fix adds a Verified by execution against Postgres 17.9, not by reading. The fixture is first run against the pre-fix migration to confirm it reproduces the abort: Then against the fix, for Greptile's precise combination ( Two further bugs of the same class were found and fixed in the same commit, since all three destinations are partial unique indexes keyed on a coalesced nullable column:
Re-running review against current HEAD. |
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit eeda0c1. Configure here.
eeda0c1 to
345278b
Compare
|
@cursor review |
|
@cursor review |
|
@cursor review |
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 461ddd4. Configure here.
…r_id FKs Contract migration closing out the generic-folders cutover: drops workflow_folder and workspace_file_folders, and adopts the two folder_id foreign keys 0272 deliberately deferred. Runs a final INSERT-ONLY reconcile first so no stranded legacy folder is lost by the drop. Insert-only because 0272 deliberately renamed 47 workflow folders to dedupe them, and an upsert would revert all 47. In production this is a no-op (verified 0 stranded); it exists for deployments that never ran the post-drain reconcile as an operational step, self-hosted upgrades above all. Hand-written rather than drizzle-generated: the generated form drops the tables BEFORE adding the FKs with no step to re-root unresolvable folder_ids, so one dangling id fails ADD CONSTRAINT after the legacy data is already gone; it also validates the FK inline, holding ACCESS EXCLUSIVE across a full scan of the 1.7M-row workspace_files, and uses DROP TABLE CASCADE. The generated snapshot is kept so drizzle-kit generate reports no schema changes. Every re-root path dedupes, because all three destinations are partial unique indexes keyed on a coalesced nullable column: folder, workflow, and workspace_files. Parents must also be reachable — an active row re-roots off a soft-deleted parent, while a soft-deleted row may keep one. Also hardens the retention sweep, which the new ON DELETE SET NULL exposes: a surviving active child re-rooted by the FK can collide at the workspace root, and chunkedBatchDelete turns that 23505 into a permanent per-chunk stall. The folder cleanup target now renames children first, covering workflows, files and subfolders, re-asserts eligibility so a restore mid-batch is not stripped, and treats the deduplicated name as a hint since both allocators can return a colliding one.
085da44 to
c1dc251
Compare
|
Rebased onto latest staging and renumbered 0275 → 0276: staging landed its own Squashed to one commit because the rebase conflicted only in Also folded in two fixes from a fresh review pass:
And the first tests for Gates on the rebuilt branch: typecheck, |
|
@cursor review |
workflow.workspace_id is nullable, and NULL is treated as EQUAL by PARTITION BY but UNKNOWN by the = in base_taken. So for personal workflows rn incremented across the group while no collision was ever detected: the dedupe could only fire spuriously, renaming a user-visible workflow that needed no rename, since the unique index treats NULL workspace_id rows as distinct anyway. The file block already carried the equivalent guard. Also gives step 1's inserts ON CONFLICT (id) DO NOTHING. It restates the stranded guard, closing the window between that read's snapshot and the index check — an operational re-run of 0274, or a live pod, committing into folder mid-statement would otherwise raise 23505, and migrate.ts retries only 55P03. 0272 and 0274 were already written this way; step 1 was the exception. Corrects three comments that claimed more than the code delivers: the header's 'no legacy folder is lost' (an id already present under another resource_type cannot be rescued), the reachability note (a cycle among stranded rows survives a per-row parent check), and a note made stale by the ON CONFLICT above.
|
A cold adversarial review of the migration (500 fuzz iterations against a throwaway Postgres 17) found one real bug and two hardening gaps. Fixed in Real bug — the re-root dedupe renamed personal workflows for no reason. Differential run — pre-fix vs post-fix, three personal workflows all named The rename achieved nothing (p1 and p0 still share the name) while mutating p2. Hardening — step 1's inserts now carry Three comments corrected for overclaiming, which has been the recurring defect in this file:
Clean per the same review: the slot arithmetic at all four dedupe sites, the parent resolver across all six cases, step 2's Also renumbered 0275 → 0276 earlier in this round; staging landed its own |
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 797bdce. Configure here.
Summary
workflow_folderandworkspace_file_folders, and adopts the twofolder_idforeign keys 0272 deliberately deferred.schema.tsand the testing schema mock.The thing to review closely: three re-root paths, three unique indexes
The migration re-roots rows in three places, and every destination is covered by a partial unique index keyed on a coalesced nullable column — so a name that is free in the source scope may already be taken in the destination:
folder_workspace_resource_parent_name_active_unique(workspace_id, resource_type, coalesce(parent_id,''), name)workflow_workspace_folder_name_active_unique(workspace_id, coalesce(folder_id,''), name)workspace_files_workspace_folder_name_active_unique(workspace_id, coalesce(folder_id,''), original_name)Three bugs were found and fixed here (all would abort the migration; all are 0-row in production and reachable on self-hosted):
foldertable. Stranded rows are by definition absent from it, so a stranded row keeping the nameX (1)was invisible to the probe run for a strandedX— both got assignedX (1). Fixed by also probing the batch's own claimed names (kept).folder_ids with no dedupe at all, colliding on the workflow / workspace_files indexes. Note the asymmetry that gave it away: step 1 dedupes meticulously when it re-roots, step 2 didn't.folder_parent_resource_type_matchtrigger (the legacy self-FK never checked workspace). Now re-roots on mismatch.Also: the stranded-row guard now keys on
idalone, matching the primary key it protects, and suffix exhaustion falls back to the row id instead of the base name (which would guarantee a collision and report a misleading error).FK side effect worth calling out
Adopting
ON DELETE SET NULLchanges what happens when the retention sweep hard-deletes afolder: previously the child'sfolder_idwas left dangling (invisible in the sidebar), now Postgres re-roots it — an improvement, except it can collide on those same indexes, andchunkedBatchDeleteturns a 23505 intohasMore = false, permanently stalling folder retention for that workspace chunk.reRootActiveFolderChildrennow renames such children before the delete, leaving the SET NULL a no-op. Onlyworkflowandworkspace_fileshave these indexes; KB and tables do not.Why the migration is hand-written
drizzle-kit generateproduces a correct-looking version that is unsafe to run:folder_ids. One dangling id makesADD CONSTRAINTfail after the legacy tables are already gone — unrecoverable.workspace_filesis ~1.7M rows / 1.8GB, so that holdsACCESS EXCLUSIVEacross a full scan of a hot table. This usesNOT VALID→COMMIT→VALIDATE, which takes onlySHARE UPDATE EXCLUSIVE.DROP TABLE ... CASCADE. Nothing references either table, so this drops them plainly and lets the absence of CASCADE act as the safety check.The generated snapshot is kept, so
drizzle-kit generatereports "No schema changes".Production preconditions (verified read-only before writing the migration)
folder_ids; 0 active rows filed under a soft-deleted folder.<old name> (N)(verified by regex shape, not by the count matching) — 0272's deliberate dedup renames. That is exactly why the reconcile is insert-only: an upsert would revert all 47.Testing
Executed end-to-end against local Postgres 17.9. The fixture is first run against the pre-fix migration to prove it reproduces all three bugs (it fails on
Batch (1),Shared, andreport.pdf), then against the fixed version. Asserted:Batch,Batch (1),Batch (2)(skips the suffix another batch row claims)Shared/Shared (1), and two danglers from different ghost folders →Twin/Twin (1)lockedpreserved; stranded child of stranded parent → parent preservedON DELETE SET NULLfires;workflow_folder_sort_idx(an index on the liveworkflowtable) survives the dropTypecheck,
check:migrations,check:api-validation, lint, and 1196 tests pass.Type of Change
Checklist