fix: disallow provider registration and initialization after API shutdown - #523
fix: disallow provider registration and initialization after API shutdown#523erka wants to merge 3 commits into
Conversation
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe change adds lifecycle state gating to ChangesProvider lifecycle coordination
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant EvaluationAPI
participant initNew
participant Provider
EvaluationAPI->>initNew: start tracked initialization
EvaluationAPI->>EvaluationAPI: transition to shutdown
EvaluationAPI->>initNew: wait for initialization completion
initNew->>Provider: initialize
EvaluationAPI->>Provider: call Shutdown after initialization settles
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #523 +/- ##
==========================================
+ Coverage 86.55% 86.74% +0.19%
==========================================
Files 22 22
Lines 2119 2135 +16
==========================================
+ Hits 1834 1852 +18
+ Misses 241 240 -1
+ Partials 44 43 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@openfeature/openfeature_api_test.go`:
- Around line 95-128: Make TestProviderInitShutdown coordinate the provider
lifecycle with initStarted and releaseInit channels: have ttprovider.Init signal
initStarted and block until releaseInit, start SetProvider asynchronously, wait
for initStarted before calling Shutdown, assert Shutdown does not return while
Init is blocked, then release Init and verify both operations complete with
Shutdown invoked only after initialization finishes.
In `@openfeature/openfeature_api.go`:
- Around line 282-286: Update the initialization path around the state check and
initializerWithContext call so admission is atomic with shutdown: prevent
Shutdown from transitioning state while the active-state validation and Init
invocation are in progress, using the existing read-lock or equivalent admission
protocol. Ensure no initializerWithContext execution can begin after shutdown
starts, while preserving the existing shutdown error behavior.
- Around line 421-425: The Shutdown path around initWg.Wait must not restore
evaluationAPIStateActive while initialization is still in flight. Replace the
detachable Wait goroutine with a mutex-protected in-flight counter and
completion channel, and have Shutdown select between cancellation and that
completion signal; only restore active after initWg.Wait has returned,
preventing later provider registration from calling initWg.Add during an
outstanding Wait.
🪄 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: 0b67a8b5-702e-4c9d-b53e-0cba8c05c1c9
📒 Files selected for processing (3)
openfeature/isolated_api_test.goopenfeature/openfeature_api.goopenfeature/openfeature_api_test.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openfeature/openfeature_api.go (1)
408-424: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftDo not permanently mark the API shut down when initialization waiting is canceled.
On
ctx.Done(),Shutdownreturns before provider teardown, but leavesevaluationAPIStateShutdownset. Every laterShutdownthen returnsnilimmediately, so in-flight providers may never receiveShutdownand bindings/event resources remain active. Keep a distinct “shutting down” state and let a later call resume or join the pending teardown; only treat the API as fully shut down after cleanup completes.🤖 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 `@openfeature/openfeature_api.go` around lines 408 - 424, The Shutdown flow around evaluationAPIStateShutdown must distinguish shutdown-in-progress from fully completed shutdown. Do not set the terminal shutdown state before waiting on a cancelable context; on ctx.Done(), preserve pending teardown so a later Shutdown call can resume or join it, and transition to evaluationAPIStateShutdown only after provider teardown and binding/event cleanup complete. Ensure repeated calls do not skip cleanup while initialization remains in flight.
🤖 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.
Outside diff comments:
In `@openfeature/openfeature_api.go`:
- Around line 408-424: The Shutdown flow around evaluationAPIStateShutdown must
distinguish shutdown-in-progress from fully completed shutdown. Do not set the
terminal shutdown state before waiting on a cancelable context; on ctx.Done(),
preserve pending teardown so a later Shutdown call can resume or join it, and
transition to evaluationAPIStateShutdown only after provider teardown and
binding/event cleanup complete. Ensure repeated calls do not skip cleanup while
initialization remains in flight.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d6fc3114-c042-42d6-a2f5-6981069239cf
📒 Files selected for processing (1)
openfeature/openfeature_api.go
170aa86 to
fb7c5f4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
openfeature/isolated_api_test.go (1)
400-410: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake the in-flight initialization test deterministic.
The fixed
100msdelay does not proveShutdownentered while initialization was still running; if scheduling lets initialization finish first, the test can pass even if the wait coordination is removed. Use explicit start/release synchronization and assert thatShutdownremains blocked until initialization is released.🤖 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 `@openfeature/isolated_api_test.go` around lines 400 - 410, Make the test around SetProvider and Shutdown deterministic by replacing the fixed initDelay timing with explicit synchronization channels or equivalent start/release signals in testContextAwareProvider. Ensure the test observes that asynchronous initialization has started, invokes Shutdown with an already-cancelled context, and asserts Shutdown remains blocked until the initialization release signal is sent; then release initialization and verify Shutdown completes with the expected active state.
🤖 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 `@openfeature/isolated_api_test.go`:
- Around line 414-421: Remove the conditional “state after shutdown” t.Errorf
from the err assertion block in the shutdown test; retain the standalone
instance.state assertion, which is the authoritative state check.
---
Nitpick comments:
In `@openfeature/isolated_api_test.go`:
- Around line 400-410: Make the test around SetProvider and Shutdown
deterministic by replacing the fixed initDelay timing with explicit
synchronization channels or equivalent start/release signals in
testContextAwareProvider. Ensure the test observes that asynchronous
initialization has started, invokes Shutdown with an already-cancelled context,
and asserts Shutdown remains blocked until the initialization release signal is
sent; then release initialization and verify Shutdown completes with the
expected active state.
🪄 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: ffc53c99-6f63-4d24-bbd4-8b129da924ef
📒 Files selected for processing (2)
openfeature/isolated_api_test.goopenfeature/openfeature_api.go
🚧 Files skipped from review as they are similar to previous changes (1)
- openfeature/openfeature_api.go
…down - track evaluationAPIState (active/shutdown) - guard SetProvider and SetDomainProvider to reject calls after shutdown - wait for in-flight provider initializations before shutting down process Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>
Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>
Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>
282169a to
ae1aad9
Compare
sahidvelji
left a comment
sahidvelji
left a comment
There was a problem hiding this comment.
Automated review pass. The lifecycle gating and the Shutdown() teardown ordering fix look correct, and the added tests are race-clean. Two inline comments, plus one outside-diff note.
These are distinct from CodeRabbit's earlier concurrency comments: those were filed against the earlier implementation that used atomic state.Load()/state.Store() and a detached Wait goroutine. In the current code, state and all initWg operations are accessed under the pre-existing a.mu write lock (state checks in setProvider/setDomainProvider and the transitions + initWg.Wait() in Shutdown all run inside a.mu.Lock() sections; initWg.Add(1) in initNew is only reached from those locked paths). That serialization closes the observe-then-transition and Add-during-Wait races CodeRabbit described, so those comments no longer apply.
Outside-diff note — the provider-replacement path still has issue #495's ordering bug (openfeature_api.go:310-319, shutdownOld). The fix covers the API Shutdown() teardown path via initWg.Wait(), but replacing a provider before its Init finishes is unaffected: SetProvider(A) then SetProvider(B) triggers shutdownOld(ctx, A), which spawns a goroutine calling A.Shutdown()/ShutdownWithContext immediately, with no coordination with A's still-in-flight Init goroutine. So A can receive Shutdown before or during its Init — the exact class of invalid ordering #495 describes. Worth confirming whether #495 should be considered fully closed, or whether the same await-in-flight-init coordination should extend to shutdownOld.
2 participants
This PR
closes #495