Skip to content

Fix false conflicts when serverOnlyField is absent from stored doc#403

Open
pubkey wants to merge 3 commits intomasterfrom
claude/fix-conflict-handling-bug-nIgZn
Open

Fix false conflicts when serverOnlyField is absent from stored doc#403
pubkey wants to merge 3 commits intomasterfrom
claude/fix-conflict-handling-bug-nIgZn

Conversation

@pubkey
Copy link
Copy Markdown
Owner

@pubkey pubkey commented Apr 14, 2026

This PR contains:

  • A BUGFIX
  • IMPROVED TESTS

Describe the problem you have without this PR

During replication push, when a serverOnlyField is absent from the stored server document (e.g., because the field is optional and was never set), the push endpoint would merge a null value for that field into the assumedMasterState before calling masterWrite. This caused masterWrite's isEqual check to fail because the stored masterState had no such property, resulting in a false conflict that silently reverted the client's update.

The Fix

Modified mergeServerDocumentFieldsMonad in src/plugins/server/helper.ts to:

  • Only copy server-only fields from serverDoc if they actually exist on that document
  • Delete the field from the merged result if it doesn't exist on serverDoc, rather than setting it to null
  • This ensures the merged document has the same key set as the stored master state, preventing false conflicts during the isEqual check

Tests

  • Added comprehensive test case 'should not produce false conflicts when a serverOnlyField is absent from the stored document' in endpoint-replication.test.ts that reproduces the bug scenario
  • Updated existing unit tests in server.test.ts to verify the fix:
    • 'should not add the field when serverDoc lacks it' - verifies the field is absent, not null
    • 'should strip client-sent values for server-only fields missing on serverDoc' - ensures malicious clients cannot inject values for missing server-only fields
    • 'should not produce extra keys when serverDoc is missing the field' - verifies key count matches between merged and master state
  • Added performance test in performance.test.ts to measure replication throughput with server-only fields

Todos

  • Tests
  • Changelog

https://claude.ai/code/session_01XBbmafik7iAy5Hs7Zw9kQR

…ng on stored doc

`mergeServerDocumentFieldsMonad` wrote a `null` value for server-only fields
that were absent from the stored server document. When the push endpoint
later called `masterWrite`, the merged `assumedMasterState` had an extra
key compared to the stored master state, so the `isEqual` check reported a
conflict that did not actually exist and the client's update was silently
reverted.

The merge helper now deletes the property in that case so the merged row
matches the stored master state. Added an end-to-end replication test and
a performance test covering the serverOnlyFields path.
@pubkey
Copy link
Copy Markdown
Owner Author

pubkey commented Apr 15, 2026

@copilot do not add performance tests in this pr

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 15, 2026

@copilot do not add performance tests in this pr

Removed the performance-test additions from this PR in commit beafb0b (deleted test/unit/performance.test.ts and removed its import from test/unit.test.ts).

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.

3 participants