fix(wallet): preserve transaction ownership and sync basket mappings - #561
Conversation
…nsaction With transactions disabled for SQLite, each DDL statement autocommits while knex records the migration in knex_migrations only after the whole file's up() resolves. A process killed mid-file — or between a file's last statement and the journal insert — leaves objects the journal never recorded, and every later migrate.latest() re-runs that file from its first statement and fails on "table ... already exists" or "duplicate column name", permanently, with no in-package recovery path. For a wallet with a local SQLite store, a force-quit during first-run bring-up is enough. SQLite DDL is transactional, so letting knex wrap each migration file rolls an interrupted migration back whole and keeps its journal insert atomic with its DDL. This also covers the alterTable and index-adding migrations, which statement-level idempotence guards cannot: knex exposes no portable hasIndex, and guards never close the journal-write window. PRAGMA foreign_keys is still issued outside migrate.latest(). SQLite's single-connection pool means the migration transaction inherits it, and knex's own alter-table rebuild deliberately leaves an ambient pragma alone while transacting (sqlite3/schema/ddl.js: enforceForeignCheck = transacting ? null : false), so the knex#4155 constraint the previous comment describes still holds. MySQL behaviour is unchanged: isSQLite was already false there, so disableTransactions was already false. dropAllData()'s own setting is deliberately untouched. Closes #538
…sqlite-migration-integration # Conflicts: # packages/wallet/wallet-toolbox/src/storage/StorageKnex.ts
…n-integration # Conflicts: # docs/reference/package-api-migrations.md
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
ty-everett
left a comment
ty-everett
left a comment
There was a problem hiding this comment.
Final maintainer critical review of 4be36b1: reviewed transaction-owned connection use, uncached transaction-local settings, background-work isolation, optional BEEF handling, SQLite/MySQL substring selection, basket-map validation before mutation, newer-update repair and equal/older conflict preservation. Regression evidence covers real SQLite rollback/retry, negative proof against the prior implementation, full wallet coverage and exact-tarball browser/mobile profiles. CI 35812837677 and every required check pass, including Codecov patch and zero-new-Sonar; no open review threads or CodeQL merge-ref alerts. Prospective merge tree onto current main 77df0ca equals the tested head tree. No blocking finding remains. Self-review is recorded as a comment because GitHub cannot approve the author’s own PR. Coordinated release remains gated on combined-main acceptance.
2 participants
Program and scope
Closes #540. Closes #543. Coordination: #558.
A cold raw-transaction read currently calls store-wide startup from inside its caller's transaction, attempting to acquire SQLite's already-owned single connection. Read settings through the caller's transaction and keep them out of the shared cache; do not start prepared-BEEF background work before commit. The same lookup now tolerates an absent optional
inputBEEFinstead of throwing while returning stored raw bytes.Output sync currently converts a missing basket mapping into an unbasketed output. Validate the source and mapped IDs before changing a new entity or applying a newer update. Newer valid updates now apply basket assignment/removal. Equal or older updates retain existing conflict behavior, including deliberate local relinquishment.
Impact and release
@bsv/wallet-toolbox, compatible fixes in the existing unpublished 2.13.2 candidate. No public signature, wire, schema, runtime-target or dependency change.Verification
Declared Node 24.18.0 / pnpm 10.33.2:
4be36b1908e6e7578f61c50af61c742d3af25646: CI run.77df0ca8exactly matches the tested head tree.Publication and deployment remain in the coordinated release after integration and full main acceptance.
Coverage follow-up adds a public StorageKnex MySQL query-compilation regression through transaction-local settings (no live MySQL claim) and a spent-output mapping comparison regression. All 22 focused cases pass. Combined integration with #539 and the current dependency-governance correction passes; thresholds and exclusions are unchanged.