From 6ea750a6987fffc68c10984f4fe2d936b3f5356e Mon Sep 17 00:00:00 2001 From: npeter83 Date: Tue, 20 Oct 2026 01:56:31 +0200 Subject: [PATCH] test: the commit check starts every request clean, and the fence docs claim what it holds The test's own writes are committed before a request, so a handler's Core DML can no longer hide in a transaction the test had already opened. The entry-point probe answers on a marked line the API's logging cannot mix into. A listed commit whose function no longer commits fails the fence. CLAUDE.md and the backend README now name what an AST cannot see; the route-commit tests no longer claim their read-back holds the commit. --- CLAUDE.md | 8 +++---- backend/app/README.md | 11 ++++++---- backend/tests/conftest.py | 16 +++++++------- backend/tests/test_entry_points.py | 12 ++++++++--- backend/tests/test_override_dep_convention.py | 12 +++++++++-- backend/tests/test_unit_fences.py | 21 +++++++++++++++++++ 6 files changed, 59 insertions(+), 21 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index f9fa1efe..dac7149b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -70,9 +70,9 @@ The domain is four **units**, each owning its tables: Capture (`downloads/`), Re sync), Catalog (`catalog/`, the `sync/` ingests) and Plex (`plex/`). A unit's route does no database work: it reads through the unit's `queries`, writes through its `service` (which flushes) and commits. A unit's code reads another unit's tables only along a declared edge and writes them only through that unit's service; core reaches a unit's tables through its -`queries`. **Fence-held, with no exemptions** (`backend/tests/test_unit_fences.py`) — as far as an AST sees: an ORM -attribute write on another unit's loaded row is review's to catch. Conventions in -[`backend/app/README.md`](backend/app/README.md). +`queries`. **Fence-held, with no exemptions** (`backend/tests/test_unit_fences.py`) — as far as an AST sees: what it +cannot (an ORM write on another unit's loaded row, SQL in `text()`, a commit by `begin()`) is review's to catch, listed in +[`backend/app/README.md`](backend/app/README.md) with the conventions. Frontend: `main.tsx` provider tree → `App.tsx` shell (a shared `/watch` link boots from its own entry, `watch.html` → `watchMain.tsx`, held app-free by a contract test) → `moduleRegistry` route-chunks → module components → the **platform layer `components/ui/`** (domain-free primitives: the popover engine and @@ -99,7 +99,7 @@ CQRS, a repository/DI framework, a router library, a generic CRUD-router factory | Upstream failure status | `422` with JSON detail | `502` (reserve 502 for raw byte-proxies) | | User-facing error | structured `detail` (`{code, reason}`) | a bare English string under a bilingual UI | | Mutation response | `{"ok": true, …}` (`{"deleted": id}`), test-held | a bare `{"unsubscribed"}`/`{"cleared": n}`, no `ok` | -| Transaction commit | the handler commits; a unit's `service` and every helper `flush` (fence-held, every module) | a helper that commits — only what owns its session does, each named with its reason: the fence's `UNIT_COMMITS` / `CORE_COMMITS`, a route's `COMMITTING_HELPERS` (`test_route_conventions`) | +| Transaction commit | the handler commits; a unit's `service` and every helper `flush` (fence-held, every module) | a helper that commits — only a listed one (what owns its session, or a core step that must land whatever its caller does next), each with its reason: the fence's `UNIT_COMMITS` / `CORE_COMMITS`, a route's `COMMITTING_HELPERS` (`test_route_conventions`) | | Admin knob storage | `sysconfig` (values) | env / `app_state` / a table of its own (the job intervals had one) | | Job status word | the `downloads/status.py` StrEnum | loose `"canceled"`/`"cancelled"` literals | | Current time | inline `datetime.now(timezone.utc)` | new `now_utc`/`_now` helpers | diff --git a/backend/app/README.md b/backend/app/README.md index 1a5c4591..66c583cc 100644 --- a/backend/app/README.md +++ b/backend/app/README.md @@ -62,10 +62,13 @@ The domain is four units, each owning its tables (`models.py` in its package): All of it is held by `tests/test_unit_fences.py`, with no exemptions: the unit map, each unit's packages and edges (`UNIT_PACKAGES`, `READS`) and what may commit (`UNIT_COMMITS`, `CORE_COMMITS`) -are listed there. It reads the code's AST: an inline owner filter in any form (an alias, `.in_()`, -`filter_by`), a model built or a statement write (`insert`/`update`/`delete` under any name) across -units, and a commit off the lists all fail it. An ORM attribute write on another unit's loaded row -is beyond it — review's to catch. +are listed there. It reads the code's AST, so it holds the forms written here: an owner column +compared inline (a model's, an import alias's or an `aliased()` one's, by `==`, `!=`, `.in_()`, or a +`filter_by(user_id=…)`), a model built or an `insert`/`update`/`delete` (under any import name, of a +model or its `__table__`) across units, and a `.commit()` call off the lists all fail it. What an AST +cannot tell is review's to catch: an ORM write on another unit's loaded row (an attribute, a +`db.delete`), SQL in `text()`, a column or model reached another way (bound to a variable, through +`__table__.c` or a module path), and a commit made by a `begin()` block or a `commit` handed in. ## Transaction ownership diff --git a/backend/tests/conftest.py b/backend/tests/conftest.py index c1654ee2..e5f8ca10 100644 --- a/backend/tests/conftest.py +++ b/backend/tests/conftest.py @@ -119,17 +119,17 @@ def _request_dep(db, unfinished: list[str]): and the suite stays green over a handler that forgot to commit (deep review, debt burn). So a request that ends normally with a write it did not commit — flushed, or still only staged — is recorded, and the test fails on it at teardown. Only the ROOT transaction's end clears it: a - savepoint released inside the handler commits nothing. The test's own staged rows are flushed before the request starts — they are the - test's business, not the handler's. A request that raises is rolled back, as production's + savepoint released inside the handler commits nothing. What the test itself staged or wrote is + committed before the request starts — it is the test's business, not the handler's, and a + request then begins with no write open, so any write left at its end is the handler's. A request that raises is rolled back, as production's `close()` rolls it back: without that, what a failing handler flushed but never committed stayed readable to the rest of the test — the one state production cannot be in (an issue created on the tracker before a failed upload was kept only by an explicit commit, and nothing saw it missing).""" def dep(conn: HTTPConnection): - db.flush() + db.commit() state = {"dirty": False} - before = _write_xact(db) def flushed(session, ctx): state["dirty"] = True @@ -145,10 +145,10 @@ def _request_dep(db, unfinished: list[str]): yield db staged = db.new or db.deleted or any(db.is_modified(o) for o in db.dirty) # Core DML (`db.execute(update(...))`) fires no flush and stages nothing; the database - # itself says whether a write is open — a transaction that wrote and was not committed - # holds an id it did not hold when the request began (see `release_read_connection`). A - # row lock (`FOR UPDATE`) takes one too: the handler ends what it locked. - core_dml = (after := _write_xact(db)) is not None and after != before + # itself says whether a write is open — the request began with none, and a transaction + # that wrote and was not committed holds an id (see `release_read_connection`). A row + # lock (`FOR UPDATE`) takes one too: the handler ends what it locked. + core_dml = _write_xact(db) is not None if state["dirty"] or staged or core_dml: unfinished.append(f"{conn.scope.get('method', 'WS')} {conn.url.path}") except Exception: diff --git a/backend/tests/test_entry_points.py b/backend/tests/test_entry_points.py index c622b30f..7310cd47 100644 --- a/backend/tests/test_entry_points.py +++ b/backend/tests/test_entry_points.py @@ -9,6 +9,7 @@ schema on its own. """ import ast +import json import subprocess import sys from pathlib import Path @@ -22,13 +23,16 @@ APP = BACKEND / "app" # Each `python -m` module the deployment starts, and the API's app module. ENTRY_POINTS = ("app.main", "app.worker", "app.downloads.runner") +# The table list goes out on one marked line: the API's logging writes to the same stdout, and a +# line it logs while importing is not the probe's answer. +_MARK = "entry-point-tables:" _PROBE = ( - "import importlib, sys\n" + "import importlib, json, sys\n" "importlib.import_module(sys.argv[1])\n" "from sqlalchemy.orm import configure_mappers\n" "from app.db import Base\n" "configure_mappers()\n" - "print('\\n'.join(sorted(Base.metadata.tables)))\n" + f"print({_MARK!r} + json.dumps(sorted(Base.metadata.tables)))\n" ) @@ -44,7 +48,9 @@ def test_each_entry_point_registers_the_whole_schema_on_its_own(): check=False, ) assert r.returncode == 0, (module, r.stderr[-2000:]) - assert r.stdout.split() == whole, module + answer = [ln for ln in r.stdout.splitlines() if ln.startswith(_MARK)] + assert len(answer) == 1, (module, r.stdout[-2000:]) + assert json.loads(answer[0].removeprefix(_MARK)) == whole, module def test_every_module_that_runs_as_a_process_is_listed(): diff --git a/backend/tests/test_override_dep_convention.py b/backend/tests/test_override_dep_convention.py index 41135240..a0b63032 100644 --- a/backend/tests/test_override_dep_convention.py +++ b/backend/tests/test_override_dep_convention.py @@ -87,7 +87,8 @@ def test_a_request_that_raises_leaves_nothing_it_did_not_commit(db): def test_the_commit_check_sees_a_core_dml_write(db, make_user): """`db.execute(update(...))` flushes nothing and stages nothing, so the session's own bookkeeping never sees it: a handler whose only write is Core DML and that forgot its commit passed. The - database's transaction id is what tells. A committed one must still pass.""" + database's transaction id is what tells — also when the test itself had a write open before the + request, which the handler's write would otherwise share. A committed one must still pass.""" import pytest from sqlalchemy import update from starlette.requests import HTTPConnection @@ -96,7 +97,9 @@ def test_the_commit_check_sees_a_core_dml_write(db, make_user): user = make_user() - def drive(commit: bool) -> None: + def drive(commit: bool, *, test_wrote: bool = False) -> None: + if test_wrote: # the test's own write, open and uncommitted, as the request starts + db.execute(update(User).where(User.id == user.id).values(display_name="test")) conn = HTTPConnection( {"type": "http", "method": "POST", "path": "/probe", "headers": [], "query_string": b""} ) @@ -113,3 +116,8 @@ def test_the_commit_check_sees_a_core_dml_write(db, make_user): drive(commit=False) assert db.info["unfinished"] == ["POST /probe"] db.info["unfinished"].clear() # recorded as it should be — not this test's failure + drive(commit=True, test_wrote=True) + assert db.info["unfinished"] == [] + drive(commit=False, test_wrote=True) + assert db.info["unfinished"] == ["POST /probe"] + db.info["unfinished"].clear() diff --git a/backend/tests/test_unit_fences.py b/backend/tests/test_unit_fences.py index 085b492d..9bce2f17 100644 --- a/backend/tests/test_unit_fences.py +++ b/backend/tests/test_unit_fences.py @@ -636,6 +636,27 @@ def test_every_unit_package_exists(): assert (APP / entry.split(":")[0]).exists(), entry +def test_every_listed_commit_is_still_there(): + """An entry names a commit that exists: one left behind by a rename, or by a function that no + longer commits, would go on allowing a commit to whatever takes its name next.""" + stale = [] + for commits in (*UNIT_COMMITS.values(), CORE_COMMITS): + for entry in commits: + rel, _, name = entry.partition(":") + tree = ast.parse((APP / rel).read_text(encoding="utf-8")) + functions = _enclosing_functions(tree) + committing = { + functions.get(node) + for node in ast.walk(tree) + if isinstance(node, ast.Call) + and isinstance(node.func, ast.Attribute) + and node.func.attr == "commit" + } + if (name not in committing) if name else not committing: + stale.append(entry) + assert stale == [] + + PACKAGE_CASES = { # (module path, source): violations ("downloads/x.py", "from app.models import Video"): 0, # capture reads catalog