From 87e5e1fa23989b8cc58c6c7319efcddecfbfbbaf Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Thu, 11 Jun 2026 12:55:55 -0700 Subject: [PATCH] #702: make mirrored-playlist cancel/reset/delete idempotent (un-wedge LB weekly sync) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Root cause (from the reporter's app.log): a ListenBrainz weekly playlist syncs through the in-memory youtube_playlist_states discovery machine. When that live state is lost — a Docker restart, or the discovery process ending while the user waits for the media-server scan — the DB discover-download snapshot survives but the live state is gone. Every recovery action (Cancel/Reset/Delete) then hit `key not in states` and returned 404 "YouTube playlist not found" (hence the confusing "Youtube" on a ListenBrainz playlist), leaving the playlist permanently wedged with no way to dismiss or re-sync. Works for the maintainer because a single session with no restart keeps the live state alive. Fix — these are cleanup ops, so "the thing is already gone" is SUCCESS, not 404: - cancel_sync core (shared by YouTube + ListenBrainz + Tidal/Deezer/Qobuz/...) → missing key returns idempotent success. - reset_youtube_playlist / delete_youtube_playlist → same. The playlist becomes recoverable: Cancel/Reset clears the dead state and the user re-syncs fresh. Tests: cancel_sync core (missing key = idempotent 200 not 404; present key still cancels + clears the worker + reverts phase); endpoint-level idempotency for cancel/reset/delete; updated the old test that locked the 404 wedge. 834 sync/ discovery tests green. --- core/discovery/endpoints.py | 8 +++- .../discovery/test_cancel_sync_idempotent.py | 45 +++++++++++++++++++ tests/discovery/test_discovery_endpoints.py | 10 +++-- .../test_youtube_sync_idempotent_endpoints.py | 39 ++++++++++++++++ web_server.py | 9 +++- 5 files changed, 105 insertions(+), 6 deletions(-) create mode 100644 tests/discovery/test_cancel_sync_idempotent.py create mode 100644 tests/test_youtube_sync_idempotent_endpoints.py diff --git a/core/discovery/endpoints.py b/core/discovery/endpoints.py index 2d0d2042..54fceb3c 100644 --- a/core/discovery/endpoints.py +++ b/core/discovery/endpoints.py @@ -104,7 +104,13 @@ def cancel_sync( """ try: if key not in states: - return {"error": not_found_message}, 404 + # Idempotent: the live discovery state is gone (a restart wiped the + # in-memory state, or it was already cancelled). Cancelling a sync + # that isn't running is a no-op SUCCESS, not a 404 — otherwise a + # mirrored playlist (e.g. a ListenBrainz weekly) whose state vanished + # is permanently wedged with "playlist not found" and can never be + # re-synced or dismissed (#702). + return {"success": True, "message": f"No active {label} sync to cancel"}, 200 state = states[key] state['last_accessed'] = time.time() diff --git a/tests/discovery/test_cancel_sync_idempotent.py b/tests/discovery/test_cancel_sync_idempotent.py new file mode 100644 index 00000000..f044ca9d --- /dev/null +++ b/tests/discovery/test_cancel_sync_idempotent.py @@ -0,0 +1,45 @@ +"""#702: a mirrored playlist (e.g. a ListenBrainz weekly) whose in-memory +discovery state was wiped by a restart must still cancel/reset cleanly instead of +404-ing into a permanent wedge. cancel_sync is the shared core for YouTube + +ListenBrainz cancel, so its idempotency is the fix.""" + +from __future__ import annotations + +import threading + +from core.discovery.endpoints import cancel_sync + + +def _lock(): + return threading.Lock() + + +def test_cancel_missing_key_is_idempotent_success_not_404(): + body, code = cancel_sync( + {}, 'state_was_wiped', label='YouTube', + not_found_message='YouTube playlist not found', + sync_lock=_lock(), sync_states={}, active_sync_workers={}) + assert code == 200 + assert body.get('success') is True + assert 'not found' not in str(body).lower() # the wedge message must be gone + + +def test_cancel_present_key_cancels_and_clears_worker(): + states = {'h': {'phase': 'syncing', 'sync_playlist_id': 'sp1'}} + sync_states, workers = {}, {'sp1': 'worker'} + body, code = cancel_sync( + states, 'h', label='YouTube', not_found_message='x', + sync_lock=_lock(), sync_states=sync_states, active_sync_workers=workers) + assert code == 200 and body['success'] is True + assert sync_states['sp1'] == {'status': 'cancelled'} + assert 'sp1' not in workers + assert states['h']['phase'] == 'discovered' + assert states['h']['sync_playlist_id'] is None + + +def test_cancel_present_with_no_active_sync_still_succeeds(): + states = {'h': {'phase': 'discovered', 'sync_playlist_id': None}} + body, code = cancel_sync( + states, 'h', label='YouTube', not_found_message='x', + sync_lock=_lock(), sync_states={}, active_sync_workers={}) + assert code == 200 and body['success'] is True diff --git a/tests/discovery/test_discovery_endpoints.py b/tests/discovery/test_discovery_endpoints.py index 5aa80d27..d78dd224 100644 --- a/tests/discovery/test_discovery_endpoints.py +++ b/tests/discovery/test_discovery_endpoints.py @@ -157,11 +157,15 @@ def test_spotify_data_takes_precedence_over_auto_fields(): # cancel_sync # --------------------------------------------------------------------------- -def test_cancel_sync_not_found_returns_404(): +def test_cancel_sync_missing_state_is_idempotent_success(): + # #702: a sync whose in-memory state was wiped (e.g. a restart) must cancel + # cleanly — cancelling a sync that isn't running is a no-op SUCCESS, not a + # 404 that permanently wedges the playlist. body, code = cancel_sync({}, 'missing', label='Tidal', not_found_message='Tidal playlist not found', **_cancel_infra()) - assert code == 404 - assert body == {"error": "Tidal playlist not found"} + assert code == 200 + assert body.get('success') is True + assert 'not found' not in str(body).lower() def test_cancel_sync_cancels_active_worker_and_reverts_state(): diff --git a/tests/test_youtube_sync_idempotent_endpoints.py b/tests/test_youtube_sync_idempotent_endpoints.py new file mode 100644 index 00000000..509585e3 --- /dev/null +++ b/tests/test_youtube_sync_idempotent_endpoints.py @@ -0,0 +1,39 @@ +"""#702: cancel/reset/delete of a mirrored-playlist sync whose in-memory state is +gone (restart/eviction) must return success, not 404 'YouTube playlist not found' +— otherwise the playlist is permanently wedged.""" + +from __future__ import annotations + +import os +import tempfile + +import pytest + +_TMP = tempfile.mkdtemp(prefix='soulsync-testdb-ytsync-') +os.environ['DATABASE_PATH'] = os.path.join(_TMP, 'y.db') +os.environ['SOULSYNC_TEST_DB_READY'] = '1' + +web_server = pytest.importorskip('web_server') + + +@pytest.fixture +def client(): + return web_server.app.test_client() + + +_GONE = 'state_wiped_by_restart_hash' + + +def test_cancel_missing_state_is_success(client): + r = client.post(f'/api/youtube/sync/cancel/{_GONE}') + assert r.status_code == 200 and r.get_json().get('success') is True + + +def test_reset_missing_state_is_success(client): + r = client.post(f'/api/youtube/reset/{_GONE}') + assert r.status_code == 200 and r.get_json().get('success') is True + + +def test_delete_missing_state_is_success(client): + r = client.delete(f'/api/youtube/delete/{_GONE}') + assert r.status_code == 200 and r.get_json().get('success') is True diff --git a/web_server.py b/web_server.py index dc5b2e36..b575e7d0 100644 --- a/web_server.py +++ b/web_server.py @@ -24662,7 +24662,10 @@ def reset_youtube_playlist(url_hash): """Reset YouTube playlist to fresh phase (clear discovery/sync data)""" try: if url_hash not in youtube_playlist_states: - return jsonify({"error": "YouTube playlist not found"}), 404 + # Idempotent: live state gone (restart/eviction) — already "fresh". + # 404 here permanently wedges a mirrored playlist whose state vanished + # (#702); treat a reset of nothing as a success so the UI recovers. + return jsonify({"success": True, "message": "Playlist already reset"}) state = youtube_playlist_states[url_hash] @@ -24694,7 +24697,9 @@ def delete_youtube_playlist(url_hash): """Remove YouTube playlist from backend storage entirely""" try: if url_hash not in youtube_playlist_states: - return jsonify({"error": "YouTube playlist not found"}), 404 + # Idempotent: already gone (restart/eviction) — deleting nothing is a + # success, not a 404 that wedges the UI (#702). + return jsonify({"success": True, "message": "Playlist already removed"}) state = youtube_playlist_states[url_hash]