fix: cap test output at 1M chars to prevent OOM

[thomhurst]

Summary

Closes #5560

• Cap per-test output at 1M characters (~2MB) in ConcurrentStringWriter to prevent OutOfMemoryException when tests produce excessive output
• Keep most recent output (trim oldest) — most useful for debugging
• Trim to 75% on overflow to avoid re-trimming on every subsequent write
• Prepend [... output truncated ...] notice when reading back truncated output

Root cause

ConcurrentStringWriter backed by StringBuilder had no size limit. When tests produced large output, ValueStringBuilder.Grow() in the reporting path (ToTestNode → GetStandardOutput) tried to allocate arrays too large for ArrayPool, causing OOM.

Design

• Where: Capping at write time in ConcurrentStringWriter.TrimIfNeeded() — all 14 Write/WriteLine overrides call it after each append, under the existing write lock
• How much: 1M chars max, trim to 768K (75%) on overflow — single int comparison per write, StringBuilder.Remove(0, N) amortized by 25% headroom
• Read path: ConcurrentStringWriter.GetContent() owns lock acquisition, content retrieval, and truncation notice — Context.GetStandardOutput()/GetErrorOutput() are now one-liners

Test plan

• ContextThreadSafetyTests — concurrent read/write safety
• CaptureOutputTests — 80 output capture scenarios
• ParallelConsoleOutputTests — 44 parallel console tests
• ConsoleTests — console interception
• TestBuildContextOutputCaptureTests — build-time output
• TUnit.Core and TUnit.Engine build with 0 warnings

