Follow-up: Review the process-exit fix in #16191 - #16372
Draft
Jakub Jareš (nohwnd) wants to merge 1 commit into
Draft
Follow-up: Review the process-exit fix in #16191#16372Jakub Jareš (nohwnd) wants to merge 1 commit into
Jakub Jareš (nohwnd) wants to merge 1 commit into
Conversation
Wait asynchronously for redirected stderr after process exit, keep deliberate aborts on the short drain budget, and publish the deliberate-termination marker only after Kill succeeds while coordinating marker reads with that outcome. Add coverage for a natural exit winning the HasExited/Kill race. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> 🤖
Contributor
There was a problem hiding this comment.
Pull request overview
This PR follows up on the process-exit marker race fix from #16191 by coordinating deliberate process termination with stderr-drain budget selection, so naturally-exited processes aren’t misclassified as “aborted” and crash stderr has enough time to drain.
Changes:
- Replace the stderr “EOF reached” signal from
ManualResetEventSlimto an awaitableTaskCompletionSource, and switch the drain wait to an async, bounded implementation. - Track deliberately terminated processes and coordinate “Kill + marker publication” under a single lock so the exit handler chooses the correct stderr-drain timeout.
- Add/extend unit tests covering late EOF delivery, fast paths, bounded waits, timeout selection, and the “Kill loses the race” regression scenario.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| test/vstest.console.UnitTests/ProcessHelperTests.cs | Updates and adds unit tests for async stderr draining and deliberate-termination race behavior. |
| src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs | Implements async bounded stderr-drain waiting and coordinated deliberate-termination marking to select correct drain budgets. |
Suppressed comments (1)
test/vstest.console.UnitTests/ProcessHelperTests.cs:90
- The upper-bound assertion here doesn’t validate that the wait is actually bounded by the requested timeout (BudgetMs = 500). As written, a regression that waits for several seconds (but still < 5s) would pass. Tie the upper bound to BudgetMs (with a generous multiplier/slack) so the test fails if the implementation ignores the provided timeout.
Assert.IsLessThan(
5000L,
stopwatch.ElapsedMilliseconds,
$"The wait must be bounded so it cannot hang (took {stopwatch.ElapsedMilliseconds} ms).");
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implemented and committed the process-exit marker race fix for microsoft/vstest#16191.
Coordinated Kill and marker publication under one lock, publishing the marker only after Kill succeeds. Added a regression test for a natural exit winning the race.
Evidence:
🤖