Skip to content

Commit 330fdd6

Browse files
authored
fix(db): give the anomaly-alert dedup claims their own alert_dedup_claims table (#8985)
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 Co-authored-by: bitfathers94 <237535319+bitfathers94@users.noreply.github.com>
1 parent 2f7104e commit 330fdd6

4 files changed

Lines changed: 60 additions & 6 deletions

File tree

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
-- Dedicated dedup-claim store for the anomaly-alerter (#8901). src/review/alerts.ts's
2+
-- `runAnomalyAlerts` throttles Discord alerts by INSERT ... ON CONFLICT(project, target_id,
3+
-- notification_key) DO NOTHING against a claim table whose columns are (project, target_id,
4+
-- notification_key) — a completely different shape than the migrated `notification_deliveries`
5+
-- badge read-model (dedup_key/channel/recipient_login/...). The port was written against
6+
-- `notification_deliveries` by name, so the moment it's wired to a cron path it would throw on its
7+
-- first INSERT (no such columns / no such unique constraint). Give it its own table with the exact
8+
-- (project, target_id, notification_key) unique index its ON CONFLICT target needs.
9+
CREATE TABLE IF NOT EXISTS alert_dedup_claims (
10+
id TEXT PRIMARY KEY,
11+
project TEXT NOT NULL,
12+
target_id TEXT NOT NULL,
13+
notification_key TEXT NOT NULL,
14+
status TEXT NOT NULL DEFAULT 'sent',
15+
created_at TEXT NOT NULL DEFAULT CURRENT_TIMESTAMP
16+
);
17+
18+
CREATE UNIQUE INDEX alert_dedup_claims_project_target_key_unique
19+
ON alert_dedup_claims(project, target_id, notification_key);

scripts/check-schema-drift.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ const MIGRATIONS_DIR = process.env.CHECK_SCHEMA_DRIFT_DIR || "migrations";
3737
// table here without also confirming it is genuinely raw-SQL-only is a reviewer-visible diff, not a silent
3838
// gap this check would otherwise catch.
3939
export const RAW_SQL_ONLY_TABLES: Set<string> = new Set([
40+
"alert_dedup_claims",
4041
"ams_instances",
4142
"ams_signals",
4243
"contributor_gate_history",

src/review/alerts.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -230,7 +230,7 @@ export async function runAnomalyAlerts(env: Env, config: AlertAgentConfig, deps:
230230
// computes + maybe alerts; the other 59 short-circuit here before touching D1.
231231
const hourBucket = new Date().toISOString().slice(0, 13); // YYYY-MM-DDTHH
232232
const checkClaim = await storage(env).prepare(
233-
`INSERT INTO notification_deliveries (id, project, target_id, notification_key, status)
233+
`INSERT INTO alert_dedup_claims (id, project, target_id, notification_key, status)
234234
VALUES (?, ?, '__healthcheck__', ?, 'sent')
235235
ON CONFLICT(project, target_id, notification_key) DO NOTHING`,
236236
)
@@ -246,7 +246,7 @@ export async function runAnomalyAlerts(env: Env, config: AlertAgentConfig, deps:
246246
// Throttle: claim a per-(condition-set, hour) key so a repeated condition alerts at most hourly.
247247
const key = await sha256Hex(`anomaly:${anomalies.join("|")}:${hourBucket}`);
248248
const claim = await storage(env).prepare(
249-
`INSERT INTO notification_deliveries (id, project, target_id, notification_key, status)
249+
`INSERT INTO alert_dedup_claims (id, project, target_id, notification_key, status)
250250
VALUES (?, ?, '__anomaly__', ?, 'sent')
251251
ON CONFLICT(project, target_id, notification_key) DO NOTHING`,
252252
)

test/unit/alerts.test.ts

Lines changed: 38 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import {
77
detectAnomalies,
88
runAnomalyAlerts,
99
} from "../../src/review/alerts";
10+
import { createTestEnv } from "../helpers/d1";
1011

1112
const healthy: AgentHealth = {
1213
byStatus: {},
@@ -174,10 +175,11 @@ describe("runAnomalyAlerts guards", () => {
174175
});
175176

176177
// ── runAnomalyAlerts send path ───────────────────────────────────────────────────────────────────────
177-
// loopover's migrated `notification_deliveries` table is the badge read-model — a DIFFERENT schema than
178-
// the native port's claim SQL (project/target_id/notification_key). So we emulate the claim store: the
179-
// INSERT ... ON CONFLICT(project, target_id, notification_key) DO NOTHING returns changes=1 the first time a
180-
// (project, target_id, notification_key) tuple is seen and changes=0 on a repeat (the per-hour throttle).
178+
// The port claims dedup slots in `alert_dedup_claims` (its own (project, target_id, notification_key)
179+
// table — #8901), so we emulate the claim store: the INSERT ... ON CONFLICT(project, target_id,
180+
// notification_key) DO NOTHING returns changes=1 the first time a (project, target_id, notification_key)
181+
// tuple is seen and changes=0 on a repeat (the per-hour throttle). The real-schema test at the bottom of
182+
// this file exercises the same INSERTs against the actual migrated table.
181183
function claimEnv(extra: Record<string, unknown> = {}): Env {
182184
const seen = new Set<string>();
183185
return {
@@ -361,3 +363,35 @@ describe("runAnomalyAlerts — send path", () => {
361363
expect(fetchSpy).not.toHaveBeenCalled(); // but the anomaly claim conflicted → no POST
362364
});
363365
});
366+
367+
// ── real alert_dedup_claims schema (#8901) ─────────────────────────────────────────────────────────────
368+
// Regression guard for the latent schema collision: alerts.ts used to INSERT into `notification_deliveries`
369+
// (project/target_id/notification_key columns + ON CONFLICT on that tuple), but the migrated
370+
// `notification_deliveries` is the badge read-model with a totally different shape, so the first real INSERT
371+
// would throw. These run the ACTUAL INSERT ... ON CONFLICT against a fully-migrated DB (createTestEnv applies
372+
// migrations/**, including 0181_alert_dedup_claims.sql) to prove the write path now succeeds end-to-end.
373+
describe("runAnomalyAlerts — real alert_dedup_claims schema (#8901)", () => {
374+
afterEach(() => vi.unstubAllGlobals());
375+
376+
it("lands both dedup claims in the real migrated table and POSTs once, then throttles the repeat", async () => {
377+
const fetchSpy = vi.fn(async () => new Response(null, { status: 204 }));
378+
vi.stubGlobal("fetch", fetchSpy);
379+
const env = createTestEnv();
380+
const config = { slug: "ac", features: { discordNotify: true }, secrets: {}, discordWebhookUrl: WEBHOOK } as AlertAgentConfig;
381+
const deps: AnomalyAlertDeps = { computeAgentHealth: async () => anomalousHealth, computeCalibration: async () => driftCal };
382+
383+
await runAnomalyAlerts(env, config, deps);
384+
expect(fetchSpy).toHaveBeenCalledTimes(1); // the INSERTs succeeded against the real (project, target_id, notification_key) schema
385+
386+
const rows = await env.DB.prepare("SELECT project, target_id, status FROM alert_dedup_claims ORDER BY target_id")
387+
.all<{ project: string; target_id: string; status: string }>();
388+
expect(rows.results.map((r) => r.target_id)).toEqual(["__anomaly__", "__healthcheck__"]);
389+
expect(rows.results.every((r) => r.project === "ac" && r.status === "sent")).toBe(true);
390+
391+
// A second run the same hour re-hits the per-hour healthcheck claim → ON CONFLICT DO NOTHING → no new POST.
392+
await runAnomalyAlerts(env, config, deps);
393+
expect(fetchSpy).toHaveBeenCalledTimes(1);
394+
const after = await env.DB.prepare("SELECT count(*) AS n FROM alert_dedup_claims").first<{ n: number }>();
395+
expect(after?.n).toBe(2); // still exactly the two claims — the conflicting re-insert added nothing
396+
});
397+
});

0 commit comments

Comments
 (0)