[fix] Include testhost process path in crash error messages

[Jakub Jareš (nohwnd)]

🤖 This is an automated fix.

Related #2952

Root Cause

When testhost process crashes, the error messages (Test host process crashed and Testhost process for source(s) '...' exited with error) include the stderr output from the crashed process but do not include the path to the testhost executable that was launched. This makes it difficult for users to verify which testhost was actually launched, especially when debugging path-related or version-related issues.

The ProxyOperationManager already tracks _testHostProcessId and calls GetTestHostProcessStartInfo to get the launch info — but the FileName from the TestProcessStartInfo was not being preserved for use in error messages.

Fix

Added a _testHostProcessFileName field to ProxyOperationManager that stores the testhost executable path when the process start info is prepared. A new private helper method BuildCrashErrorContext constructs the diagnostic message by combining the process path and stderr output:

• Both available: "Process path: /path/to/testhost\n{stderr}"
• Only path: "Process path: /path/to/testhost"
• Only stderr: "{stderr}" (backward-compatible, no change in behavior)
• Neither: "" (backward-compatible)

This is used in three error paths:

1. Crash during communication (TestHostManagerHostExited → RequestSender.OnClientProcessExit)
2. Startup crash (ThrowOnTestHostExited)
3. Connection timeout with crash (ThrowExceptionOnConnectionFailure)

No public API changes were made. The ITestRequestSender interface is unchanged.

Before

Test host process crashed : NullReferenceException: Object reference not set to an instance of an object

After

Test host process crashed : Process path: C:\path\to\testhost.exe
NullReferenceException: Object reference not set to an instance of an object

🔍 Triaged by Issue Repro Triage & Auto-Fix 🔍

[Copilot]

Pull request overview

This PR improves diagnosability of testhost crashes in the CrossPlatEngine by including the launched testhost (or host) process path alongside stderr in crash-related error messages, addressing issue #2952.

Changes:

• Persist the launched process path in ProxyOperationManager (_testHostProcessFileName) when preparing TestProcessStartInfo.
• Introduce BuildCrashErrorContext(...) to combine process path + stderr in a consistent way.
• Use the new crash context in multiple crash/exit error paths (OnClientProcessExit, ThrowOnTestHostExited, and connection failure handling).

[Jakub Jareš (nohwnd)]

Review: [fix] Include testhost process path in crash error messages

Dimensions activated: Error Reporting & Diagnostic Clarity, Process Architecture & Host Resolution, Parallel Execution & Scheduling Safety

✅ What looks good

• BuildCrashErrorContext is correct and clean — the switch expression handles all four cases with no logic errors.
• The _testHostProcessFileName field itself is well-placed; it's assigned before launch and the assignment ordering relative to the event subscription is safe.
• The fix in TestHostManagerHostExited (path 1) and ThrowOnTestHostExited (path 2) are both correct — in those paths the host has unambiguously exited.
• No public API surface changes; no IPC wire protocol impact.
• No null-safety issues: BuildCrashErrorContext handles null/empty inputs explicitly.
• Thread safety: follows the same pattern as existing _testHostProcessStdError field; no regression.

---

❌ Defect: ThrowExceptionOnConnectionFailure condition over-triggers (line 557)

The condition change || !StringUtils.IsNullOrWhiteSpace(_testHostProcessFileName) introduces a diagnostic regression for the connection-timeout scenario. See the inline comment for the full trace and a suggested fix.

