v0.9.0: play counts that can't conflict
The merge windows kept coming because finishing a track rewrote all 15 MB of library.json. Both machines did that, so Syncthing saw two edits to one big file between syncs and produced a conflict file roughly once per song — and the merge then took max() of the two counts, discarding whichever side had played less. The .resolved/ backups showed the last nine library.json merges were ~98% play counts plus exactly one real edit, with the same ~45 tracks disagreeing every time and the count only creeping down over a day. library.json now holds only a base count. Each machine owns plays/<machine-id>.json with its own per-track totals, and the effective count is base + the sum of every journal. Only the owner writes its journal, so play data can't conflict; the journal stores totals rather than an append log, so there's no compaction step to double-count in; and a machine still on older code keeps bumping its own base, which stays additive with our journal. Two places carry the whole hazard, and both are pinned by tests: save_tracks writes journal.base_fields(), never the Track's effective count, and PlayJournal.load must be given a base-valued library — which is why reload_from_disk folds the journals onto the disk copy before reconciling. On the real 21k library: three plays write 83 bytes and leave library.json untouched. Claude-Session: https://claude.ai/code/session_01M8Mzze7shr5pZoEgKQU5NW
This commit is contained in:
@@ -1,3 +1,3 @@
|
||||
"""LinTunes — iTunes-style music library manager and player for Linux."""
|
||||
|
||||
__version__ = "0.8.0"
|
||||
__version__ = "0.9.0"
|
||||
|
||||
@@ -12,6 +12,7 @@ from lintunes.importers.file_importer import organized_destination, unique_path
|
||||
from lintunes.models import Library, Playlist, PlaylistType
|
||||
from lintunes.perf import timed
|
||||
from lintunes.storage import json_storage
|
||||
from lintunes.storage.play_journal import PlayJournal
|
||||
from lintunes.undo import Command, UndoStack
|
||||
|
||||
|
||||
@@ -53,6 +54,13 @@ class LibraryManager(QObject):
|
||||
self._dirty_playlists: set[str] = set()
|
||||
self._deleted_playlists: set[str] = set()
|
||||
|
||||
# Play counts live in per-machine journals, not library.json — see
|
||||
# storage/play_journal. Folding them in here means the rest of the app
|
||||
# keeps reading one plain track.play_count.
|
||||
self.play_journal = PlayJournal()
|
||||
self.play_journal.load(self.data_dir, self.library)
|
||||
self._dirty_journal = False
|
||||
|
||||
self._save_timer = QTimer(self)
|
||||
self._save_timer.setSingleShot(True)
|
||||
self._save_timer.setInterval(SAVE_DEBOUNCE_MS)
|
||||
@@ -680,9 +688,14 @@ class LibraryManager(QObject):
|
||||
track = self.library.tracks.get(track_id)
|
||||
if not track:
|
||||
return
|
||||
now = _utc_now_iso()
|
||||
track.play_count += 1
|
||||
track.play_date_utc = _utc_now_iso()
|
||||
self._dirty_tracks = True
|
||||
track.play_date_utc = now
|
||||
# Deliberately does NOT mark library.json dirty: a play only moves this
|
||||
# machine's journal, so finishing a track no longer rewrites 15 MB (and
|
||||
# no longer hands Syncthing a conflict once per song).
|
||||
self.play_journal.record_play(track_id, now)
|
||||
self._dirty_journal = True
|
||||
self._schedule_save()
|
||||
self.track_updated.emit(track_id)
|
||||
self._touch_smart({"play_count", "play_date_utc"})
|
||||
@@ -691,9 +704,11 @@ class LibraryManager(QObject):
|
||||
track = self.library.tracks.get(track_id)
|
||||
if not track:
|
||||
return
|
||||
now = _utc_now_iso()
|
||||
track.skip_count += 1
|
||||
track.skip_date = _utc_now_iso()
|
||||
self._dirty_tracks = True
|
||||
track.skip_date = now
|
||||
self.play_journal.record_skip(track_id, now)
|
||||
self._dirty_journal = True
|
||||
self._schedule_save()
|
||||
self.track_updated.emit(track_id)
|
||||
self._touch_smart({"skip_count", "skip_date"})
|
||||
@@ -801,10 +816,16 @@ class LibraryManager(QObject):
|
||||
|
||||
def flush(self):
|
||||
self._save_timer.stop()
|
||||
if self._dirty_journal:
|
||||
self.play_journal.save(self.data_dir)
|
||||
self._dirty_journal = False
|
||||
self._record_sig(self.data_dir / "plays"
|
||||
/ f"{self.play_journal.machine}.json")
|
||||
if self._dirty_tracks:
|
||||
with timed("flush library.json (%d tracks)",
|
||||
len(self.library.tracks)):
|
||||
json_storage.save_tracks(self.library, self.data_dir)
|
||||
json_storage.save_tracks(self.library, self.data_dir,
|
||||
self.play_journal)
|
||||
self._dirty_tracks = False
|
||||
self._record_sig(self.data_dir / "library.json")
|
||||
for pid in list(self._dirty_playlists):
|
||||
@@ -831,6 +852,9 @@ class LibraryManager(QObject):
|
||||
playlists_dir = self.data_dir / "playlists"
|
||||
if playlists_dir.exists():
|
||||
files.extend(sorted(playlists_dir.glob("*.json")))
|
||||
plays_dir = self.data_dir / "plays"
|
||||
if plays_dir.exists():
|
||||
files.extend(sorted(plays_dir.glob("*.json")))
|
||||
return files
|
||||
|
||||
@staticmethod
|
||||
@@ -877,6 +901,11 @@ class LibraryManager(QObject):
|
||||
or self._dirty_metadata)
|
||||
dirty_pids = set(self._dirty_playlists)
|
||||
disk = json_storage.load_library(self.data_dir)
|
||||
# Fold the journals onto the disk copy *before* reconciling. The tracks
|
||||
# in memory already carry effective counts, so without this the merge
|
||||
# would compare effective against base and the max() would win by an
|
||||
# amount equal to this machine's journal.
|
||||
self.play_journal.load(self.data_dir, disk)
|
||||
|
||||
for tid, disk_track in disk.tracks.items():
|
||||
mem_track = self.library.tracks.get(tid)
|
||||
|
||||
@@ -66,6 +66,8 @@ def resolve_conflicts(data_dir: Path) -> list[ConflictSummary]:
|
||||
summary = _merge_metadata(original_path, conflict_path)
|
||||
elif original_path.parent.name == "playlists":
|
||||
summary = _merge_playlist(original_path, conflict_path)
|
||||
elif original_path.parent.name == "plays":
|
||||
summary = _merge_play_journal(original_path, conflict_path)
|
||||
else:
|
||||
summary = _keep_newer(original_path, conflict_path)
|
||||
except Exception as e: # never let a bad file abort startup/live reload
|
||||
@@ -264,6 +266,37 @@ def _merge_playlist(original_path: Path, conflict_path: Path) -> ConflictSummary
|
||||
return ConflictSummary(name, "playlist", lines)
|
||||
|
||||
|
||||
def _merge_play_journal(original_path: Path, conflict_path: Path) -> ConflictSummary:
|
||||
"""Insurance only. A journal is owned by one machine, so it should never
|
||||
conflict — but if a filesystem or clock oddity produces one, falling through
|
||||
to _keep_newer would throw away a machine's entire play history. Take the
|
||||
higher total per track instead."""
|
||||
original = read_json(original_path)
|
||||
incoming = read_json(conflict_path)
|
||||
merged = dict(original)
|
||||
changed = 0
|
||||
for tid, entry in incoming.items():
|
||||
mine = merged.get(tid)
|
||||
if mine is None:
|
||||
merged[tid] = entry
|
||||
changed += 1
|
||||
continue
|
||||
combined = dict(mine)
|
||||
for count in ("plays", "skips"):
|
||||
combined[count] = max(mine.get(count, 0), entry.get(count, 0))
|
||||
for stamp in ("last_played", "last_skipped"):
|
||||
a, b = mine.get(stamp), entry.get(stamp)
|
||||
combined[stamp] = a if b is None else (b if a is None else max(a, b))
|
||||
if combined != mine:
|
||||
changed += 1
|
||||
merged[tid] = combined
|
||||
write_json(original_path, merged)
|
||||
return ConflictSummary(
|
||||
f"plays/{original_path.name}", "plays",
|
||||
[f"{changed} track(s) of play history reconciled (highest total kept)."]
|
||||
if changed else ["No differences needed reconciling."])
|
||||
|
||||
|
||||
def _merge_metadata(original_path: Path, conflict_path: Path) -> ConflictSummary:
|
||||
summary = _keep_newer(original_path, conflict_path)
|
||||
summary.file, summary.kind = "library_metadata.json", "metadata"
|
||||
|
||||
@@ -22,7 +22,7 @@ def save_library(library: Library, data_dir: Path):
|
||||
playlist_file.unlink()
|
||||
|
||||
|
||||
def save_tracks(library: Library, data_dir: Path):
|
||||
def save_tracks(library: Library, data_dir: Path, journal=None):
|
||||
data_dir.mkdir(parents=True, exist_ok=True)
|
||||
tracks_dict = {}
|
||||
for tid, track in library.tracks.items():
|
||||
@@ -31,10 +31,26 @@ def save_tracks(library: Library, data_dir: Path):
|
||||
# Store paths relative to the data dir so the library is portable
|
||||
# across machines that sync the folder to different mount points.
|
||||
data["location"] = to_relative(data["location"], data_dir)
|
||||
if journal is not None:
|
||||
# A Track carries the *effective* play count (base plus every
|
||||
# machine's journal). library.json must get the base back — writing
|
||||
# the effective value here would fold this machine's plays into the
|
||||
# base and count them twice on the next load. See play_journal.
|
||||
_write_base_play_fields(data, journal.base_fields(tid))
|
||||
tracks_dict[str(tid)] = data
|
||||
write_json(data_dir / "library.json", tracks_dict)
|
||||
|
||||
|
||||
def _write_base_play_fields(data: dict, base: dict):
|
||||
"""Put the base play fields back into a to_dict() result, keeping to_dict's
|
||||
rule that a default-valued field is omitted rather than stored."""
|
||||
for field, value in base.items():
|
||||
if value in (0, None, ""):
|
||||
data.pop(field, None)
|
||||
else:
|
||||
data[field] = value
|
||||
|
||||
|
||||
def save_metadata(library: Library, data_dir: Path):
|
||||
data_dir.mkdir(parents=True, exist_ok=True)
|
||||
metadata = {
|
||||
|
||||
@@ -0,0 +1,191 @@
|
||||
"""Per-machine play journals — the reason play counts can no longer conflict.
|
||||
|
||||
``library.json`` used to carry the live play count, so finishing a track
|
||||
rewrote all 15 MB of it. Both machines did that, so Syncthing saw two edits to
|
||||
one big file between syncs and produced a conflict file roughly once per song.
|
||||
The merge then took ``max()`` of the two counts, silently discarding whichever
|
||||
side had played less.
|
||||
|
||||
Now ``library.json`` holds only a **base** count, and each machine additionally
|
||||
owns exactly one file ``<data_dir>/plays/<machine-id>.json`` recording *its own*
|
||||
per-track totals. Effective count = base + the sum of every journal.
|
||||
|
||||
Three properties fall out of that:
|
||||
|
||||
* **No conflict is possible.** A journal file is only ever written by the
|
||||
machine that owns it, so two machines never touch the same file.
|
||||
* **No compaction step.** The journal stores running totals, not an append log,
|
||||
so it is bounded by "tracks this machine has played" and never needs folding
|
||||
back into the base — which is what would risk double-counting.
|
||||
* **Version skew is safe.** A machine still on older code keeps bumping its own
|
||||
base in ``library.json``; we never write base, so the sum stays correct. When
|
||||
it updates, its already-counted plays stay frozen in base and it starts its
|
||||
own journal.
|
||||
|
||||
The machine id lives in the config dir, deliberately *outside* the synced data
|
||||
dir — a synced id would make two machines share one journal.
|
||||
"""
|
||||
import uuid
|
||||
from pathlib import Path
|
||||
|
||||
from lintunes.config import config_path
|
||||
from lintunes.storage.json_storage import read_json, write_json
|
||||
|
||||
|
||||
JOURNAL_DIR = "plays"
|
||||
|
||||
# The four Track fields a journal contributes to. Everything else about a track
|
||||
# still lives in library.json.
|
||||
PLAY_FIELDS = ("play_count", "skip_count", "play_date_utc", "skip_date")
|
||||
|
||||
|
||||
def machine_id() -> str:
|
||||
"""This machine's stable id, created on first use. Kept next to config.json
|
||||
(not in the data dir) so Syncthing never copies it to the other machine."""
|
||||
path = config_path().parent / "machine_id"
|
||||
try:
|
||||
text = path.read_text(encoding="utf-8").strip()
|
||||
if text:
|
||||
return text
|
||||
except OSError:
|
||||
pass
|
||||
new_id = uuid.uuid4().hex[:12]
|
||||
path.parent.mkdir(parents=True, exist_ok=True)
|
||||
path.write_text(new_id + "\n", encoding="utf-8")
|
||||
return new_id
|
||||
|
||||
|
||||
def _newer(a, b):
|
||||
"""The later of two ISO-8601 stamps, either of which may be None."""
|
||||
if a is None:
|
||||
return b
|
||||
if b is None:
|
||||
return a
|
||||
return a if a >= b else b
|
||||
|
||||
|
||||
class PlayJournal:
|
||||
"""The base counts from library.json plus every machine's journal.
|
||||
|
||||
``load()`` snapshots the base off the freshly loaded Tracks and then folds
|
||||
the journals in, so the rest of the app keeps reading one plain
|
||||
``track.play_count``. Only the save path needs to know the difference —
|
||||
see ``base_fields``.
|
||||
"""
|
||||
|
||||
def __init__(self, machine=None):
|
||||
self.machine = machine or machine_id()
|
||||
self._base = {} # track_id -> {field: value} as stored in library.json
|
||||
self._own = {} # track_id -> {"plays", "skips", "last_played", "last_skipped"}
|
||||
self._others = {} # machine id -> same shape as _own
|
||||
self.dirty = False
|
||||
|
||||
# ---- loading ----
|
||||
|
||||
def load(self, data_dir: Path, library):
|
||||
"""Read every journal in the data dir and fold them into ``library``.
|
||||
|
||||
``library`` must be one whose tracks still hold the **base** counts —
|
||||
the freshly loaded library at startup, or the disk copy during a reload.
|
||||
Passing an already-folded library would promote the effective count to
|
||||
base and double-count it.
|
||||
"""
|
||||
self._base = {
|
||||
tid: {f: getattr(track, f) for f in PLAY_FIELDS}
|
||||
for tid, track in library.tracks.items()
|
||||
}
|
||||
# Our own journal is only ever written by us, so unsaved bumps in memory
|
||||
# are always more current than the file — never re-read over them.
|
||||
keep_own = self._own if self.dirty else None
|
||||
self._own = {}
|
||||
self._others = {}
|
||||
journal_dir = Path(data_dir) / JOURNAL_DIR
|
||||
if journal_dir.is_dir():
|
||||
for path in sorted(journal_dir.glob("*.json")):
|
||||
if path.name.endswith(".tmp"):
|
||||
continue
|
||||
try:
|
||||
entries = read_json(path)
|
||||
except (ValueError, OSError):
|
||||
continue # a half-synced journal must never abort startup
|
||||
entries = {int(k): v for k, v in entries.items()
|
||||
if isinstance(v, dict)}
|
||||
if path.stem == self.machine:
|
||||
self._own = entries
|
||||
else:
|
||||
self._others[path.stem] = entries
|
||||
if keep_own is not None:
|
||||
self._own = keep_own
|
||||
else:
|
||||
self.dirty = False
|
||||
self.apply(library)
|
||||
|
||||
def apply(self, library):
|
||||
"""Set every track's play fields to base + the sum of all journals."""
|
||||
for tid, track in library.tracks.items():
|
||||
added = sum(e[tid].get("plays", 0)
|
||||
for e in self._all_journals() if tid in e)
|
||||
skipped_n = sum(e[tid].get("skips", 0)
|
||||
for e in self._all_journals() if tid in e)
|
||||
base = self._base.get(tid)
|
||||
if base is None:
|
||||
# A track added after the snapshot. Its current count already
|
||||
# includes anything recorded into the journal since, so back
|
||||
# that out rather than baking it into the base.
|
||||
base = {f: getattr(track, f) for f in PLAY_FIELDS}
|
||||
base["play_count"] = (base["play_count"] or 0) - added
|
||||
base["skip_count"] = (base["skip_count"] or 0) - skipped_n
|
||||
self._base[tid] = base
|
||||
plays = (base["play_count"] or 0) + added
|
||||
skips = (base["skip_count"] or 0) + skipped_n
|
||||
played = base["play_date_utc"]
|
||||
skipped = base["skip_date"]
|
||||
for entries in self._all_journals():
|
||||
e = entries.get(tid)
|
||||
if not e:
|
||||
continue
|
||||
played = _newer(played, e.get("last_played"))
|
||||
skipped = _newer(skipped, e.get("last_skipped"))
|
||||
track.play_count = plays
|
||||
track.skip_count = skips
|
||||
track.play_date_utc = played
|
||||
track.skip_date = skipped
|
||||
|
||||
def _all_journals(self):
|
||||
yield self._own
|
||||
yield from self._others.values()
|
||||
|
||||
# ---- recording ----
|
||||
|
||||
def record_play(self, track_id: int, when: str):
|
||||
entry = self._own.setdefault(track_id, {})
|
||||
entry["plays"] = entry.get("plays", 0) + 1
|
||||
entry["last_played"] = when
|
||||
self.dirty = True
|
||||
|
||||
def record_skip(self, track_id: int, when: str):
|
||||
entry = self._own.setdefault(track_id, {})
|
||||
entry["skips"] = entry.get("skips", 0) + 1
|
||||
entry["last_skipped"] = when
|
||||
self.dirty = True
|
||||
|
||||
# ---- saving ----
|
||||
|
||||
def base_fields(self, track_id: int) -> dict:
|
||||
"""What library.json should hold for this track. Writing the *effective*
|
||||
count here instead would double-count it on the next load — this is the
|
||||
one place in the change where that can go wrong."""
|
||||
base = self._base.get(track_id)
|
||||
if base is None: # never seen at load time — nothing to preserve
|
||||
return {"play_count": 0, "skip_count": 0,
|
||||
"play_date_utc": None, "skip_date": None}
|
||||
return dict(base)
|
||||
|
||||
def save(self, data_dir: Path):
|
||||
"""Write this machine's journal only. Nothing else ever writes this file,
|
||||
so it cannot conflict."""
|
||||
journal_dir = Path(data_dir) / JOURNAL_DIR
|
||||
journal_dir.mkdir(parents=True, exist_ok=True)
|
||||
write_json(journal_dir / f"{self.machine}.json",
|
||||
{str(k): v for k, v in sorted(self._own.items())})
|
||||
self.dirty = False
|
||||
@@ -34,11 +34,15 @@ class LibrarySyncWatcher(QObject):
|
||||
the old inode, so we re-add paths after every event."""
|
||||
wanted = [self._data_dir,
|
||||
self._data_dir / "playlists",
|
||||
self._data_dir / "plays",
|
||||
self._data_dir / "library.json",
|
||||
self._data_dir / "library_metadata.json"]
|
||||
playlists_dir = self._data_dir / "playlists"
|
||||
if playlists_dir.exists():
|
||||
wanted.extend(playlists_dir.glob("*.json"))
|
||||
plays_dir = self._data_dir / "plays"
|
||||
if plays_dir.exists():
|
||||
wanted.extend(plays_dir.glob("*.json"))
|
||||
already = set(self._watcher.files()) | set(self._watcher.directories())
|
||||
to_add = [str(p) for p in wanted if p.exists() and str(p) not in already]
|
||||
if to_add:
|
||||
|
||||
Reference in New Issue
Block a user