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)" 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); +} 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(())