Skip to content

fix(dev-server): close the SDK client when initialization fails - #882

Draft
nieblara wants to merge 1 commit into
mainfrom
cursor/close-sdk-client-on-init-failure-2001
Draft

nieblara wants to merge 1 commit into
mainfrom
cursor/close-sdk-client-on-init-failure-2001

Conversation

@nieblara

Copy link
Copy Markdown
Contributor

Requirements

  • I have added test coverage for new or changed functionality
  • I have followed the repository's pull request submission guidelines
  • I have validated my changes against all supported platform versions

Related issues

Follow-up to #873, which updated go-server-sdk from 7.13.4 to 7.18.0.

Describe the solution you've provided

The dev server creates an SDK client in internal/dev_server/adapters/sdk.go for each project import and sync. If ld.MakeCustomClient returned an error, GetAllFlagsState returned before its deferred Close. After a timeout, the SDK still returns a client. That client kept its stream connection open and retried in the background.

Since go-server-sdk 7.16 (#873), a rejected SDK key (HTTP 401 or 403) also gives a timeout after the full --sdk-init-timeout. The failed client then retries with a backoff of 5 minutes to 1 hour. Each failed import or sync left one more client that retried until the dev server stopped. The same leak already existed for a plain timeout.

Now, if MakeCustomClient returns an error and a client, GetAllFlagsState closes the client before it returns. The error and its message do not change: unable to get source flags from LD SDK: timeout encountered waiting for LaunchDarkly client initialization.

streamingSdk has a new unexported logger field. Tests use it to read the SDK log. Production code does not set it, so the SDK log output does not change.

The new test TestGetAllFlagsStateClosesClientWhenInitFails starts a local stream server with a 500 ms initialization timeout. It makes sure that the client closes in three cases:

  • Rejected SDK key (401): the SDK log has Closing LaunchDarkly client.
  • Stream that sends no data: the server sees the stream connection close.
  • Stream that is unavailable (503): the server gets no more requests after GetAllFlagsState returns.

All three cases fail without the fix and pass with it. The test passed five times in a row with -race. go build, go test ./..., and golangci-lint v1.63.4 pass.

I also ran the built binary with ldcli dev-server start against a fake stream that rejects the SDK key. The import fails with HTTP 500 after 3 seconds and the same message as before. The dev-server log now has one Closing LaunchDarkly client line after the timeout. Before this change, it had none.

Describe alternatives you've considered

The dev server could restore the earlier fast failure for a rejected key. To do this, it can watch the data source status and stop early on 401 or 403. That changes the error behavior, so it is not in this pull request.

Additional context

LDClient.Close only logs and stops the data system. It is safe to call on a client that did not initialize.

Open in Web Open in Cursor 

GetAllFlagsState returned before its deferred Close when MakeCustomClient
failed. After a timeout, the SDK still returns a client, and that client
kept its stream connection open and retried in the background. Since
go-server-sdk 7.16 (#873), a rejected SDK key also gives a timeout, so each
failed import or sync left one more client that retried for the life of the
dev server.

If MakeCustomClient returns an error and a client, close the client. The
error and its message do not change.

Co-authored-by: rniebla <rniebla@launchdarkly.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants