[playersettings] improve migration

CLI.md

@@ -174,13 +174,19 @@ across for a deployment that predates the move. The namespace id comes from
 `RECFLARE_PLAYER_SETTINGS` in `RECFLARE_KV` in `.env` (which the deploys used to read it from)
 or from `--namespace-id`.
 
-Upgrade in this order: `just migrate -F playersettings` (creates the table), then this
+Upgrade in this order: `just migrate -F playersettings` (creates the table), this import,
-import, then `just deploy`. The import MERGES: a row that already exists keeps every key it
+`just deploy`, then this import AGAIN. The first run does the bulk while the KV-backed workers
-has and gains the ones only KV knew, so running it after the cut-over — or again — never
+are still live; the second picks up players and keys that appeared in KV in between. The import MERGES:
-undoes something a player has toggled since. `--overwrite` makes the KV value win instead.
+KV fills in what a row lacks, and the row wins any key it has moved off the seeded default.
-Nothing is deleted on either side. Keys that aren't a `player:<id>` holding a JSON object of
+A key still AT its default (`Recroom.OOBE` 77, `TUTORIAL_COMPLETE_MASK` 11,
-strings are reported and skipped. Once it has run, the namespace (and the `RECFLARE_KV` line)
+`FIRST_TIME_IN_FLAGS` 0) loses to KV, because `GET /playersettings` seeds exactly those for a
-can be deleted.
+player with no row, so a returning player who signed in before their map was imported keeps
+their KV progress. The one thing it can't recover is a KV change made between the first
+run and the deploy to a key the first run already imported off-default; keep that window
+short. `--overwrite` makes the KV value win outright. Nothing is deleted on either side. A
+value that is a number or boolean is imported as its string; a null, object or array value
+drops only that key; a key that isn't a `player:<id>` holding a JSON object is reported and
+skipped. Once the second run is done, the namespace (and the `RECFLARE_KV` line) can be deleted.
 
 ### `lookup` — print an account
 

DEPLOYING.md

@@ -114,7 +114,8 @@ Take the IDs output from the commands and put them into `.env` (`RECFLARE_D1` an
 
 > Upgrading a deployment from before player settings moved into D1? They were in a
 > `RECFLARE_PLAYER_SETTINGS` KV namespace; copy it across with
-> `just settings-import-kv --remote` between `just migrate` and `just deploy` (see CLI.md).
+> `just settings-import-kv --remote` after `just migrate`, then again right after
+> `just deploy` (see CLI.md).
 
 The secrets store holds the shared `JWT_SECRET` HS256 signing key — every worker
 binds it so tokens signed by `auth` verify everywhere. Record its id in `.env` as

apps/playersettings/src/default-settings.ts

@@ -1,7 +1,8 @@
 /**
  * Default player settings seeded on a player's first read, ported verbatim from
  * the reference's `PlayerSettingsController.GetPlayerSettings`. Ordered; written to the player’s row the
- * first time a player has no stored settings.
+ * first time a player has no stored settings. Mirrored as `SEEDED_DEFAULTS` in
+ * packages/tools/src/cmd/admin.cmd.ts (the KV import treats these values as the seed) — keep in sync.
  */
 export const DEFAULT_SETTINGS: Array<{ Key: string; Value: string }> = [
 	{ Key: 'Recroom.OOBE', Value: '77' },

apps/playersettings/src/playersettings.app.ts

@@ -4,6 +4,7 @@ import { useWorkersLogger } from 'workers-tagged-logger'
 
 import {
 	mergePlayerSettings,
+	PlayerSettingsTooLargeError,
 	putPlayerSettingsIfChanged,
 	readPlayerSettings,
 	writePlayerSettings,
@@ -202,13 +203,16 @@ const app = new Hono<App>()
 				'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, and so is a PUT whose',
-				'values are already stored — nothing is written. Empty body on success.',
+				'values are already stored — nothing is written. A PUT that would make the stored map',
+				'larger than 64 KiB of JSON is refused with a 413 and changes nothing. Empty body on',
+				'success.',
 			].join(' '),
 			security: AUTHED,
 			requestBody: formOrJson(SettingFormWrite, SettingJsonWrite, 'The setting(s) to write'),
 			responses: {
 				200: { description: 'Applied, or nothing parseable to apply (empty body)' },
 				401: UNAUTHORIZED_RESPONSE,
+				413: { description: 'The stored map would exceed 64 KiB; nothing was written' },
 			},
 		}),
 		async (c) => {
@@ -221,7 +225,12 @@ const app = new Hono<App>()
 			const patch: Record<string, string> = {}
 			for (const { key, value } of incoming) patch[key] = value
 
-			await mergePlayerSettings(c.env.DB, id, patch)
+			try {
+				await mergePlayerSettings(c.env.DB, id, patch)
+			} catch (err) {
+				if (err instanceof PlayerSettingsTooLargeError) return c.body(null, 413)
+				throw err
+			}
 			return c.body(null, 200)
 		}
 	)

