From 0e850404919294ea0136ef63f69231728ace2608 Mon Sep 17 00:00:00 2001 From: bitfathers94 <237535319+bitfathers94@users.noreply.github.com> Date: Sun, 26 Jul 2026 14:47:55 +0000 Subject: [PATCH] fix(db): give the anomaly-alert dedup claims their own alert_dedup_claims table runAnomalyAlerts in src/review/alerts.ts writes per-hour dedup claims shaped (id, project, target_id, notification_key, status) with ON CONFLICT(project, target_id, notification_key) DO NOTHING, but pointed those raw INSERTs at notification_deliveries -- the migrated badge read-model (0031), whose columns are dedup_key/channel/recipient_login/... with a UNIQUE(dedup_key, channel) index. None of the claim columns nor the ON CONFLICT target exist there. runAnomalyAlerts has no callers yet, so this hasn't fired, but the moment it is wired to a cron path every Discord-notify invocation throws at the first INSERT. Add migration 0181 creating a distinctly-named alert_dedup_claims table with the (project, target_id, notification_key) unique index the port actually needs, and point both claim inserts at it. Allowlist the table in check-schema-drift as a raw-SQL-only feature table (alerts.ts accesses it via env.DB.prepare, not Drizzle). A new test drives runAnomalyAlerts against the real migrated D1 (createTestEnv) and asserts both claims land in alert_dedup_claims and a same-hour repeat is throttled by the unique constraint -- proving the write path no longer collides. Closes #8901 --- migrations/0181_alert_dedup_claims.sql | 19 ++++++++++++ scripts/check-schema-drift.ts | 1 + src/review/alerts.ts | 4 +-- test/unit/alerts.test.ts | 42 +++++++++++++++++++++++--- 4 files changed, 60 insertions(+), 6 deletions(-) create mode 100644 migrations/0181_alert_dedup_claims.sql diff --git a/migrations/0181_alert_dedup_claims.sql b/migrations/0181_alert_dedup_claims.sql new file mode 100644 index 0000000000..1fe68938b5 --- /dev/null +++ b/migrations/0181_alert_dedup_claims.sql @@ -0,0 +1,19 @@ +-- Dedicated dedup-claim store for the anomaly-alerter (#8901). src/review/alerts.ts's +-- `runAnomalyAlerts` throttles Discord alerts by INSERT ... ON CONFLICT(project, target_id, +-- notification_key) DO NOTHING against a claim table whose columns are (project, target_id, +-- notification_key) — a completely different shape than the migrated `notification_deliveries` +-- badge read-model (dedup_key/channel/recipient_login/...). The port was written against +-- `notification_deliveries` by name, so the moment it's wired to a cron path it would throw on its +-- first INSERT (no such columns / no such unique constraint). Give it its own table with the exact +-- (project, target_id, notification_key) unique index its ON CONFLICT target needs. +CREATE TABLE IF NOT EXISTS alert_dedup_claims ( + id TEXT PRIMARY KEY, + project TEXT NOT NULL, + target_id TEXT NOT NULL, + notification_key TEXT NOT NULL, + status TEXT NOT NULL DEFAULT 'sent', + created_at TEXT NOT NULL DEFAULT CURRENT_TIMESTAMP +); + +CREATE UNIQUE INDEX alert_dedup_claims_project_target_key_unique + ON alert_dedup_claims(project, target_id, notification_key); diff --git a/scripts/check-schema-drift.ts b/scripts/check-schema-drift.ts index 0d48fa058c..da60ce4b01 100644 --- a/scripts/check-schema-drift.ts +++ b/scripts/check-schema-drift.ts @@ -37,6 +37,7 @@ const MIGRATIONS_DIR = process.env.CHECK_SCHEMA_DRIFT_DIR || "migrations"; // table here without also confirming it is genuinely raw-SQL-only is a reviewer-visible diff, not a silent // gap this check would otherwise catch. export const RAW_SQL_ONLY_TABLES: Set = new Set([ + "alert_dedup_claims", "ams_instances", "ams_signals", "contributor_gate_history", diff --git a/src/review/alerts.ts b/src/review/alerts.ts index 255a53ea19..1cee22214a 100644 --- a/src/review/alerts.ts +++ b/src/review/alerts.ts @@ -230,7 +230,7 @@ export async function runAnomalyAlerts(env: Env, config: AlertAgentConfig, deps: // computes + maybe alerts; the other 59 short-circuit here before touching D1. const hourBucket = new Date().toISOString().slice(0, 13); // YYYY-MM-DDTHH const checkClaim = await storage(env).prepare( - `INSERT INTO notification_deliveries (id, project, target_id, notification_key, status) + `INSERT INTO alert_dedup_claims (id, project, target_id, notification_key, status) VALUES (?, ?, '__healthcheck__', ?, 'sent') ON CONFLICT(project, target_id, notification_key) DO NOTHING`, ) @@ -246,7 +246,7 @@ export async function runAnomalyAlerts(env: Env, config: AlertAgentConfig, deps: // Throttle: claim a per-(condition-set, hour) key so a repeated condition alerts at most hourly. const key = await sha256Hex(`anomaly:${anomalies.join("|")}:${hourBucket}`); const claim = await storage(env).prepare( - `INSERT INTO notification_deliveries (id, project, target_id, notification_key, status) + `INSERT INTO alert_dedup_claims (id, project, target_id, notification_key, status) VALUES (?, ?, '__anomaly__', ?, 'sent') ON CONFLICT(project, target_id, notification_key) DO NOTHING`, ) diff --git a/test/unit/alerts.test.ts b/test/unit/alerts.test.ts index 5cb034c7f8..0cfaac0416 100644 --- a/test/unit/alerts.test.ts +++ b/test/unit/alerts.test.ts @@ -7,6 +7,7 @@ import { detectAnomalies, runAnomalyAlerts, } from "../../src/review/alerts"; +import { createTestEnv } from "../helpers/d1"; const healthy: AgentHealth = { byStatus: {}, @@ -174,10 +175,11 @@ describe("runAnomalyAlerts guards", () => { }); // ── runAnomalyAlerts send path ─────────────────────────────────────────────────────────────────────── -// loopover's migrated `notification_deliveries` table is the badge read-model — a DIFFERENT schema than -// the native port's claim SQL (project/target_id/notification_key). So we emulate the claim store: the -// INSERT ... ON CONFLICT(project, target_id, notification_key) DO NOTHING returns changes=1 the first time a -// (project, target_id, notification_key) tuple is seen and changes=0 on a repeat (the per-hour throttle). +// The port claims dedup slots in `alert_dedup_claims` (its own (project, target_id, notification_key) +// table — #8901), so we emulate the claim store: the INSERT ... ON CONFLICT(project, target_id, +// notification_key) DO NOTHING returns changes=1 the first time a (project, target_id, notification_key) +// tuple is seen and changes=0 on a repeat (the per-hour throttle). The real-schema test at the bottom of +// this file exercises the same INSERTs against the actual migrated table. function claimEnv(extra: Record = {}): Env { const seen = new Set(); return { @@ -361,3 +363,35 @@ describe("runAnomalyAlerts — send path", () => { expect(fetchSpy).not.toHaveBeenCalled(); // but the anomaly claim conflicted → no POST }); }); + +// ── real alert_dedup_claims schema (#8901) ───────────────────────────────────────────────────────────── +// Regression guard for the latent schema collision: alerts.ts used to INSERT into `notification_deliveries` +// (project/target_id/notification_key columns + ON CONFLICT on that tuple), but the migrated +// `notification_deliveries` is the badge read-model with a totally different shape, so the first real INSERT +// would throw. These run the ACTUAL INSERT ... ON CONFLICT against a fully-migrated DB (createTestEnv applies +// migrations/**, including 0181_alert_dedup_claims.sql) to prove the write path now succeeds end-to-end. +describe("runAnomalyAlerts — real alert_dedup_claims schema (#8901)", () => { + afterEach(() => vi.unstubAllGlobals()); + + it("lands both dedup claims in the real migrated table and POSTs once, then throttles the repeat", async () => { + const fetchSpy = vi.fn(async () => new Response(null, { status: 204 })); + vi.stubGlobal("fetch", fetchSpy); + const env = createTestEnv(); + const config = { slug: "ac", features: { discordNotify: true }, secrets: {}, discordWebhookUrl: WEBHOOK } as AlertAgentConfig; + const deps: AnomalyAlertDeps = { computeAgentHealth: async () => anomalousHealth, computeCalibration: async () => driftCal }; + + await runAnomalyAlerts(env, config, deps); + expect(fetchSpy).toHaveBeenCalledTimes(1); // the INSERTs succeeded against the real (project, target_id, notification_key) schema + + const rows = await env.DB.prepare("SELECT project, target_id, status FROM alert_dedup_claims ORDER BY target_id") + .all<{ project: string; target_id: string; status: string }>(); + expect(rows.results.map((r) => r.target_id)).toEqual(["__anomaly__", "__healthcheck__"]); + expect(rows.results.every((r) => r.project === "ac" && r.status === "sent")).toBe(true); + + // A second run the same hour re-hits the per-hour healthcheck claim → ON CONFLICT DO NOTHING → no new POST. + await runAnomalyAlerts(env, config, deps); + expect(fetchSpy).toHaveBeenCalledTimes(1); + const after = await env.DB.prepare("SELECT count(*) AS n FROM alert_dedup_claims").first<{ n: number }>(); + expect(after?.n).toBe(2); // still exactly the two claims — the conflicting re-insert added nothing + }); +});