fix: reset API state on close#1430
Open
Krishan27 wants to merge 2 commits into
Open
Conversation
|
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 (4)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthrough
ChangesFull API state reset on close()
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Implements spec requirement 1.6.2: close() now fully resets all API-managed state after provider shutdown, including providers, global and domain-scoped hooks, event handlers, global evaluation context, domain-scoped evaluation context, and (server) transaction context propagator. clearProviders() remains provider-only for backward compatibility, as guided by the maintainers. Fixes open-feature#1374. Signed-off-by: Krishan Kant Sharma <krishansharma0327@gmail.com>
Krishan27
force-pushed
the
fix/1374-reset-api-state-on-close
branch
from
9ee9e52 to
842e998
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
packages/shared/src/open-feature.ts (1)
422-439:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDeduplicate provider instances before calling
onClose().
_shutdownAllProviders()currently closes the default wrapper provider and every domain wrapper provider independently. If the same provider instance is bound in multiple scopes,onClose()is invoked multiple times. That can trigger double-shutdown side effects for non-idempotent providers.Suggested fix
private async _shutdownAllProviders(): Promise<void> { - try { - await this?._defaultProvider.provider?.onClose?.(); - } catch (err) { - this.handleShutdownError(this._defaultProvider.provider, err); - } - - const wrappers = Array.from(this._domainScopedProviders); - - await Promise.all( - wrappers.map(async ([, wrapper]) => { - try { - await wrapper?.provider.onClose?.(); - } catch (err) { - this.handleShutdownError(wrapper?.provider, err); - } - }), - ); + const uniqueProviders = new Set<P>([ + this._defaultProvider.provider, + ...Array.from(this._domainScopedProviders.values()).map((wrapper) => wrapper.provider), + ]); + + await Promise.all( + Array.from(uniqueProviders).map(async (provider) => { + try { + await provider?.onClose?.(); + } catch (err) { + this.handleShutdownError(provider, err); + } + }), + ); }🤖 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 `@packages/shared/src/open-feature.ts` around lines 422 - 439, In the `_shutdownAllProviders()` method, the same provider instance can have `onClose()` called multiple times if it's bound to multiple scopes (default and domain-scoped). Deduplicate the provider instances by collecting all unique providers from both the default provider and domain-scoped wrappers into a single collection (such as a Set), then iterate through the deduplicated collection once to call `onClose()` on each unique provider instance to avoid triggering double-shutdown side effects.
🧹 Nitpick comments (1)
packages/web/test/open-feature.spec.ts (1)
308-334: ⚡ Quick winAdd an explicit domain-scoped context assertion for
clearProviders().This block validates global context retention but does not lock the domain-scoped context contract. Adding one test here (either “retained” or “cleared,” per intended behavior) will prevent future ambiguity/regressions.
🤖 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 `@packages/web/test/open-feature.spec.ts` around lines 308 - 334, The clearProviders() test suite in the 'does not clear global hooks', 'does not clear global evaluation context', and 'does not remove API-level event handlers' tests validates global-level contract retention but lacks explicit coverage for domain-scoped context behavior. Add a new test case within the describe('clearProviders() remains provider-only') block that sets a domain-specific context, calls clearProviders(), and then explicitly asserts whether the domain-scoped context is retained or cleared as intended by the API contract. This prevents future regressions around domain-scoped context handling.
🤖 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 `@packages/shared/src/open-feature.ts`:
- Around line 422-439: In the `_shutdownAllProviders()` method, the same
provider instance can have `onClose()` called multiple times if it's bound to
multiple scopes (default and domain-scoped). Deduplicate the provider instances
by collecting all unique providers from both the default provider and
domain-scoped wrappers into a single collection (such as a Set), then iterate
through the deduplicated collection once to call `onClose()` on each unique
provider instance to avoid triggering double-shutdown side effects.
---
Nitpick comments:
In `@packages/web/test/open-feature.spec.ts`:
- Around line 308-334: The clearProviders() test suite in the 'does not clear
global hooks', 'does not clear global evaluation context', and 'does not remove
API-level event handlers' tests validates global-level contract retention but
lacks explicit coverage for domain-scoped context behavior. Add a new test case
within the describe('clearProviders() remains provider-only') block that sets a
domain-specific context, calls clearProviders(), and then explicitly asserts
whether the domain-scoped context is retained or cleared as intended by the API
contract. This prevents future regressions around domain-scoped context
handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fdffd2a9-618f-47ff-8731-92f1e997fc7a
📒 Files selected for processing (6)
packages/nest/test/open-feature.module.spec.tspackages/server/src/open-feature.tspackages/server/test/open-feature.spec.tspackages/shared/src/open-feature.tspackages/web/src/open-feature.tspackages/web/test/open-feature.spec.ts
Signed-off-by: Krishan Kant Sharma <krishansharma0327@gmail.com>
MattIPv4
approved these changes
Jun 23, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Fixes #1374.
Updates the
close()shutdown path so it fully resets API-managed state after provider shutdown, including providers, hooks, event handlers, global evaluation context, and transaction-context propagator state.clearProviders()remains provider-only for backward compatibility, matching maintainer guidance.