fix(event_executor): update provider status before invoking API-level handlers by SAY-5 · Pull Request #494 · open-feature/go-sdk

SAY-5 · 2026-04-23T20:15:06Z

Fixes #493.

If the provider emits an event, the value of the client's provider status MUST be updated to the status associated with that event before the SDK invokes any event handlers for that event, so that handlers observe a consistent status.

triggerEvent previously dispatched API-level (global) handlers before calling states.Store, so a global handler that queried client state observed the stale status. Client-scoped handlers were fine — states.Store ran just before those.

Fix

Collect the domain writes first (every named domain matching this provider, plus defaultDomain if the handler is the default provider), call states.Store for each, then dispatch API-level → named-domain → default-domain handlers in the same order as before. No change to handler ordering, just the state-write ordering.

Test

go test -tags testtools ./openfeature/... clean.

… handlers triggerEvent dispatched API-level (global) handlers before calling states.Store, so a global handler that queries client state observed the stale status. Client-scoped handlers were fine — states.Store ran just before those. This violated OpenFeature spec requirement 5.3.5: > If the provider emits an event, the value of the client's provider > status MUST be updated to the status associated with that event > BEFORE the SDK invokes any event handlers for that event. Collect the domain writes first (for every named domain matching this provider, plus defaultDomain if the handler is the default provider), call states.Store for each, then dispatch API-level, named-domain, and default-domain handlers in the same order as before. No change to handler ordering, just the state-write ordering. Closes open-feature/go-sdk issue 493. Signed-off-by: SAY-5 <say.apm35@gmail.com>

gemini-code-assist

Code Review

This pull request refactors the triggerEvent function in openfeature/event_executor.go to comply with OpenFeature specification 5.3.5, ensuring provider status is updated before any event handlers are executed. Feedback suggests optimizing the implementation by hoisting redundant function calls, such as newProviderRef and stateFromEvent, out of loops to reduce memory pressure and improve performance.

codecov · 2026-04-23T21:16:59Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.12%. Comparing base (16f2012) to head (1399ca6).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #494      +/-   ##
==========================================
+ Coverage   84.07%   84.12%   +0.04%     
==========================================
  Files          27       27              
  Lines        2079     2085       +6     
==========================================
+ Hits         1748     1754       +6     
  Misses        292      292              
  Partials       39       39

Flag	Coverage Δ
e2e	`84.12% <100.00%> (+0.04%)`	⬆️
unit	`84.12% <100.00%> (+0.04%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

toddbaert · 2026-04-24T15:55:29Z

@SAY-5 Can you add an explicit test for this?

Adds TestEventHandler_StateUpdatedBeforeHandlersRun with three subtests: - API handler observes ReadyState when ProviderReady fires for default provider - client handler observes ErrorState when ProviderError fires for named provider - on stale->ready, handler observes ReadyState (not the previous StaleState) All three fail against the prior ordering (state written after API handlers ran) and pass with the fix in this PR. Signed-off-by: SAY-5 <say.apm35@gmail.com>

SAY-5 · 2026-04-25T05:08:18Z

@toddbaert added TestEventHandler_StateUpdatedBeforeHandlersRun with three subtests: API handler sees ReadyState on default ProviderReady, client handler sees ErrorState on named ProviderError, and stale→ready handler sees ReadyState (not the prior StaleState). I confirmed all three fail against the prior ordering (state written after handlers fired) and pass with this PR's fix.

erka

thank you @SAY-5 for working on this. it looks good.

toddbaert · 2026-04-28T19:18:39Z

@SAY-5 I think this is close! Please consider the remaining comments/issues.

@toddbaert

Per @toddbaert / @erka / @sahidvelji / Gemini reviews: - Hoist `newProviderRef(handler)` out of the loop (it's loop-invariant) and cache `stateFromEvent(event)` in `newState` so we don't re-allocate the reflection-based ref + channel on every iteration. - Drop the eager-preallocation `make([]string, 0)` for `namedDomains` in favour of `var namedDomains []string` — the realistic case is one provider, so paying for an allocation up front isn't worth it. - Trim the spec-5.3.5 rationale comment to two lines. No behaviour change. `make test` clean (open-feature/go-sdk package: 53.5% coverage, full suite green).

SAY-5 · 2026-04-29T20:22:35Z

Round-2 in 29086ca:

Hoisted newProviderRef(handler) out of the loop (loop-invariant — Gemini and @toddbaert spotted this) and cached stateFromEvent(event) in a local newState so we don't re-evaluate per iteration. newProviderRef allocates via reflection + a channel, so this also drops the per-domain allocation pressure.
Switched make([]string, 0) → var namedDomains []string per @erka / @sahidvelji — the realistic case is a single default provider with zero named domains, no point eagerly allocating.
Trimmed the spec-5.3.5 rationale comment to two lines per @toddbaert.

On the use mock providers nudge from @erka — kept the embedded NoopProvider + ProviderEventing shape for now since the test needs both FeatureProvider and EventHandler on the same value, and the gomock pattern (gomock.NewController(t) + MockFeatureProvider per sub-test) adds more setup boilerplate than the NoopProvider literal it replaces. Happy to revisit in a follow-up if you'd prefer that pattern uniformly across event-executor tests.

make test green; TestEventHandler_StateUpdatedBeforeHandlersRun passes all three sub-tests.

@toddbaert

Per @toddbaert / @erka / @sahidvelji / Gemini reviews: - Hoist `newProviderRef(handler)` out of the loop (it's loop-invariant) and cache `stateFromEvent(event)` in `newState` so we don't re-allocate the reflection-based ref + channel on every iteration. - Drop the eager-preallocation `make([]string, 0)` for `namedDomains` in favour of `var namedDomains []string` — the realistic case is one provider, so paying for an allocation up front isn't worth it. - Trim the spec-5.3.5 rationale comment to two lines. No behaviour change. `make test` clean (open-feature/go-sdk package: 53.5% coverage, full suite green). Signed-off-by: SAY-5 <say.apm35@gmail.com>

toddbaert

LGTM - will wait for a review from @erka or @sahidvelji as well.

Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>

erka

Thanks for contributing @SAY-5

SAY-5 requested review from a team as code owners April 23, 2026 20:15

gemini-code-assist Bot reviewed Apr 23, 2026

View reviewed changes

Comment thread openfeature/event_executor.go Outdated

toddbaert reviewed Apr 24, 2026

View reviewed changes

Comment thread openfeature/event_executor.go Outdated

toddbaert requested review from dd-oleksii, erka, marcozabel and sahidvelji April 24, 2026 17:20

sahidvelji reviewed Apr 25, 2026

View reviewed changes

Comment thread openfeature/event_executor.go Outdated

erka reviewed Apr 25, 2026

View reviewed changes

Comment thread openfeature/event_executor_test.go Outdated

Comment thread openfeature/event_executor.go Outdated

toddbaert reviewed Apr 28, 2026

View reviewed changes

Comment thread openfeature/event_executor.go Outdated

SAY-5 force-pushed the fix/trigger-event-state-before-api-handlers-493 branch from 29086ca to 37db71d Compare April 30, 2026 04:17

toddbaert requested review from erka, sahidvelji and toddbaert April 30, 2026 13:13

toddbaert approved these changes Apr 30, 2026

View reviewed changes

use mock provider in tests

1399ca6

Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>

erka approved these changes Apr 30, 2026

View reviewed changes

toddbaert merged commit 42d65a5 into open-feature:main Apr 30, 2026
6 checks passed

github-actions Bot mentioned this pull request Apr 20, 2026

chore(main): release 1.17.3 #490

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(event_executor): update provider status before invoking API-level handlers#494

fix(event_executor): update provider status before invoking API-level handlers#494
toddbaert merged 4 commits into
open-feature:mainfrom
SAY-5:fix/trigger-event-state-before-api-handlers-493

SAY-5 commented Apr 23, 2026

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

Uh oh!

codecov Bot commented Apr 23, 2026 •

edited

Loading

Uh oh!

Uh oh!

toddbaert commented Apr 24, 2026

Uh oh!

Uh oh!

SAY-5 commented Apr 25, 2026

Uh oh!

erka left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

toddbaert commented Apr 28, 2026

Uh oh!

SAY-5 commented Apr 29, 2026

Uh oh!

toddbaert left a comment

Uh oh!

erka left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

SAY-5 commented Apr 23, 2026

Fix

Test

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

codecov Bot commented Apr 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

toddbaert commented Apr 24, 2026

Uh oh!

Uh oh!

SAY-5 commented Apr 25, 2026

Uh oh!

erka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

toddbaert commented Apr 28, 2026

Uh oh!

SAY-5 commented Apr 29, 2026

Uh oh!

toddbaert left a comment

Choose a reason for hiding this comment

Uh oh!

erka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov Bot commented Apr 23, 2026 •

edited

Loading