Share pooling; add context to mark

[bcb37]

No description provided.

[Copilot]

Pull request overview

This PR refactors the backend “mark” path to make experiment resolution consistent with the “assign” path by introducing an explicit context parameter, switching mark-time experiment selection to use context-scoped cached experiments, and adding integration coverage for state filtering and cross-context isolation.

Changes:

• Add context to mark request validation and thread it through controllers → ExperimentAssignmentService.markExperimentPoint.
• Replace mark-specific decision-point caching with getCachedValidExperiments(context) and consolidate selection logic in resolveExperimentForMarkPoint.
• Expand integration tests to validate cancelled-experiment exclusion and prevent cross-context experiment pool contamination; update JS/Java clients to send context in mark requests.

Reviewed changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
packages/backend/test/integration/utils/index.ts	Updates test helper to pass context into markExperimentPoint.
packages/backend/test/integration/UserNotDefined/index.ts	Adjusts mark call signature in the “user not defined” integration case.
packages/backend/test/integration/index.test.ts	Registers new integration test cases for mark behavior.
packages/backend/test/integration/Experiment/withinSubject/MetricQueriesCheck.ts	Threads context through within-subject metric query mark calls.
packages/backend/test/integration/Experiment/stratification/MetricQueriesCheck.ts	Threads context through stratification metric query mark calls.
packages/backend/test/integration/Experiment/markExperimentPoint/NoExperiment.ts	Passes explicit context when marking in the NoExperiment case.
packages/backend/test/integration/Experiment/markExperimentPoint/index.ts	Exposes new markExperimentPoint integration test cases.
packages/backend/test/integration/Experiment/markExperimentPoint/ContextContamination.ts	Adds integration test ensuring no cross-context contamination for shared site/target.
packages/backend/test/integration/Experiment/markExperimentPoint/CancelledExperimentStateFilter.ts	Adds integration test ensuring cancelled experiments are excluded from mark resolution.
packages/backend/test/integration/Experiment/experimentContext/ExperimentContextAssignments.ts	Updates experiment-context integration flow to pass mark context.
packages/backend/src/api/services/ExperimentService.ts	Simplifies cache invalidation to remove mark-cache key deletion.
packages/backend/src/api/services/ExperimentAssignmentService.ts	Adds context-aware experiment resolution for mark and consolidates selection logic.
packages/backend/src/api/controllers/validators/MarkExperimentValidator.v6.ts	Makes context required for v6 mark requests.
packages/backend/src/api/controllers/validators/MarkExperimentValidator.v5.ts	Adds optional context for v5 mark requests (backward compatibility).
packages/backend/src/api/controllers/ExperimentClientController.v6.ts	Passes validated context through to mark service call.
packages/backend/src/api/controllers/ExperimentClientController.v5.ts	Passes optional context (currently defaulting to empty string) to mark service call.
docs/mark-context-refactor-plan.md	Documents the rationale and phased plan for the mark-path refactor.
clientlibs/js/src/UpGradeClient/generateUUID.spec.ts	Minor test cleanup (trailing whitespace removal).
clientlibs/js/src/types/requests.ts	Adds context to the JS mark request body type.
clientlibs/js/src/ApiService/ApiService.ts	Includes configured context in JS mark request bodies.
clientlibs/js/src/ApiService/ApiService.spec.ts	Adds JS client tests verifying context is included in mark requests.
clientlibs/java/src/main/java/org/upgradeplatform/requestbeans/MarkExperimentRequest.java	Adds context to the Java mark request bean and constructors.
clientlibs/java/src/main/java/org/upgradeplatform/client/ExperimentClient.java	Threads client context into Java mark request construction.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[danoswaltCL]

minor thing but i don't think this error message matches what's happening. maybe this was the preexisting error but i think we mean experimentId not found among valid experiments.

[danoswaltCL]

so we will read and write to the same experiments cache in the assign and mark paths in this right? that would be great if i'm reading this correctly, I think that would halve the number of db fetches to select experiments. if my math is right, which it usually isn't, but regardless, a good thing.

[bcb37]

so we will read and write to the same experiments cache in the assign and mark paths in this right? that would be great if i'm reading this correctly, I think that would halve the number of db fetches to select experiments. if my math is right, which it usually isn't, but regardless, a good thing.

Yeah, I think that simplifies the cacheing quite a bit, given that we can get rid of one of the cache keys.

[Copilot]

Pull request overview

Copilot reviewed 24 out of 24 changed files in this pull request and generated 4 comments.

Comments suppressed due to low confidence (1)

packages/backend/test/unit/services/ExperimentAssignmentService.test.ts:1127

