Files
Lyra/docs/superpowers/specs/2026-07-13-deferred-batch-design.md
2026-07-13 00:07:15 +02:00

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_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:

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 temporalWatchedArtist.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.pymaybe_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.