fix: enforce ParallelLimiter semaphore in TestRunner to prevent DependsOn bypass

[thomhurst]

Summary

Fixes #5525 — [ParallelLimiter<T>] with Limit=2 allowed 3 concurrent tests when [DependsOn] was present.

• Root cause: The parallel limiter semaphore was acquired in TestScheduler and ConstraintKeyScheduler, but TestRunner.ExecuteTestInternalAsync executes dependency tests via a direct recursive call to ExecuteTestAsync that bypassed the semaphore. When a test without [ParallelLimiter] but with [DependsOn] triggered its dependencies, those could run without holding a semaphore slot.
• Fix: Move semaphore acquisition into TestRunner.ExecuteTestInternalAsync — after dependency resolution but before the test body — so it applies regardless of entry point (scheduler or dependency recursion). Remove the now-redundant semaphore logic from TestScheduler and ConstraintKeyScheduler.
• Regression test: TUnit.TestProject/Bugs/5525/ — 20 tests with [ParallelLimiter<Limit2>] plus a check test with [DependsOn] asserting peak <= 2. Fails 5/5 before the fix, passes 10/10 after.

Test plan

• Regression test ParallelLimiterExceedsLimitTests passes consistently (10/10 runs)
• Existing ParallelLimiterTests pass
• Existing LimitedParallelTests, StrictlySerialTests, HighParallelismTests pass
• Existing DependsOnTests, DependsOnAndNotInParallelTests pass
• Existing ConstraintKeyStressTests pass
• Existing NotInParallelExecutionTests pass

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics -11 complexity

Metric	Results
Complexity	-11

View in Codacy

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

[claude]

Code Review

Summary

This is a clean fix for a genuine bug. The root cause analysis is accurate: the parallel limiter semaphore was scattered across multiple callsites, making it easy for execution paths (specifically dependency recursion in ) to bypass it. Centralising acquisition in is the right architectural call — it ensures the constraint is enforced at the single point where all test bodies eventually run.

---

What's Good

Correct fix location. Moving semaphore acquisition into means every execution path — scheduler, dependency recursion, dynamic tests — hits the same guard. The old multi-site approach was fragile by design.

Deadlock prevention is well-thought-out. Executing dependencies before acquiring the limiter (with an explanatory comment) is correct. If A depends on B and both share , acquiring the limiter for A before executing B would deadlock. The new code avoids this.

Significant simplification. Removing , , , and the partition-by-limiter logic from eliminates ~60 lines of complexity. The old \u201coptimisation\u201d that split tests into limiter/no-limiter groups was also the source of the bug — these separate paths made it easy for the constraint to be missed.

The peak-tracking pattern in the regression test is correct. The lock-free CAS loop for updating is sound.

---

Issues and Concerns

1. Lock ordering with (worth verifying)

In the new code, when a constrained test runs through :

1. Constraint keys are acquired (in )
2. Parallel limiter is acquired (in )
3. Test executes
4. Parallel limiter is released
5. Constraint keys are released

This ordering (keys → limiter) is consistent and there's no circular wait possible, because: a test only waits for the limiter after all its dependencies finish, so no test can simultaneously hold the limiter while waiting for constraint keys.

However, the deleted comment in said:

That comment was already wrong (keys were already held when the limiter was acquired), so deleting the code is correct. Worth noting in the PR though that the comment was misleading.

2. Static concurrency state in the regression test won't reset between re-runs

If the test session re-runs this class (e.g. via retry logic or IDE re-runs in the same process), accumulates from previous runs. would then see an inflated peak and could fail or incorrectly pass depending on timing.

Consider resetting in a hook:

3. parameter is dead on .NET 8+

This was pre-existing, but it's slightly more visible now that the function was simplified. Not a bug, but confusing.

4. is not thread-safe (pre-existing)

In and :

This is a pre-existing race — two threads could simultaneously see and both call . The deduplication in (via ConcurrentDictionary) ensures the test body only runs once, but two objects wrapping the same underlying work could be created. Not introduced by this PR, just worth flagging.

---

Minor Nit

The class is in the test file. If other regression tests in the same namespace later need a , they'll conflict. Consider a shared file or a namespace-qualified name like . Tiny concern.

---

Verdict

The fix is correct and the refactoring is a genuine improvement. The and are now cleaner and the invariant is clearer: "parallel limiter is always enforced inside , full stop."

The static field reset is the most actionable item before merging. The rest are pre-existing or very minor.

[claude]

Code Review

Summary

This is a clean fix for a genuine bug. The root cause analysis is accurate: the parallel limiter semaphore was scattered across multiple callsites, making it easy for execution paths (specifically dependency recursion in TestRunner) to bypass it. Centralising acquisition in TestRunner.ExecuteTestInternalAsync is the right architectural call — it ensures the constraint is enforced at the single point where all test bodies eventually run.

