fix(property-changeset): don't throw on a duplicate identical insert into an indexed collection#27702
Open
arafat-java wants to merge 1 commit into
Open
Conversation
…into an indexed collection Merging a ChangeSet into an indexed (set/map-typed) collection throws "CS-003: Internal error: Added an already existing entry" whenever the incoming insert's key already has an insert recorded in the base ChangeSet — even when the two inserts are identical, e.g. from a duplicated or replayed op. Since this runs on the normal op-processing path, a duplicate op can crash-loop a document's summarizer. Treat an insert that exactly matches the already-recorded insert for the same key as a safe no-op, for both primitive and typed/polymorphic collections. A mismatched value for the same key is still a genuine conflict and continues to throw.
|
Azure Pipelines: 1 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Azure Pipelines: 1 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
|
Hi! Thank you for opening this PR. Want me to review it? Based on the diff (68 lines, 3 files), I've queued these reviewers:
How this works
|
dannimad
requested changes
Jul 20, 2026
dannimad
left a comment
dannimad
left a comment
Contributor
There was a problem hiding this comment.
Do not merge until the release process is complete
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
ChangeSetIndexedCollectionFunctions's merge logic throwsCS-003: Internal error: Added an already existing entry: <key>whenever an incoming insert's key already has an insert recorded in the base ChangeSet — even when the two inserts are identical (e.g. from a duplicated or replayed op). Since this runs on the normal op-processing path (SharedPropertyTree._applyRemoteChangeSet→processMessage), a duplicate op can put a document's summarizer into a crash-loop.This PR treats an insert that exactly matches the already-recorded insert for the same key as a safe no-op, for both primitive and typed/polymorphic collections. A mismatched value for the same key is still treated as a genuine conflict and continues to throw.
To reproduce: apply a ChangeSet insert for a key to a base ChangeSet that already has an identical insert for that same key (see the new test cases in
indexedCollection.spec.tsfor a minimal repro). Before this change that throws; after, it's a no-op.Reviewer Guidance
I haven't been able to run this package's own test suite locally yet (environment setup issue unrelated to this change — Azure DevOps Artifacts feed auth). The two new test cases closely follow this file's existing patterns, and the equivalent fix (applied to the compiled/patched form of this exact function) has been running against a real local Fluid server with 113 passing tests in a downstream consumer — but I'd like CI/a reviewer to confirm this package's own suite passes before merging. Opening as a draft until I can confirm locally.