[fix] Fix race condition in BlameCollector: skip hang dump when testhost hasn't launched yet by nohwnd · Pull Request #16065 · microsoft/vstest

nohwnd · 2026-05-25T14:07:35Z

The inactivity timer in BlameCollector starts during Initialize, but _testHostProcessId is only set later when TestHostLaunchedHandler fires. If the timer fires before testhost launches (short timeout, slow start, or in unit tests with TimeSpan.Zero), we try to dump PID 0 — the Windows Idle process.

The fix: don't start the timer until testhost actually launches. Initialize creates the timer, TestHostLaunchedHandler sets the PID and then starts it. The timer can never fire before we know who to dump.

Remove ResetInactivityTimer() from Initialize
Swap order in TestHostLaunchedHandler: set PID before starting timer
Add integration test that poisons runtimeconfig.json with net9999.0 so testhost fails to start, then runs with CollectHangDump — verifies vstest exits cleanly without hanging or dumping PID 0
Update unit tests to reflect that the timer starts on TestHostLaunched, not Initialize

When the inactivity timer fires before the testhost process has launched, _testHostProcessId is 0 (default int). Previously this caused ProcessDumpUtility.StartHangBasedProcessDump to attempt to dump PID 0 (the Idle process on Windows / Swapper on Linux), resulting in an empty or incorrect dump file. The fix adds an early-return guard in CollectDumpAndAbortTesthost: if _testHostProcessId == 0, log a warning and skip the dump/kill. Also updates three existing hang dump unit tests to properly simulate the happy-path scenario (testhost launches before the timer fires) by: - Using a 50 ms timeout instead of 0 ms so the TestHostLaunched event can be raised before the timer callback runs - Raising TestHostLaunched with PID 1234 before the timer fires Adds a new test that verifies StartHangBasedProcessDump is NOT called when the timer fires before TestHostLaunched. Fixes #15588 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Fixes a race in BlameCollector hang-dump collection where the inactivity timer can fire before TestHostLaunched sets the testhost PID, causing the collector to attempt a dump/kill against PID 0. The PR adds a guard to skip hang-dump collection when the testhost hasn’t launched yet, and updates unit tests/localized resources accordingly.

Changes:

Add early-return in CollectDumpAndAbortTesthost when _testHostProcessId == 0, with warning logs.
Make hang-dump unit tests deterministic by avoiding TimeSpan.Zero in several tests and explicitly raising TestHostLaunched.
Add localized resource string TestHostNotLaunchedCannotCollectHangDump across .resx, designer, and all 13 .xlf files.

Reviewed changes

