Skip to content

Comments

Fix PowerManager race condition by raising events synchronously#6226

Draft
guimafelipe wants to merge 1 commit intomainfrom
user/felipeda/fix-power-race-condition
Draft

Fix PowerManager race condition by raising events synchronously#6226
guimafelipe wants to merge 1 commit intomainfrom
user/felipeda/fix-power-race-condition

Conversation

@guimafelipe
Copy link
Contributor

Description

Fixes the race condition in \SystemSuspendStatusChanged\ (issue #5224) using the existing API — no new events or event args types needed.

Root Cause

All PowerManager callbacks follow this pattern:

  1. OS callback fires → sets cached value (e.g., \m_systemSuspendStatus)
  2. Calls \RaiseEvent()\ which uses \co_await resume_background()\ to post to a thread pool
  3. Race window: between step 1 and the handler running on the thread pool, another OS callback can overwrite the cached value

For \SystemSuspendStatusChanged, this reliably happens on wake from standby: \PBT_APMRESUMEAUTOMATIC\ and \PBT_APMRESUMESUSPEND\ fire on separate threads in rapid succession, causing both handlers to read \ManualResume\ instead of \AutoResume\ then \ManualResume.

Fix

Raise events synchronously from the OS callback thread instead of posting to the thread pool. This ensures the cached value is stable when the handler reads the property.

For \SystemSuspendStatusChanged\ specifically, a \std::mutex\ serializes the concurrent OS callbacks (\PBT_APMRESUMEAUTOMATIC\ and \PBT_APMRESUMESUSPEND\ fire on separate threads).

All other callbacks (\EnergySaver, \Battery, \Display, \PowerSource, \EffectivePowerMode, \UserPresence, etc.) receive the same synchronous dispatch fix since they have the same theoretical vulnerability.

Why not a new API?

PR #6202 proposed adding \SystemSuspendStatusChanged2\ with typed event args. That's unnecessary because the race is in the dispatch mechanism, not the API surface. By raising events synchronously (removing the \co_await resume_background()\ gap), the existing \SystemSuspendStatus\ property always returns the correct value when read from within the event handler.

Changes

  • \PowerNotifications.h: All *_Callback\ methods now call event objects directly instead of via \RaiseEvent()\
  • \PowerNotifications.h: Added \m_suspendCallbackMutex\ to serialize concurrent suspend/resume OS callbacks
  • No IDL changes, no new API surface, no breaking changes

Testing

Existing tests verify registration/unregistration and property reads. Full validation requires system suspend/resume events (put computer to standby, wake by pressing power button, verify \AutoResume\ followed by \ManualResume).

Fixes #5224

The PowerManager callbacks set cached values then dispatch events
asynchronously via co_await resume_background(). This creates a window
where a subsequent OS callback can overwrite the cached value before
the first event handler reads it.

For SystemSuspendStatusChanged, this is reliably reproduced on wake
from standby: PBT_APMRESUMEAUTOMATIC and PBT_APMRESUMESUSPEND fire
on separate threads in rapid succession, causing both handlers to
read ManualResume instead of AutoResume then ManualResume.

Fix: Raise all power events synchronously from the OS callback thread
instead of posting to the thread pool. This ensures the cached value
is stable when the handler reads it. For SystemSuspend specifically,
add a mutex to serialize the concurrent OS callbacks.

All other callbacks (EnergySaver, Battery, Display, PowerSource,
EffectivePowerMode, UserPresence, etc.) have the same theoretical
vulnerability and receive the same synchronous dispatch fix.

Fixes #5224

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PowerManager SystemSuspendStatus unreliable due to race condition

1 participant