Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
10 KiB
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_albumcrash 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:
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:
-
discovery.pyDiscoveryConfig: dropmax_seeds; addchunk_size(_int("discover.chunkSize", 5)).- Seed selection
LIMITuseschunk_sizeinstead ofmax_seeds. - Update each seed's
lastDiscoveredAt = now()incrementally, right after that seed is processed (move it out of the trailing batch loop atdiscovery.py:189-190), so a mid-chunk failure still persists progress for completed seeds. run_discoveryreturns the number of seeds processed this call (0 ⇒ drained).
-
main.py—maybe_run_discovery- Introduce a
discover.inProgressConfig flag (mirrorsscan.inProgress). - A sweep starts when
inProgressis already set, OR (enabled AND schedule due), ORdiscover.requestedis set. - When starting/continuing, run exactly one chunk (
run_discovery, which now selectschunkSizeseeds). SetinProgress=trueon start. - When a chunk returns 0 processed ⇒ the sweep drained: clear
inProgress, cleardiscover.requested, writediscover.result(a short summary; keep the existing result-writing behaviour of_run_discovery). - Keep the existing "one run per iteration" guard (
maybe_run_discoveryalready dedupes the scheduled-tick vs requested branches, Tier-1+2 fix) — now it's "one chunk per iteration".
- Introduce a
-
All new Config writes use
ON CONFLICTupserts (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
requestedstays set until drain, a fresh eligible pass is guaranteed afterward. (Seeds already refreshed this sweep are ineligible untilintervalHourselapses, so "Discover now" mid-sweep mostly just finishes the current drain — acceptable.) chunkSizedefault 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/configGET/PATCH (mirror the discover-config test if one exists) and theapi/configDELETE (allowlist reject + successful delete). UI verified live via Playwright (Monitor tab toggles + saves; Clear removes a credential and the· sethint 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 stampslastDiscoveredAtper processed seed; assertmaybe_run_discoverydrains across iterations and clearsinProgress/requestedon the 0-seed chunk. Reuse thetest_discovery_trigger.pyharness 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.autoMonitorFuturesurfaced in the UI means a user can now enable auto-monitoring of future releases; it was already wired, just previously DB-only. Default staysfalse.- Retiring
discover.maxSeedschanges 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.