fix(downloads): E11.5 epic-end review round 1
A refused Facebook cookie on a group post is no longer "no video in this post":
measured on dev, Facebook answers with its login page (no post id at all), so the
adapter now raises its own POST_NOT_SHOWN verdict and the catalog explains it as
the refused cookie. The adapter owns both post verdicts and the "cannot parse
data" wall wording, so the Chrome retry and the catalog read one constant. The
group-post "no formats" entry is cookie-scoped (with a cookie yt-dlp only gets the
post's video link) and uses the adapter's own group-post test instead of a
"/groups/" substring.
Job transitions that move an asset's ref count (resume, cancel, delete,
redownload) re-read the job under a row lock, asset first, so two overlapping
requests can no longer release the same hold. get_or_create_asset loses an insert
race inside a SAVEPOINT instead of rolling back the caller's pending writes (Retry
lost its cleared error). Every worker-side job failure goes through
finalize_if_running and writes error_detail, so a cancel that landed first is kept
and a failure never shows an earlier one's copy. The worker asks the download
child's own cookie gate (credentials.usable_cookie) when a job inherits a failed
asset.
UI: the Facebook copy no longer points at a Retry button the failure window does
not have, says "paste" like Settings, and the HU wording is fixed
("Újraletöltés" on the re-download confirm). A category this client has no copy
for falls back to the generic sentence instead of a raw key.
Tests: route serialization of error_detail, the probed duration through a real
_download, the page-read cap and a failed read, a Retry off a pending asset, the
confirm-button labels, the lock and savepoint races.
This commit is contained in:
@@ -0,0 +1,119 @@
|
||||
"""Overlapping requests on ONE job (E11.5 review) — DB-backed, two real sessions.
|
||||
|
||||
The routes that move a job's asset ref count (resume, cancel, delete, redownload) used to decide on an
|
||||
unlocked read: two overlapping requests for one job (a double-click, a second tab, a replay after a
|
||||
gateway timeout) both saw it holding its asset and both released the hold. The count then reaches 0
|
||||
while another job still holds the asset, and GC or the ready-file delete removes it under that job.
|
||||
`_own_job_for_update` makes the second request wait and decide on the first one's committed result.
|
||||
"""
|
||||
|
||||
import threading
|
||||
|
||||
import pytest
|
||||
from fastapi import HTTPException
|
||||
|
||||
from app.db import SessionLocal
|
||||
from app.downloads import service
|
||||
from app.models import DownloadJob, MediaAsset, User
|
||||
from app.routes.downloads import _own_job_for_update
|
||||
|
||||
_REF = "https://example.com/video"
|
||||
|
||||
|
||||
def _asset_held_by_two_jobs(db):
|
||||
mine, theirs = User(email="lock-mine@example.com"), User(email="lock-theirs@example.com")
|
||||
db.add_all([mine, theirs])
|
||||
db.flush()
|
||||
asset = MediaAsset(
|
||||
source_kind="url", source_ref=_REF, format_sig="shared", status="error", ref_count=2
|
||||
)
|
||||
db.add(asset)
|
||||
db.flush()
|
||||
jobs = [
|
||||
DownloadJob(
|
||||
user_id=owner.id,
|
||||
asset_id=asset.id,
|
||||
source_kind="url",
|
||||
source_ref=_REF,
|
||||
status="error",
|
||||
queue_pos=0,
|
||||
profile_snapshot={},
|
||||
)
|
||||
for owner in (mine, theirs)
|
||||
]
|
||||
db.add_all(jobs)
|
||||
db.commit()
|
||||
return mine, asset, jobs[0]
|
||||
|
||||
|
||||
@pytest.mark.parametrize("transition", [service.cancel, service.delete])
|
||||
def test_overlapping_requests_for_one_job_release_its_hold_once(db, transition):
|
||||
user, asset, job = _asset_held_by_two_jobs(db)
|
||||
first, second = SessionLocal(), SessionLocal()
|
||||
finished = threading.Event()
|
||||
|
||||
def the_other_request():
|
||||
try:
|
||||
transition(second, _own_job_for_update(second, user, job.id))
|
||||
except HTTPException:
|
||||
pass # a delete after the delete: the job is gone, a 404
|
||||
finally:
|
||||
second.close()
|
||||
finished.set()
|
||||
|
||||
try:
|
||||
locked = _own_job_for_update(first, user, job.id)
|
||||
other = threading.Thread(target=the_other_request)
|
||||
other.start()
|
||||
# The second request waits for the first one's locks instead of deciding on the same row.
|
||||
assert not finished.wait(0.5), "the second request did not wait for the first"
|
||||
transition(first, locked)
|
||||
other.join(10)
|
||||
assert finished.is_set()
|
||||
finally:
|
||||
first.close()
|
||||
|
||||
db.expire_all()
|
||||
assert db.get(MediaAsset, asset.id).ref_count == 1 # the other user's job still holds it
|
||||
|
||||
|
||||
def test_losing_the_asset_insert_race_keeps_the_callers_own_writes(db, monkeypatch):
|
||||
# Retry clears the job's error, THEN may create the cookie asset. When another request inserted that
|
||||
# asset first, the loser used to roll the whole session back — and requeue the job with its stale
|
||||
# error, which a stall or crash later leaves showing under the new failure.
|
||||
user = User(email="race@example.com")
|
||||
db.add(user)
|
||||
db.flush()
|
||||
job = DownloadJob(
|
||||
user_id=user.id,
|
||||
source_kind="url",
|
||||
source_ref=_REF,
|
||||
status="error",
|
||||
error="the old failure",
|
||||
queue_pos=0,
|
||||
profile_snapshot={},
|
||||
)
|
||||
db.add(job)
|
||||
db.commit()
|
||||
job.error = None # the caller's pending write
|
||||
|
||||
real_flush, raced = db.flush, []
|
||||
|
||||
def flush_after_the_other_request_inserted(*args, **kwargs):
|
||||
if not raced: # the other request commits the same asset between our lookup and our insert
|
||||
raced.append(True)
|
||||
with SessionLocal() as other:
|
||||
other.add(
|
||||
MediaAsset(source_kind="url", source_ref=_REF, format_sig="raced", ref_count=0)
|
||||
)
|
||||
other.commit()
|
||||
return real_flush(*args, **kwargs)
|
||||
|
||||
monkeypatch.setattr(db, "flush", flush_after_the_other_request_inserted)
|
||||
|
||||
asset = service.get_or_create_asset(db, "url", _REF, "raced")
|
||||
db.commit()
|
||||
|
||||
assert raced and asset.format_sig == "raced"
|
||||
db.expire_all()
|
||||
assert db.get(DownloadJob, job.id).error is None
|
||||
Reference in New Issue
Block a user