From 4c2b4bf75bac9e1b19923d33218b5a8366af0d40 Mon Sep 17 00:00:00 2001 From: "Gustavo H. Strassburger" Date: Wed, 28 Oct 2026 17:08:08 -0300 Subject: [PATCH] fix(node): keep local flag evaluation consistent with the server --- .changeset/condition-loop-unexpected-error.md | 5 + .changeset/server-required-flag-dependency.md | 5 + .../node/src/__tests__/evaluate-flags.spec.ts | 132 ++++++++++++++++++ .../extensions/feature-flags/feature-flags.ts | 37 ++++- 4 files changed, 173 insertions(+), 6 deletions(-) create mode 100644 .changeset/condition-loop-unexpected-error.md create mode 100644 .changeset/server-required-flag-dependency.md diff --git a/.changeset/condition-loop-unexpected-error.md b/.changeset/condition-loop-unexpected-error.md new file mode 100644 index 0000000000..2a24933490 --- /dev/null +++ b/.changeset/condition-loop-unexpected-error.md @@ -0,0 +1,5 @@ +--- +'posthog-node': patch +--- + +fix: let later conditions resolve a flag locally when an unexpected error follows a filter that local evaluation cannot check, instead of failing the whole flag diff --git a/.changeset/server-required-flag-dependency.md b/.changeset/server-required-flag-dependency.md new file mode 100644 index 0000000000..53f16dee78 --- /dev/null +++ b/.changeset/server-required-flag-dependency.md @@ -0,0 +1,5 @@ +--- +'posthog-node': patch +--- + +fix: fall back to the server when a flag depends on a flag that only the server can evaluate (for example, one with a static cohort), instead of letting a later condition choose a different value locally. `unresolvedFlags` reports such a flag as `unresolved_dependency` diff --git a/packages/node/src/__tests__/evaluate-flags.spec.ts b/packages/node/src/__tests__/evaluate-flags.spec.ts index b2bce426dd..4abc21ae2f 100644 --- a/packages/node/src/__tests__/evaluate-flags.spec.ts +++ b/packages/node/src/__tests__/evaluate-flags.spec.ts @@ -1285,6 +1285,138 @@ describe('evaluateFlags', () => { expect(flags.unresolvedFlags).toEqual(unresolvedFlags) } ) + + it('lets a later condition resolve the flag when an unexpected error follows a filter that cannot be checked', async () => { + const dependsOnBeta = { + key: 'beta', + type: 'flag', + value: true, + operator: 'flag_evaluates_to', + dependency_chain: ['beta'], + } + await setupWithDefinitions([ + alwaysOn('checkout', { + filters: { + groups: [ + { properties: [missingPlan, dependsOnBeta], rollout_percentage: 100 }, + { properties: [], rollout_percentage: 100 }, + ], + }, + }), + alwaysOn('beta', { id: 3 }), + ]) + vi.spyOn((posthog as any).featureFlagsPoller, 'evaluateFlagDependency').mockRejectedValue(new Error('unexpected')) + + const flags = await posthog.evaluateFlags('user-123', { onlyEvaluateLocally: true }) + + expect(flags.getFlag('checkout')).toBe(true) + expect(flags.unresolvedFlags).toEqual({}) + }) + + describe('a dependency that only the server can evaluate', () => { + const dependentFlag = (): Record => + alwaysOn('target', { + id: 4, + filters: { + groups: [ + { + properties: [ + { key: 'dep', type: 'flag', value: true, operator: 'flag_evaluates_to', dependency_chain: ['dep'] }, + ], + rollout_percentage: 100, + variant: 'a', + }, + { properties: [], rollout_percentage: 100, variant: 'b' }, + ], + multivariate: { + variants: [ + { key: 'a', rollout_percentage: 50 }, + { key: 'b', rollout_percentage: 50 }, + ], + }, + }, + }) + const dependencyFlag = (property: Record): Record => + alwaysOn('dep', { id: 5, filters: { groups: [{ properties: [property], rollout_percentage: 100 }] } }) + const remoteTargetResponse = (): PostHogV2FlagsResponse => ({ + flags: { + target: { + key: 'target', + enabled: true, + variant: 'a', + reason: { code: 'condition_match', condition_index: 0, description: 'Matched condition set 1' }, + metadata: { id: 4, version: 1, payload: undefined, description: undefined }, + }, + }, + errorsWhileComputingFlags: false, + requestId: 'request-id-1', + }) + + it.each([ + ['requested alone', ['target']], + ['requested with all flags', undefined], + ])('falls back to the server instead of choosing a later variant, with the flag %s', async (_, flagKeys) => { + await setupWithDefinitions([dependencyFlag(staticCohort), dependentFlag()], remoteTargetResponse()) + + const flags = await posthog.evaluateFlags('user-123', { flagKeys }) + + expect(flags.getFlag('target')).toBe('a') + expect(remoteFlagCalls()).toHaveLength(1) + }) + + it('reports the flag as an unresolved dependency in local-only evaluation', async () => { + await setupWithDefinitions([dependencyFlag(staticCohort), dependentFlag()]) + + const flags = await posthog.evaluateFlags('user-123', { flagKeys: ['target'], onlyEvaluateLocally: true }) + + expect(flags.getFlag('target')).toBeUndefined() + expect(flags.unresolvedFlags).toEqual({ target: 'unresolved_dependency' }) + }) + + const dependsOnDep = { + key: 'dep', + type: 'flag', + value: true, + operator: 'flag_evaluates_to', + dependency_chain: ['dep'], + } + it.each([ + ['the dependency first', [dependsOnDep, unknownTierOperator]], + ['the unsupported filter first', [unknownTierOperator, dependsOnDep]], + ])('reports a cohort with an unsupported filter as unsupported_definition, with %s', async (_, values) => { + await setupWithDefinitions( + [ + dependencyFlag(staticCohort), + alwaysOn('target', { + id: 4, + filters: { groups: [{ properties: [{ key: 'id', type: 'cohort', value: 3 }], rollout_percentage: 100 }] }, + }), + ], + undefined, + { cohorts: { '3': { type: 'AND', values } } } + ) + + const flags = await posthog.evaluateFlags('user-123', { + flagKeys: ['target'], + onlyEvaluateLocally: true, + personProperties: { tier: 'gold' }, + }) + + expect(flags.unresolvedFlags).toEqual({ target: 'unsupported_definition' }) + }) + + it('resolves locally when the dependency resolves locally', async () => { + await setupWithDefinitions([dependencyFlag(missingPlan), dependentFlag()]) + + const flags = await posthog.evaluateFlags('user-123', { + flagKeys: ['target'], + personProperties: { plan: 'pro' }, + }) + + expect(flags.getFlag('target')).toBe('a') + expect(remoteFlagCalls()).toHaveLength(0) + }) + }) }) describe('overrides', () => { diff --git a/packages/node/src/extensions/feature-flags/feature-flags.ts b/packages/node/src/extensions/feature-flags/feature-flags.ts index 4310432421..bc336ffb34 100644 --- a/packages/node/src/extensions/feature-flags/feature-flags.ts +++ b/packages/node/src/extensions/feature-flags/feature-flags.ts @@ -67,10 +67,11 @@ function setCustomErrorPrototype(error: Error, constructor: new (message: string } class RequiresServerEvaluation extends Error { - // Server-side data such as a static cohort is never available locally. - readonly reason: UnresolvedFlagReason = 'unsupported_definition' - - constructor(message: string) { + constructor( + message: string, + // Server-side data such as a static cohort is never available locally. + readonly reason: UnresolvedFlagReason = 'unsupported_definition' + ) { super(message) setCustomErrorPrototype(this, RequiresServerEvaluation) } @@ -551,6 +552,14 @@ class FeatureFlagsPoller { const depResult = await this.computeFlagValueLocally(depFlag, evaluationContext) evaluationCache[depFlagKey] = depResult } catch (error) { + // Only the server knows this dependency's value, and that value can decide which + // condition of the dependent flag matches, so the dependent flag falls back too. + if (error instanceof RequiresServerEvaluation) { + throw new RequiresServerEvaluation( + `Flag dependency '${depFlagKey}' for flag '${targetFlagKey}' requires server evaluation: ${error.message}`, + 'unresolved_dependency' + ) + } throw preserveRegexUncertainty( new InconclusiveMatchError( `Error evaluating flag dependency '${depFlagKey}' for flag '${targetFlagKey}': ${error}`, @@ -680,8 +689,19 @@ class FeatureFlagsPoller { } } catch (e) { if (e instanceof RequiresServerEvaluation) { - // Static cohort or other missing server-side data - must fallback to API - throw e + // Static cohort or other missing server-side data - must fallback to API. + // The fallback skips later conditions, so a higher reason in them comes from the definition. + let definitionReason: UnresolvedFlagReason | undefined + try { + definitionReason = getDefinitionUnresolvedReason(flag, evaluationContext.definitions!) + } catch { + // A malformed definition must not turn the server fallback into an evaluation error. + } + const reason = pickUnresolvedReason( + pickUnresolvedReason(inconclusiveReason, e.reason), + definitionReason ?? e.reason + ) + throw reason === e.reason ? e : new RequiresServerEvaluation(e.message, reason) } else if (e instanceof InconclusiveMatchError) { // Evaluation error (bad regex, invalid date, missing property, etc.) // Track that we had an inconclusive match, but try other conditions @@ -793,6 +813,11 @@ class FeatureFlagsPoller { inconclusiveReason = pickUnresolvedReason(inconclusiveReason, e.reason) continue } + // After a filter that could not be checked, stop the condition there as inconclusive, + // so an unexpected error does not stop later conditions from resolving the flag. + if (inconclusiveReason !== undefined) { + break + } throw e }