fix(autoconfig): process each environment on its own queue - #824
Draft
aaron-zeisler wants to merge 66 commits into
Draft
fix(autoconfig): process each environment on its own queue#824aaron-zeisler wants to merge 66 commits into
aaron-zeisler wants to merge 66 commits into
Conversation
Canonical reference docs for the concurrent SDK keys project, covering: - phase1-design.md: architecture, model (anchor, accepted set, expiry, events), wire format, decisions and rationale, open questions. - phase1-plan.md: branching strategy, wave breakdown, task list with dependencies and estimates, test strategy, rollout, JIRA structure. These docs are the source of truth for the work tracked under SDK-2453. Sub-task tickets reference specific sections for design rationale.
Mobile key rotation no longer panics when RotateWithGrace is called with a non-nil grace period. The outgoing key can be recorded in a new deprecatedMobileKeys map (key → expiry), mirroring the SDK-key deprecation model at a minimal level. updateMobileKey now takes grace *GracePeriod: nil or already-expired grace still revokes the previous key immediately (same as before for existing callers). A valid grace period only stores the old key’s expiry; StepTime does not expire deprecated mobile keys yet (planned in T1.c). Re-promoting a key clears it from deprecatedMobileKeys.
Adds internal/events/event_payload_regression_test.go with regression tests for EventDispatcher Authorization routing when the incoming SDK request’s credential differs from the dispatcher’s stored anchor (authKey).
#707) Fixed an intermittent test failure in `TestAutoConfigRemovesCredentialForExpiredSDKKey` caused by a race between credential registration and relay connection mapping.
Adds reusable building blocks under internal/sharedtest so full-stack tests outside package relay can drive RAC and offline config the same way in-process relay tests already do. `relay/concurrent_keys_harness_ref_test.go` is a reference integration test covering offline archive → SDK stream and RAC mock → SDK stream paths.
Design doc (§7, §9, §13, §14): - §7 rewritten as the concrete T2.c/T2.d specification based on PoC findings. Key change: the in-memory data store is NOT shared across the swap (each client init rebuilds it). The new rule: keep the old store/anchor authoritative until the new client is Initialized(). - §9 adds the reconnect-with-jitter mechanism for malformed payloads — backend has no NAK channel, so we must reconnect to force a fresh put. - §13 (Recorded decisions): removed 'working assumption' qualifiers from Q6/Q7/Q9; added two new decisions (re-anchor store handling and re-anchor atomicity rollback). - §14 (Resolved questions): all of Q5, Q6, Q7, Q8, Q11 now have team-confirmed answers. Plan doc (T1.b, T1.c, T2.c, T2.d, T3.b, T3.c): - T1.b: ReconcileCredentials signals malformed payload via structured error; caller handles. - T1.c: scope narrows — Q8 confirms per-credential downstream tracking already exists. - T2.c: replaces 'TBD per PoC' with concrete order-of-ops, rollback, and store-handling spec. - T2.d: drops httpconfig (no change needed per H4); focuses on big-segment sync re-wire (recreate or replace-credential). - T3.b: helper signals malformed payload as structured error. - T3.c: RAC handler triggers RAC reconnect-with-jitter on malformed payload; offline handler preserves state only.
… Wave 3 T5 scope renamed to 'Tests, release, and merge-forward'. The Wave 3 chain becomes [Wave 2 terminal nodes] → T5.f (remove project scaffolding) → T5.g (squash-merge to v8 + release) → T5.e (merge-forward to v9, calendar-deferred). T5.f removes docs/concurrent-keys/ and the PoC test file before merge; this file is part of what gets removed by T5.f itself. T5.g squash-merges feat/concurrent-keys to v8 as a single feat: commit, triggering the minor version bump. JIRA tickets created: SDK-2555 (T5.f) and SDK-2556 (T5.g), both under SDK-2535. Dependencies wired.
Adds internal accepted-set maps (acceptedSDKKeys, acceptedMobileKeys) to Rotator, each entry holding an optional expiry timestamp (*time.Time, nil = permanent). Populates the maps from Initialize() so a single-key environment immediately reflects the accepted set correctly.
…#708) Fix a data race in StartHTTPServer where the shutdown goroutine calls close(errCh) concurrently with the listener goroutine sending to errCh, producing a "send on closed channel" panic.
…current keys (#702) - Adds ConcurrentKeyRep wire type for entries in the sdkKeys/mobileKeys arrays - Adds SDKKeys []ConcurrentKeyRep and MobileKeys []ConcurrentKeyRep to EnvironmentRep - Adds AcceptedSDKKey, AcceptedMobileKey, and corresponding slice fields to EnvironmentParams
Adds durable TestReanchorPoC_H* tests in env_context_reanchor_test.go that exercise today's UpdateCredential grace-period rotation and document seven hypotheses (store rebuild, SSE survival, big-segment re-wire, ordering, failure rollback, etc.) as the executable spec for T2.c. Adds .agent-docs/concurrent-keys/phase1-T0-reanchor-poc-findings.md and updates phase1-design.md §7 with validated behavior—especially store handover (SSERelayDataStoreAdapter reusing the existing store) instead of an empty in-memory window after swap.
Adds §7 "End-to-end acceptance scenarios" — 22 scenarios across multi-key auth, re-anchoring, key lifecycle, defensive behavior, sources, back-compat, and observability. Each scenario names the owning sub-task; the catalog as a whole is the release-readiness coverage check (referenced from the release-readiness checklist). Will be reflected in the project's HTML progress dashboard in a separate session.
…t sync timeout (#711) Reduces flaky Integration Tests - staging failures by relaxing two timing constants only in tests—no production Relay behavior changes. In testSDKKeyExpires, the deprecated-key grace period is increased from 5s to 30s so sequential rotateSDKKeys calls (one per environment) still send a future expiry to staging after CI latency and clock skew; the test still sleeps only until keys expire before asserting cleanup. In verifyEvaluationWithBigSegment, the assert.Eventually poll window is increased from 20s to 60s so a second big-segment update can propagate via streaming in slow environments; passing runs still exit as soon as evaluations match.
Adds a delivery-oriented milestone view (M1-M5) mapping the existing tasks to demonstrable capabilities, and records the 2026-06-23 decision to finish Milestone 2 before heavy Milestone 3 by reducing T1.b (SDK-2538) to a behavior-neutral foundation. The handler wiring + UpdateCredential removal + payload validation move to T3.c (SDK-2547); the re-queue-on-primary-switch fixes (SDK + mobile) move to T2.c (SDK-2542).
…on (#712) Introduces AcceptedSet and AcceptedSetBuilder so an environment can describe a full desired credential state (multiple SDK/mobile keys with optional expiry, anchor SDK key, primary mobile key, env ID), with Build() rejecting empty sets and sets without a designated anchor. Adds Rotator.Reconcile and EnvContext.ReconcileCredentials to diff that desired set against current state, queue additions/expirations, and apply them via the existing triggerCredentialChanges path. PrimaryCredentials now reflects all accepted non-deprecated keys in the rotator maps; the legacy Rotate / RotateWithGrace path is updated to keep those maps aligned (including revoking the previous key when SDK grace is already expired). Anchor-only upstream behavior: in addCredential, only the anchor SDK key starts an upstream client and repoints SDK event/metrics forwarding; only the primary mobile key repoints mobile event forwarding. Extra accepted keys still get streams, handlers, and connection mappings.
…ials (#713) Introduces BuildAcceptedSet, a shared envfactory helper that turns EnvironmentParams into a credential.AcceptedSet (plus anchor SDK key) for upcoming EnvContext.ReconcileCredentials use in autoconfig and filedata handlers. Handler wiring is intentionally deferred.
…le-key disconnects (#714) Generalizes Rotator.StepTime to enforce per-key expiry for mobile keys and for the reconcile path, symmetric with the existing SDK/legacy grace path. DeprecatedCredentials now surfaces deprecated mobile keys alongside deprecated SDK keys.
Fixes SDK key rotation leaks and incorrect credential lifetime behavior in the Relay Proxy.
…nv (#718) Adds TestGetClientReturnsAnchorInMultiKeyEnv to lock in the multi-key SDK contract after anchor-only upstream clients: GetClient must return the anchor’s fake upstream client before and after ReconcileCredentials adds two non-anchor keys, must stay non-nil, and must not spawn extra upstream clients.
…als (#719) Wires auto-config (RAC) and offline archive handlers to ReconcileCredentials via BuildAcceptedSet, replacing the legacy UpdateCredential / grace-period path. EnvContext drops CredentialUpdate and adds GetSDKKey() / GetMobileKey() so the status endpoint can show a single anchor and primary mobile key deterministically.
…tation The status sdkKeys/mobileKeys arrays carry the full accepted set including the anchor/primary (per the backend tech spec and RAC payload spec), not a non-anchor subset. Drop the 'anchor first, then identifier-alphabetical' ordering requirement: it was a derived-doc embellishment with no basis in the source docs or Confluence (which treat every key as equally valid), so order is unspecified. Clarify the array-presence guarantee: always present, sdkKeys always >= 1 (the anchor), mobileKeys may be empty for a server-only env.
Renames the anchor role (the single SDK key that owns the upstream connection) so it is not confused with “primary” when many SDK keys can be accepted at once.
…ator (#723) Removes the legacy imperative credential rotation API (Rotate, RotateWithGrace, GracePeriod, and deprecatedSdkKeys / deprecatedMobileKeys) from internal/credential/rotator.go. Credential state is now maintained only through Reconcile and the acceptedSDKKeys / acceptedMobileKeys maps, where “deprecated” keys are simply accepted entries with a non-nil expiry.
#725) Adds relay/concurrent_keys_auth_test.go, a dedicated integration suite for multi-key environments (sdkKeys[] / mobileKeys[]) where one anchor owns the single upstream connection.
…728) Reject a defined primary mobile key (the wire's mobKey) that is absent from mobileKeys[], mirroring the existing anchor-not-in-array invariant.
#727) Updates two relay test fixtures that still modeled the eval context they send to the SDK endpoints as a legacy LDUser ({"key":"userkey"}, no kind) to use a valid single-kind evaluation context ({"key":"userkey","kind":"user"}).
… set (#729) Extends the credential accepted-set model so each SDK/mobile key stores optional wire key identifiers (plus expiry) alongside the secret value, preparing later /status exposure without changing auth behavior in this PR.
…730) Expose the full accepted credential set via Rotator.AcceptedKeys() and EnvContext.GetAcceptedKeys() — every server-side SDK key and mobile key, including the anchor and primary mobile key — so the status endpoint can surface it.
…731) Surface the full accepted credential set on /status as sdkKeys[] / mobileKeys[] arrays — each entry carrying the obscured value, the optional wire identifier, and an optional Unix-millisecond expiry. The existing scalar sdkKey / mobileKey fields are preserved and now designate which array entry is the anchor / primary.
#760) Removes dead and redundant credential plumbing left over after the concurrent-keys / accepted-set model, without changing wire parsing for old sdkKey.expiring payloads (that path still synthesizes AcceptedSDKKeys in ToParams).
…op offline re-anchor from stranding the initial client (#763) Aligns credential reconcile with StepTime’s strictly-after expiry rule: SDK and mobile keys in reconcileSDKKeys / reconcileMobileKeys are treated as absent only when now is after the expiry instant, not when now equals it—so a reconcile at the boundary no longer drops a key the cleanup ticker would still accept for one more instant.
…a primary mobile key (#761) BuildAcceptedSet now treats a non-empty mobileKeys[] with an undefined mobKey as malformed (via NewPrimaryMobileKeyMissingError), matching the existing SDK anchor rules. Accepting that shape would clear the primary on reconcile while event forwarding could keep using the old primary.
- Bump `google.golang.org/grpc` v1.80.0 -> v1.82.1 (fixes HIGH severity [GHSA-hrxh-6v49-42gf](GHSA-hrxh-6v49-42gf)) - Bump `golang.org/x/net` v0.55.0 -> v0.56.0 (fixes CVE-2026-46600) - Bump `golang.org/x/text` v0.37.0 -> v0.39.0 (fixes CVE-2026-56852)
Dependency-only change: github.com/launchdarkly/eventsource is updated v1.11.0 → v1.11.1 in go.mod and go.sum; no Relay source changes. v1.11.1 fixes a stream-shutdown deadlock when Close races with an in-flight event: the stream could stop closing Events, so shutdown paths that drain the stream (e.g. auto-config StreamManager / consumeStream) could block indefinitely and hang relay.Close().
…ing (#775) Pre-v8 cleanup that removes concurrent-keys project scaffolding while keeping shipped feature code and regression tests.
Brings `feat/concurrent-keys` up to date with the functional commits that landed on `v8`: * PR #734: Add `X-LaunchDarkly-Instance-Id` to the browser CORS allowlist * PR #749: Bound REPORT eval request body size (SEC-8503) * PR #750: Make the usage metrics event publisher capacity configurable * PR #789: Bump otel to 1.44.0 and klauspost/compress to 1.18.7 to patch disclosed CVEs
#795) Makes the Relay Proxy reject SDK keys and mobile keys that are scoped to a view, so they never enter the accepted credential set. Filtering happens in envfactory.BuildAcceptedSet so a view-scoped key is absent from the credential lookup map entirely. An SDK presenting one gets a 401.
v8 has since shipped 8.20.0, leaving version.go as the only conflicting file between this branch and v8. Setting it back to v8's value makes both sides of the merge an identical change, so the merge applies cleanly. The 8.21.0-rc.1 string is preserved by the tag of the same name, and release-please will bump this file when it cuts the GA release.
(cherry picked from commit 4975098)
Bumps the auto-config cache model version from 1 to 2, so a relay built from this branch rejects cache entries written by a v8 relay instead of accepting them as its own.
aaron-zeisler
force-pushed
the
aaronz/SDK-2921/serialize-rac-actions-per-environment
branch
from
August 18, 2026 01:05
eea8244 to
0c6dd9c
Compare
aaron-zeisler
force-pushed
the
aaronz/SDK-2921/serialize-rac-actions-per-environment
branch
3 times, most recently
from
August 18, 2026 16:06
b28f0f2 to
1812f1e
Compare
AutoConfigActionQueue wraps AutoConfigActions so that each environment's actions run on their own goroutine, in submission order, independently of every other environment's. Nothing uses it yet; the next commit wires it into Relay. A worker exists only while its queue holds work. nextAction decides between taking the next action and retiring the queue, and it makes that decision under the same lock enqueue uses for lookup -- so an action can never land on a retired queue, and two workers can never run for one environment. The queue key includes the filter key, because a filtered environment is a separate context with its own SDK client and its own anchor. ReceivedAllEnvironments is a drain barrier: it forwards only once every environment with outstanding work has drained, so a readiness signal cannot fire while an environment is still being applied. It derives that set from the live queues, since a queue is removed only after its worker has run everything in it. It waits on its own goroutine, so the caller is never blocked. A semaphore bounds concurrent actions, and execute recovers from a panic -- an escaped panic would kill the worker and leave its queue in the map with nothing draining it, silently freezing that environment's configuration. The wrapped handler is named next, following the usual convention for a wrapper in a chain: q.next.AddEnvironment reads as delegation rather than recursion. Also widen the package doc, which described only routing by project key and did not mention the interfaces the package owns.
One goroutine reads the auto-configuration stream and, before this change, completed every environment's action inline. An environment whose SDK anchor moves rebuilds its upstream client synchronously, blocking for up to Main.InitTimeout, so one environment's rebuild delayed every other environment's updates and credential revocations. A bulk rotation across N environments cost N * InitTimeout in the worst case, and a revocation issued during one could sit unapplied for minutes. Wrap the action handler in AutoConfigActionQueue. Within an environment the reconcile is still fully synchronous, so the re-anchor's commit/rollback atomicity is unchanged; only distinct environments now overlap. StreamManager still processes one stream message at a time, since only the handler call is deferred. All 20 TestReanchor tests pass without modification. Old-format payloads synthesize the accepted-key model from the singular sdkKey field, so this affected ordinary single-key rotation too, not only environments using concurrent keys. Also rewrite the comment in AddEnvironment that justified its check-then-act by asserting a single calling goroutine. That is now false for distinct environments. The conclusion still holds, for a different reason: the check is keyed per environment, and addEnvironment holds the lock to insert.
Drives two environments through the real auto-configuration handler: environment 1's re-anchor build parks in the client factory, and environment 2's unrelated rotation must still complete while it is parked. Fails in about a second with a clear assertion when the queue is bypassed. Both environments rotate via the singular sdkKey field with no expiring slot, deliberately: that is the shape an ordinary auto-configured account sends, and it still moves the anchor.
aaron-zeisler
force-pushed
the
aaronz/SDK-2921/serialize-rac-actions-per-environment
branch
from
August 18, 2026 16:43
1812f1e to
90f9026
Compare
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.
Summary
LaunchDarkly sends the Relay Proxy its configuration on one stream. One goroutine reads that stream. Before this change, the goroutine completed all work for a message before it read the next message.
Some of that work is slow. When an environment gets a new SDK key, the Relay Proxy must build a new SDK client and wait for that client to connect. The wait can last as long as
Main.InitTimeout. The default value is 10 seconds.The goroutine did not read the stream during that wait. Therefore one environment delayed every other environment. Credential revocations for other environments also waited.
This change gives each environment its own queue and its own worker. The stream goroutine adds each message to the correct queue, then continues.
With 29 environments that rotate keys together, the worst case improves from approximately 4.8 minutes to approximately 10 seconds. Each environment now waits only for its own build.
Jira: SDK-2921
Why this was a regression
Relay v8 builds the client asynchronously. The concurrent-keys work made the build synchronous to get safe rollback: the Relay Proxy moves the connection only after the new client connects. That behavior is correct, and this change keeps it. The problem is the shared goroutine, not the synchronous build.
Two points about the scope:
sdkKeysarray still produces an anchor change, becauseEnvironmentRep.ToParamsbuilds the accepted-key set from the singlesdkKeyfield when the array is absent (see the "Old-format payload" branch ininternal/envfactory/env_rep.go).The change
AutoConfigActionQueuewraps the existingprojmanager.AutoConfigActionsinterface.relay/relay.gocreates it.Three properties make this seam safe:
ProjectRouterandEnvironmentManagernever read state back from the handler. Every call to it returns nothing. Their maps stay on the stream goroutine and need no lock.StreamManagerstill processes one message at a time.addEnvironmenttakesRelay.lock, andEnvironmentLookuphas its own mutex.Concurrent work across environments is also not new. Each environment already runs a credential cleanup ticker in its own goroutine, and that ticker changes the same relay-wide state. This change extends a pattern that the design already accepts.
Please review these decisions
ReceivedAllEnvironmentsis now a drain barrier. It sets thefullyConfiguredflag, which controls whether Relay serves requests and what the status endpoint reports. It must not fire while the Relay Proxy still applies environments from the same payload. This makes readiness stricter than before. The flag previously became true as soon as Relay read the payload.putpayloads.putthat adds every environment would otherwise construct them all at the same time. Before this change, the stream goroutine built them one at a time.submituses for lookup. Therefore no action can reach a retired queue, and two workers cannot run for one environment.executerecovers from a panic. A panic that escaped would kill the worker and leave the queue in the map. That environment would then stop all configuration updates for the life of the process.Closewaits 2 seconds, then abandons the remaining work. An indefinite wait would hold the process open during a slow build. Abandonment is safe:addEnvironmentrefuses new work after Relay closes,EnvContext.Closeis idempotent, and the re-anchor rechecks the closed flag after its build.