Skip to content

fix(flags): bound cohort recursion in local evaluation - #200

Merged
slshults merged 3 commits into
mainfrom
posthog-code/bound-cyclic-cohort-recursion
Aug 10, 2026
Merged

fix(flags): bound cohort recursion in local evaluation#200
slshults merged 3 commits into
mainfrom
posthog-code/bound-cyclic-cohort-recursion

Conversation

@slshults

@slshults slshults commented Aug 7, 2026

Copy link
Copy Markdown
Member

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 to match_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 on main before the fix:

thread '...test_self_referencing_cohort_is_inconclusive' has overflowed its stack
fatal runtime error: stack overflow, aborting
process didn't exit successfully (signal: 6, SIGABRT)

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 whole LocalEvaluationResponse fails 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 HashSet of the cohort IDs on the active resolution path is threaded through the four private resolution functions, giving two bounds for one mechanism:

  • an ID already on the path is a cycle
  • the path length is capped at MAX_COHORT_RESOLUTION_DEPTH (100, against real nesting of a handful of levels)

Both return CohortMatchError::InvalidDefinition, whose requires_server_evaluation() is already true, so the error short-circuits the enclosing group and propagates. The flag ends up inconclusive and falls back to /flags. It cannot silently resolve to false, which would be a wrong answer rather than a safe fallback. That was traced explicitly through match_property_group_values, where a plain Inconclusive would 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.txt regenerates clean). Only private function signatures moved.

Tests

  • self-referencing cohort is inconclusive
  • two-cohort cycle is inconclusive, and the error names the cycle
  • chain deeper than the limit is inconclusive
  • chain within the limit still resolves, so the bound does not break realistic nesting
  • the same cohort referenced down sibling branches still resolves, which is the guard against implementing this as a naive "visited forever" set
  • end to end through LocalEvaluator::evaluate_flag with a cyclic manifest, so the regression is pinned at the level a real bad payload would hit

Written 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

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
@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

posthog-rs-v0 Compliance Report

Date: 2026-08-10 08:30:12 UTC
Duration: 15508ms

✅ All Tests Passed!

46/46 tests passed


Capture Tests

29/29 tests passed

View Details
Test Status Duration
Format Validation.Event Has Required Fields 138ms
Format Validation.Event Has Uuid 147ms
Format Validation.Event Has Lib Properties 144ms
Format Validation.Distinct Id Is String 146ms
Format Validation.Token Is Present 144ms
Format Validation.Custom Properties Preserved 149ms
Format Validation.Event Has Timestamp 145ms
Retry Behavior.Retries On 503 5149ms
Retry Behavior.Does Not Retry On 400 2137ms
Retry Behavior.Does Not Retry On 401 2148ms
Retry Behavior.Respects Retry After Header 5104ms
Retry Behavior.Implements Backoff 15105ms
Retry Behavior.Retries On 500 5101ms
Retry Behavior.Retries On 502 5101ms
Retry Behavior.Retries On 504 5095ms
Retry Behavior.Max Retries Respected 15100ms
Deduplication.Generates Unique Uuids 102ms
Deduplication.Preserves Uuid On Retry 5024ms
Deduplication.Preserves Uuid And Timestamp On Retry 10032ms
Deduplication.Preserves Uuid And Timestamp On Batch Retry 5030ms
Deduplication.No Duplicate Events In Batch 26ms
Deduplication.Different Events Have Different Uuids 23ms
Compression.Sends Gzip When Enabled 22ms
Batch Format.Uses Proper Batch Structure 23ms
Batch Format.Flush With No Events Sends Nothing 34ms
Batch Format.Multiple Events Batched Together 101ms
Error Handling.Does Not Retry On 403 2090ms
Error Handling.Does Not Retry On 413 2068ms
Error Handling.Retries On 408 5081ms

Feature_Flags Tests

17/17 tests passed

