Merge bugfix/downloads: repair 5 Downloads backend bugs (B1-B5)
Sticky errored asset re-download, ref_count leak on errored-job delete, admin storage cap reads the DB value, and 400-not-500 on malformed edit/format specs. Reviewed clean (a B2 concurrency race was caught + fixed). Local dev only — prod publish waits for the end of the whole code-hygiene review.
This commit is contained in:
commit
0da65985d0
4 changed files with 38 additions and 11 deletions
|
|
@ -79,13 +79,17 @@ def normalize_edit_spec(spec: dict | None) -> dict:
|
||||||
# Single TRIM (one output file; the frontend fans a "separate" split out into N of these).
|
# Single TRIM (one output file; the frontend fans a "separate" split out into N of these).
|
||||||
trim = spec.get("trim") or {}
|
trim = spec.get("trim") or {}
|
||||||
if trim:
|
if trim:
|
||||||
start = max(0.0, float(trim.get("start_s") or 0.0))
|
# Guard the numeric coercion like the crop/segments branches do — a malformed value must
|
||||||
end_raw = trim.get("end_s")
|
# drop the trim (→ empty spec → 400 EditError), not raise an unhandled 500.
|
||||||
|
try:
|
||||||
|
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):
|
||||||
|
start, end = 0.0, None
|
||||||
t: dict = {"start_s": round(start, 3)}
|
t: dict = {"start_s": round(start, 3)}
|
||||||
if end_raw is not None:
|
if end is not None and end > start:
|
||||||
end = float(end_raw)
|
t["end_s"] = round(end, 3)
|
||||||
if end > start:
|
|
||||||
t["end_s"] = round(end, 3)
|
|
||||||
# A trim with only a start (open-ended) is valid (cut to the end).
|
# A trim with only a start (open-ended) is valid (cut to the end).
|
||||||
if t.get("start_s") or "end_s" in t:
|
if t.get("start_s") or "end_s" in t:
|
||||||
out["trim"] = t
|
out["trim"] = t
|
||||||
|
|
|
||||||
|
|
@ -62,7 +62,12 @@ def normalize(spec: dict) -> dict:
|
||||||
if s["mode"] not in ("av", "v", "a"):
|
if s["mode"] not in ("av", "v", "a"):
|
||||||
s["mode"] = "av"
|
s["mode"] = "av"
|
||||||
if s["max_height"] is not None:
|
if s["max_height"] is not None:
|
||||||
s["max_height"] = int(s["max_height"])
|
# A malformed max_height (non-numeric inline spec / stored profile) falls back to "best"
|
||||||
|
# rather than raising an unhandled 500 at enqueue.
|
||||||
|
try:
|
||||||
|
s["max_height"] = int(s["max_height"])
|
||||||
|
except (TypeError, ValueError):
|
||||||
|
s["max_height"] = None
|
||||||
for b in ("embed_subs", "embed_chapters", "embed_thumbnail", "sponsorblock"):
|
for b in ("embed_subs", "embed_chapters", "embed_thumbnail", "sponsorblock"):
|
||||||
s[b] = bool(s[b])
|
s[b] = bool(s[b])
|
||||||
# Audio-only files can't embed subtitles/thumbnails as video; keep the flags meaningful.
|
# Audio-only files can't embed subtitles/thumbnails as video; keep the flags meaningful.
|
||||||
|
|
|
||||||
|
|
@ -46,6 +46,14 @@ def get_or_create_asset(
|
||||||
)
|
)
|
||||||
asset = db.execute(stmt).scalar_one_or_none()
|
asset = db.execute(stmt).scalar_one_or_none()
|
||||||
if asset is not None:
|
if asset is not None:
|
||||||
|
# A previously-FAILED shared asset must be re-attempted for the new job about to hold it —
|
||||||
|
# reset it to pending (+clear the error) so the worker actually re-downloads instead of
|
||||||
|
# short-circuiting the new job with the asset's stale error. (Mirrors resume_download; without
|
||||||
|
# this a fresh enqueue of a once-failed (source,format) pair is permanently poisoned, since
|
||||||
|
# errored assets carry no expires_at and GC never clears them.)
|
||||||
|
if asset.status == "error":
|
||||||
|
asset.status = "pending"
|
||||||
|
asset.error = None
|
||||||
return asset
|
return asset
|
||||||
asset = MediaAsset(
|
asset = MediaAsset(
|
||||||
source_kind=source_kind,
|
source_kind=source_kind,
|
||||||
|
|
|
||||||
|
|
@ -15,6 +15,7 @@ from fastapi.responses import FileResponse
|
||||||
from sqlalchemy import func, select
|
from sqlalchemy import func, select
|
||||||
from sqlalchemy.orm import Session
|
from sqlalchemy.orm import Session
|
||||||
|
|
||||||
|
from app import sysconfig
|
||||||
from app.auth import admin_user, require_human
|
from app.auth import admin_user, require_human
|
||||||
from app.config import settings
|
from app.config import settings
|
||||||
from app.db import get_db
|
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
|
"""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,
|
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
|
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
|
again). `error` counts as holding: enqueue always +1'd ref_count and the worker never
|
||||||
after cancel doesn't double-release."""
|
decrements on failure (ref_count is the route's job), so an errored job releases here on
|
||||||
if not job.asset_id or job.status not in ("queued", "running", "paused", "done"):
|
delete — else its +1 leaks and a later resume→ready 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
|
return
|
||||||
asset = db.get(MediaAsset, job.asset_id)
|
asset = db.get(MediaAsset, job.asset_id)
|
||||||
if asset is None:
|
if asset is None:
|
||||||
|
|
@ -876,7 +886,7 @@ def admin_storage(
|
||||||
"ready_files": ready[0],
|
"ready_files": ready[0],
|
||||||
"total_bytes": int(ready[1]),
|
"total_bytes": int(ready[1]),
|
||||||
"total_assets": total_assets,
|
"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": [
|
"per_user": [
|
||||||
{"user_id": uid, "email": emails.get(uid), "footprint_bytes": quota.footprint(db, uid)}
|
{"user_id": uid, "email": emails.get(uid), "footprint_bytes": quota.footprint(db, uid)}
|
||||||
for uid, _ in per_user
|
for uid, _ in per_user
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue