Skip to content

fix(powermetrics): time out the powermetrics subprocess - #1397

Open
davidberenstein1957 wants to merge 1 commit into
masterfrom
fix/powermetrics-subprocess-timeout
Open

fix(powermetrics): time out the powermetrics subprocess#1397
davidberenstein1957 wants to merge 1 commit into
masterfrom
fix/powermetrics-subprocess-timeout

Conversation

@davidberenstein1957

Copy link
Copy Markdown
Collaborator

Split out of #1333, which also rejects powermetrics on non-Apple-Silicon Macs. This is just the subprocess timeout, separated so it can land independently.

ApplePowermetrics._log_values() shells out with subprocess.call(cmd, universal_newlines=True) and no timeout. The command is sudo powermetrics ..., so if sudo has no cached credential and no tty to prompt on, or if powermetrics itself wedges, the call blocks forever. It is invoked from get_details() on every measurement, which means the tracker's measurement path hangs with no output and no way out short of killing the process.

This passes a timeout and, on subprocess.TimeoutExpired, logs a warning and returns None so that measure is skipped rather than blocking. The existing non-zero-returncode path is unchanged.

The timeout value is a heuristic, not a measured bound:

timeout = self._n_points * self._interval / 1000 * 2 + 5

_n_points samples at _interval milliseconds each is the nominal runtime of the command, so n_points * interval / 1000 is that runtime in seconds. It is doubled to absorb sampler overhead and scheduling jitter, and 5 seconds is added as a floor so that short configurations still get a usable margin. With the defaults (n_points=10, interval=100) that is 1 second of expected work and a 7 second limit.

Nothing measured picks the 2x and the +5; they are chosen to be comfortably loose so a healthy run never trips them, and the timeout only exists to bound a hang. If a reviewer has real numbers for how long powermetrics overruns its nominal duration under load, that is the input that should replace this formula.

tests/test_powermetrics.py gains a case asserting _log_values() returns None and warns once when subprocess.call raises TimeoutExpired. uv run pytest tests/test_powermetrics.py passes (17 tests).

subprocess.call had no timeout, so a hung powermetrics blocked the
measurement thread forever. Allow twice the expected sampling duration
plus a startup margin.
@davidberenstein1957
davidberenstein1957 requested a review from a team as a code owner August 19, 2026 14:26
@codecov

codecov Bot commented Aug 19, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.44%. Comparing base (3ec31a0) to head (c1f52ea).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1397   +/-   ##
=======================================
  Coverage   91.43%   91.44%           
=======================================
  Files          49       49           
  Lines        5057     5062    +5     
=======================================
+ Hits         4624     4629    +5     
  Misses        433      433           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant