[msc] make playersettings more efficient

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<Record<string, string> | null> {
-	return env.RECFLARE_PLAYER_SETTINGS.get<Record<string, string>>(
+	return readPlayerSettings(env.RECFLARE_PLAYER_SETTINGS, accountId).catch(() => null)
-		`player:${accountId}`,
-		'json'
-	).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,
+ * would wipe the rest. Re-posting the value already stored writes nothing — the client
- * and racing writers here means one player toggling two of their own options at once.
+ * does that freely, and KV writes are the cost.
  */
 async function writePhotoTaggingSetting(
 	env: App['Bindings'],
 	accountId: number,
 	setting: number
 ): Promise<void> {
-	const stored = (await getPlayerSettings(env, accountId)) ?? {}
+	await mergePlayerSettings(env.RECFLARE_PLAYER_SETTINGS, accountId, {
-	await env.RECFLARE_PLAYER_SETTINGS.put(
+		[PHOTO_TAGGING_KEY]: String(setting),
-		`player:${accountId}`,
+	})
-		JSON.stringify({ ...stored, [PHOTO_TAGGING_KEY]: String(setting) })
-	)
 }
 
 /**

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')

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",

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<Record<string, string> | null> {
-	return env.RECFLARE_PLAYER_SETTINGS.get<Record<string, string>>(
+	return readPlayerSettings(env.RECFLARE_PLAYER_SETTINGS, accountId).catch(() => null)
-		`player:${accountId}`,
-		'json'
-	).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<string, string>
 ): Promise<void> {
-	const merged: Record<string, string> = { ...(await getPlayerSettings(env, accountId)), ...patch }
+	await mergeStoredPlayerSettings(env.RECFLARE_PLAYER_SETTINGS, accountId, patch)
-	await env.RECFLARE_PLAYER_SETTINGS.put(`player:${accountId}`, JSON.stringify(merged))
 }
 
 /**

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,

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<Record<string, string> | null> {
-	return env.RECFLARE_PLAYER_SETTINGS.get<Record<string, string>>(
+	return readPlayerSettings(env.RECFLARE_PLAYER_SETTINGS, accountId).catch(() => null)
-		`player:${accountId}`,
-		'json'
-	).catch(() => null)
 }
 
 /**
@@ -444,15 +443,16 @@ async function readAvoidJuniors(env: Env, accountId: number): Promise<boolean> {
  *
  * 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
+ * own would wipe the rest. The key is whichever spelling the map already carries, which is
- * the settings worker, and two writers racing over one player's own settings means that
+ * why this doesn't go through `mergePlayerSettings` — but like it, re-posting the stored
- * player toggling two options in the same instant.
+ * 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<void> {
-	const stored = (await getPlayerSettings(env, accountId)) ?? {}
+	const stored = await readPlayerSettings(env.RECFLARE_PLAYER_SETTINGS, accountId)
 	const merged: Record<string, string> = { ...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)
 }
 
 /**

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',

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",

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<App>()
 
 	// 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<App>()
 				'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<App>()
 			const incoming = await parseSettings(c)
 			if (incoming.length === 0) return c.body(null, 200)
 
-			const kvKey = `player:${id}`
+			const patch: Record<string, string> = {}
-			const existing = await c.env.RECFLARE_PLAYER_SETTINGS.get<Record<string, string>>(
+			for (const { key, value } of incoming) patch[key] = value
-				kvKey,
-				'json'
-			)
-			const merged: Record<string, string> = { ...existing }
-			for (const { key, value } of incoming) merged[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)
 		}
 	)

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)

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'

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:<accountId>`, 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<string, string>
+
+/** 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<PlayerSettings | null> {
+	return kv.get<PlayerSettings>(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<boolean> {
+	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 }
+}

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