fix(api): a token hiccup is not "reconnect"; reorders and approvals are one unit each

Deep review of the debt burn:
- `refusal_reason` told the user to reconnect on ANY token-endpoint failure — a 5xx, a 429, a
  misconfigured client — while the grant stayed and the account still read as connected, with no
  connect button. Only `invalid_grant` (the one that drops the grant) means reconnect now.
- `pins.reorder` writes only the rows that change, so two overlapping reorders each skipped rows
  that matched in their own stale snapshot and interleaved into two pins on one slot. It locks the
  user's pins first; the second waits and deals from the first's result.
- Adding a whitelist entry committed the invite and the activation it implies separately, and its
  audit row was written before a new invite had an id (target NULL). One transaction, flushed for
  the id.
- The mutation convention test skipped async routes and dicts returned by name: request-access,
  purge-discovery and the subscription sync answered without `ok`. They do now, and the test
  covers both. The helper-commit check looked only at `_`-names; it reads every non-route
  function, which listed two deliberate committers (upsert_pending_invite, ensure_welcome) — now
  allowlisted with their reasons.
This commit is contained in:
2026-09-19 19:19:29 +02:00
parent 99df44d7fb
commit 955c8c3445
9 changed files with 130 additions and 33 deletions
+12
View File
@@ -168,3 +168,15 @@ def test_deleting_the_user_also_clears_the_way_to_re_invite(db, make_user, as_us
assert (
db.execute(select(Invite).where(Invite.email == member.email)).scalar_one_or_none() is None
)
def test_whitelisting_a_new_address_audits_it_by_id(db, make_user, as_user):
"""The audit row was written before the new invite had an id, so it named no target."""
from app.models import AuditLog
admin = make_user(role="admin")
r = as_user(admin).post("/api/admin/invites", json={"email": "fresh-e27@example.com"})
assert r.status_code == 200
inv = db.scalar(select(Invite).where(Invite.email == "fresh-e27@example.com"))
row = db.scalar(select(AuditLog).where(AuditLog.target_type == "invite"))
assert row is not None and row.target_id == str(inv.id)
+46
View File
@@ -1069,3 +1069,49 @@ def test_a_hero_from_a_show_pin_names_the_pin_it_came_from(db, as_user):
"id": "ps1",
"title": "A Show",
}
def test_two_overlapping_reorders_never_share_a_slot(db, as_user):
"""`reorder` writes only the rows that change. Unlocked, two tabs reordering from the same
snapshot each skipped the rows that already matched in THEIR snapshot and interleaved row by
row: p1=1, p2=2, p3=1 — two pins on one slot (deep review, debt burn). Locked, the second waits
for the first and deals from its result."""
import threading
from app.db import SessionLocal
user, ch = _seed_yt(db)
c = as_user(user)
p1 = c.post("/api/discover/pins", json={"kind": "video", "ref": "vmed"}).json()["id"]
p2 = c.post("/api/discover/pins", json={"kind": "channel", "ref": ch.id}).json()["id"]
p3 = c.post("/api/discover/pins", json={"kind": "video", "ref": "vlong"}).json()["id"]
uid = user.id
with SessionLocal() as tab_a, SessionLocal() as tab_b, SessionLocal() as probe:
pins.reorder(tab_a, tab_a.get(User, uid), [p2, p1, p3]) # written, not yet committed
errors: list[BaseException] = []
def second_tab():
try:
pins.reorder(tab_b, tab_b.get(User, uid), [p1, p3, p2])
tab_b.commit()
except BaseException as exc: # noqa: BLE001 — re-raised in the test thread below
errors.append(exc)
worker = threading.Thread(target=second_tab)
worker.start()
# Commit A only once B is blocked on a row lock, so B's read is the stale snapshot.
for _ in range(100):
waiting = probe.scalar(text("SELECT count(*) FROM pg_locks WHERE NOT granted"))
probe.rollback()
if waiting:
break
worker.join(0.05)
tab_a.commit()
worker.join(10)
assert not worker.is_alive() and errors == []
db.expire_all()
positions = db.scalars(select(ComfortPin.position).where(ComfortPin.user_id == uid)).all()
assert sorted(positions) == [0, 1, 2]
assert [p["pin_id"] for p in c.get("/api/discover/pins").json()["items"]] == [p1, p3, p2]
+33 -10
View File
@@ -23,6 +23,11 @@ COMMITTING_HELPERS = {
"search.py:_ingest_candidates": "stages around YouTube calls: the stubs must survive an enrich "
"that fails",
"search.py:_classify_shorts": "the last stage of that ingest, after its probes",
"auth.py:upsert_pending_invite": "commits only when it writes — the contract its three "
"callers rely on (`_register_account` persists its own flushed user because a no-op here "
"commits nothing)",
"messages.py:ensure_welcome": "the websocket push after it must announce a row the client "
"can already fetch",
}
@@ -31,7 +36,17 @@ def _modules():
yield path.relative_to(ROUTES).as_posix(), ast.parse(path.read_text(encoding="utf-8"))
def _is_mutation_route(fn: ast.FunctionDef) -> bool:
FUNCTIONS = (ast.FunctionDef, ast.AsyncFunctionDef)
def _is_route(fn: ast.FunctionDef | ast.AsyncFunctionDef) -> bool:
return any(
isinstance(d, ast.Call) and getattr(d.func, "attr", "") in MUTATING | {"get", "websocket"}
for d in fn.decorator_list
)
def _is_mutation_route(fn: ast.FunctionDef | ast.AsyncFunctionDef) -> bool:
return any(
isinstance(d, ast.Call) and getattr(d.func, "attr", "") in MUTATING
for d in fn.decorator_list
@@ -39,27 +54,35 @@ def _is_mutation_route(fn: ast.FunctionDef) -> bool:
def test_every_mutation_answers_ok():
"""`{"ok": true, …}` (or `{"deleted": id}`) for every mutation that answers with a literal —
one field a client can read the same way everywhere. A serializer's return is exempt: it
describes a resource, not an acknowledgement."""
"""`{"ok": true, …}` (or `{"deleted": id}`) for every mutation, sync or async — one field a
client can read the same way everywhere. A serializer's return (a call) is exempt: it describes
a resource, not an acknowledgement. A dict built elsewhere and returned by name is not: it hid
two acknowledgements without `ok` from the literal-only scan, so it is spread into a literal."""
missing = []
for name, tree in _modules():
for fn in ast.walk(tree):
if not (isinstance(fn, ast.FunctionDef) and _is_mutation_route(fn)):
if not (isinstance(fn, FUNCTIONS) and _is_mutation_route(fn)):
continue
for node in ast.walk(fn):
if isinstance(node, ast.Return) and isinstance(node.value, ast.Dict):
keys = {k.value for k in node.value.keys if isinstance(k, ast.Constant)}
if not keys & {"ok", "deleted"}:
missing.append(f"{name}:{node.lineno} {fn.name}")
if not isinstance(node, ast.Return) or isinstance(node.value, ast.Call):
continue
keys = (
{k.value for k in node.value.keys if isinstance(k, ast.Constant)}
if isinstance(node.value, ast.Dict)
else set()
)
if not keys & {"ok", "deleted"}:
missing.append(f"{name}:{node.lineno} {fn.name}")
assert missing == []
def test_route_helpers_flush_and_leave_the_commit_to_the_handler():
"""Every function in a route module that is not itself a route is a helper — whatever its name
(two public ones committed unseen while this looked only at `_`-names) and sync or async."""
committing = set()
for name, tree in _modules():
for fn in ast.walk(tree):
if isinstance(fn, ast.FunctionDef) and fn.name.startswith("_"):
if isinstance(fn, FUNCTIONS) and not _is_route(fn):
if any(
isinstance(n, ast.Call) and getattr(n.func, "attr", "") == "commit"
for n in ast.walk(fn)
+9 -2
View File
@@ -19,6 +19,11 @@ from app.youtube.client import YouTubeError, refusal_reason
(YouTubeError("m", reason="quotaExceeded", path="playlists"), "quota"),
(YouTubeError("m", reason="backendError,dailyLimitExceeded", path="videos"), "quota"),
(YouTubeError("m", reason="invalid_grant", path="oauth2/token"), "reconnect"),
# The token endpoint failing any other way leaves the grant alone — and a reconnect would
# not help: Google's bad minute, a rate limit, a misconfigured client.
(YouTubeError("m", reason="backend_error", path="oauth2/token"), "unavailable"),
(YouTubeError("m", reason="<unparseable>", path="oauth2/token"), "unavailable"),
(YouTubeError("m", reason="invalid_client", path="oauth2/token"), "unavailable"),
(YouTubeError("No refresh token; user must re-authenticate"), "reconnect"),
(YouTubeError("m", reason="transport"), "unavailable"),
(YouTubeError("m", reason="backendError", path="subscriptions"), "unavailable"),
@@ -88,7 +93,8 @@ def test_a_revoked_grant_is_forgotten_so_the_account_can_reconnect(db, make_user
def test_any_other_token_refusal_keeps_the_grant(monkeypatch):
"""Only `invalid_grant` is final: a 5xx from the token endpoint is Google having a bad minute."""
"""Only `invalid_grant` is final: a 5xx from the token endpoint is Google having a bad minute —
and the user is told to try again, not to reconnect an account that still reads as connected."""
resp = httpx.Response(
503, json={"error": "backend_error"}, request=httpx.Request("POST", yt.GOOGLE_TOKEN_URL)
)
@@ -101,6 +107,7 @@ def test_any_other_token_refusal_keeps_the_grant(monkeypatch):
monkeypatch.setattr(yt.sysconfig, "get_str", lambda d, key: "")
monkeypatch.setattr(c, "_send", lambda *a, **k: resp)
with pytest.raises(YouTubeError):
with pytest.raises(YouTubeError) as caught:
c._access_token()
assert tok.scopes == "s"
assert refusal_reason(caught.value) == "unavailable"