Make per-PR benchmarks opt-in and trim per-route warm-up (#4012)

.github/workflows/benchmark.yml

@@ -107,8 +107,10 @@ jobs:
         with:
           docs-only: ${{ steps.detect.outputs.docs_only }}
           previous-sha: ${{ github.event.before }}
-      # script/ci-changes-detector owns benchmark-suite selection via
-      # run_core_benchmarks / run_pro_benchmarks / run_pro_node_renderer_benchmarks.
+      # detect-changes controls only non_runtime_only (force-skip every suite) and
+      # benchmarks_changed (run the script specs below). Which suites actually run is
+      # decided by event + labels in benchmarks/generate_matrix.rb: PRs are opt-in via
+      # a benchmark* label, so change-detection no longer auto-selects suites (#4012).
       - name: Set benchmark matrices
         id: benchmark-matrices
         # Always runs. GitHub Actions evaluates `strategy.matrix` for downstream jobs at
@@ -125,9 +127,6 @@ jobs:
           BENCHMARK_PULL_REQUEST_HEAD_REPO: ${{ github.event.pull_request.head.repo.full_name || '' }}
           BENCHMARK_PULL_REQUEST_LABELS: ${{ github.event_name == 'pull_request' && toJSON(github.event.pull_request.labels.*.name) || '[]' }}
           GITHUB_REPOSITORY: ${{ github.repository }}
-          RUN_CORE_BENCHMARKS: ${{ steps.detect.outputs.run_core_benchmarks || 'false' }}
-          RUN_PRO_BENCHMARKS: ${{ steps.detect.outputs.run_pro_benchmarks || 'false' }}
-          RUN_PRO_NODE_RENDERER_BENCHMARKS: ${{ steps.detect.outputs.run_pro_node_renderer_benchmarks || 'false' }}
         run: |
           BENCHMARK_MATRIX=$(ruby benchmarks/generate_matrix.rb)
           export BENCHMARK_MATRIX
@@ -160,9 +159,10 @@ jobs:
   # Initial run of every selected suite/shard. Delegates to the reusable benchmark-suite
   # workflow in "initial" mode. Which events/labels actually select suites is decided in
   # benchmarks/generate_matrix.rb (suite_requested_by_event? / suite_selected_by_input?):
-  # push to main, workflow_dispatch, relevant test-impact changes, or same-repo PRs with
-  # suite-specific benchmark labels. On a main-push alert this no longer fails the suite —
-  # track_benchmarks.rb writes a non-fatal candidate that the gate/confirmation jobs act on.
+  # push to main, workflow_dispatch, or same-repo PRs carrying a suite-specific benchmark
+  # label (PRs are opt-in — change-detection alone no longer triggers them, see #4012). On a
+  # main-push alert this no longer fails the suite — track_benchmarks.rb writes a non-fatal
+  # candidate that the gate/confirmation jobs act on.
   benchmark:
     needs: detect-changes
     name: ${{ matrix.job_name }}

AGENTS.md

@@ -271,8 +271,8 @@ Agents should recommend PR labels based on change complexity and risk. The goal
 
 - **Default: no CI-expansion label.** For docs-only changes, focused tests, small isolated fixes, and refactors with no cross-package behavior change, rely on the standard path-based CI selection and local verification.
 - **Use `full-ci`** (or ask a maintainer to comment `+ci-run-full`) when the PR is high-risk or broad: CI workflow/detector changes, dependency or lockfile updates, package manager/Ruby/Node version changes, release/build/package publishing logic, generator output, dummy app boot/build behavior, SSR or hydration behavior, cross-cutting core Ruby changes, Pro/core boundary changes, or changes where skipped suites would leave a credible regression path.
-- **Use `benchmark`** for performance-sensitive changes: server rendering paths, Node renderer, caching, bundle generation, asset serving/precompile behavior, concurrency/pooling, or anything expected to affect throughput, latency, memory, or bundle size. `full-ci` does not trigger benchmarks; use both labels when a PR is both high-risk and performance-sensitive.
-- **Use `full-ci-no-benchmarks`** when a PR needs the broad test matrix but cannot move runtime performance — CI plumbing/detector changes, lint/RuboCop config, dependency or tool-version bookkeeping, scaffolder/tooling files. This label hard-suppresses every benchmark suite for the PR, overriding both change detection and the `benchmark` label, so a 30+ minute suite never runs (and never ships meaningless Bencher data) just because an uncategorized or spec-only file was touched. Pair it with `full-ci` for full test coverage without benchmarks. (Benchmarks still run on every push to `main`, which remains the regression backstop.)
+- **Use `benchmark`** (or a suite-specific `benchmark-core` / `benchmark-pro` / `benchmark-pro-node-renderer`) for performance-sensitive changes: server rendering paths, Node renderer, caching, bundle generation, asset serving/precompile behavior, concurrency/pooling, or anything expected to affect throughput, latency, memory, or bundle size. **Benchmarks are opt-in on PRs**: without a `benchmark*` label no suite runs, because per-PR benchmark numbers are informational only and noise-dominated on shared CI runners (the regression gate runs only on `main`). They always run on push to `main`, the regression backstop. `full-ci` does not trigger benchmarks; use both labels when a PR is both high-risk and performance-sensitive.
+- **Use `full-ci-no-benchmarks`** when a PR needs the broad test matrix but cannot move runtime performance — CI plumbing/detector changes, lint/RuboCop config, dependency or tool-version bookkeeping, scaffolder/tooling files. This label hard-suppresses every benchmark suite for the PR, overriding an explicit `benchmark*` label, so a 30+ minute suite never runs (and never ships meaningless Bencher data). Pair it with `full-ci` for full test coverage without benchmarks. (Benchmarks still run on every push to `main`, which remains the regression backstop.)
 - **Remove `full-ci` when no longer needed** with `+ci-stop-full` if the PR returns to a low-risk state after splitting or reverting broad changes.
 - **Record intentional full-CI waivers** with `+ci-skip-full [optional reason]`. This is especially important for admins: the comment creates a SHA-bound audit trail without forcing docs-only or low-risk PRs to run the full matrix.
 - In PR descriptions and handoffs, state the recommended label decision explicitly: `Labels: none`, `Labels: full-ci`, `Labels: benchmark`, `Labels: full-ci, benchmark`, or `Labels: full-ci, full-ci-no-benchmarks`, with one sentence explaining why.

benchmarks/bench.rb

@@ -106,11 +106,15 @@ def benchmark_route(route)
   # Strip optional parameters from route for URL (e.g., "(/:locale)" -> "")
   target = URI.parse("http://#{BASE_URL}#{strip_optional_params(route)}")
 
-  # Warm up server for this route
-  puts "Warming up server for #{route} with 10 requests..."
-  10.times do
+  # Warm up this route before measuring: a few priming requests trigger
+  # first-request compilation/cache population so they don't skew the run. Kept
+  # small on purpose — at ~37 routes/shard the old 10×0.5s (5s/route) warm-up
+  # dominated CI time (#4012); the 30s k6 run below is what actually loads the
+  # server, and it self-warms the remaining workers in its first fraction of a second.
+  puts "Warming up server for #{route} with 3 requests..."
+  3.times do
     server_responding?(target)
-    sleep 0.5
+    sleep 0.2
   end
   puts "Warm-up complete for #{route}"
 

benchmarks/generate_matrix.rb

@@ -16,7 +16,6 @@
     suite_prefix: "CORE",
     shard_total: 1,
     app_versions: %w[both core_only],
-    run_output: "RUN_CORE_BENCHMARKS",
     labels: %w[benchmark benchmark-core],
     app_directory: "react_on_rails/spec/dummy",
     artifact_name: "benchmark-core-results",
@@ -35,7 +34,6 @@
     suite_prefix: "PRO",
     shard_total: 2,
     app_versions: %w[both pro_only pro_rails_only],
-    run_output: "RUN_PRO_BENCHMARKS",
     labels: %w[benchmark benchmark-pro],
     app_directory: "react_on_rails_pro/spec/dummy",
     artifact_name: "benchmark-pro-results",
@@ -54,7 +52,6 @@
     suite_prefix: "PRO_NODE_RENDERER",
     shard_total: 1,
     app_versions: %w[both pro_only pro_node_renderer_only],
-    run_output: "RUN_PRO_NODE_RENDERER_BENCHMARKS",
     labels: %w[benchmark benchmark-pro benchmark-pro-node-renderer],
     app_directory: "react_on_rails_pro/spec/dummy",
     artifact_name: "benchmark-pro-node-renderer-results",
@@ -75,15 +72,15 @@
 # selecting no suites (which would skip benchmarks while CI stays green).
 VALID_APP_VERSIONS = SUITES.flat_map { |suite| suite.fetch(:app_versions) }.uniq.freeze
 
-# A PR label that forces every benchmark suite OFF for this run, even when change
-# detection, a suite-specific benchmark label, or the broad `benchmark` label
-# would otherwise select suites. This is the "full CI but skip benchmarks"
-# escape hatch, paired with the `full-ci` label (which only governs the TEST
-# matrix, never benchmarks): use it on PRs that need broad test coverage but
-# cannot move runtime performance — CI plumbing, lint/config, or tooling — so
-# Bencher does not record a meaningless run (the #3919 / #3855 class of spurious
-# runs). Honored from forks too: it only ever turns benchmarks off, so there is
-# no fork-safety concern.
+# A PR label that forces every benchmark suite OFF for this run, even when a
+# suite-specific or broad `benchmark` label would otherwise select suites (PRs are
+# already opt-in, so this is the override for an explicit benchmark label). This is
+# the "full CI but skip benchmarks" escape hatch, paired with the `full-ci` label
+# (which only governs the TEST matrix, never benchmarks): use it on PRs that carry
+# a benchmark label but cannot move runtime performance — CI plumbing, lint/config,
+# or tooling — so Bencher does not record a meaningless run (the #3919 / #3855 class
+# of spurious runs). Honored from forks too: it only ever turns benchmarks off, so
+# there is no fork-safety concern.
 BENCHMARK_SUPPRESS_LABEL = "full-ci-no-benchmarks"
 
 def truthy_env?(name)
@@ -107,12 +104,17 @@ def suite_requested_by_event?(suite, labels)
   return true if event_name == "workflow_dispatch"
   return false unless event_name == "pull_request"
 
-  # Fork PRs never run: only honor the change-detection (run_output) or label
-  # triggers when the PR's head repo is this repo. GITHUB_REPOSITORY is always set
-  # by Actions, so a blank/forked head repo can't match it.
+  # PRs are opt-in: a benchmark run on a PR is informational only (the regression
+  # gate, confirmation rerun, and issue filing are all main-push only), and
+  # single-run deltas on shared CI runners are noise-dominated, so auto-running the
+  # full ~20-min/shard suite on every code change cost far more than it informed
+  # (#4012). Require an explicit benchmark* label instead. Change-detection no
+  # longer selects suites on PRs — only the event and labels do.
+  # Fork PRs never run: their labels aren't trusted. GITHUB_REPOSITORY is always
+  # set by Actions, so a blank/forked head repo can't match it.
   return false unless ENV.fetch("BENCHMARK_PULL_REQUEST_HEAD_REPO", "") == ENV.fetch("GITHUB_REPOSITORY")
 
-  truthy_env?(suite.fetch(:run_output)) || suite.fetch(:labels).intersect?(labels)
+  suite.fetch(:labels).intersect?(labels)
 end
 
 def suite_selected_by_input?(suite)

benchmarks/spec/benchmark_matrix_spec.rb

@@ -51,14 +51,16 @@ def suite_ids_for(env)
              )).to eq(%w[core pro pro pro-node-renderer])
     end
 
-    it "honors a suite-specific run_output even without labels or push" do
-      # RUN_PRO_BENCHMARKS gates only the Pro suite; core/node-renderer stay off.
+    it "does not select suites on a PR from change-detection alone (label opt-in, #4012)" do
+      # RUN_PRO_BENCHMARKS used to auto-trigger the Pro suite on a same-repo PR.
+      # PRs are now opt-in via a benchmark* label, so change-detection env is
+      # ignored and nothing is selected.
       expect(suite_ids_for(
                "BENCHMARK_EVENT_NAME" => "pull_request",
                "BENCHMARK_PULL_REQUEST_HEAD_REPO" => "shakacode/react_on_rails",
                "GITHUB_REPOSITORY" => "shakacode/react_on_rails",
                "RUN_PRO_BENCHMARKS" => "true"
-             )).to eq(%w[pro pro])
+             )).to eq(["none"])
     end
 
     it "selects suites by label intersection on a same-repo PR" do