---

What's Good

Correct fix location. Moving semaphore acquisition into ExecuteTestInternalAsync means every execution path — scheduler, dependency recursion, dynamic tests — hits the same guard. The old multi-site approach was fragile by design.

Deadlock prevention is well-thought-out. Executing dependencies before acquiring the limiter (with an explanatory comment) is correct. If A depends on B and both share Limit=1, acquiring the limiter for A before executing B would deadlock. The new code avoids this.

Significant simplification. Removing ExecuteSingleTestAsync, ExecuteWithLimitAsync, ExecuteUnlimitedAsync, and the partition-by-limiter logic from TestScheduler eliminates ~60 lines of complexity. The old "optimisation" that split tests into limiter/no-limiter groups was also the source of the bug — separate paths made it easy for the constraint to be missed.

The peak-tracking pattern in the regression test is correct. The lock-free CAS loop for updating s_peak is sound.

---

Issues and Concerns

1. Lock ordering with ConstraintKeyScheduler (worth verifying)

In the new code, when a constrained test runs through ConstraintKeyScheduler the effective order is:

1. Constraint keys are acquired
2. Parallel limiter is acquired (inside TestRunner.ExecuteTestInternalAsync)
3. Test executes
4. Parallel limiter is released
5. Constraint keys are released

There is no circular wait possible here because a test only waits for the limiter after all its dependencies finish — no test can simultaneously hold the limiter while waiting for constraint keys.

The deleted comment in ConstraintKeyScheduler previously said: // Two-phase acquisition: Acquire ParallelLimiter BEFORE executing — This ensures constrained resources are acquired before holding constraint keys. That comment was already wrong (keys were already held when the limiter was acquired in the old code), so removing the code is correct. Worth noting in the PR description so future readers looking at git blame aren't confused.

2. Static concurrency state in the regression test won't reset between re-runs

private static int s_concurrent;
private static int s_peak;

If the test session re-runs this class (e.g. via retry logic or IDE re-runs in-process), s_peak accumulates from previous runs. PeakIsAtMostLimit would then assert against a stale inflated peak. Suggest resetting in a [Before(Class)] hook:

[Before(Class)]
public static void Reset()
{
    s_concurrent = 0;
    s_peak = 0;
}

This is the most actionable item in the review.

3. globalSemaphore parameter is unused on .NET 8+

ExecuteWithGlobalLimitAsync accepts a SemaphoreSlim globalSemaphore parameter that is never read on the .NET 8+ code path (the method uses Parallel.ForEachAsync with MaxDegreeOfParallelism instead). Pre-existing, not introduced here, but slightly more visible now that the surrounding code was simplified. Minor code smell.

4. test.ExecutionTask ??= ... is not thread-safe (pre-existing)

Two threads racing on a parallel test array could both observe ExecutionTask == null and call ExecuteTestAsync, creating two Task wrappers around the same underlying work. The ConcurrentDictionary in ExecuteTestAsync ensures the test body only runs once, so there is no correctness issue — but it is worth a follow-up clean-up.

---

Minor Nit

Limit2 is declared internal sealed at the bottom of the test file. If a future regression test in the same namespace also needs a Limit2, there will be a naming conflict. Consider a shared TestLimits.cs in the Bugs folder or naming it Limit2_5525.

---

Verdict

The fix is correct and the refactoring is a genuine improvement. ConstraintKeyScheduler and TestScheduler are now simpler, and the invariant is clearer: "the parallel limiter is always enforced inside TestRunner." The multi-callsite pattern that caused the bug is fully eliminated.

The static field reset (item 2) is the most actionable item before merging. The rest are pre-existing issues or minor cosmetic concerns.

[claude]

Code Review

The root-cause diagnosis and the fix are both correct. Moving the ParallelLimiter semaphore acquisition from the schedulers into TestRunner.ExecuteTestInternalAsync is the right architectural decision — it is the single place all tests pass through regardless of entry point (scheduler dispatch or dependency recursion). The PR also correctly executes dependencies before acquiring the limiter, which avoids the obvious deadlock where dep-chain members share the same limiter.

Correctness — what I verified

No deadlock possible in the new design.
The critical invariant is: once acquiredLimiter is held, no further blocking lock acquisition occurs (constraint keys, global semaphore, or another limiter). Dependencies run without holding the limiter, and after the limiter is acquired only _testCoordinator.ExecuteTestAsync (the actual test body) runs. There is no circular waiting possible.

Deduplication is preserved.
ExecuteTestAsync uses _executingTests (ConcurrentDictionary + TCS) to ensure a test is executed exactly once. When a dependency is already running (dispatched by the scheduler), the recursive call correctly returns the existing TaskCompletionSource<T> task without double-acquiring the limiter.

