fix: complete DedicatedThreadExecutor tests only after CleanUp() returns

[glennawatson]

Description

A test run by a DedicatedThreadExecutor finished before the executor's CleanUp() ran. The message pump completed the test's TaskCompletionSource as soon as the test body finished, and the thread called CleanUp() afterwards in its finally block. The engine could then start the next test, and its Initialize(), while the previous CleanUp() was still running. In a [NotInParallel] class, an executor that sets global state in Initialize() and resets it in CleanUp() saw the late CleanUp() overwrite the next test's state.

Changes in src/TUnit.Core/Executors/DedicatedThreadExecutor.cs:

• ExecuteAsyncActionWithMessagePump no longer touches the TaskCompletionSource. It returns the finished task, or throws TimeoutException on the existing 5 minute timeout. Its outer catch is gone, so other exceptions reach the thread's existing catch.
• The thread records the outcome (finished task, or the exception from Initialize() or the pump), runs CleanUp(), and only then completes the TaskCompletionSource with the result, exception or cancellation.
• A CleanUp() exception is now reported through the task. Before this change it escaped the dedicated thread as an unhandled exception and crashed the test process. It is combined the same way as a failing [After] hook:
  • The test passed: the test fails with the CleanUp() exception.
  • The test failed: AggregateException(testException, cleanUpException). The test's exception comes first, so FailureCategorizer still categorizes the failure by the test's own exception, and the message bus reports both members.
  • The test was cancelled: AggregateException(TaskCanceledException, cleanUpException), so the cancellation stays visible.

Nothing else changes: the browser fallback, ConfigureThread, the synchronization context and task scheduler, the pump loop and its timeout, and the exception unwrapping for a faulted test. STAThreadExecutor and CultureExecutor derive from DedicatedThreadExecutor and get the fix without changes. No other executor in src/TUnit.Core/Executors runs work on its own thread.

Most of the diff is one level of dedent in ExecuteAsyncActionWithMessagePump after removing its outer try/catch. Hide whitespace changes to see the real change.

Related Issue

Fixes #6885

Type of Change

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation update
• Performance improvement
• Refactoring (no functional changes)

Checklist

Required

• I have read the Contributing Guidelines
• If this is a new feature, I started a discussion first and received agreement (not applicable, bug fix)
• My code follows the project's code style (modern C# syntax, proper naming conventions)
• I have written tests that prove my fix is effective or my feature works

TUnit-Specific Requirements

• Dual-Mode Implementation: If this change affects test discovery/execution, I have implemented it in BOTH: (not applicable, the executor runs after metadata collection on the shared path; the regression test passes in both modes)
  • Source Generator path (TUnit.Core.SourceGenerator)
  • Reflection path (TUnit.Engine)
• Snapshot Tests: If I changed source generator output or public APIs: (not applicable, no public API or generator change; TUnit.PublicAPI passes unchanged)
  • I ran TUnit.Core.SourceGenerator.Tests and/or TUnit.PublicAPI tests
  • I reviewed the .received.txt files and accepted them as .verified.txt
  • I committed the updated .verified.txt files
• Performance: If this change affects hot paths (test discovery, execution, assertions):
  • I minimized allocations and avoided LINQ in hot paths
  • I cached reflection results where appropriate (no reflection involved)
• AOT Compatibility: If this change uses reflection: (not applicable, no reflection)
  • I added appropriate [DynamicallyAccessedMembers] annotations
  • I verified the change works with dotnet publish -p:PublishAot=true

Testing

• All existing tests pass (dotnet test) (the affected suites pass; the full repository suite was not run locally)
• I have added tests that cover my changes
• I have tested both source-generated and reflection modes (if applicable)

Additional Notes

Tests added:

• tests/TUnit.UnitTests/DedicatedThreadExecutorTests.cs calls a DedicatedThreadExecutor directly. Its CleanUp() sleeps 100 ms and then records that it finished. The tests check that CleanUp() has finished when the awaited task completes, for a test that passes (synchronously and asynchronously), fails, or is cancelled, and when Initialize() throws. They also check how a CleanUp() exception is reported on its own, with a test failure, and with a cancellation.
• tests/TUnit.TestProject/Bugs/6885/Tests.cs is the reproduction from the issue: a [NotInParallel] class with three tests and an executor whose CleanUp() sleeps 300 ms and resets global state. Each test asserts that no CleanUp() was running when it started and that the state is still configured after 400 ms.
• tests/TUnit.Engine.Tests/Issue6885Tests.cs runs that class through InvokableTestBase and expects 3 passed.

Both new test sets fail on unmodified main. The unit tests fail on the ordering checks, and the CleanUp() exception tests crash the test host with an unhandled exception. All 3 TestProject tests fail with cleanUpRunningAtStart true and the state reset.

Results on Linux x64, .NET SDK 11.0.100-rc.1 (the pinned 10.0.401 rolled forward by rollForward: latestMajor), with net10.0:

• TUnit.UnitTests, DedicatedThreadExecutorTests: 8 passed, run 3 times. The full TUnit.UnitTests suite: 375 passed.
• TUnit.TestProject, /*/*/DedicatedThreadExecutorCleanUpOrderTests/*: 3 passed in source-generated mode and 3 passed with --reflection.
• TUnit.Engine.Tests, /*/*/Issue6885Tests/*: reflection row passed. The AOT row is skipped locally by the repository's CI-only policy.
• Existing executor classes in TUnit.TestProject: CultureTests, HookExecutorTests, SetHookExecutorTests, TestExecutorScopeHierarchyTests and ExecutorEventReceiverTests pass. STAThreadTests is Windows-only and did not run on Linux.
• TUnit.PublicAPI: 5 passed, no snapshot changes.
• TUnit.Core, TUnit.UnitTests and TUnit.TestProject build for all their target frameworks with 0 errors.

Summary by CodeRabbit

• Bug Fixes
  • Tests now complete only after cleanup finishes, preventing cleanup from overlapping with the next test.
  • Cleanup failures are reported alongside test failures; cleanup failures also cause otherwise passing tests to fail.
  • Cancellation and initialization failures are reported after cleanup, including when cleanup itself fails.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

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: 9be074b1-aba1-4b92-8ef7-e8ca56cfba7d

📥 Commits

Reviewing files that changed from the base of the PR and between a64685e and 3682bba.

📒 Files selected for processing (4)

• src/TUnit.Core/Executors/DedicatedThreadExecutor.cs
• tests/TUnit.Engine.Tests/Issue6885Tests.cs
• tests/TUnit.TestProject/Bugs/6885/Tests.cs
• tests/TUnit.UnitTests/DedicatedThreadExecutorTests.cs

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

---

📝 Walkthrough

Walkthrough

The dedicated-thread executor now completes a test after cleanup returns. Execution and cleanup failures are reported separately or together. New unit and engine tests cover cleanup ordering, completion status, and failure cases.

Changes

Dedicated-thread cleanup completion

Layer / File(s)	Summary
Defer test completion until cleanup returns src/TUnit.Core/Executors/DedicatedThreadExecutor.cs	The message pump returns the action task or throws on timeout. The executor runs cleanup before completing the test source, and reports execution and cleanup failures.
Unit coverage for completion and failures tests/TUnit.UnitTests/DedicatedThreadExecutorTests.cs	Tests cover success, test failure, cancellation, initialization failure, cleanup failure, and combined failures. They verify that cleanup finishes before execution completes.
Cleanup-order regression coverage tests/TUnit.TestProject/Bugs/6885/Tests.cs, tests/TUnit.Engine.Tests/Issue6885Tests.cs	The regression tests check that three tests run after cleanup and that all three pass.

Priority: ➖ Normal

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

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to 3682b

The change appears ready to merge after normal checks; no concrete remaining issue is established by the supplied evidence.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 3682b

The change prevents a previous test’s cleanup from overlapping the next test and reports cleanup failures through the test result. No new access path is evident, but timeout and concurrent-reuse behavior are not fully covered.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The examined entrypoint retains its existing action-delegate and thread-creation path. The change affects result timing rather than adding an identified caller, privilege, or sensitive sink.

Trust Boundaries and Controls

• observed — The executor still receives an action delegate through ExecuteAsync; the changed completion helper is private and operates on the action task and captured lifecycle exceptions.

Resilience and Maintainability Implications

• inferred — Delayed completion improves containment for sequential tests that use shared state. Concurrent calls reusing one executor instance and the timeout-plus-cleanup-failure path are not directly established by the identified tests.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

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

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Issue [#6885] requires the test task to complete only after CleanUp() returns. DedicatedThreadExecutor.ExecuteAsync now runs CleanUp() before CompleteTest completes the TaskCompletionSource.…
Out of Scope Changes check	✅ Passed	The changes stay within issue [#6885]. The cleanup exception handling ensures that cleanup failures are reported after cleanup and combined with test failures or cancellation. The added unit and regre…
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: DedicatedThreadExecutor tests complete only after CleanUp() returns.

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Create a new PR

---

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

_{A rabbit waits beside the thread,
Until the cleanup work is shed.
The task returns; the tests proceed,
Each failure joins the reported deed.
Three green checks hop home instead.}

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

[greptile-apps]

Confidence Score: 5/5

[High risk] Changes test executor threading and completion order.

The PR appears safe to merge; no actionable issue was identified.

Summary

The executor now waits for CleanUp() before completing a test and reports cleanup exceptions through the test task. Unit and engine regression tests cover completion order and combined failures.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Initialize] --> B[Run test and pump continuations]
  B --> C[Record test outcome]
  C --> D[CleanUp]
  D --> E[Complete test task]
  E --> F[Engine proceeds]

Loading

_{Reviews (1) · Last reviewed commit: "fix: complete DedicatedThreadExecutor te..."}

[glennawatson]

full disclosure I had claude prepare the PR for you, had a look over it though and looks good from me. In ReactIveUI we do a lot of threading based stuff so this PR likely is a scenario a lot of users won't hit.

Also stole your idea of using CodeRabbit for our stuff from your repo so thanks for that.

[thomhurst]

Lgtm - thanks!