Repository navigation
feat(flags): honor experiment holdouts in local flag evaluation - #254
posthog[bot] wants to merge 1 commit into
Conversation
Local evaluation ignored `filters.holdout`, so a held-out identifier got an ordinary variant instead of `holdout-<id>`, disagreeing with the server. Resolve the holdout before release conditions, using the backend-compatible hash over `holdout-<bucketing_value>` and the flag-level bucketing identity. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 1bb30980-7c8c-4de3-8b45-5f1b1563230b
posthog-php-fork_curl Compliance ReportDate: 2026-10-05T06:08:53.379910+00:00
|
| Test | Status | Duration |
|---|---|---|
| Format Validation.Event Has Required Fields | ✅ | 36ms |
| Format Validation.Event Has Uuid | ✅ | 528ms |
| Format Validation.Event Has Lib Properties | ✅ | 530ms |
| Format Validation.Distinct Id Is String | ✅ | 531ms |
| Format Validation.Token Is Present | ✅ | 530ms |
| Format Validation.Custom Properties Preserved | ✅ | 530ms |
| Format Validation.Event Has Timestamp | ✅ | 530ms |
| Format Validation.Non Utc Event Timestamp Is Converted To Utc | ✅ | 532ms |
| Retry Behavior.Retries On 503 | ❌ | 5534ms |
| Retry Behavior.Does Not Retry On 400 | ✅ | 2533ms |
| Retry Behavior.Does Not Retry On 401 | ✅ | 2534ms |
| Retry Behavior.Respects Retry After Header | ❌ | 5535ms |
| Retry Behavior.Implements Backoff | ❌ | 15548ms |
| Retry Behavior.Retries On 500 | ❌ | 5541ms |
| Retry Behavior.Retries On 502 | ❌ | 5534ms |
| Retry Behavior.Retries On 504 | ❌ | 5538ms |
| Retry Behavior.Max Retries Respected | ❌ | 15546ms |
| Deduplication.Generates Unique Uuids | ✅ | 539ms |
| Deduplication.Preserves Uuid On Retry | ❌ | 5537ms |
| Deduplication.Preserves Uuid And Timestamp On Retry | ❌ | 10540ms |
| Deduplication.Preserves Uuid And Timestamp On Batch Retry | ❌ | 5542ms |
| Deduplication.No Duplicate Events In Batch | ✅ | 537ms |
| Deduplication.Different Events Have Different Uuids | ✅ | 533ms |
| Compression.Sends Gzip When Enabled | ✅ | 534ms |
| Batch Format.Uses Proper Batch Structure | ✅ | 531ms |
| Batch Format.Flush With No Events Sends Nothing | ✅ | 518ms |
| Batch Format.Multiple Events Batched Together | ✅ | 522ms |
| Error Handling.Does Not Retry On 403 | ✅ | 2533ms |
| Error Handling.Does Not Retry On 413 | ✅ | 2532ms |
| Error Handling.Retries On 408 | ❌ | 5538ms |
Failures
retry_behavior.retries_on_503
Expected at least 3 requests, got 1
retry_behavior.respects_retry_after_header
Expected at least 2 requests, got 1
retry_behavior.implements_backoff
Expected at least 3 requests, got 1
retry_behavior.retries_on_500
Expected at least 2 requests, got 1
retry_behavior.retries_on_502
Expected at least 2 requests, got 1
retry_behavior.retries_on_504
Expected at least 2 requests, got 1
retry_behavior.max_retries_respected
Expected 4 requests, got 1
deduplication.preserves_uuid_on_retry
Need at least 2 requests to check retry
deduplication.preserves_uuid_and_timestamp_on_retry
Expected at least 3 requests, got 1
deduplication.preserves_uuid_and_timestamp_on_batch_retry
Expected at least 2 requests, got 1
error_handling.retries_on_408
Expected at least 2 requests, got 1
Feature_Flags Tests
✅ 17/17 tests passed
View Details
| Test | Status | Duration |
|---|---|---|
| Request Payload.Request With Person Properties Device Id | ✅ | 525ms |
| Request Payload.Flags Request Uses V2 Query Param | ✅ | 521ms |
| Request Payload.Flags Request Hits Flags Path Not Decide | ✅ | 522ms |
| Request Payload.Flags Request Omits Authorization Header | ✅ | 522ms |
| Request Payload.Token In Flags Body Matches Init | ✅ | 522ms |
| Request Payload.Groups Round Trip | ✅ | 522ms |
| Request Payload.Groups Default To Empty Object | ✅ | 521ms |
| Request Payload.Disable Geoip False Propagates As Geoip Disable False | ✅ | 523ms |
| Request Payload.Disable Geoip Omitted Defaults To False | ✅ | 522ms |
| Request Payload.Flag Keys To Evaluate Contains Only Requested Key | ✅ | 521ms |
| Request Lifecycle.No Flags Request On Init Alone | ✅ | 517ms |
| Request Lifecycle.No Flags Request On Normal Capture | ✅ | 516ms |
| Request Lifecycle.Two Flag Calls Produce Two Remote Requests | ✅ | 526ms |
| Request Lifecycle.Mock Response Value Is Returned To Caller | ✅ | 523ms |
| Retry Behavior.Retries Flags On 502 | ✅ | 624ms |
| Retry Behavior.Retries Flags On 504 | ✅ | 625ms |
| Side Effect Events.Get Feature Flag Captures Feature Flag Called Event | ✅ | 531ms |
posthog-php-lib_curl Compliance ReportDate: 2026-10-05T06:09:00.756862+00:00 ✅ All Tests Passed!47/47 tests passed Capture Tests✅ 30/30 tests passed View Details
Feature_Flags Tests✅ 17/17 tests passed View Details
|
posthog-php-socket Compliance ReportDate: 2026-10-05T06:09:19.910528+00:00
|
| Test | Status | Duration |
|---|---|---|
| Format Validation.Event Has Required Fields | ✅ | 30ms |
| Format Validation.Event Has Uuid | ✅ | 524ms |
| Format Validation.Event Has Lib Properties | ✅ | 524ms |
| Format Validation.Distinct Id Is String | ✅ | 524ms |
| Format Validation.Token Is Present | ✅ | 527ms |
| Format Validation.Custom Properties Preserved | ✅ | 526ms |
| Format Validation.Event Has Timestamp | ✅ | 525ms |
| Format Validation.Non Utc Event Timestamp Is Converted To Utc | ✅ | 525ms |
| Retry Behavior.Retries On 503 | ❌ | 9233ms |
| Retry Behavior.Does Not Retry On 400 | ✅ | 2531ms |
| Retry Behavior.Does Not Retry On 401 | ✅ | 2527ms |
| Retry Behavior.Respects Retry After Header | ❌ | 9235ms |
| Retry Behavior.Implements Backoff | ❌ | 19233ms |
| Retry Behavior.Retries On 500 | ❌ | 8750ms |
| Retry Behavior.Retries On 502 | ❌ | 9236ms |
| Retry Behavior.Retries On 504 | ❌ | 9230ms |
| Retry Behavior.Max Retries Respected | ❌ | 18736ms |
| Deduplication.Generates Unique Uuids | ✅ | 46ms |
| Deduplication.Preserves Uuid On Retry | ❌ | 9234ms |
| Deduplication.Preserves Uuid And Timestamp On Retry | ❌ | 14242ms |
| Deduplication.Preserves Uuid And Timestamp On Batch Retry | ❌ | 9239ms |
| Deduplication.No Duplicate Events In Batch | ✅ | 532ms |
| Deduplication.Different Events Have Different Uuids | ✅ | 525ms |
| Compression.Sends Gzip When Enabled | ✅ | 525ms |
| Batch Format.Uses Proper Batch Structure | ✅ | 523ms |
| Batch Format.Flush With No Events Sends Nothing | ✅ | 520ms |
| Batch Format.Multiple Events Batched Together | ✅ | 514ms |
| Error Handling.Does Not Retry On 403 | ✅ | 2526ms |
| Error Handling.Does Not Retry On 413 | ✅ | 2530ms |
| Error Handling.Retries On 408 | ❌ | 5530ms |
Failures
retry_behavior.retries_on_503
Expected at least 3 requests, got 1
retry_behavior.respects_retry_after_header
Expected at least 2 requests, got 1
retry_behavior.implements_backoff
Expected at least 3 requests, got 1
retry_behavior.retries_on_500
Expected at least 2 requests, got 1
retry_behavior.retries_on_502
Expected at least 2 requests, got 1
retry_behavior.retries_on_504
Expected at least 2 requests, got 1
retry_behavior.max_retries_respected
Expected 4 requests, got 1
deduplication.preserves_uuid_on_retry
Need at least 2 requests to check retry
deduplication.preserves_uuid_and_timestamp_on_retry
Expected at least 3 requests, got 1
deduplication.preserves_uuid_and_timestamp_on_batch_retry
Expected at least 2 requests, got 1
error_handling.retries_on_408
Expected at least 2 requests, got 1
Feature_Flags Tests
✅ 17/17 tests passed
View Details
| Test | Status | Duration |
|---|---|---|
| Request Payload.Request With Person Properties Device Id | ✅ | 525ms |
| Request Payload.Flags Request Uses V2 Query Param | ✅ | 521ms |
| Request Payload.Flags Request Hits Flags Path Not Decide | ✅ | 522ms |
| Request Payload.Flags Request Omits Authorization Header | ✅ | 521ms |
| Request Payload.Token In Flags Body Matches Init | ✅ | 521ms |
| Request Payload.Groups Round Trip | ✅ | 522ms |
| Request Payload.Groups Default To Empty Object | ✅ | 522ms |
| Request Payload.Disable Geoip False Propagates As Geoip Disable False | ✅ | 522ms |
| Request Payload.Disable Geoip Omitted Defaults To False | ✅ | 521ms |
| Request Payload.Flag Keys To Evaluate Contains Only Requested Key | ✅ | 523ms |
| Request Lifecycle.No Flags Request On Init Alone | ✅ | 517ms |
| Request Lifecycle.No Flags Request On Normal Capture | ✅ | 510ms |
| Request Lifecycle.Two Flag Calls Produce Two Remote Requests | ✅ | 525ms |
| Request Lifecycle.Mock Response Value Is Returned To Caller | ✅ | 525ms |
| Retry Behavior.Retries Flags On 502 | ✅ | 625ms |
| Retry Behavior.Retries Flags On 504 | ✅ | 624ms |
| Side Effect Events.Get Feature Flag Captures Feature Flag Called Event | ✅ | 524ms |
Fixed:
The |
dustinbyrne
left a comment
There was a problem hiding this comment.
The direct holdout calculation matches the specification. Consider correcting the dependency identity mismatch and adding the public-entry regression test described inline.
AI-assisted review.
| $flagFilters = $flag["filters"] ?? []; | ||
|
|
||
| // Experiment holdouts win over every release condition, variant override, and rollout. | ||
| $holdoutVariant = FeatureFlag::matchHoldout($flagFilters["holdout"] ?? null, $distinctId); |
There was a problem hiding this comment.
[P1] Resolve a dependency's own identity before applying its holdout
Location: lib/FeatureFlag.php:963 (RIGHT).
The new holdout check treats $distinctId as the evaluated flag's bucketing identity, but recursive evaluateFlagDependency() passes the parent condition's identity without resolving the referenced flag's aggregation (lib/FeatureFlag.php:1224–1232). Direct group evaluation correctly supplies the group key (lib/Client.php:1411–1425).
A valid public call can therefore return contradictory results:
$flags = $client->evaluateFlags(
'user-1',
['company' => 'user-5'],
[],
['company' => []],
true
);With a group-aggregated checkout flag carrying holdout 727 at 20%, otherwise selecting control, and a person checkout-banner flag requiring checkout == control:
| Result | Base, inferred | Reviewed head, inferred | Expected head |
|---|---|---|---|
checkout |
control |
control |
control |
checkout-banner |
true |
false |
true |
The company user-5 hashes to approximately 0.6563813925994418, outside the holdout. The person user-1 hashes to approximately 0.17805599206573022, inside it. Only recursive evaluation returns holdout-727, incorrectly failing the banner condition. Both results are conclusive, so enabling normal remote fallback does not repair this.
Inherited-identity recursion existed at base, but 100% ordinary rollout and 100% control neutralize its old rollout/variant-bucketing defect in this example. Base correctly returns control through both paths; the new holdout branch alone changes the dependent flag. Impact is incorrect application gating and emitted flag values for partial-holdout dependencies whose own identity differs from the parent's; direct and remote-only evaluation remain unaffected.
The governing pinned specification requires shared dependency semantics and the flag-level group identity. The backend resolves the referenced flag's aggregation, and Node re-enters full evaluation for dependencies. Backend reference validation permits this person-parent/group-dependency configuration.
Consider resolving each dependency's own flag-level identity while retaining the original person/group context through recursion. When required context is unavailable, use the existing inconclusive path rather than substituting an unrelated identity. No new user-facing API is necessary.
Test sketch — NOT EXECUTED: Add this method to FeatureFlagHoldoutTest, using its existing holdoutFlag() builder. Explicit condition aggregation mirrors backend normalization; the noop capture consumer isolates flag-request assertions.
public function testGroupHoldoutDependencyUsesItsOwnIdentity(): void
{
$checkout = self::holdoutFlag([
'id' => 727,
'exclusion_percentage' => 20,
]);
$checkout['id'] = 1;
$checkout['filters']['aggregation_group_type_index'] = 0;
$checkout['filters']['groups'][0]['aggregation_group_type_index'] = 0;
$banner = [
'id' => 2,
'key' => 'checkout-banner',
'active' => true,
'filters' => [
'groups' => [[
'aggregation_group_type_index' => null,
'rollout_percentage' => 100,
'properties' => [[
'key' => 'checkout',
'type' => 'flag',
'operator' => 'flag_evaluates_to',
'value' => 'control',
'dependency_chain' => ['checkout'],
]],
]],
],
];
$http = new MockedHttpClient(
'app.posthog.com',
flagEndpointResponse: [
'flags' => [$checkout, $banner],
'group_type_mapping' => ['0' => 'company'],
]
);
$client = new Client(
'test-project-key',
['consumer' => 'noop'],
$http,
secretKey: 'test-secret-key'
);
foreach ([true, false] as $onlyEvaluateLocally) {
$http->calls = [];
$flags = $client->evaluateFlags(
'user-1',
['company' => 'user-5'],
[],
['company' => []],
$onlyEvaluateLocally
);
self::assertSame('control', $flags->getFlag('checkout'));
self::assertTrue($flags->getFlag('checkout-banner'));
self::assertSame([], $http->calls);
}
}Expected: both values match the assertions without remote evaluation in either mode. Reviewed-source implication: checkout is control, but banner is false, failing the second assertion. The equivalent fixture and builder are expected to pass against base and after correction. These outcomes are inferred, not executed.
Proposed command, not run:
./vendor/bin/phpunit --bootstrap vendor/autoload.php --configuration phpunit.xml --filter testGroupHoldoutDependencyUsesItsOwnIdentityAlso cover the opposite membership direction: person user-5, company user-1, dependency expecting holdout-727, with checkout holdout-727 and banner true. This companion tests newly supported holdouts; unlike the control fixture, it is not a base-passing regression.
💡 Motivation and Context
Brings posthog-php into compliance with the Local Feature Flag Evaluator contract in PostHog/sdk-specs — specifically the requirements Experiment holdouts precede release conditions and Holdout membership uses backend-compatible bucketing.
The SDK compliance matrix flagged the gap (
compliance/posthog-php.md, Local Feature Flag Evaluator — 🟡 Partial):Today
filters.holdoutis dropped on the floor during local evaluation (grep -rn holdout lib/returned nothing), so a user the backend puts in an experiment holdout gets an ordinary variant locally. That is a silent correctness divergence between local and remote evaluation for every experiment that uses a holdout.What changed
FeatureFlag::matchFeatureFlagProperties()resolvesfilters.holdoutbefore iterating release conditions, returningholdout-<id>when the bucketing identity is held out. Because both the single-flag/bulk path (Client::computeFlagLocally()) and the dependency path (FeatureFlag::evaluateFlagDependency()) checkactiveand then call this method, all three evaluation paths share the inactive check and the holdout precedence: an inactive flag still resolves tofalse, and a dependent flag comparing against"holdout-727"matches.FeatureFlag::holdoutHash()implements the backend hash: SHA-1 overholdout-<bucketing_value>with no separator or salt, first 15 hex digits over0xfffffffffffffff. Neither the flag key nor the holdout id participates, so the ordinary dot-separated flag hash is deliberately not reused.hash <= percentage / 100, withexclusion_percentageclamped to 0–100 as a float (no truncation) and a clamped 100 short-circuiting without computing a hash.idorexclusion_percentageis skipped and ordinary evaluation continues unchanged.evaluateFlagDependency()passes the parent flag's distinct id down to the dependency, so a group-aggregated dependency with a partial holdout gets bucketed by the person instead of its group key. Rollout hashing for those dependencies already works this way onmain, and this PR doesn't change that.Backwards compatibility
Additive. Flags without a (complete)
filters.holdoutevaluate exactly as before, and no public signature changes —composer api:checkreports the public API snapshot is unchanged. Values change only for flags that actually carry a holdout, where the current locally-evaluated value is wrong and disagrees with the server.💚 How did you test it?
New
test/FeatureFlagHoldoutTest.phpcovers each acceptance scenario from the spec: holdout winning over an unavailable targeting property, rollout 0 and a variant override; a synthetic variant absent fromfilters.multivariate.variants; inactive flag stayingfalse; no-holdout and incomplete-holdout preserving ordinary assignment; dependency evaluation comparing against the holdout string; the spec's reference hashes (user-1→ ~0.17805599206573022 held out at 20%,user-5→ ~0.6563813925994418 not); fractional percentages not truncated; inclusive/clamped boundaries at 0, -10, 100 and 150; and the group key deciding membership for a group-aggregated flag.No manual testing against a live PostHog project.
Follow-up work
Device identity determines membership for device-bucketed flags) is untested here because posthog-php has no device-id bucketing concept; it would land with that feature.evaluateFlagDependency()resolve each dependency's own bucketing identity, the way posthog-js does by re-enteringcomputeFlagValueLocally(). That would fix rollout, variant and holdout bucketing for group-aggregated dependencies together.📝 Checklist
If releasing new changes
pnpm changeto generate a change intent file (added.changeset/local-flag-holdouts.mdby hand, matching the existing format)🤖 Agent context
Autonomy: Fully autonomous
Opened by a scheduled PostHog agent run that reads the SDK compliance matrices in
PostHog/sdk-specsand implements one backwards-compatible gap per run. Candidate selection skipped gaps with a Breaking verdict, gaps on deprecated methods, and gaps already covered by an open PR on this repo. Holdout support was chosen over the other eligible PHP candidates (an additivegetAllFlagsAndPayloads(), payload-decode warning logging, and the flag-request backoff constant) because it is a user-visible correctness divergence from the server rather than an ergonomics or diagnostics gap.Implementation note: the holdout check was placed in
matchFeatureFlagProperties()rather thanClient::computeFlagLocally()as the matrix suggested, so the dependency-evaluation path gets the same semantics without duplicating the logic.Created with PostHog Desktop
🤖 Generated with Claude Code