Harden TerminalTestReporter teardown and remove dead _buildErrorsCount#8426
Harden TerminalTestReporter teardown and remove dead _buildErrorsCount#8426Evangelink wants to merge 1 commit into
Conversation
Three small, independently-justified changes to the MTP terminal: 1. `TerminalTestReporter.Dispose` now also restores the Windows console mode (`ENABLE_VIRTUAL_TERMINAL_PROCESSING`). Previously only the normal `TestExecutionCompleted` path restored it, so an abnormal teardown (unhandled exception, `Dispose` without a completed run) left the user's shell with the mode we silently enabled. The saved value is now consumed in `TestExecutionCompleted` so the additional `Dispose` restore is a no-op when the normal lifecycle runs. 2. `TestProgressStateAwareTerminal` is now idempotently disposable and its render-thread proc no longer swallows only `ObjectDisposedException`. A broken pipe / `IOException` from the underlying console used to kill the refresher silently and leave the spinner permanently gone with no diagnostic. We now catch `Exception`, still attempt `EraseProgress` on the way out, and make `StopShowingProgress` + `Dispose` safe to call repeatedly (`Dispose` always tears the refresher down, even when `StopShowingProgress` was never reached). 3. Drop the dead `_buildErrorsCount` field. It was declared, reset to 0 in `TestExecutionCompleted` and read by the summary colorization, but nothing ever assigned it a non-zero value, so the `_buildErrorsCount == 0` conjuncts in `colorizePassed` / `colorizeSkipped` were always true. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens the terminal output device teardown paths in Microsoft.Testing.Platform so abnormal shutdowns (exceptions, incomplete lifecycles, hot reload interruptions) leave the user’s terminal in a sane state, and removes a dead summary field.
Changes:
- Make
TestProgressStateAwareTerminalteardown more resilient (broader catch in refresher thread, best-effort cleanup, idempotent Dispose/stop). - Restore Windows console mode from
TerminalTestReporter.Dispose, and “consume” the saved original mode after the normal completion path. - Remove the unused
_buildErrorsCountand simplify summary colorization logic.
Show a summary per file
| File | Description |
|---|---|
| src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TestProgressStateAwareTerminal.cs | Adds idempotent shutdown and best-effort cleanup for the progress refresher and busy indicator. |
| src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.Summary.cs | Removes _buildErrorsCount from summary colorization conditions. |
| src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.Lifecycle.cs | Restores console mode at completion and clears saved mode afterward. |
| src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.cs | Restores console mode in Dispose to cover abnormal teardown paths. |
Copilot's findings
- Files reviewed: 4/4 changed files
- Comments generated: 2
| public void StartShowingProgress(int workerCount) | ||
| { | ||
| if (GetShowProgress()) | ||
| { | ||
| _progressItems = new TestProgressState[workerCount]; | ||
| _terminal.StartBusyIndicator(); | ||
| // If we crash unexpectedly without completing this thread we don't want it to keep the process running. | ||
| _refresher = new Thread(ThreadProc) { IsBackground = true }; | ||
| _refresher.Start(); | ||
| } | ||
| } | ||
|
|
||
| internal void StopShowingProgress() | ||
| { | ||
| if (GetShowProgress()) | ||
| if (_stopped || !GetShowProgress()) | ||
| { | ||
| _cts.Cancel(); | ||
| _refresher?.Join(); | ||
| return; | ||
| } | ||
|
|
||
| _stopped = true; | ||
| _cts.Cancel(); | ||
| _refresher?.Join(); |
| _terminalWithProgress.StopShowingProgress(); | ||
|
|
||
| _terminalWithProgress.WriteToTerminal(_isDiscovery ? AppendTestDiscoverySummary : AppendTestRunSummary); | ||
|
|
||
| NativeMethods.RestoreConsoleMode(_originalConsoleMode); | ||
| // Consume the saved console mode so a later Dispose does not try to restore again. | ||
| _originalConsoleMode = null; | ||
|
|
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| catch | ||
| { | ||
| // Best-effort cleanup; we are already in teardown. | ||
| } |
| catch | ||
| { | ||
| // Best-effort cleanup; we are already in teardown. | ||
| } |
Evangelink
left a comment
Evangelink
left a comment
There was a problem hiding this comment.
Review — PR #8426: Harden TerminalTestReporter teardown
All three stated goals (console-mode restore in Dispose, idempotent StopShowingProgress/Dispose, removal of dead _buildErrorsCount) are sound and the overall direction is correct. Three findings below, one of which is a genuine correctness concern under concurrent teardown.
| # | Dimension | Finding | Severity |
|---|---|---|---|
| 1 | Threading & Concurrency | _stopped/_disposed are plain bool with no memory-visibility guarantee — if torn down from a thread other than the calling thread (e.g. finalizer), the idempotency guard can be bypassed. |
MINOR |
| 2 | Algorithmic Correctness | EraseProgress() is called twice on the normal shutdown path: once by ThreadProc (always, after the loop) and again by StopShowingProgress after Join(). Harmless today but logically inconsistent. |
MINOR |
| 3 | Code Clarity | The catch (Exception) comment in ThreadProc describes intent inaccurately — the cleanup it refers to happens after the catch block, not inside it. |
TRIVIAL |
| 4–21 | All other dimensions | N/A or clean for this change. |
Summary: The hardening changes are correct and desirable. The two minor issues are worth a quick follow-up pass but should not block merge.
Generated by Expert Code Review (on open) for issue #8426 · ● 5.5M
| } | ||
|
|
||
| _terminal.EraseProgress(); | ||
| try |
There was a problem hiding this comment.
Misleading comment (minor clarity)
The catch (Exception) block comment says the refresher must not "silently tear down ... without making a best-effort attempt to clean up the screen", but this catch block itself does nothing. The actual cleanup attempt is the unconditional try { _terminal.EraseProgress(); } that follows. The comment should describe what the block does ("swallow so the unconditional cleanup below can still run") rather than describing what it avoids:
catch (Exception)
{
// Swallow so that the unconditional EraseProgress() below still runs.
// There is no other thread to surface this to; the test run itself should
// still be allowed to complete.
}| // Best-effort cleanup; we are already in teardown. | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Double EraseProgress() on normal shutdown path
ThreadProc now always calls _terminal.EraseProgress() before it exits (lines 87–91). StopShowingProgress then calls _refresher?.Join() — which blocks until ThreadProc has already run EraseProgress() — and immediately calls _terminal.EraseProgress() a second time. The intent seems to be handling the case where the thread exits abnormally before reaching the cleanup, but because the catch (Exception) block does not re-throw and the cleanup is unconditional, EraseProgress is always called twice on this path.
Calling EraseProgress twice is harmless in practice (erasing an already-erased progress line is a no-op), but it signals a logical gap. Consider skipping the redundant call in StopShowingProgress if the refresher thread was joined cleanly, or removing the EraseProgress from ThreadProc and keeping it only in StopShowingProgress/Dispose.
_cts.Cancel();
_refresher?.Join(); // ThreadProc has already erased progress at this point
try
{
// This EraseProgress is now redundant on the normal path.
_terminal.EraseProgress();
_terminal.StopBusyIndicator();
}| @@ -33,6 +33,8 @@ | |||
| /// </summary> | |||
| private Thread? _refresher; | |||
There was a problem hiding this comment.
_stopped / _disposed not volatile (threading)
Both fields are plain bool. _stopped is written in StopShowingProgress and read in both StopShowingProgress and (indirectly) Dispose. _disposed is written and read in Dispose. While lifecycle callers are expected to be sequential in practice, the refresher thread is a background thread, and there is no memory barrier or volatile annotation to guarantee that a write on the calling thread is visible across threads.
For the idempotency guards to be reliable under any calling pattern (e.g. Dispose called from a finalizer or a different thread during process teardown), consider marking these volatile or using Interlocked.CompareExchange:
private volatile bool _stopped;
private volatile bool _disposed;
What
Three small, independently-justified hardening changes to the MTP terminal output device, drawn from a recent terminal review.
1. Restore the Windows console mode in
TerminalTestReporter.DisposeWhen the terminal is set up,
NativeMethods.QueryIsScreenAndTryEnableAnsiColorCodesmay enableENABLE_VIRTUAL_TERMINAL_PROCESSINGand save the original mode in_originalConsoleMode. Today only the normalTestExecutionCompletedpath restores it — so on an abnormal teardown (unhandled exception,Disposecalled without a completed run, hot-reload session that never reachedTestExecutionCompleted), the user's shell is left with whatever mode we enabled.Disposenow also callsRestoreConsoleMode, andTestExecutionCompletednulls out_originalConsoleModeafter restoring so the second call is a no-op when the normal lifecycle runs.2. Make
TestProgressStateAwareTerminalresilient on shutdownThreadProcused to catch onlyObjectDisposedException. A broken pipe /IOExceptionfrom the underlying console would silently kill the refresher thread, leaving subsequent writes flowing throughWriteToTerminal(still under the lock) but with no spinner and no diagnostic. We now catchExceptionand still attempt oneEraseProgresson the way out.StopShowingProgressis now idempotent and resilient toEraseProgress/StopBusyIndicatorthrowing (it's a best-effort cleanup at that point).Disposeis now idempotent and always tears the refresher down, even whenStopShowingProgresswas never reached, so the cursor (HideCursoris emitted at start) and busy indicator are always restored.3. Remove dead
_buildErrorsCountfieldThe field was declared, reset to
0inTestExecutionCompleted, and read by the summary colorization (colorizePassed/colorizeSkipped) — but nothing ever assigned it a non-zero value. The_buildErrorsCount == 0conjuncts were always true; dropping them does not change the rendered summary in any reachable state.Why now
These came out of a structured review of the terminal code. We started from a larger list of suspected concurrency issues, validated each one against the actual threading model (the platform's per-consumer
SingleReader=truechannel serializesConsumeAsync, and the renderer ticks at 500ms), and confirmed that the only items that translated to user-observable defects were the abnormal-teardown cursor/console-mode leak and the over-narrow render-threadcatch. The dead_buildErrorsCountis pure cleanup that came up while validating the summary path.Validation
dotnet build src\Platform\Microsoft.Testing.Platform\Microsoft.Testing.Platform.csproj— succeeded on all TFMs (netstandard2.0, net8.0, net9.0) with 0 warnings.Microsoft.Testing.Platform.UnitTests— all 820 tests pass on both net8.0 and net9.0 (2 unrelated cancellation tests are skipped, matching baseline).Not in scope
The review also surfaced UX/feature ideas (
--reporter=llmmode,--max-stdout-lines, source-context windows around failure frames, hint extension points, etc.) and refactoring opportunities (duplication betweenHumanReadableDurationFormatter.RenderandAppend, theobject-typed list inAnsiTerminalTestProgressFrame). Those are intentionally left out of this PR so it stays narrowly scoped to the validated correctness issues.