From 8afb9e1f677555d1e5a9b12498fe942cf79f5720 Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Sun, 27 Sep 2026 20:44:17 +0200 Subject: [PATCH 1/4] ci: take the ns-scale micro-benchmarks out of CodSpeed, and fix the #699 follow-ups (celeris#725) CodSpeed: the walltime run no longer measures ./internal/... (wakefd's BenchmarkFD* and BenchmarkSignal*) or BenchmarkInternH2HeaderName. Their test binaries were byte-identical at every main commit since CodSpeed was set up and still raised flags. go-runner v1.3.0 passes only -bench, -benchtime and the package list to go test (-skip and -tags are dropped), so the package list drops ./internal/... and the -bench pattern '^Benchmark([^I]|I[^n])' drops the one BenchmarkIn* benchmark. The trigger paths follow the set: internal/ctxkit is the only internal package it executes. The header now says how to read a flag (informational; layout and runner effects; check binary identity, then .github/scripts/bench-ab.sh, an interleaved A/B with an A/A floor), that the fork-PR approval policy is the security boundary for the macro runner (not the job's if:), all three prerequisites (done 2026-09-26), and the recounted budget. The push and pull_request path lists are one YAML anchor. Coverage: use_oidc: true (codecov-action skips OIDC for forks itself), a step that fails when the package set drifts from ci.yml's unit job, and a note on fail_ci_if_error. codecov.yml drops the adaptive component, which could only be empty. .coderabbit.yaml, CONTRIBUTING.md and GOVERNANCE.md state the thread-resolution rule that CodeRabbit's threads fall under. Fixes #725 --- .coderabbit.yaml | 13 ++- .github/scripts/bench-ab.sh | 87 ++++++++++++++ .github/workflows/codspeed.yml | 168 ++++++++++++++++------------ .github/workflows/test-coverage.yml | 59 ++++++++-- CONTRIBUTING.md | 15 ++- GOVERNANCE.md | 8 +- codecov.yml | 9 +- 7 files changed, 266 insertions(+), 93 deletions(-) create mode 100755 .github/scripts/bench-ab.sh diff --git a/.coderabbit.yaml b/.coderabbit.yaml index 0933dab7..ba4d655e 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -4,10 +4,17 @@ # Reference: https://docs.coderabbit.ai/reference/configuration # `@coderabbitai configuration` on any pull request prints the resolved config. # -# CodeRabbit is ADVISORY. It never requests changes or approves, it is not a +# CodeRabbit is ADVISORY: it never requests changes or approves, it is not a # required check, and its comments are input to the human review, not a -# verdict. The merge rule stays GOVERNANCE.md's: green required checks and a -# code-owner approval. +# verdict. It can still hold a merge until someone answers it: every inline +# comment opens a review thread, and the `main` ruleset requires every thread +# to be resolved (required_review_thread_resolution). CodeRabbit resolves its +# own thread when a later push fixes the point. A thread you disagree with +# (a false positive, a suggestion you are not taking) needs a reply that says +# why and then "Resolve conversation", or `@coderabbitai resolve`, which +# resolves all of CodeRabbit's threads on the pull request. The merge rule is +# GOVERNANCE.md's: green required checks, every review thread resolved, and a +# code-owner approval (celeris#725). language: en-US diff --git a/.github/scripts/bench-ab.sh b/.github/scripts/bench-ab.sh new file mode 100755 index 00000000..0ca54648 --- /dev/null +++ b/.github/scripts/bench-ab.sh @@ -0,0 +1,87 @@ +#!/usr/bin/env bash +# bench-ab.sh: settle a benchmark flag (a CodSpeed regression, or any "is this +# slower?") with an interleaved A/B plus an A/A floor, one process per +# observation (celeris#725). +# +# usage: .github/scripts/bench-ab.sh [rounds] +# e.g. .github/scripts/bench-ab.sh origin/main HEAD ./middleware \ +# '^BenchmarkChain(Baseline|MinimalAPI|PreRoutingOnly)$' 20 +# +# What it does: +# 1. Builds 's test binary at (arm A) and at +# (arm B), each in a temporary worktree, with -trimpath, and says whether +# the two binaries are byte-identical. If they are, any difference a +# benchmark service reported between them came from its machine. +# 2. Copies A to A2: a byte-identical third arm, whose spread against A is +# the noise floor of this machine. +# 3. Runs rounds (default 20). A round runs A, B and A2 once each, +# a fresh process per run (-test.count=1), with the arm order rotated +# every round so that no arm always runs first or last. +# 4. Prints benchstat for A vs B and for A vs A2. +# +# Reading it: a B-vs-A delta that is not larger than the A-vs-A2 delta is not +# a change. B/op and allocs/op do not depend on the machine: if they moved, +# the code moved. +# +# The numbers describe the machine they ran on. A build for your laptop has +# its own code layout, so it answers "did the code on this path get slower", +# not "why did the CodSpeed arm64 runner move". For the runner's layout, run +# this on an arm64 Linux host. Run it on a quiet machine: nothing else busy, +# on AC power. +# +# Environment: BENCHTIME (default 1s, as CodSpeed runs it), OUT (a directory +# to keep the raw outputs in; default a temporary one, removed on exit). +# Needs benchstat: go install golang.org/x/perf/cmd/benchstat@latest +set -euo pipefail + +if [ "$#" -lt 4 ]; then + sed -n '2,12p' "$0" | sed 's/^# \{0,1\}//' + exit 2 +fi +base=$1 head=$2 pkg=$3 re=$4 rounds=${5:-20} +benchtime=${BENCHTIME:-1s} +command -v benchstat > /dev/null || { echo "benchstat not found: go install golang.org/x/perf/cmd/benchstat@latest" >&2; exit 2; } + +repo=$(git rev-parse --show-toplevel) +tmp=$(mktemp -d) +cleanup() { + git -C "$repo" worktree remove --force "$tmp/a" 2> /dev/null || true + git -C "$repo" worktree remove --force "$tmp/b" 2> /dev/null || true + rm -rf "$tmp" +} +trap cleanup EXIT +out=${OUT:-$tmp/out} +mkdir -p "$out" + +git -C "$repo" worktree add --detach --quiet "$tmp/a" "$base" +git -C "$repo" worktree add --detach --quiet "$tmp/b" "$head" +(cd "$tmp/a" && go test -trimpath -c -o "$tmp/A.test" "$pkg") +(cd "$tmp/b" && go test -trimpath -c -o "$tmp/B.test" "$pkg") +cp "$tmp/A.test" "$tmp/A2.test" +if cmp -s "$tmp/A.test" "$tmp/B.test"; then + echo "A ($base) and B ($head): the $pkg test binaries are byte-identical." +else + echo "A ($base) and B ($head): the $pkg test binaries differ." +fi + +# A test binary runs in its package directory (testdata, relative paths). +dir_a=$tmp/a/${pkg#./} +dir_b=$tmp/b/${pkg#./} +arms=(A B A2) +for ((r = 1; r <= rounds; r++)); do + for ((k = 0; k < 3; k++)); do + arm=${arms[$(((r + k) % 3))]} + dir=$dir_a + [ "$arm" = B ] && dir=$dir_b + (cd "$dir" && "$tmp/$arm.test" -test.run '^$' -test.bench "$re" \ + -test.benchtime "$benchtime" -test.count 1 -test.benchmem) >> "$out/$arm.txt" + done + echo "round $r/$rounds done" +done + +echo +echo "== B ($head) vs A ($base)" +benchstat A="$out/A.txt" B="$out/B.txt" +echo +echo "== A2 vs A: the same binary twice, this machine's floor" +benchstat A="$out/A.txt" A2="$out/A2.txt" diff --git a/.github/workflows/codspeed.yml b/.github/workflows/codspeed.yml index eec8c0f2..511347c9 100644 --- a/.github/workflows/codspeed.yml +++ b/.github/workflows/codspeed.yml @@ -1,12 +1,43 @@ # CodSpeed: continuous benchmarking of the request hot path (celeris#690). # +# How to read a CodSpeed result (celeris#725). The check is INFORMATIONAL: it +# is not a required check, and a red "Performance Regression" is a prompt to +# measure, not a verdict. Two things move these numbers without any change to +# the code a benchmark runs: +# - Code layout. The runner measures one build of each test binary, and a +# commit that adds or moves functions anywhere in that binary can move a +# benchmark whose own code did not change. On main 3fe9620 (celeris#723) +# CodSpeed flagged seven middleware chain benchmarks at -11% to -19% +# against fee0d1c, although PreRouting and PreRoutingOnly execute no +# changed instruction and an interleaved A/B on another machine showed no +# difference. Re-running both commits later reproduced each commit's own +# level within 2% and the same -12% to -19% gap: the level belongs to the +# binary's layout on this runner, and an unrelated merge can move it that +# much. +# - The runner. Test binaries that were byte-identical at every commit still +# moved from run to run: a four-goroutine lock benchmark between 117 and +# 185 ns, single-digit-ns cases between 3 and 4 ns or 8 and 11 ns. Those +# micro-benchmarks are no longer run here (see "Not measured" below). +# Before acting on a flag: +# 1. See whether the flagged benchmark's code changed at all. Build the +# package's test binary for the runner at the base and at the head +# (GOOS=linux GOARCH=arm64 go test -trimpath -c -o .test ) and +# compare the two files. Identical binaries mean the flag is the runner. +# 2. Otherwise measure it with .github/scripts/bench-ab.sh +# : it runs the base, the head and a copy of the base +# interleaved, one process per round, and prints head-vs-base next to +# base-vs-copy, the machine's floor. A difference inside the floor is not +# a change; a B/op or allocs/op difference always is. On a laptop it says +# whether the code on the path got slower; on an arm64 Linux host it also +# shows this runner's layout effects. +# # What it measures: the standard `testing.B` benchmarks of the code every # request runs through -- the router, Context and handler chain (root # bench_test.go), the HTTP/1 parser, protocol detection and the H2 stream -# pool (./protocol/...), the wake-fd primitive (./internal/...), the core -# middleware chains (./middleware) and the logger. Nothing in the benchmarks -# changed: CodSpeed's Go runner builds them with an overlay of `testing` and -# measures each one (https://codspeed.io/docs/benchmarks/go). +# reset (./protocol/...), the core middleware chains (./middleware) and the +# logger. Nothing in the benchmarks changed: CodSpeed's Go runner builds them +# with an overlay of `testing` and measures each one +# (https://codspeed.io/docs/benchmarks/go). # # Not measured, on purpose: test/drivercmp/* and test/benchcmp_* (separate # modules that need Redis, Postgres or Memcached), driver/* (same), the @@ -14,7 +45,24 @@ # and the engines. Engine throughput is judged on real hardware by # goceleris/probatorium's cluster matrix, not by micro-benchmarks, and the # engines change so often that watching them here would spend the whole -# monthly budget (see below). +# monthly budget (see below). Nor the ns-scale and contention +# micro-benchmarks (celeris#725): ./internal/... (internal/wakefd's +# BenchmarkFD* and BenchmarkSignal*) and BenchmarkInternH2HeaderName. Their +# test binaries were byte-identical at every main commit from 9aa94eb to +# dccb839 and still raised flags; they run under a plain `go test -bench`. +# +# How the set is chosen. CodSpeed's Go runner (go-runner, pinned below) +# passes exactly three things from `run:` to `go test`: -bench, -benchtime +# and the package list (go-runner/src/cli.rs at v1.3.0). Every other flag, +# -skip and -tags included, is dropped with a warning, so an exclusion is +# either a package left out of the list or the -bench pattern. The pattern +# '^Benchmark([^I]|I[^n])' keeps every top-level benchmark whose name does not +# start with "BenchmarkIn" (Go's RE2 has no negative lookahead); in these +# packages that leaves out only BenchmarkInternH2HeaderName. A new benchmark +# named BenchmarkIn... would be left out too: rename it or widen the pattern. +# The selection, run once each (Linux: some benchmarks are Linux-only): +# go test -run '^$' -bench '^Benchmark([^I]|I[^n])' -benchtime 1x \ +# . ./protocol/... ./middleware ./middleware/logger # # Where it runs: a CodSpeed Macro Runner, a dedicated bare-metal machine. # Go supports only the walltime instrument, and walltime on a shared @@ -26,35 +74,52 @@ # x64 runner is Pro-only. arm64 == x86 parity for a release stays with # probatorium's cluster. # -# PREREQUISITE (an org setting, not code): Organization settings -> Actions -# -> Runner groups -> Default -> allow public repositories. Until that is -# set, the job below waits in the queue for a runner that never comes. That -# is expected and is not a failure of this workflow. +# PREREQUISITES (settings, not code). All three were done on 2026-09-26: +# 1. Organization settings -> Actions -> Runner groups -> Default: allow +# public repositories, for the selected repositories celeris and +# loadgen. Without it the job waits for a runner that never comes; a job +# that queued before the change never gets one (cancel it, re-run it). +# 2. Repository settings -> Actions -> General -> allowed actions: +# `CodSpeedHQ/action@*` on the allow-list. Without it every run ends in +# startup_failure with no job. +# 3. The repository enabled in CodSpeed (app.codspeed.io, goceleris/celeris). # -# Security: a macro runner is a self-hosted runner outside GitHub's -# sandbox, so code from a FORK pull request never runs on it (the job's -# `if:` below). Same-repository branches can only be pushed by people with -# write access. No pull_request_target, no secrets: the upload +# Security: a macro runner is a self-hosted runner outside GitHub's sandbox. +# The job's `if:` below skips fork pull requests, but it is not the security +# boundary: on `pull_request` the workflow definition comes from the pull +# request's merge ref, so a fork pull request can edit this file, delete the +# `if:` and target the macro runner. What stops it is the repository's +# fork-PR approval policy, "Require approval for all external contributors" +# (`gh api repos/goceleris/celeris/actions/permissions/fork-pr-contributor-approval` +# returns all_external_contributors). That setting must stay, and a +# maintainer must never "Approve and run" a fork pull request that touches +# .github/workflows. Same-repository branches can only be pushed by people +# with write access. No pull_request_target, no secrets: the upload # authenticates with the job's OIDC token. # -# Budget (600 macro-runner minutes a month). The trigger paths below are the -# non-test code of every package the benchmark set executes (measured: the -# set run once with -coverpkg over the whole module), the benchmark files -# themselves, the root go.mod/go.sum and this file. Replaying the 30 days to -# 2026-09-26 -- the busiest month on record, 113 pushes to main and 268 -# pushes to pull requests -- through those paths gives the run count; -# SETUP.md in the evidence for celeris#690 has the arithmetic and the -# per-run minutes. -benchtime=1s instead of the runner's 3s default is what -# keeps that month inside the budget; raise it if the minutes allow. +# Budget: the Free plan's 600 macro-runner minutes a month are shared by the +# whole goceleris organization (loadgen runs CodSpeed too, about 4 minutes a +# run). A run here takes about 5 minutes of job time (275-305 s over the +# first 15 runs). The trigger paths below are the non-test code of every +# package the benchmark set executes (measured: the set run once with +# -coverpkg over the whole module), the benchmark files, the root +# go.mod/go.sum and this file. The 30 days to 2026-09-26, the busiest month +# on record (113 pushes to main, 268 pushes to pull requests), replayed +# through these paths give 66 runs, about 330 minutes; loadgen's busiest +# month is 30 runs, about 120. (The paths as first set up gave 90 runs, about +# 450 minutes, 570 with loadgen: celeris#725.) If the organization runs +# short, gate pull request runs on the `performance` label. -benchtime=1s +# instead of the runner's 3s default is what keeps a month inside the budget. name: CodSpeed on: push: branches: [main] - paths: + # One list for both events: the pull_request trigger below aliases it. + paths: &benchmark-paths - "*.go" - "celeristest/**" - - "internal/**" + - "internal/ctxkit/**" - "observe/**" - "protocol/**" - "middleware/*.go" @@ -80,10 +145,8 @@ on: - "!mage*.go" - "!**/*_test.go" - "*bench*_test.go" - - "internal/**/*bench*_test.go" - "protocol/**/*bench*_test.go" - "protocol/detect/detect_test.go" - - "protocol/h2/stream/intern_test.go" - "protocol/h2/stream/stream_test.go" - "middleware/*bench*_test.go" - "middleware/logger/*bench*_test.go" @@ -93,47 +156,10 @@ on: pull_request: branches: [main] types: [opened, synchronize, reopened, ready_for_review] - paths: - - "*.go" - - "celeristest/**" - - "internal/**" - - "observe/**" - - "protocol/**" - - "middleware/*.go" - - "middleware/adapters/**" - - "middleware/circuitbreaker/**" - - "middleware/cors/**" - - "middleware/etag/**" - - "middleware/internal/**" - - "middleware/logger/**" - - "middleware/methodoverride/**" - - "middleware/pprof/**" - - "middleware/proxy/**" - - "middleware/ratelimit/**" - - "middleware/recovery/**" - - "middleware/redirect/**" - - "middleware/requestid/**" - - "middleware/rewrite/**" - - "middleware/secure/**" - - "middleware/singleflight/**" - - "middleware/static/**" - - "middleware/swagger/**" - - "middleware/timeout/**" - - "!mage*.go" - - "!**/*_test.go" - - "*bench*_test.go" - - "internal/**/*bench*_test.go" - - "protocol/**/*bench*_test.go" - - "protocol/detect/detect_test.go" - - "protocol/h2/stream/intern_test.go" - - "protocol/h2/stream/stream_test.go" - - "middleware/*bench*_test.go" - - "middleware/logger/*bench*_test.go" - - "go.mod" - - "go.sum" - - ".github/workflows/codspeed.yml" + paths: *benchmark-paths # Backtests: CodSpeed dispatches this to measure older commits, and it is - # the manual way to take a fresh baseline. + # the manual way to take a fresh baseline or to run a draft pull request's + # branch (gh workflow run codspeed.yml --ref ). workflow_dispatch: permissions: @@ -151,8 +177,10 @@ concurrency: jobs: benchmarks: name: Benchmarks (arm64 macro runner) - # Fork pull requests never reach a macro runner; drafts wait until they - # are marked ready (ready_for_review above), to save minutes. + # Skips fork pull requests (as this file is written; the approval policy + # in the header is what holds against a fork that edits it) and drafts, + # which wait until they are marked ready (ready_for_review above), to + # save minutes. if: >- github.event_name != 'pull_request' || (github.event.pull_request.head.repo.full_name == github.repository && @@ -184,4 +212,6 @@ jobs: # Pinned: a runner change can move every number, and 1.3.0 is the # first release that supports Go 1.27. Bump it deliberately. go-runner-version: "1.3.0" - run: go test -bench=. -benchtime=1s . ./protocol/... ./internal/... ./middleware ./middleware/logger + # The CodSpeed CLI writes this line to a script and runs it with + # bash, so the quoted pattern reaches go-runner as one argument. + run: go test -bench='^Benchmark([^I]|I[^n])' -benchtime=1s . ./protocol/... ./middleware ./middleware/logger diff --git a/.github/workflows/test-coverage.yml b/.github/workflows/test-coverage.yml index 8c78645b..a8fe624c 100644 --- a/.github/workflows/test-coverage.yml +++ b/.github/workflows/test-coverage.yml @@ -4,14 +4,24 @@ # (celeris#690). It is informational: codecov.yml marks every status # informational, and this workflow is not a required check. # +# The package selection is a copy of ci.yml's, which this workflow cannot +# share without editing ci.yml (see below). The "Same packages as ci.yml" +# step fails when the two select different packages, so a change there has +# to be made here too (celeris#725). +# # It is a separate workflow rather than new steps in ci.yml on purpose: # ci.yml's unit steps are held by PASS-count interlocks and edited by # in-flight pull requests. The cost is that the unit suites run twice per # push; GitHub-hosted minutes are free for public repositories. # # What is NOT in the profile, and why (so a low number is read correctly): -# ./test/... (integration/spec harness, not library code), ./adaptive/... -# (needs raised memlock; ci.yml's `adaptive` job), and ./middleware/websocket. +# ./test/... (integration/spec harness, not library code), ./adaptive/..., +# and ./middleware/websocket. ./adaptive/... runs in ci.yml's `adaptive` job: +# it needs memlock raised with sudo, and its switch tests are load- and +# timing-bound, with RESULT-line tallies that the job checks. Instrumenting +# them would risk what happened to websocket (next sentence) and add the +# adaptive job's run time to every push, so they stay out of the profile, +# and codecov.yml has no adaptive component (celeris#725). # ci.yml runs only websocket's backpressure tests on GitHub-hosted runners, # and those are timing-bound: under coverage instrumentation # TestBackpressurePauseDoesNotCancelInflightSend failed on both engines @@ -45,8 +55,13 @@ jobs: name: Coverage (root + middleware sub-modules) runs-on: ubuntu-latest timeout-minutes: 30 - # A fork pull request gets no OIDC token; its uploads go tokenless instead - # (the use_oidc expression below), which Codecov accepts for public repos. + env: + # ci.yml's unit selection (see "Same packages as ci.yml" below). + COVER_EXCLUDE: "/test/|/adaptive($|/)|/middleware/websocket($|/)" + # A fork pull request gets no OIDC token. codecov-action (v7.1.1) sees the + # fork itself (head repository != this repository), skips OIDC even with + # use_oidc: true, and uploads tokenless, which Codecov accepts for public + # repositories. permissions: contents: read # checkout id-token: write # Codecov upload over OIDC, so no CODECOV_TOKEN secret is stored @@ -58,12 +73,32 @@ jobs: with: go-version: "1.27.0" + # ci.yml's unit job takes `go list ./...` minus the pattern on its first + # `go list ./... | grep -vE '...'` line, and runs ./engine/iouring in a + # step of its own; this job runs all of them in one `go test`. The step + # fails, with the diff, when the two package sets differ. + - name: Same packages as ci.yml's unit job + run: | + set -o pipefail + ci_exclude=$(sed -nE "s/.*go list [.]\/[.][.][.] [|] grep -vE '([^']+)'.*/\1/p" .github/workflows/ci.yml | head -n 1) + if [ -z "$ci_exclude" ]; then + echo "::error::ci.yml has no \`go list ./... | grep -vE '...'\` line any more: re-derive COVER_EXCLUDE" + exit 1 + fi + echo "ci.yml excludes: $ci_exclude (+ ./engine/iouring in its own step)" + echo "test-coverage excludes: $COVER_EXCLUDE" + if ! diff <({ go list ./... | grep -vE "$ci_exclude"; go list ./engine/iouring; } | sort -u) \ + <(go list ./... | grep -vE "$COVER_EXCLUDE" | sort -u); then + echo "::error::test-coverage.yml and ci.yml's unit job select different packages (diff above: < ci.yml, > this job)" + exit 1 + fi + - name: Root module (ci.yml unit package set) run: | # A `go list` failure must stop the job, not hand grep a partial list. set -o pipefail echo "memlock (KiB): $(ulimit -l)" - pkgs=$(go list ./... | grep -vE '/test/|/adaptive($|/)|/middleware/websocket($|/)') + pkgs=$(go list ./... | grep -vE "$COVER_EXCLUDE") # shellcheck disable=SC2086 # word-splitting is intentional -- `go test` wants one package per arg go test -race -count=1 -timeout=300s -covermode=atomic \ -coverprofile="$RUNNER_TEMP/cover-root.out" $pkgs @@ -94,6 +129,10 @@ jobs: done exit "$fail" + # fail_ci_if_error: a failed upload turns this workflow red rather than + # leaving a stale number. During a Codecov outage that includes main. + # This workflow is not a required check, so nothing is blocked, but + # whoever sweeps main after a merge should expect it (celeris#725). - name: Upload root uses: codecov/codecov-action@303a32d7a59b442fa8d48b6a1cc6825c09c847a5 # v7.1.1 with: @@ -102,7 +141,7 @@ jobs: name: root disable_search: true fail_ci_if_error: true - use_oidc: ${{ !github.event.pull_request.head.repo.fork }} + use_oidc: true - name: Upload middleware/compress uses: codecov/codecov-action@303a32d7a59b442fa8d48b6a1cc6825c09c847a5 # v7.1.1 with: @@ -111,7 +150,7 @@ jobs: name: compress disable_search: true fail_ci_if_error: true - use_oidc: ${{ !github.event.pull_request.head.repo.fork }} + use_oidc: true - name: Upload middleware/metrics uses: codecov/codecov-action@303a32d7a59b442fa8d48b6a1cc6825c09c847a5 # v7.1.1 with: @@ -120,7 +159,7 @@ jobs: name: metrics disable_search: true fail_ci_if_error: true - use_oidc: ${{ !github.event.pull_request.head.repo.fork }} + use_oidc: true - name: Upload middleware/otel uses: codecov/codecov-action@303a32d7a59b442fa8d48b6a1cc6825c09c847a5 # v7.1.1 with: @@ -129,7 +168,7 @@ jobs: name: otel disable_search: true fail_ci_if_error: true - use_oidc: ${{ !github.event.pull_request.head.repo.fork }} + use_oidc: true - name: Upload middleware/protobuf uses: codecov/codecov-action@303a32d7a59b442fa8d48b6a1cc6825c09c847a5 # v7.1.1 with: @@ -138,4 +177,4 @@ jobs: name: protobuf disable_search: true fail_ci_if_error: true - use_oidc: ${{ !github.event.pull_request.head.repo.fork }} + use_oidc: true diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 5f66edd1..5f5c307d 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -118,9 +118,18 @@ mage -l # List all available targets The full rule lives in [GOVERNANCE.md](GOVERNANCE.md); the short version: - Every change to `main` goes through a pull request — maintainers included. -- A PR merges when **all required checks are green** and it has an - **approving review from a code owner** of the files it touches - ([`.github/CODEOWNERS`](.github/CODEOWNERS)). +- A PR merges when **all required checks are green**, **every review thread + is resolved**, and it has an **approving review from a code owner** of the + files it touches ([`.github/CODEOWNERS`](.github/CODEOWNERS)). +- Bots comment on every PR: CodeRabbit (review), Codecov (coverage) and + CodSpeed (benchmarks). None of them is a required check, but each inline + CodeRabbit comment opens a review thread, and the thread rule above applies + to it. CodeRabbit resolves its own thread when a later push fixes the + point; for one you disagree with, reply saying why and resolve it (or + comment `@coderabbitai resolve` to resolve all of its threads). A red + CodSpeed check is informational: the header of + [`.github/workflows/codspeed.yml`](.github/workflows/codspeed.yml) says how + to read it. - The author merges if they have write access (a member of the `contributors` team or a maintainer); otherwise the approving maintainer merges, usually via auto-merge. diff --git a/GOVERNANCE.md b/GOVERNANCE.md index 73c77ae1..a0b53733 100644 --- a/GOVERNANCE.md +++ b/GOVERNANCE.md @@ -38,10 +38,12 @@ is **@FumingPower3925** (see [MAINTAINERS.md](MAINTAINERS.md)). ## How changes get merged Every change to `main` goes through a pull request, including changes by -maintainers. A PR merges when **both** hold: +maintainers. A PR merges when **all three** hold: -1. all **required checks are green**, and -2. it has an **approving review from a code owner** of the files it +1. all **required checks are green**, +2. **every review thread is resolved**, bots' threads included (the + `main` ruleset requires it), and +3. it has an **approving review from a code owner** of the files it touches (CODEOWNERS is the source of truth). Who presses the button: diff --git a/codecov.yml b/codecov.yml index cb450482..f1e02701 100644 --- a/codecov.yml +++ b/codecov.yml @@ -74,7 +74,10 @@ flag_management: paths: - "middleware/protobuf/" -# Coverage by area, whichever module's upload it came from. +# Coverage by area, whichever module's upload it came from. There is no +# adaptive component: ./adaptive/... is not in the coverage profile (it runs +# in ci.yml's `adaptive` job at raised memlock; test-coverage.yml says why), +# so the component could only ever be empty (celeris#725). component_management: individual_components: - component_id: core @@ -91,10 +94,6 @@ component_management: name: engines (epoll, io_uring, std) paths: - "engine/**" - - component_id: adaptive - name: adaptive - paths: - - "adaptive/**" - component_id: protocol name: protocol (h1, h2, detect) paths: From 4444bfef022f7f668315b1092f803c7a00f8433c Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Sun, 27 Sep 2026 21:01:11 +0200 Subject: [PATCH 2/4] ci(codspeed): say that package init does not make a trigger path, and that the set is the same 54 leaves on Linux and macOS (celeris#725) Measured on linux/arm64 with -coverpkg: besides internal/ctxkit, the root test binary covers internal/conn and internal/deferlinger, but only their init() (and the 1-second date ticker internal/conn's init starts). No benchmark calls into them, so they stay out of the paths. Counting them would have made the replayed month 75 runs instead of 66. --- .github/workflows/codspeed.yml | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/.github/workflows/codspeed.yml b/.github/workflows/codspeed.yml index 511347c9..b2b92d15 100644 --- a/.github/workflows/codspeed.yml +++ b/.github/workflows/codspeed.yml @@ -60,7 +60,7 @@ # start with "BenchmarkIn" (Go's RE2 has no negative lookahead); in these # packages that leaves out only BenchmarkInternH2HeaderName. A new benchmark # named BenchmarkIn... would be left out too: rename it or widen the pattern. -# The selection, run once each (Linux: some benchmarks are Linux-only): +# The selection, each benchmark run once (54 leaves, on Linux and macOS): # go test -run '^$' -bench '^Benchmark([^I]|I[^n])' -benchtime 1x \ # . ./protocol/... ./middleware ./middleware/logger # @@ -102,8 +102,11 @@ # run). A run here takes about 5 minutes of job time (275-305 s over the # first 15 runs). The trigger paths below are the non-test code of every # package the benchmark set executes (measured: the set run once with -# -coverpkg over the whole module), the benchmark files, the root -# go.mod/go.sum and this file. The 30 days to 2026-09-26, the busiest month +# -coverpkg over the whole module, on Linux and macOS), the benchmark files, +# the root go.mod/go.sum and this file. Package initialization does not +# count: on Linux the root test binary links internal/conn and +# internal/deferlinger through the engines, and only their init() runs, so +# a change there does not start a run. The 30 days to 2026-09-26, the busiest month # on record (113 pushes to main, 268 pushes to pull requests), replayed # through these paths give 66 runs, about 330 minutes; loadgen's busiest # month is 30 runs, about 120. (The paths as first set up gave 90 runs, about From fc976f085a1c414d1994872e69bd80409ae32089 Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Sun, 27 Sep 2026 21:42:42 +0200 Subject: [PATCH 3/4] ci(codspeed): say that the kept set has run noise too, name the base to check, the org-member exemption and the measured burn; narrow the trigger paths (celeris#725) Round 2 of #748's review: - The header no longer implies that run noise left with the removed micro-benchmarks: on three pairs of runs of identical trees, ContextQueryFirstParse moved 18.5-22.7% and ChainDeepParallel 15.2%. Step 1 applies to every flag, with the base CodSpeed names in its report. - The fork-PR approval policy exempts organization members; say that it holds only while every member has write access to each repository in the macro-runner group. - The budget paragraph gives the measured burn (189 minutes in 27 hours) and points at celeris#754 for the label-gate decision. - The -bench pattern spells out the complement of BenchmarkInternH, so a future BenchmarkIn... is no longer dropped. Same 54 leaves. - Trigger paths: protocol/detect, h1 and h2/stream, and middleware/internal/fnv1a, the packages the set executes. - bench-ab.sh checks linux/arm64 binary identity as well as the host's, refuses an OUT that holds results, and says refs are commits. - The coverage package guard needs exactly one ci.yml line and runs go list outside a process substitution. - The merge rule's short forms in GOVERNANCE.md and MAINTAINERS.md name thread resolution; CONTRIBUTING says when each bot reports. --- .github/scripts/bench-ab.sh | 58 +++++++---- .github/workflows/codspeed.yml | 149 +++++++++++++++++----------- .github/workflows/test-coverage.yml | 23 +++-- CONTRIBUTING.md | 6 +- GOVERNANCE.md | 5 +- MAINTAINERS.md | 3 +- 6 files changed, 153 insertions(+), 91 deletions(-) diff --git a/.github/scripts/bench-ab.sh b/.github/scripts/bench-ab.sh index 0ca54648..5b139c38 100755 --- a/.github/scripts/bench-ab.sh +++ b/.github/scripts/bench-ab.sh @@ -8,34 +8,41 @@ # '^BenchmarkChain(Baseline|MinimalAPI|PreRoutingOnly)$' 20 # # What it does: -# 1. Builds 's test binary at (arm A) and at -# (arm B), each in a temporary worktree, with -trimpath, and says whether -# the two binaries are byte-identical. If they are, any difference a -# benchmark service reported between them came from its machine. -# 2. Copies A to A2: a byte-identical third arm, whose spread against A is -# the noise floor of this machine. -# 3. Runs rounds (default 20). A round runs A, B and A2 once each, +# 1. Checks out and in temporary worktrees. Refs are +# commits: uncommitted edits are not measured, so commit them first. +# 2. Builds 's test binary at both, with -trimpath, twice: for +# linux/arm64 (CodSpeed's runner) and for this machine. It says whether +# each pair is byte-identical. Identical linux/arm64 binaries mean any +# difference CodSpeed reported between the two refs came from its +# runner. The host pair only speaks for this host: a change to +# Linux-only code (the engines, internal/conn) leaves macOS binaries +# identical and linux/arm64 ones different. +# 3. Copies the host A to A2: a byte-identical third arm, whose spread +# against A is the noise floor of this machine. +# 4. Runs rounds (default 20). A round runs A, B and A2 once each, # a fresh process per run (-test.count=1), with the arm order rotated # every round so that no arm always runs first or last. -# 4. Prints benchstat for A vs B and for A vs A2. +# 5. Prints benchstat for A vs B and for A vs A2. # # Reading it: a B-vs-A delta that is not larger than the A-vs-A2 delta is not -# a change. B/op and allocs/op do not depend on the machine: if they moved, -# the code moved. +# a change. That holds for B/op and allocs/op too: sync.Pool and the GC make +# them vary a little between runs of one binary, so compare them with A2 as +# well. # -# The numbers describe the machine they ran on. A build for your laptop has +# The timings describe the machine they ran on. A build for your laptop has # its own code layout, so it answers "did the code on this path get slower", # not "why did the CodSpeed arm64 runner move". For the runner's layout, run # this on an arm64 Linux host. Run it on a quiet machine: nothing else busy, # on AC power. # # Environment: BENCHTIME (default 1s, as CodSpeed runs it), OUT (a directory -# to keep the raw outputs in; default a temporary one, removed on exit). +# to keep the raw outputs in; it must not hold an earlier run's A.txt, B.txt +# or A2.txt; default a temporary one, removed on exit). # Needs benchstat: go install golang.org/x/perf/cmd/benchstat@latest set -euo pipefail if [ "$#" -lt 4 ]; then - sed -n '2,12p' "$0" | sed 's/^# \{0,1\}//' + sed -n '2,8p' "$0" | sed 's/^# \{0,1\}//' exit 2 fi base=$1 head=$2 pkg=$3 re=$4 rounds=${5:-20} @@ -52,17 +59,32 @@ cleanup() { trap cleanup EXIT out=${OUT:-$tmp/out} mkdir -p "$out" +for f in A B A2; do + if [ -e "$out/$f.txt" ]; then + echo "$out/$f.txt exists: an earlier run's results would be mixed into this one. Use an empty OUT." >&2 + exit 2 + fi +done git -C "$repo" worktree add --detach --quiet "$tmp/a" "$base" git -C "$repo" worktree add --detach --quiet "$tmp/b" "$head" + +# Binary identity for CodSpeed's runner, then for this host. +(cd "$tmp/a" && GOOS=linux GOARCH=arm64 go test -trimpath -c -o "$tmp/A.linux-arm64.test" "$pkg") +(cd "$tmp/b" && GOOS=linux GOARCH=arm64 go test -trimpath -c -o "$tmp/B.linux-arm64.test" "$pkg") (cd "$tmp/a" && go test -trimpath -c -o "$tmp/A.test" "$pkg") (cd "$tmp/b" && go test -trimpath -c -o "$tmp/B.test" "$pkg") cp "$tmp/A.test" "$tmp/A2.test" -if cmp -s "$tmp/A.test" "$tmp/B.test"; then - echo "A ($base) and B ($head): the $pkg test binaries are byte-identical." -else - echo "A ($base) and B ($head): the $pkg test binaries differ." -fi +host="$(go env GOOS)/$(go env GOARCH)" +for target in linux/arm64 "$host"; do + if [ "$target" = linux/arm64 ]; then a=$tmp/A.linux-arm64.test b=$tmp/B.linux-arm64.test; else a=$tmp/A.test b=$tmp/B.test; fi + if cmp -s "$a" "$b"; then + echo "A ($base) and B ($head): the $pkg test binaries for $target are byte-identical." + else + echo "A ($base) and B ($head): the $pkg test binaries for $target differ." + fi +done +echo "The timings below are for $host." # A test binary runs in its package directory (testdata, relative paths). dir_a=$tmp/a/${pkg#./} diff --git a/.github/workflows/codspeed.yml b/.github/workflows/codspeed.yml index b2b92d15..8f4dd3cd 100644 --- a/.github/workflows/codspeed.yml +++ b/.github/workflows/codspeed.yml @@ -5,36 +5,44 @@ # measure, not a verdict. Two things move these numbers without any change to # the code a benchmark runs: # - Code layout. The runner measures one build of each test binary, and a -# commit that adds or moves functions anywhere in that binary can move a -# benchmark whose own code did not change. On main 3fe9620 (celeris#723) -# CodSpeed flagged seven middleware chain benchmarks at -11% to -19% -# against fee0d1c, although PreRouting and PreRoutingOnly execute no -# changed instruction and an interleaved A/B on another machine showed no -# difference. Re-running both commits later reproduced each commit's own -# level within 2% and the same -12% to -19% gap: the level belongs to the -# binary's layout on this runner, and an unrelated merge can move it that -# much. -# - The runner. Test binaries that were byte-identical at every commit still -# moved from run to run: a four-goroutine lock benchmark between 117 and -# 185 ns, single-digit-ns cases between 3 and 4 ns or 8 and 11 ns. Those -# micro-benchmarks are no longer run here (see "Not measured" below). -# Before acting on a flag: -# 1. See whether the flagged benchmark's code changed at all. Build the -# package's test binary for the runner at the base and at the head -# (GOOS=linux GOARCH=arm64 go test -trimpath -c -o .test ) and -# compare the two files. Identical binaries mean the flag is the runner. +# change anywhere in that binary can move a benchmark whose own code did +# not change. celeris#736 changed bridge.go's Adapt and four middleware +# packages; BenchmarkChainBaseline, which runs none of that code, went +# from 1908-1932 ns to 2292-2316 ns, and both runs of #736's tree gave +# that level. On 3fe9620 (celeris#723) seven chain benchmarks moved -11% +# to -19% although PreRouting and PreRoutingOnly execute no changed +# instruction; re-running both commits reproduced each commit's own level +# within 2%. The level belongs to the binary's layout on this runner. +# - The runner. Identical source trees still move from run to run. On +# three pairs of runs of identical trees, ContextQueryFirstParse moved +# between 737 and 904 ns (18.5% or 22.7%, by direction), +# ChainDeepParallel 15.2%, and ContextString, ResetH1Stream, RouterFind +# and ContextBlob 5-10%. The ns-scale micro-benchmarks moved most (a lock +# under four goroutines 117-185 ns, single-digit cases 3-4 and 8-11 ns) +# and are no longer run here (see "Not measured"); what stays still has +# run noise. +# No regression threshold separates these from a real change: in the first +# 15 comparisons layout alone reached -19.6%, and run noise on identical +# trees -18.5%. So before acting on ANY flag: +# 1. Take the base CodSpeed names in its report, which is often not the +# pull request's base: with path filters CodSpeed compares with the +# latest main run, and a layout shift from a main commit that started no +# run lands on the next run that does. Build the flagged package's test +# binary for the runner at that base and at the head +# (GOOS=linux GOARCH=arm64 go test -trimpath -c -o .test ) +# and compare the two files. Identical binaries: the flag is the runner. # 2. Otherwise measure it with .github/scripts/bench-ab.sh -# : it runs the base, the head and a copy of the base -# interleaved, one process per round, and prints head-vs-base next to -# base-vs-copy, the machine's floor. A difference inside the floor is not -# a change; a B/op or allocs/op difference always is. On a laptop it says -# whether the code on the path got slower; on an arm64 Linux host it also -# shows this runner's layout effects. +# . It checks the same linux/arm64 binary identity, then +# runs the base, the head and a copy of the base interleaved, one +# process per round, and prints head-vs-base next to base-vs-copy, the +# machine's floor. A difference inside the floor is not a change. On a +# laptop it says whether the code on the path got slower; on an arm64 +# Linux host it also shows this runner's layout effects. # -# What it measures: the standard `testing.B` benchmarks of the code every -# request runs through -- the router, Context and handler chain (root -# bench_test.go), the HTTP/1 parser, protocol detection and the H2 stream -# reset (./protocol/...), the core middleware chains (./middleware) and the +# What it measures: the standard `testing.B` benchmarks of the router, +# Context and handler chain (root bench_test.go), the HTTP/1 parser, +# protocol detection, the HTTP/1 stream reset (BenchmarkResetH1Stream, in +# protocol/h2/stream), the core middleware chains (./middleware) and the # logger. Nothing in the benchmarks changed: CodSpeed's Go runner builds them # with an overlay of `testing` and measures each one # (https://codspeed.io/docs/benchmarks/go). @@ -45,24 +53,33 @@ # and the engines. Engine throughput is judged on real hardware by # goceleris/probatorium's cluster matrix, not by micro-benchmarks, and the # engines change so often that watching them here would spend the whole -# monthly budget (see below). Nor the ns-scale and contention -# micro-benchmarks (celeris#725): ./internal/... (internal/wakefd's -# BenchmarkFD* and BenchmarkSignal*) and BenchmarkInternH2HeaderName. Their -# test binaries were byte-identical at every main commit from 9aa94eb to -# dccb839 and still raised flags; they run under a plain `go test -bench`. +# monthly budget (see below). Nor HTTP/2 request handling (HPACK header +# decoding, frames): no benchmark in the set runs it (celeris#754). +# Removed in celeris#725: +# - ./internal/...: its only benchmarks are internal/wakefd's, and that is +# the engines' wake primitive, judged on the cluster with them. The +# leaves that moved are celeris#655 comparison shapes that never shipped +# (RWMutex and shared cache line under four producers, 117-185 and 8-11 +# ns; plain field, 3-4 ns), on test binaries that were byte-identical at +# every main commit from 9aa94eb to dccb839. The shipping shape +# (FDAtomicPadded) and Signal/* were steady; they go with the package. +# - BenchmarkInternH2HeaderName: 49-64 ns on a byte-identical binary. +# Every router, Context, chain, handler and logger benchmark stays, the two +# noisiest above included: they are the hot path. Run the removed ones with +# a plain `go test -bench`. # # How the set is chosen. CodSpeed's Go runner (go-runner, pinned below) # passes exactly three things from `run:` to `go test`: -bench, -benchtime # and the package list (go-runner/src/cli.rs at v1.3.0). Every other flag, # -skip and -tags included, is dropped with a warning, so an exclusion is -# either a package left out of the list or the -bench pattern. The pattern -# '^Benchmark([^I]|I[^n])' keeps every top-level benchmark whose name does not -# start with "BenchmarkIn" (Go's RE2 has no negative lookahead); in these -# packages that leaves out only BenchmarkInternH2HeaderName. A new benchmark -# named BenchmarkIn... would be left out too: rename it or widen the pattern. +# either a package left out of the list or the -bench pattern. Go's RE2 has +# no negative lookahead, so the pattern spells out a complement: it selects +# every top-level benchmark except a name that starts with BenchmarkInternH +# (and the bare prefixes Benchmark to BenchmarkIntern). In these packages +# that leaves out only BenchmarkInternH2HeaderName. # The selection, each benchmark run once (54 leaves, on Linux and macOS): -# go test -run '^$' -bench '^Benchmark([^I]|I[^n])' -benchtime 1x \ -# . ./protocol/... ./middleware ./middleware/logger +# go test -run '^$' -bench '^Benchmark([^I]|I[^n]|In[^t]|Int[^e]|Inte[^r]|Inter[^n]|Intern[^H])' \ +# -benchtime 1x . ./protocol/... ./middleware ./middleware/logger # # Where it runs: a CodSpeed Macro Runner, a dedicated bare-metal machine. # Go supports only the walltime instrument, and walltime on a shared @@ -93,26 +110,36 @@ # (`gh api repos/goceleris/celeris/actions/permissions/fork-pr-contributor-approval` # returns all_external_contributors). That setting must stay, and a # maintainer must never "Approve and run" a fork pull request that touches -# .github/workflows. Same-repository branches can only be pushed by people -# with write access. No pull_request_target, no secrets: the upload +# .github/workflows. The policy exempts organization members, and the +# organization's base repository permission is read: it holds only while +# every organization member has write access to every repository in the +# macro-runner group (celeris and loadgen, prerequisite 1). A member with +# read access can run a fork pull request there without approval +# (celeris#754). Same-repository branches can only be pushed by people with +# write access. No pull_request_target, no secrets: the upload # authenticates with the job's OIDC token. # # Budget: the Free plan's 600 macro-runner minutes a month are shared by the -# whole goceleris organization (loadgen runs CodSpeed too, about 4 minutes a -# run). A run here takes about 5 minutes of job time (275-305 s over the -# first 15 runs). The trigger paths below are the non-test code of every -# package the benchmark set executes (measured: the set run once with -# -coverpkg over the whole module, on Linux and macOS), the benchmark files, -# the root go.mod/go.sum and this file. Package initialization does not -# count: on Linux the root test binary links internal/conn and -# internal/deferlinger through the engines, and only their init() runs, so -# a change there does not start a run. The 30 days to 2026-09-26, the busiest month -# on record (113 pushes to main, 268 pushes to pull requests), replayed -# through these paths give 66 runs, about 330 minutes; loadgen's busiest -# month is 30 runs, about 120. (The paths as first set up gave 90 runs, about -# 450 minutes, 570 with loadgen: celeris#725.) If the organization runs -# short, gate pull request runs on the `performance` label. -benchtime=1s -# instead of the runner's 3s default is what keeps a month inside the budget. +# whole goceleris organization; loadgen runs CodSpeed too. A run here takes +# about 4 minutes of job time (240 s with this set, measured once; 275-305 s +# before celeris#725), a loadgen run about 3 (185-190 s). Measured burn over +# the first 27 hours (2026-09-26 15:57Z to 2026-09-27 18:49Z): 43 jobs and +# 189 minutes rounded up per job (celeris 21 jobs and 101 minutes, loadgen 22 +# and 88), two thirds of it pull request runs. At that rate the 600 minutes +# last 3.6 to 4.1 days (rounded or raw job time; CodSpeed's own meter is not +# public), and pushes to main alone would use them in about two weeks. Whether to gate pull request runs on the `performance` label, +# or more, is an open decision (celeris#754). +# The trigger paths below are the non-test code of every package the +# benchmark set executes (measured: the set run once with -coverpkg over the +# whole module, on Linux and macOS), the benchmark files, the root +# go.mod/go.sum and this file. Package initialization does not count: on +# Linux the root test binary links internal/conn and internal/deferlinger +# through the engines, and only their init() runs, so a change there does +# not start a run. Replaying the 30 days to 2026-09-26 (113 pushes to main, +# 268 pushes to pull requests) through these paths gives 66 runs, against 90 +# for the paths as first set up (celeris#725). The last day ran far more than +# that month did, so the replay is a floor, not a forecast. -benchtime=1s +# (the runner's default is 3s) bounds the time each benchmark is measured. name: CodSpeed on: @@ -124,13 +151,15 @@ on: - "celeristest/**" - "internal/ctxkit/**" - "observe/**" - - "protocol/**" + - "protocol/detect/**" + - "protocol/h1/**" + - "protocol/h2/stream/**" - "middleware/*.go" - "middleware/adapters/**" - "middleware/circuitbreaker/**" - "middleware/cors/**" - "middleware/etag/**" - - "middleware/internal/**" + - "middleware/internal/fnv1a/**" - "middleware/logger/**" - "middleware/methodoverride/**" - "middleware/pprof/**" @@ -217,4 +246,4 @@ jobs: go-runner-version: "1.3.0" # The CodSpeed CLI writes this line to a script and runs it with # bash, so the quoted pattern reaches go-runner as one argument. - run: go test -bench='^Benchmark([^I]|I[^n])' -benchtime=1s . ./protocol/... ./middleware ./middleware/logger + run: go test -bench='^Benchmark([^I]|I[^n]|In[^t]|Int[^e]|Inte[^r]|Inter[^n]|Intern[^H])' -benchtime=1s . ./protocol/... ./middleware ./middleware/logger diff --git a/.github/workflows/test-coverage.yml b/.github/workflows/test-coverage.yml index a8fe624c..e7630cdf 100644 --- a/.github/workflows/test-coverage.yml +++ b/.github/workflows/test-coverage.yml @@ -73,22 +73,29 @@ jobs: with: go-version: "1.27.0" - # ci.yml's unit job takes `go list ./...` minus the pattern on its first + # ci.yml's unit job takes `go list ./...` minus the pattern on its one # `go list ./... | grep -vE '...'` line, and runs ./engine/iouring in a # step of its own; this job runs all of them in one `go test`. The step - # fails, with the diff, when the two package sets differ. + # fails, with the diff, when the two package sets differ, and when + # ci.yml no longer has exactly one such line or `go list` fails. - name: Same packages as ci.yml's unit job run: | - set -o pipefail - ci_exclude=$(sed -nE "s/.*go list [.]\/[.][.][.] [|] grep -vE '([^']+)'.*/\1/p" .github/workflows/ci.yml | head -n 1) - if [ -z "$ci_exclude" ]; then - echo "::error::ci.yml has no \`go list ./... | grep -vE '...'\` line any more: re-derive COVER_EXCLUDE" + set -euo pipefail + ci_lines=$(sed -nE "s/.*go list [.]\/[.][.][.] [|] grep -vE '([^']+)'.*/\1/p" .github/workflows/ci.yml) + if [ "$(printf '%s\n' "$ci_lines" | grep -c .)" != 1 ]; then + echo "::error::ci.yml should have exactly one \`go list ./... | grep -vE '...'\` line (its unit job's); found: ${ci_lines:-none}. Re-derive COVER_EXCLUDE." exit 1 fi + ci_exclude=$ci_lines echo "ci.yml excludes: $ci_exclude (+ ./engine/iouring in its own step)" echo "test-coverage excludes: $COVER_EXCLUDE" - if ! diff <({ go list ./... | grep -vE "$ci_exclude"; go list ./engine/iouring; } | sort -u) \ - <(go list ./... | grep -vE "$COVER_EXCLUDE" | sort -u); then + # Outside a process substitution, so that set -e sees a failure. + all=$(go list ./...) + iouring=$(go list ./engine/iouring) + ci_pkgs=$({ grep -vE "$ci_exclude" <<< "$all" || true; echo "$iouring"; } | sort -u) + cover_pkgs=$({ grep -vE "$COVER_EXCLUDE" <<< "$all" || true; } | sort -u) + echo "packages: ci.yml $(grep -c . <<< "$ci_pkgs"), this job $(grep -c . <<< "$cover_pkgs")" + if ! diff <(echo "$ci_pkgs") <(echo "$cover_pkgs"); then echo "::error::test-coverage.yml and ci.yml's unit job select different packages (diff above: < ci.yml, > this job)" exit 1 fi diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 5f5c307d..c3b1a81b 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -121,8 +121,10 @@ The full rule lives in [GOVERNANCE.md](GOVERNANCE.md); the short version: - A PR merges when **all required checks are green**, **every review thread is resolved**, and it has an **approving review from a code owner** of the files it touches ([`.github/CODEOWNERS`](.github/CODEOWNERS)). -- Bots comment on every PR: CodeRabbit (review), Codecov (coverage) and - CodSpeed (benchmarks). None of them is a required check, but each inline +- Three bots report on PRs, and none of them is a required check: + CodeRabbit (review; a draft once it is marked ready), Codecov (coverage) + and CodSpeed (benchmarks; only on a PR from a branch of this repository + that is ready for review and touches benchmarked code). Each inline CodeRabbit comment opens a review thread, and the thread rule above applies to it. CodeRabbit resolves its own thread when a later push fixes the point; for one you disagree with, reply saying why and resolve it (or diff --git a/GOVERNANCE.md b/GOVERNANCE.md index a0b53733..e4625cf7 100644 --- a/GOVERNANCE.md +++ b/GOVERNANCE.md @@ -20,8 +20,9 @@ Members of the GitHub org team **`contributors`** have *write* access to - can push branches to this repository and open PRs from them; - can **approve** pull requests; -- may **merge their own PR only after a code-owner approval** and green - required checks (see [How changes get merged](#how-changes-get-merged)). +- may **merge their own PR only after a code-owner approval**, with green + required checks and every review thread resolved (see + [How changes get merged](#how-changes-get-merged)). Criteria for an invitation: about **three merged, non-trivial pull requests** and sustained engagement (reviews, issue triage, follow-through diff --git a/MAINTAINERS.md b/MAINTAINERS.md index dcc150ff..1e7a8739 100644 --- a/MAINTAINERS.md +++ b/MAINTAINERS.md @@ -14,7 +14,8 @@ Admin access; cut releases; own [`.github/CODEOWNERS`](.github/CODEOWNERS). ## `contributors` team Write access to `goceleris/celeris` only. Members may approve PRs and merge -their own PRs after a code-owner approval and green required checks. +their own PRs after a code-owner approval, with green required checks and +every review thread resolved. | GitHub | Focus | |---------|-------| From 2249f54a1832bba8088005d7dbb0ebb12925d2aa Mon Sep 17 00:00:00 2001 From: Albert Bausili Date: Sun, 27 Sep 2026 21:51:54 +0200 Subject: [PATCH 4/4] ci(codspeed): count the fourth identical-code pair (this PR's own two runs) in the header's run-noise numbers (celeris#725) --- .github/workflows/codspeed.yml | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/.github/workflows/codspeed.yml b/.github/workflows/codspeed.yml index 8f4dd3cd..810b41d2 100644 --- a/.github/workflows/codspeed.yml +++ b/.github/workflows/codspeed.yml @@ -13,17 +13,20 @@ # to -19% although PreRouting and PreRoutingOnly execute no changed # instruction; re-running both commits reproduced each commit's own level # within 2%. The level belongs to the binary's layout on this runner. -# - The runner. Identical source trees still move from run to run. On -# three pairs of runs of identical trees, ContextQueryFirstParse moved -# between 737 and 904 ns (18.5% or 22.7%, by direction), -# ChainDeepParallel 15.2%, and ContextString, ResetH1Stream, RouterFind -# and ContextBlob 5-10%. The ns-scale micro-benchmarks moved most (a lock -# under four goroutines 117-185 ns, single-digit cases 3-4 and 8-11 ns) -# and are no longer run here (see "Not measured"); what stays still has -# run noise. +# - The runner. Identical code still moves from run to run. On four pairs +# of runs of identical code (three identical source trees, and two runs +# of celeris#748 whose test binaries are byte-identical), +# ContextQueryFirstParse moved between 737 and 904 ns (18.5% or 22.7%, +# by direction), ContextSetHeader from 113 to 136 ns (16.9%) and +# ChainDeepParallel by up to 15.2%; ContextString, ResetH1Stream, +# RouterFind and ContextBlob moved 7.8-10%. The second of #748's runs +# went red on that alone. The ns-scale micro-benchmarks moved most (a +# lock under four goroutines 117-185 ns, single-digit cases 3-4 and 8-11 +# ns) and are no longer run here (see "Not measured"); what stays still +# has run noise. # No regression threshold separates these from a real change: in the first -# 15 comparisons layout alone reached -19.6%, and run noise on identical -# trees -18.5%. So before acting on ANY flag: +# 16 comparisons layout alone reached -19.6%, and run noise alone -18.5%. +# So before acting on ANY flag: # 1. Take the base CodSpeed names in its report, which is often not the # pull request's base: with path filters CodSpeed compares with the # latest main run, and a layout shift from a main commit that started no @@ -64,9 +67,9 @@ # every main commit from 9aa94eb to dccb839. The shipping shape # (FDAtomicPadded) and Signal/* were steady; they go with the package. # - BenchmarkInternH2HeaderName: 49-64 ns on a byte-identical binary. -# Every router, Context, chain, handler and logger benchmark stays, the two -# noisiest above included: they are the hot path. Run the removed ones with -# a plain `go test -bench`. +# Every router, Context, chain, handler and logger benchmark stays, the +# noisiest ones above included: they are the hot path. Run the removed ones +# with a plain `go test -bench`. # # How the set is chosen. CodSpeed's Go runner (go-runner, pinned below) # passes exactly three things from `run:` to `go test`: -bench, -benchtime