perf(engine): skip Console.Out/Err FlushAsync when no output captured (#5712)

[thomhurst]

Closes #5712

Summary

• Adds a volatile HasCapturedConsoleOutput flag on Context, set by the console interceptor on any write path.
• TestCoordinator checks the flag before awaiting Console.Out.FlushAsync() / Console.Error.FlushAsync() in the per-test finally block, skipping both calls when nothing was written.
• Removes 2 async state machines per passing test with no output; CPU-sampling trace showed AsyncMethodBuilderCore.Start at 16.39% inclusive / 2.93% self — expected ~1.5-2% CPU reduction.

Correctness

• The flag is monotonic (false -> true) and set before the buffer append, so any write that lands triggers the flush.
• The fire-and-forget race (Task.Run(() => Console.WriteLine(...)) without awaiting) already exists under the old unconditional flush — the flag preserves identical semantics: when the Task.Run actually writes it flips the flag and is observed by the next test's finally or the test session cleanup path.
• Partial writes (Console.Write without newline) are covered — they hit GetLineBufferForWrite which marks the flag before appending to the buffer.
• Volatile read/write (no lock, no allocation) on the hot path.

Test plan

• dotnet build TUnit.slnx -c Release succeeds (0 errors, pre-existing warnings only).
• BasicTests smoke — 3/3 passed.
• CaptureOutputTests (Console.WriteLine + GetStandardOutput assertions, [Repeat(15)]) — 80/80 passed.
• TestBuildContextOutputCaptureTests — 7/7 passed.
• ParallelConsoleOutputTests — 44/44 passed.
• OutputTruncationTests — 3/3 passed.

Risks

• If a test writes only during an async continuation that resumes after the coordinator's finally runs, output is lost — but this is already the case under the old code (FlushAsync is still a point-in-time call), so no regression.
• The flag lives on Context (base class), not TestContext; writes from non-test contexts (hooks) also set it, which is harmless since the flag is per-context.

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 complexity

Metric	Results
Complexity	0

View in Codacy

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

[claude]

Code Review: perf(engine): skip Console.Out/Err FlushAsync when no output captured

Overall Assessment: Approve with minor observations. The optimization is mechanically sound, the new flag is correctly placed and correctly read, and the PR does not regress any existing behaviour. The observations below are nits and clarifying notes rather than blockers.

---

Correctness

The new flag is genuinely necessary (not a simplification of an existing one).
HasCapturedOutput (existing) checks whether _outputWriter or _errorOutputWriter was ever initialized. Those lazy fields are only initialized when RouteToSinks pushes a complete line through to the TestOutputSink. A partial write — Console.Write("partial") without a subsequent newline — lands in the ConsoleLineBuffer but never reaches OutputWriter, so HasCapturedOutput remains false while the buffer holds unflushed data. Using the existing flag for the flush guard would silently drop partial writes. The new HasCapturedConsoleOutput flag being set before the buffer append (in GetLineBufferForWrite()) is the correct place.

All write paths are covered.
Tracing the delegation chain:

• Write(bool/int/float/…) → Write(value.ToString()) → GetLineBufferForWrite() ✓
• Write(char value), Write(char[], …), Write(string?) → GetLineBufferForWrite() directly ✓
• WriteLine() → GetLineBufferForWrite().Drain() ✓
• WriteLine(string?), WriteLineAsync(string?) → GetLineBufferForWrite() ✓
• All WriteAsync(…) overrides delegate to their sync Write(…) counterparts which go through GetLineBufferForWrite() ✓
• ReadOnlySpan<char> / ReadOnlyMemory<char> overrides convert to string then call the string overload ✓

The Flush() and FlushAsync() overrides intentionally use GetLineBuffer() — not GetLineBufferForWrite(). This is correct: flushing existing buffered content is not a new write, so the flag should not be set there.

Monotonic flag is safe under retries.
RetryHelper does not reset the flag between attempts. This is intentional and correct: the flag is a signal that a flush is needed at some point, not a per-attempt tracker. A partial write from attempt 1 that sits in the buffer will still be flushed when the test-level finally eventually runs.

---

Observations

1. int vs bool for the volatile flag

private int _consoleOutputCaptured;

Volatile.Read(ref bool) and Volatile.Write(ref bool, bool) are available in .NET and work correctly. Using bool would make the intent more self-documenting and remove the != 0 comparison on the read path. The int choice is not wrong — it enables Interlocked.CompareExchange if needed later — but there is no current use case for that. If CAS is not anticipated, bool reads more clearly.

2. Guard GetLineBuffer() against future misuse

The naming convention (GetLineBuffer vs GetLineBufferForWrite) has to be learned. If a contributor adds a new Write… override and accidentally uses GetLineBuffer() instead of GetLineBufferForWrite(), the flag will silently not be set for that path and the optimization may drop output. A short comment on GetLineBuffer() would prevent this:

// Use GetLineBufferForWrite() on all write paths to ensure HasCapturedConsoleOutput is set.
// Use this method only for Flush/Close paths.
protected abstract ConsoleLineBuffer GetLineBuffer();

3. Missing automated test for the skip-flush path

The test plan in the PR description is entirely manual. There is no unit test asserting HasCapturedConsoleOutput is false after a no-output test and true after any write. Given that the correctness of the entire optimization rests on this flag, a small unit test would make future regressions immediately visible. Suggested scenarios:

• Context with no writes: HasCapturedConsoleOutput == false
• Context after Console.Write("x") (partial, no newline): HasCapturedConsoleOutput == true
• Context after Console.WriteLine() (empty line): HasCapturedConsoleOutput == true

---

Summary

The change is architecturally clean, the flag placement is correct, and the performance claim (skipping 2 async state machines per passing test with no output) is well-supported. The only items worth considering before merging:

1. (Optional) Switch private int _consoleOutputCaptured to private volatile bool for clarity.
2. (Recommended) Add a comment to GetLineBuffer() warning future contributors to use GetLineBufferForWrite() on write paths.
3. (Recommended) Add at least one automated unit test asserting flag state under no-write, partial-write, and full-write scenarios.

🤖 Generated with Claude Code

[thomhurst]

Addressed the three review suggestions:

1. Replaced int _consoleOutputCaptured + Volatile.Read/Write with private volatile bool _consoleOutputCaptured for clarity (no CAS was needed).
2. Added a WHY comment on GetLineBuffer() noting it's for Flush/Close only — write paths must use GetLineBufferForWrite() so draining the buffer doesn't spuriously set HasCapturedConsoleOutput.
3. Added ConsoleOutputCaptureFlagTests in TUnit.UnitTests covering the three requested scenarios (fresh context false; after Write("x") true; after empty WriteLine() true) plus the direct MarkConsoleOutputCaptured path. All 4 tests pass on net10.0.