fix(server): keep locally evaluated flags when one flag is inconclusive

[matheus-vb]

💡 Motivation and Context

evaluateFlags discarded the entire locally computed flag map as soon as one flag definition was inconclusive, then replaced the snapshot with a /flags response. One foreign flag gated on a person property the caller never passes therefore forced a billed request per identity, flagKeys could not prevent it because it was applied only after a successful local pass, and during a /flags outage trivially resolvable flags read false for the whole cache window.

evaluateFlags now evaluates locally first and keeps what it resolved, asking /flags only for the keys that stayed unresolved, which is what every other server SDK already does. flagKeys scopes the local evaluation loop rather than the definitions map, so flag dependencies still resolve. onlyEvaluateLocally is now strictly local and never serves cached remote values. posthog-server.api is unchanged.

Request volume

Exactly one new path reaches the billed /flags endpoint: a non InconclusiveMatchException error while evaluating a flag definition now falls back instead of propagating into the caller. That only fires for a customer whose local evaluation is already throwing, so it trades a crash for one request rather than quietly adding volume.

Every other input state is less than or equal to today, and two strictly decrease. The inconclusive trigger is unchanged, flagKeys only shrinks the evaluated set, the cache is still consulted before every request with failures honoured, and customers without a personal API key never reach the new code. Verified across fifteen input states against a worktree at the base commit: thirteen identical, two lower. $feature_flag_called volume is unchanged, since values are recomputed per call but consistently.

The change most worth reviewer attention is not billing: a group aggregated flag evaluated without groups resolves locally to false, and that now takes precedence over the server's answer.

💚 How did you test it?

Added coverage in PostHogEvaluateFlagsTest and PostHogFeatureFlagsTest for the local wins merge, flagKeys scoping, flag dependencies under scoping, outage plus negative caching, strict onlyEvaluateLocally, undefined requested keys, empty definitions, group flags, and definitions that fail to load. Each assertion was checked by mutating the implementation and confirming only the intended test fails. Full module suite is green at 442 tests, plus make checkFormat and apiDump with no diff.

📝 Checklist

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

If releasing new changes

• Ran pnpm changeset to generate a changeset file

[posthog]

🦔 ReviewHog reviewed this pull request

Found 1 must fix, 0 should fix, 2 consider.

Published 3 findings (view the review).

[posthog]

ReviewHog Alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏

[posthog]

ReviewHog Report

Changes

Issues: 3 issues

Files (4)

• .changeset/local-evaluation-local-wins-merge.md
• posthog-server/src/main/java/com/posthog/server/PostHogEvaluateFlagsOptions.kt
• posthog-server/src/main/java/com/posthog/server/PostHogInterface.kt
• posthog-server/src/main/java/com/posthog/server/internal/PostHogFeatureFlags.kt

[posthog]

Requested flags missing from stale local definitions no longer fall back to /flags

Why we think it's a valid issue

• Checked: The actual PR-head implementation (the working-tree file is the pre-PR base and lacks these changes; I fetched head ca96729 and cross-checked with gh pr diff 681): evaluateFlagsLocally (lines 254-319), the getFeatureFlagsFromLocalEvaluation shim (321-341), and evaluateFlags (934-1033).
• Found: evaluateFlagsLocally scopes the eval loop to requestedKeys and continues past non-requested definitions (281-284); needsRemote is set only for InconclusiveMatchException/Throwable (299, 302). A requested key not present in currentFlagDefinitions is only logged (307-314) and never sets needsRemote. Therefore, when flagKeys names an undefined key and every requested defined key resolves, the outcome is flags={},{missing key},needsRemote=false.
• Found: In evaluateFlags, if (local != null && (!local.needsRemote || onlyEvaluateLocally)) (line 991) returns early with local.flags — missing the undefined key — so getFeatureFlagsFromRemote (1013) is never called and no /flags request is made.
• Found (regression): Base evaluateFlags (per the diff) routes through getFeatureFlagsFromLocalEvaluation, which returns null whenever any defined flag is inconclusive, forcing a /flags fallback that passes the original flagKeys and fetches the locally-undefined key from the server. For any org with a property-gated flag evaluated without those properties (the common case), base returned the authoritative value while head returns the key as missing.
• Impact: A local-evaluation caller requesting a flag absent from stale/lagging local definitions — notably a newly created flag before the next definitions poll — now gets it reported missing (flag_missing, effectively disabled/false) instead of its real server value, for the duration of the poll window. This is a real correctness/compatibility regression on the public evaluateFlags API with a concrete trigger and consequence; the author's undefined-key log message ("check for deleted flags or typos", line 312) frames these as deleted/typos and does not account for the definitions-lag scenario. must_fix is appropriate.

