Files
siftlode/backend/app
peter 1102ad6a28 fix(downloads): make the probe verdict authoritative over yt-dlp info by probe-ran, not per-field
Deep-review round 1 on bug/download-mediakind-probe found a real HIGH the prior
review missed:

- resolve_media_facts used a per-field `probe or info` fallback, so a probe that
  RAN and deliberately found no video track (cover-art-only audio: attached_pic
  skipped; a still image: format-guarded) had its None video overridden by
  yt-dlp's stale vcodec. An audio-mode download from a progressive source (info
  carries the source's video codec, embed-thumbnail on) was then classified VIDEO
  -> a Play button + a broken in-app <video> with no video track. Only the
  download path regressed (upload calls probe_media directly).

  Fix: gate precedence on whether ffprobe RAN, not per-field. If the probe read
  the file, its codec verdict wins outright (a None means 'no such track'); only a
  full ffprobe failure falls back to info. Dimensions still gap-fill from info.
  Refactored the parsing into _facts_from_probe so resolve_media_facts can see the
  raw probe-ran signal; probe_media keeps its 5-tuple (upload + TestProbeClassifier
  unchanged).

- tests: the cover-art-audio and still-image resurrection cases, a differing
  populated-info precedence case (the prior-flagged gap), and the ffprobe-failed
  info fallback — now stubbing run_ffprobe_json so the real parsing + precedence
  run together.

Backend gate green (ruff + full pytest).
2026-08-30 18:09:26 +02:00
..
2026-06-14 04:40:22 +02:00

Backend conventions

The rules below are already followed across backend/app — they lived only in a few module docstrings (demo.py, userscope.py, routes/_common.py) until this file collected them (E3 S3, B-core-20). When you add code, match these so the next reader doesn't have to guess "which pattern do I copy".

Module layering

Three layers, by who runs the code — not by feature:

  1. routes/… — request-only. A route module holds HTTP concerns: auth dependencies, request/ response models, status-code mapping, serialization. It owns logic that runs only on a request. Business logic that a request happens to trigger does not live here if anything else runs it too.

  2. plex/, downloads/, sync/ — domain logic shared by more than one caller. Anything the scheduler or the worker runs (not just a request) lives in one of these domain packages, so the route, the worker, and the scheduler share one definition. Examples from this codebase: downloads/service.py owns enqueue + the job state transitions (routes call it); plex/sync.py is run by both the admin route and the scheduler; downloads/pipeline.py is the worker's child pipeline.

  3. Flat app/*.py core modules — cross-cutting primitives. auth, sysconfig, quota, audit, notifications, userscope, realtime, ratelimit, progress, … are single-file primitives many layers import. They depend on nothing above them (a core module must never import a routes/… module).

Extraction trigger: the SECOND consumer. Keep new logic inline in its first caller. The moment a second caller needs it (a worker as well as a route; a scheduler as well as a route), extract it down a layer — into the relevant domain package, or a flat core module if it's cross-cutting. Don't pre-abstract for a single caller; don't leave shared logic duplicated once there are two.

Transaction ownership

This follows CLAUDE.md's canonical rule verbatim — "the handler/job-wrapper commits; helpers flush (only quota owns its session)" — so read it that way, not as an exception to it. The commit belongs to the operation owner; anything called inside another transaction only stages its change (db.add / flush) and lets the owner commit, so the whole operation is one transaction. audit.record() is the reference: it adds the audit row and the caller commits it together with the mutation it records, so an audit entry can never exist without its change (or vice-versa).

  • The operation owner is usually the route handler. A service.* operation the handler delegates a whole transition to (e.g. service.cancel) is that owner and commits its one transition — it isn't a mid-operation helper. A helper that owns its OWN short-lived session (the worker's jobdb writes, the same carve-out CLAUDE.md grants quota) commits that session. Everything else flushes.
  • SessionLocal is expire_on_commit=False: after a raw UPDATE + commit, an ORM object you already loaded is stale. Read the fresh value from the row (RETURNING, or a re-SELECT), not the ORM attribute — see service.decr_ref_count.

Shared counters and status: do it atomically at the DB

  • Never read-modify-write a shared counter in Python (x.count = x.count + 1). Concurrent sessions lose an update. Use a DB-atomic statement — col = col + 1, or greatest(col - 1, 0) with RETURNING so the caller branches on the post-value under the row lock (service.incr_ref_count / decr_ref_count).
  • Download job status is a JobStatus StrEnum (downloads/status.py), not bare strings. The HOLDING set defines which states hold a MediaAsset (so release accounting stays correct); STOPPED_BY_ROUTE is the route-initiated stop set. A worker child's terminal write goes through service.finalize_if_running (an atomic UPDATE … WHERE status='running') so a finishing child can't resurrect a job a route just stopped.

Config: one home per kind

  • app.config.settings — boot/env config (DB URL, download root, worker concurrency): read once at startup, not user-tunable at runtime.
  • sysconfig.SPECS — the runtime admin knobs (DB-override-or-default). A new admin-tunable value goes in SPECS; don't invent a fourth store. (state.py's maintenance_batch predates this and is slated to fold into sysconfig — do not add more like it.)

The gate

Everything below must be green before a change is done (run via siftlode check): ruff (backend) · tsc ×3 + eslint + prettier + knip (frontend) · vitest · pytest. The pytest lane runs in backend/docker-compose.test.yml — an ephemeral Postgres + the backend tree bind-mounted — so the DB-backed integration tests (the db fixture) actually run; SIFTLODE_TEST_ REQUIRE_DB=1 there turns a would-be silent skip into a hard failure. See docs/development.md.