[vs18.8] Replace ToolTask #13351 revert with STA-safe EOF fix (#13917)

[JanProvaznik]

Reasoning: fix in main is in VS canary channel for 2 weeks without issues, we can replace the revert reintroducing hang potential with the long term fix in servicing as promised to QB.

Replaces the earlier revert of the ToolTask grandchild-pipe-handle fix (#13351) on this servicing branch with the correct, STA-safe fix.

What this does

• Reverts the revert ([vs18.8] Revert "Fix ToolTask hang when grandchild processes inherit pipe handles (#13351)" #13946), reapplying the Fix ToolTask hang when grandchild processes inherit pipe handles #13351 ToolTask fix, its unit tests, and the ChangeWaves.md entry. The earlier revert's correct vs-insertion.yml TargetBranch (rel/insiders) is preserved.
• Cherry-picks 24787e2ec6 (increase EOF pipe timeout 2s -> 30s; adds the ToolTask.PipeEOFTimeout resource string).
• Cherry-picks Fix ToolTask EOF wait to be STA-safe via CountdownEvent (MSB4018 in AspNetCompiler) #13917 (1722e4d) which makes the EOF wait STA-safe via CountdownEvent (fixes MSB4018 in AspNetCompiler on STA threads).
• Bumps VersionPrefix 18.8.2 -> 18.8.3.

The resulting ToolTask.cs EOF-handling logic is identical to main. Feature stays gated behind ChangeWave Wave18_6.

[Copilot]

Pull request overview

This PR updates ToolTask’s stdout/stderr EOF handling on the vs18.8 servicing branch by reintroducing the grandchild-pipe-handle hang fix behind Wave18_6, making the EOF wait STA-safe using CountdownEvent, and surfacing a low-importance message when EOF is not observed within a bounded timeout.

Changes:

• Switches EOF coordination to a CountdownEvent (STA-safe) and bounds the EOF wait to avoid indefinite hangs when grandchild processes inherit redirected pipe handles.
• Adds a new localized resource string (ToolTask.PipeEOFTimeout) used when the EOF wait times out.
• Updates servicing version metadata and re-documents the wave-gated feature.

Reviewed changes

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

Show a summary per file

File	Description
src/Utilities/ToolTask.cs	Implements STA-safe EOF waiting via CountdownEvent, adds bounded EOF timeout logging, and manages countdown lifecycle.
src/Utilities/Resources/Strings.resx	Adds ToolTask.PipeEOFTimeout resource string.
src/Utilities/Resources/xlf/Strings.cs.xlf	Adds localization entry for ToolTask.PipeEOFTimeout.
src/Utilities/Resources/xlf/Strings.de.xlf	Adds localization entry for ToolTask.PipeEOFTimeout.
src/Utilities/Resources/xlf/Strings.es.xlf	Adds localization entry for ToolTask.PipeEOFTimeout.
src/Utilities/Resources/xlf/Strings.fr.xlf	Adds localization entry for ToolTask.PipeEOFTimeout.
src/Utilities/Resources/xlf/Strings.it.xlf	Adds localization entry for ToolTask.PipeEOFTimeout.
src/Utilities/Resources/xlf/Strings.ja.xlf	Adds localization entry for ToolTask.PipeEOFTimeout.
src/Utilities/Resources/xlf/Strings.ko.xlf	Adds localization entry for ToolTask.PipeEOFTimeout.
src/Utilities/Resources/xlf/Strings.pl.xlf	Adds localization entry for ToolTask.PipeEOFTimeout.
src/Utilities/Resources/xlf/Strings.pt-BR.xlf	Adds localization entry for ToolTask.PipeEOFTimeout.
src/Utilities/Resources/xlf/Strings.ru.xlf	Adds localization entry for ToolTask.PipeEOFTimeout.
src/Utilities/Resources/xlf/Strings.tr.xlf	Adds localization entry for ToolTask.PipeEOFTimeout.
src/Utilities/Resources/xlf/Strings.zh-Hans.xlf	Adds localization entry for ToolTask.PipeEOFTimeout.
src/Utilities/Resources/xlf/Strings.zh-Hant.xlf	Adds localization entry for ToolTask.PipeEOFTimeout.
src/Utilities.UnitTests/ToolTask_Tests.cs	Adds regression tests covering grandchild pipe-handle inheritance and output capture.
eng/Versions.props	Bumps servicing VersionPrefix to 18.8.3.
documentation/wiki/ChangeWaves.md	Documents the ToolTask grandchild-pipe hang fix under the appropriate change wave.

[github-actions]

Test Coverage Gaps

The core motivation for this PR is two-fold: (1) prevent the grandchild-pipe hang, and (2) make the wait STA-safe by choosing CountdownEvent over WaitHandle.WaitAll. Both new tests exercise only the default Wave18_6-enabled path. Two important scenarios have no coverage:

[MAJOR] Missing STA thread regression test

The entire reason #13917 switched from WaitHandle.WaitAll to CountdownEvent was that WaitHandle.WaitAll throws NotSupportedException on STA threads (breaking AspNetCompiler). Without a regression test, a future maintainer could revert to WaitHandle.WaitAll — all current tests would still pass — and silently reintroduce the MSB4018 failure.

Suggested test (Windows-only):

[WindowsFullFrameworkOnlyFact]  // STA apartments are a Windows/.NET Framework concept
public void ToolTaskExecuteDoesNotThrowOnSTAThread()
{
    Exception? caughtException = null;
    bool result = false;

    var thread = new Thread(() =>
    {
        using MyTool t = new MyTool();
        MockEngine3 engine = new MockEngine3();
        t.BuildEngine = engine;
        t.MockCommandLineCommands = "/c echo sta-test";
        try { result = t.Execute(); }
        catch (Exception ex) { caughtException = ex; }
    });
    thread.SetApartmentState(ApartmentState.STA);
    thread.Start();
    thread.Join();

    caughtException.ShouldBeNull($"ToolTask.Execute threw on STA thread: {caughtException}");
    result.ShouldBeTrue();
}

[MAJOR] Missing ChangeWave opt-out regression test

The else { proc.WaitForExit(); } branch (Wave18_6 disabled) is entirely untested in this PR. If that branch were accidentally removed, no test would catch it. ChangeWave discipline requires testing the opt-out path.

Suggested test:

[Fact]
public void ToolTaskLegacyBehaviorWhenWave18_6Disabled()
{
    using (ChangeWaveTestHelpers.DisableChangeWave(ChangeWaves.Wave18_6))
    using (MyTool t = new MyTool())
    {
        MockEngine3 engine = new MockEngine3();
        t.BuildEngine = engine;
        t.MockCommandLineCommands = NativeMethodsShared.IsWindows
            ? "/c echo legacy"
            : "-c \"echo legacy\"";

        bool result = t.Execute();

        result.ShouldBeTrue();
        engine.Log.ShouldContain("legacy");
    }
}

Generated by Expert Code Review (on open) for issue #14082 · 2.9K AIC · ⊞ 30.1K ambient context · ◷

[github-actions]

24-Dimension Review — PR #14082

#	Dimension	Verdict
1	Backwards Compatibility Vigilance	✅ LGTM
2	ChangeWave Discipline	⚠️ MODERATE — missing opt-out test
3	Performance & Allocation Awareness	✅ LGTM
4	Test Coverage & Completeness	⚠️ MAJOR — no STA test, no ChangeWave opt-out test
5	Error Message Quality	⚠️ MODERATE + NIT
6	Logging & Diagnostics Rigor	⚠️ MODERATE — MessageImportance.Low silent at default verbosity
7	String Comparison Correctness	✅ LGTM
8	API Surface Discipline	✅ LGTM
9	MSBuild Target Authoring Conventions	✅ N/A
10	Design Before Implementation	✅ LGTM
11	Cross-Platform Correctness	✅ LGTM
12	Code Simplification	💬 NIT — redundant ChangeWave re-check in WaitForProcessExit
13	Concurrency & Thread Safety	✅ LGTM — _eventsDisposed = true set before _eofCountdown.Dispose() inside the same lock
14	Naming Precision	💬 NIT — ToolTaskCapturesAllOutputWithFix name is PR-relative
15	SDK Integration Boundaries	✅ LGTM
16	Idiomatic C# Patterns	✅ LGTM
17	File I/O & Path Handling	✅ N/A
18	Documentation Accuracy	✅ LGTM
19	Build Infrastructure Care	✅ LGTM
20	Scope & PR Discipline	✅ LGTM
21	Evaluation Model Integrity	✅ N/A
22	Correctness & Edge Cases	✅ LGTM — both streams always redirected; CountdownEvent already-set is safe
23	Dependency Management	✅ LGTM
24	Security Awareness	✅ LGTM — improved posture (bounded timeout vs indefinite hang)

✅ 19/24 dimensions clean.

Summary

The fix is correct and safe. The concurrency analysis (Dim 13) is particularly clean: _eventsDisposed = true is set before _eofCountdown?.Dispose() within the same lock (_eventCloseLock) acquisition, so no AsyncStreamReader thread can ever call Signal() on a disposed CountdownEvent. The _eofCountdown is { IsSet: false } pattern safely handles both the null case (wave off) and the already-signaled case.

Actionable items:

• [MAJOR] Add an STA-thread regression test — the PR's primary motivation (fixing MSB4018 via CountdownEvent vs WaitHandle.WaitAll) has zero test coverage. See design-level comment above for a suggested test.
• [MAJOR] Add a ChangeWave opt-out test — the else { proc.WaitForExit(); } legacy path is untested. See design-level comment for suggested test.
• [MODERATE] Raise MessageImportance.Low → Normal for the 30-second EOF timeout message (line 1145) so users at default verbosity can diagnose unexpected build slowdowns.
• [NIT] Add <comment> to the ToolTask.PipeEOFTimeout resx entry documenting that {0} is the timeout in seconds.
• [NIT] Use _eofCountdown is not null instead of re-calling ChangeWaves.AreFeaturesEnabled in WaitForProcessExit (line 1118).
• [NIT] Rename ToolTaskCapturesAllOutputWithFix to ToolTaskCapturesAllOutputWhenGrandchildInheritsPipeHandles.

Generated by Expert Code Review (on open) for issue #14082 · 2.9K AIC · ⊞ 30.1K ambient context