fix(logs): deep-review round 3 — a failure nobody could record is one scrubbed line
- The download and edit failure lines go back after their DB write: a job the write could not fail is not a failed job, and the worker's child_exit line already says it failed. - The download child's runner is the last stop before Python's hook: an escaping exception is one download.child.failed line with its chain scrubbed, then exit 1, instead of a raw stderr traceback carrying the URL. - A crash the database will not record carries its stack once, and the fail-write line has an event code. - The job's own generic-extractor name is taken out as a literal (formats.hiding), so brackets and colons in a direct link's file name no longer leak; the pattern stays as the fallback. `Downloading just the video <id>` is covered. - Tests: scrubbed_stack's cause, suppress, unprintable and text-format paths, the image steps' failure kinds, the watchdog error through the real _fail_running, exact poster/storyboard messages, ffmpeg skip marker.
This commit is contained in:
@@ -30,6 +30,11 @@ log = logs.get_logger(__name__)
|
||||
def _extract_meta(url: str, adapter: adapters.SourceAdapter, asset_id: int) -> dict | None:
|
||||
"""Metadata-only yt-dlp extraction, using the source adapter's client sets + options (YouTube
|
||||
needs the POT/player-client strategy even just to extract)."""
|
||||
with formats.hiding(url):
|
||||
return _extract_meta_scoped(url, adapter, asset_id)
|
||||
|
||||
|
||||
def _extract_meta_scoped(url: str, adapter: adapters.SourceAdapter, asset_id: int) -> dict | None:
|
||||
for clients in adapter.client_sets():
|
||||
opts = formats.build_ydl_opts(
|
||||
{}, "%(id)s.%(ext)s", lambda d: None, extra=adapter.ydl_opts_extra(clients)
|
||||
|
||||
@@ -17,10 +17,15 @@ spec shape (all keys optional; defaults + browser-playability clamp applied by `
|
||||
`container`/`vcodec` keys survive only so an old stored profile deserializes without error.
|
||||
"""
|
||||
|
||||
import contextlib
|
||||
import contextvars
|
||||
import hashlib
|
||||
import json
|
||||
import logging
|
||||
import os
|
||||
import re
|
||||
import urllib.parse
|
||||
from collections.abc import Iterator
|
||||
|
||||
from app import logs
|
||||
|
||||
@@ -38,28 +43,60 @@ _URL_RE = re.compile(r"https?://([^/\s]+)\S*")
|
||||
# after it is what `_PATH_RE` takes out.
|
||||
_ID_PREFIX_RE = re.compile(r"(\[[\w:.-]+\] )(?!Destination:)[^\s:\[\]]+:")
|
||||
# The generic extractor's "id" is the URL's basename, percent-DECODED: `My%20Holiday.mp4` is
|
||||
# `My Holiday` — spaces and all, which the pattern above stops at. Only this extractor: every
|
||||
# other one names a site id, and letting the prefix run to `: ` everywhere would take the words
|
||||
# out of lines like `[VideoConvertor] Converting video from webm to mp4; Destination: …`.
|
||||
# `My Holiday` — spaces, brackets, colons and all, which no pattern can bound (`Our Trip: Day 1`
|
||||
# ends at which `: `?). The job knows its own URL, so the exact name is taken out as a literal
|
||||
# (`hiding`). This pattern is the fallback for a line logged outside that scope: up to the first
|
||||
# `: `. Only this extractor: every other one names a site id, and letting the prefix run to `: `
|
||||
# everywhere would take the words out of lines like `[VideoConvertor] Converting …; Destination:`.
|
||||
_GENERIC_ID_RE = re.compile(r"(\[generic\] )(?!Destination:)[^\[\]\n]+?:(?= |$)", re.M)
|
||||
# `[youtube:tab] Playlist <title>: Downloading 5 items` — the title is not an id, and has spaces.
|
||||
_PLAYLIST_TITLE_RE = re.compile(r"(\] Playlist ).+?(?=: Downloading )")
|
||||
# `[download] Finished downloading playlist: <title>`, `[download] Downloading playlist: <title>`,
|
||||
# `… Skipping already downloaded playlist: <title>` — the title runs to the end of the line.
|
||||
_TITLE_TAIL_RE = re.compile(r"(playlist: |\[download\] Downloading \w+: ).*", re.I)
|
||||
# An id quoted mid-sentence, not as a prefix.
|
||||
_BARE_ID_RE = re.compile(r"((?:subtitles not available for|Downloading just video) )[^\s,;]+")
|
||||
# An id quoted mid-sentence, not as a prefix: `… not available for <id>`, and `--no-playlist`'s
|
||||
# `Downloading just the video <id>` (`_yes_playlist`) / `Downloading just video <id>` (multifeed).
|
||||
_BARE_ID_RE = re.compile(
|
||||
r"((?:subtitles not available for|Downloading just (?:the )?\w+) )[^\s,;]+"
|
||||
)
|
||||
# Two segments or more, and not glued to a word: `www.youtube.com/…` is what the URL pattern
|
||||
# leaves behind, and it must survive this one.
|
||||
_PATH_RE = re.compile(r"(?<![\w.…])/(?:[^/\s\"']+/)+[^/\s\"']*")
|
||||
|
||||
|
||||
_HIDDEN: contextvars.ContextVar[tuple[str, ...]] = contextvars.ContextVar(
|
||||
"formats_hidden", default=()
|
||||
)
|
||||
|
||||
|
||||
def generic_id(url: str) -> str:
|
||||
"""The name yt-dlp's generic extractor gives a URL: its basename, extension off, percent-
|
||||
decoded — `InfoExtractor._generic_id` (2026.07.04), pinned against it in the tests."""
|
||||
return urllib.parse.unquote(os.path.splitext(url.rstrip("/").split("/")[-1])[0])
|
||||
|
||||
|
||||
@contextlib.contextmanager
|
||||
def hiding(url: str | None) -> Iterator[None]:
|
||||
"""For the work on one URL: `log_safe` also takes out the name yt-dlp's generic extractor
|
||||
gives it, exactly, where yt-dlp puts it (`[generic] <name>:`). A context variable, so it
|
||||
holds for yt-dlp's logger calls too — they run on the same thread as the extraction."""
|
||||
name = generic_id(url) if url else ""
|
||||
token = _HIDDEN.set((*_HIDDEN.get(), name) if name else _HIDDEN.get())
|
||||
try:
|
||||
yield
|
||||
finally:
|
||||
_HIDDEN.reset(token)
|
||||
|
||||
|
||||
def log_safe(text: object) -> str:
|
||||
"""yt-dlp's text (its own lines, or a DownloadError's message) with the parts that name a
|
||||
person's download taken out. Every log line that carries yt-dlp's words goes through here,
|
||||
not just the adapter's: yt-dlp raises the same text it has just logged, so a line that skipped
|
||||
it would put back one line later exactly what the adapter took out."""
|
||||
text = _URL_RE.sub(lambda m: f"{m.group(1)}/…", str(text))
|
||||
text = str(text)
|
||||
for name in _HIDDEN.get():
|
||||
text = text.replace(f"[generic] {name}:", "[generic] …:")
|
||||
text = _URL_RE.sub(lambda m: f"{m.group(1)}/…", text)
|
||||
text = _ID_PREFIX_RE.sub(r"\1…:", text)
|
||||
text = _GENERIC_ID_RE.sub(r"\1…:", text)
|
||||
text = _PLAYLIST_TITLE_RE.sub(r"\1…", text)
|
||||
|
||||
@@ -654,30 +654,28 @@ def _download(
|
||||
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, diagnosis.code)
|
||||
# The line goes out whether or not the write below succeeds: when the database is what
|
||||
# failed, that write fails too, and the failure would otherwise leave no line at all.
|
||||
try:
|
||||
with SessionLocal() as db:
|
||||
asset = db.get(MediaAsset, asset_id)
|
||||
if asset:
|
||||
asset.status = "error"
|
||||
asset.error = msg
|
||||
# Atomic fail: only while still running, so a route-set cancel/pause is never
|
||||
# overwritten with `error` (error also holds the asset → would re-open the
|
||||
# double-release class).
|
||||
service.finalize_if_running(
|
||||
db,
|
||||
job_id,
|
||||
status=JobStatus.ERROR,
|
||||
error=msg,
|
||||
error_code=diagnosis.code,
|
||||
error_detail=diagnosis.detail,
|
||||
phase=None,
|
||||
)
|
||||
db.commit()
|
||||
finally:
|
||||
# silent-ok: not silent — `_log_download_failed` is the report.
|
||||
_log_download_failed(job_id, msg, diagnosis.code, exc)
|
||||
with SessionLocal() as db:
|
||||
asset = db.get(MediaAsset, asset_id)
|
||||
if asset:
|
||||
asset.status = "error"
|
||||
asset.error = msg
|
||||
# Atomic fail: only while still running, so a route-set cancel/pause is never
|
||||
# overwritten with `error` (error also holds the asset → would re-open the
|
||||
# double-release class).
|
||||
service.finalize_if_running(
|
||||
db,
|
||||
job_id,
|
||||
status=JobStatus.ERROR,
|
||||
error=msg,
|
||||
error_code=diagnosis.code,
|
||||
error_detail=diagnosis.detail,
|
||||
phase=None,
|
||||
)
|
||||
db.commit()
|
||||
# After the write: a job the write could not fail is not a failed job. When it raises,
|
||||
# the runner reports this failure with it (`runner._run`) and the worker fails the job.
|
||||
# silent-ok: not silent — `_log_download_failed` is the report.
|
||||
_log_download_failed(job_id, msg, diagnosis.code, exc)
|
||||
finally:
|
||||
# Unlink the decrypted cookie file explicitly (the rmtree below removes the whole staging
|
||||
# tree too, but never leave a plaintext session cookie on disk a moment longer than needed).
|
||||
@@ -729,40 +727,38 @@ def _fail_edit(job_id: int, asset_id: int, msg: str, exc: BaseException | None =
|
||||
where its line and its event code live. `exc` is the exception when there was one.
|
||||
|
||||
Its traceback goes with the line unless the message already tells it all (`_failure_stack`).
|
||||
The line goes out whether or not the write succeeds (see the download error path)."""
|
||||
try:
|
||||
with SessionLocal() as db:
|
||||
asset = db.get(MediaAsset, asset_id)
|
||||
if asset:
|
||||
asset.status = "error"
|
||||
asset.error = msg
|
||||
# Atomic fail: only while still running (see the download error path).
|
||||
service.finalize_if_running(
|
||||
db,
|
||||
job_id,
|
||||
status=JobStatus.ERROR,
|
||||
error=msg,
|
||||
error_code=dlerrors.classify(msg),
|
||||
phase=None,
|
||||
)
|
||||
db.commit()
|
||||
finally:
|
||||
# `msg` is the user's copy; ffmpeg's stderr tail in it can name the source file, whose
|
||||
# path is built from the channel and the title.
|
||||
log.warning(
|
||||
"edit job %s failed: %s",
|
||||
Logged after the write, as on the download error path."""
|
||||
with SessionLocal() as db:
|
||||
asset = db.get(MediaAsset, asset_id)
|
||||
if asset:
|
||||
asset.status = "error"
|
||||
asset.error = msg
|
||||
# Atomic fail: only while still running (see the download error path).
|
||||
service.finalize_if_running(
|
||||
db,
|
||||
job_id,
|
||||
formats.log_safe(msg),
|
||||
extra={
|
||||
**logs.event(
|
||||
"edit.job.failed",
|
||||
job_id=job_id,
|
||||
asset_id=asset_id,
|
||||
**({"error_type": type(exc).__name__} if exc is not None else {}),
|
||||
),
|
||||
**(_failure_stack(exc) if exc is not None else {}),
|
||||
},
|
||||
status=JobStatus.ERROR,
|
||||
error=msg,
|
||||
error_code=dlerrors.classify(msg),
|
||||
phase=None,
|
||||
)
|
||||
db.commit()
|
||||
# `msg` is the user's copy; ffmpeg's stderr tail in it can name the source file, whose path is
|
||||
# built from the channel and the title.
|
||||
log.warning(
|
||||
"edit job %s failed: %s",
|
||||
job_id,
|
||||
formats.log_safe(msg),
|
||||
extra={
|
||||
**logs.event(
|
||||
"edit.job.failed",
|
||||
job_id=job_id,
|
||||
asset_id=asset_id,
|
||||
**({"error_type": type(exc).__name__} if exc is not None else {}),
|
||||
),
|
||||
**(_failure_stack(exc) if exc is not None else {}),
|
||||
},
|
||||
)
|
||||
|
||||
|
||||
def _process_edit(job_id: int, asset_id: int) -> None:
|
||||
|
||||
@@ -21,6 +21,7 @@ from app import logsink
|
||||
from app.config import settings
|
||||
from app.db import SessionLocal
|
||||
from app.downloads import edit as editmod
|
||||
from app.downloads import formats
|
||||
from app.models import DownloadJob
|
||||
|
||||
log = logs.get_logger(__name__)
|
||||
@@ -81,14 +82,40 @@ def main() -> None:
|
||||
if asset_id is None:
|
||||
return
|
||||
|
||||
_run(job_id, asset_id, job_kind, source_kind, source_ref, spec, user_id)
|
||||
|
||||
|
||||
def _run(job_id, asset_id, job_kind, source_kind, source_ref, spec, user_id) -> None:
|
||||
"""Run the job, and be the last stop before Python's own exception hook.
|
||||
|
||||
What gets past the pipeline is a failure it could not even record (the database refused the
|
||||
write that fails the job). Python's hook would print it to stderr raw — and its context is the
|
||||
download's own failure, whose text is the URL or ffmpeg's stderr naming the file. Here it is
|
||||
one line with the whole chain scrubbed; the non-zero exit then has the worker fail the job
|
||||
(`cause=child_exit`)."""
|
||||
# Imported here (not at module top) so the child pays the yt-dlp import cost only when it has
|
||||
# real work, and to keep the import graph obvious: runner is a thin entry over the pipeline.
|
||||
from app.downloads.pipeline import _download, _process_edit
|
||||
|
||||
if job_kind == "edit":
|
||||
_process_edit(job_id, asset_id)
|
||||
else:
|
||||
_download(job_id, asset_id, source_kind, source_ref, spec, user_id)
|
||||
with formats.hiding(source_ref):
|
||||
try:
|
||||
if job_kind == "edit":
|
||||
_process_edit(job_id, asset_id)
|
||||
else:
|
||||
_download(job_id, asset_id, source_kind, source_ref, spec, user_id)
|
||||
except Exception as exc: # noqa: BLE001 — whatever it is, it is reported here or raw
|
||||
# noqa TRY400: the stack goes with it scrubbed (`scrubbed_stack`); `exc_info` is raw.
|
||||
log.error( # noqa: TRY400
|
||||
"the download child for job %s failed",
|
||||
job_id,
|
||||
extra={
|
||||
**logs.event(
|
||||
"download.child.failed", job_id=job_id, error_type=type(exc).__name__
|
||||
),
|
||||
**logs.scrubbed_stack(exc, formats.log_safe),
|
||||
},
|
||||
)
|
||||
raise SystemExit(1) from None
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
|
||||
Reference in New Issue
Block a user