fix(users): support the intended Bungie platforms during first sign-in

[chrispaskvan]

Closes #718.

The bug

DestinyService.getCurrentUser() returns the membership that owns cross-saved data — Steam (3) for most PC players — while anonymousUserSchema validated membershipType as min(1).max(2). Steam-first accounts were rejected at createAnonymousUser and the OAuth callback 500'd.

Two adjacent defects sat on the same path:

• #getPreferredMembership destructured memberships[0], so an account with no Destiny memberships threw a TypeError instead of reaching the 404 the route already implements.
• It read crossSaveOverride off index 0 with an index-0 fallback, which can return a membership that does not own the data.

Changes

helpers/bungie.membershipTypes.js (new) — the supported platforms in one place: 1 Xbox, 2 PSN, 3 Steam, 6 Epic. Excludes 0 None, 4 Blizzard (migrated to Steam in 2019), 5 Stadia (retired 2023), 10 Demon, 254 BungieNext. Both user schemas validate against it; userSchema.membershipType was previously unbounded z.number().int(), so the two now agree. Typing CurrentUser.membershipType as the same union is what makes the services agree at compile time — the exact class of mismatch this issue was.

destiny/destiny.service.js — selection now uses the rule already proven in twilio/mms.service.js: crossSaveOverride === membershipType || !crossSaveOverride. Resolves undefined for an empty membership list or an unsupported owner, logging which. Callers already handled a falsy result; destiny.controller.js and mcp/mcp.routes.js propagate the | undefined.

Cross-save owner changes — moving the membership that owns cross-saved data 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 (phone number, notifications, consent) on the first. Silently.

getUserByBungieMembershipId looks the player up on bungie.membership_id, the Bungie.net id that survives the change — already stored, already on the session at authentication.controller.js:55, never queried. Finding the record is not enough, though: /membershipType is the Cosmos partition key and is fixed for a document's life, so movePlatform recreates the record under the new platform and removes the old copy, carrying the registration across. Create first, delete second — a failure between the two leaves a visible duplicate rather than losing the registration.

displayName ceiling widened 16 → 32 for Steam persona names; min(3) kept.

Verification

pnpm test 971 passed, pnpm typecheck clean, pnpm swagger leaves openapi.json unchanged.

Every new test was mutation-checked against the pre-fix source:

Mutation	Tests that failed
membershipType: z.number().int().min(1).max(2)	5
const membership = memberships[0]	1
original const [{ crossSaveOverride }] = memberships	2
unsupported-platform guard disabled	1
delete before create in movePlatform	1
Cosmos system properties carried onto the new record	1
cache the moved record before clearing old keys	1
partition validation removed from movePlatform	1
no Bungie-id fallback in signIn	4
update in place instead of moving	3

Notes for review

• One new lint warning, left deliberately. users/user.service.js:365 reports suppressions/unused on the biome-ignore above the dead #deleteUser. Biome counts the bare identifier deleteUser in movePlatform's this.cacheService.deleteUser(storedUser) as a use of the private member, so the rule stops firing and its suppression reads as pointless — verified by rewriting the call as a computed access, which makes the warning vanish. Removing the suppression would make the file depend on a biome false-negative, and deleting #deleteUser belongs in its own change. biome check exits 0.
• Known gap. Documents written before the full Bungie token was persisted carry no bungie.membership_id and will not match the fallback. They acquire one on their next ordinary sign-in, so it closes itself without a backfill.
• Coverage thresholds untouched. Branch coverage was already below the recorded threshold before this change (83.96% vs 88.99%) and this moves it to 84.06%, so vitest.config.js is left alone rather than ratcheted mid-fix.
• Follow-up filed as refactor(users): move the Users container onto a stable partition key #748. movePlatform exists only because /membershipType is the partition key. A stable key deletes it.
• twilio/mms.service.js and destiny/destiny.service.js now hold two copies of the cross-save rule — worth collapsing into the new helper, as its own change.

🤖 Generated with Claude Code

https://claude.ai/code/session_018iD3V7Zyugg3atXVsogTBf

[chrispaskvan]

The #718 fix itself looks right: one list of supported platforms, the same cross-save selection rule as mms.service.js, and an empty membership list now reaching the 404. The Bungie-id fallback is the right key for a returning player. The main concern is what movePlatform leaves behind when its delete fails (inline).

Predates this PR, but now more likely: when signIn finds the user and the platform hasn't changed, updateUser looks the record up by the new displayName + membershipType (users/user.service.js:973). Steam persona names can change, so a renamed Steam player isn't found and sign-in throws. This PR brings in more Steam users, so it's probably worth its own issue.

[chrispaskvan]

Re-review of a3d6830. All three earlier comments are addressed, and the replies match the code. Two smaller issues in the new code are inline. Neither blocks merging.

[chrispaskvan]

Re-review of 27a98e2. Both earlier comments are fixed as suggested: marking the old copy before creating the new one replaces the timestamp rule, and the retry now only retries temporary errors, with fake timers in the tests. Two gaps in the new flow and one nit are inline.

[chrispaskvan]

Medium: moving back after a failed delete makes every sign-in 500, and nothing fixes it. Take a move from Xbox (1) to Steam (3) that left a marked Xbox copy A (the delete failed) and a live Steam copy B. When the player moves cross-save back to Xbox:

• getUserByMembershipId(xbox id) finds only A, which is marked, so #oneLiveDocument filters it out.
• signIn falls back to this lookup, which is unfiltered. It finds both A and B and throws here.

