From c0c4528a28fac58efaa965025557e2cabf7efcab Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Wed, 3 Jun 2026 20:45:17 -0700 Subject: [PATCH] PR #780 follow-ups: snapshot-based stale check + submit guard + dead code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Stale-cache check (playlistTrackCacheIsStale) compared raw track_count to the filtered/cached track list, so any playlist with local or unavailable tracks always looked 'stale' and refetched + re-mirrored on every modal open. Now it compares the upstream snapshot_id (stored at cache time in the shared fetch choke point), and returns not-stale when no snapshot is available — explicit invalidation on refresh still handles real changes. - organize_download: guard executor.submit so a refused job cleans up the batch instead of stranding it in 'analysis' (holding a limited analysis slot). - Removed the dead, deprecated, unused mirrorSpotifyPlaylistTracks. --- core/playlists/organize_download.py | 20 ++++++++++++++------ webui/static/core.js | 1 + webui/static/shared-helpers.js | 26 ++++++++++++++++++-------- 3 files changed, 33 insertions(+), 14 deletions(-) diff --git a/core/playlists/organize_download.py b/core/playlists/organize_download.py index 84ee2756..2fc8d9cb 100644 --- a/core/playlists/organize_download.py +++ b/core/playlists/organize_download.py @@ -152,12 +152,20 @@ def run_playlist_organize_download( except Exception as hist_err: logger.debug("organize download sync history: %s", hist_err) - deps.missing_download_executor.submit( - run_full_missing_tracks_process, - batch_id, - playlist_id, - tracks_json, - ) + try: + deps.missing_download_executor.submit( + run_full_missing_tracks_process, + batch_id, + playlist_id, + tracks_json, + ) + except Exception as submit_err: + # Don't leave the batch stranded in 'analysis' holding one of the limited + # analysis slots if the executor refuses the job. + logger.error("[Organize Download] Failed to submit batch %s: %s", batch_id, submit_err) + with tasks_lock: + download_batches.pop(batch_id, None) + return {'status': 'error', 'reason': f'submit failed: {submit_err}'} logger.info( "[Organize Download] Started batch %s for mirrored playlist %s (%s tracks)", batch_id, diff --git a/webui/static/core.js b/webui/static/core.js index 1461c559..ec7db039 100644 --- a/webui/static/core.js +++ b/webui/static/core.js @@ -50,6 +50,7 @@ let _lastWatchlistScanStatus = null; let _lastMediaScanStatus = null; let _lastWishlistStats = null; let playlistTrackCache = {}; // Key: playlist_id, Value: tracks array +let playlistTrackSnapshotCache = {}; // Key: playlist_id, Value: upstream snapshot_id at cache time let spotifyPlaylistsLoaded = false; let activeDownloadProcesses = {}; let sequentialSyncManager = null; diff --git a/webui/static/shared-helpers.js b/webui/static/shared-helpers.js index 72c8a4dc..e641daf9 100644 --- a/webui/static/shared-helpers.js +++ b/webui/static/shared-helpers.js @@ -1097,10 +1097,20 @@ function playlistTrackCacheIsStale(playlistId, playlist) { if (!cached) { return false; } - const expected = playlist?.track_count; - if (expected != null && Number(expected) !== cached.length) { - return true; + // Compare the upstream snapshot id (changes whenever the playlist's contents + // change). This avoids the false-positive from comparing raw track_count to + // the cached/filtered track list — playlists with local or unavailable + // tracks report a higher track_count than the id-bearing tracks we cache, + // which made every open look "stale" and refetch + re-mirror needlessly. + const currentSnap = playlist?.snapshot_id; + const cachedSnap = (typeof playlistTrackSnapshotCache !== 'undefined') + ? playlistTrackSnapshotCache[playlistId] + : undefined; + if (currentSnap && cachedSnap != null && cachedSnap !== '') { + return String(currentSnap) !== String(cachedSnap); } + // No reliable snapshot to compare (e.g. sources without snapshots) — don't + // force a refetch here; explicit invalidation on refresh handles real changes. return false; } @@ -1144,6 +1154,7 @@ function isPlaylistDownloadProcessStale(playlistId, playlistMeta) { function invalidatePlaylistTrackCache(playlistId = null) { if (playlistId) { delete playlistTrackCache[playlistId]; + if (typeof playlistTrackSnapshotCache !== 'undefined') delete playlistTrackSnapshotCache[playlistId]; if (currentModalPlaylistId === playlistId) { currentPlaylistTracks = []; } @@ -1151,6 +1162,7 @@ function invalidatePlaylistTrackCache(playlistId = null) { return; } playlistTrackCache = {}; + if (typeof playlistTrackSnapshotCache !== 'undefined') playlistTrackSnapshotCache = {}; currentPlaylistTracks = []; const ids = new Set(); if (typeof spotifyPlaylists !== 'undefined' && Array.isArray(spotifyPlaylists)) { @@ -1195,11 +1207,6 @@ function mirrorPlaylistTracksForSource(source, sourcePlaylistId, fullPlaylist) { }); } -/** @deprecated Use mirrorPlaylistTracksForSource */ -function mirrorSpotifyPlaylistTracks(playlistId, fullPlaylist) { - mirrorPlaylistTracksForSource('spotify', playlistId, fullPlaylist); -} - async function fetchAndCachePlaylistTracks(cacheKey, fetchUrl, mirrorSource, mirrorSourceId) { const response = await fetch(fetchUrl); const fullPlaylist = await response.json(); @@ -1207,6 +1214,9 @@ async function fetchAndCachePlaylistTracks(cacheKey, fetchUrl, mirrorSource, mir throw new Error(fullPlaylist.error); } playlistTrackCache[cacheKey] = fullPlaylist.tracks; + // Remember the upstream snapshot so staleness can compare against a stable + // signal instead of raw track_count vs the filtered track list. + playlistTrackSnapshotCache[cacheKey] = fullPlaylist.snapshot_id || ''; mirrorPlaylistTracksForSource(mirrorSource, mirrorSourceId, fullPlaylist); return fullPlaylist; }