[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

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

[claude]

Code Review

Good fix for the OOM issue (#5560). The approach is sound and the implementation is clean. A few observations:

Thread Safety: ??= on the Writer Fields (Minor)

// Lines 51-53
public TextWriter OutputWriter => _outputWriter ??= new ConcurrentStringWriter(GetOutputBuilder(), GetOutputLock());
public TextWriter ErrorOutputWriter => _errorOutputWriter ??= new ConcurrentStringWriter(GetErrorOutputBuilder(), GetErrorOutputLock());

??= is not atomic — two threads racing on first access could each construct a ConcurrentStringWriter. The "losing" writer is discarded, but if it had already set its own _truncated = true before being dropped, that state is silently lost. In practice this is a very narrow race, and GetOutputBuilder() / GetOutputLock() are both safe via LazyInitializer.EnsureInitialized, so both writers would share the same StringBuilder + lock.

This was already the pre-existing behaviour under the [field: AllowNull, MaybeNull] semi-auto property (which generates identical ??= code), so it's not a regression. But if full correctness is desired, LazyInitializer.EnsureInitialized (already used everywhere else in Context) is the consistent pattern:

private ConcurrentStringWriter? _outputWriter;
private ConcurrentStringWriter GetOutputWriter() =>
    LazyInitializer.EnsureInitialized(ref _outputWriter,
        () => new ConcurrentStringWriter(GetOutputBuilder(), GetOutputLock()))!;

public TextWriter OutputWriter => GetOutputWriter();
public virtual string GetStandardOutput() => _outputWriter?.GetContent() ?? string.Empty;

Design: TrimIfNeeded() Called on Every Write

Currently TrimIfNeeded() runs a _builder.Length > MaxOutputLength comparison after every single write, including Write(char value) which is called character-by-character. The comparison itself is O(1) and cheap, but consider that StringBuilder.Length is a property getter.

An optional micro-optimisation once truncation has never occurred is to skip the check entirely:

private void TrimIfNeeded()
{
    // Fast path: once we've never exceeded the limit and we're well under,
    // most writes can skip the branch entirely.
    if (_builder.Length <= MaxOutputLength)
        return;

    _builder.Remove(0, _builder.Length - TrimTarget);
    _truncated = true;
}

This is effectively what the code already does (the if condition is the same), so this is just a style note — the logic is already correct.

Design: "Keep most recent" is the right call

The choice to trim the oldest output and keep the most recent is a good fit for test debugging — failures tend to be logged near the end. The 75% trim target (amortises the O(N) StringBuilder.Remove call across ~256K chars before re-triggering) is a sensible tradeoff.

Encapsulation Improvement is Well Done

Moving GetContent() into ConcurrentStringWriter — centralising the lock acquisition, length check, and truncation-notice prepend — is a clean encapsulation win. The simplification of GetStandardOutput() / GetErrorOutput() to one-liners is a direct payoff. The _truncated flag is correctly written under the write lock and read under the read lock; ReaderWriterLockSlim provides the necessary acquire/release barriers.

Async Writes

No WriteAsync overrides needed — TextWriter's default async implementations delegate to the sync overrides, so TrimIfNeeded() is covered on the async path too.

---

Overall: the approach correctly addresses the root cause, the locking discipline is sound, and the refactor is a net improvement. The thread-safety note on ??= is worth considering for full correctness parity with the rest of Context's lazy-init pattern, but is not blocking.

[azure-pipelines]

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

[azure-pipelines]

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

[claude]

Code Review (Follow-up)

The previous review comment about using LazyInitializer.EnsureInitialized for the writer fields has been addressed in the second commit — the pattern is now consistent with the rest of Context. That concern is resolved.

The overall approach remains sound. Two new findings on the current state of the code:

---

1. Surrogate Pair Splitting in TrimIfNeeded()

TrimIfNeeded() does a raw StringBuilder.Remove(0, _builder.Length - TrimTarget). The first retained character is at index _builder.Length - TrimTarget. If that character happens to be a low surrogate whose paired high surrogate was just removed, the builder is left with an unpaired low surrogate — invalid UTF-16.

HtmlReporter.TruncateOutput() already handles this correctly (lines 535–540):

var cutAt = MaxOutputLength;
if (char.IsHighSurrogate(value[cutAt - 1]))
    cutAt--;

The same guard is needed in TrimIfNeeded():

private void TrimIfNeeded()
{
    if (_builder.Length <= MaxOutputLength)
        return;

    var removeCount = _builder.Length - TrimTarget;
    // Avoid splitting a surrogate pair at the boundary
    if (removeCount > 0 && char.IsHighSurrogate(_builder[removeCount - 1]))
        removeCount--;

    _builder.Remove(0, removeCount);
    _truncated = true;
}

In practice, output that triggers OOM is usually plain ASCII logs, so this is unlikely to cause real-world failures — but correctness is correctness, especially given that the HtmlReporter already treats this as worth guarding.

---

2. Double-Truncation Edge Case (Minor)

The ConcurrentStringWriter trims the builder to TrimTarget (786 432 chars) on overflow. GetContent() then prepends the TruncationNotice (~92 chars), giving ~786 524 chars total — well under both limits.

However, after a trim, subsequent writes can accumulate output again up to MaxOutputLength (1 048 576 chars) before the next trim fires. At read time GetContent() would then return 1 048 576 + 92 = 1 048 668 chars. This is just barely over HtmlReporter.MaxOutputLength (1 048 576), so HtmlReporter.TruncateOutput() fires again — truncating the first 92 characters of the string, which is exactly the TruncationNotice itself.

The effect is:

• The user sees the tail of the actual output (correct)
• The [... output truncated ...] notice is cut (replaced by HtmlReporter's own notice at the end) — probably acceptable, but slightly confusing

The simplest fix, if desired, is to either:

• Reduce ConcurrentStringWriter.MaxOutputLength by len(TruncationNotice) so the worst-case output from GetContent() stays at or below 1 048 576, or
• Accept the current behaviour since both notices refer to truncation and it is not harmful

This is a very low-priority cosmetic issue, not a correctness bug.

---

Summary

Finding	Severity	Addressed by Second Commit?
??= race on writer fields (previous review)	Low	Yes — LazyInitializer now used consistently
Surrogate split in TrimIfNeeded()	Low-Medium	No — new finding
Double-truncation edge case	Very Low (cosmetic)	No — new finding

The core fix (capping output at write time to prevent OOM) is correct and the refactoring is a net improvement. The surrogate splitting issue is worth a one-liner guard for correctness parity with HtmlReporter.

[azure-pipelines]

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

[claude]

Code Review

This is a clean, well-scoped fix for a real OOM issue. The design is sound — capping at write time with amortized trimming is the right place to enforce the limit. A few observations:

---

Correctness — Surrogate Pair Boundary

The surrogate-pair guard is correct for the case it targets: if the last-removed char is a high surrogate, decrement removeCount to keep the full pair. Two edge cases worth noting:

1. What if removeCount - 1 == 0 after the decrement? removeCount > 0 prevents underflow, but if removeCount becomes 0 the builder won't be trimmed at all, which is safe — it will trim on the next write instead.

2. What if _builder[removeCount - 1] is a low surrogate (its high surrogate was already in the removed range)? In valid UTF-16 that can't happen independently, so the current check is sufficient. A more defensive approach — though arguably over-engineering for test output — would be:

   while (removeCount > 0 && char.IsLowSurrogate(_builder[removeCount]))
       removeCount--;

   (Check the first kept char rather than the last removed one — semantically clearer.)

---

Performance of StringBuilder.Remove(0, N)

Remove(0, N) with N ≈ 256 K chars causes a full left-shift of the remaining 768 K chars every time the buffer saturates. This is O(remaining length) per trim. For a test dumping continuous output past 1 MB, that's a 768 K memcopy every ~256 K writes.

The 25% headroom amortizes this well for typical tests, but for truly pathological output generators the trim could dominate. If this ever becomes a hot path, a ring buffer (fixed char[] with head/tail indices) or just tracking an _startOffset int and slicing on GetContent() would reduce trim cost to O(1). For now the Remove approach is fine — just worth a // O(n) shift; amortized by 25% headroom comment to orient future readers.

---

GetContent() Holds Read Lock While Allocating

_lock.EnterReadLock();
// ...
var content = _builder.ToString();   // allocates up to 2 MB under the lock
return _truncated ? string.Concat(TruncationNotice, content) : content;

For a 1 MB buffer, ToString() + optional Concat allocates ~2–4 MB while blocking all writers. This is called at the end of a test run (cold path), so it's not a concern in practice — but it's worth knowing if this pattern is ever reused in a hotter context.

---

Architecture — Consolidation of GetStandardOutput()/GetErrorOutput()

Moving the lock acquisition and truncation-notice logic into GetContent() is the right call. The old pattern (duplicated read-lock try/finally in Context) was error-prone; now there's a single, tested path. The _outputWriter?.GetContent() ?? string.Empty one-liners are a clear improvement.

---

LazyInitializer Refactor

Using LazyInitializer.EnsureInitialized is consistent with the rest of the class and correctly thread-safe (CAS semantics). The trade-off vs. the old field ??= is that the factory may run more than once under heavy contention, but since the losing instances are immediately discarded and share the same underlying StringBuilder/lock via the lazy-init chain, there's no correctness issue.

---

Minor

• private const int TrimTarget = MaxOutputLength * 3 / 4; — worth a // 786,432 chars comment (or just leave the arithmetic visible as-is; it's self-documenting).
• The TruncationNotice message correctly says "showing most recent output" which accurately describes the keep-tail strategy. Good UX.

---

Overall: Solid fix. The design decisions (cap at write time, keep tail, amortized trim, truncation notice on read) are all well-reasoned and the implementation is correct. The main architectural alternative worth knowing about (ring buffer) is premature for this use case. Ready to merge.

[claude]

Code Review

This is a clean, well-scoped fix for a real OOM issue. The design is sound — capping at write time with amortized trimming is the right place to enforce the limit.

Correctness — Surrogate Pair Boundary

The surrogate-pair guard is correct for the targeted case: if the last-removed char is a high surrogate, decrement removeCount to keep the full pair. A more semantically clear alternative would check the first kept char instead:

// More readable: ensure the first kept char is not a lone low surrogate
while (removeCount > 0 && char.IsLowSurrogate(_builder[removeCount]))
    removeCount--;

The current check is logically equivalent for valid UTF-16, but checking the kept boundary is easier to reason about.

Performance of StringBuilder.Remove(0, N)

Remove(0, N) with N ≈ 256K chars causes a full left-shift of the remaining 768K chars — O(remaining length) per trim. The 25% headroom amortizes this well for typical tests. If this ever becomes a hot path (a test continuously flooding output past 1MB), the alternative is tracking a _startOffset int and slicing on GetContent() — O(1) trim. For now the Remove approach is completely justified; just worth a comment like // O(n) shift; amortized by 25% headroom for future readers.

GetContent() Holds Read Lock While Allocating

StringBuilder.ToString() for a 1MB buffer allocates ~2MB while blocking all writers. This is called at end-of-test (cold path), not a practical concern — but worth knowing if this pattern gets reused in a hotter context.

Architecture — Consolidation of GetStandardOutput()/GetErrorOutput()

Moving lock acquisition and truncation-notice logic into GetContent() is the right design. The old duplicated read-lock try/finally in Context was error-prone; the new one-liners are a clear improvement.

LazyInitializer Refactor

Consistent with the rest of the class and correctly thread-safe (CAS semantics). The factory may run more than once under contention but since losing instances share the same underlying StringBuilder/lock via the lazy-init chain, there is no correctness issue.

---

Overall: Solid, well-reasoned fix. The design decisions (cap at write time, keep tail, amortized trim, truncation notice on read) are all correct. The main architectural alternative (ring buffer) would be premature for this use case. Ready to merge.

[claude]

Code Review

This is a clean, well-scoped fix for a real OOM issue. The design is sound — capping at write time with amortized trimming is the right place to enforce the limit.

Correctness — Surrogate Pair Boundary

The surrogate-pair guard is correct for the targeted case: if the last-removed char is a high surrogate, decrement removeCount to keep the full pair. A more semantically clear alternative would check the first kept char instead:

// More readable: ensure the first kept char is not a lone low surrogate
while (removeCount > 0 && char.IsLowSurrogate(_builder[removeCount]))
    removeCount--;

The current check is logically equivalent for valid UTF-16 input, but checking the kept boundary is easier to reason about.

Performance of StringBuilder.Remove(0, N)

Remove(0, N) with N ≈ 256K chars causes a full left-shift of the remaining 768K chars — O(remaining length) per trim. The 25% headroom amortizes this well for typical tests. If this ever becomes a hot path (a test continuously flooding output past 1MB), the alternative is tracking a _startOffset int and slicing on GetContent() — O(1) trim. For now the Remove approach is completely justified; just worth a comment like // O(n) shift; amortized by 25% headroom for future readers.

GetContent() Holds Read Lock During Allocation

StringBuilder.ToString() for a 1MB buffer allocates ~2MB while blocking all writers. This is called at end-of-test (cold path), so it is not a practical concern — but worth noting if this pattern gets reused in a hotter context.

Architecture — Consolidation of GetStandardOutput/GetErrorOutput

Moving lock acquisition and truncation-notice logic into GetContent() is the right design. The old duplicated read-lock try/finally in Context was error-prone; the new one-liners are a clear improvement.

LazyInitializer Refactor

Consistent with the rest of the class and correctly thread-safe (CAS semantics). The factory may run more than once under contention but since losing instances share the same underlying StringBuilder/lock via the lazy-init chain, there is no correctness issue.

---

Overall: Solid, well-reasoned fix. The design decisions (cap at write time, keep tail, amortized trim, truncation notice on read) are all correct. The main architectural alternative (ring buffer for O(1) trim) would be premature for this use case. Ready to merge.