fix(e3): address /code-review high findings (5)

Round-1 review fixes on the E3 branch:

- prune partial-fetch (correctness): `_paginate` now reports whether it reached
  the server's reported `total`; `_prune_stale` prunes only on a COMPLETE
  non-empty listing. A truncated fetch (a mid-sequence empty/short page before
  total — a transient Plex hiccup) no longer deletes the rows it merely failed
  to list. New test: a listing that claims 5 but returns 1 prunes nothing.
- `_prune_stale` (robustness): `stats.get("pruned", 0) + 1` — no KeyError when a
  caller's stats dict omits the key.
- service transitions (conventions): pause/resume/redownload use the JobStatus
  StrEnum instead of loose "queued"/"paused"/"error" literals, matching cancel.
- ffmpeg progress writer (correctness): a stuck-percent heartbeat now runs on its
  OWN 1/interval budget, so it can't consume the progress-write slot and delay a
  real percent change by up to an interval.
- backend/app/README.md (conventions): reword the transaction-ownership rule to
  quote and extend CLAUDE.md's canonical ("handler/job-wrapper commits; helpers
  flush; only quota owns its session") instead of stating a contradicting
  "helpers MAY commit" carve-out.

ruff clean, full DB-lane 364 pass.
This commit is contained in:
2026-08-18 22:47:25 +02:00
parent b02a9e37e8
commit bc79720e81
6 changed files with 96 additions and 39 deletions
+10 -8
View File
@@ -31,15 +31,17 @@ pre-abstract for a single caller; don't leave shared logic duplicated once there
## Transaction ownership
**A helper that mutates rows as part of a larger operation only `db.add()`s / stages its change; the
CALLER commits, so the whole operation is one transaction.** `audit.record()` is the reference: it adds
the audit row and the caller commits it together with the mutation it records, so an audit entry can
never exist without its change (or vice-versa).
This follows CLAUDE.md's canonical rule verbatim — **"the handler/job-wrapper commits; helpers flush
(only `quota` owns its session)"** — so read it that way, not as an exception to it. The commit belongs
to the **operation owner**; anything called *inside* another transaction only stages its change
(`db.add` / `flush`) and lets the owner commit, so the whole operation is one transaction.
`audit.record()` is the reference: it adds the audit row and the caller commits it together with the
mutation it records, so an audit entry can never exist without its change (or vice-versa).
- A self-contained transition that owns its whole unit of work MAY commit (e.g. `service.cancel/…`
each commit their one transition; the worker's short-lived `jobdb` helpers each commit their own
write). But a helper called *in the middle* of a caller's transaction must not commit — that splits
the caller's atomic operation in two.
- The **operation owner** is usually the route handler. A `service.*` operation the handler delegates a
whole transition to (e.g. `service.cancel`) *is* that owner and commits its one transition — it isn't
a mid-operation helper. A helper that owns its OWN short-lived session (the worker's `jobdb` writes,
the same carve-out CLAUDE.md grants `quota`) commits that session. Everything else flushes.
- `SessionLocal` is `expire_on_commit=False`: after a raw `UPDATE` + commit, an ORM object you already
loaded is **stale**. Read the fresh value from the row (RETURNING, or a re-`SELECT`), not the ORM
attribute — see `service.decr_ref_count`.