fix(downloads): repair 5 confirmed backend bugs (B1-B5)

- B1 (sticky errored asset): get_or_create_asset now resets a reused status=='error'
  asset back to 'pending' (+clears the error), so a fresh enqueue/edit of a once-failed
  (source,format) pair actually re-downloads instead of the worker short-circuiting the
  new job with the stale error. Errored assets carry no expires_at, so without this the
  pair was permanently poisoned for all users until someone hit resume.
- B2 (ref_count leak): _release_asset counts 'error' as a holding state, so deleting an
  errored job decrements the ref_count that enqueue always incremented (the worker never
  decrements on failure). Errored rows are deliberately NOT deleted here — a concurrent
  B1 reuse could otherwise be lost-updated + FK-nulled; the row is fileless and harmless.
- B3: admin storage dashboard reads sysconfig.get_int(db,'download_total_max_bytes')
  (the admin-editable DB value GC enforces) instead of the raw env default.
- B4: the single-trim branch of normalize_edit_spec guards its float() coercion like the
  crop/segments branches — a malformed trim now yields a 400, not an unhandled 500.
- B5: formats.normalize guards int(max_height) → falls back to "best" instead of 500.

Reviewed (race in an earlier B2 draft caught + fixed); localdev boots, B4/B5 unit-verified.
This commit is contained in:
npeter83 2026-07-11 16:15:12 +02:00
parent bf8d4b94a0
commit 7a6f351e64
4 changed files with 38 additions and 11 deletions

View file

@ -15,6 +15,7 @@ from fastapi.responses import FileResponse
from sqlalchemy import func, select
from sqlalchemy.orm import Session
from app import sysconfig
from app.auth import admin_user, require_human
from app.config import settings
from app.db import get_db
@ -499,9 +500,18 @@ def _release_asset(db: Session, job: DownloadJob) -> None:
"""Drop this job's hold on its asset when it leaves a holding state. Once NO job holds the
asset anymore, delete the file + row immediately a deleted download should free its disk,
and the shared cache only needs to span *overlapping* holders (a later re-add just downloads
again). Idempotent: a job that's already left holding (canceled/error) is a no-op, so delete
after cancel doesn't double-release."""
if not job.asset_id or job.status not in ("queued", "running", "paused", "done"):
again). `error` counts as holding: enqueue always +1'd ref_count and the worker never
decrements on failure (ref_count is the route's job), so an errored job releases here on
delete else its +1 leaks and a later resumeready keeps the file pinned past its last holder.
Idempotent for the cancel path: cancel already released while the job was holding, then set it
`canceled` ( the set below), so delete-after-cancel is a no-op and never double-releases.
We DON'T delete an errored asset row here even at ref==0: another request may be concurrently
re-enqueuing the same (source,format) get_or_create_asset would reset that row to `pending` and
+1 it, and deleting it under that would FK-null the new job's asset (a lost update on ref_count).
A ready asset is still freed at ref==0 (its file is the point); an orphaned errored row is cheap
(no file) and gets reused+reset by the next enqueue, or reclaimed by a later GC pass."""
if not job.asset_id or job.status not in ("queued", "running", "paused", "done", "error"):
return
asset = db.get(MediaAsset, job.asset_id)
if asset is None:
@ -876,7 +886,7 @@ def admin_storage(
"ready_files": ready[0],
"total_bytes": int(ready[1]),
"total_assets": total_assets,
"total_cap_bytes": settings.download_total_max_bytes,
"total_cap_bytes": sysconfig.get_int(db, "download_total_max_bytes"),
"per_user": [
{"user_id": uid, "email": emails.get(uid), "footprint_bytes": quota.footprint(db, uid)}
for uid, _ in per_user