From 3060678f2919749cc5d3a0d9dc692505dc477172 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Sun, 31 May 2026 15:41:04 -0700 Subject: [PATCH] Quarantine: manage a quarantined file from the download modal (Listen / Accept / Search) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Clicking a quarantined track's status used to open the generic search modal, identical to a plain failure — no way to review or recover the file. It now opens a chooser: - Listen: streams the file in-app via a new /api/quarantine//stream endpoint (range-supported; the real audio Content-Type is recovered from the sidecar since the on-disk file ends in .quarantined). - Accept & Import: existing /approve (restore + re-import, gates bypassed). - Search for a different result: the existing candidates modal (old behavior). Non-quarantine failures (not_found / failed / cancelled) are unchanged — a single click listener routes by dataset set at render time, so a task that fails then later quarantines can't end up double-bound. Also fixes the Accept failure on Windows: the Listen stream holds an open file handle, so the subsequent restore move hit WinError 32 ('file in use') and the endpoint mislabeled it 'thin sidecar'. Accept now releases the audio handle before approving, and approve/recover moves retry briefly on transient OS locks (_move_with_retry). Accept also auto-falls-back to Recover-to-Staging for genuinely thin/orphaned sidecars. Tests: stream-info resolution (sidecar + filename-fallback + missing), and _move_with_retry success/give-up. --- core/imports/quarantine.py | 64 +++++++-- tests/imports/test_quarantine_management.py | 56 ++++++++ web_server.py | 28 +++- webui/static/downloads.js | 142 ++++++++++++++++++++ webui/static/style.css | 74 ++++++++++ 5 files changed, 354 insertions(+), 10 deletions(-) diff --git a/core/imports/quarantine.py b/core/imports/quarantine.py index 43ed5191..3df79f85 100644 --- a/core/imports/quarantine.py +++ b/core/imports/quarantine.py @@ -14,6 +14,7 @@ from __future__ import annotations import json import os import shutil +import time from pathlib import Path from typing import Any, Dict, List, Optional, Tuple @@ -289,6 +290,56 @@ def _restore_filename(quarantined_filename: str, sidecar_original: Optional[str] return base +def get_quarantine_entry_stream_info( + quarantine_dir: str, entry_id: str +) -> Optional[Tuple[str, str]]: + """Resolve a quarantined entry to ``(file_path, original_extension)`` for + in-app playback. + + The on-disk file carries a ``.quarantined`` suffix, so its own extension is + useless for picking an audio MIME type. Recover the real extension from the + sidecar's ``original_filename`` when present, else from the quarantine + filename convention. Returns None when the entry's file can't be found. + """ + file_path, sidecar_path = _resolve_entry_paths(quarantine_dir, entry_id) + if not file_path or not os.path.isfile(file_path): + return None + + sidecar_original: Optional[str] = None + if sidecar_path: + try: + with open(sidecar_path, encoding="utf-8") as f: + sidecar_original = json.load(f).get("original_filename") + except Exception as exc: + logger.debug("stream-info: sidecar read failed for %s: %s", entry_id, exc) + + original_name = _restore_filename(os.path.basename(file_path), sidecar_original) + extension = os.path.splitext(original_name)[1].lower() + return file_path, extension + + +def _move_with_retry(src: str, dst: str, attempts: int = 4, delay: float = 0.4) -> bool: + """Move a file, retrying briefly on transient OS locks. + + On Windows a still-open read handle (e.g. the in-app quarantine preview + player that just streamed the file) makes shutil.move raise + PermissionError / WinError 32 "file in use". The handle is released a beat + after playback stops, so a few short retries clear the common case without + failing the whole Approve/Recover. Returns True on success. + """ + last_exc: Optional[BaseException] = None + for i in range(attempts): + try: + shutil.move(src, dst) + return True + except OSError as exc: + last_exc = exc + if i < attempts - 1: + time.sleep(delay) + logger.error("move failed after %d attempts: %s -> %s: %s", attempts, src, dst, last_exc) + return False + + def approve_quarantine_entry( quarantine_dir: str, entry_id: str, @@ -334,10 +385,9 @@ def approve_quarantine_entry( restored_path = os.path.join(restore_dir, original_name) restored_path = _ensure_unique_path(restored_path) - try: - shutil.move(file_path, restored_path) - except OSError as exc: - logger.error("approve: failed to restore %s -> %s: %s", file_path, restored_path, exc) + if not _move_with_retry(file_path, restored_path): + logger.error("approve: failed to restore %s -> %s (file may still be in use)", + file_path, restored_path) return None try: @@ -377,10 +427,8 @@ def recover_to_staging( os.makedirs(staging_dir, exist_ok=True) target = _ensure_unique_path(os.path.join(staging_dir, restored_name)) - try: - shutil.move(file_path, target) - except OSError as exc: - logger.error("recover: failed to move %s -> %s: %s", file_path, target, exc) + if not _move_with_retry(file_path, target): + logger.error("recover: failed to move %s -> %s (file may still be in use)", file_path, target) return None if sidecar_path and os.path.isfile(sidecar_path): diff --git a/tests/imports/test_quarantine_management.py b/tests/imports/test_quarantine_management.py index d2c527c0..3615ca6a 100644 --- a/tests/imports/test_quarantine_management.py +++ b/tests/imports/test_quarantine_management.py @@ -5,6 +5,7 @@ from core.imports.quarantine import ( approve_quarantine_entry, delete_quarantine_entry, entry_id_from_quarantined_filename, + get_quarantine_entry_stream_info, get_quarantined_source_keys, list_quarantine_entries, recover_to_staging, @@ -125,6 +126,41 @@ def test_list_handles_orphan_quarantined_file_without_sidecar(tmp_path): assert entries[0]["has_full_context"] is False +# ────────────────────────────────────────────────────────────────────── +# get_quarantine_entry_stream_info — in-app "Listen" support +# ────────────────────────────────────────────────────────────────────── + +def test_stream_info_resolves_path_and_extension_from_sidecar(tmp_path): + qfile, _ = _write_entry(tmp_path, "20260514_120000", "song.flac", with_context=True) + entry_id = entry_id_from_quarantined_filename(qfile.name) + + info = get_quarantine_entry_stream_info(str(tmp_path), entry_id) + + assert info is not None + file_path, ext = info + assert file_path == str(qfile) + assert ext == ".flac" # real audio ext, NOT ".quarantined" + + +def test_stream_info_recovers_extension_without_sidecar(tmp_path): + # Orphan .quarantined with no sidecar — extension comes from the filename + # convention so playback still gets a correct Content-Type. + qfile = tmp_path / "20260514_120000_orphan.mp3.quarantined" + qfile.write_bytes(b"X" * 100) + entry_id = entry_id_from_quarantined_filename(qfile.name) + + info = get_quarantine_entry_stream_info(str(tmp_path), entry_id) + + assert info is not None + file_path, ext = info + assert file_path == str(qfile) + assert ext == ".mp3" + + +def test_stream_info_returns_none_for_missing_entry(tmp_path): + assert get_quarantine_entry_stream_info(str(tmp_path), "does_not_exist") is None + + def test_list_skips_orphan_sidecars_without_file(tmp_path): sidecar = tmp_path / "20260514_120000_only.json" sidecar.write_text(json.dumps({"original_filename": "only.flac", "quarantine_reason": "x"})) @@ -398,3 +434,23 @@ def test_source_keys_dedup_repeated_sources(tmp_path): keys = get_quarantined_source_keys(str(tmp_path)) assert keys == {("peer", "dupe.flac")} + + +# ────────────────────────────────────────────────────────────────────── +# _move_with_retry — resilient move (Windows file-lock case) +# ────────────────────────────────────────────────────────────────────── + +def test_move_with_retry_succeeds(tmp_path): + from core.imports.quarantine import _move_with_retry + src = tmp_path / "a.flac"; src.write_bytes(b"x" * 10) + dst = tmp_path / "out" / "a.flac" + (tmp_path / "out").mkdir() + assert _move_with_retry(str(src), str(dst)) is True + assert dst.exists() and not src.exists() + + +def test_move_with_retry_returns_false_on_missing_source(tmp_path): + from core.imports.quarantine import _move_with_retry + # attempts=1 keeps the test fast (no retry sleeps) + assert _move_with_retry(str(tmp_path / "nope.flac"), str(tmp_path / "dst.flac"), + attempts=1, delay=0) is False diff --git a/web_server.py b/web_server.py index 51724911..2025220d 100644 --- a/web_server.py +++ b/web_server.py @@ -7131,6 +7131,25 @@ def approve_quarantine_item(entry_id): return jsonify({"success": False, "error": str(e)}), 500 +@app.route('/api/quarantine//stream', methods=['GET']) +def stream_quarantine_item(entry_id): + """Stream a quarantined audio file in-app (range-supported) so the user can + listen before deciding to approve, search again, or delete it. The file + lives in the quarantine dir with a `.quarantined` suffix, so the real audio + extension (and thus Content-Type) is recovered from the sidecar.""" + try: + from core.imports.quarantine import get_quarantine_entry_stream_info + info = get_quarantine_entry_stream_info(_get_quarantine_dir(), entry_id) + if info is None: + return jsonify({"error": "Quarantined file not found"}), 404 + file_path, extension = info + mimetype = _AUDIO_MIME_TYPES.get(extension, 'audio/mpeg') + return _serve_audio_file_with_range(file_path, mimetype_override=mimetype) + except Exception as e: + logger.error(f"[Quarantine] Error streaming {entry_id}: {e}") + return jsonify({"error": str(e)}), 500 + + @app.route('/api/quarantine//recover', methods=['POST']) def recover_quarantine_item(entry_id): """Fallback for legacy thin sidecars: move file into Staging so the user @@ -11877,17 +11896,22 @@ _AUDIO_MIME_TYPES = { } -def _serve_audio_file_with_range(file_path): +def _serve_audio_file_with_range(file_path, mimetype_override=None): """Serve an on-disk audio file with HTTP range support (HTML5 seeking). Shared by /stream/audio (current track) and /stream/library-audio (the crossfade pre-loader, which plays the NEXT track on a second