diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index a025914..c3c3d6c 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -90,6 +90,7 @@ KJ Controller is a web-based karaoke show management application. A Flask backen | `sing_store.py` | ~260 | `SingStore` class: SQLite CRUD for `sing_requests` + `sing_push_subscriptions` + event-token helpers (regenerate / enable / auto-approve) on `rotation_meta` | | `routes.py` | ~1000 | Flask Blueprint with all route handlers (includes `/rotation/requests/*` admin endpoints for the public request form). Hosts the playability gates: tier-1 inline `_playability_gate` (link/upload/download hard-block) + tier-2 async render verification (`_enqueue_tier2` → single-worker queue → `_run_tier2_check` against the active renderer, stamps `playability_warning`) | | `wait_estimate.py` | ~80 | Pure function `compute_estimate(entries, target_id, cfg)` producing `{position, expected_s, range_low_s, range_high_s, spread_source, close_to_front, now_singing}`. Uses tonight's sung-entry variance for the range; falls back to a configurable minimum spread. | +| `sing_resolve.py` | ~120 | Pure decision logic for singer-submission download fallback: `classify_error` (unavailable → advance to next candidate vs transient → retry same) and `next_candidate_index` (bounded by `MAX_CANDIDATES`). No yt-dlp/network/Flask deps → exhaustively unit-tested. | ### Dependency Flow @@ -421,6 +422,15 @@ Additional `static-sing/` assets added in sub-project #4: - iOS Safari requires Add-to-Home-Screen (installed PWA) before push works. The confirmation page detects iOS non-standalone and renders an instructional card explaining the install flow. - Housekeeping: on event-token regeneration, `cleanup_stale_push_subscriptions` deletes subs on other tokens older than 7 days. Keeps the table bounded. +### Singer submission download fallback + +When an approved singer submission's YouTube download fails, the download worker auto-heals instead of surfacing a dead ❌ the KJ must fix by hand (motivated by the 2026-07-09 live incident where a picked "Say My Name" version was a *private video*). + +- **The download attempt is the probe.** `media.download_video` swallows yt-dlp errors and returns `(None, None)`, so it now records the reason on `media._last_error`. On failure the single-threaded `_download_worker` reads it and calls `sing_resolve.classify_error`. +- **Advance vs retry.** `unavailable` (private/removed/blocked) → advance to the next ranked candidate version; `transient` (timeout/429/`bgutil`/network, and any *unknown* error) → retry the same candidate up to `MAX_TRANSIENT_RETRIES`, then advance. Bounded by `MAX_CANDIDATES` (3). Because the worker is sequential, retries re-queue (back of line) rather than sleep-blocking. +- **Candidate list.** `approve_sing_request` attaches a ranked YouTube candidate list to the queue item, built from the `versions[]` snapshot via the existing `_pick_version_from_kj_pick` translator + `_ranked_version_indices`. Since binding a `kj_pick` version rewrites `source_meta`, `_preserve_versions_meta` re-attaches the snapshot at both binding sites so the list survives. v1 falls back across YouTube-type candidates only (cross-source local/Divebar is a documented follow-up). +- **Outcome.** On a successful fallback the request source is rebound (`update_request_source`) so `/my-requests` reflects the version that landed, and the singer gets a `resolved_alt` push. When every candidate is exhausted the entry surfaces the normal terminal ❌ for the KJ plus an `unavailable` push. Non-sing downloads (KJ manual, Divebar) are untouched. + ## VNC Screen Preview The KJ Controller web UI includes a live thumbnail of the Pi's screen via an embedded VNC viewer. This lets the KJ see what's on the HDMI output without a direct line of sight to the display. diff --git a/docs/TROUBLESHOOTING.md b/docs/TROUBLESHOOTING.md index 031196c..eda0615 100644 --- a/docs/TROUBLESHOOTING.md +++ b/docs/TROUBLESHOOTING.md @@ -205,6 +205,26 @@ ssh nomadpc "curl -s -X POST http://127.0.0.1:5001/fix_audio -H 'Content-Type: a Track A auto-recovery (v0.68.0+) auto-restarts the engine and shows the KJ an amber banner if a file still crashes it, so a crash is a ~2s blip rather than a dead show. +## Singer Submission Shows "Unavailable" or Auto-Swapped Version + +A singer submitted a song through the `/sing` UI and it either quietly played a **different +version** than expected, or shows a **red ❌ download-failed** with the singer told "we couldn't +find a playable version." + +- **This is expected auto-fallback behaviour (2026-07-09+).** The version the singer/KJ picked was + an unavailable YouTube video (private, deleted, region-blocked). Rather than dead-ending, the + download worker automatically tries the next-best candidate version of the same song and rebinds + the request to whichever one downloads. +- **Auto-swap (no action needed):** the entry ends up linked to a working version and `/my-requests` + shows it. If the swapped version is a poor match, use the rotation 🔗 link / "Try Another" button + to pick a different one manually — same as before. +- **Terminal ❌ (KJ action):** every candidate was unavailable (or there were no alternates — e.g. a + single-version song, or a raw pasted URL). Link a working file manually via the rotation entry. +- **Everything shows unavailable / nothing downloads:** that's a different problem — check YouTube + health (cookies, yt-dlp version) and whether the `bgutil` PO-token helper at `127.0.0.1:4416` is + reachable. Persistent transient errors (timeouts/429) exhaust the bounded retries and then surface + as terminal ❌. `journalctl -u kj-controller` shows `Sing fallback:` lines tracing each decision. + ## Docker Containers Not Running ```bash diff --git a/docs/archive/2026-07-09-singer-submission-validation-plan.md b/docs/archive/2026-07-09-singer-submission-validation-plan.md new file mode 100644 index 0000000..149ae8a --- /dev/null +++ b/docs/archive/2026-07-09-singer-submission-validation-plan.md @@ -0,0 +1,163 @@ +# Plan: Auto-resolving singer submissions (download validation + fallback) + +**Created:** 2026-07-09 +**Branch:** feat/sess-20260717-0153-singer-submission-validation +**Status:** Implemented (pending review/merge) +**Design spec:** [2026-07-09-singer-submission-validation-design.md](./2026-07-09-singer-submission-validation-design.md) + +## As-Built Deviations + +Discovered during implementation (all reflected in the code + tests): + +- **D1 — failure reason via `media._last_error`, not a caught exception.** `download_video` + swallows yt-dlp errors and returns `(None, None)`; it now records the reason on + `media._last_error`, which the single (serialized) download worker reads. No second network probe. +- **D2 — transient exhaustion advances, not terminates.** Transient retries per candidate then fall + through to the next candidate; only an exhausted candidate list is terminal. Unknown errors + default to transient. +- **D3 — no new sing-request status.** Terminal = the existing rotation `failed` download_status + + an `unavailable` push; success rebinds via the existing `update_request_source`. +- **D4 — pivot from the planned client change.** Direct `youtube`/`kn` picks are only ever + single-version (no alternates), so attaching `versions[]` client-side adds nothing. The real fix + is `_preserve_versions_meta`: keep the `versions[]` snapshot through `kj_pick` binding (both the + admin approve route and `resolve_kj_pick_best`) so multi-version songs — the incident case — have + candidates. `sing.js` was **not** changed. +- **D5 — v1 scope: YouTube-type fallback only.** Cross-source (local/Divebar) fallback is a + documented follow-up. +- Open questions resolved: push uses the existing `notify_request_decision` (+ two new copy steps); + no `unavailable` status enum needed; caps kept at 3 candidates / 2 transient retries. + +## Overview + +When a singer's picked YouTube version can't be downloaded (private/deleted video, as in the +2026-07-09 live incident), the download worker should automatically fall back to the next-best +candidate version of the same song, notifying the singer only when nothing is playable. Async, no +false rejections on transient network blips, zero added latency on the happy path. + +## Requirements + +- [ ] On a download failure classified as **unavailable**, auto-advance to the next candidate + version and retry, without KJ intervention. +- [ ] On a failure classified as **transient** (timeout / 429 / `bgutil` / network), retry the + **same** candidate — do not consume the candidate list. +- [ ] Cap total resolution effort: ≤ 3 distinct candidates tried; bounded transient retries. +- [ ] When a fallback succeeds, rebind the request's source so `/my-requests` reflects the version + actually used; when all candidates fail, mark the request terminally "unavailable" and flag + the rotation entry for the KJ. +- [ ] Singer is notified on terminal states only (version-changed / no-version), via existing + `/my-requests` polling (baseline) and Web Push (enhancement). +- [ ] Single direct `youtube`/`kn` picks carry the group's ranked `versions[]` so they have + fallback candidates too. +- [ ] Non-functional: no submit-path latency added; worker stays single-threaded and never + sleep-blocks the queue; no infinite re-queue loops. + +## Technical Approach + +**Reuse over new machinery.** The "candidate list" is the existing `versions[]` snapshot already +stored in `source_meta` (today only for `kj_pick`). Fallback = advance an index over that list and +reuse `_pick_version_from_kj_pick`'s translation of `versions[i] → (source_type, source_ref, +source_meta)`. + +**Fallback lives in the download worker's existing error branch** (`routes.py:713–739`). Today that +branch discards the exception and marks the item `error`. We change it to: +1. Capture the exception message. +2. For **sing-request-backed** items only (identified by a new `request_id` + `candidates` on the + queue item), consult the resolver. +3. `unavailable` + candidates remain under cap → translate next candidate, `update_request_source`, + reset the item to `queued` (worker re-picks it), continue. +4. `transient` under retry cap → bump an attempt counter, reset to `queued` (back of line — no + `sleep`, since the worker is sequential), continue. +5. Exhausted / terminal → mark request `unavailable`, flag rotation entry, fire one Web Push. + +Non-sing downloads (KJ manual, divebar) keep today's behaviour untouched. + +**Error classification** is isolated in a pure, unit-tested module so the risky part (deciding +unavailable vs transient) is testable without yt-dlp or the network. Unknown errors default to +`transient` (safer: retry same candidate rather than wrongly discarding a good one). + +### Trade-offs considered + +- *Separate metadata probe vs download-as-probe* → download-as-probe (per spec): no happy-path + latency, reuses the download path. +- *Transient retry inline (`sleep`) vs re-queue* → re-queue with attempt counter, because the + single worker thread must not block other singers' downloads. +- *New candidate schema vs reuse `versions[]`* → reuse; `_pick_version_from_kj_pick` already + translates it and the client already produces it. + +## Implementation Steps + +1. [ ] **`sing_resolve.py` (new, pure).** + - `classify_error(message: str) -> "unavailable" | "transient"` with pattern tables + (unavailable: `Private video`, `Video unavailable`, `has been removed`, `blocked in your + country`, account-terminated; transient: timeouts, `HTTP Error 429/5xx`, `bgutil`, connection + reset). Unknown → `transient`. + - `next_candidate_index(total, tried) -> int | None` and a `MAX_CANDIDATES = 3`, + `MAX_TRANSIENT_RETRIES = 2` constants. +2. [ ] **Unit tests `tests/test_sing_resolve.py`** — real yt-dlp strings incl. the incident's + `ERROR: [youtube] _vMTtVPhd80: Private video`; ordering/cap/exhaustion; unknown→transient. +3. [ ] **Client `static-sing/sing.js`** — attach the group's ranked `versions[]` in `source_meta` + for direct `youtube`/`kn` submissions (mirror the existing `kj_pick` line ~658). Preserves + existing single-pick behaviour otherwise. +4. [ ] **`approve_sing_request` (routes.py ~4844 queue_item build)** — for `youtube`/`kn`, add to + the queue item: `request_id`, `candidates` (the `versions[]` list, may be empty), `current_index` + (0), `tried` ([]), `transient_attempts` (0). No behaviour change when `candidates` is empty + beyond richer terminal messaging. +5. [ ] **`_download_worker` error branch (routes.py:713–739)** — capture `exc`; extract + `_attempt_sing_fallback(app, item, str(exc))` helper that implements steps 3–5 of the approach. + Keep non-sing items on the current path. +6. [ ] **`sing_store.py`** — add a terminal status transition (e.g. `mark_unavailable(request_id, + reason)`) and confirm `update_request_source` is used for successful rebinds. Ensure the status + enum/index covers `unavailable`. +7. [ ] **Singer-facing surface** — extend `_public_request_view` (sing.py) to expose the resolution + state + the version label actually used, so `/my-requests` renders "finding…/locked in + (label)/unavailable" without a new endpoint. +8. [ ] **Web Push (enhancement) `push_dispatcher.py`** — add a targeted per-request send for the two + terminal states. *Open question:* confirm the dispatcher exposes a per-singer/subscription send + (vs only rotation-mutation broadcasts). If not trivially available, ship steps 1–7 first + (polling covers the UX) and add push as a follow-up. +9. [ ] **Integration tests** — worker + stub downloader: (a) c0 unavailable→c1 ok (rebind + one + push), (b) all unavailable (terminal + KJ flag + one push), (c) transient→same candidate retried, + list not consumed, (d) fallback candidate already on disk → linked via `_existing_media_for`, + no re-download. +10. [ ] **Docs** — `docs/ARCHITECTURE.md` (new module + fallback flow), `docs/CHANGELOG.md` (dated + entry), `docs/TROUBLESHOOTING.md` (what "unavailable — KJ notified" means). + +## Files to Create/Modify + +| File | Action | Description | +|------|--------|-------------| +| `kj-controller/sing_resolve.py` | Create | Pure error classifier + candidate iteration + caps | +| `kj-controller/tests/test_sing_resolve.py` | Create | Unit tests for the resolver | +| `kj-controller/static-sing/sing.js` | Modify | Attach `versions[]` for direct youtube/kn picks | +| `kj-controller/routes.py` | Modify | Queue-item fields; `_attempt_sing_fallback`; worker error branch | +| `kj-controller/sing_store.py` | Modify | `mark_unavailable` transition; status enum coverage | +| `kj-controller/sing.py` | Modify | Expose resolution state/label in `_public_request_view` | +| `kj-controller/push_dispatcher.py` | Modify | Targeted terminal-state push (enhancement) | +| `kj-controller/tests/test_sing_fallback*.py` | Create | Integration tests for worker fallback | +| `docs/ARCHITECTURE.md`, `docs/CHANGELOG.md`, `docs/TROUBLESHOOTING.md` | Modify | Document module + flow | + +## Testing Strategy + +- **Unit:** `classify_error` (incl. incident string, unknown→transient), candidate iteration/caps. +- **Integration:** worker-with-stub scenarios (a)–(d) above; assert request status, source rebind, + push count, and no-double-download. +- **Manual (pre-merge, local):** run kj-controller locally, submit a request pointing at a known + private video with a working alternate in `versions[]`; confirm auto-fallback, `/my-requests` + copy, and KJ rotation flag. `cd kj-controller && pytest --cov`. + +## Open Questions + +- [ ] Does `push_dispatcher` expose a per-request/subscription send, or only rotation-mutation + broadcasts? Determines whether step 8 lands in this PR or as a fast follow. +- [ ] Confirm the sing_requests `status` enum should gain `unavailable` (vs reusing `rejected` with + a reason). Prefer a distinct `unavailable` for singer-facing copy. +- [ ] Candidate cap (3) and transient-retry cap (2) — tune during integration if needed. + +## Rollback Plan + +- Pure code addition behind the existing worker path; nothing changes for non-sing downloads. +- If fallback misbehaves in production, revert the `routes.py` worker-branch change (and the + `sing.js` `versions[]` attachment) — the system returns to today's "single attempt, red ❌, + manual KJ fix" behaviour with no data migration to undo (new queue-item fields are ignored). +- This is a **backend** change → requires `systemctl restart kj-controller`; deploy only in a + maintenance window, never mid-show. diff --git a/kj-controller/docs/CHANGELOG.md b/kj-controller/docs/CHANGELOG.md index 97b5691..a0b5fb7 100644 --- a/kj-controller/docs/CHANGELOG.md +++ b/kj-controller/docs/CHANGELOG.md @@ -4,6 +4,15 @@ Dated entries, newest first. Each entry notes any required deploy steps. --- +## 2026-07-17 - Auto-fallback for failed singer-submission downloads (v0.89.0) + +**Deploy:** backend change (`routes.py`, `media.py`, `push_dispatcher.py`, new `sing_resolve.py`) → **requires `systemctl restart kj-controller`** (interrupts playback — deploy between songs). No DB migration; new queue-item fields are in-memory only and ignored on rollback. + +- When an approved singer submission's YouTube download fails because the video is **unavailable** (private/deleted/blocked — the 2026-07-09 "Say My Name" private-video incident), the download worker now automatically advances to the next-best candidate version of the same song instead of dead-ending at a red ❌ the KJ had to fix by hand. Only a request with **no** playable candidate left surfaces the terminal failure (and now pushes the singer an honest "we couldn't find a playable version — your KJ has been notified"). +- **No false rejections.** Transient failures (timeouts, HTTP 429, the `bgutil` PO-token helper being down, network blips — and any unrecognised error) retry the *same* candidate a bounded number of times before advancing, so a flaky network never discards a good video. Classification lives in a pure, exhaustively unit-tested `sing_resolve` module. +- On a successful fallback the request is rebound to the version that actually downloaded (so `/my-requests` shows the right one) and the singer gets a "we queued an alternate version" push. +- Mechanics: `media.download_video` records its failure reason on `media._last_error`; the single-threaded `_download_worker` classifies it and walks a ranked candidate list attached to the queue item (bounded to 3 candidates). The `versions[]` snapshot is preserved through `kj_pick` binding so multi-version songs — the common case — have alternates to try. YouTube-type fallback only in v1; cross-source (local/Divebar) fallback is a follow-up. + ## 2026-07-17 - Disable all mpv audio processing (pitch + vocals guide) by default (v0.87.0) **Deploy:** backend change (`mpv_manager.py`, `routes.py`, `config.py`) → **requires `systemctl restart kj-controller`** (interrupts playback — deploy between songs). No DB migration. Frontend hides the affected controls automatically on the next status/renderer poll (no cache-bust needed beyond the version bump). diff --git a/kj-controller/media.py b/kj-controller/media.py index a0efe02..61ee18c 100644 --- a/kj-controller/media.py +++ b/kj-controller/media.py @@ -146,6 +146,10 @@ def __init__(self, config, media_library=None): self.index = {} self.media_library = media_library self.gen_client = None # attribute-injected by app.py after gen_client built + # Last download_video failure reason, for the sing-request fallback path + # (download_video swallows yt-dlp errors and returns None; the single + # download worker reads this to classify unavailable-vs-transient). + self._last_error = None def _finalize_download_identity(self, tmp_path, *, source, source_ref, artist_hint, title_hint, channel, @@ -571,6 +575,8 @@ def download_video(self, youtube_url): """Downloads a YouTube video with descriptive filename, updates media index.""" import yt_dlp + self._last_error = None # reset; set on each failure path for the fallback worker + download_folder = self.config.get('download_folder', os.path.expanduser("~/kjdata/videos")) os.makedirs(download_folder, exist_ok=True) @@ -587,6 +593,7 @@ def download_video(self, youtube_url): upload_date = info.get('upload_date') except Exception as e: log_message(f"Error extracting video info: {e}", self.config) + self._last_error = str(e) return None, None # Phase 2: Build descriptive filename and download @@ -618,6 +625,7 @@ def download_video(self, youtube_url): if not file_path: log_message(f"ERROR: Downloaded file not found for {basename}", self.config) + self._last_error = "downloaded file not found" return None, None gate = _gate_playable(file_path, self.config) @@ -630,6 +638,7 @@ def download_video(self, youtube_url): qpath = _quarantine_download(file_path, reason, self.config) if qpath: log_message(f"Quarantined to {qpath}", self.config) + self._last_error = f"not playable: {reason}" return None, None # Canonicalise identity: move into downloads/youtube/ with an @@ -679,6 +688,7 @@ def download_video(self, youtube_url): return real_dest, title except Exception as e: log_message(f"Error downloading video: {e}", self.config) + self._last_error = str(e) return None, None def _run_ytdlp_download(self, ydl_opts, url): diff --git a/kj-controller/push_dispatcher.py b/kj-controller/push_dispatcher.py index aa227b3..c2d6962 100644 --- a/kj-controller/push_dispatcher.py +++ b/kj-controller/push_dispatcher.py @@ -70,6 +70,9 @@ def next_entry_for_phone(entries, phone, get_linked_phone): "up_in_2": ("You're up in 2! 🎤", "{song} — head back to the venue."), "up_next": ("You're up NEXT 🎤", "{song} — stand by the mic."), "now_singing": ("🎤 You're singing now", "{song} — you're up!"), + # Auto-fallback outcomes for singer submissions. + "resolved_alt": ("You're all set 🎶", "We queued an alternate version of {song}."), + "unavailable": ("The KJ needs a word", "We couldn't find a playable video for {song} — your KJ has been notified."), } @@ -151,7 +154,9 @@ def notify_request_decision(self, request_id, decision, request_dict): subs = self.store.find_subs_by_phone(token, phone) if not subs: return - step = "approved" if decision == "approved" else "rejected" + # approve/reject plus the auto-fallback outcomes carry their own copy; + # anything unrecognised falls back to the neutral "rejected" template. + step = decision if decision in _PAYLOADS else "rejected" song = f"{request_dict.get('song_title') or ''} — {request_dict.get('song_artist') or ''}".strip(" —") fake_entry = {"id": request_id, "song_artist": song or None} payload = render_payload(step, fake_entry, request_id=request_id) diff --git a/kj-controller/pyproject.toml b/kj-controller/pyproject.toml index e58f237..e47c598 100644 --- a/kj-controller/pyproject.toml +++ b/kj-controller/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "kj-controller" -version = "0.88.0" +version = "0.89.0" description = "Web-based karaoke show management with mpv + VLC playback" requires-python = ">=3.11" diff --git a/kj-controller/routes.py b/kj-controller/routes.py index 8038f40..1036b7f 100644 --- a/kj-controller/routes.py +++ b/kj-controller/routes.py @@ -23,6 +23,7 @@ import text_normalize from text_normalize import normalize as _normalize_text, tokens as _tokens, group_key as _group_key import version_priority +import sing_resolve import youtube_health import youtube_search from config import APP_DIR, RENDER_MODE_MPV, RENDER_MODES, load_config, save_config_value @@ -713,6 +714,14 @@ def _download_worker(app): except Exception: file_path, title = None, None + # Sing-request fallback: a failed download for a singer's pick should + # auto-advance to the next candidate version (or retry on a transient + # blip) rather than dead-ending at a red ❌ the KJ must fix by hand. + if (not file_path and next_item.get('request_id') + and next_item.get('candidates')): + if _attempt_sing_fallback(app, next_item): + continue # re-queued (retry or next candidate); worker re-picks it + with app._download_lock: if file_path: next_item.update(status='completed', title=title, @@ -735,8 +744,14 @@ def _download_worker(app): ) except Exception: pass # Best-effort; entry can be linked manually + # Fell back to an alternate version → tell the singer which one landed. + if next_item.get('request_id') and next_item.get('candidate_index'): + _notify_sing_outcome(app, next_item, 'resolved_alt') else: _sync_rotation_download(app, next_item) # status='error' → entry 'failed' + # Exhausted every candidate → let the singer know the KJ was flagged. + if next_item.get('request_id'): + _notify_sing_outcome(app, next_item, 'unavailable') @routes_bp.route('/download/cancel', methods=['POST']) @@ -4947,6 +4962,27 @@ def _pick_version_from_kj_pick(req, index): raise ValueError(f"unknown version source: {src!r}") +def _preserve_versions_meta(req, src_meta): + """Merge the kj_pick ``versions`` snapshot into a freshly-bound source_meta. + + Binding a kj_pick request rewrites ``source_meta`` to the picked version's + meta, which would drop the ``versions[]`` snapshot the download worker needs + to auto-fall-back if that version turns out to be an unavailable video. + Re-attach it (no-op when the request carried no snapshot). + """ + meta_raw = req.get("source_meta") + try: + meta = meta_raw if isinstance(meta_raw, dict) else json.loads(meta_raw or "{}") + except (TypeError, ValueError): + return src_meta + versions = meta.get("versions") + if not versions: + return src_meta + merged = dict(src_meta or {}) + merged["versions"] = versions + return merged + + def _ranked_version_indices(versions, cfg): """Return version indices ordered best-first by priority_rank. @@ -4997,6 +5033,7 @@ def resolve_kj_pick_best(app, req, cfg): except ValueError as exc: last_err = exc # malformed version — try the next-best one continue + src_meta = _preserve_versions_meta(req, src_meta) return store.update_request_source(req["id"], src_type, src_ref, src_meta) raise ValueError(f"no resolvable version in kj_pick snapshot: {last_err}") @@ -5031,6 +5068,115 @@ def apply_reorder_request(app, req): rotation.move_entry(eid, target_positions.pop(0)) +def _build_sing_fallback_candidates(req, current_url, current_meta, cfg): + """Ranked YouTube fallback candidates for a sing request — current attempt first. + + Reads the ``versions`` snapshot the client rides along in ``source_meta``, + ranks it best-first (the same order the admin picker uses), and keeps the + distinct downloadable YouTube URLs. v1 falls back across YouTube-type + versions only; cross-source (local/divebar) fallback is a documented + follow-up (see the design doc). Always returns at least the current attempt. + """ + # Dedup on the canonical video id, not the raw URL: youtu.be/X and + # youtube.com/watch?v=X are the same (dead) video and must not each burn a + # candidate slot. Fall back to the raw URL when no id is derivable. + def _key(url): + return youtube_id_from_url(url) or url + + candidates = [{ + "url": current_url, + "source_type": "youtube", + "source_meta": current_meta, + }] + seen = {_key(current_url)} + meta_raw = req.get("source_meta") + try: + meta = meta_raw if isinstance(meta_raw, dict) else json.loads(meta_raw or "{}") + except (TypeError, ValueError): + return candidates + versions = meta.get("versions") or [] + for idx in _ranked_version_indices(versions, cfg): + try: + src_type, src_ref, src_meta = _pick_version_from_kj_pick(req, idx) + except ValueError: + continue # malformed version snapshot entry — skip it + if src_type != "youtube" or not src_ref or _key(src_ref) in seen: + continue + candidates.append({ + "url": src_ref, "source_type": "youtube", "source_meta": src_meta, + }) + seen.add(_key(src_ref)) + return candidates + + +def _attempt_sing_fallback(app, item): + """On a failed sing-request download, retry (transient) or advance (unavailable). + + Returns ``True`` when the item was re-queued — either the same candidate + (transient blip, bounded retries) or the next candidate (definitively + unavailable) — so the worker should ``continue``. Returns ``False`` when + resolution is exhausted and the entry should surface as a terminal ❌ for + the KJ. See ``sing_resolve`` for the classification/cap logic. + """ + reason = getattr(app.media, "_last_error", None) or "" + kind = sing_resolve.classify_error(reason) + candidates = item.get("candidates") or [] + idx = item.get("candidate_index", 0) + + # Transient blip → retry the SAME candidate a bounded number of times. The + # worker is single-threaded, so we re-queue (back of line) rather than sleep. + if kind == sing_resolve.TRANSIENT: + attempts = item.get("transient_attempts", 0) + if attempts < sing_resolve.MAX_TRANSIENT_RETRIES: + with app._download_lock: + item["transient_attempts"] = attempts + 1 + item["status"] = "queued" + log_message( + f"Sing fallback: transient error on candidate {idx}, retry " + f"{attempts + 1}/{sing_resolve.MAX_TRANSIENT_RETRIES} — {reason}", + app.kj_config) + return True + # retries exhausted → fall through and advance to the next candidate + + tried = list(range(idx + 1)) + nxt = sing_resolve.next_candidate_index(len(candidates), tried) + if nxt is None: + log_message( + f"Sing fallback: no playable candidate left for request " + f"{item.get('request_id')} — {reason}", app.kj_config) + return False + cand = candidates[nxt] + with app._download_lock: + item["url"] = cand["url"] + item["candidate_index"] = nxt + item["transient_attempts"] = 0 + item["status"] = "queued" + item["error"] = None + try: + app.sing_store.update_request_source( + item["request_id"], cand.get("source_type", "youtube"), + cand["url"], cand.get("source_meta")) + except Exception: + app.logger.exception("Sing fallback: update_request_source failed") + log_message( + f"Sing fallback: candidate {idx} unavailable, advancing to candidate " + f"{nxt} for request {item.get('request_id')} — {reason}", app.kj_config) + return True + + +def _notify_sing_outcome(app, item, decision): + """Best-effort Web Push to the singer about a terminal fallback outcome.""" + dispatcher = getattr(app, "push_dispatcher", None) + if dispatcher is None: + return + try: + req = app.sing_store.get_request(item["request_id"]) + if req: + dispatcher.notify_request_decision(item["request_id"], decision, req) + except Exception: + app.logger.exception("Sing fallback: notify failed") + + def approve_sing_request(app, req, skip_download=False): """Dispatch approval of a sing request; create/link a rotation entry. @@ -5144,6 +5290,14 @@ def approve_sing_request(app, req, skip_download=False): "status": "queued", "error": None, "rotation_entry_id": entry["id"], + # Auto-fallback: if this YouTube download turns out to be an + # unavailable video, the worker advances through these ranked + # candidates instead of dead-ending at ❌. + "request_id": req["id"], + "candidates": _build_sing_fallback_candidates( + req, queue_url, None, app.kj_config), + "candidate_index": 0, + "transient_attempts": 0, } rotation.set_download_status( entry["id"], queue_src, "queued", download_id @@ -5418,6 +5572,7 @@ def approve_sing_request_route(req_id): ) except ValueError as exc: return jsonify({"error": str(exc)}), 400 + src_meta = _preserve_versions_meta(req, src_meta) req = store.update_request_source(req_id, src_type, src_ref, src_meta) try: diff --git a/kj-controller/sing_resolve.py b/kj-controller/sing_resolve.py new file mode 100644 index 0000000..dc3a211 --- /dev/null +++ b/kj-controller/sing_resolve.py @@ -0,0 +1,108 @@ +"""Pure decision logic for auto-resolving singer submissions. + +When a singer's picked YouTube version fails to download, the download worker +consults this module to decide what to do next: + +- ``classify_error`` — was the failure because the video is *gone* + (private/deleted/blocked → try a different candidate) or a *transient* blip + (timeout/429/network → retry the same candidate)? +- ``next_candidate_index`` — which version to try next, bounded by + ``MAX_CANDIDATES`` so resolution stays fast during a live show. + +Deliberately dependency-free (no yt-dlp, no network, no Flask) so the risky +classification logic is exhaustively unit-testable. See +``docs/archive/2026-07-09-singer-submission-validation-design.md``. +""" + +# Try at most this many *distinct* candidate versions before giving up and +# flagging the entry for the KJ. Keeps auto-resolution bounded during a show. +MAX_CANDIDATES = 3 + +# Per-candidate transient retries before advancing to the next candidate. A +# transient error never terminates resolution on its own — it retries this many +# times, then falls through to the next candidate. +MAX_TRANSIENT_RETRIES = 2 + +# Classification outcomes. +UNAVAILABLE = "unavailable" # video is gone → advance to the next candidate +TRANSIENT = "transient" # network/service blip → retry the same candidate + + +# Substrings (matched case-insensitively) that mean the video itself cannot be +# fetched by anyone with our credentials — retrying is pointless, move on. +_UNAVAILABLE_PATTERNS = ( + "private video", + "video is private", + "video unavailable", + "video is unavailable", + "has been removed", + "removed by the uploader", + "removed for violating", + "no longer available", + "account associated with this video has been terminated", + "account has been terminated", + "not made this video available in your country", + "blocked it in your country", + "not available in your country", + "members-only", + "join this channel", + "confirm your age", + "video does not exist", +) + +# Substrings that mean "try again" — the request never reached a verdict about +# the video, so the same candidate deserves another shot. +_TRANSIENT_PATTERNS = ( + "timed out", + "timeout", + "temporarily", + "try again later", + "http error 5", # 500 / 502 / 503 ... + "429", + "too many requests", + "bgutil", + "pot provider", + "getaddrinfo", + "network is unreachable", + "connection reset", + "connection refused", + "connection aborted", + "eof occurred", + "unable to download webpage", + "failed to resolve", + "ssl", +) + + +def classify_error(message): + """Classify a download failure ``message`` as ``UNAVAILABLE`` or ``TRANSIENT``. + + Unknown or empty messages default to ``TRANSIENT``: retrying a still-good + candidate a couple of times is cheap, whereas wrongly discarding it on a + fluke loses the singer's song. ``UNAVAILABLE`` patterns are checked first + since they are the more specific signal. + """ + text = (message or "").lower() + for pattern in _UNAVAILABLE_PATTERNS: + if pattern in text: + return UNAVAILABLE + for pattern in _TRANSIENT_PATTERNS: + if pattern in text: + return TRANSIENT + return TRANSIENT + + +def next_candidate_index(total, tried): + """Return the next untried candidate index, or ``None`` when resolution stops. + + Stops (returns ``None``) when every candidate has been tried, when there are + no candidates, or when the ``MAX_CANDIDATES`` distinct-attempt cap is hit. + ``tried`` is the collection of indices already attempted. + """ + tried_set = set(tried) + if len(tried_set) >= MAX_CANDIDATES: + return None + for i in range(total): + if i not in tried_set: + return i + return None diff --git a/kj-controller/tests/integration/test_sing_fallback.py b/kj-controller/tests/integration/test_sing_fallback.py new file mode 100644 index 0000000..b957eea --- /dev/null +++ b/kj-controller/tests/integration/test_sing_fallback.py @@ -0,0 +1,210 @@ +"""Integration tests for auto-fallback in the download worker. + +Drives ``_download_worker`` directly with a crafted sing-request-backed queue +item and a mocked ``download_video`` that fails on chosen URLs (setting +``media._last_error`` the way the real one does). Covers the three behaviours +that keep singers from mismanaged expectations: advance on unavailable, retry +on transient, and terminal ❌ only when nothing is playable. +""" + +import time + +import routes +import sing_resolve +from routes import _download_worker, approve_sing_request + + +def _make_request(app, source_ref, versions=None): + return app.sing_store.create_request( + singer_name="Tester", phone="", song_artist="Beetlejuice", + song_title="Say My Name", source_type="youtube", + source_ref=source_ref, source_meta={"versions": versions or []}, + ) + + +def _queue_item(req, candidates, url): + return { + "id": "dl1", "url": url, "title": "Say My Name", "source": "youtube", + "source_detail": None, "status": "queued", "error": None, + "rotation_entry_id": None, # keep rotation out of the fallback assertions + "request_id": req["id"], "candidates": candidates, + "candidate_index": 0, "transient_attempts": 0, + } + + +def test_fallback_advances_to_next_candidate_on_unavailable(flask_app, mocker): + """A private-video first pick auto-resolves to the next working candidate.""" + app = flask_app + req = _make_request(app, "https://youtu.be/DEAD") + candidates = [ + {"url": "https://youtu.be/DEAD", "source_type": "youtube", "source_meta": None}, + {"url": "https://youtu.be/GOOD", "source_type": "youtube", + "source_meta": {"brand_code": "KV"}}, + ] + + def fake_dl(url): + if "DEAD" in url: + app.media._last_error = "ERROR: [youtube] x: Private video" + return (None, None) + app.media._last_error = None + return ("/videos/good.mp4", "Good KV") + + mocker.patch.object(app.media, "download_video", side_effect=fake_dl) + + item = _queue_item(req, candidates, "https://youtu.be/DEAD") + app.download_queue["items"] = [item] + _download_worker(app) + + assert item["status"] == "completed" + assert item["candidate_index"] == 1 + assert item["url"] == "https://youtu.be/GOOD" + # The request is rebound so /my-requests reflects the version that landed. + updated = app.sing_store.get_request(req["id"]) + assert updated["source_ref"] == "https://youtu.be/GOOD" + + +def test_fallback_terminal_when_all_candidates_unavailable(flask_app, mocker): + """When every candidate is a dead video, the item ends in a terminal error.""" + app = flask_app + req = _make_request(app, "https://youtu.be/DEAD1") + candidates = [ + {"url": "https://youtu.be/DEAD1", "source_type": "youtube", "source_meta": None}, + {"url": "https://youtu.be/DEAD2", "source_type": "youtube", "source_meta": None}, + ] + + def fake_dl(url): + app.media._last_error = "Video unavailable. This video has been removed" + return (None, None) + + mocker.patch.object(app.media, "download_video", side_effect=fake_dl) + + item = _queue_item(req, candidates, "https://youtu.be/DEAD1") + app.download_queue["items"] = [item] + _download_worker(app) + + assert item["status"] == "error" + # Advanced through both candidates before giving up. + assert item["candidate_index"] == 1 + + +def test_transient_error_retries_same_candidate(flask_app, mocker): + """A transient blip retries the SAME candidate and never consumes the list.""" + app = flask_app + req = _make_request(app, "https://youtu.be/ONLY") + candidates = [ + {"url": "https://youtu.be/ONLY", "source_type": "youtube", "source_meta": None}, + ] + calls = [] + + def fake_dl(url): + calls.append(url) + app.media._last_error = "ERROR: Unable to download webpage: read operation timed out" + return (None, None) + + mocker.patch.object(app.media, "download_video", side_effect=fake_dl) + + item = _queue_item(req, candidates, "https://youtu.be/ONLY") + app.download_queue["items"] = [item] + _download_worker(app) + + # Same URL retried MAX_TRANSIENT_RETRIES times, then a final terminal attempt. + assert calls == ["https://youtu.be/ONLY"] * (sing_resolve.MAX_TRANSIENT_RETRIES + 1) + assert item["candidate_index"] == 0 # never advanced — only one candidate + assert item["status"] == "error" + + +def test_build_candidates_extracts_and_dedups_youtube(flask_app): + """Candidate builder ranks YT versions, keeps current first, dedups, skips non-YT.""" + app = flask_app + versions = [ + {"source": "kn", "kn": {"youtube_url": "https://youtu.be/A", "brand_code": "KV"}}, + {"source": "kn", "kn": {"youtube_url": "https://youtu.be/DEAD", "brand_code": "X"}}, + {"source": "local", "local": {"path": "/media/song.mp4"}}, + ] + req = {"id": 1, "source_meta": {"versions": versions}} + + cands = routes._build_sing_fallback_candidates( + req, "https://youtu.be/DEAD", None, app.kj_config) + urls = [c["url"] for c in cands] + + assert urls[0] == "https://youtu.be/DEAD" # current attempt first + assert "https://youtu.be/A" in urls # other YT candidate included + assert "/media/song.mp4" not in urls # local skipped (YT-only in v1) + assert urls.count("https://youtu.be/DEAD") == 1 # deduped against current + + +def test_build_candidates_dedups_across_url_forms(flask_app): + """The same video in youtu.be vs watch?v= form must not both become candidates.""" + app = flask_app + versions = [ + # Same video id as the current pick, different URL form → must dedup out. + {"source": "kn", "kn": {"youtube_url": "https://youtu.be/DEADvideo11"}}, + {"source": "kn", "kn": {"youtube_url": "https://www.youtube.com/watch?v=GOODvideo22"}}, + ] + req = {"id": 1, "source_meta": {"versions": versions}} + + cands = routes._build_sing_fallback_candidates( + req, "https://www.youtube.com/watch?v=DEADvideo11", None, app.kj_config) + urls = [c["url"] for c in cands] + + assert urls[0] == "https://www.youtube.com/watch?v=DEADvideo11" # current first + # The youtu.be form of the same video is deduped away; only the good alt remains. + assert not any("DEADvideo11" in u for u in urls[1:]) + assert "https://www.youtube.com/watch?v=GOODvideo22" in urls + + +def test_preserve_versions_meta_keeps_snapshot(): + """Binding a kj_pick version must not drop the versions snapshot.""" + versions = [{"source": "kn", "kn": {"youtube_url": "https://youtu.be/A"}}] + req = {"source_meta": {"versions": versions}} + + merged = routes._preserve_versions_meta(req, {"brand_code": "KV"}) + assert merged["brand_code"] == "KV" + assert merged["versions"] == versions + + # No snapshot present → source_meta returned unchanged (including None). + assert routes._preserve_versions_meta({"source_meta": {}}, {"x": 1}) == {"x": 1} + assert routes._preserve_versions_meta({"source_meta": None}, None) is None + + +def test_incident_reproduction_approve_then_autofallback(flask_app, mocker): + """End-to-end: a multi-version approval whose best pick is private auto-heals. + + Mirrors the 2026-07-09 live incident — first YouTube version is a private + video, a KaraFun alternate exists — and asserts the singer ends up linked to + the working version with no manual KJ intervention. + """ + app = flask_app + versions = [ + {"source": "kn", "kn": {"youtube_url": "https://youtu.be/DEAD", "brand_code": "KV"}}, + {"source": "kn", "kn": {"youtube_url": "https://youtu.be/GOOD", "brand_code": "KV"}}, + ] + req = app.sing_store.create_request( + singer_name="Lulu", phone="", song_artist="Beetlejuice", + song_title="Say My Name", source_type="youtube", + source_ref="https://youtu.be/DEAD", source_meta={"versions": versions}, + ) + + def fake_dl(url): + if "DEAD" in url: + app.media._last_error = "ERROR: [youtube] x: Private video" + return (None, None) + app.media._last_error = None + return ("/videos/good.mp4", "Good KV") + + mocker.patch.object(app.media, "download_video", side_effect=fake_dl) + + approve_sing_request(app, req) # enqueues + auto-starts the worker thread + + items = [] + for _ in range(200): + with app._download_lock: + items = [i for i in app.download_queue["items"] if i.get("request_id") == req["id"]] + if items and items[0]["status"] in ("completed", "error"): + break + time.sleep(0.02) + + assert items and items[0]["status"] == "completed" + assert items[0]["url"] == "https://youtu.be/GOOD" + updated = app.sing_store.get_request(req["id"]) + assert updated["source_ref"] == "https://youtu.be/GOOD" diff --git a/kj-controller/tests/unit/test_media.py b/kj-controller/tests/unit/test_media.py index cb59272..f27a5b3 100644 --- a/kj-controller/tests/unit/test_media.py +++ b/kj-controller/tests/unit/test_media.py @@ -520,6 +520,9 @@ def test_download_video_success(mock_config, tmp_media_dir, mocker): assert file_path in mi.index assert mi.index[file_path]["youtube_id"] == "abc12345678" assert mi.index[file_path]["duration"] == 180 + # A successful download must clear any prior failure reason so the fallback + # worker never mistakes a stale error for the current attempt's outcome. + assert mi._last_error is None def test_download_video_extract_error(mock_config, tmp_media_dir, mocker): @@ -541,6 +544,27 @@ def test_download_video_extract_error(mock_config, tmp_media_dir, mocker): assert title is None +def test_download_video_records_last_error_for_fallback(mock_config, tmp_media_dir, mocker): + """On extract failure, _last_error carries the reason the fallback worker classifies.""" + mi = MediaIndex(mock_config) + + mock_ydl_instance = mocker.MagicMock() + mock_ydl_instance.extract_info.side_effect = Exception( + "Private video. Sign in if you've been granted access to this video." + ) + mock_ydl_class = mocker.MagicMock() + mock_ydl_class.return_value.__enter__ = mocker.MagicMock(return_value=mock_ydl_instance) + mock_ydl_class.return_value.__exit__ = mocker.MagicMock(return_value=False) + + mock_yt_dlp = mocker.MagicMock() + mock_yt_dlp.YoutubeDL = mock_ydl_class + mocker.patch.dict('sys.modules', {'yt_dlp': mock_yt_dlp}) + + file_path, _ = mi.download_video("https://youtube.com/watch?v=bad") + assert file_path is None + assert "Private video" in (mi._last_error or "") + + def test_download_video_file_not_found(mock_config, tmp_media_dir, mocker): """download_video returns None when downloaded file can't be located.""" mi = MediaIndex(mock_config) diff --git a/kj-controller/tests/unit/test_sing_resolve.py b/kj-controller/tests/unit/test_sing_resolve.py new file mode 100644 index 0000000..8156c55 --- /dev/null +++ b/kj-controller/tests/unit/test_sing_resolve.py @@ -0,0 +1,99 @@ +"""Tests for sing_resolve — pure decision logic for auto-resolving singer submissions. + +The classifier is the risky heart of the fallback feature (deciding whether a +download failure means "this video is gone, try another" vs "a network blip, +retry the same one"), so it gets exhaustive coverage here — no yt-dlp, no +network, no Flask. +""" + +import sing_resolve +from sing_resolve import ( + UNAVAILABLE, + TRANSIENT, + classify_error, + next_candidate_index, +) + + +# --- classify_error: definitively-unavailable ------------------------------- + +def test_incident_private_video_is_unavailable(): + """The exact 2026-07-09 live-incident error must classify as UNAVAILABLE.""" + msg = ( + "ERROR: [youtube] _vMTtVPhd80: Private video. Sign in if you've been " + "granted access to this video. Use --cookies-from-browser or --cookies " + "for the authentication." + ) + assert classify_error(msg) == UNAVAILABLE + + +def test_various_unavailable_messages(): + for msg in [ + "ERROR: [youtube] abc: Video unavailable", + "This video has been removed by the uploader", + "This video is no longer available because the YouTube account " + "associated with this video has been terminated.", + "The uploader has not made this video available in your country", + "Video unavailable. This video is private", + "Join this channel to get access to members-only content", + "Sign in to confirm your age. This video may be inappropriate for some users.", + ]: + assert classify_error(msg) == UNAVAILABLE, msg + + +# --- classify_error: transient --------------------------------------------- + +def test_various_transient_messages(): + for msg in [ + "ERROR: Unable to download webpage: The read operation timed out", + "ERROR: [youtube] abc: HTTP Error 429: Too Many Requests", + "ERROR: HTTP Error 503: Service Unavailable", + "WARNING: [youtube] [pot:bgutil:http] Error reaching GET " + "http://127.0.0.1:4416/ping (caused by TransportError).", + "Connection reset by peer", + "[Errno 8] nodename nor servname provided, or not known: getaddrinfo failed", + "ssl.SSLError: EOF occurred in violation of protocol", + ]: + assert classify_error(msg) == TRANSIENT, msg + + +def test_unknown_and_empty_default_to_transient(): + # Safe default: retrying a good candidate a couple times costs little, but + # wrongly discarding it on a fluke loses the singer's song. + assert classify_error("") == TRANSIENT + assert classify_error(None) == TRANSIENT + assert classify_error("some totally novel yt-dlp failure mode") == TRANSIENT + + +def test_classification_is_case_insensitive(): + assert classify_error("PRIVATE VIDEO") == UNAVAILABLE + assert classify_error("Read Operation TIMED OUT") == TRANSIENT + + +# --- next_candidate_index --------------------------------------------------- + +def test_next_candidate_basic_ordering(): + assert next_candidate_index(total=3, tried=[]) == 0 + assert next_candidate_index(total=3, tried=[0]) == 1 + assert next_candidate_index(total=3, tried=[0, 1]) == 2 + + +def test_next_candidate_skips_tried_out_of_order(): + assert next_candidate_index(total=4, tried=[1]) == 0 + assert next_candidate_index(total=4, tried=[0, 2]) == 1 + + +def test_next_candidate_none_when_exhausted(): + assert next_candidate_index(total=2, tried=[0, 1]) is None + assert next_candidate_index(total=0, tried=[]) is None + + +def test_next_candidate_respects_cap(): + # MAX_CANDIDATES distinct attempts is the hard ceiling even if more remain. + tried = list(range(sing_resolve.MAX_CANDIDATES)) + assert next_candidate_index(total=sing_resolve.MAX_CANDIDATES + 5, tried=tried) is None + + +def test_caps_are_sane(): + assert sing_resolve.MAX_CANDIDATES >= 1 + assert sing_resolve.MAX_TRANSIENT_RETRIES >= 0