From b1931d0b3b952d67416a589bf0c2b95fe80038db Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Wed, 13 May 2026 09:34:46 +0200 Subject: [PATCH] [web] cleanup archived split-test assignments from user record on login (#33365) * [web] cleanup archived split-test assignments from user record on login Co-authored-by: Anna Claire Fields * [migrations] purge archived split tests from all users Co-authored-by: Anna Claire Fields * [web] add missing mock and update snapshot test * [web] gracefully access db.users.splitTests --------- Co-authored-by: Anna Claire Fields GitOrigin-RevId: bd185074a402556d7b7c812208cf834dd52b27a5 --- .../AuthenticationController.mjs | 5 + .../Features/SplitTests/SplitTestHandler.mjs | 16 +++ .../src/CleanupUsersSplitTestsMigration.mjs | 121 ++++++++++++++++++ .../AuthenticationController.test.mjs | 11 ++ ...260504100000_cleanup_users_split_tests.mjs | 30 +++++ 5 files changed, 183 insertions(+) create mode 100644 services/web/test/acceptance/src/CleanupUsersSplitTestsMigration.mjs create mode 100644 tools/migrations/20260504100000_cleanup_users_split_tests.mjs diff --git a/services/web/app/src/Features/Authentication/AuthenticationController.mjs b/services/web/app/src/Features/Authentication/AuthenticationController.mjs index e32468b5bc..a598f601c0 100644 --- a/services/web/app/src/Features/Authentication/AuthenticationController.mjs +++ b/services/web/app/src/Features/Authentication/AuthenticationController.mjs @@ -25,6 +25,7 @@ import Modules from '../../infrastructure/Modules.mjs' import { expressify, promisify } from '@overleaf/promise-utils' import { handleAuthenticateErrors } from './AuthenticationErrors.mjs' import EmailHelper from '../Helpers/EmailHelper.mjs' +import SplitTestHandler from '../SplitTests/SplitTestHandler.mjs' const { hasAdminAccess } = AdminAuthorizationHelper @@ -647,6 +648,10 @@ function _loginAsyncHandlers(req, user, anonymousAnalyticsId, isNewUser) { UserHandler.promises.populateTeamInvites(user).catch(err => { logger.warn({ err }, 'error setting up login data') }) + SplitTestHandler.promises.userMaintenanceOnLogin(user).catch(err => { + const userId = user._id + logger.warn({ err, userId }, 'error cleaning up split-tests on login') + }) LoginRateLimiter.recordSuccessfulLogin(user.email, () => {}) AuthenticationController._recordSuccessfulLogin(user._id, () => {}) AuthenticationController.ipMatchCheck(req, user) diff --git a/services/web/app/src/Features/SplitTests/SplitTestHandler.mjs b/services/web/app/src/Features/SplitTests/SplitTestHandler.mjs index 5dd03a39af..baf8db6b49 100644 --- a/services/web/app/src/Features/SplitTests/SplitTestHandler.mjs +++ b/services/web/app/src/Features/SplitTests/SplitTestHandler.mjs @@ -978,6 +978,21 @@ async function decrementLabsVariantCounter(splitTestName) { } } +async function userMaintenanceOnLogin(user) { + const splitTests = (await SplitTestCache.get('')).values() + const toCleanup = {} + for (const splitTest of splitTests) { + if (splitTest.archived && user.splitTests?.[splitTest.name]) { + toCleanup[`splitTests.${splitTest.name}`] = 1 + } + } + if (Object.keys(toCleanup).length > 0) { + await UserUpdater.promises.updateUser(user._id, { + $unset: toCleanup, + }) + } +} + export default { getPercentile, getAssignment: callbackify(getAssignment), @@ -997,5 +1012,6 @@ export default { hasUserBeenAssignedToVariant, decrementLabsVariantCounter, incrementLabsVariantCounterIfBelowLimit, + userMaintenanceOnLogin, }, } diff --git a/services/web/test/acceptance/src/CleanupUsersSplitTestsMigration.mjs b/services/web/test/acceptance/src/CleanupUsersSplitTestsMigration.mjs new file mode 100644 index 0000000000..d7f5fd9fd7 --- /dev/null +++ b/services/web/test/acceptance/src/CleanupUsersSplitTestsMigration.mjs @@ -0,0 +1,121 @@ +import { expect } from 'chai' +import { db, ObjectId } from '../../../app/src/infrastructure/mongodb.mjs' +import { exec } from 'node:child_process' + +describe('CleanupUsersSplitTestsMigration', function () { + beforeEach('insert data', async function () { + await db.splittests.insertMany([ + { name: 'archived-test', archived: true }, + { name: 'non-archived-test', archived: false }, + ]) + await db.users.insertMany([ + { + _id: new ObjectId('50e434d90000000000000000'), + email: 'foo0@bar.com', + }, + { + _id: new ObjectId('50e434d90000000000000001'), + email: 'foo1@bar.com', + splitTests: { + 'archived-test': [ + { + variantName: 'default', + versionNumber: 2, + phase: 'release', + assignedAt: new Date('2025-10-22T14:23:29.738Z'), + }, + ], + }, + }, + { + _id: new ObjectId('50e434d90000000000000002'), + email: 'foo2@bar.com', + splitTests: { + 'non-archived-test': [ + { + variantName: 'default', + versionNumber: 2, + phase: 'release', + assignedAt: new Date('2025-10-22T14:23:29.738Z'), + }, + ], + }, + }, + { + _id: new ObjectId('50e434d90000000000000003'), + email: 'foo3@bar.com', + splitTests: { + 'non-archived-test': [ + { + variantName: 'default', + versionNumber: 2, + phase: 'release', + assignedAt: new Date('2025-10-22T14:23:29.738Z'), + }, + ], + 'archived-test': [ + { + variantName: 'default', + versionNumber: 2, + phase: 'release', + assignedAt: new Date('2025-10-22T14:23:29.738Z'), + }, + ], + }, + }, + ]) + }) + + beforeEach('run migration', function (done) { + exec( + 'cd ../../tools/migrations && yarn run migrations migrate -t saas --force 20260504100000_cleanup_users_split_tests', + done + ) + }) + + it('should update the users', async function () { + expect( + await db.users + .find({}, { projection: { _id: 1, email: 1, splitTests: 1 } }) + .toArray() + ).to.deep.equal([ + { + _id: new ObjectId('50e434d90000000000000000'), + email: 'foo0@bar.com', + }, + { + _id: new ObjectId('50e434d90000000000000001'), + email: 'foo1@bar.com', + splitTests: {}, + }, + { + _id: new ObjectId('50e434d90000000000000002'), + email: 'foo2@bar.com', + splitTests: { + 'non-archived-test': [ + { + variantName: 'default', + versionNumber: 2, + phase: 'release', + assignedAt: new Date('2025-10-22T14:23:29.738Z'), + }, + ], + }, + }, + { + _id: new ObjectId('50e434d90000000000000003'), + email: 'foo3@bar.com', + splitTests: { + 'non-archived-test': [ + { + variantName: 'default', + versionNumber: 2, + phase: 'release', + assignedAt: new Date('2025-10-22T14:23:29.738Z'), + }, + ], + }, + }, + ]) + }) +}) diff --git a/services/web/test/unit/src/Authentication/AuthenticationController.test.mjs b/services/web/test/unit/src/Authentication/AuthenticationController.test.mjs index b10da9b1db..0edfa4f002 100644 --- a/services/web/test/unit/src/Authentication/AuthenticationController.test.mjs +++ b/services/web/test/unit/src/Authentication/AuthenticationController.test.mjs @@ -102,6 +102,17 @@ describe('AuthenticationController', function () { }) ) + vi.doMock( + '../../../../app/src/Features/SplitTests/SplitTestHandler', + () => ({ + default: (ctx.SplitTestHandler = { + promises: { + userMaintenanceOnLogin: sinon.stub().resolves(), + }, + }), + }) + ) + vi.doMock('../../../../app/src/Features/User/UserUpdater', () => ({ default: (ctx.UserUpdater = { updateUser: sinon.stub(), diff --git a/tools/migrations/20260504100000_cleanup_users_split_tests.mjs b/tools/migrations/20260504100000_cleanup_users_split_tests.mjs new file mode 100644 index 0000000000..7887c877b1 --- /dev/null +++ b/tools/migrations/20260504100000_cleanup_users_split_tests.mjs @@ -0,0 +1,30 @@ +import { db } from './lib/mongodb.mjs' +import { batchedUpdate } from '@overleaf/mongo-utils/batchedUpdate.js' + +const tags = ['saas', 'server-ce', 'server-pro'] + +const migrate = async () => { + const splitTests = await db.splittests.find().toArray() + const toCleanup = {} + for (const splitTest of splitTests) { + if (splitTest.archived) { + toCleanup[`splitTests.${splitTest.name}`] = 1 + } + } + // Take the easy route and unset all the archived split tests on all the users with any split test assignment. + await batchedUpdate( + db.users, + { splitTests: { $exists: true } }, + { $unset: toCleanup } + ) +} + +const rollback = async () => { + // The data is gone. Nothing to revert to. +} + +export default { + tags, + migrate, + rollback, +}