Copilot reviewed 16 out of 17 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
test/Microsoft.TestPlatform.Extensions.BlameDataCollector.UnitTests/BlameCollectorTests.cs	Updates hang-dump timer tests and adds a regression test for “timer fires before testhost launched”.
src/Microsoft.TestPlatform.Extensions.BlameDataCollector/BlameCollector.cs	Skips hang-dump collection when testhost PID is not yet known (PID 0).
src/Microsoft.TestPlatform.Extensions.BlameDataCollector/Resources/Resources.resx	Adds new resource string for the “testhost not launched” warning.
src/Microsoft.TestPlatform.Extensions.BlameDataCollector/Resources/Resources.Designer.cs	Adds strongly-typed accessor for the new resource string.
src/Microsoft.TestPlatform.Extensions.BlameDataCollector/Resources/xlf/Resources.cs.xlf	Adds `TestHostNotLaunchedCannotCollectHangDump` localization entry (state=new).
src/Microsoft.TestPlatform.Extensions.BlameDataCollector/Resources/xlf/Resources.de.xlf	Adds `TestHostNotLaunchedCannotCollectHangDump` localization entry (state=new).
src/Microsoft.TestPlatform.Extensions.BlameDataCollector/Resources/xlf/Resources.es.xlf	Adds `TestHostNotLaunchedCannotCollectHangDump` localization entry (state=new).
src/Microsoft.TestPlatform.Extensions.BlameDataCollector/Resources/xlf/Resources.fr.xlf	Adds `TestHostNotLaunchedCannotCollectHangDump` localization entry (state=new).
src/Microsoft.TestPlatform.Extensions.BlameDataCollector/Resources/xlf/Resources.it.xlf	Adds `TestHostNotLaunchedCannotCollectHangDump` localization entry (state=new).
src/Microsoft.TestPlatform.Extensions.BlameDataCollector/Resources/xlf/Resources.ja.xlf	Adds `TestHostNotLaunchedCannotCollectHangDump` localization entry (state=new).
src/Microsoft.TestPlatform.Extensions.BlameDataCollector/Resources/xlf/Resources.ko.xlf	Adds `TestHostNotLaunchedCannotCollectHangDump` localization entry (state=new).
src/Microsoft.TestPlatform.Extensions.BlameDataCollector/Resources/xlf/Resources.pl.xlf	Adds `TestHostNotLaunchedCannotCollectHangDump` localization entry (state=new).
src/Microsoft.TestPlatform.Extensions.BlameDataCollector/Resources/xlf/Resources.pt-BR.xlf	Adds `TestHostNotLaunchedCannotCollectHangDump` localization entry (state=new).
src/Microsoft.TestPlatform.Extensions.BlameDataCollector/Resources/xlf/Resources.ru.xlf	Adds `TestHostNotLaunchedCannotCollectHangDump` localization entry (state=new).
src/Microsoft.TestPlatform.Extensions.BlameDataCollector/Resources/xlf/Resources.tr.xlf	Adds `TestHostNotLaunchedCannotCollectHangDump` localization entry (state=new).
src/Microsoft.TestPlatform.Extensions.BlameDataCollector/Resources/xlf/Resources.zh-Hans.xlf	Adds `TestHostNotLaunchedCannotCollectHangDump` localization entry (state=new).
src/Microsoft.TestPlatform.Extensions.BlameDataCollector/Resources/xlf/Resources.zh-Hant.xlf	Adds `TestHostNotLaunchedCannotCollectHangDump` localization entry (state=new).

Files not reviewed (1)

src/Microsoft.TestPlatform.Extensions.BlameDataCollector/Resources/Resources.Designer.cs: Language not supported

nohwnd · 2026-05-26T11:53:11Z

+        // Do NOT raise TestHostLaunched — _testHostProcessId stays 0.
+        warningLogged.Wait(1000, TestContext.CancellationToken);
+


Moot — reworked the fix to not start the timer until testhost launches. The PID==0 guard and warning log are gone; the test now verifies the timer never fires when testhost hasn't launched (no signal to wait for).

nohwnd

Review: Fix race condition in BlameCollector

Dimensions activated: Crash & Hang Dump Reliability · Error Reporting & Diagnostic Clarity · Parallel Execution & Scheduling Safety

Summary

The fix is correct and well-placed. Setting _inactivityTimerAlreadyFired = true before the guard ensures the timer is permanently defused regardless of the early-return path, and the test coverage faithfully exercises the new code path with TimeSpan.Zero.

One finding: _testHostProcessId is not volatile. The new guard makes this field load-bearing across a thread boundary for the first time — the timer thread now returns early based on reading 0. On x86/x64 this is unlikely to matter in practice, but volatile int is a one-liner fix that makes the cross-thread contract explicit and covers ARM64. See the inline comment for details.

Description alignment

PR description matches the diff exactly. Localization coverage (13 XLF files + resx + Resources.Designer.cs) is complete.

🧠 Reviewed by Expert Code Reviewer

🧠 Reviewed by Expert Code Reviewer 🧠

nohwnd · 2026-05-25T14:16:36Z

        }

+        // If testhost has not launched yet, we cannot dump or kill it.
+        if (_testHostProcessId == 0)


[Crash & Hang Dump Reliability / Parallel Execution & Scheduling Safety] — _testHostProcessId is not volatile

_testHostProcessId is written on the data-collection events thread in TestHostLaunchedHandler and now load-bearing read here on the timer-callback thread. The C# memory model does not guarantee that a plain int write on one thread is visible as a non-stale read on another without a memory barrier.

Without volatile (or Interlocked), the JIT is allowed to cache the 0 in a register and satisfy this guard with a stale value, causing the guard to fire and silently skip the hang dump even when testhost has already launched. On x86/x64 (TSO) this is unlikely to manifest, but it is technically a data race and can bite on ARM64 CI agents.

