Skip to content

fix: lock CPU power history swap - #1335

Open
davidberenstein1957 wants to merge 3 commits into
masterfrom
fix/cpu-power-history-race
Open

davidberenstein1957 wants to merge 3 commits into
masterfrom
fix/cpu-power-history-race

Conversation

@davidberenstein1957

@davidberenstein1957 davidberenstein1957 commented Aug 12, 2026 •

Copy link
Copy Markdown
Collaborator

Description

Fixes a lost-update race on CPU._power_history, which is appended to by the 1 Hz monitor scheduler thread (CPU.monitor_power) and drained by the measure_power_secs scheduler thread (CPU.total_power) — two distinct PeriodicScheduler timer threads. The read-then-rebind in total_power was not atomic, so any sample appended between the list comprehension and self._power_history = [] was silently discarded. Added a threading.Lock to CPU, held only for the O(1) list swap; _get_power_from_cpus() still runs outside the lock in both paths so a slow backend (e.g. the IntelPowerGadget subprocess) never blocks the monitor thread. Also removed an unreachable if not power_history_in_W: branch, since a sample is unconditionally appended before the average is taken.

Related Issue

Fixes #1315

Motivation and Context

In cpu_load mode the reported cpu_power is the mean of the buffered samples. The samples dropped in the unlocked swap window are exactly those taken while a slow measurement is in flight, so the resulting error is small but systematic and silent.

How Has This Been Tested?

New test tests/test_cpu_load.py::TestCPULoad::test_cpu_total_power_keeps_samples_added_while_draining makes the race deterministic (a list subclass that fires monitor_power() at iteration exhaustion, inside the lost-update window) and asserts the injected sample survives. It fails on master and passes with this change. uv run pytest tests/test_cpu_load.py -q → 9 passed.

Screenshots (if appropriate):

N/A

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

AI Usage Disclosure

  • 🟥 AI-vibecoded
  • 🟠 AI-generated
  • ⭐ AI-assisted
  • ♻️ No AI used

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the docs/how-to/contributing.md document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Note: uv run task format reformats a large number of unrelated files on the current tree, so only the two touched files (codecarbon/external/hardware.py, tests/test_cpu_load.py) are included in this PR.

@codecov

codecov Bot commented Aug 12, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.74%. Comparing base (e5e46ab) to head (44ef635).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1335      +/-   ##
==========================================
+ Coverage   91.70%   91.74%   +0.04%     
==========================================
  Files          49       49              
  Lines        5157     5160       +3     
==========================================
+ Hits         4729     4734       +5     
+ Misses        428      426       -2     

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

@davidberenstein1957
davidberenstein1957 marked this pull request as ready for review August 12, 2026 19:14
@davidberenstein1957
davidberenstein1957 requested a review from a team as a code owner August 12, 2026 19:14
@davidberenstein1957
davidberenstein1957 force-pushed the fix/cpu-power-history-race branch from d70b9ae to ed2b386 Compare August 19, 2026 09:18
@davidberenstein1957
davidberenstein1957 force-pushed the fix/cpu-power-history-race branch 2 times, most recently from 00f2a1e to b813d76 Compare August 19, 2026 14:29
The 1 Hz monitor scheduler thread appends to `CPU._power_history` while the
measurement scheduler thread drains it. The read-then-rebind in
`total_power()` is not atomic, so any sample appended between the list
comprehension and the rebinding was written to the discarded list and lost,
biasing the reported `cpu_power`.

Take the swap under a lock, held only for the O(1) rebinding so a slow
`_get_power_from_cpus()` backend never blocks the monitor thread. Also drop
the unreachable empty-history branch: a sample is always appended before the
average. The test asserts the drain invariant rather than the drain shape, so
it does not pin the implementation.

Closes #1315

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@davidberenstein1957
davidberenstein1957 force-pushed the fix/cpu-power-history-race branch from b813d76 to f72f432 Compare August 20, 2026 06:14
@benoit-cty

Copy link
Copy Markdown
Contributor

🤖 This review comment was written and posted by Claude Opus 5.5 (AI assistant), at the request of @benoit-cty. Findings were checked by reading the code and running tests locally (merged with current master where relevant), but please double-check before acting on them.

Verdict: ✅ Approve with nits

The race is real. The monitor scheduler can append to the old list between the list comprehension and the self._power_history = [] rebind, and that sample is lost. The lock is correct:

  • It is held only for the O(1) swap and the append. _get_power_from_cpus(), including the Power Gadget subprocess, runs outside it.
  • There is no nested locking, so no deadlock risk.
  • The removed empty-list branch was indeed unreachable.

56 tests pass on this PR merged with master.

Nits:

  1. The test only proves that monitor_power blocks on the lock. It does not show the actual fix: that a sample appended while total_power() is running is kept for the next window. A test that appends from a second thread during the swap and asserts the sample appears in the next total_power() would lock in the behaviour.
  2. The PR description describes a different test (a list subclass) from the one in the diff; please update it.
  3. The branch is behind master; please update it.

@davidberenstein1957

Copy link
Copy Markdown
Collaborator Author

Made the changes in 44ef635: merged master, added a test that a post-swap sample lands in the next window.

Not done: updating the description's test paragraph (my PR-edit was blocked; the diff's tests are the lock-blocking one plus the new next-window one, no list subclass).

This branch has not been deployed

No deployments
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.

CPU power samples are silently dropped by a race in CPU.total_power()

2 participants