Skip to content

fix: reset flag-called tracking when definitions reload - #267

Open
marandaneto wants to merge 1 commit into
mainfrom
fix/triage-ruby-189-20260905
Open

fix: reset flag-called tracking when definitions reload#267
marandaneto wants to merge 1 commit into
mainfrom
fix/triage-ruby-189-20260905

Conversation

@marandaneto

Copy link
Copy Markdown
Member

💡 Motivation and Context

Related to #189.

After local flag definitions reload, the client still remembers earlier $feature_flag_called events and suppresses the next access. Clear the existing tracker when definitions are applied or when a quota response discards loaded definitions. Manual reloads, background polling, and external-cache refreshes use the same callback.

The reset uses the existing mutex. Repeated reads between reloads remain deduplicated. HTTP 304 responses, failed requests, and responses without definitions leave tracking unchanged. This does not change the behavior of evaluations already in flight during a refresh.

💚 How did you test it?

  • Added 13 regression examples that inspect queued events, covering manual and background reloads, external-cache refreshes, empty definitions, quota discard, shared snapshot tracking, failure preservation, and concurrent duplicate reads.
  • Reproduction against the unchanged base failed 8 of those 13 examples. All 13 pass with this fix. Ten seeded regression runs also passed during implementation.
  • Re-ran bundle exec rspec: 690 examples, 0 failures.
  • Re-ran bundle exec rubocop (85 files), bundle exec rake public_api:check, and the core gem build. All passed. The gem build retains its existing missing-author warning.
  • Exact-HEAD branch autoreview against origin/main passed for 4f4b1de269fc651c7a79a1ba0f09d4ed0c90bf96 with no findings.

Validation used Ruby 4.0.6 and Bundler 4.0.13 with frozen dependencies. The CI Ruby 3.2/3.3/3.4 matrix was not run locally. Tests use WebMock rather than a live PostHog service.

📝 Checklist

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

If releasing new changes

  • Ran pnpm changeset to generate a changeset file

A patch changeset for posthog-ruby was added directly and preserved from the implementation. The generator was not run.

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

Pi agents implemented and independently reviewed this user-directed fix. Tools used were Git, Bundler/RSpec, RuboCop, Rake, RubyGems, GitHub CLI, and the isolated Pi autoreview helper. Session references: implementation 3efed453-bba9-4ebc-853b-b7247693a8d8, publication 510ebebc-a750-45a5-88ce-92821ccd3256 (local sessions, no public link).

The fix keeps the existing bounded tracker and mutex rather than adding generation tracking or changing the public client API. Human review is required before merging.

@marandaneto marandaneto self-assigned this Sep 5, 2026
@marandaneto
marandaneto marked this pull request as ready for review September 5, 2026 17:26
@marandaneto
marandaneto requested a review from a team as a code owner September 5, 2026 17:26
@marandaneto
marandaneto requested a review from a team September 5, 2026 17:26
@posthog-project-board-bot posthog-project-board-bot Bot moved this to In Review in Feature Flags Sep 5, 2026
@marandaneto

Copy link
Copy Markdown
Member Author

maybe a better approach would be to check the results and the cached flags and only clear the values that have changed, wdyt? or is it ok to emit again? i recall those events being "expensive" but later this was improved during ingestion

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown

posthog-ruby Compliance Report

Date: 2026-09-05 17:27:50 UTC
Duration: 98328ms

✅ All Tests Passed!

46/46 tests passed


Capture Tests

