diff --git a/docs/superpowers/specs/2026-07-13-deferred-batch-design.md b/docs/superpowers/specs/2026-07-13-deferred-batch-design.md new file mode 100644 index 0000000..4a05f23 --- /dev/null +++ b/docs/superpowers/specs/2026-07-13-deferred-batch-design.md @@ -0,0 +1,215 @@ +# Deferred-items batch — design + +Date: 2026-07-13 + +Clearing the deferred backlog tracked in the project-state memory. Three +independent workstreams, landing as separate commits on one feature branch: + +- **A. Settings** — Monitor tab, clear-a-credential, surface `discover.listenBrainzUrl`. +- **B. Correctness** — Lucene escaping, wanted-POST TOCTOU, `import_album` crash cleanup. +- **C. Chunk discovery** — make the discovery sweep non-blocking like the scan. + +These do not depend on each other and can be reviewed independently. Monitor and +discovery stay OFF by default; this batch only makes the monitor *toggleable from +the UI* (today it needs a hand-edited Config row). + +Out of scope (stay deferred): Last.fm second `SimilaritySource`, per-track duration +matching, crash-resume mid-pipeline, `_upsert_artist` score accumulation, +have-via-scan name-match hardening, `scan_chunk` O(N²) directory I/O. + +--- + +## Workstream A — Settings + +### A1. Monitor tab + +`web/src/app/settings/settings-form.tsx` gains a `Monitor` tab, structured exactly +like the existing Discovery tab (an enable toggle + a numeric `field-grid`, plus one +extra checkbox). New route `web/src/app/api/monitor/config/route.ts`, a clone of +`api/discover/config/route.ts` (GET returns stored-or-default; PATCH upserts keys in +the `DEFAULTS` allowlist). `DEFAULTS`: + +| key | default | control | +|---|---|---| +| `monitor.enabled` | `false` | toggle | +| `monitor.autoMonitorFuture` | `false` | checkbox | +| `monitor.pollIntervalHours` | `24` | number | +| `monitor.retryIntervalHours` | `6` | number | +| `monitor.qualityCutoff` | `2` | number | +| `monitor.upgradeWindowDays` | `14` | number | + +Defaults match the worker's fallbacks (`worker/lyra_worker/monitor.py:29-33`). No +worker change — it already reads these keys each loop. `monitor.autoMonitorFuture` +is already consumed by `monitor.discover` (Tier-1+2 batch); this just surfaces it. + +`Tab` type + `TABS` array extended with `"Monitor"`. State/handlers mirror the +`discoverCfg` / `saveDiscovery` pattern (`monitorCfg`, `saveMonitor`, a +`fetch("/api/monitor/config")` load effect). + +### A2. Clear-a-credential + +New `DELETE` handler in `web/src/app/api/config/route.ts`: + +``` +DELETE /api/config?field= +``` + +`fieldName` is validated against the existing `FIELDS` map (the field→key allowlist, +`config/route.ts:5`). Unknown field → 400. Valid → delete that one Config row by its +mapped `key` (`prisma.config.deleteMany({ where: { key } })`, idempotent — deleting an +already-absent key is a no-op 200). Using the field-name allowlist (not a raw `?key=`) +prevents deleting arbitrary Config rows (e.g. `monitor.enabled`). + +UI: in the Qobuz and Soulseek tabs, every secret field currently rendering the +`· set` hint gets a small `[Clear]` button beside it (only shown when the field is +`set`). On click → `DELETE`, then flip the corresponding `*Set` state flag to `false` +(so the hint and button disappear) and clear any typed value. A toast confirms +("Cleared"). Applies to: `qobuzPassword`, `qobuzToken`, `slskdApiKey`. (Non-secret +fields — email, user id, url — keep the existing PUT-with-value behaviour; clearing +those is not in scope.) + +### A3. Surface `discover.listenBrainzUrl` + +Add `discover.listenBrainzUrl` (default `""`) to the `DEFAULTS` in +`api/discover/config/route.ts` and one text field in the Discovery tab's grid, +labelled "ListenBrainz base URL (blank = default)". Empty string = worker uses its +built-in default. No worker change — the key is already read. + +--- + +## Workstream B — Correctness + +### B1. Lucene phrase escaping (`web/src/lib/musicbrainz.ts:58`) + +`searchReleaseGroup` builds `releasegroup:"${album}" AND artist:"${artist}"`. A `"` +(or `\`) in `album`/`artist` terminates the quoted phrase and corrupts the query. +Add a module-local helper: + +```ts +function escapeLucenePhrase(s: string): string { + return s.replace(/[\\"]/g, (c) => "\\" + c); +} +``` + +and use it on both interpolated terms. Escaping `\` and `"` is sufficient inside a +quoted phrase (other Lucene metacharacters are literal within quotes). Unit test in +`musicbrainz.test.ts`: an album title containing `"` produces a query with the +escaped sequence (assert the constructed query string / the `encodeURIComponent` +argument, mocking `mbGet`). + +### B2. wanted-POST TOCTOU (`web/src/app/api/wanted/route.ts:47`) + +Current flow: `findUnique(rgMbid)` → if absent `create`. Two concurrent Wants for the +same release-group both see "absent"; the loser's `create` violates the `rgMbid` +unique constraint and 500s. Fix: keep the fast-path check + update branch, but wrap +the `create` in `try/catch`; on Prisma `P2002` (unique violation) fall back to the +same update-and-return-200 path as the "existing" branch. Preserves the 200/201 +contract and closes the race without needing serializable isolation (which +`$transaction` alone would not provide). Extend `route.test.ts` to cover the P2002 +fallback (mock `create` to throw a `PrismaClientKnownRequestError` with `code:"P2002"`, +assert it resolves to the update path / 200). + +### B3. `import_album` crash cleanup (`worker/lyra_worker/library.py:62`) + +A crash between `os.rename(final, old)` (line 83) and `os.rename(tmp, final)` (84) +leaves `final` missing with an orphan `{final}.old` holding the previous good copy +(and possibly a stale `{final}.importing`). Nothing recovers these today. Add a +recovery preamble at the **top** of `import_album`, before assembling `tmp`: + +``` +old = final + ".old" +if os.path.isdir(old) and not os.path.isdir(final): + os.rename(old, final) # restore the previous copy from an interrupted swap +shutil.rmtree(old, ignore_errors=True) # drop a leftover .old (swap had completed) +shutil.rmtree(final + ".importing", ignore_errors=True) # drop a stale half-assembly +``` + +This makes the swap crash-safe and idempotent: after any interruption, the next +`import_album` for that `final` restores the last-good copy (if the swap was mid-flight) +or discards a stale `.old` (if the swap had finished), and never inherits a partial +`.importing`. Unit test in the worker suite: pre-create `{final}.old` + a missing +`final`, run `import_album`, assert `final` restored; and pre-create `{final}.old` + +an existing `final`, assert `.old` removed and `final` untouched by the preamble. + +--- + +## Workstream C — Chunk discovery + +Goal: the discovery sweep no longer blocks job-claim while it runs; it advances one +small chunk per worker loop iteration, mirroring the scan-chunking design +(`main.maybe_run_scan`). Discovery's cursor is **already temporal** — +`WatchedArtist.lastDiscoveredAt`, selected `ORDER BY ... NULLS FIRST LIMIT` +(`discovery.py:66-71`) — so, unlike the scan, **no separate cursor Config row is +needed**: the seed rows themselves are the cursor. + +**Decision (settled): retire `discover.maxSeeds`.** Once a sweep spans multiple +iterations off the `lastDiscoveredAt` cursor, a per-sweep cap is redundant — a sweep +naturally covers every *eligible* seed (each becomes ineligible for `intervalHours` +once processed), `chunkSize` at a time. `discover.maxSeeds` is removed from the +Settings Discovery grid and no longer consumed; the Config key is left ignored for +back-compat (harmless if present). + +Changes: + +1. **`discovery.py`** + - `DiscoveryConfig`: drop `max_seeds`; add `chunk_size` (`_int("discover.chunkSize", 5)`). + - Seed selection `LIMIT` uses `chunk_size` instead of `max_seeds`. + - Update each seed's `lastDiscoveredAt = now()` **incrementally, right after that + seed is processed** (move it out of the trailing batch loop at `discovery.py:189-190`), + so a mid-chunk failure still persists progress for completed seeds. + - `run_discovery` returns the number of seeds processed this call (0 ⇒ drained). + +2. **`main.py` — `maybe_run_discovery`** + - Introduce a `discover.inProgress` Config flag (mirrors `scan.inProgress`). + - A sweep *starts* when `inProgress` is already set, OR (enabled AND schedule due), + OR `discover.requested` is set. + - When starting/continuing, run exactly one chunk (`run_discovery`, which now + selects `chunkSize` seeds). Set `inProgress=true` on start. + - When a chunk returns 0 processed ⇒ the sweep drained: clear `inProgress`, clear + `discover.requested`, write `discover.result` (a short summary; keep the existing + result-writing behaviour of `_run_discovery`). + - Keep the existing "one run per iteration" guard (`maybe_run_discovery` already + dedupes the scheduled-tick vs requested branches, Tier-1+2 fix) — now it's "one + *chunk* per iteration". + +3. All new Config writes use `ON CONFLICT` upserts (resumable + idempotent), matching + the scan-chunk state handling. + +Behavioural notes: +- A rescan-style "Discover now" requested mid-sweep behaves like the scan: the current + sweep finishes, and because `requested` stays set until drain, a fresh eligible pass + is guaranteed afterward. (Seeds already refreshed this sweep are ineligible until + `intervalHours` elapses, so "Discover now" mid-sweep mostly just finishes the current + drain — acceptable.) +- `chunkSize` default 5 keeps each iteration's MB/ListenBrainz work well under the + job-claim cadence. + +--- + +## Testing + +- **A**: worker suite unaffected. Web: unit-test the new `api/monitor/config` GET/PATCH + (mirror the discover-config test if one exists) and the `api/config` DELETE + (allowlist reject + successful delete). UI verified live via Playwright (Monitor + tab toggles + saves; Clear removes a credential and the `· set` hint disappears; + listenBrainzUrl field persists) per the project's node-not-jsdom convention. +- **B**: unit tests as described per fix (musicbrainz escaping, wanted P2002 fallback, + library crash-recovery preamble). +- **C**: extend the discovery worker tests — assert `run_discovery(limit=n)` processes + ≤ n seeds and stamps `lastDiscoveredAt` per processed seed; assert `maybe_run_discovery` + drains across iterations and clears `inProgress`/`requested` on the 0-seed chunk. + Reuse the `test_discovery_trigger.py` harness style. + +Both suites (worker + web) green, `tsc` + web build clean, before merge. Standard +project flow: feature branch → per-workstream commits → opus whole-branch review → +ff-merge to `main` → push → `docker compose up -d --build web worker` (never `down -v`). +No new Prisma migration (all Config rows). + +## Risks / notes + +- `monitor.autoMonitorFuture` surfaced in the UI means a user can now enable + auto-monitoring of future releases; it was already wired, just previously + DB-only. Default stays `false`. +- Retiring `discover.maxSeeds` changes sweep coverage from "up to 50 seeds" to "all + eligible seeds across iterations". Intended — the batch is designed to make this safe + by not blocking the loop.