From c8c3789cb9b76b355da9881ca0c70946dde2352f Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Mon, 1 Jun 2026 13:23:08 -0700 Subject: [PATCH] Album bundle: fall back to per-track on an I/O error, don't hard-fail the batch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Defense-in-depth follow-up to #760. Even with the entrypoint chown fix, if the album-bundle staging dir ever can't be created/written (permissions, read-only mount, disk full), the dispatch caught the plugin exception and marked the whole batch failed — even though the album had already downloaded (the #715 symptom: 'release finishes downloading but the batch fails'). Now an OSError from the plugin is flagged fallback-eligible, so the dispatch returns to the per-track flow instead of hard-failing. OSError covers the staging/filesystem failure that motivated this (#760's PermissionError) and, by Python's IOError==OSError aliasing, any propagated transient I/O error — falling back is never worse than hard-failing, and per-track is the universal graceful path. Programming errors (TypeError, KeyError, RuntimeError, …) are NOT OSError and stay terminal, so genuine bugs still fail loudly — the existing 'plugin exception => failure' contract and its test are preserved. Test: new test_dispatch_staging_oserror_falls_back_to_per_track (PermissionError on the staging dir -> result False, phase 'analysis', not failed). Existing RuntimeError-is-terminal test still passes. 131 album-bundle/plugin tests green. --- core/downloads/album_bundle_dispatch.py | 17 ++++++++++++++++- tests/test_album_bundle_dispatch.py | 20 ++++++++++++++++++++ 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/core/downloads/album_bundle_dispatch.py b/core/downloads/album_bundle_dispatch.py index 634c0e95..ef6b104f 100644 --- a/core/downloads/album_bundle_dispatch.py +++ b/core/downloads/album_bundle_dispatch.py @@ -173,7 +173,22 @@ def try_dispatch( ) except Exception as exc: logger.exception("[Album Bundle] %s plugin raised: %s", mode, exc) - outcome = {'success': False, 'error': f'Plugin error: {exc}'} + # An OSError means an I/O step failed after the source already had the + # album — most importantly the staging dir not being writable (#760), + # but also any transient filesystem error. Treat it as fallback-eligible + # so we return to the per-track flow instead of hard-failing the whole + # batch (the #715 symptom: files download, then the batch fails). + # Programming errors (TypeError, KeyError, …) are NOT OSError and stay + # terminal, so genuine bugs still fail loudly. (requests' network + # exceptions also subclass OSError, but plugins normally catch those + # internally and return an outcome rather than raising; if one does + # surface here, falling back to per-track is still the safe choice.) + is_io_failure = isinstance(exc, OSError) + outcome = { + 'success': False, + 'error': f'Plugin error: {exc}', + 'fallback': is_io_failure, + } if not outcome.get('success'): err = outcome.get('error', 'Album bundle download failed') diff --git a/tests/test_album_bundle_dispatch.py b/tests/test_album_bundle_dispatch.py index 5cafa065..b9ea5424 100644 --- a/tests/test_album_bundle_dispatch.py +++ b/tests/test_album_bundle_dispatch.py @@ -275,6 +275,26 @@ def test_dispatch_plugin_exception_treated_as_failure() -> None: assert 'network down' in state.failed_with +def test_dispatch_staging_oserror_falls_back_to_per_track(): + """A filesystem/staging failure (e.g. #760's PermissionError creating the + staging dir) means the album downloaded but couldn't be staged locally — + fall back to the per-track flow rather than hard-failing the whole batch.""" + state = _FakeState() + plugin = MagicMock() + plugin.download_album_to_staging.side_effect = PermissionError( + "[Errno 13] Permission denied: 'storage/album_bundle_staging'") + result = try_dispatch( + batch_id='b1', is_album=True, + album_context={'name': 'Carnival'}, artist_context={'name': 'Some Artist'}, + config_get=_config({'download_source.mode': 'soulseek'}), + plugin_resolver=lambda _name: plugin, state=state, + ) + assert result is False # fell back; master continues per-track + assert state.failed_with == '' # NOT hard-failed + assert state.fields['phase'] == 'analysis' + assert state.fields['album_bundle_state'] == 'fallback' + + def test_dispatch_strips_whitespace_from_names() -> None: """Trailing whitespace in batch context shouldn't fail the eligibility predicate AND should be cleaned before passing to