From e00103c6b379fdbc21118c2da783d9a462085191 Mon Sep 17 00:00:00 2001 From: Devin Zuczek Date: Wed, 30 Sep 2026 18:04:56 -0400 Subject: [PATCH] [msc] make playersettings more efficient --- apps/api/src/routes/images.ts | 19 ++--- apps/api/src/test/integration/api.test.ts | 7 ++ apps/chat/package.json | 1 + apps/chat/src/chat.app.ts | 12 ++- apps/chat/src/test/integration/api.test.ts | 11 +++ apps/match/src/match.app.ts | 18 ++--- apps/match/src/test/integration/api.test.ts | 15 ++++ apps/playersettings/package.json | 1 + apps/playersettings/src/playersettings.app.ts | 19 +++-- .../src/test/integration/api.test.ts | 24 ++++++ packages/domain/src/index.ts | 1 + packages/domain/src/player-settings-kv.ts | 81 +++++++++++++++++++ pnpm-lock.yaml | 6 ++ 13 files changed, 178 insertions(+), 37 deletions(-) create mode 100644 packages/domain/src/player-settings-kv.ts diff --git a/apps/api/src/routes/images.ts b/apps/api/src/routes/images.ts index eb1a210..35d7e38 100644 --- a/apps/api/src/routes/images.ts +++ b/apps/api/src/routes/images.ts @@ -11,6 +11,8 @@ import { getImagesByRoom, getPlayerFeed, getSlideshowImages, + mergePlayerSettings, + readPlayerSettings, SavedImageType, setImageAccessibility, setImageCheer, @@ -100,10 +102,7 @@ async function getPlayerSettings( env: App['Bindings'], accountId: number ): Promise | null> { - return env.RECFLARE_PLAYER_SETTINGS.get>( - `player:${accountId}`, - 'json' - ).catch(() => null) + return readPlayerSettings(env.RECFLARE_PLAYER_SETTINGS, accountId).catch(() => null) } /** The caller's stored photo-tagging preference, or the default when they have none. */ @@ -119,19 +118,17 @@ async function readPhotoTaggingSetting(env: App['Bindings'], accountId: number): * * The write MERGES, as the `playersettings` worker's own PUT does: the map holds every * setting the player has (OOBE state, tutorial mask, …), so storing this one on its own - * would wipe the rest. Read-modify-write on KV isn't atomic, but the same is true there, - * and racing writers here means one player toggling two of their own options at once. + * would wipe the rest. Re-posting the value already stored writes nothing — the client + * does that freely, and KV writes are the cost. */ async function writePhotoTaggingSetting( env: App['Bindings'], accountId: number, setting: number ): Promise { - const stored = (await getPlayerSettings(env, accountId)) ?? {} - await env.RECFLARE_PLAYER_SETTINGS.put( - `player:${accountId}`, - JSON.stringify({ ...stored, [PHOTO_TAGGING_KEY]: String(setting) }) - ) + await mergePlayerSettings(env.RECFLARE_PLAYER_SETTINGS, accountId, { + [PHOTO_TAGGING_KEY]: String(setting), + }) } /** diff --git a/apps/api/src/test/integration/api.test.ts b/apps/api/src/test/integration/api.test.ts index 1f6194b..0ed2917 100644 --- a/apps/api/src/test/integration/api.test.ts +++ b/apps/api/src/test/integration/api.test.ts @@ -6742,6 +6742,13 @@ describe('images', () => { // The setting is per-player. expect(await (await read('710')).text()).toBe('2') + // Re-posting the stored value writes nothing to KV: the raw value is seeded with + // whitespace JSON.stringify never produces, and it survives the PUT untouched. + const padded = '{ "Recroom.OOBE": "77", "playerPhotoTaggingSetting": "1" }' + await env.RECFLARE_PLAYER_SETTINGS.put('player:712', padded) + expect(await (await write('712', { Setting: 1 })).text()).toBe('1') + expect(await env.RECFLARE_PLAYER_SETTINGS.get('player:712', 'text')).toBe(padded) + // A body with no readable Setting leaves the stored value alone rather than writing 0 // — and answers what the player still has. expect(await (await write('710', { Nothing: true })).text()).toBe('2') diff --git a/apps/chat/package.json b/apps/chat/package.json index 2fd64e5..952bf06 100644 --- a/apps/chat/package.json +++ b/apps/chat/package.json @@ -17,6 +17,7 @@ }, "dependencies": { "@2toad/profanity": "3.3.0", + "@repo/domain": "workspace:*", "@repo/hono-helpers": "workspace:*", "@repo/jwt": "workspace:*", "@standard-community/standard-json": "0.3.5", diff --git a/apps/chat/src/chat.app.ts b/apps/chat/src/chat.app.ts index c96683d..b7b7d15 100644 --- a/apps/chat/src/chat.app.ts +++ b/apps/chat/src/chat.app.ts @@ -2,6 +2,7 @@ import { Hono } from 'hono' import { describeRoute, openAPIRouteHandler } from 'hono-openapi' import { useWorkersLogger } from 'workers-tagged-logger' +import { mergePlayerSettings as mergeStoredPlayerSettings, readPlayerSettings } from '@repo/domain' import { logger, withCleanSpec, withNotFound, withOnError } from '@repo/hono-helpers' import { validateAndGetAccountId } from '@repo/jwt' @@ -201,10 +202,7 @@ async function getPlayerSettings( env: Env, accountId: number ): Promise | null> { - return env.RECFLARE_PLAYER_SETTINGS.get>( - `player:${accountId}`, - 'json' - ).catch(() => null) + return readPlayerSettings(env.RECFLARE_PLAYER_SETTINGS, accountId).catch(() => null) } /** @@ -251,15 +249,15 @@ async function writeChatPrivacy( * Merge keys into the player's settings map, the way the `playersettings` worker's own PUT * does. Never a whole-map write: the bag holds every setting the player has (OOBE state, * tutorial mask, …) and storing one key on its own would wipe the rest. Values are strings, - * which is what that worker stores and what its GET serves back. + * which is what that worker stores and what its GET serves back. Nothing is written when + * the patch changes no value — the client re-posts its privacy settings freely. */ async function mergePlayerSettings( env: Env, accountId: number, patch: Record ): Promise { - const merged: Record = { ...(await getPlayerSettings(env, accountId)), ...patch } - await env.RECFLARE_PLAYER_SETTINGS.put(`player:${accountId}`, JSON.stringify(merged)) + await mergeStoredPlayerSettings(env.RECFLARE_PLAYER_SETTINGS, accountId, patch) } /** diff --git a/apps/chat/src/test/integration/api.test.ts b/apps/chat/src/test/integration/api.test.ts index a2df8b0..7ee600c 100644 --- a/apps/chat/src/test/integration/api.test.ts +++ b/apps/chat/src/test/integration/api.test.ts @@ -956,6 +956,17 @@ describe('PUT /thread/chatPrivacySetting', () => { }) }) + // Re-posting the stored value writes nothing to KV — writes are the cost, and the client + // posts these freely. The raw value is seeded with whitespace JSON.stringify never + // produces; surviving the PUT untouched means no write happened. + it('does not write KV when the value is already stored', async () => { + const padded = '{ "Recroom.OOBE": "77", "directMessagePrivacySetting": "Favorites" }' + await env.RECFLARE_PLAYER_SETTINGS.put('player:887010', padded) + const res = await put(887010, { directMessagePrivacySetting: 'Favorites' }) + expect(await res.json()).toMatchObject({ directMessagePrivacySetting: 1 }) + expect(await env.RECFLARE_PLAYER_SETTINGS.get('player:887010', 'text')).toBe(padded) + }) + it('accepts the enum by ordinal too, and is case-insensitive about the name', async () => { expect(await (await put(887005, { directMessagePrivacySetting: '2' })).json()).toMatchObject({ directMessagePrivacySetting: 2, diff --git a/apps/match/src/match.app.ts b/apps/match/src/match.app.ts index cde14cf..f64b29d 100644 --- a/apps/match/src/match.app.ts +++ b/apps/match/src/match.app.ts @@ -38,6 +38,8 @@ import { MatchmakingErrorCode, MessageType, MOST_ACTIVE_CLUBHOUSE_LIMIT, + putPlayerSettingsIfChanged, + readPlayerSettings, recordRoomVisit, recordStat, refreshInstanceFullness, @@ -420,10 +422,7 @@ async function getPlayerSettings( env: Env, accountId: number ): Promise | null> { - return env.RECFLARE_PLAYER_SETTINGS.get>( - `player:${accountId}`, - 'json' - ).catch(() => null) + return readPlayerSettings(env.RECFLARE_PLAYER_SETTINGS, accountId).catch(() => null) } /** @@ -444,15 +443,16 @@ async function readAvoidJuniors(env: Env, accountId: number): Promise { * * The write MERGES, exactly as the `playersettings` worker's own PUT does: the map holds * every setting the player has (OOBE state, tutorial mask, …), so storing this one on its - * own would wipe the rest. Read-modify-write on KV isn't atomic, but the same is true of - * the settings worker, and two writers racing over one player's own settings means that - * player toggling two options in the same instant. + * own would wipe the rest. The key is whichever spelling the map already carries, which is + * why this doesn't go through `mergePlayerSettings` — but like it, re-posting the stored + * value writes nothing. A KV read failure throws (a 500) rather than reading as an empty + * map, which would store this one key over everything the player had. */ async function writeAvoidJuniors(env: Env, accountId: number, value: boolean): Promise { - const stored = (await getPlayerSettings(env, accountId)) ?? {} + const stored = await readPlayerSettings(env.RECFLARE_PLAYER_SETTINGS, accountId) const merged: Record = { ...stored } merged[findAvoidJuniorsKey(merged) ?? AVOID_JUNIORS_KEY] = value ? 'True' : 'False' - await env.RECFLARE_PLAYER_SETTINGS.put(`player:${accountId}`, JSON.stringify(merged)) + await putPlayerSettingsIfChanged(env.RECFLARE_PLAYER_SETTINGS, accountId, stored, merged) } /** diff --git a/apps/match/src/test/integration/api.test.ts b/apps/match/src/test/integration/api.test.ts index 4e77d86..2ed8798 100644 --- a/apps/match/src/test/integration/api.test.ts +++ b/apps/match/src/test/integration/api.test.ts @@ -642,6 +642,21 @@ describe('public endpoints', () => { expect(await stored(3202)).toEqual({ AVOID_JUNIORS: 'False' }) }) + // Re-posting the stored value writes nothing to KV — writes are the cost, and the + // client posts this freely. The raw value is seeded with whitespace JSON.stringify + // never produces; surviving the PUT untouched means no write happened. Holds across + // the loose key match too: `AVOID_JUNIORS: 'True'` already says what `avoidJuniors=True` + // asks for. + test('does not write KV when the value is already stored', async () => { + const padded = '{ "Recroom.OOBE": "77", "AVOID_JUNIORS": "True" }' + await env.RECFLARE_PLAYER_SETTINGS.put('player:3205', padded) + expect(await write(3205, 'avoidJuniors=True')).toBe(true) + expect(await env.RECFLARE_PLAYER_SETTINGS.get('player:3205', 'text')).toBe(padded) + + expect(await write(3205, 'avoidJuniors=False')).toBe(false) + expect(await stored(3205)).toEqual({ 'Recroom.OOBE': '77', AVOID_JUNIORS: 'False' }) + }) + test('accepts a JSON body', async () => { const res = await exports.default.fetch(`${ORIGIN}/player/avoidjuniors`, { method: 'PUT', diff --git a/apps/playersettings/package.json b/apps/playersettings/package.json index 3a38db9..d56b620 100644 --- a/apps/playersettings/package.json +++ b/apps/playersettings/package.json @@ -15,6 +15,7 @@ "test": "run-vitest" }, "dependencies": { + "@repo/domain": "workspace:*", "@repo/hono-helpers": "workspace:*", "@repo/jwt": "workspace:*", "@standard-community/standard-json": "0.3.5", diff --git a/apps/playersettings/src/playersettings.app.ts b/apps/playersettings/src/playersettings.app.ts index b915c58..efe5512 100644 --- a/apps/playersettings/src/playersettings.app.ts +++ b/apps/playersettings/src/playersettings.app.ts @@ -2,6 +2,7 @@ import { Hono } from 'hono' import { describeRoute, openAPIRouteHandler } from 'hono-openapi' import { useWorkersLogger } from 'workers-tagged-logger' +import { mergePlayerSettings } from '@repo/domain' import { withCleanSpec, withNotFound, withOnError } from '@repo/hono-helpers' import { validateAndGetAccountId } from '@repo/jwt' @@ -181,7 +182,9 @@ const app = new Hono() // Upsert player settings into KV, keyed by the authenticated player id. // A full replace would overwrite the player's entire set; we merge so individual key PUTs - // (e.g. `key=PlayerSessionCount&value=1`) don't wipe the rest. + // (e.g. `key=PlayerSessionCount&value=1`) don't wipe the rest. The client re-posts values + // it already has at every login and menu change, so a PUT that changes nothing is a + // read only — KV writes are what cost money here. .put( '/playersettings', describeRoute({ @@ -192,7 +195,8 @@ const app = new Hono() 'key PUT (`key=PlayerSessionCount&value=1`, which is what the client sends) leaves the', 'player’s other settings alone. A JSON body is also accepted, as one object or an', 'array, in either `key`/`value` or `Key`/`Value` casing; entries with an empty key are', - 'dropped. An unparseable or empty body is a no-op 200, not a 400. Empty body on success.', + 'dropped. An unparseable or empty body is a no-op 200, not a 400, and so is a PUT whose', + 'values are already stored — nothing is written to KV. Empty body on success.', ].join(' '), security: AUTHED, requestBody: formOrJson(SettingFormWrite, SettingJsonWrite, 'The setting(s) to write'), @@ -208,15 +212,10 @@ const app = new Hono() const incoming = await parseSettings(c) if (incoming.length === 0) return c.body(null, 200) - const kvKey = `player:${id}` - const existing = await c.env.RECFLARE_PLAYER_SETTINGS.get>( - kvKey, - 'json' - ) - const merged: Record = { ...existing } - for (const { key, value } of incoming) merged[key] = value + const patch: Record = {} + for (const { key, value } of incoming) patch[key] = value - await c.env.RECFLARE_PLAYER_SETTINGS.put(kvKey, JSON.stringify(merged)) + await mergePlayerSettings(c.env.RECFLARE_PLAYER_SETTINGS, id, patch) return c.body(null, 200) } ) diff --git a/apps/playersettings/src/test/integration/api.test.ts b/apps/playersettings/src/test/integration/api.test.ts index 4f678e3..7324fb5 100644 --- a/apps/playersettings/src/test/integration/api.test.ts +++ b/apps/playersettings/src/test/integration/api.test.ts @@ -140,6 +140,30 @@ describe('playersettings endpoints', () => { expect(stored).toEqual({ A: '1', B: '2' }) }) + // KV writes are the cost here, and the client re-posts settings it already has at every + // login. The raw value is seeded with whitespace the worker's own JSON.stringify would + // never produce: if it survives the PUT, nothing was written. + it('PUT /playersettings does not write KV when the value is already stored', async () => { + const padded = '{ "PlayerSessionCount": "5", "Recroom.OOBE": "77" }' + await env.RECFLARE_PLAYER_SETTINGS.put('player:10', padded) + + const same = await SELF.fetch( + `${ORIGIN}/playersettings`, + putForm({ key: 'PlayerSessionCount', value: '5' }, await bearer('10')) + ) + expect(same.status).toBe(200) + expect(await env.RECFLARE_PLAYER_SETTINGS.get('player:10', 'text')).toBe(padded) + + // A real change is still written — and compacted, which is how we know. + await SELF.fetch( + `${ORIGIN}/playersettings`, + putForm({ key: 'PlayerSessionCount', value: '6' }, await bearer('10')) + ) + expect(await env.RECFLARE_PLAYER_SETTINGS.get('player:10', 'text')).toBe( + JSON.stringify({ PlayerSessionCount: '6', 'Recroom.OOBE': '77' }) + ) + }) + it('PUT /playersettings 200s with no parseable settings', async () => { const res = await SELF.fetch(`${ORIGIN}/playersettings`, putForm({}, await bearer('9'))) expect(res.status).toBe(200) diff --git a/packages/domain/src/index.ts b/packages/domain/src/index.ts index 3d3cefb..e3e9072 100644 --- a/packages/domain/src/index.ts +++ b/packages/domain/src/index.ts @@ -12,6 +12,7 @@ export * from './clubs-db' export * from './d1-binds' export * from './images-db' export * from './password' +export * from './player-settings-kv' export * from './rooms-db' export * from './room-instance-db' export * from './room-comments-db' diff --git a/packages/domain/src/player-settings-kv.ts b/packages/domain/src/player-settings-kv.ts new file mode 100644 index 0000000..db3a1a5 --- /dev/null +++ b/packages/domain/src/player-settings-kv.ts @@ -0,0 +1,81 @@ +/** + * The player settings map in the `RECFLARE_PLAYER_SETTINGS` KV namespace. + * + * One key per player, `player:`, holding every setting they have as a flat + * `{ [Key]: Value }` record of strings — OOBE state, tutorial mask, chat privacy, the lot. + * The `playersettings` worker owns the namespace but `api`, `match` and `chat` each keep a + * setting of their own in the same map, so every writer merges into the record rather than + * replacing it, and all of them go through here. + * + * KV writes cost roughly ten times a read and are capped at one per second per key, and the + * client is noisy: it re-posts settings it already has at every login and menu change. Every + * write site has to read the map first to merge into it, so comparing the result against what + * was read is free — {@link putPlayerSettingsIfChanged} skips the `put` when the merged map + * is the one already stored. + */ + +export type PlayerSettings = Record + +/** The KV key holding a player's settings. */ +export function playerSettingsKey(accountId: number): string { + return `player:${accountId}` +} + +/** + * The player's stored settings, or null when they have none. A KV failure THROWS rather than + * reading as "none": a writer that mistook an outage for an empty map would then store its + * one key over everything the player had. Read-only callers that would rather degrade wrap + * this in their own `.catch`. + */ +export async function readPlayerSettings( + kv: KVNamespace, + accountId: number +): Promise { + return kv.get(playerSettingsKey(accountId), 'json') +} + +/** Whether two settings maps hold the same keys with the same values. Order is irrelevant. */ +export function samePlayerSettings(a: PlayerSettings | null, b: PlayerSettings | null): boolean { + const left = a ?? {} + const right = b ?? {} + const keys = Object.keys(left) + if (keys.length !== Object.keys(right).length) return false + return keys.every((key) => key in right && left[key] === right[key]) +} + +/** + * Store `next` as the player's settings unless it is what `existing` already says, in which + * case nothing is written. Returns whether a write happened. + * + * `existing` is the map the caller read to build `next` from; passing the map it read (not a + * fresh read) is the point — the comparison costs no KV operation. + */ +export async function putPlayerSettingsIfChanged( + kv: KVNamespace, + accountId: number, + existing: PlayerSettings | null, + next: PlayerSettings +): Promise { + if (existing !== null && samePlayerSettings(existing, next)) return false + await kv.put(playerSettingsKey(accountId), JSON.stringify(next)) + return true +} + +/** + * Merge `patch` into the player's settings, writing only if a value actually changed. The + * result carries the merged map (what the player now has) and whether KV was written. + * + * Read-modify-write on KV isn't atomic; racing writers here means one player toggling two + * of their own options in the same instant, which every caller has accepted since before + * this helper existed. + */ +export async function mergePlayerSettings( + kv: KVNamespace, + accountId: number, + patch: PlayerSettings +): Promise<{ settings: PlayerSettings; written: boolean }> { + const existing = await readPlayerSettings(kv, accountId) + const settings: PlayerSettings = { ...existing, ...patch } + const written = await putPlayerSettingsIfChanged(kv, accountId, existing, settings) + return { settings, written } +} diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index fcab0c9..1dba106 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -347,6 +347,9 @@ importers: '@2toad/profanity': specifier: 3.3.0 version: 3.3.0 + '@repo/domain': + specifier: workspace:* + version: link:../../packages/domain '@repo/hono-helpers': specifier: workspace:* version: link:../../packages/hono-helpers @@ -1041,6 +1044,9 @@ importers: apps/playersettings: dependencies: + '@repo/domain': + specifier: workspace:* + version: link:../../packages/domain '@repo/hono-helpers': specifier: workspace:* version: link:../../packages/hono-helpers