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 bsv-blockchain#538
|
|
Correction to my own numbers, made in the body above. The migration counts I first posted (21 This makes the guard option worse than I described, not better, so it does not change the recommendation — but the published numbers were wrong and are now corrected. The defect, the reproduction and the Counting rule for anything quoted above: literal occurrence counts, and where stated, restricted to |
1 participant
Opened as a draft: I cannot complete local validation on the declared root toolchain (see Verification). Hosted CI on this head is the real gate.
Program and scope
StorageKnex.migrate()disables per-file transactions, so DDL autocommits whileknex_migrationsis written only after the whole file'sup()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 latermigrate.latest()re-runs that file from its first statement and fails ontable ... already exists, 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.dropAllData()'s owndisableTransactions: isSQLite(a migrate-down path used by test tooling) is deliberately untouched. No migration file is edited.88130dc(branched from82ab4d3c)Impact
Affected:
@bsv/wallet-toolbox— one file,src/storage/StorageKnex.ts, +17 −5 (one behavioural value; the rest is comment). No signature, export, or type changes. MySQL behaviour is unchanged by construction:isSQLiteis alreadyfalsethere, sodisableTransactionswas alreadyfalse. Only the SQLite branch changes.Verification
jest --testPathIgnorePatterns='man.test.ts|live.test.ts|bench.test.ts|client/test|mobile/test'— 216/216 suites, 2023 passed, 1 skipped, exit 0. The changed line is genuinely covered: 19 test files call.migrate()against SQLite, including the sharedtest/utils/TestUtilsWalletStorage.tsharness.conformance/vectors/sync/brc40-user-state.jsonis 24 vectors, 0required/24intended, andconformance/runner/ts/dispatchers/sync.tsimports no wallet-toolbox code (it reimplements the merge rule locally), so BRC-40 conformance would not detect a regression here either way.Behavioural evidence
Reproduced and fixed against a built artifact, driving the real
StorageKnex.migrate()(interrupting one migration after its first statement, then migrating again as the app's next launch would):proven_txsexists, journal 0 rowstable proven_txs already exists, and on every retryproven_txsrolled backOn
PRAGMA foreign_keys— the reason the previous comment gaveThe replaced comment cites knex#4155. That constraint is real, but it is already satisfied without disabling transactions:
migrate()issuesPRAGMA foreign_keys = OFFoutsidemigrate.latest(), and SQLite's single-connection pool means the migration transaction runs on that connection. Measured by readingPRAGMA foreign_keysfrom inside a migration underbetter-sqlite3:0with transactions both enabled and disabled.sqlite3/schema/ddl.jsalter()usesenforceForeignCheck = this.client.transacting ? null : false— deliberately leaving an ambient pragma alone when already transacting.Because
up()migrations carry 12.alter()calls across 8alterTableblocks — each a full table rebuild on SQLite — I tested that path on a populated table rather than the fresh database the suite uses, under both settings:The orphan is an FK-violating row: it survives only if foreign keys are genuinely off during the rebuild. Identical under both.
Why not idempotence guards
Extending the
SYNC_TRANSFER_MIGRATIONhasTablepattern was the first choice and was rejected on counting. Countingup()regions only on this head: 49 DDL sites — 25createTable, 4 column-addingalterTable, 13 index-addingalterTable, and 8alterTableblocks carrying 12.alter()calls. knex exposes no portablehasIndexfor the index group, and an.alter()rebuild cannot be guarded by presence at all, since the column exists before and after. Guards are therefore a partial fix that also never closes the journal-write window.Security and dependencies
Release and operations
Completion evidence
Disclosure
The root manifest declares
engines.node >= 24.11. I do not have that toolchain available and built/tested under Node v22.22.0 with--config.engine-strict=false(the wallet-toolbox package's own floor is>=22). Everything above was measured under that configuration. Nothing should be re-qualified from my run — please treat hosted CI on the exact head as the evidence of record. I am happy to re-run anything locally in a form you specify, or to rework this as guards instead if you prefer that direction.