peter
614c5528ad
Deep-review remediation for the E7 upload path, all findings adversarially verified: - Serve every library file with a Content-Type chosen from the ffprobe-verified media_kind + a strict container allow-list (storage.served_media_type), never guessed from the user-controlled file extension. Closes a stored-XSS vector where a media/HTML polyglot uploaded as "evil.html" was served inline as text/html on the login-free /watch surface. Applied to both the authenticated /file and the public watch_file endpoints. - Classify uploaded still images (jpg/png/gif/webp/…) as media_kind='file' so they are force-downloaded and don't get a broken in-app video player. - Roll back the on-disk side of an upload when the ingest transaction fails after place_file (unlink the placed file + its deterministic poster path), so a failed commit can't orphan bytes in the GC-skipped .uploads tree. - Floor download_upload_max_bytes at min=1 so a stored 0 can't silently disable the per-file cap. - Dedupe the ffprobe-JSON invocation into edit.run_ffprobe_json (shared by probe_codecs and upload._probe) and the upload extension into storage._clean_upload_ext; drop the now-unused parse_probe_streams. Tests: public + authenticated force-download and the polyglot-not-served-as-HTML guard, the real _probe classifier (image/cover-art/dims/duration), the ingest rollback/cleanup path, the max_jobs branch, the cap-floor invariant, and run_ffprobe_json error handling.
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:
-
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. -
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.pyowns enqueue + the job state transitions (routes call it);plex/sync.pyis run by both the admin route and the scheduler;downloads/pipeline.pyis the worker's child pipeline. -
Flat
app/*.pycore 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 aroutes/…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'sjobdbwrites, the same carve-out CLAUDE.md grantsquota) commits that session. Everything else flushes. SessionLocalisexpire_on_commit=False: after a rawUPDATE+ 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 — seeservice.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, orgreatest(col - 1, 0)withRETURNINGso the caller branches on the post-value under the row lock (service.incr_ref_count/decr_ref_count). - Download job status is a
JobStatusStrEnum (downloads/status.py), not bare strings. TheHOLDINGset defines which states hold aMediaAsset(so release accounting stays correct);STOPPED_BY_ROUTEis the route-initiated stop set. A worker child's terminal write goes throughservice.finalize_if_running(an atomicUPDATE … 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 inSPECS; don't invent a fourth store. (state.py'smaintenance_batchpredates this and is slated to fold intosysconfig— 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.