Skip to content

fix(flags): sort evaluation contexts in the Rust cache builder - #92164

Merged
trunk-io[bot] merged 1 commit into
masterfrom
fix/flags-evaluation-contexts-ordering
Sep 2, 2026
Merged

trunk-io[bot] merged 1 commit into
masterfrom
fix/flags-evaluation-contexts-ordering

Conversation

@JamesPatrickGill

Copy link
Copy Markdown
Member

Problem

  • A flag with two or more evaluation contexts gets a different array from each of the two cache builders, so the shadow comparison between them reports an evaluation_contexts field mismatch.
  • Python aggregates the names with ArrayAgg(..., distinct=True), which Postgres renders as array_agg(DISTINCT ...). That sorts them.
  • Rust used a bare ARRAY_AGG with neither DISTINCT nor ORDER BY. Postgres guarantees no row order without one, so Rust returned join order, and could return a different order between runs.

Changes

  • The Rust builder emits ARRAY_AGG(DISTINCT ctx.name), so both builders produce the same sorted array (rust/feature-flags/src/flags/feature_flag_list.rs).
  • Rust changed, not Python. Python is the incumbent writer, so every live cache entry already holds Python's ordering. Moving Python to Rust's order would rewrite every team's cache instead.
  • DISTINCT rather than ORDER BY ctx.name, so both sides run the identical construct and match by construction. The explicit sort looks stricter, but it would only go on the Rust side: four Python sites use the bare ArrayAgg(..., distinct=True). If Postgres ever stopped sorting for aggregate DISTINCT, identical constructs break together and both tests go red, whereas a one-sided ORDER BY would leave Rust sorted and Python not.
  • No user-visible change. The Rust builder does not write the cache that serves flags.
  • The rest of the diff is comments and test assertions.

How did you test this code?

  • Rust: test_fetch_flags_with_evaluation_tags_from_pg now asserts the exact array. It already inserted three contexts out of alphabetical order, but asserted membership with contains, so no ordering bug could fail it. On the old aggregate it fails with left ["docs-page", "marketing-site", "app"] against right ["app", "docs-page", "marketing-site"].
  • Python: test_verify_cache_detects_evaluation_context_rename moves from one evaluation context to two, created out of alphabetical order. The single-element array it used could not vary in order. This case passes before and after, because the Python aggregate is unchanged. It catches a later removal of distinct=True, which would split the two builders again.
  • Not checked: 16 tests in cargo test -p feature-flags fail locally before this change as well. They need a GeoIP database file and a cohort_membership table the local database does not have. None reach this code path.
  • No manual testing against a running app. The change is a background cache builder with no UI surface.

Automatic notifications

  • Publish to changelog?

Docs update

None. No documented behavior changes.

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

Claude Code worked from a written brief that fixed the scope, the direction of the fix, and the build and test commands.

Skills invoked: /writing-tests, /writing-code-comments, /writing-pr-descriptions, /review-code --fix.

Two decisions are worth a reviewer's attention. The brief called for extending the Python test at test_flags_cache.py, but that test cannot fail before the change, because it never runs the Rust query. The failing test is therefore the Rust one, and the Python case is widened only to pin the reference ordering. Separately, the review pass proposed an explicit ORDER BY inside the aggregate; the Changes section records why that was declined.

The review pass also corrected a comment that named one Python builder when there are two. flags_hypercache registers both a load_fn and a batch_load_fn, and the single-team path in products/feature_flags/backend/models/feature_flag.py carries a second copy of the same ArrayAgg. That copy is the one the cache verifier compares against.

Follow-up not taken here: the same ArrayAgg(..., distinct=True) is hand-rolled at four Python sites. A shared helper would give the Rust comment one symbol to point at, but it touches files this change does not otherwise edit.

The Rust builder aggregated a flag's evaluation context names with a bare
ARRAY_AGG, so Postgres returned them in join order. The Python builder uses
ArrayAgg(distinct=True), which sorts and deduplicates. A flag with two or more
contexts therefore got a different array from each builder, and the Rust build
could differ from itself between runs.

Add DISTINCT to the Rust aggregate so both sides emit array_agg(DISTINCT ...)
and produce the same array. Python is the incumbent writer, so its ordering is
the one every live cache entry already holds.

