Share pooling; add context to mark by bcb37 · Pull Request #3180 · CarnegieLearningWeb/UpGrade

bcb37 · 2026-06-15T13:30:30Z

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.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

danoswaltCL · 2026-07-08T16:51:49Z

+      const dpExpExists = allExperimentsAtDP.filter((exp) => exp.id === experimentId);
+      if (!dpExpExists.length) {
+        const error = new Error(
+          `Experiment ID not provided for shared Decision Point in markExperimentPoint: ${userDoc.id}`


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 · 2026-07-09T21:09:55Z

+   * so the selected experiment is guaranteed to match what getAllExperimentConditions would have returned for
+   * the same user and decision point.
+   */
+  private async resolveExperimentForMarkPoint(


most of this sequence for the parts that the mark and assign calls need to do exactly the same is still implemented twice. can we get the important parts for selecting / filtering valid experiments and running through the pooling logic in one method so it really is guaranteed to match?

I suppose we could DRY this out a bit more for the cases where there's no experimentId

danoswaltCL · 2026-07-09T21:24:02Z

+      };
+    }
+
+    const [isUserExcluded, isGroupExcluded] = await this.checkUserOrGroupIsGloballyExcluded(userDoc, context);


if a user is globally excluded but is marking as if they got an assignment... what happens?

No enrollment records are created but a monitored decision point entry is.

danoswaltCL · 2026-07-10T18:52:15Z

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 · 2026-07-10T19:02:30Z

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

bcb37 added 8 commits June 5, 2026 11:02

phases 1-4 of refactor plan

ec93d7f

update js client library

b1e33b8

java client library

5d7ac7b

fix existing integration tests

4eb7b24

Phase 7a - state filter integration tests

cc4132f

Phase 7b - context contamination integration tests

d116f3a

remove non-useful test

6f8d1d2

Merge branch 'dev' into wip/share-pooling-add-context-to-mark

6b617fd

bcb37 added the release 6.6 label Jun 15, 2026

bcb37 requested a review from Copilot June 15, 2026 13:30

Copilot started reviewing on behalf of bcb37 June 15, 2026 13:31 View session

Copilot AI reviewed Jun 15, 2026

View reviewed changes

Comment thread packages/backend/src/api/services/ExperimentAssignmentService.ts

Comment thread packages/backend/src/api/services/ExperimentAssignmentService.ts

Comment thread packages/backend/src/api/controllers/ExperimentClientController.v5.ts

bcb37 and others added 2 commits June 18, 2026 14:16

null experimentId for non-mark monitored points

c901f73

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

add unit tests for resolveExperimentForMarkPoint

39fc6c1

bcb37 marked this pull request as ready for review June 18, 2026 19:10

bcb37 requested a review from danoswaltCL June 18, 2026 19:10

bcb37 linked an issue Jun 18, 2026 that may be closed by this pull request

Refactor mark path to use context-aware caching and consolidate experiment selection logic #3167

Open

bcb37 changed the title ~~Wip/share pooling add context to mark~~ Share pooling add context to mark Jun 30, 2026

bcb37 changed the title ~~Share pooling add context to mark~~ Share pooling; add context to mark Jun 30, 2026

Merge branch 'dev' into wip/share-pooling-add-context-to-mark

a24fa26

danoswaltCL reviewed Jul 8, 2026

View reviewed changes

danoswaltCL reviewed Jul 9, 2026

View reviewed changes

bcb37 requested a review from zackcl July 10, 2026 13:26

Merge branch 'dev' into wip/share-pooling-add-context-to-mark

14a8696

consolidate mark and assign logic further

5bbd084

bcb37 requested review from Copilot and danoswaltCL July 10, 2026 19:47

Copilot started reviewing on behalf of bcb37 July 10, 2026 19:47 View session

Copilot AI reviewed Jul 10, 2026

View reviewed changes

Uh oh!

Conversation

bcb37 commented Jun 15, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danoswaltCL Jul 8, 2026

Choose a reason for hiding this comment

Uh oh!

danoswaltCL Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

bcb37 Jul 10, 2026

Choose a reason for hiding this comment

Uh oh!

danoswaltCL Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

bcb37 Jul 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danoswaltCL commented Jul 10, 2026

Uh oh!

bcb37 commented Jul 10, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bcb37 Jul 10, 2026 •

edited

Loading