- The cut file is measured against what its owner has left, for a YouTube range (whose fetch never
reads yt-dlp's size ceiling) and for a Plex clip (which fetches nothing), before the fetch and
again before it is placed; a refusal leaves the asset pending for a Retry.
- A silent Plex file is cut silent; a file the probe cannot read says so.
- A second job on a failed or renamed clip gets the same YouTube diagnosis and the fetched title.
- The "too long" refusal names the server's cap; a contract test holds the client's copies of the
clip limits to clip.py; the shipped player clients are pinned against the installed yt-dlp.
- One HDR transfer set and one rating_key lookup; job status words from JobStatus; the display-name
guard sees subscript writes and the clip route; stale comments and types follow the clip job.
E2EE direct messages, the chat dock, the Messages module, the message
WebSocket and its realtime registry, and the messages / message_keys
tables (migration 0097; the downgrade restores the empty schema). Prod
held two welcome notes, no message between people and one key — never
used (owner decision, E34). WebSocket now has no owner module: the lint
bans it everywhere. Privacy policy updated: it no longer promises E2EE
for a feature that is gone.
The admin page's download_worker_concurrency and log_level were stored but never reached the worker:
it read the env once at boot (worker.py) and never looked again. The worker now re-reads both every
10 s (_sync_knobs): extra loop threads start at once, surplus ones retire after their current job, and
a changed level is applied to the process. An unreadable database keeps what already runs.
ruff gains RUF100, PLW1510 and flake8-bugbear, every hit fixed rather than
ignored. B008 stays on with FastAPI's Depends/Query declared immutable.
basedpyright moves to standard mode, pytest to --strict-markers, and a hand
run of pytest now requires the database unless SIFTLODE_TEST_REQUIRE_DB=0.
PLW1510 found a real bug: the three ffprobe probes in plex/stream.py never
looked at the exit status, so a probe that failed was read as the file's
truth (an HDR source sent down the HW-safe path, a multi-audio file collapsed
to one track, a subtitle offset taken from garbage) and the warning each
except branch promises never fired. They now run with check=True.
Deep-review round 2, on round 1's own fix commit. It found MORE than round 1
did (28 raw, against 13), and most of it was mine -- a new piece of UI attracts
findings a backend-only diff never does. The pattern in almost all of them is
the same: a principle applied to the site that prompted it and not to its
siblings.
* `display_text` reached three of the FIVE writers of a user-supplied name.
`POST /api/downloads` and `POST /api/downloads/edit` still handed Postgres an
unbounded string, so a 300-character name pasted into the Add-download dialog
or the editor was a bare 500 (main.py re-raises every DataError but the NUL
case). Four of the seven reviewers found it independently. The bound now lives
in `models.py`, the three columns are DECLARED with it, and the guard for the
next writer is parsed rather than remembered: every `display_name` /
`display_uploader` assignment under `app/routes/` must go through the helper.
That guard immediately found two more -- the Google sign-in callback writes
the profile name into the same 255-wide column -- so it is six, not five.
* The connection release reached one of the THREE routes that read their own
body. The screenshot route (10 MiB, from a phone) and the client log sink both
still held a pooled connection idle-in-transaction for the length of the
upload, which is the invariant db.py sizes the pool on. All three now call
`db.release_read_connection`, whose precondition -- nothing pending on the
shared session -- is CHECKED rather than described, because a whole-session
rollback discarding a caller's staged write is a mistake this repo has already
made once. `test_body_limit.py` holds the rule for the next such route.
* The AST drift registry keyed its sites by (module, limit expression), so a
second route copied from the first collapsed onto the same entry and went
uncovered -- the exact hole the registry exists to close. Keyed per call site
now, and it carries the release requirement too.
* The dialog's own refusal was announced once and never again: two over-cap
picks leave an identical node with identical text, and an untouched live
region says nothing. It is keyed per pick now, the drop zone reads as invalid
and points at the sentence, and the reason is reachable from the control
rather than only from a one-shot announcement.
* Two tests rebuilt the blind spot they were written to remove: the dialog's cap
test sat nowhere near the boundary (a `>` loosened to `>=` survived it, which
is the mutation round 1 fixed on the server side), and the header-precedence
test only covered an exact-case duplicate while a differently-cased one would
have been COMBINED onto the wire rather than replaced. Both fixed, and the
merge now dedupes case-insensitively because header names are.
Also: the upload dialog's unstubbed usage query was firing a real request from
jsdom in a neighbouring suite and leaking a delayed "connection lost" toast
across test boundaries; `GET /api/downloads/usage` had no test at all though the
whole client-side check hangs off it; and the comment claiming the edge 413 is
readable now says what was measured -- readable to 200 MiB over loopback, a bare
`onerror` through a proxy -- so the pre-send check is named as the mechanism and
that branch as the backstop it is.
Every test added or changed here was mutation-checked. One mutation SURVIVED and
is recorded rather than papered over: the `tooBig` term inside `submit()` cannot
be reached, because `ui/Button` swallows the activation itself, so it is marked
as belt and braces instead of being described as the guard.
- The download and edit failure lines go back after their DB write: a job the
write could not fail is not a failed job, and the worker's child_exit line
already says it failed.
- The download child's runner is the last stop before Python's hook: an
escaping exception is one download.child.failed line with its chain scrubbed,
then exit 1, instead of a raw stderr traceback carrying the URL.
- A crash the database will not record carries its stack once, and the
fail-write line has an event code.
- The job's own generic-extractor name is taken out as a literal (formats.hiding),
so brackets and colons in a direct link's file name no longer leak; the
pattern stays as the fallback. `Downloading just the video <id>` is covered.
- Tests: scrubbed_stack's cause, suppress, unprintable and text-format paths,
the image steps' failure kinds, the watchdog error through the real
_fail_running, exact poster/storyboard messages, ffmpeg skip marker.
- A failure keeps its traceback unless it is yt-dlp's or our own StatedFailure (a
message that is the whole story). A plain RuntimeError, RecursionError or OSError
is also what a bug of ours raises, and its stack is the only lead.
- The stack is scrubbed link by link (logs.scrubbed_stack): a chained DownloadError
under a DB error no longer prints yt-dlp's text unscrubbed.
- Engine hide_parameters: a DB error's message no longer carries the title or path
it was writing.
- The download and edit failure lines go out even when their DB write fails.
- The worker's child_exit/crash/supervisor lines say download.job.failed only when
the job failed there; a crash the DB will not record no longer ends the thread.
Stopping a paused child is download.child.stopped, not a second abort.
- log_safe also covers generic ids with spaces, playlist titles and bare ids.
- The ffmpeg image steps log the failure kind, not the argv with its path.
- The consent contract accepts only the body of a plain consented_trace() check,
and any level that may be TRACE.
- Tests for each; every one mutation-checked.
- yt-dlp's text is scrubbed wherever it is logged, not only in the adapter:
`formats.log_safe` takes out the URL, the `[extractor] <id>:` prefix and
file paths, and the job-failed, refused, edit-failed and backfill lines use
it. Done lines log the asset id, not the title-built path. The PII contract
now bans `rel`/`rel_path`/`title`.
- A traceback goes with a download/edit failure only when it is our own code
failing; for yt-dlp's DownloadError, our RuntimeErrors and OSErrors its foot
is the unscrubbed message.
- yt-dlp's `error` maps to WARNING: it fires on attempts the fallback client
set recovers from; the job's verdict is `download.job.failed`.
- The sink groups a bare `%s` pass-through by its text (numbers folded), and
a process's last batch closes the repeat window, so a short-lived download
child still reports what it dropped.
- Worker-side failures (stall, wall-clock, no asset, cached asset error) log
`download.job.failed` with a cause; they left no line at all before. The
crash, supervisor-error and non-zero-exit lines carry the code too.
- An edit failure is one coded line in `_fail_edit`, for every path.
- `logs.get_logger("__main__")` resolves to the `-m` module, so the worker
and the download child keep their names.
- TRACE lines carrying user content are gated on `logs.consented_trace()`,
not on the process level; a contract test pins it.
- The OAuth token refresh goes through `_upstream_failure`; transport errors
carry reason `transport`. Subscribe recognises a duplicate by reason code
only (the prose fallback was dead).
- Once-only guards for a stale sysconfig override and a refusing HLS session
directory; item_detail splits Plex failures from our own parse bugs.
- The playlist breaker line names the playlist and the action; comments that
were false (runner noqa, `_merge_vtt`, sink grouping) are corrected.
114 exception handlers neither logged nor re-raised. The defect was never that
they are silent — plenty of them should be — it is that their silence said
nothing, so nobody could tell a decision from an oversight. Each one has now been
read. 44 got a line; the other 70 carry `# silent-ok: <why>`.
What the silent ones had been hiding, in the cases that got a line:
- A failed ffprobe sent every HDR file down the 8-bit pipeline, so playback
degraded to a software transcode with nothing saying why; a failed audio probe
collapsed a multi-audio file to one track and the player showed no language
picker; a failed first-PTS probe left subtitles seconds out of sync.
- A subtitle sidecar that existed but would not parse made a PARTIAL track read as
complete, permanently — the viewer loses every cue past the cached window.
- An unreadable crop/trim/cut-list in an edit spec was dropped, so the clip came
out uncropped, untrimmed, or shorter than the one the user built.
- A dropped `max_height` downloaded at best quality against an explicit cap.
- A failed videos.list enrich made every candidate undroppable-from-judgement and
therefore dropped: a working search rendered as "no results".
- An unusable password hash answered as "wrong password", locking the owner out
and telling them they mistyped; a rotated TOKEN_ENCRYPTION_KEY turned every
stored token into "not connected".
- A stored admin setting that would not coerce fell back to the env default while
the settings page went on showing the saved value.
- A one-off InnerTube config failure pinned the whole process to the hard-coded
fallback key, and searches return nothing when that goes stale.
- The HLS sweep reported 0 swept — like a clean run — when it could not list its
own root, leaving every crashed session's segments on disk forever.
Four of them are counted rather than logged per item (dropped cues, dropped cut
list segments, unreclaimed GC files) and reported once per call, because the
alternative is one line per cue over a whole subtitle track.
ruff gains S110 and BLE001. Every blind `except Exception` that remains carries
`# noqa: BLE001 — <why>`, which is the point: catching everything is sometimes
right, and always worth a sentence. plex/sync.py's three best-effort backfill
catches now keep the traceback — a blind catch over a batch that fetches AND
writes could be Plex, the database or our own bug, and `%s` of the exception told
them apart badly. main.py's boot-time log-level read likewise.
logs.event's `code` is positional-only now: `event("x", code=...)` was a TypeError,
and `code` is exactly the word a caller reaches for when the thing that failed has
one of its own.
tests/test_swallowed_exceptions.py is the wider rule ruff cannot see: a handler
that neither logs nor re-raises must say why. Both cases mutation-checked — a
marker removed and a log line removed each fail it, naming the handler.
25 literal names covered 34 modules. plex/client.py, plex/stream.py,
plex/subtitles.py, plex/sync.py and two Plex route modules all called themselves
`siftlode.plex`; downloads/pipeline.py called itself `siftlode.worker`;
routes/channels.py called itself `siftlode.api`. The `logger` column is in every
stored row so E22.6's console can filter on it, and so an operator can pin a level
to one part of the system — neither works when six modules share a name.
`logs.get_logger(__name__)` derives it: app/plex/sync.py -> siftlode.plex.sync.
One name per module, and it cannot drift from the file it sits in.
tests/test_logger_names.py pins the derivation, that everything stays under the
one `siftlode` tree the handler and the tests attach to, and that no module
hand-rolls a name again unless the line says why (`# logger-name-ok: <why>`).
Two do: get_logger's own call, and `siftlode.http`, which is named for the traffic
it carries rather than the module it lives in — it replaced uvicorn's access log
and is what an operator pins when they want request lines and nothing else.
app/scheduler.py called its logger `logger`; every other module calls it `log`.
ruff gains LOG, G, T20, TRY400, TRY401 and S112. Four findings cleared with them:
worker.py logged `str(exc)[:120]` where the traceback was the whole question, and
three prints/f-strings that are deliberate now say so on the line. S110 and BLE001
stay off until S3 — 23 handlers are outstanding and each is a judgement about one
of them; enabling the rules first would just mean 23 blanket noqas.
The bind sat inside the try, so it was already reset when the crash line and the cleanup were logged — the one line most worth finding by job id had none.
The log now has a stored, queryable copy for the diagnostics console:
every WARNING and above plus the request line of every request, kept
for log_retention_days (default 7, no "forever") and pruned hourly by
the new log_gc job.
Rows are built on the logging thread from the same redacted fields as
the stdout line, queued without blocking and written in batches by one
background thread on its own connection. A full queue or a failed write
drops and reports instead of retrying. Flood control keeps 30 of any
one message per minute plus a count of the rest; anonymous request
lines are capped at 60 a minute per client, and the client address is
never stored. Deleting an account deletes its stored events.
Measured on prod first: a YouTube RSS outage wrote 11k warnings in four
hours, one per channel per poll. The poll now writes one warning with
per-reason counts and a few examples, and a failure on our side gets
one traceback per poll instead of one per channel.
A deployment printed four different log formats and none of them could be
joined up: uvicorn's log_config.json for the API, a hand-rolled StreamHandler in
main.py for anything not started by uvicorn, logging.basicConfig() in the worker
and in the download child (no timestamp at all), and alembic's fileConfig.
app/logs.py replaces all four with one JSON line per event (OpenTelemetry's log
data model, no SDK): ts, level, severity, source, logger, event code, message,
attributes, plus the correlation the context carries — request id, account,
download job.
Redaction is applied at the HANDLER, not at the call sites: every message, every
attribute and every traceback is scrubbed on the way out, so a secret cannot
leak by being forgotten at one of ~125 call sites. Passwords, tokens, cookies,
secret config values and email addresses never reach a line.
The access log is ours now (`http.request`): the route template instead of the
concrete path, the status, the duration and the account — and never the query
string, which uvicorn's access log printed on every single request (`?account=`,
`?source=`, the `?token=` of a share link). A well-formed client request id is
accepted and echoed in X-Request-Id, so a browser line and a server line join up.
The account comes from `request.state`, filled by the auth dependency: the
middleware is the outermost one, so it runs outside SessionMiddleware and in a
different task context.
`log_level` becomes an admin knob (sysconfig), applied without a restart and
re-applied at startup — best-effort on purpose, since a logging knob must never
be what stops the app from booting. Every compose service gets bounded json-file
logs (3×50 MB): Docker keeps every line forever by default, and the API now
writes one line per request.
Verified on the localdev stack: JSON everywhere including alembic and the
worker, the query strings gone, a supplied X-Request-Id echoed and logged, and
three /healthz hits of which only the one made while the level was DEBUG
produced a line.
Pause now re-reads the job under the same asset-then-job row lock as resume,
cancel, delete and redownload. Its unlocked write waited for a concurrent
cancel's lock and then set the released job back to paused, a holding status,
so a later delete released the same hold again and could free a file another
user's job still held (measured: the overlapping-request test ends 'paused'
instead of 'canceled' with the old read). The lock test now calls the route
handlers, so it also pins which helper each route uses.
Comments follow the code: resolve_url's contract names both verdicts and that a
failed page read is raised as itself; the inherited-failure diagnosis asks the
cookie gate as it stands when the job is claimed, so its advice fits the cookie
that job's Retry would use; the undecryptable-cookie warning no longer claims a
download goes ahead, since the worker parent reaches it only to diagnose.
A refused Facebook cookie on a group post is no longer "no video in this post":
measured on dev, Facebook answers with its login page (no post id at all), so the
adapter now raises its own POST_NOT_SHOWN verdict and the catalog explains it as
the refused cookie. The adapter owns both post verdicts and the "cannot parse
data" wall wording, so the Chrome retry and the catalog read one constant. The
group-post "no formats" entry is cookie-scoped (with a cookie yt-dlp only gets the
post's video link) and uses the adapter's own group-post test instead of a
"/groups/" substring.
Job transitions that move an asset's ref count (resume, cancel, delete,
redownload) re-read the job under a row lock, asset first, so two overlapping
requests can no longer release the same hold. get_or_create_asset loses an insert
race inside a SAVEPOINT instead of rolling back the caller's pending writes (Retry
lost its cleared error). Every worker-side job failure goes through
finalize_if_running and writes error_detail, so a cancel that landed first is kept
and a failure never shows an earlier one's copy. The worker asks the download
child's own cookie gate (credentials.usable_cookie) when a job inherits a failed
asset.
UI: the Facebook copy no longer points at a Retry button the failure window does
not have, says "paste" like Settings, and the HU wording is fixed
("Újraletöltés" on the re-download confirm). A category this client has no copy
for falls back to the generic sentence instead of a raw key.
Tests: route serialization of error_detail, the probed duration through a real
_download, the page-read cap and a failed read, a Retry off a pending asset, the
confirm-button labels, the lock and savepoint races.
The login-wall copy tells the user to add their cookies and press Retry, but
Retry kept the job on the shared asset the worker never attaches a cookie to,
so it failed on the same wall. Resume now moves a pending/errored job onto the
asset enqueue would pick with the cookie (never off a file or a download in
flight).
Re-download salted its fresh asset with the job's previous asset id, which the
cookie gate cannot recompute, so even a cookie download rebuilt without the
cookie. The rebuild salt is now the new asset's own id (plus the user's cookie
salt when one applies), and is_authed_asset recognises it.
A failed Facebook download read "Download failed": yt-dlp words the login wall
(18+ reel, closed group) as "Cannot parse data", which no category matched.
Measured on dev with the owner's cookie before writing a rule — the same text is
the login wall without a cookie and a refused cookie with one.
- errors.diagnose(msg, url, cookie_attached) tries a host-scoped catalog first
(all-of markers, cookie state, link fragment) and falls back to the category.
First entries: facebook.login_wall, facebook.cookie_rejected,
facebook.group_post_video. The module doc is the recipe for the next entry.
- download_jobs.error_detail (0084) + backfill of the stored Facebook parse
failures that were unknown.
- The pipeline's failure branch passes the link and whether the cookie went
with it; resume and redownload clear code and detail (redownload never
cleared error_code).
- A job meeting an already-failed asset got the text but no code, so it read
"Download failed" where the first job said why. It is diagnosed again, with
the cookie state from credentials.is_authed_asset (now shared with the
cookie gate); the redacted network text classifies back to network.
- FE: failCopyKeys gives the row and the failure window one lookup — catalog
copy, else the category. Copy in EN and HU; a contract test pins every
backend entry to both locales.
worker.py (~1090 L) mixed the PARENT supervisor (claim + supervise + the process
loop) with the CHILD pipeline (the yt-dlp/ffmpeg download+edit that runs in the
spawned subprocess). Extract them:
worker.py 1090 -> 475 parent: claim/supervise/loop
downloads/pipeline.py (new 592) the child pipeline (_download/_process_edit +
hooks/progress/ffmpeg helpers + _Aborted)
downloads/jobdb.py (new 73) the short-lived-session helpers both sides
share (_set_job/_job_status/_touch_job/
_requeue_if_running/_reset_asset_pending +
the staging-dir paths)
The graph is one-directional (runner -> pipeline -> jobdb; worker -> jobdb; no
worker<->pipeline edge) — the parent never calls the child directly, it spawns
runner.py as a subprocess. Consumers rewired: runner import, and two tests follow
the moved symbols (_media_info/_produced_file -> pipeline; the ffmpeg-callback
patches target pipeline where the names are looked up).
Pure code motion, verbatim bodies. ruff clean, full lane 363 pass (unchanged).
The supervisor kills a job that writes nothing for download_stall_timeout_seconds
(150s) and labels it a "network" stall. But the ffmpeg progress writer only wrote
on an integer-percent CHANGE, so a re-encode that sits on one percent for minutes
(AV1/VP9 -> H.264 or a crop on the CPU-only prod host) produced no row write and
was killed mid-encode as "stalled" — and the retry stalled the same way.
Fix: the writer now heartbeats (`_touch_job`, bumping updated_at without touching
the progress bar) once per interval even when the whole percent is unchanged, so
a live-but-quiet re-encode keeps a fresh updated_at. Reorders the rate-limit gate
first so a burst still costs one DB op — either a progress write or a heartbeat.
Pure-logic test with an injected clock: three stuck-at-10% intervals write no new
progress but heartbeat each time. Full lane green, ruff clean.
Two jobs on the same shared asset (same video+format queued twice, or by two
users) starved the worker pool: worker A downloads the asset; worker B claims the
sibling job, finds the asset `downloading`, requeues it, sleep(2)s, and re-claims
it on the next tick — burning a whole worker slot behind one in-flight asset for
the entire download (at concurrency=2, the queue stalls).
Fix: the claim SQL now excludes any job whose asset is already `downloading`
(`NOT EXISTS … media_assets.status='downloading'`), so a competing sibling simply
waits until the asset settles and the workers pick up unrelated jobs meanwhile.
The `_process_job` busy/lost-race branches drop their `time.sleep(2)` — the
exclusion keeps a requeued job out of the claim window, so there's no spin.
TDD: the new test failed on the old claim (claimed the sibling), passes now.
Full lane green, ruff clean.
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.
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.)
- 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: 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.
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).
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).
The R12 S3 underscore-publish left worker.uploader_url() sharing a name with the
`uploader_url` model attribute and the yt-dlp `"uploader_url"` info-dict key. The
function is now build_uploader_url() — the attribute, dict key, log line and
docstrings keep the plain `uploader_url` name. Rename touches only the def, its
two call sites, and the backfill import; no behaviour change (ruff clean,
pytest 193).
Layering cleanup, no behaviour change (ruff clean backend+scripts, pytest 193):
- Invert the scheduler->routes dependency: reset_demo_state (+ its demo-seed
helpers) move from routes/admin.py to a new service module app/demo.py.
Both routes/admin.py and scheduler.py now import it from app.demo, so the
scheduler no longer reaches into the routes layer — its lazy import (the
routes<->scheduler cycle workaround) is gone, hoisted to a top-level import.
- Publish the private symbols that were imported across module boundaries (the
underscore convention was lying): feed._serialize -> serialize,
feed._to_tsquery_str -> to_tsquery_str, videos._insert_stubs -> insert_stubs,
videos._stub_from_playlist_item -> stub_from_playlist_item,
worker._uploader_url -> uploader_url, plex._shared._item_or_404 ->
item_or_404. Scoped renames — the 5 other route modules keep their own truly
private _serialize helpers.
- _release_asset moves from routes/downloads.py to app/downloads/service.py as
the public release_asset (asset bookkeeping is a service concern); the routes
call it via service.release_asset.
- admin._decide: add an idempotency guard (409 unless status was 'pending') so a
repeat/stale approve OR deny can't re-run its side effects — now that deny emails,
this stops duplicate denial mails and a rejection mail to an already-approved user.
- SavedViewsWidget: extract runViewMutation(fn, failMsg) (returns success) — dedups
the five try/catch+refresh blocks AND lets create/rename close their input only on
success, so a failed create keeps the user's typed name.
- worker._notify_download_done: take user_id/title captured BEFORE the caller's commit
(no post-commit refresh SELECT); add a so the inbox labels an edit job
'Edit ready' instead of 'Download ready' (new inbox.editDone i18n, HU+EN).
Gate green (ruff/tsc/eslint-0/prettier/knip/vitest 79/pytest 193); smoke-tested the
saved-views + notifications surfaces render clean on a fresh dev server.
Give equal-weight actions equal-weight feedback (R11).
S1 Plex error toasts: the watched-toggles (card / season mark-all / episode /
info-panel / show mark-all) used to fail silently on 403/404/429; each now surfaces
the server reason via a shared notifyActionError() + resyncs the optimistic flip.
The playback auto-mark-watched stays intentionally silent (a mid-play toast is
noise; the next sync corrects it). New generic lib/actionErrors.ts (Plex + saved
views reuse it; notifyYouTubeActionError delegates its non-403 case to it).
S2 download-done inbox notification: worker.py emits a best-effort type=download_done
notice to the job owner on real completion (regular + edit jobs, not instant
cache-hits), rendered/localized in the inbox with an Open-Downloads link.
S3 optimistic rollbacks: SavedViewsWidget create/rename/toggle-default/remove/reorder
and the inbox revert-state no longer swallow failures — they toast + refetch to
resync (the reorder's optimistic order reverts to server truth).
S4 access-request deny: deny_invite now emails the requester (send_access_denied),
mirroring approve — no more silent limbo.
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.
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).
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.
- 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
- 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)
- redownload now ALWAYS rebuilds into a fresh private asset instead of ever reusing a shared
cached file: the cache sig is salted with job_id + current asset_id, so the fork is distinct
from both the dedup pool and the job's own current asset. This resolves the co-held silent
no-op (finding #1) — a shared asset is left intact for its other holders while THIS job
re-fetches its own copy. Resolving the fork BEFORE releasing the old hold (and the unique
salted key) also removes the IntegrityError-rollback-after-file-delete window (finding #2).
- format_sig gains an optional `salt` for that fork identity.
- titles: don't consume the space after `|`, so a "stats | #hashtags" remainder keeps the
leading whitespace _TRAILING_HASHTAGS needs — and fall back to the prefix+hashtag-stripped
text, so neither the stats nor the SEO hashtags are resurrected when a title empties (#3).
- worker: early-fill asset.title falls back to the video id so a title that cleaned to empty
doesn't show a blank card while downloading (#5).
- Tests for the salt fork and the hashtag-remainder case.
(Finding #4 — quota on re-download — intentionally not changed: a re-download reuses the
existing job and re-fetches ~the same bytes, so calling check_enqueue would wrongly reject on
the job-count cap while footprint is unchanged.)
- redownload: never clobber a co-held (deduped) asset. Drop the force-reset-to-pending;
_release_asset already deletes+recreates a solo asset (genuine re-fetch), while a shared
ready asset is left intact and the re-queued job re-points to it via _finish_from_cache —
so one user's re-download can't delete another's file mid-serve (finding #1).
- redownload: reset queue_pos to the tail so a re-download doesn't front-run newer queued
jobs on the worker's `ORDER BY queue_pos` claim (finding #3). Made service.next_queue_pos public.
- formats: video-only mp4 selector prefers every video-only option before any progressive
`best`, so a "Video only" download stays audio-free whenever a video-only stream exists (#2).
- titles: a title that is only the engagement prefix no longer resurrects the stripped stats
(return the stripped text / empty, not the raw) (#4).
- worker: probe the file's duration when yt-dlp gave none, so a long re-encode shows real
progress instead of a frozen 0% (#5); tighten the ensure_browser_playable docstring to its
actual mp4-profile scope (#6).
Mini-epic "Downloads correctness & UX" (S1–S3), on top of the iOS-AV1 + 720p fixes.
S1 — naming & titles:
- ASCII-fold on-disk dir/file names AND the served (Content-Disposition) filename: no
spaces (underscore-joined), no accents (á→a, ő→o, ß→ss), pure ASCII — portable and free
of the `?` mojibake accented names show in non-UTF-8 tools. (Forward-only; existing files
clean up via re-download.)
- Strip a leading social engagement prefix ("1.6M views · 66K reactions | …") from titles.
S2 — universal browser playability (generalizes the iOS re-encode):
- ensure_browser_playable guarantees the stored file is H.264 + AAC/MP3 in mp4 — the only
combo every browser decodes. Re-encode just the offending stream(s) (video for AV1/VP9/HEVC,
audio for Opus/etc), or remux when only the container is wrong. Gated on the mp4 compat
profile (an explicit vcodec / non-mp4 custom profile opts out).
S3 — force re-download (preserves share links):
- POST /downloads/{job_id}/redownload keeps the job row so its public DownloadLink survives
and resolves to the freshly-downloaded file; deletes the old file and re-fetches under the
current rules. A failed re-download leaves the job in error (the intended broken-link case).
- Library "Re-download" action (icon + confirm), api method, hu/en strings.
Tests: naming/title-strip (test_naming), browser_transcode_flags matrix; existing
download_filename expectation updated for the underscore policy.
Shared /watch links showed a "broken play button" on iPad (any iOS
browser — all forced onto WebKit) while playing fine on desktop Chrome.
Root cause: the stored mp4's video codec was AV1 (confirmed via ffprobe),
which WebKit can't decode. The existing H.264 guard was only a
format_sort *preference*, so a non-YouTube source (Facebook) that ships
AV1 as its best video-only stream slipped through.
- B1: for the mp4 compat profile, prefer H.264 at the SELECTION level
with a progressive fallback (bestvideo[vcodec^=avc1]+bestaudio /
best[vcodec^=avc1] / anything) so an avc1 progressive is chosen over
an AV1-only video-only stream.
- B2: re-encode the rare AV1/VP9-only result to H.264+AAC (+faststart)
in the worker before storing, gated on the compat profile; corrects
the asset codec metadata to match.
- Bump _SIG_VERSION 2->3 so cached AV1 assets re-derive.
- Default download preset is now 720p (migration 0058 re-seeds builtins
720p-first; ProfileEditor BLANK 1080->720).
- Tests for the selector, compat opt-outs, and the transcode predicate.
The asset-busy and claim-fail branches wrote status='queued' unconditionally, so a
pause/cancel the route had just written was clobbered back to queued and the job
silently revived. New _requeue_if_running does the requeue as a conditional UPDATE
(...WHERE status='running'), a no-op once the route has moved the job off 'running'.
download_layout and download_retention_days are registered ConfigSpec keys
(admin-editable on the Configuration page), but worker.py read them from
settings.* (env) at download/edit finalize time — so an admin's edit was a
silent no-op (files kept the old layout; TTL stayed at the env default). Read
them via sysconfig (_download_settings), which falls back to the env default
when there's no DB override. Same class as the Downloads B3 fix.
ruff clean, localdev boots, re-review clean.
peter