feat(openfeature): Java openfeature provider new spec#1096
Conversation
|
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 Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR adds local and remote Java OpenFeature providers, shared data-source contracts, file and HTTP implementations, refresh and watch lifecycles, FFI conversion utilities, typed evaluation traits, and provider SDK integration tests. ChangesProvider architecture
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant OpenFeature
participant LocalResolutionProvider
participant SuperpositionDataSource
participant ProviderCache
OpenFeature->>LocalResolutionProvider: evaluate feature
LocalResolutionProvider->>ProviderCache: resolve cached configuration
LocalResolutionProvider->>SuperpositionDataSource: refresh when data is stale
SuperpositionDataSource-->>LocalResolutionProvider: configuration or experiment data
LocalResolutionProvider->>ProviderCache: update cache
ProviderCache-->>OpenFeature: typed evaluation
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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
This PR updates the Java OpenFeature provider implementation to a new spec by introducing pluggable data sources (HTTP + file), adding local and remote resolution providers, and aligning refresh / evaluation behavior with the cross-language (Rust/Python) contract.
Changes:
- Added
LocalResolutionProvider(cached, locally evaluated) andSuperpositionAPIProvider(server-side resolution) plus shared traits for bulk evaluation and experiment metadata. - Introduced
SuperpositionDataSourceabstraction with concreteHttpDataSourceandFileDataSource, including conditional fetch (304) support and file watching. - Expanded/updated test coverage for data sources and local resolution behavior, and added a Kotlin integration test harness to run a shared provider suite.
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| clients/java/provider-sdk-tests/src/main/kotlin/io/juspay/superposition/providertests/Main.kt | Adds a shared OpenFeature provider test suite runner across provider variants. |
| clients/java/provider-sdk-tests/config.toml | Adds a local config fixture used for file fallback testing. |
| clients/java/openfeature-provider/src/test/java/io/juspay/superposition/openfeature/provider/LocalResolutionProviderTest.java | Adds unit tests for local provider initialization, typing, refresh, and watch behavior. |
| clients/java/openfeature-provider/src/test/java/io/juspay/superposition/openfeature/DefaultConfigPopulator.java | Updates test-side SDK calls to match new request/response shapes. |
| clients/java/openfeature-provider/src/test/java/io/juspay/superposition/openfeature/data_source/HttpDataSourceTest.java | Adds HTTP data source tests (304, last-modified round trip, experiment queries). |
| clients/java/openfeature-provider/src/test/java/io/juspay/superposition/openfeature/data_source/FileDataSourceTest.java | Adds file data source tests (TOML/JSON, filtering, watch, unsupported extensions). |
| clients/java/openfeature-provider/src/main/kotlin/io/juspay/superposition/openfeature/FfiUtils.kt | Adds Java-friendly wrappers and conversion helpers around UniFFI and Smithy types. |
| clients/java/openfeature-provider/src/main/kotlin/io/juspay/superposition/openfeature/EvaluationArgs.kt | Extends evaluation args construction to accept UniFFI Config and refines query mapping. |
| clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/traits/JsonValues.java | Adds strict JSON decoding that preserves integer vs float literals for type enforcement. |
| clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/traits/FeatureExperimentMeta.java | Introduces an interface for resolving applicable experiment variants from context. |
| clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/traits/AllFeatureProvider.java | Adds bulk resolution + typed helper implementations with correct OpenFeature error reporting. |
| clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/SuperpositionProviderOptions.java | Removes unused experimentation evaluation-cache options from provider options. |
| clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/SuperpositionOpenFeatureProvider.java | Updates provider to new refresh strategy API and adjusts context merge / variant logic. |
| clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/SuperpositionConfig.java | Removes unused import to align with updated OpenFeature value handling. |
| clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/RefreshJob.java | Updates refresh job duration accessors to new refresh strategy API. |
| clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/provider/SuperpositionAPIProvider.java | Adds a remote-resolution provider that delegates evaluation to the service API. |
| clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/provider/LocalResolutionProvider.java | Adds cached local-resolution provider with refresh strategies, fallback source support, and events. |
| clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/options/SuperpositionOptions.java | Adds core connection options used by data sources/providers. |
| clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/options/RefreshStrategy.java | Refactors refresh strategy into a sealed interface with millisecond-named accessors. |
| clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/options/EvaluationCacheOptions.java | Removes unused evaluation-cache options type. |
| clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/error/SuperpositionError.java | Adds a checked exception with categorized error codes and factory helpers. |
| clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/data_source/WatchStream.java | Adds a per-subscriber watch stream abstraction for change notifications. |
| clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/data_source/SuperpositionDataSource.java | Introduces a common interface for config/experiment data sources and conditional fetching. |
| clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/data_source/HttpDataSource.java | Adds Smithy-based HTTP data source with 304 handling and prefix query workaround. |
| clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/data_source/FileDataSource.java | Adds file-backed data source with FFI parsing/filtering and filesystem watch support. |
| clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/data_source/FetchResponse.java | Adds a typed fetch response wrapper supporting “not modified” semantics. |
| clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/data_source/ExperimentData.java | Adds experiment payload wrapper with fetched timestamp. |
| clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/data_source/ConfigData.java | Adds config payload wrapper with fetched timestamp. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public Optional<T> getOutput() { | ||
| if (!stopped) { | ||
| if (lastUpdated - System.currentTimeMillis() < config.ttl) { | ||
| if (lastUpdated - System.currentTimeMillis() < config.ttlMilliseconds()) { |
| private List<String> getApplicableVariantsInternal(EvaluationContext ctx) throws Exception { | ||
| EvaluationArgs args = getEvaluationArgs(ctx); | ||
| var ctx_ = defaultCtx.isPresent() ? ctx.merge(defaultCtx.get()) : ctx; | ||
| return args.getApplicableVariants(ctx_, getExperimentationArgs(ctx_)); | ||
| var ctx_ = defaultCtx.isPresent() ? defaultCtx.get().merge(ctx) : ctx; | ||
| return args.getApplicableVariants(ctx_, Objects.requireNonNull(getExperimentationArgs(ctx_))); |
| Instant staleBefore = Instant.now().minusMillis(onDemand.ttlMilliseconds()); | ||
| boolean configStale = isStale(cachedConfigData.get(), staleBefore); | ||
| boolean experimentsStale = supportsExperiments() | ||
| && isStale(cachedExperimentData.get(), staleBefore); |
b528177 to
be3453e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
clients/java/provider-sdk-tests/src/main/kotlin/io/juspay/superposition/providertests/Main.kt (1)
709-711: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winKeep a success-path shutdown
main()returns afterMain().run(), but the staticRefreshJob.SEXECis a non-daemon scheduled pool.OpenFeatureAPI.shutdown()stops the poll tasks, not the executor, so this CLI can stay alive after success unless you add an explicit success exit or shut the pool down.🤖 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 `@clients/java/provider-sdk-tests/src/main/kotlin/io/juspay/superposition/providertests/Main.kt` around lines 709 - 711, Update the success path in main() after Main().run() to explicitly terminate the CLI or shut down the non-daemon RefreshJob.SEXEC executor. Ensure successful completion cannot leave the process running, while preserving the existing exception handling path.
🧹 Nitpick comments (12)
clients/java/provider-sdk-tests/src/main/kotlin/io/juspay/superposition/providertests/Main.kt (2)
401-409: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winHardcoded relative path for the file-fallback scenario.
Path.of("config.toml")resolves relative to the JVM's working directory. If this harness is invoked from anywhere other than theprovider-sdk-testsmodule root (e.g., a different CI working directory), the file-fallbackFileDataSourcescenario will fail to find the fixture.🤖 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 `@clients/java/provider-sdk-tests/src/main/kotlin/io/juspay/superposition/providertests/Main.kt` around lines 401 - 409, The file-fallback scenario in runProviderTests uses a working-directory-dependent Path.of("config.toml"). Resolve the fixture path from a stable test-resource or module-root location before constructing FileDataSource, preserving the existing fallback test behavior regardless of the JVM working directory.
416-556: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff
runProviderTestsduplicatesrunDemo's nine test blocks almost verbatim.The same Test 1-9 assertions (default values, platinum/gold/karbik overrides, city overrides, experimentation) already exist in
runDemo(lines 585-708). Any future change to expected values/config now needs to be kept in sync across both methods (andconfig.toml/the SDK-created workspace data). Consider extracting a shared assertion helper parameterized byClientthat bothrunDemoandrunProviderTestscall.🤖 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 `@clients/java/provider-sdk-tests/src/main/kotlin/io/juspay/superposition/providertests/Main.kt` around lines 416 - 556, The test assertions are duplicated between runProviderTests and runDemo, creating two sources of truth. Extract the shared Test 1–9 assertion flow into a helper parameterized by the OpenFeature Client and experimentation flag, then have both runProviderTests and runDemo invoke it while preserving each method’s provider setup and output context.clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/SuperpositionOpenFeatureProvider.java (1)
332-336: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMerge-order fix looks correct.
Switching to
defaultCtx.get().merge(ctx)makes the per-call evaluation context override the global/default context on conflicting keys, which matches the OpenFeature spec's context-merging precedence (invocation-level values win over more general levels). No unit test inProviderTest.javaexercises this precedence for this specific provider (onlyLocalResolutionProviderTesthas an analogous test) — consider adding one here too.Per the OpenFeature docs: "At the point of flag evaluation, the evaluation context is merged, and duplicate values are overwritten as defined in the specification."
🤖 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 `@clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/SuperpositionOpenFeatureProvider.java` around lines 332 - 336, Add a ProviderTest.java unit test covering context precedence for the provider’s evaluation path: configure conflicting values in defaultCtx and the per-call ctx, invoke the method containing the shown queryData construction, and assert the per-call context value is used. Preserve the existing default-context behavior when no per-call value conflicts.clients/java/openfeature-provider/src/test/java/io/juspay/superposition/openfeature/provider/LocalResolutionProviderTest.java (1)
344-353: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a regression test for repeated
initialize()calls.This test's
unwatchableinstance is never shut down after itsinitialize()throws — a live illustration of the leaked-cache-on-failed-init path flagged inLocalResolutionProvider.java(initialize()). Once that's fixed, a test that callsinitialize()twice on the same provider and asserts no duplicate polling/watch threads survive would guard against a regression.🤖 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 `@clients/java/openfeature-provider/src/test/java/io/juspay/superposition/openfeature/provider/LocalResolutionProviderTest.java` around lines 344 - 353, Add a regression test alongside watchStrategyRejectsADataSourceThatCannotWatch using the same LocalResolutionProvider and NonWatchableSource setup, call initialize() twice on the same instance, and assert the second failure leaves no duplicate polling or watch threads. Ensure the test cleans up the provider afterward if initialization creates any resources.clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/provider/LocalResolutionProvider.java (3)
662-686: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConcurrent OnDemand evaluations can each trigger their own redundant refresh.
When cached data is older than the TTL, every evaluating thread that observes the stale state independently calls
refresh()before the first one has updated the cache. Each is harmless (conditional fetch returns "not modified" quickly) but under load this is a thundering-herd pattern against the primary data source at every TTL boundary. Coalescing concurrent refreshes (e.g. a single in-flightFutureshared across callers) would avoid the redundant calls.🤖 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 `@clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/provider/LocalResolutionProvider.java` around lines 662 - 686, Update ensureFreshData to coalesce concurrent OnDemand refreshes by sharing a single in-flight refresh operation among evaluating threads, so only one caller invokes refresh() for a stale cache while others await its result. Preserve the existing useStaleOnError behavior and propagate refresh failures according to onDemand.getUseStaleOnError().
260-311: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winOnDemand TTL isn't enforced when this provider is consumed as a plain data source.
resolveAllFeaturesWithFilter/getApplicableVariantscallensureFreshData()first, butfetchFilteredConfig/fetchActiveExperiments/fetchCandidateActiveExperiments/fetchMatchingActiveExperimentsdon't. A consumer using this instance purely as aSuperpositionDataSource(per the class's stated composability) underRefreshStrategy.OnDemandwould silently get data as stale as the last background/manual refresh, bypassing the TTL guarantee that the typed-evaluation path enforces.🤖 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 `@clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/provider/LocalResolutionProvider.java` around lines 260 - 311, Enforce OnDemand freshness in the data-source fetch methods by invoking the existing ensureFreshData() flow before reading cached data or filtering it. Update fetchFilteredConfig, fetchActiveExperiments, fetchCandidateActiveExperiments, and fetchMatchingActiveExperiments so plain SuperpositionDataSource consumers receive TTL-validated data, while preserving their existing filtering and response behavior.
323-326: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
close()fully shuts down the provider — risky if this instance is nested/shared.The class doc (L55-56) says this provider can be used as a nested
SuperpositionDataSourceby other consumers, andshutdown()itself calls.close()onprimaryDataSource/fallbackDataSource(L161-162, L727-733). If aLocalResolutionProvideris ever passed as another'sprimaryDataSource/fallbackDataSourcewhile also being independently registered as a top-level OpenFeature provider, the outer instance'sshutdown()would tear down the inner one entirely — not just release a filtered-data-source handle. Worth documenting this constraint explicitly (or decoupling "close as a nested data source" from "shut down the whole provider").🤖 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 `@clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/provider/LocalResolutionProvider.java` around lines 323 - 326, Document the lifecycle constraint around LocalResolutionProvider.close() and shutdown(): nested or shared provider instances must not be passed as primaryDataSource or fallbackDataSource when independently managed elsewhere, because shutdown() closes those dependencies. Clarify that close() fully shuts down the provider and its configured data sources, rather than merely releasing a nested data-source handle.clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/traits/AllFeatureProvider.java (2)
103-110: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
SuperpositionErroris flattened to a single generic OpenFeature error code.Every
SuperpositionError, regardless of its ownerrorCode(which distinguishesPROVIDER_ERROR,CONFIG_ERROR,NETWORK_ERROR, etc.), maps todev.openfeature.sdk.ErrorCode.GENERALhere.PROVIDER_ERRORin particular ("provider itself is unusable — typically evaluated before a successful init") maps semantically to OpenFeature'sPROVIDER_NOT_READY, which consumers commonly branch on. Mapping at least that case would give OpenFeature clients a more accurate, spec-aligned signal instead of a catch-all.♻️ Suggested mapping
} catch (SuperpositionError e) { + dev.openfeature.sdk.ErrorCode code = e.getErrorCode() == SuperpositionError.ErrorCode.PROVIDER_ERROR + ? dev.openfeature.sdk.ErrorCode.PROVIDER_NOT_READY + : dev.openfeature.sdk.ErrorCode.GENERAL; return ProviderEvaluation.<T>builder() .variant("error") .reason(Reason.ERROR.name()) - .errorCode(dev.openfeature.sdk.ErrorCode.GENERAL) + .errorCode(code) .errorMessage("Error evaluating flag '" + flagKey + "': " + e.getMessage()) .build(); }🤖 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 `@clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/traits/AllFeatureProvider.java` around lines 103 - 110, Update the SuperpositionError handling in AllFeatureProvider’s evaluation catch block to map the exception’s errorCode to the corresponding OpenFeature ErrorCode instead of always using GENERAL. Ensure PROVIDER_ERROR maps to PROVIDER_NOT_READY, while preserving appropriate mappings for other error types such as CONFIG_ERROR and NETWORK_ERROR.
69-111: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftEvery typed evaluation resolves the entire config.
resolveBool/resolveString/resolveInt/resolveFloat/resolveStructall route throughresolveTyped→resolveAllFeatures, which (perLocalResolutionProvider.resolveAllFeaturesWithFilter) evaluates the whole config on every single-flag lookup. EverygetBooleanEvaluation/getStringEvaluation/etc. call an application makes therefore pays the cost of resolving all flags to read one. IfProviderCachehas (or could have) a single-key evaluation path, using it here would avoid this multiplier on a very hot path (every flag check in the app).🤖 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 `@clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/traits/AllFeatureProvider.java` around lines 69 - 111, Update resolveTyped to resolve only flagKey through the provider’s single-key evaluation path, reusing the existing ProviderCache or equivalent lookup mechanism instead of resolveAllFeatures. Preserve the current extraction, missing-flag, type-mismatch, and SuperpositionError handling behavior.clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/data_source/FileDataSource.java (1)
218-220: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winClose active streams when the watch thread abnormally exits.
If the thread exits cleanly but unexpectedly (e.g., the watched config directory is deleted and
key.reset()returnsfalse), downstream consumer loops pollingWatchStreamwill idle indefinitely becausestream.isClosed()will never evaluate totrue.Consider broadcasting a close event upon thread termination so consumer loops know they can exit.
♻️ Proposed refactor to clear streams on exit
log.debug("FileDataSource: watch thread exiting for {}", filePath); + synchronized (watcherLock) { + // If close() was called explicitly, this evaluates to false and does nothing. + if (ws == watchService) { + subscribers.forEach(WatchStream::close); + subscribers.clear(); + try { ws.close(); } catch (IOException ignored) {} + watchService = null; + } + } }, "superposition-file-watcher-" + filePath.getFileName()); }🤖 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 `@clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/data_source/FileDataSource.java` around lines 218 - 220, Update the watch-thread termination path around the watcher runnable and its final debug log to close or broadcast a close event to every active WatchStream before the thread exits, including clean-but-unexpected termination such as key.reset() returning false. Ensure downstream polling loops observe stream.isClosed() as true while preserving normal event handling.clients/java/openfeature-provider/src/main/kotlin/io/juspay/superposition/openfeature/options/RefreshStrategy.kt (1)
19-24: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove ineffective
@JvmOverloadsannotation.The
Pollingconstructor has no default parameters, making@JvmOverloadsa no-op. Removing it allows a much cleaner standard data class declaration.♻️ Proposed refactor
- data class Polling - `@JvmOverloads` constructor( - override val timeoutMilliseconds: Int, - val intervalMilliseconds: Int, - ) : RefreshStrategy + data class Polling( + override val timeoutMilliseconds: Int, + val intervalMilliseconds: Int, + ) : RefreshStrategy🤖 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 `@clients/java/openfeature-provider/src/main/kotlin/io/juspay/superposition/openfeature/options/RefreshStrategy.kt` around lines 19 - 24, Remove the ineffective `@JvmOverloads` annotation from the Polling constructor in RefreshStrategy, leaving the existing constructor parameters and data class structure unchanged.clients/java/openfeature-provider/src/main/kotlin/io/juspay/superposition/openfeature/data_source/WatchStream.kt (1)
62-69: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShort-circuit
drainEventsif the stream is closed.As a defensive measure, consider short-circuiting
drainEventsif the stream is closed to prevent it from counting theCLOSEDsentinel as a legitimate event or emptying it unnecessarily.🛠️ Proposed fix
fun drainEvents(): Int { + if (isClosed) return 0 var count = 0 while (eventQueue.poll() != null) {🤖 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 `@clients/java/openfeature-provider/src/main/kotlin/io/juspay/superposition/openfeature/data_source/WatchStream.kt` around lines 62 - 69, Update WatchStream.drainEvents to return immediately when the stream is closed, before polling eventQueue. Preserve the existing count-and-discard behavior for open streams, using the class’s existing closed-state indicator.
🤖 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
`@clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/data_source/FileDataSource.java`:
- Around line 66-68: Update formatOf to handle a null result from
filePath.getFileName() before calling toString(), including the expected format
behavior for root paths while preserving the existing extension-based logic for
normal file paths.
- Around line 163-175: Update the WatchService setup block in FileDataSource to
catch all exceptions that can be thrown by newWatchService or dir.register,
including unchecked registration failures, and remove stream from subscribers on
every failure path. Close any allocated watchService before clearing it and
rethrow the failure as the existing SuperpositionError.dataSourceError while
preserving the original cause.
In
`@clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/provider/LocalResolutionProvider.java`:
- Around line 66-96: Update initialize() to stop any existing refresh strategy
and close the current ProviderCache before replacing it, including closing the
newly created cache when initialization fails. Extract the refresh teardown into
stopRefreshStrategy() and reuse it from shutdown(), ensuring pollingTask and
watchThread are cancelled or joined before being overwritten. Preserve
evaluation consistency by avoiding exposure of a newly replaced, unpopulated
cache during re-initialization.
- Around line 147-172: Update LocalResolutionProvider.shutdown() to await
bounded termination of refreshExecutor and refreshWorker after shutdownNow() and
before closing primaryDataSource, fallbackDataSource, or cache. Handle
interruption consistently with the existing shutdown contract, while preserving
resource cleanup even if workers do not terminate within the timeout.
In
`@clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/provider/SuperpositionAPIProvider.java`:
- Around line 87-91: Update SuperpositionAPIProvider.shutdown() to close the
Smithy SDK client before or while marking the provider not ready, using its
SdkClient/AutoCloseable close operation. Preserve the existing shutdown log and
state transition while ensuring client resources are released safely.
In
`@clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/RefreshJob.java`:
- Around line 118-123: Correct the staleness condition in RefreshJob.getOutput:
compare the elapsed time since lastUpdated (System.currentTimeMillis() -
lastUpdated) against config.getTtlMilliseconds(), so refresh runs only when the
cached output has exceeded its TTL. Preserve the existing stopped handling and
refresh flow.
In
`@clients/java/openfeature-provider/src/main/kotlin/io/juspay/superposition/openfeature/data_source/WatchStream.kt`:
- Around line 47-57: Update getNextEvent and tryGetNextEvent in WatchStream to
check isClosed before blocking on eventQueue.take or poll. Return false
immediately when the stream is already closed, while preserving the existing
queue result handling and timeout behavior for open streams.
---
Outside diff comments:
In
`@clients/java/provider-sdk-tests/src/main/kotlin/io/juspay/superposition/providertests/Main.kt`:
- Around line 709-711: Update the success path in main() after Main().run() to
explicitly terminate the CLI or shut down the non-daemon RefreshJob.SEXEC
executor. Ensure successful completion cannot leave the process running, while
preserving the existing exception handling path.
---
Nitpick comments:
In
`@clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/data_source/FileDataSource.java`:
- Around line 218-220: Update the watch-thread termination path around the
watcher runnable and its final debug log to close or broadcast a close event to
every active WatchStream before the thread exits, including clean-but-unexpected
termination such as key.reset() returning false. Ensure downstream polling loops
observe stream.isClosed() as true while preserving normal event handling.
In
`@clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/provider/LocalResolutionProvider.java`:
- Around line 662-686: Update ensureFreshData to coalesce concurrent OnDemand
refreshes by sharing a single in-flight refresh operation among evaluating
threads, so only one caller invokes refresh() for a stale cache while others
await its result. Preserve the existing useStaleOnError behavior and propagate
refresh failures according to onDemand.getUseStaleOnError().
- Around line 260-311: Enforce OnDemand freshness in the data-source fetch
methods by invoking the existing ensureFreshData() flow before reading cached
data or filtering it. Update fetchFilteredConfig, fetchActiveExperiments,
fetchCandidateActiveExperiments, and fetchMatchingActiveExperiments so plain
SuperpositionDataSource consumers receive TTL-validated data, while preserving
their existing filtering and response behavior.
- Around line 323-326: Document the lifecycle constraint around
LocalResolutionProvider.close() and shutdown(): nested or shared provider
instances must not be passed as primaryDataSource or fallbackDataSource when
independently managed elsewhere, because shutdown() closes those dependencies.
Clarify that close() fully shuts down the provider and its configured data
sources, rather than merely releasing a nested data-source handle.
In
`@clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/SuperpositionOpenFeatureProvider.java`:
- Around line 332-336: Add a ProviderTest.java unit test covering context
precedence for the provider’s evaluation path: configure conflicting values in
defaultCtx and the per-call ctx, invoke the method containing the shown
queryData construction, and assert the per-call context value is used. Preserve
the existing default-context behavior when no per-call value conflicts.
In
`@clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/traits/AllFeatureProvider.java`:
- Around line 103-110: Update the SuperpositionError handling in
AllFeatureProvider’s evaluation catch block to map the exception’s errorCode to
the corresponding OpenFeature ErrorCode instead of always using GENERAL. Ensure
PROVIDER_ERROR maps to PROVIDER_NOT_READY, while preserving appropriate mappings
for other error types such as CONFIG_ERROR and NETWORK_ERROR.
- Around line 69-111: Update resolveTyped to resolve only flagKey through the
provider’s single-key evaluation path, reusing the existing ProviderCache or
equivalent lookup mechanism instead of resolveAllFeatures. Preserve the current
extraction, missing-flag, type-mismatch, and SuperpositionError handling
behavior.
In
`@clients/java/openfeature-provider/src/main/kotlin/io/juspay/superposition/openfeature/data_source/WatchStream.kt`:
- Around line 62-69: Update WatchStream.drainEvents to return immediately when
the stream is closed, before polling eventQueue. Preserve the existing
count-and-discard behavior for open streams, using the class’s existing
closed-state indicator.
In
`@clients/java/openfeature-provider/src/main/kotlin/io/juspay/superposition/openfeature/options/RefreshStrategy.kt`:
- Around line 19-24: Remove the ineffective `@JvmOverloads` annotation from the
Polling constructor in RefreshStrategy, leaving the existing constructor
parameters and data class structure unchanged.
In
`@clients/java/openfeature-provider/src/test/java/io/juspay/superposition/openfeature/provider/LocalResolutionProviderTest.java`:
- Around line 344-353: Add a regression test alongside
watchStrategyRejectsADataSourceThatCannotWatch using the same
LocalResolutionProvider and NonWatchableSource setup, call initialize() twice on
the same instance, and assert the second failure leaves no duplicate polling or
watch threads. Ensure the test cleans up the provider afterward if
initialization creates any resources.
In
`@clients/java/provider-sdk-tests/src/main/kotlin/io/juspay/superposition/providertests/Main.kt`:
- Around line 401-409: The file-fallback scenario in runProviderTests uses a
working-directory-dependent Path.of("config.toml"). Resolve the fixture path
from a stable test-resource or module-root location before constructing
FileDataSource, preserving the existing fallback test behavior regardless of the
JVM working directory.
- Around line 416-556: The test assertions are duplicated between
runProviderTests and runDemo, creating two sources of truth. Extract the shared
Test 1–9 assertion flow into a helper parameterized by the OpenFeature Client
and experimentation flag, then have both runProviderTests and runDemo invoke it
while preserving each method’s provider setup and output context.
🪄 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
Run ID: 21536ccb-3123-4ae4-8b24-0ff9d15aedde
📒 Files selected for processing (30)
clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/RefreshJob.javaclients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/SuperpositionConfig.javaclients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/SuperpositionOpenFeatureProvider.javaclients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/SuperpositionProviderOptions.javaclients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/data_source/FileDataSource.javaclients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/data_source/HttpDataSource.javaclients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/data_source/SuperpositionDataSource.javaclients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/options/EvaluationCacheOptions.javaclients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/options/RefreshStrategy.javaclients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/provider/LocalResolutionProvider.javaclients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/provider/SuperpositionAPIProvider.javaclients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/traits/AllFeatureProvider.javaclients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/traits/FeatureExperimentMeta.javaclients/java/openfeature-provider/src/main/kotlin/io/juspay/superposition/openfeature/EvaluationArgs.ktclients/java/openfeature-provider/src/main/kotlin/io/juspay/superposition/openfeature/FfiUtils.ktclients/java/openfeature-provider/src/main/kotlin/io/juspay/superposition/openfeature/data_source/ConfigData.ktclients/java/openfeature-provider/src/main/kotlin/io/juspay/superposition/openfeature/data_source/ExperimentData.ktclients/java/openfeature-provider/src/main/kotlin/io/juspay/superposition/openfeature/data_source/FetchResponse.ktclients/java/openfeature-provider/src/main/kotlin/io/juspay/superposition/openfeature/data_source/WatchStream.ktclients/java/openfeature-provider/src/main/kotlin/io/juspay/superposition/openfeature/error/SuperpositionError.ktclients/java/openfeature-provider/src/main/kotlin/io/juspay/superposition/openfeature/options/RefreshStrategy.ktclients/java/openfeature-provider/src/main/kotlin/io/juspay/superposition/openfeature/options/SuperpositionOptions.ktclients/java/openfeature-provider/src/main/kotlin/io/juspay/superposition/openfeature/traits/JsonValues.ktclients/java/openfeature-provider/src/test/java/io/juspay/superposition/openfeature/DefaultConfigPopulator.javaclients/java/openfeature-provider/src/test/java/io/juspay/superposition/openfeature/ProviderTest.javaclients/java/openfeature-provider/src/test/java/io/juspay/superposition/openfeature/data_source/FileDataSourceTest.javaclients/java/openfeature-provider/src/test/java/io/juspay/superposition/openfeature/data_source/HttpDataSourceTest.javaclients/java/openfeature-provider/src/test/java/io/juspay/superposition/openfeature/provider/LocalResolutionProviderTest.javaclients/java/provider-sdk-tests/config.tomlclients/java/provider-sdk-tests/src/main/kotlin/io/juspay/superposition/providertests/Main.kt
💤 Files with no reviewable changes (4)
- clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/options/RefreshStrategy.java
- clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/options/EvaluationCacheOptions.java
- clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/SuperpositionConfig.java
- clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/SuperpositionProviderOptions.java
| private static String formatOf(Path filePath) throws SuperpositionError { | ||
| String name = filePath.getFileName().toString(); | ||
| int dot = name.lastIndexOf('.'); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Guard against null file name for root paths.
If the filePath resolves to a root directory (e.g., /), getFileName() will return null, causing a NullPointerException when .toString() is invoked.
💚 Proposed fix
private static String formatOf(Path filePath) throws SuperpositionError {
- String name = filePath.getFileName().toString();
+ Path fileName = filePath.getFileName();
+ if (fileName == null) {
+ throw SuperpositionError.dataSourceError("File path must not be a root directory: " + filePath);
+ }
+ String name = fileName.toString();
int dot = name.lastIndexOf('.');📝 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.
| private static String formatOf(Path filePath) throws SuperpositionError { | |
| String name = filePath.getFileName().toString(); | |
| int dot = name.lastIndexOf('.'); | |
| private static String formatOf(Path filePath) throws SuperpositionError { | |
| Path fileName = filePath.getFileName(); | |
| if (fileName == null) { | |
| throw SuperpositionError.dataSourceError("File path must not be a root directory: " + filePath); | |
| } | |
| String name = fileName.toString(); | |
| int dot = name.lastIndexOf('.'); |
🤖 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
`@clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/data_source/FileDataSource.java`
around lines 66 - 68, Update formatOf to handle a null result from
filePath.getFileName() before calling toString(), including the expected format
behavior for root paths while preserving the existing extension-based logic for
normal file paths.
| try { | ||
| watchService = dir.getFileSystem().newWatchService(); | ||
| dir.register( | ||
| watchService, | ||
| StandardWatchEventKinds.ENTRY_CREATE, | ||
| StandardWatchEventKinds.ENTRY_MODIFY, | ||
| StandardWatchEventKinds.ENTRY_DELETE); | ||
| } catch (IOException e) { | ||
| subscribers.remove(stream); | ||
| watchService = null; | ||
| throw SuperpositionError.dataSourceError( | ||
| "Failed to set up file watcher for " + filePath + ": " + e.getMessage(), e); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Fix resource and state leak on WatchService registration failure.
dir.register can throw multiple types of exceptions (e.g., UnsupportedOperationException, IllegalArgumentException, or IOException). The current block only handles IOException, meaning unchecked exceptions will bypass cleanup and permanently orphan the stream in subscribers.
Furthermore, if an exception is caught, the allocated WatchService is abandoned without being closed, resulting in a resource/file descriptor leak.
🛡️ Proposed fix to ensure cleanup on all failure paths
- try {
- watchService = dir.getFileSystem().newWatchService();
- dir.register(
- watchService,
- StandardWatchEventKinds.ENTRY_CREATE,
- StandardWatchEventKinds.ENTRY_MODIFY,
- StandardWatchEventKinds.ENTRY_DELETE);
- } catch (IOException e) {
- subscribers.remove(stream);
- watchService = null;
- throw SuperpositionError.dataSourceError(
- "Failed to set up file watcher for " + filePath + ": " + e.getMessage(), e);
- }
+ WatchService ws = null;
+ try {
+ ws = dir.getFileSystem().newWatchService();
+ dir.register(
+ ws,
+ StandardWatchEventKinds.ENTRY_CREATE,
+ StandardWatchEventKinds.ENTRY_MODIFY,
+ StandardWatchEventKinds.ENTRY_DELETE);
+ watchService = ws;
+ } catch (Exception e) {
+ if (ws != null) {
+ try { ws.close(); } catch (IOException ignored) {}
+ }
+ subscribers.remove(stream);
+ watchService = null;
+ if (e instanceof IOException ioException) {
+ throw SuperpositionError.dataSourceError(
+ "Failed to set up file watcher for " + filePath + ": " + e.getMessage(), ioException);
+ }
+ throw (RuntimeException) e;
+ }📝 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.
| try { | |
| watchService = dir.getFileSystem().newWatchService(); | |
| dir.register( | |
| watchService, | |
| StandardWatchEventKinds.ENTRY_CREATE, | |
| StandardWatchEventKinds.ENTRY_MODIFY, | |
| StandardWatchEventKinds.ENTRY_DELETE); | |
| } catch (IOException e) { | |
| subscribers.remove(stream); | |
| watchService = null; | |
| throw SuperpositionError.dataSourceError( | |
| "Failed to set up file watcher for " + filePath + ": " + e.getMessage(), e); | |
| } | |
| WatchService ws = null; | |
| try { | |
| ws = dir.getFileSystem().newWatchService(); | |
| dir.register( | |
| ws, | |
| StandardWatchEventKinds.ENTRY_CREATE, | |
| StandardWatchEventKinds.ENTRY_MODIFY, | |
| StandardWatchEventKinds.ENTRY_DELETE); | |
| watchService = ws; | |
| } catch (Exception e) { | |
| if (ws != null) { | |
| try { ws.close(); } catch (IOException ignored) {} | |
| } | |
| subscribers.remove(stream); | |
| watchService = null; | |
| if (e instanceof IOException ioException) { | |
| throw SuperpositionError.dataSourceError( | |
| "Failed to set up file watcher for " + filePath + ": " + e.getMessage(), ioException); | |
| } | |
| throw (RuntimeException) e; | |
| } |
🤖 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
`@clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/data_source/FileDataSource.java`
around lines 163 - 175, Update the WatchService setup block in FileDataSource to
catch all exceptions that can be thrown by newWatchService or dir.register,
including unchecked registration failures, and remove stream from subscribers on
every failure path. Close any allocated watchService before clearing it and
rethrow the failure as the existing SuperpositionError.dataSourceError while
preserving the original cause.
| private final SuperpositionDataSource primaryDataSource; | ||
| private final Optional<SuperpositionDataSource> fallbackDataSource; | ||
| private final RefreshStrategy refreshStrategy; | ||
|
|
||
| /** Native cache holding the resolved config and experiments; recreated on each initialize(). */ | ||
| private volatile ProviderCache cache = new ProviderCache(); | ||
|
|
||
| // Caches — updated atomically; null until first successful fetch | ||
| private final AtomicReference<ConfigData> cachedConfigData = new AtomicReference<>(null); | ||
| private final AtomicReference<ExperimentData> cachedExperimentData = new AtomicReference<>(null); | ||
|
|
||
| private final AtomicReference<ProviderState> state = new AtomicReference<>(ProviderState.NOT_READY); | ||
| private volatile EvaluationContext globalContext = new ImmutableContext(); | ||
|
|
||
| // Background refresh | ||
| private final ScheduledExecutorService refreshExecutor = | ||
| Executors.newSingleThreadScheduledExecutor(r -> { | ||
| Thread t = new Thread(r, "superposition-refresh"); | ||
| t.setDaemon(true); | ||
| return t; | ||
| }); | ||
| /** Runs the refresh itself, so a hung data source blocks a worker rather than the caller. */ | ||
| private final ExecutorService refreshWorker = | ||
| Executors.newSingleThreadExecutor(r -> { | ||
| Thread t = new Thread(r, "superposition-refresh-worker"); | ||
| t.setDaemon(true); | ||
| return t; | ||
| }); | ||
| private volatile ScheduledFuture<?> pollingTask = null; | ||
| private volatile Thread watchThread = null; | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
initialize() leaks the native cache and orphans background threads on re-init or failure.
The field doc says the cache is "recreated on each initialize()" — implying repeat calls are expected — but neither the eager field initializer's cache nor a prior successful init's cache is ever closed before this.cache = new ProviderCache() replaces it (L133), and the catch block (L139-141) rethrows without closing the half-populated cache either. Separately, startRefreshStrategy() (L136) calls startPolling/startWatching, which unconditionally overwrite pollingTask/watchThread (L543, L567) — so a second initialize() orphans the first strategy's still-running thread/task forever (nothing ever cancels it, since only shutdown() does that, and its handle is gone). Concurrently, resolveAllFeaturesWithFilter/getApplicableVariants read this.cache on each call — a re-init racing with an in-flight evaluation can hit a freshly-replaced, not-yet-populated cache even while cachedConfigData still (briefly) points at the old data.
🔧 Suggested fix: extract and reuse strategy teardown, close the stale cache
+ private void stopRefreshStrategy() {
+ if (pollingTask != null) {
+ pollingTask.cancel(false);
+ pollingTask = null;
+ }
+ if (watchThread != null) {
+ watchThread.interrupt();
+ watchThread = null;
+ }
+ }
+
`@Override`
public void initialize(EvaluationContext context) throws Exception {
log.debug("Initializing LocalResolutionProvider...");
state.set(ProviderState.NOT_READY);
+ stopRefreshStrategy();
+ ProviderCache previousCache = this.cache;
try {
this.cache = new ProviderCache();
loadInitialConfig();
loadInitialExperiments();
startRefreshStrategy();
this.globalContext = context != null ? context : new ImmutableContext();
state.set(ProviderState.READY);
} catch (Exception e) {
state.set(ProviderState.ERROR);
+ closeQuietly(this.cache, "cache");
throw e;
+ } finally {
+ if (previousCache != null) {
+ previousCache.close();
+ }
}
log.info("LocalResolutionProvider initialized successfully");
}And reuse stopRefreshStrategy() from shutdown() (currently duplicated at L150-157). Note closeQuietly currently only accepts SuperpositionDataSource; it would need a small overload for ProviderCache, or inline a try/catch.
Also applies to: 128-145
🤖 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
`@clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/provider/LocalResolutionProvider.java`
around lines 66 - 96, Update initialize() to stop any existing refresh strategy
and close the current ProviderCache before replacing it, including closing the
newly created cache when initialization fails. Extract the refresh teardown into
stopRefreshStrategy() and reuse it from shutdown(), ensuring pollingTask and
watchThread are cancelled or joined before being overwritten. Preserve
evaluation consistency by avoiding exposure of a newly replaced, unpopulated
cache during re-initialization.
| @Override | ||
| public void shutdown() { | ||
| log.debug("LocalResolutionProvider shutting down..."); | ||
| if (pollingTask != null) { | ||
| pollingTask.cancel(false); | ||
| pollingTask = null; | ||
| } | ||
| if (watchThread != null) { | ||
| watchThread.interrupt(); | ||
| watchThread = null; | ||
| } | ||
| refreshExecutor.shutdownNow(); | ||
| refreshWorker.shutdownNow(); | ||
|
|
||
| closeQuietly(primaryDataSource, "primary"); | ||
| fallbackDataSource.ifPresent(source -> closeQuietly(source, "fallback")); | ||
|
|
||
| cachedConfigData.set(null); | ||
| cachedExperimentData.set(null); | ||
| globalContext = new ImmutableContext(); | ||
| cache.close(); | ||
| state.set(ProviderState.NOT_READY); | ||
| // Detaches the SDK's event listener; without it a shut-down provider keeps a reference. | ||
| super.shutdown(); | ||
| log.info("LocalResolutionProvider shut down"); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/provider/LocalResolutionProvider.java"
wc -l "$FILE"
ast-grep outline "$FILE" --view expanded || true
sed -n '1,260p' "$FILE" | cat -nRepository: juspay/superposition
Length of output: 14552
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/provider/LocalResolutionProvider.java"
# Inspect refresh-related methods and shutdown interactions.
rg -n "refreshExecutor|refreshWorker|pollingTask|watchThread|closeQuietly|cache\.close|shutdown\(" "$FILE"
printf '\n--- refresh methods ---\n'
sed -n '1,220p' "$FILE" | awk '
/refresh|poll|watch|shutdown/ {print NR ":" $0}
'
printf '\n--- focused sections ---\n'
sed -n '80,220p' "$FILE" | cat -nRepository: juspay/superposition
Length of output: 9290
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/provider/LocalResolutionProvider.java"
sed -n '330,640p' "$FILE" | cat -nRepository: juspay/superposition
Length of output: 17094
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/provider/LocalResolutionProvider.java"
# Inspect the refresh helpers and shutdown-related call sites.
sed -n '330,760p' "$FILE" | cat -nRepository: juspay/superposition
Length of output: 21554
Wait for refresh workers before closing shared resources
shutdownNow() only interrupts the refresh executors; if a refresh is stuck in blocking I/O, it can still be using primaryDataSource or cache when closeQuietly(...) / cache.close() run. Add a bounded awaitTermination(...) before releasing those resources.
🤖 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
`@clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/provider/LocalResolutionProvider.java`
around lines 147 - 172, Update LocalResolutionProvider.shutdown() to await
bounded termination of refreshExecutor and refreshWorker after shutdownNow() and
before closing primaryDataSource, fallbackDataSource, or cache. Handle
interruption consistently with the existing shutdown contract, while preserving
resource cleanup even if workers do not terminate within the timeout.
| @Override | ||
| public Optional<T> getOutput() { | ||
| if (!stopped) { | ||
| if (lastUpdated - System.currentTimeMillis() < config.ttl) { | ||
| if (lastUpdated - System.currentTimeMillis() < config.getTtlMilliseconds()) { | ||
| log.debug("Running refresh as current output is stale."); | ||
| var o = RefreshJob.runRefreshWithTimeout(action, config.timeout); | ||
| var o = RefreshJob.runRefreshWithTimeout(action, config.getTimeoutMilliseconds()); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/RefreshJob.java"
printf '\n== Outline ==\n'
ast-grep outline "$FILE" --view expanded || true
printf '\n== Relevant slice ==\n'
nl -ba "$FILE" | sed -n '1,220p'
printf '\n== Search for ttl/lastUpdated/stopped usage ==\n'
rg -n "lastUpdated|ttlMilliseconds|stopped|getOutput\\(" "$FILE"Repository: juspay/superposition
Length of output: 843
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/RefreshJob.java"
printf '\n== Relevant slice ==\n'
nl -ba "$FILE" | sed -n '100,160p'Repository: juspay/superposition
Length of output: 220
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/RefreshJob.java"
printf '\n== Relevant slice 108-140 ==\n'
sed -n '108,140p' "$FILE"
printf '\n== Relevant slice 80-110 ==\n'
sed -n '80,110p' "$FILE"
printf '\n== Search for lastUpdated assignment and ttl usage ==\n'
rg -n "lastUpdated\\s*=|getTtlMilliseconds\\(|ttlMilliseconds|stopped" "$FILE"Repository: juspay/superposition
Length of output: 3330
Fix the TTL comparison
lastUpdated - System.currentTimeMillis() < config.getTtlMilliseconds() is backwards. With a non-negative TTL, this stays true after every refresh, so getOutput() always takes the refresh path and the cache never behaves as intended.
🤖 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
`@clients/java/openfeature-provider/src/main/java/io/juspay/superposition/openfeature/RefreshJob.java`
around lines 118 - 123, Correct the staleness condition in RefreshJob.getOutput:
compare the elapsed time since lastUpdated (System.currentTimeMillis() -
lastUpdated) against config.getTtlMilliseconds(), so refresh runs only when the
cached output has exceeded its TTL. Preserve the existing stopped handling and
refresh flow.
| @Throws(InterruptedException::class) | ||
| fun getNextEvent(): Boolean = eventQueue.take() && !isClosed | ||
|
|
||
| /** | ||
| * Wait for the next event, giving up after [timeoutMs]. | ||
| * | ||
| * @return true if a change occurred, false on timeout or if the stream is closed | ||
| */ | ||
| @Throws(InterruptedException::class) | ||
| fun tryGetNextEvent(timeoutMs: Long): Boolean = | ||
| eventQueue.poll(timeoutMs, TimeUnit.MILLISECONDS) == true && !isClosed |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Prevent indefinite blocking on a closed stream.
If the CLOSED sentinel is consumed by a blocking call (e.g., in a debounce loop), the queue becomes empty. Subsequent calls to getNextEvent or tryGetNextEvent will block (indefinitely or for the timeout) because they check isClosed only after polling the empty queue. In LocalResolutionProvider, this inadvertently delays the watcher thread's shutdown by LIVENESS_CHECK_MS.
Check isClosed before blocking so subsequent calls return immediately.
🛠️ Proposed fix
`@Throws`(InterruptedException::class)
- fun getNextEvent(): Boolean = eventQueue.take() && !isClosed
+ fun getNextEvent(): Boolean {
+ if (isClosed) return false
+ return eventQueue.take() && !isClosed
+ }
/**
* Wait for the next event, giving up after [timeoutMs].
*
* `@return` true if a change occurred, false on timeout or if the stream is closed
*/
`@Throws`(InterruptedException::class)
- fun tryGetNextEvent(timeoutMs: Long): Boolean =
- eventQueue.poll(timeoutMs, TimeUnit.MILLISECONDS) == true && !isClosed
+ fun tryGetNextEvent(timeoutMs: Long): Boolean {
+ if (isClosed) return false
+ return eventQueue.poll(timeoutMs, TimeUnit.MILLISECONDS) == true && !isClosed
+ }📝 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.
| @Throws(InterruptedException::class) | |
| fun getNextEvent(): Boolean = eventQueue.take() && !isClosed | |
| /** | |
| * Wait for the next event, giving up after [timeoutMs]. | |
| * | |
| * @return true if a change occurred, false on timeout or if the stream is closed | |
| */ | |
| @Throws(InterruptedException::class) | |
| fun tryGetNextEvent(timeoutMs: Long): Boolean = | |
| eventQueue.poll(timeoutMs, TimeUnit.MILLISECONDS) == true && !isClosed | |
| `@Throws`(InterruptedException::class) | |
| fun getNextEvent(): Boolean { | |
| if (isClosed) return false | |
| return eventQueue.take() && !isClosed | |
| } | |
| /** | |
| * Wait for the next event, giving up after [timeoutMs]. | |
| * | |
| * `@return` true if a change occurred, false on timeout or if the stream is closed | |
| */ | |
| `@Throws`(InterruptedException::class) | |
| fun tryGetNextEvent(timeoutMs: Long): Boolean { | |
| if (isClosed) return false | |
| return eventQueue.poll(timeoutMs, TimeUnit.MILLISECONDS) == true && !isClosed | |
| } |
🤖 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
`@clients/java/openfeature-provider/src/main/kotlin/io/juspay/superposition/openfeature/data_source/WatchStream.kt`
around lines 47 - 57, Update getNextEvent and tryGetNextEvent in WatchStream to
check isClosed before blocking on eventQueue.take or poll. Return false
immediately when the stream is already closed, while preserving the existing
queue result handling and timeout behavior for open streams.
b9f444f to
a6e6e40
Compare
a6e6e40 to
e4372da
Compare
Java provider in the new spec
Summary by CodeRabbit
New Features
Bug Fixes
Tests