fix(openfeature): Rust provider improvements - part 2 - #1128
Conversation
|
Review changes with Changed Files
|
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughChangesProvider runtime updates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PollingTask as Polling task
participant WatchTask as Watch task
participant RefreshCoordinator as Refresh coordinator
participant ProviderInnerOperation as Provider inner operation
PollingTask->>RefreshCoordinator: request refresh
WatchTask->>RefreshCoordinator: request refresh
RefreshCoordinator->>ProviderInnerOperation: create one shared refresh
ProviderInnerOperation-->>RefreshCoordinator: return refresh result
RefreshCoordinator-->>PollingTask: share result
RefreshCoordinator-->>WatchTask: share result
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
Follow-up improvements to the Rust OpenFeature provider implementation to make refresh behavior, provider lifecycle handling, and configuration APIs more consistent and reliable across provider modes (local cache vs. remote resolution).
Changes:
- Refactors integration tests into shared scenario/flow helpers and adds coverage for on-demand refresh behavior.
- Strengthens provider correctness and lifecycle handling (single-flight refresh + weakly-held background loops; guard remote resolution before initialization).
- Tightens configuration types (validated
SuperpositionOptions::new, millisecond-only refresh strategies) and improves file-watcher robustness.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/superposition_provider/tests/integration_test.rs | Deduplicates evaluation scenarios and exercises additional provider flows, including on-demand refresh. |
| crates/superposition_provider/src/types.rs | Adds option validation; simplifies refresh strategy types to millisecond-only fields; updates related tests. |
| crates/superposition_provider/src/remote_provider.rs | Adds a readiness guard to avoid remote resolution before initialization. |
| crates/superposition_provider/src/provider.rs | Adapts to SuperpositionOptions::new -> Result (currently via expect). |
| crates/superposition_provider/src/local_provider.rs | Adds single-flight refresh coalescing and prevents background tasks from pinning the provider. |
| crates/superposition_provider/src/lib.rs | Updates internal tests to handle validated SuperpositionOptions::new. |
| crates/superposition_provider/src/data_source/file.rs | Returns typed errors from FileDataSource::new and watches the directory to handle atomic saves reliably. |
| crates/superposition_provider/examples/polling_example.rs | Updates example to handle validated SuperpositionOptions::new and use the updated polling strategy API. |
| crates/superposition_provider/examples/local_with_fallback_example.rs | Updates example to handle validated SuperpositionOptions::new. |
| crates/superposition_provider/examples/local_http_example.rs | Updates example to handle validated SuperpositionOptions::new. |
| crates/superposition_provider/Cargo.toml | Adds futures-util dependency for the shared refresh single-flight implementation. |
| Cargo.lock | Locks the new futures-util dependency. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| AuthMethod::Token(provider_options.token), | ||
| provider_options.org_id, | ||
| provider_options.workspace_id, | ||
| ); | ||
| ) | ||
| .expect("valid Superposition options"); |
| /// Polling strategy configuration. | ||
| /// | ||
| /// Durations are milliseconds. The seconds-based `interval` and `timeout` fields still work and are | ||
| /// deprecated: a `_milliseconds` field wins when set, otherwise the old field is read as seconds. | ||
| /// | ||
| /// `Default` deliberately leaves the `_milliseconds` fields as `None`, so that the common | ||
| /// `PollingStrategy { interval: 30, ..Default::default() }` still means 30 seconds. Defaulting them | ||
| /// to `Some(..)` would let the default silently override the caller's seconds value. | ||
| /// Polling strategy configuration. All durations are milliseconds. |
| pub struct PollingStrategy { | ||
| #[deprecated(note = "seconds-based; use `interval_milliseconds`")] | ||
| pub interval: u64, | ||
| /// How often to refresh, in milliseconds. Wins over `interval` when set. | ||
| pub interval_milliseconds: Option<u64>, | ||
| #[deprecated(note = "seconds-based; use `timeout_milliseconds`")] | ||
| pub timeout: Option<u64>, | ||
| /// How often to refresh, in milliseconds. | ||
| pub interval_milliseconds: u64, | ||
| /// How long a single refresh may take before it is abandoned, in milliseconds. | ||
| /// Wins over `timeout` when set. | ||
| /// `None` means unbounded. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/superposition_provider/src/provider.rs (1)
31-37: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftA public constructor now panics on invalid configuration.
SuperpositionProvider::newis library API. With.expect(...), a blank endpoint, token, org id, or workspace id — values that usually come from environment configuration — aborts the host process instead of returning an error.init()already reports configuration problems asSuperpositionError::ConfigError, so the crate has a non-panicking path for the same class of failure.Propagate the error instead of panicking, and let the caller decide.
♻️ Proposed change
- pub fn new(provider_options: SuperpositionProviderOptions) -> Self { + pub fn new(provider_options: SuperpositionProviderOptions) -> Result<Self> { // Create CAC config let superposition_options = SuperpositionOptions::new( provider_options.endpoint, AuthMethod::Token(provider_options.token), provider_options.org_id, provider_options.workspace_id, - ) - .expect("valid Superposition options"); + )?;The
Self { .. }tail then becomesOk(Self { .. }), and call sites handle theResult. Panicking withexpectinside examples and tests stays appropriate.🤖 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 `@crates/superposition_provider/src/provider.rs` around lines 31 - 37, Update the public constructor SuperpositionProvider::new to propagate the SuperpositionOptions::new error instead of calling expect, change its return value and Self construction to use Result and Ok(Self { .. }), and update production call sites to handle the returned error while leaving example and test panics unchanged.
🧹 Nitpick comments (3)
crates/superposition_provider/src/types.rs (2)
118-125: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
PollingStrategy::newaccepts a zero interval.
new(0)producesinterval_milliseconds == 0.start_pollingincrates/superposition_provider/src/local_provider.rslines 484-493 then callssleep(Duration::from_millis(0))on every iteration and refreshes as fast as the data source answers. The result is a hot refresh loop against the service.Since option construction is now validated, consider rejecting a zero interval here as well, or clamping it to a documented minimum.
🤖 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 `@crates/superposition_provider/src/types.rs` around lines 118 - 125, Update PollingStrategy::new to prevent a zero interval from creating a hot polling loop: reject zero with the established validation behavior or clamp it to a documented positive minimum. Preserve valid nonzero intervals and ensure the resulting interval used by start_polling is always positive.
42-84: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winValidation is bypassable because the fields stay public.
SuperpositionOptions::newnow rejects blank values, but all four fields remainpub, so callers can still build an unvalidated value with a struct literal.crates/superposition_provider/tests/integration_test.rslines 515-527 does exactly that. The validation therefore is advisory, not enforced.If you want the guarantee to hold, make the fields private with read accessors, or add a
#[non_exhaustive]marker so struct-literal construction outside the crate fails to compile.🤖 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 `@crates/superposition_provider/src/types.rs` around lines 42 - 84, Make SuperpositionOptions validation enforceable by preventing external struct-literal construction: either remove public visibility from its fields and provide read accessors, or add #[non_exhaustive] to the type. Update affected callers such as the integration test to use SuperpositionOptions::new and the chosen accessors while preserving the existing validation behavior.crates/superposition_provider/src/local_provider.rs (1)
301-333: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winA cancelled caller can leave the shared refresh half-finished.
Sharedmakes progress only while some clone is polled. If the only caller drops its clone — an on-demand evaluation cancelled by a client timeout, or aselect!branch that loses — the partially-completed future stays inrefresh_in_flight. The next caller seespeek().is_none(), clones it, and resumes work that was started at an arbitrary earlier time. When the strategy timeout indo_refresh_innerhas already elapsed in wall-clock terms, that caller receives an immediate timeout error for a refresh it never actually waited on.To make progress independent of the joiners, drive the refresh on its own task and share the join result.
♻️ Sketch: drive the refresh on a spawned task
_ => { let weak = Arc::downgrade(&self.0); - let fut: BoxFuture<'static, Result<()>> = async move { - match weak.upgrade() { - Some(inner) => { - LocalResolutionProvider(inner).do_refresh_inner().await - } - None => Ok(()), - } - } - .boxed(); + let handle = tokio::spawn(async move { + match weak.upgrade() { + Some(inner) => { + LocalResolutionProvider(inner).do_refresh_inner().await + } + None => Ok(()), + } + }); + let fut: BoxFuture<'static, Result<()>> = async move { + handle.await.unwrap_or_else(|e| { + Err(SuperpositionError::RefreshError(format!( + "Refresh task failed: {e}" + ))) + }) + } + .boxed(); let shared = fut.shared();The spawned task still holds only a
Weak, so the leak guarantee thatleak_testsasserts is preserved.🤖 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 `@crates/superposition_provider/src/local_provider.rs` around lines 301 - 333, Update do_refresh so the underlying refresh is driven by an independent spawned task rather than only by callers polling the Shared future; have the task retain the existing Weak-based provider upgrade and do_refresh_inner execution, and share/join its task result through refresh_in_flight. Preserve the current single-flight behavior, cancellation independence, and no-provider-leak guarantee.
🤖 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 `@crates/superposition_provider/tests/integration_test.rs`:
- Around line 588-598: Update the refresh-strategy construction in the Flow D
setup around run_flow and LocalResolutionProvider::new to use a short
OnDemandStrategy TTL that expires during the test, ensuring the lazy refresh
path executes; apply the same short-interval adjustment to the polling flows
using their default strategy intervals, while preserving the existing flow
coverage and comments.
---
Outside diff comments:
In `@crates/superposition_provider/src/provider.rs`:
- Around line 31-37: Update the public constructor SuperpositionProvider::new to
propagate the SuperpositionOptions::new error instead of calling expect, change
its return value and Self construction to use Result and Ok(Self { .. }), and
update production call sites to handle the returned error while leaving example
and test panics unchanged.
---
Nitpick comments:
In `@crates/superposition_provider/src/local_provider.rs`:
- Around line 301-333: Update do_refresh so the underlying refresh is driven by
an independent spawned task rather than only by callers polling the Shared
future; have the task retain the existing Weak-based provider upgrade and
do_refresh_inner execution, and share/join its task result through
refresh_in_flight. Preserve the current single-flight behavior, cancellation
independence, and no-provider-leak guarantee.
In `@crates/superposition_provider/src/types.rs`:
- Around line 118-125: Update PollingStrategy::new to prevent a zero interval
from creating a hot polling loop: reject zero with the established validation
behavior or clamp it to a documented positive minimum. Preserve valid nonzero
intervals and ensure the resulting interval used by start_polling is always
positive.
- Around line 42-84: Make SuperpositionOptions validation enforceable by
preventing external struct-literal construction: either remove public visibility
from its fields and provide read accessors, or add #[non_exhaustive] to the
type. Update affected callers such as the integration test to use
SuperpositionOptions::new and the chosen accessors while preserving the existing
validation behavior.
🪄 Autofix
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: 092b489b-7cee-4fe1-9cea-001cf7c3df9b
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
crates/superposition_provider/Cargo.tomlcrates/superposition_provider/examples/local_http_example.rscrates/superposition_provider/examples/local_with_fallback_example.rscrates/superposition_provider/examples/polling_example.rscrates/superposition_provider/src/data_source/file.rscrates/superposition_provider/src/lib.rscrates/superposition_provider/src/local_provider.rscrates/superposition_provider/src/provider.rscrates/superposition_provider/src/remote_provider.rscrates/superposition_provider/src/types.rscrates/superposition_provider/tests/integration_test.rs
| // Flow D: LocalResolutionProvider over HTTP with the OnDemand refresh strategy, | ||
| // exercising the lazy TTL refresh path (experiments supported). | ||
| run_flow( | ||
| "LocalResolutionProvider with HTTP data source (on-demand refresh)", | ||
| LocalResolutionProvider::new( | ||
| Box::new(HttpDataSource::new(http_options)), | ||
| None, | ||
| RefreshStrategy::OnDemand(OnDemandStrategy::default()), | ||
| ), | ||
| true, | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The refresh paths are not actually exercised with the default strategies.
OnDemandStrategy::default() sets a 300,000 ms TTL, so no on-demand refresh happens during the run; only the initial init fetch is observed. The comment claims the flow exercises the lazy TTL refresh path. The same applies to the polling flows at lines 531-535 and 577-583, which use the 60,000 ms default interval.
Use short durations so a refresh actually occurs within the test, or adjust the comments to state that only initial resolution is covered.
💚 Proposed change for Flow D
- RefreshStrategy::OnDemand(OnDemandStrategy::default()),
+ // Short TTL so the next evaluation triggers a real on-demand refresh.
+ RefreshStrategy::OnDemand(OnDemandStrategy::new(100)),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Flow D: LocalResolutionProvider over HTTP with the OnDemand refresh strategy, | |
| // exercising the lazy TTL refresh path (experiments supported). | |
| run_flow( | |
| "LocalResolutionProvider with HTTP data source (on-demand refresh)", | |
| LocalResolutionProvider::new( | |
| Box::new(HttpDataSource::new(http_options)), | |
| None, | |
| RefreshStrategy::OnDemand(OnDemandStrategy::default()), | |
| ), | |
| true, | |
| ) | |
| // Flow D: LocalResolutionProvider over HTTP with the OnDemand refresh strategy, | |
| // exercising the lazy TTL refresh path (experiments supported). | |
| run_flow( | |
| "LocalResolutionProvider with HTTP data source (on-demand refresh)", | |
| LocalResolutionProvider::new( | |
| Box::new(HttpDataSource::new(http_options)), | |
| None, | |
| // Short TTL so the next evaluation triggers a real on-demand refresh. | |
| RefreshStrategy::OnDemand(OnDemandStrategy::new(100)), | |
| ), | |
| true, | |
| ) |
🤖 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 `@crates/superposition_provider/tests/integration_test.rs` around lines 588 -
598, Update the refresh-strategy construction in the Flow D setup around
run_flow and LocalResolutionProvider::new to use a short OnDemandStrategy TTL
that expires during the test, ensuring the lazy refresh path executes; apply the
same short-interval adjustment to the polling flows using their default strategy
intervals, while preserving the existing flow coverage and comments.
69d4790 to
e603b8f
Compare
e603b8f to
ad414c7
Compare
Change log
Follow up changes in rust provider for consistency
Summary by CodeRabbit
New Features
Bug Fixes