Issue description

When flagKeys contains a key absent from currentFlagDefinitions, the code only logs it and leaves needsRemote false. If every locally defined requested flag resolves, evaluateFlags returns immediately without querying /flags. Local definitions are periodically refreshed and can legitimately lag behind newly created flags, so an existing public call that explicitly requests such a flag now reports it missing until the next definitions poll instead of obtaining the authoritative server value. This is a behavioral compatibility regression for flagKeys.

Suggested fix

Treat requested keys without a local definition as unresolved: set needsRemote = true for them and include them in the fallback request. Ideally construct the remote flagKeys from the undefined and inconclusive keys, then merge the response underneath locally resolved values.

Prompt to fix with AI (copy-paste)

## Context
@posthog-server/src/main/java/com/posthog/server/internal/PostHogFeatureFlags.kt#L306-315
@posthog-server/src/main/java/com/posthog/server/internal/PostHogFeatureFlags.kt#L990-991

<issue_description>
When `flagKeys` contains a key absent from `currentFlagDefinitions`, the code only logs it and leaves `needsRemote` false. If every locally defined requested flag resolves, `evaluateFlags` returns immediately without querying `/flags`. Local definitions are periodically refreshed and can legitimately lag behind newly created flags, so an existing public call that explicitly requests such a flag now reports it missing until the next definitions poll instead of obtaining the authoritative server value. This is a behavioral compatibility regression for `flagKeys`.
</issue_description>

<issue_validation>
- **Checked:** The actual PR-head implementation (the working-tree file is the pre-PR base and lacks these changes; I fetched head `ca96729` and cross-checked with `gh pr diff 681`): `evaluateFlagsLocally` (lines 254-319), the `getFeatureFlagsFromLocalEvaluation` shim (321-341), and `evaluateFlags` (934-1033).
- **Found:** `evaluateFlagsLocally` scopes the eval loop to `requestedKeys` and `continue`s past non-requested definitions (281-284); `needsRemote` is set only for `InconclusiveMatchException`/`Throwable` (299, 302). A requested key not present in `currentFlagDefinitions` is only logged (307-314) and never sets `needsRemote`. Therefore, when `flagKeys` names an undefined key and every requested *defined* key resolves, the outcome is `flags={},{missing key},needsRemote=false`.
- **Found:** In `evaluateFlags`, `if (local != null && (!local.needsRemote || onlyEvaluateLocally))` (line 991) returns early with `local.flags` — missing the undefined key — so `getFeatureFlagsFromRemote` (1013) is never called and no `/flags` request is made.
- **Found (regression):** Base `evaluateFlags` (per the diff) routes through `getFeatureFlagsFromLocalEvaluation`, which returns `null` whenever *any* defined flag is inconclusive, forcing a `/flags` fallback that passes the original `flagKeys` and fetches the locally-undefined key from the server. For any org with a property-gated flag evaluated without those properties (the common case), base returned the authoritative value while head returns the key as missing.
- **Impact:** A local-evaluation caller requesting a flag absent from stale/lagging local definitions — notably a newly created flag before the next definitions poll — now gets it reported missing (`flag_missing`, effectively disabled/false) instead of its real server value, for the duration of the poll window. This is a real correctness/compatibility regression on the public `evaluateFlags` API with a concrete trigger and consequence; the author's undefined-key log message ("check for deleted flags or typos", line 312) frames these as deleted/typos and does not account for the definitions-lag scenario. `must_fix` is appropriate.
</issue_validation>

## Task
Investigate the issue and solve it

