diff --git a/TASKS.md b/TASKS.md index 167aa2a..2e94149 100644 --- a/TASKS.md +++ b/TASKS.md @@ -24,6 +24,23 @@ When a round closes, move its finished items to `tasks-done.md`. # old +## Round 27 — device-sync guards (v0.2.2) + +- [x] Quit warning while a transfer is copying (closeEvent; covers X button, + Ctrl+Q, MPRIS Quit, and the self-update restart, which used to bypass + closeEvent entirely). +- [x] Machine can't sleep or shutdown/restart mid-transfer: second + SleepInhibitor (suspend+logout flags, its own reason text for GNOME's dialog). +- [x] Found & fixed: the GNOME Inhibit D-Bus call sent signed ints against a + (susu) signature, so the *playback* sleep inhibitor had silently never + worked — `_uint` marshalling repairs both. +- [x] Investigated Syncthing changes landing mid-copy: safe by design (the + SyncPlan is a click-time snapshot; the worker never reads live library + state). Hardened the one gap: a source file relocated under the queue is + skipped + dropped from the m3u + reported, instead of aborting the sync. + +Tests in `tests/test_round27.py`. Fix round → patch bump **0.2.2**. + ## Round 26 — Device menu: sync playlist to Rabbit R1 (v0.2.0) - [x] Device menu (left of Track) with "Sync Playlist to Rabbit"; grayed out on diff --git a/lintunes/__init__.py b/lintunes/__init__.py index 9d61313..e360eba 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.2.1" +__version__ = "0.2.2" diff --git a/lintunes/device_sync.py b/lintunes/device_sync.py index f1cde9b..e2a700f 100644 --- a/lintunes/device_sync.py +++ b/lintunes/device_sync.py @@ -238,12 +238,25 @@ class DeviceSyncWorker(QObject): pass # one stubborn file shouldn't kill the sync total_kib = max(plan.bytes_to_copy // 1024, 1) done = 0 + vanished: set[str] = set() for i, item in enumerate(plan.copies, start=1): label = f"{i}/{len(plan.copies)} {Path(item.dest_name).stem}" + # A source gone since planning means Syncthing moved/deleted + # it under us (e.g. a metadata edit on the other machine + # relocated the file) — skip it, don't abort the transfer. + # Once open, the fd pins the inode, so the read stays + # consistent even if the file is moved mid-copy. + try: + fsrc = open(item.src, "rb") + except FileNotFoundError: + vanished.add(item.dest_name) + done += item.size + self.progress.emit(done // 1024, total_kib, label) + continue dest = plan.dest_dir / item.dest_name # Manual chunked copy: byte-accurate progress, and no # copystat (gvfs-MTP rejects it). - with open(item.src, "rb") as fsrc, open(dest, "wb") as fdst: + with fsrc, open(dest, "wb") as fdst: while True: chunk = fsrc.read(CHUNK) if not chunk: @@ -253,13 +266,17 @@ class DeviceSyncWorker(QObject): self.progress.emit(done // 1024, total_kib, label) dest = None # Written last so an interrupted sync leaves the old m3u intact. + # Vanished tracks are left out so the m3u only lists files that + # are really there; the next sync picks up their new locations. + entries = [e for e in plan.entries if e[0] not in vanished] (plan.dest_dir / plan.m3u_name).write_text( - build_m3u(plan.entries), encoding="utf-8") + build_m3u(entries), encoding="utf-8") device_name = plan.device.name if plan.device else "device" self.finished.emit({ "playlist": plan.playlist_name, "device": device_name, - "copied": len(plan.copies), "kept": plan.kept, + "copied": len(plan.copies) - len(vanished), "kept": plan.kept, "removed": removed, "skipped": plan.skipped, + "vanished": len(vanished), }) except OSError as e: if dest is not None: diff --git a/lintunes/gui/main_window.py b/lintunes/gui/main_window.py index ee5939d..8c4d1ff 100644 --- a/lintunes/gui/main_window.py +++ b/lintunes/gui/main_window.py @@ -12,7 +12,7 @@ from PyQt6.QtGui import QAction, QKeySequence from lintunes import device_sync, mpris, tagging, theme from lintunes.art_search import AlbumArtFetcher from lintunes.eventlog import log_control -from lintunes.inhibit import SleepInhibitor +from lintunes.inhibit import INHIBIT_LOGOUT, INHIBIT_SUSPEND, SleepInhibitor from lintunes.player import Player from lintunes.importers import file_importer from lintunes.gui.album_art_dialog import AlbumArtDialog @@ -116,6 +116,12 @@ class MainWindow(QMainWindow): self.statusBar().addPermanentWidget(self._sync_progress) self._sync_worker = None self._sync_device_name = "" + # Separate instance from the playback inhibitor: pausing music + # mid-sync must not drop the sync's hold. Logout flag included so + # GNOME's shutdown/restart dialog names the transfer as the blocker. + self._sync_inhibitor = SleepInhibitor( + reason="Syncing a playlist to a device", + flags=INHIBIT_LOGOUT | INHIBIT_SUSPEND) # Wiring self._transport.play_clicked.connect(self.play_pause) @@ -158,10 +164,14 @@ class MainWindow(QMainWindow): self._update_totals() def _restart_for_update(self): - # The pull already succeeded; quit through the normal path (flushes - # the library, tears down the player) and let main() re-exec us. + # The pull already succeeded. Route through close() — not a bare + # quit() — so closeEvent runs: it flushes the library, tears down the + # player, and can veto the restart while a device sync is copying. self.restart_requested = True - QApplication.instance().quit() + if self.close(): + QApplication.instance().quit() + else: + self.restart_requested = False # ---- status bar totals ---- @@ -322,6 +332,7 @@ class MainWindow(QMainWindow): self._sync_progress.setValue(0) self._sync_label.show() self._sync_progress.show() + self._sync_inhibitor.inhibit() self._sync_worker.start() def _on_sync_progress(self, done_kib, total_kib, label): @@ -333,6 +344,7 @@ class MainWindow(QMainWindow): f"{device_sync.format_bytes(total_kib * 1024)}") def _on_sync_finished(self, summary): + self._sync_inhibitor.release() self._sync_label.hide() self._sync_progress.hide() msg = (f"Synced “{summary['playlist']}” to the {summary['device']}: " @@ -340,9 +352,13 @@ class MainWindow(QMainWindow): f"{summary['removed']} removed") if summary["skipped"]: msg += f", {summary['skipped']} skipped (no local file)" + if summary.get("vanished"): + msg += (f", {summary['vanished']} changed under us " + "(re-sync to pick them up)") self.statusBar().showMessage(msg, 8000) def _on_sync_failed(self, message): + self._sync_inhibitor.release() self._sync_label.hide() self._sync_progress.hide() QMessageBox.warning(self, "Sync failed", message) @@ -682,7 +698,26 @@ class MainWindow(QMainWindow): self._inhibitor.release() def closeEvent(self, event): + # Quitting mid-transfer abandons a partial file on the device (it + # self-heals next sync, but silently) — make it a deliberate choice. + if self._sync_worker is not None and self._sync_worker.busy(): + box = QMessageBox(self) + box.setWindowTitle("Sync in progress") + box.setText( + f"Still copying to the {self._sync_device_name}.\n" + "If you quit now, the unfinished track will be re-copied " + "on the next sync.") + quit_button = box.addButton( + "Quit Anyway", QMessageBox.ButtonRole.DestructiveRole) + keep = box.addButton( + "Keep Syncing", QMessageBox.ButtonRole.RejectRole) + box.setDefaultButton(keep) + box.exec() + if box.clickedButton() is not quit_button: + event.ignore() + return self._inhibitor.release() + self._sync_inhibitor.release() self.player.shutdown() self._manager.flush() super().closeEvent(event) diff --git a/lintunes/inhibit.py b/lintunes/inhibit.py index 7609970..19d28fe 100644 --- a/lintunes/inhibit.py +++ b/lintunes/inhibit.py @@ -1,26 +1,43 @@ -"""Keep the machine awake while music is playing. +"""Keep the machine awake while music plays or a device sync runs. Uses the GNOME SessionManager D-Bus interface (the same session bus PyQt6's -QtDBus already talks to for MPRIS) to register a *suspend* inhibitor. GNOME's -critical-battery action overrides this inhibitor, so a near-dead laptop still -suspends as expected — we only block idle/automatic suspend while playing. +QtDBus already talks to for MPRIS) to register an inhibitor. Playback holds a +*suspend* inhibitor; a device sync additionally holds *logout* (which covers +shutdown/reboot from the session — GNOME's power dialog names the holder and +its reason). GNOME's critical-battery action overrides these, so a near-dead +laptop still suspends as expected. -On non-GNOME desktops the interface won't be available; every method then -no-ops silently rather than failing. +Instances are independent (one D-Bus cookie each), so playback and sync can +hold inhibitors simultaneously without stepping on each other. On non-GNOME +desktops the interface won't be available; every method then no-ops silently +rather than failing. """ -from PyQt6.QtDBus import QDBusConnection, QDBusInterface, QDBusReply +from PyQt6.QtCore import QMetaType +from PyQt6.QtDBus import (QDBusArgument, QDBusConnection, QDBusInterface, + QDBusReply) SERVICE = "org.gnome.SessionManager" PATH = "/org/gnome/SessionManager" +INHIBIT_LOGOUT = 1 # GsmInhibitorFlag: inhibit logout/shutdown/reboot INHIBIT_SUSPEND = 4 # GsmInhibitorFlag: inhibit suspending the session/computer APP_ID = "org.lintunes.LinTunes" REASON = "Playing music" +def _uint(value: int) -> QDBusArgument: + """Marshal as D-Bus uint32. Inhibit's signature is (susu) — a plain + Python int marshals as *signed* int32 and GNOME rejects the whole call + with a type error (which QDBusReply reported merely as "invalid", so + the inhibitor silently never took effect).""" + return QDBusArgument(value, QMetaType.Type.UInt.value) + + class SleepInhibitor: - def __init__(self): + def __init__(self, reason: str = REASON, flags: int = INHIBIT_SUSPEND): self._cookie: int | None = None + self._reason = reason + self._flags = flags bus = QDBusConnection.sessionBus() self._iface = QDBusInterface(SERVICE, PATH, SERVICE, bus) @@ -28,11 +45,11 @@ class SleepInhibitor: return self._iface.isValid() def inhibit(self): - """Block automatic suspend. No-op if already inhibiting or unavailable.""" + """Take the inhibitor. No-op if already inhibiting or unavailable.""" if self._cookie is not None or not self._available(): return reply = QDBusReply(self._iface.call( - "Inhibit", APP_ID, 0, REASON, INHIBIT_SUSPEND)) + "Inhibit", APP_ID, _uint(0), self._reason, _uint(self._flags))) if reply.isValid(): self._cookie = int(reply.value()) @@ -41,5 +58,5 @@ class SleepInhibitor: if self._cookie is None: return if self._available(): - self._iface.call("Uninhibit", self._cookie) + self._iface.call("Uninhibit", _uint(self._cookie)) self._cookie = None diff --git a/tests/test_round26.py b/tests/test_round26.py index ed18e22..1e772b2 100644 --- a/tests/test_round26.py +++ b/tests/test_round26.py @@ -177,15 +177,16 @@ class TestWorker: assert seen[-1][0] == seen[-1][1] == 2 * device_sync.CHUNK // 1024 assert "1/1" in seen[-1][2] - def test_source_vanishing_reports_failure(self, qapp, tmp_path): + def test_device_side_error_reports_failure(self, qapp, tmp_path): + # A file squatting on the dest-dir path stands in for a device-side + # write error (unplug mid-sync). Vanished *sources* are tolerated + # (round 27); device errors still abort. a = _audio(tmp_path, "a.mp3") plan = plan_sync("Mix", [_track(1, "A", "X", a)], tmp_path / "Music") - a.unlink() # unplugged/deleted between plan and run + (tmp_path / "Music").mkdir() + (tmp_path / "Music" / "Mix").write_bytes(b"not a dir") result = self._synced(qapp, plan) assert "Sync failed" in result["error"] - # No half-copied file left behind at full-looking size. - leftover = plan.dest_dir / "X - A.mp3" - assert not leftover.exists() or leftover.stat().st_size != plan.copies[0].size def test_empty_playlist_clears_folder(self, qapp, tmp_path): dest = tmp_path / "Music" / "Mix" diff --git a/tests/test_round27.py b/tests/test_round27.py new file mode 100644 index 0000000..e428670 --- /dev/null +++ b/tests/test_round27.py @@ -0,0 +1,140 @@ +"""Round 27: device-sync guards. + +A running transfer now (a) holds a GNOME suspend+logout inhibitor so the +machine won't sleep or restart under it, (b) is protected by a quit-warning +in MainWindow.closeEvent (not testable headless — no MainWindow tests exist), +and (c) tolerates a source file vanishing mid-queue: Syncthing applying a +remote metadata edit can relocate a music file between planning and copying, +so the worker skips it, drops it from the m3u, and reports it instead of +aborting the whole transfer. Device-side errors still abort. +""" + +from lintunes.device_sync import DeviceSyncWorker, plan_sync +from lintunes.inhibit import INHIBIT_LOGOUT, INHIBIT_SUSPEND, SleepInhibitor +from lintunes.models import Track + + +def _track(tid, name, artist, path): + return Track(track_id=tid, name=name, artist=artist, + location=str(path), total_time=180_000) + + +def _audio(tmp_path, filename, data=b"x" * 100): + path = tmp_path / "local" / filename + path.parent.mkdir(parents=True, exist_ok=True) + path.write_bytes(data) + return path + + +class _FakeIface: + """Stands in for the QDBusInterface: records calls, hands out cookies.""" + + def __init__(self): + self.calls = [] + self._next_cookie = 41 + + def isValid(self): + return True + + def call(self, method, *args): + from PyQt6.QtDBus import QDBusMessage + self.calls.append((method, *args)) + stub = QDBusMessage.createMethodCall("a.b", "/", "a.b", method) + if method == "Inhibit": + self._next_cookie += 1 + return stub.createReply([self._next_cookie]) + return stub.createReply([]) + + +class TestSleepInhibitor: + def _patched(self, monkeypatch, **kwargs): + # _uint wraps ints in write-only QDBusArguments; identity here so the + # fake can record and assert on the raw values. + from lintunes import inhibit as inhibit_module + monkeypatch.setattr(inhibit_module, "_uint", lambda v: v) + inhibitor = SleepInhibitor(**kwargs) + inhibitor._iface = _FakeIface() + return inhibitor + + def test_custom_reason_and_flags_reach_dbus(self, qapp, monkeypatch): + inhibitor = self._patched( + monkeypatch, reason="Syncing a playlist to a device", + flags=INHIBIT_LOGOUT | INHIBIT_SUSPEND) + inhibitor.inhibit() + method, app_id, xid, reason, flags = inhibitor._iface.calls[0] + assert method == "Inhibit" + assert reason == "Syncing a playlist to a device" + assert flags == 5 # logout (1) + suspend (4) + + def test_defaults_unchanged_for_playback(self, qapp, monkeypatch): + inhibitor = self._patched(monkeypatch) + inhibitor.inhibit() + method, app_id, xid, reason, flags = inhibitor._iface.calls[0] + assert reason == "Playing music" + assert flags == INHIBIT_SUSPEND == 4 + + def test_release_uninhibits_with_cookie(self, qapp, monkeypatch): + inhibitor = self._patched(monkeypatch) + inhibitor.inhibit() + cookie = inhibitor._cookie + inhibitor.release() + assert ("Uninhibit", cookie) in inhibitor._iface.calls + assert inhibitor._cookie is None + + def test_second_inhibit_while_held_is_noop(self, qapp, monkeypatch): + inhibitor = self._patched(monkeypatch) + inhibitor.inhibit() + inhibitor.inhibit() + assert len([c for c in inhibitor._iface.calls + if c[0] == "Inhibit"]) == 1 + + def test_release_without_hold_is_noop(self, qapp, monkeypatch): + inhibitor = self._patched(monkeypatch) + inhibitor.release() + assert inhibitor._iface.calls == [] + + +class TestVanishedSource: + def _run(self, plan): + worker = DeviceSyncWorker(plan) + results = {} + worker.finished.connect(lambda s: results.update(s)) + worker.failed.connect(lambda m: results.update(error=m)) + worker._run() + return results + + def test_vanished_source_is_skipped_not_fatal(self, qapp, tmp_path): + a = _audio(tmp_path, "a.mp3", b"a" * 50) + b = _audio(tmp_path, "b.mp3", b"b" * 70) + tracks = [_track(1, "A", "X", a), _track(2, "B", "Y", b)] + plan = plan_sync("Mix", tracks, tmp_path / "Music") + a.unlink() # Syncthing relocated it between plan and copy + + summary = self._run(plan) + + assert "error" not in summary + assert summary["vanished"] == 1 + assert summary["copied"] == 1 + assert not (plan.dest_dir / "X - A.mp3").exists() + assert (plan.dest_dir / "Y - B.mp3").read_bytes() == b"b" * 70 + + def test_vanished_track_left_out_of_m3u(self, qapp, tmp_path): + a = _audio(tmp_path, "a.mp3") + b = _audio(tmp_path, "b.mp3") + tracks = [_track(1, "A", "X", a), _track(2, "B", "Y", b)] + plan = plan_sync("Mix", tracks, tmp_path / "Music") + a.unlink() + + self._run(plan) + + m3u = (plan.dest_dir / "Mix.m3u").read_text(encoding="utf-8") + assert "X - A.mp3" not in m3u + assert "Y - B.mp3" in m3u + + def test_all_sources_vanished_still_finishes(self, qapp, tmp_path): + a = _audio(tmp_path, "a.mp3") + plan = plan_sync("Mix", [_track(1, "A", "X", a)], tmp_path / "Music") + a.unlink() + summary = self._run(plan) + assert summary["vanished"] == 1 and summary["copied"] == 0 + assert (plan.dest_dir / "Mix.m3u").read_text() == "#EXTM3U\n"