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
{{ message }}
Repository navigation
Make duplicate-stop cancellation regression independent of thread-pool scheduling - #7546
The duplicate-stop regression used two thread-pool tasks while its first caller synchronously blocked a worker. Run both callers on dedicated threads so thread-pool scheduling latency cannot consume the cancellation deadline. Keep the startup handshake and verify cancellation before releasing the inner stop, with a five-second failure guard matching the startup guard.
Always release the inner stop and drain both callers before disposing their synchronization gates, including when startup or the assertion fails.
.NET SDK 10.0.403, macOS arm64: dotnet test --project tests/TUnit.AspNetCore.Tests --framework net10.0 --treenode-filter "/*/*/HostedServiceStopOnceTests/*" — all 8 tests passed.
Temporarily invoked the inner stop under the wrapper lock: the targeted regression failed with the expected timeout and exited cleanly. Restored production code and reran all 8 tests successfully.
Windows CI load was not reproduced locally; Windows validation remains for CI. No production code changes.
Summary by CodeRabbit
Tests
Improved coverage for concurrent stop requests during synchronous startup, including cancellation handling and ensuring the stop operation runs only once.
Review was skipped as selected files did not have any reviewable changes.
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: 5ece1c4f-5915-410f-8dd7-a49f2e04fe99
Commits
Reviewing files that changed from the base of the PR and between 0d3ecf3 and f5ea962.
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.
Walkthrough
The hosted-service stop test now runs both callers on dedicated long-running tasks. It waits up to five seconds for the duplicate caller’s cancellation, then releases the blocked stop and drains both callers before disposing synchronization gates.
Changes: Hosted-service stop cancellation test
Layer / File(s)
Summary
Caller scheduling and cleanup tests/TUnit.AspNetCore.Tests/HostedServiceStopOnceTests.cs
Both stop callers use dedicated long-running tasks. The test waits up to five seconds for duplicate-call cancellation, releases the blocked stop, and drains both callers. It continues to verify that the inner stop runs once.
The change is mergeable after normal CI validation, including Windows.
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: making the duplicate-stop cancellation regression independent of thread-pool scheduling.
Linked Issues check
Issue #7506 requires a cancellation regression for Duplicate_Stop_Caller_Does_Not_Wait_For_Synchronous_Startup that does not depend on thread-pool scheduling latency, without weakening the cancellat…
Out of Scope Changes check
The only changed file is tests/TUnit.AspNetCore.Tests/HostedServiceStopOnceTests.cs, and the edits are confined to the body of Duplicate_Stop_Caller_Does_Not_Wait_For_Synchronous_Startup. The chan…
Full details: Linked Issues check
Explanation
Issue #7506 requires a cancellation regression for Duplicate_Stop_Caller_Does_Not_Wait_For_Synchronous_Startup that does not depend on thread-pool scheduling latency, without weakening the cancellation guarantee. The diff in HostedServiceStopOnceTests.cs (merge base to head) covers this. (1) Both stop callers now run through Task.Factory.StartNew(..., TaskCreationOptions.LongRunning, TaskScheduler.Default).Unwrap(), which uses dedicated threads instead of Task.Run. (2) The startup handshake is kept: inner.WaitUntilEntered() still runs before the second caller starts. (3) The duplicate caller is checked with an already-cancelled token, and the wait guard is raised from 1 to 5 seconds. The assertion still requires OperationCanceledException. (4) inner.StopCalls is still asserted equal to 1. The PR changes only the test. The production wrapper was not inspected directly. The author reports that a temporary mutation, which invoked the inner stop under the wrapper lock, made the regression fail with the expected timeout, so the test detects the regused behavior. The residual wall-clock guard is a timeout, so the test is not fully free of timing. This guard is the same kind of guard the issue accepts, and the change removes the thread-pool dependency the issue identifies. Windows CI remains the final check under load.
Full details: Out of Scope Changes check
Explanation
The only changed file is tests/TUnit.AspNetCore.Tests/HostedServiceStopOnceTests.cs, and the edits are confined to the body of Duplicate_Stop_Caller_Does_Not_Wait_For_Synchronous_Startup. The changes are: dedicated-thread task creation for both callers, a 5-second guard, and a finally block that releases the inner stop, drains both callers, and then allows disposal. Each change serves issue #7506 directly. No production code, unrelated tests, or project files changed. The PR description states that no production code changes were made, and the diff agrees with that statement.
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 1 functions across 1 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 stop-call queue, One waits while the other’s token rings through. On long-running threads, both callers depart, The blocked stop is released with care from the start. The inner stop runs once, then the gates rest, And the rabbit hops off, pleased with the test.
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: 3b4912e8-24c6-486a-9186-92980ce41234
Commits
Reviewing files that changed from the base of the PR and between 10a0af7 and 0d3ecf3.
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 0 remain after this review.
WaitUntilEntered() has its own 5s timeout and now sits inside the try. A startup failure still reaches finally, which releases the inner stop and drains first. The gates are not disposed while a caller is still running.
second is null-checked and its cancellation is swallowed only when cancellation was actually requested. That keeps the drain from hiding real errors.
The 5s assertion timeout matches the startup guard. It is only a failure ceiling, because the cancelled caller should return immediately.
Minor, non-blocking: if first faults in the finally, that exception can mask the original assertion failure. That is acceptable for a test.
No blocking issues. Windows CI remains the real validation, as the description notes.
Marked ready after Claude and Codex both returned no-blocker verdicts for 0d3ecf3, with no review threads. Claude's minor exception-masking observation is accepted as non-blocking for this test; no code change is needed.
Promotion started Claude review run 41763438515 and the ready-for-review CI workflows. Auto-merge remains unarmed until the promotion review passes and its comment is checked, as required by the merge gate.
Test-only change, and it looks good. The fix targets the cause of the flake. The old test ran both callers on thread-pool tasks while the first caller blocked a worker synchronously. Under CI load the 1s deadline could then be consumed by scheduling latency. Using LongRunning on TaskScheduler.Default gives each caller its own thread. The 5s guard matches the startup guard.
Moving WaitUntilEntered inside the try is correct. finally now always releases the inner stop and drains both callers before the gates are disposed, even if the handshake or assertion fails.
Observation: if the assertion fails and first also faults, await first in finally can replace the original assertion failure with the caller's exception. This is unlikely and fine for a test. Wrapping the drain in a try/catch that preserves the primary failure would make diagnosis a little easier.
A small helper such as RunOnDedicatedThread(Func<Task>) would remove the duplicated Task.Factory.StartNew(...).Unwrap() boilerplate. This is optional.
I did not run the tests. Windows CI is the real check for the flake. No production code changes, so there are no AOT or generator concerns. Approving.
The prerequisite is cleared: #7549 merged as 4891fce and its .NET run 41766395044 succeeded on all three operating systems. Rebased this PR onto that main commit and pushed f5ea962, preserving the original test-only diff.
Validation after rebase: SDK 10.0.403, macOS arm64, targeted HostedServiceStopOnceTests on net10.0: 8 passed, 0 failed. git diff --check passed. Existing build warnings remain tracked by #7410 and #7438. Fresh CI and reviews must validate the new head; the old snapshot failures are no longer a blocker. No CI rerun was used and auto-merge remains unarmed pending current-head review.
Review: LGTM. Test-only change that makes the duplicate-stop regression robust against thread-pool starvation.
Both callers now run on dedicated LongRunning threads, so the 5s cancellation deadline no longer depends on pool scheduling.
WaitUntilEntered has a 5s guard and now sits inside the try, so a startup failure still reaches finally.
finally always calls inner.Release() and drains both callers before the gates are disposed. Swallowing OperationCanceledException for second is correct, because that exception is the expected result.
await first in finally could in theory mask an assertion failure if first throws. That seems unlikely here and is acceptable.
Minor, non-blocking: wait on the second result with a single helper shared by the assertion and the drain. That would remove the duplicated try/catch. It isn't needed for this PR.
I did not run the tests. The PR reports 8/8 passing locally and says Windows validation is left to CI.
Final disposition for f5ea962: #7549 is merged and run 41766395044 succeeded. All 29 current checks pass, including Windows, macOS, Linux, and AOT. Claude and Codex both returned positive verdicts for this head; Greptile found no actionable issues. There are no inline comments or review threads.
Claude's optional helper suggestions do not require changes: the two explicit task launches and the separate assertion/drain paths keep this regression self-contained. The possible cleanup exception masking is accepted as non-blocking for this test, as the reviewer states. CodeRabbit's docstring-coverage warning is also non-blocking: this change is confined to an existing, descriptively named test with comments explaining its synchronization and cleanup. It introduces no undocumented public API.
The fresh Assert-PrGreen gate passed. Proceeding with the pinned squash merge.
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.
GitHub Actions
Failure
Summary
The duplicate-stop regression used two thread-pool tasks while its first caller synchronously blocked a worker. Run both callers on dedicated threads so thread-pool scheduling latency cannot consume the cancellation deadline. Keep the startup handshake and verify cancellation before releasing the inner stop, with a five-second failure guard matching the startup guard.
Always release the inner stop and drain both callers before disposing their synchronization gates, including when startup or the assertion fails.
Closes #7506
Validation
dotnet test --project tests/TUnit.AspNetCore.Tests --framework net10.0 --treenode-filter "/*/*/HostedServiceStopOnceTests/*"— all 8 tests passed.Summary by CodeRabbit