From 2a108660ae8dd81a75b90fe3e086090213a7e111 Mon Sep 17 00:00:00 2001 From: Claudio Ortolina Date: Tue, 19 May 2026 08:33:55 +0100 Subject: [PATCH] ML-169.1: performed concurrent state change safety audit --- ...-Concurrent-State-Change-Safety-Phase-3.md | 364 ++++++++++++++++++ ...-3-Concurrent-state-change-safety-audit.md | 35 +- 2 files changed, 391 insertions(+), 8 deletions(-) create mode 100644 backlog/docs/audits/phase3-concurrent-state-safety/doc-26 - Audit-Report-Concurrent-State-Change-Safety-Phase-3.md diff --git a/backlog/docs/audits/phase3-concurrent-state-safety/doc-26 - Audit-Report-Concurrent-State-Change-Safety-Phase-3.md b/backlog/docs/audits/phase3-concurrent-state-safety/doc-26 - Audit-Report-Concurrent-State-Change-Safety-Phase-3.md new file mode 100644 index 00000000..875516ca --- /dev/null +++ b/backlog/docs/audits/phase3-concurrent-state-safety/doc-26 - Audit-Report-Concurrent-State-Change-Safety-Phase-3.md @@ -0,0 +1,364 @@ +--- +id: doc-26 +title: "Audit Report: Concurrent State Change Safety (Phase 3)" +type: other +created_date: "2026-05-19 07:28" +tags: + - audit + - concurrency + - worker + - notify_update + - race-condition + - record +--- + +# Phase 3 Concurrent State Change Safety Audit + +**Date:** 2026-05-19 +**Scope:** All Oban workers that modify records matched against `Records.notify_update/1` broadcast coverage. Form-edit vs background-update race traced. Double-update race evaluated. + +## Executive Summary + +**Result: MINOR FINDING — one medium-severity race condition identified.** All 8 record-modifying workers correctly broadcast `notify_update` or `broadcast_index_changed`. Double-update between concurrent workers is safe due to Ecto changeset field-level updates + SQLite write serialization. However, the **form edit + background update race** (AC #3) can cause data loss: when a Show LiveView is in `:edit` mode and a worker broadcasts `{:update, record}`, the parent's `@record` assign is overwritten, and the user's next save can revert the worker's changes. + +--- + +## Acceptance Criterion #1: Record-Modifying Worker Broadcast Coverage + +**Status: ✅ ALL VERIFIED — every record-modifying worker broadcasts.** + +### Workers that modify existing records (broadcast `notify_update`) + +| Worker | File | Line | Fields modified | Broadcast call | +| ------------------------------ | -------------------------------------------------------------- | ---- | --------------------------------------------- | --------------------------------------- | +| `RefreshCover` | `lib/music_library/worker/refresh_cover.ex` | 14 | `cover_hash` | `Records.notify_update(updated_record)` | +| `PopulateGenres` | `lib/music_library/worker/populate_genres.ex` | 15 | `genres` | `Records.notify_update(updated_record)` | +| `GenerateRecordEmbedding` | `lib/music_library/worker/generate_record_embedding.ex` | 16 | embedding (via Similarity, not record column) | `Records.notify_update(record)` | +| `RecordRefreshMusicBrainzData` | `lib/music_library/worker/record_refresh_music_brainz_data.ex` | 14 | `musicbrainz_data` | `Records.notify_update(updated_record)` | + +**Code verification:** + +```elixir +# refresh_cover.ex:11-14 +case Records.refresh_cover(record) do + {:ok, updated_record} -> Records.notify_update(updated_record) ✅ + {:error, :cover_not_available} -> {:cancel, :cover_not_available} + other -> ErrorHandler.to_oban_result(other) +end + +# populate_genres.ex:11-16 +with {:ok, updated_record} <- Records.populate_genres(record), + {:ok, _worker} <- Records.Similarity.generate_embedding_async(updated_record) do + Records.notify_update(updated_record) ✅ +else + other -> ErrorHandler.to_oban_result(other) +end + +# generate_record_embedding.ex:14-16 +case Similarity.generate_embedding(record) do + :noop -> :ok + {:ok, _} -> Records.notify_update(record) ✅ + other -> ErrorHandler.to_oban_result(other) +end + +# record_refresh_music_brainz_data.ex:12-15 +case Records.refresh_musicbrainz_data(record) do + {:ok, updated_record} -> Records.notify_update(updated_record) ✅ + other -> ErrorHandler.to_oban_result(other) +end +``` + +### Workers that create new records (broadcast `broadcast_index_changed`) + +| Worker | File | Line | Creates | Broadcast call | +| ----------------------------------- | ------------------------------------------------------------------- | ---- | ---------- | -------------------------------------- | +| `ImportFromMusicbrainzRelease` | `lib/music_library/worker/import_from_musicbrainz_release.ex` | 24 | New record | `Records.broadcast_index_changed()` ✅ | +| `ImportFromMusicbrainzReleaseGroup` | `lib/music_library/worker/import_from_musicbrainz_release_group.ex` | 27 | New record | `Records.broadcast_index_changed()` ✅ | + +These correctly use `broadcast_index_changed()` instead of `notify_update` because new records have no existing subscribers on `"records:#{id}"`. + +### Batch workers (delegate to per-record async jobs) + +| Worker | File | Delegates to | Individual worker broadcasts? | +| --------------------------------- | ----------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------- | ----------------------------- | +| `RecordGenerateAllEmbeddings` | `lib/music_library/worker/record_generate_all_embeddings.ex` | `Records.Batch.generate_embeddings()` → `Similarity.generate_embedding_async(record)` → enqueues `GenerateRecordEmbedding` | ✅ Yes | +| `RecordRefreshAllMusicBrainzData` | `lib/music_library/worker/record_refresh_all_musicbrainz_data.ex` | `Records.Batch.refresh_musicbrainz_data()` → `Records.refresh_musicbrainz_data_async(record)` → enqueues `RecordRefreshMusicBrainzData` | ✅ Yes | + +The batch workers (lines 8-10 of each file) don't call `notify_update` directly — they delegate to the individual workers which do. **Each individual record gets its own broadcast.** ✅ + +--- + +## Acceptance Criterion #2: Non-Record-Modifying Workers + +**Status: ✅ ALL CONFIRMED — no spurious broadcasts.** + +Grep for `notify_update` and `broadcast_index_changed` across `lib/music_library/worker/` returns exactly the 6 calls documented in AC #1. No other worker emits record PubSub messages. + +| Worker | File | Modifies records? | Broadcasts? | +| --------------------------------- | ----------------------------------------------------------------- | ------------------- | ----------- | +| `ApplyScrobbleRules` | `lib/music_library/worker/apply_scrobble_rules.ex` | No — tracks only | ❌ No ✅ | +| `ArtistRefreshMusicBrainzData` | `lib/music_library/worker/artist_refresh_music_brainz_data.ex` | No — artist info | ❌ No ✅ | +| `ArtistRefreshAllDiscogsData` | `lib/music_library/worker/artist_refresh_all_discogs_data.ex` | No — artist info | ❌ No ✅ | +| `ArtistRefreshAllMusicBrainzData` | `lib/music_library/worker/artist_refresh_all_musicbrainz_data.ex` | No — artist info | ❌ No ✅ | +| `ArtistRefreshAllWikipediaData` | `lib/music_library/worker/artist_refresh_all_wikipedia_data.ex` | No — artist info | ❌ No ✅ | +| `ArtistRefreshDiscogsData` | `lib/music_library/worker/artist_refresh_discogs_data.ex` | No — artist info | ❌ No ✅ | +| `ArtistRefreshWikipediaData` | `lib/music_library/worker/artist_refresh_wikipedia_data.ex` | No — artist info | ❌ No ✅ | +| `BackfillScrobbledTracks` | `lib/music_library/worker/backfill_scrobbled_tracks.ex` | No — tracks only | ❌ No ✅ | +| `FetchArtistImage` | `lib/music_library/worker/fetch_artist_image.ex` | No — artist info | ❌ No ✅ | +| `FetchArtistInfo` | `lib/music_library/worker/fetch_artist_info.ex` | No — artist info | ❌ No ✅ | +| `FetchArtistLastFmData` | `lib/music_library/worker/fetch_artist_last_fm_data.ex` | No — artist info | ❌ No ✅ | +| `PruneArtistInfo` | `lib/music_library/worker/prune_artist_info.ex` | No — cleanup | ❌ No ✅ | +| `PruneAssetCache` | `lib/music_library/worker/prune_asset_cache.ex` | No — cleanup | ❌ No ✅ | +| `PruneAssets` | `lib/music_library/worker/prune_assets.ex` | No — cleanup | ❌ No ✅ | +| `RefreshScrobbles` | `lib/music_library/worker/refresh_scrobbles.ex` | No — tracks only | ❌ No ✅ | +| `RepoOptimize` | `lib/music_library/worker/repo_optimize.ex` | No — DB maintenance | ❌ No ✅ | +| `RepoVacuum` | `lib/music_library/worker/repo_vacuum.ex` | No — DB maintenance | ❌ No ✅ | +| `SendRecordsOnThisDayEmail` | `lib/music_library/worker/send_records_on_this_day_email.ex` | No — email | ❌ No ✅ | + +--- + +## Acceptance Criterion #3: Form Edit + Background Update Race + +**Status: ⚠️ MEDIUM SEVERITY — potential data loss.** + +### Race scenario trace + +``` +1. User opens RecordForm modal (live_action = :edit on CollectionLive.Show or WishlistLive.Show) + RecordForm rendered with @record = {genres: [], cover_hash: "abc"} + RecordForm's update/2 stores form via assign_new(:form, ...) + +2. Background worker (e.g., PopulateGenres) completes + Worker calls Records.notify_update(updated_record) with {genres: ["rock"], cover_hash: "abc"} + +3. PubSub broadcasts {:update, record} on "records:#{record.id}" + +4. Show LiveView's handle_info({:update, record}, socket) fires: + └─ RecordActions.handle_record_updated(record) + └─ assign(:record, record) ← overwrites @record! + └─ put_toast(:info, "Record updated in the background") ← user sees toast + +5. RecordForm's update/2 re-invoked with new @record + assign(assigns) ← updates @record to worker version + assign_new(:form, ...) ← skips: form already exists + +6. User edits title and clicks Save + handle_event("save", %{"record" => %{"title" => "New", "genres" => [], ...}}) + +7. save_record/3 calls: + Records.update_record(socket.assigns.record, params) + where params = %{"title" => "New", "genres" => [], "format" => "LP", ...} + params.genres == [] ← stale! worker's genres: ["rock"] is LOST +``` + +### Root cause + +The Show LiveView's `handle_info({:update, record})` does not check `live_action`: + +```elixir +# collection_live/show.ex:474-483 +def handle_info({:update, record}, socket) do + if record.id == socket.assigns.record.id do ← guards against wrong record + {:noreply, + socket + |> RecordActions.handle_record_updated(record) ← ALWAYS updates, even during :edit + |> assign_similar_records()} + else + {:noreply, socket} + end +end + +# wishlist_live/show.ex:367-373 — identical pattern +``` + +When `live_action == :edit`, the updated `@record` propagates to the RecordForm component via `update/2`. While `assign_new(:form, ...)` preserves the user's form fields, the underlying `@record` is now stale relative to the form data. On save, the stale params overwrite the worker's changes. + +### Impact assessment + +| Factor | Assessment | +| ------------------- | ------------------------------------------------------------------------------------------------------------------------------------- | +| **Probability** | LOW — workers are triggered by explicit user action, not periodic cron. User is unlikely to edit AND trigger a worker simultaneously. | +| **Impact** | MEDIUM — worker changes silently reverted without error. User might not notice. | +| **Detectability** | HIGH — toast "Record updated in the background" alerts user something changed. | +| **Affected fields** | `genres` (PopulateGenres), `musicbrainz_data` (RecordRefreshMusicBrainzData), `cover_hash` (RefreshCover) | + +### Recommended fix + +Add a `live_action` guard to skip `@record` assignment when the user is editing: + +**File:** `lib/music_library_web/live/collection_live/show.ex:474-483` + +```elixir +def handle_info({:update, record}, socket) do + if record.id == socket.assigns.record.id do + if socket.assigns.live_action in [:show] do + {:noreply, + socket + |> RecordActions.handle_record_updated(record) + |> assign_similar_records()} + else + # live_action == :edit: skip update to avoid overwriting form data. + # The worker's changes are already persisted. When the user saves or + # navigates back to show, handle_params will re-fetch the fresh record. + {:noreply, + put_toast(socket, :info, gettext("Record updated in the background. Your edits may be stale."))} + end + else + {:noreply, socket} + end +end +``` + +**Same change needed in:** `lib/music_library_web/live/wishlist_live/show.ex:367-373` + +--- + +## Acceptance Criterion #4: Double-Update Race Between Concurrent Workers + +**Status: ✅ SAFE — Ecto changeset field-level updates + SQLite serialization prevent data corruption.** + +### Why it's safe + +**1. Ecto changesets only UPDATE changed fields** + +The enrichment functions construct changesets that touch only specific fields: + +```elixir +# enrichment.ex:41-43 — PopulateGenres +record +|> Record.add_genres(response["genres"]) # changes: %{genres: [...]} +|> Repo.update() # UPDATE records SET genres = ... WHERE id = ... + +# enrichment.ex:51-53 — RefreshCover +record +|> Record.set_cover_hash(asset.hash) # changes: %{cover_hash: "xyz"} +|> Repo.update() # UPDATE records SET cover_hash = ... WHERE id = ... + +# enrichment.ex:115-118 — RefreshMusicBrainzData +record +|> Record.add_musicbrainz_data(data) # changes: %{musicbrainz_data: ...} +|> Repo.update() # UPDATE records SET musicbrainz_data = ... WHERE id = ... +``` + +Ecto's `Repo.update/1` with a changeset generates SQL that only includes columns in the changeset's `changes` map, **not** all cast fields. Two workers touching different columns produce non-overlapping UPDATEs. + +**2. SQLite serializes writes** + +SQLite uses a per-database write lock. Two concurrent writes are serialized (one waits for the other). The second write always sees the first write's committed state. + +**3. Both broadcasts arrive** + +Both workers broadcast `notify_update` after their respective commits. The Show LiveView processes both messages in order: + +``` +Worker A: commit → broadcast(record_A) +Worker B: commit → broadcast(record_B) + +Show LV: handle_info(record_A) → assign record_A +Show LV: handle_info(record_B) → assign record_B ← last wins, but DB has both changes +``` + +The final `socket.assigns.record` reflects the last broadcast (record_B), which was fetched by Worker B before Worker A committed. So the UI may momentarily lack Worker A's changes until the next full page load or PubSub refresh. This is a minor UI staleness issue, not data corruption. + +**4. Verifying no full-struct UPDATE** + +```elixir +# record.ex:134-152 +def changeset(record, attrs) do + record + |> cast(attrs, [ # cast: user-supplied params + :type, :format, :title, :musicbrainz_id, :musicbrainz_data, + :release_date, :genres, :release_ids, :selected_release_id, + :included_release_group_ids, :cover_url, :cover_hash, + :dominant_colors, :purchased_at + ]) + ... +end +``` + +`cast/3` only puts fields present in `attrs` into `changes`. The enrichment functions use `change/2` (not `cast/3`) with explicit field lists, so only targeted fields end up in the UPDATE. + +### Sequence diagram: Two workers, different fields + +``` +Worker A (PopulateGenres) Worker B (RefreshCover) SQLite DB + │ │ │ + │ get_record!(id) → v0 │ get_record!(id) → v0 │ {genres:[], cover:"abc"} + │ populate_genres(v0) │ download new cover │ + │ add_genres(["rock"]) │ set_cover_hash("xyz") │ + │ Repo.update(chg: genres) ──────────── wait for lock ──────────────► │ {genres:["rock"], cover:"abc"} + │ │ Repo.update(chg: cover) ──► │ {genres:["rock"], cover:"xyz"} + │ notify_update(record_A) │ notify_update(record_B) │ + │ record_A: {genres:["rock"]} │ record_B: {cover:"xyz"} │ + │ (no cover_hash field) │ (no genres field) │ +``` + +**Result:** DB has both changes. UI gets record_B (missing genres), but next `handle_params` reload corrects it. **No data loss.** ✅ + +--- + +## Acceptance Criterion #5: ArtistLive.Show and `{:update, record}` + +**Status: ✅ CONFIRMED CORRECT — ArtistLive.Show intentionally does not handle `{:update, record}`.** + +### Verification + +**1. No PubSub subscription for record topics** + +ArtistLive.Show's `mount/3` (line 498-500): + +```elixir +def mount(_params, _session, socket) do + {:ok, socket} +end +``` + +No `Records.subscribe/1` or `subscribe_to_index/0` call. The LiveView never receives `{:update, record}` messages. ✅ + +**2. handle_info clauses in ArtistLive.Show** + +```elixir +# line 493, 639 — only two clauses: +def handle_info({MusicLibraryWeb.Components.Chat, :chats_changed}, socket) +def handle_info({MusicLibraryWeb.ArtistLive.Form, {:saved, artist_info}}, socket) +``` + +No `{:update, record}` clause. Any such message that somehow arrived would be silently dropped by Phoenix.LiveView (unmatched messages are ignored, not crashed). ✅ + +**3. Artist update path** + +Artist updates use a distinct pub/sub path: + +``` +ArtistLive.Form (modal) → send(self(), {ArtistLive.Form, {:saved, artist_info}}) +ArtistLive.Show → handle_info({:saved, artist_info}) → assign(:artist_info, ...) +``` + +Records displayed in ArtistLive.Show's grids are refreshed via `assign_records/2` (which re-queries the DB) when the user triggers an action like `add-to-collection` or `delete`. No push-based record update. ✅ + +--- + +## Complete Worker Broadcast Matrix + +| # | Worker | File:Line | Broadcast | Fields Touched | Severity | +| --- | ----------------------------------- | --------------------------------------------- | --------------------------- | ----------------------------- | -------- | +| 1 | `RefreshCover` | `refresh_cover.ex:14` | `notify_update(updated)` | `cover_hash` | ✅ OK | +| 2 | `PopulateGenres` | `populate_genres.ex:15` | `notify_update(updated)` | `genres` | ✅ OK | +| 3 | `GenerateRecordEmbedding` | `generate_record_embedding.ex:16` | `notify_update(record)` | embedding (not record column) | ✅ OK | +| 4 | `RecordRefreshMusicBrainzData` | `record_refresh_music_brainz_data.ex:14` | `notify_update(updated)` | `musicbrainz_data` | ✅ OK | +| 5 | `ImportFromMusicbrainzRelease` | `import_from_musicbrainz_release.ex:24` | `broadcast_index_changed()` | new record insert | ✅ OK | +| 6 | `ImportFromMusicbrainzReleaseGroup` | `import_from_musicbrainz_release_group.ex:27` | `broadcast_index_changed()` | new record insert | ✅ OK | +| 7 | `RecordGenerateAllEmbeddings` | (delegates to GenerateRecordEmbedding) | per-record `notify_update` | embedding | ✅ OK | +| 8 | `RecordRefreshAllMusicBrainzData` | (delegates to RecordRefreshMusicBrainzData) | per-record `notify_update` | `musicbrainz_data` | ✅ OK | + +All 8 record-modifying workers verified. ✅ + +--- + +## Recommendations + +| # | Finding | Severity | Recommendation | +| --- | ---------------------------------------------------------------------------------------------------------------------------------------------- | ---------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| 1 | Form edit + background update race — `handle_info({:update, record})` overwrites `@record` during `:edit`, causing potential data loss on save | **MEDIUM** | Add `live_action == :show` guard to `handle_info({:update, record})` in both `CollectionLive.Show` (line 474) and `WishlistLive.Show` (line 367). Skip `assign(:record, ...)` during `:edit`. Show warning toast instead. The fresh record will be loaded on next `handle_params` when user navigates away from edit. | +| 2 | Double-update broadcasts cause brief UI staleness | ℹ️ INFO | Harmless — last broadcast wins on socket, but DB has all changes. Next `handle_params` reload fetches everything. No fix needed. | +| 3 | Batch workers (`RecordGenerateAllEmbeddings`, `RecordRefreshAllMusicBrainzData`) don't call `notify_update` directly | ℹ️ INFO | By design — they delegate to individual workers which broadcast per record. No fix needed. | diff --git a/backlog/tasks/ml-169.1 - Phase-3-Concurrent-state-change-safety-audit.md b/backlog/tasks/ml-169.1 - Phase-3-Concurrent-state-change-safety-audit.md index d842fe69..533d41e3 100644 --- a/backlog/tasks/ml-169.1 - Phase-3-Concurrent-state-change-safety-audit.md +++ b/backlog/tasks/ml-169.1 - Phase-3-Concurrent-state-change-safety-audit.md @@ -1,10 +1,10 @@ --- id: ML-169.1 title: "Phase 3: Concurrent state change safety audit" -status: To Do +status: In Progress assignee: [] created_date: "2026-05-08 08:59" -updated_date: "2026-05-11 06:46" +updated_date: "2026-05-19 07:28" labels: - audit - ready @@ -13,6 +13,10 @@ references: - lib/music_library/worker/ - lib/music_library/records.ex - lib/music_library_web/components/record_form.ex +modified_files: + - >- + docs/audits/phase3-concurrent-state-safety/doc-26 - + Audit-Report-Concurrent-State-Change-Safety-Phase-3.md parent_task_id: ML-169 priority: medium --- @@ -34,10 +38,25 @@ Key risks to investigate: -- [ ] #1 Every record-modifying Oban worker audited for notify_update/1 call after success (RefreshCover, PopulateGenres, GenerateRecordEmbedding, RecordRefreshMusicBrainzData, ImportFromMusicbrainzRelease, ImportFromMusicbrainzReleaseGroup, all RecordAll\* batch workers) -- [ ] #2 Workers that do NOT modify records confirmed as correctly NOT broadcasting (PruneAssets, ApplyScrobbleRules, all ArtistRefresh\*, etc.) -- [ ] #3 Form edit + background update race evaluated: trace what happens when RecordForm holds stale @record assign while worker broadcasts {:update, record} to parent Show page -- [ ] #4 Double-update race evaluated: confirm both handle_info calls apply safely (workers touch different fields, second wins) -- [ ] #5 ArtistLive.Show confirmed as intentionally not handling {:update, record} (artist updates use ArtistLive.Form → {:saved, artist_info} path) -- [ ] #6 Findings report written as a Backlog.md document with file:line references, severity ratings, and fix recommendations +- [x] #1 Every record-modifying Oban worker audited for notify_update/1 call after success (RefreshCover, PopulateGenres, GenerateRecordEmbedding, RecordRefreshMusicBrainzData, ImportFromMusicbrainzRelease, ImportFromMusicbrainzReleaseGroup, all RecordAll\* batch workers) +- [x] #2 Workers that do NOT modify records confirmed as correctly NOT broadcasting (PruneAssets, ApplyScrobbleRules, all ArtistRefresh\*, etc.) +- [x] #3 Form edit + background update race evaluated: trace what happens when RecordForm holds stale @record assign while worker broadcasts {:update, record} to parent Show page +- [x] #4 Double-update race evaluated: confirm both handle_info calls apply safely (workers touch different fields, second wins) +- [x] #5 ArtistLive.Show confirmed as intentionally not handling {:update, record} (artist updates use ArtistLive.Form → {:saved, artist_info} path) +- [x] #6 Findings report written as a Backlog.md document with file:line references, severity ratings, and fix recommendations + +## Implementation Notes + + + +Audit complete. Report written at docs/audits/phase3-concurrent-state-safety/doc-26. + +Key findings: + +- All 8 record-modifying workers correctly broadcast notify_update or broadcast_index_changed +- 18 non-record workers confirmed as correctly NOT broadcasting +- MEDIUM finding: form edit + background update race — handle_info({:update, record}) overwrites @record during :edit. Recommended fix: add live_action guard to both Show LiveViews. +- Double-update race is SAFE: Ecto changesets only UPDATE SET changed fields, SQLite serializes writes +- ArtistLive.Show confirmed intentionally not handling {:update, record} +