chore: public api works on atomic pointers by sighphyre · Pull Request #211 · Unleash/unleash-go-sdk

sighphyre · 2025-11-18T08:17:38Z

First step to making this lock free is to stop using patterns that require locking. This PR makes some overdue changes to the way data is held in the SDK

The Datastore layer is now a data store. It no longer caches, that's now handled elsewhere. This has the nice side effect of allowing caching to work correctly when end users implement their own caching
Segments are now correctly persisted, meaning first load from disk with the SDK will no longer cause features using segments to fail until API hydration kicks in
Segments and features are no longer cached independently in different layers in the SDK
Segments and features are held together behind an atomic pointer, meaning synchronization is now done by hardware and not between the Go runtime and the OS
Feature evaluation logic is broken into it's own home, expressed as a set of functions that operate on the held feature state

Overall this makes the p99.9 unmeasurably small when operating on a standard strategy feature (which in theory is just a boolean return).

coveralls · 2025-11-19T07:30:37Z

Pull Request Test Coverage Report for Build 19563582314

Details

254 of 351 (72.36%) changed or added relevant lines in 10 files are covered.
56 unchanged lines in 8 files lost coverage.
Overall coverage decreased (-4.3%) to 77.151%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
storage.go	7	8	87.5%
api/feature.go	4	6	66.67%
internal/strategies/flexible_rollout.go	0	2	0.0%
bootstrap_storage.go	7	14	50.0%
internal/strategies/helpers.go	0	10	0.0%
metrics.go	72	101	71.29%
feature_state.go	67	113	59.29%

Files with Coverage Reduction	New Missed Lines	%
storage.go	1	78.57%
nooplistener.go	2	62.5%
bootstrap_storage.go	3	50.0%
internal/api/metrics.go	3	0.0%
debuglistener.go	6	0.0%
unleash_mock.go	6	75.0%
client.go	12	86.93%
metrics.go	23	75.37%

Totals
Change from base Build 19537135969:	-4.3%
Covered Lines:	1587
Relevant Lines:	2057

💛 - Coveralls

sighphyre · 2025-11-19T07:53:49Z

