fix: stop rescanning the CA directory on every TLS handshake - #33
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe TLS context cache now distinguishes CA paths and certificate verification modes. Verified contexts load default and configured trust stores. Unverified contexts skip trust-store loading. Failed context initialization is not cached. ChangesTLS verification-aware contexts
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Request
participant TLSContextCache
participant SSLContext
participant X509Store
Request->>TLSContextCache: request context with CA path and verification mode
TLSContextCache->>TLSContextCache: match cached context
TLSContextCache->>SSLContext: create context when no match exists
SSLContext->>X509Store: load trust stores when verification is enabled
X509Store-->>SSLContext: return trust-source results
SSLContext-->>TLSContextCache: return initialized context
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
ngx_http_ffi_client_ssl_ctx() called SSL_CTX_set_default_verify_paths() unconditionally. That registers the hashed CA directory as X509_FILETYPE_DEFAULT, and with DEFAULT OpenSSL enumerates the whole directory on *each* handshake instead of opening the single <hash>.N file the lookup needs. On a connection-per-request TLS workload the target worker spent 11.8% of its CPU in ext4 directory code against lua-resty-http's 0.4%, and 36.7% in the kernel against 17.9%. Under load that was one openat plus two getdents64 per request, sweeping 245 entries each time; lua-resty-http issued none. Reproduced independently of this module, against the benchmark upstream: default_verify_paths getdents64=4 explicit -CAfile bundle getdents64=0 explicit -CApath dir getdents64=0 Explicit CApath still verifies correctly when the issuer is present, so the filetype is what costs, not hash-dir lookup and not lookup failure. Two changes. The trust store is now installed by hand as X509_FILETYPE_PEM, honouring SSL_CERT_FILE/SSL_CERT_DIR the way OpenSSL does, so a verifying caller no longer pays the sweep either. And it is not installed at all unless the caller asked to verify: OpenSSL checks the chain during every handshake regardless of verify mode, but the handshake handler only reads the result when ssl_verify is set, so for everyone else the whole store was wasted work. The context cache is keyed on the verify flag as well as the CA path, since a verify-off context carries no store and would fail every chain a verify-on request handed it. ffi.tlsshort goes from 932 to 1257 req/s, median of 5 saturated repeats, which turns a 0.81x result against lua-resty-http into 1.10x. Pooled TLS is unchanged at 1.53x. The benchmark harness these numbers come from, and the tlsverify and tlsverifyshort cases that cover the verifying path, are in a companion PR; this commit touches only src/.
f70c6f9 to
4ee2c7e
Compare
There was a problem hiding this comment.
Pull request overview
This PR addresses a TLS performance regression in the outbound FFI HTTP client by avoiding OpenSSL’s expensive CA-directory enumeration on every handshake, and expands the benchmark harness to measure and report a fuller request-shape matrix (including new verify-on TLS cases) in a reproducible way.
Changes:
- Update SSL_CTX creation/caching to load the system trust store as PEM (not DEFAULT) and only when verification is enabled; cache contexts by CA path + verify flag.
- Replace the prior benchmark harness with a case-table-driven runner that generates Nginx locations, performs fairness audits, and writes structured CSV results that can be folded into README tables.
- Refresh benchmark documentation/results and add an APISIX ai-proxy integration contract/gap analysis doc.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| t/003-benchmark-runner.t | Extends tests to validate the new benchmark runner structure, case pairing, and generated configs/locations. |
| src/ngx_http_ffi_client.h | Extends SSL_CTX cache key structure to include the verify flag. |
| src/ngx_http_ffi_client_request.c | Implements trust-store loading as PEM and skips trust-store install unless verify is enabled; updates SSL_CTX cache keying. |
| README.md | Rewrites benchmark section to describe the new matrix-based methodology and updated headline figures. |
| docs/ai-proxy-integration.md | Adds an APISIX ai-proxy integration contract review and gap analysis for drop-in compatibility. |
| benchmark/run.sh | Replaces hardcoded benchmark cases with case-table-driven execution, adds audits/CSV output, and adds TLS cert generation and new phases. |
| benchmark/results-full.md | Adds a full-matrix results record and a TLS-focused re-measurement section after the trust-store fix. |
| benchmark/README.md | Major rewrite documenting benchmark topology, phases, metrics, fairness audit, and CSV folding workflow. |
| benchmark/lua/bench.lua | Introduces a structured driver/shape case table used by the benchmark runner and fairness audit endpoints. |
| benchmark/fold.sh | Adds a CSV-to-markdown folding tool to generate README-ready summary tables from raw runs. |
| benchmark/conf/upstream.nginx.conf | Expands upstream mock to cover TLS, chunked/framing variants, headers/cookies, echo/audit endpoints, and trailer stream mock. |
| benchmark/conf/target.nginx.conf | Updates target config to include generated case locations and benchmark endpoints (cases, audit, gcdelta). |
| benchmark/cases.txt | Adds the canonical case list used as the single source of truth for generated locations and smoke checks. |
| .gitignore | Updates ignore pattern to cover benchmark run directories (.run*). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for (node = fmcf->ssl_ctxs; node != NULL; node = node->next) { | ||
| /* guard memcmp: NULL pointers with len 0 are UB */ | ||
| if (node->ca.len == ca->len | ||
| if (node->verify == (verify ? 1u : 0u) | ||
| && node->ca.len == ca->len | ||
| && (ca->len == 0 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Nitpick comments (7)
docs/ai-proxy-integration.md (1)
18-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd language identifiers to both fenced blocks.
Use
textfor the call-flow and header excerpts so Markdown lint passes.Proposed fix
-``` +```text ... -``` +```textAlso applies to: 134-134
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/ai-proxy-integration.md` at line 18, Update both fenced code blocks in the documentation, including the call-flow and header excerpts, to specify the text language identifier after the opening fence. Leave their contents unchanged and ensure no affected block remains an unlabeled fence.Source: Linters/SAST tools
t/003-benchmark-runner.t (1)
64-81: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winValidate case names before grouping them.
%by_shapesilently overwrites duplicate entries and accepts unknown drivers. A malformedcases.txtcan pass the pairing test while the runner executes a duplicated or unsupported case. Validate full names againstffi|restyand reject duplicate entries before building%by_shape.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@t/003-benchmark-runner.t` around lines 64 - 81, Validate each case name in `@cases` before populating %by_shape: require the full name to match the expected ffi or resty driver format, reject unknown or malformed drivers, and detect duplicate full names instead of allowing silent overwrites. Keep the existing shape-pairing check after validation so only unique, supported cases are grouped.benchmark/fold.sh (2)
200-207: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePass the latency cells as
printfarguments rather than as a computed format string.Lines 203-205 build a string and pass it to
printfas the format. Any%that ever enters those strings is then interpreted as a conversion specifier. The current values are safe, but the construct is fragile and hides the format from a reader.♻️ Proposed change
- v = med_of("p50:" name); printf (v == "" ? " n/a |" : " `" sprintf("%.3f", v) "` ms |") - v = med_of("p99:" name); printf (v == "" ? " n/a |" : " `" sprintf("%.3f", v) "` ms |") - v = med_of("p999:" name); printf (v == "" ? " n/a |" : " `" sprintf("%.3f", v) "` ms |") + v = med_of("p50:" name); if (v == "") printf " n/a |"; else printf " `%.3f` ms |", v + v = med_of("p99:" name); if (v == "") printf " n/a |"; else printf " `%.3f` ms |", v + v = med_of("p999:" name); if (v == "") printf " n/a |"; else printf " `%.3f` ms |", v🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@benchmark/fold.sh` around lines 200 - 207, Update the latency output statements in the fold.sh benchmark loop to use fixed printf format strings with each computed latency value passed as an argument, rather than constructing the format string dynamically. Apply this to the p50, p99, and p999 cells while preserving the existing n/a handling and three-decimal formatting.
144-150: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove two leftover statements.
- Lines 147-148 pass an empty extra argument to a
printfwhose format has no conversion specifier. awk discards it, so the argument has no effect.- Line 218 is an empty
forloop. It also reusesn, which by that point holds the latency-table count rather than the throughput count, so the intent is unclear.♻️ Proposed cleanup
- printf "The outbound cost ratio subtracts that baseline from both ", - "" + printf "The outbound cost ratio subtracts that baseline from both " printf "client paths and compares what is left.\n"printf " excluded from every median above:\n\n" - for (i = 0; i < n; i++) { } for (name in discarded)Also applies to: 210-222
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@benchmark/fold.sh` around lines 144 - 150, Remove the unused empty-string argument from the baseline explanatory printf in the base != "" block, and delete the empty for loop around the later latency-table section that reuses n without performing work. Leave the surrounding output and calculations unchanged.benchmark/run.sh (2)
848-866: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff
median_qpsduplicates the median and saturation logic infold.sh.This awk block re-implements the bubble-sort median from
benchmark/fold.shlines 28-35 and repeats the saturation and non-2xx gate from lines 79-89 of that file. The two copies must stay in agreement, becauselatency_phasederives its fixed rate from this copy while the published tables come from the other one. A change to the gate in one place silently changes what the latency phase measures relative to what the tables report.Consider having
fold.shexpose a single-value query mode, and calling it from here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@benchmark/run.sh` around lines 848 - 866, Update median_qps in the benchmark runner to reuse fold.sh’s single-value query mode instead of maintaining its own awk filtering and median calculation; add or invoke that mode using the existing fold.sh gate and median logic so latency_phase and published tables use identical inputs and results.
236-245: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the certificate extensions explicit and preserve OpenSSL errors.
-x509can obtain extensions from the active OpenSSL configuration. This repository provides no such configuration, sobasicConstraints=CA:TRUEis not deterministic. Add-addext "basicConstraints=critical,CA:TRUE"and preserve stderr instead of redirecting it to/dev/null.-addextrequires OpenSSL 1.1.1 or newer. A failed TLS audit is excluded from the matrix whenBENCH_STRICT=0.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@benchmark/run.sh` around lines 236 - 245, Update generate_cert to add the explicit critical basicConstraints extension with CA:TRUE alongside the existing subjectAltName extension, and stop redirecting OpenSSL stderr so failures retain their diagnostic output; keep the existing die handling and note that -addext requires OpenSSL 1.1.1 or newer.benchmark/conf/upstream.nginx.conf (1)
147-162: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe reuse audit has no coverage for trailer cases.
/statsreports only counters incremented bybenchmark_count(), which exists only in thehttpblock. Thestreamtrailer mock never increments them. For a trailer case,upstream_stat connectionsandupstream_stat requestsboth return0, sorun.shcomputes an emptyrequests_per_connectionand skips the keepalive reuse warning at run.sh lines 628-638. A trailer case that stops reusing its pool therefore passes the audit silently.Add stream-side counting so the trailer port contributes to the same counters.
♻️ Proposed change to count stream requests
Add a shared dictionary usable from
streamand increment it in the trailer loop.lua_shared_dictis per-subsystem, so declare one in thestreamblock and report it from a separate key or a separate endpoint:stream { + lua_shared_dict bench_stream_stats 1m; + init_by_lua_block {while true do local head, err = read_head() if not head then return end + + local stats = ngx.shared.bench_stream_stats + stats:incr("requests", 1, 0)Then expose the stream counters to the runner, or document that trailer cases are exempt from the reuse audit.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@benchmark/conf/upstream.nginx.conf` around lines 147 - 162, Extend the trailer mock’s stream-side request handling so it increments shared connection and request counters during the trailer loop, then expose those stream counters through the existing stats interface or another runner-consumable endpoint. Update the reuse audit inputs so trailer cases no longer produce zero or empty requests_per_connection values; do not exempt them from the audit.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@benchmark/fold.sh`:
- Around line 119-142: Guard the headline-row calculation in the loop over
seen_tp so f / r is evaluated only when both medians are positive, while
preserving the existing n/a behavior for unavailable or non-positive values.
Also replace the implementation-defined iteration over seen_tp with the same
explicit sorting approach used by the detail table, so generated headline rows
are deterministic.
In `@benchmark/README.md`:
- Line 327: Rename the second `## Results` heading in the benchmark
documentation to a distinct, descriptive section heading, preserving the
section’s content and avoiding duplicate Markdown anchors and MD024 warnings.
- Around line 249-252: Add the appropriate language identifiers to the fenced
code examples in benchmark/README.md: mark the HTTP header example around
“Connection: keep-alive” as text and the CSV example around lines 333–338 as
csv, without changing their contents.
- Around line 388-390: Update the benchmark CPU configuration around TARGET_CPU,
UPSTREAM_CPUS, and WRK_CPUS so the default WRK_CPUS set is disjoint from
UPSTREAM_CPUS and TARGET_CPU, matching the required topology. Implement the
default derivation in run.sh, or validate and reject overlapping explicitly
configured sets while preserving valid custom CPU assignments.
- Around line 88-103: Update the benchmark table and the “Where the FFI client
does not win” section so tlsshort reflects the post-fix August 4, 2026 result of
1.10x, including the corresponding QPS values and narrative comparison; do not
present the 0.79x pre-fix result as current, or explicitly label the section as
pre-fix if retaining it.
In `@benchmark/results-full.md`:
- Around line 138-154: The benchmark summary in the throughput and latency
bullets must match the tables and caveat: exclude or explicitly qualify tlsshort
when stating FFI wins, and account for pooled tls latency regressions at p50,
p99, and p99.9. Remove unsupported universal claims and avoid quoting
short-lived TLS latency, while preserving the valid per-shape results and
handshake caveat.
In `@benchmark/run.sh`:
- Around line 823-827: Update fairness_audit so failed audits honor
BENCH_STRICT, remove each affected shape from CASES, and record the failures in
RESULTS_CSV so fold.sh does not present invalid comparisons as normal results.
Track each failing shape for summarize, and report those names alongside the
existing DROPPED reporting around summarize.
- Around line 146-152: Update default_resolver to avoid returning a bare IPv6
address: select the first IPv4 nameserver when available, or wrap an IPv6
nameserver value in brackets before assigning RESOLVER. Preserve the existing
127.0.0.1 fallback and BENCH_RESOLVER override behavior.
- Around line 510-518: Update upstream_stats_reset and upstream_stat to
propagate curl failures instead of suppressing them or converting missing stats
to empty values. Make the benchmark run fail when /stats/reset is unreachable,
and ensure callers cannot compute reuse metrics from an unavailable upstream
counter.
- Around line 687-720: Update the smoke-check loop over CASES so failures are
handled per shape rather than per case: when either member of a client pair
fails, exclude both paired cases from kept/CASES and record both as dropped.
Preserve baseline handling and strict-mode failure behavior, while ensuring
surviving pairs continue through fairness_audit, throughput_phase, and fold.sh.
- Around line 160-173: Update resolve_cases to disable pathname expansion while
intentionally splitting requested in the printf branch, so wildcard case names
remain literal; preserve the existing all handling and baseline output behavior.
- Around line 444-506: Update validate_runtime to verify that WRK2_BIN supports
the required wrk2 options (-R, -L, and -U) before any benchmark starts. Fail
early with a clear error when the configured binary is plain wrk or otherwise
lacks wrk2 support, while preserving the existing runtime validation behavior.
- Around line 569-581: Update the wrk2 execution flow around sample_cpu_loop so
sampler_pid is terminated and waited on even when the wrk2 pipeline fails under
set -euo pipefail. Preserve the pipeline’s original exit status while ensuring
cleanup occurs before that status is propagated, and avoid relying solely on the
EXIT trap.
- Line 43: Update the BENCH_PARSER selection in benchmark/run.sh so benchmark
labels use the parser chosen during configure/build rather than reading
NGX_HTTP_FFI_CLIENT_USE_LLHTTP at runtime. Persist that selection for make bench
or require an explicit BENCH_PARSER value, and do not add an nginx env
directive.
In `@README.md`:
- Around line 242-247: Update the “Where this client loses” section to replace
the stale fresh-TLS `0.79x` result and loss claim with the August 4, 2026
post-fix result of `1.10x` based on `1256.86` versus `1142.44` requests/sec.
Apply the same update to the matching benchmark README, or clearly label both
existing results as pre-fix.
In `@src/ngx_http_ffi_client_request.c`:
- Around line 1560-1612: Add a concise comment near the default certificate
environment-variable lookups and X509 lookup calls in
ngx_http_ffi_client_ssl_default_store documenting LibreSSL’s intentional
ignoring of SSL_CERT_FILE/SSL_CERT_DIR and BoringSSL’s legacy/deprecation status
for these APIs. Do not change the trust-store behavior or add version checks
unless the implementation already requires cross-library compatibility handling.
In `@t/003-benchmark-runner.t`:
- Around line 168-169: Update the fairness endpoint assertion in the benchmark
runner test to validate the complete endpoint set generated from the case table,
covering every shape and client rather than only /echo/ffi.stateful. Reuse the
case-table endpoint names or assert the full explicit list, while preserving the
existing generated-location verification.
- Around line 99-103: The test loop that iterates over $shape and checks each
shape against %by_shape has a hard-coded qw() list that omits documented matrix
shapes readbody and tlsshort. Update the qw() list to include these missing
shapes and add any verification-mode shapes that are part of cases.txt, so that
removing any of these shapes from the cases.txt or bench.lua files would cause
the test to fail.
---
Nitpick comments:
In `@benchmark/conf/upstream.nginx.conf`:
- Around line 147-162: Extend the trailer mock’s stream-side request handling so
it increments shared connection and request counters during the trailer loop,
then expose those stream counters through the existing stats interface or
another runner-consumable endpoint. Update the reuse audit inputs so trailer
cases no longer produce zero or empty requests_per_connection values; do not
exempt them from the audit.
In `@benchmark/fold.sh`:
- Around line 200-207: Update the latency output statements in the fold.sh
benchmark loop to use fixed printf format strings with each computed latency
value passed as an argument, rather than constructing the format string
dynamically. Apply this to the p50, p99, and p999 cells while preserving the
existing n/a handling and three-decimal formatting.
- Around line 144-150: Remove the unused empty-string argument from the baseline
explanatory printf in the base != "" block, and delete the empty for loop around
the later latency-table section that reuses n without performing work. Leave the
surrounding output and calculations unchanged.
In `@benchmark/run.sh`:
- Around line 848-866: Update median_qps in the benchmark runner to reuse
fold.sh’s single-value query mode instead of maintaining its own awk filtering
and median calculation; add or invoke that mode using the existing fold.sh gate
and median logic so latency_phase and published tables use identical inputs and
results.
- Around line 236-245: Update generate_cert to add the explicit critical
basicConstraints extension with CA:TRUE alongside the existing subjectAltName
extension, and stop redirecting OpenSSL stderr so failures retain their
diagnostic output; keep the existing die handling and note that -addext requires
OpenSSL 1.1.1 or newer.
In `@docs/ai-proxy-integration.md`:
- Line 18: Update both fenced code blocks in the documentation, including the
call-flow and header excerpts, to specify the text language identifier after the
opening fence. Leave their contents unchanged and ensure no affected block
remains an unlabeled fence.
In `@t/003-benchmark-runner.t`:
- Around line 64-81: Validate each case name in `@cases` before populating
%by_shape: require the full name to match the expected ffi or resty driver
format, reject unknown or malformed drivers, and detect duplicate full names
instead of allowing silent overwrites. Keep the existing shape-pairing check
after validation so only unique, supported cases are grouped.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9614c75c-c4ca-4294-96b6-3882128a30ba
📒 Files selected for processing (14)
.gitignoreREADME.mdbenchmark/README.mdbenchmark/cases.txtbenchmark/conf/target.nginx.confbenchmark/conf/upstream.nginx.confbenchmark/fold.shbenchmark/lua/bench.luabenchmark/results-full.mdbenchmark/run.shdocs/ai-proxy-integration.mdsrc/ngx_http_ffi_client.hsrc/ngx_http_ffi_client_request.ct/003-benchmark-runner.t
| for (name in seen_tp) { | ||
| if (name == "baseline") continue | ||
| # not split(name, parts, ".") -- awk reads the separator as a regex, so | ||
| # "." matches every character and every field comes back empty | ||
| if (substr(name, 1, 4) != "ffi.") continue | ||
| shape = substr(name, 5) | ||
|
|
||
| f = med_of("qps:ffi." shape) | ||
| r = med_of("qps:resty." shape) | ||
| if (f == "" || r == "") continue | ||
|
|
||
| printf "| `%s` | `%.2f` | `%.2f` | `%.2fx` |", shape, f, r, f / r | ||
|
|
||
| if (base != "" && f > 0 && r > 0) { | ||
| bus = 1000000 / base | ||
| fus = 1000000 / f - bus | ||
| rus = 1000000 / r - bus | ||
| if (fus > 0 && rus > 0) printf " `%.2fx` |", rus / fus | ||
| else printf " n/a |" | ||
| } else if (base != "") { | ||
| printf " n/a |" | ||
| } | ||
| printf "\n" | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard the f / r division, and sort the headline rows.
Two points in this loop:
- Line 130 divides by
rwhile only checkingr != "". Line 132 applies the strongerf > 0 && r > 0guard for the next column. If a shape's median resolves to0, awk aborts with a division-by-zero fatal error and no table is printed.run.shcallsfold.shwith|| trueat line 932, so the failure appears only as missing output. for (name in seen_tp)iterates in implementation-defined order, so the headline row order varies between runs and between awk implementations. The detail table at lines 158-162 sorts explicitly. These tables are committed into the READMEs, so an unsorted headline produces diff noise that is unrelated to the measurements.
🛠️ Proposed fix
- for (name in seen_tp) {
+ hn = 0
+ for (name in seen_tp) hnames[hn++] = name
+ for (i = 0; i < hn; i++)
+ for (j = i + 1; j < hn; j++)
+ if (hnames[j] < hnames[i]) { t = hnames[i]; hnames[i] = hnames[j]; hnames[j] = t }
+
+ for (i = 0; i < hn; i++) {
+ name = hnames[i]
if (name == "baseline") continue
# not split(name, parts, ".") -- awk reads the separator as a regex, so
# "." matches every character and every field comes back empty
if (substr(name, 1, 4) != "ffi.") continue
shape = substr(name, 5)
f = med_of("qps:ffi." shape)
r = med_of("qps:resty." shape)
- if (f == "" || r == "") continue
+ if (f == "" || r == "" || f + 0 <= 0 || r + 0 <= 0) continue
printf "| `%s` | `%.2f` | `%.2f` | `%.2fx` |", shape, f, r, f / r📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (name in seen_tp) { | |
| if (name == "baseline") continue | |
| # not split(name, parts, ".") -- awk reads the separator as a regex, so | |
| # "." matches every character and every field comes back empty | |
| if (substr(name, 1, 4) != "ffi.") continue | |
| shape = substr(name, 5) | |
| f = med_of("qps:ffi." shape) | |
| r = med_of("qps:resty." shape) | |
| if (f == "" || r == "") continue | |
| printf "| `%s` | `%.2f` | `%.2f` | `%.2fx` |", shape, f, r, f / r | |
| if (base != "" && f > 0 && r > 0) { | |
| bus = 1000000 / base | |
| fus = 1000000 / f - bus | |
| rus = 1000000 / r - bus | |
| if (fus > 0 && rus > 0) printf " `%.2fx` |", rus / fus | |
| else printf " n/a |" | |
| } else if (base != "") { | |
| printf " n/a |" | |
| } | |
| printf "\n" | |
| } | |
| hn = 0 | |
| for (name in seen_tp) hnames[hn++] = name | |
| for (i = 0; i < hn; i++) | |
| for (j = i + 1; j < hn; j++) | |
| if (hnames[j] < hnames[i]) { t = hnames[i]; hnames[i] = hnames[j]; hnames[j] = t } | |
| for (i = 0; i < hn; i++) { | |
| name = hnames[i] | |
| if (name == "baseline") continue | |
| # not split(name, parts, ".") -- awk reads the separator as a regex, so | |
| # "." matches every character and every field comes back empty | |
| if (substr(name, 1, 4) != "ffi.") continue | |
| shape = substr(name, 5) | |
| f = med_of("qps:ffi." shape) | |
| r = med_of("qps:resty." shape) | |
| if (f == "" || r == "" || f + 0 <= 0 || r + 0 <= 0) continue | |
| printf "| `%s` | `%.2f` | `%.2f` | `%.2fx` |", shape, f, r, f / r | |
| if (base != "" && f > 0 && r > 0) { | |
| bus = 1000000 / base | |
| fus = 1000000 / f - bus | |
| rus = 1000000 / r - bus | |
| if (fus > 0 && rus > 0) printf " `%.2fx` |", rus / fus | |
| else printf " n/a |" | |
| } else if (base != "") { | |
| printf " n/a |" | |
| } | |
| printf "\n" | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@benchmark/fold.sh` around lines 119 - 142, Guard the headline-row calculation
in the loop over seen_tp so f / r is evaluated only when both medians are
positive, while preserving the existing n/a behavior for unavailable or
non-positive values. Also replace the implementation-defined iteration over
seen_tp with the same explicit sorting approach used by the detail table, so
generated headline rows are deterministic.
| | `tls` | `17732.35` | `11861.95` | `1.49x` | `1.63x` | | ||
| | `short` | `8275.94` | `6973.64` | `1.19x` | `1.21x` | | ||
| | `tlsshort` | `872.96` | `1108.38` | `0.79x` | `0.79x` | | ||
|
|
||
| Baseline: `84504.18` QPS. | ||
|
|
||
| ### Where the FFI client does not win | ||
|
|
||
| **`tlsshort`, a fresh TLS handshake per request: `0.79x`.** `872.96` QPS against | ||
| `resty.http`'s `1108.38`, so this client is about 21% slower. The bottom of the | ||
| table reads as one story rather than three: `tls` (`1.49x`), `short` (`1.19x`) | ||
| and `tlsshort` (`0.79x`) are exactly the cases where connection setup dominates | ||
| the request. The advantage this client has is in per-request work, and it | ||
| shrinks as setup takes over, then reverses once setup is nearly all of it. | ||
| Anything that cannot hold a keepalive pool, and TLS to many short-lived peers | ||
| most of all, is a case where `lua-resty-http` is currently the better choice. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not present the pre-fix TLS matrix as current.
The table and narrative still report tlsshort at 0.79x. The August 4, 2026 fix run reports 1.10x after removing the CA-directory scan. Update these values, or label this section as pre-fix.
🧰 Tools
🪛 LanguageTool
[style] ~99-~99: Consider an alternative for the overused word “exactly”.
Context: ... (1.19x) and tlsshort (0.79x) are exactly the cases where connection setup domina...
(EXACTLY_PRECISELY)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@benchmark/README.md` around lines 88 - 103, Update the benchmark table and
the “Where the FFI client does not win” section so tlsshort reflects the
post-fix August 4, 2026 result of 1.10x, including the corresponding QPS values
and narrative comparison; do not present the 0.79x pre-fix result as current, or
explicitly label the section as pre-fix if retaining it.
| ``` | ||
| Connection: keep-alive | ||
| Content-Length: 0 | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add language identifiers to fenced examples.
markdownlint-cli2 reports MD040 at Lines [249] and [333]. Mark the header example as text and the CSV example as csv.
Also applies to: 333-338
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 249-249: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@benchmark/README.md` around lines 249 - 252, Add the appropriate language
identifiers to the fenced code examples in benchmark/README.md: mark the HTTP
header example around “Connection: keep-alive” as text and the CSV example
around lines 333–338 as csv, without changing their contents.
Source: Linters/SAST tools
| --with-pcre-jit --add-module=/path/to/ngx_http_ffi_client | ||
| ``` | ||
|
|
||
| ## Results |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use a unique section heading.
The document has two ## Results headings at Lines [20] and [327]. Rename the second heading to avoid duplicate Markdown anchors and the MD024 warning.
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 327-327: Multiple headings with the same content
(MD024, no-duplicate-heading)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@benchmark/README.md` at line 327, Rename the second `## Results` heading in
the benchmark documentation to a distinct, descriptive section heading,
preserving the section’s content and avoiding duplicate Markdown anchors and
MD024 warnings.
Source: Linters/SAST tools
| | `TARGET_CPU` | `0` | CPU core used by the target OpenResty worker | | ||
| | `UPSTREAM_CPUS` | non-target cores | CPU cores used by the upstream instance | | ||
| | `WRK_CPUS` | `UPSTREAM_CPUS` | CPU cores used by wrk2 | |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Use disjoint default CPU sets.
WRK_CPUS defaults to UPSTREAM_CPUS, so wrk2 shares CPUs with the upstream mock. This conflicts with the required topology, which places the target worker, upstream, and load generator on separate CPUs. Derive a disjoint default in run.sh, or reject overlapping sets.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@benchmark/README.md` around lines 388 - 390, Update the benchmark CPU
configuration around TARGET_CPU, UPSTREAM_CPUS, and WRK_CPUS so the default
WRK_CPUS set is disjoint from UPSTREAM_CPUS and TARGET_CPU, matching the
required topology. Implement the default derivation in run.sh, or validate and
reject overlapping explicitly configured sets while preserving valid custom CPU
assignments.
| if (( failures )); then | ||
| warn "$failures shape(s) failed the fairness audit; the affected pairs" \ | ||
| "are not comparing like for like" | ||
| fi | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
A fairness failure only warns, and the folded tables do not record it.
fairness_audit counts failures and emits a warning. It does not honour BENCH_STRICT, it does not remove the affected shape from CASES, and it writes nothing into RESULTS_CSV. fold.sh reads only the CSV, so it prints a headline ratio for a shape that the audit proved is not sending the same request. A reader of the generated table cannot tell that the comparison was invalid.
The audit is the control that makes the ratio meaningful, so its result must reach the output.
♻️ Proposed change
if (( failures )); then
+ if [[ "$BENCH_STRICT" == "1" ]]; then
+ die "$failures shape(s) failed the fairness audit"
+ fi
+
warn "$failures shape(s) failed the fairness audit; the affected pairs" \
"are not comparing like for like"
fiAdditionally record each failing shape so summarize can name it next to the tables, in the same way DROPPED is reported at lines 934-943.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@benchmark/run.sh` around lines 823 - 827, Update fairness_audit so failed
audits honor BENCH_STRICT, remove each affected shape from CASES, and record the
failures in RESULTS_CSV so fold.sh does not present invalid comparisons as
normal results. Track each failing shape for summarize, and report those names
alongside the existing DROPPED reporting around summarize.
| **Where this client loses.** The advantage is in per-request work, so it shrinks | ||
| as connection setup takes over the request and eventually reverses: `1.49x` on | ||
| pooled TLS, `1.19x` on short-lived plaintext connections, and `0.79x` on a fresh | ||
| TLS handshake per request, where `lua-resty-http` is about 21% faster. Anything | ||
| that cannot hold a keepalive pool, TLS to many short-lived peers most of all, is | ||
| a case for the other client today. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Update the fresh-TLS result.
This section still presents tlsshort as 0.79x and says the C client loses. The August 4, 2026 post-fix run reports 1256.86 versus 1142.44 requests/sec (1.10x) and supersedes the old result. Update this section and the matching benchmark README, or label both as pre-fix results.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` around lines 242 - 247, Update the “Where this client loses”
section to replace the stale fresh-TLS `0.79x` result and loss claim with the
August 4, 2026 post-fix result of `1.10x` based on `1256.86` versus `1142.44`
requests/sec. Apply the same update to the matching benchmark README, or clearly
label both existing results as pre-fix.
| static ngx_int_t | ||
| ngx_http_ffi_client_ssl_default_store(X509_STORE *store, ngx_log_t *log) | ||
| { | ||
| const char *file, *dir; | ||
| ngx_uint_t loaded; | ||
| X509_LOOKUP *lookup; | ||
|
|
||
| loaded = 0; | ||
|
|
||
| file = getenv(X509_get_default_cert_file_env()); | ||
| if (file == NULL) { | ||
| file = X509_get_default_cert_file(); | ||
| } | ||
|
|
||
| dir = getenv(X509_get_default_cert_dir_env()); | ||
| if (dir == NULL) { | ||
| dir = X509_get_default_cert_dir(); | ||
| } | ||
|
|
||
| lookup = X509_STORE_add_lookup(store, X509_LOOKUP_file()); | ||
| if (lookup == NULL) { | ||
| return NGX_ERROR; | ||
| } | ||
|
|
||
| if (X509_LOOKUP_load_file(lookup, file, X509_FILETYPE_PEM) == 1) { | ||
| loaded = 1; | ||
|
|
||
| } else { | ||
| /* not fatal on its own: the hashed directory may still cover it */ | ||
| ERR_clear_error(); | ||
| } | ||
|
|
||
| lookup = X509_STORE_add_lookup(store, X509_LOOKUP_hash_dir()); | ||
| if (lookup == NULL) { | ||
| return NGX_ERROR; | ||
| } | ||
|
|
||
| if (X509_LOOKUP_add_dir(lookup, dir, X509_FILETYPE_PEM) == 1) { | ||
| loaded = 1; | ||
|
|
||
| } else { | ||
| ERR_clear_error(); | ||
| } | ||
|
|
||
| if (!loaded) { | ||
| ngx_ssl_error(NGX_LOG_ALERT, log, 0, | ||
| "failed to load the default trust store from \"%s\"" | ||
| " or \"%s\"", file, dir); | ||
| return NGX_ERROR; | ||
| } | ||
|
|
||
| return NGX_OK; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Are X509_get_default_cert_file_env, X509_LOOKUP_load_file and X509_LOOKUP_add_dir available in OpenSSL 3.x, LibreSSL and BoringSSL?
💡 Result:
The functions X509_get_default_cert_file_env, X509_LOOKUP_load_file, and X509_LOOKUP_add_dir are available in OpenSSL 3.x, LibreSSL, and BoringSSL. OpenSSL 3.x These functions are fully supported in OpenSSL 3.x [1][2]. X509_get_default_cert_file_env is used to retrieve the environment variable name for the default certificate file [1][3]. X509_LOOKUP_load_file and X509_LOOKUP_add_dir are implemented as macros that use X509_LOOKUP_ctrl to manage certificate lookup configurations [2][4]. LibreSSL These functions are available in LibreSSL [5][6][7]. The documentation confirms that they maintain compatibility with the standard OpenSSL interfaces [6][7]. Note that for security and simplicity, LibreSSL may ignore certain environment variables (such as SSL_CERT_DIR and SSL_CERT_FILE) that other implementations might use [6][7]. BoringSSL These functions are available in BoringSSL [8][9][10]. BoringSSL retains these functions as part of its legacy X.509 library for compatibility purposes [11][9]. The implementation of X509_LOOKUP_load_file and X509_LOOKUP_add_dir in BoringSSL utilizes X509_LOOKUP_ctrl [9][10]. Users are advised that these functions are part of a legacy API and may be subject to behavior changes or future removal as BoringSSL iterates on its X.509 implementation [11].
Citations:
- 1: https://docs.openssl.org/3.3/man3/X509_get_default_cert_file/
- 2: https://docs.openssl.org/3.0/man3/X509_LOOKUP/
- 3: https://docs.openssl.org/master/man3/X509_get_default_cert_file/
- 4: https://docs.openssl.org/3.4/man3/X509_LOOKUP/
- 5: https://man.openbsd.org/X509_LOOKUP_new.3
- 6: https://manpages.opensuse.org/Leap-16.0/libressl-devel-doc/X509_get_default_cert_file_env.3ssl.en.html
- 7: https://manpages.opensuse.org/Leap-16.0/libressl-devel-doc/X509_LOOKUP_new.3ssl.en.html
- 8: https://boringssl.googlesource.com/boringssl/+/55de8cd4a040/crypto/x509/by_file.cc
- 9: https://boringssl.googlesource.com/boringssl/+/096ded9f097b73a15956b04cd168c7cfe7e28f52/crypto/x509/by_file.c
- 10: https://boringssl.googlesource.com/boringssl/+/refs/tags/fips-20210429/include/openssl/x509_vfy.h
- 11: https://boringssl.googlesource.com/boringssl/+/HEAD/include/openssl/x509.h
Address BoringSSL legacy API status and LibreSSL behavior differences in X509 trust-store initialization.
The APIs X509_get_default_cert_file_env(), X509_get_default_cert_dir_env(), X509_LOOKUP_load_file(), and X509_LOOKUP_add_dir() are available in OpenSSL 3.x, LibreSSL, and BoringSSL. However:
- LibreSSL intentionally ignores the environment variables
SSL_CERT_DIRandSSL_CERT_FILEfor security reasons, so the behavior of this helper may differ from OpenSSL on systems using LibreSSL. - BoringSSL designates these as legacy X.509 APIs subject to future behavior changes or removal, creating a stability risk.
Add a comment documenting these behavioral differences and the BoringSSL deprecation risk, or consider adding explicit version checks if cross-library stability is critical.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ngx_http_ffi_client_request.c` around lines 1560 - 1612, Add a concise
comment near the default certificate environment-variable lookups and X509
lookup calls in ngx_http_ffi_client_ssl_default_store documenting LibreSSL’s
intentional ignoring of SSL_CERT_FILE/SSL_CERT_DIR and BoringSSL’s
legacy/deprecation status for these APIs. Do not change the trust-store behavior
or add version checks unless the implementation already requires cross-library
compatibility handling.
| # the axes the plan asks the matrix to cover | ||
| for my $shape (qw(oneshot stateful stream short post tls dns chunked trailers | ||
| hdr40 cookies req30)) { | ||
| ok($by_shape{$shape}, "matrix covers the $shape axis"); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Include all required matrix shapes.
This list omits readbody and tlsshort, both documented matrix shapes. Removing either from cases.txt and bench.lua would still pass the equality checks because the hard-coded requirement does not include it. Add the missing shapes, and add the verification-mode shapes if they are part of cases.txt.
Proposed assertion update
-for my $shape (qw(oneshot stateful stream short post tls dns chunked trailers
+for my $shape (qw(oneshot stateful readbody stream short post tls tlsshort dns chunked trailers
hdr40 cookies req30)) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # the axes the plan asks the matrix to cover | |
| for my $shape (qw(oneshot stateful stream short post tls dns chunked trailers | |
| hdr40 cookies req30)) { | |
| ok($by_shape{$shape}, "matrix covers the $shape axis"); | |
| } | |
| # the axes the plan asks the matrix to cover | |
| for my $shape (qw(oneshot stateful readbody stream short post tls tlsshort dns chunked trailers | |
| hdr40 cookies req30)) { | |
| ok($by_shape{$shape}, "matrix covers the $shape axis"); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@t/003-benchmark-runner.t` around lines 99 - 103, The test loop that iterates
over $shape and checks each shape against %by_shape has a hard-coded qw() list
that omits documented matrix shapes readbody and tlsshort. Update the qw() list
to include these missing shapes and add any verification-mode shapes that are
part of cases.txt, so that removing any of these shapes from the cases.txt or
bench.lua files would cause the test to fail.
| like($locations, qr/location = \/echo\/ffi\.stateful \{/, | ||
| 'generated locations include the fairness-audit echo endpoints'); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Check the complete fairness endpoint set.
This assertion checks only /echo/ffi.stateful. It does not detect a missing fairness endpoint for another shape or client. Derive the expected endpoint names from the case table, or assert the complete explicit endpoint list generated by the runner.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@t/003-benchmark-runner.t` around lines 168 - 169, Update the fairness
endpoint assertion in the benchmark runner test to validate the complete
endpoint set generated from the case table, covering every shape and client
rather than only /echo/ffi.stateful. Reuse the case-table endpoint names or
assert the full explicit list, while preserving the existing generated-location
verification.
membphis
left a comment
membphis
left a comment
There was a problem hiding this comment.
[P2][Non-blocking] Add a positive regression for the default trust store
The manual replacement for OpenSSL default trust-store loading is covered only by negative self-signed cases or tests that set ssl_trusted_certificate explicitly. A broken system/default CA loader could therefore still pass CI.
Please add a regression that trusts a local CA only through SSL_CERT_FILE or SSL_CERT_DIR, omits an explicit trusted certificate, verifies that a matching hostname succeeds and a mismatch fails, and preserves verify-on/off cache separation.
This is explicitly non-blocking for this PR.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Nothing in ngx_http_ffi_client_ssl_ctx() reads `ca` unless `verify` is set, so every verify-off context is byte-identical whatever CA the caller named. The cache still keyed on `ca`, so a caller passing ssl_trusted_certificate with ssl_verify off retained one storeless SSL_CTX per distinct path for the life of the worker. Fold `ca` away at function entry, ahead of both the lookup and the cache node, so the two can never disagree: normalising only the lookup would miss every time and allocate a context per request.
Touches only
src/. The benchmark harness these numbers come from is in #35, which should land first.What
ngx_http_ffi_client_ssl_ctx()calledSSL_CTX_set_default_verify_paths()unconditionally. That registers the hashed CA directory asX509_FILETYPE_DEFAULT, and withDEFAULTOpenSSL enumerates the whole directory on every handshake rather than opening the single<hash>.Nfile the lookup actually needs.Found by profiling
tlsshort(fresh connection per request over TLS), the one shape in the matrix where this client lost tolua-resty-http.Evidence
perf diffbetween the two clients, sampling the pinned target worker — every top delta entry is kernel directory-reading, all of it on our side:Confirmed at the syscall level: 2132
openat+ 4264getdents64for ~2168 requests, one full sweep of a 245-entry directory per handshake. The same trace againstlua-resty-httpcaught zero.Reproduced independently of this module, with
openssl s_clientagainst the benchmark upstream:Explicit
CApathstill verifies correctly (return code 0) when the issuer is present, so it is the filetype that costs — not hash-dir lookup, and not lookup failure.The fix
X509_FILETYPE_PEM, honouringSSL_CERT_FILE/SSL_CERT_DIRthe way OpenSSL does. A verifying caller no longer pays the sweep either.ssl_verifyis set — for everyone else the store was pure waste.Result
Median of 5 saturated repeats, target worker pinned to one core, upstream and load generator on separate cores:
tlsshorttlstlsverify(new)tlsverifyshort(new)Zero
openat/getdents64per handshake afterwards, verify on and off.Tests
All 447 pass, including
t/004-tls.tTEST 4, which asserts verification still rejects an untrusted self-signed cert — so the store is genuinely loaded, not silently disabled.The matrix had never exercised
ssl_verify = true(all four call sites inbench.luahardcodedfalse). Newtlsverifyandtlsverifyshortcases close that gap, so the verifying path in change 1 ships measured rather than assumed. Both clients trust the same self-signed upstream cert — the FFI client viassl_trusted_certificate,lua-resty-httpvialua_ssl_trusted_certificate, since it has no per-request trust store.Known gaps
ffi.statefulp99.9 claim of 20.94 ms that a later 3-repeat run disproved (actual: 2.730 ms vsresty.stateful's 3.320 ms). It was flagged at the time as single-sample and not yet evidence. Corrected data for the whole matrix is inbenchmark/results-full.md; re-baselining the READMEs is deliberately left to a follow-up.lua-resty-httphits the target every time. Throughput is unaffected and stable across all 5 repeats. Unexplained; no latency claim is made for those shapes here.Note on scope
The first commit is the benchmark harness this finding came out of — both columns of every comparison are now generated from one shape table, so fairness is structural rather than reviewed. It was already uncommitted in the tree and the new TLS cases depend on it. The fix itself is the second commit and touches only
src/.Summary by CodeRabbit