- Every playlist item write (add, remove, move, reorder, rename; Plex remove and remove-bulk) row-locks
the playlist before it touches an item, a move both lists in id order: remove beside reorder was a
deadlock, and two adds of one video a unique-key 500.
- The scheduled mirror refresh re-reads dirty under that lock, so an edit made during its YouTube calls
is not replaced by YouTube's order.
- A user's bulk pull holds a per-user advisory claim: two at once created duplicate mirrors. The manual
sync answers 409 sync_all_busy instead of '0 synced', and like the re-pull spends its cooldown only
on a pull that runs.
- Deleted mirrors are committed at once, not only when a later list commits.
- The claims open an unpooled connection of their own instead of a third pooled one per push.
Deep-review round 3, targeted at the surfaces round 2's fix commit changed.
Five of the eight findings are the same shape: the guards written to stop
half-done sweeps were themselves half-done.
test_upload.py's display-field guard walked only `ast.Assign`, so the one
writer with an unbounded REQUEST HEADER behind it — `spool_upload`'s
`display_name=display_text(...)` keyword — was invisible to the guard meant
to protect it. It also stopped at `app/routes/`, missing the four writers in
`downloads/` that actually construct the row, and asserted `not offenders`,
which enforces nothing once a refactor moves the writers out of its view.
It now walks assignments AND keywords across `app/`, and holds exact set
equality against a registry of all twelve write sites with each one's value.
test_body_limit.py's release guard used `any(... ast.walk(fn))`, which
accepts a `release_read_connection` that runs AFTER the await it exists to
precede — satisfying the letter of the rule while pinning a connection for
exactly the transfer it was supposed to free. Position is checked now, and a
late release is its own, louder failure. The registry keyed sites by file
basename while scanning recursively, and `admin.py`, `discover.py` and
`playlists.py` each exist both at the top level and under `plex/` — the same
entry-collapse the registry was widened to close. Keyed by relative path now.
And all three guards recognised a body read only by the `capped_` naming
convention, so a route spelling `await request.body()` inline declared no
ceiling, released no connection, and was invisible to every one of them.
`release_read_connection`'s precondition claimed the caller "must have
NOTHING PENDING, and that is checked rather than described" while checking
only UNFLUSHED ORM work. Measured: after `add`+`flush` all three collections
are empty and the rollback loses the row, and Core DML never enters them at
all — both of them this codebase's canonical write shapes. It now also asks
the transaction directly via `pg_current_xact_id_if_assigned()`.
`errors.tooLarge` was reworded to "try a smaller file", but that key answers
every route's body cap: the issue composer's over-size JSON draft got advice
about a file it does not have. Back to payload-neutral; the file-specific
sentence keeps its own key.
The upload dialog's new danger border outranked `dragOver`, so a user
replacing an over-cap file got no sign the drop zone was live — and with a
file picked the label shows its name rather than "drop it here", so the
border was the only feedback left.
Four further findings were refuted 3/3 and are not fixed: `aria-invalid` on
the drop zone (measured in Chromium — the AX node does carry it on a native
button), a bidi-override spoof through `display_text` (the served filename
already strips those categories, and the proposed filter would have broken
every ZWJ emoji), and `capped_body` not draining (the screenshot route's edge
ceiling equals its own cap, so that refusal is unreachable, and the log sink
is sent by beacon with nobody reading the answer).
Deep-review round 2, on round 1's own fix commit. It found MORE than round 1
did (28 raw, against 13), and most of it was mine -- a new piece of UI attracts
findings a backend-only diff never does. The pattern in almost all of them is
the same: a principle applied to the site that prompted it and not to its
siblings.
* `display_text` reached three of the FIVE writers of a user-supplied name.
`POST /api/downloads` and `POST /api/downloads/edit` still handed Postgres an
unbounded string, so a 300-character name pasted into the Add-download dialog
or the editor was a bare 500 (main.py re-raises every DataError but the NUL
case). Four of the seven reviewers found it independently. The bound now lives
in `models.py`, the three columns are DECLARED with it, and the guard for the
next writer is parsed rather than remembered: every `display_name` /
`display_uploader` assignment under `app/routes/` must go through the helper.
That guard immediately found two more -- the Google sign-in callback writes
the profile name into the same 255-wide column -- so it is six, not five.
* The connection release reached one of the THREE routes that read their own
body. The screenshot route (10 MiB, from a phone) and the client log sink both
still held a pooled connection idle-in-transaction for the length of the
upload, which is the invariant db.py sizes the pool on. All three now call
`db.release_read_connection`, whose precondition -- nothing pending on the
shared session -- is CHECKED rather than described, because a whole-session
rollback discarding a caller's staged write is a mistake this repo has already
made once. `test_body_limit.py` holds the rule for the next such route.
* The AST drift registry keyed its sites by (module, limit expression), so a
second route copied from the first collapsed onto the same entry and went
uncovered -- the exact hole the registry exists to close. Keyed per call site
now, and it carries the release requirement too.
* The dialog's own refusal was announced once and never again: two over-cap
picks leave an identical node with identical text, and an untouched live
region says nothing. It is keyed per pick now, the drop zone reads as invalid
and points at the sentence, and the reason is reachable from the control
rather than only from a one-shot announcement.
* Two tests rebuilt the blind spot they were written to remove: the dialog's cap
test sat nowhere near the boundary (a `>` loosened to `>=` survived it, which
is the mutation round 1 fixed on the server side), and the header-precedence
test only covered an exact-case duplicate while a differently-cased one would
have been COMBINED onto the wire rather than replaced. Both fixed, and the
merge now dedupes case-insensitively because header names are.
Also: the upload dialog's unstubbed usage query was firing a real request from
jsdom in a neighbouring suite and leaking a delayed "connection lost" toast
across test boundaries; `GET /api/downloads/usage` had no test at all though the
whole client-side check hangs off it; and the comment claiming the edge 413 is
readable now says what was measured -- readable to 200 MiB over loopback, a bare
`onerror` through a proxy -- so the pre-send check is named as the mechanism and
that branch as the backstop it is.
Every test added or changed here was mutation-checked. One mutation SURVIVED and
is recorded rather than papered over: the `tooBig` term inside `submit()` cannot
be reached, because `ui/Button` swallows the activation itself, so it is marked
as belt and braces instead of being described as the guard.
- A failure keeps its traceback unless it is yt-dlp's or our own StatedFailure (a
message that is the whole story). A plain RuntimeError, RecursionError or OSError
is also what a bug of ours raises, and its stack is the only lead.
- The stack is scrubbed link by link (logs.scrubbed_stack): a chained DownloadError
under a DB error no longer prints yt-dlp's text unscrubbed.
- Engine hide_parameters: a DB error's message no longer carries the title or path
it was writing.
- The download and edit failure lines go out even when their DB write fails.
- The worker's child_exit/crash/supervisor lines say download.job.failed only when
the job failed there; a crash the DB will not record no longer ends the thread.
Stopping a paused child is download.child.stopped, not a second abort.
- log_safe also covers generic ids with spaces, playlist titles and bare ids.
- The ffmpeg image steps log the failure kind, not the argv with its path.
- The consent contract accepts only the body of a plain consented_trace() check,
and any level that may be TRACE.
- Tests for each; every one mutation-checked.
This codebase leans on "why" comments as its architecture doc, so a header that
claims the opposite of the code below it is worse than none. Fix the seven the
review flagged: channels.py (subscribe/unsubscribe shipped), playlists.py +
sync/playlists.py (YouTube two-way sync + write-back shipped), setup.py (drop the
"epic 6c" ref — the wizard steps exist), config.py ("auth._client_ip" ->
"routes.auth._client_ip"), db.py ("routes/plex.py" -> "routes/plex/" after the
package split), sysconfig.py (the "Phase C" note — the watch-sync job that reads
plex_watch_sync_interval_min has landed; clarified it's a boot default, not a knob).
Docs only.
A collections page fires 100+ concurrent poster requests. Both image proxies
(/image and /person-image) authenticated via current_user + Depends(get_db),
holding a pooled DB connection for the whole request — including the slow Plex/
CDN fetch. Under the burst, the 15-connection pool was exhausted → QueuePool
checkout timeouts → 502s → slow, partially-loaded grids on prod.
Fix: authenticate these two high-fan-out endpoints with a signed-session check
(no DB user-load), serve disk-cache hits with zero DB access, and on a cold miss
open a short session only to resolve the image key + Plex config, releasing it
BEFORE the fetch (image_bytes uses no DB). Also raise the pool (20 + 30 overflow)
as headroom above the sync-endpoint threadpool.
- docker-compose with Postgres 16 + slim Python API image
- FastAPI app with session middleware, health endpoint, static login page
- Google OAuth (Authlib) with email invite-list whitelist; admin role support
- User + OAuthToken models; refresh tokens encrypted at rest (Fernet)
- Alembic migrations, run automatically on container startup
- Postgres backup/restore scripts for portability between machines
peter