Fix BrowserPageSession: use TimeProvider for delay, document ReferenceEquals, add timeout test#17228
Merged
Merged
Conversation
…eEquals, add timeout test - Replace Task.Delay with Task.Delay(TimeSpan, TimeProvider, CancellationToken) in the reconnect loop so that FakeTimeProvider can control the inter-attempt delay in tests. - Add a comment explaining why the ReferenceEquals check in DisposeAsync exists (future-proofing against refactors that may read _connection earlier or re-acquire the lock). - Add MonitorAsync_CompletesWithConnectionLostWhenReconnectTimesOut test that verifies the session completes with ConnectionLost when all reconnect attempts fail within the s_connectionRecoveryTimeout window. Co-authored-by: Copilot <[email protected]>
Contributor
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 17228Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 17228" |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates BrowserPageSession’s reconnect logic to be properly controllable under fake time in tests, adds a clarifying disposal comment, and introduces a new reconnection-timeout test case.
Changes:
- Switch reconnect backoff delay to
Task.Delay(..., TimeProvider, ...)soFakeTimeProvidercan drive reconnect timing deterministically. - Add an explanatory comment for the
ReferenceEqualscheck inDisposeAsync. - Add a test intended to validate that the session completes as
ConnectionLostwhen reconnect recovery times out.
Show a summary per file
| File | Description |
|---|---|
| src/Aspire.Hosting.Browsers/BrowserPageSession.cs | Use TimeProvider-based delay in reconnect loop; document ReferenceEquals disposal guard. |
| tests/Aspire.Hosting.Browsers.Tests/BrowserPageSessionTests.cs | Add a new test for reconnect-timeout behavior using FakeTimeProvider. |
Copilot's findings
- Files reviewed: 2/2 changed files
- Comments generated: 1
davidfowl
approved these changes
May 19, 2026
Use a concurrent task to advance the FakeTimeProvider clock so that Task.Delay timers fire and the reconnect loop iterates multiple times before the deadline is exceeded. Assert reconnectAttempts > 1 to verify the loop actually retried. Co-authored-by: Copilot <[email protected]>
Contributor
|
❌ CLI E2E Tests failed — 89 passed, 1 failed, 1 unknown (commit Failed Tests
View all recordings
📹 Recordings uploaded automatically from CI run #26069082870 |
Contributor
|
✅ No documentation update needed. docs_optional → |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Three fixes to
BrowserPageSessionand its tests:Use
TimeProviderfor reconnect delay — The reconnect loop usedTask.Delay(TimeSpan, CancellationToken)which bypasses the_timeProviderfield. Changed toTask.Delay(TimeSpan, TimeProvider, CancellationToken)so thatFakeTimeProvidercan control the inter-attempt delay in tests.Document the
ReferenceEqualscheck inDisposeAsync— Added a comment explaining why the seemingly redundant check exists: it future-proofs against refactors that may read_connectionearlier or release and re-acquire the lock before reaching that point.Add reconnect timeout exhaustion test — New test
MonitorAsync_CompletesWithConnectionLostWhenReconnectTimesOutverifies that when all reconnect attempts fail within the 5-second recovery window, the session completes withBrowserPageSessionCompletionKind.ConnectionLost. UsesFakeTimeProvider.AutoAdvanceAmountto fast-forward past the deadline without requiring manual timer pumping.