The field was already read without synchronization at lines 261, 271, 319, and 627 (pre-existing), but this new guard is the first place where the value of 0 has a meaningful semantic effect (early return vs. proceeding). Marking the field volatile would make the cross-thread visibility guarantee explicit and eliminate the theoretical false-positive:

// before private int _testHostProcessId; // after private volatile int _testHostProcessId;

volatile int is safe here — the field is only ever assigned once (in TestHostLaunchedHandler) and only read elsewhere, so no compare-and-swap semantics are needed.

Done — marked _testHostProcessId as volatile to make the cross-thread visibility guarantee explicit and eliminate the theoretical false-positive on weakly-ordered CPUs (ARM64).

🔧 Iterated by PR Iteration Agent 🔧

would use interlocked instead as that is more common, and less scary.

Went a step further — eliminated the cross-thread concern entirely. Timer now starts only after TestHostLaunchedHandler sets the PID, so the field is always written before any timer callback reads it. Reverted to plain int.

Ensures the hang-dump guard added in the parent commit sees a fresh value on the timer-callback thread (ARM64 / weakly-ordered CPUs). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

nohwnd · 2026-05-25T14:23:30Z

Commit pushed: 16a2275

🔧 Iterated by PR Iteration Agent 🔧

Instead of guarding against PID 0 in the timer callback, don't start the inactivity timer until TestHostLaunched fires. This eliminates the race at the source — the timer can never fire before we know which process to dump. - Remove ResetInactivityTimer() call from Initialize; timer is created but not started until TestHostLaunchedHandler sets the PID and calls ResetInactivityTimer. - Swap order in TestHostLaunchedHandler: set PID before starting timer. - Remove PID==0 guard and resource string (no longer needed). - Revert volatile — no cross-thread visibility concern with proper ordering. - Update tests to reflect that timer starts on TestHostLaunched, not Initialize. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Poisons runtimeconfig.json with net9999.0 so testhost can't launch, then runs with CollectHangDump. Verifies vstest exits cleanly without hanging or producing a dump against PID 0. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 16 comments.

        if (_collectProcessDumpOnHang)
        {
            _inactivityTimer ??= new InactivityTimer(CollectDumpAndAbortTesthost);
-            ResetInactivityTimer();
+            // Don't start the timer here — wait until TestHostLaunched so we know
+            // which process to dump. ResetInactivityTimer is called from TestHostLaunchedHandler.
        }


@@ -1,4 +1,4 @@
-<?xml version="1.0" encoding="utf-8"?>
+<?xml version="1.0" encoding="utf-8"?>


@@ -1,4 +1,4 @@
-<?xml version="1.0" encoding="utf-8"?>
+<?xml version="1.0" encoding="utf-8"?>


@@ -1,4 +1,4 @@
-<?xml version="1.0" encoding="utf-8"?>
+<?xml version="1.0" encoding="utf-8"?>


@@ -1,4 +1,4 @@
-<?xml version="1.0" encoding="utf-8"?>
+<?xml version="1.0" encoding="utf-8"?>


@@ -1,4 +1,4 @@
-<?xml version="1.0" encoding="utf-8"?>
+<?xml version="1.0" encoding="utf-8"?>


nohwnd

Review: Fix race condition in BlameCollector (rework)

Dimensions checked: Crash & Hang Dump Reliability · Parallel Execution & Scheduling Safety · Algorithmic Correctness · Resource Management

Summary

The rework is a clear improvement over the previous iteration. The structural fix — don't start the timer until TestHostLaunchedHandler fires — completely eliminates the data-race concern that motivated the earlier volatile suggestion. Once the timer can only fire after _testHostProcessId is written (on the same thread), the System.Threading.Timer ThreadPool dispatch provides an implicit memory barrier, so plain int is correct here.

Specific code changes:

Initialize: Timer is created (??=) but no longer started — right fix, right place.
TestHostLaunchedHandler: PID assigned before ResetInactivityTimer() — the invariant is now enforced by construction.
CollectDumpAndAbortTesthost: TPDebug.Assert(_testHostProcessId != 0, ...) documents the invariant in debug builds. Any path that somehow bypassed the architectural guarantee would still hit the try/catch around StartHangBasedProcessDump in release, so the fallback handling is adequate.
XLF files: The 13 xlf changes are a BOM/XML-declaration normalisation — no new resource strings were added, consistent with the absence of a .resx diff.

Previous finding — resolved ✅

The volatile int _testHostProcessId finding from the prior review is fully resolved. The architectural fix is the right call — Interlocked/volatile would have been patch-over-the-symptom; not starting the timer early removes the cross-thread read at the source.

Description alignment

PR description matches the diff exactly. Localization coverage (13 XLF files, BOM-only change with no new strings) is consistent with the resx being unmodified.

🧠 Reviewed by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠

The test mutates SimpleTestProjectMessedUpTargetFramework's runtimeconfig.json, which is shared with ProcessesInteractionTests. Wrap in try/finally to restore the original content. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

nohwnd

Review: Fix race condition in BlameCollector (follow-up — test commits)

Dimensions checked: Crash & Hang Dump Reliability · Algorithmic Correctness · Resource Management

New commits since last review (14:02 UTC)

Two test-only commits were pushed after the previous review:

Unit test additions (InitializeWithDumpForHangShouldNotStartTimerIfTestHostHasNotLaunchedYet): Directly verifies the new invariant — timer does not fire when TestHostLaunched is never raised. Timeout changed from 0 → 50ms in the three existing timing-sensitive tests; this provides sufficient margin for Thread.Sleep(100) to distinguish a correctly dormant timer from one that actually fires. ✅
Integration test (HangDumpShouldNotHangWhenTestHostFailsToStart): Poisons runtimeconfig.json with "9999.0.0" to prevent testhost from launching, then asserts vstest exits with code 1 and emits no .dmp file. The final commit wraps the file mutation in try/finally to restore the original content — correct and necessary since SimpleTestProjectMessedUpTargetFramework is shared with ProcessesInteractionTests. The Assert.DoesNotContain(".dmp", StdOut, ...) parameter order is correct (needle first). ✅

Previously reviewed findings — still resolved ✅

The architectural fix (timer only starts in TestHostLaunchedHandler after PID is set) and the TPDebug.Assert(_testHostProcessId != 0, ...) invariant guard were both confirmed in the prior review. Nothing in the new commits regresses those findings.

🧠 Reviewed by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠

nohwnd · 2026-05-27T09:16:58Z

macOS leg timed out after 1h (infrastructure). Windows, Ubuntu, and Source-Build all passed. Ready to retry or merge — the fix is Windows-specific (hang dump with PID 0).

Reviewer preference: use Interlocked for cross-thread visibility instead of plain int assignment, as it is more common and explicit. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

nohwnd · 2026-05-28T07:10:54Z

Commit pushed: b179a7d

🔧 Iterated by PR Iteration Agent 🔧

Copilot

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.

        if (_collectProcessDumpOnHang)
        {
            _inactivityTimer ??= new InactivityTimer(CollectDumpAndAbortTesthost);
-            ResetInactivityTimer();
+            // Don't start the timer here — wait until TestHostLaunched so we know
+            // which process to dump. ResetInactivityTimer is called from TestHostLaunchedHandler.
        }


+        // If testhost has not launched yet, the timer should not have been started,
+        // so this callback should not fire. Assert to catch unexpected paths.
+        TPDebug.Assert(_testHostProcessId != 0, "CollectDumpAndAbortTesthost called but testhost has not launched yet.");
+


@@ -1,4 +1,4 @@
-<?xml version="1.0" encoding="utf-8"?>
+<?xml version="1.0" encoding="utf-8"?>


nohwnd

Review: Fix race condition in BlameCollector (follow-up — Interlocked.Exchange commit)

Dimensions checked: Parallel Execution & Scheduling Safety · Algorithmic Correctness

New commit since last review (2026-05-26T14:37Z)

Interlocked.Exchange(ref _testHostProcessId, args.TestHostProcessId) — replaces the plain int assignment in TestHostLaunchedHandler.

