fix: provider state ownership via events#385
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the OpenFeature specification to shift the responsibility of emitting lifecycle events from the SDK to the providers. By requiring providers to emit events for status transitions such as initialization and context reconciliation, the SDK can derive provider status directly from the event stream, which helps prevent race conditions in multi-threaded environments. The changes include updated requirements, a new migration guide in Appendix E, and documentation adjustments. Reviewers identified an inconsistency where 'shutdown' was included in lists of methods requiring event emission despite the lack of a defined shutdown event; they provided suggestions to remove 'shutdown' from these requirements and ensure consistent terminology for context changes.
toddbaert
requested review from
aepfli,
beeme1mr,
cupofcat,
erka,
federicobond,
jonathannorris,
kinyoklion,
lukas-reining,
moredip and
nicklasl
toddbaert
force-pushed
the
fix/provider-event-derived-state
branch
from
3177a63 to
4734a58
Compare
toddbaert
force-pushed
the
fix/provider-event-derived-state
branch
from
b37af76 to
6f01fd6
Compare
erka
left a comment
erka
left a comment
There was a problem hiding this comment.
Nice work. Migrating providers will be challenging, but it’s for the best.
Agree |
lukas-reining
left a comment
lukas-reining
left a comment
There was a problem hiding this comment.
This looks good to me! Makes sense to go forward with this.
I left some small questions/remarks.
| > The provider **MUST** emit `PROVIDER_ERROR` if its `initialize` function terminates abnormally. | ||
|
|
||
| If the error is irrecoverable, the error code must indicate `PROVIDER_FATAL`. |
There was a problem hiding this comment.
in Requirement 5.3.5 of the current spec we say:
Some providers may emit events spontaneously, based on changes in their internal state (connections, caches, etc). The SDK must update its internal representation of the provider's state accordingly:
Should we add a requirement here that says that providers may emit a PROVIDER_ERROR at any time? I know that we also not say that they can't but I think it would make it clearer.
This would make the non normative from the beginning more misleading as the provider may error without initialization:
Providers that do not define lifecycle methods or an event emission mechanism cannot emit events by design
There was a problem hiding this comment.
Added a non-normative note in 2.8.5.1's explanation clarifying that spontaneous error emission is still possible and handled per 5.3.5. Let me know if that addresses it.
| The `PROVIDER_CONTEXT_CHANGED` is not emitted from the provider itself; the SDK implementation must run the `PROVIDER_CONTEXT_CHANGED` handlers if the `on context changed` function terminates normally. | ||
| It's possible that the `on context changed` function is invoked simultaneously or in quick succession; in this case the SDK will only run the `PROVIDER_CONTEXT_CHANGED` handlers after all reentrant invocations have terminated, and the last to terminate was successful (terminated normally). | ||
| The provider must emit `PROVIDER_CONTEXT_CHANGED` after it has successfully reconciled the context. | ||
| The `on context changed` function may be invoked simultaneously or in quick succession; the provider must be prepared to handle this case. |
There was a problem hiding this comment.
We are loosening the instruction a bit here around when the PROVIDER_CONTEXT_CHANGED is meant to run, is that deliberate?
Could/should we have that somewhere else?
There was a problem hiding this comment.
I don't think anything was loosened normatively; 5.3.4.2's MUST still encodes both the timing ("terminates normally") and the coalescing ("no other invocations have yet to terminate"). That said, I dropped some of the explanatory restatement when reframing the mechanism around the provider emitting the event, and I think you were right that this made the requirement harder to follow at a glance. I've restored the explanatory clarification; let me know if that helps, or if I'm missing something.
| > The client **MUST** default, run error hooks, and indicate an error if flag resolution is attempted while the provider is in `NOT_READY`. | ||
|
|
||
| The client defaults and returns the `PROVIDER_NOT_READY` `error code` if evaluation is attempted before the provider is initialized (the provider is still in a `NOT_READY` state). | ||
| The SDK avoids calling the provider's resolver functions entirely ("short-circuits") if the provider is in this state. | ||
|
|
||
| see: [error codes](../types.md#error-code), [flag value resolution](./02-providers.md#22-flag-value-resolution) | ||
|
|
||
| #### Requirement 1.7.7 | ||
|
|
||
| > The client **MUST** default, run error hooks, and indicate an error if flag resolution is attempted while the provider is in `FATAL`. | ||
|
|
||
| The client defaults and returns the `PROVIDER_FATAL` `error code` if evaluation is attempted after the provider has transitioned to an irrecoverable error state. | ||
| The SDK avoids calling the provider's resolver functions entirely ("short-circuits") if the provider is in this state. | ||
|
|
||
| see: [error codes](../types.md#error-code), [flag value resolution](./02-providers.md#22-flag-value-resolution) |
There was a problem hiding this comment.
Do we have the client defaults case on evaluation during NOT_READY/FATAL defined somewhere else?
I guess we'll need to drop the "short-circuit" parts here but just want to make sure we keep the definitions on the defaults.
We could consider moving this into 1.4 alongside 1.4.10, without the short circuit of course.
There was a problem hiding this comment.
I think this inherently becomes a provider requirement; it's basically an extension of 2.2.7. We need to get rid of the short-circuit in the SDK because it re-introduces the race (the SDK has to check state to short-circuit, and that state can lag the provider's actual state). The cleanest fix is pushing the responsibility down to the provider, which knows its own state without lag.
I'd lean toward keeping this in section 2 rather than 1.4. The provider is the only party that can answer "am I ready?" without a race, so it makes sense to put the obligation there. We could either add a non-normative note to 2.2.7 or add a new requirement for it. WDYT?
| #### 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, so that handlers observe a consistent status. | ||
| > When a provider emits an event, the SDK **MUST** update the `provider status` to the status associated with that event **before** invoking any event handlers for that event, so that handlers observe a consistent status. |
There was a problem hiding this comment.
The PR description states:
Trivial providers (no lifecycle methods, no event mechanism) are unaffected; the SDK treats them as ready immediately.
This requirement contradicts that.
There was a problem hiding this comment.
I don't understand how this line in particular contradicts the sentence you're quoting. Maybe you meant to make this comment on 5.1.1? If so, let me clarify: the thing 5.1.1 is describing is the abstraction that an implementation (the SDK) must define; it doesn't require every provider to make use of that abstraction.
The things providers themselves must do are generally described in section 2.
Totally willing to work to make this clearer. Please let me know if I've misunderstood.
|
I've not been following along with the gherkin effort, but the re-numbering in this PR would affect some of the gherkin tagging. |
kinyoklion
left a comment
kinyoklion
left a comment
There was a problem hiding this comment.
Approving the idea. There are some inline comments from others that I agree need some clarification.
|
Good catch; I'll update the gherkin tags as a follow-up. Agreed it's better to take the renumbering hit once than to live with permanently incoherent numbering. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 Walkthrough📝 WalkthroughProvider lifecycle status is updated to derive from provider-emitted events, with new reconciliation status coverage, revised event requirements, migration guidance, spec coverage references, and a parser lint rule adjustment. ChangesProvider status and event contract changes
Suggested reviewers: Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 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: 2
🤖 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 `@specification/appendix-e-migrations.md`:
- Around line 16-17: The migration guide uses the wrong normative lifecycle
callback name, which makes it inconsistent with the rest of the spec. Update the
wording in the appendix text that mentions the provider lifecycle callbacks so
it uses the same callback name as the spec, specifically the `on context
changed` callback referenced alongside `initialize` and `shutdown`. Keep the
rest of the explanation unchanged and make sure the corrected name is used
wherever that callback is mentioned in this section.
In `@specification/sections/05-events.md`:
- Around line 34-38: The provider event model in the events section does not
include any way to signal shutdown leading to NOT_READY, so the status mapping
is incomplete. Update the event set and the provider status mapping in this
section to include a shutdown-related event that drives the shutdown ->
NOT_READY transition, or explicitly document that shutdown is excluded from
provider-owned events. Make sure the change is reflected consistently alongside
the existing PROVIDER_READY, PROVIDER_ERROR, PROVIDER_CONFIGURATION_CHANGED,
PROVIDER_STALE, PROVIDER_RECONCILING, and PROVIDER_CONTEXT_CHANGED events.
🪄 Autofix (Beta)
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: dabbe29e-a41b-495e-9928-7f41186cce64
📒 Files selected for processing (7)
specification.jsonspecification/README.mdspecification/appendix-e-migrations.mdspecification/sections/01-flag-evaluation.mdspecification/sections/02-providers.mdspecification/sections/05-events.mdtools/specification_parser/lint_json_output.py
|
Follow-up: the gherkin tags for the affected scenarios (formerly |
|
@kinyoklion following up on the migration-burden concerns you raised in #365 and #265 (the "cognitive load / most intuitive way is wrong / duplicate events" issues) — I've expanded Appendix E to cover concrete migration patterns for the cases you mentioned: blocking init, event-driven init (the LD-style pattern), and context reconciliation coalescing. I also added anti-patterns and made the legacy-path behavior explicit for SDK authors. See the updated appendix. LMK if there are patterns you hit during the LD provider migrations that aren't covered. |
Providers emit events to signal all status transitions; the SDK derives provider status from these events rather than inferring from lifecycle method return values. - Add section 2.8 (Provider status) with requirements 2.8.1-2.8.4 - Add Condition 2.8.5 for simple providers (no lifecycle, no events) - Update 1.7.3/4/5 to reference provider events instead of init outcomes - Change 5.1.1 from MAY to MUST; reframe 5.3.1/2 as event-driven - Update 5.3.5 table (PROVIDER_CONTEXT_CHANGED -> READY, PROVIDER_RECONCILING -> RECONCILING) - Add non-normative threading note to 5.3.3 - Add Appendix E (Migrations) for legacy provider compatibility - Fix lint check for non-requirement entries Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
toddbaert
force-pushed
the
fix/provider-event-derived-state
branch
from
caa8091 to
c9036fb
Compare
|
Merging this; we can make adjustments if needed as we implement. |
| These are emitted only by the SDK during context reconciliation. | ||
| Providers must emit events to signal all state transitions, including those resulting from lifecycle methods (initialize, reconciliation). | ||
| The SDK derives provider status from these events. | ||
| Providers without lifecycle methods or an event emission mechanism cannot emit events by design; see [Condition 2.8.5](./02-providers.md#condition-285). |
There was a problem hiding this comment.
Is this true? My understanding is that lifecycle methods and event emission are currently decoupled and a provider could emit events even if it does not implement lifecycle methods
|
|
||
| **Event-driven initialization.** | ||
| If setup completes asynchronously (e.g. the underlying vendor SDK signals readiness via its own event), forward that signal as `PROVIDER_READY`. | ||
| `initialize` may return before the event is emitted; the SDK treats the return as a synchronization signal only. |
There was a problem hiding this comment.
This recommendation looks like it may break "set provider and wait" methods? What does "synchronization signal" mean?
| > The client's `provider status` accessor **MUST** indicate `NOT_READY` once the `shutdown` function of the associated provider terminates. | ||
|
|
||
| Regardless of the success of the provider's `shutdown` function, the `provider status` should convey the provider is no longer ready to use once the shutdown function terminates. | ||
| Unlike other status transitions, this shutdown is inferred by the SDK; because the SDK initiates the shutdown call, no event from the provider is required. |
There was a problem hiding this comment.
Not directly related to the change here but not having a "not ready" event creates an asymmetry where users can't be notified when a provider becomes not ready. If application author subscribes too all events, they'd still not have the up-to-date status of the provider. #366
7 participants
In short: providers emit events to signal all status transitions; the SDK derives provider status from these events rather than inferring from lifecycle method return values.
This resolves a race condition wherein after the provider's init() completes, but before the SDK has emitted its synthetic READY event, the provider emits some other event, and ordering is not preserved, or messages are lost.
What changes for provider authors:
What changes for SDK authors:
A migration appendix is included for SDKs supporting legacy providers.
Resolves #365
This is an alternative to #380 which took a different approach (adding getState() to providers). That one works but is ~10x larger and introduces concurrent-safety requirements on the provider that don't exist if state lives only in the SDK. Same problem, simpler solution IMO. Happy to discuss tradeoffs.