feat(review): send a PagerDuty resolve event once ops anomalies clear

src/review/ops-wire.ts

@@ -136,6 +136,46 @@ const REVIEW_FAILURE_BURST_THRESHOLD = 3;
  *  same-tick anomaly to alert on. */
 const BYOK_USAGE_WINDOW_HOURS = 24;
 
+// ── Previous-tick anomaly state (#8903) — so a cleared anomaly can auto-resolve its PagerDuty incident ──────
+
+/** system_flags (migration 0054) key for "did `repoFullName` have anomalies on the PREVIOUS runOpsAlerts
+ *  tick?" -- reuses the SAME generic key/value table outcomes-wire.ts's readUntrustworthyRuleCodes /
+ *  writeUntrustworthyRuleCodes already do for an identical "cheap cron-tick state, no new migration" shape,
+ *  and the SAME `<family>:<scope>` key convention as that table's holdonly:/closehold: families. */
+function opsAnomalyActiveFlagKey(repoFullName: string): string {
+  return `ops_anomaly_active:${repoFullName}`;
+}
+
+/** Did `repoFullName` have anomalies on the PREVIOUS tick? FAIL-SAFE: a read error or missing row degrades
+ *  to `false` ("not previously active") so a D1 blip can never spuriously fire a PagerDuty resolve for an
+ *  incident that was never actually triggered. */
+async function readOpsAnomalyWasActive(env: Env, repoFullName: string): Promise<boolean> {
+  try {
+    const row = await env.DB.prepare("SELECT value FROM system_flags WHERE key = ?").bind(opsAnomalyActiveFlagKey(repoFullName)).first<{ value: string }>();
+    return row?.value === "1";
+  } catch {
+    return false;
+  }
+}
+
+/** Persist whether `repoFullName` has anomalies on THIS tick, read back by {@link readOpsAnomalyWasActive} on
+ *  the next one. Best-effort (mirrors writeUntrustworthyRuleCodes): a write failure is swallowed -- the next
+ *  tick's read simply serves the last successfully-written value, and this tick's write is retried then. */
+async function writeOpsAnomalyActive(env: Env, repoFullName: string, active: boolean): Promise<void> {
+  const key = opsAnomalyActiveFlagKey(repoFullName);
+  if (active) {
+    await env.DB.prepare("INSERT OR REPLACE INTO system_flags (key, value, updated_at) VALUES (?, '1', CURRENT_TIMESTAMP)")
+      .bind(key)
+      .run()
+      .catch(() => undefined);
+  } else {
+    await env.DB.prepare("DELETE FROM system_flags WHERE key = ?")
+      .bind(key)
+      .run()
+      .catch(() => undefined);
+  }
+}
+
 /** One repo's outcome reports + the repo it covers — the input to the pure anomaly detector. `reviewBurst` and
  *  `reviewFailureBurst` are optional so existing snapshot-fixture tests need not be touched; absent/null means
  *  "not computed", not "healthy" -- the caller (runOpsAlerts/computeOpsStats) always populates both today. */
@@ -282,7 +322,18 @@ export async function runOpsAlerts(env: Env): Promise<Record<string, string[]>>
           findHottestInconclusiveReviewTargetForRepo(env, repoFullName, reviewBurstSinceIso),
         ]);
         const anomalies = detectOutcomeAnomalies({ repoFullName, gatePrecision, calibration, reviewBurst, reviewFailureBurst });
-        if (anomalies.length === 0) continue;
+        if (anomalies.length === 0) {
+          // #8903: the repo is healthy THIS tick -- if it had anomalies LAST tick, close the loop by sending a
+          // matching PagerDuty resolve for the same dedupKey (triggerPagerDutyIncident itself is the no-op
+          // when PagerDuty isn't configured, exactly like the trigger call below). State tracking runs
+          // unconditionally (not gated on the PagerDuty flag) so enabling/disabling PagerDuty mid-incident can
+          // never desync from the actual anomaly history.
+          if (await readOpsAnomalyWasActive(env, repoFullName)) {
+            await writeOpsAnomalyActive(env, repoFullName, false);
+            await triggerPagerDutyIncident(env, { repoFullName, dedupKey: `ops_anomaly:${repoFullName}`, eventAction: "resolve" });
+          }
+          continue;
+        }
         found[repoFullName] = anomalies;
         // Structured log = loopover's notify path (no Discord/operator webhook exists) AND the Sentry path
         // (level:"error" + an `event` field reaches forwardStructuredLogToSentry). One line per repo.
@@ -301,9 +352,9 @@ export async function runOpsAlerts(env: Env): Promise<Record<string, string[]>>
         // comment for why alert fatigue needed both controls, not just PagerDuty's own dedup_key. Awaited (not
         // fire-and-forget) so a page failure is captured within THIS tick's own error handling, not orphaned
         // after runOpsAlerts has already returned -- triggerPagerDutyIncident itself never throws and bounds
-        // its own HTTP call to a 5s timeout, so this cannot hang the scan. This does not yet send a matching
-        // "resolve" event once anomalies clear (would need tracking previous-tick state) -- an operator
-        // currently resolves the incident manually once the underlying condition is fixed.
+        // its own HTTP call to a 5s timeout, so this cannot hang the scan. #8903: a matching "resolve" event
+        // fires automatically on whichever future tick finds this repo healthy again -- see the
+        // readOpsAnomalyWasActive branch above -- so an operator no longer resolves the incident by hand.
         const worst = worstAnomaly(anomalies);
         await triggerPagerDutyIncident(env, {
           repoFullName,
@@ -312,6 +363,7 @@ export async function runOpsAlerts(env: Env): Promise<Record<string, string[]>>
           dedupKey: `ops_anomaly:${repoFullName}`,
           customDetails: { anomalies },
         });
+        await writeOpsAnomalyActive(env, repoFullName, true);
         // #ops-anomaly-metric: Prometheus counterpart to the log line above so a self-host operator can alert on
         // /metrics instead of grepping Workers Logs. Scoped to reviewBurst/reviewFailureBurst -- the two anomalies
         // this module exists to catch fast (#orb-ci-stuck-repeat / #review-burst-blind-spot) -- rather than every

src/services/notify-pagerduty.ts