29/29 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 109ms
Format Validation.Distinct Id Is String 107ms
Format Validation.Token Is Present 108ms
Format Validation.Custom Properties Preserved 107ms
Format Validation.Event Has Timestamp 107ms
Retry Behavior.Retries On 503 5314ms
Retry Behavior.Does Not Retry On 400 2109ms
Retry Behavior.Does Not Retry On 401 2109ms
Retry Behavior.Respects Retry After Header 8116ms
Retry Behavior.Implements Backoff 15608ms
Retry Behavior.Retries On 500 5210ms
Retry Behavior.Retries On 502 5211ms
Retry Behavior.Retries On 504 5212ms
Retry Behavior.Max Retries Respected 15522ms
Deduplication.Generates Unique Uuids 112ms
Deduplication.Preserves Uuid On Retry 5208ms
Deduplication.Preserves Uuid And Timestamp On Retry 10418ms
Deduplication.Preserves Uuid And Timestamp On Batch Retry 5216ms
Deduplication.No Duplicate Events In Batch 112ms
Deduplication.Different Events Have Different Uuids 107ms
Compression.Sends Gzip When Enabled 105ms
Batch Format.Uses Proper Batch Structure 105ms
Batch Format.Flush With No Events Sends Nothing 4ms
Batch Format.Multiple Events Batched Together 111ms
Error Handling.Does Not Retry On 403 2107ms
Error Handling.Does Not Retry On 413 2109ms
Error Handling.Retries On 408 5207ms

Feature_Flags Tests

17/17 tests passed

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 107ms
Request Payload.Token In Flags Body Matches Init 107ms
Request Payload.Groups Round Trip 108ms
Request Payload.Groups Default To Empty Object 106ms
Request Payload.Disable Geoip False Propagates As Geoip Disable False 106ms
Request Payload.Disable Geoip Omitted Defaults To False 105ms
Request Payload.Flag Keys To Evaluate Contains Only Requested Key 106ms
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 109ms
Request Lifecycle.Mock Response Value Is Returned To Caller 106ms
Retry Behavior.Retries Flags On 502 246ms
Retry Behavior.Retries Flags On 504 208ms
Side Effect Events.Get Feature Flag Captures Feature Flag Called Event 107ms

@greptile-apps

greptile-apps Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor
Prompt To Fix All With AI
### Issue 1
lib/posthog/feature_flags.rb:1288
**Refresh Can Duplicate Events**

During a background refresh, the new definitions become visible before this callback clears the deduplication tracker. A concurrent evaluation can use those new definitions and record its `$feature_flag_called` event, but the callback then erases that record. A later read emits the event again even though no additional reload occurred. The tracker reset must be ordered with definition publication, such as by associating entries with a definition generation.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Reviews (1): Last reviewed commit: "fix: reset flag-called tracking when def..." | Re-trigger Greptile

logger.debug "Loaded #{@feature_flags.length} feature flags and #{@cohorts.length} cohorts"
@flag_definitions_loaded_at.value = (Time.now.to_f * 1000).to_i
@loaded_flags_successfully_once.make_true if @loaded_flags_successfully_once.false?
@on_flag_definitions_updated&.call

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.

P1 Refresh Can Duplicate Events

During a background refresh, the new definitions become visible before this callback clears the deduplication tracker. A concurrent evaluation can use those new definitions and record its $feature_flag_called event, but the callback then erases that record. A later read emits the event again even though no additional reload occurred. The tracker reset must be ordered with definition publication, such as by associating entries with a definition generation.

Knowledge Base Used:

Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/posthog/feature_flags.rb
Line: 1288

Comment:
**Refresh Can Duplicate Events**

During a background refresh, the new definitions become visible before this callback clears the deduplication tracker. A concurrent evaluation can use those new definitions and record its `$feature_flag_called` event, but the callback then erases that record. A later read emits the event again even though no additional reload occurred. The tracker reset must be ordered with definition publication, such as by associating entries with a definition generation.

**Knowledge Base Used:**
- [Feature flag evaluation](https://app.greptile.com/posthog-org-19734/-/custom-context/knowledge-base/posthog/posthog-ruby/-/docs/feature-flags.md)
- [Feature flag definition cache](https://app.greptile.com/posthog-org-19734/-/custom-context/knowledge-base/posthog/posthog-ruby/-/docs/feature-flag-definition-cache.md)

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In Review

Development

Successfully merging this pull request may close these issues.

1 participant