fix: Close the demoted anchor's SDK client when a re-anchor commits#744
Open
keelerm84 wants to merge 3 commits into
Open
fix: Close the demoted anchor's SDK client when a re-anchor commits#744keelerm84 wants to merge 3 commits into
keelerm84 wants to merge 3 commits into
Conversation
A grace-demoted previous anchor stayed accepted with a future expiry, so removeCredential never fired for it at rotation time. Its SDK client kept its upstream stream open alongside the new anchor's client, and because both clients share the handed-over store wrapper (which broadcasts every Upsert), each flag update was sent twice to all connected downstream clients until the demoted key's expiry finally closed the old client. reanchor now closes and removes the previous anchor's client as soon as CommitAnchor lands. Only the client goes: the demoted key's credential mappings remain registered, so it still authenticates downstream connections for the rest of its grace period. The refcounted store wrapper survives the close because the new anchor's client holds it. Consequence: re-promoting an in-grace key back to anchor now builds a fresh client instead of reusing a retained one, since demotion tears the old client down. Tests asserting the retained-client behavior were updated to assert the new invariant (a single upstream client per environment at all times).
Exercises the real rotation path with real SDK clients against a fake LaunchDarkly streaming service: an auto-config patch rotates the anchor key while demoting the old key with a grace expiry, and a downstream SSE client (authenticated with the old key) stays connected throughout. Asserts the rotation opens a new upstream connection with the new key, closes the demoted key's client once the new anchor commits, and delivers exactly one copy of a subsequent upstream flag update to the connected downstream client. Verified against the pre-fix code: it fails there both on the unclosed client and, with that check removed, on the duplicated downstream update.
The demoted-anchor client close added at re-anchor commit lacked the offline guard that removeCredential has. An offline env has exactly one file-data SDK client and the offline re-anchor branch builds no replacement, so closing the previous anchor's client left the env with no client at all: GetClient() returned nil, /status reported the env disconnected, and with a persistent store the wrapper refcount hit zero and tore down the backing store. Mirror removeCredential: skip the client teardown when offline. The offline re-anchor still commits the anchor; the single client keeps serving across rotations. Found by multi-agent review (general, M1).
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.
A grace-demoted previous anchor stayed accepted with a future expiry, so
removeCredential never fired for it at rotation time. Its SDK client kept
its upstream stream open alongside the new anchor's client, and because
both clients share the handed-over store wrapper (which broadcasts every
Upsert), each flag update was sent twice to all connected downstream
clients until the demoted key's expiry finally closed the old client.
reanchor now closes and removes the previous anchor's client as soon as
CommitAnchor lands. Only the client goes: the demoted key's credential
mappings remain registered, so it still authenticates downstream
connections for the rest of its grace period. The refcounted store
wrapper survives the close because the new anchor's client holds it.
Consequence: re-promoting an in-grace key back to anchor now builds a
fresh client instead of reusing a retained one, since demotion tears the
old client down. Tests asserting the retained-client behavior were
updated to assert the new invariant (a single upstream client per
environment at all times).
Note
Medium Risk
Touches credential rotation and upstream client lifecycle in
reanchor; behavior change is intentional but affects all SDK key rotations and offline is explicitly exempted.Overview
Fixes double-broadcast of flag updates during SDK key rotation when the old anchor remains accepted on a grace expiry.
reanchornow closes and removes the previous anchor’s upstream SDK client immediately after a successfulcommitReanchor. The demoted key’s credential mappings stay so downstream auth still works until expiry; only the extra upstream stream is torn down so the shared store wrapper is not fed by two clients. Offline envs skip this teardown so the single file-data client keeps serving.Re-promoting a grace-period key back to anchor builds a new client (the demoted client is already gone), not reuse of a retained client.
Tests and auto-config coverage were updated for the new invariant, plus an end-to-end test with real SDK clients asserting one patch per upstream update after rotation.
Reviewed by Cursor Bugbot for commit 1c4b10d. Bugbot is set up for automated code reviews on this repo. Configure here.