Skip to content

Commit ddaa356

Browse files
fix(tree): handle redundant sequencing of local commit (#25544)
## Description Fixes a bug in the scenario where a commit from a child shared branch is concurrently merged into the main shared branch by different peers. The peer whose merged commits are not sequenced first had a mistaken expectation that it should have a commit on its main local branch for each commit that is sequenced by the service if that commit came from this peer. This is no longer a valid expectation. ## Breaking Changes None
1 parent 2a8c2a8 commit ddaa356

File tree

1 file changed

+10
-4
lines changed

1 file changed

+10
-4
lines changed

packages/dds/tree/src/shared-tree-core/editManager.ts

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -750,8 +750,8 @@ class SharedBranch<TEditor extends ChangeFamilyEditor, TChangeset> {
750750
// Local changes, i.e., changes from this client are applied by fast forwarding the local branch commit onto
751751
// the trunk.
752752
if (areLocalCommits) {
753-
for (const _ of newCommits) {
754-
this.sequenceLocalCommit(nextSequenceId, sessionId, onSequenceLocalCommit);
753+
for (const { revision } of newCommits) {
754+
this.sequenceLocalCommit(revision, nextSequenceId, sessionId, onSequenceLocalCommit);
755755
nextSequenceId = getNextSequenceId(nextSequenceId);
756756
}
757757
return;
@@ -900,10 +900,17 @@ class SharedBranch<TEditor extends ChangeFamilyEditor, TChangeset> {
900900
* Avoiding the overhead of the rebase process, even when it's a no-op, has real measured performance benefits and is worth the added complexity here.
901901
*/
902902
private sequenceLocalCommit(
903+
revision: RevisionTag,
903904
sequenceId: SequenceId,
904905
sessionId: SessionId,
905906
onSequenceLocalCommit: OnSequenceCommit<TChangeset>,
906-
): GraphCommit<TChangeset> {
907+
): void {
908+
if (this.commitMetadata.has(revision)) {
909+
// This can happen if the commit came from a shared branch and was concurrently merged by another client.
910+
// In this case, the newly sequenced commit is redundant and should therefore be ignored.
911+
return;
912+
}
913+
907914
// First, push the local commit to the trunk.
908915
// We are mutating our `localCommits` cache here,but there is no need to actually change the `localBranch` itself because it will simply catch up later if/when it next rebases.
909916
const firstLocalCommit = this.localCommits.shift();
@@ -915,7 +922,6 @@ class SharedBranch<TEditor extends ChangeFamilyEditor, TChangeset> {
915922
const prevSequenceId = this.getCommitSequenceId(this.trunk.getHead().revision);
916923
this.pushGraphCommitToTrunk(sequenceId, firstLocalCommit, sessionId);
917924
onSequenceLocalCommit(firstLocalCommit, sequenceId, prevSequenceId);
918-
return firstLocalCommit;
919925
}
920926

921927
/**

0 commit comments

Comments
 (0)