View Details
Test Status Duration
Request Payload.Request With Person Properties Device Id 64ms
Request Payload.Flags Request Uses V2 Query Param 56ms
Request Payload.Flags Request Hits Flags Path Not Decide 44ms
Request Payload.Flags Request Omits Authorization Header 37ms
Request Payload.Token In Flags Body Matches Init 38ms
Request Payload.Groups Round Trip 39ms
Request Payload.Groups Default To Empty Object 38ms
Request Payload.Disable Geoip False Propagates As Geoip Disable False 38ms
Request Payload.Disable Geoip Omitted Defaults To False 42ms
Request Payload.Flag Keys To Evaluate Contains Only Requested Key 40ms
Request Lifecycle.No Flags Request On Init Alone 35ms
Request Lifecycle.No Flags Request On Normal Capture 46ms
Request Lifecycle.Two Flag Calls Produce Two Remote Requests 42ms
Request Lifecycle.Mock Response Value Is Returned To Caller 37ms
Retry Behavior.Retries Flags On 502 233ms
Retry Behavior.Retries Flags On 504 237ms
Side Effect Events.Get Feature Flag Captures Feature Flag Called Event 45ms

@slshults
slshults requested a review from a team August 7, 2026 22:43
@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

posthog-rs-v1 Compliance Report

Date: 2026-08-10 08:30:49 UTC
Duration: 22771ms

✅ All Tests Passed!

111/111 tests passed


Capture_V1 Tests

94/94 tests passed

View Details
Test Status Duration
Endpoint And Method.Targets V1 Endpoint 115ms
Endpoint And Method.Does Not Use Legacy Endpoints 112ms
Required Headers.Has Authorization Bearer Header 114ms
Required Headers.Has Content Type Json 114ms
Required Headers.Has Posthog Sdk Info Format 113ms
Required Headers.Has Posthog Attempt Header 119ms
Required Headers.Has Posthog Request Id 112ms
Required Headers.Has Posthog Request Timestamp 113ms
Required Headers.Has User Agent 111ms
Body Format.Body Has Created At And Batch 112ms
Body Format.No Api Key In Body 92ms
Body Format.No Sent At In Body 91ms
Event Format.Event Has Required Root Fields 99ms
Event Format.Event Uuid Is Valid 90ms
Event Format.Event Timestamp Is Rfc3339 99ms
Event Format.Distinct Id Is String 99ms
Event Format.Distinct Id At Root Not Properties 99ms
Event Format.Custom Properties Preserved 99ms
Event Format.Set Properties Preserved 89ms
Event Format.Set Once Properties Preserved 97ms
Event Format.Groups Properties Preserved 105ms
Event Format.Sdk Generates Uuid If Not Provided 106ms
Event Format.Event Has Required Root Fields Batch 139ms
Event Format.Event Uuid Is Valid Batch 110ms
Event Format.Event Timestamp Is Rfc3339 Batch 142ms
Event Format.Distinct Id Is String Batch 130ms
Event Format.Distinct Id At Root Not Properties Batch 140ms
Event Format.Custom Properties Preserved Batch 132ms
Event Format.Set Properties Preserved Batch 140ms
Event Format.Set Once Properties Preserved Batch 138ms
Event Format.Groups Properties Preserved Batch 109ms
Event Format.Sdk Generates Uuid If Not Provided Batch 108ms
Batch Behavior.Multiple Events In Single Batch 153ms
Batch Behavior.Batch Envelope Smoke 123ms
Batch Behavior.Flush With No Events Sends Nothing 56ms
Batch Behavior.Flush At Triggers Batch 1079ms
Batch Behavior.Created At Reflects Batch Creation Time 70ms
Deduplication.Generates Unique Uuids 133ms
Deduplication.Different Events Same Content Different Uuids 114ms
Deduplication.Preserves Uuid On Retry 5073ms
Deduplication.Preserves Timestamp On Retry 5077ms
Deduplication.Preserves Uuid And Timestamp On Batch Retry 5097ms
Deduplication.No Duplicate Events In Batch 111ms
Header Behavior On Retry.Attempt Header Starts At One 49ms
Header Behavior On Retry.Attempt Header Increments On Retry 10050ms
Header Behavior On Retry.Request Id Preserved On Retry 5060ms
Header Behavior On Retry.Different Requests Have Different Request Ids 2052ms
Header Behavior On Retry.Request Timestamp Changes On Retry 5056ms
Response Format Validation.Success Response Has Uuid Keyed Results 47ms
Response Format Validation.Success Response Has Ok For Each Event 23ms
Response Format Validation.Success No Retry After When All Ok 22ms
Response Format Validation.Success Retry After Present When Retry Events 25ms
Response Format Validation.Success No Retry After When Drop Only 28ms
Response Format Validation.Response Echoes Request Id 20ms
Retry Behavior.Retries On 408 5022ms
Retry Behavior.Retries On 500 5020ms
Retry Behavior.Retries On 503 5020ms
Retry Behavior.Retries On 504 5019ms
Retry Behavior.Retryable Errors Have Retry After 2021ms
Retry Behavior.Respects Retry After On Retryable Error 8018ms
Retry Behavior.Does Not Retry On 400 2025ms
Retry Behavior.Does Not Retry On 401 2023ms
Retry Behavior.Does Not Retry On 402 2028ms
Retry Behavior.Does Not Retry On 413 2023ms
Retry Behavior.Does Not Retry On 415 2019ms
Retry Behavior.Non Retryable Errors Have No Retry After 2020ms
Retry Behavior.Implements Backoff 15026ms
Retry Behavior.Max Retries Respected 15025ms
Partial Batch Handling.Handles 200 Full Success 2039ms
Partial Batch Handling.Handles 200 With All Ok 3031ms
Partial Batch Handling.Does Not Retry Dropped Events 3029ms
Partial Batch Handling.Does Not Retry Limited Events 3026ms
Partial Batch Handling.Prunes Ok Events On Partial Retry 5027ms
Partial Batch Handling.Prunes Dropped Events On Partial Retry 5022ms
Partial Batch Handling.Retries Only Retry Events From Partial 5021ms
Partial Batch Handling.Partial Retry Preserves Uuids 5020ms
Partial Batch Handling.Partial Retry Attempt Header Increments 5024ms
Partial Batch Handling.Partial Retry Request Id Preserved 5028ms
Partial Batch Handling.Respects Retry After On Partial 5020ms
Partial Batch Handling.Unknown Result Treated As Terminal 3020ms
Partial Batch Handling.Mixed Ok Drop Limited No Retry 3029ms
Compression.Sends Gzip Content Encoding 25ms
Compression.No Content Encoding When Disabled 17ms
Compression.Compressed Body Is Decompressible 17ms
Error Handling.Does Not Retry On Unknown 4Xx 2018ms
Event Options.Cookieless Mode Override 20ms
Event Options.Disable Skew Correction Override 17ms
Event Options.Process Person Profile Override 18ms
Event Options.Product Tour Id Override 18ms
Event Options.Unset Options Omitted 18ms
Event Options.Options Override In Batch 17ms
Geoip And Historical Migration.Geoip Disable Injected Into Properties 18ms
Geoip And Historical Migration.Historical Migration Set In Body 17ms
Geoip And Historical Migration.Historical Migration Absent By Default 17ms

