fix(property-changeset): Prevent duplicate array insertions during rebase

[bhavin121]

Summary

• Fixes the Invalid Array Length crash (RangeError: Invalid array length) that occurs during array changeset rebasing in PropertyDDS concurrent editing scenarios.

• The root cause is duplicate INSERT operations being emitted by _rebaseArrayChangeSetForProperty. This fix adds an advanceRebaseIteratorB skip mechanism that prevents a paired INSERT from being emitted twice — once from the splitOverlapping special case and again from the iterator's standalone yield.

• Mirrors the existing advanceIteratorB skip logic already proven correct in _performApplyAfterOnPropertyArray.

Problem

When two clients concurrently edit the same array — one replacing elements (remove + insert at the same position) and another inserting elements — the rebased changeset contains duplicate insertions. These duplicate INSERT operations at the same position then flow into mergeWithLastIfPossible, where two Map entries end up pointing to the same metaInformation object (and thus the same rebasedRemoveInsertRanges array). The merge loop pushes elements from nextRange into previousRange, but since they are the same array reference, each push grows the array being iterated — creating an infinite loop that runs until the array exceeds JavaScript's maximum size (~4.3 billion elements), crashing with:

RangeError: Invalid array length
  at Array.push (<anonymous>)
  at mergeWithLastIfPossible (array.ts)
  at pushOp (array.ts)
  at applyRebaseSegment (array.ts)
  at _rebaseArrayChangeSetForProperty (array.ts)

This crashes SharedPropertyTree.processCore, disconnecting the client from the collaborative session.

Reproduction scenario:

Initial array: ["A", "B", "C", "D", "E"]
Client 1 (local):  replace first 3 → { remove: [[0, ["A","B","C"]]], insert: [[0, ["X","Y","Z"]]] }

Client 2 (remote): insert "P" at 0, "Q" at 4 → { insert: [[0, ["P"]], [3, ["Q"]]] }
Expected rebased output: { remove: [[1, ["A","B","C"]]], insert: [[1, ["X","Y","Z"]]] }

Actual rebased output:   { remove: [[1, ["A","B","C"]]], insert: [[1, ["X","Y","Z"]], [1, ["X","Y","Z"]]] }

^^^^^^^^^^^^^^^^^^

duplicate INSERT

→ triggers infinite loop in mergeWithLastIfPossible

→ crashes with "Invalid array length"

Root Cause

In _rebaseArrayChangeSetForProperty (array.ts), when the splitOverlapping special case emits a paired INSERT (because its position fell behind the advancing REMOVE), it sets the segment flag to completeB. This triggers iteratorB.next(), which yields the same INSERT again (since the iterator only peeked at it, not consumed it). Without a skip mechanism, the INSERT gets emitted a second time.

The duplicate INSERT at the same position is the upstream root cause. The infinite loop in mergeWithLastIfPossible (via shared metaInformation references) is a downstream consequence — fixing the duplicate emission eliminates the crash entirely without needing any changes to mergeWithLastIfPossible or getRangeForAppliedOperation.

Fix

Added ~15 lines of new code — an advanceRebaseIteratorB closure with a two-stage skip:

1. Stage 1 (record): When the just-emitted segment matches the paired removeInsertOperation (by reference equality), record it for skipping and do not advance the iterator.

2. Stage 2 (skip): On the next advancement, call next(), detect the duplicate INSERT via the recorded reference, call next() again to skip past it, then clear the skip state.

Replaced the two direct iteratorB.next() call sites (completeB/completeBpartOfA and completeAcompleteB) with advanceRebaseIteratorB().

Changed File

File	Change
experimental/PropertyDDS/packages/property-changeset/src/changeset_operations/array.ts	Added advanceRebaseIteratorB helper in _rebaseArrayChangeSetForProperty; replaced raw iteratorB.next() calls at the completeB/completeBpartOfA and completeAcompleteB advancement sites

Test Plan

• Run existing array.spec.ts rebase tests to verify no regressions

• Add a test case for the reproduction scenario: concurrent replace (remove+insert at position 0) vs. multi-position insert, verifying the rebased output contains exactly one copy of the INSERT and no Invalid array length crash

• Verify convergence: apply Client 2's change then the rebased Client 1 change, and confirm the final array matches the expected state

[bhavin121]

Hi @anthony-murphy, When you get a chance, could you please take a look at this PR?

[bhavin121]

Hi @anthony-murphy, any update on this PR.

[CraigMacomber]

PR description includes:

Add a test case for the reproduction scenario: concurrent replace (remove+insert at position 0) vs. multi-position insert, verifying the rebased output contains exactly one copy of the INSERT and no Invalid array length crash

