fix(sdk-commands): serialize local storage writes to prevent server-storage.json corruption

AGENTS.md

@@ -89,6 +89,7 @@ Asset-packs injection is gated behind `isEditorScene` (requires `assets/scene/ma
 
 - A newer local npm rewrites committed lockfiles with `"peer": true` / `"dev": true` metadata churn during installs (root and per-package lockfiles). Revert lockfile changes unrelated to your dependency change before committing.
 - `make format` runs prettier over the whole repo, including paths CI's `make lint` does not check (`test/`, `scripts/`, dot-files), where HEAD may carry drift — a blind `make format` can dirty dozens of unrelated files. Check your own files, revert the rest.
+- **PR base branch:** this repo runs long-lived integration branches (e.g. `auth-server`) that are far ahead of `main`. Open a PR against the branch your feature branch was cut from, not `main` — targeting `main` sweeps in hundreds of unrelated files. Find the real base with `git branch -a --contains HEAD~` or by checking which branch gives a clean `git diff <base>...HEAD`; when working from `auth-server`, point the PR at `auth-server`.
 
 ## Code conventions
 

packages/@dcl/sdk-commands/src/commands/start/server/runtime-env.ts

@@ -16,10 +16,29 @@ export interface ServerStorage {
   players: Record<string, Record<string, unknown>>
 }
 
-const DEFAULT_STORAGE: ServerStorage = {
-  env: {},
-  world: {},
-  players: {}
+function createDefaultStorage(): ServerStorage {
+  return {
+    env: {},
+    world: {},
+    players: {}
+  }
+}
+
+let writeQueue: Promise<unknown> = Promise.resolve()
+
+/**
+ * Serializes read-modify-write cycles against server-storage.json. The entire
+ * load→mutate→save must run under one lock: two handlers that each load the same
+ * snapshot would otherwise lose one update, and two concurrent saves would interleave
+ * their writes into a corrupt file.
+ */
+function serialize<T>(task: () => Promise<T>): Promise<T> {
+  const run = writeQueue.then(task, task)
+  writeQueue = run.then(
+    () => undefined,
+    () => undefined
+  )
+  return run
 }
 
 /**
@@ -45,21 +64,20 @@ export async function loadServerStorage(components: Pick<CliComponents, 'fs' | '
   try {
     const exists = await components.fs.fileExists(storagePath)
     if (!exists) {
-      return { ...DEFAULT_STORAGE }
+      return createDefaultStorage()
     }
 
     const content = await components.fs.readFile(storagePath, 'utf-8')
     const parsed = JSON.parse(content) as Partial<ServerStorage>
 
-    // Merge with defaults to ensure all keys exist
     return {
       env: parsed.env ?? {},
       world: parsed.world ?? {},
       players: parsed.players ?? {}
     }
   } catch (error) {
     components.logger.error(`Failed to load ${SERVER_STORAGE_FILE}: ${error}`)
-    return { ...DEFAULT_STORAGE }
+    return createDefaultStorage()
   }
 }
 
@@ -74,7 +92,9 @@ export async function saveServerStorage(
   const storagePath = path.join(RUNTIME_DATA_DIR, SERVER_STORAGE_FILE)
 
   try {
-    await components.fs.writeFile(storagePath, JSON.stringify(data, null, 2))
+    const tmpPath = `${storagePath}.tmp`
+    await components.fs.writeFile(tmpPath, JSON.stringify(data, null, 2))
+    await components.fs.rename(tmpPath, storagePath)
   } catch (error) {
     components.logger.error(`Failed to save ${SERVER_STORAGE_FILE}: ${error}`)
     throw error
@@ -163,23 +183,27 @@ export async function setEnvValue(
   key: string,
   value: string
 ): Promise<void> {
-  const storage = await loadServerStorage(components)
-  storage.env[key] = value
-  await saveServerStorage(components, storage)
+  return serialize(async () => {
+    const storage = await loadServerStorage(components)
+    storage.env[key] = value
+    await saveServerStorage(components, storage)
+  })
 }
 
 /**
  * Deletes a runtime environment variable.
  * Returns true if key existed and was deleted, false otherwise.
  */
 export async function deleteEnvValue(components: Pick<CliComponents, 'fs' | 'logger'>, key: string): Promise<boolean> {
-  const storage = await loadServerStorage(components)
-  if (!(key in storage.env)) {
-    return false
-  }
-  delete storage.env[key]
-  await saveServerStorage(components, storage)
-  return true
+  return serialize(async () => {
+    const storage = await loadServerStorage(components)
+    if (!(key in storage.env)) {
+      return false
+    }
+    delete storage.env[key]
+    await saveServerStorage(components, storage)
+    return true
+  })
 }
 
 /**
@@ -211,9 +235,11 @@ export async function setWorldValue(
   key: string,
   value: unknown
 ): Promise<void> {
-  const storage = await loadServerStorage(components)
-  storage.world[key] = value
-  await saveServerStorage(components, storage)
+  return serialize(async () => {
+    const storage = await loadServerStorage(components)
+    storage.world[key] = value
+    await saveServerStorage(components, storage)
+  })
 }
 
 /**
@@ -224,13 +250,15 @@ export async function deleteWorldValue(
   components: Pick<CliComponents, 'fs' | 'logger'>,
   key: string
 ): Promise<boolean> {
-  const storage = await loadServerStorage(components)
-  if (!(key in storage.world)) {
-    return false
-  }
-  delete storage.world[key]
-  await saveServerStorage(components, storage)
-  return true
+  return serialize(async () => {
+    const storage = await loadServerStorage(components)
+    if (!(key in storage.world)) {
+      return false
+    }
+    delete storage.world[key]
+    await saveServerStorage(components, storage)
+    return true
+  })
 }
 
 /**
@@ -254,12 +282,14 @@ export async function setPlayerValue(
   key: string,
   value: unknown
 ): Promise<void> {
-  const storage = await loadServerStorage(components)
-  if (!storage.players[address]) {
-    storage.players[address] = {}
-  }
-  storage.players[address][key] = value
-  await saveServerStorage(components, storage)
+  return serialize(async () => {
+    const storage = await loadServerStorage(components)
+    if (!storage.players[address]) {
+      storage.players[address] = {}
+    }
+    storage.players[address][key] = value
+    await saveServerStorage(components, storage)
+  })
 }
 
 /**
@@ -271,11 +301,13 @@ export async function deletePlayerValue(
   address: string,
   key: string
 ): Promise<boolean> {
-  const storage = await loadServerStorage(components)
-  if (!storage.players[address] || !(key in storage.players[address])) {
-    return false
-  }
-  delete storage.players[address][key]
-  await saveServerStorage(components, storage)
-  return true
+  return serialize(async () => {
+    const storage = await loadServerStorage(components)
+    if (!storage.players[address] || !(key in storage.players[address])) {
+      return false
+    }
+    delete storage.players[address][key]
+    await saveServerStorage(components, storage)
+    return true
+  })
 }

test/sdk-commands/commands/start/runtime-env.spec.ts

@@ -0,0 +1,98 @@
+import {
+  loadServerStorage,
+  saveServerStorage,
+  setEnvValue,
+  setWorldValue,
+  setPlayerValue,
+  getPlayerValue
+} from '../../../../packages/@dcl/sdk-commands/src/commands/start/server/runtime-env'
+
+/**
+ * In-memory stand-in for the `.runtime-data/` directory that runtime-env reads and
+ * writes. Async fns yield at each `await`, so concurrent read-modify-write cycles
+ * interleave exactly as they would on Node's event loop. runtime-env derives the
+ * storage path from its own package location, so it is learned from the first access.
+ */
+function makeComponents(initialFile?: string) {
+  const files = new Map<string, string>()
+  let mainPath = ''
+  const learn = (filePath: string) => {
+    if (filePath.endsWith('.tmp')) return
+    mainPath = filePath
+    if (initialFile !== undefined && !files.has(filePath)) files.set(filePath, initialFile)
+  }
+  const fs = {
+    fileExists: jest.fn(async (filePath: string) => {
+      learn(filePath)
+      return files.has(filePath)
+    }),
+    readFile: jest.fn(async (filePath: string) => files.get(filePath) ?? ''),
+    directoryExists: jest.fn(async () => true),
+    mkdir: jest.fn(async () => undefined),
+    writeFile: jest.fn(async (filePath: string, content: string) => {
+      files.set(filePath, content)
+    }),
+    rename: jest.fn(async (from: string, to: string) => {
+      files.set(to, files.get(from)!)
+      files.delete(from)
+      learn(to)
+    })
+  }
+  const logger = { debug: jest.fn(), error: jest.fn(), info: jest.fn(), log: jest.fn(), warn: jest.fn() }
+  return { components: { fs, logger } as any, fs, logger, readMain: () => files.get(mainPath) }
+}
+
+describe('runtime-env concurrent write safety', () => {
+  it('does not lose player upserts issued concurrently', async () => {
+    const { components } = makeComponents(JSON.stringify({ env: {}, world: {}, players: { '0xabc': {} } }))
+
+    const keys = Array.from({ length: 20 }, (_, i) => `k${i}`)
+    await Promise.all(keys.map((key, i) => setPlayerValue(components, '0xabc', key, i)))
+
+    for (let i = 0; i < keys.length; i++) {
+      expect(await getPlayerValue(components, '0xabc', keys[i])).toBe(i)
+    }
+  })
+
+  it('does not lose concurrent writes across the env/world/player buckets', async () => {
+    const { components, readMain } = makeComponents(JSON.stringify({ env: {}, world: {}, players: {} }))
+
+    await Promise.all([
+      setEnvValue(components, 'FOO', 'bar'),
+      setWorldValue(components, 'score', 42),
+      setPlayerValue(components, '0xabc', 'coins', 7)
+    ])
+
+    const stored = JSON.parse(readMain()!)
+    expect(stored.env).toEqual({ FOO: 'bar' })
+    expect(stored.world).toEqual({ score: 42 })
+    expect(stored.players).toEqual({ '0xabc': { coins: 7 } })
+  })
+})
+
+describe('runtime-env atomic writes', () => {
+  it('writes a temp file and renames it over the target', async () => {
+    const { components, fs, readMain } = makeComponents()
+
+    await setEnvValue(components, 'FOO', 'bar')
+
+    const writtenPath: string = fs.writeFile.mock.calls[0][0]
+    expect(writtenPath).toMatch(/server-storage\.json\..+/)
+    expect(fs.rename).toHaveBeenCalledWith(writtenPath, expect.stringMatching(/server-storage\.json$/))
+    expect(JSON.parse(readMain()!).env).toEqual({ FOO: 'bar' })
+  })
+})
+
+describe('runtime-env default isolation', () => {
+  it('does not leak state between default (no-file) loads', async () => {
+    const { components } = makeComponents()
+
+    const a = await loadServerStorage(components)
+    a.env.LEAK = 'yes'
+    a.players.someone = { x: 1 }
+
+    const b = await loadServerStorage(components)
+    expect(b.env).toEqual({})
+    expect(b.players).toEqual({})
+  })
+})