Skip to content

feat(flags): honor experiment holdouts in local flag evaluation - #301

Merged
ioannisj merged 1 commit into
mainfrom
posthog/ruby-local-eval-holdouts
Oct 16, 2026
Merged

ioannisj merged 1 commit into
mainfrom
posthog/ruby-local-eval-holdouts

Conversation

@posthog

@posthog posthog Bot commented Oct 12, 2026

Copy link
Copy Markdown
Contributor

💡 Motivation and Context

Why: The cross-SDK compliance matrix in PostHog/sdk-specs marks this SDK's Local Feature Flag Evaluator row 🟡 Partial, with experiment holdouts as the only open gap — grep -rn 'holdout' lib/ returned zero matches. A flag configured with an experiment holdout was evaluated locally as if the holdout did not exist, so every identity inside the holdout silently got an answer that disagreed with the backend. That is a correctness divergence between local and remote evaluation, not a missing convenience.

Contract: Local Feature Flag Evaluator — Experiment holdouts precede release conditions and its companion requirement Holdout membership uses backend-compatible bucketing.

Matrix note (compliance/posthog-ruby.md, n15):

Holdouts are entirely absent: grep -rn 'holdout' lib/ returns zero matches, and match_feature_flag_properties goes straight from the flag's filters to flag_conditions = flag_filters[:groups] with no holdout gate. […] Remediation: read flag_filters[:holdout] before iterating flag_conditions; when present, hash the bucketing identity per "Holdout membership uses backend-compatible bucketing" and return the holdout outcome when the identity falls inside exclusion_percentage, keeping the percentage a float.

What changed

  • match_feature_flag_properties consults filters.holdout before iterating release conditions. A held-out identifier resolves to the synthetic variant holdout-<id>, which does not need to appear in filters.multivariate.variants. The inactive-flag check still runs first, so an inactive flag stays false.
  • holdout_variant_for skips a holdout whose id or exclusion_percentage is missing or null, leaving ordinary evaluation unchanged. The percentage is clamped to 0–100 without truncating fractions, compared inclusively, and 100 holds everyone out without computing a hash.
  • _holdout_hash hashes holdout-<bucketing value> — no flag key, holdout id or salt. The existing dot-separated _hash selects a different population and is deliberately not reused.
  • Membership uses the flag-level bucketing identity the caller already resolved (distinct id, or the group key for a group-aggregated flag). Per-condition aggregation overrides inside the loop do not change it.

The same gap is being closed in posthog-go#360 and posthog-php#254; this change keeps Ruby's semantics and hash vectors aligned with those.

Backwards compatibility

Backwards-compatible. No public API changes — bundle exec rake public_api:check reports no snapshot diff, and both new methods are internal to FeatureFlagsPoller. Definitions already round-trip filters.holdout untouched because the loader stores whole filters hashes, so no loader or cache change was needed. Evaluated values change only for flags that actually carry a holdout, where today's local answer is wrong and the new one matches the server. Flags without a holdout are byte-for-byte unaffected.

💚 How did you test it?

New spec/posthog/feature_flags_holdouts_spec.rb (16 examples) covering each scenario in the spec: the two server hash vectors (user-1 → ~0.178056, user-5 → ~0.656381), holdout precedence over an unmatchable property filter / 0% rollout / condition variant override, inactive flags staying false, no-holdout and incomplete-holdout definitions assigning normally, inclusive and clamped percentage boundaries (12.5 / 12.4 / 0 / -10 / 100 / 150), the flag-level group key winning over the distinct id, and a bulk pass where a dependent flag compares against holdout-727. Verified the suite fails (9 of 16) with the implementation reverted.

Full CI-aligned checks run locally: bundle exec rspec (1277 examples, 0 failures, 2 pending — the OTel-bundle integration tests), bundle exec rubocop (clean), bundle exec rake public_api:check (no diff).

Follow-up

  • Device-id bucketing: the spec's device-bucketed holdout scenario has no counterpart here, since this SDK has no device-id bucketing path. If one is added, holdout membership must use that identity.
  • A holdout whose exclusion_percentage is present but non-numeric is skipped. The spec only defines missing/null; skipping is the conservative reading.

📝 Checklist

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • I updated the docs if needed.
  • No breaking change or entry added to the changelog.