@@ -94,6 +94,26 @@ export function resolvePagerDutyRoutingKey(env: Env, repoFullName: string): Page
  *  concept (#5119) instead of a PagerDuty-only copy. */
 export type PagerDutySeverity = LoopoverSeverity;
 
+/** {@link triggerPagerDutyIncident}'s params. A discriminated union on `eventAction` (mirrors this file's
+ *  own {@link PagerDutyRoutingResolution} union style): a `trigger` (the default, so every existing call
+ *  site needs no change) carries the incident's summary/severity/details; a `resolve` (#8903 -- closing
+ *  the incident once the underlying anomaly clears) carries neither -- PagerDuty's Events API v2 only
+ *  requires `payload` for `trigger`, and there is no "worst anomaly" to summarize once anomalies are gone. */
+export type PagerDutyIncidentParams =
+  | {
+      repoFullName: string;
+      dedupKey: string;
+      eventAction?: "trigger";
+      summary: string;
+      severity: PagerDutySeverity;
+      customDetails?: Record<string, unknown> | undefined;
+    }
+  | {
+      repoFullName: string;
+      dedupKey: string;
+      eventAction: "resolve";
+    };
+
 /** Resolve the minimum severity that pages for `repoFullName`: per-repo map entry, else the global override,
  *  else {@link DEFAULT_MIN_SEVERITY} — the quietest safe default, so an operator who never touches these vars
  *  still only gets paged for active-incident-grade conditions, never routine calibration nudges. Delegates to
@@ -148,16 +168,8 @@ async function auditPagerDutyNotification(
  *  a below-threshold/cooldown-suppressed page are audited as `denied` so they're discoverable, while the
  *  common "not opted in" case stays silent (no audit-log noise for every repo that never configured
  *  PagerDuty). */
-export async function triggerPagerDutyIncident(
-  env: Env,
-  params: {
-    repoFullName: string;
-    summary: string;
-    severity: PagerDutySeverity;
-    dedupKey: string;
-    customDetails?: Record<string, unknown> | undefined;
-  },
-): Promise<void> {
+export async function triggerPagerDutyIncident(env: Env, params: PagerDutyIncidentParams): Promise<void> {
+  const eventAction = params.eventAction ?? "trigger";
   const resolution = resolvePagerDutyRoutingKey(env, params.repoFullName);
   if (resolution.status === "disabled") {
     if (resolution.reason !== "flag_off") {
@@ -166,29 +178,35 @@ export async function triggerPagerDutyIncident(
     return;
   }
 
-  const minSeverity = resolvePagerDutyMinSeverity(env, params.repoFullName);
-  if (!meetsSeverityThreshold(params.severity, minSeverity)) {
-    await auditPagerDutyNotification(env, { repoFullName: params.repoFullName, dedupKey: params.dedupKey }, "denied", "below_min_severity", {
-      severity: params.severity,
-      minSeverity,
-    });
-    return;
-  }
+  // The two alert-fatigue controls (min-severity floor, repeat-page cooldown) exist only to stop a PAGE from
+  // crying wolf -- neither applies to a resolve. Gating a resolve behind "was a page sent for this dedupKey
+  // recently" would be actively wrong: it would suppress the exact resolve meant to follow the trigger that
+  // just primed the cooldown window.
+  if (params.eventAction !== "resolve") {
+    const minSeverity = resolvePagerDutyMinSeverity(env, params.repoFullName);
+    if (!meetsSeverityThreshold(params.severity, minSeverity)) {
+      await auditPagerDutyNotification(env, { repoFullName: params.repoFullName, dedupKey: params.dedupKey }, "denied", "below_min_severity", {
+        severity: params.severity,
+        minSeverity,
+      });
+      return;
+    }
 
-  const cooldownMinutes = resolvePagerDutyCooldownMinutes(env, params.repoFullName);
-  const cooldownSinceIso = new Date(Date.now() - cooldownMinutes * 60 * 1000).toISOString();
-  // Also count the pre-rebrand "gittensory" actor: a page recorded under the OLD actor value just before this
-  // rebrand deployed must still suppress a duplicate page after it, for as long as the configured cooldown
-  // 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),
-  ]);
-  const recentPages = recentPagesNewActor + recentPagesLegacyActor;
-  if (recentPages > 0) {
-    await auditPagerDutyNotification(env, { repoFullName: params.repoFullName, dedupKey: params.dedupKey }, "denied", "cooldown_active", { cooldownMinutes });
-    return;
+    const cooldownMinutes = resolvePagerDutyCooldownMinutes(env, params.repoFullName);
+    const cooldownSinceIso = new Date(Date.now() - cooldownMinutes * 60 * 1000).toISOString();
+    // Also count the pre-rebrand "gittensory" actor: a page recorded under the OLD actor value just before this
+    // rebrand deployed must still suppress a duplicate page after it, for as long as the configured cooldown
+    // 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),
+    ]);
+    const recentPages = recentPagesNewActor + recentPagesLegacyActor;
+    if (recentPages > 0) {
+      await auditPagerDutyNotification(env, { repoFullName: params.repoFullName, dedupKey: params.dedupKey }, "denied", "cooldown_active", { cooldownMinutes });
+      return;
+    }
   }
 
   try {
@@ -197,21 +215,26 @@ export async function triggerPagerDutyIncident(
       headers: { "content-type": "application/json" },
       body: JSON.stringify({
         routing_key: resolution.routingKey,
-        event_action: "trigger",
+        event_action: eventAction,
         dedup_key: params.dedupKey,
-        payload: {
-          summary: params.summary.slice(0, 1024),
-          source: "loopover",
-          severity: params.severity,
-          timestamp: new Date().toISOString(),
-          component: params.repoFullName,
-          custom_details: params.customDetails,
-        },
+        payload:
+          params.eventAction === "resolve"
+            ? undefined
+            : {
+                summary: params.summary.slice(0, 1024),
+                source: "loopover",
+                severity: params.severity,
+                timestamp: new Date().toISOString(),
+                component: params.repoFullName,
+                custom_details: params.customDetails,
+              },
       }),
       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", eventAction === "resolve" ? "resolved" : "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) }));