+	ds.backingStore.Init(backupPath, appName)
+}
+
+func (bs *BootstrapStorage) Load() (*api.FeatureResponse, error) {


The logic here needs to change because we now only access that data store when storing or retrieving data. So now we no longer early exit when failing to read the bootstrap, instead we attempt to fallback to the store and if that fails only then do we return an error. In practice I'm not sure the error handling matters much here since we silently ignore the error anyway

sighphyre · 2025-11-19T07:55:02Z

-	return bs.backingStore.Persist()
-}
-
-func (bs *BootstrapStorage) Get(key string) (*api.Feature, bool) {


Get isn't needed anymore since this isn't the cache now. Instead the equivalent responsibility is done on the repository layer

sighphyre · 2025-11-19T07:55:21Z

-	return bs.backingStore.Get(key)
-}
-
-func (bs *BootstrapStorage) List() []*api.Feature {


Likewise this function no longer makes sense. Honestly it probably never made sense on the bootstrapping layer

sighphyre · 2025-11-19T07:57:36Z

 // It is safe to call this method from multiple goroutines concurrently.
 func (uc *Client) IsEnabled(feature string, options ...FeatureOption) (enabled bool) {
-	result, f := uc.isEnabled(feature, options...)
+	snapshot := uc.repository.snapshot()


This is effectively the magic. The repository can yield up snapshots of it's current state of features and segments. That snapshot is backed by an atomic pointer, meaning it remains valid for as long as the pointer is held, while the repository is allowed to swap its own pointer for a new value without affecting requests in flight

sighphyre · 2025-11-19T07:58:56Z

 // isEnabled abstracts away the details of checking if a toggle is turned on or off
 // without metrics
-func (uc *Client) isEnabled(feature string, options ...FeatureOption) (api.StrategyResult, *api.Feature) {
+func (uc *Client) isEnabled(feature string, snapshot *FeatureMemoryState, options ...FeatureOption) (api.StrategyResult, *api.Feature) {


We must operate on the same pointer, otherwise we risk tearing the evaluation. That is actually a substantial risk on the existing implementation - that a single feature evaluation may be operating on a feature or segment that changes during the course of the evaluation

sighphyre · 2025-11-19T08:00:50Z

 	}
 }
+
+func resolveToggle(snapshot *FeatureMemoryState, opts featureOption, featureName string) *api.Feature {


This needs to be removed. This is a work around because the way we designed our storage interface means that end users implementing their own would lose access to caching. However, this is in the public API and it's hard and painful to remove now and this needs to be a different PR's problem

sighphyre · 2025-11-19T08:05:06Z

 type deltaProcessor struct {
-	storage            Storage
-	repository         *repository // Repository reference for segment manipulation
+	repository         *repository // ideally this shouldn't be necessary, but for now we're going going to use it to resolve a snapshot of the feature state


In the spirit of keeping this PR self contained, I've made the smallest possible changes to this file. That being said, this needs to be dealt with soonish. The circular dependency on repository is going to cause problems and there's a lot of inappropriate intimacy with other modules. Ideally this becomes a family of pure functions which collapse deltas into a feature response and allow other modules to deal with the other work

sighphyre · 2025-11-19T08:09:56Z

 		}
 	}

-	if err := dp.repository.updateStorageWithDelta(currentFeatures, segments); err != nil {


I've chosen to discard this error handling branch because I think its wrong. This can only occur if writing to the persistent storage fails in the v5 implementation. The data store is still updated, which means feature flags are still working correctly. I think this is especially problematic with custom implementations of the data store where we don't control the error paths

sighphyre · 2025-11-19T08:11:03Z

@@ -0,0 +1,163 @@
+package unleash


Harvested API from Client. This is effectively a state that exposes methods that determine if a given feature/variant is enabled for that state. This isn't quite there yet in terms of architecture but it's closer to how this conceptually should work. Don't want to do more here because this PR is already quite large

sighphyre · 2025-11-19T08:11:51Z

+}
+
+// evaluateFeature applies strategies + constraints for a single feature.
+// It does NOT handle fallbacks or missing features; that's the caller's job.


Similar to how other SDKs handle this. Top level APIs deal with fallbacks, missing features + metrics. Engine layers deal with computational evaluation of a flag

sighphyre · 2025-11-19T08:16:16Z

 	return repo
 }

+func (r *repository) updateState(features *api.FeatureResponse) {


One place to control access to mutating the state. This isn't perfect and should be its own thing. But that requires some redesign of the repository layer

sighphyre · 2025-11-19T08:19:08Z

 			if r.streamingClient != nil {
 				r.streamingClient.stop()
 			}
-			if err := r.options.storage.Persist(); err != nil {


I actually don't know what this is intended to achieve. So I've done the dumbest thing possible and removed it. Neither an error case nor a close case should cause a persistence call. Persist is now done explicitly where it's needed - when new data is fetched. Once via delta processing and once via old style feature fetch

sighphyre · 2025-11-19T08:20:11Z

 }

-// updateStorageWithDelta updates the storage with delta changes in a thread-safe manner
-func (r *repository) updateStorageWithDelta(features map[string]*api.Feature, segments map[int][]api.Constraint) error {


This now goes via the one and only one way to update state. I think this method only really exists because the delta processor and the repository are strangely coupled at the moment

sighphyre · 2025-11-19T08:21:19Z

 // IsStreaming returns whether the repository is currently in streaming mode
 func (r *repository) IsStreaming() bool {
-	r.RLock()
-	defer r.RUnlock()


The streaming property is set on construction and never mutated. This lock does nothing but introduce contention that doesn't need to be here

sighphyre · 2025-11-19T08:22:03Z

-	return feature
-}
-
-func (r *repository) resolveSegmentConstraints(strategy api.Strategy) ([]api.Constraint, error) {


This absolutely does not belong in this layer. It's been moved to feature_state where it can be happy and lock free

sighphyre · 2025-11-19T08:23:23Z


-// Storage is an interface that can be implemented in order to have control over how
-// the repository of feature toggles is persisted.
+// Storage controls persistence of the SDK state (features + segments).


This module is the heart of a lot of the problems we have. I've taken a knife to it. It now does what it says on the tin. It stores stuff. There's more to be done here but in another PR

This unfortunately is a breaking change

sighphyre · 2025-11-19T08:24:53Z

 func (m *mockEvent) Event() string { return m.event }
 func (m *mockEvent) Data() string  { return m.data }

+type NoOpStorage struct{}


So that our tests are not loading from disk and therefore hydrating their initial state from the runs of other tests. It's not technically necessary for this PR but I've had a ton of problems in SDKs that don't correctly enforce a pattern for no op storage and it's substantially worse in complex integration tests

FredrikOseberg

I think this looks sane, but it's a big change and requires heavy testing. Maybe we can go over it tomorrow together?

…s of the whole state they need

* fix: use a sync pool for strategy randomness, which offers slightly better perf under load * chore: rework metrics to use less locking on the hotpath * fix: variant strategies now correctly inherit stickiness from their strategy * chore: make metric count channels best effort send

thomasheartman added this to Issues and PRs Nov 18, 2025

github-project-automation Bot moved this to New in Issues and PRs Nov 18, 2025

chriswk assigned sighphyre Nov 18, 2025

chriswk moved this from New to In Progress in Issues and PRs Nov 18, 2025

sighphyre commented Nov 19, 2025

View reviewed changes

sighphyre marked this pull request as ready for review November 19, 2025 09:10

sighphyre requested a review from FredrikOseberg November 19, 2025 14:09

FredrikOseberg approved these changes Nov 19, 2025

View reviewed changes

github-project-automation Bot moved this from In Progress to Approved PRs in Issues and PRs Nov 19, 2025

sighphyre force-pushed the fix/storage-returns-pointers branch from e3e3fa7 to c401358 Compare November 20, 2025 12:38

sighphyre added 3 commits November 20, 2025 14:41

chore: make isEnabled and getVariant operate purely on atomic pointer…

f7a1760

…s of the whole state they need

chore: fix bootstrapping

6f15200

chore: patch tests

c23c708

sighphyre added 7 commits November 20, 2025 14:41

fix: restore feature resolver

0d909b4

fix: correctly retrieve snapshots in tests

536d0e0

fix: and one last errant snapshot in integration tests

30d8b3c

fix: backup + early exit

b281bea

fix: restore parent/child check

13f6d20

chore: restore backup init method

34f0509

chore: minor clean ups to make the PR easier to understand

cc57f12

sighphyre force-pushed the chore/public-api-works-on-atomic-pointers branch from 4b5508d to cc57f12 Compare November 20, 2025 12:41

sighphyre and others added 2 commits November 20, 2025 15:19

chore: rebase v6

19e25a8

sighphyre merged commit 6ecf2d4 into fix/storage-returns-pointers Nov 21, 2025
5 checks passed

sighphyre deleted the chore/public-api-works-on-atomic-pointers branch November 21, 2025 07:44

github-project-automation Bot moved this from Approved PRs to Done in Issues and PRs Nov 21, 2025

Uh oh!

Conversation

sighphyre commented Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 19563582314

Details

💛 - Coveralls

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sighphyre Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FredrikOseberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sighphyre commented Nov 18, 2025 •

edited

Loading

coveralls commented Nov 19, 2025 •

edited

Loading

sighphyre Nov 19, 2025 •

edited

Loading