feat: Add async feature flag evaluation#720
Conversation
posthog-python Compliance ReportDate: 2026-07-22 11:50:12 UTC ✅ All Tests Passed!111/111 tests passed Capture_V1 Tests✅ 94/94 tests passed View Details
Feature_Flags Tests✅ 17/17 tests passed View Details
|
5376ad0 to
218c362
Compare
|
Reviews (1): Last reviewed commit: "Add async feature flag evaluation" | Re-trigger Greptile |
83794b2 to
ce3aa46
Compare
f3fed37 to
8816f29
Compare
ce3aa46 to
d1e9ab5
Compare
8816f29 to
cfe5d0f
Compare
d1e9ab5 to
a365483
Compare
cfe5d0f to
3b1a554
Compare
a365483 to
84f6ba5
Compare
84f6ba5 to
7f102b9
Compare
|
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 |
|
This PR was closed due to lack of activity. Feel free to reopen if it's still relevant. |
🦔 ReviewHog reviewed this pull requestFound 0 must fix, 5 should fix, 2 consider. Published 7 findings (view the review). |
|
ReviewHog Alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
There was a problem hiding this comment.
ReviewHog Report
Feature
Issues: 7 issues
Files (1)
posthog/async_client.py
What were the main changes
- Adds awaitable feature-flag/remote-config HTTP helpers (get_flags_decision, _get_flags_decision, get_remote_config_payload) on AsyncPosthog built on async_flags/async_get/async_remote_config
- Adds async feature flag definition loading and background polling (load_feature_flags, _load_feature_flags_async, _fetch_feature_flags_from_api_async, _poll_feature_flags) plus cache-provider integration, mirroring the sync poller
- Adds get_feature_flag_result/get_feature_flag/feature_enabled/get_feature_flag_payload/get_all_flags/get_all_flags_and_payloads/evaluate_flags async APIs, with evaluate_flags() returning a synchronous FeatureFlagEvaluations snapshot built via the shared record helpers
- $feature_flag_called events are scheduled into the async capture queue via _schedule_feature_flag_called_event, which now gracefully warns and no-ops instead of raising RuntimeError when accessed without a running event loop (addresses review feedback)
- flush()/shutdown() updated to drain pending feature-flag-called capture tasks and cancel/await the background flag-poll task before closing
- capture(send_feature_flags=...) now awaits get_all_flags directly (renamed to _capture_with_feature_flags) instead of dispatching to a thread, with an updated deprecation warning
| if flag_was_locally_evaluated: | ||
| lookup_match_value = override_match_value or flag_value |
There was a problem hiding this comment.
override_match_value=False is silently discarded when the flag also evaluates locally
Why we think it's a valid issue
- Checked:
_get_feature_flag_resultatasync_client.py:865-866, every caller that can setoverride_match_value, and_compute_payload_locally(client.py:2688-2708) to trace the concrete consequence; also confirmed the pre-existing twin in the sync client. - Found: The falsy-
oratasync_client.py:866is real, andoverride_match_valuereaches it from exactly one public entry point —get_feature_flag_payload(..., match_value=...)forwards it asoverride_match_value=match_value(async_client.py:1014);get_feature_flag/feature_enabled/get_feature_flag_resultnever pass it. So the trigger is narrowly scoped toget_feature_flag_payload(key, id, match_value=False)when the flag also evaluates locally to a truthy value. - Found: Tracing the payload lookup,
_compute_payload_locallymapsFalseto keystr(False)=="False"(client.py:2702-2707); PostHog boolean flags store payloads under"true", so the correct result formatch_value=Falseis almost alwaysNone. The bug therefore returns the enabled/actual-value payload instead ofNone— a genuine wrong result, but on an atypical input and a method already emitting aDeprecationWarning. - Found: The identical
override_match_value or flag_valueexists in the sync client (client.py:2216), so this is faithful duplication rather than a freshly introduced defect, though it does now extend to the async surface. - Impact: A real correctness bug (an explicit falsy override silently swallowed) with a nameable trigger/consequence and a clean, zero-risk one-liner fix (
is Nonecheck) — but low real-world impact given the narrow input, deprecated method, and the fact that the correct output is usuallyNoneanyway. That profile matches the reviewer'sconsidertier exactly; keeping it on record without surfacing it as higher priority is appropriate.
Issue description
In _get_feature_flag_result, lookup_match_value = override_match_value or flag_value uses Python's falsy-or semantics. If a caller explicitly passes override_match_value=False (e.g. via get_feature_flag_payload(key, distinct_id, match_value=False) to fetch the payload for the disabled/control variant) and the flag also happens to evaluate successfully via local evaluation, override_match_value or flag_value evaluates to flag_value instead of the caller's explicit False, because False is falsy — silently ignoring the override and returning the payload for whatever the flag's actual live value is instead of the explicitly requested control-variant payload. This exact pattern already exists in the synchronous client (client.py:2216) and is being duplicated here rather than introduced fresh, but it now affects the new async surface as well.
Suggested fix
Use an explicit None check instead of a truthy or: lookup_match_value = flag_value if override_match_value is None else override_match_value.
Prompt to fix with AI (copy-paste)
## Context
@posthog/async_client.py#L865-866
<issue_description>
In `_get_feature_flag_result`, `lookup_match_value = override_match_value or flag_value` uses Python's falsy-`or` semantics. If a caller explicitly passes `override_match_value=False` (e.g. via `get_feature_flag_payload(key, distinct_id, match_value=False)` to fetch the payload for the disabled/control variant) and the flag also happens to evaluate successfully via local evaluation, `override_match_value or flag_value` evaluates to `flag_value` instead of the caller's explicit `False`, because `False` is falsy — silently ignoring the override and returning the payload for whatever the flag's actual live value is instead of the explicitly requested control-variant payload. This exact pattern already exists in the synchronous client (`client.py:2216`) and is being duplicated here rather than introduced fresh, but it now affects the new async surface as well.
</issue_description>
<issue_validation>
- **Checked:** `_get_feature_flag_result` at `async_client.py:865-866`, every caller that can set `override_match_value`, and `_compute_payload_locally` (`client.py:2688-2708`) to trace the concrete consequence; also confirmed the pre-existing twin in the sync client.
- **Found:** The falsy-`or` at `async_client.py:866` is real, and `override_match_value` reaches it from exactly one public entry point — `get_feature_flag_payload(..., match_value=...)` forwards it as `override_match_value=match_value` (`async_client.py:1014`); `get_feature_flag`/`feature_enabled`/`get_feature_flag_result` never pass it. So the trigger is narrowly scoped to `get_feature_flag_payload(key, id, match_value=False)` when the flag also evaluates locally to a truthy value.
- **Found:** Tracing the payload lookup, `_compute_payload_locally` maps `False` to key `str(False)=="False"` (`client.py:2702-2707`); PostHog boolean flags store payloads under `"true"`, so the *correct* result for `match_value=False` is almost always `None`. The bug therefore returns the enabled/actual-value payload instead of `None` — a genuine wrong result, but on an atypical input and a method already emitting a `DeprecationWarning`.
- **Found:** The identical `override_match_value or flag_value` exists in the sync client (`client.py:2216`), so this is faithful duplication rather than a freshly introduced defect, though it does now extend to the async surface.
- **Impact:** A real correctness bug (an explicit falsy override silently swallowed) with a nameable trigger/consequence and a clean, zero-risk one-liner fix (`is None` check) — but low real-world impact given the narrow input, deprecated method, and the fact that the correct output is usually `None` anyway. That profile matches the reviewer's `consider` tier exactly; keeping it on record without surfacing it as higher priority is appropriate.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Use an explicit `None` check instead of a truthy `or`: `lookup_match_value = flag_value if override_match_value is None else override_match_value`.
</potential_solution>
| elif send_feature_flags: | ||
| self.log.warning( | ||
| "AsyncClient.capture(send_feature_flags=...) will use the synchronous " | ||
| "feature flag path until async feature flags are enabled. Prefer passing " | ||
| "`send_feature_flags` is deprecated. Prefer passing " | ||
| "flags=await client.evaluate_flags(...)." | ||
| ) | ||
| return await self._capture_with_sync_feature_flags( | ||
| return await self._capture_with_feature_flags( |
There was a problem hiding this comment.
capture(send_feature_flags=...) deprecation notice bypasses the standard DeprecationWarning mechanism
Why we think it's a valid issue
- Checked: the flagged path at
async_client.py:240-244, the three sibling deprecated methods in the same file, and the sync client's equivalentcapture(send_feature_flags=...)branch. - Found: The divergence is real and confirmed.
async_client.py:241-244emitsself.log.warning("send_feature_flagsis deprecated. Prefer passing flags=await client.evaluate_flags(...).")— a plain log line whose message omits any removal timeline. The sync client atclient.py:1177-1184handles the identical parameter withwarnings.warn(..., DeprecationWarning, stacklevel=2)and a message stating it "will be removed in a future major version" plus the migration cost. - Found: The inconsistency is also intra-file: the other deprecated APIs added in this same chunk —
get_feature_flag(async_client.py:959),feature_enabled(:981),get_feature_flag_payload(:1005) — all correctly usewarnings.warn(..., DeprecationWarning, stacklevel=2). Sosend_feature_flagsis the lone deprecated surface here using a log line. The PR notes explicitly claim an "updated deprecation warning" for this path, so the regression was introduced deliberately in these changes. - Impact: A
DeprecationWarningis machine-detectable and promotable via warning filters (python -W error::DeprecationWarning, pytestfilterwarnings=error, IDE strikethrough); alog.warningis none of these. Async consumers relying on deprecation tooling to plan migrations get no signal, unlike sync consumers of the same option — a genuine, if modest, break in the deprecate-before-remove contract for a public API parameter. Not a functional bug, but a concrete, actionable public-SDK consistency defect with a trivial zero-risk fix (swap towarnings.warn(...)), directly tied to code this PR touched.should_fixis appropriate.
Issue description
Every other deprecated method touched in this chunk (get_feature_flag, feature_enabled, get_feature_flag_payload) correctly calls warnings.warn(..., DeprecationWarning, stacklevel=2), and the sync client's equivalent capture(send_feature_flags=...) path does the same (client.py:1176-1184, with a message that also states the removal timeline and the concrete cost of not migrating). Here, the identical deprecated parameter instead just calls self.log.warning(...) — a plain log line, not a DeprecationWarning. This means: standard tooling that filters/promotes DeprecationWarning (pytest's -W error::DeprecationWarning, python -W, IDE deprecation strikethrough) will not catch this usage in the async client even though it does in the sync client for the same option, and the message itself drops the sync client's "will be removed in a future major version" language, weakening the deprecate-before-remove guarantee for this specific code path.
Suggested fix
Replace the self.log.warning(...) call with warnings.warn("send_feature_flagsis deprecated and will be removed in a future major version. Pass aflagssnapshot fromawait client.evaluate_flags(...) instead.", DeprecationWarning, stacklevel=2), matching the pattern already used elsewhere in this file and in client.py.
Prompt to fix with AI (copy-paste)
## Context
@posthog/async_client.py#L240-245
<issue_description>
Every other deprecated method touched in this chunk (`get_feature_flag`, `feature_enabled`, `get_feature_flag_payload`) correctly calls `warnings.warn(..., DeprecationWarning, stacklevel=2)`, and the sync client's equivalent `capture(send_feature_flags=...)` path does the same (client.py:1176-1184, with a message that also states the removal timeline and the concrete cost of not migrating). Here, the identical deprecated parameter instead just calls `self.log.warning(...)` — a plain log line, not a `DeprecationWarning`. This means: standard tooling that filters/promotes `DeprecationWarning` (pytest's `-W error::DeprecationWarning`, `python -W`, IDE deprecation strikethrough) will not catch this usage in the async client even though it does in the sync client for the same option, and the message itself drops the sync client's "will be removed in a future major version" language, weakening the deprecate-before-remove guarantee for this specific code path.
</issue_description>
<issue_validation>
- **Checked:** the flagged path at `async_client.py:240-244`, the three sibling deprecated methods in the same file, and the sync client's equivalent `capture(send_feature_flags=...)` branch.
- **Found:** The divergence is real and confirmed. `async_client.py:241-244` emits `self.log.warning("`send_feature_flags` is deprecated. Prefer passing flags=await client.evaluate_flags(...).")` — a plain log line whose message omits any removal timeline. The sync client at `client.py:1177-1184` handles the identical parameter with `warnings.warn(..., DeprecationWarning, stacklevel=2)` and a message stating it "will be removed in a future major version" plus the migration cost.
- **Found:** The inconsistency is also intra-file: the other deprecated APIs added in this same chunk — `get_feature_flag` (`async_client.py:959`), `feature_enabled` (`:981`), `get_feature_flag_payload` (`:1005`) — all correctly use `warnings.warn(..., DeprecationWarning, stacklevel=2)`. So `send_feature_flags` is the lone deprecated surface here using a log line. The PR notes explicitly claim an "updated deprecation warning" for this path, so the regression was introduced deliberately in these changes.
- **Impact:** A `DeprecationWarning` is machine-detectable and promotable via warning filters (`python -W error::DeprecationWarning`, pytest `filterwarnings=error`, IDE strikethrough); a `log.warning` is none of these. Async consumers relying on deprecation tooling to plan migrations get no signal, unlike sync consumers of the same option — a genuine, if modest, break in the deprecate-before-remove contract for a public API parameter. Not a functional bug, but a concrete, actionable public-SDK consistency defect with a trivial zero-risk fix (swap to `warnings.warn(...)`), directly tied to code this PR touched. `should_fix` is appropriate.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Replace the `self.log.warning(...)` call with `warnings.warn("`send_feature_flags` is deprecated and will be removed in a future major version. Pass a `flags` snapshot from `await client.evaluate_flags(...)` instead.", DeprecationWarning, stacklevel=2)`, matching the pattern already used elsewhere in this file and in `client.py`.
</potential_solution>
| if self.flag_cache and flag_result: | ||
| self.flag_cache.set_cached_flag( | ||
| distinct_id, key, flag_result, self.flag_definition_version | ||
| ) |
There was a problem hiding this comment.
Configuring flag_fallback_cache_url="redis://..." makes every async flag evaluation block the event loop with synchronous Redis I/O
Why we think it's a valid issue
- Checked: the
flag_fallback_cache_urlplumbing fromAsyncClient.__init__into_Client, the_initialize_flag_cacheresolver,RedisFlagCache's method bodies, and everyflag_cache/_get_stale_flag_fallbackcall reachable from the async flag path. - Found: The chain is real end to end.
AsyncClient.__init__acceptsflag_fallback_cache_url(async_client.py:101) and forwards it unchanged to_Client(:146);_initialize_flag_cachemaps aredis://URL toRedisFlagCache(redis.from_url(...))using the synchronousredispackage (client.py:3097-3116); andRedisFlagCache.set_cached_flag/get_stale_cached_flagdo blockingself.redis.setex/set/get(utils.py:373-386,:355-361). The async_get_feature_flag_resultcallsset_cached_flagdirectly atasync_client.py:876and:913, and its except-handlers call the inherited (non-overridden)_get_stale_flag_fallback→get_stale_cached_flagat:919/:923/:927. Additionallyflag_cache.clear()(a redisscan+deletesweep) runs on-loop in_fetch_feature_flags_from_api_async(:712/:724). - Found: No mitigation exists —
async_client.pyoverrides neither_initialize_flag_cachenor_get_stale_flag_fallback, and theflag_cachecalls are not wrapped. The file already usesasyncio.to_threadfor blocking calls (:1338/:1340), and the sibling_flag_definition_cache_providergot an async bridge (_resolve_flag_definition_cache_provider_result_async,:601-604), confirmingflag_cachewas the one left synchronous. - Impact: Confirmed blocking network I/O on the event loop — the exact 'blocking I/O on an async path' keep-category. When a user enables the documented
redis://cache, everyget_feature_flag_result/get_feature_flag/feature_enabledcall stalls all other coroutines on the loop for a Redis round-trip. Real and worth surfacing. - Priority: Lowered must_fix→should_fix. It is milder than must_fix on concrete evidence: it is opt-in (constructor default
flag_fallback_cache_url=None; the default andmemory://paths are unaffected), the per-call cost is a single bounded Redis op (not unbounded), the impact is performance degradation rather than correctness/data loss, and the recommendedevaluate_flags/get_all_flagspaths don't touchflag_cacheat all — only the single-flag path does.
Issue description
_get_feature_flag_result (an async def override) calls self.flag_cache.set_cached_flag(...) directly (lines 875-878, 912-915), and its exception handlers (lines 916-927) fall back to the inherited _get_stale_flag_fallback, which calls self.flag_cache.get_stale_cached_flag(...) (client.py:2157-2167). AsyncClient.__init__ explicitly accepts and forwards flag_fallback_cache_url to the shared _Client.__init__ (async_client.py:101,146), which resolves a "redis://..." URL to RedisFlagCache via the plain synchronous redis package (client.py:3049-3116, backed by posthog/utils.py's self.redis.get/setex/scan/delete calls) — not redis.asyncio. So on this fully-supported, documented constructor option, every single async flag evaluation performs a blocking network round-trip directly on the running event loop thread, stalling every other coroutine scheduled on that loop for the duration of the Redis call. This is the exact class of bug the PR already solved for the other cache abstraction in this same file: _flag_definition_cache_provider gets a dedicated async bridge (_resolve_flag_definition_cache_provider_result_async, with examples/async_redis_flag_cache.py demonstrating the intended redis.asyncio-based pattern for that provider), but flag_cache (the per-flag stale-value fallback cache) was left wired straight through with no equivalent bridge, silently reintroducing blocking I/O into the async client's hot path. Neither test_async_client.py nor test_async_feature_flags.py exercises flag_fallback_cache_url, so this regression is invisible to CI.
Suggested fix
Wrap the flag_cache read/write calls in await asyncio.to_thread(...) inside the async feature-flag methods (the file already uses this pattern elsewhere, e.g. _stop_blocking_polling_resources), or require an async-native cache protocol for flag_cache analogous to the awaitable _flag_definition_cache_provider methods. Add async test coverage that configures flag_fallback_cache_url="redis://..." against AsyncClient to catch this class of regression going forward.
Prompt to fix with AI (copy-paste)
## Context
@posthog/async_client.py#L875-878
@posthog/async_client.py#L912-915
@posthog/async_client.py#L916-927
<issue_description>
`_get_feature_flag_result` (an `async def` override) calls `self.flag_cache.set_cached_flag(...)` directly (lines 875-878, 912-915), and its exception handlers (lines 916-927) fall back to the inherited `_get_stale_flag_fallback`, which calls `self.flag_cache.get_stale_cached_flag(...)` (client.py:2157-2167). `AsyncClient.__init__` explicitly accepts and forwards `flag_fallback_cache_url` to the shared `_Client.__init__` (async_client.py:101,146), which resolves a `"redis://..."` URL to `RedisFlagCache` via the plain synchronous `redis` package (`client.py:3049-3116`, backed by `posthog/utils.py`'s `self.redis.get`/`setex`/`scan`/`delete` calls) — not `redis.asyncio`. So on this fully-supported, documented constructor option, every single async flag evaluation performs a blocking network round-trip directly on the running event loop thread, stalling every other coroutine scheduled on that loop for the duration of the Redis call. This is the exact class of bug the PR already solved for the *other* cache abstraction in this same file: `_flag_definition_cache_provider` gets a dedicated async bridge (`_resolve_flag_definition_cache_provider_result_async`, with `examples/async_redis_flag_cache.py` demonstrating the intended `redis.asyncio`-based pattern for that provider), but `flag_cache` (the per-flag stale-value fallback cache) was left wired straight through with no equivalent bridge, silently reintroducing blocking I/O into the async client's hot path. Neither `test_async_client.py` nor `test_async_feature_flags.py` exercises `flag_fallback_cache_url`, so this regression is invisible to CI.
</issue_description>
<issue_validation>
- **Checked:** the `flag_fallback_cache_url` plumbing from `AsyncClient.__init__` into `_Client`, the `_initialize_flag_cache` resolver, `RedisFlagCache`'s method bodies, and every `flag_cache`/`_get_stale_flag_fallback` call reachable from the async flag path.
- **Found:** The chain is real end to end. `AsyncClient.__init__` accepts `flag_fallback_cache_url` (`async_client.py:101`) and forwards it unchanged to `_Client` (`:146`); `_initialize_flag_cache` maps a `redis://` URL to `RedisFlagCache(redis.from_url(...))` using the synchronous `redis` package (`client.py:3097-3116`); and `RedisFlagCache.set_cached_flag`/`get_stale_cached_flag` do blocking `self.redis.setex`/`set`/`get` (`utils.py:373-386`, `:355-361`). The async `_get_feature_flag_result` calls `set_cached_flag` directly at `async_client.py:876` and `:913`, and its except-handlers call the inherited (non-overridden) `_get_stale_flag_fallback` → `get_stale_cached_flag` at `:919/:923/:927`. Additionally `flag_cache.clear()` (a redis `scan`+`delete` sweep) runs on-loop in `_fetch_feature_flags_from_api_async` (`:712/:724`).
- **Found:** No mitigation exists — `async_client.py` overrides neither `_initialize_flag_cache` nor `_get_stale_flag_fallback`, and the `flag_cache` calls are not wrapped. The file already uses `asyncio.to_thread` for blocking calls (`:1338/:1340`), and the sibling `_flag_definition_cache_provider` got an async bridge (`_resolve_flag_definition_cache_provider_result_async`, `:601-604`), confirming `flag_cache` was the one left synchronous.
- **Impact:** Confirmed blocking network I/O on the event loop — the exact 'blocking I/O on an async path' keep-category. When a user enables the documented `redis://` cache, every `get_feature_flag_result`/`get_feature_flag`/`feature_enabled` call stalls all other coroutines on the loop for a Redis round-trip. Real and worth surfacing.
- **Priority:** Lowered must_fix→should_fix. It is milder than must_fix on concrete evidence: it is opt-in (constructor default `flag_fallback_cache_url=None`; the default and `memory://` paths are unaffected), the per-call cost is a single bounded Redis op (not unbounded), the impact is performance degradation rather than correctness/data loss, and the recommended `evaluate_flags`/`get_all_flags` paths don't touch `flag_cache` at all — only the single-flag path does.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Wrap the `flag_cache` read/write calls in `await asyncio.to_thread(...)` inside the async feature-flag methods (the file already uses this pattern elsewhere, e.g. `_stop_blocking_polling_resources`), or require an async-native cache protocol for `flag_cache` analogous to the awaitable `_flag_definition_cache_provider` methods. Add async test coverage that configures `flag_fallback_cache_url="redis://..."` against `AsyncClient` to catch this class of regression going forward.
</potential_solution>
| return None | ||
|
|
||
| async def _capture_with_sync_feature_flags( | ||
| async def _capture_with_feature_flags( |
There was a problem hiding this comment.
capture(send_feature_flags={"only_evaluate_locally": False}) no longer forces remote evaluation in the async client
Why we think it's a valid issue
- Checked: the async
_capture_with_feature_flagsbranch logic, the sync client's four-branch equivalent,get_feature_variants, the asyncget_all_flags_and_payloadsfallback logic, the documented sync test, and async test coverage. - Found: The divergence is real. Sync
capturehas four branches (client.py:1186-1227): explicitonly_evaluate_locally is Falseroutes toget_feature_variants(client.py:700), which callsget_flags_decisiondirectly and returnsto_values(...)— pure remote, always hitting/flags, never local. The async version (async_client.py:273-292) has only two branches:is True→ local-only, and anelsethat captures BOTH explicitFalseand unsetNone, sending them toget_all_flags(only_evaluate_locally=False)→get_all_flags_and_payloads, which evaluates locally first and only calls_get_flags_decisionfor flags left unresolved (fallback_to_flags,async_client.py:1086). If local eval resolves everything, no/flagscall happens at all. - Found: The sync behavior is a documented, asserted contract —
test_client.py:1339("forces remote evaluation") setsonly_evaluate_locally=Falseand assertspatch_flags.assert_called_once(). Reachability is real on async:enable_local_evaluationdefaults True and definitions auto-load via_ensure_feature_flags_loaded_for_local_evaluation, so with apersonal_api_keylocal eval succeeds and the explicit-False request silently yields local values. - Found: No async test exercises the dict option —
test_async_client.py:166only coverssend_feature_flags=True(boolean) withget_all_flagsmocked — so CI does not catch the gap. - Impact: A caller passing
send_feature_flags={"only_evaluate_locally": False}to force fresh server-computed values gets locally-computed ones instead (and may skip/flagsentirely) — a genuine break of the sync client's documented contract, silently attaching potentially-stale local values to the captured event. - Priority: Lowered must_fix→should_fix on concrete evidence it's milder:
send_feature_flagsis a deprecated capture option both clients warn against and actively steer off (towardevaluate_flags); the wrong-value impact only manifests when local and remote disagree (stale poller snapshot / server-only logic) — for locally-evaluable flags they typically match; and it's a parity gap on the new async client's deprecated path, not a regression on the recommended mainline flag API. Real and worth fixing for parity, but not merge-blocking.
Issue description
_capture_with_feature_flags collapses three distinct sync-client branches into two. The sync client (client.py lines ~1186-1227) treats only_evaluate_locally as three-way: True -> local-only via get_all_flags(only_evaluate_locally=True); explicit False -> pure remote via get_feature_variants (which calls get_flags_decision directly, never touching local evaluation); None/unset -> get_all_flags(only_evaluate_locally=True) if flags are loaded, else remote. This is a documented, tested contract (posthog/test/test_client.py::test_capture_with_send_feature_flags_options_only_evaluate_locally_false, docstring: "forces remote evaluation", and asserts the underlying /flags call happens exactly once regardless of local state). The async version instead does: if only_evaluate_locally is True: get_all_flags(only_evaluate_locally=True) else: get_all_flags(only_evaluate_locally=False). Both the explicit False case and the default/unset None case now go through get_all_flags_and_payloads, which tries local evaluation first and only calls the remote /flags endpoint for flags it can't resolve locally (fallback_to_flags). So a caller who explicitly passes send_feature_flags={"only_evaluate_locally": False} to force fresh server-computed values (e.g. to bypass a stale local poller snapshot) will silently get locally-computed values instead whenever local evaluation succeeds — the opposite of what they asked for and what the sync client guarantees. There is no async test covering this option combination (test_async_client.py only covers the boolean True case, mocked directly against get_all_flags), so this regression isn't caught by the test suite.
Suggested fix
Add a genuine three-way branch matching the sync client: for only_evaluate_locally is False, call the remote flags decision directly (e.g. an async equivalent of get_feature_variants, built on await self._get_flags_decision(...) + to_values(...)), bypassing _get_all_flags_and_payloads_locally entirely; keep only_evaluate_locally is True mapped to get_all_flags(only_evaluate_locally=True); and handle the None case the same way the sync client does (local-first with no remote fallback when flags are already loaded, remote-only otherwise).
Prompt to fix with AI (copy-paste)
## Context
@posthog/async_client.py#L262-299
<issue_description>
`_capture_with_feature_flags` collapses three distinct sync-client branches into two. The sync client (`client.py` lines ~1186-1227) treats `only_evaluate_locally` as three-way: `True` -> local-only via `get_all_flags(only_evaluate_locally=True)`; explicit `False` -> pure remote via `get_feature_variants` (which calls `get_flags_decision` directly, never touching local evaluation); `None`/unset -> `get_all_flags(only_evaluate_locally=True)` if flags are loaded, else remote. This is a documented, tested contract (`posthog/test/test_client.py::test_capture_with_send_feature_flags_options_only_evaluate_locally_false`, docstring: "forces remote evaluation", and asserts the underlying `/flags` call happens exactly once regardless of local state). The async version instead does: `if only_evaluate_locally is True: get_all_flags(only_evaluate_locally=True) else: get_all_flags(only_evaluate_locally=False)`. Both the explicit `False` case and the default/unset `None` case now go through `get_all_flags_and_payloads`, which tries **local evaluation first** and only calls the remote `/flags` endpoint for flags it can't resolve locally (`fallback_to_flags`). So a caller who explicitly passes `send_feature_flags={"only_evaluate_locally": False}` to force fresh server-computed values (e.g. to bypass a stale local poller snapshot) will silently get locally-computed values instead whenever local evaluation succeeds — the opposite of what they asked for and what the sync client guarantees. There is no async test covering this option combination (`test_async_client.py` only covers the boolean `True` case, mocked directly against `get_all_flags`), so this regression isn't caught by the test suite.
</issue_description>
<issue_validation>
- **Checked:** the async `_capture_with_feature_flags` branch logic, the sync client's four-branch equivalent, `get_feature_variants`, the async `get_all_flags_and_payloads` fallback logic, the documented sync test, and async test coverage.
- **Found:** The divergence is real. Sync `capture` has four branches (`client.py:1186-1227`): explicit `only_evaluate_locally is False` routes to `get_feature_variants` (`client.py:700`), which calls `get_flags_decision` directly and returns `to_values(...)` — pure remote, always hitting `/flags`, never local. The async version (`async_client.py:273-292`) has only two branches: `is True` → local-only, and an `else` that captures BOTH explicit `False` and unset `None`, sending them to `get_all_flags(only_evaluate_locally=False)` → `get_all_flags_and_payloads`, which evaluates locally first and only calls `_get_flags_decision` for flags left unresolved (`fallback_to_flags`, `async_client.py:1086`). If local eval resolves everything, no `/flags` call happens at all.
- **Found:** The sync behavior is a documented, asserted contract — `test_client.py:1339` ("forces remote evaluation") sets `only_evaluate_locally=False` and asserts `patch_flags.assert_called_once()`. Reachability is real on async: `enable_local_evaluation` defaults True and definitions auto-load via `_ensure_feature_flags_loaded_for_local_evaluation`, so with a `personal_api_key` local eval succeeds and the explicit-False request silently yields local values.
- **Found:** No async test exercises the dict option — `test_async_client.py:166` only covers `send_feature_flags=True` (boolean) with `get_all_flags` mocked — so CI does not catch the gap.
- **Impact:** A caller passing `send_feature_flags={"only_evaluate_locally": False}` to force fresh server-computed values gets locally-computed ones instead (and may skip `/flags` entirely) — a genuine break of the sync client's documented contract, silently attaching potentially-stale local values to the captured event.
- **Priority:** Lowered must_fix→should_fix on concrete evidence it's milder: `send_feature_flags` is a deprecated capture option both clients warn against and actively steer off (toward `evaluate_flags`); the wrong-value impact only manifests when local and remote disagree (stale poller snapshot / server-only logic) — for locally-evaluable flags they typically match; and it's a parity gap on the new async client's deprecated path, not a regression on the recommended mainline flag API. Real and worth fixing for parity, but not merge-blocking.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Add a genuine three-way branch matching the sync client: for `only_evaluate_locally is False`, call the remote flags decision directly (e.g. an async equivalent of `get_feature_variants`, built on `await self._get_flags_decision(...)` + `to_values(...)`), bypassing `_get_all_flags_and_payloads_locally` entirely; keep `only_evaluate_locally is True` mapped to `get_all_flags(only_evaluate_locally=True)`; and handle the `None` case the same way the sync client does (local-first with no remote fallback when flags are already loaded, remote-only otherwise).
</potential_solution>
| async def _drain_pending_feature_flag_captures(self) -> None: | ||
| while self._pending_feature_flag_capture_tasks: | ||
| tasks = list(self._pending_feature_flag_capture_tasks) | ||
| await asyncio.gather(*tasks, return_exceptions=True) |
There was a problem hiding this comment.
flush()'s timeout_seconds does not bound the new pending feature-flag-called capture drain
Why we think it's a valid issue
- Checked:
flush(async_client.py:1309-1321),_drain_pending_feature_flag_captures(:1304-1307), the_enqueuesync_mode path (:483-521), the effectiveself.sync_modevalue, and how pending tasks are populated. - Found: The gap is real.
flushawaits_drain_pending_feature_flag_captures()unconditionally at:1310, outside theasyncio.wait_for(..., timeout=timeout_seconds)that only wrapsself._queue.join()(:1315). The drain loopsawait asyncio.gather(*tasks)with no timeout (:1307). Each task runs_capture_feature_flag_called_if_needed_async→await self.capture(...)→_enqueue, and whenself.sync_modeis True,_enqueueperforms a real awaited_async_batch_post(..., timeout=self.timeout, ...)(:512-520) bounded byself.timeout(default 15), not byflush'stimeout_seconds. - Found: In sync_mode the effect is especially clean:
_enqueuesends immediately and never puts to the queue, soself._queue.join()returns instantly — meaningtimeout_secondsguards an empty join while the actual blocking work (the flag-called HTTP drain) is entirely unguarded.self.sync_modeis the user-supplied constructor value (:174, default False), so this requires an explicitsync_mode=True. - Found: Reachability is on the PR's headline path: pending tasks are populated via
_schedule_feature_flag_called_event(:1234), which is theFeatureFlagEvaluationshost callback fired byevaluate_flags(...)+flags.is_enabled()/get_flag(). A slow asyncbefore_send(awaited at:492) can also stall the drain even in default mode. - Impact: A caller doing
await client.flush(timeout_seconds=2)(e.g. in a request handler) withsync_mode=Trueand a pending$feature_flag_calledcapture can block up to ~self.timeout(≈15s) instead of 2s — a real violation of the flush timeout contract. Bounded (not infinite) and gated behind the non-default sync_mode, with a trivial fix (wrap the drain in the samewait_forbudget).should_fixis appropriate.
Issue description
flush() now unconditionally awaits self._drain_pending_feature_flag_captures() (line 1310) before the timeout_seconds-bounded self._queue.join() logic. _drain_pending_feature_flag_captures (lines 1304-1307) loops await asyncio.gather(*tasks, return_exceptions=True) over self._pending_feature_flag_capture_tasks with no timeout at all. Each of those tasks runs _capture_feature_flag_called_if_needed_async -> await self.capture(...) -> _enqueue, and when self.sync_mode is True, _enqueue performs a real, awaited _async_batch_post HTTP call bounded only by self.timeout (or an arbitrarily slow before_send callback), not by the caller-supplied flush(timeout_seconds=...). A caller that explicitly requests flush(timeout_seconds=2) to bound blocking time (e.g. inside a web request handler) can therefore block far longer than 2 seconds if any scheduled $feature_flag_called capture is slow, silently violating the documented timeout contract of flush().
Suggested fix
Wrap the drain call in the same timeout budget as the queue join, e.g. await asyncio.wait_for(self._drain_pending_feature_flag_captures(), timeout=timeout_seconds) (guarding the timeout_seconds is None case), or subtract elapsed drain time from the remaining budget passed to asyncio.wait_for(self._queue.join(), ...). At minimum, log a warning and continue if the drain step itself times out, mirroring the existing except asyncio.TimeoutError handling for the queue join.
Prompt to fix with AI (copy-paste)
## Context
@posthog/async_client.py#L1304-1307
@posthog/async_client.py#L1309-1321
<issue_description>
`flush()` now unconditionally awaits `self._drain_pending_feature_flag_captures()` (line 1310) *before* the `timeout_seconds`-bounded `self._queue.join()` logic. `_drain_pending_feature_flag_captures` (lines 1304-1307) loops `await asyncio.gather(*tasks, return_exceptions=True)` over `self._pending_feature_flag_capture_tasks` with no timeout at all. Each of those tasks runs `_capture_feature_flag_called_if_needed_async` -> `await self.capture(...)` -> `_enqueue`, and when `self.sync_mode` is True, `_enqueue` performs a real, awaited `_async_batch_post` HTTP call bounded only by `self.timeout` (or an arbitrarily slow `before_send` callback), not by the caller-supplied `flush(timeout_seconds=...)`. A caller that explicitly requests `flush(timeout_seconds=2)` to bound blocking time (e.g. inside a web request handler) can therefore block far longer than 2 seconds if any scheduled `$feature_flag_called` capture is slow, silently violating the documented timeout contract of `flush()`.
</issue_description>
<issue_validation>
- **Checked:** `flush` (`async_client.py:1309-1321`), `_drain_pending_feature_flag_captures` (`:1304-1307`), the `_enqueue` sync_mode path (`:483-521`), the effective `self.sync_mode` value, and how pending tasks are populated.
- **Found:** The gap is real. `flush` awaits `_drain_pending_feature_flag_captures()` unconditionally at `:1310`, *outside* the `asyncio.wait_for(..., timeout=timeout_seconds)` that only wraps `self._queue.join()` (`:1315`). The drain loops `await asyncio.gather(*tasks)` with no timeout (`:1307`). Each task runs `_capture_feature_flag_called_if_needed_async` → `await self.capture(...)` → `_enqueue`, and when `self.sync_mode` is True, `_enqueue` performs a real awaited `_async_batch_post(..., timeout=self.timeout, ...)` (`:512-520`) bounded by `self.timeout` (default 15), not by `flush`'s `timeout_seconds`.
- **Found:** In sync_mode the effect is especially clean: `_enqueue` sends immediately and never puts to the queue, so `self._queue.join()` returns instantly — meaning `timeout_seconds` guards an empty join while the actual blocking work (the flag-called HTTP drain) is entirely unguarded. `self.sync_mode` is the user-supplied constructor value (`:174`, default False), so this requires an explicit `sync_mode=True`.
- **Found:** Reachability is on the PR's headline path: pending tasks are populated via `_schedule_feature_flag_called_event` (`:1234`), which is the `FeatureFlagEvaluations` host callback fired by `evaluate_flags(...)` + `flags.is_enabled()/get_flag()`. A slow async `before_send` (awaited at `:492`) can also stall the drain even in default mode.
- **Impact:** A caller doing `await client.flush(timeout_seconds=2)` (e.g. in a request handler) with `sync_mode=True` and a pending `$feature_flag_called` capture can block up to ~`self.timeout` (≈15s) instead of 2s — a real violation of the flush timeout contract. Bounded (not infinite) and gated behind the non-default sync_mode, with a trivial fix (wrap the drain in the same `wait_for` budget). `should_fix` is appropriate.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Wrap the drain call in the same timeout budget as the queue join, e.g. `await asyncio.wait_for(self._drain_pending_feature_flag_captures(), timeout=timeout_seconds)` (guarding the `timeout_seconds is None` case), or subtract elapsed drain time from the remaining budget passed to `asyncio.wait_for(self._queue.join(), ...)`. At minimum, log a warning and continue if the drain step itself times out, mirroring the existing `except asyncio.TimeoutError` handling for the queue join.
</potential_solution>
| async def load_feature_flags(self) -> None: | ||
| if self.disabled: | ||
| self.feature_flags = [] | ||
| return | ||
| if not self.personal_api_key: | ||
| self.log.warning( | ||
| "[FEATURE FLAGS] You have to specify a personal_api_key to use feature flags." | ||
| ) | ||
| self.feature_flags = [] | ||
| return | ||
|
|
||
| await self._load_feature_flags_async() | ||
|
|
||
| if self.enable_local_evaluation and self._flag_poll_task is None: | ||
| self._flag_poll_task = asyncio.create_task(self._poll_feature_flags()) | ||
|
|
||
| async def _poll_feature_flags(self) -> None: | ||
| try: | ||
| while True: | ||
| await asyncio.sleep(self.poll_interval) | ||
| await self._load_feature_flags_async() | ||
| except asyncio.CancelledError: | ||
| raise |
There was a problem hiding this comment.
Background flag-definition poller can die permanently with no self-healing restart
Why we think it's a valid issue
- Checked:
load_feature_flags/_poll_feature_flags(async_client.py:740-762), every_flag_poll_taskassignment, the debug re-raise sites in_fetch_feature_flags_from_api_async, and the sync client's self-heal (client.py:1994-2001,poller.py). - Found: The structural claims all hold.
_flag_poll_taskis set only at init (:183) and reset toNoneonly in shutdown (:1336) — never on death; the restart guard isis None(:753), so a done-but-referenced task is never recreated._poll_feature_flagsre-raises onlyCancelledError(:761-762), so any other exception ends the loop permanently. The sync client instead restarts whennot (self.poller and self.poller.is_alive())(client.py:1994-1996), and itsPoller.runthread dies on an execute() exception (poller.py:17-19), sois_alive()correctly reports death — a real self-healing divergence. - Found (reachability is narrow): Tracing every exception path, the only way
_load_feature_flags_asyncraises is_fetch_feature_flags_from_api_asyncre-raising_APIErroron 401/402 and only whenself.debugis True (:714,:725). All cache-provider calls and_update_flag_stateare inside try/except (:618-652,:665-731), and non-401/402 / generic errors are caught and logged. So with the defaultdebug=False, the poller catches and logs 401/402 and keeps running — the permanent-death scenario is gated entirely behind the non-defaultdebug=True. - Impact: With
debug=True, a transient 401 (rotated key) or 402 (quota) kills the poll task for the process's life;self.feature_flagsfreezes and local evaluation silently serves stale definitions with no restart. Real reliability/parity gap with a trivial correct fix (or self._flag_poll_task.done()). - Priority: Lowered should_fix→consider. The sole reachable kill-path requires
debug=True(a dev/verbose setting, not the production default), and in the default config the poller is robust to the cited 401/402 — so the real-world blast radius is narrow. Genuine and worth keeping on record, but not production-reachable enough for should_fix.
Issue description
load_feature_flags() only (re)creates the poll task when self._flag_poll_task is None (line 753). Once assigned, that guard is never satisfied again even if the task has since completed or died — _flag_poll_task still holds a reference to the (finished) Task object, it is not reset to None. _poll_feature_flags() (lines 756-762) only catches asyncio.CancelledError; any other exception propagates out and ends the loop for good. That is reachable today: _fetch_feature_flags_from_api_async re-raises _APIError on 401/402 when self.debug is True (lines 701-728), which will kill the poll task on the very next scheduled fetch. From that point on, self.feature_flags is permanently frozen — no more automatic refreshes — even though the sync client's Poller (poller.py) is a daemon thread whose is_alive() correctly reports death, letting load_feature_flags() transparently spin up a fresh Poller the next time it's called (client.py:1993-2001). The async client has no equivalent self-healing path, so a customer running with debug=True who hits a transient 401/402 (e.g. quota limiting, a rotated personal_api_key) loses background flag-definition refresh for the remainder of the process's life with no visible error beyond one log line.
Suggested fix
Track task completion instead of only presence, e.g. if self.enable_local_evaluation and (self._flag_poll_task is None or self._flag_poll_task.done()): self._flag_poll_task = asyncio.create_task(self._poll_feature_flags()), so load_feature_flags() can restart a dead poller the same way the sync client's Poller.is_alive() check does.
Prompt to fix with AI (copy-paste)
## Context
@posthog/async_client.py#L740-762
<issue_description>
`load_feature_flags()` only (re)creates the poll task when `self._flag_poll_task is None` (line 753). Once assigned, that guard is never satisfied again even if the task has since completed or died — `_flag_poll_task` still holds a reference to the (finished) Task object, it is not reset to `None`. `_poll_feature_flags()` (lines 756-762) only catches `asyncio.CancelledError`; any other exception propagates out and ends the loop for good. That is reachable today: `_fetch_feature_flags_from_api_async` re-raises `_APIError` on 401/402 when `self.debug` is True (lines 701-728), which will kill the poll task on the very next scheduled fetch. From that point on, `self.feature_flags` is permanently frozen — no more automatic refreshes — even though the sync client's `Poller` (poller.py) is a daemon thread whose `is_alive()` correctly reports death, letting `load_feature_flags()` transparently spin up a fresh `Poller` the next time it's called (client.py:1993-2001). The async client has no equivalent self-healing path, so a customer running with `debug=True` who hits a transient 401/402 (e.g. quota limiting, a rotated personal_api_key) loses background flag-definition refresh for the remainder of the process's life with no visible error beyond one log line.
</issue_description>
<issue_validation>
- **Checked:** `load_feature_flags`/`_poll_feature_flags` (`async_client.py:740-762`), every `_flag_poll_task` assignment, the debug re-raise sites in `_fetch_feature_flags_from_api_async`, and the sync client's self-heal (`client.py:1994-2001`, `poller.py`).
- **Found:** The structural claims all hold. `_flag_poll_task` is set only at init (`:183`) and reset to `None` only in shutdown (`:1336`) — never on death; the restart guard is `is None` (`:753`), so a done-but-referenced task is never recreated. `_poll_feature_flags` re-raises only `CancelledError` (`:761-762`), so any other exception ends the loop permanently. The sync client instead restarts when `not (self.poller and self.poller.is_alive())` (`client.py:1994-1996`), and its `Poller.run` thread dies on an execute() exception (`poller.py:17-19`), so `is_alive()` correctly reports death — a real self-healing divergence.
- **Found (reachability is narrow):** Tracing every exception path, the only way `_load_feature_flags_async` raises is `_fetch_feature_flags_from_api_async` re-raising `_APIError` on 401/402 *and only when `self.debug` is True* (`:714`, `:725`). All cache-provider calls and `_update_flag_state` are inside try/except (`:618-652`, `:665-731`), and non-401/402 / generic errors are caught and logged. So with the default `debug=False`, the poller catches and logs 401/402 and keeps running — the permanent-death scenario is gated entirely behind the non-default `debug=True`.
- **Impact:** With `debug=True`, a transient 401 (rotated key) or 402 (quota) kills the poll task for the process's life; `self.feature_flags` freezes and local evaluation silently serves stale definitions with no restart. Real reliability/parity gap with a trivial correct fix (`or self._flag_poll_task.done()`).
- **Priority:** Lowered should_fix→consider. The sole reachable kill-path requires `debug=True` (a dev/verbose setting, not the production default), and in the default config the poller is robust to the cited 401/402 — so the real-world blast radius is narrow. Genuine and worth keeping on record, but not production-reachable enough for should_fix.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Track task completion instead of only presence, e.g. `if self.enable_local_evaluation and (self._flag_poll_task is None or self._flag_poll_task.done()): self._flag_poll_task = asyncio.create_task(self._poll_feature_flags())`, so `load_feature_flags()` can restart a dead poller the same way the sync client's `Poller.is_alive()` check does.
</potential_solution>
| def _schedule_feature_flag_called_event(self, **kwargs) -> None: | ||
| try: | ||
| loop = asyncio.get_running_loop() | ||
| except RuntimeError: | ||
| self.log.warning( | ||
| "[FEATURE FLAGS] Unable to capture $feature_flag_called because no event loop is running. " | ||
| "Access FeatureFlagEvaluations results before the async client shuts down." | ||
| ) | ||
| return | ||
|
|
||
| task = loop.create_task( | ||
| self._capture_feature_flag_called_if_needed_async(**kwargs) | ||
| ) | ||
| self._pending_feature_flag_capture_tasks.add(task) | ||
| task.add_done_callback(self._pending_feature_flag_capture_tasks.discard) |
There was a problem hiding this comment.
Deferred $feature_flag_called dedup check-then-act race can double-capture the same flag access
Why we think it's a valid issue
- Checked: the deferred dedup body (
async_client.py:1292-1302), the scheduling entry point_schedule_feature_flag_called_event(:1221-1235), theFeatureFlagEvaluationsaccess path (feature_flag_evaluations.py:218-243,_record_access:335-387), and the suspension points insidecapture/_enqueue. - Found: The race is real: the dedup check is at
:1292,await self.capture(...)at:1295, andreported_flags.add(...)at:1302— the add is after the await, so the check→set critical section straddles a suspension point._record_accesscallsself._host.capture_flag_called_event_if_needed(...)unconditionally on everyis_enabled/get_flag(feature_flag_evaluations.py:380;_accessedis tracked but not used to gate the call), so repeated same-key accesses each schedule a fresh task vialoop.create_task(:1231). This violates the explicitly documented exactly-once contract (is_enableddocstring,feature_flag_evaluations.py:219-220). - Found (reachability precondition): The race only materializes if
captureactually suspends between check and add. That happens withsync_mode=True(awaited_async_batch_post,:512) or a genuinely-suspending asyncbefore_send(await modified_msg,:491-492) — the latter is reachable in the default queue mode, not just the niche sync_mode. With the default config (queue mode, no/sync before_send)capturenever yields, so the first task runs check→enqueue→add atomically and no duplicate occurs. - Found (async-specific): This is a genuine regression vs the sync client: the sync
_capture_feature_flag_called_if_neededruns check/capture/add inline, so single-threaded repeated access is sequential and safe; the async deferral makes even a single coroutine's repeated accesses interleave. - Impact: Under a suspending capture, two accesses to the same (distinct_id, key, value, groups) both pass the check and emit duplicate
$feature_flag_calledevents, defeating thedistinct_ids_feature_flags_reporteddedup. Data-quality/contract defect with a clean fix (reserve the key synchronously at schedule time). Reachable via a supported asyncbefore_sendin default mode, soshould_fixstands.
Issue description
_schedule_feature_flag_called_event (lines 1221-1235) is a synchronous method invoked directly from FeatureFlagEvaluations.is_enabled()/get_flag() on every access (feature_flag_evaluations.py _record_access, unconditionally, with no per-instance memoization). It merely schedules _capture_feature_flag_called_if_needed_async via asyncio.create_task(...) and returns immediately — the dedup check (if feature_flag_reported_key in reported_flags: return, line 1292) only runs once the scheduled task actually executes, not at schedule time. In the sync client, the equivalent dedup logic runs inline/synchronously inside the same call, so check-then-set is atomic for a given call. In the async client, if the same flag key (same distinct_id/value/groups) is accessed twice before the first scheduled task gets a turn to run — e.g. two is_enabled("flag_a") calls in the same coroutine before any suspending await, or interleaving caused by an async before_send hook or sync_mode network send inside the first task's own self.capture(...) call (lines 488-521) — both tasks observe reported_flags not yet containing the key and both proceed to call self.capture("$feature_flag_called", ...), producing duplicate analytics events for a single logical flag access. This defeats the purpose of distinct_ids_feature_flags_reported, which the sync client relies on to guarantee exactly-once $feature_flag_called capture per (distinct_id, flag, value, groups) combination.
Suggested fix
Reserve the dedup key synchronously in _schedule_feature_flag_called_event before creating the task (e.g. compute feature_flag_reported_key and add it to reported_flags at schedule time, not inside the deferred coroutine), so two accesses to the same key before the task runs cannot both pass the check. Alternatively, key the pending-task set itself by feature_flag_reported_key and skip scheduling a new task if one is already pending or already recorded.
Prompt to fix with AI (copy-paste)
## Context
@posthog/async_client.py#L1221-1235
@posthog/async_client.py#L1274-1302
<issue_description>
`_schedule_feature_flag_called_event` (lines 1221-1235) is a synchronous method invoked directly from `FeatureFlagEvaluations.is_enabled()`/`get_flag()` on every access (feature_flag_evaluations.py `_record_access`, unconditionally, with no per-instance memoization). It merely schedules `_capture_feature_flag_called_if_needed_async` via `asyncio.create_task(...)` and returns immediately — the dedup check (`if feature_flag_reported_key in reported_flags: return`, line 1292) only runs once the scheduled task actually executes, not at schedule time. In the sync client, the equivalent dedup logic runs inline/synchronously inside the same call, so check-then-set is atomic for a given call. In the async client, if the same flag key (same distinct_id/value/groups) is accessed twice before the first scheduled task gets a turn to run — e.g. two `is_enabled("flag_a")` calls in the same coroutine before any suspending `await`, or interleaving caused by an async `before_send` hook or `sync_mode` network send inside the first task's own `self.capture(...)` call (lines 488-521) — both tasks observe `reported_flags` not yet containing the key and both proceed to call `self.capture("$feature_flag_called", ...)`, producing duplicate analytics events for a single logical flag access. This defeats the purpose of `distinct_ids_feature_flags_reported`, which the sync client relies on to guarantee exactly-once `$feature_flag_called` capture per (distinct_id, flag, value, groups) combination.
</issue_description>
<issue_validation>
- **Checked:** the deferred dedup body (`async_client.py:1292-1302`), the scheduling entry point `_schedule_feature_flag_called_event` (`:1221-1235`), the `FeatureFlagEvaluations` access path (`feature_flag_evaluations.py:218-243`, `_record_access` `:335-387`), and the suspension points inside `capture`/`_enqueue`.
- **Found:** The race is real: the dedup check is at `:1292`, `await self.capture(...)` at `:1295`, and `reported_flags.add(...)` at `:1302` — the add is *after* the await, so the check→set critical section straddles a suspension point. `_record_access` calls `self._host.capture_flag_called_event_if_needed(...)` unconditionally on every `is_enabled`/`get_flag` (`feature_flag_evaluations.py:380`; `_accessed` is tracked but not used to gate the call), so repeated same-key accesses each schedule a fresh task via `loop.create_task` (`:1231`). This violates the explicitly documented exactly-once contract (`is_enabled` docstring, `feature_flag_evaluations.py:219-220`).
- **Found (reachability precondition):** The race only materializes if `capture` actually suspends between check and add. That happens with `sync_mode=True` (awaited `_async_batch_post`, `:512`) or a genuinely-suspending async `before_send` (`await modified_msg`, `:491-492`) — the latter is reachable in the *default* queue mode, not just the niche sync_mode. With the default config (queue mode, no/sync before_send) `capture` never yields, so the first task runs check→enqueue→add atomically and no duplicate occurs.
- **Found (async-specific):** This is a genuine regression vs the sync client: the sync `_capture_feature_flag_called_if_needed` runs check/capture/add inline, so single-threaded repeated access is sequential and safe; the async deferral makes even a single coroutine's repeated accesses interleave.
- **Impact:** Under a suspending capture, two accesses to the same (distinct_id, key, value, groups) both pass the check and emit duplicate `$feature_flag_called` events, defeating the `distinct_ids_feature_flags_reported` dedup. Data-quality/contract defect with a clean fix (reserve the key synchronously at schedule time). Reachable via a supported async `before_send` in default mode, so `should_fix` stands.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Reserve the dedup key synchronously in `_schedule_feature_flag_called_event` before creating the task (e.g. compute `feature_flag_reported_key` and add it to `reported_flags` at schedule time, not inside the deferred coroutine), so two accesses to the same key before the task runs cannot both pass the check. Alternatively, key the pending-task set itself by `feature_flag_reported_key` and skip scheduling a new task if one is already pending or already recorded.
</potential_solution>
# Conflicts: # posthog/client.py
| async def get_flags_decision( # type: ignore[override] | ||
| self, | ||
| distinct_id=None, | ||
| groups: Optional[Mapping[str, Union[str, int]]] = None, | ||
| person_properties: Optional[Dict[str, Any]] = None, | ||
| group_properties: Optional[Dict[str, Dict[str, Any]]] = None, | ||
| disable_geoip: Optional[bool] = None, | ||
| flag_keys_to_evaluate: Optional[list[str]] = None, | ||
| device_id: Optional[str] = None, | ||
| ) -> FlagsResponse: |
There was a problem hiding this comment.
thoughts on leaving this out? we have an async evaluate_flags, ideally that's all we need
| ) | ||
| return result.get_value() if result else None | ||
|
|
||
| async def feature_enabled( # type: ignore[override] |
There was a problem hiding this comment.
similar case here and with get_feature_flag_payload. these are both already deprecated, so i think it's worth considering dropping them now, even if it means the async client is somewhat asymmetrical to the sync client
💡 Motivation and Context
Continues the async SDK stack for #103 by adding non-blocking feature flag and remote config APIs on top of
AsyncPosthog.This PR is stacked on #719. It replaces blocking
/flags, flag definition polling, and remote config calls in the async client with awaitable HTTP helpers, while keepingFeatureFlagEvaluations.is_enabled()/.get_flag()synchronous for ergonomic branching.$feature_flag_calledevents are scheduled into the async capture queue and drained byawait flush()/await shutdown().Review feedback was addressed by making feature-flag-called scheduling gracefully no-op with a warning when accessed without a running event loop, asserting the async poller task is started, and adding parameterized remote config coverage.
💚 How did you test it?
uv run ruff check posthog/async_client.py posthog/test/test_async_client.py posthog/test/test_async_feature_flags.pyuv run --extra test pytest posthog/test/test_async_request.py posthog/test/test_async_client.py posthog/test/test_async_feature_flags.py --verbose --timeout=30uv run --extra test pytest posthog/test/test_evaluate_flags.py posthog/test/test_feature_flags.py --verbose --timeout=30uv run python -W error -c "import posthog; from posthog import AsyncPosthog"uv run --extra dev python .github/scripts/check_public_api.py📝 Checklist
If releasing new changes
sampo addto generate a changeset file🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Implemented with pi. The main design choice was to keep flag snapshots synchronous to read after
await evaluate_flags(...), and to schedule their$feature_flag_calledside effects into the async client instead of making every flag accessor awaitable.