fix(logs): rule on every swallowed exception in the backend
114 exception handlers neither logged nor re-raised. The defect was never that
they are silent — plenty of them should be — it is that their silence said
nothing, so nobody could tell a decision from an oversight. Each one has now been
read. 44 got a line; the other 70 carry `# silent-ok: <why>`.
What the silent ones had been hiding, in the cases that got a line:
- A failed ffprobe sent every HDR file down the 8-bit pipeline, so playback
degraded to a software transcode with nothing saying why; a failed audio probe
collapsed a multi-audio file to one track and the player showed no language
picker; a failed first-PTS probe left subtitles seconds out of sync.
- A subtitle sidecar that existed but would not parse made a PARTIAL track read as
complete, permanently — the viewer loses every cue past the cached window.
- An unreadable crop/trim/cut-list in an edit spec was dropped, so the clip came
out uncropped, untrimmed, or shorter than the one the user built.
- A dropped `max_height` downloaded at best quality against an explicit cap.
- A failed videos.list enrich made every candidate undroppable-from-judgement and
therefore dropped: a working search rendered as "no results".
- An unusable password hash answered as "wrong password", locking the owner out
and telling them they mistyped; a rotated TOKEN_ENCRYPTION_KEY turned every
stored token into "not connected".
- A stored admin setting that would not coerce fell back to the env default while
the settings page went on showing the saved value.
- A one-off InnerTube config failure pinned the whole process to the hard-coded
fallback key, and searches return nothing when that goes stale.
- The HLS sweep reported 0 swept — like a clean run — when it could not list its
own root, leaving every crashed session's segments on disk forever.
Four of them are counted rather than logged per item (dropped cues, dropped cut
list segments, unreclaimed GC files) and reported once per call, because the
alternative is one line per cue over a whole subtitle track.
ruff gains S110 and BLE001. Every blind `except Exception` that remains carries
`# noqa: BLE001 — <why>`, which is the point: catching everything is sometimes
right, and always worth a sentence. plex/sync.py's three best-effort backfill
catches now keep the traceback — a blind catch over a batch that fetches AND
writes could be Plex, the database or our own bug, and `%s` of the exception told
them apart badly. main.py's boot-time log-level read likewise.
logs.event's `code` is positional-only now: `event("x", code=...)` was a TypeError,
and `code` is exactly the word a caller reaches for when the thing that failed has
one of its own.
tests/test_swallowed_exceptions.py is the wider rule ruff cannot see: a handler
that neither logs nor re-raises must say why. Both cases mutation-checked — a
marker removed and a log line removed each fail it, naming the handler.
This commit is contained in:
@@ -71,7 +71,13 @@ def normalize_edit_spec(spec: dict | None) -> dict:
|
||||
if crop:
|
||||
try:
|
||||
c = {k: int(round(float(crop[k]))) for k in ("x", "y", "w", "h")}
|
||||
except (KeyError, TypeError, ValueError):
|
||||
except (KeyError, TypeError, ValueError) as exc:
|
||||
# The clip still renders — uncropped. The user asked for a crop, got a whole frame,
|
||||
# and nothing anywhere called it a failure; this line is the only trace there is.
|
||||
log.warning(
|
||||
"the crop in an edit spec could not be read and was dropped",
|
||||
extra=logs.event("edit.spec.crop_dropped", reason=type(exc).__name__),
|
||||
)
|
||||
c = None
|
||||
if c and c["w"] > 0 and c["h"] > 0 and c["x"] >= 0 and c["y"] >= 0:
|
||||
out["crop"] = c
|
||||
@@ -80,6 +86,7 @@ def normalize_edit_spec(spec: dict | None) -> dict:
|
||||
segs = spec.get("segments")
|
||||
if isinstance(segs, list) and segs:
|
||||
norm: list[dict] = []
|
||||
unreadable = 0
|
||||
for s in segs:
|
||||
try:
|
||||
st = max(0.0, float((s or {}).get("start_s") or 0.0))
|
||||
@@ -88,9 +95,18 @@ def normalize_edit_spec(spec: dict | None) -> dict:
|
||||
continue
|
||||
en = float(end_raw)
|
||||
except (TypeError, ValueError):
|
||||
# silent-ok: counted, not logged here — one cut-list can drop several, and the
|
||||
# join that comes out is simply shorter than the one the user built. Reported
|
||||
# once after the loop.
|
||||
unreadable += 1
|
||||
continue
|
||||
if en > st:
|
||||
norm.append({"start_s": round(st, 3), "end_s": round(en, 3)})
|
||||
if unreadable:
|
||||
log.warning(
|
||||
"some segments of a cut list could not be read and were dropped",
|
||||
extra=logs.event("edit.spec.segments_dropped", dropped=unreadable, kept=len(norm)),
|
||||
)
|
||||
norm.sort(key=lambda x: x["start_s"])
|
||||
# SEC-13: an accurate join builds one ffmpeg `filter_complex` with two filters per segment,
|
||||
# so an unbounded list becomes an unbounded command line and an ffmpeg that never finishes.
|
||||
@@ -114,7 +130,13 @@ def normalize_edit_spec(spec: dict | None) -> dict:
|
||||
start = max(0.0, float(trim.get("start_s") or 0.0))
|
||||
end_raw = trim.get("end_s")
|
||||
end = float(end_raw) if end_raw is not None else None
|
||||
except (TypeError, ValueError):
|
||||
except (TypeError, ValueError) as exc:
|
||||
# Either a 400 the user cannot act on, or — beside a crop — a full-length clip where
|
||||
# they asked for a cut. Both are worth one line saying the spec was the problem.
|
||||
log.warning(
|
||||
"the trim in an edit spec could not be read and was dropped",
|
||||
extra=logs.event("edit.spec.trim_dropped", reason=type(exc).__name__),
|
||||
)
|
||||
start, end = 0.0, None
|
||||
t: dict = {"start_s": round(start, 3)}
|
||||
if end is not None and end > start:
|
||||
@@ -306,11 +328,14 @@ def _parse_out_time(line: str) -> float | None:
|
||||
if line.startswith("out_time_us="):
|
||||
try:
|
||||
return int(line.split("=", 1)[1]) / 1_000_000
|
||||
# silent-ok: one line of ffmpeg's `-progress` stream, several per second. None means "this
|
||||
# line carried no usable time", which the caller already treats as "no progress yet".
|
||||
except ValueError:
|
||||
return None
|
||||
if line.startswith("out_time_ms="): # (some builds mislabel this as microseconds)
|
||||
try:
|
||||
return int(line.split("=", 1)[1]) / 1_000_000
|
||||
# silent-ok: as above, the other spelling of the same line.
|
||||
except ValueError:
|
||||
return None
|
||||
return None
|
||||
@@ -329,7 +354,9 @@ def _pump(stream, sink: "queue.Queue[str | None]") -> None:
|
||||
try:
|
||||
for line in stream:
|
||||
sink.put(line)
|
||||
except (ValueError, OSError): # pipe closed under us by the kill path
|
||||
# silent-ok: the pipe was closed under us by our OWN kill path — the expected end of a
|
||||
# cancelled or timed-out run, and the code that did the killing is the code that reports it.
|
||||
except (ValueError, OSError):
|
||||
pass
|
||||
finally:
|
||||
sink.put(None)
|
||||
@@ -347,6 +374,8 @@ def _drain_tail(stream, sink: list[str]) -> None:
|
||||
sink.append(line)
|
||||
if len(sink) > 200:
|
||||
del sink[:100]
|
||||
# silent-ok: same closed pipe as `_pump` above, and the tail collected so far is still handed
|
||||
# to whoever reports the failure.
|
||||
except (ValueError, OSError):
|
||||
pass
|
||||
|
||||
@@ -362,6 +391,8 @@ def escalate_kill(proc: subprocess.Popen, terminate, kill, grace_s: float, label
|
||||
try:
|
||||
proc.wait(timeout=grace_s)
|
||||
return
|
||||
# silent-ok: the escalation below IS the handling of this timeout, and surviving THAT is what
|
||||
# the line under the second wait reports.
|
||||
except subprocess.TimeoutExpired:
|
||||
pass
|
||||
kill()
|
||||
@@ -406,6 +437,8 @@ def run_ffmpeg(
|
||||
while True:
|
||||
try:
|
||||
line = lines.get(timeout=1.0)
|
||||
# silent-ok: not a failure — the one-second timeout is how this loop gets a turn to
|
||||
# poll for cancellation while ffmpeg is quiet.
|
||||
except queue.Empty:
|
||||
line = ""
|
||||
if line is None: # stdout hit EOF — ffmpeg is done (or dying)
|
||||
@@ -473,7 +506,13 @@ def run_ffprobe_json(
|
||||
try:
|
||||
out = subprocess.run(args, capture_output=True, text=True, timeout=30, check=True).stdout
|
||||
return json.loads(out)
|
||||
except (subprocess.TimeoutExpired, subprocess.CalledProcessError, OSError, ValueError):
|
||||
except (subprocess.TimeoutExpired, subprocess.CalledProcessError, OSError, ValueError) as exc:
|
||||
# None makes every field it would have filled read as absent, and a file with no streams
|
||||
# and no duration is indistinguishable here from one that is not media at all.
|
||||
log.warning(
|
||||
"ffprobe could not read a file",
|
||||
extra=logs.event("media.ffprobe_failed", reason=type(exc).__name__),
|
||||
)
|
||||
return None
|
||||
|
||||
|
||||
@@ -527,7 +566,13 @@ def probe_duration(path: Path) -> int:
|
||||
check=True,
|
||||
).stdout.strip()
|
||||
return int(float(out)) if out else 0
|
||||
except (subprocess.TimeoutExpired, subprocess.CalledProcessError, OSError, ValueError):
|
||||
except (subprocess.TimeoutExpired, subprocess.CalledProcessError, OSError, ValueError) as exc:
|
||||
# Zero is the denominator of the re-encode progress bar, so a failed probe leaves the user
|
||||
# watching a bar that never moves rather than a job that says it went wrong.
|
||||
log.info(
|
||||
"the duration probe failed; progress for this job has no total",
|
||||
extra=logs.event("media.duration_probe_failed", reason=type(exc).__name__),
|
||||
)
|
||||
return 0
|
||||
|
||||
|
||||
@@ -672,6 +717,8 @@ def _run_ffmpeg_best_effort(cmd: list[str], timeout_s: int) -> Exception | None:
|
||||
try:
|
||||
subprocess.run(cmd, capture_output=True, timeout=timeout_s, check=True)
|
||||
return None
|
||||
# silent-ok: the exception is the RETURN VALUE. This helper exists so the three callers share
|
||||
# one run/timeout policy, and each logs the failure with the context only it has.
|
||||
except (subprocess.TimeoutExpired, subprocess.CalledProcessError, OSError) as exc:
|
||||
return exc
|
||||
|
||||
@@ -681,6 +728,8 @@ def _is_jpeg(path: Path) -> bool:
|
||||
try:
|
||||
with path.open("rb") as f:
|
||||
return f.read(3) == b"\xff\xd8\xff"
|
||||
# silent-ok: False only means "do not copy this straight through" — the caller then goes the
|
||||
# ffmpeg route, which opens the same file and reports it properly if it really is unreadable.
|
||||
except OSError:
|
||||
return False
|
||||
|
||||
|
||||
@@ -20,6 +20,10 @@ spec shape (all keys optional; defaults + browser-playability clamp applied by `
|
||||
import hashlib
|
||||
import json
|
||||
|
||||
from app import logs
|
||||
|
||||
log = logs.get_logger(__name__)
|
||||
|
||||
# Fields that affect the produced file — and therefore the cache identity.
|
||||
_SIG_KEYS = (
|
||||
"mode",
|
||||
@@ -78,7 +82,13 @@ def normalize(spec: dict) -> dict:
|
||||
# rather than raising an unhandled 500 at enqueue.
|
||||
try:
|
||||
s["max_height"] = int(s["max_height"])
|
||||
except (TypeError, ValueError):
|
||||
except (TypeError, ValueError) as exc:
|
||||
# "best" is the opposite of what a height cap asks for: the user set a ceiling to save
|
||||
# bandwidth or disk, and the download quietly takes the largest rendition there is.
|
||||
log.warning(
|
||||
"the height cap on a download could not be read and was dropped",
|
||||
extra=logs.event("download.format.max_height_dropped", reason=type(exc).__name__),
|
||||
)
|
||||
s["max_height"] = None
|
||||
for b in ("embed_subs", "embed_chapters", "embed_thumbnail", "sponsorblock"):
|
||||
s[b] = bool(s[b])
|
||||
|
||||
@@ -97,6 +97,7 @@ def _sweep_orphan_files(db: Session, download_root: str) -> int:
|
||||
if pause_s and touched % batch == 0:
|
||||
time.sleep(pause_s)
|
||||
|
||||
unreclaimed = 0
|
||||
for entry in root.iterdir():
|
||||
if entry.name.startswith("."): # .edits / .plex-* / .staging / .storyboards + any dotfile
|
||||
continue
|
||||
@@ -109,7 +110,9 @@ def _sweep_orphan_files(db: Session, download_root: str) -> int:
|
||||
f.unlink()
|
||||
removed += 1
|
||||
except OSError:
|
||||
pass
|
||||
# silent-ok: counted, not logged per file — a broken mount would put one line
|
||||
# under every orphan. The total is reported once at the end of the sweep.
|
||||
unreclaimed += 1
|
||||
if entry.is_dir(): # prune emptied dirs (deepest first), then the channel dir itself
|
||||
for d in sorted(
|
||||
(p for p in entry.rglob("*") if p.is_dir()),
|
||||
@@ -118,12 +121,20 @@ def _sweep_orphan_files(db: Session, download_root: str) -> int:
|
||||
):
|
||||
try:
|
||||
d.rmdir()
|
||||
# silent-ok: `rmdir` refusing means the directory still holds files, which is
|
||||
# the ordinary answer here — this prune only takes the ones that emptied.
|
||||
except OSError:
|
||||
pass
|
||||
try:
|
||||
entry.rmdir()
|
||||
# silent-ok: the same prune one level up, at the channel directory.
|
||||
except OSError:
|
||||
pass
|
||||
if unreclaimed:
|
||||
log.warning(
|
||||
"some orphaned download files could not be reclaimed",
|
||||
extra=logs.event("download.gc.unreclaimed", files=unreclaimed, removed=removed),
|
||||
)
|
||||
return removed
|
||||
|
||||
|
||||
|
||||
@@ -49,6 +49,8 @@ def check_grant(token: str, grant: str | None, password_version: int) -> bool:
|
||||
exp_s, sig = grant.split(".", 1)
|
||||
try:
|
||||
exp = int(exp_s)
|
||||
# silent-ok: an untrusted grant that does not parse is simply not a valid grant, and the
|
||||
# honest answer to it — a password prompt — is the same as for one that expired.
|
||||
except ValueError:
|
||||
return False
|
||||
if exp < datetime.now(timezone.utc).timestamp():
|
||||
|
||||
@@ -66,6 +66,8 @@ def _jpeg_size(path: Path) -> tuple[int, int] | None:
|
||||
width = (data[3] << 8) + data[4]
|
||||
return width, height
|
||||
f.seek(length - 2, 1) # skip this segment's payload
|
||||
# silent-ok: None omits width/height from the unfurl card, which is what the card does for
|
||||
# every poster whose dimensions were never asked for. Nothing is claimed that is not true.
|
||||
except OSError:
|
||||
return None
|
||||
|
||||
|
||||
@@ -97,6 +97,8 @@ def _parse_upload_date(raw) -> date | None:
|
||||
return None
|
||||
try:
|
||||
return datetime.strptime(str(raw), "%Y%m%d").date()
|
||||
# silent-ok: a date we cannot read becomes a date the card does not show, which is what the
|
||||
# card shows for every source that never supplies one. Absent is the honest rendering.
|
||||
except ValueError:
|
||||
return None
|
||||
|
||||
@@ -137,7 +139,14 @@ def _make_progress_hook(job_id: int, asset_id: int):
|
||||
try:
|
||||
_fill_asset_meta_early(asset_id, d["info_dict"])
|
||||
except Exception: # noqa: BLE001 — metadata is best-effort, never fail a download
|
||||
pass
|
||||
# Best-effort is about the DOWNLOAD, not about knowing. What this swallows is
|
||||
# anything from a shape change in yt-dlp's info dict to the database being down,
|
||||
# and all the user sees is a card with an id on it until the job finishes.
|
||||
log.info(
|
||||
"early metadata could not be filled in for this download",
|
||||
extra=logs.event("download.meta.early_fill_failed", asset_id=asset_id),
|
||||
exc_info=True,
|
||||
)
|
||||
if d.get("status") != "downloading":
|
||||
return
|
||||
if not allow_write():
|
||||
@@ -379,8 +388,17 @@ def _extract_with_fallback(
|
||||
for p in list(staging.iterdir()): # drop partials before the retry
|
||||
try:
|
||||
p.unlink()
|
||||
except OSError:
|
||||
pass
|
||||
except OSError as exc:
|
||||
# A partial left behind is not inert: the next client set resumes into it,
|
||||
# so the retry this loop exists to make clean starts dirty instead.
|
||||
log.info(
|
||||
"a partial file could not be removed before the retry",
|
||||
extra=logs.event(
|
||||
"download.retry.partial_unlink_failed",
|
||||
job_id=job_id,
|
||||
reason=type(exc).__name__,
|
||||
),
|
||||
)
|
||||
_set_job(job_id, progress=0, phase="downloading")
|
||||
continue
|
||||
raise
|
||||
@@ -637,7 +655,16 @@ def _download(
|
||||
phase=None,
|
||||
)
|
||||
db.commit()
|
||||
log.warning("job %s failed: %s", job_id, msg)
|
||||
# The traceback only when the classifier could not place it: a recognised yt-dlp
|
||||
# failure is fully described by its code, and the stack under it is always this same
|
||||
# `except`. An UNRECOGNISED one leaves nothing else to work from.
|
||||
log.warning(
|
||||
"job %s failed: %s",
|
||||
job_id,
|
||||
msg,
|
||||
extra=logs.event("download.job.failed", job_id=job_id, error_code=diagnosis.code),
|
||||
exc_info=diagnosis.code in (None, "", "unknown"),
|
||||
)
|
||||
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).
|
||||
@@ -757,6 +784,12 @@ def _process_edit(job_id: int, asset_id: int) -> None:
|
||||
_reset_asset_pending(asset_id)
|
||||
log.info("edit job %s aborted (pause/cancel)", job_id)
|
||||
except Exception as exc: # noqa: BLE001 — surface the failure, keep the worker alive
|
||||
# `_fail_edit` stores `str(exc)` for the user, which is the wrong half for whoever has to
|
||||
# fix it: an edit reaching here is by definition unclassified, so the stack is all there is.
|
||||
log.exception(
|
||||
"the edit job failed",
|
||||
extra=logs.event("edit.job.failed", job_id=job_id, asset_id=asset_id),
|
||||
)
|
||||
_fail_edit(job_id, asset_id, str(exc)[:500])
|
||||
finally:
|
||||
shutil.rmtree(staging, ignore_errors=True)
|
||||
|
||||
@@ -64,6 +64,8 @@ def _facts_from_probe(
|
||||
raw = fmt.get("duration")
|
||||
if raw is not None:
|
||||
duration = int(float(raw))
|
||||
# silent-ok: an unusable duration is stored as no duration, which the UI renders as unknown
|
||||
# rather than as zero — the honest state, and the one a re-probe can still fill in.
|
||||
except (TypeError, ValueError):
|
||||
duration = None
|
||||
return vcodec, acodec, width, height, duration
|
||||
|
||||
@@ -87,6 +87,8 @@ def get_or_create_asset(
|
||||
with db.begin_nested():
|
||||
db.add(asset)
|
||||
db.flush()
|
||||
# silent-ok: the expected outcome of the race this savepoint exists for — two jobs wanting the
|
||||
# same asset. Losing it means the other insert won, and re-selecting is the whole handling.
|
||||
except IntegrityError:
|
||||
asset = db.execute(stmt).scalar_one()
|
||||
return asset
|
||||
|
||||
@@ -19,8 +19,11 @@ from datetime import date
|
||||
from pathlib import Path
|
||||
from xml.sax.saxutils import escape
|
||||
|
||||
from app import logs
|
||||
from app.downloads import mediakind
|
||||
|
||||
log = logs.get_logger(__name__)
|
||||
|
||||
# Reserve room for the "_id.ext" suffix + parent dirs within the 255-char component limit.
|
||||
_MAX_TITLE = 120
|
||||
# User-facing (Content-Disposition) names can be longer than an on-disk component.
|
||||
@@ -294,7 +297,13 @@ def write_sidecars(download_root: str, rel: str, meta: MediaMeta, thumb_src: Pat
|
||||
if thumb_src and thumb_src.exists():
|
||||
shutil.copyfile(str(thumb_src), str(_sidecar(media, ".jpg")))
|
||||
return True
|
||||
except OSError:
|
||||
except OSError as exc:
|
||||
# The download itself still completes, so the job reads as done while the library it was
|
||||
# downloaded FOR has no .nfo to read and no poster to show.
|
||||
log.warning(
|
||||
"the sidecars beside a download could not be written",
|
||||
extra=logs.event("download.sidecars.write_failed", reason=type(exc).__name__),
|
||||
)
|
||||
return False
|
||||
|
||||
|
||||
@@ -304,14 +313,23 @@ def delete_asset_files(download_root: str, rel: str) -> None:
|
||||
for p in (media, _sidecar(media, ".nfo"), _sidecar(media, ".jpg")):
|
||||
try:
|
||||
p.unlink()
|
||||
except OSError:
|
||||
pass
|
||||
except FileNotFoundError:
|
||||
pass # silent-ok: already gone is the outcome this function wanted
|
||||
except OSError as exc:
|
||||
# The row goes and the bytes stay. Nothing else ever revisits this path, so the disk
|
||||
# quietly keeps a file no account has any way to see, let alone delete.
|
||||
log.info(
|
||||
"a deleted download left a file behind",
|
||||
extra=logs.event("download.asset.file_unlink_failed", reason=type(exc).__name__),
|
||||
)
|
||||
# Prune empty Season/Channel dirs up to (but not including) the root.
|
||||
root = Path(download_root).resolve()
|
||||
parent = media.parent
|
||||
while parent != root and root in parent.parents:
|
||||
try:
|
||||
parent.rmdir() # only succeeds if empty
|
||||
# silent-ok: this `rmdir` failing is the loop's stop condition — the parent still
|
||||
# holds another download's files, so the prune has gone as far up as it may.
|
||||
except OSError:
|
||||
break
|
||||
parent = parent.parent
|
||||
|
||||
Reference in New Issue
Block a user