From f99ae21b86ff8126f464a8defab161e57e73c91d Mon Sep 17 00:00:00 2001 From: cuioss oliver <23139298+cuioss@users.noreply.github.com> Date: Tue, 28 Jul 2026 15:08:04 +0200 Subject: [PATCH 1/9] fix(benchmarks): make the passthrough-relay benchmark reachable and mode-aware Two independent defects kept the passthroughRelay goal red and its empty-mode sibling from running at all. 1. Unresolvable target host. passthrough_relay.js defaulted PASSTHROUGH_TARGET_URL to https://passthrough.api-sheriff:8443/get, a name no compose file declares. Convert the api-sheriff service's networks short-form list to map form and add the alias passthrough.test.example, then repoint the script default at it. gateway.yaml already maps passthrough.test.example to PASSTHROUGH_BACKEND under tls.passthrough_sni, and the backend certificate already carries the name as a SAN, so no config change is required. 2. Unpropagated mode selector. benchmarks/pom.xml sets PASSTHROUGH_SNI=mapped and PASSTHROUGH_SNI=empty per execution, but the k6 service's environment pass-through block never listed the variable, so neither run received it. Add a value-less PASSTHROUGH_SNI entry so an unset variable stays unset. Both edits land together: fixing only the alias would move the failure from the k6 run to post-integration-test, where PassthroughBaselineComparator throws on the missing passthroughRelayEmpty summary. Co-Authored-By: Claude --- .../resources/k6-scripts/passthrough_relay.js | 2 +- integration-tests/docker-compose.yml | 17 +++++++++++++++-- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/benchmarks/src/main/resources/k6-scripts/passthrough_relay.js b/benchmarks/src/main/resources/k6-scripts/passthrough_relay.js index 2fb2e405..ad2d937e 100644 --- a/benchmarks/src/main/resources/k6-scripts/passthrough_relay.js +++ b/benchmarks/src/main/resources/k6-scripts/passthrough_relay.js @@ -34,7 +34,7 @@ const MODE = (__ENV.PASSTHROUGH_SNI || 'mapped').toLowerCase(); * container and be listed in `tls.passthrough_sni`; the benchmark compose overlay (D4) provides that * mapping and the TLS-enabled backend behind it. Overridable so a run can target a different edge. */ -const PASSTHROUGH_TARGET_URL = __ENV.PASSTHROUGH_TARGET_URL || 'https://passthrough.api-sheriff:8443/get'; +const PASSTHROUGH_TARGET_URL = __ENV.PASSTHROUGH_TARGET_URL || 'https://passthrough.test.example:8443/get'; /** * Resolves the (benchmarkName, url) pair for the selected mode. An unknown mode is fatal at module diff --git a/integration-tests/docker-compose.yml b/integration-tests/docker-compose.yml index 54f09433..e8cb6ac6 100644 --- a/integration-tests/docker-compose.yml +++ b/integration-tests/docker-compose.yml @@ -269,9 +269,16 @@ services: # the application port 8443 via /dev/tcp and never touches 9000, so it is unaffected by the # management-TLS switch. - # Network isolation (production pattern) + # Network isolation (production pattern). The alias is what lets a passthrough client reach + # this gateway under a tls.passthrough_sni name: the benchmark's ClientHello carries + # passthrough.test.example, so that name must resolve to THIS container for the accept-time + # SNI front listener to see a match and L4-relay to PASSTHROUGH_BACKEND. Map form is required + # here — the short-form list carries no aliases. The backend certificate already covers the + # name (SAN DNS:passthrough.test.example on the passthrough-backend service). networks: - - api-sheriff + api-sheriff: + aliases: + - passthrough.test.example # Production restart policy restart: unless-stopped @@ -562,11 +569,17 @@ services: # user is not an option because the uid is not known to compose, so it runs as root and # the result files stay removable by `mvn clean` (the parent directory is host-owned). user: "0:0" + # Value-less entries pass the host variable through only when it is set, so an unset + # variable stays unset in the container rather than becoming an empty string. PASSTHROUGH_SNI + # selects the passthrough_relay.js mode (mapped | empty) and is set per benchmark execution in + # benchmarks/pom.xml; without it here the selector never reaches k6 and both executions would + # run the same mode. environment: - BENCHMARK_VUS - BENCHMARK_DURATION - BENCHMARK_MAX_ERROR_RATE - GATEWAY_TARGET + - PASSTHROUGH_SNI networks: api-sheriff: From e1af85a0d30c2e62d3fb7395aaefb3f8d5c94bf5 Mon Sep 17 00:00:00 2001 From: cuioss oliver <23139298+cuioss@users.noreply.github.com> Date: Tue, 28 Jul 2026 15:36:16 +0200 Subject: [PATCH 2/9] docs(benchmarks): record session_mediated.js as deliberately unwired session_mediated.js sits in k6-scripts/ backing no Maven goal and appearing in no documentation, so a reader cannot tell "deliberately not run" from "forgotten". Its benchmark name sessionMediated is already a public constant with test coverage, so the gap is documentation, not dead code. Add a short "Deliberately unwired scripts" subsection naming the script, its benchmark name, and the reason verbatim: BFF session mediation belongs to PLAN-07A, which shipped the server-session variant and explicitly deferred its per-variant benchmark additions because benchmarks/** is PLAN-25's surface. The script itself is untouched and no thirteenth Maven goal is added. Co-Authored-By: Claude --- benchmarks/README.adoc | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/benchmarks/README.adoc b/benchmarks/README.adoc index 4f530026..2f0fd2e9 100644 --- a/benchmarks/README.adoc +++ b/benchmarks/README.adoc @@ -145,6 +145,18 @@ management interface has exactly one port — its config declares no `ssl-port` `insecure-requests` — so activating management TLS converted port 9000 itself to HTTPS. No plain-HTTP surface remains anywhere in the stack. +=== Deliberately unwired scripts + +`session_mediated.js` (benchmark name `sessionMediated`) is present in `k6-scripts/` but is wired to +*no* Maven execution, **on purpose**: BFF session mediation belongs to PLAN-07A, which shipped the +server-session variant and explicitly deferred its per-variant benchmark additions because +`benchmarks/**` is PLAN-25's surface. The script is therefore ready but unclaimed, not forgotten. + +It is listed in the CI end-of-job coverage summary under its own skip section, so its absence from a +run is stated in every job rather than reading as a silent omission. Do not treat a missing +`sessionMediated-summary.json` as a regression, and do not wire the script to a Maven goal without +the owning plan. + == Running the CI baseline lane [source,bash] From 70c6b3069fa80ff3cf8188fb9df4fc4c4cd1e36a Mon Sep 17 00:00:00 2001 From: cuioss oliver <23139298+cuioss@users.noreply.github.com> Date: Tue, 28 Jul 2026 15:38:32 +0200 Subject: [PATCH 3/9] ci(benchmarks): fail the job when an expected benchmark never ran Maven fails fast, so a goal that errors aborts every goal after it and a goal that is never reached leaves no trace in the job output. The job's only artefact check was an `ls` that asserts nothing about what SHOULD be present, so a suite that executed 3 of 12 goals rendered exactly like one that executed all 12 -- which is how goals 5-12 stayed unexecuted and unnoticed. Add an `if: always()` "Summarise benchmark coverage" step that diffs the goals expected to write a summary against the documents k6 actually wrote, renders a ran / DID NOT RUN table into $GITHUB_STEP_SUMMARY, and exits non-zero when a summary is absent while the benchmark step itself reported success. When Maven already failed the step stays green so it never masks the actionable error. Deliberately skipped goals are named with their reason instead of omitted, so "not run" is never indistinguishable from "forgotten": uploadLarge and websocketEcho (both blocked on gateway-side defects) and sessionMediated (owned by PLAN-07A). Verified locally against a synthetic results tree: all present -> exit 0; one missing with a green benchmark step -> exit 1; one missing with a failed benchmark step -> exit 0. Each expected name renders exactly once. Co-Authored-By: Claude --- .github/workflows/benchmark.yml | 57 +++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/.github/workflows/benchmark.yml b/.github/workflows/benchmark.yml index 11a647e2..b6b6f877 100644 --- a/.github/workflows/benchmark.yml +++ b/.github/workflows/benchmark.yml @@ -84,6 +84,7 @@ jobs: --output-dir "${GITHUB_WORKSPACE}/benchmark-history" - name: Run Integration Benchmarks with k6 + id: run-benchmarks run: | # Run k6-based integration benchmarks with native image. The -Pbenchmark profile wires # eight executions: the six matrix aspects (unauth, bearer, http2, graphql, upload-1MB, @@ -97,6 +98,62 @@ jobs: echo "Integration benchmark artifacts generated:" ls -la benchmarks/target/benchmark-results/ + # Maven fails fast, so a goal that errors aborts every goal after it and a goal that is never + # reached leaves no trace in the job output — a suite that ran 3 of 12 goals renders exactly + # like one that ran all 12. This step diffs the goals that MUST produce a summary document + # against the documents k6 actually wrote, so a partial suite is visibly partial and a goal + # that silently vanished fails the job. Deliberately skipped goals are named with their reason + # rather than omitted, so "not run" is never indistinguishable from "forgotten". + - name: Summarise benchmark coverage + if: always() + env: + BENCHMARK_OUTCOME: ${{ steps.run-benchmarks.outcome }} + run: | + set -uo pipefail + results_dir=benchmarks/target/benchmark-results/k6 + + # Goals expected to write a summary. A goal settled as deliberately skipped is NOT listed + # here — it belongs in the skip table below, which is what keeps the two states distinct. + expected="healthLiveCheck gatewayHealth proxiedStatic passthroughRelay passthroughRelayEmpty bearerProxied http2 graphql uploadSmall grpcUnary" + + missing=0 + { + echo "## Benchmark coverage" + echo + echo "| Benchmark | Result |" + echo "| --- | --- |" + } >> "$GITHUB_STEP_SUMMARY" + + for name in ${expected}; do + if [ -f "${results_dir}/${name}-summary.json" ]; then + echo "| \`${name}\` | ran |" >> "$GITHUB_STEP_SUMMARY" + else + echo "| \`${name}\` | **DID NOT RUN** |" >> "$GITHUB_STEP_SUMMARY" + missing=$((missing + 1)) + fi + done + + { + echo + echo "### Deliberately skipped" + echo + echo "| Benchmark | Reason |" + echo "| --- | --- |" + echo "| \`uploadLarge\` | The gateway rejects the 50MB body with 413: the Quarkus HTTP body limit is not derived from the upload anchor's 64 MiB \`max_body_bytes\`, so the declared cap is unreachable. Needs a gateway-side fix. |" + echo "| \`websocketEcho\` | Each WebSocket upgrade leaks an edge admission permit; once exhausted the gateway rejects all traffic with 503 until restarted, which also poisons every goal after it. Needs a gateway-side fix. |" + echo "| \`sessionMediated\` | Wired to no Maven goal on purpose — BFF session mediation belongs to PLAN-07A. |" + } >> "$GITHUB_STEP_SUMMARY" + + # Only fail on a missing summary when Maven itself reported success: when the benchmark + # step already failed, its own error is the actionable signal and this step must not + # mask it with a derived one. + if [ "${missing}" -gt 0 ] && [ "${BENCHMARK_OUTCOME}" = "success" ]; then + echo "::error::${missing} expected benchmark summary document(s) absent although the benchmark step succeeded." + exit 1 + fi + + echo "Benchmark coverage: ${missing} expected summary document(s) absent (benchmark step outcome: ${BENCHMARK_OUTCOME})." + - name: Assemble benchmark artifacts for deployment run: | python3 benchmarks/scripts/benchmark-pages.py assemble \ From 33a091aa6dfe5704e94d023049edc256ea21c149 Mon Sep 17 00:00:00 2001 From: cuioss oliver <23139298+cuioss@users.noreply.github.com> Date: Tue, 28 Jul 2026 15:41:17 +0200 Subject: [PATCH 4/9] docs(benchmarks): state which goals run, which are skipped, and why benchmarks/README.adoc documented the lane as designed rather than as it runs. Nothing stated that any goal had never executed, and three counts contradicted benchmarks/pom.xml. Add a "Which goals actually run" section giving every one of the twelve goals a disposition: runs, skipped with the blocking reason, or unwired. uploadLarge and websocketEcho are named as skipped with their gateway-side root causes rather than quietly omitted, matching the skip list the new CI coverage step renders. Reconcile the stale counts: --aspects said "six matrix aspects" where the matrix has eight, and the wrk-to-k6 coverage note said the lane grew "to eight" where the pom declares twelve. Add the distinction that being wired is not the same as executing, which is precisely how goals 5-12 stayed unrun and unnoticed. Extend the discontinuity note to name two further causes separately so they cannot be conflated: goals executing for the first time under this plan (a coverage cause), and the forthcoming PLAN-31 D6/D7 %it elimination (a configuration cause, at a later commit, on the same names). Three-layer documentation verdicts, per the standing clause: - benchmarks/README.adoc: CHANGED (this commit). - doc/user/: NO CHANGE REQUIRED -- zero benchmark references; nothing an operator sees changes. - doc/development/: NO CHANGE REQUIRED -- its three benchmark references are structural (module name, build commands, BFF entry points); none is altered. Co-Authored-By: Claude --- benchmarks/README.adoc | 69 +++++++++++++++++++++++++++++++++++++++--- 1 file changed, 65 insertions(+), 4 deletions(-) diff --git a/benchmarks/README.adoc b/benchmarks/README.adoc index 2f0fd2e9..ebf2e906 100644 --- a/benchmarks/README.adoc +++ b/benchmarks/README.adoc @@ -145,6 +145,53 @@ management interface has exactly one port — its config declares no `ssl-port` `insecure-requests` — so activating management TLS converted port 9000 itself to HTTPS. No plain-HTTP surface remains anywhere in the stack. +=== Which goals actually run + +The CI baseline lane declares twelve goals. Until PLAN-25 it had never executed all of them: the +job died at goal 4 (`passthroughRelay`, whose target hostname resolved nowhere) and Maven's +fail-fast discarded goals 5-12, which had therefore never run in CI even once. Being declared in +`pom.xml` is not evidence that a goal produces a number. + +Current disposition — the CI end-of-job coverage step (see `.github/workflows/benchmark.yml`) +enforces this table, so a goal that silently stops running fails the job: + +[cols="2,1,4", options="header"] +|=== +|Benchmark |State |Notes + +|`healthLiveCheck`, `gatewayHealth`, `proxiedStatic` +|runs +|Already executing before PLAN-25. + +|`passthroughRelay`, `passthroughRelayEmpty` +|runs +|Repaired by PLAN-25: the relay target now resolves, and the `mapped`/`empty` selector actually + reaches the container, so the two modes are genuinely distinct runs. + +|`bearerProxied`, `http2`, `graphql`, `uploadSmall`, `grpcUnary` +|runs +|First execution in CI under PLAN-25. Their trend series start empty here. + +|`uploadLarge` +|*skipped* +|The gateway rejects the 50MB body with `413`. The `upload` anchor declares + `max_body_bytes: 67108864`, but the Quarkus HTTP body limit is not derived from it, so the + declared 64 MiB cap is unreachable and the body is refused before the security filter runs. + Blocked on a gateway-side fix. + +|`websocketEcho` +|*skipped* +|Each WebSocket upgrade leaks an edge admission permit. Once they are exhausted the gateway rejects + *all* traffic with `503` until restarted, which also poisons every goal sequenced after it. + Blocked on a gateway-side fix. + +|`sessionMediated` +|*unwired* +|Backs no Maven goal on purpose — see below. +|=== + +A skipped goal is named in every job summary with its reason. Silence is never the signal. + === Deliberately unwired scripts `session_mediated.js` (benchmark name `sessionMediated`) is present in `k6-scripts/` but is wired to @@ -351,7 +398,7 @@ scripts/run-comparison.sh --target apisix |`api-sheriff` (default), `apisix`, or `both`. |`--aspects` -|Comma-separated subset of the six matrix aspects. Default: all six. An unknown aspect fails +|Comma-separated subset of the eight matrix aspects. Default: all eight. An unknown aspect fails before any run starts. |`--output` @@ -439,8 +486,10 @@ know: . *Post-processing*: `WrkBenchmarkConverter` / `WrkResultPostProcessor` are replaced by `K6BenchmarkConverter` / `K6ResultPostProcessor`. The downstream report pipeline (badges, 10-run history, trends, GitHub Pages) consumes an identically-shaped model and is unchanged. -. *Coverage*: the lane grew from three wired executions to eight. `bearer` is genuinely new CI +. *Coverage*: the lane grew from three wired executions to twelve. `bearer` is genuinely new CI coverage — the wrk-era bearer runner existed on disk but was registered in no Maven execution. + Being *wired* is not the same as *executing*: until PLAN-25 the job died at the fourth goal and + goals 5-12 had never run in CI even once. See _Which goals actually run_ below. === One-time gh-pages baseline restart @@ -453,8 +502,20 @@ incomparable, the k6 series starts fresh rather than continuing the wrk series: discontinuity* at the swap commit that is a toolchain artifact, not a regression. * The `error` / variability / confidence-band columns go permanently zero-width from that point (see the `latency_ms.stdev` limit above). -* The four new matrix aspects begin with no history and need ten runs before their trend series is - meaningful. +* Every benchmark executing for the first time begins with *no history* and needs ten runs before + its trend series is meaningful. Under PLAN-25 that is most of the lane: eight of the twelve goals + had never executed in CI, so their series start empty at this commit. A flat or short trend line + on those names is an absence of history, not a regression. + +*Two further discontinuities are coming, and they are distinct causes — do not conflate them:* + +. *First execution under PLAN-25* (this commit): goals that had never run begin their series here. + The cause is coverage — data that never existed now exists. +. *PLAN-31 D6/D7 (`%it` elimination)*: that work will change the measured configuration itself, so + the numbers before and after are measurements of different things on the *same* names. The cause + is configuration, not coverage, and it will break continuity again at a later commit. + +A reader comparing across either boundary is comparing incomparable eras. Compare only within one. ==== A second, target-change discontinuity From 8a51c0953cfea50dc3402e39d023ba3646767879 Mon Sep 17 00:00:00 2001 From: cuioss oliver <23139298+cuioss@users.noreply.github.com> Date: Tue, 28 Jul 2026 15:43:50 +0200 Subject: [PATCH 5/9] docs(benchmarks): correct two comments that describe a lane and a topology that do not exist Both comments assert facts contradicted by the code beside them. benchmark.yml claimed in two places that the lane runs eight executions -- the timeout-minutes rationale and the run-step comment -- while benchmarks/pom.xml declares twelve k6 goals. The wrong count was load-bearing: it was the stated justification for the 75-minute cap. Restate both against the real composition (eight matrix aspects, two retained health benchmarks, two passthrough-relay executions) and rebuild the timeout rationale from the wall time measured locally on 2026-07-28 rather than from an estimate, naming the four native gateway instances the lane has booted since #118. The cap itself stays at 75: the measured run does not approach it. passthrough_relay.js documented PASSTHROUGH_TARGET_URL by pointing at the benchmark compose overlay "(D4)" for both the SNI mapping and the TLS backend. The overlay provides neither -- it adds only nginx-static and an upstream override. Name the three real sources instead: the network alias in the base docker-compose.yml, tls.passthrough_sni in gateway.yaml, and the passthrough-backend service in the base compose file. Comment-only on both files: the PASSTHROUGH_TARGET_URL value is unchanged and the coverage step added by the previous commit is intact. Co-Authored-By: Claude --- .github/workflows/benchmark.yml | 32 ++++++++++++++----- .../resources/k6-scripts/passthrough_relay.js | 14 ++++++-- 2 files changed, 35 insertions(+), 11 deletions(-) diff --git a/.github/workflows/benchmark.yml b/.github/workflows/benchmark.yml index b6b6f877..b5bc1f86 100644 --- a/.github/workflows/benchmark.yml +++ b/.github/workflows/benchmark.yml @@ -24,11 +24,24 @@ jobs: # Only run on merged PRs, not just closed ones if: github.event_name != 'pull_request' || github.event.pull_request.merged == true # Add timeout to prevent long-running jobs (increased for integration benchmarks). - # Raised 45 -> 75 for the k6 aspect matrix: the lane went from three wired 60-second runs to - # eight (the six matrix aspects plus the two retained health benchmarks). The dominant - # contributor beyond the old budget is the upload-50MB aspect, which is transfer-bound and - # runs at reduced concurrency, so it does not finish in the same wall time as a request-rate - # run. The five additional runs land inside this cap. + # The -Pbenchmark profile wires TWELVE k6 goals: the eight cross-gateway matrix aspects, the two + # retained non-matrix health benchmarks (healthLiveCheck, gatewayHealth), and the two + # API-Sheriff-only passthrough-relay executions (mapped relay throughput and the empty-mode + # no-regression run). + # + # Budget, measured locally 2026-07-28 rather than estimated: + # * goal execution ~14 min -- 12 goals x (60s k6 window + ~10s compose/k6 start and summary + # write). upload-50MB is transfer-bound at reduced concurrency, so it does + # not finish in the same wall time as a request-rate run. + # * stack startup ~90 s -- since #118 the lane boots FOUR native gateway instances + # (api-sheriff, api-sheriff-mtls, api-sheriff-cookie, api-sheriff-cookie-2) + # alongside Keycloak, go-httpbin, nginx-static, passthrough-backend, + # grpc-echo, toxiproxy, asset-origin and prometheus, because + # start-integration-container.sh runs a bare `up -d`. + # * native compile the dominant and most variable term on a cold cache. + # + # 75 minutes holds with headroom for the cold native compile; it is kept unchanged because the + # measured run does not approach the cap. timeout-minutes: 75 permissions: # Needed to upload artifacts @@ -87,9 +100,12 @@ jobs: id: run-benchmarks run: | # Run k6-based integration benchmarks with native image. The -Pbenchmark profile wires - # eight executions: the six matrix aspects (unauth, bearer, http2, graphql, upload-1MB, - # upload-50MB) plus the two retained non-matrix health benchmarks (healthLiveCheck, - # gatewayHealth). The on-demand APISIX comparison lane is deliberately NOT run here. + # twelve executions: the eight cross-gateway matrix aspects (unauth, bearer, http2, + # graphql, upload-1MB, upload-50MB, ws, grpc), the two retained non-matrix health + # benchmarks (healthLiveCheck, gatewayHealth), and the two API-Sheriff-only + # passthrough-relay executions (mapped and empty). The on-demand APISIX comparison lane is + # deliberately NOT run here. Maven fails fast, so the coverage step below is what proves + # which goals actually produced a result. echo "Running k6 integration benchmarks with native Quarkus..." ./mvnw --no-transfer-progress clean verify -pl benchmarks -Pbenchmark \ -Dbenchmark.history.dir="${GITHUB_WORKSPACE}/benchmark-history/integration" diff --git a/benchmarks/src/main/resources/k6-scripts/passthrough_relay.js b/benchmarks/src/main/resources/k6-scripts/passthrough_relay.js index ad2d937e..5c9b3954 100644 --- a/benchmarks/src/main/resources/k6-scripts/passthrough_relay.js +++ b/benchmarks/src/main/resources/k6-scripts/passthrough_relay.js @@ -30,9 +30,17 @@ import { targetUrl } from './lib/target.js'; const MODE = (__ENV.PASSTHROUGH_SNI || 'mapped').toLowerCase(); /** - * The mapped SNI host the passthrough run's ClientHello carries. It must both resolve to the gateway - * container and be listed in `tls.passthrough_sni`; the benchmark compose overlay (D4) provides that - * mapping and the TLS-enabled backend behind it. Overridable so a run can target a different edge. + * The mapped SNI host the passthrough run's ClientHello carries. Three separate pieces must line up, + * and all three live in the BASE integration-test configuration -- not in the benchmark overlay: + * + * * `integration-tests/docker-compose.yml` puts `passthrough.test.example` in the `api-sheriff` + * service's network `aliases`, which is what makes the name resolve to the gateway container; + * * `integration-tests/src/main/docker/sheriff-config/gateway.yaml` lists it under + * `tls.passthrough_sni`, mapping it to the `PASSTHROUGH_BACKEND` topology alias; + * * the `passthrough-backend` service in that same base compose file is the TLS-enabled backend + * the relay dials, and its certificate carries the name as a SAN. + * + * Overridable so a run can target a different edge. */ const PASSTHROUGH_TARGET_URL = __ENV.PASSTHROUGH_TARGET_URL || 'https://passthrough.test.example:8443/get'; From c3f6c1bdb7f54e7ee25c66dc076e783b0474fbb0 Mon Sep 17 00:00:00 2001 From: cuioss oliver <23139298+cuioss@users.noreply.github.com> Date: Tue, 28 Jul 2026 17:20:10 +0200 Subject: [PATCH 6/9] fix(benchmarks): order the two skipped k6 goals last and split declared from result-producing Maven runs same-phase exec executions in POM declaration order, and the -Pbenchmark profile ended with uploadSmall, uploadLarge, websocketEcho, grpcUnary. uploadLarge fails (413) and websocketEcho poisons the gateway, so fail-fast aborted the build before grpcUnary was ever reached -- grpcUnary was structurally unreachable in CI despite being in the coverage step's expected set and green against a fresh gateway. Move run-k6-upload-large-benchmark and run-k6-websocket-echo-benchmark to the end of the profile's exec list, after run-k6-grpc-unary-benchmark. Pure reorder: no id, phase, goal, configuration, environmentVariables, timeout or script path changed, and no execution added or removed. Twelve k6 executions remain. Also correct the prose that conflated two states the coverage step already keeps apart: twelve goals are DECLARED, ten are expected to produce a CI result. uploadLarge and websocketEcho are declared but settled as deliberately skipped against open gateway defects, so they start no trend series, and the claim that all eight previously-unexecuted goals begin one was inaccurate for those two. The timeout budget stays sized on all twelve wired goals -- a goal skipped by disposition is still a wired Maven execution that consumes wall time before it fails -- and now says so. Addresses PR #123 review finding 0938d0. Co-Authored-By: Claude --- .github/workflows/benchmark.yml | 28 +++++++++++++++-------- benchmarks/README.adoc | 26 ++++++++++++++-------- benchmarks/pom.xml | 39 +++++++++++++++++++-------------- 3 files changed, 59 insertions(+), 34 deletions(-) diff --git a/.github/workflows/benchmark.yml b/.github/workflows/benchmark.yml index b5bc1f86..c9f80129 100644 --- a/.github/workflows/benchmark.yml +++ b/.github/workflows/benchmark.yml @@ -24,15 +24,22 @@ jobs: # Only run on merged PRs, not just closed ones if: github.event_name != 'pull_request' || github.event.pull_request.merged == true # Add timeout to prevent long-running jobs (increased for integration benchmarks). - # The -Pbenchmark profile wires TWELVE k6 goals: the eight cross-gateway matrix aspects, the two - # retained non-matrix health benchmarks (healthLiveCheck, gatewayHealth), and the two + # The -Pbenchmark profile DECLARES TWELVE k6 goals: the eight cross-gateway matrix aspects, the + # two retained non-matrix health benchmarks (healthLiveCheck, gatewayHealth), and the two # API-Sheriff-only passthrough-relay executions (mapped relay throughput and the empty-mode - # no-regression run). + # no-regression run). Being wired as a Maven execution is not the same as being + # result-producing: uploadLarge and websocketEcho are declared but settled as deliberately + # skipped against open gateway defects (see the "Deliberately skipped" table in the coverage + # step below), so the coverage step gates on TEN goals, not twelve. Both are ordered last in the + # profile so their fail-fast abort cannot mask a goal that would otherwise produce a result. # # Budget, measured locally 2026-07-28 rather than estimated: - # * goal execution ~14 min -- 12 goals x (60s k6 window + ~10s compose/k6 start and summary - # write). upload-50MB is transfer-bound at reduced concurrency, so it does - # not finish in the same wall time as a request-rate run. + # * goal execution ~14 min -- deliberately sized on all TWELVE WIRED goals rather than on the + # ten expected to produce a result, because a goal skipped by disposition is + # still a wired Maven execution that consumes wall time before it fails: + # 12 goals x (60s k6 window + ~10s compose/k6 start and summary write). + # upload-50MB is transfer-bound at reduced concurrency, so it does not + # finish in the same wall time as a request-rate run. # * stack startup ~90 s -- since #118 the lane boots FOUR native gateway instances # (api-sheriff, api-sheriff-mtls, api-sheriff-cookie, api-sheriff-cookie-2) # alongside Keycloak, go-httpbin, nginx-static, passthrough-backend, @@ -99,11 +106,14 @@ jobs: - name: Run Integration Benchmarks with k6 id: run-benchmarks run: | - # Run k6-based integration benchmarks with native image. The -Pbenchmark profile wires - # twelve executions: the eight cross-gateway matrix aspects (unauth, bearer, http2, + # Run k6-based integration benchmarks with native image. The -Pbenchmark profile declares + # twelve goals: the eight cross-gateway matrix aspects (unauth, bearer, http2, # graphql, upload-1MB, upload-50MB, ws, grpc), the two retained non-matrix health # benchmarks (healthLiveCheck, gatewayHealth), and the two API-Sheriff-only - # passthrough-relay executions (mapped and empty). The on-demand APISIX comparison lane is + # passthrough-relay executions (mapped and empty). Ten of the twelve are expected to + # produce a CI result; upload-50MB (uploadLarge) and ws (websocketEcho) are declared but + # settled as deliberately skipped against open gateway defects, and are sequenced last so + # their failure cannot mask a goal ahead of them. The on-demand APISIX comparison lane is # deliberately NOT run here. Maven fails fast, so the coverage step below is what proves # which goals actually produced a result. echo "Running k6 integration benchmarks with native Quarkus..." diff --git a/benchmarks/README.adoc b/benchmarks/README.adoc index ebf2e906..9837d0e5 100644 --- a/benchmarks/README.adoc +++ b/benchmarks/README.adoc @@ -486,10 +486,14 @@ know: . *Post-processing*: `WrkBenchmarkConverter` / `WrkResultPostProcessor` are replaced by `K6BenchmarkConverter` / `K6ResultPostProcessor`. The downstream report pipeline (badges, 10-run history, trends, GitHub Pages) consumes an identically-shaped model and is unchanged. -. *Coverage*: the lane grew from three wired executions to twelve. `bearer` is genuinely new CI +. *Coverage*: the lane grew from three wired executions to twelve *declared* goals, ten of which are + expected to produce a CI result — `uploadLarge` and `websocketEcho` are declared but settled as + deliberately skipped against open gateway defects. `bearer` is genuinely new CI coverage — the wrk-era bearer runner existed on disk but was registered in no Maven execution. - Being *wired* is not the same as *executing*: until PLAN-25 the job died at the fourth goal and - goals 5-12 had never run in CI even once. See _Which goals actually run_ below. + Being *wired* is not the same as *executing*, and *executing* is not the same as *producing a + result*: until PLAN-25 the job died at the fourth goal and goals 5-12 had never run in CI even + once, and the two skipped goals still execute and consume wall time but write no summary + document. See _Which goals actually run_ below. === One-time gh-pages baseline restart @@ -502,15 +506,19 @@ incomparable, the k6 series starts fresh rather than continuing the wrk series: discontinuity* at the swap commit that is a toolchain artifact, not a regression. * The `error` / variability / confidence-band columns go permanently zero-width from that point (see the `latency_ms.stdev` limit above). -* Every benchmark executing for the first time begins with *no history* and needs ten runs before - its trend series is meaningful. Under PLAN-25 that is most of the lane: eight of the twelve goals - had never executed in CI, so their series start empty at this commit. A flat or short trend line - on those names is an absence of history, not a regression. +* Every benchmark that writes a summary document for the first time begins with *no history* and + needs ten runs before its trend series is meaningful. Under PLAN-25 that is most of the lane: + eight of the twelve declared goals had never executed in CI, and six of those eight are expected + to produce a result and so start their series empty at this commit. `uploadLarge` and + `websocketEcho` start *no* series at all — only a goal that actually writes a summary document + opens one, and both are settled as deliberately skipped. The workflow's end-of-job coverage step + (`.github/workflows/benchmark.yml`) is the record of which goals produced a result in a given run. + A flat or short trend line on a name that *did* run is an absence of history, not a regression. *Two further discontinuities are coming, and they are distinct causes — do not conflate them:* -. *First execution under PLAN-25* (this commit): goals that had never run begin their series here. - The cause is coverage — data that never existed now exists. +. *First execution under PLAN-25* (this commit): goals that had never run and now produce a result + begin their series here. The cause is coverage — data that never existed now exists. . *PLAN-31 D6/D7 (`%it` elimination)*: that work will change the measured configuration itself, so the numbers before and after are measurements of different things on the *same* names. The cause is configuration, not coverage, and it will break continuity again at a later commit. diff --git a/benchmarks/pom.xml b/benchmarks/pom.xml index ba971a88..17886365 100644 --- a/benchmarks/pom.xml +++ b/benchmarks/pom.xml @@ -506,10 +506,11 @@ - + - run-k6-upload-large-benchmark + run-k6-grpc-unary-benchmark integration-test exec @@ -522,23 +523,29 @@ --rm k6 run - /scripts/upload_large.js + /scripts/grpc_unary.js ${integration.compose.dir} 240000 - ${k6.vus.upload.large} + ${k6.vus} ${k6.duration} ${k6.output.dir} - + + + - run-k6-websocket-echo-benchmark + run-k6-upload-large-benchmark integration-test exec @@ -551,23 +558,23 @@ --rm k6 run - /scripts/websocket_echo.js + /scripts/upload_large.js ${integration.compose.dir} 240000 - ${k6.vus} + ${k6.vus.upload.large} ${k6.duration} ${k6.output.dir} - + - run-k6-grpc-unary-benchmark + run-k6-websocket-echo-benchmark integration-test exec @@ -580,7 +587,7 @@ --rm k6 run - /scripts/grpc_unary.js + /scripts/websocket_echo.js ${integration.compose.dir} 240000 From 6c38e617a108d626b2828f89e7e7ddb5f84806f8 Mon Sep 17 00:00:00 2001 From: cuioss oliver <23139298+cuioss@users.noreply.github.com> Date: Tue, 28 Jul 2026 17:45:32 +0200 Subject: [PATCH 7/9] fix(benchmarks): actually skip the two blocked k6 goals instead of documenting it The suite documented uploadLarge and websocketEcho as skipped but implemented no skip: both remained unconditional exec executions, so uploadLarge still breached its threshold at goal 11, Maven still aborted fail-fast, websocketEcho at goal 12 still never ran, and the Performance Benchmark job was still red. The earlier reorder moved the failure from goal 10 to goal 11; it did not remove it. Give each blocked goal a real bound to its own property, both declared true in benchmarks/pom.xml with the id of the gateway finding that blocks it (0562be for uploadLarge, 218b5c for websocketEcho) and the flag that re-enables it. The reorder is kept as defense-in-depth: if a skip property is ever flipped to false, a resulting abort still cannot discard goals that would otherwise pass. Also make a truncated suite reportable. The coverage guard could only hard-fail when Maven had SUCCEEDED, so in the case that actually occurred - Maven fails and truncates the suite - the branch was suppressed by design and the reader saw a Maven error beside a table of DID NOT RUN rows with nothing stating the two were the same event. Add, on the failure path, an explicit statement distinguishing a truncated suite from a failure after every goal produced a summary. The existing missing>0 AND success hard-fail branch is left intact, and the new path does not exit non-zero: the Maven step's own failure already fails the job. Correct the prose in benchmark.yml and README.adoc that claimed the skipped goals still execute and consume wall time - false once they are genuinely skipped - including the timeout-budget rationale, which is now sized on the ten goals that actually execute rather than on twelve wired ones. The two gateway defects themselves remain reported, not fixed. --- .github/workflows/benchmark.yml | 82 ++++++++++++++++++++++----------- benchmarks/README.adoc | 48 ++++++++++++------- benchmarks/pom.xml | 40 +++++++++++++--- 3 files changed, 119 insertions(+), 51 deletions(-) diff --git a/.github/workflows/benchmark.yml b/.github/workflows/benchmark.yml index c9f80129..5dbd8045 100644 --- a/.github/workflows/benchmark.yml +++ b/.github/workflows/benchmark.yml @@ -27,19 +27,22 @@ jobs: # The -Pbenchmark profile DECLARES TWELVE k6 goals: the eight cross-gateway matrix aspects, the # two retained non-matrix health benchmarks (healthLiveCheck, gatewayHealth), and the two # API-Sheriff-only passthrough-relay executions (mapped relay throughput and the empty-mode - # no-regression run). Being wired as a Maven execution is not the same as being - # result-producing: uploadLarge and websocketEcho are declared but settled as deliberately - # skipped against open gateway defects (see the "Deliberately skipped" table in the coverage - # step below), so the coverage step gates on TEN goals, not twelve. Both are ordered last in the - # profile so their fail-fast abort cannot mask a goal that would otherwise produce a result. + # no-regression run). TEN of them execute: uploadLarge and websocketEcho each carry a + # bound to a property that defaults to true in benchmarks/pom.xml, because each is blocked by an + # open gateway defect (see the "Deliberately skipped" table in the coverage step below). Maven + # therefore never runs them, so they consume no wall time and cannot abort the suite. Both are + # still ordered last in the profile as defense-in-depth, so that flipping a skip property to + # false cannot mask a goal that would otherwise produce a result. # - # Budget, measured locally 2026-07-28 rather than estimated: - # * goal execution ~14 min -- deliberately sized on all TWELVE WIRED goals rather than on the - # ten expected to produce a result, because a goal skipped by disposition is - # still a wired Maven execution that consumes wall time before it fails: - # 12 goals x (60s k6 window + ~10s compose/k6 start and summary write). - # upload-50MB is transfer-bound at reduced concurrency, so it does not - # finish in the same wall time as a request-rate run. + # Budget, from per-goal timings measured locally 2026-07-28 rather than estimated: + # * goal execution ~12 min -- sized on the TEN goals that actually execute, which is now the + # real basis: a skipped goal is not run at all, so it costs nothing but the + # Maven bookkeeping for a skipped execution. + # 10 goals x (60s k6 window + ~10s compose/k6 start and summary write). + # Re-enabling a skipped goal costs another ~70s, and upload-50MB more than + # that: it is transfer-bound at reduced concurrency, so it does not finish + # in the same wall time as a request-rate run. Re-derive this term when a + # skip property is flipped to false. # * stack startup ~90 s -- since #118 the lane boots FOUR native gateway instances # (api-sheriff, api-sheriff-mtls, api-sheriff-cookie, api-sheriff-cookie-2) # alongside Keycloak, go-httpbin, nginx-static, passthrough-backend, @@ -110,12 +113,12 @@ jobs: # twelve goals: the eight cross-gateway matrix aspects (unauth, bearer, http2, # graphql, upload-1MB, upload-50MB, ws, grpc), the two retained non-matrix health # benchmarks (healthLiveCheck, gatewayHealth), and the two API-Sheriff-only - # passthrough-relay executions (mapped and empty). Ten of the twelve are expected to - # produce a CI result; upload-50MB (uploadLarge) and ws (websocketEcho) are declared but - # settled as deliberately skipped against open gateway defects, and are sequenced last so - # their failure cannot mask a goal ahead of them. The on-demand APISIX comparison lane is - # deliberately NOT run here. Maven fails fast, so the coverage step below is what proves - # which goals actually produced a result. + # passthrough-relay executions (mapped and empty). Ten of the twelve execute and are + # expected to produce a CI result; upload-50MB (uploadLarge) and ws (websocketEcho) are + # skipped by property against open gateway defects, so Maven does not run them and they + # cannot abort the suite. The on-demand APISIX comparison lane is deliberately NOT run + # here. Maven still fails fast on the goals that do run, so the coverage step below + # remains what proves which goals actually produced a result. echo "Running k6 integration benchmarks with native Quarkus..." ./mvnw --no-transfer-progress clean verify -pl benchmarks -Pbenchmark \ -Dbenchmark.history.dir="${GITHUB_WORKSPACE}/benchmark-history/integration" @@ -125,11 +128,13 @@ jobs: ls -la benchmarks/target/benchmark-results/ # Maven fails fast, so a goal that errors aborts every goal after it and a goal that is never - # reached leaves no trace in the job output — a suite that ran 3 of 12 goals renders exactly - # like one that ran all 12. This step diffs the goals that MUST produce a summary document - # against the documents k6 actually wrote, so a partial suite is visibly partial and a goal - # that silently vanished fails the job. Deliberately skipped goals are named with their reason - # rather than omitted, so "not run" is never indistinguishable from "forgotten". + # reached leaves no trace in the job output — a suite that ran 3 of the 10 executing goals + # renders exactly like one that ran all 10. This step diffs the goals that MUST produce a + # summary document against the documents k6 actually wrote, so a partial suite is visibly + # partial and a goal that silently vanished fails the job. When Maven failed, it also states + # in words whether the suite was truncated by that failure. Skipped goals are named with their + # reason and their skip property rather than omitted, so "not run" is never + # indistinguishable from "forgotten". - name: Summarise benchmark coverage if: always() env: @@ -163,11 +168,11 @@ jobs: echo echo "### Deliberately skipped" echo - echo "| Benchmark | Reason |" - echo "| --- | --- |" - echo "| \`uploadLarge\` | The gateway rejects the 50MB body with 413: the Quarkus HTTP body limit is not derived from the upload anchor's 64 MiB \`max_body_bytes\`, so the declared cap is unreachable. Needs a gateway-side fix. |" - echo "| \`websocketEcho\` | Each WebSocket upgrade leaks an edge admission permit; once exhausted the gateway rejects all traffic with 503 until restarted, which also poisons every goal after it. Needs a gateway-side fix. |" - echo "| \`sessionMediated\` | Wired to no Maven goal on purpose — BFF session mediation belongs to PLAN-07A. |" + echo "| Benchmark | Skipped by | Reason |" + echo "| --- | --- | --- |" + echo "| \`uploadLarge\` | \`skip.benchmark.upload.large\` (defaults to \`true\`) | The gateway rejects the 50MB body with 413: the Quarkus HTTP body limit is not derived from the upload anchor's 64 MiB \`max_body_bytes\`, so the declared cap is unreachable. Needs a gateway-side fix; re-enable with \`-Dskip.benchmark.upload.large=false\`. |" + echo "| \`websocketEcho\` | \`skip.benchmark.websocket.echo\` (defaults to \`true\`) | Each WebSocket upgrade leaks an edge admission permit; once exhausted the gateway rejects all traffic with 503 until restarted, which would also poison every goal after it. Needs a gateway-side fix; re-enable with \`-Dskip.benchmark.websocket.echo=false\`. |" + echo "| \`sessionMediated\` | not wired | Wired to no Maven goal on purpose — BFF session mediation belongs to PLAN-07A. |" } >> "$GITHUB_STEP_SUMMARY" # Only fail on a missing summary when Maven itself reported success: when the benchmark @@ -178,6 +183,27 @@ jobs: exit 1 fi + # When Maven failed, the DID NOT RUN rows above and the Maven error are almost always the + # same event, but nothing on the page says so — a reader is left to infer the causal link. + # State it. This path never exits non-zero: the benchmark step's own failure already fails + # the job, and duplicating that here would only bury the real error. + if [ "${BENCHMARK_OUTCOME}" != "success" ]; then + missing_names="" + for name in ${expected}; do + if [ ! -f "${results_dir}/${name}-summary.json" ]; then + missing_names="${missing_names:+${missing_names}, }${name}" + fi + done + { + echo + if [ "${missing}" -gt 0 ]; then + echo "> **Maven failed and the suite was TRUNCATED**: goals ${missing_names} produced no summary because the run aborted before reaching them. The DID NOT RUN rows above are a consequence of that failure, not ${missing} independent problems — fix the reported Maven error first, then re-read this table." + else + echo "> **Maven failed AFTER every expected goal produced a summary.** No goal was lost to truncation, so the failure lies outside goal execution (post-processing, the baseline comparison, or container teardown). The results above are complete and usable." + fi + } >> "$GITHUB_STEP_SUMMARY" + fi + echo "Benchmark coverage: ${missing} expected summary document(s) absent (benchmark step outcome: ${BENCHMARK_OUTCOME})." - name: Assemble benchmark artifacts for deployment diff --git a/benchmarks/README.adoc b/benchmarks/README.adoc index 9837d0e5..5026f22a 100644 --- a/benchmarks/README.adoc +++ b/benchmarks/README.adoc @@ -174,23 +174,38 @@ enforces this table, so a goal that silently stops running fails the job: |`uploadLarge` |*skipped* -|The gateway rejects the 50MB body with `413`. The `upload` anchor declares - `max_body_bytes: 67108864`, but the Quarkus HTTP body limit is not derived from it, so the - declared 64 MiB cap is unreachable and the body is refused before the security filter runs. - Blocked on a gateway-side fix. +|Skipped by `skip.benchmark.upload.large`, which defaults to `true`. The gateway rejects the 50MB + body with `413`: the `upload` anchor declares `max_body_bytes: 67108864`, but the Quarkus HTTP body + limit is not derived from it, so the declared 64 MiB cap is unreachable and the body is refused + before the security filter runs. Blocked on a gateway-side fix. |`websocketEcho` |*skipped* -|Each WebSocket upgrade leaks an edge admission permit. Once they are exhausted the gateway rejects - *all* traffic with `503` until restarted, which also poisons every goal sequenced after it. - Blocked on a gateway-side fix. +|Skipped by `skip.benchmark.websocket.echo`, which defaults to `true`. Each WebSocket upgrade leaks + an edge admission permit; once they are exhausted the gateway rejects *all* traffic with `503` + until restarted, which would also poison every goal sequenced after it. Blocked on a gateway-side + fix. |`sessionMediated` |*unwired* |Backs no Maven goal on purpose — see below. |=== -A skipped goal is named in every job summary with its reason. Silence is never the signal. +*Skipped means not executed.* Each skipped goal carries a `` element in `benchmarks/pom.xml` +bound to its own property, declared there with the id of the finding that blocks it and defaulting +to `true` — so Maven does not run the goal, it consumes no wall time, and it cannot abort the suite. +The two states are kept distinct rather than collapsed: the CI coverage step gates on the ten goals +that execute and names each skipped goal, its skip property and its reason in the job summary. When +Maven fails, that step also states in words whether the failure truncated the suite. Silence is +never the signal. + +Re-enable a goal once its gateway defect is fixed by flipping its property, which is also how you +reproduce the defect through the harness: + +[source,bash] +---- +./mvnw clean verify -pl benchmarks -Pbenchmark -Dskip.benchmark.upload.large=false +---- === Deliberately unwired scripts @@ -486,14 +501,13 @@ know: . *Post-processing*: `WrkBenchmarkConverter` / `WrkResultPostProcessor` are replaced by `K6BenchmarkConverter` / `K6ResultPostProcessor`. The downstream report pipeline (badges, 10-run history, trends, GitHub Pages) consumes an identically-shaped model and is unchanged. -. *Coverage*: the lane grew from three wired executions to twelve *declared* goals, ten of which are - expected to produce a CI result — `uploadLarge` and `websocketEcho` are declared but settled as - deliberately skipped against open gateway defects. `bearer` is genuinely new CI - coverage — the wrk-era bearer runner existed on disk but was registered in no Maven execution. - Being *wired* is not the same as *executing*, and *executing* is not the same as *producing a - result*: until PLAN-25 the job died at the fourth goal and goals 5-12 had never run in CI even - once, and the two skipped goals still execute and consume wall time but write no summary - document. See _Which goals actually run_ below. +. *Coverage*: the lane grew from three wired executions to twelve *declared* goals, ten of which + execute and are expected to produce a CI result — `uploadLarge` and `websocketEcho` are declared + but skipped by property against open gateway defects, so Maven does not run them at all. `bearer` + is genuinely new CI coverage — the wrk-era bearer runner existed on disk but was registered in no + Maven execution. Being *wired* is not the same as *executing*, and *executing* is not the same as + *producing a result*: until PLAN-25 the job died at the fourth goal and goals 5-12 had never run + in CI even once. See _Which goals actually run_ above. === One-time gh-pages baseline restart @@ -511,7 +525,7 @@ incomparable, the k6 series starts fresh rather than continuing the wrk series: eight of the twelve declared goals had never executed in CI, and six of those eight are expected to produce a result and so start their series empty at this commit. `uploadLarge` and `websocketEcho` start *no* series at all — only a goal that actually writes a summary document - opens one, and both are settled as deliberately skipped. The workflow's end-of-job coverage step + opens one, and both are skipped by property rather than executed. The workflow's end-of-job coverage step (`.github/workflows/benchmark.yml`) is the record of which goals produced a result in a given run. A flat or short trend line on a name that *did* run is an absence of history, not a regression. diff --git a/benchmarks/pom.xml b/benchmarks/pom.xml index 17886365..003bfee3 100644 --- a/benchmarks/pom.xml +++ b/benchmarks/pom.xml @@ -28,6 +28,26 @@ true + + + + true + + + true + false @@ -535,12 +555,16 @@ - + @@ -551,6 +575,8 @@ exec + + ${skip.benchmark.upload.large} docker compose @@ -580,6 +606,8 @@ exec + + ${skip.benchmark.websocket.echo} docker compose From 781811b1d31c81ce868410f436acc12c4fab4017 Mon Sep 17 00:00:00 2001 From: cuioss oliver <23139298+cuioss@users.noreply.github.com> Date: Tue, 28 Jul 2026 18:00:16 +0200 Subject: [PATCH 8/9] chore(benchmarks): collapse the duplicate missing-summary scan in the coverage step The truncation-reporting branch re-scanned `expected` purely to rebuild `missing_names`, duplicating the predicate the primary coverage loop already evaluates. Accumulate `missing_names` alongside `missing` in that single loop and drop the second scan. Behaviour-preserving: same predicate, same branch, same value at the point of use. Co-Authored-By: Claude --- .github/workflows/benchmark.yml | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/.github/workflows/benchmark.yml b/.github/workflows/benchmark.yml index 5dbd8045..7b862118 100644 --- a/.github/workflows/benchmark.yml +++ b/.github/workflows/benchmark.yml @@ -148,6 +148,7 @@ jobs: expected="healthLiveCheck gatewayHealth proxiedStatic passthroughRelay passthroughRelayEmpty bearerProxied http2 graphql uploadSmall grpcUnary" missing=0 + missing_names="" { echo "## Benchmark coverage" echo @@ -161,6 +162,7 @@ jobs: else echo "| \`${name}\` | **DID NOT RUN** |" >> "$GITHUB_STEP_SUMMARY" missing=$((missing + 1)) + missing_names="${missing_names:+${missing_names}, }${name}" fi done @@ -188,12 +190,6 @@ jobs: # State it. This path never exits non-zero: the benchmark step's own failure already fails # the job, and duplicating that here would only bury the real error. if [ "${BENCHMARK_OUTCOME}" != "success" ]; then - missing_names="" - for name in ${expected}; do - if [ ! -f "${results_dir}/${name}-summary.json" ]; then - missing_names="${missing_names:+${missing_names}, }${name}" - fi - done { echo if [ "${missing}" -gt 0 ]; then From 2f794818470153f23de90b90c692117929a2d7b0 Mon Sep 17 00:00:00 2001 From: cuioss oliver <23139298+cuioss@users.noreply.github.com> Date: Tue, 28 Jul 2026 19:01:39 +0200 Subject: [PATCH 9/9] chore(architecture): record the stale-bot-re-review insight as a durable hint The preference-emitter promoted a recurring within-plan disposition: seven review-bot comments were accepted rather than actioned, and the decisive reason in the largest case was that the bot re-reported its original diff range on a later sweep, grading code it had never seen. Records that as an api-sheriff-parent insight so the next plan ground-truths a bot verdict against current HEAD before opening a fix task, and expects one self-ingested no-op finding per review sweep. Co-Authored-By: Claude --- .plan/project-architecture/api-sheriff-parent/enriched.json | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.plan/project-architecture/api-sheriff-parent/enriched.json b/.plan/project-architecture/api-sheriff-parent/enriched.json index 80161942..4d045c44 100644 --- a/.plan/project-architecture/api-sheriff-parent/enriched.json +++ b/.plan/project-architecture/api-sheriff-parent/enriched.json @@ -4,7 +4,8 @@ ], "insights": [ "CI aggregation gates (build/conclusion, integration-tests/conclusion) fail only as derivatives of their upstream jobs, and a red build/sonar-build with successful scanner auth means a genuine server-side SonarCloud new-code quality-gate verdict (fetchable without credentials via the public sonarcloud.io API) \u2014 triage the upstream cause, never the aggregation gate.", - "CI rollup 'conclusion' gate jobs (build / conclusion, integration-tests / conclusion) fail solely as mirrors of their failing child job; the project treats them as carrying no independent root cause and dispositions them taken_into_account, triaging only the child job's failure." + "CI rollup 'conclusion' gate jobs (build / conclusion, integration-tests / conclusion) fail solely as mirrors of their failing child job; the project treats them as carrying no independent root cause and dispositions them taken_into_account, triaging only the child job's failure.", + "Verify a review bot's reported diff range before trusting a re-review verdict. CodeRabbit re-reviews on this repo have re-reported the ORIGINAL commit range (e.g. base..first-head) on a later sweep, so the bot graded code it never saw and raised a Major-severity finding against an already-fixed defect. Ground-truth every bot claim against the current HEAD before opening a fix task. Related: a plan's own batched triage-reply comment is re-fetched as a new pr-comment finding on the next sweep, so a review sweep never reaches a clean zero \u2014 expect one no-op self-ingested finding per pass." ], "internal_dependencies": [ "api-sheriff",