fix issue with subroom save list not working in room with no current save

This commit is contained in:
Devin Zuczek
2026-08-06 14:47:52 -04:00
parent 880c6ab2dc
commit aeff7d50cd
4 changed files with 78 additions and 35 deletions
+15 -6
View File
@@ -90,20 +90,29 @@ inconsistency here without checking the client first.
no asset arrays; but `unityAsset`/`unityAssetHash`). Don't unify the two projections. no asset arrays; but `unityAsset`/`unityAssetHash`). Don't unify the two projections.
- A subroom's saved scene loads from `CurrentSave.DataBlob` (`rooms`: `GET /rooms/:id`), - A subroom's saved scene loads from `CurrentSave.DataBlob` (`rooms`: `GET /rooms/:id`),
NOT the flat `DataBlob` on the subroom — a subroom with no `CurrentSave` silently loads NOT the flat `DataBlob` on the subroom — a subroom with no `CurrentSave` silently loads
nothing. The key must be present (null before the first publish); read it via nothing. Read it via `subRoomDataBlob()` so `match`/`auth` instance payloads resolve it
`subRoomDataBlob()` so `match`/`auth` instance payloads resolve it the same way. the same way. It resolves in `attachCurrentSaves`, which every room read goes through:
the PUBLISHED save (`current_save_id`), falling back to the STAGED one
(`staged_save_id`) when nothing has been published yet, and with NEITHER the key is
omitted entirely — a `CurrentSave: null` breaks the client on every endpoint that serves
rooms (not just the save flow — `/rooms/ownedby/me` and friends too). Don't reintroduce
a null in a hand-built subroom payload.
- A room save (`rooms`: `POST …/subrooms/:sid/data`) publishes only when the body says - A room save (`rooms`: `POST …/subrooms/:sid/data`) publishes only when the body says
`AutoPublish: true`; otherwise it STAGES onto `StagedSubRoomDataSaveId` and leaves `AutoPublish: true`; otherwise it STAGES onto `StagedSubRoomDataSaveId` and leaves
`CurrentSave` alone, so players keep loading the last published version until the owner `current_save_id` alone, so players keep loading the last published version until the
posts `…/subrooms/:sid/publish_save` with `subRoomDataSaveId=<id>`. DORMS always owner posts `…/subrooms/:sid/publish_save` with `subRoomDataSaveId=<id>` — except before
the FIRST publish, where `CurrentSave` falls back to the staged save (there is no older
version to keep serving, and a subroom that has been saved shouldn't read as empty).
DORMS always
publish: no publish step exists in the client for them. Saves live in the publish: no publish step exists in the client for them. Saves live in the
`subroom_save` table with globally-unique ids (a bare id has to resolve — `subroom_save` table with globally-unique ids (a bare id has to resolve —
`StagedSubRoomDataSaveId` carries no subroom context), and nothing is overwritten, so `StagedSubRoomDataSaveId` carries no subroom context), and nothing is overwritten, so
`…/saves` is real history and `publish_save` doubles as restore-a-save. `…/saves` is `…/saves` is real history and `publish_save` doubles as restore-a-save. `…/saves` is
auth-gated and CREATOR-only (not co-owners) — it lists unpublished staged saves. There auth-gated and CREATOR-only (not co-owners) — it lists unpublished staged saves. There
is no `GET …/subrooms/:sid/data`; only the POST (the room save) exists on that path. is no `GET …/subrooms/:sid/data`; only the POST (the room save) exists on that path.
- Matchmaking (`match`: `/matchmake/room/:roomId/:subRoomId`) always serves the PUBLISHED - Matchmaking (`match`: `/matchmake/room/:roomId/:subRoomId`) serves whatever
`CurrentSave` blob, creator included. Joining a private instance, the client itself asks `CurrentSave` resolves to — the published save once there is one — to everyone alike,
creator included. Joining a private instance, the client itself asks
the owner whether to load the latest or the published version and resolves it from the the owner whether to load the latest or the published version and resolves it from the
`/subrooms/:sid/saves` list — the matchmake call is identical either way. Don't make `/subrooms/:sid/saves` list — the matchmake call is identical either way. Don't make
this server-side: it would put two people in one instance on different versions. this server-side: it would put two people in one instance on different versions.
+6 -5
View File
@@ -213,9 +213,10 @@ export const SubRoomDataSaveDto = z.object({
* sequence, not per-room); a room's `SubRooms` array is reconstructed on read. * sequence, not per-room); a room's `SubRooms` array is reconstructed on read.
* *
* `CreatorAccountId` starts null on the seeded rooms and is filled in on the first save — * `CreatorAccountId` starts null on the seeded rooms and is filled in on the first save —
* the client NREs on a null one. `CurrentSave` is null until the first save; the flat * the client NREs on a null one. `CurrentSave` is the published save, or the staged one
* `DataBlob`/`RoomDataBlob`/`DataSavedAt` fields are legacy and are NOT what the client * when nothing has been published; with neither it is ABSENT rather than null (a null one
* loads from. * breaks the client's parser). The flat `DataBlob`/`RoomDataBlob`/`DataSavedAt` fields are
* legacy and are NOT what the client loads from.
*/ */
export const SubRoomDto = z.object({ export const SubRoomDto = z.object({
SubRoomId: z.int(), SubRoomId: z.int(),
@@ -231,8 +232,8 @@ export const SubRoomDto = z.object({
.describe('0 Private, 1 Public, 2 Unlisted, 3 Dev_only, 4 Dev_Unlisted — set independently'), .describe('0 Private, 1 Public, 2 Unlisted, 3 Dev_only, 4 Dev_Unlisted — set independently'),
ShouldAutoStageSaves: z.boolean(), ShouldAutoStageSaves: z.boolean(),
StagedSubRoomDataSaveId: z.int().nullable(), StagedSubRoomDataSaveId: z.int().nullable(),
CurrentSave: SubRoomDataSaveDto.nullable().describe( CurrentSave: SubRoomDataSaveDto.optional().describe(
'The latest room save — where the client finds the scene blob. Null until first save' 'The room save the client loads the scene blob from: the published one, falling back to the STAGED one when nothing has been published yet. Omitted entirely (never null) when the subroom has neither'
), ),
DataBlob: z.string().optional().describe('Legacy flat key; the client reads `CurrentSave`'), DataBlob: z.string().optional().describe('Legacy flat key; the client reads `CurrentSave`'),
RoomDataBlob: z.string().optional().describe('Uploaded room-data key; absent until first save'), RoomDataBlob: z.string().optional().describe('Uploaded room-data key; absent until first save'),
+20 -12
View File
@@ -1470,19 +1470,22 @@ describe('rooms endpoints', () => {
expect(coOwnerEnv.value.subRoomDataSave).toMatchObject({ savedByAccountId: 2 }) expect(coOwnerEnv.value.subRoomDataSave).toMatchObject({ savedByAccountId: 2 })
}) })
it('GET /rooms/:id gives every subroom a CurrentSave key (null before the first save)', async () => { it('GET /rooms/:id omits CurrentSave entirely on a never-saved subroom', async () => {
// The client loads a subroom's scene data from CurrentSave and nothing else, so // A null CurrentSave breaks the client's room parser, so an unsaved subroom has to
// the key must be PRESENT — the seeded rooms predate it and have no such field in // carry no such key at all. `in` rather than a value check: absent and null differ
// their stored blob. `in` rather than a value check: absent and null differ here. // here, and null is exactly what this must not be.
// Room 3 is seeded and never saved by another test (room 2 is the save fixture). // Room 3 is seeded and never saved by another test (room 2 is the save fixture).
const room = (await (await SELF.fetch(`${ORIGIN}/rooms/3`)).json()) as { const room = (await (await SELF.fetch(`${ORIGIN}/rooms/3`)).json()) as {
SubRooms: Array<Record<string, unknown>> SubRooms: Array<Record<string, unknown>>
} }
expect(room.SubRooms.length).toBeGreaterThan(0) expect(room.SubRooms.length).toBeGreaterThan(0)
for (const sub of room.SubRooms) { for (const sub of room.SubRooms) {
expect('CurrentSave' in sub).toBe(true) expect('CurrentSave' in sub).toBe(false)
expect(sub.CurrentSave).toBeNull()
} }
// Not just missing from the parsed object — absent from the wire bytes too, which is
// what the client's parser actually reads.
const raw = await (await SELF.fetch(`${ORIGIN}/rooms/3`)).text()
expect(raw).not.toContain('"CurrentSave"')
}) })
it('a real client room-save body stages, and publish_save makes it live', async () => { it('a real client room-save body stages, and publish_save makes it live', async () => {
@@ -1493,7 +1496,7 @@ describe('rooms endpoints', () => {
body: JSON.stringify(body), body: JSON.stringify(body),
}) })
type Sub = { type Sub = {
CurrentSave: Record<string, unknown> | null CurrentSave?: Record<string, unknown>
StagedSubRoomDataSaveId: number | null StagedSubRoomDataSaveId: number | null
} }
const subOf = async () => (await subRoomOf(5, 5)) as unknown as Sub const subOf = async () => (await subRoomOf(5, 5)) as unknown as Sub
@@ -1520,11 +1523,16 @@ describe('rooms endpoints', () => {
}) })
expect(res.status).toBe(200) expect(res.status).toBe(200)
// Room 5 is not a dorm → staged, nothing live yet. // Room 5 is not a dorm → the save is STAGED, not published. Nothing has ever been
// published here, so there is no older version to keep serving: CurrentSave falls
// back to the staged save rather than leaving the subroom reading as never-saved.
const stagedSub = await subOf() const stagedSub = await subOf()
expect(stagedSub.CurrentSave).toBeNull()
const firstId = stagedSub.StagedSubRoomDataSaveId! const firstId = stagedSub.StagedSubRoomDataSaveId!
expect(firstId).toBeGreaterThan(0) expect(firstId).toBeGreaterThan(0)
expect(stagedSub.CurrentSave).toMatchObject({
SubRoomDataSaveId: firstId,
DataBlob: '2026-07-28/f176fc3b-scene',
})
// Publishing makes it what the loader fetches, and clears the staging slot. // Publishing makes it what the loader fetches, and clears the staging slot.
expect((await publish(firstId)).status).toBe(200) expect((await publish(firstId)).status).toBe(200)
@@ -1548,7 +1556,7 @@ describe('rooms endpoints', () => {
// It's on the room read too — that's what the loader actually fetches. // It's on the room read too — that's what the loader actually fetches.
const room = (await (await SELF.fetch(`${ORIGIN}/rooms/5`)).json()) as { const room = (await (await SELF.fetch(`${ORIGIN}/rooms/5`)).json()) as {
SubRooms: Array<{ SubRoomId: number; CurrentSave: { SubRoomDataSaveId: number } | null }> SubRooms: Array<{ SubRoomId: number; CurrentSave?: { SubRoomDataSaveId: number } }>
} }
expect(room.SubRooms.find((s) => s.SubRoomId === 5)!.CurrentSave!.SubRoomDataSaveId).toBe( expect(room.SubRooms.find((s) => s.SubRoomId === 5)!.CurrentSave!.SubRoomDataSaveId).toBe(
firstId firstId
@@ -1693,9 +1701,9 @@ describe('rooms endpoints', () => {
headers: await bearer('1'), headers: await bearer('1'),
}) })
const body = (await res.json()) as { const body = (await res.json()) as {
value: { SubRooms: Array<{ SubRoomId: number; CurrentSave: { SubRoomId: number } | null }> } value: { SubRooms: Array<{ SubRoomId: number; CurrentSave?: { SubRoomId: number } }> }
} }
const clone = body.value.SubRooms.find((s) => s.SubRoomId !== 2 && s.CurrentSave !== null)! const clone = body.value.SubRooms.find((s) => s.SubRoomId !== 2 && s.CurrentSave !== undefined)!
expect(clone).toBeDefined() expect(clone).toBeDefined()
// The copy's save must claim the COPY, or the client resolves it against the source. // The copy's save must claim the COPY, or the client resolves it against the source.
expect(clone.CurrentSave!.SubRoomId).toBe(clone.SubRoomId) expect(clone.CurrentSave!.SubRoomId).toBe(clone.SubRoomId)
+37 -12
View File
@@ -797,7 +797,7 @@ export async function createSubRoom(
LastModeratedSaveModerationState: 0, LastModeratedSaveModerationState: 0,
ShouldAutoStageSaves: true, ShouldAutoStageSaves: true,
// Nothing saved yet — the first room save mints one and points current_save_id // Nothing saved yet — the first room save mints one and points current_save_id
// at it. Until then the subroom reads with `CurrentSave: null`. // at it. Until then the subroom reads with no `CurrentSave` key at all.
}) })
// Refresh the hydrated SubRooms so the returned room includes the one just inserted. // Refresh the hydrated SubRooms so the returned room includes the one just inserted.
await attachSubRooms(db, [room]) await attachSubRooms(db, [room])
@@ -863,10 +863,11 @@ const SUBROOM_COLUMNS = 'sub_room_id, room_id, data, current_save_id, staged_sav
/** /**
* Materialize a subroom row into its client shape, with the columns authoritative. * Materialize a subroom row into its client shape, with the columns authoritative.
* `CurrentSave` is left undefined here and filled in by {@link attachCurrentSaves} — it * `CurrentSave` is left alone here and resolved by {@link attachCurrentSaves} — it lives
* lives in `subroom_save`, and resolving it per row would be a query each. Callers must * in `subroom_save`, and resolving it per row would be a query each. Callers must go
* go through the helpers below so the key is never missing: the client reads the scene * through the helpers below rather than this: the client reads the scene blob from
* blob from `CurrentSave` and nowhere else, so a subroom without one loads nothing. * `CurrentSave` and nowhere else, so a subroom served without one loads nothing, and a
* stale one spread out of the stored blob would point at another subroom's save.
*/ */
const parseSubRoomRow = (row: SubRoomRow): SubRoom => ({ const parseSubRoomRow = (row: SubRoomRow): SubRoom => ({
...(JSON.parse(row.data) as SubRoom), ...(JSON.parse(row.data) as SubRoom),
@@ -903,12 +904,31 @@ const serializeRoom = (room: Room): string => {
return JSON.stringify({ ...rest, Stats: storedStats(stats) }) return JSON.stringify({ ...rest, Stats: storedStats(stats) })
} }
/**
* The save a subroom actually serves as its `CurrentSave`: the published one, or the
* staged one when nothing has been published yet. Null when it has neither.
*/
const liveSaveId = (row: SubRoomRow): number | null => row.current_save_id ?? row.staged_save_id
/** /**
* Fill in each subroom's `CurrentSave` from `subroom_save`, in ONE query for the whole * Fill in each subroom's `CurrentSave` from `subroom_save`, in ONE query for the whole
* batch. Every subroom ends up with the key present — null when it points at no save * batch.
* (never saved) or the pointer dangles — because the client's loader reads it directly.
* *
* `rows` must line up with `subs` positionally; the pointer lives on the row, not the * `current_save_id` (the published save) wins; a subroom that has none FALLS BACK to
* `staged_save_id`, so a subroom whose only save is unpublished still serves that save as
* its `CurrentSave` rather than reading as never-saved. Note what this means for staging:
* before the first publish there is nothing older to keep serving, so the staged work is
* what everyone loads — but once a save has been published, staging behaves as it did and
* players keep loading the published version.
*
* A subroom with neither pointer (never saved, or both dangling) has the key REMOVED
* rather than set to null: a null `CurrentSave` breaks the client's room parser, so an
* unsaved subroom has to look like it has no such field at all. The key is deleted rather
* than merely left unset because the stored blob of a subroom written before saves moved
* to their own table can still carry a `CurrentSave: null` of its own, which
* {@link parseSubRoomRow} would spread straight through.
*
* `rows` must line up with `subs` positionally; the pointers live on the row, not the
* parsed blob. * parsed blob.
*/ */
async function attachCurrentSaves( async function attachCurrentSaves(
@@ -916,7 +936,7 @@ async function attachCurrentSaves(
subs: SubRoom[], subs: SubRoom[],
rows: SubRoomRow[] rows: SubRoomRow[]
): Promise<void> { ): Promise<void> {
const saveIds = [...new Set(rows.map((r) => r.current_save_id).filter((id) => id != null))] const saveIds = [...new Set(rows.map(liveSaveId).filter((id) => id != null))]
const byId = new Map<number, SubRoomDataSave>() const byId = new Map<number, SubRoomDataSave>()
if (saveIds.length > 0) { if (saveIds.length > 0) {
const placeholders = saveIds.map((_, i) => `?${i + 1}`).join(',') const placeholders = saveIds.map((_, i) => `?${i + 1}`).join(',')
@@ -930,8 +950,10 @@ async function attachCurrentSaves(
for (const r of results) byId.set(r.sub_room_data_save_id, parseSubRoomSaveRow(r)) for (const r of results) byId.set(r.sub_room_data_save_id, parseSubRoomSaveRow(r))
} }
subs.forEach((sub, i) => { subs.forEach((sub, i) => {
const id = rows[i]!.current_save_id const id = liveSaveId(rows[i]!)
sub.CurrentSave = id == null ? null : (byId.get(id) ?? null) const save = id == null ? undefined : byId.get(id)
if (save) sub.CurrentSave = save
else delete sub.CurrentSave
}) })
} }
@@ -1302,9 +1324,12 @@ export async function insertSubRoom(
...sub, ...sub,
SubRoomId: subRoomId, SubRoomId: subRoomId,
RoomId: roomId, RoomId: roomId,
CurrentSave: null,
StagedSubRoomDataSaveId: null, StagedSubRoomDataSaveId: null,
} }
// A fresh subroom has no save, so the key is absent rather than null — same rule the
// read path applies (see {@link attachCurrentSaves}). The delete also drops the SOURCE's
// save that the spread carried on a clone; the copy below re-points it at its own row.
delete created.CurrentSave
// The permission overrides follow the copy too — they live in their own table (keyed by // The permission overrides follow the copy too — they live in their own table (keyed by
// the id the caller is cloning FROM), so unlike the rest of the settings they aren't // the id the caller is cloning FROM), so unlike the rest of the settings they aren't
// carried by the blob. A fresh subroom (`createSubRoom`) passes no id and copies nothing. // carried by the blob. A fresh subroom (`createSubRoom`) passes no id and copies nothing.