<potential_solution>
Treat requested keys without a local definition as unresolved: set `needsRemote = true` for them and include them in the fallback request. Ideally construct the remote `flagKeys` from the undefined and inconclusive keys, then merge the response underneath locally resolved values.
</potential_solution>

[gustavohstrassburger]

Keeping this behavior, and I don't think it's a regression — it's deliberate parity with posthog-python, which is the reference server SDK here.

In posthog/client.py, _get_all_flags_and_payloads_locally filters the definitions by flag_keys_to_evaluate before the evaluation loop:

flags_to_process = self.feature_flags
if flag_keys_to_evaluate:
    flag_keys_set = set(flag_keys_to_evaluate)
    flags_to_process = [
        flag for flag in self.feature_flags if flag["key"] in flag_keys_set
    ]

A requested key with no local definition is dropped by that filter, so it never reaches a try block and never sets fallback_to_flags. Same outcome as here: absent from the snapshot, no /flags request bought on its own.

The definitions-lag case you describe is real, and I've made it explicit rather than leaving it implied — the log line now reads "check for deleted flags, typos, or a flag created since the last definitions poll", and the same caveat is on PostHogInterface.evaluateFlags, PostHogEvaluateFlagsOptions, and the changeset. The tradeoff is deliberate: making an unknown key force a billed request per identity is the exact failure mode this PR exists to remove, and it would make a typo in flagKeys unboundedly expensive.

If an unresolved flag already forces the request, the undefined key does ride along and gets filled — covered by a requested undefined key is filled by a request an unresolved flag already forced.

[posthog]

Remote fallback fetches keys documented as absent

Why we think it's a valid issue

• Checked: PR-head implementation (fetched head ca96729; the working-tree file is the pre-PR base): evaluateFlagsLocally (254-319), the fallback + merge in evaluateFlags (1002-1033), and the changeset's "absent rather than fetched" contract. Traced flagKeys=[inconclusiveKey, undefinedKey, resolvedKey].
• Found: When any requested defined key is inconclusive, needsRemote=true bypasses the early return (line 991) and getFeatureFlagsFromRemote is called with the original full flagKeys (1013-1020). merged = LinkedHashMap(remoteFlags).apply { putAll(localFlags) } (1024) therefore contains the remote-fetched undefinedKey, contradicting the changeset claim that undefined keys are absent. resolvedKey is also included in the /flags body. Finding is factually accurate.
• Found (why not must_fix): No wrong/corrupt values result — undefinedKey/inconclusiveKey carry correct server values and resolvedKey keeps its local value via putAll. The re-sent keys ride the same /flags request needsRemote already forces, so there is no extra round-trip and no extra billed request (per-request billing) — the efficiency concern is immaterial.
• Found (prescription conflicts): The suggested fix (exclude undefined keys from the remote request) would make server-only/newly-created flags always return missing — the very regression raised in 2-1-1 — so fetching them here is the more-correct direction. The genuine residual is that the changeset/contract overstates "absent rather than fetched"; the accurate resolution is a documentation fix, not the code exclusion proposed.
• Impact: Real but low-severity: a requested locally-undefined key appears in results only when an unrelated requested flag is inconclusive (inconsistent presence), with correct values throughout. Worth recording as a contract/doc inconsistency, but it does not meet the must_fix correctness bar.
• Priority: Downgrade to consider — a real, PR-introduced inconsistency, but no incorrect values, negligible efficiency impact, and a prescribed fix that would worsen correctness elsewhere.

Issue description

The fallback forwards the original flagKeys list instead of only the keys whose local evaluation was inconclusive. Consequently, if one requested flag is inconclusive and another requested key has no local definition, /flags evaluates both and the undefined key can appear in merged. This contradicts the new public contract that keys without local definitions are absent rather than fetched. It also means locally resolved requested flags are unnecessarily sent for remote evaluation.

Suggested fix

Track unresolved keys in LocalEvaluationOutcome rather than only needsRemote. When flagKeys scopes the call, pass only unresolved keys with known local definitions to getFeatureFlagsFromRemote; exclude undefined and already-resolved keys. Add a regression test combining an inconclusive requested flag with a server-only requested flag and assert the latter is neither included in the request nor returned.

