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 --- .../src/meshbay_node/indexer/group_index.py | 13 --- .../src/meshbay_node/transport/webrtc_server.py | 96 +++++++++++++--------- 2 files changed, 58 insertions(+), 51 deletions(-) (limited to 'packages/meshbay-node/src/meshbay_node') diff --git a/packages/meshbay-node/src/meshbay_node/indexer/group_index.py b/packages/meshbay-node/src/meshbay_node/indexer/group_index.py index 1ce4e0a..25081ef 100644 --- a/packages/meshbay-node/src/meshbay_node/indexer/group_index.py +++ b/packages/meshbay-node/src/meshbay_node/indexer/group_index.py @@ -78,19 +78,6 @@ class GroupIndex: def get_entry(self, file_id: str) -> IndexEntry | None: return self._entries.get(file_id) - def get_entry_by_path(self, path: str) -> IndexEntry | None: - """ - Linear scan — entries are keyed by content id, not path, and nothing - before the Videos app needed to go the other way (a client always - already has the id from index_sync/index_delta). Fine for an - on-demand, per-tile lookup against a few thousand entries; revisit - if a future caller makes this hot. - """ - for entry in self._entries.values(): - if entry.path == path: - return entry - return None - @property def entries(self) -> list[IndexEntry]: return list(self._entries.values()) diff --git a/packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py b/packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py index b4db051..6709fbc 100644 --- a/packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py +++ b/packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py @@ -2759,21 +2759,30 @@ class WebRTCPeerSession: async def _do_music_meta_request(self, msg: dict) -> None: """ - docs/musicbay.md §4.3: MusicBrainz metadata for one path, resolved - from the group's index. Album-level (release), the direct analogue - of Videos' show-level TMDB caching: one search per (artist, album) - pair serves cover art and canonical naming to every track of the - same release, keyed off the `artist`/`album` fields enrich_audio.py - already populated at index time (from tags, or the filename-parse - fallback) — never re-parsed here. + docs/musicbay.md §4.3: MusicBrainz metadata for one track, resolved + from the group's index by its content id. Album-level (release), the + direct analogue of Videos' show-level TMDB caching: one search per + (artist, album) pair serves cover art and canonical naming to every + track of the same release, keyed off the `artist`/`album` fields + enrich_audio.py already populated at index time (from tags, or the + filename-parse fallback) — never re-parsed here. + + Keyed by `file_id` (the entry's own content hash), not `path`: found + live (2026-08-25) — `IndexEntry.path` is the *folder* a file is in + (indexer.py's `_virtual_dir`), so any two tracks in the same folder + (routinely true — an album is one folder, many tracks) shared the + same `.path`, and looking a track up by it silently resolved to + whichever entry happened to be first in the index. Three unrelated + albums showed the same wrong cover before this fix, all sharing one + folder with the track that legitimately matched it. """ - path = msg.get("path") - log.debug("music_meta_req path=%r", path) - if not isinstance(path, str) or not path: - self._send({"type": "error", "detail": "Missing path"}) + file_id = msg.get("file_id") + log.debug("music_meta_req file_id=%r", file_id) + if not isinstance(file_id, str) or not file_id: + self._send({"type": "error", "detail": "Missing file_id"}) return ctx = self._group_ctx() - entry = ctx["index"].get_entry_by_path(path) + entry = ctx["index"].get_entry(file_id) if not entry: self._send({"type": "error", "detail": "File not found"}) return @@ -2789,7 +2798,7 @@ class WebRTCPeerSession: or not ctx.get("musicbrainz_enabled", True) or not entry.artist or not entry.album): self._send({"type": MNP.MUSIC_META_RESP, "v": MNP_VERSION, - "path": path, "confidence": 0}) + "file_id": file_id, "confidence": 0}) return mbid = await media_cache.get_file_mbid(entry.id) @@ -2799,7 +2808,7 @@ class WebRTCPeerSession: result, ratio = await musicbrainz_client.search_release(entry.artist, entry.album) if result is None or ratio < 0.6: self._send({"type": MNP.MUSIC_META_RESP, "v": MNP_VERSION, - "path": path, "confidence": 0}) + "file_id": file_id, "confidence": 0}) return mbid = result.get("id") artist_credit = result.get("artist-credit") or [] @@ -2814,11 +2823,11 @@ class WebRTCPeerSession: cover_thumb_hash = await self._fetch_and_cache_cover( media_cache, musicbrainz_client, mbid) - log.debug("music_meta_req path=%r: replying mbid=%s cover=%s", - path, mbid, cover_thumb_hash) + log.debug("music_meta_req file_id=%r: replying mbid=%s cover=%s", + file_id, mbid, cover_thumb_hash) self._send({ - "type": MNP.MUSIC_META_RESP, "v": MNP_VERSION, "path": path, + "type": MNP.MUSIC_META_RESP, "v": MNP_VERSION, "file_id": file_id, "mbid": mbid, "artist": meta.get("artist"), "album": meta.get("album"), @@ -2830,17 +2839,25 @@ class WebRTCPeerSession: async def _do_media_meta_request(self, msg: dict) -> None: """ - docs/mediacenter.md §5.4: TMDB metadata for one path, resolved from - the group's index (root+relpath the client already knows from - index_sync/index_delta — never a raw filesystem path off the wire). + docs/mediacenter.md §5.4: TMDB metadata for one file, resolved from + the group's index by its content id (root+relpath the client already + knows from index_sync/index_delta identify the entry; its own `id` + is what actually names one file — never a raw filesystem path off + the wire). + + Keyed by `file_id`, not `path`: `IndexEntry.path` is the *folder* a + file is in (indexer.py's `_virtual_dir`), so two files in the same + folder — any multi-episode season, routinely — shared the same + `.path`, and a lookup by it could silently resolve to the wrong + entry (found live via the Music app's identical bug, 2026-08-25). """ - path = msg.get("path") - log.debug("media_meta_req path=%r", path) - if not isinstance(path, str) or not path: - self._send({"type": "error", "detail": "Missing path"}) + file_id = msg.get("file_id") + log.debug("media_meta_req file_id=%r", file_id) + if not isinstance(file_id, str) or not file_id: + self._send({"type": "error", "detail": "Missing file_id"}) return ctx = self._group_ctx() - entry = ctx["index"].get_entry_by_path(path) + entry = ctx["index"].get_entry(file_id) if not entry: self._send({"type": "error", "detail": "File not found"}) return @@ -2853,7 +2870,7 @@ class WebRTCPeerSession: # TMDB match" as the ordinary case. if media_cache is None or tmdb_client is None or not ctx.get("tmdb_enabled", True): self._send({"type": MNP.MEDIA_META_RESP, "v": MNP_VERSION, - "path": path, "confidence": 0}) + "file_id": file_id, "confidence": 0}) return is_show = entry.season is not None and entry.episode is not None @@ -2870,7 +2887,7 @@ class WebRTCPeerSession: result, ratio = await self._tmdb_search(tmdb_client, entry, is_show) if result is None or ratio < 0.6: self._send({"type": MNP.MEDIA_META_RESP, "v": MNP_VERSION, - "path": path, "confidence": 0}) + "file_id": file_id, "confidence": 0}) return tmdb_id = str(result["id"]) meta = await self._tmdb_build_meta(tmdb_client, tmdb_id, media_type, result) @@ -2881,11 +2898,11 @@ class WebRTCPeerSession: media_cache, tmdb_client, meta.get("poster_path")) backdrop_thumb_hash = await self._fetch_and_cache_poster( media_cache, tmdb_client, meta.get("backdrop_path")) - log.debug("media_meta_req path=%r: replying tmdb_id=%s poster=%s backdrop=%s", - path, tmdb_id, poster_thumb_hash, backdrop_thumb_hash) + log.debug("media_meta_req file_id=%r: replying tmdb_id=%s poster=%s backdrop=%s", + file_id, tmdb_id, poster_thumb_hash, backdrop_thumb_hash) resp = { - "type": MNP.MEDIA_META_RESP, "v": MNP_VERSION, "path": path, + "type": MNP.MEDIA_META_RESP, "v": MNP_VERSION, "file_id": file_id, "tmdb_id": tmdb_id, "title": meta.get("title"), "original_title": meta.get("original_title"), "overview": meta.get("overview"), @@ -3013,25 +3030,28 @@ class WebRTCPeerSession: (§3.4/§V6) — not just the one file the operator happened to be looking at, so the correction actually sticks regardless of which episode a future render picks as representative. + + Keyed by `file_id`, not `path` — see `_do_media_meta_request`'s + docstring for why a folder-level path cannot name one file. """ - path = msg.get("path") + file_id = msg.get("file_id") tmdb_id = msg.get("tmdb_id") media_type = msg.get("media_type") - if not isinstance(path, str) or not path: - self._send({"type": "error", "detail": "Missing path"}) + if not isinstance(file_id, str) or not file_id: + self._send({"type": "error", "detail": "Missing file_id"}) return if not isinstance(tmdb_id, str) or not tmdb_id or media_type not in ("movie", "tv"): self._send({"type": "error", "detail": "Missing tmdb_id or media_type"}) return ctx = self._group_ctx() - entry = ctx["index"].get_entry_by_path(path) + entry = ctx["index"].get_entry(file_id) if not entry: self._send({"type": "error", "detail": "File not found"}) return if not self._has_admin_authority(): self._send({"type": "error", "detail": "No authorized key for this"}) return - subject = f"path={path},tmdb_id={tmdb_id},media_type={media_type}" + subject = f"file_id={file_id},tmdb_id={tmdb_id},media_type={media_type}" self._issue_admin_challenge(OP_TMDB_OVERRIDE, subject) async def _admin_exec_tmdb_override( @@ -3043,10 +3063,10 @@ class WebRTCPeerSession: self._audit("admin_auth_failed", f"tmdb_override:{subject}") return fields = dict(part.split("=", 1) for part in subject.split(",")) - path, tmdb_id, media_type = fields["path"], fields["tmdb_id"], fields["media_type"] + file_id, tmdb_id, media_type = fields["file_id"], fields["tmdb_id"], fields["media_type"] ctx = self._group_ctx() - entry = ctx["index"].get_entry_by_path(path) + entry = ctx["index"].get_entry(file_id) media_cache = self._ctx.get("media_cache") if entry is None or media_cache is None: self._send({"type": "error", "detail": "File or media cache not available"}) @@ -3058,7 +3078,7 @@ class WebRTCPeerSession: await media_cache.set_file_tmdb(e.id, tmdb_id, media_type) self._audit("tmdb_override", subject) - notice = {"type": MNP.TMDB_OVERRIDE_ACK, "v": MNP_VERSION, "path": path, + notice = {"type": MNP.TMDB_OVERRIDE_ACK, "v": MNP_VERSION, "file_id": file_id, "tmdb_id": tmdb_id, "media_type": media_type} for uid, session in list(self._peer_registry().items()): try: -- cgit v1.2.3