From 7dc24b079b1ee5aa34b756592e0f50c1fdb5e10b Mon Sep 17 00:00:00 2001 From: Jeff Crouse Date: Fri, 31 Jul 2026 07:28:26 -0400 Subject: [PATCH 1/4] =?UTF-8?q?docs(adr):=20propose=20ADR-0010=20=E2=80=94?= =?UTF-8?q?=20played=20bytes=20are=20cached,=20downloads=20are=20pinned?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The engine already downloads every track whole before playing it and deletes the file when the next one loads. ADR-0009 recorded keeping those bytes as a follow-up rather than deciding it, because bytes retained because they happened to be fetched are a different promise from a track the listener asked for. This decides that follow-up, and can be rejected without disturbing ADR-0009. The mechanism is nearly free: `ManagedTempFile(url:owned:)` already means "do not delete this one", and what the engine lacks is a say in *where* the download lands. What is not free is the policy — a cache that is never evicted is a slow download of the entire library, while ADR-0009 point 10 promises nothing is evicted automatically. Both hold only if the store keeps two classes of file with two different promises, explicitly, with a test on the boundary. The web client's one recorded bug is exactly that boundary getting blurred: "metadata present, audio absent". Measured rather than assumed, and the measurement deliberately does not settle it. 63% of all bytes ever sent for playback were re-sends — 62 GB of 99 GB — but that is a 175-day aggregate with no ordering, and the only ordered data is two days of `play_events` drawn from the days the Swift client was being developed against this library. Simulated LRU over it gives 5.5% at 0.5 GB and saturates at 13.3%. The ADR claims no hit rate; a follow-up says to re-run the simulation on a month of real use and supersede this rather than implement it if the curve flattens too low. Line citations re-verified against `NativeAudioEngine.swift` today: the four `ManagedTempFile` sites and `cleanupTempFile` had all drifted 30-40 lines since drafting. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014P9p2fvFnyiywBxGkv4gfW --- ...d-bytes-are-cached-downloads-are-pinned.md | 189 +++++++++++++++++ ...-is-cached-whole-and-refreshed-by-delta.md | 198 ++++++++++++++++++ 2 files changed, 387 insertions(+) create mode 100644 docs/decisions/ADR-0010-played-bytes-are-cached-downloads-are-pinned.md create mode 100644 docs/decisions/ADR-0011-the-library-is-cached-whole-and-refreshed-by-delta.md diff --git a/docs/decisions/ADR-0010-played-bytes-are-cached-downloads-are-pinned.md b/docs/decisions/ADR-0010-played-bytes-are-cached-downloads-are-pinned.md new file mode 100644 index 00000000..1ea220cf --- /dev/null +++ b/docs/decisions/ADR-0010-played-bytes-are-cached-downloads-are-pinned.md @@ -0,0 +1,189 @@ +# ADR-0010: Played Bytes Are Cached, Downloads Are Pinned + +Status: proposed + +Date: 2026-07-31 + +Extends [ADR-0009](ADR-0009-offline-downloads-are-background-transfers.md) + +## Context + +[ADR-0009](ADR-0009-offline-downloads-are-background-transfers.md) established that the engine +already downloads every track whole before playing it, and deletes the file when the next track +loads. It recorded keeping those bytes as a follow-up rather than deciding it, because bytes retained +because they happened to be fetched are a different thing from a track the listener asked for. + +This ADR decides that follow-up. It should be read after ADR-0009 is accepted, and it can be rejected +without disturbing it — nothing in ADR-0009 depends on this. + +**The mechanism is close to free, and smaller than the follow-up in ADR-0009 implied.** The engine +already distinguishes files it must delete from files it must leave alone: +`ManagedTempFile(url:owned:)` is constructed with `owned: true` on the two network paths +(`Sources/FamiliarKit/NativeAudioEngine.swift:814` for `load`, `:1274` for `preloadNext`) and +`owned: false` on the two local paths (`:840`, `:1317`), and `cleanupTempFile()` (`:1893`) honours the +flag. "Do not delete this one" is an existing concept. + +What the engine lacks is a say in *where* the download lands: both network paths hardcode +`NSTemporaryDirectory()`. The right seam is therefore a destination provider — the engine asks for a +URL and an ownership flag, defaulting to today's tmp-and-owned behaviour — rather than a delegate +callback after the fact. `NativeAudioEngineDelegate` (`:144-160`) is entirely event notifications +(`audioEngineDidFinishPlaying`, `audioEngineDidUpdateAnalysis`, remote-command forwarding), so +notifying it post-hoc would mean a second move racing the cleanup it is trying to prevent. + +**The policy is not free, and this is the whole reason this is a decision rather than a patch.** A +cache that is never evicted is a slow download of the entire library, so cached files must be +evictable. ADR-0009 point 10 says nothing is evicted automatically. Both are correct only if the +store holds **two classes of file with two different promises**, and that distinction has to be +explicit or it will be got wrong. + +**The web client already has this distinction, and its one recorded bug is precisely a confusion +between the two classes.** ADR-0006 records it twice. In its Context: "`offlineScoring.ts` reads +`db.cachedTracks` — all cached metadata — rather than `db.offlineTracks`, so offline ambient can +select a track whose audio was never downloaded." And in its Implementation block, on the offline +radio path: "the manifest goes stale when a download is removed… metadata present, audio absent." +The Apple client is about to acquire the same two tiers. The lesson is not "avoid two tiers" — it is +that the boundary between them needs to be an invariant with a test on it, not a naming convention. + +The honest value of this is narrower than "offline downloads" and worth stating plainly, because it +is what an approver should weigh: a play cache does **not** add offline capability. Anyone going +somewhere without a network downloads deliberately, which ADR-0009 covers. What it buys is +time-to-first-audio on a repeat play — a whole-file fetch replaced by nothing — and a corresponding +reduction in load on the NAS, which is also the CI runner. On the local network that saves a second +or two per repeat play. Over Tailscale from outside the house, where a hi-res FLAC is tens of +megabytes, it is the difference between a pause and an instant start. + +**The size of that win was measured against the live database rather than assumed, and the +measurement does not settle it.** Two tables answer different questions and disagree, which is the +most useful thing found here: + +| Measured on the production database, 2026-07-29 | Value | +|---|---| +| Library | 26,462 tracks | +| `profile_play_history` span | 175 days (2026-02-04 → 2026-07-29) | +| Plays, distinct tracks played | 6,206 plays over 2,564 tracks — 2.42 plays per track | +| Bytes transferred to serve those plays | **99 GB**, against **37 GB** of distinct audio | +| Mean track size | 14.7 MB | +| Share of plays in the top 10 / 100 / 500 tracks | 5.4% / 20.9% / 49.4% | +| `play_events` span | **2 days** (2026-07-27 → 2026-07-29), 549 events | + +So **63% of all bytes the server has ever sent for playback were re-sends of a file it had already +sent** — 62 GB of 99 GB. That is the strongest available argument for caching, and it is a lifetime +aggregate. + +It does not follow that a bounded cache recovers much of it, and that is the trap this table exists +to avoid walking into. A 175-day aggregate has no ordering, so it cannot distinguish a track played +five times this week from one played once every five weeks; an LRU cache sized for a phone captures +the first and evicts its way straight past the second. The distribution is also flatter than "the +same few songs over and over" implies — the top 10 tracks are 5.4% of plays, and it takes 500 tracks +(≈7.3 GB at the mean size) to reach half. + +The only ordered data is `play_events`, and it is **two days long**, because ADR-0004 landed on +2026-07-27. Simulating LRU over its real sequence gives a 5.5% hit rate at a 0.5 GB budget, 10.7% at +2 GB, and saturating at 13.3% — that last figure being not a cache limit but the total amount of +repetition present in the window. Those numbers are also drawn from the exact days the Swift client +was being developed and tested against this library, so they describe a developer exercising a player +at least as much as a listener using one. **Neither dataset supports a confident hit rate, and this +ADR does not claim one.** What is certain comes from the code rather than the data: today every play +re-downloads the whole file, and 63% of historical playback bytes were re-sends. + +This is a case where the ordering principle in `CLAUDE.md` — start anything that accumulates data +over wall-clock time as early as possible — pays out precisely as intended. ADR-0004 was sequenced +second for exactly this reason, and a few weeks of ordinary use will answer the locality question +that two days cannot. + +## Decision + +The download store holds two classes of file: **pinned** (explicitly downloaded) and **cached** +(retained because it was played). They differ in exactly one respect — whether the store may delete +them. + +1. **The engine gains a destination provider, not a delegate callback.** A closure supplied at + construction answers `(trackId, fileExtension) -> (URL, owned: Bool)`, consulted by both network + paths. Its default is `NSTemporaryDirectory()` with `owned: true`, which is today's behaviour + exactly, so the engine's shipped semantics are unchanged when nothing supplies one. + +2. **Pinned and cached files live in separate directories** — `Application Support/Downloads/` from + ADR-0009 point 2, and `Application Support/Cache/` — rather than one directory with a flag in the + index. ADR-0009 point 4 makes the filesystem the truth and the index a cache of it; a class + distinction that exists only in the index contradicts that, and reconciliation could not repair + a mislabelled entry because there would be nothing to compare it against. + +3. **Cached files are evictable, pinned files are not.** Eviction is least-recently-played first, + under a byte budget, and touches `Cache/` only. This does not reverse ADR-0009 point 10: that + promise is about tracks the listener asked for, and it continues to hold without exception. + +4. **The offline set posted to ADR-0006's manifest is the pinned set only.** Cached tracks are + invisible to ranking. Including them would tell the server to rank toward tracks that eviction + may remove, which is the stale-manifest failure ADR-0006's Implementation block records — + reintroduced on a new client, having been written down as a lesson on the old one. + +5. **Cached tracks do not appear in the downloads list.** ADR-0009 point 9 makes that list the + offline browse surface, and everything on it must still be there later. A cache entry is a + latency detail, not a promise, and surfacing it as a download would make eviction look like data + loss. + +6. **An explicit download of an already-cached track promotes the file rather than re-fetching it.** + A rename between two directories on the same volume, and the common case for "I liked this, + keep it". + +7. **Preloaded-but-skipped tracks are cached too.** `preloadNext` (`:1274`) has already paid for the + bytes by the time the listener skips past the track; discarding them is the one case where the + current behaviour is unambiguously wasteful. + +8. **The invariant is a test, not a convention:** nothing in the pinned set is evictable, nothing in + the cached set reaches the manifest or the downloads list. This is the boundary the web client got + wrong once already. + +## Alternatives Considered + +- **Do nothing; rely on explicit downloads.** Rejected, but it has a real case and is the option to + take if the second residency class is judged not worth its complexity. Explicit downloads already + cover every offline scenario; this only improves repeat plays while connected. The case for acting + is that the mechanism is a destination closure and a directory, while the benefit lands on exactly + the tracks a listener plays most. +- **Cache into `NSTemporaryDirectory()` and simply stop deleting.** Rejected. The system purges tmp + at its own discretion, so the cache would evaporate unpredictably — and worse, the same directory + would then hold both files the engine owns and files it does not, which is how a purge takes out + something the index still lists. +- **One directory, with the class recorded in the index.** Rejected, per decision point 2: it puts + the distinction somewhere reconciliation cannot verify it, in a design whose stated principle is + that the filesystem is the truth. +- **Let cached tracks count toward the offline set.** Rejected. It is superficially attractive — + more tracks rankable offline for free — and it is the exact bug ADR-0006 documents. A larger + offline pool built from bytes that may be evicted is worse than a smaller honest one. +- **Cache on a `NativeAudioEngineDelegate` callback after the load completes.** Rejected. The + protocol is event notification, and the file has already been moved into tmp by then, so this buys + a second move racing `cleanupTempFile()` for no benefit over choosing the destination up front. +- **A single unified store with no promise about permanence, evicting anything under pressure.** + Rejected. It collapses the two classes by giving up the guarantee that makes explicit downloads + worth having, which is the one thing ADR-0001 went native to deliver. + +## Consequences + +- **Positive:** Repeat plays start instantly, and the tracks that benefit are automatically the ones + played most. No user-facing feature, no setting, no intent to express. +- **Positive:** Fewer whole-file fetches against the NAS, which also hosts the CI runner and has + been observed starving music streaming during heavy CI. +- **Positive:** The wasteful case where a preloaded track is skipped and its bytes thrown away + disappears. +- **Positive:** The engine's default behaviour is untouched — the provider defaults to tmp and + owned, so a caller that supplies nothing gets exactly what ships today. +- **Tradeoff:** The store now has two classes of residency, and every future feature touching it has + to know which one it means. That cost is real and is the reason to reject this if the latency win + is judged too small. +- **Tradeoff:** Disk usage grows silently up to the cache budget without the listener asking for + anything. The budget must be visible and adjustable, or it becomes a support question. +- **Tradeoff:** "Is this track available offline?" now has two answers depending on who is asking — + the manifest and the downloads list say pinned, the player says pinned-or-cached. That is correct + but it is a distinction that invites bugs. +- **Follow-up:** Choose the cache budget **from measured locality, not from the lifetime aggregate**. + Re-run the LRU simulation over `play_events` once it covers a month or more of ordinary use rather + than two days of client development, and pick the budget where the hit-rate curve flattens. If that + curve turns out to flatten below roughly 15%, the "do nothing" alternative above is the better + answer and this ADR should be superseded rather than implemented — the measurement is the thing + that decides it, and the data to make it will exist without any further work. +- **Follow-up:** Decide whether the budget is a fixed size or a fraction of free space. A fixed + default is easier to reason about; a fraction survives moving to a smaller device. +- **Follow-up:** Decide whether eviction is least-recently-played or least-recently-*started*, once + ADR-0004's listening events are recorded natively — the store would otherwise need its own + timestamp for something the event stream already knows. diff --git a/docs/decisions/ADR-0011-the-library-is-cached-whole-and-refreshed-by-delta.md b/docs/decisions/ADR-0011-the-library-is-cached-whole-and-refreshed-by-delta.md new file mode 100644 index 00000000..2d6f038f --- /dev/null +++ b/docs/decisions/ADR-0011-the-library-is-cached-whole-and-refreshed-by-delta.md @@ -0,0 +1,198 @@ +# ADR-0011: The Library Is Cached Whole, and Refreshed by Delta + +Status: proposed + +Date: 2026-07-29 + +Extends [ADR-0009](ADR-0009-offline-downloads-are-background-transfers.md) + +## Context + +[ADR-0009](ADR-0009-offline-downloads-are-background-transfers.md) point 9 limited offline browse to +the downloads list and explicitly deferred this decision. Deferring it leaves downloads as half a +feature: `App/Shared/LibraryStore.swift` pages from the server on every view, so with no network the +Apple client can play what it has downloaded but cannot navigate to it the way it was found. ADR-0009 +also named this as the decision that settles whether its `Codable` index becomes SQLite, which is +why it wants deciding before that index acquires dependents. + +Everything below was measured against the live library on 2026-07-29 rather than estimated. + +| Fact | Value | +|---|---| +| Tracks | 26,396 active (26,462 rows, 66 inactive) | +| Full `TrackResponse` | 496 B/track → **12.5 MB** for the library | +| Ten-field browse subset | 271 B/track → 6.8 MB | +| Requests at the endpoint's `page_size` cap of 200 | **132** | +| Albums | 3,925 from `/library/albums`, 3,871 from `/library/stats` | +| Artists | 3,475 from `/library/artists`, 3,662 from `/library/stats` | +| `tracks.updated_at` churn | 510 rows in 24 h, 580 in 30 days, 52 distinct update days | + +**The precedent for this is broken, and finding that out is the most valuable thing in this ADR.** +`services/libraryCache.ts` is 596 lines whose entry point requests the library like this +(`libraryCache.ts:15`): + +```ts +const { data } = await api.get('/tracks', { params: { limit: 10000 } }); +``` + +`limit` is not a parameter of that endpoint. `page_size` is, capped at `le=200` +(`backend/app/api/routes/tracks/listing.py:295`), and FastAPI ignores unrecognised query parameters, +so the default page size applies. Verified against the live server: `GET /api/v1/tracks?limit=10000` +returns **50 items** with `page_size: 50` and `total: 26396`. The "cache library for offline +browsing" action in `components/Settings/OfflineSettings.tsx:75` has been caching **50 tracks of +26,396**, and nothing caught it because a 50-track offline library looks like a feature that works +rather than one that is broken. Porting this design to Swift would have ported the shape of the +defect along with it. + +**There is no delta endpoint, and `updated_at` is not yet a usable cursor.** `list_tracks` takes no +`since` parameter and `TrackResponse` does not expose `updated_at`, so a full 132-request re-fetch is +the only refresh available today. The column itself is promising — `tracks.updated_at` carries +`onupdate=func.now()` (`backend/app/db/models/tracks.py:113-115`) and its churn is low: 510 rows moved +in the last 24 hours and 580 in 30 days, so a delta would carry hundreds of rows instead of 26,396. +But the scanner's bulk upsert lists 23 columns explicitly in its `set_` clause and `updated_at` is +not among them (`backend/app/services/scanner.py:721-744`), and SQLAlchemy does not apply column +`onupdate` defaults to an `on_conflict_do_update` `set_` clause. So a rescan that changes a track's +tags leaves the timestamp untouched: the cursor is unreliable in exactly the direction that matters +most, and it is cheap to fix. + +**`/library/stats` cannot serve as the staleness fingerprint**, which is the obvious thing to reach +for and the reason to look closely. It counts every `Track` row (`library.py:50-59`) while `/tracks` +applies `Track.active_filter()` — 26,462 against 26,396, which is the 66 inactive rows exactly. Its +`total_albums` is `COUNT(DISTINCT Track.album)` at 3,871 while `/library/albums` groups and reports +3,925; its `total_artists` is `COUNT(DISTINCT Track.artist)` at 3,662 while `/library/artists` reads +the artists table and reports 3,475. That is three notions of "how many albums" and two of "how many +tracks", none of them the set a client pages through. A fingerprint measuring a different set than +the data it guards reports false staleness, and — worse, because it is silent — false freshness. + +**Albums and artists cannot be derived from cached tracks.** Artists are a real table with ids and +enrichment no client can compute: `image_url` on `/library/artists` is an external URL, Wikipedia in +the live data. Albums have no id at all — they are grouped server-side and keyed by name plus artist. +Deriving either on the device means a second implementation of grouping that has to agree with the +server's, which is the drift [ADR-0006](ADR-0006-offline-ranking-is-precomputed-server-side.md) +exists to prevent. + +**Search, by contrast, is genuinely reproducible.** The server's is a case-insensitive substring +match across exactly three columns — `Track.title.ilike | Track.artist.ilike | Track.album.ilike` +(`listing.py:330-335`) — with no ranking and no relevance ordering. A local scan over the same three +fields is not an approximation of it; it is the same predicate. This is the distinction ADR-0006 turns +on: reimplementing a *scoring function* on four clients guarantees drift, while reimplementing +`contains` does not. + +## Decision + +The client caches the library whole, refreshes it by delta, and never derives what the server groups. + +1. **The whole library is cached, not a working set.** 12.5 MB and 132 requests against a 26,396-track + library, once. A working-set cache would need an eviction policy, a prefetch heuristic, and a + story for what happens when the listener scrolls past its edge offline — all to save single-digit + megabytes. + +2. **Cache `TrackResponse` as generated, not a hand-picked projection.** The ten-field subset the web + client keeps saves 5.7 MB and costs a running decision about which fields matter; ADR-0007 makes + the full type generated, so caching it whole means a schema change reaches the cache by + recompiling rather than by remembering to add a field. + +3. **Albums, artists and playlists are cached as the server returns them.** No client-side grouping, + per the Context above. Note the asymmetry this creates: tracks, albums and artists are + library-wide, while playlists are per-profile, so the cache has a library-scoped part and a + profile-scoped part and switching profiles invalidates only the latter. + +4. **Storage stays files, and stays out of SQLite — deliberately, and this is the answer ADR-0009 + deferred.** One atomically-written `Codable` file per collection, bulk-replaced on refresh. The + decisive argument is the *write pattern*, not the row count: a library cache is replaced wholesale + when a refresh completes, where ADR-0009's download index mutates per event. Bulk replace is the + shape a whole-file rewrite is good at. Search is a linear scan in memory, which at 26k rows and a + `contains` predicate needs no index. + +5. **A purpose-built fingerprint endpoint decides whether to refresh** — active track count and + `max(updated_at)` over the same filtered set the listing endpoint pages, so one request answers + "is my cache stale". Explicitly not `/library/stats`, for the reasons in Context. + +6. **Refresh is full the first time and delta afterwards**, once the backend carries it: expose + `updated_at` on `TrackResponse`, add an `updated_since` parameter to `list_tracks`, and add + `updated_at` to the scanner's upsert `set_` clause so a tag change actually moves it. Those are + backend changes under [ADR-0007](ADR-0007-clients-are-generated-from-openapi.md) and are + prerequisites for the delta, not follow-ups to it. + +7. **The delta carries removals rather than needing a separate reconcile.** A cursor query cannot see + a deleted row, but Familiar does not delete tracks — it sets `status` away from active, 66 rows + today, and that is an ORM update which moves `updated_at`. So when `updated_since` is supplied the + endpoint returns rows regardless of status, and the client drops any id that comes back + non-active. This is why the fingerprint counts *active* tracks: it is the check that catches a + drift this rule missed. + +8. **Offline search is the same predicate, not a similar one:** case-insensitive substring over + title, artist and album. Asserted by a test that runs the same inputs against both, so the two + cannot drift apart quietly the way `parseKey` did across the TypeScript and Python + implementations ADR-0006 records. + +9. **A stale cache is labelled, never silently served as current.** The client shows when the library + was last refreshed and that it is browsing a cached copy. ADR-0009 point 4 made the filesystem the + truth for downloads; the equivalent here is that a cache which cannot be verified against the + server says so. + +## Alternatives Considered + +- **Fix `libraryCache.ts` and port its design.** Rejected as the design, though the bug should be + fixed on its own merits regardless of this ADR. Its cache is a flat track list with no albums, no + artists, no fingerprint and no refresh path beyond clear-and-refetch, and its search is + `toArray()` followed by a JavaScript filter over every row. The Apple client needs the grouped + collections and a staleness check, neither of which that design has. +- **Cache a working set — recently played, favourites, downloads and their albums.** Rejected. It + trades 5–10 MB for an eviction policy, a prefetch heuristic and an "offline edge" the listener + discovers by scrolling into it. The whole library costing 12.5 MB is what makes this easy, and that + number was measured before choosing. +- **Derive albums and artists from cached tracks.** Rejected, per Context: artist enrichment is not + derivable at all, albums have no id, and the grouping would be a second implementation that must + agree with the server's. +- **SQLite (or Core Data) for the cache.** Rejected at this size, and the reasoning is the write + pattern rather than the row count — bulk replacement suits a file, incremental querying suits a + database. It becomes correct if the cache later needs partial updates in place, per-field indexes, + or to outgrow memory; ADR-0009 already names SQLite as the successor and this is the decision that + keeps it a successor rather than a rewrite. +- **`/library/stats` as the fingerprint.** Rejected on measurement: it counts a different set than + the endpoint it would guard, and the discrepancy is not theoretical — 26,462 against 26,396, 3,871 + against 3,925, 3,662 against 3,475. +- **Refresh on a timer and skip the fingerprint.** Rejected. It re-fetches 12.5 MB to discover + nothing changed, and on cellular that is the kind of background cost that gets an app deleted. +- **A server-pushed invalidation channel** (SSE, or piggybacking the existing streams). Rejected as + disproportionate for a library that changes on 52 days out of a 175-day history. A one-request + fingerprint check at launch answers the same question without a connection to maintain. +- **Full-text search via SQLite FTS.** Rejected. The server's search is `ILIKE '%q%'` over three + columns; matching it exactly is the goal, and FTS would give *better* results than the server, + which is a divergence in the same family as a worse one. + +## Consequences + +- **Positive:** Offline browse becomes real rather than nominal, and the downloads ADR-0009 delivers + become reachable through the same navigation used online. +- **Positive:** The Swift client's cache will hold 26,396 tracks where the web client's holds 50. + That gap is the reason to write this down: the shipped web behaviour would have been the model. +- **Positive:** The delta cursor makes an ordinary refresh hundreds of rows rather than 26,396 — + measured at 510 rows over the last 24 hours and 580 over 30 days. +- **Positive:** ADR-0009's storage question is answered with a reason, so the download index and the + library cache stay one mechanism instead of two. +- **Tradeoff:** Three backend changes are prerequisites, not optional: `updated_at` exposed on + `TrackResponse`, `updated_since` on the listing endpoint, and `updated_at` added to the scanner's + upsert. The last is a correctness fix the web client also benefits from and nobody has needed until + now. +- **Tradeoff:** The cache holds the full `TrackResponse`, so it grows with the schema. A field added + for one client's benefit costs every cached row on every client. +- **Tradeoff:** Holding the library in memory is 12.5 MB as JSON but more once decoded into Swift + strings, and the iOS floor is a 15.0-era device. That footprint should be measured before shipping, + not assumed comfortable. +- **Tradeoff:** A profile-scoped part and a library-scoped part in one cache is a distinction that + invites bugs at profile switch, in the same family as the pinned/cached distinction + [ADR-0010](ADR-0010-played-bytes-are-cached-downloads-are-pinned.md) introduces. +- **Follow-up:** Fix `libraryCache.ts:15` on the web client independently of this ADR. It is a + one-word change with a real user-facing effect, and it should carry a test that asserts a cache + larger than one page. +- **Follow-up:** Measure the decoded in-memory footprint of 26,396 cached tracks on an iOS 15-era + device, and revisit decision point 4 if it is uncomfortable. +- **Follow-up:** Decide whether artwork for cached albums and artists is fetched eagerly, lazily, or + not at all offline. ADR-0009 already carries the equivalent question for track artwork; the answers + should match. +- **Follow-up:** Decide whether the fingerprint endpoint also covers albums, artists and playlists, + or whether their staleness rides on the track cursor. Grouping changes when a track's tags change, + so it probably rides — but that should be asserted, since an album renamed with no track edit would + slip through. From 31865234586e227b963e1e80f28569aa09eb303b Mon Sep 17 00:00:00 2001 From: Jeff Crouse Date: Fri, 31 Jul 2026 07:28:26 -0400 Subject: [PATCH 2/4] =?UTF-8?q?docs(adr):=20propose=20ADR-0011=20=E2=80=94?= =?UTF-8?q?=20the=20library=20is=20cached=20whole,=20refreshed=20by=20delt?= =?UTF-8?q?a?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ADR-0009 point 9 limited offline browse to the downloads list and deferred this, which leaves downloads as half a feature: with no network the Apple client can play what it downloaded but cannot navigate to it the way it was found. It also named this as the decision settling whether the `Codable` index becomes SQLite, so it wants deciding before that index acquires dependents. **The premise this was drafted from has since been fixed, and is kept as history rather than deleted.** `libraryCache.ts` passed `limit` to an endpoint that takes `page_size`, so "cache library for offline browsing" cached 50 tracks of 26,396 — and nothing caught it, because a 50-track offline library looks like a feature that works. That was fixed independently in #46 on 2026-07-30. The reasoning survives the fix: the ADR's argument was never "the web client has a bug" but that porting that design would port the shape of the defect, which is the same failure ADR-0012 point 3 rejects for favourites. What the fix removes is urgency, not the case. Re-measured against the live library today rather than trusting the draft: 26,396 active tracks, 132 requests at the `page_size` cap, and **13.2 MB** at 499 B/track — the draft said 12.5 MB, and its ten-field figure did not reconcile with its own per-track number (271 B × 26,396 is 7.1 MB, not 6.8). The `/library/stats` rejection holds exactly: 26,462 vs 26,396 tracks, 3,871 vs 3,925 albums, 3,662 vs 3,475 artists — three notions of "how many albums" and two of "how many tracks", none of them the set a client pages through. The three backend prerequisites are unchanged and still true: `TrackResponse` exposes no `updated_at`, `list_tracks` takes no `since`, and the scanner's upsert `set_` clause never mentions `updated_at` — so a rescan that rewrites a track's tags leaves the cursor untouched, which is a correctness bug on its own merits. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014P9p2fvFnyiywBxGkv4gfW --- ...-is-cached-whole-and-refreshed-by-delta.md | 72 +++++++++++-------- 1 file changed, 42 insertions(+), 30 deletions(-) diff --git a/docs/decisions/ADR-0011-the-library-is-cached-whole-and-refreshed-by-delta.md b/docs/decisions/ADR-0011-the-library-is-cached-whole-and-refreshed-by-delta.md index 2d6f038f..c11c93d6 100644 --- a/docs/decisions/ADR-0011-the-library-is-cached-whole-and-refreshed-by-delta.md +++ b/docs/decisions/ADR-0011-the-library-is-cached-whole-and-refreshed-by-delta.md @@ -2,7 +2,7 @@ Status: proposed -Date: 2026-07-29 +Date: 2026-07-31 Extends [ADR-0009](ADR-0009-offline-downloads-are-background-transfers.md) @@ -15,39 +15,52 @@ Apple client can play what it has downloaded but cannot navigate to it the way i also named this as the decision that settles whether its `Codable` index becomes SQLite, which is why it wants deciding before that index acquires dependents. -Everything below was measured against the live library on 2026-07-29 rather than estimated. +Everything below was measured against the live library rather than estimated, and re-measured on +2026-07-31 before proposing. | Fact | Value | |---|---| | Tracks | 26,396 active (26,462 rows, 66 inactive) | -| Full `TrackResponse` | 496 B/track → **12.5 MB** for the library | -| Ten-field browse subset | 271 B/track → 6.8 MB | +| Full `TrackResponse` | 499 B/track → **13.2 MB** for the library | +| Ten-field browse subset | 271 B/track → 7.1 MB | | Requests at the endpoint's `page_size` cap of 200 | **132** | | Albums | 3,925 from `/library/albums`, 3,871 from `/library/stats` | | Artists | 3,475 from `/library/artists`, 3,662 from `/library/stats` | | `tracks.updated_at` churn | 510 rows in 24 h, 580 in 30 days, 52 distinct update days | -**The precedent for this is broken, and finding that out is the most valuable thing in this ADR.** -`services/libraryCache.ts` is 596 lines whose entry point requests the library like this -(`libraryCache.ts:15`): +**The precedent for this was broken, and finding that out is what this ADR is for — but the bug +itself has since been fixed, so read it as history rather than as a live defect.** When this was +drafted on 2026-07-29, `services/libraryCache.ts` requested the library like this: ```ts const { data } = await api.get('/tracks', { params: { limit: 10000 } }); ``` `limit` is not a parameter of that endpoint. `page_size` is, capped at `le=200` -(`backend/app/api/routes/tracks/listing.py:295`), and FastAPI ignores unrecognised query parameters, -so the default page size applies. Verified against the live server: `GET /api/v1/tracks?limit=10000` -returns **50 items** with `page_size: 50` and `total: 26396`. The "cache library for offline -browsing" action in `components/Settings/OfflineSettings.tsx:75` has been caching **50 tracks of -26,396**, and nothing caught it because a 50-track offline library looks like a feature that works -rather than one that is broken. Porting this design to Swift would have ported the shape of the -defect along with it. +(`backend/app/api/routes/tracks/listing.py:298`), and FastAPI ignores unrecognised query parameters, +so the default page size applied: `GET /api/v1/tracks?limit=10000` returned **50 items** with +`page_size: 50` and `total: 26396`. The "cache library for offline browsing" action had been caching +**50 tracks of 26,396**, and nothing caught it because a 50-track offline library looks like a +feature that works rather than one that is broken. + +It was fixed independently on 2026-07-30 (`5d7d1fb`, "cache the whole library, not the first page of +it", #46): the service now pages at 200 with a runaway guard, fails rather than returning a partial +cache, and carries a test asserting a cache larger than one page. + +**The lesson survives the fix, which is why this stays in the record.** The argument for writing this +ADR was never "the web client has a bug" — it was that porting that design to Swift would have +ported the *shape* of the defect, and a cache that silently holds part of a collection is +indistinguishable from a working one. That is the same failure mode +[ADR-0012](ADR-0012-favorites-are-a-collection-not-a-library-section.md) point 3 rejects for +favourites, and the reason decision point 1 below caches the library whole rather than a working set. +What the fix removes is the *urgency*, not the reasoning: the web client's cache is now a flat track +list with no albums, no artists, no fingerprint and no refresh path beyond clear-and-refetch, and +those absences are what the decisions below are actually about. **There is no delta endpoint, and `updated_at` is not yet a usable cursor.** `list_tracks` takes no `since` parameter and `TrackResponse` does not expose `updated_at`, so a full 132-request re-fetch is the only refresh available today. The column itself is promising — `tracks.updated_at` carries -`onupdate=func.now()` (`backend/app/db/models/tracks.py:113-115`) and its churn is low: 510 rows moved +`onupdate=func.now()` (`backend/app/db/models/tracks.py:112-114`) and its churn is low: 510 rows moved in the last 24 hours and 580 in 30 days, so a delta would carry hundreds of rows instead of 26,396. But the scanner's bulk upsert lists 23 columns explicitly in its `set_` clause and `updated_at` is not among them (`backend/app/services/scanner.py:721-744`), and SQLAlchemy does not apply column @@ -73,7 +86,7 @@ exists to prevent. **Search, by contrast, is genuinely reproducible.** The server's is a case-insensitive substring match across exactly three columns — `Track.title.ilike | Track.artist.ilike | Track.album.ilike` -(`listing.py:330-335`) — with no ranking and no relevance ordering. A local scan over the same three +(`listing.py:88-90`) — with no ranking and no relevance ordering. A local scan over the same three fields is not an approximation of it; it is the same predicate. This is the distinction ADR-0006 turns on: reimplementing a *scoring function* on four clients guarantees drift, while reimplementing `contains` does not. @@ -82,13 +95,13 @@ on: reimplementing a *scoring function* on four clients guarantees drift, while The client caches the library whole, refreshes it by delta, and never derives what the server groups. -1. **The whole library is cached, not a working set.** 12.5 MB and 132 requests against a 26,396-track +1. **The whole library is cached, not a working set.** 13.2 MB and 132 requests against a 26,396-track library, once. A working-set cache would need an eviction policy, a prefetch heuristic, and a story for what happens when the listener scrolls past its edge offline — all to save single-digit megabytes. 2. **Cache `TrackResponse` as generated, not a hand-picked projection.** The ten-field subset the web - client keeps saves 5.7 MB and costs a running decision about which fields matter; ADR-0007 makes + client keeps saves 6.0 MB and costs a running decision about which fields matter; ADR-0007 makes the full type generated, so caching it whole means a schema change reaches the cache by recompiling rather than by remembering to add a field. @@ -133,14 +146,15 @@ The client caches the library whole, refreshes it by delta, and never derives wh ## Alternatives Considered -- **Fix `libraryCache.ts` and port its design.** Rejected as the design, though the bug should be - fixed on its own merits regardless of this ADR. Its cache is a flat track list with no albums, no - artists, no fingerprint and no refresh path beyond clear-and-refetch, and its search is - `toArray()` followed by a JavaScript filter over every row. The Apple client needs the grouped - collections and a staleness check, neither of which that design has. +- **Port `libraryCache.ts`'s design, now that its paging bug is fixed.** Rejected, and the fix is + what makes this the honest version of the question — the design can now be judged on its merits + rather than on a defect. Its cache is a flat track list with no albums, no artists, no fingerprint + and no refresh path beyond clear-and-refetch, and its search is `toArray()` followed by a + JavaScript filter over every row. The Apple client needs the grouped collections and a staleness + check, and neither is something that design could grow without becoming this one. - **Cache a working set — recently played, favourites, downloads and their albums.** Rejected. It trades 5–10 MB for an eviction policy, a prefetch heuristic and an "offline edge" the listener - discovers by scrolling into it. The whole library costing 12.5 MB is what makes this easy, and that + discovers by scrolling into it. The whole library costing 13.2 MB is what makes this easy, and that number was measured before choosing. - **Derive albums and artists from cached tracks.** Rejected, per Context: artist enrichment is not derivable at all, albums have no id, and the grouping would be a second implementation that must @@ -166,8 +180,9 @@ The client caches the library whole, refreshes it by delta, and never derives wh - **Positive:** Offline browse becomes real rather than nominal, and the downloads ADR-0009 delivers become reachable through the same navigation used online. -- **Positive:** The Swift client's cache will hold 26,396 tracks where the web client's holds 50. - That gap is the reason to write this down: the shipped web behaviour would have been the model. +- **Positive:** Both clients will cache the whole library rather than a page of it. That was the gap + this ADR was written to stop the Swift client inheriting; the web half of it closed independently + in #46, which is the outcome the ADR wanted and not evidence it was unnecessary. - **Positive:** The delta cursor makes an ordinary refresh hundreds of rows rather than 26,396 — measured at 510 rows over the last 24 hours and 580 over 30 days. - **Positive:** ADR-0009's storage question is answered with a reason, so the download index and the @@ -178,15 +193,12 @@ The client caches the library whole, refreshes it by delta, and never derives wh now. - **Tradeoff:** The cache holds the full `TrackResponse`, so it grows with the schema. A field added for one client's benefit costs every cached row on every client. -- **Tradeoff:** Holding the library in memory is 12.5 MB as JSON but more once decoded into Swift +- **Tradeoff:** Holding the library in memory is 13.2 MB as JSON but more once decoded into Swift strings, and the iOS floor is a 15.0-era device. That footprint should be measured before shipping, not assumed comfortable. - **Tradeoff:** A profile-scoped part and a library-scoped part in one cache is a distinction that invites bugs at profile switch, in the same family as the pinned/cached distinction [ADR-0010](ADR-0010-played-bytes-are-cached-downloads-are-pinned.md) introduces. -- **Follow-up:** Fix `libraryCache.ts:15` on the web client independently of this ADR. It is a - one-word change with a real user-facing effect, and it should carry a test that asserts a cache - larger than one page. - **Follow-up:** Measure the decoded in-memory footprint of 26,396 cached tracks on an iOS 15-era device, and revisit decision point 4 if it is uncomfortable. - **Follow-up:** Decide whether artwork for cached albums and artists is fetched eagerly, lazily, or From fde075fdf3a9257fe8193bdd0a07440983d02649 Mon Sep 17 00:00:00 2001 From: Jeff Crouse Date: Fri, 31 Jul 2026 07:28:26 -0400 Subject: [PATCH 3/4] docs(adr): link ADR-0012's offline follow-up to ADR-0011, now that it exists The follow-up explained at length that it was deliberately not linking ADR-0011 because the file was held on an unpushed branch and there was nothing to link to. There is now. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014P9p2fvFnyiywBxGkv4gfW --- ...012-favorites-are-a-collection-not-a-library-section.md | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/docs/decisions/ADR-0012-favorites-are-a-collection-not-a-library-section.md b/docs/decisions/ADR-0012-favorites-are-a-collection-not-a-library-section.md index e3c16482..e73da3ba 100644 --- a/docs/decisions/ADR-0012-favorites-are-a-collection-not-a-library-section.md +++ b/docs/decisions/ADR-0012-favorites-are-a-collection-not-a-library-section.md @@ -226,10 +226,9 @@ the shape of the work.** revisit. It needs a background refresh path and a policy for intent that changed while the device was away. - **Follow-up:** Offline favourites. The set is on the device but the list is not; whether it - becomes browsable without a server depends on ADR-0011, which is drafted but held on branch - `docs/adr-0010-0011-held` and so is deliberately not linked here — there is no file to link to - until it is proposed. If the library is cached whole, favourites become a filter over it rather - than a second cache. + becomes browsable without a server depends on + [ADR-0011](ADR-0011-the-library-is-cached-whole-and-refreshed-by-delta.md), proposed 2026-07-31. + If the library is cached whole, favourites become a filter over it rather than a second cache. - **Follow-up:** Whether a favourite should be a queue source — "play my favourites" — which is a ranking question closer to [ADR-0005](ADR-0005-one-ranking-engine-serves-ambient-and-radio.md) than to this one. From 3a91fd339565f37345084970f5ac2d576dc7022f Mon Sep 17 00:00:00 2001 From: Jeff Crouse Date: Fri, 31 Jul 2026 07:44:39 -0400 Subject: [PATCH 4/4] =?UTF-8?q?docs(adr):=20correct=20ADR-0011=20=E2=80=94?= =?UTF-8?q?=20the=20delta=20cursor=20already=20works?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The draft claimed a rescan that changes a track's tags leaves `updated_at` untouched, because the scanner's upsert omits it from an `on_conflict_do_update` `set_` clause SQLAlchemy emits verbatim. The omission was real. The conclusion was not: that upsert is in `_create_track`, the new-track path, whose conflict branch fires only when two scans race on the same `file_path`. An ordinary rescan goes through `_update_track`, which is plain ORM attribute assignment, so `onupdate` applies and the timestamp moves. Caught only by measuring it. The first two attempts reported a false positive, because `expire_on_commit=False` lets the in-memory attribute be older than the row it came from — reading back on a separate connection showed the timestamp advancing exactly when the rescan ran. This makes the ADR cheaper: two backend prerequisites rather than three. The race-path gap was fixed separately, with three tests pinning what a delta rests on — a retag moves the cursor, an unchanged rescan does not, and the conflict branch moves it. Recorded rather than quietly edited, per rule 5. A premise that made the work look *larger* is worth writing down for the same reason as one that made it look smaller: the next person to read `scanner.py` will see the same `set_` clause and reach the same wrong conclusion. Also corrected: the clause lists 22 columns, not 23. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014P9p2fvFnyiywBxGkv4gfW --- ...-is-cached-whole-and-refreshed-by-delta.md | 39 +++++++++++++------ 1 file changed, 27 insertions(+), 12 deletions(-) diff --git a/docs/decisions/ADR-0011-the-library-is-cached-whole-and-refreshed-by-delta.md b/docs/decisions/ADR-0011-the-library-is-cached-whole-and-refreshed-by-delta.md index c11c93d6..d2e424ca 100644 --- a/docs/decisions/ADR-0011-the-library-is-cached-whole-and-refreshed-by-delta.md +++ b/docs/decisions/ADR-0011-the-library-is-cached-whole-and-refreshed-by-delta.md @@ -62,11 +62,25 @@ those absences are what the decisions below are actually about. the only refresh available today. The column itself is promising — `tracks.updated_at` carries `onupdate=func.now()` (`backend/app/db/models/tracks.py:112-114`) and its churn is low: 510 rows moved in the last 24 hours and 580 in 30 days, so a delta would carry hundreds of rows instead of 26,396. -But the scanner's bulk upsert lists 23 columns explicitly in its `set_` clause and `updated_at` is -not among them (`backend/app/services/scanner.py:721-744`), and SQLAlchemy does not apply column -`onupdate` defaults to an `on_conflict_do_update` `set_` clause. So a rescan that changes a track's -tags leaves the timestamp untouched: the cursor is unreliable in exactly the direction that matters -most, and it is cheap to fix. +**A second premise here was wrong, and it was wrong in the flattering direction — it made the work +look bigger than it is.** This ADR was drafted claiming that a rescan changing a track's tags leaves +`updated_at` untouched, because the scanner's upsert omits it from an `on_conflict_do_update` `set_` +clause that SQLAlchemy emits verbatim. The `set_` clause really did omit it, and it really is +emitted verbatim — but that upsert is in `_create_track`, the **new-track** path, whose conflict +branch is reached only when two scans race on the same `file_path`. An ordinary rescan goes through +`_update_track`, which is plain ORM attribute assignment, so `onupdate` applies and the timestamp +moves. + +Settled by measurement rather than by reading the code, which is the only reason it was caught: the +first two attempts at that measurement both reported a false positive, because `expire_on_commit=False` +means the in-memory attribute can be older than the row it came from. Reading the row back on a +separate connection showed the timestamp advancing exactly when the rescan ran. + +**So the cursor is sound today, and the correction makes this ADR cheaper rather than more +expensive.** The race-path omission was fixed separately, along with three tests that pin what a +delta would rest on and none of which existed: a retag moves `updated_at`, an unchanged rescan does +*not* — otherwise every delta carries the whole library — and the conflict branch moves it too. That +leaves two backend prerequisites below rather than three. **`/library/stats` cannot serve as the staleness fingerprint**, which is the obvious thing to reach for and the reason to look closely. It counts every `Track` row (`library.py:50-59`) while `/tracks` @@ -122,10 +136,11 @@ The client caches the library whole, refreshes it by delta, and never derives wh "is my cache stale". Explicitly not `/library/stats`, for the reasons in Context. 6. **Refresh is full the first time and delta afterwards**, once the backend carries it: expose - `updated_at` on `TrackResponse`, add an `updated_since` parameter to `list_tracks`, and add - `updated_at` to the scanner's upsert `set_` clause so a tag change actually moves it. Those are + `updated_at` on `TrackResponse`, and add an `updated_since` parameter to `list_tracks`. Both are backend changes under [ADR-0007](ADR-0007-clients-are-generated-from-openapi.md) and are - prerequisites for the delta, not follow-ups to it. + prerequisites for the delta, not follow-ups to it. The third prerequisite this ADR was drafted + with — making the scanner move `updated_at` — turned out to be already satisfied on the path that + matters, and the narrow race-path gap has since been closed with tests. 7. **The delta carries removals rather than needing a separate reconcile.** A cursor query cannot see a deleted row, but Familiar does not delete tracks — it sets `status` away from active, 66 rows @@ -187,10 +202,10 @@ The client caches the library whole, refreshes it by delta, and never derives wh measured at 510 rows over the last 24 hours and 580 over 30 days. - **Positive:** ADR-0009's storage question is answered with a reason, so the download index and the library cache stay one mechanism instead of two. -- **Tradeoff:** Three backend changes are prerequisites, not optional: `updated_at` exposed on - `TrackResponse`, `updated_since` on the listing endpoint, and `updated_at` added to the scanner's - upsert. The last is a correctness fix the web client also benefits from and nobody has needed until - now. +- **Tradeoff:** Two backend changes are prerequisites, not optional: `updated_at` exposed on + `TrackResponse`, and `updated_since` on the listing endpoint. It was three when this was drafted; + the third rested on a misreading of which code path a rescan takes, and the tests written while + checking it now pin the cursor behaviour the other two depend on. - **Tradeoff:** The cache holds the full `TrackResponse`, so it grows with the schema. A field added for one client's benefit costs every cached row on every client. - **Tradeoff:** Holding the library in memory is 13.2 MB as JSON but more once decoded into Swift