From c84a4be36a94906ada15115dfabde33df51145ad Mon Sep 17 00:00:00 2001
From: Justin Gordon <justin@shakacode.com>
Date: Mon, 8 Jun 2026 13:56:05 -1000
Subject: [PATCH] Filter stale Bencher alerts before reporting

---
 benchmarks/lib/bencher_report.rb         |  37 ++++--
 benchmarks/spec/bencher_report_spec.rb   | 143 ++++++++++++++++++++++-
 benchmarks/spec/track_benchmarks_spec.rb | 123 ++++++++++++++-----
 benchmarks/track_benchmarks.rb           |   9 ++
 4 files changed, 274 insertions(+), 38 deletions(-)

diff --git a/benchmarks/lib/bencher_report.rb b/benchmarks/lib/bencher_report.rb
index 12a7292302..0b0abe5957 100644
--- a/benchmarks/lib/bencher_report.rb
+++ b/benchmarks/lib/bencher_report.rb
@@ -84,7 +84,7 @@ def initialize(raw)
     raise FormatError, "report is not a JSON object (got #{raw.class})" unless raw.is_a?(Hash)
 
     @boundaries = index_boundaries(raw)
-    @alerts = parse_alerts(raw)
+    @alerts, @filtered_alerts = parse_alerts(raw).partition { |alert| current_regression_alert?(alert) }
     # Per-benchmark perf links are informational (they only decide whether a name links
     # out), so they live in a separate, fully-lenient builder — a missing field yields an
     # unlinked name, never a FormatError that would fail the job over a cosmetic link.
@@ -93,9 +93,9 @@ def initialize(raw)
 
   attr_reader :alerts
 
-  def regression?
-    !@alerts.empty?
-  end
+  def regression? = !@alerts.empty?
+
+  def filtered_alert? = !@filtered_alerts.empty?
 
   # Boundary for a benchmark+measure, or nil if absent from the report.
   def boundary(benchmark_name, measure_key)
@@ -118,9 +118,7 @@ def perf_url(benchmark_name)
   # plain name. A single benchmark missing its own uuid stays silent (a per-row cosmetic
   # miss); losing the shared context instead is a likely report-contract drift, so the
   # caller surfaces it as a ::warning:: — never a FormatError (the links are cosmetic).
-  def perf_links_unavailable?
-    @perf_urls.any_benchmarks? && !@perf_urls.context_ready?
-  end
+  def perf_links_unavailable? = @perf_urls.any_benchmarks? && !@perf_urls.context_ready?
 
   private
 
@@ -189,6 +187,31 @@ def parse_alerts(raw)
     end
   end
 
+  # rubocop:disable Metrics/CyclomaticComplexity
+  def current_regression_alert?(alert)
+    return true unless alert.benchmark
+
+    direction = { "lower" => :lower, "upper" => :upper }[normalize(alert.limit)]
+    return true unless direction
+
+    unless alert.measure
+      matching_boundaries = @boundaries.fetch(alert.benchmark, {}).values.select do |boundary|
+        threshold_side?(boundary, direction)
+      end
+      return true if matching_boundaries.empty?
+
+      return matching_boundaries.any? { |boundary| boundary.significance(direction) == :regression }
+    end
+
+    boundary = boundary(alert.benchmark, alert.measure)
+    return true unless boundary && threshold_side?(boundary, direction)
+
+    boundary.significance(direction) == :regression
+  end
+  # rubocop:enable Metrics/CyclomaticComplexity
+
+  def threshold_side?(boundary, direction) = direction == :lower ? boundary.lower_limit : boundary.upper_limit
+
   def normalize(key)
     key.to_s.downcase.gsub(/[-\s]+/, "_")
   end
diff --git a/benchmarks/spec/bencher_report_spec.rb b/benchmarks/spec/bencher_report_spec.rb
index d52d48bb63..dd5ec70ba7 100644
--- a/benchmarks/spec/bencher_report_spec.rb
+++ b/benchmarks/spec/bencher_report_spec.rb
@@ -39,6 +39,14 @@ def report_json(results: [], alerts: [])
     JSON.generate("results" => results, "alerts" => alerts)
   end
 
