From 1ca14d1c1983c9b0d6fa693a3d9fd1c9af58e2b9 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Mon, 8 Jun 2026 08:34:36 -0700 Subject: [PATCH] Cover art: surface read-only on the cover.jpg sidecar write too (Sokhi, #804 follow-up) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sokhi still hit the read-only error after the statvfs fix (6c3e285a). Root cause was a gap that fix didn't cover: read_only_fs was only set from the per-file EMBED write, but download_cover_art SWALLOWS its own cover.jpg EROFS (logs "Error downloading cover.jpg" and returns). So when an album's tracks already have embedded art, the embed loop is skipped, only the cover.jpg sidecar write runs, its EROFS is swallowed, and the filler reported success while spamming the log — exactly Sokhi's case. Fix (no blast radius — download_cover_art still never re-raises, since its import-pipeline callers aren't wrapped): on EROFS it now records '_cover_read_only' on the passed context instead of just logging; apply_art_to_ album_files passes a capture dict and promotes that to read_only_fs. So a cover-only read-only album now correctly surfaces the read-only message instead of a silent success. Tests: +1 — embed skipped (track already arted) + cover.jpg read-only → read_only_fs True. 472 cover/art/repair tests pass. --- core/metadata/art_apply.py | 11 ++++++++++- core/metadata/artwork.py | 13 ++++++++++++- tests/test_art_apply.py | 21 +++++++++++++++++++++ 3 files changed, 43 insertions(+), 2 deletions(-) diff --git a/core/metadata/art_apply.py b/core/metadata/art_apply.py index 448ff0f2..f81a35cd 100644 --- a/core/metadata/art_apply.py +++ b/core/metadata/art_apply.py @@ -168,11 +168,20 @@ def apply_art_to_album_files( target_dir = folder or (os.path.dirname(paths[0]) if paths else None) if target_dir and os.path.isdir(target_dir): + # download_cover_art swallows its own write errors, so a read-only mount + # (EROFS) on the cover.jpg sidecar would otherwise go undetected here — + # the exact gap that left cover-only albums reporting success on a + # read-only filesystem (Sokhi: tracks already had embedded art, so the + # embed loop above never tripped EROFS). Pass a capture dict and read + # the read-only flag it sets back. + cover_ctx = context if isinstance(context, dict) else {} try: - download_cover_art(album_info, target_dir, context) + download_cover_art(album_info, target_dir, cover_ctx) result["cover_written"] = folder_has_cover_sidecar(target_dir) except Exception as exc: if getattr(exc, "errno", None) == errno.EROFS: result["read_only_fs"] = True logger.warning("cover.jpg write failed for %s: %s", target_dir, exc) + if cover_ctx.get("_cover_read_only"): + result["read_only_fs"] = True return result diff --git a/core/metadata/artwork.py b/core/metadata/artwork.py index 3698a064..091ee1c4 100644 --- a/core/metadata/artwork.py +++ b/core/metadata/artwork.py @@ -2,6 +2,7 @@ from __future__ import annotations +import errno import os import re import time @@ -566,4 +567,14 @@ def download_cover_art(album_info: dict, target_dir: str, context: dict = None): handle.write(image_data) logger.info("Cover art downloaded to: %s", cover_path) except Exception as exc: - logger.error("Error downloading cover.jpg: %s", exc) + # A read-only mount (EROFS) is a "can't write" condition the caller + # needs to surface (cover-art filler #804/Tim/Sokhi) — but we must NOT + # re-raise (import callers aren't wrapped here). Record it on the + # context so callers that care can detect it, instead of just spamming + # the log with a swallowed error. + if getattr(exc, "errno", None) == errno.EROFS: + if isinstance(context, dict): + context["_cover_read_only"] = True + logger.warning("cover.jpg write blocked — read-only filesystem: %s", cover_path) + else: + logger.error("Error downloading cover.jpg: %s", exc) diff --git a/tests/test_art_apply.py b/tests/test_art_apply.py index 2be5d9e4..3a590ef1 100644 --- a/tests/test_art_apply.py +++ b/tests/test_art_apply.py @@ -194,6 +194,27 @@ def test_apply_flags_erofs_from_actual_write(tmp_path, monkeypatch): assert res['failed'] == 2 # first EROFS fails it + bails the rest +def test_cover_only_read_only_is_detected(tmp_path, monkeypatch): + """The gap that left Sokhi stuck (#804): tracks already have art so the + embed loop is skipped, but the cover.jpg write hits a read-only mount. + download_cover_art swallows that EROFS onto the context — apply must still + surface read_only_fs (not report a silent success).""" + f = tmp_path / 'a.flac'; f.write_bytes(b'') + audio = SimpleNamespace(pictures=['pic'], tags={'ok': 1}) # already has art → embed skipped + monkeypatch.setattr(aa, 'get_mutagen_symbols', lambda: _fake_symbols(audio)) + + def _fake_download(album_info, target_dir, ctx): + # mimic download_cover_art's EROFS handling: record on context, swallow + if isinstance(ctx, dict): + ctx['_cover_read_only'] = True + monkeypatch.setattr(aa, 'download_cover_art', _fake_download) + + res = aa.apply_art_to_album_files([str(f)], {}, {}, folder=str(tmp_path)) + + assert res['embedded'] == 0 and res['skipped'] == 1 # nothing embedded + assert res['read_only_fs'] is True # but read-only surfaced + + def test_apply_normal_failure_not_flagged_read_only(tmp_path, monkeypatch): def _boom(): raise PermissionError(13, 'Permission denied')