Feature_Flags Tests

17/17 tests passed

View Details
Test Status Duration
Request Payload.Request With Person Properties Device Id 12ms
Request Payload.Flags Request Uses V2 Query Param 12ms
Request Payload.Flags Request Hits Flags Path Not Decide 12ms
Request Payload.Flags Request Omits Authorization Header 11ms
Request Payload.Token In Flags Body Matches Init 12ms
Request Payload.Groups Round Trip 11ms
Request Payload.Groups Default To Empty Object 11ms
Request Payload.Disable Geoip False Propagates As Geoip Disable False 13ms
Request Payload.Disable Geoip Omitted Defaults To False 11ms
Request Payload.Flag Keys To Evaluate Contains Only Requested Key 12ms
Request Lifecycle.No Flags Request On Init Alone 9ms
Request Lifecycle.No Flags Request On Normal Capture 17ms
Request Lifecycle.Two Flag Calls Produce Two Remote Requests 13ms
Request Lifecycle.Mock Response Value Is Returned To Caller 11ms
Retry Behavior.Retries Flags On 502 215ms
Retry Behavior.Retries Flags On 504 215ms
Side Effect Events.Get Feature Flag Captures Feature Flag Called Event 19ms

@slshults
slshults requested a review from marandaneto August 7, 2026 22:43
@greptile-apps

greptile-apps Bot commented Aug 7, 2026

Copy link
Copy Markdown

Reviews (1): Last reviewed commit: "fix(flags): bound cohort recursion in lo..." | Re-trigger Greptile

Comment thread src/feature_flags.rs
@marandaneto
marandaneto requested a review from a team August 8, 2026 07:16
@marandaneto

Copy link
Copy Markdown
Member

pushed a fix

@slshults
slshults merged commit 962282c into main Aug 10, 2026
25 checks passed
@slshults
slshults deleted the posthog-code/bound-cyclic-cohort-recursion branch August 10, 2026 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants