Skip to content

Fix mixed synchronization for _runStartedClients in ParallelProxyExecutionManager#16202

Open
azat-msft wants to merge 1 commit into
microsoft:mainfrom
azat-msft:fix/runstartedclients-race-16195
Open

Fix mixed synchronization for _runStartedClients in ParallelProxyExecutionManager#16202
azat-msft wants to merge 1 commit into
microsoft:mainfrom
azat-msft:fix/runstartedclients-race-16195

Conversation

@azat-msft

Copy link
Copy Markdown
Member

Addresses task 4 of #16195.

Problem

_runStartedClients in ParallelProxyExecutionManager was incremented via Interlocked.Increment from a Task.Run lambda outside any lock, but read inside lock (_executionStatusLockObject) in HandlePartialRunComplete. The lock does not fence the atomic write performed on a concurrent thread, so per the C# memory model this is formally a data race (an existing // BUG comment acknowledged the concern).

Fix

Increment _runStartedClients under _executionStatusLockObject, matching how the sibling counter _runCompletedClients is already handled. This guards the write and read with the same lock.

Validation

  • dotnet build src/Microsoft.TestPlatform.CrossPlatEngine -c Release — 0 warnings, 0 errors.

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

…utionManager

_runStartedClients was incremented via Interlocked.Increment from a
Task.Run lambda outside any lock, while it is read inside
lock (_executionStatusLockObject) in HandlePartialRunComplete. The lock
did not fence the atomic write from the concurrent thread, which is
formally a data race per the C# memory model.

Increment the field under _executionStatusLockObject instead, matching
how _runCompletedClients is handled, so the write and read are guarded
by the same lock.

Addresses task 4 of microsoft#16195.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 2, 2026 15:31

Copilot AI 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.

Pull request overview

This PR updates ParallelProxyExecutionManager in CrossPlatEngine to eliminate mixed synchronization on the _runStartedClients counter, aligning its update strategy with the lock-based reads performed during run-completion handling (Task 4 of #16195).

Changes:

  • Moves _runStartedClients increment from an Interlocked.Increment performed outside a lock to a lock (_executionStatusLockObject)-guarded increment.
  • Adds explanatory comments clarifying why the increment is now performed under the execution-status lock.
Comments suppressed due to low confidence (1)

src/Microsoft.TestPlatform.CrossPlatEngine/Client/Parallel/ParallelProxyExecutionManager.cs:419

  • _runStartedClients is now incremented under _executionStatusLockObject, but the subsequent trace statements read _runStartedClients outside the lock. Since the motivation is to avoid mixed synchronization for this field, consider capturing a local snapshot under the lock (and use that for the trace lines) so all reads/writes in this block share the same synchronization pattern and the comment remains accurate.
                EqtTrace.Verbose("ParallelProxyExecutionManager.StartTestRunOnConcurrentManager: Initializing test run. Started clients: " + _runStartedClients);
                proxyExecutionManager.InitializeTestRun(testRunCriteria, eventHandler);

                EqtTrace.Verbose("ParallelProxyExecutionManager.StartTestRunOnConcurrentManager: Execution starting. Started clients: " + _runStartedClients);
                proxyExecutionManager.StartTestRun(testRunCriteria, eventHandler);

@nohwnd

nohwnd commented Jul 3, 2026

Copy link
Copy Markdown
Member

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants