From 8c097faacc5a02f64743735dfb82970fc0e187cc Mon Sep 17 00:00:00 2001 From: trav Date: Thu, 27 Aug 2026 18:31:33 -0400 Subject: [PATCH] v0.14.0: a removal you make sticks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 43 made the merge report honest, and in doing so made its one real limitation impossible to miss: every merge was a union, so a song removed from a playlist on one machine was handed straight back by the other on the next sync, and a track deleted from the library came back with it. A deletion you cannot make stick is not a deletion. The union was there for a real reason — two copies with no common ancestor cannot tell "A added this" from "B removed it" — so the missing evidence is written down instead of inferred. New lintunes/tombstones.py: a playlist keeps track_events {track id: [when, add|remove]}, the library keeps deleted_tracks {track id: when} in library_metadata.json, and a merge applies the newest event per track across both copies. A removal beats a copy that merely still had the song; a deliberate re-add afterwards beats the removal; a track nobody touched still merges as a union, which stays the safe behavior where there is no evidence either way. Deliberately not "the newer copy wins wholesale": that one-liner silently drops a song the other machine added while you were removing one, which test_an_unrelated_addition_is_not_lost pins. Events are recorded in the funnels that already exist — _set_track_ids diffs before/after so a reorder records nothing, _remove_tracks stamps the library, _restore_tracks clears it so Ctrl+Z takes the tombstone back — and pruned after 30 days at the save boundary. Three edges worth naming: * Track ids are never reused. The next id came from max(library.tracks), so deleting the highest-numbered track freed its id, and the next import would be dropped on sight by the dead id's own tombstone on every machine. * library_metadata.json is merged before library.json, since it carries the record the library merge is filtered against and rglob order is not a plan. * A merge applying a deletion never touches a music file — it drops the library entry only, and test_a_merge_never_touches_a_music_file fails the run if send_to_trash is so much as called. Applied removals grade WARNING and name the song. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Y9ZEFi4qNJ39FMiBtiAxy2 --- CLAUDE.md | 22 ++- lintunes/__init__.py | 2 +- lintunes/gui/conflict_dialog.py | 3 +- lintunes/library_manager.py | 38 +++- lintunes/models/library.py | 4 + lintunes/models/playlist.py | 10 ++ lintunes/storage/conflict_resolver.py | 126 +++++++++++-- lintunes/storage/json_storage.py | 25 +++ lintunes/tombstones.py | 89 ++++++++++ tasks-done.md | 50 ++++++ tests/test_round43.py | 4 +- tests/test_round44.py | 247 ++++++++++++++++++++++++++ 12 files changed, 594 insertions(+), 26 deletions(-) create mode 100644 lintunes/tombstones.py create mode 100644 tests/test_round44.py diff --git a/CLAUDE.md b/CLAUDE.md index 35360f2..1acf9d1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -57,7 +57,8 @@ persistence) → GUI (Qt widgets that read the manager and connect to its signal `plays/.json` per machine. Writes are atomic (`*.json.tmp` → rename). **`storage/conflict_resolver.py`** merges Syncthing `*.sync-conflict-*` files on startup: play counts take the max, edited fields - take the newest, playlist membership takes the union. Since Round 39 that + take the newest, playlist membership takes the union **except where a removal + was recorded** (Round 44, see `tombstones.py`). Since Round 39 that union is **anchor-based** (`merge_track_order`): a track only one copy has is re-inserted after the nearest track both share, not appended at the tail, so a middle insert stays in the middle. Whose order wins is decided by @@ -169,6 +170,25 @@ persistence) → GUI (Qt widgets that read the manager and connect to its signal back to `Path("Music")`, which resolved against a working directory GNOME's dash doesn't set predictably. +- **`lintunes/tombstones.py`** — why a deletion sticks. Two copies with no + common ancestor cannot tell "A added this" from "B removed it", which is why + every merge was a union — and why a song removed on one machine came back from + the other forever. So removals are *recorded*: `Playlist.track_events` + (`{track id: [when, "add"|"remove"]}`) and `Library.deleted_tracks` + (`{track id: when}`, in `library_metadata.json`). A merge takes the **newest + event per track** across both copies and applies it, so a removal beats a copy + that merely still had the song and a later re-add beats the removal; a track + with no event still merges as a union. Events are recorded in the same single + funnels as everything else — `_set_track_ids` for playlists, `_remove_tracks` + for the library — and pruned after `RETENTION_DAYS` (30) at the save boundary, + so a machine offline longer than that can resurrect something. Three rules: + **track ids are never reused** (`_highest_track_id` counts deletions, or a new + track would be dropped on sight by the dead id's own tombstone), + `library_metadata.json` is merged **before** `library.json` (it carries the + record the library merge is filtered against — `resolve_conflicts` sorts for + it), and a merge applying a deletion **never touches a music file**; it only + drops the library entry, and reports at WARNING. + - **`lintunes/sync_identity.py`** — turns a conflict filename's 7-char device token into a device name, by reading Syncthing's `config.xml` and deriving *our own* device ID from `cert.pem` (base32 of the SHA-256 of the DER cert). diff --git a/lintunes/__init__.py b/lintunes/__init__.py index 1730c34..bf9e4ae 100644 --- a/lintunes/__init__.py +++ b/lintunes/__init__.py @@ -1,3 +1,3 @@ """LinTunes — iTunes-style music library manager and player for Linux.""" -__version__ = "0.13.0" +__version__ = "0.14.0" diff --git a/lintunes/gui/conflict_dialog.py b/lintunes/gui/conflict_dialog.py index 27a7684..3047fb1 100644 --- a/lintunes/gui/conflict_dialog.py +++ b/lintunes/gui/conflict_dialog.py @@ -25,7 +25,8 @@ from lintunes.storage.conflict_resolver import ( INTRO = ("LinTunes found changes made on more than one machine and merged them " - "(play counts kept highest, newest edits win, nothing removed). Both " + "(play counts kept highest, newest edits win, and a removal made on " + "either machine is applied on both). Both " "versions were backed up first — restore them if a merge isn't what " "you wanted. The backup folder holds what-changed.txt: the same merge " "in full, every track named.") diff --git a/lintunes/library_manager.py b/lintunes/library_manager.py index 417feef..16cdc2f 100644 --- a/lintunes/library_manager.py +++ b/lintunes/library_manager.py @@ -8,7 +8,7 @@ from PyQt6.QtCore import QObject, QTimer, pyqtSignal log = logging.getLogger(__name__) from lintunes import music_folder as music_folder_mod -from lintunes import smart, tagging, trash +from lintunes import smart, tagging, tombstones, trash from lintunes.importers.file_importer import organized_destination, unique_path from lintunes.models import Library, Playlist, PlaylistType from lintunes.paths import to_relative @@ -96,13 +96,19 @@ class LibraryManager(QObject): self._own_sigs: dict[str, tuple | None] = {} self.snapshot_sync_state() - self._max_track_id = max(self.library.tracks.keys(), default=0) + self._max_track_id = self._highest_track_id() # ---- id generation ---- def new_track_id(self) -> int: return self._max_track_id + 1 + def _highest_track_id(self) -> int: + """The highest id ever used, deletions included. Ids must never be + reused: a new track handed a deleted one's id would be dropped on sight + by that id's own tombstone (and on every other machine too).""" + return max([*self.library.tracks, *self.library.deleted_tracks], default=0) + def new_persistent_id(self) -> str: while True: pid = secrets.token_hex(8).upper() @@ -336,6 +342,7 @@ class LibraryManager(QObject): playlist = self.library.playlists.get(pid) if playlist is None: return + before = list(playlist.track_ids) playlist.track_ids = list(ids) # The single funnel for every content change (add / remove / reorder / # undo). date_modified moves only where content genuinely changed: here, @@ -347,6 +354,10 @@ class LibraryManager(QObject): # conflict_resolver._playlist_newer. if touch: playlist.date_modified = _utc_now_iso() + # Record *what* changed, not just that something did. A merge has no + # common ancestor, so without this a removal is indistinguishable + # from the other copy's addition and the union hands the song back. + tombstones.record(playlist.track_events, before, playlist.track_ids) self._mark_playlist(pid, content=True) self.playlist_content_changed.emit(pid) @@ -465,8 +476,13 @@ class LibraryManager(QObject): def _remove_tracks(self, tracks: list, snapshots: dict[str, list[int]]): doomed = {t.track_id for t in tracks} + when = _utc_now_iso() for track in tracks: self.library.tracks.pop(track.track_id, None) + # Same reason as the playlist events: library.json merges as a + # union, so a deletion the other copy hasn't heard about comes + # straight back without a record that it was deliberate. + self.library.deleted_tracks[track.track_id] = when self._invalidate_artwork(track) for pid in snapshots: playlist = self.library.playlists.get(pid) @@ -482,6 +498,7 @@ class LibraryManager(QObject): def _restore_tracks(self, tracks: list, snapshots: dict[str, list[int]]): for track in tracks: self.library.tracks[track.track_id] = track + self.library.deleted_tracks.pop(track.track_id, None) self._max_track_id = max(self._max_track_id, track.track_id) for pid, ids in snapshots.items(): if pid in self.library.playlists: @@ -1016,6 +1033,15 @@ class LibraryManager(QObject): # amount equal to this machine's journal. self.play_journal.load(self.data_dir, disk) + # Deletion records are additive on both sides: ours are not on disk yet + # if we haven't flushed, and theirs are news to us. Union first, then + # apply, so a track deleted on either machine stays deleted on both. + self.library.deleted_tracks = tombstones.merge_events( + self.library.deleted_tracks, disk.deleted_tracks) + for tid in self.library.deleted_tracks: + self.library.tracks.pop(tid, None) + disk.tracks.pop(tid, None) + for tid, disk_track in disk.tracks.items(): mem_track = self.library.tracks.get(tid) if mem_track is None: @@ -1047,7 +1073,7 @@ class LibraryManager(QObject): self.library.library_settings = disk.library_settings self.refresh_music_folder() - self._max_track_id = max(self.library.tracks.keys(), default=0) + self._max_track_id = self._highest_track_id() self._recomputing = True try: @@ -1098,6 +1124,8 @@ def _reconcile_playlist(mem_pl, disk_pl): other machine was quietly undone. Same rule as conflict_resolver._playlist_newer, one layer up.""" from lintunes.storage.conflict_resolver import merge_track_order + mem_pl.track_events = tombstones.merge_events(mem_pl.track_events, + disk_pl.track_events) ours, theirs = mem_pl.date_modified, disk_pl.date_modified disk_newer = bool(theirs) and (not ours or theirs > ours) mine, other = list(mem_pl.track_ids), list(disk_pl.track_ids) @@ -1116,7 +1144,9 @@ def _reconcile_playlist(mem_pl, disk_pl): # so is what lets the other machine adopt it instead of the two trading # the same tracks back and forth. mem_pl.date_modified = _utc_now_iso() - mem_pl.track_ids = merged + # A union would hand back anything the newest event says was removed. + gone = tombstones.removed_ids(mem_pl.track_events) + mem_pl.track_ids = [tid for tid in merged if tid not in gone] def _utc_now_iso() -> str: diff --git a/lintunes/models/library.py b/lintunes/models/library.py index 784a15c..44e7353 100644 --- a/lintunes/models/library.py +++ b/lintunes/models/library.py @@ -24,6 +24,10 @@ class Library: music_folder_rel: str = "" music_folder_set_at: Optional[str] = None import_date: Optional[str] = None + # {track_id: when} for tracks deleted from the library inside the retention + # window. A merge unions two copies of library.json, so without this a track + # deleted here is handed straight back by the other machine's copy. + deleted_tracks: dict = field(default_factory=dict) # Column/sort settings for the all-tracks Library view library_settings: PlaylistSettings = field( default_factory=lambda: PlaylistSettings(sort_column="artist")) diff --git a/lintunes/models/playlist.py b/lintunes/models/playlist.py index 73c200a..47db18a 100644 --- a/lintunes/models/playlist.py +++ b/lintunes/models/playlist.py @@ -62,6 +62,10 @@ class Playlist: # for column widths or sort order — the conflict merge uses it to decide # whose order wins, and the file's mtime moves for cosmetic reasons. date_modified: Optional[str] = None + # {track_id: [when, "add"|"remove"]} for membership changes inside the + # retention window. Without it a merge is a union and a song removed on one + # machine comes back from the other forever; see lintunes/tombstones.py. + track_events: dict = field(default_factory=dict) # Set only for SMART playlists. Membership (track_ids) is derived from this # by the LibraryManager; for imported playlists track_ids also doubles as a # snapshot fallback when the criteria couldn't be fully parsed. @@ -99,6 +103,10 @@ class Playlist: d["parent_persistent_id"] = self.parent_persistent_id if self.date_modified: d["date_modified"] = self.date_modified + if self.track_events: + # JSON object keys are strings; track ids are ints everywhere else. + d["track_events"] = {str(tid): event + for tid, event in self.track_events.items()} if self.smart_criteria is not None: d["smart_criteria"] = self.smart_criteria.to_dict() return d @@ -118,6 +126,8 @@ class Playlist: settings=PlaylistSettings.from_dict(d.get("settings", {})), is_system=d.get("is_system", False), date_modified=d.get("date_modified"), + track_events={int(tid): list(event) + for tid, event in (d.get("track_events") or {}).items()}, smart_criteria=smart_criteria, ) diff --git a/lintunes/storage/conflict_resolver.py b/lintunes/storage/conflict_resolver.py index 698854c..3776e2e 100644 --- a/lintunes/storage/conflict_resolver.py +++ b/lintunes/storage/conflict_resolver.py @@ -4,7 +4,7 @@ from dataclasses import dataclass, field from datetime import datetime, timezone from pathlib import Path -from lintunes import sync_identity +from lintunes import sync_identity, tombstones from lintunes.storage.json_storage import read_json, write_json @@ -96,6 +96,11 @@ def resolve_conflicts(data_dir: Path) -> list[ConflictSummary]: conflict_files = _find_conflict_files(data_dir) if not conflict_files: return [] + # library_metadata.json carries the record of what was deleted, and + # library.json is merged against it — so it has to be merged first, whatever + # order rglob happened to hand them back in. + conflict_files.sort( + key=lambda item: 0 if item.original_path.name == "library_metadata.json" else 1) stamp = datetime.now().strftime("%Y%m%d-%H%M%S") backup_dir = data_dir / ".resolved" / stamp @@ -120,7 +125,8 @@ def resolve_conflicts(data_dir: Path) -> list[ConflictSummary]: "the other copy is in the backup."], level=WARNING) elif original_path.name == "library.json": - summary = _merge_library(original_path, conflict_path) + summary = _merge_library(original_path, conflict_path, + data_dir, label) elif original_path.name == "library_metadata.json": summary = _merge_metadata(original_path, conflict_path) elif original_path.parent.name == "playlists": @@ -238,14 +244,18 @@ def _track_labels(data_dir: Path) -> dict[str, str]: labels = {} try: for tid, track in read_json(data_dir / "library.json").items(): - title = track.get("name") or f"track {tid}" - artist = track.get("artist") or track.get("album_artist") - labels[str(tid)] = f"{artist} — {title}" if artist else title + labels[str(tid)] = _label_for(tid, track) except Exception: pass # no library yet, or an unreadable one: fall back to bare ids return labels +def _label_for(tid, track: dict) -> str: + title = track.get("name") or f"track {tid}" + artist = track.get("artist") or track.get("album_artist") + return f"{artist} — {title}" if artist else title + + class _Labeler: """Names tracks for the report, loading library.json only if asked.""" @@ -258,6 +268,13 @@ class _Labeler: self._labels = _track_labels(self._data_dir) return self._labels.get(str(tid)) or f"track {tid}" + def remember(self, tid, track: dict): + """Keep a name that is about to be deleted from library.json — the + playlist merge still has to say which song it dropped.""" + if self._labels is None: + self._labels = _track_labels(self._data_dir) + self._labels.setdefault(str(tid), _label_for(tid, track)) + def _local_time(stamp: str | None) -> str: """A naive-UTC ISO stamp as local wall-clock, the way the window shows it.""" @@ -278,14 +295,28 @@ def _utc_now_iso() -> str: return datetime.now(timezone.utc).replace(tzinfo=None).isoformat() -def _merge_library(original_path: Path, conflict_path: Path) -> ConflictSummary: +def _merge_library(original_path: Path, conflict_path: Path, + data_dir: Path = None, label=None) -> ConflictSummary: original = read_json(original_path) conflict = read_json(conflict_path) + # Deliberate deletions, from the file merged just before this one. A union + # without them means a track deleted on either machine is handed back by + # whichever copy hadn't heard yet — every time, forever. + deleted = _deleted_tracks(data_dir) added = 0 changed = 0 + removed = 0 examples: list[str] = [] + for tid in list(original): + if tid in deleted: + if label is not None: + label.remember(tid, original[tid]) + del original[tid] + removed += 1 for tid, conflict_track_data in conflict.items(): + if tid in deleted: + continue if tid not in original: original[tid] = conflict_track_data added += 1 @@ -305,11 +336,29 @@ def _merge_library(original_path: Path, conflict_path: Path) -> ConflictSummary: "(play counts kept highest, newest edits win).") if added: lines.append(f"{added} track(s) present only on the other machine were kept.") + if removed: + lines.append(f"{_count(removed)} deleted from the library stayed deleted, " + "though the other copy still listed " + f"{'it' if removed == 1 else 'them'}. " + "No file on this machine was touched.") if not lines: lines.append("No differences needed reconciling.") lines.extend(examples) - return ConflictSummary("library.json", "library", lines, - level=CHANGE if (changed or added) else INFO) + return ConflictSummary( + "library.json", "library", lines, + level=WARNING if removed else (CHANGE if (changed or added) else INFO)) + + +def _deleted_tracks(data_dir: Path | None) -> dict: + """The library's deletion record, keyed the way library.json is (strings).""" + if data_dir is None: + return {} + try: + metadata = read_json(data_dir / "library_metadata.json") + except (ValueError, OSError): + return {} + return {str(tid): when + for tid, when in (metadata.get("deleted_tracks") or {}).items()} # Every field _merge_track_fields reads or writes. If two copies of a track @@ -443,6 +492,12 @@ def _playlist_newer(original: dict, conflict: dict, "timestamps instead.") +def _events(playlist: dict) -> dict: + """A playlist file's membership events, with ids back in int form.""" + return {int(tid): event + for tid, event in (playlist.get("track_events") or {}).items()} + + def _positions(merged: list, tids, label) -> list[str]: """"Artist — Title → position 12, after “…”" for each of `tids`.""" index = {} @@ -531,6 +586,17 @@ def _merge_playlist(original_path: Path, conflict_path: Path, merged = merge_track_order(conf_ids, orig_ids) else: merged = merge_track_order(orig_ids, conf_ids) + + # A union alone can never honor a removal: with no common ancestor, "they + # removed it" and "we added it" look identical from the two lists. The + # newest recorded event per track breaks the tie — a removal beats a copy + # that merely still had the song, and a later re-add beats the removal. + events = tombstones.merge_events(_events(original), _events(conflict)) + gone = tombstones.removed_ids(events) & set(merged) + merged = [tid for tid in merged if tid not in gone] + if events: + original["track_events"] = {str(tid): event + for tid, event in events.items()} original["track_ids"] = merged renamed_from = "" @@ -567,9 +633,10 @@ def _merge_playlist(original_path: Path, conflict_path: Path, # merge made the copy in place harder to beat on the next one. write_json(original_path, original) - added = [tid for tid in conf_set - orig_set] - only_here = [tid for tid in orig_set - conf_set] - if orig_ids == conf_ids: + added = [tid for tid in conf_set - orig_set if tid not in gone] + only_here = [tid for tid in orig_set - conf_set if tid not in gone] + removed = sorted(gone) + if orig_ids == conf_ids and not gone: # The two copies held the same tracks in the same order, so they differed # only in column widths or sort order — a window resize, not an edit. # Note this is a test on the *inputs*: a merge whose result happens to @@ -610,19 +677,34 @@ def _merge_playlist(original_path: Path, conflict_path: Path, # no way to tell which. Both readings are offered rather than guessed. headline = (f"{_count(len(only_here))} here that the other copy did " f"not have {'was' if len(only_here) == 1 else 'were'} " - "kept — a merge never removes:") - it, they, them = (("it", "it", "it") if len(only_here) == 1 - else ("them", "they", "them")) - note = (f"If you added {it} here, that's all this is. If you deleted " - f"{it} on the other machine, delete {them} here too or " - f"{they} will come back on the next merge.") + "kept — nothing recorded a removal:") + it, they = ("it", "it") if len(only_here) == 1 else ("them", "they") + note = (f"Most likely you added {it} here. A removal made on the other " + f"machine carries across on its own now, so {they} would have " + "gone already — unless it happened before that machine " + f"updated, or longer ago than {tombstones.RETENTION_DAYS} days.") lines.append(headline) lines.extend(_capped(entries)) lines.append(note) detail.append(headline) detail.extend(f" • {entry}" for entry in entries) detail.append(note) - return ConflictSummary(name, "playlist", lines, level=CHANGE, detail=detail) + if removed: + entries = [label(tid) for tid in removed] + headline = (f"{_count(len(removed))} removed from this playlist on the " + "other machine " + f"{'was' if len(removed) == 1 else 'were'} removed here too:") + note = ("Undo (Ctrl+Z) does not reach a merge — add " + f"{'it' if len(removed) == 1 else 'them'} back from the library " + "if this wasn't what you wanted. No file was touched.") + lines.append(headline) + lines.extend(_capped(entries)) + lines.append(note) + detail.append(headline) + detail.extend(f" • {entry}" for entry in entries) + detail.append(note) + return ConflictSummary(name, "playlist", lines, + level=WARNING if removed else CHANGE, detail=detail) def _merge_play_journal(original_path: Path, conflict_path: Path) -> ConflictSummary: @@ -666,6 +748,7 @@ def _merge_play_journal(original_path: Path, conflict_path: Path) -> ConflictSum # What library_metadata.json actually holds, in words a person recognizes. _METADATA_LABELS = { "library_settings": "column layout", + "deleted_tracks": "record of deleted songs", "music_folder": "music folder", "music_folder_rel": "music folder", "music_folder_set_at": "music folder", @@ -701,6 +784,13 @@ def _merge_metadata(original_path: Path, conflict_path: Path) -> ConflictSummary # machine can silently revert a folder change made on the other. if before is not None and other is not None: kept = read_json(original_path) + # Deletions are additive: whichever whole copy the mtime pick kept, the + # other one's record of what was deleted is still true. + union = tombstones.merge_events(before.get("deleted_tracks") or {}, + other.get("deleted_tracks") or {}) + if union != (kept.get("deleted_tracks") or {}): + kept["deleted_tracks"] = union + write_json(original_path, kept) # Decide the folder from the two *inputs*, never from whichever copy the # mtime pick above landed on: when both files are written in the same # instant that pick is a coin flip, and it was deciding a deliberate diff --git a/lintunes/storage/json_storage.py b/lintunes/storage/json_storage.py index 86115ce..db1f9bc 100644 --- a/lintunes/storage/json_storage.py +++ b/lintunes/storage/json_storage.py @@ -1,6 +1,7 @@ import json from pathlib import Path +from lintunes import tombstones from lintunes.models import Track, Playlist, PlaylistSettings, Library from lintunes.paths import to_relative, to_absolute @@ -66,6 +67,10 @@ def save_metadata(library: Library, data_dir: Path): "track_count": len(library.tracks), "playlist_count": len(library.playlists), "library_settings": library.library_settings.to_dict(), + # Pruned at the boundary, like the derived-membership gate below: the + # in-memory dict stays honest, the file doesn't grow forever. + "deleted_tracks": {str(tid): when for tid, when + in tombstones.prune(library.deleted_tracks).items()}, } write_json(data_dir / "library_metadata.json", metadata) @@ -74,6 +79,10 @@ def save_playlist(playlist: Playlist, data_dir: Path): playlists_dir = data_dir / "playlists" playlists_dir.mkdir(parents=True, exist_ok=True) data = playlist.to_dict() + if data.get("track_events"): + data["track_events"] = tombstones.prune(data["track_events"]) + if not data["track_events"]: + del data["track_events"] if playlist.has_derived_membership: # Membership is a pure function of the criteria and the library, and # every machine recomputes it on load — storing it only bought a @@ -113,6 +122,7 @@ def load_library(data_dir: Path) -> Library: music_folder_rel = "" music_folder_set_at = None import_date = None + deleted_tracks = {} library_settings = PlaylistSettings() metadata_path = data_dir / "library_metadata.json" if metadata_path.exists(): @@ -121,6 +131,8 @@ def load_library(data_dir: Path) -> Library: music_folder_rel = metadata.get("music_folder_rel", "") music_folder_set_at = metadata.get("music_folder_set_at") import_date = metadata.get("import_date") + deleted_tracks = {int(tid): when for tid, when + in (metadata.get("deleted_tracks") or {}).items()} if "library_settings" in metadata: library_settings = PlaylistSettings.from_dict(metadata["library_settings"]) @@ -133,9 +145,22 @@ def load_library(data_dir: Path) -> Library: playlist = Playlist.from_dict(playlist_data) playlists[playlist.persistent_id] = playlist + # A deleted track can still be sitting in a library.json that synced in + # from a machine which hasn't heard about the deletion yet, and in the + # track_ids of any playlist that held it. Applying the record here covers + # every load path at once — startup and the reload_from_disk reconcile. + for tid in deleted_tracks: + tracks.pop(tid, None) + if deleted_tracks: + for playlist in playlists.values(): + if any(tid in deleted_tracks for tid in playlist.track_ids): + playlist.track_ids = [tid for tid in playlist.track_ids + if tid not in deleted_tracks] + return Library( tracks=tracks, playlists=playlists, + deleted_tracks=deleted_tracks, music_folder=music_folder, music_folder_rel=music_folder_rel, music_folder_set_at=music_folder_set_at, diff --git a/lintunes/tombstones.py b/lintunes/tombstones.py new file mode 100644 index 0000000..83cbcf9 --- /dev/null +++ b/lintunes/tombstones.py @@ -0,0 +1,89 @@ +"""Removals that survive a merge. + +Two copies of a playlist and no common ancestor cannot tell "A added this song" +apart from "B deleted it" — which is why every merge until now was a union, and +why a song removed on one machine came back from the other on the next sync. +A deletion you cannot make stick is not a deletion. + +So the removal is recorded rather than inferred: each playlist keeps a small +``track_events`` map of ``{track id: (when, "add"|"remove")}``, and the library +keeps ``deleted_tracks``. A merge takes the *newest* event per track across both +copies and applies it, so a removal beats a copy that simply still had the song, +and a deliberate re-add afterwards beats the removal. Tracks nobody has touched +have no event and keep the old union behavior. + +Events are pruned after ``RETENTION_DAYS``: by then every machine has seen them, +and keeping them forever would grow the file for nothing. The trade is that a +machine offline longer than that can resurrect a song it never learned was +deleted — which is the same trade every sync system makes, and the merge window +says out loud when a removal is applied. +""" + +from datetime import datetime, timedelta, timezone + +ADD = "add" +REMOVE = "remove" + +# How long a membership event is kept. Long enough that a laptop shut in a bag +# for a month still learns about the deletion; short enough that the map stays +# a handful of entries. +RETENTION_DAYS = 30 + + +def now_iso() -> str: + """Naive UTC ISO — the one stamp format every machine writes, so the strings + sort correctly against each other without parsing.""" + return datetime.now(timezone.utc).replace(tzinfo=None).isoformat() + + +def _expiry(now: str | None = None) -> str: + base = datetime.fromisoformat(now) if now else datetime.now(timezone.utc).replace(tzinfo=None) + return (base - timedelta(days=RETENTION_DAYS)).isoformat() + + +def prune(events: dict, now: str | None = None) -> dict: + """Drop events older than the retention window.""" + cutoff = _expiry(now) + return {key: value for key, value in events.items() + if _stamp(value) and _stamp(value) >= cutoff} + + +def _stamp(value): + """An event is (when, op); a bare stamp means a deletion in deleted_tracks.""" + if isinstance(value, (list, tuple)): + return value[0] if value else None + return value + + +def _op(value): + if isinstance(value, (list, tuple)) and len(value) > 1: + return value[1] + return REMOVE + + +def merge_events(ours: dict, theirs: dict) -> dict: + """Newest event per track wins. Works for ``track_events`` (stamp + op) and + for ``deleted_tracks`` (bare stamps) alike.""" + merged = dict(ours or {}) + for key, value in (theirs or {}).items(): + mine = merged.get(key) + if mine is None or (_stamp(value) or "") > (_stamp(mine) or ""): + merged[key] = value + return merged + + +def removed_ids(events: dict) -> set: + """Which tracks the newest event says are gone.""" + return {key for key, value in (events or {}).items() if _op(value) == REMOVE} + + +def record(events: dict, before, after) -> dict: + """Note what a membership change did, in place. ``before``/``after`` are the + two track_ids lists; a reorder produces no events at all.""" + was, now = set(before), set(after) + stamp = now_iso() + for tid in was - now: + events[tid] = [stamp, REMOVE] + for tid in now - was: + events[tid] = [stamp, ADD] + return events diff --git a/tasks-done.md b/tasks-done.md index dd34c76..264fb26 100644 --- a/tasks-done.md +++ b/tasks-done.md @@ -1,5 +1,55 @@ ## Done +### Round 44 (2026-08-27) — A removal you make sticks (v0.14.0) + +Round 43 made the merge report honest, which made its one real limitation +impossible to miss: every merge was a union, so a song removed from a playlist +on one machine was handed straight back by the other on the next sync, and a +track deleted from the library came back with it. A deletion you cannot make +stick is not a deletion. + +- [x] **Removals are recorded, not inferred.** The union was there for a real + reason — two copies and no common ancestor cannot tell "A added this" from + "B removed it" — so the missing evidence is now written down. New + `lintunes/tombstones.py`: `Playlist.track_events` holds + `{track id: [when, "add"|"remove"]}` and `Library.deleted_tracks` holds + `{track id: when}` in `library_metadata.json`. A merge takes the newest + event per track across both copies and applies it. A removal beats a copy + that merely still had the song; a deliberate re-add afterwards beats the + removal; a track nobody touched still merges as a union, which is the safe + old behavior for everything that has no evidence either way. +- [x] **Not "the newer copy wins wholesale".** That was the tempting one-line + version and it silently drops a song the other machine added while you + were removing one. `test_an_unrelated_addition_is_not_lost` pins it. +- [x] **Recorded in the existing funnels.** `_set_track_ids` diffs before/after + (so a reorder records nothing), `_remove_tracks` stamps + `deleted_tracks`, and `_restore_tracks` clears it, so Ctrl+Z on a delete + takes the tombstone back with it. +- [x] **Track ids are never reused.** The sharpest edge in the whole design: the + next id came from `max(library.tracks)`, so deleting the highest-numbered + track freed its id for the next import — and that new track would be + dropped on sight by the dead id's own tombstone, on every machine. + `_highest_track_id` counts the deletions too. +- [x] **`library_metadata.json` merges before `library.json`.** It carries the + record the library merge filters against, and `rglob` order is not a plan. + `_merge_metadata` unions the two copies' `deleted_tracks` regardless of + which whole copy the mtime pick kept. +- [x] **A merge applying a deletion never touches a music file.** It drops the + library entry only — the file was already trashed on the machine where the + delete happened, and the music folder is its own Syncthing share. + `test_a_merge_never_touches_a_music_file` fails the run if `send_to_trash` + is so much as called. +- [x] **Reported as a warning, with the song named.** An applied removal grades + WARNING so it opens the merge window, names each track, and says Ctrl+Z + does not reach a merge. `_Labeler.remember` keeps the name of a track + library.json is about to lose, so the playlist merge can still say which + song it dropped rather than "track 4". +- [x] **Events expire after 30 days** (`RETENTION_DAYS`), pruned at the save + boundary like the derived-membership gate. The trade is stated in the + module docstring: a machine offline longer than that can resurrect a song + it never learned was deleted. + + ### Round 43 (2026-08-27) — The merge report says who, what, and where (v0.13.0) The merge window kept saying `Order kept from this machine (most recently diff --git a/tests/test_round43.py b/tests/test_round43.py index 9f3ff3a..7aff598 100644 --- a/tests/test_round43.py +++ b/tests/test_round43.py @@ -242,7 +242,9 @@ class TestNamingTheSongs: body = "\n".join(summaries[0].lines) assert "the other copy did not have" in body assert "Pola — Abeille" in body - assert "delete it here too" in body + # Round 44: a removal now carries across on its own, so the note no + # longer tells you to delete it here by hand. + assert "Most likely you added it here" in body def test_the_window_caps_the_list_and_the_backup_holds_all_of_it(self, tmp_path): from lintunes.models import Track diff --git a/tests/test_round44.py b/tests/test_round44.py new file mode 100644 index 0000000..4830cc9 --- /dev/null +++ b/tests/test_round44.py @@ -0,0 +1,247 @@ +"""Round 44: a removal you make sticks. + +Round 43 made the merge report honest, and in doing so made its one real +limitation impossible to miss: every merge was a *union*, so a song removed from +a playlist on one machine was handed straight back by the other one on the next +sync, forever. Same for a track deleted from the library. A deletion you cannot +make stick is not a deletion. + +The reason it was a union is real — two copies and no common ancestor cannot +tell "A added this" from "B removed it". So the removal is now *recorded* +(`lintunes/tombstones.py`): a playlist keeps `track_events`, the library keeps +`deleted_tracks`, and a merge applies the newest event per track. A removal +beats a copy that merely still had the song; a deliberate re-add afterwards +beats the removal; a track nobody has touched still merges as a union. +""" +import json +import shutil + +import pytest + +from lintunes import tombstones +from lintunes.library_manager import LibraryManager +from lintunes.models import Library, Playlist, PlaylistType, Track +from lintunes.storage import json_storage, conflict_resolver +from lintunes.storage.conflict_resolver import WARNING, CHANGE + + +PID = "AAAA1111" + + +def _machine(tmp_path, name, ids=(1, 2, 3)): + """A data dir with three tracks and one playlist holding them.""" + root = tmp_path / name + lib = Library() + for tid in (1, 2, 3, 4): + lib.tracks[tid] = Track(track_id=tid, name=f"Song {tid}", artist="A") + lib.playlists[PID] = Playlist(persistent_id=PID, name="mix", + playlist_type=PlaylistType.REGULAR, + track_ids=list(ids)) + json_storage.save_library(lib, root) + return root + + +def _manager(root, qapp): + return LibraryManager(json_storage.load_library(root), root) + + +def _sync_as_conflict(src, dest, rel="playlists/AAAA1111.json"): + """What Syncthing does when both copies changed: src's version lands beside + dest's own, under a conflict name.""" + source = src / rel + name = f"{source.stem}.sync-conflict-20260827-120000-CMFNCIX{source.suffix}" + shutil.copy2(source, dest / rel.rsplit("/", 1)[0] / name) + # library_metadata.json rides along — it carries the deletion record. + meta = src / "library_metadata.json" + if meta.exists(): + shutil.copy2(meta, dest / "library_metadata.sync-conflict-" + "20260827-120000-CMFNCIX.json") + return conflict_resolver.resolve_conflicts(dest) + + +def _ids(root, pid=PID): + return json.loads((root / "playlists" / f"{pid}.json").read_text())["track_ids"] + + +class TestARemovalSticks: + def test_it_survives_the_round_trip(self, tmp_path, qapp): + """The whole complaint: remove a song here, go to the other machine, and + it is back. Twice over — the second sync used to re-add it again.""" + a, b = _machine(tmp_path, "a"), _machine(tmp_path, "b") + manager = _manager(a, qapp) + manager.remove_tracks_from_playlist(PID, [1]) # row index 1 == track 2 + manager.flush() + assert _ids(a) == [1, 3] + + _sync_as_conflict(a, b) + assert _ids(b) == [1, 3] + + # ...and B's copy, now travelling back, must not resurrect it either. + _sync_as_conflict(b, a) + assert _ids(a) == [1, 3] + + def test_a_deliberate_re_add_beats_the_removal(self, tmp_path, qapp): + a, b = _machine(tmp_path, "a"), _machine(tmp_path, "b") + manager_a = _manager(a, qapp) + manager_a.remove_tracks_from_playlist(PID, [1]) + manager_a.flush() + _sync_as_conflict(a, b) + assert _ids(b) == [1, 3] + + manager_b = _manager(b, qapp) + manager_b.add_tracks_to_playlist(PID, [2]) + manager_b.flush() + + _sync_as_conflict(b, a) + assert 2 in _ids(a) + + def test_an_unrelated_addition_is_not_lost(self, tmp_path, qapp): + """The reason this isn't just "the newer copy wins wholesale": that would + drop a song the other machine added while we were removing one.""" + a, b = _machine(tmp_path, "a"), _machine(tmp_path, "b") + manager_a = _manager(a, qapp) + manager_a.remove_tracks_from_playlist(PID, [1]) + manager_a.flush() + + manager_b = _manager(b, qapp) + manager_b.add_tracks_to_playlist(PID, [4]) + manager_b.flush() + + _sync_as_conflict(a, b) + + assert _ids(b) == [1, 3, 4] + + def test_a_track_nobody_touched_still_merges_as_a_union(self, tmp_path, qapp): + """No event means no evidence, and the old behavior is still the safe + one: keep it.""" + a = _machine(tmp_path, "a", ids=[1, 2]) + b = _machine(tmp_path, "b", ids=[1, 2, 3]) + + _sync_as_conflict(b, a) + + assert set(_ids(a)) == {1, 2, 3} + + def test_the_removal_is_reported_as_a_warning(self, tmp_path, qapp): + a, b = _machine(tmp_path, "a"), _machine(tmp_path, "b") + manager = _manager(a, qapp) + manager.remove_tracks_from_playlist(PID, [1]) + manager.flush() + + summaries = _sync_as_conflict(a, b) + + playlist = [s for s in summaries if s.kind == "playlist"][0] + assert playlist.level == WARNING + body = "\n".join(playlist.lines) + assert "removed here too" in body + assert "A — Song 2" in body + assert "No file was touched" in body + + +class TestDeletingFromTheLibrary: + def test_a_deleted_track_stays_deleted_through_a_merge(self, tmp_path, qapp): + a, b = _machine(tmp_path, "a"), _machine(tmp_path, "b") + manager = _manager(a, qapp) + removed, failures = manager.delete_tracks([2], delete_files=False) + manager.flush() + assert removed == [2] and not failures + + summaries = _sync_as_conflict(a, b) + # b's library.json still lists track 2; the deletion record must win. + shutil.copy2(a / "library.json", + b / "library.sync-conflict-20260827-120000-CMFNCIX.json") + summaries += conflict_resolver.resolve_conflicts(b) + + assert "2" not in json.loads((b / "library.json").read_text()) + assert 2 not in _ids(b) + library = [s for s in summaries if s.kind == "library"] + if library: # only when library.json actually conflicted + assert library[0].level == WARNING + + def test_load_drops_a_deleted_track_and_its_playlist_rows(self, tmp_path): + """The clean-sync path: the other machine's library.json arrives with no + conflict at all, still listing the track.""" + root = _machine(tmp_path, "a") + metadata = json.loads((root / "library_metadata.json").read_text()) + metadata["deleted_tracks"] = {"2": tombstones.now_iso()} + (root / "library_metadata.json").write_text(json.dumps(metadata)) + + library = json_storage.load_library(root) + + assert 2 not in library.tracks + assert library.playlists[PID].track_ids == [1, 3] + + def test_undo_takes_the_deletion_back(self, tmp_path, qapp): + root = _machine(tmp_path, "a") + manager = _manager(root, qapp) + manager.delete_tracks([2], delete_files=False) + assert 2 in manager.library.deleted_tracks + + manager.undo_stack.undo() + + assert 2 not in manager.library.deleted_tracks + assert 2 in manager.library.tracks + + def test_a_merge_never_touches_a_music_file(self, tmp_path, qapp, monkeypatch): + """A deletion syncing in removes the library entry only. The file was + already trashed on the machine where the delete happened, and the music + folder is its own Syncthing share.""" + a, b = _machine(tmp_path, "a"), _machine(tmp_path, "b") + manager = _manager(a, qapp) + manager.delete_tracks([2], delete_files=False) + manager.flush() + + from lintunes import trash + monkeypatch.setattr(trash, "send_to_trash", lambda *args: pytest.fail( + "a merge must never touch a music file")) + _sync_as_conflict(a, b) + + +class TestTheRecordItself: + def test_a_reorder_records_nothing(self, tmp_path, qapp): + """Only membership changes are events; moving a row is not a removal.""" + root = _machine(tmp_path, "a") + manager = _manager(root, qapp) + manager.move_tracks_in_playlist(PID, [2], 0) + assert manager.library.playlists[PID].track_events == {} + + def test_events_expire_so_the_file_does_not_grow_forever(self, tmp_path): + old = "2020-01-01T00:00:00" + fresh = tombstones.now_iso() + kept = tombstones.prune({1: [old, "remove"], 2: [fresh, "remove"]}) + assert kept == {2: [fresh, "remove"]} + + def test_the_newest_event_per_track_wins_a_merge(self): + merged = tombstones.merge_events( + {1: ["2026-08-01T00:00:00", "remove"], 2: ["2026-08-01T00:00:00", "add"]}, + {1: ["2026-08-02T00:00:00", "add"]}) + assert merged[1] == ["2026-08-02T00:00:00", "add"] + assert tombstones.removed_ids(merged) == set() + + def test_they_round_trip_through_the_playlist_file(self, tmp_path): + root = tmp_path / "a" + lib = Library() + playlist = Playlist(persistent_id=PID, playlist_type=PlaylistType.REGULAR, + track_ids=[1]) + playlist.track_events = {2: [tombstones.now_iso(), "remove"]} + lib.playlists[PID] = playlist + json_storage.save_library(lib, root) + + loaded = json_storage.load_library(root) + + assert tombstones.removed_ids(loaded.playlists[PID].track_events) == {2} + + +class TestIdsAreNeverReused: + def test_a_new_track_does_not_inherit_a_deleted_ones_id(self, tmp_path, qapp): + """The nastiest failure this design could produce: hand a fresh track + the id of a deleted one and every machine drops it on sight, its own + tombstone having outlived it.""" + root = _machine(tmp_path, "a") + manager = _manager(root, qapp) + manager.delete_tracks([4], delete_files=False) # 4 was the highest id + manager.flush() + + reopened = _manager(root, qapp) + + assert reopened.new_track_id() > 4 + assert 4 in reopened.library.deleted_tracks