Conversation
|
Contributor
Author
|
Closing: the duplicate sync_states rows came from a downstream storage engine, not from StorageKnex. Not needed upstream. |
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
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.




Problem
sync_stateshas no uniqueness on (userId,storageIdentityKey). Two concurrentfindOrInsertSyncStateAuthcalls for a new source can both miss the lookup and both insert. From then on every lookup for that pair finds two rows and fails withStorage identity has conflicting sync states, andprocessSyncChunkfails inverifyOne, so sync with that source stays broken for the user. We hit this in production (37 duplicate pairs).Fix
2026-09-30-001 unique sync state per storage identity:syncStateId) per (userId,storageIdentityKey). Nothing referencessyncStateIdby foreign key.WalletStorageManagerreads the checkpoint (and itssyncStateId) throughfindOrInsertSyncStateAuthat the start of each sync pass, so only a pass in flight across the migration can carry a deleted id.storageNames (the legacy casefindOrInsertSyncStateAuthstill tolerates), the kept row is restarted as a new sync state (syncMap,when,status,init), since its checkpoint may belong to a different source database that reused the identity key.sync_states_user_storage_identity.downdrops it, restoring the MySQLsync_states_userid_foreignindex first if MySQL discarded it.findOrInsertSyncStateAuthcatches the losing insert's unique violation and re-reads the winner's row, which the index now makes happen.Tests
test/storage/syncStateIdentity.test.ts: two concurrentfindOrInsertSyncStateAuthcalls on SQLite return the same row (oneisNew); with the index dropped the same race inserts two rows; a direct duplicate insert is rejected.test/storage/KnexMigrations.test.ts: migration cleanup (same-name duplicates keep the oldest checkpoint, different-name duplicates restart, unrelated rows untouched), index enforced,downremoves it; MySQL rollback restores the FK support index.test/storage/portable.test.ts: merge into a target whose row for the identity has a different name; restore of a legacy export with duplicate sync states.Local: wallet-toolbox jest (
--testPathIgnorePatterns='man.test.ts|live.test.ts|bench.test.ts|client/test|mobile/test') 287 suites / 3117 passed, 1 skipped;pnpm lint,pnpm format:check,pnpm typecheck,pnpm health:checkpass. MySQL and Postgres were not run locally.