fix(async): honor reset_connection when AsyncMilvusClient rotates a password - #3756
Open
Anai-Guo wants to merge 1 commit into
Open
fix(async): honor reset_connection when AsyncMilvusClient rotates a password#3756Anai-Guo wants to merge 1 commit into
Anai-Guo wants to merge 1 commit into
Conversation
AsyncMilvusClient.update_password had no reset_connection flag, so an async client that changed its own password kept sending the old one on every later call. Adding the flag alone was not enough: the async channel appends authorization interceptors and never replaces them, so the superseded credential stayed installed -- and, being first, still won. _setup_authorization_interceptor now detaches the interceptor it is replacing, and _build_stub no longer re-appends one that is already installed (honoring its own "avoid to add duplicate headers" comment). Signed-off-by: Tai An <antai12232931@anaiguo.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Anai-Guo The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Tick the box to add this pull request to the merge queue (same as
|
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.
What
AsyncMilvusClient.update_password()has noreset_connectionflag, so an async client that rotates its own password keeps sending the old one on every subsequent call.The sync client has had this since the beginning:
AsyncGrpcHandlereven carries a mirrored_setup_authorization_interceptor()— but it has zero call sites; the async client never wires it up. So today the only way for an async caller to keep working afterupdate_password()is to throw the client away and build a new one.Why adding the flag alone isn't enough
Porting the sync two-liner verbatim does not work, because the two handlers install interceptors differently:
_setup_authorization_interceptor()only recordsself._authorization_interceptor;_setup_grpc_channel()then rebuilds_final_channelfrom the raw channel, so the old interceptor is dropped._setup_authorization_interceptor()appends to the live_final_channel._unary_unary_interceptors, and_build_stub()appends again. Nothing is ever removed.Each authorization interceptor appends its own
authorizationmetadata entry (async_header_adder_interceptor), so the superseded credential stays on the wire — and, being first in the list, is the one the server sees. Measured against the currentmasterwith a naive port of the sync code:(
_setup_grpc_channel()carries the comment# avoid to add duplicate headers.—_build_stubdoes not currently honor it.)What this PR changes
AsyncMilvusClient.update_password(..., reset_connection=False)— same semantics as the sync client.AsyncGrpcHandler._uninstall_authorization_interceptor()(new) — detaches the interceptor being replaced from the live channel;_setup_authorization_interceptor()calls it before installing the new one.AsyncGrpcHandler._build_stub()— no longer re-appends an authorization interceptor that is already installed on that channel, honoring its own comment. Building a stub on a fresh channel is unchanged.After the change, the same measurement:
Signature note:
reset_connectionis appended at the end of the async signature rather than placed afternew_passwordas in the sync client, so existing positional calls likeupdate_password(u, old, new, 30)keep bindingtimeout. Happy to mirror the sync ordering instead if you'd prefer exact positional parity.Tests
New in
tests/unit/async_grpc_handler/test_async_auth.py(5 tests): credential rotation replaces the stale interceptor and installs exactly one; the uninstall helper is a no-op when nothing is installed;reset_connection=Truere-authorizes the connection and the default leaves it alone; and the sync/async signatures carry the same parameter set.The rotation tests assert on the decoded
authorizationheaders the installed interceptors would emit, not just list length, so they fail loudly if a stale credential survives.Verification (Windows, Python 3.12, no server required —
grpc.aiochannels are constructed but never connected):new code + new tests →
174 passedintests/unit/async_grpc_handler/unmodified source + new tests →
5 failed, 169 passed, the key one beingNo existing test was modified or weakened. An earlier draft moved interceptor installation out of
_setup_authorization_interceptorand broketest_setup_authorization_interceptor_appends_header; the design in this PR keeps that method's contract intact (it still appends to the channel) and instead removes the interceptor it supersedes.Full unit suite:
3377 passed. The two unrelated failures on my machine (test_check.py::test_get_commit, plus collection errors intest_version.py/ the bulk-writer modules) are missing optional dev dependencies (setuptools_scm) and reproduce identically on unmodifiedmaster.ruff checkandruff format --checkclean on all three files (tests checked withtests/ruff.toml).This was found by diffing
MilvusClientagainstAsyncMilvusClientmethod signatures. The scan also surfaced two naming drifts I have deliberately not touched, since they may be intentional and changing them would be breaking:get_load_state(partition_name=)vs(partition_names=), andtransfer_replica(source_group, target_group, num_replicas)vs(source, target, num_replica). Happy to file those separately if they're unintended.🤖 Generated with Claude Code