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
fix(aspire): preserve caller cancellation over startup cleanup faults - #7447
Caller cancellation could surface a losing startup wait's cleanup fault instead of OperationCanceledException. Linked cancellation reaches all waits, so a cleanup fault can win the task race before a cancelled wait's continuation runs. This also fails unrelated PR CI, including #7400 (run 41566483900).
Check the caller token after the startup race settles, before reading its result. Caller cancellation takes precedence at that point; the existing finally block still cancels and joins every wait and observes cleanup faults. No public API changes.
Add a deterministic regression where cancellation and faulted waits are present before the race is consumed. It fails against the original waiter with the cleanup InvalidOperationException and passes with this fix.
dotnet run --project tests/TUnit.Aspire.Tests --framework net10.0 -- --treenode-filter '/*/*/ResourceStartupWaiterTests/*': 11 passed, zero failed.
git diff --check passed.
Existing build warnings are tracked by #7410. Full Aspire integration validation is left to CI; this targeted run covers startup outcome and cleanup behavior.
Summary by CodeRabbit
Bug Fixes
Startup waits now honor caller cancellation when cancellation and wait failures occur together.
No actionable comments were generated in the recent review.
Walkthrough
ResourceStartupWaiter.WaitAsync now gives caller cancellation precedence after the readiness/failure race completes. A test verifies that it throws OperationCanceledException with the caller’s token when both waits fault.
Changes: Caller cancellation outcome
Layer / File(s)
Summary
Cancellation precedence and validation src/TUnit.Aspire.Core/ResourceStartupWaiter.cs, tests/TUnit.Aspire.Tests/ResourceStartupWaiterTests.cs
WaitAsync checks the caller’s token before processing the race result. The test verifies that caller cancellation is thrown with the caller’s token when both waits fault.
thomhurst/TUnit#7279: This PR introduced ResourceStartupWaiter.WaitAsync and its readiness/failure race, which this change updates.
Merge Risk: Minimal · up to fa0fa
The startup cancellation behavior is covered by the regression test, with no actionable merge-blocking risk identified.
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 names the main change: caller cancellation now takes precedence over faults raised by startup cleanup in the startup waiter. The raw summary confirms that WaitAsync checks the caller's cance…
Linked Issues check
Directly linked issue #7282 requires that caller cancellation surface OperationCanceledException when a losing wait faults during cleanup. At head fa0fa9f, ResourceStartupWaiter.WaitAsync calls cancel…
Out of Scope Changes check
The whole-PR diff against merge base 87d2a30 changes two files: ResourceStartupWaiter.cs (+3 lines, one ThrowIfCancellationRequested check with a comment) and ResourceStartupWaiterTests.cs (+23 lines,…
Full details: Title check
Explanation
The title names the main change: caller cancellation now takes precedence over faults raised by startup cleanup in the startup waiter. The raw summary confirms that WaitAsync checks the caller's cancellation token after the readiness/failure race and before reading its result, and that a regression test covers faulted waits. The title is specific, uses the conventional fix(aspire) prefix, and fits the changeset. It is 69 characters, within normal length limits.
Full details: Linked Issues check
Explanation
Directly linked issue #7282 requires that caller cancellation surface OperationCanceledException when a losing wait faults during cleanup. At head fa0fa9f, ResourceStartupWaiter.WaitAsync calls cancellationToken.ThrowIfCancellationRequested() immediately after await Task.WhenAny(readyTask, anyFailureTask) and before the result is read (src/TUnit.Aspire.Core/ResourceStartupWaiter.cs, lines 27-34). A cancelled caller therefore throws OperationCanceledException with the caller token, not the losing wait's InvalidOperationException. The existing finally block still cancels and joins all waits and swallows their faults, so the race outcome remains owned by the waiter. The new test CallerCancellation_TakesPrecedenceOverAnAlreadyFaultedWait (tests/TUnit.Aspire.Tests/ResourceStartupWaiterTests.cs) cancels during setup, supplies faulted ready and failure waits, and asserts OperationCanceledException whose CancellationToken equals the caller token. Both faulted tasks are already complete when WhenAny runs, so the test exercises the reported race without timing dependence. The PR reports that the test failed before the fix; I did not execute it. Residual note: if the caller cancels after all waits succeed, the method now throws OperationCanceledException where it previously returned null. This follows the caller-precedence rule and does not conflict with #7282.
Full details: Out of Scope Changes check
Explanation
The whole-PR diff against merge base 87d2a30 changes two files: ResourceStartupWaiter.cs (+3 lines, one ThrowIfCancellationRequested check with a comment) and ResourceStartupWaiterTests.cs (+23 lines, one regression test). Both changes directly implement the #7282 objective. The PR description's CI, promotion, and auto-merge details describe process, not code, and introduce no repository changes. No public API changes are present, and the diff shows none. No unrelated code change was found.
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 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
Warning
Some tools did not complete. Review the errors below.
ast-grep (0.51.1)
ast-grep could not parse rule config: /ast-grep-rules/ast-grep-essentials/rules/typescript/security/insecure-websocket-typescript.yml
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 waits beside the gate The caller’s token sets the state Two waits may fault; the check comes first Cancellation answers, not the worst The test confirms the token’s name And checks the outcome all the same
CodeRabbit Verify
Run CodeRabbit Verify to test the changes in this pull request.
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: a2ca663a-517e-454c-a24e-5cc9c79c36b2
Commits
Reviewing files that changed from the base of the PR and between 87d2a30 and fa0fa9f.
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 5 remain after this review.
The fix is small and targets the right spot. ThrowIfCancellationRequested() runs after the WhenAny race settles and before any result is read. That stops a faulted cleanup wait from replacing the caller's cancellation. The finally block still joins every wait. The regression test is deterministic: it cancels inside the fault factory, so the fault is already in place when the race is consumed.
Minor observations (non-blocking):
Cancellation now wins over a real outcome. If the caller cancels after readiness or a real failure has already completed but before the continuation resumes, the method throws OperationCanceledException and drops that result. For startup waiting, treating cancellation as authoritative is reasonable and matches the PR's stated intent. A short comment saying this is intentional would help future readers.
Alternative design. The root cause is that the waits' faults and cancellations race each other. You could instead wrap waitForReady and waitForFailure to swallow or normalise exceptions raised while waitsCts.IsCancellationRequested is set. That fixes it at the source, but it is more code and could hide real faults. The current one-line check is simpler and I prefer it.
Test.Failure(string name, CancellationToken token) has unused parameters. That is fine, but a discard-style lambda would be a bit tidier.
No correctness issues found. Looks good to merge once CI is green.
Rechecked every review surface at fa0fa9f: Claude and Codex both report no blocking findings; there are no review threads. Claude's cancellation precedence observation matches the stated contract and existing regression name; the source comment already explains caller cancellation wins. The alternative normalization and unused-parameter suggestions are optional and do not warrant expanding this fix.
Promoting this unchanged head to run full CI and unblock #7400 via #7282. The ready-for-review Claude run must pass and its review be read before auto-merge is armed. No source changes or local tests in this review-only unit.
Promotion confirmed with head unchanged at fa0fa9f. Waiting for ready-for-review Claude run 41581794658. Full CI has started. Next unit: read this promotion review, refresh all review surfaces, and arm auto-merge with scripts/Enable-AutoMerge.ps1 if the gate passes. Auto-merge remains unarmed until that review passes. Once this PR merges and closes #7282, refresh #7400 onto main for fresh CI.
The fix is small and correct. Checking cancellationToken after Task.WhenAny settles and before the result is read means caller cancellation wins over a cleanup fault from a losing wait. The finally block still cancels and joins every wait, so no faults go unobserved. The regression test is deterministic: cancellation happens during setup and both waits are already faulted when the race is consumed.
Minor notes, none blocking:
Late cancellation: if a wait completes successfully and the caller cancels just before the check, the method now throws OperationCanceledException for a startup that finished. That is reasonable, since the caller asked to cancel, and the old code had a similar race. A short comment saying this is intentional would help.
Test:Failure takes name and token parameters it doesn't use. The Cancel() side effect in the factory is a slightly indirect way to set up the scenario, but the existing comment explains it well enough.
Promotion review run 41581794658 passed. Re-read reviews, inline comments, all conversation comments, and review threads: Claude and Codex approve current head fa0fa9f; no unresolved or blocking findings. The non-blocking notes remain dispositioned in the earlier comment.
scripts/Enable-AutoMerge.ps1 passed and armed squash auto-merge pinned to this head. Waiting for full CI, including .NET run 41581793924. No code changes or local tests in this unit. After this merges and closes #7282, refresh #7400 onto main for fresh CI. The earlier review-run blocker has cleared; no on-hold label is needed.
Run 41581793924 is still active, but its Windows job 125738087455 has already failed on the known #7325 polling-throughput flake, independently of the Aspire change. Implemented #7325 in #7448; all 27 polling tests pass locally. After #7448 merges, refresh this PR onto main for fresh CI. The existing current-head Claude and Codex verdicts have no blocking findings. No rerun requested: fix the diagnosed cause first.
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
Inactive
Summary
Caller cancellation could surface a losing startup wait's cleanup fault instead of
OperationCanceledException. Linked cancellation reaches all waits, so a cleanup fault can win the task race before a cancelled wait's continuation runs. This also fails unrelated PR CI, including #7400 (run 41566483900).Check the caller token after the startup race settles, before reading its result. Caller cancellation takes precedence at that point; the existing finally block still cancels and joins every wait and observes cleanup faults. No public API changes.
Add a deterministic regression where cancellation and faulted waits are present before the race is consumed. It fails against the original waiter with the cleanup
InvalidOperationExceptionand passes with this fix.Closes #7282
Validation
.NET SDK 10.0.403, macOS arm64:
dotnet run --project tests/TUnit.Aspire.Tests --framework net10.0 -- --treenode-filter '/*/*/ResourceStartupWaiterTests/*': 11 passed, zero failed.git diff --checkpassed.Existing build warnings are tracked by #7410. Full Aspire integration validation is left to CI; this targeted run covers startup outcome and cleanup behavior.
Summary by CodeRabbit