112 lines
6.2 KiB
Markdown
112 lines
6.2 KiB
Markdown
# 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:
|
|
|
|
```js
|
|
// 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):
|
|
|
|
```js
|
|
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`:**
|
|
```js
|
|
M.maybeStartBulkDownload(trackId);
|
|
```
|
|
(remove the old `if (...size >= SEGMENTS)` + `downloadAndCacheTrack` block.)
|
|
|
|
**`ui.js:148-150`** (buffered-range scan, after a segment is marked present):
|
|
```js
|
|
M.maybeStartBulkDownload(M.currentTrackId);
|
|
```
|
|
|
|
**`audioCache.js:183-187`** (inside `fetchSegment`, after marking the segment):
|
|
```js
|
|
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(()=>{})`.
|
|
|
|
### Step 4 — (Related, recommended in same pass) Revoke blob URLs
|
|
|
|
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`.
|