Prompt to fix with AI (copy-paste)

## Context
@posthog-server/src/main/java/com/posthog/server/internal/PostHogFeatureFlags.kt#L1014-1020

<issue_description>
The fallback forwards the original `flagKeys` list instead of only the keys whose local evaluation was inconclusive. Consequently, if one requested flag is inconclusive and another requested key has no local definition, `/flags` evaluates both and the undefined key can appear in `merged`. This contradicts the new public contract that keys without local definitions are absent rather than fetched. It also means locally resolved requested flags are unnecessarily sent for remote evaluation.
</issue_description>

<issue_validation>
- **Checked:** PR-head implementation (fetched head `ca96729`; the working-tree file is the pre-PR base): `evaluateFlagsLocally` (254-319), the fallback + merge in `evaluateFlags` (1002-1033), and the changeset's "absent rather than fetched" contract. Traced `flagKeys=[inconclusiveKey, undefinedKey, resolvedKey]`.
- **Found:** When any requested defined key is inconclusive, `needsRemote=true` bypasses the early return (line 991) and `getFeatureFlagsFromRemote` is called with the original full `flagKeys` (1013-1020). `merged = LinkedHashMap(remoteFlags).apply { putAll(localFlags) }` (1024) therefore contains the remote-fetched `undefinedKey`, contradicting the changeset claim that undefined keys are absent. `resolvedKey` is also included in the `/flags` body. Finding is factually accurate.
- **Found (why not must_fix):** No wrong/corrupt values result — `undefinedKey`/`inconclusiveKey` carry correct server values and `resolvedKey` keeps its local value via `putAll`. The re-sent keys ride the same `/flags` request `needsRemote` already forces, so there is no extra round-trip and no extra billed request (per-request billing) — the efficiency concern is immaterial.
- **Found (prescription conflicts):** The suggested fix (exclude undefined keys from the remote request) would make server-only/newly-created flags *always* return missing — the very regression raised in 2-1-1 — so fetching them here is the more-correct direction. The genuine residual is that the changeset/contract overstates "absent rather than fetched"; the accurate resolution is a documentation fix, not the code exclusion proposed.
- **Impact:** Real but low-severity: a requested locally-undefined key appears in results only when an unrelated requested flag is inconclusive (inconsistent presence), with correct values throughout. Worth recording as a contract/doc inconsistency, but it does not meet the must_fix correctness bar.
- **Priority:** Downgrade to `consider` — a real, PR-introduced inconsistency, but no incorrect values, negligible efficiency impact, and a prescribed fix that would worsen correctness elsewhere.
</issue_validation>

## Task
Investigate the issue and solve it

<potential_solution>
Track unresolved keys in `LocalEvaluationOutcome` rather than only `needsRemote`. When `flagKeys` scopes the call, pass only unresolved keys with known local definitions to `getFeatureFlagsFromRemote`; exclude undefined and already-resolved keys. Add a regression test combining an inconclusive requested flag with a server-only requested flag and assert the latter is neither included in the request nor returned.
</potential_solution>

[gustavohstrassburger]

Agreed with your own downgrade note: the accurate fix is documentation, not excluding the keys from the request. Addressed in 5e13e06 — the KDoc on evaluateFlags/PostHogEvaluateFlagsOptions and the changeset no longer claim undefined keys are unconditionally absent, they now say the key is absent unless an unresolved flag already requires the /flags call, which then also fills it.

Not narrowing the request body to unresolved keys, for two reasons: it would make server-only and newly-created flags permanently invisible (the regression raised in the sibling thread), and the keys ride a request needsRemote already forced, so there's no extra round-trip and no extra billed request.

Covered by a requested undefined key is filled by a request an unresolved flag already forced.

[posthog]

Add regression coverage for unexpected local-evaluation failures

Why we think it's a valid issue

• Checked: The new catch (Throwable) branch (head PostHogFeatureFlags.kt:300-302), its sibling catch (InconclusiveMatchException) (297-299), and the 16 tests added by the PR (PostHogEvaluateFlagsTest.kt, PostHogFeatureFlagsTest.kt) via gh pr diff 681.
• Found: The catch (Throwable) path is new in this PR and changes an unexpected evaluator failure from caller-visible propagation into a per-flag remote fallback. None of the added tests injects a definition that makes computeFlagLocally throw a non-InconclusiveMatchException — the added coverage exercises inconclusive flags, outage, undefined keys, group flags, empty definitions, and definitions-fail-to-load, but not this branch. So the "generic throwable is caught, not propagated" behavior is genuinely uncovered.
• Found: The delta is narrow, though — the catch (Throwable) branch yields the same LocalEvaluationOutcome (localFlags kept, needsRemote=true) as the tested inconclusive branch, so the downstream assertions the finding proposes (local value wins, failed key filled by one /flags request, differing locallyEvaluated markers) are already pinned by the existing an inconclusive flag does not discard the flags that resolved locally test. Only the catch-not-propagate transition itself is untested.
• Impact: A real but modest coverage gap: a regression removing/narrowing the catch would restore a caller-visible crash on a malformed flag definition, and that exact transition has no test. Not noise, but most of the branch's behavior is already covered by the sibling test and the ask is a speculative regression guard.
• Priority: Downgrade to consider — genuine new-branch gap worth recording, but below the should_fix bar given the sibling test already exercises the shared downstream behavior.

Issue description

The new catch (Throwable) path changes an unexpected evaluator failure from a caller-visible exception into a per-flag remote fallback, but no test exercises it. A regression could therefore restore the crash, discard already resolved local flags, or fail to fetch the affected flag without detection.

Suggested fix

Add a test with one locally resolvable flag and one definition that makes computeFlagLocally throw a non-InconclusiveMatchException. Assert that evaluation does not throw, the resolved local value wins, the failed key is filled by exactly one /flags request, and their locallyEvaluated markers differ.

Prompt to fix with AI (copy-paste)

## Context
@posthog-server/src/main/java/com/posthog/server/internal/PostHogFeatureFlags.kt#L299-302

<issue_description>
The new `catch (Throwable)` path changes an unexpected evaluator failure from a caller-visible exception into a per-flag remote fallback, but no test exercises it. A regression could therefore restore the crash, discard already resolved local flags, or fail to fetch the affected flag without detection.
</issue_description>

<issue_validation>
- **Checked:** The new `catch (Throwable)` branch (head `PostHogFeatureFlags.kt:300-302`), its sibling `catch (InconclusiveMatchException)` (297-299), and the 16 tests added by the PR (`PostHogEvaluateFlagsTest.kt`, `PostHogFeatureFlagsTest.kt`) via `gh pr diff 681`.
- **Found:** The `catch (Throwable)` path is new in this PR and changes an unexpected evaluator failure from caller-visible propagation into a per-flag remote fallback. None of the added tests injects a definition that makes `computeFlagLocally` throw a non-`InconclusiveMatchException` — the added coverage exercises inconclusive flags, outage, undefined keys, group flags, empty definitions, and definitions-fail-to-load, but not this branch. So the "generic throwable is caught, not propagated" behavior is genuinely uncovered.
- **Found:** The delta is narrow, though — the `catch (Throwable)` branch yields the same `LocalEvaluationOutcome` (localFlags kept, `needsRemote=true`) as the tested inconclusive branch, so the downstream assertions the finding proposes (local value wins, failed key filled by one `/flags` request, differing `locallyEvaluated` markers) are already pinned by the existing `an inconclusive flag does not discard the flags that resolved locally` test. Only the catch-not-propagate transition itself is untested.
- **Impact:** A real but modest coverage gap: a regression removing/narrowing the catch would restore a caller-visible crash on a malformed flag definition, and that exact transition has no test. Not noise, but most of the branch's behavior is already covered by the sibling test and the ask is a speculative regression guard.
- **Priority:** Downgrade to `consider` — genuine new-branch gap worth recording, but below the should_fix bar given the sibling test already exercises the shared downstream behavior.
</issue_validation>

## Task
Investigate the issue and solve it

<potential_solution>
Add a test with one locally resolvable flag and one definition that makes `computeFlagLocally` throw a non-`InconclusiveMatchException`. Assert that evaluation does not throw, the resolved local value wins, the failed key is filled by exactly one `/flags` request, and their `locallyEvaluated` markers differ.
</potential_solution>

[gustavohstrassburger]

Added in 5e13e06: an error thrown while evaluating one flag falls back for that flag instead of crashing. It pairs a conclusive definition with throwingFlagDefinition (a multivariate definition holding a null variant, which makes computeFlagLocally NPE) and asserts evaluation doesn't propagate, the local value wins, the broken key is filled by exactly one /flags request, and the two keys' locally_evaluated markers differ.

While here I also narrowed the branch from catch (Throwable) to catch (Exception) (4220fa0) — Throwable would have swallowed OutOfMemoryError/StackOverflowError and then issued a network request on the way out. posthog-python catches Exception at the same spot. The test is unaffected; an NPE is an Exception.

[dustinbyrne]

isn't this effectively the same from the end user perspective? i.e., we're just swapping out locally evaluated flags for remote flags - ideally they should evaluate to be the same things, but i'd consider /flags to be the correct canonical source

this is still a billable request either way

[dustinbyrne]

i suppose which set takes precedence is ambiguous in the sdk specification

Server-side flow

1. Resolve evaluation context from distinct_id plus optional groups, person properties, group properties, device id, and geoip settings.
2. Attempt local evaluation of all requested flags when definitions are available.
3. Collect values and payloads from the local evaluation result.
4. Fall back to remote evaluation if local evaluation is unavailable, incomplete, or explicitly bypassed.
5. Return both maps together in one result object.

[gustavohstrassburger]

Not quite the same, and this is the part I'd most like a second opinion on.

On billing you're right that a request is a request — the win isn't cheaper requests, it's fewer of them. Before this change one inconclusive definition discarded the whole locally-computed batch, so a single foreign flag gated on a person property the caller never passes forced a /flags request per identity, forever. After it, that flag alone is unresolved and the rest keep their local values. When every requested flag resolves locally the request count goes to zero, which is the case flagKeys scoping now makes reachable.

Where it's genuinely not equivalent: local and remote can disagree, and the sharpest case is a group-aggregated flag evaluated without groups. computeFlagLocally answers false rather than throwing, so the flag counts as resolved and that false now beats the server's true. I called it out in the PR body and pinned it with a group flag with no groups supplied resolves locally to false and wins over the server so it's a decision rather than a surprise. It's also exactly how posthog-python behaves today.

The other non-equivalence is a /flags outage: locally-resolvable flags used to read false for the whole cache window and now hold their real values.

[gustavohstrassburger]

You're right that the spec doesn't settle it — step 4 says "fall back to remote evaluation if local evaluation is unavailable, incomplete, or explicitly bypassed" without saying which set wins on overlap. So I took posthog-python's evaluate_flags as the tiebreaker, since it's the SDK furthest along on this flow. It records local results first, then on fallback skips any key it already resolved:

for key, detail in response.get("flags", {}).items():
    if key in locally_evaluated_keys:
        continue

So: local wins, remote fills gaps only. That's what this PR now does, and I've put the reasoning in a comment next to the merge (4220fa0) so it isn't re-litigated on the next pass.

Happy to flip it to remote-wins if you'd rather treat /flags as canonical — it's a one-line change to the merge direction plus the group-flag test. But it would put android out of step with python, and it would mean local evaluation is never authoritative, which makes onlyEvaluateLocally a bit incoherent. Might be worth pinning down in sdk-specs either way so the next SDK doesn't have to guess.

[dustinbyrne]

this looks like the real bug fix imo

[gustavohstrassburger]

Agreed — that's the one with teeth. flagKeys used to be applied only after a successful local pass, so a flag you never asked about could still make the pass inconclusive and force a request. It now scopes the evaluation loop, which is what makes "ask for the two flags I care about, pay nothing" actually work.

Worth noting it scopes the loop and not the definitions map — computeFlagLocally resolves flag dependencies through the full map, so narrowing the map would make every dependent flag inconclusive. Pinned by flagKeys scoping leaves flag dependencies resolvable.

[marandaneto]

moving to draft until comments are resolved

[marandaneto]

@matheus-vb are you gonna follow up on this?