From e945a357f73d36258d9f636e5c60a50436bd7141 Mon Sep 17 00:00:00 2001
From: dailz <dailz@192.168.1.254>
Date: Fri, 5 Jun 2026 15:52:01 +0800
Subject: [PATCH] fix(bench): warn on all reset_vm_hwm errors, not just
 PermissionDenied

Issue #38: warn_reset_hwm() silently swallowed non-permission I/O errors
from /proc/self/clear_refs (e.g. missing /proc, read-only procfs, kernel
incompatibility). This left users unaware that VmHWM reset failed and
memory peak data could be contaminated across suites.

Changes:
- runner.rs: all errors now produce a warning with specific failure reason;
  PermissionDenied retains 'try running as root' hint; AtomicBool warn-once
  prevents duplicate output across 7 suite runs
- main.rs: preflight check now uses warn_reset_hwm() instead of the vague
  can_reset_vm_hwm(), sharing the same warn-once mechanism
- metrics.rs: remove dead can_reset_vm_hwm() (no callers remaining)
- tests: add hwm_warned_flag_prevents_reentry and warn_reset_hwm_does_not_panic
---
 crates/bench/src/main.rs    |  4 +--
 crates/bench/src/metrics.rs |  8 -----
 crates/bench/src/runner.rs  | 59 +++++++++++++++++++++++++++++++++++--
 3 files changed, 58 insertions(+), 13 deletions(-)

diff --git a/crates/bench/src/main.rs b/crates/bench/src/main.rs
index 3ed5b38..d1ac93f 100644
--- a/crates/bench/src/main.rs
+++ b/crates/bench/src/main.rs
@@ -59,9 +59,7 @@ fn main() {
         std::process::exit(1);
     }
 
-    if !log_viewer_bench::metrics::MetricsCollector::can_reset_vm_hwm() {
-        eprintln!("WARNING: VmHWM reset unavailable (no root). Memory peak values may be contaminated across tests.");
-    }
+    log_viewer_bench::runner::warn_reset_hwm();
 
     println!("Running benchmarks...");
     let results = log_viewer_bench::runner::run_all(&config);
diff --git a/crates/bench/src/metrics.rs b/crates/bench/src/metrics.rs
index 29e7f80..811c396 100644
--- a/crates/bench/src/metrics.rs
+++ b/crates/bench/src/metrics.rs
@@ -80,14 +80,6 @@ impl MetricsCollector {
         })
     }
 
-    /// Check if we can reset VmHWM (i.e., can open `/proc/self/clear_refs` for writing)
-    pub fn can_reset_vm_hwm() -> bool {
-        std::fs::OpenOptions::new()
-            .write(true)
-            .open("/proc/self/clear_refs")
-            .is_ok()
-    }
-
     /// Get file inode number
     pub fn get_inode(path: &Path) -> std::io::Result<u64> {
         let meta = fs::metadata(path)?;
diff --git a/crates/bench/src/runner.rs b/crates/bench/src/runner.rs
index 380d48f..fc94465 100644
--- a/crates/bench/src/runner.rs
+++ b/crates/bench/src/runner.rs
@@ -1,6 +1,7 @@
 use crate::metrics::MetricsCollector;
 use crate::types::BenchmarkResult;
 use std::path::PathBuf;
+use std::sync::atomic::{AtomicBool, Ordering};
 
 /// All recognized benchmark suite identifiers.
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
@@ -65,14 +66,35 @@ pub struct BenchConfig {
     pub suites: Option<Vec<Suite>>,
 }
 
-fn warn_reset_hwm() {
+/// Track whether we have already warned about VmHWM reset failure.
+/// Prevents duplicate warnings across multiple suite runs.
+static HWM_WARNED: AtomicBool = AtomicBool::new(false);
+
+pub fn warn_reset_hwm() {
     if let Err(e) = MetricsCollector::reset_vm_hwm() {
+        if HWM_WARNED.swap(true, Ordering::Relaxed) {
+            return;
+        }
         if e.kind() == std::io::ErrorKind::PermissionDenied {
-            eprintln!("WARNING: VmHWM reset requires root: {e}");
+            eprintln!(
+                "WARNING: Failed to reset VmHWM via /proc/self/clear_refs: {e}. \
+                 Memory peak values may be contaminated across benchmark suites. \
+                 Try running as root."
+            );
+        } else {
+            eprintln!(
+                "WARNING: Failed to reset VmHWM via /proc/self/clear_refs: {e}. \
+                 Memory peak values may be contaminated across benchmark suites."
+            );
         }
     }
 }
 
+#[cfg(test)]
+fn reset_hwm_warned() {
+    HWM_WARNED.store(false, Ordering::Relaxed);
+}
+
 pub fn run_all(config: &BenchConfig) -> Vec<BenchmarkResult> {
     let mut results = Vec::new();
 
@@ -163,4 +185,37 @@ mod tests {
         let result: Result<Vec<Suite>, _> = names.iter().map(|s| s.parse()).collect();
         assert!(result.is_err());
     }
+
+    #[test]
+    fn hwm_warned_flag_prevents_reentry() {
+        use super::reset_hwm_warned;
+        use std::sync::atomic::Ordering;
+
+        reset_hwm_warned();
+        assert!(
+            !super::HWM_WARNED.load(Ordering::Relaxed),
+            "flag should be false after reset"
+        );
+
+        // Simulate the flag being set (as warn_reset_hwm would do on error)
+        super::HWM_WARNED.store(true, Ordering::Relaxed);
+
+        // swap should now return true (old value), indicating already warned
+        assert!(
+            super::HWM_WARNED.swap(true, Ordering::Relaxed),
+            "swap should return the previous value (true)"
+        );
+    }
+
+    #[test]
+    fn warn_reset_hwm_does_not_panic() {
+        use super::reset_hwm_warned;
+
+        reset_hwm_warned();
+        // Whether reset_vm_hwm succeeds or fails, warn_reset_hwm must not panic.
+        // Multiple calls must also be safe.
+        super::warn_reset_hwm();
+        super::warn_reset_hwm();
+        super::warn_reset_hwm();
+    }
 }