feat(flags): add local feature flag evaluation - #192
Conversation
posthog-elixir Compliance ReportDate: 2026-08-26 16:12:03 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
|
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
| defp apply_operator(operator, property, filter, _now) when operator in ["regex", "not_regex"] do | ||
| case Regex.compile(to_string(filter)) do | ||
| {:ok, regex} -> | ||
| matched = Regex.match?(regex, to_string(property)) |
There was a problem hiding this comment.
Low: Unbounded regular-expression evaluation
A definitions response can supply a backtracking-heavy pattern that is matched synchronously against request-derived property values. Repeated evaluations can consume scheduler CPU before the evaluator's exception handling runs; Regex.match?/2 provides no option to set a smaller per-call match limit. citeturn0search0turn0search1 Compile and validate regexes when definitions are loaded, then evaluate them with a conservative match limit or inside a time-bounded worker.
There was a problem hiding this comment.
This is a valid hardening concern, but I am leaving it for a focused follow-up. Regex patterns come from authenticated project definitions, and adding a match limit or worker timeout needs a documented evaluator contract plus parity tests across SDKs. I am leaving this thread unresolved.
PR overviewThis pull request adds local evaluation of PostHog feature flags, including property matching with regular-expression conditions. Three security issues have been addressed, with one concern remaining. Feature flag definitions can contain computationally expensive regular expressions that are evaluated synchronously against request-derived values, potentially causing excessive scheduler CPU use. Exploitation depends on an attacker being able to influence the supplied definitions and matching inputs. Open issues (1)
Fixed/addressed: 3 · PR risk: 4/10 |
Prompt To Fix All With AI### Issue 1
lib/posthog/feature_flags.ex:343-347
**Malformed fallback response crashes**
When `/flags` returns HTTP 200 with a non-map `flags` value, `request_flags/2` accepts the response but neither branch here matches it, causing `evaluate_flags/2` to raise `CaseClauseError` instead of returning the safe local subset.
```suggestion
merged = Map.merge(remote_results, local_results)
{:ok, __MODULE__.Evaluations.from_results(name, distinct_id, merged, response_body)}
{:ok, _response} ->
snapshot_from_results(name, distinct_id, local_results)
{:error, _reason} ->
snapshot_from_results(name, distinct_id, local_results)
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "feat(flags): add local feature flag eval..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Preserve exact remote flag scopes while bypassing per-key coordination and negative caching for oversized missing-key requests. Keep retained negative knowledge updates bounded and linear.
dustinbyrne
left a comment
dustinbyrne
left a comment
There was a problem hiding this comment.
Agent-led review, human-reviewed before posting.
Thanks for the follow-up fixes. I re-reviewed the current head (089ae9e4) and left two material correctness/performance comments, two non-blocking no-throw/telemetry comments, and one compatibility consideration.
Submitted as CHANGES_REQUESTED by mistake; the inline feedback was intended to be non-blocking review comments.
ioannisj
left a comment
ioannisj
left a comment
There was a problem hiding this comment.
Left some comments my agent spotted and made sense to me. Could not follow 100% so discount these accordingly
| end | ||
| end | ||
|
|
||
| defp handle_response(state, {:ok, %{status: 304} = response}, _store?) do |
There was a problem hiding this comment.
I think the 304 branch should stop calling successful_definition_refresh/2. It resets negative_keys/negative_order and bumps definition_generation, which is right after a 200 carrying a new snapshot, but a 304 means the definitions provably didn't change, so nothing we learned about missing keys has been invalidated.
In steady state that drops the missing-key knowledge every poll interval, so the next call naming an unknown key re-enters the probe path, takes the :global lock and fires another /flags request, against a definition set that never changes. The bounding described in the PR body holds within a poll cycle but not across one.
Worth having the 304 clause only touch :etag, :loaded_at and quota_backoff_ms? missing_key_knowledge_test.exs covers persistence across reloads but nothing pins the 304 case, so a regression here would still pass.
There was a problem hiding this comment.
Thanks for raising this. The current behavior is intentional under the evaluate-flags sdk-spec: negative knowledge SHALL be cleared after every successful definitions refresh, explicitly including an unchanged/304 response. The bound is therefore one probe caused solely by that key per successful refresh interval, rather than indefinite suppression. The existing clean omission is retained sequentially and successful refresh invalidates it regression pins this distinction by retaining across a 503 and then asserting that a 304 makes the next probe eligible. I’m keeping the 304 behavior aligned with that contract.
|
|
||
| defp acquire_missing_probe_locks([key | rest], name, callback) do | ||
| lock = {{__MODULE__, :missing_probe, name, key}, self()} | ||
| :global.trans(lock, fn -> acquire_missing_probe_locks(rest, name, callback) end) |
There was a problem hiding this comment.
I think the lock wants releasing before the HTTP call. acquire_missing_probe_locks/3 wraps callback.() in :global.trans, and that callback runs all the way through to PostHog.API.flags, so the lock is held for the whole retrying round trip rather than just long enough to claim the probe. :global.trans/2 also defaults to infinite retries when contended.
On a multi-node deploy that makes one unknown flag key a cluster-wide serialization point: every caller naming that key queues behind a single in-flight request, with no bound on the wait. It's worst exactly when PostHog is slow, which is when local evaluation is meant to be insulating callers from it.
Could we claim the probe under the lock (an in-flight marker in the node-local loader state), release, then issue the request outside it, and pass an explicit Retries count to :global.trans/3 so a contended waiter falls back to its own call? Worth a comment saying the cluster-wide dedup is deliberate too, posthog-python has no cross-process probe coordination at all so the next reader will wonder.
There was a problem hiding this comment.
Thanks — the lock spanning the remote request is deliberate. The evaluate-flags sdk-spec requires overlapping evaluations to coordinate while the existence probe is in flight and forbids a duplicate request when the shared probe cleanly omits the key. Releasing after a node-local claim and allowing a contended caller to issue its own request would break that guarantee; holding the sorted per-key locks also lets disjoint keys proceed independently and avoids multi-key deadlocks. I added a clarifying comment in 335c3eb, but I’m keeping the coordination behavior. A bounded-wait/duplicate-probe tradeoff would need a corresponding contract change.
dustinbyrne
left a comment
dustinbyrne
left a comment
There was a problem hiding this comment.
Agent-led review, approved on behalf of my beloved human. 🫡
I rechecked the current head (335c3ebd). The prior findings are addressed, the focused SDK tests pass, and the remaining cast-ordering concern is non-blocking. I left one P2 inline.
3 participants
💡 Motivation and Context
PostHog Elixir currently evaluates feature flags through the remote
/flagsendpoint. Applications cannot poll flag definitions and evaluate supported flags locally, which adds network latency and makes flag checks depend on a successful request.This adds local feature flag evaluation for applications that configure a project secret key or a suitably scoped personal API key. Existing applications without a secret key remain remote-only.
Closes #148.
Changes
/flags/definitionswith ETag support and keep the last valid definitions when a refresh fails. Evaluations can continue reading the cached definitions while a refresh is in progress.distinct_idand$group_keyproperties when callers do not provide overrides.is_setusing property-key presence, including explicitly providednil, to match the flags-service runtime and the corrected contract in sdk-specs#49./flagsrequest when local data is unavailable or inconclusive. Empty unscoped definition snapshots also fall back unless local-only mode is requested.disable_geoipto the/flagswire keygeoip_disable.$feature_flag_calleddeduplication. Called events also report whether a result was evaluated locally.secret_keyoption.💚 How did you test it?
mix format --check-formattedmix compile --warnings-as-errorsmix credo --strictmix posthog.public_api --checkmix test- 400 tests passed and 18 integration-tagged tests were excludedgit diff --check origin/main...HEADThe new tests cover local evaluator behavior, definition polling, ETag responses, stale definitions during refresh, empty definitions, cache providers, malformed and failed remote fallbacks, GeoIP request translation, group-aware called-event deduplication, local-evaluation provenance, named instances, concurrent missing-key probes, delayed omission ordering, and public API integration with mocked PostHog responses.
📝 Checklist
If releasing new changes
sampo addto generate a changeset file🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Implemented with the Pi coding agent. The implementation was compared with the flags-service runtime, the current SDK specification, and maintained server SDKs. Final autoreview reported no actionable findings for the branch tree signed in commit
95b7f02.