Jonathan
913 lines
37 KiB
Markdown
913 lines
37 KiB
Markdown
# Deferred-items Batch Implementation Plan
|
||
|
||
> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.
|
||
|
||
**Goal:** Clear the deferred backlog — a Monitor settings tab + clear-a-credential + `listenBrainzUrl` surface (A), three correctness fixes (B), and a non-blocking chunked discovery sweep (C).
|
||
|
||
**Architecture:** Three independent workstreams on branch `batch/deferred-cleanup`, landing as separate commits. A mirrors the existing Discovery-tab/route patterns; B is three isolated small fixes; C mirrors the scan-chunking design (`main.maybe_run_scan`) using discovery's existing temporal cursor (`WatchedArtist.lastDiscoveredAt`).
|
||
|
||
**Tech Stack:** Next.js App Router (web, TypeScript, vitest against `lyra_test`), Python worker (psycopg, pytest against `lyra_test`), Prisma, Postgres. No new Prisma migration (all Config rows).
|
||
|
||
## Global Constraints
|
||
|
||
- Tests run against `lyra_test` ONLY; a guard refuses destructive ops on non-`*_test` DBs. Never run suites against live `lyra`.
|
||
- Web vitest env is **node, not jsdom** — no DOM-render unit tests. Pure logic + API routes are unit-tested; UI is verified in the real app via Playwright. Preserve existing `aria-label`s / button text (tests depend on them).
|
||
- Deploy is `docker compose up -d --build web worker` (NEVER `down -v`). Not part of this plan — merge + deploy happens after review.
|
||
- Both suites green + `tsc` + web `build` clean before merge.
|
||
- Config route writes use Prisma `upsert`; worker Config writes use `_set_config` (ON CONFLICT).
|
||
- Spec: `docs/superpowers/specs/2026-07-13-deferred-batch-design.md`.
|
||
|
||
---
|
||
|
||
## Workstream A — Settings
|
||
|
||
### Task A1: Monitor config API route
|
||
|
||
**Files:**
|
||
- Create: `web/src/app/api/monitor/config/route.ts`
|
||
- Test: `web/src/app/api/monitor/config/route.test.ts`
|
||
|
||
**Interfaces:**
|
||
- Produces: `GET(): Response` (stored-or-default map), `PATCH(request): Response` (upserts allowlisted keys, returns `{updated: n}`). Allowlist keys: `monitor.enabled`, `monitor.autoMonitorFuture`, `monitor.pollIntervalHours`, `monitor.retryIntervalHours`, `monitor.qualityCutoff`, `monitor.upgradeWindowDays`.
|
||
|
||
- [ ] **Step 1: Write the failing test**
|
||
|
||
```ts
|
||
// web/src/app/api/monitor/config/route.test.ts
|
||
import { describe, it, expect } from "vitest";
|
||
import { prisma } from "@/lib/db";
|
||
import { GET, PATCH } from "./route";
|
||
|
||
function patch(body: unknown) {
|
||
return PATCH(new Request("http://localhost/api/monitor/config", {
|
||
method: "PATCH", headers: { "content-type": "application/json" }, body: JSON.stringify(body),
|
||
}));
|
||
}
|
||
|
||
describe("monitor config API", () => {
|
||
it("GET returns defaults when nothing is stored", async () => {
|
||
const body = await (await GET()).json();
|
||
expect(body).toMatchObject({
|
||
"monitor.enabled": "false", "monitor.autoMonitorFuture": "false",
|
||
"monitor.pollIntervalHours": "24", "monitor.retryIntervalHours": "6",
|
||
"monitor.qualityCutoff": "2", "monitor.upgradeWindowDays": "14",
|
||
});
|
||
});
|
||
|
||
it("PATCH upserts known keys and ignores unknown ones", async () => {
|
||
const res = await patch({ "monitor.enabled": "true", "monitor.pollIntervalHours": 12, "nope": "x" });
|
||
expect((await res.json()).updated).toBe(2);
|
||
const body = await (await GET()).json();
|
||
expect(body["monitor.enabled"]).toBe("true");
|
||
expect(body["monitor.pollIntervalHours"]).toBe("12");
|
||
expect(await prisma.config.findUnique({ where: { key: "nope" } })).toBeNull();
|
||
});
|
||
});
|
||
```
|
||
|
||
- [ ] **Step 2: Run test to verify it fails**
|
||
|
||
Run: `cd web && npx vitest run src/app/api/monitor/config/route.test.ts`
|
||
Expected: FAIL — cannot resolve `./route`.
|
||
|
||
- [ ] **Step 3: Write the route** (clone of `api/discover/config/route.ts`)
|
||
|
||
```ts
|
||
// web/src/app/api/monitor/config/route.ts
|
||
import { prisma } from "@/lib/db";
|
||
|
||
const DEFAULTS: Record<string, string> = {
|
||
"monitor.enabled": "false",
|
||
"monitor.autoMonitorFuture": "false",
|
||
"monitor.pollIntervalHours": "24",
|
||
"monitor.retryIntervalHours": "6",
|
||
"monitor.qualityCutoff": "2",
|
||
"monitor.upgradeWindowDays": "14",
|
||
};
|
||
|
||
export async function GET() {
|
||
const rows = await prisma.config.findMany({ where: { key: { in: Object.keys(DEFAULTS) } } });
|
||
const stored = Object.fromEntries(rows.map((r) => [r.key, r.value]));
|
||
return Response.json(
|
||
Object.fromEntries(Object.entries(DEFAULTS).map(([k, d]) => [k, stored[k] ?? d])),
|
||
);
|
||
}
|
||
|
||
export async function PATCH(request: Request) {
|
||
let body: unknown;
|
||
try {
|
||
body = await request.json();
|
||
} catch {
|
||
return Response.json({ error: "invalid JSON" }, { status: 400 });
|
||
}
|
||
const entries = Object.entries((body ?? {}) as Record<string, unknown>).filter(
|
||
([k]) => k in DEFAULTS,
|
||
);
|
||
for (const [key, value] of entries) {
|
||
await prisma.config.upsert({
|
||
where: { key },
|
||
create: { key, value: String(value), secret: false },
|
||
update: { value: String(value) },
|
||
});
|
||
}
|
||
return Response.json({ updated: entries.length });
|
||
}
|
||
```
|
||
|
||
- [ ] **Step 4: Run test to verify it passes**
|
||
|
||
Run: `cd web && npx vitest run src/app/api/monitor/config/route.test.ts`
|
||
Expected: PASS (2 tests).
|
||
|
||
- [ ] **Step 5: Commit**
|
||
|
||
```bash
|
||
git add web/src/app/api/monitor/config/
|
||
git commit -m "feat(web): monitor config API route"
|
||
```
|
||
|
||
---
|
||
|
||
### Task A2: Monitor settings tab (UI)
|
||
|
||
**Files:**
|
||
- Modify: `web/src/app/settings/settings-form.tsx`
|
||
|
||
**Interfaces:**
|
||
- Consumes: `GET/PATCH /api/monitor/config` (Task A1).
|
||
|
||
UI-only; verified in the real app (node-not-jsdom convention). Mirrors the existing Discovery-tab state/handlers.
|
||
|
||
- [ ] **Step 1: Extend the tab type and list** — in `settings-form.tsx`, change:
|
||
|
||
```ts
|
||
type Tab = "Qobuz" | "Soulseek" | "Library" | "Monitor" | "Discovery";
|
||
const TABS: Tab[] = ["Qobuz", "Soulseek", "Library", "Monitor", "Discovery"];
|
||
```
|
||
|
||
- [ ] **Step 2: Add the Monitor field list constant** (near `DISCOVERY_FIELDS`):
|
||
|
||
```ts
|
||
const MONITOR_FIELDS: [string, string][] = [
|
||
["monitor.pollIntervalHours", "Poll interval (hours)"],
|
||
["monitor.retryIntervalHours", "Retry interval (hours)"],
|
||
["monitor.qualityCutoff", "Quality cutoff"],
|
||
["monitor.upgradeWindowDays", "Upgrade window (days)"],
|
||
];
|
||
```
|
||
|
||
- [ ] **Step 3: Add state + load + save** (mirror `discoverCfg`/`saveDiscovery`):
|
||
|
||
```ts
|
||
const [monitorCfg, setMonitorCfg] = useState<Record<string, string>>({});
|
||
const [monitorSaved, setMonitorSaved] = useState(false);
|
||
|
||
useEffect(() => {
|
||
fetch("/api/monitor/config").then((r) => r.json()).then((c: Record<string, string>) => setMonitorCfg(c));
|
||
}, []);
|
||
|
||
function setMon(key: string, value: string) {
|
||
setMonitorCfg((c) => ({ ...c, [key]: value }));
|
||
}
|
||
|
||
async function saveMonitor(e: React.FormEvent) {
|
||
e.preventDefault();
|
||
await fetch("/api/monitor/config", {
|
||
method: "PATCH",
|
||
headers: { "content-type": "application/json" },
|
||
body: JSON.stringify(monitorCfg),
|
||
});
|
||
setMonitorSaved(true);
|
||
setTimeout(() => setMonitorSaved(false), 1500);
|
||
}
|
||
```
|
||
|
||
- [ ] **Step 4: Render the Monitor tab body** (insert before the `Discovery` tab block):
|
||
|
||
```tsx
|
||
{tab === "Monitor" ? (
|
||
<form className="settings-section" onSubmit={saveMonitor}>
|
||
<div className="save-row" style={{ margin: "6px 0 4px" }}>
|
||
<label className="toggle">
|
||
<input
|
||
type="checkbox"
|
||
checked={monitorCfg["monitor.enabled"] === "true"}
|
||
onChange={(e) => setMon("monitor.enabled", e.target.checked ? "true" : "false")}
|
||
/>
|
||
Automatic monitoring (grab & upgrade)
|
||
</label>
|
||
</div>
|
||
<div className="save-row" style={{ margin: "2px 0 8px" }}>
|
||
<label className="toggle">
|
||
<input
|
||
type="checkbox"
|
||
checked={monitorCfg["monitor.autoMonitorFuture"] === "true"}
|
||
onChange={(e) => setMon("monitor.autoMonitorFuture", e.target.checked ? "true" : "false")}
|
||
/>
|
||
Auto-monitor future releases from watched artists
|
||
</label>
|
||
</div>
|
||
<div className="field-grid">
|
||
{MONITOR_FIELDS.map(([key, label]) => (
|
||
<label key={key} className="field">
|
||
<span>{label}</span>
|
||
<input type="number" step="any" value={monitorCfg[key] ?? ""} onChange={(e) => setMon(key, e.target.value)} />
|
||
</label>
|
||
))}
|
||
</div>
|
||
<div className="save-row">
|
||
<button type="submit" className="btn">Save monitor settings</button>
|
||
{monitorSaved ? <span className="saved-note">Saved</span> : null}
|
||
</div>
|
||
</form>
|
||
) : null}
|
||
```
|
||
|
||
- [ ] **Step 5: Verify — build + real app**
|
||
|
||
Run: `cd web && npx tsc --noEmit && npm run build`
|
||
Expected: clean.
|
||
Then in the running app (Playwright): open Settings → Monitor, toggle "Automatic monitoring" on, set poll interval, Save → "Saved" appears; reload → values persist. Confirm the DB row: `monitor.enabled=true` in Config.
|
||
|
||
- [ ] **Step 6: Commit**
|
||
|
||
```bash
|
||
git add web/src/app/settings/settings-form.tsx
|
||
git commit -m "feat(web): Monitor settings tab"
|
||
```
|
||
|
||
---
|
||
|
||
### Task A3: Clear-a-credential API (DELETE)
|
||
|
||
**Files:**
|
||
- Modify: `web/src/app/api/config/route.ts`
|
||
- Test: `web/src/app/api/config/route.test.ts`
|
||
|
||
**Interfaces:**
|
||
- Produces: `DELETE(request): Response` — `?field=<name>` validated against the existing `FIELDS` allowlist; deletes that key's Config row. Unknown field → 400. Valid → `{ok:true}` (idempotent).
|
||
|
||
- [ ] **Step 1: Write the failing test** (append to `route.test.ts`)
|
||
|
||
```ts
|
||
import { GET, PUT, DELETE } from "./route"; // update the existing import line
|
||
|
||
function delReq(field: string) {
|
||
return new Request(`http://localhost/api/config?field=${field}`, { method: "DELETE" });
|
||
}
|
||
|
||
describe("config DELETE", () => {
|
||
it("removes a stored secret by field name", async () => {
|
||
await PUT(putReq({ qobuzPassword: "hunter2" }));
|
||
expect(await prisma.config.findUnique({ where: { key: "qobuz.password" } })).not.toBeNull();
|
||
const res = await DELETE(delReq("qobuzPassword"));
|
||
expect(res.status).toBe(200);
|
||
expect(await prisma.config.findUnique({ where: { key: "qobuz.password" } })).toBeNull();
|
||
// GET now reports it unset
|
||
expect((await (await GET()).json()).qobuzPasswordSet).toBe(false);
|
||
});
|
||
|
||
it("rejects an unknown field with 400 and deletes nothing", async () => {
|
||
const res = await DELETE(delReq("monitor.enabled"));
|
||
expect(res.status).toBe(400);
|
||
});
|
||
|
||
it("is idempotent — deleting an absent field returns 200", async () => {
|
||
const res = await DELETE(delReq("slskdApiKey"));
|
||
expect(res.status).toBe(200);
|
||
});
|
||
});
|
||
```
|
||
|
||
- [ ] **Step 2: Run to verify it fails**
|
||
|
||
Run: `cd web && npx vitest run src/app/api/config/route.test.ts`
|
||
Expected: FAIL — `DELETE` not exported.
|
||
|
||
- [ ] **Step 3: Add the DELETE handler** to `route.ts` (after `GET`):
|
||
|
||
```ts
|
||
export async function DELETE(request: Request) {
|
||
const field = new URL(request.url).searchParams.get("field") ?? "";
|
||
const entry = FIELDS[field];
|
||
if (!entry) {
|
||
return Response.json({ error: "unknown field" }, { status: 400 });
|
||
}
|
||
await prisma.config.deleteMany({ where: { key: entry.key } }); // idempotent
|
||
return Response.json({ ok: true });
|
||
}
|
||
```
|
||
|
||
- [ ] **Step 4: Run to verify it passes**
|
||
|
||
Run: `cd web && npx vitest run src/app/api/config/route.test.ts`
|
||
Expected: PASS (all, including the 3 new).
|
||
|
||
- [ ] **Step 5: Commit**
|
||
|
||
```bash
|
||
git add web/src/app/api/config/route.ts web/src/app/api/config/route.test.ts
|
||
git commit -m "feat(web): DELETE /api/config?field= to clear a credential"
|
||
```
|
||
|
||
---
|
||
|
||
### Task A4: Clear-credential buttons (UI)
|
||
|
||
**Files:**
|
||
- Modify: `web/src/app/settings/settings-form.tsx`
|
||
|
||
**Interfaces:**
|
||
- Consumes: `DELETE /api/config?field=` (Task A3). Reuses the existing `toast` host if present in the tree (import path `../_ui/toast` — check current usage in the file; if toasts aren't wired into this component, skip the toast and just flip the flag).
|
||
|
||
- [ ] **Step 1: Add a clear handler** in the component:
|
||
|
||
```ts
|
||
async function clearField(field: "qobuzPassword" | "qobuzToken" | "slskdApiKey") {
|
||
await fetch(`/api/config?field=${field}`, { method: "DELETE" });
|
||
if (field === "qobuzPassword") { setPwSet(false); setQobuzPassword(""); }
|
||
if (field === "qobuzToken") { setTokenSet(false); setQobuzToken(""); }
|
||
if (field === "slskdApiKey") { setKeySet(false); setSlskdApiKey(""); }
|
||
}
|
||
```
|
||
|
||
- [ ] **Step 2: Render a Clear button beside each set secret.** Replace the `secretHint` usage on the three secret `<span>`s so a set secret also shows a Clear button. Example for the Qobuz password label (apply the same shape to auth token and slskd api key):
|
||
|
||
```tsx
|
||
<span>
|
||
Qobuz password{secretHint(pwSet)}
|
||
{pwSet ? (
|
||
<button type="button" className="btn ghost sm" style={{ marginLeft: 8 }}
|
||
aria-label="clear qobuz password" onClick={() => clearField("qobuzPassword")}>
|
||
Clear
|
||
</button>
|
||
) : null}
|
||
</span>
|
||
```
|
||
|
||
For the auth token span use `tokenSet` + `clearField("qobuzToken")` + `aria-label="clear qobuz auth token"`; for the slskd api key span use `keySet` + `clearField("slskdApiKey")` + `aria-label="clear slskd api key"`.
|
||
|
||
- [ ] **Step 3: Verify — build + real app**
|
||
|
||
Run: `cd web && npx tsc --noEmit && npm run build`
|
||
Expected: clean.
|
||
Real app (Playwright): save a Qobuz password → reload → "· set" + Clear shown; click Clear → hint + button disappear; reload → still cleared; confirm `qobuz.password` row absent in Config.
|
||
|
||
- [ ] **Step 4: Commit**
|
||
|
||
```bash
|
||
git add web/src/app/settings/settings-form.tsx
|
||
git commit -m "feat(web): clear-credential buttons on Settings"
|
||
```
|
||
|
||
---
|
||
|
||
### Task A5: Surface `discover.listenBrainzUrl`
|
||
|
||
**Files:**
|
||
- Modify: `web/src/app/api/discover/config/route.ts`
|
||
- Modify: `web/src/app/api/discover/config/route.test.ts`
|
||
- Modify: `web/src/app/settings/settings-form.tsx`
|
||
|
||
**Interfaces:**
|
||
- Produces: `discover.listenBrainzUrl` in the discover-config DEFAULTS (default `""`), rendered as a text field in the Discovery tab.
|
||
|
||
- [ ] **Step 1: Update the failing test** — in `route.test.ts`, extend the defaults assertion:
|
||
|
||
```ts
|
||
expect(body).toMatchObject({
|
||
"discover.enabled": "false", "discover.intervalHours": "168",
|
||
"discover.similarPerSeed": "20", "discover.albumsPerArtist": "1",
|
||
"discover.minScore": "0", "discover.chunkSize": "5", "discover.listenBrainzUrl": "",
|
||
});
|
||
```
|
||
|
||
(Note: `discover.maxSeeds` is removed here and `discover.chunkSize` added — this test line is also touched by Task C3. Whichever task runs second reconciles to this final shape.)
|
||
|
||
- [ ] **Step 2: Run to verify it fails**
|
||
|
||
Run: `cd web && npx vitest run src/app/api/discover/config/route.test.ts`
|
||
Expected: FAIL — missing keys.
|
||
|
||
- [ ] **Step 3: Update DEFAULTS** in `api/discover/config/route.ts` — remove `"discover.maxSeeds"`, add:
|
||
|
||
```ts
|
||
"discover.chunkSize": "5",
|
||
"discover.listenBrainzUrl": "",
|
||
```
|
||
|
||
(Final DEFAULTS: `discover.enabled`, `discover.intervalHours`, `discover.similarPerSeed`, `discover.albumsPerArtist`, `discover.minScore`, `discover.chunkSize`, `discover.listenBrainzUrl`.)
|
||
|
||
- [ ] **Step 4: Update the Discovery tab fields** in `settings-form.tsx` — replace `DISCOVERY_FIELDS`:
|
||
|
||
```ts
|
||
const DISCOVERY_FIELDS: [string, string][] = [
|
||
["discover.intervalHours", "Interval (hours)"],
|
||
["discover.chunkSize", "Seeds per chunk"],
|
||
["discover.similarPerSeed", "Similar per seed"],
|
||
["discover.albumsPerArtist", "Albums per artist"],
|
||
["discover.minScore", "Min score"],
|
||
];
|
||
```
|
||
|
||
Then add a text field for the URL below the grid, inside the Discovery `<form>` (before the save-row):
|
||
|
||
```tsx
|
||
<label className="field">
|
||
<span>ListenBrainz base URL (blank = default)</span>
|
||
<input value={discoverCfg["discover.listenBrainzUrl"] ?? ""}
|
||
onChange={(e) => setCfg("discover.listenBrainzUrl", e.target.value)} />
|
||
</label>
|
||
```
|
||
|
||
- [ ] **Step 5: Run tests + build**
|
||
|
||
Run: `cd web && npx vitest run src/app/api/discover/config/route.test.ts && npx tsc --noEmit`
|
||
Expected: PASS + clean. (The `discover.maxSeeds` removal is consistent with Task C3.)
|
||
|
||
- [ ] **Step 6: Commit**
|
||
|
||
```bash
|
||
git add web/src/app/api/discover/config/ web/src/app/settings/settings-form.tsx
|
||
git commit -m "feat(web): surface discover.listenBrainzUrl; swap maxSeeds->chunkSize in Discovery tab"
|
||
```
|
||
|
||
---
|
||
|
||
## Workstream B — Correctness
|
||
|
||
### Task B1: Lucene phrase escaping
|
||
|
||
**Files:**
|
||
- Modify: `web/src/lib/musicbrainz.ts:57-58`
|
||
- Test: `web/src/lib/musicbrainz.test.ts`
|
||
|
||
**Interfaces:**
|
||
- Produces: internal `escapeLucenePhrase(s: string): string`.
|
||
|
||
- [ ] **Step 1: Write the failing test** (append to `musicbrainz.test.ts`) — asserts the built query is escaped. Mock `mbGet` via the module's fetch. Match the file's existing mocking style; if it stubs `global.fetch`, assert on the URL captured:
|
||
|
||
```ts
|
||
import { describe, it, expect, vi, afterEach } from "vitest";
|
||
import * as mb from "./musicbrainz";
|
||
|
||
afterEach(() => vi.restoreAllMocks());
|
||
|
||
it("escapes quotes in the release-group query", async () => {
|
||
const spy = vi.spyOn(globalThis, "fetch").mockResolvedValue(
|
||
new Response(JSON.stringify({ "release-groups": [] }), { status: 200 }),
|
||
);
|
||
await mb.searchReleaseGroup('AC/DC', 'Back in "Black"');
|
||
const url = String(spy.mock.calls[0][0]);
|
||
// the decoded query must contain the escaped quote, not a bare one that closes the phrase
|
||
expect(decodeURIComponent(url)).toContain('Back in \\"Black\\"');
|
||
});
|
||
```
|
||
|
||
(If `musicbrainz.test.ts` already has a shared fetch-mock helper, reuse it instead of re-mocking; keep the assertion on the escaped `\\"`.)
|
||
|
||
- [ ] **Step 2: Run to verify it fails**
|
||
|
||
Run: `cd web && npx vitest run src/lib/musicbrainz.test.ts`
|
||
Expected: FAIL — query contains an unescaped `"`.
|
||
|
||
- [ ] **Step 3: Add the helper and use it** in `musicbrainz.ts`:
|
||
|
||
```ts
|
||
function escapeLucenePhrase(s: string): string {
|
||
return s.replace(/[\\"]/g, (c) => "\\" + c);
|
||
}
|
||
```
|
||
|
||
Change line 58 to:
|
||
|
||
```ts
|
||
const query = `releasegroup:"${escapeLucenePhrase(album)}" AND artist:"${escapeLucenePhrase(artist)}"`;
|
||
```
|
||
|
||
- [ ] **Step 4: Run to verify it passes**
|
||
|
||
Run: `cd web && npx vitest run src/lib/musicbrainz.test.ts`
|
||
Expected: PASS.
|
||
|
||
- [ ] **Step 5: Commit**
|
||
|
||
```bash
|
||
git add web/src/lib/musicbrainz.ts web/src/lib/musicbrainz.test.ts
|
||
git commit -m "fix(web): escape Lucene quotes in searchReleaseGroup query"
|
||
```
|
||
|
||
---
|
||
|
||
### Task B2: wanted-POST unique-race fallback
|
||
|
||
**Files:**
|
||
- Modify: `web/src/app/api/wanted/route.ts:52-64`
|
||
- Test: `web/src/app/api/wanted/route.test.ts`
|
||
|
||
**Interfaces:**
|
||
- Produces: the POST create wrapped in a `try/catch` that, on Prisma `P2002`, re-reads the row and returns the same 200 update-path response.
|
||
|
||
- [ ] **Step 1: Write the failing test** — simulate the race by inserting the row between the existence check and create. Match the existing `route.test.ts` style (it drives `POST` with a mocked MB match). Add:
|
||
|
||
```ts
|
||
it("handles a concurrent duplicate (P2002) by returning the existing release", async () => {
|
||
// Pre-create the row so the create() inside POST hits the unique constraint.
|
||
// (Mirror how other tests in this file stub the MB match; reuse that helper.)
|
||
// After POST, expect a 200 with the existing release, not a 500.
|
||
const res = await POST(postReqForKnownMatch());
|
||
expect([200, 201]).toContain(res.status);
|
||
// second identical POST must also be 200 (idempotent), never 500
|
||
const res2 = await POST(postReqForKnownMatch());
|
||
expect(res2.status).toBe(200);
|
||
});
|
||
```
|
||
|
||
(Use the file's existing MB-match stubbing helper — named `postReqForKnownMatch` here as a placeholder for whatever the current tests use; if the tests stub `searchReleaseGroup`, stub it to a fixed `rgMbid` and call `POST` twice: the first inserts, the second exercises the update/P2002 path.)
|
||
|
||
- [ ] **Step 2: Run to verify it fails** — before the fix, a genuine concurrent create would 500; the two-call test may already pass via the existing `findUnique`→update branch, so ALSO add a direct P2002 assertion by forcing `create` to throw:
|
||
|
||
```ts
|
||
import { Prisma } from "@prisma/client";
|
||
it("catches a P2002 thrown by create and falls back to update", async () => {
|
||
const spy = vi.spyOn(prisma.monitoredRelease, "create").mockRejectedValueOnce(
|
||
new Prisma.PrismaClientKnownRequestError("dup", { code: "P2002", clientVersion: "x" }),
|
||
);
|
||
// ensure the row exists so the fallback update finds it
|
||
// (insert it via prisma.monitoredRelease.create directly before mocking, or via a prior POST)
|
||
const res = await POST(postReqForKnownMatch());
|
||
expect(res.status).toBe(200);
|
||
spy.mockRestore();
|
||
});
|
||
```
|
||
|
||
Run: `cd web && npx vitest run src/app/api/wanted/route.test.ts`
|
||
Expected: FAIL — the un-caught P2002 rejects / 500s.
|
||
|
||
- [ ] **Step 3: Wrap the create** in `wanted/route.ts`. Replace the `const created = await prisma.monitoredRelease.create({...})` block (lines 52-64) with:
|
||
|
||
```ts
|
||
try {
|
||
const created = await prisma.monitoredRelease.create({
|
||
data: {
|
||
artistMbid: match.artistMbid,
|
||
artistName: match.artistName,
|
||
rgMbid: match.rgMbid,
|
||
album: match.title,
|
||
primaryType: match.primaryType,
|
||
secondaryTypes: match.secondaryTypes,
|
||
firstReleaseDate: match.firstReleaseDate,
|
||
monitored: true,
|
||
},
|
||
});
|
||
return Response.json({ id: created.id, album: created.album, monitored: created.monitored }, { status: 201 });
|
||
} catch (e) {
|
||
if (e instanceof Prisma.PrismaClientKnownRequestError && e.code === "P2002") {
|
||
// Concurrent Want for the same release-group won the create; converge to the update path.
|
||
const updated = await prisma.monitoredRelease.update({
|
||
where: { rgMbid: match.rgMbid },
|
||
data: { monitored: true },
|
||
});
|
||
return Response.json({ id: updated.id, album: updated.album, monitored: updated.monitored }, { status: 200 });
|
||
}
|
||
throw e;
|
||
}
|
||
```
|
||
|
||
Add the import at the top: `import { Prisma } from "@prisma/client";`
|
||
|
||
- [ ] **Step 4: Run to verify it passes**
|
||
|
||
Run: `cd web && npx vitest run src/app/api/wanted/route.test.ts`
|
||
Expected: PASS.
|
||
|
||
- [ ] **Step 5: Commit**
|
||
|
||
```bash
|
||
git add web/src/app/api/wanted/route.ts web/src/app/api/wanted/route.test.ts
|
||
git commit -m "fix(web): wanted POST converges on P2002 unique race instead of 500"
|
||
```
|
||
|
||
---
|
||
|
||
### Task B3: `import_album` crash-recovery preamble
|
||
|
||
**Files:**
|
||
- Modify: `worker/lyra_worker/library.py:62-68`
|
||
- Test: `worker/tests/test_library.py` (or the existing import_album test file — locate with `grep -rln import_album worker/tests`)
|
||
|
||
**Interfaces:**
|
||
- Produces: crash-recovery at the top of `import_album` — restores `{final}` from `{final}.old` if `final` is missing, else drops a stale `.old`; always drops a stale `.importing`.
|
||
|
||
- [ ] **Step 1: Write the failing tests** — add to the located import_album test file:
|
||
|
||
```python
|
||
def test_import_album_recovers_from_interrupted_swap(tmp_path):
|
||
from lyra_worker.library import import_album
|
||
final = str(tmp_path / "Artist" / "Album (2020)")
|
||
os.makedirs(final + ".old")
|
||
open(os.path.join(final + ".old", "01 Song.flac"), "w").close()
|
||
# `final` is missing (crash happened after rename(final, old), before rename(tmp, final))
|
||
staging = str(tmp_path / "staging") # empty/missing staging
|
||
import_album(staging, final)
|
||
# previous good copy restored to `final`; no leftover .old
|
||
assert os.path.isfile(os.path.join(final, "01 Song.flac"))
|
||
assert not os.path.isdir(final + ".old")
|
||
|
||
def test_import_album_drops_stale_old_when_final_present(tmp_path):
|
||
from lyra_worker.library import import_album
|
||
final = str(tmp_path / "Artist" / "Album (2020)")
|
||
os.makedirs(final)
|
||
open(os.path.join(final, "keep.flac"), "w").close()
|
||
os.makedirs(final + ".old") # leftover from a completed swap
|
||
open(os.path.join(final + ".old", "stale.flac"), "w").close()
|
||
import_album(str(tmp_path / "staging"), final)
|
||
assert not os.path.isdir(final + ".old") # stale .old removed
|
||
assert os.path.isfile(os.path.join(final, "keep.flac")) # existing copy untouched by preamble
|
||
```
|
||
|
||
(Ensure `import os` is present in the test module.)
|
||
|
||
- [ ] **Step 2: Run to verify it fails**
|
||
|
||
Run: `cd worker && python -m pytest tests/test_library.py -k "interrupted or stale_old" -v`
|
||
Expected: FAIL — orphan `.old` not handled (`final` not restored).
|
||
|
||
- [ ] **Step 3: Add the preamble** at the top of `import_album`, immediately after the docstring and before `os.makedirs(os.path.dirname(final), exist_ok=True)`:
|
||
|
||
```python
|
||
# Recover from a swap interrupted by a crash: {final}.old holds the previous copy.
|
||
old = final + ".old"
|
||
if os.path.isdir(old) and not os.path.isdir(final):
|
||
os.rename(old, final) # restore the last-good copy
|
||
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
|
||
```
|
||
|
||
- [ ] **Step 4: Run to verify it passes**
|
||
|
||
Run: `cd worker && python -m pytest tests/test_library.py -k "interrupted or stale_old" -v`
|
||
Expected: PASS. Then run the whole file to confirm no regression: `python -m pytest tests/test_library.py -v`.
|
||
|
||
- [ ] **Step 5: Commit**
|
||
|
||
```bash
|
||
git add worker/lyra_worker/library.py worker/tests/test_library.py
|
||
git commit -m "fix(worker): import_album recovers orphaned .old/.importing from an interrupted swap"
|
||
```
|
||
|
||
---
|
||
|
||
## Workstream C — Chunk discovery
|
||
|
||
### Task C1: chunk_size config + seed-count result
|
||
|
||
**Files:**
|
||
- Modify: `worker/lyra_worker/discovery.py` (DiscoveryConfig, DiscoveryResult, `_seed_artists`, `run_discovery` return)
|
||
- Test: `worker/tests/test_discovery.py`
|
||
|
||
**Interfaces:**
|
||
- Produces: `DiscoveryConfig.chunk_size: int` (from `discover.chunkSize`, default 5; `max_seeds` removed); `DiscoveryResult.seeds: int`; `_seed_artists` LIMIT uses `chunk_size`; `run_discovery` returns `DiscoveryResult(artists, albums, seeds=len(seeds))`.
|
||
- Consumed by: Task C2 (`result.seeds == 0` ⇒ sweep drained).
|
||
|
||
- [ ] **Step 1: Write the failing test** (append to `test_discovery.py`) — mirror its existing harness:
|
||
|
||
```python
|
||
def test_run_discovery_processes_at_most_chunk_size_seeds(conn):
|
||
from lyra_worker.discovery import DiscoveryConfig, run_discovery
|
||
from lyra_worker.adapters.fakes import FakeMbBrowser, FakeSimilaritySource
|
||
from lyra_worker.similarity.base import SimilarArtist
|
||
from tests.conftest import insert_watched_artist
|
||
for i in range(4):
|
||
insert_watched_artist(conn, mbid=f"s{i}", name=f"Seed{i}")
|
||
src = FakeSimilaritySource(similar={f"s{i}": [SimilarArtist(f"c{i}", f"Cand{i}", 0.9)] for i in range(4)})
|
||
cfg = DiscoveryConfig.from_config({"discover.chunkSize": "2"})
|
||
result = run_discovery(conn, [src], FakeMbBrowser(), cfg)
|
||
assert result.seeds == 2 # only chunk_size seeds this call
|
||
# exactly 2 seeds now have lastDiscoveredAt set
|
||
with conn.cursor() as cur:
|
||
cur.execute('SELECT count(*) FROM "WatchedArtist" WHERE "lastDiscoveredAt" IS NOT NULL')
|
||
assert cur.fetchone()[0] == 2
|
||
```
|
||
|
||
- [ ] **Step 2: Run to verify it fails**
|
||
|
||
Run: `cd worker && python -m pytest tests/test_discovery.py -k chunk_size -v`
|
||
Expected: FAIL — `chunk_size`/`result.seeds` don't exist.
|
||
|
||
- [ ] **Step 3: Edit `discovery.py`**
|
||
|
||
In `DiscoveryConfig`: remove `max_seeds: int = 50`; add `chunk_size: int = 5`. In `from_config`, remove the `max_seeds=` line and add:
|
||
|
||
```python
|
||
chunk_size=_int("discover.chunkSize", 5),
|
||
```
|
||
|
||
In `DiscoveryResult`, add a field:
|
||
|
||
```python
|
||
@dataclass(frozen=True)
|
||
class DiscoveryResult:
|
||
artists: int
|
||
albums: int
|
||
seeds: int = 0
|
||
```
|
||
|
||
In `_seed_artists`, change the LIMIT argument from `cfg.max_seeds` to `cfg.chunk_size`:
|
||
|
||
```python
|
||
(cfg.interval_hours, cfg.chunk_size),
|
||
```
|
||
|
||
In `run_discovery`, change the final return to include the seed count:
|
||
|
||
```python
|
||
return DiscoveryResult(artists=artists, albums=albums, seeds=len(seeds))
|
||
```
|
||
|
||
- [ ] **Step 4: Run to verify it passes**
|
||
|
||
Run: `cd worker && python -m pytest tests/test_discovery.py -v`
|
||
Expected: PASS (new test + existing discovery tests unaffected — they don't assert on `max_seeds`).
|
||
|
||
- [ ] **Step 5: Commit**
|
||
|
||
```bash
|
||
git add worker/lyra_worker/discovery.py worker/tests/test_discovery.py
|
||
git commit -m "feat(worker): discovery chunk_size + seed-count in result (retire max_seeds)"
|
||
```
|
||
|
||
---
|
||
|
||
### Task C2: chunked `maybe_run_discovery`
|
||
|
||
**Files:**
|
||
- Modify: `worker/lyra_worker/main.py` (`_run_discovery`, `maybe_run_discovery`)
|
||
- Test: `worker/tests/test_discovery_trigger.py`
|
||
|
||
**Interfaces:**
|
||
- Consumes: `run_discovery` → `DiscoveryResult` with `.seeds` (Task C1).
|
||
- Produces: `_run_discovery(conn, sources, browser, config=None) -> DiscoveryResult` (runs ONE chunk, no longer writes result/clears flags); `maybe_run_discovery(...)` drives one chunk per call with a `discover.inProgress` flag, accumulating `discover.progress` and writing `discover.result` + clearing `inProgress`/`requested` on drain.
|
||
|
||
- [ ] **Step 1: Update the existing tests** in `test_discovery_trigger.py` to the new contract.
|
||
|
||
Replace `test_run_discovery_writes_result_and_clears_flag` (the result/flag writes now live in `maybe_run_discovery`) with a drain-based test, and make the monkeypatched `_run_discovery` in the two "runs once" tests return a `DiscoveryResult` (so the caller can inspect `.seeds`):
|
||
|
||
```python
|
||
from lyra_worker.discovery import DiscoveryConfig, DiscoveryResult
|
||
|
||
def test_maybe_run_discovery_drains_across_chunks_and_finishes(conn):
|
||
from lyra_worker.adapters.fakes import FakeMbBrowser, FakeSimilaritySource
|
||
from lyra_worker.similarity.base import SimilarArtist
|
||
from tests.conftest import insert_watched_artist
|
||
for i in range(3):
|
||
insert_watched_artist(conn, mbid=f"s{i}", name=f"Seed{i}")
|
||
_set(conn, "discover.requested", "true")
|
||
_set(conn, "discover.chunkSize", "2")
|
||
src = FakeSimilaritySource(similar={f"s{i}": [SimilarArtist(f"c{i}", f"C{i}", 0.9)] for i in range(3)})
|
||
cfg = {"discover.enabled": "false", "discover.requested": "true", "discover.chunkSize": "2"}
|
||
tick = 0.0
|
||
# iteration 1: starts sweep, processes 2 seeds, still in progress
|
||
tick = maybe_run_discovery(conn, [src], FakeMbBrowser(), cfg, DiscoveryConfig.from_config(cfg), 1.0, tick)
|
||
assert _get(conn, "discover.inProgress") == "true"
|
||
assert _get(conn, "discover.requested") == "false" # cleared at sweep start
|
||
# refresh the snapshot the way the loop does
|
||
cfg2 = {**cfg, "discover.requested": "false", "discover.inProgress": "true",
|
||
"discover.progress": _get(conn, "discover.progress")}
|
||
# iteration 2: processes the last seed
|
||
maybe_run_discovery(conn, [src], FakeMbBrowser(), cfg2, DiscoveryConfig.from_config(cfg2), 2.0, tick)
|
||
cfg3 = {**cfg2, "discover.progress": _get(conn, "discover.progress")}
|
||
# iteration 3: 0 seeds left -> drains, writes result, clears inProgress
|
||
maybe_run_discovery(conn, [src], FakeMbBrowser(), cfg3, DiscoveryConfig.from_config(cfg3), 3.0, tick)
|
||
assert _get(conn, "discover.inProgress") == "false"
|
||
assert "artists" in (_get(conn, "discover.result") or "")
|
||
|
||
def test_maybe_run_discovery_runs_once_when_due_and_requested(conn, monkeypatch):
|
||
calls = []
|
||
monkeypatch.setattr(main, "_run_discovery",
|
||
lambda *a, **k: (calls.append(1), DiscoveryResult(0, 0, seeds=0))[1])
|
||
config = {"discover.enabled": "true", "discover.requested": "true"}
|
||
dcfg = DiscoveryConfig.from_config(config)
|
||
new_tick = maybe_run_discovery(conn, [], FakeMbBrowser(), config, dcfg, now=10_000.0, last_discover_tick=0.0)
|
||
assert calls == [1] # exactly one chunk
|
||
assert new_tick == 10_000.0 # schedule tick advanced
|
||
```
|
||
|
||
Also update `test_maybe_run_discovery_requested_when_disabled_does_not_advance_tick` (and any other `_run_discovery` monkeypatch) the same way — return `DiscoveryResult(0, 0, seeds=0)`. Update the top-of-file import: `from lyra_worker.discovery import DiscoveryConfig, DiscoveryResult`.
|
||
|
||
- [ ] **Step 2: Run to verify the drain test fails**
|
||
|
||
Run: `cd worker && python -m pytest tests/test_discovery_trigger.py -v`
|
||
Expected: FAIL — no `discover.inProgress` handling yet.
|
||
|
||
- [ ] **Step 3: Rewrite `_run_discovery` and `maybe_run_discovery`** in `main.py`.
|
||
|
||
Replace `_run_discovery` with a thin one-chunk runner (no side-effect config writes):
|
||
|
||
```python
|
||
def _run_discovery(conn, sources, browser, config=None) -> DiscoveryResult:
|
||
cfg = DiscoveryConfig.from_config(config if config is not None else get_config(conn))
|
||
return run_discovery(conn, sources, browser, cfg)
|
||
```
|
||
|
||
Add `DiscoveryResult` to the discovery import at the top of `main.py`:
|
||
|
||
```python
|
||
from lyra_worker.discovery import DiscoveryConfig, DiscoveryResult, run_discovery
|
||
```
|
||
|
||
Replace `maybe_run_discovery` with the chunked version:
|
||
|
||
```python
|
||
def maybe_run_discovery(conn, sources, browser, config, dcfg, now, last_discover_tick,
|
||
tick_seconds: float = DISCOVER_TICK_SECONDS) -> float:
|
||
"""Advance the discovery sweep by at most one chunk (``discover.chunkSize`` seeds) per call,
|
||
so the worker loop keeps claiming jobs between chunks. A sweep starts on ``discover.requested``
|
||
or a due schedule tick, then continues via ``discover.inProgress`` until a chunk drains 0 seeds
|
||
(every eligible seed refreshed), at which point ``discover.result`` is written. Returns the
|
||
(possibly advanced) last_discover_tick."""
|
||
requested = str(config.get("discover.requested", "")).strip().lower() in _TRUE
|
||
in_progress = str(config.get("discover.inProgress", "")).strip().lower() in _TRUE
|
||
due = dcfg.enabled and now - last_discover_tick >= tick_seconds
|
||
if not (requested or due or in_progress):
|
||
return last_discover_tick
|
||
|
||
starting = not in_progress
|
||
if starting: # begin a fresh sweep
|
||
_set_config(conn, "discover.inProgress", "true")
|
||
_set_config(conn, "discover.requested", "false")
|
||
|
||
try:
|
||
result = _run_discovery(conn, sources, browser, config) # one chunk
|
||
except Exception as e: # a discovery error must never kill the worker
|
||
print(f"worker: discovery run failed: {e}", flush=True)
|
||
conn.rollback()
|
||
_set_config(conn, "discover.inProgress", "false") # abandon; a new request restarts it
|
||
_set_config(conn, "discover.requested", "false")
|
||
return now if due else last_discover_tick
|
||
|
||
a_prev, b_prev = (0, 0) if starting else _parse_progress(config.get("discover.progress", ""))
|
||
a_total, b_total = a_prev + result.artists, b_prev + result.albums
|
||
if result.seeds == 0: # every eligible seed refreshed -> sweep drained
|
||
_set_config(conn, "discover.result", f"artists {a_total}, albums {b_total}")
|
||
_set_config(conn, "discover.progress", "")
|
||
_set_config(conn, "discover.inProgress", "false")
|
||
print(f"worker: discovery done — {a_total} artists, {b_total} albums", flush=True)
|
||
else:
|
||
_set_config(conn, "discover.progress", f"{a_total}/{b_total}")
|
||
return now if due else last_discover_tick
|
||
```
|
||
|
||
(`_parse_progress` already exists from the scan-chunk work and is reused as-is.)
|
||
|
||
- [ ] **Step 4: Run to verify it passes**
|
||
|
||
Run: `cd worker && python -m pytest tests/test_discovery_trigger.py -v`
|
||
Expected: PASS. Then the whole discovery suite: `python -m pytest tests/ -k discovery -v`.
|
||
|
||
- [ ] **Step 5: Commit**
|
||
|
||
```bash
|
||
git add worker/lyra_worker/main.py worker/tests/test_discovery_trigger.py
|
||
git commit -m "feat(worker): chunk the discovery sweep (one chunk per loop iteration)"
|
||
```
|
||
|
||
---
|
||
|
||
### Task C3: Reconcile Settings/route for chunkSize
|
||
|
||
**Note:** the `api/discover/config` DEFAULTS and the Discovery-tab field list are already updated in **Task A5** (maxSeeds → chunkSize + listenBrainzUrl). If A5 ran before this workstream, there is nothing to do here — verify only. If C ran first, do A5 now.
|
||
|
||
- [ ] **Step 1: Verify the discover-config route + Settings reflect chunkSize, not maxSeeds**
|
||
|
||
Run: `cd web && npx vitest run src/app/api/discover/config/route.test.ts && grep -n "maxSeeds" web/src/app/settings/settings-form.tsx web/src/app/api/discover/config/route.ts`
|
||
Expected: tests PASS; grep returns NOTHING (no lingering `maxSeeds` in UI/route).
|
||
|
||
- [ ] **Step 2: (only if A5 not yet done)** execute Task A5's steps 1–6.
|
||
|
||
---
|
||
|
||
## Final verification (before merge)
|
||
|
||
- [ ] **Full suites + build**
|
||
|
||
```bash
|
||
cd worker && python -m pytest -q
|
||
cd ../web && npx vitest run && npx tsc --noEmit && npm run build
|
||
```
|
||
|
||
Expected: worker all green; web all green; tsc clean; build clean.
|
||
|
||
- [ ] **Real-app smoke (Playwright, both themes):** Settings → Monitor toggles + persists; Clear removes a credential; Discovery tab shows "Seeds per chunk" + ListenBrainz URL; enabling `monitor.enabled` from the UI is reflected in Config. Worker logs a chunked discovery drain when "Discover now" is pressed (with discovery temporarily enabled).
|
||
|
||
- [ ] **Whole-branch opus review**, then ff-merge `batch/deferred-cleanup` → `main`, push. Deploy separately (`docker compose up -d --build web worker`).
|
||
|
||
---
|
||
|
||
## Self-review notes (plan author)
|
||
|
||
- **Spec coverage:** A1/A2 = Monitor tab; A3/A4 = clear-a-credential; A5 = listenBrainzUrl; B1/B2/B3 = the three correctness fixes; C1/C2/C3 = chunk discovery + maxSeeds retirement. All spec sections mapped.
|
||
- **Cross-task type consistency:** `DiscoveryResult.seeds` (C1) is consumed by C2's drain check; `_run_discovery` return-type change (C2) matches C1's `run_discovery` return; `discover.chunkSize`/removal of `discover.maxSeeds` touched consistently in A5 (route/UI) and C1 (worker) — A5 and C3 explicitly reconcile whichever runs second.
|
||
- **Known interaction (accepted, from spec risk note):** per-chunk aggregation means an artist similar to seeds spanning two chunks no longer sums scores across the whole sweep (compounded by `_upsert_artist`'s replace-not-accumulate, deferred item #1). Benign for a feed; revisit only if suggestions look jumpy. `min_score` likewise applies per chunk.
|
||
- **Ordering:** recommended order A1→A2→A3→A4→A5→B1→B2→B3→C1→C2→C3. A5 before C3 makes C3 a no-op verify.
|