docs(01): create phase plan for concurrency race fixes

2026-02-27 14:50:27 -05:00
parent c086215533
commit 4e2986e19c
2 changed files with 338 additions and 2 deletions
@@ -27,7 +27,9 @@
  2. Queue.SetContext(), Library.SetContext(), and Playlist.Service.SetContext() each acquire their mutex before writing the ctx field
  3. Player.SetContext() uses a single lock acquisition instead of the double-lock pattern
  4. Concurrent calls to SetContext from multiple goroutines do not corrupt shared state
-**Plans:** TBD
+**Plans:** 1 plan
+Plans:
+- [ ] 01-01-PLAN.md — Add mutex protection to all SetContext methods and collapse Player double-lock

 ### Phase 2: Backend Correctness
 **Goal:** All known error handling gaps are closed, configuration is secure, and the backend reports problems honestly instead of swallowing them
@@ -110,7 +112,7 @@

 | Phase | Plans Complete | Status | Completed |
 |-------|----------------|--------|-----------|
-| 1. Concurrency Race Fixes | 0/? | Not started | — |
+| 1. Concurrency Race Fixes | 0/1 | Planned | — |
 | 2. Backend Correctness | 0/? | Not started | — |
 | 3. Test Infrastructure | 0/? | Not started | — |
 | 4. Queue, Config & Player Tests | 0/? | Not started | — |
