6.2 KiB

Raw Permalink Blame History

Fix Duplicate Full-Track Download Race in Client Cache

Priority: Medium · Effort: Low · Risk: Low

Problem

M.bulkDownloadStarted is a per-track flag meant to ensure downloadAndCacheTrack runs at most once per track. But the flag is set inside downloadAndCacheTrack after an await, while multiple callers check it before that await resolves. When the last two segments complete near-simultaneously — one from the browser's buffered-range scan in ui.js, one from the explicit fetchSegment in audioCache.js — both callers observe trackCache.size >= SEGMENTS and both pass the bulkDownloadStarted guard before the first caller sets the flag. Result: the same track is fetched in full twice, wasting bandwidth and racing on the IndexedDB write.

Affected Locations

The check+call pattern appears in three places, all racy:

public/audioCache.js:99-107 — checkAndCacheComplete
public/ui.js:148-150 — buffer-segment scan
public/audioCache.js:183-187 — inside fetchSegment

And the flag is set here:

public/audioCache.js:110-112 — inside downloadAndCacheTrack, after await M.loadTrackBlob(trackId).

Root Cause

Guard-then-do where the guard write is not atomic with respect to the awaits surrounding it. JavaScript's single-threaded execution means a synchronous "check-and-set" is race-free, but here the set happens after the first await inside the function being guarded, so a second caller can enter the function and pass the check before the first caller reaches the set.

Implementation Plan

Step 1 — Move the flag set to be synchronous with the check

Refactor so the check and the set happen in the same synchronous tick, before any await. Create a single entry point:

// public/audioCache.js
M.maybeStartBulkDownload = function (trackId) {
  if (M.bulkDownloadStarted.get(trackId)) return false;   // already started
  if (M.cachedTracks.has(trackId)) return false;          // already cached
  const trackCache = M.trackCaches.get(trackId);
  if (!trackCache || trackCache.size < SEGMENTS) return false;  // not fully buffered
  // All conditions met — claim the slot synchronously:
  M.bulkDownloadStarted.set(trackId, true);
  // Fire the async work without awaiting here:
  M.downloadAndCacheTrack(trackId).catch((err) => {
    console.warn("[audioCache] bulk download failed for", trackId, err);
    M.bulkDownloadStarted.delete(trackId);   // allow a later retry
  });
  return true;
};

Then make downloadAndCacheTrack assume the flag is already set (remove the guard inside it):

M.downloadAndCacheTrack = async function (trackId) {
  // Precondition: M.bulkDownloadStarted.get(trackId) === true (set by maybeStartBulkDownload)
  const cachedUrl = await M.loadTrackBlob(trackId);
  if (cachedUrl) {                 // already cached under us
    M.cachedTracks.add(trackId);
    M.bulkDownloadStarted.delete(trackId);
    return cachedUrl;
  }
  // ... existing download + TrackStorage.set + URL.createObjectURL logic ...
  M.cachedTracks.add(trackId);
  M.bulkDownloadStarted.delete(trackId);  // clear after success
  M.renderQueue && M.renderQueue();
  M.renderLibrary && M.renderLibrary();
  return blobUrl;
};

Step 2 — Replace all three call sites

Each caller becomes a single synchronous call:

audioCache.js checkAndCacheComplete:

M.maybeStartBulkDownload(trackId);

(remove the old if (...size >= SEGMENTS) + downloadAndCacheTrack block.)

ui.js:148-150 (buffered-range scan, after a segment is marked present):

M.maybeStartBulkDownload(M.currentTrackId);

audioCache.js:183-187 (inside fetchSegment, after marking the segment):

M.maybeStartBulkDownload(trackId);

Because the check-and-claim is now synchronous, even if all three callers fire in the same tick, only the first will start the download.

Step 3 — Guard the blob-URL swap during playback

downloadAndCacheTrack swaps M.audio.src to the blob URL mid-playback (audioCache.js:213-220). After the refactor, confirm this swap still checks M.currentTrackId === trackId and that wasPlaying is captured at the moment of the swap (not earlier). Keep the existing play().catch(()=>{}).

While in this file, address the related memory leak: every URL.createObjectURL stored in M.trackBlobs is never revoked. See todo/client-cache-correctness/overview.md — that plan covers it. If doing both together, the downloadAndCacheTrack success path is the natural place to track the URL for later revocation.

Validation

Race reproduction: this is hard to trigger deterministically. Add a temporary console.log("[bulk]", trackId) at the top of the download body and another in maybeStartBulkDownload when it returns true. Play a track, let it fully buffer, and confirm exactly one "started" log and one completion per track — even when forcing the buffered-range scan and fetchSegment to both fire (e.g. seek near the end then back).
Error path: temporarily make the download throw (e.g. point the track URL at a 404) and confirm bulkDownloadStarted is cleared so a later attempt can retry, rather than permanently preventing caching.
Already-cached path: if a track is already in IndexedDB (loadTrackBlob returns a URL on the first line), confirm no full download is triggered and cachedTracks is updated.
UI: confirm the buffer bar fills, the cache indicator turns green, and queue/library re-render exactly once per completed cache (not twice).

Risk / Rollback

The refactor centralizes three near-duplicate code paths into one — net simplification.
Risk: if downloadAndCacheTrack is called from anywhere else that relied on the internal guard, that caller must be migrated to maybeStartBulkDownload. Grep for downloadAndCacheTrack( before merging.
If a download fails after the flag is set and the catch handler fails to clear it, the track becomes uncachable until reload. The .catch in maybeStartBulkDownload is the safety net — confirm it clears the flag on all rejection paths.
Rollback = revert audioCache.js and the two call-site edits in ui.js.

6.2 KiB Raw Permalink Blame History