Skip to content

fix(flags): fall back for incompatible local regex evaluation - #231

Merged
marandaneto merged 5 commits into
mainfrom
fix/local-regex-compatibility-5354
Oct 23, 2026
Merged

marandaneto merged 5 commits into
mainfrom
fix/local-regex-compatibility-5354

Conversation

@marandaneto

@marandaneto marandaneto commented Oct 22, 2026 •

Copy link
Copy Markdown
Member

💡 Motivation and Context

Local feature flag evaluation can return a definitive PCRE result that disagrees with the flags service, preventing remote fallback. For example, ^foo$ matches foo\n in default PCRE but not in the service. PCRE also treats \v as vertical whitespace, so it matches LF, CR and FF, while the service treats it as vertical tab only.

This adapts the approach discussed in posthog-js#5354 and the cross-SDK agreement to Elixir's engine. It does not assume that the JavaScript change has been merged or released.

Keep known shared ASCII patterns local and use the existing inconclusive path for uncertain syntax or semantics. This deliberately reduces local coverage for non-ASCII values and patterns, Unicode/POSIX/set classes, advanced groups and flags, word boundaries, and the \v escape. Normal callers can fall back remotely. Local-only callers leave affected flags unresolved. The strict end-anchor option keeps the final-LF case local with the service result. Empty patterns, escaped punctuation, literal/hex vertical tab and class backspace remain local.

The ASCII input check now scans the binary tail-recursively and stops at the first non-ASCII byte. It does not first allocate a list of every byte. This changes allocation and early-exit cost, not which values resolve locally.

Execution now uses the compiled pattern with :re.run, :report_errors and no captures. With the default local runtime budget, ^(a+)+$|^a+\n$ on 30 ASCII as plus LF exhausts the match limit before its matching alternative. Regex.match?/2 had hidden this as false, so the old local regex/not_regex results were false/true while the actual service returns true/false. Execution errors now stay inconclusive before either operator or cohort negation. The strict-dollar compile option, binary input scan, engine and default match budget are unchanged.

Only proven shared compilation errors become definitive false results for both regex and not_regex. Other compile or execution failures remain inconclusive. A private invalid-pattern result is normalized before direct negation. Inside cohorts it remains inconclusive so leaf, group or reference negation cannot turn an invalid search into a match. No public API, dependency or regex engine is added.

💚 How did you test it?

Regression tests failed before the original fix and before the vertical-tab correction. After integrating current main locally:

  • Focused matcher/public fallback tests: 190 passed.
  • Feature-flag tests: 470 passed.
  • All tests except test/posthog/integrations/plug_test.exs: 750 passed, 18 excluded.
  • Format, strict Credo, compilation with warnings as errors, public API snapshot and Hex package build passed.
  • The accepted candidate had zero wrong definitive results against the actual frozen service on 9,736 expanded operator cases and 170 additional supervisor/revision cases. These corpora overlap and are not exhaustive regex or resource-budget parity.
  • Final exact-commit autoreview completed against refreshed main after a signed docs-only main merge. Its stacked-repetition finding was rejected after the actual service and final evaluator both returned false for both operators on all six anchored cases, supported by 24 related earlier cases.

A bounded before/after scanner benchmark used five fresh-process samples each at 1 KiB, 64 KiB and 1 MiB, with ASCII and leading/trailing non-ASCII inputs. The old 1 MiB byte list contains 16 MiB of list cells on this 64-bit BEAM. Median immediate process memory growth was 43,776,352 bytes before and zero observed growth after, for all three 1 MiB cases. This is retained heap growth, not a total-allocation measurement. Leading-byte fallback used 18,572 reductions before and 12 after. Timings were descriptive only. Normal tests have no timing or memory thresholds.

The execution-error regression used the actual default runtime and real evaluator before source changes, with no artificial compile limit or mocked engine. Four focused tests failed before the change and all 190 focused tests pass now. Coverage includes both operators, direct and nested/reference/leaf cohort negation, local-only omission, remote fallback, and retained invalid/ordinary local results.

Public-boundary tests intercept the existing HTTP client, not the regex engine. They check remote fallback, unresolved local-only results, a missing-property control, and ordinary ASCII patterns that must remain local.

