v0.14.0: 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 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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Y9ZEFi4qNJ39FMiBtiAxy2
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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
|
||||
Reference in New Issue
Block a user