[thomhurst]

Fixes #7075

Root cause

ConstraintKeyScheduler.ExecuteTestsWithConstraintsAsync started each test with free keys by calling ExecuteTestAndReleaseKeysAsync directly from its scheduling loop. Nothing in the pipeline yields for a synchronous test that has no async hooks, so the test body ran on the loop thread and the loop could not start the next test until it finished. Tests with distinct [NotInParallel] keys ran one after another on a single thread. An async [Before(Test)] hook containing await Task.Yield() hid the problem, which is why the WithYield class in the issue ran in parallel.

The release path had the same flaw. Each waiting test's startSignal was a plain TaskCompletionSource, so SetResult ran the waiter's continuation inline on the releasing test's thread, inside the finally that signals waiters. If one release unblocked several synchronous waiters, the first one ran to completion before the others were signalled.

Fix

Two changes in ConstraintKeyScheduler:

• Tests that can start right away are dispatched with Task.Run, the same approach as the netstandard2.0 fallback in TestScheduler.ExecuteWithGlobalLimitAsync.
• startSignal is created with TaskCreationOptions.RunContinuationsAsynchronously.

Nothing blocks on async. The scheduling loop still takes keys under the lock before dispatching, so key exclusion is unchanged.

Tests

New tests/TUnit.UnitTests/ConstraintKeySchedulerTests.cs drives the real ConstraintKeyScheduler and TestRunner with a coordinator that runs each test body synchronously and returns a completed ValueTask. Each blocking test waits on a Barrier with a 30s timeout, so the tests pass only if the participants run at the same time:

• SynchronousTests_WithDistinctKeys_RunConcurrently: 4 tests with distinct keys. Before the fix, all 4 timed out (2 minutes, run serially). After the fix it passes in milliseconds.
• SynchronousWaiters_ReleasedByTheSameTest_RunConcurrently: a gate test holds two keys and two waiters each need one of them. With only the Task.Run change and the old TCS, this test still fails, so it covers the RunContinuationsAsynchronously change on its own.

TestRunnerTests.CreateTest and FakeMessageBus were made internal so the new tests can reuse them.

I first wrote this as a TUnit.TestProject engine test, but it passes without the fix there: TestProject's global async hooks yield before the test body runs. That is why the regression tests are unit tests.

Runs:

• TUnit.UnitTests (net10.0), full suite: 528 passed, 3 skipped, 0 failed. Builds for all TFMs.
• TUnit.Engine.Tests (Reflection mode; AOT mode only runs on CI), all passed: ConstraintKeySchedulerConcurrencyTests, KeyedNotInParallelCrossPhaseTests, GlobalNotInParallelDependsOnTests, ParallelismValidationEngineTests, ClearParallelizationTests.

Performance

Measured with a throwaway harness, not committed: 20,000 no-op keyed tests over 64 keys, run through the scheduler 7 times per process, Release net10.0. Warm-run medians were about 25ms before and about 38ms after, roughly 0.6µs extra per keyed test for the thread-pool hop. That is negligible next to the real per-test pipeline. In exchange, synchronous keyed tests use all their parallelism instead of running serially.

Overlap with #7084 (#7074)

#7084 also edits ConstraintKeyScheduler.cs. It adds a maxParallelism constructor parameter and takes a SemaphoreSlim slot inside ExecuteTestAndReleaseKeysAsync. The hunks don't overlap: this PR only changes the startSignal construction and the immediate-start dispatch in the loop. The two fixes are complementary. Under #7084 alone, an uncontended WaitAsync completes synchronously, so synchronous tests would still run inline on the loop thread. Whichever PR merges second must add the new constructor argument to CreateScheduler in ConstraintKeySchedulerTests (e.g. new Lazy<int>(() => 8)).

Summary by CodeRabbit

• Bug Fixes
  • Fixed scheduling behavior that could cause synchronous tests to block other tests. Tests with separate constraints, and tests waiting on constraints released by another test, can now proceed concurrently.
• Tests
  • Added regression coverage for concurrent execution and for waiters released before the scheduler begins waiting on their start signal.

[coderabbitai]

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 425db99b-60c0-4b14-a2b8-ad3d9bcb5fd3

📥 Commits

