Skip to content

fix: Treat failed feature flag records as unavailable - #970

Merged
marandaneto merged 3 commits into
mainfrom
fix/spec211-failed-flags
Nov 13, 2026
Merged

marandaneto merged 3 commits into
mainfrom
fix/spec211-failed-flags

Conversation

@marandaneto

@marandaneto marandaneto commented Nov 12, 2026 •

Copy link
Copy Markdown
Member

💡 Motivation and Context

Detailed /flags records with failed: true must represent unavailable evaluations, not disabled values or placeholder variants/payloads. Implements the failed-record contract in PostHog/sdk-specs#211 while keeping existing API and refresh policies:

  • Exclude failed values and payloads from client/server direct and bulk reads, server snapshots and automatic capture enrichment, independently of the response-wide error bit. Keep healthy false values and legacy responses valid.
  • Preserve successful same-context client values, payloads and their diagnostic source on response-wide errored refreshes. Complete nonerrored refreshes still replace state after filtering, including all-failed responses.
  • Sanitize persisted failed placeholders, preserve available diagnostic errors across reopen, and let later legacy successes replace stale detailed failure attribution.
  • Preserve successful same-context local value/payload pairs during server remote fallback. Keep existing healthy remote and absent-key precedence on deprecated bulk/capture paths.
  • On cached server bulk/enrichment hits, use only already-available definitions rather than synchronously retry an unavailable definition load. Cold-context loading, definition polling/manual refresh and changed-definition response-cache replacement remain unchanged.
  • Report failed direct reads as evaluation_failed, not flag_missing; preserve response-wide errors independently and use the existing empty-string missing-response sentinel instead of a caller default. Event suppression/deduplication remain unchanged.
  • Safely skip client metadata-dependent event enrichment when a retained failed record has Gson-null/missing metadata, including after JSON-cache reopen. Retain failure identity, request/time provenance and any reason; do not drop the record/response or manufacture IDs.

No new public API, dependency, DTO nullability or stateless/server deserialization-hardening change. Snapshot inclusion, unresolved-read and event policies are unchanged except filtering failed remote values and retaining existing successful local pairs. No intentional divergence from the touched sdk-specs #211 behavior.

The existing changeset requests patch releases for core, Android, server, Compose surveys and server OpenFeature. No redundant release intent or manual version bumps were added.

💚 How did you test it?

At final reviewed HEAD 1a7a9e1cfa357b142c269efa1e83dc79d76de37a:

  • Existing focused owner/client/capture baselines passed before the review fixes.
  • Cache regression reproduced before production edits: actual /local_evaluation HTTP count became 2 instead of 1 on the second cached bulk read and second appendFeatureFlags capture. The same two tests now pass, including cold-context retry, original failure provenance, healthy false/payload controls and manual recovery with existing cache replacement.
  • Actual Gson null/missing-metadata regressions independently reproduced the client metadata.id NPE for first scalar/structured tracked reads, live and after JSON-cache reopen. All four tests now pass across response-wide error-bit true/false/absent, optional reason, defaults, event suppression/dedupe, healthy false metadata enrichment and healthy variant/payload controls. Valid failed metadata remains asserted.
  • Full core suite: 1,100 passed, zero failures/errors/skips. Full server suite: 670 passed, zero failures/errors/skips (./gradlew :posthog:test :posthog-server:test). An initial 120-second tool timeout was terminated; an authorized unchanged-source rerun with a longer local timeout passed in 3m52s. This was not counted as a failing test or reproduction.
  • Real Android SharedPreferences Robolectric suite: 29 passed, zero failures/errors/skips (./gradlew :posthog-android:testReleaseUnitTest --tests 'com.posthog.android.internal.PostHogSharedPreferencesTests').
  • make checkFormat, core/server apiCheck, core lint, core/server animalsnifferMain and git diff --check pass. Focused regressions and healthy-precedence/local-pair/valid-metadata controls were rerun after formatting-only edits and pass.
  • Full-branch test-audit Authoring and diff-scoped deslop completed; no unresolved blockers. Mandatory autoreview --mode branch --base origin/main is clean on the exact final HEAD above, base d753954d53e62b715d810c2e91d02b11534edf8c (TruffleHog clean; no accepted/actionable findings). All three branch commits have locally verified good signatures.