Mutual exclusion between schedulers.
TestGroupingService assigns each test to exactly one group (Parallel, ConstrainedParallelGroups, KeyedNotInParallel, NotInParallel). No test can be dispatched by both TestScheduler and ConstraintKeyScheduler simultaneously, so there is no double-scheduling risk.

using System.Buffers removal is correct — no ArrayPool usage remains in TestScheduler.

netstandard2.0 semantics are unchanged.
The old netstandard2.0 path in ExecuteWithGlobalLimitAsync already held globalSemaphore while waiting for parallelLimiterSemaphore. The new code does the same (globalSemaphore is still held by Task.Run's lambda while ExecuteTestAsync — which internally awaits the limiter — runs). No regression, no new deadlock.

---

One concern worth considering

globalSemaphore starvation in netstandard2.0 path when maxParallelism < parallelLimiter.Limit.
After this PR, on netstandard2.0 a test that acquires a globalSemaphore slot can then block inside ExecuteTestAsync waiting for the parallelLimiterSemaphore. If all maxParallelism slots are occupied by threads waiting for a ParallelLimiter (because all limiter slots are taken), tests without a ParallelLimiter cannot run until a limiter slot is freed. This is not a deadlock (limiter holders are actively running their test body and will release), but it temporarily underutilises the global concurrency budget.

This was actually the motivation for the old ExecuteWithLimitAsync / ExecuteUnlimitedAsync split. Removing that split is fine for the NET8 path (which now uses Parallel.ForEachAsync — the async work-stealing scheduler handles this naturally). For the netstandard2.0 path the efficiency trade-off is acceptable given that: (a) netstandard2.0 is a legacy/compat target, (b) the scenario requires maxParallelism ≤ parallelLimiter.Limit AND all limit slots occupied simultaneously, and (c) the old optimisation added significant complexity that led to the original bug.

---

Regression test feedback

The ParallelLimiterExceedsLimitTests test is solid. A few observations:

1. Limit2 scoping is correct. internal sealed class Limit2 is in the TUnit.TestProject.Bugs._5525 namespace and does not conflict with Limit1, Limit3, Limit10 in ParallelismValidationTests. ParallelLimitLockProvider keys by Type, so the semaphore is unique to this class.

2. The CAS-based peak measurement is correct. Using Interlocked.Increment / Interlocked.CompareExchange to track s_peak and await Task.Delay(50) to force overlap is a well-established pattern for concurrency measurement.

3. Possible coverage gap. The PeakIsAtMostLimit test itself has no ParallelLimiter. The case where the dependent test also carries [ParallelLimiter<Limit2>] (and therefore needs to acquire the semaphore after its dependencies release it) is not covered. It would be worth adding a second check test like:

[Test, ParallelLimiter<Limit2>]
[DependsOn(nameof(T01)), DependsOn(nameof(T02)), /* ... */]
public async Task PeakIsAtMostLimitWithSameLimiterOnDependent()
{
    // by the time we get here all T01-T20 are done and this test holds one limiter slot
    await Assert.That(s_peak).IsLessThanOrEqualTo(2);
}

This also tests that the two-phase acquire (deps first, then limiter) works when the depending test shares the same limiter as its dependencies — which is the scenario most likely to regress.

---

Overall: The fix is correct, the design is sound, and the PR is a clear improvement. The main suggestion is a minor test coverage addition. Happy to see this merged.

[claude]

Code Review — Follow-up Pass

This is a follow-up to the two prior reviews on this PR. Both actionable items raised in those reviews have been addressed:

• [Before(Class)] reset for static fields — Reset() is present, correctly clearing s_concurrent and s_peak before each class run. Stale-peak issue is resolved.
• Coverage gap for dependent test sharing the limiter — PeakIsAtMostLimitWithSameLimiterOnDependent has been added, covering the two-phase acquire path where the dependent test itself holds [ParallelLimiter<Limit2>].

---

Current State Assessment

The fix is correct and the architecture is sound. Moving semaphore acquisition into TestRunner.ExecuteTestInternalAsync is the right design — it is the single execution chokepoint regardless of entry point (scheduler dispatch or dependency recursion). The dependency-before-limiter ordering correctly avoids deadlocks in chains that share a limiter.

No new issues introduced. The remaining pre-existing concerns noted in prior reviews (test.ExecutionTask ??= ... race, globalSemaphore parameter unused on NET8+) are unchanged.

One Minor Observation

Limit2 is internal sealed and declared inside ParallelLimiterExceedsLimitTests.cs. Since ParallelLimitLockProvider keys semaphores by Type, the semaphore is scoped to this specific class and does not conflict with any existing Limit2 in other namespaces. No action needed — just confirming the scope is intentional.

---

Verdict

Both actionable items from prior reviews are addressed. The fix is correct, the regression test is comprehensive, and the net reduction in scheduler complexity (~60 lines removed) is a genuine improvement. Ready to merge.