tools: event-engine A/B benchmark harness (oha + ab)#32
Conversation
tools/bench-engines.sh runs an identical request-hot-path matrix against two unitd build dirs and reports RPS, p50/p99 latency, error counts and router CPU us/req per build. It is build-agnostic (only needs sbin/unitd, plus a php module for the optional PHP scenario), so pointing both slots at one epoll build doubles as a null A/A drift test. Per round, per build: isolated statedir/control socket/log under a run dir, PUT config, verify success, warm up, measure with oha (JSON) plus an ab continuity pass, kill cleanly. Builds interleave A/B each round to cancel thermal/cache drift. Scenario matrix: return-200 keepalive high-conc (-c256), keepalive low-conc (-c8, listen-herd detector), close, proxy->self (-c64), and an optional PHP hello app. Engine assertion: greps the unitd log for 'using "<name>" event engine' and records the engine per build; aborts if both builds report the same engine unless --allow-same/ALLOW_SAME=1. Falls back to engine=unknown with a warning on builds that predate that log line so A/A still works. tools/bench-engines-report.py summarizes results into markdown tables with mean +/- stdev per scenario per build and the B-vs-A delta %. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
taskset at unitd spawn makes app workers inherit the server-core mask (previously only the router was re-pinned, so PHP workers could land on the load-generator cores). A failed PHP config PUT now records SKIP instead of measuring a dead listener as 100% failures. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The main process logs 'using "<name>" event engine' before the log file opens, so the line only reaches stderr; router workers log it later. Capture stderr per run and check both streams. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a benchmark harness (bench-engines.sh) and a reporting tool (bench-engines-report.py) to compare the performance of two different unitd event engines. The feedback highlights several critical areas for improvement, including fixing potential process leaks on early failures, resolving bash syntax errors when processes exit during CPU tick calculation, ensuring portability by dynamically partitioning CPU cores instead of hardcoding them, and correcting subshell file descriptor handling to avoid 'Bad file descriptor' errors. Additionally, using a context manager for file operations in the Python script is recommended to prevent resource leaks.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| router_cpu_ticks() { # utime+stime of the router process; comm has a space | ||
| sed 's/.*) //' "/proc/$ROUTER/stat" | awk '{print $12+$13}' | ||
| } | ||
|
|
||
| tree_cpu_ticks() { # unitd main + all descendants (router + app workers) | ||
| local total=0 pid | ||
| for pid in $UNITD_PID $(pgrep -P "$UNITD_PID" 2>/dev/null) \ | ||
| $(pgrep -P "$UNITD_PID" 2>/dev/null | xargs -rn1 pgrep -P 2>/dev/null); do | ||
| [ -r "/proc/$pid/stat" ] || continue | ||
| total=$((total + $(sed 's/.*) //' "/proc/$pid/stat" | awk '{print $12+$13}'))) | ||
| done | ||
| echo "$total" | ||
| } |
There was a problem hiding this comment.
If a process dies or exits between the existence check and the sed execution, sed will fail and print nothing to stdout. This causes the $((total + ...)) arithmetic expansion to be empty, resulting in a bash syntax error (operand expected). Ensuring that we default to 0 when ticks are empty prevents these runtime failures.
| router_cpu_ticks() { # utime+stime of the router process; comm has a space | |
| sed 's/.*) //' "/proc/$ROUTER/stat" | awk '{print $12+$13}' | |
| } | |
| tree_cpu_ticks() { # unitd main + all descendants (router + app workers) | |
| local total=0 pid | |
| for pid in $UNITD_PID $(pgrep -P "$UNITD_PID" 2>/dev/null) \ | |
| $(pgrep -P "$UNITD_PID" 2>/dev/null | xargs -rn1 pgrep -P 2>/dev/null); do | |
| [ -r "/proc/$pid/stat" ] || continue | |
| total=$((total + $(sed 's/.*) //' "/proc/$pid/stat" | awk '{print $12+$13}'))) | |
| done | |
| echo "$total" | |
| } | |
| router_cpu_ticks() { # utime+stime of the router process; comm has a space | |
| local ticks | |
| ticks=$(sed 's/.*) //' "/proc/$ROUTER/stat" 2>/dev/null | awk '{print $12+$13}') | |
| echo "${ticks:-0}" | |
| } | |
| tree_cpu_ticks() { # unitd main + all descendants (router + app workers) | |
| local total=0 pid ticks | |
| for pid in $UNITD_PID $(pgrep -P "$UNITD_PID" 2>/dev/null) \ | |
| $(pgrep -P "$UNITD_PID" 2>/dev/null | xargs -rn1 pgrep -P 2>/dev/null); do | |
| [ -r "/proc/$pid/stat" ] || continue | |
| ticks=$(sed 's/.*) //' "/proc/$pid/stat" 2>/dev/null | awk '{print $12+$13}') | |
| total=$((total + ${ticks:-0})) | |
| done | |
| echo "$total" | |
| } |
There was a problem hiding this comment.
Fixed in c37b6bf: both tick readers now default to 0 when the process vanished between checks (2>/dev/null on the read, ${ticks:-0} in the arithmetic).
| start_unitd "$label" "$build" "$run" "$mods" || return 1 | ||
|
|
||
| # --- HTTP scenarios --- | ||
| put_config_http "$run/control.sock" "$port" "$pport" "$run/conf-http.json" \ | ||
| || { echo "CONF FAILED $label"; cat "$run/conf-http.json"; return 1; } | ||
| wait_port "$port" 10 || { echo "listener $port never came up"; return 1; } | ||
| pin_router "$run" || return 1 |
There was a problem hiding this comment.
If any of the setup steps (such as start_unitd, put_config_http, wait_port, or pin_router) fail, run_build_round returns early with return 1 without calling cleanup. This leaks the background unitd process, which will continue running, hogging ports and CPU, and potentially corrupting subsequent benchmark rounds. Ensuring cleanup is called on early return prevents these process leaks.
| start_unitd "$label" "$build" "$run" "$mods" || return 1 | |
| # --- HTTP scenarios --- | |
| put_config_http "$run/control.sock" "$port" "$pport" "$run/conf-http.json" \ | |
| || { echo "CONF FAILED $label"; cat "$run/conf-http.json"; return 1; } | |
| wait_port "$port" 10 || { echo "listener $port never came up"; return 1; } | |
| pin_router "$run" || return 1 | |
| start_unitd "$label" "$build" "$run" "$mods" || { cleanup; return 1; } | |
| # --- HTTP scenarios --- | |
| put_config_http "$run/control.sock" "$port" "$pport" "$run/conf-http.json" \ | |
| || { echo "CONF FAILED $label"; cat "$run/conf-http.json"; cleanup; return 1; } | |
| wait_port "$port" 10 || { echo "listener $port never came up"; cleanup; return 1; } | |
| pin_router "$run" || { cleanup; return 1; } |
There was a problem hiding this comment.
Fixed in c37b6bf: every early-failure return in run_build_round() (and the preflight) now runs cleanup first, so a half-started unitd can no longer outlive its round, hold the ports, or escape the EXIT trap.
| for line in open(path): | ||
| f = line.split() | ||
| if len(f) >= 4 and f[2] == "SKIP": | ||
| skips.append((f[0], " ".join(f[3:]))) | ||
| continue | ||
| if len(f) < 4 or "=" not in f[3]: | ||
| continue | ||
| label, scenario, tool = f[0], f[1], f[2] | ||
| kv = {} | ||
| for tok in f[3:]: | ||
| if "=" in tok: | ||
| k, v = tok.split("=", 1) | ||
| kv[k] = v | ||
| for m in ("rps", "p50ms", "p99ms", "failed", "cpu_us_per_req"): | ||
| v = kv.get(m) | ||
| try: | ||
| rows[(tool, scenario, label)][m].append(float(v)) | ||
| except (TypeError, ValueError): | ||
| rows[(tool, scenario, label)][m].append(None) | ||
| if kv.get("engine"): | ||
| engines[label] = kv["engine"] | ||
| return rows, skips, engines |
There was a problem hiding this comment.
The file is opened without using a with statement, which can lead to a resource leak if the function exits or raises an exception. Wrapping the file reading in a with block ensures the file is closed properly.
with open(path) as f_in:
for line in f_in:
f = line.split()
if len(f) >= 4 and f[2] == "SKIP":
skips.append((f[0], " ".join(f[3:])))
continue
if len(f) < 4 or "=" not in f[3]:
continue
label, scenario, tool = f[0], f[1], f[2]
kv = {}
for tok in f[3:]:
if "=" in tok:
k, v = tok.split("=", 1)
kv[k] = v
for m in ("rps", "p50ms", "p99ms", "failed", "cpu_us_per_req"):
v = kv.get(m)
try:
rows[(tool, scenario, label)][m].append(float(v))
except (TypeError, ValueError):
rows[(tool, scenario, label)][m].append(None)
if kv.get("engine"):
engines[label] = kv["engine"]
return rows, skips, enginesThere was a problem hiding this comment.
Fixed in c37b6bf: the results file is read via with open(...).
| SERVER_CORES=0-3 | ||
| LOAD_CORES=4-7 |
There was a problem hiding this comment.
Hardcoding CPU affinity to cores 0-3 and 4-7 makes the script non-portable and will cause taskset to fail with an Invalid argument error on machines with fewer than 8 cores (such as standard developer laptops or smaller VMs). Dynamically detecting the number of online cores and partitioning them ensures the script runs out-of-the-box on any environment.
| SERVER_CORES=0-3 | |
| LOAD_CORES=4-7 | |
| num_cores=$(nproc 2>/dev/null || getconf _NPROCESSORS_ONLN 2>/dev/null || echo 1) | |
| if [ "$num_cores" -ge 8 ]; then | |
| SERVER_CORES=0-3 | |
| LOAD_CORES=4-7 | |
| elif [ "$num_cores" -gt 1 ]; then | |
| half=$((num_cores / 2)) | |
| SERVER_CORES="0-$((half - 1))" | |
| LOAD_CORES="$half-$((num_cores - 1))" | |
| else | |
| SERVER_CORES=0 | |
| LOAD_CORES=0 | |
| fi |
There was a problem hiding this comment.
Addressed in c37b6bf: SERVER_CORES/LOAD_CORES are now env-overridable and both masks are validated with taskset -c ... true up front, failing fast with a clear message. The 0-3/4-7 defaults are kept deliberately — they encode the reference-box methodology behind the committed results, and silently auto-partitioning would make runs from different hosts incomparable without saying so.
| if command -v ss >/dev/null 2>&1; then | ||
| ss -ltn "sport = :$1" 2>/dev/null | grep -q LISTEN | ||
| else | ||
| (exec 3<>"/dev/tcp/127.0.0.1/$1") 2>/dev/null && { exec 3>&- 3<&-; return 0; } |
There was a problem hiding this comment.
Since the TCP connection is opened in a subshell (...), the file descriptor 3 is automatically closed when the subshell exits. Attempting to close it in the parent shell with exec 3>&- 3<&- will result in a Bad file descriptor error printed to stderr. We can safely remove the parent shell close commands.
| (exec 3<>"/dev/tcp/127.0.0.1/$1") 2>/dev/null && { exec 3>&- 3<&-; return 0; } | |
| (exec 3<>"/dev/tcp/127.0.0.1/$1") 2>/dev/null && return 0 |
There was a problem hiding this comment.
The claimed 'Bad file descriptor' doesn't actually occur — closing an unopened fd via exec 3>&- is a silent no-op in bash — but the parent-side close was dead code either way (the fd lives only in the subshell). Removed in c37b6bf.
| if command -v ss >/dev/null 2>&1; then | ||
| ss -ltn "sport = :$p" 2>/dev/null | grep -q LISTEN && return 0 | ||
| else | ||
| (exec 3<>"/dev/tcp/127.0.0.1/$p") 2>/dev/null && { exec 3>&- 3<&-; return 0; } |
There was a problem hiding this comment.
Since the TCP connection is opened in a subshell (...), the file descriptor 3 is automatically closed when the subshell exits. Attempting to close it in the parent shell with exec 3>&- 3<&- will result in a Bad file descriptor error printed to stderr. We can safely remove the parent shell close commands.
| (exec 3<>"/dev/tcp/127.0.0.1/$p") 2>/dev/null && { exec 3>&- 3<&-; return 0; } | |
| (exec 3<>"/dev/tcp/127.0.0.1/$p") 2>/dev/null && return 0 |
There was a problem hiding this comment.
Same as above: no error occurs in bash, but the dead parent-side close is removed in c37b6bf.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 46bba26cee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| run_build_round "$LABEL_A" "$BUILD_A" A "$r" || { echo "round $r A failed" >&2; } | ||
| run_build_round "$LABEL_B" "$BUILD_B" B "$r" || { echo "round $r B failed" >&2; } |
There was a problem hiding this comment.
When a round fails after start_unitd (for example config failure, no listener, or no router), these handlers only log and keep going; the script can then reach exit 0 with incomplete results, and the failing daemon may remain running once the next build overwrites UNITD_PID. That makes a failed or contaminated A/B run look successful to automation, so propagate the failure or aggregate it and exit non-zero after cleanup.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c37b6bf: failed build-rounds are counted, the summary still prints for whatever data exists, and the script exits non-zero with an explicit 'results are incomplete' error. Combined with the cleanup-on-early-return fix, a failing daemon can no longer leak into the next round either.
| run_build_round "$LABEL_A" "$BUILD_A" A "$r" || { echo "round $r A failed" >&2; } | ||
| run_build_round "$LABEL_B" "$BUILD_B" B "$r" || { echo "round $r B failed" >&2; } |
There was a problem hiding this comment.
This always measures A immediately before B in every round, so any monotonic warmup, cache, or thermal drift over the minutes-long matrix is still correlated with the build label instead of being canceled. Alternate the order on even rounds (or randomize it) so each build appears in both positions before comparing means.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c37b6bf: the order now alternates per round (A/B, B/A, ...), so each build appears in both positions and monotonic warmup/thermal drift decorrelates from the build label.
| local rps p99 failed us | ||
| rps=$(awk '/Requests per second/{print $4}' "$out") | ||
| p99=$(awk '/ 99%/{print $2}' "$out") | ||
| failed=$(awk '/Failed requests/{print $3}' "$out") |
There was a problem hiding this comment.
ApacheBench reports HTTP status failures on a separate Non-2xx responses line rather than including them in Failed requests (per the Apache httpd ab docs: https://httpd.apache.org/docs/2.4/programs/ab.html). With this parser, a proxy/listener regression that returns 5xx during the secondary pass is recorded as failed=0, hiding bad runs in the machine-readable results.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c37b6bf: the ab parser now adds the 'Non-2xx responses' count into failed=, so a 5xx regression in the secondary pass is visible in the machine-readable results.
| SERVER_CORES=0-3 | ||
| LOAD_CORES=4-7 |
There was a problem hiding this comment.
I checked taskset --help, which says -c specifies CPUs in list format; with these hard-coded masks, hosts or cpuset-limited runners without CPUs 4-7 will make every load-generator taskset -c "$LOAD_CORES" ... command fail with invalid affinity. A 4-core benchmark box can still start Unit on 0-3, then silently record zero/empty measurements, so validate the masks against the available affinity or make them configurable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c37b6bf: both masks are validated with taskset -c ... true before anything runs, and SERVER_CORES/LOAD_CORES are env-overridable for hosts/cpusets without the reference-box 0-3/4-7 split.
- cleanup() before every early return in run_build_round and preflight: a half-started unitd otherwise leaks, holds the ports (aborting later rounds) and escapes the EXIT trap, which only knows the last pid. - Track failed build-rounds and exit non-zero after the summary so automation cannot mistake an incomplete/contaminated run for a good A/B comparison. - Alternate the build order per round (A/B, B/A, ...): a fixed order keeps monotonic warmup/thermal drift correlated with the build label. - Count ab's 'Non-2xx responses' line into failed=: ab reports HTTP status failures separately from 'Failed requests', so a 5xx regression in the secondary pass was recorded as failed=0. - Default CPU ticks to 0 when a process vanishes between checks; an empty command substitution broke the shell arithmetic. - Make SERVER_CORES/LOAD_CORES env-overridable and validate both masks with taskset up front, failing fast on hosts/cpusets without the reference-box 0-3/4-7 split. - Drop the parent-side close of a subshell-scoped /dev/tcp fd (dead code) and read the results file via a context manager in the summarizer. Smoke-tested: 2-round A/A run, all scenarios, exit 0, no leaked unitd. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Review addressed in c37b6bf: cleanup on every early return, non-zero exit on failed rounds, per-round A/B order alternation, ab Non-2xx counted as failures, tick-reader hardening, validated + env-overridable core masks, and the summarizer file handling. Smoke-tested with a 2-round A/A run (all scenarios, exit 0, no leaked unitd). |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c37b6bfb7a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| (ms(.latencyPercentiles."p50" // 0)), | ||
| (ms(.latencyPercentiles."p99" // 0)), | ||
| ( (( [ .statusCodeDistribution // {} | to_entries[] | ||
| | select((.key|tonumber) >= 400 or (.key|tonumber) < 200) | .value ] |
There was a problem hiding this comment.
For scenarios that are supposed to return 200, a 3xx response is still a non-2xx result, but this filter only adds statuses below 200 or at least 400 to failed. If a build starts redirecting (for example from the PHP app or proxy path), the primary oha row reports failed=0 and the summary treats the run as clean; use >= 300 here and in the Python fallback to match the AB non-2xx accounting.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e55bb3c: the failed filter is now >= 300 or < 200 in both the jq and python3 branches, matching the ab Non-2xx accounting.
| taskset -c "$LOAD_CORES" "$OHA" --no-tui --output-format json -n "$n" "${ohaka[@]}" "$url" \ | ||
| > "$js" 2>"$run/oha-$scen.err" | ||
| if [ "$mode" = tree ]; then t1=$(tree_cpu_ticks); else t1=$(router_cpu_ticks); fi | ||
| local rps p50 p99 failed | ||
| IFS=$'\t' read -r rps p50 p99 failed < <(parse_oha "$js") |
There was a problem hiding this comment.
If oha exits before writing valid JSON (for example it is killed, cannot start, or the listener disappears early), this status is ignored and parse_oha emits nothing; read then leaves the metrics empty, and Bash printf records zero values while run_build_round still succeeds. That can turn a broken primary measurement into a successful zero-RPS sample, so check the oha command/parse result and propagate the failure.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e55bb3c: oha_one (and ab_one) now check the tool's exit status and that rps parsed non-empty; a dead/unparsable measurement errors with the label/scenario and saved output path and fails the round through the FAILED_ROUNDS accounting instead of recording an all-zero row. Verified with OHA=/bin/false: run exits 1, results file stays free of zero rows.
|
|
||
| # ---- main loop -------------------------------------------------------------- | ||
| FAILED_ROUNDS=0 | ||
| for r in $(seq 1 "$ROUNDS"); do |
There was a problem hiding this comment.
When rounds is 0, negative, or non-numeric, this seq invocation yields no iterations (or only prints an error for invalid input) and, because the script never checks it, FAILED_ROUNDS remains 0; the script then summarizes an empty results file and exits successfully. Validate ROUNDS as a positive integer before this loop so automation cannot record a successful benchmark with no measurements.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e55bb3c: rounds is validated as a positive decimal integer right after argument parsing (usage error, exit 2 for empty/non-numeric/zero).
| [ -d "$mods" ] || mods="$run/modules" | ||
|
|
||
| # refuse if ports busy | ||
| for p in "$port" "$pport"; do |
There was a problem hiding this comment.
When a build has a PHP module and $pphp is already bound, this preflight does not catch it; the PHP reconfig then falls into the SKIP php (config/listener failed) path and the build round still succeeds. That omits the PHP scenario for an environmental port conflict even though the scenario is available, producing incomplete A/B results with exit 0; include $pphp in this busy-port check or fail the round on PHP listener conflicts.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e55bb3c: $pphp joined the busy-port preflight, so a bound PHP port aborts the round instead of degrading to the SKIP path.
| done | ||
|
|
||
| mkdir -p "$RUNBASE" | ||
| : > "$RESULTS" |
There was a problem hiding this comment.
If RESULTS points to a directory that does not exist or is not writable, this redirection fails but the script keeps running; later tee -a "$RESULTS" and the Python summarizer also fail without being checked, so a benchmark can finish with exit 0 while no machine-readable results were written. Check this initialization and abort before starting any measurements.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e55bb3c: the results-file initialization is checked and the script aborts with exit 2 before any measurement if it cannot be written.
- parse_oha: count 3xx as failed (>= 300 or < 200) in both the jq and python3 branches, matching ab's "Non-2xx responses" accounting - oha_one/ab_one: check the tool's exit status and that rps parsed non-empty; error with label/scenario and the saved output file and return 1 instead of recording an all-zero row - run_build_round: collect measurement failures and return 1 after cleanup so FAILED_ROUNDS catches them (php SKIP paths stay SKIPs) - validate rounds as a positive decimal integer, usage error exit 2 - include the php port in the busy-port preflight and abort the round instead of degrading to the php SKIP path - abort with exit 2 when the results file cannot be created Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e55bb3c6a3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if [ "$rc" -ne 0 ] || [ -z "$rps" ]; then | ||
| echo "oha measurement failed: $label $scen (rc=$rc, see $run/oha-$scen.err)" >&2 | ||
| return 1 |
There was a problem hiding this comment.
When oha completes but the scenario returns non-2xx responses or transport errors, parse_oha populates failed, yet this check only treats the measurement as bad when the tool exits nonzero or rps is empty. Because these benchmark scenarios are expected to return 200 and the markdown summary never surfaces failed, a build that starts returning redirects/5xx can still make the whole run exit 0 unless someone inspects the raw results file.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c17c30b: after recording the row (kept for forensics), oha_one and ab_one fail the measurement when failed > MAX_FAILED (new env knob, default 0, documented in the header) — so a redirecting/5xx build now fails the round and the run. Verified by stubbing oha with an all-404 JSON: run exits 1.
| if [ "$HAVE_JQ" = 0 ] && [ "$HAVE_PY" = 0 ]; then | ||
| echo "need jq or python3 to parse oha JSON" >&2; exit 2 | ||
| fi | ||
| [ "$SKIP_AB" = 1 ] || [ "$HAVE_AB" = 1 ] || { echo "ab not found (use --skip-ab)" >&2; SKIP_AB=1; } |
There was a problem hiding this comment.
On hosts without ab, this branch silently flips SKIP_AB and continues even when the caller did not request --skip-ab. The default documented run is the oha+ApacheBench matrix, so automation can get a successful exit with all ab rows and tables missing; abort here unless SKIP_AB=1 or --skip-ab was explicitly supplied.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c17c30b: a missing ab now aborts with exit 2 unless --skip-ab/SKIP_AB=1 was explicitly passed. Verified with ab stripped from PATH: exit 2 without the flag, full green run with it.
| fi | ||
| UNITD_PID="" | ||
| } | ||
| trap cleanup EXIT INT TERM |
There was a problem hiding this comment.
When the benchmark receives SIGINT or SIGTERM, this trap only runs cleanup; Bash then resumes the script instead of terminating. In a CI timeout or Ctrl-C during a long oha/ab run, the handler can kill unitd and the script may continue into later measurements or even finish with a misleading status, so the INT/TERM handler should clean up and explicitly exit.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c17c30b: INT and TERM now get their own handlers that clean up and exit 130/143 (cleanup is idempotent, so the subsequent EXIT trap is a no-op). Verified by SIGINT mid-measurement: prompt exit 130, no leftover unitd.
| ticks=$(sed 's/.*) //' "/proc/$ROUTER/stat" 2>/dev/null | awk '{print $12+$13}') | ||
| echo "${ticks:-0}" |
There was a problem hiding this comment.
If the router exits or is respawned during an HTTP measurement, /proc/$ROUTER/stat disappears and this helper returns 0; the caller then computes CPU as 0 - t0 while still accepting the row whenever the load generator reports an rps. That turns a router crash/restart under load into a successful sample with bogus negative or undercounted CPU instead of failing the round.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c17c30b: in router mode both measurement functions now fail when /proc/$ROUTER/stat has vanished by measurement end, instead of accepting a bogus 0-tick CPU sample. Tree mode is left alone — app-worker churn there is the daemon's business.
| for b in "$BUILD_A" "$BUILD_B"; do | ||
| [ -x "$b/sbin/unitd" ] || { echo "no executable $b/sbin/unitd" >&2; exit 2; } | ||
| done | ||
| [ "$LABEL_A" != "$LABEL_B" ] || { echo "labels must differ ($LABEL_A)" >&2; exit 2; } |
There was a problem hiding this comment.
This accepts labels containing whitespace, but result rows are whitespace-delimited and the report parser assumes field 1 is the label, field 2 is the scenario, and field 3 is the tool. If a caller uses a descriptive label like epoll baseline, all measurement rows are skipped by the summarizer while the benchmark can still exit 0, so validate labels or encode them before writing results.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c17c30b: labels are validated against [A-Za-z0-9._-]+ right after argument parsing (usage error, exit 2). Verified with a whitespace label.
Roadmap: process/fd lifecycle hardening for this harness — and the test suite behind itThis harness is one corner of a bigger problem this PR round surfaced. Research into "unitd processes must be What the research foundThe Alpine
This harness already gained cleanup-on-early-return, failure propagation, and measurement validation in this PR — but Planned deliverablesD1 — this harness (follow-up commit on this PR or a successor):
D2 — pytest suite hardening (own PR):
D3 — attribution instead of thresholds:
D4 — containment wrapper for builders (opt-in):
D5 — the actual fd leaks (after D3 lands): reproduce the aarch64 growth and the access-log+keepalive pattern from nginx/unit#1129 with histograms on, plus the io_uring build's anon-inode lifecycle; file per-attribution fixes. Verification gates (each deliverable)
Sequencing: D1 first (small, immediately useful on builders), D2 as its own PR, D3 riding both, D4+aports note after, D5 as findings-driven follow-ups. |
…r, bad labels - oha_one/ab_one: after recording the row, fail the measurement when a run logged >MAX_FAILED request failures (new env knob, default 0); the summary tables don't surface `failed`, so non-2xx/transport errors went unnoticed. - tool discovery: a missing ab now exits 2 unless --skip-ab/SKIP_AB=1 was passed, instead of silently halving the matrix. - traps: INT/TERM now run cleanup then exit 130/143 so a Ctrl-C/CI-timeout can't resume the script into later measurements with unitd already killed (cleanup is idempotent, so the EXIT trap stays a harmless no-op). - oha_one/ab_one router mode: fail if the router pid vanished mid-measurement, so a crash/restart can't pass as a bogus 0/negative CPU sample. - arg parsing: validate LABEL_A/LABEL_B against ^[A-Za-z0-9._-]+$ (usage exit 2), since whitespace/glob chars silently break the summarizer. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Round-3 review addressed in c17c30b: failed-request accounting now fails measurements (MAX_FAILED knob, default 0), missing ab aborts unless --skip-ab, INT/TERM traps exit instead of resuming the run, a router that dies mid-measurement fails the sample instead of recording zero CPU, and labels are validated against the results format. Each failure path was exercised, not just reasoned about (stubbed failing oha, PATH without ab, SIGINT mid-run, whitespace label); A/A smoke stays green. |
- spawn each unitd via setsid into its own session/process group; read the real main pid back from the pidfile (not $!) and record the pgid to <run>/unitd.pgid for the cleanup ladder and external sweepers. - rewrite cleanup() as a pgid-keyed escalation ladder: TERM the group, poll ~10s, then KILL loudly. Reaps the router/controller even when the main pid is already dead but children survive. Guards against empty/invalid pgid and reaps our own zombie group leader so the group can't look falsely non-empty. - pre-run stale-instance sweep (run_build_round + preflight): reap a prior crashed instance tied to THIS run dir via its pgid file or a cmdline match, fingerprinted to the run dir so unrelated daemons are never touched; the port_busy abort remains the second gate for ports held by anything else. - post-run leak assert: scan all <run>/unitd.pgid, print survivors, kill via the ladder, and force a non-zero exit — a leak fails the run even if every measurement passed. - document the lifecycle model in the header comment. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
D1 from the lifecycle roadmap landed as e0a68ed: each unitd now runs in its own session/process group (real main pid read from the pidfile, pgid recorded to All four verification gates from the roadmap were executed, not simulated: SIGSTOP-wedged main (escalated to group KILL, exit in ~15 s), main SIGKILLed with children frozen (orphans reaped via pgid), harness itself SIGKILLed (next run's sweep reaped the orphan group holding the port and completed green), and a no-collateral check confirming unrelated unitd daemons on the box were untouched throughout. Next per the roadmap: D2 (the pytest conftest group-kill hardening) as its own PR, D3 fd histograms riding both. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e0a68edcff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if group_alive "$pg"; then | ||
| echo "LEAK: process group $pg from ${pf%/unitd.pgid} still has survivors:" >&2 | ||
| ps -o pid,pgid,cmd -g "$pg" >&2 2>/dev/null || true | ||
| kill_pgid "$pg" |
There was a problem hiding this comment.
Because normal teardown intentionally leaves unitd.pgid files on disk, a later run can have one of these stale numeric PGIDs reused by an unrelated process group. I checked pgrep --help (-g, --pgroup ... match listed process group IDs), and this leak assert only tests that the PGID exists before printing and calling kill_pgid, without verifying any process still has this run dir in its cmdline; in that reuse scenario the benchmark kills someone else's process group and reports a leak. Revalidate ownership, or remove stale pgid files after a clean teardown, before signaling.
Useful? React with 👍 / 👎.
| | tee -a "$RESULTS" | ||
| # Recorded the row for forensics; now fail the measurement if it saw request | ||
| # failures (non-2xx/transport) — the summary tables don't surface `failed`. | ||
| if [ "${failed:-0}" -gt "$MAX_FAILED" ]; then |
There was a problem hiding this comment.
When MAX_FAILED is set to a non-numeric value, this integer comparison prints an error and evaluates false inside the if, so a measurement that recorded non-2xx or transport failures is still accepted after writing the row; ab_one has the same comparison. Because this env knob is the only gate that turns recorded request failures into a failed round, validate it as a non-negative integer before running measurements.
Useful? React with 👍 / 👎.
| local ohaka=() | ||
| [ "$ka" = 1 ] && ohaka=(-c "$c") || ohaka=(-c "$c" --disable-keepalive) | ||
| if [ "$mode" = tree ]; then t0=$(tree_cpu_ticks); else t0=$(router_cpu_ticks); fi | ||
| taskset -c "$LOAD_CORES" "$OHA" --no-tui --output-format json -n "$n" "${ohaka[@]}" "$url" \ |
There was a problem hiding this comment.
The upstream oha usage documents -t <TIMEOUT> as the per-request timeout and says the default is infinite, but this measurement invocation (and the warmups below) never set it. If a Unit regression accepts a connection and then stalls a response, the harness can hang forever instead of failing the round and running cleanup; pass a finite timeout, preferably env-overridable, to every oha call.
Useful? React with 👍 / 👎.
| # ---- config payloads -------------------------------------------------------- | ||
| put_config_http() { # runsock port proxyport -> writes conf result, returns rc | ||
| local sock=$1 port=$2 pport=$3 out=$4 | ||
| curl -s --unix-socket "$sock" -X PUT -d '{ |
There was a problem hiding this comment.
I checked curl --help all, which lists --max-time as the maximum time allowed for a transfer; these control-socket PUTs do not set any transfer timeout. When the control socket exists but the controller is wedged or never returns the reconfiguration response, the script blocks here before it can mark the build-round failed or clean up, so add a finite --max-time to both config helpers.
Useful? React with 👍 / 👎.
1 participant
tools: event-engine A/B benchmark harness
tools/bench-engines.shruns an identical request-hot-path matrix against two unitd build dirs and reports RPS, p50/p99 latency, error counts and router CPU-µs/req per build, with a markdown summary (mean ± stdev + delta%) viatools/bench-engines-report.py.using "<name>" event enginelog line (aborts on an accidental A/A unless--allow-same).Used to produce the results in the io_uring engine PRs (#30/#31).
🤖 Generated with Claude Code