Skip to content

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

Merged
benoit-cty merged 2 commits into
masterfrom
fix/powermetrics-subprocess-timeout
Aug 27, 2026
Merged

fix(powermetrics): time out the powermetrics subprocess#1397
benoit-cty merged 2 commits 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).

@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

❌ Patch coverage is 97.43590% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 91.44%. Comparing base (ab0714f) to head (9720594).

Files with missing lines Patch % Lines
codecarbon/core/powermetrics.py 97.43% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1397   +/-   ##
=======================================
  Coverage   91.43%   91.44%           
=======================================
  Files          49       49           
  Lines        5057     5073   +16     
=======================================
+ Hits         4624     4639   +15     
- Misses        433      434    +1     

☔ 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.

@benoit-cty

Copy link
Copy Markdown
Contributor

Some change made:

  1. The timeout now actually bounds the hang. Replaced subprocess.call(..., timeout=) with an explicit Popen + wait(timeout=), deliberately not under a context manager, since Popen.exit calls wait() with no timeout and would re-hang when the kill is refused. Added stdin=subprocess.DEVNULL so sudo gets EOF and exits non-zero instead of blocking on a password prompt — that removes the main hang at the source rather than relying on being able to kill a root-owned child.

  2. New _kill_process() helper that swallows OSError/SubprocessError from kill()/wait(), so a refused signal can't turn the handled TimeoutExpired into an escaping PermissionError, and closes the pipes the context manager used to close.

  3. Applied the same fix to _has_powermetrics_sudo(). It had the identical bug — a return False from inside with Popen(...) after a possibly-failing process.kill(), i.e. an unbounded wait() in exit during startup probing. It now uses a plain Popen, stdin=DEVNULL, and _kill_process(). The sudo-prompt detection still works: with DEVNULL, sudo writes "a terminal is required to read the password" to stderr, which the existing regex matches — just immediately instead of after the 3 s deadline.

  4. The skip is now a real skip. _log_values() returns bool, and get_details() returns {} when it's False instead of re-reading a log file that still holds the previous measure. Also {timeout:g} so the warning reads "7 seconds", not "7.0 seconds".

I deliberately left the returncode != 0 path returning True (warn, then read the file) — that's today's behaviour, and flipping it would silently turn real readings into zeros for anyone whose powermetrics exits non-zero but still writes data. Say the word if you'd rather that path also skip.

One thing this does not fix, and should be treat as a separate issue: when get_details() returns {}, AppleSiliconChip._get_power() records 0 W for that interval rather than omitting the sample. There's no "no measurement" concept at that layer, so a genuine skip needs a change in hardware.py.

davidberenstein1957 and others added 2 commits August 27, 2026 15:41
subprocess.call had no timeout, so a hung powermetrics blocked the
measurement thread forever. Allow twice the expected sampling duration
plus a startup margin.
1. The timeout now actually bounds the hang. Replaced subprocess.call(..., timeout=) with an explicit Popen + wait(timeout=), deliberately not under a context manager, since Popen.__exit__ calls wait() with no timeout and would re-hang when the kill is refused. Added stdin=subprocess.DEVNULL so sudo gets EOF and exits non-zero instead of blocking on a password prompt — that removes the main hang at the source rather than relying on being able to kill a root-owned child.

2. New _kill_process() helper that swallows OSError/SubprocessError from kill()/wait(), so a refused signal can't turn the handled TimeoutExpired into an escaping PermissionError, and closes the pipes the context manager used to close.

3. Applied the same fix to _has_powermetrics_sudo(). It had the identical bug — a return False from inside with Popen(...) after a possibly-failing process.kill(), i.e. an unbounded wait() in __exit__ during startup probing. It now uses a plain Popen, stdin=DEVNULL, and _kill_process(). The sudo-prompt detection still works: with DEVNULL, sudo writes "a terminal is required to read the password" to stderr, which the existing regex matches — just immediately instead of after the 3 s deadline.

4. The skip is now a real skip. _log_values() returns bool, and get_details() returns {} when it's False instead of re-reading a log file that still holds the previous measure. Also {timeout:g} so the warning reads "7 seconds", not "7.0 seconds".

I deliberately left the returncode != 0 path returning True (warn, then read the file) — that's today's behaviour, and flipping it would silently turn real readings into zeros for anyone whose powermetrics exits non-zero but still writes data. Say the word if you'd rather that path also skip.

One thing this does not fix, and I'd treat as a separate issue: when get_details() returns {}, AppleSiliconChip._get_power() records 0 W for that interval rather than omitting the sample. There's no "no measurement" concept at that layer, so a genuine skip needs a change in hardware.py.
@benoit-cty
benoit-cty force-pushed the fix/powermetrics-subprocess-timeout branch from 4ce030c to 9720594 Compare August 27, 2026 13:41

@benoit-cty benoit-cty left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks !

@benoit-cty
benoit-cty merged commit a8f103c into master Aug 27, 2026
14 checks passed
@benoit-cty
benoit-cty deleted the fix/powermetrics-subprocess-timeout branch August 27, 2026 13:48
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.

2 participants