From e431ba1f6e2ae64fda411b4f007d42a5ad492482 Mon Sep 17 00:00:00 2001 From: Jonas Alves Date: Wed, 9 Sep 2026 18:39:40 +0100 Subject: [PATCH 01/28] feat(holdouts): add holdout fields to context types (FT-2206) Add holdout-related optional fields to ContextData, ExperimentData, and Assignment in src/context.ts, ported from java-sdk. These are purely additive fields laying groundwork for holdout index construction, suppression logic, and exposure firing in later tasks. - ContextData.holdouts?: ExperimentData[] - ExperimentData.holdoutIds?: number[] - Assignment.suppressed?: boolean - Assignment.holdouts?: Experiment[] (resolved applicable-holdout list) - Assignment.holdoutAssignments?: (Assignment | null)[] (pinned per-holdout resolved assignments at decision time) Exposure type intentionally left unchanged. --- src/context.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/context.ts b/src/context.ts index 8267382..fe22f4a 100644 --- a/src/context.ts +++ b/src/context.ts @@ -47,6 +47,7 @@ export type ExperimentData = { custom: boolean; audienceMismatch: boolean; customFieldValues: CustomFieldValue[] | null; + holdoutIds?: number[]; }; type Assignment = { @@ -68,6 +69,9 @@ type Assignment = { trafficSplit?: number[]; variables?: Record; attrsSeq?: number; + suppressed?: boolean; + holdouts?: Experiment[]; + holdoutAssignments?: (Assignment | null)[]; }; export type Experiment = { @@ -126,6 +130,7 @@ export type ContextOptions = { export type ContextData = { experiments?: ExperimentData[]; + holdouts?: ExperimentData[]; }; export default class Context { From 34f036075b2ed7eec58492e708e1e50e66973c3e Mon Sep 17 00:00:00 2001 From: Jonas Alves Date: Wed, 9 Sep 2026 18:45:22 +0100 Subject: [PATCH 02/28] feat(holdouts): resolve applicable holdouts at index-build time (FT-2206) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Build a live _holdoutsById index (keyed by id, skipping holdouts with no/empty split) during _init(), and resolve each experiment's applicable holdouts from its holdoutIds against it — dropping missing ids and sorting by id ascending. Store the resolved Experiment[] (or null) on the experiment's internal index entry so later suppression logic (Task 4) can copy it onto the live Assignment.holdouts field. --- src/context.ts | 71 +++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 68 insertions(+), 3 deletions(-) diff --git a/src/context.ts b/src/context.ts index fe22f4a..06a3881 100644 --- a/src/context.ts +++ b/src/context.ts @@ -77,6 +77,7 @@ type Assignment = { export type Experiment = { data: ExperimentData; variables: Record[]; + holdouts?: Experiment[] | null; }; export type Unit = { @@ -155,6 +156,7 @@ export default class Context { private _goals: Goal[]; private _index: Record; private _indexVariables: Record; + private _holdoutsById: Record; private _overrides: Record; private _pending: number; private _attrsSeq: number; @@ -1224,11 +1226,74 @@ export default class Context { const index: Record = {}; const indexVariables: Record = {}; - for (const experiment of data.experiments || []) { + // Live index of holdout definitions by id, skipping holdouts with no/empty + // split (they can never be assigned to, so they are treated as non-existent). + // Kept as raw ExperimentData (not a resolved Experiment/Assignment) so later + // lookups (e.g. resolving a holdout's own assignment) always read against the + // currently-installed data rather than a possibly-stale cached reference. + const holdoutsById: Record = {}; + + (data.holdouts || []).forEach((holdout) => { + if (holdout.split && holdout.split.length > 0) { + holdoutsById[holdout.id] = holdout; + } + }); + + this._holdoutsById = holdoutsById; + + // Experiment wrappers (data + parsed variables) for holdouts, built lazily and + // memoized per _init() call so a holdout referenced by multiple experiments is + // only parsed once. + const holdoutExperiments: Record = {}; + + const resolveHoldoutExperiment = (holdoutId: number): Experiment | undefined => { + if (holdoutExperiments[holdoutId]) { + return holdoutExperiments[holdoutId]; + } + + // Read via the live field (not the local `holdoutsById` closure) so this + // always resolves against the currently-installed data. + const holdoutData = this._holdoutsById[holdoutId]; + if (!holdoutData) { + return undefined; + } + + const holdoutVariables: Record[] = []; + holdoutData.variants.forEach((variant, i) => { + const config = variant.config; + holdoutVariables[i] = config != null && config.length > 0 ? JSON.parse(config) : {}; + }); + + const holdoutEntry: Experiment = { + data: holdoutData, + variables: holdoutVariables, + }; + + holdoutExperiments[holdoutId] = holdoutEntry; + return holdoutEntry; + }; + + (data.experiments || []).forEach((experiment) => { const variables: Record[] = []; - const entry = { + + let holdouts: Experiment[] | null = null; + if (experiment.holdoutIds && experiment.holdoutIds.length > 0) { + const resolved: Experiment[] = []; + + experiment.holdoutIds.forEach((holdoutId) => { + const holdoutExperiment = resolveHoldoutExperiment(holdoutId); + if (holdoutExperiment) { + insertUniqueSorted(resolved, holdoutExperiment, (a, b) => a.data.id < b.data.id); + } + }); + + holdouts = resolved.length > 0 ? resolved : null; + } + + const entry: Experiment = { data: experiment, variables, + holdouts, }; index[experiment.name] = entry; @@ -1266,7 +1331,7 @@ export default class Context { variables[i] = parsed; } - } + }); this._index = index; this._indexVariables = indexVariables; From 2d337ed8dec2968c2eee82cad4c32d3b628e60fd Mon Sep 17 00:00:00 2001 From: Jonas Alves Date: Wed, 9 Sep 2026 18:51:57 +0100 Subject: [PATCH 03/28] feat(holdouts): resolve holdout's own arm assignment (FT-2206) Add _getHoldoutAssignment(holdout, unitType), memoized by holdout id/unitType (Record), which resolves the arm a unit falls into within a holdout itself. Ported from java-sdk's getHoldoutAssignment (Context.java:1412-1468), minus the read/write-lock dance since js is single-threaded. Always resolves against the live _holdoutsById definition first (falling back to the caller-supplied reference only if the id is no longer present), returns the cached assignment if id/iteration still match, otherwise recomputes via the same VariantAssigner get-or-create pattern _assign() already uses and calls .assign() directly against the holdout's split/seedHi/seedLo (holdouts have no separate traffic-split step). Returns null without caching when no unit is set for the given unitType, so a later call recomputes once the unit is set. Not yet wired into _assign() or exposure firing (Tasks 4/6); currently unused, which trips tsc's noUnusedLocals (TS6133) and therefore fails `npm test` at the ts-jest compile step until Task 4 adds a call site - expected and tracked, not worked around. --- src/context.ts | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/src/context.ts b/src/context.ts index 06a3881..050a2ae 100644 --- a/src/context.ts +++ b/src/context.ts @@ -157,6 +157,7 @@ export default class Context { private _index: Record; private _indexVariables: Record; private _holdoutsById: Record; + private _holdoutAssignments: Record; private _overrides: Record; private _pending: number; private _attrsSeq: number; @@ -185,6 +186,7 @@ export default class Context { this._cassignments = {}; this._units = {}; this._assigners = {}; + this._holdoutAssignments = {}; this._audienceMatcher = new AudienceMatcher(); this._environmentName = null; this._attrsSeq = 0; @@ -1219,6 +1221,56 @@ export default class Context { return this._hashes[unitType]; } + // Resolves the arm a unit falls into within a holdout itself (as opposed to resolving an + // ordinary experiment's assignment, which is `_assign()`). Ported from java-sdk's + // getHoldoutAssignment (Context.java:1412-1468), minus the read/write-lock dance: js is + // single-threaded, so this simplifies to a plain memoized-by-(id, unitType) cache. + // + // `holdout` may be a stale reference (e.g. captured before a data refresh) so it is always + // re-resolved against the live `_holdoutsById` index first (falling back to the caller-supplied + // reference only if the id is no longer present, e.g. the holdout was removed by the latest + // refresh) — this mirrors java-sdk's resolveLiveHoldout and ensures the cache is keyed and + // validated against the currently-installed definition rather than a possibly-dead one. + private _getHoldoutAssignment(holdout: Experiment, unitType: string): Assignment | null { + const liveHoldoutData = this._holdoutsById[holdout.data.id] ?? holdout.data; + + const cacheKey = `${liveHoldoutData.id}:${unitType}`; + const cached = this._holdoutAssignments[cacheKey]; + if (cached && cached.id === liveHoldoutData.id && cached.iteration === liveHoldoutData.iteration) { + return cached; + } + + const unit = this._unitHash(unitType); + if (unit === null) { + // No unit set for this unitType yet — mirrors java-sdk's `uid == null -> return null`. + // Do not cache: a later call, once the unit is set, must recompute. + return null; + } + + const assigner = + unitType in this._assigners ? this._assigners[unitType] : (this._assigners[unitType] = new VariantAssigner(unit)); + + const assignment: Assignment = { + id: liveHoldoutData.id, + iteration: liveHoldoutData.iteration, + fullOnVariant: 0, + unitType, + variant: assigner.assign(liveHoldoutData.split, liveHoldoutData.seedHi, liveHoldoutData.seedLo), + overridden: false, + assigned: true, + exposed: false, + eligible: true, + fullOn: false, + custom: false, + audienceMismatch: false, + ruleOverride: false, + }; + + this._holdoutAssignments[cacheKey] = assignment; + + return assignment; + } + private _init(data: ContextData, assignments: Record = {}) { this._data = data; this._environmentName = this._sdk.getClient().getEnvironment() ?? null; From 2cc59c52e6e9d7fcdcf1d301bd3730370da219d1 Mon Sep 17 00:00:00 2001 From: Jonas Alves Date: Wed, 9 Sep 2026 19:02:18 +0100 Subject: [PATCH 04/28] feat(holdouts): suppress covered experiments via holdout arms (FT-2206) Resolve applicable holdout assignments unconditionally in _assign() (both override and non-override paths) and compute per-experiment suppression via isHeldOutBy, ported verbatim from java-sdk's Context.isHeldOutBy. On the non-override path, a suppressed experiment is forced to assigned=false/variant=0 ahead of the audienceStrict/fullOnVariant/fullOn chain, taking precedence over custom assignments. The override path is left untouched so an explicit override still wins for the experiment's own variant, while assignment.holdouts/holdoutAssignments are still populated so the holdout's own exposure can fire independently. --- src/context.ts | 40 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/src/context.ts b/src/context.ts index 050a2ae..d498b3c 100644 --- a/src/context.ts +++ b/src/context.ts @@ -134,6 +134,14 @@ export type ContextData = { holdouts?: ExperimentData[]; }; +// Ported verbatim from java-sdk's Context.isHeldOutBy (Context.java:1319-1330). Decides whether +// a single holdout's resolved arm suppresses the covered experiment it applies to. +function isHeldOutBy(holdoutVariant: number, holdoutArmCount: number, fullOnVariant: number): boolean { + if (holdoutVariant === 0) return true; + if (holdoutArmCount === 3 && holdoutVariant === 1) return fullOnVariant === 0; + return false; +} + export default class Context { private readonly _assigners: Record; private readonly _attrs: Attribute[]; @@ -616,6 +624,33 @@ export default class Context { this._assignments[experimentName] = assignment; + // Resolve applicable holdouts and compute suppression unconditionally — this must run + // regardless of override/custom-assignment/rule-variant handling below, because a + // holdout's own exposure (fired later, using assignment.holdoutAssignments) must fire + // whether or not the covered experiment itself ends up overridden or suppressed. + if (experiment != null && experiment.holdouts != null && experiment.holdouts.length > 0) { + const holdouts = experiment.holdouts; + const holdoutUnitType = experiment.data.unitType; + + const holdoutAssignments: (Assignment | null)[] = holdouts.map((holdout) => + holdoutUnitType !== null ? this._getHoldoutAssignment(holdout, holdoutUnitType) : null + ); + + assignment.holdouts = holdouts; + assignment.holdoutAssignments = holdoutAssignments; + + let suppressed = false; + holdoutAssignments.forEach((holdoutAssignment, i) => { + if (holdoutAssignment != null) { + if (isHeldOutBy(holdoutAssignment.variant, holdouts[i].data.split.length, experiment.data.fullOnVariant)) { + suppressed = true; + } + } + }); + + assignment.suppressed = suppressed; + } + if (hasOverride) { if (experiment != null) { assignment.id = experiment.data.id; @@ -660,7 +695,10 @@ export default class Context { } } - if (experiment.data.audienceStrict && assignment.audienceMismatch) { + if (assignment.suppressed) { + assignment.assigned = false; + assignment.variant = 0; + } else if (experiment.data.audienceStrict && assignment.audienceMismatch) { assignment.variant = 0; } else if (experiment.data.fullOnVariant === 0) { if (unitType !== null) { From fd4c4a127bd8e04e9119efe379d7497ecd1cfc5b Mon Sep 17 00:00:00 2001 From: Jonas Alves Date: Wed, 9 Sep 2026 19:15:11 +0100 Subject: [PATCH 05/28] feat(holdouts): extend cache-validity check for holdout set changes, fix suppressed+custom exposure thrashing (FT-2206) Add holdoutSetMatches (ported from java-sdk Context.holdoutSetMatches, Context.java:991-1005) to _assign()'s fast-path staleness check: a cached assignment is now invalidated when the experiment's resolved applicable-holdout set differs from the set pinned on the assignment, by (id, iteration) per entry rather than full deep-equality, so cosmetic holdout edits don't force a duplicate exposure while membership/identity changes do. Also fix an exposure-thrashing bug flagged during Task 4's review: the custom-assignment mismatch check (_cassignments[name] === variant) always failed for a suppressed assignment, since suppression forces variant to 0 regardless of the custom assignment on file. This meant the fast path never reached experimentMatches/audienceMatches, so a fresh Assignment was rebuilt on every _assign() call and a duplicate exposure was queued on every treatment() call. Fixed by treating assignment.suppressed as bypassing the custom-assignment-mismatch check (suppression legitimately overrides the custom assignment per scenario 211), while still gating the early return on experimentMatches/audienceMatches/holdoutSetMatches so a real change still triggers a rebuild. --- src/context.ts | 40 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 38 insertions(+), 2 deletions(-) diff --git a/src/context.ts b/src/context.ts index d498b3c..c25cd93 100644 --- a/src/context.ts +++ b/src/context.ts @@ -582,6 +582,32 @@ export default class Context { return true; }; + // Ported from java-sdk's Context.holdoutSetMatches (Context.java:991-1005). Compares the + // pinned holdout set the cached assignment was built against with the freshly-resolved + // applicable-holdout set by (id, iteration) per entry — not full deep-equality, since + // cosmetic holdout edits (e.g. seed/split changes) on an unrelated field shouldn't force a + // duplicate exposure. Only membership/identity changes (added/removed holdout, or an + // existing one's id/iteration changing) invalidate the cached assignment. + const holdoutSetMatches = (experiment: Experiment, assignment: Assignment) => { + const freshHoldouts = experiment.holdouts ?? []; + const pinnedHoldouts = assignment.holdouts ?? []; + + if (freshHoldouts.length !== pinnedHoldouts.length) { + return false; + } + + for (let i = 0; i < freshHoldouts.length; i++) { + if (freshHoldouts[i].data.id !== pinnedHoldouts[i].data.id) { + return false; + } + if (freshHoldouts[i].data.iteration !== pinnedHoldouts[i].data.iteration) { + return false; + } + } + + return true; + }; + const hasCustom = experimentName in this._cassignments; const hasOverride = experimentName in this._overrides; const experiment = experimentName in this._index ? this._index[experimentName] : null; @@ -598,8 +624,18 @@ export default class Context { // previously not-running experiment return assignment; } - } else if (!hasCustom || this._cassignments[experimentName] === assignment.variant) { - if (experimentMatches(experiment.data, assignment) && audienceMatches(experiment.data, assignment)) { + } else if (assignment.suppressed || !hasCustom || this._cassignments[experimentName] === assignment.variant) { + // When the assignment is currently suppressed, a custom-assignment variant + // mismatch is expected (the holdout forces variant 0 regardless of the custom + // assignment on file per scenario 211) and must not be treated as staleness on + // its own — experimentMatches/audienceMatches/holdoutSetMatches below still gate + // the return, so a real change (unit type, holdout set, etc.) still falls through + // to a rebuild. + if ( + experimentMatches(experiment.data, assignment) && + audienceMatches(experiment.data, assignment) && + holdoutSetMatches(experiment, assignment) + ) { // assignment up-to-date return assignment; } From 231025571b575ad4f765781a520c4da08a90da9f Mon Sep 17 00:00:00 2001 From: Jonas Alves Date: Wed, 9 Sep 2026 19:33:22 +0100 Subject: [PATCH 06/28] fix(holdouts): revalidate holdout set on override fast path, pin holdout arm count (FT-2206) Two Important findings from Task 5 review, both fixed: - The hasOverride fast-path branch in _assign() returned the cached assignment early without checking holdoutSetMatches, unlike the non-override branch. An already-overridden experiment whose holdout coverage changed across a refresh (holdout added/removed, or an applicable holdout's iteration changed) kept returning the same frozen Assignment forever, with holdouts/holdoutAssignments/ suppressed stuck at their last-rebuild values. Fixed by requiring holdoutSetMatches(experiment, assignment) alongside the existing overridden/variant checks (skipped when experiment is null, since there's nothing to check against). - isHeldOutBy's arm-count argument was read from the live holdout definition (holdouts[i].data.split.length) instead of the arm count the cached holdout Assignment's variant was actually resolved against. Since _getHoldoutAssignment's cache only invalidates on (id, iteration) change, a same-iteration split-length change could desync the live arm count from the pinned resolved arm, causing isHeldOutBy to misinterpret which arm the unit is in. Fixed by pinning the arm count (split.length) onto the holdout's own Assignment at resolution time (new holdoutArmCount field) and reading it from there instead of the live definition, mirroring the existing precedent in experimentMatches (which compares assignment.trafficSplit against the live value rather than trusting iteration alone). --- src/context.ts | 37 ++++++++++++++++++++++++++++++++++--- 1 file changed, 34 insertions(+), 3 deletions(-) diff --git a/src/context.ts b/src/context.ts index c25cd93..68c9495 100644 --- a/src/context.ts +++ b/src/context.ts @@ -72,6 +72,12 @@ type Assignment = { suppressed?: boolean; holdouts?: Experiment[]; holdoutAssignments?: (Assignment | null)[]; + // Only set on a holdout's own resolved Assignment (as returned by _getHoldoutAssignment): the + // arm count (`split.length`) the holdout's definition had at the moment `variant` was + // resolved. Pinned alongside `variant` rather than re-read from the live holdout definition, + // so a same-iteration refresh that changes `split.length` can't desync the resolved arm from + // the arm count used to interpret it (mirrors java-sdk's HoldoutAssignment, Context.java:1218-1222). + holdoutArmCount?: number; }; export type Experiment = { @@ -615,7 +621,18 @@ export default class Context { if (experimentName in this._assignments) { const assignment = this._assignments[experimentName]; if (hasOverride) { - if (assignment.overridden && assignment.variant === this._overrides[experimentName]) { + // The holdout set must be revalidated here too, mirroring the non-override + // branch below — otherwise a holdout that becomes (or stops being) applicable + // to an already-overridden experiment after a refresh is never picked up, and + // assignment.holdouts/holdoutAssignments/suppressed stay frozen forever (Task 6 + // relies on holdoutAssignments to decide which holdouts' own exposures to fire). + // `experiment == null` means there's no live experiment to check against, so + // treat that as trivially matching (nothing to invalidate against). + if ( + assignment.overridden && + assignment.variant === this._overrides[experimentName] && + (experiment == null || holdoutSetMatches(experiment, assignment)) + ) { // override up-to-date return assignment; } @@ -676,9 +693,22 @@ export default class Context { assignment.holdoutAssignments = holdoutAssignments; let suppressed = false; - holdoutAssignments.forEach((holdoutAssignment, i) => { + holdoutAssignments.forEach((holdoutAssignment) => { if (holdoutAssignment != null) { - if (isHeldOutBy(holdoutAssignment.variant, holdouts[i].data.split.length, experiment.data.fullOnVariant)) { + // Read the arm count from the holdout's own pinned Assignment + // (holdoutArmCount), not the live holdout definition (holdouts[i].data.split.length) + // — the pinned Assignment's `variant` was resolved against whatever split + // length was live at that time, and a same-iteration refresh can change + // split.length without invalidating _getHoldoutAssignment's cache, so reading + // the live value here could desync the resolved arm from the arm count used + // to interpret it. See holdoutArmCount's doc comment on the Assignment type. + if ( + isHeldOutBy( + holdoutAssignment.variant, + holdoutAssignment.holdoutArmCount ?? 0, + experiment.data.fullOnVariant + ) + ) { suppressed = true; } } @@ -1338,6 +1368,7 @@ export default class Context { custom: false, audienceMismatch: false, ruleOverride: false, + holdoutArmCount: liveHoldoutData.split.length, }; this._holdoutAssignments[cacheKey] = assignment; From 1d7557c0365d77325627b379ac769246a16207ce Mon Sep 17 00:00:00 2001 From: Jonas Alves Date: Wed, 9 Sep 2026 19:56:10 +0100 Subject: [PATCH 07/28] feat(holdouts): fire holdout exposures on first evaluation (FT-2206) Wires up Task 6 of the holdouts port: _treatment() and _variableValue() now gate the covered experiment's own exposure on !assignment.suppressed (with an override exception, since Task 4 pins `suppressed` eagerly for both override and non-override paths, unlike java-sdk which only computes it for the non-override path), and always attempt to fire every applicable holdout's own exposure via a new shared _triggerApplicableHoldoutExposures method. Each holdout's own Assignment tracks its own once-only exposed flag, shared across experiments that reference the same holdout. A throwing eventLogger for one holdout does not prevent sibling holdouts from firing; the first error is collected and rethrown after the loop. _peek()/_peekVariable() remain untouched and side-effect-free. Verified against cross-sdk-tests fixtures for scenarios 210, 211, and 213 via throwaway scratch tests (deleted before this commit). --- src/context.ts | 61 ++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 59 insertions(+), 2 deletions(-) diff --git a/src/context.ts b/src/context.ts index 68c9495..9048f28 100644 --- a/src/context.ts +++ b/src/context.ts @@ -837,12 +837,62 @@ export default class Context { if (!assignment.exposed) { assignment.exposed = true; - this._queueExposure(experimentName, assignment); + // An override always fires its own exposure, even when the covered experiment is also + // suppressed by a holdout: overriding replaces the resolved variant outright (the override's + // value wins, not the holdout's), so its own exposure must still be observable. This mirrors + // java-sdk's outcome for the override path (Context.java:1184-1244, triggerExposure at + // Context.java:481-503) — java's override write path never sets `assignment.suppressed` at + // all, so its own `!assignment.suppressed` exposure gate trivially always passes there. Our + // JS port pins `suppressed` eagerly for the override path too (Task 4's deliberate + // divergence, see Assignment.suppressed doc comment), so the exposure gate here must + // special-case `overridden` explicitly to reproduce the same firing outcome. + if (!assignment.suppressed || assignment.overridden) { + this._queueExposure(experimentName, assignment); + } + + this._triggerApplicableHoldoutExposures(assignment); } return assignment; } + // Ported from java-sdk's triggerApplicableHoldoutExposures/triggerHoldoutExposure + // (Context.java:481-546). Fires each applicable holdout's own exposure exactly once, + // using the pinned `assignment.holdoutAssignments` snapshot (not a live re-resolution), + // so a data refresh landing between the suppression decision and the exposure trigger + // can't publish a holdout exposure from a different epoch (Context.java:505-514). + // A throwing eventLogger for one holdout must not prevent siblings from firing: collect + // the first error and rethrow it only after every holdout has had a chance to fire. + private _triggerApplicableHoldoutExposures(assignment: Assignment): void { + const holdouts = assignment.holdouts; + const holdoutAssignments = assignment.holdoutAssignments; + if (!holdouts || !holdoutAssignments) return; + + let firstError: unknown; + let hasError = false; + + holdoutAssignments.forEach((holdoutAssignment, i) => { + if (holdoutAssignment == null) return; + + if (!holdoutAssignment.exposed) { + holdoutAssignment.exposed = true; + + try { + this._queueExposure(holdouts[i].data.name, holdoutAssignment); + } catch (error) { + if (!hasError) { + hasError = true; + firstError = error; + } + } + } + }); + + if (hasError) { + throw firstError; + } + } + private _queueExposure(experimentName: string, assignment: Assignment) { const exposureEvent: Exposure = { id: assignment.id, @@ -963,7 +1013,14 @@ export default class Context { if (assignment.variables !== undefined) { if (shouldQueueExposure && !assignment.exposed) { assignment.exposed = true; - this._queueExposure(experimentName, assignment); + + // See _treatment's matching comment: an override always fires its own exposure, + // even when also suppressed by a holdout. + if (!assignment.suppressed || assignment.overridden) { + this._queueExposure(experimentName, assignment); + } + + this._triggerApplicableHoldoutExposures(assignment); } if (key in assignment.variables && (assignment.assigned || assignment.overridden || assignment.ruleOverride)) { From 8eda2e15ddb3c4a4fd939e5a48a13caaac847586 Mon Sep 17 00:00:00 2001 From: Jonas Alves Date: Wed, 9 Sep 2026 20:15:19 +0100 Subject: [PATCH 08/28] fix(holdouts): fix late-unit holdout exposure loss, error-sharing, and missing-variants crash (FT-2206) Addresses three review findings on the holdout exposure firing commit (91ff39a): - Critical: a holdout resolved before its covered experiment's unit type was set never fired its exposure even after the unit later arrived, permanently losing it. Root cause was two-fold: (1) no counterpart to java-sdk's invalidateAssignmentsPinnedWithMissingUnit to evict a cached assignment pinned with a null holdout entry once its unit type is installed, and (2) _unitHash permanently cached a null "unit not set" result, poisoning _getHoldoutAssignment for that unit type even after the unit was later set. Both are now fixed: unit() evicts unexposed assignments whose holdoutAssignments contain a null entry for the unit type just installed, and _unitHash no longer caches negative results. - Important: the covered experiment's own exposure call and the holdout-firing loop now share one "first error wins" outcome (matching java-sdk's triggerExposure), so a throwing eventLogger on the own exposure no longer aborts the holdout loop before it runs. - Important: _init()'s holdout resolution no longer crashes when a holdout definition omits the `variants` key, which is the actual wire shape for most holdout fixtures. Verified against the real cross-sdk-tests fixtures for scenarios 210, 211, 213, and 221 (the last previously failing before this fix) via throwaway scratch tests, deleted before this commit. --- src/context.ts | 120 +++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 101 insertions(+), 19 deletions(-) diff --git a/src/context.ts b/src/context.ts index 9048f28..09387eb 100644 --- a/src/context.ts +++ b/src/context.ts @@ -148,6 +148,12 @@ function isHeldOutBy(holdoutVariant: number, holdoutArmCount: number, fullOnVari return false; } +// Wraps a caught error so "no error occurred" (undefined) can be distinguished from "an error of +// value `undefined` was thrown" when collecting the first error across multiple try/catch sites +// (the covered experiment's own exposure attempt and the holdout-firing loop) that must share one +// "first error wins" outcome, mirroring java-sdk's triggerExposure (Context.java:481-503). +type CaughtError = { value: unknown } | undefined; + export default class Context { private readonly _assigners: Record; private readonly _attrs: Attribute[]; @@ -342,6 +348,40 @@ export default class Context { } this._units[unitType] = uid; + + this._invalidateAssignmentsPinnedWithMissingUnit(unitType); + } + + // Ported from java-sdk's invalidateAssignmentsPinnedWithMissingUnit (Context.java:325-381, + // called from setUnit at line 344). A cached assignment's `holdoutAssignments` snapshot pins a + // null entry when the covered experiment's unit was unavailable at the time it was resolved + // (see `_getHoldoutAssignment`'s `unit === null` early return) — holdouts in that snapshot are + // resolved using the covered experiment's own `unitType`, not each holdout's declared + // unitType, so only installing THAT unit type can repair the null entry. Resolving the holdout + // live later (rather than evicting and letting `_assign()` rebuild both the decision and its + // exposures together) could publish a holdout verdict inconsistent with the cached experiment + // decision, so we evict instead. + // + // Only unexposed assignments are evicted: eviction lets a later `_assign()`/`_treatment()` call + // recompute (and re-fire) exposure from scratch, so evicting an already-exposed assignment + // could publish a duplicate or contradictory experiment exposure. This is not protecting a + // pristine record — an exposure queued before this call may already carry the late unit, since + // publish() reads units from the live `_units` map — but the decision behind it was made + // without that unit, and recomputation cannot repair a record already queued, only add a + // second, conflicting one. The guard avoids compounding a degraded record. + private _invalidateAssignmentsPinnedWithMissingUnit(unitType: string): void { + for (const experimentName in this._assignments) { + const assignment = this._assignments[experimentName]; + const holdoutAssignments = assignment.holdoutAssignments; + + if (holdoutAssignments && assignment.unitType === unitType && !assignment.exposed) { + const hasMissingEntry = holdoutAssignments.some((holdoutAssignment) => holdoutAssignment === null); + + if (hasMissingEntry) { + delete this._assignments[experimentName]; + } + } + } } getUnits() { @@ -837,6 +877,13 @@ export default class Context { if (!assignment.exposed) { assignment.exposed = true; + // Ported from java-sdk's triggerExposure (Context.java:481-503): the own-exposure attempt + // and the holdout-firing loop share one "first error wins" outcome — a throwing eventLogger + // on the OWN exposure must not prevent the holdout loop from running (and vice versa), and + // whichever throws first is what ultimately propagates to the caller, only after both have + // had a chance to fire. + let firstError: CaughtError; + // An override always fires its own exposure, even when the covered experiment is also // suppressed by a holdout: overriding replaces the resolved variant outright (the override's // value wins, not the holdout's), so its own exposure must still be observable. This mirrors @@ -847,10 +894,21 @@ export default class Context { // divergence, see Assignment.suppressed doc comment), so the exposure gate here must // special-case `overridden` explicitly to reproduce the same firing outcome. if (!assignment.suppressed || assignment.overridden) { - this._queueExposure(experimentName, assignment); + try { + this._queueExposure(experimentName, assignment); + } catch (error) { + firstError = { value: error }; + } + } + + const holdoutError = this._triggerApplicableHoldoutExposures(assignment); + if (!firstError) { + firstError = holdoutError; } - this._triggerApplicableHoldoutExposures(assignment); + if (firstError) { + throw firstError.value; + } } return assignment; @@ -862,14 +920,14 @@ export default class Context { // so a data refresh landing between the suppression decision and the exposure trigger // can't publish a holdout exposure from a different epoch (Context.java:505-514). // A throwing eventLogger for one holdout must not prevent siblings from firing: collect - // the first error and rethrow it only after every holdout has had a chance to fire. - private _triggerApplicableHoldoutExposures(assignment: Assignment): void { + // the first error and return it (rather than throwing here) so the caller can combine it + // with its own try/catch's outcome and issue a single final throw after everything has fired. + private _triggerApplicableHoldoutExposures(assignment: Assignment): CaughtError { const holdouts = assignment.holdouts; const holdoutAssignments = assignment.holdoutAssignments; - if (!holdouts || !holdoutAssignments) return; + if (!holdouts || !holdoutAssignments) return undefined; - let firstError: unknown; - let hasError = false; + let firstError: CaughtError; holdoutAssignments.forEach((holdoutAssignment, i) => { if (holdoutAssignment == null) return; @@ -880,17 +938,14 @@ export default class Context { try { this._queueExposure(holdouts[i].data.name, holdoutAssignment); } catch (error) { - if (!hasError) { - hasError = true; - firstError = error; + if (!firstError) { + firstError = { value: error }; } } } }); - if (hasError) { - throw firstError; - } + return firstError; } private _queueExposure(experimentName: string, assignment: Assignment) { @@ -1014,13 +1069,27 @@ export default class Context { if (shouldQueueExposure && !assignment.exposed) { assignment.exposed = true; - // See _treatment's matching comment: an override always fires its own exposure, - // even when also suppressed by a holdout. + // See _treatment's matching comment: the own-exposure attempt and the holdout-firing + // loop share one "first error wins" outcome, and an override always fires its own + // exposure, even when also suppressed by a holdout. + let firstError: CaughtError; + if (!assignment.suppressed || assignment.overridden) { - this._queueExposure(experimentName, assignment); + try { + this._queueExposure(experimentName, assignment); + } catch (error) { + firstError = { value: error }; + } + } + + const holdoutError = this._triggerApplicableHoldoutExposures(assignment); + if (!firstError) { + firstError = holdoutError; } - this._triggerApplicableHoldoutExposures(assignment); + if (firstError) { + throw firstError.value; + } } if (key in assignment.variables && (assignment.assigned || assignment.overridden || assignment.ruleOverride)) { @@ -1374,7 +1443,20 @@ export default class Context { } if (!(unitType in this._hashes)) { - const hash = unitType in this._units ? hashUnit(this._units[unitType]) : null; + // Only cache when the unit is actually available. A `null` result here means the unit + // hasn't been set yet — that can change later (via `unit()`/`setUnit`), whereas a + // resolved hash is stable for the unit's lifetime (the same unit type can only ever be + // set once, enforced by `unit()`). Caching `null` would permanently poison this cache + // for a unit type queried before it was set — e.g. `_getHoldoutAssignment` (unlike the + // ordinary experiment-assignment path, which only calls `_unitHash` after already + // checking `unitType in this._units`) calls this unconditionally, so a holdout resolved + // via `peek()`/`_assign()` before its unit type is installed must be able to resolve + // correctly once that unit later arrives, without a stale cached `null` blocking it. + if (!(unitType in this._units)) { + return null; + } + + const hash = hashUnit(this._units[unitType]); this._hashes[unitType] = hash; return hash; } @@ -1473,7 +1555,7 @@ export default class Context { } const holdoutVariables: Record[] = []; - holdoutData.variants.forEach((variant, i) => { + (holdoutData.variants || []).forEach((variant, i) => { const config = variant.config; holdoutVariables[i] = config != null && config.length > 0 ? JSON.parse(config) : {}; }); From ac161667b4b59275633ee52285dbc5fb09fb8c50 Mon Sep 17 00:00:00 2001 From: Jonas Alves Date: Wed, 9 Sep 2026 20:34:40 +0100 Subject: [PATCH 09/28] test(holdouts): add holdout suppression, exposure, and arm-battery coverage (FT-2206) Adds permanent regression coverage for holdouts (previously verified only via throwaway scratch tests during Tasks 1-6). Ports 18 scenarios from the cross-sdk-tests fixture battery (scenarios 203-222) into a new describe("holdouts", ...) block in context.test.js, following the existing describe("rules evaluation", ...) pattern: basic suppression, normal assignment with holdout self-exposure, union-of-two-holdouts semantics, per-experiment opt-in coverage, dangling holdoutId tolerance, full-on suppression, override/custom-assignment precedence, shared-holdout exposure dedup, the full 3-arm holdout battery, the late-unit exposure guarantee (regression test for the Task 6 C-1 fix), and split-length-derived arity with no holdoutType field on the wire. 382 -> 400 tests, all green. tsc/lint/format:check all clean. --- src/__tests__/context.test.js | 1771 +++++++++++++++++++++++++++++++++ 1 file changed, 1771 insertions(+) diff --git a/src/__tests__/context.test.js b/src/__tests__/context.test.js index 4dacff5..2c7f520 100644 --- a/src/__tests__/context.test.js +++ b/src/__tests__/context.test.js @@ -2889,6 +2889,1777 @@ describe("Context", () => { }); }); + describe("holdouts", () => { + // All fixtures below are ported verbatim (field-for-field) from the real cross-sdk-tests + // scenario battery at ~/git_tree/sdks/cross-sdk-tests/test_scenarios_complete.json, indices + // 202-221 (scenarios 203-222). Each it() names the real scenario number it ports. Two + // scenarios in that range are deliberately not ported 1:1 as separate it() blocks: + // - 209 ("Old-Payload Tolerance For Uncovered Experiment") is an inertness baseline that + // must pass identically on a holdout-unaware SDK; it exercises no holdout-specific code + // path beyond what scenario 206 (opt-in coverage) already covers here. + // - 212 ("Independent Holdouts Across Experiments") exercises the same + // held-out/not-held-out-with-independent-seeds shape already covered by the combination + // of scenarios 203 and 204 below, just spread across two experiments instead of one. + const buildHoldoutResponse = (experiments, holdouts) => ({ experiments, holdouts }); + + // Scenario 203 (index 202): a unit landing in a holdout's variant 0 gets control for the + // experiment it covers (no exposure of its own) and exactly one exposure for the holdout. + it("suppresses the covered experiment and fires only the holdout's exposure when held out (scenario 203)", (done) => { + const response = buildHoldoutResponse( + [ + { + id: 1, + name: "exp_holdout_held_out", + iteration: 1, + unitType: "session_id", + seedHi: 100, + seedLo: 200, + split: [0.5, 0.5], + trafficSeedHi: 1, + trafficSeedLo: 2, + trafficSplit: [0, 1], + fullOnVariant: 0, + applications: [{ name: "website" }], + variants: [ + { name: "A", config: null }, + { name: "B", config: null }, + ], + audience: null, + audienceStrict: false, + customFieldValues: null, + holdoutIds: [11], + }, + ], + [ + { + id: 11, + name: "holdout_a", + iteration: 1, + unitType: "session_id", + seedHi: 13, + seedLo: 111, + split: [0.1, 0.9], + trafficSeedHi: 0, + trafficSeedLo: 0, + trafficSplit: [0, 1], + fullOnVariant: 0, + applications: [], + variants: [ + { name: "A", config: null }, + { name: "B", config: null }, + ], + audience: null, + audienceStrict: false, + customFieldValues: null, + holdoutType: "full", + }, + ] + ); + + const context = new Context(sdk, contextOptions, contextParams, response); + expect(context.treatment("exp_holdout_held_out")).toEqual(0); + + publisher.publish.mockReturnValue(Promise.resolve()); + + context.publish().then(() => { + expect(publisher.publish.mock.calls[0][0].exposures).toEqual([ + { + id: 11, + name: "holdout_a", + unit: "session_id", + exposedAt: timeOrigin, + variant: 0, + assigned: true, + eligible: true, + overridden: false, + fullOn: false, + custom: false, + audienceMismatch: false, + ruleOverride: false, + }, + ]); + done(); + }); + }); + + // Scenario 204 (index 203): a unit outside the holdout's suppressed arm assigns to the + // covered experiment exactly as it would without holdouts, plus one exposure for the + // holdout carrying its nonzero (not-held-out) variant — holdouts always fire on first + // evaluation, not only when they actually suppress. + it("assigns normally and still fires the holdout's own exposure when not held out (scenario 204)", (done) => { + const response = buildHoldoutResponse( + [ + { + id: 1, + name: "exp_holdout_not_held_out", + iteration: 1, + unitType: "session_id", + seedHi: 100, + seedLo: 200, + split: [0.5, 0.5], + trafficSeedHi: 1, + trafficSeedLo: 2, + trafficSplit: [0, 1], + fullOnVariant: 0, + applications: [{ name: "website" }], + variants: [ + { name: "A", config: null }, + { name: "B", config: null }, + ], + audience: null, + audienceStrict: false, + customFieldValues: null, + holdoutIds: [11], + }, + ], + [ + { + id: 11, + name: "holdout_a", + iteration: 1, + unitType: "session_id", + seedHi: 13, + seedLo: 111, + split: [0.1, 0.9], + trafficSeedHi: 0, + trafficSeedLo: 0, + trafficSplit: [0, 1], + fullOnVariant: 0, + applications: [], + variants: [ + { name: "A", config: null }, + { name: "B", config: null }, + ], + audience: null, + audienceStrict: false, + customFieldValues: null, + holdoutType: "full", + }, + ] + ); + + const notHeldOutParams = { units: { session_id: "b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3" } }; + const context = new Context(sdk, contextOptions, notHeldOutParams, response); + expect(context.treatment("exp_holdout_not_held_out")).toEqual(0); + + publisher.publish.mockReturnValue(Promise.resolve()); + + context.publish().then(() => { + expect(publisher.publish.mock.calls[0][0].exposures).toEqual([ + { + id: 1, + name: "exp_holdout_not_held_out", + unit: "session_id", + exposedAt: timeOrigin, + variant: 0, + assigned: true, + eligible: true, + overridden: false, + fullOn: false, + custom: false, + audienceMismatch: false, + ruleOverride: false, + }, + { + id: 11, + name: "holdout_a", + unit: "session_id", + exposedAt: timeOrigin, + variant: 1, + assigned: true, + eligible: true, + overridden: false, + fullOn: false, + custom: false, + audienceMismatch: false, + ruleOverride: false, + }, + ]); + done(); + }); + }); + + // Scenario 205 (index 204): an experiment covered by two holdouts is suppressed if the unit + // is held out by EITHER one (union semantics). Pinned so the low-id holdout does NOT hold the + // unit out and the high-id holdout does, ruling out an implementation that only ever consults + // holdouts[0]. Both holdouts still emit their own independent exposure. + it("suppresses via union when the higher-id holdout holds the unit out (scenario 205)", (done) => { + const response = buildHoldoutResponse( + [ + { + id: 1, + name: "exp_holdout_union", + iteration: 1, + unitType: "session_id", + seedHi: 100, + seedLo: 200, + split: [0.5, 0.5], + trafficSeedHi: 1, + trafficSeedLo: 2, + trafficSplit: [0, 1], + fullOnVariant: 0, + applications: [{ name: "website" }], + variants: [ + { name: "A", config: null }, + { name: "B", config: null }, + ], + audience: null, + audienceStrict: false, + customFieldValues: null, + holdoutIds: [11, 12], + }, + ], + [ + { + id: 11, + name: "holdout_low_id", + iteration: 1, + unitType: "session_id", + seedHi: 1, + seedLo: 222, + split: [0.1, 0.9], + trafficSeedHi: 0, + trafficSeedLo: 0, + trafficSplit: [0, 1], + fullOnVariant: 0, + applications: [], + variants: [ + { name: "A", config: null }, + { name: "B", config: null }, + ], + audience: null, + audienceStrict: false, + customFieldValues: null, + holdoutType: "full", + }, + { + id: 12, + name: "holdout_high_id", + iteration: 1, + unitType: "session_id", + seedHi: 13, + seedLo: 111, + split: [0.1, 0.9], + trafficSeedHi: 0, + trafficSeedLo: 0, + trafficSplit: [0, 1], + fullOnVariant: 0, + applications: [], + variants: [ + { name: "A", config: null }, + { name: "B", config: null }, + ], + audience: null, + audienceStrict: false, + customFieldValues: null, + holdoutType: "full", + }, + ] + ); + + const context = new Context(sdk, contextOptions, contextParams, response); + expect(context.treatment("exp_holdout_union")).toEqual(0); + + publisher.publish.mockReturnValue(Promise.resolve()); + + context.publish().then(() => { + expect(publisher.publish.mock.calls[0][0].exposures).toEqual([ + { + id: 11, + name: "holdout_low_id", + unit: "session_id", + exposedAt: timeOrigin, + variant: 1, + assigned: true, + eligible: true, + overridden: false, + fullOn: false, + custom: false, + audienceMismatch: false, + ruleOverride: false, + }, + { + id: 12, + name: "holdout_high_id", + unit: "session_id", + exposedAt: timeOrigin, + variant: 0, + assigned: true, + eligible: true, + overridden: false, + fullOn: false, + custom: false, + audienceMismatch: false, + ruleOverride: false, + }, + ]); + done(); + }); + }); + + // Scenario 206 (index 205): holdout coverage never leaks to a sibling experiment lacking + // holdoutIds, even when that sibling shares the same unit and context as an experiment the + // same holdout suppresses. + it("does not leak suppression to a sibling experiment with no holdoutIds (scenario 206)", (done) => { + const response = buildHoldoutResponse( + [ + { + id: 1, + name: "exp_holdout_covered", + iteration: 1, + unitType: "session_id", + seedHi: 100, + seedLo: 200, + split: [0.5, 0.5], + trafficSeedHi: 1, + trafficSeedLo: 2, + trafficSplit: [0, 1], + fullOnVariant: 0, + applications: [{ name: "website" }], + variants: [ + { name: "A", config: null }, + { name: "B", config: null }, + ], + audience: null, + audienceStrict: false, + customFieldValues: null, + holdoutIds: [11], + }, + { + id: 2, + name: "exp_holdout_uncovered_sibling", + iteration: 1, + unitType: "session_id", + seedHi: 100, + seedLo: 200, + split: [0.5, 0.5], + trafficSeedHi: 1, + trafficSeedLo: 2, + trafficSplit: [0, 1], + fullOnVariant: 0, + applications: [{ name: "website" }], + variants: [ + { name: "A", config: null }, + { name: "B", config: null }, + ], + audience: null, + audienceStrict: false, + customFieldValues: null, + }, + ], + [ + { + id: 11, + name: "holdout_a", + iteration: 1, + unitType: "session_id", + seedHi: 13, + seedLo: 111, + split: [0.1, 0.9], + trafficSeedHi: 0, + trafficSeedLo: 0, + trafficSplit: [0, 1], + fullOnVariant: 0, + applications: [], + variants: [ + { name: "A", config: null }, + { name: "B", config: null }, + ], + audience: null, + audienceStrict: false, + customFieldValues: null, + holdoutType: "full", + }, + ] + ); + + const context = new Context(sdk, contextOptions, contextParams, response); + expect(context.treatment("exp_holdout_covered")).toEqual(0); + expect(context.treatment("exp_holdout_uncovered_sibling")).toEqual(1); + + publisher.publish.mockReturnValue(Promise.resolve()); + + context.publish().then(() => { + expect(publisher.publish.mock.calls[0][0].exposures).toEqual([ + { + id: 11, + name: "holdout_a", + unit: "session_id", + exposedAt: timeOrigin, + variant: 0, + assigned: true, + eligible: true, + overridden: false, + fullOn: false, + custom: false, + audienceMismatch: false, + ruleOverride: false, + }, + { + id: 2, + name: "exp_holdout_uncovered_sibling", + unit: "session_id", + exposedAt: timeOrigin, + variant: 1, + assigned: true, + eligible: true, + overridden: false, + fullOn: false, + custom: false, + audienceMismatch: false, + ruleOverride: false, + }, + ]); + done(); + }); + }); + + // Scenario 207 (index 206): a holdoutIds entry with no matching holdouts[] element is + // silently ignored - wire inconsistency tolerance, not an error - while a second, valid id in + // the same list still applies normally. + it("tolerates a dangling holdoutId while a valid one in the same list still applies (scenario 207)", (done) => { + const response = buildHoldoutResponse( + [ + { + id: 1, + name: "exp_holdout_dangling_plus_valid", + iteration: 1, + unitType: "session_id", + seedHi: 100, + seedLo: 200, + split: [0.5, 0.5], + trafficSeedHi: 1, + trafficSeedLo: 2, + trafficSplit: [0, 1], + fullOnVariant: 0, + applications: [{ name: "website" }], + variants: [ + { name: "A", config: null }, + { name: "B", config: null }, + ], + audience: null, + audienceStrict: false, + customFieldValues: null, + holdoutIds: [999, 11], + }, + ], + [ + { + id: 11, + name: "holdout_a", + iteration: 1, + unitType: "session_id", + seedHi: 13, + seedLo: 111, + split: [0.1, 0.9], + trafficSeedHi: 0, + trafficSeedLo: 0, + trafficSplit: [0, 1], + fullOnVariant: 0, + applications: [], + variants: [ + { name: "A", config: null }, + { name: "B", config: null }, + ], + audience: null, + audienceStrict: false, + customFieldValues: null, + holdoutType: "full", + }, + ] + ); + + expect(() => { + const context = new Context(sdk, contextOptions, contextParams, response); + expect(context.treatment("exp_holdout_dangling_plus_valid")).toEqual(0); + + publisher.publish.mockReturnValue(Promise.resolve()); + + context.publish().then(() => { + expect(publisher.publish.mock.calls[0][0].exposures).toEqual([ + { + id: 11, + name: "holdout_a", + unit: "session_id", + exposedAt: timeOrigin, + variant: 0, + assigned: true, + eligible: true, + overridden: false, + fullOn: false, + custom: false, + audienceMismatch: false, + ruleOverride: false, + }, + ]); + done(); + }); + }).not.toThrow(); + }); + + // Scenario 208 (index 207): a holdout suppresses a full-on experiment (fullOnVariant != 0) + // exactly as it does a traffic-eligible one - explicit holdoutIds coverage, not full-on + // status, decides suppression. + it("suppresses a full-on experiment regardless of its fullOnVariant (scenario 208)", (done) => { + const response = buildHoldoutResponse( + [ + { + id: 1, + name: "exp_holdout_fullon", + iteration: 1, + unitType: "session_id", + seedHi: 100, + seedLo: 200, + split: [0.5, 0.5], + trafficSeedHi: 1, + trafficSeedLo: 2, + trafficSplit: [0, 1], + fullOnVariant: 2, + applications: [{ name: "website" }], + variants: [ + { name: "A", config: null }, + { name: "B", config: null }, + { name: "C", config: null }, + ], + audience: null, + audienceStrict: false, + customFieldValues: null, + holdoutIds: [11], + }, + ], + [ + { + id: 11, + name: "holdout_a", + iteration: 1, + unitType: "session_id", + seedHi: 13, + seedLo: 111, + split: [0.1, 0.9], + trafficSeedHi: 0, + trafficSeedLo: 0, + trafficSplit: [0, 1], + fullOnVariant: 0, + applications: [], + variants: [ + { name: "A", config: null }, + { name: "B", config: null }, + ], + audience: null, + audienceStrict: false, + customFieldValues: null, + holdoutType: "full", + }, + ] + ); + + const context = new Context(sdk, contextOptions, contextParams, response); + expect(context.treatment("exp_holdout_fullon")).toEqual(0); + + publisher.publish.mockReturnValue(Promise.resolve()); + + context.publish().then(() => { + expect(publisher.publish.mock.calls[0][0].exposures).toEqual([ + { + id: 11, + name: "holdout_a", + unit: "session_id", + exposedAt: timeOrigin, + variant: 0, + assigned: true, + eligible: true, + overridden: false, + fullOn: false, + custom: false, + audienceMismatch: false, + ruleOverride: false, + }, + ]); + done(); + }); + }); + + // Scenario 210 (index 209): an explicit override wins over holdout membership for the + // experiment's own assignment; the override path still evaluates coverage, so the holdout's + // own exposure fires alongside it. + it("lets override win the variant while the holdout still fires its own exposure (scenario 210)", (done) => { + const response = buildHoldoutResponse( + [ + { + id: 1, + name: "exp_holdout_override", + iteration: 1, + unitType: "session_id", + seedHi: 3603515, + seedLo: 233373850, + split: [0.5, 0.5], + trafficSeedHi: 449867249, + trafficSeedLo: 455443629, + trafficSplit: [0, 1], + fullOnVariant: 0, + applications: [{ name: "website" }], + variants: [ + { name: "A", config: null }, + { name: "B", config: null }, + ], + audience: null, + audienceStrict: false, + customFieldValues: null, + holdoutIds: [1], + }, + ], + [ + { + id: 1, + name: "holdout_override_precedence", + iteration: 1, + unitType: "session_id", + seedHi: 13, + seedLo: 111, + split: [0.1, 0.9], + }, + ] + ); + + const context = new Context(sdk, contextOptions, contextParams, response); + context.override("exp_holdout_override", 1); + expect(context.treatment("exp_holdout_override")).toEqual(1); + + publisher.publish.mockReturnValue(Promise.resolve()); + + context.publish().then(() => { + expect(publisher.publish.mock.calls[0][0].exposures).toEqual([ + { + id: 1, + name: "exp_holdout_override", + unit: "session_id", + exposedAt: timeOrigin, + variant: 1, + assigned: false, + eligible: true, + overridden: true, + fullOn: false, + custom: false, + audienceMismatch: false, + ruleOverride: false, + }, + { + id: 1, + name: "holdout_override_precedence", + unit: "session_id", + exposedAt: timeOrigin, + variant: 0, + assigned: true, + eligible: true, + overridden: false, + fullOn: false, + custom: false, + audienceMismatch: false, + ruleOverride: false, + }, + ]); + done(); + }); + }); + + // Scenario 211 (index 210): holdout membership forces control and suppresses the + // experiment's own exposure even with a custom assignment on file; only the holdout's own + // exposure fires. + it("lets holdout suppression beat a custom assignment (scenario 211)", (done) => { + const response = buildHoldoutResponse( + [ + { + id: 1, + name: "exp_holdout_custom", + iteration: 1, + unitType: "session_id", + seedHi: 3603515, + seedLo: 233373850, + split: [0.5, 0.5], + trafficSeedHi: 449867249, + trafficSeedLo: 455443629, + trafficSplit: [0, 1], + fullOnVariant: 0, + applications: [{ name: "website" }], + variants: [ + { name: "A", config: null }, + { name: "B", config: null }, + ], + audience: null, + audienceStrict: false, + customFieldValues: null, + holdoutIds: [1], + }, + ], + [ + { + id: 1, + name: "holdout_custom_precedence", + iteration: 1, + unitType: "session_id", + seedHi: 13, + seedLo: 111, + split: [0.1, 0.9], + }, + ] + ); + + const context = new Context(sdk, contextOptions, contextParams, response); + context.customAssignment("exp_holdout_custom", 1); + expect(context.treatment("exp_holdout_custom")).toEqual(0); + + publisher.publish.mockReturnValue(Promise.resolve()); + + context.publish().then(() => { + expect(publisher.publish.mock.calls[0][0].exposures).toEqual([ + { + id: 1, + name: "holdout_custom_precedence", + unit: "session_id", + exposedAt: timeOrigin, + variant: 0, + assigned: true, + eligible: true, + overridden: false, + fullOn: false, + custom: false, + audienceMismatch: false, + ruleOverride: false, + }, + ]); + done(); + }); + }); + + // Scenario 213 (index 212): the same unit and shared holdout definition produce consistent + // holdout membership across two different covered experiments; the shared holdout's own + // exposure fires once on first evaluation and neither covered experiment fires its own + // exposure — the second call must fire nothing at all, not even a duplicate holdout exposure. + it("fires a shared holdout's exposure once across two covered experiments (scenario 213)", () => { + const response = buildHoldoutResponse( + [ + { + id: 1, + name: "exp_shared_a", + iteration: 1, + unitType: "session_id", + seedHi: 3603515, + seedLo: 233373850, + split: [0.5, 0.5], + trafficSeedHi: 449867249, + trafficSeedLo: 455443629, + trafficSplit: [0, 1], + fullOnVariant: 0, + applications: [{ name: "website" }], + variants: [ + { name: "A", config: null }, + { name: "B", config: null }, + ], + audience: null, + audienceStrict: false, + customFieldValues: null, + holdoutIds: [1], + }, + { + id: 2, + name: "exp_shared_b", + iteration: 1, + unitType: "session_id", + seedHi: 3603515, + seedLo: 233373850, + split: [0.5, 0.5], + trafficSeedHi: 449867249, + trafficSeedLo: 455443629, + trafficSplit: [0, 1], + fullOnVariant: 0, + applications: [{ name: "website" }], + variants: [ + { name: "A", config: null }, + { name: "B", config: null }, + ], + audience: null, + audienceStrict: false, + customFieldValues: null, + holdoutIds: [1], + }, + ], + [ + { + id: 1, + name: "holdout_shared", + iteration: 1, + unitType: "session_id", + seedHi: 13, + seedLo: 111, + split: [0.1, 0.9], + }, + ] + ); + + const context = new Context(sdk, contextOptions, contextParams, response); + + expect(context.treatment("exp_shared_a")).toEqual(0); + expect(publisher.publish.mock.calls.length).toEqual(0); // nothing published yet; check pending directly + expect(context.pending()).toEqual(1); + + expect(context.treatment("exp_shared_b")).toEqual(0); + // exp_shared_b is also suppressed, and the shared holdout already fired for exp_shared_a — + // no further exposures of any kind should be queued. + expect(context.pending()).toEqual(1); + }); + + // Scenarios 214-220 (indices 213-219): the three-arm holdout battery. Same shared unit + // (e791e240fcd3df7d238cfc285f475e8152fcc0ec) throughout; each scenario pins a distinct arm + // semantic. + describe("holdout arms battery", () => { + // Scenario 214 (index 213): a 3-arm holdout's variant 0 forces control for every covered + // experiment, full-on or not, identically to a 2-arm holdout's variant 0; the holdout's + // own exposure fires once at variant 0 and neither covered experiment emits its own + // exposure. + it("3-arm variant 0 holds out both full-on and non-full-on covered experiments (scenario 214)", () => { + const response = buildHoldoutResponse( + [ + { + id: 1, + name: "exp_three_arm_0_non_fullon", + iteration: 1, + unitType: "session_id", + seedHi: 100, + seedLo: 200, + split: [0.5, 0.5], + trafficSeedHi: 1, + trafficSeedLo: 2, + trafficSplit: [0.0, 1.0], + fullOnVariant: 0, + applications: [{ name: "website" }], + variants: [ + { name: "A", config: null }, + { name: "B", config: null }, + ], + audience: null, + audienceStrict: false, + customFieldValues: null, + holdoutIds: [21], + }, + { + id: 2, + name: "exp_three_arm_0_fullon", + iteration: 1, + unitType: "session_id", + seedHi: 100, + seedLo: 200, + split: [0.5, 0.5], + trafficSeedHi: 1, + trafficSeedLo: 2, + trafficSplit: [0.0, 1.0], + fullOnVariant: 2, + applications: [{ name: "website" }], + variants: [ + { name: "A", config: null }, + { name: "B", config: null }, + { name: "C", config: null }, + ], + audience: null, + audienceStrict: false, + customFieldValues: null, + holdoutIds: [21], + }, + ], + [ + { + id: 21, + name: "holdout_three_arm_v0", + iteration: 1, + unitType: "session_id", + seedHi: 0, + seedLo: 1, + split: [0.3, 0.3, 0.4], + holdoutType: "all_full_on", + }, + ] + ); + + const context = new Context(sdk, contextOptions, contextParams, response); + + expect(context.treatment("exp_three_arm_0_non_fullon")).toEqual(0); + expect(context.pending()).toEqual(1); + + expect(context.treatment("exp_three_arm_0_fullon")).toEqual(0); + expect(context.pending()).toEqual(1); + }); + + // Scenario 215 (index 214): a 3-arm holdout's variant 1 forces control only for a covered + // experiment with fullOnVariant==0; a covered full-on experiment (fullOnVariant!=0) takes + // its normal assignment path and is assigned its own fullOnVariant with fullOn=true, not + // suppressed. + it("3-arm variant 1 holds out only the non-full-on experiment (scenario 215)", (done) => { + const response = buildHoldoutResponse( + [ + { + id: 1, + name: "exp_three_arm_1_non_fullon", + iteration: 1, + unitType: "session_id", + seedHi: 100, + seedLo: 200, + split: [0.5, 0.5], + trafficSeedHi: 1, + trafficSeedLo: 2, + trafficSplit: [0.0, 1.0], + fullOnVariant: 0, + applications: [{ name: "website" }], + variants: [ + { name: "A", config: null }, + { name: "B", config: null }, + ], + audience: null, + audienceStrict: false, + customFieldValues: null, + holdoutIds: [22], + }, + { + id: 2, + name: "exp_three_arm_1_fullon", + iteration: 1, + unitType: "session_id", + seedHi: 100, + seedLo: 200, + split: [0.5, 0.5], + trafficSeedHi: 1, + trafficSeedLo: 2, + trafficSplit: [0.0, 1.0], + fullOnVariant: 2, + applications: [{ name: "website" }], + variants: [ + { name: "A", config: null }, + { name: "B", config: null }, + { name: "C", config: null }, + ], + audience: null, + audienceStrict: false, + customFieldValues: null, + holdoutIds: [22], + }, + ], + [ + { + id: 22, + name: "holdout_three_arm_v1", + iteration: 1, + unitType: "session_id", + seedHi: 0, + seedLo: 3, + split: [0.3, 0.3, 0.4], + holdoutType: "all_full_on", + }, + ] + ); + + const context = new Context(sdk, contextOptions, contextParams, response); + expect(context.treatment("exp_three_arm_1_non_fullon")).toEqual(0); + expect(context.treatment("exp_three_arm_1_fullon")).toEqual(2); + + publisher.publish.mockReturnValue(Promise.resolve()); + + context.publish().then(() => { + expect(publisher.publish.mock.calls[0][0].exposures).toEqual([ + { + id: 22, + name: "holdout_three_arm_v1", + unit: "session_id", + exposedAt: timeOrigin, + variant: 1, + assigned: true, + eligible: true, + overridden: false, + fullOn: false, + custom: false, + audienceMismatch: false, + ruleOverride: false, + }, + { + id: 2, + name: "exp_three_arm_1_fullon", + unit: "session_id", + exposedAt: timeOrigin, + variant: 2, + assigned: true, + eligible: true, + overridden: false, + fullOn: true, + custom: false, + audienceMismatch: false, + ruleOverride: false, + }, + ]); + done(); + }); + }); + + // Scenario 216 (index 215): a 3-arm holdout's variant 2 (normal traffic) evaluates every + // covered experiment exactly as if uncovered, full-on or not; the holdout's own exposure + // fires once at variant 2. + it("3-arm variant 2 defers to the normal path for every covered experiment (scenario 216)", (done) => { + const response = buildHoldoutResponse( + [ + { + id: 1, + name: "exp_three_arm_2_non_fullon", + iteration: 1, + unitType: "session_id", + seedHi: 100, + seedLo: 200, + split: [0.5, 0.5], + trafficSeedHi: 1, + trafficSeedLo: 2, + trafficSplit: [0.0, 1.0], + fullOnVariant: 0, + applications: [{ name: "website" }], + variants: [ + { name: "A", config: null }, + { name: "B", config: null }, + ], + audience: null, + audienceStrict: false, + customFieldValues: null, + holdoutIds: [23], + }, + { + id: 2, + name: "exp_three_arm_2_fullon", + iteration: 1, + unitType: "session_id", + seedHi: 100, + seedLo: 200, + split: [0.5, 0.5], + trafficSeedHi: 1, + trafficSeedLo: 2, + trafficSplit: [0.0, 1.0], + fullOnVariant: 2, + applications: [{ name: "website" }], + variants: [ + { name: "A", config: null }, + { name: "B", config: null }, + { name: "C", config: null }, + ], + audience: null, + audienceStrict: false, + customFieldValues: null, + holdoutIds: [23], + }, + ], + [ + { + id: 23, + name: "holdout_three_arm_v2", + iteration: 1, + unitType: "session_id", + seedHi: 0, + seedLo: 0, + split: [0.3, 0.3, 0.4], + holdoutType: "all_full_on", + }, + ] + ); + + const context = new Context(sdk, contextOptions, contextParams, response); + expect(context.treatment("exp_three_arm_2_non_fullon")).toEqual(1); + expect(context.treatment("exp_three_arm_2_fullon")).toEqual(2); + + publisher.publish.mockReturnValue(Promise.resolve()); + + context.publish().then(() => { + expect(publisher.publish.mock.calls[0][0].exposures).toEqual([ + { + id: 1, + name: "exp_three_arm_2_non_fullon", + unit: "session_id", + exposedAt: timeOrigin, + variant: 1, + assigned: true, + eligible: true, + overridden: false, + fullOn: false, + custom: false, + audienceMismatch: false, + ruleOverride: false, + }, + { + id: 23, + name: "holdout_three_arm_v2", + unit: "session_id", + exposedAt: timeOrigin, + variant: 2, + assigned: true, + eligible: true, + overridden: false, + fullOn: false, + custom: false, + audienceMismatch: false, + ruleOverride: false, + }, + { + id: 2, + name: "exp_three_arm_2_fullon", + unit: "session_id", + exposedAt: timeOrigin, + variant: 2, + assigned: true, + eligible: true, + overridden: false, + fullOn: true, + custom: false, + audienceMismatch: false, + ruleOverride: false, + }, + ]); + done(); + }); + }); + + // Scenario 217 (index 216): a full-on experiment covered by a 3-arm holdout's variant 1 is + // not short-circuited to its fullOnVariant: audienceStrict is evaluated first, so an + // audience mismatch still forces control (not suppression) with assigned=false and + // audienceMismatch=true; the experiment's own exposure fires because it was evaluated and + // rejected by audience, not held out. + it("3-arm variant 1 defers to the normal path for an audience mismatch (scenario 217)", (done) => { + const response = buildHoldoutResponse( + [ + { + id: 1, + name: "exp_three_arm_1_audience_fullon", + iteration: 1, + unitType: "session_id", + seedHi: 100, + seedLo: 200, + split: [0.5, 0.5], + trafficSeedHi: 1, + trafficSeedLo: 2, + trafficSplit: [0.0, 1.0], + fullOnVariant: 2, + applications: [{ name: "website" }], + variants: [ + { name: "A", config: null }, + { name: "B", config: null }, + { name: "C", config: null }, + ], + audience: JSON.stringify({ filter: [{ gte: [{ var: "age" }, { value: 20 }] }] }), + audienceStrict: true, + customFieldValues: null, + holdoutIds: [24], + }, + ], + [ + { + id: 24, + name: "holdout_three_arm_v1", + iteration: 1, + unitType: "session_id", + seedHi: 0, + seedLo: 3, + split: [0.3, 0.3, 0.4], + holdoutType: "all_full_on", + }, + ] + ); + + const context = new Context(sdk, contextOptions, contextParams, response); + context.attribute("age", 5); + expect(context.treatment("exp_three_arm_1_audience_fullon")).toEqual(0); + + publisher.publish.mockReturnValue(Promise.resolve()); + + context.publish().then(() => { + expect(publisher.publish.mock.calls[0][0].exposures).toEqual([ + { + id: 1, + name: "exp_three_arm_1_audience_fullon", + unit: "session_id", + exposedAt: timeOrigin, + variant: 0, + assigned: false, + eligible: true, + overridden: false, + fullOn: false, + custom: false, + audienceMismatch: true, + ruleOverride: false, + }, + { + id: 24, + name: "holdout_three_arm_v1", + unit: "session_id", + exposedAt: timeOrigin, + variant: 1, + assigned: true, + eligible: true, + overridden: false, + fullOn: false, + custom: false, + audienceMismatch: false, + ruleOverride: false, + }, + ]); + done(); + }); + }); + + // Scenario 218 (index 217): the same full-on/audience-mismatch experiment, covered + // instead by the 3-arm holdout's variant 2 (normal traffic), yields the identical verdict + // as scenario 217's variant 1: control, assigned=false, audienceMismatch=true. Pins that + // arm 1 defers to the normal assignment path instead of short-circuiting to the full-on + // variant. + it("3-arm variant 2 reaches the identical audience-mismatch verdict as variant 1 (scenario 218)", (done) => { + const response = buildHoldoutResponse( + [ + { + id: 1, + name: "exp_three_arm_2_audience_fullon", + iteration: 1, + unitType: "session_id", + seedHi: 100, + seedLo: 200, + split: [0.5, 0.5], + trafficSeedHi: 1, + trafficSeedLo: 2, + trafficSplit: [0.0, 1.0], + fullOnVariant: 2, + applications: [{ name: "website" }], + variants: [ + { name: "A", config: null }, + { name: "B", config: null }, + { name: "C", config: null }, + ], + audience: JSON.stringify({ filter: [{ gte: [{ var: "age" }, { value: 20 }] }] }), + audienceStrict: true, + customFieldValues: null, + holdoutIds: [25], + }, + ], + [ + { + id: 25, + name: "holdout_three_arm_v2", + iteration: 1, + unitType: "session_id", + seedHi: 0, + seedLo: 0, + split: [0.3, 0.3, 0.4], + holdoutType: "all_full_on", + }, + ] + ); + + const context = new Context(sdk, contextOptions, contextParams, response); + context.attribute("age", 5); + expect(context.treatment("exp_three_arm_2_audience_fullon")).toEqual(0); + + publisher.publish.mockReturnValue(Promise.resolve()); + + context.publish().then(() => { + expect(publisher.publish.mock.calls[0][0].exposures).toEqual([ + { + id: 1, + name: "exp_three_arm_2_audience_fullon", + unit: "session_id", + exposedAt: timeOrigin, + variant: 0, + assigned: false, + eligible: true, + overridden: false, + fullOn: false, + custom: false, + audienceMismatch: true, + ruleOverride: false, + }, + { + id: 25, + name: "holdout_three_arm_v2", + unit: "session_id", + exposedAt: timeOrigin, + variant: 2, + assigned: true, + eligible: true, + overridden: false, + fullOn: false, + custom: false, + audienceMismatch: false, + ruleOverride: false, + }, + ]); + done(); + }); + }); + + // Scenario 219 (index 218): a covered experiment is suppressed if EITHER applicable + // holdout holds it out, even when a 3-arm holdout's own arm (variant 2, normal traffic) + // would not have held it out on its own; both holdouts still emit their own independent + // exposure with their own variant. A second experiment covered only by the + // non-suppressing 3-arm holdout takes its normal assignment path and emits its own + // exposure, proving that holdout 22's arm is not itself suppressing. + it("union: a 2-arm holdout holds out while the applicable 3-arm holdout does not (scenario 219)", (done) => { + const response = buildHoldoutResponse( + [ + { + id: 1, + name: "exp_union_two_arm_wins", + iteration: 1, + unitType: "session_id", + seedHi: 100, + seedLo: 200, + split: [0.5, 0.5], + trafficSeedHi: 1, + trafficSeedLo: 2, + trafficSplit: [0.0, 1.0], + fullOnVariant: 0, + applications: [{ name: "website" }], + variants: [ + { name: "A", config: null }, + { name: "B", config: null }, + ], + audience: null, + audienceStrict: false, + customFieldValues: null, + holdoutIds: [11, 22], + }, + { + id: 2, + name: "exp_only_three_arm_normal_path", + iteration: 1, + unitType: "session_id", + seedHi: 100, + seedLo: 200, + split: [0.5, 0.5], + trafficSeedHi: 1, + trafficSeedLo: 2, + trafficSplit: [0.0, 1.0], + fullOnVariant: 0, + applications: [{ name: "website" }], + variants: [ + { name: "A", config: null }, + { name: "B", config: null }, + ], + audience: null, + audienceStrict: false, + customFieldValues: null, + holdoutIds: [22], + }, + ], + [ + { + id: 11, + name: "holdout_union_two_arm", + iteration: 1, + unitType: "session_id", + seedHi: 13, + seedLo: 111, + split: [0.1, 0.9], + }, + { + id: 22, + name: "holdout_union_three_arm", + iteration: 1, + unitType: "session_id", + seedHi: 0, + seedLo: 0, + split: [0.3, 0.3, 0.4], + holdoutType: "all_full_on", + }, + ] + ); + + const context = new Context(sdk, contextOptions, contextParams, response); + expect(context.treatment("exp_union_two_arm_wins")).toEqual(0); + expect(context.treatment("exp_only_three_arm_normal_path")).toEqual(1); + + publisher.publish.mockReturnValue(Promise.resolve()); + + context.publish().then(() => { + expect(publisher.publish.mock.calls[0][0].exposures).toEqual([ + { + id: 11, + name: "holdout_union_two_arm", + unit: "session_id", + exposedAt: timeOrigin, + variant: 0, + assigned: true, + eligible: true, + overridden: false, + fullOn: false, + custom: false, + audienceMismatch: false, + ruleOverride: false, + }, + { + id: 22, + name: "holdout_union_three_arm", + unit: "session_id", + exposedAt: timeOrigin, + variant: 2, + assigned: true, + eligible: true, + overridden: false, + fullOn: false, + custom: false, + audienceMismatch: false, + ruleOverride: false, + }, + { + id: 2, + name: "exp_only_three_arm_normal_path", + unit: "session_id", + exposedAt: timeOrigin, + variant: 1, + assigned: true, + eligible: true, + overridden: false, + fullOn: false, + custom: false, + audienceMismatch: false, + ruleOverride: false, + }, + ]); + done(); + }); + }); + + // Scenario 220 (index 219): union, the other direction: a 3-arm holdout's variant 1 holds + // a non-full-on covered experiment out even when an applicable 2-arm holdout alone would + // not have; both holdouts still emit their own independent exposure with their own + // variant. A second experiment covered only by the non-suppressing 2-arm holdout takes its + // normal assignment path and emits its own exposure, proving that holdout 11's arm is not + // itself suppressing. + it("union: a 3-arm holdout holds out while the applicable 2-arm holdout does not (scenario 220)", (done) => { + const response = buildHoldoutResponse( + [ + { + id: 1, + name: "exp_union_three_arm_wins", + iteration: 1, + unitType: "session_id", + seedHi: 100, + seedLo: 200, + split: [0.5, 0.5], + trafficSeedHi: 1, + trafficSeedLo: 2, + trafficSplit: [0.0, 1.0], + fullOnVariant: 0, + applications: [{ name: "website" }], + variants: [ + { name: "A", config: null }, + { name: "B", config: null }, + ], + audience: null, + audienceStrict: false, + customFieldValues: null, + holdoutIds: [11, 22], + }, + { + id: 2, + name: "exp_only_two_arm_normal_path", + iteration: 1, + unitType: "session_id", + seedHi: 100, + seedLo: 200, + split: [0.5, 0.5], + trafficSeedHi: 1, + trafficSeedLo: 2, + trafficSplit: [0.0, 1.0], + fullOnVariant: 0, + applications: [{ name: "website" }], + variants: [ + { name: "A", config: null }, + { name: "B", config: null }, + ], + audience: null, + audienceStrict: false, + customFieldValues: null, + holdoutIds: [11], + }, + ], + [ + { + id: 11, + name: "holdout_union_two_arm", + iteration: 1, + unitType: "session_id", + seedHi: 1, + seedLo: 222, + split: [0.1, 0.9], + }, + { + id: 22, + name: "holdout_union_three_arm", + iteration: 1, + unitType: "session_id", + seedHi: 0, + seedLo: 3, + split: [0.3, 0.3, 0.4], + holdoutType: "all_full_on", + }, + ] + ); + + const context = new Context(sdk, contextOptions, contextParams, response); + expect(context.treatment("exp_union_three_arm_wins")).toEqual(0); + expect(context.treatment("exp_only_two_arm_normal_path")).toEqual(1); + + publisher.publish.mockReturnValue(Promise.resolve()); + + context.publish().then(() => { + expect(publisher.publish.mock.calls[0][0].exposures).toEqual([ + { + id: 11, + name: "holdout_union_two_arm", + unit: "session_id", + exposedAt: timeOrigin, + variant: 1, + assigned: true, + eligible: true, + overridden: false, + fullOn: false, + custom: false, + audienceMismatch: false, + ruleOverride: false, + }, + { + id: 22, + name: "holdout_union_three_arm", + unit: "session_id", + exposedAt: timeOrigin, + variant: 1, + assigned: true, + eligible: true, + overridden: false, + fullOn: false, + custom: false, + audienceMismatch: false, + ruleOverride: false, + }, + { + id: 2, + name: "exp_only_two_arm_normal_path", + unit: "session_id", + exposedAt: timeOrigin, + variant: 1, + assigned: true, + eligible: true, + overridden: false, + fullOn: false, + custom: false, + audienceMismatch: false, + ruleOverride: false, + }, + ]); + done(); + }); + }); + }); + + // Scenario 221 (index 220): a covered full-on experiment can resolve before its unit type is + // installed; after unit() supplies that unit, resolving it again publishes the holdout + // exposure that was unavailable during the first evaluation. This is a regression test for + // the Task 6 fix round (C-1): a real bug existed here (the holdout exposure was permanently + // lost) and was fixed via _invalidateAssignmentsPinnedWithMissingUnit plus a change to + // _unitHash's negative-result caching. + it("publishes the previously-unavailable holdout exposure once its late unit is set (scenario 221)", (done) => { + const response = buildHoldoutResponse( + [ + { + id: 1, + name: "exp_holdout_late_unit_fullon", + iteration: 1, + unitType: "user_id", + seedHi: 100, + seedLo: 200, + split: [0.5, 0.5, 0.0], + trafficSeedHi: 1, + trafficSeedLo: 2, + trafficSplit: [0, 1], + fullOnVariant: 2, + applications: [{ name: "website" }], + variants: [ + { name: "A", config: null }, + { name: "B", config: null }, + { name: "C", config: null }, + ], + audience: null, + audienceStrict: false, + customFieldValues: null, + holdoutIds: [11], + }, + ], + [ + { + id: 11, + name: "holdout_late_user_id", + iteration: 1, + unitType: "user_id", + seedHi: 1, + seedLo: 222, + split: [0.1, 0.9], + trafficSeedHi: 0, + trafficSeedLo: 0, + trafficSplit: [0, 1], + fullOnVariant: 0, + applications: [], + variants: [ + { name: "A", config: null }, + { name: "B", config: null }, + ], + audience: null, + audienceStrict: false, + customFieldValues: null, + holdoutType: "full", + }, + ] + ); + + const lateUnitParams = { units: { session_id: "context-created-without-user-id" } }; + const context = new Context(sdk, contextOptions, lateUnitParams, response); + + // Before the covered unit type is installed: resolves via the full-on path (no unit + // needed for that), the holdout can't be resolved yet, and peek() fires no exposures. + expect(context.peek("exp_holdout_late_unit_fullon")).toEqual(2); + expect(context.pending()).toEqual(0); + + context.unit("user_id", "e791e240fcd3df7d238cfc285f475e8152fcc0ec"); + + expect(context.treatment("exp_holdout_late_unit_fullon")).toEqual(2); + + publisher.publish.mockReturnValue(Promise.resolve()); + + context.publish().then(() => { + expect(publisher.publish.mock.calls[0][0].exposures).toEqual([ + { + id: 1, + name: "exp_holdout_late_unit_fullon", + unit: "user_id", + exposedAt: timeOrigin, + variant: 2, + assigned: true, + eligible: true, + overridden: false, + fullOn: true, + custom: false, + audienceMismatch: false, + ruleOverride: false, + }, + { + id: 11, + name: "holdout_late_user_id", + unit: "user_id", + exposedAt: timeOrigin, + variant: 1, + assigned: true, + eligible: true, + overridden: false, + fullOn: false, + custom: false, + audienceMismatch: false, + ruleOverride: false, + }, + ]); + done(); + }); + }); + + // Scenario 222 (index 221): identical to scenario 214 (three-arm variant 0), but the holdout + // definition omits the holdoutType field entirely. An SDK must derive the holdout's arm count + // from its split length alone, so suppression of both the full-on and non-full-on covered + // experiments is unchanged. Pins that holdoutType is not required on the wire and arm count + // is never read from it. + it("derives 3-arm arity from split length alone, with no holdoutType field on the wire (scenario 222)", (done) => { + const response = buildHoldoutResponse( + [ + { + id: 1, + name: "exp_three_arm_0_non_fullon", + iteration: 1, + unitType: "session_id", + seedHi: 100, + seedLo: 200, + split: [0.5, 0.5], + trafficSeedHi: 1, + trafficSeedLo: 2, + trafficSplit: [0.0, 1.0], + fullOnVariant: 0, + applications: [{ name: "website" }], + variants: [ + { name: "A", config: null }, + { name: "B", config: null }, + ], + audience: null, + audienceStrict: false, + customFieldValues: null, + holdoutIds: [21], + }, + { + id: 2, + name: "exp_three_arm_0_fullon", + iteration: 1, + unitType: "session_id", + seedHi: 100, + seedLo: 200, + split: [0.5, 0.5], + trafficSeedHi: 1, + trafficSeedLo: 2, + trafficSplit: [0.0, 1.0], + fullOnVariant: 2, + applications: [{ name: "website" }], + variants: [ + { name: "A", config: null }, + { name: "B", config: null }, + { name: "C", config: null }, + ], + audience: null, + audienceStrict: false, + customFieldValues: null, + holdoutIds: [21], + }, + ], + [ + { + id: 21, + name: "holdout_arity_from_split", + iteration: 1, + unitType: "session_id", + seedHi: 0, + seedLo: 1, + split: [0.3, 0.3, 0.4], + // deliberately no holdoutType field + }, + ] + ); + + const context = new Context(sdk, contextOptions, contextParams, response); + expect(context.treatment("exp_three_arm_0_non_fullon")).toEqual(0); + expect(context.treatment("exp_three_arm_0_fullon")).toEqual(0); + + publisher.publish.mockReturnValue(Promise.resolve()); + + context.publish().then(() => { + expect(publisher.publish.mock.calls[0][0].exposures).toEqual([ + { + id: 21, + name: "holdout_arity_from_split", + unit: "session_id", + exposedAt: timeOrigin, + variant: 0, + assigned: true, + eligible: true, + overridden: false, + fullOn: false, + custom: false, + audienceMismatch: false, + ruleOverride: false, + }, + ]); + done(); + }); + }); + }); + describe("variableValue()", () => { it("should not return variable values when unassigned", (done) => { const context = new Context(sdk, contextOptions, contextParams, audienceStrictContextResponse); From 0f4cffdba63175d9a47505a801bb50ba4dfc2eaf Mon Sep 17 00:00:00 2001 From: Jonas Alves Date: Thu, 10 Sep 2026 11:03:44 +0100 Subject: [PATCH 10/28] fix(holdouts): suppress rule-variant path, add Task 5 regression tests, tighten test assertions (FT-2206) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Final whole-branch review fix wave for feat/holdouts, addressing all findings in a single pass: - I-1 (Important): assignmentRules (JS-SDK-only) bypassed holdout suppression in _assign() — a matching rule set assignment.variant/ ruleOverride unconditionally, before the suppressed check, so a held-out unit was silently TREATED with the rule's variant while its exposure-firing gate (which doesn't check ruleOverride) still suppressed its own exposure. Restructured so suppression is checked first, consistent with scenario 211's precedent (custom assignment yields to suppression) - assignment rules are a deterministic per-attribute assignment mechanism, not an override in the scenario 210 sense. ruleKey bookkeeping is kept unconditional to avoid thrashing audienceMatches()'s cache-validity fast path. - I-2 (Important): added regression tests for both of Task 5's bug fixes (commit 716430c), which previously had zero coverage - override fast-path holdout-set revalidation, and same-iteration holdout arm-count pinning. Both verified to fail when their corresponding fix is reverted. - M-3 (Minor): scenario 213 (shared-holdout dedup) now asserts the exact exposures array instead of just a pending() count. - M-4 (Minor): scenario 207 (dangling holdout id) no longer wraps async .then()-internal assertions inside .not.toThrow(), which could let failures escape as unhandled rejections instead of clean failures. 400 -> 403 tests. tsc --noEmit, lint, and format:check all clean. 🤖 Generated with Claude Code Co-Authored-By: Claude --- src/__tests__/context.test.js | 449 ++++++++++++++++++++++++++++++++-- src/context.ts | 149 ++++++----- 2 files changed, 509 insertions(+), 89 deletions(-) diff --git a/src/__tests__/context.test.js b/src/__tests__/context.test.js index 2c7f520..8bf4528 100644 --- a/src/__tests__/context.test.js +++ b/src/__tests__/context.test.js @@ -3369,32 +3369,39 @@ describe("Context", () => { ] ); - expect(() => { - const context = new Context(sdk, contextOptions, contextParams, response); - expect(context.treatment("exp_holdout_dangling_plus_valid")).toEqual(0); + // Fixed per final-review finding M-4: the construction and the async publish + // assertions are split apart. `.not.toThrow()` only catches SYNCHRONOUS throws, so + // wrapping the `.then()` callback (with its `done()` call) inside it let any assertion + // failure inside the callback escape as an unhandled rejection / test timeout instead + // of a clean, attributable test failure. The synchronous construction is asserted not + // to throw on its own, and the publish/exposure assertions run unwrapped afterward, + // matching how every other test in this block is structured. + expect(() => new Context(sdk, contextOptions, contextParams, response)).not.toThrow(); - publisher.publish.mockReturnValue(Promise.resolve()); + const context = new Context(sdk, contextOptions, contextParams, response); + expect(context.treatment("exp_holdout_dangling_plus_valid")).toEqual(0); - context.publish().then(() => { - expect(publisher.publish.mock.calls[0][0].exposures).toEqual([ - { - id: 11, - name: "holdout_a", - unit: "session_id", - exposedAt: timeOrigin, - variant: 0, - assigned: true, - eligible: true, - overridden: false, - fullOn: false, - custom: false, - audienceMismatch: false, - ruleOverride: false, - }, - ]); - done(); - }); - }).not.toThrow(); + publisher.publish.mockReturnValue(Promise.resolve()); + + context.publish().then(() => { + expect(publisher.publish.mock.calls[0][0].exposures).toEqual([ + { + id: 11, + name: "holdout_a", + unit: "session_id", + exposedAt: timeOrigin, + variant: 0, + assigned: true, + eligible: true, + overridden: false, + fullOn: false, + custom: false, + audienceMismatch: false, + ruleOverride: false, + }, + ]); + done(); + }); }); // Scenario 208 (index 207): a holdout suppresses a full-on experiment (fullOnVariant != 0) @@ -3635,7 +3642,7 @@ describe("Context", () => { // holdout membership across two different covered experiments; the shared holdout's own // exposure fires once on first evaluation and neither covered experiment fires its own // exposure — the second call must fire nothing at all, not even a duplicate holdout exposure. - it("fires a shared holdout's exposure once across two covered experiments (scenario 213)", () => { + it("fires a shared holdout's exposure once across two covered experiments (scenario 213)", (done) => { const response = buildHoldoutResponse( [ { @@ -3706,6 +3713,32 @@ describe("Context", () => { // exp_shared_b is also suppressed, and the shared holdout already fired for exp_shared_a — // no further exposures of any kind should be queued. expect(context.pending()).toEqual(1); + + publisher.publish.mockReturnValue(Promise.resolve()); + + // Tightened per final-review finding M-3: assert the exact exposures array (matching + // every other test in this describe block) so this verifies the SPECIFIC exposure that + // fired is the shared holdout's own — not merely that exactly one exposure fired, which + // would pass even if the wrong exposure (e.g. exp_shared_a's own) had fired instead. + context.publish().then(() => { + expect(publisher.publish.mock.calls[0][0].exposures).toEqual([ + { + id: 1, + name: "holdout_shared", + unit: "session_id", + exposedAt: timeOrigin, + variant: 0, + assigned: true, + eligible: true, + overridden: false, + fullOn: false, + custom: false, + audienceMismatch: false, + ruleOverride: false, + }, + ]); + done(); + }); }); // Scenarios 214-220 (indices 213-219): the three-arm holdout battery. Same shared unit @@ -4658,6 +4691,372 @@ describe("Context", () => { done(); }); }); + + // Final-review Finding I-1: a matching assignmentRules rule (a JS-SDK-only feature) must + // NOT bypass holdout suppression. Rules are a deterministic-per-attribute assignment + // mechanism — structurally the same category as a custom assignment (scenario 211: custom + // assignment yields to suppression) — not an override in the sense scenario 210 + // establishes (only an explicit override() call is exempt from suppression). Before the + // fix, `ruleVariant !== null` set `assignment.variant`/`ruleOverride` unconditionally, + // bypassing the `if (assignment.suppressed)` branch entirely: a held-out unit would be + // silently TREATED with the rule's variant while its own exposure stayed suppressed (the + // exposure gate does not special-case `ruleOverride`) — measured nothing, but received + // real treatment. Ported shape from scenario 211 (custom-assignment path) applied to the + // rules path instead; the holdout fixture (id 11, seedHi 13, seedLo 111, split [0.1, 0.9]) + // is identical to scenario 203's holdout_a, which is already proven to hold out the + // default `contextParams` unit (variant 0). + it("suppresses a matching assignment rule's variant, mirroring scenario 211 for the rules path (Finding I-1)", (done) => { + const response = buildHoldoutResponse( + [ + { + id: 1, + name: "exp_holdout_rules", + iteration: 1, + unitType: "session_id", + seedHi: 100, + seedLo: 200, + split: [0.5, 0.5], + trafficSeedHi: 1, + trafficSeedLo: 2, + trafficSplit: [0, 1], + fullOnVariant: 0, + applications: [{ name: "website" }], + variants: [ + { name: "A", config: null }, + { name: "B", config: null }, + ], + audience: null, + audienceStrict: false, + assignmentRules: JSON.stringify({ + rules: [ + { + name: "US Internal Users", + type: "assign", + conditions: { and: [{ eq: [{ var: "country" }, { value: "US" }] }] }, + environments: [], + variant: 1, + }, + ], + }), + customFieldValues: null, + holdoutIds: [11], + }, + ], + [ + { + id: 11, + name: "holdout_rules_suppression", + iteration: 1, + unitType: "session_id", + seedHi: 13, + seedLo: 111, + split: [0.1, 0.9], + trafficSeedHi: 0, + trafficSeedLo: 0, + trafficSplit: [0, 1], + fullOnVariant: 0, + applications: [], + variants: [ + { name: "A", config: null }, + { name: "B", config: null }, + ], + audience: null, + audienceStrict: false, + customFieldValues: null, + holdoutType: "full", + }, + ] + ); + + const context = new Context(sdk, contextOptions, contextParams, response); + context.attribute("country", "US"); + + // Without the fix: the matching rule (variant 1) is applied unconditionally, bypassing + // suppression entirely, so this would return 1 instead of 0. + expect(context.treatment("exp_holdout_rules")).toEqual(0); + + // White-box check (the exposure that would carry these fields never fires, since the + // experiment's own exposure is correctly suppressed below — see the publish assertion) + // to confirm the rule-variant computation branch did not run at all when suppressed: + // `ruleOverride` must stay false (never set to true and then have `variant` + // overwritten to 0 afterward) and `assigned` must be false, matching every other + // suppression case. + const assignment = context._assignments["exp_holdout_rules"]; + expect(assignment.ruleOverride).toBeFalsy(); + expect(assignment.assigned).toEqual(false); + + publisher.publish.mockReturnValue(Promise.resolve()); + + context.publish().then(() => { + // Only the holdout's own exposure fires — the experiment's own exposure must NOT + // fire, exactly like scenario 211 (custom assignment yields to suppression), just + // via the rules path instead of the custom-assignment path. + expect(publisher.publish.mock.calls[0][0].exposures).toEqual([ + { + id: 11, + name: "holdout_rules_suppression", + unit: "session_id", + exposedAt: timeOrigin, + variant: 0, + assigned: true, + eligible: true, + overridden: false, + fullOn: false, + custom: false, + audienceMismatch: false, + ruleOverride: false, + }, + ]); + done(); + }); + }); + + // Final-review Finding I-2, Test A: regression coverage for Task 5's override-fast-path + // fix (commit 716430c). Before that fix, the `hasOverride` branch in `_assign()` returned + // the cached assignment early on `overridden && variant match` alone, without checking + // `holdoutSetMatches` — so an already-overridden experiment whose holdout coverage changed + // across a refresh (here: holdout coverage added where there was none) kept returning the + // same frozen assignment forever, and the newly-applicable holdout's own exposure could + // never fire. This constructs exactly that: cache an overridden assignment with NO holdout + // coverage, refresh so the SAME experiment now has holdoutIds, and confirm the holdout's + // own exposure fires on the next treatment() call (proving the fast path rebuilt rather + // than returning the stale pinned holdouts/holdoutAssignments from before the refresh). + it("Task 5 regression: override fast path revalidates the holdout set added by a refresh (I-2 Test A)", (done) => { + const baseExperiment = { + id: 1, + name: "exp_override_holdout_added", + iteration: 1, + unitType: "session_id", + seedHi: 100, + seedLo: 200, + split: [0.5, 0.5], + trafficSeedHi: 1, + trafficSeedLo: 2, + trafficSplit: [0, 1], + fullOnVariant: 0, + applications: [{ name: "website" }], + variants: [ + { name: "A", config: null }, + { name: "B", config: null }, + ], + audience: null, + audienceStrict: false, + customFieldValues: null, + }; + + const initialResponse = buildHoldoutResponse([baseExperiment], []); + + const holdout = { + id: 11, + name: "holdout_added_after_override", + iteration: 1, + unitType: "session_id", + seedHi: 13, + seedLo: 111, + split: [0.1, 0.9], + trafficSeedHi: 0, + trafficSeedLo: 0, + trafficSplit: [0, 1], + fullOnVariant: 0, + applications: [], + variants: [ + { name: "A", config: null }, + { name: "B", config: null }, + ], + audience: null, + audienceStrict: false, + customFieldValues: null, + holdoutType: "full", + }; + + const refreshedResponse = buildHoldoutResponse([{ ...baseExperiment, holdoutIds: [11] }], [holdout]); + + const context = new Context(sdk, contextOptions, contextParams, initialResponse); + context.override("exp_override_holdout_added", 1); + + // Caches the overridden assignment with no holdout coverage yet. + expect(context.treatment("exp_override_holdout_added")).toEqual(1); + expect(context.pending()).toEqual(1); + + provider.getContextData.mockReturnValue(Promise.resolve(refreshedResponse)); + + context.refresh().then(() => { + // Pre-fix: this returns the stale cached assignment (holdouts: undefined) without + // ever resolving the newly-applicable holdout, so its own exposure never fires. + // Override still wins the variant either way (scenario 210's precedent) — the + // regression is specifically about the holdout's own exposure never being able to + // fire once coverage is added after the override was cached. + expect(context.treatment("exp_override_holdout_added")).toEqual(1); + + publisher.publish.mockReturnValue(Promise.resolve()); + + context.publish().then(() => { + const exposures = publisher.publish.mock.calls[0][0].exposures; + const holdoutExposure = exposures.find((e) => e.name === "holdout_added_after_override"); + + expect(holdoutExposure).toMatchObject({ + id: 11, + unit: "session_id", + variant: 0, + assigned: true, + eligible: true, + overridden: false, + fullOn: false, + custom: false, + audienceMismatch: false, + ruleOverride: false, + }); + done(); + }); + }); + }); + + // Final-review Finding I-2, Test B: regression coverage for Task 5's holdout-arm-count + // pinning fix (commit 716430c). Before that fix, suppression read the arm count live from + // `holdouts[i].data.split.length` instead of the pinned `holdoutArmCount` on the holdout's + // own cached Assignment — but `_getHoldoutAssignment`'s cache only invalidates on (id, + // iteration) change, so a same-iteration split-length change (not expected on the real + // wire, but exactly the edge case the fix protects against) could desync the live arm + // count from the arm count the resolved variant was actually interpreted against. + // + // Construct: resolve a 2-arm holdout for one experiment (pinning holdoutArmCount: 2 and + // variant: 1, both cached independently of any one covered experiment). Refresh so the + // SAME holdout (same id, same iteration) now reports a 3-arm split. Query a SECOND, + // previously-unqueried experiment covered by the same holdout: its own assignment is built + // fresh, so it must resolve the holdout via `_getHoldoutAssignment`, hitting the (id, + // iteration) cache seeded by the first experiment. The seeds are chosen so the two arm + // counts disagree on the verdict: isHeldOutBy(1, 2, fullOnVariant=0) is false (2-arm, + // non-suppressing) but isHeldOutBy(1, 3, fullOnVariant=0) is true (3-arm variant 1 defers + // to full-on suppression) — so if the live (3) arm count were used instead of the pinned + // (2) one, the second experiment would flip from "not suppressed" to "suppressed". + // Asserting it stays NOT suppressed after the refresh proves the pinned value governs. + // Final-review Finding I-2, Test B: regression coverage for Task 5's holdout-arm-count + // pinning fix (commit 716430c). Before that fix, suppression read the arm count live from + // `holdouts[i].data.split.length` instead of the pinned `holdoutArmCount` on the holdout's + // own cached Assignment — but `_getHoldoutAssignment`'s cache only invalidates on (id, + // iteration) change, so a same-iteration split-length change (not expected on the real + // wire, but exactly the edge case the fix protects against) could desync the live arm + // count from the arm count the resolved variant was actually interpreted against. + // + // Construct: resolve a 2-arm holdout for one experiment (pinning holdoutArmCount: 2 and + // variant: 1, both cached independently of any one covered experiment). Refresh so the + // SAME holdout (same id, same iteration) now reports a 3-arm split. Query a SECOND, + // previously-unqueried experiment (fullOnVariant: 0, the arm-1-specific case per + // isHeldOutBy) covered by the same holdout: its own assignment is built fresh, so it must + // resolve the holdout via `_getHoldoutAssignment`, hitting the (id, iteration) cache seeded + // by the first experiment. The seeds are chosen so the two arm counts disagree on the + // verdict: isHeldOutBy(1, 2, fullOnVariant=0) is false (2-arm, non-suppressing) but + // isHeldOutBy(1, 3, fullOnVariant=0) is true (3-arm variant 1 forces control when + // fullOnVariant===0) — so if the live (3) arm count were used instead of the pinned (2) + // one, the second experiment would flip from "assigned normally" to "suppressed + // (assigned: false, no own exposure)". Asserting it stays assigned normally (with its own + // exposure firing) after the refresh proves the pinned value governs, not the live one. + it("Task 5 regression: same-iteration holdout split-length change does not desync the pinned arm count (I-2 Test B)", (done) => { + const holdoutTwoArm = { + id: 11, + name: "holdout_pin_test", + iteration: 1, + unitType: "session_id", + seedHi: 13, + seedLo: 111, + split: [0.1, 0.9], + trafficSeedHi: 0, + trafficSeedLo: 0, + trafficSplit: [0, 1], + fullOnVariant: 0, + applications: [], + variants: [ + { name: "A", config: null }, + { name: "B", config: null }, + ], + audience: null, + audienceStrict: false, + customFieldValues: null, + holdoutType: "full", + }; + + const expA = { + id: 1, + name: "exp_pin_seed_a", + iteration: 1, + unitType: "session_id", + seedHi: 100, + seedLo: 200, + split: [0.5, 0.5], + trafficSeedHi: 1, + trafficSeedLo: 2, + trafficSplit: [0, 1], + fullOnVariant: 0, + applications: [{ name: "website" }], + variants: [ + { name: "A", config: null }, + { name: "B", config: null }, + ], + audience: null, + audienceStrict: false, + customFieldValues: null, + holdoutIds: [11], + }; + + const initialResponse = buildHoldoutResponse([expA], [holdoutTwoArm]); + + // Same unit hashed by scenario 204 to resolve this identical holdout definition + // (seedHi 13, seedLo 111, split [0.1, 0.9]) to variant 1 — i.e. NOT held out under a + // 2-arm interpretation. + const pinParams = { units: { session_id: "b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3" } }; + const context = new Context(sdk, contextOptions, pinParams, initialResponse); + + // Seeds _getHoldoutAssignment's cache: resolves and pins holdoutArmCount: 2 (the split + // length live at this moment) alongside variant: 1. Matches scenario 204's verdict for + // the identical holdout+unit pairing (not held out under 2 arms). + expect(context.treatment("exp_pin_seed_a")).toEqual(0); + + // fullOnVariant: 0 is the key choice here — isHeldOutBy's 3-arm/variant-1 rule only + // forces control when the covered experiment's fullOnVariant === 0, so this is the + // only shape where the pinned (2) vs. live (3) arm count actually disagree on the + // suppression verdict. + const expB = { + ...expA, + id: 2, + name: "exp_pin_seed_b", + }; + + // Same holdout id AND iteration — the key precondition — but now 3 arms instead of 2. + const holdoutThreeArm = { ...holdoutTwoArm, split: [0.3, 0.3, 0.4] }; + + const refreshedResponse = buildHoldoutResponse([expA, expB], [holdoutThreeArm]); + + provider.getContextData.mockReturnValue(Promise.resolve(refreshedResponse)); + + context.refresh().then(() => { + // exp_pin_seed_b is queried for the first time here, so its own assignment is built + // fresh and must resolve the holdout via _getHoldoutAssignment, hitting the (id, + // iteration) cache seeded above. + // Pinned arm count (2, correct): isHeldOutBy(1, 2, 0) is false -> NOT suppressed + // -> normal traffic-split assignment path -> assigned: true, own exposure fires. + // Live arm count (3, the pre-fix bug): isHeldOutBy(1, 3, 0) is true -> suppressed + // -> assigned: false, variant forced to 0, own exposure does NOT fire. + context.treatment("exp_pin_seed_b"); + + publisher.publish.mockReturnValue(Promise.resolve()); + + context.publish().then(() => { + const exposures = publisher.publish.mock.calls[0][0].exposures; + const ownExposure = exposures.find((e) => e.name === "exp_pin_seed_b"); + + expect(ownExposure).toMatchObject({ + assigned: true, + eligible: true, + overridden: false, + fullOn: false, + custom: false, + audienceMismatch: false, + ruleOverride: false, + }); + done(); + }); + }); + }); }); describe("variableValue()", () => { diff --git a/src/context.ts b/src/context.ts index 09387eb..da6a6cd 100644 --- a/src/context.ts +++ b/src/context.ts @@ -768,85 +768,106 @@ export default class Context { } else { if (experiment != null) { const unitType = experiment.data.unitType; - - let ruleVariant: number | null = null; const attrs = this._getAttributesMap(); - if (experiment.data.assignmentRules && experiment.data.assignmentRules.length > 0) { - ruleVariant = this._computeRuleVariant( - experiment.data.assignmentRules, - experiment.data.variants.length, - attrs - ); - } - - assignment.ruleVariant = ruleVariant; + // `ruleKey` is bookkeeping only (a cache key derived from the rules string + env, + // not an evaluation of them against attrs), so it is always kept up to date — + // including when suppressed — mirroring `attrsSeq` below, which is also set + // unconditionally. Without this, a suppressed assignment would leave `ruleKey` + // unset, and the cache-validity check in `audienceMatches` (above) would see + // `ruleKeyChanged` as permanently true on every subsequent call for an experiment + // with assignmentRules, forcing a full rebuild (losing `assignment.exposed`) on + // every single treatment()/peek() call instead of only on a genuine change. assignment.ruleKey = experiment.data.assignmentRules ? `${experiment.data.assignmentRules}:${this._environmentName}` : ""; - if (ruleVariant !== null) { - assignment.variant = ruleVariant; - assignment.ruleOverride = true; + // Suppression is checked FIRST, before assignment rules (or audience, or the + // traffic-split/fullOn path) get any say over the variant. Assignment rules are a + // deterministic-per-attribute assignment mechanism — structurally the same category + // as a custom assignment (scenario 211: custom assignment yields to suppression) — + // not an override in the sense scenario 210 establishes (only an explicit override() + // call is exempt from suppression). If a matching rule were allowed to set the + // variant before this check, a held-out unit would be silently TREATED with the + // rule's variant while its exposure-firing gate + // (`!assignment.suppressed || assignment.overridden`, which does NOT include + // `ruleOverride`) still suppresses its own exposure — the worst combination: + // measured nothing, but received real treatment. Gating here means + // `ruleVariant`/`ruleOverride` are never computed nor set when suppressed, so the + // exposure gate needs no `ruleOverride` special-case: a suppressed assignment never + // has `ruleOverride: true` in the first place. + if (assignment.suppressed) { + assignment.assigned = false; + assignment.variant = 0; } else { - if (experiment.data.audience && experiment.data.audience.length > 0) { - const result = this._evaluateAudience(experiment.data.audience); - - // Only flag a mismatch when the audience actually evaluated - // to a boolean. A null result (e.g. an audience with no - // usable filter like `{}`) leaves audienceMismatch false, - // matching the collector (ContextAPI: `if (result != null)`). - if (result !== null) { - assignment.audienceMismatch = !result; - } + let ruleVariant: number | null = null; + + if (experiment.data.assignmentRules && experiment.data.assignmentRules.length > 0) { + ruleVariant = this._computeRuleVariant( + experiment.data.assignmentRules, + experiment.data.variants.length, + attrs + ); } - if (assignment.suppressed) { - assignment.assigned = false; - assignment.variant = 0; - } else if (experiment.data.audienceStrict && assignment.audienceMismatch) { - assignment.variant = 0; - } else if (experiment.data.fullOnVariant === 0) { - if (unitType !== null) { - if (unitType in this._units) { - const unit = this._unitHash(unitType); - if (unit !== null) { - const assigner = - unitType in this._assigners - ? this._assigners[unitType] - : (this._assigners[unitType] = new VariantAssigner(unit)); - const eligible = - assigner.assign( - experiment.data.trafficSplit, - experiment.data.trafficSeedHi, - experiment.data.trafficSeedLo - ) === 1; - - assignment.assigned = true; - assignment.eligible = eligible; - - if (eligible) { - if (hasCustom) { - assignment.variant = this._cassignments[experimentName]; - assignment.custom = true; + assignment.ruleVariant = ruleVariant; + + if (ruleVariant !== null) { + assignment.variant = ruleVariant; + assignment.ruleOverride = true; + } else { + if (experiment.data.audience && experiment.data.audience.length > 0) { + const result = this._audienceMatcher.evaluate(experiment.data.audience, attrs); + + if (typeof result === "boolean") { + assignment.audienceMismatch = !result; + } + } + + if (experiment.data.audienceStrict && assignment.audienceMismatch) { + assignment.variant = 0; + } else if (experiment.data.fullOnVariant === 0) { + if (unitType !== null) { + if (unitType in this._units) { + const unit = this._unitHash(unitType); + if (unit !== null) { + const assigner = + unitType in this._assigners + ? this._assigners[unitType] + : (this._assigners[unitType] = new VariantAssigner(unit)); + const eligible = + assigner.assign( + experiment.data.trafficSplit, + experiment.data.trafficSeedHi, + experiment.data.trafficSeedLo + ) === 1; + + assignment.assigned = true; + assignment.eligible = eligible; + + if (eligible) { + if (hasCustom) { + assignment.variant = this._cassignments[experimentName]; + assignment.custom = true; + } else { + assignment.variant = assigner.assign( + experiment.data.split, + experiment.data.seedHi, + experiment.data.seedLo + ); + } } else { - assignment.variant = assigner.assign( - experiment.data.split, - experiment.data.seedHi, - experiment.data.seedLo - ); + assignment.variant = 0; } - } else { - assignment.variant = 0; } } } + } else { + assignment.assigned = true; + assignment.eligible = true; + assignment.variant = experiment.data.fullOnVariant; + assignment.fullOn = true; } - } else { - assignment.assigned = true; - assignment.eligible = true; - assignment.variant = experiment.data.fullOnVariant; - assignment.fullOn = true; } } From bf5c3c6a0d4df5d8361e270d675eb36e84783021 Mon Sep 17 00:00:00 2001 From: Jonas Alves Date: Thu, 10 Sep 2026 11:10:58 +0100 Subject: [PATCH 11/28] test(holdouts): remove duplicated stale comment block in Task 5 regression test The final-review fix wave left two copies of the Test B setup comment, the first an abandoned draft contradicting the corrected copy beneath it. Pure comment cleanup, no behavior change. --- src/__tests__/context.test.js | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/src/__tests__/context.test.js b/src/__tests__/context.test.js index 8bf4528..eb6986b 100644 --- a/src/__tests__/context.test.js +++ b/src/__tests__/context.test.js @@ -4911,25 +4911,6 @@ describe("Context", () => { }); }); - // Final-review Finding I-2, Test B: regression coverage for Task 5's holdout-arm-count - // pinning fix (commit 716430c). Before that fix, suppression read the arm count live from - // `holdouts[i].data.split.length` instead of the pinned `holdoutArmCount` on the holdout's - // own cached Assignment — but `_getHoldoutAssignment`'s cache only invalidates on (id, - // iteration) change, so a same-iteration split-length change (not expected on the real - // wire, but exactly the edge case the fix protects against) could desync the live arm - // count from the arm count the resolved variant was actually interpreted against. - // - // Construct: resolve a 2-arm holdout for one experiment (pinning holdoutArmCount: 2 and - // variant: 1, both cached independently of any one covered experiment). Refresh so the - // SAME holdout (same id, same iteration) now reports a 3-arm split. Query a SECOND, - // previously-unqueried experiment covered by the same holdout: its own assignment is built - // fresh, so it must resolve the holdout via `_getHoldoutAssignment`, hitting the (id, - // iteration) cache seeded by the first experiment. The seeds are chosen so the two arm - // counts disagree on the verdict: isHeldOutBy(1, 2, fullOnVariant=0) is false (2-arm, - // non-suppressing) but isHeldOutBy(1, 3, fullOnVariant=0) is true (3-arm variant 1 defers - // to full-on suppression) — so if the live (3) arm count were used instead of the pinned - // (2) one, the second experiment would flip from "not suppressed" to "suppressed". - // Asserting it stays NOT suppressed after the refresh proves the pinned value governs. // Final-review Finding I-2, Test B: regression coverage for Task 5's holdout-arm-count // pinning fix (commit 716430c). Before that fix, suppression read the arm count live from // `holdouts[i].data.split.length` instead of the pinned `holdoutArmCount` on the holdout's From 3215ec6140e0592ad935d63e129c3d71431ddc83 Mon Sep 17 00:00:00 2001 From: Jonas Alves Date: Thu, 10 Sep 2026 14:15:56 +0100 Subject: [PATCH 12/28] fix(holdouts): address CodeRabbit review findings on PR #65 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Assert the exact exposure payload for scenario 214 (3-arm variant 0), matching the pattern already used by scenario 213 — a bare pending() count doesn't distinguish "the holdout fired" from "the covered experiment fired instead". - Drop the unused JSON.parse of a holdout's variants[].config: no holdout code path reads Experiment.variables for a holdout entry (_getHoldoutAssignment only reads split/seedHi/seedLo/id/iteration), so a malformed config on a holdout previously crashed the whole Context constructor for a field nothing consumes. - Extract the duplicated exposure-trigger/first-error-propagation block shared by _treatment and _variableValue into _triggerExposures, so the suppression/override gate and holdout-firing ordering can't drift between the two call sites. --- src/__tests__/context.test.js | 24 ++++++++- src/context.ts | 95 ++++++++++++++--------------------- 2 files changed, 60 insertions(+), 59 deletions(-) diff --git a/src/__tests__/context.test.js b/src/__tests__/context.test.js index eb6986b..4eb0e5c 100644 --- a/src/__tests__/context.test.js +++ b/src/__tests__/context.test.js @@ -3749,7 +3749,7 @@ describe("Context", () => { // experiment, full-on or not, identically to a 2-arm holdout's variant 0; the holdout's // own exposure fires once at variant 0 and neither covered experiment emits its own // exposure. - it("3-arm variant 0 holds out both full-on and non-full-on covered experiments (scenario 214)", () => { + it("3-arm variant 0 holds out both full-on and non-full-on covered experiments (scenario 214)", (done) => { const response = buildHoldoutResponse( [ { @@ -3819,6 +3819,28 @@ describe("Context", () => { expect(context.treatment("exp_three_arm_0_fullon")).toEqual(0); expect(context.pending()).toEqual(1); + + publisher.publish.mockReturnValue(Promise.resolve()); + + context.publish().then(() => { + expect(publisher.publish.mock.calls[0][0].exposures).toEqual([ + { + id: 21, + name: "holdout_three_arm_v0", + unit: "session_id", + exposedAt: timeOrigin, + variant: 0, + assigned: true, + eligible: true, + overridden: false, + fullOn: false, + custom: false, + audienceMismatch: false, + ruleOverride: false, + }, + ]); + done(); + }); }); // Scenario 215 (index 214): a 3-arm holdout's variant 1 forces control only for a covered diff --git a/src/context.ts b/src/context.ts index da6a6cd..337294f 100644 --- a/src/context.ts +++ b/src/context.ts @@ -898,41 +898,46 @@ export default class Context { if (!assignment.exposed) { assignment.exposed = true; - // Ported from java-sdk's triggerExposure (Context.java:481-503): the own-exposure attempt - // and the holdout-firing loop share one "first error wins" outcome — a throwing eventLogger - // on the OWN exposure must not prevent the holdout loop from running (and vice versa), and - // whichever throws first is what ultimately propagates to the caller, only after both have - // had a chance to fire. - let firstError: CaughtError; - - // An override always fires its own exposure, even when the covered experiment is also - // suppressed by a holdout: overriding replaces the resolved variant outright (the override's - // value wins, not the holdout's), so its own exposure must still be observable. This mirrors - // java-sdk's outcome for the override path (Context.java:1184-1244, triggerExposure at - // Context.java:481-503) — java's override write path never sets `assignment.suppressed` at - // all, so its own `!assignment.suppressed` exposure gate trivially always passes there. Our - // JS port pins `suppressed` eagerly for the override path too (Task 4's deliberate - // divergence, see Assignment.suppressed doc comment), so the exposure gate here must - // special-case `overridden` explicitly to reproduce the same firing outcome. - if (!assignment.suppressed || assignment.overridden) { - try { - this._queueExposure(experimentName, assignment); - } catch (error) { - firstError = { value: error }; - } - } + this._triggerExposures(experimentName, assignment); + } - const holdoutError = this._triggerApplicableHoldoutExposures(assignment); - if (!firstError) { - firstError = holdoutError; - } + return assignment; + } + + // Ported from java-sdk's triggerExposure (Context.java:481-503): the own-exposure attempt + // and the holdout-firing loop share one "first error wins" outcome — a throwing eventLogger + // on the OWN exposure must not prevent the holdout loop from running (and vice versa), and + // whichever throws first is what ultimately propagates to the caller, only after both have + // had a chance to fire. Shared by `_treatment` and `_variableValue`, whose exposure-firing + // behavior is otherwise identical once the one-shot `exposed` gate has been checked. + private _triggerExposures(experimentName: string, assignment: Assignment): void { + let firstError: CaughtError; - if (firstError) { - throw firstError.value; + // An override always fires its own exposure, even when the covered experiment is also + // suppressed by a holdout: overriding replaces the resolved variant outright (the override's + // value wins, not the holdout's), so its own exposure must still be observable. This mirrors + // java-sdk's outcome for the override path (Context.java:1184-1244, triggerExposure at + // Context.java:481-503) — java's override write path never sets `assignment.suppressed` at + // all, so its own `!assignment.suppressed` exposure gate trivially always passes there. Our + // JS port pins `suppressed` eagerly for the override path too (Task 4's deliberate + // divergence, see Assignment.suppressed doc comment), so the exposure gate here must + // special-case `overridden` explicitly to reproduce the same firing outcome. + if (!assignment.suppressed || assignment.overridden) { + try { + this._queueExposure(experimentName, assignment); + } catch (error) { + firstError = { value: error }; } } - return assignment; + const holdoutError = this._triggerApplicableHoldoutExposures(assignment); + if (!firstError) { + firstError = holdoutError; + } + + if (firstError) { + throw firstError.value; + } } // Ported from java-sdk's triggerApplicableHoldoutExposures/triggerHoldoutExposure @@ -1090,27 +1095,7 @@ export default class Context { if (shouldQueueExposure && !assignment.exposed) { assignment.exposed = true; - // See _treatment's matching comment: the own-exposure attempt and the holdout-firing - // loop share one "first error wins" outcome, and an override always fires its own - // exposure, even when also suppressed by a holdout. - let firstError: CaughtError; - - if (!assignment.suppressed || assignment.overridden) { - try { - this._queueExposure(experimentName, assignment); - } catch (error) { - firstError = { value: error }; - } - } - - const holdoutError = this._triggerApplicableHoldoutExposures(assignment); - if (!firstError) { - firstError = holdoutError; - } - - if (firstError) { - throw firstError.value; - } + this._triggerExposures(experimentName, assignment); } if (key in assignment.variables && (assignment.assigned || assignment.overridden || assignment.ruleOverride)) { @@ -1575,15 +1560,9 @@ export default class Context { return undefined; } - const holdoutVariables: Record[] = []; - (holdoutData.variants || []).forEach((variant, i) => { - const config = variant.config; - holdoutVariables[i] = config != null && config.length > 0 ? JSON.parse(config) : {}; - }); - const holdoutEntry: Experiment = { data: holdoutData, - variables: holdoutVariables, + variables: [], }; holdoutExperiments[holdoutId] = holdoutEntry; From 6c8649e2e283210a26695abfc06e765eef348611 Mon Sep 17 00:00:00 2001 From: Jonas Alves Date: Fri, 11 Sep 2026 09:57:56 +0100 Subject: [PATCH 13/28] fix(holdouts): fix exposure-loss ordering bug, log every dropped exposure error (FT-2206) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Whole-branch review fix wave: - _queueExposure now appends to the publish queue and increments pending() BEFORE calling the (user-supplied) eventLogger, wrapped so _setTimeout() still runs if the logger throws. Previously the logger ran first, so a throw meant the exposure was discarded outright, not merely unreported. Pre-existing on main, but the holdout exposure loop multiplies the number of independent places this can bite. - Every caught exposure-firing error is now reported via _logError as it's caught, not just the one ultimately rethrown to the caller. With N applicable holdouts there can be up to N+1 independent firing attempts sharing one "first error wins" outcome; without this, every failure past the first vanished with no trace. - Documented two known java-sdk-parity limitations with regression tests rather than fixing them here (see PR #65 discussion): a holdout's exposure can be permanently lost if treatment() resolves a full-on experiment before its unit is set, and a refresh adding a non-suppressing holdout to an already-exposed experiment duplicates its own exposure. Both are inherited from java-sdk's Context (invalidateAssignmentsPinnedWithMissingUnit and experimentMatches/holdoutSetMatches respectively) and are intentionally left unchanged. 407 tests (190 in context.test.js, +4). tsc --noEmit, lint, and format:check all clean. 🤖 Generated with Claude Code Co-Authored-By: Claude --- src/__tests__/context.test.js | 369 ++++++++++++++++++++++++++++++++++ src/context.ts | 18 +- 2 files changed, 384 insertions(+), 3 deletions(-) diff --git a/src/__tests__/context.test.js b/src/__tests__/context.test.js index 4eb0e5c..7c35c6e 100644 --- a/src/__tests__/context.test.js +++ b/src/__tests__/context.test.js @@ -5060,6 +5060,375 @@ describe("Context", () => { }); }); }); + + // This behavior is inherited from java-sdk's Context (Context.java:325-381, + // invalidateAssignmentsPinnedWithMissingUnit) and is intentionally left unchanged here for + // parity — see the discussion on PR #65. Documented as known behavior, not a target for a + // JS-only fix. + describe("holdouts: known java-sdk-parity behavior (not fixed here)", () => { + // A full-on experiment's own variant never needs a unit, so treatment() can resolve and + // expose it before the covered experiment's unit type is ever installed. If a holdout + // covering it also needs that unit, its own assignment resolves to null and gets pinned + // into the (now-exposed) assignment's holdoutAssignments. Once installed, `unit()`'s + // eviction only clears UNEXPOSED assignments (to avoid double-firing the covered + // experiment's own exposure — see the next test) — so this assignment, already exposed, + // is never evicted, and the holdout's null entry can never be repaired. The holdout's own + // exposure is lost for the life of the context. java-sdk has the identical gap. + it("permanently loses a holdout's exposure when treatment() (not peek()) resolves a full-on experiment before its unit is set", (done) => { + const response = buildHoldoutResponse( + [ + { + id: 1, + name: "exp_holdout_treatment_before_unit", + iteration: 1, + unitType: "user_id", + seedHi: 100, + seedLo: 200, + split: [0.5, 0.5, 0.0], + trafficSeedHi: 1, + trafficSeedLo: 2, + trafficSplit: [0, 1], + fullOnVariant: 2, + applications: [{ name: "website" }], + variants: [ + { name: "A", config: null }, + { name: "B", config: null }, + { name: "C", config: null }, + ], + audience: null, + audienceStrict: false, + customFieldValues: null, + holdoutIds: [11], + }, + ], + [ + { + id: 11, + name: "holdout_treatment_before_unit", + iteration: 1, + unitType: "user_id", + seedHi: 1, + seedLo: 222, + split: [0.1, 0.9], + trafficSeedHi: 0, + trafficSeedLo: 0, + trafficSplit: [0, 1], + fullOnVariant: 0, + applications: [], + variants: [ + { name: "A", config: null }, + { name: "B", config: null }, + ], + audience: null, + audienceStrict: false, + customFieldValues: null, + holdoutType: "full", + }, + ] + ); + + const lateUnitParams = { units: { session_id: "context-created-without-user-id" } }; + const context = new Context(sdk, contextOptions, lateUnitParams, response); + + // treatment() (not peek()) fires the covered experiment's own exposure immediately, + // since full-on doesn't need a unit — pinning a null holdout entry as a side effect. + expect(context.treatment("exp_holdout_treatment_before_unit")).toEqual(2); + expect(context.pending()).toEqual(1); + + context.unit("user_id", "e791e240fcd3df7d238cfc285f475e8152fcc0ec"); + + expect(context.treatment("exp_holdout_treatment_before_unit")).toEqual(2); + + publisher.publish.mockReturnValue(Promise.resolve()); + + context.publish().then(() => { + const exposures = publisher.publish.mock.calls[0][0].exposures; + // Known limitation: only the covered experiment's own exposure is ever published. + // The holdout's exposure never fires, even though its unit is now available. + expect(exposures).toEqual([ + { + id: 1, + name: "exp_holdout_treatment_before_unit", + unit: "user_id", + exposedAt: timeOrigin, + variant: 2, + assigned: true, + eligible: true, + overridden: false, + fullOn: true, + custom: false, + audienceMismatch: false, + ruleOverride: false, + }, + ]); + done(); + }); + }); + + // This behavior is inherited from java-sdk's Context (Context.java:974-1005, + // experimentMatches/holdoutSetMatches) and is intentionally left unchanged here for + // parity — see the discussion on PR #65. Documented as known behavior, not a target for a + // JS-only fix. + // + // experimentMatches() folds holdoutSetMatches() into its cache-validity check, so ANY + // change to an experiment's applicable-holdout set — even adding a holdout that does not + // end up suppressing the unit — invalidates the cached assignment and forces a full + // rebuild via _assign(), which starts the new Assignment with exposed: false regardless + // of whether the resolved variant/suppression outcome actually changed. If the covered + // experiment had already fired its own exposure before the refresh, the next treatment() + // call fires it again. java-sdk has the identical gap: the covered-experiment side of + // this exact scenario is untested there for the non-override path, but the override path + // has an equivalent, explicitly-asserted duplicate (see + // holdoutBecomingApplicableAfterRefreshStillFiresForOverriddenExperiment in + // ContextHoldoutTest.java), governed by the same experimentMatches/holdoutSetMatches + // mechanism. + it("duplicates the covered experiment's own exposure when a refresh adds a non-suppressing holdout to an already-exposed experiment", (done) => { + const baseExperiment = { + id: 1, + name: "exp_holdout_added_non_suppressing", + iteration: 1, + unitType: "session_id", + seedHi: 100, + seedLo: 200, + split: [0.5, 0.5], + trafficSeedHi: 1, + trafficSeedLo: 2, + trafficSplit: [0, 1], + fullOnVariant: 1, + applications: [{ name: "website" }], + variants: [ + { name: "A", config: null }, + { name: "B", config: null }, + ], + audience: null, + audienceStrict: false, + customFieldValues: null, + }; + + const initialResponse = buildHoldoutResponse([baseExperiment], []); + + const context = new Context(sdk, contextOptions, contextParams, initialResponse); + + // Fires the covered experiment's own exposure — no holdout coverage yet. + expect(context.treatment("exp_holdout_added_non_suppressing")).toEqual(1); + expect(context.pending()).toEqual(1); + + const nonSuppressingHoldout = { + id: 11, + name: "holdout_non_suppressing", + iteration: 1, + unitType: "session_id", + seedHi: 13, + seedLo: 111, + // This unit always lands in variant 1 of this holdout, which never holds out a + // full-on-variant-1 experiment (isHeldOutBy only suppresses on variant 0, or + // variant 1 of a 3-arm holdout when fullOnVariant === 0). + split: [0.0, 1.0], + trafficSeedHi: 0, + trafficSeedLo: 0, + trafficSplit: [0, 1], + fullOnVariant: 0, + applications: [], + variants: [ + { name: "A", config: null }, + { name: "B", config: null }, + ], + audience: null, + audienceStrict: false, + customFieldValues: null, + holdoutType: "full", + }; + + const refreshedResponse = buildHoldoutResponse( + [{ ...baseExperiment, holdoutIds: [11] }], + [nonSuppressingHoldout] + ); + + provider.getContextData.mockReturnValue(Promise.resolve(refreshedResponse)); + + context.refresh().then(() => { + // Known limitation: the covered experiment's outcome hasn't changed (still + // variant 1, still not suppressed), but the holdout-set change alone forces a + // rebuild that re-fires its own exposure. + expect(context.treatment("exp_holdout_added_non_suppressing")).toEqual(1); + + publisher.publish.mockReturnValue(Promise.resolve()); + + context.publish().then(() => { + const exposures = publisher.publish.mock.calls[0][0].exposures; + const ownExposures = exposures.filter((e) => e.name === "exp_holdout_added_non_suppressing"); + + expect(ownExposures).toHaveLength(2); + + const holdoutExposures = exposures.filter((e) => e.name === "holdout_non_suppressing"); + expect(holdoutExposures).toHaveLength(1); + + done(); + }); + }); + }); + }); + + describe("holdouts: error handling", () => { + // _queueExposure must append to the publish queue and increment pending() BEFORE calling + // the (user-supplied) eventLogger, so a throwing logger only fails to report the + // exposure, not discard it outright. Regression test for the ordering fix: previously + // _logEvent ran first, so a throw there meant the push/increment never happened and the + // exposure was lost for the life of the context, not merely unlogged. + it("keeps a queued exposure even when the eventLogger throws while reporting it", () => { + const throwingEventLogger = jest.fn((ctx, eventName) => { + if (eventName === "exposure") { + throw new Error("logger boom"); + } + }); + + const response = buildHoldoutResponse( + [ + { + id: 1, + name: "exp_holdout_throwing_logger", + iteration: 1, + unitType: "session_id", + seedHi: 100, + seedLo: 200, + split: [0.5, 0.5], + trafficSeedHi: 1, + trafficSeedLo: 2, + trafficSplit: [0, 1], + fullOnVariant: 1, + applications: [{ name: "website" }], + variants: [ + { name: "A", config: null }, + { name: "B", config: null }, + ], + audience: null, + audienceStrict: false, + customFieldValues: null, + }, + ], + [] + ); + + const context = new Context( + sdk, + { ...contextOptions, eventLogger: throwingEventLogger }, + contextParams, + response + ); + + expect(() => context.treatment("exp_holdout_throwing_logger")).toThrow("logger boom"); + + // The exposure was appended and counted before the logger ran, so the throw only + // fails to report it — it is not lost. + expect(context.pending()).toEqual(1); + }); + + // With N applicable holdouts there can be up to N+1 independent exposure-firing attempts + // per call (the covered experiment's own, plus one per holdout), but only one error can + // ever be rethrown to the caller. Every caught error must still be reported via the + // eventLogger's "error" event as it's caught, or every failure past the first vanishes + // with no trace at all. + it("reports every dropped exposure error via the eventLogger, not just the one that is rethrown", () => { + const loggedErrors = []; + const eventLogger = jest.fn((ctx, eventName, data) => { + if (eventName === "error") { + loggedErrors.push(data); + return; + } + if (eventName === "exposure") { + throw new Error(`boom for ${data.name}`); + } + }); + + const response = buildHoldoutResponse( + [ + { + id: 1, + name: "exp_holdout_multi_error", + iteration: 1, + unitType: "session_id", + seedHi: 100, + seedLo: 200, + split: [0.5, 0.5], + trafficSeedHi: 1, + trafficSeedLo: 2, + trafficSplit: [0, 1], + fullOnVariant: 1, + applications: [{ name: "website" }], + variants: [ + { name: "A", config: null }, + { name: "B", config: null }, + ], + audience: null, + audienceStrict: false, + customFieldValues: null, + holdoutIds: [11, 12], + }, + ], + [ + { + id: 11, + name: "holdout_multi_error_a", + iteration: 1, + unitType: "session_id", + seedHi: 1, + seedLo: 111, + split: [0.0, 1.0], + trafficSeedHi: 0, + trafficSeedLo: 0, + trafficSplit: [0, 1], + fullOnVariant: 0, + applications: [], + variants: [ + { name: "A", config: null }, + { name: "B", config: null }, + ], + audience: null, + audienceStrict: false, + customFieldValues: null, + holdoutType: "full", + }, + { + id: 12, + name: "holdout_multi_error_b", + iteration: 1, + unitType: "session_id", + seedHi: 2, + seedLo: 222, + split: [0.0, 1.0], + trafficSeedHi: 0, + trafficSeedLo: 0, + trafficSplit: [0, 1], + fullOnVariant: 0, + applications: [], + variants: [ + { name: "A", config: null }, + { name: "B", config: null }, + ], + audience: null, + audienceStrict: false, + customFieldValues: null, + holdoutType: "full", + }, + ] + ); + + const context = new Context(sdk, { ...contextOptions, eventLogger }, contextParams, response); + + expect(() => context.treatment("exp_holdout_multi_error")).toThrow(/boom for/); + + // All three exposure attempts (the covered experiment + both holdouts) threw, so all + // three must have been reported — not just the one whose error was rethrown. + expect(loggedErrors).toHaveLength(3); + expect(loggedErrors.map((e) => e.message).sort()).toEqual( + [ + "boom for exp_holdout_multi_error", + "boom for holdout_multi_error_a", + "boom for holdout_multi_error_b", + ].sort() + ); + }); + }); }); describe("variableValue()", () => { diff --git a/src/context.ts b/src/context.ts index 337294f..71d5378 100644 --- a/src/context.ts +++ b/src/context.ts @@ -910,6 +910,11 @@ export default class Context { // whichever throws first is what ultimately propagates to the caller, only after both have // had a chance to fire. Shared by `_treatment` and `_variableValue`, whose exposure-firing // behavior is otherwise identical once the one-shot `exposed` gate has been checked. + // + // Every caught error is reported via `_logError` as it's caught (unlike java-sdk, which only + // ever surfaces the first): with N applicable holdouts there can be up to N+1 independent + // exposure-firing attempts, and only one error can be rethrown to the caller, so without this + // every failure past the first would otherwise vanish with no trace at all. private _triggerExposures(experimentName: string, assignment: Assignment): void { let firstError: CaughtError; @@ -926,6 +931,7 @@ export default class Context { try { this._queueExposure(experimentName, assignment); } catch (error) { + this._logError(error as Error); firstError = { value: error }; } } @@ -964,6 +970,7 @@ export default class Context { try { this._queueExposure(holdouts[i].data.name, holdoutAssignment); } catch (error) { + this._logError(error as Error); if (!firstError) { firstError = { value: error }; } @@ -989,12 +996,17 @@ export default class Context { audienceMismatch: assignment.audienceMismatch, ruleOverride: assignment.ruleOverride, }; - this._logEvent("exposure", exposureEvent); - + // The exposure is appended and counted BEFORE the (user-supplied) eventLogger runs: a + // throwing logger must not discard the exposure itself, only fail to report it. _setTimeout + // is scheduled in a finally so a throwing logger still flushes what's already queued. this._exposures.push(exposureEvent); this._pending++; - this._setTimeout(); + try { + this._logEvent("exposure", exposureEvent); + } finally { + this._setTimeout(); + } } private _customFieldKeys() { From 5aec1412061d8f92ed0fd3c54ee3c68ac9f37b38 Mon Sep 17 00:00:00 2001 From: Jonas Alves Date: Fri, 11 Sep 2026 10:14:48 +0100 Subject: [PATCH 14/28] fix(holdouts): guard _logError calls against a throwing error-event logger (FT-2206) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit review finding on cb23cb2: if the (user-supplied) eventLogger throws while handling the "error" event itself, that throw would escape _triggerExposures before the holdout loop ever runs, or abort the holdout loop partway through — silently dropping every remaining exposure attempt, the opposite of what the error-reporting change was meant to guarantee. Adds _logErrorSafely, which swallows a throw from the error-event logger, and uses it at both exposure-firing call sites. Added a regression test with a logger that throws on both "exposure" and "error" events, confirming every exposure attempt still runs. 408 tests. tsc --noEmit, lint, and format:check all clean. 🤖 Generated with Claude Code Co-Authored-By: Claude --- src/__tests__/context.test.js | 87 +++++++++++++++++++++++++++++++++++ src/context.ts | 15 +++++- 2 files changed, 100 insertions(+), 2 deletions(-) diff --git a/src/__tests__/context.test.js b/src/__tests__/context.test.js index 7c35c6e..51547e6 100644 --- a/src/__tests__/context.test.js +++ b/src/__tests__/context.test.js @@ -5428,6 +5428,93 @@ describe("Context", () => { ].sort() ); }); + + // A throwing error-event logger must not itself interrupt exposure processing: it must + // not prevent the holdout loop from running when it's reporting the covered experiment's + // own exposure failure, and it must not stop the holdout loop partway through when it's + // reporting one holdout's exposure failure. Regression test for _logErrorSafely. + it("does not let a throwing error-event logger interrupt exposure processing", () => { + const eventLogger = jest.fn((ctx, eventName, data) => { + if (eventName === "exposure") { + throw new Error(`exposure boom for ${data.name}`); + } + if (eventName === "error") { + throw new Error("error-handler boom"); + } + }); + + const response = buildHoldoutResponse( + [ + { + id: 1, + name: "exp_holdout_throwing_error_logger", + iteration: 1, + unitType: "session_id", + seedHi: 100, + seedLo: 200, + split: [0.5, 0.5], + trafficSeedHi: 1, + trafficSeedLo: 2, + trafficSplit: [0, 1], + fullOnVariant: 1, + applications: [{ name: "website" }], + variants: [ + { name: "A", config: null }, + { name: "B", config: null }, + ], + audience: null, + audienceStrict: false, + customFieldValues: null, + holdoutIds: [11], + }, + ], + [ + { + id: 11, + name: "holdout_throwing_error_logger", + iteration: 1, + unitType: "session_id", + seedHi: 1, + seedLo: 111, + split: [0.0, 1.0], + trafficSeedHi: 0, + trafficSeedLo: 0, + trafficSplit: [0, 1], + fullOnVariant: 0, + applications: [], + variants: [ + { name: "A", config: null }, + { name: "B", config: null }, + ], + audience: null, + audienceStrict: false, + customFieldValues: null, + holdoutType: "full", + }, + ] + ); + + const context = new Context(sdk, { ...contextOptions, eventLogger }, contextParams, response); + + let caught; + try { + context.treatment("exp_holdout_throwing_error_logger"); + } catch (e) { + caught = e; + } + + // The covered experiment's exposure attempt threw, and reporting that error via the + // (also throwing) error-event logger must not have prevented the holdout loop from + // running: the holdout's own exposure attempt must still have been made (and its + // error, in turn, safely reported without escaping). + expect(caught).toBeDefined(); + expect(caught.message).toEqual("exposure boom for exp_holdout_throwing_error_logger"); + + const exposureCalls = eventLogger.mock.calls.filter((c) => c[1] === "exposure"); + expect(exposureCalls.map((c) => c[2].name).sort()).toEqual( + ["exp_holdout_throwing_error_logger", "holdout_throwing_error_logger"].sort() + ); + }); }); }); diff --git a/src/context.ts b/src/context.ts index 71d5378..053979b 100644 --- a/src/context.ts +++ b/src/context.ts @@ -931,7 +931,7 @@ export default class Context { try { this._queueExposure(experimentName, assignment); } catch (error) { - this._logError(error as Error); + this._logErrorSafely(error as Error); firstError = { value: error }; } } @@ -970,7 +970,7 @@ export default class Context { try { this._queueExposure(holdouts[i].data.name, holdoutAssignment); } catch (error) { - this._logError(error as Error); + this._logErrorSafely(error as Error); if (!firstError) { firstError = { value: error }; } @@ -1455,6 +1455,17 @@ export default class Context { } } + // Like `_logError`, but swallows a throw from the (user-supplied) eventLogger itself. Used at + // exposure-firing call sites where reporting one failure must never prevent the remaining + // exposure attempts (sibling holdouts, or the covered experiment's own) from still running. + private _logErrorSafely(error: Error) { + try { + this._logError(error); + } catch { + // Deliberately ignored — see comment above. + } + } + private _unitHash(unitType: string) { if (!this._hashes) { this._hashes = {}; From 94693deba499b13c8535141c769e13a783dcf08d Mon Sep 17 00:00:00 2001 From: Jonas Alves Date: Fri, 11 Sep 2026 10:48:38 +0100 Subject: [PATCH 15/28] fix(holdouts): fix stale doc comment, tighten multi-error test, drop rotting PR reference (FT-2206) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Whole-branch review pass after cb23cb2/32feb37: - _triggerExposures's doc comment said errors are reported via `_logError`, but 32feb37 switched both call sites to `_logErrorSafely` without updating this comment. - Added a `pending()` assertion to the multi-error logging test, confirming all three exposure attempts were queued despite all three throwing while reporting (not just verified via the logged errors). - Dropped two "see the discussion on PR #65" pointers from the known-limitation test comments — the surrounding text already carries the substance, and the PR reference would rot on merge. 408 tests. tsc --noEmit, lint, and format:check all clean. 🤖 Generated with Claude Code Co-Authored-By: Claude --- src/__tests__/context.test.js | 10 ++++++---- src/context.ts | 4 ++-- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/__tests__/context.test.js b/src/__tests__/context.test.js index 51547e6..a5fc1f8 100644 --- a/src/__tests__/context.test.js +++ b/src/__tests__/context.test.js @@ -5063,8 +5063,7 @@ describe("Context", () => { // This behavior is inherited from java-sdk's Context (Context.java:325-381, // invalidateAssignmentsPinnedWithMissingUnit) and is intentionally left unchanged here for - // parity — see the discussion on PR #65. Documented as known behavior, not a target for a - // JS-only fix. + // parity. Documented as known behavior, not a target for a JS-only fix. describe("holdouts: known java-sdk-parity behavior (not fixed here)", () => { // A full-on experiment's own variant never needs a unit, so treatment() can resolve and // expose it before the covered experiment's unit type is ever installed. If a holdout @@ -5167,8 +5166,7 @@ describe("Context", () => { // This behavior is inherited from java-sdk's Context (Context.java:974-1005, // experimentMatches/holdoutSetMatches) and is intentionally left unchanged here for - // parity — see the discussion on PR #65. Documented as known behavior, not a target for a - // JS-only fix. + // parity. Documented as known behavior, not a target for a JS-only fix. // // experimentMatches() folds holdoutSetMatches() into its cache-validity check, so ANY // change to an experiment's applicable-holdout set — even adding a holdout that does not @@ -5427,6 +5425,10 @@ describe("Context", () => { "boom for holdout_multi_error_b", ].sort() ); + + // All three exposures were queued despite all three throwing while reporting — + // the ordering fix applies independently to each attempt, not just the first. + expect(context.pending()).toEqual(3); }); // A throwing error-event logger must not itself interrupt exposure processing: it must diff --git a/src/context.ts b/src/context.ts index 053979b..b9308a3 100644 --- a/src/context.ts +++ b/src/context.ts @@ -911,8 +911,8 @@ export default class Context { // had a chance to fire. Shared by `_treatment` and `_variableValue`, whose exposure-firing // behavior is otherwise identical once the one-shot `exposed` gate has been checked. // - // Every caught error is reported via `_logError` as it's caught (unlike java-sdk, which only - // ever surfaces the first): with N applicable holdouts there can be up to N+1 independent + // Every caught error is reported via `_logErrorSafely` as it's caught (unlike java-sdk, which + // only ever surfaces the first): with N applicable holdouts there can be up to N+1 independent // exposure-firing attempts, and only one error can be rethrown to the caller, so without this // every failure past the first would otherwise vanish with no trace at all. private _triggerExposures(experimentName: string, assignment: Assignment): void { From 6f50f2bfbae7e178ae49211819bd8243a0464e75 Mon Sep 17 00:00:00 2001 From: Jonas Alves Date: Fri, 11 Sep 2026 11:23:46 +0100 Subject: [PATCH 16/28] fix(holdouts): add missing doc comment on Assignment.suppressed (FT-2206) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review finding: _triggerExposures's comment at src/context.ts:892 pointed to "Assignment.suppressed doc comment" for the override-path divergence rationale, but that field had no doc comment at all, dangling since it was introduced in 91ff39a. 408 tests. tsc --noEmit, lint, and format:check all clean. 🤖 Generated with Claude Code Co-Authored-By: Claude --- src/context.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/context.ts b/src/context.ts index b9308a3..5e7be37 100644 --- a/src/context.ts +++ b/src/context.ts @@ -69,6 +69,12 @@ type Assignment = { trafficSplit?: number[]; variables?: Record; attrsSeq?: number; + // Set whenever the covered experiment has one or more applicable holdouts, regardless of + // override/custom-assignment/rule-variant handling. Unlike java-sdk — whose override write + // path never sets this field at all, so its exposure gate's `!suppressed` check trivially + // always passes for an override — the JS port pins it eagerly on the override path too, so + // the exposure-firing gate (`_triggerExposures`) must special-case `overridden` explicitly to + // reproduce the same firing outcome (an override always fires its own exposure). suppressed?: boolean; holdouts?: Experiment[]; holdoutAssignments?: (Assignment | null)[]; From 861b2b15fc23d56e037844b51518ec7ac2067a94 Mon Sep 17 00:00:00 2001 From: Jonas Alves Date: Fri, 11 Sep 2026 11:52:13 +0100 Subject: [PATCH 17/28] fix(holdouts): add suppressedFallback so held-out units read control values (FT-2206) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Critical review finding: _variableValue/_peekVariable had no counterpart to java-sdk's getVariableAssignment suppressedFallback (Context.java:1332-1373). A held-out unit's assignment.assigned stays false by design (so it never wins variable-key resolution over a genuinely assigned experiment), but with no fallback the JS port fell straight through to the caller's default instead of the experiment's own control-variant value -- making a held-out unit observably different from a control unit for variableValue()/peekVariableValue(), exactly what holdouts are meant to prevent. Fixed by tracking the first-encountered suppressed candidate with the key while resolving (matching java's ordering) and using its value as a fallback only when no candidate wins outright. Verified with a reproduction: before the fix, a held-out unit's variableValue() on a key defined on the control variant returned the caller's literal default; after, it returns the control-variant value, matching a non-held-out control unit exactly. peekVariableValue() gets the same fallback without triggering exposures, matching its existing contract. Added 4 regression tests (variableValue reads control value + still exposes; peekVariableValue reads control value without exposing; a genuinely assigned experiment still wins over a suppressed one sharing the key; falls through to the caller default when no suppressed candidate defines the key at all). 412 tests. tsc --noEmit, lint, and format:check all clean. 🤖 Generated with Claude Code Co-Authored-By: Claude --- src/__tests__/context.test.js | 118 ++++++++++++++++++++++++++++++++++ src/context.ts | 20 +++++- 2 files changed, 135 insertions(+), 3 deletions(-) diff --git a/src/__tests__/context.test.js b/src/__tests__/context.test.js index a5fc1f8..b6f6b27 100644 --- a/src/__tests__/context.test.js +++ b/src/__tests__/context.test.js @@ -5518,6 +5518,124 @@ describe("Context", () => { ); }); }); + + describe("holdouts: variable resolution", () => { + // Ported from java-sdk's getVariableAssignment (Context.java:1332-1373). A suppressed + // experiment must never win variable-key resolution over one that is genuinely + // assigned/overridden, but it must still be usable as a fallback: a held-out unit reads + // the experiment's own control-variant value, not the caller's default, so it stays + // indistinguishable from a control unit for variable resolution purposes. + const holdoutVarExperiment = (over) => ({ + id: 1, + name: "exp_holdout_var", + iteration: 1, + unitType: "session_id", + seedHi: 100, + seedLo: 200, + split: [0.5, 0.5], + trafficSeedHi: 1, + trafficSeedLo: 2, + trafficSplit: [0, 1], + fullOnVariant: 0, + applications: [{ name: "website" }], + variants: [ + { name: "A", config: JSON.stringify({ "button.color": "CONTROL_GREY" }) }, + { name: "B", config: JSON.stringify({ "button.color": "TREATMENT_GREEN" }) }, + ], + audience: null, + audienceStrict: false, + customFieldValues: null, + holdoutIds: [11], + ...over, + }); + + const alwaysHoldsOut = (over) => ({ + id: 11, + name: "holdout_var", + iteration: 1, + unitType: "session_id", + seedHi: 13, + seedLo: 111, + split: [1.0, 0.0], + trafficSeedHi: 0, + trafficSeedLo: 0, + trafficSplit: [0, 1], + fullOnVariant: 0, + applications: [], + variants: [ + { name: "A", config: null }, + { name: "B", config: null }, + ], + audience: null, + audienceStrict: false, + customFieldValues: null, + holdoutType: "full", + ...over, + }); + + it("variableValue() returns the control-variant value (not the caller default) for a held-out unit, and still fires the holdout's own exposure", () => { + const response = buildHoldoutResponse([holdoutVarExperiment({})], [alwaysHoldsOut({})]); + const context = new Context(sdk, contextOptions, contextParams, response); + + expect(context.treatment("exp_holdout_var")).toEqual(0); + expect(context.variableValue("button.color", "APP_FALLBACK")).toEqual("CONTROL_GREY"); + expect(context.pending()).toEqual(1); + }); + + it("peekVariableValue() returns the control-variant value for a held-out unit without triggering any exposure", () => { + const response = buildHoldoutResponse([holdoutVarExperiment({})], [alwaysHoldsOut({})]); + const context = new Context(sdk, contextOptions, contextParams, response); + + expect(context.peekVariableValue("button.color", "APP_FALLBACK")).toEqual("CONTROL_GREY"); + expect(context.pending()).toEqual(0); + }); + + it("a genuinely assigned experiment still wins variable-key resolution over a suppressed one sharing the same key", () => { + const winningExperiment = { + id: 2, + name: "exp_holdout_var_winner", + iteration: 1, + unitType: "session_id", + seedHi: 300, + seedLo: 400, + split: [0, 1], + trafficSeedHi: 1, + trafficSeedLo: 2, + trafficSplit: [0, 1], + fullOnVariant: 1, + applications: [{ name: "website" }], + variants: [ + { name: "A", config: null }, + { name: "B", config: JSON.stringify({ "button.color": "WINNER_BLUE" }) }, + ], + audience: null, + audienceStrict: false, + customFieldValues: null, + }; + + const response = buildHoldoutResponse( + [holdoutVarExperiment({}), winningExperiment], + [alwaysHoldsOut({})] + ); + const context = new Context(sdk, contextOptions, contextParams, response); + + expect(context.variableValue("button.color", "APP_FALLBACK")).toEqual("WINNER_BLUE"); + }); + + it("returns the caller default when every candidate for the key is suppressed but none defines the key on its held-out variant", () => { + const noKeyOnControlExperiment = holdoutVarExperiment({ + variants: [ + { name: "A", config: null }, + { name: "B", config: JSON.stringify({ "button.color": "TREATMENT_GREEN" }) }, + ], + }); + + const response = buildHoldoutResponse([noKeyOnControlExperiment], [alwaysHoldsOut({})]); + const context = new Context(sdk, contextOptions, contextParams, response); + + expect(context.variableValue("button.color", "APP_FALLBACK")).toEqual("APP_FALLBACK"); + }); + }); }); describe("variableValue()", () => { diff --git a/src/context.ts b/src/context.ts index 5e7be37..1ae86b6 100644 --- a/src/context.ts +++ b/src/context.ts @@ -1105,7 +1105,15 @@ export default class Context { return this._customFieldValueType(experimentName, key); } + // Ported from java-sdk's getVariableAssignment (Context.java:1332-1373). A suppressed + // (held-out) experiment never wins resolution over a genuinely assigned/overridden one sharing + // the same key, but it IS used as a fallback when no candidate wins: a held-out unit must still + // read the experiment's control-variant value, not the caller's default, so it stays + // indistinguishable from a control unit. Only the first-encountered suppressed candidate is + // kept (java's ordering), since only one fallback can ever be returned. private _resolveVariableValue(key: string, defaultValue: string, shouldQueueExposure: boolean): string { + let suppressedFallback: string | undefined; + for (const experiment of this._indexVariables[key] ?? []) { const experimentName = experiment.data.name; const assignment = this._assign(experimentName); @@ -1116,13 +1124,19 @@ export default class Context { this._triggerExposures(experimentName, assignment); } - if (key in assignment.variables && (assignment.assigned || assignment.overridden || assignment.ruleOverride)) { - return assignment.variables[key] as string; + if (key in assignment.variables) { + if (assignment.assigned || assignment.overridden || assignment.ruleOverride) { + return assignment.variables[key] as string; + } + + if (assignment.suppressed && suppressedFallback === undefined) { + suppressedFallback = assignment.variables[key] as string; + } } } } - return defaultValue; + return suppressedFallback !== undefined ? suppressedFallback : defaultValue; } private _variableValue(key: string, defaultValue: string): string { From 710807e60d0e70fe847e3bf017ed264f8725d8c1 Mon Sep 17 00:00:00 2001 From: Jonas Alves Date: Fri, 11 Sep 2026 12:21:33 +0100 Subject: [PATCH 18/28] fix(holdouts): correct suppressedFallback ordering, fix multi-candidate exposure loop abort (FT-2206) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two Important review findings on the suppressedFallback fix (8517d9d): - The fallback candidate was captured only when it defined the requested key, so an earlier suppressed candidate lacking the key could be skipped in favor of a later suppressed candidate that has it. java-sdk's getVariableAssignment pins the first-encountered suppressed Assignment unconditionally and only checks the key against that one candidate afterwards -- an earlier keyless suppressed candidate must produce the caller's default, not let a later candidate's value win. Fixed by capturing the whole suppressed candidate's variables map regardless of key presence, matching java's ordering exactly. - A throwing eventLogger during one candidate's exposure firing aborted the whole _variableValue() loop, permanently losing every later candidate's holdout exposure for that call (recoverable only on a subsequent call, since the throwing candidate's assignment was already marked exposed). java's getVariableAssignment collects the first failure and continues the loop, throwing only once resolution is otherwise complete. Fixed by wrapping _triggerExposures in a try/catch inside the loop and rethrowing the first collected error after the loop finishes (or immediately once a winning candidate is found), mirroring the same collect-then-rethrow discipline _triggerExposures/_triggerApplicableHoldoutExposures already use for a single experiment's exposure set. _peekVariable is unaffected by the second fix since it never triggers exposures. Added 2 regression tests: an earlier keyless suppressed candidate correctly blocks a later suppressed candidate's value; a throwing logger on one candidate still lets the loop visit and expose later candidates. 414 tests. tsc --noEmit, lint, and format:check all clean. 🤖 Generated with Claude Code Co-Authored-By: Claude --- src/__tests__/context.test.js | 71 +++++++++++++++++++++++++++++++++++ src/context.ts | 47 +++++++++++++++++------ 2 files changed, 107 insertions(+), 11 deletions(-) diff --git a/src/__tests__/context.test.js b/src/__tests__/context.test.js index b6f6b27..3219ac3 100644 --- a/src/__tests__/context.test.js +++ b/src/__tests__/context.test.js @@ -5635,6 +5635,77 @@ describe("Context", () => { expect(context.variableValue("button.color", "APP_FALLBACK")).toEqual("APP_FALLBACK"); }); + + // Ported from java's getVariableAssignment (Context.java:1362-1364): java pins the whole + // first-encountered suppressed Assignment as the fallback and only checks the key on it + // afterwards — it never skips past a suppressed candidate lacking the key to consider a + // later one that has it. An earlier suppressed candidate without the key must produce the + // caller's default, not a later suppressed candidate's value. + it("an earlier suppressed candidate without the key blocks a later suppressed candidate that has it (java's ordering)", () => { + const firstSuppressed = holdoutVarExperiment({ + id: 1, + name: "exp_holdout_var_first", + // Held out -> forced to variant 0 (control), which does NOT define the key. + variants: [ + { name: "A", config: null }, + { name: "B", config: JSON.stringify({ "button.color": "FIRST_TREATMENT" }) }, + ], + }); + + const secondSuppressed = holdoutVarExperiment({ + id: 2, + name: "exp_holdout_var_second", + // Held out -> forced to variant 0 (control), which DOES define the key. + variants: [ + { name: "A", config: JSON.stringify({ "button.color": "SECOND_CONTROL" }) }, + { name: "B", config: null }, + ], + }); + + const response = buildHoldoutResponse([firstSuppressed, secondSuppressed], [alwaysHoldsOut({})]); + const context = new Context(sdk, contextOptions, contextParams, response); + + expect(context.variableValue("button.color", "APP_FALLBACK")).toEqual("APP_FALLBACK"); + }); + + // A throwing eventLogger for one candidate's exposure must not stop the loop from + // visiting (and firing exposures for) the remaining candidates sharing the key — the + // same "collect first error, keep going" guarantee _triggerExposures already provides + // within a single experiment's own exposure set must also hold across the candidate loop. + it("a throwing eventLogger for one candidate does not stop later candidates in the loop from being visited and exposed", () => { + const firstExperiment = holdoutVarExperiment({ + id: 1, + name: "exp_holdout_var_throw_first", + holdoutIds: [11], + }); + + const secondExperiment = holdoutVarExperiment({ + id: 2, + name: "exp_holdout_var_throw_second", + holdoutIds: [12], + }); + + const secondHoldout = alwaysHoldsOut({ id: 12, name: "holdout_var_second" }); + + const eventLogger = jest.fn((ctx, eventName, data) => { + if (eventName === "exposure" && data.name === "holdout_var") { + throw new Error("boom"); + } + }); + + const response = buildHoldoutResponse( + [firstExperiment, secondExperiment], + [alwaysHoldsOut({}), secondHoldout] + ); + const context = new Context(sdk, { ...contextOptions, eventLogger }, contextParams, response); + + expect(() => context.variableValue("button.color", "APP_FALLBACK")).toThrow("boom"); + + const exposureCalls = eventLogger.mock.calls.filter((c) => c[1] === "exposure"); + expect(exposureCalls.map((c) => c[2].name).sort()).toEqual( + ["holdout_var", "holdout_var_second"].sort() + ); + }); }); }); diff --git a/src/context.ts b/src/context.ts index 1ae86b6..95d2238 100644 --- a/src/context.ts +++ b/src/context.ts @@ -1109,10 +1109,19 @@ export default class Context { // (held-out) experiment never wins resolution over a genuinely assigned/overridden one sharing // the same key, but it IS used as a fallback when no candidate wins: a held-out unit must still // read the experiment's control-variant value, not the caller's default, so it stays - // indistinguishable from a control unit. Only the first-encountered suppressed candidate is - // kept (java's ordering), since only one fallback can ever be returned. + // indistinguishable from a control unit. The first-encountered suppressed candidate is captured + // as the fallback regardless of whether it defines the key (matching java, which pins the whole + // Assignment and only checks the key afterwards) — an earlier suppressed candidate lacking the + // key must not let a later suppressed candidate that does have it win instead. + // + // A throwing eventLogger for one candidate must not stop the loop from visiting (and firing + // exposures for) the remaining candidates, mirroring java's collect-first-failure-then-rethrow + // (Context.java:1350-1368): every candidate's exposures still fire, and the first error is + // thrown only once resolution is otherwise complete (a winning candidate found, or the loop + // exhausted). private _resolveVariableValue(key: string, defaultValue: string, shouldQueueExposure: boolean): string { - let suppressedFallback: string | undefined; + let suppressedFallback: Record | undefined; + let firstError: CaughtError; for (const experiment of this._indexVariables[key] ?? []) { const experimentName = experiment.data.name; @@ -1121,22 +1130,38 @@ export default class Context { if (shouldQueueExposure && !assignment.exposed) { assignment.exposed = true; - this._triggerExposures(experimentName, assignment); + try { + this._triggerExposures(experimentName, assignment); + } catch (error) { + if (!firstError) { + firstError = { value: error }; + } + } } - if (key in assignment.variables) { - if (assignment.assigned || assignment.overridden || assignment.ruleOverride) { - return assignment.variables[key] as string; + if (key in assignment.variables && (assignment.assigned || assignment.overridden || assignment.ruleOverride)) { + if (firstError) { + throw firstError.value; } - if (assignment.suppressed && suppressedFallback === undefined) { - suppressedFallback = assignment.variables[key] as string; - } + return assignment.variables[key] as string; + } + + if (assignment.suppressed && suppressedFallback === undefined) { + suppressedFallback = assignment.variables; } } } - return suppressedFallback !== undefined ? suppressedFallback : defaultValue; + if (firstError) { + throw firstError.value; + } + + if (suppressedFallback !== undefined && key in suppressedFallback) { + return suppressedFallback[key] as string; + } + + return defaultValue; } private _variableValue(key: string, defaultValue: string): string { From effc322923a4cf0233f3546787e1304640bcb4e2 Mon Sep 17 00:00:00 2001 From: Jonas Alves Date: Fri, 11 Sep 2026 12:32:58 +0100 Subject: [PATCH 19/28] test(holdouts): add regression coverage for peekVariable fallback ordering and winner-after-throw (FT-2206) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review pass found the two Important fixes from f465694 were correct but incompletely covered: - _peekVariable's suppressedFallback capture has the identical ordering fix as _variableValue's (same commit, same bug), but only _variableValue had a dedicated regression test for it. Added the peekVariableValue() analog: an earlier suppressed candidate lacking the key must still block a later suppressed candidate that has it. - _variableValue's immediate-rethrow-on-winner branch (throw the collected error even when a later candidate in the loop turns out to be a genuine winner, per the function's own doc comment) had no test forcing that specific branch. Added a regression test with an earlier suppressed candidate whose exposure-firing throws, followed by a later genuinely-assigned winning candidate -- confirms the call still throws (not silently returns the winner's value) and that the winning candidate's own exposure still fired. Both new tests verified non-vacuous by mutation testing (reverting each corresponding fix locally, confirming the new test fails, restoring, confirming a clean tree and full suite). 416 tests. tsc --noEmit, lint, and format:check all clean. 🤖 Generated with Claude Code Co-Authored-By: Claude --- src/__tests__/context.test.js | 80 +++++++++++++++++++++++++++++++++++ 1 file changed, 80 insertions(+) diff --git a/src/__tests__/context.test.js b/src/__tests__/context.test.js index 3219ac3..5fc389e 100644 --- a/src/__tests__/context.test.js +++ b/src/__tests__/context.test.js @@ -5706,6 +5706,86 @@ describe("Context", () => { ["holdout_var", "holdout_var_second"].sort() ); }); + + // peekVariableValue() shares the exact same suppressedFallback capture as variableValue() + // (both fixed together in the same commit), but never triggers exposures, so it needs its + // own ordering regression test rather than relying on variableValue()'s coverage. + it("peekVariableValue(): an earlier suppressed candidate without the key blocks a later suppressed candidate that has it", () => { + const firstSuppressed = holdoutVarExperiment({ + id: 1, + name: "exp_holdout_var_peek_first", + variants: [ + { name: "A", config: null }, + { name: "B", config: JSON.stringify({ "button.color": "FIRST_TREATMENT" }) }, + ], + }); + + const secondSuppressed = holdoutVarExperiment({ + id: 2, + name: "exp_holdout_var_peek_second", + variants: [ + { name: "A", config: JSON.stringify({ "button.color": "SECOND_CONTROL" }) }, + { name: "B", config: null }, + ], + }); + + const response = buildHoldoutResponse([firstSuppressed, secondSuppressed], [alwaysHoldsOut({})]); + const context = new Context(sdk, contextOptions, contextParams, response); + + expect(context.peekVariableValue("button.color", "APP_FALLBACK")).toEqual("APP_FALLBACK"); + expect(context.pending()).toEqual(0); + }); + + // The doc comment on _variableValue states the collected error is thrown "once resolution + // is otherwise complete (a winning candidate found, or the loop exhausted)" -- including + // when a LATER candidate wins after an EARLIER candidate's exposure-firing already threw. + // That immediate-rethrow-on-winner branch must not be bypassed just because resolution + // otherwise succeeded: the caller needs to know an exposure was dropped, even though a + // value could technically still be returned. + it("still throws the first collected error even when a later candidate in the loop is a genuine winner", () => { + const suppressedFirst = holdoutVarExperiment({ + id: 1, + name: "exp_holdout_var_winner_after_throw", + holdoutIds: [11], + }); + + const winningSecond = { + id: 2, + name: "exp_holdout_var_winner_after_throw_2", + iteration: 1, + unitType: "session_id", + seedHi: 300, + seedLo: 400, + split: [0, 1], + trafficSeedHi: 1, + trafficSeedLo: 2, + trafficSplit: [0, 1], + fullOnVariant: 1, + applications: [{ name: "website" }], + variants: [ + { name: "A", config: null }, + { name: "B", config: JSON.stringify({ "button.color": "WINNER_AFTER_THROW" }) }, + ], + audience: null, + audienceStrict: false, + customFieldValues: null, + }; + + const eventLogger = jest.fn((ctx, eventName, data) => { + if (eventName === "exposure" && data.name === "holdout_var") { + throw new Error("boom"); + } + }); + + const response = buildHoldoutResponse([suppressedFirst, winningSecond], [alwaysHoldsOut({})]); + const context = new Context(sdk, { ...contextOptions, eventLogger }, contextParams, response); + + expect(() => context.variableValue("button.color", "APP_FALLBACK")).toThrow("boom"); + + // The winning candidate's own exposure still fired despite the earlier throw. + const exposureCalls = eventLogger.mock.calls.filter((c) => c[1] === "exposure"); + expect(exposureCalls.map((c) => c[2].name)).toContain("exp_holdout_var_winner_after_throw_2"); + }); }); }); From 422111bdae4f71bd11617c2b4c4f56c3c913ba3c Mon Sep 17 00:00:00 2001 From: Jonas Alves Date: Fri, 11 Sep 2026 12:54:42 +0100 Subject: [PATCH 20/28] fix(holdouts): exclude overridden assignments from suppressedFallback (FT-2206) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Minor review finding: _triggerExposures already special-cases `overridden` in its exposure-firing gate (`!assignment.suppressed || assignment.overridden`) because the JS port pins `suppressed` eagerly on the override path, unlike java-sdk, which never sets it there -- so an override can never become java's suppressedFallback. The suppressedFallback capture in _variableValue/_peekVariable missed this same exclusion, so an override to a variant that happens to omit the requested key could still claim the fallback slot ahead of a sibling that is genuinely suppressed and actually defines the key. Fixed by adding the same `!assignment.overridden` guard to both capture sites, and updated the Assignment.suppressed doc comment cross-reference to explain why. Verified with a reproduction (before: returned the caller's default instead of the genuinely-suppressed sibling's control value; after: returns the sibling's value) and a mutation-tested regression test. 417 tests. tsc --noEmit, lint, and format:check all clean. 🤖 Generated with Claude Code Co-Authored-By: Claude --- src/__tests__/context.test.js | 67 +++++++++++++++++++++++++++++++++++ src/context.ts | 19 +++++----- 2 files changed, 78 insertions(+), 8 deletions(-) diff --git a/src/__tests__/context.test.js b/src/__tests__/context.test.js index 5fc389e..7ac3aff 100644 --- a/src/__tests__/context.test.js +++ b/src/__tests__/context.test.js @@ -5786,6 +5786,73 @@ describe("Context", () => { const exposureCalls = eventLogger.mock.calls.filter((c) => c[1] === "exposure"); expect(exposureCalls.map((c) => c[2].name)).toContain("exp_holdout_var_winner_after_throw_2"); }); + + // java's override write path never sets `suppressed` at all (see Assignment.suppressed's + // doc comment), so an override can never become java's suppressedFallback. The JS port + // pins `suppressed` eagerly on the override path too, so the fallback capture must + // explicitly exclude overridden assignments to reproduce the same outcome — otherwise an + // override to a variant that happens to omit the requested key could win the fallback + // slot ahead of a genuinely suppressed sibling that actually defines it. + it("excludes an overridden assignment from the suppressedFallback even when it is also held out", () => { + const overriddenExperiment = { + id: 1, + name: "exp_holdout_var_overridden", + iteration: 1, + unitType: "session_id", + seedHi: 100, + seedLo: 200, + split: [0.5, 0.5], + trafficSeedHi: 1, + trafficSeedLo: 2, + trafficSplit: [0, 1], + fullOnVariant: 0, + applications: [{ name: "website" }], + variants: [ + // Override target (variant 0) omits the key, so this candidate can't win + // outright, and must not be allowed to claim the fallback slot either. + { name: "A", config: null }, + { name: "B", config: JSON.stringify({ "button.color": "OVERRIDE_SIBLING" }) }, + ], + audience: null, + audienceStrict: false, + customFieldValues: null, + holdoutIds: [11], + }; + + const genuinelySuppressedExperiment = { + id: 2, + name: "exp_holdout_var_genuinely_suppressed", + iteration: 1, + unitType: "session_id", + seedHi: 300, + seedLo: 400, + split: [0.5, 0.5], + trafficSeedHi: 1, + trafficSeedLo: 2, + trafficSplit: [0, 1], + fullOnVariant: 0, + applications: [{ name: "website" }], + variants: [ + { name: "A", config: JSON.stringify({ "button.color": "GENUINE_SUPPRESSED_CTRL" }) }, + { name: "B", config: null }, + ], + audience: null, + audienceStrict: false, + customFieldValues: null, + holdoutIds: [12], + }; + + const secondHoldout = alwaysHoldsOut({ id: 12, name: "holdout_var_second" }); + + const response = buildHoldoutResponse( + [overriddenExperiment, genuinelySuppressedExperiment], + [alwaysHoldsOut({}), secondHoldout] + ); + const context = new Context(sdk, contextOptions, contextParams, response); + context.override("exp_holdout_var_overridden", 0); + + expect(context.variableValue("button.color", "APP_FALLBACK")).toEqual("GENUINE_SUPPRESSED_CTRL"); + }); }); }); diff --git a/src/context.ts b/src/context.ts index 95d2238..8cae1c0 100644 --- a/src/context.ts +++ b/src/context.ts @@ -1106,13 +1106,16 @@ export default class Context { } // Ported from java-sdk's getVariableAssignment (Context.java:1332-1373). A suppressed - // (held-out) experiment never wins resolution over a genuinely assigned/overridden one sharing - // the same key, but it IS used as a fallback when no candidate wins: a held-out unit must still - // read the experiment's control-variant value, not the caller's default, so it stays - // indistinguishable from a control unit. The first-encountered suppressed candidate is captured - // as the fallback regardless of whether it defines the key (matching java, which pins the whole - // Assignment and only checks the key afterwards) — an earlier suppressed candidate lacking the - // key must not let a later suppressed candidate that does have it win instead. + // (held-out) experiment never wins resolution over a genuinely assigned/overridden/rule-matched + // one sharing the same key, but it IS used as a fallback when no candidate wins: a held-out + // unit must still read the experiment's control-variant value, not the caller's default, so it + // stays indistinguishable from a control unit. The first-encountered suppressed candidate is + // captured as the fallback regardless of whether it defines the key (matching java, which pins + // the whole Assignment and only checks the key afterwards) — an earlier suppressed candidate + // lacking the key must not let a later suppressed candidate that does have it win instead. + // Overridden assignments are excluded from the capture even if `suppressed` is set: java's + // override path never sets `suppressed` at all (see Assignment.suppressed doc comment for why + // the JS port's override path pins it anyway), so an override can never become java's fallback. // // A throwing eventLogger for one candidate must not stop the loop from visiting (and firing // exposures for) the remaining candidates, mirroring java's collect-first-failure-then-rethrow @@ -1147,7 +1150,7 @@ export default class Context { return assignment.variables[key] as string; } - if (assignment.suppressed && suppressedFallback === undefined) { + if (assignment.suppressed && !assignment.overridden && suppressedFallback === undefined) { suppressedFallback = assignment.variables; } } From 626f8c713ec62f37ef126b0080e38e965976e348 Mon Sep 17 00:00:00 2001 From: Jonas Alves Date: Fri, 11 Sep 2026 13:04:41 +0100 Subject: [PATCH 21/28] test(holdouts): add peekVariableValue coverage for override-exclusion guard (FT-2206) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Confirming review pass found f476bf1's override-exclusion guard was only regression-tested on the _variableValue side; _peekVariable carries the identical guard (same fix, same commit) but had no dedicated test. Added the peekVariableValue() counterpart, verified non-vacuous by mutation testing (reverting the guard, confirming the new test fails, restoring, confirming a clean tree and full suite). 418 tests. tsc --noEmit, lint, and format:check all clean. 🤖 Generated with Claude Code Co-Authored-By: Claude --- src/__tests__/context.test.js | 63 +++++++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/src/__tests__/context.test.js b/src/__tests__/context.test.js index 7ac3aff..f3e10f2 100644 --- a/src/__tests__/context.test.js +++ b/src/__tests__/context.test.js @@ -5853,6 +5853,69 @@ describe("Context", () => { expect(context.variableValue("button.color", "APP_FALLBACK")).toEqual("GENUINE_SUPPRESSED_CTRL"); }); + + // _peekVariable carries the identical override-exclusion guard as _variableValue (same + // fix, same commit), but never triggers exposures, so it needs its own regression test + // rather than relying on variableValue()'s coverage of the guard. + it("peekVariableValue(): excludes an overridden assignment from the suppressedFallback even when it is also held out", () => { + const overriddenExperiment = { + id: 1, + name: "exp_holdout_var_peek_overridden", + iteration: 1, + unitType: "session_id", + seedHi: 100, + seedLo: 200, + split: [0.5, 0.5], + trafficSeedHi: 1, + trafficSeedLo: 2, + trafficSplit: [0, 1], + fullOnVariant: 0, + applications: [{ name: "website" }], + variants: [ + { name: "A", config: null }, + { name: "B", config: JSON.stringify({ "button.color": "OVERRIDE_SIBLING" }) }, + ], + audience: null, + audienceStrict: false, + customFieldValues: null, + holdoutIds: [11], + }; + + const genuinelySuppressedExperiment = { + id: 2, + name: "exp_holdout_var_peek_genuinely_suppressed", + iteration: 1, + unitType: "session_id", + seedHi: 300, + seedLo: 400, + split: [0.5, 0.5], + trafficSeedHi: 1, + trafficSeedLo: 2, + trafficSplit: [0, 1], + fullOnVariant: 0, + applications: [{ name: "website" }], + variants: [ + { name: "A", config: JSON.stringify({ "button.color": "GENUINE_SUPPRESSED_CTRL" }) }, + { name: "B", config: null }, + ], + audience: null, + audienceStrict: false, + customFieldValues: null, + holdoutIds: [12], + }; + + const secondHoldout = alwaysHoldsOut({ id: 12, name: "holdout_var_peek_second" }); + + const response = buildHoldoutResponse( + [overriddenExperiment, genuinelySuppressedExperiment], + [alwaysHoldsOut({}), secondHoldout] + ); + const context = new Context(sdk, contextOptions, contextParams, response); + context.override("exp_holdout_var_peek_overridden", 0); + + expect(context.peekVariableValue("button.color", "APP_FALLBACK")).toEqual("GENUINE_SUPPRESSED_CTRL"); + expect(context.pending()).toEqual(0); + }); }); }); From ea9dec7addc35d0474311eaf67deba3d1f6ca965 Mon Sep 17 00:00:00 2001 From: Jonas Alves Date: Thu, 17 Sep 2026 12:01:03 +0100 Subject: [PATCH 22/28] fix(holdouts): model the actual holdout wire shape (FT-2206) ContextData.holdouts was typed as ExperimentData[], which requires ordinary-experiment-only fields _getHoldoutAssignment never reads and rejects the real optional holdoutType field. Add a HoldoutData/ HoldoutExperiment type pair matching the accepted payload (id/name/unitType/iteration/seedHi/seedLo/split/holdoutType) so a minimal scenario-222-style holdout no longer fails tsc at the public createContextWith(..., ContextData) boundary. Purely a type change; runtime behavior (suppression, exposure firing, caching) is unchanged. --- src/context.ts | 42 ++++++++++++++++++++++++++++-------------- 1 file changed, 28 insertions(+), 14 deletions(-) diff --git a/src/context.ts b/src/context.ts index 8cae1c0..beda1fd 100644 --- a/src/context.ts +++ b/src/context.ts @@ -76,7 +76,7 @@ type Assignment = { // the exposure-firing gate (`_triggerExposures`) must special-case `overridden` explicitly to // reproduce the same firing outcome (an override always fires its own exposure). suppressed?: boolean; - holdouts?: Experiment[]; + holdouts?: HoldoutExperiment[]; holdoutAssignments?: (Assignment | null)[]; // Only set on a holdout's own resolved Assignment (as returned by _getHoldoutAssignment): the // arm count (`split.length`) the holdout's definition had at the moment `variant` was @@ -86,10 +86,25 @@ type Assignment = { holdoutArmCount?: number; }; +export type HoldoutData = { + id: number; + name: string; + unitType: string | null; + iteration: number; + seedHi: number; + seedLo: number; + split: number[]; + holdoutType?: string; +}; + +type HoldoutExperiment = { + data: HoldoutData; +}; + export type Experiment = { data: ExperimentData; variables: Record[]; - holdouts?: Experiment[] | null; + holdouts?: HoldoutExperiment[] | null; }; export type Unit = { @@ -143,7 +158,7 @@ export type ContextOptions = { export type ContextData = { experiments?: ExperimentData[]; - holdouts?: ExperimentData[]; + holdouts?: HoldoutData[]; }; // Ported verbatim from java-sdk's Context.isHeldOutBy (Context.java:1319-1330). Decides whether @@ -182,7 +197,7 @@ export default class Context { private _goals: Goal[]; private _index: Record; private _indexVariables: Record; - private _holdoutsById: Record; + private _holdoutsById: Record; private _holdoutAssignments: Record; private _overrides: Record; private _pending: number; @@ -1551,7 +1566,7 @@ export default class Context { // reference only if the id is no longer present, e.g. the holdout was removed by the latest // refresh) — this mirrors java-sdk's resolveLiveHoldout and ensures the cache is keyed and // validated against the currently-installed definition rather than a possibly-dead one. - private _getHoldoutAssignment(holdout: Experiment, unitType: string): Assignment | null { + private _getHoldoutAssignment(holdout: HoldoutExperiment, unitType: string): Assignment | null { const liveHoldoutData = this._holdoutsById[holdout.data.id] ?? holdout.data; const cacheKey = `${liveHoldoutData.id}:${unitType}`; @@ -1601,10 +1616,10 @@ export default class Context { // Live index of holdout definitions by id, skipping holdouts with no/empty // split (they can never be assigned to, so they are treated as non-existent). - // Kept as raw ExperimentData (not a resolved Experiment/Assignment) so later + // Kept as raw HoldoutData (not a resolved HoldoutExperiment/Assignment) so later // lookups (e.g. resolving a holdout's own assignment) always read against the // currently-installed data rather than a possibly-stale cached reference. - const holdoutsById: Record = {}; + const holdoutsById: Record = {}; (data.holdouts || []).forEach((holdout) => { if (holdout.split && holdout.split.length > 0) { @@ -1614,12 +1629,12 @@ export default class Context { this._holdoutsById = holdoutsById; - // Experiment wrappers (data + parsed variables) for holdouts, built lazily and + // Experiment wrappers (data only) for holdouts, built lazily and // memoized per _init() call so a holdout referenced by multiple experiments is // only parsed once. - const holdoutExperiments: Record = {}; + const holdoutExperiments: Record = {}; - const resolveHoldoutExperiment = (holdoutId: number): Experiment | undefined => { + const resolveHoldoutExperiment = (holdoutId: number): HoldoutExperiment | undefined => { if (holdoutExperiments[holdoutId]) { return holdoutExperiments[holdoutId]; } @@ -1631,9 +1646,8 @@ export default class Context { return undefined; } - const holdoutEntry: Experiment = { + const holdoutEntry: HoldoutExperiment = { data: holdoutData, - variables: [], }; holdoutExperiments[holdoutId] = holdoutEntry; @@ -1643,9 +1657,9 @@ export default class Context { (data.experiments || []).forEach((experiment) => { const variables: Record[] = []; - let holdouts: Experiment[] | null = null; + let holdouts: HoldoutExperiment[] | null = null; if (experiment.holdoutIds && experiment.holdoutIds.length > 0) { - const resolved: Experiment[] = []; + const resolved: HoldoutExperiment[] = []; experiment.holdoutIds.forEach((holdoutId) => { const holdoutExperiment = resolveHoldoutExperiment(holdoutId); From ba33bbd79c8dd4ad68b235edf8a3b8f5f9e0320d Mon Sep 17 00:00:00 2001 From: Jonas Alves Date: Tue, 22 Sep 2026 16:10:13 +0100 Subject: [PATCH 23/28] fix(holdouts): export HoldoutExperiment and make HoldoutData.unitType optional (FT-2206) unitType is resolved from the covered experiment, never read off the holdout wire payload, so requiring it rejected valid payloads that omit it. HoldoutExperiment is referenced by the exported Experiment.holdouts field but wasn't itself exported, so consumers couldn't name it. --- src/context.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/context.ts b/src/context.ts index beda1fd..3010a5d 100644 --- a/src/context.ts +++ b/src/context.ts @@ -89,7 +89,7 @@ type Assignment = { export type HoldoutData = { id: number; name: string; - unitType: string | null; + unitType?: string | null; iteration: number; seedHi: number; seedLo: number; @@ -97,7 +97,7 @@ export type HoldoutData = { holdoutType?: string; }; -type HoldoutExperiment = { +export type HoldoutExperiment = { data: HoldoutData; }; From c737827150f6fd28204aaa6e96d77cdcb760dbce Mon Sep 17 00:00:00 2001 From: Jonas Alves Date: Tue, 22 Sep 2026 20:37:16 +0100 Subject: [PATCH 24/28] fix(holdouts): drop exposure error-propagation after rebase onto main's logger isolation (FT-2206) Rebasing feat/holdouts onto main (1b5ed0e) surfaced a real conflict between two independently-developed behaviors: - This branch's exposure-firing path (_triggerExposures, _triggerApplicableHoldoutExposures, _resolveVariableValue) was built to collect a throwing eventLogger's error across multiple independent exposure attempts (the covered experiment's own, plus one per applicable holdout) and rethrow the first one to the treatment()/variableValue() caller, mirroring java-sdk's triggerExposure/getVariableAssignment. - Main's already-merged history changed _logEvent/_logError to always swallow a throwing custom eventLogger internally (catch + console.error), after repeated bugs where an observer exception corrupted unrelated control flow (init, finalize, publish). With _logEvent/_logError now never throwing, _queueExposure can never throw either, making the try/catch/collect/rethrow scaffolding in the holdout exposure path dead code that could never execute. Removed it (and the now-unused CaughtError type and _logErrorSafely helper), and updated the 5 holdout tests that asserted the old throw-propagation behavior to instead assert the new (correct) isolation behavior: every exposure attempt still fires and queues regardless of a throwing logger, but nothing propagates to the caller. 485 tests. tsc --noEmit, lint, and format:check all clean. --- src/__tests__/context.test.js | 93 +++++++++++++---------------------- src/context.ts | 91 +++------------------------------- 2 files changed, 41 insertions(+), 143 deletions(-) diff --git a/src/__tests__/context.test.js b/src/__tests__/context.test.js index f3e10f2..7dc4e92 100644 --- a/src/__tests__/context.test.js +++ b/src/__tests__/context.test.js @@ -5269,10 +5269,9 @@ describe("Context", () => { describe("holdouts: error handling", () => { // _queueExposure must append to the publish queue and increment pending() BEFORE calling - // the (user-supplied) eventLogger, so a throwing logger only fails to report the - // exposure, not discard it outright. Regression test for the ordering fix: previously - // _logEvent ran first, so a throw there meant the push/increment never happened and the - // exposure was lost for the life of the context, not merely unlogged. + // the (user-supplied) eventLogger. A throwing eventLogger is isolated by _logEvent itself + // (it never propagates — see _logEvent's doc comment), so this only verifies the exposure + // is queued regardless of the logger throwing. it("keeps a queued exposure even when the eventLogger throws while reporting it", () => { const throwingEventLogger = jest.fn((ctx, eventName) => { if (eventName === "exposure") { @@ -5314,25 +5313,19 @@ describe("Context", () => { response ); - expect(() => context.treatment("exp_holdout_throwing_logger")).toThrow("logger boom"); + expect(() => context.treatment("exp_holdout_throwing_logger")).not.toThrow(); - // The exposure was appended and counted before the logger ran, so the throw only - // fails to report it — it is not lost. + // The exposure was appended and counted before the logger ran, and the logger's throw + // was isolated by _logEvent, so it is not lost. expect(context.pending()).toEqual(1); }); - // With N applicable holdouts there can be up to N+1 independent exposure-firing attempts - // per call (the covered experiment's own, plus one per holdout), but only one error can - // ever be rethrown to the caller. Every caught error must still be reported via the - // eventLogger's "error" event as it's caught, or every failure past the first vanishes - // with no trace at all. - it("reports every dropped exposure error via the eventLogger, not just the one that is rethrown", () => { - const loggedErrors = []; + // With N applicable holdouts there are up to N+1 independent exposure-firing attempts per + // call (the covered experiment's own, plus one per holdout). A throwing eventLogger on any + // of them is isolated by _logEvent (it never propagates), so every attempt must still run + // and queue its own exposure regardless of the logger throwing on an earlier one. + it("queues every exposure even when the eventLogger throws while reporting each one", () => { const eventLogger = jest.fn((ctx, eventName, data) => { - if (eventName === "error") { - loggedErrors.push(data); - return; - } if (eventName === "exposure") { throw new Error(`boom for ${data.name}`); } @@ -5413,28 +5406,22 @@ describe("Context", () => { const context = new Context(sdk, { ...contextOptions, eventLogger }, contextParams, response); - expect(() => context.treatment("exp_holdout_multi_error")).toThrow(/boom for/); + expect(() => context.treatment("exp_holdout_multi_error")).not.toThrow(); - // All three exposure attempts (the covered experiment + both holdouts) threw, so all - // three must have been reported — not just the one whose error was rethrown. - expect(loggedErrors).toHaveLength(3); - expect(loggedErrors.map((e) => e.message).sort()).toEqual( - [ - "boom for exp_holdout_multi_error", - "boom for holdout_multi_error_a", - "boom for holdout_multi_error_b", - ].sort() + const exposureCalls = eventLogger.mock.calls.filter((c) => c[1] === "exposure"); + expect(exposureCalls.map((c) => c[2].name).sort()).toEqual( + ["exp_holdout_multi_error", "holdout_multi_error_a", "holdout_multi_error_b"].sort() ); - // All three exposures were queued despite all three throwing while reporting — - // the ordering fix applies independently to each attempt, not just the first. + // All three exposures were queued despite all three throwing while reporting. expect(context.pending()).toEqual(3); }); - // A throwing error-event logger must not itself interrupt exposure processing: it must - // not prevent the holdout loop from running when it's reporting the covered experiment's - // own exposure failure, and it must not stop the holdout loop partway through when it's - // reporting one holdout's exposure failure. Regression test for _logErrorSafely. + // A throwing eventLogger (for both "exposure" and "error" events) is isolated by + // _logEvent/_logError themselves (neither ever propagates — see their doc comments), so it + // must not interrupt exposure processing: it must not prevent the holdout loop from running + // after the covered experiment's own exposure is reported, and both exposures must still + // be queued. it("does not let a throwing error-event logger interrupt exposure processing", () => { const eventLogger = jest.fn((ctx, eventName, data) => { if (eventName === "exposure") { @@ -5498,24 +5485,14 @@ describe("Context", () => { const context = new Context(sdk, { ...contextOptions, eventLogger }, contextParams, response); - let caught; - try { - context.treatment("exp_holdout_throwing_error_logger"); - } catch (e) { - caught = e; - } - - // The covered experiment's exposure attempt threw, and reporting that error via the - // (also throwing) error-event logger must not have prevented the holdout loop from - // running: the holdout's own exposure attempt must still have been made (and its - // error, in turn, safely reported without escaping). - expect(caught).toBeDefined(); - expect(caught.message).toEqual("exposure boom for exp_holdout_throwing_error_logger"); + expect(() => context.treatment("exp_holdout_throwing_error_logger")).not.toThrow(); const exposureCalls = eventLogger.mock.calls.filter((c) => c[1] === "exposure"); expect(exposureCalls.map((c) => c[2].name).sort()).toEqual( ["exp_holdout_throwing_error_logger", "holdout_throwing_error_logger"].sort() ); + + expect(context.pending()).toEqual(2); }); }); @@ -5668,10 +5645,9 @@ describe("Context", () => { expect(context.variableValue("button.color", "APP_FALLBACK")).toEqual("APP_FALLBACK"); }); - // A throwing eventLogger for one candidate's exposure must not stop the loop from - // visiting (and firing exposures for) the remaining candidates sharing the key — the - // same "collect first error, keep going" guarantee _triggerExposures already provides - // within a single experiment's own exposure set must also hold across the candidate loop. + // A throwing eventLogger for one candidate's exposure is isolated by _logEvent (it never + // propagates), so it must not stop the loop from visiting (and firing exposures for) the + // remaining candidates sharing the key. it("a throwing eventLogger for one candidate does not stop later candidates in the loop from being visited and exposed", () => { const firstExperiment = holdoutVarExperiment({ id: 1, @@ -5699,7 +5675,7 @@ describe("Context", () => { ); const context = new Context(sdk, { ...contextOptions, eventLogger }, contextParams, response); - expect(() => context.variableValue("button.color", "APP_FALLBACK")).toThrow("boom"); + expect(() => context.variableValue("button.color", "APP_FALLBACK")).not.toThrow(); const exposureCalls = eventLogger.mock.calls.filter((c) => c[1] === "exposure"); expect(exposureCalls.map((c) => c[2].name).sort()).toEqual( @@ -5736,13 +5712,10 @@ describe("Context", () => { expect(context.pending()).toEqual(0); }); - // The doc comment on _variableValue states the collected error is thrown "once resolution - // is otherwise complete (a winning candidate found, or the loop exhausted)" -- including - // when a LATER candidate wins after an EARLIER candidate's exposure-firing already threw. - // That immediate-rethrow-on-winner branch must not be bypassed just because resolution - // otherwise succeeded: the caller needs to know an exposure was dropped, even though a - // value could technically still be returned. - it("still throws the first collected error even when a later candidate in the loop is a genuine winner", () => { + // A throwing eventLogger for an earlier candidate's exposure is isolated by _logEvent, so + // a later candidate that genuinely wins must still resolve normally and still fire its own + // exposure. + it("resolves to a later winning candidate's value even after an earlier candidate's exposure logger throws", () => { const suppressedFirst = holdoutVarExperiment({ id: 1, name: "exp_holdout_var_winner_after_throw", @@ -5780,7 +5753,7 @@ describe("Context", () => { const response = buildHoldoutResponse([suppressedFirst, winningSecond], [alwaysHoldsOut({})]); const context = new Context(sdk, { ...contextOptions, eventLogger }, contextParams, response); - expect(() => context.variableValue("button.color", "APP_FALLBACK")).toThrow("boom"); + expect(context.variableValue("button.color", "APP_FALLBACK")).toEqual("WINNER_AFTER_THROW"); // The winning candidate's own exposure still fired despite the earlier throw. const exposureCalls = eventLogger.mock.calls.filter((c) => c[1] === "exposure"); diff --git a/src/context.ts b/src/context.ts index 3010a5d..abd441e 100644 --- a/src/context.ts +++ b/src/context.ts @@ -169,12 +169,6 @@ function isHeldOutBy(holdoutVariant: number, holdoutArmCount: number, fullOnVari return false; } -// Wraps a caught error so "no error occurred" (undefined) can be distinguished from "an error of -// value `undefined` was thrown" when collecting the first error across multiple try/catch sites -// (the covered experiment's own exposure attempt and the holdout-firing loop) that must share one -// "first error wins" outcome, mirroring java-sdk's triggerExposure (Context.java:481-503). -type CaughtError = { value: unknown } | undefined; - export default class Context { private readonly _assigners: Record; private readonly _attrs: Attribute[]; @@ -925,20 +919,9 @@ export default class Context { return assignment; } - // Ported from java-sdk's triggerExposure (Context.java:481-503): the own-exposure attempt - // and the holdout-firing loop share one "first error wins" outcome — a throwing eventLogger - // on the OWN exposure must not prevent the holdout loop from running (and vice versa), and - // whichever throws first is what ultimately propagates to the caller, only after both have - // had a chance to fire. Shared by `_treatment` and `_variableValue`, whose exposure-firing - // behavior is otherwise identical once the one-shot `exposed` gate has been checked. - // - // Every caught error is reported via `_logErrorSafely` as it's caught (unlike java-sdk, which - // only ever surfaces the first): with N applicable holdouts there can be up to N+1 independent - // exposure-firing attempts, and only one error can be rethrown to the caller, so without this - // every failure past the first would otherwise vanish with no trace at all. + // Shared by `_treatment` and `_variableValue`, whose exposure-firing behavior is otherwise + // identical once the one-shot `exposed` gate has been checked. private _triggerExposures(experimentName: string, assignment: Assignment): void { - let firstError: CaughtError; - // An override always fires its own exposure, even when the covered experiment is also // suppressed by a holdout: overriding replaces the resolved variant outright (the override's // value wins, not the holdout's), so its own exposure must still be observable. This mirrors @@ -949,22 +932,10 @@ export default class Context { // divergence, see Assignment.suppressed doc comment), so the exposure gate here must // special-case `overridden` explicitly to reproduce the same firing outcome. if (!assignment.suppressed || assignment.overridden) { - try { - this._queueExposure(experimentName, assignment); - } catch (error) { - this._logErrorSafely(error as Error); - firstError = { value: error }; - } - } - - const holdoutError = this._triggerApplicableHoldoutExposures(assignment); - if (!firstError) { - firstError = holdoutError; + this._queueExposure(experimentName, assignment); } - if (firstError) { - throw firstError.value; - } + this._triggerApplicableHoldoutExposures(assignment); } // Ported from java-sdk's triggerApplicableHoldoutExposures/triggerHoldoutExposure @@ -972,15 +943,10 @@ export default class Context { // using the pinned `assignment.holdoutAssignments` snapshot (not a live re-resolution), // so a data refresh landing between the suppression decision and the exposure trigger // can't publish a holdout exposure from a different epoch (Context.java:505-514). - // A throwing eventLogger for one holdout must not prevent siblings from firing: collect - // the first error and return it (rather than throwing here) so the caller can combine it - // with its own try/catch's outcome and issue a single final throw after everything has fired. - private _triggerApplicableHoldoutExposures(assignment: Assignment): CaughtError { + private _triggerApplicableHoldoutExposures(assignment: Assignment): void { const holdouts = assignment.holdouts; const holdoutAssignments = assignment.holdoutAssignments; - if (!holdouts || !holdoutAssignments) return undefined; - - let firstError: CaughtError; + if (!holdouts || !holdoutAssignments) return; holdoutAssignments.forEach((holdoutAssignment, i) => { if (holdoutAssignment == null) return; @@ -988,18 +954,9 @@ export default class Context { if (!holdoutAssignment.exposed) { holdoutAssignment.exposed = true; - try { - this._queueExposure(holdouts[i].data.name, holdoutAssignment); - } catch (error) { - this._logErrorSafely(error as Error); - if (!firstError) { - firstError = { value: error }; - } - } + this._queueExposure(holdouts[i].data.name, holdoutAssignment); } }); - - return firstError; } private _queueExposure(experimentName: string, assignment: Assignment) { @@ -1131,15 +1088,8 @@ export default class Context { // Overridden assignments are excluded from the capture even if `suppressed` is set: java's // override path never sets `suppressed` at all (see Assignment.suppressed doc comment for why // the JS port's override path pins it anyway), so an override can never become java's fallback. - // - // A throwing eventLogger for one candidate must not stop the loop from visiting (and firing - // exposures for) the remaining candidates, mirroring java's collect-first-failure-then-rethrow - // (Context.java:1350-1368): every candidate's exposures still fire, and the first error is - // thrown only once resolution is otherwise complete (a winning candidate found, or the loop - // exhausted). private _resolveVariableValue(key: string, defaultValue: string, shouldQueueExposure: boolean): string { let suppressedFallback: Record | undefined; - let firstError: CaughtError; for (const experiment of this._indexVariables[key] ?? []) { const experimentName = experiment.data.name; @@ -1148,20 +1098,10 @@ export default class Context { if (shouldQueueExposure && !assignment.exposed) { assignment.exposed = true; - try { - this._triggerExposures(experimentName, assignment); - } catch (error) { - if (!firstError) { - firstError = { value: error }; - } - } + this._triggerExposures(experimentName, assignment); } if (key in assignment.variables && (assignment.assigned || assignment.overridden || assignment.ruleOverride)) { - if (firstError) { - throw firstError.value; - } - return assignment.variables[key] as string; } @@ -1171,10 +1111,6 @@ export default class Context { } } - if (firstError) { - throw firstError.value; - } - if (suppressedFallback !== undefined && key in suppressedFallback) { return suppressedFallback[key] as string; } @@ -1518,17 +1454,6 @@ export default class Context { } } - // Like `_logError`, but swallows a throw from the (user-supplied) eventLogger itself. Used at - // exposure-firing call sites where reporting one failure must never prevent the remaining - // exposure attempts (sibling holdouts, or the covered experiment's own) from still running. - private _logErrorSafely(error: Error) { - try { - this._logError(error); - } catch { - // Deliberately ignored — see comment above. - } - } - private _unitHash(unitType: string) { if (!this._hashes) { this._hashes = {}; From db3654378c83541a09e4c6aaa806ab9d4962c6df Mon Sep 17 00:00:00 2001 From: Jonas Alves Date: Wed, 23 Sep 2026 16:54:26 +0100 Subject: [PATCH 25/28] refactor(holdouts): trim implementation-history comments to their actual invariants (FT-2206) Cut java-sdk line citations, PR/task/scenario references, and restated code from holdout comments in context.ts; kept only the ones encoding a non-obvious invariant. --- src/context.ts | 158 ++++++------------------------------------------- 1 file changed, 18 insertions(+), 140 deletions(-) diff --git a/src/context.ts b/src/context.ts index abd441e..9087587 100644 --- a/src/context.ts +++ b/src/context.ts @@ -69,20 +69,11 @@ type Assignment = { trafficSplit?: number[]; variables?: Record; attrsSeq?: number; - // Set whenever the covered experiment has one or more applicable holdouts, regardless of - // override/custom-assignment/rule-variant handling. Unlike java-sdk — whose override write - // path never sets this field at all, so its exposure gate's `!suppressed` check trivially - // always passes for an override — the JS port pins it eagerly on the override path too, so - // the exposure-firing gate (`_triggerExposures`) must special-case `overridden` explicitly to - // reproduce the same firing outcome (an override always fires its own exposure). suppressed?: boolean; holdouts?: HoldoutExperiment[]; holdoutAssignments?: (Assignment | null)[]; - // Only set on a holdout's own resolved Assignment (as returned by _getHoldoutAssignment): the - // arm count (`split.length`) the holdout's definition had at the moment `variant` was - // resolved. Pinned alongside `variant` rather than re-read from the live holdout definition, - // so a same-iteration refresh that changes `split.length` can't desync the resolved arm from - // the arm count used to interpret it (mirrors java-sdk's HoldoutAssignment, Context.java:1218-1222). + // `split.length` at the time `variant` was resolved; a same-iteration refresh can change + // the live value without invalidating the cache. holdoutArmCount?: number; }; @@ -161,8 +152,6 @@ export type ContextData = { holdouts?: HoldoutData[]; }; -// Ported verbatim from java-sdk's Context.isHeldOutBy (Context.java:1319-1330). Decides whether -// a single holdout's resolved arm suppresses the covered experiment it applies to. function isHeldOutBy(holdoutVariant: number, holdoutArmCount: number, fullOnVariant: number): boolean { if (holdoutVariant === 0) return true; if (holdoutArmCount === 3 && holdoutVariant === 1) return fullOnVariant === 0; @@ -367,23 +356,9 @@ export default class Context { this._invalidateAssignmentsPinnedWithMissingUnit(unitType); } - // Ported from java-sdk's invalidateAssignmentsPinnedWithMissingUnit (Context.java:325-381, - // called from setUnit at line 344). A cached assignment's `holdoutAssignments` snapshot pins a - // null entry when the covered experiment's unit was unavailable at the time it was resolved - // (see `_getHoldoutAssignment`'s `unit === null` early return) — holdouts in that snapshot are - // resolved using the covered experiment's own `unitType`, not each holdout's declared - // unitType, so only installing THAT unit type can repair the null entry. Resolving the holdout - // live later (rather than evicting and letting `_assign()` rebuild both the decision and its - // exposures together) could publish a holdout verdict inconsistent with the cached experiment - // decision, so we evict instead. - // - // Only unexposed assignments are evicted: eviction lets a later `_assign()`/`_treatment()` call - // recompute (and re-fire) exposure from scratch, so evicting an already-exposed assignment - // could publish a duplicate or contradictory experiment exposure. This is not protecting a - // pristine record — an exposure queued before this call may already carry the late unit, since - // publish() reads units from the live `_units` map — but the decision behind it was made - // without that unit, and recomputation cannot repair a record already queued, only add a - // second, conflicting one. The guard avoids compounding a degraded record. + // A null holdout entry means the unit was missing when the assignment was resolved. Evict so + // `_assign()` rebuilds it; already-exposed ones are kept since re-resolving would queue a + // second, conflicting exposure. private _invalidateAssignmentsPinnedWithMissingUnit(unitType: string): void { for (const experimentName in this._assignments) { const assignment = this._assignments[experimentName]; @@ -643,12 +618,7 @@ export default class Context { return true; }; - // Ported from java-sdk's Context.holdoutSetMatches (Context.java:991-1005). Compares the - // pinned holdout set the cached assignment was built against with the freshly-resolved - // applicable-holdout set by (id, iteration) per entry — not full deep-equality, since - // cosmetic holdout edits (e.g. seed/split changes) on an unrelated field shouldn't force a - // duplicate exposure. Only membership/identity changes (added/removed holdout, or an - // existing one's id/iteration changing) invalidate the cached assignment. + // (id, iteration) only: seed/split edits must not force a duplicate exposure. const holdoutSetMatches = (experiment: Experiment, assignment: Assignment) => { const freshHoldouts = experiment.holdouts ?? []; const pinnedHoldouts = assignment.holdouts ?? []; @@ -676,13 +646,6 @@ export default class Context { if (experimentName in this._assignments) { const assignment = this._assignments[experimentName]; if (hasOverride) { - // The holdout set must be revalidated here too, mirroring the non-override - // branch below — otherwise a holdout that becomes (or stops being) applicable - // to an already-overridden experiment after a refresh is never picked up, and - // assignment.holdouts/holdoutAssignments/suppressed stay frozen forever (Task 6 - // relies on holdoutAssignments to decide which holdouts' own exposures to fire). - // `experiment == null` means there's no live experiment to check against, so - // treat that as trivially matching (nothing to invalidate against). if ( assignment.overridden && assignment.variant === this._overrides[experimentName] && @@ -697,12 +660,7 @@ export default class Context { return assignment; } } else if (assignment.suppressed || !hasCustom || this._cassignments[experimentName] === assignment.variant) { - // When the assignment is currently suppressed, a custom-assignment variant - // mismatch is expected (the holdout forces variant 0 regardless of the custom - // assignment on file per scenario 211) and must not be treated as staleness on - // its own — experimentMatches/audienceMatches/holdoutSetMatches below still gate - // the return, so a real change (unit type, holdout set, etc.) still falls through - // to a rebuild. + // Suppressed assignments are forced to variant 0, so a custom mismatch is not staleness. if ( experimentMatches(experiment.data, assignment) && audienceMatches(experiment.data, assignment) && @@ -732,10 +690,7 @@ export default class Context { this._assignments[experimentName] = assignment; - // Resolve applicable holdouts and compute suppression unconditionally — this must run - // regardless of override/custom-assignment/rule-variant handling below, because a - // holdout's own exposure (fired later, using assignment.holdoutAssignments) must fire - // whether or not the covered experiment itself ends up overridden or suppressed. + // Resolved for overrides too: holdout exposures fire from `holdoutAssignments` regardless. if (experiment != null && experiment.holdouts != null && experiment.holdouts.length > 0) { const holdouts = experiment.holdouts; const holdoutUnitType = experiment.data.unitType; @@ -750,13 +705,6 @@ export default class Context { let suppressed = false; holdoutAssignments.forEach((holdoutAssignment) => { if (holdoutAssignment != null) { - // Read the arm count from the holdout's own pinned Assignment - // (holdoutArmCount), not the live holdout definition (holdouts[i].data.split.length) - // — the pinned Assignment's `variant` was resolved against whatever split - // length was live at that time, and a same-iteration refresh can change - // split.length without invalidating _getHoldoutAssignment's cache, so reading - // the live value here could desync the resolved arm from the arm count used - // to interpret it. See holdoutArmCount's doc comment on the Assignment type. if ( isHeldOutBy( holdoutAssignment.variant, @@ -785,32 +733,14 @@ export default class Context { const unitType = experiment.data.unitType; const attrs = this._getAttributesMap(); - // `ruleKey` is bookkeeping only (a cache key derived from the rules string + env, - // not an evaluation of them against attrs), so it is always kept up to date — - // including when suppressed — mirroring `attrsSeq` below, which is also set - // unconditionally. Without this, a suppressed assignment would leave `ruleKey` - // unset, and the cache-validity check in `audienceMatches` (above) would see - // `ruleKeyChanged` as permanently true on every subsequent call for an experiment - // with assignmentRules, forcing a full rebuild (losing `assignment.exposed`) on - // every single treatment()/peek() call instead of only on a genuine change. + // Set even when suppressed, or `audienceMatches` rebuilds (losing `exposed`) on every call. assignment.ruleKey = experiment.data.assignmentRules ? `${experiment.data.assignmentRules}:${this._environmentName}` : ""; - // Suppression is checked FIRST, before assignment rules (or audience, or the - // traffic-split/fullOn path) get any say over the variant. Assignment rules are a - // deterministic-per-attribute assignment mechanism — structurally the same category - // as a custom assignment (scenario 211: custom assignment yields to suppression) — - // not an override in the sense scenario 210 establishes (only an explicit override() - // call is exempt from suppression). If a matching rule were allowed to set the - // variant before this check, a held-out unit would be silently TREATED with the - // rule's variant while its exposure-firing gate - // (`!assignment.suppressed || assignment.overridden`, which does NOT include - // `ruleOverride`) still suppresses its own exposure — the worst combination: - // measured nothing, but received real treatment. Gating here means - // `ruleVariant`/`ruleOverride` are never computed nor set when suppressed, so the - // exposure gate needs no `ruleOverride` special-case: a suppressed assignment never - // has `ruleOverride: true` in the first place. + // Suppression wins over assignment rules (only override() is exempt). The exposure gate + // does not exempt `ruleOverride`, so a rule getting through here would treat the unit + // while suppressing its exposure. if (assignment.suppressed) { assignment.assigned = false; assignment.variant = 0; @@ -919,18 +849,7 @@ export default class Context { return assignment; } - // Shared by `_treatment` and `_variableValue`, whose exposure-firing behavior is otherwise - // identical once the one-shot `exposed` gate has been checked. private _triggerExposures(experimentName: string, assignment: Assignment): void { - // An override always fires its own exposure, even when the covered experiment is also - // suppressed by a holdout: overriding replaces the resolved variant outright (the override's - // value wins, not the holdout's), so its own exposure must still be observable. This mirrors - // java-sdk's outcome for the override path (Context.java:1184-1244, triggerExposure at - // Context.java:481-503) — java's override write path never sets `assignment.suppressed` at - // all, so its own `!assignment.suppressed` exposure gate trivially always passes there. Our - // JS port pins `suppressed` eagerly for the override path too (Task 4's deliberate - // divergence, see Assignment.suppressed doc comment), so the exposure gate here must - // special-case `overridden` explicitly to reproduce the same firing outcome. if (!assignment.suppressed || assignment.overridden) { this._queueExposure(experimentName, assignment); } @@ -938,11 +857,8 @@ export default class Context { this._triggerApplicableHoldoutExposures(assignment); } - // Ported from java-sdk's triggerApplicableHoldoutExposures/triggerHoldoutExposure - // (Context.java:481-546). Fires each applicable holdout's own exposure exactly once, - // using the pinned `assignment.holdoutAssignments` snapshot (not a live re-resolution), - // so a data refresh landing between the suppression decision and the exposure trigger - // can't publish a holdout exposure from a different epoch (Context.java:505-514). + // Uses the pinned snapshot, not a live re-resolution, so a refresh between the suppression + // decision and here can't publish a holdout exposure from a different epoch. private _triggerApplicableHoldoutExposures(assignment: Assignment): void { const holdouts = assignment.holdouts; const holdoutAssignments = assignment.holdoutAssignments; @@ -1077,17 +993,8 @@ export default class Context { return this._customFieldValueType(experimentName, key); } - // Ported from java-sdk's getVariableAssignment (Context.java:1332-1373). A suppressed - // (held-out) experiment never wins resolution over a genuinely assigned/overridden/rule-matched - // one sharing the same key, but it IS used as a fallback when no candidate wins: a held-out - // unit must still read the experiment's control-variant value, not the caller's default, so it - // stays indistinguishable from a control unit. The first-encountered suppressed candidate is - // captured as the fallback regardless of whether it defines the key (matching java, which pins - // the whole Assignment and only checks the key afterwards) — an earlier suppressed candidate - // lacking the key must not let a later suppressed candidate that does have it win instead. - // Overridden assignments are excluded from the capture even if `suppressed` is set: java's - // override path never sets `suppressed` at all (see Assignment.suppressed doc comment for why - // the JS port's override path pins it anyway), so an override can never become java's fallback. + // A held-out unit reads the control value, not the caller's default, when nothing else wins. + // The first suppressed candidate is captured even if it lacks the key, so a later one can't win. private _resolveVariableValue(key: string, defaultValue: string, shouldQueueExposure: boolean): string { let suppressedFallback: Record | undefined; @@ -1460,15 +1367,7 @@ export default class Context { } if (!(unitType in this._hashes)) { - // Only cache when the unit is actually available. A `null` result here means the unit - // hasn't been set yet — that can change later (via `unit()`/`setUnit`), whereas a - // resolved hash is stable for the unit's lifetime (the same unit type can only ever be - // set once, enforced by `unit()`). Caching `null` would permanently poison this cache - // for a unit type queried before it was set — e.g. `_getHoldoutAssignment` (unlike the - // ordinary experiment-assignment path, which only calls `_unitHash` after already - // checking `unitType in this._units`) calls this unconditionally, so a holdout resolved - // via `peek()`/`_assign()` before its unit type is installed must be able to resolve - // correctly once that unit later arrives, without a stale cached `null` blocking it. + // Never cache `null`: the unit can still be set later (see `_getHoldoutAssignment`). if (!(unitType in this._units)) { return null; } @@ -1481,17 +1380,8 @@ export default class Context { return this._hashes[unitType]; } - // Resolves the arm a unit falls into within a holdout itself (as opposed to resolving an - // ordinary experiment's assignment, which is `_assign()`). Ported from java-sdk's - // getHoldoutAssignment (Context.java:1412-1468), minus the read/write-lock dance: js is - // single-threaded, so this simplifies to a plain memoized-by-(id, unitType) cache. - // - // `holdout` may be a stale reference (e.g. captured before a data refresh) so it is always - // re-resolved against the live `_holdoutsById` index first (falling back to the caller-supplied - // reference only if the id is no longer present, e.g. the holdout was removed by the latest - // refresh) — this mirrors java-sdk's resolveLiveHoldout and ensures the cache is keyed and - // validated against the currently-installed definition rather than a possibly-dead one. private _getHoldoutAssignment(holdout: HoldoutExperiment, unitType: string): Assignment | null { + // `holdout` may predate a refresh; prefer the live definition. const liveHoldoutData = this._holdoutsById[holdout.data.id] ?? holdout.data; const cacheKey = `${liveHoldoutData.id}:${unitType}`; @@ -1502,8 +1392,6 @@ export default class Context { const unit = this._unitHash(unitType); if (unit === null) { - // No unit set for this unitType yet — mirrors java-sdk's `uid == null -> return null`. - // Do not cache: a later call, once the unit is set, must recompute. return null; } @@ -1539,11 +1427,6 @@ export default class Context { const index: Record = {}; const indexVariables: Record = {}; - // Live index of holdout definitions by id, skipping holdouts with no/empty - // split (they can never be assigned to, so they are treated as non-existent). - // Kept as raw HoldoutData (not a resolved HoldoutExperiment/Assignment) so later - // lookups (e.g. resolving a holdout's own assignment) always read against the - // currently-installed data rather than a possibly-stale cached reference. const holdoutsById: Record = {}; (data.holdouts || []).forEach((holdout) => { @@ -1554,9 +1437,6 @@ export default class Context { this._holdoutsById = holdoutsById; - // Experiment wrappers (data only) for holdouts, built lazily and - // memoized per _init() call so a holdout referenced by multiple experiments is - // only parsed once. const holdoutExperiments: Record = {}; const resolveHoldoutExperiment = (holdoutId: number): HoldoutExperiment | undefined => { @@ -1564,8 +1444,6 @@ export default class Context { return holdoutExperiments[holdoutId]; } - // Read via the live field (not the local `holdoutsById` closure) so this - // always resolves against the currently-installed data. const holdoutData = this._holdoutsById[holdoutId]; if (!holdoutData) { return undefined; From ba55641ddf2efc5af6607bfbc808281da96350d1 Mon Sep 17 00:00:00 2001 From: Jonas Alves Date: Wed, 23 Sep 2026 18:38:40 +0100 Subject: [PATCH 26/28] refactor(holdouts): use Array.some for short-circuiting suppression check (FT-2206) Addresses PR review feedback: the forEach-with-mutable-flag loop computing assignment.suppressed can short-circuit on the first suppressing holdout instead of always resolving every entry's boolean. --- src/context.ts | 21 +++++---------------- 1 file changed, 5 insertions(+), 16 deletions(-) diff --git a/src/context.ts b/src/context.ts index 9087587..201b6d2 100644 --- a/src/context.ts +++ b/src/context.ts @@ -702,22 +702,11 @@ export default class Context { assignment.holdouts = holdouts; assignment.holdoutAssignments = holdoutAssignments; - let suppressed = false; - holdoutAssignments.forEach((holdoutAssignment) => { - if (holdoutAssignment != null) { - if ( - isHeldOutBy( - holdoutAssignment.variant, - holdoutAssignment.holdoutArmCount ?? 0, - experiment.data.fullOnVariant - ) - ) { - suppressed = true; - } - } - }); - - assignment.suppressed = suppressed; + assignment.suppressed = holdoutAssignments.some( + (holdoutAssignment) => + holdoutAssignment != null && + isHeldOutBy(holdoutAssignment.variant, holdoutAssignment.holdoutArmCount ?? 0, experiment.data.fullOnVariant) + ); } if (hasOverride) { From 2383344f883567dd852d5fd1de3cc0cf176f49aa Mon Sep 17 00:00:00 2001 From: Jonas Alves Date: Thu, 24 Sep 2026 17:51:54 +0100 Subject: [PATCH 27/28] fix(holdouts): let a matching assignment rule win over holdout suppression (FT-2206) A rule match is an explicit, author-specified assignment (flagged ruleOverride, excluded from stats) rather than the experiment's own randomized/custom-assignment path that suppression is meant to gate, so it's exempt from suppression the same way override() already is. Exposure gate and variable-fallback capture updated to match: a rule-driven exposure must fire when the rule wins, and a rule-exempted assignment must not be captured as the suppressed fallback for variable resolution. --- src/__tests__/context.test.js | 57 ++++++++------- src/context.ts | 134 +++++++++++++++++----------------- 2 files changed, 99 insertions(+), 92 deletions(-) diff --git a/src/__tests__/context.test.js b/src/__tests__/context.test.js index 7dc4e92..95829e9 100644 --- a/src/__tests__/context.test.js +++ b/src/__tests__/context.test.js @@ -4714,20 +4714,14 @@ describe("Context", () => { }); }); - // Final-review Finding I-1: a matching assignmentRules rule (a JS-SDK-only feature) must - // NOT bypass holdout suppression. Rules are a deterministic-per-attribute assignment - // mechanism — structurally the same category as a custom assignment (scenario 211: custom - // assignment yields to suppression) — not an override in the sense scenario 210 - // establishes (only an explicit override() call is exempt from suppression). Before the - // fix, `ruleVariant !== null` set `assignment.variant`/`ruleOverride` unconditionally, - // bypassing the `if (assignment.suppressed)` branch entirely: a held-out unit would be - // silently TREATED with the rule's variant while its own exposure stayed suppressed (the - // exposure gate does not special-case `ruleOverride`) — measured nothing, but received - // real treatment. Ported shape from scenario 211 (custom-assignment path) applied to the - // rules path instead; the holdout fixture (id 11, seedHi 13, seedLo 111, split [0.1, 0.9]) - // is identical to scenario 203's holdout_a, which is already proven to hold out the - // default `contextParams` unit (variant 0). - it("suppresses a matching assignment rule's variant, mirroring scenario 211 for the rules path (Finding I-1)", (done) => { + // A matching assignmentRules rule (a JS-SDK-only feature) wins over holdout suppression, + // the same way an explicit override() does: it's an author-specified assignment (flagged + // `ruleOverride`, excluded from stats), not the experiment's own randomized/custom path + // that suppression is meant to gate. The holdout fixture (id 11, seedHi 13, seedLo 111, + // split [0.1, 0.9]) is identical to scenario 203's holdout_a, which is already proven to + // hold out the default `contextParams` unit — proving suppression is genuinely in play + // here and the rule is what overrides it, not merely the holdout never applying. + it("lets a matching assignment rule's variant win over holdout suppression", (done) => { const response = buildHoldoutResponse( [ { @@ -4793,27 +4787,36 @@ describe("Context", () => { const context = new Context(sdk, contextOptions, contextParams, response); context.attribute("country", "US"); - // Without the fix: the matching rule (variant 1) is applied unconditionally, bypassing - // suppression entirely, so this would return 1 instead of 0. - expect(context.treatment("exp_holdout_rules")).toEqual(0); + // The rule wins: variant 1 is returned even though this unit is held out by + // holdout_rules_suppression. + expect(context.treatment("exp_holdout_rules")).toEqual(1); - // White-box check (the exposure that would carry these fields never fires, since the - // experiment's own exposure is correctly suppressed below — see the publish assertion) - // to confirm the rule-variant computation branch did not run at all when suppressed: - // `ruleOverride` must stay false (never set to true and then have `variant` - // overwritten to 0 afterward) and `assigned` must be false, matching every other - // suppression case. + // `ruleOverride` is set and `assigned` stays false (a rule match is not a randomized + // assignment), matching the semantics of every other rule-matched assignment. const assignment = context._assignments["exp_holdout_rules"]; - expect(assignment.ruleOverride).toBeFalsy(); + expect(assignment.ruleOverride).toEqual(true); expect(assignment.assigned).toEqual(false); publisher.publish.mockReturnValue(Promise.resolve()); context.publish().then(() => { - // Only the holdout's own exposure fires — the experiment's own exposure must NOT - // fire, exactly like scenario 211 (custom assignment yields to suppression), just - // via the rules path instead of the custom-assignment path. + // Both the experiment's own exposure (via the rule) and the holdout's own exposure + // fire — a rule match is exempt from suppression the same way override() is. expect(publisher.publish.mock.calls[0][0].exposures).toEqual([ + { + id: 1, + name: "exp_holdout_rules", + unit: "session_id", + exposedAt: timeOrigin, + variant: 1, + assigned: false, + eligible: true, + overridden: false, + fullOn: false, + custom: false, + audienceMismatch: false, + ruleOverride: true, + }, { id: 11, name: "holdout_rules_suppression", diff --git a/src/context.ts b/src/context.ts index 201b6d2..62c9348 100644 --- a/src/context.ts +++ b/src/context.ts @@ -727,81 +727,80 @@ export default class Context { ? `${experiment.data.assignmentRules}:${this._environmentName}` : ""; - // Suppression wins over assignment rules (only override() is exempt). The exposure gate - // does not exempt `ruleOverride`, so a rule getting through here would treat the unit - // while suppressing its exposure. - if (assignment.suppressed) { + let ruleVariant: number | null = null; + + if (experiment.data.assignmentRules && experiment.data.assignmentRules.length > 0) { + ruleVariant = this._computeRuleVariant( + experiment.data.assignmentRules, + experiment.data.variants.length, + attrs + ); + } + + assignment.ruleVariant = ruleVariant; + + // A matching assignment rule wins over holdout suppression, the same way override() + // does: it's an explicit, author-specified assignment (flagged `ruleOverride`, + // excluded from stats) rather than the experiment's own randomized/custom path, so + // suppression only applies when no rule matched. + if (ruleVariant !== null) { + assignment.variant = ruleVariant; + assignment.ruleOverride = true; + } else if (assignment.suppressed) { assignment.assigned = false; assignment.variant = 0; } else { - let ruleVariant: number | null = null; - - if (experiment.data.assignmentRules && experiment.data.assignmentRules.length > 0) { - ruleVariant = this._computeRuleVariant( - experiment.data.assignmentRules, - experiment.data.variants.length, - attrs - ); - } - - assignment.ruleVariant = ruleVariant; + if (experiment.data.audience && experiment.data.audience.length > 0) { + const result = this._audienceMatcher.evaluate(experiment.data.audience, attrs); - if (ruleVariant !== null) { - assignment.variant = ruleVariant; - assignment.ruleOverride = true; - } else { - if (experiment.data.audience && experiment.data.audience.length > 0) { - const result = this._audienceMatcher.evaluate(experiment.data.audience, attrs); - - if (typeof result === "boolean") { - assignment.audienceMismatch = !result; - } + if (typeof result === "boolean") { + assignment.audienceMismatch = !result; } + } - if (experiment.data.audienceStrict && assignment.audienceMismatch) { - assignment.variant = 0; - } else if (experiment.data.fullOnVariant === 0) { - if (unitType !== null) { - if (unitType in this._units) { - const unit = this._unitHash(unitType); - if (unit !== null) { - const assigner = - unitType in this._assigners - ? this._assigners[unitType] - : (this._assigners[unitType] = new VariantAssigner(unit)); - const eligible = - assigner.assign( - experiment.data.trafficSplit, - experiment.data.trafficSeedHi, - experiment.data.trafficSeedLo - ) === 1; - - assignment.assigned = true; - assignment.eligible = eligible; - - if (eligible) { - if (hasCustom) { - assignment.variant = this._cassignments[experimentName]; - assignment.custom = true; - } else { - assignment.variant = assigner.assign( - experiment.data.split, - experiment.data.seedHi, - experiment.data.seedLo - ); - } + if (experiment.data.audienceStrict && assignment.audienceMismatch) { + assignment.variant = 0; + } else if (experiment.data.fullOnVariant === 0) { + if (unitType !== null) { + if (unitType in this._units) { + const unit = this._unitHash(unitType); + if (unit !== null) { + const assigner = + unitType in this._assigners + ? this._assigners[unitType] + : (this._assigners[unitType] = new VariantAssigner(unit)); + const eligible = + assigner.assign( + experiment.data.trafficSplit, + experiment.data.trafficSeedHi, + experiment.data.trafficSeedLo + ) === 1; + + assignment.assigned = true; + assignment.eligible = eligible; + + if (eligible) { + if (hasCustom) { + assignment.variant = this._cassignments[experimentName]; + assignment.custom = true; } else { - assignment.variant = 0; + assignment.variant = assigner.assign( + experiment.data.split, + experiment.data.seedHi, + experiment.data.seedLo + ); } + } else { + assignment.variant = 0; } } } - } else { - assignment.assigned = true; - assignment.eligible = true; - assignment.variant = experiment.data.fullOnVariant; - assignment.fullOn = true; } + } else { + assignment.assigned = true; + assignment.eligible = true; + assignment.variant = experiment.data.fullOnVariant; + assignment.fullOn = true; } } @@ -839,7 +838,7 @@ export default class Context { } private _triggerExposures(experimentName: string, assignment: Assignment): void { - if (!assignment.suppressed || assignment.overridden) { + if (!assignment.suppressed || assignment.overridden || assignment.ruleOverride) { this._queueExposure(experimentName, assignment); } @@ -1001,7 +1000,12 @@ export default class Context { return assignment.variables[key] as string; } - if (assignment.suppressed && !assignment.overridden && suppressedFallback === undefined) { + if ( + assignment.suppressed && + !assignment.overridden && + !assignment.ruleOverride && + suppressedFallback === undefined + ) { suppressedFallback = assignment.variables; } } From d7c74b9727438ee7eb668413d2709bea38a5b896 Mon Sep 17 00:00:00 2001 From: Jonas Alves Date: Fri, 25 Sep 2026 10:52:26 +0100 Subject: [PATCH 28/28] test(holdouts): drop two restating comments flagged in review round 6 (FT-2206) Both lines just repeated the assertion immediately below them (toEqual(1) and the exposures array). The rationale comments nearby (rule-precedence design note, fixture note, assigned/ruleOverride semantics note) stay, since they explain why, not what. --- src/__tests__/context.test.js | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/__tests__/context.test.js b/src/__tests__/context.test.js index 95829e9..b5a400a 100644 --- a/src/__tests__/context.test.js +++ b/src/__tests__/context.test.js @@ -4787,8 +4787,6 @@ describe("Context", () => { const context = new Context(sdk, contextOptions, contextParams, response); context.attribute("country", "US"); - // The rule wins: variant 1 is returned even though this unit is held out by - // holdout_rules_suppression. expect(context.treatment("exp_holdout_rules")).toEqual(1); // `ruleOverride` is set and `assigned` stays false (a rule match is not a randomized @@ -4800,8 +4798,6 @@ describe("Context", () => { publisher.publish.mockReturnValue(Promise.resolve()); context.publish().then(() => { - // Both the experiment's own exposure (via the rule) and the holdout's own exposure - // fire — a rule match is exempt from suppression the same way override() is. expect(publisher.publish.mock.calls[0][0].exposures).toEqual([ { id: 1,