peter
4ce0a702f4
Follow-up to the /code-review high on the S1 branch. The per-site `if job.status == 'running'` guards were a non-atomic read-then-write (TOCTOU): a cancel committing between the child's status read and its commit could still be overwritten with done/error, re-opening the double-release window. Introduce service.finalize_if_running(db, job_id, **fields): one atomic `UPDATE download_jobs SET ... WHERE status='running'` that returns whether it fired. It flushes pending asset writes first so the asset-then-job row-lock order matches the cancel route (no deadlock), and runs in the caller's session so it commits with the asset writes in one transaction. All five worker sites (download/edit completion, both error paths, cache finish) now go through it, so the invariant lives in one place. The 'job done' logs move inside the finalized branch so a job stopped mid-completion no longer logs a false 'done'. Add tests/test_downloads_finalize.py: running -> finalizes; canceled/paused -> no-op.
76 lines
2.7 KiB
Python
76 lines
2.7 KiB
Python
"""DB-backed regression tests for `service.finalize_if_running` (B-sub-1).
|
|
|
|
The worker's terminal writes go through this helper so a child that finishes just after a route
|
|
cancel/pause can NEVER flip the job back to done/error — which would re-open the double-release ->
|
|
file-delete-under-live-holder class (done/error hold the asset, canceled does not). The guard is an
|
|
atomic `UPDATE ... WHERE status='running'`, so these tests assert it fires exactly when the job is
|
|
still running and is a no-op otherwise. Uses the real-Postgres `db` fixture; skipped without a DB.
|
|
"""
|
|
from sqlalchemy import text
|
|
|
|
from app.downloads import service
|
|
from app.downloads.status import JobStatus
|
|
from app.models import DownloadJob, User
|
|
|
|
|
|
def _running_job(db) -> DownloadJob:
|
|
user = User(email="dl@example.com")
|
|
db.add(user)
|
|
db.flush()
|
|
job = DownloadJob(
|
|
user_id=user.id,
|
|
source_kind="youtube",
|
|
source_ref="x" * 11,
|
|
status=JobStatus.RUNNING.value,
|
|
queue_pos=1,
|
|
profile_snapshot={},
|
|
)
|
|
db.add(job)
|
|
db.commit()
|
|
return job
|
|
|
|
|
|
def _row(db, job_id):
|
|
# Read straight from the DB: SessionLocal uses expire_on_commit=False, so the ORM identity-map
|
|
# copy is stale after finalize_if_running's raw UPDATE + commit.
|
|
return db.execute(
|
|
text("SELECT status, progress FROM download_jobs WHERE id = :id"), {"id": job_id}
|
|
).one()
|
|
|
|
|
|
def test_finalize_marks_a_running_job_done_and_reports_it(db):
|
|
job = _running_job(db)
|
|
won = service.finalize_if_running(db, job.id, status=JobStatus.DONE, progress=100, phase=None)
|
|
db.commit()
|
|
assert won is True
|
|
status, progress = _row(db, job.id)
|
|
assert status == "done"
|
|
assert progress == 100
|
|
|
|
|
|
def test_finalize_is_a_noop_on_a_canceled_job(db):
|
|
# The exact B-sub-1 scenario: a route already canceled the job (released its asset once), and a
|
|
# finishing child then tries to write `done`. The guard must NOT resurrect it, or a later delete
|
|
# would double-release the asset.
|
|
job = _running_job(db)
|
|
db.execute(
|
|
text("UPDATE download_jobs SET status = 'canceled' WHERE id = :id"), {"id": job.id}
|
|
)
|
|
db.commit()
|
|
won = service.finalize_if_running(db, job.id, status=JobStatus.DONE, progress=100)
|
|
db.commit()
|
|
assert won is False
|
|
assert _row(db, job.id).status == "canceled"
|
|
|
|
|
|
def test_finalize_is_a_noop_on_a_paused_job(db):
|
|
job = _running_job(db)
|
|
db.execute(
|
|
text("UPDATE download_jobs SET status = 'paused' WHERE id = :id"), {"id": job.id}
|
|
)
|
|
db.commit()
|
|
won = service.finalize_if_running(db, job.id, status=JobStatus.ERROR, error="boom", phase=None)
|
|
db.commit()
|
|
assert won is False
|
|
assert _row(db, job.id).status == "paused"
|