Catch a wrong playlist write before it is published

src/rb2engine/chain.py

@@ -0,0 +1,87 @@
+"""The one place that reconstructs an Engine ``nextEntityId`` chain.
+
+Engine stores playlist membership as a singly linked list: each
+``PlaylistEntity`` row points at its successor and the tail points at 0. Track
+order is whatever that chain says, so anything wanting to know "what does this
+playlist actually contain" has to walk it.
+
+WHY THIS IS SHARED
+------------------
+The walk existed in three places that had drifted apart. The writer's copy
+treated a row the chain never reaches as fatal; ``verify``'s copy silently
+returned the shorter, tidier list and reported no problem. Two oracles that
+disagree about what "correct" means will eventually contradict each other on a
+real stick — one refusing to publish a database the other calls clean.
+
+They still need different *reactions*: the writer must abort before publishing,
+while ``verify`` must record the finding and carry on checking. So the walk
+raises, and each caller decides what that means. What they can no longer do is
+disagree about whether there is something to react to.
+"""
+
+from __future__ import annotations
+
+from collections.abc import Sequence
+
+# Engine / libdjinterop sentinel: tail of every next*Id chain points at 0.
+NO_NEXT = 0
+
+
+class ChainInconsistent(RuntimeError):
+    """A ``nextEntityId`` chain does not account for every row in its list.
+
+    Subclasses ``RuntimeError`` because the writer's contract is to raise
+    ``RuntimeError`` on a database it refuses to publish, and callers that only
+    care about that broader promise should not need to know this type exists.
+    """
+
+    def __init__(self, list_id: int, message: str) -> None:
+        super().__init__(f"playlist listId={list_id}: {message}")
+        self.list_id = list_id
+
+
+def walk_entity_chain(
+    list_id: int, rows: Sequence[tuple[int, int, int]]
+) -> list[int]:
+    """Track ids for one list in chain order, from ``(id, trackId, nextEntityId)``.
+
+    Walking the chain rather than reading rows in id order is deliberate: a row
+    spliced into the middle, or one no walk reaches, is invisible to a plain
+    ``ORDER BY id`` comparison. A real conversion published a spurious entry
+    second-to-last, exactly where row order would have hidden it.
+
+    Raises ``ChainInconsistent`` if the chain forks or fails to reach every row.
+    """
+    by_next: dict[int, tuple[int, int]] = {}
+    for eid, tid, nxt in rows:
+        # Two rows sharing a successor would silently collapse into one dict
+        # entry. The count check below would still fire, but it would blame the
+        # wrong thing, so name this corruption for what it is.
+        if int(nxt) in by_next:
+            raise ChainInconsistent(
+                list_id,
+                f"two PlaylistEntity rows share nextEntityId={int(nxt)} — "
+                "chain is forked",
+            )
+        by_next[int(nxt)] = (int(eid), int(tid))
+
+    order: list[int] = []
+    curr = NO_NEXT
+    seen: set[int] = set()
+    while curr in by_next:
+        eid, track_id = by_next[curr]
+        if eid in seen:  # corrupt chain must not spin forever
+            break
+        seen.add(eid)
+        order.insert(0, track_id)
+        curr = eid
+
+    # A row that no chain walk reaches is still a row Engine may honour; make
+    # the count mismatch loud instead of letting the walk hide it.
+    if len(order) != len(rows):
+        raise ChainInconsistent(
+            list_id,
+            f"{len(rows)} PlaylistEntity rows but the nextEntityId chain "
+            f"reaches {len(order)} — chain is inconsistent",
+        )
+    return order

src/rb2engine/verify.py

@@ -14,6 +14,7 @@
 from pathlib import Path
 from typing import Any
 
+from rb2engine.chain import ChainInconsistent, walk_entity_chain
 from rb2engine.errors import FatalError
 from rb2engine.ir import SourceLibrary, SourcePlaylist, SourceTrack
 from rb2engine.ir_engine import artwork_content_hash
@@ -34,9 +35,6 @@
 # Empty-slot sentinel shared with writer/blobs and ir_engine.
 _EMPTY_SAMPLE = -1.0
 
-# Engine / libdjinterop: tail of every nextEntityId chain.
-_NO_NEXT = 0
-
 
 @dataclass(frozen=True, slots=True)
 class Discrepancy:
