diff --git a/packages/dds/tree/src/shared-tree-core/editManager.ts b/packages/dds/tree/src/shared-tree-core/editManager.ts index 9ea6db5f35a5..8fc5f5a30c99 100644 --- a/packages/dds/tree/src/shared-tree-core/editManager.ts +++ b/packages/dds/tree/src/shared-tree-core/editManager.ts @@ -746,8 +746,8 @@ class SharedBranch { // Local changes, i.e., changes from this client are applied by fast forwarding the local branch commit onto // the trunk. if (areLocalCommits) { - for (const _ of newCommits) { - this.sequenceLocalCommit(nextSequenceId, sessionId, onSequenceLocalCommit); + for (const { revision } of newCommits) { + this.sequenceLocalCommit(revision, nextSequenceId, sessionId, onSequenceLocalCommit); nextSequenceId = getNextSequenceId(nextSequenceId); } return; @@ -896,10 +896,17 @@ class SharedBranch { * 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. */ private sequenceLocalCommit( + revision: RevisionTag, sequenceId: SequenceId, sessionId: SessionId, onSequenceLocalCommit: OnSequenceCommit, - ): GraphCommit { + ): void { + if (this.commitMetadata.has(revision)) { + // This can happen if the commit came from a shared branch and was concurrently merged by another client. + // In this case, the newly sequenced commit is redundant and should therefore be ignored. + return; + } + // First, push the local commit to the trunk. // 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. const firstLocalCommit = this.localCommits.shift(); @@ -911,7 +918,6 @@ class SharedBranch { const prevSequenceId = this.getCommitSequenceId(this.trunk.getHead().revision); this.pushGraphCommitToTrunk(sequenceId, firstLocalCommit, sessionId); onSequenceLocalCommit(firstLocalCommit, sequenceId, prevSequenceId); - return firstLocalCommit; } /** diff --git a/packages/dds/tree/src/shared-tree-core/sharedTreeCore.ts b/packages/dds/tree/src/shared-tree-core/sharedTreeCore.ts index dcc836db81b0..1384a726e72f 100644 --- a/packages/dds/tree/src/shared-tree-core/sharedTreeCore.ts +++ b/packages/dds/tree/src/shared-tree-core/sharedTreeCore.ts @@ -320,6 +320,7 @@ export class SharedTreeCore this.editManager.localSessionId, newRevision, this.detachedRevision, + branchId, ); this.editManager.advanceMinimumSequenceNumber(newRevision, false); return undefined; diff --git a/packages/dds/tree/src/test/shared-tree/fuzz/baseModel.ts b/packages/dds/tree/src/test/shared-tree/fuzz/baseModel.ts index a374e4a00004..6d176fd7439b 100644 --- a/packages/dds/tree/src/test/shared-tree/fuzz/baseModel.ts +++ b/packages/dds/tree/src/test/shared-tree/fuzz/baseModel.ts @@ -11,7 +11,11 @@ import type { Operation } from "./operationTypes.js"; import { takeAsync } from "@fluid-private/stochastic-test-utils"; import { type EditGeneratorOpWeights, makeOpGenerator } from "./fuzzEditGenerators.js"; import { FluidClientVersion } from "../../../codec/index.js"; -import { ForestTypeOptimized, ForestTypeReference } from "../../../shared-tree/index.js"; +import { + ForestTypeOptimized, + ForestTypeReference, + SharedTreeFormatVersion, +} from "../../../shared-tree/index.js"; export const runsPerBatch = 50; // TODO: Enable other types of ops. @@ -31,6 +35,10 @@ const editGeneratorOpWeights: Partial = { nodeConstraint: 3, fork: 1, merge: 1, + createSharedBranch: 6, + checkoutSharedBranch: 9, + checkoutMainBranch: 6, + mergeSharedBranch: 6, }; const generatorFactory = () => takeAsync(100, makeOpGenerator(editGeneratorOpWeights)); @@ -42,6 +50,7 @@ export const baseTreeModel: DDSFuzzModel< workloadName: "SharedTree (Reference Forest)", factory: new SharedTreeFuzzTestFactory(createOnCreate(undefined), undefined, { oldestCompatibleClient: FluidClientVersion.EnableUnstableFeatures, + formatVersion: SharedTreeFormatVersion.vSharedBranches, forest: ForestTypeReference, }), generatorFactory, diff --git a/packages/dds/tree/src/test/shared-tree/fuzz/composeVsIndividual.fuzz.spec.ts b/packages/dds/tree/src/test/shared-tree/fuzz/composeVsIndividual.fuzz.spec.ts index 414e774e1182..20be6ab0dca8 100644 --- a/packages/dds/tree/src/test/shared-tree/fuzz/composeVsIndividual.fuzz.spec.ts +++ b/packages/dds/tree/src/test/shared-tree/fuzz/composeVsIndividual.fuzz.spec.ts @@ -34,6 +34,7 @@ import { applyFieldEdit, applySynchronizationOp, applyUndoRedoEdit, + applySharedBranchOperation, } from "./fuzzEditReducers.js"; import { createOnCreate, @@ -88,6 +89,9 @@ const fuzzComposedVsIndividualReducer = combineReducers { return applyForkMergeOperation(state, operation); }, + sharedBranchOperation: (state, operation) => { + applySharedBranchOperation(state, operation); + }, }); /** diff --git a/packages/dds/tree/src/test/shared-tree/fuzz/fuzzEditGenerators.ts b/packages/dds/tree/src/test/shared-tree/fuzz/fuzzEditGenerators.ts index 822f5051b17f..e33d8904237a 100644 --- a/packages/dds/tree/src/test/shared-tree/fuzz/fuzzEditGenerators.ts +++ b/packages/dds/tree/src/test/shared-tree/fuzz/fuzzEditGenerators.ts @@ -5,6 +5,7 @@ import { strict as assert } from "node:assert"; +import { fail } from "@fluidframework/core-utils/internal"; import { type AsyncGenerator, type BaseFuzzTestState, @@ -55,6 +56,8 @@ import { GeneratedFuzzValueType, type NodeRange, type ForkMergeOperation, + type SharedBranchOperation, + type SharedBranchNumber, } from "./operationTypes.js"; // eslint-disable-next-line import/no-internal-modules import type { SchematizingSimpleTreeView } from "../../../shared-tree/schematizingTreeView.js"; @@ -121,6 +124,11 @@ export interface FuzzTestState extends DDSFuzzTestState; + + sharedBranchIdToNumber?: Map; + sharedBranchNumberToId?: Map; + activeSharedBranch?: Map; + sharedBranchViews?: Map>; } export function viewFromState( @@ -140,31 +148,39 @@ export function viewFromState( return forkedViews[forkedBranchIndex]; } - const view = - state.transactionViews?.get(client.channel) ?? - (getOrCreate(state.clientViews, client.channel, (sharedTree) => { - const tree = sharedTree.kernel; - const treeSchema = simpleSchemaFromStoredSchema(tree.storedSchema); - const config = new TreeViewConfiguration({ - schema: treeSchema, - }); + const transactionView = state.transactionViews?.get(client.channel); + if (transactionView !== undefined) { + return transactionView; + } - const treeView = asAlpha(tree.viewWith(config)); - treeView.events.on("schemaChanged", () => { - if (!treeView.compatibility.canView) { - treeView.dispose(); - state.clientViews?.delete(client.channel); - } - }); + const sharedBranchView = state.activeSharedBranch?.get(client.channel); + if (sharedBranchView !== undefined) { + return sharedBranchView.view; + } + + const view = getOrCreate(state.clientViews, client.channel, (sharedTree) => { + const tree = sharedTree.kernel; + const treeSchema = simpleSchemaFromStoredSchema(tree.storedSchema); + const config = new TreeViewConfiguration({ + schema: treeSchema, + }); + + const treeView = asAlpha(tree.viewWith(config)); + treeView.events.on("schemaChanged", () => { + if (!treeView.compatibility.canView) { + treeView.dispose(); + state.clientViews?.delete(client.channel); + } + }); - assert(treeView.compatibility.isEquivalent); - const fuzzView = treeView as FuzzView; - assert.equal(fuzzView.currentSchema, undefined); - const nodeSchema = nodeSchemaFromTreeSchema(treeSchema); + assert(treeView.compatibility.isEquivalent); + const fuzzView = treeView as FuzzView; + assert.equal(fuzzView.currentSchema, undefined); + const nodeSchema = nodeSchemaFromTreeSchema(treeSchema); - fuzzView.currentSchema = nodeSchema ?? assert.fail("nodeSchema should not be undefined"); - return fuzzView; - }) as unknown as FuzzView); + fuzzView.currentSchema = nodeSchema ?? assert.fail("nodeSchema should not be undefined"); + return fuzzView; + }) as unknown as FuzzView; return view; } function filterFuzzNodeSchemas( @@ -279,6 +295,11 @@ export interface EditGeneratorOpWeights { nodeConstraint: number; fork: number; merge: number; + + createSharedBranch: number; + checkoutSharedBranch: number; + checkoutMainBranch: number; + mergeSharedBranch: number; } const defaultEditGeneratorOpWeights: EditGeneratorOpWeights = { set: 0, @@ -298,6 +319,10 @@ const defaultEditGeneratorOpWeights: EditGeneratorOpWeights = { nodeConstraint: 0, fork: 0, merge: 0, + createSharedBranch: 0, + checkoutSharedBranch: 0, + checkoutMainBranch: 0, + mergeSharedBranch: 0, }; export interface EditGeneratorOptions { @@ -584,6 +609,65 @@ export const makeTransactionEditGenerator = ( ]); }; +export const makeSharedBranchOpGenerator = ( + opWeightsArg: Partial, +): Generator => { + const opWeights = { + ...defaultEditGeneratorOpWeights, + ...opWeightsArg, + }; + + return createWeightedGenerator([ + [ + (state) => ({ + type: "sharedBranchOperation", + contents: { + type: "createSharedBranch", + branchNumber: (state.sharedBranchIdToNumber?.size ?? 0) + 1, + }, + }), + opWeights.createSharedBranch, + ], + [ + (state) => ({ + type: "sharedBranchOperation", + contents: { + type: "checkoutSharedBranch", + branchNumber: + state.sharedBranchIdToNumber?.get( + state.random.pick(state.client.channel.getSharedBranchIds()), + ) ?? fail("Missing branch number"), + }, + }), + opWeights.checkoutSharedBranch, + (state) => state.client.channel.getSharedBranchIds().length > 0, + ], + [ + { + type: "sharedBranchOperation", + contents: { + type: "checkoutMainBranch", + }, + }, + opWeights.checkoutMainBranch, + (state) => state.activeSharedBranch?.get(state.client.channel) !== undefined, + ], + [ + (state) => ({ + type: "sharedBranchOperation", + contents: { + type: "mergeSharedBranch", + branchNumber: state.random.pick( + Array.from(state.sharedBranchViews?.get(state.client.channel)?.keys() ?? []), + ), + }, + }), + opWeights.mergeSharedBranch, + (state) => state.sharedBranchViews?.get(state.client.channel) !== undefined, + ], + ]); +}; + export const makeBranchEditGenerator = ( opWeightsArg: Partial, ): Generator => { @@ -720,6 +804,10 @@ export function makeOpGenerator( nodeConstraint, fork, merge, + checkoutMainBranch, + checkoutSharedBranch, + createSharedBranch, + mergeSharedBranch, ...others } = weights; // This assert will trigger when new weights are added to EditGeneratorOpWeights but this function has not been @@ -728,6 +816,12 @@ export function makeOpGenerator( const editWeight = sumWeights([insert, remove, intraFieldMove, crossFieldMove, set, clear]); const transactionWeight = sumWeights([abort, commit, start]); const undoRedoWeight = sumWeights([undo, redo]); + const sharedBranchOpWeight = sumWeights([ + checkoutMainBranch, + checkoutSharedBranch, + createSharedBranch, + mergeSharedBranch, + ]); // Currently we only support node constraints, but this may be expanded in the future. const constraintWeight = nodeConstraint; @@ -750,6 +844,7 @@ export function makeOpGenerator( (state: FuzzTestState) => viewFromState(state).checkout.transaction.isInProgress(), ], [() => makeBranchEditGenerator(weights), weights.fork + weights.merge], + [() => makeSharedBranchOpGenerator(weights), sharedBranchOpWeight], ] as const ) .filter(([, weight]) => weight > 0) diff --git a/packages/dds/tree/src/test/shared-tree/fuzz/fuzzEditReducers.ts b/packages/dds/tree/src/test/shared-tree/fuzz/fuzzEditReducers.ts index 7757affd9059..cc0aee4af4ec 100644 --- a/packages/dds/tree/src/test/shared-tree/fuzz/fuzzEditReducers.ts +++ b/packages/dds/tree/src/test/shared-tree/fuzz/fuzzEditReducers.ts @@ -3,7 +3,7 @@ * Licensed under the MIT License. */ -import { strict as assert } from "node:assert"; +import { strict as assert, fail } from "node:assert"; import { type Reducer, combineReducers } from "@fluid-private/stochastic-test-utils"; import type { DDSFuzzTestState, Client } from "@fluid-private/test-dds-utils"; @@ -50,6 +50,8 @@ import { type NodeObjectValue, type GUIDNodeValue, type ForkMergeOperation, + type SharedBranchOperation, + type SharedBranchNumber, } from "./operationTypes.js"; import { getOrCreateInnerNode } from "../../../simple-tree/index.js"; @@ -99,11 +101,17 @@ const syncFuzzReducer = combineReducers< forkMergeOperation: (state, operation) => { applyForkMergeOperation(state, operation); }, + sharedBranchOperation: (state, operation) => { + applySharedBranchOperation(state, operation); + }, }); export const fuzzReducer: Reducer< Operation, DDSFuzzTestState> -> = (state, operation) => syncFuzzReducer(state, operation); +> = (state, operation) => { + // This is a good place to put a breakpoint when debugging a scenario + return syncFuzzReducer(state, operation); +}; export function checkTreesAreSynchronized( trees: readonly Client>[], @@ -193,6 +201,78 @@ export function applySchemaOp(state: FuzzTestState, operation: SchemaChange) { state.transactionViews = transactionViews; } +export function applySharedBranchOperation( + state: FuzzTestState, + sharedBranchOp: SharedBranchOperation, +) { + state.sharedBranchIdToNumber ??= new Map(); + state.sharedBranchNumberToId ??= new Map(); + state.activeSharedBranch ??= new Map(); + state.sharedBranchViews ??= new Map(); + + switch (sharedBranchOp.contents.type) { + case "createSharedBranch": { + const branchId = state.client.channel.createSharedBranch(); + state.sharedBranchIdToNumber.set(branchId, sharedBranchOp.contents.branchNumber); + state.sharedBranchNumberToId.set(sharedBranchOp.contents.branchNumber, branchId); + break; + } + case "checkoutSharedBranch": { + const branchNumber = sharedBranchOp.contents.branchNumber; + const branchId = + state.sharedBranchNumberToId.get(branchNumber) ?? fail("Branch does not exist"); + let sharedViewMap = state.sharedBranchViews.get(state.client.channel); + if (sharedViewMap === undefined) { + sharedViewMap = new Map(); + state.sharedBranchViews.set(state.client.channel, sharedViewMap); + } + let branchView = sharedViewMap.get(branchNumber); + if (branchView === undefined) { + const mainBranchView = viewFromState(state, state.client); + branchView = state.client.channel.viewSharedBranchWith( + branchId, + new TreeViewConfiguration({ schema: mainBranchView.schema }), + ) as FuzzView; + assert.equal(branchView.currentSchema, undefined); + const treeSchema = mainBranchView.currentSchema; + branchView.currentSchema = + treeSchema ?? assert.fail("nodeSchema should not be undefined"); + + convertToFuzzView(branchView, branchView.currentSchema); + sharedViewMap.set(branchNumber, branchView); + } + state.activeSharedBranch.set(state.client.channel, { + branchNumber, + view: branchView, + }); + break; + } + case "checkoutMainBranch": { + const isRemovalEffective = state.activeSharedBranch.delete(state.client.channel); + assert(isRemovalEffective, "Client should have a shared branch checked out."); + // Note that we do not delete the shared branch view from state.sharedBranchViews here, + // as the client may check out this branch again later (and SharedTree does not support checking out the same shared branch multiple times). + break; + } + case "mergeSharedBranch": { + const mainBranchView = + state.clientViews?.get(state.client.channel) ?? + fail("Client main view should be defined."); + // We only allow merging shared branches that have already been checked out by the client. + const branchView = + state.sharedBranchViews + .get(state.client.channel) + ?.get(sharedBranchOp.contents.branchNumber) ?? + fail("Branch view should be defined."); + + mainBranchView.merge(branchView, false); + break; + } + default: + break; + } +} + export function applyForkMergeOperation(state: FuzzTestState, branchEdit: ForkMergeOperation) { switch (branchEdit.contents.type) { case "fork": { diff --git a/packages/dds/tree/src/test/shared-tree/fuzz/operationTypes.ts b/packages/dds/tree/src/test/shared-tree/fuzz/operationTypes.ts index da02005b9db0..72f80bf7705f 100644 --- a/packages/dds/tree/src/test/shared-tree/fuzz/operationTypes.ts +++ b/packages/dds/tree/src/test/shared-tree/fuzz/operationTypes.ts @@ -15,7 +15,8 @@ export type TreeOperation = | UndoRedo | SchemaChange | Constraint - | ForkMergeOperation; + | ForkMergeOperation + | SharedBranchOperation; export interface TreeEdit { type: "treeEdit"; @@ -53,6 +54,46 @@ export interface ForkMergeOperation { contents: ForkBranch | MergeBranch; } +export interface SharedBranchOperation { + type: "sharedBranchOperation"; + contents: CreateSharedBranch | CheckoutSharedBranch | MergeSharedBranch | CheckoutMainBranch; +} + +/** + * An ordinal number associated with a shared branch created during a test, used in operations to refer to that branch. + * This is different from the branch ID used internally by SharedTree. + * The reason this is used instead of branch IDs is that branch IDs are not stable, which in some cases prevents replay and minification. + * SharedBranchNumber also has the advantage that it can be decided at operation generation time, which allows us to include it in CreateSharedBranch and helps test log readability. + */ +export type SharedBranchNumber = number; + +export interface CreateSharedBranch { + type: "createSharedBranch"; + branchNumber: SharedBranchNumber; +} + +export interface CheckoutSharedBranch { + type: "checkoutSharedBranch"; + branchNumber: SharedBranchNumber; +} + +/** + * Only allowed on clients which have another shared branch checked out. + */ +export interface CheckoutMainBranch { + type: "checkoutMainBranch"; +} + +/** + * Always merges the shared branch into the main branch. + * Only allowed on clients which have checked out this shared branch in the past. + * This includes clients that do not currently have that shared branch checked out. + */ +export interface MergeSharedBranch { + type: "mergeSharedBranch"; + branchNumber: SharedBranchNumber; +} + export interface ForkBranch { type: "fork"; /**