Conversation
Phase 3 of forward-porting concurrent multi-key support from v8 (#817). An environment now serves its whole accepted credential set, and moves its upstream connection between SDK keys by re-keying the one client it holds. ReconcileCredentials replaces UpdateCredential: it adds the incoming keys' mappings, re-anchors while the outgoing key still authenticates downstream traffic, then takes revoked mappings down. The re-anchor is LDClient.SetSDKKey plus the two components relay owns and the SDK cannot reach: the big segment synchronizer, which takes its key at construction and so is rebuilt, and the event publishers, which are repointed. There is no client to build, so there is nothing to roll back on a transient failure. SetSDKKey fails only on a key that is not valid in an HTTP header, which no retry fixes, so the environment parks on its current anchor and says so. Stream handlers are built per request behind a re-check of the accepted set. The map they used to live in was what made revocation racy: a handler stayed in it until the removal was processed, so a request that authenticated before a revocation still found a working handler. The REPORT stream endpoints let a client pace the body read that precedes the lookup, so that window is as long as the client wants. Removing a credential no longer closes the SDK client. The client is not tied to the key it was built with any more, so closing it there would tear down an environment's only upstream connection whenever any SDK key was revoked. The status resource's credential fields now come from one accepted-set snapshot, so they cannot disagree with each other, and neither they nor the metrics derived from them depend on map iteration order. Also pays off the deletions the previous phases deferred, because their callers live here: the single-key rotation shims, UpdateCredential and CredentialUpdate, and EnvironmentParams.ExpiringSDKKey with ExpiringKeyRep.ToParams. go.mod temporarily pins go-server-sdk#457 and go-sdk-events#63, which must become released versions before this merges.
It was written to check an assumption about the cost of building a handler per request rather than caching it, which it confirmed. The numbers are recorded in the comment on acceptsForStream; the benchmark itself is not worth carrying.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a96adce. Configure here.
| if err := client.SetSDKKey(string(anchor)); err != nil { | ||
| c.globalLogger.Error("could not apply the current SDK key to a newly built client", | ||
| "env", name, "error", err) | ||
| } |
There was a problem hiding this comment.
In-flight re-key failure not rolled back
Medium Severity
When a reconcile moves the anchor while the first client is still building, reanchor commits the new key without calling SetSDKKey. If the catch-up SetSDKKey in startSDKClient then fails, the rotator, event dispatcher, and big-segment sync already use the new key while the client keeps the old one, and nothing parks the environment on the previous anchor.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit a96adce. Configure here.
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.
Summary
Phase 3 of forward-porting concurrent multi-key support from v8 (#817) to v9. Stacked on #894, so this diff shows only phase 3; GitHub retargets it to
feat/concurrent-keys-v9as the stack merges.An environment now serves its whole accepted credential set, and moves its upstream connection between SDK keys by re-keying the one client it holds.
ReconcileCredentialsreplacesUpdateCredential: add the incoming keys' mappings, re-anchor while the outgoing key still authenticates downstream traffic, then take revoked mappings down.What a re-anchor is now
LDClient.SetSDKKey, plus the two components relay owns that the SDK cannot reach: the big segment synchronizer, which takes its key at construction and so is rebuilt, and the event publishers, which are repointed through the existingReplaceCredential.There is no client to build, so there is nothing to roll back on a transient failure.
SetSDKKeyfails only on a key that is not valid in an HTTP header, which no retry fixes, so the environment parks on its current anchor and logs loudly; the next auto-configuration payload supplies a new key. A reconcile can also move the anchor while the first client build is still in flight, sostartSDKClientre-keys the client on install if the anchor moved underneath it.The line worth reviewing closely
removeCredentialno longer closes the SDK client. The client is not tied to the key it was built with any more, so closing it there would tear down an environment's only upstream connection whenever any SDK key was revoked, including the environment's original key once it has been rotated away from.TestRotatingTheSDKKeyRekeysTheSameClientasserts the client survives both the rotation and the outgoing key's expiry.Stream handlers
Built per request, behind a re-check of the accepted set. The map they used to live in was what made revocation racy: a handler stayed in it until the removal was processed, so a request that authenticated before a revocation still found a working handler, and the REPORT stream endpoints let a client pace the body read that precedes the lookup.
The build is cheap enough to do per connect, and this is measured rather than assumed —
env_context_stream_handler_bench_test.goputs the client-side path at 13ns with no allocations, which is faster than the two-level map lookup it replaced, and the heaviest provider (server-side V2, wrapping an init deadline and a basis-header closure) at 105ns and 96 bytes. Both are invisible next to the SSE handshake that follows.Behavior changes visible in tests
GetCredentials()reports the whole accepted set, so a key inside its grace period appears there as well as inGetDeprecatedCredentials(). The shared assertions inrelay/testutils_test.gonow compare against the designated credentials via adesignatedCredentialshelper, which is what they were really checking.The accepted set is also declarative where the old model was incremental: an old-format archive payload listing one SDK key revokes a key that a previous payload had granted a grace period. That is called out in the offline-mode test.
Deletions this phase owed
The previous phases retained superseded API because its callers live here: the single-key rotation shims,
UpdateCredentialandCredentialUpdate, andEnvironmentParams.ExpiringSDKKeywithExpiringKeyRep.ToParams. Also fixes the missingc.closedguard in the reconcile path, and the missingreturninAddEnvironmentthat calledUpdateCredentialon a nilEnvContextafter an initialization error.Follow-ups filed
Authorizationper request, so the plumbing needs no change; what needs designing is resetting its retry backoff from another goroutine./statuskey arrays (sdkKeys[],mobileKeys[]) remain phase 5, which is where the singularexpiringSdkKeyfield and the metrics derived from it get re-based onto them.Part of SDK-3199, under epic SDK-3188.
Note
Overview
Replaces incremental credential rotation with a declarative accepted-set reconcile, so each environment holds one upstream SDK client and moves between anchor keys via
LDClient.SetSDKKeyinstead of spinning up a second client per key.ReconcileCredentialsapplies changes in order: register new key mappings, re-anchor (re-key client, repoint event/metrics publishers, rebuild big-segment synchronizer), then remove revoked mappings. Revoking a non-anchor key no longer closes the SDK client. LegacyUpdateCredential/rotator_legacyandEnvironmentParams.ExpiringSDKKeyare removed; auto-config and offline filedata build sets withenvfactory.BuildAcceptedSetand reconcile.Stream handlers are built per connect with an accepted-set re-check to close revocation races.
/statusreads a singleGetAcceptedKeyssnapshot and picks the soonest-expiring non-anchor SDK key forexpiringSdkKey.go.modtemporarily replaces unreleasedgo-server-sdk/go-sdk-eventscommits for runtimeSetSDKKeysupport (must be repinned before merge).Reviewed by Cursor Bugbot for commit a96adce. Bugbot is set up for automated code reviews on this repo. Configure here.