fix: share HTTP client across workers, redact reports, harden servers (v1.7.1) - #47
Merged
Merged
Conversation
The executor built a brand-new reqwest::Client (and connection pool) on
every clone_for_worker() call. That's cheap to overlook but expensive to
run: staged profiles spawned one per worker before computing their
deadline, so slow spawning silently ate into stage durations (in the
worst case, an entire stage recorded zero requests); arrival-rate
profiles built one per pacing tick, capping achieved throughput well
below the configured target with saturated_ticks reporting nothing
wrong.
- HttpClient is now Clone (Arc-backed internally) and clone_for_worker
reuses the executor's existing client instead of rebuilding one.
- Staged-profile workers are spawned before the run deadline is fixed,
and wait on it via a shared OnceLock so slow startup can never be
charged against configured stage durations.
- Per-stage throughput is computed from observed wall-clock duration
(tracked from when a stage begins to when the next one begins, or the
run ends) rather than the planned duration, so a stage cut short by
cancellation or delayed by scheduling reports what it actually
achieved. Both durations are kept in the report.
- generate_summary() recovers a poisoned lock instead of panicking: it
runs after the load test finishes, so a panic there would discard a
completed run's results.
- extract_variables() parses the response body once per scenario step
instead of once per extracted variable.
- Multipart file uploads are read from disk once and cached (capped) by
resolved path instead of on every request.
- The CSV output channel is now bounded with an explicit drop-and-count
policy (surfaced as csv_dropped_rows) instead of unbounded, so a slow
writer can't make the collector buffer an unbounded backlog; a write
failure is also logged once instead of surfacing only at the end.
- Scenario variable substitution is a single pass over the template
(deterministic regardless of HashMap iteration order, and a
substituted value can no longer be mistaken for another placeholder);
URL substitution percent-encodes inserted values; the "unresolved
{{ var }}" guard now covers the URL, headers and body, not just
multipart fields.
Redactor was only ever applied when the live dashboard read results back for display. The JSON and CSV reports embedded RequestResult.error verbatim, and those are the artifacts that get uploaded to CI, attached to tickets, and committed as baselines. A connection error routinely quotes the full request URL, so an extracted token in a query string (or a configured credential header, via the config-literal layer) could reach a persisted report even though the dashboard would have redacted the same value. Executor now builds a Redactor from its Config once and redacts every result's error string before recording it, so every consumer of stored results — reports, CSV, and the dashboard's existing redaction of its own snapshot — sees the same already-redacted text.
… servers - Terminal live metrics (get_live_metrics) reported the whole-run average as "current" RPS, unlike the dashboard's snapshot() which already uses a 5-second window. During a ramp or a stage transition this made the terminal lag well behind reality; it now shares the same windowed calculation. - The Prometheus endpoint always bound 0.0.0.0 with no way to narrow it, unlike the live dashboard which defaults to loopback and warns when opened wider. It now honors a new `prometheus_bind` setting (default 127.0.0.1) and warns the same way when bound elsewhere. This is a default-behavior change: existing configs that relied on the endpoint being reachable from other hosts need to set `prometheus_bind: 0.0.0.0` explicitly. - Both the Prometheus and dashboard accept loops treated any accept() error as fatal, taking the whole endpoint down for the rest of the run over one transient failure (an exhausted file descriptor table under load, for instance). They now log and keep serving. - The dashboard bounds concurrent connections (a client opening many and never closing them could otherwise grow spawned tasks without limit) and now times out a connection that never finishes sending its request head, instead of parking the task forever.
- load_profile.stages was never checked against every stage being target_concurrency: 0, which validated but sent no traffic for the whole run — the same silent-zero-request shape as the staged-profile deadline bug, reached through configuration instead of timing. A single zero-concurrency stage (a ramp-down, as in samples/staged-load-profile.yaml) still validates; only "the whole profile does nothing" is rejected, with a warning for the single-stage case. - load_profile.max_concurrency is drained with a u32 permit count at the end of an arrival-rate run; a configured value that does not fit in a u32 would silently truncate the `as u32` cast and let the run return before every in-flight request actually finished. Now rejected at config validation instead.
Two small timing bugs in the terminal progress loop: - It only stopped when cancelled or when its own elapsed counter reached total_secs. If the executor returned for any other reason (an error, a profile whose own duration finished short of the originally planned total), the ticker kept idling — and the summary waited to print — for the rest of that planned duration. It now also stops as soon as executor.run() returns, via a dedicated signal separate from the SIGINT/SIGTERM cancellation token (so a normal completion is not mistaken for an interrupted one later, when the code checks whether to print the "cancelled early" warning). - tokio::time::interval fires its first tick immediately rather than after one period, which ran the displayed elapsed time a full second ahead of real wall-clock time for the whole run. The first tick is now discarded before the loop starts.
- Cargo.lock was gitignored on a binary crate, so two builds of the same commit could resolve different dependency versions and a Docker release build re-resolved dependencies fresh every time. Committed; the CI workflow's cache keys already hash it, so caching starts working correctly too. - The Docker runtime image ran as root with no USER directive, and the builder stage installed pkg-config/libssl-dev that nothing links (reqwest resolves rustls here, not the openssl-sys/native-tls crates). Now runs as a non-root user; the build dependencies are dropped. The app's data/results directories are made writable by any UID rather than just this one, since they're typically bind-mounted from the host and the container's UID will rarely match the host user's. Not exercised with a live `docker build`/`docker run` in this environment (no Docker daemon available here) — worth a manual check before/after merge. - `make dev` passed a bare positional argument the CLI has not accepted since the `compare` subcommand was added; fixed to `--config`. - `make clean-all` deleted the (previously gitignored, now committed) lockfile; dropped that step. - Documented the new `prometheus_bind` setting in the README, alongside `prometheus_port`. Version bumped to 1.7.1 (Cargo.toml and vars/version) for this fix release. Note: an existing `.github/workflows/unit-test.yaml` already runs fmt/clippy/build/test on every PR against master — an earlier pass at this review incorrectly reported no CI ran the tests, from a glob that missed the `.yaml` (vs `.yml`) extension.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A full review of v1.7.0 found one root cause that made both new load profiles report numbers the run never produced, plus a set of correctness, robustness and hygiene issues. This PR fixes all of them and bumps the version to 1.7.1.
Critical
Executor::clone_for_worker()rebuilt a wholereqwest::Client(and connection pool) instead of reusing the executor's own one. Client construction costs several milliseconds — comparable to or larger than an arrival-rate pacing tick period, and enough to silently steal from a staged profile's stage durations.HttpClientis nowClone(Arc-backed) and workers share the executor's client.OnceLock, fixed only once every worker has actually started — mirroring howmark_load_phase_started()already works.generate_summary()was the one metrics accessor that panicked on a poisoned lock — after the load test finishes, so a panic there would discard a completed run's results. Now recovers, like every other accessor already does.High
Executornow redacts before recording.csv_dropped_rowsin the summary), and a write failure logs once instead of surfacing only at the end.0.0.0.0with no way to narrow it, unlike the dashboard which defaults to loopback. Addedprometheus_bind(default127.0.0.1), with the same non-loopback warning the dashboard already has. This is a default-behavior change — configs relying on the endpoint being reachable from other hosts needprometheus_bind: 0.0.0.0explicitly.accept()error (e.g. an exhausted FD table under load), taking the whole endpoint down for the rest of the run. Now logged and the loop continues.Medium / correctness
extract_variables()re-parsed the response body once per extracted variable instead of once per scenario step.String::replacepasses (order-dependent underHashMapiteration, and a substituted value could be mistaken for another placeholder). Rewritten as a single pass over the original template. URL substitution now percent-encodes inserted values. The "unresolved{{ var }}" guard — previously multipart-only — now also covers the URL, headers and body.load_profile.stagesvalidated an all-zero-concurrency profile (sends no traffic all run); now rejected, while a single zero-concurrency stage (a legitimate ramp-down, persamples/staged-load-profile.yaml) still validates with a warning.load_profile.max_concurrencyis drained with au32permit count; a value too large to fit would silently truncate. Now rejected at config validation.total_secsleft it idling for the rest of the originally planned duration before the summary could print. Also fixed a one-tick timing drift frominterval's immediate first tick.Hygiene
Cargo.lockwas gitignored on a binary crate — committed (CI's cache keys already hash it, so caching starts working correctly too).pkg-config/libssl-devfrom the builder (nothing links them —reqwestresolvesrustlshere). Not exercised with a livedocker build/docker runin the environment this was authored in (no Docker daemon available there) — worth a manual check.make devpassed a bare positional arg the CLI hasn't accepted since thecomparesubcommand shipped; fixed to--config.make clean-alldeleted the (now-committed) lockfile; dropped that step.prometheus_bindsetting.Correction to my own earlier review pass: I initially flagged "no CI runs the tests" — that was wrong, from a glob that missed
.github/workflows/unit-test.yaml's.yamlextension. That workflow already runs fmt/clippy/build/test on every PR; no new workflow was added here.Deliberately out of scope (flagged as separate follow-ups, not bundled in to keep this diff reviewable): migrating off the deprecated
serde_yamlcrate, and teachingflux compareto record/compare the load configuration two reports were generated under.Test plan
cargo test— 154 passed, 0 failed (up from 138 on master; new regression tests added for each fix)cargo clippy --all-targets -- -D warnings— cleancargo fmt --check— cleancargo build --release— succeeds,--versionreports1.7.1Generated by Claude Code