Skip to content

fix: serialize JSON sidecar writes to stop lost updates - #362

Merged
Darkvader-ship-it merged 2 commits into
PHASE-STELLAR:mainfrom
Mikey-222:fix/p1-store-concurrency
Sep 28, 2026
Merged

fix: serialize JSON sidecar writes to stop lost updates#362
Darkvader-ship-it merged 2 commits into
PHASE-STELLAR:mainfrom
Mikey-222:fix/p1-store-concurrency

Conversation

@Mikey-222

@Mikey-222 Mikey-222 commented Sep 28, 2026

Copy link
Copy Markdown
Contributor

The JSON sidecar stores each did readFile -> JSON.parse -> mutate -> writeFile with no coordination, so concurrent read-modify-write cycles raced and the second writeFile silently discarded the first. Measured on the base commit: 10 concurrent upvotes persisted 1, 50 concurrent appends persisted 1.

Adds lib/json-store.ts: a per-file-path promise-chain lock plus an atomic temp-then-rename write, and collapses the 11 hand-rolled read/write helper pairs onto it. The lock removes lost updates within a process; the rename means a reader never sees a half-written file and a crash cannot truncate a store to invalid JSON.

Also fixes a bug that needs no concurrency at all: checkAndUnlock read the store, let unlockAchievement run its own read-modify-write, then wrote its stale snapshot back over it. It reported the unlock to the caller and sent an "achievement unlocked" notification while persisting unlocked: []. It now does a single locked mutation and notifies after the lock is released.

Verified against a worktree at the base commit: 50 concurrent creates, 10 concurrent upvotes, and the achievement persistence case all fail there and pass here. Adds lib/tests/store-concurrency.test.ts (19 cases) and a test script, since the suite had no runner entry point.

Not fixed, documented in docs/TECHNICAL.md 10.1:

  • The lock is per-process. On Vercel each instance gets its own os.tmpdir() copy of the data root, so cross-instance state still diverges. That needs a shared datastore.
  • The faucet and classic-liq double-claim/double-payment races are check-then-write with the write after an on-chain submit. A file lock cannot make them atomic; they need compare-and-set or an idempotency key.
  • upvoteSignal remains a non-idempotent toggle, so a client retry still reverses the vote.

@Mikey-222
fix: extend the sidecar write lock to the six stores PHASE-STELLAR#361 …
4d87999 
…left unguarded

PHASE-STELLAR#361 fixed lib/signal-store.ts and its commit message noted that the other
six JSON stores "share the same unsafe pattern and are untouched here". This
does that: each one hand-rolled a readFile -> JSON.parse -> mutate -> writeFile
pair with no coordination, so concurrent read-modify-write cycles raced and the
second writeFile discarded the first.

Measured on this branch before the change, 50 concurrent writers:

  follows                 1/50 persisted
  profile saves           1/50 persisted
  checkAndUnlock counters 1/50 accumulated
  world save versions     1 (every writer read version 0)
  profile view counters   1/50 counted
  unlockAchievement       reported success 10/10 for the same unlock

Adds lib/json-store.ts, giving the same three guarantees PHASE-STELLAR#361 applied inline
to signals, and routes the remaining stores through it: serialized mutation,
atomic temp-then-rename replacement, and a JSON parse failure that throws
instead of degrading to {} (degrading meant the next write wiped the store).

Also fixes a bug needing no concurrency at all: checkAndUnlock read the store,
let tryUnlock call unlockAchievement (its own read-modify-write), then wrote
its stale snapshot back. It returned the unlock to its caller and fired an
"achievement unlocked" notification for an achievement that was never
persisted. The counter update and every unlock it triggers now happen in one
locked mutation. Because the lock is not reentrant, unlocks are applied to
the in-flight store rather than re-entering through unlockAchievement.

Two further read-then-write races are now decided under the lock:
claimProfileHandle's alias uniqueness check (two wallets could claim one
handle) and setNotificationPreferences' merge (concurrent updates each started
from the same base and lost a field).

Also fixes a pre-existing runtime bug found while testing: markNarrativeRead
and getReaderProgress passed "readerProgress" to serverDataJsonPath, but the
key is "worldReaderProgress", so FILES[key] was undefined and
path.join(root, undefined) threw. Reader progress was entirely non-functional.
@ts-nocheck on that file is why tsc never caught it.

Scope: the six stores PHASE-STELLAR#361 named. market-store's listings and offers are
already SQLite, so only marketProfileViews and blockList are touched here. The
route-local stores (faucet claims, classic-liq claims, artist profiles, nft
listings) still have the original pattern and are not addressed here.

Tests: lib/__tests__/json-store-concurrency.test.ts, 22 vitest cases, 12 of
which fail against the unpatched stores. Adds no new type errors (163 vs the
164 baseline, all pre-existing on main).

Not addressed, and reported separately:
- The lock is per-process; Vercel instances each hold their own os.tmpdir()
  copy, so cross-instance state still diverges.
- main is broken independently of this change: 163 type errors, plus
  lib/signal-store.ts has a duplicate `let items` declaration that is a hard
  parse error, so that module and both of its test files cannot load.
- 40 of the 43 test files on main use node:test, which `vitest run` cannot
  collect, so `npm test` reports "No test suite found" for all of them.
@Mikey-222
Mikey-222 force-pushed the fix/p1-store-concurrency branch from e6b346d to 4d87999 Compare September 28, 2026 08:02
@drips-wave

drips-wave Bot commented Sep 28, 2026

Copy link
Copy Markdown

@Mikey-222 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits.

You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀

Learn more about application limits

@Darkvader-ship-it
Darkvader-ship-it merged commit c070898 into PHASE-STELLAR:main Sep 28, 2026
0 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment