refactor: adjust parallelism handling in TestScheduler for better resource management

[thomhurst]

No description provided.

[claude]

Claude finished @thomhurst's task —— View job

---

PR Review: Parallelism Handling Refactor

I've completed my analysis of this performance-focused refactor. Here's my comprehensive review:

Summary

This PR makes two key adjustments to TUnit's parallelism handling:

1. TestScheduler.cs: Fixes unbounded concurrency issue and reduces default parallelism
2. Program.cs: Adds 117 stress tests to validate the parallelism improvements

🎯 Code Quality & Best Practices

✅ Excellent

• Modern C# syntax (collection expressions, pattern matching)
• Proper async/await patterns with ConfigureAwait(false)
• Clear variable naming and method organization
• Follows TUnit's established coding conventions
• Good separation of concerns in test grouping

✅ Performance Optimizations

• Critical Fix: ExecuteUnlimitedAsync now respects MaxDegreeOfParallelism = _maxParallelism (lines 482-484 in TestScheduler.cs)
• Default Adjustment: Reduced from 8x to 4x CPU cores (lines 557-558) to prevent thread pool saturation
• Uses Parallel.ForEachAsync for bounded concurrency (NET6+ optimization)
• Maintains separate code paths for limited vs unlimited tests

🐛 Potential Issues

⚠️ Minor Concerns

1. Comment Accuracy: Line 477 comment says "unlimited concurrency" but code now applies global limit - consider updating for clarity
2. Test Coverage: The new profile tests don't have assertions - they're purely for performance profiling (which appears intentional)

✅ No Major Issues Found

• No resource leaks detected
• Proper semaphore handling with try/finally blocks
• Cancellation tokens properly propagated
• No breaking API changes

🚀 Performance Considerations

✅ Significant Improvements

• Thread Pool Protection: Prevents saturation with high test counts
• Resource Management: Better bounded concurrency for "unlimited" tests
• Scalability: 4x CPU cores is more realistic for mixed workloads
• Stress Testing: New MassiveParallelTests class adds 117 tests covering CPU-bound, I/O-bound, and mixed scenarios

Performance Impact Analysis:

// Before: Truly unlimited parallelism (risky)
new ParallelOptions { CancellationToken = cancellationToken }

// After: Bounded by global limit (safer)
new ParallelOptions { 
    MaxDegreeOfParallelism = _maxParallelism,
    CancellationToken = cancellationToken 
}

🔒 Security Assessment

✅ No Security Concerns

• No reflection usage that affects AOT compatibility
• No credential exposure or secrets
• Proper resource disposal patterns maintained
• Cancellation handling prevents resource exhaustion

🧪 Test Coverage Assessment

⚠️ Recommendations

1. Profile Tests: Consider adding performance assertions or benchmarks to validate the parallelism improvements
2. Integration Tests: Verify behavior under thread pool pressure with the new limits
3. Edge Cases: Test with MaxDegreeOfParallelism = 1 and very high values

✅ Stress Test Coverage

• 49 CPU-bound parallel tests
• 49 I/O-bound async tests
• 19 mixed workload tests
• Good coverage of parameter expansion scenarios

📋 Specific Code Review

TUnit.Engine/Scheduling/TestScheduler.cs:482-484

new ParallelOptions
{
    MaxDegreeOfParallelism = _maxParallelism,  // ✅ Critical fix
    CancellationToken = cancellationToken
}

This change prevents the "unlimited" tests from actually being unlimited, which was causing thread pool exhaustion.

TUnit.Engine/Scheduling/TestScheduler.cs:557

var defaultLimit = Environment.ProcessorCount * 4;  // ✅ Better default

Reducing from 8x to 4x is more conservative and realistic for production workloads.

🎯 Final Verdict

✅ Recommended for Merge

This is a well-executed performance improvement that addresses a real issue with thread pool saturation. The changes are:

• Focused: Targets specific parallelism issues
• Safe: Maintains backward compatibility
• Tested: Includes comprehensive stress tests
• Documented: Clear comments explaining the rationale

The addition of 117 stress tests provides valuable validation for the parallelism changes and helps prevent regressions.

Minor Suggestion: Update the comment on line 477 to clarify that "unlimited" now means "no per-test constraints" rather than "unlimited concurrency."

---