Fix LongPolling Teardown Race on Forbidden Cleanup DELETE - #68191
Draft
vendasankarsf3945 wants to merge 3 commits into
Draft
Fix LongPolling Teardown Race on Forbidden Cleanup DELETE#68191vendasankarsf3945 wants to merge 3 commits into
vendasankarsf3945 wants to merge 3 commits into
Conversation
…into 68149-longpolling-forbidden-delete
vendasankarsf3945
requested review from
BrennanConroy and
halter73
as code owners
August 4, 2026 07:58
dotnet-policy-service
Bot
added
the
community-contribution
Contributor
|
Thanks for your PR, @vendasankarsf3945. Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request stabilizes SignalR client LongPolling test teardown by treating 403 Forbidden responses to the LongPolling cleanup DELETE as a benign “already closed” outcome (similar to the existing 404 NotFound handling), and removes test quarantine now that the scenario is reliable.
Changes:
- Updated
LongPollingTransport.SendDeleteRequestto treat403 Forbiddenas an already-closed cleanup response. - Added a unit test validating that
403during cleanupDELETEdoes not surface as a failure. - Removed the quarantine attribute from
RefreshChangingUserIdentifierClosesConnection.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/SignalR/clients/csharp/Http.Connections.Client/src/Internal/LongPollingTransport.cs | Extends benign cleanup handling to include 403 Forbidden during teardown DELETE. |
| src/SignalR/clients/csharp/Client/test/UnitTests/LongPollingTransportTests.cs | Adds coverage ensuring 403 cleanup DELETE is treated as already-closed and does not fail Stop/teardown. |
| src/SignalR/clients/csharp/Client/test/FunctionalTests/HubConnectionTests.AuthenticationRefresh.cs | Removes [QuarantinedTest] from the stabilized auth-refresh/user-identifier test. |
Comment on lines
233
to
+237
| var request = new HttpRequestMessage(HttpMethod.Delete, url); | ||
|
|
||
| var response = await _httpClient.SendAsync(request).ConfigureAwait(false); | ||
|
|
||
| if (response.StatusCode == HttpStatusCode.NotFound) | ||
| if (response.StatusCode == HttpStatusCode.NotFound || response.StatusCode == HttpStatusCode.Forbidden) |
Comment on lines
+237
to
240
| if (response.StatusCode == HttpStatusCode.NotFound || response.StatusCode == HttpStatusCode.Forbidden) | ||
| { | ||
| Log.ConnectionAlreadyClosedSendingDeleteRequest(_logger, url); | ||
| } |
Member
|
Not sure we care about hiding 403. Fixing the test in #68096 |
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.
Fix LongPolling Teardown Race on Forbidden Cleanup DELETE
Description
This PR fixes a flaky teardown race in
HubConnectionTests.RefreshChangingUserIdentifierClosesConnectionthat caused intermittent test failures in the LongPolling transport variant.When an authentication refresh changes the SignalR
UserIdentifier, the server immediately closes the connection. During client shutdown, the LongPolling transport sends a cleanup DELETE request to the server endpoint. Due to the race between server-side connection invalidation and client-side teardown, the server can return403 Forbiddenon that DELETE. Previously, only404 Not Foundwas treated as a benign "already closed" response — any other non-success status, including403, would surface as anHttpRequestExceptionand fail the test teardown even though the connection had been closed correctly.The fix extends
SendDeleteRequestinLongPollingTransportto also treat403 Forbiddenas a graceful already-closed outcome, and removes the[QuarantinedTest]attribute from the now-stable test.Validation / Investigation
RefreshChangingUserIdentifierClosesConnectiontheory — WebSockets and ServerSentEvents are not affected.UserIdentifier, the long-polling session is invalidated server-side before the client has a chance to send its cleanup DELETE.403 Forbiddenfor the stale DELETE request; the client was not handling this response code gracefully.VerifyNoErrorsScope.Dispose()intoInProcessTestServer.DisposeAsync(), causing a false failure.Changes
SendDeleteRequestinLongPollingTransport.csto treat403 Forbidden— in addition to404 Not Found— as a benign "connection already closed" response.LongPollingTransportTreatsForbiddenDeleteAsAlreadyClosedtoLongPollingTransportTests.csto lock in this behavior.[QuarantinedTest("https://github.com/dotnet/aspnetcore/issues/68149")]attribute fromRefreshChangingUserIdentifierClosesConnection.Fixes #68149.