[thomhurst]

Follow-up to #7084 (for #7074). The last CI run of #7084 failed in AOT mode on KeyedNotInParallelFailFastTests.FailFast_WithWaitingKeyedTests_AwaitsSessionHooksBeforeFixtureDisposal. The output showed the test host crashing with an unhandled OperationCanceledException from ProcessDynamicTestQueueAsync. Looking into it turned up three separate problems.

What was wrong

1. Every fail-fast run crashes the test host. This predates #7084. Fail-fast cancels a linked token that Microsoft.Testing.Platform does not own. Whatever OperationCanceledException the scheduler throws reaches TUnitTestFramework.ExecuteRequestAsync, which logs "Test execution stopped due to fail-fast." and then re-throws it. MTP only treats that exception as a cancellation when its own token fired, so the exception escapes Main and the process dies with exit code 0xE0434352. #6962 already handles this for SIGTERM, but fail-fast takes the same path. I reproduced the crash on current main and on the commit before #7084 (faf43b6ecd) with plain FailFastTest --fail-fast. CI didn't catch it because the TRX file still gets written, so the engine tests' assertions passed.

2. The dynamic test queue processor faults when the token is cancelled. ProcessDynamicTestQueueAsync waits on WaitToReadAsync(cancellationToken). If the failing test is the last one to run, the phases finish without throwing. ExecuteGroupedTestsAsync then awaits the processor on its normal path, and the cancelled wait becomes the scheduler's exception. That code path was the same before #7084. #7084's new catch only covers the path where the phases throw. A single failing keyed test (KeyedNotInParallelFailFast7074/Fails --fail-fast) hits this every time.

3. The test #7084 added was flaky. It gave each test its own key and relied on ExecutionPriority(Critical) to make Fails take the single slot first. With distinct keys, every test starts at once through Task.Run, and the tests race for the slot. In the AOT run the four Waits tests won the race and all passed (Passed was 4, against an assertion that it be less than 4). Fails then failed last, which led straight into problem 2.

Fix

• TUnitTestFramework: if a cancellation exception arrives and the platform's token has not fired, log it, mark the session failed and return. This mirrors the SIGTERM branch. The exception is still re-thrown when MTP's own token was cancelled.
• TestScheduler.ProcessDynamicTestQueueAsync: if the token is cancelled while the processor waits on the queue, the wait loop now ends instead of throwing. The processor also skips draining the queue once the token is cancelled. A dynamic batch that is already running is still awaited, so the fix(engine): apply max parallelism cap to keyed NotInParallel tests #7084 guarantee holds: session teardown waits for in-flight dynamic tests.
• KeyedNotInParallelFailFast7074: all tests now share one key. The scheduler hands out keys in priority order as it walks the tests, so Fails always runs first. When it releases the key, fail-fast has already cancelled the token, so each Waits test throws on its cancelled slot wait.
• Engine tests: the two existing fail-fast tests now also check that the process ended without the runtime's crash banner and with exit code 2 or 10. A new test, FailFast_WhenFailingTestRunsLast_EndsSessionWithoutCrash, covers problem 2.

A fail-fast run now exits with code 10 (session failed) instead of crashing.

Testing

All runs used --fail-fast against a net10.0 Debug build of TUnit.TestProject.

