feat: holdout support — normalized models, assignment precedence, exposure fields (ISS-61) - #12
Conversation
…osure fields, tests Squashed restore of feat/holdouts work (original commits lost to a tooling accident; content fully preserved): - ExperimentHoldout json model; ContextData.holdouts[] + Experiment.holdoutIds[] (normalized payload: definitions not duplicated per experiment) - Context: holdout check after override, before audience/traffic/variant (Override > Holdout > Audience > Traffic > Variant); id->def map with graceful skip of unknown ids; cache invalidation on holdoutIds and resolved definition changes - Exposure heldOut + holdoutId, serializer round-trip - ContextHoldoutTest matrix + serializer/deserializer coverage
Add missing exposure assertion for the not-held-out-but-has-holdouts path (heldOut=false/holdoutId=0 after holdouts are actually evaluated), plus empty-holdoutIds and empty-split branch coverage. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Reuse the already-bound unitType local in the traffic-split branch instead of re-reading experiment.data.unitType, matching the new holdout branch. Behaviour-preserving. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds experiment holdout models and context data, resolves configured holdouts, evaluates holdout membership during assignment, and records held-out state in exposures. Cache matching now includes holdout metadata. The change also adds deserialisation, assignment, precedence, refresh, full-on, publication, and serialisation coverage with supporting fixtures. Estimated code review effort: 4 (Complex) | ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
core-api/src/test/java/com/absmartly/sdk/ContextHoldoutTest.java (1)
223-250: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRepeated publish-assertion boilerplate.
The
PublishEvent/when(eventHandler.publish...)/verify(...)sequence is duplicated acrossskipsHoldoutWhenUnitMissingForUnitType,exposureCarriesHeldOutAndHoldoutId, andexposureCarriesNotHeldOutFieldsWhenUnitNotInHoldout. Consider extracting a small helper (e.g.assertPublishedExposure(context, expectedExposures)) to reduce repetition.Also applies to: 358-414
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core-api/src/test/java/com/absmartly/sdk/ContextHoldoutTest.java` around lines 223 - 250, The publish assertion setup is duplicated in ContextHoldoutTest across multiple holdout tests, including skipsHoldoutWhenUnitMissingForUnitType, exposureCarriesHeldOutAndHoldoutId, and exposureCarriesNotHeldOutFieldsWhenUnitNotInHoldout. Extract the repeated PublishEvent construction, when(eventHandler.publish(...)) stubbing, and verify(...) timeout assertion into a small helper such as assertPublishedExposure or assertPublishEvent so the tests stay focused on the scenario-specific expectations.core-api/src/main/java/com/absmartly/sdk/Context.java (1)
786-865: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider de-duplicating the unit lookup in the holdout/traffic-split branches.
The
uid = units_.get(unitType)lookup (plus the follow-ongetUnitHash/getVariantAssignercalls) is repeated once for holdout evaluation and again for traffic-split evaluation. Hoisting theuidlookup once at the top of this block would remove the duplication and slightly reduce the complexity of an already dense method (holdout precedence, audience matching, full-on/traffic logic are all interleaved here).♻️ Suggested simplification
if (experiment != null) { final String unitType = experiment.data.unitType; + final String uid = units_.get(unitType); assignment.holdoutIds = experiment.data.holdoutIds; assignment.holdouts = experiment.holdouts; - if (experiment.holdouts != null && experiment.holdouts.length > 0) { - final String uid = units_.get(unitType); - if (uid != null) { - final byte[] unitHash = Context.this.getUnitHash(unitType, uid); - final VariantAssigner assigner = Context.this.getVariantAssigner(unitType, unitHash); - for (final ExperimentHoldout holdout : experiment.holdouts) { + if (uid != null && experiment.holdouts != null && experiment.holdouts.length > 0) { + final byte[] unitHash = Context.this.getUnitHash(unitType, uid); + final VariantAssigner assigner = Context.this.getVariantAssigner(unitType, unitHash); + for (final ExperimentHoldout holdout : experiment.holdouts) { if (assigner.assign(holdout.split, holdout.seedHi, holdout.seedLo) == 0) { assignment.heldOut = true; assignment.holdoutId = holdout.id; assignment.variant = 0; assignment.assigned = true; break; } - } - } + } } if (!assignment.heldOut) { ... } else if (experiment.data.fullOnVariant == 0) { - final String uid = units_.get(unitType); if (uid != null) { final byte[] unitHash = Context.this.getUnitHash(unitType, uid); final VariantAssigner assigner = Context.this.getVariantAssigner(unitType, unitHash); ... } } else { ... } } ... }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core-api/src/main/java/com/absmartly/sdk/Context.java` around lines 786 - 865, The unit lookup and assigner setup in the experiment assignment flow is duplicated in both the holdout and traffic-split branches. In Context’s assignment block, hoist the units_.get(unitType) lookup (and, if available, the derived getUnitHash/getVariantAssigner setup) once before the conditional branches, then reuse it for holdout and eligibility evaluation. Keep the existing holdout precedence and audience/full-on behavior unchanged while simplifying the repeated logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@core-api/src/main/java/com/absmartly/sdk/Context.java`:
- Around line 786-865: The unit lookup and assigner setup in the experiment
assignment flow is duplicated in both the holdout and traffic-split branches. In
Context’s assignment block, hoist the units_.get(unitType) lookup (and, if
available, the derived getUnitHash/getVariantAssigner setup) once before the
conditional branches, then reuse it for holdout and eligibility evaluation. Keep
the existing holdout precedence and audience/full-on behavior unchanged while
simplifying the repeated logic.
In `@core-api/src/test/java/com/absmartly/sdk/ContextHoldoutTest.java`:
- Around line 223-250: The publish assertion setup is duplicated in
ContextHoldoutTest across multiple holdout tests, including
skipsHoldoutWhenUnitMissingForUnitType, exposureCarriesHeldOutAndHoldoutId, and
exposureCarriesNotHeldOutFieldsWhenUnitNotInHoldout. Extract the repeated
PublishEvent construction, when(eventHandler.publish(...)) stubbing, and
verify(...) timeout assertion into a small helper such as
assertPublishedExposure or assertPublishEvent so the tests stay focused on the
scenario-specific expectations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 00fd9122-82f1-4830-b808-f01c239e7192
📒 Files selected for processing (12)
core-api/src/main/java/com/absmartly/sdk/Context.javacore-api/src/main/java/com/absmartly/sdk/json/ContextData.javacore-api/src/main/java/com/absmartly/sdk/json/Experiment.javacore-api/src/main/java/com/absmartly/sdk/json/ExperimentHoldout.javacore-api/src/main/java/com/absmartly/sdk/json/Exposure.javacore-api/src/test/java/com/absmartly/sdk/ContextHoldoutTest.javacore-api/src/test/java/com/absmartly/sdk/ContextTest.javacore-api/src/test/java/com/absmartly/sdk/DefaultContextDataDeserializerTest.javacore-api/src/test/java/com/absmartly/sdk/DefaultContextEventSerializerTest.javacore-api/src/test/java/com/absmartly/sdk/json/ContextDataTest.javacore-api/src/test/java/com/absmartly/sdk/json/ExperimentHoldoutTest.javacore-api/src/test/resources/holdouts_context.json
- Assignment cache: treat held-out assignments as up-to-date even when a custom assignment is set. Holdout wins over custom assignment, so the cached variant (0) can never equal the custom variant; the old condition forced a cache miss and a fresh Assignment (exposed=false) on every call, queueing a duplicate exposure per getTreatment. - Tests: cached held-out assignment with custom assignment set; holdout precedence over fullOnVariant. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
When a custom assignment is set but the cached variant was forced by a higher-precedence rule (holdout, full-on, traffic ineligibility, or a strict audience mismatch), the custom value can never equal that variant. The old cache-validity check compared them directly, so it forced a cache miss and a fresh Assignment (exposed=false) on every getTreatment call, queueing a duplicate exposure each time. Introduce variantForcedRegardlessOfCustom() covering all forced-variant cases and treat them as cache-valid. 5af036a fixed the holdout case only; this generalizes the same fix to the audience-strict-mismatch and traffic-ineligible cases, which are pre-existing on main. Tests: cached forced-variant assignments with a custom assignment set for both the strict audience mismatch and traffic ineligible cases (stable pending count across repeated calls). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Assignment.holdoutIds/holdouts alias the arrays owned by ContextExperiment instead of copying them. This is safe only because experiment data is treated as immutable after setData and the arrays are read-only here. Note the invariant at the assignment site. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ash parity The gpt-5.6-sol pass changed the SDK normalizer to 1.0/0x100000000 to make the probability range half-open [0,1). But the collector (and every other ABsmartly SDK) uses 1.0/0xffffffff, where the max 32-bit hash maps to exactly 1.0. Assignment requires bit-exact hash parity between the SDK and the collector -- diverging the normalizer would make the SDK and server disagree on borderline units (including holdout membership). Reverted VariantAssigner to match main/collector; removed the range test that asserted the divergent behaviour.
Add optional fullOn field to ExperimentHoldout (tolerant, defaults to null/absent = full holdout, unchanged behavior). A full_on holdout only applies to full-on experiments (fullOnVariant != 0); for other experiments the SDK skips it entirely during heldOut evaluation, matching the collector's server-side behavior bit-exactly.
Holdouts arrive as ordinary Experiment entries in ContextData.holdouts, carrying holdoutType (full/full_on) and excludedExperimentIds instead of a separate ExperimentHoldout model. Experiment.holdoutIds is removed: applicability is derived client-side from unit type and the full/full_on rule, not served per-experiment. Exposure drops heldOut/holdoutId, which no longer exist server-side; a held-out unit emits no exposure for the experiments it covers instead of a tagged one.
Suppress a covered experiment's own exposure and force control values whenever the unit is variant 0 in any applicable holdout, instead of tagging the exposure with the held-out holdout's id. Applicability is now derived per experiment at data-install time from unit type, full_on/fullOnVariant, and excludedExperimentIds, since holdouts are served as ordinary experiment entries rather than a separate id-keyed model. The holdout experiment itself is assigned and cached like any other experiment (by holdout id, since it lives outside the experiments index) and emits one ordinary exposure the first time any experiment applicable to its unit type is evaluated, regardless of whether that triggering experiment's own exposure is suppressed.
…odel Replace the id-lookup ExperimentHoldout fixtures across ContextHoldoutTest, DefaultContextDataDeserializerTest, ContextDataTest, and holdouts_context.json with full Experiment entries carrying holdoutType and excludedExperimentIds. Cover suppression, control values, exclusion, the union rule across multiple holdouts, full vs full_on applicability, application-scoping independence, absent holdouts, custom-assignment precedence, and once-per-context holdout exposure emission, plus cross-SDK parity vectors for unicode unit ids, signed seed halves, and percentage/probability boundaries. Expected variants are computed from VariantAssigner rather than assumed.
…hrash The holdout Assignment cache and experimentMatches both invalidated on fields that never change who is a member: HoldoutAssignment.matches compared seedHi/seedLo/split, and experimentMatches compared applicable holdouts with Arrays.equals over Experiment (name, variants, applications, audience, customFieldValues included). Either kind of edit replaced the cached Assignment, reset its once-per-context exposed flag, and let an already-exposed unit be re-assigned into the other arm of the same holdout - violating the invariant that a unit must never appear in both arms of a holdout within one context's lifetime. Both invalidation paths (HoldoutAssignment.matches and experimentMatches's holdout comparison) and the cache key/replacement path in getHoldoutAssignment now agree on the same identity: a holdout's (id, iteration) pair, mirroring the granularity experimentMatches already used for ordinary experiments. Only an iteration bump - a genuine re-randomization epoch - replaces a cached verdict; seed, split, and any cosmetic field can change freely without disturbing an already-recorded arm.
Setting `assigned = true` on the suppressed path made a held-out unit's assignment indistinguishable from a genuinely assigned one in two ways that only affect the held-out arm: 1. getVariableAssignment / getVariableValue reach queueExposure through `assigned || overridden`. A held-out unit satisfied that check via suppression and fired the holdout's own exposure on a variable lookup; a non-held-out unit whose strict-audience check failed left `assigned` false and never did, for the identical experiment and attributes - an emission asymmetry between the two arms of the same holdout. 2. Two experiments sharing a variable key are resolved by picking the first one with `assigned == true`. A suppressed experiment's forced `assigned = true` let it win that resolution over an experiment the unit is genuinely assigned to, hijacking the key. `assigned` now stays false for a suppressed assignment. Emission symmetry is restored by decoupling the holdout trigger from `assigned`: getVariableAssignment falls back to a suppressed match only when nothing else claims the key, so a held-out unit's variable lookup still reaches queueExposure (and so still fires the holdout's own exposure) without ever letting the suppressed experiment win over a real assignment.
…ache hits getHoldoutAssignment read units_ before acquiring any lock, racing with setUnit's write-locked mutation of the same map. It also always took contextLock_'s exclusive write lock, serialising every holdout-covered evaluation in a context even when the result was already cached. The unit lookup and cache lookup now happen under the read lock, matching the pattern used elsewhere in this class (getUnit, setUnit). On a cache miss, the write lock is acquired and both lookups are repeated before computing a new assignment, so two threads racing on the same uncached holdout id can never install two different Assignment objects for it - the double-check preserves the once-per- context exposure invariant the (id, iteration) cache identity depends on.
… logger failure isExcluded binary-searches excludedExperimentIds, but the wire never guaranteed the array is sorted; an unsorted array made binary search return false negatives, silently letting a holdout wrongly cover an experiment it should have excluded (or vice versa). setData now sorts each holdout's excludedExperimentIds once at data-install time. queueExposure's single try-free call sequence meant a throwing ContextEventLogger aborted the whole method: `exposed` is CAS'd true before any of the per-holdout triggers run, so any holdout after the one that threw was skipped and could never be retried for the rest of the context's life - a permanent, silent loss of that holdout's exposure. The own-experiment enqueue and each holdout trigger are now individually guarded; every holdout still gets a chance to fire even if an earlier one's logger call throws, and the first failure is re-thrown once the loop completes so it is never swallowed.
…edge cases Add a multi-holdout test where the low-id holdout does not hold the unit out and a higher-id one does, verifying real assigner output rather than relying on ordering that let a first-holdout-only bug pass unnoticed. Add coverage for an experiment whose unit type has no configured unit at all, proving getHoldoutAssignment treats a missing unit as not evaluable instead of relying on an unexercised null guard. Add coverage for malformed holdout entries on the wire (a null array element, a null split, and an empty split), proving setData's filter drops each one rather than indexing it as applicable.
… methods variantForcedRegardlessOfCustom no longer checks assignment.suppressed: a suppressed assignment always has assigned == false, so !assignment.assigned already covers it. queueExposure/queueHoldoutExposure are renamed to triggerExposure/triggerHoldoutExposure to distinguish them from enqueueExposure, which is the one that actually appends to the pending exposure buffer. Also trims a few test comments that referenced prior review rounds or numbered an external spec instead of describing the invariant under test.
… as exposure Suppression was recomputing each applicable holdout's arm directly from the live seed/split/VariantAssigner, while getHoldoutAssignment pins that same holdout's arm by (id, iteration) for exposure purposes. A same-iteration seed refresh could flip the direct recomputation to a different arm than the one already pinned and exposed, letting an already-exposed holdout-variant-0 unit receive a covered experiment's treatment and exposure. Suppression now reads each holdout's arm from its pinned HoldoutAssignment, so exposure and suppression always agree.
…didate getVariableAssignment resolves a variable key by walking every experiment that defines it, but only ever triggered exposure for the assignment ultimately returned. A lower-id covered experiment that was evaluated and suppressed - then lost the key to a later, genuinely assigned experiment - never fired its own applicable holdouts, violating the first-evaluation trigger contract. The non-peek path now fires each candidate's applicable holdouts as it is visited, while the ordinary experiment exposure is still emitted only for the selected assignment. peekVariableValue stays side- effect free by skipping the trigger entirely.
The override branch of getAssignment copied id and unitType but never attached experiment.holdouts, so getTreatment on an overridden experiment emitted its overridden exposure without evaluating any applicable holdout. An override only replaces the returned variant; the experiment is still evaluated, so its applicable holdouts must still fire, exactly as they do on the non-overridden path.
Both branches constructed the same ExperimentVariant, which FindBugs reports as DB_DUPLICATE_BRANCHES and fails :core-api:findbugsTest under the JDK 8 toolchain used by CI. No caller requires a third variant.
…ecks getHoldoutAssignment trusted whichever Experiment object the caller passed when comparing against the pinned HoldoutAssignment and, on a cache miss, when computing a fresh one. A caller can hold a stale reference (e.g. a holdouts array captured before the most recent refresh) whose iteration no longer matches what is currently installed; recomputing from it would overwrite a cache entry a concurrent, genuinely newer evaluation already installed and exposed, producing a duplicate same-epoch exposure with a contradictory arm. Add an id-keyed index of the currently installed holdouts (holdoutsById_) and resolve the live definition by id before every matches()/compute step in getHoldoutAssignment, falling back to the caller-supplied reference only when the id is no longer present in the installed data. Id is stable across installs, so this removes any ambiguity about which definition is newer without weakening the (id, iteration) pinning itself.
The cached-override fast path in getAssignment returned as soon as assignment.overridden && assignment.variant == override held, without ever checking holdoutSetMatches - unlike the ordinary branch, which invalidates via experimentMatches whenever coverage changes. A holdout becoming applicable to an overridden experiment after a refresh would therefore never fire for that experiment again. Add the same holdoutSetMatches check used by the ordinary path to the override fast path, so a coverage change invalidates an overridden assignment exactly as it would an ordinary one.
setTimeout() was only called from triggerExposure, but enqueueExposure is also reached directly from the variable-key resolution path when a losing/suppressed candidate's applicable holdout fires while the winning assignment was already exposed. That path never called triggerExposure for the holdout's own exposure, so the enqueued exposure could sit unflushed until an unrelated event, an explicit publish(), or close(). Move the setTimeout() call into enqueueExposure itself, so every enqueued exposure - ordinary or holdout, from any call site - schedules its own flush. setTimeout() is already idempotent (guarded by timeout_ == null), so centralizing the call cannot cause double-scheduling.
…re object setData sorted holdout.excludedExperimentIds in place so isExcluded's binary search works, but the Experiment instance can be a caller-supplied, shared ContextData (ABSmartly.createContextWith). Sorting in place mutated the caller's own array, observable via getData()/equals and racy if the same ContextData instance were installed on two contexts concurrently. Add normalizeHoldout, which returns a shallow copy of the holdout carrying a private, sorted copy of excludedExperimentIds (the original array is reused unchanged when there is nothing to sort), and route setData's holdout indexing through it instead of sorting in place.
…holdoutIds Experiment.holdoutIds now carries the ids of the holdouts that cover it, server-resolved. Context.resolveApplicableHoldouts looks each id up against the installed holdouts[] rather than deriving applicability from unit-type matching plus a full_on/exclusion rule; an id absent from holdouts[] simply contributes no coverage. isExcluded and normalizeHoldout are removed as dead code now that there is no exclusion list to binary-search or defensively sort.
Pedro-Revez-Silva
left a comment
Pedro-Revez-Silva
left a comment
There was a problem hiding this comment.
Reviewed exact head 07c0ae716506c8a72f6b0ba503152e148807d768 against merge-base/base f538b502fb79c5df778569e124ace447eacdd6a1. The final implementation has evolved beyond the stale PR summary: holdouts are first-class experiment entries, covered experiments suppress their own exposure, and each applicable holdout emits an ordinary exposure.
Validation: ./gradlew clean build completed successfully under a Java 8 container (277 tests; compilation, FindBugs main/test, Spotless, JaCoCo verification, packaging), matching the successful exact-head GitHub Java-SDK / build check. I also exercised two focused temporary regressions: one deterministically refreshed from a non-holding epoch to a holding epoch between the covered experiment's assignment and exposure, and observed the contradictory ordinary-experiment + held-out holdout exposure pair; the other showed a single throwing holdout exposure logger leaves one queued event with no scheduled flush, including after retry. The temporary tests were removed and the detached worktree was restored clean.
Two verified P2 blockers remain inline, so this head is not ready to merge.
…sion getAssignment now resolves every applicable holdout up front and pins the resulting HoldoutAssignment objects on the Assignment (holdoutAssignments), instead of stopping at the first suppressing holdout. triggerExposure fires that pinned snapshot rather than re-resolving the holdout definitions at exposure time, so a refresh landing between the suppression decision and the exposure trigger can no longer publish an exposure pair from two different holdout iterations.
enqueueExposure appends the exposure and increments pendingCount before invoking the ContextEventLogger callback, so a throwing logger left setTimeout() unreached and the queued exposure with no path to a flush. setTimeout() now runs in a finally around the callback; the callback's exception still propagates to the caller.
Pedro-Revez-Silva
left a comment
Pedro-Revez-Silva
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 0f64b815c543cd402918a7bf20f5de6efe3ce26a against base/merge-base f538b502fb79c5df778569e124ace447eacdd6a1, including the 07c0ae7..0f64b81 follow-up, both previous threads and author replies, full current PR behavior, and exact-head CI.
Both previously reported failures are fixed for their reproduced scenarios: normal assignments now trigger the pinned holdout assignments used for the suppression decision, and every queued exposure schedules its flush in a finally when the event logger throws. The two new regression tests pass, and the exact head completed ./gradlew clean build under Java 8 with 279 tests plus compilation, FindBugs main/test, Spotless, JaCoCo verification, and packaging; GitHub's exact-head Java-SDK / build check is also successful.
The follow-up snapshot introduces one remaining supported lifecycle defect inline: a peek before a late unit is configured pins a null holdout result in the ordinary assignment cache, so the later treatment never emits the now-evaluable holdout exposure. I reproduced this with the public peekTreatment -> setUnit -> getTreatment flow. Therefore this exact head is not yet ready to merge.
getAssignment's holdoutAssignments snapshot (55b87fe) pins whatever getHoldoutAssignment returns for each applicable holdout, including null when the holdout's unit type is not yet installed (Context.java ~1208: units_.get(unitType) returns null before the uid is set). A full-on covered experiment does not need the unit to produce a variant - the uid is only read in the fullOnVariant == 0 branch - so the experiment resolves normally while the holdout silently could not be asked, and the null gets pinned with suppressed=false. That is the invalidation axis here: the input became complete, not that the data changed, so the existing matches-based cache checks (experimentMatches/holdoutSetMatches, comparing (id, iteration)) correctly report no change and never catch it. triggerExposure fires the pinned snapshot directly and triggerHoldoutExposure(Assignment) skips a null entry outright, so once setUnit() installs the unit, the holdout exposure is lost for the life of the context - setUnit only wrote units_ and never touched the cache. setUnit now evicts, targeted by the covered experiment's unit type, any cached Assignment whose holdoutAssignments snapshot holds a null entry for that unit. Eviction forces getAssignment to recompute suppression and every owed exposure from one coherent decision made with the complete unit set, rather than re-resolving the null in place and firing a freshly-resolved holdout exposure next to a decision that was made without it - which would publish "held out" alongside "participated" for the same unit. Two defects are fixed relative to the rejected 6ef3545: 1. Wrong predicate field. The null entry is produced by getHoldoutAssignment(holdout, assignment.unitType) - the covered experiment's unit type - never by the referenced holdout's own declared unitType, which nothing requires to match. The eviction predicate now compares against assignment.unitType (the value resolution actually used), invoking equals on the non-null setUnit argument since Experiment.unitType is nullable. 2. Eviction resetting exposure state. Removing a cached Assignment discards its exposed AtomicBoolean. A full-on covered experiment is assignable without the unit, so its exposure can already be queued before the unit arrives; evicting that Assignment let the next getTreatment() build a fresh one with exposed==false, causing a duplicate (or contradictory, if the resolved holdout now suppresses) exposure for the same unit. Eviction now skips any assignment whose exposed flag is already set - once participation has been published for a unit, retroactively holding it out is wrong, and leaving one degraded record alone is better than compounding it with a contradictory second one. publish() builds event.units from the live units_ map at publish time regardless, so an exposure queued before setUnit may already carry the late unit either way. Adds selectivity tests pinning the shape of the fix rather than just the original bug: already-exposed assignments survive eviction (own and unrelated unit type), unaffected cache entries are left untouched, eviction is keyed off the covered experiment's unit type rather than the holdout's declared one, a late-resolved suppressing holdout produces a coherent recompute (kills the "re-resolve null at trigger time" alternative), multiple holdouts with different declared unit types on one experiment evict together, and setUnit with no cached assignment is a no-op that doesn't throw. Known residual, filed separately and not addressed here: getTreatment calls getAssignment(...) then triggerExposure(...), so a thread can hold a stale Assignment object across another thread's eviction and expose it after a replacement was exposed; exposed flags are per-object so locking the map does not prevent it.
Mutant "evict whenever holdoutAssignments != null" (dropping the check that some entry is null) survived 287/287 against c6a0a5a. It is only observable when a fully-resolved, unexposed cache entry is wrongly discarded by a redundant setUnit call for the same unit type: setUnitRedundantCallDoesNotEvictFullyResolvedUnexposedHoldoutSnapshot pins the entry's audience-mismatch verdict across a redundant setUnit and an attribute change that experimentMatches ignores; a wrongly-evicted entry would recompute under the now-matching attribute and flip the variant. triggerApplicableHoldoutExposuresNeverLiveResolvesAPinnedNullEntryForAn- AlreadyExposedAssignment covers a mutation the earlier fix attempt already guards against in the common case (setUnitRecomputesSuppressionCoherentlyWhenLateResolvedHoldoutSuppresses), but not in the one setUnit deliberately leaves unevicted: an already-exposed assignment's pinned null survives setUnit forever (Defect 2 guard), and can still reach triggerApplicableHoldoutExposures via the variable-key path, which re-triggers every candidate's holdouts unconditionally on every call regardless of that candidate's own exposed state. Also adds: peekVariableValue/getVariableValue coverage for the late-unit path with a suppressing late arm distinguishing recompute from live patch-up; override and custom-assignment interaction coverage (overrides never take a snapshot and rely on live resolution at exposure time; custom assignments take the ordinary snapshot path and are evicted the same way); and a redundant setUnit(same unit type, same uid) no-op check.
The condensed comment lost the least-obvious half of why unexposed-only eviction is right: publish() reads event.units from the live units_ map, so an exposure queued before setUnit may already carry the late unit. The guard avoids adding a second, contradictory record - it is not protecting a pristine one. Without that distinction a later reader can read the guard as an oversight and remove it.
Pedro-Revez-Silva
left a comment
Pedro-Revez-Silva
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 971203264ea539ad3e07209d7e21ac3c3422c189 against base/merge-base f538b502fb79c5df778569e124ace447eacdd6a1, including the 0f64b81..9712032 follow-up, the current discussion and resolved threads, my previous review, and exact-head CI.
The sequential peekTreatment -> setUnit -> getTreatment failure from the previous review is fixed: affected unexposed null snapshots are selectively evicted, the treatment and holdout are recomputed coherently, and the added treatment/variable/override/custom/selectivity tests cover the intended behavior. Exact-head ./gradlew clean build passed under Java 8 with 293 tests, compilation, FindBugs main/test, Spotless, JaCoCo verification, and packaging; GitHub's exact-head Java-SDK / build check is also successful.
One concurrency blocker remains in the new eviction path inline. The author also identified the underlying interval in the thread, and I independently reproduced its outcome with a focused harness: an assignment obtained before setUnit eviction remains exposable afterward, independently of the replacement's exposure state. Therefore this exact head is not yet ready to merge.
java-sdk#11 landed on main while this branch was in review. Merged rather than rebased: 30 of the 42 commits here touch Context.java, so a rebase replays the same semantic conflict ~30 times over the method that decides holdout suppression, and it would rewrite the commits the open review on #12 is anchored to. Both sides of every conflict were kept: - experimentMatches keeps this branch's ContextExperiment signature and its holdoutSetMatches call, and adopts main's null-safe unitType comparison. - The cache-validity site keeps variantForcedRegardlessOfCustom from here and main's audienceMatches call - main tightened cache validity while this branch was forked, and dropping it would have silently reverted that. - Assignment keeps suppressed/holdouts/holdoutAssignments alongside main's attrsSeq, with variables staying Collections.emptyMap(). - setData keeps resolveApplicableHoldouts and main's null-safe variants length. - Class constants keep the exposure state machine and main's Logger. - DefaultContextDataDeserializerTest keeps deserializeHoldouts and all six of main's new deserializer tests.
Main's audience-aware cache validity now correctly invalidates the attribute-change signal. Use a same-iteration split refresh so the redundant setUnit test still distinguishes retained and evicted assignments.
… value variantForcedRegardlessOfCustom() (introduced in 5ec87d4) bypassed cache invalidation entirely for full-on, ineligible, and unassigned entries once ANY custom assignment was on file, including one set after the cached entry was resolved. A legitimate custom-assignment change on a full-on entry was then silently swallowed: getAssignment kept returning the stale cached Assignment forever, so getTreatment never re-exposed (conformance scenario 66 - Custom Assignment - Clear Cache). Replace the broad bypass with a pinned comparison: Assignment now records the cassignments_ entry (customAssignment) that was live when it was resolved, and the fast path compares the live entry against that pinned value instead of against the resolved variant. This keeps forced entries (where the custom value can never equal the resolved variant) cache-valid across repeated calls with the same custom assignment, while still invalidating exactly when the custom assignment itself changes. Add setCustomAssignmentAfterExposureReExposesFullOnAssignment, which fails under the reverted broad condition (pendingCount stays at 1 instead of reaching 2) and passes with the narrowed one.
Pedro-Revez-Silva
left a comment
Pedro-Revez-Silva
left a comment
There was a problem hiding this comment.
Re-reviewed exact head ac76466ec4483e3f43d39ea05a41ea1a0d69450f against supplied base/merge-base eca477379a7db5f9b7f0c02de645def65887c8a6, including the merge of current main, the 9712032..ac76466 follow-up, all current discussion and resolved threads, my previous reviews, and exact-head CI.
The previously reported late-unit eviction race is fixed for the reproduced public lifecycle: retirement and exposure now contend on the same atomic state, a detached assignment cannot enqueue, and treatment/variable callers re-resolve a retired assignment. The focused replacement-variant and detached-assignment regressions pass. Exact-head ./gradlew clean build also passed under Java 11 with 415 tests, Java 6 Animal Sniffer compatibility, compilation, Spotless, JaCoCo verification, and packaging; GitHub's exact-head Java-SDK / build check is successful.
One independent holdout-cache identity defect remains inline. A single holdout may be referenced by experiments evaluated with different effective unit types—the implementation and existing tests explicitly permit the holdout's declared unit type to differ—but the cache retains only one effective unit type per holdout id. Alternating those supported references replaces the once-per-context exposure state and re-emits an already-recorded holdout. I reproduced this with a focused exact-head test. Therefore this exact head is not yet ready to merge.
holdoutAssignmentCache_ was keyed by holdout id alone, but getHoldoutAssignment resolves each holdout against the covered experiment's effective unit type, which need not equal the holdout's own declared unitType. When one holdout id covers experiments with two different unit types, alternating lookups (A1 -> B -> A2) evicted each other's cache slot on every miss: B's lookup replaced A's entry because matches() failed only on unitType, discarding A's exposureState. The next A lookup then missed, built a brand-new Assignment with a fresh exposureState, and republished that holdout's own exposure a second time for the same unit. Introduce HoldoutCacheKey, a composite (id, unitType) key, so each (holdout, effective unit) pair gets its own cache slot and its own once-per-context exposure state. unitType is dropped from HoldoutAssignment itself since the key now carries it exclusively; matches() is reduced to the iteration check it still owns. The double-checked locking (read under readLock, recheck under writeLock) and the id-resolution-before-matches() behavior in getHoldoutAssignment are unchanged. Regression test reproduces the exact A1 -> B -> A2 alternation Pedro found: one holdout id/iteration covering two unit types, 3 covered experiments. Confirms 5 pending events (not 6) and the holdout's own exposure published once per unit type (2 total), not once per alternation (3).
Context.java has no explicit Objects import, so the wildcard java.util.* bound Objects.equals to the JDK class, which is Java 7+. Animal Sniffer enforces a Java 1.6 floor on core-api production source (gradle/compatibility.gradle), so the build failed with an undefined reference. Import com.absmartly.sdk.java.util.Objects instead, the same shim every json model class already uses.
abs#4939 removed holdoutType from the collector context response, but the holdouts fixture still carried it on both holdouts and deserializeHoldouts asserted both values - pinning a payload the server no longer produces. Dropped from holdout B only, deliberately. Holdout A keeps it so the test still pins that a legacy payload from an older collector deserializes cleanly during rollout; holdout B pins the new shape, where arity comes from split.length.
The collector is moving holdouts to a dedicated 7-field DTO (id, name, unitType, iteration, seedHi, seedLo, split), so the fixture no longer matched what the server sends: both holdouts still carried trafficSeed*/trafficSplit/fullOnVariant/ variants/audience. Slimmed holdout 12 to the new shape and left holdout 11 fat, so the deserializer test pins both directions - the new payload deserializes cleanly (absent primitive ints default to 0, arrays to null), and a legacy payload from an older collector is still tolerated during rollout.
Pedro-Revez-Silva
left a comment
Pedro-Revez-Silva
left a comment
There was a problem hiding this comment.
Approving.
The review was requested at 013a800e6ba2773277cf0cea4dc7520fca0e70a2, but the branch advanced to 944f700fc11bdd897030da221b089155d8b6b26f (one test-only commit, test: pin both the slim and legacy holdout wire shapes) while I was validating. I re-pinned and re-ran the full validation at the current head, so this approval is against 944f700fc11bdd897030da221b089155d8b6b26f, reviewed against base/merge-base eca477379a7db5f9b7f0c02de645def65887c8a6.
Every blocker I filed across this review series is fixed, and I re-verified each one with my own reproduction rather than relying on the added regression tests. All three pass at this head:
- Late unit installed after a peek pinned a
nullholdout snapshot, permanently dropping the holdout exposure — now recomputed coherently by the targetedsetUniteviction. - An assignment obtained before that eviction stayed independently exposable beside its coherent replacement — now closed by making retirement and exposure claim contend on one atomic
exposureState, with a detached assignment unable to enqueue and the caller re-resolving. - A holdout shared by experiments with different effective unit types re-published its membership on every
A -> B -> Aalternation — now closed by keyingholdoutAssignmentCache_on(id, effective unit type).
I also verified the two follow-ups the author self-reported. b05cb26 replaces the over-broad variantForcedRegardlessOfCustom() bypass with a pinned Assignment.customAssignment comparison; the pinned value is written under the same contextLock_ write lock that setCustomAssignment writes cassignments_ under, and the forced-variant paths (full-on, traffic-ineligible, strict audience mismatch, holdout-suppressed, override, variable-key) still hit the cache on repeat calls, so the older duplicate-exposure/cache-thrash bug is not reintroduced. 6a64267 correctly rebinds Objects to the bundled com.absmartly.sdk.java.util.Objects shim — a single-type import outranks the java.util.* wildcard, and the shim's equals is semantically identical for the nullable Integer comparison. I swept all of core-api/src/main for remaining Java 7+/8+ APIs against the enforced 1.6 floor and found none.
On the final two test-only commits: Context.java never reads Experiment.holdoutType, and holdout arity comes solely from split.length via Assignment.armCount, so trimming the fixture to the slim wire shape does not weaken a reachable contract. holdoutA still carries the legacy fields, so both shapes stay pinned, and Experiment.equals still compares holdoutType, making the slim entry a positive assertion that the deserializer does not fabricate values for absent fields.
Validation at the exact head: ./gradlew clean build passed under Java 11 — 417 tests, Animal Sniffer against the Java 1.6 signature, compilation, Spotless, JaCoCo verification, and packaging. GitHub's Java-SDK / build check for 944f700 is also successful. I confirmed the earlier Animal Sniffer failure I flagged at 1b2501c is genuinely resolved rather than masked, by running :core-api:animalsnifferMain explicitly.
No blocker survives and no material limitation remains. Nice work driving this one down — the mutation-proofing on each fix made the follow-ups easy to trust.
Pedro-Revez-Silva
left a comment
Pedro-Revez-Silva
left a comment
There was a problem hiding this comment.
Re-reviewed at exact head 944f700fc11bdd897030da221b089155d8b6b26f against base/merge-base eca477379a7db5f9b7f0c02de645def65887c8a6. Approving again to close out this feedback cycle.
This head is byte-identical to the one I approved in #pullrequestreview-5092275832. I confirmed rather than assumed that: the branch head and merge-base are unchanged, no commits landed since, all five inline threads remain resolved with no reply after 2026-09-02T11:44:00Z, and no new top-level discussion exists. So there is no new PR-caused risk surface to analyse in this round.
I still re-ran the evidence rather than relying on the earlier run, since approval is the consequential state. In a fresh clean detached worktree at this exact commit, ./gradlew clean build passed under Java 11: 417 tests, Animal Sniffer against the Java 1.6 signature, compilation, Spotless, JaCoCo verification, and packaging. GitHub's Java-SDK / build check for this SHA is also green.
For the record, the substantive findings from earlier rounds all remain closed at this head, and I verified the first three previously with my own reproductions rather than relying on the added regression tests:
- A late unit installed after a peek pinned a
nullholdout snapshot and permanently dropped the holdout exposure — now recomputed coherently via targetedsetUniteviction. - An assignment obtained before that eviction stayed independently exposable beside its coherent replacement — now closed by making retirement and the exposure claim contend on one atomic
exposureState, with detached assignments unable to enqueue and callers re-resolving. - A holdout shared across experiments with different effective unit types re-published its membership on each
A -> B -> Aalternation — now closed by keyingholdoutAssignmentCache_on(id, effective unit type). - The
java.util.Objectsreference that silently bound through thejava.util.*wildcard and broke the enforced Java 1.6 floor — now rebound to the bundledcom.absmartly.sdk.java.util.Objectsshim, with a full sweep ofcore-api/src/mainfinding no other Java 7+ API usage.
No blocker survives and no material limitation remains. Ready to merge.
2 participants
Summary
Client-side holdout support (ISS-61, Phase 3) — companion to absmartly/abs#4539 (server side).
ContextData.holdouts[](definitions:{id, seedHi, seedLo, split}) +Experiment.holdoutIds[]references — definitions are not duplicated per experiment.assigned=true; overrides bypass holdouts (debugging escape hatch).VariantAssignerwith the holdout's own seed — same verdict across all experiments referencing the holdout.holdoutIdsor a referenced definition (seed/split) invalidates cached assignments.heldOut+holdoutIdfor downstream analysis.holdoutIdsand malformed definitions (null/short split) are skipped gracefully.Verification
:core-api:test), findbugsMain cleanNotes
Summary by CodeRabbit
New Features
Bug Fixes
Tests