Skip to content

fix(wallet-toolbox): enforce one sync state per storage identity - #723

Closed
shruggr wants to merge 3 commits into
bsv-blockchain:mainfrom
shruggr:fix/sync-states-unique
Closed

shruggr wants to merge 3 commits into
bsv-blockchain:mainfrom
shruggr:fix/sync-states-unique

Conversation

@shruggr

@shruggr shruggr commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Problem

sync_states has no uniqueness on (userId, storageIdentityKey). Two concurrent findOrInsertSyncStateAuth calls 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 with Storage identity has conflicting sync states, and processSyncChunk fails in verifyOne, so sync with that source stays broken for the user. We hit this in production (37 duplicate pairs).

Fix

  • New Knex migration 2026-09-30-001 unique sync state per storage identity:
    • deletes duplicate rows, keeping the oldest (lowest syncStateId) per (userId, storageIdentityKey). Nothing references syncStateId by foreign key. WalletStorageManager reads the checkpoint (and its syncStateId) through findOrInsertSyncStateAuth at the start of each sync pass, so only a pass in flight across the migration can carry a deleted id.
    • when the duplicates had different storageNames (the legacy case findOrInsertSyncStateAuth still 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.
    • adds unique index sync_states_user_storage_identity. down drops it, restoring the MySQL sync_states_userid_foreign index first if MySQL discarded it.
    • plain knex query builder only, so it should also run on the Postgres dialect in feat(wallet-toolbox): run StorageKnex on Postgres #719.
  • Insert path: no code change needed. The existing retry in findOrInsertSyncStateAuth catches the losing insert's unique violation and re-reads the winner's row, which the index now makes happen.
  • BRC-38 import: an export with several sync states for one identity is collapsed by the same rule, and merge matches the target's sync state by identity only (exact name preferred when IndexedDB holds legacy duplicates), so a renamed source updates the existing row instead of violating the index.
  • CHANGELOG, README, release notes for the 2.14.5 candidate.

Tests

  • test/storage/syncStateIdentity.test.ts: two concurrent findOrInsertSyncStateAuth calls on SQLite return the same row (one isNew); 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, down removes 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.
  • Fixtures that inserted several sync states for one user and identity now use distinct identities.

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:check pass. MySQL and Postgres were not run locally.

@sonarqubecloud

sonarqubecloud Bot commented Oct 1, 2026

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

@shruggr

shruggr commented Oct 1, 2026

Copy link
Copy Markdown
Contributor Author

Closing: the duplicate sync_states rows came from a downstream storage engine, not from StorageKnex. Not needed upstream.

@shruggr shruggr closed this Oct 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant