feat(merge): protect user data across re-merges — orphan audit + ID remap (refs #242) - #248
Merged
Conversation
…r (refs #242) Phase 1 of #242. New auditOrphans() counts user_stars/playlist_tracks/ play_events rows whose unified target vanished in the destructive re-merge, and runMergePipeline() wraps mergeLibraries()+audit in one transaction at every merge call site (sync.ts, auto-sync.ts, admin peer-data reset). Report persists as JSON in the new sync_operations.details column (additive migration); non-zero totals log a warning. Report-only — Phase 2 adds the ID remap that prevents the orphaning. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011Q3riZ78G26ohMgPDh8F7u
… (refs #242) Phase 2 of #242. snapshotIdentity() captures instance-row -> unified-ID mappings before the destructive rebuild; applyRemap() diffs them after and set-based-UPDATEs user_stars / playlist_tracks / play_events onto the new IDs, so metadata edits and dedup-key changes no longer strand user data. Splits keep the majority-instance-row winner (splitsLogged); user_stars collisions resolve via stage/delete/reinsert — swap-cycle-safe (a tag fix that swaps two tracks' titles remaps both stars instead of deleting them). Drops the ON DELETE CASCADE FK on playlist_tracks.unified_track_id (table-recreate migration): it made every merge delete playlist rows outright. The table now follows the same no-FK / orphans-dropped-at-read convention as user_stars and play_events. runMergePipeline is now snapshot -> merge -> remap -> audit in one transaction and returns { orphans, remap }, both persisted to sync_operations.details. New pitfalls row: never call mergeLibraries() directly outside tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011Q3riZ78G26ohMgPDh8F7u
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs #242 (Phases 1–2; Phase 3 — worker-thread merge — deferred to a follow-up PR per owner decision, so this does not close the issue).
mergeLibraries()destructively rebuilds allunified_*tables, and unified IDs are deterministic metadata hashes — so any metadata edit or dedup-key change silently reshuffles IDs and strandsuser_stars/playlist_tracks/play_eventsrows. This PR makes user data survive every re-merge, which unblocks #158/#160/#13 (all change ID derivation and were unsafe to merge before this).auditOrphans()counts + samples rows whose unified target vanished (stars by kind, playlist_tracks, play_events);runMergePipeline()orchestrator wraps every merge call site (sync.ts,auto-sync.ts×2, admin peer-data reset); report persisted as JSON in newsync_operations.detailscolumn (additive migration).snapshotIdentity()before the rebuild,applyRemap()after — old→new pairs derived from stable per-instance identities (track_sources.instance_track_id,unified_release_sources.instance_album_id→ release group,unified_artist_sources.instance_artist_id), applied set-based via a TEMP pairs table. Pipeline is snapshot → merge → remap → audit in one transaction.Design notes
playlist_tracksgets orphaned like the other two tables, but it had anON DELETE CASCADEFK tounified_tracks— withforeign_keys = ON, every merge was deleting playlist rows outright (invisible today only because playlist endpoints are stubs). Phase 2 drops the FK via a table-recreate migration, aligning it with the documented no-FK / orphans-dropped-at-read convention ofuser_stars/play_events.user_starsremap (UPDATE OR IGNORE+ delete-losers) lost both stars when remap pairs form a cycle — exactly what a tag fix that swaps two tracks' titles produces (A→B, B→A). Final implementation stages remapped rows in a TEMP table, deletes the touched rows, thenINSERT OR IGNOREs the staged finals; regression test included. The plain UPDATEs onplaylist_tracks/play_eventsare single-pass-safe and unchanged.splitsLogged. Collisions (two old IDs → one new ID): one star per user survives, losers counted ascollisionsDropped.merge.tsitself is untouched (PK-conflict logging preserved);merge.test.tspasses unmodified, proving unified table contents are unchanged.mergeLibraries()directly outside tests —runMergePipeline()is the only sanctioned entry point.Verification
pnpm verifygreen: hub 691/691 (58 files), frontend 108/108 (16 files), typecheck +lint:boundaryclean.pnpm lintzero findings in bothhub/andfrontend/.orphan-audit.test.ts(6),id-remap.test.ts(6 — metadata edit, full-library key-shape change via saltedgenerateTrackId, split, collision, swap-cycle, playlist survival),playlist-tracks-migration.test.ts(2 — old-shape DB migrates with rows preserved, fresh DB no-ops).Acceptance criteria from #242 (Phases 1–2 scope)
generateTrackId) preserves 100% of stars/playlists/play_events.sync_operations.detailsafter every merge./rest/pinglatency during large merges — Phase 3 (worker thread), follow-up PR.🤖 Generated with Claude Code
https://claude.ai/code/session_011Q3riZ78G26ohMgPDh8F7u