Widen the two tests that could not catch this. The Rust one asserted membership
rather than order. The Python one used a single-element array, where ordering
cannot vary.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@JamesPatrickGill JamesPatrickGill self-assigned this Sep 1, 2026
@trunk-io

trunk-io Bot commented Sep 1, 2026 •

Copy link
Copy Markdown

😎 Merged successfully - details.

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

🤖 CI report

⚠️ Trunk lane — backend Python lane

This PR is assigned to the backend Python lane. It runs backend Python tests and may merge in parallel with PRs in other lanes.

@JamesPatrickGill
JamesPatrickGill marked this pull request as ready for review September 1, 2026 08:53
@pr-assigner-resolver-posthog
pr-assigner-resolver-posthog Bot requested a review from a team September 1, 2026 08:54
@posthog-project-board-bot posthog-project-board-bot Bot moved this to In Review in Feature Flags Sep 1, 2026
@greptile-apps

greptile-apps Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Reviews (1): Last reviewed commit: "fix(flags): sort evaluation contexts in ..." | Re-trigger Greptile

@trunk-io

trunk-io Bot commented Sep 1, 2026 •

Copy link
Copy Markdown

Static Badge   Static Badge   Static Badge

View Full Report ↗︎ ⋅ Docs

@posthog

posthog Bot commented Sep 1, 2026 •

Copy link
Copy Markdown
Contributor

🦔 PostHog Review reviewed this pull request

Nothing worth raising this time, so here's a calming picture instead:

A panda relaxing and waving

@posthog posthog Bot removed the reviewhog label Sep 1, 2026
@JamesPatrickGill JamesPatrickGill added the stamphog Request AI approval (no full review) label Sep 2, 2026

@stamphog stamphog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Approved.

Small, contained SQL fix aligning Rust's array aggregation with Python's existing sorted behavior in a cache-verification path with no production write path; author is on the owning team and tests were updated to pin the new behavior.

  • Author wrote 0% of the modified lines and has 8 merged PRs in these paths (familiarity MODERATE).
  • 👍 on the PR from greptile-apps[bot].
Gate mechanics and policy version
Gate Result
prerequisites ✓ all clear
deny-list ✓ no deny categories matched
size ✓ 17L, 1F substantive, 30L/2F incl. docs/generated/snapshots — within ceiling
tier ✓ T1-agent / T1b-small (30L, 2F, two-areas, fix)
stamphog 2.0.0b4 .stamphog/policy.yml @ fbfdf40 · reviewed head fbfdf40

@github-project-automation github-project-automation Bot moved this from In Review to Approved in Feature Flags Sep 2, 2026
@JamesPatrickGill

Copy link
Copy Markdown
Member Author

/trunk merge

@trunk-io
trunk-io Bot merged commit 4ed30c6 into master Sep 2, 2026
392 of 398 checks passed
@trunk-io
trunk-io Bot deleted the fix/flags-evaluation-contexts-ordering branch September 2, 2026 08:45
@github-project-automation github-project-automation Bot moved this from Approved to Done in Feature Flags Sep 2, 2026
@deployment-status-posthog

deployment-status-posthog Bot commented Sep 2, 2026 •

Copy link
Copy Markdown

Deploy status

Environment Status Deployed At Workflow
dev ✅ Deployed 2026-09-02 09:05 UTC Run
prod-us ✅ Deployed 2026-09-02 09:15 UTC Run
prod-eu ✅ Deployed 2026-09-02 09:16 UTC Run

dmarticus added a commit that referenced this pull request Sep 4, 2026
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
dmarticus added a commit that referenced this pull request Sep 4, 2026
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
dmarticus added a commit that referenced this pull request Sep 4, 2026
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
JamesPatrickGill added a commit that referenced this pull request Sep 10, 2026
…laced

A flag whose stored `filters` do not parse into `FlagFilters` is dropped by the
Rust builder and kept by Python. #92163 landed the decision to count that drop
rather than stop it, and added
`test_flag_with_malformed_filters_is_dropped_and_counted` beside the builder to
cover the counters. This file asserted the opposite, so it could only ever fail.
The module doc now points at the test that owns the case, so the assertion does
not come back.

The golden comparison is unaffected and needed no regeneration. #92164 sorted
the Rust evaluation-context aggregate, so the built payload now matches the
golden that was already committed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature/feature-flags Feature Tag: Feature flags stamphog Request AI approval (no full review) team/feature-flags

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

1 participant