fix(downloads): E11.5 epic-end review round 2
Pause now re-reads the job under the same asset-then-job row lock as resume, cancel, delete and redownload. Its unlocked write waited for a concurrent cancel's lock and then set the released job back to paused, a holding status, so a later delete released the same hold again and could free a file another user's job still held (measured: the overlapping-request test ends 'paused' instead of 'canceled' with the old read). The lock test now calls the route handlers, so it also pins which helper each route uses. Comments follow the code: resolve_url's contract names both verdicts and that a failed page read is raised as itself; the inherited-failure diagnosis asks the cookie gate as it stands when the job is claimed, so its advice fits the cookie that job's Retry would use; the undecryptable-cookie warning no longer claims a download goes ahead, since the worker parent reaches it only to diagnose.
This commit is contained in:
@@ -4,23 +4,31 @@ The routes that move a job's asset ref count (resume, cancel, delete, redownload
|
||||
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.
|
||||
A pause racing a cancel did the same one step later: its unlocked write turned the released job back
|
||||
into a holder, which a later delete releases again. `_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 sqlalchemy import select
|
||||
|
||||
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
|
||||
from app.routes.downloads import (
|
||||
_own_job_for_update,
|
||||
cancel_download,
|
||||
delete_download,
|
||||
pause_download,
|
||||
)
|
||||
|
||||
_REF = "https://example.com/video"
|
||||
|
||||
|
||||
def _asset_held_by_two_jobs(db):
|
||||
def _asset_held_by_two_jobs(db, status="error"):
|
||||
mine, theirs = User(email="lock-mine@example.com"), User(email="lock-theirs@example.com")
|
||||
db.add_all([mine, theirs])
|
||||
db.flush()
|
||||
@@ -35,7 +43,7 @@ def _asset_held_by_two_jobs(db):
|
||||
asset_id=asset.id,
|
||||
source_kind="url",
|
||||
source_ref=_REF,
|
||||
status="error",
|
||||
status=status if owner is mine else "error",
|
||||
queue_pos=0,
|
||||
profile_snapshot={},
|
||||
)
|
||||
@@ -46,23 +54,33 @@ def _asset_held_by_two_jobs(db):
|
||||
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)
|
||||
@pytest.mark.parametrize(
|
||||
("status", "transition", "route", "left"),
|
||||
[
|
||||
("error", service.cancel, cancel_download, "canceled"),
|
||||
("error", service.delete, delete_download, None),
|
||||
("queued", service.cancel, pause_download, "canceled"),
|
||||
],
|
||||
)
|
||||
def test_overlapping_requests_for_one_job_release_its_hold_once(
|
||||
db, status, transition, route, left
|
||||
):
|
||||
user, asset, job = _asset_held_by_two_jobs(db, status)
|
||||
job_id = job.id # read before the delete case removes the row under the expired instance
|
||||
first, second = SessionLocal(), SessionLocal()
|
||||
finished = threading.Event()
|
||||
|
||||
def the_other_request():
|
||||
try:
|
||||
transition(second, _own_job_for_update(second, user, job.id))
|
||||
route(job_id, user=user, db=second)
|
||||
except HTTPException:
|
||||
pass # a delete after the delete: the job is gone, a 404
|
||||
pass # a request after the delete: the job is gone, a 404
|
||||
finally:
|
||||
second.close()
|
||||
finished.set()
|
||||
|
||||
try:
|
||||
locked = _own_job_for_update(first, user, job.id)
|
||||
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.
|
||||
@@ -74,6 +92,8 @@ def test_overlapping_requests_for_one_job_release_its_hold_once(db, transition):
|
||||
first.close()
|
||||
|
||||
db.expire_all()
|
||||
mine = select(DownloadJob.status).where(DownloadJob.id == job_id)
|
||||
assert db.execute(mine).scalar_one_or_none() == left
|
||||
assert db.get(MediaAsset, asset.id).ref_count == 1 # the other user's job still holds it
|
||||
|
||||
|
||||
|
||||
Reference in New Issue
Block a user