Filter	Source-gen	Reflection
KeyedNotInParallelFailFast7074/* with --maximum-parallel-tests 1	40/40 runs: exit 10, no crash, no Waits test passed, hook marker correct	40/40 runs, same results
KeyedNotInParallelFailFastDynamic7074/* with --maximum-parallel-tests 2	15/15 runs: exit 10, no crash, dynamic marker correct	15/15 runs, same results
KeyedNotInParallelFailFast7074/Fails	15/15 runs: exit 2, no crash	15/15 runs, same results
FailFastTest/*	5/5 runs: exit 10, no crash	5/5 runs, same results

Before the fix, every run I tried of the KeyedNotInParallelFailFast7074/*, .../Fails and FailFastTest/* filters crashed.

Other test runs:

• KeyedNotInParallelFailFastTests engine tests passed 3 times in a row, and KeyedNotInParallelMaxParallelismTests passed (Reflection mode; AOT is CI-only).
• TUnit.UnitTests passed (558 passed, 3 skipped).
• TUnit.Engine builds for all target frameworks with no warnings.

AOT was not verified locally: this machine has no MSVC linker, so Native AOT publish isn't possible here. The CI AOT leg covers it.

Summary by CodeRabbit

• Bug Fixes
  • Improved fail-fast behavior when a test fails: the session now ends cleanly without an unhandled-exception crash.
  • Prevented queued tests from starting after cancellation, while preserving the expected failed-session result.
  • Improved handling of cancellation so platform-requested cancellation and fail-fast cancellation produce the appropriate session outcome.

[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-18T12:57:15.167571Z	4ef3301	PR opened

ℹ️ 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.

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 13 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 8 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 8e8a682c-ecb3-4538-8736-5e9d64941887

📥 Commits

Reviewing files that changed from the base of the PR and between 4ef3301 and a8f25bc.

📒 Files selected for processing: 1

• tests/TUnit.Engine.Tests/Scheduling/KeyedNotInParallelFailFastTests.cs

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: e427b0f1-002a-49f7-9c7e-3adfe966edcf

📥 Commits

Reviewing files that changed from the base of the PR and between 540152e and 4ef3301.

📒 Files selected for processing: 4

• src/TUnit.Engine/Framework/TUnitTestFramework.cs
• src/TUnit.Engine/Scheduling/TestScheduler.cs
• tests/TUnit.Engine.Tests/Scheduling/KeyedNotInParallelFailFastTests.cs
• tests/TUnit.TestProject/Bugs/7074/KeyedNotInParallelFailFast.cs

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

📝 Walkthrough

The engine now handles fail-fast cancellation without crashing the host. The scheduler stops processing queued tests after cancellation. Regression tests verify failed-session outcomes and process exit results.

Changes: Fail-fast cancellation

Layer / File(s)	Summary
Engine cancellation handling src/TUnit.Engine/Framework/TUnitTestFramework.cs, src/TUnit.Engine/Scheduling/TestScheduler.cs	The framework records the session as failed and returns when fail-fast cancellation occurs. The scheduler exits without processing queued tests when cancellation is requested.
Fail-fast regression coverage tests/TUnit.Engine.Tests/Scheduling/KeyedNotInParallelFailFastTests.cs, tests/TUnit.TestProject/Bugs/7074/KeyedNotInParallelFailFast.cs	The tests check failed-session outcomes and verify that child processes exit without an unhandled-exception crash. The fixture uses one shared key for the failing and waiting tests.

Priority: ➖ Normal

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

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 4ef33

The fail-fast cancellation and process-exit checks show no identified issue requiring a fix before merge.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 21.43% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 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 main change: preventing fail-fast runs from crashing the test host.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches: 💡 1

📝 Generate docstrings: 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• 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, and I watched the queue.
Fail-fast stopped the waiting tests in place.
The session marked its outcome failed.
The child process finished without a crash.
I nibble clover and celebrate the run.}

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

[github-actions]

Review

The three fixes match the root causes described in the PR, and I found no blocking issues.

• TUnitTestFramework: the new branch mirrors the existing SIGTERM branch. It returns with SessionFailed = true when MTP's own token has not fired, and it still re-throws when MTP's token was cancelled. I checked that SessionFailed and FailFastCancellationSource exist on the service provider.
• ProcessDynamicTestQueueAsync: the WaitForDynamicTestsAsync wrapper filters on when (cancellationToken.IsCancellationRequested), so only genuine cancellation is swallowed. Skipping the final drain after cancellation is reasonable. The PR says a dynamic batch that is already running is still awaited, so the fix(engine): apply max parallelism cap to keyed NotInParallel tests #7084 guarantee should hold.
• Tests: putting all the tests on one key removes the race, and the new crash assertions (AssertEndedWithoutCrash) cover the regression that CI missed earlier. The OnExecutingDelegates hook exists in InvokableTestBase.

Optional suggestions:

1. AssertEndedWithoutCrash matches the English text Unhandled exception. and the exit codes 2 and 10. This is slightly brittle. You could assert ExitCode != unchecked((int)0xE0434352) and not 134 (SIGABRT) as well. Alternatively, move the crash check into InvokableTestBase so every fail-fast test gets it.
2. Two places now log "The test run was cancelled." and "Test execution stopped due to fail-fast.". A small helper that picks the message could remove the duplication. This is minor.

Looks good to merge.

[greptile-apps]

Confidence Score: 5/5

[High impact] The PR appears safe to merge; the previous test-coverage gap is addressed.

Summary

The PR ends fail-fast runs without crashing the host and waits for dynamic tests already running.

• Fail-fast runs end as failed sessions instead of crashing the test host.
• The dynamic test queue stops cleanly when fail-fast cancels its wait.

_{Reviews (2) · Last reviewed commit: "test(engine): require exit code 2 when f..." · Reviewed by Greptile}

[github-actions]

Review: No blocking issues. The three fixes match the problems described.

• TUnitTestFramework: Returning with SessionFailed = true when only the engine's token fired mirrors the SIGTERM branch. Re-throwing only when MTP's own token fired is the correct rule.
• ProcessDynamicTestQueueAsync: Ending the wait loop on cancellation and skipping the final drain is sound. A running dynamic batch is still awaited, so the fix(engine): apply max parallelism cap to keyed NotInParallel tests #7084 teardown guarantee holds. The exception filter on IsCancellationRequested avoids swallowing unrelated cancellations.
• Test fix: Putting all tests on one key makes Fails run first by priority order instead of by a race.

Suggestions (non-blocking):

1. ProcessResultCapture and its Capture extension are generic crash-detection helpers. If other fail-fast or cancellation engine tests need them, move them next to InvokableTestBase/RunOptions instead of leaving them in one test file.
2. The ShouldNotContain check on the runtime banner text is heuristic. The exit-code check is the more robust signal.
3. When the token is cancelled the unqueued dynamic tests are not drained. Please confirm they are reported as cancelled or skipped rather than missing from results.

The new FailFast_WhenFailingTestRunsLast_EndsSessionWithoutCrash test covers the previously untested path, and the repeat runs (40/40, 15/15) give good confidence. Looks good to merge.