From c0c540c1940b57281915a4990ef4797e9201e402 Mon Sep 17 00:00:00 2001 From: Joshua King Date: Mon, 14 Sep 2026 07:05:13 -0400 Subject: [PATCH] fix: re-read schema version inside each migration transaction; spy on close in setup-failure test - apply_migrations re-reads the version under the transaction lock before applying, so two interleaved runners on one connection cannot both apply a non-idempotent migration from a stale read - setup-failure test now spies on NestQuestDatabase.close and asserts it ran and no runtime record was stored (was vacuous) --- custom_components/nestquest/migrations.py | 64 ++++++++++++++++------- tests/test_migrations.py | 27 ++++++++-- 2 files changed, 68 insertions(+), 23 deletions(-) diff --git a/custom_components/nestquest/migrations.py b/custom_components/nestquest/migrations.py index 8f35717..00526b9 100644 --- a/custom_components/nestquest/migrations.py +++ b/custom_components/nestquest/migrations.py @@ -131,6 +131,13 @@ async def apply_migrations( migrations (database already current) is a no-op that logs at debug level and opens no write transaction. + The version is re-read inside each migration transaction before + deciding to apply it: the outer read is advisory only (loop bounds, + no-op fast path), and on one shared connection two interleaved + runners would otherwise both read the same version and both apply + a non-idempotent migration. Under the transaction's lock the + second runner sees the first runner's stamp and skips. + Raises whatever the failing migration raised, with the database rolled back to its prior version and prior rows untouched. """ @@ -153,25 +160,46 @@ async def apply_migrations( return current for target in range(current + 1, latest + 1): - statements = migrations[target - 1] + applied = False async with database.transaction(): - for sql in statements: - await database.execute(sql) - # UPDATE keyed on the singleton row (id = 1): a fresh file - # has no row yet, but migration 1 is the only migration a - # version-0 database can run, so the row is seeded by the - # same statement batch when needed. UPDATE alone would - # silently match zero rows there, so seed-if-absent first. - await database.execute( - f"INSERT INTO {VERSION_TABLE} (id, version) VALUES (1, ?) " - "ON CONFLICT (id) DO UPDATE SET version = excluded.version", - (target,), + # Re-read the version INSIDE the write transaction. The + # version read before the loop is advisory (it sizes the + # loop and enables the byte-identical no-op fast path), but + # two runners on one connection interleave between their + # reads: the wrapper serializes individual statements, not + # the read-decide-apply decision. Re-reading under the + # transaction's lock means the loser of the race sees the + # winner's stamp and skips instead of re-applying a + # non-idempotent migration with a stale version. + in_tx_version = await read_schema_version(database) + if in_tx_version > latest: + raise RuntimeError( + f"Database schema version {in_tx_version} is newer " + f"than this integration understands (latest known: " + f"{latest}); downgrades are not supported" + ) + if in_tx_version < target: + for sql in migrations[target - 1]: + await database.execute(sql) + # INSERT keyed on the singleton row (id = 1): a fresh + # file has no row yet, and migration 1 is the only + # migration a version-0 database can run, so the row is + # seeded by the same statement batch when needed. + # UPDATE alone would silently match zero rows there. + await database.execute( + f"INSERT INTO {VERSION_TABLE} (id, version) " + "VALUES (1, ?) " + "ON CONFLICT (id) DO UPDATE SET version = " + "excluded.version", + (target,), + ) + applied = True + if applied: + LOGGER.info( + "NestQuest schema migrated to version %d (%d statement%s)", + target, + len(migrations[target - 1]), + "s" if len(migrations[target - 1]) != 1 else "", ) - LOGGER.info( - "NestQuest schema migrated to version %d (%d statement%s)", - target, - len(statements), - "s" if len(statements) != 1 else "", - ) return latest \ No newline at end of file diff --git a/tests/test_migrations.py b/tests/test_migrations.py index 5e7e844..7467e3b 100644 --- a/tests/test_migrations.py +++ b/tests/test_migrations.py @@ -574,9 +574,16 @@ async def test_options_reload_reopens_and_rechecks_version( async def test_setup_entry_failure_closes_database( hass, make_entry, monkeypatch ) -> None: + """A failing migration closes the opened connection before re-raising. + + Spies on the real wrapper instance: records whether close() ran and + whether the connection is left disconnected, rather than inferring + from hass.data (a failed setup never stores a runtime record). + """ import pytest as pytest_module import custom_components.nestquest as nestquest_module + from custom_components.nestquest.db import NestQuestDatabase from custom_components.nestquest.migrations import ( MIGRATIONS as _MIGRATIONS, ) @@ -587,12 +594,22 @@ async def test_setup_entry_failure_closes_database( monkeypatch.setattr( nestquest_module, "apply_migrations", _failing, raising=True ) + + close_calls: list[bool] = [] + original_close = NestQuestDatabase.close + + async def _spy_close(self): + close_calls.append(True) + return await original_close(self) + + monkeypatch.setattr(NestQuestDatabase, "close", _spy_close) + entry = make_entry() with pytest_module.raises(RuntimeError, match="boom"): await _setup_entry(hass, entry, hass.registry) - # Setup closed the connection before re-raising: no leaked handle. - # The record only lands in hass.data after the listener is added, so - # on failure the connection must be gone from the wrapper itself. + + assert close_calls == [True], ( + "failed setup must close the connection it opened" + ) record = hass.data["nestquest"].get(entry.entry_id) - if record is not None: - assert not record.database.connected \ No newline at end of file + assert record is None, "failed setup must not store runtime data" \ No newline at end of file