From 513f61ab5a6e978e46103ece0647a216a6a3a7f9 Mon Sep 17 00:00:00 2001 From: doswalt Date: Fri, 7 Aug 2026 16:37:53 -0400 Subject: [PATCH 1/3] Remove per-request JSON deep copy from getCachedValidExperiments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit getCachedValidExperiments wrapped every cache hit in JSON.parse(JSON.stringify(...)). That defensive copy existed because formattingConditionPayload mutated the experiment it was given — it deleted conditionPayloads off the cached conditions/partitions in place. Since the cache is in-memory and hands back the same object reference to every request, mutating there would corrupt the cached graph for all subsequent requests. formattingConditionPayload now rebuilds the stripped conditions and partitions as new objects instead of deleting fields in place, so the cached graph is never written to and the per-request deep copy is gone. The parentCondition/decisionPoint back-references point at the rebuilt objects, so reference identity within the returned experiment matches what the in-place version produced. Five tests in ExperimentService.test.ts guard the non-mutation invariant that makes removing the copy safe, covering both the simple and factorial paths. Co-Authored-By: Claude Opus 5 (1M context) --- .../services/ExperimentAssignmentService.ts | 3 +- .../src/api/services/ExperimentService.ts | 57 ++++++++---- .../unit/services/ExperimentService.test.ts | 86 +++++++++++++++++++ 3 files changed, 127 insertions(+), 19 deletions(-) diff --git a/packages/backend/src/api/services/ExperimentAssignmentService.ts b/packages/backend/src/api/services/ExperimentAssignmentService.ts index a618edb56..1b1d44b0b 100644 --- a/packages/backend/src/api/services/ExperimentAssignmentService.ts +++ b/packages/backend/src/api/services/ExperimentAssignmentService.ts @@ -708,7 +708,8 @@ 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. + // formattingConditionPayload does not mutate, so the cached experiments above stay pristine. return experiments.map((exp) => this.experimentService.formattingConditionPayload(exp)); } diff --git a/packages/backend/src/api/services/ExperimentService.ts b/packages/backend/src/api/services/ExperimentService.ts index f3ef511a1..b13d9f959 100644 --- a/packages/backend/src/api/services/ExperimentService.ts +++ b/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 { 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 { diff --git a/packages/backend/test/unit/services/ExperimentService.test.ts b/packages/backend/test/unit/services/ExperimentService.test.ts index 3ce8faa3d..918b5628e 100644 --- a/packages/backend/test/unit/services/ExperimentService.test.ts +++ b/packages/backend/test/unit/services/ExperimentService.test.ts @@ -1173,4 +1173,90 @@ describe('ExperimentService Testing', () => { expect(result).toContain(`name ILIKE '%100\\%\\_path\\\\name%' ESCAPE '\\'`); }); }); + + describe('formattingConditionPayload()', () => { + // 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 the input would corrupt the cached + // graph for every subsequent request — which is exactly why this call site used to need a full + // deep copy of the experiment per request. These tests guard the invariant that removed it. + const buildSimpleExperiment = (): Experiment => { + const conditionA = { id: 'condition-a', order: 1 } as ExperimentCondition; + const conditionB = { id: 'condition-b', order: 2 } as ExperimentCondition; + const decisionPoint = { id: 'dp-1', site: 'SelectSection', target: 'target-1' } as DecisionPoint; + + // Deliberately out of order so the sort inside the formatter has something to do. + decisionPoint.conditionPayloads = [ + { id: 'payload-b', parentCondition: conditionB } as ConditionPayload, + { id: 'payload-a', parentCondition: conditionA } as ConditionPayload, + ]; + + return { + id: 'experiment-1', + type: EXPERIMENT_TYPE.SIMPLE, + conditions: [conditionA, conditionB], + partitions: [decisionPoint], + } as Experiment; + }; + + const buildFactorialExperiment = (): Experiment => { + const condition = { id: 'condition-a', order: 1 } as ExperimentCondition; + condition.conditionPayloads = [{ id: 'payload-a' } as ConditionPayload]; + + return { + id: 'experiment-2', + type: EXPERIMENT_TYPE.FACTORIAL, + conditions: [condition], + partitions: [], + } as Experiment; + }; + + it('should not mutate the input experiment for a simple experiment', () => { + const experiment = buildSimpleExperiment(); + const snapshot = JSON.parse(JSON.stringify(experiment)); + + service.formattingConditionPayload(experiment); + + expect(JSON.parse(JSON.stringify(experiment))).toEqual(snapshot); + expect(experiment.partitions[0].conditionPayloads).toHaveLength(2); + }); + + it('should not mutate the input experiment for a factorial experiment', () => { + const experiment = buildFactorialExperiment(); + const snapshot = JSON.parse(JSON.stringify(experiment)); + + service.formattingConditionPayload(experiment); + + expect(JSON.parse(JSON.stringify(experiment))).toEqual(snapshot); + expect(experiment.conditions[0].conditionPayloads).toHaveLength(1); + }); + + it('should hoist payloads to the root, strip them from decision points, and sort by condition order', () => { + const result = service.formattingConditionPayload(buildSimpleExperiment()); + + expect(result.conditionPayloads.map((payload) => payload.id)).toEqual(['payload-a', 'payload-b']); + expect(result.partitions[0].conditionPayloads).toBeUndefined(); + // decisionPoint back-references must point at the stripped partition on the returned experiment + expect(result.conditionPayloads[0].decisionPoint).toBe(result.partitions[0]); + }); + + it('should hoist payloads to the root and strip them from conditions for a factorial experiment', () => { + const result = service.formattingConditionPayload(buildFactorialExperiment()); + + expect(result.conditionPayloads.map((payload) => payload.id)).toEqual(['payload-a']); + expect(result.conditions[0].conditionPayloads).toBeUndefined(); + // parentCondition back-references must point at the stripped condition on the returned experiment + expect(result.conditionPayloads[0].parentCondition).toBe(result.conditions[0]); + }); + + it('should leave repeated formatting of the same cached experiment stable', () => { + const experiment = buildSimpleExperiment(); + + const first = service.formattingConditionPayload(experiment); + const second = service.formattingConditionPayload(experiment); + + expect(second.conditionPayloads.map((payload) => payload.id)).toEqual( + first.conditionPayloads.map((payload) => payload.id) + ); + }); + }); }); From 03e4de7c156fd111fc2fb1b61e280975f0ac78eb Mon Sep 17 00:00:00 2001 From: doswalt Date: Mon, 10 Aug 2026 13:35:51 -0400 Subject: [PATCH 2/3] add test to ensure we get the same behavior we used to with JSON.parse(JSON.stringify(exp)) --- .../services/ExperimentAssignmentService.ts | 9 +- .../ExperimentAssignmentService.test.ts | 105 +++++++++++++++++- 2 files changed, 109 insertions(+), 5 deletions(-) diff --git a/packages/backend/src/api/services/ExperimentAssignmentService.ts b/packages/backend/src/api/services/ExperimentAssignmentService.ts index 1b1d44b0b..790bec91f 100644 --- a/packages/backend/src/api/services/ExperimentAssignmentService.ts +++ b/packages/backend/src/api/services/ExperimentAssignmentService.ts @@ -709,7 +709,6 @@ export class ExperimentAssignmentService { ? await this.experimentRepository.getValidExperimentsWithPreview(context) : await this.experimentService.getCachedValidExperiments(context); // adding conditionPayloads at the root level instead of inside conditions. - // formattingConditionPayload does not mutate, so the cached experiments above stay pristine. return experiments.map((exp) => this.experimentService.formattingConditionPayload(exp)); } @@ -2068,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); @@ -2086,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; } diff --git a/packages/backend/test/unit/services/ExperimentAssignmentService.test.ts b/packages/backend/test/unit/services/ExperimentAssignmentService.test.ts index 2b039345f..4bf846d24 100644 --- a/packages/backend/test/unit/services/ExperimentAssignmentService.test.ts +++ b/packages/backend/test/unit/services/ExperimentAssignmentService.test.ts @@ -238,6 +238,97 @@ describe('Experiment Assignment Service Test', () => { expect(result[0].assignedCondition[0]).toEqual(cond); }); + describe('cached experiment graph is never mutated', () => { + // getCachedValidExperiments hands the SAME object graph to every request — the cache is an + // in-memory store and the per-request `JSON.parse(JSON.stringify(...))` defensive copy was removed + // for performance (it cost ~8ms of blocking CPU per request on a production-sized payload). + // + // That makes non-mutation a load-bearing invariant rather than a nicety: an in-place write here + // (`.sort()`, `.splice()`, `delete`, a nested field assignment) corrupts the graph for every + // subsequent request in that context until the cache entry expires, silently and with no error. + // It also will not reproduce in local dev, where caching is typically off and every request gets + // a freshly loaded graph. These tests fail loudly instead. + // + // Note that top-level writes are safe by construction — formattingConditionPayload returns a + // `{...experiment}` shallow copy — so what these guard is mutation one or more levels down. + const expectNoMutation = async (exp: any, run: () => Promise) => { + const snapshot = JSON.parse(JSON.stringify(exp)); + + // Twice: a mutation that is idempotent (an in-place sort, say) can still be caught on the first + // pass, and running twice also covers request N+1 seeing what request N left behind. + await run(); + await run(); + + expect(JSON.parse(JSON.stringify(exp))).toEqual(snapshot); + }; + + it('should not mutate the cached experiment for a simple individual experiment', async () => { + const context = 'context'; + const userDoc = { id: 'user123', group: { schoolId: ['school1'] }, workingGroup: {} }; + const exp = structuredClone(simpleIndividualAssignmentExperiment); + + testedModule.cacheService.wrap = sandbox.stub().resolves([exp]); + testedModule.experimentRepository.getValidExperimentsWithPreview = sandbox.stub().resolves([]); + testedModule.experimentService.getCachedValidExperiments = sandbox.stub().resolves([exp]); + testedModule.experimentUserService = { getOriginalUserDoc: sandbox.stub().resolves(userDoc) }; + + await expectNoMutation(exp, () => testedModule.getAllExperimentConditions(userDoc, context, loggerMock)); + }); + + it('should not reorder the cached conditions of a simple group experiment', async () => { + // This fixture stores its conditions out of `order` on purpose, so an in-place sort on the + // assignment path — which is exactly the bug this guards — shows up as a reordering. + const context = 'context'; + const userDoc = { + id: 'user123', + group: { 'add-group1': ['school1'] }, + workingGroup: { 'add-group1': 'school1' }, + }; + const exp: any = structuredClone(simpleGroupAssignmentExperiment); + expect(exp.conditions.map((condition) => condition.order)).toEqual([2, 1]); + + const groupEnrollment = new GroupEnrollment(); + groupEnrollment.experiment = exp; + groupEnrollment.condition = exp.conditions[0]; + groupEnrollment.groupId = 'add-group1'; + + groupEnrollmentRepositoryMock = { + findEnrollments: sandbox.stub().resolves([groupEnrollment]), + delete: sandbox.stub().resolves(), + }; + + testedModule.experimentService.getCachedValidExperiments = sandbox.stub().resolves([exp]); + testedModule.experimentService.checkUserOrGroupIsGloballyExcluded = sandbox.stub().resolves([false, false]); + testedModule.experimentService.getAssignmentsAndExclusionsForUser = sandbox + .stub() + .resolves([ + individualEnrollmentRepositoryMock, + groupEnrollmentRepositoryMock, + individualExclusionRepositoryMock, + groupExclusionRepositoryMock, + ]); + testedModule.experimentUserService = { getOriginalUserDoc: sandbox.stub().resolves(userDoc) }; + + await expectNoMutation(exp, () => testedModule.getAllExperimentConditions(userDoc, context, loggerMock)); + }); + + it('should not mutate the cached experiment for a factorial group experiment', async () => { + const context = 'context'; + const userDoc = { + id: 'user123', + group: { 'add-group1': ['school1'] }, + workingGroup: { 'add-group1': 'school1' }, + }; + const exp: any = structuredClone(factorialGroupAssignmentExperiment); + + testedModule.experimentService.getCachedValidExperiments = sandbox.stub().resolves([exp]); + testedModule.experimentService.checkUserOrGroupIsGloballyExcluded = sandbox.stub().resolves([false, false]); + testedModule.experimentUserService = { getOriginalUserDoc: sandbox.stub().resolves(userDoc) }; + + await expectNoMutation(exp, () => testedModule.getAllExperimentConditions(userDoc, context, loggerMock)); + }); + }); + it('should not pool experiments together due to a shared pending decision point site/target', async () => { const context = 'context'; const userDoc = { id: 'user123', group: { schoolId: ['school1'] }, workingGroup: {} }; @@ -492,12 +583,18 @@ describe('Experiment Assignment Service Test', () => { const result = await testedModule.getAllExperimentConditions(userDoc, context, loggerMock); - const cond = { ...exp.conditions[0], experimentId: exp.id, payload: undefined }; + // Assignment picks from the conditions sorted by `order`, and this fixture is deliberately stored + // out of order. Name the expected condition by its order rather than by fixture position — the + // assignment path must not reorder `exp.conditions` in place (it can be the cached array). + const lowestOrderCondition = [...exp.conditions].sort((a, b) => a.order - b.order)[0]; + const cond = { ...lowestOrderCondition, experimentId: exp.id, payload: undefined }; expect(result.length).toEqual(1); expect(result[0].site).toEqual(exp.partitions[0].site); expect(result[0].target).toEqual(exp.partitions[0].target); expect(result[0].assignedFactor).toBeNull(); expect(result[0].assignedCondition[0]).toEqual(cond); + // the fixture order is untouched by the assignment path + expect(exp.conditions.map((condition) => condition.conditionCode)).toEqual(['add-con2', 'add-con1']); }); it('should return the assigned condition for a factorial group experiment', async () => { @@ -1795,13 +1892,17 @@ describe('Experiment Assignment Service Test', () => { expect(Object.keys(result)).toHaveLength(2); + // Assignment picks from the conditions sorted by `order`, and this fixture is deliberately + // stored out of order — the assignment path must not reorder `exp.conditions` in place. + const lowestOrderCondition = [...exp.conditions].sort((a, b) => a.order - b.order)[0]; + // Both users should get same assignment since they're in same group for (const userId of ['user1', 'user2']) { expect(result[userId]).toBeDefined(); expect(result[userId].site).toEqual(site); expect(result[userId].target).toEqual(target); expect(result[userId].assignedCondition[0].experimentId).toEqual(exp.id); - expect(result[userId].assignedCondition[0].conditionCode).toEqual(exp.conditions[0].conditionCode); + expect(result[userId].assignedCondition[0].conditionCode).toEqual(lowestOrderCondition.conditionCode); } }); From 88503c22354be92af2a6e98d523ed8ab78a7c7e5 Mon Sep 17 00:00:00 2001 From: doswalt Date: Mon, 10 Aug 2026 14:36:32 -0400 Subject: [PATCH 3/3] add additional tests to guard against future accidental mutation --- packages/backend/src/api/Algorithms.ts | 13 +- .../test/unit/services/Algorithms.test.ts | 175 ++++++++++++++++++ .../ExperimentAssignmentService.test.ts | 103 +++++++++++ 3 files changed, 289 insertions(+), 2 deletions(-) create mode 100644 packages/backend/test/unit/services/Algorithms.test.ts diff --git a/packages/backend/src/api/Algorithms.ts b/packages/backend/src/api/Algorithms.ts index b93814536..54048d604 100644 --- a/packages/backend/src/api/Algorithms.ts +++ b/packages/backend/src/api/Algorithms.ts @@ -117,7 +117,16 @@ export function buildWithinSubjectOrderedConditions( orderedConditions: IExperimentAssignmentv5['assignedCondition']; orderedFactors: Record[] | 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[] | null = experiment.type === EXPERIMENT_TYPE.FACTORIAL - ? experiment.conditions.map((condition) => getAssignedFactor(condition, factors)) + ? orderedExperimentConditions.map((condition) => getAssignedFactor(condition, factors)) : null; let assignedData: IExperimentAssignmentv5 = { diff --git a/packages/backend/test/unit/services/Algorithms.test.ts b/packages/backend/test/unit/services/Algorithms.test.ts new file mode 100644 index 000000000..41a630b9c --- /dev/null +++ b/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']); + }); + }); +}); diff --git a/packages/backend/test/unit/services/ExperimentAssignmentService.test.ts b/packages/backend/test/unit/services/ExperimentAssignmentService.test.ts index 4bf846d24..3f3abc211 100644 --- a/packages/backend/test/unit/services/ExperimentAssignmentService.test.ts +++ b/packages/backend/test/unit/services/ExperimentAssignmentService.test.ts @@ -27,6 +27,7 @@ import { factorialIndividualAssignmentExperiment, simpleDPExperiment, simpleWithinSubjectOrderedRoundRobinExperiment, + simpleWithinSubjectRandomRoundRobinExperiment, withinSubjectDPExperiment, } from '../mockdata'; import { GroupEnrollment } from '../../../src/api/models/GroupEnrollment'; @@ -329,6 +330,108 @@ describe('Experiment Assignment Service Test', () => { }); }); + describe('assignment does not depend on the order rows come back from the database', () => { + // `getValidExperiments` — the assignment read path — has no `ORDER BY conditions.order`; only the + // admin-side `findOneExperiment` does. So the sequence conditions arrive in is whatever Postgres + // felt like, and it can change as rows are updated. + // + // For a long time one caller got away with depending on arrival order: the within-subjects + // builder read `experiment.conditions` positionally and was correct only because `assignRandom` + // had sorted that array in place earlier in the same request. These tests state the invariant + // directly — same input, shuffled, must produce the same assignment — so the next caller that + // leans on arrival order fails here rather than in CI's integration suite or in production data. + const expectOrderIndependence = async ( + buildExperiment: () => any, + setup: (exp: any, userDoc: any) => void, + userDoc: any + ) => { + const context = 'context'; + + const run = async (reverseConditions: boolean) => { + const exp = buildExperiment(); + if (reverseConditions) { + exp.conditions = [...exp.conditions].reverse(); + } + setup(exp, userDoc); + return testedModule.getAllExperimentConditions(userDoc, context, loggerMock); + }; + + const asStored = await run(false); + const reversed = await run(true); + + expect(reversed).toEqual(asStored); + // guard the guard: a fixture with one condition would make this vacuous + expect(buildExperiment().conditions.length).toBeGreaterThan(1); + }; + + it('should assign the same condition for a simple individual experiment', async () => { + const userDoc = { id: 'user123', group: { schoolId: ['school1'] }, workingGroup: {} }; + + await expectOrderIndependence( + () => structuredClone(simpleIndividualAssignmentExperiment), + (exp) => { + testedModule.experimentService.getCachedValidExperiments = sandbox.stub().resolves([exp]); + testedModule.experimentUserService = { getOriginalUserDoc: sandbox.stub().resolves(userDoc) }; + }, + userDoc + ); + }); + + it('should produce the same rotation for a within-subjects ORDERED_ROUND_ROBIN experiment', async () => { + // The unit-level analogue of the integration failure: rotation is anchored to each condition's + // `order`, so reversing the incoming array must not change the sequence the user receives. + const userDoc = { id: 'user123', group: { schoolId: ['school1'] }, workingGroup: {} }; + + await expectOrderIndependence( + () => structuredClone(simpleWithinSubjectOrderedRoundRobinExperiment), + (exp) => { + testedModule.experimentService.getCachedValidExperiments = sandbox.stub().resolves([exp]); + testedModule.experimentUserService = { getOriginalUserDoc: sandbox.stub().resolves(userDoc) }; + // a non-zero count so the rotation actually advances rather than sitting at position 0 + testedModule.repeatedEnrollmentRepository = { + getRepeatedEnrollmentCount: sandbox + .stub() + .resolves([{ userId: userDoc.id, experimentId: exp.id, count: 1 }]), + }; + }, + userDoc + ); + }); + + it('should produce the same rotation for a within-subjects RANDOM_ROUND_ROBIN experiment', async () => { + // The seeded shuffles consume the array too, so their output is order-dependent unless the + // input is normalized first. + const userDoc = { id: 'user123', group: { schoolId: ['school1'] }, workingGroup: {} }; + + await expectOrderIndependence( + () => structuredClone(simpleWithinSubjectRandomRoundRobinExperiment), + (exp) => { + testedModule.experimentService.getCachedValidExperiments = sandbox.stub().resolves([exp]); + testedModule.experimentUserService = { getOriginalUserDoc: sandbox.stub().resolves(userDoc) }; + testedModule.repeatedEnrollmentRepository = { + getRepeatedEnrollmentCount: sandbox + .stub() + .resolves([{ userId: userDoc.id, experimentId: exp.id, count: 1 }]), + }; + }, + userDoc + ); + }); + + it('should assign the same condition and factors for a factorial individual experiment', async () => { + const userDoc = { id: 'user123', group: { schoolId: ['school1'] }, workingGroup: {} }; + + await expectOrderIndependence( + () => structuredClone(factorialIndividualAssignmentExperiment), + (exp) => { + testedModule.experimentService.getCachedValidExperiments = sandbox.stub().resolves([exp]); + testedModule.experimentUserService = { getOriginalUserDoc: sandbox.stub().resolves(userDoc) }; + }, + userDoc + ); + }); + }); + it('should not pool experiments together due to a shared pending decision point site/target', async () => { const context = 'context'; const userDoc = { id: 'user123', group: { schoolId: ['school1'] }, workingGroup: {} };