diff --git a/backend/app/downloads/errors.py b/backend/app/downloads/errors.py index d4f8f403..1d4a7ccb 100644 --- a/backend/app/downloads/errors.py +++ b/backend/app/downloads/errors.py @@ -18,8 +18,9 @@ explanation (and decides whether a retry makes sense). The raw text stays availa # it deliberately has no rule. Nothing re-classifies a stored `job.error` today; if anything ever # does, it must carry the original code rather than re-deriving one from this text. # -# Other worker-raised texts (ffmpeg, "produced no output file", the urlguard refusal) are still -# inline literals at their raise sites — pinned by markers here, but not owned here. +# Other worker-raised texts are still inline literals at their raise sites. The ffmpeg ones and +# "produced no output file" have markers here; the urlguard refusal deliberately has none and +# classifies as `unknown` — it is a refusal, not a diagnosis, and no category would say more. QUOTA_FULL_MESSAGE = "Your download storage is full — free up space, then try again." NETWORK_REDACTED_MESSAGE = "The server could not reach that address." diff --git a/backend/app/downloads/pipeline.py b/backend/app/downloads/pipeline.py index 4cc3ef27..e200cb27 100644 --- a/backend/app/downloads/pipeline.py +++ b/backend/app/downloads/pipeline.py @@ -570,7 +570,6 @@ def _download( # Fail this job only. The asset is untouched and goes back to `pending`, so another user's # job for the same video is claimed and downloaded normally instead of inheriting a reason # that is about someone else's account. - _reset_asset_pending(asset_id) msg = str(exc)[:500] with SessionLocal() as db: service.finalize_if_running( @@ -582,6 +581,10 @@ def _download( phase=None, ) db.commit() + # Released AFTER the job is terminal: the moment the asset is `pending` another job can claim + # it, and this one must already be out of the way. (The staging tree is still torn down in + # `finally` below — the same narrow window the pause/cancel path above has always had.) + _reset_asset_pending(asset_id) log.info("job %s refused for its owner: %s", job_id, msg) except Exception as exc: # noqa: BLE001 — surface any failure to the user, keep worker alive msg = str(exc)[:500] diff --git a/backend/app/routes/downloads.py b/backend/app/routes/downloads.py index 2333389e..a6dbb662 100644 --- a/backend/app/routes/downloads.py +++ b/backend/app/routes/downloads.py @@ -68,12 +68,15 @@ def resolve_source(raw: str) -> tuple[str, str]: if raw.startswith(("http://", "https://")): # `urlparse` raises ValueError on a malformed authority ("http://[::1/x"), and this runs # BEFORE the urlguard check that would otherwise turn it into a structured refusal — so - # unguarded, that typo escapes as a 500. Same answer as any other unusable link. + # unguarded, that typo escapes as a 500. Answered in urlguard's own shape, because it is the + # same typo class: ":99999" is caught there and localized, and one of the two arriving as a + # bare English string would be exactly the split the canonical-patterns table forbids. try: u = urlparse(raw) except ValueError as exc: raise HTTPException( - status_code=400, detail="That doesn't look like a valid video link." + status_code=422, + detail={"code": "url_blocked", "reason": urlguard.REASON_MALFORMED}, ) from exc host = u.netloc.lower() if "youtube.com" in host: @@ -552,7 +555,12 @@ def _clean_url(raw) -> str | None: return None if not url.startswith(("http://", "https://")): return None - parsed = urlparse(url) + # The third site in this module handing user input to `urlparse`, which raises on a malformed + # authority. This one already has a "not a plausible URL" answer, so the typo belongs in it. + try: + parsed = urlparse(url) + except ValueError: + return None if not parsed.netloc: return None return url[:2048] diff --git a/backend/tests/test_download_limits.py b/backend/tests/test_download_limits.py index 30689251..ce216652 100644 --- a/backend/tests/test_download_limits.py +++ b/backend/tests/test_download_limits.py @@ -251,8 +251,15 @@ def test_a_refusal_for_one_user_does_not_poison_the_shared_asset(db, monkeypatch full = _sized_asset(db, 1500, "already-held") _job(db, over.id, queue_pos=0, status="done", asset_id=full.id) + # `downloading` is the state the worker leaves it in — `_claim_asset` compare-and-sets it before + # the child runs — and `_reset_asset_pending` only acts on that status. Seeded as `pending` this + # assertion would pass with the release deleted. shared = MediaAsset( - source_kind="youtube", source_ref="x" * 11, format_sig="s", status="pending", ref_count=2 + source_kind="youtube", + source_ref="x" * 11, + format_sig="s", + status="downloading", + ref_count=2, ) db.add(shared) db.flush() diff --git a/docs/self-hosting.md b/docs/self-hosting.md index b6b0731d..38693ad4 100644 --- a/docs/self-hosting.md +++ b/docs/self-hosting.md @@ -194,9 +194,10 @@ docker compose -f docker-compose.selfhost.yml exec -T db \ pg_restore -U siftlode -d siftlode --clean --if-exists < backups/siftlode-.dump # Start it again only once the restore reported no errors — and, if you are rolling a release back, -# only AFTER pinning the older IMAGE_TAG. Every start runs `alembic upgrade head`, so starting the -# newer image here would re-apply the migrations you just restored away. -docker compose -f docker-compose.selfhost.yml start api worker +# only AFTER pinning the older IMAGE_TAG. Every start runs `alembic upgrade head`, so bringing the +# newer image back here would re-apply the migrations you just restored away. Use `up -d`, not +# `start`: `start` relaunches the container that already exists, which still carries the old tag. +docker compose -f docker-compose.selfhost.yml up -d ``` On Windows, do **not** pipe the dump through PowerShell — it corrupts binary data. Write it inside the @@ -209,12 +210,17 @@ copy would still run after a failed dump — and copy out the previous run's fil ```powershell & { + # Inside the block, because a scriptblock inherits the caller's preference — under a profile that + # sets SilentlyContinue, every `throw` below would be swallowed and the block would run on. + $ErrorActionPreference = "Stop" $compose = "docker-compose.selfhost.yml" New-Item -ItemType Directory -Force -Path backups | Out-Null $out = "backups\siftlode-$(Get-Date -Format yyyyMMdd-HHmmss).dump" docker compose -f $compose exec -T db pg_dump -U siftlode -Fc siftlode -f /tmp/s.dump $code = $LASTEXITCODE - if ($code -ne 0) { throw "BACKUP FAILED - pg_dump returned $code" } + # Remove the in-container dump on the failure path too: leaving it there is how a LATER failed run + # copies out this run's file and overwrites a good backup with a stale one. + if ($code -ne 0) { docker compose -f $compose exec -T db rm -f /tmp/s.dump; throw "BACKUP FAILED - pg_dump returned $code" } docker compose -f $compose cp db:/tmp/s.dump $out $code = $LASTEXITCODE docker compose -f $compose exec -T db rm -f /tmp/s.dump diff --git a/frontend/src/components/Stats.tsx b/frontend/src/components/Stats.tsx index 6a843697..734a17cd 100644 --- a/frontend/src/components/Stats.tsx +++ b/frontend/src/components/Stats.tsx @@ -7,6 +7,7 @@ import { api, type AdminQuotaRow, type Me } from "../lib/api"; import { formatEta, quotaActionLabel } from "../lib/format"; import { notify } from "../lib/notifications"; import { notifyYouTubeActionError } from "../lib/youtubeErrors"; +import { useWizard } from "./WizardProvider"; import { LS, useAccountPersistedState } from "../lib/storage"; import Tooltip from "./Tooltip"; import { LoadingState, StateMessage } from "./QueryState"; @@ -52,6 +53,8 @@ export default function Stats() { function Overview({ me }: { me: Me }) { const { t } = useTranslation(); const qc = useQueryClient(); + // The 403 case needs somewhere to go, or the toast names a fix the user cannot reach from here. + const { openWizard } = useWizard(); const status = useQuery({ queryKey: qk.myStatus(), queryFn: api.myStatus }); const usage = useQuery({ queryKey: qk.myUsage(), queryFn: api.myUsage }); const s = status.data; @@ -65,11 +68,12 @@ function Overview({ me }: { me: Me }) { message: t("stats.sync.synced", { count: r.subscriptions ?? 0 }), }); }, - // The same helper the Channels page uses, for the same two reasons: it translates the 403 - // ("connect YouTube") instead of surfacing the route's bare English detail under a Hungarian - // UI, and it delegates everything else to notifyActionError so the server's own reason wins — - // which is what makes the SEC-14 cooldown read as a pace limit rather than "Sync failed". - onError: (e) => notifyYouTubeActionError(e, t("stats.sync.syncFailed")), + // The same helper and the same wizard handle as the Channels page: it translates the 403 into + // "connect YouTube" WITH a button that opens the wizard — instead of surfacing the route's bare + // English detail under a Hungarian UI — and delegates everything else to notifyActionError so + // the server's own reason wins, which is what makes the SEC-14 cooldown read as a pace limit + // rather than "Sync failed". + onError: (e) => notifyYouTubeActionError(e, t("stats.sync.syncFailed"), openWizard), }); const deepAll = useMutation({ mutationFn: () => api.deepAll(true), diff --git a/frontend/src/lib/adminUsers.ts b/frontend/src/lib/adminUsers.ts index f8bc82d8..dea4b3ab 100644 --- a/frontend/src/lib/adminUsers.ts +++ b/frontend/src/lib/adminUsers.ts @@ -9,7 +9,7 @@ import { qk } from "./queryKeys"; // Grouped invalidation lives per domain rather than in `qk` (see the contract note there); // `invalidateDownloads` is the same pattern for the downloads domain. -/** Everything that goes stale when an account is LET IN (activate, approve an invite). +/** Everything that goes stale when an account is LET IN (activate, resend its confirmation). * * Suspend and delete move the same counters and are deliberately NOT wired here yet — they have * their own pre-existing staleness, tracked separately, and widening this in passing would put an diff --git a/frontend/src/lib/useVerifyEntry.ts b/frontend/src/lib/useVerifyEntry.ts index 4952cb95..17a00fed 100644 --- a/frontend/src/lib/useVerifyEntry.ts +++ b/frontend/src/lib/useVerifyEntry.ts @@ -68,10 +68,16 @@ export function useVerifyEntry(): VerifyEntry { }, []); useEffect(() => { + // Read the fragment BEFORE clearing it: a verify link pasted over a standing reset notice + // replaces the whole fragment, so the reset token is already gone and the flag describing it + // must go too — otherwise the banner keeps promising a recovery that signing out can no longer + // complete, and (being checked first) hides the confirmation that just arrived. + const stillHasReset = new URLSearchParams(window.location.hash.replace(/^#/, "")).has("reset"); // Clear the address bar even with no token to consume, so a spent one leaves no trace on a // reload. An unspent `#reset=` is the exception and `clearEntryUrl` says why: nothing in a // signed-in tree reads it, so erasing it there is a dead end rather than a cleanup. clearEntryUrl(); + if (!stillHasReset) setResetPending(false); if (!token || consumed.current.has(token)) return; consumed.current.add(token); // A second link arriving over an unread first outcome must not leave the OLD banner standing. diff --git a/scripts/restore.ps1 b/scripts/restore.ps1 index cbf375aa..cc60075f 100644 --- a/scripts/restore.ps1 +++ b/scripts/restore.ps1 @@ -35,11 +35,13 @@ if ($cpCode -ne 0) { throw "RESTORE FAILED — could not copy the dump into the # and the database is left half-dropped. It also keeps the app from writing into a schema that is # only partly back. # -# It is NOT restarted automatically, on purpose. `docker compose start` re-runs the container's -# command, and that runs `alembic upgrade head` — which on the documented rollback path (restore a -# pre-upgrade dump, THEN pin the older IMAGE_TAG) would immediately re-apply the very migrations the -# restore just undid. And on a FAILED restore it would put the app back on a half-restored schema, -# which is the second thing the stop exists to prevent. The operator starts it when they are ready. +# It is NOT restarted automatically, on purpose. Every start runs `alembic upgrade head`, so on the +# documented rollback path (restore a pre-upgrade dump, THEN pin the older IMAGE_TAG) an automatic +# restart would re-apply the very migrations the restore just undid. On a FAILED restore it would +# put the app back on a half-restored schema, which is the second thing the stop exists to prevent. +# The operator starts it when they are ready — with `up -d`, not `start`: `start` relaunches the +# EXISTING container, so it ignores a newly pinned IMAGE_TAG and comes back on the image being +# rolled back FROM. docker compose stop api worker $stopCode = $LASTEXITCODE if ($stopCode -ne 0) { throw "Could not stop api/worker (exit $stopCode) — refusing to restore while the app may still be writing." } @@ -52,8 +54,10 @@ Remove-RemoteDump $remote # reports every missing object it tried to drop), so a non-zero code is reported as "check this", # not swallowed and not dressed up as success. if ($restoreCode -ne 0) { - Write-Output "api and worker are left STOPPED: the database may be half-restored. Fix the dump and re-run before starting them." + # Write-Warning, not Write-Output: this must ride the same stream as the exception below, or a + # `> restore.log` redirection files the explanation away and leaves only the raw error on screen. + Write-Warning "api and worker are left STOPPED: the database may be half-restored. Fix the dump and re-run before starting them." throw "pg_restore reported errors while restoring $DumpFile (exit $restoreCode) — read the output above before treating this instance as restored." } Write-Output "Restored $DumpFile" -Write-Output "api and worker are stopped. Start them with: docker compose start api worker" +Write-Output "api and worker are stopped. Start them with: docker compose up -d (rolling a release back? pin the older IMAGE_TAG in .env FIRST — 'docker compose start' would relaunch the CURRENT image and re-apply its migrations)" diff --git a/scripts/restore.sh b/scripts/restore.sh index aa3b5bc5..5d4611a5 100644 --- a/scripts/restore.sh +++ b/scripts/restore.sh @@ -25,11 +25,13 @@ DB_NAME="${POSTGRES_DB:-siftlode}" # and the database is left half-dropped. It also keeps the app from writing into a schema that is # only partly back. # -# It is NOT restarted automatically, on purpose. `docker compose start` re-runs the container's -# command, and that runs `alembic upgrade head` — which on the documented rollback path (restore a -# pre-upgrade dump, THEN pin the older IMAGE_TAG) would immediately re-apply the very migrations the -# restore just undid. And on a FAILED restore it would put the app back on a half-restored schema, -# which is the second thing the stop exists to prevent. The operator starts it when they are ready. +# It is NOT restarted automatically, on purpose. Every start runs `alembic upgrade head`, so on the +# documented rollback path (restore a pre-upgrade dump, THEN pin the older IMAGE_TAG) an automatic +# restart would re-apply the very migrations the restore just undid. On a FAILED restore it would +# put the app back on a half-restored schema, which is the second thing the stop exists to prevent. +# The operator starts it when they are ready — with `up -d`, not `start`: `start` relaunches the +# EXISTING container, so it ignores a newly pinned IMAGE_TAG and comes back on the image being +# rolled back FROM. if ! docker compose stop api worker; then echo "Could not stop api/worker — refusing to restore while the app may still be writing." >&2 exit 1 @@ -40,7 +42,7 @@ fi # not swallowed and not dressed up as success. if docker compose exec -T db pg_restore -U "$USER_NAME" -d "$DB_NAME" --clean --if-exists < "$FILE"; then echo "Restored $FILE" - echo "api and worker are stopped. Start them with: docker compose start api worker" + echo "api and worker are stopped. Start them with: docker compose up -d (rolling a release back? pin the older IMAGE_TAG in .env FIRST — 'docker compose start' would relaunch the CURRENT image and re-apply its migrations)" else echo "pg_restore reported errors while restoring $FILE — read the output above before treating this instance as restored." >&2 echo "api and worker are left STOPPED: the database may be half-restored. Fix the dump and re-run before starting them." >&2