fix(relayenv): don't let Close() wait on a re-anchor client build - #826
Draft
aaron-zeisler wants to merge 64 commits into
Draft
fix(relayenv): don't let Close() wait on a re-anchor client build#826aaron-zeisler wants to merge 64 commits into
aaron-zeisler wants to merge 64 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.
Stream handlers are no longer pre-built and cached per credential. GetStreamHandler now calls streamProvider.Handler with a scoped (filterKey, credential) on each request and returns that handler (or the existing 404 invalid-stream handler when the provider returns nil). The envContextImpl.handlers map and all logic that populated or pruned it at env init, credential add, and credential remove are gone.
…credential package (#753)
Fixes a timing race in TestAutoConfigKeyRotationClosesOldClientAndDoesNotDuplicateUpdates that could hit 503 "client was not initialized" on the first downstream GET /all.
…antics The final pre-merge review found five places where the implementation deliberately moved past the design text without the doc being updated. Recorded as implementation notes rather than rewrites so the original reasoning stays visible: - §7: demoted anchor's upstream client closes at commit (double-broadcast avoidance); Case B re-promotion builds fresh; synchronous build occupies the RAC dispatch goroutine (operational characteristic). - §7/§8: rollback on re-anchor init failure is scoped to the anchor change; other payload changes stand; recovery requires a version-bumped update. Resolves the §8 open question on atomicity. - §9: validation runs at the parse boundary before the version is recorded, and is stricter than the two originally named cases. - §6.3: the legacy expiring slot is read for old-format payloads only.
…a closed data store (#759) Fixes a startup race where the initial anchor client’s Build and a concurrent re-anchor Build could each create a wrapper; the last install could win, and closing the superseded client would tear down the store the environment still served.
#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)
… build The credential cleanup ticker acquired reconcileMu unconditionally, and that lock is held for the whole of reconcileCredentials — including the re-anchor's synchronous SDK client build, which blocks for up to Main.InitTimeout. A tick landing during a build parked the ticker goroutine on the lock, and Close() blocks on doneMonitoringCredentials, which only that goroutine closes. Teardown therefore stalled for the remainder of the build (10s by default), observed in a goroutine dump and reproducible in the test harness, which runs a 100ms cleanup interval. Replace reconcileMu with a capacity-1 channel so the acquisition can be selected on, and have the ticker abandon it when stopMonitoringCredentials fires. The serialization the lock exists for is unchanged: a ticker that does acquire still runs its StepTime + add/remove pass exclusively against reconcileCredentials, and only the waiting is given up, on a path where the environment is being torn down anyway.
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
changed the title
…anchor-build Conflict resolution: - env_context_impl.go: #825 rewrote the comments this branch had edited. Kept the simplified prose from upstream and applied the reconcileMu -> reconcileSem rename within it, then restated this branch's own added comments in the same style. - env_context_credential_serialization_test.go: accepted #827's deletion. TestReanchorSync_CredentialExpiryDuringReanchorIsSerialized still covers the ticker-versus-reconcile serialization the deleted test asserted.
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
2 participants
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
Closing an environment could hang for up to
Main.InitTimeout(10s by default).Close()waits for the credential cleanup ticker to exit, and that ticker was waiting on the lock a re-anchor holds while it builds a new SDK client.The ticker now abandons that wait as soon as shutdown is signaled. To make the wait interruptible, the lock becomes a capacity-1 channel. Nothing else changes: a ticker that does get the lock still runs with the same exclusivity as before.