peter/siftlode
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

fix(downloads): revive the worker cancel branch and guard terminal writes (B-sub-1)

Browse Source 
The cancel route writes status 'canceled' but the supervisor compared against
'cancelled', so its "a route stopped it -> kill the child, free the asset"
branch was dead code. A canceled job's child ran to completion and wrote 'done'
unconditionally over the route's 'canceled'; since 'done' holds the asset but
'canceled' does not, a later delete released the asset a second time, dropping
ref_count below the live holder count and deleting a file still in use.

Add app/downloads/status.py as the single source of truth for the lifecycle
literals (JobStatus StrEnum + the HOLDING / STOPPED_BY_ROUTE sets) and use it at
the safety-critical sites: the supervisor stop check, the route cancel, and
release_asset's holding set. Make the worker's terminal done/error writes
conditional on the row still being 'running' so a child finishing just after a
route cancel/pause can no longer resurrect it. Add the missing 'canceled'
supervisor regression test.

(The full literal sweep across the downloads surface is left for the E3
downloads owner-unification; this is the data-safety subset.)
This commit is contained in:
peter
2026-08-18 00:21:43 +02:00
parent 429fbde866
commit 367bcebc9f
5 changed files with 86 additions and 17 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/app/worker.py
+29 -13
View File
@@ -36,6 +36,7 @@ from app.db import SessionLocal
from app.downloads import edit as editmod
from app.downloads import errors as dlerrors
from app.downloads import adapters, formats, quota, service, storage
from app.downloads.status import STOPPED_BY_ROUTE, JobStatus
from app.models import DownloadJob, MediaAsset
from app.notifications import create_notification
from app.titles import normalize_title
@@ -359,7 +360,7 @@ def _run_supervised(job_id: int, asset_id: int) -> None:
except subprocess.TimeoutExpired:
pass
status, stale = _job_liveness(job_id)
if status is None or status in ("paused", "cancelled"):
if status is None or status in STOPPED_BY_ROUTE:
reason = "cancelled" # a route stopped it (or the row vanished)
break
if status == "running":
@@ -481,8 +482,11 @@ def _finish_from_cache(job_id: int, asset_id: int) -> None:
with SessionLocal() as db:
job = db.get(DownloadJob, job_id)
asset = db.get(MediaAsset, asset_id)
if job and asset:
job.status = "done"
# Only finalize a job the route hasn't stopped: the flip is conditional on the row still being
# RUNNING so a concurrent cancel/pause is never resurrected to `done` (which would break the
# no-double-release invariant in service.release_asset — see downloads/status.py).
if job and asset and job.status == JobStatus.RUNNING:
job.status = JobStatus.DONE
job.progress = 100
job.phase = None
if not job.display_name:
@@ -868,8 +872,12 @@ def _download(job_id: int, asset_id: int, source_kind: str, source_ref: str, spe
asset.last_access_at = datetime.now(timezone.utc)
asset.expires_at = expires
job = db.get(DownloadJob, job_id)
if job:
job.status = "done"
# Conditional terminal write: only finalize while the row is still RUNNING, so a child that
# finishes AFTER a route cancel/pause (narrow window past the cooperative checkpoints) can't
# overwrite `canceled`/`paused` with `done` and re-open the double-release class fixed here.
finalized = bool(job and job.status == JobStatus.RUNNING)
if finalized:
job.status = JobStatus.DONE
job.progress = 100
job.phase = None
job.speed_bps = None
@@ -880,7 +888,8 @@ def _download(job_id: int, asset_id: int, source_kind: str, source_ref: str, spe
# Capture the owner before commit expires the row (avoids a refresh SELECT just to notify).
owner_id = job.user_id if job else None
db.commit()
_notify_download_done(db, owner_id, meta.title, job_id, kind="download")
if finalized:
_notify_download_done(db, owner_id, meta.title, job_id, kind="download")
log.info("job %s done: %s", job_id, rel)
except (_Aborted, editmod.EditAborted):
@@ -896,8 +905,10 @@ def _download(job_id: int, asset_id: int, source_kind: str, source_ref: str, spe
asset.status = "error"
asset.error = msg
job = db.get(DownloadJob, job_id)
if job:
job.status = "error"
# Don't overwrite a route-set cancel/pause with `error` (error also holds the asset, so it
# would re-open the double-release class): only fail a job that is still RUNNING.
if job and job.status == JobStatus.RUNNING:
job.status = JobStatus.ERROR
job.error = msg
job.error_code = dlerrors.classify(msg)
job.phase = None
@@ -928,8 +939,10 @@ def _fail_edit(job_id: int, asset_id: int, msg: str) -> None:
asset.status = "error"
asset.error = msg
job = db.get(DownloadJob, job_id)
if job:
job.status = "error"
# Only fail a job that is still RUNNING — never overwrite a route-set cancel/pause (see the
# download error path).
if job and job.status == JobStatus.RUNNING:
job.status = JobStatus.ERROR
job.error = msg
job.error_code = dlerrors.classify(msg)
job.phase = None
@@ -1008,15 +1021,18 @@ def _process_edit(job_id: int, asset_id: int) -> None:
asset.last_access_at = datetime.now(timezone.utc)
asset.expires_at = expires
job = db.get(DownloadJob, job_id)
if job:
job.status = "done"
# Conditional terminal write (see the download path): don't resurrect a route-stopped job.
finalized = bool(job and job.status == JobStatus.RUNNING)
if finalized:
job.status = JobStatus.DONE
job.progress = 100
job.phase = None
# Capture owner + title before commit expires the rows (avoids a refresh SELECT).
owner_id = job.user_id if job else None
dl_title = asset.title if asset else None
db.commit()
_notify_download_done(db, owner_id, dl_title, job_id, kind="edit")
if finalized:
_notify_download_done(db, owner_id, dl_title, job_id, kind="edit")
log.info("edit job %s done: %s", job_id, rel)
except editmod.EditAborted: