fix(sdk-commands): serialize local storage writes to prevent server-storage.json corruption - #1545
Conversation
…torage.json corruption The local preview/storage server persisted env/world/player state via an unguarded whole-file read-modify-write on server-storage.json. Concurrent upserts (a scene firing several set() calls on load, multiple tabs, or the storage CLI) interleaved on the event loop, causing lost updates and, with overlapping saves, byte-level corruption that the loader then discarded to defaults. - Run every load->mutate->save through a single in-process FIFO queue so writes apply in order and never race. - Make saveServerStorage atomic (write temp file, then rename) so a crash mid-write can never leave a truncated, unparseable file. - Replace the shared DEFAULT_STORAGE const with a createDefaultStorage() factory so default loads no longer alias (and leak into) each other. Adds runtime-env.spec.ts covering concurrent upserts, cross-bucket writes, atomic-write, and default isolation.
Deploying js-sdk-toolchain with
|
| Latest commit: |
5a06d4b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://23cd1dad.js-sdk-toolchain.pages.dev |
| Branch Preview URL: | https://fix-local-storage-file.js-sdk-toolchain.pages.dev |
Test this pull request
|
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Code Review — PR #1545
PR: fix(sdk-commands): serialize local storage writes to prevent server-storage.json corruption
Branch: fix/local-storage-file → auth-server
Files changed: 3 (+172 −41)
CI: ✅ All checks passing (lint, build, test, docs, CLI E2E, Cloudflare Pages)
Verdict: ✅ APPROVE
This is a well-crafted bug fix that correctly addresses three distinct failure modes in the local dev server storage:
- Lost updates — concurrent read-modify-write cycles now serialized via a promise-chain FIFO queue
- File corruption — writes are now atomic (temp file +
rename) - State leaks — shared
DEFAULT_STORAGEconst replaced with acreateDefaultStorage()factory
The serialize() implementation is correct: writeQueue.then(task, task) ensures the queue never stalls on a rejection, and the tail reassignment (writeQueue = run.then(…)) properly swallows errors for queue continuation while still propagating real results to callers. No memory leak — V8 GCs settled promises in the chain. Read-only operations correctly left outside the lock since atomic rename guarantees readers always see a complete file.
No P0 or P1 issues found. API surface is unchanged — purely behavioral fix with no breaking changes.
Git Style (ADR-6)
- ✅ PR title follows
<type>(<scope>): <summary>format - ✅ Branch follows
fix/<summary>pattern - ✅ Base branch correctly targets
auth-server(notmain) - ℹ️ No issue reference — consider adding
closes #Nif a tracking issue exists
Security Review
No security issues introduced by this PR. The storage path is constructed from constants (no user input in file paths), error messages don't leak secrets, and the atomic write pattern is sound.
Two pre-existing observations in the unchanged storage-service.ts (not introduced by this PR, noted for awareness):
- [P2] Prototype pollution surface — a
PUT /players/__proto__/values/keyrequest would write toObject.prototypesinceaddressis not validated against reserved property names. Low severity given local-dev-only context. - [P2] No runtime type validation —
setEnvValueacceptsvalue: stringat the type level, but the JSON-parsed body isn't validated at runtime.
Findings
[P2] loadServerStorage / saveServerStorage exported without serialization guard
runtime-env.ts:61, 87 — Both functions are exported, allowing external callers to bypass the serialize() queue with a manual load→mutate→save cycle. Currently no consumer does this (the only import in storage-service.ts calls the properly-serialized mutator functions), so there is no active bug. Consider either un-exporting saveServerStorage or adding a @internal JSDoc warning.
[P2] Promise<unknown | undefined> is a no-op union
runtime-env.ts:225, 271 — unknown | undefined collapses to unknown since undefined ⊂ unknown. The | undefined suggests the caller should check for it, but the type doesn't enforce that.
[P2] ensureRuntimeDir swallows mkdir failure
runtime-env.ts:48-56 — If mkdir throws, the error is logged but not re-thrown. Execution continues to writeFile, which fails with a less diagnostic error, hiding the root cause.
[P2] Empty player records not cleaned up after delete
runtime-env.ts:299-313 — deletePlayerValue removes a key but leaves the empty {} object behind. Over many deleted players, empty records accumulate in the JSON file.
[P2] Test coverage gaps
runtime-env.spec.ts — No tests for deleteEnvValue, deleteWorldValue, or deletePlayerValue under concurrency. Also no test for error recovery (task throws → queue continues). The existing tests are solid for the three bugs being fixed.
Consumer Impact
runtime-env.ts is consumed only by storage-service.ts within @dcl/sdk-commands. The module is not publicly exported from the package. Function signatures unchanged — purely internal behavioral fix with no downstream impact.
Review Agents Used
- TypeScript reviewer
- Architecture strategist
- Security sentinel
Reviewed by Jarvis 🤖 · Requested by Gabriel Díaz (<@U03MGHMAJL8>) via Slack
|
|
||
| try { | ||
| await components.fs.writeFile(storagePath, JSON.stringify(data, null, 2)) | ||
| const tmpPath = `${storagePath}.tmp` |
There was a problem hiding this comment.
[P2] The temp file name is deterministic. If two separate dcl start processes targeted the same .runtime-data/ directory, they would clobber each other's temp file. Safe for single-process local dev, but a unique suffix would be more defensive:
| const tmpPath = `${storagePath}.tmp` | |
| const tmpPath = `${storagePath}.${process.pid}.tmp` |
pravusjif
left a comment
pravusjif
left a comment
There was a problem hiding this comment.
Tested with 2 local test scenes for multiplayer server and seem to work OK
…er project
Local preview server storage (server-storage.json) had three issues:
- World values were not namespaced by scene, so previewing different scenes
shared one bucket. World storage is now keyed by scene base coordinates
("x,y"), read from scene.json.
- The file lived inside node_modules/@dcl/sdk-commands, so every SDK upgrade
wiped all local dev progress. It now lives in the project's .runtime-data/
directory (threaded via baseDir), surviving `npm i @dcl/sdk@newer` and keeping
same-base-coord scenes in different projects isolated.
- A legacy flat-format world file was discarded on load. It is now migrated once
at preview-server startup into the currently previewed scene's bucket, so no
local data is lost on the format change.
Reconciles with the upstream serialize() write-lock (#1545): every
read-modify-write still runs under the queue.
3 participants
Bug Description
The local preview/storage dev server (
@dcl/sdk-commands start) persists all runtime state —env,world, andplayers— to a singleserver-storage.jsonvia a whole-file read-modify-write. Concurrent writes were unguarded, so the file could be silently corrupted or lose data.Expected: concurrent storage upserts (e.g. a scene issuing several
set()calls on load) all persist, and the file always stays valid JSON.Actual: overlapping requests interleaved on Node's event loop — later saves clobbered earlier ones (lost updates), overlapping writes could interleave into invalid JSON (which the loader then discarded, resetting everything to defaults), and default (no-file) loads aliased shared objects so state leaked between unrelated reads.
Root Cause
Every mutator did
loadServerStorage()→ mutate in memory →saveServerStorage()with no serialization. Because each stepawaits, two in-flight requests interleave at those yield points:saveServerStoragewrote directly to the real file (no temp+rename), so two concurrent writes could interleave, and a crash mid-write left a truncated file.DEFAULT_STORAGEwas a shared const;{ ...DEFAULT_STORAGE }shallow-copied it, so every default load shared the same nestedenv/world/playersobjects.Type of Change
Fix Description
load → mutate → savecycle through a single in-process FIFO queue (serialize()), so writes apply in issue order and never race.saveServerStorageatomic: write a temp file, thenrenameover the target — a crash mid-write can no longer leave an unparseable file.DEFAULT_STORAGEconst with acreateDefaultStorage()factory so default loads no longer alias each other.How to Reproduce (Before Fix)
sdk-commands starta scene that issues severalplayer/world/envset()calls on load (or drive concurrentstorage ... set --target http://localhost:<port>).<@dcl/sdk-commands>/.runtime-data/server-storage.json.How to Verify (After Fix)
node_modules/.bin/jest --forceExit --testPathPatterns='test/sdk-commands/commands/start/runtime-env'— the new concurrency/atomicity/isolation tests pass.node_modules/.bin/jest --forceExit --testPathPatterns='test/sdk-commands/commands/start'— full start suite green (33/33).Impact Assessment
Regression Risk
Behavior/API unchanged — same functions, return types, and HTTP endpoints; purely a robustness change.
Writes are now serialized in-process; a single hung executor would delay subsequent writes (bounded to local dev use).
Checklist
Related Issues