You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
A factory that synchronously throws after observing its timeout token now follows the same timeout/external-cancellation classification as a returned canceled task. The helper preserves custom cancellation diagnostics and leaves unrelated cancellation unchanged, while continuing to arm ordinary factory timeouts before invocation.
Added synchronous coverage for timeout, external-token preservation, independent and tokenless cancellation, and diagnostic preservation alongside the existing returned-task and scheduled-body cases.
No actionable comments were generated in the recent review.
Walkthrough
The timeout helper now handles matching cancellation exceptions thrown synchronously by a task factory. Tests cover timeout classification, external cancellation tokens, and independent cancellation exceptions.
Changes: Timeout cancellation handling
Layer / File(s)
Summary
Factory cancellation classification and coverage src/TUnit.Engine/Helpers/TimeoutHelper.cs, tests/TUnit.UnitTests/TimeoutHelperTests.cs
The helper routes matching synchronous cancellation exceptions through the existing asynchronous handling flow. Tests verify timeout classification and preservation of external or independent cancellation exceptions.
thomhurst/TUnit#7527: This PR changes the same timeout helper to start timeouts around factory invocation. The current change handles synchronous cancellation from that factory.
Merge Risk: Minimal · up to 6f41d
No merge-blocking issue is identified in the supplied review evidence.
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 with cyclomatic complexity of at least 2. Documentat…
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: correcting cancellation classification for synchronous task factories during timeouts.
Linked Issues check
Directly linked issue #7532 has coding requirements only. Each one is met at head 6f41dc9 relative to merge base be37704:
1. Route synchronous factory cancellation through the token-aware classific…
Out of Scope Changes check
The whole-PR diff against merge base be37704 changes two files. Both are tied to #7532:
- src/TUnit.Engine/Helpers/TimeoutHelper.cs (+16/-1): adds the synchronous-throw handling in `ExecuteWithTim…
Full details: Linked Issues check
Explanation
Directly linked issue #7532 has coding requirements only. Each one is met at head 6f41dc9 relative to merge base be37704:
Route synchronous factory cancellation through the token-aware classification [#7532]: TimeoutHelper.cs wraps the taskFactory(timeoutCts.Token) call in a try/catch. The catch filters on timeoutCts.IsCancellationRequested and IsTimeoutCancellationToken. Routine timeout cancellation becomes Task.FromCanceled(timeoutCts.Token). The existing await and grace-period path then throws TimeoutException.
External cancellation remains OperationCanceledException with the external token [#7532]: an exception carrying externalToken does not match the timeout filter. It propagates unchanged. The async method then faults with that exception. The new test Synchronous_External_Cancellation_Preserves_External_Token asserts this.
Preserve unrelated cancellation and diagnostic exceptions [#7532]: non-routine matching exceptions go through Task.FromException(exception). IsRoutineCancellation returns false for them, so exceptionToPreserve keeps them as the TimeoutException inner exception and message. Unrelated tokens do not match the filter. Tests Synchronous_Independent_Cancellation_Is_Preserved (tokenless and token-bearing) and Synchronous_Timeout_Cancellation_Preserves_Diagnostics cover these paths.
Synchronous-throw coverage beside the returned-canceled-task case [#7532]: the parameterized Ordinary_Factory_Has_Timeout_Armed_Before_Invocation(bool throwsSynchronously) test covers both forms. Both expect TimeoutException with no inner exception and no cancellation type names in the message.
Timer-before-factory ordering [#7532]: timeoutCts.CancelAfter(timeout) still runs before taskFactory when startTimeoutImmediately is true. The diff does not change this line.
Scheduled-body timeout behavior [#7532]: ExecuteWithTimeoutFromStartAsync still uses the same core with startTimeoutImmediately: false. The new catch only acts when the timeout token has been canceled. The scheduled path's behavior is unchanged except for that case.
I did not run the test suite. The PR's reported 30-test pass result is not independently verified. The conclusion rests on source and test diff inspection.
Full details: Out of Scope Changes check
Explanation
The whole-PR diff against merge base be37704 changes two files. Both are tied to #7532:
src/TUnit.Engine/Helpers/TimeoutHelper.cs (+16/-1): adds the synchronous-throw handling in ExecuteWithTimeoutCoreAsync, which is the change the issue requests. The helper methods IsTimeoutCancellationToken and IsRoutineCancellation are reused from the existing classification. No public signatures change. Behavior for returned tasks, the grace-period path, and the scheduled path is unchanged except for the new synchronous-cancellation case.
tests/TUnit.UnitTests/TimeoutHelperTests.cs (+62/-2): supporting tests for the new behavior. They cover the synchronous timeout, external, independent, and diagnostic cases. The existing ordinary-factory test is parameterized. No unrelated test or source changes were found.
No change lacks a concrete connection to #7532.
Full details: Docstring Coverage
Explanation
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 2 functions across 2 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
Finishing Touches
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.
A rabbit checks the ticking clock, A factory throws; the tests take stock. Timeout gets its proper name, External tokens stay the same. Independent faults remain in sight, Then hops away into the night.
Recent review info
Run configuration
Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: d93ee23e-e4dd-49d0-aa9a-a5af4c0c149a
Commits
Reviewing files that changed from the base of the PR and between be37704 and 6f41dc9.
Files selected for processing: 2
src/TUnit.Engine/Helpers/TimeoutHelper.cs
tests/TUnit.UnitTests/TimeoutHelperTests.cs
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 2 remain after this review.
The fix is correct and well scoped. I found no blocking issues.
What it does: the factory call is wrapped in a try/catch. The when filter matches the existing post-await catch exactly: timeoutCts.IsCancellationRequested && IsTimeoutCancellationToken(...). A synchronous timeout or linked-token cancellation becomes Task.FromException(exception). That sends it through the same path as a returned faulted task, so classification (external cancellation vs. timeout), the grace-period observation and IsRoutineCancellation diagnostic preservation all apply.
Behaviour checks:
An unrelated or tokenless OperationCanceledException fails the filter and propagates unchanged. The Synchronous_Independent_Cancellation_Is_Preserved test covers this with IsSameReferenceAs.
External cancellation still ends in throw new OperationCanceledException(externalToken), which the test checks.
The filter is evaluated before unwinding and the non-matching case isn't swallowed, so there is no new exception-hiding risk.
The faulted task is observed by the existing ContinueWith fault observer, so there are no unobserved-exception leaks.
Tests: coverage is good. It includes timeout, external token, independent and tokenless cancellation, and diagnostic preservation, plus the parameterised sync/async variant of the armed-before-invocation test.
Optional suggestion: the filter expression is now duplicated in two catch blocks. A small helper, e.g. IsTimeoutCancellation(OperationCanceledException, timeoutCts, testContext), would keep the two sites from drifting apart. This is non-blocking.
Both diagnostic-suppression findings are fixed. Claude’s optional helper extraction is left unchanged to keep this correction scoped; the two filters still match. Regression reproduced before the fix; all 30 TimeoutHelper tests pass afterward.
The fix is correct and scoped. I found no blocking issues.
A synchronous OperationCanceledException from the factory is now classified the same way as a returned canceled or faulted task. The when filter keeps unrelated or tokenless cancellation propagating unchanged.
Routine cancellation becomes Task.FromCanceled, and custom diagnostics stay faulted. This addresses the earlier diagnostic-suppression findings.
The tests cover timeout, external token, independent and tokenless cancellation, and diagnostic preservation.
Optional, non-blocking: the timeoutCts.IsCancellationRequested && IsTimeoutCancellationToken(...) condition now appears in two catch filters. A small shared helper would stop them drifting apart. The author already declined this to keep the PR scoped, which is reasonable.
Marked ready after Claude and Codex approved current head 6f41dc9 and all review threads were resolved. Promotion started Claude review run 41782814257 and CI. Auto-merge must be armed after that new Claude run succeeds and its review is read.
The fix looks correct and small. A synchronous OperationCanceledException thrown by the task factory is now turned into a task. That task goes through the existing timeout vs. external-cancellation classification, and no new control flow is added.
Filter: the when clause (timeoutCts.IsCancellationRequested plus IsTimeoutCancellationToken) leaves unrelated cancellations alone. They propagate unchanged, and the new tests cover that.
Routine vs. custom exceptions: routine cancellations become Task.FromCanceled, so no cancellation exception ends up in the TimeoutException. Custom exceptions use Task.FromException and keep their diagnostics. This matches the returned-task path, and Synchronous_Timeout_Cancellation_Preserves_Diagnostics checks it.
Tests: they cover timeout, external-token preservation, independent and tokenless cancellation, and diagnostic preservation. Parameterizing the existing armed-before-invocation test is a good reuse.
Optional: the catch block and the later catch (OperationCanceledException) at line ~137 both repeat the IsTimeoutCancellationToken filter. A small helper that returns a classified Task from a factory call could remove the duplication. It isn't blocking.
Approving in spirit. I found no correctness or AOT concerns.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
GitHub Actions
Inactive
Summary
A factory that synchronously throws after observing its timeout token now follows the same timeout/external-cancellation classification as a returned canceled task. The helper preserves custom cancellation diagnostics and leaves unrelated cancellation unchanged, while continuing to arm ordinary factory timeouts before invocation.
Added synchronous coverage for timeout, external-token preservation, independent and tokenless cancellation, and diagnostic preservation alongside the existing returned-task and scheduled-body cases.
Closes #7532
Validation
dotnet test --project tests/TUnit.UnitTests --framework net10.0 --treenode-filter '/*/*/TimeoutHelperTests/*': 30 passed with SDK 10.0.403 on macOS arm64.git diff --checkpassed.Summary by CodeRabbit
Bug Fixes
Tests