Evaluation can throw when the same segment is used in multiple rules within a single flag

[kadler15]

Describe the bug
The 6.x Evaluator contains a bug in its segment matching logic.

When a segment is checked against a context in segmentMatchesContext, the segment key is added to a segment stack for cycle detection purposes. If no segment rule matches the context, the segment key is popped off the segment stack, and false is returned. If, however, a segment rule matches the context, true is returned without popping the segment key off the segment stack.

Should a rule not match despite containing a matching segment clause, and the same segment appear in a subsequent rule, the segment match logic will throw EvaluationException the second time it is run on the same segment.

https://github.com/launchdarkly/java-server-sdk/blob/6.0.4/src/main/java/com/launchdarkly/sdk/server/Evaluator.java#L525-L533

      state.segmentStack.add(segment.getKey());
      int nRules = rules.size();
      for (int i = 0; i < nRules; i++) {
        SegmentRule rule = rules.get(i);
        if (segmentRuleMatchesContext(rule, context, state, segment.getKey(), segment.getSalt())) {
          return true; // Returns true without popping the segment key
        }
      }
      state.segmentStack.remove(state.segmentStack.size() - 1);

To reproduce

1. Create a segment Test in LaunchDarkly, and add your user to it
2. Create a boolean feature flag with targeting on, and a default variation of false
3. Add a rule: If user is not in segment Test, serve false
4. Add a rule: If user is in segment Test, serve true
5. Evaluate the flag using your user as context

Expected behavior
Flag evaluation should not throw just because the same segment is used in multiple rules a single flag.

SDK version
6.0.1

[louis-launchdarkly]

Hello @kadler15,

Thank you for the detailed bug report about this issue. We will look into this and provide an update on this.
Filed internally as 185539.

[louis-launchdarkly]

Hello @kadler15,

I would love to get some clarification on this issue, as I am trying to reproduce this using your instruction but was not able to.

As you described in the description, this affects the segment rules (as the logic is to detect a loop when a segment rule refers to another segment). However, in the reproduction steps, there is no instruction to add any rule to the segment (as the segment rule does not serve value, steps 3 and 4 should be referring to adding a flag rule), which means the execution didn't get into the section of the code you quoted above when I debug this.

Am I missing anything that I need to configure inside the segment? If you can provide the UI screenshot on how you configure the flag, the segment, and the stack trace when the Evaluator throws an exception, that will help a lot in debugging this.

[kadler15]

Hi @louis-launchdarkly,

I'm happy to help.

As you described in the description, this affects the segment rules (as the logic is to detect a loop when a segment rule refers to another segment). However, in the reproduction steps, there is no instruction to add any rule to the segment (as the segment rule does not serve value, steps 3 and 4 should be referring to adding a flag rule), which means the execution didn't get into the section of the code you quoted above when I debug this.

Sorry, that's a mistake on my part in the steps to reproduce.

When setting up the test segment in step 1, you should add a rule: If name is one of: <your name>, <other name>, .... This bug only manifests when your user matches a segment rule, but doesn't match the flag rule being evaluated.

Am I missing anything that I need to configure inside the segment? If you can provide the UI screenshot on how you configure the flag, the segment, and the stack trace when the Evaluator throws an exception, that will help a lot in debugging this.

Also, further testing shows the LD client doesn't actually throw an exception. An exception is thrown by Evaluator.evaluateInternal and caught by Evaluator.evaluate, which logs the error and returns an error result. The client variation methods return the default value, and allFlagsState inserts null for the flag. So, flag evaluation can fail, but not throw, as a result of this bug. My apologizes for the misleading description.

See the screenshots below for my current configuration. Evaluating the flag as a user with name Admin results in error:

> client.stringVariation("test-flag", ldUser, "default-str")
ERROR c.l.sdk.server.LDClient.Evaluation - Could not evaluate flag "test-flag": null
default-str

If I remove the second flag rule, there is no error, and evaluation returns the fallback variation (first), as expected.

Thanks for your help BTW!

Test Segment rules:

String flag configuration:

[louis-launchdarkly]

Hello @kadler15,

Thank you for the clarification, What you said makes sense and I will try to reproduce this with the latest info.

[louis-launchdarkly]

Thank you for the detailed reproduction steps, we are preparing a patch release for this now. I will give another update when the release is done.

[louis-launchdarkly]

This is fixed in https://github.com/launchdarkly/java-server-sdk/releases/tag/6.0.5

Please let us know if there is any other issue.