fix(downloads): second-round review fixes (fork on re-download, title fallbacks)
- redownload now ALWAYS rebuilds into a fresh private asset instead of ever reusing a shared cached file: the cache sig is salted with job_id + current asset_id, so the fork is distinct from both the dedup pool and the job's own current asset. This resolves the co-held silent no-op (finding #1) — a shared asset is left intact for its other holders while THIS job re-fetches its own copy. Resolving the fork BEFORE releasing the old hold (and the unique salted key) also removes the IntegrityError-rollback-after-file-delete window (finding #2). - format_sig gains an optional `salt` for that fork identity. - titles: don't consume the space after `|`, so a "stats | #hashtags" remainder keeps the leading whitespace _TRAILING_HASHTAGS needs — and fall back to the prefix+hashtag-stripped text, so neither the stats nor the SEO hashtags are resurrected when a title empties (#3). - worker: early-fill asset.title falls back to the video id so a title that cleaned to empty doesn't show a blank card while downloading (#5). - Tests for the salt fork and the hashtag-remainder case. (Finding #4 — quota on re-download — intentionally not changed: a re-download reuses the existing job and re-fetches ~the same bytes, so calling check_enqueue would wrongly reject on the job-count cap while footprint is unchanged.)
This commit is contained in:
@@ -78,11 +78,16 @@ def normalize(spec: dict) -> dict:
|
||||
return s
|
||||
|
||||
|
||||
def format_sig(spec: dict) -> str:
|
||||
"""Stable short hash of the output-affecting spec — the MediaAsset cache key component."""
|
||||
def format_sig(spec: dict, salt: str | None = None) -> str:
|
||||
"""Stable short hash of the output-affecting spec — the MediaAsset cache key component.
|
||||
|
||||
`salt` forks a distinct, non-dedup identity for the same spec (used by force-redownload so it
|
||||
always re-fetches into its own asset instead of sharing a cached file with other jobs)."""
|
||||
s = normalize(spec)
|
||||
data = {k: s[k] for k in _SIG_KEYS}
|
||||
data["_v"] = _SIG_VERSION
|
||||
if salt is not None:
|
||||
data["_salt"] = salt
|
||||
payload = json.dumps(data, sort_keys=True, separators=(",", ":"))
|
||||
return hashlib.sha256(payload.encode()).hexdigest()[:24]
|
||||
|
||||
|
||||
@@ -457,25 +457,30 @@ def redownload_download(
|
||||
The SAME job row is kept, so any public share link (DownloadLink → job_id) keeps working and
|
||||
resolves to the newly-downloaded file — a re-download never breaks a link you already sent out
|
||||
(only a *failed* re-download leaves the job in `error`, which is the intended broken-link case).
|
||||
Useful after a format/naming fix: the old file is deleted and re-fetched under the current rules
|
||||
(the format-sig bump alone re-points this to a fresh asset)."""
|
||||
|
||||
The rebuild always re-fetches into a FRESH, private asset (the cache sig is salted with the job
|
||||
id + current asset id) rather than the dedup pool. That means it (a) always genuinely rebuilds
|
||||
under the current format/naming rules — never silently reuses a shared cached file — and (b)
|
||||
never deletes a file another job/user still shares: the old asset is only removed if THIS job
|
||||
was its last holder."""
|
||||
job = _own_job(db, user, job_id)
|
||||
if job.job_kind == "edit":
|
||||
raise HTTPException(status_code=409, detail="Editor clips can't be re-downloaded.")
|
||||
if job.status in ("queued", "running", "paused"):
|
||||
raise HTTPException(status_code=409, detail="This download is still in progress.")
|
||||
|
||||
# Drop this job's hold on its current asset. `_release_asset` deletes the file + row when THIS
|
||||
# job was the last holder (ref→0, ready) — so a solo re-download genuinely re-fetches. When the
|
||||
# asset is still shared by another job/user we must NOT delete it out from under them: leaving it
|
||||
# `ready` means the re-queued job just re-points to the shared file (the worker short-circuits a
|
||||
# ready asset via `_finish_from_cache`), which is why there's no force-reset-to-pending here.
|
||||
_release_asset(db, job)
|
||||
# Resolve the fresh private asset FIRST (before touching any file). Salting with the current
|
||||
# asset id makes it distinct from both the dedup pool and this job's current asset, so the
|
||||
# insert never collides — no IntegrityError/rollback can strand us after a file was deleted.
|
||||
spec = formats.normalize(job.profile_snapshot)
|
||||
sig = formats.format_sig(spec)
|
||||
sig = formats.format_sig(spec, salt=f"{job.id}:{job.asset_id}")
|
||||
asset = service.get_or_create_asset(db, job.source_kind, job.source_ref, sig)
|
||||
service.populate_from_catalog(db, asset)
|
||||
|
||||
# Now drop the old hold — `_release_asset` deletes the old file + row only if this job was its
|
||||
# last holder (a co-held asset is left intact for its other holders).
|
||||
_release_asset(db, job)
|
||||
|
||||
asset.ref_count = (asset.ref_count or 0) + 1
|
||||
job.asset_id = asset.id
|
||||
job.status = "queued"
|
||||
|
||||
+10
-5
@@ -25,9 +25,12 @@ import unicodedata
|
||||
_DROP_CATEGORIES = {"So", "Sk", "Cc", "Cf", "Cs", "Co", "Cn"}
|
||||
# Social engagement prefix some sources (Facebook) prepend, e.g. "1.6M views · 66K reactions | ".
|
||||
# Match one-or-more "<count> <label>" tokens (· / , separated) terminated by a pipe, then drop it.
|
||||
# Deliberately does NOT consume the space after `|`: leaving it lets a hashtag-only remainder
|
||||
# ("… | #foo #bar") keep the leading whitespace `_TRAILING_HASHTAGS` needs to strip it; the normal
|
||||
# case's leading space is removed later by the whitespace collapse.
|
||||
_ENGAGEMENT_PREFIX = re.compile(
|
||||
r"^\s*(?:\d[\d.,]*\s*[KMB]?\s*"
|
||||
r"(?:views?|reactions?|likes?|comments?|shares?|plays?|followers?)\s*[·,;]?\s*)+\|\s*",
|
||||
r"(?:views?|reactions?|likes?|comments?|shares?|plays?|followers?)\s*[·,;]?\s*)+\|",
|
||||
re.IGNORECASE,
|
||||
)
|
||||
_LETTER_RUN = re.compile(r"[^\W\d_]+", re.UNICODE)
|
||||
@@ -61,6 +64,7 @@ def normalize_title(raw: str | None) -> str | None:
|
||||
stripped = _ENGAGEMENT_PREFIX.sub("", t)
|
||||
prefix_removed = stripped != t
|
||||
t = _TRAILING_HASHTAGS.sub("", stripped)
|
||||
base = t # after prefix + trailing-hashtag removal — the fallback if de-shouting empties it
|
||||
|
||||
multi = [r for r in _LETTER_RUN.findall(t) if len(r) >= 2]
|
||||
caps = [r for r in multi if r.isupper()]
|
||||
@@ -88,7 +92,8 @@ def normalize_title(raw: str | None) -> str | None:
|
||||
if ch.isalpha():
|
||||
t = t[:i] + ch.upper() + t[i + 1:]
|
||||
break
|
||||
# Nothing survived cleaning: fall back to the engagement-stripped text (never resurrect a
|
||||
# stats prefix). Only use the untouched raw when no prefix was there to strip — e.g. an
|
||||
# all-emoji title whose glyphs were dropped, where returning the original is the lesser evil.
|
||||
return t or stripped.strip() or ("" if prefix_removed else raw)
|
||||
# Nothing survived cleaning: fall back to the prefix+hashtag-stripped text (so neither the
|
||||
# stats prefix nor the SEO hashtags are resurrected). Only use the untouched raw when no prefix
|
||||
# was there to strip — e.g. an all-emoji title whose glyphs were dropped, where returning the
|
||||
# original is the lesser evil.
|
||||
return t or base.strip() or ("" if prefix_removed else raw)
|
||||
|
||||
@@ -249,7 +249,10 @@ def _fill_asset_meta_early(asset_id: int, info: dict) -> None:
|
||||
asset = db.get(MediaAsset, asset_id)
|
||||
if asset is None or asset.title:
|
||||
return
|
||||
asset.title = normalize_title(info.get("title"))
|
||||
# `or info.get("id")` avoids a blank card while downloading when the cleaned title is empty
|
||||
# (e.g. a title that was only a social engagement prefix); the completion path later sets
|
||||
# the final title with its own source_ref fallback.
|
||||
asset.title = normalize_title(info.get("title")) or info.get("id")
|
||||
asset.uploader = info.get("uploader") or info.get("channel")
|
||||
asset.uploader_url = _uploader_url(info)
|
||||
asset.thumbnail_url = info.get("thumbnail")
|
||||
|
||||
@@ -17,6 +17,16 @@ def test_mp4_default_selects_avc1_with_progressive_fallback():
|
||||
assert fmt.endswith("bestvideo+bestaudio/best")
|
||||
|
||||
|
||||
def test_format_sig_salt_forks_a_distinct_identity():
|
||||
spec = {"mode": "av", "container": "mp4", "max_height": 720}
|
||||
base = formats.format_sig(spec)
|
||||
# A salt yields a distinct cache identity (force-redownload's private-asset fork); different
|
||||
# salts differ from each other and from the unsalted sig, same salt is stable.
|
||||
assert formats.format_sig(spec, salt="7:12") != base
|
||||
assert formats.format_sig(spec, salt="7:12") != formats.format_sig(spec, salt="7:13")
|
||||
assert formats.format_sig(spec, salt="7:12") == formats.format_sig(spec, salt="7:12")
|
||||
|
||||
|
||||
def test_mp4_default_keeps_h264_sort_tiebreaker():
|
||||
sort = _opts({"mode": "av", "container": "mp4"})["format_sort"]
|
||||
assert "vcodec:h264" in sort and "acodec:aac" in sort
|
||||
|
||||
@@ -68,3 +68,11 @@ def test_engagement_only_title_does_not_resurrect_the_stats():
|
||||
out = normalize_title("1.6M views · 66K reactions |")
|
||||
assert out == ""
|
||||
assert "views" not in (out or "") and "reactions" not in (out or "")
|
||||
|
||||
|
||||
def test_prefix_plus_only_hashtags_resurrects_neither_stats_nor_hashtags():
|
||||
# stats prefix + trailing SEO hashtags → both are removed; the fallback must not bring back the
|
||||
# hashtags that rule 2 strips (nor the stats prefix).
|
||||
out = normalize_title("5 views | #foo #bar")
|
||||
assert out == ""
|
||||
assert "#foo" not in (out or "") and "views" not in (out or "")
|
||||
|
||||
Reference in New Issue
Block a user