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")]), + ]; }