apps/playersettings/src/test/integration/api.test.ts

@@ -182,6 +182,29 @@ describe('playersettings endpoints', () => {
 		)
 	})
 
+	// The map lives on the D1 every worker shares, so a player can't grow it without bound.
+	it('PUT /playersettings refuses a write that would push the map past the cap', async () => {
+		await seedRaw(11, JSON.stringify({ A: '1' }))
+		const big = Array.from({ length: 70 }, (_, i) => ({ key: `K${i}`, value: 'x'.repeat(1024) }))
+
+		const res = await SELF.fetch(`${ORIGIN}/playersettings`, {
+			method: 'PUT',
+			headers: { ...(await bearer('11')), 'content-type': 'application/json' },
+			body: JSON.stringify(big),
+		})
+		expect(res.status).toBe(413)
+		expect(await stored(11)).toEqual({ A: '1' })
+
+		// Under the cap, the same kind of write still lands.
+		const ok = await SELF.fetch(`${ORIGIN}/playersettings`, {
+			method: 'PUT',
+			headers: { ...(await bearer('11')), 'content-type': 'application/json' },
+			body: JSON.stringify(big.slice(0, 10)),
+		})
+		expect(ok.status).toBe(200)
+		expect(Object.keys((await stored(11)) ?? {})).toHaveLength(11)
+	})
+
 	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/player-settings-db.ts

@@ -45,12 +45,43 @@ export async function readPlayerSettings(
 	return row === null ? null : (JSON.parse(row.data) as PlayerSettings)
 }
 
