434 lines
17 KiB
Markdown
434 lines
17 KiB
Markdown
---
|
|
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>
|