@@ -0,0 +1,334 @@
+---
+phase: 01-concurrency-race-fixes
+plan: 01
+type: execute
+wave: 1
+depends_on: []
+files_modified:
+  - backend/queue/queue.go
+  - backend/library/library.go
+  - backend/playlist/playlist.go
+  - backend/player/player.go
+autonomous: true
+requirements:
+  - CORR-01
+  - CORR-02
+  - CORR-03
+  - CORR-04
+
+must_haves:
+  truths:
+    - "Queue.SetContext() acquires q.mu before writing q.ctx"
+    - "Library.SetContext() and SetRescanHooks() acquire a mutex before writing fields"
+    - "Playlist.Service.SetContext() acquires a mutex before writing s.ctx"
+    - "Player.SetContext() uses a single lock acquisition instead of double-lock"
+    - "Running go test -race on all four packages produces zero data race reports for SetContext"
+  artifacts:
+    - path: "backend/queue/queue.go"
+      provides: "Race-free Queue.SetContext"
+      contains: "q.mu.Lock"
+    - path: "backend/library/library.go"
+      provides: "Race-free Library.SetContext and SetRescanHooks with struct-level mutex"
+      contains: "l.mu.Lock"
+    - path: "backend/playlist/playlist.go"
+      provides: "Race-free Service.SetContext with struct-level mutex"
+      contains: "s.mu.Lock"
+    - path: "backend/player/player.go"
+      provides: "Single-lock Player.SetContext"
+      contains: "p.restoreStateLocked"
+  key_links:
+    - from: "backend/queue/queue.go:SetContext"
+      to: "backend/queue/emit.go:emitQueueChanged"
+      via: "Both read q.ctx under q.mu"
+      pattern: "q\\.mu\\.Lock.*q\\.ctx"
+    - from: "backend/library/library.go:SetContext"
+      to: "backend/library/library.go:registerEventHandlers"
+      via: "SetContext acquires l.mu then calls registerEventHandlers after release"
+      pattern: "l\\.mu\\.Lock.*l\\.ctx"
+    - from: "backend/playlist/playlist.go:SetContext"
+      to: "backend/playlist/playlist.go:emitEvent"
+      via: "Both access s.ctx under s.mu"
+      pattern: "s\\.mu\\.Lock.*s\\.ctx"
+---
+
+<objective>
+Eliminate all SetContext data races across Queue, Library, Playlist, and Player packages.
+
+Purpose: These four SetContext methods write struct fields without proper synchronization, creating data races detectable by `go test -race`. Fixing them makes the codebase race-clean for all subsequent test phases.
+
+Output: Four modified Go files with mutex-protected SetContext implementations.
+</objective>
+
+<execution_context>
+@/home/caleb/.config/opencode/get-shit-done/workflows/execute-plan.md
+@/home/caleb/.config/opencode/get-shit-done/templates/summary.md
+</execution_context>
+
+<context>
+@.planning/ROADMAP.md
+@.planning/STATE.md
+@.planning/codebase/CONVENTIONS.md
+@.planning/codebase/CONCERNS.md
+
+@backend/queue/queue.go
+@backend/queue/emit.go
+@backend/library/library.go
+@backend/playlist/playlist.go
+@backend/player/player.go
+
+<interfaces>
+<!-- Key types and mutex patterns the executor needs. Extracted from codebase. -->
+
+From backend/queue/queue.go (lines 104-122):
+```go
+type Queue struct {
+	ctx    context.Context
+	logger *slog.Logger
+	db     *database.DB
+	player TrackLoader
+
+	mu               sync.Mutex
+	tracks           []Track
+	currentIndex     int
+	shuffleMode      bool
+	repeatMode       RepeatMode
+	shuffleOrder     []int
+	sourcePlaylistID int64
+
+	setQueueGen atomic.Int64
+}
+```
+
+From backend/library/library.go (lines 77-84):
+```go
+type Library struct {
+	ctx         context.Context
+	logger      *slog.Logger
+	conf        *Config
+	db          *database.DB
+	rescanHooks RescanHooks
+}
+// NOTE: No struct-level mutex exists. Must add one.
+```
+
+From backend/playlist/playlist.go (lines 97-104):
+```go
+type Service struct {
+	ctx           context.Context
+	logger        *slog.Logger
+	db            *database.DB
+	libraryDir    LibraryDirProvider
+	favoritesConf FavoritesConfigProvider
+}
+// NOTE: No mutex exists. Must add one.
+```
+
+From backend/player/player.go (lines 30-40, 163-171):
+```go
+type Player struct {
+	mu sync.Mutex
+	ctx context.Context
+	// ... other fields
+}
+
+// Current double-lock SetContext:
+func (p *Player) SetContext(ctx context.Context) {
+	p.mu.Lock()
+	p.ctx = ctx
+	p.mu.Unlock()
+
+	p.mu.Lock()
+	p.restoreStateLocked()
+	p.mu.Unlock()
+}
+```
+
+Codebase mutex convention (from CONVENTIONS.md):
+```go
+// Public method acquires lock:
+func (p *Player) Play() error {
+	p.mu.Lock()
+	defer p.mu.Unlock()
+	// ...
+}
+
+// Internal helper — caller must hold p.mu:
+func (p *Player) loadFileLocked(filePath string) error {
+	// no lock acquired here
+}
+```
+</interfaces>
+</context>
+
+<tasks>
+
+<task type="auto">
+  <name>Task 1: Add mutex protection to Queue, Library, and Playlist SetContext methods</name>
+  <files>
+    backend/queue/queue.go
+    backend/library/library.go
+    backend/playlist/playlist.go
+  </files>
+  <action>
+**Queue (backend/queue/queue.go):**
+
+In `SetContext()` (line 134), wrap the `q.ctx = ctx` assignment with the existing `q.mu`:
+
+```go
+func (q *Queue) SetContext(ctx context.Context) {
+	q.mu.Lock()
+	defer q.mu.Unlock()
+
+	q.ctx = ctx
+}
+```
+
+No other changes needed — `q.mu` already exists in the struct, and all emit methods that read `q.ctx` are called from methods that hold `q.mu`.
+
+**Library (backend/library/library.go):**
+
+1. Add a `mu sync.Mutex` field to the `Library` struct (line 78 area), placed as the first field to follow the player convention. Add a doc comment explaining it protects `ctx`, `conf`, and `rescanHooks`.
+
+2. Update `SetContext()` (line 120) to acquire `l.mu` before writing `l.ctx`, then release before calling `l.registerEventHandlers()` (which itself calls `runtime.EventsOn` — should not hold the mutex during potentially blocking Wails calls):
+
+```go
+func (l *Library) SetContext(ctx context.Context) {
+	l.mu.Lock()
+	l.ctx = ctx
+	l.mu.Unlock()
+
+	l.registerEventHandlers()
+}
+```
+
+3. Update `SetRescanHooks()` (line 88) to acquire `l.mu`:
+
+```go
+func (l *Library) SetRescanHooks(h RescanHooks) {
+	l.mu.Lock()
+	defer l.mu.Unlock()
+
+	l.rescanHooks = h
+}
+```
+
+Do NOT add mutex to scan-internal paths — the scan methods run single-threaded after startup. Only protect the fields that are written by setter methods called during initialization.
+
+**Playlist (backend/playlist/playlist.go):**
+
+1. Add a `mu sync.Mutex` field to the `Service` struct (line 98 area), placed before `ctx`. Import `"sync"` if not already imported.
+
+2. Update `SetContext()` (line 130) to acquire `s.mu` before writing `s.ctx`, then release before calling `s.migrateExistingPlaylists()`:
+
+```go
+func (s *Service) SetContext(ctx context.Context) {
+	s.mu.Lock()
+	s.ctx = ctx
+	s.mu.Unlock()
+
+	s.migrateExistingPlaylists()
+}
+```
+
+3. Update `SetFavoritesConfig()` (line 121) to acquire `s.mu`:
+
+```go
+func (s *Service) SetFavoritesConfig(
+	provider FavoritesConfigProvider,
+) {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+
+	s.favoritesConf = provider
+}
+```
+
+For all three packages: follow existing codebase conventions — `sync.Mutex` named `mu`, `Lock()/defer Unlock()` for simple setters, explicit `Lock()/Unlock()` when code after the critical section should run without the lock.
+  </action>
+  <verify>
+    <automated>cd /mnt/vault/dev/golang/yellowjacket && go build ./backend/queue/ ./backend/library/ ./backend/playlist/</automated>
+  </verify>
+  <done>
+    - Queue.SetContext acquires q.mu before writing q.ctx
+    - Library struct has a mu sync.Mutex field; SetContext and SetRescanHooks acquire it
+    - Playlist Service struct has a mu sync.Mutex field; SetContext and SetFavoritesConfig acquire it
+    - All three packages compile without errors
+  </done>
+</task>
+
+<task type="auto">
+  <name>Task 2: Collapse Player.SetContext double-lock into single acquisition</name>
+  <files>backend/player/player.go</files>
+  <action>
+Replace the current double-lock `SetContext()` (lines 163-171):
+
+```go
+func (p *Player) SetContext(ctx context.Context) {
+	p.mu.Lock()
+	p.ctx = ctx
+	p.mu.Unlock()
+
+	p.mu.Lock()
+	p.restoreStateLocked()
+	p.mu.Unlock()
+}
+```
+
+With a single lock acquisition:
+
+```go
+func (p *Player) SetContext(ctx context.Context) {
+	p.mu.Lock()
+	defer p.mu.Unlock()
+
+	p.ctx = ctx
+	p.restoreStateLocked()
+}
+```
+
+This is safe because `restoreStateLocked()` is documented as requiring `p.mu` to be held (the `Locked` suffix convention), and combining the operations prevents another goroutine from observing a partially-initialized state (ctx set but state not yet restored).
+
+WARNING: Do NOT change any other Player methods. Do NOT alter lock ordering between `p.mu` and `speaker.Lock()`. The player's lock-sensitive paths are fragile and this change is scoped only to `SetContext`.
+  </action>
+  <verify>
+    <automated>cd /mnt/vault/dev/golang/yellowjacket && go build ./backend/player/</automated>
+  </verify>
+  <done>
+    - Player.SetContext uses a single p.mu.Lock()/defer p.mu.Unlock() call
+    - p.ctx assignment and p.restoreStateLocked() both run under the same lock hold
+    - Player package compiles without errors
+  </done>
+</task>
+
+</tasks>
+
+<verification>
+After both tasks complete, run the full verification:
+
+```bash
+# 1. All four packages compile
+go build ./backend/queue/ ./backend/library/ ./backend/playlist/ ./backend/player/
+
+# 2. Existing tests still pass (with race detector)
+go test -race -count=1 ./backend/player/ ./backend/playlist/ ./backend/coverart/ ./backend/metadata/...
+
+# 3. Vet passes on modified packages
+go vet ./backend/queue/ ./backend/library/ ./backend/playlist/ ./backend/player/
+
+# 4. Lint passes (if golangci-lint available)
+golangci-lint run ./backend/queue/ ./backend/library/ ./backend/playlist/ ./backend/player/
+```
+</verification>
+
+<success_criteria>
+1. All four SetContext methods acquire their respective mutex before writing the ctx field
+2. Library and Playlist structs have new `mu sync.Mutex` fields
+3. Player.SetContext uses exactly one Lock/Unlock pair instead of two
+4. `go build` succeeds on all four packages
+5. `go test -race` on existing test files produces zero race reports
+6. `go vet` reports no issues on modified packages
+</success_criteria>
+
+<output>
+After completion, create `.planning/phases/01-concurrency-race-fixes/01-01-SUMMARY.md`
+</output>