Review round on the E2 stack (S1-S5). No correctness bugs; the surviving
low/medium findings addressed:
- altitude (#1): extract the shared `tags_by_category` helper used by BOTH
filtered_query (SQL tag WHERE) and feed_summary (Python facet/count), removing
the category-grouping duplication, and add bidirectional "keep in lockstep —
equivalence-tested by test_feed_summary_matches_count_plus_facets" comments so
the SQL<->Python tag rule can't silently drift.
- test-coverage (#2): the /feed/summary equivalence test seeded only system tags,
leaving the `ChannelTag.user_id == user.id` visibility branch untested. Add a
user-owned Tag + link + two combos; the suite is now 346 (was 344) and the new
cases pass (feed_summary agrees with /feed/count + /facets on user-visible tags).
- dead-code (#3): annotate get_feed_count, get_facets, and the FE api.feedCount/
facets as "SUPERSEDED by /feed/summary — kept only as the equivalence oracle;
retire together" so the intentionally-retained surface reads as deliberate.
Not changed (confirmed no-op): the search channel-name resolve (#4) is
once-per-request on a 633-row table and a deliberate pre-resolve for PERF-12's
BitmapOr plan; the facet loop (#5) already computes passes() once per
(channel, category) with a distinct exclude each, so there is no recomputation.
Gate green: ruff + 346 pytest; frontend tsc/knip/prettier (FE change is a comment).
The FE fired /feed/count (1 pass) + /facets (3 passes) on every filter change =
4 full scans of the subscribed set. New /api/feed/summary does ONE grouped query
(the base filter with tags stripped, GROUP BY channel_id -> per-channel video
counts) and derives the total count + every tag's distinct-channel facet count in
Python on the ~few-hundred-row channel map, faithfully mirroring filtered_query's
channel-based tag logic + the /facets drill-down (own category dropped, unless a
conjunctive topic-AND). Returns {count, facets} — the same numbers the two old
endpoints returned.
FE: a single shared query (Feed reads .count, Sidebar reads .facets); same key +
filters, so React Query dedupes to ONE request. Invalidations repointed to
qk.feedSummary. The old /feed/count + /facets endpoints (and their now-unused FE
api/qk) are kept as the equivalence-test oracle + safety net; a later cleanup can
drop them.
New equivalence test (test_feed_views, 8 filter combos incl. cross-category,
disjunctive/conjunctive topic modes, search): /feed/summary count == /feed/count
and facets == /facets, byte-identical. Structural win: 2 requests / 4-6 queries
-> 1 request / 2-3 queries (the review measured 530 -> 144 ms). Gate green:
344 pytest, frontend tsc/eslint/knip/prettier/222 vitest.
PERF-12: the search ORed `unaccent(channels.title) ILIKE '%q%'` straight into
the per-video predicate, which forced a full scan + detoast of every
search_vector and a per-candidate channel fetch. Pre-resolve the matching
channel ids (a few-hundred-row scan) and match `Video.channel_id.in_(ids)`
instead, so the planner keeps the search_vector GIN as a BitmapOr branch
(alongside ix_videos_channel_pub_desc). Empty match -> false() collapses the OR.
PERF-16: show=watched|hidden|in_progress used an outer join + coalesce(status,
'new'), so the planner couldn't start from the user's small video_states set.
Those views only return rows that HAVE a state, so inner-join video_states and
filter the raw column (status is NOT NULL on a present row, so it's identical).
'unwatched'/'all' keep the outer join. Dropped the now-dead position_expr.
New regression tests (test_feed_views): search returns both text- and channel-
name matches; the four watch-state views return the exact expected sets.
Measured on the prod-sized localdev reseed (prod planner, 0067/0068 applied):
- search /feed/count q=magyar: 663,463 -> 19,206 buffers (1,406 -> 31 ms; OLD
seq-scan+detoast -> NEW BitmapOr GIN+channel index)
- search feed page: 2,908 -> 1,479 buffers (32.6 -> 1.4 ms)
- show=watched (user with 64 watched): 40,661 -> 264 buffers (~154x)
Gate green (336 pytest, incl. the 2 new equivalence tests).
A nullable published_at forced the feed keyset into a non-sargable
`... OR published_at IS NULL` disjunction: with the 0067 index the planner still
applied it as a FILTER and scanned from the top of the index, so a deep `newest`
page discarded every earlier row (O(depth)).
- Migration 0068: backfill any NULLs (0 in prod) -> SET NOT NULL + DEFAULT now().
- Model: Video.published_at is now non-Optional, nullable=False, server_default now().
- feedquery: the `pub` key is nullable=False, and keyset_predicate is rebuilt
recursively so a non-nullable leading key gets a non-strict range bound
(`published_at <= v`) as an AND conjunct — an index range Cond instead of a
filter. Nullable keys keep the NULL-aware OR form (a `<= v` bound would drop
their NULL group). Equivalence proven by test_feed_keyset (all 13 sorts,
cursor-walk == unpaged).
- Ingest paths (stub_from_playlist_item, apply_video_details, playlists sync)
fall back to now() so they never write an explicit NULL.
Measured on the prod-sized localdev reseed at a ~50k-deep cursor (0067 index +
prod planner): OLD `Rows Removed by Filter: 50001`, 25,687 buffers / 9.16 ms ->
NEW `Index Cond: published_at <= v`, 1 row filtered, 47 buffers / 0.037 ms. SET
NOT NULL on 257k rows: 0.32s. Gate green (334 pytest).
Review round 2: the unconditional `command.upgrade(cfg, "head")` in finally ran
a redundant no-op on the success path and, on a genuinely broken re-upgrade,
replaced the try's original exception with its own. Gate the restore on a
`reupgraded` flag so it fires only when the upgrade didn't complete — the
cascade-prevention intent is kept without the double-run or error-masking.
Review round on the whole E1 stack (main..HEAD). 6 findings, all low/medium,
none blocking; fixed for a clean gate-green handoff.
- plex/sync (correctness): the un-enriched cast guard compared the stored cast
against THIS listing's length, so a trimmed Plex cast on a not-yet-enriched
row was never applied. Compare against the listing CAP (_LISTING_CAST_CAP=20)
instead: >cap = enriched (keep, survives a watermark reset), <=cap =
listing-sourced (track edits in both directions). +2 regression tests.
- main (security): add X-Frame-Options: DENY as the legacy-browser fallback for
the CSP frame-ancestors clickjacking guard.
- downloads/service (altitude): finalize_if_running now uses a SQLAlchemy 2.0
update() construct instead of a hand-built text() UPDATE string — same atomic
WHERE status='running', idiomatic, no identifier interpolation, no manual
StrEnum coercion. Drops the now-unused text import.
- auth/admin (reuse): extract the SEC-10 "drop untrusted preset password + bump
epoch" invariant into one helper (revoke_preset_password_if_unverified) so the
Google-callback and admin-activation paths can't drift.
- tests/migrations (robustness): guarantee the shared schema is restored to head
in a finally, so a downgrade/re-upgrade failure can't cascade into later tests.
- worker (simplification): align _finish_from_cache's display_name fill with the
_download path's guard (no behavior change).
Gate green: ruff clean, 334 pytest (0 skip), frontend unchanged.
SEC-10 (account pre-emption / "pre-hijack"): a password set on an account whose
email was never verified is untrusted — whoever registered the address (to
pre-empt the real owner) could have chosen it. Both the Google-adoption callback
and admin activation (_activate_user_by_email) now drop that pre-set password and
bump the session epoch when the account isn't email_verified, so a later Google
login / admin approval can't hand the pre-set password a working login. No-op for
a verified account (the legit no-SMTP flow). +2 tests.
SEC-11 (link/upgrade bound to the session default): the OAuth link/upgrade GETs
resolved to the session DEFAULT account, so a tab acting as a secondary account
would link Google / the YouTube grant to the wrong account. The frontend now
threads the active account through the navigation URL (withActiveAccount ->
?account=<active>), and _complete_link accepts any wallet account, not only the
default, so the identity lands on the account that started the flow.
The pip-audit step added in E1 S4 flagged cryptography 46.0.7 for PYSEC-2026-3552
/3553/3554 and GHSA-537c-gmf6-5ccf; all are fixed by 50.0.0. The requirements.txt
ceiling `<47` was blocking the upgrade — relaxed to `>=50.0.0,<51`. The lock is a
surgical bump (only the cryptography line changed; cffi 2.1.1 already satisfies
it, no other package moved). pip-audit -r requirements.lock is now clean.
Add tests/test_security.py (there was none for the crypto primitives): the
Fernet encrypt/decrypt round-trip, decrypt_optional degrading on garbage and on a
key mismatch (the B-core-2 path), and the argon2 password hash/verify round-trip
— so a future crypto-lib bump can't silently break the token-at-rest path. Gate
green: 330 pytest.
The drift check dropped every diff except table/column presence, so a column
type or nullability change would slip through. Add modify_type/modify_nullable
to the ops it fails on. That immediately surfaced a real pre-existing drift:
notifications.created_at was created NULLABLE by migration 0017 but the model
has always declared it NOT NULL (Mapped[datetime] + server_default=func.now()).
Migration 0066 enforces NOT NULL to match the model — safe, since the
server_default has filled every row since day one.
Address the /code-review findings on the S2 test-net:
- as_user no longer sets a global `lambda: user` override that two users in one
test would collide on; the shared current_user override now resolves the caller
from an x-test-user header, and each returned client tags its own requests, so
as_user(a)/as_user(b) are independent (#1).
- override_dep records the prior value per dependency key and restores it on
teardown instead of a blanket dependency_overrides.clear(), so fixtures compose
without clobbering each other (#5).
- a _configured fixture bakes the post-setup state into client/as_user, and the
per-test inline monkeypatch / autouse duplicates are removed (#4).
- the feed keyset walk asserts all seeded rows surfaced before comparing, so a
seeding regression can't make the sorts pass vacuously (#3).
The auth security mag had zero tests. Add a DB-lane suite over the real
auth/session stack (TestClient carries the signed cookie): password login ->
/api/me authenticated; a wrong password and an unknown email return a
byte-identical 401 (no enumeration oracle); an X-Siftlode-Account header naming
a non-wallet account falls back to the default (impersonation gate); logout-others
bumps the epoch so a second session is rejected (401 on a protected endpoint)
while the caller stays in; and password-reset/confirm is single-use (replay ->
400) and bumps the epoch so pre-reset sessions die.
The feed's cursor paging (sort_keys/order_by/keyset_predicate/encode/decode) is
the NULLS-LAST + tiebreaker algebra where a page-boundary bug hides as a
duplicated or skipped row on the busiest endpoint, and had zero tests. Seed 7
videos on 2 channels with duplicate sort values and NULLs across
published_at/view_count/duration/title/subscriber_count, then assert — for every
sort — that walking the feed in pages of 2 via the cursor reproduces exactly the
order a single unpaged query returns, with no duplicates.
Prod only ever runs `alembic upgrade head`; nothing checks that the migrated
schema still matches Base.metadata, and the downgrade path never runs. Add a
DB-lane test that (a) asserts no structural (table/column) drift between the ORM
models and the head schema — filtering the index/constraint/sequence noise that
compare_metadata reports for migration-only objects — and (b) exercises the head
migration's downgrade + re-upgrade so a broken downgrade is caught the sprint it
lands.
The DB-backed lane and the two ffmpeg tests skipped SILENTLY when the DB was
unreachable or ffmpeg was absent, so `siftlode check` could print green while
~60 tests never ran. Make the compose lane authoritative:
- SIFTLODE_TEST_REQUIRE_DB=1 in docker-compose.test.yml; conftest.pytest_configure
turns a would-be skip (no Postgres, or no ffmpeg) into a hard UsageError.
- install ffmpeg in Dockerfile.test so the thumbnail/poster tests actually run.
- `-rs` in pytest.ini so skip reasons are always visible.
Add the shared TestClient fixtures the risk-core suites build on: `make_user`
(create + commit a login-ready User), `client` (real auth/session stack, no
current_user override — for login/wallet/epoch), and `as_user` (a client
pre-authenticated as a given user, for tests that aren't exercising login).
Follow-up to the /code-review high on the S1 branch. The per-site
`if job.status == 'running'` guards were a non-atomic read-then-write (TOCTOU):
a cancel committing between the child's status read and its commit could still
be overwritten with done/error, re-opening the double-release window.
Introduce service.finalize_if_running(db, job_id, **fields): one atomic
`UPDATE download_jobs SET ... WHERE status='running'` that returns whether it
fired. It flushes pending asset writes first so the asset-then-job row-lock
order matches the cancel route (no deadlock), and runs in the caller's session
so it commits with the asset writes in one transaction. All five worker sites
(download/edit completion, both error paths, cache finish) now go through it,
so the invariant lives in one place. The 'job done' logs move inside the
finalized branch so a job stopped mid-completion no longer logs a false 'done'.
Add tests/test_downloads_finalize.py: running -> finalizes; canceled/paused ->
no-op.
The cancel route writes status 'canceled' but the supervisor compared against
'cancelled', so its "a route stopped it -> kill the child, free the asset"
branch was dead code. A canceled job's child ran to completion and wrote 'done'
unconditionally over the route's 'canceled'; since 'done' holds the asset but
'canceled' does not, a later delete released the asset a second time, dropping
ref_count below the live holder count and deleting a file still in use.
Add app/downloads/status.py as the single source of truth for the lifecycle
literals (JobStatus StrEnum + the HOLDING / STOPPED_BY_ROUTE sets) and use it at
the safety-critical sites: the supervisor stop check, the route cancel, and
release_asset's holding set. Make the worker's terminal done/error writes
conditional on the row still being 'running' so a child finishing just after a
route cancel/pause can no longer resurrect it. Add the missing 'canceled'
supervisor regression test.
(The full literal sweep across the downloads surface is left for the E3
downloads owner-unification; this is the data-safety subset.)
1. (correctness) Show hero rating regression: PlexBrowse's show hero rendered a
rating only when show.imdb_rating was set, so after the S6 IMDb-only change a
show with no imdb:// entry showed no rating. Now falls back to the audience
`rating` like PlexInfo.
2. (correctness) ratingMin column mismatch: the rating filter and the /facets
rating_max now key off coalesce(imdb_rating, rating) — the SAME "best score"
the badge displays — so a ratingMin picked from a shown IMDb score narrows the
right column instead of the raw audience `rating`.
3. (altitude) HU search gap: new query.vector_match() matches EITHER vector in HU
(or_(search_vector_hu, search_vector)) so a row with no HU twin stays findable
by its English title; ranking still uses the HU vector. field_recheck unions
the HU+EN per-field tsvectors in HU, staying a strict subset of the prefilter.
4. (test-coverage) Extracted the badge logic to lib/plexRating.pickRatingBadge
(shared by PlexInfo + the show hero) with a unit test (imdb-preferred, audience
fallback, 0-score, none). Backend tests added for the HU union search + the
IMDb-preferred rating filter.
5. (efficiency) Language switch no longer double-fetches the grid: browseQ is
gated on a committedLang state that lags `lang` by one render, so the
genre-clear effect settles into a single fetch instead of one throwaway fetch
with stale-language genres plus a second.
DB-lane suite 295 passed / 2 skip; FE tsc/knip/lint clean, 217 vitest pass.
E5 S6 backend half (no migration — the *_hu / rich columns exist from S5a/S5b).
Language-aware serving (?lang=hu serves the HU overlay twin, EN fallback per row):
- query.py: column pickers title_col / genres_col / vector_col; field_recheck and
meta_filter_conds take `lang`. DISPLAY/FILTER pickers coalesce(*_hu, *) (EN
fallback); SEARCH pickers use the raw *_hu column mirroring the generated
search_vector_hu (coalesce to '', not EN) so the field recheck stays a strict
subset of the GIN vector (the S2 invariant). genres_col type_coerces the coalesce
to JSONB so the GIN `.contains` survives.
- browse.py: `lang` param on /library, /facets, /item, /show (validated to en|hu).
unified_library title/vector/rank/field-scope + meta filters are language-aware
for both the movie and show branches; facets unnest the active-language genre
vocabulary (jsonb_typeof guard kept); detail endpoints serve HU title/summary/
genres/show_title with EN fallback. Episodes stay EN (not overlay-localized).
IMDb-only rating (folded-in S5b review #5):
- detail.py rich_detail: drop the audienceRating/rating fallback → imdb_rating is
NULL when there's no imdb:// Rating entry (stop mislabelling a Plex audience score
as IMDb). item_detail now also serves the `rating` column (audience score;
show_detail already did) so the UI can show it when there's no IMDb score.
Tests: test_plex_lang.py (HU title serving + EN fallback, language-specific facet
vocabulary, genre-filter language matching, item_detail lang + rating split);
test_plex_detail updated for IMDb-only. DB-lane suite 293 passed / 2 skipped; ruff clean.
Second-round /code-review high follow-ups:
- test_feed_cursor.py: GET /api/feed?cursor=<garbage> must return HTTP 400
(not 500) — locks the cursor-error path the refactor split across modules
(feedquery.decode_cursor raises ValueError, get_feed translates to 400).
Plus a no-cursor 200 sanity check. DB lane.
- get_feed: `raise HTTPException(...) from None` so the expected bad-input
ValueError isn't chained as __context__.
DB-lane suite 289 passed / 2 skipped; ruff clean.
Address the /code-review high finding on S5b: gating the info-page cast enrich on
`people_enriched_at is None` removed the old per-view regrow of `cast_names`, so a routine full sync
that reset it to the listing top-20 (`_apply_facet_fields`) — while never clearing people_enriched_at —
left it permanently truncated, silently dropping cast past the top ~20 from the S4 person-page filter.
Root fix: `_apply_facet_fields` now only takes the listing's cast for a NOT-yet-enriched row; an
enriched row keeps its fuller stored cast (people_text still rebuilt from the updated directors + the
preserved cast). Makes the people_enriched_at flag the endpoints/backfill trust actually reliable.
Tests: cast_names preserved for an enriched row vs applied for a fresh one; show_detail lazy-fill
persists imdb/full_cast on first view. Full suite green.
Persist the info-page rich fields that were previously read LIVE from plex.metadata() on every view —
imdb_rating (IMDb-specific, vs `rating`=audienceRating), imdb_id, tagline, and full_cast
([{name,role,thumb}]) — onto plex_items/plex_shows, and serve the detail endpoints from the row.
- migration 0065: ADD imdb_rating/imdb_id/tagline/full_cast (nullable → no rewrite/lock, unlike 0064);
NULL people_enriched_at on movie/show rows so the existing cast backfill re-fetches each once and
fills the new columns.
- new app/plex/detail.py: rich_detail(meta) (moved out of browse._rich_meta so sync + serve share one
parser) + apply_rich_detail(row, meta); wired into _enrich_people_batch and the info-page lazy fill.
- item_detail: movies serve cast/IMDb/tagline/genres from the row and keep plex.metadata(markers=True)
ONLY for markers + audio/subtitle streams (so detail survives a live-Plex outage); episodes (never
enriched) still parse detail from that live meta.
- show_detail: serves cast/IMDb from the row; `related` stays live best-effort and also lazily fills a
not-yet-enriched show.
- tests: rich_detail/apply_rich_detail parsing + persistence, backfill stamping, item_detail
outage-resilience + lazy fill + episode-live, show_detail row serving. Full suite 285 pass / 2 skip;
migration verified reversible (up→down→up).
Precondition for S6's pure-DB language-aware serving.
- Correctness: _sync_overlay now first-wins on a guid (a `seen` set) so a duplicate HU entry
sharing a guid can't overwrite the applied Hungarian metadata or double-count `overlaid`.
- Test-coverage: cover the earlier dup-guid fix (both editions of a shared-guid pair get *_hu,
counted once) and the guid-keep guard (_apply_item preserves a prior guid when the listing
carries none, still updates on a real one).
Full backend suite 276 passed / 2 skipped.
- 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.
Address review findings on the title-clamp: TITLE_MAX_LEN now reads the length straight
from Notification.title so the clamp can't drift from the column a migration would change
(single source of truth). Add a db-lane test that actually commits a >255-char title into
Postgres (end-to-end proof the truncation is gone, not just the clamp logic) and one that
asserts the constant tracks the column width.
A downloaded FB/IG reel's "title" is the whole caption (up to ~1900 chars). It flowed
untrimmed into Notification.title (String(255)); at the batched download_gc commit that
raised psycopg StringDataRightTruncation in pass 1, aborting the whole run so expiry
deletion, LRU eviction and orphan reap never executed (13/110 prod assets already exceed
the limit). Clamp the title to 255 (with an ellipsis) centrally in create_notification —
defending all six callers — since the full title already reaches the client via `data`
and the column is only an English fallback. Add a regression test.
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.
peter