Reviewing files that changed from the base of the PR and between 04d70f3 and ae84e8c.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 80433de5-f846-4c63-988b-a7fd92ae8cdb

📥 Commits

Reviewing files that changed from the base of the PR and between e85ffc9 and 04d70f3.

📒 Files selected for processing: 2

• src/TUnit.Engine/Scheduling/ConstraintKeyScheduler.cs
• tests/TUnit.UnitTests/ConstraintKeySchedulerTests.cs

🚧 Files skipped from review as they are similar to previous changes: 1

• src/TUnit.Engine/Scheduling/ConstraintKeyScheduler.cs

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 0 remain after this review.

📝 Walkthrough

The keyed scheduler dispatches eligible tests through the thread pool and forces queued tests to yield after their start signals complete. Regression tests cover synchronous tests with distinct and shared keys, and a waiter released before its start signal is awaited.

Changes: Keyed Scheduler Execution

Layer / File(s)	Summary
Asynchronous test starts src/TUnit.Engine/Scheduling/ConstraintKeyScheduler.cs, src/TUnit.Engine/Scheduling/ForceYieldingTaskAwaitable.cs	Start signals use asynchronous continuations. Eligible tests run through Task.Run. Queued tests force a yield on .NET 8 or later and use an internal awaitable on earlier targets.
Synchronous scheduler regression tests tests/TUnit.UnitTests/ConstraintKeySchedulerTests.cs, tests/TUnit.UnitTests/TestRunnerTests.cs	Tests cover concurrent execution with distinct and shared keys, and a waiter released before its start signal is awaited. Supporting helpers provide synchronous test execution and trace capture. CreateTest and FakeMessageBus are now internal.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to 04d70

