- Correctness (stale HU): _sync_overlay is now a full reconcile — it clears every guid-bearing EN
row's *_hu first, then repopulates the matches, so a title whose HU twin vanished or was re-guid'd
falls back to EN instead of keeping stale Hungarian metadata (only rows that held HU data go dirty).
- Correctness (dup guid): by_guid maps a guid to the LIST of EN rows carrying it, so two editions of
one film both receive the overlay instead of just the last-iterated one.
- Robustness (guid wipe): _apply_item/_sync_shows keep a prior guid when a listing carries none
(`_canonical_guid(meta) or row.guid`), so a server ignoring includeGuids can't erase the match key.
- Reuse: merged _sync_overlay's movie/show branches (only the select + item_type differ).
- Test-coverage: a full-sync test drives sync() over a stub Plex (sections + section_items +
overlay_map) asserting pass-2 designation (metadata_lang/overlay_of), guid capture, the HU write,
and browse exclusion; plus a reconcile test that a vanished twin clears stale *_hu.
Full backend suite 274 passed / 2 skipped.
Adds the storage + sync for the Hungarian-metadata overlay (the browsable EN sections gain HU
twin fields written from a paired HU section, matched by guid; no browsable HU rows).
Schema (2 migrations, ADD-only; run before the first HU sync while *_hu is NULL so the generated
column build is trivial):
- 0063: plex_libraries.metadata_lang ('en' default) + self-FK overlay_of.
- 0064: plex_items/plex_shows.guid (indexed) + title_hu/summary_hu/genres_hu (+ GIN) + a STORED
generated search_vector_hu mirroring the EN vector (title_hu A / people_text B / summary_hu C) + GIN.
Sync:
- includeGuids=1 on section_items so the canonical guid rides the cheap listing.
- _canonical_guid(meta) (tmdb > imdb > tvdb) captured onto every EN row in _apply_item/_sync_shows.
- Two-pass sync(): pass 1 mirrors browsable (non-overlay) sections; pass 2 reads plex_overlay_map
('huKey:enKey', new sysconfig), designates each HU section (metadata_lang='hu' + overlay_of), and
_sync_overlay writes title_hu/summary_hu/genres_hu onto the guid-matched EN rows.
Browse: _content_libs now excludes overlay libraries (overlay_of is None); the unified_library and
facets inline library partitions fold into it, and list_libraries/collections exclude overlays too.
Tests: DB-lane coverage of the overlay write (matched vs unmatched rows, no browsable HU rows, the
generated search_vector_hu populates), the guid parse, and the browse exclusion. Full suite 272
passed; migrations verified reversible (upgrade→downgrade→upgrade). S5b (persist the live detail
fields + serve item/show detail from DB) is the follow-up.
Backend: new GET /api/plex/person/{name} returns a cast/crew member's cached headshot plus
the movies (PlexItem, kind=movie) and shows (PlexShow) in the enabled libraries whose
cast_names OR directors include the name — the inverse of the cast → filter link. Introduce a
shared _content_libs(db) helper (enabled-library partition) and reuse it in people_cards;
S5 will extend it with the HU-overlay exclusion. Watch-state comes from PlexState / rollup
(reusing _movie_card / _show_card / state_map). DB-lane tests cover cast+director credits,
library scoping, the kind filter, unknown-name empties, and the partition helper.
Frontend: a new `person` history-subview. A cast-row click (PlexInfo cast row + the show
page's CastStrip) now opens the person's filmography page instead of filtering; the filter
gesture moves to a "Filter library by {name}" button on the page itself. PlexPersonView
mirrors PlexInfoView's fetch-wrapper + glass hero (headshot + name + credit counts) and
renders Movies / Shows as PlexPosterCard grids, with an optimistic per-card watch toggle on
the person cache. New plexPerson query key + api client + PlexPersonDetail type; HU/EN copy.
Add a search scope-pill to the Plex header search box (GitHub-style, one
field-scope at a time) and an instant typeahead of the top matches below it.
Backend (unified_library):
- New `field` param: all | title | people | summary | episode. `all` is
byte-identical to the pre-scope behaviour (untouched weighted search_vector);
a specific field keeps the GIN `search_vector @@ tsq` prefilter AND ANDs a
per-column tsvector recheck, so scoped hits are a strict subset of Everywhere.
Per-field expressions mirror the search_vector Computed def (same TS config,
same left(summary,1000) truncation).
- `episode` scope empties the movie/show grid and title-only-matches the grouped
Episodes section; the title/people/summary scopes suppress that section.
- Unknown field falls back to `all`. New DB-lane characterization tests
(test_plex_field_scoped_search.py): each scope narrows correctly, field=all is
identical to today, episode empties the grid and needs shows in scope.
Frontend:
- PlexSearchBox wraps the shared SearchBar with the scope-pill (reusing
ActionMenu) and the typeahead; SearchBar gains generic leading/below/focus/
keydown slots so other modules' search boxes are untouched.
- Episode scope offered only when shows are in scope (movie-only hides it),
healed at the point of use like the sort state.
- Typeahead reuses /api/plex/library (limit=10), honours scope+field (not the
sidebar filters), shows poster+title+year, keyboard nav (arrows/Enter/Esc),
and jumps straight to a movie's info page or a show's drill-down via a new
provider jumpTo hand-off (mirrors the playlistOpen pattern).
- `field` persisted per-account; en/hu labels added.
- by_rating_key: generic-typed (type[_M] -> _M | None) so call sites keep their
Model | None static type.
- Migrate the last 2 open-coded PlexCollection-by-rating_key lookups (sync.py
resync_collection + delete_collection_local) to by_rating_key; zero open-coded
rating_key lookups now remain in the package.
- Add test_playlist_bulk_add_and_remove covering the bulk add/remove endpoints
and pinning the remove-bulk synchronize_session=False path.
Full suite 258 passed, 2 skipped; ruff clean.
- Restore synchronize_session=False on the remove-bulk Core delete (the 2.0
default is 'auto'; keep the pre-conversion contract explicit).
- _episode_nav: list(...scalars()) instead of [r[0] for r in ...all()], matching
the house style used across the conversion.
- Extract app/plex/lookups.by_rating_key() and migrate the get-by-rating_key
pattern at 13 sites (imgcache x4, _shared.item_or_404, playback show/season,
browse x6).
- Coverage: add TestFacetsWithData (the converted wsq aggregation with real
data — the highest-risk untested builder), TestPlaylistAndCollectionEndpoints
(playlist CRUD + collections list), and an item-state playback test.
Full suite 256 passed, 2 skipped; ruff clean.
Locks the observable behaviour of the previously-untested Plex query hot
paths before the 2.0 conversion, so it can be proven behaviour-equivalent:
- unified_library: both/movie/show scope, added-desc ordering, the four
watch-state branches, search rank + episode grouping, pagination.
- rollup.state_map: new / in_progress / watched rollup.
- sync upsert idempotency: _sync_movies + _sync_shows run twice = no dup
rows, in-place update, episode->show linking.
Green on the current 1.x code (9 passed). The conversion commits must keep
these identical.
Deep-review round 1 found the new tests asserted end-state only, so both
safety mechanisms this change adds were untested (a regression would ship
green):
- Per-page cursor durability: add test_a_mid_run_failure_leaves_the_prior_page
_durably_committed, which fails the 2nd page's fetch after page 1 committed
and asserts the cursor + rows are already persisted (re-read via db.expire).
Mutation-verified: reverting to a single end-of-loop commit makes it fail
while the pre-existing paging tests stay green — the property the original
test could not distinguish.
- Reserve guard: add test_backfill_reserve_reached_tracks_the_boundary, driving
real record_usage spend across the 9000/2000 default boundary and asserting
the un-mocked quota.backfill_reserve_reached() flips at exactly remaining<=reserve.
Full backend suite: 244 passed, 2 skipped (network-gated).
Addresses the code-review findings on the immediate full-history change:
- Extract quota.backfill_reserve_reached(): the remaining/reserve compare was
open-coded at seven sites (recent/deep/immediate backfill, explore, rolling
revalidation, and the new per-page guard). It reads in its OWN short-lived
session (like record_usage/log_action), so a per-page guard no longer holds
the caller's transaction open across the YouTube fetch that follows — the
400-page immediate run no longer pins a pooled connection idle-in-transaction
through each network round-trip. Migrated all call sites; dropped the now-dead
sysconfig imports in channels.py and maintenance.py.
- Simplify the deep-backfill loop: channel.backfill_done = not page_token (done
is False on entry) plus one break, replacing the doubled `if not page_token`.
- Tests patch the backfill_reserve_reached seam directly instead of mocking
remaining_today internals, removing the fragile dead-value budget iterator.
Full backend suite green; ruff clean.
The user-triggered "get full history" (backfill_channel_now) capped at
max_pages=60 = 3000 videos. A channel just over that (BBC Earth: 3403
uploads = 69 pages) stopped one page short, so its tail dribbled in over
the scheduler's paced 10-minute ticks (250 videos/tick) and the button
spinner looked stuck for ~18 minutes with no visible progress.
- Raise the immediate cap to max_pages=400 (20 000 videos) so any
realistic channel completes in the single background run.
- Make a high cap safe: backfill_channel_deep now re-checks the shared
quota before each page and yields at the backfill reserve (previously
quota was checked once by the caller before the whole run), and commits
the advanced cursor after every page so a restart resumes instead of
re-fetching — and re-charging quota for — pages already stored.
- Channel page: while loading, the button shows the live stored/total
count and a tooltip explaining the tail finishes in the background, so a
very large channel's paced remainder reads as progress, not a hang.
Adds deep-backfill paging tests (per-page cursor commit + resume, and the
per-page quota yield).
- Make the immediate-backfill guard SYMMETRIC: run_deep_backfill now also
registers the channel it pages in _backfill_in_progress, so it and
backfill_channel_now mutually exclude per-channel and never advance the
same cursor concurrently (whichever started first). The interactive cap
moves to a separate _immediate_running counter so the scheduler's channels
don't false-trip it.
- Attribute the immediate backfill's spend: wrap it in
quota.attribute(user_id, VIDEOS_BACKFILL_FULL) (route passes user.id), so
it shows under the user's full-backfill action, not the OTHER/system bucket.
- Tests: the update_channel->background-task wiring + the demo guard
(scheduled for a real user, NOT for demo); distinct N/S/L counts (2 short
+ 1 live) so a short/live swap fails; extracted channelDetailPollMs with a
pure unit test of the poll-or-stop predicate.
- a11y: role="img" on the fully-synced charm so screen readers announce it.
- list_channels no longer computes the Normal/Shorts/Live aggregation the
frontend stopped reading when R14 dropped the N/S/L table column; the dead
ManagedChannel.count_* fields are gone too.
- Immediate "get full history" backfill now coordinates with the paced
scheduler via a small in-flight set: capped at 2 concurrent + per-channel
dedup, and run_deep_backfill skips a channel an immediate backfill owns —
killing the cursor race and bounding the quota-drain burst, without losing
the speed win.
- Add test_channel_backfill.py: the channelDetail N/S/L counts + the guard's
no-op / cap / scheduler-skip behaviour.
- Inline the syncedChannels useMemo (its dep is a fresh array each render, so
it never cached); fix the stale "~2-min ticks" comment (it's 10 minutes).
- Floor the effective max_concurrent at 1 on the read path (quota.resolve)
and in the claim SQL (GREATEST(..., 1)). admin_set_quota already floors on
write and the sysconfig default's min is 1, but a stray stored 0 (direct DB
edit) would diverge the two "at limit" checks — the SQL claim-window
exclusion can't see a user "capped" with zero running jobs, while
at_concurrency_limit can — reopening the starvation the fairness fix closes.
Keeping the effective limit >= 1 makes the two checks provably agree.
- shared_with_me now orders by (created_at DESC, id DESC) to match
list_downloads, so the newly-added 500 cap has a stable boundary among
same-timestamp shares instead of a non-deterministic one.
- 0062 upgrade loop: rename the unused loop var "table" to "_table"
(matching the underscore-marked "_col").
Test: a stored max_concurrent=0 flooder no longer wedges the claim loop.
#2 — _claim_job excluded users at their concurrency limit only in Python,
AFTER a fixed LIMIT-50 fetch window. One user flooding the queue with >50
queued jobs while at their cap filled the whole window, chosen came back None,
and every other user's work past position 50 was never seen. Exclude capped
users IN THE SQL (per-user max_concurrent from the quota row, else the
sysconfig default; unlimited users never capped), keeping the Python re-check
as a race guard that can only reject, never over-claim.
#3 — list_downloads and shared_with_me loaded every job for a user with no
bound. Cap both at 500 newest-first (a shared _MAX_LIST, matching the admin
listing's existing cap); the Download Center renders newest-first and 500 is
far beyond any real manual-download history, so no frontend change is needed.
Tests: DB-backed — a capped user's 60-job flood no longer starves a user
behind the window; oldest-by-queue_pos claim, all-capped->None, unlimited
never excluded; and the list cap bounds the result set.
resolve_adapter matched sources by loose substring over the netloc, so a
look-alike host ('myyoutube.com', 'youtube.com.evil') misresolved. Match the
host exactly or as a subdomain via a _host_is() helper, and identify from the
netloc of BOTH source_ref and yt-dlp's webpage_url — the latter by host only,
never a substring over its path. This also restores a host signal to the
early-fill path (resolve_adapter(None, None, info)), which has no source_ref,
so a Facebook asset's uploader_url no longer depends on extractor_key being
present in the progress-hook info_dict.
Tests: look-alike hosts stay generic while real subdomains resolve; a
webpage_url netloc identifies the source when extractor_key is absent.
- resolve_adapter: identify from the yt-dlp extractor_key only, not the full
webpage_url (a sibling source's name in a URL path no longer misresolves);
add youtube-nocookie.com to the YouTube host match so nocookie embeds keep
the POT/player-client extraction strategy.
- _download: re-resolve the adapter with the post-extraction info dict so the
.nfo uniqueid type and uploader_url agree on one source (dedups the inline
resolve too).
- MediaMeta.uniqueid_type defaults to source-neutral "generic" instead of
"youtube", so an un-updated caller can never falsely label a non-YouTube
download as youtube.
- Tests: lock extractor-key-only resolution + nocookie host; default nfo type.
write_sidecars / delete_asset_files computed `base = media.with_suffix("")` and then
`base.with_suffix(".nfo")`. Stripping the real `.mp4` first leaves a stem whose own last dot may
be inside the title (e.g. "Sonka Házilag."), which `with_suffix` then treats as the extension and
clobbers — writing the .nfo/.jpg under a TRUNCATED basename that no longer matches the media, so
Plex/Kodi never associate them (and delete_asset_files targets the wrong name, leaking orphans).
Swap the extension on the FULL media path via a shared `_sidecar` helper instead.
Pre-existing bug (predates the source-adapters work); found during its E2E and folded in here.
Verified end-to-end: a dotted-title Facebook reel now writes a single shared basename.
Consolidates source-specific policy (youtube / facebook / reddit / generic) that was scattered
across worker.py, backfill.py, service.py, formats.py and storage.py with no shared source
identity. app/downloads/adapters.py holds the base (= generic yt-dlp behaviour) + per-source
overrides; resolve_adapter(source_kind, source_ref, info) picks one by source_kind, the yt-dlp
extractor_key, or the URL host, falling back to generic. yt-dlp stays the engine — the adapter is
policy only.
Moved into adapters (behaviour-equivalent):
- uploader_url derivation (YouTube channel_url / Facebook uploader_id / reddit None) — removes the
worker/backfill duplication (build_uploader_url + _should_refetch gone).
- the player-client fallback sets + retriable markers + YouTube extractor_args/POT (build_ydl_opts
now takes source-agnostic opts plus an `extra` dict from the adapter).
- source_url reconstruction (service.source_url delegates to the adapter).
Two intentional (approved) behaviour changes:
- the player-client fallback is now YouTube-only; non-YouTube sources make a single extraction
attempt instead of a pointless double attempt with youtube-only client args.
- the .nfo <uniqueid type=...> follows the source (adapter.nfo_uniqueid_type) instead of the
hardcoded "youtube" that mislabelled every url-source download.
Deliberately left generic (documented non-moves): title cleaning (normalize_title is harmless
across sources), the classify marker table, thumbnail_to_jpg, the format selectors, and og.py's
YouTube-hqdefault og:image preference (a self-contained thumbnail heuristic on the share path).
Verified end-to-end on localdev across all four adapters: downloads succeed and uploader_url + the
.nfo uniqueid type are correct per source.
- worker.py: keep an absolute wall-clock backstop for ANY still-alive child. A child that
reached a terminal state (done/error) but wedges in teardown is now killed to free the worker
slot ("abandon"), with its DB status left intact; only a still-'running' over-cap job is failed.
Previously gating the wall-clock on status=='running' let a wedged finisher loop forever (#1).
- worker.py: reconcile the claimed asset on ANY clean child exit (not only non-zero), so a child
that exits without finalizing it — e.g. an early return on a job DELETEd mid-flight — no longer
leaves the asset stuck 'downloading' until boot recovery (#2).
- worker.py: extract _stop_and_free + _fail_after_kill; the exception / stall / timeout / cancel /
shutdown / abandon paths now share one kill+free (+fail) tail instead of three copies (#3).
- tests: a wedged-finished child is killed by the wall-clock without a status change; clean exits
now assert the reconciliation no-op (#1, #4).
- worker.py: the watchdog loop now treats only 'paused'/'cancelled' (or a vanished row) as a
stop; a terminal status (done/error, or a re-queued contended asset) means the child already
wrote its outcome and is exiting, so keep polling instead of killing it. Previously a
proc.wait timeout landing between the child's terminal commit and its exit was misread as a
route-cancel and SIGKILLed a successful download (dropping its completion notification). The
stall/wall-clock/shutdown checks are now gated on status=='running' (#1, #5 test).
- worker.py: guard the crash-path _job_liveness probe so a DB blip there can't propagate (the
child is already dead; boot recovery is the backstop) (#2).
- worker.py: drop _stop_child's redundant poll guard — escalate_kill no-ops on an exited proc
and getpgid yields None for a reaped pid (#3).
- worker.py: note the extraction heartbeat is per-attempt (a single over-timeout attempt is
still killed, bounded by socket_timeout) (#4).
- worker.py: wrap the watchdog loop so a failure in it (e.g. a transient DB error in
_job_liveness) still hard-kills the child + fails the job, instead of leaving the child
running unsupervised — the exact wedge the feature prevents (#1).
- worker.py: _touch_job heartbeat bumps updated_at at each extraction attempt, so a slow /
multi-client-set fallback extraction (which writes no progress, and re-writes identical
values so ORM onupdate does not fire) is not false-killed as "stalled" (#2).
- edit.py: extract escalate_kill (terminate->wait->kill escalation); _stop and worker's
_stop_child (process-group variant) now share it (#3).
- worker.py: document the edit path's second, tighter run_ffmpeg watchdog and that the
supervisor is POSIX/Linux-container-only (#4, #6).
- tests: a long download stays alive while progress is fresh, and a watchdog-loop error
still stops the child (#1, #5).
Defense-in-depth after the ReDoS incident. The heavy work (yt-dlp download / ffmpeg edit)
now runs in a child process (app.downloads.runner) supervised by the worker thread, not in
the thread itself. A thread can't be killed and a C-level spin can't even be cooperatively
cancelled, so one wedged download used to pin a core, hold the GIL and freeze the whole
queue; a child process can be SIGTERM/SIGKILLed.
The supervisor kills the child (and its process group, so ffmpeg grandchildren die too)
when: a route pauses/cancels the job, it makes no progress for download_stall_timeout_seconds
(150s), it runs past download_wall_clock_max_seconds (2h), the worker is shutting down, or
the child crashes without setting state. On a kill it frees the asset and fails the job with
a retryable network error (stalls are not auto-requeued). A clean exit is left alone — the
child already wrote the terminal state, including a cooperative pause via the progress hook.
- config.py: stall / wall-clock / supervisor-poll / kill-grace settings.
- worker.py: _run_supervised + _spawn_download_child / _job_liveness / _stop_child /
_cleanup_staging; _process_job claims the asset, then hands the heavy work to the child.
- app/downloads/runner.py: the child entrypoint.
- docker-compose.localdev.yml: init:true on the worker so tini (PID 1) reaps orphaned
ffmpeg grandchildren left by a hard-kill (the worker reaps its own direct children).
- tests: watchdog decision logic + SIGTERM->SIGKILL escalation (fake proc, no real subprocess).
- edit.py: extract _run_ffmpeg_best_effort (one run+timeout+swallow policy shared by
thumbnail_to_jpg / ensure_poster / ensure_storyboard) + a build_thumbnail_jpg_cmd
builder, matching the module's other ffmpeg commands.
- thumbnail_to_jpg: copy an already-JPEG thumbnail (Facebook, and Reddit's jpeg-under-
a-.png-name) instead of re-encoding it; only webp/png-proper are decoded via ffmpeg.
- errors.py: drop the redundant "account authentication" classify marker
("authentication is required" already covers the known Reddit message).
- formats.py: document that the opt-in EmbedThumbnail is unaffected by dropping the
convertor (it converts unsupported thumbs to PNG, not the crashing mjpeg2jpeg path).
- test_titles.py: guard the ReDoS regression with a thread + timeout (deterministic
"must not hang") instead of a wall-clock assertion.
Three prod-observed download bugs, root-caused from a frozen prod queue with the
worker pinned at 100% CPU:
- ReDoS in normalize_title: _TRAILING_HASHTAGS had two overlapping unbounded
quantifiers around the required letter, so a Facebook title with a long hashtag
cluster that is NOT at the end ("... #a #b #c | Page") backtracked catastrophically.
It runs inside the yt-dlp progress hook holding the GIL, so one such download froze
the whole worker (and could stall feed enrichment too - sync/videos.py calls it).
Rewrite with a zero-width "has a letter" lookahead + a possessive tag body: one
unambiguous split, nothing to backtrack. 40-tag stress case: ~0.1ms.
- Reddit download killed by a cosmetic thumbnail convert: yt-dlp's
FFmpegThumbnailsConvertor runs "-bsf:v mjpeg2jpeg" on a JPEG-served-under-a-.png-name
(Reddit) and fails; being a postprocessor, it failed the WHOLE download even though
the media was fine. Drop it and produce the <base>.jpg poster/og:image ourselves via
editmod.thumbnail_to_jpg (plain ffmpeg decode->mjpeg, best-effort -> ensure_poster
cuts a frame on failure). The FB Messenger / MS Teams share preview keeps working
(posters verified as real JPEGs).
- classify: map "authentication is required" (Reddit's auth wall) to login_required
instead of unknown, so the UI explains it rather than offering a pointless retry.
Tests: ReDoS-timing + numeric-marker regression, the Reddit classify case, and
thumbnail_to_jpg (real JPEG out / None on bad input, never raises).
R12 S2. app/auth.py (1154 lines) mixed the core auth surface with 18 HTTP
endpoints on one /auth router. Split by dependency graph:
- app/auth.py (223 lines) keeps ONLY the core surface other modules import
(current_user, resolved_user_id, admin_user, require_*, has_*_scope,
purge_user, count_admins, get_or_create_demo_user, google_enabled,
is_allowed, operator_contact, optional_current_user) plus everything it
transitively depends on.
- app/routes/auth.py (883 lines, new) gets the router + all 18 endpoints +
their endpoint-only helpers, importing the core surface from app.auth.
- main.py includes auth_routes.router (was app.auth.router).
- test_oauth_redirect imports _oauth_redirect_uri from its new home.
The STAY/MOVE partition is driven by an AST scan of the COMPLETE external
surface (every `from app.auth import` incl. multiline + `from app import
auth; auth.X`), so no still-referenced symbol was moved. Behaviour-equivalent:
every symbol relocated verbatim (get_source_segment equality), route set
proven identical, no undefined names (symtable sweep).
Gate: ruff clean (backend + scripts), pytest 193 passed.
A full cold sync crashed and left plex_people empty (cast/people missing in
every detail view). enrich_row_people added PlexPerson rows via the ORM with no
ON CONFLICT, and because SessionLocal is autoflush=False a person shared by two
items in the same enrich batch was queued twice -> the single batch commit
violated the unique index ix_plex_people_name and rolled the whole batch back.
Prod dodged it by building up incrementally; dev hits it on every reseed+sync.
Insert missing people via an immediately-executed pg_insert with
on_conflict_do_nothing (deduped within the cast by name, keeping the first
non-null thumb), mirroring app/sync/videos.py:_insert_stubs. The immediate
execute also lets a later row in the batch see the row via the existing-names
query and skip it; the conflict guard additionally covers the concurrent
info-page enrichment path.
Adds a DB-backed regression test: two items with overlapping cast now enrich
without a unique-violation, one row per name, and a prior-NULL headshot is
back-filled. Mutation-checked (reverting the fix reddens it).
Address the two remaining review findings on the tag work:
- extract useChannelTagActions — the optimistic attach/detach + create-and-attach flow that
the channel manager and the channel detail page had copy-pasted; the cache-shape-specific
optimistic update is injected (manager maps a ManagedChannel[]; detail patches one
ChannelDetail), so the two stay in sync;
- add a DB-lane test asserting GET /api/channels/{id} returns exactly this user's personal
tag links — not another user's, not the auto/system (user_id NULL) links.
The FB Sharing Debugger confirmed the real cause of the missing desktop-Messenger preview: the
scraper fetches og:image ASYNCHRONOUSLY on a URL's first scrape ("The provided 'og:image'
properties are not yet available… specify the dimensions using 'og:image:width'/'og:image:height'"),
so a freshly shared /watch link unfurled title-only. Read the poster JPEG's real dimensions from its
SOF marker (Pillow isn't a dep; the poster aspect differs from the video's, so the stored video
width/height won't do) and emit og:image:width/height. Only runs on a crawler hit. Unit-tested.
Failed downloads stored the raw yt-dlp/ffmpeg text and dumped it as a truncated red
one-liner in the queue. Classify the failure into a category (unavailable / login_required /
geo_blocked / format_unavailable / drm / postprocess / source_missing / network / unknown)
and surface it properly:
- backend: app/downloads/errors.classify maps the raw message to a category code; the worker
stores it in the new download_jobs.error_code (migration 0061) at both failure sites; resume
clears it; the serializer exposes it. Raw text kept as technical detail.
- frontend: the queue row shows a clean localized reason + a "details" link opening a modal
with the explanation, the raw text behind a disclosure, and a Retry only for transient
categories (network/postprocess/unknown).
- tests: classify() unit coverage over verbatim yt-dlp/ffmpeg message shapes.
/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.
Transaction ownership (C-A5):
- quota.record_usage / log_action write in their OWN SessionLocal(), committed
immediately, and no longer take/commit the caller's db. The spend stays DURABLE
regardless of the caller (read-only YouTube paths never commit; a failed job rolls
back) — under-counting the shared daily budget would risk overspending the real
YouTube quota — AND quota stops flushing a job's partial state mid-run behind its
own `except: db.rollback()`. units_used_today switches to a scalar SELECT so
measured()'s before/after diff sees the separate session's commits (READ COMMITTED).
Call sites updated (youtube/client.py ×3, routes/search.py ×1).
- plex sync commits PER SECTION (moved the end-of-loop commit inside), so a PlexError
on a later library keeps the ones already mirrored (idempotent; next run reconciles).
The sync jobs did NOT rely on record_usage's incidental commit for persistence — their
per-item functions (apply_rss_feed, backfill_channel_recent, import_subscriptions,
sync_user_playlists, apply_channel_autotags) all commit internally, so the in-loop
rollback handlers already isolate a failed item. No loop restructuring needed.
DB-lane tests (test_quota_ownership, 4): spend survives a caller rollback while the
caller's uncommitted row does NOT leak; accumulation; measured-style mid-txn read.
Gate: ruff clean; DB lane 180 green.
/code-review high on S3 — 3 low findings, 2 fixed, 1 no-change:
- _db_reachable() now probes via a throwaway engine with connect_timeout=3, so a
slow/firewalled DATABASE_URL can't hang the whole suite at import (the app engine
has no connect timeout).
- test_plex_facets sets the "configured" gate flag via monkeypatch (auto-restored)
instead of poking state._configured_cache in a manual finally — no leak risk, and
the now-unnecessary mark_configured DB write is dropped.
No change: test_plex_jsonb_normalize copying migration 0060's SQL is deliberate —
migrations must stay self-contained (no app imports) and a shipped migration is
immutable, so there's no real drift to guard against.
Gate: ruff clean; DB lane 176 passed; pure-logic 172 passed / 4 skipped (fast).
First real customers of the DB lane:
- test_plex_facets: GET /api/plex/facets?scope=show returns 200 (not the
"cannot extract elements from a scalar" 500) for a show whose genres is a JSONB
scalar `null` — the 0.53.1 read-guard's regression test. Mutation-checked:
removing the jsonb_typeof='array' guard turns it red with that exact error.
- migration 0060 (S3a): one-time UPDATE …=NULL WHERE jsonb_typeof=… 'null' over the
plex_items/plex_shows tag columns, so pre-0.53.1 JSONB-null rows become uniform SQL
NULL and the read-guards go belt-only. test_plex_jsonb_normalize verifies scalar-null
→ SQL NULL while a real array is preserved.
Gate: ruff clean; DB lane 176 passed; pure-logic lane unchanged (skips the DB tests).
The suite was pure-logic only (R2 S2b deferred the DB lane). Add an opt-in `db`
fixture (tests/conftest.py) that hands a test a real session against an ephemeral
Postgres; schema is built with `alembic upgrade head` (the real prod schema needs
the unaccent extension + unaccent_simple TS config that create_all can't provide,
and this also proves the migration chain applies). Each test starts from a TRUNCATEd
clean DB so tests that COMMIT — the whole point, for R6 S2's transaction work —
don't leak.
backend/docker-compose.test.yml wires an ephemeral tmpfs pg + the test image. When
no Postgres is reachable (a plain `docker run`), the DB tests SKIP and the pure-logic
lane is unchanged: 172 passed, 2 skipped. With the DB: 174 passed.
test_db_lane.py smoke-proves commit round-trip + TRUNCATE isolation. The gate wiring
(siftlode.sh) lands in the ops repo.
/code-review high round 1 on S1b — 5 findings, 1 fixed, 4 by-design/deferred:
- FIXED: VideoProgressIn / ItemProgressIn position_seconds & duration_seconds
typed `int` would 422 a raw float currentTime on the 10s progress checkpoint
(the old `int(payload.get(...))` truncated it). Back to `float`, truncated in
the handler; +test. The live frontend floors so this was latent, not live.
By-design / deferred (no change): the 422 detail-array isn't localized by the FE
(filed under R7); PATCH is-not-none (channels/quota, null-safe) vs model_fields_set
(nullable-clear) is a deliberate split; missing-field→422 is the point of the epic;
me/switch_account's numeric-string coercion is more correct and still session-gated.
Gate: ruff clean, pytest 172 green.
Pure-logic tests that the three ROADMAP-named 500-producers (channel priority,
tag_id, setup password) and admin quota now reject bad input at the model
boundary, and that the PATCH models separate absent from explicit-null via
model_fields_set. Mutation-checked: reverting a field type turns them red.
/code-review high round 1 (5 findings):
- iso(dt) now typed `date | None` — MediaAsset.upload_date is a plain `date`, not a
datetime; the helper's signature was too narrow for the type-contract epic. +test.
- restore alphabetical app.* import order in audit.py and plex.py (the new iso/_common
imports had been dropped in mid-block).
The other 3 findings are deliberate design choices, left as-is for UAT: push/push-plan
now gate write-scope before the 404 (dependency ordering); the unified 403 message; and
owned_or_404's owner_attr kept for S1b's DownloadShare.to_user_id-style owners.
Gate: ruff clean, pytest 164 green.
Establish the R6 API-contract infra before the S1b payload:dict migration:
- iso(dt) in utils.py replaces the ~16 hand-rolled `x.isoformat() if x else None`
serializer idioms across the routes.
- get_or_404 / owned_or_404 in routes/_common.py — one 404 + existence-leak policy;
plex's _own_playlist_or_404 and saved_views' _own_view now delegate to it.
- require_write_scope dependency in auth.py collapses the inline has_write_scope→403
checks (channels subscribe/unsubscribe, playlists push/push-plan) that had drifted
into three different messages into one; the conditional delete_playlist guard stays
inline (its message was already canonical).
No response-shape change (iso == isoformat for a present value). Gate: ruff clean,
pytest 163 green (+ test_utils for iso).
Found by running the round-8 fix in the real container instead of trusting the
unit test: a directory created 2ms AFTER `_PROCESS_START` was stamped with an
mtime 2ms BEFORE it, so the sweep deleted it as a leftover. The two sides come
from different clocks — a file's mtime from the kernel's coarse clock (one timer
tick of lag), `time.time()` from the fine one — and the unit test could not see it
because it supplies an artificial cutoff.
The window is small and lands exactly where it hurts: the sweep runs at boot, so
the session at risk is one started in the first instants after startup — the case
the guard exists for. Keep-side slack of 2s; the cost is that a leftover from the
last seconds before a restart survives until the next one.
Verified in the container: a backdated leftover is swept, a directory created by
this run is kept, and a registered live session with an old mtime is kept — 1 of 3.
Test pins the skew (mutation-checked: with the grace at 0 it goes red).
The worst one was, again, the previous round's own fix.
- Round 7 stopped awaiting the HLS sweep so startup could serve immediately — which
also made it run CONCURRENTLY with playback, and `sweep_orphans` snapshotted the
live-session set BEFORE listing the root and rmtree'd outside the lock. Since
`start_session` reuses a directory path for the same (key, offset, tag), a stale
name can become a live session at any moment. Two guards now: nothing whose mtime
is newer than this process's start is ever deleted, and the live check + rmtree
happen together under the lock, one directory at a time.
- The sweep task is awaited after cancel, so shutdown can't leave it pending.
- `_rate_limiter` used a truthiness test on its timestamp, so a `now()` of exactly
0.0 read as "never ran" — unreachable in production, but `now` exists to be
injected and a test clock starting at 0 is the obvious choice.
- The RSS total-outage error now carries the first underlying error: "all N channels
failed" is also what ONE dead feed id looks like on a small instance, and a red
card the user can't act on is its own kind of dishonest.
- The WebKit fullscreen handling moved into `lib/fullscreen` and PlexPlayer uses it
too — it had the same unprefixed-only reads in three places, including an Escape
branch that would have closed the whole player instead of leaving fullscreen.
- The `--max` CSS comment claimed `inset: 0` did the sizing; Tailwind's `w-full` is
still on the element and wins. Comment now states the real mechanism.
Verified in real Chrome (the pane can't composite or do native fullscreen):
- F fills the 1920x889 viewport; the exit pill renders top-right on the letterbox
band and overlaps NO YouTube control — the collision this round suspected does not
reproduce in this embed (its own buttons sit bottom-left / bottom-right).
- Four real ArrowDown presses while maximised: persisted volume 100 -> 80, with the
level flash visible on screen.
- A real click on the exit pill un-maximises and returns focus to the card.
- lib/fullscreen driven from a real click: enter (innerHeight 889 -> 1024), the
change listener fires exactly once per transition, exit returns to 889.
Backend suite 150 -> 158; the new sweep tests are mutation-checked, and they read
their cutoff from the filesystem rather than a clock (the container VM and the
bind-mounted host disagree by enough to make a clock-based assertion flaky).
Five of them sit where round 6 turned F from a Fullscreen API call into a CSS
maximise: the guards around it still asked `isFullscreen`, which that pivot made
permanently false.
- Arrow-key volume was dead in the maximised view: the scroll-deference checked
only `isFullscreenRef`, so the (now hidden but still scrollable) card kept
swallowing Up/Down. Both flags now count as "the video fills the screen".
Measured: 87px of scroll room on the focused card, ArrowDown moves 80 -> 75
and scrolls nothing.
- The maximised view had no way out once the pointer touched the video. Reaching
YouTube's controls means clicking into the cross-origin iframe, which takes
keyboard focus with it, and the re-arm paths (stage mouseleave, window focus)
need the pointer to leave the browser entirely while the stage fills it. So
the exit now lives on screen, in our DOM: a button that un-maximises AND pulls
focus back, re-arming every shortcut.
- WebKit fires only `webkitfullscreenchange`/`webkitFullscreenElement`, so on
Safari/iPadOS YouTube's own fullscreen never registered — no overlay yield, no
focus reclaim. One `fullscreenEl()` reader, both event spellings.
- The stage claimed `z-index: 9999`, the glass tuner's reserved level, for a job
that only needs to beat the modal chrome it is nested in (its siblings top out
at z-menu). Dropped to rail-level; nothing escapes the modal's stacking
context either way.
- `inset: 0` already sizes a fixed layer — the inherited `100vw`/`100vh`
over-constrained it, which is how a classic scrollbar gets a horizontal
overflow and a mobile URL bar hides the bottom edge (YouTube's control bar).
- The shortcut hint still promised "F: fullscreen" in both locales.
Backend, both "the fix stopped one layer short":
- The startup HLS sweep was awaited inside lifespan, so uvicorn still served
nothing until it finished — a thread doesn't help when nothing is listening
yet. Fire-and-forget task instead, cancelled on shutdown. Measured with 200
planted segments: "Application startup complete" now precedes "swept 1".
- An RSS poll where every channel failed returned normally, so the scheduler
recorded `ok` with the outage buried in the summary string. `failed == total`
now raises: a run that reached nothing is a failed run.
Suites: backend 147 -> 150.
The reported bug: after F, the embed's own fullscreen button is dead. Root cause
measured in Chrome — the browser's fullscreen lock sits on OUR wrapper, so
YouTube's button would need a nested request from a cross-origin frame. Handing
the lock to the iframe fixes it, but `iframe.requestFullscreen()` only resolves
from a CLICK (from a key press the promise stays pending forever, and awaiting
it spends the activation so a fallback is denied too).
So: F keeps fullscreening our wrapper (unchanged, our overlays stay visible) and
a new toolbar button hands the lock to the iframe — verified in real Chrome:
document.fullscreenElement is the IFRAME at 1920x1024, i.e. a plain YouTube
fullscreen where its own controls work.
Round-6 review findings, all fixed:
- one _rate_limiter + one _cancel_poll shared by the yt-dlp hook and run_ffmpeg
(was three hand-rolled throttles; cancel latency differed 2s vs 1s)
- stepVolume takes {delta, fallback} — two adjacent number args were silently
transposable
- volume gestures before the player is ready are honoured instead of dropped
- normalizeVolume's fallback is typed unknown (its guard was unreachable) and
applyVolume takes a number again
- the throttle tests inject a clock instead of patching global time.monotonic
- run_ffmpeg stops the process from ONE exit path (`except BaseException`), so
an exception out of on_progress (a DB write) can no longer orphan a live
ffmpeg after a 10s join on its pipes; _stop is now a no-op on a dead process
- both run_ffmpeg callbacks are throttled to ~1s (they were a DB round-trip per
output line: ~10^5 queries on a long re-encode), mirroring the download hook
- volume arithmetic extracted to lib/playerVolume with unit tests; a corrupt
level now falls back to the LAST GOOD one instead of full blast, and the
sanitising happens where the stored value enters
- RSS is back to two except clauses (the shared tail already sits after the try)
- new tests: callback-exception cleanup, the two throttles; the fake Popen grew
poll() and its wait count is now asserted
Mutation-checked: each new group goes red when its fix is reverted.
- Feed memoizes onClose (like Playlists already did): the inline closure re-ran
PlayerModal's keydown effect on every Feed render, which since the debounce
landed also flushed the volume write and re-stole focus
- the player reads its volume with readAccountMerged again — a partial stored
object made the level undefined -> NaN -> a persisted null; applyVolume also
refuses a non-finite level outright
- that read moved into a lazy useState initializer (was running per render)
- the two RSS except branches share one tail, so a counter can't drift again
- run_ffmpeg tests cover the SIGTERM->SIGKILL escalation and the lingering-
process path; dropped an assertion that only restated the fixture
- storage.ts documents the actual { volume } shape (0 IS muted)
- the sweep guard anchors the rating_key to digits: the round-2 widening had
degenerated to "anything ending in _<digits>" (backup_2024, release_10)
- RSS counts apply-side failures too, so a read-only database no longer reports
a wholly failed poll as "ok — new=0, failed=0"
- the player reads/writes its volume directly (readAccount/writeAccount): the
persisted-object hook rendered nothing and cost a second write per settle
- new tests: the sweep guard's accept/reject table, and run_ffmpeg's threading
(drain, cancel while silent, stall watchdog, stderr tail on a nonzero exit)
- the breaker sentinel is checked one way (isinstance) at all four sites
- run_rss_poll is typed dict[str, int]
All three new test groups were mutation-checked: reverting each fix turns them
red (or, for the cancel/stall pair, hangs — which is the bug itself).
- the volume keys no longer defer to a scrollable card while in fullscreen
(mirrored into a ref: the keydown handler is bound once and state would be stale)
- the debounced volume flush writes straight to storage; persisting inside a
setState updater is not guaranteed to run for an unmounting component
- the HLS sweep also matches the original {key}_{start} directory naming, which
is what the oldest leftovers on disk actually use
- run_rss_poll returns {new, failed} so an egress outage stops rendering as
"ok — 0" in the scheduler card and the audit log; /api/sync/rss reports it too
- drop the unused poll_rss_channel (it silently gained a new exception)
- _Breaker.run is generic, so a call site keeps its item-id type
- test helper instead of the generator-throw idiom
- HLS sweep only deletes directories matching the session-name shape; the root
is admin-configurable and may be a shared path
- playlist unwrap picks the first entry that actually downloaded, not entries[0]
- concat quoting keeps backslashes literal (ffmpeg treats them so inside '')
- ArrowUp/Down defer while the focused element can still scroll that way
- the staging fallback skips yt-dlp working files (.part-Frag/.ytdl/.temp/.fNNN)
- the boot HLS sweep runs off the event loop
- volume persistence is debounced (was a setItem per wheel notch) and a spin no
longer loses notches to the iframe's lagging getVolume
- _Breaker.run owns the whole open/call/record protocol; four call sites shrink
- should_prune extracted and unit-tested, plus RSS error-vs-empty tests
- the worker exits non-zero when the schema never lands
ffprobe prints codec_name BEFORE codec_type, so the line parser attributed every
name to the previous stream: a plain H.264+AAC mp4 probed as (aac, None) and the
browser-playability check re-encoded a perfectly good file on EVERY download.
Verified on localdev — the same source now stores without a re-encode pass.
- never prune subscriptions on an empty fetch: one anomalous empty-but-200
response wiped every subscription (and its deep_requested flag) in one run
- push_playlist: stop after 5 consecutive write failures — a quota-dead or
scope-revoked account otherwise burns 50 units per remaining item
- RSS: raise RssError instead of returning [] on a transport/HTTP failure, so a
failed poll no longer stamps last_rss_at and a 404 feed stops looking healthy
- worker: exit when the schema never appears instead of "continuing" into a
crash one call later
- pick the produced file safely: Path("") is Path("."), which passes .exists(),
so a missing yt-dlp filepath skipped the staging fallback and crashed in
with_name ("PosixPath('.') has an empty name") on non-YouTube sources
- unwrap a playlist/multi_video extraction to the entry that carries the media
- run_ffmpeg: drain stderr while it runs (a full 64KB pipe blocked ffmpeg
forever and hung the worker thread), 1s cancel polling, stall watchdog and a
bounded wait on exit
- escape apostrophes/backslashes in the ffmpeg concat list (pre-0.51.0 names)
- sweep orphaned HLS segment dirs at startup (nothing else ever reaped them)
peter