Keep a replaced Azure DevOps session open until its last holder releases it [patch] - #315
Merged
Merged
Conversation
…releases it [patch] CredentialedSessionCache disposed the previous VssConnection as soon as the credentials changed, while requests that had already been handed it were still waiting out the rate-limit delay or mid-call. Set Credentials changes the account and then the token in two popups, so re-entering a PAT did this twice while polling continued, and the resulting ObjectDisposedException escaped MakeAzureDevOpsRequestAsync and faulted the update loop. Get now returns a lease. A session that is replaced, invalidated, or outlived by the cache is retired rather than disposed, and the last lease released on it disposes it. The Azure DevOps update paths hold their lease for the whole request with `using`. Fixes #298 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XkEHuDnqdEMrwMy3iu9Kyr
Each update path now changes by one line: it takes the lease with `using` and gets the session through an out parameter, so its existing null check and client calls are untouched. The provider paths cannot be driven from a test, because they build a real VssConnection, so keeping the change there to a single line per path keeps the new code's coverage above the quality gate. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XkEHuDnqdEMrwMy3iu9Kyr
|
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
1 participant
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.
Fixes #298
What was wrong
CredentialedSessionCache.Getdisposed the previousVssConnectionas soon as the credentials changed. Requests that had already been handed that session were still using it, either waiting out the rate-limit delay or partway through the HTTP call. The Set Credentials flow changes the account and then the token in two separate popups, so re-entering a PAT did this twice while polling carried on. TheObjectDisposedExceptionthen escapedMakeAzureDevOpsRequestAsyncand faulted the update loop.The fix
Sessions are now leased with a reference count, which is the first option the issue proposed.
Getreturns aCredentialedSessionCache<T>.Lease(IDisposable) instead of the bare session.Invalidated, or the cache itself is disposed. The last lease released on it disposes it, exactly once. Releasing the same lease twice does nothing.usingfor the whole request.Tests
ASessionHandedToACallerSurvivesARebuildBehindItnow also asserts that the held session is not disposed, which the issue asked for.ASessionReplacedWhileHeldIsDisposedOnlyWhenReleased: a deterministic version of the PAT re-entry path.AnInvalidatedSessionStaysOpenUntilItsLastHolderReleasesIt: two holders across anInvalidate.DisposeDisposesTheCachedSessionOncenow checks that disposal waits for an outstanding lease.I checked that the tests catch the bug by temporarily making retirement dispose immediately. Five tests failed, the four listed above plus
EverySupersededSessionIsDisposedExactlyOnce. After restoring the fix,dotnet testpasses 74 of 74.🤖 Generated with Claude Code
https://claude.ai/code/session_01XkEHuDnqdEMrwMy3iu9Kyr
Generated by Claude Code