From f99277e0bcd6dff79f1f67e0f933b8d2dfbd8597 Mon Sep 17 00:00:00 2001 From: "sweetman.eth" Date: Tue, 16 Jun 2026 11:40:47 -0500 Subject: [PATCH 1/8] feat(measurement-jobs): free-tier card gate (setup mode) + instant backfill drain (#671) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two chat#1796 refinements on the historical (Songstats) path: 1. Free-tier card-on-file link. The gate was issuing the paid subscription checkout ($99/mo after a 30-day trial). New createCardOnFileSession uses Stripe Checkout `mode: "setup"` — collects a card for $0, no subscription, no Stripe product. The account then pays only for metered usage via credits. 2. Instant drain. After enqueuing a historical job, fire-and-forget start(songstatsBackfillWorkflow) so the backfill begins immediately instead of waiting up to 24h for the cron. Safe by reuse: the workflow's budget gate (limit − reserve − rolling-30d ledger) caps it to the Songstats quota and SKIP LOCKED prevents double-claiming with the daily cron, which stays as the backstop. Only kicks when something was actually enqueued. 26 new/updated unit tests; research+stripe+workflows suite 453 green; tsc/lint/format clean. --- .../enqueueHistoricalBackfill.test.ts | 19 +++++++++ .../ensureSongstatsPaymentMethod.test.ts | 12 +++--- .../enqueueHistoricalBackfill.ts | 15 +++++++ .../ensureSongstatsPaymentMethod.ts | 7 +++- .../__tests__/createCardOnFileSession.test.ts | 39 +++++++++++++++++++ lib/stripe/createCardOnFileSession.ts | 31 +++++++++++++++ 6 files changed, 115 insertions(+), 8 deletions(-) create mode 100644 lib/stripe/__tests__/createCardOnFileSession.test.ts create mode 100644 lib/stripe/createCardOnFileSession.ts diff --git a/lib/research/measurement_jobs/__tests__/enqueueHistoricalBackfill.test.ts b/lib/research/measurement_jobs/__tests__/enqueueHistoricalBackfill.test.ts index 20cde74a7..03010fa41 100644 --- a/lib/research/measurement_jobs/__tests__/enqueueHistoricalBackfill.test.ts +++ b/lib/research/measurement_jobs/__tests__/enqueueHistoricalBackfill.test.ts @@ -3,6 +3,7 @@ import { enqueueHistoricalBackfill } from "../enqueueHistoricalBackfill"; import { resolveScopeSongs } from "../resolveScopeSongs"; import { selectSongMeasurements } from "@/lib/supabase/song_measurements/selectSongMeasurements"; import { upsertSongstatsBackfillQueue } from "@/lib/supabase/songstats_backfill_queue/upsertSongstatsBackfillQueue"; +import { start } from "workflow/api"; vi.mock("../resolveScopeSongs", () => ({ resolveScopeSongs: vi.fn() })); vi.mock("@/lib/supabase/song_measurements/selectSongMeasurements", () => ({ @@ -11,6 +12,10 @@ vi.mock("@/lib/supabase/song_measurements/selectSongMeasurements", () => ({ vi.mock("@/lib/supabase/songstats_backfill_queue/upsertSongstatsBackfillQueue", () => ({ upsertSongstatsBackfillQueue: vi.fn(), })); +vi.mock("workflow/api", () => ({ start: vi.fn() })); +vi.mock("@/app/workflows/songstatsBackfillWorkflow", () => ({ + songstatsBackfillWorkflow: vi.fn(), +})); describe("enqueueHistoricalBackfill", () => { beforeEach(() => { @@ -47,6 +52,8 @@ describe("enqueueHistoricalBackfill", () => { expect(r).toEqual({ data: { status: "success", source: "historical", id: null, enqueued: 2, skipped: 1 }, }); + // kick the drain immediately so the user doesn't wait for the daily cron + expect(start).toHaveBeenCalledTimes(1); }); it("ranks a song with no prior measurement at 0", async () => { @@ -57,4 +64,16 @@ describe("enqueueHistoricalBackfill", () => { expect(upsertSongstatsBackfillQueue).toHaveBeenCalledWith({ song: "I9", rank_score: 0 }); }); + + it("does NOT kick the drain when everything was already backfilled (nothing enqueued)", async () => { + vi.mocked(resolveScopeSongs).mockResolvedValue(["I1"]); + vi.mocked(selectSongMeasurements).mockResolvedValue([ + { song: "I1", value: 500, data_source: "songstats" }, + ] as never); + + const r = await enqueueHistoricalBackfill({ isrcs: ["I1"] }); + + expect((r as { data: { enqueued: number } }).data.enqueued).toBe(0); + expect(start).not.toHaveBeenCalled(); + }); }); diff --git a/lib/research/measurement_jobs/__tests__/ensureSongstatsPaymentMethod.test.ts b/lib/research/measurement_jobs/__tests__/ensureSongstatsPaymentMethod.test.ts index 1e2c1f072..d75629a53 100644 --- a/lib/research/measurement_jobs/__tests__/ensureSongstatsPaymentMethod.test.ts +++ b/lib/research/measurement_jobs/__tests__/ensureSongstatsPaymentMethod.test.ts @@ -2,7 +2,7 @@ import { describe, it, expect, vi, beforeEach } from "vitest"; import { ensureSongstatsPaymentMethod } from "../ensureSongstatsPaymentMethod"; import { findStripeCustomerForAccount } from "@/lib/stripe/findStripeCustomerForAccount"; import { findDefaultPaymentMethodForCustomer } from "@/lib/stripe/findDefaultPaymentMethodForCustomer"; -import { createStripeSession } from "@/lib/stripe/createStripeSession"; +import { createCardOnFileSession } from "@/lib/stripe/createCardOnFileSession"; vi.mock("@/lib/networking/getCorsHeaders", () => ({ getCorsHeaders: vi.fn(() => ({})) })); vi.mock("@/lib/stripe/findStripeCustomerForAccount", () => ({ @@ -11,7 +11,7 @@ vi.mock("@/lib/stripe/findStripeCustomerForAccount", () => ({ vi.mock("@/lib/stripe/findDefaultPaymentMethodForCustomer", () => ({ findDefaultPaymentMethodForCustomer: vi.fn(), })); -vi.mock("@/lib/stripe/createStripeSession", () => ({ createStripeSession: vi.fn() })); +vi.mock("@/lib/stripe/createCardOnFileSession", () => ({ createCardOnFileSession: vi.fn() })); describe("ensureSongstatsPaymentMethod", () => { beforeEach(() => vi.clearAllMocks()); @@ -23,17 +23,17 @@ describe("ensureSongstatsPaymentMethod", () => { const r = await ensureSongstatsPaymentMethod("acc_1"); expect(r).toBeNull(); - expect(createStripeSession).not.toHaveBeenCalled(); + expect(createCardOnFileSession).not.toHaveBeenCalled(); }); it("402s with a free-tier checkout link when there is no Stripe customer", async () => { vi.mocked(findStripeCustomerForAccount).mockResolvedValue(null); - vi.mocked(createStripeSession).mockResolvedValue({ url: "https://checkout/free" } as never); + vi.mocked(createCardOnFileSession).mockResolvedValue({ url: "https://checkout/free" } as never); const r = await ensureSongstatsPaymentMethod("acc_1"); expect(findDefaultPaymentMethodForCustomer).not.toHaveBeenCalled(); - expect(createStripeSession).toHaveBeenCalledWith("acc_1", expect.any(String)); + expect(createCardOnFileSession).toHaveBeenCalledWith("acc_1", expect.any(String)); expect((r as Response).status).toBe(402); expect(await (r as Response).json()).toMatchObject({ status: "error", @@ -44,7 +44,7 @@ describe("ensureSongstatsPaymentMethod", () => { it("402s with a checkout link when the customer exists but has no card", async () => { vi.mocked(findStripeCustomerForAccount).mockResolvedValue("cus_1"); vi.mocked(findDefaultPaymentMethodForCustomer).mockResolvedValue(null); - vi.mocked(createStripeSession).mockResolvedValue({ url: "https://checkout/free" } as never); + vi.mocked(createCardOnFileSession).mockResolvedValue({ url: "https://checkout/free" } as never); const r = await ensureSongstatsPaymentMethod("acc_1"); diff --git a/lib/research/measurement_jobs/enqueueHistoricalBackfill.ts b/lib/research/measurement_jobs/enqueueHistoricalBackfill.ts index 3c0cc7af5..ffd6f3f20 100644 --- a/lib/research/measurement_jobs/enqueueHistoricalBackfill.ts +++ b/lib/research/measurement_jobs/enqueueHistoricalBackfill.ts @@ -1,6 +1,8 @@ +import { start } from "workflow/api"; import { resolveScopeSongs } from "./resolveScopeSongs"; import { selectSongMeasurements } from "@/lib/supabase/song_measurements/selectSongMeasurements"; import { upsertSongstatsBackfillQueue } from "@/lib/supabase/songstats_backfill_queue/upsertSongstatsBackfillQueue"; +import { songstatsBackfillWorkflow } from "@/app/workflows/songstatsBackfillWorkflow"; import type { CreateMeasurementJobBody } from "./validateCreateMeasurementJobRequest"; const METRIC = "platform_displayed_play_count"; @@ -61,5 +63,18 @@ export async function enqueueHistoricalBackfill( enqueued += batch.length; } + // Kick the drain now instead of waiting for the daily cron. The workflow's own + // budget gate (limit − reserve − rolling-30d ledger) means it only drains what + // the Songstats quota allows and then stops; SKIP LOCKED claims keep it from + // double-processing alongside the cron, which stays as the backstop. Fire-and- + // forget — a scheduling hiccup must not fail the (already-enqueued) job. + if (enqueued > 0) { + try { + await start(songstatsBackfillWorkflow); + } catch (error) { + console.error("[measurement-jobs] failed to kick backfill drain:", error); + } + } + return { data: { status: "success", source: "historical", id: null, enqueued, skipped } }; } diff --git a/lib/research/measurement_jobs/ensureSongstatsPaymentMethod.ts b/lib/research/measurement_jobs/ensureSongstatsPaymentMethod.ts index 71e3dd37c..8654dd19e 100644 --- a/lib/research/measurement_jobs/ensureSongstatsPaymentMethod.ts +++ b/lib/research/measurement_jobs/ensureSongstatsPaymentMethod.ts @@ -2,7 +2,7 @@ import { NextResponse } from "next/server"; import { getCorsHeaders } from "@/lib/networking/getCorsHeaders"; import { findStripeCustomerForAccount } from "@/lib/stripe/findStripeCustomerForAccount"; import { findDefaultPaymentMethodForCustomer } from "@/lib/stripe/findDefaultPaymentMethodForCustomer"; -import { createStripeSession } from "@/lib/stripe/createStripeSession"; +import { createCardOnFileSession } from "@/lib/stripe/createCardOnFileSession"; import { CREDIT_AUTO_RECHARGE_FALLBACK_SUCCESS_URL } from "@/lib/credits/const"; /** @@ -22,7 +22,10 @@ export async function ensureSongstatsPaymentMethod( const paymentMethod = customerId ? await findDefaultPaymentMethodForCustomer(customerId) : null; if (paymentMethod) return null; - const session = await createStripeSession(accountId, CREDIT_AUTO_RECHARGE_FALLBACK_SUCCESS_URL); + const session = await createCardOnFileSession( + accountId, + CREDIT_AUTO_RECHARGE_FALLBACK_SUCCESS_URL, + ); return NextResponse.json( { status: "error", diff --git a/lib/stripe/__tests__/createCardOnFileSession.test.ts b/lib/stripe/__tests__/createCardOnFileSession.test.ts new file mode 100644 index 000000000..14d1e0016 --- /dev/null +++ b/lib/stripe/__tests__/createCardOnFileSession.test.ts @@ -0,0 +1,39 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; + +const { checkoutSessionsCreate, resolveStripeCustomerForAccountMock } = vi.hoisted(() => ({ + checkoutSessionsCreate: vi.fn(), + resolveStripeCustomerForAccountMock: vi.fn(), +})); + +vi.mock("@/lib/stripe/client", () => ({ + default: { checkout: { sessions: { create: checkoutSessionsCreate } } }, +})); +vi.mock("@/lib/stripe/resolveStripeCustomerForAccount", () => ({ + resolveStripeCustomerForAccount: resolveStripeCustomerForAccountMock, +})); + +const { createCardOnFileSession } = await import("@/lib/stripe/createCardOnFileSession"); + +describe("createCardOnFileSession", () => { + beforeEach(() => { + vi.clearAllMocks(); + checkoutSessionsCreate.mockResolvedValue({ id: "cs_x", url: "https://checkout/setup" }); + resolveStripeCustomerForAccountMock.mockResolvedValue("cus_acc1"); + }); + + it("creates a setup-mode session (collect card only, no subscription/price)", async () => { + await createCardOnFileSession("acc-1", "https://example.com/success"); + + expect(resolveStripeCustomerForAccountMock).toHaveBeenCalledWith("acc-1"); + const params = checkoutSessionsCreate.mock.calls[0][0]; + expect(params).toMatchObject({ + customer: "cus_acc1", + mode: "setup", + client_reference_id: "acc-1", + success_url: "https://example.com/success", + }); + // free tier: no subscription, no line_items/price + expect(params.mode).not.toBe("subscription"); + expect(params.line_items).toBeUndefined(); + }); +}); diff --git a/lib/stripe/createCardOnFileSession.ts b/lib/stripe/createCardOnFileSession.ts new file mode 100644 index 000000000..f94959542 --- /dev/null +++ b/lib/stripe/createCardOnFileSession.ts @@ -0,0 +1,31 @@ +import type Stripe from "stripe"; +import stripeClient from "@/lib/stripe/client"; +import { resolveStripeCustomerForAccount } from "@/lib/stripe/resolveStripeCustomerForAccount"; + +/** + * A Stripe Checkout session that **only collects a card on file** — `mode: + * "setup"`, $0, no subscription or price. This is the "free tier": the account + * saves a payment method (so metered Songstats usage can be charged later via + * the credits system) without committing to any recurring plan. Needs no Stripe + * product. Contrast with {@link createStripeSession}, which is the paid + * subscription flow. + * + * @param accountId - The account to attach the card to. + * @param successUrl - Where Stripe redirects after the card is saved. + */ +export async function createCardOnFileSession( + accountId: string, + successUrl: string, +): Promise { + const metadata = { accountId }; + const customer = await resolveStripeCustomerForAccount(accountId); + + return stripeClient.checkout.sessions.create({ + customer, + mode: "setup", + currency: "usd", + client_reference_id: accountId, + metadata, + success_url: successUrl, + }); +} From 5fa5e3ac7c580ba52c5c53db7a9bdc2f7216035b Mon Sep 17 00:00:00 2001 From: "sweetman.eth" Date: Tue, 16 Jun 2026 16:41:22 -0500 Subject: [PATCH 2/8] fix(songstats-backfill): backoff on 429 + defer instead of churn (chat#1797) (#673) Pacing/backoff + per-step logging for the Songstats backfill drain (chat#1797 bullets 1 & 3). Bounded exponential backoff (fetchSongstatsWithBackoff, 502/503/504/408/429), defer-to-pending past the bound with claimed-batch release, per-step + per-batch logging. --- .../__tests__/backfillTrackStep.test.ts | 92 +++++++++---------- .../songstatsBackfillWorkflow.test.ts | 61 ++++++++++++ app/workflows/backfillTrackStep.ts | 59 +++++++----- app/workflows/releaseClaimedRowsStep.ts | 15 +++ app/workflows/songstatsBackfillWorkflow.ts | 38 ++++++-- .../fetchSongstatsWithBackoff.test.ts | 91 ++++++++++++++++++ .../__tests__/isRetryableStatus.test.ts | 26 ++++++ lib/songstats/fetchSongstatsWithBackoff.ts | 61 ++++++++++++ lib/songstats/isRetryableStatus.ts | 14 +++ .../updateSongstatsBackfillQueue.test.ts | 32 +++++-- .../updateSongstatsBackfillQueue.ts | 15 ++- 11 files changed, 406 insertions(+), 98 deletions(-) create mode 100644 app/workflows/__tests__/songstatsBackfillWorkflow.test.ts create mode 100644 app/workflows/releaseClaimedRowsStep.ts create mode 100644 lib/songstats/__tests__/fetchSongstatsWithBackoff.test.ts create mode 100644 lib/songstats/__tests__/isRetryableStatus.test.ts create mode 100644 lib/songstats/fetchSongstatsWithBackoff.ts create mode 100644 lib/songstats/isRetryableStatus.ts diff --git a/app/workflows/__tests__/backfillTrackStep.test.ts b/app/workflows/__tests__/backfillTrackStep.test.ts index ebe773704..6c211ed61 100644 --- a/app/workflows/__tests__/backfillTrackStep.test.ts +++ b/app/workflows/__tests__/backfillTrackStep.test.ts @@ -1,12 +1,14 @@ import { describe, it, expect, vi, beforeEach } from "vitest"; import { backfillTrackStep } from "../backfillTrackStep"; -import { fetchSongstats } from "@/lib/songstats/fetchSongstats"; +import { fetchSongstatsWithBackoff } from "@/lib/songstats/fetchSongstatsWithBackoff"; import { upsertSongMeasurements } from "@/lib/supabase/song_measurements/upsertSongMeasurements"; import { insertSongstatsQuotaLedger } from "@/lib/supabase/songstats_quota_ledger/insertSongstatsQuotaLedger"; import { updateSongstatsBackfillQueue } from "@/lib/supabase/songstats_backfill_queue/updateSongstatsBackfillQueue"; -vi.mock("@/lib/songstats/fetchSongstats", () => ({ fetchSongstats: vi.fn() })); +vi.mock("@/lib/songstats/fetchSongstatsWithBackoff", () => ({ + fetchSongstatsWithBackoff: vi.fn(), +})); vi.mock("@/lib/supabase/song_measurements/upsertSongMeasurements", () => ({ upsertSongMeasurements: vi.fn(), })); @@ -22,22 +24,20 @@ const ROW = { id: "q1", song: "USA2P2015959" } as never; describe("backfillTrackStep", () => { beforeEach(() => { vi.clearAllMocks(); + vi.spyOn(console, "log").mockImplementation(() => {}); vi.mocked(upsertSongMeasurements).mockResolvedValue([] as never); }); - it("writes the historic series as songstats measurements, records spend, marks done", async () => { - vi.mocked(fetchSongstats).mockResolvedValue({ + it("writes the historic series, records the spend, marks done on 200", async () => { + vi.mocked(fetchSongstatsWithBackoff).mockResolvedValue({ status: 200, + attempts: 1, + retriesExhausted: false, data: { stats: [ { source: "spotify", - data: { - history: [ - { date: "2025-01-01", streams_total: 1008736324 }, - { date: "2026-01-01", streams_total: 1330251464 }, - ], - }, + data: { history: [{ date: "2025-01-01", streams_total: 1008736324 }] }, }, ], }, @@ -45,10 +45,11 @@ describe("backfillTrackStep", () => { const result = await backfillTrackStep(ROW); - expect(fetchSongstats).toHaveBeenCalledWith("tracks/historic_stats", { + expect(fetchSongstatsWithBackoff).toHaveBeenCalledWith("tracks/historic_stats", { isrc: "USA2P2015959", source: "spotify", }); + // exact transformation: each history point → a permanent songstats measurement expect(upsertSongMeasurements).toHaveBeenCalledWith([ { song: "USA2P2015959", @@ -59,74 +60,65 @@ describe("backfillTrackStep", () => { data_source: "songstats", raw_ref: "songstats-backfill", }, - { - song: "USA2P2015959", - platform: "spotify", - metric: "platform_displayed_play_count", - value: 1330251464, - captured_at: "2026-01-01T00:00:00.000Z", - data_source: "songstats", - raw_ref: "songstats-backfill", - }, ]); expect(insertSongstatsQuotaLedger).toHaveBeenCalledWith({ hits: 1, purpose: "backfill USA2P2015959", }); - expect(updateSongstatsBackfillQueue).toHaveBeenCalledWith("q1", { status: "done" }); + expect(updateSongstatsBackfillQueue).toHaveBeenCalledWith(["q1"], { status: "done" }); expect(result).toEqual({ ok: true, hitsSpent: 1 }); }); - it("marks a transient upstream error (429) as failed (reclaimable) and records the spend", async () => { - vi.mocked(fetchSongstats).mockResolvedValue({ status: 429, data: {} }); - - const result = await backfillTrackStep(ROW); - - // transient -> 'failed' so the daily reclaim sweep returns it to 'pending' - expect(updateSongstatsBackfillQueue).toHaveBeenCalledWith("q1", { status: "failed" }); - expect(insertSongstatsQuotaLedger).toHaveBeenCalledWith({ - hits: 1, - purpose: "backfill USA2P2015959 (failed 429)", + it("DEFERS (pending, no quota hit, signals stop) when backoff is exhausted on 429", async () => { + vi.mocked(fetchSongstatsWithBackoff).mockResolvedValue({ + status: 429, + attempts: 6, + retriesExhausted: true, + data: {}, }); - expect(upsertSongMeasurements).not.toHaveBeenCalled(); - expect(result).toEqual({ ok: false, hitsSpent: 1 }); - }); - - it("marks a transient 5xx as failed (reclaimable)", async () => { - vi.mocked(fetchSongstats).mockResolvedValue({ status: 504, data: {} }); const result = await backfillTrackStep(ROW); - expect(updateSongstatsBackfillQueue).toHaveBeenCalledWith("q1", { status: "failed" }); - expect(result).toEqual({ ok: false, hitsSpent: 1 }); + // left pending for the next drain; NO ledger hit (Songstats consumed nothing) + expect(updateSongstatsBackfillQueue).toHaveBeenCalledWith(["q1"], { status: "pending" }); + expect(insertSongstatsQuotaLedger).not.toHaveBeenCalled(); + expect(upsertSongMeasurements).not.toHaveBeenCalled(); + expect(result).toEqual({ ok: false, hitsSpent: 0, deferred: true }); }); - it("marks a permanent client error (403) as done so reclaim never recycles it", async () => { - vi.mocked(fetchSongstats).mockResolvedValue({ status: 403, data: {} }); + it("marks a definitive 404 (no history) as done and records the spend", async () => { + vi.mocked(fetchSongstatsWithBackoff).mockResolvedValue({ + status: 404, + attempts: 1, + retriesExhausted: false, + data: {}, + }); const result = await backfillTrackStep(ROW); - // non-retryable 4xx (not 408/429) is terminal -> 'done', not 'failed' - expect(updateSongstatsBackfillQueue).toHaveBeenCalledWith("q1", { status: "done" }); + expect(updateSongstatsBackfillQueue).toHaveBeenCalledWith(["q1"], { status: "done" }); expect(insertSongstatsQuotaLedger).toHaveBeenCalledWith({ hits: 1, - purpose: "backfill USA2P2015959 (terminal 403)", + purpose: "backfill USA2P2015959 (no data 404)", }); expect(result).toEqual({ ok: false, hitsSpent: 1 }); }); - it("marks a definitive 404 (no history exists) as done so it is never retried", async () => { - vi.mocked(fetchSongstats).mockResolvedValue({ status: 404, data: {} }); + it("marks a permanent 4xx (403) as done (terminal) and records the spend", async () => { + vi.mocked(fetchSongstatsWithBackoff).mockResolvedValue({ + status: 403, + attempts: 1, + retriesExhausted: false, + data: {}, + }); const result = await backfillTrackStep(ROW); - // terminal no-data -> 'done', not 'failed' — the reclaim sweep must not resurrect it - expect(updateSongstatsBackfillQueue).toHaveBeenCalledWith("q1", { status: "done" }); + expect(updateSongstatsBackfillQueue).toHaveBeenCalledWith(["q1"], { status: "done" }); expect(insertSongstatsQuotaLedger).toHaveBeenCalledWith({ hits: 1, - purpose: "backfill USA2P2015959 (no data 404)", + purpose: "backfill USA2P2015959 (terminal 403)", }); - expect(upsertSongMeasurements).not.toHaveBeenCalled(); expect(result).toEqual({ ok: false, hitsSpent: 1 }); }); }); diff --git a/app/workflows/__tests__/songstatsBackfillWorkflow.test.ts b/app/workflows/__tests__/songstatsBackfillWorkflow.test.ts new file mode 100644 index 000000000..cc435558a --- /dev/null +++ b/app/workflows/__tests__/songstatsBackfillWorkflow.test.ts @@ -0,0 +1,61 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { songstatsBackfillWorkflow } from "../songstatsBackfillWorkflow"; + +import { getBackfillBudgetStep } from "../getBackfillBudgetStep"; +import { claimBackfillRowsStep } from "../claimBackfillRowsStep"; +import { backfillTrackStep } from "../backfillTrackStep"; +import { releaseClaimedRowsStep } from "../releaseClaimedRowsStep"; + +vi.mock("../getBackfillBudgetStep", () => ({ getBackfillBudgetStep: vi.fn() })); +vi.mock("../claimBackfillRowsStep", () => ({ claimBackfillRowsStep: vi.fn() })); +vi.mock("../backfillTrackStep", () => ({ backfillTrackStep: vi.fn() })); +vi.mock("../releaseClaimedRowsStep", () => ({ releaseClaimedRowsStep: vi.fn() })); + +const row = (id: string) => ({ id, song: id }) as never; + +describe("songstatsBackfillWorkflow", () => { + beforeEach(() => { + vi.clearAllMocks(); + vi.spyOn(console, "log").mockImplementation(() => {}); + vi.mocked(releaseClaimedRowsStep).mockResolvedValue(undefined); + }); + + it("releases the rest of the claimed batch to pending when a track defers", async () => { + vi.mocked(getBackfillBudgetStep).mockResolvedValue(100); + vi.mocked(claimBackfillRowsStep).mockResolvedValue([row("r1"), row("r2"), row("r3")]); + vi.mocked(backfillTrackStep) + .mockResolvedValueOnce({ ok: true, hitsSpent: 1 }) // r1 + .mockResolvedValueOnce({ ok: false, hitsSpent: 0, deferred: true }); // r2 defers + + const result = await songstatsBackfillWorkflow(); + + // r2 is set pending by the step itself; the unprocessed remainder (r3) is released here + expect(releaseClaimedRowsStep).toHaveBeenCalledWith(["r3"]); + expect(backfillTrackStep).toHaveBeenCalledTimes(2); // stopped at the defer, never reached r3 + expect(result).toEqual({ backfilled: 1, failed: 0, deferred: true }); + }); + + it("drains until the queue is empty and never releases when nothing defers", async () => { + vi.mocked(getBackfillBudgetStep).mockResolvedValue(100); + vi.mocked(claimBackfillRowsStep) + .mockResolvedValueOnce([row("a"), row("b")]) + .mockResolvedValueOnce([]); // queue drained + vi.mocked(backfillTrackStep) + .mockResolvedValueOnce({ ok: true, hitsSpent: 1 }) + .mockResolvedValueOnce({ ok: false, hitsSpent: 1 }); // terminal (e.g. 404) + + const result = await songstatsBackfillWorkflow(); + + expect(releaseClaimedRowsStep).not.toHaveBeenCalled(); + expect(result).toEqual({ backfilled: 1, failed: 1, deferred: false }); + }); + + it("does not drain when there is no budget", async () => { + vi.mocked(getBackfillBudgetStep).mockResolvedValue(0); + + const result = await songstatsBackfillWorkflow(); + + expect(claimBackfillRowsStep).not.toHaveBeenCalled(); + expect(result).toEqual({ backfilled: 0, failed: 0, deferred: false }); + }); +}); diff --git a/app/workflows/backfillTrackStep.ts b/app/workflows/backfillTrackStep.ts index 3247ff4cf..fd3947432 100644 --- a/app/workflows/backfillTrackStep.ts +++ b/app/workflows/backfillTrackStep.ts @@ -1,4 +1,4 @@ -import { fetchSongstats } from "@/lib/songstats/fetchSongstats"; +import { fetchSongstatsWithBackoff } from "@/lib/songstats/fetchSongstatsWithBackoff"; import { upsertSongMeasurements } from "@/lib/supabase/song_measurements/upsertSongMeasurements"; import { insertSongstatsQuotaLedger } from "@/lib/supabase/songstats_quota_ledger/insertSongstatsQuotaLedger"; import { updateSongstatsBackfillQueue } from "@/lib/supabase/songstats_backfill_queue/updateSongstatsBackfillQueue"; @@ -8,40 +8,48 @@ import { Tables } from "@/types/database.types"; const METRIC = "platform_displayed_play_count"; /** - * Backfill one claimed queue row: fetch the track's Songstats historic series - * (one quota hit — recorded win or lose), write each point as a permanent - * `songstats`-labeled measurement, and close the row. Failures mark the row - * failed without failing the run — the next row may still succeed. + * Backfill one claimed queue row, with bounded exponential backoff on Songstats' + * rate limit (Songstats is the rate authority — see chat#1797): + * - **200** → write each history point as a permanent `songstats` measurement, + * record the spend, mark `done`. + * - **404 / other 4xx** → a real request with a definitive answer; terminal, so + * mark `done` (404 = no history) and record the spend. + * - **backoff exhausted** (still 429/5xx after retries) → **defer**: leave the row + * `pending` for the next drain, consume no quota, and signal the workflow to + * stop (`deferred`) — Songstats is saturated right now. * * @param row - The claimed queue row (already in_progress) - * @returns ok + hits spent (always 1; the hit is consumed even on failure) + * @returns ok + hitsSpent (0 when deferred) + `deferred` when Songstats is saturated */ export async function backfillTrackStep( row: Tables<"songstats_backfill_queue">, -): Promise<{ ok: boolean; hitsSpent: number }> { +): Promise<{ ok: boolean; hitsSpent: number; deferred?: boolean }> { "use step"; - const result = await fetchSongstats("tracks/historic_stats", { + const result = await fetchSongstatsWithBackoff("tracks/historic_stats", { isrc: row.song, source: "spotify", }); - if (result.status !== 200) { - const status = result.status; - const isNoData = status === 404; - // Only transient errors are retryable: 408 (timeout), 429 (quota), any 5xx. - const isRetryable = status === 408 || status === 429 || status >= 500; - - // `failed` is reclaimable (the daily sweep returns it to `pending`, bounded - // by the rolling-window budget). 404 no-data and other permanent 4xx are - // terminal → `done`, so reclaim never recycles a track that can't succeed. - const nextStatus = isRetryable ? "failed" : "done"; - - let outcome = `terminal ${status}`; - if (isNoData) outcome = "no data 404"; - else if (isRetryable) outcome = `failed ${status}`; + if (result.retriesExhausted) { + // Still retryable (429 throttle / 408 / gateway 5xx) past the backoff bound — + // leave it for the next run, spend nothing. + console.log( + `[backfill] ${row.song} deferred (retryable ${result.status} after ${result.attempts} tries)`, + ); + await updateSongstatsBackfillQueue([row.id], { status: "pending" }); + return { ok: false, hitsSpent: 0, deferred: true }; + } - await insertSongstatsQuotaLedger({ hits: 1, purpose: `backfill ${row.song} (${outcome})` }); - await updateSongstatsBackfillQueue(row.id, { status: nextStatus }); + if (result.status !== 200) { + const noData = result.status === 404; + console.log( + `[backfill] ${row.song} done (${noData ? "no data 404" : `terminal ${result.status}`})`, + ); + await insertSongstatsQuotaLedger({ + hits: 1, + purpose: `backfill ${row.song} (${noData ? "no data 404" : `terminal ${result.status}`})`, + }); + await updateSongstatsBackfillQueue([row.id], { status: "done" }); return { ok: false, hitsSpent: 1 }; } @@ -67,7 +75,8 @@ export async function backfillTrackStep( }); await upsertSongMeasurements(rows); + console.log(`[backfill] ${row.song} done (${rows.length} points written)`); await insertSongstatsQuotaLedger({ hits: 1, purpose: `backfill ${row.song}` }); - await updateSongstatsBackfillQueue(row.id, { status: "done" }); + await updateSongstatsBackfillQueue([row.id], { status: "done" }); return { ok: true, hitsSpent: 1 }; } diff --git a/app/workflows/releaseClaimedRowsStep.ts b/app/workflows/releaseClaimedRowsStep.ts new file mode 100644 index 000000000..788d752a0 --- /dev/null +++ b/app/workflows/releaseClaimedRowsStep.ts @@ -0,0 +1,15 @@ +import { updateSongstatsBackfillQueue } from "@/lib/supabase/songstats_backfill_queue/updateSongstatsBackfillQueue"; + +/** + * Durable step: return unprocessed claimed rows to `pending` when the drain + * stops early on a defer, so the next run retries them immediately instead of + * waiting on the stale-reclaim sweep. + * + * @param ids - Queue row ids still `in_progress` from the aborted batch. + */ +export async function releaseClaimedRowsStep(ids: string[]): Promise { + "use step"; + if (ids.length === 0) return; + await updateSongstatsBackfillQueue(ids, { status: "pending" }); + console.log(`[songstats-backfill] released ${ids.length} claimed rows back to pending`); +} diff --git a/app/workflows/songstatsBackfillWorkflow.ts b/app/workflows/songstatsBackfillWorkflow.ts index 0a1960bf9..f94c0223d 100644 --- a/app/workflows/songstatsBackfillWorkflow.ts +++ b/app/workflows/songstatsBackfillWorkflow.ts @@ -1,15 +1,20 @@ import { getBackfillBudgetStep } from "@/app/workflows/getBackfillBudgetStep"; import { claimBackfillRowsStep } from "@/app/workflows/claimBackfillRowsStep"; import { backfillTrackStep } from "@/app/workflows/backfillTrackStep"; +import { releaseClaimedRowsStep } from "@/app/workflows/releaseClaimedRowsStep"; const BATCH_SIZE = 25; /** - * Durable Songstats backfill drain (recoupable/chat#1791 write path): check - * the rolling-window budget, claim value-ranked rows via the SKIP LOCKED RPC, - * backfill each track's historic series into the measurement store, and stop - * when the queue or the budget is dry. Every quota hit converts into - * permanent owned data (fetch-once: captured history is never refetched). + * Durable Songstats backfill drain (recoupable/chat#1791 write path): claim + * value-ranked rows via the SKIP LOCKED RPC and backfill each track's historic + * series, with per-track exponential backoff handling Songstats' rate limit + * (chat#1797). **Stops as soon as a track defers** — Songstats still + * rate-limiting it past the backoff bound — releasing the rest of the claimed + * batch back to `pending` (so the next drain retries them immediately rather + * than waiting on stale-reclaim) instead of hammering a saturated API. Every + * successful hit converts into permanent owned data (fetch-once: captured + * history is never refetched). */ export async function songstatsBackfillWorkflow() { "use workflow"; @@ -17,13 +22,23 @@ export async function songstatsBackfillWorkflow() { let budget = await getBackfillBudgetStep(); let backfilled = 0; let failed = 0; + let deferred = false; - while (budget > 0) { + drain: while (budget > 0) { const rows = await claimBackfillRowsStep(Math.min(budget, BATCH_SIZE)); if (rows.length === 0) break; + console.log(`[songstats-backfill] claimed ${rows.length} rows`); - for (const row of rows) { - const result = await backfillTrackStep(row); + for (let i = 0; i < rows.length; i += 1) { + const result = await backfillTrackStep(rows[i]); + if (result.deferred) { + // Songstats is saturated — stop now. The deferred row is already back to + // `pending`; release the rest of this claimed batch too so they don't sit + // `in_progress` until stale-reclaim. + deferred = true; + await releaseClaimedRowsStep(rows.slice(i + 1).map(r => r.id)); + break drain; + } budget -= result.hitsSpent; if (result.ok) backfilled += 1; else failed += 1; @@ -31,6 +46,9 @@ export async function songstatsBackfillWorkflow() { } } - console.log(`[songstats-backfill] done: ${backfilled} backfilled, ${failed} failed`); - return { backfilled, failed }; + console.log( + `[songstats-backfill] done: ${backfilled} backfilled, ${failed} terminal` + + (deferred ? ", deferred (rate-limited)" : ""), + ); + return { backfilled, failed, deferred }; } diff --git a/lib/songstats/__tests__/fetchSongstatsWithBackoff.test.ts b/lib/songstats/__tests__/fetchSongstatsWithBackoff.test.ts new file mode 100644 index 000000000..21015ef14 --- /dev/null +++ b/lib/songstats/__tests__/fetchSongstatsWithBackoff.test.ts @@ -0,0 +1,91 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { fetchSongstatsWithBackoff } from "../fetchSongstatsWithBackoff"; +import { fetchSongstats } from "../fetchSongstats"; + +vi.mock("../fetchSongstats", () => ({ fetchSongstats: vi.fn() })); + +const noSleep = vi.fn().mockResolvedValue(undefined); + +describe("fetchSongstatsWithBackoff", () => { + beforeEach(() => vi.clearAllMocks()); + + it("returns immediately on 200 with no retries or sleeps", async () => { + vi.mocked(fetchSongstats).mockResolvedValue({ status: 200, data: { ok: true } }); + + const r = await fetchSongstatsWithBackoff( + "tracks/historic_stats", + { isrc: "I" }, + { sleep: noSleep }, + ); + + expect(fetchSongstats).toHaveBeenCalledTimes(1); + expect(noSleep).not.toHaveBeenCalled(); + expect(r).toMatchObject({ status: 200, attempts: 1, retriesExhausted: false }); + }); + + it("does NOT retry a non-retryable status (404)", async () => { + vi.mocked(fetchSongstats).mockResolvedValue({ status: 404, data: {} }); + + const r = await fetchSongstatsWithBackoff("p", undefined, { sleep: noSleep }); + + expect(fetchSongstats).toHaveBeenCalledTimes(1); + expect(r).toMatchObject({ status: 404, retriesExhausted: false }); + }); + + it("backs off and retries on 429, succeeding on a later attempt", async () => { + vi.mocked(fetchSongstats) + .mockResolvedValueOnce({ status: 429, data: {} }) + .mockResolvedValueOnce({ status: 429, data: {} }) + .mockResolvedValueOnce({ status: 200, data: { ok: true } }); + + const r = await fetchSongstatsWithBackoff("p", undefined, { sleep: noSleep, baseMs: 100 }); + + expect(fetchSongstats).toHaveBeenCalledTimes(3); + expect(noSleep).toHaveBeenCalledTimes(2); + // exponential: 100 then 200 + expect(noSleep).toHaveBeenNthCalledWith(1, 100); + expect(noSleep).toHaveBeenNthCalledWith(2, 200); + expect(r).toMatchObject({ status: 200, attempts: 3, retriesExhausted: false }); + }); + + it("gives up after maxRetries on persistent 429 and flags retriesExhausted", async () => { + vi.mocked(fetchSongstats).mockResolvedValue({ status: 429, data: {} }); + + const r = await fetchSongstatsWithBackoff("p", undefined, { sleep: noSleep, maxRetries: 3 }); + + expect(fetchSongstats).toHaveBeenCalledTimes(4); // 1 initial + 3 retries + expect(noSleep).toHaveBeenCalledTimes(3); + expect(r).toMatchObject({ status: 429, retriesExhausted: true }); + }); + + it("treats transient gateway 5xx (503) and 408 as retryable", async () => { + vi.mocked(fetchSongstats) + .mockResolvedValueOnce({ status: 503, data: {} }) + .mockResolvedValueOnce({ status: 200, data: {} }); + const r = await fetchSongstatsWithBackoff("p", undefined, { sleep: noSleep, maxRetries: 2 }); + expect(fetchSongstats).toHaveBeenCalledTimes(2); + expect(r).toMatchObject({ status: 200, retriesExhausted: false }); + }); + + it("does NOT retry a 500 (fetchSongstats maps missing key / fetch failure to 500)", async () => { + vi.mocked(fetchSongstats).mockResolvedValue({ status: 500, data: {} }); + + const r = await fetchSongstatsWithBackoff("p", undefined, { sleep: noSleep }); + + expect(fetchSongstats).toHaveBeenCalledTimes(1); + expect(noSleep).not.toHaveBeenCalled(); + expect(r).toMatchObject({ status: 500, retriesExhausted: false }); + }); + + it("caps the backoff at maxMs", async () => { + vi.mocked(fetchSongstats).mockResolvedValue({ status: 429, data: {} }); + await fetchSongstatsWithBackoff("p", undefined, { + sleep: noSleep, + baseMs: 1000, + maxMs: 1500, + maxRetries: 3, + }); + // 1000, 2000->capped 1500, 4000->capped 1500 + expect(noSleep.mock.calls.map(c => c[0])).toEqual([1000, 1500, 1500]); + }); +}); diff --git a/lib/songstats/__tests__/isRetryableStatus.test.ts b/lib/songstats/__tests__/isRetryableStatus.test.ts new file mode 100644 index 000000000..a1b707f09 --- /dev/null +++ b/lib/songstats/__tests__/isRetryableStatus.test.ts @@ -0,0 +1,26 @@ +import { describe, it, expect } from "vitest"; +import { isRetryableStatus } from "../isRetryableStatus"; + +describe("isRetryableStatus", () => { + it("retries transient throttling/timeouts: 408, 429", () => { + expect(isRetryableStatus(408)).toBe(true); + expect(isRetryableStatus(429)).toBe(true); + }); + + it("retries transient gateway 5xx: 502, 503, 504", () => { + expect(isRetryableStatus(502)).toBe(true); + expect(isRetryableStatus(503)).toBe(true); + expect(isRetryableStatus(504)).toBe(true); + }); + + it("does NOT retry 500/501 (fetchSongstats maps missing key + fetch failures to 500)", () => { + expect(isRetryableStatus(500)).toBe(false); + expect(isRetryableStatus(501)).toBe(false); + }); + + it("does NOT retry definitive responses: 200, 404, 403", () => { + expect(isRetryableStatus(200)).toBe(false); + expect(isRetryableStatus(404)).toBe(false); + expect(isRetryableStatus(403)).toBe(false); + }); +}); diff --git a/lib/songstats/fetchSongstatsWithBackoff.ts b/lib/songstats/fetchSongstatsWithBackoff.ts new file mode 100644 index 000000000..8b3a4b097 --- /dev/null +++ b/lib/songstats/fetchSongstatsWithBackoff.ts @@ -0,0 +1,61 @@ +import { fetchSongstats } from "@/lib/songstats/fetchSongstats"; +import { isRetryableStatus } from "@/lib/songstats/isRetryableStatus"; +import { delay } from "@/lib/time/delay"; +import type { ProxyResult } from "@/lib/research/ProxyResult"; + +// Short, in-step backoff for Songstats' per-second rate limit. The default +// budget is 1+2+4+8 = 15s of waits (base 1s, doubling, capped at 8s, 4 retries), +// well within a workflow step's duration. Persistent rejection defers the row to +// the next drain run rather than sleeping for minutes inside one invocation. +const DEFAULT_MAX_RETRIES = 4; +const DEFAULT_BASE_MS = 1000; +const DEFAULT_MAX_MS = 8_000; + +export type FetchSongstatsBackoffOptions = { + maxRetries?: number; + baseMs?: number; + maxMs?: number; + /** Injectable for tests; defaults to a real timer. */ + sleep?: (ms: number) => Promise; +}; + +export type BackoffResult = ProxyResult & { + /** Total attempts made (1 + retries). */ + attempts: number; + /** True when the final result is still retryable after exhausting retries. */ + retriesExhausted: boolean; +}; + +/** + * Call Songstats with bounded exponential backoff on transient rejections + * (429 rate limit, 408, 5xx). Retries the **same** request up to `maxRetries` + * with `min(maxMs, baseMs * 2^attempt)` waits; a 200 or any non-retryable status + * (e.g. 404) returns immediately. When backoff is exhausted and the call is + * still being rejected, `retriesExhausted` is true so the caller can defer the + * work (leave it `pending`) rather than burn it — Songstats is the rate + * authority, not a local quota ledger. + * + * @param path - Songstats enterprise path (e.g. `tracks/historic_stats`). + * @param queryParams - Query params forwarded to Songstats. + * @param options - Backoff tuning + an injectable `sleep`. + */ +export async function fetchSongstatsWithBackoff( + path: string, + queryParams?: Record, + options: FetchSongstatsBackoffOptions = {}, +): Promise { + const maxRetries = options.maxRetries ?? DEFAULT_MAX_RETRIES; + const baseMs = options.baseMs ?? DEFAULT_BASE_MS; + const maxMs = options.maxMs ?? DEFAULT_MAX_MS; + const sleep = options.sleep ?? delay; + + let result = await fetchSongstats(path, queryParams); + let retries = 0; + while (isRetryableStatus(result.status) && retries < maxRetries) { + await sleep(Math.min(maxMs, baseMs * 2 ** retries)); + result = await fetchSongstats(path, queryParams); + retries += 1; + } + + return { ...result, attempts: retries + 1, retriesExhausted: isRetryableStatus(result.status) }; +} diff --git a/lib/songstats/isRetryableStatus.ts b/lib/songstats/isRetryableStatus.ts new file mode 100644 index 000000000..eb0004ee3 --- /dev/null +++ b/lib/songstats/isRetryableStatus.ts @@ -0,0 +1,14 @@ +/** + * Whether a Songstats response status is worth retrying with backoff. + * + * Transient: 408 (request timeout), 429 (rate limit), and the gateway 5xx + * 502/503/504 (bad gateway / unavailable / gateway timeout). Deliberately + * **excludes** 500/501 — `fetchSongstats` maps a missing API key and any + * non-HTTP fetch failure to 500, which are permanent for that request, so + * retrying them just burns the backoff budget before the row defers. + * + * @param status - HTTP status from `fetchSongstats`. + */ +export function isRetryableStatus(status: number): boolean { + return status === 408 || status === 429 || status === 502 || status === 503 || status === 504; +} diff --git a/lib/supabase/songstats_backfill_queue/__tests__/updateSongstatsBackfillQueue.test.ts b/lib/supabase/songstats_backfill_queue/__tests__/updateSongstatsBackfillQueue.test.ts index eb8d02300..73be73472 100644 --- a/lib/supabase/songstats_backfill_queue/__tests__/updateSongstatsBackfillQueue.test.ts +++ b/lib/supabase/songstats_backfill_queue/__tests__/updateSongstatsBackfillQueue.test.ts @@ -14,24 +14,40 @@ vi.mock("../../serverClient", () => { describe("updateSongstatsBackfillQueue", () => { beforeEach(() => vi.clearAllMocks()); - it("updates a queue row's status by id", async () => { - const eq = vi.fn().mockResolvedValue({ error: null }); - const update = vi.fn().mockReturnValue({ eq }); + it("updates a single row by id (one-element array)", async () => { + const inFn = vi.fn().mockResolvedValue({ error: null }); + const update = vi.fn().mockReturnValue({ in: inFn }); vi.mocked(supabase.from).mockReturnValue({ update } as never); - await updateSongstatsBackfillQueue("q1", { status: "done" }); + await updateSongstatsBackfillQueue(["q1"], { status: "done" }); expect(supabase.from).toHaveBeenCalledWith("songstats_backfill_queue"); expect(update).toHaveBeenCalledWith({ status: "done" }); - expect(eq).toHaveBeenCalledWith("id", "q1"); + expect(inFn).toHaveBeenCalledWith("id", ["q1"]); + }); + + it("bulk-updates many rows in one call (e.g. releasing a claimed batch)", async () => { + const inFn = vi.fn().mockResolvedValue({ error: null }); + const update = vi.fn().mockReturnValue({ in: inFn }); + vi.mocked(supabase.from).mockReturnValue({ update } as never); + + await updateSongstatsBackfillQueue(["q1", "q2"], { status: "pending" }); + + expect(update).toHaveBeenCalledWith({ status: "pending" }); + expect(inFn).toHaveBeenCalledWith("id", ["q1", "q2"]); + }); + + it("is a no-op on an empty id list (no DB call)", async () => { + await updateSongstatsBackfillQueue([], { status: "pending" }); + expect(supabase.from).not.toHaveBeenCalled(); }); it("throws on update error", async () => { - const eq = vi.fn().mockResolvedValue({ error: { message: "boom" } }); - const update = vi.fn().mockReturnValue({ eq }); + const inFn = vi.fn().mockResolvedValue({ error: { message: "boom" } }); + const update = vi.fn().mockReturnValue({ in: inFn }); vi.mocked(supabase.from).mockReturnValue({ update } as never); - await expect(updateSongstatsBackfillQueue("q1", { status: "failed" })).rejects.toThrow( + await expect(updateSongstatsBackfillQueue(["q1"], { status: "failed" })).rejects.toThrow( "Failed to update songstats backfill queue: boom", ); }); diff --git a/lib/supabase/songstats_backfill_queue/updateSongstatsBackfillQueue.ts b/lib/supabase/songstats_backfill_queue/updateSongstatsBackfillQueue.ts index b462381ab..d2469412d 100644 --- a/lib/supabase/songstats_backfill_queue/updateSongstatsBackfillQueue.ts +++ b/lib/supabase/songstats_backfill_queue/updateSongstatsBackfillQueue.ts @@ -2,17 +2,22 @@ import supabase from "../serverClient"; import { TablesUpdate } from "@/types/database.types"; /** - * Update a backfill queue row (mark done/failed after a claim). + * Update one or more backfill queue rows by id in a single round trip. Handles + * both the per-row status flip after a claim (`[row.id]`) and the bulk release + * of a claimed batch back to `pending` when the drain stops early (`.in` works + * for one id or many). No-op on an empty id list. * - * @param id - The queue row id - * @param fields - Fields to update + * @param ids - Queue row ids to update + * @param fields - Fields to set (e.g. `{ status: "done" }`) * @throws Error if the update fails */ export async function updateSongstatsBackfillQueue( - id: string, + ids: string[], fields: TablesUpdate<"songstats_backfill_queue">, ): Promise { - const { error } = await supabase.from("songstats_backfill_queue").update(fields).eq("id", id); + if (ids.length === 0) return; + + const { error } = await supabase.from("songstats_backfill_queue").update(fields).in("id", ids); if (error) { throw new Error(`Failed to update songstats backfill queue: ${error.message}`); From 5f459463996e1139a40743eaf5fd83c54f4ed04a Mon Sep 17 00:00:00 2001 From: "sweetman.eth" Date: Tue, 16 Jun 2026 17:14:31 -0500 Subject: [PATCH 3/8] refactor(songstats): remove local quota ledger + budget gate (chat#1797) (#674) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bullet 2 of chat#1797 (code half). Songstats is the rate authority — removes getBackfillBudgetStep, the budget gate, and insertSongstatsQuotaLedger/selectSongstatsQuotaSpent. The drain now claims+processes regardless of the ledger (un-stalls the backfill); the songstats_quota_ledger table is dropped in recoupable/database#35 (apply AFTER this deploys). --- .../__tests__/backfillTrackStep.test.ts | 34 +++++------------- .../__tests__/getBackfillBudgetStep.test.ts | 32 ----------------- .../songstatsBackfillWorkflow.test.ts | 24 ++++++------- app/workflows/backfillTrackStep.ts | 29 +++++++-------- app/workflows/getBackfillBudgetStep.ts | 21 ----------- app/workflows/songstatsBackfillWorkflow.ts | 29 +++++++-------- .../insertSongstatsQuotaLedger.test.ts | 32 ----------------- .../selectSongstatsQuotaSpent.test.ts | 35 ------------------- .../insertSongstatsQuotaLedger.ts | 19 ---------- .../selectSongstatsQuotaSpent.ts | 23 ------------ 10 files changed, 42 insertions(+), 236 deletions(-) delete mode 100644 app/workflows/__tests__/getBackfillBudgetStep.test.ts delete mode 100644 app/workflows/getBackfillBudgetStep.ts delete mode 100644 lib/supabase/songstats_quota_ledger/__tests__/insertSongstatsQuotaLedger.test.ts delete mode 100644 lib/supabase/songstats_quota_ledger/__tests__/selectSongstatsQuotaSpent.test.ts delete mode 100644 lib/supabase/songstats_quota_ledger/insertSongstatsQuotaLedger.ts delete mode 100644 lib/supabase/songstats_quota_ledger/selectSongstatsQuotaSpent.ts diff --git a/app/workflows/__tests__/backfillTrackStep.test.ts b/app/workflows/__tests__/backfillTrackStep.test.ts index 6c211ed61..c923cb030 100644 --- a/app/workflows/__tests__/backfillTrackStep.test.ts +++ b/app/workflows/__tests__/backfillTrackStep.test.ts @@ -3,7 +3,6 @@ import { backfillTrackStep } from "../backfillTrackStep"; import { fetchSongstatsWithBackoff } from "@/lib/songstats/fetchSongstatsWithBackoff"; import { upsertSongMeasurements } from "@/lib/supabase/song_measurements/upsertSongMeasurements"; -import { insertSongstatsQuotaLedger } from "@/lib/supabase/songstats_quota_ledger/insertSongstatsQuotaLedger"; import { updateSongstatsBackfillQueue } from "@/lib/supabase/songstats_backfill_queue/updateSongstatsBackfillQueue"; vi.mock("@/lib/songstats/fetchSongstatsWithBackoff", () => ({ @@ -12,9 +11,6 @@ vi.mock("@/lib/songstats/fetchSongstatsWithBackoff", () => ({ vi.mock("@/lib/supabase/song_measurements/upsertSongMeasurements", () => ({ upsertSongMeasurements: vi.fn(), })); -vi.mock("@/lib/supabase/songstats_quota_ledger/insertSongstatsQuotaLedger", () => ({ - insertSongstatsQuotaLedger: vi.fn(), -})); vi.mock("@/lib/supabase/songstats_backfill_queue/updateSongstatsBackfillQueue", () => ({ updateSongstatsBackfillQueue: vi.fn(), })); @@ -28,7 +24,7 @@ describe("backfillTrackStep", () => { vi.mocked(upsertSongMeasurements).mockResolvedValue([] as never); }); - it("writes the historic series, records the spend, marks done on 200", async () => { + it("writes the historic series and marks done on 200", async () => { vi.mocked(fetchSongstatsWithBackoff).mockResolvedValue({ status: 200, attempts: 1, @@ -61,15 +57,11 @@ describe("backfillTrackStep", () => { raw_ref: "songstats-backfill", }, ]); - expect(insertSongstatsQuotaLedger).toHaveBeenCalledWith({ - hits: 1, - purpose: "backfill USA2P2015959", - }); expect(updateSongstatsBackfillQueue).toHaveBeenCalledWith(["q1"], { status: "done" }); - expect(result).toEqual({ ok: true, hitsSpent: 1 }); + expect(result).toEqual({ ok: true }); }); - it("DEFERS (pending, no quota hit, signals stop) when backoff is exhausted on 429", async () => { + it("DEFERS (pending, signals stop) when backoff is exhausted on 429", async () => { vi.mocked(fetchSongstatsWithBackoff).mockResolvedValue({ status: 429, attempts: 6, @@ -79,14 +71,12 @@ describe("backfillTrackStep", () => { const result = await backfillTrackStep(ROW); - // left pending for the next drain; NO ledger hit (Songstats consumed nothing) expect(updateSongstatsBackfillQueue).toHaveBeenCalledWith(["q1"], { status: "pending" }); - expect(insertSongstatsQuotaLedger).not.toHaveBeenCalled(); expect(upsertSongMeasurements).not.toHaveBeenCalled(); - expect(result).toEqual({ ok: false, hitsSpent: 0, deferred: true }); + expect(result).toEqual({ ok: false, deferred: true }); }); - it("marks a definitive 404 (no history) as done and records the spend", async () => { + it("marks a definitive 404 (no history) as done", async () => { vi.mocked(fetchSongstatsWithBackoff).mockResolvedValue({ status: 404, attempts: 1, @@ -97,14 +87,10 @@ describe("backfillTrackStep", () => { const result = await backfillTrackStep(ROW); expect(updateSongstatsBackfillQueue).toHaveBeenCalledWith(["q1"], { status: "done" }); - expect(insertSongstatsQuotaLedger).toHaveBeenCalledWith({ - hits: 1, - purpose: "backfill USA2P2015959 (no data 404)", - }); - expect(result).toEqual({ ok: false, hitsSpent: 1 }); + expect(result).toEqual({ ok: false }); }); - it("marks a permanent 4xx (403) as done (terminal) and records the spend", async () => { + it("marks a permanent 4xx (403) as done (terminal)", async () => { vi.mocked(fetchSongstatsWithBackoff).mockResolvedValue({ status: 403, attempts: 1, @@ -115,10 +101,6 @@ describe("backfillTrackStep", () => { const result = await backfillTrackStep(ROW); expect(updateSongstatsBackfillQueue).toHaveBeenCalledWith(["q1"], { status: "done" }); - expect(insertSongstatsQuotaLedger).toHaveBeenCalledWith({ - hits: 1, - purpose: "backfill USA2P2015959 (terminal 403)", - }); - expect(result).toEqual({ ok: false, hitsSpent: 1 }); + expect(result).toEqual({ ok: false }); }); }); diff --git a/app/workflows/__tests__/getBackfillBudgetStep.test.ts b/app/workflows/__tests__/getBackfillBudgetStep.test.ts deleted file mode 100644 index 6c3fb8159..000000000 --- a/app/workflows/__tests__/getBackfillBudgetStep.test.ts +++ /dev/null @@ -1,32 +0,0 @@ -import { describe, it, expect, vi, beforeEach } from "vitest"; -import { getBackfillBudgetStep } from "../getBackfillBudgetStep"; - -import { selectSongstatsQuotaSpent } from "@/lib/supabase/songstats_quota_ledger/selectSongstatsQuotaSpent"; - -vi.mock("@/lib/supabase/songstats_quota_ledger/selectSongstatsQuotaSpent", () => ({ - selectSongstatsQuotaSpent: vi.fn(), -})); - -describe("getBackfillBudgetStep", () => { - beforeEach(() => { - vi.clearAllMocks(); - delete process.env.SONGSTATS_QUOTA_LIMIT; - delete process.env.SONGSTATS_QUOTA_RESERVE; - }); - - it("computes limit - reserve - spent over the rolling 30d window", async () => { - vi.mocked(selectSongstatsQuotaSpent).mockResolvedValue(300); - - const budget = await getBackfillBudgetStep(); - - expect(budget).toBe(1000 - 100 - 300); - const since = vi.mocked(selectSongstatsQuotaSpent).mock.calls[0][0]; - const ageDays = (Date.now() - new Date(since).getTime()) / 86400000; - expect(Math.round(ageDays)).toBe(30); - }); - - it("never returns negative", async () => { - vi.mocked(selectSongstatsQuotaSpent).mockResolvedValue(5000); - expect(await getBackfillBudgetStep()).toBe(0); - }); -}); diff --git a/app/workflows/__tests__/songstatsBackfillWorkflow.test.ts b/app/workflows/__tests__/songstatsBackfillWorkflow.test.ts index cc435558a..b18c5aeb7 100644 --- a/app/workflows/__tests__/songstatsBackfillWorkflow.test.ts +++ b/app/workflows/__tests__/songstatsBackfillWorkflow.test.ts @@ -1,12 +1,10 @@ import { describe, it, expect, vi, beforeEach } from "vitest"; import { songstatsBackfillWorkflow } from "../songstatsBackfillWorkflow"; -import { getBackfillBudgetStep } from "../getBackfillBudgetStep"; import { claimBackfillRowsStep } from "../claimBackfillRowsStep"; import { backfillTrackStep } from "../backfillTrackStep"; import { releaseClaimedRowsStep } from "../releaseClaimedRowsStep"; -vi.mock("../getBackfillBudgetStep", () => ({ getBackfillBudgetStep: vi.fn() })); vi.mock("../claimBackfillRowsStep", () => ({ claimBackfillRowsStep: vi.fn() })); vi.mock("../backfillTrackStep", () => ({ backfillTrackStep: vi.fn() })); vi.mock("../releaseClaimedRowsStep", () => ({ releaseClaimedRowsStep: vi.fn() })); @@ -21,41 +19,39 @@ describe("songstatsBackfillWorkflow", () => { }); it("releases the rest of the claimed batch to pending when a track defers", async () => { - vi.mocked(getBackfillBudgetStep).mockResolvedValue(100); vi.mocked(claimBackfillRowsStep).mockResolvedValue([row("r1"), row("r2"), row("r3")]); vi.mocked(backfillTrackStep) - .mockResolvedValueOnce({ ok: true, hitsSpent: 1 }) // r1 - .mockResolvedValueOnce({ ok: false, hitsSpent: 0, deferred: true }); // r2 defers + .mockResolvedValueOnce({ ok: true }) // r1 + .mockResolvedValueOnce({ ok: false, deferred: true }); // r2 defers const result = await songstatsBackfillWorkflow(); // r2 is set pending by the step itself; the unprocessed remainder (r3) is released here expect(releaseClaimedRowsStep).toHaveBeenCalledWith(["r3"]); expect(backfillTrackStep).toHaveBeenCalledTimes(2); // stopped at the defer, never reached r3 - expect(result).toEqual({ backfilled: 1, failed: 0, deferred: true }); + expect(result).toEqual({ backfilled: 1, terminal: 0, deferred: true }); }); it("drains until the queue is empty and never releases when nothing defers", async () => { - vi.mocked(getBackfillBudgetStep).mockResolvedValue(100); vi.mocked(claimBackfillRowsStep) .mockResolvedValueOnce([row("a"), row("b")]) .mockResolvedValueOnce([]); // queue drained vi.mocked(backfillTrackStep) - .mockResolvedValueOnce({ ok: true, hitsSpent: 1 }) - .mockResolvedValueOnce({ ok: false, hitsSpent: 1 }); // terminal (e.g. 404) + .mockResolvedValueOnce({ ok: true }) + .mockResolvedValueOnce({ ok: false }); // terminal (e.g. 404) const result = await songstatsBackfillWorkflow(); expect(releaseClaimedRowsStep).not.toHaveBeenCalled(); - expect(result).toEqual({ backfilled: 1, failed: 1, deferred: false }); + expect(result).toEqual({ backfilled: 1, terminal: 1, deferred: false }); }); - it("does not drain when there is no budget", async () => { - vi.mocked(getBackfillBudgetStep).mockResolvedValue(0); + it("stops immediately when the first claim is empty", async () => { + vi.mocked(claimBackfillRowsStep).mockResolvedValue([]); const result = await songstatsBackfillWorkflow(); - expect(claimBackfillRowsStep).not.toHaveBeenCalled(); - expect(result).toEqual({ backfilled: 0, failed: 0, deferred: false }); + expect(backfillTrackStep).not.toHaveBeenCalled(); + expect(result).toEqual({ backfilled: 0, terminal: 0, deferred: false }); }); }); diff --git a/app/workflows/backfillTrackStep.ts b/app/workflows/backfillTrackStep.ts index fd3947432..4a627ee88 100644 --- a/app/workflows/backfillTrackStep.ts +++ b/app/workflows/backfillTrackStep.ts @@ -1,6 +1,5 @@ import { fetchSongstatsWithBackoff } from "@/lib/songstats/fetchSongstatsWithBackoff"; import { upsertSongMeasurements } from "@/lib/supabase/song_measurements/upsertSongMeasurements"; -import { insertSongstatsQuotaLedger } from "@/lib/supabase/songstats_quota_ledger/insertSongstatsQuotaLedger"; import { updateSongstatsBackfillQueue } from "@/lib/supabase/songstats_backfill_queue/updateSongstatsBackfillQueue"; import { historicStatsPayloadSchema } from "@/lib/research/playcounts/historicStatsPayloadSchema"; import { Tables } from "@/types/database.types"; @@ -9,21 +8,22 @@ const METRIC = "platform_displayed_play_count"; /** * Backfill one claimed queue row, with bounded exponential backoff on Songstats' - * rate limit (Songstats is the rate authority — see chat#1797): + * rate limit (Songstats is the rate authority — see chat#1797; there is no local + * quota ledger): * - **200** → write each history point as a permanent `songstats` measurement, - * record the spend, mark `done`. + * mark `done`. * - **404 / other 4xx** → a real request with a definitive answer; terminal, so - * mark `done` (404 = no history) and record the spend. + * mark `done` (404 = no history). * - **backoff exhausted** (still 429/5xx after retries) → **defer**: leave the row - * `pending` for the next drain, consume no quota, and signal the workflow to - * stop (`deferred`) — Songstats is saturated right now. + * `pending` for the next drain and signal the workflow to stop (`deferred`) — + * Songstats is saturated right now. * * @param row - The claimed queue row (already in_progress) - * @returns ok + hitsSpent (0 when deferred) + `deferred` when Songstats is saturated + * @returns `ok` (true on a written backfill) + `deferred` when Songstats is saturated */ export async function backfillTrackStep( row: Tables<"songstats_backfill_queue">, -): Promise<{ ok: boolean; hitsSpent: number; deferred?: boolean }> { +): Promise<{ ok: boolean; deferred?: boolean }> { "use step"; const result = await fetchSongstatsWithBackoff("tracks/historic_stats", { isrc: row.song, @@ -32,12 +32,12 @@ export async function backfillTrackStep( if (result.retriesExhausted) { // Still retryable (429 throttle / 408 / gateway 5xx) past the backoff bound — - // leave it for the next run, spend nothing. + // leave it for the next run. console.log( `[backfill] ${row.song} deferred (retryable ${result.status} after ${result.attempts} tries)`, ); await updateSongstatsBackfillQueue([row.id], { status: "pending" }); - return { ok: false, hitsSpent: 0, deferred: true }; + return { ok: false, deferred: true }; } if (result.status !== 200) { @@ -45,12 +45,8 @@ export async function backfillTrackStep( console.log( `[backfill] ${row.song} done (${noData ? "no data 404" : `terminal ${result.status}`})`, ); - await insertSongstatsQuotaLedger({ - hits: 1, - purpose: `backfill ${row.song} (${noData ? "no data 404" : `terminal ${result.status}`})`, - }); await updateSongstatsBackfillQueue([row.id], { status: "done" }); - return { ok: false, hitsSpent: 1 }; + return { ok: false }; } const parsed = historicStatsPayloadSchema.safeParse(result.data); @@ -76,7 +72,6 @@ export async function backfillTrackStep( await upsertSongMeasurements(rows); console.log(`[backfill] ${row.song} done (${rows.length} points written)`); - await insertSongstatsQuotaLedger({ hits: 1, purpose: `backfill ${row.song}` }); await updateSongstatsBackfillQueue([row.id], { status: "done" }); - return { ok: true, hitsSpent: 1 }; + return { ok: true }; } diff --git a/app/workflows/getBackfillBudgetStep.ts b/app/workflows/getBackfillBudgetStep.ts deleted file mode 100644 index 866ee7605..000000000 --- a/app/workflows/getBackfillBudgetStep.ts +++ /dev/null @@ -1,21 +0,0 @@ -import { selectSongstatsQuotaSpent } from "@/lib/supabase/songstats_quota_ledger/selectSongstatsQuotaSpent"; - -const DEFAULT_QUOTA_LIMIT = 1000; -const DEFAULT_QUOTA_RESERVE = 100; -const WINDOW_DAYS = 30; - -/** - * Remaining Songstats budget for this run: plan limit minus a reserve (kept - * for request-path fallbacks and manual research) minus hits spent in the - * rolling 30-day window. Never negative. - * - * @returns Hits the backfill worker may spend now - */ -export async function getBackfillBudgetStep(): Promise { - "use step"; - const limit = Number(process.env.SONGSTATS_QUOTA_LIMIT) || DEFAULT_QUOTA_LIMIT; - const reserve = Number(process.env.SONGSTATS_QUOTA_RESERVE) || DEFAULT_QUOTA_RESERVE; - const since = new Date(Date.now() - WINDOW_DAYS * 24 * 60 * 60 * 1000).toISOString(); - const spent = await selectSongstatsQuotaSpent(since); - return Math.max(0, limit - reserve - spent); -} diff --git a/app/workflows/songstatsBackfillWorkflow.ts b/app/workflows/songstatsBackfillWorkflow.ts index f94c0223d..459d18870 100644 --- a/app/workflows/songstatsBackfillWorkflow.ts +++ b/app/workflows/songstatsBackfillWorkflow.ts @@ -1,4 +1,3 @@ -import { getBackfillBudgetStep } from "@/app/workflows/getBackfillBudgetStep"; import { claimBackfillRowsStep } from "@/app/workflows/claimBackfillRowsStep"; import { backfillTrackStep } from "@/app/workflows/backfillTrackStep"; import { releaseClaimedRowsStep } from "@/app/workflows/releaseClaimedRowsStep"; @@ -8,24 +7,22 @@ const BATCH_SIZE = 25; /** * Durable Songstats backfill drain (recoupable/chat#1791 write path): claim * value-ranked rows via the SKIP LOCKED RPC and backfill each track's historic - * series, with per-track exponential backoff handling Songstats' rate limit - * (chat#1797). **Stops as soon as a track defers** — Songstats still - * rate-limiting it past the backoff bound — releasing the rest of the claimed - * batch back to `pending` (so the next drain retries them immediately rather - * than waiting on stale-reclaim) instead of hammering a saturated API. Every - * successful hit converts into permanent owned data (fetch-once: captured - * history is never refetched). + * series. Per-track exponential backoff absorbs Songstats' rate limit; the run + * **stops as soon as a track defers** (still retryable past the backoff bound), + * releasing the rest of the claimed batch back to `pending` so the next drain + * retries them immediately rather than waiting on stale-reclaim. Otherwise it + * drains until the queue has no claimable `pending` rows. Every backfill + * converts into permanent owned data (fetch-once). */ export async function songstatsBackfillWorkflow() { "use workflow"; - let budget = await getBackfillBudgetStep(); let backfilled = 0; - let failed = 0; + let terminal = 0; let deferred = false; - drain: while (budget > 0) { - const rows = await claimBackfillRowsStep(Math.min(budget, BATCH_SIZE)); + drain: while (true) { + const rows = await claimBackfillRowsStep(BATCH_SIZE); if (rows.length === 0) break; console.log(`[songstats-backfill] claimed ${rows.length} rows`); @@ -39,16 +36,14 @@ export async function songstatsBackfillWorkflow() { await releaseClaimedRowsStep(rows.slice(i + 1).map(r => r.id)); break drain; } - budget -= result.hitsSpent; if (result.ok) backfilled += 1; - else failed += 1; - if (budget <= 0) break; + else terminal += 1; } } console.log( - `[songstats-backfill] done: ${backfilled} backfilled, ${failed} terminal` + + `[songstats-backfill] done: ${backfilled} backfilled, ${terminal} terminal` + (deferred ? ", deferred (rate-limited)" : ""), ); - return { backfilled, failed, deferred }; + return { backfilled, terminal, deferred }; } diff --git a/lib/supabase/songstats_quota_ledger/__tests__/insertSongstatsQuotaLedger.test.ts b/lib/supabase/songstats_quota_ledger/__tests__/insertSongstatsQuotaLedger.test.ts deleted file mode 100644 index 4f7d5c2a8..000000000 --- a/lib/supabase/songstats_quota_ledger/__tests__/insertSongstatsQuotaLedger.test.ts +++ /dev/null @@ -1,32 +0,0 @@ -import { describe, it, expect, vi, beforeEach } from "vitest"; -import { insertSongstatsQuotaLedger } from "../insertSongstatsQuotaLedger"; -import supabase from "../../serverClient"; - -vi.mock("../../serverClient", () => { - const mockFrom = vi.fn(); - const mockRpc = vi.fn(); - return { default: { from: mockFrom, rpc: mockRpc } }; -}); - -describe("insertSongstatsQuotaLedger", () => { - beforeEach(() => vi.clearAllMocks()); - - it("records spent hits", async () => { - const insert = vi.fn().mockResolvedValue({ error: null }); - vi.mocked(supabase.from).mockReturnValue({ insert } as never); - - await insertSongstatsQuotaLedger({ hits: 1, purpose: "backfill USA2P2015959" }); - - expect(supabase.from).toHaveBeenCalledWith("songstats_quota_ledger"); - expect(insert).toHaveBeenCalledWith([{ hits: 1, purpose: "backfill USA2P2015959" }]); - }); - - it("throws on insert error (spend must be recorded)", async () => { - const insert = vi.fn().mockResolvedValue({ error: { message: "boom" } }); - vi.mocked(supabase.from).mockReturnValue({ insert } as never); - - await expect(insertSongstatsQuotaLedger({ hits: 1 })).rejects.toThrow( - "Failed to record songstats quota spend: boom", - ); - }); -}); diff --git a/lib/supabase/songstats_quota_ledger/__tests__/selectSongstatsQuotaSpent.test.ts b/lib/supabase/songstats_quota_ledger/__tests__/selectSongstatsQuotaSpent.test.ts deleted file mode 100644 index 6d59c6267..000000000 --- a/lib/supabase/songstats_quota_ledger/__tests__/selectSongstatsQuotaSpent.test.ts +++ /dev/null @@ -1,35 +0,0 @@ -import { describe, it, expect, vi, beforeEach } from "vitest"; -import { selectSongstatsQuotaSpent } from "../selectSongstatsQuotaSpent"; -import supabase from "../../serverClient"; - -vi.mock("../../serverClient", () => { - const mockFrom = vi.fn(); - const mockRpc = vi.fn(); - return { default: { from: mockFrom, rpc: mockRpc } }; -}); - -describe("selectSongstatsQuotaSpent", () => { - beforeEach(() => vi.clearAllMocks()); - - it("sums hits spent since the window start", async () => { - const gte = vi.fn().mockResolvedValue({ data: [{ hits: 3 }, { hits: 7 }], error: null }); - const select = vi.fn().mockReturnValue({ gte }); - vi.mocked(supabase.from).mockReturnValue({ select } as never); - - const result = await selectSongstatsQuotaSpent("2026-05-12T00:00:00Z"); - - expect(supabase.from).toHaveBeenCalledWith("songstats_quota_ledger"); - expect(gte).toHaveBeenCalledWith("spent_at", "2026-05-12T00:00:00Z"); - expect(result).toBe(10); - }); - - it("treats errors as full quota spent (fail safe — do not overspend)", async () => { - const consoleError = vi.spyOn(console, "error").mockImplementation(() => {}); - const gte = vi.fn().mockResolvedValue({ data: null, error: { message: "boom" } }); - const select = vi.fn().mockReturnValue({ gte }); - vi.mocked(supabase.from).mockReturnValue({ select } as never); - - expect(await selectSongstatsQuotaSpent("2026-05-12T00:00:00Z")).toBe(Number.MAX_SAFE_INTEGER); - consoleError.mockRestore(); - }); -}); diff --git a/lib/supabase/songstats_quota_ledger/insertSongstatsQuotaLedger.ts b/lib/supabase/songstats_quota_ledger/insertSongstatsQuotaLedger.ts deleted file mode 100644 index 777c42a79..000000000 --- a/lib/supabase/songstats_quota_ledger/insertSongstatsQuotaLedger.ts +++ /dev/null @@ -1,19 +0,0 @@ -import supabase from "../serverClient"; -import { TablesInsert } from "@/types/database.types"; - -/** - * Record Songstats quota spend. Throws on failure — unrecorded spend would - * let the worker silently blow the rolling-window budget. - * - * @param entry - hits spent, optional purpose/account attribution - * @throws Error if the insert fails - */ -export async function insertSongstatsQuotaLedger( - entry: TablesInsert<"songstats_quota_ledger">, -): Promise { - const { error } = await supabase.from("songstats_quota_ledger").insert([entry]); - - if (error) { - throw new Error(`Failed to record songstats quota spend: ${error.message}`); - } -} diff --git a/lib/supabase/songstats_quota_ledger/selectSongstatsQuotaSpent.ts b/lib/supabase/songstats_quota_ledger/selectSongstatsQuotaSpent.ts deleted file mode 100644 index 0610a4b27..000000000 --- a/lib/supabase/songstats_quota_ledger/selectSongstatsQuotaSpent.ts +++ /dev/null @@ -1,23 +0,0 @@ -import supabase from "../serverClient"; - -/** - * Sum Songstats hits spent since a window start (the rolling 30-day quota - * check). Errors are treated as the full quota being spent — failing safe: - * the worker must never overspend because the ledger was unreadable. - * - * @param since - Inclusive ISO lower bound of the window - * @returns Total hits spent in the window - */ -export async function selectSongstatsQuotaSpent(since: string): Promise { - const { data, error } = await supabase - .from("songstats_quota_ledger") - .select("hits") - .gte("spent_at", since); - - if (error) { - console.error("Error reading songstats quota ledger:", error); - return Number.MAX_SAFE_INTEGER; - } - - return (data || []).reduce((sum, row) => sum + row.hits, 0); -} From f24d4c7ecaf4469c234378916dafffddc1425416 Mon Sep 17 00:00:00 2001 From: "sweetman.eth" Date: Thu, 18 Jun 2026 15:06:39 -0500 Subject: [PATCH 4/8] feat: POST /api/catalogs (create + materialize from valuation snapshot) (#677) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat: POST /api/catalogs create + materialize from valuation snapshot Creates a catalog owned by the authenticated account (account derived from credentials via validateAuthContext, never the body). With from.snapshot_id, materializes the catalog from a completed valuation snapshot: creates the catalogs row, links account_catalogs, adds the snapshot's measured ISRCs as catalog_songs, and records the catalog on the snapshot. Re-claiming the same snapshot is idempotent. TDD: validateCreateCatalogBody (6 tests) + createCatalogHandler (8 tests), red->green. New supabase wrappers: insertCatalog, selectCatalogById, insertAccountCatalog, updateSnapshotCatalog. Implements recoupable/chat#1801 Phase 2. Matches docs contract recoupable/docs#243. Co-Authored-By: Claude Opus 4.8 (1M context) * refactor: re-anchor POST /api/catalogs to merged contract + review fixes - Flatten request to the merged docs#243 contract: from:{snapshot_id} -> a root snapshot field (validator + handler + tests). Error copy follows. - DRY/SRP: drop the inline success() helper; use the shared successResponse(). - KISS rename: materializeSnapshotCatalog.ts -> createSnapshotCatalog.ts. - DRY: delete the redundant updateSnapshotCatalog helper; reuse the existing updatePlaycountSnapshot(id, fields). Validator change done red->green. lib/catalog: 24 tests pass; tsc + eslint clean. Addresses review on PR #677. * fix: materialize catalog songs from song_measurements, not snapshot.isrcs Testing the full materialize path surfaced a real bug: a valuation snapshot is album_ids-scoped, so its own isrcs column is null — createSnapshotCatalog read snapshot.isrcs and would link an EMPTY catalog. The measured ISRCs live in song_measurements (snapshot lineage), so source them there. New selectSnapshotIsrcs(snapshotId) helper (distinct song_measurements.song for the snapshot). createSnapshotCatalog now uses it. TDD: new createSnapshotCatalog.test.ts (3 tests) red->green; lib/catalog 27 pass. Addresses PR #677 verification. * refactor: reuse selectSongMeasurements (snapshot filter) instead of a new helper KISS/DRY per review: drop selectSnapshotIsrcs; add an optional snapshot filter to the existing selectSongMeasurements, and derive distinct ISRCs in createSnapshotCatalog. lib/catalog + song_measurements: 36 tests pass. Addresses review on PR #677. --------- Co-authored-by: Claude Opus 4.8 (1M context) --- app/api/catalogs/route.ts | 26 +++ .../__tests__/createCatalogHandler.test.ts | 160 ++++++++++++++++++ .../__tests__/createSnapshotCatalog.test.ts | 92 ++++++++++ .../validateCreateCatalogBody.test.ts | 50 ++++++ lib/catalog/createCatalogHandler.ts | 74 ++++++++ lib/catalog/createSnapshotCatalog.ts | 45 +++++ lib/catalog/validateCreateCatalogBody.ts | 47 +++++ .../account_catalogs/insertAccountCatalog.ts | 21 +++ lib/supabase/catalogs/insertCatalog.ts | 19 +++ lib/supabase/catalogs/selectCatalogById.ts | 19 +++ .../selectSongMeasurements.ts | 14 +- 11 files changed, 562 insertions(+), 5 deletions(-) create mode 100644 app/api/catalogs/route.ts create mode 100644 lib/catalog/__tests__/createCatalogHandler.test.ts create mode 100644 lib/catalog/__tests__/createSnapshotCatalog.test.ts create mode 100644 lib/catalog/__tests__/validateCreateCatalogBody.test.ts create mode 100644 lib/catalog/createCatalogHandler.ts create mode 100644 lib/catalog/createSnapshotCatalog.ts create mode 100644 lib/catalog/validateCreateCatalogBody.ts create mode 100644 lib/supabase/account_catalogs/insertAccountCatalog.ts create mode 100644 lib/supabase/catalogs/insertCatalog.ts create mode 100644 lib/supabase/catalogs/selectCatalogById.ts diff --git a/app/api/catalogs/route.ts b/app/api/catalogs/route.ts new file mode 100644 index 000000000..5ab6c6692 --- /dev/null +++ b/app/api/catalogs/route.ts @@ -0,0 +1,26 @@ +import { NextRequest, NextResponse } from "next/server"; +import { getCorsHeaders } from "@/lib/networking/getCorsHeaders"; +import { createCatalogHandler } from "@/lib/catalog/createCatalogHandler"; + +/** + * OPTIONS handler for CORS preflight requests. + * + * @returns A NextResponse with CORS headers. + */ +export async function OPTIONS() { + return new NextResponse(null, { + status: 200, + headers: getCorsHeaders(), + }); +} + +/** + * POST handler for creating a catalog (optionally materialized from a + * valuation snapshot). + * + * @param request - The request object containing the catalog body. + * @returns A NextResponse with the created catalog. + */ +export async function POST(request: NextRequest) { + return createCatalogHandler(request); +} diff --git a/lib/catalog/__tests__/createCatalogHandler.test.ts b/lib/catalog/__tests__/createCatalogHandler.test.ts new file mode 100644 index 000000000..5e2b1ebf0 --- /dev/null +++ b/lib/catalog/__tests__/createCatalogHandler.test.ts @@ -0,0 +1,160 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { NextRequest, NextResponse } from "next/server"; + +import { createCatalogHandler } from "../createCatalogHandler"; +import { validateCreateCatalogBody } from "../validateCreateCatalogBody"; +import { createSnapshotCatalog } from "../createSnapshotCatalog"; +import { validateAuthContext } from "@/lib/auth/validateAuthContext"; +import { selectPlaycountSnapshots } from "@/lib/supabase/playcount_snapshots/selectPlaycountSnapshots"; +import { selectCatalogById } from "@/lib/supabase/catalogs/selectCatalogById"; +import { insertCatalog } from "@/lib/supabase/catalogs/insertCatalog"; +import { insertAccountCatalog } from "@/lib/supabase/account_catalogs/insertAccountCatalog"; + +vi.mock("@/lib/networking/getCorsHeaders", () => ({ + getCorsHeaders: vi.fn(() => ({ "Access-Control-Allow-Origin": "*" })), +})); +vi.mock("../validateCreateCatalogBody", () => ({ validateCreateCatalogBody: vi.fn() })); +vi.mock("../createSnapshotCatalog", () => ({ createSnapshotCatalog: vi.fn() })); +vi.mock("@/lib/auth/validateAuthContext", () => ({ validateAuthContext: vi.fn() })); +vi.mock("@/lib/supabase/playcount_snapshots/selectPlaycountSnapshots", () => ({ + selectPlaycountSnapshots: vi.fn(), +})); +vi.mock("@/lib/supabase/catalogs/selectCatalogById", () => ({ selectCatalogById: vi.fn() })); +vi.mock("@/lib/supabase/catalogs/insertCatalog", () => ({ insertCatalog: vi.fn() })); +vi.mock("@/lib/supabase/account_catalogs/insertAccountCatalog", () => ({ + insertAccountCatalog: vi.fn(), +})); + +const accountId = "550e8400-e29b-41d4-a716-446655440000"; +const otherAccountId = "550e8400-e29b-41d4-a716-446655440999"; +const snapshotId = "11111111-2222-3333-4444-555555555555"; +const catalogId = "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee"; + +const catalog = { + id: catalogId, + name: "Bad Bunny — Catalog", + created_at: "2026-06-18T00:00:00Z", + updated_at: "2026-06-18T00:00:00Z", +}; + +const makeRequest = () => new NextRequest("http://localhost/api/catalogs", { method: "POST" }); + +const okAuth = () => + vi.mocked(validateAuthContext).mockResolvedValue({ accountId, orgId: null, authToken: "t" }); + +describe("createCatalogHandler", () => { + beforeEach(() => vi.clearAllMocks()); + + it("short-circuits with the validator error and never authenticates", async () => { + const err = NextResponse.json({ status: "error" }, { status: 400 }); + vi.mocked(validateCreateCatalogBody).mockReturnValue(err); + + const res = await createCatalogHandler(makeRequest()); + + expect(res).toBe(err); + expect(validateAuthContext).not.toHaveBeenCalled(); + }); + + it("short-circuits with the auth error when unauthenticated", async () => { + vi.mocked(validateCreateCatalogBody).mockReturnValue({ name: "X" }); + const authErr = NextResponse.json({ status: "error" }, { status: 401 }); + vi.mocked(validateAuthContext).mockResolvedValue(authErr); + + const res = await createCatalogHandler(makeRequest()); + + expect(res).toBe(authErr); + expect(insertCatalog).not.toHaveBeenCalled(); + }); + + it("creates an empty catalog from a name-only body and links the account", async () => { + vi.mocked(validateCreateCatalogBody).mockReturnValue({ name: "Bad Bunny — Catalog" }); + okAuth(); + vi.mocked(insertCatalog).mockResolvedValue(catalog); + + const res = await createCatalogHandler(makeRequest()); + const body = await res.json(); + + expect(res.status).toBe(200); + expect(insertCatalog).toHaveBeenCalledWith("Bad Bunny — Catalog"); + expect(insertAccountCatalog).toHaveBeenCalledWith({ account: accountId, catalog: catalogId }); + expect(body).toEqual({ status: "success", catalog, songs_added: 0 }); + expect(selectPlaycountSnapshots).not.toHaveBeenCalled(); + }); + + it("materializes a catalog from an owned, not-yet-claimed snapshot", async () => { + vi.mocked(validateCreateCatalogBody).mockReturnValue({ + name: "Bad Bunny — Catalog", + snapshot: snapshotId, + }); + okAuth(); + vi.mocked(selectPlaycountSnapshots).mockResolvedValue([ + { id: snapshotId, account: accountId, catalog: null, isrcs: ["A", "B"] } as never, + ]); + vi.mocked(createSnapshotCatalog).mockResolvedValue({ catalog, songsAdded: 2 }); + + const res = await createCatalogHandler(makeRequest()); + const body = await res.json(); + + expect(res.status).toBe(200); + expect(createSnapshotCatalog).toHaveBeenCalledWith({ + accountId, + snapshot: expect.objectContaining({ id: snapshotId }), + name: "Bad Bunny — Catalog", + }); + expect(body).toEqual({ status: "success", catalog, songs_added: 2 }); + }); + + it("returns 404 when the snapshot does not exist", async () => { + vi.mocked(validateCreateCatalogBody).mockReturnValue({ snapshot: snapshotId }); + okAuth(); + vi.mocked(selectPlaycountSnapshots).mockResolvedValue([]); + + const res = await createCatalogHandler(makeRequest()); + + expect(res.status).toBe(404); + expect(createSnapshotCatalog).not.toHaveBeenCalled(); + }); + + it("returns 403 when the snapshot belongs to a different account", async () => { + vi.mocked(validateCreateCatalogBody).mockReturnValue({ snapshot: snapshotId }); + okAuth(); + vi.mocked(selectPlaycountSnapshots).mockResolvedValue([ + { id: snapshotId, account: otherAccountId, catalog: null, isrcs: ["A"] } as never, + ]); + + const res = await createCatalogHandler(makeRequest()); + + expect(res.status).toBe(403); + expect(createSnapshotCatalog).not.toHaveBeenCalled(); + }); + + it("is idempotent: returns the existing catalog when the snapshot is already claimed", async () => { + vi.mocked(validateCreateCatalogBody).mockReturnValue({ snapshot: snapshotId }); + okAuth(); + vi.mocked(selectPlaycountSnapshots).mockResolvedValue([ + { id: snapshotId, account: accountId, catalog: catalogId, isrcs: ["A", "B"] } as never, + ]); + vi.mocked(selectCatalogById).mockResolvedValue(catalog); + + const res = await createCatalogHandler(makeRequest()); + const body = await res.json(); + + expect(res.status).toBe(200); + expect(selectCatalogById).toHaveBeenCalledWith(catalogId); + expect(createSnapshotCatalog).not.toHaveBeenCalled(); + expect(body).toEqual({ status: "success", catalog, songs_added: 0 }); + }); + + it("returns a generic 500 without leaking the underlying error", async () => { + vi.mocked(validateCreateCatalogBody).mockReturnValue({ name: "X" }); + okAuth(); + vi.mocked(insertCatalog).mockRejectedValue(new Error("db down at 10.0.0.1:5432")); + + const res = await createCatalogHandler(makeRequest()); + const body = await res.json(); + + expect(res.status).toBe(500); + expect(body.status).toBe("error"); + expect(JSON.stringify(body)).not.toContain("10.0.0.1"); + }); +}); diff --git a/lib/catalog/__tests__/createSnapshotCatalog.test.ts b/lib/catalog/__tests__/createSnapshotCatalog.test.ts new file mode 100644 index 000000000..9ee7a3ede --- /dev/null +++ b/lib/catalog/__tests__/createSnapshotCatalog.test.ts @@ -0,0 +1,92 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; + +import { createSnapshotCatalog } from "../createSnapshotCatalog"; +import { insertCatalog } from "@/lib/supabase/catalogs/insertCatalog"; +import { insertAccountCatalog } from "@/lib/supabase/account_catalogs/insertAccountCatalog"; +import { insertCatalogSongs } from "@/lib/supabase/catalog_songs/insertCatalogSongs"; +import { updatePlaycountSnapshot } from "@/lib/supabase/playcount_snapshots/updatePlaycountSnapshot"; +import { selectSongMeasurements } from "@/lib/supabase/song_measurements/selectSongMeasurements"; + +vi.mock("@/lib/supabase/catalogs/insertCatalog", () => ({ insertCatalog: vi.fn() })); +vi.mock("@/lib/supabase/account_catalogs/insertAccountCatalog", () => ({ + insertAccountCatalog: vi.fn(), +})); +vi.mock("@/lib/supabase/catalog_songs/insertCatalogSongs", () => ({ insertCatalogSongs: vi.fn() })); +vi.mock("@/lib/supabase/playcount_snapshots/updatePlaycountSnapshot", () => ({ + updatePlaycountSnapshot: vi.fn(), +})); +vi.mock("@/lib/supabase/song_measurements/selectSongMeasurements", () => ({ + selectSongMeasurements: vi.fn(), +})); + +const accountId = "550e8400-e29b-41d4-a716-446655440000"; +const snapshotId = "11111111-2222-3333-4444-555555555555"; +const catalogId = "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee"; +const catalog = { id: catalogId, name: "Bad Bunny Catalog", created_at: "t", updated_at: "t" }; +// A real valuation snapshot is scoped by album_ids, so its own `isrcs` column is null — +// the measured ISRCs live in song_measurements (sourced via selectSongMeasurements). +const snapshot = { id: snapshotId, account: accountId, catalog: null, isrcs: null } as never; +const measurement = (song: string) => ({ song }) as never; + +describe("createSnapshotCatalog", () => { + beforeEach(() => { + vi.clearAllMocks(); + vi.mocked(insertCatalog).mockResolvedValue(catalog); + }); + + it("sources measured ISRCs from song_measurements (by snapshot) and adds them as catalog songs", async () => { + vi.mocked(selectSongMeasurements).mockResolvedValue([ + measurement("ISRC_A"), + measurement("ISRC_B"), + measurement("ISRC_C"), + ]); + + const result = await createSnapshotCatalog({ accountId, snapshot, name: "Bad Bunny Catalog" }); + + expect(insertCatalog).toHaveBeenCalledWith("Bad Bunny Catalog"); + expect(insertAccountCatalog).toHaveBeenCalledWith({ account: accountId, catalog: catalogId }); + // ISRCs come from measurements by snapshot, NOT snapshot.isrcs (null here) + expect(selectSongMeasurements).toHaveBeenCalledWith({ snapshot: snapshotId }); + expect(insertCatalogSongs).toHaveBeenCalledWith([ + { catalog: catalogId, song: "ISRC_A" }, + { catalog: catalogId, song: "ISRC_B" }, + { catalog: catalogId, song: "ISRC_C" }, + ]); + expect(updatePlaycountSnapshot).toHaveBeenCalledWith(snapshotId, { catalog: catalogId }); + expect(result).toEqual({ catalog, songsAdded: 3 }); + }); + + it("dedupes ISRCs across multiple measurement rows per track", async () => { + vi.mocked(selectSongMeasurements).mockResolvedValue([ + measurement("ISRC_A"), + measurement("ISRC_A"), + measurement("ISRC_B"), + ]); + + const result = await createSnapshotCatalog({ accountId, snapshot }); + + expect(insertCatalogSongs).toHaveBeenCalledWith([ + { catalog: catalogId, song: "ISRC_A" }, + { catalog: catalogId, song: "ISRC_B" }, + ]); + expect(result).toEqual({ catalog, songsAdded: 2 }); + }); + + it("adds no songs when the snapshot has no measurements", async () => { + vi.mocked(selectSongMeasurements).mockResolvedValue([]); + + const result = await createSnapshotCatalog({ accountId, snapshot }); + + expect(insertCatalogSongs).not.toHaveBeenCalled(); + expect(updatePlaycountSnapshot).toHaveBeenCalledWith(snapshotId, { catalog: catalogId }); + expect(result).toEqual({ catalog, songsAdded: 0 }); + }); + + it("falls back to a default name when none is supplied", async () => { + vi.mocked(selectSongMeasurements).mockResolvedValue([]); + + await createSnapshotCatalog({ accountId, snapshot }); + + expect(insertCatalog).toHaveBeenCalledWith("Valuation Catalog"); + }); +}); diff --git a/lib/catalog/__tests__/validateCreateCatalogBody.test.ts b/lib/catalog/__tests__/validateCreateCatalogBody.test.ts new file mode 100644 index 000000000..670895f6a --- /dev/null +++ b/lib/catalog/__tests__/validateCreateCatalogBody.test.ts @@ -0,0 +1,50 @@ +import { describe, it, expect } from "vitest"; +import { NextResponse } from "next/server"; + +import { validateCreateCatalogBody } from "../validateCreateCatalogBody"; + +const snapshotId = "550e8400-e29b-41d4-a716-446655440000"; + +describe("validateCreateCatalogBody", () => { + it("accepts a name-only body", () => { + const result = validateCreateCatalogBody({ name: "My Catalog" }); + expect(result).toEqual({ name: "My Catalog" }); + }); + + it("accepts a snapshot-only body", () => { + const result = validateCreateCatalogBody({ snapshot: snapshotId }); + expect(result).toEqual({ snapshot: snapshotId }); + }); + + it("accepts name and snapshot together", () => { + const result = validateCreateCatalogBody({ + name: "Bad Bunny — Catalog", + snapshot: snapshotId, + }); + expect(result).toEqual({ + name: "Bad Bunny — Catalog", + snapshot: snapshotId, + }); + }); + + it("rejects an empty body (neither name nor snapshot) with 400", async () => { + const result = validateCreateCatalogBody({}); + expect(result).toBeInstanceOf(NextResponse); + const res = result as NextResponse; + expect(res.status).toBe(400); + const body = await res.json(); + expect(body.status).toBe("error"); + }); + + it("rejects a non-uuid snapshot with 400", async () => { + const result = validateCreateCatalogBody({ snapshot: "not-a-uuid" }); + expect(result).toBeInstanceOf(NextResponse); + expect((result as NextResponse).status).toBe(400); + }); + + it("rejects a null/non-object body with 400", async () => { + const result = validateCreateCatalogBody(null); + expect(result).toBeInstanceOf(NextResponse); + expect((result as NextResponse).status).toBe(400); + }); +}); diff --git a/lib/catalog/createCatalogHandler.ts b/lib/catalog/createCatalogHandler.ts new file mode 100644 index 000000000..583746971 --- /dev/null +++ b/lib/catalog/createCatalogHandler.ts @@ -0,0 +1,74 @@ +import { NextRequest, NextResponse } from "next/server"; +import { errorResponse } from "@/lib/networking/errorResponse"; +import { successResponse } from "@/lib/networking/successResponse"; +import { validateAuthContext } from "@/lib/auth/validateAuthContext"; +import { validateCreateCatalogBody } from "./validateCreateCatalogBody"; +import { createSnapshotCatalog } from "./createSnapshotCatalog"; +import { selectPlaycountSnapshots } from "@/lib/supabase/playcount_snapshots/selectPlaycountSnapshots"; +import { selectCatalogById } from "@/lib/supabase/catalogs/selectCatalogById"; +import { insertCatalog } from "@/lib/supabase/catalogs/insertCatalog"; +import { insertAccountCatalog } from "@/lib/supabase/account_catalogs/insertAccountCatalog"; + +const DEFAULT_CATALOG_NAME = "Valuation Catalog"; + +/** + * POST /api/catalogs + * + * Creates a catalog owned by the authenticated account. The owning account is + * resolved from credentials (Privy bearer or x-api-key), never from the body. + * + * With `snapshot`, materializes the catalog from a completed valuation snapshot: + * the snapshot must be owned by the caller, and re-claiming the same snapshot is + * idempotent (returns the catalog already created for that run). + * + * @param request - The request object + * @returns A NextResponse with `{ status, catalog, songs_added }` + */ +export async function createCatalogHandler(request: NextRequest): Promise { + try { + const body = await request.json().catch(() => null); + + const validated = validateCreateCatalogBody(body); + if (validated instanceof NextResponse) { + return validated; + } + + const authResult = await validateAuthContext(request); + if (authResult instanceof NextResponse) { + return authResult; + } + const { accountId } = authResult; + + if (!validated.snapshot) { + const catalog = await insertCatalog(validated.name ?? DEFAULT_CATALOG_NAME); + await insertAccountCatalog({ account: accountId, catalog: catalog.id }); + return successResponse({ catalog, songs_added: 0 }); + } + + const [snapshot] = await selectPlaycountSnapshots({ id: validated.snapshot }); + if (!snapshot) { + return errorResponse("Snapshot not found", 404); + } + if (snapshot.account !== accountId) { + return errorResponse("Snapshot belongs to a different account", 403); + } + + // Idempotent re-claim: the run already produced a catalog. + if (snapshot.catalog) { + const existing = await selectCatalogById(snapshot.catalog); + if (existing) { + return successResponse({ catalog: existing, songs_added: 0 }); + } + } + + const { catalog, songsAdded } = await createSnapshotCatalog({ + accountId, + snapshot, + name: validated.name, + }); + return successResponse({ catalog, songs_added: songsAdded }); + } catch (error) { + console.error("Error creating catalog:", error); + return errorResponse("Internal server error", 500); + } +} diff --git a/lib/catalog/createSnapshotCatalog.ts b/lib/catalog/createSnapshotCatalog.ts new file mode 100644 index 000000000..ab1ba96c2 --- /dev/null +++ b/lib/catalog/createSnapshotCatalog.ts @@ -0,0 +1,45 @@ +import { Tables } from "@/types/database.types"; +import { insertCatalog } from "@/lib/supabase/catalogs/insertCatalog"; +import { insertAccountCatalog } from "@/lib/supabase/account_catalogs/insertAccountCatalog"; +import { insertCatalogSongs } from "@/lib/supabase/catalog_songs/insertCatalogSongs"; +import { updatePlaycountSnapshot } from "@/lib/supabase/playcount_snapshots/updatePlaycountSnapshot"; +import { selectSongMeasurements } from "@/lib/supabase/song_measurements/selectSongMeasurements"; + +const DEFAULT_CATALOG_NAME = "Valuation Catalog"; + +/** + * Creates an account-linked catalog from a valuation snapshot: creates the + * `catalogs` row, links it to the account via `account_catalogs`, adds the + * snapshot's **measured** ISRCs (from `song_measurements`, not the snapshot's + * own `isrcs` column — that's null for album-scoped valuation runs) as + * `catalog_songs`, and records the new catalog on the snapshot (the idempotency + * key for re-claims). + * + * Callers must first confirm the snapshot is owned by `accountId` and not yet + * claimed (`snapshot.catalog` is null). + * + * @param params.accountId - Owning account (already authorized) + * @param params.snapshot - The owned, unclaimed snapshot row + * @param params.name - Optional catalog name; falls back to a default + * @returns The created catalog and the number of songs added + */ +export async function createSnapshotCatalog(params: { + accountId: string; + snapshot: Tables<"playcount_snapshots">; + name?: string; +}): Promise<{ catalog: Tables<"catalogs">; songsAdded: number }> { + const { accountId, snapshot, name } = params; + + const catalog = await insertCatalog(name ?? DEFAULT_CATALOG_NAME); + await insertAccountCatalog({ account: accountId, catalog: catalog.id }); + + const measurements = await selectSongMeasurements({ snapshot: snapshot.id }); + const isrcs = [...new Set(measurements.map(m => m.song))]; + if (isrcs.length > 0) { + await insertCatalogSongs(isrcs.map(isrc => ({ catalog: catalog.id, song: isrc }))); + } + + await updatePlaycountSnapshot(snapshot.id, { catalog: catalog.id }); + + return { catalog, songsAdded: isrcs.length }; +} diff --git a/lib/catalog/validateCreateCatalogBody.ts b/lib/catalog/validateCreateCatalogBody.ts new file mode 100644 index 000000000..b41c058c1 --- /dev/null +++ b/lib/catalog/validateCreateCatalogBody.ts @@ -0,0 +1,47 @@ +import { NextResponse } from "next/server"; +import { getCorsHeaders } from "@/lib/networking/getCorsHeaders"; +import { z } from "zod"; + +export const createCatalogBodySchema = z + .object({ + name: z.string().min(1, "name must not be empty").optional(), + snapshot: z.string().uuid("snapshot must be a valid UUID").optional(), + }) + .refine(data => data.name !== undefined || data.snapshot !== undefined, { + message: "Provide at least one of name or snapshot", + }); + +export type CreateCatalogBody = z.infer; + +/** + * Validates a create-catalog request body. + * + * Accepts `{ name?, snapshot? }`; at least one is required. `snapshot` is a + * completed playcount snapshot id (valuation run) to materialize from. The + * owning account is never taken from the body - it is resolved from the + * request credentials by the handler. + * + * @param body - The parsed request body to validate. + * @returns A NextResponse with a 400 error if validation fails, or the + * validated body if it passes. + */ +export function validateCreateCatalogBody(body: unknown): NextResponse | CreateCatalogBody { + const result = createCatalogBodySchema.safeParse(body); + + if (!result.success) { + const firstError = result.error.issues[0]; + return NextResponse.json( + { + status: "error", + missing_fields: firstError.path, + error: firstError.message, + }, + { + status: 400, + headers: getCorsHeaders(), + }, + ); + } + + return result.data; +} diff --git a/lib/supabase/account_catalogs/insertAccountCatalog.ts b/lib/supabase/account_catalogs/insertAccountCatalog.ts new file mode 100644 index 000000000..1facd5b63 --- /dev/null +++ b/lib/supabase/account_catalogs/insertAccountCatalog.ts @@ -0,0 +1,21 @@ +import supabase from "../serverClient"; + +/** + * Links a catalog to an account by inserting an `account_catalogs` row. + * + * @param params.account - Account id (owner) + * @param params.catalog - Catalog id to link + * @throws Error if the insert fails + */ +export async function insertAccountCatalog(params: { + account: string; + catalog: string; +}): Promise { + const { error } = await supabase + .from("account_catalogs") + .insert({ account: params.account, catalog: params.catalog }); + + if (error) { + throw new Error(`Failed to link account_catalogs: ${error.message}`); + } +} diff --git a/lib/supabase/catalogs/insertCatalog.ts b/lib/supabase/catalogs/insertCatalog.ts new file mode 100644 index 000000000..ecd0c667b --- /dev/null +++ b/lib/supabase/catalogs/insertCatalog.ts @@ -0,0 +1,19 @@ +import supabase from "../serverClient"; +import { Tables } from "@/types/database.types"; + +/** + * Inserts a new `catalogs` row and returns it. + * + * @param name - Display name for the catalog + * @returns The inserted catalog row + * @throws Error if the insert fails + */ +export async function insertCatalog(name: string): Promise> { + const { data, error } = await supabase.from("catalogs").insert({ name }).select().single(); + + if (error || !data) { + throw new Error(`Failed to insert catalog: ${error?.message ?? "no row returned"}`); + } + + return data; +} diff --git a/lib/supabase/catalogs/selectCatalogById.ts b/lib/supabase/catalogs/selectCatalogById.ts new file mode 100644 index 000000000..39f961e3d --- /dev/null +++ b/lib/supabase/catalogs/selectCatalogById.ts @@ -0,0 +1,19 @@ +import supabase from "../serverClient"; +import { Tables } from "@/types/database.types"; + +/** + * Selects a single `catalogs` row by id, or null if none exists. + * + * @param id - Catalog id + * @returns The catalog row, or null if not found + * @throws Error if the query fails + */ +export async function selectCatalogById(id: string): Promise | null> { + const { data, error } = await supabase.from("catalogs").select("*").eq("id", id).maybeSingle(); + + if (error) { + throw new Error(`Failed to fetch catalog: ${error.message}`); + } + + return data; +} diff --git a/lib/supabase/song_measurements/selectSongMeasurements.ts b/lib/supabase/song_measurements/selectSongMeasurements.ts index 24d50bd44..1e118d14b 100644 --- a/lib/supabase/song_measurements/selectSongMeasurements.ts +++ b/lib/supabase/song_measurements/selectSongMeasurements.ts @@ -2,13 +2,14 @@ import supabase from "../serverClient"; import { Tables } from "@/types/database.types"; /** - * Select measurements newest-first. Filter by one song or a batch of songs - * (one is required), and optionally by platform/metric. Uses the - * (song, platform, metric, captured_at DESC) series index; pass `limit: 1` - * for the latest capture, omit for the full series. + * Select measurements newest-first. Filter by one song, a batch of songs, or a + * snapshot (one of the three is required), and optionally by platform/metric. + * Uses the (song, platform, metric, captured_at DESC) series index; pass + * `limit: 1` for the latest capture, omit for the full series. * * @param params.song - Single song ISRC filter * @param params.songs - Batch of song ISRCs (e.g. all tracks on an album) + * @param params.snapshot - Snapshot id filter (all measurements captured for a run) * @param params.platform - Optional platform filter (e.g. "spotify") * @param params.metric - Optional metric filter (e.g. "platform_displayed_play_count") * @param params.limit - Optional cap on returned rows @@ -17,17 +18,19 @@ import { Tables } from "@/types/database.types"; export async function selectSongMeasurements({ song, songs, + snapshot, platform, metric, limit, }: { song?: string; songs?: string[]; + snapshot?: string; platform?: string; metric?: string; limit?: number; }): Promise[]> { - if (!song && (!songs || songs.length === 0)) return []; + if (!song && (!songs || songs.length === 0) && !snapshot) return []; let query = supabase .from("song_measurements") @@ -36,6 +39,7 @@ export async function selectSongMeasurements({ if (song) query = query.eq("song", song); if (songs && songs.length > 0) query = query.in("song", songs); + if (snapshot) query = query.eq("snapshot", snapshot); if (platform) query = query.eq("platform", platform); if (metric) query = query.eq("metric", metric); if (limit) query = query.limit(limit); From a520a1ddac9b7dd4f3bf9d040c2270ecdb7c1dbe Mon Sep 17 00:00:00 2001 From: "sweetman.eth" Date: Thu, 18 Jun 2026 17:00:50 -0500 Subject: [PATCH 5/8] fix: LEFT-join artists in catalog-songs read (materialized tracks were hidden) (#681) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: LEFT-join artists in catalog-songs read so materialized tracks surface selectCatalogSongsWithArtists used song_artists!inner -> accounts!inner, so valuation-captured tracks (which have songs + song_measurements but no song_artists yet) were filtered out — a materialized catalog read back as 0 songs (verified live on api#677). Drop the two !inner so artist-less songs return with artists: []; songs!inner stays (catalog_songs.song FK guarantees it). Closes the read-path half of the song_artists follow-up in recoupable/chat#1801. Longer-term (option a): the capture pipeline should also write song_artists. * Update lib/supabase/catalog_songs/selectCatalogSongsWithArtists.ts --- lib/supabase/catalog_songs/selectCatalogSongsWithArtists.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/supabase/catalog_songs/selectCatalogSongsWithArtists.ts b/lib/supabase/catalog_songs/selectCatalogSongsWithArtists.ts index 8639eb405..47cc14206 100644 --- a/lib/supabase/catalog_songs/selectCatalogSongsWithArtists.ts +++ b/lib/supabase/catalog_songs/selectCatalogSongsWithArtists.ts @@ -55,9 +55,9 @@ export async function selectCatalogSongsWithArtists( album, notes, updated_at, - song_artists!inner ( + song_artists ( artist, - accounts!inner ( + accounts ( id, name, timestamp From 709ea0cb339e425cdf4b27cbbd34bfaccf374319 Mon Sep 17 00:00:00 2001 From: "sweetman.eth" Date: Thu, 18 Jun 2026 17:58:44 -0500 Subject: [PATCH 6/8] feat: add X (Twitter) + LinkedIn to the Composio connector whitelist (chat#1793) (#679) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat: add X (Twitter) + LinkedIn to the Composio connector whitelist (chat#1793) Expand the existing whitelist pattern to two new platforms — no architecture changes: - SUPPORTED_TOOLKITS (getConnectors.ts) + ENABLED_TOOLKITS (getComposioTools.ts) - CONNECTOR_DISPLAY_NAMES: twitter → "X (Twitter)", linkedin → "LinkedIn" - buildAuthConfigs() reads COMPOSIO_TWITTER_AUTH_CONFIG_ID + COMPOSIO_LINKEDIN_AUTH_CONFIG_ID - document both env vars in .env.example TDD: new buildAuthConfigs unit + expanded getConnectors / handler / ENABLED_TOOLKITS assertions, RED before GREEN. Full lib/composio suite green (157 tests). Implements the contract from docs#244. Co-Authored-By: Claude Opus 4.8 (1M context) * chore: fix lint/format — relocate ENABLED_TOOLKITS test block, reformat toolkit array - Move the ENABLED_TOOLKITS describe block below the imports (import/first) - Prettier-format the expanded toolkits array in getConnectors.test.ts Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Claude Opus 4.8 (1M context) --- .env.example | 2 + .../__tests__/buildAuthConfigs.test.ts | 48 +++++++++++++++++++ .../__tests__/getConnectors.test.ts | 22 ++++++++- .../__tests__/getConnectorsHandler.test.ts | 2 + lib/composio/connectors/buildAuthConfigs.ts | 6 +++ lib/composio/connectors/getConnectors.ts | 2 + .../connectors/getConnectorsHandler.ts | 2 + .../__tests__/getComposioTools.test.ts | 9 +++- lib/composio/toolRouter/getComposioTools.ts | 2 + 9 files changed, 92 insertions(+), 3 deletions(-) create mode 100644 lib/composio/connectors/__tests__/buildAuthConfigs.test.ts diff --git a/.env.example b/.env.example index 9d6de84d9..80bca6864 100644 --- a/.env.example +++ b/.env.example @@ -34,6 +34,8 @@ COMPOSIO_INSTAGRAM_AUTH_CONFIG_ID= COMPOSIO_GOOGLE_SHEETS_AUTH_CONFIG_ID= COMPOSIO_GOOGLE_DOCS_AUTH_CONFIG_ID= COMPOSIO_GOOGLE_DRIVE_AUTH_CONFIG_ID= +COMPOSIO_TWITTER_AUTH_CONFIG_ID= +COMPOSIO_LINKEDIN_AUTH_CONFIG_ID= # Slack (coding agent) SLACK_BOT_TOKEN= diff --git a/lib/composio/connectors/__tests__/buildAuthConfigs.test.ts b/lib/composio/connectors/__tests__/buildAuthConfigs.test.ts new file mode 100644 index 000000000..451dc92ed --- /dev/null +++ b/lib/composio/connectors/__tests__/buildAuthConfigs.test.ts @@ -0,0 +1,48 @@ +import { describe, it, expect, beforeEach, afterEach } from "vitest"; +import { buildAuthConfigs } from "../buildAuthConfigs"; + +const AUTH_CONFIG_ENV_KEYS = [ + "COMPOSIO_TIKTOK_AUTH_CONFIG_ID", + "COMPOSIO_INSTAGRAM_AUTH_CONFIG_ID", + "COMPOSIO_GOOGLE_SHEETS_AUTH_CONFIG_ID", + "COMPOSIO_GOOGLE_DOCS_AUTH_CONFIG_ID", + "COMPOSIO_GOOGLE_DRIVE_AUTH_CONFIG_ID", + "COMPOSIO_YOUTUBE_AUTH_CONFIG_ID", + "COMPOSIO_TWITTER_AUTH_CONFIG_ID", + "COMPOSIO_LINKEDIN_AUTH_CONFIG_ID", +]; + +describe("buildAuthConfigs", () => { + let saved: Record; + + beforeEach(() => { + // Snapshot then clear every auth-config env var so the test is isolated + // from whatever .env.local provides. + saved = {}; + for (const key of AUTH_CONFIG_ENV_KEYS) { + saved[key] = process.env[key]; + delete process.env[key]; + } + }); + + afterEach(() => { + for (const key of AUTH_CONFIG_ENV_KEYS) { + if (saved[key] === undefined) delete process.env[key]; + else process.env[key] = saved[key]; + } + }); + + it("returns undefined when no auth-config env vars are set", () => { + expect(buildAuthConfigs()).toBeUndefined(); + }); + + it("reads the Twitter and LinkedIn auth configs from env", () => { + process.env.COMPOSIO_TWITTER_AUTH_CONFIG_ID = "ac_twitter_123"; + process.env.COMPOSIO_LINKEDIN_AUTH_CONFIG_ID = "ac_linkedin_456"; + + expect(buildAuthConfigs()).toEqual({ + twitter: "ac_twitter_123", + linkedin: "ac_linkedin_456", + }); + }); +}); diff --git a/lib/composio/connectors/__tests__/getConnectors.test.ts b/lib/composio/connectors/__tests__/getConnectors.test.ts index 762c07e07..b637f1026 100644 --- a/lib/composio/connectors/__tests__/getConnectors.test.ts +++ b/lib/composio/connectors/__tests__/getConnectors.test.ts @@ -37,7 +37,16 @@ describe("getConnectors", () => { expect(getComposioClient).toHaveBeenCalled(); expect(mockComposio.create).toHaveBeenCalledWith("account-123", { - toolkits: ["googlesheets", "googledrive", "googledocs", "tiktok", "instagram", "youtube"], + toolkits: [ + "googlesheets", + "googledrive", + "googledocs", + "tiktok", + "instagram", + "youtube", + "twitter", + "linkedin", + ], }); expect(result).toEqual([ { @@ -92,7 +101,16 @@ describe("getConnectors", () => { await getConnectors("account-123"); expect(mockComposio.create).toHaveBeenCalledWith("account-123", { - toolkits: ["googlesheets", "googledrive", "googledocs", "tiktok", "instagram", "youtube"], + toolkits: [ + "googlesheets", + "googledrive", + "googledocs", + "tiktok", + "instagram", + "youtube", + "twitter", + "linkedin", + ], authConfigs: { tiktok: "ac_tiktok_123", instagram: "ac_instagram_456", diff --git a/lib/composio/connectors/__tests__/getConnectorsHandler.test.ts b/lib/composio/connectors/__tests__/getConnectorsHandler.test.ts index d6c3fe31f..bc68d0af6 100644 --- a/lib/composio/connectors/__tests__/getConnectorsHandler.test.ts +++ b/lib/composio/connectors/__tests__/getConnectorsHandler.test.ts @@ -72,6 +72,8 @@ describe("getConnectorsHandler", () => { googlesheets: "Google Sheets", googledrive: "Google Drive", googledocs: "Google Docs", + twitter: "X (Twitter)", + linkedin: "LinkedIn", }, }); }); diff --git a/lib/composio/connectors/buildAuthConfigs.ts b/lib/composio/connectors/buildAuthConfigs.ts index 2c31d571a..4edb893c0 100644 --- a/lib/composio/connectors/buildAuthConfigs.ts +++ b/lib/composio/connectors/buildAuthConfigs.ts @@ -23,5 +23,11 @@ export function buildAuthConfigs(): Record | undefined { if (process.env.COMPOSIO_YOUTUBE_AUTH_CONFIG_ID) { configs.youtube = process.env.COMPOSIO_YOUTUBE_AUTH_CONFIG_ID; } + if (process.env.COMPOSIO_TWITTER_AUTH_CONFIG_ID) { + configs.twitter = process.env.COMPOSIO_TWITTER_AUTH_CONFIG_ID; + } + if (process.env.COMPOSIO_LINKEDIN_AUTH_CONFIG_ID) { + configs.linkedin = process.env.COMPOSIO_LINKEDIN_AUTH_CONFIG_ID; + } return Object.keys(configs).length > 0 ? configs : undefined; } diff --git a/lib/composio/connectors/getConnectors.ts b/lib/composio/connectors/getConnectors.ts index dcb5fb38e..d692bf40c 100644 --- a/lib/composio/connectors/getConnectors.ts +++ b/lib/composio/connectors/getConnectors.ts @@ -23,6 +23,8 @@ const SUPPORTED_TOOLKITS = [ "tiktok", "instagram", "youtube", + "twitter", + "linkedin", ]; /** diff --git a/lib/composio/connectors/getConnectorsHandler.ts b/lib/composio/connectors/getConnectorsHandler.ts index d92f1293d..86633e32b 100644 --- a/lib/composio/connectors/getConnectorsHandler.ts +++ b/lib/composio/connectors/getConnectorsHandler.ts @@ -12,6 +12,8 @@ const CONNECTOR_DISPLAY_NAMES: Record = { googlesheets: "Google Sheets", googledrive: "Google Drive", googledocs: "Google Docs", + twitter: "X (Twitter)", + linkedin: "LinkedIn", }; /** diff --git a/lib/composio/toolRouter/__tests__/getComposioTools.test.ts b/lib/composio/toolRouter/__tests__/getComposioTools.test.ts index f49aff1d1..ae8d5d718 100644 --- a/lib/composio/toolRouter/__tests__/getComposioTools.test.ts +++ b/lib/composio/toolRouter/__tests__/getComposioTools.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; -import { getComposioTools } from "../getComposioTools"; +import { getComposioTools, ENABLED_TOOLKITS } from "../getComposioTools"; import { getComposioClient } from "../../client"; import { getCallbackUrl } from "../../getCallbackUrl"; @@ -235,3 +235,10 @@ describe("getComposioTools", () => { expect(result).toHaveProperty("COMPOSIO_MULTI_EXECUTE_TOOL"); }); }); + +describe("ENABLED_TOOLKITS", () => { + it("includes twitter and linkedin", () => { + expect(ENABLED_TOOLKITS).toContain("twitter"); + expect(ENABLED_TOOLKITS).toContain("linkedin"); + }); +}); diff --git a/lib/composio/toolRouter/getComposioTools.ts b/lib/composio/toolRouter/getComposioTools.ts index 61f0f887f..7f74b4c2f 100644 --- a/lib/composio/toolRouter/getComposioTools.ts +++ b/lib/composio/toolRouter/getComposioTools.ts @@ -16,6 +16,8 @@ export const ENABLED_TOOLKITS = [ "tiktok", "instagram", "youtube", + "twitter", + "linkedin", ]; const SHARED_ACCOUNT_ID = "recoup-shared-767f498e-e1e9-43c6-a152-a96ae3bd8d07"; From 66cc2fe2518ae526d15f12a70528de93410af815 Mon Sep 17 00:00:00 2001 From: "sweetman.eth" Date: Thu, 18 Jun 2026 18:30:11 -0500 Subject: [PATCH 7/8] chore: remove unused ALLOWED_ARTIST_CONNECTORS from api (chat#1793) (#680) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat: allow artists to connect X (Twitter); keep LinkedIn label-only (chat#1793) Add `twitter` to ALLOWED_ARTIST_CONNECTORS — artist-facing social, same class as tiktok/instagram/youtube. `linkedin` is intentionally left out (label/owner-only). TDD: isAllowedArtistConnector.test.ts asserts twitter allowed + linkedin excluded, RED before GREEN. Full lib/composio suite green (157 tests). Co-Authored-By: Claude Opus 4.8 (1M context) * feat: allow artists to connect LinkedIn too (chat#1793) Reversal of the earlier "LinkedIn label/owner-only" call: per owner decision 2026-06-18, LinkedIn is now an artist-facing connector like the others. Add `linkedin` to ALLOWED_ARTIST_CONNECTORS. TDD: flipped the linkedin assertions (now allowed/included), RED before GREEN. Full lib/composio suite green (159 tests). Co-Authored-By: Claude Opus 4.8 (1M context) * chore: remove unused ALLOWED_ARTIST_CONNECTORS from api (chat#1793) The api copy of the artist connector allow-list had no runtime consumer — only its definition, test, and an (also-unused) barrel re-export. The connector routes are unopinionated (allow any connector for any account); the allow-list that actually drives the artist Connectors tab lives in `chat` (`lib/composio/allowedArtistConnectors.ts`). Removing the dead code. Supersedes the earlier plan to add twitter/linkedin to this api constant (decision: owner, 2026-06-18) — the artist allow-list is chat-only. Deletes isAllowedArtistConnector.ts + its test, and the barrel re-export. lib/composio suite green (149); no new tsc errors vs test (198 baseline). Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Claude Opus 4.8 (1M context) --- .../isAllowedArtistConnector.test.ts | 43 ------------------- lib/composio/connectors/index.ts | 5 --- .../connectors/isAllowedArtistConnector.ts | 16 ------- 3 files changed, 64 deletions(-) delete mode 100644 lib/composio/connectors/__tests__/isAllowedArtistConnector.test.ts delete mode 100644 lib/composio/connectors/isAllowedArtistConnector.ts diff --git a/lib/composio/connectors/__tests__/isAllowedArtistConnector.test.ts b/lib/composio/connectors/__tests__/isAllowedArtistConnector.test.ts deleted file mode 100644 index 596407d20..000000000 --- a/lib/composio/connectors/__tests__/isAllowedArtistConnector.test.ts +++ /dev/null @@ -1,43 +0,0 @@ -import { describe, it, expect } from "vitest"; -import { isAllowedArtistConnector, ALLOWED_ARTIST_CONNECTORS } from "../isAllowedArtistConnector"; - -describe("isAllowedArtistConnector", () => { - it("should return true for 'tiktok'", () => { - expect(isAllowedArtistConnector("tiktok")).toBe(true); - }); - - it("should return true for 'instagram'", () => { - expect(isAllowedArtistConnector("instagram")).toBe(true); - }); - - it("should return true for 'youtube'", () => { - expect(isAllowedArtistConnector("youtube")).toBe(true); - }); - - it("should return false for connectors not in ALLOWED_ARTIST_CONNECTORS", () => { - expect(isAllowedArtistConnector("googlesheets")).toBe(false); - expect(isAllowedArtistConnector("googledrive")).toBe(false); - expect(isAllowedArtistConnector("random")).toBe(false); - }); - - it("should return false for empty string", () => { - expect(isAllowedArtistConnector("")).toBe(false); - }); - - it("should be case-sensitive", () => { - expect(isAllowedArtistConnector("TikTok")).toBe(false); - expect(isAllowedArtistConnector("TIKTOK")).toBe(false); - }); -}); - -describe("ALLOWED_ARTIST_CONNECTORS", () => { - it("should include tiktok, instagram, and youtube", () => { - expect(ALLOWED_ARTIST_CONNECTORS).toContain("tiktok"); - expect(ALLOWED_ARTIST_CONNECTORS).toContain("instagram"); - expect(ALLOWED_ARTIST_CONNECTORS).toContain("youtube"); - }); - - it("should be a readonly array", () => { - expect(Array.isArray(ALLOWED_ARTIST_CONNECTORS)).toBe(true); - }); -}); diff --git a/lib/composio/connectors/index.ts b/lib/composio/connectors/index.ts index 848b357ba..be0178edb 100644 --- a/lib/composio/connectors/index.ts +++ b/lib/composio/connectors/index.ts @@ -5,9 +5,4 @@ export { type AuthorizeConnectorOptions, } from "./authorizeConnector"; export { disconnectConnector, type DisconnectConnectorOptions } from "./disconnectConnector"; -export { - ALLOWED_ARTIST_CONNECTORS, - isAllowedArtistConnector, - type AllowedArtistConnector, -} from "./isAllowedArtistConnector"; export { verifyConnectorOwnership } from "./verifyConnectorOwnership"; diff --git a/lib/composio/connectors/isAllowedArtistConnector.ts b/lib/composio/connectors/isAllowedArtistConnector.ts deleted file mode 100644 index f3d83d82d..000000000 --- a/lib/composio/connectors/isAllowedArtistConnector.ts +++ /dev/null @@ -1,16 +0,0 @@ -/** - * List of toolkit slugs that artists are allowed to connect. - * Only these connectors will be shown in the artist-connectors API. - */ -export const ALLOWED_ARTIST_CONNECTORS = ["tiktok", "instagram", "youtube"] as const; - -export type AllowedArtistConnector = (typeof ALLOWED_ARTIST_CONNECTORS)[number]; - -/** - * Check if a connector slug is an allowed artist connector. - * - * @param slug - */ -export function isAllowedArtistConnector(slug: string): slug is AllowedArtistConnector { - return (ALLOWED_ARTIST_CONNECTORS as readonly string[]).includes(slug); -} From 6fc10cc3659360ee582c045509004764e086d7be Mon Sep 17 00:00:00 2001 From: "sweetman.eth" Date: Thu, 18 Jun 2026 18:52:05 -0500 Subject: [PATCH 8/8] fix: enrich valuation-captured songs (artists + notes) so they render in the catalog (#684) * fix: enrich captured songs with artists + notes (root cause) The valuation capture path created songs rows from the Spotify track lookup but discarded track.artists and never ran the manual flow's enrichment, so captured songs had no song_artists and no notes -> the chat catalog view's isCompleteSong filter (artist + notes required, on by default) hid every valuation track (count shown, list empty). mapUnmappedAlbumTracks now carries track.artists through and runs the same enrichment as processSongsInput: linkSongsToArtists (auto-creates the artist account) + queueRedisSongs (queues note generation). TDD: new test asserts artists are linked + queued; lib/research/playcounts + lib/songs 109 tests pass. Root-cause follow-up on recoupable/chat#1801. * style: prettier-format the capture-enrichment test Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Claude Opus 4.8 (1M context) --- .../__tests__/mapUnmappedAlbumTracks.test.ts | 30 ++++++++++++++++ .../playcounts/mapUnmappedAlbumTracks.ts | 34 ++++++++++++++++--- 2 files changed, 60 insertions(+), 4 deletions(-) diff --git a/lib/research/playcounts/__tests__/mapUnmappedAlbumTracks.test.ts b/lib/research/playcounts/__tests__/mapUnmappedAlbumTracks.test.ts index 954a5851c..714d597f7 100644 --- a/lib/research/playcounts/__tests__/mapUnmappedAlbumTracks.test.ts +++ b/lib/research/playcounts/__tests__/mapUnmappedAlbumTracks.test.ts @@ -5,6 +5,8 @@ import generateAccessToken from "@/lib/spotify/generateAccessToken"; import getTracks from "@/lib/spotify/getTracks"; import { upsertSongs } from "@/lib/supabase/songs/upsertSongs"; import { upsertSongIdentifiers } from "@/lib/supabase/song_identifiers/upsertSongIdentifiers"; +import { linkSongsToArtists } from "@/lib/songs/linkSongsToArtists"; +import { queueRedisSongs } from "@/lib/songs/queueRedisSongs"; import { SpotifyRateLimitError } from "@/lib/spotify/SpotifyRateLimitError"; vi.mock("@/lib/spotify/generateAccessToken", () => ({ default: vi.fn() })); @@ -13,6 +15,8 @@ vi.mock("@/lib/supabase/songs/upsertSongs", () => ({ upsertSongs: vi.fn() })); vi.mock("@/lib/supabase/song_identifiers/upsertSongIdentifiers", () => ({ upsertSongIdentifiers: vi.fn(), })); +vi.mock("@/lib/songs/linkSongsToArtists", () => ({ linkSongsToArtists: vi.fn() })); +vi.mock("@/lib/songs/queueRedisSongs", () => ({ queueRedisSongs: vi.fn() })); const ALBUMS = [ { @@ -55,6 +59,32 @@ describe("mapUnmappedAlbumTracks", () => { expect([...mapped.entries()]).toEqual([["t_new", "ISRC_NIKES"]]); }); + it("links captured songs to their Spotify artists and queues them for note enrichment", async () => { + vi.mocked(getTracks).mockResolvedValue({ + tracks: [ + { + id: "t_new", + name: "Nikes on My Feet", + external_ids: { isrc: "ISRC_NIKES" }, + artists: [{ id: "a1", name: "Mac Miller" }], + }, + ], + error: null, + } as never); + + await mapUnmappedAlbumTracks(ALBUMS, new Set(["t_mapped", "t_noisrc"])); + + // Root-cause fix: captured songs get the same enrichment as the manual flow — + // artists linked + queued for notes — so they aren't "missing info" in the catalog. + expect(linkSongsToArtists).toHaveBeenCalledWith([ + expect.objectContaining({ + isrc: "ISRC_NIKES", + spotifyArtists: [{ id: "a1", name: "Mac Miller" }], + }), + ]); + expect(queueRedisSongs).toHaveBeenCalledWith([expect.objectContaining({ isrc: "ISRC_NIKES" })]); + }); + it("returns an empty map without Spotify calls when everything is mapped", async () => { const mapped = await mapUnmappedAlbumTracks(ALBUMS, new Set(["t_mapped", "t_new", "t_noisrc"])); diff --git a/lib/research/playcounts/mapUnmappedAlbumTracks.ts b/lib/research/playcounts/mapUnmappedAlbumTracks.ts index e987b0376..e9fdb8fcd 100644 --- a/lib/research/playcounts/mapUnmappedAlbumTracks.ts +++ b/lib/research/playcounts/mapUnmappedAlbumTracks.ts @@ -4,6 +4,10 @@ import { upsertSongs } from "@/lib/supabase/songs/upsertSongs"; import { upsertSongIdentifiers } from "@/lib/supabase/song_identifiers/upsertSongIdentifiers"; import { SpotifyAlbumPlayCounts } from "@/lib/apify/spotify/fetchSpotifyAlbumPlayCounts"; import { SpotifyRateLimitError } from "@/lib/spotify/SpotifyRateLimitError"; +import { getSpotifyArtists } from "@/lib/songs/getSpotifyArtists"; +import { linkSongsToArtists } from "@/lib/songs/linkSongsToArtists"; +import { queueRedisSongs } from "@/lib/songs/queueRedisSongs"; +import { SongWithSpotify } from "@/lib/songs/getSongsByIsrc"; /** * Self-mapping bootstrap (chat#1794): resolve ISRCs for actor tracks that have @@ -49,20 +53,35 @@ export async function mapUnmappedAlbumTracks( name: track.name, albumId: context.albumId, albumName: context.albumName, + spotifyArtists: getSpotifyArtists(track.artists ?? []), }, ]; }); if (resolved.length === 0) return new Map(); // Dedupe by ISRC: reissues put the same recording on several albums in one - // batch, and upsertSongs' DO UPDATE cannot affect the same row twice. - const songsByIsrc = new Map( + // batch, and upsertSongs' DO UPDATE cannot affect the same row twice. Carry + // the Spotify artists through for enrichment. + const songsByIsrc = new Map( resolved.map(r => [ r.isrc, - { isrc: r.isrc, name: r.name ?? null, album: r.albumName ?? null }, + { + isrc: r.isrc, + name: r.name ?? null, + album: r.albumName ?? null, + spotifyArtists: r.spotifyArtists, + }, ]), ); - await upsertSongs([...songsByIsrc.values()]); + const songs = [...songsByIsrc.values()]; + + await upsertSongs( + songs.map(song => { + // eslint-disable-next-line @typescript-eslint/no-unused-vars + const { spotifyArtists, ...record } = song; + return record; + }), + ); await upsertSongIdentifiers( resolved.flatMap(r => [ { song: r.isrc, platform: "spotify", identifier_type: "track_id", value: r.trackId }, @@ -72,6 +91,13 @@ export async function mapUnmappedAlbumTracks( ]), ); + // Root cause (chat#1801): give captured songs the same enrichment as the + // manual/CSV flow — link artists (auto-creating the artist account) and + // queue note generation — so valuation tracks aren't "missing info" and + // render in the catalog view rather than being filtered out. + await linkSongsToArtists(songs); + await queueRedisSongs(songs); + return new Map(resolved.map(r => [r.trackId, r.isrc])); } catch (error) { if (error instanceof SpotifyRateLimitError) throw error;