From 07d2e3fee3354fc8de90b050417889b26539899b Mon Sep 17 00:00:00 2001 From: Claudio Ortolina Date: Fri, 8 May 2026 09:57:00 +0100 Subject: [PATCH] ML-169: research and plan --- ...-functional-correctness-and-performance.md | 234 ++++++++++++++++++ 1 file changed, 234 insertions(+) create mode 100644 backlog/tasks/ml-169 - Comprehensive-application-audit-functional-correctness-and-performance.md diff --git a/backlog/tasks/ml-169 - Comprehensive-application-audit-functional-correctness-and-performance.md b/backlog/tasks/ml-169 - Comprehensive-application-audit-functional-correctness-and-performance.md new file mode 100644 index 00000000..b669a57b --- /dev/null +++ b/backlog/tasks/ml-169 - Comprehensive-application-audit-functional-correctness-and-performance.md @@ -0,0 +1,234 @@ +--- +id: ML-169 +title: "Comprehensive application audit: functional correctness and performance" +status: To Do +assignee: [] +created_date: "2026-05-08 08:46" +updated_date: "2026-05-08 08:49" +labels: + - audit + - performance + - reliability +dependencies: [] +references: + - docs/architecture.md + - docs/project-conventions.md + - >- + backlog/tasks/ml-168 - + Update-wishlist-index-and-collection-index-when-background-import-finishes.md +priority: medium +--- + +## Description + + + +Large-scale audit of the application covering two dimensions: + +**1. Functional Issues**: Unhandled async messages from LiveComponents, stale PubSub subscriptions, unexpected state changes from concurrent operations (background workers updating records while user is viewing them). + +**2. Performance Low-Hanging Fruit**: Unnecessary server round trips, missing opportunities for optimistic updates, redundant data reloads, and synchronous blocking work in mount. + +The audit is divided into 4 independent phases that can be tackled in parallel or sequentially. Each phase produces findings that feed into fix tasks. Full implementation plan in the task plan section. + + + +## Acceptance Criteria + + + +- [ ] #1 Phase 1 (async messages): all LiveComponents that send messages to parent LiveViews have matching handle_info clauses documented; missing handlers identified with severity +- [ ] #2 Phase 2 (PubSub lifecycle): all subscribe/unsubscribe pairs verified correct; stale subscription risks identified; ML-168 gap separately accounted for +- [ ] #3 Phase 3 (state change safety): concurrent record-mutation patterns audited; {:update, record} guards and manage_subscription patterns verified across all navigation paths +- [ ] #4 Phase 4 (performance): unnecessary round trips, redundant full-stream reloads, and synchronous mount work identified with estimated impact and fix approach +- [ ] #5 Each phase produces a written finding report with file paths, line references, severity ratings, and recommended fixes + + +## Implementation Plan + + + +# Audit Implementation Plan + +## Audit Scope + +This audit covers all 14 LiveViews, all LiveComponents that send messages to parents, all PubSub subscription points, and all Oban worker-LiveView interaction patterns. The full architecture is documented in `docs/architecture.md`. + +## Phase Structure + +Each phase is a standalone investigation that produces a findings document. Fixes are NOT implemented in these phases — only identification, severity rating, and fix recommendations. + +--- + +## Phase 1: Async Message Audit (LiveComponent → Parent) + +**Goal**: Verify every LiveComponent message has a matching `handle_info` in its parent LiveView. + +**Known message producers and their parents**: + +| Component | Messages sent | Consumer LiveViews | Status | +| ------------------------------ | -------------------------------------------------------- | -------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------- | +| `RecordForm` | `{:saved, record}` | CollectionLive.Index, WishlistLive.Index, CollectionLive.Show, WishlistLive.Show | ✅ All four have handlers | +| `AddRecord` | `{:imported_single, record}`, `{:imported_async, count}` | CollectionLive.Index, WishlistLive.Index | ✅ Both have handlers | +| `Chat` | `:chats_changed` | CollectionLive.Index, CollectionLive.Show, WishlistLive.Show, ArtistLive.Show | ✅ All four have handlers | +| `Release` | `{tag, release}` (dynamic tag) | ScrobbleLive.ReleaseShow, CollectionLive.Show | ⚠️ CollectionLive.Show renders Release but has NO `handle_info({:release_loaded, _})` — needs investigation | +| `ArtistLive.Form` | `{:saved, artist_info}` | ArtistLive.Show | ✅ Has handler | +| `RecordSetLive.Form` | `{:created, _}`, `{:updated, _}` | RecordSetLive.Index | ✅ Has handlers | +| `RecordSetLive.RecordPicker` | `{:added, record_set}` | RecordSetLive.Index | ✅ Has handler | +| `ScrobbleRulesLive.Form` | `{:created, _}`, `{:updated, _}` | ScrobbleRulesLive.Index | ✅ Has handlers | +| `ScrobbledTracksLive.Form` | `{:saved, _track}` | ScrobbledTracksLive.Index | ✅ Has handler | +| `OnlineStoreTemplateLive.Form` | `{:saved, template}` | OnlineStoreTemplateLive.Index | ✅ Has handler | +| `ScrobbleRulePicker` | `{:rule_created, _rule}` | ScrobbledTracksLive.Index, StatsLive.Index | ✅ Both have handlers | + +**Investigation steps**: + +1. Cross-reference every `send(self(), ...)` call site (found via `grep -rn "send(self()"`) with `handle_info` clauses in the parent LiveView +2. Specifically trace the `Release` component usage in `CollectionLive.Show` — it renders `Release` with `on_release_loaded={:release_loaded}`, but `CollectionLive.Show` has no `handle_info({:release_loaded, _})` clause. Determine if this is a dead message or a missing handler +3. Verify no component sends messages when rendered in a context that doesn't handle them (e.g., modal closed, wrong live_action) +4. Look for `send/2` calls using patterns NOT matching `{__MODULE__, _}` + +**Files to check**: All `lib/music_library_web/components/*.ex`, all `lib/music_library_web/live/**/*.ex` + +--- + +## Phase 2: PubSub Subscription Lifecycle Audit + +**Goal**: Verify all PubSub subscriptions are properly paired with unsubscriptions; identify stale subscription risks. + +**Subscription inventory**: + +| Subscribe | Topic | Where | Unsubscribe | Risk | +| ---------------------------- | -------------------------- | -------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------- | +| `Records.subscribe(id)` | `"records:#{id}"` | CollectionLive.Show, WishlistLive.Show (via `manage_subscription/2`) | ✅ `manage_subscription/2` unsubscribes old ID before subscribing new — called on every `handle_params` | Low | +| `ListeningStats.subscribe()` | `"listening_stats:update"` | StatsLive.Index (`mount`), ScrobbledTracksLive.Index (`mount`) | ⚠️ Never unsubscribed. Only called when connected. | Medium — PID monitoring handles cleanup but double-subscription on reconnect possible | + +**Broadcast inventory**: + +| Broadcast | Topic | Message | Called from | +| ----------------------------------- | -------------------------- | ------------------- | ----------------------------------------- | +| `Records.notify_update/1` | `"records:#{id}"` | `{:update, record}` | Workers (needs audit — see Phase 3) | +| `ListeningStats.broadcast_update/1` | `"listening_stats:update"` | `%{track_count: n}` | `lib/music_library/listening_stats.ex:68` | + +**Investigation steps**: + +1. Verify `manage_subscription/2` is called on ALL navigation paths: browser back/forward, direct URL, push_navigate, push_patch +2. Check that `Records.subscribe/1` is NEVER called outside of `manage_subscription/2` (only two Show LiveViews should call it) +3. Verify `ListeningStats.subscribe()` doesn't double-subscribe on LiveView reconnect (check if Phoenix.LiveView re-mounts fully or reuses process) +4. Check that `Records.unsubscribe/1` is called on terminate — currently it's only called on navigation, not process termination. Phoenix.PubSub auto-cleans when PID dies, but verify +5. Audit `Records.notify_update/1` broadcast sites against intended behavior (see Phase 3) + +**Files to check**: `lib/music_library/records.ex`, `lib/music_library/listening_stats.ex`, `lib/music_library_web/live_helpers/record_actions.ex`, all Show LiveViews + +--- + +## Phase 3: Concurrent State Change Safety Audit + +**Goal**: Verify that background worker updates to records don't corrupt LiveView state. + +**Pattern**: Oban worker modifies record → calls `Records.notify_update/1` → PubSub broadcasts `{:update, record}` → Show LiveView's `handle_info({:update, record}, socket)` applies update via `RecordActions.handle_record_updated/2`. + +**Guard**: `record.id == socket.assigns.record.id` in both CollectionLive.Show and WishlistLive.Show ✅ + +**Worker-to-broadcast audit** (every worker that modifies records): + +| Worker | Modifies record? | Calls notify_update? | Check | +| ----------------------------------- | ----------------------------------- | -------------------- | ------------------------ | +| `RefreshCover` | Yes — updates cover_url, cover_hash | Need to verify | Read worker source | +| `PopulateGenres` | Yes — updates genres | Need to verify | Read worker source | +| `GenerateRecordEmbedding` | Yes (indirect via Similarity) | Need to verify | Read worker + Similarity | +| `RecordRefreshMusicBrainzData` | Yes — updates musicbrainz_data | Need to verify | Read worker source | +| `RecordRefreshAllMusicBrainzData` | Yes (batch) | Need to verify | Read worker + Batch | +| `RecordGenerateAllEmbeddings` | Yes (batch) | Need to verify | Read worker + Batch | +| `ImportFromMusicbrainzRelease` | Creates new record | Need to verify | Read worker source | +| `ImportFromMusicbrainzReleaseGroup` | Creates new records | Need to verify | Read worker source | +| `PruneAssets` | No (assets only) | N/A | Skip | +| `PruneAssetCache` | No (cache only) | N/A | Skip | +| `ApplyScrobbleRules` | No (scrobble data) | N/A | Skip | +| `FetchArtistInfo` | No (artist info) | N/A | Skip | +| All `ArtistRefresh*` workers | No (artist info) | N/A | Skip | +| `RefreshScrobbles` | No (scrobble data) | N/A | Skip | +| `BackfillScrobbledTracks` | No (scrobble data) | N/A | Skip | +| `SendRecordsOnThisDayEmail` | No (email) | N/A | Skip | +| `RepoVacuum` / `RepoOptimize` | No (maintenance) | N/A | Skip | + +**Additional checks**: + +1. **Double-update race**: If two workers update the same record simultaneously, does the LiveView handle both `handle_info` calls correctly? (Each re-assigns `:record` — second update is latest, which is correct since workers touch different fields) +2. **Form edit + background update race**: User editing in `RecordForm` modal while a background worker updates the record. The form holds a stale `@record` assign. When the worker broadcasts `{:update, record}`, the Show page updates `socket.assigns.record`, but the form LiveComponent has its OWN `@record` assign set during `update_many`. What happens? This is a potential functional bug. +3. **ArtistLive.Show**: Does NOT handle `{:update, record}` — artist updates use a different mechanism (artist info is updated through `ArtistLive.Form` which sends `{:saved, artist_info}` directly). Verify this is intentional and complete. + +**Files to check**: All `lib/music_library/worker/*.ex`, `lib/music_library/records.ex` (notify_update), `lib/music_library/records/similarity.ex`, all Show LiveViews, `RecordForm` component + +--- + +## Phase 4: Performance Low-Hanging Fruit Audit + +**Goal**: Identify unnecessary work — DB queries, stream resets, server round trips — that could be eliminated or deferred. + +### 4a. Unnecessary Full-Stream Reloads + +| LiveView | Stream name | Reset trigger | Optimizable? | Impact | +| ----------------------------- | ---------------------------------- | -------------------------------------------------------------------- | -------------------------------------------------------------------------------- | ---------------------------------------- | +| CollectionLive.Index | `:records` | Every param change, form save, display toggle | Display toggle (grid↔list) could be CSS class only — same data, different render | Medium: 1 FTS query per toggle | +| WishlistLive.Index | `:records` | Same as above | Same | Medium | +| ScrobbleRulesLive.Index | `:scrobble_rules` | Form save: does `stream_insert` then `load_and_assign_rules` (reset) | The `stream_insert` is immediately overwritten by full reload — redundant | Low: cosmetic | +| OnlineStoreTemplateLive.Index | `:templates` | Form save: same redundant `stream_insert` + full reload | Same | Low | +| ScrobbledTracksLive.Index | `:tracks` | Form save, PubSub scrobble update | Could incrementally `stream_insert` the edited track instead of full reload | Medium: full FTS on every scrobble batch | +| RecordSetLive.Index | `:record_sets` | Form create | Could `stream_insert` the new set without full reload | Low | +| StatsLive.Index | `:recent_tracks`, `:recent_albums` | PubSub scrobble update | Incremental `stream_insert` could replace full reset | Medium: resets on every scrobble batch | + +### 4b. Synchronous Mount Work → Async Candidates + +| LiveView | Blocking queries | Recommendation | Impact | +| -------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------- | -------------------------------------------------------- | ----------------------------- | +| StatsLive.Index | `count_records_by_artist(20)`, `count_records_by_genre(20)`, `count_records_by_release_year(20)`, `get_records_on_this_day()`, `get_latest_record()` | Move to `assign_async` — these are below-the-fold charts | High: 5 queries blocking TTFB | +| CollectionLive.Index | `Chats.count_chats(:collection, ...)` | Small, acceptable | None | + +### 4c. Server Round Trips → Client-Side Candidates + +| Interaction | Current | Optimizable? | Impact | +| ------------------------------- | --------------------------------------------------------------------------- | ---------------------------------------------------------- | -------------------------------------------- | +| Display mode toggle (grid↔list) | Server `handle_event("set_display")` → full DB query + stream reset | Switch to client-side JS hook that toggles CSS class | Medium: eliminates 1 round trip + 1 DB query | +| Search input | Every keystroke triggers `phx-change` → `handle_event("search")` → DB query | Check if `phx-debounce` is set. If not, add 300ms debounce | Medium: reduces query load | +| Sort order change | `push_patch` → `handle_params` → DB query | Cannot be client-side (pagination depends on sort order) | N/A | + +### 4d. Optimistic UI Opportunities + +| Operation | Current behavior | Optimistic approach | Risk | +| ---------------------------- | ------------------------------------------- | ---------------------------------------------------------------------------- | ------------------------------ | +| Record delete | `Repo.delete` → `stream_delete` (confirmed) | `stream_delete` immediately → `Repo.delete` in background; rollback on error | Low: delete failures are rare | +| Form save in modal | `push_patch` to base index → full reload | `stream_insert` updated record into parent stream without navigation | Medium: need to handle re-sort | +| Scrobble rule enabled toggle | Full list reload via `push_patch` | Toggle `stream_insert` item in-place | Low | +| Online store template toggle | Full list reload via `push_patch` | Toggle `stream_insert` item in-place | Low | + +### 4e. N+1 and Query Pattern Audit + +**Checklist**: + +- [ ] `CollectionLive.Show.handle_params/3` — loads record, last_listened_track, play_count, record_sets, chat_count, embedding_text, similar_records. Check for N+1. +- [ ] `ArtistLive.Show.handle_params/3` — loads artist, collection_records, wishlist_records. Check for N+1. +- [ ] Stream rendering — check `.record_grid`, `.record_list`, `.record_card` components for per-item DB calls +- [ ] Verify preloads cover all rendered fields (artists, formats, genres, dominant_colors) + +**Investigation method**: + +1. Use QueryReporter skill to capture SQL for: `/collection`, `/collection/:id`, `/artists/:id`, `/stats` +2. Review stream usage with `grep -rn "stream(" lib/music_library_web/live/` +3. Check for `Repo` calls inside LiveComponents (should delegate to context) +4. Check for `Repo` calls inside `.heex` templates (should be in `handle_params`) + +**Files to check**: All `lib/music_library_web/live/**/*.ex`, all `lib/music_library_web/components/*.ex` + + + +## Definition of Done + + + +- [ ] #1 Audit report for each phase written as a Backlog.md document with file paths, line references, severity ratings, and fix recommendations +- [ ] #2 At least one concrete finding identified per phase before considering the phase complete +- [ ] #3 Each finding includes a specific file:line reference (not just a general concern) +- [ ] #4 Findings are triaged by severity: HIGH (crashes/data loss), MEDIUM (wrong behavior/noticeable perf), LOW (cosmetic/redundant) +- [ ] #5 ML-168 is referenced where relevant but not duplicated — this audit identifies NEW issues only +- [ ] #6 No fixes implemented during audit — only identification and recommendation +