test(plex)+cleanup(quota): S2 code-review fixes (R6 S2)
/code-review high on S2 — 4 findings, 2 fixed, 2 no-change: - test_plex_sync_txn: a faked PlexClient fails on the second library; asserts the first library's row survives (per-section commit). Mutation-checked — moving the commit back out of the loop turns it red ([] == ['1']). - dropped the `[[quota-owns-its-session]]` memory slugs from quota.py docstrings (a dangling private-note reference in shipped code; the reasoning is already inline). No change: record_usage's per-call second connection is a transient spike, fine for this app's concurrency; the two-site `with SessionLocal()` duplication isn't worth a helper until a third caller appears. Gate: ruff clean; DB lane 181 green.
This commit is contained in:
@@ -126,7 +126,7 @@ def log_action(user_id: int, action: str) -> None:
|
|||||||
|
|
||||||
Writes in its OWN session (like record_usage) — quota accounting is durable independent of the
|
Writes in its OWN session (like record_usage) — quota accounting is durable independent of the
|
||||||
caller's transaction and must never commit the caller's session (that would flush a job's
|
caller's transaction and must never commit the caller's session (that would flush a job's
|
||||||
partial state mid-run, defeating its own rollback). See [[quota-owns-its-session]]."""
|
partial state mid-run, defeating its own rollback)."""
|
||||||
with SessionLocal() as qs:
|
with SessionLocal() as qs:
|
||||||
qs.add(QuotaEvent(user_id=user_id, action=action, units=0))
|
qs.add(QuotaEvent(user_id=user_id, action=action, units=0))
|
||||||
qs.commit()
|
qs.commit()
|
||||||
@@ -158,7 +158,7 @@ def record_usage(units: int) -> None:
|
|||||||
back (or is read-only and never commits) must NOT lose the record — under-counting the shared
|
back (or is read-only and never commits) must NOT lose the record — under-counting the shared
|
||||||
daily budget risks overspending the real API quota. Equally, quota must never commit the
|
daily budget risks overspending the real API quota. Equally, quota must never commit the
|
||||||
CALLER's session (the old behaviour) — that persisted a job's partial state mid-run behind its
|
CALLER's session (the old behaviour) — that persisted a job's partial state mid-run behind its
|
||||||
own `except: db.rollback()`. See [[quota-owns-its-session]]."""
|
own `except: db.rollback()`."""
|
||||||
if units <= 0:
|
if units <= 0:
|
||||||
return
|
return
|
||||||
day = pacific_today()
|
day = pacific_today()
|
||||||
|
|||||||
@@ -0,0 +1,44 @@
|
|||||||
|
"""DB-backed test for R6 S2's Plex per-section commit: a PlexError on a LATER library must keep the
|
||||||
|
libraries already mirrored (committed per section), not roll the whole reconcile back. Uses the DB
|
||||||
|
lane (`db` fixture) with a faked PlexClient — skipped without Postgres.
|
||||||
|
"""
|
||||||
|
from sqlalchemy import select
|
||||||
|
|
||||||
|
from app.plex import sync
|
||||||
|
from app.plex.client import PlexError
|
||||||
|
from app.models import PlexLibrary
|
||||||
|
|
||||||
|
|
||||||
|
class _FakePlex:
|
||||||
|
def __enter__(self):
|
||||||
|
return self
|
||||||
|
|
||||||
|
def __exit__(self, *exc):
|
||||||
|
return False # don't suppress the PlexError
|
||||||
|
|
||||||
|
def sections(self):
|
||||||
|
return [
|
||||||
|
{"type": "movie", "key": "1", "title": "Lib One"},
|
||||||
|
{"type": "movie", "key": "2", "title": "Lib Two"},
|
||||||
|
]
|
||||||
|
|
||||||
|
|
||||||
|
def test_late_section_failure_keeps_earlier_sections(db, monkeypatch):
|
||||||
|
monkeypatch.setattr(sync.sysconfig, "get_bool", lambda *a, **k: True) # plex_enabled
|
||||||
|
monkeypatch.setattr(sync, "_enabled_section_keys", lambda db: None) # all sections wanted
|
||||||
|
monkeypatch.setattr(sync, "PlexClient", lambda db: _FakePlex())
|
||||||
|
monkeypatch.setattr(sync, "_sync_movies", lambda *a, **k: None) # skip the real Plex fetch
|
||||||
|
|
||||||
|
# Section 1 completes and commits; section 2's collections step raises → its work rolls back.
|
||||||
|
def collections(db, plex, lib, stats):
|
||||||
|
if lib.plex_key == "2":
|
||||||
|
raise PlexError("boom on the second library")
|
||||||
|
|
||||||
|
monkeypatch.setattr(sync, "_sync_collections", collections)
|
||||||
|
|
||||||
|
result = sync.sync(db)
|
||||||
|
|
||||||
|
assert "error" in result # the run reports the failure honestly
|
||||||
|
# Section 1's library survived the section-2 failure (per-section commit); section 2 rolled back.
|
||||||
|
keys = db.scalars(select(PlexLibrary.plex_key).order_by(PlexLibrary.plex_key)).all()
|
||||||
|
assert keys == ["1"]
|
||||||
Reference in New Issue
Block a user