From 3ef95ee66d943b38478f73782c316f0ce3cfaba7 Mon Sep 17 00:00:00 2001 From: Chris Paskvan Date: Sat, 26 Sep 2026 19:49:32 -0500 Subject: [PATCH 1/4] fix(users): support the intended Bungie platforms during first sign-in Steam-first accounts could not sign in. getCurrentUser() returns the membership that owns cross-saved data - Steam (3) for most PC players - while anonymousUserSchema validated membershipType as min(1).max(2), so createAnonymousUser rejected a valid account and the OAuth callback 500'd. Define the supported platforms once in helpers/bungie.membershipTypes.js (1 Xbox, 2 PSN, 3 Steam, 6 Epic) and validate both user schemas against it. userSchema was previously unbounded, so the two now agree. Fix two adjacent defects on the same path. #getPreferredMembership destructured memberships[0], throwing a TypeError on an account with no Destiny memberships rather than reaching the 404 the route already implements; it also read crossSaveOverride off index 0 and fell back to index 0, which can return a membership that does not own the data. It now uses the rule already proven in twilio/mms.service.js and resolves undefined when nothing is playable. Also recognise a player who moves their cross-save owner. That changes both the platform membershipId and the membershipType, so sign-in missed the existing record and created a second document in a second Cosmos partition, stranding the registration on the first. Fall back to the Bungie.net membership id, then move the record rather than update it: membershipType is the partition key and is fixed for a document's life. Widen the anonymous displayName ceiling to 32 for Steam persona names. Closes #718 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_018iD3V7Zyugg3atXVsogTBf --- destiny/destiny.controller.js | 2 +- destiny/destiny.service.js | 57 ++++-- destiny/destiny.service.spec.js | 99 +++++++++++ helpers/bungie.membershipTypes.js | 27 +++ mcp/mcp.routes.js | 13 ++ users/user.controller.js | 32 +++- users/user.controller.spec.js | 278 ++++++++++++++++++++++++++++-- users/user.service.js | 137 ++++++++++++++- users/user.service.spec.js | 210 ++++++++++++++++++++++ 9 files changed, 824 insertions(+), 31 deletions(-) create mode 100644 helpers/bungie.membershipTypes.js diff --git a/destiny/destiny.controller.js b/destiny/destiny.controller.js index 37fd1086..d172a210 100644 --- a/destiny/destiny.controller.js +++ b/destiny/destiny.controller.js @@ -63,7 +63,7 @@ class DestinyController { * * @param {string} displayName * @param {number} membershipType - * @returns {Promise} + * @returns {Promise} */ async getCurrentUser(displayName, membershipType) { const currentUser = await this.users.getUserByDisplayName(displayName, membershipType); diff --git a/destiny/destiny.service.js b/destiny/destiny.service.js index 360418d3..80d48864 100644 --- a/destiny/destiny.service.js +++ b/destiny/destiny.service.js @@ -12,8 +12,10 @@ */ import { stringify } from 'qs'; import { get, post } from '../helpers/bungie.request.js'; +import supportedMembershipTypes from '../helpers/bungie.membershipTypes.js'; import DestinyError from './destiny.error.js'; import configuration from '../helpers/config.js'; +import log from '../helpers/log.js'; const { bungie: { apiKey, host, clientId, clientSecret }, @@ -52,8 +54,8 @@ const { * @typedef {Object} DestinyMembership * @property {string} displayName * @property {string} membershipId - * @property {number} membershipType - Platform: 1 Xbox, 2 PSN, 3 Steam, etc. - * @property {number} [crossSaveOverride] - The membershipType that owns cross-saved data + * @property {number} membershipType - A Bungie platform value; see `helpers/bungie.membershipTypes.js`. + * @property {number} [crossSaveOverride] - The membershipType that owns cross-saved data, or 0 when cross save is off */ /** @@ -61,7 +63,7 @@ const { * @typedef {Object} CurrentUser * @property {string} displayName * @property {string} membershipId - * @property {number} membershipType + * @property {SupportedMembershipType} membershipType * @property {string} [profilePicturePath] */ @@ -73,6 +75,7 @@ const { * @property {{ membershipId: string, membershipType: number }} [characterBase] */ +/** @typedef {import('../helpers/bungie.membershipTypes.js').SupportedMembershipType} SupportedMembershipType */ /** @typedef {import('./destiny.cache.js').DestinyManifest} DestinyManifest */ /** @typedef {import('./destiny.cache.js').ManifestResult} ManifestResult */ @@ -260,8 +263,13 @@ class DestinyService { /** * Get the current user based on the Bungie access token. * + * Resolves undefined when the account has nothing this application can sign + * in - no Destiny memberships at all, or none on a supported platform. That + * is a client outcome, not a failure: `users/user.routes.js` turns it into + * the same 404 an unknown user gets, having created nothing. + * * @param {string} accessToken - * @returns {Promise} + * @returns {Promise} */ async getCurrentUser(accessToken) { const options = { @@ -284,8 +292,13 @@ class DestinyService { } const { destinyMemberships, bungieNetUser: { profilePicturePath } = {} } = user; - const { displayName, membershipId, membershipType } = - this.#getPreferredMembership(destinyMemberships); + const membership = this.#getPreferredMembership(destinyMemberships); + + if (!membership) { + return undefined; + } + + const { displayName, membershipId, membershipType } = membership; return { displayName, @@ -314,17 +327,37 @@ class DestinyService { } /** - * Pick the membership that owns cross-saved data, falling back to the first. + * The membership the player actually plays on: either the one cross save + * points at, or an account that never enabled it. Every membership on a + * cross-saved account carries the owner's `membershipType`, so the owner is + * the one that names itself. * * @param {DestinyMembership[]} memberships - * @returns {DestinyMembership} + * @returns {(DestinyMembership & { membershipType: SupportedMembershipType }) | undefined} + * undefined when nothing here is playable */ #getPreferredMembership(memberships) { - const [{ crossSaveOverride }] = memberships; + const membership = memberships.find( + ({ crossSaveOverride, membershipType }) => + !crossSaveOverride || crossSaveOverride === membershipType, + ); + + if (!membership) { + log.info({ memberships: memberships.length }, 'No playable Destiny membership'); + + return undefined; + } + + const membershipType = /** @type {SupportedMembershipType} */ (membership.membershipType); + + if (!supportedMembershipTypes.includes(membershipType)) { + log.info({ membershipType }, 'Destiny membership is on an unsupported platform'); + + return undefined; + } - return ( - memberships.find(({ membershipType }) => membershipType === crossSaveOverride) || - memberships[0] + return /** @type {DestinyMembership & { membershipType: SupportedMembershipType }} */ ( + membership ); } } diff --git a/destiny/destiny.service.spec.js b/destiny/destiny.service.spec.js index 6640a34e..01171ee4 100644 --- a/destiny/destiny.service.spec.js +++ b/destiny/destiny.service.spec.js @@ -111,6 +111,105 @@ describe('DestinyService', () => { }); }); + describe('when cross save is enabled', () => { + it('should return the membership that owns the data', async () => { + const owner = { + crossSaveOverride: 3, + displayName: chance.word(), + membershipId: '3', + membershipType: 3, + }; + + get.mockImplementation(() => + Promise.resolve({ + ErrorCode: 1, + Response: { + destinyMemberships: [ + { + crossSaveOverride: 3, + displayName: chance.word(), + membershipId: '2', + membershipType: 2, + }, + owner, + ], + }, + }), + ); + + await expect(destinyService.getCurrentUser(chance.hash())).resolves.toEqual({ + displayName: owner.displayName, + membershipId: owner.membershipId, + membershipType: owner.membershipType, + profilePicturePath: undefined, + }); + }); + }); + + describe('when cross save is off', () => { + it('should return the only membership', async () => { + const membership = { + crossSaveOverride: 0, + displayName: chance.word(), + membershipId: '6', + membershipType: 6, + }; + + get.mockImplementation(() => + Promise.resolve({ + ErrorCode: 1, + Response: { destinyMemberships: [membership] }, + }), + ); + + await expect(destinyService.getCurrentUser(chance.hash())).resolves.toEqual({ + displayName: membership.displayName, + membershipId: membership.membershipId, + membershipType: membership.membershipType, + profilePicturePath: undefined, + }); + }); + }); + + describe('when the account has no Destiny memberships', () => { + it('should resolve undefined rather than throw', async () => { + get.mockImplementation(() => + Promise.resolve({ + ErrorCode: 1, + Response: { destinyMemberships: [] }, + }), + ); + + await expect( + destinyService.getCurrentUser(chance.hash()), + ).resolves.toBeUndefined(); + }); + }); + + describe('when the playable membership is on an unsupported platform', () => { + it('should resolve undefined', async () => { + get.mockImplementation(() => + Promise.resolve({ + ErrorCode: 1, + Response: { + destinyMemberships: [ + { + crossSaveOverride: 0, + displayName: chance.word(), + membershipId: '5', + membershipType: 5, + }, + ], + }, + }), + ); + + await expect( + destinyService.getCurrentUser(chance.hash()), + ).resolves.toBeUndefined(); + }); + }); + describe('when ErrorCode is not 1', () => { it('should throw carrying the error details from the response', async () => { get.mockImplementation(() => diff --git a/helpers/bungie.membershipTypes.js b/helpers/bungie.membershipTypes.js new file mode 100644 index 00000000..7be422d7 --- /dev/null +++ b/helpers/bungie.membershipTypes.js @@ -0,0 +1,27 @@ +// @ts-check +/** + * The Bungie platform values this application signs users in on, a subset of + * Bungie's `BungieMembershipType` enum. + * + * Deliberately not every value the enum defines: 0 (None) and 254 (BungieNext) + * are not playable platforms, 4 (Blizzard) was migrated to Steam in 2019, 5 + * (Stadia) retired with the service in 2023, and 10 (Demon) is internal. A + * membership on one of those is not something a Destiny player can log into + * today, so it is treated as no playable membership rather than accepted and + * stored. + * + * @type {readonly [1, 2, 3, 6]} + */ +const supportedMembershipTypes = /** @type {const} */ ([ + 1, // Xbox + 2, // PlayStation Network + 3, // Steam + 6, // Epic Games Store +]); + +/** + * One of the platform values above. + * @typedef {(typeof supportedMembershipTypes)[number]} SupportedMembershipType + */ + +export default supportedMembershipTypes; diff --git a/mcp/mcp.routes.js b/mcp/mcp.routes.js index 2270c9eb..b351c692 100644 --- a/mcp/mcp.routes.js +++ b/mcp/mcp.routes.js @@ -1,6 +1,7 @@ // @ts-check import { StreamableHTTPServerTransport } from '@modelcontextprotocol/sdk/server/streamableHttp.js'; import { Router } from 'express'; +import { StatusCodes } from 'http-status-codes'; import { createId } from '@paralleldrive/cuid2'; import { LRUCache as LruCache } from 'lru-cache'; import authorizeUser from '../authorization/authorization.middleware.js'; @@ -57,6 +58,18 @@ const routes = ({ destinyController }) => { administrator.displayName, administrator.membershipType, ); + + if (!user) { + log.error( + { displayName: administrator.displayName }, + 'The configured administrator has no playable Destiny membership.', + ); + + return res + .status(StatusCodes.SERVICE_UNAVAILABLE) + .send('Failed to initialize MCP session'); + } + const server = createMcpServer({ destinyController, user, diff --git a/users/user.controller.js b/users/user.controller.js index 135daf42..1998387b 100644 --- a/users/user.controller.js +++ b/users/user.controller.js @@ -512,16 +512,46 @@ class UserController { membershipType, profilePicturePath, }; - const destinyGhostUser = /** @type {MutableUser | undefined} */ ( + let destinyGhostUser = /** @type {MutableUser | undefined} */ ( await this.users.getUserByMembershipId(/** @type {string} */ (user.membershipId)) ); + /** + * Moving the membership that owns cross-saved data changes both the + * platform membership id and the platform, so the lookup above misses + * and the player would sign in as a stranger: a second document, in a + * second Cosmos partition, with their registration stranded on the + * first. The Bungie.net membership id on the token is the one + * identifier that survives the change. + */ + if (!destinyGhostUser) { + destinyGhostUser = /** @type {MutableUser | undefined} */ ( + await this.users.getUserByBungieMembershipId(bungie.membership_id) + ); + } + if (!destinyGhostUser) { return await this.users .createAnonymousUser(/** @type {AnonymousUser} */ (user)) .then(() => user); } + /** + * `membershipType` is the partition key, so a changed platform is a + * move rather than an update - `updateUser` would look the document up + * under the new platform, find nothing, and throw. + */ + if (destinyGhostUser.membershipType !== user.membershipType) { + return await this.users + .movePlatform( + /** @type {import('../helpers/documents.js').CosmosDocument} */ ( + /** @type {unknown} */ (destinyGhostUser) + ), + user, + ) + .then(() => user); + } + Object.assign(destinyGhostUser, user); return ( diff --git a/users/user.controller.spec.js b/users/user.controller.spec.js index 8d2f7770..0adbda3d 100644 --- a/users/user.controller.spec.js +++ b/users/user.controller.spec.js @@ -1,9 +1,13 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; import Chance from 'chance'; import getEpoch from '../helpers/get-epoch.js'; +import { get, post } from '../helpers/bungie.request.js'; +import DestinyService from '../destiny/destiny.service.js'; import UserController from './user.controller.js'; +import UserService from './user.service.js'; import InvalidPhoneNumberError from './invalid-phone-number.error.js'; +vi.mock('../helpers/bungie.request.js'); vi.mock('../helpers/postmaster.js', () => ({ default: class { register = vi.fn(); @@ -36,8 +40,10 @@ const userService = { getUserByEmailAddress: vi.fn(), getUserByEmailAddressToken: vi.fn(), getUserById: vi.fn(), + getUserByBungieMembershipId: vi.fn(), getUserByMembershipId: vi.fn(), getUserByPhoneNumber: vi.fn(), + movePlatform: vi.fn().mockImplementation(user => Promise.resolve(user)), updateAnonymousUser: vi.fn().mockImplementation(user => Promise.resolve(user)), updateUser: vi.fn().mockImplementation(user => Promise.resolve(user)), }; @@ -473,12 +479,18 @@ describe('UserController', () => { }); describe('signIn', () => { + /** + * `membership_id` is the Bungie.net id, the one identifier that + * survives a cross-save owner change. Shared between the mock and the + * assertions so the fixture cannot drift from what is asserted. + */ + const bungieToken = { + access_token: 'some-access-token', + membership_id: 'bungie-net-99', + }; + beforeEach(() => { - destinyService.getAccessTokenFromCode.mockImplementation(() => - Promise.resolve({ - access_token: 'some-access-token', - }), - ); + destinyService.getAccessTokenFromCode.mockResolvedValue(bungieToken); }); describe('when current user is not found', () => { @@ -498,9 +510,7 @@ describe('UserController', () => { const currentUser = await userController.signIn({}); expect(currentUser).toEqual({ - bungie: { - access_token: 'some-access-token', - }, + bungie: bungieToken, ...mockUser, }); expect(userService.createAnonymousUser).toHaveBeenCalled(); @@ -517,15 +527,80 @@ describe('UserController', () => { const currentUser = await userController.signIn({}); expect(currentUser).toEqual({ - bungie: { - access_token: 'some-access-token', - }, + bungie: bungieToken, ...mockUser, }); expect(userService.updateAnonymousUser).toHaveBeenCalled(); }); }); + describe('when the cross-save owner has changed', () => { + /** + * Bungie reports the owning membership, so moving cross save + * to another platform changes both the membership id and the + * platform. Nothing the old record is keyed on still matches. + */ + const steamUser = { + displayName: 'SteamPersona', + membershipId: 'steam-membership', + membershipType: 3, + profilePicturePath: 'some-profile-picture-path', + }; + const storedXboxUser = { + dateRegistered: Temporal.Now.instant().toString(), + displayName: 'XboxGamertag', + id: 'user-1', + membershipId: 'xbox-membership', + membershipType: 1, + }; + + beforeEach(() => { + destinyService.getCurrentUser.mockResolvedValue(steamUser); + userService.getUserByMembershipId.mockResolvedValue(undefined); + userService.getUserByBungieMembershipId.mockResolvedValue(storedXboxUser); + }); + + it('should find the record by its Bungie membership id', async () => { + await userController.signIn({}); + + expect(userService.getUserByBungieMembershipId).toHaveBeenCalledWith( + 'bungie-net-99', + ); + }); + + it('should move the record rather than update it in place', async () => { + await userController.signIn({}); + + expect(userService.movePlatform).toHaveBeenCalledWith( + storedXboxUser, + expect.objectContaining({ membershipType: 3 }), + ); + expect(userService.updateUser).not.toHaveBeenCalled(); + expect(userService.createAnonymousUser).not.toHaveBeenCalled(); + }); + + it('should return the signed-in user', async () => { + await expect(userController.signIn({})).resolves.toMatchObject({ + displayName: 'SteamPersona', + membershipType: 3, + }); + }); + }); + + describe('when the platform is unchanged', () => { + it('should update in place without moving partitions', async () => { + userService.getUserByMembershipId.mockResolvedValue({ + dateRegistered: Temporal.Now.instant().toString(), + ...mockUser, + }); + + await userController.signIn({}); + + expect(userService.movePlatform).not.toHaveBeenCalled(); + expect(userService.updateUser).toHaveBeenCalled(); + }); + }); + describe('when current user is registered', () => { it('should update the registered user', async () => { userService.getUserByMembershipId.mockImplementation(() => @@ -538,9 +613,7 @@ describe('UserController', () => { const currentUser = await userController.signIn({}); expect(currentUser).toEqual({ - bungie: { - access_token: 'some-access-token', - }, + bungie: bungieToken, ...mockUser, }); expect(userService.updateUser).toHaveBeenCalled(); @@ -921,3 +994,180 @@ describe('UserController', () => { }); }); }); + +/** + * Issue #718 lived in the seam rather than in either module: the membership + * `DestinyService` picks is the one `UserService` validates, and the two + * disagreed about which platforms exist. Both are real here, with only Bungie + * and Cosmos replaced, so a fixture has to survive the whole first sign-in. + */ +describe('UserController.signIn against the real Destiny and User services', () => { + const cacheService = { deleteUser: vi.fn(), getUser: vi.fn(), setUser: vi.fn() }; + const documentService = { + createDocument: vi.fn(), + deleteDocumentById: vi.fn(), + getDocuments: vi.fn(() => []), + }; + + let controller; + + beforeEach(() => { + vi.clearAllMocks(); + + controller = new UserController({ + destinyService: new DestinyService({ cacheService: {} }), + notificationService, + userService: new UserService({ cacheService, client: {}, documentService }), + worldRepository, + }); + }); + + describe.each([ + ['Xbox', 1], + ['PlayStation Network', 2], + ['Steam', 3], + ['Epic Games Store', 6], + ])('when a first time visitor signs in on %s', (_platform, platformMembershipType) => { + it('should persist the anonymous user', async () => { + const bungieDisplayName = chance.string({ length: 12, alpha: true }); + + post.mockResolvedValue({ + access_token: 'some-access-token', + expires_in: 3600, + membership_id: '99', + refresh_token: 'some-refresh-token', + }); + get.mockResolvedValueOnce({ + ErrorCode: 1, + Response: { + bungieNetUser: { profilePicturePath: '/img/profile/avatars/Destiny26.jpg' }, + destinyMemberships: [ + { + crossSaveOverride: 0, + displayName: bungieDisplayName, + membershipId: '4611686018400000000', + membershipType: platformMembershipType, + }, + ], + }, + }); + + await expect(controller.signIn({ code: 'some-code' })).resolves.toMatchObject({ + displayName: bungieDisplayName, + membershipType: platformMembershipType, + }); + + expect(documentService.createDocument).toHaveBeenCalledWith( + 'Users', + expect.objectContaining({ + displayName: bungieDisplayName, + membershipType: platformMembershipType, + }), + ); + }); + }); + + describe('when a registered player moves their cross-save owner to Steam', () => { + /** + * The lookup by platform membership id misses, because that id changed + * with the owner. Only `bungie.membership_id` still matches - and the + * record cannot simply be updated, because `membershipType` is the + * Cosmos partition key. + */ + const storedXboxUser = { + _etag: 'stored-etag', + bungie: { access_token: 'old-token', membership_id: 'bungie-net-99' }, + dateRegistered: '2026-01-01T00:00:00Z', + displayName: 'XboxGamertag', + emailAddress: 'player@destiny-ghost.com', + id: 'user-1', + membershipId: 'xbox-membership', + membershipType: 1, + phoneNumber: '+12085551234', + }; + + beforeEach(() => { + post.mockResolvedValue({ + access_token: 'some-access-token', + expires_in: 3600, + membership_id: 'bungie-net-99', + refresh_token: 'some-refresh-token', + }); + get.mockResolvedValueOnce({ + ErrorCode: 1, + Response: { + destinyMemberships: [ + { + crossSaveOverride: 3, + displayName: 'XboxGamertag', + membershipId: 'xbox-membership', + membershipType: 1, + }, + { + crossSaveOverride: 3, + displayName: 'SteamPersona', + membershipId: 'steam-membership', + membershipType: 3, + }, + ], + }, + }); + documentService.getDocuments.mockImplementation((_collection, query) => + Promise.resolve( + query.query.includes('bungie.membership_id') ? [storedXboxUser] : [], + ), + ); + documentService.createDocument.mockImplementation((_collection, document) => + Promise.resolve(document), + ); + }); + + it('should keep the registration instead of stranding it on the old partition', async () => { + await expect(controller.signIn({ code: 'some-code' })).resolves.toMatchObject({ + displayName: 'SteamPersona', + membershipType: 3, + }); + + expect(documentService.createDocument).toHaveBeenCalledWith( + 'Users', + expect.objectContaining({ + displayName: 'SteamPersona', + membershipId: 'steam-membership', + membershipType: 3, + dateRegistered: '2026-01-01T00:00:00Z', + phoneNumber: '+12085551234', + emailAddress: 'player@destiny-ghost.com', + }), + ); + expect(documentService.deleteDocumentById).toHaveBeenCalledWith('Users', 'user-1', 1); + }); + + it('should not sign them in as a brand new anonymous user', async () => { + await controller.signIn({ code: 'some-code' }); + + const [, document] = documentService.createDocument.mock.calls[0]; + + expect(document.dateRegistered).toBeDefined(); + expect(documentService.createDocument).toHaveBeenCalledTimes(1); + }); + }); + + describe('when the account has no playable membership', () => { + it('should sign nobody in and store nothing', async () => { + post.mockResolvedValue({ + access_token: 'some-access-token', + expires_in: 3600, + membership_id: '99', + refresh_token: 'some-refresh-token', + }); + get.mockResolvedValueOnce({ + ErrorCode: 1, + Response: { destinyMemberships: [] }, + }); + + await expect(controller.signIn({ code: 'some-code' })).resolves.toBeUndefined(); + + expect(documentService.createDocument).not.toHaveBeenCalled(); + }); + }); +}); diff --git a/users/user.service.js b/users/user.service.js index 8b558761..13a37a40 100644 --- a/users/user.service.js +++ b/users/user.service.js @@ -1,5 +1,6 @@ // @ts-check import { z } from 'zod'; +import supportedMembershipTypes from '../helpers/bungie.membershipTypes.js'; import QueryBuilder from '../helpers/queryBuilder.js'; import log from '../helpers/log.js'; import notificationTypes from '../notifications/notification.types.js'; @@ -36,12 +37,30 @@ const storedBungieTokenSchema = bungieTokenSchema.partial().required({ * @private */ const anonymousUserSchema = z.object({ - displayName: z.string().min(3).max(16), + /** + * Bungie supplies this, so the bounds describe the platforms rather than a + * form: a Steam persona name runs to 32 characters, well past the 16 an + * Xbox gamertag or a PSN online ID stops at. + */ + displayName: z.string().min(3).max(32), membershipId: z.string(), - membershipType: z.number().int().min(1).max(2), + membershipType: z.literal(supportedMembershipTypes), profilePicturePath: z.string(), }); +/** + * The identity fields a platform move rewrites. Picked from + * `anonymousUserSchema` rather than restated, so the supported platform list + * keeps one definition - `membershipType` is the Cosmos partition key, and + * `movePlatform` is the only write that chooses a new one. + * @private + */ +const platformMembershipSchema = anonymousUserSchema.pick({ + displayName: true, + membershipId: true, + membershipType: true, +}); + /** * Schema for user notifications. * @private @@ -87,7 +106,7 @@ const userSchema = z.object({ */ consentUpdatedAt: z.number().int().optional(), membershipId: z.string(), - membershipType: z.number().int(), + membershipType: z.literal(supportedMembershipTypes), lastName: z.string(), notifications: z.array(notificationSchema).default([]), patches: z.array(z.object({})).default([]), @@ -646,6 +665,41 @@ class UserService { return user; } + /** + * Get user from their Bungie.net membership id. + * + * The platform `membershipId` changes when a player moves the membership + * that owns their cross-saved data; this one does not, which makes it the + * only way to recognise such an account as a returning user. + * + * Documents written before the full token was persisted carry no + * `bungie.membership_id` and will not match. They pick one up on their next + * ordinary sign-in, so the gap closes itself rather than needing a backfill. + * @param {string} bungieMembershipId + * @returns {Promise | undefined>} + */ + async getUserByBungieMembershipId(bungieMembershipId) { + if (typeof bungieMembershipId !== 'string' || !bungieMembershipId) { + return Promise.reject(new Error('bungieMembershipId string is required')); + } + + const qb = new QueryBuilder(); + const documents = /** @type {import('../helpers/documents.js').CosmosDocument[]} */ ( + await this.documents.getDocuments( + userCollectionId, + qb.where('bungie.membership_id', bungieMembershipId).getQuery(), + ) + ); + + if (documents.length > 1) { + throw new Error( + `more than 1 document found for bungie.membership_id ${bungieMembershipId}`, + ); + } + + return documents[0]; + } + /** * Read only the consent fields for a number, always from Cosmos. * @@ -791,6 +845,83 @@ class UserService { } } + /** + * Move a user to the platform that now owns their cross-saved data. + * + * `membershipType` is the Cosmos partition key, and a partition key is + * fixed for the life of a document: no update changes it, and there is no + * cross-partition transaction to do it atomically. So the record is + * recreated under the new platform and the old copy removed, carrying the + * registration - phone number, notifications, consent - across with it. + * + * Create first, delete second. A failure between the two leaves a + * duplicate, which is visible and recoverable; the other order would leave + * a registered user with no document at all. + * + * Deliberately skips `userSchema`, like `updateUserSubscription`: the + * stored document is already in hand, and a legacy record that fails the + * schema must still be movable. Only the incoming identity is validated, + * because it chooses the new partition. + * @param {import('../helpers/documents.js').CosmosDocument} storedUser + * @param {Record} membership - the membership Bungie now reports + * @returns {Promise>} + */ + async movePlatform(storedUser, membership) { + try { + platformMembershipSchema.parse(membership); + } catch (err) { + if (err instanceof z.ZodError) { + return Promise.reject(Error(JSON.stringify(err.issues))); + } + return Promise.reject(err); + } + + const { _etag, _rid, _self, _attachments, _ts, ...carried } = storedUser; + const moved = { ...carried, ...membership }; + const created = await this.documents.createDocument(userCollectionId, moved); + + try { + await this.documents.deleteDocumentById( + userCollectionId, + storedUser.id, + storedUser.membershipType, + ); + } catch (err) { + /** + * The move itself already succeeded. Rejecting here would report a + * completed sign-in as failed and still leave the duplicate, so the + * only useful thing left to do is say so loudly. + */ + log.error( + { err, userId: storedUser.id, membershipType: storedUser.membershipType }, + 'Moved the user but could not remove the old document; a duplicate remains.', + ); + } + + /** + * Ahead of caching the new document, not after: the phone-number and + * email-address keys are shared by both records and hold a + * displayName+membershipType pointer that no longer resolves. + */ + try { + await this.cacheService.deleteUser(storedUser); + } catch (err) { + log.warn( + { err, userId: storedUser.id }, + 'Failed to clear the cache after moving the user; continuing without it.', + ); + } + + const movedDocument = + /** @type {import('../helpers/documents.js').CosmosDocument} */ ( + created ?? moved + ); + + await this.#cache(movedDocument); + + return movedDocument; + } + /** * Update anonymous user. * @param {AnonymousUser} anonymousUser diff --git a/users/user.service.spec.js b/users/user.service.spec.js index 3677709e..22ef9e6c 100644 --- a/users/user.service.spec.js +++ b/users/user.service.spec.js @@ -7,6 +7,7 @@ import UserService from './user.service.js'; import log from '../helpers/log.js'; const cacheService = { + deleteUser: vi.fn(), getUser: vi.fn(), setUser: vi.fn(), }; @@ -115,6 +116,60 @@ describe('UserService', () => { }); }); + describe.each([ + ['Xbox', 1], + ['PlayStation Network', 2], + ['Steam', 3], + ['Epic Games Store', 6], + ])('when the anonymous user is on %s', (_platform, membershipType) => { + it('should create the anonymous user', async () => { + userService.getUserByDisplayName = vi.fn().mockResolvedValue(); + + await userService.createAnonymousUser({ ...anonymousUser, membershipType }); + + expect(documentService.createDocument).toHaveBeenCalledWith( + 'Users', + expect.objectContaining({ membershipType }), + ); + }); + }); + + describe.each([ + ['None', 0], + ['Blizzard, migrated to Steam in 2019', 4], + ['Stadia, retired in 2023', 5], + ['BungieNext, not a playable platform', 254], + ])('when the membership type is %s', (_platform, membershipType) => { + it('should reject without creating a partial user', async () => { + userService.getUserByDisplayName = vi.fn().mockResolvedValue(); + + await expect( + userService.createAnonymousUser({ ...anonymousUser, membershipType }), + ).rejects.toThrow(); + + expect(documentService.createDocument).not.toHaveBeenCalled(); + }); + }); + + describe('when the display name is a 32 character Steam persona name', () => { + it('should create the anonymous user', async () => { + userService.getUserByDisplayName = vi.fn().mockResolvedValue(); + + const displayName = chance.string({ length: 32, alpha: true }); + + await userService.createAnonymousUser({ + ...anonymousUser, + displayName, + membershipType: 3, + }); + + expect(documentService.createDocument).toHaveBeenCalledWith( + 'Users', + expect.objectContaining({ displayName }), + ); + }); + }); + describe('when anonymous user is valid', () => { describe('when the anonymous user exists', () => { it('should reject the anonymous user', async () => { @@ -398,6 +453,161 @@ describe('UserService', () => { }); }); + describe('getUserByBungieMembershipId', () => { + it('should return the user whose stored token carries that id', async () => { + documentService.getDocuments.mockResolvedValue([user]); + + await expect(userService.getUserByBungieMembershipId('99')).resolves.toEqual(user); + }); + + it('should query on the Bungie membership id, not the platform one', async () => { + documentService.getDocuments.mockResolvedValue([user]); + + await userService.getUserByBungieMembershipId('99'); + + const [, query] = documentService.getDocuments.mock.calls[0]; + + expect(query.query).toContain('bungie.membership_id'); + expect(query.parameters).toEqual([{ name: '@membership_id', value: '99' }]); + }); + + it('should return undefined when nothing matches', async () => { + documentService.getDocuments.mockResolvedValue([]); + + await expect(userService.getUserByBungieMembershipId('99')).resolves.toBeUndefined(); + }); + + it('should fail when more than one document matches', async () => { + documentService.getDocuments.mockResolvedValue([user, user]); + + await expect(userService.getUserByBungieMembershipId('99')).rejects.toThrow(); + }); + + it('should reject an empty id without querying', async () => { + await expect(userService.getUserByBungieMembershipId('')).rejects.toThrow(); + + expect(documentService.getDocuments).not.toHaveBeenCalled(); + }); + }); + + describe('movePlatform', () => { + const storedUser = { + ...user, + _etag: 'some-etag', + _rid: 'some-rid', + _self: 'some-self', + _attachments: 'attachments/', + _ts: 1700000000, + dateRegistered: '2026-01-01T00:00:00Z', + id: 'user-1', + membershipId: '11', + membershipType: 1, + }; + const steamMembership = { + displayName: 'SteamPersona', + membershipId: '4611686018400000000', + membershipType: 3, + profilePicturePath: '/thing1', + }; + + beforeEach(() => { + documentService.createDocument.mockImplementation(document => + Promise.resolve(document), + ); + documentService.createDocument.mockImplementation((_collection, document) => + Promise.resolve({ ...document, _etag: 'new-etag' }), + ); + documentService.deleteDocumentById.mockResolvedValue(); + cacheService.deleteUser.mockResolvedValue(); + cacheService.setUser.mockResolvedValue(); + }); + + it('should create the record under the new platform, carrying the registration', async () => { + await userService.movePlatform(storedUser, steamMembership); + + expect(documentService.createDocument).toHaveBeenCalledWith( + 'Users', + expect.objectContaining({ + displayName: 'SteamPersona', + membershipType: 3, + membershipId: '4611686018400000000', + // the whole point of the move: registration survives it + dateRegistered: '2026-01-01T00:00:00Z', + phoneNumber: storedUser.phoneNumber, + emailAddress: storedUser.emailAddress, + id: 'user-1', + }), + ); + }); + + it('should not carry the old Cosmos system properties onto the new record', async () => { + await userService.movePlatform(storedUser, steamMembership); + + const [, document] = documentService.createDocument.mock.calls[0]; + + expect(document).not.toHaveProperty('_etag'); + expect(document).not.toHaveProperty('_rid'); + expect(document).not.toHaveProperty('_self'); + expect(document).not.toHaveProperty('_attachments'); + expect(document).not.toHaveProperty('_ts'); + }); + + it('should delete the old record from its own partition', async () => { + await userService.movePlatform(storedUser, steamMembership); + + expect(documentService.deleteDocumentById).toHaveBeenCalledWith('Users', 'user-1', 1); + }); + + it('should create before deleting, so a failure leaves a duplicate and not a gap', async () => { + const { promise: createPending, resolve: finishCreate } = Promise.withResolvers(); + + documentService.createDocument.mockReturnValue(createPending); + + const moving = userService.movePlatform(storedUser, steamMembership); + + await Promise.resolve(); + + expect(documentService.deleteDocumentById).not.toHaveBeenCalled(); + + finishCreate({ ...storedUser, ...steamMembership }); + await moving; + + expect(documentService.deleteDocumentById).toHaveBeenCalled(); + }); + + it('should still resolve when the old record cannot be removed', async () => { + const errorLog = vi.spyOn(log, 'error').mockImplementation(() => {}); + + documentService.deleteDocumentById.mockRejectedValue(new Error('cosmos unavailable')); + + await expect( + userService.movePlatform(storedUser, steamMembership), + ).resolves.toBeDefined(); + + expect(errorLog).toHaveBeenCalled(); + + errorLog.mockRestore(); + }); + + it('should clear the old cache keys before caching the moved record', async () => { + await userService.movePlatform(storedUser, steamMembership); + + expect(cacheService.deleteUser).toHaveBeenCalledWith(storedUser); + expect(cacheService.deleteUser.mock.invocationCallOrder[0]).toBeLessThan( + cacheService.setUser.mock.invocationCallOrder[0], + ); + }); + + it('should reject an unsupported platform without writing anything', async () => { + await expect( + userService.movePlatform(storedUser, { ...steamMembership, membershipType: 5 }), + ).rejects.toThrow(); + + expect(documentService.createDocument).not.toHaveBeenCalled(); + expect(documentService.deleteDocumentById).not.toHaveBeenCalled(); + }); + }); + describe('getUserByMembershipId', () => { describe('when membership Id is defined', () => { it('should return an existing user', () => { From a3d683037188d43056761b84c9b4fba4ffe5fd2a Mon Sep 17 00:00:00 2001 From: Chris Paskvan Date: Sat, 26 Sep 2026 20:03:22 -0500 Subject: [PATCH 2/4] fix(users): keep reads correct while a platform move is unfinished Review feedback on #749. A failed delete in movePlatform left two documents sharing an id, and every cross-partition lookup threw `more than 1 document found` on them. That took out inbound STOP handling (twilio.controller.js), the consent gate, SMS sign-in and the notification path - for as long as the stale copy survived, which nothing bounded. "Visible and recoverable" understated it. Reduce a multi-document result through #oneDocument: a shared id is a failed move, so read the newest _ts and log loudly. Two documents with different ids are a real duplicate and still throw. getConsentByPhoneNumber projects id and _ts so the same rule reaches the gate that decides whether an SMS goes out. Retry the delete itself, since correct-despite-a-duplicate is a fallback rather than the resting state. A 404 means it is already gone. Treat a conflicting create as a move a concurrent sign-in already made and carry on to the delete, instead of 500ing the second of two OAuth callbacks. getUserByBungieMembershipId resolves undefined for a missing id rather than rejecting: it is a fallback, and rejecting turned a brand-new user whose token lacked the field into a failed sign-in. The existing "more than one document" fixtures returned the same object twice, which Cosmos cannot do - ids are unique per partition. They now use genuinely distinct documents. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_018iD3V7Zyugg3atXVsogTBf --- users/user.service.js | 179 +++++++++++++++++++++++++++---------- users/user.service.spec.js | 169 +++++++++++++++++++++++++++++++--- 2 files changed, 292 insertions(+), 56 deletions(-) diff --git a/users/user.service.js b/users/user.service.js index 13a37a40..b11b619c 100644 --- a/users/user.service.js +++ b/users/user.service.js @@ -1,8 +1,10 @@ // @ts-check +import { StatusCodes } from 'http-status-codes'; import { z } from 'zod'; import supportedMembershipTypes from '../helpers/bungie.membershipTypes.js'; import QueryBuilder from '../helpers/queryBuilder.js'; import log from '../helpers/log.js'; +import { withRetry } from '../helpers/retry.js'; import notificationTypes from '../notifications/notification.types.js'; /** @@ -116,6 +118,16 @@ const userSchema = z.object({ bungie: storedBungieTokenSchema.optional(), }); +/** + * The HTTP status Cosmos attached to a rejection. The driver puts it on `code` + * and, in places, on `statusCode`; read both rather than depend on which. + * @param {*} err + * @returns {number | undefined} + */ +function cosmosStatus(err) { + return typeof err?.code === 'number' ? err.code : err?.statusCode; +} + /** * Copy the consent watermark from the stored document onto the merged one, * discarding whatever the caller supplied. @@ -571,12 +583,9 @@ class UserService { ) ); if (documents.length) { - if (documents.length > 1) { - throw new Error(`more than 1 document found for emailAddress ${emailAddress}`); - } - await this.#cache(documents[0]); + user = UserService.#oneDocument(documents, `emailAddress ${emailAddress}`); - [user] = documents; + await this.#cache(/** @type {*} */ (user)); } return user; @@ -625,11 +634,7 @@ class UserService { await this.documents.getDocuments(userCollectionId, qb.where('id', userId).getQuery()) ); if (documents) { - if (documents.length > 1) { - throw new Error(`more than 1 document found for userId ${userId}`); - } - - [user] = documents; + user = UserService.#oneDocument(documents, `userId ${userId}`); } return user; @@ -655,11 +660,7 @@ class UserService { ) ); if (documents) { - if (documents.length > 1) { - throw new Error(`more than 1 document found for membershipId ${membershipId}`); - } - - [user] = documents; + user = UserService.#oneDocument(documents, `membershipId ${membershipId}`); } return user; @@ -679,8 +680,15 @@ class UserService { * @returns {Promise | undefined>} */ async getUserByBungieMembershipId(bungieMembershipId) { + /** + * Resolves undefined rather than rejecting, unlike its siblings. This + * is a fallback behind `getUserByMembershipId`, so a missing id means + * "nothing more to try" - and rejecting would turn a brand-new user + * whose token lacked the field into a failed sign-in, where before + * this lookup existed they were simply created. + */ if (typeof bungieMembershipId !== 'string' || !bungieMembershipId) { - return Promise.reject(new Error('bungieMembershipId string is required')); + return undefined; } const qb = new QueryBuilder(); @@ -691,13 +699,9 @@ class UserService { ) ); - if (documents.length > 1) { - throw new Error( - `more than 1 document found for bungie.membership_id ${bungieMembershipId}`, - ); - } - - return documents[0]; + return /** @type {*} */ ( + UserService.#oneDocument(documents, `bungie.membership_id ${bungieMembershipId}`) + ); } /** @@ -726,6 +730,8 @@ class UserService { await this.documents.getDocuments( userCollectionId, qb + .select('id') + .select('_ts') .select('isSubscribed') .select('notifications') .select('consentUpdatedAt') @@ -734,11 +740,7 @@ class UserService { ) ); - if (documents.length > 1) { - throw new Error(`more than 1 document found for phoneNumber ${phoneNumber}`); - } - - return documents[0]; + return /** @type {*} */ (UserService.#oneDocument(documents, `phoneNumber ${phoneNumber}`)); } /** @@ -780,12 +782,9 @@ class UserService { ) ); if (documents.length) { - if (documents.length > 1) { - throw new Error(`more than 1 document found for phoneNumber ${phoneNumber}`); - } - await this.#cache(documents[0]); + user = UserService.#oneDocument(documents, `phoneNumber ${phoneNumber}`); - [user] = documents; + await this.#cache(/** @type {*} */ (user)); } return user; @@ -878,26 +877,66 @@ class UserService { const { _etag, _rid, _self, _attachments, _ts, ...carried } = storedUser; const moved = { ...carried, ...membership }; - const created = await this.documents.createDocument(userCollectionId, moved); + /** @type {*} */ + let created; try { - await this.documents.deleteDocumentById( - userCollectionId, - storedUser.id, - storedUser.membershipType, - ); + created = await this.documents.createDocument(userCollectionId, moved); } catch (err) { /** - * The move itself already succeeded. Rejecting here would report a - * completed sign-in as failed and still leave the duplicate, so the - * only useful thing left to do is say so loudly. + * Two OAuth callbacks arriving together - a double-click, a retry - + * both miss the lookup and both move. The second create conflicts + * on the id it is recreating, which means the first one already + * did this; carry on to the delete so the old copy still goes. */ - log.error( - { err, userId: storedUser.id, membershipType: storedUser.membershipType }, - 'Moved the user but could not remove the old document; a duplicate remains.', + if (cosmosStatus(err) !== StatusCodes.CONFLICT) { + throw err; + } + + log.info( + { userId: storedUser.id }, + 'The platform move was already completed by a concurrent sign-in.', ); } + try { + /** + * Retried, because the alternative is a stale copy that nothing + * bounded: until it goes, every lookup sharing its phone number or + * id sees two documents. `#oneDocument` keeps those reads correct + * meanwhile, but correct-despite-a-duplicate is a fallback, not the + * resting state. A 404 means it is already gone. + */ + await withRetry( + () => + this.documents.deleteDocumentById( + userCollectionId, + storedUser.id, + storedUser.membershipType, + ), + { + maxRetries: 3, + shouldRetry: (/** @type {*} */ err) => + cosmosStatus(err) !== StatusCodes.NOT_FOUND, + }, + ); + } catch (err) { + if (cosmosStatus(err) !== StatusCodes.NOT_FOUND) { + /** + * The move itself already succeeded. Rejecting here would + * report a completed sign-in as failed and still leave the + * stale copy, so the only useful thing left is to say so + * loudly - reads stay correct through `#oneDocument`, and the + * next sign-in moves nothing because the record is already on + * the right platform, so this one needs removing by hand. + */ + log.error( + { err, userId: storedUser.id, membershipType: storedUser.membershipType }, + 'Moved the user but could not remove the old document; a stale copy remains.', + ); + } + } + /** * Ahead of caching the new document, not after: the phone-number and * email-address keys are shared by both records and hold a @@ -922,6 +961,56 @@ class UserService { return movedDocument; } + /** + * Reduce a multi-document result to the one the caller meant, or throw. + * + * `movePlatform` recreates a record under a new partition with the same + * `id` before removing the old copy, so a failure between the two leaves + * two documents that share an id and differ in `membershipType`. Every + * cross-partition lookup then sees both, and the throw below used to take + * out the phone-number lookup that inbound STOP handling runs on + * (`twilio.controller.js`), the consent gate, and SMS sign-in - for the + * whole time the stale copy survived, which nothing bounded. + * + * A shared `id` is the signal: two genuinely distinct users never have + * one. The newest `_ts` is the record the move intended to leave behind. + * Anything else is a real duplicate and still throws, because that is a + * different bug and silently picking a winner would hide it. + * + * The stale copy is left in place: a getter is the wrong place to delete + * from, and `movePlatform` retries its own delete. This keeps reads + * correct meanwhile. + * @param {import('../helpers/documents.js').CosmosDocument<*>[]} documents + * @param {string} description - what was looked up, for the error message + * @returns {import('../helpers/documents.js').CosmosDocument<*> | undefined} + */ + static #oneDocument(documents, description) { + if (documents.length <= 1) { + return documents[0]; + } + + const [first] = documents; + const sharesAnId = first.id && documents.every(({ id }) => id === first.id); + + if (!sharesAnId) { + throw new Error(`more than 1 document found for ${description}`); + } + + const newest = documents.reduce((winner, document) => + (document._ts ?? 0) > (winner._ts ?? 0) ? document : winner, + ); + + log.error( + { + userId: newest.id, + membershipTypes: documents.map(({ membershipType }) => membershipType), + }, + 'An unfinished platform move left a stale copy; reading the newest until it is removed.', + ); + + return newest; + } + /** * Update anonymous user. * @param {AnonymousUser} anonymousUser diff --git a/users/user.service.spec.js b/users/user.service.spec.js index 22ef9e6c..dc1d821b 100644 --- a/users/user.service.spec.js +++ b/users/user.service.spec.js @@ -306,7 +306,10 @@ describe('UserService', () => { it('should fail when more than one existing user is found', async () => { documentService.getDocuments.mockImplementation(() => - Promise.resolve([user, user]), + Promise.resolve([ + { ...user, id: 'user-a' }, + { ...user, id: 'user-b' }, + ]), ); await expect( @@ -362,7 +365,10 @@ describe('UserService', () => { it('should fail when more than one existing user is found', async () => { documentService.getDocuments.mockImplementation(() => - Promise.resolve([user, user]), + Promise.resolve([ + { ...user, id: 'user-a' }, + { ...user, id: 'user-b' }, + ]), ); await expect( @@ -414,7 +420,10 @@ describe('UserService', () => { it('should fail when more than one existing user is found', async () => { documentService.getDocuments.mockImplementation(() => - Promise.resolve([user, user]), + Promise.resolve([ + { ...user, id: 'user-a' }, + { ...user, id: 'user-b' }, + ]), ); await expect( @@ -453,6 +462,61 @@ describe('UserService', () => { }); }); + /** + * What every other lookup does while a `movePlatform` delete has not landed. + * The two documents share an id and differ only in partition, and before + * this they took out inbound STOP handling, the consent gate and SMS + * sign-in for as long as the stale copy survived. + */ + describe('when an unfinished platform move has left a stale copy', () => { + const stale = { ...user, _ts: 1000, id: 'user-1', membershipType: 1 }; + const moved = { ...user, _ts: 2000, id: 'user-1', membershipType: 3 }; + + beforeEach(() => { + // Cosmos returns them in no particular order; the oldest is first here. + documentService.getDocuments.mockResolvedValue([stale, moved]); + cacheService.setUser.mockResolvedValue(); + }); + + it('should keep the phone-number lookup working, which STOP handling runs on', async () => { + await expect(userService.getUserByPhoneNumber(user.phoneNumber, true)).resolves.toEqual( + moved, + ); + }); + + it('should keep the consent gate working rather than suppressing notifications', async () => { + await expect(userService.getConsentByPhoneNumber(user.phoneNumber)).resolves.toEqual( + moved, + ); + }); + + it('should keep the id and Bungie-id lookups working', async () => { + await expect(userService.getUserById('user-1')).resolves.toEqual(moved); + await expect(userService.getUserByBungieMembershipId('99')).resolves.toEqual(moved); + }); + + it('should say so loudly, because the stale copy still needs removing', async () => { + const errorLog = vi.spyOn(log, 'error').mockImplementation(() => {}); + + await userService.getUserById('user-1'); + + expect(errorLog).toHaveBeenCalled(); + + errorLog.mockRestore(); + }); + + it('should still throw when the documents are two different users', async () => { + documentService.getDocuments.mockResolvedValue([ + { ...user, id: 'user-a' }, + { ...user, id: 'user-b' }, + ]); + + await expect(userService.getUserById('user-a')).rejects.toThrow( + 'more than 1 document found', + ); + }); + }); + describe('getUserByBungieMembershipId', () => { it('should return the user whose stored token carries that id', async () => { documentService.getDocuments.mockResolvedValue([user]); @@ -477,14 +541,26 @@ describe('UserService', () => { await expect(userService.getUserByBungieMembershipId('99')).resolves.toBeUndefined(); }); - it('should fail when more than one document matches', async () => { - documentService.getDocuments.mockResolvedValue([user, user]); + it('should fail when two genuinely different users match', async () => { + documentService.getDocuments.mockResolvedValue([ + { ...user, id: 'user-a' }, + { ...user, id: 'user-b' }, + ]); await expect(userService.getUserByBungieMembershipId('99')).rejects.toThrow(); }); - it('should reject an empty id without querying', async () => { - await expect(userService.getUserByBungieMembershipId('')).rejects.toThrow(); + /** + * Unlike its siblings, which reject. This is a fallback behind + * `getUserByMembershipId`, so a missing id means "nothing more to try": + * rejecting would turn a brand-new user whose token lacked the field + * into a failed sign-in. + */ + it('should resolve undefined for a missing id without querying', async () => { + await expect(userService.getUserByBungieMembershipId('')).resolves.toBeUndefined(); + await expect( + userService.getUserByBungieMembershipId(undefined), + ).resolves.toBeUndefined(); expect(documentService.getDocuments).not.toHaveBeenCalled(); }); @@ -598,6 +674,68 @@ describe('UserService', () => { ); }); + it('should retry a failed delete rather than leave the stale copy', async () => { + vi.useFakeTimers(); + + const errorLog = vi.spyOn(log, 'error').mockImplementation(() => {}); + + documentService.deleteDocumentById + .mockRejectedValueOnce(Object.assign(new Error('throttled'), { code: 429 })) + .mockResolvedValueOnce(); + + const moving = userService.movePlatform(storedUser, steamMembership); + + await vi.runAllTimersAsync(); + await moving; + + expect(documentService.deleteDocumentById).toHaveBeenCalledTimes(2); + expect(errorLog).not.toHaveBeenCalled(); + + errorLog.mockRestore(); + vi.useRealTimers(); + }); + + it('should not retry a delete that 404s, because the copy is already gone', async () => { + const errorLog = vi.spyOn(log, 'error').mockImplementation(() => {}); + + documentService.deleteDocumentById.mockRejectedValue( + Object.assign(new Error('not found'), { code: 404 }), + ); + + await expect( + userService.movePlatform(storedUser, steamMembership), + ).resolves.toBeDefined(); + + expect(documentService.deleteDocumentById).toHaveBeenCalledTimes(1); + expect(errorLog).not.toHaveBeenCalled(); + + errorLog.mockRestore(); + }); + + it('should treat a conflicting create as a concurrent move and still delete', async () => { + documentService.createDocument.mockRejectedValue( + Object.assign(new Error('conflict'), { code: 409 }), + ); + + await expect( + userService.movePlatform(storedUser, steamMembership), + ).resolves.toMatchObject({ membershipType: 3 }); + + expect(documentService.deleteDocumentById).toHaveBeenCalledWith('Users', 'user-1', 1); + }); + + it('should surface a create failure that is not a conflict', async () => { + documentService.createDocument.mockRejectedValue( + Object.assign(new Error('boom'), { code: 500 }), + ); + + await expect(userService.movePlatform(storedUser, steamMembership)).rejects.toThrow( + 'boom', + ); + + expect(documentService.deleteDocumentById).not.toHaveBeenCalled(); + }); + it('should reject an unsupported platform without writing anything', async () => { await expect( userService.movePlatform(storedUser, { ...steamMembership, membershipType: 5 }), @@ -621,7 +759,10 @@ describe('UserService', () => { it('should fail when more than one existing user is found', async () => { documentService.getDocuments.mockImplementation(() => - Promise.resolve([user, user]), + Promise.resolve([ + { ...user, id: 'user-a' }, + { ...user, id: 'user-b' }, + ]), ); await expect( @@ -671,7 +812,7 @@ describe('UserService', () => { expect(collectionId).toBe('Users'); expect(query.query).toBe( - 'SELECT r.isSubscribed, r.notifications, r.consentUpdatedAt FROM root r WHERE r.phoneNumber = @phoneNumber', + 'SELECT r.id, r._ts, r.isSubscribed, r.notifications, r.consentUpdatedAt FROM root r WHERE r.phoneNumber = @phoneNumber', ); expect(query.parameters).toEqual([{ name: '@phoneNumber', value: user.phoneNumber }]); }); @@ -765,7 +906,10 @@ describe('UserService', () => { it('should fail when more than one existing user is found', async () => { documentService.getDocuments.mockImplementation(() => - Promise.resolve([user, user]), + Promise.resolve([ + { ...user, id: 'user-a' }, + { ...user, id: 'user-b' }, + ]), ); await expect(userService.getUserByPhoneNumber(user.phoneNumber)).rejects.toThrow(); @@ -847,7 +991,10 @@ describe('UserService', () => { it('should fail when more than one existing user is found', async () => { documentService.getDocuments.mockImplementation(() => - Promise.resolve([user, user]), + Promise.resolve([ + { ...user, id: 'user-a' }, + { ...user, id: 'user-b' }, + ]), ); await expect(userService.getUserById(user.id)).rejects.toThrow(); From 27a98e2b105ad25d54e402b5c88950c1825a7b93 Mon Sep 17 00:00:00 2001 From: Chris Paskvan Date: Sat, 26 Sep 2026 20:14:44 -0500 Subject: [PATCH 3/4] fix(users): mark a superseded record rather than ranking by timestamp Review feedback on #749. The previous commit resolved a duplicate by taking the newest _ts, which assumed nothing writes to the superseded copy. Two things do: a session predating the move still carries the old displayName and membershipType, and user.routes.js writes through both; and a player moving cross save back lands on the old copy by its platform membershipId. Either makes the stale copy the newest, so every lookup starts returning the consent it never received - a STOP undone, and texts going out again. movePlatform now marks the old copy with movedTo before creating its replacement. That write is on the old copy's own partition, so a failure stops the move with nothing duplicated. Every lookup filters marked documents out, which is a fact about the record rather than a race with whatever wrote last. getUserByBungieMembershipId is the exception and still sees them, because that is the path signIn recovers through: a move whose successor was never created would otherwise leave the player invisible. signIn clears the mark when the platform turns out to be unchanged. A conflicting create now replaces the target instead of skipping it, which covers a player moving back to a platform whose superseded copy is still present as well as two concurrent callbacks. Retry the delete only on statuses that can clear on their own (408, 429, 449, 5xx) with a 100ms base, rather than spending eight seconds of an OAuth callback on a 403. The delete-failure specs use fake timers, taking the suite from 8.7s back to 4.6s. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_018iD3V7Zyugg3atXVsogTBf --- users/user.controller.js | 19 ++++ users/user.controller.spec.js | 19 ++++ users/user.service.js | 208 ++++++++++++++++++++-------------- users/user.service.spec.js | 166 ++++++++++++++++++++++----- 4 files changed, 302 insertions(+), 110 deletions(-) diff --git a/users/user.controller.js b/users/user.controller.js index 1998387b..1f26225e 100644 --- a/users/user.controller.js +++ b/users/user.controller.js @@ -536,6 +536,25 @@ class UserController { .then(() => user); } + /** + * A record marked for a move whose successor was never created. The + * player is signing in on the platform they were already on, so the + * move is moot and the mark has to come off - while it is there, every + * lookup but the Bungie-id one skips this record. + */ + if ( + destinyGhostUser.movedTo !== undefined && + destinyGhostUser.membershipType === user.membershipType + ) { + await this.users.clearPlatformMove( + /** @type {import('../helpers/documents.js').CosmosDocument} */ ( + /** @type {unknown} */ (destinyGhostUser) + ), + ); + + delete destinyGhostUser.movedTo; + } + /** * `membershipType` is the partition key, so a changed platform is a * move rather than an update - `updateUser` would look the document up diff --git a/users/user.controller.spec.js b/users/user.controller.spec.js index 0adbda3d..f46c8836 100644 --- a/users/user.controller.spec.js +++ b/users/user.controller.spec.js @@ -43,6 +43,7 @@ const userService = { getUserByBungieMembershipId: vi.fn(), getUserByMembershipId: vi.fn(), getUserByPhoneNumber: vi.fn(), + clearPlatformMove: vi.fn().mockResolvedValue(), movePlatform: vi.fn().mockImplementation(user => Promise.resolve(user)), updateAnonymousUser: vi.fn().mockImplementation(user => Promise.resolve(user)), updateUser: vi.fn().mockImplementation(user => Promise.resolve(user)), @@ -587,6 +588,23 @@ describe('UserController', () => { }); }); + describe('when a previous move was marked but never completed', () => { + it('should restore the record instead of leaving it invisible', async () => { + userService.getUserByMembershipId.mockResolvedValue(undefined); + userService.getUserByBungieMembershipId.mockResolvedValue({ + dateRegistered: Temporal.Now.instant().toString(), + ...mockUser, + movedTo: 3, + }); + + await userController.signIn({}); + + expect(userService.clearPlatformMove).toHaveBeenCalled(); + expect(userService.movePlatform).not.toHaveBeenCalled(); + expect(userService.updateUser).toHaveBeenCalled(); + }); + }); + describe('when the platform is unchanged', () => { it('should update in place without moving partitions', async () => { userService.getUserByMembershipId.mockResolvedValue({ @@ -1007,6 +1025,7 @@ describe('UserController.signIn against the real Destiny and User services', () createDocument: vi.fn(), deleteDocumentById: vi.fn(), getDocuments: vi.fn(() => []), + updateDocument: vi.fn((_collection, document) => Promise.resolve(document)), }; let controller; diff --git a/users/user.service.js b/users/user.service.js index b11b619c..3c76df90 100644 --- a/users/user.service.js +++ b/users/user.service.js @@ -115,6 +115,12 @@ const userSchema = z.object({ phoneNumber: z.string(), roles: z.array(z.string()).default(['User']), type: z.string().optional(), + /** + * Set by `movePlatform` on the copy it is superseding, naming the platform + * the record moved to. Its presence, not its value, is what matters: a + * marked document is not the live record and no lookup returns it. + */ + movedTo: z.number().int().optional(), bungie: storedBungieTokenSchema.optional(), }); @@ -128,6 +134,23 @@ function cosmosStatus(err) { return typeof err?.code === 'number' ? err.code : err?.statusCode; } +/** + * Cosmos statuses worth a second attempt: request timeout, throttling, retry- + * with, and anything the service blames on itself. A 400, 401 or 403 will + * never succeed on a retry, and this runs inside the OAuth callback. + * @param {*} err + * @returns {boolean} + */ +function isTransientCosmosError(err) { + const status = cosmosStatus(err); + + if (typeof status !== 'number') { + return false; + } + + return status === 408 || status === 429 || status === 449 || status >= 500; +} + /** * Copy the consent watermark from the stored document onto the merged one, * discarding whatever the caller supplied. @@ -542,15 +565,14 @@ class UserService { ); if (documents.length) { - if (documents.length > 1) { - throw new Error( - `more than 1 document found for displayName ${displayName} and membershipType ${membershipType}`, - ); - } - - await this.#cache(documents[0]); + user = UserService.#oneLiveDocument( + documents, + `displayName ${displayName} and membershipType ${membershipType}`, + ); - [user] = documents; + if (user) { + await this.#cache(user); + } } return user; @@ -583,9 +605,11 @@ class UserService { ) ); if (documents.length) { - user = UserService.#oneDocument(documents, `emailAddress ${emailAddress}`); + user = UserService.#oneLiveDocument(documents, `emailAddress ${emailAddress}`); - await this.#cache(/** @type {*} */ (user)); + if (user) { + await this.#cache(user); + } } return user; @@ -634,7 +658,7 @@ class UserService { await this.documents.getDocuments(userCollectionId, qb.where('id', userId).getQuery()) ); if (documents) { - user = UserService.#oneDocument(documents, `userId ${userId}`); + user = UserService.#oneLiveDocument(documents, `userId ${userId}`); } return user; @@ -660,7 +684,7 @@ class UserService { ) ); if (documents) { - user = UserService.#oneDocument(documents, `membershipId ${membershipId}`); + user = UserService.#oneLiveDocument(documents, `membershipId ${membershipId}`); } return user; @@ -699,9 +723,19 @@ class UserService { ) ); - return /** @type {*} */ ( - UserService.#oneDocument(documents, `bungie.membership_id ${bungieMembershipId}`) - ); + /** + * Deliberately not filtered on the mark, unlike every other lookup. + * This is the path `signIn` recovers through, so it has to be able to + * see a copy whose move never finished - that is what lets the next + * sign-in complete it instead of leaving the player invisible. + */ + if (documents.length > 1) { + throw new Error( + `more than 1 document found for bungie.membership_id ${bungieMembershipId}`, + ); + } + + return documents[0]; } /** @@ -730,8 +764,7 @@ class UserService { await this.documents.getDocuments( userCollectionId, qb - .select('id') - .select('_ts') + .select('movedTo') .select('isSubscribed') .select('notifications') .select('consentUpdatedAt') @@ -740,7 +773,9 @@ class UserService { ) ); - return /** @type {*} */ (UserService.#oneDocument(documents, `phoneNumber ${phoneNumber}`)); + return /** @type {*} */ ( + UserService.#oneLiveDocument(documents, `phoneNumber ${phoneNumber}`) + ); } /** @@ -782,9 +817,11 @@ class UserService { ) ); if (documents.length) { - user = UserService.#oneDocument(documents, `phoneNumber ${phoneNumber}`); + user = UserService.#oneLiveDocument(documents, `phoneNumber ${phoneNumber}`); - await this.#cache(/** @type {*} */ (user)); + if (user) { + await this.#cache(user); + } } return user; @@ -875,7 +912,21 @@ class UserService { return Promise.reject(err); } - const { _etag, _rid, _self, _attachments, _ts, ...carried } = storedUser; + const { membershipType } = /** @type {{ membershipType: number }} */ (membership); + + /** + * Mark the old copy before anything is duplicated. The write is on its + * own partition, so a failure here stops the move with nothing lost - + * and once it lands, no lookup will return that copy again however many + * times it is written to afterwards. + */ + await this.documents.updateDocument( + userCollectionId, + { ...storedUser, movedTo: membershipType }, + storedUser.membershipType, + ); + + const { _etag, _rid, _self, _attachments, _ts, movedTo, ...carried } = storedUser; const moved = { ...carried, ...membership }; /** @type {*} */ let created; @@ -883,29 +934,35 @@ class UserService { try { created = await this.documents.createDocument(userCollectionId, moved); } catch (err) { - /** - * Two OAuth callbacks arriving together - a double-click, a retry - - * both miss the lookup and both move. The second create conflicts - * on the id it is recreating, which means the first one already - * did this; carry on to the delete so the old copy still goes. - */ if (cosmosStatus(err) !== StatusCodes.CONFLICT) { throw err; } + /** + * Something already holds this id on the target partition: a + * concurrent sign-in that moved first, or a superseded copy from a + * platform the player is moving back to. Replacing suits both - + * `moved` is the current record, and it carries no mark, so the + * target comes back to life. + */ log.info( - { userId: storedUser.id }, - 'The platform move was already completed by a concurrent sign-in.', + { userId: storedUser.id, membershipType }, + 'The target already holds this record; replacing it.', + ); + + created = await this.documents.updateDocument( + userCollectionId, + /** @type {*} */ (moved), + membershipType, ); } try { /** - * Retried, because the alternative is a stale copy that nothing - * bounded: until it goes, every lookup sharing its phone number or - * id sees two documents. `#oneDocument` keeps those reads correct - * meanwhile, but correct-despite-a-duplicate is a fallback, not the - * resting state. A 404 means it is already gone. + * Retried on the statuses that can clear on their own. The old copy + * is marked either way, so a leftover is inert rather than + * dangerous - this is about tidiness, which is why it does not get + * to spend eight seconds of an OAuth callback failing. */ await withRetry( () => @@ -914,25 +971,13 @@ class UserService { storedUser.id, storedUser.membershipType, ), - { - maxRetries: 3, - shouldRetry: (/** @type {*} */ err) => - cosmosStatus(err) !== StatusCodes.NOT_FOUND, - }, + { baseDelay: 100, maxRetries: 3, shouldRetry: isTransientCosmosError }, ); } catch (err) { if (cosmosStatus(err) !== StatusCodes.NOT_FOUND) { - /** - * The move itself already succeeded. Rejecting here would - * report a completed sign-in as failed and still leave the - * stale copy, so the only useful thing left is to say so - * loudly - reads stay correct through `#oneDocument`, and the - * next sign-in moves nothing because the record is already on - * the right platform, so this one needs removing by hand. - */ log.error( { err, userId: storedUser.id, membershipType: storedUser.membershipType }, - 'Moved the user but could not remove the old document; a stale copy remains.', + 'Moved the user but could not remove the superseded document.', ); } } @@ -962,53 +1007,52 @@ class UserService { } /** - * Reduce a multi-document result to the one the caller meant, or throw. + * Reduce a lookup result to the one live document, or throw. * - * `movePlatform` recreates a record under a new partition with the same - * `id` before removing the old copy, so a failure between the two leaves - * two documents that share an id and differ in `membershipType`. Every - * cross-partition lookup then sees both, and the throw below used to take - * out the phone-number lookup that inbound STOP handling runs on - * (`twilio.controller.js`), the consent gate, and SMS sign-in - for the - * whole time the stale copy survived, which nothing bounded. + * `movePlatform` marks the copy it supersedes before creating the + * replacement, so a marked document is never the live record. Filtering on + * the mark rather than comparing `_ts` matters because a superseded copy + * can still be written to - a session predating the move still carries the + * old `displayName` and `membershipType`, and a player moving cross save + * back lands on it by `membershipId`. Either write would make the stale + * copy the newest, and a timestamp rule would then hand back the consent + * it never received. * - * A shared `id` is the signal: two genuinely distinct users never have - * one. The newest `_ts` is the record the move intended to leave behind. - * Anything else is a real duplicate and still throws, because that is a - * different bug and silently picking a winner would hide it. - * - * The stale copy is left in place: a getter is the wrong place to delete - * from, and `movePlatform` retries its own delete. This keeps reads - * correct meanwhile. + * Two live documents remain an error: that is a different bug, and quietly + * picking a winner would hide it. * @param {import('../helpers/documents.js').CosmosDocument<*>[]} documents * @param {string} description - what was looked up, for the error message * @returns {import('../helpers/documents.js').CosmosDocument<*> | undefined} */ - static #oneDocument(documents, description) { - if (documents.length <= 1) { - return documents[0]; - } + static #oneLiveDocument(documents, description) { + const live = documents.filter(({ movedTo }) => movedTo === undefined); - const [first] = documents; - const sharesAnId = first.id && documents.every(({ id }) => id === first.id); - - if (!sharesAnId) { + if (live.length > 1) { throw new Error(`more than 1 document found for ${description}`); } - const newest = documents.reduce((winner, document) => - (document._ts ?? 0) > (winner._ts ?? 0) ? document : winner, - ); + return live[0]; + } - log.error( - { - userId: newest.id, - membershipTypes: documents.map(({ membershipType }) => membershipType), - }, - 'An unfinished platform move left a stale copy; reading the newest until it is removed.', + /** + * Take the mark off a record whose move never created its successor. + * + * `movePlatform` marks before it creates, so a failure in between leaves a + * marked document with nothing to supersede it - invisible to every lookup + * but the Bungie-id one. When the player signs in again on the platform + * they were already on, that is the record, and it needs to be live again. + * @param {import('../helpers/documents.js').CosmosDocument} storedUser + * @returns {Promise} + */ + async clearPlatformMove(storedUser) { + const { movedTo: _movedTo, ...live } = storedUser; + + log.info( + { userId: storedUser.id, membershipType: storedUser.membershipType }, + 'Restoring a record whose platform move never completed.', ); - return newest; + return await this.#replaceAndCache(/** @type {*} */ (live), storedUser.membershipType); } /** diff --git a/users/user.service.spec.js b/users/user.service.spec.js index dc1d821b..38a28bb1 100644 --- a/users/user.service.spec.js +++ b/users/user.service.spec.js @@ -463,49 +463,70 @@ describe('UserService', () => { }); /** - * What every other lookup does while a `movePlatform` delete has not landed. - * The two documents share an id and differ only in partition, and before - * this they took out inbound STOP handling, the consent gate and SMS - * sign-in for as long as the stale copy survived. + * What every other lookup does while a `movePlatform` delete has not + * landed. The superseded copy carries `movedTo`, so it is not the live + * record however recently it was written to - which matters because an old + * session or a move back to the original platform can write to it, and a + * timestamp rule would then hand back the consent it never received. */ - describe('when an unfinished platform move has left a stale copy', () => { - const stale = { ...user, _ts: 1000, id: 'user-1', membershipType: 1 }; - const moved = { ...user, _ts: 2000, id: 'user-1', membershipType: 3 }; + describe('when an unfinished platform move has left a superseded copy', () => { + // Deliberately the *newer* of the two, which is what a timestamp rule got wrong. + const superseded = { ...user, _ts: 9999, id: 'user-1', membershipType: 1, movedTo: 3 }; + const live = { ...user, _ts: 1000, id: 'user-1', membershipType: 3 }; beforeEach(() => { - // Cosmos returns them in no particular order; the oldest is first here. - documentService.getDocuments.mockResolvedValue([stale, moved]); + documentService.getDocuments.mockResolvedValue([superseded, live]); cacheService.setUser.mockResolvedValue(); }); it('should keep the phone-number lookup working, which STOP handling runs on', async () => { await expect(userService.getUserByPhoneNumber(user.phoneNumber, true)).resolves.toEqual( - moved, + live, ); }); - it('should keep the consent gate working rather than suppressing notifications', async () => { + it('should give the consent gate the live record, not the superseded one', async () => { await expect(userService.getConsentByPhoneNumber(user.phoneNumber)).resolves.toEqual( - moved, + live, ); }); - it('should keep the id and Bungie-id lookups working', async () => { - await expect(userService.getUserById('user-1')).resolves.toEqual(moved); - await expect(userService.getUserByBungieMembershipId('99')).resolves.toEqual(moved); + it('should keep the id, e-mail and displayName lookups working', async () => { + await expect(userService.getUserById('user-1')).resolves.toEqual(live); + await expect(userService.getUserByEmailAddress(user.emailAddress)).resolves.toEqual( + live, + ); + await expect(userService.getUserByDisplayName(user.displayName, 3)).resolves.toEqual( + live, + ); }); - it('should say so loudly, because the stale copy still needs removing', async () => { - const errorLog = vi.spyOn(log, 'error').mockImplementation(() => {}); + /** + * The write path a session predating the move goes through. It must not + * find the superseded copy, or the write lands on a record nothing reads. + */ + it('should hide the superseded copy from a lookup on the old platform', async () => { + documentService.getDocuments.mockResolvedValue([superseded]); - await userService.getUserById('user-1'); + await expect( + userService.getUserByDisplayName(user.displayName, 1), + ).resolves.toBeUndefined(); + await expect(userService.getUserByMembershipId('11')).resolves.toBeUndefined(); + }); - expect(errorLog).toHaveBeenCalled(); + /** + * The exception, and the reason `signIn` can recover: a move that never + * created its successor would otherwise leave the player invisible. + */ + it('should still expose it to the Bungie-id lookup, which recovery runs on', async () => { + documentService.getDocuments.mockResolvedValue([superseded]); - errorLog.mockRestore(); + await expect(userService.getUserByBungieMembershipId('99')).resolves.toEqual( + superseded, + ); }); - it('should still throw when the documents are two different users', async () => { + it('should still throw when two live documents match', async () => { documentService.getDocuments.mockResolvedValue([ { ...user, id: 'user-a' }, { ...user, id: 'user-b' }, @@ -517,6 +538,24 @@ describe('UserService', () => { }); }); + describe('clearPlatformMove', () => { + it('should take the mark off and cache the restored record', async () => { + const marked = { ...user, _etag: 'e', id: 'user-1', membershipType: 3, movedTo: 1 }; + + documentService.updateDocument.mockImplementation((_c, document) => + Promise.resolve(document), + ); + cacheService.setUser.mockResolvedValue(); + + await userService.clearPlatformMove(marked); + + const [, document, partitionKey] = documentService.updateDocument.mock.calls[0]; + + expect(document).not.toHaveProperty('movedTo'); + expect(partitionKey).toBe(3); + }); + }); + describe('getUserByBungieMembershipId', () => { it('should return the user whose stored token carries that id', async () => { documentService.getDocuments.mockResolvedValue([user]); @@ -587,17 +626,56 @@ describe('UserService', () => { }; beforeEach(() => { - documentService.createDocument.mockImplementation(document => - Promise.resolve(document), - ); documentService.createDocument.mockImplementation((_collection, document) => Promise.resolve({ ...document, _etag: 'new-etag' }), ); + documentService.updateDocument.mockImplementation((_collection, document) => + Promise.resolve(document), + ); documentService.deleteDocumentById.mockResolvedValue(); cacheService.deleteUser.mockResolvedValue(); cacheService.setUser.mockResolvedValue(); }); + it('should mark the old copy before anything is duplicated', async () => { + const { promise: markPending, resolve: finishMark } = Promise.withResolvers(); + + documentService.updateDocument.mockReturnValue(markPending); + + const moving = userService.movePlatform(storedUser, steamMembership); + + await Promise.resolve(); + + expect(documentService.updateDocument).toHaveBeenCalledWith( + 'Users', + expect.objectContaining({ id: 'user-1', movedTo: 3 }), + 1, + ); + expect(documentService.createDocument).not.toHaveBeenCalled(); + + finishMark({}); + await moving; + }); + + it('should abort with nothing duplicated when the mark cannot be written', async () => { + documentService.updateDocument.mockRejectedValue(new Error('cosmos unavailable')); + + await expect(userService.movePlatform(storedUser, steamMembership)).rejects.toThrow( + 'cosmos unavailable', + ); + + expect(documentService.createDocument).not.toHaveBeenCalled(); + expect(documentService.deleteDocumentById).not.toHaveBeenCalled(); + }); + + it('should not carry the mark onto the record it creates', async () => { + await userService.movePlatform({ ...storedUser, movedTo: 3 }, steamMembership); + + const [, document] = documentService.createDocument.mock.calls[0]; + + expect(document).not.toHaveProperty('movedTo'); + }); + it('should create the record under the new platform, carrying the registration', async () => { await userService.movePlatform(storedUser, steamMembership); @@ -654,12 +732,35 @@ describe('UserService', () => { it('should still resolve when the old record cannot be removed', async () => { const errorLog = vi.spyOn(log, 'error').mockImplementation(() => {}); - documentService.deleteDocumentById.mockRejectedValue(new Error('cosmos unavailable')); + documentService.deleteDocumentById.mockRejectedValue( + Object.assign(new Error('cosmos unavailable'), { code: 503 }), + ); + + vi.useFakeTimers(); + + const moving = userService.movePlatform(storedUser, steamMembership); + + await vi.runAllTimersAsync(); + + await expect(moving).resolves.toBeDefined(); + expect(errorLog).toHaveBeenCalled(); + + vi.useRealTimers(); + errorLog.mockRestore(); + }); + + it('should not retry a status that will never succeed', async () => { + const errorLog = vi.spyOn(log, 'error').mockImplementation(() => {}); + + documentService.deleteDocumentById.mockRejectedValue( + Object.assign(new Error('forbidden'), { code: 403 }), + ); await expect( userService.movePlatform(storedUser, steamMembership), ).resolves.toBeDefined(); + expect(documentService.deleteDocumentById).toHaveBeenCalledTimes(1); expect(errorLog).toHaveBeenCalled(); errorLog.mockRestore(); @@ -674,7 +775,7 @@ describe('UserService', () => { ); }); - it('should retry a failed delete rather than leave the stale copy', async () => { + it('should retry a delete on a status that can clear on its own', async () => { vi.useFakeTimers(); const errorLog = vi.spyOn(log, 'error').mockImplementation(() => {}); @@ -712,7 +813,12 @@ describe('UserService', () => { errorLog.mockRestore(); }); - it('should treat a conflicting create as a concurrent move and still delete', async () => { + /** + * Either a concurrent sign-in moved first, or the player is moving back + * to a platform they left and the superseded copy is still sitting + * there. Replacing brings the target back to life in both cases. + */ + it('should replace the target when the create conflicts, then delete the source', async () => { documentService.createDocument.mockRejectedValue( Object.assign(new Error('conflict'), { code: 409 }), ); @@ -721,6 +827,10 @@ describe('UserService', () => { userService.movePlatform(storedUser, steamMembership), ).resolves.toMatchObject({ membershipType: 3 }); + const replace = documentService.updateDocument.mock.calls.at(-1); + + expect(replace[1]).not.toHaveProperty('movedTo'); + expect(replace[2]).toBe(3); expect(documentService.deleteDocumentById).toHaveBeenCalledWith('Users', 'user-1', 1); }); @@ -812,7 +922,7 @@ describe('UserService', () => { expect(collectionId).toBe('Users'); expect(query.query).toBe( - 'SELECT r.id, r._ts, r.isSubscribed, r.notifications, r.consentUpdatedAt FROM root r WHERE r.phoneNumber = @phoneNumber', + 'SELECT r.movedTo, r.isSubscribed, r.notifications, r.consentUpdatedAt FROM root r WHERE r.phoneNumber = @phoneNumber', ); expect(query.parameters).toEqual([{ name: '@phoneNumber', value: user.phoneNumber }]); }); From 5936ae220bfa1acb613e9b3af0365b1283049c58 Mon Sep 17 00:00:00 2001 From: Chris Paskvan Date: Sat, 26 Sep 2026 20:27:30 -0500 Subject: [PATCH 4/4] fix(users): let a player move back to a platform whose old copy survived Review feedback on #749. getUserByBungieMembershipId saw superseded copies on purpose, so signIn could finish an interrupted move. After a failed delete both copies carry the same Bungie id, so it threw - and since that is the lookup signIn falls through to when the platform membershipId no longer matches, a player moving cross save back could never sign in again. The replace-on-conflict branch meant to handle exactly that was unreachable from signIn; only its unit test reached it. It now prefers the live copy and returns a superseded one only when nothing live matches, which is the interrupted-move case it exists for. A controller test drives signIn with both copies present. Mark without the etag. It was sent as IfMatch, so of two sign-ins arriving together the second failed its precondition on the mark the first had just written. Marking is idempotent, so the write does not need one. Clear the cache directly after the mark rather than at the end. Everything between can throw, and a cached copy outlives the failure by an hour holding the record unmarked, so display-name lookups on the old identity kept resolving to something no query returns. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_018iD3V7Zyugg3atXVsogTBf --- users/user.controller.spec.js | 76 +++++++++++++++++++++++++++++++++++ users/user.service.js | 58 ++++++++++++++++---------- users/user.service.spec.js | 31 +++++++++++++- 3 files changed, 142 insertions(+), 23 deletions(-) diff --git a/users/user.controller.spec.js b/users/user.controller.spec.js index f46c8836..4f16ad87 100644 --- a/users/user.controller.spec.js +++ b/users/user.controller.spec.js @@ -1171,6 +1171,82 @@ describe('UserController.signIn against the real Destiny and User services', () }); }); + /** + * The case a `movePlatform` unit test could not reach. After a failed + * delete both copies carry the same Bungie id, so the lookup `signIn` falls + * through to used to throw - and the player could never sign in again. + */ + describe('when a player moves back to a platform whose superseded copy survived', () => { + const supersededXbox = { + _etag: 'a-etag', + bungie: { access_token: 'old', membership_id: 'bungie-net-99' }, + dateRegistered: '2026-01-01T00:00:00Z', + displayName: 'XboxGamertag', + id: 'user-1', + membershipId: 'xbox-membership', + membershipType: 1, + movedTo: 3, + phoneNumber: '+12085551234', + }; + const liveSteam = { + ...supersededXbox, + _etag: 'b-etag', + displayName: 'SteamPersona', + membershipId: 'steam-membership', + membershipType: 3, + movedTo: undefined, + }; + + beforeEach(() => { + post.mockResolvedValue({ + access_token: 'some-access-token', + expires_in: 3600, + membership_id: 'bungie-net-99', + refresh_token: 'some-refresh-token', + }); + // Bungie now reports Xbox as the owner again. + get.mockResolvedValueOnce({ + ErrorCode: 1, + Response: { + destinyMemberships: [ + { + crossSaveOverride: 1, + displayName: 'XboxGamertag', + membershipId: 'xbox-membership', + membershipType: 1, + }, + ], + }, + }); + documentService.getDocuments.mockImplementation((_collection, query) => + Promise.resolve( + query.query.includes('bungie.membership_id') ? [supersededXbox, liveSteam] : [], + ), + ); + documentService.createDocument.mockRejectedValue( + Object.assign(new Error('conflict'), { code: 409 }), + ); + }); + + it('should sign them in rather than throwing', async () => { + await expect(controller.signIn({ code: 'some-code' })).resolves.toMatchObject({ + displayName: 'XboxGamertag', + membershipType: 1, + }); + }); + + it('should bring the Xbox record back to life and retire the Steam one', async () => { + await controller.signIn({ code: 'some-code' }); + + const replace = documentService.updateDocument.mock.calls.at(-1); + + expect(replace[1]).toMatchObject({ membershipType: 1, id: 'user-1' }); + expect(replace[1]).not.toHaveProperty('movedTo'); + expect(replace[2]).toBe(1); + expect(documentService.deleteDocumentById).toHaveBeenCalledWith('Users', 'user-1', 3); + }); + }); + describe('when the account has no playable membership', () => { it('should sign nobody in and store nothing', async () => { post.mockResolvedValue({ diff --git a/users/user.service.js b/users/user.service.js index 3c76df90..27feb77e 100644 --- a/users/user.service.js +++ b/users/user.service.js @@ -724,18 +724,26 @@ class UserService { ); /** - * Deliberately not filtered on the mark, unlike every other lookup. - * This is the path `signIn` recovers through, so it has to be able to - * see a copy whose move never finished - that is what lets the next - * sign-in complete it instead of leaving the player invisible. + * The one lookup that can see a superseded copy, because it is the path + * `signIn` recovers through: a move whose successor was never created + * would otherwise leave the player invisible. + * + * Live wins when there is one. A superseded copy is returned only when + * nothing live matches, which is exactly the interrupted-move case. The + * order matters after a failed delete: both copies carry the same + * Bungie id, and preferring the live one is what lets a player moving + * back to their old platform reach `movePlatform` instead of a throw. */ - if (documents.length > 1) { + const live = documents.filter(({ movedTo }) => movedTo === undefined); + const candidates = live.length ? live : documents; + + if (candidates.length > 1) { throw new Error( `more than 1 document found for bungie.membership_id ${bungieMembershipId}`, ); } - return documents[0]; + return candidates[0]; } /** @@ -914,19 +922,39 @@ class UserService { const { membershipType } = /** @type {{ membershipType: number }} */ (membership); + const { _etag, _rid, _self, _attachments, _ts, movedTo, ...carried } = storedUser; + /** * Mark the old copy before anything is duplicated. The write is on its * own partition, so a failure here stops the move with nothing lost - * and once it lands, no lookup will return that copy again however many * times it is written to afterwards. + * + * Sent without the etag, so it is unconditional. Marking is idempotent, + * and two sign-ins arriving together would otherwise have the second + * fail its precondition on the mark the first just wrote. */ await this.documents.updateDocument( userCollectionId, - { ...storedUser, movedTo: membershipType }, + /** @type {*} */ ({ ...carried, movedTo: membershipType }), storedUser.membershipType, ); - const { _etag, _rid, _self, _attachments, _ts, movedTo, ...carried } = storedUser; + /** + * Directly after the mark, not at the end: everything below can throw, + * and a cached copy outlives the failure by an hour. It holds the + * record unmarked, so display-name lookups on the old identity would + * keep resolving to something no query returns any more. + */ + try { + await this.cacheService.deleteUser(storedUser); + } catch (err) { + log.warn( + { err, userId: storedUser.id }, + 'Failed to clear the cache while moving the user; continuing without it.', + ); + } + const moved = { ...carried, ...membership }; /** @type {*} */ let created; @@ -982,20 +1010,6 @@ class UserService { } } - /** - * Ahead of caching the new document, not after: the phone-number and - * email-address keys are shared by both records and hold a - * displayName+membershipType pointer that no longer resolves. - */ - try { - await this.cacheService.deleteUser(storedUser); - } catch (err) { - log.warn( - { err, userId: storedUser.id }, - 'Failed to clear the cache after moving the user; continuing without it.', - ); - } - const movedDocument = /** @type {import('../helpers/documents.js').CosmosDocument} */ ( created ?? moved diff --git a/users/user.service.spec.js b/users/user.service.spec.js index 38a28bb1..71e16979 100644 --- a/users/user.service.spec.js +++ b/users/user.service.spec.js @@ -518,7 +518,7 @@ describe('UserService', () => { * The exception, and the reason `signIn` can recover: a move that never * created its successor would otherwise leave the player invisible. */ - it('should still expose it to the Bungie-id lookup, which recovery runs on', async () => { + it('should expose a lone superseded copy to the Bungie-id lookup', async () => { documentService.getDocuments.mockResolvedValue([superseded]); await expect(userService.getUserByBungieMembershipId('99')).resolves.toEqual( @@ -526,6 +526,15 @@ describe('UserService', () => { ); }); + /** + * Both copies carry the same Bungie id after a failed delete. Throwing + * here made every sign-in fail for a player moving back to their old + * platform, because that is the lookup `signIn` falls through to. + */ + it('should prefer the live copy over the superseded one, not throw', async () => { + await expect(userService.getUserByBungieMembershipId('99')).resolves.toEqual(live); + }); + it('should still throw when two live documents match', async () => { documentService.getDocuments.mockResolvedValue([ { ...user, id: 'user-a' }, @@ -657,6 +666,26 @@ describe('UserService', () => { await moving; }); + it('should mark unconditionally, so a concurrent move does not fail its precondition', async () => { + await userService.movePlatform(storedUser, steamMembership); + + const [, document] = documentService.updateDocument.mock.calls[0]; + + expect(document).not.toHaveProperty('_etag'); + }); + + it('should clear the cache even when the move fails after the mark', async () => { + documentService.createDocument.mockRejectedValue( + Object.assign(new Error('boom'), { code: 500 }), + ); + + await expect(userService.movePlatform(storedUser, steamMembership)).rejects.toThrow( + 'boom', + ); + + expect(cacheService.deleteUser).toHaveBeenCalledWith(storedUser); + }); + it('should abort with nothing duplicated when the mark cannot be written', async () => { documentService.updateDocument.mockRejectedValue(new Error('cosmos unavailable'));