Cover-art filler: name the real cure when the music mount is read-only
Tim (Discord): cover-art automation fails with '[Errno 30] Read-only file system' on every file; he chmod 777'd and nothing changed — because EROFS is the KERNEL refusing writes to a docker ':ro' volume mount, which no chmod can fix. SoulSync's response was a wall of per-file warnings and a fix result that still said success with a soft "(read-only?)" hint. - apply_art_to_album_files now pre-flights the album folder with statvfs (asks the kernel, writes nothing): a read-only mount short-circuits the whole album instead of failing file by file. Belt: a per-file/cover EROFS (overlay quirks where statvfs lies) still sets the flag. - the repair worker's apply now FAILS the finding with the actual cure: "remove ':ro' from the volume mapping and recreate the container — chmod cannot change this". EACCES (a real permissions problem chmod CAN fix) deliberately keeps the old soft path. Tests: RO mount short-circuits before any file/cover write, save-time EROFS still flagged, EACCES not conflated with EROFS. 29 art/repair tests pass.
This commit is contained in:
parent
142a1aaf38
commit
3c758635d5
3 changed files with 98 additions and 2 deletions
|
|
@ -15,6 +15,7 @@ Two jobs, both reusing the post-processing standard so the user's
|
|||
from __future__ import annotations
|
||||
|
||||
import contextlib
|
||||
import errno
|
||||
import os
|
||||
from typing import Iterable
|
||||
|
||||
|
|
@ -111,13 +112,34 @@ def apply_art_to_album_files(
|
|||
musicbrainz_release_id). Existing tags are preserved — only art is added.
|
||||
|
||||
Returns counts; never raises (unwritable/read-only files are skipped).
|
||||
``read_only_fs`` is True when the target filesystem itself rejects writes
|
||||
(EROFS — a docker ``:ro`` volume mount; chmod can't fix that) so callers
|
||||
can tell the user the actual cure instead of a generic failure.
|
||||
"""
|
||||
result = {"embedded": 0, "failed": 0, "skipped": 0, "cover_written": False}
|
||||
result = {"embedded": 0, "failed": 0, "skipped": 0, "cover_written": False,
|
||||
"read_only_fs": False}
|
||||
symbols = get_mutagen_symbols()
|
||||
paths = [p for p in (file_paths or []) if p]
|
||||
if not symbols:
|
||||
return result
|
||||
|
||||
# Pre-flight: if the mount is read-only, every save below would fail with
|
||||
# EROFS one by one (Tim's report: a wall of per-file warnings and a 777
|
||||
# chmod that couldn't help). statvfs asks the kernel without writing.
|
||||
probe_dir = folder or (os.path.dirname(paths[0]) if paths else None)
|
||||
if probe_dir:
|
||||
try:
|
||||
if os.statvfs(probe_dir).f_flag & os.ST_RDONLY:
|
||||
logger.warning(
|
||||
"Art apply skipped: %s is on a READ-ONLY filesystem "
|
||||
"(docker ':ro' volume mount — chmod cannot fix this)",
|
||||
probe_dir)
|
||||
result["read_only_fs"] = True
|
||||
result["failed"] = len(paths)
|
||||
return result
|
||||
except OSError:
|
||||
pass # statvfs unavailable/odd fs — fall through to per-file handling
|
||||
|
||||
for fp in paths:
|
||||
if not os.path.isfile(fp):
|
||||
result["skipped"] += 1
|
||||
|
|
@ -144,6 +166,8 @@ def apply_art_to_album_files(
|
|||
result["failed"] += 1
|
||||
except Exception as exc:
|
||||
# Read-only mounts / permission errors land here — skip, don't crash.
|
||||
if getattr(exc, "errno", None) == errno.EROFS:
|
||||
result["read_only_fs"] = True
|
||||
logger.warning("Could not embed art into %s: %s", fp, exc)
|
||||
result["failed"] += 1
|
||||
|
||||
|
|
@ -153,5 +177,7 @@ def apply_art_to_album_files(
|
|||
download_cover_art(album_info, target_dir, context)
|
||||
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)
|
||||
return result
|
||||
|
|
|
|||
|
|
@ -1349,11 +1349,23 @@ class RepairWorker:
|
|||
art_result = apply_art_to_album_files(resolved, metadata, album_info, folder=folder)
|
||||
|
||||
embedded = art_result.get('embedded', 0)
|
||||
if art_result.get('read_only_fs'):
|
||||
# Tim's report: a docker ':ro' volume — every write fails EROFS,
|
||||
# the user chmod 777's in vain, and a soft "(read-only?)" hint
|
||||
# wasn't loud enough. Fail the fix with the actual cure.
|
||||
return {'success': False, 'action': 'applied_cover_art',
|
||||
'error': ('Your music folder is mounted READ-ONLY — the container '
|
||||
'cannot write to it, and chmod cannot change that. '
|
||||
"Remove ':ro' from the volume mapping in your docker "
|
||||
"compose/run (e.g. '/music:/music:ro' → '/music:/music') "
|
||||
'and recreate the container. (Database thumbnail was '
|
||||
'still updated.)'),
|
||||
'art_result': art_result}
|
||||
msg = f'Applied cover art: embedded into {embedded}/{len(resolved)} file(s)'
|
||||
if art_result.get('cover_written'):
|
||||
msg += ' + wrote cover.jpg'
|
||||
if embedded == 0 and not art_result.get('cover_written'):
|
||||
# DB updated but nothing reached disk (e.g. read-only mount).
|
||||
# DB updated but nothing reached disk (e.g. permissions).
|
||||
msg = 'Updated database thumbnail, but could not write art to files (read-only?)'
|
||||
return {'success': True, 'action': 'applied_cover_art', 'message': msg, 'art_result': art_result}
|
||||
|
||||
|
|
|
|||
|
|
@ -145,3 +145,61 @@ def test_apply_counts_failures_without_raising(tmp_path, monkeypatch):
|
|||
res = aa.apply_art_to_album_files([str(f1)], {'album': 'B'}, {'album_name': 'B'}, folder=str(tmp_path))
|
||||
assert res['embedded'] == 0
|
||||
assert res['failed'] == 1 # save() raised (read-only) — counted, not crashed
|
||||
|
||||
|
||||
# ── read-only filesystem detection (Tim: docker ':ro' mount, Errno 30) ──
|
||||
|
||||
|
||||
def test_apply_short_circuits_on_read_only_mount(tmp_path, monkeypatch):
|
||||
"""statvfs says the mount is RO -> bail before touching any file, flag
|
||||
read_only_fs so the caller can name the actual cure (remove ':ro')."""
|
||||
f = tmp_path / 'a.mp3'
|
||||
f.write_bytes(b'')
|
||||
audio = SimpleNamespace(pictures=[], tags=None)
|
||||
monkeypatch.setattr(aa, 'get_mutagen_symbols', lambda: _fake_symbols(audio))
|
||||
monkeypatch.setattr(aa.os, 'statvfs',
|
||||
lambda p: SimpleNamespace(f_flag=aa.os.ST_RDONLY))
|
||||
called = []
|
||||
monkeypatch.setattr(aa, 'download_cover_art', lambda *a, **k: called.append(a))
|
||||
|
||||
res = aa.apply_art_to_album_files([str(f)], {}, {}, folder=str(tmp_path))
|
||||
|
||||
assert res['read_only_fs'] is True
|
||||
assert res['embedded'] == 0 and res['failed'] == 1
|
||||
assert called == [] # cover.jpg write never attempted
|
||||
|
||||
|
||||
def test_apply_flags_erofs_from_save(tmp_path, monkeypatch):
|
||||
"""statvfs passes (overlay quirk) but the save itself raises EROFS ->
|
||||
per-file detection still sets read_only_fs."""
|
||||
import errno as _errno
|
||||
f = tmp_path / 'a.mp3'
|
||||
f.write_bytes(b'')
|
||||
|
||||
def _boom():
|
||||
raise OSError(_errno.EROFS, 'Read-only file system')
|
||||
audio = SimpleNamespace(pictures=[], tags={'ok': 1}, save=_boom)
|
||||
monkeypatch.setattr(aa, 'get_mutagen_symbols', lambda: _fake_symbols(audio))
|
||||
monkeypatch.setattr(aa, 'embed_album_art_metadata', lambda *a, **k: True)
|
||||
monkeypatch.setattr(aa, 'download_cover_art', lambda *a, **k: None)
|
||||
|
||||
res = aa.apply_art_to_album_files([str(f)], {}, {}, folder=str(tmp_path))
|
||||
|
||||
assert res['read_only_fs'] is True
|
||||
assert res['failed'] == 1
|
||||
|
||||
|
||||
def test_apply_normal_failure_not_flagged_read_only(tmp_path, monkeypatch):
|
||||
def _boom():
|
||||
raise PermissionError(13, 'Permission denied')
|
||||
f = tmp_path / 'a.mp3'
|
||||
f.write_bytes(b'')
|
||||
audio = SimpleNamespace(pictures=[], tags={'ok': 1}, save=_boom)
|
||||
monkeypatch.setattr(aa, 'get_mutagen_symbols', lambda: _fake_symbols(audio))
|
||||
monkeypatch.setattr(aa, 'embed_album_art_metadata', lambda *a, **k: True)
|
||||
monkeypatch.setattr(aa, 'download_cover_art', lambda *a, **k: None)
|
||||
|
||||
res = aa.apply_art_to_album_files([str(f)], {}, {}, folder=str(tmp_path))
|
||||
|
||||
assert res['read_only_fs'] is False # EACCES is chmod-able, EROFS is not
|
||||
assert res['failed'] == 1
|
||||
|
|
|
|||
Loading…
Reference in a new issue