+  def rps_regression_result(name: "/foo: Core", value: 80.0)
+    benchmark_result(
+      name:,
+      measures: [measure_entry(slug: "rps", name: "rps", value:, baseline: 100.0,
+                               lower_limit: 90.0, upper_limit: nil)]
+    )
+  end
+
   describe ".parse / #regression?" do
     it "reports no regression when alerts is empty" do
       report = described_class.parse(report_json)
@@ -46,9 +54,12 @@ def report_json(results: [], alerts: [])
       expect(report.alerts).to be_empty
     end
 
-    it "reports a regression and exposes the active alert's benchmark/measure/side" do
+    it "reports a current regression and exposes the active alert's benchmark/measure/side" do
       report = described_class.parse(
-        report_json(alerts: [alert(benchmark: "/foo: Core", measure_slug: "rps", limit: "lower")])
+        report_json(
+          results: [[rps_regression_result]],
+          alerts: [alert(benchmark: "/foo: Core", measure_slug: "rps", limit: "lower")]
+        )
       )
       expect(report.regression?).to be(true)
       expect(report.alerts.size).to eq(1)
@@ -57,6 +68,134 @@ def report_json(results: [], alerts: [])
       expect(report.alerts.first.limit).to eq("lower")
     end
 
+    it "ignores active alerts when the current metric is not a regression" do
+      report = described_class.parse(
+        report_json(
+          results: [[rps_regression_result(value: 95.0)]],
+          alerts: [alert(benchmark: "/foo: Core", measure_slug: "rps", limit: "lower")]
+        )
+      )
+
+      expect(report.regression?).to be(false)
+      expect(report.alerts).to be_empty
+      expect(report.filtered_alert?).to be(true)
+    end
+
+    it "keeps active alerts with no benchmark name fail-safe" do
+      malformed_alert = alert(benchmark: "/foo: Core", measure_slug: "rps", limit: "lower")
+      malformed_alert.delete("benchmark")
+
+      report = described_class.parse(report_json(alerts: [malformed_alert]))
+
+      expect(report.regression?).to be(true)
+      expect(report.alerts.first.benchmark).to be_nil
+      expect(report.filtered_alert?).to be(false)
+    end
+
+    it "keeps active alerts with no matching boundary fail-safe" do
+      report = described_class.parse(
+        report_json(
+          results: [[benchmark_result(
+            name: "/foo: Core",
+            measures: [measure_entry(slug: "rps", name: "rps", value: 95.0, boundary: nil)]
+          )]],
+          alerts: [alert(benchmark: "/foo: Core", measure_slug: "rps", limit: "lower")]
+        )
+      )
+
+      expect(report.regression?).to be(true)
+      expect(report.filtered_alert?).to be(false)
+    end
+
+    it "keeps measure-less active alerts fail-safe when no boundary exists on the alert side" do
+      measureless_alert = alert(benchmark: "/foo: Core", measure_slug: "rps", limit: "lower")
+      measureless_alert["threshold"] = {}
+
+      report = described_class.parse(
+        report_json(
+          results: [[benchmark_result(
+            name: "/foo: Core",
+            measures: [measure_entry(slug: "p50-latency", name: "p50_latency", value: 5.5,
+                                     baseline: 5.0, lower_limit: nil, upper_limit: 6.0)]
+          )]],
+          alerts: [measureless_alert]
+        )
+      )
+
+      expect(report.regression?).to be(true)
+      expect(report.filtered_alert?).to be(false)
+    end
+
+    it "keeps measure-less active alerts when the benchmark has a current regression on the alert side" do
+      measureless_alert = alert(benchmark: "/foo: Core", measure_slug: "rps", limit: "lower")
+      measureless_alert["threshold"] = {}
+
+      report = described_class.parse(
+        report_json(
+          results: [[rps_regression_result]],
+          alerts: [measureless_alert]
+        )
+      )
+
+      expect(report.regression?).to be(true)
+      expect(report.alerts.first.benchmark).to eq("/foo: Core")
+      expect(report.alerts.first.measure).to be_nil
+    end
+
+    it "ignores measure-less active alerts when the benchmark has no current regression on the alert side" do
+      measureless_alert = alert(benchmark: "/foo: Core", measure_slug: "rps", limit: "lower")
+      measureless_alert["threshold"] = {}
+
+      report = described_class.parse(
+        report_json(
+          results: [[rps_regression_result(value: 95.0)]],
+          alerts: [measureless_alert]
+        )
+      )
+
+      expect(report.regression?).to be(false)
+      expect(report.alerts).to be_empty
+    end
+
+    it "ignores opposite-side improvements when matching measure-less active alerts" do
+      measureless_alert = alert(benchmark: "/foo: Core", measure_slug: "rps", limit: "lower")
+      measureless_alert["threshold"] = {}
+
+      report = described_class.parse(
+        report_json(
+          results: [[benchmark_result(
+            name: "/foo: Core",
+            measures: [
+              measure_entry(slug: "rps", name: "rps", value: 100.0,
+                            baseline: 100.0, lower_limit: 90.0, upper_limit: nil),
+              measure_entry(slug: "p50-latency", name: "p50_latency", value: 3.0,
+                            baseline: 5.0, lower_limit: nil, upper_limit: 6.0)
+            ]
+          )]],
+          alerts: [measureless_alert]
+        )
+      )
+
+      expect(report.regression?).to be(false)
+      expect(report.alerts).to be_empty
+    end
+
+    it "still reports hidden failed_pct regressions when the current value crosses its upper boundary" do
+      report = described_class.parse(
+        report_json(
+          results: [[benchmark_result(
+            name: "/foo: Core",
+            measures: [measure_entry(slug: "failed-pct", name: "failed_pct", value: 2.0,
+                                     baseline: 0.0, lower_limit: nil, upper_limit: 1.0)]
+          )]],
+          alerts: [alert(benchmark: "/foo: Core", measure_slug: "failed-pct", limit: "upper")]
+        )
+      )
+
+      expect(report.regression?).to be(true)
+      expect(report.alerts.first.measure).to eq("failed-pct")
+    end
+
     it "ignores dismissed/silenced alerts (only active counts as a regression)" do
       report = described_class.parse(
         report_json(alerts: [
diff --git a/benchmarks/spec/track_benchmarks_spec.rb b/benchmarks/spec/track_benchmarks_spec.rb
index 78e42b3c7a..b36a4b2492 100644
--- a/benchmarks/spec/track_benchmarks_spec.rb
+++ b/benchmarks/spec/track_benchmarks_spec.rb
@@ -14,17 +14,41 @@
 # mutually exclusive by design: an alert must never trigger a start-point-hash retry,
 # or a real regression would be silently re-measured against the wrong baseline.
 RSpec.describe "track_benchmarks" do
+  def result(name, measures)
+    { "benchmark" => { "name" => name }, "measures" => measures }
+  end
+
+  def rps_measure(value: 80.0)
+    {
+      "measure" => { "slug" => "rps", "name" => "rps" },
+      "metric" => { "value" => value },
+      "boundary" => { "baseline" => 100.0, "lower_limit" => 90.0, "upper_limit" => nil }
+    }
+  end
+
+  def p50_measure(value: 7.0)
+    {
+      "measure" => { "slug" => "p50-latency", "name" => "p50_latency" },
+      "metric" => { "value" => value },
+      "boundary" => { "baseline" => 5.0, "lower_limit" => nil, "upper_limit" => 6.0 }
+    }
+  end
+
+  def active_alert(benchmark: "/x", measure: "rps", limit: "lower")
+    {
+      "benchmark" => { "name" => benchmark },
+      "threshold" => { "measure" => { "slug" => measure } },
+      "metric" => { "value" => 1.0 },
+      "limit" => limit,
+      "status" => "active"
+    }
+  end
+
   def report_with_alert
     BencherReport.parse(
       JSON.generate(
-        "results" => [],
-        "alerts" => [{
-          "benchmark" => { "name" => "/x" },
-          "threshold" => { "measure" => { "slug" => "rps" } },
-          "metric" => { "value" => 1.0 },
-          "limit" => "lower",
-          "status" => "active"
-        }]
+        "results" => [[result("/x", [rps_measure])]],
+        "alerts" => [active_alert]
       )
     )
   end
@@ -51,14 +75,14 @@ def report_without_alert
     it "returns the deduped benchmark names from active alerts" do
       report = BencherReport.parse(
         JSON.generate(
-          "results" => [],
+          "results" => [[
+            result("/posts_page: Pro", [rps_measure, p50_measure]),
+            result("/other: Pro", [rps_measure])
+          ]],
           "alerts" => [
-            { "benchmark" => { "name" => "/posts_page: Pro" },
-              "threshold" => { "measure" => { "slug" => "rps" } }, "status" => "active" },
-            { "benchmark" => { "name" => "/posts_page: Pro" },
-              "threshold" => { "measure" => { "slug" => "p50-latency" } }, "status" => "active" },
-            { "benchmark" => { "name" => "/other: Pro" },
-              "threshold" => { "measure" => { "slug" => "rps" } }, "status" => "active" }
+            active_alert(benchmark: "/posts_page: Pro"),
+            active_alert(benchmark: "/posts_page: Pro", measure: "p50-latency", limit: "upper"),
+            active_alert(benchmark: "/other: Pro")
           ]
         )
       )
@@ -143,18 +167,29 @@ def report_without_alert
       it "returns deduped benchmark+measure pairs from active alerts, dropping nameless ones" do
         report = BencherReport.parse(
           JSON.generate(
-            "results" => [],
+            "results" => [[result("/x: Pro", [rps_measure])]],
             "alerts" => [
-              { "benchmark" => { "name" => "/x: Pro" },
-                "threshold" => { "measure" => { "slug" => "rps" } }, "status" => "active" },
-              { "benchmark" => { "name" => "/x: Pro" },
-                "threshold" => { "measure" => { "slug" => "rps" } }, "status" => "active" },
+              active_alert(benchmark: "/x: Pro"),
+              active_alert(benchmark: "/x: Pro"),
               { "threshold" => { "measure" => { "slug" => "rps" } }, "status" => "active" }
             ]
           )
         )
         expect(regressed_alert_pairs(report)).to eq([{ "benchmark" => "/x: Pro", "measure" => "rps" }])
       end
+
+      it "preserves benchmark-only fallback pairs when Bencher omits the alert measure slug" do
+        measureless_alert = active_alert(benchmark: "/x: Pro")
+        measureless_alert["threshold"] = {}
+        report = BencherReport.parse(
+          JSON.generate(
+            "results" => [[result("/x: Pro", [rps_measure])]],
+            "alerts" => [measureless_alert]
+          )
+        )
+
+        expect(regressed_alert_pairs(report)).to eq([{ "benchmark" => "/x: Pro", "measure" => nil }])
+      end
     end
 
     describe "#load_candidate" do
@@ -217,9 +252,8 @@ def pair(benchmark, measure)
       it "is confirmed when the same benchmark+measure re-alerts" do
         report = BencherReport.parse(
           JSON.generate(
-            "results" => [],
-            "alerts" => [{ "benchmark" => { "name" => "/x: Pro" },
-                           "threshold" => { "measure" => { "slug" => "rps" } }, "status" => "active" }]
+            "results" => [[result("/x: Pro", [rps_measure])]],
+            "alerts" => [active_alert(benchmark: "/x: Pro")]
           )
         )
         status, confirmed = confirmation_outcome(report, 1, [pair("/x: Pro", "rps")])
@@ -230,9 +264,8 @@ def pair(benchmark, measure)
       it "is cleared when a different benchmark/measure alerts than the candidate's" do
         report = BencherReport.parse(
           JSON.generate(
-            "results" => [],
-            "alerts" => [{ "benchmark" => { "name" => "/y: Pro" },
-                           "threshold" => { "measure" => { "slug" => "rps" } }, "status" => "active" }]
+            "results" => [[result("/y: Pro", [rps_measure])]],
+            "alerts" => [active_alert(benchmark: "/y: Pro")]
           )
         )
         expect(confirmation_outcome(report, 1, [pair("/x: Pro", "rps")])).to eq([:cleared, []])
@@ -242,9 +275,8 @@ def pair(benchmark, measure)
         ignored = RegressionReport::IGNORED_REGRESSION_BENCHMARKS.first
         report = BencherReport.parse(
           JSON.generate(
-            "results" => [],
-            "alerts" => [{ "benchmark" => { "name" => ignored },
-                           "threshold" => { "measure" => { "slug" => "rps" } }, "status" => "active" }]
+            "results" => [[result(ignored, [rps_measure])]],
+            "alerts" => [active_alert(benchmark: ignored)]
           )
         )
         expect(confirmation_outcome(report, 1, [pair(ignored, "rps")])).to eq([:cleared, []])
@@ -272,6 +304,39 @@ def pair(benchmark, measure)
       expect { run_bencher("branch", []) }
         .to output(/::warning::Bencher report listed benchmarks but no perf-link context/).to_stdout
     end
+
+    it "preserves the original alert exit so retry handling can run first" do
+      status = instance_double(Process::Status, exitstatus: 1)
+      report_json = JSON.generate(
+        "results" => [[result("/x", [rps_measure(value: 95.0)])]],
+        "alerts" => [active_alert]
+      )
+
+      allow(Open3).to receive(:capture3).and_return([report_json, "", status])
+      allow(File).to receive(:write).with(REPORT_JSON, report_json)
+
+      _stderr, exit_code, report = run_bencher("branch", [])
+
+      expect(exit_code).to eq(1)
+      expect(report).not_to be_regression
+      expect(report.filtered_alert?).to be(true)
+      expect(retry_without_start_point_hash?("Head Version abc123 not found", exit_code, report)).to be(true)
+    end
+
+    it "normalizes an alert exit to success after retry handling when every active alert is stale" do
+      report = BencherReport.parse(
+        JSON.generate(
+          "results" => [[result("/x", [rps_measure(value: 95.0)])]],
+          "alerts" => [active_alert]
+        )
+      )
+
+      exit_code = nil
+      expect { exit_code = normalized_bencher_exit_code(1, report) }
+        .to output(/::notice::Bencher reported only stale active alert/).to_stdout
+
+      expect(exit_code).to eq(0)
+    end
   end
 
   describe "GitHub warning annotations" do
diff --git a/benchmarks/track_benchmarks.rb b/benchmarks/track_benchmarks.rb
index 567cfa8a88..5a4bb6e35e 100755
--- a/benchmarks/track_benchmarks.rb
+++ b/benchmarks/track_benchmarks.rb
@@ -195,9 +195,17 @@ def run_bencher(branch, start_point_args)
       )
     end
   end
+
   [stderr, status.exitstatus, report]
 end
 
+def normalized_bencher_exit_code(exit_code, report)
+  return exit_code unless exit_code != 0 && report&.filtered_alert? && !regression?(report)
+
+  Github.notice("Bencher reported only stale active alert(s); no current boundary-backed regression remains.")
+  0
+end
+
 def append_step_summary(markdown)
   File.open(ENV.fetch("GITHUB_STEP_SUMMARY"), "a") { |file| file.write(markdown) }
 end
@@ -573,6 +581,7 @@ def finish_confirmation(status, confirmed_alerts, suite_name)
     # and this path only triggers when the first run had no regression.
     _retry_stderr, bencher_exit_code, report = run_bencher(branch, retry_args)
   end
+  bencher_exit_code = normalized_bencher_exit_code(bencher_exit_code, report)
 
   # Build the Markdown summary table once; the same body feeds the job summary, the
   # PR comment, and the candidate/confirmation hand-offs.