fix: adminnotify observability — money-critical rows sort first, flood-cap suppression surfaced to operator, stale coordination doc fixed

- notifications priority ordering: money-critical reasons (webhooks, sweeps, refunds, gift-card, manual-refund failures) above routine - admin notifications page exposes the flood-cap suppressed count - adminnotify.go contract doc: removed stale 2FA reissue-fail site, current insert-site list Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai> Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)
2026-08-22 00:34:51 +01:00
parent e9b34d0ad6
commit 049c361e16
16 changed files with 1048 additions and 314 deletions
@@ -2,39 +2,47 @@
 // notification centre (admin_notifications). That table is the single
 // operator's ONLY pager for money events, so every site that inserts an
 // operator-facing alert must bound its unacknowledged queue — a hostile flood
-// (attacker-registered accounts triggering refresh_token_reuse / reissue-fail /
-// webhook alerts) must not be able to bury the notification centre under rows
-// the operator can never work through.
+// (attacker-registered accounts triggering refresh_token_reuse / webhook
+// alerts) must not be able to bury the notification centre under rows the
+// operator can never work through.
 //
 // Coordination contract (Round 2 Loop B finding 1): the cap is applied
 // atomically at every insert site in this codebase:
 //
-//   - handlers/payments/twofa.go — the reissue-fail alert (per-issue capped,
-//     see finding 2; NOT globally capped).
 //   - auth/jwt.go VerifyRefreshToken — the 'refresh_token_reuse' alert.
-//   - handlers/webhooks/square.go — the three critical_payment_log inserts
+//   - handlers/webhooks/square.go — the 'critical_payment_log' inserts
 //     (dispute, booking, unknown-event, orphan-replay).
 //   - handlers/user/account.go InsertSquareErasureCriticalNotification.
+//   - handlers/payments/sweep.go insertCriticalPaymentNotification.
+//   - handlers/scheduling/time-blockers.go InsertSquareCleanupCriticalNotification
+//     and the deposit-deadline cleanup insert.
+//   - internal/jobs/cleanup.go ScanCriticalPaymentLogs.
+//   - handlers/scheduling/scheduled-cleanup.go — the '1_week_no_pay',
+//     '1_month_no_pay' and 'default_hours_changed' inserts.
+//   - handlers/bookings/bookings.go — the 'new_booking' / 'pending_booking' /
+//     'cancelled_booking' inserts.
+//   - handlers/bookings/manage.go — the 'cancelled_booking' / 'edit_requested'
+//     inserts.
+//   - handlers/payments/refunds.go — the 'refund_failed' insert.
 //
-// Sites owned by OTHER agents (coordination notes):
-//
-//   - handlers/payments/sweep.go insertCriticalPaymentNotification — NOW capped
-//     (same atomic fold + pre-check as every other site).
-//   - handlers/scheduling/time-blockers.go insertSquareCleanupCriticalNotification
-//     and internal/jobs/cleanup.go ScanCriticalPaymentLogs — both NOW capped
-//     (same atomic fold + pre-check as every other site).
-//   - main.go has NO admin_notifications insert sites (it only mounts the
-//     notification read/ack routes), so nothing to cap there.
+// main.go has NO admin_notifications insert sites (it only mounts the
+// notification read/ack routes), so nothing to cap there.
 //
 // Every site folds the cap INTO its INSERT (a conditional
 // `INSERT ... SELECT ... WHERE (SELECT COUNT(*) ...) < $cap`) so the
 // count-then-insert is ATOMIC — closing the TOCTOU where two concurrent
 // inserts both read a below-cap count and overshoot together (finding 2).
+// Every site ALSO pre-checks CriticalLogsCapExceeded to log the suppression;
+// that pre-check is the single choke point that records the suppression in
+// admin_notification_suppressions, so the operator-facing "suppressed this
+// cycle" count on GET /api/admin/notifications stays accurate without any
+// insert site having to change.
 package adminnotify

 import (
 	"context"
 	"log"
+	"time"

 	"crussell/db"
 )
