tools: event-engine A/B benchmark harness (oha + ab)#32
Conversation
![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.
|
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.
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. |
|
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. |
|
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 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 866f462: a new group_owns_run() helper requires a live member of the group to still reference THIS run dir in /proc/PID/cmdline before any signaling, in both the post-run leak assert and sweep_stale_run. A genuinely leaked unitd always carries the run dir in argv (--control unix:<run>/control.sock, --pid <run>/unit.pid), so real-leak detection is preserved while a recycled pgid is ignored rather than mis-killed.
| | 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 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 1fe0600: MAX_FAILED is validated as a non-negative integer right after its default assignment (same style as the rounds check; usage error, exit 2). Verified: MAX_FAILED=abc and MAX_FAILED=-1 exit 2 with a clear message.
| 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 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 03e1101: every oha invocation (the measurement in oha_one and both warmups) now passes -t "$OHA_TIMEOUT" — env-overridable, default 30s, documented in the header. A timed-out request counts as an error, so a stalled build surfaces through the failed-requests gate instead of hanging the harness.
| # ---- 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 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 03e1101: both control-socket PUT helpers now pass --max-time "$CURL_MAXTIME" (env-overridable, default 30, documented in the header), so a wedged controller fails the round instead of blocking the script.
e0a68ed to
866f462
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 866f462523
ℹ️ 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".
| # The run dir is a full path under RUNBASE, so this can never match an | ||
| # unrelated unitd daemon. | ||
| local pids p g | ||
| pids=$(pgrep -f "unitd.*$run" 2>/dev/null | tr '\n' ' ') |
There was a problem hiding this comment.
When RUNBASE or a label contains regular-expression metacharacters (labels explicitly permit .), pgrep -f interprets the interpolated $run as a pattern rather than a literal path. For example, a run path ending in A.B-r1 also matches an unrelated unitd command line containing AxB-r1; this branch then kills that process's entire group without the exact group_owns_run verification used above. Escape the pattern or enumerate candidates and confirm each command line with a fixed-string match before signaling it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in a5f28cf: the sweep's cmdline branch no longer feeds $run to pgrep as a pattern — it enumerates unitd candidates with a fixed literal (pgrep -f unitd) and confirms each with grep -qaF -- "$run" /proc/$p/cmdline before signaling, the same idiom group_owns_run uses. Checked the script's other pgrep -f uses: the only one (-P $UNITD_PID -f router) is a metachar-free literal scoped by parent pid. Verified functionally: the old pattern matched a stranger's 'AxB-r1' argv for run 'A.B-r1'; the literal confirm selects only the genuine leak.
| fi | ||
| i=$((i+1)); sleep 0.1 | ||
| done | ||
| [ -S "$run/control.sock" ] || { echo "no control socket ($label)"; return 1; } |
There was a problem hiding this comment.
Fresh evidence beyond the cleanup-on-early-return fix is the startup path where the spawned process stays alive for the full wait but never creates either unit.pid or the control socket: this return runs while both global cleanup identifiers are still empty, and cleanup cannot recover them because resolve_pgid_from_run requires the missing pidfile. A unitd startup regression that hangs before writing its pidfile therefore leaves its setsid process group running, and the post-run scan also misses it because no unitd.pgid was written; retain the spawn PGID as a fallback and terminate it on this failure path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in a5f28cf: the spawn PGID is now recorded as the kill target immediately at spawn (PGID=$spawn_pid — setsid child's PID==PGID — plus an immediate <run>/unitd.pgid write for the external sweeper), so a unitd that hangs before writing its pidfile or opening the control socket is reaped by cleanup on the no-control-socket failure path and remains visible to the post-run scan. resolve_pgid_from_run overwrites with the authoritative pidfile-derived value once unitd is up.
| 'BEGIN{printf "%.2f", (b-a)*1000000/clk/n}') | ||
| printf '%s %s oha rps=%.0f p50ms=%.3f p99ms=%.3f failed=%s cpu_us_per_req=%s engine=%s\n' \ | ||
| "$label" "$scen" "$rps" "$p50" "$p99" "${failed:-0}" "$us" "${ENGINE[$label]}" \ | ||
| | tee -a "$RESULTS" |
There was a problem hiding this comment.
Fresh evidence beyond the results-file initialization check is that RESULTS=/dev/full successfully passes the zero-byte : > "$RESULTS" preflight but this tee -a fails; the subsequent if overwrites that pipeline status, so oha_one returns success. The same issue affects the ab writer and occurs if a filesystem fills during a long run, allowing an empty or incomplete results file and an exit status of 0; check and propagate every append failure.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in f80c9f1: both row writers now propagate the append status (if ! printf ... | tee -a "$RESULTS"; then ... return 1; fi), so a failing append fails the measurement instead of being overwritten by the trailing failed-count check. Verified with RESULTS=/dev/full: passes the zero-byte preflight as you noted, and the new guard catches it (rc=1, loud error).
| taskset -c "$LOAD_CORES" "$OHA" --no-tui --output-format json -t "$OHA_TIMEOUT" -n 2000 -c 16 \ | ||
| "http://127.0.0.1:$port/" >/dev/null 2>&1 |
There was a problem hiding this comment.
If the load generator fails or the server returns errors during this warmup, both the command status and JSON result are discarded and the round proceeds as though the build were successfully warmed. A transient timeout or router recovery confined to the warmup can therefore leave the subsequent measurement running against a cold or recovering build while the overall A/B run exits successfully; save and validate the warmup result using the same request-failure policy as measurements. The PHP warmup has the same unchecked behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in f80c9f1: both warmups now go through a validated warmup_one helper applying the same policy as measurements — oha exit status, parseable JSON, and failed-count vs MAX_FAILED — with output kept in $run/warmup-$tag.{json,err} for forensics and no result row. A failing warmup fails the build-round via the existing meas_failed path.
866f462 to
a5f28cf
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a5f28cff0b
ℹ️ 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".
| reap_child | ||
| UNITD_PID="" | ||
| PGID="" | ||
| } |
There was a problem hiding this comment.
After a normal teardown, RUN_CUR still points at a directory containing the dead daemon's numeric pidfile, so the later EXIT trap invokes cleanup again and resolve_pgid_from_run trusts that stale PID. On a high-churn host where the PID is reused before the summary finishes, this resolves the unrelated process's PGID and kill_pgid terminates its entire process group without the ownership check used by the stale-run sweeper; clear RUN_CUR once teardown finishes (or revalidate ownership before resolving it).
Useful? React with 👍 / 👎.
|
@claude please review this benchmark harness PR (tools/bench-engines.sh + report script). Focus on: (1) process-tree ownership — setsid/pgid ladder, stale-run sweeps, the fingerprinted kill paths (can it ever kill an unrelated process, or miss a genuine leak?); (2) measurement validity — A/B alternation, warmup validation, failure propagation into round/run status, anything that could silently record bogus numbers; (3) portability/robustness of the shell (set -u interactions, quoting, parsing of oha/ab output). It intentionally avoids bare 'unitd' pkill patterns and validates all inputs — check for gaps. |
|
@codex review — full review of the current head: kill-path safety of the pgid ladder and sweeps (never kill an unrelated process, never miss a genuine leak), measurement validity (warmups, A/B alternation, failure propagation), and shell robustness. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a5f28cff0b
ℹ️ 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 [ -z "$PGID" ] && [ -n "$RUN_CUR" ]; then | ||
| resolve_pgid_from_run "$RUN_CUR" 2>/dev/null || true |
There was a problem hiding this comment.
When cleanup has already completed, RUN_CUR still names the old run, so the unconditional EXIT trap invokes cleanup again and resolves the stale unit.pid. If that PID has been reused—especially after the last round or during PID churn—resolve_pgid_from_run accepts the unrelated process without checking its command line and kill_pgid signals its entire group. Clear RUN_CUR after cleanup or validate the pidfile identity before reusing it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed (folded into the process-tree commit, new head 790873f): cleanup no longer signals a group resolved from a stale pidfile without proof of ownership — it revalidates with group_owns_run before killing and clears RUN_CUR afterward so the EXIT trap cannot re-resolve a possibly-recycled pid.
| for p in $(pgrep -f unitd 2>/dev/null); do | ||
| grep -qaF -- "$run" "/proc/$p/cmdline" 2>/dev/null && pids="$pids $p" |
There was a problem hiding this comment.
When any unrelated long-lived command contains both unitd and this run path—for example, an observer reading <run>/unitd.pgid—this search treats it as stale Unit state and subsequently terminates its whole process group. A literal run-directory match proves only that the process references the benchmark files, not that it belongs to the daemon, so the sweep needs an executable/session identity check before signaling anything.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 790873f: the sweep now requires a real unitd process image (new proc_is_unitd, via /proc//exe) before signaling, so an innocent observer whose cmdline merely contains both "unitd" and the run path (e.g. tail -f /unitd.pgid) is no longer mistaken for a leak.
| for p in $(pgrep -g "$pgid" 2>/dev/null); do | ||
| grep -qaF -- "$run" "/proc/$p/cmdline" 2>/dev/null && return 0 |
There was a problem hiding this comment.
If the Unit main process dies while a router, controller, or application child remains, this ownership test misses the genuine leak: Unit rewrites child argv to unit: <process> in src/nxt_process.c:647, removing the run-directory arguments, while the fallback sweep also searches specifically for unitd. Thus a hard-killed harness can leave a live child group that neither the next-run sweep nor the post-run assertion recognizes; record a stable group identity rather than relying on current child command lines.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 790873f: an orphaned child (main died first) has its argv rewritten to "unit: " with no run-dir reference, so cmdline matching missed it. group_owns_run now also accepts a unitd-image member whose session id equals the recorded pgid — that session is exactly the one our setsid spawn created, so the genuine leak is caught while a recycled pgid is not.
| 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 |
There was a problem hiding this comment.
For the PHP scenario, the two snapshots enumerate only descendants alive at each endpoint. If an app worker exits during a measurement, all of its consumed ticks disappear from t1; if it is replaced, the snapshots also compare different process sets, so the reported tree CPU can be severely understated or even negative despite the code explicitly allowing worker churn. CPU accounting needs cumulative child usage or per-process identity tracking across the interval.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 486c4a4: tree CPU now sums cutime+cstime ($14+$15) in addition to utime+stime, so a PHP worker that was spawned and reaped within the window still contributes its ticks. Live children are never counted in a parent cutime, so there is no double-count.
| else | ||
| echo "$label php_ka SKIP php (config/listener failed)" | tee -a "$RESULTS" |
There was a problem hiding this comment.
When a build contains the PHP module and the generated app exists but applying its configuration or opening its listener fails, this path records a SKIP and leaves meas_failed unchanged. A genuine regression in one engine can therefore remove that side's PHP measurements while the harness exits successfully, so config/listener failure should propagate as a failed build-round rather than being treated like an unavailable optional dependency.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in dc10c01: a PHP config/listener failure when the module and app are present now sets meas_failed=1 (fails the round) instead of recording a SKIP that left the run green.
| group_alive() { # pgid -> 0 if any NON-zombie process remains in the group | ||
| local pgid=$1 p st | ||
| for p in $(pgrep -g "$pgid" 2>/dev/null); do |
There was a problem hiding this comment.
On a minimal host without pgrep, this command failure is redirected away and the empty loop makes group_alive report every live process group as dead. Consequently kill_pgid returns without sending TERM, the post-run assertion also sees no survivors, and the harness can leave the complete Unit tree running; discover pgrep up front or make enumeration errors distinct from an empty group.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 790873f: pgrep and ps are now required up front (exit 2 if missing) — without pgrep, group_alive reported everything dead, which would have left a whole unitd tree running behind a green exit.
| SUMMARIZER="$(dirname "$0")/bench-engines-report.py" | ||
| if [ "$HAVE_PY" = 1 ] && [ -f "$SUMMARIZER" ]; then | ||
| python3 "$SUMMARIZER" "$RESULTS" "$LABEL_A" "$LABEL_B" |
There was a problem hiding this comment.
If the Python reporter fails—for example because the results file becomes unreadable or the reporter encounters an unexpected value—its nonzero status is ignored and the harness can still exit 0 after producing no valid summary. Treat failure of either the Python reporter or the awk fallback as a run failure so automation cannot accept a benchmark whose reporting stage failed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 790873f: the summarizer exit status is captured (python rc / awk PIPESTATUS) into SUMMARY_FAILED and the script exits 1 if it failed, instead of silently ignoring a broken summary.
| trap cleanup EXIT | ||
| trap 'cleanup; exit 130' INT | ||
| trap 'cleanup; exit 143' TERM |
There was a problem hiding this comment.
When SIGTERM is sent only to the harness PID while it is synchronously waiting for oha or ab, Bash defers this trap until the foreground command returns, so the load generator and Unit server can continue for the full measurement—potentially many timeout waves—despite the requested termination. Track and signal the active load-generator child, or run it in a controllable job, before entering cleanup.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 790873f: the load generator now runs backgrounded with an interruptible wait, and cleanup kills $LOAD_PID, so an INT/TERM during a measurement is handled promptly instead of being deferred until the foreground oha/ab returns.
| ensure_phpapp() { # -> prints app root, or empty on failure | ||
| local root=$PHPAPP | ||
| if [ -z "$root" ]; then root="$RUNBASE/phpapp"; fi | ||
| if [ ! -f "$root/index.php" ]; then | ||
| mkdir -p "$root" 2>/dev/null || { return 1; } | ||
| printf '%s\n' '<?php echo "hello\n";' > "$root/index.php" 2>/dev/null || return 1 |
There was a problem hiding this comment.
When the caller supplies PHPAPP pointing to an existing application directory without index.php, this code silently writes a synthetic index into that user-owned directory. The documented auto-creation applies only when PHPAPP is unset, so an explicit app should be validated and left untouched rather than modified by a benchmark run.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 624c2b5: an explicit PHPAPP is validated up front to contain index.php (exit 2 otherwise), and ensure_phpapp never writes a synthetic script into a caller-supplied root — only into a temp dir it created.
| for tool in ("oha", "ab"): | ||
| table(rows, tool, la, lb, "rps", "req/s, higher better") | ||
| table(rows, tool, la, lb, "p99ms", "ms, lower better") | ||
| table(rows, tool, la, lb, "cpu_us_per_req", "router µs/req, lower better") |
There was a problem hiding this comment.
The reporter labels every CPU table as router µs/req, but php_ka rows are produced with mode=tree and include the main process, router, controller, and PHP workers. Readers can therefore interpret the PHP value as router-only CPU and compare it incorrectly with the HTTP rows; label the metric per scenario or use a neutral process-scope description.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 624c2b5: the report no longer labels every CPU column router us/req — it now states router-only for the http scenarios and whole-unitd-tree for php_ka, matching the tree-mode measurement the harness actually does for PHP.
a5f28cf to
790873f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 790873f292
ℹ️ 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".
| proc_is_unitd "$p" || continue | ||
| grep -qaF -- "$run" "/proc/$p/cmdline" 2>/dev/null && return 0 | ||
| sid=$(ps -o sid= -p "$p" 2>/dev/null | tr -d ' ') | ||
| [ "$sid" = "$pgid" ] && return 0 |
There was a problem hiding this comment.
When a retained unitd.pgid is later reused as the PID/PGID of an unrelated Unit daemon launched with setsid, that daemon naturally has SID == PGID, so this fallback declares ownership without establishing any relationship to run; sweep_stale_run or the post-run scan can consequently send TERM/KILL to the unrelated daemon. Fresh evidence beyond the earlier stale-PGID check is this SID-only orphan fallback; use a stable per-run identity rather than a reusable numeric session ID.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in ff00d4d: the orphan clause (sid==pgid) alone could match an unrelated setsid-d unitd that reused the numeric pgid. It is now gated on the session-leader slot: accept the orphan only when the pid==pgid process is vacant or a zombie. The kernel reserves a pgid while the group is non-empty, so the number cannot become a new pid until our last member exits — a LIVE process at pid==pgid therefore means the id was recycled (reject), while a vacant/zombie leader with surviving members is our crashed tree. I rejected the environ-token option because Unit s nxt_process_title overwrites the environ region for title space (unreliable /proc//environ), and starttime does not help the main-dead orphan case; the pgid-reservation invariant is robust and Unit-agnostic while preserving orphan-leak coverage.
| local run="$RUNBASE/$label-preflight" | ||
| local mods="$build/lib/unit/modules" | ||
| [ -d "$mods" ] || mods="$run/modules" | ||
| sweep_stale_run "$run" |
There was a problem hiding this comment.
When two invocations share RUNBASE and use the same labels, their deterministic preflight paths coincide, so the second invocation treats the first invocation's live, correctly fingerprinted Unit group as stale and kills it here before any port check; claim_run_dir can then delete its marked run directory as well. Fresh evidence beyond the invocation-scoped post-run scan is that this pre-run sweep has no invocation identity; use an invocation-specific root or locking that distinguishes active runs from crashed ones.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in ff00d4d: the pre-run sweep_stale_run could TERM a concurrent invocations live tree (same RUNBASE+labels → same deterministic run dir). Each round now holds an exclusive flock on .lock (a sibling of the run dir, so claim_run_dir s rm -rf leaves it) for its lifetime; the sweep only proceeds for a run whose lock is FREE (crashed), and a run held by a live sibling is skipped, never swept. A crashed run leaves a free lock so the external-sweeper story still holds. This composes with the 965 fix — a lock we can hold proves no live sibling. flock added to the required-tools preflight.
| key=$1" "$2; sum[key]+=rps; cnt[key]++ | ||
| } END{ for(k in sum) printf " %s mean_rps=%.0f (n=%d)\n", k, sum[k]/cnt[k], cnt[k] }' \ | ||
| "$RESULTS" | sort | ||
| [ "${PIPESTATUS[0]}" = 0 ] && [ "${PIPESTATUS[1]}" = 0 ] || SUMMARY_FAILED=1 |
There was a problem hiding this comment.
On a host with jq but no Python, an otherwise successful run always exits nonzero after using the awk fallback: the first [ command consumes and replaces PIPESTATUS, so the subsequent expansion of ${PIPESTATUS[1]} is empty and sets SUMMARY_FAILED=1. Fresh evidence beyond the earlier summary-failure handling is that the new status check rejects successful pipelines; copy both pipeline statuses into variables before executing either test.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed (new head ff00d4d): the summary block did [ ${PIPESTATUS[0]} ] && [ ${PIPESTATUS[1]} ], but the first [ command resets PIPESTATUS, so the second read got the tests status → spurious SUMMARY_FAILED=1 on a clean awk|sort run (jq-present, python-absent). Fixed by snapshotting sumstat=("${PIPESTATUS[@]}") immediately after the pipeline and testing from that. (A follow-on to our finding-136 fix.)
| curl -s --max-time "$CURL_MAXTIME" --unix-socket "$sock" -X PUT -d '{ | ||
| "listeners": {"*:'"$port"'": {"pass": "applications/hello"}}, | ||
| "applications": {"hello": { | ||
| "type": "php", "root": "'"$root"'", "script": "index.php", |
There was a problem hiding this comment.
When PHPAPP or the auto-created root under RUNBASE contains a valid pathname character that is special inside JSON, such as " or \, this direct interpolation produces an invalid or altered configuration payload, causing every PHP-capable build-round to fail despite the application being usable. Serialize the root as a JSON string rather than inserting the raw shell value.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in ff00d4d: put_config_php interpolated the app root raw into the JSON body, so a " or \ in PHPAPP (or the RUNBASE path) corrupted the payload. Added a json_string helper (jq/python) and the encoded root is embedded; round-tripped a quote/backslash path through a JSON parser.
| expand_cores() { # "0-3,5" -> one cpu id per line (N and N-M list parts) | ||
| echo "$1" | tr ',' '\n' | while IFS=- read -r lo hi; do | ||
| case "$lo" in ''|*[!0-9]*) continue ;; esac | ||
| case "${hi:-$lo}" in *[!0-9]*) continue ;; esac |
There was a problem hiding this comment.
When callers use valid taskset stride lists, such as SERVER_CORES=0-6:2 and LOAD_CORES=2-6:2, both affinity probes can succeed but this parser rejects each range because its upper field contains :2, leaving CORE_OVERLAP empty even though the masks overlap. I checked taskset --help, which states that ranges in list format can take a stride argument and gives 0-31:2 as an example; parse that documented syntax before deciding the server and load masks are disjoint.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in ff00d4d: expand_cores rejected the taskset stride form N-M:S (the :S landed in the hi field, failing the digit check, so a strided range expanded to nothing and overlapping strided masks passed as disjoint). It now parses the optional :stride and expands via seq lo stride hi. Verified: 0-6:2 → 0 2 4 6; 0-6:2 ∩ 2-6:2 detects overlap 2 4 6; 0-6:2 vs 1-6:2 is disjoint. (Follow-on to our finding-118 fix.)
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>
- 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>
- 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>
…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>
- 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>
790873f to
ff00d4d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff00d4d4d1
ℹ️ 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".
| # Reap any instance left in THIS run dir by a previously crashed/hard-killed | ||
| # harness before we probe the ports (an orphan holding them would otherwise | ||
| # trip the port_busy abort below on a run we can legitimately recover). | ||
| sweep_stale_run "$run" |
There was a problem hiding this comment.
Fresh evidence beyond the deletion fix is that, when the deterministic run path already belongs to a manually launched Unit daemon and lacks .bench-engines-run, this call executes before claim_run_dir performs its ownership check. The sibling lock will normally be free, so sweep_stale_run finds the real unitd whose command line references that foreign path and terminates its entire process group; only afterward does startup refuse to delete the directory. Check the ownership marker before sweeping as well as before deletion.
Useful? React with 👍 / 👎.
| PGID=$spawn_pid | ||
| echo "$spawn_pid" > "$run/unitd.pgid" 2>/dev/null || true |
There was a problem hiding this comment.
Fresh evidence beyond the pre-pidfile cleanup fix is the parent/child scheduling window immediately after this assignment: if INT or TERM arrives before the child executes setsid, spawn_pid exists but is still in the harness's process group, so group_alive "$PGID" reports no group and cleanup sends no signal. The orphaned child can then execute setsid and continue running Unit after the harness exits; retain the spawn PID separately and terminate it by PID when the expected group has not appeared yet.
Useful? React with 👍 / 👎.
| echo "cleanup: process group $pgid survived TERM after ~10s; sending KILL" >&2 | ||
| kill -KILL -- "-$pgid" 2>/dev/null |
There was a problem hiding this comment.
On a high-PID-churn host, TERM can make the original group empty and reap_child can release its leader PID; if that numeric PGID is reused before the next 0.2-second poll, group_alive silently starts tracking the unrelated replacement group and this line eventually sends KILL to it. Caller-side fingerprint checks cannot prevent reuse during the ladder itself, so the ladder needs the run fingerprint and must stop escalating as soon as the group no longer owns that run.
Useful? React with 👍 / 👎.
| mkdir -p "$RUNBASE" | ||
| # fail fast: an unwritable results file would otherwise only surface after | ||
| # the first measurement (or lose every tee -a silently under set +e) | ||
| : > "$RESULTS" || { echo "cannot write results file $RESULTS" >&2; exit 2; } |
There was a problem hiding this comment.
When two invocations use the same RESULTS path—most easily two runs started from the same working directory with the default—each truncates the file here before acquiring any run-directory lock. A second invocation can therefore erase rows already written by the first and then fail on the first preflight lock, while the original invocation continues, summarizes only its remaining rows, and exits successfully because no expected-row-count check detects the loss. Lock the results file for the whole invocation or write to an invocation-specific file.
Useful? React with 👍 / 👎.
| group_alive "$pgid" \ | ||
| && echo "cleanup: process group $pgid STILL has survivors after KILL" >&2 | ||
| return 0 |
There was a problem hiding this comment.
When a stale Unit process remains non-zombie after the TERM and KILL budgets—for example while blocked in uninterruptible I/O—this function only logs the survivor and still returns success. sweep_stale_run consequently proceeds to claim_run_dir, which deletes the old directory and overwrites its unitd.pgid with the new daemon's group; a stale preflight process with no listener can then survive through the measurements, contaminate CPU results, and evade the post-run scan while the harness exits successfully. Return failure here and abort before reclaiming the run directory.
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