Repository navigation
Avoid I/O-thread starvation in HangDump lifecycle tests - #12303
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🧪 Test quality review — PR #12303
Compared merge base 8bdfc29f415696738d845d56aca5d438d0a068de with head bc2d6e480fbb137227968d26fb9e0b9aa4d2cfda. Evidence applies to this head, not every intermediate commit.
Test-value evidence
| Contract | Scenario | Evidence | Status |
|---|---|---|---|
HangDumpInProcessProtocolFixture.ConnectAsync ordering |
The child starts and reaches "Waiting for connection for the pipe name" before the controller starts, avoiding a synchronous controller wait taking the .NET Framework I/O completion thread the child's pipe-name request needs. | ChildReverseAccepting is completed by RecordLog on the child's matching log message, so the new await has a real signal. No execution records for old/new revisions are accessible. |
Static prediction / unverified. This is a test-infrastructure deadlock/flakiness fix, so a deterministic red test is not expected, and green CI is not proof. |
The change is in a fixture helper only, so it is not a production contract and no regression test is suggested.
🧵 Parallel safety
STATUS: PARTIAL. The specialist task was not run for this review, to keep it bounded. I read the one changed helper instead. It adds no shared mutable state, static state, or scheduling change; the only change is an ordering await on a per-fixture TaskCompletionSource. No findings, but this is not a clean specialist result.
Per-test assessment
No test methods changed. The only changed lines are in the fixture helper ConnectAsync, so nothing was graded (Not applicable).
<sub>This advisory review is heuristic and non-blocking.
Re-run the combined review with /review-tests, or only the parallel-safety specialist with /parallel-audit.</sub>
🤖 Automated content by GitHub Copilot. Generated by the Test Quality Review on PR (on open / sync) workflow. · copilot · auto · ~27 AIC · ⌖ ~1 AIC · ⊞ 8.1K
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues are recorded for the fixture-only connection-ordering change.
No open issues 🎉
What changed in this PR
This PR adjusts the HangDump lifecycle test fixture’s connection ordering to avoid a .NET Framework I/O-thread starvation path, without changing production code.
Changes:
- Starts the child and awaits
ChildReverseAcceptingbefore starting the controller. - Documents why the handshake must precede the controller’s synchronous wait.
- Preserves existing assertions, timeouts, parallelization, and partial/canceled-handshake scenarios.
Test-value evidence: For commit bc2d6e480fbb137227968d26fb9e0b9aa4d2cfda, the description reports a constrained-I/O probe that stalls before the fix and completes afterward, plus 82 passing lifecycle cases across four target frameworks. The underlying local artifacts were unavailable for independent verification; fresh CI validation remains outstanding.
| File | Description |
|---|---|
test/UnitTests/Microsoft.Testing.Extensions.UnitTests/Helpers/HangDumpInProcessProtocolFixture.cs |
Orders the child handshake before controller startup and explains the starvation risk. |
Review effort: Balanced
Pull request
Summary
Address the two Windows net462 HangDump lifecycle timeouts in pipeline run 20261109.13:
HangProtocol_SupersededCompletionandHangProtocol_SynchronousDisposal. Both stopped in the fixture's initialConnectAsynchandshake.Start the child first and await the existing reverse-accept rendezvous before starting the controller. This prevents the controller's synchronous pipe-name wait from occupying a .NET Framework I/O completion thread needed to read the child's pipe-name request.
This is a one-file test-fixture change. Production code, domain assertions, timeouts, assembly parallelization, and the explicit partial/canceled-handshake scenarios are unchanged.
Design and decision record
A separate feature decision record is not needed for this fixture-only scheduling repair. Reuse
ChildReverseAcceptingrather than adding sleeps, increasing timeouts, serializing the class, or changing the shipping transport.The initial worker-only starvation probe survived: the old fixture completed with its ordinary I/O-thread budget. Constraining the isolated probe to one available worker and one I/O completion thread reproduced the blocked pipe-name handoff. The same probe completed with the fix, without releasing the constrained budget first. Both variants drained all owned tasks during cleanup.
Demonstration
Screenshots are inapplicable to this nonvisual change. The isolated net462 probe reported:
The CI logs identify the blocked handshake, but do not capture its I/O-thread state. The probe demonstrates a starvation path in that same handshake; it is not a claim that the original CI failure was reproduced under unconstrained scheduling.
Validation and regression proof
Validated commit:
bc2d6e480fbb137227968d26fb9e0b9aa4d2cfda, parent8bdfc29f415696738d845d56aca5d438d0a068de. Its committed diff exactly matches the executed patch, SHA2560A9759CC1EB5AF28BDE72DC5201519F8A3F89887EB9C0DA90D3DABFED19867C9.All builds used the repository script and pinned SDK
12.0.100-alpha.1.26556.115. Tests used Release x64, with MethodLevel scheduling and 32 workers unchanged.Both original failing data cases executed and passed. TRX, JUnit, and CTRF files were parsed and their counts reconciled. The full assembly run used a short task-owned
TEMP/TMPafter this host's shared-temp permissions and legacy path-length limits interrupted earlier attempts; no repository settings or filesystem permissions were changed.Recorded commands and results
Private absolute artifact paths are represented by
$EvidenceDir;$RepoRootis the checkout root. Other arguments below are the recorded execution arguments. Windows PowerShell and System32 were added only to the build child process's PATH.Both builds passed, exit 0, with zero warnings/errors and the normal four-target project graph.
All four commands passed, exit 0: respectively 20, 20, 21, and 21 executed cases, with zero failures/skips. The baseline net462 command used the same selection and reporting arguments with
baselinereport names andbaseline-net462as its results directory; all 20 cases passed, exit 0.With
TEMPandTMPset to the freshly created short task-owned directory:Passed, exit 0: 3399 total, 3392 executed/passed, zero failed, seven existing skips.
This command ran against the pre-fix and rebuilt fixed net462 assemblies in separate probe processes, using identical probe code and the same 10-second completion check. Old fixture: handshake incomplete before releasing the budget, exit 2. Fixed fixture: handshake complete before releasing the budget, exit 0. Both variants completed the handshake and drained every fixture/blocker task after restoring capacity.
The probe source, logs, reports, and unique binlogs are retained in local session evidence, not uploaded. No shipping package changed, so packed-product validation is inapplicable. Fresh Azure CI and native Unix runs have not been performed for this commit.
Test-value evidence
Executed: detected. The isolated constrained-I/O probe stalls at the original fixture's pipe-name handoff and succeeds with the committed ordering change. No assertions were changed to manufacture red, and the ordinary-budget control passed on the old fixture.
Executed: preserved. The existing lifecycle suite is green before/after on net462, and all 82 fixed lifecycle cases are green across the four TFMs.
HangProtocol_SupersededCompletionandHangProtocol_SynchronousDisposalremain present and pass. This is a fixture repair, not new production behavior or a newly added regression test; unconstrained recurrence of the original CI flake remains unverified.