From 2d144d76cee55cf8faaacf196e716a0930dfd7e9 Mon Sep 17 00:00:00 2001 From: Christophe Besson Date: Wed, 26 Aug 2026 01:08:58 +0200 Subject: fix(node,hub): key music/media metadata lookups by file_id, not path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit IndexEntry.path is the *folder* a file is in (indexer.py's _virtual_dir docstring: "the directory a file appears in"), not the file itself. GroupIndex.get_entry_by_path() treated it as if it named one file, and every one of its four callers did too: _do_music_meta_request, _do_media_meta_request, _do_tmdb_override, and _admin_exec_tmdb_override. Any two files sharing a folder — an album is one folder with many tracks, a season is one folder with many episodes — collided: a lookup by path silently returned whichever entry the index happened to iterate to first, regardless of which file the client actually asked about. Found live (2026-08-25): three unrelated albums ("High Tone - Various", two "Le Peuple de l'Herbe" albums) all showed the same MusicBrainz cover, because all their representative tracks happened to sit in one "high_tone" folder alongside a track that legitimately matched that cover. A force-reload didn't help — the bug is server-side, not a stale client state. Fixed by keying these four request/response pairs by `file_id` (the entry's own content hash — already unique, already how every other lookup in the system identifies a file) instead of `path`, both in the wire messages (music_meta_req/resp, media_meta_req/resp, tmdb_override) and in music-app.js/video-app.js's own hooks. GroupIndex.get_entry_by_path is now unused and removed — GroupIndex.get_entry(file_id) already did the right thing. No test previously exercised either handler with two entries sharing a folder — the only existing coverage (test_tmdb_override_policy.py) gave each entry its own folder, so the bug never had a chance to show up. Added that scenario there and in two new test files, all confirmed failing against the pre-fix code before being confirmed green against the fix. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_013XSohfUQQiaE77qyFLgSv3 --- .../meshbay-node/tests/test_music_meta_request.py | 136 +++++++++++++++++++++ 1 file changed, 136 insertions(+) create mode 100644 packages/meshbay-node/tests/test_music_meta_request.py (limited to 'packages/meshbay-node/tests/test_music_meta_request.py') diff --git a/packages/meshbay-node/tests/test_music_meta_request.py b/packages/meshbay-node/tests/test_music_meta_request.py new file mode 100644 index 0000000..96307f2 --- /dev/null +++ b/packages/meshbay-node/tests/test_music_meta_request.py @@ -0,0 +1,136 @@ +""" +`_do_music_meta_request`, keyed by `file_id` (2026-08-25 fix). + +Regression found live: `IndexEntry.path` is the *folder* a track is in +(indexer.py's `_virtual_dir`), not the track itself — an album is one +folder with many tracks in it, so looking a track up by `.path` alone +(the pre-fix behaviour, `GroupIndex.get_entry_by_path`) silently resolved +every track in that folder to whichever entry the index happened to +return first. Three unrelated albums showed the same wrong MusicBrainz +cover in production before this was found and fixed. +""" + +import pytest +from cryptography.hazmat.primitives.asymmetric.ed25519 import Ed25519PrivateKey +from meshbay_common.protocol import MNP, IndexEntry +from meshbay_node.indexer.group_index import GroupIndex +from meshbay_node.media_cache import MediaCache +from meshbay_node.transport.webrtc_server import WebRTCPeerSession + +pytestmark = pytest.mark.asyncio + + +class FakeMusicBrainzClient: + """Returns a distinct, deterministic match per (artist, album) pair — + real enough to prove the server routed to the *right* track's own tags, + not a stand-in for musicbrainz.py's own search-quality tests.""" + + def __init__(self): + self.calls = [] + + async def search_release(self, artist, album): + self.calls.append((artist, album)) + return ( + {"id": f"mbid-for-{artist}-{album}", "title": album, + "artist-credit": [{"name": artist}]}, + 1.0, + ) + + async def fetch_cover_art(self, mbid): + return f"cover-bytes-for-{mbid}".encode() + + +def _entry(path: str, name: str, file_id: str, artist: str, album: str) -> IndexEntry: + return IndexEntry( + id=file_id, name=name, path=path, size=1, type="audio", added_at=0, + artist=artist, album=album, display_title=name, + ) + + +@pytest.fixture +async def media_cache(tmp_path): + c = MediaCache(db_path=tmp_path / "media_cache.db") + await c.open() + yield c + await c.close() + + +def _session(index, media_cache, musicbrainz_client): + session = WebRTCPeerSession.__new__(WebRTCPeerSession) + session._ctx = { + "index": index, + "media_cache": media_cache, + "musicbrainz_client": musicbrainz_client, + "musicbrainz_enabled": True, + } + session._group_id = None + session.sent = [] + session._send = session.sent.append + return session + + +async def test_two_tracks_in_the_same_folder_each_get_their_own_metadata(media_cache): + """The exact production scenario: two tracks share a folder (an album), + with different artist/album tags of their own (one mistagged, sitting + in the wrong physical folder — a real, if messy, real-world case). Each + must resolve against its *own* tags, not whichever track the index + happens to return first for that shared folder path.""" + index = GroupIndex(group_id="g" * 32, sk_node=Ed25519PrivateKey.generate()) + track_a = _entry("music/high_tone", "a.mp3", "id-a", "High Tone", "Future Dub (1)") + track_b = _entry("music/high_tone", "b.mp3", "id-b", "Le Peuple de l'Herbe", "Triple Zero") + index.add_entry(track_a) + index.add_entry(track_b) + client = FakeMusicBrainzClient() + session = _session(index, media_cache, client) + + await session._do_music_meta_request({"file_id": "id-a"}) + await session._do_music_meta_request({"file_id": "id-b"}) + + resp_a, resp_b = session.sent + assert resp_a["file_id"] == "id-a" + assert resp_a["artist"] == "High Tone" + assert resp_a["album"] == "Future Dub (1)" + assert resp_b["file_id"] == "id-b" + assert resp_b["artist"] == "Le Peuple de l'Herbe" + assert resp_b["album"] == "Triple Zero" + assert resp_a["cover_thumb_hash"] != resp_b["cover_thumb_hash"], ( + "two different tracks sharing a folder must not end up with the same cover") + assert set(client.calls) == {("High Tone", "Future Dub (1)"), + ("Le Peuple de l'Herbe", "Triple Zero")} + + +async def test_missing_file_id_is_refused(media_cache): + index = GroupIndex(group_id="g" * 32, sk_node=Ed25519PrivateKey.generate()) + session = _session(index, media_cache, FakeMusicBrainzClient()) + + await session._do_music_meta_request({}) + + assert session.sent == [{"type": "error", "detail": "Missing file_id"}] + + +async def test_unknown_file_id_is_refused(media_cache): + index = GroupIndex(group_id="g" * 32, sk_node=Ed25519PrivateKey.generate()) + session = _session(index, media_cache, FakeMusicBrainzClient()) + + await session._do_music_meta_request({"file_id": "nope"}) + + assert session.sent == [{"type": "error", "detail": "File not found"}] + + +async def test_no_confidence_below_threshold_still_answers_by_file_id(media_cache): + index = GroupIndex(group_id="g" * 32, sk_node=Ed25519PrivateKey.generate()) + index.add_entry(_entry("music/x", "a.mp3", "id-a", "Some Artist", "Some Album")) + + class LowConfidenceClient(FakeMusicBrainzClient): + async def search_release(self, artist, album): + return {"id": "mbid", "title": album, + "artist-credit": [{"name": artist}]}, 0.1 + + session = _session(index, media_cache, LowConfidenceClient()) + + await session._do_music_meta_request({"file_id": "id-a"}) + + assert session.sent == [{ + "type": MNP.MUSIC_META_RESP, "v": session.sent[0]["v"], + "file_id": "id-a", "confidence": 0, + }] -- cgit v1.2.3