@@ -563,7 +561,18 @@ def _compare_playlists(
         expected_track_ids = _expected_entity_track_ids(
             pl, source, by_path=by_path, drive_root=drive_root, engine_lib=engine_lib
         )
-        actual_track_ids = _entity_track_order(conn, list_id)
+        actual_track_ids, chain_problem = _entity_track_order(conn, list_id)
+        if chain_problem is not None:
+            # Report it in its own right: a broken chain is a defect even when
+            # the set of tracks happens to match what the source expected.
+            discrepancies.append(
+                Discrepancy(
+                    track_id=None,
+                    field=f"playlist[{pl.name}].chain",
+                    expected="every row reachable from the nextEntityId chain",
+                    actual=chain_problem,
+                )
+            )
         if expected_track_ids != actual_track_ids:
             discrepancies.append(
                 Discrepancy(
@@ -615,25 +624,32 @@ def _expected_entity_track_ids(
     return out
 
 
-def _entity_track_order(conn: sqlite3.Connection, list_id: int) -> list[int]:
-    """Reconstruct track order from nextEntityId (tail sentinel = 0)."""
-    rows = conn.execute(
-        "SELECT id, trackId, nextEntityId FROM PlaylistEntity WHERE listId = ?",
-        (list_id,),
-    ).fetchall()
-    by_next = {int(next_id): (int(eid), int(track_id)) for eid, track_id, next_id in rows}
-    order: list[int] = []
-    curr = _NO_NEXT
-    # Guard against cycles in a corrupted chain.
-    seen_entity: set[int] = set()
-    while curr in by_next:
-        eid, track_id = by_next[curr]
-        if eid in seen_entity:
-            break
-        seen_entity.add(eid)
-        order.insert(0, track_id)
-        curr = eid
-    return order
+def _entity_track_order(
+    conn: sqlite3.Connection, list_id: int
+) -> tuple[list[int], str | None]:
+    """Track order from the nextEntityId chain, plus any inconsistency found.
+
+    Returns ``(order, problem)``. ``problem`` is None when the chain accounts
+    for every row; otherwise it describes what is wrong and ``order`` holds the
+    rows in id order as a best effort.
+
+    This used to swallow an inconsistent chain and return the shorter, tidier
+    list, which reported a clean library while the writer's own gate would
+    refuse to publish that exact database. Both now walk the same code
+    (``rb2engine.chain``); they differ only in how they react, because ``verify``
+    must record the finding and keep checking rather than abort.
+    """
+    rows = [
+        (int(eid), int(track_id), int(next_id))
+        for eid, track_id, next_id in conn.execute(
+            "SELECT id, trackId, nextEntityId FROM PlaylistEntity WHERE listId = ?",
+            (list_id,),
+        )
+    ]
+    try:
+        return walk_entity_chain(list_id, rows), None
+    except ChainInconsistent as exc:
+        return [track_id for _, track_id, _ in rows], str(exc)
 
 
 def _compare_artwork(

src/rb2engine/writer/build.py

@@ -23,6 +23,7 @@
 import sqlite3
 import sys
 import tempfile
+from collections.abc import Sequence
 from pathlib import Path
 
 from rb2engine.errors import FatalError
@@ -190,6 +191,7 @@ def build_library(
     # Late imports keep this module importable while sibling writer modules
     # land (concurrent workers). Contract signatures are fixed.
     from rb2engine.writer import database as database_mod
+    from rb2engine.writer import playlists as playlists_mod
     from rb2engine.writer.playlists import insert_playlists
 
     conn: sqlite3.Connection | None = None
@@ -315,8 +317,12 @@ def build_library(
         # --- playlists ---------------------------------------------------------
         if on_progress is not None:
             on_progress("playlists", 0, 0)
+        intended_playlists: dict[int, Sequence[int]] = {}
         n_playlists = insert_playlists(
-            conn, lib.playlists, track_id_map=track_id_map or {}
+            conn,
+            lib.playlists,
+            track_id_map=track_id_map or {},
+            intended_out=intended_playlists,
         )
         report.counters.playlists_converted = n_playlists
 
@@ -343,6 +349,25 @@ def build_library(
         _fsync_file(tmp_path)
         _fsync_dir(db2)
 
+        # Re-check the copy that actually crossed to the target volume.
+        #
+        # The check inside insert_playlists runs in the writing transaction, so
+        # it can only prove SQLite agreed with us at that moment — it cannot see
+        # the commit, the half-gigabyte copy over USB, or this volume's driver.
+        # A conversion once published playlists containing a track that was in
+        # no source playlist and still exited 0, and the staged database is
+        # discarded before anyone can compare it, so this is the last point
+        # where that class of corruption is still catchable. Running it before
+        # os.replace means a failure leaves the user's previous m.db in place.
+        if intended_playlists:
+            check_conn = sqlite3.connect(f"file:{tmp_path}?mode=ro", uri=True)
+            try:
+                playlists_mod.assert_entities_match_intent(
+                    check_conn, intended_playlists
+                )
+            finally:
+                check_conn.close()
+
         os.replace(tmp_path, m_db_path)
         _fsync_dir(db2)
 

src/rb2engine/writer/playlists.py

@@ -20,8 +20,9 @@
 import logging
 import sqlite3
 from collections import defaultdict
-from collections.abc import Mapping, Sequence
+from collections.abc import Mapping, MutableMapping, Sequence
 
+from rb2engine.chain import walk_entity_chain
 from rb2engine.ir import SourcePlaylist
 
 # Engine / libdjinterop sentinel: tail of every next*Id chain.
@@ -39,6 +40,7 @@ def insert_playlists(
     playlists: Sequence[SourcePlaylist],
     *,
     track_id_map: Mapping[int, int],
+    intended_out: MutableMapping[int, Sequence[int]] | None = None,
 ) -> int:
     """Insert Playlist + PlaylistEntity rows for *playlists*.
 
@@ -53,6 +55,11 @@ def insert_playlists(
     track_id_map:
         ``SourceTrack.rb_id`` → Engine ``Track.id``. Entries whose rb_id is
         missing are skipped (soft track skips must not fail playlist write).
+    intended_out:
+        Optional sink receiving ``listId`` → intended track order. The caller
+        needs it to re-check the database *after* it has been committed and
+        copied to the target volume; the in-transaction check below cannot see
+        that far.
 
     Returns
     -------
@@ -194,6 +201,9 @@ def insert_playlists(
     next_entity_id = 1
 
     duplicate_entries = 0
+    # listId → the exact track order this function intends to write. Kept so
+    # the write can be read back and checked against intent (see below).
+    intended: dict[int, list[int]] = {}
     for rb_id, pl in by_rb.items():
         list_id = engine_id_of[rb_id]
         # Preserve source order; drop members whose tracks were skipped.
@@ -219,6 +229,7 @@ def insert_playlists(
             continue
 
         n = len(engine_track_ids)
+        intended[list_id] = engine_track_ids
         entity_ids = list(range(next_entity_id, next_entity_id + n))
         next_entity_id += n
         for i, track_id in enumerate(engine_track_ids):
@@ -240,6 +251,10 @@ def insert_playlists(
         )
         _bump_sequence(conn, "PlaylistEntity", next_entity_id - 1)
 
+    assert_entities_match_intent(conn, intended)
+    if intended_out is not None:
+        intended_out.update(intended)
+
     if duplicate_entries:
         logger.warning(
             "dropped %d duplicate playlist entrie(s): rekordbox allows the same "
@@ -250,6 +265,49 @@ def insert_playlists(
     return len(playlists)
 
 
+def assert_entities_match_intent(
+    conn: sqlite3.Connection, intended: Mapping[int, Sequence[int]]
+) -> None:
+    """Read every written chain back and fail if it differs from intent.
+
+    The writer builds each chain in memory and inserts it in one pass, so in
+    principle the database must agree. In practice a conversion shipped two
+    playlists containing a track that appeared nowhere in the source, and
+    ``convert`` still exited 0 — the corruption was only found later by
+    ``verify``. Checking here turns that class of silent wrong-database into a
+    build-time failure, before the new m.db is swapped into place.
+
+    Every row in the table is examined, not just the lists we meant to fill: a
+    spurious row landing on a folder or an empty playlist is precisely as wrong
+    as one landing on a playlist we wrote, and scoping the check to *intended*
+    would leave that whole class invisible.
+    """
+    grouped: dict[int, list[tuple[int, int, int]]] = defaultdict(list)
+    for list_id, eid, tid, nxt in conn.execute(
+        "SELECT listId, id, trackId, nextEntityId FROM PlaylistEntity"
+    ):
+        grouped[int(list_id)].append((int(eid), int(tid), int(nxt)))
+
+    unexpected_lists = sorted(set(grouped) - set(intended))
+    if unexpected_lists:
+        raise RuntimeError(
+            f"PlaylistEntity rows exist for listId(s) {unexpected_lists} that "
+            "no playlist write intended"
+        )
+
+    for list_id, expected in intended.items():
+        actual = walk_entity_chain(list_id, grouped.get(list_id, []))
+        if list(actual) != list(expected):
+            extra = sorted(set(actual) - set(expected))
+            missing = sorted(set(expected) - set(actual))
+            raise RuntimeError(
+                f"playlist listId={list_id}: written entries do not match the "
+                f"{len(expected)} intended ({len(actual)} written; "
+                f"unexpected track ids {extra or 'none'}; "
+                f"absent track ids {missing or 'none'})"
+            )
+
+
 def _bump_sequence(conn: sqlite3.Connection, table: str, seq: int) -> None:
     """Ensure sqlite_sequence.seq >= *seq* after explicit PRIMARY KEY inserts.