The replace-on-conflict branch in movePlatform that the reply says handles move-back is never reached from signIn. Only the movePlatform unit test exercises it.

Suggested fix: return the single live document if there is one, fall back to the single marked one only when nothing is live (the recovery case this lookup exists for), and throw otherwise. B is then returned, its platform differs from Xbox, and the move and replace run as intended. A signIn controller test with [A marked, B live] would catch this.

[chrispaskvan]

You're right, and the sharper part of this is that my previous reply was wrong: I claimed the replace-on-conflict branch handled move-back, and as you say signIn can't reach it — only the movePlatform unit test could. Writing a test at the wrong level let me believe a path was covered when nothing exercised it.

Fixed in 5936ae2, exactly as you suggested: getUserByBungieMembershipId filters to live documents and uses those when any exist, falls back to the full set only when nothing is live (the interrupted-move case it exists for), and throws otherwise. Moving back then returns B, its platform differs from Xbox, and the move and replace run as intended.

Two tests, at both levels this time. The service one asserts live wins over a superseded copy sharing the id. The controller one drives signIn end to end with [A marked, B live] and Bungie reporting Xbox as owner again — it asserts the sign-in resolves, the Xbox record comes back with no mark on partition 1, and the Steam copy is deleted from partition 3. Reverting the fix fails 3 tests, including that one.

🤖 Reviewed with Claude Code · https://claude.ai/code/session_018iD3V7Zyugg3atXVsogTBf

[chrispaskvan]

Low: two sign-ins at once 500 again, now at this step. storedUser carries its _etag, so updateDocument sends it as IfMatch. If two callbacks read A, the first one marks it and changes its etag, and the second gets a 412 and throws, so the 409 handling below is never reached. Marking is safe to repeat, so dropping _etag from this write (or treating a 412 here as "someone else is already moving it") would fix it.

[chrispaskvan]

Confirmed and fixed in 5936ae2 — updateDocument sets IfMatch whenever the document carries an _etag, and storedUser always does.

Took the first of your two options: the mark is built from the already-destructured carried, which has the system properties stripped, so it goes unconditionally. Marking is idempotent, so there's nothing a precondition would protect. Treating a 412 as "someone else is moving it" would have worked too, but it needs the reader to know why a conditional write is there at all, and the answer would have been "no reason".

Test asserts the mark write carries no _etag.

🤖 Reviewed with Claude Code · https://claude.ai/code/session_018iD3V7Zyugg3atXVsogTBf

[chrispaskvan]

Nit: if this create fails with anything other than a 409, the method throws before cacheService.deleteUser. The cache then keeps serving the unmarked copy for its lifetime, so display-name lookups on the old identity still return it. Writes are safe, because the stale _etag makes them fail with a 412 rather than remove the mark, but those reads are wrong. Clearing the cache right after the mark lands would close this.

[chrispaskvan]

Fixed in 5936ae2 — the cache clear now runs directly after the mark lands, so it happens before anything that can throw.

Your read of the write path is right, and it's worth keeping in the comment: a stale cached _etag makes writes fail their precondition rather than silently removing the mark, so the exposure was reads only. The comment now says the clear is there because everything below it can throw and a cached copy outlives the failure by an hour.

Test drives a create failure and asserts cacheService.deleteUser was still called.

🤖 Reviewed with Claude Code · https://claude.ai/code/session_018iD3V7Zyugg3atXVsogTBf

[chrispaskvan]

Re-review of 5936ae2: all three comments from the last round are fixed, and this looks ready to merge from my side.

• Moving back after a failed delete: getUserByBungieMembershipId now returns the live document when there is one and a marked copy only when nothing live matches. Following Xbox → Steam → Xbox through: the live Steam copy is returned, movePlatform marks it, the create conflicts in the Xbox partition, and the old Xbox copy is replaced with current data and no mark before the Steam copy is deleted. The new signIn controller test covers this end to end.
• Two sign-ins at once: the mark is written without the etag, so the second callback no longer fails with a 412. Covered by a test.
• Stale cache: the cache is cleared right after the mark, before anything that can throw, with a test for a move that fails afterwards.

Optional follow-up, not blocking: the mark replaces the whole document with the copy read at sign-in. A write (a STOP, say) that lands on the old copy in the few milliseconds between that read and the mark would be overwritten. The new document is built from the same copy, so this narrow window predates these changes. A Cosmos patch that only adds /movedTo would stop the mark overwriting anything, and could go alongside #748.

Verified locally: the four changed spec files pass (174 tests, including the six new ones). CI is green apart from Snyk Code's test-limit check.

[chrispaskvan]

Thanks for tracing Xbox → Steam → Xbox through — that's the path my unit test couldn't reach, so it's the one I most wanted a second pair of eyes on.

On the optional follow-up: you're right about the window, and I'd add one thing that changes where it belongs. helpers/documents.js has no patch method, so this needs a new patchDocument wrapping container.item(id, pk).patch([{ op: 'add', path: '/movedTo', value }]) — small, but a new capability on the Cosmos helper rather than a tweak to movePlatform.

The catch with putting it alongside #748: if that lands, movePlatform goes away entirely and the mark with it, so a patch-based mark would be throwaway work. The two are alternatives rather than companions. It's worth doing only if #748 is far enough out that this window matters in the meantime — your call, and happy to do it either here or as its own PR if you'd rather not sit on it.

I've noted the interaction on #748 so whoever picks that up knows this exists.

Not merging — that's yours to call.

🤖 Reviewed with Claude Code · https://claude.ai/code/session_018iD3V7Zyugg3atXVsogTBf