diff --git a/core/metadata/art_apply.py b/core/metadata/art_apply.py index faac0b7d..448ff0f2 100644 --- a/core/metadata/art_apply.py +++ b/core/metadata/art_apply.py @@ -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 diff --git a/core/repair_worker.py b/core/repair_worker.py index 887c6efa..cd17ee8c 100644 --- a/core/repair_worker.py +++ b/core/repair_worker.py @@ -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'): diff --git a/tests/test_art_apply.py b/tests/test_art_apply.py index 4169eab9..2be5d9e4 100644 --- a/tests/test_art_apply.py +++ b/tests/test_art_apply.py @@ -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