fix(db): reconcile folders stranded by old pods during the cutover drain - #6049
fix(db): reconcile folders stranded by old pods during the cutover drain#6049waleedlatif1 wants to merge 2 commits into
Conversation
0272 and 0274 both ran while the previous app version was still serving. During the blue/green window the old pods write the legacy folder tables exclusively while the new pods write `folder` exclusively, so any folder an old pod creates between the migration and its own termination exists only in the legacy table and is invisible to the new code. 0274 asked to be re-run once the deploy drained; a journaled migration never replays, so the re-run needs its own file. Covers both trees, where 0274 covered only `file`. The workflow side is the higher-volume one and is where the stranded row actually landed in production. Insert-only for ids missing from `folder`, never a mirror upsert: 0272's dedup renamed 47 workflow folders, and an upsert would revert all 47 to their colliding names and violate the active-unique index doing it. Names are deduplicated against the first free suffix rather than a row number, since the target name may already be held by a row this block is not inserting. Only active rows dedupe — the partial index ignores soft-deleted rows, so an archived folder keeps its name and reappears under it if restored. Rows insert in depth order so a parent always precedes its children, which the self-referencing FK and the parent-resource-type trigger both require. Wrapped in a DO block for the same reason 0274 is: 0272 ends in an embedded COMMIT, so files after it are not guaranteed to run inside drizzle's batch transaction. Verified by executing the file against Postgres 17 with adversarial fixtures: a 0272-renamed row is left untouched, a stranded active row whose name and whose "(2)" are both taken lands on "(3)", a stranded archived row keeps its colliding name, a fully-stranded three-level chain inserts parents first, sibling dedup scopes to the parent, and cross-workspace and cross-resource-type names do not interfere. Idempotent across three consecutive runs with byte-identical state. Production at 06:52Z has 1 row to insert (archived, empty, no parent) on the workflow side and 0 on the file side.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryMedium Risk Overview The script is insert-only for missing ids so it does not undo Ops: run before soft-delete cleanup hard-purges expired Reviewed by Cursor Bugbot for commit a5dac82. Configure here. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Greptile SummaryAdds post-drain migration
Confidence Score: 5/5The PR appears safe to merge; prior lock and suffix issues are fixed and no blocking failure remains. No blocking failure remains. Workflow locked is selected and inserted; suffix enumeration starts at 0 so the first free name is (1); both prior threads are resolved.
|
| Filename | Overview |
|---|---|
| packages/db/migrations/0275_folder_cutover_post_drain_reconcile.sql | Post-drain insert-only reconcile for both legacy folder trees with depth order, active-name dedup from (1), and workflow locked carry-over. |
| packages/db/migrations/meta/_journal.json | Registers migration 0275 after 0274 in the drizzle journal. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
WF[workflow_folder legacy]
FF[workspace_file_folders legacy]
F[folder]
WF -->|missing id only depth order| F
FF -->|missing id only depth order| F
F -->|active name collision| Dedup["suffix from (1)"]
Reviews (2): Last reviewed commit: "fix(db): carry locked through the reconc..." | Re-trigger Greptile
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Two review findings, both real. `locked` was hardcoded false on the workflow insert. `workflow_folder` has a `locked` column and the app still enforces the lock feature against `folder.locked`, so a stranded locked folder would have come back unlocked. 0272 carries `f.locked` through for exactly this reason; this now matches it. `workspace_file_folders` has no such column — file folders have never been lockable — so the file pass still writes false. Name dedup started at ' (2)', skipping a free ' (1)'. 0272 enumerates `generate_series(1, 10000)` and the app's own dedup produces "New folder (1)", "New folder (2)", ..., so starting at (2) was inconsistent with both. Fixtures extended to cover both: a stranded locked folder must arrive locked, a stranded unlocked one must stay unlocked (proving the value is copied rather than blanket-set), and a collision with a free ' (1)' must take it. Still idempotent across three consecutive runs. The single row stranded in production is an archived, unlocked canary folder, so neither bug would have bitten on today's run — but this file is meant to be re-runnable and ships to self-hosted rolling deploys.
|
@cursor review |
|
Applied directly to production instead — the reconcile was a one-time operational cleanup and running it by hand meant it took effect immediately rather than waiting on the next deploy. Applied 2026-07-29T07:22Z after confirming the pre-cutover pods had fully drained (last legacy-table write 06:27:21Z, zero in the preceding 5 minutes). Dry-run first inside a transaction and rolled back, which exercised the real constraints and triggers: exactly 1 row to insert, 0 rows modified (confirming insert-only, so none of 0272's 47 dedup renames were reverted), and all post-state integrity checks at 0. Post-apply verification on production:
The single stranded row was Worth recording that the review round on this PR was not wasted: it caught two real bugs in the SQL that was ultimately run. Residual gap, left open deliberately: self-hosted installs doing rolling upgrades (Helm/k8s) across this cutover can strand folders the same way, and now ship no reconcile. Single-container docker-compose upgrades are unaffected, since there is no window where old and new code serve concurrently. Reopen this if that is worth covering.
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a5dac82. Configure here.
| INSERT INTO "folder" (id, resource_type, name, user_id, workspace_id, parent_id, locked, sort_order, created_at, updated_at, deleted_at) | ||
| VALUES (rec."id", 'workflow', candidate, rec."user_id", rec."workspace_id", rec."parent_id", | ||
| rec."locked", rec."sort_order", rec."created_at", rec."updated_at", rec.deleted_at) | ||
| ON CONFLICT (id) DO NOTHING; |
There was a problem hiding this comment.
Stranded child keeps deleted parent
Medium Severity
Stranded inserts copy parent_id from the legacy row without checking whether that parent is still present and active in folder. During the drain, new pods can soft-delete a parent in folder while old pods still create children under it in the legacy table, so this reinserts an active child under a deleted parent. That disagrees with create/restore, which reject or re-root when the parent is archived, and can later break cleanup when ON DELETE SET NULL collides with an active root name.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit a5dac82. Configure here.
1 participant
Adds
0275, the post-drain reconcile for the generic-folder cutover shipped in #6045.Why this exists
0272and0274both ran while the previous app version was still serving. During the blue/green window the old pods write the legacy folder tables exclusively while the new pods writefolderexclusively, so any folder an old pod creates between the migration and its own termination lives only in the legacy table and is invisible to the new code.0274's header asked to be run again once the deploy drained. A journaled migration never replays, so that re-run has to be its own file. This is it.The stranding window did materialize in production — measured live at 06:52Z, mid-drain:
workflow_folderworkspace_file_foldersThe single stranded row has no user impact — it was created and archived within 100ms by canary traffic. The count at execution time may be higher, since old pods were still serving when that was taken.
Covers both trees, where
0274covered onlyfile. The workflow side is the higher-volume one and is where the stranded row actually landed.Design
Insert-only for ids missing from
folder, never a mirror upsert.0274could upsert every column because file folders can never diverge in name —workspace_file_foldersenforces the same active-unique keyfolderdoes. That does not hold here:0272's dedup renamed 47 workflow folders across 35 collision groups, sofolderis deliberately different fromworkflow_folderfor those rows. An upsert would revert all 47 to their colliding names and violatefolder_workspace_resource_parent_name_active_uniquein the process.First free suffix, not a row number. The target name may already be held by a row this block is not inserting, so the dedup probes upward until it finds a genuinely free name.
Only active rows dedupe. The partial unique index ignores soft-deleted rows, so an archived folder keeps its original name and reappears under it if restored.
Depth-ordered inserts. A parent always lands before its children — the self-referencing FK is checked immediately and the
folder_parent_resource_type_matchtrigger reads the parent row, so a child inserted first fails both.Wrapped in a
DOblock, for the same reason0274is:0272ends with an embeddedCOMMITfor its trailingCONCURRENTLYindex builds, so files after it are not guaranteed to run inside drizzle's batch transaction.Verification
Executed the actual migration file against Postgres 17 with adversarial fixtures. Every case a naive implementation gets wrong:
0272renamed toReports (2)Reports, withReports (2)already takenReports (3)ReportsNested (2)Reportsvs workflowReportsIdempotency: three consecutive runs, byte-identical state.
bun run check:migrationspasses.Operational note — this has a deadline
Run before the soft-delete cleanup job purges expired
folderrows. Cleanup hard-deletes fromfolderbut never from the legacy tables, so a run landing after a purge would reinstate every purged folder as a soft-deleted phantom in Recently Deleted whose files and workflows are already gone. The retention window is far longer than any drain, so shipping promptly is sufficient — but this is the one constraint that makes deferring it indefinitely dangerous rather than merely redundant.Not in this PR — dropping the legacy tables
Deliberately held. Verified for a future contract PR, but not acted on:
What is not satisfied is rollback posture. The tables are the only rollback path, KB/Tables folders have hours of production exposure with no meaningful user traffic yet, and
schema.tscarries a binding policy that dropping on the strength of "no code references it" alone destroys that path. They cost essentially nothing to keep. That call belongs to the repo owner once the new folder trees have seen real usage.workflow_folder*when dropping indexes.workflow_folder_sort_idxis an index on the liveworkflowtable, not on the legacy folder table. It looks obviously safe in review and is not.