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..5b139c38 --- /dev/null +++ b/.github/scripts/bench-ab.sh @@ -0,0 +1,109 @@ +#!/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. 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. +# 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. 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 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; 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,8p' "$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" +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" +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#./} +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..810b41d2 100644 --- a/.github/workflows/codspeed.yml +++ b/.github/workflows/codspeed.yml @@ -1,12 +1,54 @@ # CodSpeed: continuous benchmarking of the request hot path (celeris#690). # -# 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). +# 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 +# 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 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 +# 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 +# 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 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 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). # # Not measured, on purpose: test/drivercmp/* and test/benchcmp_* (separate # modules that need Redis, Postgres or Memcached), driver/* (same), the @@ -14,7 +56,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). +# 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 +# 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 +# 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. 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]|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 @@ -26,43 +94,75 @@ # 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 +# 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. 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 (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. 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: 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/**" + - "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/**" @@ -80,10 +180,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 +191,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 +212,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 +247,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]|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 8c78645b..e7630cdf 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,39 @@ jobs: with: go-version: "1.27.0" + # 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, 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 -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" + # 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 + - 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 +136,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 +148,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 +157,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 +166,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 +175,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 +184,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..c3b1a81b 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -118,9 +118,20 @@ 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)). +- 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 + 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..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 @@ -38,10 +39,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/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 | |---------|-------| 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: