From 18c11ca437b4826b39b22f78e44462536f418fd1 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sun, 13 Sep 2026 21:53:25 +0200 Subject: [PATCH 01/10] =?UTF-8?q?feat(downloads):=20a=20per-site=20failure?= =?UTF-8?q?=20catalog=20=E2=80=94=20Facebook's=20walls=20say=20why=20and?= =?UTF-8?q?=20what=20to=20do?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../versions/0084_dl_job_error_detail.py | 41 ++++ backend/app/downloads/credentials.py | 10 +- backend/app/downloads/errors.py | 108 ++++++++- backend/app/downloads/pipeline.py | 11 +- backend/app/downloads/service.py | 3 + backend/app/models.py | 2 + backend/app/routes/downloads.py | 1 + backend/app/worker.py | 25 ++- backend/tests/test_download_errors.py | 65 ++++++ .../tests/test_download_failure_catalog.py | 206 ++++++++++++++++++ .../src/components/LibraryPlayer.dom.test.tsx | 1 + frontend/src/components/downloadModals.tsx | 5 +- .../src/components/downloadRow.dom.test.tsx | 1 + frontend/src/components/downloadRow.tsx | 3 +- frontend/src/i18n/backendIds.contract.test.ts | 20 ++ frontend/src/i18n/locales/en/downloads.json | 16 ++ frontend/src/i18n/locales/hu/downloads.json | 16 ++ frontend/src/lib/api/downloads.ts | 1 + frontend/src/lib/downloadFailure.test.ts | 43 ++++ frontend/src/lib/downloadFailure.ts | 17 ++ 20 files changed, 582 insertions(+), 13 deletions(-) create mode 100644 backend/alembic/versions/0084_dl_job_error_detail.py create mode 100644 backend/tests/test_download_failure_catalog.py create mode 100644 frontend/src/lib/downloadFailure.test.ts create mode 100644 frontend/src/lib/downloadFailure.ts diff --git a/backend/alembic/versions/0084_dl_job_error_detail.py b/backend/alembic/versions/0084_dl_job_error_detail.py new file mode 100644 index 00000000..148aabff --- /dev/null +++ b/backend/alembic/versions/0084_dl_job_error_detail.py @@ -0,0 +1,41 @@ +"""add download_jobs.error_detail — the domain-specific entry of the failure catalog (E11.5 S1) + +Revision ID: 0084_dl_job_error_detail +Revises: 0083_comfort_pins +Create Date: 2026-09-13 + +`error_code` is the generic category (it decides whether a Retry is offered). `error_detail` names +the host-specific catalog entry `downloads.errors.classify` matched on top of it — `facebook.login_wall` +and its siblings — so the UI can say what went wrong ON THAT SITE and what to do about it, instead of +the category's generic sentence. Nullable: most failures have no host-specific entry. + +Backfill: a job's code is written once, at failure time, so a new rule never reaches the rows that +failed before it. The one shape measured on prod (2026-09-13, jobs 9/66/75/145/156/157/182/256/257) is +yt-dlp's `[facebook] : Cannot parse data`, stored as `unknown`. Without a cookie that is Facebook's +login wall. Whether a cookie was attached at the time is not recorded anywhere, so every old row gets +the no-cookie entry — on prod no account had a Facebook cookie, so that is what they all were. +""" + +import sqlalchemy as sa +from alembic import op + +revision: str = "0084_dl_job_error_detail" +down_revision: str | None = "0083_comfort_pins" +branch_labels: str | None = None +depends_on: str | None = None + + +def upgrade() -> None: + op.add_column("download_jobs", sa.Column("error_detail", sa.String(length=48), nullable=True)) + op.execute( + sa.text( + "UPDATE download_jobs SET error_code = 'login_required', " + "error_detail = 'facebook.login_wall' " + "WHERE (error_code IS NULL OR error_code = 'unknown') " + "AND error ILIKE '%[facebook]%cannot parse data%'" + ) + ) + + +def downgrade() -> None: + op.drop_column("download_jobs", "error_detail") diff --git a/backend/app/downloads/credentials.py b/backend/app/downloads/credentials.py index af5b79fe..5594143c 100644 --- a/backend/app/downloads/credentials.py +++ b/backend/app/downloads/credentials.py @@ -104,6 +104,14 @@ def authed_format_sig(spec: dict, user_id: int, host_key: str) -> str: return formats.format_sig(spec, salt=cookie_salt(user_id, host_key)) +def is_authed_asset(asset_format_sig: str, spec: dict, user_id: int, url: str | None) -> bool: + """Whether an asset is `user_id`'s private, cookie-salted identity for `url`'s host — the one + kind of asset a cookie is ever attached to. The worker's cookie gate asks it before a download, + and the failure diagnosis asks it after one (a failure on this asset happened WITH the cookie).""" + host_key = host_key_for_url(url) + return host_key is not None and asset_format_sig == authed_format_sig(spec, user_id, host_key) + + def salt_for_enqueue(db: Session, user_id: int, url: str | None) -> str | None: """At enqueue: the cache salt to apply if this user has a cookie for `url`'s host, else None (a normal shared download). Returns the salt string so the caller passes it straight to @@ -224,7 +232,7 @@ def resolve_cookiefile( cred = get_credential(db, user_id, host_key) if cred is None: return None - if asset_format_sig != authed_format_sig(spec, user_id, host_key): + if not is_authed_asset(asset_format_sig, spec, user_id, url): return None # shared asset — never attach a cookie to a non-private identity raw = decrypt_optional(cred.cookies_enc) if not raw: diff --git a/backend/app/downloads/errors.py b/backend/app/downloads/errors.py index 1d4a7ccb..0d79b480 100644 --- a/backend/app/downloads/errors.py +++ b/backend/app/downloads/errors.py @@ -1,4 +1,5 @@ -"""Classify a raw yt-dlp / ffmpeg failure string into a stable category code. +"""Classify a raw yt-dlp / ffmpeg failure string into a stable category code — and, where a site's +failure has been measured, into that site's entry of the failure catalog. The worker stores the raw exception text (truncated) for technical detail, but a raw `ERROR: [youtube] : Private video. Sign in…` is noise to a user. `classify()` maps that text @@ -6,17 +7,36 @@ to one of a small set of category codes; the frontend turns the code into a loca explanation (and decides whether a retry makes sense). The raw text stays available behind a "details" disclosure. This is the same string-matching approach the worker already trusts for `_RETRIABLE_MARKERS` — kept here as the single source of truth for "what kind of failure is this". + +**The failure catalog** (`_CATALOG`, E11.5). A category is generic: "Sign-in required" cannot tell a +Facebook user that the cookie they already gave was refused, and yt-dlp words several different +Facebook walls identically (`Cannot parse data`). A catalog entry is scoped to ONE host and may also +depend on whether a cookie was attached and on the kind of link, and names a `detail` code the UI has +its own sentence for — why it failed on that site, and what the user can do. `diagnose()` tries the +catalog first and falls back to the category. How an entry is added, every time a failure is reported: + 1. reproduce it on dev (with and without the user's cookie) and keep the verbatim yt-dlp text; + 2. add the entry below, scoped as narrowly as the measurement allows (a marker that is yt-dlp's + generic wording for every site must never match without its host); + 3. pin the real text in `tests/test_download_errors.py`; + 4. add `downloads.errors.fail.catalog..{short,body}` in EN and HU (a contract test checks it; + the group is not called `detail`, which `actionErrors.contract` reads as a raw `HttpError.detail`). """ +from dataclasses import dataclass +from typing import NamedTuple +from urllib.parse import urlparse + +from app.downloads.credentials import host_key_for_url + # Two worker-authored strings that this module owns because both are coupled to `_RULES`. # # QUOTA_FULL_MESSAGE is raised by the worker and must classify back to `quota_exceeded`; its rule # below matches a fragment of it, and test_download_errors pins the pair, because a message and its # rule drifting apart is exactly how a category silently becomes unreachable. # -# NETWORK_REDACTED_MESSAGE is the opposite direction: it REPLACES a message after classification, so -# it deliberately has no rule. Nothing re-classifies a stored `job.error` today; if anything ever -# does, it must carry the original code rather than re-deriving one from this text. +# NETWORK_REDACTED_MESSAGE REPLACES a message after classification (`user_error_text`). It is written +# to the shared asset too, and a second job that inherits that failed asset re-diagnoses the stored +# text (`worker._process_job`), so it has a rule of its own that gives the category back. # # Other worker-raised texts are still inline literals at their raise sites. The ffmpeg ones and # "produced no output file" have markers here; the urlguard refusal deliberately has none and @@ -30,6 +50,8 @@ _RULES: tuple[tuple[str, str], ...] = ( # Our own refusal: the worker found the user already at their storage cap (SEC-13). Placed # first — it is our wording, so it can never be a false match on an upstream message. ("download storage is full", "quota_exceeded"), + # Our own redaction of a transport failure (see NETWORK_REDACTED_MESSAGE) — our wording too. + (NETWORK_REDACTED_MESSAGE.lower(), "network"), # Login / age / members / bot-check — the user must be signed in (we deliberately are not). ("sign in to confirm your age", "login_required"), ("confirm your age", "login_required"), @@ -101,6 +123,62 @@ _RULES: tuple[tuple[str, str], ...] = ( ) +class Diagnosis(NamedTuple): + """What a failure was: the generic `code` (category) and, when a catalog entry matched, its + host-specific `detail`.""" + + code: str + detail: str | None = None + + +@dataclass(frozen=True) +class _Entry: + """One measured failure of one site. It matches when the download URL belongs to `host` + (`credentials.SUPPORTED_HOSTS`), every one of `markers` is in the message, the cookie state is + `cookie` (None: either), and — when `link` is given — one of those fragments is in the URL path.""" + + host: str + markers: tuple[str, ...] + code: str + detail: str + cookie: bool | None = None + link: tuple[str, ...] = () + + +_CATALOG: tuple[_Entry, ...] = ( + # Facebook answers a signed-out client with a page yt-dlp cannot parse — an 18+ reel, a closed + # group's post (measured 2026-09-13: prod jobs 145, 156, 256 download once the owner's cookie is + # attached). "Cannot parse data" is yt-dlp's generic wording for a broken extractor on EVERY site, + # which is why the entry is scoped to Facebook and never becomes a general rule. + _Entry( + host="facebook", + markers=("[facebook]", "cannot parse data"), + code="login_required", + detail="facebook.login_wall", + cookie=False, + ), + # The same text WITH a cookie: an expired / signed-out cookie, or a page that also needs + # impersonation (prod job 257). Telling this user to add cookies would be wrong — they have. + _Entry( + host="facebook", + markers=("[facebook]", "cannot parse data"), + code="login_required", + detail="facebook.cookie_rejected", + cookie=True, + ), + # A group post whose video yt-dlp does not reach from the post's own link, although the video's + # own link downloads (prod job 66, with the cookie). Only for group links: a reel with no formats + # is a different failure. + _Entry( + host="facebook", + markers=("[facebook]", "no video formats found"), + code="format_unavailable", + detail="facebook.group_post_video", + link=("/groups/",), + ), +) + + def classify(msg: str | None) -> str: """Return a category code for a raw failure message (``"unknown"`` if nothing matches).""" if not msg: @@ -112,6 +190,28 @@ def classify(msg: str | None) -> str: return "unknown" +def diagnose( + msg: str | None, *, url: str | None = None, cookie_attached: bool = False +) -> Diagnosis: + """The catalog entry for this failure of this link, else just its category (see the module doc). + + `url` is the address yt-dlp was given; `cookie_attached` whether the acting user's cookie went + with it. Without a URL only the category can be known.""" + host = host_key_for_url(url) if msg and url else None + if host is not None: + low = msg.lower() # type: ignore[union-attr] — `host` is only set when `msg` is truthy + path = urlparse(url).path.lower() + for e in _CATALOG: + if ( + e.host == host + and all(m in low for m in e.markers) + and (e.cookie is None or e.cookie == cookie_attached) + and (not e.link or any(fragment in path for fragment in e.link)) + ): + return Diagnosis(e.code, e.detail) + return Diagnosis(classify(msg)) + + def user_error_text(source_kind: str, msg: str, code: str) -> str: """The failure text to STORE on the job, given the raw message and its category. diff --git a/backend/app/downloads/pipeline.py b/backend/app/downloads/pipeline.py index d1293eb6..a29dcc66 100644 --- a/backend/app/downloads/pipeline.py +++ b/backend/app/downloads/pipeline.py @@ -425,6 +425,7 @@ def _download( ) -> None: staging = _staging_dir(asset_id) cookiefile: Path | None = None + url: str | None = None # read by the failure branch, which can run before it is resolved try: if staging.exists(): shutil.rmtree(staging, ignore_errors=True) @@ -578,6 +579,7 @@ def _download( status=JobStatus.ERROR, error=msg, error_code=dlerrors.classify(msg), + error_detail=None, phase=None, ) db.commit() @@ -588,9 +590,11 @@ def _download( log.info("job %s refused for its owner: %s", job_id, msg) except Exception as exc: # noqa: BLE001 — surface any failure to the user, keep worker alive msg = str(exc)[:500] - code = dlerrors.classify(msg) + # The catalog needs the link and whether the user's cookie went with it: the same yt-dlp + # text is Facebook's login wall without one and a refused cookie with one. + diagnosis = dlerrors.diagnose(msg, url=url, cookie_attached=cookiefile is not None) # SEC-12: redact a transport-level readout for user-supplied URLs (see user_error_text). - msg = dlerrors.user_error_text(source_kind, msg, code) + msg = dlerrors.user_error_text(source_kind, msg, diagnosis.code) with SessionLocal() as db: asset = db.get(MediaAsset, asset_id) if asset: @@ -603,7 +607,8 @@ def _download( job_id, status=JobStatus.ERROR, error=msg, - error_code=code, + error_code=diagnosis.code, + error_detail=diagnosis.detail, phase=None, ) db.commit() diff --git a/backend/app/downloads/service.py b/backend/app/downloads/service.py index 5eaa81b7..8f1aa971 100644 --- a/backend/app/downloads/service.py +++ b/backend/app/downloads/service.py @@ -346,6 +346,7 @@ def resume(db: Session, job: DownloadJob) -> None: job.progress = 0 job.error = None job.error_code = None + job.error_detail = None if job.asset_id: asset = db.get(MediaAsset, job.asset_id) if asset and asset.status == "error": # MediaAsset.status (no enum), not a JobStatus @@ -389,6 +390,8 @@ def redownload(db: Session, job: DownloadJob) -> None: job.progress = 0 job.phase = None job.error = None + job.error_code = None # a rebuild that fails again gets its own diagnosis, never the old one + job.error_detail = None job.speed_bps = None job.eta_s = None # Re-derive the display name from the freshly cleaned title on completion (else an older, un-cleaned diff --git a/backend/app/models.py b/backend/app/models.py index 6ea02966..f29695d9 100644 --- a/backend/app/models.py +++ b/backend/app/models.py @@ -871,6 +871,8 @@ class DownloadJob(Base, TimestampMixin, UpdatedAtMixin): error_code: Mapped[str | None] = mapped_column( String(24) ) # category, see downloads.errors.classify + # The host-specific failure-catalog entry on top of the category (`facebook.login_wall`), or NULL. + error_detail: Mapped[str | None] = mapped_column(String(48)) # see downloads.errors.diagnose queue_pos: Mapped[int] = mapped_column(Integer, default=0, server_default="0") diff --git a/backend/app/routes/downloads.py b/backend/app/routes/downloads.py index 01a56f56..3ef241e3 100644 --- a/backend/app/routes/downloads.py +++ b/backend/app/routes/downloads.py @@ -107,6 +107,7 @@ def _serialize(job: DownloadJob, asset: MediaAsset | None) -> dict: "phase": job.phase, "error": job.error, "error_code": job.error_code, + "error_detail": job.error_detail, "queue_pos": job.queue_pos, "created_at": iso(job.created_at), "source_kind": job.source_kind, diff --git a/backend/app/worker.py b/backend/app/worker.py index cb617259..ebc0c1d1 100644 --- a/backend/app/worker.py +++ b/backend/app/worker.py @@ -32,7 +32,8 @@ from app import sysconfig from app.config import settings from app.db import SessionLocal from app.downloads import edit as editmod -from app.downloads import quota, service +from app.downloads import credentials, formats, quota, service +from app.downloads import errors as dlerrors from app.downloads.jobdb import ( _edit_staging_dir, _requeue_if_running, @@ -399,6 +400,19 @@ def _process_job(job_id: int) -> None: asset_id = asset.id if asset else None asset_status = asset.status if asset else None asset_error = asset.error if asset else None + if asset_status == "error": + # A job that meets an already-failed asset gets the SAME explanation the job that failed + # it got — not a bare "Download failed". Only the text is on the asset, so it is + # diagnosed again, for this link, with the cookie state of that asset: a user's private + # (cookie-salted) asset failed with their cookie attached, the shared one without. + url = service.source_url(job.source_kind, job.source_ref) + diagnosis = dlerrors.diagnose( + asset_error, + url=url, + cookie_attached=credentials.is_authed_asset( + asset.format_sig, formats.normalize(job.profile_snapshot), job.user_id, url + ), + ) if asset is None: _set_job(job_id, status="error", error="No media asset", phase=None) @@ -408,7 +422,14 @@ def _process_job(job_id: int) -> None: _finish_from_cache(job_id, asset_id) return if asset_status == "error": - _set_job(job_id, status="error", error=asset_error or "Download failed", phase=None) + _set_job( + job_id, + status="error", + error=asset_error or "Download failed", + error_code=diagnosis.code, + error_detail=diagnosis.detail, + phase=None, + ) return if asset_status == "downloading": # Another worker won this asset between our claim and here (a narrow race — the claim SQL diff --git a/backend/tests/test_download_errors.py b/backend/tests/test_download_errors.py index 680d2d13..b467dee6 100644 --- a/backend/tests/test_download_errors.py +++ b/backend/tests/test_download_errors.py @@ -64,6 +64,71 @@ def test_our_own_worker_refusals_classify_as_themselves(): assert classify(dlerrors.QUOTA_FULL_MESSAGE) == "quota_exceeded" +def test_the_redacted_network_text_classifies_back_to_network(): + # The redacted text is also what a failed asset keeps, and a second job that meets that asset is + # diagnosed from it — without its own rule it would read "Download failed". + assert classify(dlerrors.NETWORK_REDACTED_MESSAGE) == "network" + + +# Verbatim yt-dlp 2026.07.04 output from prod jobs, measured on dev 2026-09-13 (E11.5). +_FB_PARSE = ( + "ERROR: [facebook] 1736360027626346: Cannot parse data; please report this issue on " + "https://github.com/yt-dlp/yt-dlp/issues?q= , filling out the appropriate issue template." +) +_FB_NO_FORMATS = ( + "ERROR: [facebook] 1755988225525547: No video formats found!; please report this issue on " + "https://github.com/yt-dlp/yt-dlp/issues?q= , filling out the appropriate issue template." +) +_REEL = "https://www.facebook.com/reel/1736360027626346/?fs=e&fs=e" +_GROUP_POST = "https://www.facebook.com/groups/983554479435596/permalink/1755988225525547/?" + + +def test_facebook_cannot_parse_data_is_the_login_wall_without_a_cookie(): + for url in ( + _REEL, + _GROUP_POST, + "https://fb.watch/Ia_iJYI2Vw/?fs=e", + "https://m.facebook.com/x", + ): + assert dlerrors.diagnose(_FB_PARSE, url=url) == ("login_required", "facebook.login_wall") + + +def test_facebook_cannot_parse_data_with_a_cookie_is_the_cookie_refused(): + # Telling a user who already gave a cookie to add one is the failure this entry exists to avoid. + assert dlerrors.diagnose(_FB_PARSE, url=_REEL, cookie_attached=True) == ( + "login_required", + "facebook.cookie_rejected", + ) + + +def test_a_facebook_group_post_without_formats_names_the_link(): + assert dlerrors.diagnose(_FB_NO_FORMATS, url=_GROUP_POST, cookie_attached=True) == ( + "format_unavailable", + "facebook.group_post_video", + ) + # A reel with no formats is another failure: only the category. + assert dlerrors.diagnose(_FB_NO_FORMATS, url=_REEL, cookie_attached=True) == ( + "format_unavailable", + None, + ) + + +def test_a_catalog_entry_never_leaves_its_host(): + # "Cannot parse data" is yt-dlp's wording for a broken extractor on every site. + other = _FB_PARSE.replace("[facebook]", "[vimeo]") + assert dlerrors.diagnose(other, url="https://vimeo.com/1") == ("unknown", None) + # The Facebook text for a link that is not Facebook's (a look-alike host) is not the Facebook wall. + assert dlerrors.diagnose(_FB_PARSE, url="https://facebook.com.evil/reel/1") == ("unknown", None) + # No link known: the category only. + assert dlerrors.diagnose(_FB_PARSE) == ("unknown", None) + + +def test_diagnose_falls_back_to_the_category(): + msg = "ERROR: [facebook] 123: You must be logged in to see this post" + assert dlerrors.diagnose(msg, url=_REEL) == ("login_required", None) + assert dlerrors.diagnose(None, url=_REEL) == ("unknown", None) + + def test_a_url_jobs_network_failure_is_redacted_but_youtube_keeps_its_text(): # SEC-12: the raw transport text for a user-chosen host:port is a port-scanner readout, so it is # replaced for `url` jobs. Everything else must survive verbatim, or a real diagnosis is lost. diff --git a/backend/tests/test_download_failure_catalog.py b/backend/tests/test_download_failure_catalog.py new file mode 100644 index 00000000..9ae95199 --- /dev/null +++ b/backend/tests/test_download_failure_catalog.py @@ -0,0 +1,206 @@ +"""DB-backed tests for the download failure catalog (E11.5 S1) at the places a failure is WRITTEN. + +`test_download_errors` pins which entry a message matches; these pin that every writer hands the +matcher what it needs — the link and whether the user's cookie went with it — and that a stored entry +never outlives the failure it describes: +- the download child's failure branch (`pipeline._download`); +- a second job meeting an already-failed asset (`worker._process_job`), which only has the text; +- Retry / Re-download, which requeue the job; +- the `0084` backfill of the rows that failed before the catalog existed. +""" + +from alembic import command +from alembic.config import Config +from sqlalchemy import text + +from app.config import settings +from app.db import engine +from app.downloads import credentials, formats, pipeline, service +from app.models import DownloadJob, DownloadQuota, MediaAsset, User +from app.worker import _process_job + +_REEL = "https://www.facebook.com/reel/1736360027626346/?fs=e&fs=e" +# Verbatim yt-dlp 2026.07.04, prod job 256. +_FB_PARSE = ( + "ERROR: [facebook] 1736360027626346: Cannot parse data; please report this issue on " + "https://github.com/yt-dlp/yt-dlp/issues?q= , filling out the appropriate issue template." +) + + +def _user(db, email): + u = User(email=email) + db.add(u) + db.flush() + db.add( + DownloadQuota( + user_id=u.id, max_bytes=10**12, max_concurrent=5, max_jobs=10**6, unlimited=True + ) + ) + return u + + +def _asset_and_job(db, user, *, sig, asset_status, job_status, error=None): + a = MediaAsset( + source_kind="url", + source_ref=_REEL, + format_sig=sig, + status=asset_status, + error=error, + ref_count=1, + ) + db.add(a) + db.flush() + j = DownloadJob( + user_id=user.id, + asset_id=a.id, + source_kind="url", + source_ref=_REEL, + status=job_status, + queue_pos=0, + profile_snapshot={}, + ) + db.add(j) + db.commit() + return a, j + + +def _fail_the_child(monkeypatch, tmp_path, *, cookie: bool): + monkeypatch.setattr(settings, "download_root", str(tmp_path)) + monkeypatch.setattr(pipeline.urlguard, "check_url", lambda url: None) + cookiefile = tmp_path / "cookies.txt" + cookiefile.write_text("# Netscape HTTP Cookie File\n") + monkeypatch.setattr( + pipeline.credentials, + "resolve_cookiefile", + lambda *a, **k: cookiefile if cookie else None, + ) + + def extract(*a, **k): + raise RuntimeError(_FB_PARSE) + + monkeypatch.setattr(pipeline, "_extract_with_fallback", extract) + + +def test_the_download_child_diagnoses_with_the_link_and_no_cookie(db, monkeypatch, tmp_path): + u = _user(db, "fb-nocookie@example.com") + a, j = _asset_and_job(db, u, sig="shared", asset_status="downloading", job_status="running") + _fail_the_child(monkeypatch, tmp_path, cookie=False) + + pipeline._download(j.id, a.id, "url", _REEL, {}, u.id) + + db.expire_all() + job = db.get(DownloadJob, j.id) + assert (job.status, job.error_code, job.error_detail) == ( + "error", + "login_required", + "facebook.login_wall", + ) + + +def test_the_download_child_knows_the_cookie_went_with_it(db, monkeypatch, tmp_path): + # The same text with the user's cookie attached: telling them to add cookies would be wrong. + u = _user(db, "fb-cookie@example.com") + a, j = _asset_and_job(db, u, sig="private", asset_status="downloading", job_status="running") + _fail_the_child(monkeypatch, tmp_path, cookie=True) + + pipeline._download(j.id, a.id, "url", _REEL, {}, u.id) + + db.expire_all() + job = db.get(DownloadJob, j.id) + assert (job.error_code, job.error_detail) == ("login_required", "facebook.cookie_rejected") + + +def test_a_job_meeting_a_failed_shared_asset_gets_the_same_explanation(db): + # Before the catalog this path wrote the text and NO code: "Download failed" for the second + # job, "Facebook sign-in required" for the first. + u = _user(db, "fb-second@example.com") + _, j = _asset_and_job( + db, u, sig="shared", asset_status="error", job_status="running", error=_FB_PARSE + ) + + _process_job(j.id) + + db.expire_all() + job = db.get(DownloadJob, j.id) + assert (job.status, job.error_code, job.error_detail) == ( + "error", + "login_required", + "facebook.login_wall", + ) + + +def test_a_job_meeting_its_users_failed_private_asset_knows_the_cookie_was_used(db): + u = _user(db, "fb-private@example.com") + sig = credentials.authed_format_sig(formats.normalize({}), u.id, "facebook") + _, j = _asset_and_job( + db, u, sig=sig, asset_status="error", job_status="running", error=_FB_PARSE + ) + + _process_job(j.id) + + db.expire_all() + job = db.get(DownloadJob, j.id) + assert (job.error_code, job.error_detail) == ("login_required", "facebook.cookie_rejected") + + +def test_retry_and_redownload_drop_the_old_explanation(db): + u = _user(db, "fb-requeue@example.com") + for requeue in (service.resume, service.redownload): + _, j = _asset_and_job( + db, u, sig=f"s-{requeue.__name__}", asset_status="error", job_status="error" + ) + j.error, j.error_code, j.error_detail = _FB_PARSE, "login_required", "facebook.login_wall" + db.commit() + + requeue(db, j) + + db.refresh(j) + assert (j.status, j.error_code, j.error_detail) == ("queued", None, None), requeue.__name__ + + +def test_0084_backfills_only_the_facebook_parse_failures_it_measured(db): + cfg = Config("alembic.ini") + command.downgrade(cfg, "0083_comfort_pins") + upgraded = False + try: + with engine.begin() as conn: + uid = conn.execute( + text("INSERT INTO users (email) VALUES ('backfill@example.com') RETURNING id") + ).scalar_one() + rows = { + "fb_unknown": ("error", "unknown", _FB_PARSE), + "fb_null": ("canceled", None, _FB_PARSE), + "other_site": ("error", "unknown", _FB_PARSE.replace("[facebook]", "[vimeo]")), + "fb_other_code": ("error", "network", _FB_PARSE), + } + ids = {} + for name, (status, code, error) in rows.items(): + ids[name] = conn.execute( + text( + "INSERT INTO download_jobs (user_id, source_kind, source_ref, " + "profile_snapshot, status, error_code, error) " + "VALUES (:u, 'url', :ref, '{}', :s, :c, :e) RETURNING id" + ), + {"u": uid, "ref": _REEL, "s": status, "c": code, "e": error}, + ).scalar_one() + command.upgrade(cfg, "head") + upgraded = True + with engine.connect() as conn: + got = { + name: tuple( + conn.execute( + text("SELECT error_code, error_detail FROM download_jobs WHERE id = :i"), + {"i": i}, + ).one() + ) + for name, i in ids.items() + } + assert got == { + "fb_unknown": ("login_required", "facebook.login_wall"), + "fb_null": ("login_required", "facebook.login_wall"), + "other_site": ("unknown", None), + "fb_other_code": ("network", None), + } + finally: + if not upgraded: + command.upgrade(cfg, "head") diff --git a/frontend/src/components/LibraryPlayer.dom.test.tsx b/frontend/src/components/LibraryPlayer.dom.test.tsx index aaa05ede..38e119b6 100644 --- a/frontend/src/components/LibraryPlayer.dom.test.tsx +++ b/frontend/src/components/LibraryPlayer.dom.test.tsx @@ -24,6 +24,7 @@ const baseJob: DownloadJob = { phase: null, error: null, error_code: null, + error_detail: null, queue_pos: 0, created_at: null, source_kind: "youtube", diff --git a/frontend/src/components/downloadModals.tsx b/frontend/src/components/downloadModals.tsx index 7c54a069..0e618af8 100644 --- a/frontend/src/components/downloadModals.tsx +++ b/frontend/src/components/downloadModals.tsx @@ -8,6 +8,7 @@ import { api, type DownloadJob } from "../lib/api"; import { inputCls } from "./ui/form"; import { Button } from "./ui/Button"; import { IconBtn } from "./downloadRow"; +import { failCopyKeys } from "../lib/downloadFailure"; // Per-item user modals split out of DownloadCenter (self-contained, prop-driven): rename/metadata // editing and the failure-detail window. @@ -142,8 +143,8 @@ export function ErrorModal({ return (
-
{t(`downloads.errors.fail.cat.${code}.short`)}
-

{t(`downloads.errors.fail.cat.${code}.body`)}

+
{t(failCopyKeys(job, "short"))}
+

{t(failCopyKeys(job, "body"))}

{job.error && (
) : null} diff --git a/frontend/src/i18n/backendIds.contract.test.ts b/frontend/src/i18n/backendIds.contract.test.ts index 68d1ddef..3b2eb699 100644 --- a/frontend/src/i18n/backendIds.contract.test.ts +++ b/frontend/src/i18n/backendIds.contract.test.ts @@ -67,6 +67,13 @@ const adminRefusals = [ ...backend("routes/admin.py").matchAll(/\{\s*"code":\s*"(\w+)",\s*"reason":\s*"(\w+)"/g), ].filter((m) => adminCodes.includes(m[1]!)); const adminReasons = [...new Set(adminRefusals.map((m) => m[2]!))]; +// The download failure catalog's host-specific entries → `downloads.errors.fail.catalog.`. +// A code holds a dot (`facebook.login_wall`), so the reverse check strips the prefix and the part +// rather than splitting on dots. +const failDetails = [ + ...new Set([...backend("downloads/errors.py").matchAll(/detail="([\w.]+)"/g)].map((m) => m[1]!)), +]; +const DETAIL = "downloads.errors.fail.catalog."; /** The ids under `prefix` a locale names, `prefix.` or `prefix..`. */ const idsUnder = (keys: Set, prefix: string) => @@ -83,6 +90,7 @@ describe("backend ids have their names in both languages", () => { expect(auditActions).toEqual(expect.arrayContaining(["config_set", "audit_clear"])); expect(quotaActions.length).toBeGreaterThan(0); expect(adminCodes).toEqual(expect.arrayContaining(["activate", "setpw"])); + expect(failDetails).toEqual(expect.arrayContaining(["facebook.login_wall"])); // Every code the frontend localizes is raised somewhere the scan can see. expect(adminCodes.filter((c) => !adminRefusals.some((m) => m[1] === c))).toEqual([]); }); @@ -117,6 +125,18 @@ describe("backend ids have their names in both languages", () => { expect(quotaActions.filter((a) => !idsUnder(keys, "quotaActions").has(a))).toEqual([]); }); + it(`${lang}: every download failure-catalog entry`, () => { + expect( + missing(failDetails.flatMap((d) => [`${DETAIL}${d}.short`, `${DETAIL}${d}.body`])), + ).toEqual([]); + const named = new Set( + [...keys] + .filter((k) => k.startsWith(DETAIL)) + .map((k) => k.slice(DETAIL.length).replace(/\.(short|body)$/, "")), + ); + expect([...named].filter((d) => !failDetails.includes(d))).toEqual([]); + }); + it(`${lang}: every admin refusal reason`, () => { expect(missing(adminReasons.map((r) => `errors.admin.${r}`))).toEqual([]); expect([...idsUnder(keys, "errors.admin")].filter((r) => !adminReasons.includes(r))).toEqual( diff --git a/frontend/src/i18n/locales/en/downloads.json b/frontend/src/i18n/locales/en/downloads.json index e0cc6b04..1c21a6c4 100644 --- a/frontend/src/i18n/locales/en/downloads.json +++ b/frontend/src/i18n/locales/en/downloads.json @@ -287,6 +287,22 @@ "short": "Storage full", "body": "This download was stopped because your storage quota is already used up. Delete something from your library, then try again." } + }, + "catalog": { + "facebook": { + "login_wall": { + "short": "Facebook sign-in required", + "body": "Facebook only shows this to signed-in users (e.g. 18+ content or a closed group). Add your Facebook cookies under Settings → Account → Download authentication, then press Retry." + }, + "cookie_rejected": { + "short": "Facebook didn't accept the cookies", + "body": "The cookies have probably expired, or you signed out in that browser. Export them again, upload them, and retry. If it still fails, the content has been removed or isn't visible to you." + }, + "group_post_video": { + "short": "The video isn't reachable from the post link", + "body": "This is the link of a group post, not of the video. Open the video on Facebook and paste its link." + } + } } }, "cookies": { diff --git a/frontend/src/i18n/locales/hu/downloads.json b/frontend/src/i18n/locales/hu/downloads.json index a396d465..c1883b52 100644 --- a/frontend/src/i18n/locales/hu/downloads.json +++ b/frontend/src/i18n/locales/hu/downloads.json @@ -287,6 +287,22 @@ "short": "Betelt a tárhely", "body": "Ez a letöltés leállt, mert a tárhelykereted már elfogyott. Törölj valamit a könyvtáradból, aztán próbáld újra." } + }, + "catalog": { + "facebook": { + "login_wall": { + "short": "Facebook-bejelentkezés szükséges", + "body": "A Facebook ezt csak bejelentkezve mutatja (pl. 18+ tartalom vagy zárt csoport). Add meg a Facebook-cookie-kat a Beállítások → Fiók → Letöltés-hitelesítés alatt, majd nyomd meg az Újra gombot." + }, + "cookie_rejected": { + "short": "A Facebook nem fogadta el a cookie-t", + "body": "A cookie valószínűleg lejárt, vagy kijelentkeztél abban a böngészőben. Exportáld újra, töltsd fel, és próbáld újra. Ha így sem megy, a tartalom törölt, vagy számodra nem látható." + }, + "group_post_video": { + "short": "A videó nem érhető el a poszt linkjéről", + "body": "Ez a csoportposzt linkje, nem a videóé. Nyisd meg a videót a Facebookon, és annak a linkjét másold be." + } + } } }, "cookies": { diff --git a/frontend/src/lib/api/downloads.ts b/frontend/src/lib/api/downloads.ts index ed8ca53c..1e07e265 100644 --- a/frontend/src/lib/api/downloads.ts +++ b/frontend/src/lib/api/downloads.ts @@ -76,6 +76,7 @@ export interface DownloadJob { phase: string | null; error: string | null; // raw yt-dlp/ffmpeg text (technical detail) error_code: string | null; // failure category, see backend downloads.errors.classify + error_detail: string | null; // host-specific failure-catalog entry, see downloads.errors.diagnose queue_pos: number; created_at: string | null; source_kind: string; diff --git a/frontend/src/lib/downloadFailure.test.ts b/frontend/src/lib/downloadFailure.test.ts new file mode 100644 index 00000000..ed290cef --- /dev/null +++ b/frontend/src/lib/downloadFailure.test.ts @@ -0,0 +1,43 @@ +// @vitest-environment jsdom +// +// A failed download's explanation: the failure-catalog entry first, its category as the fallback +// (E11.5 S1). The second case runs the REAL i18next with the real locales, because the fallback is +// i18next's list-key lookup, not code of ours — an older client meeting a detail it has no copy for +// must still say the category, not print a key. +import { describe, expect, it } from "vitest"; +import i18n from "../i18n"; +import { failCopyKeys } from "./downloadFailure"; + +describe("failCopyKeys", () => { + it("puts a catalog entry before its category, and is the category alone without one", () => { + expect( + failCopyKeys({ error_code: "login_required", error_detail: "facebook.login_wall" }, "body"), + ).toEqual([ + "downloads.errors.fail.catalog.facebook.login_wall.body", + "downloads.errors.fail.cat.login_required.body", + ]); + expect(failCopyKeys({ error_code: "network", error_detail: null }, "short")).toEqual([ + "downloads.errors.fail.cat.network.short", + ]); + expect(failCopyKeys({ error_code: null, error_detail: null }, "short")).toEqual([ + "downloads.errors.fail.cat.unknown.short", + ]); + }); + + it("reads the entry's copy, and the category's for an entry this client does not know", async () => { + await i18n.changeLanguage("en"); + expect( + i18n.t( + failCopyKeys( + { error_code: "login_required", error_detail: "facebook.login_wall" }, + "short", + ), + ), + ).toBe("Facebook sign-in required"); + expect( + i18n.t( + failCopyKeys({ error_code: "login_required", error_detail: "facebook.newer" }, "short"), + ), + ).toBe("Sign-in required"); + }); +}); diff --git a/frontend/src/lib/downloadFailure.ts b/frontend/src/lib/downloadFailure.ts new file mode 100644 index 00000000..58c77fd1 --- /dev/null +++ b/frontend/src/lib/downloadFailure.ts @@ -0,0 +1,17 @@ +import type { DownloadJob } from "./api"; + +// The i18n keys that explain a failed download, most specific first — the list form `t()` takes, so +// the first key a locale has wins. A failure-catalog entry (`error_detail`, e.g. `facebook.login_wall`) +// names what went wrong on that site and what to do; the category (`error_code`) is the generic +// sentence, and the fallback when an older client meets a detail it has no copy for. One helper for +// the queue row's headline and the failure window, so the two can never explain the same job +// differently. +export function failCopyKeys( + job: Pick, + part: "short" | "body", +): string[] { + const category = `downloads.errors.fail.cat.${job.error_code || "unknown"}.${part}`; + return job.error_detail + ? [`downloads.errors.fail.catalog.${job.error_detail}.${part}`, category] + : [category]; +} From 98ec1ffcabdc7b9b16461ebdedd83d1b6c4d7046 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sun, 13 Sep 2026 22:31:49 +0200 Subject: [PATCH 02/10] fix(downloads): Retry and Re-download take a cookie added after the failure 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. --- backend/app/downloads/credentials.py | 35 +++-- backend/app/downloads/pipeline.py | 9 +- backend/app/downloads/service.py | 61 ++++++++- backend/app/routes/downloads.py | 5 +- backend/app/worker.py | 2 +- backend/tests/test_download_credentials.py | 145 ++++++++++++++++++++- 6 files changed, 229 insertions(+), 28 deletions(-) diff --git a/backend/app/downloads/credentials.py b/backend/app/downloads/credentials.py index 5594143c..917ad96e 100644 --- a/backend/app/downloads/credentials.py +++ b/backend/app/downloads/credentials.py @@ -1,10 +1,12 @@ """Per-user, per-host download cookies — the single source of truth for "does the acting user's cookie apply to this download, and how does that keep the result private". -Two call sites depend on ONE consistent answer (house rule: an invariant lives in shared code, not +Two sides depend on ONE consistent answer (house rule: an invariant lives in shared code, not in prose): * `enqueue` salts the MediaAsset cache signature with the user id WHEN a cookie applies, so the - authed download becomes a PRIVATE asset that is never cache-served to another user. + authed download becomes a PRIVATE asset that is never cache-served to another user. Retry + (`service.resume`) and Re-download (`service.redownload`) ask the same question again, so a + cookie the user added after a failure goes with the next attempt. * the worker attaches the cookie file to yt-dlp ONLY when a cookie still applies AND the asset it is downloading into is that private, salted identity — so a cookie can never poison a shared asset (the load-bearing cross-user privacy fix). @@ -30,7 +32,7 @@ from sqlalchemy.orm import Session from app.downloads import formats from app.downloads.adapters import _host_is -from app.models import DownloadCredential +from app.models import DownloadCredential, MediaAsset from app.security import decrypt_optional, encrypt log = logging.getLogger("siftlode.downloads.credentials") @@ -104,12 +106,27 @@ def authed_format_sig(spec: dict, user_id: int, host_key: str) -> str: return formats.format_sig(spec, salt=cookie_salt(user_id, host_key)) -def is_authed_asset(asset_format_sig: str, spec: dict, user_id: int, url: str | None) -> bool: +def rebuild_salt(asset_id: int, cookie: str | None) -> str: + """The salt of a Re-download's fresh asset: its OWN row id (so every rebuild is a new identity no + enqueue can ever compute), prefixed with the user's `cookie_salt` when a cookie applies. The id is + what makes it recognisable — the worker has the asset row, so `is_authed_asset` can recompute this + one, where a salt built from the job's previous asset id could not be.""" + return f"{cookie}:rebuild:{asset_id}" if cookie else f"rebuild:{asset_id}" + + +def is_authed_asset(asset: MediaAsset, spec: dict, user_id: int, url: str | None) -> bool: """Whether an asset is `user_id`'s private, cookie-salted identity for `url`'s host — the one - kind of asset a cookie is ever attached to. The worker's cookie gate asks it before a download, + kind of asset a cookie is ever attached to: the one enqueue creates (`authed_format_sig`), or a + Re-download's rebuild of it (`rebuild_salt`). The worker's cookie gate asks it before a download, and the failure diagnosis asks it after one (a failure on this asset happened WITH the cookie).""" host_key = host_key_for_url(url) - return host_key is not None and asset_format_sig == authed_format_sig(spec, user_id, host_key) + if host_key is None: + return False + if asset.format_sig == authed_format_sig(spec, user_id, host_key): + return True + return asset.id is not None and asset.format_sig == formats.format_sig( + spec, salt=rebuild_salt(asset.id, cookie_salt(user_id, host_key)) + ) def salt_for_enqueue(db: Session, user_id: int, url: str | None) -> str | None: @@ -211,7 +228,7 @@ def resolve_cookiefile( db: Session, user_id: int, url: str | None, - asset_format_sig: str, + asset: MediaAsset, spec: dict, staging: Path, ) -> Path | None: @@ -220,7 +237,7 @@ def resolve_cookiefile( The double gate that prevents cross-user leakage: 1. a credential must still exist for `url`'s host (host-scoped via `host_key_for_url`), AND 2. the asset being downloaded into must BE this user's private, salted identity - (`asset_format_sig == authed_format_sig(...)`). + (`is_authed_asset`). If (2) fails the asset is the SHARED (unsalted) one — attaching the cookie would poison the global cache with a private-group video, so we attach nothing. An unreadable blob (rotated key → @@ -232,7 +249,7 @@ def resolve_cookiefile( cred = get_credential(db, user_id, host_key) if cred is None: return None - if not is_authed_asset(asset_format_sig, spec, user_id, url): + if not is_authed_asset(asset, spec, user_id, url): return None # shared asset — never attach a cookie to a non-private identity raw = decrypt_optional(cred.cookies_enc) if not raw: diff --git a/backend/app/downloads/pipeline.py b/backend/app/downloads/pipeline.py index a29dcc66..e683913f 100644 --- a/backend/app/downloads/pipeline.py +++ b/backend/app/downloads/pipeline.py @@ -8,7 +8,6 @@ from datetime import date, datetime, timedelta, timezone from pathlib import Path import yt_dlp -from sqlalchemy import select from app import sysconfig from app.config import settings @@ -446,12 +445,10 @@ def _download( # this asset is that user's private, salted identity (the double gate lives in credentials). remaining_bytes: int | None = None with SessionLocal() as cdb: - asset_sig = cdb.execute( - select(MediaAsset.format_sig).where(MediaAsset.id == asset_id) - ).scalar_one_or_none() - if asset_sig is not None: + target = cdb.get(MediaAsset, asset_id) + if target is not None: cookiefile = credentials.resolve_cookiefile( - cdb, user_id, url, asset_sig, spec, staging + cdb, user_id, url, target, spec, staging ) # SEC-13: what this user has left. `check_enqueue` only compared the footprint as it # stood when the job was QUEUED — a download's size is unknown until it finishes, so a diff --git a/backend/app/downloads/service.py b/backend/app/downloads/service.py index 8f1aa971..77ff9fd2 100644 --- a/backend/app/downloads/service.py +++ b/backend/app/downloads/service.py @@ -7,6 +7,7 @@ handles everything else. State transitions used by the routes (pause/resume/canc the worker live in `worker.py` / the routes; this module owns enqueue + asset bookkeeping. """ +import uuid from datetime import datetime, timezone from sqlalchemy import func, select, update @@ -341,7 +342,11 @@ def pause(db: Session, job: DownloadJob) -> None: def resume(db: Session, job: DownloadJob) -> None: """Requeue a paused/errored job for a fresh attempt, clearing its error. If the SHARED asset itself failed, reset it to pending so the worker actually re-downloads — otherwise the requeued job would - instantly re-inherit the asset's stale error (the worker short-circuits an errored asset).""" + instantly re-inherit the asset's stale error (the worker short-circuits an errored asset). + + A cookie the user added AFTER the job was queued moves the job onto their private asset first + (`_follow_the_cookie`): the worker only ever attaches a cookie to that identity, so without the move + the Retry a login-wall message asks for would fail on the same wall.""" if job.status in (JobStatus.PAUSED, JobStatus.ERROR): job.progress = 0 job.error = None @@ -349,6 +354,8 @@ def resume(db: Session, job: DownloadJob) -> None: job.error_detail = None if job.asset_id: asset = db.get(MediaAsset, job.asset_id) + if asset is not None: + asset = _follow_the_cookie(db, job, asset) if asset and asset.status == "error": # MediaAsset.status (no enum), not a JobStatus asset.status = "pending" asset.error = None @@ -356,6 +363,32 @@ def resume(db: Session, job: DownloadJob) -> None: db.commit() +def _follow_the_cookie(db: Session, job: DownloadJob, asset: MediaAsset) -> MediaAsset: + """Re-point a requeued job at the asset enqueue would pick NOW, when the user has a cookie for the + link's host but the job still holds an asset the worker won't attach it to (queued before the + cookie existed). Returns the asset the job holds afterwards. + + Only a `pending` / `error` asset moves — neither has a file nor a download in flight. A `ready` one + already has the file, and a `downloading` one may be this job's own child still being stopped by + the pause; the next Retry after it settles moves the job then. Removing a cookie never moves a job + back: the worker attaches nothing without a credential, so the private asset is as good as any.""" + if asset.status not in ("pending", "error"): + return asset + url = source_url(job.source_kind, job.source_ref) + salt = credentials.salt_for_enqueue(db, job.user_id, url) + spec = formats.normalize(job.profile_snapshot) + if salt is None or credentials.is_authed_asset(asset, spec, job.user_id, url): + return asset + target = get_or_create_asset( + db, job.source_kind, job.source_ref, formats.format_sig(spec, salt=salt) + ) + populate_from_catalog(db, target) + release_asset(db, job) # drop the old hold BEFORE re-pointing — it keys off job.asset_id + incr_ref_count(db, target.id) + job.asset_id = target.id + return target + + def cancel(db: Session, job: DownloadJob) -> None: """Cancel a job unless it's already done/canceled, releasing its asset hold WHILE it still counts as holding — `canceled` ∉ HOLDING, so a later delete won't double-release.""" @@ -374,13 +407,31 @@ def delete(db: Session, job: DownloadJob) -> None: def redownload(db: Session, job: DownloadJob) -> None: """Rebuild a finished/failed job's file into a FRESH, private asset (the cache sig is salted with - the job id + current asset id, so it never reuses the dedup pool) and requeue the SAME job row — + the new asset's own id, so it never reuses the dedup pool) and requeue the SAME job row — so any public share link (DownloadLink -> job_id) keeps resolving to the newly-downloaded file. The old hold is dropped first (freeing the old file iff this job was its last holder). The caller - has already rejected edit clips + still-in-progress jobs (HTTP 409).""" + has already rejected edit clips + still-in-progress jobs (HTTP 409). + + When the user has a cookie for the link's host, the salt carries it (`credentials.rebuild_salt`), + so the worker recognises the rebuild as their private identity and attaches the cookie — a cookie + download, or one the user added after a failure, rebuilds with the cookie.""" spec = formats.normalize(job.profile_snapshot) - sig = formats.format_sig(spec, salt=f"{job.id}:{job.asset_id}") - asset = get_or_create_asset(db, job.source_kind, job.source_ref, sig) + cookie = credentials.salt_for_enqueue( + db, job.user_id, source_url(job.source_kind, job.source_ref) + ) + # The salt names the row's own id, which exists only once the row does: insert it under a unique + # placeholder, then write the real signature in the same transaction (nobody sees the placeholder). + asset = MediaAsset( + source_kind=job.source_kind, + source_ref=job.source_ref, + format_sig=f"rebuild:{uuid.uuid4().hex}", + status="pending", + ref_count=0, + ) + db.add(asset) + db.flush() + asset.format_sig = formats.format_sig(spec, salt=credentials.rebuild_salt(asset.id, cookie)) + db.flush() populate_from_catalog(db, asset) # Drop the old hold BEFORE re-pointing the job — release_asset keys off the job's current asset. release_asset(db, job) diff --git a/backend/app/routes/downloads.py b/backend/app/routes/downloads.py index 3ef241e3..0ec65afa 100644 --- a/backend/app/routes/downloads.py +++ b/backend/app/routes/downloads.py @@ -630,8 +630,9 @@ def redownload_download( resolves to the newly-downloaded file — a re-download never breaks a link you already sent out (only a *failed* re-download leaves the job in `error`, which is the intended broken-link case). - The rebuild always re-fetches into a FRESH, private asset (the cache sig is salted with the job - id + current asset id) rather than the dedup pool. That means it (a) always genuinely rebuilds + The rebuild always re-fetches into a FRESH, private asset (the cache sig is salted with the new + asset's own id, and the user's cookie salt when one applies — `credentials.rebuild_salt`) rather + than the dedup pool. That means it (a) always genuinely rebuilds under the current format/naming rules — never silently reuses a shared cached file — and (b) never deletes a file another job/user still shares: the old asset is only removed if THIS job was its last holder.""" diff --git a/backend/app/worker.py b/backend/app/worker.py index ebc0c1d1..24138a11 100644 --- a/backend/app/worker.py +++ b/backend/app/worker.py @@ -410,7 +410,7 @@ def _process_job(job_id: int) -> None: asset_error, url=url, cookie_attached=credentials.is_authed_asset( - asset.format_sig, formats.normalize(job.profile_snapshot), job.user_id, url + asset, formats.normalize(job.profile_snapshot), job.user_id, url ), ) diff --git a/backend/tests/test_download_credentials.py b/backend/tests/test_download_credentials.py index 48e4f42c..b5bc2ab6 100644 --- a/backend/tests/test_download_credentials.py +++ b/backend/tests/test_download_credentials.py @@ -18,7 +18,7 @@ from cryptography.fernet import Fernet from app import security from app.downloads import credentials, formats, service -from app.models import DownloadCredential, MediaAsset, User +from app.models import DownloadCredential, DownloadJob, MediaAsset, User @pytest.fixture(autouse=True) @@ -189,7 +189,9 @@ class TestResolveCookiefile: db.commit() spec = formats.normalize({}) salted = credentials.authed_format_sig(spec, u.id, "facebook") - path = credentials.resolve_cookiefile(db, u.id, FB_URL, salted, spec, tmp_path) + path = credentials.resolve_cookiefile( + db, u.id, FB_URL, self._asset_sig(salted), spec, tmp_path + ) assert path is not None and Path(path).read_text() == COOKIES def test_never_attaches_to_a_shared_asset(self, db, tmp_path): @@ -200,7 +202,12 @@ class TestResolveCookiefile: db.commit() spec = formats.normalize({}) shared = formats.format_sig(spec) - assert credentials.resolve_cookiefile(db, u.id, FB_URL, shared, spec, tmp_path) is None + assert ( + credentials.resolve_cookiefile( + db, u.id, FB_URL, self._asset_sig(shared), spec, tmp_path + ) + is None + ) def test_non_matching_host_gets_no_cookie(self, db, tmp_path): u = _user(db) @@ -210,13 +217,141 @@ class TestResolveCookiefile: # A YouTube URL has no supported host_key -> no cookie regardless of what's stored. yt = "https://www.youtube.com/watch?v=abc" salted = credentials.authed_format_sig(spec, u.id, "facebook") - assert credentials.resolve_cookiefile(db, u.id, yt, salted, spec, tmp_path) is None + assert ( + credentials.resolve_cookiefile(db, u.id, yt, self._asset_sig(salted), spec, tmp_path) + is None + ) def test_no_credential_gets_no_cookie(self, db, tmp_path): u = _user(db) spec = formats.normalize({}) salted = credentials.authed_format_sig(spec, u.id, "facebook") - assert credentials.resolve_cookiefile(db, u.id, FB_URL, salted, spec, tmp_path) is None + assert ( + credentials.resolve_cookiefile( + db, u.id, FB_URL, self._asset_sig(salted), spec, tmp_path + ) + is None + ) + + +# --- Retry / Re-download take a cookie added later (DB) ----------------------------------------- +# Measured on dev (E11.5): a Facebook reel failed on the login wall, the user added their cookie as +# the failure copy tells them to, pressed Retry — and it failed again, because the job still held the +# shared asset the worker never attaches a cookie to. Re-download forked a salt the gate could not +# recognise, so even a job downloaded WITH a cookie rebuilt without it. + + +def _held_job(db, u, *, sig, asset_status="error", job_status="error"): + a = MediaAsset( + source_kind="url", source_ref=FB_URL, format_sig=sig, status=asset_status, ref_count=1 + ) + db.add(a) + db.flush() + j = DownloadJob( + user_id=u.id, + asset_id=a.id, + source_kind="url", + source_ref=FB_URL, + status=job_status, + queue_pos=0, + profile_snapshot={}, + ) + db.add(j) + db.commit() + return a, j + + +class TestRequeueTakesTheCookie: + spec = formats.normalize({}) + + def test_retry_after_adding_a_cookie_moves_the_job_to_the_private_asset(self, db, tmp_path): + u = _user(db) + shared, j = _held_job(db, u, sig=formats.format_sig(self.spec)) + credentials.upsert_credential(db, u.id, "facebook", COOKIES) + db.commit() + + service.resume(db, j) + + db.expire_all() + job = db.get(DownloadJob, j.id) + target = db.get(MediaAsset, job.asset_id) + assert job.status == "queued" and target.id != shared.id + assert target.format_sig == credentials.authed_format_sig(self.spec, u.id, "facebook") + assert (target.status, target.ref_count) == ("pending", 1) + assert db.get(MediaAsset, shared.id).ref_count == 0 # the old hold went with the move + assert credentials.resolve_cookiefile(db, u.id, FB_URL, target, self.spec, tmp_path) + + def test_retry_without_a_cookie_keeps_the_shared_asset(self, db): + u = _user(db) + shared, j = _held_job(db, u, sig=formats.format_sig(self.spec)) + + service.resume(db, j) + + db.expire_all() + assert db.get(DownloadJob, j.id).asset_id == shared.id + assert db.get(MediaAsset, shared.id).ref_count == 1 + + @pytest.mark.parametrize( + ("asset_status", "job_status"), [("ready", "error"), ("downloading", "paused")] + ) + def test_retry_never_moves_off_a_file_or_a_download_in_flight( + self, db, asset_status, job_status + ): + u = _user(db) + shared, j = _held_job( + db, + u, + sig=formats.format_sig(self.spec), + asset_status=asset_status, + job_status=job_status, + ) + credentials.upsert_credential(db, u.id, "facebook", COOKIES) + db.commit() + + service.resume(db, j) + + db.expire_all() + assert db.get(DownloadJob, j.id).asset_id == shared.id + + def test_redownload_with_a_cookie_rebuilds_into_an_asset_the_cookie_goes_with( + self, db, tmp_path + ): + u = _user(db) + other = _user(db, "other@example.com") + credentials.upsert_credential(db, u.id, "facebook", COOKIES) + credentials.upsert_credential(db, other.id, "facebook", COOKIES) + authed = credentials.authed_format_sig(self.spec, u.id, "facebook") + first, j = _held_job(db, u, sig=authed, asset_status="ready", job_status="done") + + seen = {first.id} + for _ in range(2): # a rebuild of a rebuild is still recognised + service.redownload(db, j) + db.expire_all() + job = db.get(DownloadJob, j.id) + target = db.get(MediaAsset, job.asset_id) + assert target.id not in seen and target.format_sig != authed + seen.add(target.id) + assert credentials.resolve_cookiefile(db, u.id, FB_URL, target, self.spec, tmp_path) + # Private: another user's cookie is never attached to it. + assert not credentials.is_authed_asset(target, self.spec, other.id, FB_URL) + + # A Retry of a failed rebuild keeps the rebuild — it is already the cookie's identity. + target.status, job.status = "error", "error" + db.commit() + service.resume(db, job) + db.expire_all() + assert db.get(DownloadJob, j.id).asset_id == target.id + + def test_a_rebuild_made_without_a_cookie_is_no_cookie_identity(self, db, tmp_path): + u = _user(db) + _, j = _held_job(db, u, sig=formats.format_sig(self.spec)) + service.redownload(db, j) + credentials.upsert_credential(db, u.id, "facebook", COOKIES) + db.commit() + + db.expire_all() + target = db.get(MediaAsset, db.get(DownloadJob, j.id).asset_id) + assert not credentials.is_authed_asset(target, self.spec, u.id, FB_URL) # --- API: write-only, never echoes the blob (DB) ---------------------------------------------- From 1354f5b503796a77ce74bfcad244bf6941b80154 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sun, 13 Sep 2026 22:48:21 +0200 Subject: [PATCH 03/10] feat(downloads): a Facebook cookie download stuck at "Cannot parse data" is retried as Chrome MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Measured on dev: with the user's cookie some reels still answer "Cannot parse data" and only extract when the request impersonates Chrome. Impersonation is a second attempt behind that one error, only when a cookie is attached — links that already download with the cookie keep their plain path, and without a cookie the same error is the login wall. curl_cffi is pinned inside yt-dlp's supported range (0.10-0.15; outside it every target silently reports "unavailable"); the lock only gains the new packages. A test fails the gate if the locked pair can no longer impersonate. --- backend/app/downloads/adapters.py | 29 +++++- backend/app/downloads/pipeline.py | 5 +- backend/requirements.lock | 5 ++ backend/requirements.txt | 6 ++ backend/tests/test_adapters.py | 11 ++- backend/tests/test_facebook_impersonation.py | 92 ++++++++++++++++++++ 6 files changed, 141 insertions(+), 7 deletions(-) create mode 100644 backend/tests/test_facebook_impersonation.py diff --git a/backend/app/downloads/adapters.py b/backend/app/downloads/adapters.py index 30c85919..86242787 100644 --- a/backend/app/downloads/adapters.py +++ b/backend/app/downloads/adapters.py @@ -13,6 +13,8 @@ Resolution works both before extraction (from source_kind / URL host) and after from urllib.parse import urlparse +from yt_dlp.networking.impersonate import ImpersonateTarget + from app.config import settings @@ -22,9 +24,10 @@ class SourceAdapter: key = "generic" # --- extraction (worker._extract_with_fallback / backfill) --------------------------------- - def client_sets(self) -> list: - """Player-client sets to attempt, in order. A generic source needs no client override, so a - single default attempt (`[None]`) — the per-client fallback is a YouTube-only concept.""" + def client_sets(self, authed: bool = False) -> list: + """Attempts to make, in order — each entry is handed back to `ydl_opts_extra`, and the next one + is tried when `retriable` says so. A generic source makes a single default attempt (`[None]`). + `authed` is whether the acting user's cookie file goes with the download.""" return [None] def ydl_opts_extra(self, clients) -> dict: @@ -67,7 +70,7 @@ class YouTubeAdapter(SourceAdapter): "Sign in to confirm", ) - def client_sets(self) -> list: + def client_sets(self, authed: bool = False) -> list: return [ c for c in (settings.player_client_list, settings.player_client_fallback_list) if c ] or [None] @@ -93,6 +96,24 @@ class YouTubeAdapter(SourceAdapter): class FacebookAdapter(SourceAdapter): key = "facebook" + # Measured (E11.5, yt-dlp 2026.07.04): with the user's cookie, some reels answer a plain request + # with "Cannot parse data" and parse only when the request looks like Chrome. Impersonation is a + # SECOND attempt behind that one error, not the default: the links that already download with the + # cookie keep their proven path, and only the ones stuck at the wall go through curl_cffi. Without + # a cookie the same error is the login wall, which no disguise gets past. + _IMPERSONATE = "impersonate" + _WALL = "cannot parse data" + + def client_sets(self, authed: bool = False) -> list: + return [None, self._IMPERSONATE] if authed else [None] + + def ydl_opts_extra(self, clients) -> dict: + if clients == self._IMPERSONATE: + return {"impersonate": ImpersonateTarget.from_str("chrome")} + return {} + + def retriable(self, error_text: str) -> bool: + return self._WALL in error_text.lower() def uploader_url(self, info: dict) -> str | None: # Facebook gives neither channel_url nor uploader_url, but exposes a numeric `uploader_id` diff --git a/backend/app/downloads/pipeline.py b/backend/app/downloads/pipeline.py index e683913f..23251899 100644 --- a/backend/app/downloads/pipeline.py +++ b/backend/app/downloads/pipeline.py @@ -322,13 +322,14 @@ def _extract_with_fallback( ): """Download via yt-dlp using the source `adapter`'s client sets + options. For YouTube the adapter supplies the primary player-client set then a POT-backed fallback (retried on a bot/DRM/format - error); other sources make a single attempt. Returns the info dict of the successful attempt. + error); a Facebook download WITH a cookie is retried as Chrome behind "Cannot parse data"; other + sources make a single attempt. Returns the info dict of the successful attempt. `cookiefile` (S6) is the acting user's per-host cookies.txt, already gated by `credentials.resolve_cookiefile`; passed to every attempt so the fallback stays authenticated. `max_bytes` (SEC-13) is the owner's remaining storage budget, applied to every attempt.""" - client_sets = adapter.client_sets() + client_sets = adapter.client_sets(authed=cookiefile is not None) outtmpl = str(staging / "%(id)s.%(ext)s") last_exc: Exception | None = None for i, clients in enumerate(client_sets): diff --git a/backend/requirements.lock b/backend/requirements.lock index f1dd08e0..737de225 100644 --- a/backend/requirements.lock +++ b/backend/requirements.lock @@ -7,6 +7,7 @@ Authlib==1.7.2 Mako==1.4.1 MarkupSafe==3.0.3 PyYAML==6.0.3 +Pygments==2.21.0 SQLAlchemy==2.0.52 alembic==1.19.1 annotated-doc==0.0.5 @@ -21,6 +22,7 @@ cffi==2.1.1 charset-normalizer==3.5.1 click==8.4.2 cryptography==50.0.0 +curl_cffi==0.15.0 fastapi==0.141.1 feedparser-sgmllib==2.1.0 feedparser==6.0.14 @@ -32,6 +34,8 @@ httpx==0.28.1 idna==3.18 itsdangerous==2.2.0 joserfc==1.7.4 +markdown-it-py==4.2.0 +mdurl==0.1.2 mutagen==1.48.1 numpy==2.5.2 psycopg-binary==3.3.4 @@ -45,6 +49,7 @@ pydantic_core==2.46.4 python-dotenv==1.2.3 python-multipart==0.0.32 requests==2.34.2 +rich==15.0.0 starlette==1.6.0 typing-inspection==0.4.4 typing_extensions==4.16.0 diff --git a/backend/requirements.txt b/backend/requirements.txt index d801c0fd..63f9a24b 100644 --- a/backend/requirements.txt +++ b/backend/requirements.txt @@ -21,6 +21,12 @@ argon2-cffi>=23.1,<26 # scripts Deno runs for YouTube's n-signature). yt-dlp is release-often (YouTube changes), so # keep the floor loose and bump it when extraction breaks. yt-dlp[default]>=2025.5.22 +# Browser impersonation (`impersonate`) for Facebook downloads made with the user's cookie: some +# links answer a signed-in plain request with "Cannot parse data" and only parse as Chrome (E11.5, +# measured). The range is YT-DLP's, not ours — yt-dlp 2026.07.04 imports curl_cffi only in +# 0.10.x–0.15.x and silently reports every target "unavailable" outside it. Move it when bumping +# yt-dlp; `test_adapters` fails the gate if the locked pair can no longer impersonate. +curl_cffi>=0.10,<0.16 # PO Token provider plugin: talks to the bgutil sidecar (brainicism/bgutil-ytdlp-pot-provider) # to mint YouTube Proof-of-Origin tokens on demand, bypassing "confirm you're not a bot" and # unlocking high-quality formats. Pin to match the sidecar image tag (versions must align). diff --git a/backend/tests/test_adapters.py b/backend/tests/test_adapters.py index 15da7d5b..d5909a44 100644 --- a/backend/tests/test_adapters.py +++ b/backend/tests/test_adapters.py @@ -103,12 +103,21 @@ def test_generic_source_url_is_passthrough_youtube_reconstructs(): ) -def test_client_sets_and_retriable_are_youtube_only(): +def test_client_sets_and_retriable_are_per_source(): # Generic sources make a single attempt and never retry on the YouTube per-client markers. assert SourceAdapter().client_sets() == [None] + assert SourceAdapter().client_sets(authed=True) == [None] assert SourceAdapter().retriable("Sign in to confirm you're not a bot") is False assert YouTubeAdapter().retriable("Sign in to confirm you're not a bot") is True assert YouTubeAdapter().retriable("some unrelated network blip") is False + # Facebook: a second, Chrome-impersonating attempt only WITH the user's cookie, and only behind + # the parse wall (without a cookie that wall is the login wall — no disguise gets past it). + fb = FacebookAdapter() + assert fb.client_sets() == [None] + assert len(fb.client_sets(authed=True)) == 2 + assert fb.ydl_opts_extra(fb.client_sets(authed=True)[0]) == {} + assert fb.retriable("ERROR: [facebook] 1: Cannot parse data; please report this issue") is True + assert fb.retriable("ERROR: [facebook] 1: No video formats found!") is False def test_youtube_ydl_opts_extra_carries_player_client_and_pot(): diff --git a/backend/tests/test_facebook_impersonation.py b/backend/tests/test_facebook_impersonation.py new file mode 100644 index 00000000..04f1ec1c --- /dev/null +++ b/backend/tests/test_facebook_impersonation.py @@ -0,0 +1,92 @@ +"""E11.5 S3 — a Facebook cookie download stuck at "Cannot parse data" is retried as Chrome. + +Measured on dev (yt-dlp 2026.07.04, the owner's cookie): reel 806379462330494 fails with the cookie +alone and extracts 2560p with `--impersonate chrome`; the reels that already download with the cookie +keep their plain first attempt. These pin the attempt order through the real download loop +(`pipeline._extract_with_fallback`, with yt-dlp faked at the `YoutubeDL` boundary), and that the +LOCKED curl_cffi can actually impersonate — the pair only works inside yt-dlp's own version range, and +outside it every target silently reports "unavailable". +""" + +import pytest +import yt_dlp +from yt_dlp.networking.impersonate import ImpersonateTarget + +from app.downloads import adapters, pipeline + +_URL = "https://www.facebook.com/reel/806379462330494/?fs=e&fs=e" +_WALL = ( + "ERROR: [facebook] 806379462330494: Cannot parse data; please report this issue on " + "https://github.com/yt-dlp/yt-dlp/issues?q= , filling out the appropriate issue template." +) +_NO_FORMATS = "ERROR: [facebook] 2541212983053999: No video formats found!" + + +def test_the_locked_curl_cffi_can_impersonate_chrome(): + # YoutubeDL refuses to start when the target is unavailable — the same check a download makes. + with yt_dlp.YoutubeDL({"impersonate": ImpersonateTarget.from_str("chrome"), "quiet": True}): + pass + + +@pytest.fixture +def ydl(monkeypatch): + """Fake `yt_dlp.YoutubeDL`: records each attempt's options and plays back `outcomes` in order.""" + seen: list[dict] = [] + outcomes: list = [] + + class FakeYDL: + def __init__(self, opts): + seen.append(opts) + + def __enter__(self): + return self + + def __exit__(self, *exc): + return False + + def extract_info(self, url, download): + outcome = outcomes.pop(0) + if isinstance(outcome, str): + raise yt_dlp.utils.DownloadError(outcome) + return outcome + + monkeypatch.setattr(pipeline.yt_dlp, "YoutubeDL", FakeYDL) + monkeypatch.setattr(pipeline, "_touch_job", lambda job_id: None) + monkeypatch.setattr(pipeline, "_set_job", lambda job_id, **fields: None) + return seen, outcomes + + +def _run(tmp_path, cookiefile): + return pipeline._extract_with_fallback( + 1, 1, _URL, {}, tmp_path, adapters.FacebookAdapter(), cookiefile=cookiefile + ) + + +def test_a_cookie_download_at_the_wall_is_retried_as_chrome(ydl, tmp_path): + seen, outcomes = ydl + outcomes += [_WALL, {"id": "806379462330494"}] + + assert _run(tmp_path, "/cookies.txt") == {"id": "806379462330494"} + + assert [o.get("impersonate") for o in seen] == [None, ImpersonateTarget.from_str("chrome")] + assert all(o["cookiefile"] == "/cookies.txt" for o in seen) # the retry stays signed in + + +def test_without_a_cookie_the_wall_is_the_login_wall_and_final(ydl, tmp_path): + seen, outcomes = ydl + outcomes += [_WALL] + + with pytest.raises(yt_dlp.utils.DownloadError): + _run(tmp_path, None) + + assert len(seen) == 1 and "impersonate" not in seen[0] + + +def test_other_facebook_failures_are_not_retried_as_chrome(ydl, tmp_path): + seen, outcomes = ydl + outcomes += [_NO_FORMATS] + + with pytest.raises(yt_dlp.utils.DownloadError): + _run(tmp_path, "/cookies.txt") + + assert len(seen) == 1 and "impersonate" not in seen[0] From 636372511ba1423d5f9de715dc95ff09a0cf978f Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sun, 13 Sep 2026 23:12:16 +0200 Subject: [PATCH 04/10] chore: remove the unreferenced backend/Dockerfile Nothing builds from it: both compose files build the root Dockerfile, and the test image has its own Dockerfile.test. It installed the unpinned requirements.txt, contradicting the lock header that every image installs requirements.lock. --- backend/Dockerfile | 19 ------------------- 1 file changed, 19 deletions(-) delete mode 100644 backend/Dockerfile diff --git a/backend/Dockerfile b/backend/Dockerfile deleted file mode 100644 index 2f852566..00000000 --- a/backend/Dockerfile +++ /dev/null @@ -1,19 +0,0 @@ -FROM python:3.12-slim - -ENV PYTHONDONTWRITEBYTECODE=1 \ - PYTHONUNBUFFERED=1 \ - PIP_NO_CACHE_DIR=1 \ - PIP_DISABLE_PIP_VERSION_CHECK=1 - -WORKDIR /app - -COPY requirements.txt . -RUN pip install -r requirements.txt - -COPY . . - -RUN adduser --disabled-password --gecos "" appuser && chown -R appuser /app -USER appuser - -EXPOSE 8000 -CMD ["sh", "entrypoint.sh"] From 118fc70e08bea620fb7102f6e6e05a02a12ad3e9 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sun, 13 Sep 2026 23:12:16 +0200 Subject: [PATCH 05/10] fix(downloads): take the duration from the file when yt-dlp gives none A Facebook or Instagram reel extracts with no duration, so the asset kept a NULL duration_s although the file plays for minutes, and the Library row had no trim action. The finalize probe now returns the duration with the codecs (the file wins, info fills a gap), and runs before the sidecars so the .nfo runtime gets it too. --- backend/app/downloads/pipeline.py | 19 +++++++++------ backend/app/downloads/probe.py | 27 ++++++++++++++------- backend/tests/test_upload.py | 40 ++++++++++++++++++++++++++----- 3 files changed, 64 insertions(+), 22 deletions(-) diff --git a/backend/app/downloads/pipeline.py b/backend/app/downloads/pipeline.py index 23251899..0a08fdb6 100644 --- a/backend/app/downloads/pipeline.py +++ b/backend/app/downloads/pipeline.py @@ -483,13 +483,19 @@ def _download( produced = _produced_file(info, staging) produced = _ensure_browser_playable(job_id, produced, spec, info) ext = produced.suffix.lstrip(".").lower() + # Codecs/dims/duration come from a PROBE of the produced file — the ground truth — NOT yt-dlp's + # `info` dict, whose fields are absent for some sources (Facebook among them): deriving + # media_kind from those nulls misclassified a real H.264 video as a download-only 'file', and a + # missing duration hid the trim action. Probed before the sidecars so the .nfo runtime has it + # too. Same probe the upload ingest uses. + vcodec, acodec, width, height, duration_s = probe.resolve_media_facts(produced, info) meta = storage.MediaMeta( video_id=info.get("id") or source_ref, title=normalize_title(info.get("title")) or source_ref, uploader=info.get("uploader") or info.get("channel") or "Unknown Channel", upload_date=_parse_upload_date(info.get("upload_date")), uniqueid_type=adapter.nfo_uniqueid_type(), - duration_s=int(info["duration"]) if info.get("duration") else None, + duration_s=duration_s, description=info.get("description"), thumbnail_url=info.get("thumbnail"), ) @@ -515,12 +521,11 @@ def _download( asset.rel_path = rel asset.size_bytes = size asset.duration_s = meta.duration_s - # Codecs/dims come from a PROBE of the finished file — the ground truth — NOT yt-dlp's - # `info` dict, whose codec fields are absent for some sources (Facebook among them): - # deriving media_kind from those nulls misclassified a real H.264 video as a - # download-only 'file' with no in-app play/edit. Same probe the upload ingest uses. - asset.vcodec, asset.acodec, asset.width, asset.height = probe.resolve_media_facts( - final, info + asset.vcodec, asset.acodec, asset.width, asset.height = ( + vcodec, + acodec, + width, + height, ) asset.container = ext asset.media_kind = mediakind.derive(asset.vcodec, asset.acodec) diff --git a/backend/app/downloads/probe.py b/backend/app/downloads/probe.py index 109339e6..4187d00c 100644 --- a/backend/app/downloads/probe.py +++ b/backend/app/downloads/probe.py @@ -81,10 +81,12 @@ def probe_media(path: Path) -> tuple[str | None, str | None, int | None, int | N def resolve_media_facts( path: Path, info: dict -) -> tuple[str | None, str | None, int | None, int | None]: - """(vcodec, acodec, width, height) for the stored download asset: PROBE the finished file on disk - (the ground truth). yt-dlp's `info` omits codec/dimension fields for some sources (Facebook among - them), so deriving media_kind from it there labelled a real H.264 video a download-only 'file'. +) -> tuple[str | None, str | None, int | None, int | None, int | None]: + """(vcodec, acodec, width, height, duration_s) for the stored download asset: PROBE the finished + file on disk (the ground truth). yt-dlp's `info` omits codec/dimension fields for some sources + (Facebook among them), so deriving media_kind from it there labelled a real H.264 video a + download-only 'file'. It omits the duration too (a Facebook reel, an Instagram reel), and a NULL + duration hides the Library row's trim action although the file plays for minutes. Precedence is by whether the probe RAN, not per-field: if ffprobe read the file, its codec verdict is authoritative — INCLUDING a deliberate None (cover-art-only audio, a still image). A per-field @@ -92,16 +94,23 @@ def resolve_media_facts( have (yt-dlp reports a progressive `vcodec` for an audio-mode download whose video it then strips), mislabelling an audio download as a broken in-app 'video'. Only when ffprobe could NOT read the file at all do we fall back to `info` (the pre-probe behaviour, a safety net); the cosmetic - dimensions still gap-fill from `info` in either case.""" + dimensions and the duration still gap-fill from `info` in either case.""" data = editmod.run_ffprobe_json(path, show_streams=True, show_format=True) info_v = info.get("vcodec") if info.get("vcodec") not in (None, "none") else None info_a = info.get("acodec") if info.get("acodec") not in (None, "none") else None info_w = int(info["width"]) if info.get("width") else None info_h = int(info["height"]) if info.get("height") else None + info_d = int(info["duration"]) if info.get("duration") else None if data is None: # ffprobe could not read the file — fall back to the extractor's info entirely. - return info_v, info_a, info_w, info_h - pv, pa, pw, ph, _dur = _facts_from_probe(data) + return info_v, info_a, info_w, info_h, info_d + pv, pa, pw, ph, pd = _facts_from_probe(data) # Probe ran → its codecs win outright (a None means "no such track", not "unknown"); info only - # gap-fills the cosmetic width/height the probe couldn't read. - return pv, pa, pw if pw is not None else info_w, ph if ph is not None else info_h + # gap-fills the width/height/duration the probe couldn't read. + return ( + pv, + pa, + pw if pw is not None else info_w, + ph if ph is not None else info_h, + pd if pd is not None else info_d, + ) diff --git a/backend/tests/test_upload.py b/backend/tests/test_upload.py index 6e7a9d01..d1e8c9c2 100644 --- a/backend/tests/test_upload.py +++ b/backend/tests/test_upload.py @@ -424,10 +424,36 @@ class TestResolveMediaFacts: ], }, ) - v, a, w, h = probemod.resolve_media_facts(Path("x"), {"width": None, "height": None}) + v, a, w, h, _d = probemod.resolve_media_facts(Path("x"), {"width": None, "height": None}) assert (v, a, w, h) == ("h264", "aac", 720, 1280) assert mediakind.derive(v, a) == mediakind.VIDEO # NOT 'file' + def test_the_duration_comes_from_the_file_when_info_has_none(self, monkeypatch): + # Facebook reel 257 (E11.5): yt-dlp gave no duration, the file is 128.4 s. A NULL duration hid + # the Library row's trim action. + self._ffprobe( + monkeypatch, + { + "format": {"format_name": "mov,mp4,m4a", "duration": "128.400000"}, + "streams": [{"codec_type": "video", "codec_name": "h264"}], + }, + ) + assert probemod.resolve_media_facts(Path("x"), {"duration": None})[4] == 128 + # The file wins over a differing info duration, like its codecs do. + assert probemod.resolve_media_facts(Path("x"), {"duration": 300})[4] == 128 + + def test_info_fills_the_duration_the_probe_could_not_read(self, monkeypatch): + self._ffprobe( + monkeypatch, + { + "format": {"format_name": "mov,mp4,m4a"}, + "streams": [{"codec_type": "video", "codec_name": "h264"}], + }, + ) + assert probemod.resolve_media_facts(Path("x"), {"duration": 61.7})[4] == 61 + self._ffprobe(monkeypatch, None) + assert probemod.resolve_media_facts(Path("x"), {"duration": 61.7})[4] == 61 + def test_probe_wins_over_a_DIFFERING_populated_info(self, monkeypatch): # Both the probe AND info are populated but DISAGREE (info carries a pre-remux/pre-merge codec). # The finished file on disk is ground truth, so the probe wins outright — pins the precedence a @@ -442,7 +468,7 @@ class TestResolveMediaFacts: ], }, ) - v, a, w, h = probemod.resolve_media_facts( + v, a, w, h, _d = probemod.resolve_media_facts( Path("x"), {"vcodec": "vp9", "acodec": "opus", "width": 640, "height": 480} ) assert (v, a, w, h) == ("h264", "aac", 1920, 1080) # probe, not info @@ -466,7 +492,7 @@ class TestResolveMediaFacts: ], }, ) - v, a, w, h = probemod.resolve_media_facts( + v, a, w, h, _d = probemod.resolve_media_facts( Path("x"), {"vcodec": "avc1", "acodec": "mp4a", "width": 1280, "height": 720} ) assert v is None and a == "mp3" # info's avc1 did NOT win @@ -482,7 +508,7 @@ class TestResolveMediaFacts: "streams": [{"codec_type": "video", "codec_name": "png", "width": 8, "height": 8}], }, ) - v, a, w, h = probemod.resolve_media_facts( + v, a, w, h, _d = probemod.resolve_media_facts( Path("x"), {"vcodec": "png", "width": 8, "height": 8} ) assert v is None and mediakind.derive(v, a) == mediakind.FILE @@ -490,7 +516,7 @@ class TestResolveMediaFacts: def test_info_fills_in_only_when_ffprobe_could_not_read_the_file(self, monkeypatch): # ffprobe failed entirely (None) → fall back to info so we never regress the working path. self._ffprobe(monkeypatch, None) - v, a, w, h = probemod.resolve_media_facts( + v, a, w, h, _d = probemod.resolve_media_facts( Path("x"), {"vcodec": "vp9", "acodec": "opus", "width": 1920, "height": 1080} ) assert (v, a, w, h) == ("vp9", "opus", 1920, 1080) @@ -498,6 +524,8 @@ class TestResolveMediaFacts: def test_info_none_codec_string_is_treated_as_absent(self, monkeypatch): # yt-dlp writes the literal "none" for a missing stream — it must not become a codec name. self._ffprobe(monkeypatch, None) # probe failed → info path, where the "none" scrub applies - v, a, w, h = probemod.resolve_media_facts(Path("x"), {"vcodec": "none", "acodec": "aac"}) + v, a, w, h, _d = probemod.resolve_media_facts( + Path("x"), {"vcodec": "none", "acodec": "aac"} + ) assert (v, a) == (None, "aac") assert mediakind.derive(v, a) == mediakind.AUDIO From 90ee34a1b474315047698f9038526c8a6ae5d8b6 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sun, 13 Sep 2026 23:38:11 +0200 Subject: [PATCH 06/10] feat(downloads): a Facebook group post downloads its own video, or says it has none Given a group post's link, yt-dlp either found no formats or took another video from the page's feed data, so a "successful" retry of such a post was a wrong download. With the user's cookie the Facebook adapter now reads the post page, takes the single video node in the script blocks that carry the post id, and downloads that video's watch link (the Chrome retry does the rest). No single video in the post fails with its own message and the group-post copy, reworded to say the post has no video; the post link is never handed to yt-dlp as a fallback. Without a cookie nothing changes. --- backend/app/downloads/adapters.py | 52 ++++++++ backend/app/downloads/errors.py | 22 ++- backend/app/downloads/pipeline.py | 20 ++- backend/tests/test_download_errors.py | 12 ++ backend/tests/test_facebook_impersonation.py | 133 +++++++++++++++++-- frontend/src/i18n/locales/en/downloads.json | 4 +- frontend/src/i18n/locales/hu/downloads.json | 4 +- 7 files changed, 228 insertions(+), 19 deletions(-) diff --git a/backend/app/downloads/adapters.py b/backend/app/downloads/adapters.py index 86242787..02a7f822 100644 --- a/backend/app/downloads/adapters.py +++ b/backend/app/downloads/adapters.py @@ -11,8 +11,10 @@ Resolution works both before extraction (from source_kind / URL host) and after `extractor_key` in the info dict), so a non-YouTube "url" job still lands on the right adapter. """ +import re from urllib.parse import urlparse +import yt_dlp from yt_dlp.networking.impersonate import ImpersonateTarget from app.config import settings @@ -24,6 +26,12 @@ class SourceAdapter: key = "generic" # --- extraction (worker._extract_with_fallback / backfill) --------------------------------- + def resolve_url(self, url: str, ydl_opts: dict) -> str | None: + """The link to hand yt-dlp for `url`, resolved BEFORE the first attempt, or None when the source + itself shows there is no single video to download there. `ydl_opts` are the download's own + options (cookie, timeouts, address family) for any page this needs to read. Generic: `url`.""" + return url + def client_sets(self, authed: bool = False) -> list: """Attempts to make, in order — each entry is handed back to `ydl_opts_extra`, and the next one is tried when `retriable` says so. A generic source makes a single default attempt (`[None]`). @@ -103,6 +111,22 @@ class FacebookAdapter(SourceAdapter): # a cookie the same error is the login wall, which no disguise gets past. _IMPERSONATE = "impersonate" _WALL = "cannot parse data" + # A post page is ~4–5 MB of inline JSON; anything far past that is not a post page. + _POST_PAGE_MAX_BYTES = 16 * 1024 * 1024 + + def resolve_url(self, url: str, ydl_opts: dict) -> str | None: + # Measured (E11.5 S4): given a group post's link, yt-dlp either finds no formats or — worse — + # downloads ANOTHER video from the page's feed data. The post's own video is the one Video node + # in the page scripts that carry the post id; its watch link downloads (plain first, then the + # Chrome retry above). Needs the cookie: signed out, the page is the login wall. + post_id = facebook_group_post_id(url) + if post_id is None or not ydl_opts.get("cookiefile"): + return url + with yt_dlp.YoutubeDL(ydl_opts) as ydl: + raw = ydl.urlopen(url).read(self._POST_PAGE_MAX_BYTES) + video_id = facebook_post_video_id(raw.decode("utf-8", "replace"), post_id) + # Never fall back to the post link: that is exactly where the wrong video comes from. + return f"https://www.facebook.com/watch/?v={video_id}" if video_id else None def client_sets(self, authed: bool = False) -> list: return [None, self._IMPERSONATE] if authed else [None] @@ -128,6 +152,34 @@ class FacebookAdapter(SourceAdapter): return True +_FB_GROUP_POST_PATH = re.compile(r"^/groups/[^/]+/(?:permalink|posts)/(\d+)/?$") +_SCRIPT_BLOCK = re.compile(r"]*>(.*?)", re.S) +_VIDEO_NODE = re.compile(r'"__typename":"Video","id":"(\d+)"') + + +def facebook_group_post_id(url: str) -> str | None: + """The post id of a Facebook group post link (`/groups//permalink|posts//`), else None.""" + parsed = urlparse(url) + if not _host_is(parsed.netloc.lower(), "facebook.com"): + return None + match = _FB_GROUP_POST_PATH.match(parsed.path) + return match.group(1) if match else None + + +def facebook_post_video_id(page: str, post_id: str) -> str | None: + """The id of the one video attached to `post_id`, read from its page: the Video nodes of the script + blocks that carry the post id (the page's other ~10 videos — feed, reels — sit in other blocks). + None for no video AND for more than one: guessing between two would be the wrong-video bug again.""" + quoted = f'"{post_id}"' + ids = { + vid + for block in _SCRIPT_BLOCK.findall(page) + if quoted in block + for vid in _VIDEO_NODE.findall(block) + } + return ids.pop() if len(ids) == 1 else None + + class RedditAdapter(SourceAdapter): key = "reddit" diff --git a/backend/app/downloads/errors.py b/backend/app/downloads/errors.py index 0d79b480..3b68e300 100644 --- a/backend/app/downloads/errors.py +++ b/backend/app/downloads/errors.py @@ -28,7 +28,7 @@ from urllib.parse import urlparse from app.downloads.credentials import host_key_for_url -# Two worker-authored strings that this module owns because both are coupled to `_RULES`. +# Worker-authored strings that this module owns because each is coupled to `_RULES`. # # QUOTA_FULL_MESSAGE is raised by the worker and must classify back to `quota_exceeded`; its rule # below matches a fragment of it, and test_download_errors pins the pair, because a message and its @@ -43,6 +43,10 @@ from app.downloads.credentials import host_key_for_url # classifies as `unknown` — it is a refusal, not a diagnosis, and no category would say more. QUOTA_FULL_MESSAGE = "Your download storage is full — free up space, then try again." NETWORK_REDACTED_MESSAGE = "The server could not reach that address." +# POST_WITHOUT_VIDEO_MESSAGE is raised by the worker when a source adapter read a post's own page and +# found no single video in it (`SourceAdapter.resolve_url` → None; a Facebook group post, E11.5 S4). +# Its rule gives the category back without a link; the catalog entry gives the site's sentence. +POST_WITHOUT_VIDEO_MESSAGE = "No single video was found in this post." # (marker, category) — first match wins, so order most-specific → most-general. Matching is # case-insensitive on a substring, against the raw yt-dlp/ffmpeg message. @@ -52,6 +56,8 @@ _RULES: tuple[tuple[str, str], ...] = ( ("download storage is full", "quota_exceeded"), # Our own redaction of a transport failure (see NETWORK_REDACTED_MESSAGE) — our wording too. (NETWORK_REDACTED_MESSAGE.lower(), "network"), + # Our own verdict on a post link whose page holds no single video — our wording too. + (POST_WITHOUT_VIDEO_MESSAGE.lower(), "format_unavailable"), # Login / age / members / bot-check — the user must be signed in (we deliberately are not). ("sign in to confirm your age", "login_required"), ("confirm your age", "login_required"), @@ -166,9 +172,17 @@ _CATALOG: tuple[_Entry, ...] = ( detail="facebook.cookie_rejected", cookie=True, ), - # A group post whose video yt-dlp does not reach from the post's own link, although the video's - # own link downloads (prod job 66, with the cookie). Only for group links: a reel with no formats - # is a different failure. + # A group post with no single video of its own. With a cookie the adapter reads the post's page + # and says so itself (POST_WITHOUT_VIDEO_MESSAGE); without one yt-dlp is given the post link and + # answers "No video formats found" (measured on prod jobs 66 and 145, whose videos are reshared + # clips). Only for group links: a reel with no formats is a different failure. + _Entry( + host="facebook", + markers=(POST_WITHOUT_VIDEO_MESSAGE.lower(),), + code="format_unavailable", + detail="facebook.group_post_video", + link=("/groups/",), + ), _Entry( host="facebook", markers=("[facebook]", "no video formats found"), diff --git a/backend/app/downloads/pipeline.py b/backend/app/downloads/pipeline.py index 0a08fdb6..e5e5f277 100644 --- a/backend/app/downloads/pipeline.py +++ b/backend/app/downloads/pipeline.py @@ -325,12 +325,30 @@ def _extract_with_fallback( error); a Facebook download WITH a cookie is retried as Chrome behind "Cannot parse data"; other sources make a single attempt. Returns the info dict of the successful attempt. + Before the first attempt the adapter may resolve `url` to the link that really holds the video (a + Facebook group post → its video's watch link); when it finds no single video there the job fails + with POST_WITHOUT_VIDEO_MESSAGE instead of letting yt-dlp guess. + `cookiefile` (S6) is the acting user's per-host cookies.txt, already gated by `credentials.resolve_cookiefile`; passed to every attempt so the fallback stays authenticated. `max_bytes` (SEC-13) is the owner's remaining storage budget, applied to every attempt.""" client_sets = adapter.client_sets(authed=cookiefile is not None) outtmpl = str(staging / "%(id)s.%(ext)s") + # Resolving may read a page first — network time the stall watchdog must not see as a stall. + _touch_job(job_id) + target = adapter.resolve_url( + url, + formats.build_ydl_opts( + spec, + outtmpl, + _make_progress_hook(job_id, asset_id), + cookiefile=cookiefile, + max_bytes=max_bytes, + ), + ) + if target is None: + raise RuntimeError(dlerrors.POST_WITHOUT_VIDEO_MESSAGE) last_exc: Exception | None = None for i, clients in enumerate(client_sets): opts = formats.build_ydl_opts( @@ -348,7 +366,7 @@ def _extract_with_fallback( # killed — acceptable, as socket_timeout + extractor_retries bound one attempt well under it.) try: with yt_dlp.YoutubeDL(opts) as ydl: - return ydl.extract_info(url, download=True) + return ydl.extract_info(target, download=True) except yt_dlp.utils.DownloadError as exc: last_exc = exc if i < len(client_sets) - 1 and adapter.retriable(str(exc)): diff --git a/backend/tests/test_download_errors.py b/backend/tests/test_download_errors.py index b467dee6..e85d3b93 100644 --- a/backend/tests/test_download_errors.py +++ b/backend/tests/test_download_errors.py @@ -113,6 +113,18 @@ def test_a_facebook_group_post_without_formats_names_the_link(): ) +def test_our_post_without_a_video_verdict_names_the_group_link(): + # The worker's own text when the post page holds no single video (S4): the message and its rule and + # catalog entry are pinned together, or an edit to the wording orphans the copy. + msg = dlerrors.POST_WITHOUT_VIDEO_MESSAGE + assert classify(msg) == "format_unavailable" + assert dlerrors.diagnose(msg, url=_GROUP_POST, cookie_attached=True) == ( + "format_unavailable", + "facebook.group_post_video", + ) + assert dlerrors.diagnose(msg, url=_REEL, cookie_attached=True) == ("format_unavailable", None) + + def test_a_catalog_entry_never_leaves_its_host(): # "Cannot parse data" is yt-dlp's wording for a broken extractor on every site. other = _FB_PARSE.replace("[facebook]", "[vimeo]") diff --git a/backend/tests/test_facebook_impersonation.py b/backend/tests/test_facebook_impersonation.py index 04f1ec1c..186fa7ea 100644 --- a/backend/tests/test_facebook_impersonation.py +++ b/backend/tests/test_facebook_impersonation.py @@ -1,11 +1,16 @@ -"""E11.5 S3 — a Facebook cookie download stuck at "Cannot parse data" is retried as Chrome. +"""E11.5 S3 + S4 — the Facebook download path with the user's cookie. -Measured on dev (yt-dlp 2026.07.04, the owner's cookie): reel 806379462330494 fails with the cookie +S3, measured on dev (yt-dlp 2026.07.04, the owner's cookie): reel 806379462330494 fails with the cookie alone and extracts 2560p with `--impersonate chrome`; the reels that already download with the cookie keep their plain first attempt. These pin the attempt order through the real download loop (`pipeline._extract_with_fallback`, with yt-dlp faked at the `YoutubeDL` boundary), and that the LOCKED curl_cffi can actually impersonate — the pair only works inside yt-dlp's own version range, and outside it every target silently reports "unavailable". + +S4, measured on dev and in the owner's Chrome: given a group post's link, yt-dlp finds no formats or +downloads another video from the page's feed. The post's own video is the single Video node in the +page scripts that carry the post id (prod posts 66, 145, 140, 143, 221; text posts have none while the +page holds ~10 other videos), and its watch link downloads. """ import pytest @@ -13,8 +18,31 @@ import yt_dlp from yt_dlp.networking.impersonate import ImpersonateTarget from app.downloads import adapters, pipeline +from app.downloads import errors as dlerrors _URL = "https://www.facebook.com/reel/806379462330494/?fs=e&fs=e" +_POST = "1755988225525547" +_GROUP_LINK = f"https://www.facebook.com/groups/983554479435596/permalink/{_POST}/?" +_POST_VIDEO = "1408015447857753" +_FEED_VIDEO = "1056963063816966" # what yt-dlp downloaded from this post's link: a feed video +_CHROME = ImpersonateTarget.from_str("chrome") + + +def _video_node(video_id: str) -> str: + # The measured shape of a post's attachment in the page JSON. + return '{"media":{"__typename":"Video","id":"%s","__isNode":"Video"},"styles":{}}' % video_id + + +def _page(*post_videos: str, post: str = _POST) -> str: + attachments = ",".join(_video_node(v) for v in post_videos) + return ( + '' + '' + ) + + _WALL = ( "ERROR: [facebook] 806379462330494: Cannot parse data; please report this issue on " "https://github.com/yt-dlp/yt-dlp/issues?q= , filling out the appropriate issue template." @@ -30,9 +58,11 @@ def test_the_locked_curl_cffi_can_impersonate_chrome(): @pytest.fixture def ydl(monkeypatch): - """Fake `yt_dlp.YoutubeDL`: records each attempt's options and plays back `outcomes` in order.""" + """Fake `yt_dlp.YoutubeDL`: records each attempt's options and plays back `outcomes` in order; + `web["page"]` is what a page read returns, and `web` records the pages opened and links extracted.""" seen: list[dict] = [] outcomes: list = [] + web: dict = {"page": "", "opened": [], "extracted": []} class FakeYDL: def __init__(self, opts): @@ -44,7 +74,18 @@ def ydl(monkeypatch): def __exit__(self, *exc): return False + def urlopen(self, url): + web["opened"].append(url) + body = web["page"].encode() + + class Response: + def read(self, amt=None): + return body + + return Response() + def extract_info(self, url, download): + web["extracted"].append(url) outcome = outcomes.pop(0) if isinstance(outcome, str): raise yt_dlp.utils.DownloadError(outcome) @@ -53,27 +94,28 @@ def ydl(monkeypatch): monkeypatch.setattr(pipeline.yt_dlp, "YoutubeDL", FakeYDL) monkeypatch.setattr(pipeline, "_touch_job", lambda job_id: None) monkeypatch.setattr(pipeline, "_set_job", lambda job_id, **fields: None) - return seen, outcomes + return seen, outcomes, web -def _run(tmp_path, cookiefile): +def _run(tmp_path, cookiefile, url=_URL): return pipeline._extract_with_fallback( - 1, 1, _URL, {}, tmp_path, adapters.FacebookAdapter(), cookiefile=cookiefile + 1, 1, url, {}, tmp_path, adapters.FacebookAdapter(), cookiefile=cookiefile ) def test_a_cookie_download_at_the_wall_is_retried_as_chrome(ydl, tmp_path): - seen, outcomes = ydl + seen, outcomes, web = ydl outcomes += [_WALL, {"id": "806379462330494"}] assert _run(tmp_path, "/cookies.txt") == {"id": "806379462330494"} - assert [o.get("impersonate") for o in seen] == [None, ImpersonateTarget.from_str("chrome")] + assert [o.get("impersonate") for o in seen] == [None, _CHROME] assert all(o["cookiefile"] == "/cookies.txt" for o in seen) # the retry stays signed in + assert web["opened"] == [] # a reel is not a group post: no page read def test_without_a_cookie_the_wall_is_the_login_wall_and_final(ydl, tmp_path): - seen, outcomes = ydl + seen, outcomes, _ = ydl outcomes += [_WALL] with pytest.raises(yt_dlp.utils.DownloadError): @@ -83,10 +125,81 @@ def test_without_a_cookie_the_wall_is_the_login_wall_and_final(ydl, tmp_path): def test_other_facebook_failures_are_not_retried_as_chrome(ydl, tmp_path): - seen, outcomes = ydl + seen, outcomes, _ = ydl outcomes += [_NO_FORMATS] with pytest.raises(yt_dlp.utils.DownloadError): _run(tmp_path, "/cookies.txt") assert len(seen) == 1 and "impersonate" not in seen[0] + + +# --- S4: a group post downloads ITS video, or fails saying it has none ---------------------------- + + +def test_a_cookie_group_post_downloads_the_posts_own_video(ydl, tmp_path): + seen, outcomes, web = ydl + web["page"] = _page(_POST_VIDEO) + outcomes += [_WALL, {"id": _POST_VIDEO}] # the watch link needs Chrome too (measured) + + assert _run(tmp_path, "/cookies.txt", _GROUP_LINK) == {"id": _POST_VIDEO} + + assert web["opened"] == [_GROUP_LINK] + assert seen[0]["cookiefile"] == "/cookies.txt" # the post page is read signed in + watch = f"https://www.facebook.com/watch/?v={_POST_VIDEO}" + # Never the post link: that is where the feed video comes from. + assert web["extracted"] == [watch, watch] + assert [o.get("impersonate") for o in seen[1:]] == [None, _CHROME] + + +def test_a_group_post_without_a_single_video_fails_saying_so(ydl, tmp_path): + _, _, web = ydl + web["page"] = _page() # a text post: the only video on the page is the feed's + + with pytest.raises(RuntimeError) as exc: + _run(tmp_path, "/cookies.txt", _GROUP_LINK) + + assert web["extracted"] == [] # yt-dlp is not left to guess from the post link + assert dlerrors.diagnose(str(exc.value), url=_GROUP_LINK, cookie_attached=True) == ( + "format_unavailable", + "facebook.group_post_video", + ) + + +def test_without_a_cookie_the_group_post_link_goes_to_ytdlp_unchanged(ydl, tmp_path): + _, outcomes, web = ydl + outcomes += [_NO_FORMATS] + + with pytest.raises(yt_dlp.utils.DownloadError): + _run(tmp_path, None, _GROUP_LINK) + + assert web["opened"] == [] # signed out the page is the login wall: nothing to read + assert web["extracted"] == [_GROUP_LINK] + + +def test_the_group_post_link_forms_are_recognised(): + for url in ( + _GROUP_LINK, + f"https://www.facebook.com/groups/983554479435596/posts/{_POST}/", + f"https://m.facebook.com/groups/983554479435596/permalink/{_POST}/?", + f"https://www.facebook.com/groups/chatgptmagyarul/posts/{_POST}", + ): + assert adapters.facebook_group_post_id(url) == _POST, url + for url in ( + _URL, + f"https://www.facebook.com/watch/?v={_POST_VIDEO}", + f"https://facebook.com.evil/groups/x/permalink/{_POST}/", + "https://www.facebook.com/groups/983554479435596/", + ): + assert adapters.facebook_group_post_id(url) is None, url + assert adapters.SourceAdapter().resolve_url(_GROUP_LINK, {"cookiefile": "/c"}) == _GROUP_LINK + + +def test_only_the_single_video_of_the_posts_own_blocks_counts(): + assert adapters.facebook_post_video_id(_page(_POST_VIDEO), _POST) == _POST_VIDEO + # No video in the post, although the page has one elsewhere. + assert adapters.facebook_post_video_id(_page(), _POST) is None + # Two videos: guessing between them would be the wrong-video bug again. + assert adapters.facebook_post_video_id(_page(_POST_VIDEO, "2063302677911049"), _POST) is None + # A longer number that merely STARTS with the post id is another object. + assert adapters.facebook_post_video_id(_page(_POST_VIDEO, post=_POST + "0"), _POST) is None diff --git a/frontend/src/i18n/locales/en/downloads.json b/frontend/src/i18n/locales/en/downloads.json index 1c21a6c4..7f0a8242 100644 --- a/frontend/src/i18n/locales/en/downloads.json +++ b/frontend/src/i18n/locales/en/downloads.json @@ -299,8 +299,8 @@ "body": "The cookies have probably expired, or you signed out in that browser. Export them again, upload them, and retry. If it still fails, the content has been removed or isn't visible to you." }, "group_post_video": { - "short": "The video isn't reachable from the post link", - "body": "This is the link of a group post, not of the video. Open the video on Facebook and paste its link." + "short": "No video found in this post", + "body": "We couldn't find a single downloadable video in this group post. If it does have one, open the video on Facebook and paste its link." } } } diff --git a/frontend/src/i18n/locales/hu/downloads.json b/frontend/src/i18n/locales/hu/downloads.json index c1883b52..57c3b775 100644 --- a/frontend/src/i18n/locales/hu/downloads.json +++ b/frontend/src/i18n/locales/hu/downloads.json @@ -299,8 +299,8 @@ "body": "A cookie valószínűleg lejárt, vagy kijelentkeztél abban a böngészőben. Exportáld újra, töltsd fel, és próbáld újra. Ha így sem megy, a tartalom törölt, vagy számodra nem látható." }, "group_post_video": { - "short": "A videó nem érhető el a poszt linkjéről", - "body": "Ez a csoportposzt linkje, nem a videóé. Nyisd meg a videót a Facebookon, és annak a linkjét másold be." + "short": "Ebben a posztban nem található videó", + "body": "Nem találtunk egyértelműen letölthető videót ebben a csoportposztban. Ha mégis van benne, nyisd meg a videót a Facebookon, és annak a linkjét másold be." } } } From b17d92d7c2ba6055b5a697a86a646e8637806a2f Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sun, 13 Sep 2026 23:38:11 +0200 Subject: [PATCH 07/10] fix(downloads): confirm dialogs name the action on their confirm button The download center's confirm button repeated the dialog's question ("Re-download this file?"). It now reads the action: Stop download, Re-download, Remove, Remove from my list. --- frontend/src/components/DownloadCenter.tsx | 10 ++++++++-- frontend/src/i18n/locales/en/downloads.json | 4 +++- frontend/src/i18n/locales/hu/downloads.json | 4 +++- 3 files changed, 14 insertions(+), 4 deletions(-) diff --git a/frontend/src/components/DownloadCenter.tsx b/frontend/src/components/DownloadCenter.tsx index 0980baaa..08e41265 100644 --- a/frontend/src/components/DownloadCenter.tsx +++ b/frontend/src/components/DownloadCenter.tsx @@ -109,8 +109,10 @@ export default function DownloadCenter() { meta: { failTitle: "errors.writeFailed.removeShared" }, }); - const confirmThen = async (title: string, body: string, fn: () => void) => { - if (await confirm({ title, message: body, confirmLabel: title, danger: true })) fn(); + // The confirm button names the ACTION ("Remove"), never the dialog's question: labelled with the title + // it read "Re-download this file?", and the cancel dialog's own "Cancel" would sit beside "Cancel". + const confirmThen = async (title: string, body: string, action: string, fn: () => void) => { + if (await confirm({ title, message: body, confirmLabel: action, danger: true })) fn(); }; // One-click share (E7 S3 · D2-A): ensure a downloadable link, copy it, toast with "open in new tab" @@ -254,6 +256,7 @@ export default function DownloadCenter() { confirmThen( t("downloads.confirm.cancelTitle"), t("downloads.confirm.cancelBody"), + t("downloads.confirm.cancelAction"), () => act.mutate({ id: job.id, op: "cancel" }), ) } @@ -327,6 +330,7 @@ export default function DownloadCenter() { confirmThen( t("downloads.confirm.redownloadTitle"), t("downloads.confirm.redownloadBody"), + t("downloads.confirm.redownloadAction"), () => act.mutate({ id: job.id, op: "redownload" }), ) } @@ -340,6 +344,7 @@ export default function DownloadCenter() { confirmThen( t("downloads.confirm.deleteTitle"), t("downloads.confirm.deleteBody"), + t("downloads.actions.delete"), () => act.mutate({ id: job.id, op: "delete" }), ) } @@ -394,6 +399,7 @@ export default function DownloadCenter() { confirmThen( t("downloads.confirm.removeSharedTitle"), t("downloads.confirm.removeSharedBody"), + t("downloads.actions.removeShared"), () => removeShared.mutate(job.id), ) } diff --git a/frontend/src/i18n/locales/en/downloads.json b/frontend/src/i18n/locales/en/downloads.json index 7f0a8242..fb73b553 100644 --- a/frontend/src/i18n/locales/en/downloads.json +++ b/frontend/src/i18n/locales/en/downloads.json @@ -152,12 +152,14 @@ "confirm": { "cancelTitle": "Cancel download?", "cancelBody": "This stops the download and removes it from your queue.", + "cancelAction": "Stop download", "deleteTitle": "Remove download?", "deleteBody": "This removes it from your list. The file may stay cached for other users until it expires.", "removeSharedTitle": "Remove from your list?", "removeSharedBody": "This removes it from your shared list only — it won't delete the owner's file.", "redownloadTitle": "Re-download this file?", - "redownloadBody": "The current file is deleted and downloaded again under the latest format and naming rules. Any share links you've sent keep working and will point to the new file (a failed re-download leaves the link broken)." + "redownloadBody": "The current file is deleted and downloaded again under the latest format and naming rules. Any share links you've sent keep working and will point to the new file (a failed re-download leaves the link broken).", + "redownloadAction": "Re-download" }, "profiles": { "manage": "Manage formats", diff --git a/frontend/src/i18n/locales/hu/downloads.json b/frontend/src/i18n/locales/hu/downloads.json index 57c3b775..1c2685fe 100644 --- a/frontend/src/i18n/locales/hu/downloads.json +++ b/frontend/src/i18n/locales/hu/downloads.json @@ -152,12 +152,14 @@ "confirm": { "cancelTitle": "Megszakítod a letöltést?", "cancelBody": "Ez leállítja a letöltést és eltávolítja a sorból.", + "cancelAction": "Letöltés leállítása", "deleteTitle": "Eltávolítod a letöltést?", "deleteBody": "Ez eltávolítja a listádról. A fájl a lejáratáig még a gyorsítótárban maradhat mások számára.", "removeSharedTitle": "Eltávolítod a listádból?", "removeSharedBody": "Csak a te megosztott listádból tűnik el — a tulajdonos fájlját nem törli.", "redownloadTitle": "Újratöltöd ezt a fájlt?", - "redownloadBody": "A jelenlegi fájl törlődik és újra letöltődik a legfrissebb formátum- és elnevezési szabályokkal. A már elküldött megosztási linkek működnek tovább, és az új fájlra fognak mutatni (sikertelen újratöltés esetén a link törötten marad)." + "redownloadBody": "A jelenlegi fájl törlődik és újra letöltődik a legfrissebb formátum- és elnevezési szabályokkal. A már elküldött megosztási linkek működnek tovább, és az új fájlra fognak mutatni (sikertelen újratöltés esetén a link törötten marad).", + "redownloadAction": "Újratöltés" }, "profiles": { "manage": "Formátumok kezelése", From e887233210525b39f126df9528e8c8dd5b0f1758 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Mon, 14 Sep 2026 00:50:05 +0200 Subject: [PATCH 08/10] fix(downloads): E11.5 epic-end review round 1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- backend/app/downloads/adapters.py | 42 +++++-- backend/app/downloads/credentials.py | 30 +++-- backend/app/downloads/errors.py | 53 +++++--- backend/app/downloads/pipeline.py | 6 +- backend/app/downloads/service.py | 8 +- backend/app/routes/downloads.py | 33 ++++- backend/app/worker.py | 51 +++++--- backend/requirements.txt | 3 +- backend/tests/test_download_credentials.py | 17 ++- backend/tests/test_download_errors.py | 26 +++- .../tests/test_download_failure_catalog.py | 114 ++++++++++++++++- backend/tests/test_download_job_locks.py | 119 ++++++++++++++++++ backend/tests/test_facebook_impersonation.py | 42 ++++++- backend/tests/test_worker_supervisor.py | 29 +++-- .../DownloadCenter.confirm.dom.test.tsx | 103 +++++++++++++++ frontend/src/components/downloadModals.tsx | 5 +- frontend/src/i18n/locales/en/downloads.json | 4 +- frontend/src/i18n/locales/hu/downloads.json | 14 +-- frontend/src/lib/downloadFailure.test.ts | 6 + frontend/src/lib/downloadFailure.ts | 17 +-- 20 files changed, 610 insertions(+), 112 deletions(-) create mode 100644 backend/tests/test_download_job_locks.py create mode 100644 frontend/src/components/DownloadCenter.confirm.dom.test.tsx diff --git a/backend/app/downloads/adapters.py b/backend/app/downloads/adapters.py index 02a7f822..f2ec3a33 100644 --- a/backend/app/downloads/adapters.py +++ b/backend/app/downloads/adapters.py @@ -19,6 +19,14 @@ from yt_dlp.networking.impersonate import ImpersonateTarget from app.config import settings +# Facebook's "Cannot parse data": the page yt-dlp got was a wall, not the content. Both the Chrome retry +# below and the failure catalog (errors.py) decide on it, so the wording exists once. +FACEBOOK_PARSE_WALL = "cannot parse data" +# A source's own verdicts on a link, raised from `resolve_url`. errors.py classifies them; it imports +# them from here because this module must not import errors (errors → credentials → adapters). +POST_WITHOUT_VIDEO_MESSAGE = "No single video was found in this post." +POST_NOT_SHOWN_MESSAGE = "Facebook did not show this post to the signed-in session." + class SourceAdapter: """Generic source: yt-dlp handles everything and the ref is already a fetchable URL.""" @@ -26,10 +34,11 @@ class SourceAdapter: key = "generic" # --- extraction (worker._extract_with_fallback / backfill) --------------------------------- - def resolve_url(self, url: str, ydl_opts: dict) -> str | None: - """The link to hand yt-dlp for `url`, resolved BEFORE the first attempt, or None when the source - itself shows there is no single video to download there. `ydl_opts` are the download's own - options (cookie, timeouts, address family) for any page this needs to read. Generic: `url`.""" + def resolve_url(self, url: str, ydl_opts: dict) -> str: + """The link to hand yt-dlp for `url`, resolved BEFORE the first attempt. Raises a RuntimeError + carrying one of this module's verdicts when the source itself shows there is no single video to + download there. `ydl_opts` are the download's own options (cookie, timeouts, address family) for + any page this needs to read. Generic: `url`.""" return url def client_sets(self, authed: bool = False) -> list: @@ -110,11 +119,10 @@ class FacebookAdapter(SourceAdapter): # cookie keep their proven path, and only the ones stuck at the wall go through curl_cffi. Without # a cookie the same error is the login wall, which no disguise gets past. _IMPERSONATE = "impersonate" - _WALL = "cannot parse data" # A post page is ~4–5 MB of inline JSON; anything far past that is not a post page. _POST_PAGE_MAX_BYTES = 16 * 1024 * 1024 - def resolve_url(self, url: str, ydl_opts: dict) -> str | None: + def resolve_url(self, url: str, ydl_opts: dict) -> str: # Measured (E11.5 S4): given a group post's link, yt-dlp either finds no formats or — worse — # downloads ANOTHER video from the page's feed data. The post's own video is the one Video node # in the page scripts that carry the post id; its watch link downloads (plain first, then the @@ -124,9 +132,16 @@ class FacebookAdapter(SourceAdapter): return url with yt_dlp.YoutubeDL(ydl_opts) as ydl: raw = ydl.urlopen(url).read(self._POST_PAGE_MAX_BYTES) - video_id = facebook_post_video_id(raw.decode("utf-8", "replace"), post_id) - # Never fall back to the post link: that is exactly where the wrong video comes from. - return f"https://www.facebook.com/watch/?v={video_id}" if video_id else None + page = raw.decode("utf-8", "replace") + # Measured (E11.5 review): a refused or expired cookie gets the login page (~450 KB, a login + # form, no post id at all), which is not a post without a video — the user's cookie is the cause. + if _quoted(post_id) not in page: + raise RuntimeError(POST_NOT_SHOWN_MESSAGE) + video_id = facebook_post_video_id(page, post_id) + if video_id is None: + # Never fall back to the post link: that is exactly where the wrong video comes from. + raise RuntimeError(POST_WITHOUT_VIDEO_MESSAGE) + return f"https://www.facebook.com/watch/?v={video_id}" def client_sets(self, authed: bool = False) -> list: return [None, self._IMPERSONATE] if authed else [None] @@ -137,7 +152,7 @@ class FacebookAdapter(SourceAdapter): return {} def retriable(self, error_text: str) -> bool: - return self._WALL in error_text.lower() + return FACEBOOK_PARSE_WALL in error_text.lower() def uploader_url(self, info: dict) -> str | None: # Facebook gives neither channel_url nor uploader_url, but exposes a numeric `uploader_id` @@ -157,6 +172,11 @@ _SCRIPT_BLOCK = re.compile(r"]*>(.*?)", re.S) _VIDEO_NODE = re.compile(r'"__typename":"Video","id":"(\d+)"') +def _quoted(post_id: str) -> str: + # How the page JSON carries the id: quoted, so a longer id that merely starts with it never counts. + return f'"{post_id}"' + + def facebook_group_post_id(url: str) -> str | None: """The post id of a Facebook group post link (`/groups//permalink|posts//`), else None.""" parsed = urlparse(url) @@ -170,7 +190,7 @@ def facebook_post_video_id(page: str, post_id: str) -> str | None: """The id of the one video attached to `post_id`, read from its page: the Video nodes of the script blocks that carry the post id (the page's other ~10 videos — feed, reels — sit in other blocks). None for no video AND for more than one: guessing between two would be the wrong-video bug again.""" - quoted = f'"{post_id}"' + quoted = _quoted(post_id) ids = { vid for block in _SCRIPT_BLOCK.findall(page) diff --git a/backend/app/downloads/credentials.py b/backend/app/downloads/credentials.py index 917ad96e..38785e17 100644 --- a/backend/app/downloads/credentials.py +++ b/backend/app/downloads/credentials.py @@ -224,15 +224,12 @@ def status_list(db: Session, user_id: int) -> list[dict]: # --- worker: resolve + write the cookie file -------------------------------------------------- -def resolve_cookiefile( - db: Session, - user_id: int, - url: str | None, - asset: MediaAsset, - spec: dict, - staging: Path, -) -> Path | None: - """In the worker child: the cookies.txt to hand yt-dlp for this download, or None. +def usable_cookie( + db: Session, user_id: int, url: str | None, asset: MediaAsset, spec: dict +) -> str | None: + """The user's cookie text a download of `url` into `asset` goes with, or None — the ONE decision + whether a download is signed in. The worker child writes it out for yt-dlp (`resolve_cookiefile`); + the worker asks it again for a job that inherits a failed asset, so both jobs are told the same. The double gate that prevents cross-user leakage: 1. a credential must still exist for `url`'s host (host-scoped via `host_key_for_url`), AND @@ -260,7 +257,20 @@ def resolve_cookiefile( host_key, ) return None - return _write_cookiefile(raw, staging) + return raw + + +def resolve_cookiefile( + db: Session, + user_id: int, + url: str | None, + asset: MediaAsset, + spec: dict, + staging: Path, +) -> Path | None: + """In the worker child: the cookies.txt to hand yt-dlp for this download (`usable_cookie`), or None.""" + raw = usable_cookie(db, user_id, url, asset, spec) + return _write_cookiefile(raw, staging) if raw else None def _write_cookiefile(raw: str, staging: Path) -> Path: diff --git a/backend/app/downloads/errors.py b/backend/app/downloads/errors.py index 3b68e300..47810a8a 100644 --- a/backend/app/downloads/errors.py +++ b/backend/app/downloads/errors.py @@ -16,16 +16,23 @@ its own sentence for — why it failed on that site, and what the user can do. ` catalog first and falls back to the category. How an entry is added, every time a failure is reported: 1. reproduce it on dev (with and without the user's cookie) and keep the verbatim yt-dlp text; 2. add the entry below, scoped as narrowly as the measurement allows (a marker that is yt-dlp's - generic wording for every site must never match without its host); + generic wording for every site must never match without its host; a marker another module also + decides on — the Facebook Chrome retry's FACEBOOK_PARSE_WALL — is imported, never retyped); 3. pin the real text in `tests/test_download_errors.py`; 4. add `downloads.errors.fail.catalog..{short,body}` in EN and HU (a contract test checks it; the group is not called `detail`, which `actionErrors.contract` reads as a raw `HttpError.detail`). """ +from collections.abc import Callable from dataclasses import dataclass from typing import NamedTuple -from urllib.parse import urlparse +from app.downloads.adapters import ( + FACEBOOK_PARSE_WALL, + POST_NOT_SHOWN_MESSAGE, + POST_WITHOUT_VIDEO_MESSAGE, + facebook_group_post_id, +) from app.downloads.credentials import host_key_for_url # Worker-authored strings that this module owns because each is coupled to `_RULES`. @@ -41,12 +48,12 @@ from app.downloads.credentials import host_key_for_url # Other worker-raised texts are still inline literals at their raise sites. The ffmpeg ones and # "produced no output file" have markers here; the urlguard refusal deliberately has none and # classifies as `unknown` — it is a refusal, not a diagnosis, and no category would say more. +# +# A source adapter's verdicts on a link (`SourceAdapter.resolve_url`, E11.5 S4) are adapters.py's +# constants, imported above; each has a rule below that gives its category back, and a catalog entry +# that gives the site's sentence. QUOTA_FULL_MESSAGE = "Your download storage is full — free up space, then try again." NETWORK_REDACTED_MESSAGE = "The server could not reach that address." -# POST_WITHOUT_VIDEO_MESSAGE is raised by the worker when a source adapter read a post's own page and -# found no single video in it (`SourceAdapter.resolve_url` → None; a Facebook group post, E11.5 S4). -# Its rule gives the category back without a link; the catalog entry gives the site's sentence. -POST_WITHOUT_VIDEO_MESSAGE = "No single video was found in this post." # (marker, category) — first match wins, so order most-specific → most-general. Matching is # case-insensitive on a substring, against the raw yt-dlp/ffmpeg message. @@ -56,8 +63,10 @@ _RULES: tuple[tuple[str, str], ...] = ( ("download storage is full", "quota_exceeded"), # Our own redaction of a transport failure (see NETWORK_REDACTED_MESSAGE) — our wording too. (NETWORK_REDACTED_MESSAGE.lower(), "network"), - # Our own verdict on a post link whose page holds no single video — our wording too. + # Our own verdicts on a post link (adapters.py) — our wording too: its page holds no single video, + # or the site answered the signed-in read with something that is not the post at all. (POST_WITHOUT_VIDEO_MESSAGE.lower(), "format_unavailable"), + (POST_NOT_SHOWN_MESSAGE.lower(), "login_required"), # Login / age / members / bot-check — the user must be signed in (we deliberately are not). ("sign in to confirm your age", "login_required"), ("confirm your age", "login_required"), @@ -141,14 +150,15 @@ class Diagnosis(NamedTuple): class _Entry: """One measured failure of one site. It matches when the download URL belongs to `host` (`credentials.SUPPORTED_HOSTS`), every one of `markers` is in the message, the cookie state is - `cookie` (None: either), and — when `link` is given — one of those fragments is in the URL path.""" + `cookie` (None: either), and — when `link` is given — it recognises the URL (a truthy result). A + link predicate is the adapter's own, so the catalog and the download agree on what a link is.""" host: str markers: tuple[str, ...] code: str detail: str cookie: bool | None = None - link: tuple[str, ...] = () + link: Callable[[str], object] | None = None _CATALOG: tuple[_Entry, ...] = ( @@ -158,7 +168,7 @@ _CATALOG: tuple[_Entry, ...] = ( # which is why the entry is scoped to Facebook and never becomes a general rule. _Entry( host="facebook", - markers=("[facebook]", "cannot parse data"), + markers=("[facebook]", FACEBOOK_PARSE_WALL), code="login_required", detail="facebook.login_wall", cookie=False, @@ -167,7 +177,16 @@ _CATALOG: tuple[_Entry, ...] = ( # impersonation (prod job 257). Telling this user to add cookies would be wrong — they have. _Entry( host="facebook", - markers=("[facebook]", "cannot parse data"), + markers=("[facebook]", FACEBOOK_PARSE_WALL), + code="login_required", + detail="facebook.cookie_rejected", + cookie=True, + ), + # A group post read with a cookie that Facebook answered with its login page (measured with a + # refused cookie: no post id anywhere on the page) — the same refused cookie, found by the adapter. + _Entry( + host="facebook", + markers=(POST_NOT_SHOWN_MESSAGE.lower(),), code="login_required", detail="facebook.cookie_rejected", cookie=True, @@ -175,20 +194,23 @@ _CATALOG: tuple[_Entry, ...] = ( # A group post with no single video of its own. With a cookie the adapter reads the post's page # and says so itself (POST_WITHOUT_VIDEO_MESSAGE); without one yt-dlp is given the post link and # answers "No video formats found" (measured on prod jobs 66 and 145, whose videos are reshared - # clips). Only for group links: a reel with no formats is a different failure. + # clips). With a cookie yt-dlp only ever gets the post's VIDEO link, so the same text there is about + # that video, not the post — the category says it. Only for group post links (the adapter's own + # test of one): a reel with no formats is a different failure. _Entry( host="facebook", markers=(POST_WITHOUT_VIDEO_MESSAGE.lower(),), code="format_unavailable", detail="facebook.group_post_video", - link=("/groups/",), + link=facebook_group_post_id, ), _Entry( host="facebook", markers=("[facebook]", "no video formats found"), code="format_unavailable", detail="facebook.group_post_video", - link=("/groups/",), + cookie=False, + link=facebook_group_post_id, ), ) @@ -214,13 +236,12 @@ def diagnose( host = host_key_for_url(url) if msg and url else None if host is not None: low = msg.lower() # type: ignore[union-attr] — `host` is only set when `msg` is truthy - path = urlparse(url).path.lower() for e in _CATALOG: if ( e.host == host and all(m in low for m in e.markers) and (e.cookie is None or e.cookie == cookie_attached) - and (not e.link or any(fragment in path for fragment in e.link)) + and (e.link is None or e.link(url)) # type: ignore[arg-type] — as `msg` above ): return Diagnosis(e.code, e.detail) return Diagnosis(classify(msg)) diff --git a/backend/app/downloads/pipeline.py b/backend/app/downloads/pipeline.py index e5e5f277..5b15fe0e 100644 --- a/backend/app/downloads/pipeline.py +++ b/backend/app/downloads/pipeline.py @@ -326,8 +326,8 @@ def _extract_with_fallback( sources make a single attempt. Returns the info dict of the successful attempt. Before the first attempt the adapter may resolve `url` to the link that really holds the video (a - Facebook group post → its video's watch link); when it finds no single video there the job fails - with POST_WITHOUT_VIDEO_MESSAGE instead of letting yt-dlp guess. + Facebook group post → its video's watch link); when the source shows no single video there, the + adapter raises its own verdict (adapters.POST_*_MESSAGE) instead of letting yt-dlp guess. `cookiefile` (S6) is the acting user's per-host cookies.txt, already gated by `credentials.resolve_cookiefile`; passed to every attempt so the fallback stays authenticated. @@ -347,8 +347,6 @@ def _extract_with_fallback( max_bytes=max_bytes, ), ) - if target is None: - raise RuntimeError(dlerrors.POST_WITHOUT_VIDEO_MESSAGE) last_exc: Exception | None = None for i, clients in enumerate(client_sets): opts = formats.build_ydl_opts( diff --git a/backend/app/downloads/service.py b/backend/app/downloads/service.py index 77ff9fd2..96918e0f 100644 --- a/backend/app/downloads/service.py +++ b/backend/app/downloads/service.py @@ -81,11 +81,13 @@ def get_or_create_asset( status="pending", ref_count=0, ) - db.add(asset) try: - db.flush() + # A SAVEPOINT: losing the race undoes only this insert, never the caller's own pending writes + # (Retry clears the job's error before it gets here; a whole-session rollback threw that away). + with db.begin_nested(): + db.add(asset) + db.flush() except IntegrityError: - db.rollback() asset = db.execute(stmt).scalar_one() return asset diff --git a/backend/app/routes/downloads.py b/backend/app/routes/downloads.py index 0ec65afa..91d56d2a 100644 --- a/backend/app/routes/downloads.py +++ b/backend/app/routes/downloads.py @@ -173,6 +173,31 @@ def _own_job(db: Session, user: User, job_id: int) -> DownloadJob: return job +def _own_job_for_update(db: Session, user: User, job_id: int) -> DownloadJob: + """`_own_job` for a transition that moves its asset's ref count (resume, cancel, delete, redownload). + The job is re-read under a row lock, so two overlapping requests for one job run one after the other + and the second decides on the first's committed result — never both releasing the same hold, which + can free a file another job still holds. + + The asset is locked BEFORE the job: the order a finishing worker child takes them in + (`service.finalize_if_running`), so the two can never deadlock.""" + while True: + asset_id = _own_job(db, user, job_id).asset_id + if asset_id is not None: + db.execute(select(MediaAsset.id).where(MediaAsset.id == asset_id).with_for_update()) + job = db.execute( + select(DownloadJob) + .where(DownloadJob.id == job_id) + .with_for_update() + .execution_options(populate_existing=True) + ).scalar_one_or_none() + if job is None: + raise HTTPException(status_code=404, detail="Unknown download") + if job.asset_id == asset_id: + return job + db.rollback() # another request moved the job to another asset meanwhile: lock that one + + # --- profiles ------------------------------------------------------------------------------ @@ -615,7 +640,7 @@ def pause_download( def resume_download( job_id: int, user: User = Depends(require_human), db: Session = Depends(get_db) ) -> dict: - job = _own_job(db, user, job_id) + job = _own_job_for_update(db, user, job_id) service.resume(db, job) return _serialize_job(db, job) @@ -636,7 +661,7 @@ def redownload_download( under the current format/naming rules — never silently reuses a shared cached file — and (b) never deletes a file another job/user still shares: the old asset is only removed if THIS job was its last holder.""" - job = _own_job(db, user, job_id) + job = _own_job_for_update(db, user, job_id) if job.job_kind in ("edit", "upload"): # Neither has a re-fetchable source: an editor clip is derived locally, an upload IS the # source. Re-running yt-dlp against source_kind='upload'/'edit' would just fail. @@ -651,7 +676,7 @@ def redownload_download( def cancel_download( job_id: int, user: User = Depends(require_human), db: Session = Depends(get_db) ) -> dict: - job = _own_job(db, user, job_id) + job = _own_job_for_update(db, user, job_id) service.cancel(db, job) return _serialize_job(db, job) @@ -660,7 +685,7 @@ def cancel_download( def delete_download( job_id: int, user: User = Depends(require_human), db: Session = Depends(get_db) ) -> dict: - job = _own_job(db, user, job_id) + job = _own_job_for_update(db, user, job_id) service.delete(db, job) return {"deleted": job_id} diff --git a/backend/app/worker.py b/backend/app/worker.py index 24138a11..ff9a1485 100644 --- a/backend/app/worker.py +++ b/backend/app/worker.py @@ -38,7 +38,6 @@ from app.downloads.jobdb import ( _edit_staging_dir, _requeue_if_running, _reset_asset_pending, - _set_job, _staging_dir, ) from app.downloads.status import STOPPED_BY_ROUTE, JobStatus @@ -254,7 +253,27 @@ def _fail_after_kill( """`_stop_and_free` + mark the job errored — for the watchdog's stall/timeout kills and the supervisor-error fallback.""" _stop_and_free(job_id, asset_id, proc, grace_s) - _set_job(job_id, status="error", error=error, error_code=error_code, phase=None) + _fail_running(job_id, error=error, error_code=error_code) + + +def _fail_running( + job_id: int, *, error: str, error_code: str | None, error_detail: str | None = None +) -> None: + """Fail a claimed job — only while it is still `running` (`service.finalize_if_running`), so a + cancel/pause a route landed first is never turned back into `error`, which holds the asset the + route already released. `error_detail` is written every time: a failure never shows an earlier + failure's explanation.""" + with SessionLocal() as db: + service.finalize_if_running( + db, + job_id, + status=JobStatus.ERROR, + error=error, + error_code=error_code, + error_detail=error_detail, + phase=None, + ) + db.commit() def _run_supervised(job_id: int, asset_id: int) -> None: @@ -330,12 +349,10 @@ def _run_supervised(job_id: int, asset_id: int) -> None: status, _ = _job_liveness(job_id) if status == "running": _cleanup_staging(asset_id) - _set_job( + _fail_running( job_id, - status="error", - error_code="unknown", - phase=None, error=f"The download process exited unexpectedly (code {proc.returncode}).", + error_code="unknown", ) log.warning( "job %s: child exited %s without finishing", job_id, proc.returncode @@ -403,32 +420,28 @@ def _process_job(job_id: int) -> None: if asset_status == "error": # A job that meets an already-failed asset gets the SAME explanation the job that failed # it got — not a bare "Download failed". Only the text is on the asset, so it is - # diagnosed again, for this link, with the cookie state of that asset: a user's private - # (cookie-salted) asset failed with their cookie attached, the shared one without. + # diagnosed again, for this link, and whether the user's cookie went with the download + # is asked of the same gate the download child used (`credentials.usable_cookie`). url = service.source_url(job.source_kind, job.source_ref) - diagnosis = dlerrors.diagnose( - asset_error, - url=url, - cookie_attached=credentials.is_authed_asset( - asset, formats.normalize(job.profile_snapshot), job.user_id, url - ), + cookie = credentials.usable_cookie( + db, job.user_id, url, asset, formats.normalize(job.profile_snapshot) ) + diagnosis = dlerrors.diagnose(asset_error, url=url, cookie_attached=cookie is not None) + del cookie # only whether there is one matters here if asset is None: - _set_job(job_id, status="error", error="No media asset", phase=None) + _fail_running(job_id, error="No media asset", error_code=None) return if asset_status == "ready": _finish_from_cache(job_id, asset_id) return if asset_status == "error": - _set_job( + _fail_running( job_id, - status="error", error=asset_error or "Download failed", error_code=diagnosis.code, error_detail=diagnosis.detail, - phase=None, ) return if asset_status == "downloading": @@ -482,7 +495,7 @@ def _worker_loop(idx: int) -> None: _process_job(job_id) except Exception: # noqa: BLE001 log.exception("processing job %s crashed", job_id) - _set_job(job_id, status="error", error="Worker error", phase=None) + _fail_running(job_id, error="Worker error", error_code=None) def main() -> None: diff --git a/backend/requirements.txt b/backend/requirements.txt index 63f9a24b..c562cfff 100644 --- a/backend/requirements.txt +++ b/backend/requirements.txt @@ -25,7 +25,8 @@ yt-dlp[default]>=2025.5.22 # links answer a signed-in plain request with "Cannot parse data" and only parse as Chrome (E11.5, # measured). The range is YT-DLP's, not ours — yt-dlp 2026.07.04 imports curl_cffi only in # 0.10.x–0.15.x and silently reports every target "unavailable" outside it. Move it when bumping -# yt-dlp; `test_adapters` fails the gate if the locked pair can no longer impersonate. +# yt-dlp; `tests/test_facebook_impersonation.py::test_the_locked_curl_cffi_can_impersonate_chrome` +# fails the gate if the locked pair can no longer impersonate. curl_cffi>=0.10,<0.16 # PO Token provider plugin: talks to the bgutil sidecar (brainicism/bgutil-ytdlp-pot-provider) # to mint YouTube Proof-of-Origin tokens on demand, bypassing "confirm you're not a bot" and diff --git a/backend/tests/test_download_credentials.py b/backend/tests/test_download_credentials.py index b5bc2ab6..94412ba4 100644 --- a/backend/tests/test_download_credentials.py +++ b/backend/tests/test_download_credentials.py @@ -264,9 +264,22 @@ def _held_job(db, u, *, sig, asset_status="error", job_status="error"): class TestRequeueTakesTheCookie: spec = formats.normalize({}) - def test_retry_after_adding_a_cookie_moves_the_job_to_the_private_asset(self, db, tmp_path): + @pytest.mark.parametrize( + ("asset_status", "job_status"), + # A failed download; and a job paused before it ever started (its asset never downloaded). + [("error", "error"), ("pending", "paused")], + ) + def test_retry_after_adding_a_cookie_moves_the_job_to_the_private_asset( + self, db, tmp_path, asset_status, job_status + ): u = _user(db) - shared, j = _held_job(db, u, sig=formats.format_sig(self.spec)) + shared, j = _held_job( + db, + u, + sig=formats.format_sig(self.spec), + asset_status=asset_status, + job_status=job_status, + ) credentials.upsert_credential(db, u.id, "facebook", COOKIES) db.commit() diff --git a/backend/tests/test_download_errors.py b/backend/tests/test_download_errors.py index e85d3b93..0745e898 100644 --- a/backend/tests/test_download_errors.py +++ b/backend/tests/test_download_errors.py @@ -1,5 +1,6 @@ """Category classification of raw yt-dlp / ffmpeg download-failure strings (S2).""" +from app.downloads import adapters from app.downloads import errors as dlerrors from app.downloads.errors import classify @@ -102,21 +103,36 @@ def test_facebook_cannot_parse_data_with_a_cookie_is_the_cookie_refused(): def test_a_facebook_group_post_without_formats_names_the_link(): - assert dlerrors.diagnose(_FB_NO_FORMATS, url=_GROUP_POST, cookie_attached=True) == ( + # Without a cookie yt-dlp is given the post link itself (measured, prod jobs 66 and 145). + assert dlerrors.diagnose(_FB_NO_FORMATS, url=_GROUP_POST) == ( "format_unavailable", "facebook.group_post_video", ) - # A reel with no formats is another failure: only the category. - assert dlerrors.diagnose(_FB_NO_FORMATS, url=_REEL, cookie_attached=True) == ( + # With one it is only ever given the post's VIDEO link, so the text is about that video: the adapter + # found it, and "this post has no video" would be false. + assert dlerrors.diagnose(_FB_NO_FORMATS, url=_GROUP_POST, cookie_attached=True) == ( "format_unavailable", None, ) + # A reel, or a group link that is not a post, is another failure: only the category. + for url in (_REEL, "https://www.facebook.com/groups/983554479435596/"): + assert dlerrors.diagnose(_FB_NO_FORMATS, url=url) == ("format_unavailable", None), url + + +def test_our_post_not_shown_verdict_is_the_refused_cookie(): + # The adapter's text when a signed-in read of the post got a page without the post (the login page). + msg = adapters.POST_NOT_SHOWN_MESSAGE + assert classify(msg) == "login_required" + assert dlerrors.diagnose(msg, url=_GROUP_POST, cookie_attached=True) == ( + "login_required", + "facebook.cookie_rejected", + ) def test_our_post_without_a_video_verdict_names_the_group_link(): - # The worker's own text when the post page holds no single video (S4): the message and its rule and + # The adapter's own text when the post page holds no single video (S4): the message and its rule and # catalog entry are pinned together, or an edit to the wording orphans the copy. - msg = dlerrors.POST_WITHOUT_VIDEO_MESSAGE + msg = adapters.POST_WITHOUT_VIDEO_MESSAGE assert classify(msg) == "format_unavailable" assert dlerrors.diagnose(msg, url=_GROUP_POST, cookie_attached=True) == ( "format_unavailable", diff --git a/backend/tests/test_download_failure_catalog.py b/backend/tests/test_download_failure_catalog.py index 9ae95199..44bc5542 100644 --- a/backend/tests/test_download_failure_catalog.py +++ b/backend/tests/test_download_failure_catalog.py @@ -6,13 +6,21 @@ never outlives the failure it describes: - the download child's failure branch (`pipeline._download`); - a second job meeting an already-failed asset (`worker._process_job`), which only has the text; - Retry / Re-download, which requeue the job; -- the `0084` backfill of the rows that failed before the catalog existed. +- the `0084` backfill of the rows that failed before the catalog existed; +- the downloads list, which is how the entry reaches the UI. +It also holds the one success-path check of the same child: the duration it stores (E11.5 S3). """ +import shutil +import subprocess + +import pytest from alembic import command from alembic.config import Config +from cryptography.fernet import Fernet from sqlalchemy import text +from app import security from app.config import settings from app.db import engine from app.downloads import credentials, formats, pipeline, service @@ -25,6 +33,13 @@ _FB_PARSE = ( "ERROR: [facebook] 1736360027626346: Cannot parse data; please report this issue on " "https://github.com/yt-dlp/yt-dlp/issues?q= , filling out the appropriate issue template." ) +_COOKIES = "# Netscape HTTP Cookie File\n.facebook.com\tTRUE\t/\tTRUE\t0\tc_user\t123\n" + + +@pytest.fixture +def _fernet(monkeypatch): + # A throwaway key, so a stored credential encrypts and decrypts (mirrors test_download_credentials). + monkeypatch.setattr(security, "_fernet", Fernet(Fernet.generate_key())) def _user(db, email): @@ -129,8 +144,9 @@ def test_a_job_meeting_a_failed_shared_asset_gets_the_same_explanation(db): ) -def test_a_job_meeting_its_users_failed_private_asset_knows_the_cookie_was_used(db): +def test_a_job_meeting_its_users_failed_private_asset_knows_the_cookie_was_used(db, _fernet): u = _user(db, "fb-private@example.com") + credentials.upsert_credential(db, u.id, "facebook", _COOKIES) sig = credentials.authed_format_sig(formats.normalize({}), u.id, "facebook") _, j = _asset_and_job( db, u, sig=sig, asset_status="error", job_status="running", error=_FB_PARSE @@ -143,6 +159,100 @@ def test_a_job_meeting_its_users_failed_private_asset_knows_the_cookie_was_used( assert (job.error_code, job.error_detail) == ("login_required", "facebook.cookie_rejected") +def test_a_private_asset_the_cookie_could_not_go_with_is_explained_as_the_download_had_it(db): + # The cookie was deleted (or can no longer be decrypted) after the jobs were queued: the download + # child had nothing to attach and said "add your cookies". The job meeting that failed asset must say + # the same — the asset's salt alone would have told it "your cookies were refused". + u = _user(db, "fb-gone@example.com") + sig = credentials.authed_format_sig(formats.normalize({}), u.id, "facebook") + _, j = _asset_and_job( + db, u, sig=sig, asset_status="error", job_status="running", error=_FB_PARSE + ) + + _process_job(j.id) + + db.expire_all() + job = db.get(DownloadJob, j.id) + assert (job.error_code, job.error_detail) == ("login_required", "facebook.login_wall") + + +def test_a_job_canceled_before_it_meets_a_failed_asset_stays_canceled(db): + # The cancel landed between the claim and this write. `error` would hold the asset again after the + # route already released it — a later delete releases twice. + u = _user(db, "fb-canceled@example.com") + _, j = _asset_and_job( + db, u, sig="shared", asset_status="error", job_status="canceled", error=_FB_PARSE + ) + + _process_job(j.id) + + db.expire_all() + job = db.get(DownloadJob, j.id) + assert (job.status, job.error_code, job.error_detail) == ("canceled", None, None) + + +def test_the_downloads_list_carries_the_catalog_entry(db, as_user): + # The whole explanation rides on this one field: without it every row falls back to the category. + u = _user(db, "fb-list@example.com") + _, j = _asset_and_job( + db, u, sig="shared", asset_status="error", job_status="error", error=_FB_PARSE + ) + j.error_code, j.error_detail = "login_required", "facebook.login_wall" + db.commit() + + [row] = as_user(u).get("/api/downloads").json() + + assert (row["error_code"], row["error_detail"]) == ("login_required", "facebook.login_wall") + + +@pytest.mark.skipif(shutil.which("ffmpeg") is None, reason="ffmpeg not available") +def test_a_download_yt_dlp_gave_no_duration_stores_the_files_duration(db, monkeypatch, tmp_path): + # Facebook reel 257 (E11.5): yt-dlp's info had no duration, the asset stored NULL, and the Library + # row lost its trim action. Through the real finalize, with a real 3-second file. + u = _user(db, "fb-duration@example.com") + a, j = _asset_and_job(db, u, sig="shared", asset_status="downloading", job_status="running") + monkeypatch.setattr(settings, "download_root", str(tmp_path)) + monkeypatch.setattr(pipeline.urlguard, "check_url", lambda url: None) + + def extract(job_id, asset_id, url, spec, staging, adapter, **kwargs): + media = staging / "1736360027626346.mp4" + subprocess.run( + [ + "ffmpeg", + "-y", + "-loglevel", + "error", + "-f", + "lavfi", + "-i", + "testsrc=size=64x64:rate=10", + "-f", + "lavfi", + "-i", + "sine=frequency=440", + "-t", + "3", + "-c:v", + "libx264", + "-pix_fmt", + "yuv420p", + "-c:a", + "aac", + str(media), + ], + check=True, + ) + return {"id": "1736360027626346", "ext": "mp4", "title": "Reel", "filepath": str(media)} + + monkeypatch.setattr(pipeline, "_extract_with_fallback", extract) + + pipeline._download(j.id, a.id, "url", _REEL, {}, u.id) + + db.expire_all() + assert db.get(DownloadJob, j.id).status == "done" + assert db.get(MediaAsset, a.id).duration_s == 3 + + def test_retry_and_redownload_drop_the_old_explanation(db): u = _user(db, "fb-requeue@example.com") for requeue in (service.resume, service.redownload): diff --git a/backend/tests/test_download_job_locks.py b/backend/tests/test_download_job_locks.py new file mode 100644 index 00000000..3ce5511b --- /dev/null +++ b/backend/tests/test_download_job_locks.py @@ -0,0 +1,119 @@ +"""Overlapping requests on ONE job (E11.5 review) — DB-backed, two real sessions. + +The routes that move a job's asset ref count (resume, cancel, delete, redownload) used to decide on an +unlocked read: two overlapping requests for one job (a double-click, a second tab, a replay after a +gateway timeout) both saw it holding its asset and both released the hold. The count then reaches 0 +while another job still holds the asset, and GC or the ready-file delete removes it under that job. +`_own_job_for_update` makes the second request wait and decide on the first one's committed result. +""" + +import threading + +import pytest +from fastapi import HTTPException + +from app.db import SessionLocal +from app.downloads import service +from app.models import DownloadJob, MediaAsset, User +from app.routes.downloads import _own_job_for_update + +_REF = "https://example.com/video" + + +def _asset_held_by_two_jobs(db): + mine, theirs = User(email="lock-mine@example.com"), User(email="lock-theirs@example.com") + db.add_all([mine, theirs]) + db.flush() + asset = MediaAsset( + source_kind="url", source_ref=_REF, format_sig="shared", status="error", ref_count=2 + ) + db.add(asset) + db.flush() + jobs = [ + DownloadJob( + user_id=owner.id, + asset_id=asset.id, + source_kind="url", + source_ref=_REF, + status="error", + queue_pos=0, + profile_snapshot={}, + ) + for owner in (mine, theirs) + ] + db.add_all(jobs) + db.commit() + return mine, asset, jobs[0] + + +@pytest.mark.parametrize("transition", [service.cancel, service.delete]) +def test_overlapping_requests_for_one_job_release_its_hold_once(db, transition): + user, asset, job = _asset_held_by_two_jobs(db) + first, second = SessionLocal(), SessionLocal() + finished = threading.Event() + + def the_other_request(): + try: + transition(second, _own_job_for_update(second, user, job.id)) + except HTTPException: + pass # a delete after the delete: the job is gone, a 404 + finally: + second.close() + finished.set() + + try: + locked = _own_job_for_update(first, user, job.id) + other = threading.Thread(target=the_other_request) + other.start() + # The second request waits for the first one's locks instead of deciding on the same row. + assert not finished.wait(0.5), "the second request did not wait for the first" + transition(first, locked) + other.join(10) + assert finished.is_set() + finally: + first.close() + + db.expire_all() + assert db.get(MediaAsset, asset.id).ref_count == 1 # the other user's job still holds it + + +def test_losing_the_asset_insert_race_keeps_the_callers_own_writes(db, monkeypatch): + # Retry clears the job's error, THEN may create the cookie asset. When another request inserted that + # asset first, the loser used to roll the whole session back — and requeue the job with its stale + # error, which a stall or crash later leaves showing under the new failure. + user = User(email="race@example.com") + db.add(user) + db.flush() + job = DownloadJob( + user_id=user.id, + source_kind="url", + source_ref=_REF, + status="error", + error="the old failure", + queue_pos=0, + profile_snapshot={}, + ) + db.add(job) + db.commit() + job.error = None # the caller's pending write + + real_flush, raced = db.flush, [] + + def flush_after_the_other_request_inserted(*args, **kwargs): + if not raced: # the other request commits the same asset between our lookup and our insert + raced.append(True) + with SessionLocal() as other: + other.add( + MediaAsset(source_kind="url", source_ref=_REF, format_sig="raced", ref_count=0) + ) + other.commit() + return real_flush(*args, **kwargs) + + monkeypatch.setattr(db, "flush", flush_after_the_other_request_inserted) + + asset = service.get_or_create_asset(db, "url", _REF, "raced") + db.commit() + + assert raced and asset.format_sig == "raced" + db.expire_all() + assert db.get(DownloadJob, job.id).error is None diff --git a/backend/tests/test_facebook_impersonation.py b/backend/tests/test_facebook_impersonation.py index 186fa7ea..86b4fb83 100644 --- a/backend/tests/test_facebook_impersonation.py +++ b/backend/tests/test_facebook_impersonation.py @@ -15,6 +15,7 @@ page holds ~10 other videos), and its watch link downloads. import pytest import yt_dlp +from yt_dlp.networking.exceptions import TransportError from yt_dlp.networking.impersonate import ImpersonateTarget from app.downloads import adapters, pipeline @@ -59,10 +60,11 @@ def test_the_locked_curl_cffi_can_impersonate_chrome(): @pytest.fixture def ydl(monkeypatch): """Fake `yt_dlp.YoutubeDL`: records each attempt's options and plays back `outcomes` in order; - `web["page"]` is what a page read returns, and `web` records the pages opened and links extracted.""" + `web["page"]` is what a page read returns (`web["fail"]`, when set, is raised instead), and `web` + records the pages opened, the byte cap each read asked for, and the links extracted.""" seen: list[dict] = [] outcomes: list = [] - web: dict = {"page": "", "opened": [], "extracted": []} + web: dict = {"page": "", "fail": None, "opened": [], "reads": [], "extracted": []} class FakeYDL: def __init__(self, opts): @@ -76,10 +78,13 @@ def ydl(monkeypatch): def urlopen(self, url): web["opened"].append(url) + if web["fail"] is not None: + raise web["fail"] body = web["page"].encode() class Response: def read(self, amt=None): + web["reads"].append(amt) return body return Response() @@ -145,6 +150,8 @@ def test_a_cookie_group_post_downloads_the_posts_own_video(ydl, tmp_path): assert _run(tmp_path, "/cookies.txt", _GROUP_LINK) == {"id": _POST_VIDEO} assert web["opened"] == [_GROUP_LINK] + # Never an unbounded read. + assert web["reads"] == [adapters.FacebookAdapter._POST_PAGE_MAX_BYTES] assert seen[0]["cookiefile"] == "/cookies.txt" # the post page is read signed in watch = f"https://www.facebook.com/watch/?v={_POST_VIDEO}" # Never the post link: that is where the feed video comes from. @@ -166,6 +173,37 @@ def test_a_group_post_without_a_single_video_fails_saying_so(ydl, tmp_path): ) +def test_a_group_post_page_without_the_post_is_the_refused_cookie(ydl, tmp_path): + # Measured with a refused cookie (E11.5 review): Facebook answers the post link with its login page — + # a login form, no post id anywhere. Saying "this post has no video" would blame the post. + _, _, web = ydl + web["page"] = 'ChatGPT magyarul
' + + with pytest.raises(RuntimeError) as exc: + _run(tmp_path, "/cookies.txt", _GROUP_LINK) + + assert web["extracted"] == [] + assert dlerrors.diagnose(str(exc.value), url=_GROUP_LINK, cookie_attached=True) == ( + "login_required", + "facebook.cookie_rejected", + ) + + +def test_a_failed_post_page_read_fails_the_download_as_itself(ydl, tmp_path): + # A read that failed says nothing about the post: it must not turn into "no video in this post". + _, _, web = ydl + web["fail"] = TransportError("Connection reset by peer") + + with pytest.raises(TransportError) as exc: + _run(tmp_path, "/cookies.txt", _GROUP_LINK) + + assert web["extracted"] == [] + assert dlerrors.diagnose(str(exc.value), url=_GROUP_LINK, cookie_attached=True) == ( + "network", + None, + ) + + def test_without_a_cookie_the_group_post_link_goes_to_ytdlp_unchanged(ydl, tmp_path): _, outcomes, web = ydl outcomes += [_NO_FORMATS] diff --git a/backend/tests/test_worker_supervisor.py b/backend/tests/test_worker_supervisor.py index b62fe553..1898bc6f 100644 --- a/backend/tests/test_worker_supervisor.py +++ b/backend/tests/test_worker_supervisor.py @@ -51,7 +51,7 @@ def rec(monkeypatch): monkeypatch.setattr(worker, "_reset_asset_pending", record("reset_asset")) monkeypatch.setattr(worker, "_requeue_if_running", record("requeue")) - monkeypatch.setattr(worker, "_set_job", record("set_job")) + monkeypatch.setattr(worker, "_fail_running", record("fail")) monkeypatch.setattr(worker, "_cleanup_staging", record("cleanup")) monkeypatch.setattr(worker, "_stop_child", record("stop_child")) monkeypatch.setattr(worker._stop, "is_set", lambda: False) @@ -84,9 +84,9 @@ def test_no_progress_stall_kills_and_fails_the_job(monkeypatch, rec): worker._run_supervised(1, 99) names = _names(rec) assert "stop_child" in names and "reset_asset" in names - set_job = next(c for c in rec if c[0] == "set_job") - assert set_job[2]["status"] == "error" and set_job[2]["error_code"] == "network" - assert "stalled" in set_job[2]["error"].lower() + fail = next(c for c in rec if c[0] == "fail") + assert fail[2]["error_code"] == "network" + assert "stalled" in fail[2]["error"].lower() def test_route_pause_kills_but_keeps_the_route_status(monkeypatch, rec): @@ -97,7 +97,7 @@ def test_route_pause_kills_but_keeps_the_route_status(monkeypatch, rec): worker._run_supervised(1, 99) names = _names(rec) assert "stop_child" in names and "reset_asset" in names - assert "set_job" not in names and "requeue" not in names + assert "fail" not in names and "requeue" not in names def test_route_cancel_kills_but_keeps_the_route_status(monkeypatch, rec): @@ -110,7 +110,7 @@ def test_route_cancel_kills_but_keeps_the_route_status(monkeypatch, rec): worker._run_supervised(1, 99) names = _names(rec) assert "stop_child" in names and "reset_asset" in names - assert "set_job" not in names and "requeue" not in names + assert "fail" not in names and "requeue" not in names def test_wall_clock_cap_kills_even_while_progressing(monkeypatch, rec): @@ -118,9 +118,8 @@ def test_wall_clock_cap_kills_even_while_progressing(monkeypatch, rec): _spawn(monkeypatch, _FakeProc(["timeout"])) _liveness(monkeypatch, ("running", 1.0)) # fresh progress, but over the absolute cap worker._run_supervised(1, 99) - set_job = next(c for c in rec if c[0] == "set_job") - assert set_job[2]["status"] == "error" - assert "time limit" in set_job[2]["error"].lower() + fail = next(c for c in rec if c[0] == "fail") + assert "time limit" in fail[2]["error"].lower() def test_child_crash_without_finishing_is_failed(monkeypatch, rec): @@ -130,8 +129,8 @@ def test_child_crash_without_finishing_is_failed(monkeypatch, rec): worker._run_supervised(1, 99) names = _names(rec) assert "reset_asset" in names - set_job = next(c for c in rec if c[0] == "set_job") - assert set_job[2]["status"] == "error" and "unexpectedly" in set_job[2]["error"].lower() + fail = next(c for c in rec if c[0] == "fail") + assert "unexpectedly" in fail[2]["error"].lower() def test_child_nonzero_exit_that_already_set_state_is_left_alone(monkeypatch, rec): @@ -139,7 +138,7 @@ def test_child_nonzero_exit_that_already_set_state_is_left_alone(monkeypatch, re _spawn(monkeypatch, _FakeProc([("exit", 1)])) _liveness(monkeypatch, ("error", None)) worker._run_supervised(1, 99) - assert "set_job" not in _names(rec) + assert "fail" not in _names(rec) def test_spawn_failure_frees_the_asset_and_requeues(monkeypatch, rec): @@ -158,7 +157,7 @@ def test_shutdown_kills_and_requeues_for_next_boot(monkeypatch, rec): _liveness(monkeypatch, ("running", 1.0)) worker._run_supervised(1, 99) names = _names(rec) - assert "stop_child" in names and "requeue" in names and "set_job" not in names + assert "stop_child" in names and "requeue" in names and "fail" not in names def test_a_long_download_survives_while_progress_stays_fresh(monkeypatch, rec): @@ -191,7 +190,7 @@ def test_a_wedged_finished_child_is_killed_by_the_wall_clock(monkeypatch, rec): _liveness(monkeypatch, ("done", 1.0)) # finished, but the process lingers worker._run_supervised(1, 99) names = _names(rec) - assert "stop_child" in names and "set_job" not in names # killed to free the slot, status kept + assert "stop_child" in names and "fail" not in names # killed to free the slot, status kept def test_a_watchdog_error_still_stops_the_child(monkeypatch, rec): @@ -206,7 +205,7 @@ def test_a_watchdog_error_still_stops_the_child(monkeypatch, rec): worker._run_supervised(1, 99) names = _names(rec) assert "stop_child" in names and "reset_asset" in names - assert next(c for c in rec if c[0] == "set_job")[2]["status"] == "error" + assert "fail" in _names(rec) # --- _stop_child escalation ----------------------------------------------------------------- diff --git a/frontend/src/components/DownloadCenter.confirm.dom.test.tsx b/frontend/src/components/DownloadCenter.confirm.dom.test.tsx new file mode 100644 index 00000000..f5edb5f2 --- /dev/null +++ b/frontend/src/components/DownloadCenter.confirm.dom.test.tsx @@ -0,0 +1,103 @@ +// @vitest-environment jsdom +// +// The Download Center's confirm dialogs name their ACTION on the confirm button (E11.5): the shared +// `confirmThen` once labelled it with the dialog's question — "Re-download this file?" on the button, +// and the cancel dialog's own "Cancel" beside "Cancel". i18n is key-passthrough, so labels read as keys. +import { afterEach, describe, expect, it, vi } from "vitest"; +import { cleanup, fireEvent, render, screen, waitFor } from "@testing-library/react"; +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import type { DownloadJob } from "../lib/api"; + +const mocks = vi.hoisted(() => ({ + jobs: [] as unknown[], + confirm: vi.fn(async (_opts: { title: string; confirmLabel?: string }) => false), +})); + +vi.mock("react-i18next", async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, useTranslation: () => ({ t: (key: string) => key }) }; +}); +vi.mock("./ConfirmProvider", () => ({ useConfirm: () => mocks.confirm })); +vi.mock("../lib/useLiveQuery", () => ({ + useLiveQuery: (key: readonly unknown[]) => ({ + data: key[0] === "downloads" ? mocks.jobs : undefined, + isError: false, + isLoading: false, + refetch: vi.fn(), + }), +})); +vi.mock("../lib/useMe", () => ({ useMe: () => ({ role: "user" }) })); +// The page toolbar (title + tabs) renders into the app shell's slot, which a bare render has none of. +vi.mock("./ui/PageShell", async (importOriginal) => ({ + ...(await importOriginal()), + PageToolbar: ({ children }: { children: React.ReactNode }) => <>{children}, +})); + +import DownloadCenter from "./DownloadCenter"; + +const job = (id: number, status: DownloadJob["status"]): DownloadJob => ({ + id, + status, + progress: 0, + speed_bps: null, + eta_s: null, + phase: null, + error: null, + error_code: null, + error_detail: null, + queue_pos: 0, + created_at: null, + source_kind: "youtube", + source_ref: `v${id}`, + source_url: null, + poster_url: null, + profile_id: null, + spec: { + mode: "av", + max_height: null, + container: null, + audio_format: null, + vcodec: null, + embed_subs: false, + embed_chapters: false, + embed_thumbnail: false, + sponsorblock: false, + }, + display_name: `Clip ${id}`, + display_uploader: null, + display_uploader_url: null, + extra_links: [], + can_download: status === "done", + expired: false, + asset: null, +}); + +afterEach(() => { + cleanup(); + mocks.confirm.mockClear(); +}); + +describe("DownloadCenter confirm dialogs", () => { + it("label the confirm button with the action, not the dialog's question", async () => { + mocks.jobs = [job(1, "running"), job(2, "done")]; + render( + + + , + ); + + fireEvent.click(screen.getByTitle("downloads.actions.cancel")); + await waitFor(() => expect(mocks.confirm).toHaveBeenCalledTimes(1)); + // The tab's label shares its button with a count badge. + fireEvent.click(screen.getByRole("button", { name: /downloads\.tabs\.done/ })); + fireEvent.click(await screen.findByTitle("downloads.actions.redownload")); + fireEvent.click(screen.getByTitle("downloads.actions.delete")); + await waitFor(() => expect(mocks.confirm).toHaveBeenCalledTimes(3)); + + expect(mocks.confirm.mock.calls.map(([o]) => [o.title, o.confirmLabel])).toEqual([ + ["downloads.confirm.cancelTitle", "downloads.confirm.cancelAction"], + ["downloads.confirm.redownloadTitle", "downloads.confirm.redownloadAction"], + ["downloads.confirm.deleteTitle", "downloads.actions.delete"], + ]); + }); +}); diff --git a/frontend/src/components/downloadModals.tsx b/frontend/src/components/downloadModals.tsx index 0e618af8..e12ebdae 100644 --- a/frontend/src/components/downloadModals.tsx +++ b/frontend/src/components/downloadModals.tsx @@ -119,8 +119,9 @@ export function MetaEditModal({ job, onClose }: { job: DownloadJob; onClose: () } // Transient failures worth an in-modal Retry button; the rest are permanent (private/geo/DRM/ -// sign-in/unsupported-host/…), where a retry can't help — the queue row still keeps its own retry -// control regardless. `unknown` is deliberately NOT here: an unclassified failure is most often an +// unsupported-host/…), where a retry can't help, or need a fix outside this window first (sign-in: a +// cookie in Settings — its copy then sends the user to the row) — the queue row still keeps its own +// retry control regardless. `unknown` is deliberately NOT here: an unclassified failure is most often an // auth wall we couldn't parse (a login-walled Facebook post reads as a bare "no format"), so a // prominent Retry would mislead; force-retrying from the row stays available for genuine transients. const RETRYABLE_ERRORS = ["postprocess", "network"]; diff --git a/frontend/src/i18n/locales/en/downloads.json b/frontend/src/i18n/locales/en/downloads.json index fb73b553..55c0d9e4 100644 --- a/frontend/src/i18n/locales/en/downloads.json +++ b/frontend/src/i18n/locales/en/downloads.json @@ -294,11 +294,11 @@ "facebook": { "login_wall": { "short": "Facebook sign-in required", - "body": "Facebook only shows this to signed-in users (e.g. 18+ content or a closed group). Add your Facebook cookies under Settings → Account → Download authentication, then press Retry." + "body": "Facebook only shows this to signed-in users (e.g. 18+ content or a closed group). Add your Facebook cookies under Settings → Account → Download authentication, then retry the download from the list." }, "cookie_rejected": { "short": "Facebook didn't accept the cookies", - "body": "The cookies have probably expired, or you signed out in that browser. Export them again, upload them, and retry. If it still fails, the content has been removed or isn't visible to you." + "body": "The cookies have probably expired, or you signed out in that browser. Export them again, paste them in under Settings → Account → Download authentication, then retry the download from the list. If it still fails, the content has been removed or isn't visible to you." }, "group_post_video": { "short": "No video found in this post", diff --git a/frontend/src/i18n/locales/hu/downloads.json b/frontend/src/i18n/locales/hu/downloads.json index 1c2685fe..1bfecac3 100644 --- a/frontend/src/i18n/locales/hu/downloads.json +++ b/frontend/src/i18n/locales/hu/downloads.json @@ -157,9 +157,9 @@ "deleteBody": "Ez eltávolítja a listádról. A fájl a lejáratáig még a gyorsítótárban maradhat mások számára.", "removeSharedTitle": "Eltávolítod a listádból?", "removeSharedBody": "Csak a te megosztott listádból tűnik el — a tulajdonos fájlját nem törli.", - "redownloadTitle": "Újratöltöd ezt a fájlt?", - "redownloadBody": "A jelenlegi fájl törlődik és újra letöltődik a legfrissebb formátum- és elnevezési szabályokkal. A már elküldött megosztási linkek működnek tovább, és az új fájlra fognak mutatni (sikertelen újratöltés esetén a link törötten marad).", - "redownloadAction": "Újratöltés" + "redownloadTitle": "Újraletöltöd ezt a fájlt?", + "redownloadBody": "A jelenlegi fájl törlődik és újra letöltődik a legfrissebb formátum- és elnevezési szabályokkal. A már elküldött megosztási linkek működnek tovább, és az új fájlra fognak mutatni (sikertelen újraletöltés esetén a link törötten marad).", + "redownloadAction": "Újraletöltés" }, "profiles": { "manage": "Formátumok kezelése", @@ -251,7 +251,7 @@ }, "login_required": { "short": "Bejelentkezés szükséges", - "body": "Ehhez a videóhoz bejelentkezett fiók kell (korhatáros, csak tagoknak szóló vagy zárt csoport). Töltsd fel a cookie-kat ehhez az oldalhoz a Beállítások → Fiók → Letöltés-hitelesítés alatt a letöltéshez." + "body": "Ehhez a videóhoz bejelentkezett fiók kell (korhatáros, csak tagoknak szóló vagy zárt csoport). Illeszd be a cookie-kat ehhez az oldalhoz a Beállítások → Fiók → Letöltés-hitelesítés alatt a letöltéshez." }, "unsupported_host": { "short": "Nem támogatott oldal", @@ -294,11 +294,11 @@ "facebook": { "login_wall": { "short": "Facebook-bejelentkezés szükséges", - "body": "A Facebook ezt csak bejelentkezve mutatja (pl. 18+ tartalom vagy zárt csoport). Add meg a Facebook-cookie-kat a Beállítások → Fiók → Letöltés-hitelesítés alatt, majd nyomd meg az Újra gombot." + "body": "A Facebook ezt csak bejelentkezve mutatja (pl. 18+ tartalom vagy zárt csoport). Illeszd be a Facebook-cookie-kat a Beállítások → Fiók → Letöltés-hitelesítés alatt, majd a listában indítsd újra a letöltést." }, "cookie_rejected": { - "short": "A Facebook nem fogadta el a cookie-t", - "body": "A cookie valószínűleg lejárt, vagy kijelentkeztél abban a böngészőben. Exportáld újra, töltsd fel, és próbáld újra. Ha így sem megy, a tartalom törölt, vagy számodra nem látható." + "short": "A Facebook nem fogadta el a cookie-kat", + "body": "A cookie-k valószínűleg lejártak, vagy kijelentkeztél abban a böngészőben. Exportáld újra őket, illeszd be a Beállítások → Fiók → Letöltés-hitelesítés alatt, majd a listában indítsd újra a letöltést. Ha így sem megy, a tartalmat törölték, vagy nem látható számodra." }, "group_post_video": { "short": "Ebben a posztban nem található videó", diff --git a/frontend/src/lib/downloadFailure.test.ts b/frontend/src/lib/downloadFailure.test.ts index ed290cef..9c4039ff 100644 --- a/frontend/src/lib/downloadFailure.test.ts +++ b/frontend/src/lib/downloadFailure.test.ts @@ -15,9 +15,11 @@ describe("failCopyKeys", () => { ).toEqual([ "downloads.errors.fail.catalog.facebook.login_wall.body", "downloads.errors.fail.cat.login_required.body", + "downloads.errors.fail.cat.unknown.body", ]); expect(failCopyKeys({ error_code: "network", error_detail: null }, "short")).toEqual([ "downloads.errors.fail.cat.network.short", + "downloads.errors.fail.cat.unknown.short", ]); expect(failCopyKeys({ error_code: null, error_detail: null }, "short")).toEqual([ "downloads.errors.fail.cat.unknown.short", @@ -39,5 +41,9 @@ describe("failCopyKeys", () => { failCopyKeys({ error_code: "login_required", error_detail: "facebook.newer" }, "short"), ), ).toBe("Sign-in required"); + // A category this client has no copy for (a newer backend): the generic sentence, never a key. + expect(i18n.t(failCopyKeys({ error_code: "rate_limited", error_detail: null }, "short"))).toBe( + "Download failed", + ); }); }); diff --git a/frontend/src/lib/downloadFailure.ts b/frontend/src/lib/downloadFailure.ts index 58c77fd1..2b55c329 100644 --- a/frontend/src/lib/downloadFailure.ts +++ b/frontend/src/lib/downloadFailure.ts @@ -3,15 +3,18 @@ import type { DownloadJob } from "./api"; // The i18n keys that explain a failed download, most specific first — the list form `t()` takes, so // the first key a locale has wins. A failure-catalog entry (`error_detail`, e.g. `facebook.login_wall`) // names what went wrong on that site and what to do; the category (`error_code`) is the generic -// sentence, and the fallback when an older client meets a detail it has no copy for. One helper for -// the queue row's headline and the failure window, so the two can never explain the same job -// differently. +// sentence, and the fallback when an older client meets a detail it has no copy for; a category it has +// no copy for falls back to the unknown one, so the row never prints a key. One helper for the queue +// row's headline and the failure window, so the two can never explain the same job differently. export function failCopyKeys( job: Pick, part: "short" | "body", ): string[] { - const category = `downloads.errors.fail.cat.${job.error_code || "unknown"}.${part}`; - return job.error_detail - ? [`downloads.errors.fail.catalog.${job.error_detail}.${part}`, category] - : [category]; + const category = (code: string) => `downloads.errors.fail.cat.${code}.${part}`; + const code = job.error_code || "unknown"; + return [ + ...(job.error_detail ? [`downloads.errors.fail.catalog.${job.error_detail}.${part}`] : []), + category(code), + ...(code === "unknown" ? [] : [category("unknown")]), + ]; } From 2a9ddb78ac804e0d44c622fa773373cff67026c4 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Mon, 14 Sep 2026 01:42:57 +0200 Subject: [PATCH 09/10] fix(downloads): E11.5 epic-end review round 2 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. --- backend/app/downloads/adapters.py | 12 ++++--- backend/app/downloads/credentials.py | 6 ++-- backend/app/downloads/pipeline.py | 6 ++-- backend/app/routes/downloads.py | 11 ++++--- backend/app/worker.py | 8 ++--- backend/tests/test_download_job_locks.py | 40 ++++++++++++++++++------ 6 files changed, 56 insertions(+), 27 deletions(-) diff --git a/backend/app/downloads/adapters.py b/backend/app/downloads/adapters.py index f2ec3a33..7e4705db 100644 --- a/backend/app/downloads/adapters.py +++ b/backend/app/downloads/adapters.py @@ -35,10 +35,14 @@ class SourceAdapter: # --- extraction (worker._extract_with_fallback / backfill) --------------------------------- def resolve_url(self, url: str, ydl_opts: dict) -> str: - """The link to hand yt-dlp for `url`, resolved BEFORE the first attempt. Raises a RuntimeError - carrying one of this module's verdicts when the source itself shows there is no single video to - download there. `ydl_opts` are the download's own options (cookie, timeouts, address family) for - any page this needs to read. Generic: `url`.""" + """The link to hand yt-dlp for `url`, resolved BEFORE the first attempt. `ydl_opts` are the + download's own options (cookie, timeouts, address family) for any page this needs to read. + + Raises a RuntimeError carrying one of this module's verdicts when the page read shows the + download cannot go ahead: the content is there but holds no single video + (`POST_WITHOUT_VIDEO_MESSAGE`), or the page is not the content at all — the login page a + refused cookie gets (`POST_NOT_SHOWN_MESSAGE`), never the no-video verdict. A failed page read + is raised as itself, so it is classified like any other download error. Generic: `url`.""" return url def client_sets(self, authed: bool = False) -> list: diff --git a/backend/app/downloads/credentials.py b/backend/app/downloads/credentials.py index 38785e17..cb6fa3cc 100644 --- a/backend/app/downloads/credentials.py +++ b/backend/app/downloads/credentials.py @@ -229,7 +229,9 @@ def usable_cookie( ) -> str | None: """The user's cookie text a download of `url` into `asset` goes with, or None — the ONE decision whether a download is signed in. The worker child writes it out for yt-dlp (`resolve_cookiefile`); - the worker asks it again for a job that inherits a failed asset, so both jobs are told the same. + the worker asks it again for a job that inherits a failed asset. That second ask sees the + credential as it is when the job is claimed: a cookie the user removed or added since the failed + download changes this job's explanation to fit the cookie its Retry would go with. The double gate that prevents cross-user leakage: 1. a credential must still exist for `url`'s host (host-scoped via `host_key_for_url`), AND @@ -252,7 +254,7 @@ def usable_cookie( if not raw: log.warning( "download cookie for user %s host %s could not be decrypted (rotated key?) — " - "proceeding without it", + "treated as no cookie", user_id, host_key, ) diff --git a/backend/app/downloads/pipeline.py b/backend/app/downloads/pipeline.py index 5b15fe0e..d7308661 100644 --- a/backend/app/downloads/pipeline.py +++ b/backend/app/downloads/pipeline.py @@ -326,8 +326,10 @@ def _extract_with_fallback( sources make a single attempt. Returns the info dict of the successful attempt. Before the first attempt the adapter may resolve `url` to the link that really holds the video (a - Facebook group post → its video's watch link); when the source shows no single video there, the - adapter raises its own verdict (adapters.POST_*_MESSAGE) instead of letting yt-dlp guess. + Facebook group post → its video's watch link); when the page shows no single video there, or is not + the post at all (a refused cookie's login page), the adapter raises its own verdict + (adapters.POST_*_MESSAGE) instead of letting yt-dlp guess, and a failed page read fails the + download as itself. `cookiefile` (S6) is the acting user's per-host cookies.txt, already gated by `credentials.resolve_cookiefile`; passed to every attempt so the fallback stays authenticated. diff --git a/backend/app/routes/downloads.py b/backend/app/routes/downloads.py index 91d56d2a..b128bc6e 100644 --- a/backend/app/routes/downloads.py +++ b/backend/app/routes/downloads.py @@ -174,10 +174,11 @@ def _own_job(db: Session, user: User, job_id: int) -> DownloadJob: def _own_job_for_update(db: Session, user: User, job_id: int) -> DownloadJob: - """`_own_job` for a transition that moves its asset's ref count (resume, cancel, delete, redownload). - The job is re-read under a row lock, so two overlapping requests for one job run one after the other - and the second decides on the first's committed result — never both releasing the same hold, which - can free a file another job still holds. + """`_own_job` for a transition that moves its asset's ref count (resume, cancel, delete, redownload) + or writes the status those decide on (pause). The job is re-read under a row lock, so two + overlapping requests for one job run one after the other and the second decides on the first's + committed result — never both releasing the same hold, and never a pause turning a job a cancel + just released back into a holder; either can free a file another job still holds. The asset is locked BEFORE the job: the order a finishing worker child takes them in (`service.finalize_if_running`), so the two can never deadlock.""" @@ -631,7 +632,7 @@ def update_download_meta( def pause_download( job_id: int, user: User = Depends(require_human), db: Session = Depends(get_db) ) -> dict: - job = _own_job(db, user, job_id) + job = _own_job_for_update(db, user, job_id) service.pause(db, job) return _serialize_job(db, job) diff --git a/backend/app/worker.py b/backend/app/worker.py index ff9a1485..359f5844 100644 --- a/backend/app/worker.py +++ b/backend/app/worker.py @@ -418,10 +418,10 @@ def _process_job(job_id: int) -> None: asset_status = asset.status if asset else None asset_error = asset.error if asset else None if asset_status == "error": - # A job that meets an already-failed asset gets the SAME explanation the job that failed - # it got — not a bare "Download failed". Only the text is on the asset, so it is - # diagnosed again, for this link, and whether the user's cookie went with the download - # is asked of the same gate the download child used (`credentials.usable_cookie`). + # A job that meets an already-failed asset gets that failure's explanation — not a bare + # "Download failed". Only the text is on the asset, so it is diagnosed again, for this + # link, with whether a cookie goes with it asked of the same gate the download child + # used (`credentials.usable_cookie`) — as it stands now, so the advice fits this job's Retry. url = service.source_url(job.source_kind, job.source_ref) cookie = credentials.usable_cookie( db, job.user_id, url, asset, formats.normalize(job.profile_snapshot) diff --git a/backend/tests/test_download_job_locks.py b/backend/tests/test_download_job_locks.py index 3ce5511b..f55dbf13 100644 --- a/backend/tests/test_download_job_locks.py +++ b/backend/tests/test_download_job_locks.py @@ -4,23 +4,31 @@ The routes that move a job's asset ref count (resume, cancel, delete, redownload unlocked read: two overlapping requests for one job (a double-click, a second tab, a replay after a gateway timeout) both saw it holding its asset and both released the hold. The count then reaches 0 while another job still holds the asset, and GC or the ready-file delete removes it under that job. -`_own_job_for_update` makes the second request wait and decide on the first one's committed result. +A pause racing a cancel did the same one step later: its unlocked write turned the released job back +into a holder, which a later delete releases again. `_own_job_for_update` makes the second request +wait and decide on the first one's committed result. """ import threading import pytest from fastapi import HTTPException +from sqlalchemy import select from app.db import SessionLocal from app.downloads import service from app.models import DownloadJob, MediaAsset, User -from app.routes.downloads import _own_job_for_update +from app.routes.downloads import ( + _own_job_for_update, + cancel_download, + delete_download, + pause_download, +) _REF = "https://example.com/video" -def _asset_held_by_two_jobs(db): +def _asset_held_by_two_jobs(db, status="error"): mine, theirs = User(email="lock-mine@example.com"), User(email="lock-theirs@example.com") db.add_all([mine, theirs]) db.flush() @@ -35,7 +43,7 @@ def _asset_held_by_two_jobs(db): asset_id=asset.id, source_kind="url", source_ref=_REF, - status="error", + status=status if owner is mine else "error", queue_pos=0, profile_snapshot={}, ) @@ -46,23 +54,33 @@ def _asset_held_by_two_jobs(db): return mine, asset, jobs[0] -@pytest.mark.parametrize("transition", [service.cancel, service.delete]) -def test_overlapping_requests_for_one_job_release_its_hold_once(db, transition): - user, asset, job = _asset_held_by_two_jobs(db) +@pytest.mark.parametrize( + ("status", "transition", "route", "left"), + [ + ("error", service.cancel, cancel_download, "canceled"), + ("error", service.delete, delete_download, None), + ("queued", service.cancel, pause_download, "canceled"), + ], +) +def test_overlapping_requests_for_one_job_release_its_hold_once( + db, status, transition, route, left +): + user, asset, job = _asset_held_by_two_jobs(db, status) + job_id = job.id # read before the delete case removes the row under the expired instance first, second = SessionLocal(), SessionLocal() finished = threading.Event() def the_other_request(): try: - transition(second, _own_job_for_update(second, user, job.id)) + route(job_id, user=user, db=second) except HTTPException: - pass # a delete after the delete: the job is gone, a 404 + pass # a request after the delete: the job is gone, a 404 finally: second.close() finished.set() try: - locked = _own_job_for_update(first, user, job.id) + locked = _own_job_for_update(first, user, job_id) other = threading.Thread(target=the_other_request) other.start() # The second request waits for the first one's locks instead of deciding on the same row. @@ -74,6 +92,8 @@ def test_overlapping_requests_for_one_job_release_its_hold_once(db, transition): first.close() db.expire_all() + mine = select(DownloadJob.status).where(DownloadJob.id == job_id) + assert db.execute(mine).scalar_one_or_none() == left assert db.get(MediaAsset, asset.id).ref_count == 1 # the other user's job still holds it From 17bfea8754fd1273ab1d867a5bc9c2f76383e128 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Mon, 14 Sep 2026 01:52:34 +0200 Subject: [PATCH 10/10] release: 0.86.0 notes --- CHANGELOG.md | 23 +++++++++++++++++++++++ VERSION | 2 +- frontend/src/lib/releaseNotes.ts | 22 ++++++++++++++++++++++ 3 files changed, 46 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 93de23a2..9d76ad55 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,29 @@ edit the release notes and re-run the generator. Upgrading, pinning a version and rolling back: [docs/self-hosting.md](docs/self-hosting.md#upgrading). +## 0.86.0 — 2026-09-14 + +A failed download now tells you why it failed and what you can do about it, instead of repeating yt-dlp's raw error. Facebook comes first: downloads that used to die on “Cannot parse data” go through, and a Facebook group post downloads its own video instead of a different one from the page. + +### Features + +- A failure says why, in your language: the row names the cause (“Facebook needs you to be signed in”, “Facebook did not accept your cookies”, “No video found in this post”) and its details window says what to do next. The raw error text is still there below it. +- A Facebook download with your cookies that stops at “Cannot parse data” is tried again as Chrome, which gets past it. +- A Facebook group post link downloads the post's own video. Before, yt-dlp either found nothing or quietly downloaded another video from the page; a post without a video now says so. +- Retry and Re-download use a cookie you added after the failure, so the Retry the message asks for actually goes out signed in. + +### Fixes + +- A Re-download of a download that needed your cookies used to rebuild without them. +- A job queued behind a download that already failed shows that failure's explanation instead of a bare “Download failed”. +- A download yt-dlp reports no length for takes its length from the file, so its Edit / trim action appears. +- Pressing Pause, Stop or Remove twice, or in two tabs, can no longer free a file another download still uses. +- The confirm buttons in Downloads name the action (Stop download, Re-download, Remove) instead of repeating the question. + +### Internal + +- Four sprints, each Facebook behaviour measured on the development server with a real cookie before it was built. Two epic-end review rounds (20 → 17 fixed, then a targeted 4 → 4), every regression test mutation-checked. + ## 0.85.0 — 2026-09-13 Siftlode can now be driven from the keyboard, and Back, dialogs and menus stop losing your place. j/k and the arrow keys walk every list and grid, Enter opens what you are on, and Back puts you on the item you opened a view from instead of at the top of the page. diff --git a/VERSION b/VERSION index 137c1281..63cd8847 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.85.0 +0.86.0 diff --git a/frontend/src/lib/releaseNotes.ts b/frontend/src/lib/releaseNotes.ts index 65288f7f..afa65b2e 100644 --- a/frontend/src/lib/releaseNotes.ts +++ b/frontend/src/lib/releaseNotes.ts @@ -14,6 +14,28 @@ export interface ReleaseEntry { } export const RELEASE_NOTES: ReleaseEntry[] = [ + { + version: "0.86.0", + date: "2026-09-14", + summary: + "A failed download now tells you why it failed and what you can do about it, instead of repeating yt-dlp's raw error. Facebook comes first: downloads that used to die on “Cannot parse data” go through, and a Facebook group post downloads its own video instead of a different one from the page.", + features: [ + "A failure says why, in your language: the row names the cause (“Facebook needs you to be signed in”, “Facebook did not accept your cookies”, “No video found in this post”) and its details window says what to do next. The raw error text is still there below it.", + "A Facebook download with your cookies that stops at “Cannot parse data” is tried again as Chrome, which gets past it.", + "A Facebook group post link downloads the post's own video. Before, yt-dlp either found nothing or quietly downloaded another video from the page; a post without a video now says so.", + "Retry and Re-download use a cookie you added after the failure, so the Retry the message asks for actually goes out signed in.", + ], + fixes: [ + "A Re-download of a download that needed your cookies used to rebuild without them.", + "A job queued behind a download that already failed shows that failure's explanation instead of a bare “Download failed”.", + "A download yt-dlp reports no length for takes its length from the file, so its Edit / trim action appears.", + "Pressing Pause, Stop or Remove twice, or in two tabs, can no longer free a file another download still uses.", + "The confirm buttons in Downloads name the action (Stop download, Re-download, Remove) instead of repeating the question.", + ], + chores: [ + "Four sprints, each Facebook behaviour measured on the development server with a real cookie before it was built. Two epic-end review rounds (20 → 17 fixed, then a targeted 4 → 4), every regression test mutation-checked.", + ], + }, { version: "0.85.0", date: "2026-09-13",