docs: spec for deferred-items batch (settings, correctness, chunk discovery)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Jonathan
@@ -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>
|
||||||
|
```
|
||||||
|
|
||||||
|
`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.
|
||||||
Reference in New Issue
Block a user