If releasing new changes

  • Ran pnpm changeset to generate a changeset file

🤖 Agent context

Autonomy: Fully autonomous

Written by Claude Opus 5 running as an unattended PostHog workflow that reads the compliance/ matrices in PostHog/sdk-specs and implements one open, backwards-compatible gap per run. This gap was chosen over the other open Ruby items because it is a silent correctness divergence on a widely-used surface, it is additive, and it is the only one of the three server SDKs sharing it without a PR already in flight.

Two implementation decisions worth a reviewer's attention: the holdout gate sits at the top of match_feature_flag_properties rather than in _compute_flag_locally, so it runs after the inactive-flag check and uses the bucketing identity the caller already resolved for both the person and group paths; and _holdout_hash is a separate function rather than a parameter on _hash, because the separator and the absence of a salt are what select the holdout population and inlining them into the flag hash would make that easy to lose.


Created with PostHog Desktop

🤖 Generated with Claude Code

A flag definition carrying `filters.holdout` was evaluated locally as though the
holdout did not exist, so every identity inside the holdout got a variant that
disagreed with the server.

Check the holdout before release conditions, using the flag-level bucketing
identity, and resolve a held-out identifier to the synthetic `holdout-<id>`
variant. Membership hashes `holdout-<bucketing value>` rather than reusing the
dot-separated flag hash, which selects a different population.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Generated-By: PostHog Desktop
Task-Id: 4c71ea2a-1fb6-45d3-ace5-a6acc28b9676
@github-actions

Copy link
Copy Markdown

posthog-ruby-async Compliance Report

Date: 2026-10-12T06:09:17.483614+00:00
Duration: 98381ms

⚠️ Some Tests Failed

46/47 tests passed, 1 failed


Capture Tests

✅ 30/30 tests passed

View Details
Test Status Duration
Format Validation.Event Has Required Fields ✅ 108ms
Format Validation.Event Has Uuid ✅ 106ms
Format Validation.Event Has Lib Properties ✅ 108ms
Format Validation.Distinct Id Is String ✅ 106ms
Format Validation.Token Is Present ✅ 107ms
Format Validation.Custom Properties Preserved ✅ 106ms
Format Validation.Event Has Timestamp ✅ 106ms
Format Validation.Non Utc Event Timestamp Is Converted To Utc ✅ 9ms
Retry Behavior.Retries On 503 ✅ 5311ms
Retry Behavior.Does Not Retry On 400 ✅ 2108ms
Retry Behavior.Does Not Retry On 401 ✅ 2109ms
Retry Behavior.Respects Retry After Header ✅ 8115ms
Retry Behavior.Implements Backoff ✅ 15522ms
Retry Behavior.Retries On 500 ✅ 5212ms
Retry Behavior.Retries On 502 ✅ 5212ms
Retry Behavior.Retries On 504 ✅ 5212ms
Retry Behavior.Max Retries Respected ✅ 15722ms
Deduplication.Generates Unique Uuids ✅ 111ms
Deduplication.Preserves Uuid On Retry ✅ 5211ms
Deduplication.Preserves Uuid And Timestamp On Retry ✅ 10317ms
Deduplication.Preserves Uuid And Timestamp On Batch Retry ✅ 5214ms
Deduplication.No Duplicate Events In Batch ✅ 112ms
Deduplication.Different Events Have Different Uuids ✅ 108ms
Compression.Sends Gzip When Enabled ✅ 106ms
Batch Format.Uses Proper Batch Structure ✅ 106ms
Batch Format.Flush With No Events Sends Nothing ✅ 5ms
Batch Format.Multiple Events Batched Together ✅ 110ms
Error Handling.Does Not Retry On 403 ✅ 2108ms
Error Handling.Does Not Retry On 413 ✅ 2108ms
Error Handling.Retries On 408 ✅ 5212ms

Feature_Flags Tests

⚠️ 16/17 tests passed, 1 failed

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

Failures

request_payload.disable_geoip_omitted_defaults_to_false

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

@github-actions

Copy link
Copy Markdown

posthog-ruby-sync Compliance Report

Date: 2026-10-12T06:09:30.363740+00:00
Duration: 93772ms

⚠️ Some Tests Failed

45/47 tests passed, 2 failed