This is correct and clean. The architectural guarantee (timer cannot fire until ResetInactivityTimer() is called, which happens on the next line after the Exchange) already ensures the timer thread will always observe a non-zero PID. Interlocked.Exchange makes the full memory fence explicit and is a reasonable stylistic choice for cross-thread writes, consistent with the reviewer's preference.

No new issues introduced. All previously reviewed findings remain resolved. ✅

🧠 Reviewed by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠

nohwnd · 2026-05-29T15:32:58Z

+        var originalContent = File.ReadAllText(runtimeConfigJson);
+        try
+        {
+            var updatedContent = originalContent.Replace("\"version\": \"11.0.0", "\"version\": \"9999.0.0");


regex, so we don't have to update every time we update runtime.

Make the test do-not-parallelize, just so we avoid running in conflict with other possible test that edits the project.

Done — the test now uses Regex.Replace to match any "version": "x.y.z" pattern (so the test won't need updating as the runtime version changes), and the [DoNotParallelize] attribute was added to avoid conflicts with other tests that might read the same runtimeconfig.json file on disk.

🔧 Iterated by PR Iteration Agent 🔧

… runtime-not-found error Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

nohwnd · 2026-05-29T15:50:19Z

Commit pushed: a30e0c2

🔧 Iterated by PR Iteration Agent 🔧

IntegrationTestBase now auto-injects /diag and throws when tests pass it manually (see #16055). Remove the explicit /Diag argument to match the new convention. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

nohwnd · 2026-06-04T11:41:49Z

Commit pushed: e5bfefd

🔧 Iterated by PR Iteration Agent 🔧

nohwnd

Review: Fix race condition in BlameCollector (follow-up — /Diag cleanup commit)

New commit reviewed: e5bfefd — test: remove manual /Diag flag from HangDumpOnTimeout

Change: Removes the explicit /Diag argument from the HangDumpOnTimeout test in BlameDataCollectorTests.cs. IntegrationTestBase now auto-injects /diag and throws when tests pass it manually (per #16055). This is a 1-line mechanical update to match the new convention.

Assessment: No issues. The change is correct and complete. All prior findings remain resolved.

🧠 Reviewed by expert-reviewer workflow · [Dimensions checked: Test Harness Consistency]

🧠 Reviewed by Expert Code Reviewer 🧠

PortArgumentExecutor.Execute() was reading _commandLineOptions.Port from the shared CommandLineOptions.Instance singleton. When InProcessVsTestConsoleWrapper runs executor.Execute(args) on a background Task.Run thread, it initializes the port argument (e.g. port=1234), which overwrites CommandLineOptions.Instance.Port. If this races with another test that set Port=2345, the wrong port is passed to ConnectToClientAndProcessRequests. Fix: capture the port number in a local _port field during Initialize() and use that captured value in Execute() instead of re-reading the shared singleton. Fixes #15730 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Fix flaky EnableLoggersArgumentProcessorTests by resetting RunSettingsManager The test class uses RunSettingsManager.Instance (a static singleton) but did not reset it between tests, causing potential cross-test contamination on macOS where test execution order may differ. Add TestInitialize reset and TestCleanup to ensure each test starts with and leaves behind a clean RunSettingsManager state. This matches the existing pattern in RunSettingsManagerTests.cs. Fixes #15614 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Reorder: reset RunSettingsManager before SetupMockExtensions in TestInitialize Ensure clean state before setting up mocks, not after. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

nohwnd · 2026-06-06T10:18:55Z

Commit pushed: 59d0c36

🔧 Iterated by PR Iteration Agent 🔧

Copilot

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 15 comments.

@@ -1,4 +1,4 @@
-<?xml version="1.0" encoding="utf-8"?>
+<?xml version="1.0" encoding="utf-8"?>


@@ -1,4 +1,4 @@
-<?xml version="1.0" encoding="utf-8"?>
+<?xml version="1.0" encoding="utf-8"?>


@@ -1,4 +1,4 @@
-<?xml version="1.0" encoding="utf-8"?>
+<?xml version="1.0" encoding="utf-8"?>


@@ -1,4 +1,4 @@
-<?xml version="1.0" encoding="utf-8"?>
+<?xml version="1.0" encoding="utf-8"?>


@@ -1,4 +1,4 @@
-<?xml version="1.0" encoding="utf-8"?>
+<?xml version="1.0" encoding="utf-8"?>


@@ -1,4 +1,4 @@
-<?xml version="1.0" encoding="utf-8"?>
+<?xml version="1.0" encoding="utf-8"?>


@@ -1,4 +1,4 @@
-<?xml version="1.0" encoding="utf-8"?>
+<?xml version="1.0" encoding="utf-8"?>


nohwnd

Review: Fix race condition in BlameCollector (follow-up — two flaky-test commits)

New commits reviewed: ebdfa4cd — Fix flaky PortArgumentProcessor test by caching port locally (#16050) · 59d0c361 — [fix] Fix flaky EnableLoggersArgumentProcessorTests on macOS (#16054)

Dimensions checked: Parallel Execution & Scheduling Safety · Algorithmic Correctness · Null Safety & Boundary Validation

`PortArgumentProcessor.cs` — `ebdfa4cd`

Rating: ✅ Clean

The fix is correct. Execute() was reading _commandLineOptions.Port from the shared CommandLineOptions.Instance singleton. If InProcessVsTestConsoleWrapper ran Initialize() on a background thread and another test concurrently overwrote CommandLineOptions.Instance.Port, Execute() would pick up the wrong port.

Capturing _port = portNumber at the end of Initialize() and using _port throughout Execute() (including the TimeoutException error message) eliminates that race. The write to _commandLineOptions.Port is preserved — other code paths that read the singleton still see the correct value — but Execute() is now isolated from any subsequent mutation.

No issues.

`EnableLoggersArgumentProcessorTests.cs` — `59d0c361`

Rating: ✅ Clean

Adds RunSettingsManager.Instance = null before SetupMockExtensions() in TestInitialize and adds a matching TestCleanup. Resetting first ensures SetupMockExtensions() operates on a fresh instance rather than a dirty one left by a prior test. TestCleanup guarantees the null-reset runs even if the test throws, preventing contamination of subsequent tests.

[DoNotParallelize] was already present on the class, so no new cross-thread hazard is introduced.

No issues.

[Description] PR description is out of date

The PR title and description describe only the BlameCollector inactivity-timer race fix (Issue #15588). The PR diff now also includes:

src/vstest.console/Processors/PortArgumentProcessor.cs — port-capture fix for the flaky PortArgumentProcessor test (Issue #15730)
test/vstest.console.UnitTests/Processors/EnableLoggersArgumentProcessorTests.cs — RunSettingsManager reset for the flaky macOS test (Issue #15614)

These originated in separate PRs (#16050, #16054) that were merged into this branch. Consider updating the PR description to mention them so reviewers know all three issues are covered here.

🧠 Reviewed by expert-reviewing skill · Parallel Execution & Scheduling Safety · Algorithmic Correctness · Null Safety & Boundary Validation

🧠 Reviewed by Expert Code Reviewer 🧠

…config.json The regex [\d.]+ only matches digits and dots, so it fails to match pre-release version strings like '11.0.0-preview.3.26170.106'. With that pattern the runtimeconfig.json was not modified, testhost started successfully with the real runtime, and the assertion that '9999.0.0' appeared in the output always failed on CI. Change to [^"]+ to match any non-quote characters, which handles both stable ('11.0.0') and pre-release ('11.0.0-preview.X') versions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

nohwnd · 2026-06-07T08:51:03Z

Commit pushed: e946d17

🔧 Iterated by PR Iteration Agent 🔧

nohwnd

Review: Fix race condition in BlameCollector (follow-up — regex fix commit)

New commit reviewed: e946d177 — fix: use [^"] pattern to match pre-release version strings in runtimeconfig.json

Dimensions checked: Algorithmic Correctness · Crash & Hang Dump Reliability

`BlameDataCollectorTests.cs` — `e946d177`

Rating: ✅ Clean

The old pattern [\d.]+ only matches digits and dots. On CI with a pre-release SDK (e.g. 11.0.0-preview.3.26170.106), the Regex.Replace found no match, left the runtimeconfig.json unmodified, testhost started successfully against the real runtime, and the assertion that 9999.0.0 appeared in the output always failed.

The new pattern [^"]+ (C# verbatim: [^""]+) matches one or more non-double-quote characters — the correct boundary for a JSON string value. It handles stable (11.0.0) and pre-release (11.0.0-preview.3.26170.106) versions alike. The surrounding JSON structure ("version"\s*:\s*"...")
constrained the match to the version field value before this change too; broadening the character class doesn't affect correctness.

The try/finally restore, [DoNotParallelize], and StdErr || StdOut assertion from previous commits are all intact.

No issues.

Note: the PR description being out of date (does not mention the PortArgumentProcessor, EnableLoggersArgumentProcessorTests, or this regex fix) was flagged in the 2026-06-06 review. Still stands.

🧠 Reviewed by automated expert-reviewer workflow (PR #16065, commit e946d177)

🧠 Reviewed by Expert Code Reviewer 🧠

nohwnd · 2026-06-07T09:09:08Z

The expert reviewer has flagged the PR description as out of date (twice now, most recently in the review of commit e946d177). Here is a suggested replacement description that reflects everything in the current diff:

Fixes #15588

Changes

1. Fix race condition in BlameCollector

The inactivity timer in BlameCollector starts during Initialize, but _testHostProcessId is only set later when TestHostLaunchedHandler fires. If the timer fires before testhost launches (short timeout, slow start, or in unit tests with TimeSpan.Zero), we try to dump PID 0 — the Windows Idle process.

The fix: don't start the timer until testhost actually launches. Initialize creates the timer; TestHostLaunchedHandler sets the PID (via Interlocked.Exchange) and then starts it. The timer can never fire before we know who to dump.

Remove ResetInactivityTimer() from Initialize
Use Interlocked.Exchange to set _testHostProcessId in TestHostLaunchedHandler
Start timer only after PID is set
Update unit tests to reflect the new startup sequence

2. Integration test for hang dump when testhost fails to start

Adds a BlameDataCollectorTests test that poisons runtimeconfig.json with a non-existent TFM (net9999.0) so testhost fails to launch. The test runs with CollectHangDump and verifies vstest exits cleanly (exit code 1, runtime-not-found message in output) without hanging or dumping PID 0.

Uses [^"]+ regex pattern to replace version strings so the test works with both stable (11.0.0) and pre-release (11.0.0-preview.X) SDKs
[DoNotParallelize] to avoid file-system races with other tests modifying the same runtimeconfig.json
Restores the original runtimeconfig.json in a try/finally teardown

3. Fix flaky PortArgumentProcessor test

Caches the port number locally before the assertion loop to prevent a race condition on the port allocation (backport from #16050).

4. Fix flaky EnableLoggersArgumentProcessorTests on macOS

Addresses intermittent test failures on macOS caused by timing in the EnableLoggers argument processor initialization (backport from #16054).

🔧 Iterated by PR Iteration Agent 🔧

Copilot AI review requested due to automatic review settings May 25, 2026 14:07

Copilot started reviewing on behalf of nohwnd May 25, 2026 14:07 View session

Copilot AI reviewed May 25, 2026

View reviewed changes

nohwnd commented May 25, 2026

View reviewed changes

fix: mark _testHostProcessId as volatile for cross-thread visibility

16a2275

Ensures the hang-dump guard added in the parent commit sees a fresh value on the timer-callback thread (ARM64 / weakly-ordered CPUs). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

nohwnd mentioned this pull request May 26, 2026

Daily Maintenance Digest — 2026-06-11 #16066

Closed

Copilot AI review requested due to automatic review settings May 26, 2026 11:52

Copilot started reviewing on behalf of nohwnd May 26, 2026 12:15 View session

Copilot AI reviewed May 26, 2026

Copilot AI review requested due to automatic review settings May 26, 2026 13:53

Copilot started reviewing on behalf of nohwnd May 26, 2026 13:53 View session

Copilot AI reviewed May 26, 2026

View reviewed changes

nohwnd commented May 26, 2026

View reviewed changes

Use Interlocked.Exchange to set _testHostProcessId

b179a7d

Reviewer preference: use Interlocked for cross-thread visibility instead of plain int assignment, as it is more common and explicit. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings May 28, 2026 07:10

Copilot started reviewing on behalf of nohwnd May 28, 2026 07:11 View session

Copilot AI reviewed May 28, 2026

View reviewed changes

nohwnd commented May 28, 2026

View reviewed changes

nohwnd commented May 29, 2026

View reviewed changes

Comment thread test/Microsoft.TestPlatform.Acceptance.IntegrationTests/BlameDataCollectorTests.cs

test: use regex for version replacement, add DoNotParallelize, assert…

a30e0c2

… runtime-not-found error Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

This comment has been minimized.

Sign in to view

nohwnd commented Jun 4, 2026

View reviewed changes

nohwnd mentioned this pull request Jun 6, 2026

[aw] PR Iteration Agent 🔧 failed #16056

Closed

This comment has been minimized.

Sign in to view

nohwnd and others added 2 commits June 6, 2026 12:18

Copilot AI review requested due to automatic review settings June 6, 2026 10:18

This comment has been minimized.

Sign in to view

Copilot started reviewing on behalf of nohwnd June 6, 2026 10:19 View session

Copilot AI reviewed Jun 6, 2026

View reviewed changes

nohwnd commented Jun 6, 2026

View reviewed changes

This comment has been minimized.

Sign in to view

nohwnd commented Jun 7, 2026

View reviewed changes

nohwnd merged commit 86055d8 into main Jun 11, 2026
20 checks passed

nohwnd deleted the fix/issue-15588-hang-dump-race-condition-5bee74e4b58b61b6 branch June 11, 2026 15:04

dotnet-maestro Bot mentioned this pull request Jun 12, 2026

[release/11.0.1xx-preview5] Source code updates from microsoft/vstest dotnet/dotnet#7209

Closed

This was referenced Jun 23, 2026

[main] Source code updates from microsoft/vstest dotnet/dotnet#7319

Closed

[release/11.0.1xx-preview6] Source code updates from microsoft/vstest dotnet/dotnet#7356

Merged

		// Do NOT raise TestHostLaunched — _testHostProcessId stays 0.
		warningLogged.Wait(1000, TestContext.CancellationToken);

		@@ -1,4 +1,4 @@
		<?xml version="1.0" encoding="utf-8"?>
		<?xml version="1.0" encoding="utf-8"?>

Uh oh!

Conversation

nohwnd commented May 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

nohwnd May 26, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nohwnd left a comment

Choose a reason for hiding this comment

Review: Fix race condition in BlameCollector

Summary

Description alignment

Uh oh!

nohwnd May 25, 2026

Choose a reason for hiding this comment

Uh oh!

nohwnd May 25, 2026

Choose a reason for hiding this comment

Uh oh!

nohwnd May 26, 2026

Choose a reason for hiding this comment

Uh oh!

nohwnd May 26, 2026

Choose a reason for hiding this comment

Uh oh!

nohwnd commented May 25, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nohwnd left a comment

Choose a reason for hiding this comment

Review: Fix race condition in BlameCollector (rework)

Summary

Previous finding — resolved ✅

Description alignment

Uh oh!

nohwnd left a comment

Choose a reason for hiding this comment

Review: Fix race condition in BlameCollector (follow-up — test commits)

New commits since last review (14:02 UTC)

Previously reviewed findings — still resolved ✅

Uh oh!

nohwnd commented May 27, 2026

Uh oh!

nohwnd commented May 28, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

nohwnd left a comment

Choose a reason for hiding this comment

Review: Fix race condition in BlameCollector (follow-up — Interlocked.Exchange commit)

New commit since last review (2026-05-26T14:37Z)

Uh oh!

nohwnd May 29, 2026

Choose a reason for hiding this comment

Uh oh!

nohwnd Jun 1, 2026

Choose a reason for hiding this comment

Uh oh!

nohwnd commented May 25, 2026 •

edited

Loading

`PortArgumentProcessor.cs` — `ebdfa4cd`

`EnableLoggersArgumentProcessorTests.cs` — `59d0c361`

`BlameDataCollectorTests.cs` — `e946d177`