Cover art: surface read-only on the cover.jpg sidecar write too (Sokhi, #804 follow-up)
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.
This commit is contained in:
parent
902eb38fb8
commit
1ca14d1c19
3 changed files with 43 additions and 2 deletions
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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')
|
||||
|
|
|
|||
Loading…
Reference in a new issue