Skip to content

fix(server): updateRow per-key merge + updateInstanceConfig transaction (CWD-3) - #322

Merged
eliotlim merged 5 commits into
mainfrom
fix/cwd3-merge-asymmetry
Aug 14, 2026
Merged

fix(server): updateRow per-key merge + updateInstanceConfig transaction (CWD-3)#322
eliotlim merged 5 commits into
mainfrom
fix/cwd3-merge-asymmetry

Conversation

@eliotlim

Copy link
Copy Markdown
Collaborator

Problem

Two server-side lost-update bugs, including a live data-loss path: updateRow replaced the whole properties jsonb blob, so single-key writers — MCP set_db_cell (packages/mcp/src/server.ts:1718) and the AI bridge (aiBridge.ts:410) — wiped every other cell in the row on each write. updateInstanceConfig was an untransacted read-modify-write that lost concurrent keys. Board: CWD-3 (epic OB-76).

Solution

updateRow now locks the row (SELECT … FOR UPDATE, same predicate as the UPDATE, in-transaction) and merges per-key — null deletes, undefined skipped, absent preserved — matching what every existing caller already assumed. updateInstanceConfig materializes + locks the singleton settings row and merges inside one transaction (mirrors the claimOwnership pattern). No API shape change. The known null-semantics divergence vs setPageProperties (stores null vs deletes) is documented at both sites; alignment lands with CWD-2's guarded rewrite.
Key file: packages/server/src/store.ts. New deps: none.

Before / After

No visual change — server write semantics.

Behavior Before After
MCP set_db_cell / AI bridge single-cell write Wiped all other cells in the row Other cells preserved (route-level regression test)
Concurrent row-property writes, different keys Second write clobbered the first Both survive (PGlite FIFO test + real-Postgres FOR UPDATE test, run green against postgres:16.9)
{properties: null} PATCH 200 (stored jsonb null) 200, no-op (was briefly a 500 mid-review; guarded + tested)
{properties: {}} PATCH Wiped all properties No-op (pinned by test, documented)
Concurrent instance-config updates Lost keys (untransacted RMW) Merge in txn under lock

Test procedure

pnpm --filter @book.dev/server test -- src/mergeAsymmetry.test.ts        # PGlite path
OPENBOOK_TEST_DATABASE_URL=postgres://…:55433/postgres \
  npx vitest run src/mergeAsymmetry.test.ts                              # unlocks the FOR UPDATE test (10/10)
pnpm verify

Operational notes

None — no schema change, no config. The instance-config authorization TOCTOU at the route layer (pre-existing) is deliberately deferred to CWD-2's expectedRev CAS and recorded as its acceptance criterion. When this merges after CWD-11, the two now-fixed harness races flip from test.fails to test (flip commit included at merge time).


Verify: green (full pnpm verify; real-Postgres lock test 10/10). Reviews cleared: code (Quinn — pre-endorsed conditional on the null-guard, applied in aa81ac9 with all touch-ups; lock/deadlock analysis clean on both backends).

🤖 Generated with Claude Code

https://claude.ai/code/session_018RUicirLRuuZvjJN13N6yN

treat null property bags and undefined property values as no-ops, document the intentional null
divergence, and cover store, local-client, http, pglite fifo, and optional postgres concurrency.

this repairs a live data-loss bug: single-key writers previously wiped all other row cells:
mcp set_db_cell (packages/mcp/src/server.ts:1718) and the ui ai bridge
(packages/ui/src/lib/aiBridge.ts:410).
@vercel

vercel Bot commented Aug 14, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
app.book.pub Ready Ready Preview Aug 14, 2026 2:48pm

Request Review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant