fix: reset API state on close#1430
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 (6)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthrough
ChangesFull API state reset on close()
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
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
|
@MattIPv4 @beeme1mr Thank you for review, @lukas-reining please review and If there is nothing else to edit in this PR we might can merge it and close it. Thank you again. |
lukas-reining
left a comment
lukas-reining
left a comment
There was a problem hiding this comment.
This looks great beside one thought. Thank you @Krishan27!
| } | ||
| }), | ||
| ); | ||
| await this._shutdownAllProviders(); |
There was a problem hiding this comment.
If handleShutdownError in _shutdownAllProviders throws, the state will not be reset.
In clearProvidersAndSetDefault we have catch for this case.
There was a problem hiding this comment.
@lukas-reining Good catch, thanks! Now I just Wrapped _shutdownAllProviders() in close() with the same try/catch/finally pattern used in clearProvidersAndSetDefault(), so state is always reset even if provider shutdown throws.
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>
Signed-off-by: Krishan Kant Sharma <krishansharma0327@gmail.com>
Wrap _shutdownAllProviders() in try/catch/finally so that hooks, context, event handlers, and emitter are always cleared, matching the pattern already used in clearProvidersAndSetDefault(). Signed-off-by: Krishan Kant Sharma <krishansharma0327@gmail.com>
Krishan27
force-pushed
the
fix/1374-reset-api-state-on-close
branch
from
2a2bc6d to
4549bd0
Compare
3 participants
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.