danvics/soulsync
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 1

Cover art: detect read-only from the actual write, not statvfs (Sokhi false-positive)

Browse source 
Sokhi got a read-only error from the cover-art filler with NO ':ro' in his
compose. Root cause: my earlier Tim fix added a statvfs pre-flight that bailed
when f_flag & ST_RDONLY — but union/FUSE/network filesystems (mergerfs,
rclone, NFS), ubiquitous in self-hosted setups, misreport those mount flags.
A perfectly writable library could be flagged read-only and blocked. statvfs
is a guess; the only honest test is whether an actual write raises EROFS.

- Removed the statvfs pre-flight entirely. Read-only is now detected solely
  from a real EROFS on the embed write, which also fast-fails the remaining
  files (so no statvfs needed for the fast-fail Tim wanted either).
- Broadened the user message: a genuine read-only mount isn't always ':ro' —
  could be a read-only host/NFS/SMB mount or a mergerfs read-only branch.

Tests: writable FS succeeds even when statvfs would claim read-only (the
regression), real-EROFS-on-write still flagged + bails the rest, EACCES still
not conflated with EROFS. Dropped the now-moot Windows-statvfs test (statvfs
is no longer referenced). 445 art/cover/repair tests pass.
This commit is contained in:
BoulderBadgeDad 2026-06-07 19:57:34 -07:00
parent ed38d60b18
commit 6c3e285a49
3 changed files with 46 additions and 65 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

36
core/metadata/art_apply.py
View file

@ -113,8 +113,15 @@ def apply_art_to_album_files(
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.
(a real EROFS from an actual write — a ':ro' volume, a read-only host/NFS/
SMB mount, or a read-only underlying fs) so callers can tell the user the
real cause instead of a generic failure.
NOTE: read-only is detected from an ACTUAL write raising EROFS, never from
statvfs/mount flags — union/FUSE/network filesystems (mergerfs, rclone,
NFS) common in self-hosted setups misreport those flags, which would
false-block a perfectly writable library (Sokhi: read-only error with no
':ro' in compose). The write itself is the only honest test.
"""
result = {"embedded": 0, "failed": 0, "skipped": 0, "cover_written": False,
"read_only_fs": False}
@ -123,26 +130,6 @@ def apply_art_to_album_files(
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.
# POSIX-only — Windows has no os.statvfs (and no ':ro' bind mounts);
# there we skip straight to the per-file path, same as before this fix.
probe_dir = folder or (os.path.dirname(paths[0]) if paths else None)
_statvfs = getattr(os, "statvfs", None)
if probe_dir and _statvfs is not None:
try:
if _statvfs(probe_dir).f_flag & getattr(os, "ST_RDONLY", 1):
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
@ -169,8 +156,13 @@ def apply_art_to_album_files(
result["failed"] += 1
except Exception as exc:
# Read-only mounts / permission errors land here — skip, don't crash.
# A real EROFS = the mount is read-only; flag it and stop trying the
# rest (fast-fail without the unreliable statvfs guess).
if getattr(exc, "errno", None) == errno.EROFS:
result["read_only_fs"] = True
logger.warning("Could not embed art into %s: read-only filesystem", fp)
result["failed"] += len(paths) - paths.index(fp) # remaining all fail too
break
logger.warning("Could not embed art into %s: %s", fp, exc)
result["failed"] += 1

19
core/repair_worker.py
View file

@ -1394,16 +1394,17 @@ class RepairWorker:
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.
# The music folder is genuinely read-only at the OS level (the
# write raised EROFS). Most common cause is a docker ':ro' volume,
# but it can also be a read-only host mount (NFS/SMB exported ro),
# a mergerfs/union read-only branch, or the library mounted from
# another container as read-only — chmod can't change any of these.
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.)'),
'error': ('Your music folder is READ-ONLY — the container cannot '
'write to it (chmod cannot change this). Check that the '
"volume isn't mapped ':ro', and that the underlying host "
'mount (NFS/SMB/mergerfs) is read-write, then 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'):

56
tests/test_art_apply.py
View file

@ -150,31 +150,36 @@ def test_apply_counts_failures_without_raising(tmp_path, monkeypatch):
# ── 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')."""
def test_apply_does_not_trust_statvfs_writable_fs_succeeds(tmp_path, monkeypatch):
"""Regression (Sokhi): a WRITABLE union/FUSE/NFS mount can misreport
ST_RDONLY in statvfs. The apply must NOT use statvfs — it writes anyway,
and only the actual write decides. Here the embed succeeds even though
statvfs (if it were consulted) would claim read-only."""
f = tmp_path / 'a.mp3'
f.write_bytes(b'')
audio = SimpleNamespace(pictures=[], tags=None)
saved = []
audio = SimpleNamespace(pictures=[], tags=None, add_tags=lambda: None,
save=lambda: saved.append(True))
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)
# Even if statvfs lies and says read-only, the write still happens.
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))
lambda p: SimpleNamespace(f_flag=getattr(aa.os, 'ST_RDONLY', 1)),
raising=False)
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
assert res['embedded'] == 1 and saved == [True]
assert res['read_only_fs'] is False # statvfs ignored; write succeeded
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."""
def test_apply_flags_erofs_from_actual_write(tmp_path, monkeypatch):
"""read-only is detected from a real EROFS on write (the only honest test),
and it fast-fails the remaining files."""
import errno as _errno
f = tmp_path / 'a.mp3'
f.write_bytes(b'')
f1 = tmp_path / 'a.mp3'; f1.write_bytes(b'')
f2 = tmp_path / 'b.mp3'; f2.write_bytes(b'')
def _boom():
raise OSError(_errno.EROFS, 'Read-only file system')
@ -183,10 +188,10 @@ def test_apply_flags_erofs_from_save(tmp_path, monkeypatch):
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))
res = aa.apply_art_to_album_files([str(f1), str(f2)], {}, {}, folder=str(tmp_path))
assert res['read_only_fs'] is True
assert res['failed'] == 1
assert res['failed'] == 2 # first EROFS fails it + bails the rest
def test_apply_normal_failure_not_flagged_read_only(tmp_path, monkeypatch):
@ -205,20 +210,3 @@ def test_apply_normal_failure_not_flagged_read_only(tmp_path, monkeypatch):
assert res['failed'] == 1
def test_apply_works_without_statvfs_windows(tmp_path, monkeypatch):
"""Windows has no os.statvfs — the pre-flight must vanish gracefully,
not crash the apply (Boulder runs on Windows)."""
f = tmp_path / 'a.mp3'
f.write_bytes(b'')
saved = []
audio = SimpleNamespace(pictures=[], tags=None, add_tags=lambda: None,
save=lambda: saved.append(True))
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)
monkeypatch.delattr(aa.os, 'statvfs', raising=False)
res = aa.apply_art_to_album_files([str(f)], {}, {}, folder=str(tmp_path))
assert res['embedded'] == 1 and saved == [True]
assert res['read_only_fs'] is False