fix: route ITestOutput writes through synchronized ConcurrentStringWriter

[thomhurst]

Summary

• Root cause: ITestOutput.WriteLine/WriteError used a plain StringWriter (_outputWriter/_errorWriter) with no synchronization. Concurrent writes and reads (via GetStandardOutput() calling StringBuilder.ToString()) raced on the internal StringBuilder, causing intermittent ArgumentOutOfRangeException at StringBuilder.ToString() — the same crash reported in Flaky ArgumentOutOfRangeException in StringBuilder.ToString() during parallel test execution #5411 that resurfaced.
• Fix: Route writes through the base Context.OutputWriter/ErrorOutputWriter, which are ConcurrentStringWriter instances already protected by ReaderWriterLockSlim. Remove the now-redundant _outputWriter/_errorWriter fields.
• Cleanup: Deduplicate GetOutput()/GetOutputError() into a shared CombineOutputs helper (was ~40 lines of near-identical code).

Test plan

• TUnit.UnitTests — 177/177 pass
• Full solution build — 0 errors
• Verify with ASP.NET Core WebApplicationFactory integration tests under parallel execution (reproduces the original race)

Fixes #5556

[azure-pipelines]

Azure Pipelines:
1 pipeline(s) were filtered out due to trigger conditions.

[claude]

Code Review

Summary: This PR fixes a legitimate thread-safety bug and simplifies the output collection logic. The approach is correct and the code is clean.

---

Bug Fix (Core Issue) — Correct

The root cause was that ITestOutput.WriteLine/WriteError was writing to a lazy-init StringWriter (_outputWriter/_errorWriter) with no synchronization, while GetOutput() read from it concurrently via _outputWriter.ToString() → StringBuilder.ToString(), causing the reported ArgumentOutOfRangeException. Routing through OutputWriter/ErrorOutputWriter (backed by ConcurrentStringWriter with ReaderWriterLockSlim) is the right fix.

---

CombineOutputs — One Minor Observation

if (string.IsNullOrEmpty(runtimeOutput))
{
    return buildTimeOutput!;  // null-forgiving operator is redundant here
}

The ! on buildTimeOutput! at line 91 is unnecessary — string.IsNullOrEmpty(buildTimeOutput) already returned false at this point, so the compiler knows it's non-null. A trivial nit, no functional impact.

---

Behavior Change Worth Noting

Previously, ITestOutput.WriteLine output and console-intercepted output were collected in separate buckets (_outputWriter vs. the base OutputWriter), then concatenated in GetOutput(). Now both go into the same ConcurrentStringWriter, so they're interleaved in temporal order rather than grouped by source.

This is arguably better behavior (chronological ordering makes more sense), but it's a subtle observable change worth documenting in the PR description or a comment for anyone debugging output ordering in the future.

---

Missing Test Coverage

The test plan has an unchecked item:

[ ] Verify with ASP.NET Core WebApplicationFactory integration tests under parallel execution

This is the scenario that reproduces the original race. If an automated integration test can be added (even a simple stress test with Parallel.For calling ITestOutput.WriteLine and GetStandardOutput() concurrently), it would prevent regressions. The fix is correct, but without a test the race can silently reappear if the plumbing changes again.

---

Summary

The fix is well-targeted and the CombineOutputs refactor correctly preserves semantics for the two-source case. The removal of the redundant _outputWriter/_errorWriter fields is clean. The only meaningful ask is a regression test for the concurrent write scenario to lock in the fix long-term.

[claude]

Code Review

Summary: This PR fixes a legitimate thread-safety bug and simplifies the output collection logic. The approach is correct and the code is clean.

---

Bug Fix (Core Issue) — Correct

The root cause was that ITestOutput.WriteLine/WriteError was writing to a lazy-init StringWriter (_outputWriter/_errorWriter) with no synchronization, while GetOutput() read from it concurrently via _outputWriter.ToString() → StringBuilder.ToString(), causing the reported ArgumentOutOfRangeException. Routing through OutputWriter/ErrorOutputWriter (backed by ConcurrentStringWriter with ReaderWriterLockSlim) is the right fix.

---

CombineOutputs — One Minor Observation

The null-forgiving operator on buildTimeOutput! at the early-return path is redundant — string.IsNullOrEmpty(buildTimeOutput) already returned false by that point, proving non-null. Trivial nit, no functional impact.

---

Behavior Change Worth Noting

Previously, ITestOutput.WriteLine output and console-intercepted output were collected in separate buckets (_outputWriter vs. the base OutputWriter), then concatenated in GetOutput(). Now both go into the same ConcurrentStringWriter, so they are interleaved in temporal order rather than grouped by source. This is arguably better behavior (chronological ordering makes more sense), but it is a subtle observable change worth noting in the PR description for anyone debugging output ordering in the future.

---

Missing Test Coverage

The test plan still has an unchecked item:

[ ] Verify with ASP.NET Core WebApplicationFactory integration tests under parallel execution

This is the scenario that reproduces the original race. A regression test — even a simple stress test with Parallel.For calling ITestOutput.WriteLine and GetStandardOutput() concurrently — would lock in the fix and prevent it from silently reappearing if the plumbing changes again.

---

Overall