The scheduler changes enable synchronous keyed tests to run concurrently; no material merge risk is established in the reviewed scope.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 25 functions across 4 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the primary change: enabling synchronous keyed NotInParallel tests to run concurrently.
Linked Issues check		Issue [#7075] requires synchronous tests with distinct NotInParallel keys to run concurrently. ConstraintKeyScheduler dispatches immediately startable tests with Task.Run, and it forces queued w…
Out of Scope Changes check		The changes stay within [#7075]. ForceYieldingTaskAwaitable provides the required compatibility behavior for targets before .NET 8. The added coordinator, trace device, helper accessibility, and reg…

Full details: Linked Issues check

Explanation

Issue [#7075] requires synchronous tests with distinct NotInParallel keys to run concurrently. ConstraintKeyScheduler dispatches immediately startable tests with Task.Run, and it forces queued waiter continuations to yield on all target frameworks. RunContinuationsAsynchronously prevents one releasing test from running a synchronous waiter inline before other waiters are signalled. The added barrier tests cover distinct-key execution and waiter release concurrency.

Full details: Out of Scope Changes check

Explanation

The changes stay within [#7075]. ForceYieldingTaskAwaitable provides the required compatibility behavior for targets before .NET 8. The added coordinator, trace device, helper accessibility, and regression tests support scheduler coverage. No unrelated production behavior or public API change is shown.

✨ Finishing Touches: 💡 2

📝 Generate docstrings: 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

🛠️ Fix failing CI checks: 💡

• Commit to this branch
• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{I’m a rabbit; I hop by the queue,
As tests start together, not one by one.
A signal now yields when its turn comes around,
Three gates and key sets keep watch for the flow,
Then I nibble a clover and bound through the code.}

_{Comment @coderabbitai help to get the list of available commands.}

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-18T08:09:42.881554Z	ae84e8c	New commits

ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[greptile-apps]

Confidence Score: 5/5

[High impact] The PR appears safe to merge; no new actionable issues remain.

Summary

Synchronous tests with different constraint keys now run concurrently.

• Synchronous keyed tests run alongside tests with different keys.

The three earlier, unnumbered findings are addressed by the always-yielding await and trace-controlled gates. thomhurst explicitly leaves the global parallel limit to #7084 because it is a separate, complementary fix.

No new actionable issues were found. Tests were not run during this review.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Check keys under lock] --> B{Keys free?}
    B -->|Yes| C[Reserve keys]
    C --> D[Dispatch with Task.Run]
    B -->|No| E[Queue waiter]
    E --> F[Await signal and always yield]
    D --> G[Run test]
    F --> G
    G --> H[Release keys under lock]
    H --> I[Reserve keys for eligible waiters]
    I --> J[Signal waiters asynchronously]
    J --> F

Loading

_{Reviews (4) · Last reviewed commit: "chore: merge origin/main into fix/7075-k..." · Reviewed by Greptile}

[github-actions]

Review of #7087

I found no blocking issues.

Diagnosis. The root cause matches the code. A synchronous test with no yielding hook ran on the scheduling loop's thread. Signalling a plain TaskCompletionSource also ran the waiter's continuation inline in the releaser's finally.

The fix is small and targeted.

• Task.Run dispatches only tests that can start immediately. Keys are still taken under the lock before dispatch, so key exclusion is unchanged.
• RunContinuationsAsynchronously on startSignal stops one release from running its waiters one after another on the same thread.
• Task.Run unwraps the returned Task, so test.ExecutionTask and activeTasks still observe completion and exceptions as before.
• Passing CancellationToken.None to Task.Run is deliberate. It guarantees the key-release finally always runs, because a cancelled token could stop the delegate from ever starting and leak the keys.

Tests.

• The Barrier-based tests fail on timeout when execution is serial and pass when the participants run concurrently.
• The second test covers the RunContinuationsAsynchronously change on its own.
• Both tests use the real ConstraintKeyScheduler and TestRunner, which is better than a mock-heavy test.

Non-blocking suggestions.

1. Each keyed test now costs one extra thread-pool hop. The PR description reports measurements, which is enough. If you ever want to avoid the hop, you could dispatch only when the test is not already running on a pool thread. I don't think that is worth the added complexity.
2. The Barrier timeout is 30s, so a regression makes the suite wait for the timeout before failing. That is acceptable, but a shorter timeout would fail faster. Barrier participants only need to start together, so 10s would still be generous.
3. TestRunnerTests.CreateTest and FakeMessageBus are now internal to share them. If more scheduler tests follow, consider moving them to a shared test-helpers file.

Looks good to merge.

[github-actions]

Review of #7087

The root-cause analysis is sound, and the fix looks correct and minimal.

• Task.Run dispatch: it moves the synchronous test body off the scheduling loop's thread. Keys are still taken under the lock before dispatch, so key exclusion is unchanged.
• RunContinuationsAsynchronously on startSignal: it stops the releasing test from running the first waiter inline before the other waiters are signalled.
• ForceYielding in WaitAndExecuteTestAsync: this covers the race where the keys are released between queueing and awaiting. The netstandard fallback awaitable reports IsCompleted => false. Its continuation is registered through ConfigureAwait(false), so it is queued to the thread pool rather than run inline.
• Tests: they use barriers with timeouts, so they fail if execution is serialized instead of passing by accident. Each of the three tests targets a distinct failure path. Making CreateTest and FakeMessageBus internal is a reasonable way to reuse them.

Minor, non-blocking points:

• Task.Run adds one thread-pool hop per test that can start immediately. Your performance numbers cover this. If the hop ever matters, you could dispatch only when the test body is not already yielding, but that is not worth the complexity now.
• Passing CancellationToken.None to Task.Run is intentional, because the key release has to happen even when cancellation is requested. A short comment saying so would help the next reader.

I haven't run the tests myself. Otherwise this looks good to merge.

[github-actions]

Review of #7087

The root-cause analysis is sound and the fix is minimal. Task.Run stops the scheduling loop from running a synchronous test body. RunContinuationsAsynchronously stops the releasing test from running its waiters inline. Key exclusion is still taken under the lock before dispatch, so it is unchanged. Passing CancellationToken.None to Task.Run is correct and well explained, because the finally block must run to release keys. The tests are good: barrier-based, so they fail on a serial run rather than relying on timing.

Concerns

1. Is ForceYieldingTaskAwaitable needed? The two changes above already fix the issue. After Task.Run, the loop's thread no longer executes that method body. A completed startSignal just continues on a pool thread, and RunContinuationsAsynchronously covers the waiter-release path. A custom struct awaiter for netstandard2.0/net472 is extra surface for a theoretical case. A simpler fallback is await Task.Yield() after the await.

2. The polyfill doc comment looks inaccurate. It says registering a continuation on an already-completed task queues it to the thread pool rather than inline. As far as I recall, AwaitTaskContinuation.Run can inline when there is no sync context and the scheduler is the default. If so, the fallback may not really force a yield. Task.Yield() is the reliable way to get a real yield on those TFMs. Please verify on net472/netstandard2.0, or drop the polyfill per point 1.

3. Test coverage of the non-NET8 path. If the polyfill stays, make sure at least one run exercises that branch.

Overall this is a good fix. I would simplify or justify the polyfill before merging.

[thomhurst]

Responding to the 16:14 review's concerns about ForceYieldingTaskAwaitable:

1. It is needed. Task.Run only covers tests that can start immediately. WaitAndExecuteTestAsync is still called directly by the scheduling loop. If a running test releases the keys between waitingTestIndex.Add and the first await, the start signal is already complete when it is awaited. A normal await then continues synchronously on the loop thread, whatever RunContinuationsAsynchronously says, because that option only affects continuations registered before completion. Greptile's P1 described this case, and SynchronousWaiter_ReleasedBeforeItAwaits_DoesNotBlockLaterTests reproduces it deterministically. Without the forced yield it times out.

2. The doc comment is accurate. IsCompleted => false makes the state machine call UnsafeOnCompleted, which goes through Task.SetContinuationForAwait. When the task is already complete, AddTaskContinuation fails. The no-context path then calls AwaitTaskContinuation.UnsafeScheduleAction, which queues the work to the thread pool. The context-flowing path calls tc.Run(this, canInlineContinuationTask: false), which also queues it. Neither path runs the continuation inline. This is the same mechanism .NET 8's ConfigureAwaitOptions.ForceYielding relies on: its awaiter also reports IsCompleted == false and registers the continuation the same way. Task.Yield() after a normal await would not help, because the inline continuation is what runs the test body. The yield has to replace that continuation, not follow it.

3. Coverage of the pre-net8 path. TUnit.UnitTests targets net8.0+, so CI does not exercise that branch. As noted in the thread reply, the regression test was run with the netstandard2.0 awaitable compiled into net10.0, and it passes.

I also merged origin/main (ae84e8c) to pick up b1cceb3, which fixes the failing Apple smoke jobs. After the merge, the ConstraintKeySchedulerTests and the full TUnit.UnitTests suite pass on net10.0 (533 passed, 3 skipped), and so do the new KeyedNotInParallelOrderTests (#7088) engine tests.

[github-actions]

Review of #7087

The root-cause analysis is sound and the fix is small and well targeted. I found no blocking issues.

What I checked

• Task.Run(..., CancellationToken.None) is correct. Keys are already taken under the lock, so the delegate must always run to release them. The comment explains this.
• RunContinuationsAsynchronously on startSignal stops the releasing test's thread from running a synchronous waiter inline.
• The ForceYieldingTaskAwaitable fallback is only compiled for pre-.NET 8 targets. Its IsCompleted => false forces a yield. As far as I recall, ConfiguredTaskAwaiter queues the continuation instead of inlining it when the task is already complete. I did not verify this on netstandard2.0, so please confirm it on the netstandard2.0 build.
• The tests are deterministic. They use a barrier with a 30s timeout and trace-hook gating, not sleeps, and each fails without its corresponding fix.

Suggestions (non-blocking)

1. The PR description is out of date. It lists two changes, but the diff also has the ForceYielding await in WaitAndExecuteTestAsync, the ForceYieldingTaskAwaitable helper, and a third regression test (SynchronousWaiter_ReleasedBeforeItAwaits...). Please update it so reviewers and release notes match.
2. The #if NET8_0_OR_GREATER split adds a custom awaiter just to force a yield. Since Task.Run is already used for the netstandard fallback in TestScheduler, you could use await Task.Yield() after the signal await on all TFMs. That removes the extra type and the conditional compilation. The trade-off is that Task.Yield can resume on a captured context or scheduler. That is harmless in the engine, which uses ConfigureAwait(false) throughout. Keep the current approach if you prefer the guarantee.
3. The PR notes overlap with fix(engine): apply max parallelism cap to keyed NotInParallel tests #7084. Please make sure whichever merges second updates CreateScheduler in the new tests, as described.

Overall: good fix, good regression coverage, and the perf cost (~0.6µs per keyed test) is acceptable. Approving in spirit.