From aa002392d6e6c7d286e4229383491d9c504ee965 Mon Sep 17 00:00:00 2001 From: Aaron Zeisler Date: Thu, 6 Aug 2026 13:32:26 -0700 Subject: [PATCH] fix(relayenv): discard SDK client build that finishes after Close() startSDKClient builds the SDK client without holding c.mu, since the build can block up to sdkInitTimeout. It then unconditionally installs the result into c.clients. If Close() runs to completion while a build is still in flight, Close() has already closed and cleared c.clients and will never revisit it - a later Close() call short-circuits on c.closed without touching the map. A build that finishes afterward would silently install its client into the map, leaking its streaming connection and goroutines forever. Check c.closed after re-acquiring the lock and close the client instead of installing it when the environment has already been torn down. --- internal/relayenv/env_context_impl.go | 23 +++++++++-- internal/relayenv/env_context_impl_test.go | 44 ++++++++++++++++++++++ 2 files changed, 63 insertions(+), 4 deletions(-) diff --git a/internal/relayenv/env_context_impl.go b/internal/relayenv/env_context_impl.go index dc6d5836f..571c0d1b3 100644 --- a/internal/relayenv/env_context_impl.go +++ b/internal/relayenv/env_context_impl.go @@ -492,7 +492,16 @@ func (c *envContextImpl) startSDKClient(sdkKey config.SDKKey, readyCh chan<- Env client, err := c.sdkClientFactory(sdkKey, c.sdkConfig, c.sdkInitTimeout) c.mu.Lock() name := c.identifiers.GetDisplayName() - if client != nil { + // The build above happens without c.mu held, and can take up to c.sdkInitTimeout. If Close() runs to + // completion while it's in flight, Close() has already closed and cleared c.clients and will never + // revisit it (a later Close() call short-circuits on c.closed). Installing this client now would leak + // its streaming connection and goroutines forever, so close it instead of installing it. + discarded := c.closed + if discarded { + if client != nil { + _ = client.Close() + } + } else if client != nil { c.clients[sdkKey] = client // The data store instance is created by the SDK when it creates the client. Now that @@ -511,10 +520,16 @@ func (c *envContextImpl) startSDKClient(sdkKey config.SDKKey, readyCh chan<- Env } c.evaluator = ldeval.NewEvaluatorWithOptions(dataProvider, evalOptions...) } - c.initErr = err + if !discarded { + c.initErr = err + } c.mu.Unlock() - if err != nil { + switch { + case discarded: + c.globalLoggers.Infof("SDK key %s finished initializing after the environment %q was already closed; discarding it", + sdkKey.Masked(), name) + case err != nil: if suppressErrors { c.globalLoggers.Warnf("Ignoring error initializing LaunchDarkly client for %q: %+v", name, err) @@ -526,7 +541,7 @@ func (c *envContextImpl) startSDKClient(sdkKey config.SDKKey, readyCh chan<- Env } return } - } else { + default: c.globalLoggers.Infof("Initialized LaunchDarkly client for %q (SDK key %s)", name, sdkKey.Masked()) } if readyCh != nil { diff --git a/internal/relayenv/env_context_impl_test.go b/internal/relayenv/env_context_impl_test.go index 5c0aa671d..56da28926 100644 --- a/internal/relayenv/env_context_impl_test.go +++ b/internal/relayenv/env_context_impl_test.go @@ -34,6 +34,7 @@ import ( "github.com/launchdarkly/go-sdk-common/v3/ldvalue" ldevents "github.com/launchdarkly/go-sdk-events/v3" "github.com/launchdarkly/go-server-sdk-evaluation/v3/ldbuilders" + ld "github.com/launchdarkly/go-server-sdk/v7" "github.com/launchdarkly/go-server-sdk/v7/ldcomponents" "github.com/launchdarkly/go-server-sdk/v7/subsystems" "github.com/launchdarkly/go-server-sdk/v7/subsystems/ldstoreimpl" @@ -134,6 +135,49 @@ func TestConstructorWithOnlySDKKey(t *testing.T) { assert.Nil(t, env.GetInitError()) } +func TestSDKClientBuildIsDiscardedIfEnvClosedFirst(t *testing.T) { + // startSDKClient calls the SDK client factory without holding c.mu, since the build can take a while. + // If Close() runs to completion while a build is still in flight, the late-finishing build must not + // install its client into c.clients - Close() has already closed and cleared that map and will never + // look at it again (a later Close() call short-circuits on c.closed), so a client installed after the + // fact would never be closed, leaking its streaming connection and goroutines. + envConfig := st.EnvMain.Config + + mockLog := ldlogtest.NewMockLog() + defer mockLog.DumpIfTestFailed(t) + + buildStarted := make(chan struct{}) + proceedWithBuild := make(chan struct{}) + clientCh := make(chan *testclient.FakeLDClient, 1) + realFactory := testclient.FakeLDClientFactoryWithChannel(true, clientCh) + + blockingFactory := func(sdkKey config.SDKKey, sdkConfig ld.Config, timeout time.Duration) (sdks.LDClientContext, error) { + close(buildStarted) + <-proceedWithBuild + return realFactory(sdkKey, sdkConfig, timeout) + } + + env := makeBasicEnv(t, envConfig, blockingFactory, mockLog.Loggers, nil) + + if !helpers.AssertChannelClosed(t, buildStarted, time.Second, "timed out waiting for SDK client build to start") { + t.FailNow() + } + + require.NoError(t, env.Close()) + + // Let the build finish only after Close() has already run to completion. + close(proceedWithBuild) + + client := requireClientReady(t, clientCh) + client.AwaitClose(t, time.Second) // must be closed rather than leaked + + envImpl := env.(*envContextImpl) + envImpl.mu.RLock() + _, present := envImpl.clients[envConfig.SDKKey] + envImpl.mu.RUnlock() + assert.False(t, present, "client built after Close() must not be installed into c.clients") +} + func TestConstructorWithJSClientContext(t *testing.T) { envConfig := st.EnvWithAllCredentials.Config jsClientContext := JSClientContext{Origins: []string{"origin"}}