From 42506b80ee53edaec2fa19ba3fe322de710045e6 Mon Sep 17 00:00:00 2001 From: shin-core <153108882+shin-core@users.noreply.github.com> Date: Wed, 29 Jul 2026 19:35:09 +0900 Subject: [PATCH] fix(ops): scope the PagerDuty cooldown to rows that actually paged The cooldown counted every `external_notification.pagerduty` row for the dedupKey via `countRecentAuditEventsForActorAndTarget`, which filters neither `outcome` nor `detail`. But that eventType is written for every path: a real page (completed/triggered), a suppression (denied/cooldown_active), a failed page (error), and an auto-resolve (completed/resolved). So a single page self-renewed the window every cron tick (its own denied row kept the count > 0), a non-page silenced a real one, a failed page blocked its own retry, and an auto-resolve suppressed the re-page for a flapping condition. Add `countRecentAuditEventsForActorTargetAndOutcome`, shaped like its sibling with `outcome` + `detail` equality terms, and count only completed/triggered rows for both the loopover and legacy gittensory actors. Export `PAGERDUTY_AUDIT_DETAIL_TRIGGERED`/`_RESOLVED` and use them at both the write and read sites so the two spellings can never drift. The denied/error/resolved rows are still written (the operator's evidence); they are simply no longer counted. Closes #9695 --- src/db/repositories.ts | 33 ++++++++++++++++++ src/services/notify-pagerduty.ts | 16 ++++++--- test/unit/notify-pagerduty.test.ts | 54 ++++++++++++++++++++++++++++++ 3 files changed, 98 insertions(+), 5 deletions(-) diff --git a/src/db/repositories.ts b/src/db/repositories.ts index ec9bed2e76..fe0ccf3a7c 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -3161,6 +3161,39 @@ export async function countRecentAuditEventsForActorAndTarget(env: Env, actor: s return row.count; } +/** Same as {@link countRecentAuditEventsForActorAndTarget} but additionally scoped to a specific `outcome` + + * `detail`. Backs the PagerDuty cooldown (#9695): a repeat page is suppressed only by rows that ACTUALLY paged + * (outcome "completed", detail "triggered"), never by the suppression rows or the auto-resolve rows that share + * the same eventType/targetKey — otherwise a single page (or even a `denied` non-page) self-renewed the window + * every cron tick and the repo never paged again. */ +export async function countRecentAuditEventsForActorTargetAndOutcome( + env: Env, + actor: string, + eventType: string, + targetKey: string, + outcome: string, + detail: string, + sinceIso: string, +): Promise { + const db = getDb(env.DB); + const [row] = await db + .select({ count: sql`count(*)` }) + .from(auditEvents) + .where( + and( + eq(auditEvents.actor, actor), + eq(auditEvents.eventType, eventType), + eq(auditEvents.targetKey, targetKey), + eq(auditEvents.outcome, outcome), + eq(auditEvents.detail, detail), + gte(auditEvents.createdAt, sinceIso), + ), + ); + /* v8 ignore next -- count(*) always returns exactly one row; the empty-array guard only satisfies the destructure type. */ + if (!row) return 0; + return row.count; +} + /** Shared by every `targetKey` literal-prefix `LIKE` scan below ({@link countRecentAuditEventsForActorInRepo}, * {@link findHottestReviewTargetForRepo}) so a repo name containing a SQL `LIKE` wildcard (`%`/`_`) is always * matched literally, never as a pattern -- e.g. `owner/foo_bar` must never spuriously match `owner/fooXbar#...`'s diff --git a/src/services/notify-pagerduty.ts b/src/services/notify-pagerduty.ts index 28f117ec31..9f9ebcf610 100644 --- a/src/services/notify-pagerduty.ts +++ b/src/services/notify-pagerduty.ts @@ -1,4 +1,4 @@ -import { countRecentAuditEventsForActorAndTarget, recordAuditEvent } from "../db/repositories"; +import { countRecentAuditEventsForActorTargetAndOutcome, recordAuditEvent } from "../db/repositories"; import { errorMessage } from "../utils/json"; import { meetsSeverityThreshold, resolveSeverityThreshold, type LoopoverSeverity } from "./severity-threshold"; @@ -21,6 +21,12 @@ import { meetsSeverityThreshold, resolveSeverityThreshold, type LoopoverSeverity // every cron tick doesn't re-page every tick. const PAGERDUTY_EVENTS_URL = "https://events.pagerduty.com/v2/enqueue"; + +// The audit `detail` written for a real page vs an auto-resolve. Both share outcome "completed", so the cooldown +// must key off `detail` to count only rows that ACTUALLY paged (#9695). Exported + used at both the write and +// the read site so the two spellings can never drift. +export const PAGERDUTY_AUDIT_DETAIL_TRIGGERED = "triggered"; +export const PAGERDUTY_AUDIT_DETAIL_RESOLVED = "resolved"; // PagerDuty routing/integration keys are 32 lowercase hex characters. const ROUTING_KEY_RE = /^[a-f0-9]{32}$/i; const DEFAULT_MIN_SEVERITY: PagerDutySeverity = "error"; @@ -182,8 +188,8 @@ export async function triggerPagerDutyIncident( // window can reach back across the deploy boundary. Querying both actors costs one extra indexed count and // removes the whole risk category rather than requiring a precisely-timed follow-up cleanup. const [recentPagesNewActor, recentPagesLegacyActor] = await Promise.all([ - countRecentAuditEventsForActorAndTarget(env, "loopover", "external_notification.pagerduty", params.dedupKey, cooldownSinceIso), - countRecentAuditEventsForActorAndTarget(env, "gittensory", "external_notification.pagerduty", params.dedupKey, cooldownSinceIso), + countRecentAuditEventsForActorTargetAndOutcome(env, "loopover", "external_notification.pagerduty", params.dedupKey, "completed", PAGERDUTY_AUDIT_DETAIL_TRIGGERED, cooldownSinceIso), + countRecentAuditEventsForActorTargetAndOutcome(env, "gittensory", "external_notification.pagerduty", params.dedupKey, "completed", PAGERDUTY_AUDIT_DETAIL_TRIGGERED, cooldownSinceIso), ]); const recentPages = recentPagesNewActor + recentPagesLegacyActor; if (recentPages > 0) { @@ -211,7 +217,7 @@ export async function triggerPagerDutyIncident( signal: AbortSignal.timeout(5000), }); if (!response.ok) throw new Error(`pagerduty_events_http_${response.status}`); - await auditPagerDutyNotification(env, { repoFullName: params.repoFullName, dedupKey: params.dedupKey }, "completed", "triggered", { source: resolution.source }); + await auditPagerDutyNotification(env, { repoFullName: params.repoFullName, dedupKey: params.dedupKey }, "completed", PAGERDUTY_AUDIT_DETAIL_TRIGGERED, { source: resolution.source }); } catch (error) { const message = errorMessage(error); console.warn(JSON.stringify({ event: "pagerduty_trigger_failed", repo: params.repoFullName, message: message.slice(0, 200) })); @@ -251,7 +257,7 @@ export async function resolvePagerDutyIncident( signal: AbortSignal.timeout(5000), }); if (!response.ok) throw new Error(`pagerduty_events_http_${response.status}`); - await auditPagerDutyNotification(env, { repoFullName: params.repoFullName, dedupKey: params.dedupKey }, "completed", "resolved", { source: resolution.source }); + await auditPagerDutyNotification(env, { repoFullName: params.repoFullName, dedupKey: params.dedupKey }, "completed", PAGERDUTY_AUDIT_DETAIL_RESOLVED, { source: resolution.source }); } catch (error) { const message = errorMessage(error); console.warn(JSON.stringify({ event: "pagerduty_resolve_failed", repo: params.repoFullName, message: message.slice(0, 200) })); diff --git a/test/unit/notify-pagerduty.test.ts b/test/unit/notify-pagerduty.test.ts index 5158dd0032..9785a98f3e 100644 --- a/test/unit/notify-pagerduty.test.ts +++ b/test/unit/notify-pagerduty.test.ts @@ -230,6 +230,60 @@ describe("triggerPagerDutyIncident — cooldown gate (alert fatigue control #2)" expect(calls).toHaveLength(1); }); + it("REGRESSION: a recent suppression (denied/cooldown_active) row does NOT itself suppress the next page (#9695)", async () => { + const calls = stubFetch(); + const env = enabledEnv(); + const fiveMinutesAgo = new Date(Date.now() - 5 * 60 * 1000).toISOString(); + // Before #9695 this `denied` row was counted (the counter did not filter outcome/detail), so a single + // suppression self-renewed the window and the repo could never page again. + await recordAuditEvent(env, { + eventType: "external_notification.pagerduty", + actor: "loopover", + targetKey: "ops_anomaly:acme/widgets", + outcome: "denied", + detail: "cooldown_active", + metadata: {}, + createdAt: fiveMinutesAgo, + }); + await trigger(env); + expect(calls).toHaveLength(1); // it pages, because no ACTUAL page happened in the window + }); + + it("REGRESSION: a recent auto-resolve (completed/resolved) row does NOT suppress the next page (#9695)", async () => { + const calls = stubFetch(); + const env = enabledEnv(); + const fiveMinutesAgo = new Date(Date.now() - 5 * 60 * 1000).toISOString(); + // resolved shares outcome "completed" with a real page, so an outcome-only filter would wrongly count it. + await recordAuditEvent(env, { + eventType: "external_notification.pagerduty", + actor: "loopover", + targetKey: "ops_anomaly:acme/widgets", + outcome: "completed", + detail: "resolved", + metadata: {}, + createdAt: fiveMinutesAgo, + }); + await trigger(env); + expect(calls).toHaveLength(1); // a re-page for a flapping condition is not blocked by its own resolve + }); + + it("REGRESSION: a recent failed page (error) does NOT suppress its own retry (#9695)", async () => { + const calls = stubFetch(); + const env = enabledEnv(); + const fiveMinutesAgo = new Date(Date.now() - 5 * 60 * 1000).toISOString(); + await recordAuditEvent(env, { + eventType: "external_notification.pagerduty", + actor: "loopover", + targetKey: "ops_anomaly:acme/widgets", + outcome: "error", + detail: "The page failed", + metadata: {}, + createdAt: fiveMinutesAgo, + }); + await trigger(env); + expect(calls).toHaveLength(1); // nobody was paged, so the retry must go through + }); + it("REGRESSION: a recent page recorded under the pre-rebrand 'loopover' actor still suppresses a duplicate within the cooldown window", async () => { const calls = stubFetch(); const env = enabledEnv();