From 27844b39a5f60935534495b06d311b020868ead8 Mon Sep 17 00:00:00 2001 From: Anna Garcia Date: Wed, 19 Aug 2026 20:22:59 +0200 Subject: [PATCH 1/4] fix(flags): keep displaced callbacks when a queued reload is replaced MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit executeFeatureFlags holds one pending reload behind the request in flight. A third reload overwrote that slot wholesale, so the displaced request's internalOnFeatureFlags and onFeatureFlags were dropped and never invoked — a caller waiting on that callback would wait forever. This is latent, not user-visible. Every executor the SDK constructs is single threaded (PostHog.kt, PostHogStateless.kt) and api.flags blocks on that thread, so two executeFeatureFlags calls cannot overlap on any stock configuration; the queuing branch is unreachable. It was already unreachable when #407 introduced it. Forcing overlap with a multi-threaded executor is the only way to observe the drop, and it loses 8 of 10 callbacks. The pending slot now carries every displaced caller's callbacks: newest parameters still win, but nothing queued loses its completion. That is the shape the cross-SDK spec describes for this mechanism, and matches the fix landing in posthog-ios. The accompanying test injects a multi-threaded executor because that is the only way to reach the branch. It pins the contract the machinery claims, not a scenario the SDK ships. --- .changeset/queued-flags-reload-callbacks.md | 5 +++ .../posthog/internal/PostHogRemoteConfig.kt | 23 ++++++++--- .../internal/PostHogRemoteConfigTest.kt | 38 +++++++++++++++++++ 3 files changed, 60 insertions(+), 6 deletions(-) create mode 100644 .changeset/queued-flags-reload-callbacks.md diff --git a/.changeset/queued-flags-reload-callbacks.md b/.changeset/queued-flags-reload-callbacks.md new file mode 100644 index 000000000..acd1de977 --- /dev/null +++ b/.changeset/queued-flags-reload-callbacks.md @@ -0,0 +1,5 @@ +--- +"posthog": patch +--- + +Carry a displaced caller's callbacks forward when a queued feature flag reload is replaced. `executeFeatureFlags` keeps one pending reload behind the in-flight one, and a third reload overwrote that slot wholesale, so the displaced request's `onFeatureFlags` was never invoked. This is latent rather than user-visible — every executor the SDK uses is single-threaded and `api.flags` blocks on that thread, so two reloads cannot overlap on a stock configuration — but the queuing machinery now honours the contract it was written for. diff --git a/posthog/src/main/java/com/posthog/internal/PostHogRemoteConfig.kt b/posthog/src/main/java/com/posthog/internal/PostHogRemoteConfig.kt index f8b2cc33c..6dcd04d14 100644 --- a/posthog/src/main/java/com/posthog/internal/PostHogRemoteConfig.kt +++ b/posthog/src/main/java/com/posthog/internal/PostHogRemoteConfig.kt @@ -58,8 +58,8 @@ public class PostHogRemoteConfig( val distinctId: String, val anonymousId: String?, val groups: Map?, - val internalOnFeatureFlags: PostHogOnFeatureFlags?, - val onFeatureFlags: PostHogOnFeatureFlags?, + val internalOnFeatureFlags: List, + val onFeatureFlags: List, ) private var pendingFeatureFlagsRequest: PendingFeatureFlagsRequest? = null @@ -232,6 +232,13 @@ public class PostHogRemoteConfig( return recordingActive } + private fun List.asSingleCallback(): PostHogOnFeatureFlags? = + when (size) { + 0 -> null + 1 -> first() + else -> PostHogOnFeatureFlags { forEach { it.loaded() } } + } + private fun runOnFeatureFlagsCallbacks( internalOnFeatureFlags: PostHogOnFeatureFlags?, onFeatureFlags: PostHogOnFeatureFlags?, @@ -705,13 +712,17 @@ public class PostHogRemoteConfig( // This ensures that requests with $anon_distinct_id (from identify()) are not lost synchronized(pendingFeatureFlagsLock) { pendingFeatureFlagsReload.set(true) + // The newest parameters win, but a displaced caller's callbacks are carried over so + // that queuing a reload never loses one. + val displaced = pendingFeatureFlagsRequest pendingFeatureFlagsRequest = PendingFeatureFlagsRequest( distinctId = distinctId, anonymousId = anonymousId, groups = groups, - internalOnFeatureFlags = internalOnFeatureFlags, - onFeatureFlags = onFeatureFlags, + internalOnFeatureFlags = + displaced?.internalOnFeatureFlags.orEmpty() + listOfNotNull(internalOnFeatureFlags), + onFeatureFlags = displaced?.onFeatureFlags.orEmpty() + listOfNotNull(onFeatureFlags), ) } return @@ -915,8 +926,8 @@ public class PostHogRemoteConfig( distinctId = request.distinctId, anonymousId = request.anonymousId, groups = request.groups, - internalOnFeatureFlags = request.internalOnFeatureFlags, - onFeatureFlags = request.onFeatureFlags, + internalOnFeatureFlags = request.internalOnFeatureFlags.asSingleCallback(), + onFeatureFlags = request.onFeatureFlags.asSingleCallback(), ) } } diff --git a/posthog/src/test/java/com/posthog/internal/PostHogRemoteConfigTest.kt b/posthog/src/test/java/com/posthog/internal/PostHogRemoteConfigTest.kt index 7f952925f..22c9f4b7f 100644 --- a/posthog/src/test/java/com/posthog/internal/PostHogRemoteConfigTest.kt +++ b/posthog/src/test/java/com/posthog/internal/PostHogRemoteConfigTest.kt @@ -2744,4 +2744,42 @@ internal class PostHogRemoteConfigTest { sut.clear() http.shutdown() } + + // The queuing branch in executeFeatureFlags is unreachable on the single-threaded executor every + // production path uses, so forcing overlap is the only way to exercise it. This pins the contract + // that machinery claims — no queued reload loses its callback — rather than a shipping scenario. + @Test + fun `queued reloads displaced from the pending slot still run their callbacks`() { + val http = mockHttp(response = MockResponse().setBody(responseFlagsApi)) + repeat(9) { + http.enqueue(MockResponse().setBody(responseFlagsApi).setBodyDelay(300, TimeUnit.MILLISECONDS)) + } + val overlapping = Executors.newFixedThreadPool(4, PostHogThreadFactory("TestOverlap")) + val config = PostHogConfig(API_KEY, http.url("/").toString()).apply { cachePreferences = preferences } + val sut = + PostHogRemoteConfig( + config, + PostHogApi(config), + executor = overlapping, + defaultPersonPropertiesProvider = { emptyMap() }, + onRemoteConfigLoaded = null, + ) + + val reloads = 10 + val fired = CountDownLatch(reloads) + repeat(reloads) { + sut.loadFeatureFlags( + "my_identify", + anonymousId = "anonId", + emptyMap(), + onFeatureFlags = PostHogOnFeatureFlags { fired.countDown() }, + ) + } + + assertTrue(fired.await(30, TimeUnit.SECONDS), "every queued reload must run its callback, ${fired.count} never did") + + sut.clear() + overlapping.shutdownAndAwaitTermination() + http.shutdown() + } } From ddea0d5554a983e52e445e9638e228956f8f1adb Mon Sep 17 00:00:00 2001 From: Anna Garcia Date: Wed, 19 Aug 2026 20:40:20 +0200 Subject: [PATCH 2/4] fix(flags): close the second path that strands a queued reload MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of the first commit found another way to lose a queued reload, and reproduced it with that fix already applied. isLoadingFeatureFlags was claimed and released outside pendingFeatureFlagsLock while the pending slot was written and drained inside it. Two mechanisms, one piece of logical state: a reload could observe "already loading", the in-flight request could then drain an empty queue and clear the flag, and only afterwards would the reload write itself into a slot nothing would ever execute. Its request never went out and its callback never fired — the same failure the queuing machinery exists to prevent, reached by a different door. The claim and the queue write now happen under one lock, and the flag is released inside the same block as the drain. The fan-out also had no error isolation: collapsing N carried callbacks into one meant the first host callback that threw skipped the rest, and a throwing internal callback skipped every user callback. Each now runs behind its own try/catch, which is what "queuing a reload never loses a callback" has to mean. The test asserted at-least-once, so it could not have caught a double invocation; it now counts per callback and asserts exactly one. Its latch drops from 30s to 10s so a regression fails fast instead of stalling CI. Changeset reworded: the reachability claim is "no executor the SDK constructs", not "not user-visible" — remoteConfigProvider is a public hook and a host can supply a pooled executor. --- .changeset/queued-flags-reload-callbacks.md | 2 +- .../posthog/internal/PostHogRemoteConfig.kt | 40 ++++++++++++------- .../internal/PostHogRemoteConfigTest.kt | 13 ++++-- 3 files changed, 37 insertions(+), 18 deletions(-) diff --git a/.changeset/queued-flags-reload-callbacks.md b/.changeset/queued-flags-reload-callbacks.md index acd1de977..aaa56baf5 100644 --- a/.changeset/queued-flags-reload-callbacks.md +++ b/.changeset/queued-flags-reload-callbacks.md @@ -2,4 +2,4 @@ "posthog": patch --- -Carry a displaced caller's callbacks forward when a queued feature flag reload is replaced. `executeFeatureFlags` keeps one pending reload behind the in-flight one, and a third reload overwrote that slot wholesale, so the displaced request's `onFeatureFlags` was never invoked. This is latent rather than user-visible — every executor the SDK uses is single-threaded and `api.flags` blocks on that thread, so two reloads cannot overlap on a stock configuration — but the queuing machinery now honours the contract it was written for. +Carry a displaced caller's callbacks forward when a queued feature flag reload is replaced. `executeFeatureFlags` keeps one pending reload behind the in-flight one, and a third reload overwrote that slot wholesale, so the displaced request's `onFeatureFlags` was never invoked. Also closes a second way a queued reload could be stranded: the in-flight claim and the pending slot were guarded by different mechanisms, so a reload could queue itself just after the in-flight request had drained an empty queue, leaving it with nothing to execute it. Both are latent — every executor the SDK constructs is single-threaded and `api.flags` blocks on that thread, so two reloads cannot overlap unless a host supplies its own pooled executor through `remoteConfigProvider`. diff --git a/posthog/src/main/java/com/posthog/internal/PostHogRemoteConfig.kt b/posthog/src/main/java/com/posthog/internal/PostHogRemoteConfig.kt index 6dcd04d14..968831404 100644 --- a/posthog/src/main/java/com/posthog/internal/PostHogRemoteConfig.kt +++ b/posthog/src/main/java/com/posthog/internal/PostHogRemoteConfig.kt @@ -236,21 +236,25 @@ public class PostHogRemoteConfig( when (size) { 0 -> null 1 -> first() - else -> PostHogOnFeatureFlags { forEach { it.loaded() } } + else -> PostHogOnFeatureFlags { forEach { it.runSafely() } } } + private fun PostHogOnFeatureFlags.runSafely() { + try { + loaded() + } catch (e: Throwable) { + config.logger.log("Executing the feature flags callback failed: $e") + } + } + private fun runOnFeatureFlagsCallbacks( internalOnFeatureFlags: PostHogOnFeatureFlags?, onFeatureFlags: PostHogOnFeatureFlags?, ) { // if we don't load the feature flags (because there are none), we need to call the callback // because the app might be waiting for it. - try { - internalOnFeatureFlags?.loaded() - onFeatureFlags?.loaded() - } catch (e: Throwable) { - config.logger.log("Executing the feature flags callback failed: $e") - } + internalOnFeatureFlags?.runSafely() + onFeatureFlags?.runSafely() } // Notifies that a remote config resolution attempt finished. Callers set @@ -706,14 +710,17 @@ public class PostHogRemoteConfig( return } - if (isLoadingFeatureFlags.getAndSet(true)) { - config.logger.log("Feature flags are being loaded already, queuing reload.") - // Queue the reload request instead of dropping it - // This ensures that requests with $anon_distinct_id (from identify()) are not lost - synchronized(pendingFeatureFlagsLock) { + // Claiming the in-flight slot and queuing behind it must happen under one lock: otherwise a + // reload can observe "already loading", have the in-flight request drain an empty queue, and + // only then write itself into a slot nothing will ever drain. + val queued: Boolean + synchronized(pendingFeatureFlagsLock) { + queued = isLoadingFeatureFlags.getAndSet(true) + if (queued) { pendingFeatureFlagsReload.set(true) // The newest parameters win, but a displaced caller's callbacks are carried over so - // that queuing a reload never loses one. + // that queuing a reload never loses one. The internal callback may repeat in this + // list, so it must stay idempotent. val displaced = pendingFeatureFlagsRequest pendingFeatureFlagsRequest = PendingFeatureFlagsRequest( @@ -725,6 +732,11 @@ public class PostHogRemoteConfig( onFeatureFlags = displaced?.onFeatureFlags.orEmpty() + listOfNotNull(onFeatureFlags), ) } + } + if (queued) { + // Queue the reload request instead of dropping it + // This ensures that requests with $anon_distinct_id (from identify()) are not lost + config.logger.log("Feature flags are being loaded already, queuing reload.") return } @@ -917,8 +929,8 @@ public class PostHogRemoteConfig( } else { pendingRequest = null } + isLoadingFeatureFlags.set(false) } - isLoadingFeatureFlags.set(false) pendingRequest?.let { request -> config.logger.log("Executing pending feature flags reload.") diff --git a/posthog/src/test/java/com/posthog/internal/PostHogRemoteConfigTest.kt b/posthog/src/test/java/com/posthog/internal/PostHogRemoteConfigTest.kt index 22c9f4b7f..001e1599f 100644 --- a/posthog/src/test/java/com/posthog/internal/PostHogRemoteConfigTest.kt +++ b/posthog/src/test/java/com/posthog/internal/PostHogRemoteConfigTest.kt @@ -2767,16 +2767,23 @@ internal class PostHogRemoteConfigTest { val reloads = 10 val fired = CountDownLatch(reloads) - repeat(reloads) { + val counts = List(reloads) { AtomicInteger(0) } + repeat(reloads) { index -> sut.loadFeatureFlags( "my_identify", anonymousId = "anonId", emptyMap(), - onFeatureFlags = PostHogOnFeatureFlags { fired.countDown() }, + onFeatureFlags = + PostHogOnFeatureFlags { + counts[index].incrementAndGet() + fired.countDown() + }, ) } - assertTrue(fired.await(30, TimeUnit.SECONDS), "every queued reload must run its callback, ${fired.count} never did") + assertTrue(fired.await(10, TimeUnit.SECONDS), "every queued reload must run its callback, ${fired.count} never did") + // at-least-once is not enough: a displaced callback must not be invoked twice either + counts.forEachIndexed { index, count -> assertEquals(1, count.get(), "callback $index ran ${count.get()} times") } sut.clear() overlapping.shutdownAndAwaitTermination() From 9a2be99c00c8d85b62e1b6927f29537fba16a8d0 Mon Sep 17 00:00:00 2001 From: Anna Garcia Date: Wed, 19 Aug 2026 20:55:24 +0200 Subject: [PATCH 3/4] chore(flags): write the changeset for release notes, not reviewers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The changeset body is published verbatim into CHANGELOG.md, so it is read by a developer skimming release notes to decide whether to upgrade — not by this PR's reviewer. It carried a paragraph of root cause, internal function names and concurrency rationale, all of which belongs in the PR body and is already there. Reduced to the one user-observable line. Also trims the code comments to the parts that are not visible from the code: the reason the lock has to cover both the claim and the queue write, the idempotency requirement on the repeated internal callback, and why the test needs an executor no production path uses. Dropped the ones restating what the next line does. --- .changeset/queued-flags-reload-callbacks.md | 2 +- .../com/posthog/internal/PostHogRemoteConfig.kt | 13 +++++-------- .../com/posthog/internal/PostHogRemoteConfigTest.kt | 6 ++---- 3 files changed, 8 insertions(+), 13 deletions(-) diff --git a/.changeset/queued-flags-reload-callbacks.md b/.changeset/queued-flags-reload-callbacks.md index aaa56baf5..b6c42b54a 100644 --- a/.changeset/queued-flags-reload-callbacks.md +++ b/.changeset/queued-flags-reload-callbacks.md @@ -2,4 +2,4 @@ "posthog": patch --- -Carry a displaced caller's callbacks forward when a queued feature flag reload is replaced. `executeFeatureFlags` keeps one pending reload behind the in-flight one, and a third reload overwrote that slot wholesale, so the displaced request's `onFeatureFlags` was never invoked. Also closes a second way a queued reload could be stranded: the in-flight claim and the pending slot were guarded by different mechanisms, so a reload could queue itself just after the in-flight request had drained an empty queue, leaving it with nothing to execute it. Both are latent — every executor the SDK constructs is single-threaded and `api.flags` blocks on that thread, so two reloads cannot overlap unless a host supplies its own pooled executor through `remoteConfigProvider`. +Fix `reloadFeatureFlags` callbacks being dropped when several flag reloads overlap diff --git a/posthog/src/main/java/com/posthog/internal/PostHogRemoteConfig.kt b/posthog/src/main/java/com/posthog/internal/PostHogRemoteConfig.kt index 968831404..4450f0187 100644 --- a/posthog/src/main/java/com/posthog/internal/PostHogRemoteConfig.kt +++ b/posthog/src/main/java/com/posthog/internal/PostHogRemoteConfig.kt @@ -710,17 +710,15 @@ public class PostHogRemoteConfig( return } - // Claiming the in-flight slot and queuing behind it must happen under one lock: otherwise a - // reload can observe "already loading", have the in-flight request drain an empty queue, and - // only then write itself into a slot nothing will ever drain. + // Claiming the in-flight slot and queuing behind it must happen under one lock, or a reload can + // observe "already loading" after the in-flight request has drained, and strand itself. val queued: Boolean synchronized(pendingFeatureFlagsLock) { queued = isLoadingFeatureFlags.getAndSet(true) if (queued) { pendingFeatureFlagsReload.set(true) - // The newest parameters win, but a displaced caller's callbacks are carried over so - // that queuing a reload never loses one. The internal callback may repeat in this - // list, so it must stay idempotent. + // Newest parameters win; displaced callers' callbacks are carried over so none is + // lost. The internal callback can repeat here, so it must stay idempotent. val displaced = pendingFeatureFlagsRequest pendingFeatureFlagsRequest = PendingFeatureFlagsRequest( @@ -734,8 +732,7 @@ public class PostHogRemoteConfig( } } if (queued) { - // Queue the reload request instead of dropping it - // This ensures that requests with $anon_distinct_id (from identify()) are not lost + // Queuing rather than dropping keeps identify()'s $anon_distinct_id request alive config.logger.log("Feature flags are being loaded already, queuing reload.") return } diff --git a/posthog/src/test/java/com/posthog/internal/PostHogRemoteConfigTest.kt b/posthog/src/test/java/com/posthog/internal/PostHogRemoteConfigTest.kt index 001e1599f..f432964fd 100644 --- a/posthog/src/test/java/com/posthog/internal/PostHogRemoteConfigTest.kt +++ b/posthog/src/test/java/com/posthog/internal/PostHogRemoteConfigTest.kt @@ -2745,9 +2745,8 @@ internal class PostHogRemoteConfigTest { http.shutdown() } - // The queuing branch in executeFeatureFlags is unreachable on the single-threaded executor every - // production path uses, so forcing overlap is the only way to exercise it. This pins the contract - // that machinery claims — no queued reload loses its callback — rather than a shipping scenario. + // The queuing branch is unreachable on the single-threaded executor every production path uses, + // so forcing overlap is the only way to exercise the contract that machinery claims. @Test fun `queued reloads displaced from the pending slot still run their callbacks`() { val http = mockHttp(response = MockResponse().setBody(responseFlagsApi)) @@ -2782,7 +2781,6 @@ internal class PostHogRemoteConfigTest { } assertTrue(fired.await(10, TimeUnit.SECONDS), "every queued reload must run its callback, ${fired.count} never did") - // at-least-once is not enough: a displaced callback must not be invoked twice either counts.forEachIndexed { index, count -> assertEquals(1, count.get(), "callback $index ran ${count.get()} times") } sut.clear() From a454eb966a3168f8af4d6cbfd720e5db1a626503 Mon Sep 17 00:00:00 2001 From: Anna Garcia Date: Wed, 19 Aug 2026 21:25:32 +0200 Subject: [PATCH 4/4] fix(flags): stop amplifying shared listeners and wedging on a throwing logger Cycle 2 of review found a regression this branch introduced and a pre-existing way to disable flag loading permanently. The pending slot appended callbacks without dedup, but the SDK reuses single instances - internalOnFeatureFlagsLoaded and config.onFeatureFlags are passed on every automatic reload - so a coalesced reload fired them once per queued reload rather than once per response. Measured 10 invocations for 2 responses, against 2 on main. distinct() restores one-per-response; the lists still carry every distinct caller, so nothing is dropped. runSafely logged from inside its own catch, unguarded. A PostHogLogger that throws escaped the finally before the in-flight claim was released, leaving it set forever: flags never reload again for the process lifetime, and every later identify/group/reset appends to a slot nothing will drain. Logging is now guarded and the callback fan-out cannot escape the finally at all. This was reachable on the shipped single-threaded executor, unlike the rest of this branch. Both are pinned by tests that fail without the fix ("shared listener fired 10 times for 2 responses", "a later reload never ran - the in-flight claim was stranded"), plus one for the isolation change below. The queued-reload test now also asserts the reloads actually coalesced, so it cannot pass vacuously if the scheduler happens to serialise it. isLoadingFeatureFlags and pendingFeatureFlagsReload were only ever touched under pendingFeatureFlagsLock, but AtomicBoolean advertises lock-free access - the signal that produced the strand bug in cycle 1. The first is now a plain lock-guarded Boolean; the second was exactly redundant with pendingFeatureFlagsRequest != null and is gone. Changeset rewritten. The queuing fixes are unreachable on any executor the SDK constructs, but isolating the two callbacks means a throwing internal callback no longer suppresses the caller's onFeatureFlags, and that happens on ordinary single-threaded flag loads. That is the user-visible change, so it is what the release note describes. --- .changeset/queued-flags-reload-callbacks.md | 2 +- .../posthog/internal/PostHogRemoteConfig.kt | 45 +++++--- .../internal/PostHogRemoteConfigTest.kt | 108 ++++++++++++++++++ 3 files changed, 136 insertions(+), 19 deletions(-) diff --git a/.changeset/queued-flags-reload-callbacks.md b/.changeset/queued-flags-reload-callbacks.md index b6c42b54a..71b0f9747 100644 --- a/.changeset/queued-flags-reload-callbacks.md +++ b/.changeset/queued-flags-reload-callbacks.md @@ -2,4 +2,4 @@ "posthog": patch --- -Fix `reloadFeatureFlags` callbacks being dropped when several flag reloads overlap +Fix `onFeatureFlags` not running when the internal flags-loaded callback throws diff --git a/posthog/src/main/java/com/posthog/internal/PostHogRemoteConfig.kt b/posthog/src/main/java/com/posthog/internal/PostHogRemoteConfig.kt index 4450f0187..12f3c79b2 100644 --- a/posthog/src/main/java/com/posthog/internal/PostHogRemoteConfig.kt +++ b/posthog/src/main/java/com/posthog/internal/PostHogRemoteConfig.kt @@ -39,7 +39,8 @@ public class PostHogRemoteConfig( PostHogFeatureFlagCalledProvider { _, _ -> }, private val onRemoteConfigLoaded: PostHogOnRemoteConfigLoaded? = null, ) : PostHogFeatureFlagsInterface { - private var isLoadingFeatureFlags = AtomicBoolean(false) + // guarded by pendingFeatureFlagsLock + private var isLoadingFeatureFlags = false private var isLoadingRemoteConfig = AtomicBoolean(false) // True once the live remote config has been resolved for the current identity, via either the @@ -50,7 +51,6 @@ public class PostHogRemoteConfig( // Track if an additional reload was requested while a request was in flight // This prevents dropping reload requests (e.g., from identify()) when preload is in progress - private var pendingFeatureFlagsReload = AtomicBoolean(false) private val pendingFeatureFlagsLock = Any() // Stores the parameters for the pending feature flags reload @@ -243,7 +243,11 @@ public class PostHogRemoteConfig( try { loaded() } catch (e: Throwable) { - config.logger.log("Executing the feature flags callback failed: $e") + try { + config.logger.log("Executing the feature flags callback failed: $e") + } catch (ignored: Throwable) { + // a logger that throws must not escape and strand the in-flight claim below + } } } @@ -714,9 +718,9 @@ public class PostHogRemoteConfig( // observe "already loading" after the in-flight request has drained, and strand itself. val queued: Boolean synchronized(pendingFeatureFlagsLock) { - queued = isLoadingFeatureFlags.getAndSet(true) + queued = isLoadingFeatureFlags + isLoadingFeatureFlags = true if (queued) { - pendingFeatureFlagsReload.set(true) // Newest parameters win; displaced callers' callbacks are carried over so none is // lost. The internal callback can repeat here, so it must stay idempotent. val displaced = pendingFeatureFlagsRequest @@ -725,9 +729,14 @@ public class PostHogRemoteConfig( distinctId = distinctId, anonymousId = anonymousId, groups = groups, + // distinct(): the SDK's own listeners and config.onFeatureFlags are singleton + // instances reused across reloads, so appending each queued reload would fire + // them once per coalesced reload instead of once per response. internalOnFeatureFlags = - displaced?.internalOnFeatureFlags.orEmpty() + listOfNotNull(internalOnFeatureFlags), - onFeatureFlags = displaced?.onFeatureFlags.orEmpty() + listOfNotNull(onFeatureFlags), + (displaced?.internalOnFeatureFlags.orEmpty() + listOfNotNull(internalOnFeatureFlags)) + .distinct(), + onFeatureFlags = + (displaced?.onFeatureFlags.orEmpty() + listOfNotNull(onFeatureFlags)).distinct(), ) } } @@ -912,21 +921,21 @@ public class PostHogRemoteConfig( notifyRemoteConfigResolved() } } finally { - runOnFeatureFlagsCallbacks( - internalOnFeatureFlags = internalOnFeatureFlags, - onFeatureFlags = onFeatureFlags, - ) + try { + runOnFeatureFlagsCallbacks( + internalOnFeatureFlags = internalOnFeatureFlags, + onFeatureFlags = onFeatureFlags, + ) + } catch (ignored: Throwable) { + // never skip the release below: leaving the claim set stops all future flag loads + } // Check if there's a pending reload request and execute it val pendingRequest: PendingFeatureFlagsRequest? synchronized(pendingFeatureFlagsLock) { - if (pendingFeatureFlagsReload.getAndSet(false)) { - pendingRequest = pendingFeatureFlagsRequest - pendingFeatureFlagsRequest = null - } else { - pendingRequest = null - } - isLoadingFeatureFlags.set(false) + pendingRequest = pendingFeatureFlagsRequest + pendingFeatureFlagsRequest = null + isLoadingFeatureFlags = false } pendingRequest?.let { request -> diff --git a/posthog/src/test/java/com/posthog/internal/PostHogRemoteConfigTest.kt b/posthog/src/test/java/com/posthog/internal/PostHogRemoteConfigTest.kt index f432964fd..016101f06 100644 --- a/posthog/src/test/java/com/posthog/internal/PostHogRemoteConfigTest.kt +++ b/posthog/src/test/java/com/posthog/internal/PostHogRemoteConfigTest.kt @@ -2781,10 +2781,118 @@ internal class PostHogRemoteConfigTest { } assertTrue(fired.await(10, TimeUnit.SECONDS), "every queued reload must run its callback, ${fired.count} never did") + assertTrue(http.requestCount < reloads, "reloads must have coalesced, otherwise this never reached the queuing branch") counts.forEachIndexed { index, count -> assertEquals(1, count.get(), "callback $index ran ${count.get()} times") } sut.clear() overlapping.shutdownAndAwaitTermination() http.shutdown() } + + @Test + fun `a listener reused across queued reloads is notified once per response`() { + val http = mockHttp(response = MockResponse().setBody(responseFlagsApi)) + repeat(9) { + http.enqueue(MockResponse().setBody(responseFlagsApi).setBodyDelay(300, TimeUnit.MILLISECONDS)) + } + val overlapping = Executors.newFixedThreadPool(4, PostHogThreadFactory("TestShared")) + val config = PostHogConfig(API_KEY, http.url("/").toString()).apply { cachePreferences = preferences } + val sut = + PostHogRemoteConfig( + config, + PostHogApi(config), + executor = overlapping, + defaultPersonPropertiesProvider = { emptyMap() }, + onRemoteConfigLoaded = null, + ) + + // the SDK passes the same listener instance on every automatic reload + val invocations = AtomicInteger(0) + val shared = PostHogOnFeatureFlags { invocations.incrementAndGet() } + repeat(10) { + sut.loadFeatureFlags("my_identify", anonymousId = "anonId", emptyMap(), onFeatureFlags = shared) + } + + Thread.sleep(3000) + assertTrue( + invocations.get() <= http.requestCount, + "shared listener fired ${invocations.get()} times for ${http.requestCount} responses", + ) + + sut.clear() + overlapping.shutdownAndAwaitTermination() + http.shutdown() + } + + @Test + fun `a throwing logger in a throwing callback does not stop later reloads`() { + val http = mockHttp(response = MockResponse().setBody(responseFlagsApi)) + repeat(3) { http.enqueue(MockResponse().setBody(responseFlagsApi)) } + val config = + PostHogConfig(API_KEY, http.url("/").toString()).apply { + cachePreferences = preferences + logger = + object : PostHogLogger { + override fun log(message: String) = throw IllegalStateException("logger blew up") + + override fun isEnabled(): Boolean = true + } + } + val sut = + PostHogRemoteConfig( + config, + PostHogApi(config), + executor = executor, + defaultPersonPropertiesProvider = { emptyMap() }, + onRemoteConfigLoaded = null, + ) + + val first = CountDownLatch(1) + sut.loadFeatureFlags( + "my_identify", + anonymousId = "anonId", + emptyMap(), + onFeatureFlags = + PostHogOnFeatureFlags { + first.countDown() + throw IllegalStateException("callback blew up") + }, + ) + first.await(5, TimeUnit.SECONDS) + val afterFirst = http.requestCount + + val second = CountDownLatch(1) + sut.loadFeatureFlags( + "my_identify", + anonymousId = "anonId", + emptyMap(), + onFeatureFlags = PostHogOnFeatureFlags { second.countDown() }, + ) + + assertTrue(second.await(5, TimeUnit.SECONDS), "a later reload never ran - the in-flight claim was stranded") + assertTrue(http.requestCount > afterFirst, "a later reload never reached the network") + + sut.clear() + http.shutdown() + } + + @Test + fun `a throwing internal callback does not suppress the caller's callback`() { + val http = mockHttp(response = MockResponse().setBody(responseFlagsApi)) + val sut = getSut(host = http.url("/").toString()) + + val userCallbackRan = CountDownLatch(1) + sut.loadFeatureFlags( + "my_identify", + anonymousId = "anonId", + emptyMap(), + internalOnFeatureFlags = PostHogOnFeatureFlags { throw IllegalStateException("internal blew up") }, + onFeatureFlags = PostHogOnFeatureFlags { userCallbackRan.countDown() }, + ) + + assertTrue(userCallbackRan.await(5, TimeUnit.SECONDS), "the caller's callback was suppressed by the internal one") + + sut.clear() + http.shutdown() + } }