Local Gradle validation used the installed Java 21 via command-local JAVA_HOME, JAVA_TOOL_OPTIONS=-Dnet.bytebuddy.experimental=true, and the existing Android SDK. No dependency installs, toolchain changes or lockfile churn. Full Android suites, Compose, OpenFeature, device/instrumentation, all-module build/release checks, remote CI and the shared cross-SDK harness were not run in this follow-up. Older caches cannot recover response-wide errors that older SDKs never stored. Prior branch red evidence was not regenerated here; the two review concerns have newly executed red/green evidence.

📝 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 API or documentation changes are needed.
  • No breaking change or entry added to the changelog.

If releasing new changes

  • The existing five-package patch changeset is retained; no additional intent was needed for this review follow-up.

🤖 Agent context

Autonomy: Human-driven (agent-assisted). Human DRI: @marandaneto. Pi implemented the approved fixes and ran the tests, source/test audit, and isolated autoreview listed above. Human review is required before merge. No shareable session link is available.

@marandaneto marandaneto self-assigned this Nov 12, 2026
@github-actions

github-actions Bot commented Nov 12, 2026 •

Copy link
Copy Markdown
Contributor

posthog-android Compliance Report

Date: 2026-11-12 13:49:28 UTC
Duration: 118394ms

✅ All Tests Passed!

46/46 tests passed


Capture Tests

✅ 29/29 tests passed

View Details
Test Status Duration
Format Validation.Event Has Required Fields ✅ 399ms
Format Validation.Event Has Uuid ✅ 28ms
Format Validation.Event Has Lib Properties ✅ 25ms
Format Validation.Distinct Id Is String ✅ 26ms
Format Validation.Token Is Present ✅ 25ms
Format Validation.Custom Properties Preserved ✅ 25ms
Format Validation.Event Has Timestamp ✅ 25ms
Retry Behavior.Retries On 503 ✅ 7028ms
Retry Behavior.Does Not Retry On 400 ✅ 4025ms
Retry Behavior.Does Not Retry On 401 ✅ 4024ms
Retry Behavior.Respects Retry After Header ✅ 7026ms
Retry Behavior.Implements Backoff ✅ 17033ms
Retry Behavior.Retries On 500 ✅ 7016ms
Retry Behavior.Retries On 502 ✅ 7017ms
Retry Behavior.Retries On 504 ✅ 7020ms
Retry Behavior.Max Retries Respected ✅ 17029ms
Deduplication.Generates Unique Uuids ✅ 41ms
Deduplication.Preserves Uuid On Retry ✅ 7016ms
Deduplication.Preserves Uuid And Timestamp On Retry ✅ 12030ms
Deduplication.Preserves Uuid And Timestamp On Batch Retry ✅ 7019ms
Deduplication.No Duplicate Events In Batch ✅ 34ms
Deduplication.Different Events Have Different Uuids ✅ 25ms
Compression.Sends Gzip When Enabled ✅ 17ms
Batch Format.Uses Proper Batch Structure ✅ 19ms
Batch Format.Flush With No Events Sends Nothing ✅ 16ms
Batch Format.Multiple Events Batched Together ✅ 31ms
Error Handling.Does Not Retry On 403 ✅ 4021ms
Error Handling.Does Not Retry On 413 ✅ 4020ms
Error Handling.Retries On 408 ✅ 5035ms

Feature_Flags Tests

✅ 17/17 tests passed

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

@marandaneto
marandaneto marked this pull request as ready for review November 12, 2026 12:13
@marandaneto
marandaneto requested a review from a team as a code owner November 12, 2026 12:13
@veria-ai

veria-ai Bot commented Nov 12, 2026 •

Copy link
Copy Markdown

PR overview

All previously flagged issues have been addressed. No open security concerns remain on this pull request.

Fixed/addressed: 1 · PR risk: 0/10

Reviews

@posthog posthog Bot 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.

PostHog Review · Standard · reviewed a63ed08: 1 Should fix.

See this review and its findings in PostHog Review. Comment @posthog review --deep for a Deep review. A Deep review takes longer and looks deeper than a Standard review.

@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.

No findings from me. I went through the failed-record handling on the client and server read paths and the $feature_flag_called properties.

@marandaneto
marandaneto merged commit b3afe48 into main Nov 13, 2026
17 of 18 checks passed
@marandaneto
marandaneto deleted the fix/spec211-failed-flags branch November 13, 2026 13:49
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.

2 participants