The fix is well-targeted. Removing the redundant unsynchronized writers and routing through the existing ConcurrentStringWriter eliminates the race cleanly. The CombineOutputs refactor correctly preserves two-source semantics with less code. The main ask is a regression test to make the fix self-enforcing.

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics -10 complexity

Metric	Results
Complexity	-10

View in Codacy

_{TIP This summary will be updated as you push new changes. Give us feedback}

[azure-pipelines]

Azure Pipelines:
1 pipeline(s) were filtered out due to trigger conditions.

[claude]

Code Review

The core fix is well-targeted and architecturally sound — routing ITestOutput.WriteLine/WriteError through the already-synchronized ConcurrentStringWriter (OutputWriter/ErrorOutputWriter) directly addresses the StringBuilder data race reported in #5556/#5411. Good root-cause analysis.

---

Issue: Cache Rebuild Outside Lock — Potential for Permanently Stale Data

Files: AssemblyHookContext.cs:44, TestSessionContext.cs:78-80

// AssemblyHookContext.cs
public IReadOnlyList<TestContext> AllTests => _cachedAllTests ??= TestClasses.SelectMany(x => x.Tests).ToArray();

// TestSessionContext.cs
public IReadOnlyList<ClassHookContext> TestClasses => _cachedTestClasses ??= Assemblies.SelectMany(x => x.TestClasses).ToArray();
public IReadOnlyList<TestContext> AllTests => _cachedAllTests ??= TestClasses.SelectMany(x => x.Tests).ToArray();

InvalidateCaches() is correctly called under the lock. However, the ??= rebuilds happen outside the lock. This creates a TOCTOU window:

1. Thread A sees _cachedAllTests == null, starts computing the array
2. Thread B calls AddClass → acquires lock → InvalidateCaches() nulls the field → releases lock → adds the new class
3. Thread A completes computation with stale data (missing B's new class) and assigns it to _cachedAllTests
4. No further invalidation is triggered → the cache is permanently stale

On x64/ARM64 the reference writes are individually atomic, so there is no torn read — but that doesn't save from the TOCTOU race. The stale value wins.

Better approach: Guard the rebuild inside the lock, or use a well-known lazy-init idiom:

// Option 1 – simplest fix: rebuild inside lock
public IReadOnlyList<TestContext> AllTests
{
    get
    {
        lock (_lock)
        {
            return _cachedAllTests ??= TestClasses.SelectMany(x => x.Tests).ToArray();
        }
    }
}

// Option 2 – lock-free with Interlocked (avoids blocking readers):
public IReadOnlyList<TestContext> AllTests
{
    get
    {
        var cached = Volatile.Read(ref _cachedAllTests);
        if (cached is not null) return cached;
        var computed = TestClasses.SelectMany(x => x.Tests).ToArray();
        return Interlocked.CompareExchange(ref _cachedAllTests, computed, null) ?? computed;
    }
}

The Volatile.Read + Interlocked.CompareExchange approach preserves the lock-free read path and guarantees correct visibility without a mutex. This matters because AllTests and TestClasses are likely read frequently during parallel execution.

---

Minor: TestCount Allocates a Full Snapshot Just to Count

File: ClassHookContext.cs

public int TestCount => Tests.Count;

Tests now returns [.. _tests] (a copy), so .Count creates a full list snapshot just to get the integer count. Instead:

public int TestCount { get { lock (_lock) return _tests.Count; } }

---

Observation: TOCTOU Between empty Check and Parent Remove

Files: AssemblyHookContext.cs:55-68, ClassHookContext.cs:81-95

bool empty;
lock (_lock)
{
    _testClasses.Remove(classContext);
    empty = _testClasses.Count == 0;
}
if (empty)               // ← window here
{
    TestSessionContext.RemoveAssembly(this);  // could be wrong by now
}

Between releasing _lock and calling RemoveAssembly, another thread could AddClass to this assembly — making it non-empty again. The parent remove would then incorrectly evict a live, non-empty assembly from the session. The lock-then-call-out-of-lock approach is correct for avoiding lock ordering issues, but the emptiness decision is no longer valid by the time it's acted on. This is a pre-existing design problem (not introduced by this PR), but now that locking is added, it would be worth addressing with a double-check pattern or by letting the parent perform the remove decision.

---

What Works Well

• The primary fix is correct: ConcurrentStringWriter (backed by ReaderWriterLockSlim) properly serializes both Write and read (GetStandardOutput) operations. Routing ITestOutput writes through it is the right fix.
• C# 13 Lock type: Better than locking on object — it's more efficient and documents intent.
• Snapshot pattern [.. _list]: Returning a copy from TestClasses/Tests under the lock prevents callers from mutating the internal list or iterating while another thread mutates. Clean.
• CombineOutputs refactor: The three-source → two-source change is correct (the third was the now-removed _outputWriter that flows into base.GetStandardOutput()). The deduplication from ~40 lines to ~15 is a net improvement. The early-return optimizations (string.IsNullOrEmpty guards) avoid ValueStringBuilder allocation for the common single-source case.
• bool empty pattern in Remove methods: Capturing the state inside the lock before acting outside is the right way to avoid holding multiple locks simultaneously.

The cache-rebuild issue is the most actionable concern — fixing it would make the threading model consistent throughout. Everything else is a minor polish.