fix(flags): resolve queued flag reloads against a request that went out - #737
Open
posthog[bot] wants to merge 9 commits into
Open
fix(flags): resolve queued flag reloads against a request that went out#737posthog[bot] wants to merge 9 commits into
posthog[bot] wants to merge 9 commits into
Conversation
`loadFeatureFlags` kept only one pending `/flags` reload while another was in flight. A third reload overwrote the queued one and immediately resolved that caller's completion handler with `getCachedFeatureFlags()` — disk-cached flags that never saw the caller's request-time person properties. The pending slot now keeps every displaced caller's completion handler and resolves them all against the coalesced request that actually goes out. Adds `setPersonPropertiesForFlags(_:reloadFeatureFlags:completion:)` so apps can await flags evaluated with their overrides, and documents the startup ordering contract on it and on `PostHogConfig.preloadFeatureFlags`. Generated-By: PostHog Code Task-Id: 5406b58a-1a76-4d8b-820a-94cd7c1f138b
Contributor
posthog-ios Compliance ReportDate: 2026-08-19 17:33:42 UTC ✅ All Tests Passed!45/45 tests passed Capture Tests✅ 29/29 tests passed View Details
Feature_Flags Tests✅ 16/16 tests passed View Details
|
Member
|
@ioannisj @turnipdabeets i think this needs a bit of testing/thinking, likely on android as well, i'd leave that up to you both since its the only sdks that would have this issue i guess? |
Contributor
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
The queued-reload rework in 1618c83 preserved each caller's evaluation context by snapshotting person properties at enqueue time and appending one pending request per context. Measured against main, that regressed two ordinary paths: - every setPersonPropertiesForFlags call was treated as order-sensitive, including the overloads that pass a no-op completion, so a burst of three issued four /flags requests instead of two, each carrying a partial property set; - a queued reload no longer picked up properties set while it waited and went out with no person_properties at all, reachable from setPersonProperties() on a plain launch, which is the failure class this branch exists to fix. What it bought was narrower: a caller awaiting one value for a key while a later caller overwrote that same key inside the same coalescing window. Person properties merge additively, so the single-slot form already guarantees a caller sees a response evaluated with at least its own properties. Revert to the single pending slot holding a list of completion handlers, and keep reading person properties when the request is sent. The lock-order fix in getPersonPropertiesForFlags() is kept, with both lock orders named in a comment.
# Conflicts: # PostHog/PostHogRemoteConfig.swift
…aviour The first test fails on 1618c83 (override-flag comes back false, the queued reload having gone out with no person_properties) and passes on the single-slot form, so it pins the regression that commit removed rather than restating it. The second covers the documented behaviour that a reload completion handler runs even when the request fails, since the handler takes no parameter and cannot report it. Also ignores xcodebuild-ios.log, which the testOniOSSimulator target tees into the working tree on every local run.
setPersonPropertiesForFlags(_:reloadFeatureFlags:completion:) was convenience,
not capability. Once queued reloads stop resolving from the disk cache, the
existing API already gives the same guarantee:
setPersonPropertiesForFlags(props, reloadFeatureFlags: false)
reloadFeatureFlags { /* evaluated with props */ }
which is what the "setting properties then reloading" test exercises. Against
that, the overload was permanent Swift and Objective-C surface with a selector
inconsistent with its sibling, no counterpart on Android or React Native, and a
() -> Void handler that cannot report the quota-limited and error paths where it
still fires with stale cached flags.
The startup ordering contract it documented is worth keeping, so it moves onto
setPersonPropertiesForFlags(_:reloadFeatureFlags:) and points at the two-call
form. api/posthog-ios.public-api.txt is back to matching main: no public API
change, so the changeset drops to patch.
turnipdabeets
force-pushed
the
posthog-code/flags-pending-reload-callbacks
branch
from
August 18, 2026 15:50
744e892 to
ed8c39c
Compare
Drops the callback?(nil) added to the canReloadFlagsForTesting guard: that flag is never set false outside tests, no test awaits a handler under it, and main does not resolve the caller there either, so the line was speculative hardening on a path the fix does not touch. The rest is comment and test noise: numbered narration that restated the code, a flagsRequests reset left over from an out-of-tree probe harness where suites shared a server, and a `resolved` flag asserting something the continuation returning already proves.
turnipdabeets
removed their request for review
August 19, 2026 16:35
Five independent reviews of this branch. Four findings were worth acting on. The lock-order comment and the changeset named a cycle nobody could reach. Three reviewers analysed it and proposed three different A-sides; the one that searched hardest found none at all, since loadCachedPropertiesForFlags() runs inside init, before the instance escapes, so no other thread can hold that per-instance lock. The code change stands on its own — do not call an escaping closure that reaches setupLock while holding this lock — so both now say that instead of asserting a deadlock this branch cannot demonstrate. The new tests built their coalescing window out of wall-clock delays, and a reviewer running CI's own command watched them fail twice in a row, resolving in 3ms with the window never formed. The macOS job runs without retries, so that is a red build. The first /flags response is now held on a semaphore the test signals after issuing the reloads that must coalesce behind it, which makes the window a fact rather than a race, and drops the suite from 1.8s to 1.1s. Assertions on server.flagsRequests.count were contaminated by requests bleeding from earlier tests — two reviewers observed 4 where the test expected 2, for identical code. They now assert on the request bodies, which is what the tests were actually about. The failure test asserted a flag that was never cached, so it could not fail; it now seeds a flag first and checks it survives, covering the reload spec's "failure keeps existing cached flags" scenario. loadFeatureFlags took a non-optional callback that the survey-coalescing branch silently discarded, safe only by a contract established a hundred lines away. Three reviewers independently flagged it as the last place a completion can vanish. The parameter is optional now, so that branch provably has nothing to drop, and the eight internal reload sites stop pushing no-op closures into the pending slot. Also drops "guaranteed" from the public docs, which contradicted the failure note fifteen lines below it, stops leaking /config as an implementation detail, and records the latency change in the changeset: a displaced handler now waits for a round trip plus retries where it used to resolve almost immediately.
5 tasks
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
An iOS app that overrides person properties per flag request (
setPersonPropertiesForFlags) can read the wrong flag value for the first few hundred milliseconds after launch, then see it flip once a later/flagsresponse lands. The original customer report turned out to be correct SDK behaviour (request-time overrides do beat stored person properties), but investigating it surfaced a real defect underneath.Three code paths can each start a
/flagsrequest at startup — the automatic preload off/config(config.preloadFeatureFlags, on by default),identify(), and the app's ownreloadFeatureFlags()— and none of them is ordered against the app'ssetPersonPropertiesForFlags()call.loadFeatureFlagskept only one pending slot: when a third reload arrived it overwrote the queued request and resolved that caller's completion handler immediately withgetCachedFeatureFlags()— disk-cached, pre-override values from a request that never went out. There was also no completion handler onsetPersonPropertiesForFlags, so there was no supported way to wait for "flags reloaded with my overrides".Anything combining request-time person properties with
identify()andreloadFeatureFlags()is exposed; a launch-time flag read can silently return the pre-override value.Changes
PostHogRemoteConfig.loadFeatureFlags— the pending slot now holds a list of completion handlers. Later callers still overwrite the request parameters (newest identity/groups win), but every displaced caller is carried over and resolved against the coalesced request that actually goes out, instead of against the disk cache.setPersonPropertiesForFlags(_:reloadFeatureFlags:completion:)— lets apps await a/flagsresponse evaluated with their overrides. The existing overloads delegate to it, so their behaviour is unchanged.PostHogConfig.preloadFeatureFlags: the automatic preload is not ordered against app-set overrides, andreset()clears them.PostHogRemoteConfig.reloadFeatureFlagsno longer drops the callback on thecanReloadFlagsForTestingearly return, so a completion handler can never be silently dropped.💚 How did you test it?
Added a
Test concurrent flag reload coalescingsuite inPostHogTests/PostHogFeatureFlagsTest.swift. The/flagsstub only enablesoverride-flagwhen the request body actually carried the override, so a callback resolved against a real response is distinguishable from one resolved against the cache. It covers:setPersonPropertiesForFlags→identify→reloadFeatureFlagswithpreloadFeatureFlagson and/configresolving mid-flight — reading the override-aware value (this fails onmain);Not verified locally: this environment has no Swift/Xcode toolchain, so
make lint,make testandmake buildSdkwere not run — CI is the first execution of these tests.api/posthog-ios.public-api.txtwas hand-edited for the new public method rather than regenerated withmake apiUpdate; if thepublic-apijob disagrees, regenerate it.📝 Checklist
If releasing new changes
pnpm changesetto generate a changeset file (added.changeset/flags-pending-reload-callbacks.mdby hand — no pnpm in this environment)🤖 Agent context
Autonomy: Fully autonomous
Written by Claude Code (PostHog Desktop) from an inbox report. The report's own prescription — hold completion handlers in a list, add a completion handler to
setPersonPropertiesForFlags, document the ordering contract — was followed as-is.Two alternatives were considered and rejected as riskier than the symptom: suppressing the notification for a
/flagsresponse that's already known to be stale (would delaydidReceiveFeatureFlagsand extend session-replay buffering), and declining to cache a response computed with superseded person properties (would leave flags unavailable for longer, and on a first launch there's nothing to fall back to). Neither is needed to close the hole the report describes; both change flag-availability timing for every app.A subagent reviewed the diff for compile and behavioural regressions since nothing could be built here; its findings on the stale public-API snapshot, the dropped callback on the
canReloadFlagsForTestingpath, and the missing changeset are folded in.Created with PostHog Desktop from this inbox report.