-/** Store `settings` as the player's whole map, replacing whatever row they had. */
+/**
+ * The most a player's stored map may hold, in bytes of its JSON. The map is player-written
+ * and lives on the D1 every worker shares, so an uncapped one lets any account grow its row
+ * toward D1's ~2 MB row limit and, across enough accounts, fill the database. Real maps are
+ * tiny: the 15,134 imported from KV averaged ~2 KB (46 keys), and the largest was ~12 KB
+ * (182 keys) — this leaves about five times that.
+ */
+export const MAX_PLAYER_SETTINGS_BYTES = 64 * 1024
+
+/** A write that would grow a player's map past {@link MAX_PLAYER_SETTINGS_BYTES}. */
+export class PlayerSettingsTooLargeError extends Error {
+	constructor(
+		readonly accountId: number,
+		readonly bytes: number
+	) {
+		super(
+			`player ${accountId}'s settings would be ${bytes} bytes (limit ${MAX_PLAYER_SETTINGS_BYTES})`
+		)
+		this.name = 'PlayerSettingsTooLargeError'
+	}
+}
+
+function settingsBytes(settings: PlayerSettings): number {
+	return new TextEncoder().encode(JSON.stringify(settings)).byteLength
+}
+
+/**
+ * Store `settings` as the player's whole map, replacing whatever row they had. Throws
+ * {@link PlayerSettingsTooLargeError} when the map is over {@link MAX_PLAYER_SETTINGS_BYTES}.
+ */
 export async function writePlayerSettings(
 	db: D1Database,
 	accountId: number,
 	settings: PlayerSettings
 ): Promise<void> {
+	const bytes = settingsBytes(settings)
+	if (bytes > MAX_PLAYER_SETTINGS_BYTES) throw new PlayerSettingsTooLargeError(accountId, bytes)
 	await db
 		.prepare(
 			`INSERT INTO player_settings (account_id, data) VALUES (?1, ?2)
@@ -76,7 +107,8 @@ export function samePlayerSettings(a: PlayerSettings | null, b: PlayerSettings |
 
 /**
  * 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.
+ * case nothing is written. Returns whether a write happened. Throws
+ * {@link PlayerSettingsTooLargeError} when `next` is over {@link MAX_PLAYER_SETTINGS_BYTES}.
  *
  * `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 database operation.

packages/tools/src/cmd/admin.cmd.ts

@@ -444,11 +444,50 @@ async function settingsNamespaceIdFromEnv(): Promise<string> {
  * by the `playersettings` worker). A KV value was the player's whole map as a JSON object of
  * strings, which is exactly what the row's `data` holds, so each key becomes one row.
  *
- * Rows that already exist are MERGED, with D1 winning: `json_patch(kv, d1)` keeps every key
+ * Rows that already exist are MERGED: KV fills in what D1 lacks, and D1 wins any key it has
- * the live row has and adds the ones only KV knew. That makes the import safe to run after
+ * moved off the seeded default. A key D1 still holds AT its default (`SEEDED_DEFAULTS`) does
- * the workers have cut over (a player who has since toggled something keeps it) and to
+ * not count as the player's — `GET /playersettings` seeds those for any player with no row, so
- * re-run. `--overwrite` makes KV win outright instead. Nothing is deleted on either side.
+ * a player who signed in after the cut-over but before the import has a fresh row of
+ * defaults, and their KV progress (OOBE, tutorial mask) has to beat it. That makes the import
+ * safe to run after the cut-over and to re-run. `--overwrite` makes KV win outright instead.
+ * Nothing is deleted on either side.
+ *
+ * What it cannot recover is a KV write made after the import read the namespace, to a key the
+ * imported row already holds off-default: D1 keeps the imported value. So run it once before
+ * `just deploy` (the bulk) and once after (to catch anyone who changed something in between).
  */
+/**
+ * What `GET /playersettings` seeds for a player with no row — mirror of `DEFAULT_SETTINGS` in
+ * apps/playersettings/src/default-settings.ts. A D1 value equal to one of these may be the
+ * seed rather than the player's, so the import lets KV replace it.
+ */
+const SEEDED_DEFAULTS: Record<string, string> = {
+	'Recroom.OOBE': '77',
+	TUTORIAL_COMPLETE_MASK: '11',
+	FIRST_TIME_IN_FLAGS: '0',
+}
+
+/**
+ * A KV value as a settings map, or null when it isn't one. Numbers and booleans are kept as
+ * their string form (what a PUT would have stored); a null, object or array value drops just
+ * that key rather than the player's whole map.
+ */
+function kvSettingsMap(value: string | null | undefined): Record<string, string> | null {
+	let parsed: unknown
+	try {
+		parsed = value == null ? null : JSON.parse(value)
+	} catch {
+		return null
+	}
+	if (parsed === null || typeof parsed !== 'object' || Array.isArray(parsed)) return null
+	const map: Record<string, string> = {}
+	for (const [k, v] of Object.entries(parsed)) {
+		if (typeof v === 'string') map[k] = v
+		else if (typeof v === 'number' || typeof v === 'boolean') map[k] = String(v)
+	}
+	return map
+}
+
 const settingsImportKv = new Command('settings-import-kv')
 	.description('Copy the old RECFLARE_PLAYER_SETTINGS KV namespace into player_settings (merges)')
 	.option(
@@ -485,7 +524,9 @@ const settingsImportKv = new Command('settings-import-kv')
 			const chunk = keys.slice(i, i + KV_BULK_GET_LIMIT)
 			const file = path.join(os.tmpdir(), `recflare-kv-keys-${Date.now()}-${i}.json`)
 			await fs.writeFile(file, JSON.stringify(chunk))
-			let got: Record<string, { value: string | null }>
+			// `bulk get` answers `{ key: value }`, or `{ key: { value, metadata } }` with
+			// `--with-metadata`; read either.
+			let got: Record<string, string | { value: string | null } | null>
 			try {
 				got = parseWranglerJson(
 					await runKv(['bulk', 'get', file, '--namespace-id', namespaceId], remote)
@@ -495,43 +536,38 @@ const settingsImportKv = new Command('settings-import-kv')
 			}
 			for (const key of chunk) {
 				const accountId = Number(key.slice(SETTINGS_KV_PREFIX.length))
-				const value = got[key]?.value
+				const raw = got[key]
-				let parsed: unknown = null
+				const map = kvSettingsMap(typeof raw === 'string' ? raw : raw?.value)
-				try {
+				if (!Number.isSafeInteger(accountId) || accountId <= 0 || map === null) {
-					parsed = value == null ? null : JSON.parse(value)
-				} catch {
-					parsed = null
-				}
-				const isMap =
-					parsed !== null &&
-					typeof parsed === 'object' &&
-					!Array.isArray(parsed) &&
-					Object.values(parsed).every((v) => typeof v === 'string')
-				if (!Number.isSafeInteger(accountId) || accountId <= 0 || !isMap) {
 					skipped.push(key)
 					continue
 				}
 				// An empty map is what a player with no row reads as; nothing to import.
-				if (Object.keys(parsed as object).length === 0) {
+				if (Object.keys(map).length === 0) {
 					empty++
 					continue
 				}
-				rows.push({ accountId, data: JSON.stringify(parsed) })
+				rows.push({ accountId, data: JSON.stringify(map) })
 			}
 			console.log(`  fetched ${Math.min(i + chunk.length, keys.length)}/${keys.length}`)
 		}
 		for (const key of skipped) console.log(chalk.yellow(`  skipping ${key}: not a settings map`))
 
+		// Merge: start from D1, lay KV over it, then lay back every D1 key that isn't sitting at
+		// its seeded default. (`json_group_object` of no rows is `{}`, which patches nothing.)
+		const d1OwnKeys = `(SELECT json_group_object(s.key, s.value) FROM json_each(player_settings.data) AS s WHERE NOT EXISTS (SELECT 1 FROM json_each('${sqlStr(JSON.stringify(SEEDED_DEFAULTS))}') AS d WHERE d.key = s.key AND d.value = s.value))`
 		const onConflict = opts.overwrite
 			? 'excluded.data'
-			: 'json_patch(excluded.data, player_settings.data)'
+			: `json_patch(json_patch(player_settings.data, excluded.data), ${d1OwnKeys})`
 		const statements = rows.map(
 			(r) =>
 				`INSERT INTO player_settings (account_id, data) VALUES (${r.accountId}, '${sqlStr(r.data)}') ON CONFLICT (account_id) DO UPDATE SET data = ${onConflict};`
 		)
 		console.log(
 			`${rows.length} player(s) to import, ${empty} empty, ${skipped.length} skipped` +
-				(opts.overwrite ? ' (KV overwrites existing rows)' : ' (existing rows merge, D1 wins)')
+				(opts.overwrite
+					? ' (KV overwrites existing rows)'
+					: ' (existing rows merge; D1 wins unless still at a seeded default)')
 		)
 		if (opts.dryRun) {
 			console.log(
@@ -561,16 +597,20 @@ const settingsImportKv = new Command('settings-import-kv')
 
 		// Prove it landed: a merge never deletes, so the table holds at least every imported id.
 		const after = await count()
+		// Chunked: one IN list of every id outgrows D1's statement limit (and argv) at ~15k players.
 		const ids = rows.map((r) => r.accountId)
-		const found = Number(
+		let found = 0
-			(
+		for (let i = 0; i < ids.length; i += 1000) {
-				await execSql(
+			found += Number(
-					`SELECT COUNT(*) AS n FROM player_settings WHERE account_id IN (${ids.join(', ')})`,
+				(
-					remote,
+					await execSql(
-					'playersettings'
+						`SELECT COUNT(*) AS n FROM player_settings WHERE account_id IN (${ids.slice(i, i + 1000).join(', ')})`,
-				)
+						remote,
-			).results[0]?.n ?? 0
+						'playersettings'
-		)
+					)
+				).results[0]?.n ?? 0
+			)
+		}
 		if (found !== ids.length) {
 			throw new Error(
 				`import did not land: ${ids.length - found} of ${ids.length} rows are missing. Re-run it.`