Remove per-request JSON deep copy from getCachedValidExperiments

packages/backend/src/api/Algorithms.ts

@@ -117,7 +117,16 @@ export function buildWithinSubjectOrderedConditions(
   orderedConditions: IExperimentAssignmentv5['assignedCondition'];
   orderedFactors: Record<string, { level: string; payload: IPayload }>[] | null;
 } {
-  const baseConditions: IExperimentAssignmentv5['assignedCondition'] = experiment.conditions.map((condition) => ({
+  // Order matters here: ORDERED_ROUND_ROBIN rotates this array directly, and the RANDOM /
+  // RANDOM_ROUND_ROBIN shuffles are seeded, so their output depends on the input sequence too.
+  // `getValidExperiments` (the assignment read path) does not ORDER BY conditions.order — only
+  // `findOneExperiment` does — so sort explicitly rather than inheriting whatever order the DB
+  // returned. Sort a copy: `experiment.conditions` can be the array owned by the in-memory cache.
+  const orderedExperimentConditions = [...experiment.conditions].sort(
+    (condition1, condition2) => condition1.order - condition2.order
+  );
+
+  const baseConditions: IExperimentAssignmentv5['assignedCondition'] = orderedExperimentConditions.map((condition) => ({
     conditionCode: condition.conditionCode,
     payload: undefined,
     experimentId: experiment.id,
@@ -126,7 +135,7 @@ export function buildWithinSubjectOrderedConditions(
 
   const baseFactors: Record<string, { level: string; payload: IPayload }>[] | null =
     experiment.type === EXPERIMENT_TYPE.FACTORIAL
-      ? experiment.conditions.map((condition) => getAssignedFactor(condition, factors))
+      ? orderedExperimentConditions.map((condition) => getAssignedFactor(condition, factors))
       : null;
 
   let assignedData: IExperimentAssignmentv5 = {

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

@@ -708,7 +708,7 @@ export class ExperimentAssignmentService {
     const experiments = previewUser
       ? await this.experimentRepository.getValidExperimentsWithPreview(context)
       : await this.experimentService.getCachedValidExperiments(context);
-    // adding conditionPayloads at the root level instead of inside conditions
+    // adding conditionPayloads at the root level instead of inside conditions.
     return experiments.map((exp) => this.experimentService.formattingConditionPayload(exp));
   }
 
@@ -2067,7 +2067,8 @@ export class ExperimentAssignmentService {
         ? `${experiment.id}_${user.id}`
         : `${experiment.id}_${user.workingGroup?.[experiment.group]}`;
 
-    const sortedExperimentCondition = experiment.conditions.sort(
+    // Make a copy before sorting so we don't mutate the original array
+    const sortedExperimentCondition = [...experiment.conditions].sort(
       (condition1, condition2) => condition1.order - condition2.order
     );
     let spec = sortedExperimentCondition.map((condition) => condition.assignmentWeight);
@@ -2085,7 +2086,10 @@ export class ExperimentAssignmentService {
         break;
       }
     }
-    const experimentalCondition = experiment.conditions[randomConditions];
+    // Index the sorted copy: `randomConditions` is an index into `spec`, which is derived from it.
+    // (This used to read `experiment.conditions`, which was equivalent only because the sort above
+    // mutated that array in place.)
+    const experimentalCondition = sortedExperimentCondition[randomConditions];
     return experimentalCondition;
   }
 

packages/backend/src/api/services/ExperimentService.ts

@@ -302,13 +302,22 @@ export class ExperimentService {
     };
   }
 
+  /**
+   * Returns the valid experiments for a context.
+   *
+   * The cache is an in-memory store, so this hands back the same object reference on every hit. That
+   * used to require a defensive `JSON.parse(JSON.stringify(...))` deep copy on every assignment
+   * request, because `formattingConditionPayload` mutated what it was given. That function now builds
+   * new objects instead, so the cached graph is never written to and the per-request copy is gone.
+   *
+   * Callers must keep it that way: treat this result, and everything reachable from it, as read-only.
+   */
   public async getCachedValidExperiments(context: string): Promise<Experiment[]> {
     const cacheKey = CACHE_PREFIX.EXPERIMENT_KEY_PREFIX + context;
-    return this.cacheService
-      .wrap(cacheKey, this.experimentRepository.getValidExperiments.bind(this.experimentRepository, context))
-      .then((validExperiment) => {
-        return JSON.parse(JSON.stringify(validExperiment));
-      });
+    return this.cacheService.wrap(
+      cacheKey,
+      this.experimentRepository.getValidExperiments.bind(this.experimentRepository, context)
+    );
   }
 
   public async create(
@@ -1898,35 +1907,47 @@ export class ExperimentService {
     return searchStringConcatenated;
   }
 
+  /**
+   * Hoists conditionPayloads from conditions (factorial) or decision points (everything else) up to
+   * the root of the experiment.
+   *
+   * This does not mutate `experiment` or anything reachable from it. That matters because the
+   * assignment read path calls this on experiments handed out by the in-memory cache, which returns
+   * the same object reference to every request: mutating here would corrupt the cached graph for all
+   * subsequent requests, which is why this call site previously needed a full deep copy of the
+   * experiment on every request. Stripped conditions/partitions are rebuilt as new objects instead,
+   * and the `parentCondition`/`decisionPoint` back-references point at those same rebuilt objects, so
+   * reference identity within the returned experiment matches what the old in-place version produced.
+   */
   public formattingConditionPayload(experiment: Experiment): Experiment {
     if (experiment.type === EXPERIMENT_TYPE.FACTORIAL) {
       const conditionPayload: ConditionPayload[] = [];
-      experiment.conditions.forEach((condition) => {
-        const conditionPayloads = condition.conditionPayloads.map((conditionPayload) => {
-          return { ...conditionPayload, parentCondition: condition };
+      const conditions = experiment.conditions.map(({ conditionPayloads, ...rest }) => rest as ExperimentCondition);
+
+      experiment.conditions.forEach((condition, index) => {
+        (condition.conditionPayloads || []).forEach((payload) => {
+          conditionPayload.push({ ...payload, parentCondition: conditions[index] });
         });
-        conditionPayload.push(...conditionPayloads);
-        delete condition.conditionPayloads;
       });
 
-      return { ...experiment, conditionPayloads: conditionPayload };
+      return { ...experiment, conditions, conditionPayloads: conditionPayload };
     }
 
-    const { conditions, partitions } = experiment;
+    const partitions = experiment.partitions.map(({ conditionPayloads, ...rest }) => rest as DecisionPoint);
 
     const conditionPayload: ConditionPayload[] = [];
-    partitions.forEach((partition) => {
-      const conditionPayloadData = partition.conditionPayloads;
-      delete partition.conditionPayloads;
+    experiment.partitions.forEach((partition, index) => {
+      // Copy before sorting — the source array belongs to the (possibly cached) experiment.
+      const conditionPayloadData = [...(partition.conditionPayloads || [])];
 
       conditionPayloadData.sort((a, b) => a.parentCondition.order - b.parentCondition.order);
       conditionPayloadData.forEach((x) => {
-        if (x && conditions.filter((con) => con.id === x.parentCondition.id).length > 0) {
-          conditionPayload.push({ ...x, decisionPoint: partition });
+        if (x && experiment.conditions.some((con) => con.id === x.parentCondition.id)) {
+          conditionPayload.push({ ...x, decisionPoint: partitions[index] });
         }
       });
     });
-    return { ...experiment, conditionPayloads: conditionPayload };
+    return { ...experiment, partitions, conditionPayloads: conditionPayload };
   }
 
   public reducedConditionPayload(experiment: Experiment): any {

packages/backend/test/unit/services/Algorithms.test.ts

@@ -0,0 +1,175 @@
+import { buildWithinSubjectOrderedConditions } from '../../../src/api/Algorithms';
+import { Experiment } from '../../../src/api/models/Experiment';
+import { ExperimentCondition } from '../../../src/api/models/ExperimentCondition';
+import { FactorDTO } from '../../../src/api/DTO/FactorDTO';
+import { CONDITION_ORDER, EXPERIMENT_TYPE } from 'upgrade_types';
+
+/**
+ * These guard two invariants that the removal of the per-request deep copy in
+ * getCachedValidExperiments made load-bearing:
+ *
+ *  1. ORDER INDEPENDENCE — the assignment read path (`getValidExperiments`) does NOT
+ *     `ORDER BY conditions.order`; only the admin path (`findOneExperiment`) does. So whatever
+ *     sequence Postgres happens to return rows in is what this code receives. Output must depend on
+ *     each condition's `order` field, never on its position in the incoming array.
+ *
+ *     This is not hypothetical: ORDERED_ROUND_ROBIN previously produced correct output only because
+ *     `assignRandom` had sorted `experiment.conditions` IN PLACE earlier in the same request, and
+ *     this function silently consumed that side effect. Removing the in-place sort broke it. Worse,
+ *     `assignRandom` is skipped when a user is already enrolled, so the ordering a user got on their
+ *     first request differed from later ones.
+ *
+ *  2. NON-MUTATION — `experiment.conditions` can be the array owned by the in-memory experiment
+ *     cache, which hands the same reference to every request.
+ */
+describe('Algorithms: buildWithinSubjectOrderedConditions', () => {
+  const USER_ID = 'user-123';
+
+  const makeCondition = (conditionCode: string, order: number, levelId?: string): ExperimentCondition =>
+    ({
+      id: `condition-${conditionCode}`,
+      conditionCode,
+      order,
+      assignmentWeight: 50,
+      levelCombinationElements: levelId ? [{ level: { id: levelId } }] : [],
+    } as unknown as ExperimentCondition);
+
+  // Deliberately stored out of `order`, the way an unordered query can return them.
+  const scrambled = (): ExperimentCondition[] => [
+    makeCondition('C', 3, 'level-c'),
+    makeCondition('A', 1, 'level-a'),
+    makeCondition('B', 2, 'level-b'),
+  ];
+
+  const sorted = (): ExperimentCondition[] => [
+    makeCondition('A', 1, 'level-a'),
+    makeCondition('B', 2, 'level-b'),
+    makeCondition('C', 3, 'level-c'),
+  ];
+
+  const makeExperiment = (
+    conditions: ExperimentCondition[],
+    conditionOrder: CONDITION_ORDER,
+    type: EXPERIMENT_TYPE = EXPERIMENT_TYPE.SIMPLE
+  ): Experiment =>
+    ({
+      id: 'experiment-1',
+      type,
+      conditionOrder,
+      conditions,
+    } as unknown as Experiment);
+
+  const factors: FactorDTO[] = [
+    {
+      name: 'Color',
+      order: 1,
+      levels: [
+        { id: 'level-a', name: 'Red', payload: { type: 'string', value: 'red' } },
+        { id: 'level-b', name: 'Blue', payload: { type: 'string', value: 'blue' } },
+        { id: 'level-c', name: 'Green', payload: { type: 'string', value: 'green' } },
+      ],
+    },
+  ] as unknown as FactorDTO[];
+
+  const CONDITION_ORDERS = [
+    CONDITION_ORDER.ORDERED_ROUND_ROBIN,
+    CONDITION_ORDER.RANDOM,
+    CONDITION_ORDER.RANDOM_ROUND_ROBIN,
+  ];
+
+  describe.each(CONDITION_ORDERS)('with conditionOrder %s', (conditionOrder) => {
+    it.each([0, 1, 2, 5])(
+      'should produce identical output regardless of incoming condition order (enrollment count %i)',
+      (repeatedEnrollmentLength) => {
+        const fromScrambled = buildWithinSubjectOrderedConditions(
+          makeExperiment(scrambled(), conditionOrder),
+          factors,
+          USER_ID,
+          repeatedEnrollmentLength
+        );
+        const fromSorted = buildWithinSubjectOrderedConditions(
+          makeExperiment(sorted(), conditionOrder),
+          factors,
+          USER_ID,
+          repeatedEnrollmentLength
+        );
+
+        expect(fromScrambled.orderedConditions.map((condition) => condition.conditionCode)).toEqual(
+          fromSorted.orderedConditions.map((condition) => condition.conditionCode)
+        );
+      }
+    );
+
+    it('should not mutate the experiment conditions it was handed', () => {
+      const conditions = scrambled();
+      const experiment = makeExperiment(conditions, conditionOrder);
+      const snapshot = JSON.parse(JSON.stringify(experiment));
+
+      buildWithinSubjectOrderedConditions(experiment, factors, USER_ID, 1);
+      buildWithinSubjectOrderedConditions(experiment, factors, USER_ID, 1);
+
+      expect(JSON.parse(JSON.stringify(experiment))).toEqual(snapshot);
+      expect(experiment.conditions).toBe(conditions);
+      expect(experiment.conditions.map((condition) => condition.conditionCode)).toEqual(['C', 'A', 'B']);
+    });
+  });
+
+  describe('ORDERED_ROUND_ROBIN', () => {
+    // The strongest statement of the bug CI caught: the rotation baseline is `order`, not array
+    // position, so an unsorted input must still start at the order-1 condition.
+    it('should start the rotation at the lowest-order condition, not the first array element', () => {
+      const { orderedConditions } = buildWithinSubjectOrderedConditions(
+        makeExperiment(scrambled(), CONDITION_ORDER.ORDERED_ROUND_ROBIN),
+        factors,
+        USER_ID,
+        0
+      );
+
+      expect(orderedConditions.map((condition) => condition.conditionCode)).toEqual(['A', 'B', 'C']);
+    });
+
+    it('should advance the rotation by the repeated enrollment count', () => {
+      const rotationFor = (repeatedEnrollmentLength: number) =>
+        buildWithinSubjectOrderedConditions(
+          makeExperiment(scrambled(), CONDITION_ORDER.ORDERED_ROUND_ROBIN),
+          factors,
+          USER_ID,
+          repeatedEnrollmentLength
+        ).orderedConditions.map((condition) => condition.conditionCode);
+
+      expect(rotationFor(1)).toEqual(['B', 'C', 'A']);
+      expect(rotationFor(2)).toEqual(['C', 'A', 'B']);
+      // wraps back around
+      expect(rotationFor(3)).toEqual(['A', 'B', 'C']);
+    });
+  });
+
+  describe('factorial experiments', () => {
+    it('should keep orderedFactors aligned with orderedConditions regardless of incoming order', () => {
+      const fromScrambled = buildWithinSubjectOrderedConditions(
+        makeExperiment(scrambled(), CONDITION_ORDER.ORDERED_ROUND_ROBIN, EXPERIMENT_TYPE.FACTORIAL),
+        factors,
+        USER_ID,
+        0
+      );
+
+      expect(fromScrambled.orderedConditions.map((condition) => condition.conditionCode)).toEqual(['A', 'B', 'C']);
+      // condition A carries level-a (Red), B carries level-b (Blue), C carries level-c (Green) — the
+      // factor array must be permuted in lockstep with the conditions, not left in arrival order.
+      expect(fromScrambled.orderedFactors.map((factor) => factor['Color'].level)).toEqual(['Red', 'Blue', 'Green']);
+    });
+  });
+
+  describe('single-condition experiments', () => {
+    it('should return the condition untouched without consulting conditionOrder', () => {
+      const { orderedConditions } = buildWithinSubjectOrderedConditions(
+        makeExperiment([makeCondition('solo', 1)], CONDITION_ORDER.RANDOM),
+        factors,
+        USER_ID,
+        3
+      );
+
+      expect(orderedConditions.map((condition) => condition.conditionCode)).toEqual(['solo']);
+    });
+  });
+});