Local runtime: Elixir 1.20.4 / OTP 29 / PCRE2 10.47. Older supported runtimes still need CI. The unfiltered suite is not locally green: seven unrelated Plug/Bandit/Cowboy tests previously failed because port 8001 is occupied. Those tests were not removed or changed, and no process was killed to free the port. Live-service tests were not run. No all-green CI claim is made. The existing posted compliance report lists seven failures outside this input-scan feedback change; this update neither fixes nor disproves them.

📝 Checklist

  • All commits are signed (see Commit signing)
  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • I updated the docs if needed. No public documentation change is needed.
  • No breaking change or entry added to the changelog.

If releasing new changes

  • Ran sampo add to generate a changeset file

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

Directed by @marandaneto using a Pi implementation agent, local Git/gh/Mix tools, the frozen service oracle, and the isolated Pi autoreview helper. No public session link is available. A human review is still required.

The chosen approach is conservative fallback rather than a new regex engine or a Unicode-mode switch. Review caught the PCRE vertical-whitespace difference, which now defers in both class and non-class forms. Invalid-pattern handling stays private and preserves cohort error propagation.

PR review follow-up: PCRE verbs and misplaced quantifiers

Addressed the remaining suggestion in 8b442012a3ad0bd1c2120c4bb6f6bc4c8191de2e. The portability scanner now rejects *, +, and ? after the start of the pattern, an opening group, or an alternation. This prevents PCRE-only verbs and controls such as (*FAIL), (*ACCEPT), (*COMMIT), and (*LIMIT_MATCH=1) from producing definitive local results. Both operators remain inconclusive through direct and cohort negation; ordinary callers can fall back remotely, and local-only snapshots omit the unresolved flag. Valid quantifiers, including lazy and grouped repetition, remain local.

