From 4e2986e19cedf803a8bbf3b559ac77b00016d48f Mon Sep 17 00:00:00 2001 From: Caleb Allen Date: Fri, 27 Feb 2026 14:50:27 -0500 Subject: [PATCH] docs(01): create phase plan for concurrency race fixes --- .planning/ROADMAP.md | 6 +- .../01-concurrency-race-fixes/01-01-PLAN.md | 334 ++++++++++++++++++ 2 files changed, 338 insertions(+), 2 deletions(-) create mode 100644 .planning/phases/01-concurrency-race-fixes/01-01-PLAN.md diff --git a/.planning/ROADMAP.md b/.planning/ROADMAP.md index ca512c3..a6a3999 100644 --- a/.planning/ROADMAP.md +++ b/.planning/ROADMAP.md @@ -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 | — | diff --git a/.planning/phases/01-concurrency-race-fixes/01-01-PLAN.md b/.planning/phases/01-concurrency-race-fixes/01-01-PLAN.md new file mode 100644 index 0000000..404618b --- /dev/null +++ b/.planning/phases/01-concurrency-race-fixes/01-01-PLAN.md @@ -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" +--- + + +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. + + + +@/home/caleb/.config/opencode/get-shit-done/workflows/execute-plan.md +@/home/caleb/.config/opencode/get-shit-done/templates/summary.md + + + +@.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 + + + + +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 +} +``` + + + + + + + Task 1: Add mutex protection to Queue, Library, and Playlist SetContext methods + + backend/queue/queue.go + backend/library/library.go + backend/playlist/playlist.go + + +**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. + + + cd /mnt/vault/dev/golang/yellowjacket && go build ./backend/queue/ ./backend/library/ ./backend/playlist/ + + + - 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 + + + + + Task 2: Collapse Player.SetContext double-lock into single acquisition + backend/player/player.go + +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`. + + + cd /mnt/vault/dev/golang/yellowjacket && go build ./backend/player/ + + + - 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 + + + + + + +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/ +``` + + + +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 + + + +After completion, create `.planning/phases/01-concurrency-race-fixes/01-01-SUMMARY.md` +