diff --git a/tests/registry.json b/tests/registry.json index cd8c29853..232432cd4 100644 --- a/tests/registry.json +++ b/tests/registry.json @@ -1948,6 +1948,19 @@ ], "description": "Migration runner atomicity + cross-process lock (#1160). _atomic_rebuild rename-swap inside an explicit transaction closes the DROP-rebuild data-loss window (crash mid-rebuild rolls back with no data loss; verified for agent_sharing + agent_skills incl. lowercase/NULL-email behavior preservation and index recreation); replay-after-partial-apply completes cleanly; a failed migration is named on its traceback via add_note (original exception type preserved). Cross-process flock (db/migration_lock.py) serialises concurrent boots — multiprocessing tests prove mutual exclusion and a safe concurrent rebuild." }, + { + "file": "unit/test_migrations.py", + "feature": "#3395", + "added": "2026-10-26", + "categories": [ + "backend", + "unit", + "database", + "migrations", + "guard" + ], + "description": "Migration runner unit tests (#236): schema_migrations tracking, idempotency and resume, failure propagation (only `no such table` is a fresh-install skip), and MIGRATIONS sanity. #3395 adds the registry bijection: the `_migrate*` functions defined in db/migrations.py, discovered from the file's parsed source (top-level `def` or `async def`, decorated or not) and cross-checked against the callables the loaded module binds (non-vacuity), equal the functions registered in MIGRATIONS, compared by identity, each registered exactly once. An unregistered function (including a data-only backfill no schema diff can see, and a decorated or `async def` one) is named; so is a registered function that is not a `_migrate*` of the module, and an `async def` migration, which the runner would record as applied without running. A deliberately unregistered helper must not use the `_migrate` prefix." + }, { "file": "unit/test_compatibility_checks.py", "feature": "#668", diff --git a/tests/unit/test_migrations.py b/tests/unit/test_migrations.py index 655d39729..93c295a79 100644 --- a/tests/unit/test_migrations.py +++ b/tests/unit/test_migrations.py @@ -8,8 +8,13 @@ Uses an in-memory SQLite database. """ +import ast +import functools import importlib.util +import inspect import sqlite3 +import types +from collections.abc import Callable, Sequence from datetime import datetime, timezone from pathlib import Path from unittest.mock import patch @@ -279,6 +284,165 @@ def test_fan_out_id_migration_present(self): names = [name for name, _ in MIGRATIONS] assert "execution_fan_out_id" in names + def test_every_migrate_function_is_registered_and_vice_versa(self): + """The registry is a bijection with the module's `_migrate*` functions (#3395). + + `run_all_migrations` runs only what `MIGRATIONS` lists, so a `_migrate*` + function that is written but never registered never runs on an existing + install. No schema diff can see the data-only case (an `UPDATE` + backfill), and a column the function adds is masked on a fresh install + whenever `init_schema` creates the table. This generalises + `test_fan_out_id_migration_present` from one name to every name, in both + directions: every `_migrate*` function defined in `db/migrations.py` is + registered exactly once, and every registered function is one of them. + + Rule: a helper in `db/migrations.py` that is deliberately not a + migration must not use the `_migrate` prefix. + + The functions are discovered from the file's source, never from a + hand-written list: every top-level `def` or `async def` named + `_migrate*`, decorated or not. Each name resolves to the object the + loaded module binds, compared by identity against the `MIGRATIONS` + captured at import (other tests `patch.object` the module attribute). + The non-vacuity floor keeps the test from passing on an empty or + partial discovery, e.g. after `db/migrations.py` is split into a + package: the discovered names must be non-empty and equal the + `_migrate*` callables the loaded module binds. + + An `async def` migration is refused even when registered: + `run_all_migrations` calls it without awaiting it, so its body never + runs and the migration is still recorded as applied. + + Other shape: none of the delivery brief's six cases (fresh install, + empty data, another origin, flag off, clock ahead, restricted + container) applies to a module-introspection test; the second case is + the converse direction (a registered function that is not a + `_migrate*` function of this module). + """ + self._assert_registry_bijection(_migrations_mod, MIGRATIONS) + + @staticmethod + def _assert_registry_bijection( + module: types.ModuleType, + migrations: Sequence[tuple[str, Callable[..., object]]], + ) -> None: + assert module.__file__ is not None, f"{module.__name__} has no source file" + module_file = Path(module.__file__).resolve() + # Read from the source, so a decorator or `async def` cannot hide one. + source_names = { + node.name + for node in ast.parse(module_file.read_text(encoding="utf-8")).body + if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)) + and node.name.startswith("_migrate") + } + registered = {fn for _, fn in migrations} + + # Non-vacuity: discovery is non-empty and agrees with a second, + # independent derivation from the loaded module, so an empty or + # partial discovery cannot make the bijection pass. + bound_names = { + name + for name, obj in vars(module).items() + if name.startswith("_migrate") and callable(obj) + } + assert source_names, f"no _migrate* functions defined in {module_file}" + assert registered, "MIGRATIONS is empty" + assert source_names == bound_names, ( + f"the top-level _migrate* defs in {module_file} disagree with the " + "_migrate* callables the loaded module binds: only in source " + f"{sorted(source_names - bound_names)}, only bound " + f"{sorted(bound_names - source_names)}" + ) + # The bound object is the one MIGRATIONS holds (the wrapper, if decorated). + defined = {getattr(module, name): name for name in source_names} + + problems = [] + unregistered = sorted( + name for fn, name in defined.items() if fn not in registered + ) + if unregistered: + problems.append( + "_migrate* functions defined in db/migrations.py but not registered " + f"in MIGRATIONS (existing installs never run them): {unregistered}" + ) + foreign = sorted( + f"{name} -> {fn.__module__}.{fn.__qualname__}" + for name, fn in migrations + if fn not in defined + ) + if foreign: + problems.append( + "MIGRATIONS entries whose function is not a _migrate* function " + f"defined in db/migrations.py: {foreign}" + ) + entries_by_fn: dict[Callable[..., object], list[str]] = {} + for name, fn in migrations: + entries_by_fn.setdefault(fn, []).append(name) + repeated = sorted( + f"{fn.__name__} <- {names}" + for fn, names in entries_by_fn.items() + if len(names) > 1 + ) + if repeated: + problems.append( + f"functions registered under more than one MIGRATIONS entry: {repeated}" + ) + never_run = sorted( + name + for fn, name in defined.items() + if inspect.iscoroutinefunction(fn) or inspect.isasyncgenfunction(fn) + ) + if never_run: + problems.append( + "async _migrate* functions: run_all_migrations calls each migration " + "without awaiting it, so the body never runs and the migration is " + f"still recorded as applied: {never_run}" + ) + assert not problems, "\n".join(problems) + + def test_registry_bijection_names_unregistered_decorated_and_async_migrations( + self, tmp_path: Path + ) -> None: + """An unregistered decorated or `async def` `_migrate*` is named as unregistered (#3395).""" + module = _load_synthetic_migrations( + tmp_path, + "def _migrate_plain(cursor, conn):\n pass\n\n" + "@foreign_decorator\ndef _migrate_decorated(cursor, conn):\n pass\n\n" + "async def _migrate_async(cursor, conn):\n pass\n\n" + "MIGRATIONS = [('plain', _migrate_plain)]\n", + ) + with pytest.raises(AssertionError) as raised: + self._assert_registry_bijection(module, module.MIGRATIONS) + message = str(raised.value) + assert ( + "not registered in MIGRATIONS (existing installs never run them): " + "['_migrate_async', '_migrate_decorated']" + ) in message, message + assert "still recorded as applied: ['_migrate_async']" in message, message + + def test_registry_bijection_accepts_a_registered_decorated_migration( + self, tmp_path: Path + ) -> None: + """A decorator from another module does not hide a registered migration.""" + module = _load_synthetic_migrations( + tmp_path, + "@foreign_decorator\ndef _migrate_decorated(cursor, conn):\n pass\n\n" + "MIGRATIONS = [('decorated', _migrate_decorated)]\n", + ) + self._assert_registry_bijection(module, module.MIGRATIONS) + + def test_registry_bijection_refuses_a_registered_async_migration( + self, tmp_path: Path + ) -> None: + """Registering an `async def` migration does not make it run.""" + module = _load_synthetic_migrations( + tmp_path, + "async def _migrate_async(cursor, conn):\n pass\n\n" + "MIGRATIONS = [('async', _migrate_async)]\n", + ) + with pytest.raises(AssertionError, match=r"never runs.*\['_migrate_async'\]"): + self._assert_registry_bijection(module, module.MIGRATIONS) + # --- helpers --- @@ -287,3 +451,27 @@ def wrapper(cursor, conn): log.append(name) return original_fn(cursor, conn) return wrapper + + +def _foreign_decorator( + fn: Callable[[object, object], None], +) -> Callable[[object, object], None]: + """A decorator defined outside the migrations module, as a shared helper would be.""" + + @functools.wraps(fn) + def wrapper(cursor: object, conn: object) -> None: + fn(cursor, conn) + + return wrapper + + +def _load_synthetic_migrations(tmp_path: Path, source: str) -> types.ModuleType: + """Load `source` as a stand-in `migrations.py`, with `foreign_decorator` in scope.""" + path = tmp_path / "migrations.py" + path.write_text(source, encoding="utf-8") + spec = importlib.util.spec_from_file_location("synthetic_migrations", path) + assert spec is not None and spec.loader is not None, f"cannot load {path}" + module = importlib.util.module_from_spec(spec) + module.__dict__["foreign_decorator"] = _foreign_decorator + spec.loader.exec_module(module) + return module