Download clients: don't crash init when the download path can't be created
The SoundCloud/Amazon/Tidal/Qobuz/Deezer/HiFi/Lidarr clients did an UNGUARDED
mkdir(parents=True) on the configured download path in __init__. With a Docker
'/app' path (or any unmounted/misconfigured volume), that raises Permission
Denied, the plugin registry nulls the whole client, and the source vanishes —
SoulseekClient already guards the identical mkdir and just warns. Outside the
container this also failed every test_download_orchestrator_soundcloud.py test
(10) by leaving client('soundcloud') = None for the patch targets.
Fix: wrap the mkdir in try/except OSError + warn (matching soulseek) across all
seven clients and the orchestrator's runtime path-update; the dir is created
lazily at download time. Real robustness win: a slow/unmounted volume at boot no
longer silently drops download sources. Regression test forces an uncreatable
path and asserts init doesn't raise — pinned in any environment.
Full suite green: 6713 passed, 0 failed (was 10 failed).
This commit is contained in:
parent
ed0a2079cf
commit
729a06c6d7
9 changed files with 52 additions and 8 deletions
|
|
@ -78,7 +78,10 @@ class AmazonDownloadClient(DownloadSourcePlugin):
|
|||
if download_path is None:
|
||||
download_path = config_manager.get("soulseek.download_path", "./downloads")
|
||||
self.download_path = Path(download_path)
|
||||
self.download_path.mkdir(parents=True, exist_ok=True)
|
||||
try:
|
||||
self.download_path.mkdir(parents=True, exist_ok=True)
|
||||
except OSError as e:
|
||||
logger.warning(f"Could not verify download path {self.download_path}: {e}")
|
||||
|
||||
self._quality = quality_tier_for_source("amazon", default="flac")
|
||||
self._allow_fallback = config_manager.get("amazon_download.allow_fallback", True)
|
||||
|
|
|
|||
|
|
@ -93,7 +93,10 @@ class DeezerDownloadClient(DownloadSourcePlugin):
|
|||
if download_path is None:
|
||||
download_path = config_manager.get('soulseek.download_path', './downloads')
|
||||
self.download_path = Path(download_path)
|
||||
self.download_path.mkdir(parents=True, exist_ok=True)
|
||||
try:
|
||||
self.download_path.mkdir(parents=True, exist_ok=True)
|
||||
except OSError as e:
|
||||
logger.warning(f"Could not verify download path {self.download_path}: {e}")
|
||||
|
||||
# Engine reference is populated by set_engine() at registration
|
||||
# time. None until orchestrator wires the registry.
|
||||
|
|
|
|||
|
|
@ -143,7 +143,10 @@ class DownloadOrchestrator:
|
|||
continue
|
||||
if hasattr(client, 'download_path') and client.download_path != new_path:
|
||||
client.download_path = new_path
|
||||
client.download_path.mkdir(parents=True, exist_ok=True)
|
||||
try:
|
||||
client.download_path.mkdir(parents=True, exist_ok=True)
|
||||
except OSError as e:
|
||||
logger.warning(f"Could not verify download path {new_path}: {e}")
|
||||
# YouTube also caches path in yt-dlp opts
|
||||
if hasattr(client, 'download_opts') and 'outtmpl' in client.download_opts:
|
||||
client.download_opts['outtmpl'] = str(new_path / '%(title)s.%(ext)s')
|
||||
|
|
|
|||
|
|
@ -257,7 +257,10 @@ class HiFiClient(DownloadSourcePlugin):
|
|||
if download_path is None:
|
||||
download_path = config_manager.get('soulseek.download_path', './downloads')
|
||||
self.download_path = Path(download_path)
|
||||
self.download_path.mkdir(parents=True, exist_ok=True)
|
||||
try:
|
||||
self.download_path.mkdir(parents=True, exist_ok=True)
|
||||
except OSError as e:
|
||||
logger.warning(f"Could not verify download path {self.download_path}: {e}")
|
||||
|
||||
self._instances = []
|
||||
self._instance_lock = threading.Lock()
|
||||
|
|
|
|||
|
|
@ -47,7 +47,10 @@ class LidarrDownloadClient(DownloadSourcePlugin):
|
|||
if download_path is None:
|
||||
download_path = config_manager.get('soulseek.download_path', './downloads')
|
||||
self.download_path = Path(download_path)
|
||||
self.download_path.mkdir(parents=True, exist_ok=True)
|
||||
try:
|
||||
self.download_path.mkdir(parents=True, exist_ok=True)
|
||||
except OSError as e:
|
||||
logger.warning(f"Could not verify download path {self.download_path}: {e}")
|
||||
|
||||
self.active_downloads: Dict[str, Dict[str, Any]] = {}
|
||||
self._download_lock = threading.Lock()
|
||||
|
|
|
|||
|
|
@ -120,7 +120,10 @@ class QobuzClient(DownloadSourcePlugin):
|
|||
download_path = config_manager.get('soulseek.download_path', './downloads')
|
||||
|
||||
self.download_path = Path(download_path)
|
||||
self.download_path.mkdir(parents=True, exist_ok=True)
|
||||
try:
|
||||
self.download_path.mkdir(parents=True, exist_ok=True)
|
||||
except OSError as e:
|
||||
logger.warning(f"Could not verify download path {self.download_path}: {e}")
|
||||
|
||||
logger.info(f"Qobuz client using download path: {self.download_path}")
|
||||
|
||||
|
|
|
|||
|
|
@ -128,7 +128,14 @@ class SoundcloudClient(DownloadSourcePlugin):
|
|||
download_path = config_manager.get('soulseek.download_path', './downloads')
|
||||
|
||||
self.download_path = Path(download_path)
|
||||
self.download_path.mkdir(parents=True, exist_ok=True)
|
||||
# Don't crash construction if the path isn't creatable yet (e.g. an
|
||||
# unmounted/misconfigured volume) — the registry would null the whole
|
||||
# client and the source vanishes. Warn and continue, same as
|
||||
# SoulseekClient; the dir is (re)created lazily at download time.
|
||||
try:
|
||||
self.download_path.mkdir(parents=True, exist_ok=True)
|
||||
except OSError as e:
|
||||
logger.warning(f"Could not verify SoundCloud download path {self.download_path}: {e}")
|
||||
|
||||
logger.info(f"SoundCloud client using download path: {self.download_path}")
|
||||
|
||||
|
|
|
|||
|
|
@ -120,7 +120,10 @@ class TidalDownloadClient(DownloadSourcePlugin):
|
|||
download_path = config_manager.get('soulseek.download_path', './downloads')
|
||||
|
||||
self.download_path = Path(download_path)
|
||||
self.download_path.mkdir(parents=True, exist_ok=True)
|
||||
try:
|
||||
self.download_path.mkdir(parents=True, exist_ok=True)
|
||||
except OSError as e:
|
||||
logger.warning(f"Could not verify download path {self.download_path}: {e}")
|
||||
|
||||
logger.info(f"Tidal download client using download path: {self.download_path}")
|
||||
|
||||
|
|
|
|||
|
|
@ -846,3 +846,19 @@ def test_live_download_a_known_public_track(tmp_dl: Path) -> None:
|
|||
assert final_path is not None
|
||||
assert os.path.exists(final_path)
|
||||
assert os.path.getsize(final_path) > 100 * 1024
|
||||
|
||||
|
||||
# ── construction robustness: an uncreatable download path must not crash init ──
|
||||
# Regression: the download path is read from config (often a Docker /app path). If
|
||||
# mkdir fails (unmounted/misconfigured volume, or running outside the container),
|
||||
# the client must warn and continue — NOT raise. An unguarded mkdir made the
|
||||
# registry null the whole client, dropping SoundCloud as a source entirely (and
|
||||
# failing every orchestrator-soundcloud test outside Docker).
|
||||
def test_init_survives_uncreatable_download_path(tmp_path):
|
||||
from core.soundcloud_client import SoundcloudClient
|
||||
blocker = tmp_path / "iam_a_file"
|
||||
blocker.write_text("x")
|
||||
bad_path = str(blocker / "subdir") # mkdir under a file -> NotADirectoryError (OSError)
|
||||
client = SoundcloudClient(download_path=bad_path) # must not raise
|
||||
assert client is not None
|
||||
assert str(client.download_path) == bad_path
|
||||
|
|
|
|||
Loading…
Reference in a new issue