Contract note: the local evaluator spec specifies definitive false for syntactically invalid patterns. This conservative implementation deliberately defers misplaced quantifiers (including previously definitive * and ?*) and PCRE-only controls instead of attempting to validate them against the service grammar. Remote evaluation remains eligible; local-only callers receive unresolved results rather than definitive false. Proven shared invalid patterns such as [ retain their existing false result. No new public API is introduced.

Latest validation, superseding the earlier counts above:

  • Before changing production code, mix test test/posthog/feature_flags/regex_compatibility_test.exs test/posthog/feature_flags/local_evaluation_integration_test.exs failed 32 tests, including both public fallback regressions. The same command now passes all 226 tests.
  • Feature-flag tests: 506 passed.
  • All tests except test/posthog/integrations/plug_test.exs: 786 passed, 18 excluded.
  • Full suite: 799 passed, seven Plug/Bandit/Cowboy failures due to occupied port 8001, 18 excluded. No test was removed and no process was killed.
  • Format check, strict Credo, MIX_ENV=test mix compile --warnings-as-errors, public API snapshot, and git diff --check passed.
  • The new public-boundary tests mock only the existing HTTP client and check local-only omission and remote results, with missing-property controls. No new live-service comparison or independent autoreview was run for this follow-up.

@marandaneto marandaneto self-assigned this Oct 22, 2026
@github-actions

github-actions Bot commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

posthog-elixir Compliance Report

Date: 2026-10-23T10:47:41.839878+00:00
Duration: 108424ms

⚠️ Some Tests Failed

40/47 tests passed, 7 failed


Capture Tests

⚠️ 29/30 tests passed, 1 failed

View Details
Test Status Duration
Format Validation.Event Has Required Fields ✅ 609ms
Format Validation.Event Has Uuid ✅ 608ms
Format Validation.Event Has Lib Properties ✅ 607ms
Format Validation.Distinct Id Is String ✅ 608ms
Format Validation.Token Is Present ✅ 608ms
Format Validation.Custom Properties Preserved ✅ 610ms
Format Validation.Event Has Timestamp ✅ 610ms
Format Validation.Non Utc Event Timestamp Is Converted To Utc ❌ 613ms
Retry Behavior.Retries On 503 ✅ 5613ms
Retry Behavior.Does Not Retry On 400 ✅ 2611ms
Retry Behavior.Does Not Retry On 401 ✅ 2610ms
Retry Behavior.Respects Retry After Header ✅ 5614ms
Retry Behavior.Implements Backoff ✅ 15624ms
Retry Behavior.Retries On 500 ✅ 5614ms
Retry Behavior.Retries On 502 ✅ 5614ms
Retry Behavior.Retries On 504 ✅ 5613ms
Retry Behavior.Max Retries Respected ✅ 15624ms
Deduplication.Generates Unique Uuids ✅ 614ms
Deduplication.Preserves Uuid On Retry ✅ 5614ms
Deduplication.Preserves Uuid And Timestamp On Retry ✅ 10617ms
Deduplication.Preserves Uuid And Timestamp On Batch Retry ✅ 5616ms
Deduplication.No Duplicate Events In Batch ✅ 612ms
Deduplication.Different Events Have Different Uuids ✅ 610ms
Compression.Sends Gzip When Enabled ✅ 608ms
Batch Format.Uses Proper Batch Structure ✅ 608ms
Batch Format.Flush With No Events Sends Nothing ✅ 605ms
Batch Format.Multiple Events Batched Together ✅ 611ms
Error Handling.Does Not Retry On 403 ✅ 2611ms
Error Handling.Does Not Retry On 413 ✅ 2610ms
Error Handling.Retries On 408 ✅ 5613ms

Failures

format_validation.non_utc_event_timestamp_is_converted_to_utc

Event 0 field 'timestamp' instant '2026-10-23T10:45:57.857211Z' != expected '2025-01-02T03:04:05Z'

Feature_Flags Tests

⚠️ 11/17 tests passed, 6 failed

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

Failures

request_payload.groups_default_to_empty_object

Field 'groups' not found in /flags request body at path 'groups'. Available keys: ['api_key', 'distinct_id', 'flag_keys_to_evaluate']

request_payload.disable_geoip_omitted_defaults_to_false

Field 'geoip_disable' not found in /flags request body at path 'geoip_disable'. Available keys: ['api_key', 'distinct_id', 'flag_keys_to_evaluate']

request_lifecycle.mock_response_value_is_returned_to_caller

Last action result missing field 'value'. Keys: ['error', 'success']

retry_behavior.retries_flags_on_502

Last action result missing field 'value'. Keys: ['error', 'success']

retry_behavior.retries_flags_on_504

Last action result missing field 'value'. Keys: ['error', 'success']

side_effect_events.get_feature_flag_captures_feature_flag_called_event

Expected 1 events with name '$feature_flag_called', got 0

@posthog posthog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PostHog Review · Standard · reviewed 43c0ac1: 1 Should fix.

This is the only automatic review while this pull request is a draft. Comment @posthog review to review new changes sooner. Otherwise, a review of the new changes runs when you mark it ready for review.

Comment thread lib/posthog/feature_flags/local_evaluator.ex Outdated
@marandaneto
marandaneto requested a review from a team October 23, 2026 08:43
@marandaneto
marandaneto marked this pull request as ready for review October 23, 2026 08:43
@marandaneto
marandaneto requested a review from a team as a code owner October 23, 2026 08:43
@posthog-project-board-bot posthog-project-board-bot Bot moved this to In Progress in Feature Flags Oct 23, 2026
@posthog-project-board-bot posthog-project-board-bot Bot moved this from In Progress to In Review in Feature Flags Oct 23, 2026

@posthog posthog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PostHog Review · Standard · reviewed 85fa63b: 1 Should fix.

This Standard review started from the posthog-review label. Comment @posthog review --deep for a Deep review, or @posthog help to see all commands.

Comment thread lib/posthog/feature_flags/local_evaluator.ex Outdated
@posthog posthog Bot removed the posthog-review label Oct 23, 2026
Comment thread lib/posthog/feature_flags/local_evaluator.ex Outdated
@github-project-automation github-project-automation Bot moved this from In Review to Approved in Feature Flags Oct 23, 2026
@posthog-project-board-bot posthog-project-board-bot Bot moved this from Approved to In Review in Feature Flags Oct 23, 2026
@marandaneto
marandaneto merged commit 5c2e74f into main Oct 23, 2026
31 checks passed
@marandaneto
marandaneto deleted the fix/local-regex-compatibility-5354 branch October 23, 2026 10:47
@github-project-automation github-project-automation Bot moved this from In Review to Done in Feature Flags Oct 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants