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>

[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>

[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>

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

[dustinbyrne]

this looks like the real bug fix imo