[thomhurst]

Summary

Before: TimeoutHelperTests.Timeout_Preserves_Custom_Task_Cancellation(False) (and the other executionCompletesFirst: false cases) failed intermittently on loaded CI runners with Expected to be of type TaskCanceledException but Value is of type null.
After: the released operation completes on the releasing thread before the test awaits TimeoutHelper, so the result no longer depends on how busy the thread pool is.

Cause

Timeout detection was already deterministic. The factory cancels timeoutCts synchronously, and Task.WhenAny returns the already-completed detection task. The race was in the step after that. TimeoutHelper then waits up to TimeoutGracePeriod (1s) for the execution task so it can capture the exception to preserve. The test released the operation through a RunContinuationsAsynchronously TCS, which queued the continuation on the thread pool. When the pool was saturated, the grace-period timer fired before that work item ran. ObserveExceptionDuringGracePeriodAsync then found the task still incomplete and returned null. The CI failure's 3.3s duration is consistent with this.

I reproduced it locally by queuing 16x min-threads blocking work items and calling SetResult from a non-pool thread. That gave the identical assertion failure.

Fix

Test-only change in ExecuteWithControlledTimeoutAsync:

• The release TCS now runs continuations inline, and the awaits in CompleteAfterReleaseAsync use ConfigureAwait(false). SetResult() therefore runs the operation, and every operation in this file completes synchronously, before it returns.
• An explicit guard throws if the released operation has not completed after SetResult(). A future non-synchronous operation then fails clearly instead of flaking.

There are no timeout increases or retries.

Tests run

• With the starvation repro left in, the old code failed and the fixed code passed all 19 TimeoutHelperTests.
• With the repro removed, TimeoutHelperTests passed 19/19 on net10.0 (3 runs) and on net8.0.
• The full TUnit.UnitTests suite on net10.0 had 0 failures (612 total, 608 passed, 4 skipped).

Fixes #7172

Summary by CodeRabbit

• Tests
  • Improved validation of controlled timeout behavior, including synchronous completion after release and delayed execution scenarios.
  • Added safeguards to detect unexpected asynchronous completion during timeout handling.

[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-20T05:09:32.994108Z	0216241	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 16 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: 224eaf2d-0457-4983-abee-297cb9255a7d

📥 Commits

Reviewing files that changed from the base of the PR and between 0216241 and 8445026.

📒 Files selected for processing: 1

• tests/TUnit.UnitTests/TimeoutHelperTests.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: 9a654f0d-0662-4592-9b42-73e66d269e39

📥 Commits

Reviewing files that changed from the base of the PR and between c4b9977 and 0216241.

📒 Files selected for processing: 1

• tests/TUnit.UnitTests/TimeoutHelperTests.cs

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

📝 Walkthrough

The timeout test helper now uses inline task continuations, checks that delayed execution completes synchronously after release, and throws InvalidOperationException if it does not. The release helper awaits its signal without capturing the synchronization context.

Changes: Timeout test helper

Layer / File(s)	Summary
Controlled operation release tests/TUnit.UnitTests/TimeoutHelperTests.cs	The helper uses inline continuations and checks that the released task completes synchronously. The release helper awaits the completion signal with ConfigureAwait(false).

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Other · Severity of issue fixed: Low

Suggested reviewers: nice3point

Merge Risk: ⚪ Minimal · up to 02162

No actionable merge-blocking risk remains for this test-only synchronization change.

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 1 functions across 1 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: improving TimeoutHelperTests determinism under thread-pool load.
Linked Issues check		Issue #7172 requires deterministic timeout tests. In ExecuteWithControlledTimeoutAsync, timeoutCts.Cancel() completes timeout detection synchronously. The delayed execution remains blocked until `…
Out of Scope Changes check		The diff changes only tests/TUnit.UnitTests/TimeoutHelperTests.cs. The changes adjust controlled test scheduling and add a guard for the linked timeout test scenario in issue #7172. No unrelated pro…

Full details: Linked Issues check

Explanation

Issue #7172 requires deterministic timeout tests. In ExecuteWithControlledTimeoutAsync, timeoutCts.Cancel() completes timeout detection synchronously. The delayed execution remains blocked until releaseOperation.SetResult(). The release TCS now runs continuations inline, and both awaits use ConfigureAwait(false), so the operation completes before TimeoutHelper observes the grace-period result. The completion guard fails clearly if a future operation is not synchronous. The change is limited to the requested test helper.

Full details: Out of Scope Changes check

Explanation

The diff changes only tests/TUnit.UnitTests/TimeoutHelperTests.cs. The changes adjust controlled test scheduling and add a guard for the linked timeout test scenario in issue #7172. No unrelated product code, timeout value, retry, or public API change is present.

✨ 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

_{A rabbit checks the timeout’s pace,
Then lets the waiting task take place.
The signal travels, context free,
The released task completes with glee.
No carrots lost along the way.}

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

[greptile-apps]

Confidence Score: 5/5

[Low impact] The PR appears safe to merge; the stronger completion guard matches the test helper’s current callers.

Summary

Makes TimeoutHelperTests less dependent on thread-pool availability.

• Timeout tests release operations before checking their outcome.

_{Reviews (2) · Last reviewed commit: "test: fail the timeout helper guard if t..." · Reviewed by Greptile}

[github-actions]

Review

This is a test-only fix, and it looks correct. It removes the flake at its cause and doesn't add timeouts or retries.

• Cause: with RunContinuationsAsynchronously, a saturated thread pool could delay the released operation past TimeoutGracePeriod. TimeoutHelper would then return null for the exception to preserve. Running the continuation inline on SetResult() makes the result independent of pool load.
• ConfigureAwait(false): it's needed so the continuations after the awaits don't hop to a captured context. That keeps completion synchronous.
• Guard: the InvalidOperationException makes a future non-synchronous operation fail with a clear message instead of flaking.
• Minor, optional: releasedExecution stays null if the factory is never invoked synchronously, and the guard then passes silently. That's harmless here because timeoutTask would hang or fail visibly. You could assert releasedExecution is not null in the !executionCompletesFirst case, to make the guard stricter.

I found no blocking issues. Approving from my side.

[thomhurst]

Dispositions for the review findings (there are no inline threads):

• github-actions (optional: make the guard stricter, because releasedExecution stays null if the factory is never invoked synchronously): fixed in 8445026. In the !executionCompletesFirst case the guard now requires the released operation to exist and to have completed, so a factory that was not invoked synchronously fails with a clear message instead of passing silently.
• Codex, CodeRabbit and Greptile: no findings.

Local check (net10.0): TimeoutHelperTests passed 19/19 in 10 of 10 runs before this change and 10 of 10 runs after it.

[github-actions]

Review

Test-only change. It looks correct and I have no blocking concerns.

• Root cause: the diagnosis fits the code. The old RunContinuationsAsynchronously TCS queued the release continuation on the thread pool. Under load, the 1s grace period could expire before that work item ran, so ObserveExceptionDuringGracePeriodAsync returned null. Running the continuation inline on SetResult() removes the scheduler dependency. It does this without adding timeouts or retries.
• Guard: the releasedExecution is not { IsCompleted: true } check turns a future non-synchronous operation into a clear failure instead of a flake. It also catches TimeoutHelper ceasing to invoke the factory synchronously.
• ConfigureAwait(false): this is harmless here. It keeps the continuation from being marshalled back to a sync context.

Optional: the inline-continuation TCS is easy to change back to async by accident. The comment explains why it must stay inline, which is enough, so I'd leave it.

LGTM.