cloud/music_library
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

ML-169.1: performed concurrent state change safety audit

Browse Source
This commit is contained in:
Claudio Ortolina
2026-05-19 08:33:55 +01:00
parent 2d4b82e7bb
commit 2a108660ae
2 changed files with 391 additions and 8 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backlog/docs/audits/phase3-concurrent-state-safety/doc-26 - Audit-Report-Concurrent-State-Change-Safety-Phase-3.md
+364
View File
@@ -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. |
backlog/tasks/ml-169.1 - Phase-3-Concurrent-state-change-safety-audit.md
+27 -8
View File
@@ -1,10 +1,10 @@
--- ---
id: ML-169.1 id: ML-169.1
title: "Phase 3: Concurrent state change safety audit" title: "Phase 3: Concurrent state change safety audit"
status: To Do status: In Progress
assignee: [] assignee: []
created_date: "2026-05-08 08:59" created_date: "2026-05-08 08:59"
updated_date: "2026-05-11 06:46" updated_date: "2026-05-19 07:28"
labels: labels:
- audit - audit
- ready - ready
@@ -13,6 +13,10 @@ references:
- lib/music_library/worker/ - lib/music_library/worker/
- lib/music_library/records.ex - lib/music_library/records.ex
- lib/music_library_web/components/record_form.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 parent_task_id: ML-169
priority: medium priority: medium
--- ---
@@ -34,10 +38,25 @@ Key risks to investigate:
<!-- AC:BEGIN --> <!-- AC:BEGIN -->
- [ ] #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] #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.) - [x] #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 - [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
- [ ] #4 Double-update race evaluated: confirm both handle_info calls apply safely (workers touch different fields, second wins) - [x] #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) - [x] #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] #6 Findings report written as a Backlog.md document with file:line references, severity ratings, and fix recommendations
<!-- AC:END --> <!-- AC:END -->
## Implementation Notes
<!-- SECTION:NOTES:BEGIN -->
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}
<!-- SECTION:NOTES:END -->