diff --git a/migrations/0181_alert_dedup_claims.sql b/migrations/0181_alert_dedup_claims.sql new file mode 100644 index 0000000000..78f888687f --- /dev/null +++ b/migrations/0181_alert_dedup_claims.sql @@ -0,0 +1,16 @@ +-- #8901: src/review/alerts.ts's runAnomalyAlerts (the anomaly-alert port) writes hourly dedup CLAIMS with the +-- shape (id, project, target_id, notification_key, status) and ON CONFLICT(project, target_id, notification_key) +-- DO NOTHING. It was pointing those raw INSERTs at `notification_deliveries` (migration 0031), whose real schema +-- has none of those columns and no such unique constraint — so the moment runAnomalyAlerts is wired to a cron +-- path, its first INSERT throws. This gives the port its own correctly-shaped table with the real unique index +-- it needs, independent of whether it is ever wired. +CREATE TABLE IF NOT EXISTS alert_dedup_claims ( + id TEXT PRIMARY KEY, -- opaque claim id (newId("hc")/newId("anm")) + project TEXT NOT NULL, -- agent slug + target_id TEXT NOT NULL, -- '__healthcheck__' or '__anomaly__' + notification_key TEXT NOT NULL, -- hashed per-hour / per-condition-set key + status TEXT NOT NULL DEFAULT 'sent', + -- The claim mechanism relies on this exact unique constraint: the ON CONFLICT(...) DO NOTHING lets exactly one + -- writer per (project, target_id, notification_key) win the hourly claim; the losers see zero changed rows. + UNIQUE (project, target_id, notification_key) +); 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..a1a9aa6369 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: {}, @@ -229,6 +230,32 @@ describe("runAnomalyAlerts — send path", () => { expect(body.embeds[0]?.description).toMatch(/calibration drift|DEAD-LETTERED|permanently failed/); }); + it("REGRESSION (#8901): the dedup-claim inserts succeed against the REAL migrated schema (alert_dedup_claims)", async () => { + // The other send-path tests use a mocked claim store (claimEnv), which is exactly why the + // notification_deliveries collision stayed latent. Against a REAL migrated D1, the old code's + // `INSERT INTO notification_deliveries (id, project, target_id, notification_key, status)` threw + // "no column named project"; alert_dedup_claims (migration 0181) has those columns + the unique index. + const fetchSpy = vi.fn(async () => new Response(null, { status: 204 })); + vi.stubGlobal("fetch", fetchSpy); + const env = createTestEnv(); + const config = { slug: "anomaly-agent", 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); // reached the send path => both claim inserts succeeded + + // Both claims landed in the new table, and its unique constraint powers the throttle: a second run in the + // same hour loses the healthcheck claim (ON CONFLICT DO NOTHING => changes=0) and short-circuits, so no + // second Discord POST fires. + const rows = await env.DB.prepare("SELECT target_id FROM alert_dedup_claims WHERE project = ? ORDER BY target_id") + .bind("anomaly-agent") + .all<{ target_id: string }>(); + expect((rows.results ?? []).map((r) => r.target_id)).toEqual(["__anomaly__", "__healthcheck__"]); + + await runAnomalyAlerts(env, config, deps); + expect(fetchSpy).toHaveBeenCalledTimes(1); // still once: this hour's healthcheck claim was already taken + }); + it("resolves the webhook from a named secret (config.secrets.discordWebhook → env[name])", async () => { const fetchSpy = vi.fn(async () => new Response(null, { status: 204 })); vi.stubGlobal("fetch", fetchSpy);