Skip to content

fix(signals): stop losing writes on concurrent signal mutations - #361

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

fix(signals): stop losing writes on concurrent signal mutations#361
Darkvader-ship-it merged 2 commits into
PHASE-STELLAR:mainfrom
Chidubemkingsley:fix/signal-store-concurrency

Conversation

@Chidubemkingsley

@Chidubemkingsley Chidubemkingsley commented Sep 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

The signal store did readFile -> JSON.parse -> mutate -> writeFile on a shared JSON sidecar with no locking and no version. Under concurrency every writer read the same snapshot and the last writeFile won, so writers silently erased each other. Measured with 50 concurrent writers: 49 of 50 signals, upvotes, and replies were lost.

Three layers, all in lib/signal-store.ts:

  • Serialized read-modify-write via a per-file promise queue, so two mutations cannot both read the same snapshot.
  • Atomic replacement: write to a sibling .tmp file, then rename(2), so readers never observe a half-flushed file and a crash mid-write cannot corrupt the live store.
  • A JSON parse failure now throws instead of degrading to {}. Returning {} on a parse error meant the next write overwrote every record.

Each Signal gains an integer version, bumped on every mutation:

  • GET /api/signals/[id] publishes it as an ETag.
  • POST /api/signals/[id] accepts an optional If-Match and returns 409 with current_version plus the fresh ETag on a stale value.
  • POST /api/signals/[id]/replies accepts an optional parent_version and returns 409 on a stale value.
  • Conflicts log under the signals.version_conflict event.

Both version inputs are optional, so clients that omit them keep the previous last-writer-wins behaviour. Records written before this change have no version and are normalized to 1 on read, so there is no migration step.

Adds the project's first test suite (vitest): 13 tests covering 50-concurrent-writer create/upvote/reply, CAS races, corrupt-store refusal, legacy version backfill, and version headers. Verified the concurrency tests fail against the old store before passing against the new one.

Residual limitation, documented in docs/TECHNICAL.md 5.3: the queue is per-process, so two Vercel instances can still interleave. The other six stores (follow, profile, market, notification, achievement, narrative-world) share the same unsafe pattern and are untouched here.

Chidubemkingsley and others added 2 commits September 27, 2026 22:28
@Chidubemkingsley
fix(signals): stop losing writes on concurrent signal mutations
6413db9 
The signal store did readFile -> JSON.parse -> mutate -> writeFile on a
shared JSON sidecar with no locking and no version. Under concurrency
every writer read the same snapshot and the last writeFile won, so
writers silently erased each other. Measured with 50 concurrent
writers: 49 of 50 signals, upvotes, and replies were lost.

Three layers, all in lib/signal-store.ts:

- Serialized read-modify-write via a per-file promise queue, so two
  mutations cannot both read the same snapshot.
- Atomic replacement: write to a sibling .tmp file, then rename(2), so
  readers never observe a half-flushed file and a crash mid-write
  cannot corrupt the live store.
- A JSON parse failure now throws instead of degrading to {}. Returning
  {} on a parse error meant the next write overwrote every record.

Each Signal gains an integer version, bumped on every mutation:

- GET /api/signals/[id] publishes it as an ETag.
- POST /api/signals/[id] accepts an optional If-Match and returns 409
  with current_version plus the fresh ETag on a stale value.
- POST /api/signals/[id]/replies accepts an optional parent_version
  and returns 409 on a stale value.
- Conflicts log under the signals.version_conflict event.

Both version inputs are optional, so clients that omit them keep the
previous last-writer-wins behaviour. Records written before this change
have no version and are normalized to 1 on read, so there is no
migration step.

Adds the project's first test suite (vitest): 13 tests covering
50-concurrent-writer create/upvote/reply, CAS races, corrupt-store
refusal, legacy version backfill, and version headers. Verified the
concurrency tests fail against the old store before passing against the
new one.

Residual limitation, documented in docs/TECHNICAL.md 5.3: the queue is
per-process, so two Vercel instances can still interleave. The other
six stores (follow, profile, market, notification, achievement,
narrative-world) share the same unsafe pattern and are untouched here.
@Darkvader-ship-it
Merge branch 'main' into fix/signal-store-concurrency
dbec6e4
@drips-wave

drips-wave Bot commented Sep 28, 2026

Copy link
Copy Markdown

@Chidubemkingsley 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 2e2f64e into PHASE-STELLAR:main Sep 28, 2026
1 of 5 checks passed
Mikey-222 added a commit to Mikey-222/Phase that referenced this pull request Sep 28, 2026
@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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment