Skip to content

fix: Return a malformed flag result instead of throwing for a definition the evaluator cannot read - #2095

Draft
kinyoklion wants to merge 1 commit into
mainfrom
rlamb/evaluator-malformed-guard
Draft

kinyoklion wants to merge 1 commit into
mainfrom
rlamb/evaluator-malformed-guard

Conversation

@kinyoklion

@kinyoklion kinyoklion commented Oct 9, 2026 •

Copy link
Copy Markdown
Member

fix: Return a malformed flag result instead of throwing for a definition the evaluator cannot read

Summary

The server evaluator assumed every flag and segment it received was well formed. When a definition was not, for example a flag with no variations, a target or clause with no values, or a segment whose included list is not an array, reading it threw a TypeError inside the evaluation. For variation() that surfaced as a rejected promise. For allFlagsState() it was worse: the iteration helper ran Promise.all(...).then(cb) with no rejection path, so the call never resolved and the rejection was unhandled.

With this change evaluation is total. An exception raised while reading a flag, or a prerequisite or segment it references, whether it happens synchronously or inside one of the asynchronous continuations (a store lookup that calls back later, a big segment membership lookup, or an iteration step deferred to a resolved promise), becomes an EvalResult error of kind MALFORMED_FLAG, delivered through the normal callback exactly once. variation() returns the default value with that reason, and allFlagsState() reports the flag through the existing error path, records it with a null value and an ERROR reason, and continues with the other flags.

Data from LaunchDarkly is well formed, so this does not change behavior for flags delivered by the service. It matters for everything else that can hand the evaluator a definition: a custom feature store, the file data source, test data, and any future override source. Those are hand written or user controlled, and a shape mistake there should produce a default value and an error, not a hang or a crash.

What changed

Evaluator (packages/shared/sdk-server/src/evaluation/Evaluator.ts):

  • evaluateCb runs the evaluation inside a guard and gives the per-evaluation state an onError handler. The handler builds the error through EvalResult.forError(ErrorKinds.MalformedFlag, ...), the same path the evaluator uses for its other error results, and delivers it through the same deliverResult step that decorates every result with big segment status, prerequisites, and events. The message names the flag, says the definition (or a prerequisite or segment it references) is malformed, and includes the underlying error message.
  • Delivery is tracked with a delivered flag on the state, so nothing is handed to the caller twice. If an exception arrives after delivery it came from the caller's callback, not from the flag, and it is rethrown so it stays visible exactly as it did before.
  • The continuations of getFlag and getSegment store lookups are guarded, because a store backed by a database calls back after the original try/catch has left the stack. The big segment promise chains gain a .catch that routes to the same handler, and the two-step chain is joined so a throw in either step is covered.
  • Every call into the iteration helpers passes the state's onError.

Iteration helpers (packages/shared/sdk-server/src/evaluation/collection.ts):

  • allSeriesAsync, firstSeriesAsync, and allAsync take a required onError callback. An exception thrown by a check or by the completion callback is delivered to it instead of escaping or, in the deferred and Promise.all cases, becoming an unhandled rejection that leaves the caller waiting forever.
  • The series helpers reject a collection that is not an array with a TypeError. Previously a string was indexed character by character and an object was treated as empty, which for a rule with clauses: {} meant the rule matched every context. firstResult already threw for a non-array.

Variations (packages/shared/sdk-server/src/evaluation/variations.ts):

  • getVariation checks that variations is an array before indexing it. A missing list used to throw and a string used to serve one of its characters as the flag value.

Client (packages/shared/sdk-server/src/LDClientImpl.ts):

  • allFlagsState passes an error handler to allAsync. A malformed flag is already an error result and keeps the existing behavior (reported through onError, recorded with a null value, other flags continue). The new handler covers an unexpected failure outside of evaluation while the state is assembled: it is reported through onError and the call settles with an invalid, empty state instead of never resolving. An exception from the caller's own callback is rethrown as before.

Tests

  • __tests__/evaluation/Evaluator.malformed.test.ts (new): one test per malformed shape, each asserting a MALFORMED_FLAG result with a null value and index and a message naming the flag: no variations; variations that is a string; a rule whose clauses is an object; a clause with no values; a target with no values; a context target with no values; a segment whose included is an object; a segment rule clause with no values; a malformed prerequisite under a valid parent (the parent gets the error and, because the prerequisite never produced a result, nothing is recorded or emitted for it). The segment and prerequisite cases run against a store that calls back synchronously and one that calls back asynchronously. Two more cover the big segment path: a malformed rule evaluated after a membership miss, and a membership lookup that rejects. One test checks that an exception thrown by the receiving callback propagates and that the result was delivered once.
  • __tests__/evaluation/collection.test.ts (new): for each series helper, a throw from a check in a small collection and in a large collection past the deferral point reaches onError without cb being called; a throw from the completion callback reaches onError both when the checks called back synchronously and when a check called back from a promise; a non-array collection is reported without running a check; the normal aggregate result is unchanged. allAsync has the same check, completion callback, and aggregate cases.
  • __tests__/LDClient.allFlags.test.ts: allFlagsState with a flag that has no variations resolves, keeps the healthy flag, records the malformed one with an ERROR reason, and reports it once through the error callback. A store item that throws after evaluation (standing in for an unexpected failure) is reported and yields an invalid state, through both the promise and the callback.
  • __tests__/LDClient.evaluation.test.ts: variation() and variationDetail() return the default value with a MALFORMED_FLAG reason for a flag with a target that has no values.

Not in this change

  • Validation of definition shape at the data source. The file data source and the test data source still accept any object; this change makes the evaluator safe regardless of the source.
  • processFlag in serialization.ts still throws for a non-array rules, which affects deserialization in the data sources rather than evaluation.
  • A rule with no clauses field at all keeps its existing behavior (treated as not matching); only a clauses value that is present but not an array is reported.
  • variation() does not log evaluator error results, which is pre-existing and unchanged. The reason is visible through variationDetail(), and allFlagsState() reports through the error path.

Note

Overview
Malformed or corrupt flag/segment data no longer crashes or hangs evaluation. Exceptions while reading definitions (missing variations, non-array clauses/included, targets/clauses without values, async store callbacks, big-segment promise failures) are converted into a single MALFORMED_FLAG EvalResult delivered once via the normal callback. variation() / variationDetail() return the default with that reason; allFlagsState() records the bad flag as null with an ERROR reason, invokes onError, and continues other flags.

Iteration helpers (allSeriesAsync, firstSeriesAsync, allAsync) now take a required onError handler, reject non-array collections with a TypeError, and route throws/rejections (including after deferred iteration) so callers are not left with pending promises. getVariation validates that variations is an array before indexing.

allFlagsState wires that error path: unexpected failures while building state call onError and resolve with an invalid empty state instead of never settling; post-delivery exceptions from the caller’s callback still propagate.

Reviewed by Cursor Bugbot for commit 3021333. Bugbot is set up for automated code reviews on this repo. Configure here.

…ion the evaluator cannot read

A flag or segment with a missing or mistyped field, such as a flag without variations or a clause without values, threw a TypeError while it was evaluated. variation() rejected, and allFlagsState() never resolved because the iteration helper ran Promise.all(...).then(cb) with no rejection path.

The evaluator now runs each evaluation inside a guard and routes exceptions from the asynchronous continuations (store lookups, big segment membership, iteration steps deferred to a resolved promise) to the same handler. The handler builds a MALFORMED_FLAG error through EvalResult.forError, like the evaluator's other error results, and delivers it through the normal callback exactly once. The message names the flag and includes the underlying error. An exception raised after delivery came from the caller and is rethrown as before.

The iteration helpers take an onError callback and deliver a throw from a check or completion callback to it instead of letting it escape or hang. They also report a collection that is not an array, which for a rule with clauses set to an object previously matched every context. getVariation checks that variations is an array, since a string was indexed character by character.

allFlagsState keeps its existing handling of error results, reporting the flag through onError and continuing with the other flags. If assembling the state fails for another reason it reports the failure and settles with an invalid state instead of leaving the promise pending.

Data from LaunchDarkly is well formed. This protects evaluations fed by a custom store, the file data source, test data, or a future override source.
@kinyoklion

Copy link
Copy Markdown
Member Author

bugbot review

@github-actions

github-actions Bot commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

@launchdarkly/js-sdk-common size report
This is the brotli compressed size of the ESM build.
Compressed size: 29603 bytes
Compressed size limit: 30000
Uncompressed size: 142116 bytes

@github-actions

github-actions Bot commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

@launchdarkly/js-client-sdk size report
This is the brotli compressed size of the ESM build.
Compressed size: 32662 bytes
Compressed size limit: 34000
Uncompressed size: 117055 bytes

@github-actions

github-actions Bot commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

@launchdarkly/js-client-sdk-common size report
This is the brotli compressed size of the ESM build.
Compressed size: 25437 bytes
Compressed size limit: 44000
Uncompressed size: 165420 bytes

@cursor cursor 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.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 3021333. Configure here.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant