docs(02): create phase plan for backend correctness
This commit is contained in:
@@ -41,7 +41,10 @@ Plans:
|
||||
3. MPRIS lifecycle callback errors (Pause, Seek) appear in the application log instead of being silently discarded
|
||||
4. Artist credit link creation checks the actual error — only UNIQUE constraint violations are ignored, all other errors are surfaced
|
||||
5. Library.Scan() returns warnings (skipped files, partial failures) in ScanMetrics and fatal errors (database failures) in the error return, so callers can distinguish between "scan completed with issues" and "scan failed"
|
||||
**Plans:** TBD
|
||||
**Plans:** 2 plans
|
||||
Plans:
|
||||
- [ ] 02-01-PLAN.md — Fix startupErr global state, config permissions, and MPRIS callback error logging
|
||||
- [ ] 02-02-PLAN.md — Add IsUniqueViolation helper, migration 3, and separate scan warnings from fatal errors
|
||||
|
||||
### Phase 3: Test Infrastructure
|
||||
**Goal:** A reliable, production-mirroring test foundation exists so that all subsequent test phases can write database-backed tests with confidence
|
||||
@@ -113,7 +116,7 @@ Plans:
|
||||
| Phase | Plans Complete | Status | Completed |
|
||||
|-------|----------------|--------|-----------|
|
||||
| 1. Concurrency Race Fixes | 1/1 | Complete | 2026-02-28 |
|
||||
| 2. Backend Correctness | 0/? | Not started | — |
|
||||
| 2. Backend Correctness | 0/2 | Planned | — |
|
||||
| 3. Test Infrastructure | 0/? | Not started | — |
|
||||
| 4. Queue, Config & Player Tests | 0/? | Not started | — |
|
||||
| 5. Database & Library Tests | 0/? | Not started | — |
|
||||
@@ -123,4 +126,4 @@ Plans:
|
||||
|
||||
---
|
||||
*Roadmap created: 2026-02-27*
|
||||
*Last updated: 2026-02-28*
|
||||
*Last updated: 2026-03-02*
|
||||
|
||||
@@ -0,0 +1,220 @@
|
||||
---
|
||||
phase: 02-backend-correctness
|
||||
plan: 01
|
||||
type: execute
|
||||
wave: 1
|
||||
depends_on: []
|
||||
files_modified:
|
||||
- backend/app.go
|
||||
- backend/config/config.go
|
||||
autonomous: true
|
||||
requirements: [CORR-05, CORR-06, CORR-07]
|
||||
|
||||
must_haves:
|
||||
truths:
|
||||
- "Package-level startupErr variable no longer exists; startup errors are stored in a YellowJacketApp struct field"
|
||||
- "Config files are written with 0o644 permissions"
|
||||
- "MPRIS callback errors (Pause, Seek) appear in the application log instead of being silently discarded"
|
||||
artifacts:
|
||||
- path: "backend/app.go"
|
||||
provides: "Startup error as struct field + MPRIS error logging"
|
||||
contains: "startupErr error"
|
||||
- path: "backend/config/config.go"
|
||||
provides: "Secure config file permissions"
|
||||
contains: "0o644"
|
||||
key_links:
|
||||
- from: "backend/app.go:OnStartup"
|
||||
to: "backend/app.go:OnDomReady"
|
||||
via: "yj.startupErr field (not package-level var)"
|
||||
pattern: "yj\\.startupErr"
|
||||
- from: "backend/app.go:MPRIS callbacks"
|
||||
to: "yj.logger"
|
||||
via: "Warn log on Pause/Seek error"
|
||||
pattern: "yj\\.logger\\.Warn.*MPRIS"
|
||||
---
|
||||
|
||||
<objective>
|
||||
Fix three independent error handling gaps in the application shell and config layer: eliminate the package-level startupErr variable, secure config file permissions, and log MPRIS callback errors.
|
||||
|
||||
Purpose: Remove global mutable state (startupErr), prevent world-writable config files, and ensure MPRIS failures are observable in logs.
|
||||
Output: Modified `backend/app.go` and `backend/config/config.go` with all three fixes applied.
|
||||
</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/phases/02-backend-correctness/02-CONTEXT.md
|
||||
@.planning/phases/02-backend-correctness/02-RESEARCH.md
|
||||
|
||||
@backend/app.go
|
||||
@backend/config/config.go
|
||||
|
||||
<interfaces>
|
||||
<!-- Key types and contracts the executor needs. Extracted from codebase. -->
|
||||
|
||||
From backend/app.go:
|
||||
```go
|
||||
// YellowJacketApp is the main application struct for Wails.
|
||||
type YellowJacketApp struct {
|
||||
FEBindings []any
|
||||
FrontendUtil *frontendutil.FrontendUtil
|
||||
|
||||
logger *slog.Logger
|
||||
assetHandler *assets.Handler
|
||||
database *database.DB
|
||||
library *library.Library
|
||||
player *player.Player
|
||||
playlist *playlist.Service
|
||||
queue *queue.Queue
|
||||
mediaControls mediacontrols.Handler
|
||||
appContext context.Context
|
||||
appConfig *config.Config
|
||||
}
|
||||
|
||||
var startupErr error // line 134 — TO BE REMOVED
|
||||
|
||||
func (yj *YellowJacketApp) OnStartup(ctx context.Context) // line 137 — uses startupErr
|
||||
func (yj *YellowJacketApp) OnDomReady(ctx context.Context) // line 251 — checks startupErr
|
||||
```
|
||||
|
||||
MPRIS callback closures at lines 181-203:
|
||||
```go
|
||||
OnPause: func() { _ = yj.player.Pause() },
|
||||
OnPlayPause: func() {
|
||||
if yj.player.IsPlaying() {
|
||||
_ = yj.player.Pause()
|
||||
} else {
|
||||
yj.queue.Play()
|
||||
}
|
||||
},
|
||||
OnStop: func() { _ = yj.player.Pause() },
|
||||
OnSeek: func(positionSec int) {
|
||||
_ = yj.player.Seek(positionSec)
|
||||
},
|
||||
```
|
||||
</interfaces>
|
||||
</context>
|
||||
|
||||
<tasks>
|
||||
|
||||
<task type="auto">
|
||||
<name>Task 1: Move startupErr to struct field and fix config permissions</name>
|
||||
<files>backend/app.go, backend/config/config.go</files>
|
||||
<action>
|
||||
**CORR-05 — Startup error struct field (backend/app.go):**
|
||||
1. Add `startupErr error` field to the `YellowJacketApp` struct (after `appConfig`)
|
||||
2. Delete the package-level `var startupErr error` declaration at line 134
|
||||
3. In `OnStartup` (line 154-155): change `startupErr = errors.Join(startupErr, ...)` to `yj.startupErr = errors.Join(yj.startupErr, ...)`
|
||||
4. In `OnDomReady` (line 252-254): change `if startupErr != nil` to `if yj.startupErr != nil`, and `startupErr.Error()` to `yj.startupErr.Error()`
|
||||
5. Verify no other references to the package-level `startupErr` exist
|
||||
|
||||
**CORR-06 — Config permissions (backend/config/config.go):**
|
||||
1. At line 152, change `os.FileMode(int(0o666))` to `0o644`
|
||||
2. This is a single expression replacement — the `os.WriteFile` call signature stays the same
|
||||
</action>
|
||||
<verify>
|
||||
<automated>cd /mnt/vault/dev/golang/yellowjacket && go vet ./backend/... && grep -q "startupErr error" backend/app.go && ! grep -q "^var startupErr" backend/app.go && grep -q "0o644" backend/config/config.go && ! grep -q "0o666" backend/config/config.go</automated>
|
||||
</verify>
|
||||
<done>Package-level startupErr is gone; YellowJacketApp has startupErr field; OnStartup and OnDomReady reference yj.startupErr; config.go writes with 0o644 permissions</done>
|
||||
</task>
|
||||
|
||||
<task type="auto">
|
||||
<name>Task 2: Log MPRIS callback errors</name>
|
||||
<files>backend/app.go</files>
|
||||
<action>
|
||||
**CORR-07 — MPRIS callback error logging (backend/app.go):**
|
||||
|
||||
Replace the four MPRIS closures (lines 183-195) that discard errors with closures that log on failure. Use `Warn` level per research recommendation — these are non-fatal conditions. Keep inline closures (no named method extraction).
|
||||
|
||||
1. **OnPause** (line 183): Replace `func() { _ = yj.player.Pause() }` with:
|
||||
```go
|
||||
func() {
|
||||
if err := yj.player.Pause(); err != nil {
|
||||
yj.logger.Warn("MPRIS Pause failed", "err", err)
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
2. **OnPlayPause** (lines 184-189): Replace the `_ = yj.player.Pause()` inside the `if yj.player.IsPlaying()` branch:
|
||||
```go
|
||||
func() {
|
||||
if yj.player.IsPlaying() {
|
||||
if err := yj.player.Pause(); err != nil {
|
||||
yj.logger.Warn("MPRIS PlayPause(pause) failed", "err", err)
|
||||
}
|
||||
} else {
|
||||
yj.queue.Play()
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
3. **OnStop** (line 191): Replace `func() { _ = yj.player.Pause() }` with:
|
||||
```go
|
||||
func() {
|
||||
if err := yj.player.Pause(); err != nil {
|
||||
yj.logger.Warn("MPRIS Stop failed", "err", err)
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
4. **OnSeek** (lines 194-196): Replace `func(positionSec int) { _ = yj.player.Seek(positionSec) }` with:
|
||||
```go
|
||||
func(positionSec int) {
|
||||
if err := yj.player.Seek(positionSec); err != nil {
|
||||
yj.logger.Warn("MPRIS Seek failed", "err", err)
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
Ensure all four closures no longer use `_ =` to discard errors.
|
||||
</action>
|
||||
<verify>
|
||||
<automated>cd /mnt/vault/dev/golang/yellowjacket && go vet ./backend/... && ! grep -q '_ = yj.player.Pause()' backend/app.go && ! grep -q '_ = yj.player.Seek' backend/app.go && grep -c 'MPRIS.*failed' backend/app.go | grep -q '^4$'</automated>
|
||||
</verify>
|
||||
<done>All four MPRIS callbacks (OnPause, OnPlayPause, OnStop, OnSeek) check errors and log at Warn level; no discarded errors remain in MPRIS closures</done>
|
||||
</task>
|
||||
|
||||
</tasks>
|
||||
|
||||
<verification>
|
||||
```bash
|
||||
# All backend packages compile and pass vet
|
||||
go vet ./backend/...
|
||||
|
||||
# No package-level startupErr
|
||||
! grep -q "^var startupErr" backend/app.go
|
||||
|
||||
# Struct field exists
|
||||
grep -q "startupErr error" backend/app.go
|
||||
|
||||
# Config permissions fixed
|
||||
grep -q "0o644" backend/config/config.go
|
||||
! grep -q "0o666" backend/config/config.go
|
||||
|
||||
# MPRIS errors logged (4 occurrences)
|
||||
test "$(grep -c 'MPRIS.*failed' backend/app.go)" -eq 4
|
||||
|
||||
# No discarded player errors in MPRIS closures
|
||||
! grep -q '_ = yj.player' backend/app.go
|
||||
|
||||
# Linting passes
|
||||
golangci-lint run ./backend/...
|
||||
```
|
||||
</verification>
|
||||
|
||||
<success_criteria>
|
||||
- `go vet ./backend/...` passes
|
||||
- `golangci-lint run ./backend/...` passes
|
||||
- Package-level `startupErr` variable eliminated
|
||||
- Config file written with 0o644 permissions
|
||||
- All four MPRIS callbacks log errors at Warn level
|
||||
</success_criteria>
|
||||
|
||||
<output>
|
||||
After completion, create `.planning/phases/02-backend-correctness/02-01-SUMMARY.md`
|
||||
</output>
|
||||
@@ -0,0 +1,433 @@
|
||||
---
|
||||
phase: 02-backend-correctness
|
||||
plan: 02
|
||||
type: execute
|
||||
wave: 1
|
||||
depends_on: []
|
||||
files_modified:
|
||||
- backend/database/errors.go
|
||||
- backend/database/database.go
|
||||
- backend/library/metrics.go
|
||||
- backend/library/library.go
|
||||
autonomous: true
|
||||
requirements: [CORR-08, CORR-09]
|
||||
|
||||
must_haves:
|
||||
truths:
|
||||
- "Artist credit link creation checks the actual error — only UNIQUE constraint violations are ignored, all other errors are surfaced as scan warnings"
|
||||
- "Library.Scan() returns warnings (skipped files, partial failures) in ScanMetrics.Warnings and fatal errors (database failures) in the error return"
|
||||
- "Callers like handleConfigUpdate log warnings at Warn level and only propagate fatal errors"
|
||||
artifacts:
|
||||
- path: "backend/database/errors.go"
|
||||
provides: "IsUniqueViolation helper for SQLite constraint detection"
|
||||
exports: ["IsUniqueViolation"]
|
||||
- path: "backend/database/database.go"
|
||||
provides: "Migration 3: UNIQUE index on artist_credit_artist(artist_id, credit_id)"
|
||||
contains: "migration 3"
|
||||
- path: "backend/library/metrics.go"
|
||||
provides: "ScanWarning struct and addWarning method on ScanMetrics"
|
||||
contains: "ScanWarning"
|
||||
- path: "backend/library/library.go"
|
||||
provides: "Reclassified error paths in Scan() and updated cachedLinkArtist"
|
||||
contains: "metrics.addWarning"
|
||||
key_links:
|
||||
- from: "backend/library/library.go:cachedLinkArtist"
|
||||
to: "backend/database/errors.go:IsUniqueViolation"
|
||||
via: "Error check on CreateArtistCreditArtist result"
|
||||
pattern: "database\\.IsUniqueViolation"
|
||||
- from: "backend/library/library.go:Scan"
|
||||
to: "backend/library/metrics.go:addWarning"
|
||||
via: "Non-fatal errors reclassified as warnings"
|
||||
pattern: "metrics\\.addWarning"
|
||||
- from: "backend/database/database.go:runMigrations"
|
||||
to: "artist_credit_artist table"
|
||||
via: "Migration 3 adds UNIQUE index"
|
||||
pattern: "idx_artist_credit_artist_unique"
|
||||
---
|
||||
|
||||
<objective>
|
||||
Add proper error checking to artist credit link creation and separate library scan warnings from fatal errors. This involves creating a SQLite UNIQUE constraint helper, adding a schema migration, introducing a structured warning type to ScanMetrics, and reclassifying non-fatal scan errors as warnings.
|
||||
|
||||
Purpose: The backend currently swallows artist credit errors entirely and mixes non-fatal scan issues with catastrophic failures in a single error return. After this plan, callers can distinguish "scan completed with issues" from "scan failed."
|
||||
Output: New `backend/database/errors.go`, updated migration in `database.go`, enhanced `ScanMetrics` with warnings, reclassified error paths throughout `Scan()`.
|
||||
</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/phases/02-backend-correctness/02-CONTEXT.md
|
||||
@.planning/phases/02-backend-correctness/02-RESEARCH.md
|
||||
|
||||
@backend/database/database.go
|
||||
@backend/library/metrics.go
|
||||
@backend/library/library.go
|
||||
@backend/library/rescan.go
|
||||
|
||||
<interfaces>
|
||||
<!-- Key types and contracts the executor needs. Extracted from codebase. -->
|
||||
|
||||
From backend/database/database.go:
|
||||
```go
|
||||
type DB struct {
|
||||
db *sql.DB
|
||||
Ctx context.Context
|
||||
Queries *sqlcgen.Queries
|
||||
logger *slog.Logger
|
||||
}
|
||||
|
||||
// Migration pattern — runMigrations at line 156:
|
||||
// Checks PRAGMA user_version, runs migrations conditionally.
|
||||
// Latest migration is 2 (migration2BasenameAndFTS).
|
||||
// Migration 3 should follow the same pattern at end of runMigrations().
|
||||
func runMigrations(ctx context.Context, db *sql.DB, logger *slog.Logger) error
|
||||
```
|
||||
|
||||
From backend/library/metrics.go:
|
||||
```go
|
||||
type ScanMetrics struct {
|
||||
mu sync.Mutex
|
||||
// ... timing/count fields ...
|
||||
Added int64 `json:"added"`
|
||||
Updated int64 `json:"updated"`
|
||||
Skipped int64 `json:"skipped"`
|
||||
Removed int64 `json:"removed"`
|
||||
}
|
||||
|
||||
// Existing mutex-protected method pattern:
|
||||
func (m *ScanMetrics) addExtraction(fileType string, tagTime, durationTime time.Duration)
|
||||
```
|
||||
|
||||
From backend/library/library.go:
|
||||
```go
|
||||
func (l *Library) Scan() (*ScanMetrics, error) // line 175
|
||||
func (l *Library) commitBatch(batch []importResult, ...) error // line 652
|
||||
func (l *Library) saveAudioFile(q *sqlcgen.Queries, tx *sql.Tx, ...) error // line 713
|
||||
func (l *Library) updateAudioFileMetadata(q *sqlcgen.Queries, tx *sql.Tx, ...) error // line 809
|
||||
func (l *Library) cachedLinkArtist(q *sqlcgen.Queries, cache *entityCache, name string, creditID int64) // line 1074
|
||||
|
||||
// Current error accumulation pattern in Scan():
|
||||
var scanErr error
|
||||
var errMu sync.Mutex
|
||||
// Various error paths use: scanErr = errors.Join(scanErr, err)
|
||||
```
|
||||
|
||||
From backend/library/library.go — cachedLinkArtist (line 1074-1108):
|
||||
```go
|
||||
func (l *Library) cachedLinkArtist(
|
||||
q *sqlcgen.Queries,
|
||||
cache *entityCache,
|
||||
name string,
|
||||
creditID int64,
|
||||
) {
|
||||
// ... artist upsert ...
|
||||
_, _ = q.CreateArtistCreditArtist(l.ctx, ...) // <-- discards BOTH returns
|
||||
cache.linkedCredits[linkKey] = struct{}{}
|
||||
}
|
||||
```
|
||||
|
||||
From backend/library/rescan.go — handleConfigUpdate calls Scan:
|
||||
```go
|
||||
func (l *Library) handleConfigUpdate(updatedConfigValues Config) error {
|
||||
if _, err := l.Scan(); err != nil { // <-- only checks error return
|
||||
updateErr = errors.Join(updateErr, ...)
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
SQLite driver types (from modernc.org/sqlite):
|
||||
```go
|
||||
// modernc.org/sqlite — Error type
|
||||
type Error struct { ... }
|
||||
func (e *Error) Code() int // returns extended result code
|
||||
|
||||
// modernc.org/sqlite/lib — Constants
|
||||
const SQLITE_CONSTRAINT_UNIQUE = 2067
|
||||
```
|
||||
</interfaces>
|
||||
</context>
|
||||
|
||||
<tasks>
|
||||
|
||||
<task type="auto">
|
||||
<name>Task 1: Create IsUniqueViolation helper and add migration 3</name>
|
||||
<files>backend/database/errors.go, backend/database/database.go</files>
|
||||
<action>
|
||||
**CORR-08 Part 1 — IsUniqueViolation helper (new file: backend/database/errors.go):**
|
||||
|
||||
Create `backend/database/errors.go` with:
|
||||
```go
|
||||
package database
|
||||
|
||||
import (
|
||||
"errors"
|
||||
|
||||
"modernc.org/sqlite"
|
||||
sqlite3 "modernc.org/sqlite/lib"
|
||||
)
|
||||
|
||||
// IsUniqueViolation reports whether err is a SQLite UNIQUE
|
||||
// constraint violation (extended result code 2067).
|
||||
func IsUniqueViolation(err error) bool {
|
||||
var sqliteErr *sqlite.Error
|
||||
if errors.As(err, &sqliteErr) {
|
||||
return sqliteErr.Code() == sqlite3.SQLITE_CONSTRAINT_UNIQUE
|
||||
}
|
||||
return false
|
||||
}
|
||||
```
|
||||
|
||||
**CORR-08 Part 2 — Migration 3 (backend/database/database.go):**
|
||||
|
||||
Add migration 3 at the end of `runMigrations()`, after the `if version < 2` block (after line 221) and before the final `return nil`:
|
||||
|
||||
```go
|
||||
// Migration 3: add UNIQUE constraint to artist_credit_artist.
|
||||
if version < 3 {
|
||||
logger.Info(
|
||||
"applying migration 3: artist_credit_artist unique constraint",
|
||||
)
|
||||
|
||||
// Remove duplicates first (keep lowest ID per pair).
|
||||
if _, err := db.ExecContext(ctx, `
|
||||
DELETE FROM artist_credit_artist
|
||||
WHERE id NOT IN (
|
||||
SELECT MIN(id)
|
||||
FROM artist_credit_artist
|
||||
GROUP BY artist_id, credit_id
|
||||
)
|
||||
`); err != nil {
|
||||
return fmt.Errorf(
|
||||
"migration 3: could not deduplicate: %w", err,
|
||||
)
|
||||
}
|
||||
|
||||
if _, err := db.ExecContext(ctx, `
|
||||
CREATE UNIQUE INDEX IF NOT EXISTS
|
||||
idx_artist_credit_artist_unique
|
||||
ON artist_credit_artist(artist_id, credit_id)
|
||||
`); err != nil {
|
||||
return fmt.Errorf(
|
||||
"migration 3: could not create unique index: %w",
|
||||
err,
|
||||
)
|
||||
}
|
||||
|
||||
if _, err := db.ExecContext(
|
||||
ctx, "PRAGMA user_version = 3",
|
||||
); err != nil {
|
||||
return fmt.Errorf(
|
||||
"could not set user_version to 3: %w", err,
|
||||
)
|
||||
}
|
||||
|
||||
logger.Info("migration 3 complete")
|
||||
}
|
||||
```
|
||||
|
||||
Ensure `fmt` is imported in database.go (it already is — verify).
|
||||
</action>
|
||||
<verify>
|
||||
<automated>cd /mnt/vault/dev/golang/yellowjacket && go vet ./backend/database/... && go build ./backend/database/... && grep -q "IsUniqueViolation" backend/database/errors.go && grep -q "version < 3" backend/database/database.go && grep -q "idx_artist_credit_artist_unique" backend/database/database.go</automated>
|
||||
</verify>
|
||||
<done>IsUniqueViolation exported function exists in backend/database/errors.go; migration 3 deduplicates existing rows and creates UNIQUE index on artist_credit_artist(artist_id, credit_id); database package compiles cleanly</done>
|
||||
</task>
|
||||
|
||||
<task type="auto">
|
||||
<name>Task 2: Add ScanWarning type and reclassify scan errors as warnings</name>
|
||||
<files>backend/library/metrics.go, backend/library/library.go</files>
|
||||
<action>
|
||||
**CORR-09 Part 1 — ScanWarning type (backend/library/metrics.go):**
|
||||
|
||||
1. Add `ScanWarning` struct and `Warnings` field to `ScanMetrics`:
|
||||
```go
|
||||
// ScanWarning represents a non-fatal issue encountered during scanning.
|
||||
type ScanWarning struct {
|
||||
FilePath string `json:"filePath"`
|
||||
Phase string `json:"phase"`
|
||||
Err error `json:"err"`
|
||||
}
|
||||
```
|
||||
|
||||
2. Add `Warnings []ScanWarning` field to `ScanMetrics` struct (after the file count fields, before the closing brace). Add JSON tag: `json:"warnings"`.
|
||||
|
||||
3. Add `addWarning` method:
|
||||
```go
|
||||
// addWarning records a non-fatal scan issue. Safe for concurrent use.
|
||||
func (m *ScanMetrics) addWarning(filePath, phase string, err error) {
|
||||
m.mu.Lock()
|
||||
defer m.mu.Unlock()
|
||||
m.Warnings = append(m.Warnings, ScanWarning{
|
||||
FilePath: filePath,
|
||||
Phase: phase,
|
||||
Err: err,
|
||||
})
|
||||
}
|
||||
```
|
||||
|
||||
**CORR-09 Part 2 — Reclassify error paths in Scan() (backend/library/library.go):**
|
||||
|
||||
The key rule: **transaction begin/commit failures and context cancellation are ALWAYS fatal. Individual file operations (save, FTS index, orphan delete, walk errors, variant generation) are ALWAYS warnings.**
|
||||
|
||||
Changes to `Scan()`:
|
||||
|
||||
1. **WalkDir errors (lines 319-328):** Replace `scanErr = errors.Join(scanErr, ...)` with `metrics.addWarning("", "walk", walkErr)`. Walk errors are non-fatal — the scan already processed files discovered before the error.
|
||||
|
||||
2. **Metadata extraction failures (lines 436-438):** Replace the `errMu.Lock(); scanErr = errors.Join(scanErr, err); errMu.Unlock()` block with `metrics.addWarning(work.absolutePath, "extraction", err)`. The `errMu` lock is no longer needed for this path (addWarning has its own mutex).
|
||||
|
||||
3. **commitBatch errors (lines 388-390):** This requires splitting. The `commitBatch` function currently returns both transaction failures and individual file save failures as a single error.
|
||||
- Modify `commitBatch` to accept `metrics *ScanMetrics` (it already does — line 655) and call `metrics.addWarning` for individual file save failures instead of accumulating into `batchErr`.
|
||||
- The `batchErr` variable in `commitBatch` is eliminated. Individual `saveErr` values go to `metrics.addWarning(result.absolutePath, "commit", saveErr)`.
|
||||
- Only the `tx.Commit()` failure (line 702-706) remains as a returned error — this is a fatal transaction failure.
|
||||
- In `Scan()`, the caller at lines 383-391 still checks `batchErr` — since `commitBatch` now only returns fatal commit errors, rename the check to reflect this: if commitBatch returns an error, it's fatal. **Return immediately** from the DB writer goroutine with the fatal error set via `errMu`.
|
||||
|
||||
4. **Orphan delete failures (lines 484-495):** Already logged but silently continued. Add `metrics.addWarning(path, "orphan", err)` alongside the existing log. The `return true` (continue iteration) stays.
|
||||
|
||||
5. **Orphan FTS delete failures (lines 498-505):** Already logged but silently continued. Add `metrics.addWarning(path, "orphan", err)` alongside the existing log.
|
||||
|
||||
6. **Variant generation failure (lines 518-522):** Already logged. Add `metrics.addWarning("", "variant", err)` alongside the existing log.
|
||||
|
||||
7. **FTS indexing failures in saveAudioFile (lines 787-798) and updateAudioFileMetadata (lines 866-893):** These are currently logged but don't return errors. Convert to warnings: add `metrics.addWarning(result.absolutePath, "commit", err)` alongside the existing log. Since `saveAudioFile` and `updateAudioFileMetadata` already receive `metrics`, this is straightforward.
|
||||
|
||||
8. **Remove `errMu` and `scanErr` accumulation pattern.** After reclassification:
|
||||
- `scanErr` should only contain fatal errors (context cancellation, transaction commit failures)
|
||||
- `errMu` may still be needed if the DB writer goroutine sets a fatal error that Scan() reads. Keep `errMu` but only use it for fatal error paths.
|
||||
- The extraction worker pool no longer writes to `scanErr` — all extraction failures are warnings.
|
||||
|
||||
**CORR-08 Part 3 — Update cachedLinkArtist (backend/library/library.go):**
|
||||
|
||||
Per CONTEXT.md decision: pass `metrics *ScanMetrics` as an additional parameter. Per research recommendation: call `metrics.addWarning()` directly for non-UNIQUE errors.
|
||||
|
||||
1. Change `cachedLinkArtist` signature to:
|
||||
```go
|
||||
func (l *Library) cachedLinkArtist(
|
||||
q *sqlcgen.Queries,
|
||||
cache *entityCache,
|
||||
metrics *ScanMetrics,
|
||||
name string,
|
||||
creditID int64,
|
||||
)
|
||||
```
|
||||
|
||||
2. Replace the `_, _ = q.CreateArtistCreditArtist(...)` at line 1101 with:
|
||||
```go
|
||||
_, err = q.CreateArtistCreditArtist(
|
||||
l.ctx,
|
||||
sqlcgen.CreateArtistCreditArtistParams{
|
||||
ArtistID: artist.ID,
|
||||
CreditID: creditID,
|
||||
},
|
||||
)
|
||||
if err != nil {
|
||||
if !database.IsUniqueViolation(err) {
|
||||
l.logger.Warn(
|
||||
"could not link artist to credit",
|
||||
"artist", name,
|
||||
"creditID", creditID,
|
||||
"err", err,
|
||||
)
|
||||
metrics.addWarning(
|
||||
name, "commit",
|
||||
fmt.Errorf(
|
||||
"artist-credit link failed for %q: %w",
|
||||
name, err,
|
||||
),
|
||||
)
|
||||
}
|
||||
// UNIQUE violation: link already exists in DB, not an error.
|
||||
}
|
||||
```
|
||||
|
||||
3. Add `"yellowjacket/backend/database"` to the imports in `library.go` if not already present.
|
||||
|
||||
4. Update ALL callers of `cachedLinkArtist` (in `processMetadata`) to pass `metrics` as the new parameter. Search for `l.cachedLinkArtist(` and add the metrics argument.
|
||||
|
||||
**CORR-09 Part 3 — Update handleConfigUpdate caller (backend/library/library.go):**
|
||||
|
||||
In `handleConfigUpdate` (line 1325), after calling `l.Scan()`, log any warnings from the returned metrics:
|
||||
|
||||
```go
|
||||
if metrics, err := l.Scan(); err != nil {
|
||||
updateErr = errors.Join(updateErr, fmt.Errorf(
|
||||
"problem scanning library on config update: %w", err,
|
||||
))
|
||||
} else if len(metrics.Warnings) > 0 {
|
||||
l.logger.Warn(
|
||||
"library scan completed with warnings",
|
||||
"warningCount", len(metrics.Warnings),
|
||||
)
|
||||
}
|
||||
```
|
||||
|
||||
Note: change the `_` discard of metrics to capture it.
|
||||
</action>
|
||||
<verify>
|
||||
<automated>cd /mnt/vault/dev/golang/yellowjacket && go vet ./backend/... && go build ./backend/... && grep -q "ScanWarning" backend/library/metrics.go && grep -q "addWarning" backend/library/metrics.go && grep -q "IsUniqueViolation" backend/library/library.go && grep -q "metrics.addWarning" backend/library/library.go && grep -c "metrics.addWarning" backend/library/library.go | grep -qE '^[5-9]|^[1-9][0-9]'</automated>
|
||||
</verify>
|
||||
<done>ScanWarning struct exists with FilePath/Phase/Err fields; addWarning is mutex-protected; Scan() returns only fatal errors in error return; all non-fatal errors (extraction, FTS, orphan, walk, variant, individual file save) go to ScanMetrics.Warnings; cachedLinkArtist checks errors with IsUniqueViolation and records non-UNIQUE failures as warnings; handleConfigUpdate logs warning count</done>
|
||||
</task>
|
||||
|
||||
</tasks>
|
||||
|
||||
<verification>
|
||||
```bash
|
||||
# All backend packages compile
|
||||
go build ./backend/...
|
||||
|
||||
# All backend packages pass vet
|
||||
go vet ./backend/...
|
||||
|
||||
# Linting passes
|
||||
golangci-lint run ./backend/...
|
||||
|
||||
# IsUniqueViolation helper exists
|
||||
grep -q "func IsUniqueViolation" backend/database/errors.go
|
||||
|
||||
# Migration 3 exists
|
||||
grep -q "version < 3" backend/database/database.go
|
||||
grep -q "idx_artist_credit_artist_unique" backend/database/database.go
|
||||
|
||||
# ScanWarning type and addWarning method exist
|
||||
grep -q "type ScanWarning struct" backend/library/metrics.go
|
||||
grep -q "func (m \*ScanMetrics) addWarning" backend/library/metrics.go
|
||||
|
||||
# cachedLinkArtist uses IsUniqueViolation
|
||||
grep -q "database.IsUniqueViolation" backend/library/library.go
|
||||
|
||||
# No discarded CreateArtistCreditArtist returns
|
||||
! grep -q '_, _ = q.CreateArtistCreditArtist' backend/library/library.go
|
||||
|
||||
# Warnings are collected (multiple addWarning calls)
|
||||
test "$(grep -c 'metrics.addWarning' backend/library/library.go)" -ge 5
|
||||
|
||||
# scanErr only used for fatal errors (should be minimal occurrences)
|
||||
# handleConfigUpdate captures metrics
|
||||
grep -q 'metrics.Warnings' backend/library/library.go
|
||||
|
||||
# Race detector passes
|
||||
go test -race -count=1 ./backend/database/... ./backend/library/...
|
||||
```
|
||||
</verification>
|
||||
|
||||
<success_criteria>
|
||||
- `go build ./backend/...` compiles cleanly
|
||||
- `go vet ./backend/...` passes
|
||||
- `golangci-lint run ./backend/...` passes
|
||||
- `go test -race ./backend/database/... ./backend/library/...` passes
|
||||
- `IsUniqueViolation` helper correctly detects UNIQUE constraint violations
|
||||
- Migration 3 deduplicates and adds UNIQUE index
|
||||
- `ScanWarning` struct exists with `FilePath`, `Phase`, `Err` fields
|
||||
- `addWarning` is mutex-protected for concurrent use
|
||||
- `Scan()` error return only contains fatal errors
|
||||
- All non-fatal scan errors are accumulated in `ScanMetrics.Warnings`
|
||||
- `cachedLinkArtist` checks errors and only ignores UNIQUE violations
|
||||
- `handleConfigUpdate` logs warning count after scan
|
||||
</success_criteria>
|
||||
|
||||
<output>
|
||||
After completion, create `.planning/phases/02-backend-correctness/02-02-SUMMARY.md`
|
||||
</output>
|
||||
Reference in New Issue
Block a user