From 04d21ed93ce21a33a3248d144092b56024a376a5 Mon Sep 17 00:00:00 2001 From: Devin Zuczek Date: Mon, 26 Oct 2026 13:55:36 -0400 Subject: [PATCH] [playersettings] improve migration --- CLI.md | 20 ++-- DEPLOYING.md | 3 +- apps/playersettings/src/default-settings.ts | 3 +- apps/playersettings/src/playersettings.app.ts | 13 ++- .../src/test/integration/api.test.ts | 23 ++++ packages/domain/src/player-settings-db.ts | 36 ++++++- packages/tools/src/cmd/admin.cmd.ts | 102 ++++++++++++------ 7 files changed, 156 insertions(+), 44 deletions(-) diff --git a/CLI.md b/CLI.md index 8d19406..a905356 100644 --- a/CLI.md +++ b/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 -import, then `just deploy`. The import MERGES: a row that already exists keeps every key it -has and gains the ones only KV knew, so running it after the cut-over — or again — never -undoes something a player has toggled since. `--overwrite` makes the KV value win instead. -Nothing is deleted on either side. Keys that aren't a `player:` holding a JSON object of -strings are reported and skipped. Once it has run, the namespace (and the `RECFLARE_KV` line) -can be deleted. +Upgrade in this order: `just migrate -F playersettings` (creates the table), this import, +`just deploy`, then this import AGAIN. The first run does the bulk while the KV-backed workers +are still live; the second picks up players and keys that appeared in KV in between. The import MERGES: +KV fills in what a row lacks, and the row wins any key it has moved off the seeded default. +A key still AT its default (`Recroom.OOBE` 77, `TUTORIAL_COMPLETE_MASK` 11, +`FIRST_TIME_IN_FLAGS` 0) loses to KV, because `GET /playersettings` seeds exactly those for a +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:` 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 diff --git a/DEPLOYING.md b/DEPLOYING.md index 9ada880..571e918 100644 --- a/DEPLOYING.md +++ b/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 diff --git a/apps/playersettings/src/default-settings.ts b/apps/playersettings/src/default-settings.ts index 1d5b8b7..e4ab5bc 100644 --- a/apps/playersettings/src/default-settings.ts +++ b/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' }, diff --git a/apps/playersettings/src/playersettings.app.ts b/apps/playersettings/src/playersettings.app.ts index 8b419b7..cd10543 100644 --- a/apps/playersettings/src/playersettings.app.ts +++ b/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() '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() const patch: Record = {} 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) } ) diff --git a/apps/playersettings/src/test/integration/api.test.ts b/apps/playersettings/src/test/integration/api.test.ts index f41a78b..dca2cfe 100644 --- a/apps/playersettings/src/test/integration/api.test.ts +++ b/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) diff --git a/packages/domain/src/player-settings-db.ts b/packages/domain/src/player-settings-db.ts index 9e556c9..cc69941 100644 --- a/packages/domain/src/player-settings-db.ts +++ b/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 { + 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. diff --git a/packages/tools/src/cmd/admin.cmd.ts b/packages/tools/src/cmd/admin.cmd.ts index 4ee19f5..bd8b5ee 100644 --- a/packages/tools/src/cmd/admin.cmd.ts +++ b/packages/tools/src/cmd/admin.cmd.ts @@ -444,11 +444,50 @@ async function settingsNamespaceIdFromEnv(): Promise { * 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 - * the live row has and adds the ones only KV knew. That makes the import safe to run after - * the workers have cut over (a player who has since toggled something keeps it) and to - * re-run. `--overwrite` makes KV win outright instead. Nothing is deleted on either side. + * Rows that already exist are MERGED: KV fills in what D1 lacks, and D1 wins any key it has + * moved off the seeded default. A key D1 still holds AT its default (`SEEDED_DEFAULTS`) does + * not count as the player's — `GET /playersettings` seeds those for any player with no row, so + * 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 = { + '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 | 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 = {} + 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 + // `bulk get` answers `{ key: value }`, or `{ key: { value, metadata } }` with + // `--with-metadata`; read either. + let got: Record 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 - let parsed: unknown = null - try { - 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) { + const raw = got[key] + const map = kvSettingsMap(typeof raw === 'string' ? raw : raw?.value) + if (!Number.isSafeInteger(accountId) || accountId <= 0 || map === null) { 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( - ( - await execSql( - `SELECT COUNT(*) AS n FROM player_settings WHERE account_id IN (${ids.join(', ')})`, - remote, - 'playersettings' - ) - ).results[0]?.n ?? 0 - ) + let found = 0 + for (let i = 0; i < ids.length; i += 1000) { + found += Number( + ( + await execSql( + `SELECT COUNT(*) AS n FROM player_settings WHERE account_id IN (${ids.slice(i, i + 1000).join(', ')})`, + remote, + '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.`