docs: spec for cleanup batch (Tier A loose ends + Tier B design fixes)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Jonathan
@@ -0,0 +1,213 @@
|
||||
# Cleanup batch — Tier A (loose ends) + Tier B (design tradeoffs)
|
||||
|
||||
**Date:** 2026-07-13
|
||||
**Status:** design approved, pending spec review
|
||||
**Branch:** `cleanup/tier-a-b`
|
||||
|
||||
## Goal
|
||||
|
||||
Close the deferred backlog down to zero *fixes* so the codebase has no known loose
|
||||
ends before we brainstorm net-new features (Tier C). This batch covers the clear
|
||||
hardening/polish items (Tier A) and the two benign-but-real design tradeoffs the
|
||||
user chose to fix properly (Tier B). Tier C (Last.fm source, crash-resume
|
||||
mid-pipeline, per-track duration matching, UI error states) is explicitly **out of
|
||||
scope** here — it goes to the feature brainstorm next.
|
||||
|
||||
Non-goals: no behavior change to the acquisition pipeline, no new user-facing
|
||||
features, no schema change beyond the two additive tables below.
|
||||
|
||||
---
|
||||
|
||||
## Tier A — loose ends (web, mechanical)
|
||||
|
||||
### A1. PATCH allowlist hardening
|
||||
Both `web/src/app/api/monitor/config/route.ts` and
|
||||
`web/src/app/api/discover/config/route.ts` PATCH handlers currently gate writes on
|
||||
`k in DEFAULTS`, which walks the prototype chain. The DELETE `/api/config` handler
|
||||
was already hardened to `Object.prototype.hasOwnProperty.call(DEFAULTS, k)`; apply
|
||||
the same own-property check to both PATCH routes. This only ever prevented writing
|
||||
harmless junk `Config` rows (neither PATCH deletes), so this is consistency
|
||||
hardening, not a live vulnerability — but it removes the last prototype-chain gap.
|
||||
|
||||
### A2. a11y + copy
|
||||
- Add an `aria-label` to the Monitor tab `enabled` toggle (it currently has none;
|
||||
every other control on the page is labelled and web tests key off labels).
|
||||
- Update the Settings page eyebrow at `web/src/app/settings/settings-form.tsx:159`
|
||||
from `"Credentials · library · discovery"` to cover all five tabs, e.g.
|
||||
`"Credentials · library · monitor · discovery"`.
|
||||
|
||||
### A3. DELETE allowlist test
|
||||
Add a `?field=constructor` case to the DELETE `/api/config` allowlist test
|
||||
(`web/src/app/api/config/route.test.ts`) alongside the existing `__proto__` case, so
|
||||
the own-property guard is covered against both prototype-chain keys. Structural fix
|
||||
already in place; this is belt-and-suspenders coverage.
|
||||
|
||||
---
|
||||
|
||||
## Tier B1 — discovery scoring: per-seed contributions table (worker + migration)
|
||||
|
||||
### Problem
|
||||
`_upsert_artist` / `_upsert_album` upsert with `score = EXCLUDED.score` (**replace**),
|
||||
but the intended semantic is *score = sum of every seed's contribution*. Since
|
||||
`discover.maxSeeds` was retired and chunking is now the default, a single sweep
|
||||
partitions its seeds across many worker iterations. A candidate surfaced by seeds in
|
||||
different chunks has its score **overwritten** by the last chunk instead of summed
|
||||
(item 4), and the same replace-not-accumulate breaks across throttled sweeps
|
||||
(item 5). The score a suggestion shows reflects only its most-recent chunk, not the
|
||||
whole library's affinity.
|
||||
|
||||
### Design
|
||||
Introduce a normalized contributions table so a suggestion's score is a derived
|
||||
aggregate, independent of chunk and sweep boundaries.
|
||||
|
||||
```prisma
|
||||
model DiscoverySeedContribution {
|
||||
id String @id @default(cuid())
|
||||
candidateMbid String
|
||||
candidateName String
|
||||
seedMbid String
|
||||
score Float
|
||||
sources String[]
|
||||
updatedAt DateTime @updatedAt
|
||||
|
||||
@@unique([candidateMbid, seedMbid])
|
||||
@@index([candidateMbid])
|
||||
@@index([seedMbid])
|
||||
}
|
||||
```
|
||||
|
||||
`run_discovery`, per seed **S** processed in the chunk:
|
||||
|
||||
1. `affected := SELECT DISTINCT "candidateMbid" WHERE "seedMbid" = S` — the
|
||||
candidates S *used* to contribute to (so drop-offs are recomputed too).
|
||||
2. `DELETE FROM "DiscoverySeedContribution" WHERE "seedMbid" = S`.
|
||||
3. Build a per-seed aggregate over the live sources: for each similar candidate
|
||||
(skipping already-followed mbids), sum score across sources and union source
|
||||
names. `INSERT` one contribution row per (candidate, S).
|
||||
4. `affected |= {candidate mbids newly inserted for S}`.
|
||||
|
||||
After the chunk's seeds are processed, recompute every candidate in the union of all
|
||||
`affected` sets:
|
||||
|
||||
- `SELECT SUM(score), COUNT(*), <sources>, any candidateName
|
||||
FROM "DiscoverySeedContribution" WHERE "candidateMbid" = ANY(affected)
|
||||
GROUP BY "candidateMbid"`.
|
||||
`COUNT(*)` is the seedCount (unique on `(candidate, seed)` → one row per seed).
|
||||
- For each with `score >= min_score`, upsert `DiscoverySuggestion` exactly as today,
|
||||
keeping the `WHERE status = 'pending'` guard (dismissed/wanted rows untouched) and
|
||||
the rowcount-drives-album-derivation logic. Album suggestions inherit the
|
||||
recomputed artist aggregate.
|
||||
|
||||
### Properties
|
||||
- Score is now **exactly** the sum of all seeds' latest contributions — no chunk
|
||||
artifact, no partial-sweep reset. A sweep triggered by one newly-followed artist
|
||||
updates only that seed's rows and leaves every other seed's contribution intact.
|
||||
- Deleting-then-reinserting a seed's rows self-heals stale entries: if S stops
|
||||
finding a candidate, S's row for it vanishes and the candidate is recomputed
|
||||
(seedCount drops) because it was in `affected`.
|
||||
- A candidate whose contributions fall to zero: its pending suggestion is **left as
|
||||
is** (not auto-retracted), consistent with today's semantics — suggestions persist
|
||||
until dismissed/actioned. Documented, not a bug.
|
||||
|
||||
### Migration & cleanup
|
||||
- Additive table only; no data backfill (existing suggestions keep their current
|
||||
score and are corrected on the next sweep that touches their seeds).
|
||||
- Consider a one-time note: existing `DiscoverySuggestion` scores stay stale until
|
||||
re-swept — acceptable for an off-by-default feed.
|
||||
|
||||
---
|
||||
|
||||
## Tier B2 — scan worklist table (worker + migration)
|
||||
|
||||
### Problem
|
||||
`scan_chunk` re-walks the whole `/music` tree every chunk and skips entries sorting
|
||||
`<= cursor` — `O(N²/C)` directory stat ops over a full scan. The scan is
|
||||
MB-rate-limited (~1 req/s), so this is ~2% overhead, but the only true `O(N)` fix is
|
||||
a persisted worklist (a JSON-blob-in-Config would just move the cost to re-parsing
|
||||
the blob each chunk).
|
||||
|
||||
### Design
|
||||
Persist the album worklist once at scan start and pop it by indexed cursor.
|
||||
|
||||
```prisma
|
||||
model ScanWorkItem {
|
||||
id String @id @default(cuid())
|
||||
scanId String
|
||||
artist String
|
||||
album String
|
||||
path String
|
||||
done Boolean @default(false)
|
||||
|
||||
@@unique([scanId, path])
|
||||
@@index([scanId, done, artist, album])
|
||||
}
|
||||
```
|
||||
|
||||
`scan.py`:
|
||||
- `scan_build_worklist(conn, scan_id, dest_root)` — walk once via the existing
|
||||
`_iter_album_entries`, `INSERT` a row per album folder (`ON CONFLICT (scanId, path)
|
||||
DO NOTHING`, so a rebuild is idempotent). Store `artist`, `album` (= album folder),
|
||||
`path` so the chunk needs no re-derivation.
|
||||
- `scan_chunk(conn, resolver, probe, browser, scan_id, limit)` — `SELECT` the next
|
||||
`limit` rows `WHERE scanId = %s AND NOT done ORDER BY artist, album`, process each
|
||||
via the existing `_process_album`, mark `done = true`. Returns
|
||||
`(imported, skipped, done)` where `done` is True when no undone rows remain.
|
||||
- `scan_library(conn, resolver, probe, browser, dest_root)` (test-only unbounded
|
||||
path) — reimplemented on the worklist: build with a fixed scan id, drain fully,
|
||||
delete that scan's rows, return `ScanResult`. Keeps existing tests meaningful.
|
||||
|
||||
`main.maybe_run_scan`:
|
||||
- On start (`scan.requested` and not in progress): set `scan.inProgress=true`,
|
||||
`scan.requested=false`, `scan.progress="0/0"`, generate a `scan_id`
|
||||
(uuid), store it in `scan.id`, and `scan_build_worklist`.
|
||||
- Each iteration: read `scan.id`, pop one chunk, accumulate imported/skipped in
|
||||
`scan.progress`.
|
||||
- On drain: write `scan.result` (unchanged `"imported X, skipped Y"` format), clear
|
||||
`scan.progress`/`scan.id`, set `scan.inProgress=false`, and
|
||||
`DELETE FROM "ScanWorkItem" WHERE scanId = %s`. Preserve the existing
|
||||
"rescan requested mid-scan triggers one more full pass" guarantee.
|
||||
- `scan.cursor` is retired (worklist replaces the string cursor).
|
||||
|
||||
### Properties
|
||||
- `O(log N + C)` per chunk via the `(scanId, done, artist, album)` index; one tree
|
||||
walk per scan total.
|
||||
- Robust to folders appearing/disappearing mid-scan: the worklist is frozen at scan
|
||||
start, so the set processed is deterministic.
|
||||
- Resumable/idempotent: `done` flag + `ON CONFLICT` inserts + `ON CONFLICT` record
|
||||
writes mean an interrupted worker resumes cleanly.
|
||||
|
||||
---
|
||||
|
||||
## Migration
|
||||
|
||||
One additive Prisma migration adds both tables (`DiscoverySeedContribution`,
|
||||
`ScanWorkItem`). No column changes to existing tables, no backfill. Reaches live
|
||||
`lyra` via the web entrypoint's `migrate deploy` on rebuild.
|
||||
|
||||
## Testing
|
||||
|
||||
- **Worker (`pytest`, against `lyra_test`):**
|
||||
- Contribution accumulation: a candidate surfaced by seeds split across two
|
||||
simulated chunks sums to the same score as a single-chunk sweep; a partial sweep
|
||||
(one seed) preserves other seeds' contributions; a seed dropping a candidate
|
||||
lowers its seedCount and recomputes the suggestion.
|
||||
- Scan worklist: build → pop chunks → drain marks all done; resume after a partial
|
||||
chunk; idempotent rebuild; `scan_library` unbounded path still returns the same
|
||||
`ScanResult`.
|
||||
- **Web (`vitest`):**
|
||||
- PATCH `/api/monitor/config` and `/api/discover/config` reject a
|
||||
prototype-chain key (`__proto__`, `constructor`) — no `Config` write.
|
||||
- DELETE `/api/config` `?field=constructor` rejected (added to existing test).
|
||||
- Full suites + `tsc` + `next build` green before merge.
|
||||
|
||||
## Delivery
|
||||
|
||||
Branch `cleanup/tier-a-b` → writing-plans → subagent-driven build (per-task review +
|
||||
Opus whole-branch review) → ff-merge to `main` → `git push` →
|
||||
`docker compose up -d --build web worker` (runs `migrate deploy`) → verify live
|
||||
(web 200, worker healthy, migration applied). Then move to the Tier C feature
|
||||
brainstorm.
|
||||
|
||||
## Out of scope (→ Tier C brainstorm)
|
||||
Last.fm second `SimilaritySource`, crash-resume mid-pipeline, per-track duration
|
||||
matching, UI error states on mutations.
|
||||
Reference in New Issue
Block a user