diff --git a/backend/app/discover/pins.py b/backend/app/discover/pins.py index c3c88a35..78f50e01 100644 --- a/backend/app/discover/pins.py +++ b/backend/app/discover/pins.py @@ -84,7 +84,10 @@ def reorder(db: Session, user: User, ids: list[int]) -> None: ones jumped about when they came back. So the pins left out keep their slots, and the listed ones are dealt into the slots the listed set occupies. Ids that are not this user's, or repeated, are ignored rather than failing a stale replay wholesale. Flushes; the route commits.""" - current = list_pins(db, user) + # Locked: the rows it leaves alone are part of the answer too. Two overlapping reorders each + # skipped the rows that already matched in their own snapshot and interleaved into two pins on + # one slot; the second now waits and deals from the first one's result. + current = list_pins(db, user, lock=True) owned = {p.id for p in current} listed: list[int] = [] for i in ids: @@ -100,16 +103,13 @@ def reorder(db: Session, user: User, ids: list[int]) -> None: db.flush() -def list_pins(db: Session, user: User) -> list[ComfortPin]: - return list( - db.execute( - select(ComfortPin) - .where(ComfortPin.user_id == user.id) - .order_by(ComfortPin.position.asc(), ComfortPin.id.asc()) - ) - .scalars() - .all() +def list_pins(db: Session, user: User, *, lock: bool = False) -> list[ComfortPin]: + stmt = ( + select(ComfortPin) + .where(ComfortPin.user_id == user.id) + .order_by(ComfortPin.position.asc(), ComfortPin.id.asc()) ) + return list(db.execute(stmt.with_for_update() if lock else stmt).scalars().all()) def next_position(db: Session, user: User) -> int: diff --git a/backend/app/routes/admin.py b/backend/app/routes/admin.py index 43fad55d..b58ff724 100644 --- a/backend/app/routes/admin.py +++ b/backend/app/routes/admin.py @@ -264,10 +264,11 @@ def add_invite( inv.status = "approved" inv.decided_at = datetime.now(timezone.utc) inv.decided_by = admin.email + db.flush() # a new invite's id, for the audit row — before it, the row named no target audit.record( db, admin.id, AuditAction.INVITE_ADD, target_type="invite", target_id=inv.id, summary=email ) - db.commit() + # One transaction: the whitelisting and the activation it implies land together or not at all. _activate_user_by_email(db, email) db.commit() return _serialize(inv, has_account=bool(_accounts_for(db, [email]))) @@ -855,4 +856,4 @@ def purge_discovery(admin: User = Depends(admin_user), db: Session = Depends(get after=result, ) db.commit() - return result + return {"ok": True, **result} diff --git a/backend/app/routes/auth.py b/backend/app/routes/auth.py index c3bbd32f..405acb02 100644 --- a/backend/app/routes/auth.py +++ b/backend/app/routes/auth.py @@ -656,13 +656,13 @@ async def request_access( if not valid_email(email): raise refused(400, "invalid_email") if not _request_access_limiter.allow(client_key(request)): - return {"status": "pending"} + return {"ok": True, "status": "pending"} if is_allowed(db, email): - return {"status": "pending"} # already allowed — don't reveal it + return {"ok": True, "status": "pending"} # already allowed — don't reveal it req = upsert_pending_invite(db, email, body.lang) if req.is_new: notify_admins_of_request(db, background, email) - return {"status": "pending"} + return {"ok": True, "status": "pending"} @router.post("/demo") diff --git a/backend/app/routes/sync.py b/backend/app/routes/sync.py index baed9463..5617f0e1 100644 --- a/backend/app/routes/sync.py +++ b/backend/app/routes/sync.py @@ -42,7 +42,7 @@ def sync_subscriptions(user: User = Depends(require_human), db: Session = Depend raise youtube_refusal(exc) result["quota_used_estimate"] = m.used result["quota_remaining_today"] = m.remaining - return result + return {"ok": True, **result} @router.get("/status") diff --git a/backend/app/youtube/client.py b/backend/app/youtube/client.py index 0fa9679b..3439ca5b 100644 --- a/backend/app/youtube/client.py +++ b/backend/app/youtube/client.py @@ -109,18 +109,26 @@ def is_quota_refusal(exc: YouTubeError) -> bool: return any(r in _QUOTA_REASONS for r in exc.reason.split(",")) +# The token endpoint's answer for a revoked or expired grant — the one refusal a reconnect fixes. +_DEAD_GRANT = "invalid_grant" + # What a refused YouTube action means to the person who asked for it — the three things they can # act on differently: wait for tomorrow's budget, reconnect the account, or try again later. YouTubeRefusal = Literal["quota", "reconnect", "unavailable"] def refusal_reason(exc: YouTubeError) -> YouTubeRefusal: - """Sort a failure by what the user can do about it. The token step failing (Google refused the - refresh, or there is no usable token at all — raised before any endpoint answered, so with no - path and no transport reason) means the account must be reconnected; retrying cannot help.""" + """Sort a failure by what the user can do about it. A dead grant (Google refused the refresh + with `invalid_grant`, which also drops the grant — see `_drop_dead_grant`) or no usable token at + all (raised before any endpoint answered, so with no path and no transport reason) means the + account must be reconnected; retrying cannot help. Any other token-endpoint failure — a 5xx, a + 429, a misconfigured client — leaves the grant as it was: reconnecting would not fix it, and + the account would still read as connected, with no connect button to press.""" if is_quota_refusal(exc): return "quota" - if exc.path == "oauth2/token" or (not exc.path and exc.reason != "transport"): + if exc.path == "oauth2/token": + return "reconnect" if exc.reason == _DEAD_GRANT else "unavailable" + if not exc.path and exc.reason != "transport": return "reconnect" return "unavailable" @@ -199,7 +207,7 @@ class YouTubeClient: ) if resp.status_code != 200: exc = _upstream_failure("POST", "oauth2/token", resp) - if exc.reason == "invalid_grant": + if exc.reason == _DEAD_GRANT: self._drop_dead_grant(tok) raise exc data = resp.json() diff --git a/backend/tests/test_admin_invites.py b/backend/tests/test_admin_invites.py index 2786aa4e..4c2fd053 100644 --- a/backend/tests/test_admin_invites.py +++ b/backend/tests/test_admin_invites.py @@ -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) diff --git a/backend/tests/test_discover.py b/backend/tests/test_discover.py index c8c3bc47..e1fc5872 100644 --- a/backend/tests/test_discover.py +++ b/backend/tests/test_discover.py @@ -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] diff --git a/backend/tests/test_route_conventions.py b/backend/tests/test_route_conventions.py index b0b6b988..42aa81a9 100644 --- a/backend/tests/test_route_conventions.py +++ b/backend/tests/test_route_conventions.py @@ -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) diff --git a/backend/tests/test_youtube_refusals.py b/backend/tests/test_youtube_refusals.py index 724ef8e3..a87cfb78 100644 --- a/backend/tests/test_youtube_refusals.py +++ b/backend/tests/test_youtube_refusals.py @@ -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="", 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"