The core problem: _testHostProcessFileName is set unconditionally before every launch and is never reset, so the added OR clause is true for every launch attempt. This causes the informative timeout message (including elapsed seconds, process ID, and the VSTEST_CONNECTION_TIMEOUT hint) to be replaced by TestHostExitedWithError: Process path: dotnet — which is both incorrect (the process didn't exit) and less actionable.

---

Description alignment

The PR description says path 3 covers "Connection timeout with crash", but the actual condition change covers all timeouts where a testhost was launched (i.e., virtually all connection failures). The description is slightly optimistic about the fix's precision; after correcting the condition, it would match the description accurately.

---

🧠 Reviewed by expert-reviewer workflow

🧠 Reviewed by Expert Code Reviewer 🧠

[Jakub Jareš (nohwnd)]

[Error Reporting — Diagnostic Regression] The || !StringUtils.IsNullOrWhiteSpace(_testHostProcessFileName) addition causes this condition to fire on every launch attempt, not just on crashes.

_testHostProcessFileName is set unconditionally at line 235 (before LaunchTestHostAsync) and is never reset. This means the condition is true whenever any testhost has been launched, regardless of whether it actually crashed.

Consider the normal connection-timeout path (testhost running, never connected):

• _testHostExited.IsSet = false → ThrowOnTestHostExited doesn't throw, falls through to here
• _testHostProcessStdError = "" (reset at start of SetupChannel, never updated since host didn't exit)
• _testHostProcessFileName = "dotnet" or "testhost.exe" → always truthy

Result: the informative timeout message (including process ID, elapsed seconds, and the environment variable hint) is overwritten with:

Testhost process for source(s) '...' exited with error: Process path: dotnet

The user sees a misleading "exited with error" message even though the process never exited — it just timed out.

Suggested fix: Keep the condition unchanged and instead append the path as context to the existing error messages, or scope the path to the errorMsg addendum only:

// After testhost process launched failed with error.
if (!StringUtils.IsNullOrWhiteSpace(_testHostProcessStdError))
{
    errorMsg = string.Format(CultureInfo.CurrentCulture, CrossPlatEngineResources.TestHostExitedWithError,
        string.Join("', '", sources),
        BuildCrashErrorContext(_testHostProcessFileName, _testHostProcessStdError));
}
else if (!StringUtils.IsNullOrWhiteSpace(_testHostProcessFileName))
{
    // Process launched but no stderr — append path as a diagnostic addendum.
    errorMsg += $" Process path: {_testHostProcessFileName}";
}

This preserves the existing diagnostic clarity for timeout scenarios while still surfacing the process path when it's the only available context.

[Jakub Jareš (nohwnd)]

Fixed. The || condition has been split into two cases:

1. stderr present → replace errorMsg with TestHostExitedWithError (crash path, unchanged behavior)
2. no stderr, but path available → append " Process path: {_testHostProcessFileName}" as an addendum to the existing timeout message, preserving the elapsed-seconds/process-ID/VSTEST_CONNECTION_TIMEOUT hint

This ensures connection-timeout diagnostics are never overwritten by a misleading "exited with error" message.

🔧 Iterated by PR Iteration Agent 🔧

[Jakub Jareš (nohwnd)]

Commit pushed: ca86acf

🔧 Iterated by PR Iteration Agent 🔧

[Jakub Jareš (nohwnd)]

Re-review: [fix] Include testhost process path in crash error messages

The previous review raised one REQUEST_CHANGES defect in ThrowExceptionOnConnectionFailure. The iteration agent has pushed a fix on commit ca86acf8. This review checks that fix.

✅ Previous defect — Resolved

The || condition that over-triggered on every launch (causing timeout messages to be replaced with misleading "exited with error" text) is correctly fixed. The updated code uses the exact else if pattern suggested:

// stderr present → crash: replace message
if (!StringUtils.IsNullOrWhiteSpace(_testHostProcessStdError))
{
    errorMsg = string.Format(...TestHostExitedWithError..., BuildCrashErrorContext(...));
}
// no stderr, path available → timeout: append as addendum only
else if (!StringUtils.IsNullOrWhiteSpace(_testHostProcessFileName))
{
    errorMsg += $" Process path: {_testHostProcessFileName}";
}

All three error paths now behave correctly:

1. Crash during communication — BuildCrashErrorContext used, path + stderr included ✅
2. Startup crash — BuildCrashErrorContext used, path + stderr included ✅
3. Connection timeout — timeout message preserved; path appended as addendum when no stderr ✅

✅ BuildCrashErrorContext helper

The 4-case switch expression is correct and exhaustive. No issues.

✅ Thread safety / field reset

_testHostProcessFileName follows the same access pattern as _testHostProcessStdError. Set unconditionally at line 235 before launch; read only in error paths after the process has exited or timed out. No regression.

---

🧠 Reviewed by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠

[Jakub Jareš (nohwnd)]

Commit pushed: 2aaace0

🔧 Iterated by PR Iteration Agent 🔧

[Copilot]

Pull request overview

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

[Jakub Jareš (nohwnd)]

Re-review: [fix] Include testhost process path in crash error messages

Dimensions activated: Error Reporting & Diagnostic Clarity, Parallel Execution & Scheduling Safety, Process Architecture & Host Resolution

This review covers the new commit 2aaace0 ("refactor: remove redundant comment in ThrowExceptionOnConnectionFailure") pushed since the previous re-review on ca86acf.

✅ New commit — clean

The third commit is a pure comment cleanup. No logic changes. The code is functionally identical to what was verified in the prior re-review.

✅ All three error paths — still correct

Path	Method	Behavior
Crash during communication	TestHostManagerHostExited → OnClientProcessExit	BuildCrashErrorContext(path, stderr) ✅
Startup crash	ThrowOnTestHostExited	BuildCrashErrorContext(path, stderr) ✅
Connection timeout	ThrowExceptionOnConnectionFailure	stderr → replace with crash msg; no stderr → append path as addendum ✅

✅ BuildCrashErrorContext — exhaustive and correct

The 4-case switch over (hasPath, hasError) covers all combinations. The stdError! null-forgiveness on the (false, true) branch is sound — hasError guarantees non-null at that point.

✅ Thread safety — no regression

_testHostProcessFileName follows the same access pattern as _testHostProcessStdError: written once before launch in SetupChannel, read only in error paths after the process has exited or timed out.

✅ No public API surface changes, no IPC wire impact

---

🧠 Reviewed by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠

[Jakub Jareš (nohwnd)]

Commit pushed: 9265d38

🔧 Iterated by PR Iteration Agent 🔧

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

[Jakub Jareš (nohwnd)]

Re-review: [fix] Include testhost process path in crash error messages

This review covers the three new commits pushed since the prior re-review on 2aaace0f:

• e52c37d — "resolving copilot comments" (adds 5 unit tests + 1 logic change)
• da05c88 — "attempt to fix pipeline: tests update" (RunTests.cs patterns)
• 9265d38 — "fix: update test assertions" (ExecutionTests.cs + Assert.DoesNotContain arg-order fix)

⚠️ Minor defect introduced by e52c37d (line 564)

The refactoring from $" Process path: {_testHostProcessFileName}" to BuildCrashErrorContext(_testHostProcessFileName, null) dropped the leading space separator. See the inline comment for details. This is a COMMENT-level finding — the error message is still readable and functional, just poorly formatted for the timeout-with-no-stderr path.

✅ New unit tests — correct

All five added tests in ProxyOperationManagerTests.cs exercise the right code paths and use correct Assert.Contains/Assert.DoesNotContain argument ordering (needle, haystack). The Assert.DoesNotContain argument-order fix from 9265d38 is also correct.

✅ ExecutionTests.cs assertion — correct

Simplifying from "Test host process crashed : Stack overflow." to "Stack overflow." makes the assertion format-independent and is the right approach.

✅ RunTests.cs patterns — correct

The updated wildcard patterns (Process path: *{Environment.NewLine}Stack overflow.*) correctly use Environment.NewLine consistently with BuildCrashErrorContext and exercise the right crash scenario. The using System; import added alongside is appropriate.

---

🧠 Reviewed by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠

[Jakub Jareš (nohwnd)]

[Error Reporting — Missing Separator] Commit e52c37d refactored errorMsg += $" Process path: {_testHostProcessFileName}" (with a leading space) to errorMsg += BuildCrashErrorContext(_testHostProcessFileName, null).

BuildCrashErrorContext returns $"Process path: {processFileName}" — no leading space. The result in the timeout-with-no-stderr path is now:

Testhost process for source(s) 'test.dll' timed out after 90 seconds without establishing connection.Process path: /path/to/testhost

instead of:

Testhost process for source(s) 'test.dll' timed out after 90 seconds without establishing connection. Process path: /path/to/testhost

The separator (space) from the original suggestion was intentional and was confirmed in the prior review. The refactor silently dropped it. The existing unit test (SetupChannelShouldIncludeProcessPathWhenTestHostTimesOutWithNoStdError) only checks Assert.Contains($"Process path: {processPath}", ex.Message) so it doesn't catch the missing separator.

Suggested fix — keep the separator explicit:

errorMsg += $" {BuildCrashErrorContext(_testHostProcessFileName, null)}";

or just:

errorMsg += $" Process path: {_testHostProcessFileName}";

[Jakub Jareš (nohwnd)]

Fixed. Changed line 564 from errorMsg += BuildCrashErrorContext(...) to errorMsg += $" {BuildCrashErrorContext(...)}" to restore the leading space. Also tightened the test assertion from Assert.Contains($"Process path: {processPath}", ...) to Assert.Contains($" Process path: {processPath}", ...) so the separator is explicitly verified going forward.

🔧 Iterated by PR Iteration Agent 🔧

[Jakub Jareš (nohwnd)]

Commit pushed: 6e50b54

🔧 Iterated by PR Iteration Agent 🔧

[Jakub Jareš (nohwnd)]

Re-review: [fix] Include testhost process path in crash error messages

This review covers commit 6e50b54 pushed since the prior re-review on 9265d38.

✅ Previous defect — Resolved

The missing space separator before Process path: in the timeout-with-no-stderr path is correctly fixed. The updated line:

errorMsg += $" {BuildCrashErrorContext(_testHostProcessFileName, null)}";

restores the leading space that was dropped when e52c37d refactored from the inline $" Process path: ..." to the helper call. The test assertion was also correctly tightened:

Assert.Contains($" Process path: {processPath}", ex.Message);

This now explicitly verifies the separator, so any future refactor that drops it will be caught.

✅ All three error paths — clean at HEAD

Path	Method	Behavior
Crash during communication	TestHostManagerHostExited → OnClientProcessExit	BuildCrashErrorContext(path, stderr) ✅
Startup crash	ThrowOnTestHostExited	BuildCrashErrorContext(path, stderr) ✅
Connection timeout	ThrowExceptionOnConnectionFailure	stderr → replace with crash msg; no stderr → append " Process path: ..." as addendum ✅

✅ All prior defects resolved

No open findings remain.

---

🧠 Reviewed by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