# 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.