• This test calls markExperimentPoint with the old argument order: the third argument should be a MARKED_DECISION_POINT_STATUS, but target (undefined) is being passed instead. The current test passes only because the method doesn’t validate status, and it risks masking real regressions in the mark path.

    const result = await testedModule.markExperimentPoint(
      { id: userId },
      site,
      target,
      condition,
      loggerMock,
      'context',
      undefined,
      undefined,
      undefined,
      clientError

[danoswaltCL]

if "context" is going mandatory in /mark, how are we gonna roll this out? all current consumers would 400 unless they make changes ahead of time, which would be hard, do we need to keep some form of the non-context logic?

[bcb37]

if "context" is going mandatory in /mark, how are we gonna roll this out? all current consumers would 400 unless they make changes ahead of time, which would be hard, do we need to keep some form of the non-context logic?

Well, that might defeat the purpose. If it would be too hard to get all the client libraries to update, we could publish the changes to those and then wait until we're sure everybody's on the new ones.

[danoswaltCL]

hm. we have had to do that in the past, it was a tricky, slow dance moving through the envs even when it was just assign-prog and we have a lot more consumers now. However, most are not doing experiments, they're doing flags and not marking, and it's a different team situation now, we're more capable now than we were then... not an appetizing proposition but can be done

• if we do decide that's what we'll need to do, then this oughtn't go into 6.6 because it'll block releasing all of the rest of the good stuff going into 6.6 until everyone is moved over, we'll be at the mercy of everyone else's releases and priorities and that could be months.

• if we can allow for a temporary band-aid where context is not yet required until 6.7 so we can gracefully deprecate it, the good thing is that mark is not blocking, so if we have to introduce some logic for deducing the context, even if imperfect, i don't think that's the worst thing, and would still be better than what we have been doing.

[bcb37]

hm. we have had to do that in the past, it was a tricky, slow dance moving through the envs even when it was just assign-prog and we have a lot more consumers now. However, most are not doing experiments, they're doing flags and not marking, and it's a different team situation now, we're more capable now than we were then... not an appetizing proposition but can be done

• if we do decide that's what we'll need to do, then this oughtn't go into 6.6 because it'll block releasing all of the rest of the good stuff going into 6.6 until everyone is moved over, we'll be at the mercy of everyone else's releases and priorities and that could be months.
• if we can allow for a temporary band-aid where context is not yet required until 6.7 so we can gracefully deprecate it, the good thing is that mark is not blocking, so if we have to introduce some logic for deducing the context, even if imperfect, i don't think that's the worst thing, and would still be better than what we have been doing.

The context-inferring code is in, but I guess we're still deferring this to 6.7?

[danoswaltCL]

i wish i hadn't missed this update, it would be helpful to see this in there for sure but 6.7 definitely, i will circle back to this once we're done messing around with 6.6 stuff.

[danoswaltCL]

i don't claim to have found this but Claude noticed a thing here: selectExperimentsForUser has this code:

// gets two same-length arrays, filteredExperiments, exclusionReason

const [filteredExperiments, exclusionReason] = await this.experimentLevelExclusionInclusion(
      validExperiments,
      userDoc,
      logger
    );

// we narrow filteredExperiments down to 1 experiment
    const selectedExperiments = this.processExperimentPools(
      filteredExperiments,
      mergedIndividualEnrollments,
      groupEnrollments,
      individualExclusions,
      groupExclusions,
      userDoc,
      previewUser
    );

   return {
      selectedExperiments, // 1 experiment
      exclusionReason, // still an array of multiple experiments
      mergedIndividualEnrollments,
      groupEnrollments,
      individualExclusions,
      groupExclusions,
    };

So when we get to this:

// "selectedExperiments" is not plural, it's definitely one experiment
// "exclusionReason" is not singular, it can be an array of experiment exclusion reasons
const { selectedExperiments, exclusionReason } = await this.selectExperimentsForUser(

This makes for a few potential downstream issues where exclusionReason is expected to refer to one or zero experiment exclusion reasons, such as this spot in updateEnrollmentExclusion, where this array is eventually passed down and local variable is experimentLevelExcluded. Even though we may have enrolled "selectedExperiment", we get here where if there was a decision pool with multiple experiments and there was an exclusion in one of them, this array is >0, experimentExcluded is accidentally true.

let experimentExcluded = false;
    if (experimentLevelExcluded.length > 0) {
      experimentExcluded = true;
    }

It hurts my head to try and think through how big a deal that is, but it does seem unintentional and looks like a straightforward fix just below this comment:

const experiments = selectedExperiments.length > 0 ? [selectedExperiments[0]] : [];
    const resolvedExperimentId = experiments[0]?.id ?? null;
    const resolvedExclusionReason = exclusionReason.filter((reason) => reason.experiment?.id === resolvedExperimentId);

    if (resolvedExperimentId) {
      // Scope the decision-point-wide exclusion list down to the experiment actually being marked.
      // selectExperimentsForUser evaluates every experiment at the decision point, so exclusionReason
      // can hold entries for sibling experiments. updateEnrollmentExclusion only checks whether the
      // list is non-empty, so letting a sibling's reason through would exclude the user from the very
      // experiment getAllExperimentConditions just assigned them a condition for.
      return {
        experiments,
        experimentId: resolvedExperimentId,
        isUserExcluded,
        isGroupExcluded,
        exclusionReason: resolvedExclusionReason,
      };

[bcb37]

// gets two same-length arrays, filteredExperiments, exclusionReason

This is false. They will not be guaranteed to be same length arrays. One is the list of experiments that are not excluded, and the other is a list of reasons for exclusion for the experiments that ARE excluded.

// we narrow filteredExperiments down to 1 experiment

This is also false. It selects one experiment for each set of experiments that have competing decision points.

I think the confusion comes from the fact that resolveExperimentForMarkPoint() should not have been returning an array of experiments, but rather a single experiment, since mark doesn't operate on more than one at a time. It's not possible for an exclusion reason for a different experiment to be found, since we're only ever considering one experiment in this case. I'll make the change to the method so it's explicitly operating on a single experiment.

[danoswaltCL]

yes that would help, but inaccurate "same-length" comment aside, I think you're completely dismissing what I spent hours trying to test and share.

here's what I see: create two experiments, A and B, same decision point, include all for both except exclude test user in B. assign will give A, mark the A condition as null, with no experimentId or status.

The user will be excluded from A, which I think is right, but with "exclusion reason" recorded as "participant was on exclusion list". That's the wrong reason, it is recording B's reason, that's the symptom of the bug I am trying to describe.

[danoswaltCL]

it's actually worse if a real condition is marked in this same scenario without experimentId being sent. If in same scenario as above I'm sending in a condition that should get enrolled for experiment A instead of null. instead of getting enrolled, it get excluded wrongly and marked as "participant was on exclusion list". because it's taking experiment B's information.

in practice i think our consumers are always sending the experimentId so it's not as urgent but it's a valid request and it's the same basic issue, the excluded-reasons array needs to be empty by the time it gets to the exclusion code or it will always mark it as true here and excluded.

[bcb37]

with no experimentId or status.

As far as I've been able to determine (and I tested this multiple times on dev and release/6.6), if you mark with no 'status' the user will be enrolled if the pooling algorithm selects an experiment and excluded if it selects one from which the user is excluded. From Claude's reading of the code:

CONDITION_APPLIED, NO_CONDITION_ASSIGNED, and undefined (no status sent) These three cases are indistinguishable to the rest of the function — status is not checked anywhere else in updateEnrollmentExclusion. For all of them, the outcome (enroll vs. exclude) is determined entirely by other inputs.

The user will be excluded from A, which I think is right.

I think that would be true if 'status' was 'EXCLUDED_BY_CLIENT' but not if it's undefined.
I added the resolvedExclusionReason line from the example above and it now produces what I'm fairly sure is the expected result in every case.

mark with no status or experimentId -> enrolled in experiment A (the one the user is not excluded from)
mark with 'condition not applied' -> excluded from experiment A with the correct exclusion reason recorded

An additional wrinkle I discovered was that if you don't send in an experimentId it filters out excluded experiments completely from the list - calling the same method that's called in the assign path, so the lists will match for pooling purposes. However, we need the excluded experiments for mark in order to populate the exclusion record. If there are only excluded experiments, then we can chose the first one in the list for the exclusion record, since we don't have worry about matching assign in cases where assign will return an empty list. I've added that.

I hope this makes sense and handles everything we're trying to support. Thanks for the careful reading and testing.

[danoswaltCL]

typo

[danoswaltCL]

is it possible to have zero here? i think the downstream code seems to assume exactly 1, and if that's the case, it would help following the logic of this all a lot if this returned one Experiment instead of Experiment[], because it keeps getting referred to as experiments and/or experiments[0] and length is checked for truthiness... it is hard to follow and the bug noted above may have been simpler for a human to have found instead of my robot friend if the variable names were clearer.

[bcb37]

See above.

[danoswaltCL]

@bcb37 just a few things that i think are an artifact of moving things around during refactors, on the new main-line path with "context" regardless of the old code.

[danoswaltCL]

I think we need to filter out decision points that are in pendingActivation state. This matches what happens during creating experiment pool so i think we can reuse this existing private function

const allExperimentsAtDP = allExperimentsForContext.filter((exp) =>
  this.getActiveDecisionPoints(exp).some((p) => p.site === site && p.target === target)
);

[danoswaltCL]

I just added one little quick-fix comment. I think this should be ready to go otherwise, there's a list of things that Claude has suggested that are a little edge-casey that I can put into a ticket or something and we can see what's worth a follow-up. but it's all much better than what we've been having mark do since the beginning of time so let's get it into dev, this is great

[danoswaltCL]

There seems to be a nonzero possibility of exclusionReason being undefined, so we might as well do exclusionReason[0]?.experiment to guard:

const experiment = selectedExperiments.length > 0 ? selectedExperiments[0] : exclusionReason[0]?.experiment || null;

[danoswaltCL]

just this last one! it seems unlikely, but if remotely possible it would crash on Can't access selectedExperiments on undefined type of thing.