diff --git a/tests/remote/unit/test_download_manager.py b/tests/remote/unit/test_download_manager.py index b8accba0..14d3d11f 100644 --- a/tests/remote/unit/test_download_manager.py +++ b/tests/remote/unit/test_download_manager.py @@ -7,6 +7,7 @@ cleanup/serialize); they do not exercise the actual subprocess download path. from __future__ import annotations from datetime import datetime, timedelta +from pathlib import Path import pytest @@ -141,3 +142,30 @@ def test_normalize_sub_format_preserves_original_sentinel() -> None: # A real codec still maps to its enum member; unknowns collapse to None. assert normalize_sub_format("srt") is Subtitle.Codec.SubRip assert normalize_sub_format("nonsense") is None + + +def test_worker_write_result_retries_denied_replace(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + """Windows denies os.replace while the parent holds the progress file open, so the write must retry. + + Without the retry a denied replace loses that update. A value the worker writes once, such as a + phase change, then stays invisible until the next write. + """ + import json + import os + + from unshackle.core.api import download_worker + + target = tmp_path / "progress.json" + real_replace = os.replace + attempts = [] + + def flaky_replace(src: Path, dst: Path) -> None: + attempts.append(dst) + if len(attempts) == 1: + raise PermissionError(13, "Permission denied", str(dst)) + real_replace(src, dst) + + monkeypatch.setattr(download_worker.os, "replace", flaky_replace) + download_worker.write_result(target, {"progress": 42.0}) + assert json.loads(target.read_text(encoding="utf-8")) == {"progress": 42.0} + assert len(attempts) == 2 diff --git a/unshackle/core/api/download_manager.py b/unshackle/core/api/download_manager.py index 1cbe6e58..cbdf1c76 100644 --- a/unshackle/core/api/download_manager.py +++ b/unshackle/core/api/download_manager.py @@ -1004,7 +1004,6 @@ class DownloadQueueManager: stat = os.stat(progress_path) stat_key = (stat.st_mtime_ns, stat.st_size) if stat_key != last_progress_stat: - last_progress_stat = stat_key with open(progress_path, "r", encoding="utf-8") as handle: progress_data = json.load(handle) if progress_data.get("phase") and progress_data["phase"] != job.phase: @@ -1041,7 +1040,8 @@ class DownloadQueueManager: if progress_data != last_published: last_published = progress_data self.publish(job, "progress") - except (FileNotFoundError, json.JSONDecodeError, ValueError) as e: + last_progress_stat = stat_key + except (OSError, json.JSONDecodeError, ValueError) as e: log.debug(f"Could not read progress for job {job.job_id}: {e}") if job.cancel_event.is_set() or job.status == JobStatus.CANCELLED: diff --git a/unshackle/core/api/download_worker.py b/unshackle/core/api/download_worker.py index 5767b7b0..fb64abe8 100644 --- a/unshackle/core/api/download_worker.py +++ b/unshackle/core/api/download_worker.py @@ -6,6 +6,7 @@ import json import logging import os import sys +import time import traceback from pathlib import Path from typing import Any, Dict @@ -21,10 +22,19 @@ def read_payload(path: Path) -> Dict[str, Any]: def write_result(path: Path, payload: Dict[str, Any]) -> None: - # Atomic replace: the parent polls this file while it is being rewritten. + """Write the payload with an atomic replace, because the parent polls this file during the write. + + Windows denies the replace while the parent holds the destination open, so retry with a backoff. + """ path.parent.mkdir(parents=True, exist_ok=True) tmp = path.with_name(path.name + ".tmp") tmp.write_text(json.dumps(payload), encoding="utf-8") + for attempt in range(5): + try: + os.replace(tmp, path) + return + except PermissionError: + time.sleep(0.02 * (attempt + 1)) os.replace(tmp, path)