Repository navigation
feat(engine): raise thread pool minimum when blocking tests starve the pool - #7030
Conversation
…e pool Test bodies run inline on thread pool threads, with up to 4x ProcessorCount tests in flight. When tests block (Thread.Sleep, .Result, sync I/O), the pool adds threads beyond its minimum at only one or two per second. Timer callbacks (Task.Delay, CancelAfter, [Timeout]) also need a pool thread, so timing-sensitive tests in unrelated classes overshoot their deadlines and flake. A static SetMinThreads at run start fixes the timing but costs ~15% wall time on the 12k-test PerformanceBenchmarks suite, because extra pool threads spin and contend. Instead, ThreadPoolStarvationMonitor samples on a dedicated thread every 50ms: it queues a probe on the global queue (where timer callbacks wait) and, once the probe waits a full interval while CPU utilization is under 50%, raises the minimum to the parallel test limit plus ProcessorCount in a single step. Incremental raises can stall when threads are already in cooperative blocking (Task.Wait), because the runtime defers the raise to its blocking adjustment. The minimum is restored when the run ends, a user-configured minimum is never lowered, and TUNIT_DISABLE_THREADPOOL_TUNING=1 opts out. Timeout messages now include a thread pool snapshot and a hint when the pool looks saturated. Co-Authored-By: Claude <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: 🔵 Low · up to Runs with a preconfigured thread-pool minimum can raise it sooner than intended. This is a bounded tuning risk that can be accepted with owner awareness or corrected before merge. Pre-merge checks |
|
Review: ThreadPoolStarvationMonitorI reviewed the diff and found no blocking issues. I did not build the PR or run its tests. The What I checked
Non-blocking suggestions
Overall this looks good to merge. |
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f8da2a342
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Adds a strongly typed opt-out for the thread pool starvation monitor, set via context.Settings.Execution.ThreadPoolTuningEnabled in a [Before(HookType.TestDiscovery)] hook like the other TUnitSettings. The TUNIT_DISABLE_THREADPOOL_TUNING environment variable keeps precedence. Co-Authored-By: Claude <noreply@anthropic.com>
Review: ThreadPoolStarvationMonitorI read the monitor itself; I did not run the tests or build the PR. The reactive design is sound and the PR description explains it well. The probe is queued on the global queue, where timer callbacks wait. A raise is gated on low CPU, which separates blocked threads from busy ones. It raises once, then stops sampling. It also restores the original minimum on dispose and never lowers a user-set one. The opt-outs (env var and setting) and the timeout diagnostics are useful. Notes, none blocking:
LGTM overall. |
- Run the monitor only on Windows, Linux, macOS and FreeBSD. Browser and WASI are single-threaded, so Thread.Start throws, and iOS/tvOS do not support reading process CPU time. - Catch exceptions on the monitor thread, so a failure stops monitoring instead of terminating the test process. - Restore the minimum observed at raise time, not the one captured at Start, so a value user code set in between is not undone. - Add tests for raise/restore, stopping without a raise, preserving user changes, the at-cap short circuit, and the environment variable opt-out. Co-Authored-By: Claude <noreply@anthropic.com>
Review: ThreadPoolStarvationMonitorI read The design holds up.
Suggestions, none blocking:
Looks good overall. |
…sample allocation - Use a 40ms probe-wait threshold under the 50ms sample interval, so wait jitter cannot delay detection by a sample. - Below .NET 9, reuse one Process instance for CPU sampling instead of allocating one per sample. Co-Authored-By: Claude <noreply@anthropic.com>
|
Responding to the claude-review notes:
|
Review: ThreadPoolStarvationMonitorI read the diff and found no blocking problems. The design is sound. What I like
Points to consider (non-blocking)
The timeout diagnostics addition is useful and low risk. Overall this looks good to merge, with the unbounded-parallelism cap as the one item I would address. |
- Clamp the raised minimum to 64 threads per processor, so an oversized --maximum-parallel-tests value cannot set a minimum of thousands of threads and disable the runtime's injection throttling. - Log at debug level when the monitor raises the minimum, restores it, or stops after an error. - Share the TUNIT_DISABLE_* truthy parsing (1/true/yes) between the monitor and HtmlReporter via EnvironmentFlags, and document the accepted values. Co-Authored-By: Claude <noreply@anthropic.com>
|
Responding to the latest claude-review notes, addressed in e97f2e3:
|
Problem
Suites with timing, cancellation, or deadline-based tests time out or flake when many tests run in parallel. The cause is thread pool starvation, not yielding: an
awaitthat yields frees its thread.ProcessorCount * 4tests concurrently, and test bodies run inline on pool threads.ProcessorCount. Beyond that, it adds threads at only about one or two per second.Thread.Sleep,.Result, sync I/O), timer callbacks (Task.Delay,CancelAfter, TUnit's own[Timeout]) also wait for a thread. Timing-sensitive tests in unrelated classes then overshoot their deadlines.Fix
ThreadPoolStarvationMonitorruns while the scheduler executes tests.ProcessorCount, in one step.ThreadPool.GetMaxThreads. Keyed[NotInParallel]tests and dynamic tests run alongside the limited parallel phase, so more tests than the limit can block at once. Escalation waits for the pool to catch up, so raises never stack.context.Settings.Execution.ThreadPoolTuningEnabled = falsein a[Before(HookType.TestDiscovery)]hook, or withTUNIT_DISABLE_THREADPOOL_TUNING=1. The environment variable takes precedence.Timeout diagnostics now include a thread pool snapshot: threads, busy workers, pending work items, and min/max workers. When work is queued and every pool thread is busy, the message says the timeout may come from blocking calls in other tests.
Why reactive instead of a static
SetMinThreadsbenchmarks/TUnit.PerformanceBenchmarks(12k tests, 2.63s → 3.02s), because extra pool threads spin and contend. Even min=24 on 20 cores was measurably slower.Task.Wait/.Result), the runtime handsSetMinThreadsto its blocking adjustment (PortableThreadPool.SetMinThreads/Blocking.cs). Incremental raises then left the pool stuck at about 22 threads for seconds in about 40% of runs on a mixed Sleep + GetResult workload. A single raise was reliable in every run after the change. Escalation therefore only raises again onceThreadPool.ThreadCounthas reached the previous minimum.Measurements (20 cores, A/B on the same binary via the opt-out env var)
Task.Delay(50)overshoot (10 runs each)The blocking repro ran 240 blocking tests alongside 240 timer probes.
Escalation, measured with 60 blocking tests on distinct
[NotInParallel]keys plus 40 timer probes, at--maximum-parallel-tests 4(first target 24 threads), 4 runs each:Task.Delay(50)overshootThe PerformanceBenchmarks A/B was re-run after escalation was added and was still neutral (3686ms off vs 3462ms on, on a noisier machine).
Testing
ThreadPoolStarvationMonitorTests(raise and restore, escalation gating, user-changed minimums, clamping to the pool maximum, opt-outs, env flag parsing) and the existingTimeoutHelperTestspass on net10.0 and net8.0.tests/TUnit.PublicAPIsnapshots are updated for the one new public property,ExecutionSettings.ThreadPoolTuningEnabled.TUnitSettingsTestscover the new default and check that the monitor does not start when the setting is off.TimeoutTests1,HookTimeoutTests, andDefaultTimeoutClassificationTestspass.TUnit.Engine.Testsrun failsFSharp.Test. The same 7 F# project tests fail locally with the monitor on and off, so this change does not cause it.Thread,ThreadPool,Environment.CpuUsage, andProcess, with no reflection.Metadata collection is unchanged, so source-gen and reflection modes are unaffected.
🤖 Generated with Claude Code
Summary by CodeRabbit
Summary by CodeRabbit