fix(flags): stop losing queued feature flag reload callbacks - #710
Merged
Conversation
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.
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.
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.
turnipdabeets
marked this pull request as ready for review
August 19, 2026 18:56
Contributor
posthog-android Compliance ReportDate: 2026-08-19 19:31:53 UTC ✅ All Tests Passed!46/46 tests passed Capture Tests✅ 29/29 tests passed View Details
Feature_Flags Tests✅ 17/17 tests passed View Details
|
Contributor
|
Reviews (1): Last reviewed commit: "chore(flags): write the changeset for re..." | Re-trigger Greptile |
…g 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.
Contributor
🦔 ReviewHog reviewed this pull requestNothing worth raising this time, so here's a calming picture instead:
|
10 tasks
ioannisj
approved these changes
Aug 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
💡 Motivation and Context
executeFeatureFlagskeeps one pending reload behind the request in flight. Two separate paths could lose a queued reload's callback entirely — the caller waits forever.1. A displaced reload's callbacks were discarded. A third reload overwrote
pendingFeatureFlagsRequestwholesale, so the displaced request'sinternalOnFeatureFlagsandonFeatureFlagswere never invoked.2. A queued reload could be stranded with nothing to execute it.
isLoadingFeatureFlagswas claimed and released outsidependingFeatureFlagsLock, while the pending slot was written and drained inside it — two mechanisms guarding 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 drain.Both are latent, not user-visible. Every executor the SDK constructs is single-threaded (
PostHog.kt,PostHogStateless.kt) andapi.flagsblocks on that thread, so twoexecuteFeatureFlagscalls cannot overlap. The branch was already unreachable when #407 introduced it. A host can reach it by supplying its own pooled executor through the publicremoteConfigProviderhook, which is why this is worth closing rather than deleting.The pending slot now carries every displaced caller's callbacks — newest parameters still win, but nothing queued loses its completion — and the in-flight claim, the queue write and the release all happen under one lock. Each callback also runs behind its own
try/catch, so one throwing host callback can no longer skip the rest.This is the Android counterpart to PostHog/posthog-ios#737, and matches the shape the cross-SDK spec describes for this mechanism ("queue one pending reload … so identity-sensitive requests are not lost").
💚 How did you test it?
queued reloads displaced from the pending slot still run their callbacksinPostHogRemoteConfigTest. It 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. Written before the fix, and it fails onmain:It counts invocations per callback and asserts exactly one, so a double-invocation would also fail. 5 consecutive runs green.
:posthog:test(102 tests inPostHogRemoteConfigTest, full module suite) — passingspotlessCheckand:posthog:apiCheck— clean, public API unchanged/flagsand the flag reads correctly, ~1s. This is a regression check only — it exercises the normal single-threaded path, so it cannot demonstrate the fix.Not run: instrumented/Robolectric tests, or anything on a physical device.
📝 Checklist
If releasing new changes
pnpm changesetto generate a changeset file🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Written with Claude Code, directed by @turnipdabeets while reviewing the iOS counterpart (PostHog/posthog-ios#737), where @marandaneto asked whether Android needed the same fix.