From a867145bcb8e1ab8457edaf547d060eea4fde2f5 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sat, 10 Oct 2026 20:28:52 +0200 Subject: [PATCH] fix(downloads): a suspended user's queued jobs wait, and a clip too big for the space left says so A suspension now blocks the user's downloads as quota.resolve already advised: the worker leaves their queued jobs unclaimed until it is lifted (the confirm says so). A cut clip larger than the remaining storage is refused with its own message (not "storage is full"), and a clip's quota refusal reads as a clip's in both languages. The cancel poll compares to JobStatus.RUNNING. --- backend/app/downloads/errors.py | 10 ++++++++-- backend/app/downloads/pipeline.py | 4 ++-- backend/app/downloads/quota.py | 3 ++- backend/app/models.py | 3 ++- backend/app/routes/admin.py | 3 ++- backend/app/worker.py | 10 +++++++++- backend/tests/test_clip.py | 3 ++- backend/tests/test_download_errors.py | 1 + backend/tests/test_download_limits.py | 19 +++++++++++++++++++ frontend/src/i18n/locales/en/downloads.json | 4 ++++ frontend/src/i18n/locales/en/users.json | 2 +- frontend/src/i18n/locales/hu/downloads.json | 4 ++++ frontend/src/i18n/locales/hu/users.json | 2 +- frontend/src/lib/downloadFailure.test.ts | 16 ++++++++++++++++ 14 files changed, 73 insertions(+), 11 deletions(-) diff --git a/backend/app/downloads/errors.py b/backend/app/downloads/errors.py index e390d355..e336d871 100644 --- a/backend/app/downloads/errors.py +++ b/backend/app/downloads/errors.py @@ -37,8 +37,8 @@ from app.downloads.credentials import host_key_for_url # Worker-authored strings that this module owns because each is coupled to `_RULES`. # -# QUOTA_FULL_MESSAGE is raised by the worker and must classify back to `quota_exceeded`; its rule -# below matches a fragment of it, and test_download_errors pins the pair, because a message and its +# QUOTA_FULL_MESSAGE and CLIP_TOO_BIG_MESSAGE are raised by the worker and must classify back to +# `quota_exceeded`; their rules below match a fragment of each, and test_download_errors pins the pair, because a message and its # rule drifting apart is exactly how a category silently becomes unreachable. # # NETWORK_REDACTED_MESSAGE REPLACES a message after classification (`user_error_text`). It is written @@ -53,6 +53,11 @@ from app.downloads.credentials import host_key_for_url # constants, imported above; each has a rule below that gives its category back, and a catalog entry # that gives the site's sentence. QUOTA_FULL_MESSAGE = "Your download storage is full — free up space, then try again." +# A cut clip held to the space left, which is not zero: "full" would be untrue. +CLIP_TOO_BIG_MESSAGE = ( + "The clip is larger than the download storage you have left — " + "free up space or cut a shorter clip, then try again." +) UPLOAD_MISSING_MESSAGE = "The uploaded file is missing." NETWORK_REDACTED_MESSAGE = "The server could not reach that address." @@ -62,6 +67,7 @@ _RULES: tuple[tuple[str, str], ...] = ( # Our own refusal: the worker found the user already at their storage cap (SEC-13). Placed # first — it is our wording, so it can never be a false match on an upstream message. ("download storage is full", "quota_exceeded"), + ("larger than the download storage you have left", "quota_exceeded"), # Our own redaction of a transport failure (see NETWORK_REDACTED_MESSAGE) — our wording too. (NETWORK_REDACTED_MESSAGE.lower(), "network"), # Our own verdicts on a post link (adapters.py) — our wording too: its page holds no single video, diff --git a/backend/app/downloads/pipeline.py b/backend/app/downloads/pipeline.py index 21e2ebea..28baf133 100644 --- a/backend/app/downloads/pipeline.py +++ b/backend/app/downloads/pipeline.py @@ -226,7 +226,7 @@ def _cancel_poll(job_id: int, interval_s: float = _CANCEL_POLL_INTERVAL_S, *, no def cancelled() -> bool: if not state["cancelled"] and allow(): - state["cancelled"] = _job_status(job_id) not in ("running", None) + state["cancelled"] = _job_status(job_id) not in (JobStatus.RUNNING, None) return state["cancelled"] return cancelled @@ -1061,7 +1061,7 @@ def _process_clip(job_id: int, asset_id: int) -> None: # again, since other jobs of this owner may have landed during the encode. remaining = _clip_room(user_id) if remaining is not None and out.stat().st_size > remaining: - raise _OwnerRefused(dlerrors.QUOTA_FULL_MESSAGE) + raise _OwnerRefused(dlerrors.CLIP_TOO_BIG_MESSAGE) ext = out.suffix.lstrip(".").lower() or "mp4" rel = storage.edit_rel_path(user_id, asset_id, ext) storage.place_file(out, settings.download_root, rel) diff --git a/backend/app/downloads/quota.py b/backend/app/downloads/quota.py index 422c926c..d5832910 100644 --- a/backend/app/downloads/quota.py +++ b/backend/app/downloads/quota.py @@ -67,7 +67,8 @@ def resolve(db: Session, user_id: int) -> QuotaLimits: # "at limit" checks — the worker's claim-window exclusion vs at_concurrency_limit — since a # 0-limit user is "capped" with zero running jobs, which the SQL exclusion can't see. Keep # them consistent by never letting the effective limit drop below 1. (To BLOCK a user's - # downloads, suspend them or set max_jobs=0, not max_concurrent=0.) + # downloads, suspend them — the worker leaves a suspended user's queued jobs unclaimed — or + # set max_jobs=0, not max_concurrent=0.) return QuotaLimits(row.max_bytes, max(1, row.max_concurrent), row.max_jobs, row.unlimited) return QuotaLimits( max_bytes=sysconfig.get_int(db, "download_default_max_bytes"), diff --git a/backend/app/models.py b/backend/app/models.py index 8d899b1b..ec4c780c 100644 --- a/backend/app/models.py +++ b/backend/app/models.py @@ -123,7 +123,8 @@ class User(Base, TimestampMixin, CanonicalEmailMixin): is_active: Mapped[bool] = mapped_column(Boolean, default=True, server_default="true") # Admin-imposed access block, distinct from is_active (approval lifecycle). A suspended # account can't sign in by any method and its live sessions are rejected; the user is told - # why and pointed at the operator's contact. + # why and pointed at the operator's contact. The worker leaves its queued downloads unclaimed + # until the block is lifted (`worker._claim_job`). is_suspended: Mapped[bool] = mapped_column(Boolean, default=False, server_default="false") # Server-side session revocation (SA4): a monotonic counter bumped on password reset, password # change, or "log out everywhere". A signed session cookie records the epoch it was minted under; diff --git a/backend/app/routes/admin.py b/backend/app/routes/admin.py index 331a1fdc..f4a87fae 100644 --- a/backend/app/routes/admin.py +++ b/backend/app/routes/admin.py @@ -484,7 +484,8 @@ def set_user_suspended( db: Session = Depends(get_db), ) -> dict: """Suspend or un-suspend an account. A suspended user can't sign in by any method and any - live session is dropped on its next request. Guards: can't suspend yourself, and can't suspend the last active admin (that would lock admin out). On + live session is dropped on its next request; their queued downloads wait (the worker skips them) + until they are un-suspended. Guards: can't suspend yourself, and can't suspend the last active admin (that would lock admin out). On un-suspend the user is emailed they can return (suspend is announced reactively, on a blocked sign-in).""" suspended = body.suspended diff --git a/backend/app/worker.py b/backend/app/worker.py index e644dbfb..d185a01a 100644 --- a/backend/app/worker.py +++ b/backend/app/worker.py @@ -163,7 +163,11 @@ def _claim_job() -> int | None: seen.) The per-user limit is the quota row's `max_concurrent` (else the sysconfig default); `unlimited` users are never capped. A final Python `at_concurrency_limit` re-check still guards the tiny race between the exclusion subquery and the claim (it can only ever reject, never - over-claim), so correctness never depends on the two observations agreeing.""" + over-claim), so correctness never depends on the two observations agreeing. + + A suspended user's queued jobs are left where they are: suspension blocks their downloads (not + only their sign-in), and lifting it lets them run in their old place. A job already running + when the user is suspended finishes.""" with SessionLocal() as db: default_mc = sysconfig.get_int(db, "download_default_max_concurrent") rows = db.execute( @@ -171,6 +175,10 @@ def _claim_job() -> int | None: """ SELECT id, user_id FROM download_jobs WHERE status='queued' + AND NOT EXISTS ( + SELECT 1 FROM users u + WHERE u.id = download_jobs.user_id AND u.is_suspended + ) AND NOT EXISTS ( SELECT 1 FROM media_assets a WHERE a.id = download_jobs.asset_id AND a.status = 'downloading' diff --git a/backend/tests/test_clip.py b/backend/tests/test_clip.py index 8e7d7f27..8e06c5c0 100644 --- a/backend/tests/test_clip.py +++ b/backend/tests/test_clip.py @@ -696,9 +696,10 @@ def test_a_clip_larger_than_what_its_owner_has_left_is_refused_and_not_placed( job = _clip(as_user(make_user()), source=source, ref=ref).json() monkeypatch.setattr(quota, "remaining_bytes", lambda _db, _uid: 3) # the cut is 4 bytes job, asset = worker["run"](job) + # Not "full": there is room left, just not this much. assert (job.status, job.error, asset.status, asset.rel_path) == ( "error", - dlerrors.QUOTA_FULL_MESSAGE, + dlerrors.CLIP_TOO_BIG_MESSAGE, "pending", None, ) diff --git a/backend/tests/test_download_errors.py b/backend/tests/test_download_errors.py index 285a5c5f..6b6e0e5d 100644 --- a/backend/tests/test_download_errors.py +++ b/backend/tests/test_download_errors.py @@ -63,6 +63,7 @@ def test_our_own_worker_refusals_classify_as_themselves(): # fragment of the text. Pinning message and rule together is what stops an edit to the wording # from silently orphaning the category — the UI would then fall back to "Download failed". assert classify(dlerrors.QUOTA_FULL_MESSAGE) == "quota_exceeded" + assert classify(dlerrors.CLIP_TOO_BIG_MESSAGE) == "quota_exceeded" def test_the_redacted_network_text_classifies_back_to_network(): diff --git a/backend/tests/test_download_limits.py b/backend/tests/test_download_limits.py index 2237dbd3..b1958c8f 100644 --- a/backend/tests/test_download_limits.py +++ b/backend/tests/test_download_limits.py @@ -155,6 +155,25 @@ def test_unlimited_user_is_never_excluded(db): assert _claim_job() == q.id +def test_a_suspended_users_queued_jobs_wait(db): + # Suspending a user blocks their downloads (quota.resolve's advice): a job they queued before + # stays queued, unclaimed, and runs once the suspension is lifted. + s = User(email="suspended@example.com", is_suspended=True) + o = User(email="other@example.com") + db.add_all([s, o]) + db.flush() + waits = _job(db, s.id, queue_pos=1) + served = _job(db, o.id, queue_pos=2) + db.commit() + assert _claim_job() == served.id + assert _claim_job() is None + db.expire_all() + assert db.get(DownloadJob, waits.id).status == "queued" + db.get(User, s.id).is_suspended = False + db.commit() + assert _claim_job() == waits.id + + # --- #3 · list cap -------------------------------------------------------------------------- diff --git a/frontend/src/i18n/locales/en/downloads.json b/frontend/src/i18n/locales/en/downloads.json index 93d65bf6..d9869a81 100644 --- a/frontend/src/i18n/locales/en/downloads.json +++ b/frontend/src/i18n/locales/en/downloads.json @@ -247,6 +247,10 @@ } }, "edit": { + "quota_exceeded": { + "short": "Not enough storage for the clip", + "body": "This clip doesn't fit in the storage you have left. Delete something from your library, or cut a shorter clip, then try again." + }, "unknown": { "short": "Couldn't make the clip", "body": "Something went wrong while making this clip. The technical details below may help." diff --git a/frontend/src/i18n/locales/en/users.json b/frontend/src/i18n/locales/en/users.json index 75b6f944..11fec23c 100644 --- a/frontend/src/i18n/locales/en/users.json +++ b/frontend/src/i18n/locales/en/users.json @@ -45,7 +45,7 @@ "undoAction": "Unsuspend", "title": "Suspend account?", "undoTitle": "Unsuspend account?", - "confirm": "Suspend {{email}}? They won't be able to sign in (by password or Google) until you unsuspend them, and any active session ends.", + "confirm": "Suspend {{email}}? They won't be able to sign in (by password or Google) until you unsuspend them, and any active session ends. Their queued downloads wait until then.", "undoConfirm": "Unsuspend {{email}}? They'll be able to sign in again.", "done": "{{email}} suspended", "undone": "{{email}} unsuspended", diff --git a/frontend/src/i18n/locales/hu/downloads.json b/frontend/src/i18n/locales/hu/downloads.json index 3b371083..b7b3ba68 100644 --- a/frontend/src/i18n/locales/hu/downloads.json +++ b/frontend/src/i18n/locales/hu/downloads.json @@ -247,6 +247,10 @@ } }, "edit": { + "quota_exceeded": { + "short": "Nincs elég hely a klipnek", + "body": "Ez a klip nem fér bele a megmaradt tárhelyedbe. Törölj valamit a könyvtáradból, vagy vágj rövidebb klipet, majd próbáld újra." + }, "unknown": { "short": "Nem sikerült elkészíteni a klipet", "body": "Hiba történt a klip készítése közben. Az alábbi technikai részletek segíthetnek." diff --git a/frontend/src/i18n/locales/hu/users.json b/frontend/src/i18n/locales/hu/users.json index 7fdb2749..1e4330fe 100644 --- a/frontend/src/i18n/locales/hu/users.json +++ b/frontend/src/i18n/locales/hu/users.json @@ -45,7 +45,7 @@ "undoAction": "Feloldás", "title": "Felfüggeszted a fiókot?", "undoTitle": "Feloldod a felfüggesztést?", - "confirm": "Felfüggeszted: {{email}}? Nem fog tudni belépni (sem jelszóval, sem Google-lel), amíg fel nem oldod, és az aktív munkamenete megszűnik.", + "confirm": "Felfüggeszted: {{email}}? Nem fog tudni belépni (sem jelszóval, sem Google-lel), amíg fel nem oldod, és az aktív munkamenete megszűnik. A sorban álló letöltései addig várnak.", "undoConfirm": "Feloldod a felfüggesztést: {{email}}? Újra be tud majd lépni.", "done": "{{email}} felfüggesztve", "undone": "{{email}} felfüggesztése feloldva", diff --git a/frontend/src/lib/downloadFailure.test.ts b/frontend/src/lib/downloadFailure.test.ts index 0adcff5f..39e104f2 100644 --- a/frontend/src/lib/downloadFailure.test.ts +++ b/frontend/src/lib/downloadFailure.test.ts @@ -84,6 +84,22 @@ describe("failTitleKey", () => { } }); + it("a clip refused for storage says so of a clip, and not that the quota is used up", async () => { + // Refused before it is cut (nothing left) or after (the cut is bigger than what is left): one + // sentence true of both, worded for a clip and naming the clip-side way out. + for (const job_kind of ["clip", "edit"] as const) { + const clip = { error_code: "quota_exceeded", error_detail: null, job_kind } as const; + for (const lng of ["en", "hu"]) { + await i18n.changeLanguage(lng); + for (const part of ["short", "body"] as const) { + const own = i18n.t(failCopyKeys(clip, part)); + expect(own).not.toMatch(/^downloads\./); + expect(own).not.toBe(i18n.t(`downloads.errors.fail.cat.quota_exceeded.${part}`)); + } + } + } + }); + it("a clip that fails for no known reason says so of a clip, in both languages", async () => { // Not "Couldn't download / Something went wrong with this download" — nothing was downloaded. const clip = { error_code: "unknown", error_detail: null, job_kind: "edit" } as const;