feat: report STALE reason when a sync source is disconnected - #2017
feat: report STALE reason when a sync source is disconnected#2017scottt732 wants to merge 1 commit into
Conversation
Closes open-feature#400. When a sync source goes offline, flagd keeps serving the flags it already holds -- deliberately, since last-known-good data beats failing evaluations open. Until now nothing distinguished those values from live ones: a flagd whose sync stream had dropped kept answering STATIC/TARGETING_MATCH indefinitely, /readyz stayed 200 (documented to latch on first successful sync and never change), and no metric exposed the condition. Behind a load balancer, one disconnected replica among healthy ones returns different values for the same flag on successive requests, with nothing to indicate which answer is current. This implements the behaviour agreed in open-feature#400, where STALE was added to the OpenFeature specification for exactly this case. - sync.DataSync gains a Stale field. It marks a connection-state notification rather than a flag payload; FlagData is ignored and the store is left untouched. - The gRPC sync emits one when its stream drops, both on the initial failure and on each failed re-establishment, and stops once a payload arrives. The send is best-effort against the context so a blocked runtime can never stall the reconnect loop. - store.SourceState records which sources are currently disconnected. A nil value reports nothing stale, so embedders that do not wire it up keep the previous behaviour exactly. - The runtime marks a source stale on such a notification and clears it on the next successful payload. It does not Emit: no flag data changed, and the sync protocol cannot convey staleness downstream. - The evaluator reports model.StaleReason for flags resolved from a disconnected source, wired via the new evaluator.WithSourceState option. STALE only replaces a successful resolution. Errors keep ERROR -- a failed evaluation has no value to be uncertain about -- and FALLBACK is left alone because it carries internal meaning translated in the API response. Because reasons already surface on feature_flag.flagd.result.reason, this makes a disconnected replica visible per pod with no new telemetry. Verified end to end against two flagd instances: STATIC while connected, STALE with the value still served once the source is killed, and STATIC again on reconnect. Signed-off-by: Scott Holodak <scottt732@gmail.com>
dosubot
Bot
added
the
size:L
✅ Deploy Preview for polite-licorice-3db33c canceled.
|
📝 WalkthroughWalkthroughThe evaluator now tracks per-source connection state. gRPC sync emits stale notifications after stream failures. Runtime preserves stored flag data during disconnection, and successful evaluations use ChangesStale source tracking
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant gRPCSync
participant Runtime
participant SourceState
participant JSONEvaluator
gRPCSync->>Runtime: Send stale DataSync
Runtime->>SourceState: Mark source stale
Runtime->>JSONEvaluator: Preserve existing flag data
JSONEvaluator-->>Runtime: Evaluate retained flag
Runtime-->>gRPCSync: Return STALE reason
gRPCSync->>Runtime: Send successful payload
Runtime->>JSONEvaluator: Update flag data
Runtime->>SourceState: Clear source stale state
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/pkg/sync/grpc/grpc_sync.go`:
- Around line 169-178: Update notifyStale to include a default select branch so
a full dataSync channel causes the stale notification to be dropped immediately,
while preserving delivery when space is available and cancellation handling. Add
a regression test that invokes notifyStale with a full dataSync channel and
verifies it returns without blocking.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ac389090-10fb-4c4e-905b-f491de988d36
📒 Files selected for processing (10)
core/pkg/evaluator/json.gocore/pkg/evaluator/stale_test.gocore/pkg/model/reason.gocore/pkg/store/source_state.gocore/pkg/store/source_state_test.gocore/pkg/sync/grpc/grpc_sync.gocore/pkg/sync/grpc/grpc_sync_test.gocore/pkg/sync/isync.goflagd/pkg/runtime/from_config.goflagd/pkg/runtime/runtime.go
| // notifyStale tells the runtime that this source is disconnected, so evaluations | ||
| // served from its flags can be reported as stale. Flags stay in the store; only | ||
| // the reported reason changes. The send is best-effort: a blocked or cancelled | ||
| // runtime must never stall the reconnection loop. | ||
| func (g *Sync) notifyStale(ctx context.Context, dataSync chan<- sync.DataSync) { | ||
| select { | ||
| case dataSync <- sync.DataSync{Source: g.URI, Stale: true}: | ||
| case <-ctx.Done(): | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Prevent stale notification delivery from blocking reconnects.
The select blocks when dataSync is full and ctx remains active. This stops the retry loop after a stream failure. Add a default branch so delivery is actually best-effort. Add a regression test with a full dataSync channel.
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
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // notifyStale tells the runtime that this source is disconnected, so evaluations | |
| // served from its flags can be reported as stale. Flags stay in the store; only | |
| // the reported reason changes. The send is best-effort: a blocked or cancelled | |
| // runtime must never stall the reconnection loop. | |
| func (g *Sync) notifyStale(ctx context.Context, dataSync chan<- sync.DataSync) { | |
| select { | |
| case dataSync <- sync.DataSync{Source: g.URI, Stale: true}: | |
| case <-ctx.Done(): | |
| } | |
| } | |
| // notifyStale tells the runtime that this source is disconnected, so evaluations | |
| // served from its flags can be reported as stale. Flags stay in the store; only | |
| // the reported reason changes. The send is best-effort: a blocked or cancelled | |
| // runtime must never stall the reconnection loop. | |
| 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: | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/pkg/sync/grpc/grpc_sync.go` around lines 169 - 178, Update notifyStale
to include a default select branch so a full dataSync channel causes the stale
notification to be dropped immediately, while preserving delivery when space is
available and cancellation handling. Add a regression test that invokes
notifyStale with a full dataSync channel and verifies it returns without
blocking.
|
I am not sure that a stal reason is the right approach. I see the problem, and this is definitely something we should help/support/make easier to find. But the reason of the flag evaluation is not STALE. This is the data state and not the evaluation outcome, and i feel like we are mixing domains. Still I have no better solution, but I feel like we should maybe think more carefully. |
|
Thanks, @aepfli. Happy to rework the PR if you have any alternative approaches. In our case, one replica of eight lost its stream, kept serving a flag document from hours earlier, and stayed in the Service endpoints because every health signal said it was fine. I stumbled on #400 which pointed out that STALE is now available as a resolution reason in the openfeature spec. But there are two distinct staleness conditions:
An in-process client can see (1) but is completely blind to (2). If our flagd's upstream sync breaks, every pod using it keeps serving rules downloaded hours earlier, perfectly happily, with no signal available to it at any layer. |
2 participants
Closes #400.
The problem
When a sync source goes offline, flagd keeps serving the flags it already holds. That is the right call — last-known-good data beats failing evaluations open. But until now nothing distinguished those values from live ones:
STATIC/TARGETING_MATCHindefinitely/readyzstays200, documented to latch on the first successful sync and "not change from there on"feature_flag.flagd.sync.active_streamscounts inbound subscribers, not the health of flagd's own sync clientBehind a load balancer this is genuinely hard to diagnose. We hit it in production: one replica of eight lost its stream, kept serving a flag document from hours earlier, and stayed in the Service endpoints because every health signal said it was fine. Successive requests to the same Service returned different values for the same flag, and nothing indicated which answer was current.
This implements the behaviour agreed in #400, where
STALEwas subsequently added to the OpenFeature specification for exactly this case.The change
sync.DataSyncgains aStalefield, marking a connection-state notification rather than a flag payload.FlagDatais ignored and the store is left untouched.store.SourceStaterecords which sources are disconnected. It is nil-safe: a nil value reports nothing stale, so embedders that never wire it up keep the previous behaviour exactly.Emit— no flag data changed, and the sync protocol has no way to convey staleness downstream.model.StaleReasonfor flags resolved from a disconnected source, wired via a newevaluator.WithSourceStateoption.STALEonly ever replaces a successful resolution.ERRORis preserved — a failed evaluation has no value to be uncertain about — andFALLBACKis left alone because it carries internal meaning that is translated in the API response.I implemented this only for the gRPC sync, since that is the push-based case where a silently dead stream is invisible. The same
DataSync.Stalemechanism extends to the other syncs without further plumbing if maintainers would like that in this PR or a follow-up.Why this shape
Because reasons already surface on the
feature_flag.flagd.result.reasonmetric, a disconnected replica becomes visible per pod with no new telemetry — just a new value on an existing attribute. It also reaches SDKs through a spec-defined reason rather than anything flagd-specific.Testing
New:
core/pkg/store/source_state_test.go(including nil-receiver and concurrency) andcore/pkg/evaluator/stale_test.go(reason replacement, error preservation, source isolation, recovery, and the unconfigured no-op case).Two existing gRPC sync tests asserted the data channel was empty after a stream ended, which is no longer true now that a stale notification is emitted; they now assert no further flag payloads arrive, and
Test_SyncRetryadditionally asserts the stale notification is emitted. While there, I fixed a latenterr.Error()on a nilerrin that test which turned an assertion failure into a segfault.Full
coreandflagdsuites pass.Verified end to end with two real flagd instances, one syncing from the other:
Happy to adjust the naming, the
FALLBACKcarve-out, or split the test fixes into their own PR if you'd prefer.