156 lines
7.7 KiB
Markdown
156 lines
7.7 KiB
Markdown
# Client Cache Correctness: Revoke Blob URLs, LRU Pruning, Real `streamOnly`
|
|
|
|
Priority: **Medium** · Effort: **Medium** · Risk: **Low**
|
|
|
|
## Problem
|
|
|
|
Three independent correctness/efficiency bugs in the client caching layer:
|
|
|
|
1. **Blob URL leak.** `URL.createObjectURL(blob)` results stored in `M.trackBlobs` (`public/audioCache.js:80,127`) are never revoked. `M.trackBlobs.clear()` (`queue.js:188`) drops the references but the URLs stay live in the document until unload — a slow memory leak over long sessions with many cached tracks.
|
|
|
|
2. **`pruneCache` is expensive and non-LRU.** `public/audioCache.js:27-54` calls `TrackStorage.get(key)` for every cached track (pulling each blob out of IndexedDB) just to read `.size`, and evicts oldest-first by Map iteration order — *not* by the `cachedAt` timestamp that is actually written at `public/trackStorage.js:86`. So eviction is arbitrary, not least-recently-used.
|
|
|
|
3. **`streamOnly` doesn't do what it says.** The flag is stored (`audioCache.js:8-15`) and checked by exactly one context-menu item (`trackContainer.js:752`), but the background prefetch loop (`audioCache.js:226`) never consults it — so "stream-only mode" still fetches every segment in the background and still triggers bulk caching.
|
|
|
|
## Affected Locations
|
|
|
|
- `public/audioCache.js:27-54` — `pruneCache`
|
|
- `public/audioCache.js:80,127` — blob URL creation
|
|
- `public/audioCache.js:224-262` — prefetch loop (no `streamOnly` check)
|
|
- `public/trackStorage.js:139-155` — `getStats` (loads all blobs)
|
|
- `public/trackStorage.js:86` — `cachedAt` is written but unused
|
|
- `public/queue.js:188` — `M.trackBlobs.clear()` without revoke
|
|
|
|
## Implementation Plan
|
|
|
|
### Part A — Track and revoke blob URLs
|
|
|
|
**Step A1: Track blob URLs in a single Map with a revocation helper.**
|
|
|
|
`M.trackBlobs` is already `Map<trackId, blobUrl>`. Add:
|
|
|
|
```js
|
|
// public/audioCache.js
|
|
M.revokeTrackBlob = function (trackId) {
|
|
const url = M.trackBlobs.get(trackId);
|
|
if (url) {
|
|
URL.revokeObjectURL(url);
|
|
M.trackBlobs.delete(trackId);
|
|
}
|
|
};
|
|
```
|
|
|
|
**Step A2: Revoke on eviction and on explicit clear.**
|
|
|
|
- In `pruneCache` (after Part B), when a track is evicted from IndexedDB, also call `M.revokeTrackBlob(trackId)` so the in-memory URL is released.
|
|
- In `queue.js:188` (`clearAllCaches`), revoke every entry before clearing:
|
|
|
|
```js
|
|
for (const url of M.trackBlobs.values()) URL.revokeObjectURL(url);
|
|
M.trackBlobs.clear();
|
|
```
|
|
|
|
- When a track is removed from the library (`track_removed` handler in `channelSync.js`), revoke its blob if present (it can't be played anymore).
|
|
|
|
**Step A3: Don't double-revoke.** `URL.revokeObjectURL` is safe to call on an already-revoked URL, but guard with the Map check anyway to keep state clean.
|
|
|
|
### Part B — Make pruning LRU and cheap
|
|
|
|
**Step B1: Store size at write time, avoid loading blobs at prune time.**
|
|
|
|
Extend the IndexedDB record to carry `size` (the blob already has `.size` — store it alongside). In `trackStorage.js`:
|
|
|
|
```js
|
|
// in set(): store { filename (keyPath), blob, size, cachedAt }
|
|
const record = { filename: trackId, blob, size: blob.size, cachedAt: Date.now() };
|
|
```
|
|
|
|
Add a lightweight metadata accessor that uses a cursor and reads only `size`/`cachedAt`, not the blob:
|
|
|
|
```js
|
|
// trackStorage.js
|
|
getMetaList: function () {
|
|
return new Promise((resolve) => {
|
|
const result = [];
|
|
const tx = db.transaction(STORE, "readonly");
|
|
const store = tx.objectStore(STORE);
|
|
const req = store.openCursor();
|
|
req.onsuccess = (e) => {
|
|
const cursor = e.target.result;
|
|
if (cursor) {
|
|
const v = cursor.value;
|
|
result.push({ id: v.filename, size: v.size || 0, cachedAt: v.cachedAt || 0 });
|
|
cursor.continue();
|
|
} else {
|
|
resolve(result);
|
|
}
|
|
};
|
|
req.onerror = () => resolve([]);
|
|
});
|
|
}
|
|
```
|
|
|
|
**Step B2: Rewrite `pruneCache` to be LRU and blob-free.**
|
|
|
|
```js
|
|
// audioCache.js
|
|
async function pruneCache() {
|
|
const limit = M.cacheLimitBytes ?? 500 * 1024 * 1024; // tune as needed
|
|
const meta = await TrackStorage.getMetaList();
|
|
let total = meta.reduce((s, m) => s + m.size, 0);
|
|
if (total <= limit) return;
|
|
|
|
// Evict least-recently-used first.
|
|
meta.sort((a, b) => a.cachedAt - b.cachedAt);
|
|
for (const m of meta) {
|
|
if (total <= limit) break;
|
|
await TrackStorage.delete(m.id);
|
|
M.revokeTrackBlob(m.id);
|
|
M.cachedTracks.delete(m.id);
|
|
total -= m.size;
|
|
}
|
|
}
|
|
```
|
|
|
|
**Step B3: Update `cachedAt` on access** (optional, makes LRU reflect actual use). When a cached track is played (`loadTrackBlob` returns a URL), bump its `cachedAt`. If skipped, the policy becomes "first-in-first-out", which is still better than today's arbitrary order.
|
|
|
|
**Step B4: Rewrite `getStats`** to use `getMetaList` so the settings/stats panel doesn't load every blob either.
|
|
|
|
### Part C — Make `streamOnly` actually disable background fetching
|
|
|
|
**Step C1: Consult the flag in the prefetch loop.**
|
|
|
|
In `audioCache.js:226` (top of the prefetch loop):
|
|
|
|
```js
|
|
M.prefetchSegments = async function () {
|
|
if (prefetching) return;
|
|
if (M.streamOnly) return; // <-- new
|
|
prefetching = true;
|
|
try {
|
|
// ... existing logic ...
|
|
} finally {
|
|
prefetching = false;
|
|
}
|
|
};
|
|
```
|
|
|
|
**Step C2: Also short-circuit the buffered-range-scan-triggered bulk download when in stream-only mode**, since streaming clients don't want IndexedDB writes. Easiest: have `maybeStartBulkDownload` (see `todo/duplicate-download-race/overview.md`) check `M.streamOnly` and return `false`.
|
|
|
|
**Step C3: Decide intent for the already-playing track.** Even in stream-only mode the *current* track is being buffered by the browser's own media element — that's fine and desirable. The flag is about suppressing *additional* segment prefetching ahead of the playhead and *bulk* caching. Confirm the UX label reflects this ("Don't pre-cache ahead" rather than "stream nothing").
|
|
|
|
## Validation
|
|
|
|
- **Blob revoke**: open DevTools → Memory, take a snapshot; play and fully cache ~10 tracks; confirm `trackBlobs` grows by 10 and the document's blob URL count grows accordingly. Trigger a prune (or set a tiny `cacheLimitBytes`) and confirm the count drops as tracks are evicted. Run `clearAllCaches()` and confirm all blob URLs are revoked.
|
|
- **LRU**: with a small cache limit, cache tracks A, B, C; play A again (bump cachedAt if Part B3 is implemented); cache D. Confirm B (not A) is evicted.
|
|
- **Prune performance**: with a large cache (e.g. 200 tracks), instrument `pruneCache` and confirm it no longer reads blob payloads — wall time should drop from "loads every blob" to a cheap cursor scan.
|
|
- **streamOnly**: toggle the flag on, play a track, seek around, and confirm via DevTools Network that only the media element's own range requests fire (no `/api/tracks/:id` segment prefetches), and IndexedDB write count stays flat. Toggle off and confirm prefetching resumes.
|
|
- **Regression**: with streamOnly off, confirm normal caching behavior (green indicators, blob swap, IndexedDB entries) is unchanged.
|
|
|
|
## Risk / Rollback
|
|
|
|
- **IndexedDB schema change** (adding `size`/`cachedAt` fields to the record): old records written before this change will lack those fields. `getMetaList` must default missing fields to `0` (shown above), and `pruneCache` must handle `size === 0` gracefully (skip eviction of unknown-size entries rather than evicting everything). Consider a one-time migration that deletes records lacking `size`, or simply let them age out.
|
|
- Revoking a blob URL that is *currently* the `src` of the audio element will break playback. Only revoke blobs for tracks that are not the currently-playing track, or that have just been removed from the library. Add a guard: `if (trackId === M.currentTrackId) return;` in `revokeTrackBlob` callers that run during playback.
|
|
- streamOnly is additive; default stays off.
|
|
- Rollback per part is independent — A, B, C can land separately.
|