That sound like a good idea (a regression test for this bug). There however does not seem to be one included in this PR.

Verify convergence: apply Client 2's change then the rebased Client 1 change, and confirm the final array matches the expected state

I'm not sure if that's intended to be part of the above mentioned test (which is missing) or some separate verification, but might be good to ensure that is done as well (and clarified in the PR description)

[CraigMacomber]

These casts to any are extremally non-type safe.

Please avoid introducing new code which defeats type safety.

If it is impractical to be type safe here, please document why that is the case, why its actually safe, and do something less unsafe (like cast to some specific type which exposes removeInsertOperation with the correct tying instead of it also being just any.

[bhavin121]

Thanks for catching this. I've replaced (opB as any) with (opB as NoneNOPOperation) in the new advanceRebaseIteratorB, and also updated the pre-existing casts in advanceIteratorB (line ~1568) to match.

NoneNOPOperation is the union RemoveOperation | InsertOperation | ModifyOperation — all three subtypes define removeInsertOperation?, so the cast is type-safe. The only member of GenericOperation that lacks this property is NOPOperation, but in that case removeInsertOperation evaluates to undefined, and the if condition short-circuits harmlessly at the first check.

Commit: 87206eb

[CraigMacomber]

This code is relatively complex, and, given the bug, somewhat fragile. I think it would be a good idea to include some comments on these new locals (skipIteratorBOperation, opB, advanceRebaseIteratorB) to explain their semantics.

Without such details, it's quite hard to know if this code is correct, both for current reviewers, and future readers and editors of this code.

[bhavin121]

Agreed — the two-stage skip mechanism is non-obvious without context. I've added doc comments on all three locals:

• opB: Reference to iteratorB's mutable operation descriptor (shared object updated in-place on each next() call).
• skipIteratorBOperation: Two-stage skip state — records the INSERT's operation reference when splitOverlapping's special case has already emitted it, so the next advancement knows to skip past the duplicate yield.
• advanceRebaseIteratorB: The skip-aware advancement helper. Stage 1 records, Stage 2 skips.

I've also added equivalent comments to the existing advanceIteratorB in _performApplyAfterOnPropertyArray for consistency, and noted that the two closures are intentionally kept separate (they capture different local scopes).

Commit: 87206eb

[github-actions]

Hi! Thank you for opening this PR. Want me to review it?

Based on the diff (121 lines, 2 files), I've queued these reviewers:

• Correctness — logic errors, race conditions, lifecycle issues
• Security — vulnerabilities, secret exposure, injection
• API Compatibility — breaking changes, release tags, type design
• Performance — algorithmic regressions, memory leaks
• Testing — coverage gaps, hollow tests

How this works

• Adjust the reviewer set by ticking/unticking boxes above. Reviewer toggles alone don't trigger anything.

• Tick Start review below to dispatch the review fleet.

• After review finishes, tick Start review again to request another run — it auto-resets after each dispatch.

• This comment updates as new commits land; your reviewer selections are preserved.

• Start review

[Copilot]

Pull request overview

Fixes an array changeset rebasing issue in PropertyDDS where a paired INSERT (from a remove+insert “replace” pattern) could be emitted twice, leading to downstream crashes (e.g., RangeError: Invalid array length) in concurrent editing scenarios.

Changes:

• Introduces advanceRebaseIteratorB() in _rebaseArrayChangeSetForProperty to skip a duplicated iterator yield when splitOverlapping() already emitted the paired INSERT.
• Replaces direct iteratorB.next() advancement sites with the new helper to ensure consistent skip behavior.

[bhavin121]

Good observation. The two helpers (advanceIteratorB and advanceRebaseIteratorB) are structurally identical, but they close over different local variables in different function scopes — extracting a shared helper would require passing ~6 mutable references via a parameter object, which would add complexity and indirection for minimal benefit (~15 lines of code).

Additionally, the two functions operate in different contexts (apply-after vs. rebase) that could theoretically diverge in the future. Refactoring advanceIteratorB, which has been stable for years, carries more risk than the duplication itself.

I've added a comment in both helpers noting the relationship and explaining why they are kept separate.

[bhavin121]

Added two regression tests in array.spec.ts:

1. "Concurrent replace (remove+insert) vs multi-position insert should not produce duplicate inserts" — directly verifies that the rebased output contains exactly one INSERT (not two), and that positions are correctly shifted.

2. "Convergence: concurrent replace (remove+insert) vs multi-position insert" — uses the existing testRebasedApplies helper to verify that both paths (local-then-delta vs. remote-then-rebased-local) converge to the same final state.

Both tests pass (ESM and CJS). All 268 existing tests continue to pass with no regressions.

Commit: 87206eb