fix(store): run SQLite startup migrations inside one transaction (BACKLOG #1586)

[wshallwshall]

What changed

MessageStore.open now runs _migrate and its commit inside _writer_txn, the store's existing writer-transaction helper. Before this, the SQLite store ran its startup migrations with no BEGIN. Each ALTER TABLE ... ADD COLUMN committed on its own. A failure between an ADD and its paired backfill (users.password_claimed_at, #1245; users.notify_email, #1139) left the column present. The next open's column-missing guard then skipped the backfill forever.

The fix is small; the placement is what matters. The transaction opens after the connection PRAGMAs and after executescript(_SCHEMA), and nowhere earlier works:

• Before the PRAGMAs, PRAGMA journal_mode=WAL raises cannot change into wal mode from within a transaction, and PRAGMA foreign_keys=ON is a silent no-op.
• Before executescript, its implicit COMMIT ends the transaction before _migrate runs.

_writer_txn rolls back on BaseException with its bounded, shielded unwind. It needs a lock, so open passes a fresh asyncio.Lock(); nothing else can reach this connection yet. The #1670 cleanup in open's except BaseException arm is unchanged and still closes the connection; the new test asserts its worker thread is gone.

I also corrected two comments whose premise this change makes false: the FIFO index swap note in _migrate, and the docstring of test_partial_migration_converges_and_stays_correct in tests/test_fifo_index_migration.py.

Where this disagreed with the brief

1. I used _writer_txn rather than a bare BEGIN. tests/test_writer_txn_is_the_only_begin.py pins every BEGIN in store.py to _writer_txn and _read. A bare BEGIN in open would red it. Reusing the helper keeps that guard unchanged and gets the BaseException unwind for free.
2. A BEGIN before the PRAGMAs is loud, not only silent. The brief expected only the foreign-keys no-op. Measured: journal_mode=WAL raises first, so every open fails.
3. The brief named one test file; this touches two. The second edit is the docstring correction above, with no logic change.
4. I built the legacy database by hand, not by dropping columns. The test writes users as it stood before any column _migrate adds, primary key included, then runs _SCHEMA over it with the stdlib driver. DROP COLUMN fails on password_claimed_at because of its trailing comment, and the rebuild workaround in test_auth_service.py loses the primary key.

Readings 1, 3 and 4 in the brief matched my measurement (Python 3.14.6, SQLite 3.50.4, aiosqlite 0.22.1, isolation_level == ''). Nothing in _migrate or _migrate_outbox_to_queue commits, runs a PRAGMA that needs to sit outside a transaction, or calls executescript.

Placement proof

I moved the transaction to each wrong spot, ran tests/test_backlog1586_sqlite_migrate_txn.py, and reverted. "interrupted backfill" is the test parametrized over the two backfilled columns. "any addition" interrupts after every ADD the clean run issues (11 on this file). "PRAGMAs" checks foreign_keys == 1 and journal_mode == 'wal'.

Placement	interrupted backfill (x2)	any addition	PRAGMAs
A. No BEGIN (origin/main)	FAIL: migration left changes on disk	FAIL: file changed after channel_scope ADD	pass
B. BEGIN before the PRAGMAs	FAIL: cannot change into wal mode	FAIL: same	FAIL: same
C. BEGIN between synchronous and foreign_keys	FAIL: changes on disk	FAIL: file changed	FAIL: foreign_keys read 0
D. BEGIN between the PRAGMAs and executescript	FAIL: changes on disk	FAIL: file changed	pass
E. This PR (after executescript, via _writer_txn)	pass	pass	pass

The defect reproduces at origin/main. With the on-disk assertion disabled, the row's own acceptance check still fails at A: after the reopen, alice's password_claimed_at is NULL where a clean migration gives 1000.0.

Checks run

• ruff check messagefoundry tests: pass
• ruff format --check messagefoundry tests: pass (1096 files)
• mypy messagefoundry (strict): pass, 275 files
• Targeted pytest: 243 passed, 6 skipped, over test_backlog1586_sqlite_migrate_txn.py, test_auth_store.py, test_auth_service.py, test_fifo_index_migration.py, test_retention.py, test_store_schema_hash.py, test_writer_txn_is_the_only_begin.py, test_backlog1548_writer_txn_cancel_unwind.py, test_store.py. The 6 skips are the SQL Server and Postgres cases, which need MEFOR_TEST_* env.
• pre-commit hooks: all passed.
• /simplify: the four review agents ended without returning a report, so I reviewed the diff myself. One cleanup applied: the sqlite3 read helpers use contextlib.closing.

Checks NOT run

• The full pytest suite. Only the targeted files above ran locally.
• The SQL Server and Postgres CI legs. This change does not touch those backends, but please read those legs before landing.

One thing I did not measure

_writer_txn issues a deferred BEGIN. On a database that needs migrating, the first write inside the transaction upgrades a read lock. If another process commits to the same file in that window, SQLite can return SQLITE_BUSY without running the busy handler. Before this change, each ALTER ran in its own autocommit and waited on busy_timeout. The failure is clean (rollback, open raises, the next open retries). It only matters when two processes open one SQLite file during an upgrade, and I did not test that case.

Ledger banner text for the Lander

#1586, landed in this PR (pending merge): MessageStore.open now runs the SQLite startup migrations inside one transaction through _writer_txn. It opens after the connection PRAGMAs and after executescript(_SCHEMA), and rolls back on any failure. An interrupted ALTER TABLE ... ADD COLUMN now leaves the file unchanged, so the password_claimed_at and notify_email backfills run on the next open. tests/test_backlog1586_sqlite_migrate_txn.py interrupts after every column addition on a hand-built legacy database. It requires the file unchanged, then two reopens that match a clean migration, plus foreign_keys=1 and WAL on the opened store. Each wrong placement fails at least one of those tests (table in the PR body).

[github-actions]

Security failed while this pull request was in the merge queue, so the queue ejected it.

Its own head can still be green: the queue revalidates the merge, and the path gates that skip on a pull request run there. Read the run before retrying.

https://github.com/MEFORORG/MessageFoundry/actions/runs/35498606442