feat(flags): hypercache flag provider#50737
Merged
matheus-vb merged 23 commits intomasterfrom Mar 17, 2026
Merged
Conversation
haacked
reviewed
Mar 11, 2026
Contributor
There was a problem hiding this comment.
Excited for this. Curious why this is a separate PR from PostHog/posthog-python#462 which is effectively a two-line change. Why not combine them?
Update: Dumb question. Different repos
Contributor
|
After we merge PostHog/posthog-python#462, we'll need to edit |
Contributor
|
One more question:
Do we need every single one of those to do that? Perhaps that's an audit for a follow-up. |
Contributor
Prompt To Fix All With AIThis is a comment left during a code review.
Path: posthog/feature_flags/sdk_cache_provider.py
Line: 51
Comment:
**Module-level `assert` is unreliable and redundant**
`assert` statements are silently stripped when Python runs with the `-O` (optimize) flag, so this protocol compliance check provides no guarantee in production. Additionally, it duplicates the `test_implements_protocol` test case, violating the OnceAndOnlyOnce principle and "no superfluous parts" rule.
Remove this and rely solely on the test — or if a hard import-time guard is wanted, use an explicit `raise` instead:
```suggestion
# Protocol compliance is verified in tests (TestHyperCacheFlagProvider.test_implements_protocol)
```
Or, if a runtime guard is genuinely needed:
```python
if not isinstance(HyperCacheFlagProvider(team_id=0), FlagDefinitionCacheProvider):
raise TypeError("HyperCacheFlagProvider does not implement FlagDefinitionCacheProvider protocol")
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: posthog/feature_flags/test_sdk_cache_provider.py
Line: 22-63
Comment:
**Tests not parameterised (repo guidelines)**
The four `get_flag_definitions` test variants (`cache_hit`, `cache_miss`, `exception_returns_none`, `handles_missing_keys`) all share the same structure: patch the hypercache, call `get_flag_definitions()`, assert the result. The repository guidelines say "we always prefer parameterised tests."
These can be consolidated into a single `@pytest.mark.parametrize` test:
```python
import pytest
@pytest.mark.parametrize(
"cache_return, side_effect, expected_flags, expected_group_mapping, expected_cohorts",
[
(
{"flags": [{"key": "test-flag", "active": True}], "group_type_mapping": {"0": "company"}, "cohorts": {"1": {"properties": []}}},
None,
[{"key": "test-flag", "active": True}],
{"0": "company"},
{"1": {"properties": []}},
),
(None, None, None, None, None), # cache miss
({"flags": [{"key": "flag-1"}]}, None, [{"key": "flag-1"}], {}, {}), # missing keys default
(None, Exception("Redis connection failed"), None, None, None), # exception
],
)
@patch("posthog.models.feature_flag.local_evaluation.flag_definitions_hypercache")
def test_get_flag_definitions(self, mock_hypercache, cache_return, side_effect, expected_flags, ...):
...
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: posthog/apps.py
Line: 72-78
Comment:
**Missing comment on `personal_api_key` fallback behaviour**
This code path changes how `personal_api_key` is used: it is no longer polled at startup but is now only the emergency fallback if `HyperCacheFlagProvider.get_flag_definitions()` returns `None`. In the E2E testing branch, `personal_api_key` is explicitly set to `None`, meaning both the HyperCache read _and_ the emergency fallback will silently produce no flags. An inline comment explaining this relationship (and what happens when `personal_api_key` is `None` and the cache is cold) would help future readers.
```suggestion
# Use HyperCache to provide flag definitions instead of per-process API polling.
# Falls back to the SDK's emergency API fetch (via personal_api_key) only when
# the cache is cold. Note: in E2E testing personal_api_key is None, so a cold
# cache will result in no flag definitions being loaded — which is acceptable there.
from posthog.feature_flags.sdk_cache_provider import HyperCacheFlagProvider
posthoganalytics.flag_definition_cache_provider = HyperCacheFlagProvider(
team_id=int(os.environ.get("POSTHOG_SELF_TEAM_ID", "2"))
)
```
**Rule Used:** Add inline comments to clarify the purpose of sign... ([source](https://app.greptile.com/review/custom-context?memory=4d5b48c5-045d-4693-9cd9-4081bb19508b))
**Learnt From**
[PostHog/posthog#32083](https://github.com/PostHog/posthog/pull/32083)
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: 702909d |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Member
Author
|
@haacked With this PR the polling is already eliminated, every process reads from Redis instead. But I guess auditing whether all process types actually need flag definitions is a good follow-up, agreed |
Contributor
❌ Hobby deploy smoke test: FAILEDFailing fast because: Timed out after 45 minutes Connection errors: 144 Run 23200565619 | Consecutive failures: 1 |
haacked
approved these changes
Mar 12, 2026
Contributor
haacked
left a comment
haacked
left a comment
There was a problem hiding this comment.
Excited to see us stop slamming ourselves!
Contributor
Visual regression: Playwright E2E E2E screenshots updatedChanges: 2 snapshots (2 modified, 0 added, 0 deleted) What this means:
Next steps:
|
matheus-vb
force-pushed
the
matheus-vb/hypercache-flag-provider
branch
from
9b17583 to
eb4578f
Compare
Contributor
|
Size Change: 0 B Total Size: 109 MB ℹ️ View Unchanged
|
Contributor
Visual regression: Playwright E2E E2E screenshots updatedChanges: 2 snapshots (2 modified, 0 added, 0 deleted) What this means:
Next steps:
|
MattBro
pushed a commit
that referenced
this pull request
Mar 17, 2026
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
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.
Problem
Every Django process (web workers, Celery workers, Temporal workers) independently polls the
/api/feature_flag/local_evaluation/endpoint every 90 seconds to fetch feature flag definitions. Multiplied across all workers, this creates significant unnecessary load on team 2 (PostHog's own project). The monolith already has a HyperCache infrastructure that maintains flag definitions in Redis (updated via Django signals on flag changes + periodic refresh tasks), but the SDK's module-level API doesn't use it.Solution
Create a read-only
HyperCacheFlagProviderthat implements the SDK'sFlagDefinitionCacheProviderprotocol, reading flag definitions directly from the existing HyperCache instead of polling the API. The provider:Falsefromshould_fetch_flag_definitions()so the SDK never polls the APIflag_definitions_hypercache.get_from_cache(team_id)on each evaluation cyclepersonal_api_keyChanges
posthog/feature_flags/sdk_cache_provider.py(new)HyperCacheFlagProviderclass implementingFlagDefinitionCacheProviderflag_definitions_hypercacheto avoid circular importsNoneto let the SDK handle fallbackposthog/apps.pyposthoganalytics.flag_definition_cache_providerwithHyperCacheFlagProviderbeforeload_feature_flags()POSTHOG_SELF_TEAM_IDenv varposthoganalyticsis not disabled (skipped in tests, local dev, OPT_OUT_CAPTURE)posthog/feature_flags/test_sdk_cache_provider.py(new)SDK dependency (separate PR, merge first)
posthog/__init__.pyinposthog-pythonflag_definition_cache_providermodule-level variablesetup()to theClientconstructorHow did you test this code?
HyperCacheFlagProviderunit tests passruff checkandruff formatcleanFlagDefinitionCacheProviderprotocol compliance at runtimeNote
PostHog/posthog-python#462 should be merged before this one.
Publish to changelog?
No