fix(flags): bound cohort recursion in local evaluation - #200
Merged
Conversation
Cohort resolution recursed through nested cohort references with nothing bounding the recursion. A cohort referencing itself, directly or through another cohort, recursed until the thread stack was exhausted, and a Rust stack overflow aborts the process rather than panicking catchably, so one bad definitions response takes down the calling server and does it again on restart once the poller refetches. A cycle check alone would not have been enough: a chain of distinct cohorts is acyclic and still recurses once per link, and because each cohort is a separate shallow entry in the manifest such a chain also clears serde_json's nesting limit on the way in. Resolution now tracks the cohorts on the active path, bounding both: a repeat is a cycle, and the path is capped at 100. Both are inconclusive, so the flag falls back to `/flags` rather than resolving wrongly. IDs are released as the recursion unwinds, so the same cohort referenced down sibling branches still resolves. Generated-By: PostHog Code Task-Id: 52072a7c-7431-4b59-8de1-90d9ababf0db
Contributor
posthog-rs-v0 Compliance ReportDate: 2026-08-10 08:30:12 UTC ✅ All Tests Passed!46/46 tests passed Capture Tests✅ 29/29 tests passed View Details
Feature_Flags Tests✅ 17/17 tests passed View Details
|
Contributor
posthog-rs-v1 Compliance ReportDate: 2026-08-10 08:30:49 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
|
|
Reviews (1): Last reviewed commit: "fix(flags): bound cohort recursion in lo..." | Re-trigger Greptile |
marandaneto
reviewed
Aug 8, 2026
Member
|
pushed a fix |
marandaneto
approved these changes
Aug 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Local flag evaluation can abort the calling process.
Cohort resolution recurses through nested cohort references (
match_cohort_by_id->match_property_group->match_property_group_values->match_nested_cohort-> back tomatch_cohort_by_id) with nothing bounding the recursion. A cohort that references itself, directly or through another cohort, recurses until the thread stack is exhausted. Reproduced onmainbefore the fix:A Rust stack overflow is not a catchable panic. It aborts the whole process, not just the evaluating thread, so a single bad definitions response takes down the customer's server, and takes it down again on restart as soon as the poller refetches. That is an outage, not a blip.
Greptile flagged this on #199, where it showed up only because that branch was stacked on #187 before the squash merge. It is not #199's code, so it gets its own PR.
Why a cycle check alone is not enough
A cycle guard bounds repeats, not depth. A chain of distinct cohorts, where cohort N references cohort N+1, is perfectly acyclic and still recurses once per link. It reaches the identical abort.
That case also slips past the one accidental protection that exists today. A single deeply nested property group is rejected at parse time by
serde_json's recursion limit of 128, so the wholeLocalEvaluationResponsefails to deserialize and the previous good cache is kept. But a chain spreads its depth across many separate shallow map entries, none of which comes close to that limit, so it deserializes cleanly and only blows up later during evaluation.So the fix bounds both.
What changed
A
HashSetof the cohort IDs on the active resolution path is threaded through the four private resolution functions, giving two bounds for one mechanism:MAX_COHORT_RESOLUTION_DEPTH(100, against real nesting of a handful of levels)Both return
CohortMatchError::InvalidDefinition, whoserequires_server_evaluation()is alreadytrue, so the error short-circuits the enclosing group and propagates. The flag ends up inconclusive and falls back to/flags. It cannot silently resolve tofalse, which would be a wrong answer rather than a safe fallback. That was traced explicitly throughmatch_property_group_values, where a plainInconclusivewould instead be deferred and could be masked by a decisive sibling in an OR group.IDs are removed as the recursion unwinds, so this tracks the active path rather than everything ever visited. The same cohort referenced twice down sibling branches, which is a diamond and not a cycle, still resolves normally.
No public API change (
api/public-api.txtregenerates clean). Only private function signatures moved.Tests
LocalEvaluator::evaluate_flagwith a cyclic manifest, so the regression is pinned at the level a real bad payload would hitWritten test-first: the self-reference test aborted the test binary with SIGABRT before the fix.
Note on how the depth case was found
The cycle fix was written first, and a review pass specifically asked whether the fix was complete. It was not: the acyclic-chain bypass was caught and reproduced at a 5000-link chain. Worth recording, because "fixed the stack overflow" would have been half true with only the cycle guard, and the remaining half is just as fatal.
Created with PostHog Code