fix: prevent StringBuilder race in console interceptor during parallel tests

[JSkimming]

Fixes #5411

Problem

OptimizedConsoleInterceptor.Write(StringBuilder) calls .ToString() on the caller's StringBuilder. ASP.NET Core's ConsoleLogger pools its StringBuilder and may return it to the pool immediately after Write returns. Under parallel test execution with code coverage, another thread can pick up the pooled StringBuilder and mutate it while .ToString() is still traversing the internal chunk list, causing:

ArgumentOutOfRangeException: Index was out of range. (Parameter 'chunkLength')
    at System.Text.StringBuilder.ToString()

Fix

OptimizedConsoleInterceptor.cs: Replace value?.ToString() with CopyStringBuilderSafely(value) for all StringBuilder-accepting overloads (Write, WriteAsync, WriteLine, WriteLineAsync). The helper copies via string.Create + StringBuilder.CopyTo and catches ArgumentOutOfRangeException if the race hits — losing one log entry rather than crashing the test.

Context.cs: Move the _outputBuilder.Length early-return check inside the ReaderWriterLockSlim in GetStandardOutput() and GetErrorOutput(). StringBuilder.Length is not thread-safe and was being read outside the lock.

Test plan

• Existing ContextThreadSafetyTests covers concurrent read/write on the output builder
• The flaky failure is inherently difficult to reproduce locally — it requires high concurrency + code coverage instrumentation on Linux CI

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 5 complexity

Metric	Results
Complexity	5

View in Codacy

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

[JSkimming]

OK, so this is the real me, not Claude on my behalf. I'm somewhat sceptical about ASP.NET mutating the string builder before the call completes, but with the stack tracing being so sparse, there's not much to go on. The change seems at least neutral. The locking changes are certainly worthwhile, as Length shouldn't be read outside of a lock, though it isn't a mutating operation and shouldn't cause the issue.

Happy to take feedback and make adjustments.

[claude]

Code Review

Good defensive fix for a real crash. The two changes have different confidence levels — the fix is clearly correct, while the fix deserves a closer look.

---

Context.cs — Clean and correct

Moving the early-return inside the read lock is the right call. Reading outside the lock was a genuine data race (the backing field is not volatile). This is a straightforward, unambiguous improvement.

---

OptimizedConsoleInterceptor.cs — Concerns

1. CopyTo doesn't actually improve thread-safety over ToString()

Both and traverse the same internal chunk list. If the StringBuilder is mutated concurrently, both throw for the same reasons. The real safety net here is the — the use of + is a more complex spelling of the same vulnerability.

A simpler implementation that is functionally equivalent:

If there's a measurable allocation or perf reason to prefer , it should be documented. As written, it adds complexity without safety benefit.

2. Silent log loss is a debugging hazard

When the race fires, the log entry is silently discarded with no trace. In a CI environment debugging flaky tests, this makes the problem harder to diagnose — you lose the very output that would help. Consider at least incrementing a counter or writing a trace-level warning to stderr so there's some signal that truncation occurred:

3. Is the root cause actually what we think it is?

The PR description attributes the crash to ASP.NET Core returning the pooled to the pool before returns. But the overload is synchronous — and the author's own comment acknowledges scepticism about this explanation.

Two alternative causes worth considering:

• Code coverage instrumentation can introduce GC pressure and unexpected reordering that exposes pre-existing unsafe reads (like the length-outside-lock bug fixed in this PR).
• **A bug in ASP.NET Core's ** returning the StringBuilder before the Write completes — if so, it's worth filing an upstream issue in addition to this defensive fix.

The fix alone might resolve the original crash if the actual race was on the output builder rather than the ASP.NET Core pooled one.

---

Suggestion

If the approach is kept, add a comment explaining why it's preferred over (e.g. a measured perf difference). Otherwise, simplify to + catch — the intent is clearer and the behaviour is identical.

---

Summary

The locking fix in is solid and should definitely be merged. The approach in is a pragmatic safety net for an inherently racy situation with externally-owned memory, but the + implementation doesn't improve on +catch in either safety or clarity. The silent log-loss behaviour is the main concern for long-term maintainability.

[claude]

Code Review

Good defensive fix for a real crash. The two changes have different confidence levels — the Context.cs fix is clearly correct, while the OptimizedConsoleInterceptor.cs fix deserves a closer look.

---

Context.cs — Clean and correct

Moving the early-return length check inside the read lock is the right call. Reading StringBuilder.Length outside the lock was a genuine data race. This is a straightforward, unambiguous improvement.

---

OptimizedConsoleInterceptor.cs — Concerns

1. CopyTo does not actually improve thread-safety over ToString()

Both StringBuilder.ToString() and StringBuilder.CopyTo() traverse the same internal chunk list. If the StringBuilder is mutated concurrently, both throw ArgumentOutOfRangeException for the same reasons. The real safety net here is the try/catch — the string.Create + CopyTo approach is a more complex spelling of the same vulnerability.

A simpler implementation that is functionally equivalent:

private static string CopyStringBuilderSafely(StringBuilder? value)
{
    if (value is null) return string.Empty;
    try
    {
        return value.ToString();
    }
    catch (ArgumentOutOfRangeException)
    {
        return string.Empty;
    }
}

If there is a measurable allocation or perf reason to prefer CopyTo, it should be documented. As written, it adds complexity without safety benefit.

2. Silent log loss is a debugging hazard

When the race fires, the log entry is silently discarded with no trace. In a CI environment debugging flaky tests, this makes the problem harder to diagnose — you lose the very output that would have helped. Consider at minimum a Trace.WriteLine or a counter so there is some signal that truncation occurred.

3. Is the root cause actually what we think it is?

The PR description attributes the crash to ASP.NET Core returning the pooled StringBuilder to the pool before Write(StringBuilder) returns. But Write(StringBuilder) is synchronous — and the author's own comment acknowledges scepticism about this.

Two alternative causes worth considering:

• Code coverage instrumentation can introduce GC pressure and unexpected reordering that exposes pre-existing unsafe reads (like the Context.cs length-outside-lock bug fixed in this PR).
• If it is an ASP.NET Core ConsoleLogger bug (returning the StringBuilder before Write completes), it may be worth filing an upstream issue in addition to this defensive workaround.

The Context.cs fix alone might resolve the original crash if the actual race was on the Context output builder rather than the ASP.NET Core pooled one.

---

Suggestion

If the CopyTo approach is kept over ToString(), add a comment explaining the performance rationale. Otherwise, simplify to ToString() + catch — the intent is clearer and the behaviour is identical.

---

Summary

The locking fix in Context.cs is solid and should definitely be merged. The CopyStringBuilderSafely approach in OptimizedConsoleInterceptor.cs is a pragmatic safety net, but the string.Create + CopyTo implementation does not improve on ToString() + catch in either safety or clarity. The silent log-loss behaviour is the main concern for long-term maintainability.

[thomhurst]

If you can reproduce it again, run with --detailed-stacktrace to hopefully give you more info

[JSkimming]

Superseded by #5414 — the owner took a cleaner approach: removing the StringBuilder overrides entirely rather than catching the exception. The base TextWriter handles StringBuilder chunk-by-chunk via GetChunks(), avoiding the race without any ToString() call.