diff --git a/.changeset/provider-only-local-evaluation.md b/.changeset/provider-only-local-evaluation.md new file mode 100644 index 000000000..72f490437 --- /dev/null +++ b/.changeset/provider-only-local-evaluation.md @@ -0,0 +1,5 @@ +--- +"posthog-server": patch +--- + +Allow local feature flag evaluation from a shared definition cache without a personal API key when local evaluation is enabled. diff --git a/posthog-server/src/main/java/com/posthog/server/PostHogConfig.kt b/posthog-server/src/main/java/com/posthog/server/PostHogConfig.kt index f224c892d..264e46bee 100644 --- a/posthog-server/src/main/java/com/posthog/server/PostHogConfig.kt +++ b/posthog-server/src/main/java/com/posthog/server/PostHogConfig.kt @@ -126,13 +126,13 @@ public open class PostHogConfig constructor( * Enable local evaluation of feature flags * When enabled, the SDK periodically fetches flag definitions and evaluates flags locally * without making API calls for each flag check. Falls back to API if evaluation is inconclusive. - * Requires personalApiKey to be set. + * Requires personalApiKey or flagDefinitionCacheProvider to be set. * Defaults to false */ public var localEvaluation: Boolean = false, /** - * Personal API key for local evaluation - * Required when localEvaluation is true. + * Personal API key for fetching local-evaluation definitions from PostHog. + * Not required to read definitions from flagDefinitionCacheProvider. * Defaults to null */ personalApiKey: String? = null, @@ -184,8 +184,8 @@ public open class PostHogConfig constructor( public val host: String = host.trim().ifBlank { DEFAULT_HOST } /** - * Personal API key for local evaluation - * Required when localEvaluation is true. + * Personal API key for fetching local-evaluation definitions from PostHog. + * Not required to read definitions from [flagDefinitionCacheProvider]. * Defaults to null */ public var personalApiKey: String? = personalApiKey?.trim()?.ifBlank { null } @@ -194,7 +194,9 @@ public open class PostHogConfig constructor( * Shared cache provider for local-evaluation feature flag definitions. * * This can reduce duplicate definition fetches when multiple SDK instances run in - * the same service. Defaults to null. + * the same service. With [localEvaluation] enabled, instances without [personalApiKey] + * only read cached definitions, without consulting fetch leadership or publishing. + * Direct definition fetches still require [personalApiKey]. Defaults to null. */ public var flagDefinitionCacheProvider: PostHogFlagDefinitionCacheProvider? = null @@ -648,7 +650,7 @@ public open class PostHogConfig constructor( public fun localEvaluation(localEvaluation: Boolean): Builder = apply { this.localEvaluation = localEvaluation } /** - * Sets the personal API key used for local feature flag evaluation. + * Sets the personal API key used to fetch local feature flag definitions from PostHog. * * Setting a non-blank personal API key automatically enables [localEvaluation] unless it has * already been set explicitly. @@ -682,6 +684,7 @@ public open class PostHogConfig constructor( /** * Sets the provider for caching feature flag definitions. + * Enable [localEvaluation] to read cached definitions without a personal API key. * * @param flagDefinitionCacheProvider The cache provider, or null to use the default. * @return This builder. diff --git a/posthog-server/src/main/java/com/posthog/server/PostHogFlagDefinitionCacheProvider.kt b/posthog-server/src/main/java/com/posthog/server/PostHogFlagDefinitionCacheProvider.kt index ead0b16f1..00f30e025 100644 --- a/posthog-server/src/main/java/com/posthog/server/PostHogFlagDefinitionCacheProvider.kt +++ b/posthog-server/src/main/java/com/posthog/server/PostHogFlagDefinitionCacheProvider.kt @@ -6,10 +6,13 @@ import java.util.concurrent.CompletionStage /** * Shared cache provider for local-evaluation feature flag definitions. * - * Implementations usually coordinate leadership across SDK instances. When - * [shouldFetchFlagDefinitions] returns true this instance fetches definitions from + * With local evaluation enabled, instances without a personal API key only read + * [getFlagDefinitions]. They do not call [shouldFetchFlagDefinitions] or publish definitions. + * + * Implementations usually coordinate fetch leadership across instances with a personal API key. + * When [shouldFetchFlagDefinitions] returns true, this instance fetches definitions from * PostHog and receives them through [onFlagDefinitionsReceived]. When it returns - * false this instance reads definitions from [getFlagDefinitions] instead. + * false, this instance reads definitions from [getFlagDefinitions] instead. * * Provider methods return [CompletionStage] so implementations can use either blocking * backends or async clients such as Reactor `Mono.toFuture()`; `Flux` should be reduced @@ -17,9 +20,9 @@ import java.util.concurrent.CompletionStage * these stages in its current synchronous flag-loading flow. * * Provider errors are handled defensively by the SDK: failed reads fall back to the - * API only when no definitions are already loaded, and failed writes/shutdowns are - * logged without failing flag evaluation. A failed write also makes the next fetch - * unconditional, so the SDK retries the write with a full set of definitions. + * API only when no definitions are already loaded and a personal API key is configured. + * Failed writes/shutdowns are logged without failing flag evaluation. A failed write also + * makes the next fetch unconditional, so the SDK retries the write with a full set of definitions. */ public interface PostHogFlagDefinitionCacheProvider { /** @@ -34,6 +37,7 @@ public interface PostHogFlagDefinitionCacheProvider { /** * Return true when this SDK instance should fetch definitions from PostHog. + * Called only when a personal API key is configured. */ public fun shouldFetchFlagDefinitions(): CompletionStage @@ -65,6 +69,7 @@ public abstract class PostHogBlockingFlagDefinitionCacheProvider : PostHogFlagDe /** * Return true when this SDK instance should fetch definitions from PostHog. + * Called only when a personal API key is configured. */ public abstract fun shouldFetchFlagDefinitionsBlocking(): Boolean diff --git a/posthog-server/src/main/java/com/posthog/server/internal/PostHogFeatureFlags.kt b/posthog-server/src/main/java/com/posthog/server/internal/PostHogFeatureFlags.kt index 2f31d3956..eb9ec7212 100644 --- a/posthog-server/src/main/java/com/posthog/server/internal/PostHogFeatureFlags.kt +++ b/posthog-server/src/main/java/com/posthog/server/internal/PostHogFeatureFlags.kt @@ -533,7 +533,7 @@ internal class PostHogFeatureFlags( if (!localEvaluation) { return } - if (personalApiKey == null) { + if (!localEvaluationEnabled()) { logMissingPersonalApiKey() return } @@ -579,6 +579,11 @@ internal class PostHogFeatureFlags( config.logger.log("Flag definition cache empty before initial load, falling back to API") } + if (personalApiKey == null) { + logMissingPersonalApiKey() + return + } + config.logger.log("Loading feature flags for local evaluation") val response = api.localEvaluation(personalApiKey, etag) @@ -639,6 +644,10 @@ internal class PostHogFeatureFlags( } private fun shouldFetchFlagDefinitions(): Boolean { + // Cache-only readers must not claim the provider's fetch leadership. + if (personalApiKey == null) { + return false + } val provider = flagDefinitionCacheProvider ?: return true return awaitFlagDefinitionCacheProvider( errorDescription = "Error in flag definition cache provider shouldFetchFlagDefinitions", @@ -855,7 +864,7 @@ internal class PostHogFeatureFlags( return } - if (personalApiKey == null) { + if (!localEvaluationEnabled()) { logMissingPersonalApiKey() return } @@ -949,11 +958,11 @@ internal class PostHogFeatureFlags( } private fun localEvaluationEnabled(): Boolean { - return localEvaluation && personalApiKey != null + return localEvaluation && (personalApiKey != null || flagDefinitionCacheProvider != null) } private fun logMissingPersonalApiKey() { - config.logger.log("Local evaluation requires a personal API key. This call will be ignored.") + config.logger.log("Fetching feature flag definitions requires a personal API key. This call will be ignored.") } /** @@ -1051,7 +1060,7 @@ internal class PostHogFeatureFlags( return EMPTY_EVALUATE_FLAGS_RESULT } - if (onlyEvaluateLocally && personalApiKey == null) { + if (onlyEvaluateLocally && personalApiKey == null && flagDefinitionCacheProvider == null) { logMissingPersonalApiKey() return EMPTY_EVALUATE_FLAGS_RESULT } diff --git a/posthog-server/src/test/java/com/posthog/server/PostHogFlagDefinitionCacheTest.kt b/posthog-server/src/test/java/com/posthog/server/PostHogFlagDefinitionCacheTest.kt new file mode 100644 index 000000000..9f149ec4f --- /dev/null +++ b/posthog-server/src/test/java/com/posthog/server/PostHogFlagDefinitionCacheTest.kt @@ -0,0 +1,287 @@ +package com.posthog.server + +import com.posthog.PostHogOnFeatureFlags +import okhttp3.mockwebserver.Dispatcher +import okhttp3.mockwebserver.MockResponse +import okhttp3.mockwebserver.MockWebServer +import okhttp3.mockwebserver.RecordedRequest +import java.io.StringReader +import java.util.concurrent.CopyOnWriteArrayList +import java.util.concurrent.CountDownLatch +import java.util.concurrent.TimeUnit +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertFalse +import kotlin.test.assertNotNull +import kotlin.test.assertTrue + +@Suppress("DEPRECATION") +internal class PostHogFlagDefinitionCacheTest { + private fun definitions(rolloutPercentage: Int = 100): Map = + com.posthog.PostHogConfig(TEST_API_KEY).serializer.deserialize( + StringReader(createLocalEvaluationResponse("cached-flag", rolloutPercentage = rolloutPercentage)), + ) + + private fun withProvider( + provider: CacheProvider, + configure: (PostHogConfig) -> Unit = {}, + block: (PostHogInterface) -> Unit, + ) { + val server = MockWebServer() + server.dispatcher = + object : Dispatcher() { + override fun dispatch(request: RecordedRequest): MockResponse = MockResponse().setResponseCode(503) + } + server.start() + var client: PostHogInterface? = null + try { + val config = + PostHogConfig.builder(TEST_API_KEY) + .host(server.url("/").toString()) + .localEvaluation(true) + .personalApiKey(" \t\n") + .flagDefinitionCacheProvider(provider) + .sendFeatureFlagEvent(false) + .pollIntervalSeconds(3600) + .flushIntervalSeconds(3600) + .build() + configure(config) + client = PostHog.with(config) + block(client) + } finally { + try { + client?.close() + assertEquals(0, server.requestCount, "provider-only evaluation must not make HTTP requests") + assertEquals(0, provider.calls.count { it == "shouldFetch" }, "cache-only readers must not claim fetch leadership") + assertEquals(0, provider.calls.count { it == "store" }, "cache-only readers must not publish") + assertEquals(1, provider.calls.count { it == "shutdown" }) + } finally { + server.shutdown() + } + } + } + + @Test + fun `provider-only startup hydrates definitions for public local and legacy evaluation`() { + val provider = CacheProvider(definitions()) + val loaded = CountDownLatch(1) + withProvider(provider, configure = { it.onFeatureFlags = PostHogOnFeatureFlags { loaded.countDown() } }) { client -> + assertTrue(loaded.await(5, TimeUnit.SECONDS), "startup must load cached definitions") + val local = client.evaluateFlags("user-1", onlyEvaluateLocally = true) + assertTrue(local.isEnabled("cached-flag")) + assertNotNull(local.definitionsLoadedAt) + assertTrue(client.evaluateFlags("user-2").isEnabled("cached-flag")) + assertEquals(true, client.getFeatureFlag("user-3", "cached-flag")) + assertTrue(assertNotNull(client.getFeatureFlagResult("user-4", "cached-flag")).enabled) + assertEquals(listOf("get"), provider.calls.toList()) + } + } + + @Test + fun `provider-only first public evaluation loads cached definitions`() { + val provider = CacheProvider(definitions()) + withProvider(provider) { client -> + assertTrue(client.evaluateFlags("user-1", onlyEvaluateLocally = true).isEnabled("cached-flag")) + assertTrue(provider.calls.contains("get")) + } + } + + @Test + fun `provider-only public reload replaces cached definitions`() { + val provider = CacheProvider(definitions()) + val loaded = CountDownLatch(1) + withProvider(provider, configure = { it.onFeatureFlags = PostHogOnFeatureFlags { loaded.countDown() } }) { client -> + assertTrue(loaded.await(5, TimeUnit.SECONDS)) + assertTrue(client.evaluateFlags("user-1").isEnabled("cached-flag")) + provider.data = definitions(rolloutPercentage = 0) + client.reloadFeatureFlags() + assertFalse(client.evaluateFlags("user-1", onlyEvaluateLocally = true).isEnabled("cached-flag")) + assertEquals(listOf("get", "get"), provider.calls.toList()) + } + } + + @Test + fun `provider-only refresh preserves matching version on failure and resets it on fresh omission`() { + val cacheData: Map = + com.posthog.PostHogConfig(TEST_API_KEY).serializer.deserialize( + StringReader( + """ + { + "property_matching_version": 2, + "flags": [{ + "id": 1, "name": "cached-flag", "key": "cached-flag", "active": true, + "filters": {"groups": [{"properties": [ + {"key": "value", "value": false, "operator": "exact", "type": "person"} + ]}]}, "version": 1 + }], + "group_type_mapping": {}, "cohorts": {} + } + """.trimIndent(), + ), + ) + val provider = CacheProvider(cacheData) + val loaded = CountDownLatch(1) + withProvider(provider, configure = { it.onFeatureFlags = PostHogOnFeatureFlags { loaded.countDown() } }) { client -> + assertTrue(loaded.await(5, TimeUnit.SECONDS)) + + fun evaluate(): Boolean { + val snapshot = + client.evaluateFlags("user-1", personProperties = mapOf("value" to "banana"), onlyEvaluateLocally = true) + assertEquals(listOf("cached-flag"), snapshot.keys) + return snapshot.isEnabled("cached-flag") + } + assertFalse(evaluate()) + provider.data = null + client.reloadFeatureFlags() + assertFalse(evaluate()) + provider.failRead = true + client.reloadFeatureFlags() + assertFalse(evaluate()) + provider.failRead = false + provider.shouldFetch = true + client.reloadFeatureFlags() + assertFalse(evaluate()) + provider.shouldFetch = false + provider.data = cacheData - "property_matching_version" + client.reloadFeatureFlags() + assertTrue(evaluate()) + } + } + + @Test + fun `provider-only polling refreshes definitions and shuts down the provider`() { + val provider = CacheProvider(definitions()) + val initialLoad = CountDownLatch(1) + val refreshed = CountDownLatch(1) + withProvider(provider, configure = { config -> + config.pollIntervalSeconds = 1 + config.onFeatureFlags = + PostHogOnFeatureFlags { + if (initialLoad.count > 0) { + provider.data = definitions(rolloutPercentage = 0) + initialLoad.countDown() + } else { + refreshed.countDown() + } + } + }) { client -> + assertTrue(initialLoad.await(5, TimeUnit.SECONDS)) + assertTrue(refreshed.await(5, TimeUnit.SECONDS), "poller must refresh from the provider") + assertFalse(client.evaluateFlags("user-1", onlyEvaluateLocally = true).isEnabled("cached-flag")) + assertTrue(provider.calls.count { it == "get" } >= 2) + } + val callsAfterClose = provider.calls.toList() + assertEquals("shutdown", callsAfterClose.last()) + } + + @Test + fun `provider-only reload preserves valid definitions on empty malformed or failed reads`() { + val provider = CacheProvider(definitions()) + val loaded = CountDownLatch(1) + withProvider(provider, configure = { it.onFeatureFlags = PostHogOnFeatureFlags { loaded.countDown() } }) { client -> + assertTrue(loaded.await(5, TimeUnit.SECONDS)) + val loadedAt = client.evaluateFlags("user-1").definitionsLoadedAt + provider.data = null + client.reloadFeatureFlags() + assertTrue(client.evaluateFlags("user-1", onlyEvaluateLocally = true).isEnabled("cached-flag")) + provider.failRead = true + client.reloadFeatureFlags() + assertTrue(client.evaluateFlags("user-1", onlyEvaluateLocally = true).isEnabled("cached-flag")) + provider.failRead = false + provider.shouldFetch = true + val readsBeforeFetch = provider.calls.count { it == "get" } + client.reloadFeatureFlags() + assertEquals(readsBeforeFetch + 1, provider.calls.count { it == "get" }) + assertTrue(client.evaluateFlags("user-1", onlyEvaluateLocally = true).isEnabled("cached-flag")) + provider.failDecision = true + provider.data = mapOf("flags" to "invalid") + client.reloadFeatureFlags() + val preserved = client.evaluateFlags("user-1", onlyEvaluateLocally = true) + assertTrue(preserved.isEnabled("cached-flag")) + assertEquals(loadedAt, preserved.definitionsLoadedAt) + } + } + + @Test + fun `provider-only empty cache cannot fall back to the definition API`() { + val provider = CacheProvider(null) + withProvider(provider) { client -> + client.reloadFeatureFlags() + assertTrue(client.evaluateFlags("user-1", onlyEvaluateLocally = true).keys.isEmpty()) + assertTrue(provider.calls.contains("get")) + } + } + + @Test + fun `provider-only failed cache read cannot fall back to the definition API`() { + val provider = CacheProvider(null).apply { failRead = true } + withProvider(provider) { client -> + client.reloadFeatureFlags() + assertTrue(client.evaluateFlags("user-1", onlyEvaluateLocally = true).keys.isEmpty()) + assertTrue(provider.calls.contains("get")) + } + } + + @Test + fun `provider-only reader ignores positive fetch decision with a normalized blank key`() { + val provider = CacheProvider(definitions()).apply { shouldFetch = true } + withProvider(provider) { client -> + client.reloadFeatureFlags() + assertTrue(client.evaluateFlags("user-1", onlyEvaluateLocally = true).isEnabled("cached-flag")) + assertTrue(provider.calls.contains("get")) + } + } + + @Test + fun `provider-only reader ignores failing fetch decision`() { + val provider = CacheProvider(definitions()).apply { failDecision = true } + withProvider(provider) { client -> + client.reloadFeatureFlags() + assertTrue(client.evaluateFlags("user-1", onlyEvaluateLocally = true).isEnabled("cached-flag")) + assertTrue(provider.calls.contains("get")) + } + } + + @Test + fun `local evaluation opt out does not consult the definition cache`() { + val provider = CacheProvider(definitions()) + withProvider(provider, configure = { it.localEvaluation = false }) { client -> + client.reloadFeatureFlags() + assertTrue(client.evaluateFlags("user-1", onlyEvaluateLocally = true).keys.isEmpty()) + assertTrue(provider.calls.isEmpty()) + } + } + + private class CacheProvider( + @Volatile var data: Map?, + ) : PostHogBlockingFlagDefinitionCacheProvider() { + val calls = CopyOnWriteArrayList() + + @Volatile var shouldFetch = false + + @Volatile var failRead = false + + @Volatile var failDecision = false + + override fun shouldFetchFlagDefinitionsBlocking(): Boolean { + calls.add("shouldFetch") + if (failDecision) error("fetch decision failed") + return shouldFetch + } + + override fun getFlagDefinitionsBlocking(): Map? { + calls.add("get") + if (failRead) error("cache read failed") + return data + } + + override fun onFlagDefinitionsReceivedBlocking(data: Map) { + calls.add("store") + this.data = data + } + + override fun shutdownBlocking() { + calls.add("shutdown") + } + } +} diff --git a/posthog-server/src/test/java/com/posthog/server/internal/PostHogFeatureFlagsTest.kt b/posthog-server/src/test/java/com/posthog/server/internal/PostHogFeatureFlagsTest.kt index dff366e11..c29c3c03b 100644 --- a/posthog-server/src/test/java/com/posthog/server/internal/PostHogFeatureFlagsTest.kt +++ b/posthog-server/src/test/java/com/posthog/server/internal/PostHogFeatureFlagsTest.kt @@ -1019,7 +1019,7 @@ internal class PostHogFeatureFlagsTest { featureFlags.loadFeatureFlagDefinitions() assertEquals(0, mockServer.requestCount) - assertTrue(logger.containsLog("Local evaluation requires a personal API key")) + assertTrue(logger.containsLog("Fetching feature flag definitions requires a personal API key")) mockServer.shutdown() } @@ -1055,7 +1055,7 @@ internal class PostHogFeatureFlagsTest { assertTrue(result.flags.isEmpty()) assertEquals(0, mockServer.requestCount) - assertTrue(logger.containsLog("Local evaluation requires a personal API key")) + assertTrue(logger.containsLog("Fetching feature flag definitions requires a personal API key")) mockServer.shutdown() } @@ -3003,6 +3003,113 @@ internal class PostHogFeatureFlagsTest { mockServer.shutdown() } + @Test + fun `cache-only reader loads lazily and releases the load lock after empty reads`() { + val logger = TestLogger() + val mockServer = MockWebServer() + mockServer.start() + val config = createTestConfig(logger, mockServer.url("/").toString()) + val provider = + TestFlagDefinitionCacheProvider( + cacheData = createFlagDefinitionCacheData(config, "cached-flag"), + shouldFetch = true, + throwOnShouldFetch = true, + ) + val featureFlags = + PostHogFeatureFlags( + config, + PostHogApi(config), + 60000, + 100, + localEvaluation = true, + personalApiKey = null, + pollerEnabled = false, + flagDefinitionCacheProvider = provider, + ) + try { + assertEquals(true, featureFlags.getFeatureFlag("cached-flag", false, "test-user")) + provider.cacheData = null + featureFlags.loadFeatureFlagDefinitions() + featureFlags.loadFeatureFlagDefinitions() + assertEquals(0, provider.shouldFetchCalls) + assertEquals(3, provider.getCalls) + assertEquals(0, provider.onReceivedCalls) + assertEquals(0, mockServer.requestCount) + assertEquals(true, featureFlags.getFeatureFlag("cached-flag", false, "test-user")) + assertTrue(logger.containsLog("Flag definition cache empty, keeping existing definitions")) + } finally { + featureFlags.shutDown() + mockServer.shutdown() + } + } + + @Test + fun `cache-only reader cannot claim shared leadership before keyed publisher refreshes`() { + val logger = TestLogger() + val mockServer = createMockHttp(jsonResponse(createLocalEvaluationResponse("published-flag"))) + val config = createTestConfig(logger, mockServer.url("/").toString()) + var sharedData: Map = createFlagDefinitionCacheData(config, "cached-flag") + var fetchClaimed = false + var decisionCalls = 0 + var storeCalls = 0 + val provider = + object : PostHogBlockingFlagDefinitionCacheProvider() { + override fun getFlagDefinitionsBlocking(): Map = sharedData + + override fun shouldFetchFlagDefinitionsBlocking(): Boolean { + decisionCalls++ + if (fetchClaimed) return false + fetchClaimed = true + return true + } + + override fun onFlagDefinitionsReceivedBlocking(data: Map) { + storeCalls++ + sharedData = data + } + } + val reader = + PostHogFeatureFlags( + config, + PostHogApi(config), + 60000, + 100, + localEvaluation = true, + pollerEnabled = false, + flagDefinitionCacheProvider = provider, + ) + val publisher = + PostHogFeatureFlags( + config, + PostHogApi(config), + 60000, + 100, + localEvaluation = true, + personalApiKey = "test-personal-key", + pollerEnabled = false, + flagDefinitionCacheProvider = provider, + ) + try { + reader.loadFeatureFlagDefinitions() + publisher.loadFeatureFlagDefinitions() + assertEquals(1, mockServer.requestCount, "reader must leave fetch leadership available to the keyed publisher") + assertTrue(fetchClaimed) + assertEquals(1, decisionCalls) + assertEquals(1, storeCalls) + assertEquals(true, publisher.getFeatureFlag("published-flag", false, "test-user")) + + reader.loadFeatureFlagDefinitions() + assertEquals(true, reader.getFeatureFlag("published-flag", false, "test-user")) + assertEquals(1, decisionCalls, "reader must not claim or renew leadership") + assertEquals(1, storeCalls, "only the keyed publisher stores definitions") + assertEquals(1, mockServer.requestCount) + } finally { + reader.shutDown() + publisher.shutDown() + mockServer.shutdown() + } + } + @Test fun `loadFeatureFlagDefinitions awaits async cached definitions when provider skips fetch`() { val logger = TestLogger()