Repository navigation
fix(flags): track snapshot access with atomics instead of a process - #235
Conversation
There was a problem hiding this comment.
PostHog Review · Standard · reviewed 3327ee3: 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.
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.
posthog-elixir Compliance ReportDate: 2026-10-30T08:17:39.448834+00:00
|
| Test | Status | Duration |
|---|---|---|
| Format Validation.Event Has Required Fields | ✅ | 611ms |
| Format Validation.Event Has Uuid | ✅ | 611ms |
| Format Validation.Event Has Lib Properties | ✅ | 611ms |
| Format Validation.Distinct Id Is String | ✅ | 610ms |
| Format Validation.Token Is Present | ✅ | 612ms |
| Format Validation.Custom Properties Preserved | ✅ | 612ms |
| Format Validation.Event Has Timestamp | ✅ | 612ms |
| Format Validation.Non Utc Event Timestamp Is Converted To Utc | ❌ | 612ms |
| Retry Behavior.Retries On 503 | ✅ | 5618ms |
| Retry Behavior.Does Not Retry On 400 | ✅ | 2614ms |
| Retry Behavior.Does Not Retry On 401 | ✅ | 2613ms |
| Retry Behavior.Respects Retry After Header | ✅ | 5615ms |
| Retry Behavior.Implements Backoff | ✅ | 15616ms |
| Retry Behavior.Retries On 500 | ✅ | 5617ms |
| Retry Behavior.Retries On 502 | ✅ | 5616ms |
| Retry Behavior.Retries On 504 | ✅ | 5617ms |
| Retry Behavior.Max Retries Respected | ✅ | 15626ms |
| Deduplication.Generates Unique Uuids | ✅ | 623ms |
| Deduplication.Preserves Uuid On Retry | ✅ | 5617ms |
| Deduplication.Preserves Uuid And Timestamp On Retry | ✅ | 10618ms |
| Deduplication.Preserves Uuid And Timestamp On Batch Retry | ✅ | 5616ms |
| Deduplication.No Duplicate Events In Batch | ✅ | 617ms |
| Deduplication.Different Events Have Different Uuids | ✅ | 615ms |
| Compression.Sends Gzip When Enabled | ✅ | 611ms |
| Batch Format.Uses Proper Batch Structure | ✅ | 611ms |
| Batch Format.Flush With No Events Sends Nothing | ✅ | 608ms |
| Batch Format.Multiple Events Batched Together | ✅ | 615ms |
| Error Handling.Does Not Retry On 403 | ✅ | 2613ms |
| Error Handling.Does Not Retry On 413 | ✅ | 2613ms |
| Error Handling.Retries On 408 | ✅ | 5616ms |
Failures
format_validation.non_utc_event_timestamp_is_converted_to_utc
Event 0 field 'timestamp' instant '2026-10-30T08:15:55.353576Z' != expected '2025-01-02T03:04:05Z'
Feature_Flags Tests
View Details
| Test | Status | Duration |
|---|---|---|
| Request Payload.Request With Person Properties Device Id | ✅ | 9ms |
| Request Payload.Flags Request Uses V2 Query Param | ✅ | 9ms |
| Request Payload.Flags Request Hits Flags Path Not Decide | ✅ | 8ms |
| Request Payload.Flags Request Omits Authorization Header | ✅ | 8ms |
| Request Payload.Token In Flags Body Matches Init | ✅ | 8ms |
| Request Payload.Groups Round Trip | ✅ | 7ms |
| Request Payload.Groups Default To Empty Object | ❌ | 8ms |
| Request Payload.Disable Geoip False Propagates As Geoip Disable False | ✅ | 8ms |
| Request Payload.Disable Geoip Omitted Defaults To False | ❌ | 7ms |
| Request Payload.Flag Keys To Evaluate Contains Only Requested Key | ✅ | 8ms |
| Request Lifecycle.No Flags Request On Init Alone | ✅ | 3ms |
| Request Lifecycle.No Flags Request On Normal Capture | ✅ | 609ms |
| Request Lifecycle.Two Flag Calls Produce Two Remote Requests | ✅ | 11ms |
| Request Lifecycle.Mock Response Value Is Returned To Caller | ❌ | 7ms |
| Retry Behavior.Retries Flags On 502 | ❌ | 311ms |
| Retry Behavior.Retries Flags On 504 | ❌ | 311ms |
| Side Effect Events.Get Feature Flag Captures Feature Flag Called Event | ❌ | 610ms |
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
ioannisj
left a comment
There was a problem hiding this comment.
🤖 An AI agent wrote this on behalf of @ioannisj.
Nothing blocking from me. I went through the :atomics index mapping and the nil path for empty snapshots, the accessed_pid field staying nil, the accessed/1 change against the evaluate-flags spec and onlyAccessed in posthog-js, and the OpenFeature provider no longer stopping an agent. The one thing I'd like your take on is whether the changeset should be minor rather than patch, since two runtime behaviours change for existing callers.
💡 Motivation and Context
Follow-up to #234. That PR stops the snapshot's access-tracking Agent when the calling process exits. Two leaks remained:
evaluate_flags/2,Evaluations.only/2, andEvaluations.only_accessed/1call starts an Agent linked to the caller. The empty snapshot returned when nodistinct_idis available starts one too. The Agent only stops when the caller exits, not when the snapshot goes out of scope. In a GenServer, LiveView, channel, or queue consumer, these Agents pile up for the life of that process. In a local repro, 10,000evaluate_flags/2calls from one process left 10,000 extra processes (about 32 MB) alive.Agent.stop/1on every evaluation. Because the Agent is linked, a caller withtrap_exitenabled gets an{:EXIT, pid, :normal}message each time. In the repro, 1,000 evaluations left 1,000 unread messages. A GenServer without a matchinghandle_infologs an "unexpected message" error on every evaluation instead.Snapshots now track access with
:atomics, one counter per flag key in the snapshot. The atomics array is freed when the snapshot is garbage-collected, so no process is started and nothing needs to be stopped. The OpenFeature cleanup is removed because nothing is left to clean up. As a side effect, a snapshot can now be used from any process, including after the process that created it has exited. Before, that raised an exit error.Behaviour changes
This ships as a patch because it fixes a leak. The changeset describes two visible changes:
accessed_pidstruct field is kept for compatibility but is deprecated and alwaysnil. It was internal tracking state. Code that reads or pattern-matches it still compiles. Code that passes it toAgent.stop/1, as the OpenFeature provider used to, must remove that call, because there is no process to stop anymore. Tracking state now lives in a new opaqueaccessedfield.Evaluations.accessed/1now only returns accessed keys that are present in the snapshot. Keys read but missing from the snapshot are no longer listed, because the counters are created from the snapshot's own flags.only_accessed/1already ignored those keys, so its result is unchanged. Reading a missing key still sends$feature_flag_calledwith theflag_missingerror.GitHub code search found no public Elixir code using
accessed_pid,Evaluations.accessed, oronly_accessedoutside this repo.The sdk-specs
evaluate-flagsspec defines the accessed-key filter only in terms of flags present in the snapshot, so this matches the spec. The spec does not cover how access is stored or how long it lives.💚 How did you test it?
accessed/1leaves out missing keys. Each of these failed onmainbefore the change.accessed_pidis still present andnilon every kind of snapshot. Without the field, it fails to compile.mix testpasses except 7PostHog.Integrations.PlugTesttests. They also fail onmainon my machine because port 8001 is already in use.mix format --check-formatted,mix credo --strict, andmix posthog.public_api --checkpass.mainto 0 and 0 on this branch.📝 Checklist
If releasing new changes
sampo addto generate a changeset file🤖 Agent context
Autonomy: Human-driven (agent-assisted)
accessed/1. That needs a shared mutable store such as ETS or a process, which brings back the cleanup problem, so we accepted the change.accessed_pidstays as a deprecatednilfield instead of being removed. The behaviour changes are described in the changeset.