-
Notifications
You must be signed in to change notification settings - Fork 10
feat(apify): digest rate cap (1 per artist per 24h) + email_send_log audit trail #762
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
360268b
9a991be
7960d93
68bd515
c10c383
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -3,13 +3,18 @@ import { getRunDigestSection } from "@/lib/apify/digest/getRunDigestSection"; | |||||||
| import type { RunDigestSection } from "@/lib/apify/digest/getRunDigestSection"; | ||||||||
| import { getScrapeDigestRecipients } from "@/lib/apify/digest/getScrapeDigestRecipients"; | ||||||||
| import { sendScrapeDigestEmail } from "@/lib/apify/digest/sendScrapeDigestEmail"; | ||||||||
| import { selectScrapeDigestLogs } from "@/lib/supabase/email_send_log/selectScrapeDigestLogs"; | ||||||||
| import { logEmailAttempt } from "@/lib/emails/logEmailAttempt"; | ||||||||
|
|
||||||||
| /** | ||||||||
| * Batch-completion check for the one-digest-per-scrape design (chat#1855): | ||||||||
| * called after each platform run's results are processed. Sends the single | ||||||||
| * consolidated digest when (a) every sibling run in the batch has completed | ||||||||
| * and (b) at least one platform found genuinely new posts. Platforms with | ||||||||
| * nothing new are omitted; a batch with nothing new sends nothing. | ||||||||
| * consolidated digest when (a) every sibling run in the batch has completed, | ||||||||
| * (b) at least one platform found genuinely new posts, (c) this batch has | ||||||||
| * never sent before (exact idempotency via its `scrape-digest:<batchId>` | ||||||||
| * audit row — a webhook retry must not duplicate the send), and (d) no | ||||||||
| * digest went to any watching artist in the last 24h (product frequency | ||||||||
| * cap). Every send is recorded in email_send_log per watched artist. | ||||||||
| */ | ||||||||
| export async function maybeSendScrapeDigest(batchId: string | null | undefined) { | ||||||||
| if (!batchId) return null; | ||||||||
|
|
@@ -22,10 +27,35 @@ export async function maybeSendScrapeDigest(batchId: string | null | undefined) | |||||||
| ); | ||||||||
| if (!sections.length) return null; | ||||||||
|
|
||||||||
| const emails = await getScrapeDigestRecipients( | ||||||||
| const { emails, artistIds } = await getScrapeDigestRecipients( | ||||||||
| runs.map(r => r.social_id).filter((id): id is string => Boolean(id)), | ||||||||
| ); | ||||||||
| // A batch is one artist's scrape — any platform's profile name addresses it. | ||||||||
|
|
||||||||
| // Exact non-duplication: one digest per batch, ever. | ||||||||
| const already = await selectScrapeDigestLogs({ batchId }); | ||||||||
| if (already.length > 0) return null; | ||||||||
|
|
||||||||
| // Product frequency cap: at most one digest per watched artist per 24h. | ||||||||
| const since = new Date(Date.now() - 24 * 60 * 60 * 1000).toISOString(); | ||||||||
| const recent = await selectScrapeDigestLogs({ artistIds, since }); | ||||||||
| if (recent.length > 0) return null; | ||||||||
|
|
||||||||
| const artistName = sections.map(s => s.artistName).find(Boolean) ?? null; | ||||||||
| return await sendScrapeDigestEmail({ emails, sections, artistName }); | ||||||||
| const sent = await sendScrapeDigestEmail({ emails, sections, artistName }); | ||||||||
| if (!sent) return null; | ||||||||
|
|
||||||||
| // Audit trail: record the send per watched artist entity — what the batch | ||||||||
| // dedup and the 24h cap read (the legacy solo path never logged at all). | ||||||||
|
Comment on lines
+47
to
+48
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Replace "entity" terminology per coding guidelines. The comment uses "watched artist entity" which violates the guideline to never use As per coding guidelines: "Use ✏️ Proposed fix- // Audit trail: record the send per watched artist entity — what the batch
+ // Audit trail: record the send per watched artist account — what the batch📝 Committable suggestion
Suggested change
🤖 Prompt for AI AgentsSource: Coding guidelines |
||||||||
| const resendId = (sent as { id?: string }).id; | ||||||||
| await Promise.all( | ||||||||
| artistIds.map(artistId => | ||||||||
| logEmailAttempt({ | ||||||||
| rawBody: `scrape-digest:${batchId}`, | ||||||||
| status: "sent", | ||||||||
| accountId: artistId, | ||||||||
| resendId, | ||||||||
| }), | ||||||||
| ), | ||||||||
| ); | ||||||||
| return sent; | ||||||||
| } | ||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| import { describe, it, expect, vi, beforeEach } from "vitest"; | ||
| import { updateApifyScraperRun } from "@/lib/supabase/apify_scraper_runs/updateApifyScraperRun"; | ||
|
|
||
| const maybeSingle = vi.fn(); | ||
| const chain = { | ||
| update: vi.fn(() => chain), | ||
| eq: vi.fn(() => chain), | ||
| is: vi.fn(() => chain), | ||
| select: vi.fn(() => chain), | ||
| maybeSingle, | ||
| }; | ||
| vi.mock("@/lib/supabase/serverClient", () => ({ default: { from: vi.fn(() => chain) } })); | ||
|
|
||
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| }); | ||
|
|
||
| describe("updateApifyScraperRun", () => { | ||
| it("claims completion only for runs not yet completed — the idempotency guard", async () => { | ||
| maybeSingle.mockResolvedValue({ data: { run_id: "r1", batch_id: "b1" }, error: null }); | ||
| const row = await updateApifyScraperRun("r1", ["https://x.com/a/status/1"]); | ||
| expect(chain.eq).toHaveBeenCalledWith("run_id", "r1"); | ||
| expect(chain.is).toHaveBeenCalledWith("completed_at", null); | ||
| expect(row).toEqual({ run_id: "r1", batch_id: "b1" }); | ||
| }); | ||
|
|
||
| it("returns null on a webhook retry (row already completed) so the digest can never re-trigger", async () => { | ||
| maybeSingle.mockResolvedValue({ data: null, error: null }); | ||
| expect(await updateApifyScraperRun("r1", [])).toBeNull(); | ||
| }); | ||
|
|
||
| it("returns null and logs on error", async () => { | ||
| maybeSingle.mockResolvedValue({ data: null, error: { message: "boom" } }); | ||
| expect(await updateApifyScraperRun("r1", [])).toBeNull(); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,37 @@ | ||||||
| import supabase from "@/lib/supabase/serverClient"; | ||||||
|
|
||||||
| /** | ||||||
| * Select scrape-digest send-log rows with optional filters. | ||||||
| * | ||||||
| * @param batchId - Rows recording exactly this batch's send (idempotency check). | ||||||
| * @param artistIds - Rows for any of these watched artist entities. | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Replace "entity" terminology per coding guidelines. The JSDoc uses "watched artist entities" which violates the guideline to never use As per coding guidelines: "Use ✏️ Proposed fix- * `@param` artistIds - Rows for any of these watched artist entities.
+ * `@param` artistIds - Rows for any of these watched artist accounts.📝 Committable suggestion
Suggested change
🤖 Prompt for AI AgentsSource: Coding guidelines |
||||||
| * @param since - Only rows created at or after this ISO timestamp (rate-cap lookback). | ||||||
| */ | ||||||
| export async function selectScrapeDigestLogs({ | ||||||
| batchId, | ||||||
| artistIds, | ||||||
| since, | ||||||
| }: { | ||||||
| batchId?: string; | ||||||
| artistIds?: string[]; | ||||||
| since?: string; | ||||||
| } = {}) { | ||||||
|
Comment on lines
+10
to
+18
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Add an explicit return type annotation. The function lacks a return type annotation. Path instructions require "Return typed results using Tables<"table_name">". Since only As per path instructions: "Return typed results using Tables<"table_name">". ✏️ Proposed fix-export async function selectScrapeDigestLogs({
+export async function selectScrapeDigestLogs({
batchId,
artistIds,
since,
}: {
batchId?: string;
artistIds?: string[];
since?: string;
} = {}) {
+): Promise<Pick<Tables<"email_send_log">, "account_id" | "created_at">[]> {🤖 Prompt for AI AgentsSource: Path instructions |
||||||
| if (artistIds !== undefined && artistIds.length === 0) return []; | ||||||
|
|
||||||
| let query = supabase.from("email_send_log").select("account_id, created_at"); | ||||||
|
|
||||||
| if (batchId) { | ||||||
| query = query.eq("raw_body", `scrape-digest:${batchId}`); | ||||||
| } else { | ||||||
| query = query.like("raw_body", "scrape-digest:%"); | ||||||
| } | ||||||
| if (artistIds) query = query.in("account_id", artistIds); | ||||||
| if (since) query = query.gte("created_at", since); | ||||||
|
|
||||||
| const { data, error } = await query; | ||||||
| if (error) { | ||||||
| console.error("[ERROR] selectScrapeDigestLogs:", error); | ||||||
| return []; | ||||||
| } | ||||||
| return data ?? []; | ||||||
| } | ||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
TOCTOU race between idempotency check and audit log write can cause duplicate sends.
The batch idempotency check (
selectScrapeDigestLogs({ batchId })on line 35) and the audit log write (logEmailAttempton lines 50-58) are not atomic. WhileupdateApifyScraperRunprevents the same run from being claimed twice, two different runs in the same batch completing concurrently can both pass the idempotency check before either writes audit entries. The race window includes the email send (a network call to Resend), widening the vulnerability window.Consider writing audit entries before sending (e.g., with a
pendingstatus, updated tosentafter successful delivery), or using a database-level constraint/advisory lock on the batch to ensure atomicity.🤖 Prompt for AI Agents