Capture Tests

⚠️ 29/30 tests passed, 1 failed

View Details
Test Status Duration
Format Validation.Event Has Required Fields ✅ 7ms
Format Validation.Event Has Uuid ✅ 4ms
Format Validation.Event Has Lib Properties ✅ 4ms
Format Validation.Distinct Id Is String ✅ 7ms
Format Validation.Token Is Present ✅ 5ms
Format Validation.Custom Properties Preserved ✅ 5ms
Format Validation.Event Has Timestamp ✅ 6ms
Format Validation.Non Utc Event Timestamp Is Converted To Utc ✅ 5ms
Retry Behavior.Retries On 503 ✅ 5224ms
Retry Behavior.Does Not Retry On 400 ✅ 2009ms
Retry Behavior.Does Not Retry On 401 ✅ 2008ms
Retry Behavior.Respects Retry After Header ✅ 8010ms
Retry Behavior.Implements Backoff ✅ 15364ms
Retry Behavior.Retries On 500 ✅ 5114ms
Retry Behavior.Retries On 502 ✅ 5146ms
Retry Behavior.Retries On 504 ✅ 5114ms
Retry Behavior.Max Retries Respected ✅ 15457ms
Deduplication.Generates Unique Uuids ✅ 18ms
Deduplication.Preserves Uuid On Retry ✅ 5159ms
Deduplication.Preserves Uuid And Timestamp On Retry ✅ 10255ms
Deduplication.Preserves Uuid And Timestamp On Batch Retry ✅ 5153ms
Deduplication.No Duplicate Events In Batch ✅ 13ms
Deduplication.Different Events Have Different Uuids ✅ 6ms
Compression.Sends Gzip When Enabled ✅ 6ms
Batch Format.Uses Proper Batch Structure ✅ 4ms
Batch Format.Flush With No Events Sends Nothing ✅ 2ms
Batch Format.Multiple Events Batched Together ❌ 12ms
Error Handling.Does Not Retry On 403 ✅ 2006ms
Error Handling.Does Not Retry On 413 ✅ 2009ms
Error Handling.Retries On 408 ✅ 5139ms

Failures

batch_format.multiple_events_batched_together

Expected 1 requests, got 5

Feature_Flags Tests

⚠️ 16/17 tests passed, 1 failed

View Details
Test Status Duration
Request Payload.Request With Person Properties Device Id ✅ 8ms
Request Payload.Flags Request Uses V2 Query Param ✅ 7ms
Request Payload.Flags Request Hits Flags Path Not Decide ✅ 6ms
Request Payload.Flags Request Omits Authorization Header ✅ 5ms
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 ✅ 1ms
Request Lifecycle.No Flags Request On Normal Capture ✅ 5ms
Request Lifecycle.Two Flag Calls Produce Two Remote Requests ✅ 7ms
Request Lifecycle.Mock Response Value Is Returned To Caller ✅ 5ms
Retry Behavior.Retries Flags On 502 ✅ 107ms
Retry Behavior.Retries Flags On 504 ✅ 155ms
Side Effect Events.Get Feature Flag Captures Feature Flag Called Event ✅ 5ms

Failures

request_payload.disable_geoip_omitted_defaults_to_false

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

@ioannisj ioannisj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🤖 An AI agent wrote this on behalf of @ioannisj.

This matches the flags service and posthog-node. The hash input is the same (holdout-<value>, no salt), the check is inclusive, the percentage is clamped to 0-100 and 100 skips the hash, and the gate runs after the inactive check and before release conditions. That means single, bulk and dependency evaluation all go through it. Nothing to fix here.

Two older issues are worth tracking separately. Both predate this PR and also affect rollout hashing:

  • a dependency flag is bucketed by the parent's identity even when its aggregation differs
  • the group-key rendering requirement from sdk-specs#89 isn't in Ruby yet

@ioannisj
ioannisj marked this pull request as ready for review October 12, 2026 18:26
@ioannisj
ioannisj requested a review from a team as a code owner October 12, 2026 18:26
@ioannisj
ioannisj merged commit 1643b9d into main Oct 16, 2026
25 checks passed
@ioannisj
ioannisj deleted the posthog/ruby-local-eval-holdouts branch October 16, 2026 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant