fix(e1): address /code-review high findings on the E1 stack
Review round on the whole E1 stack (main..HEAD). 6 findings, all low/medium, none blocking; fixed for a clean gate-green handoff. - plex/sync (correctness): the un-enriched cast guard compared the stored cast against THIS listing's length, so a trimmed Plex cast on a not-yet-enriched row was never applied. Compare against the listing CAP (_LISTING_CAST_CAP=20) instead: >cap = enriched (keep, survives a watermark reset), <=cap = listing-sourced (track edits in both directions). +2 regression tests. - main (security): add X-Frame-Options: DENY as the legacy-browser fallback for the CSP frame-ancestors clickjacking guard. - downloads/service (altitude): finalize_if_running now uses a SQLAlchemy 2.0 update() construct instead of a hand-built text() UPDATE string — same atomic WHERE status='running', idiomatic, no identifier interpolation, no manual StrEnum coercion. Drops the now-unused text import. - auth/admin (reuse): extract the SEC-10 "drop untrusted preset password + bump epoch" invariant into one helper (revoke_preset_password_if_unverified) so the Google-callback and admin-activation paths can't drift. - tests/migrations (robustness): guarantee the shared schema is restored to head in a finally, so a downgrade/re-upgrade failure can't cascade into later tests. - worker (simplification): align _finish_from_cache's display_name fill with the _download path's guard (no behavior change). Gate green: ruff clean, 334 pytest (0 skip), frontend unchanged.
This commit is contained in:
@@ -8,7 +8,7 @@ the worker live in `worker.py` / the routes; this module owns enqueue + asset bo
|
||||
"""
|
||||
from datetime import datetime, timezone
|
||||
|
||||
from sqlalchemy import func, select, text
|
||||
from sqlalchemy import func, select, update
|
||||
from sqlalchemy.exc import IntegrityError
|
||||
from sqlalchemy.orm import Session
|
||||
|
||||
@@ -234,19 +234,21 @@ def finalize_if_running(db: Session, job_id: int, **fields) -> bool:
|
||||
|
||||
Runs in the CALLER's session and does NOT commit — the caller commits it together with its asset
|
||||
writes in one transaction. It flushes first so any pending asset write takes its row lock BEFORE
|
||||
this takes the job's, matching the cancel route's asset-then-job order (no deadlock). `fields` keys
|
||||
are internal column names (never user input). Returns True when the row was still running and got
|
||||
this takes the job's, matching the cancel route's asset-then-job order (no deadlock). `fields` are
|
||||
mapped-column kwargs (never user input). Returns True when the row was still running and got
|
||||
updated, False when a route had already moved it out of 'running'."""
|
||||
if not fields:
|
||||
return False
|
||||
db.flush() # asset-then-job lock order: push pending asset writes before we lock the job row
|
||||
# StrEnum members carry through psycopg as strings, but coerce explicitly so a bound status value
|
||||
# is always the plain literal the column stores.
|
||||
params = {k: (v.value if isinstance(v, JobStatus) else v) for k, v in fields.items()}
|
||||
assignments = ", ".join(f"{col} = :{col}" for col in fields)
|
||||
# A Core UPDATE with the guard as a WHERE — same atomic "only while still running" write as the
|
||||
# rest of the codebase's 2.0 statements, no hand-built SQL string. synchronize_session=False: this
|
||||
# is a terminal write, so we don't pay to sync the (about-to-be-discarded) ORM identity map. Bound
|
||||
# StrEnum values bind as their plain str literal (StrEnum is a str subclass).
|
||||
result = db.execute(
|
||||
text(f"UPDATE download_jobs SET {assignments} WHERE id = :_job_id AND status = :_running"),
|
||||
{**params, "_job_id": job_id, "_running": JobStatus.RUNNING.value},
|
||||
update(DownloadJob)
|
||||
.where(DownloadJob.id == job_id, DownloadJob.status == JobStatus.RUNNING.value)
|
||||
.values(**fields)
|
||||
.execution_options(synchronize_session=False)
|
||||
)
|
||||
return result.rowcount > 0
|
||||
|
||||
|
||||
Reference in New Issue
Block a user