@@ -58,6 +66,11 @@ const MaxUnacknowledgedCriticalLogs = 100
 // a money alert is never dropped because the count query failed (the insert's
 // own atomic cap condition below still guards the row in that case — the
 // pre-check only decides whether to log a suppression).
+//
+// At the cap the suppression is ALSO recorded in admin_notification_suppressions
+// (recordSuppression) so the admin notifications page can surface how many
+// alerts were dropped. The record write is best-effort too — a failure only
+// loses the counter, never the boolean decision.
 func CriticalLogsCapExceeded(ctx context.Context, q db.Querier, reason string) bool {
 	var n int
 	err := q.QueryRow(ctx, `
@@ -68,5 +81,66 @@ func CriticalLogsCapExceeded(ctx context.Context, q db.Querier, reason string) b
 		log.Printf("adminnotify: failed to count unacknowledged %s admin notifications: %v", reason, err)
 		return false
 	}
-	return n >= MaxUnacknowledgedCriticalLogs
+	if n >= MaxUnacknowledgedCriticalLogs {
+		recordSuppression(ctx, q, reason)
+		return true
+	}
+	return false
+}
+
+// recordSuppression upserts the per-reason flood-cap suppression counter
+// (admin_notification_suppressions). Every insert site funnels its pre-check
+// through CriticalLogsCapExceeded, so a suppression is recorded exactly once
+// per dropped alert, and a running suppressed_count accumulates while the
+// reason's unacknowledged queue stays at the cap. Best-effort: a failure is
+// logged, never propagated — the alert is already being dropped at the cap and
+// the operator-facing counter is informational, not load-bearing.
+func recordSuppression(ctx context.Context, q db.Querier, reason string) {
+	if _, err := q.Exec(ctx, `
+		INSERT INTO admin_notification_suppressions (reason, suppressed_count, first_suppressed_at, last_suppressed_at)
+		VALUES ($1::admin_notification_reason, 1, NOW(), NOW())
+		ON CONFLICT (reason) DO UPDATE SET
+			suppressed_count = admin_notification_suppressions.suppressed_count + 1,
+			last_suppressed_at = NOW()
+	`, reason); err != nil {
+		log.Printf("adminnotify: failed to record flood-cap suppression for %s: %v", reason, err)
+	}
+}
+
+// Suppression is the per-reason flood-cap suppression counter surfaced to the
+// operator: how many alerts were dropped for that reason while its
+// unacknowledged queue sat at MaxUnacknowledgedCriticalLogs.
+type Suppression struct {
+	Reason           string
+	SuppressedCount  int
+	LastSuppressedAt time.Time
+}
+
+// ActiveSuppressions returns the per-reason flood-cap suppressions whose
+// underlying unacknowledged queue is STILL at the cap — i.e. the operator has
+// not yet worked it down ("this cycle"). The admin notifications handler sums
+// these into the response's suppressed count; the count naturally resets once
+// the operator acknowledges the queue back below the cap. Best-effort: the
+// caller must treat a non-nil error as "no suppression data available".
+func ActiveSuppressions(ctx context.Context, q db.Querier) ([]Suppression, error) {
+	rows, err := q.Query(ctx, `
+		SELECT s.reason, s.suppressed_count, s.last_suppressed_at
+		FROM admin_notification_suppressions s
+		WHERE (SELECT COUNT(*) FROM admin_notifications an
+		       WHERE an.reason = s.reason AND an.acknowledged_at IS NULL) >= $1
+		ORDER BY s.last_suppressed_at DESC
+	`, MaxUnacknowledgedCriticalLogs)
+	if err != nil {
+		return nil, err
+	}
+	defer rows.Close()
+	var out []Suppression
+	for rows.Next() {
+		var s Suppression
+		if err := rows.Scan(&s.Reason, &s.SuppressedCount, &s.LastSuppressedAt); err != nil {
+			return nil, err
+		}
+		out = append(out, s)
+	}
+	return out, rows.Err()
 }
@@ -35,6 +35,7 @@ func TestCriticalLogsCapExceeded(t *testing.T) {
 	ctx := context.Background()
 	t.Cleanup(func() {
 		_, _ = db.Conn.Exec(ctx, "DELETE FROM admin_notifications WHERE reason = 'critical_payment_log'")
+		_, _ = db.Conn.Exec(ctx, "DELETE FROM admin_notification_suppressions WHERE reason = 'critical_payment_log'")
 	})

 	if CriticalLogsCapExceeded(ctx, db.Conn, "critical_payment_log") {
@@ -70,3 +71,72 @@ func TestCriticalLogsCapExceeded(t *testing.T) {
 		t.Fatal("expected acknowledging the queue to re-arm inserts")
 	}
 }
+
+// TestCriticalLogsCapExceeded_RecordsSuppression pins FIX 3a: hitting the
+// flood cap records the suppression in admin_notification_suppressions (the
+// operator-facing "suppressed this cycle" counter) with a running count, and
+// ActiveSuppressions reports it only while the underlying queue is still at the
+// cap — acknowledging the queue down resets the visible count.
+func TestCriticalLogsCapExceeded_RecordsSuppression(t *testing.T) {
+	ctx := context.Background()
+	t.Cleanup(func() {
+		_, _ = db.Conn.Exec(ctx, "DELETE FROM admin_notifications WHERE reason = 'critical_payment_log'")
+		_, _ = db.Conn.Exec(ctx, "DELETE FROM admin_notification_suppressions WHERE reason = 'critical_payment_log'")
+	})
+
+	// Below the cap no suppression is recorded.
+	for i := 0; i < MaxUnacknowledgedCriticalLogs-1; i++ {
+		if _, err := db.Conn.Exec(ctx, `
+			INSERT INTO admin_notifications (reason, created_at)
+			VALUES ('critical_payment_log', NOW())
+		`); err != nil {
+			t.Fatalf("failed to insert row %d: %v", i, err)
+		}
+	}
+	if supps, err := ActiveSuppressions(ctx, db.Conn); err != nil {
+		t.Fatalf("ActiveSuppressions failed below the cap: %v", err)
+	} else if len(supps) != 0 {
+		t.Fatalf("expected no suppression below the cap, got %d", len(supps))
+	}
+
+	// Fill to the cap, then hit it twice: each suppressed alert is recorded.
+	if _, err := db.Conn.Exec(ctx, `
+		INSERT INTO admin_notifications (reason, created_at)
+		VALUES ('critical_payment_log', NOW())
+	`); err != nil {
+		t.Fatalf("failed to insert the cap row: %v", err)
+	}
+	if !CriticalLogsCapExceeded(ctx, db.Conn, "critical_payment_log") {
+		t.Fatal("expected the queue to be at the cap")
+	}
+	if !CriticalLogsCapExceeded(ctx, db.Conn, "critical_payment_log") {
+		t.Fatal("expected the queue to stay at the cap")
+	}
+
+	supps, err := ActiveSuppressions(ctx, db.Conn)
+	if err != nil {
+		t.Fatalf("ActiveSuppressions failed: %v", err)
+	}
+	if len(supps) != 1 {
+		t.Fatalf("expected exactly 1 suppression record, got %d", len(supps))
+	}
+	if supps[0].Reason != "critical_payment_log" {
+		t.Errorf("expected reason critical_payment_log, got %s", supps[0].Reason)
+	}
+	if supps[0].SuppressedCount != 2 {
+		t.Errorf("expected suppressed_count 2, got %d", supps[0].SuppressedCount)
+	}
+
+	// Acknowledging the queue below the cap makes the suppression inactive, so
+	// the "this cycle" count resets.
+	if _, err := db.Conn.Exec(ctx, "UPDATE admin_notifications SET acknowledged_at = NOW() WHERE reason = 'critical_payment_log'"); err != nil {
+		t.Fatalf("failed to acknowledge the queue: %v", err)
+	}
+	supps, err = ActiveSuppressions(ctx, db.Conn)
+	if err != nil {
+		t.Fatalf("ActiveSuppressions failed after acknowledge: %v", err)
+	}
+	if len(supps) != 0 {
+		t.Errorf("expected the suppression to reset once the queue is below the cap, got %d", len(supps))
+	}
+}