Fix: enhance error handling in process monitoring and update performa…#1273
Conversation
…nce thresholds for Linux and macOS
There was a problem hiding this comment.
Pull request overview
This PR updates the perf-monitor utility used by HardwareVisualizer to (1) provide more actionable diagnostics when the monitored process exits early, and (2) relax Linux/macOS memory thresholds to better match real-world baselines.
Changes:
- Increase Linux/macOS memory thresholds (avg + p95) in
perf-thresholds.toml. - Pipe and capture the monitored process’s stderr and include it in early-exit errors.
- Add a helper to format early-exit errors with phase context (warmup vs measurement).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| perf-thresholds.toml | Raises Linux/macOS max avg/p95 memory thresholds to reduce false failures on those platforms. |
| perf-monitor/src/monitor.rs | Enhances early-exit error reporting by piping stderr and formatting richer error messages during warmup/measurement. |
| for _ in 0..timing.warmup_seconds { | ||
| thread::sleep(Duration::from_secs(1)); | ||
| if guard.0.try_wait()?.is_some() { | ||
| return Err("Process exited during warmup".into()); | ||
| return Err(format_exit_error(&mut guard, "warmup")); | ||
| } |
There was a problem hiding this comment.
run_monitor calls try_wait() to detect early exit, then format_exit_error calls try_wait() again. If the first try_wait() already reaped the child, the second call can fail/return None, so the formatted error may miss the exit status (and you do an extra syscall). Capture the ExitStatus from the first try_wait() and pass it into format_exit_error (or restructure to call wait/try_wait only once).
| let child = Command::new(binary_path) | ||
| .stderr(Stdio::piped()) | ||
| .spawn()?; |
There was a problem hiding this comment.
Piping the child’s stderr without draining it can deadlock the monitored process if it writes enough output to fill the OS pipe buffer (which would stall the process and skew measurements). Consider inheriting stderr by default, or spawn a thread to continuously read/drain stderr into a bounded buffer that you can include in the error message on early exit.
| let child = Command::new(binary_path) | |
| .stderr(Stdio::piped()) | |
| .spawn()?; | |
| let child = Command::new(binary_path).spawn()?; |
| fn format_exit_error( | ||
| guard: &mut ProcessGuard, | ||
| phase: &str, | ||
| ) -> Box<dyn std::error::Error> { | ||
| let status = guard.0.try_wait().ok().flatten(); | ||
| let stderr = guard | ||
| .0 | ||
| .stderr | ||
| .take() | ||
| .and_then(|mut s| { | ||
| let mut buf = String::new(); | ||
| s.read_to_string(&mut buf).ok()?; | ||
| if buf.is_empty() { None } else { Some(buf) } | ||
| }); | ||
|
|
||
| let mut msg = format!("Process exited during {phase}"); | ||
| if let Some(st) = status { | ||
| msg.push_str(&format!(" (exit status: {st})")); | ||
| } | ||
| if let Some(err) = stderr { | ||
| msg.push_str(&format!("\n--- process stderr ---\n{err}")); | ||
| } | ||
| msg.into() | ||
| } |
There was a problem hiding this comment.
New format_exit_error behavior (including stderr capture and exit-status formatting) isn’t covered by tests, even though this module already has unit tests. Add targeted tests that spawn a short-lived process which writes to stderr and exits non-zero, and assert the returned error string includes the phase + captured stderr/exit status (using OS-specific commands gated by cfg).
…nce thresholds for Linux and macOS
Summary
Related Issues
Type of Change
fix/branch)feat/branch)refactor/branch)docs/branch)chore/branch)Screenshots / Videos
Test Plan
Checklist
npm run lint && npm run format/cargo tauri-lint && cargo tauri-fmt)npm test/cargo tauri-test)