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.