From 14dd33419692fae4f6f79d29ec761ba10d136e83 Mon Sep 17 00:00:00 2001 From: Stefan Steiner Date: Sat, 5 Sep 2026 14:14:12 -0700 Subject: [PATCH 1/3] fix(mcp): identify hyperd by executable, not thread name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `slow_health_watchdog_reaps_hyperd_after_child_timeout` failed once on `test (ubuntu-latest)` and passed on a plain re-run. The CI log names the cause exactly: refusing to terminate reported PID 20613: process is "hyperdMain\n", not hyperd `stop_reported_hyperd` refuses to signal a PID whose identity it cannot confirm, and `validate_hyperd_process` confirmed identity with `ps -p -o comm=`. On Linux that reads `/proc//comm`, which is the **main thread's name** — not the process image — and `hyperd` renames its main thread to `hyperdMain` via `pthread_setname_np` during startup (the string and the `pthread_setname_np` import are both in the shipped binary). So on Linux the guard rejected the very process it existed to reap, every single time it ran. It looked healthy only because it was almost always *skipped*: the engine is normally already dead by the time the guard is reached, and the `Err(_) if !process_is_alive(pid)` arm forgives an identity failure on an exited process. The flake is therefore not a race in the watchdog's 250 ms poll at all — it is the probability that `hyperd` is still alive when the guard runs, multiplied by a guard that was deterministically wrong. Measured, rather than assumed: - On Linux (procps-ng 4.0.2, an executable named `hyperd` that renames its main thread as the real one does), over 200 iterations against a live process: the old `comm` probe rejected a genuine `hyperd` **200/200**; `readlink /proc//exe` rejected it **0/200**. - On macOS, 700 iterations of the failing test: **0** failures, because macOS `ps -o comm=` prints the executable *path*, so the platform cannot observe this bug. The guard was nonetheless reached with the engine still alive in ~26% of runs, and hit the full Linux-fatal condition in 5/700 (~0.7%) — the same order as a "failed once" CI flake. - The old `0.0.26359` pin shows the same rate as `0.0.26479` (3/200 vs 2/500), so the engine bump did not introduce this; it is pre-existing. Resolve identity from the kernel's own record of the mapped image — `/proc//exe` on Linux, which no `prctl`/`pthread_setname_np` can rewrite — and keep `ps -o comm=` on macOS/BSD, where it is a path and therefore already sound. This is a deterministic construction rather than a longer timeout: nothing about it depends on who wins the shutdown race. Add `hyperd_identity_guard_accepts_a_live_engine`, which validates identity against a running engine unconditionally. All three `slow_health_*` tests shared the defect through `stop_reported_hyperd`, and all three only exercised the guard by accident; this pins it on every run. --- hyperdb-mcp/tests/recovery_tests.rs | 68 ++++++++++++++++++++++++++--- 1 file changed, 62 insertions(+), 6 deletions(-) diff --git a/hyperdb-mcp/tests/recovery_tests.rs b/hyperdb-mcp/tests/recovery_tests.rs index c392f354..11386b8e 100644 --- a/hyperdb-mcp/tests/recovery_tests.rs +++ b/hyperdb-mcp/tests/recovery_tests.rs @@ -183,6 +183,36 @@ fn slow_health_watchdog_reaps_hyperd_after_child_timeout() { ); } +/// The reaping guard must recognize a live, fully started `hyperd`. +/// +/// Every `slow_health_*` test above reaps its engine through +/// `stop_reported_hyperd`, which refuses to signal a PID whose identity it +/// cannot confirm. That guard used to read `ps -o comm=`; on Linux that is the +/// main *thread* name, which `hyperd` rewrites to `hyperdMain`, so the guard +/// rejected the very process it existed to clean up. The rejection was only +/// forgiven when the engine had already exited, which made it a load-dependent +/// flake instead of a hard failure — the guard was passing by being skipped. +/// +/// Asserting against a running engine exercises the identity path +/// unconditionally, with no dependence on who wins a shutdown race. +#[test] +fn hyperd_identity_guard_accepts_a_live_engine() { + let temp = tempfile::TempDir::new().expect("create identity-probe directory"); + let hyper_pid_path = temp.path().join("hyperd.pid"); + let (hyper, _endpoint) = start_reported_hyper_process(temp.path(), &hyper_pid_path); + let pid = hyper.pid().expect("HyperProcess must own a child PID"); + + assert!( + process_is_alive(pid).expect("poll the live probe engine"), + "probe engine PID {pid} must be alive before its identity is validated" + ); + validate_hyperd_process(pid).expect("identity guard must accept a live, fully started hyperd"); + + hyper + .shutdown_timeout(Duration::from_secs(5)) + .expect("shut down the identity-probe engine"); +} + fn child_mode() -> Option { std::env::var_os(SLOW_HEALTH_CHILD_ENV)?; std::env::var(SLOW_HEALTH_CHILD_MODE_ENV).ok() @@ -815,8 +845,26 @@ fn process_is_alive(_pid: u32) -> Result { Err("process liveness polling is unsupported on this platform".to_string()) } -#[cfg(unix)] -fn validate_hyperd_process(pid: u32) -> Result<(), String> { +/// Resolve the executable backing `pid` from `/proc`, which records the image +/// the kernel actually mapped. +/// +/// Deliberately *not* `ps -o comm=`: on Linux that reports +/// `/proc//comm`, which is the **main thread's name**, and `hyperd` +/// renames its main thread to `hyperdMain` via `pthread_setname_np` during +/// startup. A `comm`-based identity probe therefore rejects a genuine +/// `hyperd`, and the reject is only forgiven when the engine has already +/// exited — so the guard failed exactly when it had real work to do. The +/// `exe` link cannot be renamed by the process it describes. +#[cfg(target_os = "linux")] +fn reported_executable(pid: u32) -> Result { + std::fs::read_link(format!("/proc/{pid}/exe")) + .map_err(|error| format!("resolve executable of reported Hyper PID {pid}: {error}")) +} + +/// macOS/BSD have no `/proc`, but there `ps -o comm=` prints the executable +/// path rather than a thread name, so it is a sound identity source. +#[cfg(all(unix, not(target_os = "linux")))] +fn reported_executable(pid: u32) -> Result { let output = Command::new("ps") .args(["-p", &pid.to_string(), "-o", "comm="]) .stdin(Stdio::null()) @@ -825,14 +873,22 @@ fn validate_hyperd_process(pid: u32) -> Result<(), String> { if !output.status.success() { return Err(format!("ps could not inspect reported Hyper PID {pid}")); } - let command = String::from_utf8_lossy(&output.stdout); - let executable = Path::new(command.trim()) + Ok(PathBuf::from( + String::from_utf8_lossy(&output.stdout).trim(), + )) +} + +#[cfg(unix)] +fn validate_hyperd_process(pid: u32) -> Result<(), String> { + let executable = reported_executable(pid)?; + let file_name = executable .file_name() .and_then(|name| name.to_str()) .unwrap_or_default(); - if executable != "hyperd" { + if file_name != "hyperd" { return Err(format!( - "refusing to terminate reported PID {pid}: process is {command:?}, not hyperd" + "refusing to terminate reported PID {pid}: executable is {}, not hyperd", + executable.display() )); } Ok(()) From 1684bfc21aaa3614904cb737ccbff8813b1c174c Mon Sep 17 00:00:00 2001 From: Stefan Steiner Date: Sat, 5 Sep 2026 14:14:33 -0700 Subject: [PATCH 2/3] fix(bench): report decimal MB, not MiB under an MB label MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `BenchRecord::mb_per_sec`, `fmt_mb` and the `memory_*_mb` helpers in `benches/common.rs` all divided byte counts by 1024² while labelling the result `MB`, so the harness emitted MiB. Their sibling formatters in the same module — `fmt_count`, `fmt_rate`, `fmt_size` — were already decimal, which made the MiB ones the outliers rather than the convention. The consequence reached the published numbers: in `BENCHMARK_GUIDE.md` the macOS tables are decimal MB while the native Windows tables are raw MiB, under one shared `MB/sec` header. Verified arithmetically against the recorded times and the 24 B/row schema — macOS full-scan at 3.218 s reads 745.8 (decimal; MiB would be 711.3), Windows sync `Inserter` at 22.716 s reads 100.8 (MiB; decimal would be 105.7), and Windows `query.filtered` at 1.263 s reads 90.6 on its 12 B/row projection (MiB; decimal 95.0). The gap is 4.86%. Make the harness decimal (`/1e6`) rather than relabelling to MiB. Relabelling would make the freshly re-measured macOS tables overstate by 4.86%; going decimal makes the harness self-consistent with its own sibling formatters, matches the conventional unit for I/O throughput, and validates the already-published macOS numbers. Also converts the same 1024²-under-an-`MB`-label divisions in `benchmark` (whose own `mb_per_sec` helper was binary while its `QueryBenchmarkResult::new` was already decimal — inconsistent within one file), `arrow_batching_benchmark` and `async_parallel_benchmark`. Installed RAM stays binary, with a comment saying why: a 36 GiB machine must report "36.0 GB", not "38.7". Windows tables are left exactly as measured and footnoted as MiB rather than multiplied by 1.048576. They are stale on three further axes — `hyperdb-api` 0.1.0-rc.1, rustc 1.92.0, and an unrecorded `hyperd` predating `0.0.26479` — so they need re-measuring regardless, and an arithmetic conversion would dress stale data up as a fresh sample. Their column header now reads `MiB/sec` so the table is not self-contradictory in the meantime. Windows benchmarks cannot be run from this host. Adds `hyperdb-api/tests/bench_common_tests.rs`, which pins the unit. The benches are registered as *examples* (`autobenches = false`), so `cargo test --benches` runs nothing and a `#[test]` inside a bench file would never execute; this target pulls in the same `common.rs` through the same `#[path]` include the benches use, so the assertions run under `make test`. Per AGENTS.md, converts the narrowing `i64 as i32` in `gen_id` to `TryFrom`. It is the canonical shared row generator, and past 2^31 rows the cast silently wrapped to duplicate and negative IDs — corrupting the very throughput numbers the suite publishes. --- docs/BENCHMARK_GUIDE.md | 24 ++- hyperdb-api/CHANGELOG.md | 10 ++ .../benches/arrow_batching_benchmark.rs | 17 +- .../benches/async_parallel_benchmark.rs | 10 +- hyperdb-api/benches/benchmark.rs | 8 +- hyperdb-api/benches/common.rs | 34 +++- hyperdb-api/tests/bench_common_tests.rs | 158 ++++++++++++++++++ 7 files changed, 236 insertions(+), 25 deletions(-) create mode 100644 hyperdb-api/tests/bench_common_tests.rs diff --git a/docs/BENCHMARK_GUIDE.md b/docs/BENCHMARK_GUIDE.md index d9b55ca2..e1d0b5b5 100644 --- a/docs/BENCHMARK_GUIDE.md +++ b/docs/BENCHMARK_GUIDE.md @@ -136,6 +136,13 @@ Contributions welcome for additional platforms — paste the summary table under the appropriate section and include the host block from the suite's stdout. +> **Units.** `MB/sec` means **decimal** megabytes per second — 10^6 bytes/s — +> matching what `benches/common.rs` emits and the conventional unit for I/O +> throughput. Every other unit the harness prints (`fmt_count`, `fmt_rate`, +> `fmt_size`) is decimal too. The one exception in this document is flagged +> inline: the [native Windows tables](#platform-windows-x86_64-native) predate +> the harness fix and are still in MiB/s. + ### Platform: macOS (Apple Silicon) **Hardware / software** @@ -332,7 +339,20 @@ unchanged — opt into `ArrowInserter` and `executeQueryColumnar` / #### Rust suite — 100M rows per workload, 4 parallel workers, TCP loopback -| Workload | Variant | Flavor | Rows | Time (s) | Rows/sec | MB/sec | +> **The `MB/sec` column below is MiB/s (2^20 B/s), not decimal MB/s.** These +> numbers were captured on 2026-05-02, when the harness divided byte counts by +> 1024² while labelling the result `MB`. That has since been corrected to +> decimal, so this table reads **4.86% low** against every other table here. +> +> The cells are left exactly as measured rather than multiplied by 1.048576. +> The run is also stale on three other axes — `hyperdb-api` 0.1.0-rc.1, rustc +> 1.92.0, and an unrecorded `hyperd` predating the current `0.0.26479` pin — so +> it needs re-measuring regardless, and an arithmetic conversion would make +> stale data look freshly sampled. **These figures will be restated from a real +> run the next time the suite is executed on native Windows**; until then, +> compare them only against each other, never against the macOS tables. + +| Workload | Variant | Flavor | Rows | Time (s) | Rows/sec | MiB/sec | |---|---|---|---:|---:|---:|---:| | insert.bulk | AsyncArrowInserter | async | 100.00M | 18.563 | 5.39 M/s | 123.3 | | insert.bulk | AsyncArrowInserter × 4 | async | 100.00M | 4.931 | 20.28 M/s | 464.1 | @@ -351,7 +371,7 @@ unchanged — opt into `ArrowInserter` and `executeQueryColumnar` / **Headline takeaways (Rust, native Windows / i9-10980XE):** -- **Parallel async inserts** are the throughput-dominant path — `spawn_blocking + ChunkSender × 4` reaches **20.9 M rows/s / 479 MB/s**, ~2× faster than sync inserts and within ~30% of the TCP loopback ceiling on this box. The 4-way parallel insert numbers are roughly on par with macOS / M3 Max in absolute throughput, suggesting hyperd's ingest path is *not* the bottleneck here. +- **Parallel async inserts** are the throughput-dominant path — `spawn_blocking + ChunkSender × 4` reaches **20.9 M rows/s / 479 MiB/s**, ~2× faster than sync inserts and within ~30% of the TCP loopback ceiling on this box. The 4-way parallel insert numbers are roughly on par with macOS / M3 Max in absolute throughput, suggesting hyperd's ingest path is *not* the bottleneck here. - **Single-connection sync query** went from 2.89 M/s (pre-2026-05 tuning) to **7.08 M/s** — a 2.5× improvement — after the read-window + TCP-buffer changes documented below. - **Single-connection sync inserts on Windows lag** native Linux/macOS by ~5× even after tuning. This is a residual `hyperd`-side gap; the parallel paths hide it because they exercise multiple ingest threads. diff --git a/hyperdb-api/CHANGELOG.md b/hyperdb-api/CHANGELOG.md index 25d17a27..f7526ee1 100644 --- a/hyperdb-api/CHANGELOG.md +++ b/hyperdb-api/CHANGELOG.md @@ -25,6 +25,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/). since the rename to `hyperdb-bootstrap`. The command failed with "package(s) `hyperd-bootstrap` not found in workspace", which is the first thing a new user saw when `HYPERD_PATH` was unset. +- The benchmark examples reported **MiB/s under an `MB/s` label**. `fmt_mb`, + `BenchRecord::mb_per_sec` and the `memory_*_mb` helpers in + `benches/common.rs`, plus the equivalent divisions in `benchmark`, + `arrow_batching_benchmark` and `async_parallel_benchmark`, all divided byte + counts by 1024². Their sibling formatters (`fmt_count`, `fmt_rate`, + `fmt_size`) were already decimal, so a single `MB/sec` column in + [docs/BENCHMARK_GUIDE.md](../docs/BENCHMARK_GUIDE.md) carried two different + units depending on which platform produced the row — a 4.86% discrepancy. + All `MB`-labelled output is now decimal (10^6). Installed-RAM reporting stays + binary, since RAM is conventionally quoted that way. ### Changed diff --git a/hyperdb-api/benches/arrow_batching_benchmark.rs b/hyperdb-api/benches/arrow_batching_benchmark.rs index a66d4b70..6c1ec459 100644 --- a/hyperdb-api/benches/arrow_batching_benchmark.rs +++ b/hyperdb-api/benches/arrow_batching_benchmark.rs @@ -43,6 +43,11 @@ use hyperdb_api::{ HyperProcess, Parameters, Result, SqlType, TableDefinition, TransportMode, }; +/// Bytes in one megabyte — decimal (10^6), matching the rest of the bench +/// suite. This benchmark is standalone (it does not pull in `common.rs`), so +/// it carries its own copy of the constant. +const BYTES_PER_MB: f64 = 1_000_000.0; + const DEFAULT_ROW_COUNT: usize = 10_000_000; const BATCH_SIZE: usize = 100_000; @@ -158,7 +163,7 @@ fn main() -> Result<()> { // Print database file size if let Ok(metadata) = std::fs::metadata(async_db_path) { - let size_mb = metadata.len() as f64 / (1024.0 * 1024.0); + let size_mb = metadata.len() as f64 / BYTES_PER_MB; println!("\nDatabase file size: {size_mb:.2} MB"); } @@ -237,10 +242,10 @@ fn main() -> Result<()> { let tcp_rows_per_sec = tcp_result.rows as f64 / tcp_result.elapsed.as_secs_f64(); let tcp_mb_per_sec = - tcp_result.total_bytes as f64 / (1024.0 * 1024.0) / tcp_result.elapsed.as_secs_f64(); + tcp_result.total_bytes as f64 / BYTES_PER_MB / tcp_result.elapsed.as_secs_f64(); let ipc_rows_per_sec = ipc_result.rows as f64 / ipc_result.elapsed.as_secs_f64(); let ipc_mb_per_sec = - ipc_result.total_bytes as f64 / (1024.0 * 1024.0) / ipc_result.elapsed.as_secs_f64(); + ipc_result.total_bytes as f64 / BYTES_PER_MB / ipc_result.elapsed.as_secs_f64(); let speedup = tcp_result.elapsed.as_secs_f64() / ipc_result.elapsed.as_secs_f64(); println!( @@ -386,7 +391,7 @@ fn run_benchmark( format_number(rows as usize), elapsed.as_secs_f64(), rows as f64 / elapsed.as_secs_f64(), - total_bytes as f64 / (1024.0 * 1024.0) / elapsed.as_secs_f64() + total_bytes as f64 / BYTES_PER_MB / elapsed.as_secs_f64() ); Ok(BenchmarkResult { @@ -398,7 +403,7 @@ fn run_benchmark( fn print_result_row_wide(name: &str, result: &BenchmarkResult, speedup: f64) { let rows_per_sec = result.rows as f64 / result.elapsed.as_secs_f64(); - let mb_per_sec = result.total_bytes as f64 / (1024.0 * 1024.0) / result.elapsed.as_secs_f64(); + let mb_per_sec = result.total_bytes as f64 / BYTES_PER_MB / result.elapsed.as_secs_f64(); println!( "║ {:29} │ {:8.2} │ {:11} │ {:6.1} │ {:8.2}x ║", @@ -566,7 +571,7 @@ async fn run_async_benchmark( format_number(rows as usize), elapsed.as_secs_f64(), rows as f64 / elapsed.as_secs_f64(), - total_bytes as f64 / (1024.0 * 1024.0) / elapsed.as_secs_f64() + total_bytes as f64 / BYTES_PER_MB / elapsed.as_secs_f64() ); Ok(BenchmarkResult { diff --git a/hyperdb-api/benches/async_parallel_benchmark.rs b/hyperdb-api/benches/async_parallel_benchmark.rs index 790889dd..b1b97bf4 100644 --- a/hyperdb-api/benches/async_parallel_benchmark.rs +++ b/hyperdb-api/benches/async_parallel_benchmark.rs @@ -59,7 +59,7 @@ use hyperdb_api::{ InsertChunk, Parameters, Result, SqlType, TableDefinition, }; -use common::{BYTES_PER_ROW, fmt_count, fmt_rate}; +use common::{BYTES_PER_MB, BYTES_PER_ROW, fmt_count, fmt_rate}; /// Await all `handles`, converting join errors into `hyperdb_api::Error` /// and collecting successful results. Replaces `futures::try_join_all` @@ -236,7 +236,7 @@ impl BenchTotals { self.total_rows() as f64 / self.wall_secs } fn agg_mb_per_sec(&self) -> f64 { - (self.total_bytes() as f64) / (1024.0 * 1024.0) / self.wall_secs + (self.total_bytes() as f64) / BYTES_PER_MB / self.wall_secs } /// Ratio of summed per-worker time to wall-clock time. ~N means /// near-perfect parallelism; 1.0 means fully serial. @@ -403,7 +403,7 @@ async fn arrow_worker( worker_id, fmt_count(rows), worker_time.as_secs_f64(), - (total_bytes as f64) / (1024.0 * 1024.0) / worker_time.as_secs_f64() + (total_bytes as f64) / BYTES_PER_MB / worker_time.as_secs_f64() ); Ok(WorkerResult { @@ -531,7 +531,7 @@ fn chunk_sender_worker( worker_id, fmt_count(rows), worker_time.as_secs_f64(), - (total_bytes as f64) / (1024.0 * 1024.0) / worker_time.as_secs_f64() + (total_bytes as f64) / BYTES_PER_MB / worker_time.as_secs_f64() ); Ok(WorkerResult { @@ -674,7 +674,7 @@ async fn query_worker( worker_id, fmt_count(rows), worker_time.as_secs_f64(), - (bytes as f64) / (1024.0 * 1024.0) / worker_time.as_secs_f64().max(1e-9), + (bytes as f64) / BYTES_PER_MB / worker_time.as_secs_f64().max(1e-9), checksum ); diff --git a/hyperdb-api/benches/benchmark.rs b/hyperdb-api/benches/benchmark.rs index 88f41202..998d7ba6 100644 --- a/hyperdb-api/benches/benchmark.rs +++ b/hyperdb-api/benches/benchmark.rs @@ -40,7 +40,7 @@ use std::sync::Arc; use std::sync::mpsc; use std::thread; -use common::{ResourceMonitor, ResourceStats, SAMPLE_INTERVAL_MS}; +use common::{BYTES_PER_MB, ResourceMonitor, ResourceStats, SAMPLE_INTERVAL_MS}; // Default 10M rows for comparison with C++ benchmark const DEFAULT_ROW_COUNT: i64 = 10_000_000; @@ -98,12 +98,12 @@ fn bytes_per_row() -> usize { 24 } -/// Calculates MB/sec from bytes and elapsed time. +/// Calculates decimal MB/sec from bytes and elapsed time. fn mb_per_sec(bytes: f64, elapsed_secs: f64) -> f64 { if elapsed_secs <= 0.0 { return 0.0; } - bytes / elapsed_secs / (1024.0 * 1024.0) + bytes / elapsed_secs / BYTES_PER_MB } /// Result of a benchmark run including timing and resource stats. @@ -1223,7 +1223,7 @@ fn main() -> Result<()> { // Print database file size before deletion if let Ok(metadata) = std::fs::metadata(db_path) { let size_bytes = metadata.len(); - let size_mb = size_bytes as f64 / (1024.0 * 1024.0); + let size_mb = size_bytes as f64 / BYTES_PER_MB; println!("\nDatabase file size: {size_mb:.2} MB ({size_bytes} bytes)"); } diff --git a/hyperdb-api/benches/common.rs b/hyperdb-api/benches/common.rs index 1c8918ab..b8b89be6 100644 --- a/hyperdb-api/benches/common.rs +++ b/hyperdb-api/benches/common.rs @@ -15,7 +15,8 @@ //! - one `ResourceStats` + `ResourceMonitor` is the sole source of //! CPU/memory metrics — no more copy-pastes drifting apart. //! - formatting helpers (`fmt_count`, `fmt_rate`, `fmt_size`, -//! `fmt_mb`) produce identical output everywhere. +//! `fmt_mb`) produce identical output everywhere, all in decimal +//! units (see [`BYTES_PER_MB`]). //! - `HOST_ENV` collects the OS, CPU, RAM, Rust version, hyperd git //! hash so the unified result tables in `BENCHMARK_GUIDE.md` are //! self-describing. @@ -64,7 +65,9 @@ pub(crate) const BYTES_PER_ROW: usize = 24; /// trivial (`SELECT COUNT(*)`, `SELECT SUM(value)` etc). #[inline] pub(crate) fn gen_id(start_id: i64, i: i64) -> i32 { - (start_id + i) as i32 + // Narrowing `as` would silently wrap past 2^31 rows and emit duplicate, + // negative IDs — corrupting the very throughput numbers being measured. + i32::try_from(start_id + i).expect("benchmark row IDs must fit the `id INT` column") } #[inline] pub(crate) fn gen_sensor_id(id: i32) -> i32 { @@ -110,14 +113,14 @@ impl ResourceStats { } else { let avg = self.memory_samples.iter().sum::() as f64 / self.memory_samples.len() as f64; - avg / (1024.0 * 1024.0) + avg / BYTES_PER_MB } } pub(crate) fn memory_max_mb(&self) -> f64 { - self.memory_samples.iter().copied().max().unwrap_or(0) as f64 / (1024.0 * 1024.0) + self.memory_samples.iter().copied().max().unwrap_or(0) as f64 / BYTES_PER_MB } pub(crate) fn memory_min_mb(&self) -> f64 { - self.memory_samples.iter().copied().min().unwrap_or(0) as f64 / (1024.0 * 1024.0) + self.memory_samples.iter().copied().min().unwrap_or(0) as f64 / BYTES_PER_MB } } @@ -177,6 +180,17 @@ impl ResourceMonitor { // Formatting helpers. // ============================================================================= +/// Bytes in one megabyte. +/// +/// **Decimal (10^6), not binary (2^20).** Every unit this module reports is +/// decimal: `fmt_count`, `fmt_rate` and `fmt_size` were already, but the +/// `MB`-labelled helpers divided by 1024² and so emitted MiB under an `MB` +/// label. That made a single `MB/sec` column in `BENCHMARK_GUIDE.md` carry two +/// different units depending on which platform's run produced the row — a +/// 4.86% discrepancy. Decimal is also the conventional unit for I/O +/// throughput. Anything labelled `MB` here divides by this constant. +pub(crate) const BYTES_PER_MB: f64 = 1_000_000.0; + /// Format a row count like `123.4K`, `12.3M`, `1.23B`. pub(crate) fn fmt_count(n: u64) -> String { if n >= 1_000_000_000 { @@ -203,12 +217,12 @@ pub(crate) fn fmt_rate(rows_per_sec: f64) -> String { } } -/// Format MB/sec with two decimals. +/// Format decimal MB/sec with one decimal place. pub(crate) fn fmt_mb(bytes: usize, elapsed_secs: f64) -> String { if elapsed_secs <= 0.0 { return "—".to_string(); } - let mb = bytes as f64 / (1024.0 * 1024.0); + let mb = bytes as f64 / BYTES_PER_MB; format!("{:.1} MB/s", mb / elapsed_secs) } @@ -257,6 +271,9 @@ impl HostEnv { .unwrap_or_default(); let physical = sysinfo::System::physical_core_count().unwrap_or(cpus.len()); let logical = cpus.len(); + // Deliberately binary (2^30), unlike the decimal `MB` throughput + // units above: installed RAM is universally quoted in binary units, + // so a 36 GiB machine must read "36.0 GB" here and not "38.7". let total_memory_gb = sys.total_memory() as f64 / (1024.0 * 1024.0 * 1024.0); HostEnv { @@ -358,11 +375,12 @@ impl BenchRecord { self.rows as f64 / self.elapsed_secs } } + /// Decimal megabytes per second — see [`BYTES_PER_MB`]. pub(crate) fn mb_per_sec(&self) -> f64 { if self.elapsed_secs <= 0.0 { 0.0 } else { - (self.bytes as f64 / (1024.0 * 1024.0)) / self.elapsed_secs + (self.bytes as f64 / BYTES_PER_MB) / self.elapsed_secs } } } diff --git a/hyperdb-api/tests/bench_common_tests.rs b/hyperdb-api/tests/bench_common_tests.rs new file mode 100644 index 00000000..2a549d8f --- /dev/null +++ b/hyperdb-api/tests/bench_common_tests.rs @@ -0,0 +1,158 @@ +// Copyright (c) 2026, Salesforce, Inc. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 OR MIT + +//! Unit coverage for the benchmark harness helpers in `benches/common.rs`. +//! +//! The benches themselves are registered as **examples** (`autobenches = +//! false`), so `cargo test --benches` runs nothing and a `#[test]` placed +//! inside a bench file would never execute. This target pulls the same +//! `common.rs` in through the same `#[path]` include the benches use, so the +//! helpers get real, `make test`-visible coverage. +//! +//! The point of these tests is to pin the **unit**. Every `MB`-labelled +//! helper here reports decimal megabytes (10^6). They used to divide by +//! 1024², emitting MiB under an `MB` label, which put two different units in +//! a single `MB/sec` column of `docs/BENCHMARK_GUIDE.md`. The assertions +//! below fail if anyone reintroduces a binary divisor. + +// Mirrors the relevant crate-level expectation every bench that includes +// `common.rs` declares. Only `cast_precision_loss` fires from this target — +// the truncation/sign/wrap casts live in the bench binaries, not in +// `common.rs` — and `expect` rejects a lint that never triggers. +#![expect( + clippy::cast_precision_loss, + reason = "benchmark harness: throughput math needs f64" +)] + +#[path = "../benches/common.rs"] +mod common; + +use common::{BYTES_PER_MB, BenchRecord, ResourceStats, fmt_count, fmt_mb, fmt_rate, fmt_size}; + +/// Tolerance-based float equality — the workspace denies `clippy::float_cmp`. +#[track_caller] +fn assert_close(actual: f64, expected: f64) { + assert!( + (actual - expected).abs() < 1e-9, + "expected {expected}, got {actual}" + ); +} + +/// One megabyte is 10^6 bytes, not 2^20. +#[test] +fn bytes_per_mb_is_decimal() { + assert_close(BYTES_PER_MB, 1_000_000.0); + assert!( + (BYTES_PER_MB - 1_048_576.0).abs() > 1.0, + "MB must not be MiB" + ); +} + +/// `fmt_mb` divides by 10^6: exactly 1 MB in 1 s is 1.0 MB/s. +#[test] +fn fmt_mb_is_decimal_per_second() { + assert_eq!(fmt_mb(1_000_000, 1.0), "1.0 MB/s"); + assert_eq!(fmt_mb(2_600_000, 2.0), "1.3 MB/s"); + assert_eq!(fmt_mb(45_000_000, 0.5), "90.0 MB/s"); + // Under the old binary divisor this read "0.9 MB/s". + assert_eq!(fmt_mb(1_048_576, 1.0), "1.0 MB/s"); +} + +/// A non-positive duration has no defined rate. +#[test] +fn fmt_mb_rejects_non_positive_elapsed() { + assert_eq!(fmt_mb(1_000_000, 0.0), "—"); + assert_eq!(fmt_mb(1_000_000, -1.0), "—"); +} + +/// `BenchRecord::mb_per_sec` is decimal, and agrees with `fmt_mb`. +#[test] +fn bench_record_mb_per_sec_is_decimal() { + let record = BenchRecord { + workload: "insert.bulk".to_string(), + flavor: "sync", + variant: String::new(), + rows: 1_000, + bytes: 24_000_000, + elapsed_secs: 2.0, + }; + // 24 MB over 2 s = 12 MB/s decimal (11.44 under the old MiB divisor). + assert_close(record.mb_per_sec(), 12.0); + assert_close(record.rows_per_sec(), 500.0); +} + +/// Guards the divide-by-zero path rather than emitting `inf`. +#[test] +fn bench_record_zero_elapsed_is_zero_not_infinite() { + let record = BenchRecord { + workload: "query.full_scan".to_string(), + flavor: "async", + variant: "1 connection".to_string(), + rows: 10, + bytes: 240, + elapsed_secs: 0.0, + }; + assert_close(record.mb_per_sec(), 0.0); + assert_close(record.rows_per_sec(), 0.0); +} + +/// The `memory_*_mb` accessors report decimal MB from raw byte samples. +#[test] +fn resource_stats_memory_is_decimal_mb() { + let stats = ResourceStats { + cpu_samples: vec![10.0, 30.0], + memory_samples: vec![1_000_000, 3_000_000], + sample_count: 2, + }; + assert_close(stats.memory_min_mb(), 1.0); + assert_close(stats.memory_max_mb(), 3.0); + assert_close(stats.memory_avg_mb(), 2.0); + assert!((stats.cpu_avg() - 20.0).abs() < 1e-5); + assert!((stats.cpu_max() - 30.0).abs() < 1e-5); +} + +/// Empty sample sets must not panic or divide by zero. +#[test] +fn resource_stats_empty_samples_are_zero() { + let stats = ResourceStats::default(); + assert_close(stats.memory_avg_mb(), 0.0); + assert_close(stats.memory_max_mb(), 0.0); + assert_close(stats.memory_min_mb(), 0.0); + assert!(stats.cpu_avg().abs() < 1e-5); +} + +/// The sibling formatters were already decimal; assert it so the whole +/// module keeps one unit system. +#[test] +fn sibling_formatters_are_decimal() { + assert_eq!(fmt_size(1_000_000), "1.00 MB"); + assert_eq!(fmt_size(1_000_000_000), "1.00 GB"); + assert_eq!(fmt_size(2_500), "2.50 KB"); + assert_eq!(fmt_size(999), "999 B"); + + assert_eq!(fmt_count(1_000), "1.0K"); + assert_eq!(fmt_count(1_500_000), "1.50M"); + assert_eq!(fmt_count(2_000_000_000), "2.00B"); + assert_eq!(fmt_count(42), "42"); + + assert_eq!(fmt_rate(1_000.0), "1.00 K/s"); + assert_eq!(fmt_rate(2_500_000.0), "2.50 M/s"); + assert_eq!(fmt_rate(1e9), "1.00 B/s"); +} + +/// `gen_id` must reject a row index that cannot fit the `id INT` column +/// rather than silently wrapping to a duplicate or negative ID. +#[test] +#[should_panic(expected = "benchmark row IDs must fit the `id INT` column")] +fn gen_id_panics_instead_of_wrapping_past_i32() { + let _ = common::gen_id(i64::from(i32::MAX), 1); +} + +/// Deterministic generators stay stable, so numbers compare across benches. +#[test] +fn row_generators_are_deterministic() { + assert_eq!(common::gen_id(1_000, 24), 1_024); + assert_eq!(common::gen_sensor_id(1_024), 4); + assert_close(common::gen_value(10), 1.0); + assert_eq!(common::gen_timestamp(1), 1_700_000_001_000); +} From 82d832b48d3630bc422bd81cdbc76c36df0d7867 Mon Sep 17 00:00:00 2001 From: "Stefan R. Steiner" Date: Sat, 5 Sep 2026 18:56:43 -0700 Subject: [PATCH 3/3] chore: Update required JSON for CI workflow --- .github/workflows/npm-build-publish.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/workflows/npm-build-publish.yml b/.github/workflows/npm-build-publish.yml index 6ab14573..ececb1a4 100644 --- a/.github/workflows/npm-build-publish.yml +++ b/.github/workflows/npm-build-publish.yml @@ -59,7 +59,8 @@ jobs: # silently on the v0.2.2 release. Exact-string match avoids # the whole class of regex-escaping bugs. REQUIRED_JSON='[ - "rustfmt","clippy","cargo-audit","cargo-deny", + "rustfmt","clippy (ubuntu-latest)","clippy (windows-latest)", + "cargo-audit","cargo-deny", "version consistency","publish dry-run", "test (ubuntu-latest)","test (macos-14)","test (windows-latest)", "hyperdb-api-node (build + smoke)"