From c83d4862e804208f3dc266d2b6d0e6c7c646d463 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Mon, 8 Jun 2026 09:06:37 -0700 Subject: [PATCH] Cover art: stop crying "(read-only?)" when files are simply already arted (Sokhi/Boulder) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The real reason Sokhi's cover-art fix "didn't work" and Boulder saw the same message on a WRITABLE Windows box: _fix_missing_cover_art reported "Updated database thumbnail, but could not write art to files (read-only?)" whenever embedded==0 AND cover_written==False — but the overwhelmingly common cause of that is every file ALREADY having embedded art (skipped, not failed). The message blamed read-only on a perfectly writable library, which sent us chasing a read-only ghost across three commits. Sokhi's /fix returns 200 (success), so he was never hitting the genuine EROFS path at all. Now the message is derived from the art_result breakdown: - embedded/cover written → "Applied cover art: …" - failed>0 (non-EROFS) → "… check file/folder permissions" - skipped>0 (already arted) → "Cover art already present on all N file(s)" - otherwise → "no file artwork was applied" Genuine read-only (EROFS) still hard-fails with the clear mount message. Tests: already-arted→present (not read-only), failed→permissions, EROFS→hard fail, embedded→success. 453 cover/repair/art tests pass. --- core/repair_worker.py | 34 +++++++++++++++---- tests/test_cover_art_targets.py | 60 +++++++++++++++++++++++++++++++++ 2 files changed, 88 insertions(+), 6 deletions(-) diff --git a/core/repair_worker.py b/core/repair_worker.py index 311109c9..58dc2517 100644 --- a/core/repair_worker.py +++ b/core/repair_worker.py @@ -1407,12 +1407,34 @@ class RepairWorker: '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'): - msg += ' + wrote cover.jpg' - if embedded == 0 and not art_result.get('cover_written'): - # DB updated but nothing reached disk (e.g. permissions). - msg = 'Updated database thumbnail, but could not write art to files (read-only?)' + skipped = art_result.get('skipped', 0) + failed = art_result.get('failed', 0) + cover_written = art_result.get('cover_written') + + wrote_parts = [] + if embedded: + wrote_parts.append(f'embedded into {embedded}/{len(resolved)} file(s)') + if cover_written: + wrote_parts.append('wrote cover.jpg') + + if wrote_parts: + msg = 'Applied cover art: ' + ' + '.join(wrote_parts) + elif failed: + # Real per-file write failures that were NOT a read-only mount + # (genuine EROFS is handled above) — almost always file/folder + # permissions or a locked file. + msg = (f'Updated database thumbnail, but could not write art to ' + f'{failed} file(s) — check file/folder permissions') + elif skipped: + # Every file already had embedded art and no new cover.jpg was + # needed — nothing to do, NOT a failure. This is the case that made + # the old "(read-only?)" message fire on perfectly writable + # libraries (Boulder on Windows, Sokhi): the files were simply + # already arted, so embedded==0 and cover_written==False. + msg = f'Cover art already present on all {skipped} file(s) — database thumbnail updated' + else: + # No file art applied and nothing found to write. + msg = 'Updated database thumbnail (no file artwork was applied)' if artist_result is not None and artist_result.get('success'): msg += ' + applied artist image' return {'success': True, 'action': 'applied_cover_art', 'message': msg, 'art_result': art_result} diff --git a/tests/test_cover_art_targets.py b/tests/test_cover_art_targets.py index 3c4de992..0538fd8d 100644 --- a/tests/test_cover_art_targets.py +++ b/tests/test_cover_art_targets.py @@ -128,3 +128,63 @@ def test_artist_action_without_found_artist_url_fails_cleanly(tmp_path): assert res['success'] is False album_thumb, artist_thumb = _thumbs(w) assert artist_thumb == 'http://old/artist.jpg' # nothing changed + + +# ── apply-result message accuracy (Sokhi/Boulder: "read-only?" on writable fs) ── + +def _add_track(w, path): + conn = w.db._get_connection() + c = conn.cursor() + c.execute("INSERT INTO tracks VALUES ('t1', 'al1', ?)", (str(path),)) + conn.commit() + conn.close() + + +def _apply_returns(monkeypatch, **art_result): + import core.metadata.art_apply as aa + base = {'embedded': 0, 'failed': 0, 'skipped': 0, 'cover_written': False, 'read_only_fs': False} + base.update(art_result) + monkeypatch.setattr(aa, 'apply_art_to_album_files', lambda *a, **k: base) + + +def test_already_arted_reports_present_not_readonly(tmp_path, monkeypatch): + # The bug: all files already had art (skipped) → embedded 0, cover 0 → the + # old message cried "(read-only?)" on a perfectly writable library. + w = _worker(tmp_path) + f = tmp_path / 'song.mp3'; f.write_bytes(b'x') + _add_track(w, f) + _apply_returns(monkeypatch, skipped=1) + res = w._fix_missing_cover_art('album', 'al1', None, {**DETAILS, '_fix_action': 'album'}) + assert res['success'] is True + assert 'already present' in res['message'].lower() + assert 'read-only' not in res['message'].lower() + + +def test_failed_writes_blame_permissions_not_readonly(tmp_path, monkeypatch): + w = _worker(tmp_path) + f = tmp_path / 'song.mp3'; f.write_bytes(b'x') + _add_track(w, f) + _apply_returns(monkeypatch, failed=1) + res = w._fix_missing_cover_art('album', 'al1', None, {**DETAILS, '_fix_action': 'album'}) + assert res['success'] is True + assert 'permission' in res['message'].lower() + assert 'read-only' not in res['message'].lower() + + +def test_genuine_read_only_still_hard_fails(tmp_path, monkeypatch): + w = _worker(tmp_path) + f = tmp_path / 'song.mp3'; f.write_bytes(b'x') + _add_track(w, f) + _apply_returns(monkeypatch, read_only_fs=True) + res = w._fix_missing_cover_art('album', 'al1', None, {**DETAILS, '_fix_action': 'album'}) + assert res['success'] is False + assert 'read-only' in res['error'].lower() + + +def test_embedded_success_message(tmp_path, monkeypatch): + w = _worker(tmp_path) + f = tmp_path / 'song.mp3'; f.write_bytes(b'x') + _add_track(w, f) + _apply_returns(monkeypatch, embedded=1) + res = w._fix_missing_cover_art('album', 'al1', None, {**DETAILS, '_fix_action': 'album'}) + assert res['success'] is True and 'embedded into 1' in res['message']