test/unit/notify-pagerduty.test.ts

@@ -53,6 +53,15 @@ function trigger(env: Env, over: Partial<{ repoFullName: string; summary: string
   });
 }
 
+function resolve(env: Env, over: Partial<{ repoFullName: string; dedupKey: string }> = {}): Promise<void> {
+  return triggerPagerDutyIncident(env, {
+    repoFullName: "acme/widgets",
+    dedupKey: "ops_anomaly:acme/widgets",
+    eventAction: "resolve",
+    ...over,
+  });
+}
+
 describe("isPagerDutyEnabled", () => {
   it("accepts the codebase-standard truthy strings, case-insensitively", () => {
     for (const value of ["1", "true", "YES", "On"]) expect(isPagerDutyEnabled({ LOOPOVER_ENABLE_PAGERDUTY: value })).toBe(true);
@@ -284,3 +293,54 @@ describe("triggerPagerDutyIncident — HTTP delivery", () => {
     warn.mockRestore();
   });
 });
+
+// #8903: `eventAction: "resolve"` closes the incident once the underlying anomaly clears.
+describe("triggerPagerDutyIncident — eventAction: resolve (#8903)", () => {
+  it("still respects the flag/routing gate: flag off → no fetch and no audit row", async () => {
+    const calls = stubFetch();
+    const env = withEnv();
+    await resolve(env);
+    expect(calls).toEqual([]);
+    expect(await pagerDutyAudit(env)).toEqual([]);
+  });
+
+  it("still respects the flag/routing gate: flag on, no routing key resolves → no fetch, audited denied", async () => {
+    const calls = stubFetch();
+    const env = withEnv({ LOOPOVER_ENABLE_PAGERDUTY: "1" });
+    await resolve(env);
+    expect(calls).toEqual([]);
+    expect(await pagerDutyAudit(env)).toEqual([expect.objectContaining({ outcome: "denied", detail: "missing_global_key" })]);
+  });
+
+  it("bypasses the min-severity gate entirely — fires even below the default error floor (there is no severity to gate on)", async () => {
+    const calls = stubFetch();
+    await resolve(enabledEnv());
+    expect(calls).toHaveLength(1);
+  });
+
+  it("bypasses the cooldown gate entirely — fires right after the trigger that primed the SAME dedupKey's cooldown window", async () => {
+    const calls = stubFetch();
+    const env = enabledEnv();
+    await trigger(env);
+    await resolve(env);
+    expect(calls).toHaveLength(2);
+    expect(calls[1]?.body.event_action).toBe("resolve");
+  });
+
+  it("posts event_action: resolve with no payload key, and audits detail: resolved on success", async () => {
+    const calls = stubFetch(202);
+    const env = enabledEnv();
+    await resolve(env);
+    expect(calls).toHaveLength(1);
+    expect(calls[0]?.body).toEqual({ routing_key: VALID_KEY, event_action: "resolve", dedup_key: "ops_anomaly:acme/widgets" });
+    expect(Object.prototype.hasOwnProperty.call(calls[0]!.body, "payload")).toBe(false);
+    expect(await pagerDutyAudit(env)).toEqual([expect.objectContaining({ outcome: "completed", detail: "resolved" })]);
+  });
+
+  it("a non-ok response is audited as an error and never throws", async () => {
+    stubFetch(500);
+    const env = enabledEnv();
+    await expect(resolve(env)).resolves.toBeUndefined();
+    expect(await pagerDutyAudit(env)).toEqual([expect.objectContaining({ outcome: "error" })]);
+  });
+});