-
Notifications
You must be signed in to change notification settings - Fork 131
feat: report STALE reason when a sync source is disconnected #2017
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
scottt732
wants to merge
1
commit into
open-feature:main
Choose a base branch
from
scottt732:feat/stale-reason-on-disconnected-sync
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,133 @@ | ||
| //nolint:wrapcheck | ||
| package evaluator_test | ||
|
|
||
| import ( | ||
| "context" | ||
| "testing" | ||
|
|
||
| flagdEvaluator "github.com/open-feature/flagd/core/pkg/evaluator" | ||
| "github.com/open-feature/flagd/core/pkg/logger" | ||
| "github.com/open-feature/flagd/core/pkg/model" | ||
| "github.com/open-feature/flagd/core/pkg/store" | ||
| "github.com/open-feature/flagd/core/pkg/sync" | ||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| const staleSource = "testSource" | ||
|
|
||
| func staleEvaluator(t *testing.T, state *store.SourceState) *flagdEvaluator.JSON { | ||
| t.Helper() | ||
|
|
||
| e := flagdEvaluator.NewJSON( | ||
| logger.NewLogger(nil, false), | ||
| store.NewFlags(), | ||
| flagdEvaluator.WithSourceState(state), | ||
| ) | ||
| require.NoError(t, e.SetState(sync.DataSync{FlagData: flagConfig, Source: staleSource})) | ||
|
|
||
| return e | ||
| } | ||
|
|
||
| // Flags are still served while their source is disconnected -- serving | ||
| // last-known-good data beats failing evaluations open -- but the reason tells | ||
| // the caller the value may no longer match the source of truth. | ||
| func TestStale_ReplacesSuccessfulReasons(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| flagKey string | ||
| evalCtx map[string]interface{} | ||
| freshReason string | ||
| expectedVal bool | ||
| }{ | ||
| { | ||
| name: "static resolution", | ||
| flagKey: StaticBoolFlag, | ||
| freshReason: model.StaticReason, | ||
| expectedVal: StaticBoolValue, | ||
| }, | ||
| { | ||
| name: "targeting match", | ||
| flagKey: DynamicBoolFlag, | ||
| evalCtx: map[string]interface{}{ColorProp: ColorValue}, | ||
| freshReason: model.TargetingMatchReason, | ||
| expectedVal: StaticBoolValue, | ||
| }, | ||
| } | ||
|
|
||
| for _, test := range tests { | ||
| t.Run(test.name, func(t *testing.T) { | ||
| state := store.NewSourceState() | ||
| e := staleEvaluator(t, state) | ||
|
|
||
| // baseline: a connected source keeps its usual reason | ||
| val, _, reason, _, err := e.ResolveBooleanValue(context.TODO(), "default", test.flagKey, test.evalCtx) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, test.freshReason, reason) | ||
| assert.Equal(t, test.expectedVal, val) | ||
|
|
||
| // the source drops; the value is unchanged but now reported stale | ||
| state.SetStale(staleSource, true) | ||
|
|
||
| val, _, reason, _, err = e.ResolveBooleanValue(context.TODO(), "default", test.flagKey, test.evalCtx) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, model.StaleReason, reason, "a disconnected source must resolve as STALE") | ||
| assert.Equal(t, test.expectedVal, val, "the last-known-good value must still be served") | ||
|
|
||
| // the source recovers | ||
| state.SetStale(staleSource, false) | ||
|
|
||
| _, _, reason, _, err = e.ResolveBooleanValue(context.TODO(), "default", test.flagKey, test.evalCtx) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, test.freshReason, reason, "reconnecting must restore the original reason") | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| // STALE reports uncertainty about a value that was resolved. An evaluation that | ||
| // failed has no value to be uncertain about, so its error reason must survive. | ||
| func TestStale_DoesNotMaskErrors(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| flagKey string | ||
| errorCode string | ||
| }{ | ||
| {"missing flag", MissingFlag, model.FlagNotFoundErrorCode}, | ||
| {"type mismatch", StaticObjectFlag, model.TypeMismatchErrorCode}, | ||
| } | ||
|
|
||
| for _, test := range tests { | ||
| t.Run(test.name, func(t *testing.T) { | ||
| state := store.NewSourceState() | ||
| state.SetStale(staleSource, true) | ||
| e := staleEvaluator(t, state) | ||
|
|
||
| _, _, reason, _, err := e.ResolveBooleanValue(context.TODO(), "default", test.flagKey, nil) | ||
|
|
||
| assert.EqualError(t, err, test.errorCode) | ||
| assert.Equal(t, model.ErrorReason, reason, "errors must keep ERROR, not be rewritten to STALE") | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestStale_OnlyAffectsTheDisconnectedSource(t *testing.T) { | ||
| state := store.NewSourceState() | ||
| e := staleEvaluator(t, state) | ||
|
|
||
| state.SetStale("some-other-source", true) | ||
|
|
||
| _, _, reason, _, err := e.ResolveBooleanValue(context.TODO(), "default", StaticBoolFlag, nil) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, model.StaticReason, reason, "an unrelated stale source must not affect this flag") | ||
| } | ||
|
|
||
| // Source tracking is opt-in. Without it flagd must behave exactly as it did | ||
| // before stale reporting existed. | ||
| func TestStale_NoSourceStateConfigured(t *testing.T) { | ||
| e := flagdEvaluator.NewJSON(logger.NewLogger(nil, false), store.NewFlags()) | ||
| require.NoError(t, e.SetState(sync.DataSync{FlagData: flagConfig, Source: staleSource})) | ||
|
|
||
| _, _, reason, _, err := e.ResolveBooleanValue(context.TODO(), "default", StaticBoolFlag, nil) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, model.StaticReason, reason) | ||
| } |
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| package store | ||
|
|
||
| import "sync" | ||
|
|
||
| // SourceState tracks, per sync source, whether that source is currently | ||
| // disconnected. Flags already held in the store remain servable while a source | ||
| // is down -- flagd deliberately keeps serving last-known-good data rather than | ||
| // failing evaluations -- but consumers deserve to know the data may be out of | ||
| // date. Evaluations resolved from a disconnected source are reported with | ||
| // model.StaleReason. | ||
| // | ||
| // It is written by the runtime (driven by sync.DataSync payloads) and read on | ||
| // the evaluation hot path, so reads are cheap and lock-free-ish via RWMutex. | ||
| // The zero value is not usable; construct with NewSourceState. | ||
| type SourceState struct { | ||
| mu sync.RWMutex | ||
| stale map[string]bool | ||
| } | ||
|
|
||
| func NewSourceState() *SourceState { | ||
| return &SourceState{stale: map[string]bool{}} | ||
| } | ||
|
|
||
| // SetStale records whether the given source is currently disconnected. | ||
| func (s *SourceState) SetStale(source string, stale bool) { | ||
| if s == nil { | ||
| return | ||
| } | ||
| s.mu.Lock() | ||
| defer s.mu.Unlock() | ||
| if stale { | ||
| s.stale[source] = true | ||
| return | ||
| } | ||
| delete(s.stale, source) | ||
| } | ||
|
|
||
| // IsStale reports whether the given source is currently disconnected. A nil | ||
| // receiver reports false so that callers which never wire up source tracking | ||
| // (tests, embedders) behave exactly as before. | ||
| func (s *SourceState) IsStale(source string) bool { | ||
| if s == nil { | ||
| return false | ||
| } | ||
| s.mu.RLock() | ||
| defer s.mu.RUnlock() | ||
| return s.stale[source] | ||
| } | ||
|
|
||
| // StaleSources returns the sources currently marked disconnected. Intended for | ||
| // diagnostics and tests; order is not guaranteed. | ||
| func (s *SourceState) StaleSources() []string { | ||
| if s == nil { | ||
| return nil | ||
| } | ||
| s.mu.RLock() | ||
| defer s.mu.RUnlock() | ||
| sources := make([]string, 0, len(s.stale)) | ||
| for source := range s.stale { | ||
| sources = append(sources, source) | ||
| } | ||
| return sources | ||
| } |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,72 @@ | ||
| package store | ||
|
|
||
| import "testing" | ||
|
|
||
| func TestSourceState_DefaultsToNotStale(t *testing.T) { | ||
| s := NewSourceState() | ||
|
|
||
| if s.IsStale("grpc://example:8015") { | ||
| t.Fatal("a source with no recorded state must not be reported stale") | ||
| } | ||
| } | ||
|
|
||
| func TestSourceState_SetAndClear(t *testing.T) { | ||
| const source = "grpc://example:8015" | ||
| s := NewSourceState() | ||
|
|
||
| s.SetStale(source, true) | ||
| if !s.IsStale(source) { | ||
| t.Fatal("expected source to be stale after SetStale(true)") | ||
| } | ||
|
|
||
| s.SetStale(source, false) | ||
| if s.IsStale(source) { | ||
| t.Fatal("expected source to be fresh after SetStale(false)") | ||
| } | ||
| if got := len(s.StaleSources()); got != 0 { | ||
| t.Fatalf("expected no stale sources retained, got %d", got) | ||
| } | ||
| } | ||
|
|
||
| func TestSourceState_IsolatesSources(t *testing.T) { | ||
| s := NewSourceState() | ||
| s.SetStale("a", true) | ||
|
|
||
| if !s.IsStale("a") { | ||
| t.Fatal("expected source a to be stale") | ||
| } | ||
| if s.IsStale("b") { | ||
| t.Fatal("marking source a stale must not affect source b") | ||
| } | ||
| } | ||
|
|
||
| // A nil SourceState is the zero-configuration case: embedders and tests that | ||
| // never wire up source tracking must see exactly the pre-existing behaviour | ||
| // rather than a panic. | ||
| func TestSourceState_NilReceiverIsSafe(t *testing.T) { | ||
| var s *SourceState | ||
|
|
||
| s.SetStale("a", true) | ||
| if s.IsStale("a") { | ||
| t.Fatal("a nil SourceState must never report a source stale") | ||
| } | ||
| if s.StaleSources() != nil { | ||
| t.Fatal("a nil SourceState must return no stale sources") | ||
| } | ||
| } | ||
|
|
||
| func TestSourceState_ConcurrentAccess(t *testing.T) { | ||
| s := NewSourceState() | ||
| done := make(chan struct{}) | ||
|
|
||
| go func() { | ||
| for i := 0; i < 1000; i++ { | ||
| s.SetStale("a", i%2 == 0) | ||
| } | ||
| close(done) | ||
| }() | ||
| for i := 0; i < 1000; i++ { | ||
| _ = s.IsStale("a") | ||
| } | ||
| <-done | ||
| } |
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Prevent stale notification delivery from blocking reconnects.
The
selectblocks whendataSyncis full andctxremains active. This stops the retry loop after a stream failure. Add adefaultbranch so delivery is actually best-effort. Add a regression test with a fulldataSyncchannel.Proposed fix
func (g *Sync) notifyStale(ctx context.Context, dataSync chan<- sync.DataSync) { select { case dataSync <- sync.DataSync{Source: g.URI, Stale: true}: case <-ctx.Done(): + default: } }📝 Committable suggestion
🤖 Prompt for AI Agents